fix(#3584): the verb owns the count token and nothing else (#3635)

* test(3584): failing-first coverage for Plans-line trailing text

roadmap update-plan-progress preserves trailing text only when the line begins with a
canonical count token. Every other phrasing — including the TBD value the shipped
template itself suggests — is replaced to end-of-line, and a sentence wrapping onto a
second line has its first line deleted, leaving the continuation standing alone so the
roadmap asserts something nobody wrote. Exit 0, updated:true, and the diff reads as a
routine count bump.

These tests fail on that, and pin the arms that must keep working: the template
placeholder is still replaced, the #2853 token-plus-annotation path is unchanged, CRLF is
neither stranded nor duplicated, and a run that leaves the line alone still updates the
Progress table and checkboxes rather than becoming a no-op.

* fix(3584): the verb owns the count token and nothing else

RED proven at 105bdf7c: 7 failures — the preserving cases (freeform prose, wrapped
continuation, TBD, CRLF) failed while the template-placeholder and #2853 token arms
passed on base.

The trailing-text guard fired only when the line began with a canonical count token:
 dropped the rest of the line whenever the
regex's count group did not match. #2853 fixed end-of-line truncation on that one path
only. The in-code comment justified the rest as 'the fresh-template bracketed placeholder
or other freeform guidance, not user prose' — a heuristic that misreads ordinary human
phrasing and destroys even TBD, the value the shipped template itself suggests at
templates/roadmap.md:37.

The sharper failure was the wrapped sentence: only the first line is inside the match, so
the verb deleted line one and left line two standing alone, leaving the roadmap asserting
something nobody wrote — at exit 0, updated:true, in a diff that reads as a routine count
bump.

Inverted the default into three arms. A real count token is rewritten with its annotation
preserved (unchanged, #2853). A bracketed placeholder is detected POSITIVELY and replaced.
Everything else — freeform prose, TBD, a wrapped sentence's first line, an empty value —
returns the match untouched. That last arm resolves the wrapped case by construction: an
untouched first line cannot orphan its continuation.

Positive detection is the load-bearing part. Implemented as 'not a count token, therefore
disposable', rows 1-3 come straight back; the detector instead asks whether the value IS a
bracketed placeholder.

Leaving the line alone does not make the verb a no-op: the phase checkbox, the
Progress-table cells and the plan-checklist row still update in the same run, and that is
asserted. CRLF is unaffected in every arm — the pattern's [^\r\n]* never consumes the
\r, so it sits outside the match regardless of which arm runs.

Fixes #3584

* fix(3584): detect the template placeholder by its text, not by its brackets

Two defects in the arm-2 detector shipped in fc23e49e, both found in review.

Finding A: isBracketedPlaceholder asked only whether the trimmed value was wrapped
in [...]. Brackets are ordinary prose punctuation in a roadmap, so any hand-written
bracketed note — '[Deferred pending re-scope]', '[blocked on #1234]' — was classified
as the fresh-template placeholder and destroyed. That is the very defect #3584 is
about, reintroduced one arm over. The detector now matches the placeholder's TEXT
(/^\[\s*Number of plans\b[\s\S]*\]$/i), so it recognizes the shipped template
value and its short form and nothing else.

Finding B: the count group matched '\\d+\\s+plans' only. The plural is not the
template's own output shape — templates/roadmap.md:62 ships '1 plan' — so a
single-plan phase fell through every arm and its line froze permanently, never
updating again. Widened to 'plans?'. This one was introduced by the arm-3 default:
before it, the singular fell through to the old replace-everything path and at
least stayed current.

Cases 11-14 cover both: a bracketed human note preserved, the short placeholder
still replaced, '1 plan' rewritten, and '1 plan (annotation)' rewritten with the
annotation intact. Verified against the live binary, not just re-read.

Also converted all 15 cases in this block from try/finally to t.after(), per
CONTRIBUTING.md:356-370 which bans try/finally in test bodies. The existing #2853
block above is untouched — it is not in this change's scope and its conversion is
not this fix's concern.

Refs #3584

* chore(3584): add changeset fragment

Fixed-type fragment for the roadmap Plans-line trailing-text fix. pr:0 placeholder, backfilled once the PR number exists.

Refs #3584

* chore(3584): backfill changeset PR number (#3635)

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-18 16:39:17 -04:00
committed by GitHub
parent ac1b6d679f
commit 0f417aa6d0
3 changed files with 266 additions and 15 deletions

View File

@@ -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)

View File

@@ -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

View File

@@ -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 — <annotation>` 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}`);
});
});
});
}