Agent review: only client-run tools in reviews (#85)
* Agent review: no paid provider-side tools in reviews A review's requests are sent without the tools that run on the provider and cost extra beyond tokens: Anthropic's web search, web fetch, code execution and MCP servers, and OpenAI's web search, file search, code interpreter, image generation and MCP. Claude Code reviews run with WebSearch and WebFetch disallowed, and Codex reviews with web search disabled. Tasks are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VDQqT8oL18uLJ9fpsNxW2 * Agent review: keep only client-run tools (an allowlist), so dated and new server tools are dropped too Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VDQqT8oL18uLJ9fpsNxW2 --------- Co-authored-by: Claude <noreply@anthropic.com>
Canberk Demirci authored · GitHub committed signed1 parent ad2fbf5commit 3b4bcba486d816cddb40f53822f6e7b55a7d72b2Browse files
7 files changed+98−13
Files (7)
modifiedapps/diffinery/docs/agents/review.md+1−1View file
| @@ -115,7 +115,7 @@ | ||
| 115 | 115 | |
| 116 | 116 | - Before a request goes to the provider, the proxy books the most it may cost: a token per 3 bytes of request, plus the output it allows (its `max_tokens` or the endpoint's equivalent, or 32,000 when it sets none; a Codex compaction, which can't be capped, books 128,000). If that could take the review past its budget, the request is refused and the review stops. So requests sent at once can't overrun the budget together. |
| 117 | 117 | - Once the response has streamed through, the booking is replaced by the usage the provider reports: input, output and cache writes in full, cache reads as a tenth, since providers bill them at about a tenth. A response that reports no usage, or whose usage can't be read, stays charged its booking. An error response costs nothing. |
| 118 | -- A review can't start background responses, use `previous_response_id`, `conversation` or `prompt` (they pull in context stored at the provider that the booking can't see), ask for more than one answer (`n`), or set routing and paid extras (`models`, `route`, `provider`, `plugins`, `web_search_options`, `speed`, `best_of`). A review through OpenRouter asks only for its configured model. A review's request is sent with every output cap set to the one booked (the largest it set, or 32,000 when it set none), so its booking holds. | |
| 118 | +- A review can't start background responses, use `previous_response_id`, `conversation` or `prompt` (they pull in context stored at the provider that the booking can't see), ask for more than one answer (`n`), or set routing and paid extras (`models`, `route`, `provider`, `plugins`, `web_search_options`, `speed`, `best_of`). A review through OpenRouter asks only for its configured model. A review's requests keep only tools the agent runs itself (function and custom tools, and client-run ones such as bash, text editor, memory, local shell and apply patch); every tool that runs on the provider is stripped, along with MCP servers, so web search, web fetch, code execution, file search, image generation and any server tool a provider adds later never reach it. Claude Code reviews also run with WebSearch and WebFetch turned off, and Codex reviews with web search disabled. A review's request is sent with every output cap set to the one booked (the largest it set, or 32,000 when it set none), so its booking holds. | |
| 119 | 119 | |
| 120 | 120 | Every run's model requests and tokens show in its transcript and in the API's `runs` (`model_requests`, `tokens`). |
| 121 | 121 |
modifiedapps/diffinery/src/agentic/ProjectDO.ts+1−1View file
| @@ -589,7 +589,7 @@ | ||
| 589 | 589 | const token = `dmt_${t.repo_id}_${randomToken(32)}`; |
| 590 | 590 | this.sql.exec("UPDATE runs SET token_hash = ?, checked_at = ? WHERE task = ? AND seq = ?", await sha256(token), now(), r.task, r.seq); |
| 591 | 591 | const { env, secret } = agentEnv(this.env.PUBLIC_URL, providers, token, { runtime: t.runtime, route: { via: t.via, model: t.model } }); |
| 592 | - return this.startProc(r, "agent", RUNTIMES[t.runtime].command(t.command ?? undefined, t.via ?? undefined), { ...env, DIFFINERY_TASK_PROMPT: r.prompt }, secret, timeoutOf(t) - (Date.now() - Date.parse(r.started_at)), null); | |
| 592 | + return this.startProc(r, "agent", RUNTIMES[t.runtime].command(t.command ?? undefined, t.via ?? undefined, t.kind === "review"), { ...env, DIFFINERY_TASK_PROMPT: r.prompt }, secret, timeoutOf(t) - (Date.now() - Date.parse(r.started_at)), null); | |
| 593 | 593 | } |
| 594 | 594 | if (!r.checked_at || Date.parse(r.checked_at) + RECHECK_MS < Date.now()) { |
| 595 | 595 | await this.checkAccess(t); |
modifiedapps/diffinery/src/agentic/runtimes.ts+5−4View file
| @@ -29,7 +29,7 @@ | ||
| 29 | 29 | * The run itself; the prompt is in $DIFFINERY_TASK_PROMPT and an OpenRouter |
| 30 | 30 | * model in $DIFFINERY_MODEL, never on the command line we build. |
| 31 | 31 | */ |
| 32 | - command: (custom?: string, via?: Provider) => string; | |
| 32 | + command: (custom?: string, via?: Provider, review?: boolean) => string; | |
| 33 | 33 | } |
| 34 | 34 | |
| 35 | 35 | export const RUNTIMES: Record<RuntimeId, Runtime> = { |
| @@ -42,7 +42,8 @@ | ||
| 42 | 42 | alternatives: ["openrouter"], |
| 43 | 43 | install: `npm install --global --no-fund --no-audit --loglevel=error @anthropic-ai/claude-code@${RUNTIME_VERSIONS["claude-code"]} && claude --version`, |
| 44 | 44 | // The sandbox runs as root in its own container; IS_SANDBOX lets Claude Code skip permission prompts there. |
| 45 | - command: () => 'claude -p "$DIFFINERY_TASK_PROMPT" --output-format stream-json --verbose --permission-mode bypassPermissions', | |
| 45 | + // A review has no web tools: they run on the provider and cost extra (the model proxy strips them too). | |
| 46 | + command: (_custom, _via, review) => `claude -p "$DIFFINERY_TASK_PROMPT" --output-format stream-json --verbose --permission-mode bypassPermissions${review ? ' --disallowedTools "WebSearch,WebFetch"' : ""}`, | |
| 46 | 47 | }, |
| 47 | 48 | codex: { |
| 48 | 49 | id: "codex", |
| @@ -52,8 +53,8 @@ | ||
| 52 | 53 | alternatives: ["openrouter"], |
| 53 | 54 | install: `npm install --global --no-fund --no-audit --loglevel=error @openai/codex@${RUNTIME_VERSIONS.codex} && codex --version`, |
| 54 | 55 | // Through OpenRouter: a model provider for its OpenAI-compatible Responses API, defined on the command line. |
| 55 | - command: (_custom, via) => | |
| 56 | - `codex exec --json --dangerously-bypass-approvals-and-sandbox --skip-git-repo-check${ | |
| 56 | + command: (_custom, via, review) => | |
| 57 | + `codex exec --json --dangerously-bypass-approvals-and-sandbox --skip-git-repo-check${review ? " -c web_search=disabled" : ""}${ | |
| 57 | 58 | via === "openrouter" |
| 58 | 59 | ? ' -c model_provider=openrouter -c model_providers.openrouter.name=OpenRouter -c "model_providers.openrouter.base_url=$OPENROUTER_BASE_URL" -c model_providers.openrouter.env_key=OPENROUTER_API_KEY -c model_providers.openrouter.wire_api=responses -m "$DIFFINERY_MODEL"' |
| 59 | 60 | : "" |
modifiedapps/diffinery/src/agentic/usage.ts+50−4View file
| @@ -143,6 +143,49 @@ | ||
| 143 | 143 | return undefined; |
| 144 | 144 | } |
| 145 | 145 | |
| 146 | +/** | |
| 147 | + * The tools a review's requests may carry: tools the agent runs itself. Function and custom tools (and Anthropic's, | |
| 148 | + * which have no type), and the client-run built-ins, versioned or not (bash_20250124, text_editor_20250728, OpenAI's | |
| 149 | + * local_shell and apply_patch). Every other tool type runs on the provider and may cost extra beyond tokens (web | |
| 150 | + * search, web fetch, code execution, file search, MCP, and whatever providers add next), so it is dropped, with | |
| 151 | + * `mcp_servers`: a review's token budget then bounds what it spends. | |
| 152 | + */ | |
| 153 | +const CLIENT_TOOL = /^(function|custom|(bash|text_editor|computer|memory|local_shell|shell|apply_patch)(_\d+)?)$/; | |
| 154 | + | |
| 155 | +const typeOf = (t: unknown): unknown => (t && typeof t === "object" ? (t as { type?: unknown }).type : undefined); | |
| 156 | +const clientTool = (t: unknown): boolean => { | |
| 157 | + const type = typeOf(t); | |
| 158 | + return !!t && typeof t === "object" && (type === undefined || (typeof type === "string" && CLIENT_TOOL.test(type))); | |
| 159 | +}; | |
| 160 | +/** The name a tool or a tool choice gives: Anthropic's and Responses' `name`, or Chat Completions' `function.name`. */ | |
| 161 | +const nameOf = (t: unknown): unknown => (t && typeof t === "object" ? ((t as { name?: unknown }).name ?? ((t as { function?: { name?: unknown } }).function?.name)) : undefined); | |
| 162 | + | |
| 163 | +/** A request's fields with only client-run tools; the same object when it has nothing else. */ | |
| 164 | +function withoutPaidTools(x: Record<string, unknown>): Record<string, unknown> { | |
| 165 | + const tools = Array.isArray(x.tools) ? (x.tools as unknown[]) : null; | |
| 166 | + const kept = tools?.filter(clientTool) ?? []; | |
| 167 | + const dropTools = !!tools && kept.length !== tools.length; | |
| 168 | + if (!dropTools && x.mcp_servers === undefined) return x; | |
| 169 | + const out: Record<string, unknown> = { ...x }; | |
| 170 | + delete out.mcp_servers; | |
| 171 | + if (dropTools) { | |
| 172 | + if (kept.length) out.tools = kept; | |
| 173 | + else delete out.tools; | |
| 174 | + // A tool choice that names a tool, or a tool type, that isn't kept would be refused upstream, as would any choice once no tools are left. | |
| 175 | + const choice = out.tool_choice; | |
| 176 | + if (!kept.length) delete out.tool_choice; | |
| 177 | + else if (choice && typeof choice === "object") { | |
| 178 | + const type = typeOf(choice); | |
| 179 | + const name = nameOf(choice); | |
| 180 | + const general = type === "auto" || type === "any" || type === "none"; | |
| 181 | + const keptName = name !== undefined && kept.some((t) => nameOf(t) === name); | |
| 182 | + const keptType = typeof type === "string" && type !== "function" && type !== "custom" && type !== "tool" && kept.some((t) => typeOf(t) === type); | |
| 183 | + if (!general && !keptName && !keptType) delete out.tool_choice; | |
| 184 | + } | |
| 185 | + } | |
| 186 | + return out; | |
| 187 | +} | |
| 188 | + | |
| 146 | 189 | /** Responses API fields that pull in context stored at the provider, which the request's size doesn't show. */ |
| 147 | 190 | const STORED_CONTEXT = ["previous_response_id", "conversation", "prompt"] as const; |
| 148 | 191 | |
| @@ -153,8 +196,9 @@ | ||
| 153 | 196 | * and streamed Chat Completions are asked to include usage. A review may not |
| 154 | 197 | * start a background response, which it could leave unfetched and so |
| 155 | 198 | * uncharged, pull in stored context its estimate can't see, or ask for more |
| 156 | - * than one answer; and a review's request that doesn't cap its output is | |
| 157 | - * capped at the estimate's DEFAULT_MAX_OUTPUT. A compaction can't be capped, | |
| 199 | + * than one answer; a review's request that doesn't cap its output is | |
| 200 | + * capped at the estimate's DEFAULT_MAX_OUTPUT, and paid provider-side tools | |
| 201 | + * are stripped from it. A compaction can't be capped, | |
| 158 | 202 | * so it books COMPACT_MAX_OUTPUT. |
| 159 | 203 | */ |
| 160 | 204 | export function prepareRequest(method: string, path: string, body: string, size: number): PreparedRequest { |
| @@ -185,9 +229,11 @@ | ||
| 185 | 229 | if (reviewRefuse) out.reviewRefuse = reviewRefuse; |
| 186 | 230 | const routing = ROUTING.find((k) => x[k] != null); |
| 187 | 231 | if (routing) out.routing = routing; |
| 188 | - // A review sends every cap as the one booked, and the default when it sets none. | |
| 232 | + // A review sends every cap as the one booked (the default when it sets none), and no paid provider-side tools. | |
| 189 | 233 | const set = caps.filter((k) => x[k] !== undefined); |
| 190 | - if (caps[0] && (!set.length || set.some((k) => x[k] !== estimate.output))) out.reviewBody = JSON.stringify({ ...fields, ...Object.fromEntries((set.length ? set : [caps[0]]).map((k) => [k, estimate.output])) }); | |
| 234 | + const recap = caps[0] && (!set.length || set.some((k) => x[k] !== estimate.output)); | |
| 235 | + const unpaid = withoutPaidTools(fields); | |
| 236 | + if (recap || unpaid !== fields) out.reviewBody = JSON.stringify(recap ? { ...unpaid, ...Object.fromEntries((set.length ? set : [caps[0]!]).map((k) => [k, estimate.output])) } : unpaid); | |
| 191 | 237 | return out; |
| 192 | 238 | } |
| 193 | 239 |
modifiedapps/diffinery/test/e2e/review.test.mjs+9−3View file
| @@ -511,6 +511,7 @@ | ||
| 511 | 511 | (k) => `echo "${k} $(curl -s -o /dev/null -w '%{http_code}' -X POST "$OPENROUTER_BASE_URL/chat/completions" -H "authorization: Bearer $OPENROUTER_API_KEY" -H "content-type: application/json" -d '{"model":"m","max_tokens":5,"${k}":[],"messages":[]}')"`, |
| 512 | 512 | ), |
| 513 | 513 | `curl -s -o /dev/null -X POST "$OPENROUTER_BASE_URL/chat/completions" -H "authorization: Bearer $OPENROUTER_API_KEY" -H "content-type: application/json" -d '{"model":"m","max_completion_tokens":1,"max_tokens":300,"messages":[]}'`, |
| 514 | + `curl -s -o /dev/null -X POST "$ANTHROPIC_BASE_URL/v1/messages" -H "x-api-key: $ANTHROPIC_API_KEY" -H "content-type: application/json" -d '{"model":"m","max_tokens":5,"tools":[{"name":"Read","input_schema":{}},{"type":"web_search_20250305","name":"web_search"},{"type":"web_search_2025_08_26"},{"type":"some_server_tool_20990101","name":"x"},{"type":"bash_20250124","name":"bash"}],"mcp_servers":[{"type":"url","url":"https://x"}],"messages":[]}'`, | |
| 514 | 515 | `curl -s -o /dev/null -X POST "$ANTHROPIC_BASE_URL/v1/messages" -H "x-api-key: $ANTHROPIC_API_KEY" -H "content-type: application/json" -d '${CUT}'`, |
| 515 | 516 | "sleep 1", |
| 516 | 517 | "", |
| @@ -527,7 +528,12 @@ | ||
| 527 | 528 | const text3 = (await ok("GET", `/repos/${owner}/review-demo/diffinery/tasks/${task}/transcript`)).filter((l) => l.run === run3.number).map((l) => l.text).join("\n"); |
| 528 | 529 | for (const k of ["background", "previous_response_id", "conversation", "prompt", "n", "models", "provider", "plugins"]) assert.match(text3, new RegExp(`^${k} 403$`, "m"), text3); |
| 529 | 530 | const served3 = modelRequests.slice(before3); |
| 530 | - assert.equal(served3.length, 4, "the refused requests never reached the provider"); | |
| 531 | + assert.equal(served3.length, 5, "the refused requests never reached the provider"); | |
| 532 | + // Provider-side tools are stripped from a review's requests; its own tools stay. | |
| 533 | + const tooled = JSON.parse(served3[3].body); | |
| 534 | + // An allowlist: dated variants and unknown tool types go too; only client-run tools stay. | |
| 535 | + assert.deepEqual(tooled.tools, [{ name: "Read", input_schema: {} }, { type: "bash_20250124", name: "bash" }]); | |
| 536 | + assert.equal(tooled.mcp_servers, undefined); | |
| 531 | 537 | // Two caps: the largest is booked, and the review sends both as that one. |
| 532 | 538 | const twoCaps = JSON.parse(served3[2].body); |
| 533 | 539 | assert.equal(twoCaps.max_completion_tokens, 300); |
| @@ -538,8 +544,8 @@ | ||
| 538 | 544 | // 42 bytes / 3 + max_tokens 7 for the answer without usage, the streamed answer's own 40,000, |
| 539 | 545 | // and the estimate the cut-off answer was booked, which stays charged since its usage was never read. |
| 540 | 546 | const cut = Math.ceil(Buffer.byteLength(CUT) / 3) + 500; |
| 541 | - // The two-caps answer reports usage 10 in, 5 out. | |
| 542 | - await until("the estimates and the streamed usage to be counted", async () => (await runTokens(run3.number)) === 14 + 7 + 40_000 + 15 + cut, 15_000); | |
| 547 | + // The two-caps and the tools answers each report usage 10 in, 5 out. | |
| 548 | + await until("the estimates and the streamed usage to be counted", async () => (await runTokens(run3.number)) === 14 + 7 + 40_000 + 15 + 15 + cut, 15_000); | |
| 543 | 549 | |
| 544 | 550 | // Per repository: past the payer's daily limit, a request is refused, the check asks for action, and the payer gets one email. |
| 545 | 551 | assert.equal((await asPerson("PUT", S, { ...settings, daily_limit: 1 })).status, 200); |
modifiedapps/diffinery/test/unit/tasks.test.ts+5−0View file
| @@ -50,6 +50,11 @@ | ||
| 50 | 50 | expect(RUNTIMES["claude-code"].needs).toEqual(["anthropic"]); |
| 51 | 51 | expect(RUNTIMES.codex.needs).toEqual(["openai"]); |
| 52 | 52 | expect(RUNTIMES.custom.command("echo hi")).toBe("echo hi"); |
| 53 | + // Reviews run without web tools, which cost extra on the provider. | |
| 54 | + expect(RUNTIMES["claude-code"].command(undefined, undefined, true)).toContain('--disallowedTools "WebSearch,WebFetch"'); | |
| 55 | + expect(RUNTIMES["claude-code"].command()).not.toContain("disallowedTools"); | |
| 56 | + expect(RUNTIMES.codex.command(undefined, undefined, true)).toContain("-c web_search=disabled"); | |
| 57 | + expect(RUNTIMES.codex.command()).not.toContain("web_search"); | |
| 53 | 58 | }); |
| 54 | 59 | }); |
| 55 | 60 |
modifiedapps/diffinery/test/unit/usage.test.ts+27−0View file
| @@ -110,6 +110,33 @@ | ||
| 110 | 110 | // Requests that generate nothing aren't pinned. |
| 111 | 111 | expect(pinRefusal(prep("POST", "/v1/messages/count_tokens", '{"model":"other"}'), pinned)).toBeUndefined(); |
| 112 | 112 | }); |
| 113 | + it("strips paid provider-side tools from a review's request, and keeps the rest", () => { | |
| 114 | + const fn = { name: "Read", input_schema: {} }; | |
| 115 | + const a = prep("POST", "/v1/messages", JSON.stringify({ max_tokens: 9, tools: [fn, { type: "web_search_20250305", name: "web_search" }, { type: "web_fetch_20250910", name: "web_fetch" }, { type: "code_execution_20250825", name: "code_execution" }, { type: "mcp_toolset", mcp_server_name: "x" }], mcp_servers: [{ url: "https://x" }], tool_choice: { type: "auto" } })); | |
| 116 | + expect(JSON.parse(a.reviewBody!)).toEqual({ max_tokens: 9, tools: [fn], tool_choice: { type: "auto" } }); | |
| 117 | + const local = { type: "local_shell" }; | |
| 118 | + const o = prep("POST", "/v1/responses", JSON.stringify({ max_output_tokens: 9, tools: [local, { type: "web_search" }, { type: "web_search_preview_2025_03_11" }, { type: "file_search" }, { type: "code_interpreter" }, { type: "image_generation" }, { type: "mcp", server_url: "https://x" }], tool_choice: { type: "web_search" } })); | |
| 119 | + expect(JSON.parse(o.reviewBody!)).toEqual({ max_output_tokens: 9, tools: [local] }); | |
| 120 | + // An allowlist: dated variants and tool types nobody has invented yet are dropped too; client-run tools stay. | |
| 121 | + const custom = { type: "custom", name: "grep" }; | |
| 122 | + const patch = { type: "apply_patch" }; | |
| 123 | + const f = { type: "function", name: "Read", parameters: {} }; | |
| 124 | + const d = prep("POST", "/v1/responses", JSON.stringify({ max_output_tokens: 9, tools: [f, custom, patch, { type: "web_search_2025_08_26" }, { type: "deep_research_2027_01_01" }, { type: 7 }], tool_choice: { type: "function", name: "Read" } })); | |
| 125 | + expect(JSON.parse(d.reviewBody!)).toEqual({ max_output_tokens: 9, tools: [f, custom, patch], tool_choice: { type: "function", name: "Read" } }); | |
| 126 | + const bash = { type: "bash_20250124", name: "bash" }; | |
| 127 | + const editor = { type: "text_editor_20250728", name: "str_replace_based_edit_tool" }; | |
| 128 | + const m = prep("POST", "/v1/messages", JSON.stringify({ max_tokens: 9, tools: [bash, editor, { type: "memory_20250818", name: "memory" }, { type: "web_search_20260101", name: "web_search" }, { type: "tool_search_tool_regex_20251119", name: "tool_search" }], tool_choice: { type: "tool", name: "web_search" } })); | |
| 129 | + expect(JSON.parse(m.reviewBody!)).toEqual({ max_tokens: 9, tools: [bash, editor, { type: "memory_20250818", name: "memory" }] }); | |
| 130 | + const cf = { type: "function", function: { name: "Read", parameters: {} } }; | |
| 131 | + const c = prep("POST", "/v1/chat/completions", JSON.stringify({ max_tokens: 9, tools: [cf, { type: "web_search" }], tool_choice: { type: "function", function: { name: "Read" } } })); | |
| 132 | + expect(JSON.parse(c.reviewBody!)).toEqual({ max_tokens: 9, tools: [cf], tool_choice: { type: "function", function: { name: "Read" } } }); | |
| 133 | + // With no tools left, neither tools nor a tool choice is sent. | |
| 134 | + expect(JSON.parse(prep("POST", "/v1/responses", JSON.stringify({ max_output_tokens: 9, tools: [{ type: "web_search" }], tool_choice: "required" })).reviewBody!)).toEqual({ max_output_tokens: 9 }); | |
| 135 | + // Nothing to strip and nothing to cap: the request goes as it is. | |
| 136 | + expect(prep("POST", "/v1/messages", JSON.stringify({ max_tokens: 9, tools: [fn] })).reviewBody).toBeUndefined(); | |
| 137 | + // Ordinary tasks keep their tools: only the review body changes. | |
| 138 | + expect(JSON.parse(a.body).tools).toHaveLength(5); | |
| 139 | + }); | |
| 113 | 140 | it("gives reviews a body capped at the estimate's output when the request sets no cap", () => { |
| 114 | 141 | expect(JSON.parse(prep("POST", "/v1/responses", '{"input":"x"}').reviewBody!)).toEqual({ input: "x", max_output_tokens: DEFAULT_MAX_OUTPUT }); |
| 115 | 142 | expect(JSON.parse(prep("POST", "/v1/messages", '{"messages":[]}').reviewBody!)).toEqual({ messages: [], max_tokens: DEFAULT_MAX_OUTPUT }); |