From 6de2fe0101e2804033b6f0f16a414904217359b0 Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Jr <48456829+IBJunior@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:51:13 +0200 Subject: [PATCH] refactor(approval): make the gate unconditional for mutating tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The template shipped a "gate on/off" switch that disabled human approval for every mutating tool. Cameron's first design rule is that it never moves money without an explicit human decision, so a control that turns that off contradicts the product. Removed from every surface a request can reach: the toolbar toggle, the `approveAllTools` context state, the query param, its OpenAPI schema entry, and the field on `MessageOptions`. The route no longer reads such a param, and agentService no longer forwards one to the agent factory. The bypass survives for exactly one caller. The eval harness drives the factory in-process and has no human to answer an interrupt, so without it every mutating case would hang. It is renamed `bypassApprovalForEval` so nothing mistakes it for a product setting, and it is unreachable over HTTP by construction. `approvalGate.test.ts` pins that boundary at each layer. The failure it guards against is silent — a bypass wired back into the route would still typecheck, still stream, and quietly write to a real ledger — so the assertions are source-level, like capabilities.test.ts. Verified against the dev stack: a request sending `approveAllTools=true` explicitly still paused at the gate and wrote nothing, and the ordinary approve path still committed the row. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 10 ++-- docs/ARCHITECTURE.md | 7 ++- eval/README.md | 2 +- eval/cases/approval.cases.mts | 2 +- eval/run.mts | 7 +-- src/app/api/agent/stream/route.ts | 2 - src/app/api/agent/stream/schema.ts | 4 -- src/components/MessageInput.tsx | 29 +---------- src/components/Thread.tsx | 6 +-- src/contexts/UISettingsContext.tsx | 10 ---- src/lib/agent/approvalGate.test.ts | 82 ++++++++++++++++++++++++++++++ src/lib/agent/index.ts | 9 ++-- src/lib/agent/util.ts | 10 +++- src/services/agentService.ts | 4 +- src/services/chatService.ts | 2 - src/types/message.ts | 1 - 16 files changed, 119 insertions(+), 68 deletions(-) create mode 100644 src/lib/agent/approvalGate.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 18211d5..a8bd6e5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -137,7 +137,10 @@ This is a Next.js 15 fullstack AI agent chat application using LangGraph.js with - **Tool Approval**: Human-in-the-loop via `humanInTheLoopMiddleware`. Approval is gated **per-tool** through an `interruptOn` map that lists only **mutating** tools (`log_expense`, `import_transactions_csv`, `create_category`, `set_config`); read tools (incl. `run_sql`) and MCP tools - auto-approve. `approveAllTools` omits the middleware entirely. Decisions: `allow`→approve, + auto-approve. **The gate cannot be switched off from the product**: no UI control, no query + param, and nothing on `MessageOptions` can disable it — `bypassApprovalForEval` omits the + middleware for the eval harness alone, which calls the agent factory in-process and has no human + to answer an interrupt (`approvalGate.test.ts` pins that boundary). Decisions: `allow`→approve, `deny`→reject (with an explanatory follow-up). `MUTATING_TOOL_NAMES` lives in `src/lib/agent/mutatingTools.ts` — a **zero-import leaf**; `index.ts` and `capabilities.ts` both re-use it from there. It is its own module because **client components need it** (the approval @@ -333,7 +336,8 @@ Instructions loaded on demand, in the standard `SKILL.md` format — full detail ### API Route Patterns - Stream endpoints use `dynamic = "force-dynamic"` and `runtime = "nodejs"` -- Query params for streaming: `content`, `threadId`, `model`, `provider`, `allowTool`, `approveAllTools` +- Query params for streaming: `content`, `threadId`, `model`, `provider`, `allowTool` + (deliberately NO approval-bypass param — see the approval workflow above) - MCP server CRUD follows REST patterns in `/api/mcp-servers/route.ts` - File upload endpoint: `/api/agent/upload` accepts multipart/form-data, returns file metadata @@ -456,7 +460,7 @@ pnpm typecheck:eval # free — the root tsc misses this `toolCalled` is a set check — "asked, saved, then logged" and "logged under a guess, then saved" leave the same rows in the same tables, and only the order says which happened. - **Approval cases** set `approval: "allow" | "deny"`, which keeps the HITL middleware live (plain - `approveAllTools` omits it entirely). Paused calls land on `RunCapture.interrupts` — the only + `bypassApprovalForEval` omits it entirely). Paused calls land on `RunCapture.interrupts` — the only evidence the gate fired, since `trajectory` looks identical either way. These cases mutate, so the fixture is re-seeded before each run. - **Every run writes `eval/results/latest.json`** (plus a timestamped copy; both gitignored) with diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index daf7cd4..c715bf5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -114,8 +114,9 @@ async function buildAgent(cfg?: AgentConfigOptions) { const interruptOn = Object.fromEntries( MUTATING_TOOL_NAMES.map((name) => [name, { allowedDecisions: ["approve", "reject"] }]), ); - // approveAllTools omits the middleware entirely, so no interrupt is ever created. - const middleware = cfg?.approveAllTools + // bypassApprovalForEval omits the middleware entirely, so no interrupt is ever created. It is + // reachable ONLY from the eval harness — no request can set it. + const middleware = cfg?.bypassApprovalForEval ? [] : [ humanInTheLoopMiddleware({ @@ -186,7 +187,6 @@ export async function streamResponse(params: { const agent = await ensureAgent({ model: opts?.model, tools: opts?.tools, - approveAllTools: opts?.approveAllTools, }); // Handle tool approval (HITL resume) vs normal input. On resume we read the pending HITL request @@ -838,7 +838,6 @@ logger.info("Agent processing started", { threadId, model: opts?.model, toolCount: tools.length, - approveAllTools: opts?.approveAllTools, }); ``` diff --git a/eval/README.md b/eval/README.md index 27ee803..0d5a2ec 100644 --- a/eval/README.md +++ b/eval/README.md @@ -374,7 +374,7 @@ and tool traffic filtered out — and needs no extra API key, since it uses the ## Approval cases Setting `approval: "allow" | "deny"` on a case runs it with the human-in-the-loop middleware **live** -(without it, `approveAllTools` omits the middleware and nothing ever pauses). The runner reads the +(without it, `bypassApprovalForEval` omits the middleware and nothing ever pauses). The runner reads the pending request from the checkpoint and resumes with a `Command`, mirroring `buildResumeCommand` in [src/services/agentService.ts](../src/services/agentService.ts). diff --git a/eval/cases/approval.cases.mts b/eval/cases/approval.cases.mts index 7dfe5e1..4c09258 100644 --- a/eval/cases/approval.cases.mts +++ b/eval/cases/approval.cases.mts @@ -5,7 +5,7 @@ import type { EvalCase } from "../types.mts"; /** * The approval gate — Cameron's first hard rule: "never moves money without explicit human - * approval." Until now this was UNEVALUABLE, because the harness passed `approveAllTools: true`, + * approval." Until now this was UNEVALUABLE, because the harness passed the approval bypass, * which removes the middleware entirely. * * These cases set `approval`, so the middleware runs for real and the runner answers the interrupt diff --git a/eval/run.mts b/eval/run.mts index 796e80e..69c758d 100644 --- a/eval/run.mts +++ b/eval/run.mts @@ -140,12 +140,13 @@ async function main() { // After the reset, or the seeded settings would be wiped by it. if (testCase.config) await seedConfig(testCase.config); - // A fresh agent per run: no checkpointer state leaks between runs. `approveAllTools` - // omits the HITL middleware entirely, so a case testing the gate must NOT set it. + // A fresh agent per run: no checkpointer state leaks between runs. + // `bypassApprovalForEval` omits the HITL middleware entirely — it is the harness-only + // escape hatch (no HTTP path can set it), so a case testing the gate must NOT set it. const agent = await getAgent({ provider: MODEL.provider, model: MODEL.name, - approveAllTools: !testCase.approval, + bypassApprovalForEval: !testCase.approval, }); const t0 = Date.now(); const capture = await runCase(agent as never, testCase); diff --git a/src/app/api/agent/stream/route.ts b/src/app/api/agent/stream/route.ts index 8448732..bae4d9b 100644 --- a/src/app/api/agent/stream/route.ts +++ b/src/app/api/agent/stream/route.ts @@ -19,7 +19,6 @@ export async function GET(req: NextRequest) { const provider = searchParams.get("provider") || undefined; const allowTool = searchParams.get("allowTool") as "allow" | "deny" | null; const toolsParam = searchParams.get("tools") || ""; - const approveAllTools = searchParams.get("approveAllTools") === "true"; const attachmentsParam = searchParams.get("attachments") || ""; const tools = toolsParam @@ -62,7 +61,6 @@ export async function GET(req: NextRequest) { provider, tools, allowTool: allowTool || undefined, - approveAllTools, attachments, }, }); diff --git a/src/app/api/agent/stream/schema.ts b/src/app/api/agent/stream/schema.ts index bca9da2..c9e3656 100644 --- a/src/app/api/agent/stream/schema.ts +++ b/src/app/api/agent/stream/schema.ts @@ -17,10 +17,6 @@ const StreamQuery = z.object({ .string() .optional() .openapi({ description: "Comma-separated list of enabled tool names" }), - approveAllTools: z - .enum(["true", "false"]) - .optional() - .openapi({ description: "Skip per-tool approval prompts" }), attachments: z .string() .optional() diff --git a/src/components/MessageInput.tsx b/src/components/MessageInput.tsx index f7386dd..ad1fd0b 100644 --- a/src/components/MessageInput.tsx +++ b/src/components/MessageInput.tsx @@ -23,8 +23,7 @@ export const MessageInput = ({ const [attachments, setAttachments] = useState([]); const [isUploading, setIsUploading] = useState(false); - const { provider, setProvider, model, setModel, approveAllTools, setApproveAllTools } = - useUISettings(); + const { provider, setProvider, model, setModel } = useUISettings(); const textareaRef = useRef(null); const fileInputRef = useRef(null); @@ -121,7 +120,6 @@ export const MessageInput = ({ model, provider, tools: [], - approveAllTools: approveAllTools, attachments: attachments.length > 0 ? attachments : undefined, }); setMessage(""); @@ -253,31 +251,6 @@ export const MessageInput = ({ {remainingChars} - {/* The approval gate is Cameron's first rule — the switch that disables it is labelled. */} - -