Repository navigation
Commit 1abccdb
fix(core): stop misdiagnosing malformed tool-call args as max_tokens truncation (#12982)
* fix(core): stop misdiagnosing malformed tool-call args as max_tokens truncation
When a provider streams a fused/malformed tool-call argument bag, the
streaming parser flags the JSON as incomplete (brace depth > 0) and the
OpenAI converter unconditionally rewrites finish_reason to "length",
which turn.ts maps to wasOutputTruncated and the scheduler then appends
the max_tokens truncation note to the schema-validation error. The note
is asserted with no evidence: neither the provider's real finish_reason
nor usageMetadata is consulted, so a response that ended at 185
completion tokens is reported as token-limit truncation and the model
retries the identical call.
Guard the finish_reason override with the usage evidence: the pipeline
records the wire output budget (max_tokens or a provider-specific budget
key) on the request context, and the converter skips the override when
reported completion tokens fall well below that budget. Genuine
truncation (completion tokens at the cap, or usage/ceiling unavailable)
keeps the legacy inference, preserving the #4964 recovery path, and an
explicit provider-reported "length" is still trusted verbatim.
Refs #12970
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-issue-patrol/jmum5kp9p2z
* style(core): apply prettier formatting
* fix(core): settle the truncation override where delayed usage lands
Two defects in the #12970 corroboration guard, both reported on the review
thread at head 8b7848a:
R1-1 — the guard read `chunk.usage` off the chunk carrying `finish_reason`,
but this pipeline requests `stream_options.include_usage` (pipeline.ts:1079)
and under that convention the finish chunk reports no usage: the totals arrive
on a later `choices: []` chunk that `handleChunkMerging` folds into the parked
finish response. For that whole provider class the verdict was always
"inconclusive", the `stop` -> `length` rewrite fired exactly as before, and the
misdiagnosis this PR exists to remove still shipped. The converter now parks
the provider's own finish reason on the request context, and the pipeline
settles the rewrite on the parked response — before every one of its three
delivery points (post-merge yield, Stage 2d flush, error-path flush), so the
consumer only ever observes the settled reason. The park is reset when a
stream starts, because a retry can reuse the same context object.
R1-2 — only `undefined` counted as "no evidence", so a zero-filled usage object
(ModelScope sends exactly that on the finish chunk) or an explicit `null` fell
through to the ratio comparison and was read as proof *against* truncation.
That suppressed the #4964 override and, since `wasOutputTruncated` is the sole
key for the scheduler's reject-file-writes-while-truncated guard, disarmed it
on the responses it exists for — a truncated `write_file` repaired by jsonrepair
could then write a partial file. A missing, non-numeric or non-positive count
is now inconclusive rather than a disproof; it cannot mean "no output" here,
because this branch is only reached once the parser found incomplete JSON.
The corroboration helper becomes an exported three-state verdict so the
converter (chunk usage) and the pipeline (merged usageMetadata) share one
threshold and one notion of "no evidence".
Tests: 6 new converter cases — zero-filled usage, null completion_tokens, the
50% threshold on both sides, and park / no-park. 252/252 pass.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmumj5dw0er
* test(core): pin the wire-budget capture and the parked-override settlement
Co-authored-by: Qwen-Coder <[email protected]>
R1-7: nothing proved the pipeline *sets* RequestContext.maxOutputTokens — all
the corroboration tests injected it themselves through the converter harness,
and getWireOutputBudget has exactly one caller. Three cases now assert the
context handed to the converter carries the budget as max_tokens, as either
provider key (max_completion_tokens and max_new_tokens, since the clamp path
treats both as budget keys), or as undefined when the request caps by neither.
R1-1: the reference stream shape — argument chunks, a finish_reason chunk with
no usage, then a trailing choices:[] chunk carrying the totals — now has an
end-to-end case asserting the yielded finish response reads STOP, plus the two
invariants that keep it honest: trailing usage at the cap still yields
MAX_TOKENS (#4964), and no trailing usage at all still yields MAX_TOKENS
rather than clearing the inference on a guess. The converter module is stubbed
wholesale in this file, so the mock mirrors the parking contract that
converter.test.ts pins on the real implementation, and the mock factory now
re-exports the real verdict helper the pipeline settles with.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmumj5dw0er
* test(core): access the wire max_tokens key via index signature
tsc --noEmit flags TS4111 on dot access of an index-signature property; the
captured wire request is typed Record<string, unknown>.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmumj5dw0er
* fix(core): keep the incomplete-file-write guard when the max_tokens diagnosis is withdrawn
`wasOutputTruncated` is derived solely from `finishReason === MAX_TOKENS`
(turn.ts), and it feeds two consumers with different needs:
- the user-visible note appended to a validation failure, and the max_tokens
recovery loop — both of which must NOT fire when the response's own usage
disproves a token-limit cut (#12970);
- the scheduler's rejection of file-modifying calls, whose real precondition is
that the arguments arrived incomplete, whatever cut them off.
Suppressing the `stop` -> `length` override on a disproof fixed the first and
silently disarmed the second. A model emitting `write_file` and stopping
partway through `content` at low usage — the same malformed-generation class
#12970 reports, on a `Kind.Edit` tool — kept `finish_reason: 'stop'`, so
`getCompletedToolCalls()` repaired the unterminated string and brace into
schema-valid arguments and the call executed, overwriting the target with half
its intended content. The merge base rejected it ahead of `buildInvocation`.
Carry the fact separately from the diagnosis: a non-enumerable symbol on the
`FunctionCall` (the `PROVIDER_TOOL_CALL_ID` precedent in `toolCallIdUtils.ts`),
set at both points where the token-limit diagnosis is withdrawn — the
converter's immediate `disproved` branch and the pipeline's delayed settle —
surfaced as `hadIncompleteArguments`, and honoured by the guard alongside
`wasOutputTruncated`. The guard now keys on what it always should have.
The refusal wording follows the cause, which is the second half of #12970's
expected behavior 1: a malformed-generation rejection says so and asks for one
tool call per turn with schema-valid parameters, rather than blaming max_tokens
and advising the model to split content it never ran out of room for. Its error
type is `invalid_tool_params`, matching the issue report, not `output_truncated`.
Also log both rewrite directions with the two numbers they were decided from.
The heuristic has now been wrong in both directions once (#4964 missed a real
cut, #12970 invented one), so the next field report should not have to
re-derive the wire budget from a request capture.
Regression pins: the guard still rejects on `hadIncompleteArguments` alone,
with the cause-matched wording and error type; both marking points are asserted
on the paths production actually takes. All three were mutation-checked —
reverting the guard condition makes the write execute (`expected 'success' to
be 'error'`), and removing either marking point fails its own assertion.
Co-authored-by: Qwen-Coder <[email protected]>
* test(core): keep the real truncation verdict in the omni-cache converter mock
`pipeline.omniCacheInvalidation.test.ts` stubbed `./converter.js` with a plain
factory, so `corroborateTruncationFromCompletionTokens` was absent from the
module namespace. It passes today only because that suite's converter stub never
parks an override, so `settleParkedTruncationOverride` returns at `if (!parked)`
before touching the missing export. The first parked-override case added there
would fail with a mock-plumbing error naming the mock rather than the missing
wiring, which reads as a converter bug.
Keep the pure verdict real, as `pipeline.test.ts` already does.
Co-authored-by: Qwen-Coder <[email protected]>
* test(core): drive the real converter and the real pipeline through one stream
The park/settle handshake has two ends that each own half of one contract: the
converter suspects a rewrite and parks the provider's own reason, the pipeline
settles it once the delayed usage totals land. `converter.test.ts` drives the
real converter against a hand-written expectation of what the pipeline will do;
`pipeline.test.ts` drives the real pipeline against a stubbed converter that
hand-writes the park. Either side can stay green while the seam between them
breaks — which is how the R1-1 guard shipped inert.
These two cases run both ends for real, on the reference protocol's stream shape
rather than the convenient one: under `stream_options.include_usage` the chunk
carrying `finish_reason` reports no usage, and the totals arrive on a later
`choices: []` chunk. They pin both outcomes of the same fused `write_file`
stream — 185 of 8192 withdraws the max_tokens diagnosis while keeping the
incomplete-write guard armed, 8192 of 8192 keeps the override so the #4964
recovery still fires.
Mutation-checked: removing the settle-path marking fails the disproof case and
leaves the corroboration case green.
Co-authored-by: Qwen-Coder <[email protected]>
* fix(core,cli): carry the incomplete-arguments marker to every consumer
R2-1: normalizeModelToolCallIds rebuilds each FunctionCall with an object
spread, which copies enumerable own properties only, so the non-enumerable
incomplete-arguments marker was dropped at the llm-chat hop and never reached
turn.ts. Both scheduler consumers of hadIncompleteArguments were therefore
unreachable. Re-attach it the same way PROVIDER_TOOL_CALL_ID already is.
R3-1: the subagent runtime builds its own ToolCallRequestInfo and derived
wasOutputTruncated solely from finishReason === MAX_TOKENS, so the data-loss
guard stayed disarmed for every subagent even with the marker preserved.
Mirror turn.ts and read the marker there too.
R3-2: the hosted-workspace tool turn refused incomplete-argument calls on
wasOutputTruncated alone and executes them itself rather than through
CoreToolScheduler. That profile commits to a remote workspace with no undo
backup, so it has to consult the new field as well.
R3-6: drop the pendingTruncationOverride reset at the top of
processStreamWithLogging. No RequestContext can carry a stale override into it
- the streaming executor returns a lazy generator, so an executeAttempt retry
can only be entered before the generator body runs - and the comment above it
described a retry model the code does not have.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuo2vk3phn
* test(core): pin the incomplete-args marker producers and fix the parked-override contract
Round-4 review triage (#12970):
- turn.test.ts: pin the main-session producer of hadIncompleteArguments —
a marked call under a STOP finish must surface the flag on the emitted
ToolCallRequest (with wasOutputTruncated untouched), and an unmarked
control call must surface without it. Deleting the producer spread or
making it unconditional now reddens the suite.
- agent-core.test.ts: extend the subagent marker test with an unmarked
sibling call so over-application of the marker is detectable.
- coreToolScheduler.test.ts: pin INCOMPLETE_ARGS_PARAM_GUIDANCE with a
non-Edit tool failing schema validation — the branch the Edit-guard
tests can never reach — asserting the guidance names malformed
generation and not a max_tokens cut.
- types.ts: the stream-start clear of pendingTruncationOverride was
deliberately removed earlier in this PR; restate the field contract to
the guarantee the code actually provides (lazy generator + fresh
RequestContext per attempt make a stale park unreachable).
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuodld0708
* style(core): let Prettier join the wrapped assertion in turn.test.ts
The previous commit left one expectation in turn.test.ts wrapped narrower
than Prettier 3.6.1 wants it, so Lint & Static failed at its Run Prettier
step. Reformatted with the pinned Prettier; formatting only, no behaviour
change.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmuod8i2ti5
* fix(core): use an incomplete-args retry loop directive for pre-validation Edit rejections
The incomplete-args arm of the Edit guard rejects before buildInvocation,
so schema validation never runs on those calls. Attaching the validation
retry-loop directive there misdiagnosed the cause ('failed validation ...
re-examine the tool schema') and would drive the wrong recovery. Add a
third directive for repeated incomplete file writes, keep the directive
appended after the rejectionMessage counter key is recorded, and cover
the branch with a threshold test that goes red if the arm falls back to
either existing directive.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuofqisz0a
* test(core): rebuild the truncation witnesses on the compressed helpers
main's #13007 test-suite compression reshaped two helpers these
witnesses depend on, and this branch was merged from a pre-compression
main, so `tsc --build` broke in the CI prepare step:
src/core/coreToolScheduler.test.ts(7207,7): error TS2554: Expected 1 arguments, but got 2.
src/core/coreToolScheduler.test.ts(7266,7): error TS2554: Expected 1 arguments, but got 2.
src/core/coreToolScheduler.test.ts(7391,7): error TS2554: Expected 1 arguments, but got 2.
src/core/turn.test.ts(951,27): error TS2304: Cannot find name 'GenerateContentResponse'.
(and three more GenerateContentResponse sites in turn.test.ts)
`createTruncationTestScheduler` now takes only the tool and derives
`getAllToolNames` from `tool.name`, which equals the explicit name array
each call site passed, so dropping the second argument preserves the
registry shape the witness asserts against. `turn.test.ts` kept casting
stream chunks to `GenerateContentResponse` after the compression dropped
that type-only import; restore it alongside the other `@google/genai`
types.
`tsc --build` on packages/core is now clean.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmup4109ajg
* test(core): pin the per-call marker quantifier and the truncated errorType arm
R5-1: the truncated arm of the scheduler's errorType ternary had no test
naming OUTPUT_TRUNCATED, so collapsing the ternary to INVALID_TOOL_PARAMS
left the suite green. The shared truncation-rejection helper now asserts
errorType, and the collapse mutant turns the truncation cases red.
R5-2: the main-session producer test carried a single-call response, so a
response-wide read of the incomplete-arguments marker was indistinguishable
from the per-call read. The positive case now carries a marked call plus a
clean sibling and asserts both directions; hoisting the read turns it red.
Co-authored-by: Qwen-Coder <[email protected]>
---------
Co-authored-by: yiliang114 <[email protected]>
Co-authored-by: Qwen-Coder <[email protected]>
Co-authored-by: yiliang114 <[email protected]>1 parent 623cfc6 commit 1abccdb
18 files changed
Lines changed: 1766 additions & 37 deletions
File tree
- packages
- cli/src/serve
- core/src
- agents/runtime
- core
- openaiContentGenerator
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
827 | 827 | | |
828 | 828 | | |
829 | 829 | | |
| 830 | + | |
| 831 | + | |
| 832 | + | |
| 833 | + | |
830 | 834 | | |
831 | 835 | | |
832 | 836 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
716 | 716 | | |
717 | 717 | | |
718 | 718 | | |
719 | | - | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
720 | 725 | | |
721 | 726 | | |
722 | 727 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
82 | 82 | | |
83 | 83 | | |
84 | 84 | | |
| 85 | + | |
85 | 86 | | |
86 | 87 | | |
87 | 88 | | |
| |||
1439 | 1440 | | |
1440 | 1441 | | |
1441 | 1442 | | |
| 1443 | + | |
| 1444 | + | |
| 1445 | + | |
| 1446 | + | |
| 1447 | + | |
| 1448 | + | |
| 1449 | + | |
| 1450 | + | |
| 1451 | + | |
| 1452 | + | |
| 1453 | + | |
| 1454 | + | |
| 1455 | + | |
| 1456 | + | |
| 1457 | + | |
| 1458 | + | |
| 1459 | + | |
| 1460 | + | |
| 1461 | + | |
| 1462 | + | |
| 1463 | + | |
| 1464 | + | |
| 1465 | + | |
| 1466 | + | |
| 1467 | + | |
| 1468 | + | |
| 1469 | + | |
| 1470 | + | |
| 1471 | + | |
| 1472 | + | |
| 1473 | + | |
| 1474 | + | |
| 1475 | + | |
| 1476 | + | |
| 1477 | + | |
| 1478 | + | |
| 1479 | + | |
| 1480 | + | |
| 1481 | + | |
| 1482 | + | |
| 1483 | + | |
| 1484 | + | |
| 1485 | + | |
| 1486 | + | |
| 1487 | + | |
| 1488 | + | |
| 1489 | + | |
| 1490 | + | |
| 1491 | + | |
| 1492 | + | |
| 1493 | + | |
| 1494 | + | |
| 1495 | + | |
| 1496 | + | |
| 1497 | + | |
| 1498 | + | |
| 1499 | + | |
| 1500 | + | |
| 1501 | + | |
| 1502 | + | |
| 1503 | + | |
| 1504 | + | |
| 1505 | + | |
| 1506 | + | |
| 1507 | + | |
| 1508 | + | |
| 1509 | + | |
| 1510 | + | |
| 1511 | + | |
| 1512 | + | |
| 1513 | + | |
| 1514 | + | |
| 1515 | + | |
| 1516 | + | |
| 1517 | + | |
| 1518 | + | |
| 1519 | + | |
| 1520 | + | |
| 1521 | + | |
| 1522 | + | |
| 1523 | + | |
| 1524 | + | |
1442 | 1525 | | |
1443 | 1526 | | |
1444 | 1527 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
86 | 86 | | |
87 | 87 | | |
88 | 88 | | |
| 89 | + | |
89 | 90 | | |
90 | 91 | | |
91 | 92 | | |
| |||
2418 | 2419 | | |
2419 | 2420 | | |
2420 | 2421 | | |
| 2422 | + | |
| 2423 | + | |
| 2424 | + | |
| 2425 | + | |
| 2426 | + | |
| 2427 | + | |
2421 | 2428 | | |
2422 | 2429 | | |
2423 | 2430 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7181 | 7181 | | |
7182 | 7182 | | |
7183 | 7183 | | |
| 7184 | + | |
| 7185 | + | |
| 7186 | + | |
| 7187 | + | |
7184 | 7188 | | |
7185 | 7189 | | |
7186 | 7190 | | |
| |||
7195 | 7199 | | |
7196 | 7200 | | |
7197 | 7201 | | |
| 7202 | + | |
| 7203 | + | |
| 7204 | + | |
| 7205 | + | |
| 7206 | + | |
| 7207 | + | |
| 7208 | + | |
| 7209 | + | |
| 7210 | + | |
| 7211 | + | |
| 7212 | + | |
| 7213 | + | |
| 7214 | + | |
| 7215 | + | |
| 7216 | + | |
| 7217 | + | |
| 7218 | + | |
| 7219 | + | |
| 7220 | + | |
| 7221 | + | |
| 7222 | + | |
| 7223 | + | |
| 7224 | + | |
| 7225 | + | |
| 7226 | + | |
| 7227 | + | |
| 7228 | + | |
| 7229 | + | |
| 7230 | + | |
| 7231 | + | |
| 7232 | + | |
| 7233 | + | |
| 7234 | + | |
| 7235 | + | |
| 7236 | + | |
| 7237 | + | |
| 7238 | + | |
| 7239 | + | |
| 7240 | + | |
| 7241 | + | |
| 7242 | + | |
| 7243 | + | |
| 7244 | + | |
| 7245 | + | |
| 7246 | + | |
| 7247 | + | |
| 7248 | + | |
| 7249 | + | |
| 7250 | + | |
| 7251 | + | |
| 7252 | + | |
| 7253 | + | |
| 7254 | + | |
| 7255 | + | |
| 7256 | + | |
| 7257 | + | |
| 7258 | + | |
| 7259 | + | |
| 7260 | + | |
| 7261 | + | |
| 7262 | + | |
| 7263 | + | |
| 7264 | + | |
| 7265 | + | |
| 7266 | + | |
| 7267 | + | |
| 7268 | + | |
| 7269 | + | |
| 7270 | + | |
| 7271 | + | |
| 7272 | + | |
| 7273 | + | |
| 7274 | + | |
| 7275 | + | |
| 7276 | + | |
| 7277 | + | |
| 7278 | + | |
| 7279 | + | |
| 7280 | + | |
| 7281 | + | |
| 7282 | + | |
| 7283 | + | |
| 7284 | + | |
| 7285 | + | |
| 7286 | + | |
| 7287 | + | |
| 7288 | + | |
| 7289 | + | |
| 7290 | + | |
| 7291 | + | |
| 7292 | + | |
| 7293 | + | |
| 7294 | + | |
| 7295 | + | |
| 7296 | + | |
| 7297 | + | |
| 7298 | + | |
| 7299 | + | |
| 7300 | + | |
| 7301 | + | |
| 7302 | + | |
| 7303 | + | |
| 7304 | + | |
| 7305 | + | |
7198 | 7306 | | |
7199 | 7307 | | |
7200 | 7308 | | |
| |||
7261 | 7369 | | |
7262 | 7370 | | |
7263 | 7371 | | |
| 7372 | + | |
| 7373 | + | |
| 7374 | + | |
| 7375 | + | |
| 7376 | + | |
| 7377 | + | |
| 7378 | + | |
| 7379 | + | |
| 7380 | + | |
| 7381 | + | |
| 7382 | + | |
| 7383 | + | |
| 7384 | + | |
| 7385 | + | |
| 7386 | + | |
| 7387 | + | |
| 7388 | + | |
| 7389 | + | |
| 7390 | + | |
| 7391 | + | |
| 7392 | + | |
| 7393 | + | |
| 7394 | + | |
| 7395 | + | |
| 7396 | + | |
| 7397 | + | |
| 7398 | + | |
| 7399 | + | |
| 7400 | + | |
| 7401 | + | |
| 7402 | + | |
| 7403 | + | |
| 7404 | + | |
| 7405 | + | |
| 7406 | + | |
| 7407 | + | |
| 7408 | + | |
| 7409 | + | |
| 7410 | + | |
| 7411 | + | |
| 7412 | + | |
| 7413 | + | |
| 7414 | + | |
| 7415 | + | |
| 7416 | + | |
| 7417 | + | |
| 7418 | + | |
| 7419 | + | |
| 7420 | + | |
| 7421 | + | |
| 7422 | + | |
| 7423 | + | |
| 7424 | + | |
| 7425 | + | |
| 7426 | + | |
| 7427 | + | |
| 7428 | + | |
| 7429 | + | |
| 7430 | + | |
| 7431 | + | |
| 7432 | + | |
| 7433 | + | |
| 7434 | + | |
| 7435 | + | |
7264 | 7436 | | |
7265 | 7437 | | |
7266 | 7438 | | |
| |||
0 commit comments