Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #319] [MERGED] fix(pr-risk): publish the Check Run on a manual dispatch regardless of `enabled` #319

New issue

Closed

opened 2026-10-26 22:38:25 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:25 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/319
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15438-pr-risk-publish-check-dispatch

📝 Commits (2)

96cc68d fix(pr-risk): publish the Check Run on a manual dispatch regardless of enabled
71b5eea test(pr-risk): pin the job gates by equality, and scan the whole publish-check job

📊 Changes

5 files changed (+261 additions, -7 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+21 -2)
📝 .github/workflows/test-pr-risk.yml (+6 -1)
📝 scripts/pr-risk/resolve-enabled.sh (+7 -4)
➕ scripts/pr-risk/tests/test_job_gates.sh (+210 -0)
📝 scripts/pr-risk/tests/test_resolve_enabled.sh (+17 -0)

📄 Description

ELI-5

A repo can enrol the PR-risk grader switched OFF, then run it by hand on one PR to see what it would say before turning it on for real. That preview graded the PR, labelled it and posted the comment — but not the Check Run, the one permanent, commit-attached record the preview mostly exists to show off. This makes the Check Run come along too.

What changed

publish-check in .github/workflows/pr-risk.yml was gated on needs.gate.outputs.enabled == 'true' && inputs.check_run && !cancelled(). It is now gated on grade having run:

if: >-
  needs.grade.result != 'skipped' &&
  inputs.check_run &&
  !cancelled()

needs: [gate, grade] is unchanged, grade's own condition is unchanged, and the hardcoded neutral conclusion is unchanged.

The full behaviour table, assuming check_run: true and a run that was not cancelled. Exactly one row moves:

event	`enabled`	`grade.result`	before	after
`pull_request`	true	success	publishes	publishes
`pull_request`	true	failure	publishes	publishes
`pull_request`	false	skipped	skipped	skipped
`workflow_dispatch`	true	success	publishes	publishes
`workflow_dispatch`	false	success	skipped — the bug	publishes — the fix

All six live callers set enabled: true, so their behaviour is byte-identical and no consumer pin is bumped in this PR.

Why not the shorter `inputs.check_run && !cancelled()`

!cancelled() is a status check function, and any status function in an if: replaces the implicit "all needs succeeded" condition. With no result test the job would therefore RUN on every disabled pull_request run — where grade was merely skipped — boot a runner, find an empty surfaces, publish nothing and exit 0. Green, invisible, and one wasted runner per disabled PR across every enrolled repo, which is precisely the cost the cheap gate job exists to avoid. needs.grade.result != 'skipped' is what makes the !cancelled() safe.

!= 'skipped' rather than == 'success' because the publish step already tolerates a partial render (an empty surfaces is "nothing to publish", not an error; per-target failures are warnings). A success()-shaped gate would discard the targets that did render whenever grade failed partway.

Chesterton's Fence — why the `enabled` clause was safe to remove

This changes an existing condition, so the history was read before the edit rather than after:

