From e2eaa5b04637fe34b33ec062aedaec7eaae9be23 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 10 Jul 2026 09:14:15 -0400 Subject: [PATCH] =?UTF-8?q?fix(#2128):=20address=20review=20=E2=80=94=20mi?= =?UTF-8?q?grate=209=20mis-allowlisted=20sites,=20harden=20scanner=20+=20g?= =?UTF-8?q?uards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness review of the Phase 4 guard found the allowlist over-broad and the scanner/guards evadable. Fixed all findings: - Migrate 9 sites that were wrongly sanctioned: their regex is the PURE canonical token (`\d+[A-Z]?(?:\.\d+)*`, no variant), byte-identical to already-migrated siblings. The old justification argued against swapping to the extractPhaseToken() FUNCTION (behavior-risky) — but the guard only wants the same regex built from the SOURCE string (byte-equal, zero risk). Coverage is now 32 migrated / 5 sanctioned, not the overstated 23 / 14 (audit.cts x3, uat.cts, init.cts x4, roadmap-upgrade.cts). Each conversion proven byte-equal (.source + .flags). - Harden the drift detector: also catch the `[0-9]`-in-place-of-`\d` variant; document the accepted limits (cross-line split, semantic restructuring — covered by the identity guard + review, not a text scan). - Sanction robustness: a `phase-id-owner:` marker now counts only inside a `//` comment (a bare substring in a string no longer suppresses a real flag), and the preceding-line window skips blank lines (an auto-formatter's blank line no longer reactivates the flag). - roadmap-parser.cts:462 comment: corrected — that regex carries no /i flag, so its [A-Za-z] class does real case work (matches state.cts:1409's rationale). - Identity guard: surface require failures instead of silently skipping, and floor coverage at >75% of consumer modules (inspects 156/157). Co-Authored-By: Claude Opus 4.8 --- scripts/lint-phase-id-drift.cjs | 34 ++++++++++++++++++++--------- src/audit.cts | 12 +++++----- src/init.cts | 12 ++++------ src/roadmap-parser.cts | 2 +- src/roadmap-upgrade.cts | 3 +-- src/uat.cts | 6 +++-- tests/phase-id-drift-guard.test.cjs | 34 +++++++++++++++++++++++------ 7 files changed, 67 insertions(+), 36 deletions(-) diff --git a/scripts/lint-phase-id-drift.cjs b/scripts/lint-phase-id-drift.cjs index 33cecc3cd..3516814e1 100644 --- a/scripts/lint-phase-id-drift.cjs +++ b/scripts/lint-phase-id-drift.cjs @@ -34,19 +34,31 @@ const path = require('node:path'); // The canonical phase-number token as it appears in SOURCE TEXT: // \d+[A-Z]?(?:\.\d+)* in a regex literal -> one backslash before d/. // \\d+[A-Z]?(?:\\.\\d+)* in a template string -> two backslashes -// Also tolerate the [A-Za-z] letter-class and the [.-] (dot-or-dash) separator -// near-variants that a few enumeration call sites use. -const TOKEN_DRIFT_RE = /\\{1,2}d\+\[A-Z(?:a-z)?\]\??\(\?:(?:\\{1,2}\.|\[\.-\])\\{1,2}d\+\)\*/; +// Tolerated near-variants so a trivial rewrite does not silently evade the guard: +// digit class \d \\d or [0-9] +// letter class [A-Z] or [A-Za-z] +// sub-phase sep \. \\. or [.-] (dot-or-dash) +// KNOWN, ACCEPTED limits of a per-line textual scan (covered instead by the +// identity guard + code review, not by this regex): a re-derivation split +// across lines via string concatenation, a capturing `(\.\d+)*` in place of the +// non-capturing group, or a semantically-equivalent restructuring. This guard +// targets the common case — an accidental copy of the exact grammar — not an +// adversary deliberately obfuscating a re-derivation. +const TOKEN_DRIFT_RE = /(?:\\{1,2}d|\[0-9\])\+\[A-Z(?:a-z)?\]\??\(\?:(?:\\{1,2}\.|\[\.-\])(?:\\{1,2}d|\[0-9\])\+\)\*/; -const OWNER_MARK = 'phase-id-owner:'; +// A `phase-id-owner:` sanction only counts inside a `//` line comment — a bare +// substring in a string literal or identifier must NOT suppress a real flag. +const OWNER_RE = /\/\/[^\n]*phase-id-owner:/; const CANON_REF = 'PHASE_NUMBER_TOKEN_SOURCE'; /** * Pure: find every literal re-derivation of the canonical phase-number token in - * `text` that is NOT sanctioned. A site is sanctioned when its line — or the - * line directly above it — contains `// phase-id-owner:`, or when the line - * references `PHASE_NUMBER_TOKEN_SOURCE` (i.e. it is built from the canonical - * source, not a literal). Returns [{ line, found }]. + * `text` that is NOT sanctioned. A site is sanctioned when its own line — or the + * nearest preceding NON-BLANK line (so an auto-formatter's blank line between a + * `// phase-id-owner:` comment and its regex does not reactivate the flag) — + * carries a `// phase-id-owner:` comment, or when the line references + * `PHASE_NUMBER_TOKEN_SOURCE` (built from the canonical source, not a literal). + * Returns [{ line, found }]. */ function findPhaseIdRegexDrift(text) { const out = []; @@ -55,9 +67,11 @@ function findPhaseIdRegexDrift(text) { const line = lines[i]; const m = TOKEN_DRIFT_RE.exec(line); if (!m) continue; - if (line.includes(OWNER_MARK)) continue; - if (i > 0 && lines[i - 1].includes(OWNER_MARK)) continue; + if (OWNER_RE.test(line)) continue; if (line.includes(CANON_REF)) continue; + let j = i - 1; + while (j >= 0 && lines[j].trim() === '') j--; // nearest preceding non-blank line + if (j >= 0 && OWNER_RE.test(lines[j])) continue; out.push({ line: i + 1, found: m[0] }); } return out; diff --git a/src/audit.cts b/src/audit.cts index c7bf2f983..f9a500b9a 100644 --- a/src/audit.cts +++ b/src/audit.cts @@ -20,6 +20,9 @@ const { planningDir } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); const { extractFrontmatter } = frontmatter; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import phaseIdMod = require('./phase-id.cjs'); +const { PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; import { requireSafePath, sanitizeForDisplay } from './security.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -482,8 +485,7 @@ function scanUatGaps(planDir: string): UatGapItem[] { for (const dir of dirs) { const phaseDir = path.join(phasesDir, dir); - // phase-id-owner: cosmetic phase label derived from a dir name for JSON output; the single-segment capture is not equivalent to extractPhaseToken dash-continuation semantics, so not a behavior-preserving drop-in. - const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const phaseMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); const phaseNum = phaseMatch ? phaseMatch[1] : dir; let files: string[]; @@ -553,8 +555,7 @@ function scanVerificationGaps(planDir: string): VerificationGapItem[] { for (const dir of dirs) { const phaseDir = path.join(phasesDir, dir); - // phase-id-owner: cosmetic phase label derived from a dir name for JSON output; the single-segment capture is not equivalent to extractPhaseToken dash-continuation semantics, so not a behavior-preserving drop-in. - const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const phaseMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); const phaseNum = phaseMatch ? phaseMatch[1] : dir; let files: string[]; @@ -616,8 +617,7 @@ function scanContextQuestions(planDir: string): ContextQuestionItem[] { for (const dir of dirs) { const phaseDir = path.join(phasesDir, dir); - // phase-id-owner: cosmetic phase label derived from a dir name for JSON output; the single-segment capture is not equivalent to extractPhaseToken dash-continuation semantics, so not a behavior-preserving drop-in. - const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const phaseMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); const phaseNum = phaseMatch ? phaseMatch[1] : dir; let files: string[]; diff --git a/src/init.cts b/src/init.cts index ab6bcafb1..13b2a890c 100644 --- a/src/init.cts +++ b/src/init.cts @@ -1181,8 +1181,7 @@ function cmdInitMilestoneOp(cwd: string, raw: boolean): void { const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); for (const e of entries) { if (!e.isDirectory()) continue; - // phase-id-owner: dir-name phase-token parse; extractPhaseToken dash-separated sub-phase semantics differ, so a token-source swap would risk remapping phase<->directory matches. Kept local. - const m = stripProjectCodePrefix(e.name).match(/^(\d+[A-Z]?(?:\.\d+)*)/); + const m = stripProjectCodePrefix(e.name).match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`)); if (!m) continue; diskPhaseDirs.set(canonicalizePhase(m[1]), e.name); } @@ -1715,17 +1714,14 @@ function cmdInitProgress(cwd: string, raw: boolean): void { .map((e) => e.name) .filter(isDirInMilestone) .sort((a, b) => { - // phase-id-owner: dir-name phase-token parse; extractPhaseToken dash-separated sub-phase semantics differ, so a token-source swap would risk remapping phase<->directory matches. Kept local. - const pa = a.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); - // phase-id-owner: dir-name phase-token parse; extractPhaseToken dash-separated sub-phase semantics differ, so a token-source swap would risk remapping phase<->directory matches. Kept local. - const pb = b.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const pa = a.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); + const pb = b.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); if (!pa || !pb) return a.localeCompare(b); return parseInt(pa[1], 10) - parseInt(pb[1], 10); }); for (const dir of dirs) { - // phase-id-owner: dir-name phase-token parse; extractPhaseToken dash-separated sub-phase semantics differ, so a token-source swap would risk remapping phase<->directory matches. Kept local. - const dirMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)-?(.*)/i); + const dirMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})-?(.*)`, 'i')); const phaseNumber = dirMatch ? dirMatch[1] : dir; const phaseName = dirMatch && dirMatch[2] ? dirMatch[2] : null; seenPhaseNums.add(phaseNumber.replace(/^0+/, '') || '0'); diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 8647da8d1..091789c48 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -459,7 +459,7 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p // the milestone as a bogus "46-6" id. const numericRe = roadmapUsesHyphenedIds ? /^0*(\d+(?:-\d{2,})*[A-Za-z]?(?:\.\d+)*)/ - // phase-id-owner: [A-Za-z] case-variant token (identical under /i); kept literal, not source-byte-equal to the canonical PHASE_NUMBER_TOKEN_SOURCE. + // phase-id-owner: the [A-Za-z] letter class does real case handling here — this regex carries NO /i flag; kept literal, not source-byte-equal to the canonical PHASE_NUMBER_TOKEN_SOURCE. : /^0*(\d+[A-Za-z]?(?:\.\d+)*)/; function isDirInMilestone(dirName: string): boolean { diff --git a/src/roadmap-upgrade.cts b/src/roadmap-upgrade.cts index e7202a495..2c47995b9 100644 --- a/src/roadmap-upgrade.cts +++ b/src/roadmap-upgrade.cts @@ -175,8 +175,7 @@ function extractPhaseNumFromDir(dirName: string): string | null { const stripped = stripProjectCodePrefix(dirName); // Matches: digits + optional letter + optional decimal suffix, followed by '-' or end. // e.g. "02.1-hotfix" → "02.1", "01-setup" → "01" - // phase-id-owner: strips a leading phase number from a dir name; extractPhaseToken returns the project-code-prefixed token, so it is not a behavior-preserving drop-in. - const m = stripped.match(/^(\d+[A-Z]?(?:\.\d+)*)(?:-|$)/i); + const m = stripped.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})(?:-|$)`, 'i')); return m ? m[1] : null; } diff --git a/src/uat.cts b/src/uat.cts index c5c3900ab..e5cd5e964 100644 --- a/src/uat.cts +++ b/src/uat.cts @@ -29,6 +29,9 @@ const { planningDir } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); const { extractFrontmatter } = frontmatter; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import phaseIdMod = require('./phase-id.cjs'); +const { PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; import { requireSafePath, sanitizeForDisplay } from './security.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -82,8 +85,7 @@ function cmdAuditUat(cwd: string, raw: boolean): void { .sort(); for (const dir of dirs) { - // phase-id-owner: display phase field derived from a dir name (same family as the audit.cts sites); not equivalent to extractPhaseToken for dash-form dirs. - const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const phaseMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); const phaseNum = phaseMatch ? phaseMatch[1] : dir; const phaseDir = path.join(phasesDir, dir); const files = fs.readdirSync(phaseDir); diff --git a/tests/phase-id-drift-guard.test.cjs b/tests/phase-id-drift-guard.test.cjs index 3e20b291a..63858f23c 100644 --- a/tests/phase-id-drift-guard.test.cjs +++ b/tests/phase-id-drift-guard.test.cjs @@ -60,9 +60,10 @@ describe('#2128 phase-id drift scanner: findPhaseIdRegexDrift (pure)', () => { assert.equal(v.length, 1); }); - test('the [A-Za-z] and [.-] near-variants ARE flagged', () => { - assert.equal(findPhaseIdRegexDrift('/(\\d+[A-Za-z]?(?:\\.\\d+)*)/').length, 1); - assert.equal(findPhaseIdRegexDrift('/(\\d+[A-Z]?(?:[.-]\\d+)*)/').length, 1); + test('the [A-Za-z], [.-] and [0-9] near-variants ARE flagged (no trivial evasion)', () => { + assert.equal(findPhaseIdRegexDrift('/(\\d+[A-Za-z]?(?:\\.\\d+)*)/').length, 1, '[A-Za-z] letter class'); + assert.equal(findPhaseIdRegexDrift('/(\\d+[A-Z]?(?:[.-]\\d+)*)/').length, 1, '[.-] separator'); + assert.equal(findPhaseIdRegexDrift('/([0-9]+[A-Z]?(?:\\.[0-9]+)*)/').length, 1, '[0-9] in place of \\d'); }); test('a same-line // phase-id-owner: sanction suppresses the flag', () => { @@ -79,6 +80,18 @@ describe('#2128 phase-id drift scanner: findPhaseIdRegexDrift (pure)', () => { ); }); + test('a blank line between the // phase-id-owner: comment and the regex still suppresses', () => { + assert.deepEqual( + findPhaseIdRegexDrift('// phase-id-owner: sanctioned exception\n\nconst re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'), + [], + ); + }); + + test('a bare "phase-id-owner:" substring in a STRING (not a // comment) does NOT suppress', () => { + const v = findPhaseIdRegexDrift('const msg = "ping the phase-id-owner: for review"; const re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'); + assert.equal(v.length, 1); + }); + test('non-token phase regexes are NOT flagged (no false positives)', () => { assert.deepEqual(findPhaseIdRegexDrift('/^Executing Phase\\s+\\d+/'), [], 'status-message bare \\d+'); assert.deepEqual(findPhaseIdRegexDrift('/#{2,4}\\s*Phase\\s+(\\d+)[A-Z]?(?:\\.\\d+)*/'), [], 'digits-only capture is non-contiguous'); @@ -119,14 +132,18 @@ describe('#2128 phase-id single-owner identity guard', () => { const libDir = path.join(ROOT, 'gsd-core', 'bin', 'lib'); const consumers = fs.readdirSync(libDir).filter((f) => f.endsWith('.cjs') && f !== 'phase-id.cjs'); let checked = 0; + const requireFailures = []; for (const f of consumers) { let mod; try { mod = require(path.join(libDir, f)); - } catch { - continue; // a module that cannot be required in isolation can't re-export anything + } catch (e) { + // Surfaced, not silently skipped — a module that cannot be required + // would otherwise erode the guard's coverage without any signal. + requireFailures.push(`${f}: ${e.message}`); + continue; } - if (!mod || typeof mod !== 'object') continue; + if (!mod || typeof mod !== 'object') continue; // bare-function exports carry no named canonical member checked++; for (const name of CANONICAL) { if (Object.prototype.hasOwnProperty.call(mod, name)) { @@ -138,6 +155,9 @@ describe('#2128 phase-id single-owner identity guard', () => { } } } - assert.ok(checked > 0, 'expected to inspect at least one consumer module'); + assert.deepEqual(requireFailures, [], `consumer module(s) failed to require (guard coverage would silently degrade):\n ${requireFailures.join('\n ')}`); + // Coverage floor: the vast majority of the ~150 built lib modules export an + // object and must actually be inspected — not a token "at least one". + assert.ok(checked > consumers.length * 0.75, `expected to inspect most of the ${consumers.length} consumer modules, only inspected ${checked}`); }); });