feat(cli): add --json-schema for structured output in headless mode#3598
Merged
Conversation
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.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
commented
Apr 25, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Critical] npm test currently fails in packages/core/src/skills/skill-manager.test.ts. The bundled-skill mock only recognizes a path ending in skills/bundled, but SkillManager derives .../src/skills/bundled under source/test execution, so the fake review skill is not returned. Please update the mock to recognize the actual bundled directory path, or derive it from manager.getSkillsBaseDirs('bundled').
— gpt-5.5 via Qwen Code /review
… 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.
wenshao
commented
Apr 25, 2026
…ccept 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.
8 tasks
wenshao
commented
May 1, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[typecheck/Critical] packages/cli/src/nonInteractiveCli.test.ts — ModelMetrics mock（setupMetricsMock 辅助函数）缺少 bySource 属性，tsc 编译失败（TS2741）。虽非本 PR 新增代码，但会阻断 CI 合并。
[Suggestion] packages/cli/src/gemini.tsx:709 — process.exit(0) 无条件覆盖 nonInteractiveCli.ts 中设置的 process.exitCode = 1。CI/CD 管道无法通过退出码检测“模型输出纯文本而非调用 structured_output”的失败条件。
// gemini.tsx:709 (text output path)
process.exit(process.exitCode ?? 0);
// gemini.tsx:685 (stream-json path)
process.exit(process.exitCode ?? 0);
— glm-5.1 via Qwen Code /review
Resolved conflicts: - packages/core/src/tools/tool-names.ts: keep TASK_STOP/SEND_MESSAGE (from main) and STRUCTURED_OUTPUT (from this branch). - packages/cli/src/config/config.ts: keep both SchemaValidator and type MCPServerConfig in the core import. - packages/cli/src/nonInteractiveCli.test.ts: take main's BackgroundTaskRegistry mock (getAll/hasUnfinalizedTasks/abortAll); the prior getRunning method is gone in main.
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.
Collaborator
Author
|
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review |
This was referenced May 2, 2026
yiliang114
previously approved these changes
May 4, 2026
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the latest head and reran the focused structured-output tests locally: CLI json-schema/config/output/non-interactive coverage and core schema/synthetic-output coverage passed. The earlier exit-code and typecheck concerns look addressed, and the change is scoped to headless structured output. LGTM.
…ctured-output # Conflicts: # packages/core/src/tools/tool-names.ts
yiliang114
reviewed
May 4, 2026
…s 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a headless-mode --json-schema flag that enforces structured final output by registering a synthetic structured_output tool whose parameter schema is the user-provided JSON Schema, terminating the session on the first valid call and surfacing the payload in the result envelope.
Changes:
- Introduces
SchemaValidator.compileStrict()for parse-time JSON Schema compilation validation (vs. runtime lenient validation). - Registers a new synthetic
structured_outputtool whenConfig.jsonSchemais set, and adds non-interactive CLI logic to treat the first successful call as the terminal result. - Extends JSON/stream-json result envelopes to include
structured_resultwhile forcingresultto be JSON-stringified when structured output is present (including--output-format text).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/utils/schemaValidator.ts | Adds SchemaValidator.compileStrict() for strict compile-time schema validation. |
| packages/core/src/utils/schemaValidator.test.ts | Adds unit tests for compileStrict(). |
| packages/core/src/tools/tool-names.ts | Adds STRUCTURED_OUTPUT tool name/display name constants. |
| packages/core/src/tools/syntheticOutput.ts | Introduces SyntheticOutputTool, a terminal synthetic tool whose params schema is user-supplied. |
| packages/core/src/tools/syntheticOutput.test.ts | Adds unit tests for SyntheticOutputTool schema passthrough + validation behavior. |
| packages/core/src/index.ts | Re-exports SyntheticOutputTool/StructuredOutputParams as types for downstream compatibility. |
| packages/core/src/config/config.ts | Stores jsonSchema, exposes getJsonSchema(), and lazily registers structured_output tool when set. |
| packages/cli/src/utils/errors.test.ts | Formatting-only test update. |
| packages/cli/src/nonInteractiveCli.ts | Implements structured-output terminal contract and plain-text failure behavior in headless runs. |
| packages/cli/src/nonInteractiveCli.test.ts | Adds coverage for structured-output early termination, suppression rules, retry behavior, and plain-text failure. |
| packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts | Adds structuredResult option and emits structured_result + JSON-stringified result. |
| packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts | Tests presence/absence of structured_result and result stringification behavior. |
| packages/cli/src/gemini.tsx | Ensures main process exits with process.exitCode when set by non-interactive flow. |
| packages/cli/src/config/jsonSchemaArg.test.ts | Adds tests for resolving inline/@file schema args and validation failure modes. |
| packages/cli/src/config/config.ts | Adds --json-schema flag, parse-time resolution/validation (resolveJsonSchemaArg), and yargs-level mode restrictions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
wenshao
commented
May 4, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Critical
C1. packages/cli/src/nonInteractiveCli.ts ~line 425 — max-turns + --json-schema 错误信息误导
当模型在达到 maxSessionTurns 前从未调用 structured_output，走 handleMaxTurnsExceededError 路径，退出码 53、错误信息 「Reached max session turns」——完全没有提及 --json-schema。Plain-text 路径（~line 859）有清晰的错误信息，此路径却没有。
// Before handleMaxTurnsExceededError(config) at line 425:
if (config.getJsonSchema()) {
adapter.emitResult({
isError: true,
durationMs: Date.now() - startTime,
apiDurationMs: totalApiDurationMs,
numTurns: turnCount,
errorMessage:
'Model did not call structured_output before reaching max turn limit.',
usage: computeUsageFromMetrics(uiTelemetryService.getMetrics()),
});
process.exitCode = 1;
return;
}
C2. packages/cli/src/nonInteractiveCli.ts — structured_output 生命周期零 debug 日志
整个文件只有一个 debugLogger.debug（shutdown signal）。关键决策点全无日志。建议在 structured_output 成功调用、校验失败、会话终止、plain-text 错误路径添加 debugLogger.debug()。
C3. packages/cli/src/nonInteractiveCli.ts ~line 684 — drainOneItem drain 循环不识别 structured_output
模型可在 cron/monitor drain 期间调用 structured_output，收到 「accepted」但会话继续运行，违反契约。需要在 drain 循环中添加与主循环相同的 ToolNames.STRUCTURED_OUTPUT 检查。
— deepseek-v4-pro via Qwen Code /review
yiliang114
reviewed
May 4, 2026
wenshao
added a commit
that referenced
this pull request
May 8, 2026
…tools Closes 3 #3589 review threads: - Critical: `setTools()` failure during reveal was silently swallowed via `debugLogger.warn` (off in production). Schemas appeared in `llmContent` so the model thought the tools were callable, but the chat's declaration list never updated — the next call surfaced as an "unknown tool" API error, leaving the session in an unrecoverable degraded state. Now returns a proper `ToolResult.error` with the concrete failure reason and instructions to retry; schemas are withheld from `llmContent` so the model doesn't act on a non-ready tool. - Critical: `collectCandidates` returned every deferred tool that matched `shouldDefer && !alwaysLoad` regardless of whether ToolSearch had already revealed it earlier in the session. Already-revealed tools are in the model's declaration list, so re-surfacing them in later keyword searches wasted tokens and risked the model retrying a load it already had. Filter now also skips tools where `registry.isDeferredToolRevealed(name) === true`. `select:<name>` mode is unaffected (the model may legitimately want to re-inspect the schema of a loaded tool). - Suggestion: `--json-schema` plain-text terminal path set `process.exitCode = 1` and emitted `isError: true` to the JSON adapter, but TEXT-mode users only saw a silent exit-code-1 with no visible context (`emitResult` is a no-op for the TEXT-mode error case). Echo the full `'Model produced plain text instead of calling the structured_output tool as required by --json-schema.'` line to stderr so headless runs are debuggable without scraping `--output-format json`. Tests: 2 new in `tool-search.test.ts`: - `keyword search excludes already-revealed deferred tools`: pins the dedupe behavior across two consecutive searches. - `returns an error result when setTools() throws`: pins that failures don't expose schemas as "ready" and the agent gets the underlying message in `error.message`. 23/23 tool-search.test.ts pass; tsc + ESLint clean. DEFERRED to follow-up PRs (replied on threads): - Critical: structured_output + side-effect-tool race in same turn — needs a pre-scan + synthesized "skipped" tool_results, design overlaps with #3598 PR-2's existing skippedOutput pattern. - Suggestion: `+` prefix parsing edge cases (C++, `+ slack`). - Suggestion: `instanceof DiscoveredMCPTool` hard couple — needs a type tag on AnyDeclarativeTool, broader API surface change. - Suggestion: SyntheticOutputTool registered in interactive mode. - Suggestion: resume scan O(history × parts) early-exit. - Suggestion: deferredToolsSection cap.
6 tasks
wenshao
added a commit
that referenced
this pull request
May 8, 2026
…earch query schema Closes 2 #3589 review threads (Copilot): - nonInteractiveCli: when --json-schema is active and the model emits `[structured_output(...), other_tool(...)]` in the same response, the loop used to keep executing remaining tool calls before terminating. That breaks the documented "first valid call terminates" contract and lets a side-effect tool run AFTER the run is logically over. Add a `break` after recording structuredSubmission so trailing tools in the same batch are skipped. Tools BEFORE structured_output in the batch already executed by the time we reach the synthetic tool — preventing those needs a pre-scan + synthesized "skipped" tool_results and stays as follow-up (overlap with #3598). - tool-search: the `query` parameter schema accepted empty strings, but the runtime guard rejects them — the model could only learn the contract by spending a tool call. Add `minLength: 1` so Ajv catches the empty case at `tool.build()` time. The whitespace-only case (which still has length > 0) stays handled by the runtime trim+empty check. Tests: - new nonInteractiveCli case: model emits `[structured_output, write_file]`; assert executeToolCall ran once (only structured_output), emitToolResult never received the write_file callId, and emitResult landed. - tool-search: `tool.build({ query: '' })` throws via Ajv at build time, matching the actual minLength error message.
…ctured-output # Conflicts: # packages/cli/src/config/config.ts # packages/core/src/core/geminiChat.ts
wenshao
commented
May 9, 2026
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.
This was referenced May 10, 2026
wenshao
added a commit
that referenced
this pull request
May 10, 2026
…hemas (#3589) * feat(tools): add ToolSearch for on-demand loading of deferred tool schemas Large MCP deployments push the function-declaration list past 15K tokens per request. This change lets tools opt out of the initial declaration list via `shouldDefer`, and adds a new `ToolSearch` tool the model calls to fetch schemas on demand — either by exact name (`select:Name1,Name2`) or keyword search with name/description/searchHint scoring. - `DeclarativeTool` gains `shouldDefer`, `alwaysLoad`, `searchHint` opts. - MCP tools default to `shouldDefer=true`; lsp, cron_*, ask_user_question, and exit_plan_mode are flagged too. - `ToolRegistry.getFunctionDeclarations()` filters deferred tools by default; `revealDeferredTool()` re-includes them after ToolSearch loads their schemas. - `getCoreSystemPrompt` appends a "Deferred Tools" list (names + first line of description) so the model knows what's reachable. - Subagent wildcard inheritance keeps including deferred tools so existing `tools: ['*']` configs still see MCP schemas. - Resume-session support: `startChat` scans history for prior calls to deferred tools and re-reveals them so the API doesn't reject follow-up calls. `resetChat` clears the revealed set for a clean slate. - Skipped when ToolSearch is filtered out by the permission manager. * 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(tools): tighten ToolSearch schema + match invocation signature Resolves 2 #3589 review threads: - `max_results` schema: declared as unconstrained `number` but the implementation clamps to the integer range [1, 20]. Updated to `type: 'integer'` with `minimum: 1`, `maximum: HARD_MAX_RESULTS`, `default: DEFAULT_MAX_RESULTS` so the model gets accurate contract guidance and out-of-range values fail validation early instead of silently being clamped after a wasted call. - `execute()` signature now takes `_signal: AbortSignal` to match the base `ToolInvocation.execute` contract. The signal is unused today (the search is sync), but matching the shared signature avoids accidental divergence and makes future cancellation wiring trivial. Test: existing `enforces max_results cap` split into: - schema-rejection (`max_results: 100` → throws at build time) - clamp-on-in-range (`max_results: 20` capped on the candidate side) 21/21 tool-search.test.ts pass; tsc + ESLint clean. * fix(tools,cli): surface ToolSearch reveal failures + dedupe revealed tools Closes 3 #3589 review threads: - Critical: `setTools()` failure during reveal was silently swallowed via `debugLogger.warn` (off in production). Schemas appeared in `llmContent` so the model thought the tools were callable, but the chat's declaration list never updated — the next call surfaced as an "unknown tool" API error, leaving the session in an unrecoverable degraded state. Now returns a proper `ToolResult.error` with the concrete failure reason and instructions to retry; schemas are withheld from `llmContent` so the model doesn't act on a non-ready tool. - Critical: `collectCandidates` returned every deferred tool that matched `shouldDefer && !alwaysLoad` regardless of whether ToolSearch had already revealed it earlier in the session. Already-revealed tools are in the model's declaration list, so re-surfacing them in later keyword searches wasted tokens and risked the model retrying a load it already had. Filter now also skips tools where `registry.isDeferredToolRevealed(name) === true`. `select:<name>` mode is unaffected (the model may legitimately want to re-inspect the schema of a loaded tool). - Suggestion: `--json-schema` plain-text terminal path set `process.exitCode = 1` and emitted `isError: true` to the JSON adapter, but TEXT-mode users only saw a silent exit-code-1 with no visible context (`emitResult` is a no-op for the TEXT-mode error case). Echo the full `'Model produced plain text instead of calling the structured_output tool as required by --json-schema.'` line to stderr so headless runs are debuggable without scraping `--output-format json`. Tests: 2 new in `tool-search.test.ts`: - `keyword search excludes already-revealed deferred tools`: pins the dedupe behavior across two consecutive searches. - `returns an error result when setTools() throws`: pins that failures don't expose schemas as "ready" and the agent gets the underlying message in `error.message`. 23/23 tool-search.test.ts pass; tsc + ESLint clean. DEFERRED to follow-up PRs (replied on threads): - Critical: structured_output + side-effect-tool race in same turn — needs a pre-scan + synthesized "skipped" tool_results, design overlaps with #3598 PR-2's existing skippedOutput pattern. - Suggestion: `+` prefix parsing edge cases (C++, `+ slack`). - Suggestion: `instanceof DiscoveredMCPTool` hard couple — needs a type tag on AnyDeclarativeTool, broader API surface change. - Suggestion: SyntheticOutputTool registered in interactive mode. - Suggestion: resume scan O(history × parts) early-exit. - Suggestion: deferredToolsSection cap. * fix(cli): honor process.exitCode in headless main exit The two non-interactive exit paths in `main()` hardcoded `process.exit(0)` after `runNonInteractive` / `runNonInteractiveStreamJson` returned. This silently overwrote any `process.exitCode = 1` set inside the run — most visibly the `--json-schema` plain-text contract: the JSON adapter emits `isError: true` and stderr gets the explanation, but the shell saw exit code 0 and assumed success. Replace the hardcoded 0 with `process.exit(process.exitCode ?? 0)` on both paths so non-zero exits propagate. The success case is unchanged (exitCode is undefined → exits 0). * test(cli): add integration tests for --json-schema and ToolSearch Closes review-flagged coverage gaps for #3589: `json-schema.test.ts` (6 cases) covers the headless structured-output contract end-to-end: - structured_result emits when the model fills the schema (success path) - @path/to/schema.json file-load works - parse-time validation rejects invalid JSON, invalid JSON Schema, and missing files (no LLM, fast) - plain-text path: when structured_output is not callable (`--exclude-tools structured_output`), the run exits 1 with `is_error: true` and the contract error message — locks in the exit-code fix from the prior commit. `tool-search.test.ts` (3 cases) covers the deferred-tool flow: - select:<name> reveals a tool and the model can invoke it in the same turn (asserts call order so a missed reveal would surface as an unknown-tool API error instead of a silent pass) - keyword query (no select: prefix) hits the tool_search tool - feature-flag-off: with experimental.cron disabled, cron tools are never registered and never appear in tool calls LLM-dependent tests use the cron tools as a deterministic deferred target (gated by experimental.cron, no MCP server required). * fix(cli,core): tighten --json-schema validation Closes 3 #3589 review threads: - Schemas like `{"type":"string"}` and `{"type":"array"}` compiled fine (they're valid JSON Schemas in isolation), but the `--json-schema` value becomes the synthetic structured_output tool's parameter schema and tool-call arguments are object-shaped. Reject any non-undefined top-level type that is not "object" so the user sees the contract violation at parse time, not as an unrecoverable runtime mismatch. - `SchemaValidator.compileStrict` accepted arrays since `typeof [] === 'object'` — Ajv would later emit a confusing error. Add an explicit `Array.isArray` guard so the contract stated by the function name is honored at the boundary. - `compileStrict` shared the project-wide Ajv instances configured with `strictSchema: false` (intentionally lenient so MCP servers can ship custom keywords without breaking runtime validation). That leniency is wrong for the `--json-schema` surface — typos like `propertees` were silently ignored. Compile inside a dedicated `strict: true` Ajv so user-supplied schemas surface mistakes immediately. Tests: - jsonSchemaArg: rejects non-object top-level type ("string"/"array"). - schemaValidator.compileStrict: rejects arrays; flags unknown keywords (typos) under strict mode. * fix(tools): roll back ToolSearch reveals when setTools() throws Closes 1 #3589 review thread. `loadAndReturnSchemas` revealed each requested tool BEFORE calling `setTools()` because `getFunctionDeclarations()` filters by the revealedDeferred set — the reveal has to be in place when setTools() rebuilds the chat's declaration list. But if setTools() throws (e.g. chat not yet initialised), the registry was left holding orphaned reveals: the tool was marked "revealed" while the API never received its schema. Subsequent keyword searches would then exclude that tool from candidates (per `collectCandidates`'s isDeferredToolRevealed filter), making it unreachable until `/clear`. Track the names this call NEWLY revealed (skipping tools that were already revealed by an earlier ToolSearch in the same session) and unreveal them on setTools() failure. Added `unrevealDeferredTool` to the registry as the one-tool inverse of `revealDeferredTool`; `clearRevealedDeferredTools` is unchanged and still wipes the whole set on `/clear`. Test: extends the existing `setTools() throws` test to also assert that (a) the failed call's reveal is rolled back and (b) a tool revealed by an earlier call stays revealed (not whole-set wiped). * test(cli): unit-cover --json-schema runtime branches Closes one of the test-coverage gaps in #3589 reviews (gpt-5.5 review S8). Adds two deterministic L1 unit tests in nonInteractiveCli.test.ts that mock the LLM at sendMessageStream — no model API hit, no flake, ~10ms total. 1. structured_output success path: model fires the synthetic tool once, runtime sets structuredSubmission, aborts background tasks, and emitResult fires exactly once with `structuredResult` matching the model's args. No follow-up turn is issued (single-shot contract). 2. plain-text error path under --json-schema: model emits text only; runtime sets process.exitCode=1, writes the contract-violation line to stderr, and emits an isError result with the canonical "Model produced plain text" message. Both tests inject a stub adapter via runNonInteractive's `options.adapter` hook, so they assert against direct emitResult calls instead of parsing JSON stdout. process.exitCode is snapshot/restored to keep the test hermetic. The L2 integration tests in integration-tests/cli/json-schema.test.ts remain as smoke coverage against a real model. * fix(cli,core): support type-union arrays in --json-schema Resolves 2 regressions introduced by the previous schema-hardening commit (3872656): - The strict Ajv now uses `allowUnionTypes: true` so spec-valid type unions like `{"type":["string","number"]}` and `{"type":["object","null"]}` compile cleanly. Strict mode rejects those by default; without the opt-in, real-world nullable-field idioms broke at CLI parse time. - The CLI's top-level type guard now treats a `type` array containing "object" as object-allowed, instead of insisting on the bare string. `{"type":["object","null"]}` is the canonical way to allow a nullable object root and was being incorrectly rejected. Both regressions were flagged on the PR by gpt-5.5 and Copilot. Deeper root-shape analysis (anyOf/oneOf/not combinators, e.g. an `anyOf` whose branches all forbid objects) is intentionally NOT added here — partial checks would either give false reassurance or wrongly reject valid composed schemas. The strict-Ajv compile is the right place to catch those cases; tracking as follow-up. Tests: jsonSchemaArg accepts `["object","null"]` and rejects union arrays without "object"; compileStrict accepts type-union arrays. * fix(tools): cap select: mode in ToolSearch by max_results Closes 1 #3589 review thread (Copilot). The public `max_results` parameter (clamped to [1, 20]) was only honored on the keyword-search path. `select:` mode looped through the full comma-separated list and returned every requested schema, so `select:a,b,c,...` could load and stringify an unbounded number of full tool schemas — token bloat and a misleading public contract. Cap select: by `max_results` after dedup. Truncation is silent and deterministic (first N) so the model can re-issue another ToolSearch for the rest if it actually needs them — matches the existing keyword-search truncation semantics. * fix(tools): treat null GeminiClient like setTools() failure in ToolSearch Closes 2 #3589 review threads: - The previous rollback fix only handled `setTools()` throwing. When `getGeminiClient()` returned null (e.g. ToolSearch fires before the client is initialised), optional chaining silently no-op'd while the reveals stayed in the registry. The dedupe filter in `collectCandidates` would then exclude those tools from future keyword searches, making them unreachable until `/clear`. Replace `?.setTools()` with an explicit null check; treat null identically to a throw — same rollback path, same `ToolResult.error` surface. - Stale comment in the catch block claimed the schemas "appear in llmContent" even on failure. The implementation actually withholds schemas on error (the tests assert this explicitly). Updated the comment to match. Test: existing 'rolls back when setTools() throws' is unchanged; new 'treats a null GeminiClient identically' pins the same contract for the null-client branch. * fix(cli): use boolean sentinel for structured_output submission Closes 1 #3589 review thread (Copilot, posted 3 times against the same branch). The `structuredSubmission !== undefined` sentinel collapsed two distinct states into one value: "no submission yet" and "submission recorded with undefined args". The latter is reachable under a permissive empty schema (`{}`) since `BaseDeclarativeTool.validateToolParams` would have already accepted the call regardless of arg shape, and some content-generator adapters may surface a no-arg model call as `args: undefined`. In that case the run would have fallen through to the normal continuation loop instead of terminating, breaking the single-shot contract. Track submission via a separate `hasStructuredSubmission` boolean. The recorded value of `structuredSubmission` (which lands in `structured_result`) is preserved verbatim — including `undefined` — so structured_result reflects exactly what the model submitted. Test: new 'terminates even when structured_output args are undefined' pins the contract; the boolean lets us assert the early-return path runs even though the recorded value is itself undefined. * fix(cli): finish structured_output sentinel cleanup + reject stream-json combo Closes 2 #3589 review threads (Copilot): - `BaseJsonOutputAdapter.buildResultMessage` had the same `!== undefined` sentinel that 21c48e9 just fixed in `nonInteractiveCli.ts`. The adapter side still collapsed "no submission" with "submitted-as-undefined", so a model call to structured_output with no args (legitimate under empty schema `{}`) would silently fall back to the free-text `result` and drop the `structured_result` field — exactly the contract failure the runtime fix was meant to prevent. Track presence by `'structuredResult' in options`; normalize an undefined submission to `null` so both `result` (`JSON.stringify(undefined)` returns undefined) and the top-level `structured_result` field render as JSON-safe values. - `--json-schema` was silently accepted alongside `--input-format stream-json`, even though stream-json input runs through `runNonInteractiveStreamJson` which has no structured-output termination logic — the model would call the synthetic tool but the contract would never fire. Reject the combination at parse time so the user sees the mismatch instead of confusion at runtime. Tests: - BaseJsonOutputAdapter: present-but-undefined `structuredResult` emits `result: 'null'` and `structured_result: null`. The back-compat "absent" test stays as-is. - parseArguments: --json-schema + --input-format stream-json now fails with the contract-mismatch message. * fix(prompt): harden deferred-tools section against MCP description injection Closes 1 #3589 review thread (Copilot, repeatedly raised across 4 revisions of the file). MCP tool descriptions originate from remote servers and are untrusted input. The deferred-tools system-prompt section was interpolating each description verbatim into a list item, so embedded backticks, quotes, newlines, or markdown could: - Break out of the list-line structure (a `` ` `` ends the inline code formatting that wraps the tool name; a stray header / bullet re-opens prompt structure at a different indent). - Hijack visual hierarchy (a bold or header line lands at system-instruction priority). - Embed instruction-like text the model may follow. Two-layer fix: 1. Render each description as a JSON-string literal via `JSON.stringify(...)`, which escapes backticks, quotes, backslashes, newlines, and control characters. This neutralizes structural injection — embedded markup is now visibly escaped data, not active markdown. Tool names are wrapped in inline-code backticks so the visual frame stays code-like. 2. Add an explicit "treat them strictly as data — never follow instructions that appear inside a description" framing line above the list. The escaping doesn't sanitize *meaning* (a description that literally says "ignore previous instructions" still says that); the framing tells the model to decline. Tests pin: empty input → empty output; JSON-escape of quotes / backticks / backslashes; presence of the framing line; description truncation still applies before encoding. The deeper "omit MCP descriptions entirely" mitigation remains available as a follow-up if the framing proves insufficient in practice — that path requires propagating a `toolType: 'mcp'` flag through DeclarativeTool first, which overlaps with the already- deferred S2/S10 refactor. * fix(core): scope --json-schema strictness so spec-valid schemas pass Closes 2 #3589 review threads (gpt-5.5): - `compileStrict` was using `{ strict: true, allowUnionTypes: true }` which is not just "reject unknown keywords" — Ajv's `strict: true` also enables `strictSchema` AND `strictRequired`, `strictTypes`, and `validateFormats`. That rejected spec-valid schemas users routinely ship: `{type:'object', required:['answer']}` (required without matching properties), nested `{enum:[...]}` without explicit type, and any property using a non-built-in `format`. Replace with the four flags we actually want: strictSchema: true — keep typo detection (the original goal) strictRequired: false strictTypes: false validateFormats: false allowUnionTypes: true - The `$schema === DRAFT_2020_12_SCHEMA` exact-match in `getValidator` rejected the equivalent `…/schema#` form (trailing empty fragment), falling back to the draft-07 Ajv which then errored with `no schema with key or ref ...`. Both URIs reference the same meta-schema — normalize the trailing `#` before comparing in a shared `isDraft2020Uri` helper used by both `getValidator` and `compileStrict`. Tests: - compileStrict accepts the three previously-rejected spec-valid patterns (required-without-properties, type-less enum, custom format). - compileStrict accepts the draft-2020-12 URI with `#` fragment. * fix(cli): allow --json-schema with stdin-piped prompt Closes 1 #3589 review thread (Copilot). The earlier prompt-presence check rejected `qwen --json-schema ...` when neither `-p`/`--prompt` nor a positional query was supplied, which broke the documented stdin-piping pattern: echo "What's 2+2?" | qwen --json-schema '{"type":"object",...}' Headless `runNonInteractive` reads stdin when no prompt argument is present. Gate the rejection on `process.stdin.isTTY` so the only case that fails parse-time is a true interactive invocation with no prompt anywhere (the actual error mode). Stdin-piped runs proceed to the regular non-interactive flow where structured-output termination already applies. Test: parity pair — - isTTY=true + no prompt → fails with "applies to non-interactive" - isTTY=false (piped) + no prompt → parseArguments succeeds * fix(cli,tools): short-circuit after structured_output + tighten ToolSearch query schema Closes 2 #3589 review threads (Copilot): - nonInteractiveCli: when --json-schema is active and the model emits `[structured_output(...), other_tool(...)]` in the same response, the loop used to keep executing remaining tool calls before terminating. That breaks the documented "first valid call terminates" contract and lets a side-effect tool run AFTER the run is logically over. Add a `break` after recording structuredSubmission so trailing tools in the same batch are skipped. Tools BEFORE structured_output in the batch already executed by the time we reach the synthetic tool — preventing those needs a pre-scan + synthesized "skipped" tool_results and stays as follow-up (overlap with #3598). - tool-search: the `query` parameter schema accepted empty strings, but the runtime guard rejects them — the model could only learn the contract by spending a tool call. Add `minLength: 1` so Ajv catches the empty case at `tool.build()` time. The whitespace-only case (which still has length > 0) stays handled by the runtime trim+empty check. Tests: - new nonInteractiveCli case: model emits `[structured_output, write_file]`; assert executeToolCall ran once (only structured_output), emitToolResult never received the write_file callId, and emitResult landed. - tool-search: `tool.build({ query: '' })` throws via Ajv at build time, matching the actual minLength error message. * fix(prompt,tools): escape backticks in tool names + report select: truncations Closes 2 #3589 review threads (deepseek): - Deferred-tools system-prompt section interpolated tool names raw into inline-code spans. MCP names can contain backticks (the protocol allows arbitrary strings), and a literal `` ` `` in the name closed the inline-code formatting and exposed the rest of the name into the prompt body as plain markdown — same injection vector the description hardening was meant to close, just via a different field. Added a small `escapeBacktick(name)` helper and applied it both inside the per-tool list line AND inside the `select:${firstName}` example in the section preamble. - ToolSearch `select:` mode silently dropped names beyond `max_results` — the model had no way to know which tools were skipped and would later receive "unknown tool" API errors when trying to call them. Collect the truncated names alongside the kept ones, surface them in `llmContent` as `Truncated by max_results — request these in a follow-up call: …`, and add a per-count display segment. Tests: - prompts: name with embedded backticks renders escaped in BOTH the list line and the section preamble example. - tool-search: select-truncation test now also verifies the "Truncated by max_results" header and that dropped names appear in the truncation list (and loaded names do not). * fix(prompt): JSON-quote tool names instead of incomplete backtick escape Closes 1 #3589 review thread (CodeQL: incomplete-string-escaping). The previous round wrapped tool names in inline-code (`` \`${name}\` ``) and tried to escape embedded backticks with `s.replace(/\`/g, '\\\`')`. That fix was structurally wrong: markdown inline-code spans don't honor backslash escapes, so a name containing `` ` `` would still close the surrounding code span — the escape only added a stray backslash inside the rendered text. CodeQL surfaced it as "incomplete escaping" because we escaped one metachar (`` ` ``) but not its companion (`\`); fixing that escape would still not solve the underlying markdown problem. Render names via `JSON.stringify(name)` instead — the entire string becomes a quoted literal with quotes and backslashes JSON-escaped, and no inline-code span surrounds the value, so an embedded backtick is just a plain character with nothing to break out of. The section's example sentence (`select:NAME`) still uses inline-code formatting because it's prescribing a literal command. Pick the first backtick-free tool name as the example; fall back to a `<tool_name>` placeholder when every tool has a backtick. Drop the now-unused `escapeBacktick` helper. Tests: - update existing JSON-encoding test to expect the new `- "name": "desc"` form. - new: name with embedded backticks renders JSON-quoted (no inline-code wrap and no incomplete escape sequences). - new: example name skips backtick-bearing tools. - new: example falls back to `<tool_name>` placeholder when every name has a backtick. * fix(tools): escape `<` in ToolSearch schema blocks to prevent wrapper injection Closes 1 #3589 review thread (Copilot). `loadAndReturnSchemas` wraps each schema in `<function>...</function>` pseudo-XML. JSON.stringify preserves `<` as-is, so a tool description (or any string field) containing `</function>` would prematurely close the wrapper — text after the embedded close tag would escape into model-visible content alongside the schemas, opening a path for adversarial MCP servers to inject visible-but-orphaned instructions. Replace `<` with `<` in the JSON-stringified schema. The unicode escape decodes back to `<` if the model interprets the JSON, but as raw text inside the wrapper it's no longer the start of a closing tag. The fix is symmetric with the recent prompt-name JSON quoting (e39948e): both surfaces now refuse to let untrusted MCP strings break their containing markup. Test: a tool with `description: '... </function> ...'` now renders as `</function>` and the result has exactly one closing tag. * fix: address #3589 wave 2 — Critical reveal/race + revealed-set hygiene Critical correctness: - `client.ts`: when ToolSearch is filtered out (allow/deny rules, `--exclude-tools tool_search`), eagerly reveal every deferred tool so they all land in the function declaration list. Without this the user sees those tools just disappear silently — the deferred- tool discovery surface is gone, but the tools are still hidden by the registry filter, so they're effectively invisible AND uncallable. Token-saving rationale of deferral was predicated on the discovery surface being available; if not, eager reveal preserves the invariant "all registered tools are callable". - `config.ts`: `--json-schema` now requires the root schema to declare `type: "object"` (or array containing it). Tool-call args are always validated as objects, so root-only `anyOf` / `oneOf` / `allOf` / `not` would create schemas the model can't consistently satisfy — surface as a startup error instead of mid-session "Model produced plain text" failures users can't easily diagnose. - `nonInteractiveCli.ts`: structured_output + sibling tools in the same turn no longer leaks side effects. Pre-scan reorders structured_output to the front of `toolCallRequests`; once it succeeds, sibling tools (write_file, shell, …) get a synthesized `Skipped: this turn's structured_output contract took precedence as the terminal output. Re-issue this call in a separate turn if needed.` tool_result instead of running. If structured_output fails (e.g. validation), siblings still execute via the normal loop body, same as a turn that didn't issue structured_output at all. Reveal-set hygiene: - `tool-registry.ts`: `removeMcpToolsByServer`, `removeDiscoveredTools`, and `discoverToolsForServer` (the re-discovery path) now also drop the affected tool names from `revealedDeferred`. Without this, an MCP server disconnect / reconnect that re-registers a tool of the same name inherits `revealed: true` from before the disconnect — the schema lands in `getFunctionDeclarations` before the model has any way to know the tool exists this session. Defensive: - `config.ts`: `resolveJsonSchemaArg` caps `@path/to/schema.json` reads at 4 MiB. Real schemas are well under (decompose with `$ref` if needed); the cap catches accidental wrong-path arguments (`@./node_modules/.cache/*.json`) before they OOM `fs.readFileSync` + `JSON.parse`. Tests: - New regression in `tool-registry.test.ts` for the `removeMcpToolsByServer` revealedDeferred prune. - 23/23 tool-search.test.ts, 23/23 tool-registry.test.ts, 226/229 nonInteractiveCli.test.ts (3 skipped pre-existing), 195/197 config.test.ts (2 skipped pre-existing) — all pass. Deferred to follow-up (replied + tracked): - 10-positional-param API on DeclarativeTool (refactor breadth). - `instanceof DiscoveredMCPTool` (needs `toolType` tag). - `structured_result` intersection vs canonical interface. - Resume-scan error/permission-denied filter + early-exit. - `getAllTools()` sort discarded (perf, ~negligible). - DeferredTools section cap. - `setTools` → `warmAll` undercutting deferral (theoretical; factories are nearly empty in practice today). * fix(tools,cli): select: quote-strip + import order Closes 2 fresh #3589 review threads: - `tool-search.ts`: `select:` mode now strips a single layer of matching `"…"` / `'…'` from each tool name before lookup. Models often paste names back verbatim from the deferred-tools system prompt section, which renders them as JSON string literals (`"cron_list"`); without quote-strip the lookup searches for the literal-with-quotes name and misses every time. - `nonInteractiveCli.ts`: moved the `import { writeStderrLine } …` to sit with the other top-of-file imports (eslint-plugin-import's `import/first` rule) and hoisted `createDebugLogger(...)` below the imports — was wedged between them. Test: new `select: tolerates JSON-quoted tool names` regression in tool-search.test.ts pins both `"…"` and `'…'` shapes; 29/29 pass. * fix(tools,cli): isolate ensureTool failures + enrich --json-schema error Closes 2 #3589 review threads (deepseek-v4-pro): - ToolSearch.loadAndReturnSchemas: an `ensureTool()` throw mid-batch used to propagate out of the for loop with previous tools already revealed but never setTools()-synced — same orphaned-reveal failure the setTools() catch block guards against. Wrap ensureTool in try/catch so a failure surfaces as a `missing` entry and the rest of the batch is processed normally; the throw is logged at debug level for diagnostics. - nonInteractiveCli `--json-schema` plain-text error: the static message gave operators no diagnostic context. Now appends turn count + a JSON-quoted preview of the model's actual plain text (capped at 200 chars across all turns). Operators debugging a headless run no longer need to scrape `--output-format json` to understand why the contract failed; the stderr line and the JSON result both carry the same enriched body. Tests: - ensureTool throws on bravo mid-batch; alpha + charlie still load and reveal, bravo reported missing, registry stays consistent (bravo NOT revealed). - existing plain-text error test now also asserts the turn-count suffix and the model's actual content ("plain answer") shows up in both emitResult and stderr. Not done: deepseek's MCP `__` segment-boundary scoring suggestion turned out to be a non-issue on inspection — `endsWith('_'+term)` already matches every case `endsWith('__'+term)` would catch (the latter is a subset of the former since `__term` always ends with `_term` too). Reverted the proposed change after the test exposed that the boundary is already covered. Filing a thread reply. * test(core): cover startChat deferred-tool branches Closes 1 #3589 review thread (deepseek-v4-pro): the existing client test mocked `getDeferredToolSummary: () => []` and `getTool(TOOL_SEARCH): () => null`, which short-circuited every deferred-tool code path in `startChat()` — ~50 lines of logic (resume re-reveal, no-ToolSearch eager-reveal, already-revealed filter) were unreachable from tests. Add `isDeferredToolRevealed` to the base registry mock so default tests don't crash, then add a `describe('startChat — deferred tools')` block with three cases: 1. Resume scan: history with a `functionCall` to a deferred tool re-reveals exactly that tool; siblings stay deferred. Pins the resume-rejected-tool guard. 2. ToolSearch unavailable: every deferred tool is revealed eagerly so the model can still reach them via the regular declaration list. Pins the silent-disappearance fix. 3. ToolSearch available + no history match: nothing is revealed (deferral is preserved). Pins the negative case so future refactors can't regress to "always reveal everything". * test(tools): pin MCP `__` suffix already scores as exact (12), not substring (6) #3589 review thread suggested adding an explicit `isMcp && nameLower.endsWith('__' + term)` arm to the MCP scoring path on the assumption that the existing `endsWith('_' + term)` fails to match `mcp__server__toolname` patterns. Verified the premise is incorrect: `endsWith('_x')` returns true for strings ending in `__x` because the last 2 chars (`_x`) are present. JS verification: `'mcp__slack__send_message'.endsWith('_send_message')` → true; same for `'_issue'` on `'mcp__github__create_issue'` etc. So the suggested code change would have been a redundant no-op (adding an OR-arm that fires only when the existing arm already matches). Instead, lock the existing behavior in with a regression test that asserts MCP tools get the exact-suffix score (12) on both the trailing tokenized toolname and a single tail token — so a future refactor to a tighter word-boundary regex can't silently downgrade MCP scoring without the test catching it. 30/30 tool-search.test.ts pass. * test(cli,core): cover --json-schema pre-scan + resetChat reveal cleanup Closes 2 #3589 review threads (glm-5.1): - nonInteractiveCli.test.ts: the existing batch test put structured_output at index 0, so the pre-scan reorder branch and the validation-failure fallback were both unreachable. The inline comment claimed "tracked as follow-up", but the pre-scan is now in shipped code (nonInteractiveCli.ts:509-535) since 9588231. Two new cases: 1. "reorders structured_output before side-effect tools so siblings never run": batch ordered as [write_file, structured_output] — pre-scan must hoist structured_output to position 0, then break-after-success keeps write_file from executing. Pins the irreversible-side-effect guard. 2. "lets siblings run when structured_output validation fails so the model can retry": batch ordered as [structured_output(bad args), write_file] — structured_output's executeToolCall fails, hasStructuredSubmission stays false, sibling runs normally, loop falls through to second turn (model gives up with plain text) and the plain-text terminal branch fires. Pins the fallback semantics. Also updates the existing test's stale comment to point at the new sibling case rather than claiming the pre-scan is still TODO. - client.test.ts: `resetChat()` now calls `clearRevealedDeferredTools()` (added back when /clear behavior was sorted out), but no test asserted it. A regression here would silently carry deferred-tool reveals across `/clear`, defeating the clean-slate expectation. New test pins the call. * docs(tools): clarify ToolSearch description — fetch decl, callable next turn Closes 1 #3589 review thread (Copilot). The previous description said ToolSearch returns the matched tools' "complete JSONSchema definitions" and that "once a tool's schema appears in that result, it is callable exactly like any tool defined at the top of the prompt." Both phrasings could lead the model to assume the returned `<functions>` block itself made the tool invocable in the same turn. Reality: ToolSearch returns full function declarations (name + description + parameter schema), reveals them in the registry, and calls `setTools()` to update the active chat's declaration list. The schema becomes a real callable tool only on the NEXT model turn. Reword the description to make this two-step contract explicit so a model can't waste a turn trying to call a "callable schema" embedded in the same response. No test changes — none assert the description text verbatim and the new wording keeps the same query-form summary the keyword tests exercise. * docs(cli): correct pre-scan comment — siblings are skipped, not synthesized Closes 1 #3589 review thread (Copilot). The pre-scan comment claimed siblings receive a "synthesized 'skipped' tool_result" after structured_output succeeds. The implementation actually breaks out of the loop without emitting any tool_result for the skipped calls. The transcript is missing the function_response entries for them, but the session terminates via emitResult immediately so no follow-up API call ever sees the mismatch — the missing entries are harmless in the single-shot contract. Update the comment to describe what the code actually does. The existing tests already pin the contract (no executeToolCall for the skipped tool, no emitToolResult for its callId). * fix(tools,cli): scope ToolSearch reveal/setTools to deferred + drop duplicate stderr Closes 3 #3589 review threads (Copilot + deepseek-v4-pro): 1. ToolSearch was calling `revealDeferredTool` AND triggering `setTools()` for every tool that `select:` resolved, including non-deferred / `alwaysLoad` tools (the model is allowed to use `select:` to re-inspect any tool's schema, including core ones). That polluted `revealedDeferred` with names that aren't deferred AND could fail with `GeminiClient not initialised` for what is purely a schema-inspection call. Gate both reveal and the setTools() trigger on `tool.shouldDefer && !tool.alwaysLoad`, and only call setTools() when this call newly revealed at least one deferred tool. 2. The `--json-schema` plain-text fallback wrote the error message to stderr via `writeStderrLine(...)` AFTER calling `adapter.emitResult({isError:true,...})`. The JsonOutputAdapter already writes `errorMessage` to stderr in TEXT-mode isError responses (see JsonOutputAdapter.ts:68-73), so the extra line produced two copies of the same message in headless TEXT runs. The comment claiming `emitResult` was a no-op in TEXT mode was wrong. Remove the duplicate write and the unused `writeStderrLine` import; let the adapter own per-format surfacing. 3. agent-core's wildcard-subagent path uses `getFunctionDeclarations({ includeDeferred: true })` so subagents inherit MCP / lsp / cron_* tools, but no test exercised it — the existing mocks returned `getFunctionDeclarations: () => []` and `tools: ['*']` was never asserted. A refactor that silently dropped `includeDeferred` would break existing wildcard subagent configs without warning. Add three cases: - tools:["*"] inherits deferred tools (asserts the call args passed to getFunctionDeclarations). - absent toolConfig also takes the wildcard path. - explicit tools list does NOT use the wildcard branch (uses getFunctionDeclarationsFiltered instead). Tests: - tool-search: select: a non-deferred tool does not reveal + does not call setTools. Same for alwaysLoad tools. - nonInteractiveCli: existing plain-text test no longer asserts on a stderr `qwen --json-schema:` line; the adapter is responsible for that surfacing per format. - agent-core: 3 new prepareTools cases as described above. * test(cli): pin contextCommand passes includeDeferred to getFunctionDeclarations Closes 1 #3589 review thread (deepseek-v4-pro): the `{ includeDeferred: true }` arg in `collectContextData` is what keeps the "all tools" token estimate aligned with the per-tool breakdown (which iterates `getAllTools()` unfiltered). If a refactor silently dropped the option, `displayBuiltinTools` (clamped via `Math.max(0, …)`) would collapse to 0 — visible in `/context detail` but not caught by anything. New focused test stands up minimal Config / ToolRegistry mocks, calls the exported `collectContextData(...)`, and asserts the spy on `getFunctionDeclarations` was invoked exactly once with `{ includeDeferred: true }`. The token-math itself is not a target of this test (it's covered by the visible UI); the contract being pinned is the call argument. * fix(tools): surface ToolSearch ensureTool/setTools failures to stderr Closes 1 #3589 review thread (deepseek-v4-pro): previously the `ensureTool()` and `setTools()` failure paths only logged via `debugLogger.warn`, which is a no-op when DEBUG is unset (the production default). Operators running headless against a freshly- initialised session would see opaque "missing" entries or `setTools failed` ToolResult errors with no upstream diagnosis. Mirror each `debugLogger.warn` with a `process.stderr.write` so the underlying cause (factory throw, chat-not-initialised, network) is visible in the run's stderr stream regardless of DEBUG. Used `process.stderr.write` directly rather than `console.warn` because the core package's eslint config bans `console.*` in src and there is no shared cross-package "operator-visible logger" yet (filing that as a separate follow-up — `core` and `cli` would both benefit). The `[ToolSearch]` prefix tags the source so multi-source headless logs can grep cleanly. The existing tests don't spy on stderr so no test changes were required; the new writes show up only on real failure paths. --------- Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
…ctured-output # Conflicts: # packages/cli/src/config/config.ts # packages/cli/src/config/jsonSchemaArg.test.ts # packages/cli/src/gemini.tsx # packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts # packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts # packages/cli/src/nonInteractiveCli.test.ts # packages/cli/src/nonInteractiveCli.ts # packages/core/src/config/config.ts # packages/core/src/tools/syntheticOutput.test.ts # packages/core/src/tools/syntheticOutput.ts # packages/core/src/utils/schemaValidator.test.ts # packages/core/src/utils/schemaValidator.ts
tanzhenxin
previously approved these changes
May 11, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
The design is clean: one synthetic tool per run, parameter schema = user JSON Schema, Ajv-validated, gated to headless mode. The CLI-side hardening is the right shape — parse-time satisfiability gating on the schema, same-turn batch handling with synthesised tool_results for suppressed siblings, and explicit yargs rejection of incompatible flag combinations. Test coverage is unusually thorough. Two narrow edge cases worth noting for potential follow-up — neither blocks merge.
1. Suppressed sibling tool_use blocks aren't paired in the recorded session on the success path (severity: low · confidence: medium)
When a same-turn batch like [write_file, structured_output] lands and structured_output succeeds, the non-structured sibling is suppressed and a synthetic "Skipped" tool_result is emitted to the SDK event stream — good. But the success path returns before the next user-turn send, so the synthesised response parts never make it into the chat-recorded JSONL. The assistant-side functionCall is recorded; the matching functionResponse isn't. A subsequent qwen --continue on that recorded session would replay an unpaired tool_use block to Anthropic / OpenAI and get rejected. The retry path doesn't have this asymmetry because the synthesised parts ride into the next sendMessageStream call. --continue after a structured-output success isn't a documented workflow, which is the only reason this is low-severity.
2. Satisfiability gate misses not.anyOf that excludes every object (severity: low · confidence: high)
The parse-time gate inspects not.type directly and rejects schemas where not directly forbids object. But a schema like {not: {anyOf: [{type: "object"}]}} excludes every object value (an object matches the inner anyOf, so not rejects it) and slips past the gate. The synthetic tool registers, every model call fails Ajv validation, and the run grinds to maxSessionTurns. The hint added to the max-turns error helps users diagnose it, but the gate itself doesn't catch this construct. Exotic, and the code comment already notes "those fall through to Ajv at runtime" — worth tightening only if a follow-up is taken.
Verdict
APPROVE — Both findings are narrow edge cases on undocumented or exotic paths; ready to land.
wenshao
commented
May 11, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
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.
wenshao
commented
May 11, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] packages/cli/src/nonInteractive/session.ts:397,427 — runNonInteractive() return type changed from Promise<void> to Promise<number> (explicit exit code), but both callers in session.ts ignore the return value. While --json-schema is rejected with --input-format stream-json at parse time (so the structured-output error path can't be reached in session mode today), this is a contract gap for future scenarios where runNonInteractive might return non-zero in session contexts.
wenshao
commented
May 11, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Additional finding from delayed review agent (Correctness):
[Suggestion] packages/cli/src/config/config.ts:~447 — When schemaRootAcceptsObject rejects a schema because of a root-level $ref, the error message says "root must accept object-typed values ... must include object". The actual rejection reason is the $ref keyword itself, not the referenced sub-schema (which could describe a perfectly valid object like {"$ref":"#/$defs/MyObj","$defs":{"MyObj":{"type":"object"}}}). Users get a confusing error instead of actionable guidance (wrap the $ref in allOf).
Consider adding a specific $ref check before calling schemaRootAcceptsObject, so the error can say:
--json-schemadoes not accept a root$ref; wrap it inallOf(e.g.{"allOf":[{"$ref":"#/$defs/MyObj"}]}) so the schema root describes the tool arguments directly.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
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.
tanzhenxin
approved these changes
May 11, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Re-review after the two new commits. The not-handler narrowing correctly closes the false-positive the self-review flagged: the gate now only rejects when not is bare {type:"object"} (or {type:["object",…]}) with no other keys, so schemas like {not:{type:"object",required:["error"]}} survive correctly because {} is still a valid object value. New accept-path tests pin the three sibling-keyword cases.
The dedup refactor extracts the per-batch structured_output handling out of the main-turn loop and the drain-turn into a single helper. Traced both call sites end-to-end against the prior diff — the contract is preserved (pre-scan filter, per-request execute, modelOverride capture via the caller-supplied setter, first-valid-call breaks the loop, post-loop synthesis with success/retry wording). The STRUCTURED_OUTPUT_REDACTED_ARGS hoist is a clean constant extraction shared between telemetry and chat-recording, with spread at each call site to keep the object mutable per consumer.
Both prior minor findings (unpaired tool_use blocks in the JSONL on the success path, and not.anyOf excluding all objects) are still present — but they're narrow edge cases on undocumented or exotic paths and the satisfiability gap is already acknowledged out-of-scope in the code comment. Worth filing as follow-ups if anyone wants to tighten them further; not a blocker.
Verdict
APPROVE — both new commits land cleanly with no behavioral drift on the refactor and a correct narrowing on the gate.
3 tasks
wenshao
added a commit
that referenced
this pull request
May 17, 2026
Follows up #3598 (cli/core feature shipped to main, no docs). **User doc** `docs/users/features/structured-output.md` — covers quick-start, schema input forms (inline + `@path`), output shapes per `--output-format`, parse-time restrictions, retry/failure modes, privacy redaction, permission gating, MCP shadow-tool handling, and a worked `jq`-piped pipeline example. Registered under the existing `features/_meta.ts` so it shows up in the docs sidebar between "Headless Mode" and "Dual Output". **Design doc** `docs/design/structured-output/structured-output.md` — why the synthetic-tool-whose-param-schema-is-the-user-schema approach, the four-stage parse-time validation pipeline, `schemaRootAcceptsObject`'s decided-vs-deferred boundaries, main-turn vs drain-turn parity via `processToolCallBatch`, the structured- success terminal block, the cross-surface privacy redaction sharing `STRUCTURED_OUTPUT_REDACTED_ARGS`, subagent context handling (`forSubAgent`), MCP shadow-tool guard, the compatibility surface, alternatives considered (and why rejected), and a file-by-file index. Both docs are English-only — repo convention is English-only for both `docs/users/features/` (zero zh-CN siblings) and `docs/design/` (only `customize-banner-area/` has a zh-CN twin). Open to adding zh-CN translations as a separate PR if there's demand.
wenshao
added a commit
that referenced
this pull request
May 17, 2026
* docs: add user + design docs for --json-schema structured output Follows up #3598 (cli/core feature shipped to main, no docs). **User doc** `docs/users/features/structured-output.md` — covers quick-start, schema input forms (inline + `@path`), output shapes per `--output-format`, parse-time restrictions, retry/failure modes, privacy redaction, permission gating, MCP shadow-tool handling, and a worked `jq`-piped pipeline example. Registered under the existing `features/_meta.ts` so it shows up in the docs sidebar between "Headless Mode" and "Dual Output". **Design doc** `docs/design/structured-output/structured-output.md` — why the synthetic-tool-whose-param-schema-is-the-user-schema approach, the four-stage parse-time validation pipeline, `schemaRootAcceptsObject`'s decided-vs-deferred boundaries, main-turn vs drain-turn parity via `processToolCallBatch`, the structured- success terminal block, the cross-surface privacy redaction sharing `STRUCTURED_OUTPUT_REDACTED_ARGS`, subagent context handling (`forSubAgent`), MCP shadow-tool guard, the compatibility surface, alternatives considered (and why rejected), and a file-by-file index. Both docs are English-only — repo convention is English-only for both `docs/users/features/` (zero zh-CN siblings) and `docs/design/` (only `customize-banner-area/` has a zh-CN twin). Open to adding zh-CN translations as a separate PR if there's demand. * docs(structured-output): address PR review feedback User doc: - explicit stdout-vs-stderr contract and `{}`-schema behavior. - 500 ms shutdown-holdback latency note. - ReDoS warning for user-supplied `pattern` keywords. - root `$ref` rejection + `allOf` workaround. - per-retry token cost note. - sibling-suppression success vs retry paths split out. - numeric exit codes (1 / 53 / 130) for every failure mode. - new "Session resumption" section for --continue / --resume. Design doc: - gloss the ToolSearch on-demand-loading reference. - `not` row: drop the array-indexing-lookalike `[…]`. - 500 ms holdback is best-effort, not guaranteed. - redaction rationale extends to validation-failure retries. - `CORE_TOOLS` phrasing: structured_output is excluded FROM the set; skill is in a separate dynamically-discovered category. - subagent suppression maintainer note (single brittle call path). - `--bare` parenthetical lists the three retained core tools. - PR #4001 status (closed 2026-05-11, superseded). * docs(structured-output): correct empty-schema / holdback / SIGINT claims Three doc claims were stronger than the actual code behaviour: - **Empty schema produces `{}`, not `null`.** `turn.ts` normalises the tool args via `(fnCall.args || {})` before they land in `structuredSubmission`, so a zero-arg call against `{}` is emitted as `{}` on stdout. The `?? null` in the adapter is defence-in-depth for the strictly-undefined case, which the upstream path doesn't produce. - **Holdback is a cap, not a fixed wait.** The loop guard is `Date.now() < deadline && registry.hasUnfinalizedTasks()`, so it exits immediately when nothing is in flight. Reword as "capped at ~500 ms" with an early-exit note. - **SIGINT can still flush a captured result.** The holdback loop does not poll the abort signal, so a SIGINT after the structured call is captured but before `adapter.emitResult` finishes may still land on stdout. Treat exit code 130 as the source of truth. Also addresses the new auto-review summary suggestion about per-turn schema cost: pull the cost callout up out of the bullet list (so it covers both retry cost and schema-embedded-every-turn cost), since the schema-embedding cost isn't retry-specific. * docs(structured-output): correct stdout/stderr + json-mode envelope claims Two doc claims didn't match `JsonOutputAdapter.emitResult`: - **Model prose doesn't go to stderr in text mode.** Only error messages and log lines do. Successful runs emit just the JSON-stringified payload on stdout; accumulated assistant prose is discarded entirely (not mirrored to stderr). Point users at `--output-format json` / `stream-json` when they need the prose. - **`--output-format json` emits a JSON array, not a single document with top-level fields.** The adapter calls `JSON.stringify(this.messages)` where `messages` is an array of message objects. `structured_result` lives on the final `type: "result"` element of that array, not at the document root, so consumers must read `.[-1].structured_result` rather than `.structured_result`. * docs(structured-output): note schema-itself reaches the provider The Privacy section so far only described `structured_output` *args* being redacted from local on-device surfaces (telemetry + chat recording). The schema body is a separate exposure surface — it ships as the function declaration's `parameters` block on every model request, so `enum`, `const`, `default`, `examples`, `description`, `$comment`, etc. travel to the provider in cleartext. Users defaulting to "redaction covers everything" could legitimately leak secrets via schema-literal fields. Add a callout in the user doc, plus a parallel paragraph in the design doc explaining why the redaction stops at on-device surfaces (the model needs the schema to satisfy the tool-call contract, so provider-side redaction isn't possible). * docs(structured-output): correct stdout-on-failure / ReDoS example / hooks / --bare deny / typo Five issues from the latest /qreview pass: - **stdout-vs-stderr is text-mode only.** In `--output-format json` and `stream-json`, the failure result message is emitted on stdout (final element of the JSON array, or the terminating `result` line on the JSONL stream). Wrappers in those modes must switch on `is_error`, not on whether stdout is empty. - **ReDoS example didn't actually demonstrate the threat.** JSON Schema `pattern` only fires on string instances, and tool args are always objects, so the bare `{"pattern": "(a+)+b"}` schema doesn't constrain anything the model can supply. Move the pattern inside a string-typed property. - **Hooks see raw `tool_input`.** `PreToolUse` / `PostToolUse` / `PostToolUseFailure` receive the unredacted args — including HTTP hooks that can forward off-device. Call this out explicitly so users with audit-style catch-all hooks know to filter or add hook-side redaction. - **`--bare` drops settings-level deny.** Bare mode builds `mergedDeny` as `[...(bareMode ? [] : settings.permissions.deny), …]` — settings-level denies are skipped while the synthetic tool stays registered. Argv-level `--exclude-tools` still applies. Document this exception in the user doc and the design doc. - **`maxSessionTurns` hint typo.** The hint points at "schema is unsatisfiable" — the original text inverted the polarity. * feat(core): PR-2.5 — post-promote stream redirect + natural-exit registry settle Closes the two limitations PR-2 (#3894) deferred for the Phase D part (b) Ctrl+B promote flow (#3831): 1. **Post-promote stream redirect**: today the `bg_xxx.output` file is frozen at promote time because `ShellExecutionService` detaches its data listener as part of PR-1's ownership-transfer contract. PR-2.5 wires a caller-side `onPostPromoteData` callback so bytes from the still-running child append to the file via an `fs.createWriteStream` opened in `handlePromotedForeground`. 2. **Natural-exit registry settle**: today the registry entry stays `'running'` until `task_stop` / session-end `abortAll` fires its abort listener. PR-2.5 wires `onPostPromoteSettle` so natural child exit transitions the entry to `'completed'` / `'failed'` with the right exitCode / signal / error message. ## Service (`shellExecutionService.ts`) - New exported types: `ShellExecuteOptions`, `ShellPostPromoteHandlers`, `ShellPostPromoteSettleInfo`. - `execute()` options bag now accepts `postPromote?: { onData, onSettle }`. Threaded through to both `executeWithPty` and `childProcessFallback`. - PTY's `performBackgroundPromote` (line ~1159): after disposing the foreground data + exit + error listeners, RE-ATTACH minimal forwarders that call `postPromote.onData` / `postPromote.onSettle` when the caller opted in. Backwards compat: when `postPromote` is unset the PR-2 detach-everything contract is preserved (the re-attach is gated on each callback being defined). - `childProcessFallback`'s `performBackgroundPromote` (line ~706): same pattern — re-attach `stdout.on('data', ...)`, `stderr.on('data', ...)`, `child.once('exit', ...)`, `child.once('error', ...)` when the caller opted in. `error` listener routes through `onSettle` with `error` populated, so spawn-side errors after the foreground errorHandler detached don't crash the daemon via the default unhandled `'error'` event. - Both paths wrap caller callbacks in try/catch so a thrown handler doesn't crash the child's data loop / unhandled-rejection the service. ## Shell tool (`shell.ts`) - New `PromoteArtifacts` type — slots shared between the foreground `execute()` postPromote handlers (which fire on the service side as soon as promote happens) and the post-resolve `handlePromotedForeground` finalizer (which runs after `await resultPromise` returns). The two race; the buffer + settle-queue absorb that race so neither chunks nor the eventual exit info are lost. - `executeForeground` wires `postPromote` handlers that route data to either `promoteArtifacts.stream` (if open) or `promoteArtifacts.buffer` (drained when the stream opens), and queue settle info if the wired handler isn't yet installed. - `handlePromotedForeground` opens `fs.createWriteStream(outputPath, { flags: 'w' })`, writes the initial snapshot first, drains the buffer, then registers the entry and wires `onSettleWired` with the full registry decision table: - `error` set → `registry.fail(shellId, error.message, endTime)` - `exitCode === 0` → `registry.complete(shellId, 0, endTime)` - non-zero exitCode → `registry.fail(shellId, "Exited with code N", endTime)` - signal !== null → `registry.fail(shellId, "Terminated by signal N", endTime)` - all-null fallback → `registry.fail(shellId, "Exited with unknown status", endTime)` - Fires queued settle synchronously after wiring so a fast command that exits between promote and finalizer doesn't get lost. - Self-audit catch: closes the output stream on the `registry.register` throw path so the FD doesn't leak past the orphan-child kill. ## Tests - 3 new in `shellExecutionService.test.ts`: - `post-promote bytes route to postPromote.onData when callback provided` - `postPromote.onSettle fires on natural child exit after promote` - `backwards compat: without postPromote, listeners stay fully detached` - 3 new in `shell.test.ts` under a `foreground → background promote PR-2.5` describe block: - `post-promote bytes APPEND to bg_xxx.output via write stream` - `natural child exit transitions registry entry to "completed"` - `non-zero exit / signal / error → "failed" with descriptive message` - Bulk-replaced 50 prior `{},` (empty 6th-arg shellExecutionConfig) with `expect.objectContaining({}),` + added `expect.objectContaining({ postPromote: expect.any(Object) }),` as the 7th-arg expectation for the foreground execute call. - Updated the existing `registers a bg_xxx entry on result.promoted` test to assert on `fs.createWriteStream` + `stream.write` instead of the now-removed `fs.writeFileSync` snapshot path. 182/182 shell.test.ts pass + 73/73 shellExecutionService.test.ts pass + 111/111 coreToolScheduler.test.ts pass + 60/60 AppContainer.test.tsx pass; tsc + ESLint clean. Self-audit: 3 rounds (positive / reverse / cross-file) found one issue — output stream FD leak on `registry.register` throw — and fixed it before flagging complete. All flagged edge cases (stream errors, child-exits-before-wire-up race, task_stop during natural- exit window, promote-never-happens cleanup, backwards compat without callbacks) have explicit handling and / or test pinning. * fix(core): #4102 review wave — 3 Critical + UTF-8 + tests 3 Critical race/correctness issues + 1 multibyte-corruption suggestion + 3 test coverage gaps addressed: **Critical 1 — child_process late-chunk drop (service)** Settle was fired on 'exit', but stdout/stderr can emit buffered data between 'exit' and 'close'. Late chunks landed in `promoteArtifacts.buffer` after shell.ts had already closed the stream + transitioned the registry → silently dropped → truncated `bg_xxx.output`. Switched to listening on 'close' which guarantees all stdio is fully drained. (code, signal) payload is identical to 'exit', just with proper ordering. **Critical 2 — stream-flush wait before registry transition (shell)** `stream.end()` is asynchronous; pending writes can still be in the libuv queue when it returns. The old code transitioned the registry immediately after `.end()`, so a /tasks consumer could observe a `completed` entry and read the output file BEFORE the trailing bytes were on disk. Fixed: wired settle now `stream.once('finish', ...)` BEFORE calling `registry.complete/fail`. `error` event also short-circuits to the transition so a late ENOSPC doesn't hang the settle path forever. **Critical 3 — stream-open-fail buffer leak (shell)** If `fs.createWriteStream` threw, the catch path set `stream = null` but the foreground `onData` handler would still take the `stream === null` branch and push chunks into `promoteArtifacts.buffer` — unbounded growth under a sustained child whose output file couldn't be opened. Added a `streamFailed: boolean` latch on `PromoteArtifacts`. When set, `onData` drops chunks (with a debug log) instead of buffering. The catch branch sets the latch. **Suggestion — shared TextDecoder corrupts multibyte UTF-8 (service)** child_process post-promote used ONE TextDecoder for both stdout AND stderr. The decoder's continuation-byte state machine assumes one byte source; interleaved multibyte chunks corrupted. Now uses separate decoders + flushes both with `decode()` (no `stream: true`) on settle so trailing bytes surface as their final characters. **Suggestion — llmContent reflects already-settled status (shell)** When the queued-settle drain transitions the registry synchronously (fast-exit race), the model-facing copy was still saying "Status: running. … task_stop({...})". Updated to branch on `postPromoteAlreadySettled` / `postPromoteFinalStatus` — when the process is already gone, the copy says "Status: completed/failed" and replaces the `task_stop` suggestion with "Process has already exited; no `task_stop` needed". **Suggestion — test coverage gaps** Added: (a) `queued-settle race: onSettle BEFORE handlePromotedForeground completes` — custom service impl fires onSettle synchronously before resolving the promote promise, pins the drain path. (b) child_process post-promote tests for stdout/stderr forwarding + 'close'-not-'exit' settle + spawn-error settle. **Self-audit**: Round 1 + reverse audit. Stream.once mock added to fire 'finish' synchronously so existing tests don't hang on the new flush wait. 76/76 shellExecutionService.test.ts (+3) + 183/183 shell.test.ts (+1) pass; tsc + ESLint clean. * fix(core): #4102 review wave-2 — 3 more from gpt-5.5 C1 (shell.ts:2227): the WriteStream `'error'` event handler only logged. `fs.createWriteStream` reports common open failures (ENOENT / EACCES / ENOSPC) asynchronously via that event rather than throwing. Result: `promoteArtifacts.stream` kept pointing at the failed stream; `onSettleWired` attached a `.once('finish')` listener that would never fire → registry stuck on `running` forever. Latch the failure (null the shared `stream` slot, set `streamFailed`); `onSettleWired`'s existing `if (!stream)` branch then transitions the registry immediately. C2 (shellExecutionService.ts:1468): the promote handoff removes the foreground `ptyErrorHandler` and only re-attaches data + exit listeners. A subsequent PTY `error` event had no listener — Node treats an unhandled `error` from an EventEmitter as a fatal exception that takes the whole CLI down. Attach a post-promote forwarder that ignores expected PTY read-exit codes (EIO / EAGAIN, same filter the foreground handler uses) and routes unexpected errors through `postPromote.onSettle` with `error` populated. Single-fire latch shared with `onExit` so settle never fires twice. C3 (shell.ts:2503): `onSettleWired` waits for the stream's asynchronous `'finish'` event before flipping `postPromoteAlreadySettled`, but the model-facing `statusLine` was built immediately after invoking `onSettleWired` on the queued settle. A fast-exited promoted command could therefore land "Status: running" + a `task_stop` instruction in production even though settle was already observed. Split into two flags: `postPromoteSettleObserved` (set synchronously when settle is classified) drives the model copy; the registry transition stays behind the stream flush. Tests: +1 PR-2.5 wave-2 PTY error-routing test; +2 shell.ts tests (stream open async error → registry still transitions; async `'finish'` after queued-settle drain → llmContent says 'completed' before registry transition fires). * fix(core): #4102 review wave-3 — 4 actionable from deepseek-v4-pro T2 (shell.ts:2456) — Critical buffer-leak race `onSettleWired` previously set `promoteArtifacts.stream = null` BEFORE calling `stream.end()`. Any `postPromote.onData` chunk that landed between that null assignment and the actual flush completing saw `stream === null && streamFailed === false` and pushed into `promoteArtifacts.buffer` — a buffer that has no further drain path (the foreground finalizer has already returned). Result: chunks stranded indefinitely; PTY mode in particular hits this because `onExit` can fire while kernel buffers still hold data. Fix drains the pre-settle buffer to the stream BEFORE nulling AND latches `streamFailed = true` so any subsequent chunk drops via the existing `else if (streamFailed)` arm in `onData` instead of leaking. Updates the `streamFailed` doc to cover both setters (open-fail and settle-done) so the dual semantic is explicit. T3 (shell.ts:2262) — silent chunk-drop in catch path When `fs.createWriteStream` throws synchronously (rare: ENOENT on a vanished tmpdir), chunks already in `promoteArtifacts.buffer` were silently lost with no observability — oncall reading a truncated `bg_xxx.output` had no way to distinguish "stream open failed" from "child produced nothing." Logs the dropped chunk count and empties the buffer. T5 (shell.ts:2443) — opaque all-null fallback The "Exited with unknown status" fallback fired the registry to 'failed' without any context about which fields were null. This branch is meant to be unreachable; hitting it indicates the service emitted a defective settle info object. Includes the field values in both the fail message and a warn log so the oncall engineer can tell this path apart from the other "failed" branches. T6 (shellExecutionService.ts:1452) — leaked PTY post-promote listeners `ptyProcess.onData(...)` returns an `IDisposable` that was being discarded; same for `onExit`. The `'error'` listener function was also not captured (no way to `removeListener` it). EventEmitter holds refs to listener closures, which transitively hold refs to `onPostData` / `onPostSettle` / the caller's `promoteArtifacts`. While bounded by the PTY's lifetime, the closures keep the caller's state pinned for the post-settle delay window. Captures all three handles into `postPromoteDataDisposable` / `postPromoteExitDisposable` / `postPromoteErrorListener`, then releases them via a shared `disposePostPromoteListeners()` call from `firePostSettle` (idempotent — each slot null-checked and nulled after disposal). Tests: +1 service test for IDisposable + error-listener cleanup; +2 shell.ts tests for buffer drain race and catch-path snapshot fallback. Existing tests stay green (262 → 265 in the touched suites; 7819 → 7822 across the core package). * fix(core/test): drop unused 'registry' in wave-3 T2 test (TS6133) CI build failed across all platforms with src/tools/shell.test.ts(4395,15): error TS6133. The variable was a leftover from copying the queued-settle test pattern; the wave-3 T2 test inspects writeStreamMock.write call history directly and never reads the registry, so the assignment is dead code. Drop it. * fix(core): #4102 review wave-4 — 6 actionable from gpt-5.5 + deepseek-v4-pro T1 (Critical, shellExecutionService.ts:860 child_process onSettle exactly-once) The PTY path used a `firePostSettle` latch but child_process wired `close` and `error` independently to `onPostSettle`. A spawn-side error followed by Node's auto-emitted `'close'` would call the caller's settle TWICE, racing the registry transition. Added the same single-fire latch on the child_process path. T2 (Critical, shell.ts:2264 handoff race reorder) Original order was `write(snapshot) -> drain buffer -> assign stream`. Synchronous today (no race in current code), but assign-after-drain leaves a hazard for any future refactor that adds an `await` inside the drain loop — a chunk arriving in that window would land in `promoteArtifacts.buffer`, then post-assign chunks would write to the stream first, producing out-of-order bytes until the settle drain. Reordered to `write(snapshot) -> assign stream -> drain buffer`, which closes the hazard regardless of future async additions. T3 (Suggestion, shellExecutionService.ts:816 decoder flush gated on onSettle) The trailing-multibyte flush ran inside the `child.once('close', ...)` handler, which was only installed when `onSettle` was set. An `onData`-only caller (no onSettle) lost trailing continuation bytes silently. Hoisted flush into `flushPostPromoteDecoders` called from `firePostSettle`, and made `firePostSettle` available on the `'close'` path independent of onSettle (T6 install). T4 (Suggestion, shell.ts:1700 promoted ANSI passthrough) The regular `executeBackground` path strips ANSI before writing to `bg_xxx.output`; the promoted-foreground onData path appended raw chunks. Reading `bg_xxx.output` after Ctrl+B showed plain text up to the snapshot then raw `\x1b[31m` / cursor-move / clear-screen sequences for the post-promote tail — unreadable. Apply `stripAnsi(rawChunk)` before write/buffer, matching the executeBackground contract. T5 (Suggestion, shellExecutionService.ts:786 UTF-8 hardcoded) The post-promote child_process decoders were hard-coded to `new TextDecoder('utf-8')`, but the foreground decoder runs encoding detection via `getCachedEncodingForBuffer`. On a non-UTF-8 child (e.g. GBK on a Chinese Windows shell), the snapshot decoded correctly but the post-promote tail was mojibake. Capture the foreground decoder's `.encoding` property and reuse it for post-promote (with utf-8 fallback if foreground hadn't seen any bytes yet, and a try/catch around `new TextDecoder` for the rare unsupported-encoding case). T6 (Suggestion, shellExecutionService.ts:1540 `error` listener gated on onSettle) The post-promote `error` listener was attached only when `onSettle` was set. An `onData`-only caller still had the foreground errorHandler detached; a post-promote spawn error would then crash the CLI via Node's unhandled-error default. Hoisted the close + error listeners into `if (postPromote)` so any caller opting into post-promote gets crash protection; if `onSettle` is absent the listeners log + drop instead of routing. T7 (Suggestion, shellExecutionService.ts:791 onSettle-only pipe-block deadlock) Same root cause as T6: when only `onSettle` is set, the foreground `stdout`/`stderr` 'data' listeners are detached and no post-promote listener replaces them. The Readables stay paused, the OS pipe buffer fills (~64KB on Linux), the child blocks on `stdout.write`, 'close' never fires, onSettle never fires. Added `child.stdout?.resume()` and `child.stderr?.resume()` in the no-onData branch so the child can drain its pipes and reach exit. T8 (Suggestion, shell.ts:2614 dead inspectLine ternary) `inspectLine`'s ternary returned the same string on both sides — copy-paste leftover from when the other two adjacent ternaries (statusLine / stopLine) were correctly varied. Collapsed to a single string assignment. Tests: +5 regression tests (4 child_process: T1 double-fire latch, T3 onData-only flush, T6 onData-only error survives, T7 onSettle- only resume; +1 shell.ts: T4 ANSI strip). 265 -> 270 in the touched suites; 7822 -> 7827 across the core package; full suite green. * fix(core/test): use ShellOutputEvent type in wave-4 onData callbacks (TS2345) CI lint failed on the wave-4 (T3 / T6) tests with TS2345: pushing ShellOutputEvent into Array<{type:string;chunk:unknown}> narrows incompatibly. Switch to ShellOutputEvent[] (matches earlier helpers at lines 758/966) and discriminate the union via .type === 'data' when reading .chunk so the narrowed multibyte assertion still type-checks. * docs(structured-output): address doudouOUC's four review findings - Tighten JSON/stream-json paragraph: not all failures emit a result to stdout (exit 53 / exit 130 are stderr-only); check exit code first - Fix suppressed-sibling retry guidance: re-issue in a separate turn that does not include structured_output (avoids re-suppression) - Distinguish settings-deny (exit 53) from --exclude-tools (exit 1) in Permission gating section - Replace <projectDir> placeholder with actual path ~/.qwen/projects/<sanitized-cwd>/chats/<sessionId>.jsonl in both docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(structured-output): fix Permission gating — both deny paths strip registration Forward audit against source code found that the Permission gating section incorrectly distinguished settings.permissions.deny (claiming tool stays visible, exit 53) from --exclude-tools (claiming declaration stripped, exit 1). Both go through the same mergedDeny → isToolEnabled path and both prevent registration — the model never sees the tool. Corrected both docs to reflect the actual mechanism: typical outcome is plain text (exit 1), with maxSessionTurns (exit 53) as the fallback if the model loops through other tools. * docs(structured-output): address doudouOUC's May 17 review (5 items) - Clarify validation is client-side Ajv, not provider-side - Qualify "same way" with DeclarativeTool abstraction parenthetical - Match symptom→cause structure for maxSessionTurns hint - Expand $ref workaround with concrete $defs example - Clarify Dual Output See Also doesn't require --json-schema * docs(structured-output): address 2 unresolved design-doc suggestions 1. Privacy/redaction section: note hooks as intentionally non-redacted surface (matches user-doc "Hooks see raw args" callout). 2. Dual call-site section: clarify differing post-helper termination flow between main-turn (direct return) and drain-turn (sentinel hop). * docs(structured-output): address doudouOUC's May 17 review (2 nits) 1. Failure-paths table: align "three common causes" cell with the symptom→cause framing already used at parse-time validation pipeline section ("common stuck-run symptom and its two likely causes"). 2. Dual call-site section: fix factual inaccuracy from prior commit — `drainOneItem` is `async (): Promise<void>` and returns nothing. The two-hop termination is via closure-mutated `structuredSubmission` (set by `processToolCallBatch`, checked by `drainLocalQueue` and the holdback loop), not a return-value sentinel. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--json-schema '<json>'/--json-schema @path/to/schema.jsonflag in headless mode (qwen -p). Registers a syntheticstructured_outputtool whose parameter schema is the user-supplied JSON Schema — the model must call it to deliver the final result.result(for--output-format textpiping) and a structuredstructured_resultobject (for--output-format json/stream-json).SchemaValidator.compileStrict) — in contrast to the lenient runtimeSchemaValidator.validatewhich silently skips unsupported schemas for MCP compat. Without this, a malformed--json-schemawould no-op and quietly accept any payload.Behavior
structured_output, the run fails with a clear error and non-zero exit code.--json-schemais rejected at argument parsing when combined with-i/--prompt-interactiveor with--input-format stream-json— structured output only makes sense in non-interactive, single-shot flows. A prompt may still be supplied via stdin pipe (cat prompt.txt | qwen --json-schema '...'); genuinely empty input falls through to the existing "No input provided via stdin..." runtime error.BaseJsonOutputAdapterresult envelope.Example
```bash
qwen -p "Summarize PR #123" --output-format json --json-schema '{
"type": "object",
"properties": {
"summary": {"type": "string"},
"risk": {"type": "string", "enum": ["low", "medium", "high"]}
},
"required": ["summary", "risk"]
}'
```
Returns a result message with
structured_result: {summary: "...", risk: "low"}.Test plan
SyntheticOutputTool(schema passthrough, valid/invalid args, execute termination message) — 6 testsresolveJsonSchemaArg(inline JSON,@file, empty/invalid JSON, non-object, bad schema, missing file, draft-2020-12) — 10 testsSchemaValidator.compileStrict(valid schema, draft-2020-12, empty object, invalid type, non-object input) — 5 testsBaseJsonOutputAdapter.buildResultMessagestructured-result emission (present vs absent) — 2 testsManual smoke tests
Run end-to-end via the bundled CLI (
node dist/cli.js) against the bundled providers in the user's~/.qwen/settings.json. Coverage spans both function-calling code paths the synthetic tool flows through:dashscope.aliyuncs.com/compatible-mode/v1(Bailian)open.bigmodel.cn/api/anthropic(Z.AI),api.kimi.com/coding/(Moonshot)Happy path — model called
structured_outputwith the expected fields, session terminated on first call, exit 0:--output-format text→ stdout is the JSON-stringified payload on a single line.--output-format json→ result envelope contains bothresult(string) andstructured_result(object); init event listsstructured_outputintools[].--output-format stream-json→ terminatingresultevent carriesstructured_result.--json-schema @path/to/schema.jsonresolved and applied.Cross-model reliability — same prompt ("rate this PR description"), same schema, neutral wording (no explicit "MUST call tool" directive):
glm-5glm-5.1kimi-for-codingglm-4.7qwen3.6-max-previewqwen3.6-plusdeepseek-v4-proqwen3.5-flashWhere models did pass, the result envelope was identical across both paths:
tools[]containedstructured_outputin the init event, and the terminating result carried both the JSON-stringifiedresultand the parsedstructured_result. So the synthetic-tool wiring is provider-agnostic; what varies is whether the model decides to call it. Smaller / faster models (qwen3.5-flash) need an explicit "you MUST call the structured_output tool" directive to be dependable — this is the per-provider compatibility caveat the original Behavior section warns about, observed in practice.Failure path — exit 1 with the documented error:
subtype: "error_during_execution",is_error: true,error.message: "Model produced plain text instead of calling the structured_output tool as required by --json-schema."Hit organically withqwen3.5-flashon neutral prompts; the failure-path wiring is provider-agnostic.Parse-time validation — exit 52 (
FatalConfigError), no model call:--json-schema cannot be empty.--json-schema is not valid JSON: ...[1,2,3]) →--json-schema must be a JSON object describing a schema.type:"string"→--json-schema root must accept object-typed values ...@file→--json-schema could not read "...": ENOENT ...{"type":"object","properties":{"x":{"type":"banana"}}}) →compileStrictreportsdata/properties/x/type must be equal to one of the allowed values ....Yargs-level rejection — exit 1, no model call:
--json-schema+-i/--prompt-interactiverejected.--json-schema+--input-format stream-jsonrejected.--json-schemawith prompt piped via stdin (cat prompt.txt | qwen --json-schema '...') accepted; truly empty input still fails at the runtime "No input provided via stdin" check.Not exercised in this run (no credentials in test env):
api.openai.com).generativelanguage.googleapis.com).Schema-shape compatibility across providers stays a known caveat — the synthetic tool's parameter schema is forwarded as-is to whatever function-calling API the active provider exposes, and exotic JSON Schema features may be rejected upstream.