Skip to content

fix(session): completion verifier fails toward a real message, not silence (ENG-1079) - #276

Open
torrmal wants to merge 2 commits into
stagingfrom
jorge/eng-1079-verifier-failsafe-waiting
Open

fix(session): completion verifier fails toward a real message, not silence (ENG-1079)#276
torrmal wants to merge 2 commits into
stagingfrom
jorge/eng-1079-verifier-failsafe-waiting

Conversation

@torrmal

@torrmal torrmal commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The completion-verifier's generate_object_code call can throw (provider hiccup, malformed structured output, etc.). Before this fix, that exception was silently mapped to a fake COMPLETE verdict, so the task loop just broke with no message at all — the agent looked like it had died on the first hurdle it hit, even in a fresh session (ENG-1079).
  • On a verifier-call exception, this now routes through the same honest-diagnosis pattern already used for STUCK: append a SYSTEM note admitting the check itself failed, and let the model generate a real message — summarize progress, be honest about the internal failure, and ask the user how they'd like to proceed. Functionally a WAITING-style stop, but with a real, model-generated message instead of silence.
  • Also persists that generated message to history directly, rather than relying on the shared post-loop fallback (which re-appends the stale pre-verification reply and would otherwise leave history out of sync with what the user actually saw).
  • Follow-up: none of the three "hand control back to the user" paths (STUCK, budget-exhausted, and the new verifier-failure path) asked the model for a solvability self-assessment — just "what I tried" + "next steps," which can read as "here's what happened, good luck" instead of an actual recommendation. Added a shared _SOLVABILITY_CLAUSE ("is this still solvable on your own with a different approach, or does it genuinely need the user's input/decision/credentials") wired into all three.

Context

Traced from a regression report that Anton seems to get stuck/"die" on the first hurdle even after restarting from scratch. Root-caused to the ENG-716 verifier rework (db10fdcf/92f47b14/etc.), which moved the verdict to a generate_object_code call on the cheap coding model over a compact transcript — a less forgiving code path than before, sitting behind a silent fail-safe that had never existed pre-ENG-716.

Test plan

  • tests/test_verifier_failsafe.py — verifier exception yields a real streamed message (not silence), makes exactly one honest-diagnosis LLM call, does NOT inject a "Continue working" continuation, persists the real message to history, and asks for the solvability self-assessment.
  • uv run pytest tests/ -k "session or verifier or loop_safety" --ignore=tests/e2e — 89 passed, 2 skipped.
  • uv run pytest tests/e2e/scenarios/test_loop_safety.py — passes in an isolated worktree; occasional single-test timeouts under load are pre-existing environmental flakiness (real subprocess CLI spawn, confirmed to pass in isolation both before and after this change).

🤖 Generated with Claude Code

torrmal and others added 2 commits July 27, 2026 11:00
…lence (ENG-1079)

An exception from the completion verifier's generate_object_code call
(provider hiccup, malformed structured output, etc.) was silently treated
as a COMPLETE verdict — the task loop just broke with no message at all,
making the agent look like it had died on the first hurdle it hit.

On a verifier-call exception, route through the same honest-diagnosis
pattern already used for STUCK: append a SYSTEM note admitting the internal
check failed, and let the model summarize progress and ask the user how
they'd like to proceed — a real WAITING-style message instead of silence.
Also persist that generated message to history (the STUCK path relies on a
stale pre-verification `reply` for its post-loop history fallback, which
would otherwise duplicate old text instead of recording what was actually
said).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…y assessment (ENG-1079)

STUCK, budget-exhausted, and the new verifier-failure diagnosis all asked
the model to summarize what it tried and suggest next steps, but never to
state whether it actually believes the task is still solvable on its own
(with a different approach) or genuinely needs the user's input/decision.
Without that, a diagnosis reads as "here's what happened, good luck"
instead of a real recommendation the user can act on.

Add a shared _SOLVABILITY_CLAUSE and wire it into all three paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alecantu7

Copy link
Copy Markdown
Contributor

Measured the verifier failure rate in prod Langfuse to check the root cause behind this fix. The fail-safe direction here is right, and the data supports landing it — but two findings change the details. Full write-up in ENG-1081 / ENG-1082; the parts that touch this PR:

1. Merge order matters. The verifier failure isn't a rare "provider hiccup" — it's 171 of 687 verdict calls since ENG-716 (24.9%; 37.1% for external users), and it's concentrated: 98.6% of mindshub_air calls (145/147). So on its own, this PR would show "an internal check failed, how would you like to proceed?" to those users on nearly every tool-using turn. Worth landing the token-ceiling fix (ENG-1081) first or together, so this path stays the rare last resort it's designed to be.

