From fcadd1934aeefd30aec15d28aac938ae1f47329a Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 01/16] docs(plans): C5 fail-all batch, rows verified on main (#1251) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c5-fail-all-batch.md | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 docs/plans/2026-09-27-c5-fail-all-batch.md diff --git a/docs/plans/2026-09-27-c5-fail-all-batch.md b/docs/plans/2026-09-27-c5-fail-all-batch.md new file mode 100644 index 000000000..fdf919351 --- /dev/null +++ b/docs/plans/2026-09-27-c5-fail-all-batch.md @@ -0,0 +1,33 @@ +# C5 fail-all batch (#1251) + +Source: the read-only C5 hunt in `temp/quality-loop/hunt-c5.md`, rows 8–13. Each row was verified on origin/main `5e22c7b0` before any fix. "Fail-first" means the new test was run red against main's implementation first. + +## Principle + +One bad record must cost only itself. Two constraints shape every fix: + +1. **Owner rule: do not delete stuff often (2026-09-27).** A record this build cannot read is carried verbatim whenever the file is rewritten, never dropped. +2. **Ambiguity fails closed (q40).** + - A skipped approval grants nothing. + - A skipped ledger row can only fail to protect a bundle it does not name. + - Destructive transforms keep refusing (row 10). + +## Rows + +| Row | Verified on main | Decision | Test (fail-first) | +|---|---|---|---| +| 8: Codex rollouts, `conversations/sources/codex.ts` | Yes. `readRolloutHead` streams through readline, which rethrows EACCES/EIO. `fromHead` and both discovery loops await it with no catch, so `discover()` rejected and the Codex column emptied. | Skip the unreadable rollout (it has no cwd to scope it by); cache nothing, so it is retried later. | `codex.system.test.ts`, "skips an unreadable rollout…": red with EACCES. | +| 9: monitor incidents, `performance/MonitorHistoryStore.ts` | Yes, and worse than the hunt said. One unparseable row hid the run's whole incident list. On a helper restart in that run, `persistIncidents` merged from an empty list and rewrote `incidents.json`, erasing the readable evidence AND the unknown row. Realistic: Preview and stable builds share this directory. | Parse per row. Carry unknown rows per run in `foreignIncidents`, and re-append them on every rewrite via `incidentFileBody`. Leave room under `INCIDENT_LIMIT`, keep the run from expiry-by-emptiness, and mark the store degraded. | `MonitorHistoryStore.test.ts`, "keeps readable incidents…": red (`null` for the readable incident). | +| 10: Pi JSONL, `providerSwitch/piTranscript.ts` | Real, but **strict by design**. `loadPiSnapshotAt` feeds destructive transforms (switch, duplicate, rewind). Skipping a malformed middle line would move or rewind a conversation with a silent hole. | No change. The error names the file and line and never reaches a toast raw. | none | +| 11: workflow approvals, `workflows/WorkflowSourceApprovalStore.ts` | Yes. `load()` threw on the first bad entry and never set `loaded`, so every `authorize()` rethrew: all repository workflows were blocked. | Skip the entry (it approves nothing, so its source is prompted again) and carry it verbatim through `persist()`. A wrong file version still throws, because that is not one bad row. | `WorkflowSourceApprovalStore.test.ts`, "honours valid approvals…": red. | +| 12: TLDR batch, `main/tldr/ipc.ts` | Yes. `z.array(z.string().refine(validTldrIdentity))` rejected the whole batch, and Agent Activity reads every TLDR and goal in one batch. | Keep the payload shape strict (a bounded array of bounded strings) and drop invalid identities. This is exact: the store only writes valid identities, so an invalid one has no record. | New `tldr/ipc.test.ts`: red (ZodError). A second test pins that malformed payloads are still refused. | +| 13: legacy bundle ledger, `storage/debugRetention.ts` | Yes. A JSON-valid non-entry line (`null`, or a row with a non-string `bundlePath`) threw TypeError, which rejected `collectArtifacts` and stopped every prune pass. | Extract `parseManualLegacyBundlePaths`, then shape-check each row. A non-string reason counts as manual, so retention keeps the bundle. | `debugRetention.test.ts`, "keeps every readable manual row…": red (`null.event`). | + +Rows 14–15 (key vault index, agent-name registry, tmux recovery) are strict by design per the issue and are only recorded there. + +## Residuals + +- **Row 9:** + - Carried foreign rows never expire on their own. They leave disk only with their run directory (budget pruning, clear). + - A run whose incident file holds only foreign rows is kept from expiry-by-emptiness. It is still pruned by the data budget. +- **Row 12:** a renderer that sends an invalid identity gets no record and no error for it. That is the same answer as "no TLDR yet". From 318c0d5d201e273c228c253c8dbf4a08c310a9fc Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 02/16] fix(conversations): one unreadable Codex rollout no longer empties the list (#1251 row 8) Co-Authored-By: Claude Opus 5.5 --- .../sources/codex.system.test.ts | 30 ++++++++++++++++++- src/main/conversations/sources/codex.ts | 18 ++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/main/conversations/sources/codex.system.test.ts b/src/main/conversations/sources/codex.system.test.ts index 8b6fd394c..abd80057d 100644 --- a/src/main/conversations/sources/codex.system.test.ts +++ b/src/main/conversations/sources/codex.system.test.ts @@ -1,4 +1,4 @@ -import { rename } from 'node:fs/promises' +import { chmod, readdir, rename } from 'node:fs/promises' import { join } from 'node:path' import { DatabaseSync } from 'node:sqlite' import { afterEach, describe, expect, it } from 'vitest' @@ -90,6 +90,34 @@ describe('Codex conversation source', () => { expect(everywhere.filter(r => r.origin === 'scan')).toHaveLength(counts.codex.unindexedSampled) }) + it('skips an unreadable rollout instead of failing the whole Codex list (#1251 row 8)', async () => { + // readline's async iterator rethrows a stream error (EACCES here, EIO on a + // failing disk), and nothing between readRolloutHead and discover() caught + // it, so one rollout the app cannot open emptied the Codex column. + const { corpus, source, listWorktrees } = await setup() + const counts = corpus.manifest.counts as { codex: { inFamily: number; unindexedSampled: number } } + const rollouts = (await readdir(join(corpus.codexHome, 'sessions'), { recursive: true })) + .filter(name => /rollout-.*\.jsonl$/.test(name)).map(name => join(corpus.codexHome, 'sessions', name)) + expect(rollouts.length).toBeGreaterThan(1) + for (const file of rollouts) await chmod(file, 0o000) + // unshift: permissions come back before the corpus cleanup removes the tree. + cleanups.unshift(async () => { for (const file of rollouts) await chmod(file, 0o600) }) + const family = await resolveFamily('/fixture/repo', 'everywhere', { listWorktrees }) + + // Index path: the indexed rows never open a rollout and must all survive; + // the unindexed union is what reads heads, and it now skips what it cannot. + const indexed = await source.discover({ scope: 'everywhere', family }) + expect(indexed.filter(r => r.origin === 'index').length).toBeGreaterThanOrEqual(counts.codex.inFamily) + expect(indexed.filter(r => r.origin === 'scan')).toHaveLength(0) + + // Fallback path: one readable rollout still lists beside unreadable ones. + await chmod(rollouts[0]!, 0o600) + await rename(join(corpus.codexHome, 'state_5.sqlite'), join(corpus.codexHome, 'state_5.sqlite.away')) + const fresh = new CodexConversationSource({ codexHome: corpus.codexHome }) + const scanned = await fresh.discover({ scope: 'everywhere', family }) + expect(scanned.map(r => r.file)).toEqual([rollouts[0]]) + }) + it('falls back to the rollout scan when the index is missing and reports why', async () => { const { corpus, source, listWorktrees } = await setup() await rename(join(corpus.codexHome, 'state_5.sqlite'), join(corpus.codexHome, 'state_5.sqlite.away')) diff --git a/src/main/conversations/sources/codex.ts b/src/main/conversations/sources/codex.ts index 5a25536ca..5735586fb 100644 --- a/src/main/conversations/sources/codex.ts +++ b/src/main/conversations/sources/codex.ts @@ -169,7 +169,23 @@ export class CodexConversationSource implements ConversationSource { } } const cached = this.heads.get(file) - const head = cached && cached.mtime === mtime ? cached.head : await readRolloutHead(file) + let head: RolloutHead + if (cached && cached.mtime === mtime) head = cached.head + else { + // WHY one unreadable rollout is skipped here (#1251 row 8): readline's + // async iterator rethrows a stream error (EACCES, EIO, a file removed + // between the walk and the read), and both callers await this in a plain + // loop, so a single rollout the app cannot open used to reject + // discover() and empty the whole Codex column. Skipping costs exactly + // the row that cannot be labelled anyway (without its head there is no + // cwd to scope it by). Nothing is cached, so the next discovery retries + // it once the file is readable again. + try { + head = await readRolloutHead(file) + } catch { + return null + } + } this.heads.set(file, { mtime, head }) if (scope.scope !== 'everywhere' && !scope.family.matches(head.cwd)) return null return { From e14cd3eb3ffe1742d24a07ccf26d1602860b667c Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 03/16] fix(performance): keep readable incidents and carry unknown rows through rewrites (#1251 row 9) Co-Authored-By: Claude Opus 5.5 --- .../performance/MonitorHistoryStore.test.ts | 25 ++++++++ src/main/performance/MonitorHistoryStore.ts | 57 ++++++++++++++----- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/main/performance/MonitorHistoryStore.test.ts b/src/main/performance/MonitorHistoryStore.test.ts index ea27de350..80eb5df73 100644 --- a/src/main/performance/MonitorHistoryStore.test.ts +++ b/src/main/performance/MonitorHistoryStore.test.ts @@ -108,6 +108,31 @@ describe('bounded local performance history', () => { }) }) + it('keeps readable incidents and never erases a row it does not recognise (#1251 row 9)', async () => { + // A preview build can write an incident rule this build does not know, and + // the owner moves between the Preview and stable channels. One such row + // used to hide the run's whole incident list AND, on a helper restart in + // that run, persistIncidents replaced the file with only the new engine's + // rows, erasing the evidence it could not read. + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const foreign = { ...incident, id: 9, at: 10_500, rule: 'rule-from-a-newer-build' } + await mkdir(join(root, 'runs', 'run-mixed'), { recursive: true }) + await writeFile(join(root, 'runs', 'run-mixed', 'incidents.json'), JSON.stringify([incident, foreign])) + + const restarted = new MonitorHistoryStore(root, 'run-mixed') + await restarted.settled() + restarted.record(snapshot(11_000), null, [{ ...incident, id: 2, at: 11_000 }], 0, 1) + await restarted.settled() + + expect(await restarted.readIncident(10_000, 1)).toMatchObject({ rule: 'renderer-stall' }) + expect(await restarted.readIncident(11_000, 2)).toMatchObject({ rule: 'renderer-stall' }) + expect(restarted.status().state).toBe('degraded') + const onDisk = JSON.parse(await readFile(join(root, 'runs', 'run-mixed', 'incidents.json'), 'utf8')) as Array<{ id: number; rule: string }> + expect(onDisk.map(row => row.id).sort()).toEqual([1, 2, 9]) + expect(onDisk.find(row => row.id === 9)).toEqual(foreign) + }) + it('repairs a torn append and keeps coarse tiers peak-preserving', async () => { const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) roots.push(root) diff --git a/src/main/performance/MonitorHistoryStore.ts b/src/main/performance/MonitorHistoryStore.ts index 2db7c5fea..4bead304d 100644 --- a/src/main/performance/MonitorHistoryStore.ts +++ b/src/main/performance/MonitorHistoryStore.ts @@ -64,6 +64,19 @@ export class MonitorHistoryStore { private exporting = false private index = new Map() private incidentRuns = new Map() + // Rows of a run's incidents.json that this build cannot parse, kept + // verbatim per run (#1251 row 9). WHY they are carried instead of dropped: + // the owner moves between the Preview and stable channels, so a newer build + // can leave an incident rule this one does not know. Hiding the run's whole + // list for one such row lost the readable evidence, and every rewrite of the + // file (a helper restart's persistIncidents, capture repair, expiry, + // eviction) then replaced it with only the rows this build understood, + // deleting the rest. Invariant: every write of a run's incident file goes + // through incidentFileBody, which re-appends these rows. They never enter + // incidentRuns, so queries, exports and the 50-incident eviction only ever + // see rows this build can vouch for; the rows leave disk only with the run + // directory itself (budget pruning, retention, clear). + private foreignIncidents = new Map() private repairedTails = new Set() // False until startup indexing completes. Retention deletes any run the // index does not know about, so a partial index (EPERM, ENOSPC or an I/O @@ -296,7 +309,7 @@ export class MonitorHistoryStore { try { await rm(join(this.root, RUNS_DIR), { recursive: true, force: true }) await mkdir(this.runDir, { recursive: true }) - this.index.clear(); this.incidentRuns.clear(); this.repairedTails.clear(); this.indexed = true + this.index.clear(); this.incidentRuns.clear(); this.foreignIncidents.clear(); this.repairedTails.clear(); this.indexed = true this.bytes = 0; this.shortened = false; this.degraded = false this.operationFingerprint = ''; this.unindexedRuns.clear() this.lastMaintenanceAt = -Infinity @@ -374,13 +387,13 @@ export class MonitorHistoryStore { } } const file = join(this.root, RUNS_DIR, run, 'incidents.json') - const stored = await this.readIncidentFile(file) + const stored = await this.readIncidentFile(file, run) // Every retained run is repaired, not only the current one. A crash or // force-quit in ANY earlier run left its last capture as "capturing" // forever, and only a helper restart within the same run fixed it. const repaired = stored.map(incident => incident.state === 'capturing' ? { ...incident, state: 'interrupted' as const } : incident) if (repaired.some((incident, position) => incident !== stored[position])) { - await this.replaceBounded(file, JSON.stringify(repaired), INCIDENT_BUDGET).catch(() => { this.degraded = true }) + await this.replaceBounded(file, this.incidentFileBody(run, repaired), INCIDENT_BUDGET).catch(() => { this.degraded = true }) } if (repaired.length) this.incidentRuns.set(run, repaired) } @@ -405,10 +418,13 @@ export class MonitorHistoryStore { const existing = this.incidentRuns.get(this.runId) ?? [] const merged = new Map(existing.map(incident => [`${incident.at}:${incident.id}`, incident])) for (const incident of current) merged.set(`${incident.at}:${incident.id}`, incident) - const rows = [...merged.values()].sort((a, b) => a.at - b.at).slice(-INCIDENT_LIMIT) + // Room is left for this run's carried foreign rows, or the rewritten file + // would exceed INCIDENT_LIMIT and the next launch would reject all of it. + const room = Math.max(0, INCIDENT_LIMIT - (this.foreignIncidents.get(this.runId)?.length ?? 0)) + const rows = room ? [...merged.values()].sort((a, b) => a.at - b.at).slice(-room) : [] // Memory mirrors disk: a capacity-shortened write keeps the previous rows, // which is what status, queries and the next launch will actually find. - if (!(await this.replaceBounded(join(this.runDir, 'incidents.json'), JSON.stringify(rows), INCIDENT_BUDGET))) return + if (!(await this.replaceBounded(join(this.runDir, 'incidents.json'), this.incidentFileBody(this.runId, rows), INCIDENT_BUDGET))) return this.incidentRuns.set(this.runId, rows) await this.enforceIncidentLimit() } @@ -545,7 +561,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.incidentRuns.has(run) || this.foreignIncidents.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() @@ -580,11 +596,12 @@ export class MonitorHistoryStore { private async writeRunIncidents(run: string, rows: MonitorIncident[]): Promise { const file = join(this.root, RUNS_DIR, run, 'incidents.json') - if (rows.length) { - if (await this.replaceBounded(file, JSON.stringify(rows), INCIDENT_BUDGET)) this.incidentRuns.set(run, rows) - } else { + if (!rows.length && !this.foreignIncidents.has(run)) { await rm(file, { force: true }) this.incidentRuns.delete(run) + } else if (await this.replaceBounded(file, this.incidentFileBody(run, rows), INCIDENT_BUDGET)) { + if (rows.length) this.incidentRuns.set(run, rows) + else this.incidentRuns.delete(run) } } @@ -691,14 +708,27 @@ export class MonitorHistoryStore { } catch { return null } } - private async readIncidentFile(file: string): Promise { + private incidentFileBody(run: string, rows: MonitorIncident[]): string { + return JSON.stringify([...rows, ...(this.foreignIncidents.get(run) ?? [])]) + } + + private async readIncidentFile(file: string, run: string): Promise { try { + // Whole-file refusals stay whole-file: an oversized or non-array file is + // not "one row this build does not know" but a file it cannot trust at + // all, and nothing here rewrites it (the run has no incidentRuns entry). if ((await stat(file)).size > INCIDENT_BUDGET) { this.degraded = true; return [] } const value: unknown = JSON.parse(await readFile(file, 'utf8')) if (!Array.isArray(value) || value.length > INCIDENT_LIMIT) { this.degraded = true; return [] } - const parsed = value.map(parseMonitorIncident) - if (parsed.some(incident => incident === null)) { this.degraded = true; return [] } - return parsed as MonitorIncident[] + const rows: MonitorIncident[] = [] + const foreign: unknown[] = [] + for (const row of value) { + const incident = parseMonitorIncident(row) + if (incident) rows.push(incident) + else foreign.push(row) + } + if (foreign.length) { this.degraded = true; this.foreignIncidents.set(run, foreign) } + return rows } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') this.degraded = true return [] @@ -753,6 +783,7 @@ export class MonitorHistoryStore { await rm(join(this.root, RUNS_DIR, run), { recursive: true, force: true }) for (const [file, entry] of [...this.index]) if (entry.run === run) { this.index.delete(file); this.repairedTails.delete(file) } this.incidentRuns.delete(run) + this.foreignIncidents.delete(run) this.unindexedRuns.delete(run) total = Math.max(0, total - size); this.shortened = true } From 77384ab301517a56684a44e79b82456f4811d1fb Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 04/16] fix(workflows): one invalid source approval no longer blocks every workflow (#1251 row 11) Co-Authored-By: Claude Opus 5.5 --- .../WorkflowSourceApprovalStore.test.ts | 32 ++++++++++++++++++- .../workflows/WorkflowSourceApprovalStore.ts | 27 ++++++++++++---- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/main/workflows/WorkflowSourceApprovalStore.test.ts b/src/main/workflows/WorkflowSourceApprovalStore.test.ts index ab847797d..986d99daa 100644 --- a/src/main/workflows/WorkflowSourceApprovalStore.test.ts +++ b/src/main/workflows/WorkflowSourceApprovalStore.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp } from 'node:fs/promises' +import { mkdtemp, readFile, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -36,4 +36,34 @@ describe('WorkflowSourceApprovalStore', () => { .resolves.toBe(false) expect(shouldNotPrompt).toHaveBeenCalledOnce() }) + + it('honours valid approvals beside an unreadable entry and never approves or drops that entry (#1251 row 11)', async () => { + // One bad entry used to throw from load() on every authorize(), so every + // repository workflow failed until the user hand-edited the file. + const root = await mkdtemp(join(tmpdir(), 'workflow-source-approval-')) + const filePath = join(root, 'approvals.json') + const unreadable = { canonicalIdentity: '/repo/.claude/workflows/other.js', sourceHash: 'not-a-sha', approvedAt: '2026-09-01T00:00:00.000Z' } + await writeFile(filePath, JSON.stringify({ + version: 1, + approvals: [ + { canonicalIdentity: request.canonicalIdentity, sourceHash: request.sourceHash, approvedAt: '2026-09-01T00:00:00.000Z' }, + unreadable, + ], + })) + const store = new WorkflowSourceApprovalStore(filePath) + const shouldNotPrompt = vi.fn(async () => false) + await expect(store.authorize(request, shouldNotPrompt)).resolves.toBe(true) + expect(shouldNotPrompt).not.toHaveBeenCalled() + + // The unreadable entry grants nothing: its source still needs a prompt. + const approve = vi.fn(async () => true) + const other = { ...request, canonicalIdentity: unreadable.canonicalIdentity, sourceHash: 'c'.repeat(64) } + await expect(store.authorize(other, approve)).resolves.toBe(true) + expect(approve).toHaveBeenCalledOnce() + + // Persisting the new grant keeps the entry this build could not read. + const written = JSON.parse(await readFile(filePath, 'utf8')) as { approvals: unknown[] } + expect(written.approvals).toContainEqual(unreadable) + expect(written.approvals).toHaveLength(3) + }) }) diff --git a/src/main/workflows/WorkflowSourceApprovalStore.ts b/src/main/workflows/WorkflowSourceApprovalStore.ts index e947baff3..93c7d097b 100644 --- a/src/main/workflows/WorkflowSourceApprovalStore.ts +++ b/src/main/workflows/WorkflowSourceApprovalStore.ts @@ -12,7 +12,7 @@ type StoredApproval = { type StoredApprovalFile = { version: 1 - approvals: StoredApproval[] + approvals: unknown[] } /** @@ -25,6 +25,17 @@ type StoredApprovalFile = { export class WorkflowSourceApprovalStore { private readonly filePath: string private readonly approvals = new Map() + // Entries this build could not read, carried verbatim (#1251 row 11). + // WHY skip-and-carry instead of the old throw: load() threw on the first bad + // entry and never set `loaded`, so every authorize() rethrew and one bad row + // blocked every repository workflow until the file was hand-edited. Skipping + // still fails closed where it matters: an unreadable entry never enters + // `approvals`, so its source is prompted for again exactly like a new one. + // WHY carry rather than drop: persist() rewrites the whole file on the next + // grant, and dropping would silently delete a record a newer build may + // understand. The whole-file version check below still throws, because a + // file of an unknown version is not one bad row. + private readonly unreadable: unknown[] = [] private readonly prompts = new Map>() private loaded = false @@ -83,7 +94,8 @@ export class WorkflowSourceApprovalStore { !/^[a-f0-9]{64}$/.test(value.sourceHash) || typeof value.approvedAt !== 'string' ) { - throw new Error(`Workflow source approval entry is invalid: ${this.filePath}`) + this.unreadable.push(value) + continue } const approval = value as StoredApproval this.approvals.set(approvalKey(approval.canonicalIdentity, approval.sourceHash), approval) @@ -97,10 +109,13 @@ export class WorkflowSourceApprovalStore { const temporary = `${this.filePath}.tmp-${process.pid}-${randomUUID()}` const document: StoredApprovalFile = { version: 1, - approvals: [...this.approvals.values()].sort((left, right) => ( - left.canonicalIdentity.localeCompare(right.canonicalIdentity) || - left.sourceHash.localeCompare(right.sourceHash) - )), + approvals: [ + ...[...this.approvals.values()].sort((left, right) => ( + left.canonicalIdentity.localeCompare(right.canonicalIdentity) || + left.sourceHash.localeCompare(right.sourceHash) + )), + ...this.unreadable, + ], } try { await writeFile(temporary, `${JSON.stringify(document)}\n`, { encoding: 'utf8', mode: 0o600 }) From bb4c15741771bec1ac3407bf106e06e1bc909a7a Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 05/16] fix(tldr): drop invalid identities from a read batch instead of failing it (#1251 row 12) Co-Authored-By: Claude Opus 5.5 --- src/main/tldr/ipc.test.ts | 71 +++++++++++++++++++++++++++++++++++++++ src/main/tldr/ipc.ts | 12 ++++++- 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 src/main/tldr/ipc.test.ts diff --git a/src/main/tldr/ipc.test.ts b/src/main/tldr/ipc.test.ts new file mode 100644 index 000000000..79387c5d8 --- /dev/null +++ b/src/main/tldr/ipc.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it, vi } from 'vitest' + +import type { TldrStore } from './TldrStore.js' +import type { TldrEnforcement } from './enforcement.js' + +// vi.hoisted, because vi.mock factories are hoisted above ordinary consts. +const { handlers } = vi.hoisted(() => ({ + handlers: new Map unknown>(), +})) + +vi.mock('electron', () => ({ + ipcMain: { + handle: (channel: string, handler: (event: unknown, ...args: unknown[]) => unknown) => { handlers.set(channel, handler) }, + on: () => {}, + }, + systemPreferences: {}, +})) +vi.mock('@main/window/windowRegistry.js', () => ({ + windowIdFor: () => 'window-one', + getBrowserWindow: () => ({ id: 1 }), + broadcastToWindows: () => {}, +})) +vi.mock('@main/dictation/macHotkeyHelper.js', () => ({ ensureMacHotkeyHelperBinary: async () => null })) +vi.mock('./holdRelease.js', () => ({ watchMacTldrRelease: () => () => {} })) + +const { registerGoalIpc, registerTldrIpc } = await import('./ipc.js') + +function fakes() { + const read = vi.fn(async (identities: string[]) => Object.fromEntries(identities.map(id => [id, { text: `tldr of ${id}` }]))) + const status = vi.fn((identities: string[]) => Object.fromEntries(identities.map(id => [id, 'active']))) + const store = { read, on: () => {} } as unknown as TldrStore + const enforcement = { status } as unknown as Pick + return { store, enforcement, read, status } +} + +const sender = { mainFrame: {} } +const event = { sender, senderFrame: sender.mainFrame } + +describe('TLDR read IPC batches (#1251 row 12)', () => { + // One identity outside the stored-identity alphabet used to fail zod's + // array parse, so Agent Activity's single batched read rejected and every + // TLDR (and goal) on screen went blank. A TLDR can only ever be written under + // a valid identity (the MCP writer validates the same predicate), so an + // invalid one has no record to return: dropping it from the batch answers it + // exactly as the store would, "none", without taking the rest down. + it('answers the valid identities of a batch that also holds an invalid one', async () => { + const { store, enforcement, read, status } = fakes() + registerTldrIpc(store, enforcement) + registerGoalIpc(store) + const batch = ['session-1', 'not a/valid identity', 'session-2'] + + await expect(handlers.get('tldr:read')!(event, batch)).resolves.toEqual({ + 'session-1': { text: 'tldr of session-1' }, + 'session-2': { text: 'tldr of session-2' }, + }) + expect(read).toHaveBeenLastCalledWith(['session-1', 'session-2']) + await handlers.get('goal:read')!(event, batch) + expect(read).toHaveBeenLastCalledWith(['session-1', 'session-2']) + await handlers.get('tldr:enforcement')!(event, batch) + expect(status).toHaveBeenLastCalledWith(['session-1', 'session-2']) + }) + + it('still refuses a payload that is not a bounded list of strings', async () => { + const { store, enforcement } = fakes() + registerTldrIpc(store, enforcement) + expect(() => handlers.get('tldr:read')!(event, 'session-1')).toThrow() + expect(() => handlers.get('tldr:read')!(event, [42])).toThrow() + expect(() => handlers.get('tldr:read')!(event, Array.from({ length: 10_001 }, (_, i) => `s${i}`))).toThrow() + expect(() => handlers.get('tldr:read')!(event, ['x'.repeat(100_000)])).toThrow() + }) +}) diff --git a/src/main/tldr/ipc.ts b/src/main/tldr/ipc.ts index f2f9c4ed4..c2da8f4d6 100644 --- a/src/main/tldr/ipc.ts +++ b/src/main/tldr/ipc.ts @@ -16,7 +16,17 @@ function assertApplicationWindow(event: Electron.IpcMainInvokeEvent): void { } } -const identityList = z.array(z.string().refine(validTldrIdentity)).max(10_000) +// WHY invalid identities are dropped from a batch instead of failing it +// (#1251 row 12): Agent Activity reads every visible agent's TLDR and goal in +// ONE batch, and a single identity outside the alphabet used to reject the +// whole parse and blank every row. Dropping is exact, not lenient: TldrStore +// only ever writes under identities that pass validTldrIdentity, so an invalid +// one has no record, and "absent from the result" is the answer the store +// would give it anyway. The shape stays strict (a bounded array of bounded +// strings), so a malformed payload is still refused outright. The 256-char +// element cap only bounds what zod copies; the predicate's own limit is 128. +const identityList = z.array(z.string().max(256)).max(10_000) + .transform(identities => identities.filter(validTldrIdentity)) const singleIdentity = z.string().refine(validTldrIdentity) /** From 129f8c5a85b270a7286070e6d29c61f3302f43ab Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 06/16] fix(storage): one malformed legacy ledger row no longer stops debug pruning (#1251 row 13) Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 32 ++++++++++++++++++++++++- src/main/storage/debugRetention.ts | 23 +++++++++++++----- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index e4c0683f7..4dcca5395 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { collectSessionRecordingDirs, runPrunePasses } from './debugRetention.js' +import { collectSessionRecordingDirs, parseManualLegacyBundlePaths, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -226,3 +226,33 @@ describe('runPrunePasses', () => { expect(result).toEqual({ removed: 0, bytesFreed: 0, remainingBytes: 500 }) }) }) + +describe('parseManualLegacyBundlePaths (#1251 row 13)', () => { + // The legacy ledger is append-only JSONL written across many app versions. + // A row that parses as JSON but is not a saved-entry object (a bare `null`, + // a number, an entry without a string bundlePath) used to throw out of the + // loop (`null.event`, `resolve(undefined)`), which rejected collectArtifacts + // and so stopped EVERY prune pass, for every bucket, on every trigger. + it('keeps every readable manual row and skips rows that are not saved-entry objects', () => { + const raw = [ + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-01T00-00-00' }), + 'null', + '42', + '"saved"', + JSON.stringify({ event: 'saved', reason: 'manual' }), + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: 42 }), + JSON.stringify({ event: 'saved', reason: 7, bundlePath: '/bundles/2026-01-03T00-00-00' }), + '{not json', + JSON.stringify({ event: 'saved', reason: 'autosave-crash', bundlePath: '/bundles/2026-01-02T00-00-00' }), + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-04T00-00-00' }), + ].join('\n') + expect([...parseManualLegacyBundlePaths(raw)]).toEqual([ + '/bundles/2026-01-01T00-00-00', + // A non-string reason is not an autosave label, and an unlabelled save + // was user-triggered in the versions that wrote this ledger, so it stays + // protected: when in doubt, retention keeps the bundle. + '/bundles/2026-01-03T00-00-00', + '/bundles/2026-01-04T00-00-00', + ]) + }) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 8bdaace51..6c4065811 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -625,25 +625,36 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { } async function loadManualLegacyBundlePaths(): Promise> { - const manual = new Set() let raw: string try { raw = await readFile(DEBUG_BUNDLE_LOG_FILE, 'utf8') } catch { - return manual + return new Set() } + return parseManualLegacyBundlePaths(raw) +} +export function parseManualLegacyBundlePaths(raw: string): Set { + const manual = new Set() for (const line of raw.split('\n')) { const trimmed = line.trim() if (!trimmed) continue - let entry: DebugBundleLogEntry + let parsed: unknown try { - entry = JSON.parse(trimmed) as DebugBundleLogEntry + parsed = JSON.parse(trimmed) } catch { continue } - if (entry.event !== 'saved') continue - if (isAutosaveDebugBundleReason(entry.reason)) continue + // WHY a shape check and not only the JSON.parse guard (#1251 row 13): a + // line can be valid JSON and still not an entry (`null`, a number, a row + // from a build that wrote bundlePath differently). Such a row threw here, + // which rejected collectArtifacts and stopped every prune pass for every + // bucket. Skipping it can only fail to protect a bundle the row does not + // name, so it never exposes a manual bundle to deletion. + if (typeof parsed !== 'object' || parsed === null) continue + const entry = parsed as Partial & { bundlePath?: unknown; reason?: unknown } + if (entry.event !== 'saved' || typeof entry.bundlePath !== 'string') continue + if (isAutosaveDebugBundleReason(typeof entry.reason === 'string' ? entry.reason : null)) continue // WHY manual legacy classification comes from the old mixed ledger instead // of folder contents: every bundle contains a manifest, but reading // thousands of manifests during retention would turn a cheap directory From 56d60268ca6755f60060abc6a58b2dd759510da8 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:19:32 -0700 Subject: [PATCH 07/16] revert(storage): row 13 moves to #1417 (manager q109) Row 13 of #1251 and steering q109 on #1417 both change loadManualLegacyBundlePaths, so the row ships with #1417 (commit 02df3652 there), with a test through the real loader. This PR no longer touches debugRetention. Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 32 +------------------------ src/main/storage/debugRetention.ts | 23 +++++------------- 2 files changed, 7 insertions(+), 48 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index 4dcca5395..e4c0683f7 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { collectSessionRecordingDirs, parseManualLegacyBundlePaths, runPrunePasses } from './debugRetention.js' +import { collectSessionRecordingDirs, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -226,33 +226,3 @@ describe('runPrunePasses', () => { expect(result).toEqual({ removed: 0, bytesFreed: 0, remainingBytes: 500 }) }) }) - -describe('parseManualLegacyBundlePaths (#1251 row 13)', () => { - // The legacy ledger is append-only JSONL written across many app versions. - // A row that parses as JSON but is not a saved-entry object (a bare `null`, - // a number, an entry without a string bundlePath) used to throw out of the - // loop (`null.event`, `resolve(undefined)`), which rejected collectArtifacts - // and so stopped EVERY prune pass, for every bucket, on every trigger. - it('keeps every readable manual row and skips rows that are not saved-entry objects', () => { - const raw = [ - JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-01T00-00-00' }), - 'null', - '42', - '"saved"', - JSON.stringify({ event: 'saved', reason: 'manual' }), - JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: 42 }), - JSON.stringify({ event: 'saved', reason: 7, bundlePath: '/bundles/2026-01-03T00-00-00' }), - '{not json', - JSON.stringify({ event: 'saved', reason: 'autosave-crash', bundlePath: '/bundles/2026-01-02T00-00-00' }), - JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-04T00-00-00' }), - ].join('\n') - expect([...parseManualLegacyBundlePaths(raw)]).toEqual([ - '/bundles/2026-01-01T00-00-00', - // A non-string reason is not an autosave label, and an unlabelled save - // was user-triggered in the versions that wrote this ledger, so it stays - // protected: when in doubt, retention keeps the bundle. - '/bundles/2026-01-03T00-00-00', - '/bundles/2026-01-04T00-00-00', - ]) - }) -}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 6c4065811..8bdaace51 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -625,36 +625,25 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { } async function loadManualLegacyBundlePaths(): Promise> { + const manual = new Set() let raw: string try { raw = await readFile(DEBUG_BUNDLE_LOG_FILE, 'utf8') } catch { - return new Set() + return manual } - return parseManualLegacyBundlePaths(raw) -} -export function parseManualLegacyBundlePaths(raw: string): Set { - const manual = new Set() for (const line of raw.split('\n')) { const trimmed = line.trim() if (!trimmed) continue - let parsed: unknown + let entry: DebugBundleLogEntry try { - parsed = JSON.parse(trimmed) + entry = JSON.parse(trimmed) as DebugBundleLogEntry } catch { continue } - // WHY a shape check and not only the JSON.parse guard (#1251 row 13): a - // line can be valid JSON and still not an entry (`null`, a number, a row - // from a build that wrote bundlePath differently). Such a row threw here, - // which rejected collectArtifacts and stopped every prune pass for every - // bucket. Skipping it can only fail to protect a bundle the row does not - // name, so it never exposes a manual bundle to deletion. - if (typeof parsed !== 'object' || parsed === null) continue - const entry = parsed as Partial & { bundlePath?: unknown; reason?: unknown } - if (entry.event !== 'saved' || typeof entry.bundlePath !== 'string') continue - if (isAutosaveDebugBundleReason(typeof entry.reason === 'string' ? entry.reason : null)) continue + if (entry.event !== 'saved') continue + if (isAutosaveDebugBundleReason(entry.reason)) continue // WHY manual legacy classification comes from the old mixed ledger instead // of folder contents: every bundle contains a manifest, but reading // thousands of manifests during retention would turn a cheap directory From 4cd7c706773054bf5cbd49b68441bc2df5797612 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:24:58 -0700 Subject: [PATCH 08/16] fix(performance): a wholly refused incident file is set aside, never overwritten or expired (#1411 review) Co-Authored-By: Claude Opus 5.5 --- .../performance/MonitorHistoryStore.test.ts | 72 ++++++++++++++++++- src/main/performance/MonitorHistoryStore.ts | 57 ++++++++++++--- 2 files changed, 118 insertions(+), 11 deletions(-) diff --git a/src/main/performance/MonitorHistoryStore.test.ts b/src/main/performance/MonitorHistoryStore.test.ts index 80eb5df73..8bdd96081 100644 --- a/src/main/performance/MonitorHistoryStore.test.ts +++ b/src/main/performance/MonitorHistoryStore.test.ts @@ -1,4 +1,4 @@ -import { appendFile, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { appendFile, mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' @@ -133,6 +133,76 @@ describe('bounded local performance history', () => { expect(onDisk.find(row => row.id === 9)).toEqual(foreign) }) + // Review of #1411 (a, b, c): a file refused WHOLE (not an array, a newer + // format, over the row limit, oversized) kept no marker, so a restarted + // helper's next incident replaced it with only the new row, and it was lost. + for (const [label, body] of [ + ['a newer-format object', JSON.stringify({ version: 2, incidents: [incident] })], + ['51 valid rows, one over the limit', JSON.stringify(Array.from({ length: 51 }, (_, index) => ({ ...incident, id: index + 1, at: 1000 + index })))], + ['malformed JSON', '[{"id":1,'], + ] as const) { + it(`sets a wholly refused current-run incident file aside instead of overwriting it: ${label}`, async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runDir = join(root, 'runs', 'run-refused') + await mkdir(runDir, { recursive: true }) + await writeFile(join(runDir, 'incidents.json'), body) + + const restarted = new MonitorHistoryStore(root, 'run-refused') + await restarted.settled() + restarted.record(snapshot(11_000), null, [{ ...incident, id: 99, at: 11_000 }], 0, 1) + await restarted.settled() + + expect(await restarted.readIncident(11_000, 99)).toMatchObject({ rule: 'renderer-stall' }) + const aside = (await readdir(runDir)).filter(name => name.startsWith('incidents.refused-')) + expect(aside).toHaveLength(1) + expect(await readFile(join(runDir, aside[0]!), 'utf8')).toBe(body) + expect(restarted.status().state).toBe('degraded') + }) + } + + // Review of #1411 (a, b, c), a surviving mutation: maintenance deletes a + // prior run it believes empty. A run whose incident file holds only rows + // this build cannot read, or that it refused whole, is not empty. + it('never expires a prior run whose incidents it could not read', async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const foreignOnly = join(root, 'runs', 'run-foreign-only') + const refused = join(root, 'runs', 'run-refused-whole') + await mkdir(foreignOnly, { recursive: true }) + await mkdir(refused, { recursive: true }) + await writeFile(join(foreignOnly, 'incidents.json'), JSON.stringify([{ ...incident, rule: 'rule-from-a-newer-build' }])) + await writeFile(join(refused, 'incidents.json'), JSON.stringify({ version: 2 })) + + const store = new MonitorHistoryStore(root, 'run-now') + await store.settled() + store.record(snapshot(90_000), null, [], 0, 0) + await store.settled() + + await expect(stat(foreignOnly)).resolves.toBeTruthy() + await expect(stat(refused)).resolves.toBeTruthy() + }) + + // Review of #1411 (a): with the run's file full of carried rows, a new + // readable incident has no room. Keeping the carried rows is right; hiding + // that the new one was not kept is not. + it('reports coverage as shortened when carried rows leave no room for a new incident', async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runDir = join(root, 'runs', 'run-full') + await mkdir(runDir, { recursive: true }) + await writeFile(join(runDir, 'incidents.json'), JSON.stringify(Array.from({ length: 50 }, (_, index) => ({ ...incident, id: index + 1, rule: 'rule-from-a-newer-build' })))) + + const store = new MonitorHistoryStore(root, 'run-full') + await store.settled() + store.record(snapshot(11_000), null, [{ ...incident, id: 99, at: 11_000 }], 0, 1) + await store.settled() + + expect(store.status().shortened).toBe(true) + const onDisk = JSON.parse(await readFile(join(runDir, 'incidents.json'), 'utf8')) as unknown[] + expect(onDisk).toHaveLength(50) + }) + it('repairs a torn append and keeps coarse tiers peak-preserving', async () => { const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) roots.push(root) diff --git a/src/main/performance/MonitorHistoryStore.ts b/src/main/performance/MonitorHistoryStore.ts index 4bead304d..2687df7bb 100644 --- a/src/main/performance/MonitorHistoryStore.ts +++ b/src/main/performance/MonitorHistoryStore.ts @@ -1,5 +1,5 @@ import { createReadStream, createWriteStream } from 'node:fs' -import { appendFile, mkdir, open, readdir, readFile, rm, stat, writeFile } from 'node:fs/promises' +import { appendFile, mkdir, open, readdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' import { once } from 'node:events' import { finished } from 'node:stream/promises' import { dirname, join } from 'node:path' @@ -77,6 +77,17 @@ export class MonitorHistoryStore { // see rows this build can vouch for; the rows leave disk only with the run // directory itself (budget pruning, retention, clear). private foreignIncidents = new Map() + // Runs whose incidents.json was refused WHOLE (not an array, over the row + // limit, oversized, malformed, unreadable) — review of #1411 (a, b, c). + // Unlike a foreign row, such a file cannot be carried row by row, and the + // first version kept no marker at all: the run then looked like it had no + // incidents, so the current run's next persistIncidents replaced the file + // with only the new rows and maintenance expired a prior run as empty. + // Invariant: a refused file is never written over. The current run sets it + // aside (setRefusedIncidentsAside) before its first write; a prior run is + // kept by maintenance and leaves disk only with its directory (budget + // pruning, clear), like foreign rows. + private refusedIncidentRuns = new Set() private repairedTails = new Set() // False until startup indexing completes. Retention deletes any run the // index does not know about, so a partial index (EPERM, ENOSPC or an I/O @@ -309,7 +320,7 @@ export class MonitorHistoryStore { try { await rm(join(this.root, RUNS_DIR), { recursive: true, force: true }) await mkdir(this.runDir, { recursive: true }) - this.index.clear(); this.incidentRuns.clear(); this.foreignIncidents.clear(); this.repairedTails.clear(); this.indexed = true + this.index.clear(); this.incidentRuns.clear(); this.foreignIncidents.clear(); this.refusedIncidentRuns.clear(); this.repairedTails.clear(); this.indexed = true this.bytes = 0; this.shortened = false; this.degraded = false this.operationFingerprint = ''; this.unindexedRuns.clear() this.lastMaintenanceAt = -Infinity @@ -422,9 +433,16 @@ export class MonitorHistoryStore { // would exceed INCIDENT_LIMIT and the next launch would reject all of it. const room = Math.max(0, INCIDENT_LIMIT - (this.foreignIncidents.get(this.runId)?.length ?? 0)) const rows = room ? [...merged.values()].sort((a, b) => a.at - b.at).slice(-room) : [] + // Carried rows are kept over new ones (the owner's "do not delete stuff"), + // but a readable incident that found no room is missing evidence, so say + // so (review of #1411, a). Trimming to INCIDENT_LIMIT itself is the normal + // cap and is not a shortfall. + if (rows.length < Math.min(merged.size, INCIDENT_LIMIT)) this.shortened = true + const file = join(this.runDir, 'incidents.json') + if (this.refusedIncidentRuns.has(this.runId) && !(await this.setRefusedIncidentsAside(file))) return // Memory mirrors disk: a capacity-shortened write keeps the previous rows, // which is what status, queries and the next launch will actually find. - if (!(await this.replaceBounded(join(this.runDir, 'incidents.json'), this.incidentFileBody(this.runId, rows), INCIDENT_BUDGET))) return + if (!(await this.replaceBounded(file, this.incidentFileBody(this.runId, rows), INCIDENT_BUDGET))) return this.incidentRuns.set(this.runId, rows) await this.enforceIncidentLimit() } @@ -561,7 +579,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.foreignIncidents.has(run) || this.unindexedRuns.has(run) || [...this.index.values()].some(entry => entry.run === run)) continue + if (run === this.runId || this.incidentRuns.has(run) || this.foreignIncidents.has(run) || this.refusedIncidentRuns.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() @@ -712,14 +730,32 @@ export class MonitorHistoryStore { return JSON.stringify([...rows, ...(this.foreignIncidents.get(run) ?? [])]) } + /** + * Moves a refused incidents.json to `incidents.refused-.json` in the same + * run directory, keeping its bytes (and counting them in the budget), so the + * run can record new incidents. False, and nothing is written, when the + * move fails: losing the new rows beats overwriting the refused ones. + */ + private async setRefusedIncidentsAside(file: string): Promise { + try { + await rename(file, join(dirname(file), `incidents.refused-${Date.now()}.json`)) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { this.degraded = true; this.shortened = true; return false } + } + this.refusedIncidentRuns.delete(this.runId) + return true + } + private async readIncidentFile(file: string, run: string): Promise { + // Whole-file refusals stay whole-file: an oversized, non-array, over-limit, + // malformed or unreadable file is not "one row this build does not know" + // but a file it cannot trust at all. It is marked refused, never rewritten; + // see refusedIncidentRuns. + const refuse = (): MonitorIncident[] => { this.degraded = true; this.refusedIncidentRuns.add(run); return [] } try { - // Whole-file refusals stay whole-file: an oversized or non-array file is - // not "one row this build does not know" but a file it cannot trust at - // all, and nothing here rewrites it (the run has no incidentRuns entry). - if ((await stat(file)).size > INCIDENT_BUDGET) { this.degraded = true; return [] } + if ((await stat(file)).size > INCIDENT_BUDGET) return refuse() const value: unknown = JSON.parse(await readFile(file, 'utf8')) - if (!Array.isArray(value) || value.length > INCIDENT_LIMIT) { this.degraded = true; return [] } + if (!Array.isArray(value) || value.length > INCIDENT_LIMIT) return refuse() const rows: MonitorIncident[] = [] const foreign: unknown[] = [] for (const row of value) { @@ -730,7 +766,7 @@ export class MonitorHistoryStore { if (foreign.length) { this.degraded = true; this.foreignIncidents.set(run, foreign) } return rows } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') this.degraded = true + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return refuse() return [] } } @@ -784,6 +820,7 @@ export class MonitorHistoryStore { for (const [file, entry] of [...this.index]) if (entry.run === run) { this.index.delete(file); this.repairedTails.delete(file) } this.incidentRuns.delete(run) this.foreignIncidents.delete(run) + this.refusedIncidentRuns.delete(run) this.unindexedRuns.delete(run) total = Math.max(0, total - size); this.shortened = true } From 0d5de99d67402e59650381f4066fbaabb0a14bdf Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:24:58 -0700 Subject: [PATCH 09/16] fix(conversations): type each Codex index row by value; count and report skipped rows and rollouts (#1411 review) Co-Authored-By: Claude Opus 5.5 --- .../sources/codex.system.test.ts | 20 ++++++ src/main/conversations/sources/codex.ts | 63 +++++++++++++++++-- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/main/conversations/sources/codex.system.test.ts b/src/main/conversations/sources/codex.system.test.ts index abd80057d..6927c8d8e 100644 --- a/src/main/conversations/sources/codex.system.test.ts +++ b/src/main/conversations/sources/codex.system.test.ts @@ -109,6 +109,9 @@ describe('Codex conversation source', () => { const indexed = await source.discover({ scope: 'everywhere', family }) expect(indexed.filter(r => r.origin === 'index').length).toBeGreaterThanOrEqual(counts.codex.inFamily) expect(indexed.filter(r => r.origin === 'scan')).toHaveLength(0) + // Review of #1411 (c): the skip must not look like a complete result. + expect(counts.codex.unindexedSampled).toBeGreaterThan(0) + expect(source.lastDowngradeReason()).toMatch(/skipped \d+ unreadable rollout/) // Fallback path: one readable rollout still lists beside unreadable ones. await chmod(rollouts[0]!, 0o600) @@ -116,6 +119,23 @@ describe('Codex conversation source', () => { const fresh = new CodexConversationSource({ codexHome: corpus.codexHome }) const scanned = await fresh.discover({ scope: 'everywhere', family }) expect(scanned.map(r => r.file)).toEqual([rollouts[0]]) + expect(fresh.lastDowngradeReason()).toMatch(/no state_N\.sqlite.*; skipped \d+ unreadable rollout/) + }) + + // Review of #1411 (b): SQLite keeps any value in any column, so one thread + // whose title is a BLOB made `.trim()` throw and rejected the whole index. + it('lists every indexed thread when one row holds a value of the wrong type (#1251 row 8)', async () => { + const { corpus, source, listWorktrees } = await setup() + const family = await resolveFamily('/fixture/repo', 'everywhere', { listWorktrees }) + const before = await source.discover({ scope: 'everywhere', family }) + const db = new DatabaseSync(join(corpus.codexHome, 'state_5.sqlite')) + const victim = (db.prepare('select id from threads where archived = 0 limit 1').get() as { id: string }).id + db.prepare("update threads set title = x'00', first_user_message = 'fallback label' where id = ?").run(victim) + db.close() + + const after = await new CodexConversationSource({ codexHome: corpus.codexHome }).discover({ scope: 'everywhere', family }) + expect(after).toHaveLength(before.length) + expect(after.find(r => r.nativeId === victim)?.userTexts).toEqual(['fallback label']) }) it('falls back to the rollout scan when the index is missing and reports why', async () => { diff --git a/src/main/conversations/sources/codex.ts b/src/main/conversations/sources/codex.ts index 5735586fb..14a490a3f 100644 --- a/src/main/conversations/sources/codex.ts +++ b/src/main/conversations/sources/codex.ts @@ -72,6 +72,39 @@ type RolloutHead = { lastUserAt: number | null } +/** + * One `threads` row, typed by value rather than trusted by column (review of + * #1411, b). SQLite stores any value in any column whatever its declared type, + * so a BLOB title made `(row.title ?? '').trim()` throw, and that one row + * rejected the whole Codex discovery. A field of the wrong type becomes its + * empty value (a title then falls back to the next label); only a row with no + * string id is dropped, because nothing can address it. + */ +function normalizeIndexRow(raw: Record): IndexRow | null { + const text = (value: unknown): string | null => typeof value === 'string' ? value : null + const num = (value: unknown): number | null => typeof value === 'number' && Number.isFinite(value) ? value : null + const id = text(raw.id) + if (!id) return null + return { + id, + rollout_path: text(raw.rollout_path) ?? '', + cwd: text(raw.cwd) ?? '', + title: text(raw.title), + first_user_message: text(raw.first_user_message), + preview: text(raw.preview), + name: text(raw.name), + source: text(raw.source) ?? '', + thread_source: text(raw.thread_source), + agent_role: text(raw.agent_role), + git_branch: text(raw.git_branch), + created_at_ms: num(raw.created_at_ms), + updated_at_ms: num(raw.updated_at_ms), + recency_at_ms: num(raw.recency_at_ms), + archived: num(raw.archived) ?? 0, + originator: text(raw.originator), + } +} + function isSubagentSource(row: IndexRow): boolean { if (row.thread_source === 'subagent') return true if (row.agent_role) return true @@ -118,6 +151,12 @@ async function readRolloutHead(file: string): Promise { export class CodexConversationSource implements ConversationSource { readonly provider = 'codex' as const private downgradeReason: string | null = null + // What one discovery skipped (review of #1411, c): a skipped rollout or index + // row used to leave the result looking complete. The counts reach the + // discovery span, lastDowngradeReason and one console warning (counts only, + // never paths). The picker itself has no degraded indicator for any source + // yet, including the existing no-index downgrade; that is a residual. + private skipped = { rollouts: 0, indexRows: 0 } private walk: { at: number; files: Map } | null = null private readonly heads = new Map() // Rollout paths learnt at discovery, so a search that reads prompts for a @@ -183,6 +222,7 @@ export class CodexConversationSource implements ConversationSource { try { head = await readRolloutHead(file) } catch { + this.skipped.rollouts++ return null } } @@ -211,16 +251,25 @@ export class CodexConversationSource implements ConversationSource { } } + private withSkipped(reason: string | null): string | null { + const { rollouts, indexRows } = this.skipped + if (!rollouts && !indexRows) return reason + const note = `skipped ${rollouts} unreadable rollout(s) and ${indexRows} malformed index row(s)` + console.warn(`[conversations.codex] ${note}`) + return reason ? `${reason}; ${note}` : note + } + async discover(scope: SourceScope): Promise { const span = performanceService.span('conversations.codex.discover', { scope: scope.scope }) + this.skipped = { rollouts: 0, indexRows: 0 } const dbPath = newestCodexStateDb(this.deps.codexHome) const opened = dbPath ? openReadOnlySqlite(dbPath, CODEX_INDEX_COLUMNS) : { ok: false as const, reason: `no state_N.sqlite under ${this.deps.codexHome}` } if (!opened.ok) { - this.downgradeReason = opened.reason const rows = await this.scanEverything(scope) - span.end({ mode: 'scan', rows: rows.length }) + this.downgradeReason = this.withSkipped(opened.reason) + span.end({ mode: 'scan', rows: rows.length, ...this.skipped }) return rows } this.downgradeReason = null @@ -263,7 +312,12 @@ export class CodexConversationSource implements ConversationSource { } const where = predicates.length > 0 ? `where archived = 0 and (${predicates.join(' or ')})` : 'where archived = 0' const columns = CODEX_INDEX_COLUMNS.threads.map(c => `"${c}"`).join(', ') - for (const row of opened.db.prepare(`select ${columns} from threads ${where}`).all(...args) as unknown as IndexRow[]) { + for (const raw of opened.db.prepare(`select ${columns} from threads ${where}`).all(...args) as Array>) { + const row = normalizeIndexRow(raw) + if (!row) { + this.skipped.indexRows++ + continue + } this.rolloutPaths.set(row.id, row.rollout_path) const title = (row.title ?? '').trim() || (row.first_user_message ?? '').trim() || (row.preview ?? '').trim() const name = (row.name ?? '').trim() @@ -302,7 +356,8 @@ export class CodexConversationSource implements ConversationSource { const row = await this.fromHead(file, meta.mtime, meta.id, scope) if (row) rows.push(row) } - span.end({ mode: 'index', rows: rows.length, unindexed: rows.filter(r => r.origin === 'scan').length }) + this.downgradeReason = this.withSkipped(null) + span.end({ mode: 'index', rows: rows.length, unindexed: rows.filter(r => r.origin === 'scan').length, ...this.skipped }) return rows } From 6077c0a059f0f38fc2adf7da3ea4fd67384addcc Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:24:58 -0700 Subject: [PATCH 10/16] fix(tldr): history answers an invalid identity with an empty list (#1411 review) Co-Authored-By: Claude Opus 5.5 --- src/main/tldr/ipc.test.ts | 17 +++++++++++++++-- src/main/tldr/ipc.ts | 15 +++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/main/tldr/ipc.test.ts b/src/main/tldr/ipc.test.ts index 79387c5d8..ab8f685e5 100644 --- a/src/main/tldr/ipc.test.ts +++ b/src/main/tldr/ipc.test.ts @@ -28,9 +28,10 @@ const { registerGoalIpc, registerTldrIpc } = await import('./ipc.js') function fakes() { const read = vi.fn(async (identities: string[]) => Object.fromEntries(identities.map(id => [id, { text: `tldr of ${id}` }]))) const status = vi.fn((identities: string[]) => Object.fromEntries(identities.map(id => [id, 'active']))) - const store = { read, on: () => {} } as unknown as TldrStore + const history = vi.fn(async (identity: string) => [{ identity }]) + const store = { read, history, on: () => {} } as unknown as TldrStore const enforcement = { status } as unknown as Pick - return { store, enforcement, read, status } + return { store, enforcement, read, status, history } } const sender = { mainFrame: {} } @@ -68,4 +69,16 @@ describe('TLDR read IPC batches (#1251 row 12)', () => { expect(() => handlers.get('tldr:read')!(event, Array.from({ length: 10_001 }, (_, i) => `s${i}`))).toThrow() expect(() => handlers.get('tldr:read')!(event, ['x'.repeat(100_000)])).toThrow() }) + + it('answers history for an invalid identity with an empty list, as the batch reads do', async () => { + const { store, enforcement, history } = fakes() + registerTldrIpc(store, enforcement) + registerGoalIpc(store) + for (const channel of ['tldr:history', 'goal:history']) { + await expect(handlers.get(channel)!(event, 'not a/valid identity')).resolves.toEqual([]) + await expect(handlers.get(channel)!(event, 'session-1')).resolves.toEqual([{ identity: 'session-1' }]) + expect(() => handlers.get(channel)!(event, 42)).toThrow() + } + expect(history).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/main/tldr/ipc.ts b/src/main/tldr/ipc.ts index c2da8f4d6..e58fd6c60 100644 --- a/src/main/tldr/ipc.ts +++ b/src/main/tldr/ipc.ts @@ -27,7 +27,15 @@ function assertApplicationWindow(event: Electron.IpcMainInvokeEvent): void { // element cap only bounds what zod copies; the predicate's own limit is 128. const identityList = z.array(z.string().max(256)).max(10_000) .transform(identities => identities.filter(validTldrIdentity)) -const singleIdentity = z.string().refine(validTldrIdentity) +const singleIdentity = z.string().max(256) +// History answers an invalid identity the way the batch reads do (review of +// #1411, b): it cannot have a record, so its history is empty, not an error +// that puts the history modal into its failure state. A non-string or +// oversized payload is still refused by the parse. +const historyFor = (store: TldrStore, raw: unknown) => { + const identity = singleIdentity.parse(raw) + return validTldrIdentity(identity) ? store.history(identity) : Promise.resolve([]) +} /** * Record an unobservable hold ONCE per app run. @@ -151,14 +159,13 @@ export function registerTldrIpc( if (hold && hold.token === token) hold.cancel() }) const identities = identityList - const identity = singleIdentity ipcMain.handle('tldr:read', (event, raw: unknown) => { assertApplicationWindow(event) return store.read(identities.parse(raw)) }) ipcMain.handle('tldr:history', (event, raw: unknown) => { assertApplicationWindow(event) - return store.history(identity.parse(raw)) + return historyFor(store, raw) }) // Read-only, like every renderer TLDR API: whether this identity's provider // hooks have reached main. The renderer uses it to say when enforcement is not @@ -184,7 +191,7 @@ export function registerGoalIpc(store: TldrStore): void { }) ipcMain.handle('goal:history', (event, raw: unknown) => { assertApplicationWindow(event) - return store.history(singleIdentity.parse(raw)) + return historyFor(store, raw) }) store.on('changed', (update: TldrUpdate) => broadcastToWindows('goal:changed', update)) } From 18d7da87552d3f73a40c83821fdef32bb0c91cb1 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:24:58 -0700 Subject: [PATCH 11/16] test(workflows): an approval entry missing approvedAt prompts (#1411 review survivor) Co-Authored-By: Claude Opus 5.5 --- .../workflows/WorkflowSourceApprovalStore.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/main/workflows/WorkflowSourceApprovalStore.test.ts b/src/main/workflows/WorkflowSourceApprovalStore.test.ts index 986d99daa..02f36bcfc 100644 --- a/src/main/workflows/WorkflowSourceApprovalStore.test.ts +++ b/src/main/workflows/WorkflowSourceApprovalStore.test.ts @@ -66,4 +66,19 @@ describe('WorkflowSourceApprovalStore', () => { expect(written.approvals).toContainEqual(unreadable) expect(written.approvals).toHaveLength(3) }) + + // Review of #1411 (a), a surviving mutation: dropping the approvedAt check + // still passed. An entry for the exact source that lacks approvedAt is not a + // grant this build wrote, so it must prompt, never authorize. + it('does not treat an entry missing approvedAt as an approval', async () => { + const root = await mkdtemp(join(tmpdir(), 'workflow-source-approval-')) + const filePath = join(root, 'approvals.json') + await writeFile(filePath, JSON.stringify({ + version: 1, + approvals: [{ canonicalIdentity: request.canonicalIdentity, sourceHash: request.sourceHash }], + })) + const prompt = vi.fn(async () => false) + await expect(new WorkflowSourceApprovalStore(filePath).authorize(request, prompt)).resolves.toBe(false) + expect(prompt).toHaveBeenCalledOnce() + }) }) From 7204503c1c49440357822f9db662251c31856b56 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:24:58 -0700 Subject: [PATCH 12/16] docs(plans): #1411 round-1 review disposition Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c5-fail-all-batch.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/plans/2026-09-27-c5-fail-all-batch.md b/docs/plans/2026-09-27-c5-fail-all-batch.md index fdf919351..cebbd243b 100644 --- a/docs/plans/2026-09-27-c5-fail-all-batch.md +++ b/docs/plans/2026-09-27-c5-fail-all-batch.md @@ -31,3 +31,17 @@ Rows 14–15 (key vault index, agent-name registry, tmux recovery) are strict by - Carried foreign rows never expire on their own. They leave disk only with their run directory (budget pruning, clear). - A run whose incident file holds only foreign rows is kept from expiry-by-emptiness. It is still pruned by the data budget. - **Row 12:** a renderer that sends an invalid identity gets no record and no error for it. That is the same answer as "no TLDR yet". + +## Review round 1 (a, b, c codex at `129f8c5a`): all FIX-BEFORE-MERGE + +| Finding | Verdict | Change | +|---|---|---| +| **a1 / b1 / c1, major:** a WHOLLY refused incident file (a newer-format object, over the row limit, oversized, malformed JSON, unreadable) kept no marker. The current run's next incident replaced it with only the new row, and maintenance expired a prior run holding one as empty. | valid | The run is marked refused. The current run moves the refused file aside to `incidents.refused-.json` (same run dir, counted in the byte budget) before its first write, and writes nothing if the move fails. Maintenance keeps refused runs. Three fail-first cases (object, 51 rows, malformed JSON). | +| **a and c survivor:** the foreign-only prior-run expiry guard was unpinned. | valid | The prior-run test covers a foreign-only run and a refused run; removing either guard goes red. | +| **b2, major:** one indexed row with a wrong-typed value (a BLOB title) made `.trim()` throw and rejected the whole index. | valid | `normalizeIndexRow` types each field by value. A wrong type becomes the empty value, so the title falls back; only a row with no string id is dropped (and counted). Fail-first with a BLOB title on the recorded corpus: the row still lists under its fallback label. | +| **c2, minor:** a skipped rollout left discovery looking complete. | valid | Skips are counted into the discovery span, `lastDowngradeReason` and one console warning (counts only). **Residual:** the picker has no degraded indicator for any source yet, including the existing no-index downgrade. | +| **b3, minor:** `tldr:history` and `goal:history` threw for an invalid identity that the batch reads skip. | valid | Empty history for an invalid identity; a non-string payload is still refused. Mutant red. | +| **a3, minor:** with the file full of carried rows, a new incident was silently not kept. | valid | Carried rows still win (owner rule), but `shortened` is set. Fail-first. | +| **a survivor:** dropping the `approvedAt` check passed. | valid | An entry missing `approvedAt` prompts. Mutant red. | +| **b survivor:** the row 13 loader returning an empty set passed. | valid | Row 13 moved to #1417 (manager q109: the same loader as steering q109), with a test through the real loader there. This PR no longer touches `debugRetention`. | +| **a2, minor:** carried rows change position on rewrite. | declined | Values are all kept. Neither reader gives order any authority: incidents are sorted by `at`, and approvals are keyed by identity plus hash. Preserving the original interleaving would need positional bookkeeping for no reader. | From c38a67a64391960eb27d5727f7861af612c7d218 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:02:34 -0700 Subject: [PATCH 13/16] fix(performance): a set-aside refused file keeps its run and never collides (#1411 review round 2) Co-Authored-By: Claude Opus 5.5 --- .../performance/MonitorHistoryStore.test.ts | 93 ++++++++++++++++++- src/main/performance/MonitorHistoryStore.ts | 24 ++++- 2 files changed, 111 insertions(+), 6 deletions(-) diff --git a/src/main/performance/MonitorHistoryStore.test.ts b/src/main/performance/MonitorHistoryStore.test.ts index 8bdd96081..d72a67754 100644 --- a/src/main/performance/MonitorHistoryStore.test.ts +++ b/src/main/performance/MonitorHistoryStore.test.ts @@ -1,7 +1,7 @@ -import { appendFile, mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises' +import { appendFile, chmod, mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import type { MonitorWorkerSnapshot } from '@shared/performance/monitorSnapshot.js' import { MonitorHistoryStore } from './MonitorHistoryStore.js' @@ -161,6 +161,95 @@ describe('bounded local performance history', () => { }) } + // Review of #1411, round 2 (a, b, c): once the refused file was set aside, + // nothing marked its run, so a later run's retention expired the run's + // readable incidents, found it empty, and deleted the directory with the + // set-aside bytes in it. + it('keeps a run holding a set-aside refused file after its readable incidents expire', async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runA = join(root, 'runs', 'run-a') + const refused = JSON.stringify({ version: 2, incidents: [incident] }) + await mkdir(runA, { recursive: true }) + await writeFile(join(runA, 'incidents.json'), refused) + const first = new MonitorHistoryStore(root, 'run-a') + await first.settled() + first.record(snapshot(11_000), null, [{ ...incident, id: 2, at: 11_000 }], 0, 1) + await first.settled() + + const later = new MonitorHistoryStore(root, 'run-b') + await later.settled() + later.record(snapshot(11_000 + 8 * 24 * 60 * 60_000), null, [], 0, 0) + await later.settled() + + const aside = (await readdir(runA)).filter(name => name.startsWith('incidents.refused-')) + expect(aside).toHaveLength(1) + expect(await readFile(join(runA, aside[0]!), 'utf8')).toBe(refused) + }) + + // Review of #1411, round 2 (a, b, c): the set-aside name was only the time, + // and rename replaces an existing file, so a second refusal in the same + // millisecond erased the first. Also pins that recording again after a + // set-aside writes normally and does not set the new file aside (a stale + // refusal marker survived mutation in round 2). + it('keeps every refused file when two refusals set aside in the same millisecond', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + vi.setSystemTime(123_456) + try { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runDir = join(root, 'runs', 'run-twice') + await mkdir(runDir, { recursive: true }) + const firstBody = JSON.stringify({ version: 2, generation: 'first' }) + const secondBody = JSON.stringify({ version: 3, generation: 'second' }) + await writeFile(join(runDir, 'incidents.json'), firstBody) + const store = new MonitorHistoryStore(root, 'run-twice') + await store.settled() + store.record(snapshot(11_000), null, [{ ...incident, id: 2, at: 11_000 }], 0, 1) + await store.settled() + store.record(snapshot(12_000), null, [{ ...incident, id: 2, at: 11_000 }, { ...incident, id: 3, at: 12_000 }], 0, 1) + await store.settled() + expect((await readdir(runDir)).filter(name => name.startsWith('incidents.refused-'))).toHaveLength(1) + const current = JSON.parse(await readFile(join(runDir, 'incidents.json'), 'utf8')) as Array<{ id: number }> + expect(current.map(row => row.id)).toEqual([2, 3]) + + await writeFile(join(runDir, 'incidents.json'), secondBody) + const restarted = new MonitorHistoryStore(root, 'run-twice') + await restarted.settled() + restarted.record(snapshot(13_000), null, [{ ...incident, id: 4, at: 13_000 }], 0, 1) + await restarted.settled() + + const aside = (await readdir(runDir)).filter(name => name.startsWith('incidents.refused-')) + const bodies = await Promise.all(aside.map(name => readFile(join(runDir, name), 'utf8'))) + expect(bodies.sort()).toEqual([firstBody, secondBody].sort()) + } finally { + vi.useRealTimers() + } + }) + + // Review of #1411, round 2 (b): the guard that writes nothing when the + // set-aside rename fails was unpinned. Losing the new rows beats + // overwriting the refused ones. + it('writes nothing over a refused file it could not set aside', async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runDir = join(root, 'runs', 'run-stuck') + await mkdir(runDir, { recursive: true }) + const refused = JSON.stringify({ version: 2 }) + await writeFile(join(runDir, 'incidents.json'), refused) + const store = new MonitorHistoryStore(root, 'run-stuck') + await store.settled() + await chmod(runDir, 0o500) + try { + store.record(snapshot(11_000), null, [{ ...incident, id: 2, at: 11_000 }], 0, 1) + await store.settled() + } finally { + await chmod(runDir, 0o700) + } + expect(await readFile(join(runDir, 'incidents.json'), 'utf8')).toBe(refused) + expect(store.status().shortened).toBe(true) + }) + // Review of #1411 (a, b, c), a surviving mutation: maintenance deletes a // prior run it believes empty. A run whose incident file holds only rows // this build cannot read, or that it refused whole, is not empty. diff --git a/src/main/performance/MonitorHistoryStore.ts b/src/main/performance/MonitorHistoryStore.ts index 2687df7bb..bf0b4fed5 100644 --- a/src/main/performance/MonitorHistoryStore.ts +++ b/src/main/performance/MonitorHistoryStore.ts @@ -2,6 +2,7 @@ import { createReadStream, createWriteStream } from 'node:fs' import { appendFile, mkdir, open, readdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' import { once } from 'node:events' import { finished } from 'node:stream/promises' +import { randomUUID } from 'node:crypto' import { dirname, join } from 'node:path' import { createInterface } from 'node:readline' import type { MonitorIncident } from '@shared/performance/monitorIncidents.js' @@ -26,6 +27,7 @@ const INCIDENT_BUDGET = 8 * 1024 * 1024 const OPERATIONS_BUDGET = 1024 * 1024 const REPORT_BUDGET = 8 * 1024 * 1024 const LINE_LIMIT = 16 * 1024 +const REFUSED_INCIDENTS_PREFIX = 'incidents.refused-' const INCIDENT_LIMIT = MONITOR_POLICY.incidentCount // About seven minutes of rolled-up points (67 per minute across tiers). A disk // that stalls longer sheds new points as "shortened" coverage instead of @@ -88,6 +90,13 @@ export class MonitorHistoryStore { // kept by maintenance and leaves disk only with its directory (budget // pruning, clear), like foreign rows. private refusedIncidentRuns = new Set() + // Runs that HOLD a set-aside `incidents.refused-*.json` (review of #1411, + // round 2). Setting a refused file aside clears refusedIncidentRuns, so the + // run then looked empty again once its readable incidents expired, and + // maintenance deleted the directory with the set-aside bytes in it. Found by + // listing each run at startup and added on every set-aside; maintenance + // never expires such a run. Only budget pruning and clear() remove it. + private refusedAsideRuns = new Set() private repairedTails = new Set() // False until startup indexing completes. Retention deletes any run the // index does not know about, so a partial index (EPERM, ENOSPC or an I/O @@ -320,7 +329,7 @@ export class MonitorHistoryStore { try { await rm(join(this.root, RUNS_DIR), { recursive: true, force: true }) await mkdir(this.runDir, { recursive: true }) - this.index.clear(); this.incidentRuns.clear(); this.foreignIncidents.clear(); this.refusedIncidentRuns.clear(); this.repairedTails.clear(); this.indexed = true + this.index.clear(); this.incidentRuns.clear(); this.foreignIncidents.clear(); this.refusedIncidentRuns.clear(); this.refusedAsideRuns.clear(); this.repairedTails.clear(); this.indexed = true this.bytes = 0; this.shortened = false; this.degraded = false this.operationFingerprint = ''; this.unindexedRuns.clear() this.lastMaintenanceAt = -Infinity @@ -376,6 +385,8 @@ export class MonitorHistoryStore { try { await this.cleanupTemps() for (const run of await this.runNames()) { + const entries = await readdir(join(this.root, RUNS_DIR, run)).catch(() => [] as string[]) + if (entries.some(name => name.startsWith(REFUSED_INCIDENTS_PREFIX))) this.refusedAsideRuns.add(run) for (const resolution of TIERS) { const file = join(this.root, RUNS_DIR, run, `${resolution}.jsonl`) try { @@ -579,7 +590,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.foreignIncidents.has(run) || this.refusedIncidentRuns.has(run) || this.unindexedRuns.has(run) || [...this.index.values()].some(entry => entry.run === run)) continue + if (run === this.runId || this.incidentRuns.has(run) || this.foreignIncidents.has(run) || this.refusedIncidentRuns.has(run) || this.refusedAsideRuns.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() @@ -731,18 +742,22 @@ export class MonitorHistoryStore { } /** - * Moves a refused incidents.json to `incidents.refused-.json` in the same + * Moves a refused incidents.json to `incidents.refused--.json` in the same * run directory, keeping its bytes (and counting them in the budget), so the * run can record new incidents. False, and nothing is written, when the * move fails: losing the new rows beats overwriting the refused ones. */ private async setRefusedIncidentsAside(file: string): Promise { try { - await rename(file, join(dirname(file), `incidents.refused-${Date.now()}.json`)) + // A UUID, not only the time (review of #1411, round 2): rename replaces an + // existing destination, so a second refusal in the same millisecond, or + // after the clock stepped back, overwrote the first set-aside file. + await rename(file, join(dirname(file), `${REFUSED_INCIDENTS_PREFIX}${Date.now()}-${randomUUID()}.json`)) } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { this.degraded = true; this.shortened = true; return false } } this.refusedIncidentRuns.delete(this.runId) + this.refusedAsideRuns.add(this.runId) return true } @@ -821,6 +836,7 @@ export class MonitorHistoryStore { this.incidentRuns.delete(run) this.foreignIncidents.delete(run) this.refusedIncidentRuns.delete(run) + this.refusedAsideRuns.delete(run) this.unindexedRuns.delete(run) total = Math.max(0, total - size); this.shortened = true } From 7a51d5dfa0b792b693d13cedf7d79b8f799873a9 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:02:34 -0700 Subject: [PATCH 14/16] test(conversations): every projected Codex column survives a wrong type (#1411 review round 2 survivors) Co-Authored-By: Claude Opus 5.5 --- .../conversations/sources/codex.system.test.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/main/conversations/sources/codex.system.test.ts b/src/main/conversations/sources/codex.system.test.ts index 6927c8d8e..c37ce9bfa 100644 --- a/src/main/conversations/sources/codex.system.test.ts +++ b/src/main/conversations/sources/codex.system.test.ts @@ -129,13 +129,24 @@ describe('Codex conversation source', () => { const family = await resolveFamily('/fixture/repo', 'everywhere', { listWorktrees }) const before = await source.discover({ scope: 'everywhere', family }) const db = new DatabaseSync(join(corpus.codexHome, 'state_5.sqlite')) - const victim = (db.prepare('select id from threads where archived = 0 limit 1').get() as { id: string }).id - db.prepare("update threads set title = x'00', first_user_message = 'fallback label' where id = ?").run(victim) + const [victim, second] = (db.prepare('select id from threads where archived = 0 limit 2').all() as Array<{ id: string }>).map(row => row.id) + // Every string column the row projects, not only the title (review of + // #1411, round 2: guards on `source` and the others survived mutation). + db.prepare(`update threads set title = x'00', preview = x'00', name = x'00', source = x'00', thread_source = x'00', + agent_role = x'00', git_branch = x'00', originator = x'00', cwd = x'00', rollout_path = x'00', + created_at_ms = x'00', updated_at_ms = x'00', first_user_message = 'fallback label' where id = ?`).run(victim) + // The fallback chain itself: an empty title falls to a BLOB first message, + // which must fall through to the preview rather than throw. + db.prepare("update threads set title = '', first_user_message = x'00', preview = 'preview label' where id = ?").run(second) db.close() const after = await new CodexConversationSource({ codexHome: corpus.codexHome }).discover({ scope: 'everywhere', family }) expect(after).toHaveLength(before.length) - expect(after.find(r => r.nativeId === victim)?.userTexts).toEqual(['fallback label']) + const victimRow = after.find(r => r.nativeId === victim) + expect(victimRow?.userTexts).toEqual(['fallback label']) + expect(victimRow?.cwd).toBeNull() + expect(victimRow?.gitBranch).toBeNull() + expect(after.find(r => r.nativeId === second)?.userTexts).toEqual(['preview label']) }) it('falls back to the rollout scan when the index is missing and reports why', async () => { From 5689181222d98348171de110e08fc1927a4df1e6 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:02:35 -0700 Subject: [PATCH 15/16] docs(plans): #1411 round-2 disposition; picker residual filed as #1433 Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c5-fail-all-batch.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/plans/2026-09-27-c5-fail-all-batch.md b/docs/plans/2026-09-27-c5-fail-all-batch.md index cebbd243b..586cec716 100644 --- a/docs/plans/2026-09-27-c5-fail-all-batch.md +++ b/docs/plans/2026-09-27-c5-fail-all-batch.md @@ -45,3 +45,14 @@ Rows 14–15 (key vault index, agent-name registry, tmux recovery) are strict by | **a survivor:** dropping the `approvedAt` check passed. | valid | An entry missing `approvedAt` prompts. Mutant red. | | **b survivor:** the row 13 loader returning an empty set passed. | valid | Row 13 moved to #1417 (manager q109: the same loader as steering q109), with a test through the real loader there. This PR no longer touches `debugRetention`. | | **a2, minor:** carried rows change position on rewrite. | declined | Values are all kept. Neither reader gives order any authority: incidents are sorted by `at`, and approvals are keyed by identity plus hash. Preserving the original interleaving would need positional bookkeeping for no reader. | + +## Review round 2 (a, b, c codex at `7204503c`): all FIX-BEFORE-MERGE (final round) + +| Finding | Verdict | Change | +|---|---|---| +| **a / b / c, major:** a set-aside refused file was deleted by a later run's retention. Setting it aside cleared the refusal marker, so once the run's readable incidents expired it looked empty. | valid | `refusedAsideRuns`: every run holding an `incidents.refused-*` file (listed at startup, added on set-aside) is never expired; only budget pruning and `clear()` remove it. Fail-first with a day-8 later run. | +| **a / b / c, major:** the aside name was only `Date.now()`, and `rename` replaces, so a same-millisecond second refusal overwrote the first | valid | The name adds a UUID. Fail-first with a fixed clock and two refusals: both bodies are kept. | +| **c survivor:** removing `refusedIncidentRuns.delete` after a set-aside (a stale marker) | valid | The collision test records again after a set-aside: one aside file, and the canonical file holds both incidents. Mutant red. | +| **b survivor:** the rename-failure guard | valid | A read-only run dir makes the set-aside fail; the refused file stays byte-for-byte and `shortened` is set. Mutant red. | +| **a / c survivors:** the `source`, `first_user_message`, `cwd` and `git_branch` type guards | valid | The wrong-type test now BLOBs every projected string column, plus a second row whose empty title falls to a BLOB first message; all four mutants red. | +| **c3, minor:** the picker still shows a partial Codex list as complete | residual, **filed as #1433** | Needs a per-source degraded status through `Discovery` and a curated picker line (q39); out of scope for a fail-all fix. | From 3c807a1b915814834248d74435a50bda1cfecf2b Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:08:22 -0700 Subject: [PATCH 16/16] fix(performance): a failed run listing is unknown, not empty; the run is kept (steering q115) Startup found set-aside refused files by listing each run with .catch(() => []). A failed listing read as 'no set-aside file', so a prior run holding only one had no marker and the next maintenance deleted it with the refused bytes (the unknown-as-empty shape q109 forbade). Now the run is marked unindexed (never expired) and the store degraded; only ENOENT means nothing to find. Real temp run + one injected per-run listing failure: red at 56891812 (ENOENT on the deleted run); removing the unindexed mark is red. Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c5-fail-all-batch.md | 12 ++++ ...MonitorHistoryStore.listingFailure.test.ts | 60 +++++++++++++++++++ src/main/performance/MonitorHistoryStore.ts | 17 +++++- 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 src/main/performance/MonitorHistoryStore.listingFailure.test.ts diff --git a/docs/plans/2026-09-27-c5-fail-all-batch.md b/docs/plans/2026-09-27-c5-fail-all-batch.md index 586cec716..95bd63d0b 100644 --- a/docs/plans/2026-09-27-c5-fail-all-batch.md +++ b/docs/plans/2026-09-27-c5-fail-all-batch.md @@ -56,3 +56,15 @@ Rows 14–15 (key vault index, agent-name registry, tmux recovery) are strict by | **b survivor:** the rename-failure guard | valid | A read-only run dir makes the set-aside fail; the refused file stays byte-for-byte and `shortened` is set. Mutant red. | | **a / c survivors:** the `source`, `first_user_message`, `cwd` and `git_branch` type guards | valid | The wrong-type test now BLOBs every projected string column, plus a second row whose empty title falls to a BLOB first message; all four mutants red. | | **c3, minor:** the picker still shows a partial Codex list as complete | residual, **filed as #1433** | Needs a per-source degraded status through `Discovery` and a curated picker line (q39); out of scope for a fail-all fix. | + +## Steering q115 (after round 2) + +**Finding.** Startup found set-aside refused files by listing each run with `.catch(() => [])`. A failed listing therefore read as "no set-aside file". A prior run holding ONLY a set-aside file had no marker, so the next maintenance deleted it with the refused bytes. This is the unknown-as-empty shape q109 forbade. + +**Fix.** A failed per-run listing is unknown: the run is marked unindexed, which maintenance never expires, and the store is degraded. Only ENOENT (the run is already gone) means there is nothing to find. + +**Test.** `MonitorHistoryStore.listingFailure.test.ts` uses a real temp run holding only an `incidents.refused-*` file and injects one failed plain listing of that run at startup (the parent `runNames()` listing succeeds). Maintenance then runs after reads recover, and the run and its exact bytes must survive. +- Red at `56891812`: `ENOENT` on the deleted run. +- Removing the unindexed mark: red. + +**Loss-path audit.** Whole runs leave disk only through `clear()`, budget `pruneRuns` and maintenance expiry, and expiry is guarded by the incident, foreign, refused, set-aside and unindexed markers. The other file removals are expired tier files, an empty `incidents.json` and `*.tmp` scratch. The remaining `.catch(() => [])` listings either sweep only `*.tmp` or undercount bytes, which makes budget pruning less aggressive, never more. diff --git a/src/main/performance/MonitorHistoryStore.listingFailure.test.ts b/src/main/performance/MonitorHistoryStore.listingFailure.test.ts new file mode 100644 index 000000000..8082bff9a --- /dev/null +++ b/src/main/performance/MonitorHistoryStore.listingFailure.test.ts @@ -0,0 +1,60 @@ +import { mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' +import type { MonitorWorkerSnapshot } from '@shared/performance/monitorSnapshot.js' + +// One per-run listing fails, once, while the parent listing succeeds: the +// shape of a transient EIO/EACCES on a single run directory. Everything else +// is the real filesystem. Its own file because this mock covers the module. +const failOnce = vi.hoisted(() => ({ path: null as string | null })) +vi.mock('node:fs/promises', async importOriginal => { + const real = await importOriginal() + return { + ...real, + readdir: ((...args: Parameters) => { + // Only the plain name listing (no options): cleanupTemps and runBytes list + // the same directory with file types first and must not absorb the fault. + if (failOnce.path !== null && String(args[0]) === failOnce.path && args[1] === undefined) { + failOnce.path = null + return Promise.reject(Object.assign(new Error('injected EIO'), { code: 'EIO' })) + } + return (real.readdir as (...a: unknown[]) => unknown)(...args) + }) as typeof real.readdir, + } +}) +const { MonitorHistoryStore } = await import('./MonitorHistoryStore.js') + +const roots: string[] = [] +afterEach(async () => { await Promise.all(roots.splice(0).map(root => rm(root, { recursive: true, force: true }))) }) +const snapshot = (at: number): MonitorWorkerSnapshot => ({ + schemaVersion: 1, sampledAt: at, + main: { at, cpuPercent: 2, rss: 1024, heapUsed: 256, heapLimit: 2048, loopMeanMs: 20, loopP99Ms: 22, loopMaxMs: 25, sleepGap: false }, + windows: [], operations: [], recent: [], workerRss: 4096, +}) + +// Steering q115: startup found set-aside refused files by listing each run +// and read a failed listing as "no files" (`.catch(() => [])`). A prior run +// holding ONLY a set-aside file then had no marker at all, and the next +// maintenance deleted it with the refused bytes, the same unknown-as-empty +// shape q109 forbade for the debug ledger. +it('keeps a run whose listing failed at startup, with its set-aside bytes intact', async () => { + const root = await mkdtemp(join(tmpdir(), 'agent-code-monitor-')) + roots.push(root) + const runOld = join(root, 'runs', 'run-old') + await mkdir(runOld, { recursive: true }) + const aside = 'incidents.refused-1-00000000-0000-4000-8000-000000000000.json' + const refused = JSON.stringify({ version: 2, incidents: ['evidence'] }) + await writeFile(join(runOld, aside), refused) + + failOnce.path = runOld + const store = new MonitorHistoryStore(root, 'run-now') + await store.settled() + expect(failOnce.path).toBeNull() + store.record(snapshot(90_000), null, [], 0, 0) + await store.settled() + + expect(await readdir(runOld)).toContain(aside) + expect(await readFile(join(runOld, aside), 'utf8')).toBe(refused) + expect(store.status().state).toBe('degraded') +}) diff --git a/src/main/performance/MonitorHistoryStore.ts b/src/main/performance/MonitorHistoryStore.ts index bf0b4fed5..c0078dbdf 100644 --- a/src/main/performance/MonitorHistoryStore.ts +++ b/src/main/performance/MonitorHistoryStore.ts @@ -385,8 +385,21 @@ export class MonitorHistoryStore { try { await this.cleanupTemps() for (const run of await this.runNames()) { - const entries = await readdir(join(this.root, RUNS_DIR, run)).catch(() => [] as string[]) - if (entries.some(name => name.startsWith(REFUSED_INCIDENTS_PREFIX))) this.refusedAsideRuns.add(run) + // A failed listing is UNKNOWN, not empty (steering q115): read as "no + // set-aside file", a prior run holding only one had no marker left and + // the next maintenance deleted it with the refused bytes. Such a run is + // marked unindexed, which maintenance never expires, and the store is + // degraded. Only ENOENT (the run is already gone) means nothing to find. + // Budget pruning (pruneRuns) stays the only way this run leaves disk. + try { + const entries = await readdir(join(this.root, RUNS_DIR, run)) + if (entries.some(name => name.startsWith(REFUSED_INCIDENTS_PREFIX))) this.refusedAsideRuns.add(run) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + this.degraded = true + this.unindexedRuns.add(run) + } + } for (const resolution of TIERS) { const file = join(this.root, RUNS_DIR, run, `${resolution}.jsonl`) try {