diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index e0082986..acbc3756 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -736,7 +736,7 @@ Matching semantics: a query containing `/` matches the relative path, otherwise ⚠️ **Images only through existing file surfaces.** A Bash image candidate counts only when the file's mtime lies in the call's window (start − 2 s … end + 2 s) and no entry already covers that file version; `SendUserFile` images need no window. The panel shows a thumbnail only inside the workspace, through the confined `file-raw` route. An image outside it gets no thumbnail (each would mint an attachment id per render); a click opens the shared file preview, which registers it once through `POST /api/sessions/:id/attachments` with `notify: false` (its extension allowlist and `isSensitivePath` apply). SVGs get no thumbnail. No route of this feature serves a file, least of all a transcript path. -⚠️ **Transcript paths are agent-printed, so they are probed BOUNDED and scanned in linear time** (#516; #693 review). `probeLocalPaths()` sends each candidate through `probePathKind()` (no `pastCap`: a bulk scan), one at a time, and only a path that answered `'file'` gets `realpath` + `stat`; a path on a mount that stopped answering costs one stalled stat, never a sync, a queued sync or `remove()` that cannot finish (`test/edit-history-probe-stall.test.ts`). `PATH_TOKEN_RE` runs synchronously in the request, over Bash output of up to 30,000 characters: an unquoted token may start only at a token boundary (line start or a token-ending character, `:` excluded so URLs stay ignored), and the `file://` branch stops at `:`. A lookbehind that merely excludes some characters lets a match start at every character of a long token and rescan it, quadratic in the line (2.7 s for 30,000 `#`). Time-budget regression tests in `test/edit-history-parse.test.ts`. "Load older" yields to the event loop between the files it parses (up to 3 × 8 MiB). +⚠️ **Transcript paths are agent-printed, so they are probed BOUNDED and scanned in linear time** (#516; #693 review). `probeLocalPaths()` sends each candidate through `probePathKind()` (no `pastCap`: a bulk scan), one at a time, and only a path that answered `'file'` gets `realpath` + `stat`; a path on a mount that stopped answering costs one stalled stat, never a sync, a queued sync or `remove()` that cannot finish (`test/edit-history-probe-stall.test.ts`). `PATH_TOKEN_RE` runs synchronously in the request, over Bash output of up to 30,000 characters: an unquoted token may start only at a token boundary (line start or a token-ending character, `:` excluded so URLs stay ignored), and the `file://` branch stops at the next `file://` (`:` stays allowed for timestamped names). A lookbehind that merely excludes some characters lets a match start at every character of a long token and rescan it, quadratic in the line (2.7 s for 30,000 `#`). Time-budget regression tests in `test/edit-history-parse.test.ts`. Forward reads and "Load older" yield to the event loop between the files they parse (up to 8 MiB each); a store removed meanwhile ends the read before its images are probed. Remote, Docker and non-claude sessions never load a store on any route. Paths, agent descriptions and diff lines render via `textContent` only (browser test with a hostile path). The header button uses the marker-class pattern (`btn-edit-history--hidden`) and stays off phones (`mobile.css`, locked in `test/mobile-header-buttons-policy.test.ts`). Remote, Docker and non-claude sessions answer `unsupported` and are never read. diff --git a/src/edit-history-parse.ts b/src/edit-history-parse.ts index 81b7cbdc..beb7cba6 100644 --- a/src/edit-history-parse.ts +++ b/src/edit-history-parse.ts @@ -104,9 +104,10 @@ const TOKEN_CHAR = '[^\\s\'"`<>|;&()=,:*?]'; const PATH_TOKEN_RE = new RegExp( [ // 1. file:// URL (Codex: `Saved to: file:///…`) — may contain spaces up to the extension. - // No `:` inside: it keeps the lazy scan from running to the line end from every - // `file://` of a line that repeats it (quadratic otherwise). - `file://(/[^\\n:]*?\\.${IMAGE_EXT})${EXT_END}`, + // The lazy scan stops at the next `file://`: otherwise it runs to the line end from + // every `file://` of a line that repeats it (quadratic). `:` stays allowed, so a + // timestamped name (`plot-12:30.png`) is still found. + `file://(/(?:(?!file://)[^\\n])*?\\.${IMAGE_EXT})${EXT_END}`, // 2./3. Quoted paths, spaces allowed. `"([^"\\n]*\\.${IMAGE_EXT})"`, `'([^'\\n]*\\.${IMAGE_EXT})'`, diff --git a/src/edit-history-store.ts b/src/edit-history-store.ts index a8df090b..1cccb961 100644 --- a/src/edit-history-store.ts +++ b/src/edit-history-store.ts @@ -573,6 +573,11 @@ export class EditHistoryStore { * Delete the persisted file (session deleted) and forget the content. The store takes no * changes afterwards, so a read still in flight neither writes nor refills it. */ + /** True once {@link remove} ran: the store ignores every later write. */ + isRemoved(): boolean { + return this.removed; + } + async remove(): Promise { this.removed = true; if (this.saveTimer) { diff --git a/src/web/edit-history-sync.ts b/src/web/edit-history-sync.ts index bcb70f8d..e8b1c69d 100644 --- a/src/web/edit-history-sync.ts +++ b/src/web/edit-history-sync.ts @@ -110,6 +110,11 @@ export function unsupportedReason(session: EditHistorySession): string | undefin return undefined; } +/** One turn of the event loop, between synchronous parses of up to one budget each. */ +function breathe(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} + /** * Bash candidates whose probed file is a regular file with an mtime inside the call's window * (files claude sent to the user skip the window: they exist and were shown on purpose). @@ -386,13 +391,18 @@ export class EditHistorySync { const requested = new Set(requests.map((r) => r.relPath)); let more = false; const candidates: ImageCandidate[] = []; + let parsed = false; for (const f of res.files) { + // Each file is up to one budget (8 MiB) of synchronous parsing: yield between them. + if (parsed) await breathe(); + if (store.isRemoved()) break; // A requested file reads on from its cursor, a new one its tail. const available = requested.has(f.relPath) ? f.size - f.startOffset : f.size; if (f.chunk.length < Math.min(this.budget, available)) { more = true; continue; } + parsed = true; const r = store.apply({ transcriptId: res.transcriptId, source: f.relPath, @@ -439,11 +449,10 @@ export class EditHistorySync { let parsed = false; for (const f of res.files) { if (!wanted.has(f.relPath)) continue; - // Each file is up to one budget (8 MiB) of synchronous parsing: let the server breathe - // between them. A store removed meanwhile ignores the rest (it is inert once removed). - if (parsed) await new Promise((resolve) => setImmediate(resolve)); + // Each file is up to one budget (8 MiB) of synchronous parsing: yield between them. + if (parsed) await breathe(); parsed = true; - if (slot.gone) break; + if (slot.gone) return true; const r = store.applyOlder({ transcriptId: res.transcriptId, source: f.relPath, @@ -465,7 +474,8 @@ export class EditHistorySync { private async addImages(store: EditHistoryStore, candidates: readonly ImageCandidate[]): Promise { const paths = [...new Set(candidates.flatMap((c) => c.paths))]; - if (paths.length === 0) return; + // A removed store keeps nothing: do not probe agent-printed paths for it. + if (paths.length === 0 || store.isRemoved()) return; let probes: Array; try { probes = await this.deps.probe(paths); diff --git a/src/web/routes/edit-history-routes.ts b/src/web/routes/edit-history-routes.ts index 010f6b91..227a16a1 100644 --- a/src/web/routes/edit-history-routes.ts +++ b/src/web/routes/edit-history-routes.ts @@ -86,6 +86,11 @@ function listOf( return list; } +/** The answer for a session that is never read (remote, Docker, non-claude): no store involved. */ +function unsupportedList(reason: string): EditHistoryList { + return { state: 'unsupported', reason, backfillTruncated: false, agents: {}, entries: [] }; +} + export function registerEditHistoryRoutes(app: FastifyInstance, ctx: SessionPort, sync: EditHistorySync): void { const sessionOf = (id: string, req: Parameters[2]) => { const session = findSessionOrFail(ctx, id, req); @@ -106,12 +111,7 @@ export function registerEditHistoryRoutes(app: FastifyInstance, ctx: SessionPort const { session, view } = sessionOf(id, req); const why = unsupportedReason(view); // Never read: answer an empty list without loading (or creating) a store for it. - if (why) { - return { - success: true, - data: { state: 'unsupported', reason: why, backfillTruncated: false, agents: {}, entries: [] }, - }; - } + if (why) return { success: true, data: unsupportedList(why) }; // The sync starts in the same tick as the session check: a deletion right after it marks // the running sync gone (EditHistorySync.remove), so nothing writes a deleted session. const status: EditHistoryStatus = doSync === '1' ? await sync.sync(view) : sync.getStatus(session.id); @@ -121,14 +121,16 @@ export function registerEditHistoryRoutes(app: FastifyInstance, ctx: SessionPort app.post('/api/sessions/:id/edits/older', async (req): Promise> => { const { id } = req.params as { id: string }; const { session, view } = sessionOf(id, req); + const why = unsupportedReason(view); + if (why) return { success: true, data: unsupportedList(why) }; const status = await sync.sync(view, { older: true }); return { success: true, data: listOf(await sync.getStore(session.id), session.workingDir, status) }; }); app.get('/api/sessions/:id/edits/:entryId', async (req, reply): Promise> => { const { id, entryId } = req.params as { id: string; entryId: string }; - const { session } = sessionOf(id, req); - const entry = (await sync.getStore(session.id)).get(entryId); + const { session, view } = sessionOf(id, req); + const entry = unsupportedReason(view) ? undefined : (await sync.getStore(session.id)).get(entryId); if (!entry) { reply.code(404); return createErrorResponse(ApiErrorCode.NOT_FOUND, 'No such edit in this session'); diff --git a/test/edit-history-parse.test.ts b/test/edit-history-parse.test.ts index bbf16030..7e9577c3 100644 --- a/test/edit-history-parse.test.ts +++ b/test/edit-history-parse.test.ts @@ -308,6 +308,12 @@ describe('parseClaudeEditLines: images sent to the user', () => { }); describe('extractImagePathCandidates', () => { + it('keeps a colon inside a file:// path (timestamped names)', () => { + expect(extractImagePathCandidates('', 'Saved to file:///tmp/plot-12:30.png', '', CWD)).toEqual([ + '/tmp/plot-12:30.png', + ]); + }); + it('resolves relative paths against cwd and keeps ~/ paths literal', () => { expect(extractImagePathCandidates('cp a.png ../b.jpeg ~/shots/c.gif', '', '', CWD)).toEqual([ '/work/repo/a.png', @@ -353,7 +359,8 @@ describe('extractImagePathCandidates: linear in the line length (#693 review)', // Claude Code keeps up to 30,000 characters of Bash output, all of it agent-printed, and the // scan runs synchronously in the request. Before the fix one such line took ~2.7 s. const N = 30_000; - const BUDGET_MS = 250; + // Generous: the gated suite runs at nice 19 next to the live service. Linear is ~1 ms. + const BUDGET_MS = 1000; const timed = (text: string) => { const t = performance.now(); const out = extractImagePathCandidates('render', text, '', CWD); @@ -366,6 +373,8 @@ describe('extractImagePathCandidates: linear in the line length (#693 review)', ['a base64 blob', base64] as const, ['repeated file:///', 'file:///'.repeat(N / 8)] as const, ['repeated file:///x', 'file:///x'.repeat(N / 9)] as const, + ['repeated file:///a:', 'file:///a:'.repeat(N / 10)] as const, + ['file:/// then 30,000 characters', 'file:///' + 'a'.repeat(N)] as const, ['an unclosed quote', '"' + 'a'.repeat(N)] as const, ['repeated =a', '=a'.repeat(N / 2)] as const, ])('%s stays under the time budget and still finds a path after it', (_label, line) => { @@ -389,7 +398,7 @@ describe('extractImagePathCandidates: linear in the line length (#693 review)', } const t = performance.now(); parseClaudeEditLines(lines.join('\n')); - expect(performance.now() - t).toBeLessThan(1500); + expect(performance.now() - t).toBeLessThan(3000); // 6.6 s before the fix }); }); diff --git a/test/edit-history-sync.test.ts b/test/edit-history-sync.test.ts index aab8f8c4..4a9f6266 100644 --- a/test/edit-history-sync.test.ts +++ b/test/edit-history-sync.test.ts @@ -229,6 +229,24 @@ describe('EditHistorySync', () => { ]); }); + it('a removal during a read stops it: no further parsing, no probe of printed paths', async () => { + const probe = vi.fn(async (paths: readonly string[]) => paths.map(() => null)); + let removal: Promise | undefined; + const ctx = setup({ + probe, + readSources: async (path, req, budget, opts) => { + const res = await ctx.host.read(path, req, budget, opts); + removal = ctx.sync.remove('s1'); // the session is deleted while this read is out + return res; + }, + }); + ctx.host.append('main', bashPair('b1', T0, T0 + 5000, 'render out/a.png')); + ctx.host.append('subagents/agent-a1.jsonl', bashPair('b2', T0, T0 + 5000, 'render out/b.png')); + await ctx.sync.sync(local); + await removal; + expect(probe).not.toHaveBeenCalled(); + }); + it('keeps the edits when only the image probe fails', async () => { const { host, sync } = setup({ probe: async () => { diff --git a/test/routes/edit-history-routes.test.ts b/test/routes/edit-history-routes.test.ts index a9625947..2b67529b 100644 --- a/test/routes/edit-history-routes.test.ts +++ b/test/routes/edit-history-routes.test.ts @@ -190,10 +190,12 @@ describe('GET /api/sessions/:id/edits', () => { const loadStore = vi.fn(async (id: string) => EditHistoryStore.load(id, { dir, debounceMs: 0 })); const { app, read } = await setup({ loadStore }); Object.assign(session, patch); + const empty = { state: 'unsupported', reason, backfillTruncated: false, agents: {}, entries: [] }; for (const url of [URL, `${URL}?sync=1`]) { - const data = (await app.inject({ method: 'GET', url })).json().data; - expect(data).toEqual({ state: 'unsupported', reason, backfillTruncated: false, agents: {}, entries: [] }); + expect((await app.inject({ method: 'GET', url })).json().data).toEqual(empty); } + expect((await app.inject({ method: 'POST', url: `${URL}/older` })).json().data).toEqual(empty); + expect((await app.inject({ method: 'GET', url: `${URL}/t1` })).statusCode).toBe(404); expect(read).not.toHaveBeenCalled(); expect(loadStore).not.toHaveBeenCalled(); });