From 8c1192d0dd1d82edee2a096ae41963f689ee7915 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 13:49:17 -0700 Subject: [PATCH] fix(hooks): timeline repair counts started vs completed per key instead of set-masking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dangling-event repair kept only the FIRST "started" entry per skill+session key and treated "completed" as a set, so any key where one run completed and another dangles was never repaired — and keys are not unique per run: legacy entries with no session field all share the bare-skill key, and the preamble's "$$-epoch" session ids collide within the same second. One old completion masked every future dangler forever. The hook now counts started vs completed per key and appends completions for the DIFFERENCE. Idempotency holds by construction: the appended completions balance the counts, so the next Stop appends nothing. Pinned with the two-runs-one-dangling case plus a re-run no-op assertion; all existing fail-open cases pass unchanged. Co-Authored-By: Claude Fable 5 --- hosts/claude/hooks/timeline-stop-hook.ts | 31 ++++++++++++++++++++---- test/timeline-stop-hook.test.ts | 24 ++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/hosts/claude/hooks/timeline-stop-hook.ts b/hosts/claude/hooks/timeline-stop-hook.ts index 91156184d..30fca9f8b 100755 --- a/hosts/claude/hooks/timeline-stop-hook.ts +++ b/hosts/claude/hooks/timeline-stop-hook.ts @@ -142,8 +142,18 @@ function main(): void { } // Corrupt LINES are skipped individually; a fully corrupt file repairs nothing. - const started = new Map(); - const completed = new Set(); + // + // COUNT started vs completed per key rather than treating completed as a + // set: keys are not unique per run — legacy entries with no session field + // all share the bare-skill key, and the preamble's "$$-epoch" session ids + // can collide within the same second. With set semantics, a key where one + // run completed and another dangles was NEVER repaired (the lone + // completion masked every dangler forever). Closing the count DIFFERENCE + // repairs exactly the open runs and stays idempotent: the appended + // completions balance the counts, so the next Stop appends nothing. + const startedCount = new Map(); + const firstStarted = new Map(); + const completedCount = new Map(); for (const line of raw.split('\n')) { if (!line.trim()) continue; let entry: TimelineEntry; @@ -154,11 +164,22 @@ function main(): void { } if (!entry || typeof entry.skill !== 'string') continue; const key = `${entry.skill}\u0000${entry.session ?? ''}`; - if (entry.event === 'started' && !started.has(key)) started.set(key, entry); - if (entry.event === 'completed') completed.add(key); + if (entry.event === 'started') { + startedCount.set(key, (startedCount.get(key) ?? 0) + 1); + if (!firstStarted.has(key)) firstStarted.set(key, entry); + } + if (entry.event === 'completed') { + completedCount.set(key, (completedCount.get(key) ?? 0) + 1); + } } - const dangling = [...started.entries()].filter(([key]) => !completed.has(key)); + const dangling: Array<[string, TimelineEntry]> = []; + for (const [key, count] of startedCount) { + const open = count - (completedCount.get(key) ?? 0); + const entry = firstStarted.get(key); + if (!entry) continue; + for (let i = 0; i < open; i++) dangling.push([key, entry]); + } if (dangling.length === 0) return; if (Date.now() - startedAt > DEADLINE_MS) { diff --git a/test/timeline-stop-hook.test.ts b/test/timeline-stop-hook.test.ts index e4498b542..b64eb33bb 100644 --- a/test/timeline-stop-hook.test.ts +++ b/test/timeline-stop-hook.test.ts @@ -115,6 +115,30 @@ describe('timeline-stop-hook (#2553, F5 fail-open)', () => { expect(timelineEntries().length).toBe(afterFirst); }); + test('count semantics: two runs under one key, one completed — the dangler is still repaired', () => { + // Legacy entries carry no session field, so both runs share the same + // skill+session key (same-second "$$-epoch" ids collide the same way). + // With set semantics the first run's completion masked the second run's + // dangler forever; counting closes the difference. + fs.writeFileSync( + timelinePath, + [ + JSON.stringify({ skill: 'review', event: 'started' }), + JSON.stringify({ skill: 'review', event: 'completed', outcome: 'success' }), + JSON.stringify({ skill: 'review', event: 'started' }), + ].join('\n') + '\n', + ); + + expect(runHook(stopPayload()).exitCode).toBe(0); + const repairs = timelineEntries().filter((e) => e.source === 'stop-hook'); + expect(repairs).toHaveLength(1); + expect(repairs[0]).toMatchObject({ skill: 'review', event: 'completed', outcome: 'unknown' }); + + // Idempotent under count semantics too: started=2, completed=2 → no-op. + expect(runHook(stopPayload()).exitCode).toBe(0); + expect(timelineEntries().filter((e) => e.source === 'stop-hook')).toHaveLength(1); + }); + test('exit 0 on missing timeline (nothing written, nothing created)', () => { const r = runHook(stopPayload()); expect(r.exitCode).toBe(0);