HOTFIX ENG-1081: give the completion verifier room to answer (silent task death, 25% of verdicts) - #278
Conversation
The verdict call is a forced tool call with max_tokens=256. Models that narrate before acting — the open-weight aliases MindsHub serves through Fireworks (mindshub_air/kimi, deepseek) — spend the whole budget on plain prose and never reach the tool call, so `generate_object_code` raises and the fail-safe converts it into a silent "task complete": the turn ends with no message and the agent looks like it died mid-task. Measured in prod Langfuse over 2026-07-14..07-27: 171 of 687 verdict calls returned no tool call (24.9%; 37.1% for external users), 145/147 of them on mindshub_air, every single one with completion_tokens == max_tokens exactly. Not new with ENG-716 — the old free-text verifier also passed max_tokens=256 and 21% of its verdicts were unparseable for the same reason; ENG-716 only changed the consequence from "keep working" to "stop silently". - Verifier budget 256 -> 2048, with one retry at 4096 when truncated. 2048 is measured: mindshub_air spent 1,654 output tokens even with the no-preamble instruction, so 1024 was not enough (1/3 at 1024, 3/3 at 2048). - Verifier prompt now asks for the tool call first, no preamble. Needed alongside the budget, not instead of it (0/3 at 256 even with it). - StructuredOutputError carries `truncated` so a blown budget is retryable while a genuine failure isn't. Detected by output-token count, because the gateway reports finish_reason "stop" at the cap for most aliases (ENG-1082); "length"/"max_tokens" are honoured when reported. - Truncation is logged distinctly from other verifier failures, which all logged identically as "verifier unavailable" before — the reason a 25% failure rate went unseen for 10 days. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review follow-up. If the truncation retry is itself truncated, the model won't fit a verdict at any budget we're willing to pay for — so a chronically-narrating model would otherwise cost 2048+4096 wasted output tokens on every turn instead of the old 256. Latch it after the first proof and stop buying the retry for the rest of the session; the first attempt still runs, so a transient truncation still recovers. Also drops a pointless f-string in the verifier prompt concatenation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…NG-1081)
Adversarial-review follow-up. Four fixes:
1. The detection lived in `client.py`, so the sync twin in the scratchpad
subprocess (`_ScratchpadLLM.generate_object`) still raised a blind
`ValueError("LLM did not return structured output.")` — the same silent,
causeless failure this branch set out to remove, on the path that is
handed to model-written scratchpad code with a caller-chosen max_tokens.
`structured.py` exists to keep those two paths in lockstep, so the
classifier moves there as `raise_missing_tool_call` and both call it.
2. Drop the per-session retry latch. It assumed a double truncation proves
the model can never fit a verdict. Measured: 16 identical verdict calls to
mindshub_air spent 245..1654 output tokens — a 6.7x spread for the same
request — so one truncation is a tail sample, not proof about the next
turn, and latching the retry off on that evidence would reintroduce silent
stops. The retry is cheap because it only fires when the first attempt
truncates (0 of 12 at 2048).
3. The session tests hand-built StructuredOutputError, so they would have
passed even with the detection broken. Added an end-to-end test wiring a
fake provider through the real LLMClient into the session; verified by
mutation (breaking the classifier fails it, and 4 others).
4. The plan_stream fake alternated on `call_count % 2`, coupling it to the
tool loop's internal call count; a third call in a turn would desync it
and the test would assert the wrong thing. Now keyed off turn state.
Also records the measured distribution next to the budget constant, so the
2048 number carries its evidence (median ~290, max 1654, ~1.24x headroom)
instead of a single sample.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (ENG-1081) Third review pass found the same bug behind a second door. The budget can run out *inside* the forced tool call's JSON, not only in the prose before it. That arrives as a non-empty but incomplete tool call — safe_parse_tool_input repairs what it can and sets parse_error — so the missing-call check never sees it, unwrap_structured_response raises a bare ValidationError, the verifier's `except Exception` treats it as a hard failure, skips the retry, and lands on exactly the silent-stop fail-safe this branch exists to remove. Given output length varies ~6.7x per call, a model that narrates ~2000 tokens and then starts emitting the call is precisely the case that lands here. - `looks_truncated` extracted so the same evidence rule covers both shapes. - `raise_missing_tool_call` -> `raise_unusable_tool_call`, message adapts to "did not return" vs "returned an unusable", and now typed NoReturn. - Both the async client and the scratchpad's sync twin classify a failed unwrap as truncation *only* when the response shows it; a malformed call with budget to spare still propagates as a validation error, so genuine schema bugs stay visible instead of buying a hopeless retry. - Parity test upgraded from a substring grep (which a comment could satisfy) to an AST check that the call really is inside the sync `generate_object`. Tests: 15 in the file, mutation-verified — breaking `looks_truncated` fails 7 of them, including both end-to-end shapes. Full unit suite 1329 passed with the same 2 pre-existing scratchpad-exec failures; all 36 e2e scenarios pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…error cause Tool-assisted review pass. - The e2e stub could not express a truncated response (usage.completion_tokens was hardcoded to 10), so the retry had no coverage on the real subprocess path. `_Response.output_tokens` + `queue_verification_truncated()` emulate what the gateway actually returns — prose, no tool call, completion_tokens at the cap, and `finish_reason: "stop"` anyway (ENG-1082). New scenario asserts the session makes a second verdict call with a larger budget and honours the retried verdict instead of stopping silently. - The verifier's generic `except Exception` logged a bare `verdict=ERROR` with no cause — the same undiagnosable swallow this branch exists to fix, and the same thing I flagged on #276. Now logs the exception type and message. Full unit suite 1329 passed; all 37 e2e scenarios pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pnewsam, Major on #277: logging `str(exc)` can copy conversation content into ordinary application logs. Correct — a pydantic ValidationError embeds the rejected `input_value`, which for the verifier is model-generated text derived from the user's conversation, and provider exceptions can carry response bodies. It also contradicted this PR's own security claim. - `_safe_error_detail(exc)` emits the exception type plus, for validation errors, the failing field locations and error codes only — via `errors(include_input=False, include_url=False, include_context=False)`, and reading only `loc`/`type` even on the v1 fallback, so `msg`/`input`/`ctx` can never be quoted. Provider errors reduce to type + status_code. - Same leak class found in a pre-existing line: the summary log wrote `reason=%s`, the model's free-text justification, on EVERY verdict — a broader exposure than the one reported. Dropped; status and counters stay, and the reason remains in the Langfuse trace where conversation content belongs. - 3 regression tests, including one asserting a planted value never reaches the log detail. Also shortened the long comment blocks flagged in review (structured.py, session.py budget/prompt constants, client.py, scratchpad_boot.py) — same facts, roughly half the lines. 1332 unit tests pass; all 37 e2e scenarios pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It runs inside the verifier's `except` handler, so an exception escaping it would convert a gracefully-handled verifier failure into a dead turn — the exact failure mode this branch exists to remove. A custom exception can expose `status_code`/`errors` as a property that raises, so the attribute reads are wrapped too. Degrades to the exception type name, never a crash and never the message. Found while assessing hotfix risk for #278. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It runs inside the verifier's `except` handler, so an exception escaping it would convert a gracefully-handled verifier failure into a dead turn — the exact failure mode this branch exists to remove. A custom exception can expose `status_code`/`errors` as a property that raises, so the attribute reads are wrapped too. Degrades to the exception type name, never a crash and never the message. Found while assessing hotfix risk for #278. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT — merge-safe with one non-blocking documentation correction
I reviewed the complete main...bee97f0 diff and compared it with the approved #277 implementation. The six cherry-picked changes retain the reviewed behavior; the seventh commit safely hardens _safe_error_detail so hostile exception properties cannot escape from inside the verifier's error handler. I found no blocking or major code defects.
Summary of findings
| Severity | Area | Issue |
|---|---|---|
| Minor | PR description | The description says this is the same six commits and hash-identical to #277, but the current branch has a seventh hotfix-only commit (bee97f0) and therefore differs in anton/core/session.py and tests/test_verifier_truncation.py. Please update the provenance wording so the release audit trail matches the code being merged. |
Risk assessment
Overall code risk is Medium and acceptable for a hotfix:
- The change touches shared structured-output code in the LLM client, session, and scratchpad paths, but preserves catcher compatibility through a
ValueErrorsubclass and has focused unit plus subprocess-level coverage. - The main operational risk is output cost/latency: verifier capacity rises from 256 tokens to 2,048, with one 4,096-token retry only after classified truncation. Measured successful calls stay well below those ceilings, but the theoretical double-truncation path can consume 6,144 output tokens.
- There are no migrations, persistent-data changes, dependency changes, auth changes, new endpoints, or deployment-configuration changes. A code rollback is straightforward.
- All required CI, CodeQL, and CLA checks are green, and GitHub reports a clean merge state.
Release recommendations
- Assign owners for the cowork-server
anton-agentbump and hub snapshot rebuild; publishing PyPI alone does not deliver this fix to the affected surfaces. - Create the manual
main → stagingsync immediately after release, since this hotfix branch does not trigger the normal staging unfreeze/sync path. - Monitor verifier
TRUNCATED, retry, token-usage, and latency rates after rollout; revert if retry volume or cost materially exceeds the measured distribution.
What's working well
The retry is bounded and evidence-based, truncation is classified across provider dialects and mid-argument tool calls, sync/async paths share the classifier, sensitive exception content stays out of ordinary logs, and regression coverage exercises provider → client → session behavior end to end.
Reviewed by Codex · alejandrocantu/eng-1081-hotfix-verifier-budget → main · 2026-07-27
|
Thanks — the provenance wording was stale and is now corrected in the description. Detail worth recording for the audit trail: the seventh commit On the 6,144-token double-truncation path you flagged: that is a deliberate tradeoff, and it replaced something worse. An earlier revision latched the retry off per session after one double-truncation; I removed it because output length is stochastic — 16 identical calls spanned 245–1654 tokens, a 6.7x spread — so one truncation is a tail sample, not proof about the next turn, and latching on that evidence would have reintroduced silent stops for a model that usually succeeds. Measured: 0 of 12 calls truncate at 2048, and the worst case across all 19 advertised models is 1054 tokens, so the retry should be rare and the 6,144 path rarer still. Both are visible in the logs ( Agreed on all three release recommendations — the delivery owners, the immediate manual |
Hotfix: ENG-1081 — the completion verifier's 256-token ceiling
This is #277 retargeted at
mainas a hotfix. #277 is approved (@pnewsam) and stays open until this lands — see the plan at the bottom.Provenance (7 commits). Six are the reviewed #277 commits, cherry-picked onto
origin/mainwith zero conflicts. The seventh,bee97f0, hardens_safe_error_detailso it cannot raise from inside the verifier'sexcepthandler — found while assessing hotfix risk, after #277 was approved. It was applied to #277's branch as well (9b7906b), so the two branches are byte-identical: verified per file withgit hash-objectacrosssession.py,client.py,provider.py,structured.py,scratchpad_boot.py,test_verifier_truncation.py,stub_server.pyandtest_loop_safety.py. Reviewing either branch reviews the same code.Why it can't wait for next Monday
v2.26.7.27.1shipped today at 11:34 UTC without it. Measured in prod Langfuse over 2026-07-14 → 07-27:COMPLETE, and end the turn with no message at all.mindshub_air(98.6%) — for those users it is not intermittent, it is every turn. Whole sessions where it never once worked: 15/15, 12/12, 8/8.Root cause is the verdict call's
max_tokens=256versus models that narrate ~1,200–1,300 characters before the forced tool call — the preamble alone exceeds the whole budget, so the call could never fit.Why the diff is safe to put straight on main
mainandstaging(session.py,client.py,provider.py,structured.py,scratchpad_boot.py,conftest.py,stub_server.py). The two branches differ in exactly one unrelated file (prompts.py, ENG-838).Merge plan (team hotfix convention — Option B)
release.ymlauto-tags and publishes (expectv2.26.7.27.2).mainback intostagingwith a sync PR. Notestaging-unfreeze.ymlis gated onhead.ref == 'staging', so the usual automatic sync-back will not fire for this PR — it needs doing by hand.mindshub_airfix(session): completion verifier fails toward a real message, not silence (ENG-1079) #276 alone would show "an internal check failed, how would you like to proceed?" on ~99% of turns.Delivery note: the PyPI release is not the delivery path for either surface. Desktop needs the cowork-server
anton-agentbump; hub agents need a snapshot rebuild.Full detail
Evidence, the measured token distribution behind
2048, the review trail (4 adversarial passes, @pnewsam's privacy finding), and the model matrix are all on #277 — that discussion is the review of record for this change.Closes ENG-1081.