From b5ef0d6c9db237ed7d9c2cbb992b13abad506bd2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" Date: Fri, 21 Aug 2026 15:06:43 +0000 Subject: [PATCH 1/2] fix(scripts): check-required-contexts reads comments as prose, not as wiring (#10818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `uncommentedYaml` dropped a line only when its first non-space character was `#`, so a TRAILING `# --verify-required-set` on a live line — and a trailing shell comment inside a `run:` block scalar — survived the strip and reddened `Lint & Repo Gates` on prose. A workflow stacks two comment grammars and one line filter was wrong about both. Each now goes to the thing that knows it: `yaml.parse` for the YAML layer, and check-shard-attestation's `shellCommands()` lexer — imported, not re-typed — for the shell inside each `run:`. Both limbs are pinned in `--self-test`: four prose shapes that must NOT read as wiring, and six live shapes that must. The trailing-comment pair differs by the `#` alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- scripts/check-required-contexts.mjs | 311 +++++++++++++++++++++++++++- 1 file changed, 301 insertions(+), 10 deletions(-) diff --git a/scripts/check-required-contexts.mjs b/scripts/check-required-contexts.mjs index 222404ead5..e9bea35075 100644 --- a/scripts/check-required-contexts.mjs +++ b/scripts/check-required-contexts.mjs @@ -181,6 +181,7 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { shellCommands } from './check-shard-attestation.mjs'; import { isEntrypoint } from './invoked-as.mjs'; /** @@ -573,6 +574,106 @@ function triggersOf(doc) { return block && typeof block === 'object' ? block : undefined; } +/** + * The flag whose WIRING the pin below judges, as one constant rather than four + * regex literals: the recognizer, the two absence assertions and the caller + * sweep all have to be asking about the SAME string. + */ +const LIVE_READ_FLAG = '--verify-required-set'; + +/** + * The text of a workflow that a RUNNER would ACT ON — every comment gone. + * + * ## Why this is not one regex + * + * It was one, and the regex was wrong: `text.split('\n').filter((l) => + * !/^\s*#/.test(l))` drops a line only when its FIRST non-space character is + * `#`. A workflow file stacks TWO comment grammars, and a line filter is wrong + * about both of them: + * + * - YAML's `#` opens a comment at line start OR after whitespace, and does + * NOT open one inside a quoted scalar — so a TRAILING `# ...` survived the + * filter whole, carrying its prose into the match; + * - a `run:` block scalar is not YAML at all, it is SHELL. There `#` opens a + * comment at a word boundary, `#` inside quotes is an argument, and a + * backslash-newline continues the command — none of which a per-line test + * can see. A trailing `# ...` on a live command line survived there too. + * + * So a comment WARNING people about `--verify-required-set` read as WIRING. + * That is not a hypothetical shape, it is the ordinary way anyone documents a + * trap, and the neighbouring `ci.yml` block already writes exactly such a + * comment for check-shard-attestation's sibling flag. Documenting the trap + * re-arms it — the #4890 shape, which is why check-shard-attestation's own + * `shellCommands()` drops comments and pins that it does. + * + * ## Each grammar goes to the thing that knows it + * + * `yaml.parse` answers the YAML layer: comments are not part of the parsed + * document, leading or trailing, while a `#` inside a quoted scalar stays in + * the string where it belongs. `shellCommands()` answers the shell layer inside + * each `run:` — check-shard-attestation's lexer, IMPORTED rather than re-typed. + * Two private comment strippers drifting apart in opposite directions is the + * exact history `js-comment-mask.mjs` exists to have ended, and a second + * hand-rolled one here would be the third family. + * + * ## Width is deliberate, and so is where it stops + * + * Outside a `run:` block this keeps the scalar WHOLE instead of judging what it + * would do, so the flag reaching a runner through an `env:` value, a `with:` + * input or a matrix entry still reads as wiring. That width is the safe error + * direction for an ABSENCE pin: a false red names a file and a line and is + * fixed in a minute, while a false green is a gate that quietly stopped + * guarding. It is also why this does NOT adopt check-shard-attestation's + * stricter `invokesScript()` adjacency test. That test earns its keep there + * because `--verify` and `--emit` are flags OTHER programs really take (`git + * rev-parse --verify` is what #6589 misread); `--verify-required-set` is spelled + * nowhere else in this tree, so adjacency would remove a false positive that + * cannot occur while introducing false negatives that can — the flag passed + * through a shell variable, or by a spelling of the invocation this file did not + * think to enumerate. + * + * @param {unknown} doc a parsed workflow document + * @returns {string} every scalar a runner acts on, joined, comments removed + */ +function liveWorkflowText(doc) { + const parts = []; + const visit = (node) => { + if (typeof node === 'string') { + parts.push(node); + return; + } + if (Array.isArray(node)) { + for (const item of node) visit(item); + return; + } + if (!node || typeof node !== 'object') return; + for (const [key, value] of Object.entries(node)) { + // Two different keys are spelled `run`. A step's is a COMMAND string and + // goes through the shell lexer; `defaults.run` is a mapping (`shell:`, + // `working-directory:`) and is ordinary structure. The typeof test is + // what tells them apart. + if (key === 'run' && typeof value === 'string') parts.push(...shellCommands(value)); + else visit(value); + } + }; + visit(doc); + return parts.join('\n'); +} + +/** + * Does this workflow WIRE UP the live required-set read? + * + * The question the two absence assertions and the caller sweep in `--self-test` + * all ask. A mention in a comment — YAML or shell, leading or trailing — is + * prose and answers `false`; a mention anywhere a runner would act on answers + * `true`. + * + * @param {unknown} doc a parsed workflow document + */ +function wiresLiveRead(doc) { + return liveWorkflowText(doc).includes(LIVE_READ_FLAG); +} + /** * Judge a registry against already-parsed workflows. * @@ -2132,10 +2233,9 @@ async function selfTest() { // the two-step. Wiring it is a maintainer decision, and it goes red HERE // first rather than silently in the queue. { - const uncommentedYaml = (text) => text.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n'); for (const [file, text] of Object.entries(sources)) { assert( - !/--verify-required-set/.test(uncommentedYaml(text)), + !wiresLiveRead(parse(text)), `wiring: ${file} must not RUN the live required-set read — report-only, off the required path (#9642)`, ); } @@ -2156,9 +2256,27 @@ async function selfTest() { // carries: a second caller appearing in some third workflow is exactly the // thing the absences above are guarding against, and they cannot see it. const workflowDir = join(root, '.github', 'workflows'); + // A document this sweep could not READ is not a document with nothing in + // it (#4690), and the recognizer now needs a parse to answer at all. So an + // unparseable workflow falls back to the widest possible reading — it can + // only ever OVER-report a caller, never hide one — and is named as its own + // failure rather than silently classified. + const unreadable = []; const callers = readdirSync(workflowDir) .filter((f) => f.endsWith('.yml') || f.endsWith('.yaml')) - .filter((f) => /--verify-required-set/.test(uncommentedYaml(readFileSync(join(workflowDir, f), 'utf8')))); + .filter((f) => { + const text = readFileSync(join(workflowDir, f), 'utf8'); + try { + return wiresLiveRead(parse(text)); + } catch (error) { + unreadable.push(`${f}: ${error.message}`); + return text.includes(LIVE_READ_FLAG); + } + }); + assert( + unreadable.length === 0, + `wiring: every .github/workflows document must PARSE before this sweep can classify it — ${JSON.stringify(unreadable)} did not (#4690)`, + ); assert( callers.join(',') === PATROL_WORKFLOW, `wiring: exactly one workflow runs the live read and it is ${PATROL_WORKFLOW} — found [${callers.join(', ')}] (#9678)`, @@ -2169,13 +2287,24 @@ async function selfTest() { // made this block throw mid-self-test, so the `callers` failure above was // recorded and never printed and every later assertion never ran — a stack // trace where the gate's own authored verdict belongs (#4690's family). The - // downstream cases each carry `patrolPresent` for the same reason: on an - // absent file `!/merge_group/` is vacuously true, which is a false green - // about a workflow that does not exist. + // downstream cases each carry `patrolReadable` for the same reason: on an + // absent or unparseable file "it declares no merge_group" is vacuously + // true, which is a false green about a workflow that does not exist. const patrolPath = join(workflowDir, PATROL_WORKFLOW); const patrolPresent = existsSync(patrolPath); assert(patrolPresent, `wiring: the standing caller .github/workflows/${PATROL_WORKFLOW} is missing — the live mode is wired nowhere again (#9678)`); - const patrolYaml = patrolPresent ? uncommentedYaml(readFileSync(patrolPath, 'utf8')) : ''; + let patrolDoc; + if (patrolPresent) { + try { + patrolDoc = parse(readFileSync(patrolPath, 'utf8')); + } catch { + // Named by the `unreadable` assertion above — the patrol lives in the + // swept directory — so this stays quiet and the cases below fail on + // `patrolReadable` instead of throwing mid-self-test. + patrolDoc = undefined; + } + } + const patrolReadable = patrolDoc !== undefined; // The mechanical proxy for "never required". Assertion 6 of this pin is // that every required context's workflow carries `merge_group:` — without // one, the queue build never produces the context and the whole queue @@ -2183,7 +2312,7 @@ async function selfTest() { // therefore CANNOT be validly required-ized, and required-izing it anyway // wedges the queue rather than deadlocking a rename two-step quietly. assert( - patrolPresent && !/^\s{0,4}merge_group\s*:/m.test(patrolYaml), + patrolReadable && triggersOf(patrolDoc) !== undefined && !Object.prototype.hasOwnProperty.call(triggersOf(patrolDoc), 'merge_group'), `wiring: ${PATROL_WORKFLOW} must declare no merge_group trigger — a workflow without one deadlocks the queue if it is ever required-ized (#9678)`, ); assert( @@ -2198,9 +2327,170 @@ async function selfTest() { // the same character. A rendering change costs the patrol a FALSE ALARM, // never a false all-clear — it annotates on the mark's ABSENCE. assert( - patrolPresent && patrolYaml.includes(REQUIRED_SET_CLEAN_MARK), + patrolReadable && liveWorkflowText(patrolDoc).includes(REQUIRED_SET_CLEAN_MARK), `wiring: ${PATROL_WORKFLOW} must key its drift annotation on the clean mark ${REQUIRED_SET_CLEAN_MARK} this file renders (#9678)`, ); + + // ── the recognizer itself, in BOTH directions ──────────────────────────── + // + // The three assertions above are only as good as `wiresLiveRead`, and what + // it replaced was a `/^\s*#/` line filter that dropped a comment only when + // its FIRST non-space character was `#`. Two ordinary shapes walked through + // it and reddened `Lint & Repo Gates` on prose: a TRAILING comment on an + // otherwise-live line, and a trailing SHELL comment inside a `run:` block + // scalar. Nothing in the tree spelled either one, so the gate was green and + // the trigger was someone WRITING a warning about this very flag — which is + // what the neighbouring `ci.yml` block already does for + // check-shard-attestation's sibling flag. That script pins its own + // `--verify`-in-a-comment fixture for exactly this reason; this is the same + // pin for this flag. + // + // ⭐ Both limbs, always. A comment-stripper that also swallowed the GENUINE + // mention would be a strictly worse defect than the false positive it fixes + // — the live read wired into a required job, and this gate green about it — + // so every "prose" case below is paired with the same text made LIVE and + // asserted as wiring. The trailing-comment pair differs by the `#` alone. + // + // The fixtures are workflow SOURCE and `wired()` is the whole pipeline + // under test — parse, then lex each `run:`, then look for the flag. One + // seam, so the recognizer can be swapped for the old line filter under + // reverse verification without touching a single fixture. + const wired = (source) => wiresLiveRead(parse(source)); + const workflowFixture = (...lines) => lines.join('\n'); + const stepFixture = (...runLines) => + workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - name: step', + ' run: |', + ...runLines.map((line) => ` ${line}`), + ); + + // (a) the reported defect: a trailing YAML comment on a live line. + assert( + !wired( + workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' steps:', + ` - name: step # never wire ${LIVE_READ_FLAG} into a required job`, + ' run: echo hi', + ), + ), + `recognizer: a TRAILING YAML comment naming ${LIVE_READ_FLAG} is prose, not wiring — the line filter this replaced kept the whole line`, + ); + // (b) the one shape the line filter did get right, pinned so it stays right. + assert( + !wired( + workflowFixture( + 'name: fixture', + `# ${LIVE_READ_FLAG} is report-only and must stay off the required path.`, + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - name: step', + ' run: echo hi', + ), + ), + 'recognizer: a whole-line YAML comment is prose', + ); + // (c)/(c′) the trailing SHELL comment inside a `run:` block scalar, and the + // same command with the `#` removed. One variable between them. + assert(!wired(stepFixture(`ls # ${LIVE_READ_FLAG}`)), 'recognizer: a TRAILING shell comment inside a run: block scalar is prose'); + assert( + wired(stepFixture(`ls ${LIVE_READ_FLAG}`)), + 'recognizer: the SAME command without the `#` is wiring — the pair differs by the comment marker alone', + ); + // (d) documenting the invocation inside a `run:` — the #4890 shape. + assert( + !wired(stepFixture(`# node scripts/check-required-contexts.mjs ${LIVE_READ_FLAG} would wire this job up`, 'echo documented')), + 'recognizer: DOCUMENTING the invocation inside a run: block is not making one (the #4890 shape)', + ); + // (e) the genuine article, spelled the way the patrol spells it: the flag + // before a backslash continuation, inside a multi-command block. + assert( + wired(stepFixture('set +e', `node scripts/check-required-contexts.mjs ${LIVE_READ_FLAG} \\`, ' > "$RUNNER_TEMP/required-set.md"')), + 'recognizer: the patrol’s own spelling — the flag ahead of a backslash continuation — is wiring', + ); + // (f) a quoted `#` is an ARGUMENT. Truncating a command there would hide + // real wiring behind a plausible-looking argument, which is the one outcome + // worse than the false positive this whole block is about. + assert( + wired(stepFixture(`node scripts/check-required-contexts.mjs --label 'a # b' ${LIVE_READ_FLAG}`)), + 'recognizer: a QUOTED `#` inside a run: command is an argument, not a comment', + ); + // (g) the same property one grammar up: `#` inside a quoted YAML scalar + // does not open a YAML comment either. + assert( + wired( + workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' steps:', + ` - name: "a # ${LIVE_READ_FLAG}"`, + ' run: echo hi', + ), + ), + 'recognizer: a `#` inside a QUOTED YAML scalar does not open a comment — the mention is live text', + ); + // (h) the width outside `run:`, stated as a pin rather than left to the + // docblock: a flag reaching the runner through an `env:` value is wiring, + // even though no single command spells it. + assert( + wired( + workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' env:', + ` FLAG: ${LIVE_READ_FLAG}`, + ' steps:', + ' - name: step', + ' run: node scripts/check-required-contexts.mjs $FLAG', + ), + ), + 'recognizer: the flag reaching a runner through an env: value is wiring — outside a run: block the scalar is kept whole', + ); + // (i) `defaults.run` is a MAPPING, not a command. Handing it to the shell + // lexer would drop its keys and could only ever narrow the reading. + assert( + wired( + workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' defaults:', + ' run:', + ` working-directory: ${LIVE_READ_FLAG}`, + ' steps:', + ' - name: step', + ' run: echo hi', + ), + ), + 'recognizer: `defaults.run` is structure, not a command — its scalars are still read', + ); } { @@ -2263,7 +2553,8 @@ async function selfTest() { `✓ check-required-contexts --self-test: ${checked} assertions ` + `(rename ablations across both workflows + matrix/continue-on-error/trigger shapes + the shard-name collision + ` + `the \`carries\` step-count ban + the instruction-surface stale-name scan (#9491) + ` + - `the live required-set diff and its off-the-required-path wiring (#9642) + the #4690 pins).`, + `the live required-set diff and its off-the-required-path wiring (#9642), with the comment-vs-code recognizer ` + + `pinned in both directions + the #4690 pins).`, ); } From 957ea2aa39a99e0be617e0e190f725beeaf7ef8e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 15:41:47 +0000 Subject: [PATCH 2/2] fix(scripts): the required-context self-test proves WIRING with invocations, not prose (#10877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two presence assertions in `--self-test` decided "is this wired up?" from text a comment, a label or an `echo` could supply. Both error toward a silent GREEN — the gate reporting that a required-context pin runs when it does not. - the `lint` job block carried a second `/^\s*#/` line filter and matched `/run: pnpm check:required-contexts\b/` over a raw text slice. It is now a structural read of `jobs.lint.steps`. The slice was also bounded by `'\n typecheck:'`, which is not the job after `lint:` — measured, it covered five jobs. - the caller sweep's single equality was two assertions in opposite directions sharing one WIDE recognizer. Split: the absence half keeps `wiresLiveRead`, the presence half gets `invokesLiveRead`. The two recognizers are deliberately different widths and are pinned together, including one case where they disagree by design. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- scripts/check-required-contexts.mjs | 457 ++++++++++++++++++++++++++-- 1 file changed, 430 insertions(+), 27 deletions(-) diff --git a/scripts/check-required-contexts.mjs b/scripts/check-required-contexts.mjs index e9bea35075..2b564f35a6 100644 --- a/scripts/check-required-contexts.mjs +++ b/scripts/check-required-contexts.mjs @@ -637,27 +637,159 @@ const LIVE_READ_FLAG = '--verify-required-set'; */ function liveWorkflowText(doc) { const parts = []; - const visit = (node) => { - if (typeof node === 'string') { - parts.push(node); - return; + visitLive(doc, { onScalar: (scalar) => parts.push(scalar), onCommand: (command) => parts.push(command) }); + return parts.join('\n'); +} + +/** + * Walk a workflow document the way a RUNNER reads it, once. + * + * Two questions in this file need this same walk and need DIFFERENT answers out + * of it (see `invokesLiveRead` for why the two must not be merged), so the + * traversal is shared and the reading is not: + * + * - `onScalar` receives every string a runner would act on — an `env:` value, + * a `with:` input, a matrix entry, a step `name:`; + * - `onCommand` receives every shell COMMAND inside a step's `run:`, lexed by + * `shellCommands()` so comments and separators are already gone. + * + * Sharing the walk is the point: a `run:` key that one reading understood and + * the other did not is precisely the drift that gave this file two `/^\s*#/` + * filters answering in opposite directions in the first place. + * + * @param {unknown} node a parsed workflow document, or any node of one + * @param {{ onScalar?: (scalar: string) => void, onCommand?: (command: string) => void }} visitors + */ +function visitLive(node, { onScalar = () => {}, onCommand = () => {} } = {}) { + if (typeof node === 'string') { + onScalar(node); + return; + } + if (Array.isArray(node)) { + for (const item of node) visitLive(item, { onScalar, onCommand }); + return; + } + if (!node || typeof node !== 'object') return; + for (const [key, value] of Object.entries(node)) { + // Two different keys are spelled `run`. A step's is a COMMAND string and + // goes through the shell lexer; `defaults.run` is a mapping (`shell:`, + // `working-directory:`) and is ordinary structure. The typeof test is + // what tells them apart. + if (key === 'run' && typeof value === 'string') for (const command of shellCommands(value)) onCommand(command); + else visitLive(value, { onScalar, onCommand }); + } +} + +/** + * Every shell command the steps under `node` would actually EXECUTE. + * + * The `onCommand` half of the walk on its own — scalars deliberately dropped. + * A step `name:` that spells an invocation is a LABEL, and the whole point of + * the narrow readings below is that a label is not a command. + * + * @param {unknown} node a parsed workflow document, a job, or a step + * @returns {string[]} the commands, in order, comments already removed + */ +function runCommands(node) { + const commands = []; + visitLive(node, { onCommand: (command) => commands.push(command) }); + return commands; +} + +/** + * One shell command, split into words, each marked as QUOTED or not. + * + * ⚠️ Not a third comment stripper — it handles no `#` at all. It runs on + * `shellCommands()` OUTPUT, where comments are already gone, and recovers the + * one thing that function does not expose: where the words are, and which of + * them came out of a quoted region. Shell word-splitting does not happen inside + * quotes, so `echo "pnpm check:required-contexts is below"` is TWO words — the + * program `echo`, and one quoted blob — and no amount of splitting on + * whitespace will make the blob look like a program being run. + * + * That distinction is the whole narrow recognizer. `invokesScript()` in + * check-shard-attestation splits on `/\s+/` and strips the quotes off each + * piece, which is right for the question IT asks and cannot answer this one: + * it would read the `echo` above as running `pnpm`. + * + * @param {string} command one command from `shellCommands()` + * @returns {{ word: string, quoted: boolean }[]} + */ +function commandWords(command) { + const words = []; + let word = ''; + let quoted = false; + let started = false; + let quote = ''; + const flush = () => { + if (started) words.push({ word, quoted }); + word = ''; + quoted = false; + started = false; + }; + for (let i = 0; i < command.length; i += 1) { + const ch = command[i]; + if (quote) { + // Inside double quotes a backslash escapes the next character; inside + // single quotes it is literal. Either way the result stays in this word. + if (ch === '\\' && quote === '"' && i + 1 < command.length) { + word += command[i + 1]; + i += 1; + continue; + } + if (ch === quote) { + quote = ''; + continue; + } + word += ch; + continue; } - if (Array.isArray(node)) { - for (const item of node) visit(item); - return; + if (ch === '\\' && i + 1 < command.length) { + word += command[i + 1]; + i += 1; + started = true; + continue; } - if (!node || typeof node !== 'object') return; - for (const [key, value] of Object.entries(node)) { - // Two different keys are spelled `run`. A step's is a COMMAND string and - // goes through the shell lexer; `defaults.run` is a mapping (`shell:`, - // `working-directory:`) and is ordinary structure. The typeof test is - // what tells them apart. - if (key === 'run' && typeof value === 'string') parts.push(...shellCommands(value)); - else visit(value); + if (ch === "'" || ch === '"') { + quote = ch; + quoted = true; + started = true; + continue; } - }; - visit(doc); - return parts.join('\n'); + if (ch === ' ' || ch === '\t') { + flush(); + continue; + } + word += ch; + started = true; + } + flush(); + return words; +} + +/** + * Does this command RUN `program`, with `arg` among the arguments it passes? + * + * Adjacency, as check-shard-attestation's `invokesScript()` reads it (#6589): + * the argument must belong to a command that runs the program, not merely + * appear near it. Plus one condition that function does not impose — the + * PROGRAM word must be UNQUOTED. A quoted word is a string being passed to + * something else, and the something else is usually `echo`. + * + * Arguments may be quoted freely: `--verify-required-set` and + * `"--verify-required-set"` are the same argument to the same invocation, and + * refusing the quoted spelling would narrow a PRESENCE assertion into a false + * red — the failure mode that gets a pin loosened rather than fixed. + * + * @param {string} command one command from `shellCommands()` + * @param {string} program an executable or script basename, e.g. `pnpm` + * @param {string} arg the argument that makes it THE invocation + */ +function invokes(command, program, arg) { + const words = commandWords(command); + const at = words.findIndex(({ word, quoted }) => !quoted && (word === program || word.endsWith(`/${program}`))); + if (at === -1) return false; + return words.slice(at + 1).some(({ word }) => word === arg || word.startsWith(`${arg}=`)); } /** @@ -674,6 +806,82 @@ function wiresLiveRead(doc) { return liveWorkflowText(doc).includes(LIVE_READ_FLAG); } +/** + * The script this file is, as its own basename — the thing an invocation of the + * live read has to actually name. + */ +const LIVE_READ_SCRIPT = 'check-required-contexts.mjs'; + +/** + * The package script that carries the pin into `Lint & Repo Gates`. + */ +const PIN_SCRIPT = 'check:required-contexts'; + +/** + * Does this workflow actually INVOKE the live required-set read? + * + * ## ⭐ Why this is not `wiresLiveRead`, and must not become it + * + * These two ask about the same flag and are deliberately DIFFERENT WIDTHS, + * because they are consumed in opposite directions and a recognizer's error + * direction is only safe with respect to one of them: + * + * | question | recognizer | a mention in live prose costs | + * | --- | --- | --- | + * | may this workflow wire the read? (ABSENCE) | `wiresLiveRead`, WIDE | a false RED — loud, names a file, fixed in a minute | + * | does the patrol still run it? (PRESENCE) | `invokesLiveRead`, NARROW | a false GREEN — the patrol reads nothing and this gate says it does | + * + * So width is not a quality one of them has more of. For the absence half, + * reading an `echo` as wiring is the SAFE mistake, which is why + * `wiresLiveRead` keeps whole scalars (#9642's `env:`/`with:`/matrix routes) + * and why widening it further would still be defensible. For the presence + * half it is the ONLY mistake that matters: anything that widens what counts + * as "present" makes it easier for prose to satisfy the assertion, and the + * symptom is a green line over a patrol that stopped reading. + * + * ⛔ Do not tidy these onto one recognizer. One recognizer serving both + * directions is what this file already had — a `/^\s*#/` line filter, written + * twice, feeding an absence assertion and a presence assertion — and it was + * wrong in both directions at once for exactly this reason. Merging them again + * needs an argument that the merged width is correct for BOTH, not that it is + * tidier. The shared TRAVERSAL (`visitLive`) is the part that should be shared; + * the reading is the part that must not be. + * + * @param {unknown} node a parsed workflow document + */ +function invokesLiveRead(node) { + return runCommands(node).some((command) => invokes(command, LIVE_READ_SCRIPT, LIVE_READ_FLAG)); +} + +/** + * The steps of one job that INVOKE the required-context pin. + * + * Structure, not text shapes. What this replaced sliced lint.yml between + * `'\n lint:'` and `'\n typecheck:'` and matched `/run: pnpm + * check:required-contexts\b/` over the slice, which was wrong three ways at + * once and all three were false GREENS on the wiring of a required context: + * + * - it kept trailing comments (the `/^\s*#/` filter drops a line only when + * its FIRST non-space character is `#`), so `- uses: actions/checkout@v7 # + * run: pnpm check:required-contexts is below` satisfied it; + * - it matched any live text, so a step `name:` naming the invocation + * satisfied it too — prose that survives even a correct comment stripper; + * - `'\n typecheck:'` is not the job after `lint:`. Measured on the + * checked-in file: the slice ran from `lint` through `typecheck-consumers`, + * FIVE jobs, so the pin could migrate out of the required job entirely and + * this assertion would not notice. + * + * Reading `jobs..steps` closes all three, because none of them is a fact + * about the job at all. + * + * @param {unknown} job a parsed job + * @returns {unknown[]} the steps that run the pin + */ +function pinSteps(job) { + const steps = job && typeof job === 'object' && Array.isArray(job.steps) ? job.steps : []; + return steps.filter((step) => runCommands(step).some((command) => invokes(command, 'pnpm', PIN_SCRIPT))); +} + /** * Judge a registry against already-parsed workflows. * @@ -2277,9 +2485,27 @@ async function selfTest() { unreadable.length === 0, `wiring: every .github/workflows document must PARSE before this sweep can classify it — ${JSON.stringify(unreadable)} did not (#4690)`, ); + // ⭐ This was ONE equality — `callers.join(',') === PATROL_WORKFLOW` — and + // one equality is the wrong shape, because it is two assertions pointing in + // opposite directions sharing a single WIDE recognizer: + // + // - "no OTHER workflow wires the read" is an ABSENCE. Wide is correct: + // over-reporting a caller costs a false red that names the file. + // - "…and the patrol is one of them" is a PRESENCE. Wide is WRONG, and + // measured on the checked-in patrol: of its 7 mentions of the flag, 4 + // are whole-line YAML comments and 3 survive as live text — the genuine + // `node scripts/check-required-contexts.mjs --verify-required-set`, and + // TWO `echo` steps that only report on its exit code. Delete the genuine + // invocation and either `echo` keeps the patrol classified as the + // standing caller, so this half stays green over a patrol that reads + // nothing (#4690). + // + // So the halves are separated and each gets the recognizer its direction + // needs. See `invokesLiveRead` for why they must not be merged back. + const strangers = callers.filter((f) => f !== PATROL_WORKFLOW); assert( - callers.join(',') === PATROL_WORKFLOW, - `wiring: exactly one workflow runs the live read and it is ${PATROL_WORKFLOW} — found [${callers.join(', ')}] (#9678)`, + strangers.length === 0, + `wiring: no workflow other than ${PATROL_WORKFLOW} may wire the live read — found [${strangers.join(', ')}] (#9642/#9678)`, ); // Read defensively and turn "missing" into a NAMED assertion rather than an @@ -2305,6 +2531,18 @@ async function selfTest() { } } const patrolReadable = patrolDoc !== undefined; + // ⭐ The PRESENCE half of the sweep above, on the narrow recognizer its + // direction needs: the patrol must INVOKE the live read, not merely name + // the flag somewhere a runner would act on. `wiresLiveRead` cannot answer + // this — its width is deliberate and correct for the absence half, and it + // reads the patrol's own `echo "::error::… --verify-required-set …"` as a + // caller. This is the assertion that #9678 actually wanted: the mode's + // whole first life was a sweep whose only scheduled caller was its own + // offline self-test, and an `echo` about the flag is that same nothing. + assert( + patrolReadable && invokesLiveRead(patrolDoc), + `wiring: ${PATROL_WORKFLOW} must INVOKE the live read (\`${LIVE_READ_SCRIPT} ${LIVE_READ_FLAG}\`), not merely mention the flag — a step that only reports on it leaves the mode wired nowhere (#9678/#4690)`, + ); // The mechanical proxy for "never required". Assertion 6 of this pin is // that every required context's workflow carries `merge_group:` — without // one, the queue build never produces the context and the whole queue @@ -2491,22 +2729,187 @@ async function selfTest() { ), 'recognizer: `defaults.run` is structure, not a command — its scalars are still read', ); + + // ── …and the NARROW recognizer beside it, in both directions ───────────── + // + // ⭐ `invokesLiveRead` answers the PRESENCE half of the caller sweep, and + // the two recognizers are pinned TOGETHER here because the point is that + // they deliberately disagree. Reading these cases as "one of them is wrong" + // is the mistake this block exists to prevent; see `invokesLiveRead`. + const invoked = (source) => invokesLiveRead(parse(source)); + + // (j′) the measured defect: the patrol's own `echo` about the flag. Spelled + // WITH the `.mjs` basename, so it would also satisfy a plain adjacency test + // — it is the quoting that makes it prose, not the spelling. + const echoOnly = stepFixture(`echo "${LIVE_READ_SCRIPT} ${LIVE_READ_FLAG} exited $code"`); + assert( + !invoked(echoOnly), + `recognizer: a step that only ECHOES ${LIVE_READ_FLAG} does not INVOKE it — the presence half must not go green on a patrol that reads nothing (#10877)`, + ); + assert( + wired(echoOnly), + 'recognizer: …and the WIDE reading still calls that same echo wiring — the absence half wants it that way, which is exactly why one recognizer cannot serve both (#10877)', + ); + // (k′) the genuine article, spelled the way the patrol spells it: the flag + // ahead of a backslash continuation, redirected. + assert( + invoked(stepFixture(`node scripts/${LIVE_READ_SCRIPT} ${LIVE_READ_FLAG} \\`, ' > "$RUNNER_TEMP/required-set.md"')), + 'recognizer: the patrol’s own spelling IS an invocation', + ); + // (l′) a QUOTED argument to a genuine invocation is still that invocation. + // Refusing it would narrow a presence assertion into a false red. + assert( + invoked(stepFixture(`node scripts/${LIVE_READ_SCRIPT} "${LIVE_READ_FLAG}"`)), + 'recognizer: quoting the ARGUMENT does not un-invoke the script — only the PROGRAM word has to be unquoted', + ); + // (m′) ⭐ the disagreement stated as a fact. A flag reaching the runner + // through `env:` is wiring for the absence half (pinned as (h) above) and + // is NOT an invocation for the presence half — no command names the script + // with the flag. That asymmetry is the cost of the narrow half, and it is + // the LOUD direction: if the patrol is ever rewritten to pass the flag this + // way, this assertion reddens and names itself rather than going quiet. + const viaEnv = workflowFixture( + 'name: fixture', + 'on:', + ' push: {}', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' env:', + ` FLAG: ${LIVE_READ_FLAG}`, + ' steps:', + ' - name: step', + ` run: node scripts/${LIVE_READ_SCRIPT} $FLAG`, + ); + assert(wired(viaEnv), 'recognizer: the env: route is WIRING for the absence half (restating (h) from the other side)'); + assert( + !invoked(viaEnv), + 'recognizer: …and is NOT an invocation for the presence half — the two widths disagree BY DESIGN, and this is the pin that says so (#10877)', + ); } { - const uncommented = (text) => text.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n'); - const lintJobStart = sources['lint.yml'].indexOf('\n lint:'); - const lintJobEnd = sources['lint.yml'].indexOf('\n typecheck:'); - const lintJob = uncommented(lintJobStart === -1 ? '' : sources['lint.yml'].slice(lintJobStart, lintJobEnd === -1 ? undefined : lintJobEnd)); + // ── is the pin WIRED into the required job? ────────────────────────────── + // + // A PRESENCE assertion about a required context's own wiring, which makes + // its error direction a silent GREEN: whatever satisfies it wrongly leaves + // this gate reporting that the pin runs while it does not. `pinSteps` reads + // `jobs.lint.steps` and asks each step's `run:` whether it INVOKES the + // script; the three text shapes that used to satisfy this are catalogued + // there and pinned below in both directions. + const lintDoc = parse(sources['lint.yml']); + const lintJob = lintDoc?.jobs?.lint; + // #4690: "no readable `lint` job" must be a NAMED failure, not a zero that + // reads like a clean answer. Every assertion after this one degrades to + // vacuously-something on an empty step list. + assert( + lintJob !== undefined && lintJob !== null && typeof lintJob === 'object', + 'wiring: lint.yml must declare a readable `lint` job — this pin cannot say anything about a job it could not read (#4690)', + ); + /** The seam the fixtures below pin: lint.yml SOURCE in, "is the pin wired" out. */ + const pinWiredIn = (source) => pinSteps(parse(source)?.jobs?.lint).length > 0; assert( - /run: pnpm check:required-contexts\b/.test(lintJob), + pinWiredIn(sources['lint.yml']), 'wiring: lint.yml\'s `lint` job (the "Lint & Repo Gates" context) must run `pnpm check:required-contexts` — an unwired pin verifies nothing (#4690)', ); - const step = lintJob.split(/\n(?= - name: )/).find((s) => /run: pnpm check:required-contexts\b/.test(s)) ?? ''; + // Unchanged in intent (#10877 left this one alone deliberately: it is an + // ABSENCE assertion and was never satisfiable by prose), but it consumed + // the text slice above, so it is re-homed onto the same structural read. + // `every` over the steps rather than the first match: two steps running the + // pin and only one of them conditional is still a way to arrange that a PR + // does not get pinned. Non-vacuous because the presence assertion above + // establishes there is at least one. assert( - !/^\s*if:/m.test(step), + pinSteps(lintJob).every((step) => !Object.prototype.hasOwnProperty.call(step, 'if')), 'wiring: the required-context pin step must carry NO `if:` — whatever a condition reads is a way for a PR to arrange that this pin does not run on it', ); + + // ── the wiring recognizer, in BOTH directions ──────────────────────────── + // + // ⭐ Both limbs, always. A presence test tightened until nothing satisfies + // it is a WORSE defect than the prose it was tightened to exclude: it shows + // up as a permanently red self-test, and the thing a permanently red + // self-test gets is loosened. So every "prose" case below is paired with the + // same wiring made REAL and asserted as wiring, and the checked-in lint.yml + // is asserted above as the sixth live limb. + const lintFixture = (...jobLines) => ['name: Lint', 'on:', ' push: {}', 'jobs:', ...jobLines].join('\n'); + const lintJobFixture = (...stepLines) => lintFixture(' lint:', ' runs-on: ubuntu-latest', ' steps:', ...stepLines); + + // (j) the reported defect: a TRAILING YAML comment supplying the wiring + // text. The `/^\s*#/` filter this replaced drops a line only when its FIRST + // non-space character is `#`, so this line survived whole and satisfied the + // presence regex — a green line over a job that runs no pin at all. + assert( + !pinWiredIn(lintJobFixture(` - name: checkout # run: pnpm ${PIN_SCRIPT} is below`, ' uses: actions/checkout@v7')), + `wiring: a TRAILING YAML comment naming \`run: pnpm ${PIN_SCRIPT}\` is prose, not wiring (#10877)`, + ); + // (k) prose that survives even a CORRECT comment stripper: the invocation + // spelled in a step `name:`. A label is not a command, and this is why the + // recognizer reads `run:` blocks rather than the job's live text. + assert( + !pinWiredIn(lintJobFixture(` - name: "run: pnpm ${PIN_SCRIPT}"`, ' run: echo hi')), + `wiring: the pin spelled in a step \`name:\` is a LABEL, not an invocation (#10877)`, + ); + // (l) the same thing one grammar down: a quoted string being handed to + // `echo`. Splitting the command on whitespace and stripping quotes off each + // piece — which is what the sibling's `invokesScript()` does — would read + // this as running `pnpm`, so `commandWords` tracks quoting instead. + assert( + !pinWiredIn( + lintJobFixture(' - name: talk about it', ' run: |', ` echo "run: pnpm ${PIN_SCRIPT} is wired below"`), + ), + `wiring: an \`echo\` QUOTING the invocation is prose — the quoted blob is one argument, not a program being run (#10877)`, + ); + // (p) a TRAILING SHELL comment inside a `run:` block scalar. `#` opens a + // shell comment at a word boundary, which no per-line YAML test can see. + assert( + !pinWiredIn( + lintJobFixture(' - name: gates', ' run: |', ` echo x # run: pnpm ${PIN_SCRIPT} is elsewhere`), + ), + `wiring: a TRAILING SHELL comment inside a run: block is prose (#10877)`, + ); + // (m) ⭐ genuinely wired — into the WRONG JOB. The text slice this replaced + // ran from `'\n lint:'` to `'\n typecheck:'`, and `typecheck-debt:` does + // not match `typecheck:`, so on the checked-in file the slice covered FIVE + // jobs. The pin could migrate out of the required job and this assertion + // would not notice — a false green about which job publishes the context. + assert( + !pinWiredIn( + lintFixture( + ' lint:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - name: something else', + ' run: pnpm lint', + ' typecheck-debt:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - name: Required-context name pin', + ` run: pnpm ${PIN_SCRIPT}`, + ' typecheck:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - name: tsc', + ' run: pnpm typecheck', + ), + ), + `wiring: the pin running in a job that is NOT \`lint\` does not wire \`Lint & Repo Gates\` (#10877)`, + ); + // (n) the genuine article, spelled the way lint.yml spells it. + assert( + pinWiredIn(lintJobFixture(' - name: Required-context name pin', ` run: pnpm ${PIN_SCRIPT}`)), + `wiring: the real spelling \`run: pnpm ${PIN_SCRIPT}\` IS wiring`, + ); + // (o) ⭐ the live limb the OLD matcher got wrong in the loud direction: a + // block scalar. `/run: pnpm check:required-contexts\b/` needs that literal + // run of characters, which a `run: |` block never contains — so genuinely + // wiring the pin as one of several commands would have reddened this gate. + // A presence pin that rejects a legitimate spelling is how a pin gets + // loosened, so it is pinned as wiring rather than left to the docblock. + assert( + pinWiredIn(lintJobFixture(' - name: gates', ' run: |', ' pnpm check:nul-bytes', ` pnpm ${PIN_SCRIPT}`)), + `wiring: the pin as one command of a \`run: |\` block IS wiring — the matcher this replaced could not see it`, + ); const pkg = JSON.parse(readFileSync(join(root, 'package.json'), 'utf8')); const wiring = pkg.scripts?.['check:required-contexts'] ?? ''; assert(/check-required-contexts\.mjs --self-test/.test(wiring), 'wiring: `check:required-contexts` must run this file\'s --self-test first');