diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 623b3db448e..0e5802ec146 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -12,6 +12,7 @@ This skill is a dispatcher: identify the task type, load the matching `.github/a Read only the files you need: Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/agentic-chat.md` +- `.github/aw/agentic-workflows-mcp.md` - `.github/aw/asciicharts.md` - `.github/aw/campaign.md` - `.github/aw/charts-trending.md` @@ -27,6 +28,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` - `.github/aw/llms.md` +- `.github/aw/mcp-clis.md` - `.github/aw/memory.md` - `.github/aw/messages.md` - `.github/aw/network.md` diff --git a/actions/setup/js/collect_ndjson_output.cjs b/actions/setup/js/collect_ndjson_output.cjs index 2ef53d845d4..d7bab7f759b 100644 --- a/actions/setup/js/collect_ndjson_output.cjs +++ b/actions/setup/js/collect_ndjson_output.cjs @@ -323,8 +323,12 @@ async function main() { } continue; } - // Update item with normalized values - Object.assign(item, validationResult.normalizedItem); + // SECURITY: Use normalizedItem (which strips infrastructure-only fields + // like patch_path, bundle_path, base_commit, diff_size) instead of the + // original item, to prevent agent-injected transport metadata from + // reaching the privileged handler. + core.info(`Line ${i + 1}: Valid ${itemType} item`); + parsedItems.push(validationResult.normalizedItem); } else { // Fall back to validateItemWithSafeJobConfig for unknown types const jobOutputType = expectedOutputTypes[itemType]; @@ -341,10 +345,9 @@ async function main() { } Object.assign(item, validation.normalizedItem); } + core.info(`Line ${i + 1}: Valid ${itemType} item`); + parsedItems.push(item); } - - core.info(`Line ${i + 1}: Valid ${itemType} item`); - parsedItems.push(item); } catch (error) { const errorMsg = getErrorMessage(error); errors.push(`Line ${i + 1}: Invalid JSON - ${errorMsg}`); diff --git a/actions/setup/js/generate_git_patch.cjs b/actions/setup/js/generate_git_patch.cjs index 45339dd9d68..a98eb421044 100644 --- a/actions/setup/js/generate_git_patch.cjs +++ b/actions/setup/js/generate_git_patch.cjs @@ -49,6 +49,9 @@ function debugLog(message) { * @param {string[]} [options.excludedFiles] - Glob patterns for files to exclude from the patch. * Each pattern is passed to `git format-patch` as a `:(exclude)` magic pathspec so * matching files are never included in the generated patch. + * @param {string} [options.pinnedSha] - SECURITY: When set, use this SHA as the branch tip instead + * of resolving refs/heads/. Prevents TOCTOU races where the agent flips the branch + * ref between patch and bundle generation. * @returns {Promise} Object with patch info or error */ async function generateGitPatch(branchName, baseBranch, options = {}) { @@ -130,15 +133,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { // can use it directly. The From header in format-patch output contains the // *new* commit SHA which won't exist in the target checkout. let baseCommitSha = null; + let resolvedTipRef = null; try { // Strategy 1: If we have a branch name, check if that branch exists and get its diff if (branchName) { - debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`); - // Check if the branch exists locally + resolvedTipRef = options.pinnedSha || branchName; + // SECURITY: When pinnedSha is provided, use it directly as the tip commit instead + // of dereferencing refs/heads/. This prevents TOCTOU races where the + // agent can flip the branch ref between patch and bundle generation. + const tipRef = resolvedTipRef; try { - execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd }); - debugLog(`Strategy 1: Branch '${branchName}' exists locally`); + if (options.pinnedSha) { + debugLog(`Strategy 1: Using pinned SHA ${options.pinnedSha} (branch: ${branchName})`); + } else { + debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`); + // Check if the branch exists locally + execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd }); + debugLog(`Strategy 1: Branch '${branchName}' exists locally`); + } // Determine base ref for patch generation let baseRef; @@ -251,7 +264,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { } if (defaultBranchRef) { - baseRef = execGitSync(["merge-base", "--", defaultBranchRef, branchName], { cwd }).trim(); + baseRef = execGitSync(["merge-base", "--", defaultBranchRef, tipRef], { cwd }).trim(); debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`); } else { // No remote refs available - fall through to Strategy 2 @@ -265,12 +278,12 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { debugLog(`Strategy 1: Resolved baseRef ${baseRef} to SHA ${baseCommitSha}`); // Count commits to be included - const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10); - debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${branchName}`); + const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${tipRef}`], { cwd }).trim(), 10); + debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${tipRef}`); if (commitCount > 0) { // Generate patch from the determined base to the branch - const patchContent = execGitSync(["format-patch", `${baseRef}..${branchName}`, "--stdout", ...excludeArgs()], { cwd }); + const patchContent = execGitSync(["format-patch", `${baseRef}..${tipRef}`, "--stdout", ...excludeArgs()], { cwd }); if (patchContent && patchContent.trim()) { fs.writeFileSync(patchPath, patchContent, "utf8"); @@ -299,16 +312,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { // error preserves the "incremental" contract that the patch reflects only // the new commits. if (!patchGenerated && mode === "incremental") { - debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`); + debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`); return { success: false, - error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s).`, + error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s).`, patchPath: patchPath, }; } } catch (branchError) { - // Branch does not exist locally + // Branch does not exist locally (or pinnedSha failed) debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`); + if (options.pinnedSha) { + // SECURITY: When pinnedSha is set, fail closed — do not fall through to + // other strategies that would resolve a different commit. + return { + success: false, + error: `Pinned SHA ${options.pinnedSha} failed to generate patch: ${getErrorMessage(branchError)}`, + patchPath: patchPath, + }; + } if (mode === "incremental") { return { success: false, @@ -510,7 +532,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { // of the PR head. Use the merge-base as the effective diff base to exclude those // merged upstream commits from the size measurement. let diffBaseForSize = baseCommitSha; - if (mode === "incremental" && baseCommitSha && branchName && defaultBranch) { + if (mode === "incremental" && baseCommitSha && resolvedTipRef && defaultBranch) { try { let baseBranchRemoteRef = null; try { @@ -527,15 +549,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { // baseCommitSha as the diff base. let baseIsAncestorOfBranch = false; try { - execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, branchName], { cwd }); + execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, resolvedTipRef], { cwd }); baseIsAncestorOfBranch = true; } catch { - // baseCommitSha is not an ancestor of branchName (rebase / force-push) - debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${branchName} (rebase/force-push?); skipping merge-base adjustment`); + // baseCommitSha is not an ancestor of tipRef (rebase / force-push) + debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${resolvedTipRef} (rebase/force-push?); skipping merge-base adjustment`); } if (baseIsAncestorOfBranch) { - const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, branchName], { cwd }).trim(); + const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, resolvedTipRef], { cwd }).trim(); // Check if mb is already an ancestor of baseCommitSha. // If it is, baseCommitSha is "later" and the agent did NOT merge the default // branch ahead of the PR head — keep baseCommitSha as the diff base. @@ -561,15 +583,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { } let diffSize = null; - if (mode === "incremental" && diffBaseForSize && branchName) { + if (mode === "incremental" && diffBaseForSize && resolvedTipRef) { diffSize = computeIncrementalDiffSize({ baseRef: diffBaseForSize, - headRef: branchName, + headRef: resolvedTipRef, cwd, tmpPath: `${patchPath}.diff.tmp`, excludedFiles: options.excludedFiles, }); - debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${branchName})`); + debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${resolvedTipRef})`); } debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}, diffSize=${diffSize ?? "(n/a)"} bytes, baseCommit=${baseCommitSha || "(unknown)"}`); diff --git a/actions/setup/js/safe_output_type_validator.cjs b/actions/setup/js/safe_output_type_validator.cjs index d9de7359671..0196d83d4b5 100644 --- a/actions/setup/js/safe_output_type_validator.cjs +++ b/actions/setup/js/safe_output_type_validator.cjs @@ -545,6 +545,14 @@ function validateItem(item, itemType, lineNum, options) { } const normalizedItem = { ...item }; + // SECURITY: Strip infrastructure fields that must only be set by the MCP handler, + // never by the agent. If an agent injects these via NDJSON output, it could bypass + // file-protection policy (patch_path/bundle_path point to attacker-controlled files) + // or circumvent size limits (diff_size). + delete normalizedItem.patch_path; + delete normalizedItem.bundle_path; + delete normalizedItem.base_commit; + delete normalizedItem.diff_size; const errors = []; // Run custom validation first if defined diff --git a/actions/setup/js/safe_output_type_validator.test.cjs b/actions/setup/js/safe_output_type_validator.test.cjs index dfb92016c4e..1b9dfb9abf1 100644 --- a/actions/setup/js/safe_output_type_validator.test.cjs +++ b/actions/setup/js/safe_output_type_validator.test.cjs @@ -803,4 +803,111 @@ describe("safe_output_type_validator", () => { expect(result.error).toContain("must contain only strings"); }); }); + + describe("infrastructure field stripping (security)", () => { + it("should strip patch_path from normalizedItem", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_pull_request", + title: "Fix bug", + body: "Fixes the thing", + branch: "fix/bug", + patch_path: "/tmp/evil.patch", + }; + + const result = validateItem(item, "create_pull_request", 1); + expect(result.isValid).toBe(true); + expect(result.normalizedItem).not.toHaveProperty("patch_path"); + }); + + it("should strip bundle_path from normalizedItem", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_pull_request", + title: "Fix bug", + body: "Fixes the thing", + branch: "fix/bug", + bundle_path: "/tmp/evil.bundle", + }; + + const result = validateItem(item, "create_pull_request", 1); + expect(result.isValid).toBe(true); + expect(result.normalizedItem).not.toHaveProperty("bundle_path"); + }); + + it("should strip base_commit from normalizedItem", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_pull_request", + title: "Fix bug", + body: "Fixes the thing", + branch: "fix/bug", + base_commit: "abc123deadbeef", + }; + + const result = validateItem(item, "create_pull_request", 1); + expect(result.isValid).toBe(true); + expect(result.normalizedItem).not.toHaveProperty("base_commit"); + }); + + it("should strip diff_size from normalizedItem", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_pull_request", + title: "Fix bug", + body: "Fixes the thing", + branch: "fix/bug", + diff_size: 1, + }; + + const result = validateItem(item, "create_pull_request", 1); + expect(result.isValid).toBe(true); + expect(result.normalizedItem).not.toHaveProperty("diff_size"); + }); + + it("should strip all infrastructure fields simultaneously", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_pull_request", + title: "Fix bug", + body: "Fixes the thing", + branch: "fix/bug", + patch_path: "/tmp/evil.patch", + bundle_path: "/tmp/evil.bundle", + base_commit: "abc123", + diff_size: 999999, + }; + + const result = validateItem(item, "create_pull_request", 1); + expect(result.isValid).toBe(true); + expect(result.normalizedItem).not.toHaveProperty("patch_path"); + expect(result.normalizedItem).not.toHaveProperty("bundle_path"); + expect(result.normalizedItem).not.toHaveProperty("base_commit"); + expect(result.normalizedItem).not.toHaveProperty("diff_size"); + // Legitimate fields should still be present + expect(result.normalizedItem.title).toBe("Fix bug"); + expect(result.normalizedItem.branch).toBe("fix/bug"); + }); + + it("should preserve non-infrastructure undeclared fields", async () => { + const { validateItem } = await import("./safe_output_type_validator.cjs"); + + const item = { + type: "create_issue", + title: "Test", + body: "Body text", + metadata: { project: "test" }, + }; + + const result = validateItem(item, "create_issue", 1); + expect(result.isValid).toBe(true); + // Non-infrastructure extra fields should pass through + expect(result.normalizedItem.metadata).toEqual({ project: "test" }); + }); + }); }); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 190d501808b..da0128e100c 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -12,7 +12,7 @@ const { getCurrentBranch } = require("./get_current_branch.cjs"); const { getBaseBranch } = require("./get_base_branch.cjs"); const { generateGitPatch } = require("./generate_git_patch.cjs"); const { generateGitBundle } = require("./generate_git_bundle.cjs"); -const { hasMergeCommitsInRange } = require("./git_helpers.cjs"); +const { hasMergeCommitsInRange, execGitSync } = require("./git_helpers.cjs"); const { enforceCommentLimits } = require("./comment_limit_helpers.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_codes.cjs"); @@ -554,6 +554,36 @@ function createHandlers(server, appendSafeOutput, config = {}) { transportOptions.token = prConfig["github-token"]; } + // SECURITY: Pin the branch ref to a SHA before generating any transport artifacts. + // This prevents TOCTOU races where the agent flips the ref between patch and bundle + // generation, causing the two to represent different commit sets. + const gitCwd = repoCwd || process.env.GITHUB_WORKSPACE || process.cwd(); + let pinnedSha; + try { + pinnedSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: gitCwd }) + .toString() + .trim(); + server.debug(`Pinned branch '${entry.branch}' to SHA ${pinnedSha}`); + } catch (pinError) { + server.debug(`Failed to pin branch '${entry.branch}': ${getErrorMessage(pinError)}`); + if (useBundle) { + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: `Failed to pin branch '${entry.branch}' before bundle generation: ${getErrorMessage(pinError)}`, + details: `Bundle transport requires branch pinning to prevent patch/bundle desynchronization. Retry after ensuring the branch exists locally (for example: git branch --list '${entry.branch}').`, + }), + }, + ], + isError: true, + }; + } + pinnedSha = null; + } + // Always generate a patch for policy enforcement (allowed-files/protected-files/excluded-files), // even when bundle transport is selected for apply-time commit transport. server.debug(`Generating patch for create_pull_request with branch: ${entry.branch}${repoCwd ? ` in ${repoCwd} baseBranch: ${baseBranch}` : ""}`); @@ -566,6 +596,10 @@ function createHandlers(server, appendSafeOutput, config = {}) { if (Array.isArray(prConfig.excluded_files) && prConfig.excluded_files.length > 0) { patchOptions.excludedFiles = prConfig.excluded_files; } + // Pass pinnedSha so patch generation uses the pinned commit, not a potentially-flipped ref + if (pinnedSha) { + patchOptions.pinnedSha = pinnedSha; + } const patchResult = await generateGitPatch(entry.branch, baseBranch, patchOptions); if (!patchResult.success) { @@ -628,6 +662,47 @@ function createHandlers(server, appendSafeOutput, config = {}) { server.debug(`Bundle generated successfully: ${bundleResult.bundlePath} (${bundleResult.bundleSize} bytes)`); + // SECURITY: Verify the branch ref hasn't been flipped between patch and bundle + // generation (TOCTOU check). If the SHA changed, the bundle may contain different + // commits than the patch used for file-protection policy enforcement. + if (pinnedSha) { + try { + const currentSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: gitCwd }) + .toString() + .trim(); + if (currentSha !== pinnedSha) { + server.debug(`SECURITY: Branch '${entry.branch}' SHA changed during transport generation (was ${pinnedSha}, now ${currentSha}). Aborting.`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: "Branch ref changed during transport artifact generation. This may indicate a concurrent modification. Please retry.", + details: `Branch '${entry.branch}' pointed to ${pinnedSha} at start but ${currentSha} after bundle generation.`, + }), + }, + ], + isError: true, + }; + } + } catch (verifyError) { + server.debug(`SECURITY: Failed to verify branch SHA after bundle generation: ${getErrorMessage(verifyError)}`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: `Failed to verify branch integrity after bundle generation: ${getErrorMessage(verifyError)}`, + }), + }, + ], + isError: true, + }; + } + } + // Store the bundle path in the entry so consumers know which file to use entry.bundle_path = bundleResult.bundlePath; @@ -876,6 +951,36 @@ function createHandlers(server, appendSafeOutput, config = {}) { pushTransportOptions.token = pushConfig["github-token"]; } + // SECURITY: Pin the branch ref to a SHA before generating any transport artifacts. + // This prevents TOCTOU races where the agent flips the ref between patch and bundle + // generation, causing the two to represent different commit sets. + const pushGitCwd = repoCwd || process.env.GITHUB_WORKSPACE || process.cwd(); + let pushPinnedSha; + try { + pushPinnedSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: pushGitCwd }) + .toString() + .trim(); + server.debug(`Pinned branch '${entry.branch}' to SHA ${pushPinnedSha}`); + } catch (pinError) { + server.debug(`Failed to pin branch '${entry.branch}': ${getErrorMessage(pinError)}`); + if (useBundle) { + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: `Failed to pin branch '${entry.branch}' before bundle generation: ${getErrorMessage(pinError)}`, + details: `Bundle transport requires branch pinning to prevent patch/bundle desynchronization. Retry after ensuring the branch exists locally (for example: git branch --list '${entry.branch}').`, + }), + }, + ], + isError: true, + }; + } + pushPinnedSha = null; + } + // Always generate an incremental patch for policy enforcement (allowed-files/protected-files/excluded-files), // even when bundle transport is selected for apply-time commit transport. server.debug(`Generating incremental patch for push_to_pull_request_branch with branch: ${entry.branch}, baseBranch: ${baseBranch}`); @@ -888,6 +993,10 @@ function createHandlers(server, appendSafeOutput, config = {}) { if (Array.isArray(pushConfig.excluded_files) && pushConfig.excluded_files.length > 0) { pushPatchOptions.excludedFiles = pushConfig.excluded_files; } + // Pass pinnedSha so patch generation uses the pinned commit, not a potentially-flipped ref + if (pushPinnedSha) { + pushPatchOptions.pinnedSha = pushPinnedSha; + } const patchResult = await generateGitPatch(entry.branch, baseBranch, pushPatchOptions); if (!patchResult.success) { @@ -958,6 +1067,46 @@ function createHandlers(server, appendSafeOutput, config = {}) { server.debug(`Bundle generated successfully: ${bundleResult.bundlePath} (${bundleResult.bundleSize} bytes)`); + // SECURITY: Verify the branch ref hasn't been flipped between patch and bundle + // generation (TOCTOU check). + if (pushPinnedSha) { + try { + const currentSha = execGitSync(["rev-parse", "--verify", `refs/heads/${entry.branch}^{commit}`], { cwd: pushGitCwd }) + .toString() + .trim(); + if (currentSha !== pushPinnedSha) { + server.debug(`SECURITY: Branch '${entry.branch}' SHA changed during transport generation (was ${pushPinnedSha}, now ${currentSha}). Aborting.`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: "Branch ref changed during transport artifact generation. This may indicate a concurrent modification. Please retry.", + details: `Branch '${entry.branch}' pointed to ${pushPinnedSha} at start but ${currentSha} after bundle generation.`, + }), + }, + ], + isError: true, + }; + } + } catch (verifyError) { + server.debug(`SECURITY: Failed to verify branch SHA after bundle generation: ${getErrorMessage(verifyError)}`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: `Failed to verify branch integrity after bundle generation: ${getErrorMessage(verifyError)}`, + }), + }, + ], + isError: true, + }; + } + } + // Store the bundle path in the entry so consumers know which file to use entry.bundle_path = bundleResult.bundlePath; diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index f39f613377d..2d58a48d1ef 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -601,10 +601,10 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); expect(responseData.error).toBeDefined(); - expect(responseData.error).toContain("Failed to generate patch"); + expect(responseData.error).toContain("Failed to pin branch"); + expect(responseData.error).toContain("before bundle generation"); expect(responseData.details).toBeDefined(); - expect(responseData.details).toContain("Make sure you have committed your changes"); - expect(responseData.details).toContain("git add and git commit"); + expect(responseData.details).toContain("Bundle transport requires branch pinning"); // Should not have appended to safe output since patch generation failed expect(mockAppendSafeOutput).not.toHaveBeenCalled(); @@ -623,10 +623,8 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); // Verify the details provide actionable guidance - expect(responseData.details).toContain("create a pull request"); - expect(responseData.details).toContain("git add"); - expect(responseData.details).toContain("git commit"); - expect(responseData.details).toContain("create_pull_request"); + expect(responseData.details).toContain("Bundle transport requires branch pinning"); + expect(responseData.details).toContain("branch exists locally"); }); it("should return error when repo parameter is not in the allowed-repos list", async () => { @@ -663,7 +661,7 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); // Should be a patch generation error, not a repo not found error expect(responseData.error).not.toContain("not found in workspace"); - expect(responseData.error).toContain("Failed to generate patch"); + expect(responseData.error).toContain("Failed to pin branch"); }); it("should treat whitespace-only repo as workspace root", async () => { @@ -1062,10 +1060,9 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); expect(responseData.error).toBeDefined(); - expect(responseData.error).toContain("does not exist locally"); + expect(responseData.error).toContain("Failed to pin branch"); expect(responseData.details).toBeDefined(); - expect(responseData.details).toContain("push to the pull request branch"); - expect(responseData.details).toContain("git add and git commit"); + expect(responseData.details).toContain("Bundle transport requires branch pinning"); // Should not have appended to safe output since patch generation failed expect(mockAppendSafeOutput).not.toHaveBeenCalled(); @@ -1096,10 +1093,8 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); // Verify the details provide actionable guidance - expect(responseData.details).toContain("push to the pull request branch"); - expect(responseData.details).toContain("git add"); - expect(responseData.details).toContain("git commit"); - expect(responseData.details).toContain("push_to_pull_request_branch"); + expect(responseData.details).toContain("Bundle transport requires branch pinning"); + expect(responseData.details).toContain("branch exists locally"); }); it("should return error when repo checkout is not found for explicit repo", async () => {