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. 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 { 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 = ""; }