From 229a2aa0c0adc4d2e524dd0b3bed4f91ac2a69a9 Mon Sep 17 00:00:00 2001 From: Evanfeenstra Date: Tue, 15 Sep 2026 12:04:47 -0700 Subject: [PATCH 1/2] Ephemeral previews: capture the worktree diff, guard the silent tool-loop exit, withhold web_search on confined runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit propose_code_change preview runs were coming back "completed" with the model's narration as the answer and no diff, while the edit had actually landed in the worktree. Root cause, confirmed from swarm38 logs: the Bifrost gateway rewrites the web_search_20250305 server tool to a newer version, the Anthropic API then implicitly offers code_execution, Sonnet answers with a server_tool_use the SDK never registered, `ai` marks the call invalid with a tool-error that lacks providerExecuted, the loop's client call/output counts disagree, and streamText exits with no error and no stop condition. get_context saw a last step with tool calls, skipped the stall nudge, fell through to extractFinalAnswer's all-text fallback, and reported success. Three changes, each sufficient on its own for the observed failure: - tools.ts: web_search is withheld on confined runs (create_pr worktrees and ephemeral previews). Those runs edit files and have no use for it, and with no server tool in the request there is nothing for a gateway to rewrite. - agent.ts / utils.ts: get_context detects a last step with unresolved tool calls (unresolvedToolCalls: invalid, unregistered, or without an output), retries once with those calls stripped from the transcript (stripToolCallParts — the API rejects a tool_result for a server tool it never ran) and a nudge naming the unavailable tool, and throws if the retry ends the same way. Two related holes closed on the same path: a mid-run abort now surfaces as AbortError instead of a completed run, and exhausted stall/length continuations set `incomplete` on the ContextResult so a caller that needs a deliverable can refuse it. - git_pr.ts / index.ts: captureWorktreeDiff stages the ephemeral worktree (git add -A, so new files are included — `git diff HEAD` alone omits them) and returns `git diff --cached`, with landChange's caps and gitleaks fail-closed scan. The non-stream terminal result carries it as `diff` / `diff_files` / `diff_error`, next to `pr`, so the caller no longer depends on the model pasting a diff into its final message. Tests: unresolved_tools.test.ts (helpers, including the exact production step shape), worktree_diff.test.ts (real git fixtures), and confinement tests asserting web_search is absent on ephemeral and prMode runs. The streaming path is untouched; Hive uses the non-stream path. --- mcp/src/repo/__tests__/confinement.test.ts | 35 ++++ .../repo/__tests__/unresolved_tools.test.ts | 140 +++++++++++++++ mcp/src/repo/__tests__/worktree_diff.test.ts | 167 ++++++++++++++++++ mcp/src/repo/agent.ts | 140 ++++++++++++--- mcp/src/repo/git_pr.ts | 124 +++++++++++++ mcp/src/repo/index.ts | 38 +++- mcp/src/repo/tools.ts | 11 +- mcp/src/repo/utils.ts | 63 +++++++ mcp/src/tools/types.ts | 6 + 9 files changed, 696 insertions(+), 28 deletions(-) create mode 100644 mcp/src/repo/__tests__/unresolved_tools.test.ts create mode 100644 mcp/src/repo/__tests__/worktree_diff.test.ts diff --git a/mcp/src/repo/__tests__/confinement.test.ts b/mcp/src/repo/__tests__/confinement.test.ts index 8b7128588..71d68965a 100644 --- a/mcp/src/repo/__tests__/confinement.test.ts +++ b/mcp/src/repo/__tests__/confinement.test.ts @@ -361,3 +361,38 @@ describe("acquireEphemeralWorktree", () => { if (!result.ok) assert.match(result.error, /Path traversal rejected/); }); }); + +// --------------------------------------------------------------------------- +// 5. web_search withheld on confined runs +// --------------------------------------------------------------------------- + +describe("web_search server tool on confined runs", () => { + async function buildTools(options: GetToolsOptions | undefined) { + return get_tools( + baseCloneDir, + "fake-api-key", + FAKE_PAT, + { bash: true, web_search: true }, + "anthropic", + undefined, undefined, undefined, undefined, undefined, undefined, + undefined, undefined, undefined, undefined, undefined, undefined, + options, + ); + } + + it("unconfined run: web_search is registered", async () => { + const tools = await buildTools(undefined); + assert.ok((tools as any).web_search, "web_search must be present on an unconfined anthropic run"); + }); + + it("ephemeral run: no web_search — a server tool in the request is what let a gateway's tool rewrite surface an unregistered code_execution call", async () => { + const tools = await buildTools({ ephemeral: true, baseCheckoutPath: "/tmp/owner/repo" }); + assert.equal((tools as any).web_search, undefined); + assert.ok((tools as any).bash?.execute, "bash stays available"); + }); + + it("prMode run: no web_search", async () => { + const tools = await buildTools(PR_MODE_OPTS(baseCloneDir)); + assert.equal((tools as any).web_search, undefined); + }); +}); diff --git a/mcp/src/repo/__tests__/unresolved_tools.test.ts b/mcp/src/repo/__tests__/unresolved_tools.test.ts new file mode 100644 index 000000000..ef29140e5 --- /dev/null +++ b/mcp/src/repo/__tests__/unresolved_tools.test.ts @@ -0,0 +1,140 @@ +/** + * Tests for the tool-loop exit guard (utils.ts): + * unresolvedToolCalls — which tool calls on a step the loop could not resolve + * stripToolCallParts — removing those calls from a replayable transcript + * + * Background: the AI SDK ends its tool loop on a step whose client-side call + * and output counts disagree, without an error or a stop condition. Seen in + * production when an LLM gateway rewrote a server tool's version and the API + * answered with a server_tool_use (code_execution) the run never registered. + * + * Harness: node:test (tsx --test). Pure functions, no fixtures. + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import type { ModelMessage, StepResult, ToolSet } from "ai"; + +import { unresolvedToolCalls, stripToolCallParts } from "../utils.js"; + +const TOOLS = ["bash", "web_search"]; + +function step(content: unknown[]): StepResult { + return { content, finishReason: "tool-calls" } as unknown as StepResult; +} + +describe("unresolvedToolCalls", () => { + it("returns nothing for an undefined step", () => { + assert.deepEqual(unresolvedToolCalls(undefined, TOOLS), []); + }); + + it("a registered tool with a result is resolved", () => { + const s = step([ + { type: "tool-call", toolCallId: "a", toolName: "bash", input: {} }, + { type: "tool-result", toolCallId: "a", toolName: "bash", output: "ok" }, + ]); + assert.deepEqual(unresolvedToolCalls(s, TOOLS), []); + }); + + it("a registered tool whose execution errored is still resolved", () => { + const s = step([ + { type: "tool-call", toolCallId: "a", toolName: "bash", input: {} }, + { type: "tool-error", toolCallId: "a", toolName: "bash", error: "boom" }, + ]); + assert.deepEqual(unresolvedToolCalls(s, TOOLS), []); + }); + + it("the production shape: an invalid provider-executed call next to a resolved bash call", () => { + // What the SDK recorded on 2026-09-15: code_execution (never registered) + // marked invalid with a tool-error, plus the bash edit that did run. + const s = step([ + { type: "text", text: "Now let's apply the edit." }, + { + type: "tool-call", + toolCallId: "srv_1", + toolName: "code_execution", + input: { type: "text_editor_code_execution", command: "str_replace" }, + providerExecuted: true, + dynamic: true, + invalid: true, + }, + { type: "tool-error", toolCallId: "srv_1", toolName: "code_execution", error: "No such tool", dynamic: true }, + { type: "tool-call", toolCallId: "b", toolName: "bash", input: { command: "python3 - <<'EOF' ..." } }, + { type: "tool-result", toolCallId: "b", toolName: "bash", output: "done" }, + ]); + assert.deepEqual(unresolvedToolCalls(s, TOOLS), [ + { toolCallId: "srv_1", toolName: "code_execution" }, + ]); + }); + + it("an unknown tool name is unresolved even when it has a result", () => { + const s = step([ + { type: "tool-call", toolCallId: "x", toolName: "apply_patch", input: {} }, + { type: "tool-result", toolCallId: "x", toolName: "apply_patch", output: "ok" }, + ]); + assert.deepEqual(unresolvedToolCalls(s, TOOLS), [{ toolCallId: "x", toolName: "apply_patch" }]); + }); + + it("a registered tool with no output is unresolved", () => { + const s = step([{ type: "tool-call", toolCallId: "a", toolName: "bash", input: {} }]); + assert.deepEqual(unresolvedToolCalls(s, TOOLS), [{ toolCallId: "a", toolName: "bash" }]); + }); + + it("accepts a ToolSet as well as a name list", () => { + const tools = { bash: {}, web_search: {} } as unknown as ToolSet; + const s = step([ + { type: "tool-call", toolCallId: "a", toolName: "bash", input: {} }, + { type: "tool-result", toolCallId: "a", toolName: "bash", output: "ok" }, + { type: "tool-call", toolCallId: "z", toolName: "nope", input: {} }, + { type: "tool-result", toolCallId: "z", toolName: "nope", output: "ok" }, + ]); + assert.deepEqual(unresolvedToolCalls(s, tools), [{ toolCallId: "z", toolName: "nope" }]); + }); +}); + +describe("stripToolCallParts", () => { + const messages: ModelMessage[] = [ + { + role: "assistant", + content: [ + { type: "text", text: "Now let's apply the edit." }, + { type: "tool-call", toolCallId: "srv_1", toolName: "code_execution", input: {} }, + { type: "tool-call", toolCallId: "b", toolName: "bash", input: { command: "true" } }, + ], + }, + { + role: "tool", + content: [ + { type: "tool-result", toolCallId: "srv_1", toolName: "code_execution", output: { type: "error-text", value: "no such tool" } }, + { type: "tool-result", toolCallId: "b", toolName: "bash", output: { type: "text", value: "done" } }, + ], + }, + ] as ModelMessage[]; + + it("returns the input untouched when there is nothing to strip", () => { + assert.equal(stripToolCallParts(messages, new Set()), messages); + }); + + it("removes the call and its result, keeping everything else in order", () => { + const out = stripToolCallParts(messages, new Set(["srv_1"])); + assert.equal(out.length, 2); + const asst = out[0].content as Array<{ type: string; toolCallId?: string }>; + assert.deepEqual(asst.map((p) => p.type), ["text", "tool-call"]); + assert.equal(asst[1].toolCallId, "b"); + const tool = out[1].content as Array<{ toolCallId?: string }>; + assert.deepEqual(tool.map((p) => p.toolCallId), ["b"]); + }); + + it("drops a message left with no content", () => { + const only: ModelMessage[] = [ + { role: "assistant", content: [{ type: "tool-call", toolCallId: "srv_1", toolName: "code_execution", input: {} }] }, + { role: "tool", content: [{ type: "tool-result", toolCallId: "srv_1", toolName: "code_execution", output: { type: "text", value: "" } }] }, + ] as ModelMessage[]; + assert.deepEqual(stripToolCallParts(only, new Set(["srv_1"])), []); + }); + + it("leaves string-content messages alone", () => { + const msgs: ModelMessage[] = [{ role: "user", content: "hi" }]; + assert.deepEqual(stripToolCallParts(msgs, new Set(["srv_1"])), msgs); + }); +}); diff --git a/mcp/src/repo/__tests__/worktree_diff.test.ts b/mcp/src/repo/__tests__/worktree_diff.test.ts new file mode 100644 index 000000000..36ecddfc3 --- /dev/null +++ b/mcp/src/repo/__tests__/worktree_diff.test.ts @@ -0,0 +1,167 @@ +/** + * Tests for captureWorktreeDiff (git_pr.ts): the ground-truth diff of an + * ephemeral preview worktree, read before releaseWorktree discards it. + * + * Harness: node:test (tsx --test). Real git fixtures, no mocks. The secret + * scan shells out to gitleaks and fails closed when the binary is absent, + * so the success-path assertions branch on whether it is installed. + */ + +import { describe, it, before, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "fs"; +import path from "path"; +import os from "os"; +import { execSync } from "child_process"; +import { randomUUID } from "crypto"; + +import { + acquireEphemeralWorktree, + releaseWorktree, + captureWorktreeDiff, + type WorktreeHandle, +} from "../git_pr.js"; + +function localEnv(): NodeJS.ProcessEnv { + return { + PATH: process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin", + HOME: os.tmpdir(), + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + GIT_CONFIG_GLOBAL: "/dev/null", + GIT_CONFIG_SYSTEM: "/dev/null", + GIT_AUTHOR_NAME: "Test", + GIT_AUTHOR_EMAIL: "test@example.com", + GIT_COMMITTER_NAME: "Test", + GIT_COMMITTER_EMAIL: "test@example.com", + }; +} + +function hasGitleaks(): boolean { + try { + execSync("gitleaks version", { stdio: "ignore" }); + return true; + } catch { + return false; + } +} + +let testRootDir: string; +let baseCloneDir: string; + +before(() => { + testRootDir = fs.mkdtempSync(path.join(os.tmpdir(), "worktree_diff_test_")); + baseCloneDir = path.join(testRootDir, "base-clone"); + fs.mkdirSync(baseCloneDir, { recursive: true }); + const env = localEnv(); + execSync("git init -b main .", { cwd: baseCloneDir, env, stdio: "ignore" }); + fs.writeFileSync(path.join(baseCloneDir, "README.md"), "# worktree diff test repo\n"); + fs.writeFileSync(path.join(baseCloneDir, ".gitignore"), "ignored.log\n"); + execSync("git add .", { cwd: baseCloneDir, env, stdio: "ignore" }); + execSync('git commit -m "Initial commit"', { cwd: baseCloneDir, env, stdio: "ignore" }); +}); + +after(() => { + fs.rmSync(testRootDir, { recursive: true, force: true }); +}); + +async function withWorktree(fn: (handle: WorktreeHandle) => Promise): Promise { + const result = await acquireEphemeralWorktree({ + baseDir: baseCloneDir, + owner: "owner", + repo: "repo", + runId: randomUUID(), + }); + assert.ok(result.ok, `acquire failed: ${(result as any).error}`); + try { + return await fn(result.handle); + } finally { + await releaseWorktree(result.handle); + } +} + +describe("captureWorktreeDiff", () => { + it("reports no_changes for an untouched worktree", async () => { + await withWorktree(async (handle) => { + const out = await captureWorktreeDiff(handle); + assert.equal(out.ok, false); + if (!out.ok) assert.equal(out.failure, "no_changes"); + }); + }); + + it("includes a modified file AND a brand-new untracked file, ignoring .gitignore'd ones", async () => { + await withWorktree(async (handle) => { + fs.writeFileSync(path.join(handle.worktreePath, "README.md"), "# edited\n"); + fs.mkdirSync(path.join(handle.worktreePath, "src"), { recursive: true }); + fs.writeFileSync(path.join(handle.worktreePath, "src", "new.test.ts"), "export const x = 1;\n"); + fs.writeFileSync(path.join(handle.worktreePath, "ignored.log"), "noise\n"); + + const out = await captureWorktreeDiff(handle); + + if (hasGitleaks()) { + assert.ok(out.ok, `expected ok, got ${JSON.stringify(out)}`); + if (out.ok) { + assert.equal(out.filesChanged, 2); + assert.ok(out.diff.includes("+++ b/README.md"), "modified file present"); + assert.ok(out.diff.includes("+++ b/src/new.test.ts"), "new untracked file present"); + assert.ok(out.diff.includes("new file mode"), "new file recorded as such"); + assert.ok(!out.diff.includes("ignored.log"), "ignored file excluded"); + } + } else { + // Fails closed on the scan, but only after staging succeeded and the + // size checks passed — so the failure is the scan, nothing earlier. + assert.equal(out.ok, false); + if (!out.ok) { + assert.equal(out.failure, "secrets_detected"); + assert.match(out.error, /gitleaks/); + } + } + + // The base checkout is never touched. + const baseStatus = execSync("git status --porcelain", { + cwd: baseCloneDir, + env: localEnv(), + encoding: "utf8", + }).trim(); + assert.equal(baseStatus, ""); + }); + }); + + it("enforces the file cap before reading the diff", async () => { + await withWorktree(async (handle) => { + fs.writeFileSync(path.join(handle.worktreePath, "a.txt"), "a\n"); + fs.writeFileSync(path.join(handle.worktreePath, "b.txt"), "b\n"); + const out = await captureWorktreeDiff(handle, { maxFiles: 1 }); + assert.equal(out.ok, false); + if (!out.ok) assert.equal(out.failure, "change_too_large"); + }); + }); + + it("enforces the byte cap", async () => { + await withWorktree(async (handle) => { + fs.writeFileSync(path.join(handle.worktreePath, "big.txt"), "x".repeat(4096) + "\n"); + const out = await captureWorktreeDiff(handle, { maxBytes: 1024 }); + assert.equal(out.ok, false); + if (!out.ok) assert.equal(out.failure, "change_too_large"); + }); + }); + + it("reports git_failed when the worktree is already gone", async () => { + const handle = { + worktreePath: path.join(testRootDir, "does-not-exist"), + baseDir: baseCloneDir, + baseSha: "", + baseName: "main", + branch: "", + owner: "owner", + repo: "repo", + runId: randomUUID(), + runHome: os.tmpdir(), + _children: new Set(), + _released: false, + } as unknown as WorktreeHandle; + const out = await captureWorktreeDiff(handle); + assert.equal(out.ok, false); + if (!out.ok) assert.equal(out.failure, "git_failed"); + }); +}); diff --git a/mcp/src/repo/agent.ts b/mcp/src/repo/agent.ts index a47a9e45a..ffcbf5405 100644 --- a/mcp/src/repo/agent.ts +++ b/mcp/src/repo/agent.ts @@ -39,6 +39,8 @@ import { extractFinalAnswer, maxOutputTokensFor, needsContinuation, + unresolvedToolCalls, + stripToolCallParts, isContinuationNudge, timeBudgetNudge, createHasEndMarkerCondition, @@ -1204,6 +1206,22 @@ function continuationNudge( return `You ended your turn without completing the task. Do not stop to describe a plan or intention — execute it now: make the tool calls you described, and when you have the final answer, write it followed by [END_OF_ANSWER]. If you are blocked on information only the user can provide, ${askPath} instead of guessing.`; } +/** + * Nudge after the loop exited on a tool call it could not resolve (see + * unresolvedToolCalls). The offending call and its result have already been + * removed from the transcript by the time this is sent, so the model is told + * that nothing it asked that tool for actually happened. + */ +function unavailableToolNudge(unavailable: string[], available: string[]): string { + const names = [...new Set(unavailable)].map((n) => `\`${n}\``).join(", "); + const plural = unavailable.length > 1; + return `Your call${plural ? "s" : ""} to ${names} could not be executed: ${ + plural ? "those tools are" : "that tool is" + } not available in this run, and the call has been removed from the conversation — nothing it would have done happened. Available tools: ${available.join( + ", " + )}. Redo that work with the tools you have (bash runs shell commands and edits files), then continue the task. When the answer is complete, end with [END_OF_ANSWER].`; +} + /** The exact messages sent to the model on the initial call (model-facing, not storage). */ function initialModelMessages(prepared: PreparedAgent): ModelMessage[] { const { finalPrompt, previousMessages, userMessage } = prepared; @@ -1347,47 +1365,80 @@ export async function get_context( // nudges didn't fix won't be fixed by a third. let stallNudges = 0; let lengthContinuations = 0; + let unresolvedRetried = false; for (;;) { const allSteps = segments.flatMap((s) => s.steps); - if (!needsContinuation(allSteps)) break; - const lastFinish = allSteps[allSteps.length - 1]?.finishReason; - const kind: ContinuationKind = - lastFinish === "length" - ? "length" - : lastFinish === "error" - ? "error" - : "stall"; - // Involuntary interruptions (truncation, mid-stream connection errors) - // share the larger progress-gated allowance; only voluntary stalls are - // capped at MAX_STALL_NUDGES. - if (kind !== "stall") { - if (lengthContinuations >= MAX_LENGTH_CONTINUATIONS) break; - lengthContinuations++; + const lastStep = allSteps[allSteps.length - 1]; + const lastFinish = lastStep?.finishReason; + // Tool calls the loop could not resolve on its last step. The SDK exits + // on such a step with no further model turn and no error — as it does + // for a client-executed tool whose result the caller must supply — so + // without this check the run reads as complete, with narration for an + // answer. Observed cause: an LLM gateway rewrote a server tool's + // version and the API answered with a server_tool_use (code_execution) + // that was never offered. One retry: drop the unresolvable calls from + // the transcript (the API rejects a tool_result for a server tool it + // never ran) and tell the model which tool it does not have. Still + // unresolved after that, the run fails after the loop rather than + // reporting success. + const unresolved = unresolvedToolCalls(lastStep, prepared.tools); + let kind: ContinuationKind | "unresolved"; + if (unresolved.length > 0) { + if (unresolvedRetried) break; + unresolvedRetried = true; + kind = "unresolved"; } else { - if (stallNudges >= MAX_STALL_NUDGES) break; - stallNudges++; + if (!needsContinuation(allSteps)) break; + kind = + lastFinish === "length" + ? "length" + : lastFinish === "error" + ? "error" + : "stall"; + // Involuntary interruptions (truncation, mid-stream connection errors) + // share the larger progress-gated allowance; only voluntary stalls are + // capped at MAX_STALL_NUDGES. + if (kind !== "stall") { + if (lengthContinuations >= MAX_LENGTH_CONTINUATIONS) break; + lengthContinuations++; + } else { + if (stallNudges >= MAX_STALL_NUDGES) break; + stallNudges++; + } } - const generated = (await run.streamResult.response).messages as ModelMessage[]; + const generatedRaw = (await run.streamResult.response).messages as ModelMessage[]; + const generated = + kind === "unresolved" + ? stripToolCallParts(generatedRaw, new Set(unresolved.map((u) => u.toolCallId))) + : generatedRaw; // Tagged via providerOptions so session consumers (transcript rendering, // turn counting) can tell this synthetic message from a real user turn; // model providers only read their own providerOptions key and ignore it. const nudge: ModelMessage = { role: "user", - content: continuationNudge( - prepared.askQuestionsEnabled, - kind, - maxOutputTokensFor(prepared.provider) - ), + content: + kind === "unresolved" + ? unavailableToolNudge( + unresolved.map((u) => u.toolName), + Object.keys(prepared.tools) + ) + : continuationNudge( + prepared.askQuestionsEnabled, + kind, + maxOutputTokensFor(prepared.provider) + ), providerOptions: { stakgraph: { continuationNudge: true } }, }; const convo = [...sent, ...generated, nudge]; console.warn( `===> agent ended turn without finishing (finishReason: ${lastFinish}, raw: ${ - allSteps[allSteps.length - 1]?.rawFinishReason + lastStep?.rawFinishReason }); ${ - kind !== "stall" - ? `${kind} continuation ${lengthContinuations}/${MAX_LENGTH_CONTINUATIONS}` - : `stall nudge ${stallNudges}/${MAX_STALL_NUDGES}` + kind === "unresolved" + ? `unresolved tool call(s): ${[...new Set(unresolved.map((u) => u.toolName))].join(", ")} — retrying once without them` + : kind !== "stall" + ? `${kind} continuation ${lengthContinuations}/${MAX_LENGTH_CONTINUATIONS}` + : `stall nudge ${stallNudges}/${MAX_STALL_NUDGES}` }` ); run = await runWithBadRequestRetry( @@ -1416,6 +1467,24 @@ export async function get_context( } steps = segments.flatMap((s) => s.steps); + // A last step the loop could not resolve, even after the retry above, is + // not a result. Fail loudly; the caller would otherwise get narration. + const stillUnresolved = unresolvedToolCalls(steps[steps.length - 1], prepared.tools); + if (stillUnresolved.length > 0) { + const names = [...new Set(stillUnresolved.map((u) => u.toolName))].join(", "); + throw new Error( + `Run ended on a tool call that could not be resolved: ${names}. ` + + `That tool is not registered for this run — if the request went through an LLM gateway, check whether it rewrites the tool list.` + ); + } + // Abort mid-run: the SDK closes the stream and resolves with the steps + // so far (no finish part, so finish reason "other"), and nothing throws. + // Report it as the abort it is rather than as a completed run. + if (opts.abortSignal?.aborted && (await run.streamResult.finishReason) === "other") { + const abortErr = new Error("Run aborted before completion"); + abortErr.name = "AbortError"; + throw abortErr; + } // The final call's own messages plus what it generated — byte-identical to // what the provider cached, unlike the (possibly truncated) stored copy. modelFacingMessages = [ @@ -1495,6 +1564,24 @@ export async function get_context( reflection = await reflectOnConcepts(prepared, opts, modelFacingMessages); } + // Continuation allowance exhausted without a proper termination: the text + // that extractFinalAnswer will pick up is narration, not an answer. Say so + // on the result so a caller that needs a deliverable can refuse it. + const lastStepFinish = steps[steps.length - 1]?.finishReason; + const incomplete: ContextResult["incomplete"] = needsContinuation(steps) + ? { + reason: + lastStepFinish === "length" + ? "length" + : lastStepFinish === "error" + ? "error" + : "stall", + } + : undefined; + if (incomplete) { + console.warn(`===> run ended incomplete (${incomplete.reason}); final answer is narration`); + } + const final = extractFinalAnswer(steps); let finalAnswer = final.answer; @@ -1527,6 +1614,7 @@ export async function get_context( sessionId, reflection, pr: terminalPrResult(prepared.prCollector.result, opts.toolsConfig, opts.prMode), + incomplete, }; } diff --git a/mcp/src/repo/git_pr.ts b/mcp/src/repo/git_pr.ts index cb11e3f3f..3699c1cb0 100644 --- a/mcp/src/repo/git_pr.ts +++ b/mcp/src/repo/git_pr.ts @@ -1103,3 +1103,127 @@ export async function landChange( export function _clearLandedState(runId: string): void { landedResults.delete(runId); } + +// --------------------------------------------------------------------------- +// captureWorktreeDiff — ground-truth diff of an ephemeral preview worktree +// --------------------------------------------------------------------------- + +export type WorktreeDiffFailure = + | "no_changes" + | "change_too_large" + | "secrets_detected" + | "git_failed"; + +export type WorktreeDiffResult = + | { ok: true; diff: string; filesChanged: number } + | { ok: false; failure: WorktreeDiffFailure; error: string }; + +export interface CaptureWorktreeDiffOpts { + maxFiles?: number; + maxBytes?: number; + signal?: AbortSignal; +} + +/** + * Read what an ephemeral preview run actually changed, straight from its + * worktree, before releaseWorktree discards it. + * + * Everything is staged first: `git diff HEAD` on its own skips untracked + * files, so a run that created a file would come back without it. The + * worktree is throwaway and detached, so staging has no side effects, and + * `git add -A` honours .gitignore. Same caps and secret scan as landChange, + * and like landChange a scan that cannot run fails closed. + * + * This replaces trusting the model to paste `git diff` output into its + * answer — fragile (truncation, code fences, missing new files) and silently + * empty whenever the run ended before its final message. + */ +export async function captureWorktreeDiff( + handle: WorktreeHandle, + opts: CaptureWorktreeDiffOpts = {} +): Promise { + const maxFiles = opts.maxFiles ?? DEFAULT_MAX_FILES; + const maxBytes = opts.maxBytes ?? DEFAULT_MAX_BYTES; + const cwd = handle.worktreePath; + if (!fs.existsSync(cwd)) { + return { ok: false, failure: "git_failed", error: `Worktree is gone: ${cwd}` }; + } + // Local-only git: no identity, no credentials, no prompts. + const env: NodeJS.ProcessEnv = { + PATH: process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin", + HOME: os.tmpdir(), + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + }; + const git = (args: string[], timeoutMs: number) => + runGit(args, { cwd, env, timeoutMs, signal: opts.signal }); + + const add = await git(["add", "-A"], 30_000); + if (add.code !== 0) { + return { ok: false, failure: "git_failed", error: `git add failed: ${add.stderr}` }; + } + + const names = await git(["diff", "--cached", "--name-only"], 10_000); + if (names.code !== 0) { + return { + ok: false, + failure: "git_failed", + error: `git diff --name-only failed: ${names.stderr}`, + }; + } + const files = names.stdout + .split("\n") + .map((l) => l.trim()) + .filter((l) => l.length > 0); + if (files.length === 0) { + return { ok: false, failure: "no_changes", error: "No changes in the worktree after the run" }; + } + if (files.length > maxFiles) { + return { + ok: false, + failure: "change_too_large", + error: `${files.length} files changed, limit is ${maxFiles}`, + }; + } + + const diffRes = await git(["diff", "--cached", "--no-color"], 30_000); + if (diffRes.code !== 0) { + return { ok: false, failure: "git_failed", error: `git diff failed: ${diffRes.stderr}` }; + } + const diff = diffRes.stdout; + const bytes = Buffer.byteLength(diff, "utf8"); + if (bytes > maxBytes) { + return { + ok: false, + failure: "change_too_large", + error: `Diff is ${bytes} bytes, limit is ${maxBytes}`, + }; + } + + try { + const findings = gitleaksProtect(cwd); + if (findings.length > 0) { + const summary = findings + .map((f) => `${f.File}:${f.StartLine} (${f.RuleID})`) + .join(", "); + return { + ok: false, + failure: "secrets_detected", + error: `Secret scan found ${findings.length} finding(s): ${summary}`, + }; + } + } catch (scanErr: unknown) { + const msg = scanErr instanceof Error ? scanErr.message : String(scanErr); + const absent = + msg.includes("ENOENT") || msg.includes("not found") || msg.includes("No such file"); + return { + ok: false, + failure: "secrets_detected", + error: absent + ? "gitleaks binary not found — failing closed on secret scan" + : `Secret scan failed: ${msg}`, + }; + } + + return { ok: true, diff, filesChanged: files.length }; +} diff --git a/mcp/src/repo/index.ts b/mcp/src/repo/index.ts index daba57d24..ba68742a4 100644 --- a/mcp/src/repo/index.ts +++ b/mcp/src/repo/index.ts @@ -30,12 +30,14 @@ import { db } from "../graph/neo4j.js"; import { acquireWorktree, acquireEphemeralWorktree, + captureWorktreeDiff, releaseWorktree, resolveIdentity, gitEnv, branchName, OctokitGitHubClient, type WorktreeHandle, + type WorktreeDiffResult, } from "./git_pr.js"; import { withRepoLock } from "./repo_lock.js"; import fs from "fs"; @@ -1039,10 +1041,36 @@ export async function repo_agent(req: Request, res: Response) { }, }); }) - .then((result) => { + .then(async (result) => { // Guard: if drainForShutdown() has already flipped this record and // delivered a failed/retryable:true webhook, do not overwrite. if (shuttingDown) return; + // Ephemeral preview: read the diff from the worktree itself, here, + // before teardown in .finally() discards it. Ground truth for what + // the run changed — new files included — rather than whatever the + // model chose to paste into its answer. Sits next to `pr`, its + // create_pr counterpart. A capture failure is reported on the + // result, never thrown: the run itself finished. + let diff: WorktreeDiffResult | undefined; + if (body.ephemeral && nonStreamWorktreeHandle) { + try { + diff = await captureWorktreeDiff(nonStreamWorktreeHandle, { + signal: abortController.signal, + }); + } catch (e) { + diff = { + ok: false, + failure: "git_failed", + error: e instanceof Error ? e.message : String(e), + }; + } + console.log( + `[repo_agent] ephemeral diff: request_id=${request_id} ` + + (diff.ok + ? `${diff.filesChanged} file(s), ${Buffer.byteLength(diff.diff, "utf8")} bytes` + : `${diff.failure}: ${diff.error}`), + ); + } const terminalResult = { success: true, final_answer: result.final, @@ -1057,6 +1085,14 @@ export async function repo_agent(req: Request, res: Response) { reflection: result.reflection, // Present when create_pr was enabled; structured result from landChange(). pr: result.pr, + // Present on ephemeral runs: the worktree's staged diff, or why + // there is none. See captureWorktreeDiff. + diff: diff?.ok ? diff.diff : undefined, + diff_files: diff?.ok ? diff.filesChanged : undefined, + diff_error: diff && !diff.ok ? { failure: diff.failure, error: diff.error } : undefined, + // Present when the run ended without a proper termination — + // `final_answer` is then narration, not an answer. + incomplete: result.incomplete, }; asyncReqs.finishReq(request_id, terminalResult); // Post-completion side effects are isolated: if one throws it must diff --git a/mcp/src/repo/tools.ts b/mcp/src/repo/tools.ts index 7f8c1ec77..6eba8205b 100644 --- a/mcp/src/repo/tools.ts +++ b/mcp/src/repo/tools.ts @@ -573,8 +573,17 @@ export async function get_tools( return null; } + // Web search is a provider-executed (server-side) tool, and it is withheld + // on confined runs — create_pr worktrees and ephemeral previews. Those runs + // exist to edit files and have no use for it, and carrying a server tool in + // the request is a liability there: an LLM gateway that rewrites the tool's + // version makes the Anthropic API offer code_execution implicitly, the + // model then answers with a server_tool_use the SDK never registered, and + // the tool loop exits silently on that step. Observed 2026-09-15 on + // propose_code_change previews routed through Bifrost: the edit landed in + // the worktree, the run reported success, and the answer was narration. const web_search_tool = - provider === "anthropic" + provider === "anthropic" && !confinedRun(options) ? getProviderTool(provider, apiKey, "webSearch") : undefined; const bash_tool = diff --git a/mcp/src/repo/utils.ts b/mcp/src/repo/utils.ts index f9a3c5028..e99fcd02d 100644 --- a/mcp/src/repo/utils.ts +++ b/mcp/src/repo/utils.ts @@ -180,6 +180,69 @@ export function needsContinuation(steps: StepResult[]): boolean { ); } +/** + * Tool calls on a step that the tool loop could not resolve. The AI SDK + * exits its loop on such a step — no further model turn, no stop condition, + * no error — exactly as it does for a client-executed tool whose result the + * caller is expected to supply. A run whose last step has any of these is + * therefore incomplete whatever its text says. + * + * A call is unresolved when it names a tool that is not registered for the + * run (the SDK marks these `invalid`), or when the step holds no result or + * error for it. Observed cause: an LLM gateway rewrote a server tool's + * version, and the API answered with a server_tool_use (code_execution) that + * was never offered. + */ +export function unresolvedToolCalls( + step: StepResult | undefined, + tools: ToolSet | readonly string[], +): { toolCallId: string; toolName: string }[] { + if (!step) return []; + const known = new Set(Array.isArray(tools) ? tools : Object.keys(tools)); + const resolved = new Set(); + for (const item of step.content) { + if (item.type === "tool-result" || item.type === "tool-error") { + resolved.add(item.toolCallId); + } + } + const out: { toolCallId: string; toolName: string }[] = []; + for (const item of step.content) { + if (item.type !== "tool-call") continue; + const invalid = (item as { invalid?: boolean }).invalid === true; + if (invalid || !known.has(item.toolName) || !resolved.has(item.toolCallId)) { + out.push({ toolCallId: item.toolCallId, toolName: item.toolName }); + } + } + return out; +} + +/** + * Drop every part that references one of `toolCallIds` from a run of model + * messages — the call itself and any result or error recorded for it — so + * the transcript can be replayed. A tool_result for a server tool the API + * never ran is rejected on replay, and a dangling tool_use without a result + * is rejected too. A message left with no content is dropped whole. + */ +export function stripToolCallParts( + messages: ModelMessage[], + toolCallIds: ReadonlySet, +): ModelMessage[] { + if (toolCallIds.size === 0) return messages; + const out: ModelMessage[] = []; + for (const m of messages) { + if (!Array.isArray(m.content)) { + out.push(m); + continue; + } + const content = (m.content as Array<{ toolCallId?: string }>).filter( + (part) => !(typeof part.toolCallId === "string" && toolCallIds.has(part.toolCallId)), + ); + if (content.length === 0) continue; + out.push({ ...m, content } as ModelMessage); + } + return out; +} + /** * Time-budget status nudges, injected between steps as a run approaches the * busy-timeout hard kill (which aborts the stream and discards all work). diff --git a/mcp/src/tools/types.ts b/mcp/src/tools/types.ts index 0f1c41550..1237fa2b3 100644 --- a/mcp/src/tools/types.ts +++ b/mcp/src/tools/types.ts @@ -28,4 +28,10 @@ export interface ContextResult { // Present when a create_pr-enabled run successfully (or unsuccessfully) // attempted to land a PR. Structured result from landChange(). pr?: LandChangeResult; + // Present when the run ended without a proper termination: the + // continuation allowance ran out on a voluntary stall, an output-token + // truncation, or a mid-stream error. `final` then holds whatever + // narration the model produced, not a finished answer, and a caller that + // needs a real deliverable (a diff, a PR) should treat the run as failed. + incomplete?: { reason: "stall" | "length" | "error" }; } From ea6c17b204f27664b8c9c15cd6204ca756ea7f45 Mon Sep 17 00:00:00 2001 From: Evanfeenstra Date: Tue, 15 Sep 2026 12:11:00 -0700 Subject: [PATCH 2/2] Report the ephemeral preview diff as `preview`, mirroring `pr` Same ok / failure / error shape as the create_pr result, one field instead of three, so the two run types read alike on the terminal result. --- mcp/src/repo/index.ts | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/mcp/src/repo/index.ts b/mcp/src/repo/index.ts index ba68742a4..36f5c93c3 100644 --- a/mcp/src/repo/index.ts +++ b/mcp/src/repo/index.ts @@ -1048,27 +1048,28 @@ export async function repo_agent(req: Request, res: Response) { // Ephemeral preview: read the diff from the worktree itself, here, // before teardown in .finally() discards it. Ground truth for what // the run changed — new files included — rather than whatever the - // model chose to paste into its answer. Sits next to `pr`, its - // create_pr counterpart. A capture failure is reported on the - // result, never thrown: the run itself finished. - let diff: WorktreeDiffResult | undefined; + // model chose to paste into its answer. Reported as `preview`, the + // counterpart of `pr` on create_pr runs, with the same + // `ok` / `failure` / `error` shape. A capture failure is reported + // on the result, never thrown: the run itself finished. + let preview: WorktreeDiffResult | undefined; if (body.ephemeral && nonStreamWorktreeHandle) { try { - diff = await captureWorktreeDiff(nonStreamWorktreeHandle, { + preview = await captureWorktreeDiff(nonStreamWorktreeHandle, { signal: abortController.signal, }); } catch (e) { - diff = { + preview = { ok: false, failure: "git_failed", error: e instanceof Error ? e.message : String(e), }; } console.log( - `[repo_agent] ephemeral diff: request_id=${request_id} ` + - (diff.ok - ? `${diff.filesChanged} file(s), ${Buffer.byteLength(diff.diff, "utf8")} bytes` - : `${diff.failure}: ${diff.error}`), + `[repo_agent] ephemeral preview diff: request_id=${request_id} ` + + (preview.ok + ? `${preview.filesChanged} file(s), ${Buffer.byteLength(preview.diff, "utf8")} bytes` + : `${preview.failure}: ${preview.error}`), ); } const terminalResult = { @@ -1086,10 +1087,8 @@ export async function repo_agent(req: Request, res: Response) { // Present when create_pr was enabled; structured result from landChange(). pr: result.pr, // Present on ephemeral runs: the worktree's staged diff, or why - // there is none. See captureWorktreeDiff. - diff: diff?.ok ? diff.diff : undefined, - diff_files: diff?.ok ? diff.filesChanged : undefined, - diff_error: diff && !diff.ok ? { failure: diff.failure, error: diff.error } : undefined, + // there is none. Same shape as `pr`. See captureWorktreeDiff. + preview, // Present when the run ended without a proper termination — // `final_answer` is then narration, not an answer. incomplete: result.incomplete,