2. The root cause is a token ceiling, not provider flakiness — and it isn't from ENG-716. Every failure has completion_tokens == max_tokens exactly (256/256), with ~1,300 chars of reasoning prose in content, cut off before the tool call. Three aliases narrate before acting (kimi, mindshub_air = kimi-k2p6, deepseek); the other nine emit the forced tool call immediately. Ran the identical verifier payload against all 14 aliases: 9/9 succeed at max_tokens=256 in 43–115 tokens with zero prose, those 3 fail 0/2 at exactly 256.

The old pre-ENG-716 free-text verifier passed max_tokens=256 too, and 21% (116/554) of legacy verdicts were unparseable for the same reason. So db10fdcf didn't make the verifier less reliable — it changed what truncation costs, from "keep working" to "stop silently". Which is exactly the gap this PR closes; just worth correcting in the PR/ticket framing so the fix lands in the token budget too.

3. Heads-up for any truncation handling here: finish_reason is unusable on this gateway. It returns "stop" while handing back exactly max_tokens tokens, for 9 of 11 aliases (all Anthropic- and OpenAI-backed ones; only Gemini reports "length"). Filed as ENG-1082 with the two-line fix. Until that ships, truncation has to be detected as completion_tokens >= max_tokens.

Empirically, both halves of the ceiling fix are needed — mindshub_air, 3 runs each: 256 → 0/3, 256 + "no preamble" prompt → 0/3, 1024 alone → 1/3, 2048 + no-preamble → 3/3. Note it used 1,654 output tokens even with the no-preamble instruction, so 1024 would not have been enough.

One small thing on the diff itself: the new failure path logs verdict=ERROR but doesn't distinguish truncation from a genuine provider error. Given 25% of calls hit this branch, splitting the two would make the rate measurable from logs alone rather than needing a Langfuse scan next time.

@alecantu7 alecantu7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review. The direction is right — failing toward a real message is strictly better than the two options ENG-716 chose between, and sharing _SOLVABILITY_CLAUSE across all three hand-back paths is the correct instinct. Seven findings below, ranked. #1 is a confirmed bug in code this PR edits; #2 and #3 are substantive; the rest are small.


1. The history-sync bug you fixed still exists in the two other paths this PR touches — confirmed, with repro

The PR body diagnoses this exactly right: the post-loop fallback "re-appends the stale pre-verification reply... leaving history out of sync with what the user actually saw." That fix is applied only to the new verifier-error path. STUCK and budget-exhausted have the identical shape and the identical bug — and both are edited in this PR for _SOLVABILITY_CLAUSE.

Both do: append the SYSTEM note → stream the diagnosis → break, with no assistant append. So self._history[-1]["role"] == "user", the post-loop guard at session.py:2795 doesn't fire, and it appends llm_response.content — the pre-verification reply, which was already appended at session.py:2615.

Verified by driving a STUCK verdict through turn_stream (probe test, not committed):

===== HISTORY AFTER A STUCK VERDICT =====
  0. user      load my db
  1. assistant [text "Running." + tool_use scratchpad]
  2. user      [tool_result]
  3. assistant PRE-VERIFY REPLY: all set!
  4. user      SYSTEM: Task verification determined this task is stuck. ...
  5. assistant PRE-VERIFY REPLY: all set!        <-- duplicated

  DIAGNOSIS present in history?   False
  PRE-VERIFY REPLY count:         2

So on the STUCK path the model's next turn sees itself cheerfully saying "all set!" twice and never sees the honest diagnosis it just streamed to the user. That's worse than the case this PR fixes: the model is now actively misinformed about its own last message. Budget-exhausted is structurally identical (session.py:2618-2637 — append note, stream, break).

Since all three paths are touched here, suggest fixing all three, and extracting the shared shape rather than a third copy of it — something like _stream_diagnosis(system) that appends the SYSTEM note, yields the progress event, streams, and persists the streamed text. Three hand-back paths that must stay in lockstep have already drifted once; a helper makes the next drift impossible.

2. The swallow is still undiagnosable for operators

except Exception: binds no exception, and verdict=ERROR logs no cause. This PR fixes the silent failure for the user but keeps it silent for whoever has to debug it.