git blame puts the clause in cc03872 (#120), the commit that introduced the publish-check job — 2026-08-04 14:57.
The dispatch exemption on grade is 0c9c2b5 (#116), 2026-08-04 14:22 — 35 minutes earlier.
#120's commit message describes check_run as off-by-default and explains the separate-job split at length; it never mentions enabled. Neither does the job's own header comment, which discusses only check_run and the unconditional checks: write grant.

So #120 copied the older gate shape and did not pick up the exemption #116 had just landed on the job it depends on. The clause was incidental, not a decision, and the two conditions genuinely disagree in exactly one state — the last row of the table above — which is the state #116 created on purpose.

Prose corrected alongside it

publish-check's header comment: one sentence that the job follows grade on a dispatch regardless of enabled, plus the rationale inline above the if:.
The check_run input description: published whenever grade ran, including a dispatch on a repo whose enabled resolves false.
The enabled input description: its "A MANUAL workflow_dispatch GRADES REGARDLESS" now adds "(and, when opted in, posts the sticky comment and the Check Run)".
scripts/pr-risk/resolve-enabled.sh: the disabled-dispatch step summary said the run "will sync a risk:* label". The sticky comment is posted from inside grade and so already rode along on a disabled dispatch before this PR; the Check Run now does too. The summary names all three, and test_resolve_enabled.sh pins the new clause.
The staleness warning below it named only the label, while the sentence above it had just told the reader the run posts a comment too (review, 71b5eea). The comment is refreshed only by a later re-grade, which a disabled repo never performs, so it keeps showing this moment's tier while subsequent commits go ungraded. It is named in the warning now; the Check Run is called out as carrying no such caveat, being pinned to the commit it graded. Line breaks keep every later commit will go ungraded intact on one line, since the suite pins that phrase and the summary is built from separate echos. The new assertion was confirmed falsifiable.

New structural test

scripts/pr-risk/tests/test_job_gates.sh, modelled on test_pin_contract.sh — hermetic, reads the workflow as text, no network. A job if: exists only in the workflow and is evaluated by the runner before any step is dispatched, so no script suite can reach it: resolve-enabled.sh can be green on every input it takes and still have its answer consumed by the wrong job.

Reworked in 71b5eea after review, which found the first draft proved less than it claimed. Every assertion was a substring-presence needle, and a needle sees a term but never how the terms are joined — so gates meaning the opposite of the pinned contract passed it. Both expressions are now pinned by equality, the same division test_pin_contract.sh already settled on for the workflows_ref guard: the needles stay, but as restatements that name which invariant broke rather than as the defence. Three things the equality does not cover are covered separately — grade's membership in needs: (without it needs.grade.result resolves to empty, '' != 'skipped' is true, and the gate silently degenerates), the forbidden needs.gate.outputs.enabled scanned over the whole job body rather than just the if: (a step-level if: reproduces the bug with the job gate still reading correctly), and the slicer's two boundaries.

Those boundaries were fragile in both directions, and neither was hypothetical. Bounding on the job's own name: key let the slice run on into the following job, concatenating two gates into one blob where either could satisfy a needle; it now bounds on the next 2-space job key. And continuation was matched by six-space indent, so a blank line inside the >- scalar — legal YAML — truncated the expression to needs.grade.result != 'skipped' &&, over which every negative assertion passed vacuously while the coverage self-check still reported a cheerful non-empty ok. Blank lines are continuations now. The comment strip also had to move ahead of the terminator: this workflow divides its jobs with comment blocks at job indent, and terminating on one truncates the body — a vacuous pass for the whole-job scan, the one assertion with no equality behind it.

Negative check, as required, and extended to the review's cases — a test that cannot fail is precisely the defect being fixed here. Seven regressions are caught: the OR-joined gate (3 failures), the inverted !inputs.check_run opt-in (2), grade dropped from needs: (1), a step-level enabled re-imposition (1), the OLD pre-fix gate (5), the naive inputs.check_run && !cancelled() (3), and grade's dispatch exemption dropped (2). Two benign reformats stay green, so the equality is not merely brittle: a blank line inside the scalar, and needs: rewritten from flow to block style. A further case pairs the step-level if: with a job-indent comment above it — green under the naive rule order, red now. pr-risk.yml was restored from a byte copy after each mutation and confirmed byte-identical to its committed state.

Wired into .github/workflows/test-pr-risk.yml (shellcheck list + its own run step). That workflow's paths: filter already covers both scripts/pr-risk/** and .github/workflows/pr-risk.yml, so no filter change was needed.

Docs — the `docs/callers/pr-risk.md` branch taken

Branch (b): no docs edit in this PR. #317 is still OPEN, and main has no check_run row in that guide yet (it has no enabled row either — the word does not appear in the file). Editing nothing here is correct; the corrected sentence was left as a comment on #317 so the row lands stating the right contract. The README.md catalog row was checked and needs no change — it already attributes the publish decision to check_run and never claims a Check Run requires enabled: true.

.github/workflow-pins/tests/test_inputs_docs_drift.py checks row presence, not content, and needed no change; it was run anyway.

Provenance

Authored by: agent-work loop
Verified: shellcheck -x on the full test-pr-risk.yml script list (13 files): clean. All 7 scripts/pr-risk suites green after the review round: 52 / 138 / 116 / 22 / 28 / 94 / 29 passed, 0 failed (test_job_gates.sh 11 → 22, test_resolve_enabled.sh 28 → 29). check_workflow_pins.py: OK (11 workflows declare workflows_ref, none with a default, every ref checkout guarded; 155 uses: pinned). check_agents_md.py --root .: passed, with the 2 pre-existing warnings this PR does not touch (AGENTS.md at 156 lines; no CODEOWNERS). workflow-pins unittest suite: 499 tests, OK (32 skipped). Negative check on the job-gate suite: 7 regressions caught, 2 benign reformats stay green, detailed above. actionlint was run on pr-risk.yml + test-pr-risk.yml for the first commit (v1.7.12, clean) and is not installed in the environment that produced 71b5eea — that commit leaves pr-risk.yml byte-identical, so the earlier result still stands for the YAML.
Deviations: none against the plan as specified. 71b5eea is review response only — it changes no workflow YAML and no grading behaviour; it hardens the new suite and extends one step-summary sentence.

Residual

The docs/callers/pr-risk.md row is not fixed by this PR and needs a human to land it. #317 adds a check_run row whose paragraph asserts "A Check Run also requires enabled: true." — a sentence this PR makes false. Nothing in CI catches it: test_inputs_docs_drift.py checks that a row for each input exists, never what it says, so #317 can merge after this PR with the stale contract and stay green indefinitely. Whichever of the two merges second must apply the replacement sentence, which is quoted verbatim in the comment linked above. If #317 merges first, a one-line follow-up edit to that row on main closes it.

The behaviour change is verified structurally and by documented runner semantics, not by a live Actions run. The claim that a status check function in an if: replaces the implicit success() on needs — which is the whole reason the needs.grade.result test is required rather than optional — comes from GitHub's documented expression semantics and from the shape of the existing gates, not from a dispatch executed against a repo with enabled: false. Exercising it for real needs an enrolled consumer repo and a workflow_dispatch that would write a label, a comment and a Check Run to someone's PR, which is a mutating action on a private consumer and was deliberately not performed. The first real dispatch preview after this merges is where the fix is actually observed; if the Check Run still does not appear there, the needs.grade.result test is the line to look at.

One artifact behind this change could not be read from the build environment. The originating read-only investigation — the one that established the clause was never a stated decision, and whose findings write-up this change is built on — lives in the team's issue tracker, which is not reachable from here. That premise was therefore re-derived independently from git blame and the two commit messages (cc03872 / 0c9c2b5) rather than taken on trust; the Chesterton's Fence section above is that re-derivation, and it agrees. The investigation's own write-up may contain reasoning not reflected here.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/319 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15438-pr-risk-publish-check-dispatch` --- ### 📝 Commits (2) - [`96cc68d`](https://github.com/Comfy-Org/github-workflows/commit/96cc68dc0b5891aa1ecc265d7ef62fd96c2af667) fix(pr-risk): publish the Check Run on a manual dispatch regardless of `enabled` - [`71b5eea`](https://github.com/Comfy-Org/github-workflows/commit/71b5eea2f1b510a2e91969e2d23ff821991e0fc8) test(pr-risk): pin the job gates by equality, and scan the whole publish-check job ### 📊 Changes **5 files changed** (+261 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+21 -2) 📝 `.github/workflows/test-pr-risk.yml` (+6 -1) 📝 `scripts/pr-risk/resolve-enabled.sh` (+7 -4) ➕ `scripts/pr-risk/tests/test_job_gates.sh` (+210 -0) 📝 `scripts/pr-risk/tests/test_resolve_enabled.sh` (+17 -0) </details> ### 📄 Description ## ELI-5 A repo can enrol the PR-risk grader switched OFF, then run it by hand on one PR to see what it would say before turning it on for real. That preview graded the PR, labelled it and posted the comment — but not the Check Run, the one permanent, commit-attached record the preview mostly exists to show off. This makes the Check Run come along too. ## What changed `publish-check` in `.github/workflows/pr-risk.yml` was gated on `needs.gate.outputs.enabled == 'true' && inputs.check_run && !cancelled()`. It is now gated on `grade` having run: ```yaml if: >- needs.grade.result != 'skipped' && inputs.check_run && !cancelled() ``` `needs: [gate, grade]` is unchanged, `grade`'s own condition is unchanged, and the hardcoded `neutral` conclusion is unchanged. The full behaviour table, assuming `check_run: true` and a run that was not cancelled. Exactly one row moves: | event | `enabled` | `grade.result` | before | after | |---|---|---|---|---| | `pull_request` | true | success | publishes | publishes | | `pull_request` | true | failure | publishes | publishes | | `pull_request` | false | skipped | skipped | skipped | | `workflow_dispatch` | true | success | publishes | publishes | | `workflow_dispatch` | **false** | success | **skipped** — the bug | **publishes** — the fix | All six live callers set `enabled: true`, so their behaviour is byte-identical and no consumer pin is bumped in this PR. ## Why not the shorter `inputs.check_run && !cancelled()` `!cancelled()` is a status check function, and any status function in an `if:` replaces the implicit "all needs succeeded" condition. With no result test the job would therefore RUN on every disabled `pull_request` run — where `grade` was merely *skipped* — boot a runner, find an empty `surfaces`, publish nothing and exit 0. Green, invisible, and one wasted runner per disabled PR across every enrolled repo, which is precisely the cost the cheap `gate` job exists to avoid. `needs.grade.result != 'skipped'` is what makes the `!cancelled()` safe. `!= 'skipped'` rather than `== 'success'` because the publish step already tolerates a partial render (an empty `surfaces` is "nothing to publish", not an error; per-target failures are warnings). A `success()`-shaped gate would discard the targets that *did* render whenever grade failed partway. ## Chesterton's Fence — why the `enabled` clause was safe to remove This changes an existing condition, so the history was read before the edit rather than after: - `git blame` puts the clause in cc03872 (#120), the commit that introduced the `publish-check` job — 2026-08-04 14:57. - The dispatch exemption on `grade` is 0c9c2b5 (#116), 2026-08-04 14:22 — **35 minutes earlier**. - #120's commit message describes `check_run` as off-by-default and explains the separate-job split at length; it never mentions `enabled`. Neither does the job's own header comment, which discusses only `check_run` and the unconditional `checks: write` grant. So #120 copied the older gate shape and did not pick up the exemption #116 had just landed on the job it depends on. The clause was incidental, not a decision, and the two conditions genuinely disagree in exactly one state — the last row of the table above — which is the state #116 created on purpose. ## Prose corrected alongside it - `publish-check`'s header comment: one sentence that the job follows `grade` on a dispatch regardless of `enabled`, plus the rationale inline above the `if:`. - The `check_run` input description: published whenever `grade` ran, including a dispatch on a repo whose `enabled` resolves false. - The `enabled` input description: its "A MANUAL `workflow_dispatch` GRADES REGARDLESS" now adds "(and, when opted in, posts the sticky comment and the Check Run)". - `scripts/pr-risk/resolve-enabled.sh`: the disabled-dispatch step summary said the run "will sync a `risk:*` label". The sticky comment is posted from inside `grade` and so already rode along on a disabled dispatch before this PR; the Check Run now does too. The summary names all three, and `test_resolve_enabled.sh` pins the new clause. - The **staleness warning** below it named only the label, while the sentence above it had just told the reader the run posts a comment too (review, 71b5eea). The comment is refreshed only by a later re-grade, which a disabled repo never performs, so it keeps showing this moment's tier while subsequent commits go ungraded. It is named in the warning now; the Check Run is called out as carrying no such caveat, being pinned to the commit it graded. Line breaks keep `every later commit will go ungraded` intact on one line, since the suite pins that phrase and the summary is built from separate `echo`s. The new assertion was confirmed falsifiable. ## New structural test `scripts/pr-risk/tests/test_job_gates.sh`, modelled on `test_pin_contract.sh` — hermetic, reads the workflow as text, no network. A job `if:` exists only in the workflow and is evaluated by the runner before any step is dispatched, so no script suite can reach it: `resolve-enabled.sh` can be green on every input it takes and still have its answer consumed by the wrong job. **Reworked in 71b5eea after review**, which found the first draft proved less than it claimed. Every assertion was a substring-presence needle, and a needle sees a *term* but never how the terms are *joined* — so gates meaning the opposite of the pinned contract passed it. Both expressions are now pinned by **equality**, the same division `test_pin_contract.sh` already settled on for the `workflows_ref` guard: the needles stay, but as restatements that name *which* invariant broke rather than as the defence. Three things the equality does not cover are covered separately — `grade`'s membership in `needs:` (without it `needs.grade.result` resolves to empty, `'' != 'skipped'` is true, and the gate silently degenerates), the forbidden `needs.gate.outputs.enabled` scanned over the **whole job body** rather than just the `if:` (a step-level `if:` reproduces the bug with the job gate still reading correctly), and the slicer's two boundaries. Those boundaries were fragile in both directions, and neither was hypothetical. Bounding on the job's own `name:` key let the slice run **on into the following job**, concatenating two gates into one blob where either could satisfy a needle; it now bounds on the next 2-space job key. And continuation was matched by six-space indent, so a blank line inside the `>-` scalar — legal YAML — **truncated the expression to `needs.grade.result != 'skipped' &&`**, over which every negative assertion passed vacuously while the coverage self-check still reported a cheerful non-empty `ok`. Blank lines are continuations now. The comment strip also had to move *ahead* of the terminator: this workflow divides its jobs with comment blocks at job indent, and terminating on one truncates the body — a vacuous pass for the whole-job scan, the one assertion with no equality behind it. **Negative check, as required**, and extended to the review's cases — a test that cannot fail is precisely the defect being fixed here. Seven regressions are caught: the OR-joined gate (3 failures), the inverted `!inputs.check_run` opt-in (2), `grade` dropped from `needs:` (1), a step-level `enabled` re-imposition (1), the OLD pre-fix gate (5), the naive `inputs.check_run && !cancelled()` (3), and `grade`'s dispatch exemption dropped (2). Two *benign* reformats stay green, so the equality is not merely brittle: a blank line inside the scalar, and `needs:` rewritten from flow to block style. A further case pairs the step-level `if:` with a job-indent comment above it — green under the naive rule order, red now. `pr-risk.yml` was restored from a byte copy after each mutation and confirmed byte-identical to its committed state. Wired into `.github/workflows/test-pr-risk.yml` (shellcheck list + its own run step). That workflow's `paths:` filter already covers both `scripts/pr-risk/**` and `.github/workflows/pr-risk.yml`, so no filter change was needed. ## Docs — the `docs/callers/pr-risk.md` branch taken **Branch (b): no docs edit in this PR.** #317 is still OPEN, and `main` has no `check_run` row in that guide yet (it has no `enabled` row either — the word does not appear in the file). Editing nothing here is correct; the corrected sentence was left as a [comment on #317](https://github.com/Comfy-Org/github-workflows/pull/317#issuecomment-5727332589) so the row lands stating the right contract. The `README.md` catalog row was checked and needs no change — it already attributes the publish decision to `check_run` and never claims a Check Run requires `enabled: true`. `.github/workflow-pins/tests/test_inputs_docs_drift.py` checks row presence, not content, and needed no change; it was run anyway. ## Provenance - **Authored by:** agent-work loop - **Verified:** `shellcheck -x` on the full `test-pr-risk.yml` script list (13 files): clean. All 7 `scripts/pr-risk` suites green after the review round: 52 / 138 / 116 / **22** / 28 / 94 / **29** passed, 0 failed (`test_job_gates.sh` 11 → 22, `test_resolve_enabled.sh` 28 → 29). `check_workflow_pins.py`: OK (11 workflows declare `workflows_ref`, none with a default, every ref checkout guarded; 155 `uses:` pinned). `check_agents_md.py --root .`: passed, with the 2 pre-existing warnings this PR does not touch (AGENTS.md at 156 lines; no CODEOWNERS). `workflow-pins` unittest suite: 499 tests, OK (32 skipped). Negative check on the job-gate suite: 7 regressions caught, 2 benign reformats stay green, detailed above. `actionlint` was run on `pr-risk.yml` + `test-pr-risk.yml` for the first commit (v1.7.12, clean) and is not installed in the environment that produced 71b5eea — that commit leaves `pr-risk.yml` byte-identical, so the earlier result still stands for the YAML. - **Deviations:** none against the plan as specified. 71b5eea is review response only — it changes no workflow YAML and no grading behaviour; it hardens the new suite and extends one step-summary sentence. ## Residual **The `docs/callers/pr-risk.md` row is not fixed by this PR and needs a human to land it.** #317 adds a `check_run` row whose paragraph asserts "**A Check Run also requires** `enabled: true`**.**" — a sentence this PR makes false. Nothing in CI catches it: `test_inputs_docs_drift.py` checks that a row for each input *exists*, never what it says, so #317 can merge after this PR with the stale contract and stay green indefinitely. Whichever of the two merges second must apply the replacement sentence, which is quoted verbatim in the comment linked above. If #317 merges first, a one-line follow-up edit to that row on `main` closes it. **The behaviour change is verified structurally and by documented runner semantics, not by a live Actions run.** The claim that a status check function in an `if:` replaces the implicit `success()` on `needs` — which is the whole reason the `needs.grade.result` test is required rather than optional — comes from GitHub's documented expression semantics and from the shape of the existing gates, not from a dispatch executed against a repo with `enabled: false`. Exercising it for real needs an enrolled consumer repo and a `workflow_dispatch` that would write a label, a comment and a Check Run to someone's PR, which is a mutating action on a private consumer and was deliberately not performed. The first real dispatch preview after this merges is where the fix is actually observed; if the Check Run still does not appear there, the `needs.grade.result` test is the line to look at. **One artifact behind this change could not be read from the build environment.** The originating read-only investigation — the one that established the clause was never a stated decision, and whose findings write-up this change is built on — lives in the team's issue tracker, which is not reachable from here. That premise was therefore re-derived independently from `git blame` and the two commit messages (`cc03872` / `0c9c2b5`) rather than taken on trust; the Chesterton's Fence section above is that re-derivation, and it agrees. The investigation's own write-up may contain reasoning not reflected here. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:25 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#319

No description provided.

Rows
Columns

[PR #319] [MERGED] fix(pr-risk): publish the Check Run on a manual dispatch regardless of enabled #319