Repository navigation
fix(core): keep nested tool_call arguments open on Responses; name the target in bridged validation errors - #12901
Conversation
…t schema
The tool_call bridge declaration deliberately types arguments as a bare
object so the model-facing schema stays byte-stable across catalog
changes, which makes an empty {} envelope-valid even when the deferred
target requires fields. resolveDeferredToolCall validated only that
envelope and returned the arguments verbatim, so the target's
required-field error surfaced post-unwrap in the scheduler as a bare
Ajv message (params must have required property 'url') with no target
name or remedy; models resubmitted the same empty object until the
validation-retry loop guard stopped the turn
(LOOP_DETECTED/invalid_tool_params_stagnation).
Pre-validate the bridged arguments with the target's own
validateToolParams before resolving. A failure now returns an
INVALID_TOOL_PARAMS bridge refusal naming the target and the missing
field, which the scheduler's existing bridge-refusal accounting already
counts toward the retry-loop threshold. The pre-check runs on a
structuredClone because SchemaValidator coerces values in place; the
scheduler re-validates the returned arguments at build time, so valid
calls are unaffected.
Fixes #12889
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-issue-patrol/jmukuf6832h
…heck resolveDeferredToolCall now pre-validates bridged arguments against the target schema (#12889), but omni media-policy targets do not hold their final arguments at that point: both frontends run evaluateMediaPolicyToolCall AFTER bridge resolution (coreToolScheduler before buildInvocation, ACP Session.runTool), and that gate is what resolves resourceId to inputPath and merges defaultArguments/lockedArguments, while BaseMediaPolicyTool's validateToolParams deliberately checks the NATIVE schema rather than the model-visible projection tool_search returned. So the pre-check refused calls the very next stage accepted. A bridged {resourceId, outputDir} call — the shape omni/media-guidance.ts tells the model to send — was refused with "provide exactly one of inputPath ... or resourceId" even though resourceId was provided, and for path-less media the model can never learn the real path, so it cannot repair the call. An operator-pinned modelAccess.lockedArguments.outputDir was unwinnable both ways: projectMediaPolicyToolDeclaration strips the locked key from the model-visible properties AND required, so omitting it failed the pre-check while sending it failed the gate. Each refusal fed recordBatchRetryableToolError and gained RETRY_LOOP_STOP_DIRECTIVE, ending the turn in LOOP_DETECTED / invalid_tool_params_stagnation on a call shape that previously worked. All 14 omniPolicyToolFactories tools inherit the first trigger; the 13 with a native required outputDir also reach the second. Key the exemption off mediaPolicyDescriptor: the code-level fact the gate itself keys off (it passes every non-policy tool through untouched), so the exemption covers exactly the targets whose arguments a downstream stage completes. Nothing fails open — the gate still emits named invalid_params refusals before build(), and build() re-validates the merged arguments against the native schema. Acceptance test added to the existing #12889 block: a target whose model-visible schema omits a key its validateToolParams requires (native required: ['inputPath','outputDir'], projected required: []) resolves with projection-satisfying arguments instead of being refused. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmul3pj9gc4
…policy gates Round-2 review follow-up on the #12889 pre-validation: - Yield the pre-check when the request is stamped wasOutputTruncated, so a max_tokens-cut bridged envelope surfaces as truncation (the scheduler's existing truncation guards own the message) instead of a schema mismatch. - Consult the caller's per-target execution policy (scheduler execution allowlist / ACP permission-manager enablement) before validating arguments, so a denied target keeps its specific EXECUTION_DENIED. - Media-policy targets now pre-check against the model-visible projection (target.schema) instead of skipping validation: a missing model-visible required field is refused naming the target, while gate-completed arguments (resourceId handles, operator-locked keys) stay unrefusable at the bridge. - Plumb the validated targetName through bridgeResolutionError and key per-tool retry accounting on it (widening the batch-start prune to match), so alternating failures against distinct targets accrue per-target and reach the retry-loop threshold. - Coalesce AgentTool.refreshSubagents kicks onto the in-flight refresh — validation now fires twice per bridged Agent call — while the change listener re-arms one follow-up so a mid-refresh change is not dropped. - Read mediaPolicyDescriptor typed instead of through a structural cast. - Tests pin: the resourceId trigger shape, a projector-derived projection fixture, the throwing-validator fall-through, and scheduler coverage for the truncation / policy-denial / per-target-accounting orderings. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-issue-patrol/jmulbkgfd2o
|
Round-2 findings addressed in R2-1 [Critical] — truncation must win over the schema pre-check. Fixed as suggested: R2-2 — resourceId trigger shape. Added: locked-outputDir media-policy target called with R2-3 — hand-written projection literal. The test fixture now derives its R2-4 — exemption skipped validation entirely. Narrowed: media-policy targets are pre-checked against the model-visible projection ( R2-5 — structural cast. Dropped; R2-6 — policy denial must precede the pre-check. Added R2-7 — R2-8 — per-target retry accounting. R2-9 — untested swallow branch. Added: a target whose Verification: |
The Lint & Static job failed at the "Run Prettier" step on packages/core/src/core/coreToolScheduler.test.ts and packages/core/src/tools/tool-call.ts. Reformat both with the repository's pinned prettier (3.6.1); no semantic change. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-conflict/jmulf520ncq
…ounting, and agent refresh Round-3 review follow-ups for the tool_call bridge argument pre-check: - Scheduler: forward the permission-manager policy as a pre-check suppression so a pm-denied bridged target surfaces the loop's richer EXECUTION_DENIED (deny-rule attribution) instead of an INVALID_TOOL_PARAMS refusal for a call that could never run (R3-3). - Scheduler: drop the post-resolution isToolExecutionAllowed gate — resolution already consults the same constructor-fixed predicate on the same target name, so the copy was unreachable and free to drift (R3-2). - Scheduler: widen the retry-counter presence set only for INVALID_TOOL_PARAMS bridge refusals (the one error type that accrues), so an EXECUTION_DENIED batch no longer preserves stale counters (R3-4), and record bridged pre-check refusals in a channel-marked namespace so they no longer prefix-prune (or get pruned by) the same target's direct validation failures every mixed batch (R3-5). - tool-call: narrow the pre-check to the target's model-visible schema (SchemaValidator) for all targets, so value-level rules (fs stats, content scans, the AgentTool refresh kick) run exactly once at build() time instead of twice per bridged call (R3-12). - AgentTool: route the unknown-subagent_type validation kick through the listener's arm-once path so a kick landing mid-scan queues one follow-up instead of being dropped (R3-6), guard the armed follow-up against post-dispose execution (R3-8), and catch the armed chain per this file's void-boundary contract (R3-9). Tests: pin the ACP policy-denial path (R3-1), the listener mid-refresh arm (R3-7), the policy-gate/hidden-gate ordering (R3-10), and each behavior fix above with its own mutation-checked case. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-issue-patrol/jmuluuwer2v
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
E2E verification for #12889
Result: the original user-visible failure still reproduces at 4c7ebf961cc6126e0e26c8139a365b238c645753. The new bridge validation runs and improves the error message, but the model repeats the same empty arguments and the ACP turn still ends under tool-call loop protection.
Setup
- Built and ran the PR base
fc4e01b9fc381952c15592ead9e0cf170754dd8d, then checked out and built PR head4c7ebf961cc6126e0e26c8139a365b238c645753in the same local Git worktree. - Used a fresh ACP session for each run, an isolated Qwen configuration and runtime, Direct mode (
tools.codeModeOnly: false), and the same configuredgpt-6-astramodel through the OpenAI Responses-compatible provider. The private endpoint and credentials are omitted. - Sent the original prompt:
帮我找一下 opus-5.5 代理集群解决 图遍历算法的新闻. Captured ACP events and inspected the provider logs for the model's raw tool-call arguments. One matched base/head pair was run; a second head run confirmed the same final failure.
Observed behavior
| Base | PR head | |
|---|---|---|
| Model calls | tool_search(select:web_fetch), then three tool_call calls with {"name":"web_fetch","arguments":{}} |
Same sequence and arguments |
| Tool errors | Three target-tool errors: params must have required property 'url' |
Three bridge refusals naming web_fetch, the missing url, and tool_search as the way to inspect the schema |
| ACP turn | Tool-call loop protection stopped this turn |
Same result; the requested news was not retrieved |
The PR branch returned this error for each empty call:
[tool_call bridge refused] Deferred tool "web_fetch" rejected the arguments: params must have required property 'url'. Pass arguments matching the schema returned by tool_search for "web_fetch".
Both revisions built successfully. On the PR head, packages/core/src/tools/tool-call.test.ts passed 44/44 tests. A separate direct call to resolveDeferredToolCall confirmed that the base accepts {} and fails later in the target's build(), while the PR head rejects {} at bridge resolution and still accepts complete arguments.
Conclusion
The PR fixes the bridge's validation and error attribution, but it does not resolve #12889's reproduced end-to-end failure for this model/provider combination. The model still sends {} after receiving the richer error. I would keep the user-visible recovery criterion open until an ACP E2E run with the original prompt completes successfully, or narrow the PR's stated scope to bridge validation and diagnostics.
This observation covers the specified model/provider combination and prompt; it does not establish behavior for other models or providers.
中文说明
在同一 worktree 中依次构建并运行 base fc4e01b9 与 PR head 4c7ebf9，使用隔离配置、相同的 gpt-6-astra + OpenAI Responses 兼容 provider、ACP 路径和 issue 原提示词。两版均先搜索到 web_fetch，随后连续三次调用 tool_call，arguments 都是 {}，最终由循环保护结束任务。PR head 确实把缺少 url 的错误提前到桥接层，并补上目标工具名与修正提示；模型仍未恢复，新闻检索未完成。PR head 的定向测试为 44/44 通过。建议保留端到端恢复的验收条件，或将 PR 的目标明确收窄为参数预校验和错误诊断。
…p failure Two review findings on the bridged tool_call argument pre-validation: - tool-call: validate a per-call structural copy of the target's parametersJsonSchema. Ajv caches a compiled schema by object identity for the life of the process, and AgentTool mutates its own parameterSchema in place, so the first bridged call pinned every later one to the pre-refresh shape and refused a property the target had since advertised. The copy resolves through the JSON-text tier, which still shares one compiled validator per distinct schema text. - coreToolScheduler: log the permission-manager lookup failure that the suppressArgumentPreCheck guard swallows. On the refusal path _schedule continues ahead of the permission gate, so the catch was the only place that saw the error and it recorded nothing. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmumcpwj7eg
TS4111 rejects dot access on a Record<string, unknown> index signature. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmumcpwj7eg
Picks up #13000 (5ef7983), which accepts the takeover scan's settlement in the durable-local fault gate. Without it, Hosted process fault gates fails with: DurableLocalRuntimeFaultGateTest.brokerCrashAdoptsOriginalWorkerWithoutReplaying:41 timed out waiting for original journal settlement; last value: ALREADY_SETTLED introduced by #12964 (2f5a62e) reconciling executions on session takeover. This branch does not touch packages/sdk-java/runtime-broker, so the merge takes main's version wholesale. Co-authored-by: Qwen-Coder <[email protected]>
…wn label The armed follow-up chain attached its .catch AFTER .finally. Because .finally propagates the original rejection and its callback is synchronous, that handler could only ever receive the IN-FLIGHT refresh's rejection while its message said 'Follow-up subagent refresh failed'. runRefreshSubagents' finally awaits llmClient.setTools(), which can reject, so a transient setTools failure was reported as a failed follow-up scan even when the follow-up succeeded. The follow-up's own rejection is already reported by requestRefresh's 'Subagent refresh failed' handler. Move the .catch before the .finally so each rejection keeps its own label; the rejection stays handled (no unhandledRejection) and the arm still runs. Tests: extend 'absorbs a setTools rejection from an armed follow-up refresh' to assert both labels, and add 'reports an in-flight refresh rejection under the in-flight label only' (setTools rejects once, then resolves) pinning that the follow-up label is never logged for the in-flight error. Both go red if the chain is reordered back. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmumpkv7rf3
…amily The channel marker keeps a bridged pre-check refusal from prefix-pruning the same target's direct validation failure WITHIN a batch, but the batch-start presence prune widened the presence set only for requests carrying an INVALID_TOOL_PARAMS bridge refusal. Each batch therefore preserved just the channel it contained and deleted the other, so a model alternating between bridging a deferred target and calling it directly reset both counters every turn: neither ever reached VALIDATION_RETRY_LOOP_THRESHOLD and RETRY LOOP DETECTED was never injected. That is a regression against the merge base, where both channels produced the same message under the same key and accrued together. Fix both halves, since widening presence alone is not safe: - presence: a request naming X now preserves both `X` and `bridgeRetryToolName(X)`, so alternating channels across batches accrue toward one threshold. - clearing: `clearRetryCountsForTool` now also deletes the bridge-marked keys. A resolved bridge renames the request to the target, so the widened presence set keeps its stale bridge count alive across the successful execution; without the clearing half that resurrects the premature RETRY LOOP DETECTED the marker's prune was relied on to drop. Unrelated tools are unaffected: the widening is keyed on names present in the current batch, and an EXECUTION_DENIED refusal still keeps the wrapper name, so it cannot retain the denied target's counters. Tests: add 'accrues alternating bridged and direct failures of one target across separate batches' (six single-request batches; the directive fires once, on the fifth) and 'clears a target's bridge-marked counter when a bridged call to it succeeds'; extend the existing mixed-channel test to also assert the directive on the bridged half, which its name already claimed. Dropping the presence widening turns the first red; reverting clearRetryCountsForTool to the bare prefix turns the second red. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmumpkv7rf3
ACP wired the permission manager into isTargetExecutionAllowed, but that
option is the outer owner's execution-allowlist hook — only agent-core
supplies one, and this frontend reads neither an execution allowlist nor a
disallowedTools blocklist. So with `permissions.deny: ["web_fetch"]` driven
through ACP, resolution short-circuited with the bridge's scheduler-flavoured
wording ("execution allowlist or disallowedTools blocklist"), pointing the
operator at two knobs that cannot be the cause and never naming the deny rule,
while the identical configuration on the terminal frontend prints
`Matching deny rule: "permissions.deny: web_fetch"`.
Mirror the scheduler's split instead: the permission manager goes into
suppressArgumentPreCheck, so a pm-denied target skips the argument pre-check
(no INVALID_TOOL_PARAMS strike for a call that could never run) and the L1
enablement gate below stays ACP's only policy authority — carrying ACP's own
denial wording and its isTrustedLiveTool exemption, exactly as a direct call
to the same target already does.
The load-bearing invariants are unchanged: the call is still denied before
execution, still EXECUTION_DENIED, the target is still never built, and no
invalid-parameter strike is recorded.
Test: retarget `keeps EXECUTION_DENIED for a policy-denied bridge target
instead of a parameter pre-check refusal` from the bridge wording to ACP's own
`Tool "web_fetch" is disabled.`, and assert the bridge wording is gone.
Reverting the wiring turns it red.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmumpkv7rf3
|
Scope check at 2059aea: keep this PR limited to bridge argument pre-validation and correct error attribution. Two remaining correctness issues need closure: a rejected permission lookup can escape the pre-check, and the Agent fork instructions require name even when the advertised schema omits it. The Agent refresh-coalescing changes are independent of the current schema-only pre-check; I will assess removing that expansion rather than grow it further. This does not claim to fix the separate model retry behavior in #12889. No full local suite or remote CI polling in this pass. |
The new `keeps a %s target policy failure within its ACP call` cases stub
`getPermissionManager` with only `isToolEnabled`. The independent sibling
call in the same batch still reaches `evaluatePermissionRules`, which calls
`pm.hasRelevantRules()` whenever the tool's own default permission is not
`deny` -- and the sibling declares `getDefaultPermission: 'allow'`. The
missing method threw, so the sibling's functionResponse carried
`pm.hasRelevantRules is not a function` instead of its output and the
containment assertion (`response: { output: 'sibling completed' }`) failed
for both `denied` and `unavailable`.
No PM rules are configured in these cases, so `hasRelevantRules` returning
false is the faithful stub: the sibling is then governed by its own
`getDefaultPermission`, which is what the test already asserts. The
assertions themselves are untouched -- this only makes the fixture
callable. Also applies Prettier formatting to the new block.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmun0ao3qfm
Bring the branch up to date with main (16 commits, base was a631573) so the CI lint-gate freshness check and the required checks run against a current base. Clean merge, no conflicts. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmun0ao3qfm
|
Local UI verification on Linux: PASS for the bridged validation error, N/A for the Responses converter
Scenario — the #12889 path, reached the way a model reaches it: review the schema through the bridge, then hand the target bad arguments and do not repair them.
Both arms first produced Before After
Read against #12889: the unlabelled
|
Resolve the single conflict in packages/core/src/core/coreToolScheduler.ts as a union of two orthogonal generalizations of the same base expression: main generalized which guidance string is attached (hadIncompleteArguments -> INCOMPLETE_ARGS_PARAM_GUIDANCE, #12970), this branch generalized which message body it is attached to (describeBridgedArgumentError when the call arrived through the tool_call bridge). Each side keeps its own behavior. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmur8h2ce0s
|
@qwen-code /triage Re-triage after a base merge. Head is now Why the standing
The merge contains no new production logic. Its only hand-written part is the conflict resolution in Local verification on the merged head: |
Stale: this review targets an ancestor of the current head; its actionable findings were addressed in later pushes (remaining open threads, if any, are Suggestion-level and deferred).
main's 1a5aae8 (#13167, run Managed session tools in a Runtime worker) and this branch both edited the runTool catch block. Keep both behaviours: main's ManagedRuntimeOutcomeUnknownError re-throw runs first so the typed error still reaches the outer catch, then this branch's bridged tool_call argument labelling wraps everything else. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-conflict/jmus7hcqa2c
|
Closed the remaining local feedback in 67ea7b62ab3f without expanding production scope.
Validation: frozen-lockfile install and full prepare build, full typecheck, 622 core tests + 3 ACP bridge cases, changed-test lint/format/diff checks passed. Independent test-engineer observation confirmed the mixed retry regression; test-value review found it minimal and non-duplicative. Commit hooks preserved the tested test-file bytes, and all production files are unchanged by this repair. The original #12889 prompt still requires the reporter's gpt-6-astra/OpenAI Responses service configuration, which is unavailable locally. These schema/error/retry controls do not establish that provider's model recovery, so #12889 remains open. The owner checkout failure is already repaired on main in #13306; independent native Git verification covers that repair without claiming a historical job rerun. |
|
@tanzhenxin Your review offered two ways forward — keep the user-visible recovery criterion open until an ACP E2E run with the original prompt passes, or narrow the PR's stated scope to bridge validation and diagnostics. The PR now does both, so re-requesting your eyes on the current head The scope is narrowed to what was actually verified. The description claims only the Responses-converter change and the target-named validation errors, states the decoding-constraint hypothesis as unconfirmed ("under default JSON Schema rules, an empty The recovery criterion stays open. #12889 is still open, and the closeout note records that the reporter's Your observation isn't disputed, and nothing since it claims to change the outcome: between If the narrowed scope reads right to you, could you refresh the request-changes? Non-blocking either way — CI is green at this head (23 checks pass / 0 fail) and the branch is |
|
@qwen-code /triage |
|
Maintainer-local verification, round 9: ✅ Follow-up to the sandboxed round 8 (same head). What this round adds: a real-environment E2E A/B on macOS (real built CLI over real loopback HTTP against a scripted fake OpenAI Responses server, replaying the #12889 scenario, headless + real TUI), closure of round 8's open ACP-mutation gap (4/4 mutants killed), F-5 disproven by measurement, macOS gate datapoints, and a trial merge against a 中文摘要结论： Previous-round findings (round 8 → now). Head unchanged; all four PR-touched production files re-hashed identical to round 8's citations, so its harness findings carry forward on an identical input closure: F-2 fixed (carried), F-3 fixed (carried + statically re-verified), F-4 stands (pre-existing walk boundary, identical on base), C4 stands (re-measured on the real wire: no Real-environment E2E A/B (recorded wire bytes; 14/14 assertions per arm, 28 total):
Real TUI (tmux-driven, same fake server, same prompt) — base renders the bare #12889 symptom; head renders the labelled error:
ACP mutation matrix (round 8's cut rows, run to completion;
Trial merge into current Not covered: the reporter-provider #12889 acceptance run (the premise that the reporter's backend constrains decoding remains unconfirmed — the PR body says the same and defers closing #12889); round 8's 47-schema census (carried on a byte-identical converter input); per-commit attribution (37 commits, aggregate diff verified); Windows/Linux local runs. The Harness scripts and raw logs ( — Qwen Code · maintainer-local verification (round 9) |
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only scan at head 67ea7b62ab3f6cfc06497f041a8d34546a7eba73 — complete.
Verdict: COMMENT. I found no Critical in this diff, and both Criticals I filed on this PR in earlier rounds are now resolved. This is not an approval for one reason only: a maintainer's CHANGES_REQUESTED is still live and its stated acceptance criterion is, by the author's own account at this head, unmet.
My two prior Criticals are resolved — by deletion, which is the strongest form
My review at 0ab0c146 filed R6-1 (the bridge pre-check's structuredClone + flipped additionalProperties could claim a schema $id in the module-level Ajv singleton, making the target's later build-time validation throw schema with key or id already exists and hit SchemaValidator.validate's documented fail-open return null, silently disabling validation for the process) and R4-1 (the pre-check enforced the model-visible schema layer on every target, so it could refuse a bridged call the target's own validateToolParams would have normalized and accepted).
Both are gone at this head because the pre-check itself is gone. packages/core/src/tools/tool-call.ts is now +14/-0: a single pure addition of describeBridgedArgumentError, a string formatter. No structuredClone, no SchemaValidator.validate call, no additionalProperties flip survives anywhere in this PR's diff, so neither the shared-Ajv-state chain nor the stricter-than-target entrance has any code left to run through. The PR body records the removed mechanism on archive branches, and the description now states the bridge pre-check was dropped.
R4-1's direction is also closed positively rather than merely vacated: the replacement only relabels a rejection the target's own build() already raised, so it cannot refuse a call the target would accept. That is the property the finding asked for.
Current diff — no Critical
I read all five production files at this head and checked the four places a defect could plausibly hide:
- The retry budget is still keyed on the raw error, not the relabelled one.
recordBatchRetryableToolError(reqInfo.name, invocationOrError.message)atcoreToolScheduler.ts:3400uses the target's raw message and raw name, while onlydisplayErrorandfinalErrorcarrytargetMessage. Had the count keyed ontargetMessage, a bridged and a direct call to the same target would accrue into separate buckets and alternating calls would never trip loop protection — the exact regression the newshares validation retries across bridged and direct callstest pins withexpect(message.includes('RETRY LOOP DETECTED')).toBe(index === 2)across a bridge/direct/bridge sequence. The body reports this test was mutation-checked against keying by the labelled message. - The discriminator has a negative control. The relabel is gated on
reqInfo.modelFacingName !== undefinedin the scheduler, andleaves a direct call's validation error unlabelledassertsnot.toContain('Deferred tool')for the same missing-urlfailure sent directly. Both directions of the gate are pinned rather than just the new one. - The ACP relabel cannot misattribute an execution failure as an argument rejection.
Session.ts:16044gates onbridgedThroughToolCall && !toolBuildSucceeded.toolBuildSucceededis not new to this PR — it is declared at:14035, set at:14049, and already consumed at:16105and:16112(recordInvalidToolParams: !toolBuildSucceeded) for precisely this build-versus-execution distinction, so the new use reuses an established flag rather than introducing a second notion of it. - The converter change preserves every stated exception.
isRootis threaded only from the public entry (normalizeResponsesParameterspassestrue); array-item and property recursion default tofalse, so nested objects are the only ones marked. TheadditionalProperties: truewrite sits inside the existingproperties === undefinedpatch branch and is guarded byout['additionalProperties'] === undefined, so an explicitadditionalProperties: falsesurvives and the root keeps the bare patch a zero-argument tool needs. The tests pin root-versus-nested acrossproperties, arrayitems,anyOf,oneOfandallOf.
The added describeBridgedArgumentError export in packages/core/src/index.ts is consumed by the ACP session in packages/cli, so the cross-package import is the reason for the barrel entry rather than a speculative widening.
The one reason this is not an approval
@tanzhenxin's CHANGES_REQUESTED (submitted 2026-09-29 against 4c7ebf96) is still present in the review collection and reviewDecision still reports CHANGES_REQUESTED. It is not a code-defect finding but an E2E acceptance result: the original #12889 user-visible failure reproduced at that head, and the stated condition for clearing it is "an ACP E2E run with the original prompt completes successfully, or narrow the PR's stated scope to bridge validation and diagnostics."
At the current head the PR body still lists "Acceptance: rerun the #12889 E2E on the reporter's Responses provider with the original prompt" as an open test-plan item and states plainly: "This verifies schema/error/retry boundaries locally; the original #12889 provider E2E has not been run." The Tested-on table repeats that. So the first half of the maintainer's condition is unmet and admitted unmet.
Two things I want to record rather than adjudicate:
- That reproduction predates the
additionalProperties: trueconverter change, which is this PR's actual root-cause hypothesis for the empty-argumentsloop. Its observation table may therefore no longer hold at this head. But the body itself grades the decoding-constraint premise "unconfirmed", and no rerun exists, so recovery cannot be established by reading code — and an unconfirmed historical blocker is treated as blocking under this channel's rules. - @wenshao approved at this head. The approval carries an empty body, so there is no scope disclosure to borrow coverage from, and it does not clear
reviewDecisionwhile the earlier request stands.
All 37 inline review threads read; zero unresolved. Every thread I checked was verified against the head source rather than accepted on its resolved flag — the two threads whose replies say "not taking this one this round" and "escalating as human-gated" both concerned the pre-check that has since been deleted outright, which supersedes the dispute rather than leaving it open.
CI
Healthy at this head — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, both Desktop Shell legs and review-pr all pass, with no failing check.
Next step
Nothing here asks for a code change; on the code this is ready. To clear the merge gate, either run the #12889 E2E on the reporter's Responses provider with the original prompt and post the outcome, or ask @tanzhenxin to withdraw or convert the change request now that the scope is narrowed to converter behaviour plus error attribution and the decoding premise is explicitly marked unconfirmed.
Changed files span packages/core/src/core/, packages/core/src/tools/ and packages/cli/src/acp-integration/, so require_code_owner_review applies. This review is submitted as qqqys.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope. Source only — responses-converter.ts (schema patching logic + isRoot propagation), tool-call.ts (describeBridgedArgumentError, exports), coreToolScheduler.ts (labelled-message assembly, retry counter keying), Session.ts (bridged-build-failure labelling), and the four test files. Not reviewed: E2E on the original #12889 provider (explicitly out of scope); Windows/Linux runtime behaviour (PR flags both
Prior-round ledger. Round 1 (0ab0c146, DISMISSED) found two blockers in the argument pre-check ($id shared-registry pollution, stricter-than-target validation). Both are resolved: the pre-check is entirely removed at this head. No surviving round-1 finding.
Findings
No blocking findings.
What was checked
normalizeResponsesSchemaNode recursive call structure — confirmed from source that all recursive call sites (array-items branch, per-property iteration at nextProps[k], items field, and combinator-list processing) omit the isRoot parameter, defaulting to false. Only the entry point normalizeResponsesSchemaNode(schema, true) passes true. Result: exactly the top-level parameter object is exempted from the additionalProperties: true patch; every nested {type:"object"} node added by the Azure compat pass gets the flag unless additionalProperties is already specified. Tests for the explicit-false and explicit-typed-schema cases confirm the guard is respected.
describeBridgedArgumentError labelling scope — the condition bridgedThroughToolCall && !toolBuildSucceeded in Session.ts correctly limits labelling to calls where the target's own build() threw. Execute failures (toolBuildSucceeded = true) pass through the original error unchanged. The ACP it.each parameterised test covers the success / build-error / execute-error paths and verifies this distinction explicitly.
Retry counter budget sharing — confirmed by code comment ("Counts accumulate per (tool, error message) pair") and by the new shares validation retries across bridged and direct calls test: the counter is keyed on reqInfo.name + invocationOrError.message (the raw validation error before wrapping), so bridged and direct calls to the same target with identical invalid arguments consume the same budget. targetMessage is used only for the displayed error, not the counter key; the labelled-message mutation in the test verifies the production source uses the raw key.
Public API extension — describeBridgedArgumentError is exported from packages/core/src/index.ts. This matches the existing pattern for resolveDeferredToolCall and DEFERRED_TOOL_CALL_* prefixes, and is needed by the CLI package's Session.ts. Additive, no breaking change.
Cross-check against existing reviews
tanzhenxinCHANGES_REQUESTED (4c7ebf961c): E2E withgpt-6-astrastill reproduced the loop at that head. The current head explicitly narrows scope to bridge validation and diagnostics, acknowledges the provider E2E as unverified, and keeps#12889open. The scope narrowing is whattanzhenxinoffered as the alternative path forward.qwen-code-dev-bot/chiga0round-1 criticals (0ab0c146): both concerned the now-removed pre-check; resolved by deletion.qqqys(0ab0c146): same two criticals, same resolution.doudouOUC(a674e3a6, "one residual"): the comment was filed against an intermediate head; the subsequent commit log showscompileIsolatedand the$id-related fixes were applied, and the current head removes the pre-check entirely.wenshao(67ea7b62, maintainer local): 2436 assertions pass, real-environment E2E A/B on macOS, merge-ready verdict.
No blocking findings. Approval blockers: none.
Reviewed with AI assistance.
8 participants
What this PR does
Two changes for #12889:
propertiesmap to a nested object schema that has none (an Azure compatibility patch), it now also marks that objectadditionalProperties: trueunless the schema states otherwise. This keepstool_call's free-formargumentsexplicitly open. The root of a tool schema keeps the bare patch, since a zero-argument tool's top level really is empty.tool_callfails its own validation, the error names the target and says where to read its schema, for exampleDeferred tool "web_fetch" (called through tool_call) rejected the arguments: params must have required property 'url'. Pass arguments matching the schema returned by tool_search for "web_fetch".This applies to both the terminal scheduler and the ACP session. A direct call's error is unchanged.No bridge pre-check (removed, kept at
archive/12901-bridge-arg-precheck). No auto-reveal of the target after an empty call (removed, kept atarchive/12901-empty-arg-auto-reveal; see the scope decision).Why it's needed
In #12889 the model received a deferred web tool's schema through an OpenAI Responses-compatible provider, then kept calling
tool_callwith{"arguments":{}}until loop protection stopped the turn. A clearer error alone did not recover that run. On the Responses wire only, the nestedargumentsobject went out as{type: "object", properties: {}}. A backend that constrains decoding to the schema may read that as "empty object only". The Chat Completions and Anthropic converters have no such patch. Whether the reporter's backend actually constrained decoding this way is unconfirmed: under default JSON Schema rules, an emptypropertiesmap alone does not close an object.The labelled error makes a genuine argument mistake attributable to the target instead of to
tool_call's envelope.Reviewer Test Plan
How to verify
{type: "object"}withoutproperties: the nested node carriesproperties: {}andadditionalProperties: true; an explicitadditionalPropertiesis kept; the root node is unchanged.{}to a target requiringurl:INVALID_TOOL_PARAMS, answered undertool_call, message names the target, the missing field andtool_search. The same arguments sent directly give the original message.INVALID_TOOL_PARAMS, and the third reportsRETRY LOOP DETECTEDdespite the bridge-specific message prefix.tool_callschema allows empty arguments for tools with required fields #12889 E2E on the reporter's Responses provider with the original prompt.Evidence (Before & After)
At
67ea7b62ab3f6cfc06497f041a8d34546a7eba73, frozen-lockfile installation and its full prepare build, full typecheck, changed-test lint/format and622targeted core tests plus the three ACP bridge outcomes passed. The mixed bridge/direct/bridge regression detects a mutation that keys retries by the labelled display message instead of the raw target validation error; the production source was restored and is byte-identical to the starting head. Independent observation confirms the three calls retain one retry budget. N/A for UI. This verifies schema/error/retry boundaries locally; the original #12889 provider E2E has not been run.Remaining-feedback report.
Tested on
Risk & Scope
task_create.metadataandtask_update.metadataas well astool_call.arguments. That matches their JSON Schema meaning, but a strict backend could treat the explicit flag differently. The labelled error only touches an error that was already returned.Linked Issues
Refs #12889 (close after the E2E passes). The schema-contract consolidation stays in #12999.
中文说明
改动
针对 #12889 的两处改动：
properties的嵌套 object schema 补一个空的properties（兼容 Azure 的补丁）。现在补的同时会标上additionalProperties: true（schema 自己写了的除外），让tool_call的自由形式arguments明确保持开放。工具 schema 的根节点仍按原样补丁，因为零参数工具的顶层本来就是空的。tool_call调用的延迟工具，如果被它自己的校验拒绝，报错会写明目标工具以及去哪看它的 schema，例如Deferred tool "web_fetch" (called through tool_call) rejected the arguments: params must have required property 'url'. Pass arguments matching the schema returned by tool_search for "web_fetch".。终端调度器和 ACP 会话都适用，直接调用的报错不变。不再有桥接预校验（已删除，保存在
archive/12901-bridge-arg-precheck）。也不再有空参调用后自动亮出目标工具（已删除，保存在archive/12901-empty-arg-auto-reveal，见范围决定）。原因
在 #12889 中，模型通过 OpenAI Responses 兼容 provider 拿到了一个延迟网页工具的 schema，然后一直用
{"arguments":{}}调用tool_call，直到被循环保护终止。只把报错写清楚并没有让那次实跑恢复。只有在 Responses 接口上，嵌套的arguments会被发成{type: "object", properties: {}}，按 schema 约束解码的后端可能把它理解为"只能是空对象"。Chat Completions 和 Anthropic 的转换器没有这个补丁。报告者的后端是否真的这样约束解码尚未确认：按 JSON Schema 默认规则，仅有空的properties并不会让对象变成封闭的。带名字的报错让真实的参数错误能归到目标工具上，而不是被误认为是
tool_call外壳的问题。评审测试计划
如何验证
{type: "object"}、没写properties的工具，构建 Responses 请求时：嵌套节点带有properties: {}和additionalProperties: true；显式写了的additionalProperties保持不变；根节点不变。url的目标用{}发起桥接调用：返回INVALID_TOOL_PARAMS，以tool_call名义回复，报错写明目标、缺失字段和tool_search。同样的参数直接调用时报错保持原样。INVALID_TOOL_PARAMS，第三次报告RETRY LOOP DETECTED，桥接消息增加的目标前缀不应重置预算。tool_callschema allows empty arguments for tools with required fields #12889 报告者的 Responses provider 上用原始提示词重新做端到端测试。证据（前后对比）
在
67ea7b62ab3f6cfc06497f041a8d34546a7eba73上，冻结 lockfile 安装及 prepare 完整构建、完整 typecheck、修改测试 lint／格式、622项 core 定向测试与 ACP 桥接的三种结果均通过。新增桥接／直接／桥接回归能够检出“以带标签的展示消息代替目标原始校验错误作为重试键”的变异；生产源码已恢复，逐字节与起始 head 相同。独立观察确认三次调用共享一个重试预算。无 UI 变化。本地验证覆盖 schema／报错／重试边界，#12889 原提供商的端到端测试仍未执行。剩余反馈处理报告。
测试平台
风险与范围
tool_call.arguments外，还包括task_create.metadata和task_update.metadata。这与它们在 JSON Schema 中的含义一致，但严格的后端可能会对这个显式标记有不同处理。带名字的报错只改动了一个本来就会返回的报错。关联 Issue
Refs #12889（端到端测试通过后关闭）。schema 契约的整合仍在 #12999。