Skip to content

[finding] check-required-contexts' uncommentedYaml strips only WHOLE-LINE comments, so a trailing # --verify-required-set in any workflow reds the gate #10818

Description

@claude

Noticed while implementing #10713 (PR #10817), which corrects a ci.yml comment that names
a gate's flag. That edit is safe because check-shard-attestation.mjs drops shell comments
with a real lexer. The sibling guard in scripts/check-required-contexts.mjs does not.

What it is

scripts/check-required-contexts.mjs (in the "the live mode stays OFF the required path"
block, #9642/#9678) strips comments before testing workflow text with:

constuncommentedYaml=(text)=>text.split('\n').filter((l)=>!/^\s*#/.test(l)).join('\n');

That drops a line only when its first non-space character is #. A trailing comment on
an otherwise-live line survives, and so does a shell comment inside a run: block scalar —
which is the ordinary way anyone would write a warning about this very flag:

false <- flag still seen after stripping | whole-line comment (stripped, correct)
true <- flag still seen after stripping | TRAILING comment (survives the filter)
true <- flag still seen after stripping | block-scalar shell comment (survives)

Two assertions read that stripped text, and the surviving mention misfires both:

  • the absence assertion — !/--verify-required-set/.test(uncommentedYaml(text)) — fails with
    "must not RUN the live required-set read", naming a comment that runs nothing;
  • the presence assertion sweeping .github/workflows collects that file into callers, so
    callers.join(',') === PATROL_WORKFLOW fails and reports a second caller that does not exist.

Both are false positives on prose, and both land on Lint & Repo Gates.

Why it is worth recording

Nothing is red today — no workflow currently spells the flag in a comment. It is latent, and
the trigger is writing a comment warning people about the flag, which is exactly what the
neighbouring ci.yml block does for the shard classifier's flag today.

The contrast is the useful part: the same repo already solved this properly one file over.
check-shard-attestation.mjs's shellCommands() is a small lexer that handles trailing
comments, quoting and continuations, and its docblock records why — documenting the trap
inside a run: re-armed it, "the #4890 shape". Its --self-test pins a trailing-comment
fixture. check-required-contexts.mjs guards a comparable "this flag must not be wired up"
property with a whole-line regex instead.

This is the same family as the open comment-vs-code findings #10570, #10683, #10794 and
#10514 — a recognizer reading prose as code — but a distinct site; none of those name this
function.

Suggested scope

Narrow the recognizer in scripts/check-required-contexts.mjs so a mention inside a comment
(trailing or block-scalar) is not read as wiring, and pin the trailing-comment case in that
script's self-test the way check-shard-attestation.mjs pins its own. Reusing the existing
lexer rather than growing a second regex is the obvious route, but which shared helper is
right is a judgment call for whoever picks this up. Not fixed in PR #10817: different file,
different gate, and #10713 was explicitly comment-only.

Verified against origin/main at f4e5d916d6.

Generated by Claude Code


Generated by Claude Code

Metadata

Metadata

Assignees

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions