diff --git a/.changeset/curious-pandas-howl.md b/.changeset/curious-pandas-howl.md new file mode 100644 index 000000000..fbbc13626 --- /dev/null +++ b/.changeset/curious-pandas-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2253 +--- +**state record-metric no longer appends per-plan rows into the By-Phase velocity table** — it now maintains its own Per-Plan Metrics table (self-created on first use), and its auto-create scaffold header is corrected. (#2253) diff --git a/.changeset/humble-tunas-travel.md b/.changeset/humble-tunas-travel.md new file mode 100644 index 000000000..0b4cc9104 --- /dev/null +++ b/.changeset/humble-tunas-travel.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2253 +--- +**`phase remove` no longer destroys the Progress table when removing the last phase** — deleting a phase used a whole-document regex whose scan, on the final phase, ran past the section and swept away the `## Progress` heading and its entire tracking table; the deletion is now structurally bounded to the phase’s own section. (#2253) diff --git a/.changeset/steady-lynx-fly.md b/.changeset/steady-lynx-fly.md new file mode 100644 index 000000000..ef8bc0678 --- /dev/null +++ b/.changeset/steady-lynx-fly.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2253 +--- +**Roadmap, requirements, and state table edits are confined to the right table** — the last ad-hoc table writers (phase completion updating roadmap progress, `requirements mark-complete`, and `state record-metric`/velocity) now route through the shared markdown-table seam, so a stray decoy table elsewhere in a document can no longer swallow a phase-progress update, a single ragged neighbouring row no longer silently aborts the whole edit, and per-plan metric recording no longer drops trailing section content or duplicates the section. (#2253) diff --git a/.changeset/sturdy-wolves-gather.md b/.changeset/sturdy-wolves-gather.md new file mode 100644 index 000000000..ec4efcef9 --- /dev/null +++ b/.changeset/sturdy-wolves-gather.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2253 +--- +**STATE.md `## Session` fields now resolve on Windows** — the session-section reader used a `\n`-only heading regex that silently failed on a CRLF `## Session` heading, nulling all session state on Windows checkouts; it now reads through the CRLF-safe section seam. (#2253) diff --git a/eslint-rules/no-adhoc-markdown-parsing.cjs b/eslint-rules/no-adhoc-markdown-parsing.cjs index 309ce6dc7..af2d36adc 100644 --- a/eslint-rules/no-adhoc-markdown-parsing.cjs +++ b/eslint-rules/no-adhoc-markdown-parsing.cjs @@ -4,7 +4,8 @@ * no-adhoc-markdown-parsing * * Flags hand-rolled markdown-structure scanning in src/*.cts that duplicates - * the canonical seam (src/markdown-sectionizer.cts). Applies to two patterns: + * the canonical seam (src/markdown-sectionizer.cts, src/markdown-table.cts). + * Applies to three patterns: * * 1. FENCE-BLOCK-STRIP — regex literals whose source contains a triple-backtick * or triple-tilde fence delimiter AND a multiline body @@ -22,12 +23,61 @@ * Fingerprint: [\\s\\S] (multiline body) AND (?= lookahead * that references a heading anchor #. * + * 3. TABLE-REGEX — regex literals whose source contains an escaped pipe + * (\|, the GFM table-cell delimiter) AND a negated-pipe + * cell-capture character class ([^|] / [^\|]), e.g. + * /\|[^|]*\|/. This is the fingerprint of a hand-rolled + * markdown table-row/cell scan (ADR-2143 §7). Kept + * deliberately conservative — a regex with an escaped + * pipe but no negated-pipe cell class (e.g. a bare + * `\|` delimiter probe) is NOT flagged, to keep the + * false-positive rate low. + * + * ALSO inspects `new RegExp()` where `` is a + * string Literal or a TemplateLiteral (ADR-2143 §7 + * Phase 4) — a dynamically-built table-row pattern such + * as `new RegExp(`^(\\|\\s*${phase}...[^|]*)$`)` is the + * exact same hand-rolled table scan, just constructed + * at runtime instead of written as a literal. Only the + * STATIC text is inspected: for a TemplateLiteral, the + * quasis (cooked, escapes resolved) are concatenated + * and any `${...}` expression contributes nothing — + * conservative by design, so a dynamic segment can + * never manufacture or hide the fingerprint. A `new + * RegExp(someIdentifier)` IS ALSO inspected (#2245 + * audit) when `someIdentifier` resolves to a same- + * scope `const` declaration whose initializer is + * itself a regex Literal, a string Literal, or a + * TemplateLiteral — mirroring the ADHOC-REPLACE- + * MUTATION resolver below. A `let`/`var` binding, a + * function parameter, a call result, or string + * concatenation is deliberately NOT resolved — a real, + * documented boundary, not a recall hole. + * + * 4. ADHOC-REPLACE-MUTATION — a `.replace(` call whose receiver identifier + * name matches /roadmap|state|reqContent|content/i AND + * whose first argument is a regex Literal or a `new + * RegExp()` matching EITHER + * the TABLE-REGEX or the SECTION-COLLECT fingerprint + * (ADR-2143 §7 Phase 4). Targets the specific ad-hoc + * write pattern `roadmapContent.replace(tableRowPattern, + * ...)` / `stateContent.replace(sectionPattern, ...)` — + * a mutation of a roadmap/state document via a hand- + * rolled table or section regex. `withSection(...)` / + * `withPhaseSection(...)` / `updateTableCell(...)` + * calls (the canonical seam mutators) are never + * `.replace(` calls on such a receiver and so never trip + * this — nor does `.replace()` on a receiver whose name + * doesn't match the roadmap/state fingerprint. + * * Per-finding exemption: add // allow-adhoc-markdown: as a * trailing comment on the same source line, OR as a standalone comment on the * line immediately preceding the flagged node. (Mirrors no-source-grep's * // allow-test-rule: mechanism but is scoped to individual findings.) * - * Authors must import from src/markdown-sectionizer.cts instead. + * Authors must import from src/markdown-sectionizer.cts (fences/sections) or + * src/markdown-table.cts (parseMarkdownTable / findTableWithColumns / + * updateTableCell / TABLE_SCHEMAS) instead. */ /** @type {import('eslint').Rule.RuleModule} */ @@ -36,7 +86,7 @@ const rule = { type: 'problem', docs: { description: - 'Disallow hand-rolled markdown-structure scanning (fence-block-strip, section-collect) in src/*.cts — import the markdown-sectionizer seam instead.', + 'Disallow hand-rolled markdown-structure scanning (fence-block-strip, section-collect, table-regex) in src/*.cts — import the markdown-sectionizer / markdown-table seam instead.', category: 'Best Practices', }, schema: [], @@ -45,6 +95,10 @@ const rule = { 'Ad-hoc fence-block-strip regex detected (triple-fence delimiter + multiline body). Import stripFencedCode() from ./markdown-sectionizer instead. Suppress with: // allow-adhoc-markdown: ', sectionCollect: 'Ad-hoc section-collect regex detected (heading + [\\s\\S]*? + lookahead). Import collectSection() from ./markdown-sectionizer instead. Suppress with: // allow-adhoc-markdown: ', + tableRegex: + 'Ad-hoc table-row/cell regex detected (escaped pipe + negated-pipe cell-capture class). Use parseMarkdownTable() / findTableWithColumns() / TABLE_SCHEMAS from ./markdown-table instead. Suppress with: // allow-adhoc-markdown: ', + adhocReplaceMutation: + 'Ad-hoc .replace() mutation of a roadmap/state document using a hand-rolled table or section regex. Use updateTableCell() (./markdown-table) or withSection()/withPhaseSection() (./markdown-sectionizer) instead. Suppress with: // allow-adhoc-markdown: ', }, }, @@ -96,9 +150,7 @@ const rule = { // /(#{1,6}...\n)([\s\S]*?)(?=\n#{...}|$)/ // The key fingerprint is: [\\s\\S] (or [\s\S]) AND (?= (lookahead) AND # in // the same regex, forming the "body up to next heading" construct. - function isSectionCollectRegex(node) { - if (node.type !== 'Literal' || !node.regex) return false; - const src = node.regex.pattern || ''; + function isSectionCollectRegexSource(src) { // Must contain [\s\S] (the non-greedy body) const hasMultilineBody = src.includes('[\\s\\S]') || src.includes('[\\S\\s]'); if (!hasMultilineBody) return false; @@ -107,6 +159,165 @@ const rule = { return hasHeadingLookahead; } + function isSectionCollectRegex(node) { + if (node.type !== 'Literal' || !node.regex) return false; + return isSectionCollectRegexSource(node.regex.pattern || ''); + } + + // ── Table-regex detection ─────────────────────────────────────────────── + // A regex source containing an escaped pipe (\| — the GFM table-cell + // delimiter) AND a negated-pipe cell-capture character class ([^|] or + // [^\|]), indicating a hand-rolled table-row/cell scan such as /\|[^|]*\|/. + // Conservative by design: a bare escaped-pipe delimiter probe with no + // negated-pipe cell class is NOT flagged. + function isTableRegexSource(src) { + // Must contain an escaped pipe + if (!src.includes('\\|')) return false; + // Must ALSO contain a negated-pipe cell-capture class: [^|] or [^\|] + return /\[\^\\?\|\]/.test(src); + } + + function isTableRegex(node) { + if (node.type !== 'Literal' || !node.regex) return false; + return isTableRegexSource(node.regex.pattern || ''); + } + + // ── new RegExp() source extraction ───── + // Builds the EFFECTIVE regex source text for a `new RegExp(...)` call so + // the same fingerprint checks above can run against it. Only a string + // Literal or a TemplateLiteral first argument is inspected — anything else + // (an Identifier, a call, string concatenation via `+`) is out of scope, + // keeping this conservative (no false positives from an un-inspectable + // dynamic pattern). For a TemplateLiteral, only the STATIC quasis (cooked — + // escapes already resolved, matching what `node.regex.pattern` gives for a + // literal) are concatenated; every `${...}` expression contributes nothing, + // so a dynamic segment can neither manufacture nor hide the fingerprint. + /** + * Same scope walk as `resolveVariableInit` below (~line 224), but + * additionally requires the binding be a `const` declaration (#2245 + * audit). Used ONLY by `getNewRegExpSource`'s Identifier-resolution + * branch: a `let`/`var` regex identifier can be reassigned elsewhere in + * its scope, so trusting its FIRST initializer would be unsound in a way + * a `const` binding's initializer never is. A function parameter or a + * call-result initializer already fails the VariableDeclarator/`init` + * shape check and returns `null` regardless. + */ + function resolveConstVariableInit(identifierName, scope) { + let s = scope; + while (s) { + const variable = s.variables.find((v) => v.name === identifierName); + if (variable) { + const def = variable.defs && variable.defs[0]; + if ( + def + && def.node + && def.node.type === 'VariableDeclarator' + && def.node.init + && def.parent + && def.parent.type === 'VariableDeclaration' + && def.parent.kind === 'const' + ) { + return def.node.init; + } + return null; + } + s = s.upper; + } + return null; + } + + function getNewRegExpSource(node, scope) { + if (node.type !== 'NewExpression') return null; + if (!node.callee || node.callee.type !== 'Identifier' || node.callee.name !== 'RegExp') return null; + const arg = node.arguments && node.arguments[0]; + if (!arg) return null; + if (arg.type === 'Literal' && typeof arg.value === 'string') { + return arg.value; + } + if (arg.type === 'TemplateLiteral') { + return arg.quasis.map((q) => (q.value && q.value.cooked) || '').join(''); + } + // Identifier resolution (#2245 audit — recall-hole fix): `new + // RegExp(tableRe)` where `const tableRe = /.../` (or a string/template + // literal) escaped the fingerprint entirely before this. Conservative + // by design: only a const-declared literal/template initializer is + // followed; anything else (param, call, `let`/`var`, concatenation) + // resolves to `null` and is silently out of scope, same as before. + if (arg.type === 'Identifier' && scope) { + const init = resolveConstVariableInit(arg.name, scope); + if (init) { + if (init.type === 'Literal' && init.regex) return init.regex.pattern || ''; + if (init.type === 'Literal' && typeof init.value === 'string') return init.value; + if (init.type === 'TemplateLiteral') { + return init.quasis.map((q) => (q.value && q.value.cooked) || '').join(''); + } + } + } + return null; + } + + function isNewRegExpTableRegex(node, scope) { + const src = getNewRegExpSource(node, scope); + return src !== null && isTableRegexSource(src); + } + + // ── ADHOC-REPLACE-MUTATION detection (ADR-2143 §7 Phase 4) ────────────── + // A `.replace(` call whose receiver is a roadmap/state-ish identifier AND + // whose first argument resolves (directly, or via a same-scope `const` + // declaration) to a regex Literal or `new RegExp(...)` matching either the + // TABLE or SECTION-COLLECT fingerprint. + const REPLACE_RECEIVER_RE = /roadmap|state|reqContent|content/i; + + /** Resolve a Literal-regex or new-RegExp(...) source directly from `node`. */ + function directRegexSource(node) { + if (node.type === 'Literal' && node.regex) return node.regex.pattern || ''; + const newRegExpSrc = getNewRegExpSource(node); + if (newRegExpSrc !== null) return newRegExpSrc; + return null; + } + + /** Walk up the scope chain from `scope` to find `identifierName`'s declared initializer. */ + function resolveVariableInit(identifierName, scope) { + let s = scope; + while (s) { + const variable = s.variables.find((v) => v.name === identifierName); + if (variable) { + const def = variable.defs && variable.defs[0]; + if (def && def.node && def.node.type === 'VariableDeclarator' && def.node.init) { + return def.node.init; + } + return null; + } + s = s.upper; + } + return null; + } + + /** Resolve the effective regex source for a `.replace()` pattern argument. */ + function resolveReplacePatternSource(argNode, scope) { + const direct = directRegexSource(argNode); + if (direct !== null) return direct; + if (argNode.type === 'Identifier') { + const init = resolveVariableInit(argNode.name, scope); + if (init) return directRegexSource(init); + } + return null; + } + + function isAdhocReplaceMutation(node, scope) { + if (node.type !== 'CallExpression') return false; + const callee = node.callee; + if (!callee || callee.type !== 'MemberExpression' || callee.computed) return false; + if (!callee.property || callee.property.name !== 'replace') return false; + const receiver = callee.object; + if (!receiver || receiver.type !== 'Identifier' || !REPLACE_RECEIVER_RE.test(receiver.name)) return false; + const patternArg = node.arguments && node.arguments[0]; + if (!patternArg) return false; + const src = resolveReplacePatternSource(patternArg, scope); + if (src === null) return false; + return isTableRegexSource(src) || isSectionCollectRegexSource(src); + } + return { Literal(node) { // 1. Fence-block-strip regex @@ -122,6 +333,35 @@ const rule = { if (!isAllowed(node)) { context.report({ node, messageId: 'sectionCollect' }); } + return; + } + + // 3. Table-regex (hand-rolled table-row/cell scan) + if (isTableRegex(node)) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'tableRegex' }); + } + } + }, + + // 3b. Table-regex built via new RegExp() + NewExpression(node) { + const scope = context.getScope ? context.getScope() : sourceCode.getScope(node); + if (isNewRegExpTableRegex(node, scope)) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'tableRegex' }); + } + } + }, + + // 4. Ad-hoc .replace() mutation of a roadmap/state document + CallExpression(node) { + const scope = context.getScope ? context.getScope() : sourceCode.getScope(node); + if (isAdhocReplaceMutation(node, scope)) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'adhocReplaceMutation' }); + } } }, }; diff --git a/gsd-core/bin/lib/api-coverage.cjs b/gsd-core/bin/lib/api-coverage.cjs index c51f9b6d7..3ee12dd74 100644 --- a/gsd-core/bin/lib/api-coverage.cjs +++ b/gsd-core/bin/lib/api-coverage.cjs @@ -264,13 +264,12 @@ function parseCoverageMatrix(text) { const src = text.replace(/\r\n/g, '\n'); // (1) fenced ```coverage JSON block takes precedence if present. // Case-insensitive info string (```coverage and ```Coverage are both legal CommonMark). - // allow-adhoc-markdown: extracting a NAMED ```coverage fence (extraction of one tagged block), not stripping all fences — stripFencedCode/extractTaggedBlocks do not cover named-fence extraction. - const fenceMatch = src.match(/```coverage\s*\n([\s\S]*?)\n```/i); - if (fenceMatch && fenceMatch[1]) { + const fenceBody = (0, markdown_sectionizer_cjs_1.extractFencedBlock)(src, 'coverage'); + if (fenceBody) { out.format = 'json'; let parsed; try { - parsed = JSON.parse(fenceMatch[1]); + parsed = JSON.parse(fenceBody); } catch { out.errors.push('fenced ```coverage block is not valid JSON'); diff --git a/gsd-core/bin/lib/state-transition.cjs b/gsd-core/bin/lib/state-transition.cjs index e047f9428..8ea8ec8a6 100644 --- a/gsd-core/bin/lib/state-transition.cjs +++ b/gsd-core/bin/lib/state-transition.cjs @@ -28,7 +28,7 @@ const markdown_sectionizer_cjs_1 = require("./markdown-sectionizer.cjs"); const phase_lifecycle_cjs_1 = require("./phase-lifecycle.cjs"); // eslint-disable-next-line @typescript-eslint/no-require-imports const phaseIdMod = require("./phase-id.cjs"); -const { extractFrontmatter, reconstructFrontmatter } = frontmatter; +const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontmatter; const { escapeRegex } = phaseIdMod; // Stop predicate for section-body slicing: a level-2+ heading ends the section. const STOP_H2_PLUS = (lv) => lv >= 2; @@ -284,8 +284,8 @@ function beginPhaseCore(content, intent, deps) { updated.push('Current focus'); } // ## Current Position section mutation (#1104, #1365). - // ADR-1372 T6: tokenizeHeadings + offset splicing (replaceSection adoption - // deferred to a later phase). Mirrors state.cts:2261-2324 byte-for-behaviour. + // `locateCurrentPosition` (fence-aware, tokenizeHeadings-based) locates + // the section; mirrors state.cts:2261-2324 byte-for-behaviour. body = mutateCurrentPositionFirstTime(body, intent, today, updated); } else { @@ -338,6 +338,20 @@ function sliceCurrentPositionSection(body) { * First-time ## Current Position mutation: update Phase / Plan / Status / * Last activity lines. Mirrors state.cts:2261-2324 byte-for-behaviour * (inline regex first, pipe-table fallback via stateReplaceField — #1257). + * + * F2 (#2245 review, MAJOR): a prior revision of this function used + * `collectSection`/`replaceSection` here, whose default `levelBounded: true` + * only stops the section at the next heading of level <= the opener's own + * level (H1/H2 for a `##`-opened section) — an H3+ subsection nested under + * `## Current Position` was NOT a stop boundary and got folded into + * `sectionBody`, so the field regexes below (which run with the `m` flag, + * matching ANY line start in the body) could clobber a same-named line + * inside that subsection (the #2130/#2067/#2080 truncation/clobber class). + * Restored to the fence-aware `locateCurrentPosition` locator (which stops + * at ANY heading level >= 2, `STOP_H2_PLUS` — H2 through H6) + manual splice, + * exactly matching the `mutateCurrentPositionResume`/ + * `mutateCurrentPositionForAdvance` siblings below, both of which use + * `locateCurrentPosition` directly. */ function mutateCurrentPositionFirstTime(body, intent, today, updated) { const span = locateCurrentPosition(body); @@ -414,25 +428,6 @@ function mutateCurrentPositionResume(body, intent, today, updated) { } return body.slice(0, span.start) + sectionBody + body.slice(span.end); } -/** - * Strip ALL frontmatter blocks from the start of `content`. - * - * TODO (ADR-1769 follow-up): move to `frontmatter.cjs` or `state-document.cjs` - * so it's a shared primitive. Inlined here in Phase 1 to avoid touching - * `state.cjs` (which is the migration target itself) and to keep the Phase 1 - * diff contained. Body is byte-identical to `state.cts:1653 stripFrontmatter` - * (same CRLF + stacked-block handling). - */ -function stripFrontmatter(content) { - let result = content; - while (true) { - const stripped = result.replace(/^\s*---\r?\n[\s\S]*?\r?\n---\s*/, ''); - if (stripped === result) - break; - result = stripped; - } - return result; -} /** * Update fields within the ## Current Position section for advancePlan. * Mirrors `updateCurrentPositionFields` (state.cts:496) byte-for-behaviour: @@ -903,6 +898,70 @@ function milestoneSwitchCore(content, intent, deps) { // ---------------------------------------------------------------------------- // milestoneComplete — intent implementation (Phase 5) // ---------------------------------------------------------------------------- +/** + * Replace a section's ENTIRE body with `newBody`, discarding whatever was + * there — the "wholesale reset" write pattern used by milestoneComplete's + * closure write (## Current Position / ## Operator Next Steps). Retires the + * fence-blind raw regex `(##\s*\s*\n)([\s\S]*?)(?=\n##|$)`, which a + * literal `##` inside a fenced code block in the section body could fool into + * stopping early (the #2130/#2067/#2080 truncation class) — heading location + * here goes through `tokenizeHeadings`, which is fence-aware. + * + * Byte-parity note: the retired regex's greedy `\s*` (before its mandatory + * `\n`) swallowed any blank line(s) immediately after the heading into the + * discarded match, and its non-greedy body match always left exactly ONE + * newline unconsumed before the next heading (or EOF), regardless of how many + * blank lines originally separated the section from what followed. Both + * edges are reproduced explicitly (rather than delegated to `collectSection`'s + * `trimEnd()`-based body, which trims a *different* amount and would drift + * the surrounding blank-line count) so `newBody`'s own leading/trailing + * formatting is exactly what appears in the output. + * + * Returns `null` when no heading matches `headingPredicate` (mirrors the + * retired regex's `pattern.test(body)` miss) — callers fall back to their own + * append-a-new-section path. + */ +function resetSectionVerbatim(content, headingPredicate, newBody) { + const headings = (0, markdown_sectionizer_cjs_1.tokenizeHeadings)(content); + const idx = headings.findIndex(headingPredicate); + if (idx === -1) + return null; + const target = headings[idx]; + const lines = content.split('\n'); + const headingLineEnd = target.offset + lines[target.line - 1].length + 1; + // Swallow blank line(s) immediately after the heading (mirrors the retired + // regex's greedy `\s*` folding them into the discarded match). + // + // F7 (#2245 review, nit): recognise a CRLF blank line (`\r\n`), not only a + // bare LF — a lone `content[bodyStart] === '\n'` check never advances past + // a `\r` byte, so on a CRLF STATE.md the blank line right after the + // heading fell into the DISCARDED [bodyStart, bodyEnd) span instead of the + // KEPT prefix, silently dropping one blank line (contradicting this + // function's own byte-parity docstring). + let bodyStart = headingLineEnd; + while (bodyStart < content.length) { + if (content[bodyStart] === '\n') { + bodyStart += 1; + continue; + } + if (content[bodyStart] === '\r' && content[bodyStart + 1] === '\n') { + bodyStart += 2; + continue; + } + break; + } + // Stop at the next heading of level >= 2 (mirrors the retired regex's + // literal `##` lookahead, which matches any ATX heading two-or-more levels + // deep); leave exactly one newline unconsumed before it, or run to EOF. + let bodyEnd = content.length; + for (let j = idx + 1; j < headings.length; j++) { + if (STOP_H2_PLUS(headings[j].level)) { + bodyEnd = headings[j].offset - 1; + break; + } + } + return content.slice(0, bodyStart) + newBody + content.slice(bodyEnd); +} /** * Apply a `milestoneComplete` transition to STATE.md content. * @@ -917,12 +976,6 @@ function milestoneSwitchCore(content, intent, deps) { * runtime-specific next-milestone slash command, injecting it via * `intent.nextMilestoneCommand` so the core stays pure. * - * The two section resets use raw regex (with the pre-seam `allow-adhoc-markdown` - * waivers carried from milestone.cts) rather than tokenizeHeadings because the - * `## Operator Next Steps` section is non-canonical (not in STATE_MD_SECTIONS) - * and the existing behavior + its tests pin the exact regex semantics. A future - * collectSection migration (#1372) can swap both to section primitives. - * * Behavior is byte-for-byte with the pre-migration milestone.cts:314-353 block. */ function milestoneCompleteCore(content, intent, deps) { @@ -963,22 +1016,22 @@ function milestoneCompleteCore(content, intent, deps) { } // ## Current Position reset — stop resume/progress flows pointing at closed // execution instructions. - const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 const closedPositionBody = `\nPhase: Milestone ${version} complete\n` + `Plan: —\n` + `Status: Awaiting next milestone\n` + `Last activity: ${today} — Milestone ${version} completed and archived\n\n`; - if (positionPattern.test(body)) { - body = body.replace(positionPattern, (_m, header) => `${header}${closedPositionBody}`); + const positionReset = resetSectionVerbatim(body, (h) => h.level === 2 && /^current\s+position$/i.test(h.text), closedPositionBody); + if (positionReset !== null) { + body = positionReset; } else { body = `${body.trimEnd()}\n\n## Current Position\n${closedPositionBody}`; } updated.push('Current Position'); // ## Operator Next Steps — normalize stale tails that can persist after close. - const operatorPattern = /(##\s*Operator Next Steps\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 - if (operatorPattern.test(body)) { - body = body.replace(operatorPattern, `$1\n- Start the next milestone with ${intent.nextMilestoneCommand}\n\n`); + const operatorReset = resetSectionVerbatim(body, (h) => h.level === 2 && /^operator\s+next\s+steps$/i.test(h.text), `\n- Start the next milestone with ${intent.nextMilestoneCommand}\n\n`); + if (operatorReset !== null) { + body = operatorReset; } else { body = `${body.trimEnd()}\n\n## Operator Next Steps\n\n- Start the next milestone with ${intent.nextMilestoneCommand}\n`; diff --git a/package.json b/package.json index 9d0353524..e9770428c 100644 --- a/package.json +++ b/package.json @@ -99,7 +99,8 @@ "pretest:coverage": "npm run build:lib && npm run lint:skill-deps", "lint": "eslint . --cache --cache-location node_modules/.cache/eslint/", "lint:fix": "eslint . --fix", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/validate-registry.cjs", + "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index af7f01615..5cf81b351 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -99,7 +99,6 @@ "tests/mcp-tool-inheritance.test.cjs :: source-text-is-the-product", "tests/milestone-summary.test.cjs :: source-text-is-the-product", "tests/milestone.test.cjs :: source-text-is-the-product", - "tests/milestone.test.cjs :: structural-regression-guard", "tests/model-catalog-runtime-defaults.test.cjs :: source-text-is-the-product", "tests/next-safety-gates.test.cjs :: source-text-is-the-product", "tests/next-up-clear-order.test.cjs :: source-text-is-the-product", diff --git a/scripts/lint-table-schema-drift.cjs b/scripts/lint-table-schema-drift.cjs new file mode 100644 index 000000000..0ad3749f1 --- /dev/null +++ b/scripts/lint-table-schema-drift.cjs @@ -0,0 +1,157 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Table-schema drift lint (ADR-2143 §3 Generative-Fix-Divergence guard, §7 + * "prohibition with teeth", epic #2143). + * + * `TABLE_SCHEMAS` (`src/markdown-table.cts`, compiled to + * `gsd-core/bin/lib/markdown-table.cjs`) is the single-source registry naming + * every canonical GSD pipe-table's column-header shape. Each registered + * variant's exact `| col | col |` header string MUST appear verbatim in the + * ONE template/workflow file that emits that table — otherwise the writer and + * the registry have silently drifted apart, the exact bug class this ADR + * closes (#2137 — reader hard-codes a shape the writer doesn't emit; #2133 — + * the `quick.md` writer and its shell guard disagree on column count; #2119 — + * dual SECURITY.md writers with conflicting shapes). + * + * Modeled on scripts/lint-phase-id-drift.cjs / scripts/lint-package-identity-drift.cjs: + * a standalone node script (not a node:test), wired into `lint:ci`, exit 0 clean / + * exit 1 + message on drift. + */ + +const fs = require('node:fs'); +const path = require('node:path'); + +// Schema id -> the ONE canonical template/workflow file that must emit every +// variant's header verbatim (ADR-2143 §3). +const SCHEMA_SOURCE_FILES = { + RoadmapProgress: path.join('gsd-core', 'templates', 'roadmap.md'), + RequirementsTraceability: path.join('gsd-core', 'templates', 'requirements.md'), + QuickTasks: path.join('gsd-core', 'workflows', 'quick.md'), + Security: path.join('gsd-core', 'templates', 'SECURITY.md'), +}; + +/** Build the exact `| a | b | c |` header line for one schema variant. */ +function buildHeader(variant) { + return `| ${variant.columns.join(' | ')} |`; +} + +/** Normalize whitespace around pipes so template formatting quirks (extra + * spaces added by an editor, etc.) don't produce a false drift report. */ +function normalize(line) { + return line.replace(/[ \t]*\|[ \t]*/g, '|').trim(); +} + +/** + * Pure: given `schemas` (a `TABLE_SCHEMAS`-shaped object) and a + * `readFile(relPath) -> string|null` accessor, find every variant whose exact + * header does not appear verbatim (pipe-whitespace normalized) in its + * schema's registered source file. Returns + * `[{ schemaId, label, header, file, reason }]`; empty when clean. + */ +function findTableSchemaDrift(schemas, readFile, sourceFiles = SCHEMA_SOURCE_FILES) { + const violations = []; + for (const [schemaId, variants] of Object.entries(schemas)) { + const relPath = sourceFiles[schemaId]; + if (!relPath) { + violations.push({ + schemaId, + label: null, + header: null, + file: null, + reason: 'no canonical source file registered for this schema id in SCHEMA_SOURCE_FILES', + }); + continue; + } + + const content = readFile(relPath); + if (content == null) { + for (const variant of variants) { + violations.push({ + schemaId, + label: variant.label, + header: buildHeader(variant), + file: relPath, + reason: 'source file not found or unreadable', + }); + } + continue; + } + + const normalizedLines = content.split(/\r?\n/).map(normalize); + for (const variant of variants) { + const expected = normalize(buildHeader(variant)); + if (!normalizedLines.includes(expected)) { + violations.push({ + schemaId, + label: variant.label, + header: buildHeader(variant), + file: relPath, + reason: 'header not found verbatim in source file', + }); + } + } + } + return violations; +} + +/** + * Load the built seam and scan the real repo tree. Returns the same shape as + * `findTableSchemaDrift`. If the seam hasn't been built yet (`npm run + * build:lib`), reports a single actionable violation rather than throwing. + */ +function scanRepo(root) { + const seamPath = path.join(root, 'gsd-core', 'bin', 'lib', 'markdown-table.cjs'); + let seam; + try { + seam = require(seamPath); + } catch (e) { + return [{ + schemaId: null, + label: null, + header: null, + file: null, + reason: `cannot load the markdown-table seam at ${path.relative(root, seamPath)} — run 'npm run build:lib' first (${e.message})`, + }]; + } + + const readFile = (relPath) => { + try { + return fs.readFileSync(path.join(root, relPath), 'utf8'); + } catch { + return null; + } + }; + + return findTableSchemaDrift(seam.TABLE_SCHEMAS, readFile); +} + +function main() { + const root = path.join(__dirname, '..'); + const violations = scanRepo(root); + if (violations.length === 0) { + process.stdout.write( + 'ok table-schema-drift: every TABLE_SCHEMAS variant header appears verbatim in its canonical template/workflow\n', + ); + return; + } + process.stderr.write( + 'table-schema-drift: TABLE_SCHEMAS variant(s) whose header is absent from their canonical source file (ADR-2143 §3).\n', + ); + process.stderr.write( + 'Either update the template/workflow to emit the registered header verbatim, or update\n' + + 'TABLE_SCHEMAS in src/markdown-table.cts to match — the two must never drift.\n', + ); + for (const v of violations) { + const id = v.label ? `${v.schemaId}.${v.label}` : (v.schemaId ?? '?'); + const loc = v.file ? ` (${v.file})` : ''; + const expected = v.header ? ` — expected ${JSON.stringify(v.header)}` : ''; + process.stderr.write(` ${id}${loc}: ${v.reason}${expected}\n`); + } + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { findTableSchemaDrift, scanRepo, buildHeader, normalize, SCHEMA_SOURCE_FILES }; diff --git a/src/api-coverage.cts b/src/api-coverage.cts index ab0b43d91..f63b14ee8 100644 --- a/src/api-coverage.cts +++ b/src/api-coverage.cts @@ -47,7 +47,7 @@ * exit 0 = integration detected, 1 = none, 2 = startup error */ -import { stripFencedCode } from './markdown-sectionizer.cjs'; +import { stripFencedCode, extractFencedBlock } from './markdown-sectionizer.cjs'; // ─── Integration-signal vocabulary ──────────────────────────────────────────── @@ -311,13 +311,12 @@ export function parseCoverageMatrix(text: unknown): CoverageParseResult { // (1) fenced ```coverage JSON block takes precedence if present. // Case-insensitive info string (```coverage and ```Coverage are both legal CommonMark). - // allow-adhoc-markdown: extracting a NAMED ```coverage fence (extraction of one tagged block), not stripping all fences — stripFencedCode/extractTaggedBlocks do not cover named-fence extraction. - const fenceMatch = src.match(/```coverage\s*\n([\s\S]*?)\n```/i); - if (fenceMatch && fenceMatch[1]) { + const fenceBody = extractFencedBlock(src, 'coverage'); + if (fenceBody) { out.format = 'json'; let parsed: unknown; try { - parsed = JSON.parse(fenceMatch[1]); + parsed = JSON.parse(fenceBody); } catch { out.errors.push('fenced ```coverage block is not valid JSON'); return out; diff --git a/src/audit.cts b/src/audit.cts index f9a500b9a..015db2f4c 100644 --- a/src/audit.cts +++ b/src/audit.cts @@ -14,6 +14,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { platformReadSync } from './shell-command-projection.cjs'; +import { collectSection } from './markdown-sectionizer.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspace; @@ -165,9 +166,9 @@ function scanDebugSessions(planDir: string): DebugSessionItem[] { // Extract hypothesis from "Current Focus" block if parseable let hypothesis = ''; - const focusMatch = content.match(/##\s*Current Focus[^\n]*\n([\s\S]*?)(?=\n##\s|$)/i); // allow-adhoc-markdown: pre-seam read-only section extract in audit.cts; pending migration #1372 - if (focusMatch) { - const focusText = focusMatch[1].trim().split('\n')[0].trim(); + const focusSection = collectSection(content, (h) => h.level === 2 && h.text.trim().toLowerCase().startsWith('current focus'), { levelBounded: true }); + if (focusSection) { + const focusText = focusSection.body.trim().split('\n')[0].trim(); hypothesis = sanitizeForDisplay(focusText.slice(0, 100)); } @@ -652,9 +653,9 @@ function scanContextQuestions(planDir: string): ContextQuestionItem[] { // Also check for ## Open Questions section in body if (questions.length === 0) { - const oqMatch = content.match(/##\s*Open Questions[^\n]*\n([\s\S]*?)(?=\n##\s|$)/i); // allow-adhoc-markdown: pre-seam read-only section extract in audit.cts; pending migration #1372 - if (oqMatch) { - const oqBody = oqMatch[1].trim(); + const oqSection = collectSection(content, (h) => h.level === 2 && h.text.trim().toLowerCase().startsWith('open questions'), { levelBounded: true }); + if (oqSection) { + const oqBody = oqSection.body.trim(); if (oqBody && oqBody.length > 0 && !/^\s*none\s*$/i.test(oqBody)) { const items = oqBody.split('\n') .map((l: string) => l.trim()) diff --git a/src/commands.cts b/src/commands.cts index 1dd767081..d9a1563da 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1469,9 +1469,6 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, raw: boolean function cmdScaffold(cwd: string, type: string, options: ScaffoldOptions, raw: boolean): void { const { phase, name } = options; const padded = phase ? normalizePhaseName(phase) : '00'; - // #2136 sibling site (deliberately deferred per the issue's scope): scaffold's - // date stays on the raw UTC slice for now; route through realClock.localToday() - // alongside workstream.cts/gsd2-import.cts/template.cts/verify.cts in a follow-up. const today = realClock.localToday(); // Find phase directory diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 0ea2d453c..cca80dadf 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -519,6 +519,27 @@ const FRONTMATTER_SCHEMAS: Record = { verification: { required: ['phase', 'verified', 'status', 'score'] }, }; +/** + * Strip ALL frontmatter blocks from the start of `content`. + * + * Handles CRLF line endings and multiple stacked blocks (corruption + * recovery): greedily strips consecutive `---...---` blocks separated by + * optional whitespace, so a doubled/tripled frontmatter header (e.g. from a + * botched merge) is fully removed, not just the first block. + * + * Canonical home for this primitive (#2143 audit dedup): previously + * duplicated byte-identically in both `state.cts` and `state-transition.cts`. + */ +function stripFrontmatter(content: string): string { + let result = content; + while (true) { + const stripped = result.replace(/^\s*---\r?\n[\s\S]*?\r?\n---\s*/, ''); + if (stripped === result) break; + result = stripped; + } + return result; +} + function cmdFrontmatterGet(cwd: string, filePath: string, field: string | undefined, raw: boolean): void { if (!filePath) { error('file path required'); } // Path traversal guard: reject null bytes @@ -613,6 +634,7 @@ export = { parseFrontmatter: extractFrontmatter, reconstructFrontmatter, spliceFrontmatter, + stripFrontmatter, noOpObjectListSetError, parseMustHavesBlock, FRONTMATTER_SCHEMAS, diff --git a/src/markdown-sectionizer.cts b/src/markdown-sectionizer.cts index 24d26217e..c279ebddd 100644 --- a/src/markdown-sectionizer.cts +++ b/src/markdown-sectionizer.cts @@ -157,6 +157,121 @@ export function stripFencedCode(content: string): StripFencedResult { return { text: kept.join('\n'), unterminatedFence: openFence !== null }; } +// ─── extractFencedBlock ─────────────────────────────────────────────────────── + +/** A fenced code block located by `scanFencedBlocks`: line-index span + info string. */ +interface FencedBlockRecord { + /** Fence delimiter character (`` ` `` or `~`). */ + char: '`' | '~'; + /** Fence delimiter run length (≥3). */ + len: number; + /** Opening line's trailing text (untrimmed) — the CommonMark "info string". */ + infoString: string; + /** 0-based index (into the `lines` array) of the OPENING delimiter line. */ + openLineIdx: number; + /** + * 0-based index of the CLOSING delimiter line, or `-1` when the fence is + * unterminated (EOF reached while still open — mirrors `stripFencedCode`'s + * `unterminatedFence` signal). + */ + closeLineIdx: number; +} + +/** + * Shared low-level fence-scanning engine. Walks `lines` and returns every + * fenced block found, applying the EXACT SAME CommonMark delimiter rules as + * `stripFencedCode` (≥3 backticks/tildes, ≤3-space indent tolerance, a closer + * must be the same delimiter char with run length ≥ the opener and no + * trailing non-whitespace text; a mismatched delimiter char — or a same-char + * run that is too short or carries trailing text — encountered while a fence + * is already open is fence CONTENT, not a new open/close event). This is the + * "engine" `extractFencedBlock` reuses instead of an ad-hoc regex, so a + * different-info-string fence, a fence nested/indented inside another fence, + * and a `~~~` fence are all classified exactly as `stripFencedCode` would. + * + * Tracked duplication (same status as `tokenizeHeadings`'s copy, see its + * comment above): this is a second independent copy of the fence state + * machine, pending a T-tier consolidation. + */ +function scanFencedBlocks(lines: string[]): FencedBlockRecord[] { + const delimRe = /^( {0,3})(`{3,}|~{3,})(.*)$/; + const blocks: FencedBlockRecord[] = []; + let open: { char: '`' | '~'; len: number; infoString: string; openLineIdx: number } | null = null; + + for (let i = 0; i < lines.length; i++) { + const line = lines[i].replace(/\r$/, ''); + const m = delimRe.exec(line); + if (!m) continue; + + const char = m[2][0] as '`' | '~'; + const len = m[2].length; + const trailing = m[3]; + + if (open === null) { + // CommonMark §4.5: backtick fence info string must not contain a backtick. + if (char === '`' && trailing.includes('`')) continue; // not a valid opener — ordinary content + open = { char, len, infoString: trailing.trim(), openLineIdx: i }; + } else if (char === open.char && len >= open.len && /^\s*$/.test(trailing)) { + blocks.push({ + char: open.char, + len: open.len, + infoString: open.infoString, + openLineIdx: open.openLineIdx, + closeLineIdx: i, + }); + open = null; + } + // else: mismatched/insufficient delimiter while a fence is open — content, not a boundary. + } + + if (open !== null) { + blocks.push({ + char: open.char, + len: open.len, + infoString: open.infoString, + openLineIdx: open.openLineIdx, + closeLineIdx: -1, + }); + } + + return blocks; +} + +/** + * Return the INNER text (the lines between the delimiters, joined by `\n`) of + * the FIRST fenced code block whose opening info string — trimmed, + * case-insensitive — equals `infoString`. Returns `null` when no such block + * exists, including when the only matching-name fence is left unterminated + * (EOF inside the fence — there is no well-defined inner span to return, + * matching a non-greedy `\n```-anchored` regex's behaviour of also failing to + * match an unclosed fence). + * + * Built on `scanFencedBlocks`, the same CommonMark fence-tracking engine + * `stripFencedCode` uses — so a fence of a DIFFERENT info string, a fence + * nested/indented inside another fence, and a `~~~` fence are all handled + * exactly as `stripFencedCode` would classify them; this is not a fresh + * ad-hoc regex. + * + * Migrated from `api-coverage.cts`'s bespoke + * `` /```coverage\s*\n([\s\S]*?)\n```/i `` (ADR-1372 tier migration, #2143 audit). + */ +export function extractFencedBlock(content: string, infoString: string): string | null { + if (typeof content !== 'string' || content.length === 0) return null; + if (typeof infoString !== 'string') return null; + + const target = infoString.trim().toLowerCase(); + const lines = content.split('\n'); + const blocks = scanFencedBlocks(lines); + + for (const block of blocks) { + if (block.closeLineIdx === -1) continue; // unterminated — no well-defined inner span + if (block.infoString.trim().toLowerCase() !== target) continue; + return lines.slice(block.openLineIdx + 1, block.closeLineIdx).join('\n'); + } + + return null; +} + // ─── tokenizeHeadings ───────────────────────────────────────────────────────── /** @@ -509,6 +624,146 @@ export function iterateBullets(sectionText: string): BulletItem[] { return items; } +// ─── updateBullet ───────────────────────────────────────────────────────────── + +/** + * Locate the FIRST top-level bullet-opening line — checkbox (`- [ ]`/`- [x]`), + * dash/asterisk/plus (`- `/`* `/`+ `), or numbered (`1. `) — whose bullet text + * satisfies `match(bulletText, rawLine)`, replace that ONE physical line with + * `transform(rawLine)`, and return the resulting full content string. Every + * other byte in `content` — surrounding bullets, indentation, EOL style — is + * left untouched: this is a pure single-line splice, not a document-wide + * regex `.replace()`. + * + * Unlike `iterateBullets` (read-only, no offsets, and not itself fence-aware + * — callers pre-strip fences when that matters), `updateBullet` tracks + * character offsets itself so it can splice the transformed line back into + * the ORIGINAL `content`, and is fence-aware on its own: a bullet-shaped line + * inside a fenced code block (``` / ~~~, same CommonMark delimiter rules as + * `stripFencedCode`) is never offered to `match`/`transform`. (Tracked + * duplication of the fence state machine — same status as `tokenizeHeadings`'s + * copy, see its doc comment — pending a T-tier consolidation.) + * + * `rawLine` (second argument to both `match` and `transform`) is the + * UNMODIFIED physical line exactly as it appears between `\n` separators — so + * on a CRLF document its trailing `\r` is included, matching what a + * hand-rolled `^...[^\n]*`-shaped, `m`-flagged regex applied to the whole + * document would have seen. `bulletText` (first argument to `match`) is the + * bullet's own text with marker/checkbox stripped and any trailing `\r` + * removed — the same extraction `iterateBullets` uses for `BulletItem.text`. + * + * Only the OPENING line of a (possibly multi-line) bullet is ever matched or + * replaced — indented continuation lines are never presented to `match` or + * `transform`. + * + * The gap between the marker and its content tolerates 1 or more spaces — not + * only exactly one — mirroring CommonMark/GFM's 1–4-space allowance for + * list-marker spacing (`checkboxRe`/`numberedRe`/`dashRe`'s own dedicated + * quantifier caps at 4 per GFM; a wider run still recognises the line as a + * bullet opener via the uncapped `dashRe` fallback catching the excess as + * ordinary bullet text). So `- [ ] text` (two spaces), `1. text` (three + * spaces), and even a pathologically wide run are all recognised bullet + * openers, just as the canonical single-space `- [ ] text` / `1. text` are. + * + * Bounded no-op: if no bullet-opening line satisfies `match`, or `transform` + * returns a non-string, `content` is returned completely unchanged. + */ +export function updateBullet( + content: string, + match: (bulletText: string, rawLine: string) => boolean, + transform: (rawLine: string) => string, +): string { + if (typeof content !== 'string' || content.length === 0) return content; + + const lines = content.split('\n'); + + // Marker-to-content gap: CommonMark/GFM tolerates 1–4 spaces between a list + // marker and its content (5+ pushes the content into indented-code-block + // territory) — so `- [ ] Phase 1: Foo` (two spaces) is still a valid + // bullet opener, not just the single-space `- [ ] …` shape. A hand-rolled + // single-space-only regex (e.g. the OLD `mutateMilestonePhase` checkbox + // regex before its `updateBullet` migration, which used `-\s*\[` — no cap, + // but at least 0+) would flip such a line; matching that requires this + // primitive's own bullet-opening recognition to tolerate the same gap, + // otherwise a wider-spaced bullet is silently never offered to `match`. + // F5 (#2245 review, nit): the gap also tolerates a literal TAB (`\t`), not + // only spaces — the OLD `-\s*\[` regex's `\s` class matched a tab too, so a + // `-\t[ ] text` bullet (tab-separated marker) must still be recognised here. + // Checkbox bullet: `- [ ] text` or `- [x] text` + const checkboxRe = /^(\s*)-[ \t]{1,4}\[([xX ])\] (.*)$/; + // Plain dash/asterisk/plus bullet: `- text`, `* text`, `+ text` + const dashRe = /^(\s*)[-*+][ \t]{1,4}(.*)$/; + // Numbered bullet: `1. text` + const numberedRe = /^(\s*)\d+\.[ \t]{1,4}(.*)$/; + + // Fence tracking — same CommonMark delimiter rules as stripFencedCode + // (tracked duplication, see doc comment above). + const delimRe = /^( {0,3})(`{3,}|~{3,})(.*)$/; + let openFence: FenceState | null = null; + + let offset = 0; + for (let i = 0; i < lines.length; i++) { + const rawLine = lines[i]; + const line = rawLine.replace(/\r$/, ''); + + const dm = delimRe.exec(line); + if (dm) { + const char = dm[2][0] as '`' | '~'; + const len = dm[2].length; + const trailing = dm[3]; + if (openFence === null) { + // CommonMark §4.5: backtick fence info string must not contain a backtick. + if (!(char === '`' && trailing.includes('`'))) { + // Valid opener — record fence state; this delimiter line is not a bullet. + openFence = { char, len }; + offset += rawLine.length + 1; + continue; + } + // else: not a valid opener — falls through to the bullet check below. + } else if (char === openFence.char && len >= openFence.len && /^\s*$/.test(trailing)) { + // Closing delimiter — close the fence; this line is not a bullet. + openFence = null; + offset += rawLine.length + 1; + continue; + } else { + // Mismatched/insufficient delimiter while a fence is open — fence content. + offset += rawLine.length + 1; + continue; + } + } + + if (openFence !== null) { + // Inside a fence — never a bullet candidate. + offset += rawLine.length + 1; + continue; + } + + let bulletText: string | null = null; + const cbm = checkboxRe.exec(line); + if (cbm) { + bulletText = cbm[3]; + } else { + const dm2 = dashRe.exec(line); + if (dm2) { + bulletText = dm2[2]; + } else { + const nm = numberedRe.exec(line); + if (nm) bulletText = nm[2]; + } + } + + if (bulletText !== null && match(bulletText, rawLine)) { + const newLine = transform(rawLine); + if (typeof newLine !== 'string') return content; + return content.slice(0, offset) + newLine + content.slice(offset + rawLine.length); + } + + offset += rawLine.length + 1; + } + + return content; +} + // ─── extractTaggedBlocks ────────────────────────────────────────────────────── /** @@ -667,5 +922,94 @@ export function withSection( return replaceSection(content, section, newBody); } +// ─── deleteSection ──────────────────────────────────────────────────────────── + +/** + * Delete an entire section — the matching heading line ITSELF plus its body — + * and return the resulting full content string. + * + * Locates the target heading via the SAME machinery `collectSection` uses + * (`tokenizeHeadings` + `headingPredicate`), then determines the stop boundary + * with the SAME level-bounding rule (`levelBounded` / `stopAtLevel`, see + * `CollectSectionOptions`): the deleted range runs from the target heading's + * OWN start offset up to (but not including) the next heading whose level is + * the same-or-higher (lower level number) than the target's — so a level-3 + * `### Phase N` section deletes through any nested `####` content but STOPS at + * the next `##`/`###` sibling, whatever that heading's text is (unlike a + * hand-rolled regex anchored to a specific heading TEXT pattern, which keeps + * scanning past an unrelated heading and can run away to EOF when no further + * heading of that specific text shape follows — the whole-section-deletion + * data-loss class this primitive retires). + * + * Unlike `collectSection`/`withSection` (which operate on a section's BODY + * only, leaving the heading line untouched), `deleteSection` removes the + * heading line too — the counterpart for "delete section" call sites that + * `withSection` structurally cannot serve. + * + * Collapses at most one resulting blank-line seam: if removing the section + * leaves 2+ blank lines immediately at the splice point (e.g. the original + * document already had a double-blank separator immediately before the + * deleted heading), the seam is normalized down to a single blank line so no + * double-blank gap accumulates where the section used to sit. Content + * elsewhere in the document is never touched. + * + * Returns `content` unchanged when no heading matches `headingPredicate` + * (bounded no-op, mirroring `withSection`'s miss behaviour). + */ +export function deleteSection( + content: string, + headingPredicate: (heading: HeadingToken) => boolean, + opts: CollectSectionOptions = {}, +): string { + if (typeof content !== 'string') return content; + + const { levelBounded = true, stopAtLevel } = opts; + + const headings = tokenizeHeadings(content); + const targetIdx = headings.findIndex(headingPredicate); + if (targetIdx === -1) return content; + + const target = headings[targetIdx]; + const lines = content.split('\n'); + + // Determine the stop line using the SAME level-bounding rule collectSection uses. + let stopLine = lines.length + 1; // 1-based, exclusive (default: EOF+1) + for (let j = targetIdx + 1; j < headings.length; j++) { + const next = headings[j]; + let isStop: boolean; + if (stopAtLevel !== undefined) { + isStop = next.level <= stopAtLevel; + } else { + isStop = levelBounded ? next.level <= target.level : true; + } + if (isStop) { + stopLine = next.line; + break; + } + } + + // Character offsets — same line-offset table collectSection builds. + const lineOffsets: number[] = new Array(lines.length); + let acc = 0; + for (let i = 0; i < lines.length; i++) { + lineOffsets[i] = acc; + acc += lines[i].length + 1; // +1 for the '\n' separator + } + const eofOffset = acc; + + const sectionStart = lineOffsets[target.line - 1]; // start of the target heading LINE itself + const sectionEnd = stopLine <= lines.length ? lineOffsets[stopLine - 1] : eofOffset; + + const before = content.slice(0, sectionStart); + const after = content.slice(sectionEnd); + + // Collapse a resulting blank-line seam to at most one blank line (2 newlines). + // Only the tail of `before` (immediately at the splice point) is touched — + // this never reaches into unrelated content elsewhere in the document. + const collapsedBefore = before.replace(/(?:\r\n|\n){3,}$/, (m) => (m.includes('\r\n') ? '\r\n\r\n' : '\n\n')); + + return collapsedBefore + after; +} + // 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/markdown-table.cts b/src/markdown-table.cts index 2627efaaf..0c76ce5b3 100644 --- a/src/markdown-table.cts +++ b/src/markdown-table.cts @@ -111,15 +111,23 @@ export function matchTableSchema(columns: string[]): { id: string; label: string * reverse of `escapeCell`'s `\`->`\\` then `|`->`\|` order below), so cell * values round-trip exactly — including literal backslashes. */ -function splitTableRow(line: string): string[] { +export function splitTableRow(line: string): string[] { let stripped = line.trim(); if (stripped.startsWith('|')) stripped = stripped.slice(1); if (stripped.endsWith('|')) stripped = stripped.slice(0, -1); return stripped.split(/(? cell.trim().replace(/\\([\\|])/g, '$1')); } -/** True when every delimiter cell matches GFM's `:?-{1,}:?` shape (spaces removed). */ -function isDelimiterRow(cells: string[]): boolean { +/** + * True when every delimiter cell matches GFM's `:?-{1,}:?` shape (spaces + * removed). Exported (alongside `splitTableRow`) so callers that need their + * own ragged-tolerant header/delimiter detection — e.g. state.cts's + * `cmdStateRecordMetric` row-append, which must recognize an existing table + * without requiring every DATA row to also parse cleanly (#2245 Blocker 2) — + * reuse the exact same header/delimiter-shape check `parseMarkdownTable` uses, + * instead of re-deriving it and risking divergence. + */ +export function isDelimiterRow(cells: string[]): boolean { return cells.every((cell) => /^:?-{1,}:?$/.test(cell.replace(/\s+/g, ''))); } @@ -194,6 +202,419 @@ export function parseMarkdownTable(sectionText: string): Result { return { ok: true, value: { columns, rows } }; } +// ─── updateTableCell (ADR-2143 §7 formatting-preserving cell write) ────────── + +/** One line of `text`, with its absolute start offset and original EOL length. */ +interface LineOffset { + line: string; + start: number; +} + +/** + * Split `text` into lines exactly like `.split(/\r?\n/)` (bare `\r` is NOT a + * line break, matching `parseMarkdownTable`), tracking each line's absolute + * start offset in `text` so cell ranges can be computed relative to the + * ORIGINAL string, not the trimmed/relative line. + */ +function splitLinesWithOffsets(text: string): LineOffset[] { + const result: LineOffset[] = []; + let start = 0; + const re = /\r\n|\n/g; + let m: RegExpExecArray | null; + while ((m = re.exec(text)) !== null) { + result.push({ line: text.slice(start, m.index), start }); + start = m.index + m[0].length; + } + result.push({ line: text.slice(start), start }); + return result; +} + +/** + * Cell range within one row: `text.slice(start, end)` is the RAW cell text + * (untrimmed, still `\`-escaped) between its two delimiting `|` characters. + */ +interface CellRange { + start: number; + end: number; +} + +/** + * Split one GFM table row LINE into raw cell ranges, absolute to the original + * `text` the line was sliced from (`lineStart` = that line's start offset). + * Mirrors `splitTableRow`'s trim + strip-leading/trailing-pipe + unescaped-pipe + * split EXACTLY, but returns character ranges instead of trimmed values, so a + * caller can splice a replacement into the original string byte-for-byte. + */ +function splitTableRowRanges(line: string, lineStart: number): CellRange[] { + const leftTrim = /^\s*/.exec(line)![0].length; + const rightTrim = /\s*$/.exec(line)![0].length; + let stripped = line.slice(leftTrim, line.length - rightTrim); + let strippedStart = lineStart + leftTrim; + + if (stripped.startsWith('|')) { + stripped = stripped.slice(1); + strippedStart += 1; + } + if (stripped.endsWith('|')) { + stripped = stripped.slice(0, -1); + } + + const cells: CellRange[] = []; + const re = /(? `\` and `\|` -> `|`. */ +function unescapeCellText(raw: string): string { + return raw.trim().replace(/\\([\\|])/g, '$1'); +} + +/** + * Surgically edit ONE table cell while preserving the table's exact byte + * formatting (ADR-2143 §7). Locates the first GFM table's header + delimiter + * row in `tableText` (own header/delimiter detection — deliberately does NOT + * gate on `parseMarkdownTable(tableText).ok`), finds the first DATA row where + * `match(row, index)` is true, and replaces ONLY that row's `column` cell's + * raw inner text (the span between its two delimiting `|` characters) — every + * other byte of `tableText` (other cells, padding, alignment, EOL style) is + * left BYTE-IDENTICAL. This is deliberately NOT a parse-then-render: a + * render pass would reformat padding/alignment/dates that mutation sites + * (e.g. `status.padEnd(11)`) depend on staying pinned. + * + * Ragged-tolerant by design (#2245 review Fix 2): each data row's + * `{colName:cellText}` record is built ONLY from the columns physically + * present in THAT row — a short row simply omits its trailing column names; + * an over-long row's extra trailing cells are ignored — so `match` is called + * with whatever partial record a ragged row yields. A single sibling row + * whose cell count doesn't match the header must never silently no-op the + * whole write (the prior `parseMarkdownTable(tableText).ok` gate failed the + * ENTIRE table — including an otherwise-well-formed target row — the moment + * ANY other row in the same table was ragged). A row that matches on content + * but is too short to physically contain `column` has no cell to splice + * into, so it cannot be selected; the scan continues past it. + * + * `newValue` is spliced in VERBATIM as the new raw cell span — it is the + * caller's responsibility to supply the fully-formatted text (including any + * leading/trailing padding needed to reproduce the table's existing column + * alignment, and to escape a literal `|` or `\` the value might contain via + * the same convention `splitTableRow`/`escapeCell` use elsewhere in this + * module). When `newValue` is a function, it receives the CURRENT (trimmed, + * unescaped) cell value — the same value that appears in `match`'s `row` + * argument — and must return the full literal replacement text. Returning + * the current value unchanged is a supported no-op-probe pattern for callers + * that need to know whether (and to what current value) a row matched + * without necessarily writing a new value. + * + * Returns `{ok:false, reason}` only for a genuinely absent/malformed table + * (no header line, or no valid delimiter row immediately below it), an + * unknown `column`, or zero rows satisfying `match` while physically + * containing `column` — never for a ragged sibling row. + */ +export function updateTableCell( + tableText: string, + match: (row: Record, index: number) => boolean, + column: string, + newValue: string | ((current: string) => string), +): Result { + const lines = splitLinesWithOffsets(tableText); + + let headerIdx = -1; + for (let i = 0; i < lines.length; i++) { + const trimmed = lines[i].line.trim(); + if (trimmed.startsWith('|') && trimmed.indexOf('|', 1) !== -1) { + headerIdx = i; + break; + } + } + if (headerIdx === -1) { + return { ok: false, reason: 'no table found' }; + } + + const delimiterLine = lines[headerIdx + 1]?.line; + if (delimiterLine === undefined || !delimiterLine.trim().startsWith('|')) { + return { ok: false, reason: 'missing delimiter row' }; + } + + const headerRanges = splitTableRowRanges(lines[headerIdx].line, lines[headerIdx].start); + const columns = headerRanges.map((r) => unescapeCellText(tableText.slice(r.start, r.end))); + + const delimiterCells = splitTableRow(delimiterLine); + if (!isDelimiterRow(delimiterCells)) { + return { ok: false, reason: 'missing delimiter row' }; + } + if (delimiterCells.length !== columns.length) { + return { ok: false, reason: 'delimiter/header column count mismatch' }; + } + + if (!columns.includes(column)) { + return { ok: false, reason: `unknown column: ${column}` }; + } + const targetColIdx = columns.indexOf(column); + + let selectedRange: CellRange | undefined; + let dataRowIndex = 0; + for (let i = headerIdx + 2; i < lines.length; i++) { + const trimmed = lines[i].line.trim(); + if (!trimmed.startsWith('|')) break; + + const cellRanges = splitTableRowRanges(lines[i].line, lines[i].start); + const record: Record = {}; + const presentCount = Math.min(cellRanges.length, columns.length); + for (let c = 0; c < presentCount; c++) { + record[columns[c]] = unescapeCellText(tableText.slice(cellRanges[c].start, cellRanges[c].end)); + } + + if (targetColIdx < cellRanges.length && match(record, dataRowIndex)) { + selectedRange = cellRanges[targetColIdx]; + break; + } + dataRowIndex += 1; + } + + if (!selectedRange) { + return { ok: false, reason: 'no matching row' }; + } + + const currentValue = unescapeCellText(tableText.slice(selectedRange.start, selectedRange.end)); + const replacement = typeof newValue === 'function' ? newValue(currentValue) : newValue; + + // True no-op guard: a function `newValue` that returns `current` UNCHANGED + // (the documented no-op-probe pattern) must leave `tableText` genuinely + // byte-identical, padding included. `current` is already trimmed/unescaped, + // so naively splicing it back in would strip the raw cell's original + // leading/trailing padding — this returns the ORIGINAL text untouched + // instead whenever the callback's answer is "no change". + if (typeof newValue === 'function' && replacement === currentValue) { + return { ok: true, value: tableText }; + } + + return { + ok: true, + value: tableText.slice(0, selectedRange.start) + replacement + tableText.slice(selectedRange.end), + }; +} + +// ─── deleteTableRow (ADR-2143 §7 row-removal sibling of updateTableCell) ───── + +/** + * Surgically delete ONE whole table row while preserving every other byte of + * `tableText` (ADR-2143 §7, row-removal sibling of `updateTableCell`). Locates + * the first GFM table's header + delimiter row in `tableText` using the exact + * same self-contained, ragged-tolerant scan `updateTableCell` uses (own + * header/delimiter detection — does NOT gate on `parseMarkdownTable(tableText).ok`), + * finds the FIRST data row where `match(row, index)` is true, and splices out + * that row's entire LINE — including its trailing newline (`\r\n` or `\n`, + * whichever terminates it) — from `tableText`. Every other byte (header, + * delimiter, other rows, surrounding prose before/after the table, EOL style) + * is left BYTE-IDENTICAL. + * + * Ragged-tolerant by design, mirroring `updateTableCell` (#2245 review Fix 2): + * each data row's `{colName:cellText}` record is built ONLY from the columns + * physically present in THAT row — a sibling row whose cell count doesn't + * match the header must never abort the whole scan; `match` is simply called + * with whatever partial record a ragged row yields. + * + * Returns `{ok:false, reason}` for a genuinely absent/malformed table (no + * header line, or no valid delimiter row immediately below it) or zero rows + * satisfying `match` — never for a ragged sibling row. + */ +export function deleteTableRow( + tableText: string, + match: (row: Record, index: number) => boolean, +): Result { + const lines = splitLinesWithOffsets(tableText); + + let headerIdx = -1; + for (let i = 0; i < lines.length; i++) { + const trimmed = lines[i].line.trim(); + if (trimmed.startsWith('|') && trimmed.indexOf('|', 1) !== -1) { + headerIdx = i; + break; + } + } + if (headerIdx === -1) { + return { ok: false, reason: 'no table found' }; + } + + const delimiterLine = lines[headerIdx + 1]?.line; + if (delimiterLine === undefined || !delimiterLine.trim().startsWith('|')) { + return { ok: false, reason: 'missing delimiter row' }; + } + + const headerRanges = splitTableRowRanges(lines[headerIdx].line, lines[headerIdx].start); + const columns = headerRanges.map((r) => unescapeCellText(tableText.slice(r.start, r.end))); + + const delimiterCells = splitTableRow(delimiterLine); + if (!isDelimiterRow(delimiterCells)) { + return { ok: false, reason: 'missing delimiter row' }; + } + if (delimiterCells.length !== columns.length) { + return { ok: false, reason: 'delimiter/header column count mismatch' }; + } + + let selectedLineIdx = -1; + let dataRowIndex = 0; + for (let i = headerIdx + 2; i < lines.length; i++) { + const trimmed = lines[i].line.trim(); + if (!trimmed.startsWith('|')) break; + + const cellRanges = splitTableRowRanges(lines[i].line, lines[i].start); + const record: Record = {}; + const presentCount = Math.min(cellRanges.length, columns.length); + for (let c = 0; c < presentCount; c++) { + record[columns[c]] = unescapeCellText(tableText.slice(cellRanges[c].start, cellRanges[c].end)); + } + + if (match(record, dataRowIndex)) { + selectedLineIdx = i; + break; + } + dataRowIndex += 1; + } + + if (selectedLineIdx === -1) { + return { ok: false, reason: 'no matching row' }; + } + + // Splice out the whole LINE including its trailing EOL: the next line's + // recorded `start` offset is already positioned right after whatever EOL + // (`\r\n` or `\n`) terminated the selected line (see `splitLinesWithOffsets` + // above) — when the selected row is the LAST line in `tableText` (no + // trailing EOL to preserve), fall back to the end of the string. + let rowStart = lines[selectedLineIdx].start; + let rowEnd: number; + if (selectedLineIdx + 1 < lines.length) { + rowEnd = lines[selectedLineIdx + 1].start; + } else { + // The selected row is the LAST line and has no trailing EOL: deleting from + // its `start` to end-of-string would strand the EOL that terminated the + // PREVIOUS line as a dangling newline. Back `rowStart` up over that + // preceding `\n` (and its `\r`, if any) so the table ends cleanly after the + // new last row. + rowEnd = tableText.length; + if (rowStart > 0 && tableText[rowStart - 1] === '\n') { + rowStart -= 1; + if (rowStart > 0 && tableText[rowStart - 1] === '\r') rowStart -= 1; + } + } + + return { + ok: true, + value: tableText.slice(0, rowStart) + tableText.slice(rowEnd), + }; +} + +// ─── insertTableRow (ADR-2143 §7 row-insertion sibling of updateTableCell) ─── + +/** + * Insert ONE new row into a GFM table while preserving every other byte of + * `tableText` (ADR-2143 §7, row-insertion sibling of `updateTableCell` / + * `deleteTableRow`). Locates the first table's header + delimiter row using + * the exact same self-contained, ragged-tolerant scan the other two use (own + * header/delimiter detection — does NOT gate on `parseMarkdownTable(tableText).ok`), + * builds the new row's cells in the table's ACTUAL header order — each column + * name is passed through `valueFor(column)`; a column for which `valueFor` + * returns `undefined` gets `fallback` (default `'-'`) — and splices it in + * immediately after the table's LAST existing data row (or immediately after + * the delimiter row when the table has zero data rows). + * + * Name-addressed and header-order-agnostic by construction: unlike a + * hardcoded positional literal (`| ${a} | ${b} | - | - |`), this never + * silently no-ops or mis-maps a value onto the wrong column when the header + * is reordered or a superset of the columns `valueFor` knows about (#2245 + * audit sibling finding — the bug this helper replaces). + * + * EOL-preserving: the new row reuses whatever exact EOL bytes (`\r\n` or + * `\n`) already terminate the line it's inserted after, so a CRLF document + * stays CRLF and an LF document stays LF — never guessed or hardcoded. When + * the insertion point is at the very end of `tableText` with no following + * line (the table's last row has no trailing EOL of its own), the existing + * last row is terminated with the header/delimiter boundary's own EOL (so it + * gains a terminator, since it is no longer the last line) and the new row + * becomes the new EOL-less tail — mirroring `tableText`'s own convention of + * not forcing a trailing newline that wasn't already there. + * + * Escaping (F4 #2245 review): unlike `updateTableCell`, whose `newValue` is + * spliced in VERBATIM (caller-must-escape — see its doc comment above), every + * value returned by `valueFor` (and `fallback`) IS escaped internally here via + * `escapeCell` before being joined into the new row, exactly like + * `appendQuickTaskRow` below — a caller-supplied name containing a literal + * `|` or `\` cannot silently split the new row into extra columns. Callers do + * NOT need to pre-escape their values. + * + * Returns `{ok:false, reason}` only for a genuinely absent/malformed table + * (no header line, or no valid delimiter row immediately below it) — never + * for a ragged data row (mirrors `updateTableCell`/`deleteTableRow`). + */ +export function insertTableRow( + tableText: string, + valueFor: (column: string) => string | undefined, + fallback = '-', +): Result { + const lines = splitLinesWithOffsets(tableText); + + let headerIdx = -1; + for (let i = 0; i < lines.length; i++) { + const trimmed = lines[i].line.trim(); + if (trimmed.startsWith('|') && trimmed.indexOf('|', 1) !== -1) { + headerIdx = i; + break; + } + } + if (headerIdx === -1) { + return { ok: false, reason: 'no table found' }; + } + + const delimiterLine = lines[headerIdx + 1]?.line; + if (delimiterLine === undefined || !delimiterLine.trim().startsWith('|')) { + return { ok: false, reason: 'missing delimiter row' }; + } + const delimiterCells = splitTableRow(delimiterLine); + if (!isDelimiterRow(delimiterCells)) { + return { ok: false, reason: 'missing delimiter row' }; + } + + const headerRanges = splitTableRowRanges(lines[headerIdx].line, lines[headerIdx].start); + const columns = headerRanges.map((r) => unescapeCellText(tableText.slice(r.start, r.end))); + + // Header -> delimiter EOL, reused as the fallback terminator for the "insert + // point is at the absolute end of tableText" edge case below. + const headerToDelimiterEol = tableText.slice( + lines[headerIdx].start + lines[headerIdx].line.length, + lines[headerIdx + 1].start, + ) || '\n'; + + let lastLineIdx = headerIdx + 1; // delimiter row, when the table has zero data rows + for (let i = headerIdx + 2; i < lines.length; i++) { + if (!lines[i].line.trim().startsWith('|')) break; + lastLineIdx = i; + } + + const newRow = `| ${columns.map((col) => escapeCell(valueFor(col) ?? fallback)).join(' | ')} |`; + + if (lastLineIdx + 1 < lines.length) { + // A following line exists — insert the new row, reusing the EXACT EOL + // that already terminates the current last table line, so every other + // byte (including everything after the table) stays untouched. + const insertAt = lines[lastLineIdx + 1].start; + const eol = tableText.slice(lines[lastLineIdx].start + lines[lastLineIdx].line.length, insertAt); + return { ok: true, value: tableText.slice(0, insertAt) + newRow + eol + tableText.slice(insertAt) }; + } + + // The table's last row is also the last line of `tableText` (no trailing + // EOL). Terminate it now — it needs one, since it is no longer last — and + // append the new row as the new EOL-less tail. + return { ok: true, value: tableText + headerToDelimiterEol + newRow }; +} + /** * Find the first table in `text` whose header matches `TABLE_SCHEMAS[schemaId]`, * scanning the WHOLE document (not just a named section). Returns `null` when @@ -266,8 +687,18 @@ export function findTableWithColumns(text: string, required: string[]): Markdown * `description`) would otherwise corrupt the table (extra column / a fake * extra row) and get rejected by the now-fail-loud `parseMarkdownTable` as a * ragged row. + * + * Exported (F3/#2245 review) so callers of `updateTableCell` that build a + * replacement value by transforming the CURRENT (already-unescaped) cell + * text — e.g. phase.cts's Progress-ordinal renumber, which decrements the + * leading digit of a `Phase` cell like `3. Parser | Lexer` and splices the + * rest of the cell text back verbatim — can re-escape that value before + * returning it from the `newValue` callback, honoring `updateTableCell`'s + * caller-must-re-escape contract (see its doc comment above) instead of + * spliceing a raw, unescaped `|` back into the table and silently splitting + * the cell. */ -function escapeCell(value: string): string { +export function escapeCell(value: string): string { return String(value) .replace(/\r?\n+/g, ' ') .replace(/\\/g, '\\\\') // escape the escape char FIRST (CodeQL js/incomplete-sanitization) diff --git a/src/milestone.cts b/src/milestone.cts index b09c64105..2cb958da6 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -20,6 +20,7 @@ import { realClock } from './clock.cjs'; import { transitionCore } from './state-transition.cjs'; import { writeSetComplete } from './write-set.cjs'; import type { WriteSet } from './write-set.cjs'; +import { updateTableCell } from './markdown-table.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; @@ -43,6 +44,49 @@ interface MilestoneCompleteOptions { dryRun?: boolean; } +/** + * Scope an `updateTableCell` call to the `## Traceability` (or + * `## Traceability Status`) heading's own section — up to the next H1/H2 + * heading — instead of handing it the WHOLE REQUIREMENTS.md content. + * + * F1 (#2245 review, BLOCKER): `updateTableCell` binds to the FIRST GFM table + * found in whatever text it is given. The shipped requirements template + * (gsd-core/templates/requirements.md) puts an `## Out of Scope` table + * (`| Feature | Reason |`, no `Status` column) BEFORE `## Traceability` — so + * an unscoped whole-file call targets the Out-of-Scope table instead, fails + * with `{ok:false, reason:'unknown column: Status'}`, and the real + * Traceability row is never flipped, while the checkbox surface still flips + * and the command reports success (the #2140 silent-divergence class one + * level deeper). Mirrors phase.cts's `editProgressHeadingSlice` scoping of + * `## Progress` writes to that heading's own slice. + * + * Falls back to running `updateTableCell` against the whole `text` when no + * `## Traceability` heading exists — matching the previous (unscoped) + * behaviour for a REQUIREMENTS.md whose traceability table sits under some + * other heading, or with no heading at all (never worse than before this fix). + */ +function updateTraceabilityCell( + text: string, + match: (row: Record, index: number) => boolean, + column: string, + newValue: string | ((current: string) => string), +): ReturnType { + const headingMatch = text.match(/^##[ \t]+Traceability(?:[ \t]+Status)?\b/im); + if (!headingMatch || headingMatch.index === undefined) { + return updateTableCell(text, match, column, newValue); + } + const headingOffset = headingMatch.index; + const before = text.slice(0, headingOffset); + const fromHeading = text.slice(headingOffset); + const nextHeadingOffset = fromHeading.search(/\n#{1,2}[ \t]/); + const scoped = nextHeadingOffset >= 0 ? fromHeading.slice(0, nextHeadingOffset) : fromHeading; + const after = nextHeadingOffset >= 0 ? fromHeading.slice(nextHeadingOffset) : ''; + + const result = updateTableCell(scoped, match, column, newValue); + if (!result.ok) return result; + return { ok: true, value: before + result.value + after }; +} + function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: boolean): void { if (!reqIdsRaw || reqIdsRaw.length === 0) { error('requirement IDs required. Usage: requirements mark-complete REQ-01,REQ-02 or REQ-01 REQ-02'); @@ -76,10 +120,14 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool // reads the table) still sees Pending while the CLI reported success. const tableUnmatched: string[] = []; - // A traceability table is present if the file has a "| Requirement | … |" - // header. A REQUIREMENTS.md with no such table is legitimate (mid-roadmap), so - // a missing row only counts as drift when a table actually exists. - const hasTable = /^\|\s*Requirement\s*\|/im.test(reqContent); + // A traceability table is present if the file has a requirement-ID column + // header: "Requirement", "Requirement ID", or "REQ-ID" (#2769/#2203) — kept + // in sync with the positional first-cell rowMatch/hasRow below so a + // REQ-ID-headed table (the real-world format) participates in the + // write-set and the #2140 drift check below, not just the "Requirement" + // case. A REQUIREMENTS.md with no such table is legitimate (mid-roadmap), + // so a missing row only counts as drift when a table actually exists. + const hasTable = /^\|\s*(?:Requirement(?:\s*ID)?|REQ[-\s]?ID)\s*\|/im.test(reqContent); // ADR-2143 §6 per-surface write-set, tracked PER requirement ID: a // multi-ID batch must not OR one ID's surface outcome into another's — @@ -104,11 +152,32 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool const checkboxHit = afterCheckbox !== reqContent; if (checkboxHit) reqContent = afterCheckbox; - // Surface 2 — the traceability row: | REQ-ID | Phase N | Pending | → ... Complete | - const tablePattern = new RegExp(`(\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|)\\s*Pending\\s*(\\|)`, 'gi'); - const afterTable = reqContent.replace(tablePattern, '$1 Complete $2'); - const tableHit = afterTable !== reqContent; - if (tableHit) reqContent = afterTable; + // Surface 2 — the traceability row: | | Phase N | Pending | → ... Complete | + // via the markdown-table seam (ADR-2143 §7) — supersedes the prior ordinal + // regex. Match the row by its FIRST cell's value (the requirement-ID column) + // regardless of that column's HEADER name — real tables head it `REQ-ID`, + // others `Requirement` (#2769/#2203); this mirrors the prior regex's first-cell + // `\|\s*\s*\|` anchor. Object.values(row) is in header order so [0] is the + // first column. Case-insensitive (mirrors the prior regex's 'i' flag). + const rowMatch = (row: Record): boolean => + (Object.values(row)[0] ?? '').trim().toLowerCase() === reqId.toLowerCase(); + // Ragged-tolerant (#2245 Blocker 2): drive the write purely off + // updateTableCell's own tolerant row scan — a DIFFERENT requirement's row + // elsewhere in the same table having a mismatched cell count must never + // silently no-op THIS requirement's write. The "only flip Pending -> + // Complete" gate is folded into the newValue callback so one + // updateTableCell call both probes the current value and writes. + let tableHit = false; + const tableUpdate = updateTraceabilityCell(reqContent, rowMatch, 'Status', (current) => { + if (/^pending$/i.test(current.trim())) { + tableHit = true; + return ' Complete '; + } + return current; + }); + if (tableUpdate.ok) { + reqContent = tableUpdate.value; + } // ADR-2143 §6 per-ID write-set entries: this ID's checkbox surface is // always tracked; the traceability surface is tracked only when the file @@ -121,11 +190,21 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool } // Coverage of the traceability surface for this ID (computed after any flip). - // hasRow keys on the ID + a second cell (`| ID | |`) so a bare mention - // of the ID in a non-traceability table does not masquerade as a real row. - const hasRow = new RegExp(`\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|`, 'i').test(reqContent); + // hasRow keys on the ID's FIRST cell (the requirement-ID column, by position — + // see rowMatch above) so a bare mention of the ID in a non-traceability table + // does not masquerade as a real row. + // Ragged-tolerant (#2245 Blocker 2): same reasoning as the write above — a + // sibling row's raggedness must not blind this classification to a row + // that genuinely exists. Probe via a no-op updateTableCell write (its own + // tolerant scan) instead of findTableWithColumns (whole-table parse gate). + let currentStatusCell = ''; + const statusProbe = updateTraceabilityCell(reqContent, rowMatch, 'Status', (current) => { + currentStatusCell = current; + return current; + }); + const hasRow = statusProbe.ok; const doneCheckbox = new RegExp(`-\\s*\\[x\\]\\s*\\*\\*${reqEscaped}\\*\\*`, 'i').test(reqContent); - const doneTable = new RegExp(`\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|\\s*Complete\\s*\\|`, 'i').test(reqContent); + const doneTable = Boolean(hasRow && /^complete$/i.test(currentStatusCell.trim())); if (checkboxHit || tableHit) { updated.push(reqId); @@ -327,12 +406,20 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo totalTasks += xmlTaskMatches.length || mdTaskMatches.length; } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): one unreadable/malformed SUMMARY.md + * must not abort the accomplishments/task-count roll-up for every + * OTHER summary across every OTHER phase — it's simply excluded + * from the milestone's shipped-summary text. */ } } } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): mirrors the phaseDirEntries IIFE a few + * lines below this function (same phasesDir, same "try readdirSync, + * tolerate ENOENT" pattern) — phasesDir may legitimately not exist yet + * (e.g. milestone being force-completed before any phase directories + * were created). Degrades stats to phaseCount/totalPlans/totalTasks=0, + * accomplishments=[] rather than crash `milestone complete`. */ } // #2118: --dry-run preview — compute what WOULD happen without mutating. @@ -451,21 +538,31 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo let phasesArchived = false; // #1871: archive phase dirs by default on milestone complete (opt out via --no-archive-phases). if (options.archivePhases !== false) { + // #2245 audit (was ERROR-HIDING): retryRenameSync moves one phase dir at a + // time — a mid-loop failure (e.g. the Nth rename) used to leave + // `phasesArchived` at its `false` default even though the first N-1 dirs + // had ALREADY been moved to phaseArchiveDir on disk, silently + // under-reporting a real partial archive in the JSON result. archivedCount + // is now computed in a `finally` so it reflects whatever succeeded before + // any failure, instead of being lost with the swallowed exception. + let archivedCount = 0; try { const phaseArchiveDir = path.join(archiveDir, `${version}-phases`); platformEnsureDir(phaseArchiveDir); const phaseEntries = fs.readdirSync(phasesDir, { withFileTypes: true }); const phaseDirNames = phaseEntries.filter((e) => e.isDirectory()).map((e) => e.name); - let archivedCount = 0; for (const dir of phaseDirNames) { if (!isDirInMilestone(dir)) continue; retryRenameSync(path.join(phasesDir, dir), path.join(phaseArchiveDir, dir)); archivedCount++; } - phasesArchived = archivedCount > 0; } catch { - /* intentionally empty */ + /* best-effort: phasesDir may not exist yet, or the archive rename loop + * failed partway — phasesArchived below still reflects whatever + * archivedCount succeeded before the failure. */ + } finally { + phasesArchived = archivedCount > 0; } } diff --git a/src/phase.cts b/src/phase.cts index d453b23e4..cf20e1004 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -55,6 +55,8 @@ import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; import { transitionCore } from './state-transition.cjs'; +import { updateTableCell, deleteTableRow, escapeCell } from './markdown-table.cjs'; +import { deleteSection, updateBullet } from './markdown-sectionizer.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- uat-predicate.cjs is an export= CommonJS module import uatPredicate = require('./uat-predicate.cjs'); const { evaluateUatPassed } = uatPredicate; @@ -86,6 +88,49 @@ const looksLikePlanFile = (f: string): boolean => !PLAN_OUTLINE_RE.test(f) && !PLAN_PRE_BOUNCE_RE.test(f); +/** + * Scope an `updateTableCell` call to the `## Traceability` (or + * `## Traceability Status`) heading's own section — up to the next H1/H2 + * heading — instead of handing it the WHOLE REQUIREMENTS.md content. + * + * F1 (#2245 review, BLOCKER): `updateTableCell` binds to the FIRST GFM table + * found in whatever text it is given. The shipped requirements template + * (gsd-core/templates/requirements.md) puts an `## Out of Scope` table + * (`| Feature | Reason |`, no `Status` column) BEFORE `## Traceability` — so + * an unscoped whole-file call targets the Out-of-Scope table instead, fails + * with `{ok:false, reason:'unknown column: Status'}`, and the real + * Traceability row is never flipped, while the checkbox surface still flips + * and the command reports success (the #2140 silent-divergence class one + * level deeper). Mirrors `editProgressHeadingSlice` below, which scopes + * `## Progress` writes to that heading's own slice for the same reason. + * + * Falls back to running `updateTableCell` against the whole `text` when no + * `## Traceability` heading exists — matching the previous (unscoped) + * behaviour for a REQUIREMENTS.md whose traceability table sits under some + * other heading, or with no heading at all (never worse than before this fix). + */ +function updateTraceabilityCell( + text: string, + match: (row: Record, index: number) => boolean, + column: string, + newValue: string | ((current: string) => string), +): ReturnType { + const headingMatch = text.match(/^##[ \t]+Traceability(?:[ \t]+Status)?\b/im); + if (!headingMatch || headingMatch.index === undefined) { + return updateTableCell(text, match, column, newValue); + } + const headingOffset = headingMatch.index; + const before = text.slice(0, headingOffset); + const fromHeading = text.slice(headingOffset); + const nextHeadingOffset = fromHeading.search(/\n#{1,2}[ \t]/); + const scoped = nextHeadingOffset >= 0 ? fromHeading.slice(0, nextHeadingOffset) : fromHeading; + const after = nextHeadingOffset >= 0 ? fromHeading.slice(nextHeadingOffset) : ''; + + const result = updateTableCell(scoped, match, column, newValue); + if (!result.ok) return result; + return { ok: true, value: before + result.value + after }; +} + function describeNonCanonicalPlans(dirFiles: string[], matchedFiles: string[]): string | null { const matched = new Set(matchedFiles); const offenders = dirFiles.filter((f) => looksLikePlanFile(f) && !matched.has(f)); @@ -922,8 +967,26 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra const normalizedBase = normalizePhaseName(afterPhase); const decimalSet = new Set(); - try { - const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); + // #2245 audit: existsSync-guarded, mirroring cmdPhaseNextDecimal's identical + // scan above — a missing phasesDir (no decimal sub-phases yet) is the + // expected, silent case (empty decimalSet). A readdirSync failure once the + // dir is confirmed to EXIST is a genuine anomaly; swallowing it used to let + // `phase insert` proceed with an incomplete decimalSet and risk writing a + // decimal phase number that collides with an existing on-disk directory + // the scan simply never saw — surfaced loud instead, like the sibling. + if (fs.existsSync(phasesDir)) { + // Initialized (not just declared) so TS's definite-assignment check is + // satisfied without relying on control-flow narrowing through error()'s + // `never` return, which TS does not propagate through a destructured + // module-property function reference — error() still halts the process + // before `dirs` below is ever computed from this placeholder value. + let entries: fs.Dirent[] = []; + try { + entries = fs.readdirSync(phasesDir, { withFileTypes: true }); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + error(`Failed to scan phase directories for existing decimal phases: ${msg}`); + } const dirs = entries.filter((e) => e.isDirectory()).map((e) => e.name); const decimalPattern = new RegExp( `^${OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}${escapeRegex(normalizedBase)}\\.(\\d+)`, @@ -932,8 +995,6 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra const dm = dir.match(decimalPattern); if (dm) decimalSet.add(parseInt(dm[1], 10)); } - } catch { - /* intentionally empty */ } const rmPhasePattern = new RegExp( @@ -1164,6 +1225,39 @@ function decrementRoadmapPaddedPhaseNumber(raw: string, removedInt: number): str return String(num - 1).padStart(raw.length, '0'); } +/** + * Return the RAW text of the `dataRowIndex`-th data row line (0-based, in + * file order — header and delimiter rows excluded) of the FIRST GFM table + * found in `sectionText`, or `null` when the table or that row doesn't exist. + * + * F8 (#2245 review, nit) support helper: addresses a table row by its + * STRUCTURAL position rather than by matching its (possibly non-unique) + * trimmed cell content — see the Progress-ordinal renumber's padding-recovery + * use below for why content-matching is unsafe here (two rows with identical + * trimmed Phase text, or a row whose already-rewritten new value coincides + * with another row's pre-edit text, would otherwise resolve to the wrong line). + */ +function findDataRowLine(sectionText: string, dataRowIndex: number): string | null { + const lines = sectionText.split(/\r?\n/); + let headerIdx = -1; + for (let i = 0; i < lines.length; i++) { + const trimmed = lines[i].trim(); + if (trimmed.startsWith('|') && trimmed.indexOf('|', 1) !== -1) { + headerIdx = i; + break; + } + } + if (headerIdx === -1) return null; + + let seen = -1; + for (let i = headerIdx + 2; i < lines.length; i++) { + if (!lines[i].trim().startsWith('|')) break; + seen += 1; + if (seen === dataRowIndex) return lines[i]; + } + return null; +} + function updateRoadmapAfterPhaseRemoval( roadmapPath: string, targetPhase: string, @@ -1175,21 +1269,69 @@ function updateRoadmapAfterPhaseRemoval( let content = fs.readFileSync(roadmapPath, 'utf-8'); const escaped = escapeRegex(targetPhase); - content = content.replace( - new RegExp( - `\\n?(?#{2,4})\\s*Phase\\s+${escaped}${OPTIONAL_PHASE_TAG_SOURCE}\\s*:[\\s\\S]*?(?=\\n\\k(?!#)\\s+Phase\\s+[^\\n:]+\\s*:|$)`, - 'i', - ), - '', + // SECTION-DELETION (not a section-body edit) — removes the phase's ENTIRE + // detail section INCLUDING its own heading line. Migrated onto deleteSection + // (ADR-2143 §4 / markdown-sectionizer T7): it locates the target heading via + // tokenizeHeadings + this predicate, then splices out the range from that + // heading's own start through the next heading of the SAME-OR-HIGHER level — + // whatever that heading's text is. This fixes a data-loss bug in the prior + // hand-rolled regex, whose lookahead only recognised ANOTHER "Phase N:" + // heading as a stop boundary: removing the LAST phase in a roadmap left no + // such heading to stop at, so the lazy `[\s\S]*?` scan ran to EOF and swept + // away everything after it — including a trailing `## Progress` heading and + // its tracking table. + const phaseHeadingRe = new RegExp( + `^Phase\\s+${escaped}${OPTIONAL_PHASE_TAG_SOURCE}\\s*:`, + 'i', + ); + content = deleteSection( + content, + (h) => h.level >= 2 && h.level <= 4 && phaseHeadingRe.test(h.text), ); content = content.replace( new RegExp(`\\n?-\\s*\\[[ x]\\]\\s*.*Phase\\s+${escaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*`, 'gi'), '', ); - content = content.replace( - new RegExp(`\\n?\\|\\s*${escaped}\\.?\\s[^|]*\\|[^\\n]*`, 'gi'), - '', - ); + // ROW-DELETION (not a cell update) — removes the WHOLE Progress-table row + // for a removed phase via deleteTableRow (ADR-2143 §7 row-removal sibling + // of updateTableCell). Scoped to the `## Progress` section — mirroring + // deriveProgressFromRoadmap's read-side scoping (phase-lifecycle.cts) — + // so a same-numbered row in an earlier, unrelated table (e.g. a + // `| Phase | Requirements | Count |` table preceding `## Progress`, + // #2012) is never touched. Matches the row by its FIRST cell only: for an + // integer removal, a zero-pad-insensitive leading-integer comparison + // (`01.`, `1.`, `1 `, bare `1` all match phase 1; a decimal sub-phase + // cell like `2.5` never matches an integer removal); for a decimal + // removal, the exact decimal token. This replaces the prior regex's + // `\.?\s` requirement, which silently left a COMPACT unpadded row (e.g. + // `|2|0/2|Planned|-|`) undeleted — its closing `|` follows the digit with + // no whitespace to match (#2245 audit) — and which was also unscoped to + // any particular table. + const progressHeadingMatch = content.match(/^##[ \t]+Progress\b/im); + if (progressHeadingMatch && progressHeadingMatch.index !== undefined) { + const headingOffset = progressHeadingMatch.index; + const before = content.slice(0, headingOffset); + const fromHeading = content.slice(headingOffset); + const nextHeadingOffset = fromHeading.search(/\n#{1,2}[ \t]/); + const progressSection = + nextHeadingOffset >= 0 ? fromHeading.slice(0, nextHeadingOffset) : fromHeading; + const rest = nextHeadingOffset >= 0 ? fromHeading.slice(nextHeadingOffset) : ''; + + const matchRemovedProgressRow = (row: Record): boolean => { + const firstCellRaw = (Object.values(row)[0] ?? '').trim(); + if (isDecimal) { + return new RegExp(`^${escaped}\\.?(?:\\s|$)`, 'i').test(firstCellRaw); + } + const leadingMatch = firstCellRaw.match(/^0*(\d+)(\.\d+)?/); + if (!leadingMatch || leadingMatch[2]) return false; + return parseInt(leadingMatch[1], 10) === removedInt; + }; + + const deleteResult = deleteTableRow(progressSection, matchRemovedProgressRow); + if (deleteResult.ok) { + content = before + deleteResult.value + rest; + } + } if (!isDecimal) { // #1729: fold an optional pre-colon ( ) tag into the suffix capture so it @@ -1204,11 +1346,97 @@ function updateRoadmapAfterPhaseRemoval( (_match, prefix: string, num: string, suffix: string) => `${prefix}${decrementRoadmapPhaseNumber(num, removedInt)}${suffix}`, ); - content = content.replace( - /(\|\s*)(\d+)(\.\s)/g, - (_match, prefix: string, num: string, suffix: string) => - `${prefix}${decrementRoadmapPhaseNumber(num, removedInt)}${suffix}`, - ); + // ORDINAL-RENUMBER — CELL EDIT (not row-deletion) — migrated onto + // updateTableCell (ADR-2143 §7, sibling of the deleteTableRow scoping + // directly above). The prior whole-document regex + // `/(\|\s*)(\d+)(\.\s)/g` rewrote ANY `| N. ` cell anywhere in the + // file — including a same-shaped cell in an UNRELATED, earlier table + // (e.g. a `| Phase | Requirements | Count |` table, or a decoy table, + // preceding `## Progress`; #2245-class scoping defect, same family as + // the row-delete fix above). Scoped here to the `## Progress` section + // only, mirroring that same section-slice-then-splice-back pattern. + // + // Loops because updateTableCell only rewrites the FIRST matching row + // per call. `processedOrdinalRows` tracks by row INDEX (stable across + // iterations — this only edits cell content, it never inserts/deletes + // rows) so an already-decremented row's new value — which may still + // numerically exceed `removedInt` — is never re-selected and + // decremented a second time (matching on the row's CURRENT value alone, + // without this guard, would keep re-firing on each pass). + // + // `phaseCellShapeRe` is the exact digit+dot-space shape the old regex + // required: a decimal sub-phase ordinal like `2.5` (no whitespace + // between the dot and the next character) never matches it, so it is + // left untouched — identical decimal-safety to the prior behaviour. + // + // updateTableCell hands the callback the TRIMMED, UNESCAPED cell value + // only, so the row's original leading/trailing alignment padding is + // recovered by a narrow, anchored lookup within that row's OWN raw + // line — addressed by ROW INDEX (`matchedRowIndex`, via + // `findDataRowLine`), not by searching the whole section for content + // matching the trimmed value (F8 #2245 review: two rows with identical + // trimmed Phase text, or a row whose already-rewritten new value + // coincides with another row's pre-edit text, would otherwise resolve + // to the WRONG row's padding — the first/leftmost content match found). + // The lookup searches for `escapeCell(current)` (F3 #2245 review: the + // ESCAPED form, e.g. `Foo \| Bar`) — the raw line always carries the + // escaped form, so searching for the unescaped `current` would + // silently fail to find an escaped-pipe cell's own line — preserving + // every other byte of the row (ADR-2143 §7 byte-parity) while only the + // digits actually change. + const ordinalHeadingMatch = content.match(/^##[ \t]+Progress\b/im); + if (ordinalHeadingMatch && ordinalHeadingMatch.index !== undefined) { + const ordinalHeadingOffset = ordinalHeadingMatch.index; + const ordinalBefore = content.slice(0, ordinalHeadingOffset); + const ordinalFromHeading = content.slice(ordinalHeadingOffset); + const ordinalNextHeadingOffset = ordinalFromHeading.search(/\n#{1,2}[ \t]/); + let ordinalSection = + ordinalNextHeadingOffset >= 0 + ? ordinalFromHeading.slice(0, ordinalNextHeadingOffset) + : ordinalFromHeading; + const ordinalRest = + ordinalNextHeadingOffset >= 0 ? ordinalFromHeading.slice(ordinalNextHeadingOffset) : ''; + + const phaseCellShapeRe = /^(\d+)(\.\s)/; + const processedOrdinalRows = new Set(); + let matchedRowIndex: number | null = null; + + for (;;) { + matchedRowIndex = null; + const cellResult = updateTableCell( + ordinalSection, + (row, index) => { + if (processedOrdinalRows.has(index)) return false; + const m = phaseCellShapeRe.exec(row['Phase'] ?? ''); + if (!m) return false; + const num = parseInt(m[1], 10); + if (!Number.isInteger(num) || num <= removedInt || num === 999) return false; + processedOrdinalRows.add(index); + matchedRowIndex = index; + return true; + }, + 'Phase', + (current) => { + const m = phaseCellShapeRe.exec(current); + if (!m) return current; + const decremented = decrementRoadmapPhaseNumber(m[1], removedInt); + const newContent = `${decremented}${m[2]}${current.slice(m[0].length)}`; + const targetLine = + matchedRowIndex === null ? null : findDataRowLine(ordinalSection, matchedRowIndex); + const padMatch = targetLine + ? new RegExp(`^[ \\t]*\\|(\\s*)${escapeRegex(escapeCell(current))}(\\s*)\\|`).exec(targetLine) + : null; + const leadPad = padMatch ? padMatch[1] : ' '; + const trailPad = padMatch ? padMatch[2] : ' '; + return `${leadPad}${escapeCell(newContent)}${trailPad}`; + }, + ); + if (!cellResult.ok) break; + ordinalSection = cellResult.value; + } + + content = ordinalBefore + ordinalSection + ordinalRest; + } content = content.replace( /(? @@ -1278,8 +1506,18 @@ function cmdPhaseRemove( : renameIntegerPhases(phasesDir, parseInt(normalized, 10)); renamedDirs = renamed.renamedDirs; renamedFiles = renamed.renamedFiles; - } catch { - /* intentionally empty */ + } catch (e) { + // #2245 audit (was ERROR-HIDING): renameDecimalPhases/renameIntegerPhases + // rename subsequent phase directories ON DISK one at a time — a mid-loop + // failure leaves SOME directories already renumbered and others not, with + // no way to recover which (the callee's own renamedDirs/renamedFiles never + // reach this scope when it throws). Silently swallowing this and falling + // through to updateRoadmapAfterPhaseRemoval below used to rewrite + // ROADMAP.md's phase numbers assuming the ENTIRE renumbering succeeded, + // permanently desyncing ROADMAP.md from the actual (partially-renamed) + // on-disk directory names. Surface loud instead of compounding it. + const msg = e instanceof Error ? e.message : String(e); + error(`Failed to renumber phase directories after removing phase ${targetPhase}: ${msg}`); } updateRoadmapAfterPhaseRemoval( @@ -1447,7 +1685,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { if (verStatus === 'gaps_found') warnings.push(`${file}: has unresolved gaps`); } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): this is an ADVISORY pre-scan of UAT/ + * VERIFICATION files for `warnings` in the phase-complete output — the + * actual completion GATE is readVerificationStatus below (a separate + * mechanism). A readdirSync/readFileSync failure here just means fewer + * warnings are surfaced this run, not a blocked or corrupted completion. */ } let nextPhaseNum: string | null = null; @@ -1475,54 +1717,75 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // so completing an already-checked phase (idempotent re-run) checked the // wrong phase's box. Mirrors the tight pattern used by phase-insert // (`]\\s*(?:\\*\\*)?Phase`). - // #2067/#2200: line-anchored (^ + m flag, optional leading indent) so an + // #2067/#2200: line-anchored (^, optional leading indent) so an // 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. + // ADR-2143 §4 note / #2245 audit: 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. Migrated onto the sectionizer's + // `updateBullet` bullet-write seam: the pattern itself is unchanged, + // only the "find the right line, splice it back" plumbing moved off a + // whole-slice `.replace()` onto the seam. Applied per single physical + // line by updateBullet, so the pattern no longer needs the `m` flag + // (it never sees more than one line at a time); see + // planCountBodyPattern below for the sites that were migrated onto + // withPhaseSection instead. + // + // #2245 review Fix 6: this is behaviour-preserving for GSD-GENERATED + // inputs (the only shape ROADMAP.md ever actually has), NOT byte-parity + // across every conceivable input. `updateBullet` is fence-aware — a + // checkbox-shaped line inside a fenced (``` / ~~~) code block is never + // offered to `match`/`transform` — whereas the retired whole-slice + // `.replace()` had no such fence tracking and would have flipped a + // bullet-shaped line inside a fence too. That divergence has no live + // bug because a GSD-authored ROADMAP.md milestone checklist never puts + // its own `- [ ] Phase N: …` entries inside a fenced code block, but it + // is a real (and correct) behavioural difference on pathological input. const checkboxPattern = new RegExp( `^[ \\t]*(-\\s*\\[)[ ](\\]\\s*(?:\\*\\*)?\\s*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`, - 'im', + 'i', ); - const tableRowPattern = new RegExp( - `^(\\|\\s*${phaseEscaped}\\.?\\s[^|]*(?:\\|[^\\n]*))$`, - 'im', - ); - // Scope the Progress-row search to the ## Progress section so the regex - // doesn't bind to an earlier table (e.g. | Phase | Requirements | Count |) - // whose rows also start with the phase number. (#2012) - const updateProgressRow = (fullRow: string): string => { - const cells = fullRow.split('|').slice(1, -1); - const dateShape = /^\d{4}-\d{2}-\d{2}$/; - if (cells.length === 5) { - cells[2] = ` ${summaryCount}/${planCount} `; - cells[3] = ' Complete '; - // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) - const existingDate5 = cells[4].trim(); - cells[4] = dateShape.test(existingDate5) ? cells[4] : ` ${today} `; - } else if (cells.length === 4) { - cells[1] = ` ${summaryCount}/${planCount} `; - cells[2] = ' Complete '; - // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) - const existingDate4 = cells[3].trim(); - cells[3] = dateShape.test(existingDate4) ? cells[3] : ` ${today} `; + // Progress table row: update Plans Complete/Status/Completed columns BY + // COLUMN NAME (handles 4- or 5-column RoadmapProgress tables) via the + // markdown-table seam (ADR-2143 §7) — supersedes the prior ordinal + // cells[]-index regex. Applied inside mutateMilestonePhase below (per + // milestone window), further scoped to the ## Progress heading within + // that window so the row lookup doesn't bind to an earlier table (e.g. + // | Phase | Requirements | Count |) whose rows also start with the + // phase number (#2012). + // #2245 Blocker 4: optional dot must be followed by whitespace-or-end, + // not dot-OR-whitespace-OR-end as alternatives — the prior form let a + // bare "." satisfy the whole lookahead, so completing phase "2" + // over-matched a decimal sub-phase row like "2.5 Extra". Matches "2", + // "2.", "2 Alpha"; rejects "2.5 Extra". + const phaseCellRe = new RegExp(`^${phaseEscaped}\\.?(?:\\s|$)`, 'i'); + const rowMatch = (row: Record): boolean => phaseCellRe.test((row['Phase'] ?? '').trim()); + const dateShape = /^\d{4}-\d{2}-\d{2}$/; + + /** + * Within `text` (already scoped to one milestone window by the + * caller), scope further to the `## Progress` heading section (up to + * the next `#`/`##` heading) when present, run `edit` against just + * that slice, and splice the result back — falling back to the whole + * `text` when no `## Progress` heading exists (mirrors phase- + * lifecycle.cjs's deriveProgressFromRoadmap read-side scoping). + */ + const editProgressHeadingSlice = (text: string, edit: (scoped: string) => string): string => { + const progressMatch = text.match(/^##[ \t]+Progress\b/im); + if (!progressMatch || progressMatch.index === undefined) { + return edit(text); } - return '|' + cells.join('|') + '|'; + const headingOffset = progressMatch.index; + const beforeHeading = text.slice(0, headingOffset); + const fromHeading = text.slice(headingOffset); + const nextHeading = fromHeading.search(/\n#{1,2}[ \t]/); + const scoped = nextHeading >= 0 ? fromHeading.slice(0, nextHeading) : fromHeading; + const after = nextHeading >= 0 ? fromHeading.slice(nextHeading) : ''; + return beforeHeading + edit(scoped) + after; }; - const progressIdx = roadmapContent.indexOf('## Progress'); - if (progressIdx >= 0) { - const beforeProgress = roadmapContent.slice(0, progressIdx); - const progressSection = roadmapContent.slice(progressIdx); - roadmapContent = beforeProgress + progressSection.replace(tableRowPattern, updateProgressRow); - } else { - roadmapContent = roadmapContent.replace(tableRowPattern, updateProgressRow); - } // ADR-2143 §4: the plan-count write is now routed through // withPhaseSection (see mutateMilestonePhase below), which hands this @@ -1543,7 +1806,35 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // milestone, fall back to whole-content mutation (prior behaviour). const mutateMilestonePhase = (slice: string): string => { let s = slice; - s = s.replace(checkboxPattern, `$1x$2 (completed ${today})`); + s = updateBullet( + s, + (_bulletText, rawLine) => checkboxPattern.test(rawLine), + (rawLine) => rawLine.replace(checkboxPattern, `$1x$2 (completed ${today})`), + ); + + s = editProgressHeadingSlice(s, (scoped) => { + let text = scoped; + + const plansResult = updateTableCell(text, rowMatch, 'Plans Complete', ` ${summaryCount}/${planCount} `); + if (plansResult.ok) text = plansResult.value; + + const statusResult = updateTableCell(text, rowMatch, 'Status', ' Complete '); + if (statusResult.ok) text = statusResult.value; + + // Preserve only a valid ISO date (#1161: idempotent; self-heal + // garbage). Ragged-tolerant (#2245 Blocker 2): decide via the + // CURRENT Completed cell inside a single updateTableCell callback + // (its own tolerant row scan) rather than gating on + // findTableWithColumns (which requires the WHOLE table to parse — + // a ragged SIBLING row elsewhere used to silently no-op this + // row's date stamp too). + const completedResult = updateTableCell(text, rowMatch, 'Completed', (current) => + dateShape.test(current.trim()) ? current : ` ${today} `); + if (completedResult.ok) text = completedResult.value; + + return text; + }); + // 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 @@ -1621,13 +1912,26 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'), '$1x$2', ); - reqContent = reqContent.replace( - new RegExp( - `(\\|\\s*${reqEscaped}\\s*\\|[^|]+\\|)\\s*(?:Pending|In Progress)\\s*(\\|)`, - 'gi', - ), - '$1 Complete $2', - ); + + // Traceability row: | | Phase N | Pending|In Progress | -> + // ... Complete | via the markdown-table seam (ADR-2143 §7). Match the + // row by its FIRST cell's value (the requirement-ID column) regardless + // of that column's HEADER name — real tables head it `REQ-ID`, others + // `Requirement` (#2769/#2203); this mirrors the prior regex's first-cell + // `\|\s*\s*\|` anchor, not a by-name lookup. Object.values(row) is in + // header order, so [0] is the first column. Case-insensitive. + const reqRowMatch = (row: Record): boolean => + (Object.values(row)[0] ?? '').trim().toLowerCase() === reqId.toLowerCase(); + // Ragged-tolerant (#2245 Blocker 2): drive the write purely off + // updateTableCell's own tolerant row scan — a DIFFERENT + // requirement's row elsewhere in the same table having a + // mismatched cell count must never silently no-op THIS + // requirement's write. The "only flip Pending/In Progress -> + // Complete" gate is folded into the newValue callback so one + // updateTableCell call both probes and writes. + const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (current) => + /^(?:pending|in progress)$/i.test(current.trim()) ? ' Complete ' : current); + if (reqUpdate.ok) reqContent = reqUpdate.value; } } @@ -1734,7 +2038,13 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): stage 1 of a deliberate 3-stage + * cascading fallback for locating the next phase (disk dirs → roadmap + * headings/checkboxes → lowest-outstanding-checkbox override, #2028 + * below). A disk-scan failure here is indistinguishable from "found + * nothing on disk" and correctly falls through to stage 2, which + * derives the same information independently from ROADMAP.md content + * — not a silent data-loss path. */ } if (isLastPhase && roadmapContent !== null) { @@ -1773,7 +2083,10 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): stage 2 of the next-phase cascade + * (see stage 1's comment above) — a failure here just leaves + * isLastPhase as stage 1 left it; stage 3 (#2028) below runs next + * regardless and provides a further, independent override. */ } } @@ -1817,7 +2130,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { nextPhaseName = lowestOutstanding.name; } } catch { - /* intentionally empty */ + /* best-effort (#2245 audit): stage 3 (#2028) of the next-phase + * cascade — a failure here simply leaves isLastPhase/nextPhaseNum + * as stages 1-2 already determined them; this stage only ever + * overrides toward "not last" when it finds a genuinely lower + * outstanding phase, never the reverse. */ } } diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 7f4a4262c..d1bfc98fc 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -391,7 +391,13 @@ function getMilestoneInfo(cwd: string): MilestoneInfo { const m = stateRaw.match(/^milestone:\s*(.+)/m); if (m) stateVersion = m[1].trim(); } - } catch { /* intentionally empty */ } + } catch { + /* best-effort (#2245 audit): platformReadSync re-throws for a non-ENOENT + * failure (e.g. EACCES) reading STATE.md. Consulting STATE.md's + * `milestone:` field is an OPTIONAL enhancement here — on failure this + * function already falls back to ROADMAP-only heuristics below, the + * same fallback path taken when STATE.md simply doesn't exist. */ + } } if (stateVersion) { @@ -557,7 +563,14 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p if (!/^999\b/.test(bm[1])) milestonePhaseNums.add(bm[1]); } } - } catch { /* intentionally empty */ } + } catch { + /* best-effort (#2245 audit): the real throw source is platformReadSync + * at the top of this try (re-throws for a non-ENOENT read failure). On + * any failure milestonePhaseNums stays empty, which below already + * degrades to the same pass-all filter this function returns when a + * ROADMAP genuinely has zero recognizable phase headings — a safe, + * non-corrupting (over-inclusive, never under-inclusive) degrade. */ + } if (milestonePhaseNums.size === 0) { const passAll = (() => true) as unknown as MilestonePhaseFilter; diff --git a/src/roadmap.cts b/src/roadmap.cts index e8ef6c967..87e48977a 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -22,6 +22,7 @@ const { findPhaseInternal } = phaseLocatorMod; import roadmapParserModule = require('./roadmap-parser.cjs'); const { stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone } = roadmapParserModule; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; +import { updateTableCell } from './markdown-table.cjs'; import { platformWriteSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); @@ -365,24 +366,28 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { let hasContext = false; let hasResearch = false; - try { - const dirMatch = _phaseDirNames.find(d => phaseTokenMatches(d, normalized)); + // DEAD catch removed (#2245 audit): _phaseDirNames.find(...) is a pure + // array lookup on an already-resolved string array, and + // countPhasePlansAndSummaries is itself fully defensive (its own + // readdirSync is self-guarded, and it delegates to scanPhasePlans, which + // never throws) — nothing in this block can throw, so the try/catch could + // never be triggered. + const dirMatch = _phaseDirNames.find(d => phaseTokenMatches(d, normalized)); - if (dirMatch) { - const counts = countPhasePlansAndSummaries(path.join(phasesDir, dirMatch)); - planCount = counts.planCount; - summaryCount = counts.summaryCount; - hasContext = counts.hasContext; - hasResearch = counts.hasResearch; + if (dirMatch) { + const counts = countPhasePlansAndSummaries(path.join(phasesDir, dirMatch)); + planCount = counts.planCount; + summaryCount = counts.summaryCount; + hasContext = counts.hasContext; + hasResearch = counts.hasResearch; - if (summaryCount >= planCount && planCount > 0) diskStatus = 'complete'; - else if (summaryCount > 0) diskStatus = 'partial'; - else if (planCount > 0) diskStatus = 'planned'; - else if (hasResearch) diskStatus = 'researched'; - else if (hasContext) diskStatus = 'discussed'; - else diskStatus = 'empty'; - } - } catch { /* intentionally empty */ } + if (summaryCount >= planCount && planCount > 0) diskStatus = 'complete'; + else if (summaryCount > 0) diskStatus = 'partial'; + else if (planCount > 0) diskStatus = 'planned'; + else if (hasResearch) diskStatus = 'researched'; + else if (hasContext) diskStatus = 'discussed'; + else diskStatus = 'empty'; + } // Check ROADMAP checkbox status. // #3537: padding-tolerant fragment — the heading discovered above may use @@ -466,6 +471,43 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { // ─── cmdRoadmapUpdatePlanProgress ───────────────────────────────────────────── +/** + * Scope a ROADMAP.md content string down to its "Progress table" writable + * slice, run `edit` against just that slice, then splice the result back into + * the original content (ADR-2143 §7). Layered scoping: + * 1. Milestone scope — everything after the LAST `` close tag + * (mirrors `replaceInCurrentMilestone`), so a same-numbered phase row in + * an archived milestone is never touched. + * 2. Heading scope — within that milestone slice, the `## Progress` heading + * section (up to the next `#`/`##` heading) when present, else the whole + * milestone slice (mirrors phase-lifecycle.cjs's `deriveProgressFromRoadmap` + * read-side scoping, #2012 decoy avoidance — a differently-headed table + * sharing the same column names must not be picked up instead). + * `edit` always returns a string and never fails — a no-op edit (table/row not + * found within the scoped slice) simply returns its input unchanged, mirroring + * the prior regex `.replace()`'s no-match-is-a-no-op semantics. + */ +function editProgressTableSlice(content: string, edit: (scoped: string) => string): string { + const lastDetailsClose = content.lastIndexOf(''); + const milestoneOffset = lastDetailsClose === -1 ? 0 : lastDetailsClose + ''.length; + const before = content.slice(0, milestoneOffset); + const milestoneSlice = content.slice(milestoneOffset); + + const progressMatch = milestoneSlice.match(/^##[ \t]+Progress\b/im); + if (!progressMatch || progressMatch.index === undefined) { + return before + edit(milestoneSlice); + } + + const headingOffset = progressMatch.index; + const beforeHeading = milestoneSlice.slice(0, headingOffset); + const fromHeading = milestoneSlice.slice(headingOffset); + const nextHeading = fromHeading.search(/\n#{1,2}[ \t]/); + const scoped = nextHeading >= 0 ? fromHeading.slice(0, nextHeading) : fromHeading; + const after = nextHeading >= 0 ? fromHeading.slice(nextHeading) : ''; + + return before + beforeHeading + edit(scoped) + after; +} + function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | undefined, raw: boolean): void { if (!phaseNum) { error('phase number required for roadmap update-plan-progress'); @@ -509,34 +551,48 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und let roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); const phasePattern = phaseMarkdownRegexSource(phaseNum); - // Progress table row: update Plans/Status/Date columns (handles 4 or 5 column tables) - const tableRowPattern = new RegExp( - `^(\\|\\s*${phasePattern}\\.?\\s[^|]*(?:\\|[^\\n]*))$`, - 'im' - ); - roadmapContent = roadmapContent.replace(tableRowPattern, (fullRow) => { - const cells = fullRow.split('|').slice(1, -1); // drop leading/trailing empty from split - const dateShape = /^\d{4}-\d{2}-\d{2}$/; - if (cells.length === 5) { - // 5-col: Phase | Milestone | Plans | Status | Completed - cells[2] = ` ${summaryCount}/${planCount} `; - cells[3] = ` ${status.padEnd(11)}`; - // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) - const existingDate5 = cells[4].trim(); - cells[4] = isComplete - ? (dateShape.test(existingDate5) ? cells[4] : ` ${today} `) - : ' '; - } else if (cells.length === 4) { - // 4-col: Phase | Plans | Status | Completed - cells[1] = ` ${summaryCount}/${planCount} `; - cells[2] = ` ${status.padEnd(11)}`; - // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) - const existingDate4 = cells[3].trim(); - cells[3] = isComplete - ? (dateShape.test(existingDate4) ? cells[3] : ` ${today} `) - : ' '; - } - return '|' + cells.join('|') + '|'; + // Progress table row: update Plans Complete/Status/Completed columns BY + // COLUMN NAME (handles 4- or 5-column RoadmapProgress tables regardless of + // Milestone-column presence) via the markdown-table seam (ADR-2143 §7) — + // supersedes the prior ordinal cells[]-index regex. Scoped to the current + // milestone's `## Progress` table (editProgressTableSlice above). + // #2245 Blocker 4: optional dot must be followed by whitespace-or-end, not + // dot-OR-whitespace-OR-end as alternatives — the prior form let a bare "." + // satisfy the whole lookahead, so completing phase "2" over-matched a + // decimal sub-phase row like "2.5 Extra". Matches "2", "2.", "2 Alpha"; + // rejects "2.5 Extra" (replicates OLD's `\.?\s` intent on the now-TRIMMED + // cell value, where end-of-string is the trimmed equivalent of "no more + // characters after the optional dot"). + const phaseCellRe = new RegExp(`^${phasePattern}\\.?(?:\\s|$)`, 'i'); + const rowMatch = (row: Record): boolean => phaseCellRe.test((row['Phase'] ?? '').trim()); + const dateShape = /^\d{4}-\d{2}-\d{2}$/; + + roadmapContent = editProgressTableSlice(roadmapContent, (scoped) => { + let text = scoped; + + const plansResult = updateTableCell(text, rowMatch, 'Plans Complete', ` ${summaryCount}/${planCount} `); + if (plansResult.ok) text = plansResult.value; + + const statusResult = updateTableCell(text, rowMatch, 'Status', ` ${status.padEnd(11)}`); + if (statusResult.ok) text = statusResult.value; + + // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage). + // Ragged-tolerant (#2245 Blocker 2): probe the CURRENT Completed cell via + // a no-op updateTableCell write (its own tolerant row scan) rather than + // findTableWithColumns (which requires the WHOLE table to parse — a + // ragged SIBLING row elsewhere used to silently no-op this row's date + // stamp/clear too). The decision (write vs no-op) is folded into the + // newValue callback so a single updateTableCell call both reads and + // writes. + const completedResult = updateTableCell(text, rowMatch, 'Completed', (current) => { + if (isComplete) { + return dateShape.test(current.trim()) ? current : ` ${today} `; + } + return ' '; + }); + if (completedResult.ok) text = completedResult.value; + + return text; }); // Update plan count in phase detail section. diff --git a/src/security.cts b/src/security.cts index fdaeee54c..261bdbe75 100644 --- a/src/security.cts +++ b/src/security.cts @@ -364,7 +364,7 @@ export function sanitizeForDisplay(text: unknown): string { const protocolLeakPatterns = [ /^\s*(?:assistant|user|system)\s+to=[^:\s]+:[^\n]+$/i, - /^\s*<\|(?:assistant|user|system)[^|]*\|>\s*$/i, + /^\s*<\|(?:assistant|user|system)[^|]*\|>\s*$/i, // allow-adhoc-markdown: not a GFM table-cell scan — matches `<|role|>` protocol-leak marker tokens (prompt-injection sanitization), a false-positive on the table-regex pipe+cell-class fingerprint ]; sanitized = sanitized diff --git a/src/smart-entry.cts b/src/smart-entry.cts index 42151bcc8..91a36b6fa 100644 --- a/src/smart-entry.cts +++ b/src/smart-entry.cts @@ -32,6 +32,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { execFileSync } from 'node:child_process'; +import { collectSection } from './markdown-sectionizer.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output } = ioMod; @@ -329,9 +330,9 @@ export function detectSignals(cwd: string, now: () => number = Date.now): SmartE // Blockers list: `- ` items under a `## Blockers` heading. const blockers: string[] = []; - const blockersMatch = body.match(/##\s*Blockers\s*\n([\s\S]*?)(?=\n##|$)/i); // allow-adhoc-markdown: read-only blockers section-collect in smart-entry.cts; mirrors state.cts (#1372), pending collectSection migration - if (blockersMatch) { - const items = blockersMatch[1].match(/^-\s+(.+)$/gm) || []; + const blockersSection = collectSection(body, (h) => h.level === 2 && h.text.trim().toLowerCase() === 'blockers', { levelBounded: true }); + if (blockersSection) { + const items = blockersSection.body.match(/^-\s+(.+)$/gm) || []; for (const item of items) blockers.push(item.replace(/^-\s+/, '').trim()); } diff --git a/src/state-transition.cts b/src/state-transition.cts index 7b0226904..4b82c9156 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -19,11 +19,12 @@ import frontmatter = require('./frontmatter.cjs'); import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback } from './state-document.cjs'; import { KNOWN_TEMPLATE_DEFAULTS } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; +import type { HeadingToken } from './markdown-sectionizer.cjs'; import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { extractFrontmatter, reconstructFrontmatter } = frontmatter; +const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontmatter; const { escapeRegex } = phaseIdMod; // Stop predicate for section-body slicing: a level-2+ heading ends the section. @@ -489,8 +490,8 @@ function beginPhaseCore( } // ## Current Position section mutation (#1104, #1365). - // ADR-1372 T6: tokenizeHeadings + offset splicing (replaceSection adoption - // deferred to a later phase). Mirrors state.cts:2261-2324 byte-for-behaviour. + // `locateCurrentPosition` (fence-aware, tokenizeHeadings-based) locates + // the section; mirrors state.cts:2261-2324 byte-for-behaviour. body = mutateCurrentPositionFirstTime(body, intent, today, updated); } else { // Resume path: only update Last activity timestamp in Current Position @@ -547,6 +548,20 @@ export function sliceCurrentPositionSection(body: string): string | null { * First-time ## Current Position mutation: update Phase / Plan / Status / * Last activity lines. Mirrors state.cts:2261-2324 byte-for-behaviour * (inline regex first, pipe-table fallback via stateReplaceField — #1257). + * + * F2 (#2245 review, MAJOR): a prior revision of this function used + * `collectSection`/`replaceSection` here, whose default `levelBounded: true` + * only stops the section at the next heading of level <= the opener's own + * level (H1/H2 for a `##`-opened section) — an H3+ subsection nested under + * `## Current Position` was NOT a stop boundary and got folded into + * `sectionBody`, so the field regexes below (which run with the `m` flag, + * matching ANY line start in the body) could clobber a same-named line + * inside that subsection (the #2130/#2067/#2080 truncation/clobber class). + * Restored to the fence-aware `locateCurrentPosition` locator (which stops + * at ANY heading level >= 2, `STOP_H2_PLUS` — H2 through H6) + manual splice, + * exactly matching the `mutateCurrentPositionResume`/ + * `mutateCurrentPositionForAdvance` siblings below, both of which use + * `locateCurrentPosition` directly. */ function mutateCurrentPositionFirstTime( body: string, @@ -633,25 +648,6 @@ function mutateCurrentPositionResume( return body.slice(0, span.start) + sectionBody + body.slice(span.end); } -/** - * Strip ALL frontmatter blocks from the start of `content`. - * - * TODO (ADR-1769 follow-up): move to `frontmatter.cjs` or `state-document.cjs` - * so it's a shared primitive. Inlined here in Phase 1 to avoid touching - * `state.cjs` (which is the migration target itself) and to keep the Phase 1 - * diff contained. Body is byte-identical to `state.cts:1653 stripFrontmatter` - * (same CRLF + stacked-block handling). - */ -function stripFrontmatter(content: string): string { - let result = content; - while (true) { - const stripped = result.replace(/^\s*---\r?\n[\s\S]*?\r?\n---\s*/, ''); - if (stripped === result) break; - result = stripped; - } - return result; -} - /** * Update fields within the ## Current Position section for advancePlan. * Mirrors `updateCurrentPositionFields` (state.cts:496) byte-for-behaviour: @@ -1194,6 +1190,69 @@ function milestoneSwitchCore( // milestoneComplete — intent implementation (Phase 5) // ---------------------------------------------------------------------------- +/** + * Replace a section's ENTIRE body with `newBody`, discarding whatever was + * there — the "wholesale reset" write pattern used by milestoneComplete's + * closure write (## Current Position / ## Operator Next Steps). Retires the + * fence-blind raw regex `(##\s*\s*\n)([\s\S]*?)(?=\n##|$)`, which a + * literal `##` inside a fenced code block in the section body could fool into + * stopping early (the #2130/#2067/#2080 truncation class) — heading location + * here goes through `tokenizeHeadings`, which is fence-aware. + * + * Byte-parity note: the retired regex's greedy `\s*` (before its mandatory + * `\n`) swallowed any blank line(s) immediately after the heading into the + * discarded match, and its non-greedy body match always left exactly ONE + * newline unconsumed before the next heading (or EOF), regardless of how many + * blank lines originally separated the section from what followed. Both + * edges are reproduced explicitly (rather than delegated to `collectSection`'s + * `trimEnd()`-based body, which trims a *different* amount and would drift + * the surrounding blank-line count) so `newBody`'s own leading/trailing + * formatting is exactly what appears in the output. + * + * Returns `null` when no heading matches `headingPredicate` (mirrors the + * retired regex's `pattern.test(body)` miss) — callers fall back to their own + * append-a-new-section path. + */ +function resetSectionVerbatim( + content: string, + headingPredicate: (heading: HeadingToken) => boolean, + newBody: string, +): string | null { + const headings = tokenizeHeadings(content); + const idx = headings.findIndex(headingPredicate); + if (idx === -1) return null; + + const target = headings[idx]; + const lines = content.split('\n'); + const headingLineEnd = target.offset + lines[target.line - 1].length + 1; + + // Swallow blank line(s) immediately after the heading (mirrors the retired + // regex's greedy `\s*` folding them into the discarded match). + // + // F7 (#2245 review, nit): recognise a CRLF blank line (`\r\n`), not only a + // bare LF — a lone `content[bodyStart] === '\n'` check never advances past + // a `\r` byte, so on a CRLF STATE.md the blank line right after the + // heading fell into the DISCARDED [bodyStart, bodyEnd) span instead of the + // KEPT prefix, silently dropping one blank line (contradicting this + // function's own byte-parity docstring). + let bodyStart = headingLineEnd; + while (bodyStart < content.length) { + if (content[bodyStart] === '\n') { bodyStart += 1; continue; } + if (content[bodyStart] === '\r' && content[bodyStart + 1] === '\n') { bodyStart += 2; continue; } + break; + } + + // Stop at the next heading of level >= 2 (mirrors the retired regex's + // literal `##` lookahead, which matches any ATX heading two-or-more levels + // deep); leave exactly one newline unconsumed before it, or run to EOF. + let bodyEnd = content.length; + for (let j = idx + 1; j < headings.length; j++) { + if (STOP_H2_PLUS(headings[j].level)) { bodyEnd = headings[j].offset - 1; break; } + } + + return content.slice(0, bodyStart) + newBody + content.slice(bodyEnd); +} + /** * Apply a `milestoneComplete` transition to STATE.md content. * @@ -1208,12 +1267,6 @@ function milestoneSwitchCore( * runtime-specific next-milestone slash command, injecting it via * `intent.nextMilestoneCommand` so the core stays pure. * - * The two section resets use raw regex (with the pre-seam `allow-adhoc-markdown` - * waivers carried from milestone.cts) rather than tokenizeHeadings because the - * `## Operator Next Steps` section is non-canonical (not in STATE_MD_SECTIONS) - * and the existing behavior + its tests pin the exact regex semantics. A future - * collectSection migration (#1372) can swap both to section primitives. - * * Behavior is byte-for-byte with the pre-migration milestone.cts:314-353 block. */ function milestoneCompleteCore( @@ -1272,26 +1325,31 @@ function milestoneCompleteCore( // ## Current Position reset — stop resume/progress flows pointing at closed // execution instructions. - const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 const closedPositionBody = `\nPhase: Milestone ${version} complete\n` + `Plan: —\n` + `Status: Awaiting next milestone\n` + `Last activity: ${today} — Milestone ${version} completed and archived\n\n`; - if (positionPattern.test(body)) { - body = body.replace(positionPattern, (_m, header: string) => `${header}${closedPositionBody}`); + const positionReset = resetSectionVerbatim( + body, + (h) => h.level === 2 && /^current\s+position$/i.test(h.text), + closedPositionBody, + ); + if (positionReset !== null) { + body = positionReset; } else { body = `${body.trimEnd()}\n\n## Current Position\n${closedPositionBody}`; } updated.push('Current Position'); // ## Operator Next Steps — normalize stale tails that can persist after close. - const operatorPattern = /(##\s*Operator Next Steps\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 - if (operatorPattern.test(body)) { - body = body.replace( - operatorPattern, - `$1\n- Start the next milestone with ${intent.nextMilestoneCommand}\n\n`, - ); + const operatorReset = resetSectionVerbatim( + body, + (h) => h.level === 2 && /^operator\s+next\s+steps$/i.test(h.text), + `\n- Start the next milestone with ${intent.nextMilestoneCommand}\n\n`, + ); + if (operatorReset !== null) { + body = operatorReset; } else { body = `${body.trimEnd()}\n\n## Operator Next Steps\n\n- Start the next milestone with ${intent.nextMilestoneCommand}\n`; } diff --git a/src/state.cts b/src/state.cts index f7f3811ac..78db837b7 100644 --- a/src/state.cts +++ b/src/state.cts @@ -27,7 +27,7 @@ const { planningDir, planningPaths } = planningWorkspace; import { realClock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -const { extractFrontmatter, reconstructFrontmatter } = frontmatter; +const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -46,7 +46,9 @@ import { KNOWN_TEMPLATE_DEFAULTS, stateReplaceFieldIfTemplate, } from './state-document.cjs'; -import { tokenizeHeadings } from './markdown-sectionizer.cjs'; +import { tokenizeHeadings, collectSection, replaceSection } from './markdown-sectionizer.cjs'; +import type { HeadingToken } from './markdown-sectionizer.cjs'; +import { parseMarkdownTable, updateTableCell, deleteTableRow, insertTableRow, splitTableRow, isDelimiterRow } from './markdown-table.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -274,8 +276,11 @@ function _stateLockBodyPid(lockPath: string): number | null { // Monotonic sequence for unique stale-steal rename targets (no crypto dependency). let _stateStealSeq = 0; -// Hoisted to module scope — compiled once, not per call (#320). Stateless (/i, used with .match). -const byPhaseTablePattern = /(\|\s*Phase\s*\|\s*Plans\s*\|\s*Total\s*\|\s*Avg\/Plan\s*\|[ \t]*\r?\n\|(?:[- :\t]+\|)+[ \t]*\r?\n)((?:[ \t]*\|[^\n]*\n)*)(?=\r?\n|$)/i; +// The `byPhaseTablePattern` regex hoisted here for #320 (canonical-column- +// ORDER-only By-Phase table match) is retired (#2245 audit): its last caller +// — updatePerformanceMetricsSection's row-INSERT branch — now locates the +// table via findTableStartOffset/insertTableRow, name-addressed and +// header-order-agnostic like the update/sum halves of the same function. // ─── ADR-1372 T6: seam-based section splice helper ─────────────────────────── @@ -540,33 +545,133 @@ function cmdStateRecordMetric(cwd: string, options: StateRecordMetricOptions, ra let _recorded = false; let created = false; readModifyWriteStateMd(statePath, (content) => { - // Find Performance Metrics section and its table - const metricsPattern = /(##\s*Performance Metrics[\s\S]*?\n\|[^\n]+\n\|[-|\s]+\n)([\s\S]*?)(?=\n##|\n$|$)/i; // allow-adhoc-markdown: metrics-table write-path section-collect in state.cts; pending collectSection migration #1372 - const metricsMatch = content.match(metricsPattern); - const newRow = `| Phase ${phase} P${plan} | ${duration} | ${tasks || '-'} tasks | ${files || '-'} files |`; - if (metricsMatch) { - let tableBody = metricsMatch[2].trimEnd(); + // Find the "## Performance Metrics" section via the markdown-sectionizer + // seam (ADR-2143 §7) — supersedes the prior hand-rolled section+table + // regex. + const metricsSection = collectSection(content, (h) => /^performance metrics$/i.test(h.text.trim())); - if (tableBody.trim() === '' || tableBody.includes('None yet')) { - tableBody = newRow; - } else { - tableBody = tableBody + '\n' + newRow; + const eol = metricsSection && /\r\n/.test(metricsSection.body) ? '\r\n' : '\n'; + const lines = metricsSection ? metricsSection.body.split(/\r?\n/) : []; + + // Locate THIS command's OWN metrics table by its HEADER shape, using the + // exact same splitTableRow/isDelimiterRow header/delimiter-shape checks + // `parseMarkdownTable` uses. A live "## Performance Metrics" section + // (gsd-core/templates/state.md:39-56) also carries the "By Phase" + // velocity table (`| Phase | Plans | Total | Avg/Plan |`) — the prior + // "first table in the section" targeting spliced every per-plan row into + // THAT table instead, polluting it on EVERY plan completion + // (execute-plan.md:414 calls record-metric per-plan) (#2245/#2143). + // Matching the header cells to this command's own canonical + // `Plan | Duration | Tasks | Files` shape (case-insensitive/trimmed) + // finds the right table regardless of what else shares the section, and + // deliberately does NOT require `parseMarkdownTable(...).ok` (which + // additionally requires every DATA row's cell count to match the + // header) — a single ragged sibling row (a hand-edited stray/extra pipe) + // must not blind this scan (#2245 Blocker 2 parity with the other + // Phase-4 ragged-tolerance fixes: updateTableCell / findTableStartOffset). + const METRICS_HEADER = ['plan', 'duration', 'tasks', 'files']; + let headerIdx = -1; + for (let i = 0; i < lines.length - 1; i++) { + const trimmed = lines[i].trim(); + if (!trimmed.startsWith('|') || trimmed.indexOf('|', 1) === -1) continue; + const delimiterLine = lines[i + 1]; + if (delimiterLine === undefined || !delimiterLine.trim().startsWith('|')) continue; + const headerCells = splitTableRow(lines[i]); + const delimiterCells = splitTableRow(delimiterLine); + if (!isDelimiterRow(delimiterCells) || delimiterCells.length !== headerCells.length) continue; + const normalized = headerCells.map((cell) => cell.trim().toLowerCase()); + const isMetricsHeader = normalized.length === METRICS_HEADER.length + && normalized.every((cell, idx) => cell === METRICS_HEADER[idx]); + if (isMetricsHeader) { headerIdx = i; break; } + } + const hasTable = headerIdx !== -1; + + if (metricsSection && hasTable) { + const delimiterIdx = headerIdx + 1; + const prefixLines = lines.slice(0, delimiterIdx + 1); + + // Ragged-tolerant row scan: every consecutive `|`-prefixed line + // following the delimiter counts as an existing row REGARDLESS of its + // cell count matching the header — a ragged sibling row must never + // blind this scan to the table's true last row (unlike + // `parsedTable.value.rows.length`, which this replaces). Anchored to + // the METRICS table's OWN header/delimiter (`headerIdx` above), never + // the section's first table (#2245/#2143). + let lastRowIdx = delimiterIdx; + for (let i = delimiterIdx + 1; i < lines.length; i++) { + if (!lines[i].trim().startsWith('|')) break; + lastRowIdx = i; } + const rowCount = lastRowIdx - delimiterIdx; _recorded = true; - return content.replace(metricsPattern, (_match, header: string) => `${header}${tableBody}\n`); + + let newBody: string; + if (rowCount > 0) { + // Splice the new row immediately after the table's LAST existing data + // row — every other byte of the section, INCLUDING any trailing prose + // that follows the table (e.g. the default template's "**Recent + // Trend:**" subsection + "*Updated after each plan completion*" + // footer), is preserved verbatim. The prior implementation truncated + // the section body to header+delimiter+rows+newRow, silently dropping + // everything that followed the table on a live STATE.md (#2245 + // Blocker 1 — a per-plan path, run after every plan execution). + // `lastRowIdx` (computed above by the ragged-tolerant scan) already + // equals `delimiterIdx + rowCount` by construction. + const before = lines.slice(0, lastRowIdx + 1); + const after = lines.slice(lastRowIdx + 1); + newBody = [...before, newRow, ...after].join(eol); + } else { + // No existing data rows (e.g. a "None yet" placeholder line instead of + // a real row) — replace the placeholder/table-body remainder with the + // new row, matching the section's prior (verified) collapse-to- + // first-row behavior for an otherwise-empty table. + // No trailing eol here: replaceSection's `content.slice(bodyEnd)` + // already supplies the newline(s) that followed the (trimEnd()-ed) + // section body. + newBody = prefixLines.join(eol) + eol + newRow; + } + + return replaceSection(content, metricsSection, newBody); } - // Section absent — DWIM: auto-create canonical ## Performance Metrics scaffold, - // then append the row. Matches state begin-phase / advance-plan DWIM behavior. + if (metricsSection) { + // Section EXISTS but carries no metrics table of its own — e.g. a live + // STATE.md whose "## Performance Metrics" section holds only the + // By-Phase velocity table (gsd-core/templates/state.md:48). Self-heal + // by appending a fresh Per-Plan Metrics table to the END of the + // section body — every existing byte (By-Phase table, Recent Trend, + // footer) is preserved verbatim, and no second "## Performance + // Metrics" heading is introduced. The section already existed, so + // `created` stays false (#2245/#2143). + _recorded = true; + const newBody = metricsSection.body + + eol + '**Per-Plan Metrics:**' + + eol + eol + + '| Plan | Duration | Tasks | Files |' + + eol + + '|------|----------|-------|-------|' + + eol + + newRow + + eol; + return replaceSection(content, metricsSection, newBody); + } + + // Section absent (or malformed) — DWIM: auto-create canonical + // ## Performance Metrics scaffold, then append the row. Matches state + // begin-phase / advance-plan DWIM behavior. Header corrected to this + // command's own canonical shape (`Plan | Duration | Tasks | Files`) — + // the prior scaffold's `| Phase | Plan | Duration | Notes |` header + // matched neither the appended row's shape nor the canonical table + // above (#2245/#2143). const scaffold = [ '', '## Performance Metrics', '', - '| Phase | Plan | Duration | Notes |', - '|-------|------|----------|-------|', + '| Plan | Duration | Tasks | Files |', + '|------|----------|-------|-------|', newRow, '', ].join('\n'); @@ -1120,15 +1225,22 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, * Match the session section body from a STATE.md body. #1101: recognise the * bootstrap `## Session Continuity` heading but PREFER the normalized `## Session` * block when both exist (legacy duplicate files), so the reader agrees with the - * writer (which updates `## Session` first). `(?:^|\n)` line-anchors (kept out of - * `/m` so `$` stays end-of-string for the `(?=\n##|$)` section boundary), which - * excludes an h3 `### Session Continuity`; the trailing-` Archive` boundary still - * excludes `## Session Continuity Archive` (preserving the #2444 scoping). - * Returns the match whose group 1 is the section body, or null. + * writer (which updates `## Session` first). Level-2-exact heading match + * (excludes an h3 `### Session Continuity`); the exact `'session continuity'` + * text match still excludes `## Session Continuity Archive` (preserving the + * #2444 scoping). Migrated onto the `collectSection` seam (#2143 audit, + * epic #2143): CRLF-safe — the prior hand-rolled `[ \t]*\n` regex silently + * failed to match a CRLF `## Session\r\n` heading line (the `\r` broke the + * `[ \t]*\n` boundary); `tokenizeHeadings` strips the trailing `\r` before + * heading-text extraction, so this now matches CRLF headings too. + * Returns the section body, or null. */ -function matchSessionSection(body: string): RegExpMatchArray | null { - return body.match(/(?:^|\n)##[ \t]*Session[ \t]*\n([\s\S]*?)(?=\n##|$)/i) // allow-adhoc-markdown: read-only session-section extract in state.cts; pending collectSection migration #1372 - || body.match(/(?:^|\n)##[ \t]*Session Continuity[ \t]*\n([\s\S]*?)(?=\n##|$)/i); // allow-adhoc-markdown: read-only session-continuity section extract in state.cts; pending collectSection migration #1372 +function matchSessionSection(body: string): string | null { + const isSession = (h: HeadingToken): boolean => h.level === 2 && h.text.trim().toLowerCase() === 'session'; + const isSessionContinuity = (h: HeadingToken): boolean => h.level === 2 && h.text.trim().toLowerCase() === 'session continuity'; + const section = collectSection(body, isSession, { levelBounded: true }) + ?? collectSection(body, isSessionContinuity, { levelBounded: true }); + return section ? section.body : null; } function parseProsePhaseField(value: string | null): { phase: string | null; name: string | null } { @@ -1200,14 +1312,15 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { const totalPlansInPhase = totalPlansRaw ? parseInt(totalPlansRaw, 10) : null; const progressPercent = progressRaw ? parseInt(progressRaw.replace('%', ''), 10) : null; - // Extract decisions table + // Extract decisions table — via the markdown-sectionizer/markdown-table + // seams (ADR-2143 §7), cells addressed by column NAME rather than a + // hand-rolled section+table regex. const decisions: Array<{ phase: string; summary: string; rationale: string }> = []; - const decisionsMatch = body.match(/##\s*Decisions Made[\s\S]*?\n\|[^\n]+\n\|[-|\s]+\n([\s\S]*?)(?=\n##|\n$|$)/i); // allow-adhoc-markdown: read-only decisions-table section-collect in state.cts; pending collectSection migration #1372 - if (decisionsMatch) { - const tableBody = decisionsMatch[1]; - const rows = tableBody.trim().split('\n').filter(r => r.includes('|')); - for (const row of rows) { - const cells = row.split('|').map(c => c.trim()).filter(Boolean); + const decisionsSection = collectSection(body, (h) => /^decisions made$/i.test(h.text.trim())); + const decisionsTable = decisionsSection ? parseMarkdownTable(decisionsSection.body) : null; + if (decisionsTable && decisionsTable.ok) { + for (const row of decisionsTable.value.rows) { + const cells = decisionsTable.value.columns.map((c) => (row[c] ?? '').trim()).filter(Boolean); if (cells.length >= 3) { decisions.push({ phase: cells[0], @@ -1220,10 +1333,9 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { // Extract blockers list const blockers: string[] = []; - const blockersMatch = body.match(/##\s*Blockers\s*\n([\s\S]*?)(?=\n##|$)/i); // allow-adhoc-markdown: read-only blockers section-collect in state.cts; pending collectSection migration #1372 - if (blockersMatch) { - const blockersSection = blockersMatch[1]; - const items = blockersSection.match(/^-\s+(.+)$/gm) || []; + const blockersSection = collectSection(body, (h) => h.level === 2 && h.text.trim().toLowerCase() === 'blockers', { levelBounded: true }); + if (blockersSection) { + const items = blockersSection.body.match(/^-\s+(.+)$/gm) || []; for (const item of items) { blockers.push(item.replace(/^-\s+/, '').trim()); } @@ -1239,8 +1351,8 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { // #1101: prefer the canonical `## Session` block, falling back to the bootstrap // `## Session Continuity` heading. See matchSessionSection for the anchoring. const sessionMatch = matchSessionSection(body); - if (sessionMatch) { - const sessionSection = sessionMatch[1]; + if (sessionMatch !== null) { + const sessionSection = sessionMatch; // Accept both `**Last Date:**` (canonical template form) and `**Last session:**` // (the form written by the DWIM auto-create / normalize path added for #944). const lastDateMatch = sessionSection.match(/\*\*Last Date:\*\*\s*(.+)/i) @@ -1360,18 +1472,20 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re // #1101: prefer the canonical `## Session` block, falling back to the bootstrap // `## Session Continuity` heading. See matchSessionSection for the anchoring. const sessionSectionMatch = matchSessionSection(bodyContent); - const sessionBodyScope = sessionSectionMatch ? sessionSectionMatch[1] : bodyContent; + const sessionBodyScope = sessionSectionMatch ?? bodyContent; const stoppedAt = stateExtractField(sessionBodyScope, 'Stopped At') || stateExtractField(sessionBodyScope, 'Stopped at'); const pausedAt = stateExtractField(bodyContent, 'Paused At'); let milestone: string | null = null; let milestoneName: string | null = null; if (cwd) { - try { - const info = getMilestoneInfo(cwd); - milestone = info.version; - milestoneName = info.name; - } catch { /* intentionally empty */ } + // DEAD catch removed (#2245 audit): getMilestoneInfo has its own outer + // try/catch (roadmap-parser.cts) that already swallows every internal + // failure and always returns a MilestoneInfo — it never throws, so this + // wrapper could never be triggered. + const info = getMilestoneInfo(cwd); + milestone = info.version; + milestoneName = info.name; } let totalPhases: number | null = totalPhasesRaw ? parseInt(totalPhasesRaw, 10) : null; @@ -1508,6 +1622,13 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re completedPlans = cached.completedPlans; milestoneUnbounded = cached.milestoneBounded === false; } + /* best-effort (#2245 audit): this is a READ path building STATE.md's + * display frontmatter. The real throw source is fs.readdirSync(phasesDir) + * a few lines up — an inaccessible/racily-removed phases dir must not + * crash `state show`; on failure this simply keeps whatever + * frontmatter-derived totals/completedPhases/etc. were already set + * above, a graceful degrade rather than a corrupted write (nothing is + * persisted from this block). */ } catch { /* intentionally empty */ } } @@ -1552,20 +1673,6 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re return fm; } -function stripFrontmatter(content: string): string { - // Strip ALL frontmatter blocks at the start of the file. - // Handles CRLF line endings and multiple stacked blocks (corruption recovery). - // Greedy: keeps stripping ---...--- blocks separated by optional whitespace. - let result = content; - - while (true) { - const stripped = result.replace(/^\s*---\r?\n[\s\S]*?\r?\n---\s*/, ''); - if (stripped === result) break; - result = stripped; - } - return result; -} - function syncStateFrontmatter(content: string, cwd: string | undefined): string { // Read existing frontmatter BEFORE stripping — it may contain values // that the body no longer has (e.g., Status field removed by an agent). @@ -1954,7 +2061,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // A stale "Stopped at:" in a non-Session section (e.g. Session Continuity // Archive prose) must not interfere with the delta comparison. const preSessionMatch = matchSessionSection(preBody); - const preSessionScope = preSessionMatch ? preSessionMatch[1] : preBody; + const preSessionScope = preSessionMatch ?? preBody; const preBodyStoppedAt = stateExtractField(preSessionScope, 'Stopped At') || stateExtractField(preSessionScope, 'Stopped at'); // ADR-1769 Phase 6 / #1743 / #1695: snapshot the body source for the curated @@ -1988,7 +2095,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, // consistent with the pre-transform snapshot above and buildStateFrontmatter. const postSessionMatch = matchSessionSection(postBody); - const postSessionScope = postSessionMatch ? postSessionMatch[1] : postBody; + const postSessionScope = postSessionMatch ?? postBody; const postBodyStoppedAt = stateExtractField(postSessionScope, 'Stopped At') || stateExtractField(postSessionScope, 'Stopped at'); // ADR-1769 Phase 6 / #1695: post-transform body Phase source for the // current_phase_name delta comparison. @@ -2157,6 +2264,38 @@ function cmdSignalResume(cwd: string, raw: boolean): void { // ─── Gate Functions (STATE.md consistency enforcement) ──────────────────────── +/** + * Find the character offset where the FIRST GFM table whose header is a + * superset of `required` column names begins (order-independent; extra + * columns tolerated) — the position-aware counterpart to markdown-table's + * `findTableWithColumns`, used to scope `updateTableCell` (which always + * operates on "the first table in its input") to the RIGHT table when an + * unrelated earlier table (that doesn't itself name every required column) + * may precede it in the same document. Returns `null` when no such table is + * found. Never trips the table-regex fingerprint (no `[^|]` cell-capture + * class) and never throws. + * + * Ragged-tolerant (#2245 Blocker 2): accepts the offset the moment a HEADER + * line names every required column — it deliberately does NOT additionally + * require `parseMarkdownTable(text.slice(m.index)).ok`, which validates every + * DATA row's cell count. A ragged sibling row anywhere in the table used to + * make that whole-table parse fail, so the offset came back `null` and the + * caller's `updateTableCell` calls (which scope to this offset) never even + * ran against an otherwise-perfectly-findable row. + */ +function findTableStartOffset(text: string, required: string[]): number | null { + const lineRe = /^[ \t]*\|.*\|[ \t]*$/gm; + let m: RegExpExecArray | null; + while ((m = lineRe.exec(text)) !== null) { + const trimmed = m[0].trim(); + const cols = trimmed.replace(/^\|/, '').replace(/\|$/, '').split(/(? c.trim()); + if (required.every((rq) => cols.includes(rq))) { + return m.index; + } + } + return null; +} + /** * Update the ## Performance Metrics section in STATE.md content. * Increments Velocity totals and upserts a By Phase table row. @@ -2168,46 +2307,125 @@ function updatePerformanceMetricsSection(content: string, cwd: string, phaseNum: // the same phase again upserts the same row, so the column sum is stable. The previous // blind-add (prevTotal + summaryCount) re-read the cumulative total each call and // double-counted on every re-run. (#1582) - const byPhaseMatch = content.match(byPhaseTablePattern); - if (byPhaseMatch) { - let tableBody = byPhaseMatch[2].trim(); + // + // Located by column NAME via the markdown-table seam (ADR-2143 §7) — + // supersedes the prior module-level byPhaseTablePattern regex for the + // existence/lookup half of this logic. + const byPhaseCols = ['Phase', 'Plans', 'Total', 'Avg/Plan']; + // Ragged-tolerant (#2245 Blocker 2): scope to the table's start offset + // (findTableStartOffset — itself now ragged-tolerant, see above) rather + // than gating existence/lookup on findTableWithColumns, which requires the + // WHOLE table to parse — a ragged row for a DIFFERENT phase used to + // silently no-op every phase's upsert. + const tableStart = findTableStartOffset(content, byPhaseCols); + if (tableStart !== null) { // Match the existing row for this phase, tolerating leading-zero padding in either // direction (#1659): canonicalize a numeric phase to its integer form so a seeded // "| 05 |" row is upserted (not duplicated) by `phase complete 5`, and vice-versa. const phaseNumStr = String(phaseNum); const canonCell = /^\d+$/.test(phaseNumStr) ? `0*${Number(phaseNumStr)}` : escapeRegex(phaseNumStr); - const phaseRowPattern = new RegExp(`^\\|\\s*${canonCell}\\s*\\|.*$`, 'm'); - const newRow = `| ${phaseNum} | ${summaryCount} | - | - |`; + const phaseCellRe = new RegExp(`^${canonCell}$`, 'i'); + const rowMatch = (row: Record): boolean => phaseCellRe.test((row['Phase'] ?? '').trim()); - if (phaseRowPattern.test(tableBody)) { - // Update existing row - tableBody = tableBody.replace(phaseRowPattern, newRow); + const before = content.slice(0, tableStart); + let tableText = content.slice(tableStart); + + // Ragged-tolerant existence probe: a no-op updateTableCell write on the + // identifying "Phase" column (its own tolerant row scan) decides whether + // this phase's row already exists, without requiring every OTHER row in + // the table to also parse cleanly. + let rowExists = false; + const existsProbe = updateTableCell(tableText, rowMatch, 'Phase', (current) => { + rowExists = true; + return current; + }); + void existsProbe; + + if (rowExists) { + // Update existing row — one updateTableCell call per column (Phase + // itself may also change shape, e.g. "05" -> "5" per #1659). + const phaseResult = updateTableCell(tableText, rowMatch, 'Phase', ` ${phaseNum} `); + if (phaseResult.ok) tableText = phaseResult.value; + const plansResult = updateTableCell(tableText, rowMatch, 'Plans', ` ${summaryCount} `); + if (plansResult.ok) tableText = plansResult.value; + const totalResult = updateTableCell(tableText, rowMatch, 'Total', ' - '); + if (totalResult.ok) tableText = totalResult.value; + const avgResult = updateTableCell(tableText, rowMatch, 'Avg/Plan', ' - '); + if (avgResult.ok) tableText = avgResult.value; + + content = before + tableText; } else { - // Remove placeholder row and add new row - tableBody = tableBody.replace(/^\|\s*-\s*\|\s*-\s*\|\s*-\s*\|\s*-\s*\|$/m, '').trim(); - tableBody = tableBody ? tableBody + '\n' + newRow : newRow; - } + // Row doesn't exist — INSERT a new row. Row insertion (unlike a cell + // update) is outside updateTableCell's scope (ADR-2143 §7 Phase 4); + // `insertTableRow` (markdown-table.cjs) is its name-addressed, + // header-order-agnostic sibling (#2245 audit: this used to locate the + // table via `byPhaseTablePattern`, a canonical-column-ORDER-only regex, + // and build the row as a hardcoded positional literal — so a reordered/ + // superset By-Phase header, already tolerated above by + // findTableStartOffset and read by-NAME in the update/sum halves, + // silently inserted NOTHING). + // + // Drop a lone all-placeholder row first (e.g. the freshly-scaffolded + // "| - | - | - | - |" seed row) — same convention the prior + // canonical-order path used, generalized to any column order/count: + // a row whose every PRESENT cell is "-" is the placeholder. + const placeholderRow = (row: Record): boolean => + Object.values(row).every((cell) => cell.trim() === '-'); + const withoutPlaceholder = deleteTableRow(tableText, placeholderRow); + if (withoutPlaceholder.ok) tableText = withoutPlaceholder.value; - content = content.replace(byPhaseTablePattern, (_match, tableHeader: string) => `${tableHeader}${tableBody}\n`); + // Map the By-Phase values onto the table's ACTUAL header columns by + // NAME — an unrecognized column (a superset header) falls back to "-", + // insertTableRow's default. + const valueFor = (col: string): string | undefined => { + if (col === 'Phase') return String(phaseNum); + if (col === 'Plans') return String(summaryCount); + if (col === 'Total' || col === 'Avg/Plan') return '-'; + return undefined; + }; + const insertResult = insertTableRow(tableText, valueFor); + if (insertResult.ok) tableText = insertResult.value; + + content = before + tableText; + } } // Velocity: Total plans completed — DERIVED as the sum of the By-Phase Plans column - // (the second cell) across all data rows. Idempotent by construction (re-running phase - // complete upserts the same row → same sum) and self-healing (a hand-edited inflated - // total is corrected to the true sum on the next completion). When the By-Phase table - // is absent, leave the velocity total unchanged rather than guess. (#1582) + // across all data rows. Idempotent by construction (re-running phase complete upserts + // the same row → same sum) and self-healing (a hand-edited inflated total is corrected + // to the true sum on the next completion). When the By-Phase table is absent, leave the + // velocity total unchanged rather than guess. (#1582) + // + // Ragged-tolerant AND name-addressed (#2245 audit): each data row is split via + // `splitTableRow` and its "Plans" cell located by the HEADER's own column + // order (not a fixed ordinal), so a reordered/superset By-Phase header is + // summed correctly instead of silently reading the wrong cell. A row that's + // too short to physically contain the "Plans" column is skipped, not + // treated as an error — mirrors updateTableCell's ragged-row tolerance + // (a hand-edited/ragged row for one phase must not blank out the derived + // total for every phase). Still scoped via findTableStartOffset so the RIGHT + // table is summed when an earlier unrelated table also has a "Phase" + // column (#2012). if (/Total plans completed:\s*(\d+|\[N\])/.test(content)) { - const tableForSum = content.match(byPhaseTablePattern); - if (tableForSum) { + const sumTableStart = findTableStartOffset(content, byPhaseCols); + if (sumTableStart !== null) { + const tableLines = content.slice(sumTableStart).split(/\r?\n/); + const headerCells = splitTableRow(tableLines[0] ?? ''); + const plansIdx = headerCells.indexOf('Plans'); let sum = 0; - for (const row of tableForSum[2].split(/\r?\n/)) { - // Data rows look like `| | | … |`, optionally indented (the - // byPhaseTablePattern data-row capture allows `[ \t]*` leading whitespace, so the - // sum must too or hand-edited/legacy indented rows are silently skipped — #1582 - // codex review). Header (`| Phase | Plans | …`) and separator (`| --- | --- | …`) - // rows have a non-numeric second cell and are skipped; non-numeric cells → 0. - const cellMatch = row.match(/^\s*\|\s*[^|]+\s*\|\s*(\d+)\s*\|/); - if (cellMatch) sum += parseInt(cellMatch[1], 10); + if (plansIdx !== -1) { + // The delimiter row is skipped by NAME (isDelimiterRow), not by a + // hardcoded "always line index 1" assumption, so this stays + // self-consistent with the ragged-tolerant read below. + const delimiterCells = splitTableRow(tableLines[1] ?? ''); + const dataStart = isDelimiterRow(delimiterCells) ? 2 : 1; + for (const row of tableLines.slice(dataStart)) { + if (!row.trim().startsWith('|')) break; + const cells = splitTableRow(row); + if (plansIdx < cells.length && /^\d+$/.test(cells[plansIdx])) { + sum += parseInt(cells[plansIdx], 10); + } + } } content = content.replace( /Total plans completed:\s*(\d+|\[N\])/, @@ -2344,7 +2562,10 @@ function cmdStateValidate(cwd: string, raw: boolean): void { warnings.push(`Status drift: STATE.md says "${status}" but ${vf} shows verification passed — phase may be complete`); drift['verification_status'] = { state_status: status, verification: 'passed' }; } - } catch { /* intentionally empty */ } + } catch { /* best-effort (#2245 audit): cmdStateValidate is a diagnostic + * warnings scan across N VERIFICATION.md files — one unreadable file + * (permission/race) must not abort the scan of the rest; it's simply + * excluded from drift detection. */ } } // Check if all plans have summaries but status still says executing @@ -2355,7 +2576,13 @@ function cmdStateValidate(cwd: string, raw: boolean): void { } } } - } catch { /* intentionally empty */ } + } catch { /* best-effort (#2245 audit): cmdStateValidate is a read-only + * diagnostic scan of the current phase's directory (readdirSync + + * scanPhasePlans). A disk-scan failure here means drift detection for + * this phase is skipped for this run, degrading to "no warnings from + * that scan" rather than crashing the validate command — the same + * degrade-on-scan-failure pattern buildStateFrontmatter's own disk scan + * already uses. */ } } const valid = warnings.length === 0; @@ -2451,29 +2678,30 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b // Determine total phases from ROADMAP (may be larger than realized disk dirs). // Mirrors the logic in buildStateFrontmatter so both report consistent percents (#3242 Bug B). + // DEAD catch removed (#2245 audit): every operation in this block is a regex + // exec/test over an already-read string plus pure Set/Math ops — none of + // which can throw — so the try/catch could never be triggered. let syncTotalPhases: number | null = null; - try { - let roadmapPhaseCount = 0; - if (syncRoadmapScope !== null) { - // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). - const phaseHeadingPattern = /#{2,4}\s*Phase\s+([\w][\w.-]*)(?:\s*\([^)\n]{0,200}\))?\s*:/gi; - let m: RegExpExecArray | null; - while ((m = phaseHeadingPattern.exec(syncRoadmapScope)) !== null) { - // Only count tokens that contain at least one digit — excludes - // pure-word section headings (Overview, Details) while keeping - // numeric phases (01, 05.1) and project-code IDs (PROJ-42). - if (!/\d/.test(m[1])) continue; - // #1514: retired/folded phases are struck through; exclude from total. - if (syncRetiredPhaseNums.has(phaseKeyFromToken(m[1]))) continue; - roadmapPhaseCount++; - } + let roadmapPhaseCount = 0; + if (syncRoadmapScope !== null) { + // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). + const phaseHeadingPattern = /#{2,4}\s*Phase\s+([\w][\w.-]*)(?:\s*\([^)\n]{0,200}\))?\s*:/gi; + let m: RegExpExecArray | null; + while ((m = phaseHeadingPattern.exec(syncRoadmapScope)) !== null) { + // Only count tokens that contain at least one digit — excludes + // pure-word section headings (Overview, Details) while keeping + // numeric phases (01, 05.1) and project-code IDs (PROJ-42). + if (!/\d/.test(m[1])) continue; + // #1514: retired/folded phases are struck through; exclude from total. + if (syncRetiredPhaseNums.has(phaseKeyFromToken(m[1]))) continue; + roadmapPhaseCount++; } - if (roadmapPhaseCount > 0) { - syncTotalPhases = Math.max(entries.length, roadmapPhaseCount); - } else { - syncTotalPhases = entries.length; - } - } catch { /* intentionally empty */ } + } + if (roadmapPhaseCount > 0) { + syncTotalPhases = Math.max(entries.length, roadmapPhaseCount); + } else { + syncTotalPhases = entries.length; + } // ADR-1769 Phase 7: the body writes (Total Plans in Phase, Progress bar, Last // Activity) are the pure `syncCore` in src/state-transition.cts. diff --git a/src/uat.cts b/src/uat.cts index e5cd5e964..9e271c2be 100644 --- a/src/uat.cts +++ b/src/uat.cts @@ -18,6 +18,9 @@ const { output, error } = io; import markdownSectionizer = require('./markdown-sectionizer.cjs'); const { collectSection, tokenizeHeadings } = markdownSectionizer; // eslint-disable-next-line @typescript-eslint/no-require-imports +import markdownTable = require('./markdown-table.cjs'); +const { splitTableRow } = markdownTable; +// eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParser = require('./roadmap-parser.cjs'); const { getMilestonePhaseFilter } = roadmapParser; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -377,24 +380,67 @@ function parseVerificationItems(content: string, status: string): UatItem[] { { levelBounded: true }, ); if (hvSection) { + // #2245 review Fix 3: reverted to the pre-Phase-4 (HEAD 2cbf18642) + // implementation. The live Human Verification section is NOT a strict + // GFM table — the planner/verifier templates mix table rows, numbered + // items, and bullet items in the same section (and a `### N.` heading + // format is common too), so a table-XOR-list read (parse a table, and + // if it parses, suppress numbered/bullet items entirely) silently + // dropped items on any mixed or malformed section: a malformed + // `| N | … |` table with no valid header/delimiter yielded ZERO items + // instead of reading the rows positionally. This per-line scan reads + // table rows AND numbered items AND bullet items as a UNION (whichever + // pattern a given line matches), exactly like OLD, and reads + // `| N | desc |` rows even without a valid table header/delimiter. + // + // #2245 audit: the table-row branch's CELL SPLIT is name/position- + // addressed via `splitTableRow` (escape-aware, canonical) instead of a + // hand-rolled pipe regex — candidacy itself is decided WITHOUT a table + // regex (a leading `|` plus a purely-numeric first cell), so this no + // longer needs an allow-adhoc-markdown suppression at all. const lines = hvSection.body.split('\n'); for (const line of lines) { - // Match table rows: | N | description | ... | - const tableMatch = line.match(/\|\s*(\d+)\s*\|\s*([^|]+)/); + const trimmedLine = line.trim(); + // Match table rows: | N | description | ... — candidacy requires a + // leading pipe and a purely-numeric first cell (mirrors what the old + // regex effectively required: a "|digit|" cell immediately followed + // by more content), with at least 2 physical cells so a bare "| N |" + // with nothing after it is NOT treated as a row. + // + // #2245 review Fix 9: this is NOT the same as OLD for a row whose + // ONLY content past the digit cell is trailing whitespace (e.g. + // "| N | ", no second delimiting `|`). OLD's `([^|]+)` regex ran + // against the RAW (untrimmed) line and its `\s*` would backtrack to + // let `[^|]+` swallow that trailing whitespace, so OLD matched and + // pushed an item with an EMPTY (`.trim()`-collapsed) name. Here, + // `trimmedLine = line.trim()` strips that trailing whitespace BEFORE + // `splitTableRow` ever sees it, collapsing the line to a single cell + // (`candidateCells.length === 1`), which fails the `>= 2` check — + // the item is silently dropped instead. A real, acceptable behaviour + // change (an empty-named UAT item is not useful either way), but the + // two implementations are NOT equivalent on this input. + let tableCells: string[] | null = null; + if (trimmedLine.startsWith('|')) { + const candidateCells = splitTableRow(trimmedLine); + if (candidateCells.length >= 2 && /^\d+$/.test(candidateCells[0])) { + tableCells = candidateCells; + } + } // Match bullet items: - description const bulletMatch = line.match(/^[-*]\s+(.+)/); // Match numbered items: 1. description const numberedMatch = line.match(/^(\d+)\.\s+(.+)/); - if (tableMatch) { + if (tableCells) { // Skip rows that already have a passing result (PASS, pass, resolved, etc.) - const rowRemainder = line.slice(tableMatch.index! + tableMatch[0].length); - const cellValues = rowRemainder.split('|').map(c => c.trim()); - const hasPassResult = cellValues.some(c => /^pass$/i.test(c) || /^resolved$/i.test(c)); + // — checked over every cell AFTER the description column, mirroring + // OLD's rowRemainder scan (which only ever saw cells past the + // description, the description itself having already been consumed). + const hasPassResult = tableCells.slice(2).some(c => /^pass$/i.test(c) || /^resolved$/i.test(c)); if (hasPassResult) continue; items.push({ - test: parseInt(tableMatch[1], 10), - name: tableMatch[2].trim(), + test: parseInt(tableCells[0], 10), + name: tableCells[1] ?? '', result: 'human_needed', category: 'human_uat', }); diff --git a/tests/eslint-rules.test.cjs b/tests/eslint-rules.test.cjs index 127bf8823..0e12dd390 100644 --- a/tests/eslint-rules.test.cjs +++ b/tests/eslint-rules.test.cjs @@ -977,4 +977,322 @@ describe('no-adhoc-markdown-parsing rule', () => { invalid: [], }); }); + + // ── TABLE-REGEX (ADR-2143 §7) ────────────────────────────────────────────── + + test('invalid: table-row/cell regex with escaped pipe and negated-pipe cell class', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [], + invalid: [ + { + // /\|[^|]*\|/ — the classic hand-rolled table-row/cell scan fingerprint + code: String.raw`const rowRe = /\|[^|]*\|/;`, + filename: 'src/some-module.cts', + errors: [{ messageId: 'tableRegex' }], + }, + ], + }); + }); + + test('invalid: table-cell regex with escaped-pipe class variant [^\\|]', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [], + invalid: [ + { + code: String.raw`const cellRe = /\|\s*([^\|]+)\s*\|/;`, + filename: 'src/some-module.cts', + errors: [{ messageId: 'tableRegex' }], + }, + ], + }); + }); + + test('valid: parseMarkdownTable() seam call is NOT flagged (no regex literal)', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [ + { + code: ` + const { parseMarkdownTable } = require('./markdown-table'); + const result = parseMarkdownTable(sectionText); + `, + filename: 'src/some-module.cts', + }, + ], + invalid: [], + }); + }); + + test('valid: escaped pipe alone (no negated-pipe cell class) is NOT flagged', () => { + // A bare delimiter probe like /^\|/ or /\|\|/ has an escaped pipe but no + // [^|] cell-capture class — not a table-row/cell scan, so it must stay + // conservative and not fire. + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [ + { + code: String.raw`const isPipeDelim = /^\|/;`, + filename: 'src/some-module.cts', + }, + { + code: String.raw`const orDelim = /a\|b/;`, + filename: 'src/some-module.cts', + }, + ], + invalid: [], + }); + }); + + test('valid: annotated table-regex with allow-adhoc-markdown is NOT flagged', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [ + { + code: String.raw`const rowRe = /\|[^|]*\|/; // allow-adhoc-markdown: not a table scan, protocol-marker probe`, + filename: 'src/some-module.cts', + }, + ], + invalid: [], + }); + }); + + test('valid: table-regex in a non-src/*.cts file is not flagged (rule is inert there)', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [ + { + code: String.raw`const rowRe = /\|[^|]*\|/;`, + filename: 'scripts/helper.cjs', + }, + ], + invalid: [], + }); + }); + + // ── TABLE-REGEX via new RegExp() (#2143 Phase 4) ─ + + test('invalid: new RegExp() matching the table fingerprint', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [], + invalid: [ + { + // new RegExp('\|[^|]*\|') — doubled backslashes cook to a literal \| + code: String.raw`const rowRe = new RegExp('\\|[^|]*\\|');`, + filename: 'src/some-module.cts', + errors: [{ messageId: 'tableRegex' }], + }, + ], + }); + }); + + test('invalid: new RegExp(