diff --git a/.changeset/agile-cranes-frolic.md b/.changeset/agile-cranes-frolic.md new file mode 100644 index 000000000..8cc9d6c7f --- /dev/null +++ b/.changeset/agile-cranes-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3635 +--- +**Roadmap `Plans:` lines keep their hand-written text instead of being overwritten with a plan count** — `roadmap update-plan-progress` replaced everything after the `Plans:` label whenever the line did not already begin with a canonical `N/N plans` token, silently destroying freeform prose, a `TBD` note, or a hand-written annotation. A sentence that wrapped onto a second line lost only its first line, leaving the continuation stranded so the roadmap asserted something nobody wrote — at exit 0, in a diff that read as a routine count bump. The count is now written only over a real count token or the fresh-template placeholder, and a single-plan phase (`1 plan`) is recognized rather than frozen. (#3584) diff --git a/src/roadmap.cts b/src/roadmap.cts index a41a03b0a..706536cfb 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -874,29 +874,68 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und // `**Plans:** N plans` — bold "Plans:" (colon inside bold) // `Plans: N plans` — plain text header // - // #2853: the verb owns the count token ONLY — it must not destroy hand-written - // prose a human placed after the count (e.g. "(11-16 are gap closure ...)"). - // Group $1 = phase header → `Plans:` label + trailing whitespace (unchanged). - // Group $2 = the existing count token to replace: matches `N/N plans complete`, - // `N/N plans executed`, or the bare template `N plans` form. Group $3 = whatever - // else is on the line (`[^\r\n]*`, so CRLF `\r` is preserved). + // #2853 / #3584: the verb owns the count token ONLY — it must not destroy + // hand-written prose a human placed on the line. Group $1 = phase header → + // `Plans:` label + trailing whitespace (unchanged). Group $2 = the existing + // count token to replace: matches `N/N plans complete`, `N/N plans executed`, + // or the bare template `N plan(s)` form — singular is part of the tool's OWN + // grammar (gsd-core/templates/roadmap.md:62 ships `**Plans**: 1 plan` as the + // documented one-plan-phase shape), so the `s` is optional there (bug #3584 + // Finding B; pre-fix a bare `1 plan` fell into the drop-everything path and + // was accidentally overwritten with the correct count — post-fix it must be + // recognised as a token in its own right or it freezes stale forever). Group + // $3 = whatever else is on the line (`[^\r\n]*`, so a CRLF `\r` is never part + // of the match and rides along untouched in the unmatched remainder of the + // string — never stranded, never duplicated). // - // Trailing text is preserved ONLY when a real count token ($2) was present — - // i.e. an annotation a human wrote after a real count. When $2 is absent the - // line is the fresh-template bracketed placeholder (`[Number of plans…]`) or - // other freeform guidance, not user prose: the count replaces the whole token, - // preserving the pre-#2853 clean-output behaviour on the template path. + // Three arms, in order: + // 1. $2 present (a real count token) → rewrite the token, preserve $3 + // verbatim (an annotation a human wrote after a real count; #2853). + // 2. $2 absent AND $3, trimmed, is the fresh-template PLACEHOLDER shipped + // by gsd-core/templates/roadmap.md — either + // `[Number of plans, e.g., "3 plans" or "TBD"]` (line 37) or + // `[Number of plans]` (lines 51/75/88) → replace it with the computed + // count. Detected POSITIVELY on the distinctive `Number of plans` + // wording (anchored, case-insensitive), NEVER on "wholly bracketed" — + // a bracketed HUMAN annotation such as `[Deferred pending re-scope]` + // is structurally identical but must be arm-3 preserved (bug #3584 + // Finding A). + // 3. Anything else (freeform prose, `TBD` / `TBD — annotation`, a + // bracketed human note, the first line of a wrapped sentence, an + // empty value) → leave the whole matched line untouched by returning + // `_match` unchanged. An untouched first line cannot orphan its own + // continuation on the next line, since the pattern never spans past + // `\n` in the first place. const planCountPattern = new RegExp( - `(#{2,4}\\s*Phase\\s+${phasePattern}${OPTIONAL_PHASE_TAG_SOURCE}(?=[:\\s])(?:(?!\\n#{1,4}\\s)[\\s\\S])*?(?:\\*\\*Plans\\*\\*:|\\*\\*Plans:\\*\\*|(?:^|\\n)Plans:)\\s*)(\\d+\\s*\\/\\s*\\d+\\s+plans(?:\\s+(?:complete|executed))?|\\d+\\s+plans)?([^\\r\\n]*)`, + `(#{2,4}\\s*Phase\\s+${phasePattern}${OPTIONAL_PHASE_TAG_SOURCE}(?=[:\\s])(?:(?!\\n#{1,4}\\s)[\\s\\S])*?(?:\\*\\*Plans\\*\\*:|\\*\\*Plans:\\*\\*|(?:^|\\n)Plans:)\\s*)(\\d+\\s*\\/\\s*\\d+\\s+plans(?:\\s+(?:complete|executed))?|\\d+\\s+plans?)?([^\\r\\n]*)`, 'i' ); const planCountText = isComplete ? `${summaryCount}/${planCount} plans complete` : `${summaryCount}/${planCount} plans executed`; + // Positive detector for the fresh-template placeholder ONLY (bug #3584 + // Finding A). Anchored to the distinctive `Number of plans` wording that + // gsd-core/templates/roadmap.md actually ships, not to "anything in + // brackets" — a bracketed human annotation like `[Deferred pending + // re-scope]` is structurally bracketed too but carries none of this + // wording, so it correctly falls through to arm 3 untouched. + const isTemplatePlaceholder = (value: string): boolean => { + const trimmed = value.trim(); + return /^\[\s*Number of plans\b[\s\S]*\]$/i.test(trimmed); + }; roadmapContent = replaceInCurrentMilestone(roadmapContent, planCountPattern, (_match, label, existingCount, trailing) => { - // Preserve trailing text only when a real count preceded it. - const suffix = existingCount ? trailing : ''; - return `${label}${planCountText}${suffix}`; + if (existingCount) { + // Arm 1: real count token — rewrite it, preserve the trailing annotation. + return `${label}${planCountText}${trailing}`; + } + if (isTemplatePlaceholder(trailing)) { + // Arm 2: fresh-template placeholder — replace with the count. + return `${label}${planCountText}`; + } + // Arm 3: freeform prose, TBD, a bracketed human annotation, a wrapped + // sentence's first line, or an empty value — leave the line exactly as + // it was. + return _match; }); // If complete: check checkbox diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 7738bc936..7371c3f7d 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -8676,6 +8676,213 @@ describe('bug #2853: update-plan-progress preserves hand-written annotations', ( } }); }); + +// ──────────────────────────────────────────────────────────────────────── +// Regression: bug #3584 — #2853 only preserved trailing text when a real +// count token preceded it. When no token was present (freeform prose, `TBD`, +// a wrapped sentence's first line, an empty value), the verb still dropped +// $3 and glued the computed count in its place — and since only the FIRST +// line of a wrapped sentence sits inside the match, this orphaned the +// continuation line. Fix inverts the default: the count is only ever +// inserted (a) over a real count token (#2853's arm, unchanged) or (b) over +// the fresh-template bracketed placeholder, detected positively. Everything +// else is left untouched. +// ──────────────────────────────────────────────────────────────────────── +describe('bug #3584: update-plan-progress leaves non-count Plans text untouched', () => { + test('case 1 — freeform prose with no count token is preserved verbatim', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c1-')); + t.after(() => cleanup(tmp)); + const prose = 'This phase intentionally has no plan count yet.'; + const { roadmapPath } = setupFixture2853(tmp, `**Plans**: ${prose}`); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must succeed; stderr: ${result.stderr}`); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, `**Plans**: ${prose}`, `freeform prose must survive verbatim; got: ${line}`); + }); + + test('case 2 — a sentence wrapping onto a second line never orphans the continuation', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c2-')); + t.after(() => cleanup(tmp)); + const planningDir = path.join(tmp, '.planning'); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: This phase needs additional scoping before'); + const before = fs.readFileSync(roadmapPath, 'utf-8'); + const withContinuation = before.replace( + '**Plans**: This phase needs additional scoping before\n', + '**Plans**: This phase needs additional scoping before\nthe plan count can be finalized.\n' + ); + fs.writeFileSync(roadmapPath, withContinuation); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must succeed; stderr: ${result.stderr}`); + const after = fs.readFileSync(roadmapPath, 'utf-8'); + assert.ok( + after.includes('**Plans**: This phase needs additional scoping before\nthe plan count can be finalized.'), + `both wrapped lines must survive intact; got:\n${after}` + ); + void planningDir; + }); + + test('case 3 — `TBD — ` survives with the annotation intact', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c3-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: TBD — awaiting scoping decision'); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: TBD — awaiting scoping decision', `TBD annotation must survive; got: ${line}`); + }); + + test('case 4 — the fresh-template bracketed placeholder is still replaced with the computed count', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c4-')); + t.after(() => cleanup(tmp)); + // Exact shape shipped by gsd-core/templates/roadmap.md. + const placeholder = '[Number of plans, e.g., "3 plans" or "TBD"]'; + const { roadmapPath } = setupFixture2853(tmp, `**Plans**: ${placeholder}`); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: 1/1 plans complete', `placeholder must still be replaced; got: ${line}`); + }); + + test('case 5 — canonical token with no annotation is rewritten', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c5-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: 0/1 plans'); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: 1/1 plans complete', `token must be rewritten; got: ${line}`); + }); + + test('case 6 — canonical token WITH a hand-written annotation (#2853): token rewritten, annotation preserved', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c6-')); + t.after(() => cleanup(tmp)); + const annotation = '(11-16 are gap closure from VERIFICATION)'; + const { roadmapPath } = setupFixture2853(tmp, `**Plans**: 0/1 plans executed ${annotation}`); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, `**Plans**: 1/1 plans complete ${annotation}`, `#2853 arm must be unchanged; got: ${line}`); + }); + + test('case 7 — bare `N plans` form (no slash) is rewritten', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c7-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: 3 plans'); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: 1/1 plans complete', `bare form must be rewritten; got: ${line}`); + }); + + test('case 8a — CRLF variant, preserving arm: `\\r` neither stranded nor duplicated', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c8a-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: TBD — pending decision', { eol: '\r\n' }); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const after = fs.readFileSync(roadmapPath, 'utf-8'); + const plansIdx = after.indexOf('**Plans**'); + const nlIdx = after.indexOf('\n', plansIdx); + const restOfLine = after.slice(plansIdx, nlIdx === -1 ? after.length : nlIdx); + // Exactly the text plus at most a single trailing \r — never two, never none-when-expected. + assert.ok( + restOfLine === '**Plans**: TBD — pending decision' || restOfLine === '**Plans**: TBD — pending decision\r', + `CRLF preserving arm must not strand/duplicate \\r; got: ${JSON.stringify(restOfLine)}` + ); + assert.equal((restOfLine.match(/\r/g) || []).length <= 1, true, 'must not duplicate \\r'); + }); + + test('case 8b — CRLF variant, rewriting arm: `\\r` neither stranded nor duplicated', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c8b-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: 0/1 plans', { eol: '\r\n' }); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const after = fs.readFileSync(roadmapPath, 'utf-8'); + const plansIdx = after.indexOf('**Plans**'); + const nlIdx = after.indexOf('\n', plansIdx); + const restOfLine = after.slice(plansIdx, nlIdx === -1 ? after.length : nlIdx); + assert.ok( + restOfLine === '**Plans**: 1/1 plans complete' || restOfLine === '**Plans**: 1/1 plans complete\r', + `CRLF rewriting arm must not strand/duplicate \\r; got: ${JSON.stringify(restOfLine)}` + ); + assert.equal((restOfLine.match(/\r/g) || []).length <= 1, true, 'must not duplicate \\r'); + }); + + test('case 9 — leaving the Plans line untouched does not turn the verb into a no-op', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c9-')); + t.after(() => cleanup(tmp)); + const prose = 'Scoping still pending — do not touch.'; + const { roadmapPath } = setupFixture2853(tmp, `**Plans**: ${prose}`); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must exit 0; stderr: ${result.stderr}`); + const parsed = JSON.parse(result.stdout); + assert.equal(parsed.updated, true, 'updated must be true even when the Plans line is left alone'); + assert.equal(parsed.plan_count, 1, 'plan_count must still be computed correctly'); + assert.equal(parsed.summary_count, 1, 'summary_count must still be computed correctly'); + assert.equal(parsed.complete, true, 'complete must still be computed correctly'); + + const after = fs.readFileSync(roadmapPath, 'utf-8'); + // Plans line itself untouched. + assert.ok(after.includes(`**Plans**: ${prose}`), 'Plans line must remain untouched'); + // Phase checkbox in the phase list must still flip. + assert.match(after, /- \[x\] \*\*Phase 10: Test Phase\*\* \(completed \d{4}-\d{2}-\d{2}\)/, 'phase checkbox must still be checked'); + // Progress table Status/Completed cells must still update. + assert.match(after, /\|\s*10 Test Phase\s*\|\s*0\/1\s*\|\s*Complete\s*\|\s*\d{4}-\d{2}-\d{2}\s*\|/, 'progress table row must still update'); + // Plan checklist row must still be checked. + assert.match(after, /- \[x\] 10-01-PLAN\.md/, 'plan checklist row must still be checked'); + }); + + test('case 10 — empty value after the label is left alone, no fabricated count', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c10-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**:'); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must succeed; stderr: ${result.stderr}`); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**:', `empty value must be left alone with no fabricated count; got: ${line}`); + }); + + // Finding A (adversarial review): a bracketed HUMAN annotation is + // structurally identical to the bracketed template placeholder but carries + // none of its wording — it must be preserved, not destroyed. + test('case 11 — a bracketed human annotation is preserved verbatim, not mistaken for the template placeholder', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c11-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: [Deferred pending re-scope]'); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must succeed; stderr: ${result.stderr}`); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: [Deferred pending re-scope]', `human bracketed note must survive verbatim; got: ${line}`); + }); + + // The shorter template placeholder shape (gsd-core/templates/roadmap.md + // lines 51/75/88) must still be replaced. + test('case 12 — the short template placeholder `[Number of plans]` is still replaced', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c12-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: [Number of plans]'); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: 1/1 plans complete', `short placeholder must still be replaced; got: ${line}`); + }); + + // Finding B (adversarial review): the singular bare `N plan` form is the + // tool's own documented one-plan-phase grammar (templates/roadmap.md:62) + // and must be recognised as a real count token, not frozen forever. + test('case 13 — bare singular `1 plan` form (no `s`) is rewritten to the computed count', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c13-')); + t.after(() => cleanup(tmp)); + const { roadmapPath } = setupFixture2853(tmp, '**Plans**: 1 plan'); + const result = run2853(['roadmap', 'update-plan-progress', '10'], tmp); + assert.ok(result.ok, `run must succeed; stderr: ${result.stderr}`); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, '**Plans**: 1/1 plans complete', `singular token must be rewritten; got: ${line}`); + }); + + test('case 14 — bare singular `1 plan` WITH a hand-written annotation: token rewritten, annotation preserved', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3584-c14-')); + t.after(() => cleanup(tmp)); + const annotation = '(scope confirmed small)'; + const { roadmapPath } = setupFixture2853(tmp, `**Plans**: 1 plan ${annotation}`); + run2853(['roadmap', 'update-plan-progress', '10'], tmp); + const line = fs.readFileSync(roadmapPath, 'utf-8').split(/\r?\n/).find((l) => l.includes('**Plans**')); + assert.equal(line, `**Plans**: 1/1 plans complete ${annotation}`, `singular token must be rewritten with annotation preserved; got: ${line}`); + }); +}); }); }