From 107b076ab42f1dac457b144ea7be27ee45a4184c Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 16:55:37 +0000 Subject: [PATCH] fix: date a review-log row from its binding start when it has no timestamp The Step 9.4 template asks the writer to fill `timestamp`; the logger stamps `review_binding.started_at` but never `timestamp`. `gstack-review-scope summary` dropped such a row silently, so this PR's own pass 2 was missing from REVIEW_LOOP (passes=1 for a two-pass loop). A row now dates from `timestamp`, else from its binding start; a row with neither is still not counted. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-scope.ts | 13 +++++++++++-- test/review-scope.test.ts | 12 ++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/review-scope.ts b/lib/review-scope.ts index a451fed0a..cdf22714d 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -224,10 +224,19 @@ export function renderDeferredSection(deferred: readonly DeferredRecord[], noted export interface ReviewLoopSummary { passes: number; blocking_found: number; deferred: number; noted: number; fixed: number; scope: ReviewScopePolicy | 'unknown' } /** Review-log rows of one /ship invocation (skill review, via ship, at or after `since`) → the loop summary. */ +/** When a review-log row was written: its `timestamp`, else the start of the pass the logger bound it to (the row's writer fills `timestamp`; the logger does not). */ +function rowTime(row: Record): number { + for (const value of [row.timestamp, row.review_binding?.started_at]) { + const ms = typeof value === 'string' ? Date.parse(value) : NaN; + if (Number.isFinite(ms)) return ms; + } + return NaN; +} + export function summarizeReviewLoop(rows: readonly Record[], since: string): ReviewLoopSummary { const sinceMs = Date.parse(since); - const passes = rows.filter(r => r && r.skill === 'review' && r.via === 'ship' && typeof r.timestamp === 'string' - && Number.isFinite(Date.parse(r.timestamp)) && (!Number.isFinite(sinceMs) || Date.parse(r.timestamp) >= sinceMs)); + const passes = rows.filter(r => r && r.skill === 'review' && r.via === 'ship' && Number.isFinite(rowTime(r)) + && (!Number.isFinite(sinceMs) || rowTime(r) >= sinceMs)); const blocking = new Set(); const deferred = new Set(); const noted = new Set(); diff --git a/test/review-scope.test.ts b/test/review-scope.test.ts index 2276cf101..d88d77154 100644 --- a/test/review-scope.test.ts +++ b/test/review-scope.test.ts @@ -176,6 +176,18 @@ describe('loop summary', () => { expect(s).toEqual({ passes: 2, blocking_found: 2, deferred: 1, noted: 1, fixed: 1, scope: 'frozen' }); expect(reviewLoopLine(s)).toBe('REVIEW_LOOP: passes=2 blocking_found=2 deferred=1 noted=1 fixed=1 scope=frozen'); }); + + // A pass logged without the template's timestamp (the logger stamps review_binding.started_at, never + // timestamp) is still this invocation's pass; dropping it silently under-reported passes on #3481. + test('a row with no timestamp dates from its binding start; a row with neither is not counted', () => { + const rows = [ + { skill: 'review', via: 'ship', timestamp: '2026-10-26T10:00:00Z', review_scope: 'frozen', findings: [{ fingerprint: 'a:1:x', blocking: true, action: 'auto-fixed' }] }, + { skill: 'review', via: 'ship', review_scope: 'frozen', review_binding: { state: 'changed', started_at: '2026-10-26T10:30:00.000Z' }, findings: [{ fingerprint: 'c:1:z', action: 'noted' }] }, + { skill: 'review', via: 'ship', review_binding: { state: 'changed', started_at: '2026-10-26T09:00:00.000Z' }, findings: [{ fingerprint: 'old:1:q', action: 'noted' }] }, + { skill: 'review', via: 'ship', findings: [{ fingerprint: 'undated:1:q', action: 'noted' }] }, + ]; + expect(summarizeReviewLoop(rows, '2026-10-26T09:30:00Z')).toEqual({ passes: 2, blocking_found: 1, deferred: 0, noted: 1, fixed: 1, scope: 'frozen' }); + }); }); describe('gstack-review-scope (bin)', () => {