fix: address codex review round 3
- spill-policy enforces the true cap invariant: it never emits a replacement larger than maxInlineBytes. When the notice alone exceeds the cap (tiny cap or long spill root) there is no within-cap replacement, so the inline result is kept — the previous guard only compared against the original size and could still return content over the cap for a large original. A within-cap replacement is always smaller than the original, so this subsumes the earlier check. - Add the HMR-disposal test the conventions require for a new registration: dispose the plugin fiber and assert oversized results stop being transformed and nothing more is spilled (no leaked tools/post-execute listener on reload).
This commit is contained in:
@@ -24,7 +24,7 @@ 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.)
|
(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.
|
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 would exceed `maxInlineBytes`, the policy keeps the inline result — it never emits a replacement over the cap (and a within-cap replacement is always smaller than the original, so this also means spilling never adds 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.
|
**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.
|
||||||
|
|
||||||
|
|||||||
@@ -157,12 +157,15 @@ export function apply(ctx: Context, config: Config): void {
|
|||||||
const { text: previewText, omitted } = preview(text, previewBudget)
|
const { text: previewText, omitted } = preview(text, previewBudget)
|
||||||
const notice = spillNotice(omitted, path)
|
const notice = spillNotice(omitted, path)
|
||||||
const replacedText = previewText.length > 0 ? `${previewText}\n\n${notice}` : notice
|
const replacedText = previewText.length > 0 ? `${previewText}\n\n${notice}` : notice
|
||||||
// Guard against a pathological tiny cap + long path where even the
|
// Invariant: the policy NEVER emits a replacement larger than the cap. When
|
||||||
// notice-only replacement is not smaller than the original: spilling then
|
// the notice alone exceeds maxInlineBytes (a tiny cap or a long spill root),
|
||||||
// gains nothing and would only add bytes, so keep the inline result. (The
|
// there is no within-cap replacement, so keep the inline result — spilling
|
||||||
// spill file already written is a harmless orphan; cleanup is deferred.)
|
// would break the advertised context cap. (A within-cap replacement is
|
||||||
if (Buffer.byteLength(replacedText, 'utf8') >= totalBytes) {
|
// always smaller than the original, which is > cap by the entry condition,
|
||||||
ctx.logger.warn(`spill-policy: spill notice for ${exec.name} is not smaller than the result; keeping the inline result`)
|
// so this one check subsumes "not smaller than the original" too. The spill
|
||||||
|
// file already written is a harmless orphan; cleanup is deferred.)
|
||||||
|
if (Buffer.byteLength(replacedText, 'utf8') > maxInlineBytes) {
|
||||||
|
ctx.logger.warn(`spill-policy: spill notice for ${exec.name} exceeds maxInlineBytes; keeping the inline result`)
|
||||||
return decision
|
return decision
|
||||||
}
|
}
|
||||||
const replaced: ContentBlock[] = [{ type: 'text', text: replacedText }]
|
const replaced: ContentBlock[] = [{ type: 'text', text: replacedText }]
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ function exec(name: string, session = 's1'): ToolExecution {
|
|||||||
* Build a context with tools + the policy, and optionally a spill backend.
|
* Build a context with tools + the policy, and optionally a spill backend.
|
||||||
* Returns the context and the backend handle (undefined when `withSpill` false).
|
* Returns the context and the backend handle (undefined when `withSpill` false).
|
||||||
*/
|
*/
|
||||||
async function setup(config: SpillPolicy.Config, withSpill = true): Promise<{ ctx: Context; spill?: StubSpill }> {
|
async function setup(config: SpillPolicy.Config, withSpill = true): Promise<{ ctx: Context; spill?: StubSpill; fiber: Awaited<ReturnType<Context['plugin']>> }> {
|
||||||
const ctx = new Context()
|
const ctx = new Context()
|
||||||
await ctx.plugin(SystemPrompt)
|
await ctx.plugin(SystemPrompt)
|
||||||
await ctx.plugin(ToolRegistry)
|
await ctx.plugin(ToolRegistry)
|
||||||
@@ -62,8 +62,8 @@ async function setup(config: SpillPolicy.Config, withSpill = true): Promise<{ ct
|
|||||||
await ctx.plugin(StubSpill)
|
await ctx.plugin(StubSpill)
|
||||||
spill = ctx.spillFiles as StubSpill
|
spill = ctx.spillFiles as StubSpill
|
||||||
}
|
}
|
||||||
await ctx.plugin(SpillPolicy, config)
|
const fiber = await ctx.plugin(SpillPolicy, config)
|
||||||
return { ctx, ...spill ? { spill } : {} }
|
return { ctx, fiber, ...spill ? { spill } : {} }
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Flatten a result's text blocks. */
|
/** Flatten a result's text blocks. */
|
||||||
@@ -118,9 +118,9 @@ describe('oversized plain-text replacement', () => {
|
|||||||
expect(Buffer.byteLength(text, 'utf8')).toBeLessThan(body.length)
|
expect(Buffer.byteLength(text, 'utf8')).toBeLessThan(body.length)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('keeps the inline result when even the notice-only replacement is not smaller', async () => {
|
it('keeps the inline result when the notice-only replacement would exceed the cap', async () => {
|
||||||
// A body just over a tiny cap: the notice alone is larger than the result,
|
// A body just over a tiny cap: the notice alone is larger than the cap, so
|
||||||
// so spilling would only add bytes — the policy keeps the inline result.
|
// there is no within-cap replacement — the policy keeps the inline result.
|
||||||
const { ctx } = await setup({ maxInlineBytes: 4 })
|
const { ctx } = await setup({ maxInlineBytes: 4 })
|
||||||
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {})
|
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {})
|
||||||
const body = 'xxxxx' // 5 bytes > 4, but far shorter than the notice
|
const body = 'xxxxx' // 5 bytes > 4, but far shorter than the notice
|
||||||
@@ -198,7 +198,7 @@ describe('best-effort fallback', () => {
|
|||||||
|
|
||||||
describe('composition', () => {
|
describe('composition', () => {
|
||||||
it('bounds content a downstream post-execute listener replaced', async () => {
|
it('bounds content a downstream post-execute listener replaced', async () => {
|
||||||
const { ctx, spill } = await setup({ maxInlineBytes: 10 })
|
const { ctx, spill } = await setup({ maxInlineBytes: 200 })
|
||||||
// A later-registered listener replaces the (small) tool result with a big one;
|
// A later-registered listener replaces the (small) tool result with a big one;
|
||||||
// the policy delegated via next(), so it bounds the replacement.
|
// the policy delegated via next(), so it bounds the replacement.
|
||||||
ctx.on('tools/post-execute', async (_e, _r, _next) =>
|
ctx.on('tools/post-execute', async (_e, _r, _next) =>
|
||||||
@@ -210,7 +210,7 @@ describe('composition', () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
it('preserves a downstream accept decision additionalContext when spilling', async () => {
|
it('preserves a downstream accept decision additionalContext when spilling', async () => {
|
||||||
const { ctx } = await setup({ maxInlineBytes: 10 })
|
const { ctx } = await setup({ maxInlineBytes: 200 })
|
||||||
const context = { content: [{ type: 'text' as const, text: 'note' }], source: { kind: 'plugin' as const, plugin: 'test' } }
|
const context = { content: [{ type: 'text' as const, text: 'note' }], source: { kind: 'plugin' as const, plugin: 'test' } }
|
||||||
ctx.on('tools/post-execute', async (_e, _r, _next) =>
|
ctx.on('tools/post-execute', async (_e, _r, _next) =>
|
||||||
({ kind: 'accept', additionalContext: context }))
|
({ kind: 'accept', additionalContext: context }))
|
||||||
@@ -220,3 +220,38 @@ describe('composition', () => {
|
|||||||
expect(result.additionalContext).toEqual(context)
|
expect(result.additionalContext).toEqual(context)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe('cap invariant', () => {
|
||||||
|
it('keeps the inline result when the notice alone exceeds the cap, even for a large original', async () => {
|
||||||
|
// A large body (so it is well over the cap) but a cap smaller than the
|
||||||
|
// notice itself: there is no within-cap replacement, so the policy must keep
|
||||||
|
// the inline result rather than emit content over maxInlineBytes.
|
||||||
|
const { ctx } = await setup({ maxInlineBytes: 8 })
|
||||||
|
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {})
|
||||||
|
const body = 'x'.repeat(5000)
|
||||||
|
ctx.tools.register(textTool('big', body))
|
||||||
|
const result = await ctx.tools.execute(exec('big'))
|
||||||
|
expect(textOf(result.content)).toBe(body)
|
||||||
|
expect(warn).toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
describe('disposal (HMR safety)', () => {
|
||||||
|
it('stops transforming oversized results after the plugin fiber is disposed', async () => {
|
||||||
|
const { ctx, spill, fiber } = await setup({ maxInlineBytes: 200 })
|
||||||
|
const body = 'HEAD'.repeat(200) + 'TAIL'.repeat(200)
|
||||||
|
ctx.tools.register(textTool('big', body))
|
||||||
|
|
||||||
|
// Live: the listener spills and replaces.
|
||||||
|
const before = await ctx.tools.execute(exec('big'))
|
||||||
|
expect(textOf(before.content)).toContain('Full formatted result saved to')
|
||||||
|
expect(spill?.saves).toHaveLength(1)
|
||||||
|
|
||||||
|
// After disposal the listener is gone — the result passes through untouched
|
||||||
|
// and nothing more is spilled (no leaked registration across reload).
|
||||||
|
await fiber.dispose()
|
||||||
|
const after = await ctx.tools.execute(exec('big'))
|
||||||
|
expect(textOf(after.content)).toBe(body)
|
||||||
|
expect(spill?.saves).toHaveLength(1)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user