From ff161f2281d376d6bfe16c549272ee232aa0c5ee Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 24 Jun 2026 14:36:01 -0400 Subject: [PATCH] fix(#1582): derive phase-complete velocity from By-Phase table (idempotent) (#1655) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1582): derive phase-complete velocity from By-Phase table (idempotent) updatePerformanceMetricsSection blind-added summaryCount onto the prior velocity total on every phase complete, so re-running phase complete on an already-complete phase incremented the total each time (the sibling of #4, which fixed the Completed Phases counter the same way). The velocity total is now derived as the sum of the By-Phase table's Plans column AFTER the row upsert — re-completing a phase upserts the same row, so the sum is stable; a hand-edited inflated total self-heals downward to the true sum on the next completion. When the By-Phase table is absent the total is left unchanged (no crash). Strengthens the misnamed 'idempotent' test (its comment explicitly declined to assert velocity idempotency — the latent gap) and adds a self-heal regression; corrects the #320 behavior-lock velocity assertion which had encoded the blind-add (3 = 1+2 double-count) — the derived value is 2. * chore(#1582): backfill changeset pr ref to 1655 * fix(#1582): velocity sum tolerates indented By-Phase rows (codex review) Adversarial review (codex, gpt-5.5/high) flagged that byPhaseTablePattern's data-row capture allows leading whitespace ([ \t]*\|), but the derive sum was anchored at ^\| and would skip indented hand-edited/legacy rows — capturing them in the table but silently undercounting. Align the sum regex (^\s*\|) with the table capture's tolerance. Adds an indented-row regression. Two other codex findings are pre-existing and out of scope: padded/unpadded phase dedup (phaseRowPattern, identical in old code — derive yields the same value as the old blind-add) and CRLF tables (the shared byPhaseTablePattern header requires bare \n, so the upsert was already broken on CRLF; the fix changes stale-vs- double-count, does not worsen it). * fix(#1582): add verification fixtures to velocity tests under #1522 gate Post-rebase onto next+#1548, the #1582 velocity tests (self-heal, indented-row) use phase complete, which now fail-closes under #1522's canonical verification gate without a passed *-VERIFICATION.md. Add writePassedVerification(tmpDir,'02-next','02') to both. --- .changeset/zesty-elks-click.md | 5 ++ src/state.cts | 40 ++++++++--- tests/state.test.cjs | 121 +++++++++++++++++++++++++++++++-- 3 files changed, 151 insertions(+), 15 deletions(-) create mode 100644 .changeset/zesty-elks-click.md diff --git a/.changeset/zesty-elks-click.md b/.changeset/zesty-elks-click.md new file mode 100644 index 000000000..64e174d31 --- /dev/null +++ b/.changeset/zesty-elks-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1655 +--- +**`phase complete` no longer double-counts Total plans completed velocity on re-run** — re-running `phase complete` on an already-complete phase incremented the velocity total each time (2 -> 4 -> 6 ...), because the metric re-read the cumulative total and blind-added the phase's plan count on every invocation. The total is now derived from the By-Phase table's Plans column (the same source the table upserts against), so re-completing a phase upserts the same row and the sum stays stable — and a hand-edited inflated total self-heals to the true sum on the next completion. (#1582) diff --git a/src/state.cts b/src/state.cts index 716e4b851..51def1cf3 100644 --- a/src/state.cts +++ b/src/state.cts @@ -2417,16 +2417,11 @@ function cmdSignalResume(cwd: string, raw: boolean): void { * Returns modified content string. */ function updatePerformanceMetricsSection(content: string, cwd: string, phaseNum: string | number, planCount: number, summaryCount: number): string { - // Update Velocity: Total plans completed - const totalMatch = content.match(/Total plans completed:\s*(\d+|\[N\])/); - const prevTotal = totalMatch && totalMatch[1] !== '[N]' ? parseInt(totalMatch[1], 10) : 0; - const newTotal = prevTotal + summaryCount; - content = content.replace( - /Total plans completed:\s*(\d+|\[N\])/, - `Total plans completed: ${newTotal}` - ); - - // Update By Phase table — upsert row for this phase + // By Phase table — upsert the row for THIS phase FIRST. The velocity total is then + // DERIVED from the table's Plans column so it stays idempotent on re-run: completing + // the same phase again upserts the same row, so the column sum is stable. The previous + // blind-add (prevTotal + summaryCount) re-read the cumulative total each call and + // double-counted on every re-run. (#1582) const byPhaseMatch = content.match(byPhaseTablePattern); if (byPhaseMatch) { let tableBody = byPhaseMatch[2].trim(); @@ -2445,6 +2440,31 @@ function updatePerformanceMetricsSection(content: string, cwd: string, phaseNum: content = content.replace(byPhaseTablePattern, (_match, tableHeader: string) => `${tableHeader}${tableBody}\n`); } + // Velocity: Total plans completed — DERIVED as the sum of the By-Phase Plans column + // (the second cell) across all data rows. Idempotent by construction (re-running phase + // complete upserts the same row → same sum) and self-healing (a hand-edited inflated + // total is corrected to the true sum on the next completion). When the By-Phase table + // is absent, leave the velocity total unchanged rather than guess. (#1582) + if (/Total plans completed:\s*(\d+|\[N\])/.test(content)) { + const tableForSum = content.match(byPhaseTablePattern); + if (tableForSum) { + let sum = 0; + for (const row of tableForSum[2].split(/\r?\n/)) { + // Data rows look like `| | | … |`, optionally indented (the + // byPhaseTablePattern data-row capture allows `[ \t]*` leading whitespace, so the + // sum must too or hand-edited/legacy indented rows are silently skipped — #1582 + // codex review). Header (`| Phase | Plans | …`) and separator (`| --- | --- | …`) + // rows have a non-numeric second cell and are skipped; non-numeric cells → 0. + const cellMatch = row.match(/^\s*\|\s*[^|]+\s*\|\s*(\d+)\s*\|/); + if (cellMatch) sum += parseInt(cellMatch[1], 10); + } + content = content.replace( + /Total plans completed:\s*(\d+|\[N\])/, + `Total plans completed: ${sum}`, + ); + } + } + return content; } diff --git a/tests/state.test.cjs b/tests/state.test.cjs index efac13e86..38eb079a8 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2046,17 +2046,125 @@ describe('updatePerformanceMetricsSection', () => { runGsdTools('phase complete 5', tmpDir); const afterSecond = fs.readFileSync(statePath, 'utf-8'); - // Both should have same total plans count (idempotent update for same phase) + // #1582: the velocity total must be IDEMPOTENT across re-runs of the same phase. + // The old blind-add (prevTotal + summaryCount) double-counted on every re-run + // (1 -> 2 here); the fix derives the total from the By-Phase Plans column, so + // re-running the same phase upserts the same row and the sum stays stable. const firstCount = afterFirst.match(/Total plans completed:\s*(\d+)/); const secondCount = afterSecond.match(/Total plans completed:\s*(\d+)/); assert.ok(firstCount, 'First run should have total plans'); assert.ok(secondCount, 'Second run should have total plans'); - // Second run adds another completion for phase 5, so count increments - // The key is the By Phase row for phase 5 should be updated, not duplicated + assert.equal( + firstCount[1], + secondCount[1], + `velocity total must be idempotent across re-runs of phase 5 (#1582): first=${firstCount[1]} second=${secondCount[1]}`, + ); + assert.equal(firstCount[1], '1', 'phase 5 has 1 plan, so the velocity total must be 1'); + // The By Phase row for phase 5 should be updated, not duplicated. const phase5Rows = (afterSecond.match(/\|\s*5\s*\|/g) || []).length; assert.ok(phase5Rows <= 1, 'Phase 5 should appear at most once in By Phase table (no duplicates)'); }); + test('#1582 — velocity self-heals a hand-inflated total down to the true By-Phase sum', () => { + // A hand-edited STATE.md whose velocity line says 99 but whose By-Phase table + // records the true completed plans. Completing a fresh phase must RECOMPUTE the + // total from the table (derive, not accumulate), correcting the inflated value + // downward rather than adding to it. + const content = `# Project State + +**Current Phase:** 02 +**Status:** Executing Phase 2 + +## Performance Metrics + +**Velocity:** +- Total plans completed: 99 +- Average duration: 5 min +- Total execution time: 0.1 hours + +**By Phase:** + +| Phase | Plans | Total | Avg/Plan | +|-------|-------|-------|----------| +| 1 | 2 | 10 min | 5 min | + +## Accumulated Context +`; + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const phaseDir = path.join(tmpDir, '.planning', 'phases', '02-next'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '02-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '02-01-SUMMARY.md'), '# Summary\n'); + writePassedVerification(tmpDir, '02-next', '02'); + + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n## Phase 2: Next\n\n- [ ] Phase 2: Next\n` + ); + + const result = runGsdTools('phase complete 2', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error}`); + + const stateAfter = fs.readFileSync(statePath, 'utf-8'); + // True sum = phase 1 (2) + phase 2 (1) = 3. Old blind-add would yield 99 + 1 = 100. + assert.ok( + stateAfter.match(/Total plans completed:\s*3\b/), + 'velocity total must self-heal to the true By-Phase sum (3), not accumulate from the inflated 99 (#1582)', + ); + }); + + test('#1582 — velocity sums indented By-Phase data rows too (codex review: byPhaseTablePattern allows [ \\t]* leading whitespace, so the sum must match it)', () => { + // byPhaseTablePattern's data-row capture is `(?:[ \\t]*\\|...)*` — it ALLOWS leading + // whitespace. The derive sum must tolerate the same, or a hand-edited/legacy indented + // row is captured by the table but silently skipped by the sum (undercount). + const content = `# Project State + +**Current Phase:** 02 +**Status:** Executing Phase 2 + +## Performance Metrics + +**Velocity:** +- Total plans completed: 0 +- Average duration: N/A +- Total execution time: 0 hours + +**By Phase:** + +| Phase | Plans | Total | Avg/Plan | +|-------|-------|-------|----------| + | 1 | 2 | - | - | + +## Accumulated Context +`; + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const phaseDir = path.join(tmpDir, '.planning', 'phases', '02-next'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '02-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '02-01-SUMMARY.md'), '# Summary\n'); + writePassedVerification(tmpDir, '02-next', '02'); + + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n## Phase 2: Next\n\n- [ ] Phase 2: Next\n` + ); + + const result = runGsdTools('phase complete 2', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error}`); + + const stateAfter = fs.readFileSync(statePath, 'utf-8'); + // Indented phase-1 row (2) + new column-0 phase-2 row (1) = 3. A sum regex anchored + // at ^\\| would skip the indented row and report 1. + assert.ok( + stateAfter.match(/Total plans completed:\s*3\b/), + 'velocity must sum indented By-Phase rows too (codex review, #1582): expected 3 (2 + 1)', + ); + }); + test('byPhaseTablePattern behavior-lock (#320): By Phase table header preserved and phase row upserted after hoist to module scope', () => { // Exercises the byPhaseTablePattern match path directly: header must be preserved, // an existing phase row must be replaced (not duplicated), and a new phase row inserted. @@ -2108,8 +2216,11 @@ describe('updatePerformanceMetricsSection', () => { const phase6Rows = (stateAfter.match(/\|\s*6\s*\|/g) || []).length; assert.strictEqual(phase6Rows, 1, 'Phase 6 row must appear exactly once in By Phase table (upsert, not append)'); - // Total plans count updated correctly (1 pre-existing + 2 new summaries) - assert.ok(stateAfter.match(/Total plans completed:\s*3/), 'Total plans completed should be 3 after upsert'); + // Total plans count = sum of the By-Phase Plans column after the upsert. Phase 6's + // row is upserted to its current summaryCount (2), and it is the only row, so the + // derived total is 2. (#1582: derived from the table, not blind-added onto the prior + // velocity — which previously produced 1+2=3 by double-counting phase 6.) + assert.ok(stateAfter.match(/Total plans completed:\s*2\b/), 'Total plans completed should equal the By-Phase Plans sum (2) after upsert (#1582)'); }); test('#1658 — By-Phase table row upserts on a CRLF STATE.md (byPhaseTablePattern must be CRLF-tolerant)', () => {