diff --git a/.changeset/sturdy-voles-tumble.md b/.changeset/sturdy-voles-tumble.md new file mode 100644 index 000000000..0a6c36ead --- /dev/null +++ b/.changeset/sturdy-voles-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2250 +--- +**ROADMAP phase edits can no longer escape their section** — completing a phase updated its plan count and per-plan checkboxes with whole-document regexes that could bleed into a neighbouring phase; those per-phase writes are now structurally bounded to the phase own section via a new `withSection` / `withPhaseSection` seam (#2130, #2067, #2080). (#2250) diff --git a/CONTEXT.md b/CONTEXT.md index 700331e44..33632bc2d 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -140,13 +140,13 @@ Primary installer for all runtimes. Single production file: `bin/install.js` (ge Module owning the tool's CLI I/O primitives: `output()` result emission (with large-payload temp-file spillover via `GSD_TEMP_DIR`/`ensureGsdTempDir`/`reapStaleTempFiles`), `error()` stderr emission with exit-code mapping, and the JSON-error-mode toggle (`setJsonErrorMode`/`getJsonErrorMode`, `ERROR_REASON`). Extracted from the Core module per ADR-857 rollout phase 1 (#859) so feature modules (`graphify`, `intel`, `audit`, `profile-pipeline`) depend on a small I/O seam instead of the core god-module; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/io.cjs` (generated from `src/io.cts`). ### Markdown Sectionizer -Canonical markdown-structure parsing seam (`gsd-core/bin/lib/markdown-sectionizer.cjs`, generated from `src/markdown-sectionizer.cts`). Pure functions, Node built-ins only. Exports: `stripFencedCode(content) → { text, unterminatedFence }` (CommonMark-correct state machine, CRLF-safe, signals unterminated fences); `tokenizeHeadings(content) → HeadingToken[]` (ATX headings outside fenced blocks, `{ level, text, line, offset }`); `collectSections(content, stopPredicate) → Section[]` (line-by-line section collection driven by a heading predicate); `collectSection(content, headingPredicate, { levelBounded, stripFences }) → Section | null` (single named section with level-bounded stop); `iterateBullets(sectionText) → BulletItem[]` (dash/checkbox/numbered markers with indented continuation); `extractTaggedBlocks(content, tagName) → string[]` (inner text of every `…` block in document order, tagName regex-escaped, caller decides fence-stripping — generalises `decisions.cts`'s bespoke extractor for T1); `replaceSection(content, section, newBody) → string` (pure character-offset splice using `Section.bodyStart`/`bodyEnd` for read-modify-write callers — eliminates T6 `state.cts`'s 7× inline `content.replace` pattern). `Section` carries `bodyStart`/`bodyEnd` offsets for `replaceSection`. ADR-1372 (epic #1372) establishes this seam and a tiered migration plan (T0–T7) to retire the 8+ ad-hoc markdown parsers and ~20 inline section-collects across `src/*.cts`. New `src/*.cts` modules must import this seam instead of hand-rolling fence strippers or heading-regex section walks (enforced by the `no-adhoc-markdown-parsing` ESLint rule landing in tier T7). +Canonical markdown-structure parsing seam (`gsd-core/bin/lib/markdown-sectionizer.cjs`, generated from `src/markdown-sectionizer.cts`). Pure functions, Node built-ins only. Exports: `stripFencedCode(content) → { text, unterminatedFence }` (CommonMark-correct state machine, CRLF-safe, signals unterminated fences); `tokenizeHeadings(content) → HeadingToken[]` (ATX headings outside fenced blocks, `{ level, text, line, offset }`); `collectSections(content, stopPredicate) → Section[]` (line-by-line section collection driven by a heading predicate); `collectSection(content, headingPredicate, { levelBounded, stripFences }) → Section | null` (single named section with level-bounded stop); `iterateBullets(sectionText) → BulletItem[]` (dash/checkbox/numbered markers with indented continuation); `extractTaggedBlocks(content, tagName) → string[]` (inner text of every `…` block in document order, tagName regex-escaped, caller decides fence-stripping — generalises `decisions.cts`'s bespoke extractor for T1); `replaceSection(content, section, newBody) → string` (pure character-offset splice using `Section.bodyStart`/`bodyEnd` for read-modify-write callers — eliminates T6 `state.cts`'s 7× inline `content.replace` pattern); `withSection(content, target, edit) → string` (resolve the section whose heading matches `target` — exact heading text or a `HeadingToken` predicate — and run `edit(body)` against ONLY that section's body before splicing the result back; bounded no-op when no heading matches or `edit` returns the same/non-string body; ADR-2143 §4 structurally retires the #2130/#2067/#2080 boundary-crossing class by confining any regex the caller runs to the matched section). `Section` carries `bodyStart`/`bodyEnd` offsets for `replaceSection`. ADR-1372 (epic #1372) establishes this seam and a tiered migration plan (T0–T7) to retire the 8+ ad-hoc markdown parsers and ~20 inline section-collects across `src/*.cts`. New `src/*.cts` modules must import this seam instead of hand-rolling fence strippers or heading-regex section walks (enforced by the `no-adhoc-markdown-parsing` ESLint rule landing in tier T7). ### Markdown Table Model Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-table.cjs`, generated from `src/markdown-table.cts`; ADR-2143, epic #2143). Pure functions, Node built-ins only, string-in/value-out, no I/O. Exports: `parseMarkdownTable(sectionText) → Result` (parses the first GFM pipe table found; typed `{ok:false,reason}` parse errors for no-table, missing/misaligned delimiter row, and ragged data rows — never silently drops or coerces a malformed row); `MarkdownTable` (`{columns: string[], rows: Record[]}`, rows addressed by column name, not position); `Result` (`{ok:true,value}\|{ok:false,reason}` — deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`; the two never mix); `TABLE_SCHEMAS` (`Record` — the canonical column-header variants for every GFM table GSD parses or generates: `RoadmapProgress` flat/milestone-grouped, `RequirementsTraceability`, `QuickTasks` no-status/with-status, `Security` trust-boundaries/threat-register/accepted-risks/audit-trail); `matchTableSchema(columns) → {id,label}\|null` (resolves a parsed header back to its canonical schema by exact column-name/order match). This registry is the single source of truth for ROADMAP/STATE/SECURITY canonical tables — a parity test (`tests/markdown-table.test.cjs`) asserts every variant's header appears verbatim in the template/workflow file that generates it, so the registry and templates can never silently drift (ADR-2143 §3 Generative-Fix-Divergence guard). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` is the first consumer: it locates the Progress section via the Markdown Sectionizer's `collectSection` and reads cells by column NAME through this seam, fixing #2137 (the prior position-anchored regex assumed `Status` was always the 3rd cell, which broke for the 5-column milestone-grouped `Milestone` variant). ### Roadmap Parser Module -Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). +Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `withPhaseSection`). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `filterPlanFiles`, `filterSummaryFiles`, `extractCanonicalPlanId`, `timeAgo`). Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 2aa86d030..10b595fbf 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -465,7 +465,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `legacy-cleanup.cjs` | Detect and remove leftover get-shit-done-cc artifacts; exports `planLegacyCleanup` (pure scan) and `applyLegacyCleanup` (thin IO applier) that root out stale files from the old package across every GSD-managed runtime config directory (#607) | | `loop-host-contract.cjs` | Generated Loop Host Contract — 12 loop points, per-step agent roles, and core artifacts for the five-step pipeline (discuss/plan/execute/verify/ship); emitted by `scripts/gen-loop-host-contract.cjs --write` (ADR-894 §3); consumed by `gen-capability-registry.cjs` | | `loop-resolver.cjs` | Loop Extension Point resolver — ADR-857 phase 3c/6 registry-consuming query; given a canonical loop point, filters `byLoopPoint` by resolved Capability State plus config activation (`when` key traversal with prototype-pollution guard), returns `{ point, activeHooks, rendered }` envelope; `resolveLoopHooks` and `renderLoopHooks` are pure (no I/O); command surface: `gsd-tools loop render-hooks [--config-dir ]` | -| `markdown-sectionizer.cjs` | Canonical markdown-structure parsing seam (ADR-1372, epic #1372) — pure, Node built-ins only; exports `stripFencedCode` (CommonMark-correct fence stripper, CRLF-safe), `tokenizeHeadings` (ATX headings outside fenced blocks), `collectSections`/`collectSection` (line-by-line section collection with `bodyStart`/`bodyEnd` offsets), `iterateBullets` (dash/checkbox/numbered markers), `extractTaggedBlocks` (inner text of `…` blocks, caller decides fence-stripping), and `replaceSection` (pure character-offset body splice for read-modify-write callers); foundation for T0–T7 migration tiers retiring 8+ ad-hoc parsers | +| `markdown-sectionizer.cjs` | Canonical markdown-structure parsing seam (ADR-1372, epic #1372) — pure, Node built-ins only; exports `stripFencedCode` (CommonMark-correct fence stripper, CRLF-safe), `tokenizeHeadings` (ATX headings outside fenced blocks), `collectSections`/`collectSection` (line-by-line section collection with `bodyStart`/`bodyEnd` offsets), `iterateBullets` (dash/checkbox/numbered markers), `extractTaggedBlocks` (inner text of `…` blocks, caller decides fence-stripping), `replaceSection` (pure character-offset body splice for read-modify-write callers), and `withSection` (resolve a section by heading/predicate and run an edit callback against ONLY its body, splicing the result back — ADR-2143 §4 bounded mutation); foundation for T0–T7 migration tiers retiring 8+ ad-hoc parsers | | `markdown-table.cjs` | Canonical GFM table model + `TABLE_SCHEMAS` registry seam (ADR-2143, epic #2143) — pure, Node built-ins only; exports `parseMarkdownTable(sectionText) → Result` (parses the first GFM pipe table, typed parse errors for ragged/malformed rows rather than silent coercion), `MarkdownTable` (`{columns, rows}`, rows addressed by column name), `Result` (`{ok:true,value}\|{ok:false,reason}` — distinct from command-routing-hub's dispatch `Result`), `TABLE_SCHEMAS` (canonical column-header variants for `RoadmapProgress`/`RequirementsTraceability`/`QuickTasks`/`Security` tables), and `matchTableSchema(columns) → {id,label}\|null` (resolves parsed headers back to a canonical schema); consumed by `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` (fixes #2137, the 5-column milestone-grouped Progress table) | | `milestone.cjs` | Milestone archival, requirements marking | | `model-catalog.cjs` | CJS adapter over the shared model catalog JSON; exports canonical runtime tier defaults, agent profile maps, alias maps, and routing metadata for all CLI consumers | diff --git a/src/markdown-sectionizer.cts b/src/markdown-sectionizer.cts index c9b0727c6..24d26217e 100644 --- a/src/markdown-sectionizer.cts +++ b/src/markdown-sectionizer.cts @@ -62,6 +62,13 @@ export interface Section { bodyEnd: number; } +/** Options shared by `collectSection` and `withSection` (see `collectSection`'s doc comment). */ +export interface CollectSectionOptions { + levelBounded?: boolean; + stopAtLevel?: number; + stripFences?: boolean; +} + /** Recognised bullet markers. */ export type BulletMarker = 'dash' | 'checkbox-unchecked' | 'checkbox-checked' | 'numbered'; @@ -340,7 +347,7 @@ export function collectSections( export function collectSection( content: string, headingPredicate: (heading: HeadingToken) => boolean, - opts: { levelBounded?: boolean; stopAtLevel?: number; stripFences?: boolean } = {}, + opts: CollectSectionOptions = {}, ): Section | null { if (typeof content !== 'string' || content.length === 0) return null; @@ -618,5 +625,47 @@ export function replaceSection(content: string, section: Section, newBody: strin return content.slice(0, section.bodyStart) + newBody + content.slice(section.bodyEnd); } +// ─── withSection ────────────────────────────────────────────────────────────── + +/** + * Locate the section whose heading matches `target`, run `edit` against ONLY + * that section's body, and splice the result back into `content`. + * + * `target` is either an exact (trimmed) heading-text match or a predicate + * function over `HeadingToken`. `edit` receives ONLY the section body — so any + * regex it runs is physically confined to that section — an edit cannot cross + * a section boundary (ADR-2143 §4, structurally retires the #2130/#2067/#2080 + * boundary-crossing class, where a hand-rolled regex escaped its intended + * section and mutated a sibling/shipped/backticked-literal occurrence instead). + * + * Bounded no-op behaviour (Phase 3 of ADR-2143 adds fail-loud diagnostics on + * top of this): + * - No heading matches `target` → `content` is returned unchanged. + * - `edit` returns a non-string, or returns the same string it was given → + * `content` is returned unchanged (no-op splice avoided). + * + * `opts` is forwarded verbatim to `collectSection` (see its doc comment for + * `levelBounded` / `stopAtLevel` / `stripFences` semantics) — it lets a caller + * whose heading levels are non-uniform (e.g. a mix of `###`/`####` phase + * headings) choose the correct section-end rule instead of relying on the + * `levelBounded: true` default. + */ +export function withSection( + content: string, + target: string | ((h: HeadingToken) => boolean), + edit: (body: string) => string, + opts: CollectSectionOptions = {}, +): string { + if (typeof content !== 'string') return content; + const predicate = typeof target === 'function' + ? target + : (h: HeadingToken) => h.text.trim() === target.trim(); + const section = collectSection(content, predicate, opts); + if (!section) return content; // bounded no-op on miss (Phase 3 adds fail-loud) + const newBody = edit(section.body); + if (typeof newBody !== 'string' || newBody === section.body) return content; + return replaceSection(content, section, newBody); +} + // Consumers: require('../gsd-core/bin/lib/markdown-sectionizer.cjs') // Named CJS exports are the canonical surface (ADR-457 .cts → .cjs build-at-publish). diff --git a/src/phase.cts b/src/phase.cts index 97dec6580..d453b23e4 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -44,7 +44,7 @@ import phaseLocatorMod = require('./phase-locator.cjs'); const { findPhaseInternal, getArchivedPhaseDirs } = phaseLocatorMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module import roadmapParserMod = require('./roadmap-parser.cjs'); -const { stripShippedMilestones, extractCurrentMilestone, getMilestonePhaseFilter, currentMilestoneRawRanges } = roadmapParserMod; +const { stripShippedMilestones, extractCurrentMilestone, getMilestonePhaseFilter, currentMilestoneRawRanges, withPhaseSection } = roadmapParserMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module @@ -1479,6 +1479,12 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // inline / backticked prose literal cannot match. Milestone-scoped below // (mutateMilestonePhase) so a Backlog entry or a same-numbered shipped- // milestone phase cannot be flipped either. + // ADR-2143 §4 note: this is the phase-LIST checkbox — it lives in the + // milestone's `- [ ] Phase N: …` checklist, OUTSIDE any `### Phase N` + // detail section, so there is no section for withPhaseSection to bind + // to. Left as a milestone-slice-scoped regex (not migrated to the + // sectionizer seam); see planCountBodyPattern below for the sites that + // WERE migrated. const checkboxPattern = new RegExp( `^[ \\t]*(-\\s*\\[)[ ](\\]\\s*(?:\\*\\*)?\\s*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`, 'im', @@ -1518,10 +1524,14 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { roadmapContent = roadmapContent.replace(tableRowPattern, updateProgressRow); } - const planCountPattern = new RegExp( - `(#{2,4}\\s*Phase\\s+${phaseEscaped}(?:(?!\\n#{1,4}\\s)[\\s\\S])*?\\*\\*Plans:\\*\\*\\s*)[^\\n]+`, - 'i', - ); + // ADR-2143 §4: the plan-count write is now routed through + // withPhaseSection (see mutateMilestonePhase below), which hands this + // pattern ONLY phase N's own detail-section body — so the pattern no + // longer needs its own `#{2,4}\s*Phase\s+N` anchor + skip-ahead-past- + // interior-headings lookahead; the section boundary itself confines + // the match (the #2067/#2200 boundary-crossing class is now + // structurally impossible for this site rather than regex-enforced). + const planCountBodyPattern = /(\*\*Plans:\*\*\s*)[^\n]+/i; const phaseInfoSummaries = phaseInfo['summaries'] as string[]; @@ -1534,17 +1544,25 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const mutateMilestonePhase = (slice: string): string => { let s = slice; s = s.replace(checkboxPattern, `$1x$2 (completed ${today})`); - s = s.replace(planCountPattern, `$1${summaryCount}/${planCount} plans complete`); - for (const summaryFile of phaseInfoSummaries) { - const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); - if (!planId) continue; - const planEscaped = escapeRegex(planId); - const planCheckboxPattern = new RegExp( - `(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`, - 'i', - ); - s = s.replace(planCheckboxPattern, '$1x$2'); - } + // ADR-2143 §4: the plan-count write and the per-plan checkbox flips + // are both scoped to phase N's OWN detail section via + // withPhaseSection — the edit callback below only ever sees that + // section's body, so neither regex can escape into a sibling + // phase's section, a shipped milestone, or a Backlog entry. + s = withPhaseSection(s, phaseNum, (body) => { + let b = body.replace(planCountBodyPattern, `$1${summaryCount}/${planCount} plans complete`); + for (const summaryFile of phaseInfoSummaries) { + const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); + if (!planId) continue; + const planEscaped = escapeRegex(planId); + const planCheckboxPattern = new RegExp( + `(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`, + 'i', + ); + b = b.replace(planCheckboxPattern, '$1x$2'); + } + return b; + }); return s; }; diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index cfa7d4c69..7f4a4262c 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -13,6 +13,7 @@ * - ./phase-id.cjs (escapeRegex, phaseMarkdownRegexSource) * - ./planning-workspace.cjs (planningDir) * - ./shell-command-projection.cjs (platformReadSync) + * - ./markdown-sectionizer.cjs (tokenizeHeadings, stripTaggedBlocks, withSection) */ import fs from 'node:fs'; @@ -32,7 +33,8 @@ const { import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspace; import { platformReadSync } from './shell-command-projection.cjs'; -import { tokenizeHeadings, stripTaggedBlocks } from './markdown-sectionizer.cjs'; +import { tokenizeHeadings, stripTaggedBlocks, withSection } from './markdown-sectionizer.cjs'; +import type { HeadingToken } from './markdown-sectionizer.cjs'; // ─── Roadmap milestone scoping ─────────────────────────────────────────────── @@ -200,6 +202,49 @@ function replaceInCurrentMilestone(content: string, pattern: RegExp, replacement return before + after.replace(pattern, replacement); } +/** + * Resolve a single phase's detail-section heading (`### Phase N: …`, any level + * 1–6, via the #2121 phase-id source) and run `edit` against ONLY that + * section's body. Delegates to `withSection` (markdown-sectionizer.cjs), so a + * per-phase ROADMAP edit is structurally bounded to that phase's own section — + * it cannot escape into a sibling phase, a shipped-milestone `
` block, + * or a backticked prose literal (ADR-2143 §4). + * + * `content` is expected to already be scoped to the current milestone's raw + * range(s) by the caller (see `currentMilestoneRawRanges`) — `withPhaseSection` + * composes with that milestone-level scoping rather than replacing it. + * + * The matched phase number must be delimited by whitespace, a colon, an + * open-paren tag, or end-of-heading — never a bare `\b`. A trailing `\b` sits + * between the last digit and a following `.` or letter, so it would let a + * query for phase `1` prefix-match a decimal sub-phase heading like + * `### Phase 1.1: Sub` or a distinct suffixed phase like `### Phase 1A: …`. + * + * The phase token must additionally anchor to the START of the heading text + * (after an optional leading `[tag]`, mirroring `findRoadmapPhaseInContent` + * below) — never merely appear anywhere in it. Without this anchor, a query + * for phase `1` would match a SIBLING phase whose own TITLE happens to + * mention "Phase 1" (e.g. `### Phase 3: Migrate off Phase 1 legacy pipeline`), + * and — because `collectSection` picks the first matching heading in document + * order — that sibling would be hijacked instead of the real Phase 1 section. + * + * The section body is bounded by `{ levelBounded: false }`: it ends at the + * next ATX heading of ANY level, not merely a heading at or above the phase + * heading's own level. Real ROADMAPs are not guaranteed to use a uniform + * phase-heading level, so a level-bounded stop could fold a deeper sibling + * heading (e.g. a `####` phase following a `###` phase) into this phase's + * body and let `edit` reach into it. + */ +function withPhaseSection( + content: string, + phaseId: unknown, + edit: (body: string) => string, +): string { + const src = phaseMarkdownRegexSource(phaseId); + const headingRe = new RegExp(`^\\s*(?:\\[[^\\]]{1,200}\\]\\s*)?Phase\\s+${src}(?=[\\s:(]|$)`, 'i'); + return withSection(content, (h: HeadingToken) => headingRe.test(h.text), edit, { levelBounded: false }); +} + // ─── Roadmap phase lookup ───────────────────────────────────────────────────── // #2199: a bullet/checkbox phase entry, e.g. `- [ ] **Phase 36 — Authentication**` @@ -650,4 +695,5 @@ export = { getMilestoneInfo, getMilestonePhaseFilter, currentMilestoneRawRanges, + withPhaseSection, }; diff --git a/tests/markdown-sectionizer.test.cjs b/tests/markdown-sectionizer.test.cjs index 655b839b3..94047d5cf 100644 --- a/tests/markdown-sectionizer.test.cjs +++ b/tests/markdown-sectionizer.test.cjs @@ -5,7 +5,7 @@ * * Module: gsd-core/bin/lib/markdown-sectionizer.cjs * Exports: stripFencedCode, tokenizeHeadings, collectSections, collectSection, - * iterateBullets, extractTaggedBlocks, replaceSection + * iterateBullets, extractTaggedBlocks, replaceSection, withSection * * Covers the parser QA matrix from CONTRIBUTING.md §'Parser and project-file inputs': * - LF vs CRLF line endings @@ -16,7 +16,9 @@ * - All three bullet markers (dash/checkbox/numbered) + indented continuation lines * - Empty/whitespace/non-string input * - * Includes a fast-check property test (stripFencedCode idempotence invariant). + * Includes fast-check property tests: stripFencedCode idempotence invariant, + * and withSection's ADR-2143 §4 bounded-mutation guarantee (an edit confined to + * one section cannot alter any other section, even with a greedy regex). * The parity guard for the T0-era tracked duplication (stripFencedCode vs * uat-predicate _stripFencedBlocks) was removed in T5: uat-predicate now imports * the seam directly, so the guard would compare the seam to itself. @@ -35,6 +37,7 @@ const { extractTaggedBlocks, stripTaggedBlocks, replaceSection, + withSection, } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); // ─── stripFencedCode ────────────────────────────────────────────────────────── @@ -1053,6 +1056,210 @@ describe('extractTaggedBlocks: nested same-name tag behavior (#2128 stop-at-next }); }); +// ─── withSection ─────────────────────────────────────────────────────────────── + +describe('withSection', () => { + test('edits only the targeted section, leaving surrounding sections untouched', () => { + const content = '## Intro\nIntro body.\n## Name\nOld name body.\n## Footer\nFooter body.\n'; + const result = withSection(content, 'Name', (body) => body.replace('Old name body.', 'New name body.')); + assert.ok(result.includes('## Intro\nIntro body.'), 'Intro section preserved verbatim'); + assert.ok(result.includes('## Name\nNew name body.'), 'Name section updated'); + assert.ok(!result.includes('Old name body.'), 'old body removed'); + assert.ok(result.includes('## Footer\nFooter body.'), 'Footer section preserved verbatim'); + }); + + test('a section not present in content leaves content unchanged (bounded no-op)', () => { + const content = '## A\nBody A\n## B\nBody B\n'; + const result = withSection(content, 'Nonexistent', (body) => body + ' MUTATED'); + assert.equal(result, content, 'no matching heading -> content returned unchanged'); + }); + + test('predicate form: target may be a HeadingToken predicate function', () => { + const content = '## Alpha\nAlpha body\n## Beta\nBeta body\n'; + const result = withSection(content, (h) => h.level === 2 && h.text === 'Beta', (body) => body.toUpperCase()); + assert.ok(result.includes('## Beta\nBETA BODY'), 'predicate-matched section edited'); + assert.ok(result.includes('## Alpha\nAlpha body'), 'non-matched section untouched'); + }); + + test('edit returning the identical body is a no-op (no splice performed)', () => { + const content = '## A\nBody A\n## B\nBody B\n'; + const result = withSection(content, 'A', (body) => body); + assert.equal(result, content, 'identical body -> no-op'); + }); + + test('edit returning a non-string is a no-op (defensive)', () => { + const content = '## A\nBody A\n## B\nBody B\n'; + // Deliberately malformed edit callback (returns a number, not a string). + const result = withSection(content, 'A', () => 42); + assert.equal(result, content, 'non-string return -> no-op'); + }); + + test('non-string content is returned unchanged', () => { + assert.equal(withSection(null, 'A', (b) => b + 'x'), null); + assert.equal(withSection(undefined, 'A', (b) => b + 'x'), undefined); + }); + + test('ADR-2143 §4: a greedy regex inside the edit callback cannot cross a section boundary', () => { + // Build a 3-section document; run a maximally-greedy regex (`[\s\S]*`) inside + // the edit callback for section 2. The callback only ever sees section 2's + // body, so sections 1 and 3 must remain byte-identical. + const content = [ + '## Section One', + 'alpha content line 1', + 'alpha content line 2', + '## Section Two', + 'beta content to be replaced', + '## Section Three', + 'gamma content line 1', + 'gamma content line 2', + ].join('\n') + '\n'; + + const before = collectSection(content, (h) => h.text === 'Section One'); + const afterSectionThree = collectSection(content, (h) => h.text === 'Section Three'); + assert.ok(before !== null && afterSectionThree !== null); + + const result = withSection(content, 'Section Two', (body) => body.replace(/[\s\S]*/, 'REPLACED ENTIRELY')); + + const resultOne = collectSection(result, (h) => h.text === 'Section One'); + const resultThree = collectSection(result, (h) => h.text === 'Section Three'); + assert.equal(resultOne.body, before.body, 'Section One byte-identical after greedy edit on Section Two'); + assert.equal(resultThree.body, afterSectionThree.body, 'Section Three byte-identical after greedy edit on Section Two'); + assert.ok(result.includes('## Section Two\nREPLACED ENTIRELY'), 'Section Two was replaced as intended'); + }); +}); + +describe('withSection: property-based tests', () => { + // Anchored phase-heading predicate mirroring roadmap-parser.cjs's + // withPhaseSection fix: the phase token must sit at the START of the + // heading text, so a section whose TITLE merely mentions another phase + // number is never matched by that other phase's query. + const anchoredPhasePredicate = (k) => { + const re = new RegExp(`^Phase\\s+${k}(?=[\\s:(]|$)`, 'i'); + return (h) => re.test(h.text); + }; + + test('property (ADR-2143 §4): editing phase k never alters any sibling section j≠k', () => { + // Model a ROADMAP-like document with N `## Phase k` sections (k=1..N), each + // with a distinct, generated body. Pick a random k, run withSection to append + // ' EDITED' to that section's body, and assert every OTHER section (j≠k) is + // byte-identical in the output — the bounded-mutation guarantee this seam + // exists to provide (structurally retires the #2130/#2067/#2080 boundary- + // crossing class). + // + // Also exercises Blocker 1 (title-collision hijack): each section's heading + // TITLE may be decorated with a reference to a DIFFERENT phase number + // (e.g. `Phase 3: legacy Phase 1 notes`), and the predicate above (anchored + // to the start of the heading text) must still resolve to the section whose + // OWN number matches, never a differently-numbered section whose title + // happens to mention the queried number. + // + // Heading level is kept UNIFORM (`##`) across sections deliberately: mixing + // random heading levels would make "which section is k" ambiguous for this + // property (a shallower section can syntactically nest a deeper one) — see + // the separate explicit mixed-level test below instead. + fc.assert( + fc.property( + fc.integer({ min: 2, max: 6 }).chain((n) => + fc.record({ + n: fc.constant(n), + bodies: fc.array( + fc.string({ minLength: 1, maxLength: 40 }).filter((s) => !/[\r\n]/.test(s) && s.trim().length > 0), + { minLength: n, maxLength: n }, + ), + targetIdx: fc.integer({ min: 0, max: n - 1 }), + // For each section, optionally reference a DIFFERENT phase number in + // its own heading title (title-collision decoy). `undefined`/self + // means "no decoy for this section". + titleDecoys: fc.array( + fc.option(fc.integer({ min: 1, max: 6 }), { nil: undefined }), + { minLength: n, maxLength: n }, + ), + }), + ), + ({ n, bodies, targetIdx, titleDecoys }) => { + const lines = []; + for (let k = 1; k <= n; k++) { + const decoy = titleDecoys[k - 1]; + const heading = decoy !== undefined && decoy !== k + ? `Phase ${k}: legacy Phase ${decoy} notes` + : `Phase ${k}`; + lines.push(`## ${heading}`); + lines.push(`body-${k}: ${bodies[k - 1]}`); + } + const doc = lines.join('\n') + '\n'; + const targetPhase = targetIdx + 1; + + // Snapshot every section's body BEFORE the edit. + const before = []; + for (let k = 1; k <= n; k++) { + const s = collectSection(doc, anchoredPhasePredicate(k)); + assert.ok(s !== null, `Phase ${k} section must be found before edit`); + before.push(s.body); + } + + const result = withSection( + doc, + anchoredPhasePredicate(targetPhase), + (body) => body + ' EDITED', + ); + + for (let k = 1; k <= n; k++) { + const s = collectSection(result, anchoredPhasePredicate(k)); + assert.ok(s !== null, `Phase ${k} section must still be found after edit`); + if (k === targetPhase) { + assert.equal(s.body, before[k - 1] + ' EDITED', `Phase ${targetPhase} (the target) must be edited`); + } else { + assert.equal(s.body, before[k - 1], `Phase ${k} (j≠k) must be byte-identical after editing Phase ${targetPhase}`); + } + } + }, + ), + ); + }); + + test('mixed heading levels + title collision: withSection({levelBounded:false}) does not fold a deeper heading into the target section, and the anchored predicate is not hijacked by a title mentioning another phase', () => { + // Phase 3 (appearing FIRST) has a title that mentions "Phase 1" — under the + // OLD unanchored regex this would be matched first (document order) by a + // query for phase 1 (Blocker 1). Phase 1 is followed by a DEEPER heading + // (`#### Phase 2`, level 4 vs Phase 1's level 3) — under the default + // `levelBounded: true` this would nest Phase 2 inside Phase 1's section + // and let an edit on Phase 1 reach into it (Blocker 2). + const content = [ + '### Phase 3: Migrate off Phase 1 pipeline', + 'gamma body line', + '### Phase 1: Foundation', + 'alpha body line', + '#### Phase 2: API (deeper level, nested syntactically under Phase 1)', + '**Plans:** 1 plans', + ].join('\n') + '\n'; + + const before2 = collectSection(content, anchoredPhasePredicate(2)); + assert.ok(before2 !== null, 'Phase 2 section must be found before edit'); + + const result = withSection( + content, + anchoredPhasePredicate(1), + (body) => body + ' EDITED', + { levelBounded: false }, + ); + + assert.ok( + result.includes('### Phase 3: Migrate off Phase 1 pipeline\ngamma body line'), + 'Phase 3 (title mentions "Phase 1") is byte-identical — not hijacked by the anchored Phase-1 query', + ); + assert.ok(!result.includes('gamma body line EDITED'), 'the edit did not land in Phase 3'); + + const after2 = collectSection(result, anchoredPhasePredicate(2)); + assert.equal( + after2.body, + before2.body, + 'Phase 2 (deeper level than Phase 1) stays byte-identical — levelBounded:false stopped Phase 1 at the next heading of ANY level', + ); + + assert.ok(result.includes('alpha body line EDITED'), "Phase 1's own body was correctly edited"); + }); +}); + // Parity guard removed in T5 (ADR-1372): uat-predicate now imports stripFencedCode // from the seam directly, so comparing the seam to itself is tautological. // The seam's stripFencedCode correctness is already covered by the tests above. diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index 493cc9c84..0ff47e83d 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -32,6 +32,7 @@ const { getRoadmapPhaseInternal, getMilestoneInfo, getMilestonePhaseFilter, + withPhaseSection, } = roadmapParser; // ─── helpers ───────────────────────────────────────────────────────────────── @@ -732,6 +733,157 @@ describe('roadmap-parser: getMilestonePhaseFilter', () => { }); }); +// ─── withPhaseSection (ADR-2143 §4 — bounded mutation) ──────────────────────── + +describe('roadmap-parser: withPhaseSection', () => { + test('mutating phase k leaves phase j (j≠k) byte-identical', () => { + const content = [ + '# Roadmap', + '', + '### Phase 1: Foundation', + '**Goal:** Setup', + '**Plans:** 1 plans', + '', + '### Phase 2: API', + '**Goal:** Build API', + '**Plans:** 1 plans', + '', + '### Phase 3: Polish', + '**Goal:** Harden', + '**Plans:** 1 plans', + '', + ].join('\n'); + + const result = withPhaseSection(content, '2', (body) => + body.replace(/(\*\*Plans:\*\*\s*)[^\n]+/i, '$11/1 plans complete'), + ); + + assert.ok( + result.includes('### Phase 2: API\n**Goal:** Build API\n**Plans:** 1/1 plans complete'), + 'phase 2 (the target) is updated', + ); + assert.ok( + result.includes('### Phase 1: Foundation\n**Goal:** Setup\n**Plans:** 1 plans'), + 'phase 1 (j≠k) is byte-identical', + ); + assert.ok( + result.includes('### Phase 3: Polish\n**Goal:** Harden\n**Plans:** 1 plans'), + 'phase 3 (j≠k) is byte-identical', + ); + }); + + test('a greedy edit callback cannot escape phase N\'s own section', () => { + const content = [ + '### Phase 1: Alpha', + 'alpha body', + '### Phase 2: Beta', + 'beta body', + '### Phase 3: Gamma', + 'gamma body', + ].join('\n') + '\n'; + + const result = withPhaseSection(content, '2', (body) => body.replace(/[\s\S]*/, 'REPLACED')); + assert.ok(result.includes('### Phase 1: Alpha\nalpha body'), 'Phase 1 untouched by a greedy regex targeting Phase 2'); + assert.ok(result.includes('### Phase 3: Gamma\ngamma body'), 'Phase 3 untouched by a greedy regex targeting Phase 2'); + assert.ok(result.includes('### Phase 2: Beta\nREPLACED'), 'Phase 2 was the intended target'); + }); + + test('no matching phase heading -> content unchanged (bounded no-op)', () => { + const content = '### Phase 1: Foundation\n**Plans:** 1 plans\n'; + const result = withPhaseSection(content, '99', (body) => body + ' MUTATED'); + assert.equal(result, content, 'no Phase 99 heading -> unchanged'); + }); + + test('resolves the phase heading via the #2121 phase-id source (zero-padding tolerant)', () => { + const content = [ + '### Phase 02: Padded', + '**Plans:** 1 plans', + '### Phase 3: Next', + '**Plans:** 1 plans', + ].join('\n') + '\n'; + + // Query with the un-padded form ("2") — must still resolve "Phase 02". + const result = withPhaseSection(content, '2', (body) => body.replace('1 plans', '1/1 plans complete')); + assert.ok(result.includes('### Phase 02: Padded\n**Plans:** 1/1 plans complete'), 'un-padded query resolves padded heading'); + assert.ok(result.includes('### Phase 3: Next\n**Plans:** 1 plans'), 'Phase 3 untouched'); + }); + + test('a query for phase "1" does not prefix-match a decimal sub-phase heading "Phase 1.1"', () => { + // Sub-phase appears BEFORE the parent phase in the document, so a bare + // `\b`-terminated regex (which would match "1" as a prefix of "1.1") + // could resolve the wrong (first-encountered) section. + const content = [ + '### Phase 1.1: Sub', + 'sub body', + '### Phase 1: Base', + 'base body', + ].join('\n') + '\n'; + + const result = withPhaseSection(content, '1', (body) => body + ' EDITED'); + assert.ok(result.includes('### Phase 1.1: Sub\nsub body\n'), 'Phase 1.1 body is byte-identical (untouched)'); + assert.ok(!result.includes('sub body EDITED'), 'the edit did not land in Phase 1.1'); + assert.ok(result.includes('### Phase 1: Base\nbase body EDITED'), "Phase 1's own body received the edit"); + + const subResult = withPhaseSection(content, '1.1', (body) => body + ' EDITED'); + assert.ok(subResult.includes('### Phase 1.1: Sub\nsub body EDITED'), "Phase 1.1's own body received the edit"); + assert.ok(subResult.includes('### Phase 1: Base\nbase body\n'), 'Phase 1 body is byte-identical (untouched)'); + }); + + test('Blocker 1 regression: a query for phase "1" is not hijacked by a sibling phase whose TITLE mentions "Phase 1"', () => { + // Phase 3's own title mentions "Phase 1" ("Migrate off Phase 1 pipeline") + // and appears BEFORE the real Phase 1 heading in document order. Under the + // OLD unanchored regex (`(?:^|\s)Phase\s+1(?=[\s:(]|$)`), that substring + // inside Phase 3's heading text would match first — and because + // `collectSection` picks the FIRST matching heading, `withPhaseSection` + // would edit Phase 3's section instead of Phase 1's. + const content = [ + '### Phase 3: Migrate off Phase 1 pipeline', + '**Plans:** 1 plans', + '### Phase 1: Foundation', + '**Plans:** 1 plans', + ].join('\n') + '\n'; + + const result = withPhaseSection(content, '1', (body) => + body.replace(/(\*\*Plans:\*\*\s*)[^\n]+/, '$1DONE'), + ); + + assert.ok( + result.includes('### Phase 1: Foundation\n**Plans:** DONE'), + "Phase 1's own Plans line is edited", + ); + assert.ok( + result.includes('### Phase 3: Migrate off Phase 1 pipeline\n**Plans:** 1 plans'), + 'Phase 3 (title mentions "Phase 1") is byte-identical — not hijacked', + ); + }); + + test('Blocker 2 regression: a following DEEPER heading is not folded into phase 1\'s section body', () => { + // Phase 1 is `###` (level 3); the very next heading, `#### Phase 2: API` + // (level 4), is DEEPER than Phase 1. Under the default `levelBounded: true` + // stop rule, a deeper heading does not terminate the section (it only stops + // at a heading whose level <= the target's own level), so Phase 2's whole + // section — including its `**Plans:**` line — would be folded into Phase + // 1's body and reachable by `edit`. + const content = [ + '### Phase 1: Foundation', + '#### Phase 2: API', + '**Plans:** 1 plans', + '### Phase 3: Polish', + '**Plans:** 1 plans', + ].join('\n') + '\n'; + + const phase2Snippet = '#### Phase 2: API\n**Plans:** 1 plans'; + assert.ok(content.includes(phase2Snippet), 'sanity: fixture contains the expected Phase 2 snippet'); + + const result = withPhaseSection(content, '1', (body) => `${body}[EDITED]`); + + assert.ok( + result.includes(phase2Snippet), + "Phase 2's heading + Plans line stay contiguous and byte-identical — Phase 1's edit did not reach into it", + ); + assert.ok(!result.includes('1 plans[EDITED]'), "the edit did not land inside Phase 2's Plans line"); + }); +}); // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-2554-decimal-phase-filter.test.cjs — consolidation epic #1969 (B3 #1972)