fix(#2128): address shared-seam review regressions (#557, action-scan, comment-strip)

Final convergence review found the shared <tag> seam introduced 3 behavior
regressions; fixed all + locked with tests:

- #557 REGRESSION: stripTaggedBlocks's attribute-tolerance stripped `<details open>`
  (the ACTIVE-milestone marker) that the old `<details>`-only regex preserved. The
  seam now takes `allowAttributes` (default false) — details/decisions strip is
  attr-INTOLERANT (preserves `<details open>`); only `<task type="…">` opts in.
  Regression test added to roadmap-parser + markdown-sectionizer suites.
- verify.cts actionZones (negative-grep-echo security scan): reverted to a bounded
  to-first-close scan `<action>([\s\S]{0,20000}?)</action>` so a grep-echo trick
  can't hide behind an unterminated inner <action> (the seam's stop-at-next-open
  would drop it). ReDoS-safe via the cap.
- check-command-router HTML-comment strip: `(?:-->|$)` fallback wiped to EOF
  (fail-closed spurious gate block) — replaced with stop-at-next-open so an
  unclosed `<!--` leaves downstream tags intact.
- Updated the extractTaggedBlocks nested-tag tests to the new (stop-at-next-open)
  behavior: `<x><x>inner</x></x>` -> ['inner'].

All vectors still linear; every fix verified in-process.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

.changeset/phase-id-redos-hardening.md

@@ -0,0 +1,5 @@
+---
+type: Security
+pr: 0
+---
+**Hardened phase/roadmap/plan markdown parsing against quadratic-time (ReDoS) CPU exhaustion** — a crafted `ROADMAP.md`, `STATE.md`, or `PLAN.md` with large runs of unclosed `(`, `[`, `<tag>`, `<!--`, or `<details>` could drive the phase-header, Plans-count, `files_modified`, and `<tag>`-block parsers into O(n²) scans (tens of seconds on a ~1.5 MB file). Every affected regex is now linear: header tag/bracket clauses are length-bounded, the Plans-count scan is section-local, and all `<tag>…</tag>` extraction routes through a single ReDoS-safe seam. (#2128)

src/check-command-router.cts

@@ -143,7 +143,10 @@ const XML_DECISION_TAGS_RE = /<(?:objective|tasks?|action)(?:\s[^>]{0,1000})?>((
 
 function stripCommentsAndFences(text: string): string {
   // HTML-comment stripping stays caller-side (the seam does not strip HTML comments).
-  const htmlStripped = text.replace(/<!--[\s\S]*?(?:-->|$)/g, ' ');
+  // Stop-at-next-open body (ReDoS-safe, #2128); an UNCLOSED `<!--` does not match,
+  // so downstream tags are preserved (unlike a `(?:-->|$)` fallback, which would
+  // wipe to EOF and fail-close the decision-coverage gate).
+  const htmlStripped = text.replace(/<!--(?:(?!<!--)[\s\S])*?-->/g, ' ');
   // Fenced-code stripping: delegate to the canonical CommonMark-correct seam.
   // replaces the prior independent regex copy (```` ``` ``` ````  + `~~~ ~~~`).
   return stripFencedCode(htmlStripped).text;

src/markdown-sectionizer.cts

@@ -520,22 +520,25 @@ export function iterateBullets(sectionText: string): BulletItem[] {
  * fenced code blocks itself. If a `<tagName>` block appears inside a fenced code
  * block and should be excluded, the caller should apply `stripFencedCode` first.
  *
- * **Nested tags are NOT supported.** The underlying regex uses a non-greedy
- * `[\s\S]*?` match, which means it closes at the FIRST `</tagName>` encountered.
- * Given `<x><x>inner</x></x>`, `extractTaggedBlocks(content, 'x')` returns
- * `['<x>inner']` — the inner `<x>` is captured as literal text, and the second
- * `</x>` is left unmatched (or matched as a second block with empty inner text
- * if another `<x>` follows). Callers that need to handle nested tags must
- * pre-process the input or use a proper XML/HTML parser.
+ * **Nested tags are NOT supported.** The body scan terminates at the NEXT
+ * opening of the same tag (the ReDoS-safe boundary, #2128). Given
+ * `<x><x>inner</x></x>`, `extractTaggedBlocks(content, 'x')` returns `['inner']`
+ * — the well-formed inner block; the unterminated outer `<x>` is skipped.
+ * Callers that need true nesting must use a proper XML/HTML parser.
+ *
+ * `allowAttributes` (default `false`): when `true`, the opening tag may carry
+ * bounded attributes (`<tag foo="x">`) — needed for `<task type="…">` blocks.
+ * Leave `false` for tags that must match exactly (e.g. `<decisions>`), and never
+ * enable it for a tag where an attributed form is semantically distinct.
  *
  * Generalises `decisions.cts`'s bespoke `matchAll(/<decisions>([\s\S]*?)<\/decisions>/g)`
  * so tier T1 can drop its own copy (tracked duplication until T1 lands).
  */
-export function extractTaggedBlocks(content: string, tagName: string): string[] {
+export function extractTaggedBlocks(content: string, tagName: string, allowAttributes = false): string[] {
   if (typeof content !== 'string' || content.length === 0) return [];
   if (typeof tagName !== 'string' || tagName.length === 0) return [];
 
-  const pattern = taggedBlockPattern(tagName, 'g');
+  const pattern = taggedBlockPattern(tagName, 'g', allowAttributes);
   const results: string[] = [];
   let match: RegExpExecArray | null;
   while ((match = pattern.exec(content)) !== null) {
@@ -548,30 +551,37 @@ export function extractTaggedBlocks(content: string, tagName: string): string[]
  * Build the single, ReDoS-safe `<tag>…</tag>` block regex shared by
  * `extractTaggedBlocks` (extract bodies) and `stripTaggedBlocks` (remove blocks).
  *
- * Safety: the body uses a `(?:(?!<tag[\s>])[\s\S])*?` negative-lookahead scan
- * that terminates at the NEXT opening `<tag>` instead of lazily rescanning the
- * whole remaining document for a `</tag>` that may never appear — so a large
- * document full of unclosed `<tag>` openings stays LINEAR, not quadratic
- * (#2128). The opening tag tolerates optional attributes (`<tag foo="bar">`),
- * bounded to 1000 chars so the attribute scan cannot itself ReDoS.
- * Group 1 is the block body.
+ * Safety: the body terminates at the NEXT opening of this tag (stop-at-next-open)
+ * instead of lazily rescanning the whole remaining document for a `</tag>` that
+ * may never appear — so a document full of unclosed `<tag>` openings scans
+ * LINEARLY, not quadratically (#2128). Group 1 is the block body.
+ *
+ * `allowAttributes`: when `true`, the opener accepts bounded attributes
+ * (`<tag foo="x">`) and the body boundary is `<tag` followed by a space or `>`.
+ * When `false`, the opener is the EXACT `<tag>` and the boundary is exact `<tag>`,
+ * so an attributed `<tag foo>` is neither an opener nor a boundary — it is body
+ * content. That exact form is load-bearing for `<details>` stripping: `<details
+ * open>` marks the ACTIVE milestone and must be preserved, not stripped (#557).
  */
-function taggedBlockPattern(tagName: string, flags: string): RegExp {
+function taggedBlockPattern(tagName: string, flags: string, allowAttributes: boolean): RegExp {
   const esc = tagName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
-  return new RegExp(`<${esc}(?:\\s[^>]{0,1000})?>((?:(?!<${esc}[\\s>])[\\s\\S])*?)</${esc}>`, flags);
+  const open = allowAttributes ? `<${esc}(?:\\s[^>]{0,1000})?>` : `<${esc}>`;
+  const boundary = allowAttributes ? `<${esc}[\\s>]` : `<${esc}>`;
+  return new RegExp(`${open}((?:(?!${boundary})[\\s\\S])*?)</${esc}>`, flags);
 }
 
 /**
  * Remove every `<tagName>…</tagName>` block (opening tag, body, and closing tag)
  * from `content`. The ReDoS-safe counterpart to `extractTaggedBlocks` — same
- * hardened pattern, `.replace(…, '')` instead of body extraction. Case-insensitive
- * by default (matching the `<details>` strip call sites); pass `caseSensitive`
- * to force exact-case matching.
+ * hardened pattern, `.replace(…, '')` instead of body extraction. `allowAttributes`
+ * defaults to `false` so `<details open>` (active milestone) is preserved (#557);
+ * case-insensitive by default (matching the `<details>` strip call sites), pass
+ * `caseSensitive` to force exact-case matching.
  */
-export function stripTaggedBlocks(content: string, tagName: string, caseSensitive = false): string {
+export function stripTaggedBlocks(content: string, tagName: string, allowAttributes = false, caseSensitive = false): string {
   if (typeof content !== 'string' || content.length === 0) return '';
   if (typeof tagName !== 'string' || tagName.length === 0) return content;
-  return content.replace(taggedBlockPattern(tagName, caseSensitive ? 'g' : 'gi'), '');
+  return content.replace(taggedBlockPattern(tagName, caseSensitive ? 'g' : 'gi', allowAttributes), '');
 }
 
 // ─── replaceSection ───────────────────────────────────────────────────────────

src/verify.cts

@@ -206,7 +206,15 @@ function scanNegativeGrepCommentEcho(content: string): { errors: string[]; warni
   //    while a prose echo on the same line is still caught.
   const cmdSpanRe =
     /grep(?:\s+-{1,2}[A-Za-z][A-Za-z-]*)+\s+(?:'[^']*'|"[^"]*"|[^\s'"|>&;]+)[^\n]*?(?:==|-eq|=)\s*0\b/g;
-  const actionZones = extractTaggedBlocks(text, 'action');
+  // Security scan: must see the FULL text up to the first </action> — including a
+  // malformed inner <action> — so a grep-echo-0 trick cannot hide behind a
+  // deliberately-unclosed tag. Use a bounded to-first-close scan (ReDoS-safe via
+  // the {0,20000} cap, #2128), NOT the stop-at-next-open extractTaggedBlocks seam
+  // (which would drop the span before an unterminated inner <action>).
+  const actionZones: string[] = [];
+  const actionRe = /<action>([\s\S]{0,20000}?)<\/action>/g;
+  let acm: RegExpExecArray | null;
+  while ((acm = actionRe.exec(text)) !== null) actionZones.push(acm[1]);
   const scannableActionText = actionZones.map((zone) => zone.replace(cmdSpanRe, ' ')).join('\n');
 
   // 3. Per shell SEGMENT (split lines on && / ||) extract count-grep literals and
@@ -363,7 +371,7 @@ function scanFileWideNegativeGateConflict(content: string): { warnings: string[]
     reqText: string;   // <action>+<acceptance_criteria> text (requirement side)
   }
   const tasks: TaskInfo[] = [];
-  for (const tc of extractTaggedBlocks(text, 'task')) {
+  for (const tc of extractTaggedBlocks(text, 'task', true)) {
     // Extract task name.
     const namem = extractTaggedBlocks(tc, 'name');
     const name = namem.length ? namem[0].trim() : 'unnamed';
@@ -565,7 +573,7 @@ function cmdVerifyPlanStructure(cwd: string, filePath: string, raw: boolean): vo
   }
 
   const tasks: Record<string, unknown>[] = [];
-  for (const taskContent of extractTaggedBlocks(content, 'task')) {
+  for (const taskContent of extractTaggedBlocks(content, 'task', true)) {
     const nameArr = extractTaggedBlocks(taskContent, 'name');
     const taskName = nameArr.length ? nameArr[0].trim() : 'unnamed';
     const hasFiles = /<files>/.test(taskContent);

tests/markdown-sectionizer.test.cjs

@@ -33,6 +33,7 @@ const {
   collectSection,
   iterateBullets,
   extractTaggedBlocks,
+  stripTaggedBlocks,
   replaceSection,
 } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs');
 
@@ -1015,15 +1016,14 @@ describe('stripFencedCode and tokenizeHeadings: backtick info string with backti
 
 // ─── FIX 6: extractTaggedBlocks — nested tag behavior ─────────────────────────
 
-describe('extractTaggedBlocks: nested same-name tag behavior (non-greedy limitation)', () => {
-  test('nested <x><x>…</x></x> closes at first </x> (non-greedy; nested tags not supported)', () => {
-    // Non-greedy match: <x>([\s\S]*?)</x> closes at the FIRST </x>.
-    // So <x><x>inner</x></x> → first block captures "<x>inner", second </x> is unmatched.
+describe('extractTaggedBlocks: nested same-name tag behavior (#2128 stop-at-next-open)', () => {
+  test('nested <x><x>inner</x></x> extracts the well-formed inner block', () => {
+    // #2128: the ReDoS-safe body scan terminates at the NEXT opening <x>, so the
+    // unterminated outer <x> is skipped and the inner block is extracted.
     const content = '<x><x>inner</x></x>';
     const result = extractTaggedBlocks(content, 'x');
-    // The first match closes at the first </x>, capturing "<x>inner"
-    assert.equal(result.length, 1, 'non-greedy match produces exactly one result from nested input');
-    assert.equal(result[0], '<x>inner', 'inner capture is the content up to the first closing tag');
+    assert.equal(result.length, 1, 'exactly one result from nested input');
+    assert.equal(result[0], 'inner', 'the well-formed inner block is extracted; the unterminated outer is skipped');
   });
 
   test('back-to-back blocks (not nested) are both extracted', () => {
@@ -1033,6 +1033,24 @@ describe('extractTaggedBlocks: nested same-name tag behavior (non-greedy limitat
     assert.equal(result[0], 'first');
     assert.equal(result[1], 'second');
   });
+
+  test('#2128: a document full of unclosed <x> openings stays linear and yields no match', () => {
+    const content = '<x>a\n'.repeat(50) + 'no closing tag';
+    assert.deepEqual(extractTaggedBlocks(content, 'x'), [], 'no </x> anywhere -> no blocks');
+  });
+
+  test('#557 / #2128: attr-intolerant by default preserves <details open>; opt-in matches <task type=…>', () => {
+    // stripTaggedBlocks(details) must PRESERVE <details open> (the active-milestone
+    // marker) and strip only bare <details>; extractTaggedBlocks(task, true) must
+    // match attributed tasks, and must NOT when allowAttributes is left false.
+    assert.equal(
+      stripTaggedBlocks('X<details>shipped</details>Y<details open>active</details>Z', 'details'),
+      'XY<details open>active</details>Z',
+      '#557: <details open> preserved; bare <details> stripped',
+    );
+    assert.deepEqual(extractTaggedBlocks('<task type="auto">body</task>', 'task', true), ['body'], 'attributed task matched with allowAttributes=true');
+    assert.deepEqual(extractTaggedBlocks('<task type="auto">body</task>', 'task'), [], 'attributed task NOT matched with allowAttributes=false');
+  });
 });
 
 // Parity guard removed in T5 (ADR-1372): uat-predicate now imports stripFencedCode

tests/roadmap-parser.test.cjs

@@ -77,6 +77,19 @@ describe('roadmap-parser: stripShippedMilestones', () => {
     assert.ok(!result.includes('closed content'), 'content removed');
     assert.ok(result.includes('after'), 'after content preserved');
   });
+
+  test('#557: preserves an active <details open> block while stripping shipped bare <details>', () => {
+    // <details open> marks the ACTIVE milestone (roadmap.analyze must still see its
+    // phases); only closed/shipped bare <details> blocks are stripped. Regression for
+    // #557, which the #2128 shared-seam migration briefly reintroduced via the seam's
+    // attribute-tolerance — the details strip is now attr-INTOLERANT to keep #557 fixed.
+    const input = '<details>\nshipped phase\n</details>\n<details open>\n- [ ] **Phase 9: Active**\n</details>\nafter';
+    const result = stripShippedMilestones(input);
+    assert.ok(!result.includes('shipped phase'), 'shipped bare <details> stripped');
+    assert.ok(result.includes('<details open>'), 'active <details open> tag preserved');
+    assert.ok(result.includes('Phase 9: Active'), 'active-milestone phases preserved');
+    assert.ok(result.includes('after'), 'trailing content preserved');
+  });
 });
 
 // ─── extractCurrentMilestone ──────────────────────────────────────────────────