fix(compaction): keep the failure record a projection is re-read through - #8432
Merged
Merged
Conversation
KeepErrors classifies a failure from ToolExecution rather than from message text, because text matching was proven to miss real failures: a failing `go test` log opens with "=== RUN", and no prefix match can see it. But checkpointProjectionMessages wrote the projection through ModelMessages, which strips ToolExecution on the way to a provider — so the record survived exactly one fold. The second fold saw a tool result with no failure record and text that matches nothing, folded it into the digest, and the model stopped being told about the failure. Measured on a fixture shaped like real `go test` output: present after fold 1, record stripped after fold 2, gone from the projection after fold 3. A projection is read twice — as what the model is sent, and as the input the next compaction classifies — and only the second reading needs the host's own record. So the strip belongs at the provider boundary, which every request path already crosses (context_manager.go builds each request through ModelMessages of its own accord, making the strip at write time redundant as well as harmful). ProjectionMessages is that same projection with ToolExecution preserved. Both functions and their shared implementation move to provider/projection.go: deriving the two copies of a transcript is one responsibility, and provider.go was already carrying recorded file-size debt this would have widened. The regression test uses a "=== RUN" fixture on purpose. A synthetic "error: ..." body survives either way through the text-prefix arm, which is what let this go unnoticed. Verified by mutation: restoring ModelMessages at the write site turns it red at fold 2. Compatibility holds in both directions: ToolExecution is already persisted in canonical transcripts, so no new field enters the sidecar format, and a projection written by an older build simply has no record to read.
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.
Found while auditing a suspicion raised during the #8419 / #8424 work. The
suspicion (that
KeepErrorscontent accumulates without bound) turned out to bewrong —
failure_snip.goalready caps each failure at 60 lines and its owncomment acknowledges the retention is deliberate. The real defect was the
opposite: the protection expires.
Measured behaviour, before
Fixture shaped like real
go testoutput,KeepErrorson, three folds:ToolExecutionWhy
KeepErrorsclassifies a failure fromToolExecution, not from message text —that change was made deliberately because text matching missed real failures. A
failing
go testlog opens with=== RUN, so theerror:/blocked:prefixarm cannot see it.
checkpointProjectionMessageswrote the projection throughModelMessages,which strips
ToolExecution:That strip is correct for a provider request. The problem is that a
projection is read twice: once as what the model is sent, and once as the input
the next compaction classifies. Only the second reading needs the host's record.
Writing the projection through the provider-boundary function collapsed those two
roles, so fold 2 saw a tool result with no failure record and text matching
nothing, folded it into the digest, and the model stopped being told.
Note this makes the write-time strip not merely harmful but redundant:
context_manager.go:190already builds every request throughModelMessagesonits own.
Change
ProjectionMessages— the same projection,ToolExecutionpreserved. The stripstays at the provider boundary where it belongs. Both functions and their shared
implementation move to
provider/projection.go: deriving the two copies of atranscript is one responsibility, and
provider.gowas already carrying recordedfile-size debt that keeping them there would have widened.
Tests
The regression test uses a
=== RUNfixture on purpose. A synthetic"error: ..."body survives either way through the text-prefix arm — which isexactly what let this sit unnoticed. It asserts three things per fold: the
failure is still in the projection, its record is still readable, and no
ToolExecutionreaches the provider request.Verified by mutation, not just by passing: restoring
ModelMessagesat the writesite turns it red at fold 2 with
the failure record was stripped, so the next fold cannot classify it. Restored and re-verified green.Compatibility
ToolExecutionis already persisted in canonical transcripts (its own doccomment says so), so no new field enters the sidecar format and no privacy
boundary moves. A projection written by an older build simply has no record to
read, degrading to today's behaviour rather than failing.
Verification
Cache-impact: none - the provider-visible bytes are byte-identical. ProjectionMessages differs from ModelMessages only in a field that ModelMessages strips again at every request boundary, so no request payload, prompt-cache prefix, or projection fingerprint changes; providerVisibleFingerprint calls ModelMessages itself.
Cache-guard: internal/agent/failure_survives_folds_test.go asserts no ToolExecution reaches provider.ModelMessages output on every fold, alongside the unchanged internal/agent/cachehit_e2e_test.go.
Documentation-impact: updated - docs/SPEC.md "What survives a fold" now explains why a stored projection keeps the ToolExecution record that a provider request does not, with the matching section in docs/SPEC.zh-CN.md.