fix: address Codex review on PR #3604

This commit is contained in:
netcatty-bot committed 2026-10-08 11:03:33 +00:00
1 parent 5e067eac06
commit 956b262e1f
5 files changed
+101 -6

No files matched your search

+4 -1
View File
@@ -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
+16
View File
@@ -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']);
});
+9 -1
View File
@@ -186,8 +186,15 @@ export function planSessionFork<M extends ForkMessageLike>(
* `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<string>();
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;
}
@@ -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<string, { record: PersistedToolOutputRecord; content: string }>();
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<string, { record: PersistedToolOutputRecord; content: string }>();
const deletedPaths: string[] = [];
+33 -4
View File
@@ -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<T extends { handle: ToolOutputHandle }>(entries: ReadonlyArray<T>): T | undefined {
const borrowedPaths = new Set<string>();
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) {