From 8aa6b553551d3d4e233477ff21cf55cbe7523e80 Mon Sep 17 00:00:00 2001 From: teyrebaz33 Date: Mon, 17 Aug 2026 04:26:12 +0300 Subject: [PATCH 1/3] fix(harness): truncation marker could push output past its char budget 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. --- .../src/core/collector/normalizer.test.ts | 22 +++++++++++++---- .../harness/src/core/collector/normalizer.ts | 24 +++++++++++++++++-- .../harness/src/core/record-archive.test.ts | 12 +++++++--- packages/harness/src/core/record-archive.ts | 22 +++++++++++++++-- 4 files changed, 68 insertions(+), 12 deletions(-) diff --git a/packages/harness/src/core/collector/normalizer.test.ts b/packages/harness/src/core/collector/normalizer.test.ts index 5aa7361eb..9860bbf38 100644 --- a/packages/harness/src/core/collector/normalizer.test.ts +++ b/packages/harness/src/core/collector/normalizer.test.ts @@ -150,10 +150,22 @@ describe("truncateForPayload", () => { expect(truncateForPayload({ a: 1 })).toBe(JSON.stringify({ a: 1 })); }); - it("truncates long strings with a marker", () => { - const long = "y".repeat(20); - const result = truncateForPayload(long, 10); - expect(result.startsWith("y".repeat(10))).toBe(true); - expect(result).toContain("[truncated 10 chars]"); + it("truncates long strings with a marker, bounding the total length", () => { + const long = "y".repeat(200); + const result = truncateForPayload(long, 50); + // The marker's own length has to come out of the 50-char budget too, so + // less than 50 chars of content survive -- the total (content + marker) + // is what's bounded, not the content alone. + expect(result).toBe(`${"y".repeat(28)}…[truncated 172 chars]`); + expect(result.length).toBe(50); + }); + + it("still respects the bound when maxLength is smaller than the marker itself", () => { + // Degenerate case: production budgets (MAX_FIELD_LENGTH, MAX_TOOL_RESPONSE_LENGTH) + // are always far larger than a marker, but the bound has to hold even + // here -- there's no content left to keep, so the marker itself gets + // clipped too, rather than the output exceeding maxLength. + const result = truncateForPayload("y".repeat(20), 10); + expect(result.length).toBe(10); }); }); diff --git a/packages/harness/src/core/collector/normalizer.ts b/packages/harness/src/core/collector/normalizer.ts index 14250fa91..b1e66981e 100644 --- a/packages/harness/src/core/collector/normalizer.ts +++ b/packages/harness/src/core/collector/normalizer.ts @@ -50,11 +50,31 @@ const MAX_FIELD_LENGTH = 4000; /** Tool output can be much larger than other fields; cap it separately. */ const MAX_TOOL_RESPONSE_LENGTH = 16 * 1024; -/** Stringify + truncate a value so a giant tool payload can't blow up storage. */ +/** + * Stringify + truncate a value so a giant tool payload can't blow up storage. + * Bounds the TOTAL output (content + marker) to `maxLength`: 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 converges over a few passes + * the same way {@link clip} in record-archive.ts does (kept in sync with it + * intentionally — both write the same {@link PAYLOAD_TRUNCATION_MARKER} + * format and both need the same fix for the same reason). + */ export function truncateForPayload(value: unknown, maxLength = MAX_FIELD_LENGTH): string { const str = typeof value === "string" ? value : JSON.stringify(value ?? null); if (str.length <= maxLength) return str; - return `${str.slice(0, maxLength)}…[truncated ${str.length - maxLength} chars]`; + let sliceLen = maxLength; + for (let i = 0; i < 5; i++) { + const dropped = str.length - sliceLen; + const marker = `…[truncated ${dropped} chars]`; + const nextSliceLen = Math.max(0, maxLength - marker.length); + if (nextSliceLen === sliceLen) { + return `${str.slice(0, sliceLen)}${marker}`.slice(0, maxLength); + } + sliceLen = nextSliceLen; + } + const dropped = str.length - sliceLen; + const marker = `…[truncated ${dropped} chars]`; + return `${str.slice(0, sliceLen)}${marker}`.slice(0, maxLength); } /** diff --git a/packages/harness/src/core/record-archive.test.ts b/packages/harness/src/core/record-archive.test.ts index 190258959..61843d447 100644 --- a/packages/harness/src/core/record-archive.test.ts +++ b/packages/harness/src/core/record-archive.test.ts @@ -79,7 +79,10 @@ describe("compactSessionRecord", () => { const call = compacted.turns[0].toolCalls[0]; expect(call.input).toMatch(/…\[truncated \d+ chars\]$/); expect(call.input?.length).toBeLessThan(200); - expect(call.responseSummary).toBe(`${"y".repeat(100)}…[truncated 3900 chars]`); + // Bounded to maxToolResponseChars total (content + marker), not content + // alone -- 100 chars minus the marker's own length leaves 77 y's here. + expect(call.responseSummary).toBe(`${"y".repeat(77)}…[truncated 3923 chars]`); + expect(call.responseSummary?.length).toBe(100); // A result this pass shortened is truncated, whoever shortened it. expect(call.responseTruncated).toBe(true); // The conversation itself is untouched — that's the part worth archiving. @@ -111,10 +114,13 @@ describe("compactSessionRecord", () => { { maxToolResponseChars: 100 }, ); - // 300 - 100 taken here, plus the 1000 the collector had already dropped. + // 300 z's clipped to 77 (100-char budget minus the marker's own length), + // plus the 1000 the collector had already dropped: 223 newly dropped + // here + 1000 folded in = 1223. expect(compacted.turns[0].toolCalls[0].responseSummary).toBe( - `${"z".repeat(100)}…[truncated 1200 chars]`, + `${"z".repeat(77)}…[truncated 1223 chars]`, ); + expect(compacted.turns[0].toolCalls[0].responseSummary?.length).toBe(100); }); it("claims no compaction when nothing was actually clipped", () => { diff --git a/packages/harness/src/core/record-archive.ts b/packages/harness/src/core/record-archive.ts index 473da5fa2..aa381e472 100644 --- a/packages/harness/src/core/record-archive.ts +++ b/packages/harness/src/core/record-archive.ts @@ -157,9 +157,27 @@ function clip(value: string | null, maxChars: number): string | null { if (value === null) return null; const marker = PAYLOAD_TRUNCATION_MARKER.exec(value); const body = marker ? value.slice(0, value.length - marker[0].length) : value; + const alreadyOmitted = marker ? Number(marker[1]) : 0; if (body.length <= maxChars) return value; - const omitted = body.length - maxChars + (marker ? Number(marker[1]) : 0); - return `${body.slice(0, maxChars)}…[truncated ${omitted} chars]`; + // Same converging-slice fix as truncateForPayload, plus the unwrap step + // above: the omitted count folds in whatever a prior pass already cut, and + // idempotency holds because the fixed point always lands the body at + // exactly `maxChars - newMarker.length`, which a second pass's unwrap step + // reproduces exactly (see truncateForPayload's doc comment for why this + // needs a loop instead of a single subtraction). + let sliceLen = maxChars; + for (let i = 0; i < 5; i++) { + const omitted = body.length - sliceLen + alreadyOmitted; + const newMarker = `…[truncated ${omitted} chars]`; + const nextSliceLen = Math.max(0, maxChars - newMarker.length); + if (nextSliceLen === sliceLen) { + return `${body.slice(0, sliceLen)}${newMarker}`.slice(0, maxChars); + } + sliceLen = nextSliceLen; + } + const omitted = body.length - sliceLen + alreadyOmitted; + const newMarker = `…[truncated ${omitted} chars]`; + return `${body.slice(0, sliceLen)}${newMarker}`.slice(0, maxChars); } export interface CompactionOptions { From 52180c117f61cca1e4c293e61252cbb4dc72cf59 Mon Sep 17 00:00:00 2001 From: teyrebaz33 Date: Fri, 21 Aug 2026 19:58:15 +0300 Subject: [PATCH 2/3] fix(harness): resume-brief's clamp() had the same marker-budget overrun 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. --- .../harness/src/core/resume-brief.test.ts | 10 ++++++++ packages/harness/src/core/resume-brief.ts | 24 +++++++++++++++---- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/packages/harness/src/core/resume-brief.test.ts b/packages/harness/src/core/resume-brief.test.ts index 767f70989..7be7f5c05 100644 --- a/packages/harness/src/core/resume-brief.test.ts +++ b/packages/harness/src/core/resume-brief.test.ts @@ -261,6 +261,16 @@ describe("buildResumeBrief", () => { it("returns null for a tool call with no recorded input at all", () => { expect(describeToolTarget(toolCall("Bash", null as unknown as string, { input: null }), null)).toBeNull(); }); + + it("bounds a clamped target to MAX_TOOL_TARGET_CHARS total, not content alone", () => { + // A command longer than the 80-char cap forces describeToolTarget's + // internal clamp() to truncate. The returned string (content + marker) + // must not exceed the cap -- the marker's own length has to come out + // of the budget too. + const target = describeToolTarget(toolCall("Bash", { command: "x".repeat(200) }), "/Users/dev/project"); + expect(target?.length).toBeLessThanOrEqual(80); + expect(target).toContain("…[truncated]"); + }); }); describe("token budget", () => { diff --git a/packages/harness/src/core/resume-brief.ts b/packages/harness/src/core/resume-brief.ts index ddb27acbe..5c398a50b 100644 --- a/packages/harness/src/core/resume-brief.ts +++ b/packages/harness/src/core/resume-brief.ts @@ -85,7 +85,13 @@ const MAX_FILES = 30; const MAX_COMMANDS = 10; const MAX_COMMAND_CHARS = 120; -/** The marker `truncateForPayload` (core/collector/normalizer.ts) leaves. */ +/** + * Fixed marker `clamp` appends when it cuts text short. Deliberately distinct + * from `truncateForPayload`'s `…[truncated N chars]` (core/collector/normalizer.ts) + * — that one records a dropped-char count computed by the collector; this one + * has no count to report (the resume brief clamps for display, not storage + * accounting), so it stays a plain, fixed string. + */ const CLAMP_MARKER = "…[truncated]"; /** The workflow the prior session was bound to, resolved by the caller against @@ -120,12 +126,18 @@ export function estimateBriefTokens(text: string): number { return Math.ceil(text.length / CHARS_PER_TOKEN); } -/** Clamp `text` to `maxChars`, marking the cut so nothing reads as complete - * when it isn't. */ +/** + * Clamp `text` to `maxChars` TOTAL (content + marker), marking the cut so + * nothing reads as complete when it isn't. CLAMP_MARKER's length is fixed + * (unlike a "chars dropped" marker whose length depends on where the slice + * lands), so a single subtraction — not a converging loop — correctly + * reserves room for it within maxChars. + */ function clamp(text: string, maxChars: number): string { const trimmed = text.trim(); if (trimmed.length <= maxChars) return trimmed; - return `${trimmed.slice(0, maxChars).trimEnd()}${CLAMP_MARKER}`; + const sliceLen = Math.max(0, maxChars - CLAMP_MARKER.length); + return `${trimmed.slice(0, sliceLen).trimEnd()}${CLAMP_MARKER}`.slice(0, maxChars); } /** @@ -409,7 +421,9 @@ export function buildResumeBrief( // pass strictly shortens `summaryText`, so this terminates. while (summaryText.length > 0 && over()) { const overflow = assemble().length - budgetChars; - summaryText = clamp(summaryText, Math.max(0, summaryText.length - overflow - CLAMP_MARKER.length)); + // clamp() now self-enforces its own total-length bound, so no need to + // pre-subtract CLAMP_MARKER.length here the way this had to before. + summaryText = clamp(summaryText, Math.max(0, summaryText.length - overflow)); if (summaryText === CLAMP_MARKER) summaryText = ""; } From eb9a88c77882f3cdf559cfb98f5ddb0fd50ab491 Mon Sep 17 00:00:00 2001 From: teyrebaz33 Date: Fri, 21 Aug 2026 20:12:12 +0300 Subject: [PATCH 3/3] chore: add changeset for harness truncation marker budget fix --- .changeset/fix-harness-truncation-marker-budget.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fix-harness-truncation-marker-budget.md diff --git a/.changeset/fix-harness-truncation-marker-budget.md b/.changeset/fix-harness-truncation-marker-budget.md new file mode 100644 index 000000000..07839ac29 --- /dev/null +++ b/.changeset/fix-harness-truncation-marker-budget.md @@ -0,0 +1,5 @@ +--- +"@sapiom/harness": patch +--- + +Fix truncation helpers that could return output slightly longer than their configured character budget.