Skip to content

fix(agent): an empty completion gets one repair before the turn dies - #264

Open
plombeer31 wants to merge 1 commit into
mainfrom
fix/empty-completion-repair
Open

fix(agent): an empty completion gets one repair before the turn dies#264
plombeer31 wants to merge 1 commit into
mainfrom
fix/empty-completion-repair

Conversation

@plombeer31

Copy link
Copy Markdown
Collaborator

What Sentry shows

ModelError with reason=empty is the largest single failure bucket in the project — and it is still firing on 0.4.1:

Issue Events Users Tags
CLI-2W 157 64 category=model, reason=empty
CLI-2X 131 41 category=model, reason=empty
CLI-2Z 90 35 category=model, reason=empty
CLI-5J 73 27 category=model, reason=empty
CLI-4R 53 25 category=model, reason=empty

Plus CLI-B and CLI-H in the same shape — ~500+ events across 200+ users, every one of them a turn that ended because the model returned nothing once.

Root cause

detectModelFailure flags empty, and executeStepInner throws ModelError on the spot — before the parser ever sees the body, and therefore before the one-shot repair path that recovers every other malformed completion (a truncated array, a stray prelude, prose where JSON belongs, a batch that failed validation).

Only native_tools was exempted, via isNativeToolsEmptyCompletionHandledByParser. On the grammar transports a single empty body ends the turn with no recovery attempt at all.

The rationale in detect-model-failure.ts — "re-running the same prompt just reproduces the wall" — holds for truncated and no_stop, where the model already spent its budget on this prefix. It does not hold for empty: the repair path does not replay the prompt. It rebuilds it through buildToolCallRepairPrompt with a corrective notice and a bounded token cap (REPAIR_MAX_TOKENS), which is a materially different request.

The change

isGrammarEmptyCompletionWorthRepairing lets an empty body on a non-native_tools transport fall through to the parser, which fails to parse it and routes into the existing one-shot repair. Nothing new is built — this is the same machinery, reached by one more case.

"Twice empty" stays terminal: the repair's own detectModelFailure check throws ModelError("empty") exactly as before.

Two things deliberately unchanged:

  • native_tools keeps its current contract. A native completion with nothing in any channel routes through ModelError by design, pinned by "native_tools: routes 'no tool_calls and no content' through ModelError, not parse_retry". That is a documented decision, not an oversight, so this PR does not reverse it.
  • truncated / no_stop still fail fast, for the original reason.

The gap this does not close

should-advance.ts documents model → advance: a defective completion should fall over to the next provider. That policy is structurally unreachable today. runWithFallback resolves as soon as the stream opens (llm-fallback-seam.ts), while the emptiness is only detectable after the stream is consumed — so advanceFrom never sees a model defect and the chain never switches on one.

Closing that means either validating completions inside the fallback attempt (impossible for the streaming seam without buffering) or giving the step executor a way to report a defect back to the chain. Both are larger than this fix and change the seam's contract, so I left them out rather than half-doing them here.

This reverses a deliberate decision — please push back if the trade is wrong

agent-loop.test.ts pinned the old behaviour explicitly, under the name "classifies an empty completion as ModelError and skips parse retry" (llmCalls === 1). That test is updated, not worked around: it now pins two calls and never three, and the failure it ends on is still category: model.

If the original call was correctness-driven — some reason an empty grammar body cannot be repaired that I have missed — this PR is wrong and should be closed. If it was load-driven, the trade is one bounded REPAIR_MAX_TOKENS (1024) completion against a turn that currently just dies, on the largest error bucket in production.

Tests

Two new cases: an empty body followed by a good repair completes the step (2 LLM calls, reply executed); an empty body followed by an empty repair still throws ModelError { reason: "empty" }. Verified non-vacuous — the first fails against main with the ModelError thrown at the pre-parser check.

**~500 events across 200+ users, still firing on 0.4.1.**

  CLI-2W  157 ev / 64 users  reason=empty
  CLI-2X  131 ev / 41 users  reason=empty
  CLI-2Z   90 ev / 35 users  reason=empty
  CLI-5J   73 ev / 27 users  reason=empty
  CLI-4R   53 ev / 25 users  reason=empty

`ModelError(reason=empty)` is the largest single failure bucket in the
project. Every one of those events is a turn that ended because the model
returned nothing — once.

`detectModelFailure` flags `empty`, and `executeStepInner` throws on the
spot: before the parser sees the body, and therefore before the one-shot
repair that recovers every OTHER malformed completion — a truncated
array, a stray prelude, prose where JSON belongs, a batch that failed
validation. Only `native_tools` was exempted.

The rationale in `detect-model-failure.ts` — "re-running the same prompt
just reproduces the wall" — holds for `truncated` and `no_stop`, where
the model already spent its budget on this prefix. It does not hold here:
the repair path does not replay the prompt. It rebuilds it through
`buildToolCallRepairPrompt`, with a corrective notice and a bounded token
cap. That is a different request.

So an empty body on a grammar transport now falls through to the parser,
fails to parse, and takes the repair. Nothing new is built — the same
machinery, reached by one more case. Twice empty is still terminal: the
repair's own `detectModelFailure` throws `ModelError("empty")` exactly as
before.

`native_tools` keeps its current contract untouched. A native completion
with nothing in any channel routes through `ModelError` by design, with
its own salvage path for the reasoning-only case.

**This reverses behaviour that was pinned on purpose.**
`agent-loop.test.ts` asserted `llmCalls === 1` under the name "classifies
an empty completion as ModelError and skips parse retry". That test is
updated, not worked around: it now pins two calls and never three, and
the failure it ends on is still `category: model`. If the original
decision was load-driven rather than correctness-driven, this is the
trade to argue with — one bounded 1024-token completion against a turn
that currently just dies.
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.

1 participant