From 84ecb5b8a381d3e4f332b037482ee0f0dfc6e856 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 11 May 2026 14:21:55 +0800 Subject: [PATCH] feat(cli): add --json-schema for structured output in headless mode (#3598) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): add --json-schema for structured output in headless mode Registers a synthetic `structured_output` tool whose parameter schema IS the user-supplied JSON Schema. In headless mode (`qwen -p`), the first successful call terminates the session and exposes the validated payload via the result message's `structured_result` field. Invalid schemas are rejected at CLI parse time via a new strict Ajv compile helper so they can't silently no-op at runtime. * fix(cli): honour "first structured_output call ends session" + reject non-object root schemas Two review fixes for the `--json-schema` feature: 1. `runNonInteractive` now breaks out of the tool-call loop as soon as the first successful `structured_output` invocation is captured, rather than continuing to execute any trailing tool calls the model emitted in the same turn. This restores the documented single-shot contract and prevents side-effecting tools from running after the final answer has already been accepted. 2. `resolveJsonSchemaArg` rejects schemas whose root `type` is anything other than "object" (or a type array including "object"). Function- calling APIs require tool arguments to be JSON objects, so a schema like `{"type": "array"}` would have registered an unusable synthetic tool the model could never satisfy. Absent `type` and `type: "object"` remain accepted. Adds tests for both paths and updates the existing Ajv-compile test to exercise that path without tripping the new root-type guard first. * fix(cli): also reject root anyOf/oneOf schemas whose branches can't accept objects Addresses a review follow-up: the previous root-object check only inspected the top-level `type` keyword, so a schema like `{"anyOf":[{"type":"array"},{"type":"string"}]}` slipped through even though none of its branches can ever validate the object-shaped arguments that function-calling APIs send. Replace the single `type` check with `schemaRootAcceptsObject`, which recursively walks root-level anyOf/oneOf branches and requires at least one to accept objects. Absent `type`, `type: "object"`, `type: ["object", ...]`, and mixed anyOf branches where one accepts object all still pass. `allOf` is left to Ajv's runtime behaviour — guessing intent across contradictory allOf branches at parse time is fragile. * fix(cli): propagate exitCode from --json-schema failure path + tests Address two PR-3598 review findings: 1. gemini.tsx unconditionally called process.exit(0) after runNonInteractive/runNonInteractiveStreamJson, clobbering the process.exitCode = 1 set by nonInteractiveCli.ts when the model emits plain text instead of the structured_output tool. Switch both call sites to process.exit(process.exitCode ?? 0) so CI can detect the failure via the exit code. 2. nonInteractiveCli.test.ts: strengthen the structured-output success path to assert registry.abortAll() is called and that the stdout result envelope carries the JSON-stringified args under `result` plus the raw object under `structured_result`. Add a retry-path test that mocks executeToolCall to return an error on the first structured_output call, then verifies sendMessageStream is called a second time so the model can retry rather than the session terminating early. * fix(cli): suppress non-structured tool calls when structured_output is in the same turn When --json-schema is active and the model emits a batch like [write_file(...), structured_output(...)], the previous implementation ran the leading side-effecting tool before accepting the structured result, violating the "structured_output is the terminal contract" guarantee. The trailing-only break also let an invalid first structured_output fall through to subsequent tools before the retry turn. Pre-scan the batch: if a structured_output request is present, execute ONLY the first one and skip everything else (leading and trailing). This is consistent with the existing terminal-path semantics — the suppressed tool_use blocks lack a matching tool_result, the same way max-turns / cancellation leave the stream. Adds a test covering the reverse-order [side_effect, structured_output] case alongside the existing trailing-suppression and retry tests. * fix(cli): tighten --json-schema root validation per review feedback Three small holes flagged in the latest pass: 1. `schemaRootAcceptsObject` returned early when a root `type` keyword was present, ignoring sibling `anyOf`/`oneOf`. JSON Schema applies keywords at the same level conjunctively, so e.g. `{type:"object", anyOf:[{type:"string"}]}` is unsatisfiable for any value but used to pass. Now both `type` AND any sibling `anyOf`/`oneOf` must independently admit object. 2. The FatalConfigError text said "Every branch of a root anyOf/oneOf must be satisfiable by an object", but the actual logic only requires *at least one* branch (and tests still accept `anyOf:[object, string]`). Reworded to "at least one branch" so the message matches the behaviour. 3. `compileStrict` used `typeof schema !== 'object'` to gate input, which lets arrays through (`typeof [] === 'object'`). The contract says "schema must be a JSON object", so add an `Array.isArray` check so array input gets the intended error rather than a less helpful Ajv compile message. Tests cover the new rejection paths and the array case. * fix(cli): handle root $ref and allOf in --json-schema accept-object check `schemaRootAcceptsObject` previously only inspected `type`, `anyOf`, and `oneOf` at the root, so a couple of unsatisfiable shapes still slipped through: 1. `{"$ref":"#/$defs/Foo","$defs":{"Foo":{"type":"array"}}}` would be accepted because we don't follow $refs, but registers a synthetic tool whose params resolve to "array" — the model can never produce a valid object. Now reject any root $ref unless the user adds a sibling `type:"object"` as an explicit anchor. 2. `allOf` was deferred to Ajv runtime, but allOf is conjunctive at the same level as `type` / `anyOf` / `oneOf`, so an entry like `{"allOf":[{"type":"object"},{"type":"string"}]}` is unsatisfiable for any value. Walk it like the others, requiring every branch to admit object. Tests cover the new $ref-rejected / $ref+anchor-accepted paths and the allOf reject/accept paths. * fix(cli): explicit exit code from runNonInteractive + pair suppressed tool calls Three review threads on the structured-output flow: 1. The break that ends the for-loop on a successful structured_output call sat *before* the responseParts.push and modelOverride capture. SyntheticOutputTool currently returns neither, so it was safe today — but anyone wiring extra signals into the synthetic tool later would see them silently dropped. Move the break after both captures so the contract is explicit, not implicit. 2. The failure path used to set process.exitCode = 1 and return void, relying on global mutable state across an async boundary. Any cleanup task between runNonInteractive and process.exit could silently turn the structured-output failure into exit 0. Switch runNonInteractive to Promise, return 0 / 1 directly from each function-level exit, and have gemini.tsx use the captured return value. 3. The pre-scan from the prior commit suppresses sibling tool calls when structured_output is in the same turn. On the retry path — when structured_output fails validation — the next-turn payload has tool_result for structured_output but no entry for the suppressed siblings, leaving the prior assistant turn's tool_use blocks unpaired. Anthropic and OpenAI both reject that batch shape, so the retry would surface as an opaque provider error. Synthesize a "skipped" functionResponse for every suppressed call so every tool_use in the prior assistant message has a matching tool_result. Tests cover the new retry-pairing contract and update the existing plain-text-failure test to assert on the return value rather than process.exitCode. * fix: address Copilot follow-up review on --json-schema scaffolding Five small but real findings flagged on the latest pass: 1. core/src/index.ts re-exported `SyntheticOutputTool` via `export type`, but it's a runtime class — that erased it from the emitted JS and would break value imports. Split into a value `export { ... }` and a `export type { StructuredOutputParams }`. 2. The structured-output success path returned without flushing `localQueue` notifications or finalising one-shot monitors. If a background task had already emitted `task_started`, exiting here could drop its paired `task_notification` and leave SDK consumers with unpaired lifecycle events. Mirror the regular terminal path's `flushQueuedNotificationsToSdk` + `finalizeOneShotMonitors` calls before `emitResult`. 3. `schemaRootAcceptsObject` ignored the `not` keyword, so `{not:{type:"object"}}` (which forbids every object value) slipped through. Add a best-effort `not` check that rejects when `not.type` directly excludes object. Deeper negated patterns still fall through to Ajv at runtime. 4. `compileStrict`'s JSDoc claimed it errored on "Ajv versions we can't support", but the function doesn't actually check Ajv versions. Reword to "malformed or uses unsupported draft/features for our Ajv configuration" so the contract matches the implementation. 5. The pre-scan suppressed sibling tool calls but only synthesised tool_result events for them on the retry path — the success path left those tool_use blocks unpaired in the emitted JSONL/stream-json event log. Move the synthesis after the for loop so it runs for both the success break and the validation-failure fall-through; the event log is now consistent regardless of which path the run takes. Tests cover the new \`not\`-rejection paths, the success-path tool_result synthesis, and the existing retry-pairing test still passes against the restructured emit ordering. * fix(cli): tighten --json-schema parse-time gate per Copilot review Two more shapes that used to slip through: 1. `schemaRootAcceptsObject` defaulted to true when no narrowing keyword was present, so root-value constraints like `{const: 1}` or `{enum: [1, 2]}` registered an unsatisfiable structured_output contract — the model could never produce a value matching the tool's parameter schema, and the run would loop on validation failures until max-turns. Reject `const` whose literal isn't an object, and `enum` whose members include no object. 2. The yargs check rejected `--json-schema` with `-i` and with no prompt, but not with `--input-format stream-json`. Stream-json keeps the process open waiting for protocol messages, so "terminate on the first valid structured_output call" silently drops everything queued after that point. Refuse the combination at parse time so the contradiction surfaces immediately. Tests cover the new const/enum reject and accept paths. * fix(cli): handle empty/boolean subschemas + allow stdin-only prompt Three more shapes flagged on the latest review pass: 1. `schemaRootAcceptsObject` treated an empty root `anyOf`/`oneOf` as "no constraint" (skipped when length === 0), but per JSON Schema an empty union is unsatisfiable — no value can match a member of the empty set. Reject those at parse time so users get a clear parse error instead of an opaque runtime never-validates loop. 2. JSON Schema (draft-06+) allows boolean subschemas anywhere a schema is accepted: `true` matches every value, `false` matches nothing. The `anyOf`/`oneOf`/`allOf` walks were rejecting booleans via the typeof-object guard, which incorrectly rejected `{anyOf:[true]}` and `{allOf:[true,{type:"object"}]}` while letting `{anyOf:[false]}` slip through. Replace the per-branch object guard with a `variantAcceptsObject` helper that treats `true` as accepting and `false` as rejecting, then recurses on object subschemas. 3. The yargs `.check` rejected `--json-schema` when no `-p` / positional prompt was given, but the headless CLI also reads the prompt from stdin (`cat prompt.txt | qwen --json-schema '...'`) — a legit usage pattern that was being blocked. Drop the parse-time no-prompt rejection; the existing runtime "No input provided via stdin..." error in gemini.tsx still catches genuinely empty input. Tests cover the empty-union, all-`false`, mixed-boolean accept, and `false`-in-allOf reject paths. Live-verified against the bundled CLI: `echo "..." | qwen --json-schema '...'` now reaches the model call, and the four schema edge cases all surface the expected error text or proceed past parse time. * docs(core): note SyntheticOutputTool as the value-export exception The block comment above the lazy-load type re-exports said tool classes "are now lazy-loaded and are not exported as values from the package root", but `SyntheticOutputTool` was just promoted to a runtime export in 62038527c so the CLI's `--json-schema` flow can construct it from the package root. Document that exception inline so downstream consumers reading the comment don't get told the wrong story. * fix(cli): try every structured_output in a same-turn batch in order The pre-scan used to pick only the FIRST structured_output call from a turn and suppress everything else, even other structured_output calls. That created two avoidable failure modes: 1. `[structured_output(bad), structured_output(good)]` would attempt only the bad one, fail validation, and force a full retry turn. The model already produced a valid structured payload — we should try it before asking again. 2. The trailing structured_output's tool_result was synthesised with the "Skipped: structured_output was also requested in this turn..." message, which is misleading because that call WAS the structured output we should have tried. Filter `requestsToExecute` to ALL structured_output calls (in original order) when --json-schema is active, and let the existing loop break on the first success. Track an `executedCallIds` set, then synthesise tool_result + retry parts after the loop for every tool_use the model emitted that we never actually executed — covering both non-structured siblings (always suppressed) and any structured_output left over after the success break (only one terminal contract per turn). Reworded the synthesised "skipped" output to "this turn's structured_output contract took precedence" so it reads correctly regardless of whether the suppressed call was structured or not. Tests cover the multi-structured retry-free success path; the existing single-structured retry and trailing/leading suppression tests still pass against the updated emit ordering. * fix: address gpt-5.5 review on --json-schema (privacy + $ref + core-tools) Three findings, three changes: 1. Reject every root `$ref` in --json-schema, even with a sibling `type: "object"` anchor. Ajv applies `$ref` conjunctively with sibling keywords, so the previous "accept when type:object is present" carve-out was unsound: `{type:"object",$ref:"#/$defs/Foo", $defs:{Foo:{type:"array"}}}` parsed fine but no object value can satisfy both at runtime, leaving the model to loop until maxTurns. Updated docstring + test cases (replaced the accept-with-anchor case with a reject case for both anchored and well-formed $ref shapes — users wanting composition should inline at the root). 2. Redact `function_args` for structured_output in ToolCallEvent. The args ARE the user's structured payload (already emitted via stdout `result` / `structured_result`); recording them again as ordinary tool-call function_args duplicates that data into OTLP exports, QwenLogger, ui-telemetry, and the chat-recording UI event mirror — surfaces that can leak off-device. Replace with a stable `__redacted` placeholder so consumers still see the call happened (duration, success, decision metrics preserved) but the payload itself doesn't ride along. Two new uiTelemetry tests cover the redacted vs non-redacted paths. 3. Document and test that structured_output bypasses the --core-tools allowlist (same as agent / skill / exit_plan_mode / ask_user_question etc.). The synthetic tool only exists when --json-schema is set, so adding it to CORE_TOOLS would let `--core-tools read_file --json-schema X` silently drop the terminal contract and loop the model until maxTurns — bypass is intentional. Expanded the CORE_TOOLS docstring to enumerate the synthetic-tool exclusions and added a permission-manager test mirroring the pattern used for agent / skill / exit_plan_mode. * fix(cli): apply structured_output terminal handling to drain turns The synthetic structured_output tool is registered for the entire headless session, so it can be invoked from EITHER the main assistant-turn loop OR from a drain turn (queued cron-job / notification reply). The drain path (drainOneItem) was treating it like any other tool: execute, append the response back into itemMessages, keep going. The submitted args were never captured and no structured_result envelope was emitted, so a run that legitimately satisfied --json-schema mid-drain ended up failing the contract with "Model produced plain text..." anyway. Apply the same terminal handling to drain turns: - Hoist `structuredSubmission` to session scope so both paths write to one variable. - In `drainOneItem`, run the same pre-scan: when --json-schema is active and structured_output is in the batch, execute every structured_output in original order until one succeeds; suppress every non-structured sibling. Synthesise tool_results for any unexecuted tool_use the model emitted, mirroring the main path. - On capture, return early from drainOneItem so the drained item's inner while loop stops. - `drainLocalQueue` short-circuits when a captured submission is in flight, so subsequent queued items don't run. - The cron `checkCronDone` watches the same flag and stops the scheduler immediately on capture, releasing the surrounding `await new Promise(...)`. - The final holdback loop bails out on capture so monitor lifecycle doesn't extend past the structured submission. - After the holdback, before the existing failure / regular-success emit, emit the structured success envelope and return 0. Adds a focused unit test that drives the drain path end-to-end via a synchronously-fired monitor notification: main turn produces plain text, the drain reply calls structured_output, and the test asserts exit 0 + structured_result populated + no "Model produced plain text..." error. * fix(cli): address gpt-5.5 review follow-ups on --json-schema scaffolding Six review findings, six small fixes: 1. **Nested $ref incorrectly rejected.** `schemaRootAcceptsObject` recurses into anyOf/oneOf/allOf branches and used to apply the root-only $ref rejection at every level, blocking common composition shapes like `{anyOf:[{$ref:"#/$defs/Foo"},{type:"string"}]}`. Add an `isRoot=true` parameter; non-root recursion treats `$ref` as opaque and defers to Ajv at runtime. Tests cover nested refs in anyOf / oneOf / allOf. 2. **Inaccurate package-root export comment.** `core/src/index.ts` claimed `SyntheticOutputTool` was exported as a runtime value for the CLI's --json-schema flow, but the only construction is inside `Config.registerLazy` via a relative dynamic import — no value consumer reaches into `@qwen-code/qwen-code-core`. Revert to a type-only re-export so `SyntheticOutputTool` lines up with every other lazy-loaded tool class. 3. **Unused constructor parameter.** `SyntheticOutputTool` took `(_config: Config, schema)` but never read `_config`. Drop the parameter (and the corresponding pass-through at the registration call site) so readers don't wonder why a Config is being threaded through. 4. **Tool description claimed "exactly once".** The retry path explicitly tolerates multiple calls until one validates, so "Call this tool exactly once" is misleading to a model that tried twice. Reword to "Call this tool to deliver the final result; the first call with valid arguments ends the session" so the description matches the actual contract. 5. **Asymmetric shutdown on the structured-output success path.** The regular terminal path waits in a holdback loop until `hasUnfinalizedTasks()` is false; the structured-output path used to call `abortAll()` and flush immediately, dropping the matching `task_notification` for any agent whose natural handler hadn't yet enqueued it. Add a bounded holdback (capped at 500ms via STRUCTURED_SHUTDOWN_HOLDBACK_MS) — long enough for typical abort callbacks to enqueue, short enough that a hung agent can't block exit. 6. **gemini.tsx exit-code asymmetry.** `runNonInteractive` returns an explicit exit code, but `runNonInteractiveStreamJson` still reads `process.exitCode` after `runExitCleanup`. Currently safe because the yargs `.check` rejects --json-schema with stream-json input, but a future stream-json equivalent of structured output would need to plumb the exit code through the return value too. Document this in a comment so the constraint is visible at the call site. Plus: strengthen `synthesises tool_result for suppressed sibling calls when structured_output fails validation` to assert the failed structured_output's `functionResponse.response` carries the actual validation error string ("args invalid"), not the synthesised "Skipped:" prose — a regression that overwrote it would otherwise slip past the existing pairing assertion. * fix(cli): close --json-schema gaps surfaced in self-audit + review Five fixes layered onto the same robustness pass over the `--json-schema` flow: 1. **bare-mode registration** (`packages/core/src/config/config.ts`): `qwen --bare --json-schema X -p "..."` previously skipped the synthetic `structured_output` registration entirely (the registration block lives below the bare-mode early-return), so the model had no way to terminate and the run looped to `maxSessionTurns`. Register the synthetic tool inside the bare branch too. 2. **TTY interactive rejection** (`packages/cli/src/gemini.tsx`): `qwen --json-schema X` on a TTY with no `-p` and no piped stdin routes to `isInteractive=true` (priority-3 fallback) and would launch the TUI, where `structured_output` is just an inert tool that prints "accepted" and lets the chat continue. Parse-time gating can't catch this (stdin isn't probed yet at parse time), so reject at runtime before the UI launches; runs `runExitCleanup` first so MCP subprocesses get torn down. 3. **drain-turn structured-success flush** (`packages/cli/src/nonInteractiveCli.ts`): when a drain turn captures `structured_output`, `drainLocalQueue` returns early, leaving any items the drain didn't process in `localQueue`. The prior emit path then ran `registry.abortAll()` + `emitResult` without flushing — stream-json consumers saw `task_started` events without paired `task_notification`. Add the same 500ms holdback + `flushQueuedNotificationsToSdk` the main-turn structured-success path uses, so the two paths agree. 4. **ACP mutual-exclusion** (`packages/cli/src/config/config.ts`): `--acp` runs an independent `runAcpAgent` turn loop that doesn't honour the synthetic-tool terminal contract, so `--acp --json-schema X` would register the tool but never terminate. Add a yargs `.check` rejection covering both `--acp` and the deprecated `--experimental-acp` alias. 5. **max-turns + Skipped wording** (review comments #3198579251/#3198579389/#3198579567 from yiliang114): - `handleMaxTurnsExceededError` now appends a `--json-schema`- specific hint pointing at the common stuck-run causes (structured_output denied by `permissions.deny` / `--exclude-tools`, unsatisfiable schema, prompt didn't instruct the model). Without this, three different failures all surfaced as the same generic "increase maxSessionTurns" line. - The synthesised "Skipped:" tool_result for suppressed sibling calls drops the trailing "Re-issue this call in a separate turn if needed." sentence on the success path, where the session terminates immediately and no consumer (model or SDK) can act on the advice. Retry path keeps the sentence — the model is about to receive these parts and may legitimately re-issue. Tests cover each fix: bare-mode registration order, ACP / experimental- acp rejection (×2), `--json-schema` hint in both text and JSON max-turns output, and explicit Skipped-text assertions on the success and retry paths. * fix: address 9 self-qreview comments on --json-schema PR Folds the 9 Suggestion-level comments from the previous /qreview pass into code/test fixes. Each one is a real issue, but mostly defensive — none changes the user-visible happy path. **Refactors (F4/F5/F6 — code-quality)** - F4 `nonInteractiveCli.ts`: extract `SUPPRESSED_OUTPUT_SUCCESS` / `SUPPRESSED_OUTPUT_RETRY` module-level constants and a `suppressedOutputBody(structuredCaptured)` helper. Both the main-turn and drain-turn synthesis sites previously had a 4-way duplicated ternary; future wording changes can no longer drift between them. - F5 `nonInteractiveCli.ts`: extract `emitStructuredSuccess()` closure inside `runNonInteractive`. The "abortAll → bounded holdback → flush → finalize one-shot monitors → emitResult → return 0" terminal block is now defined once and called from both the main-turn and drain-turn success paths. `finalizeOneShotMonitors` is idempotent (`oneShotMonitorsFinalized` guard) so the unconditional invocation is safe even when the drain-turn already finalized monitors before reaching the helper. - F6 `core/config/config.ts`: extract `registerStructuredOutputIfRequested()` helper. The synthetic-tool registration block is no longer duplicated between the bare-mode early-return branch and the regular registration branch. **Tests (F7/F8/F9 — pin existing behaviour)** - F7 `nonInteractiveCli.test.ts`: new test "holds back for in-flight background tasks before emitting structured success" — flips `hasUnfinalizedTasks: true → false` mid-poll so the holdback `while` body actually executes; spies on `abortAll` and asserts ordering of `task_notification` (must precede the result envelope) and the bounded elapsed-time cap. None of the existing structured-output success tests entered this branch (they all pinned `hasUnfinalizedTasks: () => false`). - F8 `gemini.test.tsx`: new test "rejects --json-schema when running in interactive (TUI) mode" — pins the TUI guard at gemini.tsx:694, asserting the headless-only stderr message AND the exact ordering `writeStderrLine → runExitCleanup → process.exit(1)` so a future refactor can't swap any of those steps. - F9 `cli/config.test.ts`: pin the two previously-untested `--json-schema` mutual-exclusion branches: `-i`/`--prompt-interactive` and `--input-format stream-json`. The stream-json check is load-bearing — `gemini.tsx:768` explicitly relies on this rejection holding (the parse-time `process.exitCode ?? 0` plumbing in the stream-json branch is only safe because `--json-schema` can't reach it). **Behaviour fixes (F1/F2/F15 — privacy / security / correctness)** - F1 `core/core/geminiChat.ts`: redact `functionCall.args` for `structured_output` tool calls before passing them to `chatRecordingService.recordAssistantTurn`. Without this, the user's structured payload (already emitted on stdout via `result` / `structured_result`) was persisted verbatim to `/chats/.jsonl` and re-fed into model context on `--continue` / `--resume`, contradicting the privacy contract documented next to the existing `ToolCallEvent` redaction. Each validation-failure retry was also recorded. Now mirrors the same `__redacted` placeholder. Helper extracted as `redactStructuredOutputArgsForRecording` so it's unit-testable. - F2 `cli/config/config.ts`: `resolveJsonSchemaArg`'s `@path` reader now (a) `fs.statSync`s first to refuse non-regular files (FIFOs, character devices like `/dev/zero`, directories), (b) caps the schema file at 1 MiB so an attacker who can influence the path through a wrapping process can't OOM the run, and (c) on JSON parse failure for `@path` source emits a generic "content of is not valid JSON" instead of echoing the SyntaxError — Node ≥18's SyntaxError embeds a ~10-char file-content prefix in its message, which would otherwise ride out on stderr through any wrapper that surfaces the error. Inline (non-`@path`) JSON keeps the SyntaxError detail because the user is the source. - F15 `core/tools/tool-registry.ts`: `registerTool` now also checks the lazy `factories` map for name collisions, not just the eager `tools` map. An MCP server registering a tool whose name shadows a built-in lazy factory (e.g. `structured_output`) now gets auto-qualified to `mcp____`, instead of silently winning the resolution. The synthetic structured-output tool no longer needs renaming for the corner case to be safe. Targeted suite (13 changed-area test files): 883/886 pass — 3 pre-existing skips. Typecheck clean on both packages. * fix: address 3 deepseek-v4-pro qreview comments on --json-schema PR Three Suggestion-level comments from the latest /qreview pass. **N1 — `schemaRootAcceptsObject` skips `if/then/else`** (cli/config/config.ts): A schema like `{"if": true, "then": {"type": "string"}}` passed parse-time gating but is unsatisfiable for object-typed tool args at runtime — the model would loop until maxSessionTurns. Add a best-effort check for the two decidable shapes: - `if: true` → object MUST match `then`; if `then` excludes objects (boolean `false`, non-object `type`, etc.), reject at parse time. - `if: false` → object MUST match `else` (`true` if absent); same check. Object-schema `if` cases stay runtime-decidable and fall through to Ajv, matching the existing best-effort scope on `not`. 4 new test cases pin both reject and accept paths. **N2 — subagent registries register `structured_output` too** (core/config/config.ts, core/tools/agent/agent.ts, core/agents/backends/InProcessBackend.ts): `createApprovalModeOverride` and `buildSubagentContextOverride` rebuild the tool registry on a `Object.create(base)` config. `this.jsonSchema` propagates through the prototype chain, so `registerStructuredOutputIfRequested` was firing for every subagent registry rebuild — but only `runNonInteractive`'s main / drain loops detect a successful `structured_output` call as terminal. A subagent that called the tool would receive "Session will end now" and then keep running because its own loop has no terminator: wasted tokens, no structured payload on stdout. Add a `forSubAgent: true` option to `createToolRegistry` (alongside the existing `skipDiscovery`), and propagate it from both subagent rebuild sites. The structured-output registration helper short-circuits when the flag is set. Bare-mode init does NOT set the flag, preserving the F6 fix where `qwen --bare --json-schema X -p "..."` still gets the synthetic tool. New test asserts the registry rebuilt with `forSubAgent: true` registers READ_FILE / EDIT / SHELL but NOT STRUCTURED_OUTPUT. **N3 — TEXT-mode `structuredResult` not integration-tested** (nonInteractiveCli.test.ts): All 8 existing `--json-schema` tests pin `OutputFormat.JSON` or `STREAM_JSON`. TEXT (the default for `qwen -p ...`) has no integration coverage, so a regression in `BaseJsonOutputAdapter.buildResultMessage`'s `hasStructured ? JSON.stringify(structuredResult) : resultText` contract or in `JsonOutputAdapter.emitResult`'s text-mode `process.stdout.write(`${result}\n`)` path would only surface to plain `qwen -p` users. New test pins TEXT-mode behaviour: stdout is exactly `${JSON.stringify(structuredArgs)}\n` — no JSON envelope, no event log. Targeted suite (13 spec files): 945/948 pass — 3 pre-existing skips. Typecheck clean on both packages. * fix(cli): narrow `not` rejection in schemaRootAcceptsObject Address Critical review comment #3216123734. `schemaRootAcceptsObject`'s `not` handler previously rejected any schema whose `not.type` included `"object"`, regardless of what other constraints `not` had. That's a false positive for schemas where the extra constraints NARROW what `not` excludes: { "not": { "type": "object", "required": ["error"] } } excludes only objects with an `error` key — the value `{}` satisfies this schema fine, but the old check rejected it at parse time with "--json-schema root must accept object-typed values". Fix: only reject when `not` is exactly `{type: ...}` with no narrowing siblings (the unambiguous "every object is excluded" case). When other keywords are present (`required`, `properties`, `minProperties`, `enum`, etc.), defer to Ajv at runtime — same best-effort scope as the sibling `anyOf`/`oneOf`/`allOf` deep-content checks. 3 new test cases pin the fixed accept paths (`{not:{type:"object",required:[...]}}`, `{not:{type:"object",properties:...,required:[...]}}`, `{not:{type:"object",minProperties:1}}`). The existing reject test for bare `{not:{type:"object"}}` still passes. * refactor: dedupe structured_output handling per qreview C1/C2/C3 Three Suggestion-level review comments from the latest /qreview pass. **C1 — main-turn / drain-turn `structured_output` dispatch was duplicated ~120 lines** (`nonInteractiveCli.ts`) The two batch-handling sites had near-identical bodies (filter `structured_output` from the batch when `--json-schema` is active → iterate with `executeToolCall` → write to `structuredSubmission` on first valid call → synthesise tool_result events for suppressed siblings). The only meaningful difference was which `modelOverride` binding the loop wrote to (session-scoped `modelOverride` for the main turn vs per-drain-item `itemModelOverride`). Extracted `processToolCallBatch(batchRequests, setModelOverride)` defined inside `runNonInteractive`: - Closes over session-scoped state (`adapter`, `config`, `abortController`, `options`, `structuredSubmission`, `executeToolCall`, `handleToolError`, `suppressedOutputBody`, the progress-handler helpers). - Takes the `modelOverride` setter as the one call-site-specific parameter so the main turn binds to the session var and the drain binds to the per-item var. Main-turn body went from ~120 lines to a single call; drain-turn body likewise. Net file shrink ~80 lines, no behaviour change. All 42 existing structured-output tests still pass (including `stops executing remaining tool calls...`, `tries multiple structured_output calls in the same turn...`, `synthesises tool_result for suppressed sibling calls...`, `captures structured_output emitted from a drain-turn (queued notification)`). **C2 + C3 — `{__redacted: '…'}` placeholder duplicated in two files** (`telemetry/types.ts` + `core/geminiChat.ts`) The `ToolCallEvent` constructor (for telemetry surfaces — OTLP / QwenLogger / ui-telemetry / chat-recording UI event mirror) and `redactStructuredOutputArgsForRecording` (for the on-disk chat-recording JSONL) each had a verbatim copy of: { __redacted: 'structured_output payload (see stdout result)' } If the redaction wording (or the `__redacted` key, or the placeholder text) ever drifted between the two surfaces, the privacy contract would be subtly broken on one and not the other. Hoisted to `STRUCTURED_OUTPUT_REDACTED_ARGS` exported from `packages/core/src/tools/syntheticOutput.ts`, imported in both sites. The constant carries its rationale in a JSDoc block so future readers see both call sites at once. Targeted suite (13 spec files): 961/964 pass — 3 pre-existing skips. Typecheck clean on both packages. --------- Co-authored-by: wenshao --- packages/cli/src/config/config.test.ts | 117 ++ packages/cli/src/config/config.ts | 276 +++- packages/cli/src/config/jsonSchemaArg.test.ts | 383 ++++- packages/cli/src/gemini.test.tsx | 97 +- packages/cli/src/gemini.tsx | 48 +- packages/cli/src/nonInteractiveCli.test.ts | 1415 +++++++++++------ packages/cli/src/nonInteractiveCli.ts | 574 ++++--- packages/cli/src/utils/errors.test.ts | 47 + packages/cli/src/utils/errors.ts | 17 +- .../src/agents/backends/InProcessBackend.ts | 2 +- packages/core/src/config/config.test.ts | 76 + packages/core/src/config/config.ts | 46 +- packages/core/src/core/geminiChat.test.ts | 70 + packages/core/src/core/geminiChat.ts | 46 +- .../permissions/permission-manager.test.ts | 14 + .../src/permissions/permission-manager.ts | 12 +- packages/core/src/telemetry/types.ts | 17 +- .../core/src/telemetry/uiTelemetry.test.ts | 59 + packages/core/src/tools/agent/agent.ts | 1 + .../core/src/tools/syntheticOutput.test.ts | 15 +- packages/core/src/tools/syntheticOutput.ts | 33 +- packages/core/src/tools/tool-names.ts | 4 +- packages/core/src/tools/tool-registry.test.ts | 42 + packages/core/src/tools/tool-registry.ts | 17 +- packages/core/src/utils/schemaValidator.ts | 11 +- 25 files changed, 2653 insertions(+), 786 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 266e02093a5..6942c5efc7e 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -254,6 +254,123 @@ describe('parseArguments', () => { expect(argv.prompt).toBeUndefined(); }); + it('rejects --json-schema combined with --acp', async () => { + // ACP runs an independent turn loop (runAcpAgent) that doesn't honour + // the synthetic structured_output terminal contract. The yargs check + // must reject the combination at parse time so users get an actionable + // error instead of silently watching the run never terminate. + process.argv = [ + 'node', + 'script.js', + '--acp', + '--json-schema', + '{"type":"object"}', + ]; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + mockWriteStderrLine.mockClear(); + + await expect(parseArguments()).rejects.toThrow('process.exit called'); + + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('--json-schema cannot be used with --acp'), + ); + + mockExit.mockRestore(); + }); + + it('rejects --json-schema combined with --experimental-acp (deprecated alias)', async () => { + // --experimental-acp is the deprecated alias; the same mutual- + // exclusion logic must apply or users get the silent-no-terminate + // behaviour the --acp check was added to prevent. + process.argv = [ + 'node', + 'script.js', + '--experimental-acp', + '--json-schema', + '{"type":"object"}', + ]; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + mockWriteStderrLine.mockClear(); + + await expect(parseArguments()).rejects.toThrow('process.exit called'); + + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('--json-schema cannot be used with --acp'), + ); + + mockExit.mockRestore(); + }); + + it('rejects --json-schema combined with --prompt-interactive (-i)', async () => { + // The interactive flow doesn't honour the synthetic-tool terminal + // contract — `structured_output` would just print "accepted" and + // leave the chat alive. The yargs check must reject this at parse + // time so users get an actionable message instead of a silently + // misbehaving run. + process.argv = [ + 'node', + 'script.js', + '-i', + 'do work then submit', + '--json-schema', + '{"type":"object"}', + ]; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + mockWriteStderrLine.mockClear(); + + await expect(parseArguments()).rejects.toThrow('process.exit called'); + + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'structured output only terminates the non-interactive flow', + ), + ); + + mockExit.mockRestore(); + }); + + it('rejects --json-schema combined with --input-format stream-json', async () => { + // The "first valid structured_output call ends the session" + // contract is incompatible with the long-lived stream-json input + // protocol. Also load-bearing: gemini.tsx's + // `process.exit(process.exitCode ?? 0)` plumbing in the stream-json + // branch explicitly relies on this rejection holding. Pair with + // --output-format stream-json because input/output formats must + // match (a separate yargs check fires first otherwise). + process.argv = [ + 'node', + 'script.js', + '--input-format', + 'stream-json', + '--output-format', + 'stream-json', + '--json-schema', + '{"type":"object"}', + ]; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + mockWriteStderrLine.mockClear(); + + await expect(parseArguments()).rejects.toThrow('process.exit called'); + + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('first structured_output call ends the session'), + ); + + mockExit.mockRestore(); + }); + it('should parse --system-prompt', async () => { process.argv = [ 'node', diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 51fbcadb304..0968ee0b61a 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -30,9 +30,9 @@ import { NativeLspService, isBareMode, isToolEnabled, + SchemaValidator, type ConfigParameters, type MCPServerConfig, - SchemaValidator, } from '@qwen-code/qwen-code-core'; import { extensionsCommand } from '../commands/extensions.js'; import { hooksCommand } from '../commands/hooks.js'; @@ -170,6 +170,181 @@ export interface CliArgs { inputFile?: string | undefined; } +/** + * Returns true if the root of the given schema can accept a JSON object. + * + * JSON Schema applies sibling keywords conjunctively, so `type`, `anyOf`, + * `oneOf`, and `allOf` at the same level must EACH allow an object — they + * can't rescue one another. For example, `{type:"object", anyOf:[{type:"string"}]}` + * is unsatisfiable for any value because `type` requires object while + * `anyOf` requires string. Walk all four rather than returning on the + * first hit. + * + * For `anyOf` / `oneOf`, at least one branch must admit object (a value + * only has to match one branch). For `allOf`, every branch must admit + * object (a value has to match all of them). Root `$ref` is rejected + * unconditionally — Ajv applies `$ref` conjunctively with sibling + * keywords, so even `{type:"object", $ref:"#/$defs/Foo"}` is + * unsatisfiable when `Foo` resolves to a non-object schema. We don't + * follow refs ourselves (local-only resolution would still need to + * handle remote / recursive refs) so users wanting composition should + * inline the schema at the root or use `allOf`. + * + * The `$ref` rejection is **root-only**. Sub-schemas inside `anyOf` / + * `oneOf` / `allOf` recurse with `isRoot=false`, where a `$ref` is + * treated as opaque (assume-object-compatible) and deferred to Ajv at + * runtime — otherwise common composition shapes like + * `{anyOf:[{$ref:"#/$defs/Foo"}, {type:"string"}]}` would be wrongly + * rejected at parse time even though Ajv can resolve them. + */ +function schemaRootAcceptsObject( + schema: Record, + isRoot = true, +): boolean { + if (isRoot && typeof schema['$ref'] === 'string') { + // Reject any root `$ref`. The previous "accept when sibling + // `type:"object"` is present" carve-out was unsound: Ajv applies + // both keywords, so `{type:"object", $ref:"#/$defs/Foo", + // $defs:{Foo:{type:"array"}}}` parses fine but no object argument + // can satisfy both at runtime — the model would loop forever on + // validation failures. + return false; + } + + const rawType = schema['type']; + const typeIncludesObject = + rawType !== undefined && + (Array.isArray(rawType) ? rawType : [rawType]).includes('object'); + + if (rawType !== undefined && !typeIncludesObject) { + return false; + } + + // Root `const` / `enum` pin the value to specific literals. If those + // literals can never be a JSON object (e.g. `{const: 1}` or + // `{enum: ["a", "b"]}`), no object satisfies the schema — reject. + if ('const' in schema) { + const constVal = schema['const']; + if ( + typeof constVal !== 'object' || + constVal === null || + Array.isArray(constVal) + ) { + return false; + } + } + const enumVal = schema['enum']; + if (Array.isArray(enumVal)) { + const anyObjectMember = enumVal.some( + (v) => typeof v === 'object' && v !== null && !Array.isArray(v), + ); + if (!anyObjectMember) return false; + } + + // JSON Schema (draft-06+) treats `true` and `false` as valid subschemas + // for any keyword that accepts a schema: `true` matches every value, + // `false` matches nothing. Honour those alongside object subschemas so + // shapes like `{anyOf:[true]}` or `{allOf:[true,{type:"object"}]}` pass + // and `{anyOf:[false]}` is rejected. + const variantAcceptsObject = (v: unknown): boolean => { + if (v === true) return true; + if (v === false) return false; + if (typeof v === 'object' && v !== null && !Array.isArray(v)) { + // isRoot=false: nested branches don't trigger the root-only `$ref` + // rejection — the parent's keyword scope already pins the + // sub-schema's role to "candidate value type", and Ajv will + // resolve the ref at runtime. + return schemaRootAcceptsObject(v as Record, false); + } + return false; + }; + + for (const key of ['anyOf', 'oneOf'] as const) { + const variants = schema[key]; + if (Array.isArray(variants)) { + // Empty anyOf/oneOf is unsatisfiable per JSON Schema — no value can + // match a member of an empty union. Reject rather than treating it + // as "no constraint". + if (variants.length === 0) return false; + if (!variants.some(variantAcceptsObject)) return false; + } + } + + const allOf = schema['allOf']; + if (Array.isArray(allOf) && allOf.length > 0) { + // allOf is conjunctive — `false` in any branch makes the schema + // unsatisfiable, `true` is neutral. + if (!allOf.every(variantAcceptsObject)) return false; + } + + // Best-effort `not` handling: when `not` directly forbids object via its + // own `type` keyword (e.g. `{not:{type:"object"}}` or + // `{not:{type:["object","null"]}}`), the schema can never be satisfied + // by an object — reject. We don't try to do full satisfiability analysis + // for arbitrary `not` schemas (e.g. `not:{const:"foo"}` is fine, but + // `not:{anyOf:[{type:"object"},…]}` would also reject objects); those + // fall through to Ajv at runtime. + const notSchema = schema['not']; + if ( + typeof notSchema === 'object' && + notSchema !== null && + !Array.isArray(notSchema) + ) { + const notRecord = notSchema as Record; + const notType = notRecord['type']; + if (notType !== undefined) { + const types = Array.isArray(notType) ? notType : [notType]; + // If `not` is JUST `{type: "object"[…]}` (no additional keywords), + // every object value matches the `not` subschema and so gets + // excluded — schema is unsatisfiable for objects, reject. + // + // If `not` has additional constraints alongside `type` (e.g. + // `{not:{type:"object",required:["error"]}}`), those constraints + // NARROW what `not` excludes: only objects matching ALL of `not`'s + // keywords are rejected, so objects that fail any of the + // narrowing constraints survive. Example: `{}` satisfies + // `{not:{type:"object",required:["error"]}}` because the value + // lacks the `error` key. Rejecting at parse time would be a + // false positive — defer to Ajv at runtime. + if (types.includes('object') && Object.keys(notRecord).length === 1) { + return false; + } + } + } + + // Best-effort `if/then/else` handling for the decidable cases. The + // semantics: if the value matches `if`, it must match `then`; otherwise + // it must match `else` (defaults to `true`). For root-acceptance we can + // only decide statically when `if` is itself a constant boolean + // subschema: + // `if: true` → every object matches `if`, so it MUST match `then`. + // `if: false` → no value matches `if`, so it must match `else`. + // Other shapes for `if` (object schemas) depend on the candidate value + // and fall through to Ajv at runtime — we can't decide acceptance + // without seeing the value. + if ('if' in schema) { + const ifSchema = schema['if']; + if (ifSchema === true) { + // Object MUST match `then` (if absent, defaults to `true`, no + // constraint on root acceptance). + const thenSchema = schema['then']; + if (thenSchema !== undefined && !variantAcceptsObject(thenSchema)) { + return false; + } + } else if (ifSchema === false) { + // Object MUST match `else` (if absent, defaults to `true`). + const elseSchema = schema['else']; + if (elseSchema !== undefined && !variantAcceptsObject(elseSchema)) { + return false; + } + } + // ifSchema is an object schema — runtime Ajv decides; do nothing. + } + + // No narrowing at the root — lenient default, treated as object-compatible. + return true; +} + /** 4 MiB — well above any real schema, well below an accidental * gigabyte-sized file that would OOM `fs.readFileSync` + `JSON.parse`. */ @@ -194,16 +369,25 @@ export function resolveJsonSchemaArg( } let payload: string; + let payloadSource: 'inline' | 'file' = 'inline'; + let payloadSourcePath: string | undefined; if (trimmed.startsWith('@')) { const resolvedPath = resolvePath(trimmed.slice(1)); + payloadSource = 'file'; + payloadSourcePath = resolvedPath; try { - // Cap the read size at 4 MiB. Real-world JSON schemas are well - // under this (the spec encourages decomposition via `$ref`); any - // multi-MiB file is almost certainly a wrong-path mistake or a - // typo'd argument. Without a cap, a multi-GB file (e.g. accidental - // `--json-schema @./node_modules/.cache/*.json`) loads into memory - // before `JSON.parse` even runs and OOMs the CLI. + // Stat first so we can refuse non-regular files (directories, + // character devices like `/dev/zero`, FIFOs that would block + // synchronously) and cap by size before pulling bytes into memory. + // The cap (`MAX_JSON_SCHEMA_FILE_BYTES`) is set well above any real + // schema and well below an accidental gigabyte-sized file that + // would OOM `fs.readFileSync` + `JSON.parse`. const stat = fs.statSync(resolvedPath); + if (!stat.isFile()) { + throw new FatalConfigError( + `--json-schema "@${resolvedPath}" must be a regular file.`, + ); + } if (stat.size > MAX_JSON_SCHEMA_FILE_BYTES) { throw new FatalConfigError( `--json-schema file "${resolvedPath}" is ${stat.size} bytes ` + @@ -230,6 +414,16 @@ export function resolveJsonSchemaArg( try { parsed = JSON.parse(payload); } catch (err) { + // For inline JSON the user IS the source — echoing the SyntaxError + // (which on Node ≥18 embeds a 10-char input snippet) is fine. For + // @path, the error message would leak a prefix of the file's bytes + // through stderr to whatever wrapping process surfaces it; emit a + // generic message instead. + if (payloadSource === 'file') { + throw new FatalConfigError( + `--json-schema content of "${payloadSourcePath}" is not valid JSON.`, + ); + } throw new FatalConfigError( `--json-schema is not valid JSON: ${ err instanceof Error ? err.message : String(err) @@ -243,6 +437,23 @@ export function resolveJsonSchemaArg( ); } + // The schema will be installed as a TOOL PARAMETER schema. All function- + // calling APIs (Gemini/OpenAI/Anthropic) require tool arguments to be a + // JSON object, so a schema that cannot accept objects registers an + // unusable synthetic tool the model could never satisfy. `schemaRootAcceptsObject` + // walks `type`/`const`/`enum`/`anyOf`/`oneOf`/`allOf`/`not`/`if` (with + // best-effort decidable cases for the harder shapes); the strict Ajv + // compile below catches structural validity. The two together cover both + // "schema can be parsed" and "schema can be satisfied by an object value". + if (!schemaRootAcceptsObject(parsed as Record)) { + throw new FatalConfigError( + '--json-schema root must accept object-typed values (tool parameters ' + + 'are always JSON objects). At least one branch of a root anyOf/oneOf ' + + 'must be satisfiable by an object, and a root `type` (when present) ' + + 'must include "object".', + ); + } + // Ajv compile-time validation. SchemaValidator.validate is deliberately // lenient at runtime (falls back to no-op on compile failure to support // exotic MCP schemas) — but `--json-schema` is explicit user intent, so @@ -254,30 +465,6 @@ export function resolveJsonSchemaArg( ); } - // The schema becomes the parameter schema of the synthetic - // structured_output tool, and tool-call arguments are object-shaped. - // A schema like `{"type":"string"}` would compile fine but be - // unsatisfiable as a tool-call argument — fail at parse time so the - // user sees the contract violation immediately instead of at runtime. - // - // We only check the direct `type` field (or array of types). Deeper - // analysis of `anyOf`/`oneOf`/`not` is intentionally not done here: - // the strict-Ajv compile is the right place for full structural - // validation, and a partial check would either give false reassurance - // or wrongly reject valid composed schemas. Schemas with no top-level - // `type` are allowed through (covers `{}`, plain `properties`, etc). - const schemaType = (parsed as { type?: unknown }).type; - const isObjectAllowed = - schemaType === undefined || - schemaType === 'object' || - (Array.isArray(schemaType) && schemaType.includes('object')); - if (!isObjectAllowed) { - throw new FatalConfigError( - `--json-schema top-level type must include "object" (got ${JSON.stringify(schemaType)}); ` + - 'wrap your value under an object property if you need a non-object payload.', - ); - } - return parsed as Record; } @@ -729,12 +916,24 @@ export async function parseArguments(): Promise { if (argv['promptInteractive']) { return '--json-schema cannot be used with --prompt-interactive (-i); structured output only terminates the non-interactive flow.'; } - // Stream-json input runs through runNonInteractiveStreamJson, - // which doesn't honor the structured-output termination - // contract. Reject the combination explicitly so users see - // the mismatch at parse time instead of confusion at runtime. if (argv['inputFormat'] === 'stream-json') { - return '--json-schema cannot be used with --input-format stream-json; structured output is a single-shot contract incompatible with stream-json input.'; + // The "first valid structured_output call ends the session" + // contract assumes a single one-shot prompt. Stream-json + // input keeps the process open waiting for more protocol + // messages, so terminating on the first call would silently + // drop subsequent prompts. Refuse the combination here + // rather than letting the run race to whichever message + // wins. + return '--json-schema cannot be used with --input-format stream-json; the "first structured_output call ends the session" contract is incompatible with the long-lived stream-json input protocol.'; + } + if (argv['acp'] || argv['experimentalAcp']) { + // ACP runs an external IDE/Zed protocol on its own turn loop + // (runAcpAgent), which doesn't honour the synthetic + // structured_output contract. Without this check the tool + // would register but its "session ends now" llmContent would + // just be relayed back into the ACP chat, leaving the run + // open and silently ignoring --json-schema. + return '--json-schema cannot be used with --acp; structured output is only honoured by the headless non-interactive flow.'; } const hasPrompt = !!argv['prompt']; const query = argv['query'] as string | string[] | undefined; @@ -744,7 +943,10 @@ export async function parseArguments(): Promise { // Allow stdin piping (`echo "..." | qwen --json-schema ...`): // when stdin is not a TTY, the prompt is supplied via the pipe // and headless mode runs normally. Only reject true interactive - // invocations with neither flag nor positional nor pipe. + // invocations with neither flag nor positional nor pipe — the + // synthetic tool's "session ends now" llmContent has no + // termination handler in the TUI loop, so silently launching + // the TUI would strand the run. const stdinIsPiped = !process.stdin.isTTY; if (!hasPrompt && !hasPositionalQuery && !stdinIsPiped) { return '--json-schema only applies to non-interactive mode; pass a prompt via -p, as a positional argument, or piped via stdin.'; diff --git a/packages/cli/src/config/jsonSchemaArg.test.ts b/packages/cli/src/config/jsonSchemaArg.test.ts index f24a32b5046..55bab5d731d 100644 --- a/packages/cli/src/config/jsonSchemaArg.test.ts +++ b/packages/cli/src/config/jsonSchemaArg.test.ts @@ -58,11 +58,89 @@ describe('resolveJsonSchemaArg', () => { ).toThrow(/could not read/); }); + it('rejects @path that resolves to a directory', () => { + // stat-based "must be a regular file" guard. Without this, a path + // pointing at a directory would surface a less-specific Node EISDIR + // error from the readFileSync call (or worse, on systems where + // readFileSync on a directory does not error). + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-schema-dir-')); + try { + expect(() => resolveJsonSchemaArg(`@${tmp}`)).toThrow( + /must be a regular file/, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + it('rejects @path schema files that exceed the size cap', () => { + // Defence against a wrapper that forwards a user-supplied path into + // `qwen --json-schema "$X"` where X is e.g. `@/dev/zero` or any + // pathologically large file. We pre-check size via fs.statSync so the + // huge buffer never gets allocated. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-schema-big-')); + const file = path.join(tmp, 'huge.json'); + // Cap is 4 MiB; write 4 MiB + 1 byte to trip it. + fs.writeFileSync(file, Buffer.alloc(4 * 1024 * 1024 + 1, 0x20)); + try { + expect(() => resolveJsonSchemaArg(`@${file}`)).toThrow( + /Refusing to read/, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + it('does not echo the JSON parse error message for @path source', () => { + // The Node ≥18 SyntaxError message for `JSON.parse('hello world…')` + // embeds a ~10-char prefix of the input. For inline JSON that's + // fine — the user typed it themselves — but for @path it would leak + // a prefix of the referenced file through stderr to any wrapper + // that surfaces qwen's error output. Sanitise by emitting a generic + // "content of is not valid JSON" instead. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-schema-bad-')); + const file = path.join(tmp, 'leaky.txt'); + const secretContent = 'SECRET_TOKEN_PREFIX hello world'; + fs.writeFileSync(file, secretContent); + try { + let caught: Error | undefined; + try { + resolveJsonSchemaArg(`@${file}`); + } catch (err) { + caught = err as Error; + } + expect(caught).toBeDefined(); + expect(caught!.message).toMatch(/is not valid JSON/); + // The file's contents must NOT appear in the error message. + expect(caught!.message).not.toContain('SECRET_TOKEN_PREFIX'); + expect(caught!.message).not.toContain('hello worl'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + it('still echoes JSON parse error detail for inline (non-@path) source', () => { + // Inline JSON is the user's own input — keeping the SyntaxError detail + // is helpful for debugging typos, and there's no third-party file + // content to leak. + let caught: Error | undefined; + try { + resolveJsonSchemaArg('{"foo":}'); + } catch (err) { + caught = err as Error; + } + expect(caught).toBeDefined(); + expect(caught!.message).toMatch(/--json-schema is not valid JSON/); + // Should mention the JSON parser detail (echoes SyntaxError text). + expect(caught!.message).not.toBe('--json-schema is not valid JSON: '); + }); + it('throws when schema is syntactically JSON but invalid as a JSON Schema', () => { - // `type` must be a string or array, not a number. - expect(() => resolveJsonSchemaArg('{"type": 42}')).toThrow( - /not a valid JSON Schema/, - ); + // The root-type check fires first for an integer `type`; drop type + // entirely to exercise the Ajv compile-path rejection instead. + expect(() => + resolveJsonSchemaArg('{"properties":{"foo":{"type":42}}}'), + ).toThrow(/not a valid JSON Schema/); }); it('accepts a minimal empty-object schema', () => { @@ -77,34 +155,293 @@ describe('resolveJsonSchemaArg', () => { expect(schema).toBeDefined(); }); - it('rejects schemas whose top-level type is not "object"', () => { - // The schema becomes structured_output's parameter schema; tool args - // are object-shaped, so non-object roots can never be satisfied. - expect(() => resolveJsonSchemaArg('{"type":"string"}')).toThrow( - /top-level type must include "object"/, - ); + it('rejects a schema whose root type is not object', () => { expect(() => resolveJsonSchemaArg('{"type":"array"}')).toThrow( - /top-level type must include "object"/, + /must accept object-typed values/, + ); + expect(() => resolveJsonSchemaArg('{"type":"string"}')).toThrow( + /must accept object-typed values/, ); }); - it('accepts type-arrays that include "object"', () => { - // `{"type":["object","null"]}` is a common nullable-object pattern. - // The earlier guard rejected anything that wasn't exactly the string - // "object", which broke this idiom. + it('accepts a schema whose type array includes "object"', () => { + // Rare but valid; don't over-restrict nullable object roots. + const schema = resolveJsonSchemaArg('{"type":["object","null"]}'); + expect(schema).toEqual({ type: ['object', 'null'] }); + }); + + it('accepts a schema without an explicit root type', () => { + // Absent type is tolerated — Ajv treats it as "anything" which covers + // the object case the model will actually submit. + const schema = resolveJsonSchemaArg('{"properties":{"foo":{}}}'); + expect(schema).toBeDefined(); + }); + + it('rejects root anyOf where no branch accepts object', () => { + expect(() => + resolveJsonSchemaArg('{"anyOf":[{"type":"array"},{"type":"string"}]}'), + ).toThrow(/must accept object-typed values/); + }); + + it('rejects root oneOf where no branch accepts object', () => { + expect(() => + resolveJsonSchemaArg('{"oneOf":[{"type":"number"},{"type":"boolean"}]}'), + ).toThrow(/must accept object-typed values/); + }); + + it('accepts root anyOf when at least one branch accepts object', () => { + const schema = resolveJsonSchemaArg( + '{"anyOf":[{"type":"object"},{"type":"string"}]}', + ); + expect(schema).toBeDefined(); + }); + + it('accepts nested anyOf/oneOf chains where a deep branch accepts object', () => { + // The recursion should see through one level of nesting. + const schema = resolveJsonSchemaArg( + '{"anyOf":[{"oneOf":[{"type":"object"}]},{"type":"string"}]}', + ); + expect(schema).toBeDefined(); + }); + + it('rejects type:"object" combined with an anyOf that excludes object', () => { + // type and anyOf are AND'd at the same level — type:"object" alone is + // not enough if a sibling anyOf forbids every object branch. Without + // this check the synthetic tool would register an unsatisfiable schema. + expect(() => + resolveJsonSchemaArg( + '{"type":"object","anyOf":[{"type":"string"},{"type":"number"}]}', + ), + ).toThrow(/must accept object-typed values/); + }); + + it('accepts type:"object" combined with anyOf where one branch admits object', () => { + const schema = resolveJsonSchemaArg( + '{"type":"object","anyOf":[{"type":"object","properties":{"a":{"type":"string"}}},{"type":"object","properties":{"b":{"type":"number"}}}]}', + ); + expect(schema).toBeDefined(); + }); + + it('rejects any root $ref, even with a sibling type:"object" anchor', () => { + // Ajv applies `$ref` conjunctively with sibling keywords, so a sibling + // `type:"object"` is NOT enough to make the schema satisfiable — when + // the referenced subschema is non-object, the resulting AND is + // unsatisfiable at runtime. We reject root `$ref` outright rather than + // following the reference ourselves (local-only resolution would still + // have to handle remote / recursive refs). + expect(() => + resolveJsonSchemaArg( + '{"$ref":"#/$defs/Foo","$defs":{"Foo":{"type":"array"}}}', + ), + ).toThrow(/must accept object-typed values/); + expect(() => + resolveJsonSchemaArg( + '{"type":"object","$ref":"#/$defs/Foo","$defs":{"Foo":{"type":"array"}}}', + ), + ).toThrow(/must accept object-typed values/); + // Even when the referenced schema IS object-shaped, we still reject — + // the contract for `--json-schema` is "the root schema describes the + // tool args directly", not "follow these refs". Users wanting + // composition should inline at the root or use `allOf`. + expect(() => + resolveJsonSchemaArg( + '{"type":"object","$ref":"#/$defs/Foo","$defs":{"Foo":{"type":"object","properties":{"a":{"type":"string"}}}}}', + ), + ).toThrow(/must accept object-typed values/); + }); + + it('rejects allOf where any branch forbids object at the root', () => { + // allOf is conjunctive — every branch must accept object. A schema + // like `allOf:[{type:"object"}, {type:"string"}]` is unsatisfiable. + expect(() => + resolveJsonSchemaArg('{"allOf":[{"type":"object"},{"type":"string"}]}'), + ).toThrow(/must accept object-typed values/); + }); + + it('accepts allOf where every branch admits object', () => { + const schema = resolveJsonSchemaArg( + '{"allOf":[{"type":"object","properties":{"a":{"type":"string"}}},{"type":"object","required":["a"]}]}', + ); + expect(schema).toBeDefined(); + }); + + it('rejects a root `not` that directly forbids object', () => { + // `not:{type:"object"}` excludes every object value, so the schema is + // unsatisfiable for tool-call args. Best-effort check — only inspects + // `not.type`; deeper negated patterns fall through to Ajv at runtime. + expect(() => resolveJsonSchemaArg('{"not":{"type":"object"}}')).toThrow( + /must accept object-typed values/, + ); + expect(() => + resolveJsonSchemaArg('{"not":{"type":["object","null"]}}'), + ).toThrow(/must accept object-typed values/); + }); + + it('accepts a root `not` whose negated type does not exclude object', () => { + // `not:{type:"string"}` only forbids strings — objects are still fine. + const schema = resolveJsonSchemaArg('{"not":{"type":"string"}}'); + expect(schema).toBeDefined(); + }); + + it('accepts root `not:{type:"object", ...narrowing}` because narrowing keywords leave some objects satisfiable', () => { + // `not:{type:"object",required:["error"]}` only excludes objects + // that have an `error` key. An object like `{}` is NOT excluded + // (it doesn't match the `required` constraint), so the schema is + // satisfiable for at least one object value. + // + // The previous parse-time check looked only at `not.type` and + // rejected this as "must accept object-typed values" — a false + // positive. The fix: only reject when `not` is exactly + // `{type: ...}` with no narrowing siblings; otherwise defer to + // Ajv at runtime. + expect( + resolveJsonSchemaArg('{"not":{"type":"object","required":["error"]}}'), + ).toBeDefined(); expect( resolveJsonSchemaArg( - '{"type":["object","null"],"properties":{"x":{"type":"string"}}}', + '{"not":{"type":"object","properties":{"k":{"type":"string"}},"required":["k"]}}', ), - ).toEqual({ - type: ['object', 'null'], - properties: { x: { type: 'string' } }, - }); + ).toBeDefined(); + expect( + resolveJsonSchemaArg('{"not":{"type":"object","minProperties":1}}'), + ).toBeDefined(); }); - it('rejects type-arrays that do not include "object"', () => { - expect(() => resolveJsonSchemaArg('{"type":["string","number"]}')).toThrow( - /top-level type must include "object"/, + it('rejects a root `const` whose value is not an object', () => { + expect(() => resolveJsonSchemaArg('{"const":1}')).toThrow( + /must accept object-typed values/, + ); + expect(() => resolveJsonSchemaArg('{"const":"hello"}')).toThrow( + /must accept object-typed values/, + ); + expect(() => resolveJsonSchemaArg('{"const":[1,2]}')).toThrow( + /must accept object-typed values/, ); }); + + it('accepts a root `const` whose value is an object', () => { + const schema = resolveJsonSchemaArg( + '{"const":{"summary":"hello","risk":"low"}}', + ); + expect(schema).toBeDefined(); + }); + + it('rejects a root `enum` with no object members', () => { + expect(() => resolveJsonSchemaArg('{"enum":[1,2,"three"]}')).toThrow( + /must accept object-typed values/, + ); + // Empty enum admits nothing — also reject. + expect(() => resolveJsonSchemaArg('{"enum":[]}')).toThrow( + /must accept object-typed values/, + ); + }); + + it('accepts a root `enum` when at least one member is an object', () => { + const schema = resolveJsonSchemaArg( + '{"enum":[{"summary":"a","risk":"low"},{"summary":"b","risk":"high"}]}', + ); + expect(schema).toBeDefined(); + }); + + it('rejects an empty root anyOf / oneOf as unsatisfiable', () => { + expect(() => resolveJsonSchemaArg('{"anyOf":[]}')).toThrow( + /must accept object-typed values/, + ); + expect(() => resolveJsonSchemaArg('{"oneOf":[]}')).toThrow( + /must accept object-typed values/, + ); + }); + + it('accepts boolean subschemas in anyOf where any branch is true', () => { + // `true` matches every value (per JSON Schema 2019-09+), so it admits + // objects. `{anyOf:[true]}` should pass. + const a = resolveJsonSchemaArg('{"anyOf":[true]}'); + expect(a).toBeDefined(); + const b = resolveJsonSchemaArg('{"anyOf":[false,true]}'); + expect(b).toBeDefined(); + const c = resolveJsonSchemaArg('{"anyOf":[true,{"type":"string"}]}'); + expect(c).toBeDefined(); + }); + + it('rejects anyOf where every branch is `false`', () => { + // `false` matches nothing, so an anyOf of all-false is unsatisfiable. + expect(() => resolveJsonSchemaArg('{"anyOf":[false]}')).toThrow( + /must accept object-typed values/, + ); + expect(() => resolveJsonSchemaArg('{"anyOf":[false,false]}')).toThrow( + /must accept object-typed values/, + ); + }); + + it('accepts $ref nested inside anyOf / oneOf / allOf branches', () => { + // Root $ref is rejected unconditionally (Ajv applies it conjunctively + // with siblings), but $ref *inside* a composition branch is opaque + // at parse time — Ajv will resolve it at runtime. Refusing nested + // refs would block common $defs/$ref composition shapes. + const a = resolveJsonSchemaArg( + '{"anyOf":[{"$ref":"#/$defs/Foo"},{"type":"string"}],"$defs":{"Foo":{"type":"object"}}}', + ); + expect(a).toBeDefined(); + const b = resolveJsonSchemaArg( + '{"oneOf":[{"$ref":"#/$defs/A"},{"$ref":"#/$defs/B"}],"$defs":{"A":{"type":"object"},"B":{"type":"object"}}}', + ); + expect(b).toBeDefined(); + const c = resolveJsonSchemaArg( + '{"allOf":[{"$ref":"#/$defs/Bar"},{"type":"object"}],"$defs":{"Bar":{"type":"object"}}}', + ); + expect(c).toBeDefined(); + }); + + it('handles boolean subschemas in allOf', () => { + // `true` is neutral in allOf, `false` makes the whole schema unsatisfiable. + const ok = resolveJsonSchemaArg('{"allOf":[true,{"type":"object"}]}'); + expect(ok).toBeDefined(); + expect(() => + resolveJsonSchemaArg('{"allOf":[false,{"type":"object"}]}'), + ).toThrow(/must accept object-typed values/); + expect(() => resolveJsonSchemaArg('{"allOf":[false]}')).toThrow( + /must accept object-typed values/, + ); + }); + + it('rejects if/then/else when the decidable branch admits no objects', () => { + // `if: true` reduces root acceptance to `then`'s acceptance. + // `if: false` reduces it to `else`'s acceptance. Object schemas in + // `if` are runtime-decidable only and fall through to Ajv. + expect(() => + resolveJsonSchemaArg('{"if":true,"then":{"type":"string"}}'), + ).toThrow(/must accept object-typed values/); + expect(() => resolveJsonSchemaArg('{"if":true,"then":false}')).toThrow( + /must accept object-typed values/, + ); + expect(() => + resolveJsonSchemaArg('{"if":false,"else":{"type":"array"}}'), + ).toThrow(/must accept object-typed values/); + expect(() => resolveJsonSchemaArg('{"if":false,"else":false}')).toThrow( + /must accept object-typed values/, + ); + }); + + it('accepts if/then/else when the decidable branch admits objects', () => { + // `if: true` + object-compatible `then` passes (parse-time + // schemaRootAcceptsObject reduces to checking `then`). + expect( + resolveJsonSchemaArg('{"if":true,"then":{"type":"object"}}'), + ).toBeDefined(); + // `if: false` + object-compatible `else`. + expect( + resolveJsonSchemaArg('{"if":false,"else":{"type":"object"}}'), + ).toBeDefined(); + // Object schema for `if` — runtime-decidable; defer to Ajv. We + // accept at parse time even when `then` excludes object, because + // an object value may not match `if` and so isn't bound by `then`. + expect( + resolveJsonSchemaArg( + '{"if":{"type":"object","properties":{"k":{"const":"x"}}},"then":{"type":"object","properties":{"v":{"type":"string"}}}}', + ), + ).toBeDefined(); + // (The degenerate `{if:true}` / `{if:false}` shapes — no `then` and + // no `else` — are rejected by Ajv strict mode as meaningless rather + // than by schemaRootAcceptsObject; that's fine.) + }); }); diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 6820cbd612c..1c55ecd9c95 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -279,7 +279,7 @@ describe('gemini.tsx main function', () => { vi.mocked(loadSandboxConfig).mockResolvedValue(undefined); vi.mocked(relaunchAppInChildProcess).mockResolvedValue(undefined); vi.mocked(loadCliConfig).mockResolvedValue(configStub); - vi.spyOn(nonInteractiveModule, 'runNonInteractive').mockResolvedValue(); + vi.spyOn(nonInteractiveModule, 'runNonInteractive').mockResolvedValue(0); try { await main(); @@ -711,6 +711,101 @@ describe('gemini.tsx main function kitty protocol', () => { processOnceSpy.mockRestore(); processExitSpy.mockRestore(); }); + + it('rejects --json-schema when running in interactive (TUI) mode', async () => { + // The synthetic structured_output tool only terminates the run inside + // runNonInteractive. In TUI mode it's an inert tool that prints + // "accepted" and leaves the chat alive — silently stranding the run. + // gemini.tsx must reject this combination at runtime (parse-time + // gating can't catch the no-prompt-on-TTY case because stdin + // availability isn't probed yet at parse time). + const { loadCliConfig, parseArguments } = await import( + './config/config.js' + ); + const { loadSettings } = await import('./config/settings.js'); + const cleanupModule = await import('./utils/cleanup.js'); + + const callOrder: string[] = []; + const exitCodes: Array = []; + + mockWriteStderrLine.mockClear(); + mockWriteStderrLine.mockImplementation(() => { + callOrder.push('writeStderrLine'); + }); + const runExitCleanupMock = vi.mocked(cleanupModule.runExitCleanup); + runExitCleanupMock.mockReset(); + runExitCleanupMock.mockImplementation(async () => { + callOrder.push('runExitCleanup'); + }); + const processExitSpy = vi.spyOn(process, 'exit').mockImplementation((( + code?: string | number | null, + ) => { + callOrder.push('processExit'); + exitCodes.push(code); + throw new MockProcessExitError(code); + }) as unknown as typeof process.exit); + + vi.mocked(loadCliConfig).mockResolvedValue({ + isInteractive: () => true, + getJsonSchema: () => ({ type: 'object' }), + getQuestion: () => '', + getSandbox: () => false, + getDebugMode: () => false, + getListExtensions: () => false, + getMcpServers: () => ({}), + initialize: vi.fn(), + getIdeMode: () => false, + getExperimentalZedIntegration: () => false, + getScreenReader: () => false, + getGeminiMdFileCount: () => 0, + getWarnings: () => [], + getModelsConfig: () => ({ getCurrentAuthType: () => null }), + getUsageStatisticsEnabled: () => true, + getSessionId: () => 'test-session-id', + shutdown: vi.fn(), + } as unknown as Config); + vi.mocked(loadSettings).mockReturnValue({ + errors: [], + merged: { + advanced: {}, + security: { auth: {} }, + ui: {}, + }, + setValue: vi.fn(), + forScope: () => ({ settings: {}, originalSettings: {}, path: '' }), + migrationWarnings: [], + getUserHooks: () => undefined, + getProjectHooks: () => undefined, + } as never); + vi.mocked(parseArguments).mockResolvedValue({} as never); + + try { + await main(); + } catch (e) { + if (!(e instanceof MockProcessExitError)) throw e; + } + + // The headless-only message must reach stderr… + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('--json-schema is a headless-only flag'), + ); + // …runExitCleanup must run before exit so MCP subprocesses / + // telemetry exporters registered earlier get torn down… + expect(runExitCleanupMock).toHaveBeenCalledTimes(1); + // …and exit must be 1, not 0. + expect(processExitSpy).toHaveBeenCalledWith(1); + // Order: stderr → cleanup → exit. A regression that swapped any of + // these (cleanup before stderr; exit without cleanup; exit 0 + // instead of 1) would silently strand TUI users. + expect(callOrder).toEqual([ + 'writeStderrLine', + 'runExitCleanup', + 'processExit', + ]); + expect(exitCodes).toEqual([1]); + + processExitSpy.mockRestore(); + }); }); describe('validateDnsResolutionOrder', () => { diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 79b8d8f76bc..5f0aafbef84 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -711,6 +711,24 @@ export async function main() { finalizeStartupProfile(config.getSessionId()); if (config.isInteractive()) { + // --json-schema is a headless-only contract: the synthetic + // structured_output tool only terminates the run inside + // runNonInteractive's main/drain loops. In TUI mode the same call + // would just emit "Structured output accepted." and keep the chat + // alive, which silently strands the user's run. Parse-time gating + // can't catch this case (`qwen --json-schema '...'` on a TTY with + // no prompt routes to interactive only after stdin TTY detection), + // so reject here before the UI launches. + if (config.getJsonSchema?.()) { + writeStderrLine( + 'Error: --json-schema is a headless-only flag. Provide a one-shot prompt via -p / --prompt or pipe one in via stdin.', + ); + // Run cleanup so MCP subprocesses + telemetry exporters that the + // earlier initializeApp() / loadCliConfig() registered get shut + // down — process.exit() doesn't drain them on its own. + await runExitCleanup(); + process.exit(1); + } // Need kitty detection to be complete before we can start the interactive UI. await kittyProtocolDetectionComplete; // Drain the auto-theme probe before render so the OSC 11 response is @@ -773,9 +791,15 @@ export async function main() { settings, ); await runExitCleanup(); - // Honor any exitCode set by the run (e.g. --json-schema plain-text - // path sets it to 1). Hardcoding 0 here would silently mask non-zero - // shell exits so the caller can't detect failures. + // `runNonInteractiveStreamJson` doesn't return an explicit exit + // code yet, so a cleanup task that mutates `process.exitCode` + // could clobber a non-zero failure signal. This is currently safe + // because `--json-schema` is rejected at parse time when combined + // with `--input-format stream-json` (see the yargs `.check` in + // resolveCliGenerationConfig), so structured-output failures + // never reach this branch. If a future stream-json equivalent of + // structured output is added, plumb the exit code through the + // function's return value the way `runNonInteractive` below does. process.exit(process.exitCode ?? 0); } @@ -797,13 +821,19 @@ export async function main() { debugLogger.debug(`Session ID: ${config.getSessionId()}`); - await runNonInteractive(nonInteractiveConfig, settings, input, prompt_id); - // Call cleanup before process.exit, which causes cleanup to not run + const exitCode = await runNonInteractive( + nonInteractiveConfig, + settings, + input, + prompt_id, + ); + // Call cleanup before process.exit, which causes cleanup to not run. + // Capture the exit code BEFORE cleanup so any cleanup task that + // mutates process.exitCode can't silently turn a structured-output + // failure (or other explicit non-zero return from runNonInteractive) + // into a zero exit. await runExitCleanup(); - // Honor any exitCode set by the run (e.g. --json-schema plain-text - // path sets it to 1). Hardcoding 0 here would silently mask non-zero - // shell exits so the caller can't detect failures. - process.exit(process.exitCode ?? 0); + process.exit(exitCode); } } diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index af978ed8f81..c8cd6c1abe4 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -21,9 +21,7 @@ import { FatalInputError, ApprovalMode, SendMessageType, - ToolNames, } from '@qwen-code/qwen-code-core'; -import type { JsonOutputAdapterInterface } from './nonInteractive/io/BaseJsonOutputAdapter.js'; import type { Part } from '@google/genai'; import { runNonInteractive } from './nonInteractiveCli.js'; import { vi, type Mock, type MockInstance } from 'vitest'; @@ -2453,469 +2451,990 @@ describe('runNonInteractive', () => { ); }); - // ---- --json-schema (structured_output) contract -------------------- - // These two cover the runtime branches gpt-5.5 review S8 flagged as - // missing test coverage. We mock the LLM at sendMessageStream so the - // assertions are deterministic; the L2 integration tests in - // integration-tests/cli/json-schema.test.ts run the same flow against - // a real model as smoke coverage. + describe('--json-schema structured output', () => { + // Helper: walk an emitted event and extract the first tool_use_id when + // it represents a tool_result block. Returns undefined for any other + // event shape. + const extractToolResultId = (event: unknown): string | undefined => { + if (typeof event !== 'object' || event === null) return undefined; + const e = event as { + type?: unknown; + message?: { content?: unknown }; + }; + if (e.type !== 'user') return undefined; + const content = e.message?.content; + if (!Array.isArray(content) || content.length === 0) return undefined; + const block = content[0] as { type?: unknown; tool_use_id?: unknown }; + if (block?.type !== 'tool_result') return undefined; + return typeof block.tool_use_id === 'string' + ? block.tool_use_id + : undefined; + }; - function makeMockAdapter(): JsonOutputAdapterInterface { - return { - startAssistantMessage: vi.fn(), - processEvent: vi.fn(), - finalizeAssistantMessage: vi.fn().mockReturnValue({ - type: 'assistant', - uuid: 'mock-uuid', - session_id: 'test-session-id', - parent_tool_use_id: null, - message: { - id: 'mock-uuid', - type: 'message', - role: 'assistant', - model: 'test-model', - content: [], - stop_reason: null, - usage: { input_tokens: 0, output_tokens: 0 }, + it('stops executing remaining tool calls from the same turn once structured_output succeeds', async () => { + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + // Spy on the registry returned by getBackgroundTaskRegistry so we can + // assert abortAll() is called as part of the deterministic shutdown + // contract for structured-output mode. + const abortAllSpy = vi.fn(); + (mockConfig.getBackgroundTaskRegistry as Mock).mockReturnValue({ + setNotificationCallback: vi.fn(), + setRegisterCallback: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + abortAll: abortAllSpy, + }); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + // Same turn: the model emits structured_output FIRST, then a second + // (hypothetical side-effecting) tool. The break must prevent the + // second tool from running. + const structuredArgs = { summary: 'done' }; + const structuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured', + name: 'structured_output', + args: structuredArgs, + isClientInitiated: false, + prompt_id: 'prompt-id-structured', }, - }), - emitResult: vi.fn(), - emitMessage: vi.fn(), - emitUserMessage: vi.fn(), - emitToolResult: vi.fn(), - emitSystemMessage: vi.fn(), - emitToolProgress: vi.fn(), - getSessionId: vi.fn().mockReturnValue('test-session-id'), - getModel: vi.fn().mockReturnValue('test-model'), - } as unknown as JsonOutputAdapterInterface; - } + }; + const trailingCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-trailing', + name: 'side_effect_tool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-id-structured', + }, + }; - it('emits structuredResult and stops when structured_output is called under --json-schema', async () => { - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({ - type: 'object', - required: ['answer'], - properties: { answer: { type: 'number' } }, + mockCoreExecuteToolCall.mockResolvedValue({ + responseParts: [{ text: 'ok' }], + }); + + mockGeminiClient.sendMessageStream.mockReturnValueOnce( + createStreamFromEvents([structuredCall, trailingCall]), + ); + + await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output', + 'prompt-id-structured', + ); + + // Only structured_output should have been executed. The trailing tool + // should have been skipped because structured output ended the session. + expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(1); + const firstCallArg = mockCoreExecuteToolCall.mock.calls[0][1] as { + name: string; + }; + expect(firstCallArg.name).toBe('structured_output'); + + // And we should not have sent a second follow-up turn. + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); + + // abortAll() must be called so any in-flight background agents are + // torn down before we emit the terminal result. + expect(abortAllSpy).toHaveBeenCalledTimes(1); + + const events = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)) + .flat(); + + // The emitted result must carry the submitted args under `result` as + // the JSON-stringified payload (the headless JSON formatter encodes + // the structured submission so SDK consumers always see a string here, + // matching how text-mode `result` is also a string). + const result = events.find( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + expect(result).toBeDefined(); + expect(result.is_error).toBe(false); + expect(typeof result.result).toBe('string'); + expect(JSON.parse(result.result)).toEqual(structuredArgs); + // The raw object is also exposed under `structured_result` for SDK + // consumers that don't want to re-parse the stringified payload. + expect(result.structured_result).toEqual(structuredArgs); + + // The suppressed trailing tool_use must have a synthesised + // tool_result so the event log pairs every tool_use with a + // tool_result, even on the success path. + const trailingToolResult = events.find( + (m: unknown) => extractToolResultId(m) === 'tool-trailing', + ); + expect(trailingToolResult).toBeDefined(); }); - // Single turn: model fires the synthetic structured_output tool with - // its final payload as args, then Finished. No follow-up turn should - // run — runNonInteractive's structured-output branch returns early. - const toolCallEvent: ServerGeminiStreamEvent = { - type: GeminiEventType.ToolCallRequest, - value: { - callId: 'so-1', - name: ToolNames.STRUCTURED_OUTPUT, - args: { answer: 42 }, - isClientInitiated: false, - prompt_id: 'p-structured-success', - }, - }; - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - toolCallEvent, + it('skips side-effecting tool calls that precede structured_output in the same turn', async () => { + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + const abortAllSpy = vi.fn(); + (mockConfig.getBackgroundTaskRegistry as Mock).mockReturnValue({ + setNotificationCallback: vi.fn(), + setRegisterCallback: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + abortAll: abortAllSpy, + }); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + // Same turn, reverse order: a side-effecting tool comes BEFORE + // structured_output. The pre-scan must drop the leading call so the + // side effect never runs — accepting the structured result while + // having already executed write_file would violate the "structured + // output is the terminal contract" guarantee. + const structuredArgs = { summary: 'done' }; + const leadingCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-leading', + name: 'side_effect_tool', + args: { path: '/tmp/should-not-write' }, + isClientInitiated: false, + prompt_id: 'prompt-id-leading', + }, + }; + const structuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured', + name: 'structured_output', + args: structuredArgs, + isClientInitiated: false, + prompt_id: 'prompt-id-leading', + }, + }; + + mockCoreExecuteToolCall.mockResolvedValue({ + responseParts: [{ text: 'ok' }], + }); + + mockGeminiClient.sendMessageStream.mockReturnValueOnce( + createStreamFromEvents([leadingCall, structuredCall]), + ); + + await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output', + 'prompt-id-leading', + ); + + // Only the structured_output call should have been executed; the + // leading side-effect tool must have been suppressed by the pre-scan. + expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(1); + const onlyCallArg = mockCoreExecuteToolCall.mock.calls[0][1] as { + name: string; + }; + expect(onlyCallArg.name).toBe('structured_output'); + // No follow-up turn should have been issued. + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); + expect(abortAllSpy).toHaveBeenCalledTimes(1); + + const events = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)) + .flat(); + const result = events.find( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + expect(result).toBeDefined(); + expect(result.is_error).toBe(false); + expect(result.structured_result).toEqual(structuredArgs); + + // The suppressed leading tool_use must have a synthesised + // tool_result event so the event log pairs every tool_use with a + // tool_result on the success path. + const leadingToolResult = events.find( + (m: unknown) => extractToolResultId(m) === 'tool-leading', + ); + expect(leadingToolResult).toBeDefined(); + // On the success path, the synthesised "Skipped" message must NOT + // include the trailing "Re-issue this call in a separate turn" + // advice — the session terminates immediately so neither the model + // nor any SDK consumer can act on it. Keeps the success-path event + // stream clean and avoids contradictory guidance ("re-issue" + the + // run already exited). + const leadingContent = ( + leadingToolResult as { + message?: { content?: Array<{ content?: string }> }; + } + )?.message?.content?.[0]?.content; + expect(leadingContent).toMatch(/Skipped:/); + expect(leadingContent).not.toMatch(/Re-issue this call/); + }); + + it('tries multiple structured_output calls in the same turn until one succeeds', async () => { + // Same-turn batch: [structured_output(bad), structured_output(good)]. + // The first fails validation; the second has valid args and should + // be tried in-order, ending the session without an extra turn — + // rather than the older behaviour of only attempting the first + // structured_output and forcing a retry. + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + required: ['summary'], + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + const abortAllSpy = vi.fn(); + (mockConfig.getBackgroundTaskRegistry as Mock).mockReturnValue({ + setNotificationCallback: vi.fn(), + setRegisterCallback: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + abortAll: abortAllSpy, + }); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + const goodArgs = { summary: 'ok' }; + const badStructured: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-bad', + name: 'structured_output', + args: { wrong: 'shape' }, + isClientInitiated: false, + prompt_id: 'prompt-id-multi-struct', + }, + }; + const goodStructured: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-good', + name: 'structured_output', + args: goodArgs, + isClientInitiated: false, + prompt_id: 'prompt-id-multi-struct', + }, + }; + + mockGeminiClient.sendMessageStream.mockReturnValueOnce( + createStreamFromEvents([badStructured, goodStructured]), + ); + + // First structured_output returns a tool-execution error (bad args); + // second one returns clean responseParts so the session can capture. + mockCoreExecuteToolCall + .mockResolvedValueOnce({ + error: new Error('args invalid'), + errorType: 'TOOL_INVALID_ARGUMENTS', + responseParts: [ + { + functionResponse: { + id: 'tool-structured-bad', + name: 'structured_output', + response: { error: 'args invalid' }, + }, + }, + ], + }) + .mockResolvedValueOnce({ + responseParts: [{ text: 'ok' }], + }); + + await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output', + 'prompt-id-multi-struct', + ); + + // Both structured_output calls must have been attempted in original + // order; the loop stops at the first success so no third execution. + const executedNames = mockCoreExecuteToolCall.mock.calls.map( + (call) => (call[1] as { name: string; callId: string }).name, + ); + const executedIds = mockCoreExecuteToolCall.mock.calls.map( + (call) => (call[1] as { name: string; callId: string }).callId, + ); + expect(executedNames).toEqual(['structured_output', 'structured_output']); + expect(executedIds).toEqual([ + 'tool-structured-bad', + 'tool-structured-good', + ]); + + // No retry turn was needed. + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); + expect(abortAllSpy).toHaveBeenCalledTimes(1); + + // Result must reflect the second (successful) structured_output's + // submitted args, not a retry payload. + const events = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)) + .flat(); + const result = events.find( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + expect(result).toBeDefined(); + expect(result.is_error).toBe(false); + expect(result.structured_result).toEqual(goodArgs); + }); + + it('keeps the session running when structured_output args fail validation so the model can retry', async () => { + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + required: ['summary'], + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + // First turn: model calls structured_output with invalid args (the + // tool returns a tool-execution error). The session must NOT terminate + // — `!toolResponse.error` keeps `structuredSubmission` undefined and + // we feed the validation failure back so the model can retry. + const invalidStructured: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-invalid', + name: 'structured_output', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-id-retry', + }, + }; + // Second turn: model retries with valid args. + const validStructured: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-valid', + name: 'structured_output', + args: { summary: 'second try' }, + isClientInitiated: false, + prompt_id: 'prompt-id-retry', + }, + }; + + mockGeminiClient.sendMessageStream + .mockReturnValueOnce(createStreamFromEvents([invalidStructured])) + .mockReturnValueOnce(createStreamFromEvents([validStructured])); + + mockCoreExecuteToolCall + .mockResolvedValueOnce({ + error: new Error('args failed schema validation'), + errorType: 'TOOL_INVALID_ARGUMENTS', + resultDisplay: 'missing required field: summary', + responseParts: [ + { text: 'Tool error: args failed schema validation' }, + ], + }) + .mockResolvedValueOnce({ + responseParts: [{ text: 'ok' }], + }); + + await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output', + 'prompt-id-retry', + ); + + // Both attempts must have been executed (no early termination on the + // first call's error). + expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(2); + const firstName = ( + mockCoreExecuteToolCall.mock.calls[0][1] as { name: string } + ).name; + const secondName = ( + mockCoreExecuteToolCall.mock.calls[1][1] as { name: string } + ).name; + expect(firstName).toBe('structured_output'); + expect(secondName).toBe('structured_output'); + + // A second sendMessageStream call confirms the retry turn was issued + // — the failed first attempt did not short-circuit the run. + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); + }); + + it('errors with non-zero exit when model emits plain text instead of structured_output', async () => { + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + const plainTextTurn: ServerGeminiStreamEvent[] = [ + { type: GeminiEventType.Content, value: 'Here is my answer as text.' }, { type: GeminiEventType.Finished, value: { reason: undefined, usageMetadata: { totalTokenCount: 5 } }, }, - ]), - ); - // No error → runtime sets structuredSubmission and terminates. - mockCoreExecuteToolCall.mockResolvedValue({ - responseParts: [{ text: 'Structured output accepted.' }], - }); - - const adapter = makeMockAdapter(); - await runNonInteractive( - mockConfig, - mockSettings, - 'compute 21*2', - 'p-structured-success', - { adapter }, - ); - - // Single-shot contract: no follow-up turn was issued. - expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); - // Background tasks aborted so they don't race the terminal emitResult. - expect(mockBackgroundTaskRegistry.abortAll).toHaveBeenCalledTimes(1); - // structuredResult lands in the result message exactly as the model - // submitted it (no schema massaging at the runtime layer). - expect(adapter.emitResult).toHaveBeenCalledTimes(1); - expect(adapter.emitResult).toHaveBeenCalledWith( - expect.objectContaining({ - isError: false, - structuredResult: { answer: 42 }, - numTurns: 1, - }), - ); - }); - - it('skips remaining tool calls in the same batch after structured_output succeeds', async () => { - // Single-shot contract: a model emitting - // `[structured_output(...), write_file(...)]` in the same response - // must not have write_file run after structured_output records. - // This test pins the break-after-success path; structured_output - // is at index 0 so the pre-scan reorder is a no-op here. See - // sibling test "reorders structured_output before side-effect tools" - // for the reorder coverage. - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({ - type: 'object', - required: ['answer'], - properties: { answer: { type: 'number' } }, - }); - - const sideEffectCallId = 'write-1'; - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: 'so-batch', - name: ToolNames.STRUCTURED_OUTPUT, - args: { answer: 7 }, - isClientInitiated: false, - prompt_id: 'p-batch', - }, - }, - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: sideEffectCallId, - name: 'write_file', - args: { path: '/tmp/should-not-run', content: 'side effect' }, - isClientInitiated: false, - prompt_id: 'p-batch', - }, - }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), - ); - mockCoreExecuteToolCall.mockResolvedValue({ - responseParts: [{ text: 'Structured output accepted.' }], - }); - - const adapter = makeMockAdapter(); - await runNonInteractive(mockConfig, mockSettings, 'go', 'p-batch', { - adapter, - }); - - // executeToolCall called exactly once — for structured_output. - // write_file would be the second call if the loop hadn't broken. - expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(1); - expect(mockCoreExecuteToolCall).toHaveBeenCalledWith( - mockConfig, - expect.objectContaining({ name: ToolNames.STRUCTURED_OUTPUT }), - expect.any(AbortSignal), - expect.any(Object), - ); - // emitResult landed; side-effect tool name should not appear in any - // emitToolResult call. - expect(adapter.emitResult).toHaveBeenCalledWith( - expect.objectContaining({ - isError: false, - structuredResult: { answer: 7 }, - }), - ); - const toolResultCalls = vi.mocked(adapter.emitToolResult).mock.calls; - const sideEffectEmitted = toolResultCalls.some( - ([req]) => req.callId === sideEffectCallId, - ); - expect(sideEffectEmitted).toBe(false); - }); - - it('reorders structured_output before side-effect tools so siblings never run', async () => { - // Pre-scan: when --json-schema is active and the model emitted - // [write_file, structured_output] (struct NOT first), the pre-scan - // must hoist structured_output to position 0 and then the - // break-after-success path skips write_file entirely. Without the - // pre-scan, write_file would persist its side effect BEFORE - // structured_output's terminal flag fired. - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({ - type: 'object', - properties: { answer: { type: 'number' } }, - }); - - const writeFileCallId = 'write-pre-1'; - const structuredCallId = 'so-pre-1'; - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - // Order matters: write_file FIRST, structured_output second. - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: writeFileCallId, - name: 'write_file', - args: { path: '/tmp/should-not-run', content: 'side effect' }, - isClientInitiated: false, - prompt_id: 'p-prescan', - }, - }, - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: structuredCallId, - name: ToolNames.STRUCTURED_OUTPUT, - args: { answer: 1 }, - isClientInitiated: false, - prompt_id: 'p-prescan', - }, - }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), - ); - mockCoreExecuteToolCall.mockResolvedValue({ - responseParts: [{ text: 'Structured output accepted.' }], - }); - - const adapter = makeMockAdapter(); - await runNonInteractive(mockConfig, mockSettings, 'go', 'p-prescan', { - adapter, - }); - - // Exactly ONE executeToolCall — for structured_output. write_file - // never runs because the pre-scan moved it after structured_output - // and the break-after-success path skipped it. - expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(1); - expect(mockCoreExecuteToolCall).toHaveBeenCalledWith( - mockConfig, - expect.objectContaining({ - name: ToolNames.STRUCTURED_OUTPUT, - callId: structuredCallId, - }), - expect.any(AbortSignal), - expect.any(Object), - ); - expect(adapter.emitResult).toHaveBeenCalledWith( - expect.objectContaining({ - isError: false, - structuredResult: { answer: 1 }, - }), - ); - // No tool_result emitted for write_file: it never executed. - const toolResultCalls = vi.mocked(adapter.emitToolResult).mock.calls; - expect( - toolResultCalls.some(([req]) => req.callId === writeFileCallId), - ).toBe(false); - }); - - it('lets siblings run when structured_output validation fails so the model can retry', async () => { - // Validation-failure fallback: if structured_output execute() fails - // (e.g. arg validation rejected the payload), `hasStructuredSubmission` - // stays false and the loop continues normally — sibling tools in the - // batch SHOULD run, same as a turn that didn't issue structured_output - // at all. The model gets the validation error in the tool_result and - // can retry next turn. - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({ - type: 'object', - required: ['answer'], - properties: { answer: { type: 'number' } }, - }); - - const writeFileCallId = 'write-fallback'; - const structuredCallId = 'so-fail'; - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: structuredCallId, - name: ToolNames.STRUCTURED_OUTPUT, - args: { answer: 'not-a-number' }, - isClientInitiated: false, - prompt_id: 'p-fallback', - }, - }, - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: writeFileCallId, - name: 'write_file', - args: { path: '/tmp/x', content: 'sibling' }, - isClientInitiated: false, - prompt_id: 'p-fallback', - }, - }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), - ); - // Second turn (after the sibling tool ran) — model gives up cleanly - // by emitting plain text so the run terminates without - // hasStructuredSubmission ever flipping. - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - { type: GeminiEventType.Content, value: 'gave up' }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), - ); - // structured_output → error; write_file → success. - mockCoreExecuteToolCall.mockImplementation(async (_cfg, req) => { - if (req.name === ToolNames.STRUCTURED_OUTPUT) { - return { - responseParts: [{ text: 'validation error' }], - error: new Error('Schema validation failed'), - errorType: 'TOOL_EXECUTION_ERROR', - } as never; - } - return { responseParts: [{ text: 'wrote' }] } as never; - }); - - const priorExitCode = process.exitCode; - process.exitCode = undefined; - try { - const adapter = makeMockAdapter(); - await runNonInteractive(mockConfig, mockSettings, 'go', 'p-fallback', { - adapter, - }); - - // Both tools executed (one successfully, one failed). The - // critical assertion is that the sibling DID run — fallback - // semantics are restored when structured_output fails. - expect(mockCoreExecuteToolCall).toHaveBeenCalledWith( - mockConfig, - expect.objectContaining({ name: ToolNames.STRUCTURED_OUTPUT }), - expect.any(AbortSignal), - expect.any(Object), - ); - expect(mockCoreExecuteToolCall).toHaveBeenCalledWith( - mockConfig, - expect.objectContaining({ name: 'write_file' }), - expect.any(AbortSignal), - expect.any(Object), - ); - // No structuredResult emitted — the failure didn't terminate via - // the success branch. The plain-text terminal branch fires - // instead (model gave up on turn 2). - const resultCalls = vi.mocked(adapter.emitResult).mock.calls; - const successCall = resultCalls.find( - ([opts]) => - 'structuredResult' in (opts as Record) && - !(opts as { isError?: boolean }).isError, - ); - expect(successCall).toBeUndefined(); - } finally { - process.exitCode = priorExitCode; - } - }); - - it('terminates even when structured_output args are undefined under an empty schema', async () => { - // Pin the boolean-sentinel contract: the previous - // `structuredSubmission !== undefined` check broke if the model - // called structured_output with args missing/undefined (which can - // happen under a permissive `{}` schema, since validateToolParams - // would have already accepted the call). The run must still - // terminate on the first successful structured_output call. - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({}); - - mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - { - type: GeminiEventType.ToolCallRequest, - value: { - callId: 'so-undef', - name: ToolNames.STRUCTURED_OUTPUT, - args: undefined as unknown as Record, - isClientInitiated: false, - prompt_id: 'p-structured-undef', - }, - }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), - ); - mockCoreExecuteToolCall.mockResolvedValue({ - responseParts: [{ text: 'Structured output accepted.' }], - }); - - const adapter = makeMockAdapter(); - await runNonInteractive( - mockConfig, - mockSettings, - 'go', - 'p-structured-undef', - { adapter }, - ); - - // Single turn — boolean sentinel kicked us out even though the args - // value itself is undefined. - expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); - expect(adapter.emitResult).toHaveBeenCalledTimes(1); - expect(adapter.emitResult).toHaveBeenCalledWith( - expect.objectContaining({ - isError: false, - structuredResult: undefined, - }), - ); - }); - - it('sets process.exitCode=1 and writes stderr when model emits text under --json-schema', async () => { - setupMetricsMock(); - vi.mocked(mockConfig.getJsonSchema).mockReturnValue({ - type: 'object', - required: ['answer'], - properties: { answer: { type: 'string' } }, - }); - - // Snapshot/restore the global so a stray exitCode=1 doesn't bleed into - // sibling tests in the file. - const priorExitCode = process.exitCode; - process.exitCode = undefined; - - try { + ]; mockGeminiClient.sendMessageStream.mockReturnValueOnce( - createStreamFromEvents([ - { type: GeminiEventType.Content, value: 'plain answer' }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } }, - }, - ]), + createStreamFromEvents(plainTextTurn), ); - const adapter = makeMockAdapter(); + const exitCode = await runNonInteractive( + mockConfig, + mockSettings, + 'Should call structured_output', + 'prompt-id-plaintext', + ); + expect(exitCode).toBe(1); + + const result = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)) + .flat() + .find( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + expect(result?.is_error).toBe(true); + expect(result?.error?.message).toMatch(/structured_output/); + }); + + it('synthesises tool_result for suppressed sibling calls when structured_output fails validation', async () => { + // Same-turn batch: [side_effect_tool, structured_output(bad)]. The + // pre-scan suppresses the side_effect_tool; structured_output then + // fails validation. The retry turn must still pair both tool_use + // blocks from the prior assistant message with tool_result blocks, + // or providers like Anthropic reject the request. We synthesise a + // "skipped" functionResponse for every suppressed call. + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + required: ['summary'], + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.JSON); + setupMetricsMock(); + + const leadingCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-leading', + name: 'side_effect_tool', + args: { path: '/tmp/should-not-write' }, + isClientInitiated: false, + prompt_id: 'prompt-id-suppress-pair', + }, + }; + const badStructuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-bad', + name: 'structured_output', + args: { wrong: 'shape' }, + isClientInitiated: false, + prompt_id: 'prompt-id-suppress-pair', + }, + }; + const goodStructuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-good', + name: 'structured_output', + args: { summary: 'retry ok' }, + isClientInitiated: false, + prompt_id: 'prompt-id-suppress-pair', + }, + }; + + mockGeminiClient.sendMessageStream + .mockReturnValueOnce( + createStreamFromEvents([leadingCall, badStructuredCall]), + ) + .mockReturnValueOnce(createStreamFromEvents([goodStructuredCall])); + + // First call (the bad structured_output) returns an error response; + // second call (the retry's good structured_output) succeeds. + mockCoreExecuteToolCall + .mockResolvedValueOnce({ + error: new Error('args invalid'), + errorType: 'TOOL_INVALID_ARGUMENTS', + responseParts: [ + { + functionResponse: { + id: 'tool-structured-bad', + name: 'structured_output', + response: { error: 'args invalid' }, + }, + }, + ], + }) + .mockResolvedValueOnce({ + responseParts: [{ text: 'ok' }], + }); + await runNonInteractive( mockConfig, mockSettings, - 'q', - 'p-structured-text', - { adapter }, + 'Emit structured output', + 'prompt-id-suppress-pair', ); - expect(process.exitCode).toBe(1); - // adapter sees the contract violation as an error result, with - // diagnostic context: turn count + truncated preview of model's - // plain-text output ("plain answer" from the mocked stream). - // The adapter is responsible for surfacing the message per output - // format (TEXT → stderr; JSON / STREAM_JSON → structured result); - // runNonInteractive no longer writes a duplicate stderr line. - expect(adapter.emitResult).toHaveBeenCalledTimes(1); - expect(adapter.emitResult).toHaveBeenCalledWith( - expect.objectContaining({ - isError: true, - errorMessage: expect.stringMatching( - /Model produced plain text.+after 1 turn\(s\).+Output preview.+plain answer/s, - ), - }), + // The side-effect tool must NEVER have been executed. + const executedNames = mockCoreExecuteToolCall.mock.calls.map( + (call) => (call[1] as { name: string }).name, ); - } finally { - process.exitCode = priorExitCode; - } + expect(executedNames).toEqual(['structured_output', 'structured_output']); + + // The retry message sent to the model must contain BOTH a tool_result + // for the suppressed side_effect_tool and one for the failed + // structured_output, so every prior tool_use is paired. + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); + const retryParts = mockGeminiClient.sendMessageStream.mock.calls[1][0] as + | Array<{ + functionResponse?: { id?: string; name?: string }; + }> + | undefined; + const retryPartsTyped = (retryParts || []) as Array<{ + functionResponse?: { + id?: string; + name?: string; + response?: unknown; + }; + }>; + const responseIds = retryPartsTyped + .map((p) => p.functionResponse?.id) + .filter(Boolean); + expect(responseIds).toContain('tool-leading'); + expect(responseIds).toContain('tool-structured-bad'); + const suppressed = retryPartsTyped.find( + (p) => p.functionResponse?.id === 'tool-leading', + ); + expect(suppressed?.functionResponse?.name).toBe('side_effect_tool'); + // On the retry path the suppressed call's synthesised body must keep + // the "Re-issue this call" guidance: the model is about to receive + // these parts and may legitimately want to retry the suppressed call + // in the next turn (the structured contract didn't terminate yet). + const suppressedOutput = JSON.stringify( + suppressed?.functionResponse?.response, + ); + expect(suppressedOutput).toMatch(/Skipped:/); + expect(suppressedOutput).toMatch(/Re-issue this call/); + + // The failed structured_output's tool_result must carry the actual + // validation error from `executeToolCall` so the model has signal + // to correct itself on the retry — a regression that overwrote it + // with the synthesised "Skipped" message would leave the model + // blind. Assert the shape: the bad call's response carries the + // validation error string, not the suppressed-output prose. + const failedStructured = retryPartsTyped.find( + (p) => p.functionResponse?.id === 'tool-structured-bad', + ); + expect(failedStructured?.functionResponse?.name).toBe( + 'structured_output', + ); + expect( + JSON.stringify(failedStructured?.functionResponse?.response), + ).toContain('args invalid'); + expect( + JSON.stringify(failedStructured?.functionResponse?.response), + ).not.toMatch(/Skipped:/); + }); + + it('captures structured_output emitted from a drain-turn (queued notification)', async () => { + // Main turn ends with plain text → control falls into the drain + // block. A monitor notification then arrives and the model's reply + // to it calls structured_output. The synthetic tool is registered + // for the whole session, so the drain turn must apply the same + // terminal handling as the main loop — capture the args, abort + // background work, and emit the structured success envelope. + // Without this fix the drain treated structured_output as a regular + // tool, sent its response back to the model, and the run exited + // with the "Model produced plain text..." failure even though a + // valid structured payload had already been accepted. + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + required: ['summary'], + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue( + OutputFormat.STREAM_JSON, + ); + (mockConfig.getIncludePartialMessages as Mock).mockReturnValue(false); + setupMetricsMock(); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + // Inject a monitor notification synchronously when the registry + // wires up — same trick the existing notification tests use to + // enqueue a drain item before the first turn runs. + const notificationXml = + '\n' + + 'mon_1\n' + + 'monitor\n' + + 'running\n' + + 'Monitor emitted event #1.\n' + + 'ready\n' + + ''; + mockMonitorRegistry.setNotificationCallback.mockImplementation((cb) => { + if (!cb) return; + cb('Monitor "logs" event #1: ready', notificationXml, { + monitorId: 'mon_1', + toolUseId: 'tool_mon_1', + status: 'running', + eventCount: 1, + }); + }); + + const drainStructuredArgs = { summary: 'drain-captured' }; + const drainStructuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-drain-structured', + name: 'structured_output', + args: drainStructuredArgs, + isClientInitiated: false, + prompt_id: 'prompt-drain-struct', + }, + }; + + // First turn: plain text, no tool calls — drains into the queue. + // Drain turn: model invokes structured_output as the reply to the + // notification. + mockGeminiClient.sendMessageStream + .mockReturnValueOnce( + createStreamFromEvents([ + { type: GeminiEventType.Content, value: 'Monitor launched.' }, + { + type: GeminiEventType.Finished, + value: { + reason: undefined, + usageMetadata: { totalTokenCount: 2 }, + }, + }, + ]), + ) + .mockReturnValueOnce(createStreamFromEvents([drainStructuredCall])); + + mockCoreExecuteToolCall.mockResolvedValue({ + responseParts: [{ text: 'ok' }], + }); + + const exitCode = await runNonInteractive( + mockConfig, + mockSettings, + 'Watch the logs', + 'prompt-drain-struct', + ); + + // The drain turn captured structured_output → success exit, not the + // "Model produced plain text..." failure path. + expect(exitCode).toBe(0); + + // Two stream calls: main + drain reply. structured_output executed + // exactly once (during drain). + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); + expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(1); + const drainCallArg = mockCoreExecuteToolCall.mock.calls[0][1] as { + name: string; + }; + expect(drainCallArg.name).toBe('structured_output'); + + // The terminating result event must carry the drain-captured args + // under structured_result, not be flagged as an error. + const events = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)); + const result = events.find( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + expect(result).toBeDefined(); + expect(result.is_error).toBe(false); + expect(result.structured_result).toEqual(drainStructuredArgs); + }); + + it('holds back for in-flight background tasks before emitting structured success', async () => { + // The structured-success terminal block has a bounded holdback: + // `while (Date.now() < holdbackDeadline && registry.hasUnfinalizedTasks())` + // sleeping 50 ms between polls. All other success-path tests pin + // `hasUnfinalizedTasks: () => false`, so the loop body never + // enters and the cap, polling, and ordering of flush + finalize + // are unverified. This test flips `hasUnfinalizedTasks` true → + // false mid-run so the body executes at least once, and asserts + // (a) the structured success result still emits, (b) the + // suppressed in-flight task's `task_notification` is flushed + // BEFORE the result event in the SDK output stream. + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue( + OutputFormat.STREAM_JSON, + ); + setupMetricsMock(); + + const abortAllSpy = vi.fn(); + // Returns true once, then false. After abortAll() is called the + // holdback's `while` body executes one iteration of `setTimeout(50)` + // and re-checks; on the second call we report tasks finalized. + let unfinalizedCalls = 0; + const hasUnfinalizedTasksSpy = vi.fn(() => { + unfinalizedCalls++; + return unfinalizedCalls === 1; + }); + // Capture the notification callback so we can fire a + // `task_notification` from "the agent's natural handler" during + // the holdback. Without flushing localQueue before emitResult, + // this notification would be silently dropped. + let notificationCallback: + | (( + displayText: string, + modelText: string, + meta: { + agentId: string; + toolUseId?: string; + status: string; + stats?: unknown; + }, + ) => void) + | null = null; + (mockConfig.getBackgroundTaskRegistry as Mock).mockReturnValue({ + setNotificationCallback: vi.fn((cb) => { + notificationCallback = cb; + }), + setRegisterCallback: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + hasUnfinalizedTasks: hasUnfinalizedTasksSpy, + abortAll: vi.fn(() => { + abortAllSpy(); + // The natural cancel-handler enqueues the terminal + // task_notification synchronously when abortAll is invoked. + // Fire the captured callback immediately so it lands in + // localQueue before the holdback flush runs. + notificationCallback?.( + 'Agent cancelled: bg-task-1', + 'Agent bg-task-1 was cancelled', + { + agentId: 'bg-task-1', + toolUseId: 'tool-bg-1', + status: 'cancelled' as never, + }, + ); + }), + }); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + const structuredArgs = { summary: 'done' }; + const structuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured', + name: 'structured_output', + args: structuredArgs, + isClientInitiated: false, + prompt_id: 'prompt-id-holdback', + }, + }; + mockCoreExecuteToolCall.mockResolvedValue({ + responseParts: [{ text: 'ok' }], + }); + mockGeminiClient.sendMessageStream.mockReturnValueOnce( + createStreamFromEvents([structuredCall]), + ); + + const startedAt = Date.now(); + const exitCode = await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output', + 'prompt-id-holdback', + ); + const elapsed = Date.now() - startedAt; + + expect(exitCode).toBe(0); + expect(abortAllSpy).toHaveBeenCalledTimes(1); + // The holdback while-body must have executed at least one poll. + expect(unfinalizedCalls).toBeGreaterThanOrEqual(2); + // …but it must NOT exceed the 500 ms cap by a meaningful margin. + // 1000 ms is generous (test env CI noise) while still proving the + // cap exists; without the cap, an infinitely-true + // hasUnfinalizedTasks would never return. + expect(elapsed).toBeLessThan(1000); + + // Find the result event and the simulated cancellation + // task_notification. The notification must appear BEFORE the + // result event in the JSONL output, proving + // flushQueuedNotificationsToSdk(localQueue) ran before emitResult. + const lines = writes + .join('') + .split('\n') + .filter((line) => line.trim().length > 0); + const events = lines.map((line) => JSON.parse(line)); + const resultIdx = events.findIndex( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string }).type === 'result', + ); + const taskNotificationIdx = events.findIndex( + (m: unknown) => + typeof m === 'object' && + m !== null && + (m as { type?: string; subtype?: string }).type === 'system' && + (m as { subtype?: string }).subtype === 'task_notification', + ); + expect(resultIdx).toBeGreaterThan(-1); + expect(taskNotificationIdx).toBeGreaterThan(-1); + expect(taskNotificationIdx).toBeLessThan(resultIdx); + }); + + it('emits structuredResult to stdout in OutputFormat.TEXT mode', async () => { + // The other --json-schema tests pin OutputFormat.JSON / + // OutputFormat.STREAM_JSON. TEXT is the default for headless runs + // (`qwen -p "..."` without --output-format), so it needs its own + // pin: a regression that diverged the TEXT adapter's + // structuredResult handling from the JSON / stream-json paths + // would only surface to users running plain `qwen -p`. + (mockConfig.getJsonSchema as Mock).mockReturnValue({ + type: 'object', + properties: { summary: { type: 'string' } }, + }); + (mockConfig.getOutputFormat as Mock).mockReturnValue(OutputFormat.TEXT); + setupMetricsMock(); + + (mockConfig.getBackgroundTaskRegistry as Mock).mockReturnValue({ + setNotificationCallback: vi.fn(), + setRegisterCallback: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + abortAll: vi.fn(), + }); + + const writes: string[] = []; + processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => { + writes.push( + typeof chunk === 'string' + ? chunk + : Buffer.from(chunk).toString('utf8'), + ); + return true; + }); + + const structuredArgs = { summary: 'text-mode-ok' }; + const structuredCall: ServerGeminiStreamEvent = { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'tool-structured-text', + name: 'structured_output', + args: structuredArgs, + isClientInitiated: false, + prompt_id: 'prompt-id-text', + }, + }; + mockCoreExecuteToolCall.mockResolvedValue({ + responseParts: [{ text: 'ok' }], + }); + mockGeminiClient.sendMessageStream.mockReturnValueOnce( + createStreamFromEvents([structuredCall]), + ); + + const exitCode = await runNonInteractive( + mockConfig, + mockSettings, + 'Emit structured output (text mode)', + 'prompt-id-text', + ); + + expect(exitCode).toBe(0); + // TEXT mode writes the JSON-stringified structured payload as the + // result on stdout (BaseJsonOutputAdapter.buildResultMessage forces + // `result = JSON.stringify(structuredResult)` when the field is + // set; JsonOutputAdapter writes `result` directly to stdout in + // TEXT mode). The line should be exactly the stringified args plus + // a trailing newline — no JSON envelope, no extra event log. + const stdout = writes.join(''); + expect(stdout).toBe(`${JSON.stringify(structuredArgs)}\n`); + }); }); }); diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index 1c30d426d97..b1389452ddf 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -43,6 +43,43 @@ import { handleCancellationError, handleMaxTurnsExceededError, } from './utils/errors.js'; + +const debugLogger = createDebugLogger('NON_INTERACTIVE_CLI'); + +/** + * Maximum wait, in milliseconds, for in-flight background tasks to emit + * their terminal `task_notification` after `abortAll()` on the + * structured-output success path. Tasks are marked cancelled + * synchronously by `abortAll`, but the natural task handler emits the + * notification on a later microtask — without a brief holdback the + * structured-output run would silently drop those events. Capped so a + * slow agent can't block exit indefinitely. + */ +const STRUCTURED_SHUTDOWN_HOLDBACK_MS = 500; + +/** + * Body of the synthesised `tool_result` for a `tool_use` block that was + * suppressed because a sibling `structured_output` call took precedence + * as the terminal output for the same turn. + * + * Two variants — the success-path body drops the trailing "Re-issue this + * call in a separate turn if needed." sentence because the session + * terminates immediately after synthesis (no model or SDK consumer can + * act on the advice). The retry-path body keeps it: when the structured + * call failed validation, the model is about to receive these parts in + * the next turn and may legitimately re-issue the suppressed call. + * + * Shared between the main-turn and drain-turn synthesis sites so a + * future wording change can't desync them. + */ +const SUPPRESSED_OUTPUT_SUCCESS = + "Skipped: this turn's structured_output contract took precedence as the terminal output."; +const SUPPRESSED_OUTPUT_RETRY = `${SUPPRESSED_OUTPUT_SUCCESS} Re-issue this call in a separate turn if needed.`; +function suppressedOutputBody(structuredCaptured: boolean): string { + return structuredCaptured + ? SUPPRESSED_OUTPUT_SUCCESS + : SUPPRESSED_OUTPUT_RETRY; +} import { normalizePartList, extractPartsFromUserMessage, @@ -52,8 +89,6 @@ import { computeUsageFromMetrics, } from './utils/nonInteractiveHelpers.js'; -const debugLogger = createDebugLogger('NON_INTERACTIVE_CLI'); - // Human-readable labels for the detectors that can fire mid-stream. // Surfaced to stderr in TEXT mode so a headless run that halts on a loop // doesn't exit with empty stdout and no explanation — see PR #3236 review. @@ -156,8 +191,8 @@ export async function runNonInteractive( input: string, prompt_id: string, options: RunNonInteractiveOptions = {}, -): Promise { - return promptIdContext.run(prompt_id, async () => { +): Promise { + return promptIdContext.run(prompt_id, async (): Promise => { // Create output adapter based on format let adapter: JsonOutputAdapterInterface; const outputFormat = config.getOutputFormat(); @@ -289,7 +324,7 @@ export async function runNonInteractive( config, startTimeMs: startTime, }); - return; + return slashCommandResult.messageType === 'error' ? 1 : 0; } case 'stream_messages': throw new FatalInputError( @@ -303,7 +338,7 @@ export async function runNonInteractive( config, startTimeMs: startTime, }); - return; + return 1; } case 'no_command': break; @@ -417,12 +452,237 @@ export async function runNonInteractive( let isFirstTurn = true; let modelOverride: string | undefined; + // Session-scoped because the synthetic `structured_output` tool can + // be invoked from EITHER the main assistant-turn loop or from a + // drain-turn (queued notification / cron prompt); whichever fires + // first wins, and both paths need to surface the same structured + // result envelope. + let structuredSubmission: unknown = undefined; // Captures the first ~200 chars of model-emitted plain text across // turns. Used only to enrich the --json-schema "produced plain // text" error: the user/operator gets a hint of what the model // actually said instead of a static, context-free message. let plainTextPreview = ''; const PLAIN_TEXT_PREVIEW_LIMIT = 200; + + // Shared terminal block for the structured-output success + // contract. Both the main-turn loop and the drain-turn post-loop + // previously reproduced this block verbatim + // (`registry.abortAll()` → bounded holdback for in-flight + // background-task `task_notification` events → flush localQueue → + // finalize one-shot monitors → `adapter.emitResult` → return 0). + // `finalizeOneShotMonitors` is idempotent (the + // `oneShotMonitorsFinalized` guard makes the second call a + // no-op), so unconditional invocation is safe even when the drain + // path already finalized monitors before reaching here. + const emitStructuredSuccess = async (): Promise<0> => { + registry.abortAll(); + // `abortAll()` marks each task `cancelled` synchronously, but + // the matching `task_notification` is emitted later by the + // task's natural handler. Hold back briefly (capped at + // STRUCTURED_SHUTDOWN_HOLDBACK_MS) so consumers see every + // `task_started` paired with its terminal notification, without + // blocking exit on a slow agent that the user has already + // declared done. + const holdbackDeadline = Date.now() + STRUCTURED_SHUTDOWN_HOLDBACK_MS; + while ( + Date.now() < holdbackDeadline && + registry.hasUnfinalizedTasks() + ) { + await new Promise((r) => setTimeout(r, 50)); + } + flushQueuedNotificationsToSdk(localQueue); + finalizeOneShotMonitors(); + const metrics = uiTelemetryService.getMetrics(); + const usage = computeUsageFromMetrics(metrics); + const stats = + outputFormat === OutputFormat.JSON + ? uiTelemetryService.getMetrics() + : undefined; + adapter.emitResult({ + isError: false, + durationMs: Date.now() - startTime, + apiDurationMs: totalApiDurationMs, + numTurns: turnCount, + usage, + stats, + structuredResult: structuredSubmission, + }); + return 0; + }; + + /** + * Shared per-turn tool-call dispatch for the main-turn loop and + * `drainOneItem`. Both call sites used to reproduce ~120 lines of + * near-identical logic that filtered `structured_output` to its + * own pre-scan when `--json-schema` is active, executed each + * request through `executeToolCall`, captured the `structured_output` + * args into the session-scoped `structuredSubmission`, and + * synthesised `tool_result` events for every suppressed sibling + * `tool_use`. The two blocks differed only by variable name + * prefixes (`requestsToExecute` vs `itemRequestsToExecute`, etc.) + * and which scope's `modelOverride` to update — passed in as + * `setModelOverride` so the caller controls binding. + * + * The helper mutates the closure-captured `structuredSubmission` + * directly (it's session-scoped on purpose: whichever turn + * captures it terminates the run). The caller is responsible for + * acting on a non-undefined `structuredSubmission` after the + * helper returns (main-turn → emitStructuredSuccess(); drain-turn + * → return so the post-drain code emits success). + */ + const processToolCallBatch = async ( + batchRequests: ToolCallRequestInfo[], + setModelOverride: (override: string | undefined) => void, + ): Promise => { + const toolResponseParts: Part[] = []; + + // Pre-scan: when --json-schema is active and the model emitted + // a `structured_output` call alongside other tools in the same + // turn, the structured call is the terminal contract. Execute + // every structured_output in original order until one succeeds, + // suppress every non-structured sibling. See the multi-shape + // examples in the main loop's prior comment for the + // [bad/good/side-effect] permutations. + let requestsToExecute = batchRequests; + if ( + config.getJsonSchema() && + batchRequests.some((r) => r.name === ToolNames.STRUCTURED_OUTPUT) + ) { + requestsToExecute = batchRequests.filter( + (r) => r.name === ToolNames.STRUCTURED_OUTPUT, + ); + } + const executedCallIds = new Set(); + + for (const requestInfo of requestsToExecute) { + executedCallIds.add(requestInfo.callId); + + const inputFormat = + typeof config.getInputFormat === 'function' + ? config.getInputFormat() + : InputFormat.TEXT; + const toolCallUpdateCallback = + inputFormat === InputFormat.STREAM_JSON && options.controlService + ? options.controlService.permission.getToolCallUpdateCallback() + : undefined; + + // Build outputUpdateHandler for this tool call. Agent tool + // has its own complex handler (subagent messages). All other + // tools with canUpdateOutput=true (e.g., MCP tools) get a + // generic handler that emits progress via the adapter. + const isAgentTool = requestInfo.name === 'agent'; + const { handler: outputUpdateHandler } = isAgentTool + ? createAgentToolProgressHandler( + config, + requestInfo.callId, + adapter, + ) + : createToolProgressHandler(requestInfo, adapter); + + const toolResponse = await executeToolCall( + config, + requestInfo, + abortController.signal, + { + outputUpdateHandler, + ...(toolCallUpdateCallback && { + onToolCallsUpdate: toolCallUpdateCallback, + }), + }, + ); + + if (toolResponse.error) { + // In JSON/STREAM_JSON mode, tool errors are tolerated and + // formatted as tool_result blocks. handleToolError detects + // mode from config and allows the session to continue so + // the LLM can decide what to do next. In text mode, we + // still log the error. + handleToolError( + requestInfo.name, + toolResponse.error, + config, + toolResponse.errorType || 'TOOL_EXECUTION_ERROR', + typeof toolResponse.resultDisplay === 'string' + ? toolResponse.resultDisplay + : undefined, + ); + } + + adapter.emitToolResult(requestInfo, toolResponse); + config + .getGeminiClient() + .recordCompletedToolCall( + requestInfo.name, + requestInfo.args as Record, + ); + + if (toolResponse.responseParts) { + toolResponseParts.push(...toolResponse.responseParts); + } + + // Capture model override from skill tool results. + // Use `in` so that undefined (from inherit/no-model skills) + // clears a prior override, while non-skill tools (field + // absent) leave the current override intact. + if ('modelOverride' in toolResponse) { + setModelOverride(toolResponse.modelOverride); + } + + if ( + requestInfo.name === ToolNames.STRUCTURED_OUTPUT && + !toolResponse.error + ) { + // Honour the "first valid call ends the session" contract. + // The break is after the responseParts/modelOverride capture + // above so future changes to SyntheticOutputTool can't + // silently drop those signals. structuredSubmission is the + // session-scoped binding from the enclosing scope. + structuredSubmission = requestInfo.args; + break; + } + } + + // Synthesise tool_result events + retry parts for every + // tool_use block from the prior assistant message that we did + // NOT actually execute — non-structured siblings that were + // suppressed up front, plus any structured_output calls left + // unexecuted after an earlier one in the batch already + // succeeded. Runs for both the success and retry paths so the + // emitted event log pairs every tool_use with a tool_result + // AND the retry-turn payload (when reached) doesn't leave + // Anthropic / OpenAI staring at unpaired tool_use blocks. + const unexecutedCalls = batchRequests.filter( + (r) => !executedCallIds.has(r.callId), + ); + if (unexecutedCalls.length > 0) { + const skippedOutput = suppressedOutputBody( + structuredSubmission !== undefined, + ); + for (const call of unexecutedCalls) { + const responseParts: Part[] = [ + { + functionResponse: { + id: call.callId, + name: call.name, + response: { output: skippedOutput }, + }, + }, + ]; + adapter.emitToolResult(call, { + callId: call.callId, + responseParts, + resultDisplay: skippedOutput, + error: undefined, + errorType: undefined, + }); + toolResponseParts.push(...responseParts); + } + } + + return toolResponseParts; + }; + while (true) { turnCount++; if ( @@ -496,162 +756,31 @@ export async function runNonInteractive( totalApiDurationMs += Date.now() - apiStartTime; if (toolCallRequests.length > 0) { - const toolResponseParts: Part[] = []; - // When --json-schema is active, the first successful call to the - // synthetic structured_output tool terminates the session with the - // submitted args as the structured result. A separate boolean - // tracks whether a submission happened, since `args` itself may - // legitimately be undefined or any falsy value (an empty schema - // `{}` accepts any payload, including no fields at all). - let structuredSubmission: unknown = undefined; - let hasStructuredSubmission = false; + // Dispatch the per-turn tool-call batch through the shared + // helper (see processToolCallBatch above). The helper handles + // the `--json-schema` pre-scan, executes each request, writes + // the first valid `structured_output` call's args into the + // session-scoped `structuredSubmission`, and synthesises + // tool_result events for every suppressed sibling. The + // `modelOverride` setter is the only call-site-specific + // binding — the main turn updates the session-scoped + // `modelOverride` so the next turn's sendMessageStream sees + // it; the drain turn updates a per-item `itemModelOverride` + // scoped to that drain item. + const toolResponseParts = await processToolCallBatch( + toolCallRequests, + (override) => { + modelOverride = override; + }, + ); - // Pre-scan: when --json-schema is active and the model emitted - // structured_output alongside other tools in the same turn, - // execute structured_output FIRST so its terminal-flag wins - // before sibling tools' side effects (write_file, shell, …) - // get a chance to persist. If structured_output succeeds the - // loop breaks immediately and siblings are skipped — no - // tool_result is emitted for them; the session terminates via - // the emitResult call below so the missing function_response - // entries cause no API protocol issue (there is no next turn). - // If structured_output fails (validation), `hasStructuredSubmission` - // stays false and the siblings still run via the normal loop - // body — same behavior as a turn that didn't issue - // structured_output at all. - // - // Without this, [write_file, structured_output] runs write_file - // first (irreversible), THEN structured_output sets the flag, - // and the user gets back a "successful" structured result with - // unrelated side-effects already on disk. - let orderedToolCallRequests = toolCallRequests; - if (config.getJsonSchema()) { - const structIdx = orderedToolCallRequests.findIndex( - (r) => r.name === ToolNames.STRUCTURED_OUTPUT, - ); - if (structIdx > 0) { - orderedToolCallRequests = [ - orderedToolCallRequests[structIdx], - ...orderedToolCallRequests.slice(0, structIdx), - ...orderedToolCallRequests.slice(structIdx + 1), - ]; - } - } - - for (const requestInfo of orderedToolCallRequests) { - const finalRequestInfo = requestInfo; - - const inputFormat = - typeof config.getInputFormat === 'function' - ? config.getInputFormat() - : InputFormat.TEXT; - const toolCallUpdateCallback = - inputFormat === InputFormat.STREAM_JSON && options.controlService - ? options.controlService.permission.getToolCallUpdateCallback() - : undefined; - - // Build outputUpdateHandler for this tool call. - // Agent tool has its own complex handler (subagent messages). - // All other tools with canUpdateOutput=true (e.g., MCP tools) - // get a generic handler that emits progress via the adapter. - const isAgentTool = finalRequestInfo.name === 'agent'; - const { handler: outputUpdateHandler } = isAgentTool - ? createAgentToolProgressHandler( - config, - finalRequestInfo.callId, - adapter, - ) - : createToolProgressHandler(finalRequestInfo, adapter); - - const toolResponse = await executeToolCall( - config, - finalRequestInfo, - abortController.signal, - { - outputUpdateHandler, - ...(toolCallUpdateCallback && { - onToolCallsUpdate: toolCallUpdateCallback, - }), - }, - ); - - if (toolResponse.error) { - // In JSON/STREAM_JSON mode, tool errors are tolerated and formatted - // as tool_result blocks. handleToolError will detect JSON/STREAM_JSON mode - // from config and allow the session to continue so the LLM can decide what to do next. - // In text mode, we still log the error. - handleToolError( - finalRequestInfo.name, - toolResponse.error, - config, - toolResponse.errorType || 'TOOL_EXECUTION_ERROR', - typeof toolResponse.resultDisplay === 'string' - ? toolResponse.resultDisplay - : undefined, - ); - } - - adapter.emitToolResult(finalRequestInfo, toolResponse); - config - .getGeminiClient() - .recordCompletedToolCall( - finalRequestInfo.name, - finalRequestInfo.args as Record, - ); - - if ( - finalRequestInfo.name === ToolNames.STRUCTURED_OUTPUT && - !toolResponse.error && - !hasStructuredSubmission - ) { - structuredSubmission = finalRequestInfo.args; - hasStructuredSubmission = true; - } - - if (toolResponse.responseParts) { - toolResponseParts.push(...toolResponse.responseParts); - } - - // Capture model override from skill tool results. - // Use `in` so that undefined (from inherit/no-model skills) clears a prior override, - // while non-skill tools (field absent) leave the current override intact. - if ('modelOverride' in toolResponse) { - modelOverride = toolResponse.modelOverride; - } - - // Single-shot contract: structured_output is terminal. - // The pre-scan above hoists it to the front of the batch, - // so once it succeeds the remaining (now reordered) - // entries are guaranteed to be siblings the model - // intended for THIS turn — break and let the terminal - // emitResult fire below. Unpaired tool_use entries in - // the model's record are harmless because no next API - // call happens (the session is over). - if (hasStructuredSubmission) { - break; - } - } - if (hasStructuredSubmission) { - // Abort any in-flight background agents so they don't race the - // terminal emitResult; structured-output mode is a single-shot - // contract and the caller expects a deterministic shutdown. - registry.abortAll(); - const metrics = uiTelemetryService.getMetrics(); - const usage = computeUsageFromMetrics(metrics); - const stats = - outputFormat === OutputFormat.JSON - ? uiTelemetryService.getMetrics() - : undefined; - adapter.emitResult({ - isError: false, - durationMs: Date.now() - startTime, - apiDurationMs: totalApiDurationMs, - numTurns: turnCount, - usage, - stats, - structuredResult: structuredSubmission, - }); - return; + if (structuredSubmission !== undefined) { + // Single-shot terminal contract; aborts in-flight background + // agents, holds back briefly for their terminal + // task_notification events to land, then emits the + // structured success envelope. Same helper as the drain-turn + // post-loop branch — see emitStructuredSuccess above. + return emitStructuredSuccess(); } currentMessages = [{ role: 'user', parts: toolResponseParts }]; } else { @@ -672,15 +801,6 @@ export async function runNonInteractive( await handleMaxTurnsExceededError(config); } - const inputFormat = - typeof config.getInputFormat === 'function' - ? config.getInputFormat() - : InputFormat.TEXT; - const toolCallUpdateCallback = - inputFormat === InputFormat.STREAM_JSON && options.controlService - ? options.controlService.permission.getToolCallUpdateCallback() - : undefined; - let itemMessages: Content[] = [ { role: 'user', parts: [{ text: item.modelText }] }, ]; @@ -742,57 +862,23 @@ export async function runNonInteractive( totalApiDurationMs += Date.now() - itemApiStartTime; if (itemToolCallRequests.length > 0) { - const itemToolResponseParts: Part[] = []; + // Same shared dispatch as the main-turn loop. The only + // call-site difference is `itemModelOverride` is local to + // the drain item (so the next iteration's + // sendMessageStream picks up the per-item override), + // while the main loop binds to the session-scoped + // `modelOverride`. + const itemToolResponseParts = await processToolCallBatch( + itemToolCallRequests, + (override) => { + itemModelOverride = override; + }, + ); - for (const requestInfo of itemToolCallRequests) { - const isAgentTool = requestInfo.name === 'agent'; - const { handler: outputUpdateHandler } = isAgentTool - ? createAgentToolProgressHandler( - config, - requestInfo.callId, - adapter, - ) - : createToolProgressHandler(requestInfo, adapter); - - const toolResponse = await executeToolCall( - config, - requestInfo, - abortController.signal, - { - outputUpdateHandler, - ...(toolCallUpdateCallback && { - onToolCallsUpdate: toolCallUpdateCallback, - }), - }, - ); - - if (toolResponse.error) { - handleToolError( - requestInfo.name, - toolResponse.error, - config, - toolResponse.errorType || 'TOOL_EXECUTION_ERROR', - typeof toolResponse.resultDisplay === 'string' - ? toolResponse.resultDisplay - : undefined, - ); - } - - adapter.emitToolResult(requestInfo, toolResponse); - config - .getGeminiClient() - .recordCompletedToolCall( - requestInfo.name, - requestInfo.args as Record, - ); - - if (toolResponse.responseParts) { - itemToolResponseParts.push(...toolResponse.responseParts); - } - - if ('modelOverride' in toolResponse) { - itemModelOverride = toolResponse.modelOverride; - } + if (structuredSubmission !== undefined) { + // Stop processing further turns for this drain item; + // the post-drain code will emit the terminal result. + return; } itemMessages = [{ role: 'user', parts: itemToolResponseParts }]; } else { @@ -813,6 +899,10 @@ export async function runNonInteractive( if (drainPromise) return drainPromise; const p = (async () => { while (localQueue.length > 0) { + // Stop draining once a queued item's structured_output + // call captured the terminal contract — no point running + // more queued prompts that can't influence the result. + if (structuredSubmission !== undefined) return; await drainOneItem(); } })(); @@ -846,6 +936,16 @@ export async function runNonInteractive( }); const checkCronDone = () => { + // A drain-turn structured_output makes the rest of the + // cron schedule moot: we already have a terminal result + // and the post-drain emit is about to fire. Stop the + // scheduler so no further jobs enqueue. + if (structuredSubmission !== undefined) { + abortController.signal.removeEventListener('abort', onAbort); + scheduler.stop(); + resolve(); + return; + } if (scheduler.size === 0 && !drainPromise) { abortController.signal.removeEventListener('abort', onAbort); scheduler.stop(); @@ -897,6 +997,10 @@ export async function runNonInteractive( // through the model, but later monitor output is SDK-only. captureMonitorTurnsInLocalQueue = false; await drainLocalQueue(); + // A drain-turn structured_output captured the terminal + // contract — bail out of the holdback loop early and let the + // post-loop code emit the success result. + if (structuredSubmission !== undefined) break; // Wait for every background task's terminal notification, not // just the running ones: cancel() marks status 'cancelled' // synchronously but the notification is emitted later by the @@ -925,12 +1029,22 @@ export async function runNonInteractive( ? uiTelemetryService.getMetrics() : undefined; + // A drain-turn structured_output captured the terminal contract + // — emit the structured success envelope rather than falling + // through to the "Model produced plain text..." failure path. + // Same helper as the main-turn path; recomputes its own + // metrics snapshot after the holdback so any task notifications + // that landed during shutdown contribute to the totals. + if (structuredSubmission !== undefined) { + return emitStructuredSuccess(); + } + // --json-schema contract: the model MUST terminate via the // structured_output tool. Reaching this branch means it emitted // plain text instead — surface as an error rather than silently // returning whatever free-form summary the adapter collected. - // Setting exitCode + returning (rather than throwing) avoids the - // outer catch re-emitting the result a second time. + // Returning a non-zero exit code (rather than throwing) avoids + // the outer catch re-emitting the result a second time. if (config.getJsonSchema()) { // Enrich the static contract message with diagnostic context: // turn count (how many tries the model got) + a preview of @@ -955,15 +1069,7 @@ export async function runNonInteractive( usage, stats, }); - // Adapter handles user-visible feedback per output format: - // - TEXT: writes errorMessage to stderr (JsonOutputAdapter - // emitResult, line ~70). - // - JSON / STREAM_JSON: emits the structured result with - // is_error=true. - // No extra stderr write here — duplicating in TEXT mode - // produced two copies of the same line in headless runs. - process.exitCode = 1; - return; + return 1; } adapter.emitResult({ @@ -974,7 +1080,7 @@ export async function runNonInteractive( usage, stats, }); - return; + return 0; } } } catch (error) { @@ -1050,5 +1156,9 @@ export async function runNonInteractive( await shutdownTelemetry(); } } + // Unreachable in practice: the catch block awaits handleError() which + // returns Promise (it always exits the process or rethrows). + // This return exists only so TS sees the function as total. + return 1; }); } diff --git a/packages/cli/src/utils/errors.test.ts b/packages/cli/src/utils/errors.test.ts index 3f91fd4b642..7dec71fd03f 100644 --- a/packages/cli/src/utils/errors.test.ts +++ b/packages/cli/src/utils/errors.test.ts @@ -725,6 +725,53 @@ describe('errors', () => { ); }); }); + + describe('with --json-schema active', () => { + // When the structured-output run hits maxSessionTurns the generic + // "increase maxSessionTurns" message can be misleading: the real + // cause is usually that structured_output never got called (denied + // by permissions, unsatisfiable schema, prompt didn't instruct the + // model). Append a contextual hint so users debugging a stuck + // --json-schema run know where to look. + beforeEach(() => { + (mockConfig as unknown as { getJsonSchema: Mock }).getJsonSchema = vi + .fn() + .mockReturnValue({ type: 'object' }); + }); + + it('appends a json-schema-specific hint in text mode', async () => { + ( + mockConfig.getOutputFormat as ReturnType + ).mockReturnValue(OutputFormat.TEXT); + + await expect(handleMaxTurnsExceededError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 53', + ); + + const written = mockWriteStderrLine.mock.calls[0]?.[0] as string; + expect(written).toMatch(/Reached max session turns for this session\./); + expect(written).toMatch(/--json-schema is active/); + expect(written).toMatch(/permissions\.deny.*--exclude-tools/); + }); + + it('appends a json-schema-specific hint inside the JSON error message', async () => { + ( + mockConfig.getOutputFormat as ReturnType + ).mockReturnValue(OutputFormat.JSON); + + await expect(handleMaxTurnsExceededError(mockConfig)).rejects.toThrow( + 'process.exit called with code: 53', + ); + + const written = mockWriteStderrLine.mock.calls[0]?.[0] as string; + const parsed = JSON.parse(written) as { + error: { type: string; message: string; code: number }; + }; + expect(parsed.error.type).toBe('FatalTurnLimitedError'); + expect(parsed.error.code).toBe(53); + expect(parsed.error.message).toMatch(/--json-schema is active/); + }); + }); }); describe('cleanup-before-exit invariant', () => { diff --git a/packages/cli/src/utils/errors.ts b/packages/cli/src/utils/errors.ts index 62c8ae1678f..6e4e4a5ebad 100644 --- a/packages/cli/src/utils/errors.ts +++ b/packages/cli/src/utils/errors.ts @@ -246,13 +246,24 @@ export async function handleCancellationError(config: Config): Promise { /** * Handles max session turns exceeded consistently. + * + * When `--json-schema` is active the error gets an extra hint pointing at the + * common reasons a structured-output run never terminated: the model never + * called `structured_output`, the tool was denied by `permissions.deny` / + * `--exclude-tools`, or the schema is unsatisfiable. Without this, all three + * failure modes surface as the same generic "increase maxSessionTurns" line + * even though the fix is a permissions / schema change, not a turns bump. */ export async function handleMaxTurnsExceededError( config: Config, ): Promise { - const maxTurnsError = new FatalTurnLimitedError( - 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.', - ); + const baseMessage = + 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.'; + const jsonSchemaActive = config.getJsonSchema?.() !== undefined; + const message = jsonSchemaActive + ? `${baseMessage}\nNote: --json-schema is active. If the model never called structured_output, verify it isn't denied by permissions.deny / --exclude-tools and that the schema is satisfiable.` + : baseMessage; + const maxTurnsError = new FatalTurnLimitedError(message); if (config.getOutputFormat() === OutputFormat.JSON) { const formatter = new JsonFormatter(); diff --git a/packages/core/src/agents/backends/InProcessBackend.ts b/packages/core/src/agents/backends/InProcessBackend.ts index d5912400daa..87a8a4e1056 100644 --- a/packages/core/src/agents/backends/InProcessBackend.ts +++ b/packages/core/src/agents/backends/InProcessBackend.ts @@ -413,7 +413,7 @@ async function createPerAgentConfig( const agentRegistry: ToolRegistry = await override.createToolRegistry( undefined, - { skipDiscovery: true }, + { skipDiscovery: true, forSubAgent: true }, ); agentRegistry.copyDiscoveredToolsFrom(base.getToolRegistry()); override.getToolRegistry = () => agentRegistry; diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 321fba2db5a..14724f7693f 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1376,6 +1376,82 @@ describe('Server Config (config.ts)', () => { ).toEqual([ToolNames.READ_FILE, ToolNames.EDIT, ToolNames.SHELL]); }); + it('registers structured_output in bare mode when jsonSchema is set', async () => { + // Bare mode strips the toolset to READ_FILE/EDIT/SHELL, but the + // synthetic structured_output tool is the terminal contract for + // --json-schema runs. Without it the model loops until + // maxSessionTurns and exits via the "plain text" failure path — + // expensive in tokens for what's almost always a CI use case. The + // synthetic tool must be registered alongside the bare three. + const config = new Config({ + ...baseParams, + bareMode: true, + jsonSchema: { type: 'object', properties: { ok: { type: 'boolean' } } }, + }); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerFactory: Mock } }; + } + ).ToolRegistry.prototype.registerFactory; + + expect( + (registerToolMock as Mock).mock.calls.map((call) => call[0]), + ).toEqual([ + ToolNames.READ_FILE, + ToolNames.EDIT, + ToolNames.SHELL, + ToolNames.STRUCTURED_OUTPUT, + ]); + }); + + it('does NOT register structured_output when createToolRegistry is called with forSubAgent=true', async () => { + // Subagent overrides reuse the parent Config via prototype + // delegation (createApprovalModeOverride / buildSubagentContextOverride + // → Object.create(base)) and rebuild the tool registry with + // `forSubAgent: true`. Even though `this.jsonSchema` propagates + // through the prototype chain, the synthetic tool MUST NOT register + // in the subagent registry: only runNonInteractive's main / drain + // loops detect a successful structured_output call as terminal, so + // a subagent calling the tool would receive "Session will end now" + // and then keep running because its own loop has no terminator — + // wasted tokens and no structured payload on stdout. + const config = new Config({ + ...baseParams, + bareMode: true, + jsonSchema: { type: 'object', properties: { ok: { type: 'boolean' } } }, + }); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerFactory: Mock } }; + } + ).ToolRegistry.prototype.registerFactory; + // Initial bare init registers READ_FILE / EDIT / SHELL / + // STRUCTURED_OUTPUT (asserted by the test above). Reset so we can + // observe ONLY the forSubAgent rebuild's calls. + (registerToolMock as Mock).mockClear(); + + // Rebuild registry as if for a subagent override. + await config.createToolRegistry(undefined, { + skipDiscovery: true, + forSubAgent: true, + }); + + const registeredNames = (registerToolMock as Mock).mock.calls.map( + (call) => call[0], + ); + expect(registeredNames).not.toContain(ToolNames.STRUCTURED_OUTPUT); + // The bare three still register so the subagent has its toolset. + expect(registeredNames).toEqual([ + ToolNames.READ_FILE, + ToolNames.EDIT, + ToolNames.SHELL, + ]); + }); + it('should register a tool if coreTools contains an argument-specific pattern', async () => { const params: ConfigParameters = { ...baseParams, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 79c9c4c0a73..d83117778f7 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2899,7 +2899,7 @@ export class Config { async createToolRegistry( sendSdkMcpMessage?: SendSdkMcpMessage, - options?: { skipDiscovery?: boolean }, + options?: { skipDiscovery?: boolean; forSubAgent?: boolean }, ): Promise { const registry = new ToolRegistry( this, @@ -2933,6 +2933,37 @@ export class Config { } }; + // The synthetic structured_output tool is the terminal contract for + // --json-schema runs. It must be registered in BOTH the bare-mode + // branch and the regular branch — without it the model can't finish + // a structured run, so omitting either branch causes + // `qwen [--bare] --json-schema X -p "..."` to loop until + // maxSessionTurns and exit via the "plain text" failure path. Hoisted + // out of the two branches so the dynamic-import factory shape stays + // in sync between them. + // + // Skipped when building a subagent-context registry. `this.jsonSchema` + // propagates to subagent overrides via prototype delegation + // (`Object.create(base)` in `createApprovalModeOverride` / + // `buildSubagentContextOverride`), but only `runNonInteractive`'s main + // and drain loops detect a successful structured_output call as + // terminal. A subagent that called the tool would receive the + // "Session will end now" llmContent, then keep running because its + // own loop has no termination handler — wasted tokens with no + // structured payload surfacing on stdout. Strip the registration in + // those contexts. + const registerStructuredOutputIfRequested = async (): Promise => { + if (!this.jsonSchema) return; + if (options?.forSubAgent) return; + const schema = this.jsonSchema; + await registerLazy(ToolNames.STRUCTURED_OUTPUT, async () => { + const { SyntheticOutputTool } = await import( + '../tools/syntheticOutput.js' + ); + return new SyntheticOutputTool(schema); + }); + }; + if (this.getBareMode()) { await registerLazy(ToolNames.READ_FILE, async () => { const { ReadFileTool } = await import('../tools/read-file.js'); @@ -2946,6 +2977,7 @@ export class Config { const { ShellTool } = await import('../tools/shell.js'); return new ShellTool(this); }); + await registerStructuredOutputIfRequested(); this.debugLogger.debug( `ToolRegistry created: ${JSON.stringify(registry.getAllToolNames())} (${registry.getAllToolNames().length} tools)`, ); @@ -3063,15 +3095,9 @@ export class Config { // Register synthetic structured-output tool when --json-schema is set. // The tool's parameter schema IS the user-supplied JSON Schema, so the // model's arguments must match it (Ajv-validated in BaseDeclarativeTool). - if (this.jsonSchema) { - const schema = this.jsonSchema; - await registerLazy(ToolNames.STRUCTURED_OUTPUT, async () => { - const { SyntheticOutputTool } = await import( - '../tools/syntheticOutput.js' - ); - return new SyntheticOutputTool(this, schema); - }); - } + // Same helper as the bare-mode branch above to keep the registration + // shape and permission gating in sync between the two paths. + await registerStructuredOutputIfRequested(); // Register cron tools unless disabled if (this.isCronEnabled()) { diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index dc7802498b3..d9bb4eb6370 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -15,6 +15,7 @@ import { AuthType, type ContentGenerator } from '../core/contentGenerator.js'; import { GeminiChat, InvalidStreamError, + redactStructuredOutputArgsForRecording, StreamEventType, type StreamEvent, } from './geminiChat.js'; @@ -3322,6 +3323,75 @@ describe('GeminiChat', async () => { }); }); + describe('redactStructuredOutputArgsForRecording', () => { + // The chat-recording JSONL persists assistant turns to disk and re-feeds + // them on `--continue` / `--resume`. For `--json-schema` runs the + // structured_output args ARE the user's structured payload, already + // emitted on stdout; recording them verbatim here would silently + // contradict the redaction the ToolCallEvent telemetry path applies. + // These tests pin the helper that scrubs them. + + it('replaces args on a structured_output functionCall with the placeholder', () => { + const result = redactStructuredOutputArgsForRecording({ + functionCall: { + id: 'call-1', + name: 'structured_output', + args: { + extracted: 'sensitive answer', + score: 0.9, + details: { token: 'shhhh' }, + }, + }, + }); + expect(result).not.toBeNull(); + expect(result!.functionCall.name).toBe('structured_output'); + expect(result!.functionCall.id).toBe('call-1'); + expect(result!.functionCall.args).toEqual({ + __redacted: 'structured_output payload (see stdout result)', + }); + // The original payload must NOT survive in any field of the output. + expect(JSON.stringify(result)).not.toContain('sensitive answer'); + expect(JSON.stringify(result)).not.toContain('shhhh'); + }); + + it('passes non-structured_output functionCalls through untouched', () => { + const original = { + id: 'call-2', + name: 'write_file', + args: { path: '/tmp/x', content: 'hello' }, + }; + const result = redactStructuredOutputArgsForRecording({ + functionCall: original, + }); + expect(result).not.toBeNull(); + expect(result!.functionCall).toEqual(original); + // Reference identity not required, but the args object must equal + // the input (no redaction applied). + expect(result!.functionCall.args).toEqual({ + path: '/tmp/x', + content: 'hello', + }); + }); + + it('returns null for parts with no functionCall', () => { + expect(redactStructuredOutputArgsForRecording({ text: 'hi' })).toBeNull(); + expect(redactStructuredOutputArgsForRecording({})).toBeNull(); + }); + + it('does not mutate the input part', () => { + const original = { + functionCall: { + id: 'call-3', + name: 'structured_output', + args: { ok: true, data: [1, 2, 3] }, + }, + }; + const snapshot = JSON.parse(JSON.stringify(original)); + redactStructuredOutputArgsForRecording(original); + expect(original).toEqual(snapshot); + }); + }); + // Compression logic is tested in chatCompressionService.test.ts; this // suite covers per-chat state on GeminiChat: hasFailedCompressionAttempt // stickiness, token-count mutation, history replacement, and conditional diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 4e61579af6b..4f06666c405 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -34,6 +34,8 @@ import { tokenLimit, } from './tokenLimits.js'; import { hasCycleInSchema } from '../tools/tools.js'; +import { ToolNames } from '../tools/tool-names.js'; +import { STRUCTURED_OUTPUT_REDACTED_ARGS } from '../tools/syntheticOutput.js'; import type { StructuredError } from './turn.js'; import { logContentRetry, @@ -54,6 +56,41 @@ import { getContextLengthExceededInfo } from '../utils/contextLengthError.js'; const debugLogger = createDebugLogger('QWEN_CODE_CHAT'); +/** + * Replaces the args on a `structured_output` `functionCall` with the + * same `__redacted` placeholder used by `ToolCallEvent` telemetry + * (`packages/core/src/telemetry/types.ts`). + * + * The chat-recording JSONL (`/chats/.jsonl`) + * persists assistant turns to disk and re-feeds them on + * `--continue` / `--resume`. For `--json-schema` runs the tool args + * ARE the user's structured payload — already emitted on stdout via + * `result` / `structured_result`. Recording them verbatim here would + * mean the same payload (and every validation-failure retry along the + * way) sits on disk indefinitely, contradicting the privacy contract + * documented next to the telemetry redaction. Mirror the placeholder + * here so the chat-recording surface matches. + * + * Non-`structured_output` `functionCall`s pass through untouched. + * + * Exported for tests; callers should prefer the inline use inside + * `recordAssistantTurn` invocation below. + */ +export function redactStructuredOutputArgsForRecording( + part: Part, +): { functionCall: NonNullable } | null { + if (!part.functionCall) return null; + if (part.functionCall.name !== ToolNames.STRUCTURED_OUTPUT) { + return { functionCall: part.functionCall }; + } + return { + functionCall: { + ...part.functionCall, + args: { ...STRUCTURED_OUTPUT_REDACTED_ARGS }, + }, + }; +} + function isCompressionFailureStatus(status: CompressionStatus): boolean { return ( status === CompressionStatus.COMPRESSION_FAILED_INFLATED_TOKEN_COUNT || @@ -1257,8 +1294,13 @@ export class GeminiChat { ...(contentText ? [{ text: contentText }] : []), ...(hasToolCall ? contentParts - .filter((part) => part.functionCall) - .map((part) => ({ functionCall: part.functionCall })) + .map(redactStructuredOutputArgsForRecording) + .filter( + ( + p, + ): p is { functionCall: NonNullable } => + p !== null, + ) : []), ], tokens: usageMetadata, diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 9e5a031b2c1..766422cc7dc 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -1592,6 +1592,20 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('ask_user_question')).toBe(true); }); + it('structured_output tool bypasses coreTools allowlist check', async () => { + // structured_output is a synthetic tool that only exists when the + // user opts into --json-schema. Treating it like agent/skill/ + // exit_plan_mode (bypass the coreTools allowlist) is the right + // default: a run that combines `--json-schema X --core-tools read_file` + // intends "restrict the model's pluggable toolbelt to read_file" + // while still receiving the structured payload — silently dropping + // structured_output here would leave --json-schema with no terminal + // contract, so the run would loop until maxTurns. + pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); + pm.initialize(); + expect(await pm.isToolEnabled('structured_output')).toBe(true); + }); + it('Non-core tools still respect deny rules', async () => { pm = new PermissionManager( makeConfig({ diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 68a6ce1e727..414cf0c9fc2 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -427,7 +427,17 @@ export class PermissionManager { /** * Core tools that are subject to the coreTools allowlist check. - * Tools not in this set (MCP, Skill, Agent, etc.) bypass the check. + * + * Tools NOT in this set bypass the check. Two categories live outside: + * - Dynamically discovered tools (MCP, Skill). + * - Synthetic system tools that the framework injects when a feature is + * opted into and that have no meaning when missing — `agent`, + * `exit_plan_mode`, `ask_user_question`, `task_stop`, `send_message`, + * `structured_output` (registered only when `--json-schema` is set). + * Excluding `structured_output` from `--core-tools` would leave a + * `--json-schema` run with no terminal contract, so the synthetic + * tool stays available regardless of the allowlist (deny rules still + * apply). */ private static readonly CORE_TOOLS = new Set([ 'read_file', diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index fb604d49a0b..fdd1bbc7ed3 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -19,6 +19,7 @@ import type { FileOperation } from './metrics.js'; export { ToolCallDecision }; import type { OutputFormat } from '../output/types.js'; import { ToolNames } from '../tools/tool-names.js'; +import { STRUCTURED_OUTPUT_REDACTED_ARGS } from '../tools/syntheticOutput.js'; import type { SkillTool } from '../tools/skill.js'; import type { AgentTool } from '../tools/agent/agent.js'; @@ -189,7 +190,21 @@ export class ToolCallEvent implements BaseTelemetryEvent { this['event.name'] = 'tool_call'; this['event.timestamp'] = new Date().toISOString(); this.function_name = call.request.name; - this.function_args = call.request.args; + // structured_output args ARE the user's final structured payload (the + // command's actual answer, already emitted in stdout `result` / + // `structured_result`). Recording them again as ordinary tool-call + // function_args duplicates that data into telemetry surfaces (OTLP + // exports, QwenLogger, ui-telemetry stream, the chat-recording UI + // event mirror) where it can leak off-device. Replace with a shared + // placeholder constant so consumers still see the call happened — + // duration, success, decision metrics are preserved — but the + // payload itself doesn't ride along. The same constant is used by + // `redactStructuredOutputArgsForRecording` in `core/geminiChat.ts` + // for the on-disk JSONL surface so neither side can silently drift. + this.function_args = + call.request.name === ToolNames.STRUCTURED_OUTPUT + ? { ...STRUCTURED_OUTPUT_REDACTED_ARGS } + : call.request.args; this.duration_ms = call.durationMs ?? 0; this.status = call.status; this.success = call.status === 'success'; // Keep for backward compatibility diff --git a/packages/core/src/telemetry/uiTelemetry.test.ts b/packages/core/src/telemetry/uiTelemetry.test.ts index aa7ee6d565c..664fa8f7605 100644 --- a/packages/core/src/telemetry/uiTelemetry.test.ts +++ b/packages/core/src/telemetry/uiTelemetry.test.ts @@ -737,6 +737,65 @@ describe('UiTelemetryService', () => { expect(tools.byName['tool_A'].count).toBe(1); expect(tools.byName['tool_B'].count).toBe(1); }); + + it('redacts function_args for structured_output calls while preserving metrics', () => { + const toolCall = createFakeCompletedToolCall( + 'structured_output', + true, + 250, + ToolConfirmationOutcome.ProceedOnce, + ); + // The fake helper hardcodes args to { foo: 'bar' }; in the real + // structured-output flow this would be the user's extracted payload. + // ToolCallEvent must not pass that through to telemetry. + (toolCall.request as { args: Record }).args = { + secret: 'extracted private value', + }; + + const event = new ToolCallEvent(toolCall); + + expect(event.function_name).toBe('structured_output'); + expect(event.function_args).not.toHaveProperty('secret'); + expect(event.function_args).toEqual({ + __redacted: 'structured_output payload (see stdout result)', + }); + + // Metrics still flow through normally — duration, success, decision. + service.addEvent({ + ...structuredClone(event), + 'event.name': EVENT_TOOL_CALL, + } as ToolCallEvent & { 'event.name': typeof EVENT_TOOL_CALL }); + + const { tools } = service.getMetrics(); + expect(tools.totalCalls).toBe(1); + expect(tools.totalSuccess).toBe(1); + expect(tools.totalDurationMs).toBe(250); + expect(tools.byName['structured_output']).toMatchObject({ + count: 1, + success: 1, + durationMs: 250, + }); + }); + + it('does not redact function_args for non-structured_output tools', () => { + const toolCall = createFakeCompletedToolCall( + 'write_file', + true, + 100, + ToolConfirmationOutcome.ProceedOnce, + ); + (toolCall.request as { args: Record }).args = { + path: '/tmp/x', + content: 'hello', + }; + + const event = new ToolCallEvent(toolCall); + + expect(event.function_args).toEqual({ + path: '/tmp/x', + content: 'hello', + }); + }); }); describe('resetLastPromptTokenCount', () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index af223a74740..c511cc276d3 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -281,6 +281,7 @@ export async function rebuildToolRegistryOnOverride( const ov = override as any; const agentRegistry = await ov.createToolRegistry(undefined, { skipDiscovery: true, + forSubAgent: true, }); agentRegistry.copyDiscoveredToolsFrom(base.getToolRegistry()); ov.getToolRegistry = () => agentRegistry; diff --git a/packages/core/src/tools/syntheticOutput.test.ts b/packages/core/src/tools/syntheticOutput.test.ts index a6c2894000d..7cee900a554 100644 --- a/packages/core/src/tools/syntheticOutput.test.ts +++ b/packages/core/src/tools/syntheticOutput.test.ts @@ -4,14 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect } from 'vitest'; import { SyntheticOutputTool } from './syntheticOutput.js'; import { ToolNames } from './tool-names.js'; -import type { Config } from '../config/config.js'; function makeTool(schema: Record): SyntheticOutputTool { - const mockConfig = { isInteractive: vi.fn().mockReturnValue(false) } as unknown as Config; - return new SyntheticOutputTool(mockConfig, schema); + return new SyntheticOutputTool(schema); } describe('SyntheticOutputTool', () => { @@ -37,9 +35,7 @@ describe('SyntheticOutputTool', () => { it('accepts args that match the user schema', () => { const tool = makeTool(objectSchema); - expect( - tool.validateToolParams({ summary: 'ok', score: 1 }), - ).toBeNull(); + expect(tool.validateToolParams({ summary: 'ok', score: 1 })).toBeNull(); }); it('rejects args missing required fields', () => { @@ -68,6 +64,11 @@ describe('SyntheticOutputTool', () => { }); it('is always loaded (never hidden behind ToolSearch)', () => { + // The synthetic terminal tool MUST be visible to the model from the + // very first turn. If ToolSearch's deferred-load logic ever hid it, + // the structured-output contract would silently break (the model + // wouldn't know the tool exists, would emit plain text, and the run + // would exit via the "Model produced plain text..." failure path). const tool = makeTool(objectSchema); expect(tool.alwaysLoad).toBe(true); expect(tool.shouldDefer).toBe(false); diff --git a/packages/core/src/tools/syntheticOutput.ts b/packages/core/src/tools/syntheticOutput.ts index 7fdab7e98fa..2763098bc5a 100644 --- a/packages/core/src/tools/syntheticOutput.ts +++ b/packages/core/src/tools/syntheticOutput.ts @@ -6,17 +6,39 @@ import type { ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import type { Config } from '../config/config.js'; import { ToolDisplayNames, ToolNames } from './tool-names.js'; const structuredOutputDescription = `Submit your final answer as structured JSON that conforms to the provided schema. -CRITICAL: In structured-output mode, this is the ONLY way to deliver the final result. Call this tool exactly once when you are ready to finish. Do not emit the final answer as plain text — it will be discarded. Use other tools (Read, Grep, etc.) to gather the information you need before calling this tool. +CRITICAL: In structured-output mode, this is the ONLY way to deliver the final result. Call this tool to deliver the final result; the first call with valid arguments ends the session. Do not emit the final answer as plain text — it will be discarded. Use other tools (Read, Grep, etc.) to gather the information you need before calling this tool. The arguments you pass MUST validate against the tool's parameter schema. If validation fails you will receive the error and may retry with corrected fields.`; export type StructuredOutputParams = Record; +/** + * Placeholder that replaces a `structured_output` tool call's `args` on + * every surface that would otherwise persist or re-broadcast the user's + * structured payload. + * + * Two on-device surfaces redact via this constant: + * 1. `ToolCallEvent` in `telemetry/types.ts` — keeps the payload out + * of OTLP exports / QwenLogger / ui-telemetry stream / chat-recording + * UI event mirror. + * 2. `redactStructuredOutputArgsForRecording` in `core/geminiChat.ts` + * — keeps the payload out of the on-disk chat-recording JSONL + * (which gets re-fed into model context on `--continue` / + * `--resume`). + * + * Shared so both sites can't drift. The args ARE the user's final + * structured payload; they're already on stdout via `result` / + * `structured_result`, so persisting them again is duplication that + * leaks payload data into long-lived storage. + */ +export const STRUCTURED_OUTPUT_REDACTED_ARGS = { + __redacted: 'structured_output payload (see stdout result)', +} as const; + /** * Synthetic tool that is registered only when the user passes --json-schema. * The parameter schema of the tool IS the user-provided JSON Schema, so the @@ -25,6 +47,11 @@ export type StructuredOutputParams = Record; * * The caller (nonInteractiveCli) recognizes a successful invocation of this * tool and ends the session, using request.args as the structured result. + * + * Wired into the ToolSearch infrastructure with `alwaysLoad: true` so the + * tool is never hidden behind on-demand schema loading — the model has to + * see this tool in its function-declaration list from the very first turn, + * otherwise the structured-output contract can't be honored at all. */ export class SyntheticOutputTool extends BaseDeclarativeTool< StructuredOutputParams, @@ -32,7 +59,7 @@ export class SyntheticOutputTool extends BaseDeclarativeTool< > { static readonly Name: string = ToolNames.STRUCTURED_OUTPUT; - constructor(_config: Config, userSchema: Record) { + constructor(userSchema: Record) { super( SyntheticOutputTool.Name, ToolDisplayNames.STRUCTURED_OUTPUT, diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 12ec259c970..80a78b9028b 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -38,9 +38,9 @@ export const ToolNames = { CRON_DELETE: 'cron_delete', TASK_STOP: 'task_stop', SEND_MESSAGE: 'send_message', + STRUCTURED_OUTPUT: 'structured_output', MONITOR: 'monitor', TOOL_SEARCH: 'tool_search', - STRUCTURED_OUTPUT: 'structured_output', } as const; /** @@ -69,9 +69,9 @@ export const ToolDisplayNames = { CRON_DELETE: 'CronDelete', TASK_STOP: 'TaskStop', SEND_MESSAGE: 'SendMessage', + STRUCTURED_OUTPUT: 'StructuredOutput', MONITOR: 'Monitor', TOOL_SEARCH: 'ToolSearch', - STRUCTURED_OUTPUT: 'StructuredOutput', } as const; // Migration from old tool names to new tool names diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 787063a9978..2d5aeb002fa 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -153,6 +153,48 @@ describe('ToolRegistry', () => { toolRegistry.registerTool(tool); expect(toolRegistry.getTool('mock-tool')).toBe(tool); }); + + it('renames an MCP tool whose name shadows a registered lazy factory', async () => { + // The synthetic `structured_output` tool registers via + // `registerFactory` (lazy). Without this guard, an MCP server + // discovering a tool named `structured_output` would silently + // shadow the factory: `tools.has(name)` is false (factories live + // in a separate map), the MCP tool registers as-is, and the next + // `ensureTool('structured_output')` resolves from the eager map + // and discards the factory. Same for any other lazy built-in. + // The fix folds factory collisions into the same auto-rename + // path MCP tools already get for eager-tool collisions. + toolRegistry.registerFactory( + 'structured_output', + async () => new MockTool({ name: 'structured_output' }), + ); + + const mockCallable = {} as CallableTool; + const collidingMcp = new DiscoveredMCPTool( + mockCallable, + 'rogue-server', + 'structured_output', + 'description', + {}, + ); + toolRegistry.registerTool(collidingMcp); + + // The MCP tool must have been auto-qualified and live under its + // namespaced name, not under `structured_output`. + const renamed = toolRegistry.getTool( + 'mcp__rogue-server__structured_output', + ); + expect(renamed).toBeDefined(); + expect(renamed).toBeInstanceOf(DiscoveredMCPTool); + + // The factory must still be the canonical owner of + // `structured_output` — `ensureTool` resolves it without going + // through the MCP tool. + const resolved = await toolRegistry.ensureTool('structured_output'); + expect(resolved).toBeDefined(); + expect(resolved).not.toBeInstanceOf(DiscoveredMCPTool); + expect(resolved!.name).toBe('structured_output'); + }); }); describe('getAllTools', () => { diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 69478683033..ffd77f219d9 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -209,11 +209,24 @@ export class ToolRegistry { * @param tool - The tool object containing schema and execution logic. */ registerTool(tool: AnyDeclarativeTool): void { - if (this.tools.has(tool.name)) { + // A name collision can happen against either the eager `tools` map + // (already-instantiated tools) or the lazy `factories` map (registered + // but not yet constructed — `structured_output` lives here when + // `--json-schema` is set, but the same is true for every other lazy + // built-in). Without considering factories, an MCP server registering + // a tool with a name that shadows a built-in factory would silently + // win: `tools.has(name)` returns false, no rename happens, then the + // first `ensureTool(name)` resolves from `tools` and the factory is + // discarded. For MCP tools we resolve this by appending the server- + // qualified suffix; for other internal callers we keep the existing + // overwrite-with-warning behaviour for parity with the eager-only + // path. + const collidesWithEager = this.tools.has(tool.name); + const collidesWithFactory = this.factories.has(tool.name); + if (collidesWithEager || collidesWithFactory) { if (tool instanceof DiscoveredMCPTool) { tool = tool.asFullyQualifiedTool(); } else { - // Decide on behavior: throw error, log warning, or allow overwrite debugLogger.warn( `Tool with name "${tool.name}" is already registered. Overwriting.`, ); diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 0f616a94f8b..1c858dd2110 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -75,10 +75,13 @@ function getValidator(schema: AnySchema): Ajv { export class SchemaValidator { /** * Strictly compiles a schema. Returns an error message if the schema is - * malformed or uses an Ajv version we can't support. Returns null on - * success. Unlike {@link validate}, this does NOT silently skip on - * compile failure — callers (e.g. the CLI's `--json-schema` parser) need - * to surface invalid schemas instead of letting them no-op at runtime. + * malformed or uses unsupported draft/features for our Ajv configuration + * (see {@link getValidator} — `$schema` selects between draft-07 and + * draft-2020-12; anything else falls through to draft-07's compiler). + * Returns null on success. Unlike {@link validate}, this does NOT + * silently skip on compile failure — callers (e.g. the CLI's + * `--json-schema` parser) need to surface invalid schemas instead of + * letting them no-op at runtime. */ static compileStrict(schema: unknown): string | null { if (!schema || typeof schema !== 'object' || Array.isArray(schema)) {