Skip to content

fix(scripts): check-required-contexts reads comments as prose, not as wiring (#10818)#10878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
os-zhuang merged 1 commit into from
Aug 21, 2026
Merged

fix(scripts): check-required-contexts reads comments as prose, not as wiring (#10818) #10878

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
311 changes: 301 additions & 10 deletions scripts/check-required-contexts.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';

/**
Expand DownExpand Up@@ -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.
*
Expand DownExpand Up@@ -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)`,
);
}
Expand All@@ -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)`,
Expand All@@ -2169,21 +2287,32 @@ 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
// stalls waiting for it. A patrol that declares no merge_group trigger
// 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(
Expand All@@ -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',
);
}

{
Expand DownExpand Up@@ -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).`,
);
}

Expand Down
Loading