mirror of
https://github.com/Comfy-Org/github-workflows.git
synced 2026-11-05 00:17:37 +00:00
[PR #228] [MERGED] fix(workflow-pins): strip comments before classifying a ref-checkout line #235
Labels
No labels
cursor-review-catalog-drift
groom
groom
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Comfy-Org/github-workflows#235
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
📋 Pull Request Information
Original PR: https://github.com/Comfy-Org/github-workflows/pull/228
Author: @mattmillerai
Created: 8/26/2026
Status: ✅ Merged
Merged: 8/26/2026
Merged by: @mattmillerai
Base:
main← Head:matt/be-9129-workflow-pins-strip-comment📝 Commits (6)
cc49579fix(workflow-pins): classify ref-checkout lines on comment-stripped text29585bedocs(workflow-pins): note that classification ignores#commentsd665bfefix(workflow-pins): only strip a#where it really opens a comment (BE-9129)1e8a5a3fix(workflow-pins): open the ref: window for a quoted scalar split after its opening text (BE-9129)f43738efix(workflow-pins): carry quote state across lines and open every multi-line ref: window (BE-9129)7a3f802fix(workflow-pins): bound the carried quote state and judge every split ref: on its fold (BE-9129)📊 Changes
3 files changed (+1045 additions, -38 deletions)
View changed files
📝
.github/workflow-pins/README.md(+74 -0)📝
.github/workflow-pins/check_workflow_pins.py(+370 -38)📝
.github/workflow-pins/tests/test_check_workflow_pins.py(+601 -0)📄 Description
ELI-5
The workflow-pin lint reads each line of a workflow file and decides what it is:
a
WORKFLOWS_REFbinding, a checkout at the input, or a checkout at a step'soutput. It was making that decision on the raw line — comment and all. So if
you wrote a perfectly correct,
if:-guarded checkout and added a helpful noteafter it:
…the
.*in_REF_USE_BLOCK_REswallowed the comment, the line looked like adirect checkout at the input, and CI failed a compliant workflow. The same bug
runs the other way: a comment that merely spells out
{WORKFLOWS_REF: "${{ inputs.workflows_ref }}"}registered a resolver step that does not exist.The fix is to classify on the comment-stripped text. A comment is prose; it
configures nothing, so it now changes no verdict.
What changed
ref_checkoutsin.github/workflow-pins/check_workflow_pins.py, three callsites:
code = _strip_comment(line)—_GUARD_BINDING_RE,is_ref_use, and_GUARD_BINDING_FLOW_REall takecode.ref:continuation path takesmention_re.search(_strip_comment(line))._indent,_REF_KEY_OPEN_RE,_steps_output_sites,_pins_to_job_workflow_sha,_leading_operand_reaches_input,_binding_step_id,is_guard_step— because_strip_commentalso drops leading indentation, and those readers judge bycolumn. (
_pins_to_job_workflow_shaand_leading_operand_reaches_inputalready strip internally, so they were already consistent.)
is_ref_useitself is untouched. Plus a README paragraph documenting the rule,and 7 tests.
Verification
The 4 bug directions, before → after:
ref:+ explanatory comment(False, False, False), 1 unguarded(False, True, True), 0 unguarded(False, False, False), 1 unguarded(False, True, True), 0 unguarded[(…, False, False, True)][]ref: main # {WORKFLOWS_REF: …}[(…, False, False, False)][]Fail-closed directions are pinned, not assumed. Three of the seven new tests
pass both before and after by design — they exist to prove the strip costs
nothing:
env:binding carrying a trailing# boundstill guards its job;ref: ${{ inputs.workflows_ref }} # noteis still reported,in both the same-line and continuation spellings;
run: |body line (echo "ref: ${{ … }} # PR #${n}") records no sitebefore or after — pinning the over-protection
_strip_comment's own docstringdocuments.
Removing the source fix and re-running: exactly 4 of the 7 new tests fail —
the 4 bug directions — and the 3 fail-closed pins stay green. Tests that pass in
both states would have been worthless here, so this was checked rather than
inferred.
Corpus sweep. Ran
ref_checkoutsover all 48 workflow files in.github/workflows/, pre-fix and post-fix, and diffed the full per-file sitetuples: byte-identical. 26 judged ref-checkout sites across 11 workflows,
all guarded, both ways. Also swept the tree for the bug shape itself —
0
ref:lines carry a trailing comment, 0WORKFLOWS_REFbindings do, and the1 line mentioning
inputs.workflows_refinside a comment is ordinary prose in arun:body that records nothing. So this is a latent false red today: itfires the first time someone documents a
ref:line, which the README arguablyinvites.
Adversarial probe battery. Because the risk here is fail-open — a strip
hiding a real checkout — I ran 21 hand-built shapes through both versions and
diffed: unguarded block/flow/continuation/fallback checkouts, a leading-falsey
||operand, a dangling step output, anUNPARSEDvalue, a flow mapping with anapostrophe in a sibling scalar, a
curl -d '{"ref": …}'line in arun:body,the quoted-scalar decoy, and
env_aliaseson a commented-out alias. Nodifference on any of them, and 19 of the 21 record a real site, so the diff is
not vacuous.
Commands run:
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'— 279 passed, 0 failed (272 before, +7)python3 .github/workflow-pins/check_workflow_pins.py—OK — 11 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt)test-workflow-pins.ymlCI smoke tests, run locally: reintroduced-default still rejected, unguarded checkout still rejected, unreadable-steps:still warns-without-failing (exit 0 +::warning)python3 .github/agents-md-integrity/check_agents_md.py --root .— passed (1 pre-existing unrelated CODEOWNERS warning)python3 -m py_compileon both touched filesRiskiest line
ref_use = binding is None and is_ref_use(code, ref_res). If_strip_commentever removed text that is really config, a genuine unguarded checkout would
stop being reported — the one direction this lint must never move. Three things
make it safe: an unquoted
#in YAML is a comment, so there is nothingreal past it;
_strip_commentis quote-aware and deliberatelyover-protective (any quote opens a masked region), so it strips less than a
naive regex and is biased toward keeping text visible; and the probe battery
above shows no real shape flipping from reported to unreported.
The decoy tests assert
[]where a site used to be recorded, which reads like acapability being taken away — it isn't. The binding in those fixtures lives
entirely inside a YAML comment, which Actions never evaluates, so there is no
runtime resolver and nothing real goes unguarded; the assertion is checked
against the same steps with the comment deleted, which produce the identical
[].Review round 1 — two fail-open regressions closed (
d665bfe)The panel found that classifying on
_strip_comment(line)unconditionallymoved the lint fail-OPEN in the two places a
#is not a comment at all — theone direction ref-use recognition may never move. Both were reproduced against
the base before fixing, and both now match base behaviour exactly:
ref: >-/${{ 'a/b # c' && inputs.workflows_ref }}- {name: "foo/bar # baz", …, with: {ref: …}}#inside a block or quoted scalar is content, not a comment._REF_KEY_OPEN_REnow captures which spelling opened the scalar(
quoted/block), and only the plainref:-then-value form — the oneYAML really ends at a
#— has its continuations stripped.pending_partsfollows the same gate, so the folded step-output read cannot drift from it.
_strip_commentrescans each physical line from scratch, so the tail of aflow mapping split across lines had its closing quote's
#read as anopener and the checkout truncated away.
ref_checkoutsnow tracks theprevious line's end-of-line quote state on the strict
_quote_maskreading— the one
_flow_brace_deltatakes for the same structural question — andjudges RAW when it was left open. That is the pre-strip reading, i.e. the
fail-closed one.
Both gates are pinned by tests that fail on the previous commit
(
test_a_hash_inside_a_block_scalar_body_is_content_not_a_comment,test_a_quote_opened_on_the_previous_line_suspends_the_strip), plus twocomplements that pass either way so the gates cannot be read as "never strip a
continuation": the quoted-scalar case, and the plain
ref:continuation whosecomment is still stripped.
Re-verified after the change: 283 tests pass; the 48-file corpus sweep is
byte-identical across base / first cut / now; a 21-shape battery diffed against
both earlier versions changes on exactly these two shapes and nothing else. The
only shapes where the branch still records fewer sites than base are the two
comment-only decoys it set out to fix, where no runtime site exists.
Review round 2
Eight panel findings on d665bfe/1e8a5a3, all reproduced before acting, each
diffed against
mainto tell introduced from pre-existing (f43738e):main_quote_scancarries the open quote line to line (strict reading)_quote_maskmatches quote types; the line is reported on both trees# … inputs.workflows_refas value|/>/quoted bodies; judged as the same words on one line without the#_steps_output_sites(strip=False)reads the gated text&anchor/!!tagmain_opens_quoted_scalarwalks over node propertiesref:window skips>2-and a plain same-line RHS_BLOCK_HEADER; plain window seeded from the key lineWORKFLOWS_REF:in arun:body registers a bindingenv:-parent check vs skipping block-scalar bodies)plaingate was unpinned (suite passed with it deleted)Every fix reverted in turn fails at least one new test (mutation-checked);
ref_checkoutsover all 48 workflow files is byte-identical acrossmain,the previous head and
f43738e.Review round 3 (
7a3f802)Ten panel findings on
f43738e, every one reproduced on the branch head beforeacting, and every fix pinned by a test that fails on
f43738e(9 new tests failthere; the tenth is the fixture drift guard):
run:line/body disables the windows and the strip for the rest of the job"${{ steps.r.outputs.ref/}}"records nothing,"${{/steps.r.outputs.ref }}"is UNPARSED_open_quoted_valuejudges the ref value's own quote on the scan maskref:gained_env_bindingsfolds a quoted-open or plain env value with its deeper continuationsinputs[/'workflows_ref'])continues past a still-open${{_leading_operand_reaches_input(strip=False)runs on the fold, so the split spelling matches the one-line verdict (this covers BE-9637's premise)_strip_after_carried_quotestrips the remainder once the scalar closeswhileloopQUOTED_SPLIT_CHECKOUThas no drift guardref_checkoutsandenv_aliasesover all 48 workflow files: byte-identicalbetween
mainand7a3f802.Residual
Deliberately not fixed here, each stated so it is actionable without this PR
open:
name: "{ref: ${{ inputs.workflows_ref }}}"stillclassifies as a ref use and is reported unguarded. That is a
_REF_USE_FLOW_RE/ quote-mask question rather than a comment question, itfails closed (a spurious failure, not a missed hole), and it is
out of scope for this change. Confirmed still present after the fix by direct
probe. 0 occurrences in the 48 real workflow files.
if:. Anif:written inside a flow mapping is still notread, so such a step is reported unguarded even when guarded at runtime —
pre-existing, documented in
.github/workflow-pins/README.md, untouched.— where the evidence behind this change was originally gathered — lives in an
issue tracker this environment has no access to, so it was not read. The
fix was instead re-derived from scratch against the checked-in checker: each of
the four failure modes was reproduced on this branch's base commit before any
edit, and each is now pinned by a test that fails without the fix. The
original run's transcript is likewise unreadable from here.
ref:continuation arm is still deferred(BE-9638): it judges
_pins_to_job_workflow_shaon one physical line, so afallback folded across two lines loses
uses_fallbackand, downstream,check_dir'sdefault: ''carve-out (fails closed; low). Its sibling —the missing leading-operand check, BE-9637 — was fixed in round 3 once the
new plain window started routing fresh spellings into that arm.
_strip_commentopens a masked region on anyquote, so
name: don't resolve # {WORKFLOWS_REF: …}leaves the commentunstripped and read as config. Verified in both directions; both fail
closed (a spurious report, never a missed checkout), and the
over-protection is what keeps a
run: |body'secho "PR #${n}"intact.Documented in the README rather than narrowed.
indentation bound is taken from the line that first opened the scalar; a
continuation that closes one quote and opens another keeps that bound rather
than its own. Over-approximates in the fail-closed direction (the raw reading
persists a line longer), only inside the original opener's block.
--workflows-diragainst consumer repos. The CLI was exercised onlyagainst this repo's own tree and synthetic temp fixtures, not against any
consumer repo's workflows — consumer rosters are private and out of reach from
a public-repo change.
env_aliasesdoes nothave the analogous phantom-alias bug (a commented-out
BASE: ${{ inputs.workflows_ref }}registers no alias, verified by probe), andref_checkoutsis the only caller of_GUARD_BINDING_RE/is_ref_use/_GUARD_BINDING_FLOW_RE, so there is no second call site carrying the sameraw-line classification.
Judgment calls
and
env_aliasesstrip comments; the_REF_USE_*trio's section did not. Thechange would otherwise have left the doc silently half-true, so a short
paragraph was added rather than leaving it as a follow-up.
file's unusually high comment density and naming why every other reader in
the loop stays raw. Future edits to that loop are the realistic way this
regresses.
RESOLVER—the comment, not the guard shape, is what is under test, and the larger fixture
would have made the assertions harder to read.
Provenance
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 308 passed, 0 failed (272 before the branch, +36);python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 exempt;check_agents_md.py --root .: passed (1 pre-existing CODEOWNERS warning);ref_checkoutsswept over all 48 workflow files atmain/ the previous head (1e8a5a3) /f43738e: byte-identical all three ways, andref_checkouts+env_aliasesover the same 48 files identical betweenmainand7a3f802; the 9 behavioural round-3 tests all fail onf43738eand pass on7a3f802; each round-2 fix reverted in turn -> at least one of the new tests fails (sticky quote state, node properties,strip=False, plain opener, block header, theplaingate, thequoted_opencarried-state gate); all 8 round-2 findings reproduced against bothmainand the branch before acting; the merged result against currentmain(clean auto-merge) passes the suite and the repo lintd665bfe); round 2 (f43738e) fixed two more regressions the round-1 fix left (carried quote state, node properties), closed three pre-existing gaps on the same axis or the same edited pattern (step-output re-strip,>2-, plain same-line RHS), refuted one finding with evidence, answered one as YAML-correct with a README correction, and deferred one pre-existing structural finding to a follow-up. Round 3 (7a3f802) fixed all ten findings, including the previously-deferred leading-operand check (BE-9637) once the new plain window made it reachable through code this PR adds; no new deferrals. All twenty-three review threads answered and resolved. See## Review round 1,## Review round 2and## Residual.🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.