That matters concretely: the reason ENG-1079 sat for 10 days is that four different failure modes (truncation, provider 5xx, schema mismatch, disabled-model 403) all logged the identical reason="verifier unavailable". As of today ~25% of verdict calls take this branch (ENG-1081 — 171/687 measured in prod, 98.6% on mindshub_air), and you still can't tell from logs which of the four you're looking at. except Exception as exc: plus type(exc).__name__ in the log line is a one-liner that would have made this whole investigation a grep.

3. Cost: this adds a full planning-model turn over the whole history to a quarter of verified turns

plan_stream_with_recovery(system=system) runs on the planning model against the entire history — the expensive call (cf. ENG-578). At the measured 24.9% failure rate that's an extra full-history planning turn on a quarter of all verified turns; for a mindshub_air user (98.6% failure) it's an extra one on nearly every turn, on top of the wasted verdict call.

Two implications:

  • Land ENG-1081 (#277) first or together. It removes ~99% of the triggers, which turns this from a per-turn tax into the rare fallback it's designed to be.
  • Consider latching per session: once the diagnosis path has fired and the next verdict call fails the same way, stop paying for a fresh full-history diagnosis every turn. (#277 does this for the truncation retry — same reasoning, same shape.)

4. An empty diagnosis silently reverts to the old behaviour

_append_history no-ops on empty content (session.py:682-684), so if the diagnosis returns empty (refusal, provider hiccup), nothing is appended → the post-loop appends the stale reply → you're back to exactly the out-of-sync history this PR fixes, with no trace. The diagnosis call inherits the default max_tokens so truncation is unlikely, but if diagnosis_response is not None isn't quite the guard you want — if diagnosis_response and diagnosis_response.response.content plus a warning log when it's empty makes the failure visible instead of circular.

5. The progress copy says the opposite of what's happening

"Checking in before continuing..." — the turn is stopping to hand control back, not continuing. Users read this text. Something like "Something went wrong — checking in with you..." matches the actual behaviour.

6. Test gaps that follow from #1

test_verifier_failsafe.py covers the new path well (I like that it pins the call count at 3 and asserts no "Continue working" injection). Nothing covers history sync on the STUCK or budget-exhausted paths — the two places the bug still lives — and nothing asserts the stale reply isn't duplicated. If #1 is fixed here, those are the two tests that lock it down.

7. Nits

  • tests/test_verifier_failsafe.py:35 uses Path(__file__).resolve().parent / ".pytest-workspace"; every other test file uses parents[1] (repo root). As written it creates a stray tests/.pytest-workspace.
  • Two f"..." prefixes with no placeholders in the new and edited prompt blocks.
  • The new path returns before the existing completion-verifier verdict=%s ... reason=%s summary line, so log consumers see a different line shape for this branch. Fine by me, but worth knowing if anything parses that line.

Note for whichever of us merges second: #277 and this PR both rewrite the same try/except. #277 keeps the fail-safe in an else: branch after a budget-retry loop, so the honest-diagnosis body here drops into that else: directly. Deliberately not stacked.

alecantu7 added a commit that referenced this pull request Jul 27, 2026
…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>
alecantu7 added a commit that referenced this pull request Jul 27, 2026
…task death, 25% of verdicts) (#278) (#279)

* fix(session): give the completion verifier room to answer (ENG-1081)

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.



* fix(session): bound the verifier retry to once per session (ENG-1081)

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.



* fix(llm): share the truncation classifier with the scratchpad path (ENG-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.



* fix(llm): treat a tool call truncated mid-arguments as truncation too (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.



* test(e2e): cover the truncation retry end-to-end; log the verifier's 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.



* fix(session): keep conversation content out of verifier logs (review)

@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.



* fix(session): _safe_error_detail must never raise (hotfix hardening)

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>
woss pushed a commit to woss/fork-anton that referenced this pull request Jul 27, 2026
…task death, 25% of verdicts) (mindsdb#278)

* fix(session): give the completion verifier room to answer (ENG-1081)

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>

* fix(session): bound the verifier retry to once per session (ENG-1081)

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>

* fix(llm): share the truncation classifier with the scratchpad path (ENG-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>

* fix(llm): treat a tool call truncated mid-arguments as truncation too (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>

* test(e2e): cover the truncation retry end-to-end; log the verifier's 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 mindsdb#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>

* fix(session): keep conversation content out of verifier logs (review)

@pnewsam, Major on mindsdb#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>

* fix(session): _safe_error_detail must never raise (hotfix hardening)

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 mindsdb#278.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants