From 956b262e1f145cd658c9faf8e1343fd07768f772 Mon Sep 17 00:00:00 2001 From: netcatty-bot <308658023+netcatty-bot@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:03:13 +0000 Subject: [PATCH] fix: address Codex review on PR #3604 --- application/state/useAIState.ts | 5 ++- domain/aiSessionFork.test.ts | 16 ++++++++ domain/aiSessionFork.ts | 10 ++++- .../ai/harness/toolOutputStore.test.ts | 39 +++++++++++++++++++ infrastructure/ai/harness/toolOutputStore.ts | 37 ++++++++++++++++-- 5 files changed, 101 insertions(+), 6 deletions(-) diff --git a/application/state/useAIState.ts b/application/state/useAIState.ts index f2b31293b..e03b2000d 100644 --- a/application/state/useAIState.ts +++ b/application/state/useAIState.ts @@ -732,10 +732,13 @@ export function useAIState() { // configure it here too or retained handles cannot be restored from // durable storage before rehoming. await installToolOutputPersistence(store); + // A carried-over compaction summary can reference archive handles not + // present in any retained message; collect those too so the summary + // stays readable via tool_output_read in the fork. await store.rehomeChatSession( sessionId, fork.id, - collectForkHandleIds(plan.messages), + collectForkHandleIds(plan.messages, plan.contextCompaction?.summary), ); } catch { // Rehoming is best-effort; the fork is still usable (reads fall back diff --git a/domain/aiSessionFork.test.ts b/domain/aiSessionFork.test.ts index 948e79789..1cb1c0d80 100644 --- a/domain/aiSessionFork.test.ts +++ b/domain/aiSessionFork.test.ts @@ -4,6 +4,7 @@ import test from 'node:test'; import { buildForkTitle, canForkFromMessage, + collectForkHandleIds, planSessionFork, stripMessageContinuationState, } from './aiSessionFork.ts'; @@ -219,3 +220,18 @@ test('canForkFromMessage mirrors planSessionFork success', () => { assert.equal(canForkFromMessage(source, source.messages[0].id), false); assert.equal(canForkFromMessage(source, 'nope'), false); }); + +test('collectForkHandleIds also scans a carried-over compaction summary', () => { + const messages: ForkMessage[] = [ + msg({ role: 'user', content: 'continue from the summary' }), + msg({ content: 'noted, handleId=tool-output-abc123 is the live one' }), + ]; + const summary = 'Earlier turn archived locally: handleId=tool-output-xyz789. ' + + 'Uses handleId=tool-output-abc123 again.'; + assert.deepEqual( + collectForkHandleIds(messages, summary), + ['tool-output-abc123', 'tool-output-xyz789'], + ); + // No summary: unchanged behavior. + assert.deepEqual(collectForkHandleIds(messages), ['tool-output-abc123']); +}); diff --git a/domain/aiSessionFork.ts b/domain/aiSessionFork.ts index c28250041..3af33bf2c 100644 --- a/domain/aiSessionFork.ts +++ b/domain/aiSessionFork.ts @@ -186,8 +186,15 @@ export function planSessionFork( * `handleId=tool-output-…`. Collect the ids the retained prefix references so * the fork's handles can be rehomed/restored into the new session's namespace * (keeps the notices valid for `tool_output_read` after the fork). + * + * A carried-over compaction summary can also reference archive handles that + * no longer appear in any retained message (compaction replaces all earlier + * messages with the summary), so scan the optional summary text too. */ -export function collectForkHandleIds(messages: readonly ForkMessageLike[]): string[] { +export function collectForkHandleIds( + messages: readonly ForkMessageLike[], + summary?: string, +): string[] { const ids: string[] = []; const seen = new Set(); const scan = (value: string | undefined) => { @@ -203,6 +210,7 @@ export function collectForkHandleIds(messages: readonly ForkMessageLike[]): stri scan(message.thinking); for (const result of message.toolResults ?? []) scan(result.content); } + scan(summary); return ids; } diff --git a/infrastructure/ai/harness/toolOutputStore.test.ts b/infrastructure/ai/harness/toolOutputStore.test.ts index d98beef51..0a8c39ef4 100644 --- a/infrastructure/ai/harness/toolOutputStore.test.ts +++ b/infrastructure/ai/harness/toolOutputStore.test.ts @@ -203,6 +203,45 @@ test('ToolOutputStore rehome falls back to a non-owning alias of the source spil assert.ok(files.has(`/netcatty/${handle.id}-chat-source.log`)); }); +test('ToolOutputStore global quotas evict the borrowed alias, not the owner of its spill path', async () => { + const files = new Map(); + const deletedPaths: string[] = []; + const persistence: ToolOutputPersistence = { + write: async (record, content) => { + const path = `/netcatty/${record.handleId}-${record.chatSessionId}.log`; + files.set(path, { record, content }); + return path; + }, + read: async () => null, + delete: async path => { + deletedPaths.push(path); + files.delete(path); + }, + }; + + // The global handle quota is already full when the fork is created, so the + // alias pushed the registry over the limit: the owner of the borrowed path + // must survive, the alias itself is the right thing to evict. + const original = new ToolOutputStore({ spillThresholdChars: 0, maxHandlesGlobal: 1, persistence }); + const handle = original.store({ + chatSessionId: 'chat-source', + capabilityId: 'terminal.execute', + content: 'C'.repeat(30_000), + }); + await handle.spillPromise; + + // `read` cannot serve the durable content, so the clone aliases the + // source-owned spill path as a non-owning borrow. + await original.rehomeChatSession('chat-source', 'chat-fork'); + + const sourceCopy = original.get(handle.id, 'chat-source'); + assert.ok(sourceCopy); + assert.ok(files.has(`/netcatty/${handle.id}-chat-source.log`)); + assert.equal(deletedPaths.length, 0); + // The alias was evicted to satisfy the quota. + assert.equal(original.get(handle.id, 'chat-fork'), undefined); +}); + test('ToolOutputStore respilled forked copies clear the borrowed flag so eviction frees their file', async () => { const files = new Map(); const deletedPaths: string[] = []; diff --git a/infrastructure/ai/harness/toolOutputStore.ts b/infrastructure/ai/harness/toolOutputStore.ts index b2857ed64..c1456a119 100644 --- a/infrastructure/ai/harness/toolOutputStore.ts +++ b/infrastructure/ai/harness/toolOutputStore.ts @@ -642,10 +642,10 @@ export class ToolOutputStore { sessionMap.size > this.maxHandlesPerSession || totalChars() > this.maxCharsPerSession ) { - const oldest = [...sessionMap.values()].sort((a, b) => a.accessedAt - b.accessedAt)[0]; + const oldest = this.pickEvictionEntry([...sessionMap.values()].map(handle => ({ handle }))); if (!oldest) break; - sessionMap.delete(oldest.id); - this.evictHandle(oldest); + sessionMap.delete(oldest.handle.id); + this.evictHandle(oldest.handle); } if (sessionMap.size === 0) this.bySession.delete(chatSessionId); } @@ -670,7 +670,7 @@ export class ToolOutputStore { const entries = allHandles(); const totalChars = entries.reduce((sum, entry) => sum + entry.handle.storedChars, 0); if (entries.length <= this.maxHandlesGlobal && totalChars <= this.maxCharsGlobal) break; - const oldest = entries.sort((a, b) => a.handle.accessedAt - b.handle.accessedAt)[0]; + const oldest = this.pickEvictionEntry(entries); if (!oldest) break; oldest.sessionMap.delete(oldest.handle.id); this.evictHandle(oldest.handle); @@ -678,6 +678,35 @@ export class ToolOutputStore { } } + /** + * Pick the next handle to evict: a handle owning a durable spill path that + * other handles borrow as a non-owning alias must stay in memory while the + * borrower does — evicting the owner would delete the very file the alias + * points at, leaving that alias unreadable. Prefer the oldest borrowed + * alias instead (evicting an alias never deletes a file) before falling + * back to the oldest handle overall. + */ + private pickEvictionEntry(entries: ReadonlyArray): T | undefined { + const borrowedPaths = new Set(); + for (const sessionMap of this.bySession.values()) { + for (const handle of sessionMap.values()) { + if (handle.borrowedFilePath && handle.filePath) borrowedPaths.add(handle.filePath); + } + } + const candidates = borrowedPaths.size + ? entries.filter(entry => !( + entry.handle.filePath + && !entry.handle.borrowedFilePath + && borrowedPaths.has(entry.handle.filePath) + )) + : entries; + let oldest: T | undefined; + for (const entry of candidates) { + if (!oldest || entry.handle.accessedAt < oldest.handle.accessedAt) oldest = entry; + } + return oldest; + } + private evictHandle(handle: ToolOutputHandle): void { handle.evicted = true; if (handle.filePath && !handle.borrowedFilePath && this.persistence) {