* 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.
This commit is contained in:
Tom Boucher
5
.changeset/zesty-elks-click.md
Normal file
5
.changeset/zesty-elks-click.md
Normal file
@@ -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)
|
||||
@@ -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 `| <phase> | <plans> | … |`, 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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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)', () => {
|
||||
|
||||
Reference in New Issue
Block a user