fix(core): a truncated completion is no longer a silent success (#699) - #716
Open
rejifald wants to merge 1 commit into
Open
fix(core): a truncated completion is no longer a silent success (#699)#716rejifald wants to merge 1 commit into
rejifald wants to merge 1 commit into
Conversation
Both first-party llm provider mappings have always lifted `finishReason`
— anthropic's `stop_reason`, openai's `finish_reason` — and nothing ever
read it. A completion the token cap cut off therefore resolved `ok: true`
with `findings: []`, byte-identical on every observer to a model that
finished on its own terms. The failure mode is the worst shape an API can
have: the caller gets half an answer and no way to know it, so the
truncation surfaces downstream as a parse error, a malformed tool call,
or a wrong answer nobody traces back here.
`LlmResult` gains `truncated`, derived by the surface from the normalised
`finishReason` so a caller never has to know that anthropic spells the cap
`max_tokens` and openai spells it `length`. It is three-state on purpose:
`true`, `false`, and ABSENT when the provider lifted no finish reason at
all — a confident `false` over silence would be the same bug this field
exists to remove, one level up. A BYO provider that already decided the
question in its own `parse` wins over the two-entry vocabulary, which is
the P21 seam doing its job rather than a special case.
Alongside it, a `warn` drift finding, so the signal reaches
`.inspect().findings`, the `drift` event and the trace with no wiring.
That needs a channel: `interpret` could previously only fail a call or say
nothing, and truncation is neither. `SurfaceOutcome`'s success arm gains an
optional `findings`, merged by the engine into the same stream the `output`
contract and `flagFinding` already feed. It reuses the `coerced` change
kind rather than widening `SoftDriftChange` — `flagFinding`'s precedent —
and `coerced` already defaults to `warn`, so kind and level agree.
Reported, NOT thrown. The call succeeded at every layer this surface owns:
the transport worked, the body is well-formed, `text` holds real tokens.
Whether a partial answer is usable is the caller's question, and this
change is only about putting them in a position to ask it. Making
truncation fatal is a semver-major behaviour change and a maintainer's
decision; it is deliberately left as a follow-up.
Which is why `makeLlmSurface` is exported, and why that is the unblocker
rather than a nicety. The exported `llmSurface` is a bare `{ id: 'llm' }`
identity with nothing to wrap, and the real factory — which closes over
the provider and defaults `llm(config)` assembles internally — had no
`export`, so a caller could not implement any of this themselves, nor
adopt the strict policy this PR declines to impose. With it public, that
policy is a five-line `interpret` wrapper (pinned in the tests) instead of
a fork. `LlmDefaults`, its argument, is exported with it — a public
factory taking a private parameter type is not constructible.
Twelve tests in llm-truncation.spec.ts: both vendor shapes truncated and
not, the unknown third state, the BYO provider overriding in both
directions, case-insensitive recognition, and `makeLlmSurface` imported,
driven through `stitch({ kind })`, and wrapped to make truncation fatal.
The whole-entry bundle is unchanged (24.10 KB gzip, measured against a
stashed tree); llm rides its own subpath and never enters it.
Refs #699 — §2 (the unrequested `max_tokens: 1024` default for anthropic,
which is what makes the default config truncate in the first place) and
§4–§7 stay open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A truncated LLM completion was a success on every observer. Both first-party provider
mappings have always lifted
finishReason, and nothing ever read it:origin/main, 44a9fcc)interpretisverdictOfthen{ ok: true, data: provider.parse(res.body) }packages/core/src/llm.ts:145-149stop_reason→finishReasonpackages/core/src/llm.ts:308finish_reason→finishReasonpackages/core/src/llm.ts:355-356makeLlmSurfacehas noexport; its only non-comment reference isllm.ts:177packages/core/src/llm.ts:130llmSurfaceis a bare identity — nothing to wrappackages/core/src/llm.ts:117So a completion the token cap cut off resolved
ok: truewithfindings: [], byte-identical toa model that finished on its own terms. The caller gets half an answer and no way to know it —
the truncation resurfaces downstream as a parse error, a malformed tool call, or a wrong answer
nobody traces back here.
What changed
1.
makeLlmSurfaceis exported (llm.ts). Pure addition, no behaviour change — and theunblocker.
llmSurfaceis only the{ id: 'llm' }identity, so there was nothing to wrap, andthe real factory closes over the provider and defaults that
llm(config)assembles internally.Without it a caller could not implement the fix themselves, nor adopt the strict policy this PR
deliberately declines to impose.
LlmDefaults, its argument, is exported with it — a publicfactory taking a private parameter type is not constructible.
llmis published as its own subpath entry ("./llm"→lib/llm.*inpackages/core/package.json),and
src/index.tsre-exports nothing from it. So theexportkeyword is the whole publicationstep; there is no barrel to update. Verified in the emitted artefacts:
lib/llm.d.mtsexportsLlmDefaults+makeLlmSurface, andlib/llm.mjsexportsanthropic, llm, llmSurface, makeLlmSurface, openaiat runtime.2. Truncation is observable, in the two places a caller looks:
LlmResult.truncated?: boolean, derived by the surface from thealready-normalised
finishReason, so a caller never has to know anthropic spells the capmax_tokensand openai spells itlength. Three-state on purpose:true,false, andabsent when the provider lifted no finish reason at all. A confident
falseover silencewould be the same bug this field exists to remove, one level up.
warndrift finding — so it reaches.inspect().findings, thedriftevent and thetrace with no extra wiring.
The finding needed a channel.
interpretcould previously only fail a call or say nothing, andtruncation is neither — so
SurfaceOutcome's success arm gains an optionalfindings?: DriftFinding[], which the engine merges into the same stream theoutputcontractand
flagFindingalready feed (engine.ts, one merge site). It reuses thecoercedchange kindrather than widening
SoftDriftChange—flagFinding's own precedent — andcoercedalreadydefaults to
warn, so kind and level agree rather than argue.Extension seam (P21): a BYO provider that already decided the question in its own
parsewins over the two-entry vocabulary; the surface only derives
truncatedwhen the providerdidn't declare one. Recognition is case-insensitive so a BYO provider passing its vendor's string
through verbatim isn't defeated by case alone.
Why reported, not thrown
The call succeeded at every layer this surface owns: the transport worked, the body is
well-formed,
textholds real tokens. Whether a partial answer is usable is the caller'squestion — this change is only about putting them in a position to ask it.
Making truncation fatal is a semver-major behaviour change and a maintainer's decision, not
mine. It is a deliberate follow-up, as is an opt-in
strict-style spelling if one is wanted.I did not find an existing idiomatic opt-in mechanism for exactly this, so I added none —
rather than invent config vocabulary on a maintainer's behalf. Exporting
makeLlmSurfaceis whatmakes the strict policy a five-line
interpretwrapper instead of a fork, and that wrapper ispinned in the tests so the follow-up has a working starting point:
Semver: minor. Two additive optional fields (
LlmResult.truncated,SurfaceOutcome.findings)and one new export. No call that succeeded before fails now — only
level: 'error'findings arefatal and this one is
warn. The one observable change is a newwarnfinding in.inspect().findings/ the drift stream on truncated completions, which is the point.Out of scope
max_tokens: 1024injection (llm.ts:283): anthropic'sbuildBodyinjects a capthe caller never wrote, which is why the default config truncates and then reports
ok: true.Left alone deliberately: it is a defaults question worth its own discussion (anthropic's API
requires
max_tokens, so "just remove it" isn't available — the choice is between a documenteddefault, a model-aware one, and failing closed). This PR makes the consequence visible; it does
not pick the default.
Refs #699, notFixes.Two honest notes
SurfaceOutcome.findingsis not levelled bydrift.severity, exactly likeflagFindingtoday:severityresolves inside the diff, over the kinds the diff produces.A
severity: 'info'allowlist will not suppress thiswarn. Pre-existing behaviour, nowshared by one more finding source, and written into the comment at the merge site.
runFrom), which is whereflagFindinglives. The paginated path collects no engine-level findings today and
llmis not paginated.Tested
12 new tests in
packages/core/test/llm-truncation.spec.ts(style followsllm.spec.ts— acapturing adapter, no network):
stop_reason: 'max_tokens'→truncated: true+ awarnfinding on.inspect()finish_reason: 'length'→ likewiseend_turn/stop→truncated: false,findingsemptyfinishReasonat all →truncatedabsent, notfalsetruncatedwins in both directionsMAX_TOKENS)makeLlmSurfaceimportable, builds a live surface, drives astitch({ kind }), and wraps tomake truncation fatal
Gates
All green from the repo root:
prettier --check(changed files),check:lint,check:types,test(1503 passed, 140 files),check:types-d,check-changelog.mjs,check-contract.mjs,check-unknown-keys.mjs, plus the two the new export could move —check:exports(attw, allsubpaths 🟢) and
check:size.On the size gate: the whole-entry budget reports
0.00 KB left. That is pre-existing onorigin/main— I measured a stashed tree and got the identical 67.11 KB minified / 24.10 KBgzip. This change adds zero bytes:
llmrides its own subpath and never enters the coreentry, and the
surface.tschange is types and comments only.Refs #699🤖 Generated with Claude Code