From a1de52d71bea145085707c35874ac46c4eca140e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 10 Jul 2026 09:24:49 -0400 Subject: [PATCH] fix(#2128): sanctions must be a dedicated // comment line (decoy-proof) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review found the `// phase-id-owner:` suppression treated a `//` embedded in a string literal as a comment — help/doc text quoting the sanction syntax (the exact string the scanner's own main() prints) would silently suppress a real re-derivation. Require the marker to LEAD its own comment line (`^\s*//…`), so a `//` inside a string or trailing a code line never counts. All 5 real sanctions are already dedicated lines (scanRepo stays green); trailing same-line sanctions are no longer honored — put the comment on the line directly above. Co-Authored-By: Claude Opus 4.8 --- scripts/lint-phase-id-drift.cjs | 23 +++++++++++--------- tests/phase-id-drift-guard.test.cjs | 33 ++++++++++++++++++----------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/scripts/lint-phase-id-drift.cjs b/scripts/lint-phase-id-drift.cjs index 3516814e1..ff04bfa72 100644 --- a/scripts/lint-phase-id-drift.cjs +++ b/scripts/lint-phase-id-drift.cjs @@ -46,18 +46,21 @@ const path = require('node:path'); // 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\])\+\)\*/; -// 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:/; +// A `phase-id-owner:` sanction must be a DEDICATED `//` comment line (the marker +// as the line's leading token). A `//` or the phrase embedded in a string +// literal or trailing a code line is NOT a comment and must never suppress a real +// flag — so sanctions live on their own line directly above the regex. +const OWNER_RE = /^\s*\/\/.*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 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). + * `text` that is NOT sanctioned. A site is sanctioned when the nearest preceding + * NON-BLANK line is a dedicated `// phase-id-owner:` comment (blank lines between + * the comment and the regex are tolerated, so an auto-formatter cannot reactivate + * the flag), or when the regex line references `PHASE_NUMBER_TOKEN_SOURCE` (built + * from the canonical source, not a literal). A `//`/phrase inside a string or + * trailing a code line does NOT count — put the sanction on its own line above. * Returns [{ line, found }]. */ function findPhaseIdRegexDrift(text) { @@ -67,7 +70,6 @@ function findPhaseIdRegexDrift(text) { const line = lines[i]; const m = TOKEN_DRIFT_RE.exec(line); if (!m) 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 @@ -135,7 +137,8 @@ function main() { } process.stderr.write('phase-id-drift: literal re-derivation(s) of the canonical phase-number token found.\n'); process.stderr.write('Build the regex from phase-id.cjs `PHASE_NUMBER_TOKEN_SOURCE` (or phaseMarkdownRegexSource for a\n'); - process.stderr.write('known number), or sanction the site with a `// phase-id-owner: ` comment:\n'); + process.stderr.write('known number), or sanction the site with a dedicated `// phase-id-owner: `\n'); + process.stderr.write('comment on the line directly above the regex:\n'); for (const d of violations) { process.stderr.write(` ${d.file}:${d.line} ${d.found}\n`); } diff --git a/tests/phase-id-drift-guard.test.cjs b/tests/phase-id-drift-guard.test.cjs index 63858f23c..960483dac 100644 --- a/tests/phase-id-drift-guard.test.cjs +++ b/tests/phase-id-drift-guard.test.cjs @@ -66,29 +66,38 @@ describe('#2128 phase-id drift scanner: findPhaseIdRegexDrift (pure)', () => { 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', () => { + test('a dedicated preceding // phase-id-owner: comment line suppresses the flag', () => { assert.deepEqual( - findPhaseIdRegexDrift('const re = /(\\d+[A-Z]?(?:\\.\\d+)*)/; // phase-id-owner: sanctioned exception'), - [], - ); - }); - - test('a preceding-line // phase-id-owner: sanction suppresses the flag', () => { - assert.deepEqual( - findPhaseIdRegexDrift('// phase-id-owner: sanctioned exception\nconst re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'), + findPhaseIdRegexDrift(' // phase-id-owner: sanctioned exception\n const re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'), [], ); }); 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+)*)/;'), + findPhaseIdRegexDrift(' // phase-id-owner: sanctioned exception\n\n const 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+)*)/;'); + test('a trailing same-line // phase-id-owner: is NOT a sanction (must be a dedicated line above)', () => { + // The marker must lead its own comment line; a trailing comment on a code + // line is not honored, so the regex is still flagged. + const v = findPhaseIdRegexDrift('const re = /(\\d+[A-Z]?(?:\\.\\d+)*)/; // phase-id-owner: not honored here'); + assert.equal(v.length, 1); + }); + + test('a // phase-id-owner: embedded in a STRING literal does NOT suppress (decoy)', () => { + // A `//` inside a string is not a comment — help/doc text that quotes the + // sanction syntax must not silently suppress a real re-derivation. + const decoyLine = findPhaseIdRegexDrift('const help = "use // phase-id-owner: "; const re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'); + assert.equal(decoyLine.length, 1); + const decoyPrev = findPhaseIdRegexDrift('const help = "use // phase-id-owner: ";\nconst re = /(\\d+[A-Z]?(?:\\.\\d+)*)/;'); + assert.equal(decoyPrev.length, 1); + }); + + test('a bare "phase-id-owner:" substring with no // 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); });