From 326b199f255c61647c190286960dd974488a70ee Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Thu, 9 Jul 2026 09:51:35 +0800 Subject: [PATCH] fix: address codex review round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - spill-policy reserves the spill notice's byte cost inside maxInlineBytes, so the replacement (preview + notice) never exceeds the documented model-facing cap. When the notice alone fills the budget the preview is empty; when even a notice-only replacement is not smaller than the original, the inline result is kept (spilling would only add bytes). - retention TextRetainer trims an oversized single suffix chunk to the last suffixCap bytes on push, so tail/headTail retention stays bounded by suffixCap instead of retaining and re-copying the whole chunk in finish() — this is the spill preview path, which pushes the whole result as one chunk. --- packages/spill/spill-policy/README.md | 4 +- packages/spill/spill-policy/src/index.ts | 46 +++++++++++++------ .../spill-policy/tests/spill-policy.spec.ts | 22 +++++++-- packages/util/retention/src/index.ts | 13 ++++++ packages/web/tool-web/tests/spill.spec.ts | 8 ++-- 5 files changed, 71 insertions(+), 22 deletions(-) diff --git a/packages/spill/spill-policy/README.md b/packages/spill/spill-policy/README.md index 24e720c5c8..dcc2fffb30 100644 --- a/packages/spill/spill-policy/README.md +++ b/packages/spill/spill-policy/README.md @@ -16,7 +16,7 @@ This plugin registers **no service** and owns no storage or preview mechanics: p 2. Skip `read` (avoids a `read → spill file → read again` loop) and any non-`accept` decision (a `block`'s corrective feedback passes through). 3. Flatten the accepted content only when it is **plain text** (all `text` blocks); a result with any non-text block is left untouched. 4. If its UTF-8 size is `≤ maxInlineBytes`, leave it unchanged. -5. Otherwise save the full text and replace the result with a preview + this notice: +5. Otherwise save the full text and replace the result with a preview + this notice, sized so the whole replacement (preview + blank line + notice) stays within `maxInlineBytes` — the notice's byte cost is reserved out of the budget, so the preview shrinks to fit and the model-facing result never exceeds the cap: ```text @@ -24,6 +24,8 @@ This plugin registers **no service** and owns no storage or preview mechanics: p (Omitted N bytes. Full formatted result saved to: /…/session-…/…-web_fetch.txt. Use read with offset/limit to inspect it.) ``` + When the notice alone fills the budget (a tiny cap or a long path) the preview is empty and only the notice is returned. If even that notice-only replacement is not smaller than the original result, the policy keeps the inline result — spilling would only add bytes. + **Best-effort:** no session owner, no `ctx.spillFiles` backend, or a `saveText` rejection ⇒ the policy logs a warning and returns the original result. A spill failure never turns a successful call into an `isError` or hides the inline result. ## Scope diff --git a/packages/spill/spill-policy/src/index.ts b/packages/spill/spill-policy/src/index.ts index 57c651d70c..9fa55fcdf4 100644 --- a/packages/spill/spill-policy/src/index.ts +++ b/packages/spill/spill-policy/src/index.ts @@ -76,25 +76,20 @@ function ownerSessionId(exec: ToolExecution): SessionId | undefined { return (exec as SpillPolicyExec).agent?.session.header.id } -/** Build the bounded head/tail preview for `text`, splitting `maxInlineBytes` across the two ends. */ -function preview(text: string, maxInlineBytes: number): { text: string; omitted: Omitted } { - const headBytes = Math.ceil(maxInlineBytes / 2) - const tailBytes = Math.floor(maxInlineBytes / 2) +/** Build the bounded head/tail preview for `text`, splitting `budget` bytes across the two ends. */ +function preview(text: string, budget: number): { text: string; omitted: Omitted } { + const headBytes = Math.ceil(budget / 2) + const tailBytes = Math.floor(budget / 2) const retainer = new TextRetainer({ kind: 'headTail', headBytes, tailBytes }) retainer.push(text) const kept = retainer.finish() return { text: kept.text, omitted: kept.omittedBytes } } -/** - * Compose the replacement text: the bounded preview, a blank line, then the - * spill notice. The omission clause comes from the retention library - * (`describeOmitted`); the recovery sentence names the concrete spill path. - */ -function replacementText(previewText: string, omitted: Omitted, spillPath: string): string { +/** The spill-notice line for a given omission + path (no preview, no leading blank line). */ +function spillNotice(omitted: Omitted, spillPath: string): string { const omission = describeOmitted(omitted, 'bytes') - const notice = `(${omission} Full formatted result saved to: ${spillPath}. Use read with offset/limit to inspect it.)` - return `${previewText}\n\n${notice}` + return `(${omission} Full formatted result saved to: ${spillPath}. Use read with offset/limit to inspect it.)` } export function apply(ctx: Context, config: Config): void { @@ -119,7 +114,8 @@ export function apply(ctx: Context, config: Config): void { const content = decision.content ?? result.content const text = flattenPlainText(content) if (text === undefined) return decision - if (Buffer.byteLength(text, 'utf8') <= maxInlineBytes) return decision + const totalBytes = Buffer.byteLength(text, 'utf8') + if (totalBytes <= maxInlineBytes) return decision const sessionId = ownerSessionId(exec) if (sessionId === undefined) { @@ -148,8 +144,28 @@ export function apply(ctx: Context, config: Config): void { return decision } - const { text: previewText, omitted } = preview(text, maxInlineBytes) - const replaced: ContentBlock[] = [{ type: 'text', text: replacementText(previewText, omitted, path) }] + // Reserve the notice's byte cost INSIDE maxInlineBytes so the replacement + // (preview + blank line + notice) never exceeds the documented cap — a naive + // preview that spent the whole budget then appended the notice could be + // larger than the cap, and for a marginally-over result even larger than the + // original. The reservation uses a notice priced at the worst-case omission + // count (the full byte total): its digit count bounds the real count's, so + // the reserved size is a safe upper bound and the final notice is never + // longer than what we reserved. `\n\n` is the 2-byte join. + const reserve = Buffer.byteLength(spillNotice({ kind: 'exact', count: totalBytes }, path), 'utf8') + 2 + const previewBudget = Math.max(0, maxInlineBytes - reserve) + const { text: previewText, omitted } = preview(text, previewBudget) + const notice = spillNotice(omitted, path) + const replacedText = previewText.length > 0 ? `${previewText}\n\n${notice}` : notice + // Guard against a pathological tiny cap + long path where even the + // notice-only replacement is not smaller than the original: spilling then + // gains nothing and would only add bytes, so keep the inline result. (The + // spill file already written is a harmless orphan; cleanup is deferred.) + if (Buffer.byteLength(replacedText, 'utf8') >= totalBytes) { + ctx.logger.warn(`spill-policy: spill notice for ${exec.name} is not smaller than the result; keeping the inline result`) + return decision + } + const replaced: ContentBlock[] = [{ type: 'text', text: replacedText }] return { kind: 'accept', content: replaced, ...decision.additionalContext ? { additionalContext: decision.additionalContext } : {} } }) } diff --git a/packages/spill/spill-policy/tests/spill-policy.spec.ts b/packages/spill/spill-policy/tests/spill-policy.spec.ts index 61da0abe76..dd6235a9a3 100644 --- a/packages/spill/spill-policy/tests/spill-policy.spec.ts +++ b/packages/spill/spill-policy/tests/spill-policy.spec.ts @@ -93,9 +93,9 @@ describe('config validation', () => { }) describe('oversized plain-text replacement', () => { - it('spills the full text and replaces the result with a preview + path', async () => { - const { ctx, spill } = await setup({ maxInlineBytes: 20 }) - const body = 'HEAD'.repeat(20) + 'TAIL'.repeat(20) // 160 bytes > 20 + it('spills the full text and replaces the result with a preview + path within the cap', async () => { + const { ctx, spill } = await setup({ maxInlineBytes: 200 }) + const body = 'HEAD'.repeat(200) + 'TAIL'.repeat(200) // 1600 bytes > 200 ctx.tools.register(textTool('big', body)) const result = await ctx.tools.execute(exec('big')) @@ -112,6 +112,22 @@ describe('oversized plain-text replacement', () => { expect(text).toContain('Full formatted result saved to: /spill/big.txt') expect(text).toContain('Use read with offset/limit') expect(text).toContain('Omitted') + // The replacement (preview + blank line + notice) stays within the cap and + // is smaller than the original — the whole point of spilling. + expect(Buffer.byteLength(text, 'utf8')).toBeLessThanOrEqual(200) + expect(Buffer.byteLength(text, 'utf8')).toBeLessThan(body.length) + }) + + it('keeps the inline result when even the notice-only replacement is not smaller', async () => { + // A body just over a tiny cap: the notice alone is larger than the result, + // so spilling would only add bytes — the policy keeps the inline result. + const { ctx } = await setup({ maxInlineBytes: 4 }) + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {}) + const body = 'xxxxx' // 5 bytes > 4, but far shorter than the notice + ctx.tools.register(textTool('big', body)) + const result = await ctx.tools.execute(exec('big')) + expect(textOf(result.content)).toBe(body) + expect(warn).toHaveBeenCalled() }) it('leaves a small plain-text result unchanged', async () => { diff --git a/packages/util/retention/src/index.ts b/packages/util/retention/src/index.ts index f0f40ba7af..8c3b924a16 100644 --- a/packages/util/retention/src/index.ts +++ b/packages/util/retention/src/index.ts @@ -355,6 +355,19 @@ export class TextRetainer { this.suffixHeld -= head.length head = this.suffixChunks[0] } + // The head chunk can still hold leading bytes beyond the last `suffixCap` + // — a single chunk LARGER than the window is retained whole by the loop + // above (dropping the only chunk would leave < cap). Trim those leading + // bytes so the accumulator (and finish()'s concat) stays bounded by + // `suffixCap` instead of allocating/copying the full chunk again; + // finish() only ever reads the last `suffixLen ≤ suffixCap` bytes, so this + // drops nothing it would return. (head.length > excess by the loop + // invariant `suffixHeld - head.length < suffixCap`, so the slice is non-empty.) + if (head !== undefined && this.suffixHeld > this.suffixCap) { + const excess = this.suffixHeld - this.suffixCap + this.suffixChunks[0] = head.subarray(excess) + this.suffixHeld -= excess + } } // Dropped = bytes that no side can keep. Compute cumulative omission the diff --git a/packages/web/tool-web/tests/spill.spec.ts b/packages/web/tool-web/tests/spill.spec.ts index 6e2ce6d11d..e44cbb7b45 100644 --- a/packages/web/tool-web/tests/spill.spec.ts +++ b/packages/web/tool-web/tests/spill.spec.ts @@ -33,7 +33,8 @@ let handler: Handler let spillRoot: string let ctx: Context -const BODY = 'X'.repeat(4000) // formatted result is well over the 200-byte policy cap +const BODY = 'X'.repeat(4000) // formatted result is well over the policy cap +const MAX_INLINE_BYTES = 1000 // leaves room for a head/tail preview beside the notice beforeEach(async () => { handler = (_req, res) => { res.writeHead(200, { 'content-type': 'text/plain' }); res.end(BODY) } @@ -50,7 +51,7 @@ beforeEach(async () => { // policy cap is what triggers the spill (the RFC's separation of concerns). await ctx.plugin(WebFetchLocal, { maxBodyChars: 500_000 }) await ctx.plugin(LocalSpillFiles, { root: spillRoot }) - await ctx.plugin(SpillPolicy, { maxInlineBytes: 200 }) + await ctx.plugin(SpillPolicy, { maxInlineBytes: MAX_INLINE_BYTES }) await ctx.plugin(ToolWeb) }) @@ -72,8 +73,9 @@ describe('web_fetch spill showcase', () => { expect(out.isError).toBe(false) const text = out.content.map(b => b.text).join('') - // Model-facing text is a preview + notice, NOT the full body. + // Model-facing text is a preview + notice within the cap, NOT the full body. expect(text.length).toBeLessThan(BODY.length) + expect(Buffer.byteLength(text, 'utf8')).toBeLessThanOrEqual(MAX_INLINE_BYTES) expect(text).toContain(`Fetched ${base}`) // the head of the formatted result survives expect(text).toContain('Full formatted result saved to:') expect(text).toContain('Use read with offset/limit')