From 61d45c925cf362a7dfcecf0c74770aa5a91df06c Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 01:38:31 -0700 Subject: [PATCH 01/31] fix(debug-retention): collect key-log-only proxy run dirs #1385 (q91 follow-up of #1380): a run dir holding only session-meta.json + sslkeylog.log was walked into and never collected, so its plaintext TLS secrets stayed on disk indefinitely (23 dirs, 5.18 MB on the owner's machine). A run dir is now recognised by either evidence file. Red before. HOLD for the owner's q91 decision: once collectable, the 48 h TTL pass removes these months-old dirs on the first prune, so this IS the sweep. Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 13 ++++++ .../storage/debugRetention.keylog.test.ts | 43 +++++++++++++++++++ src/main/storage/debugRetention.ts | 13 +++++- 3 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md create mode 100644 src/main/storage/debugRetention.keylog.test.ts diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md new file mode 100644 index 000000000..85f7540c1 --- /dev/null +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -0,0 +1,13 @@ +# Debug retention collects key-log-only proxy run dirs (#1385) + +## Problem +`collectProxyRunDirs` recognised a run dir only by `proxy-events.jsonl`. A run dir holding just `session-meta.json` + `sslkeylog.log` was walked into and never collected. #1380 review c recounted names and sizes only (contents never read): 23 such dirs on the owner's machine, 5.18 MB of plaintext TLS session secrets, May–September 2026. + +## Fix +A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. Match the key log itself (review c). `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. + +## CONSEQUENCE, needs the owner's decision BEFORE merge (q91) +Once collectable, these dirs fall under the normal TTL pass (48 h, `AGENT_CODE_DEBUG_TTL_HOURS`). All 23 are months old, so **the first prune after this merges deletes them**. This fix therefore IS the one-time sweep q91 asked the owner about. It permanently removes potential forensic material. If the owner says keep them, this PR must not merge as-is; the alternative is to budget them without TTL-expiring them. That is a different design, not needed if the answer is yes. + +## Tests +`debugRetention.keylog.test.ts`, on the real directory shapes (`proxy////`): a key-log-only dir is collected as a `proxy` dir artifact beside a normal run dir; `_shared-conf` and a metadata-only dir are not. It is red before the fix. diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts new file mode 100644 index 000000000..1a6ce91de --- /dev/null +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -0,0 +1,43 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, relative } from 'node:path' +import { afterEach, expect, it } from 'vitest' + +import { collectProxyRunDirs } from './debugRetention.js' + +// #1385 (q91 follow-up of #1380): retention collected a proxy run dir only +// once it held `proxy-events.jsonl`. A run dir holding just +// `session-meta.json` + `sslkeylog.log` was walked into, never collected, never +// budgeted and never removed, and those are plaintext TLS session secrets. The +// owner's machine had 23 such dirs (5.18 MB, May-September 2026; names and +// sizes recounted by #1380 review c, contents never read). Shapes below are +// those real layouts: proxy////. +const roots: string[] = [] +afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }) }) + +function runDir(root: string, parts: string[], files: Record): string { + const dir = join(root, ...parts) + mkdirSync(dir, { recursive: true }) + for (const [name, body] of Object.entries(files)) writeFileSync(join(dir, name), body) + return dir +} + +it('collects a key-log-only run dir as a proxy artifact, alongside normal run dirs', async () => { + const root = mkdtempSync(join(tmpdir(), 'proxy-retention-')) + roots.push(root) + runDir(root, ['medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) + runDir(root, ['agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'], { 'session-meta.json': '{}', 'proxy-events.jsonl': '{}\n', 'sslkeylog.log': 'x'.repeat(1024) }) + // Shared mitmproxy state is never a run dir, whatever it holds. + runDir(root, ['_shared-conf'], { 'mitmproxy-ca-cert.pem': 'ca' }) + // Metadata alone is not a run's evidence; leave it for its own pass. + runDir(root, ['agent-code', 'shell-empty', '2026-09-01T00-00-00-000Z'], { 'session-meta.json': '{}' }) + + const artifacts = await collectProxyRunDirs(root) + expect(artifacts.map(artifact => relative(root, artifact.path)).sort()).toEqual([ + join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), + join('medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'), + ]) + const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-89d43b9b'))! + expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) + expect(keyLogOnly.bytes).toBeGreaterThanOrEqual(4096) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 5532ae0f1..f044ad68d 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -585,7 +585,9 @@ async function loadManualLegacyBundlePaths(): Promise> { return manual } -async function collectProxyRunDirs(root: string): Promise { +const PROXY_RUN_EVIDENCE = new Set(['proxy-events.jsonl', 'sslkeylog.log']) + +export async function collectProxyRunDirs(root: string): Promise { const out: Artifact[] = [] async function walk(dir: string, depth: number): Promise { let entries @@ -594,7 +596,14 @@ async function collectProxyRunDirs(root: string): Promise { } catch { return } - if (entries.some(entry => entry.isFile() && entry.name === 'proxy-events.jsonl')) { + // A run dir is recognised by its EVIDENCE files, either of them (#1385). + // Keying on proxy-events.jsonl alone missed run dirs that held only + // session-meta.json + sslkeylog.log: walked into, never collected, never + // budgeted, never removed. Those are plaintext TLS session secrets; the + // owner's machine had 23 such dirs (5.18 MB, May-September 2026, #1380 + // review c). Match the key log itself rather than assume it sits beside an + // events file. session-meta.json alone is NOT evidence of a run. + if (entries.some(entry => entry.isFile() && PROXY_RUN_EVIDENCE.has(entry.name))) { const artifact = await collectDirArtifact(dir, 'proxy') if (artifact) out.push(artifact) return From cb7843f2686603fd9228550c6044f7c665eb2c8f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:32:54 -0700 Subject: [PATCH 02/31] fix(lsp): re-check physical containment after server startup, right before didOpen (#1268) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-lsp-open-containment.md | 18 +++++++ src/main/ipc/lsp.ts | 20 ++++++++ src/main/ipc/lspPhysicalTarget.test.ts | 51 +++++++++++++++++++ src/main/lspManager.test.ts | 42 +++++++++++++++ src/main/lspManager.ts | 27 ++++++++++ 5 files changed, 158 insertions(+) create mode 100644 docs/plans/2026-09-27-lsp-open-containment.md create mode 100644 src/main/ipc/lspPhysicalTarget.test.ts diff --git a/docs/plans/2026-09-27-lsp-open-containment.md b/docs/plans/2026-09-27-lsp-open-containment.md new file mode 100644 index 000000000..ed29f1e64 --- /dev/null +++ b/docs/plans/2026-09-27-lsp-open-containment.md @@ -0,0 +1,18 @@ +# LSP open: re-check physical containment at the moment of use (#1268) + +**Gap.** `authorizeContext` (`src/main/ipc/lsp.ts`) validates the physical target, then `LspManager.openDocumentNow` awaits server startup, which can take seconds on a cold spawn. Only after that does it build a lexical `file://` URI and send `didOpen`. If a directory on the path is swapped for a symlink to an outside directory during startup, the server is handed a URI that resolves outside the root. + +**Fix.** +- **Manager:** `OpenDocumentParams.assertPhysicalTarget`, a callback from the authorizing caller. The manager awaits it inside the per-document queue, after server startup and immediately before a NEW server document's `didOpen`. A refusal fails open (returns false: no LSP for this document) and names nothing to the server. +- **IPC:** both open paths (`lsp:open-document`, `lsp:reopen-document`) pass `lspPhysicalTargetAssertion(context)`. It re-runs the same rule: `resolveInsideRoot` + `validateExistingTarget` (no symlink, canonical inside the root) + regular file + an unchanged relative location. +- **Why a callback and not a filesystem check in the manager:** the IPC layer owns authorization for both editor roots and AI Workspace entries, and the manager's unit tests run on fake roots. + +**Tests.** +- **Real filesystem:** the reviewer's probe. Authorize `src/a.ts`, then swap `src` for a symlink to an outside directory: refused. A leaf that became a symlink: refused. An untouched file: passes. A virtual document: nothing to check. +- **Manager:** + - the re-check runs after `initialized` and before `didOpen`; + - a refusal returns false with no notification; + - a pass opens normally. +- **Mutations killed:** no re-check in the manager; no physical validation in the assertion. + +**Residual.** The window between the re-check and `didOpen` is one await, which is inherent to any path-based open. The IPC wiring of the callback is not covered by a test (the handlers need Electron). diff --git a/src/main/ipc/lsp.ts b/src/main/ipc/lsp.ts index 8b97fee7f..e36a0b681 100644 --- a/src/main/ipc/lsp.ts +++ b/src/main/ipc/lsp.ts @@ -12,6 +12,24 @@ import type { LspPosition, } from '@shared/types/lsp.js' +/** + * The manager re-runs the physical check that authorizeContext ran, at the + * moment of use, after server startup (#1268). Same rule, same root: the + * relative path must still resolve, without symlinks, to a regular file + * inside the root, at the same relative location. Undefined for a virtual + * (pathless) document, which names no file to the server. + */ +export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; filePath: string | null }): (() => Promise) | undefined { + const { workspaceRoot, filePath } = context + if (filePath === null) return undefined + return async () => { + const requested = resolveInsideRoot(workspaceRoot, filePath) + const physical = await validateExistingTarget(workspaceRoot, requested) + if (!(await lstat(physical)).isFile()) throw new Error('LSP document is not a file') + if (relative(workspaceRoot, physical) !== filePath) throw new Error('LSP document moved after authorization') + } +} + // LSP-backed code intelligence for Monaco surfaces. // // The renderer's CodeBlock component opens a document per visible @@ -287,6 +305,7 @@ export function registerLspIpc( language: params.language, workspaceRoot: context.workspaceRoot, filePath: context.filePath, + assertPhysicalTarget: lspPhysicalTargetAssertion(context), }) // A page that left meanwhile does not get the marker back: clear() // dropped it, and the close clear() queued behind this entry will @@ -402,6 +421,7 @@ export function registerLspIpc( language: params.language, workspaceRoot: context.workspaceRoot, filePath: context.filePath, + assertPhysicalTarget: lspPhysicalTargetAssertion(context), }) if (!ok) break } diff --git a/src/main/ipc/lspPhysicalTarget.test.ts b/src/main/ipc/lspPhysicalTarget.test.ts new file mode 100644 index 000000000..5f8c05d25 --- /dev/null +++ b/src/main/ipc/lspPhysicalTarget.test.ts @@ -0,0 +1,51 @@ +import { mkdir, mkdtemp, realpath, rename, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ ipcMain: { handle: vi.fn(), on: vi.fn() } })) + +const { lspPhysicalTargetAssertion } = await import('./lsp.js') + +// #1268 review A's real-filesystem probe: authorize `src/a.ts` inside the root, +// then swap `src` for a symlink to an outside directory holding its own +// `a.ts`. The lexical path is unchanged, but it now names an escaped file. +let base = '' +afterEach(async () => { if (base) await rm(base, { recursive: true, force: true }) }) + +async function layout() { + base = await realpath(await mkdtemp(join(tmpdir(), 'lsp-toctou-'))) + const root = join(base, 'root') + const outside = join(base, 'outside') + await mkdir(join(root, 'src'), { recursive: true }) + await mkdir(outside, { recursive: true }) + await writeFile(join(root, 'src', 'a.ts'), 'export const inside = 1\n') + await writeFile(join(outside, 'a.ts'), 'export const secret = 1\n') + return { root, outside } +} + +it('passes while the authorized file is still inside the root', async () => { + const { root } = await layout() + await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })!()).resolves.toBeUndefined() +}) + +it('refuses once a directory on the path was swapped for a symlink outside the root', async () => { + const { root, outside } = await layout() + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })! + await rename(join(root, 'src'), join(root, 'src-moved')) + await symlink(outside, join(root, 'src')) + await expect(assertion()).rejects.toThrow(/escapes project root/) +}) + +it('refuses a leaf that became a symlink, even to a file inside the root', async () => { + const { root } = await layout() + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })! + await writeFile(join(root, 'src', 'b.ts'), 'export const other = 1\n') + await rm(join(root, 'src', 'a.ts')) + await symlink(join(root, 'src', 'b.ts'), join(root, 'src', 'a.ts')) + await expect(assertion()).rejects.toThrow(/symbolic links/) +}) + +it('has nothing to check for a pathless (virtual) document', () => { + expect(lspPhysicalTargetAssertion({ workspaceRoot: '/repo', filePath: null })).toBeUndefined() +}) diff --git a/src/main/lspManager.test.ts b/src/main/lspManager.test.ts index e74f13095..8623904cc 100644 --- a/src/main/lspManager.test.ts +++ b/src/main/lspManager.test.ts @@ -677,4 +677,46 @@ describe('LspManager document ownership', () => { }), ]) }) + + // #1268: the caller's physical re-check runs AFTER server startup and + // immediately before didOpen; a refusal fails open (no LSP) and names + // nothing to the server. + it('re-checks the physical target after server startup and sends no didOpen when it fails', async () => { + const manager = new LspManager() + const internal = manager as unknown as LspManagerInternals + let startServer!: () => void + const started = new Promise(resolve => { startServer = resolve }) + const order: string[] = [] + const server = { key: 'server', initialized: started.then(() => { order.push('initialized'); return {} }), closed: false, abandonedRequests: 0 } + internal.servers.set('server', server) + internal.getOrCreateServer = async () => server + const notifications: string[] = [] + internal.sendNotificationIfOpen = async (_server, method) => { notifications.push(method) } + const opening = manager.openDocument({ + clientUri: 'cc-file://root/src/a.ts', content: 'text', language: 'typescript', + workspaceRoot: '/repo', filePath: 'src/a.ts', + assertPhysicalTarget: async () => { order.push('re-check'); throw new Error('escaped') }, + }) + startServer() + await expect(opening).resolves.toBe(false) + expect(order).toEqual(['initialized', 're-check']) + expect(notifications).toEqual([]) + }) + + it('opens normally when the physical re-check passes', async () => { + const manager = new LspManager() + const internal = manager as unknown as LspManagerInternals + const server = { key: 'server', initialized: Promise.resolve({}), closed: false, abandonedRequests: 0 } + internal.servers.set('server', server) + internal.getOrCreateServer = async () => server + const notifications: string[] = [] + internal.sendNotificationIfOpen = async (_server, method) => { notifications.push(method) } + const check = vi.fn(async () => {}) + await expect(manager.openDocument({ + clientUri: 'cc-file://root/src/a.ts', content: 'text', language: 'typescript', + workspaceRoot: '/repo', filePath: 'src/a.ts', assertPhysicalTarget: check, + })).resolves.toBe(true) + expect(check).toHaveBeenCalledTimes(1) + expect(notifications).toEqual(['textDocument/didOpen']) + }) }) diff --git a/src/main/lspManager.ts b/src/main/lspManager.ts index a1469c2dd..f5a4edbef 100644 --- a/src/main/lspManager.ts +++ b/src/main/lspManager.ts @@ -53,6 +53,21 @@ type OpenDocumentParams = { language: string workspaceRoot: string filePath?: string | null + /** + * Re-check, at the moment of use, that `filePath` still resolves physically + * inside `workspaceRoot` (#1268). Throws when it no longer does. + * + * WHY a callback from the caller and not a check here: the IPC layer owns + * authorization (editor roots, AI Workspace entries) and ran the physical + * check once, BEFORE this open awaited server startup. A cold server spawn + * can take seconds, and in that window a directory under the root can be + * swapped for a symlink to an outside directory, so the lexical URI built + * after startup would name an escaped file. Calling the same authority + * again inside the per-document queue, immediately before didOpen, closes + * that window to the one await this check itself takes. Keeping the + * filesystem out of the manager also keeps its unit tests on fake roots. + */ + assertPhysicalTarget?: () => Promise } type OpenDocumentRecord = { @@ -556,6 +571,18 @@ export class LspManager extends EventEmitter { const shared = this.serverDocuments.get(key) if (!shared) { + // Only a NEW server document names the path to the server; joining an + // existing shared one sends text changes for a URI already validated. + // A refused re-check fails open like every other LSP failure here: the + // editor keeps working, without LSP for this document (#1268). + if (params.assertPhysicalTarget) { + try { + await params.assertPhysicalTarget() + } catch { + return false + } + if (server.closed) return false + } await this.sendNotificationIfOpen(server, 'textDocument/didOpen', { textDocument: { uri: serverUri, From fe09a29d0ecf520deef13c41d57049c323b9d5de Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:38:41 -0700 Subject: [PATCH 03/31] fix(agent-activity): cache a context id only after its line is written (#1303) Co-Authored-By: Claude Opus 5.5 --- .../agentActivity/AgentActivityStore.test.ts | 22 +++++++++++++++++++ src/main/agentActivity/AgentActivityStore.ts | 11 +++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/main/agentActivity/AgentActivityStore.test.ts b/src/main/agentActivity/AgentActivityStore.test.ts index a4c1eab99..1ce886102 100644 --- a/src/main/agentActivity/AgentActivityStore.test.ts +++ b/src/main/agentActivity/AgentActivityStore.test.ts @@ -124,3 +124,25 @@ describe('AgentActivityStore', () => { expect(keys.size).toBe(1) }) }) + +// #1303: the context id was cached BEFORE its context line was written. One +// failed append (ENOSPC, EIO) then left every later interval for that agent +// this month pointing at a context line that never reached disk, and +// readIntervals dropped each one silently. +describe('a failed context write', () => { + it('does not orphan the agent\'s later intervals', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let failNext = true + internal.appendLines = async (file, lines) => { + if (failNext) { failNext = false; throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + await expect(store.appendInterval({ context, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.startedAt)).toEqual([start + 2 * HOUR]) + }) +}) diff --git a/src/main/agentActivity/AgentActivityStore.ts b/src/main/agentActivity/AgentActivityStore.ts index 49d69572d..f73a41857 100644 --- a/src/main/agentActivity/AgentActivityStore.ts +++ b/src/main/agentActivity/AgentActivityStore.ts @@ -154,15 +154,24 @@ export class AgentActivityStore { const key = contextKey(interval.context) const lines: string[] = [] let id = ids.get(key) + const isNewContext = id === undefined if (id === undefined) { id = ids.size + 1 - ids.set(key, id) const contextLine: ContextLine = { t: 'c', c: id, ...interval.context } lines.push(JSON.stringify(contextLine)) } const intervalLine: IntervalLine = { t: 'i', c: id, s: interval.startedAt, e: interval.endedAt } lines.push(JSON.stringify(intervalLine)) await this.appendLines(join(this.dir, `${month}.jsonl`), lines) + // Cache the id only once its context line is on disk (#1303). Caching it + // first meant one failed append (ENOSPC, EIO) left every later interval + // for this agent this month pointing at a context line that never + // landed, and readIntervals drops an interval with no context. Not + // caching on failure means the next interval re-mints the same id + // (`ids.size + 1` is unchanged) and writes the context line again. If + // the failed append landed partially, a duplicate context line with the + // same id and content is harmless on read. + if (isNewContext) ids.set(key, id) }) } From c92ebe818cd7d669e2b3908cfb33a40cbdac0863 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:38:27 -0700 Subject: [PATCH 04/31] fix(lsp): re-check every open incl. shared joins, guard the virtual-document directory, allow case-only renames (#1412 review a) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-lsp-open-containment.md | 10 +++++- src/main/ipc/lsp.ts | 36 ++++++++++++++----- src/main/ipc/lspPhysicalTarget.test.ts | 32 ++++++++++++++--- src/main/lspManager.test.ts | 20 +++++++++++ src/main/lspManager.ts | 32 ++++++++++------- 5 files changed, 103 insertions(+), 27 deletions(-) diff --git a/docs/plans/2026-09-27-lsp-open-containment.md b/docs/plans/2026-09-27-lsp-open-containment.md index ed29f1e64..0d90612f8 100644 --- a/docs/plans/2026-09-27-lsp-open-containment.md +++ b/docs/plans/2026-09-27-lsp-open-containment.md @@ -15,4 +15,12 @@ - a pass opens normally. - **Mutations killed:** no re-check in the manager; no physical validation in the assertion. -**Residual.** The window between the re-check and `didOpen` is one await, which is inherent to any path-based open. The IPC wiring of the callback is not covered by a test (the handlers need Electron). +## After review a of #1412 +- **Shared joins:** the re-check runs at the top of the queued step for EVERY open, including one that joins an existing shared document (another alias of the same file). Before, only a new document's `didOpen` was guarded, so a join after a swap sent `didChange` for the escaped URI. +- **Virtual documents:** they are named under `root/.agent-code-lsp`, so that directory must not be a symlink out of the root. They now get a re-check too. +- **No exact relative-path comparison:** a case-only rename on a case-insensitive filesystem still resolves inside the root, and refusing it only lost LSP. Containment plus a regular file is the property. + +**Residuals.** +- **One await:** the window between the re-check and the notification is one await, inherent to any path-based open. +- **A swap after a document is already open** (a later `didChange`, or a document request, for a URI the server already holds) is not guarded. The server already has that URI, and no per-change path check stops it from reading the path later. This issue is the authorization-to-use window of an OPEN. +- **The IPC wiring** of the callback is not covered by a test, because the handlers need Electron. diff --git a/src/main/ipc/lsp.ts b/src/main/ipc/lsp.ts index e36a0b681..21a720533 100644 --- a/src/main/ipc/lsp.ts +++ b/src/main/ipc/lsp.ts @@ -1,10 +1,11 @@ import { ipcMain, type WebContents } from 'electron' import { lstat } from 'fs/promises' -import { relative } from 'path' +import { join, relative } from 'path' import type { AiWorkspaceRegistry } from '@main/aiWorkspace/AiWorkspaceRegistry.js' import { resolveInsideRoot, validateExistingTarget } from '@main/ipc/editorFs.js' import type { EditorFsRootRegistry } from '@main/ipc/editorFsRootRegistry.js' +import { LSP_VIRTUAL_DIR } from '@main/lspManager.js' import type { LspManager } from '@main/lspManager.js' import type { LspCompletionContext, @@ -14,19 +15,38 @@ import type { /** * The manager re-runs the physical check that authorizeContext ran, at the - * moment of use, after server startup (#1268). Same rule, same root: the - * relative path must still resolve, without symlinks, to a regular file - * inside the root, at the same relative location. Undefined for a virtual - * (pathless) document, which names no file to the server. + * moment of use, after server startup (#1268): the path must still resolve, + * without symlinks, to a regular file inside the root. + * + * WHY no "same relative path" check (review a of #1412): on a case- + * insensitive filesystem a case-only rename still resolves, and realpath + * returns the new spelling, so an exact comparison refused a legitimate open. + * Containment is the property that matters. + * + * A pathless (virtual) document still names a file to the server: + * `root/.agent-code-lsp/virtual-.` (makeVirtualServerUri). That + * directory must not be a symlink out of the root. */ -export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; filePath: string | null }): (() => Promise) | undefined { +export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; filePath: string | null }): () => Promise { const { workspaceRoot, filePath } = context - if (filePath === null) return undefined + if (filePath === null) { + return async () => { + const directory = join(workspaceRoot, LSP_VIRTUAL_DIR) + let entry + try { + entry = await lstat(directory) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return + throw error + } + if (entry.isSymbolicLink()) throw new Error('LSP virtual document directory is a symbolic link') + await validateExistingTarget(workspaceRoot, directory) + } + } return async () => { const requested = resolveInsideRoot(workspaceRoot, filePath) const physical = await validateExistingTarget(workspaceRoot, requested) if (!(await lstat(physical)).isFile()) throw new Error('LSP document is not a file') - if (relative(workspaceRoot, physical) !== filePath) throw new Error('LSP document moved after authorization') } } diff --git a/src/main/ipc/lspPhysicalTarget.test.ts b/src/main/ipc/lspPhysicalTarget.test.ts index 5f8c05d25..d6298e3a5 100644 --- a/src/main/ipc/lspPhysicalTarget.test.ts +++ b/src/main/ipc/lspPhysicalTarget.test.ts @@ -26,12 +26,12 @@ async function layout() { it('passes while the authorized file is still inside the root', async () => { const { root } = await layout() - await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })!()).resolves.toBeUndefined() + await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })()).resolves.toBeUndefined() }) it('refuses once a directory on the path was swapped for a symlink outside the root', async () => { const { root, outside } = await layout() - const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })! + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') }) await rename(join(root, 'src'), join(root, 'src-moved')) await symlink(outside, join(root, 'src')) await expect(assertion()).rejects.toThrow(/escapes project root/) @@ -39,13 +39,35 @@ it('refuses once a directory on the path was swapped for a symlink outside the r it('refuses a leaf that became a symlink, even to a file inside the root', async () => { const { root } = await layout() - const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })! + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') }) await writeFile(join(root, 'src', 'b.ts'), 'export const other = 1\n') await rm(join(root, 'src', 'a.ts')) await symlink(join(root, 'src', 'b.ts'), join(root, 'src', 'a.ts')) await expect(assertion()).rejects.toThrow(/symbolic links/) }) -it('has nothing to check for a pathless (virtual) document', () => { - expect(lspPhysicalTargetAssertion({ workspaceRoot: '/repo', filePath: null })).toBeUndefined() +// Review a of #1412: a virtual document is named under root/.agent-code-lsp, +// so that directory must not be a symlink out of the root. +it('refuses a virtual document whose directory is a symlink out of the root', async () => { + const { root, outside } = await layout() + await symlink(outside, join(root, '.agent-code-lsp')) + await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null })()).rejects.toThrow(/symbolic link/) +}) + +it('passes a virtual document with no directory yet, or a real one', async () => { + const { root } = await layout() + await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null })()).resolves.toBeUndefined() + await mkdir(join(root, '.agent-code-lsp')) + await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null })()).resolves.toBeUndefined() +}) + +// Review a: a case-only rename on a case-insensitive filesystem still resolves +// to the same file inside the root, and must not be refused. +it('accepts a case-only rename of the authorized file', async () => { + const { root } = await layout() + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') }) + await rename(join(root, 'src', 'a.ts'), join(root, 'src', 'A.ts')) + const caseInsensitive = await realpath(join(root, 'src', 'a.ts')).then(() => true, () => false) + if (caseInsensitive) await expect(assertion()).resolves.toBeUndefined() + else await expect(assertion()).rejects.toThrow() }) diff --git a/src/main/lspManager.test.ts b/src/main/lspManager.test.ts index 8623904cc..e1fdc9abc 100644 --- a/src/main/lspManager.test.ts +++ b/src/main/lspManager.test.ts @@ -719,4 +719,24 @@ describe('LspManager document ownership', () => { expect(check).toHaveBeenCalledTimes(1) expect(notifications).toEqual(['textDocument/didOpen']) }) + + // Review a of #1412: an open that JOINS an existing shared document (another + // alias of the same file) must run the re-check too, or a swap between its + // authorization and use sends didChange for the escaped URI. + it('runs the physical re-check when joining an existing shared document', async () => { + const manager = new LspManager() + const internal = manager as unknown as LspManagerInternals + const server = { key: 'server', initialized: Promise.resolve({}), closed: false, abandonedRequests: 0 } + internal.servers.set('server', server) + internal.getOrCreateServer = async () => server + const notifications: string[] = [] + internal.sendNotificationIfOpen = async (_server, method) => { notifications.push(method) } + const common = { content: 'text', language: 'typescript', workspaceRoot: '/repo', filePath: 'src/a.ts' } + await expect(manager.openDocument({ ...common, clientUri: 'cc-file://first/src/a.ts', assertPhysicalTarget: async () => {} })).resolves.toBe(true) + await expect(manager.openDocument({ + ...common, clientUri: 'cc-file://second/src/a.ts', + assertPhysicalTarget: async () => { throw new Error('escaped') }, + })).resolves.toBe(false) + expect(notifications).toEqual(['textDocument/didOpen']) + }) }) diff --git a/src/main/lspManager.ts b/src/main/lspManager.ts index f5a4edbef..1844d6eb0 100644 --- a/src/main/lspManager.ts +++ b/src/main/lspManager.ts @@ -209,11 +209,15 @@ function hashText(input: string): string { return Math.abs(hash).toString(16) } +/** The root-relative directory virtual (pathless) documents are named under. + * Exported because the IPC layer's physical re-check guards it (#1268). */ +export const LSP_VIRTUAL_DIR = '.agent-code-lsp' + function makeVirtualServerUri(workspaceRoot: string, clientUri: string, language: string): string { const ext = languageFileExtension(language) const filePath = resolve( workspaceRoot, - '.agent-code-lsp', + LSP_VIRTUAL_DIR, `virtual-${hashText(clientUri)}.${ext}`, ) return pathToFileURL(filePath).href @@ -552,6 +556,20 @@ export class LspManager extends EventEmitter { // could not fan out for us — this URI had no record when it ran. this.notifyDocumentIntent(key) return await this.serializeServerDocument(key, async () => { + // The caller's physical re-check runs FIRST, for every open (#1268). A + // refusal fails open like every other LSP failure here: the editor keeps + // working, without LSP for this document. Review a of #1412: running it + // only before a NEW document's didOpen let an open that JOINED an + // existing shared document (another alias of the same file) pass after + // a swap and send didChange for the now-escaped URI. + if (params.assertPhysicalTarget) { + try { + await params.assertPhysicalTarget() + } catch { + return false + } + if (server.closed) return false + } const existing = this.docs.get(params.clientUri) if (existing) { if (existing.serverKey !== server.key || existing.serverUri !== serverUri) { @@ -571,18 +589,6 @@ export class LspManager extends EventEmitter { const shared = this.serverDocuments.get(key) if (!shared) { - // Only a NEW server document names the path to the server; joining an - // existing shared one sends text changes for a URI already validated. - // A refused re-check fails open like every other LSP failure here: the - // editor keeps working, without LSP for this document (#1268). - if (params.assertPhysicalTarget) { - try { - await params.assertPhysicalTarget() - } catch { - return false - } - if (server.closed) return false - } await this.sendNotificationIfOpen(server, 'textDocument/didOpen', { textDocument: { uri: serverUri, From 8755ce23f7f18dc52a0c2bf6b9dbe0adf702fdd5 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:44:57 -0700 Subject: [PATCH 05/31] fix(agent-activity): never reissue a context id after a failed or partial write (#1414 review a+b) Co-Authored-By: Claude Opus 5.5 --- .../agentActivity/AgentActivityStore.test.ts | 30 +++++++++++++++++++ src/main/agentActivity/AgentActivityStore.ts | 23 ++++++++++---- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/main/agentActivity/AgentActivityStore.test.ts b/src/main/agentActivity/AgentActivityStore.test.ts index 1ce886102..2b629ba14 100644 --- a/src/main/agentActivity/AgentActivityStore.test.ts +++ b/src/main/agentActivity/AgentActivityStore.test.ts @@ -146,3 +146,33 @@ describe('a failed context write', () => { expect(read.map(interval => interval.startedAt)).toEqual([start + 2 * HOUR]) }) }) + +// #1414 review a+b: a PARTIAL write (A's context line lands, then the append +// fails before its interval line) left id 1 on disk for A. A was not cached, so +// B's next context also took id 1; after a restart a later A interval reused +// id 1 and read back as B's time. +describe('a partially written context', () => { + it('never lets another agent reuse its id', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let partial = true + internal.appendLines = async (file, lines) => { + if (partial) { + partial = false + await appendFile(file, lines[0] + '\n') + throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) + } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + await expect(store.appendInterval({ context: a, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: a, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['B', 'A']) + }) +}) diff --git a/src/main/agentActivity/AgentActivityStore.ts b/src/main/agentActivity/AgentActivityStore.ts index f73a41857..1584fea02 100644 --- a/src/main/agentActivity/AgentActivityStore.ts +++ b/src/main/agentActivity/AgentActivityStore.ts @@ -115,6 +115,15 @@ export class AgentActivityStore { private readonly cleanTails = new Set() /** Context ids already written to each month file this process has touched. */ private readonly monthContexts = new Map>() + /** + * The next context id to mint per month (#1414 review). Minting always + * advances it, even when the write then fails, so an id is never issued + * twice: a PARTIAL write can leave a context line on disk for an id whose + * mapping was never cached, and reusing that id for another agent made the + * later reads attribute one agent's time to the other. Loaded as the + * file's highest id + 1. + */ + private readonly monthNextId = new Map() constructor(private readonly dir: string) {} @@ -132,9 +141,11 @@ export class AgentActivityStore { const known = this.monthContexts.get(month) if (known) return known const ids = new Map() + let highest = 0 try { for (const line of parseJsonLines(await readFile(join(this.dir, `${month}.jsonl`), 'utf8'))) { if (line.t !== 'c' || !isNumber(line.c)) continue + highest = Math.max(highest, line.c) const context = parseContext(line) if (context) ids.set(contextKey(context), line.c) } @@ -142,6 +153,7 @@ export class AgentActivityStore { // No file yet for this month. } this.monthContexts.set(month, ids) + this.monthNextId.set(month, highest + 1) return ids } @@ -156,7 +168,8 @@ export class AgentActivityStore { let id = ids.get(key) const isNewContext = id === undefined if (id === undefined) { - id = ids.size + 1 + id = this.monthNextId.get(month) ?? ids.size + 1 + this.monthNextId.set(month, id + 1) const contextLine: ContextLine = { t: 'c', c: id, ...interval.context } lines.push(JSON.stringify(contextLine)) } @@ -167,10 +180,10 @@ export class AgentActivityStore { // first meant one failed append (ENOSPC, EIO) left every later interval // for this agent this month pointing at a context line that never // landed, and readIntervals drops an interval with no context. Not - // caching on failure means the next interval re-mints the same id - // (`ids.size + 1` is unchanged) and writes the context line again. If - // the failed append landed partially, a duplicate context line with the - // same id and content is harmless on read. + // caching on failure means the next interval for this agent mints a + // NEW id (monthNextId already advanced) and writes its context line + // again. The failed id is burned: if its line landed partially, it + // still names this agent, and no other agent is ever given that id. if (isNewContext) ids.set(key, id) }) } From f72b614346d892f66b4a3bf6e135d4268e6d1737 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:11:28 -0700 Subject: [PATCH 06/31] fix(debug-retention): an unreadable child or bundle ledger protects, never counts as empty (q109, q115) dirStats skipped any child it could not read, so a run whose fresh data sat in an unreadable child was dated by its oldest file and TTL-removed. A failed read of the saved-bundles ledger returned an empty manual set, so manual legacy bundles were bucketed as prunable autosave. Only ENOENT now means absent; any other failure leaves the artifact uncollected. Co-Authored-By: Claude Opus 5.5 --- .../storage/debugRetention.keylog.test.ts | 101 +++++++++++++++++- src/main/storage/debugRetention.ts | 37 +++++-- 2 files changed, 127 insertions(+), 11 deletions(-) diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 1a6ce91de..18faac7ac 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -1,9 +1,11 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, utimesSync, writeFileSync } from 'node:fs' +import { rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join, relative } from 'node:path' import { afterEach, expect, it } from 'vitest' -import { collectProxyRunDirs } from './debugRetention.js' +import { collectLegacyDebugBundleDirs, collectProxyRunDirs, loadManualLegacyBundlePaths, runPrunePasses } from './debugRetention.js' +import type { DebugStorageBucket, DebugStoragePrunePolicy } from './debugRetention.js' // #1385 (q91 follow-up of #1380): retention collected a proxy run dir only // once it held `proxy-events.jsonl`. A run dir holding just @@ -13,7 +15,11 @@ import { collectProxyRunDirs } from './debugRetention.js' // sizes recounted by #1380 review c, contents never read). Shapes below are // those real layouts: proxy////. const roots: string[] = [] -afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }) }) +const locked: string[] = [] +afterEach(() => { + for (const dir of locked.splice(0)) chmodSync(dir, 0o700) + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }) +}) function runDir(root: string, parts: string[], files: Record): string { const dir = join(root, ...parts) @@ -41,3 +47,92 @@ it('collects a key-log-only run dir as a proxy artifact, alongside normal run di expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) expect(keyLogOnly.bytes).toBeGreaterThanOrEqual(4096) }) + +// Worker rule "unknown is never empty" (q109, q115). dirStats swallowed EVERY +// child error as "best effort", so a child it could not read simply did not +// count: the run dir's age came from what WAS readable. A run whose fresh +// data sits in a child the pass cannot read (EACCES, EIO, EMFILE) looked as +// old as its oldest file, and the TTL pass removed it. An unreadable child is +// UNKNOWN, and unknown must protect the whole dir. Only ENOENT (a concurrent +// remover won the race) means "not there". Real filesystem throughout: fail +// once, recover, maintain, and the bytes survive; then a truly old dir still +// goes, so the protection is not permanent. +it('a run dir with a child it cannot read is protected, and is collected normally once readable again', async () => { + const root = mkdtempSync(join(tmpdir(), 'proxy-retention-')) + roots.push(root) + const now = Date.now() + const old = new Date(now - 30 * 24 * 3_600_000) + const dir = runDir(root, ['agent-code', 'shell-1', '2026-09-27T00-00-00-000Z'], { 'sslkeylog.log': 'k'.repeat(512) }) + utimesSync(join(dir, 'sslkeylog.log'), old, old) + const streams = join(dir, 'streams') + mkdirSync(streams) + writeFileSync(join(streams, 'fresh.bin'), 'f'.repeat(256)) + utimesSync(dir, old, old) + + const caps = {} as Record + for (const bucket of ['proxy'] as DebugStorageBucket[]) caps[bucket] = 1_000_000_000 + const policy: DebugStoragePrunePolicy = { now, ttlMs: 48 * 3_600_000, activeGraceMs: 10 * 60_000, budgetBytes: 1_000_000_000, caps } + const prune = async () => runPrunePasses(await collectProxyRunDirs(root), policy, async artifact => { + try { await rm(artifact.path, { recursive: true, force: true }); return true } catch { return false } + }) + + chmodSync(streams, 0o000) + locked.push(streams) + await prune() + expect(existsSync(join(dir, 'sslkeylog.log'))).toBe(true) + + chmodSync(streams, 0o700) + locked.splice(0) + await prune() + await prune() + expect(existsSync(join(dir, 'sslkeylog.log'))).toBe(true) + expect(existsSync(join(streams, 'fresh.bin'))).toBe(true) + + utimesSync(join(streams, 'fresh.bin'), old, old) + utimesSync(streams, old, old) + utimesSync(dir, old, old) + await prune() + expect(existsSync(dir)).toBe(false) +}) + +// Same rule, the PROTECT side. Legacy root-level bundles are classified manual +// (protected forever) or autosave (prunable) from the saved-bundles ledger. A +// failed ledger read returned an EMPTY manual set, so every manual legacy +// bundle was bucketed as autosave and aged out. Unknown must protect: a ledger +// that exists but cannot be read leaves legacy bundles uncollected that pass. +// Only a ledger that is not there (ENOENT) means "no manual bundles". +it('an unreadable bundle ledger protects legacy bundles, and they are classified normally once readable', async () => { + const root = mkdtempSync(join(tmpdir(), 'legacy-bundles-')) + roots.push(root) + const now = Date.now() + const old = new Date(now - 30 * 24 * 3_600_000) + const bundle = runDir(root, ['2026-05-01T10-00-00-000-manual-report'], { 'manifest.json': '{}' }) + utimesSync(join(bundle, 'manifest.json'), old, old) + utimesSync(bundle, old, old) + const ledger = join(root, 'saved-debug-bundles.jsonl') + writeFileSync(ledger, JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: bundle }) + '\n') + + const caps = {} as Record + for (const bucket of ['debug-bundles-legacy', 'debug-bundles-manual'] as DebugStorageBucket[]) caps[bucket] = 1_000_000_000 + const policy: DebugStoragePrunePolicy = { now, ttlMs: 48 * 3_600_000, activeGraceMs: 10 * 60_000, budgetBytes: 1_000_000_000, caps } + const prune = async () => runPrunePasses( + await collectLegacyDebugBundleDirs(root, await loadManualLegacyBundlePaths(ledger)), policy, + async artifact => { try { await rm(artifact.path, { recursive: true, force: true }); return true } catch { return false } }) + + chmodSync(ledger, 0o000) + locked.push(ledger) + await prune() + expect(existsSync(join(bundle, 'manifest.json'))).toBe(true) + + chmodSync(ledger, 0o600) + locked.splice(0) + await prune() + await prune() + expect(existsSync(join(bundle, 'manifest.json'))).toBe(true) + + // A ledger that is not there really means "no manual bundles". + rmSync(ledger) + await prune() + expect(existsSync(bundle)).toBe(false) +}) + diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index f044ad68d..854db08bc 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -500,10 +500,13 @@ async function collectIncidentRunDirs(): Promise { })) } -async function collectLegacyDebugBundleDirs( +export async function collectLegacyDebugBundleDirs( dir: string, - manualLegacyBundlePaths: Set, + manualLegacyBundlePaths: Set | null, ): Promise { + // Unknown classification: collect nothing this pass rather than guess a + // bundle is autosave (see loadManualLegacyBundlePaths). + if (manualLegacyBundlePaths === null) return [] try { const entries = await readdir(dir, { withFileTypes: true }) // WHY legacy root folders are still collected: old versions wrote both @@ -554,13 +557,24 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { artifact.bucket === 'debug-bundles-manual' } -async function loadManualLegacyBundlePaths(): Promise> { +/** + * The manual (protected) legacy bundles, or null when that is UNKNOWN. + * + * WHY null and not an empty set (q109, q115, "unknown is never empty"): this + * set is what PROTECTS a manual legacy bundle. An empty set on a failed read + * (EACCES, EIO, EMFILE) bucketed every manual legacy bundle as prunable + * autosave, and the TTL pass deleted user-saved incidents. Only ENOENT, a + * ledger that is not there, means "no manual bundles". `file` is a parameter + * so the real-fs test can point it at a temp ledger. + */ +export async function loadManualLegacyBundlePaths(file = DEBUG_BUNDLE_LOG_FILE): Promise | null> { const manual = new Set() let raw: string try { - raw = await readFile(DEBUG_BUNDLE_LOG_FILE, 'utf8') - } catch { - return manual + raw = await readFile(file, 'utf8') + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return manual + return null } for (const line of raw.split('\n')) { @@ -648,8 +662,15 @@ async function dirStats(path: string): Promise<{ bytes: number; mtimeMs: number bytes += childStats.size mtimeMs = Math.max(mtimeMs, childStats.mtimeMs) } - } catch { - // Best-effort accounting; a concurrent writer/remover can race us. + } catch (error) { + // Only ENOENT is "not there": a concurrent remover won the race, and the + // child's bytes are gone either way. Anything else (EACCES, EIO, + // EMFILE) is UNKNOWN, and unknown is never empty (q109, q115): skipping + // the child dated the dir by what WAS readable, so a run whose newest + // data sat in an unreadable child looked old and the TTL pass removed + // it. Rethrowing makes collectDirArtifact return null, which leaves the + // whole dir uncollected (protected) until a later pass can read it. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } } return { bytes, mtimeMs } From 608dc546dbf132b752cdd30dfeb7901b721f4047 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:48:05 -0700 Subject: [PATCH 07/31] revert(debug-retention): leave the manual-ledger loader to #1417 (q118) W4's #1417 owns loadManualLegacyBundlePaths and treats an absent ledger as unknown, stricter than this branch's ENOENT-means-empty. Keep only the dirStats fix; use #1417's loader as-is after it merges. Co-Authored-By: Claude Opus 5.5 --- .../storage/debugRetention.keylog.test.ts | 44 +------------------ src/main/storage/debugRetention.ts | 26 +++-------- 2 files changed, 7 insertions(+), 63 deletions(-) diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 18faac7ac..232197d14 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from 'node:os' import { join, relative } from 'node:path' import { afterEach, expect, it } from 'vitest' -import { collectLegacyDebugBundleDirs, collectProxyRunDirs, loadManualLegacyBundlePaths, runPrunePasses } from './debugRetention.js' +import { collectProxyRunDirs, runPrunePasses } from './debugRetention.js' import type { DebugStorageBucket, DebugStoragePrunePolicy } from './debugRetention.js' // #1385 (q91 follow-up of #1380): retention collected a proxy run dir only @@ -94,45 +94,3 @@ it('a run dir with a child it cannot read is protected, and is collected normall await prune() expect(existsSync(dir)).toBe(false) }) - -// Same rule, the PROTECT side. Legacy root-level bundles are classified manual -// (protected forever) or autosave (prunable) from the saved-bundles ledger. A -// failed ledger read returned an EMPTY manual set, so every manual legacy -// bundle was bucketed as autosave and aged out. Unknown must protect: a ledger -// that exists but cannot be read leaves legacy bundles uncollected that pass. -// Only a ledger that is not there (ENOENT) means "no manual bundles". -it('an unreadable bundle ledger protects legacy bundles, and they are classified normally once readable', async () => { - const root = mkdtempSync(join(tmpdir(), 'legacy-bundles-')) - roots.push(root) - const now = Date.now() - const old = new Date(now - 30 * 24 * 3_600_000) - const bundle = runDir(root, ['2026-05-01T10-00-00-000-manual-report'], { 'manifest.json': '{}' }) - utimesSync(join(bundle, 'manifest.json'), old, old) - utimesSync(bundle, old, old) - const ledger = join(root, 'saved-debug-bundles.jsonl') - writeFileSync(ledger, JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: bundle }) + '\n') - - const caps = {} as Record - for (const bucket of ['debug-bundles-legacy', 'debug-bundles-manual'] as DebugStorageBucket[]) caps[bucket] = 1_000_000_000 - const policy: DebugStoragePrunePolicy = { now, ttlMs: 48 * 3_600_000, activeGraceMs: 10 * 60_000, budgetBytes: 1_000_000_000, caps } - const prune = async () => runPrunePasses( - await collectLegacyDebugBundleDirs(root, await loadManualLegacyBundlePaths(ledger)), policy, - async artifact => { try { await rm(artifact.path, { recursive: true, force: true }); return true } catch { return false } }) - - chmodSync(ledger, 0o000) - locked.push(ledger) - await prune() - expect(existsSync(join(bundle, 'manifest.json'))).toBe(true) - - chmodSync(ledger, 0o600) - locked.splice(0) - await prune() - await prune() - expect(existsSync(join(bundle, 'manifest.json'))).toBe(true) - - // A ledger that is not there really means "no manual bundles". - rmSync(ledger) - await prune() - expect(existsSync(bundle)).toBe(false) -}) - diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 854db08bc..0e4f38700 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -500,13 +500,10 @@ async function collectIncidentRunDirs(): Promise { })) } -export async function collectLegacyDebugBundleDirs( +async function collectLegacyDebugBundleDirs( dir: string, - manualLegacyBundlePaths: Set | null, + manualLegacyBundlePaths: Set, ): Promise { - // Unknown classification: collect nothing this pass rather than guess a - // bundle is autosave (see loadManualLegacyBundlePaths). - if (manualLegacyBundlePaths === null) return [] try { const entries = await readdir(dir, { withFileTypes: true }) // WHY legacy root folders are still collected: old versions wrote both @@ -557,24 +554,13 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { artifact.bucket === 'debug-bundles-manual' } -/** - * The manual (protected) legacy bundles, or null when that is UNKNOWN. - * - * WHY null and not an empty set (q109, q115, "unknown is never empty"): this - * set is what PROTECTS a manual legacy bundle. An empty set on a failed read - * (EACCES, EIO, EMFILE) bucketed every manual legacy bundle as prunable - * autosave, and the TTL pass deleted user-saved incidents. Only ENOENT, a - * ledger that is not there, means "no manual bundles". `file` is a parameter - * so the real-fs test can point it at a temp ledger. - */ -export async function loadManualLegacyBundlePaths(file = DEBUG_BUNDLE_LOG_FILE): Promise | null> { +async function loadManualLegacyBundlePaths(): Promise> { const manual = new Set() let raw: string try { - raw = await readFile(file, 'utf8') - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return manual - return null + raw = await readFile(DEBUG_BUNDLE_LOG_FILE, 'utf8') + } catch { + return manual } for (const line of raw.split('\n')) { From 49e23d57073222747ebf393194847ce46a4a58af Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 10:25:50 -0700 Subject: [PATCH 08/31] plan: a monitor run this store never examined is not empty (#1453) Co-Authored-By: Claude Opus 5.5 --- .../2026-09-27-monitor-unexamined-run.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 docs/plans/2026-09-27-monitor-unexamined-run.md diff --git a/docs/plans/2026-09-27-monitor-unexamined-run.md b/docs/plans/2026-09-27-monitor-unexamined-run.md new file mode 100644 index 000000000..851bf920d --- /dev/null +++ b/docs/plans/2026-09-27-monitor-unexamined-run.md @@ -0,0 +1,22 @@ +# A run the monitor store never examined is not empty (#1453) + +## Evidence +- Found while verifying #1411 (q115), with a probe: store B indexes the monitor folder at startup. A second store under another run id then creates `runs/run-a` and writes `incidents.json` + `operations.json`. B's next `maintain()` deletes `run-a` (ENOENT afterwards). +- Cause, `MonitorHistoryStore.maintain()`: the expired-run pass deletes every run folder not in `index` / `incidentRuns` / `unindexedRuns`. Those maps are filled only by startup indexing, so a run that appeared later is "not known", which the pass reads as "empty". +- Two app processes can share one data folder: `--packaging-smoke` skips the single-instance lock. +- It is the same "never seen means empty" shape the worker rule forbids (q109, q115). + +## Change +- `examinedRuns`: the runs startup indexing actually looked at. +- Retention deletes a run as empty only if it was examined. An unexamined run is unknown and waits for the next start to index it. +- `clear()` resets the set with the rest. + +## Not changed (residual) +- The capacity budget (`pruneRuns`) may still remove an unexamined run, as it already may for `unindexedRuns`. That is the documented policy: the 128 MiB ceiling wins over unknown runs. It orders an unexamined run as oldest, since it has no indexed points. +- Two live stores can still examine each other's run while it is empty at startup. That needs a live-run marker, which is a different design and not what #1453 reports. + +## Test (real files) +`MonitorHistoryStore.test.ts`: run-a appears after store B indexed. +- It survives two of B's maintenance passes. This is red before the fix (ENOENT on the first pass). +- A restarted store examines it and keeps its in-retention incident. +- Past retention, it is deleted as before, so the protection is not permanent. From 674fb1fe1daccb4f17ce4e935f34966e2f66bb2f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 10:25:51 -0700 Subject: [PATCH 09/31] fix(performance): retention keeps a run this store never examined (#1453) A run folder created after startup indexing (a second store sharing the folder) was absent from the index, read as empty, and deleted by the next maintenance pass. Retention now deletes only runs it examined. Co-Authored-By: Claude Opus 5.5 --- .../performance/MonitorHistoryStore.test.ts | 44 +++++++++++++++++++ src/main/performance/MonitorHistoryStore.ts | 11 ++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/main/performance/MonitorHistoryStore.test.ts b/src/main/performance/MonitorHistoryStore.test.ts index ea27de350..06384c451 100644 --- a/src/main/performance/MonitorHistoryStore.test.ts +++ b/src/main/performance/MonitorHistoryStore.test.ts @@ -137,3 +137,47 @@ describe('bounded local performance history', () => { expect((await store.query(10 * 60_000, 16 * 60_000, undefined, 7)).resolution).toBe('1s') }) }) + +// #1453 (q115 "unknown is never empty"): retention deleted any run folder +// its in-memory index did not know. A run created AFTER this store indexed, +// by a second store sharing the folder (`--packaging-smoke` skips the +// single-instance lock), was never examined, so it looked empty and was +// deleted on the next maintenance pass. Real files throughout: the unknown +// run survives, a restart examines it, and it still expires once its data +// really is past retention (the protection is not permanent). +describe('a run this store never examined', () => { + it('is kept by retention until a later start examines it, then expires normally', async () => { + const root = await mkdtemp(join(tmpdir(), 'monitor-unexamined-')) + roots.push(root) + const DAY = 24 * 60 * 60_000 + let now = 20_000 + const first = new MonitorHistoryStore(root, 'run-b', () => now) + await first.settled() + // What the other store writes once it starts: its incidents and operations. + const foreign = join(root, 'runs', 'run-a') + await mkdir(foreign, { recursive: true }) + await writeFile(join(foreign, 'incidents.json'), JSON.stringify([incident])) + await writeFile(join(foreign, 'operations.json'), '[]') + + first.record(snapshot(now), null, [], 0, 0) + await first.settled() + expect(JSON.parse(await readFile(join(foreign, 'incidents.json'), 'utf8'))).toHaveLength(1) + + now += 2 * 60_000 + first.record(snapshot(now), null, [], 0, 0) + await first.settled() + expect(JSON.parse(await readFile(join(foreign, 'incidents.json'), 'utf8'))).toHaveLength(1) + + // A later start examines run-a; its incident is still within retention. + const restarted = new MonitorHistoryStore(root, 'run-c', () => now) + restarted.record(snapshot(now), null, [], 0, 0) + await restarted.settled() + expect(JSON.parse(await readFile(join(foreign, 'incidents.json'), 'utf8'))).toHaveLength(1) + + // Past retention, the examined run goes as before. + now += 8 * DAY + restarted.record(snapshot(now), null, [], 0, 0) + await restarted.settled() + await expect(readFile(join(foreign, 'incidents.json'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }) + }) +}) diff --git a/src/main/performance/MonitorHistoryStore.ts b/src/main/performance/MonitorHistoryStore.ts index 2db7c5fea..71fe25055 100644 --- a/src/main/performance/MonitorHistoryStore.ts +++ b/src/main/performance/MonitorHistoryStore.ts @@ -73,6 +73,12 @@ export class MonitorHistoryStore { // Runs with a file that could not be indexed. Retention must treat them as // unknown, not empty; only the capacity budget may still remove them. private unindexedRuns = new Set() + // Runs startup indexing actually looked at (#1453). Retention may delete a + // run as empty only if it was examined. A run that appeared later (a second + // store sharing this folder: `--packaging-smoke` skips the single-instance + // lock) is UNKNOWN, not empty, and waits for the next start to index it. + // Only the capacity budget may still remove it, as with unindexedRuns. + private examinedRuns = new Set() private rollups: Record = { '1s': new TierRollup('1s'), '10s': new TierRollup('10s'), '1m': new TierRollup('1m') } // Coalesced work. Incidents and operations are whole-value replacements, so // only the newest value matters; the old promise chain queued one rewrite @@ -298,7 +304,7 @@ export class MonitorHistoryStore { await mkdir(this.runDir, { recursive: true }) this.index.clear(); this.incidentRuns.clear(); this.repairedTails.clear(); this.indexed = true this.bytes = 0; this.shortened = false; this.degraded = false - this.operationFingerprint = ''; this.unindexedRuns.clear() + this.operationFingerprint = ''; this.unindexedRuns.clear(); this.examinedRuns.clear() this.lastMaintenanceAt = -Infinity } catch { this.degraded = true } return this.status() @@ -352,6 +358,7 @@ export class MonitorHistoryStore { try { await this.cleanupTemps() for (const run of await this.runNames()) { + this.examinedRuns.add(run) for (const resolution of TIERS) { const file = join(this.root, RUNS_DIR, run, `${resolution}.jsonl`) try { @@ -545,7 +552,7 @@ export class MonitorHistoryStore { // with no remaining points or incidents holds only an unattributable // operations snapshot, so it is retention-expired, not capacity-pruned. if (this.indexed) for (const run of await this.runNames()) { - if (run === this.runId || this.incidentRuns.has(run) || this.unindexedRuns.has(run) || [...this.index.values()].some(entry => entry.run === run)) continue + if (run === this.runId || !this.examinedRuns.has(run) || this.incidentRuns.has(run) || this.unindexedRuns.has(run) || [...this.index.values()].some(entry => entry.run === run)) continue await rm(join(this.root, RUNS_DIR, run), { recursive: true, force: true }) } this.bytes = await this.diskBytes() From 949b2d9764e45ce6d4ffc4130e0aef1f108864cd Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 10:41:02 -0700 Subject: [PATCH 10/31] fix(debug-retention): collect key-log-only run dirs from new runs only; existing key logs stay (#1385) The 23 existing key-log-only dirs are an owner decision (q91). Collection now starts at runs dated on or after 2026-09-28; earlier or undated dirs are left untouched and never walked into. Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 22 ++++++++--- .../storage/debugRetention.keylog.test.ts | 15 ++++++-- src/main/storage/debugRetention.ts | 38 ++++++++++++++----- 3 files changed, 57 insertions(+), 18 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index 85f7540c1..dcf916ab3 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -3,11 +3,23 @@ ## Problem `collectProxyRunDirs` recognised a run dir only by `proxy-events.jsonl`. A run dir holding just `session-meta.json` + `sslkeylog.log` was walked into and never collected. #1380 review c recounted names and sizes only (contents never read): 23 such dirs on the owner's machine, 5.18 MB of plaintext TLS session secrets, May–September 2026. -## Fix -A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. Match the key log itself (review c). `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. +## Fix (narrowed to FUTURE runs; B6's oldest-first list) +- A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. It matches the key log itself (review c). +- A dir with `proxy-events.jsonl` is collected as before. +- A key-log-only dir is collected only when its run started at or after `KEY_LOG_ONLY_SINCE` (`2026-09-28T00-00-00-000Z`). Run dirs are named by their ISO start time, which sorts as text. +- Every earlier key-log-only dir, including the owner's 23, is left untouched and never walked into. So is one whose name cannot be dated. +- `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. -## CONSEQUENCE, needs the owner's decision BEFORE merge (q91) -Once collectable, these dirs fall under the normal TTL pass (48 h, `AGENT_CODE_DEBUG_TTL_HOURS`). All 23 are months old, so **the first prune after this merges deletes them**. This fix therefore IS the one-time sweep q91 asked the owner about. It permanently removes potential forensic material. If the owner says keep them, this PR must not merge as-is; the alternative is to budget them without TTL-expiring them. That is a different design, not needed if the answer is yes. +## Owner decision kept (q91) +- Deleting the EXISTING key logs is still the owner's decision. This PR no longer makes it: the first prune after merge does not touch any of the 23 dirs. +- New key-log-only runs fall under the normal TTL pass (48 h, `AGENT_CODE_DEBUG_TTL_HOURS`) and the proxy budget. + +## Also (q115, "unknown is never empty") +`dirStats` no longer skips a child it cannot read. Only ENOENT means absent; any other error leaves the whole dir uncollected that pass. The manual-bundle ledger loader is NOT changed here, because W4's #1417 owns it (q118). ## Tests -`debugRetention.keylog.test.ts`, on the real directory shapes (`proxy////`): a key-log-only dir is collected as a `proxy` dir artifact beside a normal run dir; `_shared-conf` and a metadata-only dir are not. It is red before the fix. +`debugRetention.keylog.test.ts`, on the real directory shapes (`proxy////`): +- a NEW key-log-only dir is collected beside a normal run dir; +- an existing-dated one, an undated one, `_shared-conf` and a metadata-only dir are not; +- the unreadable-child test: fail once, recover, maintain, and the bytes survive. +- Mutations killed: removing the cutoff, and removing the name check. diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 232197d14..ed063c193 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -28,10 +28,17 @@ function runDir(root: string, parts: string[], files: Record): s return dir } -it('collects a key-log-only run dir as a proxy artifact, alongside normal run dirs', async () => { +// Narrowed to FUTURE runs (B6 oldest-first list, owner decision q91 kept): +// a key-log-only dir is collected only when it started on or after the +// cutoff. The existing ones (the owner's 23, May-September 2026, shaped like +// medlo/shell-89d43b9b below) are left untouched, and so is one whose name +// cannot be dated. +it('collects a NEW key-log-only run dir, never an existing one, alongside normal run dirs', async () => { const root = mkdtempSync(join(tmpdir(), 'proxy-retention-')) roots.push(root) runDir(root, ['medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) + runDir(root, ['medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) + runDir(root, ['medlo', 'shell-undated', 'run-without-a-timestamp'], { 'sslkeylog.log': 'x'.repeat(128) }) runDir(root, ['agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'], { 'session-meta.json': '{}', 'proxy-events.jsonl': '{}\n', 'sslkeylog.log': 'x'.repeat(1024) }) // Shared mitmproxy state is never a run dir, whatever it holds. runDir(root, ['_shared-conf'], { 'mitmproxy-ca-cert.pem': 'ca' }) @@ -41,9 +48,9 @@ it('collects a key-log-only run dir as a proxy artifact, alongside normal run di const artifacts = await collectProxyRunDirs(root) expect(artifacts.map(artifact => relative(root, artifact.path)).sort()).toEqual([ join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), - join('medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'), + join('medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'), ]) - const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-89d43b9b'))! + const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-7a1b2c3d'))! expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) expect(keyLogOnly.bytes).toBeGreaterThanOrEqual(4096) }) @@ -62,7 +69,7 @@ it('a run dir with a child it cannot read is protected, and is collected normall roots.push(root) const now = Date.now() const old = new Date(now - 30 * 24 * 3_600_000) - const dir = runDir(root, ['agent-code', 'shell-1', '2026-09-27T00-00-00-000Z'], { 'sslkeylog.log': 'k'.repeat(512) }) + const dir = runDir(root, ['agent-code', 'shell-1', '2026-09-29T00-00-00-000Z'], { 'sslkeylog.log': 'k'.repeat(512) }) utimesSync(join(dir, 'sslkeylog.log'), old, old) const streams = join(dir, 'streams') mkdirSync(streams) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 0e4f38700..2d279a723 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -1,5 +1,5 @@ import { mkdir, readFile, readdir, rm, stat, statfs } from 'node:fs/promises' -import { dirname, join, resolve } from 'node:path' +import { basename, dirname, join, resolve } from 'node:path' import { AUTOSAVE_DEBUG_BUNDLE_DIR, @@ -585,7 +585,21 @@ async function loadManualLegacyBundlePaths(): Promise> { return manual } -const PROXY_RUN_EVIDENCE = new Set(['proxy-events.jsonl', 'sslkeylog.log']) +/** + * Key-log-only run dirs are collected FORWARD ONLY (#1385, owner-approved + * narrowing in B6's oldest-first list): only those whose run timestamp is at + * or after this cutoff. Every earlier one, including the 23 dirs found on the + * owner's machine (May-September 2026), is left exactly as it is, because + * deleting existing TLS key logs is an owner decision (q91) that this PR does + * not make. The cutoff is the day this narrowing was written, so every run + * made by a build that contains it is newer. + * + * Run dirs are named by their ISO start time with `:` and `.` replaced + * (`2026-08-28T17-30-06-452Z`), which sorts as text. A name that is not in + * that shape cannot be dated, so it is not collected (unknown is never "new"). + */ +const KEY_LOG_ONLY_SINCE = '2026-09-28T00-00-00-000Z' +const RUN_DIR_NAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z$/ export async function collectProxyRunDirs(root: string): Promise { const out: Artifact[] = [] @@ -596,16 +610,22 @@ export async function collectProxyRunDirs(root: string): Promise { } catch { return } - // A run dir is recognised by its EVIDENCE files, either of them (#1385). - // Keying on proxy-events.jsonl alone missed run dirs that held only + // A run dir is recognised by its EVIDENCE files (#1385). Keying on + // proxy-events.jsonl alone missed run dirs that held only // session-meta.json + sslkeylog.log: walked into, never collected, never // budgeted, never removed. Those are plaintext TLS session secrets; the // owner's machine had 23 such dirs (5.18 MB, May-September 2026, #1380 - // review c). Match the key log itself rather than assume it sits beside an - // events file. session-meta.json alone is NOT evidence of a run. - if (entries.some(entry => entry.isFile() && PROXY_RUN_EVIDENCE.has(entry.name))) { - const artifact = await collectDirArtifact(dir, 'proxy') - if (artifact) out.push(artifact) + // review c). A key-log-only dir is a run dir either way (never walked + // into), but it is COLLECTED only when it is new (KEY_LOG_ONLY_SINCE). + // session-meta.json alone is NOT evidence of a run. + const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) + if (files.has('proxy-events.jsonl') || files.has('sslkeylog.log')) { + const name = basename(dir) + const collectable = files.has('proxy-events.jsonl') || (RUN_DIR_NAME.test(name) && name >= KEY_LOG_ONLY_SINCE) + if (collectable) { + const artifact = await collectDirArtifact(dir, 'proxy') + if (artifact) out.push(artifact) + } return } if (depth >= 4) return From d8c62e984e0741a2f941f2eff3e334ce03a1d615 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:24:42 -0700 Subject: [PATCH 11/31] fix(debug-retention): key-log-only collection starts at this machine's first pass, not a date constant (#1385, #1388 review a) Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 10 ++- .../storage/debugRetention.keylog.test.ts | 43 +++++++++++-- src/main/storage/debugRetention.ts | 64 +++++++++++++++---- 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index dcf916ab3..3b2b1a3df 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -6,7 +6,8 @@ ## Fix (narrowed to FUTURE runs; B6's oldest-first list) - A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. It matches the key log itself (review c). - A dir with `proxy-events.jsonl` is collected as before. -- A key-log-only dir is collected only when its run started at or after `KEY_LOG_ONLY_SINCE` (`2026-09-28T00-00-00-000Z`). Run dirs are named by their ISO start time, which sorts as text. +- A key-log-only dir is collected only when its run started AFTER this machine's first retention pass with this code: `keyLogRetentionSince()` writes that moment once (exclusive create) to `STATE_DIR/debug-retention-keylog-since`, and later passes read it. Run dirs are named by their ISO start time, which sorts as text, and the marker uses the same shape. If the marker cannot be read or written, or has an unknown shape, NO key-log-only dir is collected (fail closed). +- **WHY not a date constant (#1388 review a):** the first version used tomorrow's date, so a run this build made today was excluded forever, and any earlier date would sweep key logs from before the upgrade. - Every earlier key-log-only dir, including the owner's 23, is left untouched and never walked into. So is one whose name cannot be dated. - `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. @@ -23,3 +24,10 @@ - an existing-dated one, an undated one, `_shared-conf` and a metadata-only dir are not; - the unreadable-child test: fail once, recover, maintain, and the bytes survive. - Mutations killed: removing the cutoff, and removing the name check. + +## Review a (round 1) +- **Fixed:** the date constant replaced by the first-run marker (above). +- **Tests:** a same-day run after the marker is collected; one before it is not; a null cutoff collects no key-log-only dir; the marker is written once and kept, and fails closed on an unknown shape or an unreadable path. +- **Mutations killed:** no cutoff; null collecting everything; any marker shape accepted. Removing the up-front marker read ALONE survives, because the exclusive create then hits EEXIST and reads the stored marker. Removing both guards fails. +- **Not changed (finding 1):** a run dir with an events file AND a key log is collected whole, key log included. That is main's existing behaviour for event-bearing runs, which this PR does not touch. The owner decision (q91) is about the key-log-only dirs, which stay untouched. + diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index ed063c193..14efe7122 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from 'node:os' import { join, relative } from 'node:path' import { afterEach, expect, it } from 'vitest' -import { collectProxyRunDirs, runPrunePasses } from './debugRetention.js' +import { collectProxyRunDirs, keyLogRetentionSince, runPrunePasses } from './debugRetention.js' import type { DebugStorageBucket, DebugStoragePrunePolicy } from './debugRetention.js' // #1385 (q91 follow-up of #1380): retention collected a proxy run dir only @@ -29,26 +29,36 @@ function runDir(root: string, parts: string[], files: Record): s } // Narrowed to FUTURE runs (B6 oldest-first list, owner decision q91 kept): -// a key-log-only dir is collected only when it started on or after the -// cutoff. The existing ones (the owner's 23, May-September 2026, shaped like -// medlo/shell-89d43b9b below) are left untouched, and so is one whose name -// cannot be dated. +// a key-log-only dir is collected only when it started after this machine's +// first retention pass with this code (the marker, below). The existing ones +// (the owner's 23, May-September 2026, shaped like medlo/shell-89d43b9b) are +// left untouched, and so is one whose name cannot be dated. #1388 review a: +// a run made the SAME DAY, after the marker, is collected (a date constant +// excluded it forever). it('collects a NEW key-log-only run dir, never an existing one, alongside normal run dirs', async () => { const root = mkdtempSync(join(tmpdir(), 'proxy-retention-')) roots.push(root) runDir(root, ['medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) runDir(root, ['medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) runDir(root, ['medlo', 'shell-undated', 'run-without-a-timestamp'], { 'sslkeylog.log': 'x'.repeat(128) }) + runDir(root, ['medlo', 'shell-same-day-before', '2026-09-27T17-59-59-999Z'], { 'sslkeylog.log': 'x'.repeat(128) }) + runDir(root, ['medlo', 'shell-same-day-after', '2026-09-27T19-00-00-000Z'], { 'sslkeylog.log': 'x'.repeat(128) }) runDir(root, ['agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'], { 'session-meta.json': '{}', 'proxy-events.jsonl': '{}\n', 'sslkeylog.log': 'x'.repeat(1024) }) // Shared mitmproxy state is never a run dir, whatever it holds. runDir(root, ['_shared-conf'], { 'mitmproxy-ca-cert.pem': 'ca' }) // Metadata alone is not a run's evidence; leave it for its own pass. runDir(root, ['agent-code', 'shell-empty', '2026-09-01T00-00-00-000Z'], { 'session-meta.json': '{}' }) - const artifacts = await collectProxyRunDirs(root) + const since = '2026-09-27T18-00-00-000Z' + const artifacts = await collectProxyRunDirs(root, since) expect(artifacts.map(artifact => relative(root, artifact.path)).sort()).toEqual([ join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), join('medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'), + join('medlo', 'shell-same-day-after', '2026-09-27T19-00-00-000Z'), + ]) + // With no established cutoff, no key-log-only dir is collected at all. + expect((await collectProxyRunDirs(root, null)).map(artifact => relative(root, artifact.path))).toEqual([ + join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), ]) const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-7a1b2c3d'))! expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) @@ -79,7 +89,7 @@ it('a run dir with a child it cannot read is protected, and is collected normall const caps = {} as Record for (const bucket of ['proxy'] as DebugStorageBucket[]) caps[bucket] = 1_000_000_000 const policy: DebugStoragePrunePolicy = { now, ttlMs: 48 * 3_600_000, activeGraceMs: 10 * 60_000, budgetBytes: 1_000_000_000, caps } - const prune = async () => runPrunePasses(await collectProxyRunDirs(root), policy, async artifact => { + const prune = async () => runPrunePasses(await collectProxyRunDirs(root, '2026-09-28T00-00-00-000Z'), policy, async artifact => { try { await rm(artifact.path, { recursive: true, force: true }); return true } catch { return false } }) @@ -101,3 +111,22 @@ it('a run dir with a child it cannot read is protected, and is collected normall await prune() expect(existsSync(dir)).toBe(false) }) + +// The marker behind "future runs only" (#1388 review a). The first pass +// records the moment this machine started collecting and later passes keep +// it. An unreadable marker, or one in an unknown shape, yields null, which +// collects NO key-log-only run (an unknown cutoff never widens collection). +it('records the key-log cutoff once, keeps it, and fails closed when it cannot be read', async () => { + const dir = mkdtempSync(join(tmpdir(), 'keylog-since-')) + roots.push(dir) + const file = join(dir, 'state', 'debug-retention-keylog-since') + const first = await keyLogRetentionSince(file, () => new Date('2026-09-27T18:20:27.123Z')) + expect(first).toBe('2026-09-27T18-20-27-123Z') + expect(await keyLogRetentionSince(file, () => new Date('2026-10-01T00:00:00.000Z'))).toBe(first) + writeFileSync(file, 'not a timestamp') + expect(await keyLogRetentionSince(file)).toBeNull() + rmSync(file) + mkdirSync(file) + expect(await keyLogRetentionSince(file)).toBeNull() +}) + diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 804fd9e51..dab096e21 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, readdir, rm, stat, statfs } from 'node:fs/promises' +import { mkdir, readFile, readdir, rm, stat, statfs, writeFile } from 'node:fs/promises' import { basename, dirname, join, resolve } from 'node:path' import { @@ -456,12 +456,13 @@ function bucketCaps(totalBudget: number): Record { async function collectArtifacts(): Promise { const manualLegacyBundlePaths = await loadManualLegacyBundlePaths() + const keyLogOnlySince = await keyLogRetentionSince() const [feed, manualBundles, autosaveBundles, legacyBundles, proxy, performance, incidents, heapSnapshots, sessionRecordings] = await Promise.all([ collectFiles(FEED_DEBUG_DIR, 'feed-debug', name => name.endsWith('.jsonl')), collectImmediateDirs(MANUAL_DEBUG_BUNDLE_DIR, 'debug-bundles-manual'), collectImmediateDirs(AUTOSAVE_DEBUG_BUNDLE_DIR, 'debug-bundles-autosave'), collectLegacyDebugBundleDirs(DEBUG_BUNDLE_DIR, manualLegacyBundlePaths), - collectProxyRunDirs(PROXY_EVENTS_DIR), + collectProxyRunDirs(PROXY_EVENTS_DIR, keyLogOnlySince), collectImmediateDirs(PERFORMANCE_RUNS_DIR, 'performance'), collectIncidentRunDirs(), collectFiles(HEAP_SNAPSHOT_DIR, 'heap-snapshots', name => name.endsWith('.heapsnapshot')), @@ -668,21 +669,56 @@ const PROXY_RUN_MARKERS = new Set(['proxy-events.jsonl', 'proxy-events.1.jsonl'] /** * Key-log-only run dirs are collected FORWARD ONLY (#1385, owner-approved - * narrowing in B6's oldest-first list): only those whose run timestamp is at - * or after this cutoff. Every earlier one, including the 23 dirs found on the - * owner's machine (May-September 2026), is left exactly as it is, because - * deleting existing TLS key logs is an owner decision (q91) that this PR does - * not make. The cutoff is the day this narrowing was written, so every run - * made by a build that contains it is newer. + * narrowing in B6's oldest-first list): only runs that started after the + * first retention pass of a build containing this code. Every earlier one, + * including the 23 dirs found on the owner's machine (May-September 2026), is + * left exactly as it is: deleting existing TLS key logs is an owner decision + * (q91) this PR does not make. + * + * WHY a marker written on first run and not a date constant (#1388 review a): + * a fixed cutoff can only approximate "after the upgrade". The first version + * used tomorrow's date, so a run this build made TODAY was excluded forever + * (its name never crosses the cutoff), and an earlier date would have swept + * key logs older than the upgrade. The marker records the actual moment this + * machine started collecting. * * Run dirs are named by their ISO start time with `:` and `.` replaced - * (`2026-08-28T17-30-06-452Z`), which sorts as text. A name that is not in - * that shape cannot be dated, so it is not collected (unknown is never "new"). + * (`2026-08-28T17-30-06-452Z`), which sorts as text, and the marker uses the + * same shape. A name that is not in that shape cannot be dated, so it is not + * collected (unknown is never "new"). */ -const KEY_LOG_ONLY_SINCE = '2026-09-28T00-00-00-000Z' const RUN_DIR_NAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z$/ +const KEY_LOG_SINCE_FILE = join(STATE_DIR, 'debug-retention-keylog-since') + +/** + * When this machine started collecting key-log-only runs, in run-dir name + * form, or null when that cannot be established. Null collects NO key-log-only + * run (fail closed: an unknown cutoff must never widen collection to old key + * logs). The first call writes the marker exclusively, so two passes racing + * agree on one value. + */ +export async function keyLogRetentionSince(file = KEY_LOG_SINCE_FILE, now: () => Date = () => new Date()): Promise { + const readMarker = async (): Promise => { + const value = (await readFile(file, 'utf8')).trim() + return RUN_DIR_NAME.test(value) ? value : null + } + try { + return await readMarker() + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return null + } + const since = now().toISOString().replace(/[:.]/g, '-') + try { + await mkdir(dirname(file), { recursive: true }) + await writeFile(file, since, { flag: 'wx', mode: 0o600 }) + return since + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EEXIST') return readMarker().catch(() => null) + return null + } +} -export async function collectProxyRunDirs(root: string): Promise { +export async function collectProxyRunDirs(root: string, keyLogOnlySince: string | null = null): Promise { const out: Artifact[] = [] async function walk(dir: string, depth: number): Promise { let entries @@ -697,7 +733,7 @@ export async function collectProxyRunDirs(root: string): Promise { // budgeted, never removed. Those are plaintext TLS session secrets; the // owner's machine had 23 such dirs (5.18 MB, May-September 2026, #1380 // review c). A key-log-only dir is a run dir either way (never walked - // into), but it is COLLECTED only when it is new (KEY_LOG_ONLY_SINCE). + // into), but it is COLLECTED only when it started after keyLogOnlySince. // session-meta.json alone is NOT evidence of a run. const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) // Events markers (the live file or its rotated `.1` generation, #1376) @@ -705,7 +741,7 @@ export async function collectProxyRunDirs(root: string): Promise { const hasEvents = [...PROXY_RUN_MARKERS].some(marker => files.has(marker)) if (hasEvents || files.has('sslkeylog.log')) { const name = basename(dir) - const collectable = hasEvents || (RUN_DIR_NAME.test(name) && name >= KEY_LOG_ONLY_SINCE) + const collectable = hasEvents || (keyLogOnlySince !== null && RUN_DIR_NAME.test(name) && name > keyLogOnlySince) if (collectable) { const artifact = await collectDirArtifact(dir, 'proxy') if (artifact) out.push(artifact) From 8bd7e32dccfaead093c4332c4f23ea2655ad097d Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:31:56 -0700 Subject: [PATCH 12/31] fix(debug-retention): key-log-only collection excludes a baseline captured at run start, not a timestamp (#1385, #1388 review a r2 + b) A first-prune marker excluded runs made during the boot delay forever, and any timestamp comparison admits a pre-upgrade run after a clock step back. The baseline is the set of key-log-only dirs that existed when this build first started, captured strictly and written once; with none, no key-log-only dir is collected. Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 16 ++- .../storage/debugRetention.keylog.test.ts | 101 ++++++++++------ src/main/storage/debugRetention.ts | 110 +++++++++++------- 3 files changed, 148 insertions(+), 79 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index 3b2b1a3df..2264185f5 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -6,8 +6,12 @@ ## Fix (narrowed to FUTURE runs; B6's oldest-first list) - A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. It matches the key log itself (review c). - A dir with `proxy-events.jsonl` is collected as before. -- A key-log-only dir is collected only when its run started AFTER this machine's first retention pass with this code: `keyLogRetentionSince()` writes that moment once (exclusive create) to `STATE_DIR/debug-retention-keylog-since`, and later passes read it. Run dirs are named by their ISO start time, which sorts as text, and the marker uses the same shape. If the marker cannot be read or written, or has an unknown shape, NO key-log-only dir is collected (fail closed). -- **WHY not a date constant (#1388 review a):** the first version used tomorrow's date, so a run this build made today was excluded forever, and any earlier date would sweep key logs from before the upgrade. +- A key-log-only dir is collected only when it is NOT in the BASELINE: the set of key-log-only dirs that existed when a build containing this code first started (`keyLogBaseline()`, captured at run start in `holdDebugStoragePruneUntilRecovered`, written once with an exclusive create to `STATE_DIR/debug-retention-keylog-baseline.json`). Capture is strict: any directory it cannot list means no baseline, nothing is written, and a later start retries. With no baseline, NO key-log-only dir is collected. +- **WHY a captured set (#1388 review a, two rounds):** + - a date constant excluded runs made on the merge day forever; + - a first-prune marker was written minutes after start (the boot gate delays the first prune), so runs made in between were excluded forever; + - any timestamp comparison admits a pre-upgrade run whose name sorts later after a clock step back. + Membership in "what already existed" needs no clock. - Every earlier key-log-only dir, including the owner's 23, is left untouched and never walked into. So is one whose name cannot be dated. - `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. @@ -31,3 +35,11 @@ - **Mutations killed:** no cutoff; null collecting everything; any marker shape accepted. Removing the up-front marker read ALONE survives, because the exclusive create then hits EEXIST and reads the stored marker. Removing both guards fails. - **Not changed (finding 1):** a run dir with an events file AND a key log is collected whole, key log included. That is main's existing behaviour for event-bearing runs, which this PR does not touch. The owner decision (q91) is about the key-log-only dirs, which stay untouched. +## Review a round 2 + b (fixed at the next head) +- The marker is replaced by the baseline set, captured at run start. +- **Tests:** a baseline dir named after a new run (a clock step back) stays excluded; a run made after capture is collected; capture over an unreadable subtree yields no baseline and writes nothing; a baseline that cannot be written is not established (review b); the file is reused and a malformed one fails closed. +- **Mutations killed:** membership ignored; null collecting everything; lenient capture; capture recording nothing; returning an unsaved baseline. +- **Residuals:** + - The early-capture wiring in `holdDebugStoragePruneUntilRecovered` is not separately pinned; the boot-gate suite exercises it against a scratch state dir. + - `dirStats`' EIO/ELOOP branches (review b) are not reproducible on a real filesystem: symlink entries are skipped, and EIO cannot be produced on demand. EACCES is pinned. + diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 14efe7122..1545be93c 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from 'node:os' import { join, relative } from 'node:path' import { afterEach, expect, it } from 'vitest' -import { collectProxyRunDirs, keyLogRetentionSince, runPrunePasses } from './debugRetention.js' +import { collectProxyRunDirs, keyLogBaseline, runPrunePasses } from './debugRetention.js' import type { DebugStorageBucket, DebugStoragePrunePolicy } from './debugRetention.js' // #1385 (q91 follow-up of #1380): retention collected a proxy run dir only @@ -29,40 +29,40 @@ function runDir(root: string, parts: string[], files: Record): s } // Narrowed to FUTURE runs (B6 oldest-first list, owner decision q91 kept): -// a key-log-only dir is collected only when it started after this machine's -// first retention pass with this code (the marker, below). The existing ones -// (the owner's 23, May-September 2026, shaped like medlo/shell-89d43b9b) are -// left untouched, and so is one whose name cannot be dated. #1388 review a: -// a run made the SAME DAY, after the marker, is collected (a date constant -// excluded it forever). -it('collects a NEW key-log-only run dir, never an existing one, alongside normal run dirs', async () => { +// a key-log-only dir is collected only when it is NOT in the baseline, the +// set of key-log-only dirs that existed when this build first started. The +// existing ones (the owner's 23, May-September 2026, shaped like +// medlo/shell-89d43b9b) are in it and stay untouched. #1388 review a: names +// and clocks prove nothing, so a baseline dir whose name sorts AFTER a new +// run (a clock rolled back) is still excluded, and a run made minutes after +// start, before the first prune, is still new. +it('collects a key-log-only run dir only when it is not in the baseline, alongside normal run dirs', async () => { const root = mkdtempSync(join(tmpdir(), 'proxy-retention-')) roots.push(root) - runDir(root, ['medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) - runDir(root, ['medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) - runDir(root, ['medlo', 'shell-undated', 'run-without-a-timestamp'], { 'sslkeylog.log': 'x'.repeat(128) }) - runDir(root, ['medlo', 'shell-same-day-before', '2026-09-27T17-59-59-999Z'], { 'sslkeylog.log': 'x'.repeat(128) }) - runDir(root, ['medlo', 'shell-same-day-after', '2026-09-27T19-00-00-000Z'], { 'sslkeylog.log': 'x'.repeat(128) }) + const existing = join('medlo', 'shell-89d43b9b', '2026-08-28T17-30-06-452Z') + const rolledBack = join('medlo', 'shell-clock', '2026-09-27T18-02-00-000Z') + runDir(root, existing.split('/'), { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) + runDir(root, rolledBack.split('/'), { 'sslkeylog.log': 'x'.repeat(128) }) + runDir(root, ['medlo', 'shell-new', '2026-09-27T18-00-30-000Z'], { 'session-meta.json': '{}', 'sslkeylog.log': 'x'.repeat(4096) }) runDir(root, ['agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'], { 'session-meta.json': '{}', 'proxy-events.jsonl': '{}\n', 'sslkeylog.log': 'x'.repeat(1024) }) // Shared mitmproxy state is never a run dir, whatever it holds. runDir(root, ['_shared-conf'], { 'mitmproxy-ca-cert.pem': 'ca' }) // Metadata alone is not a run's evidence; leave it for its own pass. runDir(root, ['agent-code', 'shell-empty', '2026-09-01T00-00-00-000Z'], { 'session-meta.json': '{}' }) - const since = '2026-09-27T18-00-00-000Z' - const artifacts = await collectProxyRunDirs(root, since) + const baseline = new Set([existing, rolledBack]) + const artifacts = await collectProxyRunDirs(root, baseline) expect(artifacts.map(artifact => relative(root, artifact.path)).sort()).toEqual([ join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), - join('medlo', 'shell-7a1b2c3d', '2026-09-29T08-15-00-000Z'), - join('medlo', 'shell-same-day-after', '2026-09-27T19-00-00-000Z'), + join('medlo', 'shell-new', '2026-09-27T18-00-30-000Z'), ]) - // With no established cutoff, no key-log-only dir is collected at all. + const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-new'))! + expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) + expect(keyLogOnly.bytes).toBeGreaterThanOrEqual(4096) + // With no established baseline, no key-log-only dir is collected at all. expect((await collectProxyRunDirs(root, null)).map(artifact => relative(root, artifact.path))).toEqual([ join('agent-code', 'resume-a5fb379b', '2026-09-27T01-03-52-273Z'), ]) - const keyLogOnly = artifacts.find(artifact => artifact.path.includes('shell-7a1b2c3d'))! - expect(keyLogOnly).toMatchObject({ kind: 'dir', bucket: 'proxy' }) - expect(keyLogOnly.bytes).toBeGreaterThanOrEqual(4096) }) // Worker rule "unknown is never empty" (q109, q115). dirStats swallowed EVERY @@ -89,7 +89,7 @@ it('a run dir with a child it cannot read is protected, and is collected normall const caps = {} as Record for (const bucket of ['proxy'] as DebugStorageBucket[]) caps[bucket] = 1_000_000_000 const policy: DebugStoragePrunePolicy = { now, ttlMs: 48 * 3_600_000, activeGraceMs: 10 * 60_000, budgetBytes: 1_000_000_000, caps } - const prune = async () => runPrunePasses(await collectProxyRunDirs(root, '2026-09-28T00-00-00-000Z'), policy, async artifact => { + const prune = async () => runPrunePasses(await collectProxyRunDirs(root, new Set()), policy, async artifact => { try { await rm(artifact.path, { recursive: true, force: true }); return true } catch { return false } }) @@ -112,21 +112,46 @@ it('a run dir with a child it cannot read is protected, and is collected normall expect(existsSync(dir)).toBe(false) }) -// The marker behind "future runs only" (#1388 review a). The first pass -// records the moment this machine started collecting and later passes keep -// it. An unreadable marker, or one in an unknown shape, yields null, which -// collects NO key-log-only run (an unknown cutoff never widens collection). -it('records the key-log cutoff once, keeps it, and fails closed when it cannot be read', async () => { - const dir = mkdtempSync(join(tmpdir(), 'keylog-since-')) +// The baseline behind "future runs only" (#1388 review a, round 2). It is +// captured once, the first time this build starts, and reused. Capture is +// strict: a subtree it cannot read would leave its old key logs out of the +// baseline, so any unreadable directory means NO baseline (nothing +// key-log-only is collected) and nothing is written, so a later start retries. +it('captures the key-log baseline once, reuses it, and fails closed when capture or the file cannot be read', async () => { + const dir = mkdtempSync(join(tmpdir(), 'keylog-baseline-')) roots.push(dir) - const file = join(dir, 'state', 'debug-retention-keylog-since') - const first = await keyLogRetentionSince(file, () => new Date('2026-09-27T18:20:27.123Z')) - expect(first).toBe('2026-09-27T18-20-27-123Z') - expect(await keyLogRetentionSince(file, () => new Date('2026-10-01T00:00:00.000Z'))).toBe(first) - writeFileSync(file, 'not a timestamp') - expect(await keyLogRetentionSince(file)).toBeNull() - rmSync(file) - mkdirSync(file) - expect(await keyLogRetentionSince(file)).toBeNull() -}) + const root = join(dir, 'proxy') + const file = join(dir, 'state', 'debug-retention-keylog-baseline.json') + runDir(root, ['medlo', 'shell-old', '2026-08-28T17-30-06-452Z'], { 'sslkeylog.log': 'k' }) + runDir(root, ['agent-code', 'run-with-events', '2026-09-01T00-00-00-000Z'], { 'proxy-events.jsonl': '{}', 'sslkeylog.log': 'k' }) + const locked = join(root, 'locked-project') + mkdirSync(locked) + chmodSync(locked, 0o000) + try { + expect(await keyLogBaseline(file, root)).toBeNull() + expect(existsSync(file)).toBe(false) + } finally { + chmodSync(locked, 0o700) + } + + const first = await keyLogBaseline(file, root) + expect(first && [...first]).toEqual([join('medlo', 'shell-old', '2026-08-28T17-30-06-452Z')]) + runDir(root, ['medlo', 'shell-later', '2026-09-27T19-00-00-000Z'], { 'sslkeylog.log': 'k' }) + expect([...(await keyLogBaseline(file, root))!]).toEqual([join('medlo', 'shell-old', '2026-08-28T17-30-06-452Z')]) + + writeFileSync(file, '{not json') + expect(await keyLogBaseline(file, root)).toBeNull() + + // A baseline that cannot be WRITTEN is not established either (#1388 + // review b): returning the unsaved set would let the next start capture a + // different one, including key logs made in between. + const readOnlyState = join(dir, 'read-only-state') + mkdirSync(readOnlyState) + chmodSync(readOnlyState, 0o500) + try { + expect(await keyLogBaseline(join(readOnlyState, 'baseline.json'), root)).toBeNull() + } finally { + chmodSync(readOnlyState, 0o700) + } +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index dab096e21..742da75c8 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -1,5 +1,5 @@ import { mkdir, readFile, readdir, rm, stat, statfs, writeFile } from 'node:fs/promises' -import { basename, dirname, join, resolve } from 'node:path' +import { dirname, join, relative, resolve } from 'node:path' import { AUTOSAVE_DEBUG_BUNDLE_DIR, @@ -200,6 +200,10 @@ function unrefTimer(timer: ReturnType): void { export function holdDebugStoragePruneUntilRecovered(): void { if (bootGateUsed) return bootGateUsed = true + // The key-log baseline is captured NOW, at the start of the run, before any + // session of this build can write a run dir; the first prune (minutes later, + // behind this gate) awaits it. See keyLogBaseline (#1388 review a). + keyLogBaselineTask ??= keyLogBaseline() const fallback = setTimeout(openDebugStoragePruneGate, DEBUG_PRUNE_BOOT_FALLBACK_MS) unrefTimer(fallback) bootGate = { pendingReason: null, fallback, opening: null } @@ -456,13 +460,15 @@ function bucketCaps(totalBudget: number): Record { async function collectArtifacts(): Promise { const manualLegacyBundlePaths = await loadManualLegacyBundlePaths() - const keyLogOnlySince = await keyLogRetentionSince() + // Captured at run start (holdDebugStoragePruneUntilRecovered); a run that + // never held the gate (tests, odd call orders) captures here instead. + const keyLogOnlyBaseline = await (keyLogBaselineTask ??= keyLogBaseline()) const [feed, manualBundles, autosaveBundles, legacyBundles, proxy, performance, incidents, heapSnapshots, sessionRecordings] = await Promise.all([ collectFiles(FEED_DEBUG_DIR, 'feed-debug', name => name.endsWith('.jsonl')), collectImmediateDirs(MANUAL_DEBUG_BUNDLE_DIR, 'debug-bundles-manual'), collectImmediateDirs(AUTOSAVE_DEBUG_BUNDLE_DIR, 'debug-bundles-autosave'), collectLegacyDebugBundleDirs(DEBUG_BUNDLE_DIR, manualLegacyBundlePaths), - collectProxyRunDirs(PROXY_EVENTS_DIR, keyLogOnlySince), + collectProxyRunDirs(PROXY_EVENTS_DIR, keyLogOnlyBaseline), collectImmediateDirs(PERFORMANCE_RUNS_DIR, 'performance'), collectIncidentRunDirs(), collectFiles(HEAP_SNAPSHOT_DIR, 'heap-snapshots', name => name.endsWith('.heapsnapshot')), @@ -669,56 +675,83 @@ const PROXY_RUN_MARKERS = new Set(['proxy-events.jsonl', 'proxy-events.1.jsonl'] /** * Key-log-only run dirs are collected FORWARD ONLY (#1385, owner-approved - * narrowing in B6's oldest-first list): only runs that started after the - * first retention pass of a build containing this code. Every earlier one, - * including the 23 dirs found on the owner's machine (May-September 2026), is - * left exactly as it is: deleting existing TLS key logs is an owner decision - * (q91) this PR does not make. + * narrowing in B6's oldest-first list): every key-log-only dir that existed + * when a build containing this code first started is in the BASELINE and is + * left exactly as it is (the 23 on the owner's machine among them): deleting + * existing TLS key logs is an owner decision (q91) this PR does not make. + * Any key-log-only dir not in the baseline was made afterwards, and is + * collected like any other run. * - * WHY a marker written on first run and not a date constant (#1388 review a): - * a fixed cutoff can only approximate "after the upgrade". The first version - * used tomorrow's date, so a run this build made TODAY was excluded forever - * (its name never crosses the cutoff), and an earlier date would have swept - * key logs older than the upgrade. The marker records the actual moment this - * machine started collecting. - * - * Run dirs are named by their ISO start time with `:` and `.` replaced - * (`2026-08-28T17-30-06-452Z`), which sorts as text, and the marker uses the - * same shape. A name that is not in that shape cannot be dated, so it is not - * collected (unknown is never "new"). + * WHY a captured set and not a timestamp (#1388 review a, two rounds): a date + * constant excluded runs made on the merge day forever; a first-prune marker + * was written minutes after start (the boot gate delays the first prune), so + * runs made in between were excluded forever; and any timestamp comparison + * admits a pre-upgrade run whose name sorts later after a clock step back. + * Membership in the set of what already existed is the exact definition and + * needs no clock. */ -const RUN_DIR_NAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z$/ -const KEY_LOG_SINCE_FILE = join(STATE_DIR, 'debug-retention-keylog-since') +const KEY_LOG_BASELINE_FILE = join(STATE_DIR, 'debug-retention-keylog-baseline.json') +let keyLogBaselineTask: Promise | null> | null = null /** - * When this machine started collecting key-log-only runs, in run-dir name - * form, or null when that cannot be established. Null collects NO key-log-only - * run (fail closed: an unknown cutoff must never widen collection to old key - * logs). The first call writes the marker exclusively, so two passes racing - * agree on one value. + * The key-log-only run dirs (paths relative to `root`) that existed when + * this build first started, or null when that cannot be established. Null + * collects NO key-log-only run: an unknown baseline must never widen + * collection to old key logs. + * + * Captured once and written with an exclusive create; later calls read it. + * Capture is STRICT: a directory it cannot list (anything but ENOENT) could + * hide old key logs from the set, so capture fails, nothing is written, and a + * later start retries. */ -export async function keyLogRetentionSince(file = KEY_LOG_SINCE_FILE, now: () => Date = () => new Date()): Promise { - const readMarker = async (): Promise => { - const value = (await readFile(file, 'utf8')).trim() - return RUN_DIR_NAME.test(value) ? value : null +export async function keyLogBaseline(file = KEY_LOG_BASELINE_FILE, root = PROXY_EVENTS_DIR): Promise | null> { + const read = async (): Promise | null> => { + try { + const parsed = JSON.parse(await readFile(file, 'utf8')) as unknown + return Array.isArray(parsed) && parsed.every(entry => typeof entry === 'string') ? new Set(parsed as string[]) : null + } catch { + return null + } } try { - return await readMarker() + await stat(file) + return await read() } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return null } - const since = now().toISOString().replace(/[:.]/g, '-') + const existing: string[] = [] + async function walk(dir: string, depth: number): Promise { + let entries + try { + entries = await readdir(dir, { withFileTypes: true }) + } catch (error) { + if (dir === root && (error as NodeJS.ErrnoException).code === 'ENOENT') return + throw error + } + const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) + const hasEvents = [...PROXY_RUN_MARKERS].some(marker => files.has(marker)) + if (hasEvents || files.has('sslkeylog.log')) { + if (!hasEvents) existing.push(relative(root, dir)) + return + } + if (depth >= 4) return + for (const entry of entries) { + if (!entry.isDirectory() || entry.name === '_shared-conf') continue + await walk(join(dir, entry.name), depth + 1) + } + } try { + await walk(root, 0) await mkdir(dirname(file), { recursive: true }) - await writeFile(file, since, { flag: 'wx', mode: 0o600 }) - return since + await writeFile(file, JSON.stringify(existing.sort()), { flag: 'wx', mode: 0o600 }) + return new Set(existing) } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'EEXIST') return readMarker().catch(() => null) + if ((error as NodeJS.ErrnoException).code === 'EEXIST') return read() return null } } -export async function collectProxyRunDirs(root: string, keyLogOnlySince: string | null = null): Promise { +export async function collectProxyRunDirs(root: string, keyLogOnlyBaseline: ReadonlySet | null = null): Promise { const out: Artifact[] = [] async function walk(dir: string, depth: number): Promise { let entries @@ -733,15 +766,14 @@ export async function collectProxyRunDirs(root: string, keyLogOnlySince: string // budgeted, never removed. Those are plaintext TLS session secrets; the // owner's machine had 23 such dirs (5.18 MB, May-September 2026, #1380 // review c). A key-log-only dir is a run dir either way (never walked - // into), but it is COLLECTED only when it started after keyLogOnlySince. + // into), but it is COLLECTED only when it is not in the baseline. // session-meta.json alone is NOT evidence of a run. const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) // Events markers (the live file or its rotated `.1` generation, #1376) // are collected as before; a key-log-only dir only when it is new. const hasEvents = [...PROXY_RUN_MARKERS].some(marker => files.has(marker)) if (hasEvents || files.has('sslkeylog.log')) { - const name = basename(dir) - const collectable = hasEvents || (keyLogOnlySince !== null && RUN_DIR_NAME.test(name) && name > keyLogOnlySince) + const collectable = hasEvents || (keyLogOnlyBaseline !== null && !keyLogOnlyBaseline.has(relative(root, dir))) if (collectable) { const artifact = await collectDirArtifact(dir, 'proxy') if (artifact) out.push(artifact) From d3804ecabb3bdc49f3b1a6df2e509f3a9f0cafa3 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:38:53 -0700 Subject: [PATCH 13/31] fix(debug-retention): no baseline from a missing proxy root; runs born during capture stay out of it (#1385, #1388 review a round 3) Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 6 ++++ .../storage/debugRetention.keylog.test.ts | 33 +++++++++++++++++++ src/main/storage/debugRetention.ts | 17 ++++++++-- 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index 2264185f5..e830d8f0e 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -43,3 +43,9 @@ - The early-capture wiring in `holdDebugStoragePruneUntilRecovered` is not separately pinned; the boot-gate suite exercises it against a scratch state dir. - `dirStats`' EIO/ELOOP branches (review b) are not reproducible on a real filesystem: symlink entries are skipped, and EIO cannot be produced on demand. EACCES is pinned. + +## Review a round 3 (last pass) +- **Fixed, unsafe direction:** a proxy root missing at capture saved an empty baseline, so old key logs that reappeared became collectable. Capture now has no ENOENT exception, even for the root: no baseline, nothing written, retried at a later start. +- **Fixed, conservative direction:** a run created WHILE the asynchronous scan ran was baselined forever. Only dirs whose filesystem birthtime is at or before the moment capture started join the set. A missing birthtime (0) keeps the dir (conservative); the race test is macOS-only, where birthtimes exist. +- **Residual, conservative direction:** after a failed capture (for example an unwritable state dir), runs made before a later successful capture are baselined and never collected. That is a retention gap, never a deletion. The same holds on a fresh install that has no proxy folder yet: the first capture happens at the start after the folder appears. +- **Mutations killed:** the root ENOENT exception, and birthtime ignored. diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 1545be93c..93f1179bb 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -155,3 +155,36 @@ it('captures the key-log baseline once, reuses it, and fails closed when capture chmodSync(readOnlyState, 0o700) } }) + +// #1388 review a round 3 (1): a proxy root that is missing at capture is +// UNKNOWN, not empty. Saving [] would let every old key log that reappears +// be collected, so no baseline is saved and none is returned. +it('saves no baseline when the proxy root is missing at capture', async () => { + const dir = mkdtempSync(join(tmpdir(), 'keylog-baseline-')) + roots.push(dir) + const file = join(dir, 'state', 'baseline.json') + expect(await keyLogBaseline(file, join(dir, 'proxy-renamed-away'))).toBeNull() + expect(existsSync(file)).toBe(false) +}) + +// #1388 review a round 3 (2): capture is asynchronous, so a run created +// while it scans must not become a permanent baseline member. Only dirs that +// existed when capture STARTED (by filesystem birthtime) join it. +it.skipIf(process.platform !== 'darwin')('keeps a run created during capture out of the baseline', async () => { + const dir = mkdtempSync(join(tmpdir(), 'keylog-baseline-')) + roots.push(dir) + const root = join(dir, 'proxy') + const file = join(dir, 'state', 'baseline.json') + runDir(root, ['p', 's', 'old-run'], { 'sslkeylog.log': 'k' }) + await new Promise(resolve => setTimeout(resolve, 20)) + const capturing = keyLogBaseline(file, root) + // Synchronously, before the capture's first await resumes: the run exists + // when the scan reaches it. A short spin puts its birthtime clearly after + // the moment capture started. + const spinUntil = Date.now() + 3 + while (Date.now() < spinUntil) { /* spin */ } + runDir(root, ['p', 's', 'new-run'], { 'sslkeylog.log': 'k' }) + const baseline = await capturing + expect(baseline && [...baseline]).toEqual([join('p', 's', 'old-run')]) +}) + diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 742da75c8..43bf5afc0 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -705,6 +705,13 @@ let keyLogBaselineTask: Promise | null> | null = null * later start retries. */ export async function keyLogBaseline(file = KEY_LOG_BASELINE_FILE, root = PROXY_EVENTS_DIR): Promise | null> { + // Taken synchronously, before the first await (#1388 review a round 3): a + // run a session creates WHILE the scan runs must not become a permanent + // baseline member, so only dirs born before this moment join the set. A + // filesystem without birthtimes (0) keeps every dir: the conservative side + // (a new run kept, never an old one exposed). Same millisecond counts as + // existing, for the same reason. + const captureStartedAt = Date.now() const read = async (): Promise | null> => { try { const parsed = JSON.parse(await readFile(file, 'utf8')) as unknown @@ -725,13 +732,19 @@ export async function keyLogBaseline(file = KEY_LOG_BASELINE_FILE, root = PROXY_ try { entries = await readdir(dir, { withFileTypes: true }) } catch (error) { - if (dir === root && (error as NodeJS.ErrnoException).code === 'ENOENT') return + // No exception for a missing ROOT either (#1388 review a round 3): a + // proxy folder renamed away or not mounted at capture would save an + // empty baseline, and every old key log that reappeared would then be + // collected. Unknown, so no baseline; a later start retries. throw error } const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) const hasEvents = [...PROXY_RUN_MARKERS].some(marker => files.has(marker)) if (hasEvents || files.has('sslkeylog.log')) { - if (!hasEvents) existing.push(relative(root, dir)) + if (!hasEvents) { + const born = (await stat(dir)).birthtimeMs + if (!born || Math.floor(born) <= captureStartedAt) existing.push(relative(root, dir)) + } return } if (depth >= 4) return From 94204b022cb1eba91173ad395622463e12ecbe8b Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:45:05 -0700 Subject: [PATCH 14/31] docs(debug-retention): baseline membership, not names, decides; the q91 decision is tracked in #1460 (#1388 review c) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index e830d8f0e..b912e7222 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -12,7 +12,7 @@ - a first-prune marker was written minutes after start (the boot gate delays the first prune), so runs made in between were excluded forever; - any timestamp comparison admits a pre-upgrade run whose name sorts later after a clock step back. Membership in "what already existed" needs no clock. -- Every earlier key-log-only dir, including the owner's 23, is left untouched and never walked into. So is one whose name cannot be dated. +- Every key-log-only dir in the baseline, including the owner's 23, is left untouched and never walked into, whatever its name. Names play no part: a NEW key-log-only dir is collected even if its name is not a timestamp. The decision on the existing ones is tracked in #1460 (q91). - `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. ## Owner decision kept (q91) @@ -25,7 +25,7 @@ ## Tests `debugRetention.keylog.test.ts`, on the real directory shapes (`proxy////`): - a NEW key-log-only dir is collected beside a normal run dir; -- an existing-dated one, an undated one, `_shared-conf` and a metadata-only dir are not; +- baseline members (including one whose name sorts after a new run), `_shared-conf` and a metadata-only dir are not; - the unreadable-child test: fail once, recover, maintain, and the bytes survive. - Mutations killed: removing the cutoff, and removing the name check. From 1cb8b3cf0de1d7e6ab20fdaf4c399b8e0ba0d506 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:53:31 -0700 Subject: [PATCH 15/31] fix(lsp): every containment assertion first checks the root still resolves to itself (#1268, #1412 review b) A pathless document's check returned early when the virtual directory was missing, without checking the root; a root renamed away and replaced by a symlink out then let didOpen name root/.agent-code-lsp/... outside. Co-Authored-By: Claude Opus 5.5 --- src/main/ipc/lsp.ts | 16 +++++++++++++++- src/main/ipc/lspPhysicalTarget.test.ts | 13 +++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/main/ipc/lsp.ts b/src/main/ipc/lsp.ts index 21a720533..f11df4aed 100644 --- a/src/main/ipc/lsp.ts +++ b/src/main/ipc/lsp.ts @@ -1,5 +1,5 @@ import { ipcMain, type WebContents } from 'electron' -import { lstat } from 'fs/promises' +import { lstat, realpath } from 'fs/promises' import { join, relative } from 'path' import type { AiWorkspaceRegistry } from '@main/aiWorkspace/AiWorkspaceRegistry.js' @@ -27,10 +27,23 @@ import type { * `root/.agent-code-lsp/virtual-.` (makeVirtualServerUri). That * directory must not be a symlink out of the root. */ +/** + * The authorized root must still BE the root (#1412 review b): renamed away and + * replaced by a symlink out, every lexical path under it names outside + * content. `workspaceRoot` is the canonical path `roots.authorize` returned, + * so it must still resolve to exactly itself. + */ +async function assertRootUnchanged(workspaceRoot: string): Promise { + if ((await realpath(workspaceRoot)) !== workspaceRoot) throw new Error('LSP workspace root no longer resolves to the authorized root') +} + export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; filePath: string | null }): () => Promise { const { workspaceRoot, filePath } = context if (filePath === null) { return async () => { + // Checked FIRST: a missing virtual directory below proves nothing about + // where the root now points (review b, finding 2). + await assertRootUnchanged(workspaceRoot) const directory = join(workspaceRoot, LSP_VIRTUAL_DIR) let entry try { @@ -44,6 +57,7 @@ export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; fil } } return async () => { + await assertRootUnchanged(workspaceRoot) const requested = resolveInsideRoot(workspaceRoot, filePath) const physical = await validateExistingTarget(workspaceRoot, requested) if (!(await lstat(physical)).isFile()) throw new Error('LSP document is not a file') diff --git a/src/main/ipc/lspPhysicalTarget.test.ts b/src/main/ipc/lspPhysicalTarget.test.ts index d6298e3a5..d08d2d412 100644 --- a/src/main/ipc/lspPhysicalTarget.test.ts +++ b/src/main/ipc/lspPhysicalTarget.test.ts @@ -71,3 +71,16 @@ it('accepts a case-only rename of the authorized file', async () => { if (caseInsensitive) await expect(assertion()).resolves.toBeUndefined() else await expect(assertion()).rejects.toThrow() }) + +// #1412 review b (finding 2): the ROOT itself swapped for a symlink out, with +// no virtual-document directory at the target. The virtual branch returned +// early on ENOENT without checking the root, and didOpen then named +// `root/.agent-code-lsp/…`, which resolves outside. Every assertion now first +// checks that the root still resolves to itself. +it('refuses a pathless document when the root was replaced by a symlink out', async () => { + const { root, outside } = await layout() + const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null }) + await rename(root, join(base, 'root-moved')) + await symlink(outside, root) + await expect(assertion()).rejects.toThrow(/root/) +}) From 68baa3e5ac8ae79f7c272b2775e068a76e0e1891 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:56:38 -0700 Subject: [PATCH 16/31] fix(debug-retention): drop the birthtime filter; a clock step back could exclude an old key log from the baseline (#1388 review b round 3) Co-Authored-By: Claude Opus 5.5 --- ...9-27-retention-collects-keylog-run-dirs.md | 4 ++-- .../storage/debugRetention.keylog.test.ts | 22 ------------------- src/main/storage/debugRetention.ts | 19 +++++++--------- 3 files changed, 10 insertions(+), 35 deletions(-) diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md index b912e7222..ca5ccf319 100644 --- a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -46,6 +46,6 @@ ## Review a round 3 (last pass) - **Fixed, unsafe direction:** a proxy root missing at capture saved an empty baseline, so old key logs that reappeared became collectable. Capture now has no ENOENT exception, even for the root: no baseline, nothing written, retried at a later start. -- **Fixed, conservative direction:** a run created WHILE the asynchronous scan ran was baselined forever. Only dirs whose filesystem birthtime is at or before the moment capture started join the set. A missing birthtime (0) keeps the dir (conservative); the race test is macOS-only, where birthtimes exist. +- **Not fixed, conservative direction (decided with review b round 3):** a run created WHILE the startup scan runs is baselined and kept forever. A birthtime filter was tried and REVERTED: after a clock step back, a pre-existing dir's birthtime can look later than the capture start, which would exclude an old key log from the baseline (the unsafe direction). Capture starts at run start, before any session exists, so the window is the few milliseconds of the scan. - **Residual, conservative direction:** after a failed capture (for example an unwritable state dir), runs made before a later successful capture are baselined and never collected. That is a retention gap, never a deletion. The same holds on a fresh install that has no proxy folder yet: the first capture happens at the start after the folder appears. -- **Mutations killed:** the root ENOENT exception, and birthtime ignored. +- **Mutation killed:** the root ENOENT exception. diff --git a/src/main/storage/debugRetention.keylog.test.ts b/src/main/storage/debugRetention.keylog.test.ts index 93f1179bb..dd85f3285 100644 --- a/src/main/storage/debugRetention.keylog.test.ts +++ b/src/main/storage/debugRetention.keylog.test.ts @@ -166,25 +166,3 @@ it('saves no baseline when the proxy root is missing at capture', async () => { expect(await keyLogBaseline(file, join(dir, 'proxy-renamed-away'))).toBeNull() expect(existsSync(file)).toBe(false) }) - -// #1388 review a round 3 (2): capture is asynchronous, so a run created -// while it scans must not become a permanent baseline member. Only dirs that -// existed when capture STARTED (by filesystem birthtime) join it. -it.skipIf(process.platform !== 'darwin')('keeps a run created during capture out of the baseline', async () => { - const dir = mkdtempSync(join(tmpdir(), 'keylog-baseline-')) - roots.push(dir) - const root = join(dir, 'proxy') - const file = join(dir, 'state', 'baseline.json') - runDir(root, ['p', 's', 'old-run'], { 'sslkeylog.log': 'k' }) - await new Promise(resolve => setTimeout(resolve, 20)) - const capturing = keyLogBaseline(file, root) - // Synchronously, before the capture's first await resumes: the run exists - // when the scan reaches it. A short spin puts its birthtime clearly after - // the moment capture started. - const spinUntil = Date.now() + 3 - while (Date.now() < spinUntil) { /* spin */ } - runDir(root, ['p', 's', 'new-run'], { 'sslkeylog.log': 'k' }) - const baseline = await capturing - expect(baseline && [...baseline]).toEqual([join('p', 's', 'old-run')]) -}) - diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 43bf5afc0..387904a02 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -705,13 +705,13 @@ let keyLogBaselineTask: Promise | null> | null = null * later start retries. */ export async function keyLogBaseline(file = KEY_LOG_BASELINE_FILE, root = PROXY_EVENTS_DIR): Promise | null> { - // Taken synchronously, before the first await (#1388 review a round 3): a - // run a session creates WHILE the scan runs must not become a permanent - // baseline member, so only dirs born before this moment join the set. A - // filesystem without birthtimes (0) keeps every dir: the conservative side - // (a new run kept, never an old one exposed). Same millisecond counts as - // existing, for the same reason. - const captureStartedAt = Date.now() + // NO birthtime filter (#1388 review b round 3): excluding dirs "born after + // capture started" re-introduced a clock comparison in the UNSAFE direction. + // After a clock step back, a pre-existing dir's birthtime can look later than + // the capture start, so it would be left out of the baseline and collected. + // The price is conservative: a run a session creates during the few + // milliseconds of the startup scan is baselined and kept forever (never + // deleted). Capture starts at run start, before any session exists. const read = async (): Promise | null> => { try { const parsed = JSON.parse(await readFile(file, 'utf8')) as unknown @@ -741,10 +741,7 @@ export async function keyLogBaseline(file = KEY_LOG_BASELINE_FILE, root = PROXY_ const files = new Set(entries.filter(entry => entry.isFile()).map(entry => entry.name)) const hasEvents = [...PROXY_RUN_MARKERS].some(marker => files.has(marker)) if (hasEvents || files.has('sslkeylog.log')) { - if (!hasEvents) { - const born = (await stat(dir)).birthtimeMs - if (!born || Math.floor(born) <= captureStartedAt) existing.push(relative(root, dir)) - } + if (!hasEvents) existing.push(relative(root, dir)) return } if (depth >= 4) return From 4c64d3d1355b790c7a221e0bce3742de9f497287 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 11:59:01 -0700 Subject: [PATCH 17/31] fix(agent-activity): an unreadable month file refuses the append instead of restarting ids (#1303, #1414 review a round 2) Co-Authored-By: Claude Opus 5.5 --- .../agentActivity/AgentActivityStore.test.ts | 58 ++++++++++++++++++- src/main/agentActivity/AgentActivityStore.ts | 11 +++- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/main/agentActivity/AgentActivityStore.test.ts b/src/main/agentActivity/AgentActivityStore.test.ts index 2b629ba14..8ad550480 100644 --- a/src/main/agentActivity/AgentActivityStore.test.ts +++ b/src/main/agentActivity/AgentActivityStore.test.ts @@ -1,4 +1,4 @@ -import { appendFile, mkdtemp, readFile, rm } from 'node:fs/promises' +import { appendFile, chmod, mkdtemp, readFile, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -176,3 +176,59 @@ describe('a partially written context', () => { expect(read.map(interval => interval.context.agentKey)).toEqual(['B', 'A']) }) }) + +// #1414 review a round 2 (q115, "unknown is never empty"): an existing month +// file that cannot be READ was treated as absent, so a restarted store started +// ids at 1 and gave a second agent the id the first agent's lines already use. +// Once readable again, the first agent's later hours read back as the second +// agent's. Only ENOENT means "no file yet"; any other failure refuses the +// append, and the bytes stay as they were. +describe('an unreadable month file', () => { + it('refuses the append instead of restarting ids, and ids continue once it is readable', async () => { + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + await new AgentActivityStore(dir).appendInterval({ context: a, startedAt: start, endedAt: start + HOUR }) + const file = join(dir, '2026-09.jsonl') + const before = await readFile(file) + await chmod(file, 0o200) + try { + await expect(new AgentActivityStore(dir).appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR })).rejects.toThrow() + } finally { + await chmod(file, 0o600) + } + expect(await readFile(file)).toEqual(before) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + await restarted.appendInterval({ context: a, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['A', 'B', 'A']) + }) +}) + +// #1414 review b round 2 (test gap): a failed write that wrote NOTHING still +// consumed its id, and a restart must continue from the highest id on disk, +// not from the count of contexts (which would reissue a live id). +describe('an id gap left by a failed write', () => { + it('is never filled by a later context after a restart', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let failNext = true + internal.appendLines = async (file, lines) => { + if (failNext) { failNext = false; throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + const c = { ...context, agentKey: 'C', label: 'C' } + await expect(store.appendInterval({ context: a, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: c, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + await restarted.appendInterval({ context: b, startedAt: start + 3 * HOUR, endedAt: start + 4 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 5 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['B', 'C', 'B']) + }) +}) diff --git a/src/main/agentActivity/AgentActivityStore.ts b/src/main/agentActivity/AgentActivityStore.ts index 1584fea02..9e4ffc722 100644 --- a/src/main/agentActivity/AgentActivityStore.ts +++ b/src/main/agentActivity/AgentActivityStore.ts @@ -149,8 +149,15 @@ export class AgentActivityStore { const context = parseContext(line) if (context) ids.set(contextKey(context), line.c) } - } catch { - // No file yet for this month. + } catch (error) { + // Only a missing file means "no contexts yet" (#1414 review a round 2, + // q115 "unknown is never empty"). A file that exists but cannot be read + // was treated as empty: ids restarted at 1, a second agent got the id + // the first agent's lines already use, and once readable the first + // agent's later hours read back as the second's. Refuse the append + // instead (the interval is lost, as for any failed write); nothing is + // cached, so the next append reads again. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } this.monthContexts.set(month, ids) this.monthNextId.set(month, highest + 1) From 995e993c1f779d82ddb003acbd97ca824953bee9 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 12:03:11 -0700 Subject: [PATCH 18/31] fix(agent-activity): an unreadable open-interval snapshot is set aside and retried, never overwritten (#1414 review a round 3) Recovery treated any failure as 'no open file' and overwrote open.json with an empty snapshot, losing the pending interval. Only ENOENT is empty now; anything else is moved to open.json.unrecovered-