golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2128): address review — migrate 9 mis-allowlisted sites, harden scanner + guards

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-10 09:14:15 -04:00
parent dfad3a7510
commit e2eaa5b046
7 changed files with 67 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
scripts/lint-phase-id-drift.cjs
View File

@@ -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;

12
src/audit.cts
View File

@@ -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[];

12
src/init.cts
View File

@@ -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');

2
src/roadmap-parser.cts
View File

@@ -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 {

3
src/roadmap-upgrade.cts
View File

@@ -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;
}

6
src/uat.cts
View File

@@ -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);

34
tests/phase-id-drift-guard.test.cjs
View File

@@ -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}`);
});
});