From b0f1722662eb5f51a31b09908291eb01048d4ec3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 5 Aug 2026 16:52:37 -0400 Subject: [PATCH] fix(#2969): ratchet completed_plans up for gap-closure plans under deriveProgressKeys (#3091) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2969): completed_plans must ratchet up for gap-closure plans Failing-first regression: when deriveProgressKeys=true (cmdStatePlannedPhase's opt-in), applyStatePreservation restores completed_plans to its pre-growth curated value, so gap-closure plans that complete never increment it — STATE.md shows completed_plans < total_plans forever even though every PLAN has a SUMMARY. Three cases: the ratchet-up (derived 54 > curated 50), the ratchet-down protection (derived 47 < curated 50 keeps curated), and the body-only write protection (no deriveProgressKeys → wholesale restore). The existing #2440 test covers the case where derived < curated (ratchet holds); this adds the missing case where derived > curated (ratchet must release upward). * fix(#2969): ratchet completed_plans/plases up under deriveProgressKeys applyStatePreservation's deriveProgressKeys path (cmdStatePlannedPhase's opt-in) let total_plans/total_phases take the derived value but restored completed_plans/completed_phases to their pre-growth curated value — so gap-closure plans that completed after the plan count grew never incremented them, leaving STATE.md at completed_plans < total_plans forever (every PLAN had a SUMMARY). Extend the deriveProgressKeys exclusion to also let completed_plans and completed_phases take the derived value, but ratcheted UP only (never derive downward past curated) — preserving the #3242 curated-progress protection for cases unrelated to plan-count growth (e.g. a deleted SUMMARY). percent takes the derived value (the resync already recomputed it from disk counts). Scoped to deriveProgressKeys (plan-phase only); body-only writes (state.update/patch without the flag) keep the full #3242 wholesale restore. * fix(#2969): also take derived percent under deriveProgressKeys Isolated-review blocker: percent fell into the else branch and was overwritten with the stale curated value, contradicting the inline comment and leaving STATE.md incoherent (e.g. completed_plans:54/total_plans:54 at percent:93). Skip percent in the ratchet loop so the derived (resync- recomputed) value survives. * chore(#2969): add changeset fragment * chore(#2969): backfill changeset PR number 3091 --------- Co-authored-by: sim --- .changeset/lucky-tunas-leap.md | 5 +++ src/state-transition.cts | 19 +++++++++++- tests/state-transition.test.cjs | 55 +++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 .changeset/lucky-tunas-leap.md diff --git a/.changeset/lucky-tunas-leap.md b/.changeset/lucky-tunas-leap.md new file mode 100644 index 000000000..c548f2d6d --- /dev/null +++ b/.changeset/lucky-tunas-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3091 +--- +**`progress.completed_plans` no longer stays pinned after a gap-closure cycle** — when plan-phase re-planned a phase and added gap-closure plans, `total_plans` corrected upward but `completed_plans` was restored to its pre-growth value, so STATE.md showed `completed_plans < total_plans` permanently even after every plan (including the gap-closure ones) was summarized. `completed_plans` and `completed_phases` now ratchet up to the disk-derived count under the plan-phase progress opt-in (never deriving downward, preserving the curated-progress ratchet for unrelated edits). (#2969) diff --git a/src/state-transition.cts b/src/state-transition.cts index 9db1f96f3..362180719 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -195,8 +195,25 @@ export function applyStatePreservation(input: StatePreservationInput): StatePres const derived = (postFm['progress'] ?? {}) as Record; const merged: Record = { ...derived }; if (curated) { + // #2440: total_plans and total_phases always take the derived value. + // #2969: completed_plans and completed_phases take the derived value + // when it is GREATER than the curated value (gap-closure plans that + // completed after the plan count grew) — ratcheting UP only, never + // deriving downward (preserves the #3242 curated-progress protection + // for cases unrelated to plan-count growth, e.g. a deleted SUMMARY). + // percent also takes the derived value — the resync recomputed it from + // disk counts, and a stale curated percent would be incoherent against + // the ratcheted-up completed counts (e.g. 54/54 at 93%). + const ratchetUpKeys = new Set(['completed_plans', 'completed_phases']); for (const [key, value] of Object.entries(curated)) { - if (key !== 'total_plans' && key !== 'total_phases') { + if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; + if (ratchetUpKeys.has(key)) { + const derivedNum = typeof derived[key] === 'number' ? derived[key] : -Infinity; + const curatedNum = typeof value === 'number' ? value : -Infinity; + // Take the derived value only when it ratchets up; else keep curated. + if (derivedNum > curatedNum) continue; + merged[key] = value; + } else { merged[key] = value; } } diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 6b7409a2d..89b95e3b4 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -1340,6 +1340,61 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons 'total_plans equality → derived value (identity)'); }); + test('#2969: deriveProgressKeys=true — completed_plans ratchets UP when disk count exceeds curated (gap-closure plans completed)', () => { + // Gap-closure scenario: a phase had 50 plans all summarized (completed_plans: 50), + // then 4 gap-closure plans were added (total_plans -> 54) and all 4 got SUMMARYs. + // Disk scan now counts 54 summaries. The curated completed_plans (50) must + // ratchet UP to the derived value (54), not stay pinned at 50 — otherwise + // completed_plans < total_plans forever even though every plan is summarized. + const curated = { progress: { total_plans: 54, completed_plans: 50, total_phases: 2, completed_phases: 1, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 54, total_phases: 2, completed_phases: 1, percent: 100 } }, + resync: false, + deriveProgressKeys: true, + ...untouched, + }); + assert.equal(r.postFm.progress.total_plans, 54, 'total_plans takes derived value'); + assert.equal(r.postFm.progress.completed_plans, 54, + 'completed_plans must ratchet UP to derived (54 > curated 50 — gap-closure plans completed) (#2969)'); + assert.equal(r.postFm.progress.percent, 100, + 'percent must reflect the true completion fraction (54/54) (#2969)'); + }); + + test('#2969 ratchet-down protection: deriveProgressKeys=true keeps curated when disk count < curated', () => { + // The ratchet must only go UP. If the disk count is somehow LOWER than + // curated (e.g. a SUMMARY was deleted), keep the curated value — do not + // derive downward. (#3242 curated-progress protection, scoped to deriveProgressKeys.) + const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 47, percent: 87 } }, + resync: false, + deriveProgressKeys: true, + ...untouched, + }); + assert.equal(r.postFm.progress.completed_plans, 50, + 'completed_plans must NOT derive downward (47 < curated 50) — ratchet-up only (#2969/#3242)'); + }); + + test('#2969 body-only write protection: deriveProgressKeys absent keeps wholesale restore', () => { + // state.update/patch (no deriveProgressKeys flag) must keep the full #3242 + // wholesale curated restore — completed_plans never moves for a body-only edit. + const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 54, completed_plans: 54, percent: 100 } }, + resync: false, + // deriveProgressKeys NOT set — body-only write path + ...untouched, + }); + assert.equal(r.postFm.progress.completed_plans, 50, + 'body-only write must keep curated completed_plans (no deriveProgressKeys) (#2969/#3242)'); + }); + test('progress: NOT restored when transition re-derives from disk (resync=true) — sync/advancePlan/completePhase path', () => { const recomputed = { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }; const r = applyStatePreservation({