diff --git a/.changeset/brave-orcas-roar.md b/.changeset/brave-orcas-roar.md new file mode 100644 index 000000000..c2ff95656 --- /dev/null +++ b/.changeset/brave-orcas-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2889 +--- +**The markdown-parsing lint rule now catches the stricter cell-regex spelling it previously missed** — a hand-rolled table scan written as `[^|\n]` (excluding both the pipe and the newline, which is the more correct form) slipped past the guard entirely, so `STATE.md` field replacement kept parsing tables with a local regex and rewriting the whole document. The rule now flags any pipe-excluding character class, and the STATE.md field writer edits a bounded byte range instead. (#2880) diff --git a/CONTEXT.md b/CONTEXT.md index 986008b20..f8cb30dd0 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -407,7 +407,7 @@ A test that generates many adversarial inputs automatically (via `fast-check`) a Stryker injects small code mutations (e.g., flipping a `>` to `>=`, deleting a `return` statement) and reruns the test suite for each. A mutation is "killed" if at least one test fails; "surviving" if all tests pass despite the mutation. Mutation score = killed / total. Score below 80 % on the changed scope blocks PR merge. See `RULESET.TESTS.mutation-score`. ### ESLint harness -The canonical lint infrastructure adopted in ADR 452 (`docs/adr/452-eslint-lint-harness.md`): ESLint flat config (`eslint.config.mjs`) with `typescript-eslint`, `eslint-plugin-n`, `eslint-plugin-no-only-tests`, and a local AST-rule plugin at `scripts/eslint-rules/`. Replaces the homegrown `scripts/lint-*.cjs` regex scanners. The three custom test-rigor rules (`local/no-source-grep`, `local/no-magic-sleep-in-tests`, `local/no-elapsed-assertion`) initially ship at `warn`; they become `error` after the cleanup sweep tracked at issue #453 merges. +The canonical lint infrastructure adopted in ADR 452 (`docs/adr/452-eslint-lint-harness.md`): ESLint flat config (`eslint.config.mjs`) with `typescript-eslint`, `eslint-plugin-n`, `eslint-plugin-no-only-tests`, and a local AST-rule plugin at `eslint-rules/`. Replaces the homegrown `scripts/lint-*.cjs` regex scanners. The three custom test-rigor rules (`local/no-source-grep`, `local/no-magic-sleep-in-tests`, `local/no-elapsed-assertion`) initially ship at `warn`; they become `error` after the cleanup sweep tracked at issue #453 merges. ### External-job-waiting half-state A legal deferred state of an Execute step (`external_job_waiting`): the executor has dispatched a long-running async external job and committed an async-job manifest at `.planning/async-jobs/.json` instead of a SUMMARY.md. Distinct from the synchronous "mid-production-commits" half-state and from an illegal partial-plan state. The core loop's step-completion + safe-resume/pause contract treats a non-terminal manifest as legal and reconciles against it (never re-dispatching the plan, which would duplicate the external job); SUMMARY.md is deferred until the job reaches a terminal state and its `expected_artifacts` are verified. The manifest is a versioned stability contract (`docs/reference/planning-artifacts.md`); core *consumes* it while a default-off scheduler-adapter Capability (#1164) *produces* it at `execute:wave:post` — the contract-is-core / producer-is-capability seam mirrors ADR-857's verification-substrate decision. Status enum is closed and scheduler-agnostic: `submitted`, `running`, `completed-unverified`, `failed`, `cancelled`, `timeout`. diff --git a/eslint-rules/no-adhoc-markdown-parsing.cjs b/eslint-rules/no-adhoc-markdown-parsing.cjs index af2d36adc..124cb12d7 100644 --- a/eslint-rules/no-adhoc-markdown-parsing.cjs +++ b/eslint-rules/no-adhoc-markdown-parsing.cjs @@ -170,11 +170,92 @@ const rule = { // [^\|]), 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. + // + // The negated class qualifies ONLY when its body is the pipe (escaped or + // bare) plus zero or more of the two-character line-terminator/tab escapes + // `\n`, `\r`, `\t` (#2880): the common, and strictly MORE correct, spelling + // is `[^|\n]` (a GFM cell can span neither a pipe nor a line break) — + // `src/state-document.cts` used exactly that and evaded the rule entirely, + // which is the ADR-2143 §7 enforcement hole this widening closes. A + // negated class that excludes the pipe alongside anything ELSE — e.g. + // `[^\s|]`, `[^"|]`, `[^a-z|]` — is a different (non-table) idiom and is + // NOT flagged, and a negated class that does not exclude a pipe at all + // (`[^\n]`, `[^a-z]`) is still NOT a cell scan. + // + // Implemented as a single-pass scanner (not a regex) — see + // hasQualifyingNegatedPipeClass below for why the regex encoding of this + // fingerprint was rejected (quadratic-on-failure blowup). 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); + return hasQualifyingNegatedPipeClass(src); + } + + // ── Single-pass negated-character-class scanner (FIX 3 + FIX 4) ───────── + // Walks `src` once, left to right. On encountering a negated class + // `[^...]` it scans forward to the class's closing `]` (honoring `\` + // escapes within the class) exactly once, then resumes scanning + // immediately AFTER that `]` — never backtracking into the class body. + // This keeps the whole walk O(n) regardless of how many negated classes + // (or how large) the source contains — unlike the regex it replaces, + // /\[\^[^\]]*\\?\|[^\]]*\]/, which is quadratic on failure (two unbounded + // [^\]]* runs around an optional), measured at ~23s for a 256000-char + // adversarial input. + // + // Returns true if ANY negated class in `src` QUALIFIES as a hand-rolled + // GFM cell-capture class: it excludes a pipe (`\|` or bare `|`) and, + // after removing that pipe, every remaining member is one of the + // two-character line-terminator/tab escapes `\n`, `\r`, `\t` (zero extra + // members is fine — `[^|]` alone qualifies). A class that excludes the + // pipe alongside anything ELSE (`[^\s|]`, `[^"|]`, `[^a-z|]`) does NOT + // qualify — that is a different, non-table idiom. A class that never + // excludes a pipe at all (`[^\n]`, `[^a-z]`) does not qualify either. + function hasQualifyingNegatedPipeClass(src) { + let i = 0; + while (i < src.length) { + const ch = src[i]; + if (ch === '\\') { + i += 2; + continue; + } + if (ch === '[' && src[i + 1] === '^') { + let j = i + 2; + let classHasPipe = false; + let classIsPure = true; + let closed = false; + while (j < src.length) { + const cc = src[j]; + if (cc === '\\') { + const next = src[j + 1]; + if (next === '|') { + classHasPipe = true; + } + else if (next !== 'n' && next !== 'r' && next !== 't') { + classIsPure = false; + } + j += 2; + continue; + } + if (cc === ']') { + closed = true; + j += 1; + break; + } + if (cc === '|') { + classHasPipe = true; + } + else { + classIsPure = false; + } + j += 1; + } + if (closed && classHasPipe && classIsPure) return true; + i = closed ? j : src.length; + continue; + } + i += 1; + } + return false; } function isTableRegex(node) { diff --git a/src/state-document.cts b/src/state-document.cts index 942c571ad..c0c2910be 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -7,6 +7,8 @@ * from the prior hand-written .cjs; only types are added. */ +import { splitTableRow } from './markdown-table.cjs'; + // Internal helpers function escapeRegex(str: string): string { return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -42,25 +44,171 @@ function isTableSeparatorRow(firstCell: string): boolean { return /^[\s\-:]+$/.test(firstCell.trim()); } +function countLeading(str: string): number { + const match = /^[ \t]*/.exec(str); + return match ? match[0].length : 0; +} + /** - * Build a regex that matches a pipe-table row `| FieldName | value |` for the - * given (already-escaped) field name. The match is case-insensitive and - * tolerates variable amounts of whitespace around the cell contents. - * - * Capture group 1: leading pipe + whitespace before the field cell - * Capture group 2: the field name cell text (trimmed) - * Capture group 3: whitespace between field cell and separator pipe - * Capture group 4: the value cell text (trimmed) - * Capture group 5: trailing whitespace + closing pipe(s) - * - * We use a single-line match (`m` flag so ^ anchors work on each line) to - * avoid cross-row replacement. + * Canonicalize one UTF-16 code unit per the ECMAScript non-unicode + * `Canonicalize` abstract operation, which governs how a case-insensitive + * (`/i`, no `u` flag) RegExp compares characters: take `ch.toUpperCase()`. + * The uppercasing is REJECTED (the original character is kept as-is) in + * either of two cases: (1) `ch.toUpperCase()` does not produce exactly one + * character (e.g. "ß" -> "SS" — a multi-character case-fold can never be a + * per-character regex match, so Canonicalize leaves it alone), or (2) it + * produces exactly one character but the original character's code point is + * >= 128 while the uppercased character's code point is < 128 (this is what + * stops a non-ASCII character from folding onto an ASCII one under `/i` — + * e.g. KELVIN SIGN U+212A uppercases to ASCII "K" (U+004B), so this rule + * rejects the fold and keeps U+212A, meaning `/k/i`/`/K/i` do NOT match + * U+212A). Otherwise, the uppercased character is used. Plain + * `.toLowerCase()`/`.toUpperCase()` folds both of these cases, which is + * exactly why they diverge from real regex `/i` semantics. */ -function tableRowPattern(escapedFieldName: string): RegExp { - return new RegExp( - `^(\\|[ \\t]*)(${escapedFieldName})([ \\t]*\\|[ \\t]*)([^|\\n]*?)([ \\t]*\\|[ \\t]*)$`, - 'im', - ); +function canonicalizeCharForCaselessCompare(ch: string): string { + const upper = ch.toUpperCase(); + if (upper.length !== 1) { + return ch; + } + if (ch.charCodeAt(0) >= 128 && upper.charCodeAt(0) < 128) { + return ch; + } + return upper; +} + +/** + * Canonicalize a whole string, one UTF-16 code unit at a time, per the + * ECMAScript non-unicode `Canonicalize` rule (see + * canonicalizeCharForCaselessCompare) so that two strings compare equal + * under this function iff a non-`u`-flag `/i` RegExp would treat them as + * the same literal text. This is the correct replacement for + * `.toLowerCase()` when replicating a non-`u` `/i` regex: `.toLowerCase()` + * folds some non-ASCII characters (e.g. KELVIN SIGN U+212A) onto their + * ASCII counterparts, which real `/i` regex semantics do not. Iteration is + * by UTF-16 code unit (not code point) to match how a non-`u` regex engine + * itself operates on surrogate halves individually. + */ +function canonicalizeForCaselessCompare(str: string): string { + let result = ''; + for (let i = 0; i < str.length; i++) { + result += canonicalizeCharForCaselessCompare(str[i]); + } + return result; +} + +/** + * Return true when the caller's raw (untrimmed) `fieldName` may be considered + * to match a row's raw (untrimmed) field cell text. Faithfully replicates the + * backtracking of the regex this function replaced: `^(\|[ \t]*)(FieldName) + * ([ \t]*\|...)`. Group 1 (`\|[ \t]*`, greedy but backtrackable) can hand any + * PREFIX of the cell's leading `[ \t]` run over to group 2 (the literal, + * case-insensitive `fieldName` text) — so `fieldName` is tried at every offset + * `j` from 0 up to the length of that leading run. For a given `j` to be a + * genuine match, two things must hold: `rawCell.slice(j, j + fieldName.length)` + * must equal `fieldName` case-insensitively (group 2), AND everything left + * over after it — `rawCell.slice(j + fieldName.length)` — must be entirely + * `[ \t]` characters, because group 3 (`[ \t]*\|`) must consume that leftover + * as whitespace before it can reach the delimiter pipe. + * + * A simple count-of-leading/trailing-whitespace comparison is NOT equivalent: + * it ignores that group 2 is a literal-character match, not a whitespace- + * class match, so it can produce false positives whenever `fieldName`'s own + * padding is a different run of `[ \t]` characters than the cell's (e.g. + * `fieldName` padded with spaces against a cell padded with tabs) — caught by + * differential fuzzing against the regex this replaces. + * + * The case-insensitive comparison itself is done via + * canonicalizeForCaselessCompare, NOT `.toLowerCase()`: the replaced regex + * used `/i` WITHOUT the `u` flag, whose case-folding is the ECMAScript + * non-unicode `Canonicalize` operation. `.toLowerCase()` folds some non-ASCII + * characters onto ASCII ones (e.g. KELVIN SIGN U+212A -> "k") that `/i` + * (no `u`) does NOT fold, so `.toLowerCase()` alone would NOT faithfully + * replicate the old regex's semantics; canonicalizeForCaselessCompare does. + */ +function fieldNameMatchesRawCell(fieldName: string, rawCell: string): boolean { + const n = fieldName.length; + const cellLength = rawCell.length; + if (n > cellLength) + return false; + const leadingRun = countLeading(rawCell); + const maxOffset = Math.min(leadingRun, cellLength - n); + const canonicalFieldName = canonicalizeForCaselessCompare(fieldName); + for (let j = 0; j <= maxOffset; j++) { + if (canonicalizeForCaselessCompare(rawCell.slice(j, j + n)) !== canonicalFieldName) + continue; + if (/^[ \t]*$/.test(rawCell.slice(j + n))) + return true; + } + return false; +} + +/** + * Locate the value cell of a pipe-table row `| FieldName | value |` for the + * given field name, by scanning `content` line by line (no whole-document + * regex). Only a strict two-column row (exactly 3 `|` chars, starting the + * line, ending the line after trailing space/tab is stripped) is considered; + * this is what makes a 3-column row or an unescaped-pipe-bearing value cell + * fail to match, mirroring the previous regex's behaviour. Separator rows + * (`| --- | --- |`) are skipped, not matched. The match is case-insensitive. + * A line terminator is `\r\n`, a lone `\r`, or a lone `\n` — matching the `m` + * flag semantics of the regex this function replaced. Returns the byte range + * of the value cell (after trimming surrounding space/tab) so the caller can + * splice it directly. + */ +function locateFieldRow(content: string, fieldName: string): { valueStart: number; valueEnd: number; rawValue: string } | null { + let lineStart = 0; + while (lineStart <= content.length) { + // A line terminator is `\r\n`, a lone `\r`, or a lone `\n` (JS treats a + // bare `\r` as a line terminator too — the regex this replaced used the + // `m` flag, which honors all three). Scan for whichever of `\r`/`\n` + // occurs first; if it's `\r` immediately followed by `\n`, the terminator + // is 2 chars wide, otherwise 1. + let terminatorIndex = -1; + let terminatorLength = 0; + for (let i = lineStart; i < content.length; i++) { + const ch = content[i]; + if (ch === '\n') { + terminatorIndex = i; + terminatorLength = 1; + break; + } + if (ch === '\r') { + terminatorIndex = i; + terminatorLength = content[i + 1] === '\n' ? 2 : 1; + break; + } + } + const lineEnd = terminatorIndex === -1 ? content.length : terminatorIndex; + const line = content.slice(lineStart, lineEnd); + if (line.startsWith('|')) { + const pipeCount = (line.match(/\|/g) || []).length; + const trimmedEnd = line.replace(/[ \t]+$/, ''); + if (pipeCount === 3 && trimmedEnd.endsWith('|')) { + const cells = splitTableRow(line); + if (cells.length === 2 && !isTableSeparatorRow(cells[0])) { + // Line has exactly 3 pipes (enforced above): opening pipe, the + // field/value separator pipe, and the row-closing pipe. + const fieldValueSeparatorPipe = line.indexOf('|', line.indexOf('|') + 1); + const rawCell = line.slice(1, fieldValueSeparatorPipe); + if (fieldNameMatchesRawCell(fieldName, rawCell)) { + const rowClosingPipe = line.indexOf('|', fieldValueSeparatorPipe + 1); + let valueStart = lineStart + fieldValueSeparatorPipe + 1; + while (content[valueStart] === ' ' || content[valueStart] === '\t') + valueStart++; + let valueEnd = lineStart + rowClosingPipe; + while (valueEnd - 1 >= valueStart && (content[valueEnd - 1] === ' ' || content[valueEnd - 1] === '\t')) + valueEnd--; + return { valueStart, valueEnd, rawValue: content.slice(valueStart, valueEnd) }; + } + } + } + } + if (terminatorIndex === -1) + break; + lineStart = terminatorIndex + terminatorLength; + } + return null; } export function stateExtractField(content: string, fieldName: string): string | null { @@ -77,9 +225,9 @@ export function stateExtractField(content: string, fieldName: string): string | return plainMatch[1].trim(); // Pipe-table format: | FieldName | value | // (Separator rows such as `| --- | --- |` are excluded.) - const tableMatch = content.match(tableRowPattern(escaped)); - if (tableMatch && !isTableSeparatorRow(tableMatch[2])) - return tableMatch[4].trim(); + const hit = locateFieldRow(content, fieldName); + if (hit) + return hit.rawValue.trim(); return null; } @@ -97,13 +245,9 @@ export function stateReplaceField(content: string, fieldName: string, newValue: } // Pipe-table format: | FieldName | value | // Preserve the surrounding pipe/whitespace structure; only swap the value cell. - const tblPat = tableRowPattern(escaped); - const tblMatch = content.match(tblPat); - if (tblMatch && !isTableSeparatorRow(tblMatch[2])) { - // Reconstruct the row, preserving the original surrounding whitespace/pipes. - return content.replace(tblPat, (_m, leadPipe: string, fieldCell: string, midPipe: string, _oldVal: string, trailPipe: string) => - `${leadPipe}${fieldCell}${midPipe}${newValue}${trailPipe}`, - ); + const hit = locateFieldRow(content, fieldName); + if (hit) { + return content.slice(0, hit.valueStart) + newValue + content.slice(hit.valueEnd); } return null; } diff --git a/tests/eslint-rules.test.cjs b/tests/eslint-rules.test.cjs index 0e12dd390..0e9f90fad 100644 --- a/tests/eslint-rules.test.cjs +++ b/tests/eslint-rules.test.cjs @@ -14,6 +14,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const { RuleTester } = require('eslint'); +const fc = require('fast-check'); const noSourceGrep = require('../eslint-rules/no-source-grep.cjs'); const noMagicSleepInTests = require('../eslint-rules/no-magic-sleep-in-tests.cjs'); @@ -1295,4 +1296,296 @@ describe('no-adhoc-markdown-parsing rule', () => { invalid: [], }); }); + + // ── TABLE-REGEX widening: [^|\n] and escaped-pipe-plus-others classes (#2880) ── + + test('invalid: content.replace() — the exact shape that evaded the rule before #2880', () => { + ruleTester.run('no-adhoc-markdown-parsing', noAdhocMarkdownParsing, { + valid: [], + invalid: [ + { + // /\|[^|\n]*\|/ — pipe-excluding cell class ALSO excludes newline; this + // is the src/state-document.cts shape that the sole-member-class check + // missed prior to the #2880 widening. + code: String.raw`content.replace(/\|[^|\n]*\|/, 'x');`, + filename: 'src/state-document.cts', + // CallExpression is the outer/enter-first node (adhocReplaceMutation); + // its Literal argument (visited next, on descent) is the second, + // independent tableRegex finding — same ordering as the established + // roadmapContent.replace(...) case above. + errors: [{ messageId: 'adhocReplaceMutation' }, { messageId: 'tableRegex' }], + }, + ], + }); + }); + + test('invalid: factory function returning new RegExp(