fix(harness): truncation marker could push output past its char budget - #656
Open
teyrebaz33 wants to merge 3 commits into
Open
fix(harness): truncation marker could push output past its char budget#656teyrebaz33 wants to merge 3 commits into
teyrebaz33 wants to merge 3 commits into
Conversation
Two truncation helpers write the shared PAYLOAD_TRUNCATION_MARKER
format and had the same bug: they sliced content to exactly the
configured budget, then appended the marker AFTER the slice, so the
actual output was budget + marker.length chars, not budget.
- truncateForPayload (core/collector/normalizer.ts), used for
toolInput/toolResponseSummary in the collector's hook normalization.
- clip (core/record-archive.ts), used by compactSessionRecord for
prompt/assistantText/tool input/responseSummary. This one also
documents an idempotency guarantee ("a second pass finds a body
already at the cap and returns it untouched"); the old
implementation satisfied that (a brute-force sweep found 0
idempotency violations in the old code across 56 cases) but
converged on a value that was already over budget, since the bug
was in the first pass's arithmetic, not the unwrap/fold logic.
Neither is a practical incident today: the aggregate RECORD_MAX_BYTES
cap in compactSessionRecord is enforced separately via a real
Buffer.byteLength() measurement after clipping and drops whole turns
oldest-first until it fits, so it doesn't trust clip()'s per-field
promise. But both functions directly contradicted their own documented
"bounded" guarantees, and any future caller relying on that documented
per-field bound would be silently wrong.
Fix: the same converging-slice approach in both functions (the
marker's own length depends on the dropped-char count, which depends
on where we slice, which depends on the marker's length, so this
takes a few passes to land exactly on budget), with a final
.slice(0, budget) backstop for degenerate tiny budgets. Verified with
a 56-case brute-force sweep (budgets down to 0, including 0) checking
both the bound AND the idempotency property together -- the fix holds
both simultaneously.
Updated three existing tests whose exact-match assertions had encoded
the old (over-budget) output as the expected value; added one new
test for the degenerate maxLength-smaller-than-marker case. All other
tests unchanged and passing. The two failures in
record-archive-wiring.test.ts are a pre-existing node-pty/tmp-dir
environment issue, unrelated to and unaffected by this change
(verified identical via git stash).
No dependency or lockfile changes.
Same bug class as the previous commit on this branch, found while doing a final repo-wide sweep before opening the PR: clamp() (core/resume-brief.ts) sliced text to exactly maxChars, then appended CLAMP_MARKER after the slice, so the total could exceed maxChars by the marker's length (12 chars). Unlike truncateForPayload/clip's "chars dropped" marker, CLAMP_MARKER is a fixed string with no embedded count, so its length doesn't depend on where the slice lands -- a single subtraction (no converging loop) correctly reserves room for it. Of clamp()'s 7 call sites, 6 had no compensation and were exposed to the overrun; the 7th (the last-resort summary-shortening loop) had manually pre-subtracted CLAMP_MARKER.length from its own maxChars argument specifically to work around this exact bug. Now that clamp() self-enforces the bound internally, that workaround is removed -- fixing the primitive once beats every caller re-deriving the same compensation. Verified with a 54-case brute-force sweep (0 failures) and a new test exercising describeToolTarget's use of clamp() through the public API, confirmed to fail (92 > 80) against the pre-fix implementation via git stash and pass against the fix. Also corrects a stale doc comment on CLAMP_MARKER that claimed it was "the marker truncateForPayload leaves" -- the two markers are actually different formats (truncateForPayload's embeds a dropped-char count; CLAMP_MARKER doesn't) and CLAMP_MARKER is never used to parse truncateForPayload's output, so the claim was inaccurate. No dependency or lockfile changes.
This was referenced Aug 21, 2026
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.
Primary change type
Problem and motivation
Three truncation helpers in
packages/harnesswrite similar truncation-marker formats and had the same bug: they sliced content to exactly the configured budget, then appended the marker AFTER the slice, so the actual output wasbudget + marker.lengthcharacters, notbudget.truncateForPayload(core/collector/normalizer.ts) — used fortoolInput/toolResponseSummary.clip(core/record-archive.ts) — used bycompactSessionRecord. Also documents an idempotency guarantee ("a second pass finds a body already at the cap and returns it untouched"); that guarantee itself held (0 idempotency violations in a 56-case sweep of the pre-fix code) but converged on a value that was already over budget — the bug was in the first pass's arithmetic, not the unwrap/fold logic.clamp(core/resume-brief.ts) — found in a final repo-wide sweep before opening this PR. Same bug, but its marker (CLAMP_MARKER) is a fixed string with no embedded count, so the fix is a single subtraction rather than a converging loop. Of its 7 call sites, 6 had no compensation; the 7th had manually pre-subtractedCLAMP_MARKER.lengthfrom its own budget to work around this exact bug.None of the three is a practical incident today: the aggregate
RECORD_MAX_BYTEScap incompactSessionRecordis enforced separately via a realBuffer.byteLength()measurement after clipping and drops whole turns oldest-first until it fits, andresume-brief's overrun is ~12 chars against a token-based ceiling with its own headroom. But all three directly contradicted their own documented guarantees.Summary and scope
truncateForPayload/clip: a small converging loop that shrinks the slice point until slice + marker fits within budget (the marker's own length depends on the dropped-char count, which depends on where we slice, which depends on the marker's length), with a final.slice(0, budget)backstop for degenerate tiny budgets.clamp: a single subtraction, sinceCLAMP_MARKER's length is fixed. Also removed the now-unnecessary manual- CLAMP_MARKER.lengthworkaround atclamp's one self-compensating call site, and corrected a stale doc comment that inaccurately describedCLAMP_MARKERas "the markertruncateForPayloadleaves" (the two are different formats and unrelated at runtime).Out of scope:
packages/harness/web/src/lib/extract-step-context.ts'sformatValue, a fourth instance of the same pattern found in the same sweep, has its own separate PR (different subsystem — web frontend, not this collector/archive backend code).Related work
Related issue or discussion: #655
Validation
Tests and documentation
Updated three existing tests whose exact-match assertions had encoded the old (over-budget) output as the expected value, with new expected values computed independently and cross-checked. Added two new tests: one for
truncateForPayload's degenerate maxLength-smaller-than-marker case, one forclamp's bound viadescribeToolTarget's public API — confirmed viagit stashto fail (92 > 80) against the pre-fix code. Added a 56-case brute-force sweep (clip, bound + idempotency together) and a 54-case sweep (clamp, bound) as one-off verification, referenced in commit messages. No user-facing documentation changes needed.Compatibility and release impact
.changeset/fix-harness-truncation-marker-budget.md), patch bump for@sapiom/harness.Security
will follow the
Security Policy for
private reporting.
AI assistance
I used Claude (Anthropic) as a coding assistant throughout: it helped find the bug pattern (including the third instance in
resume-brief.ts, found via a repo-wide grep sweep), draft the fixes and tests, and run the verification commands (tests/typecheck/lint, brute-force sweeps,git stashcomparisons) quoted above. I reviewed and ran every command myself, read and understood the resulting diff line by line, and can explain and maintain every change in this PR.Checklist
CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.any N/A checks above.