[PR #187] [MERGED] docs(callers): sync workflows_ref rows and prose to the required-no-default reality (BE-6507) #193

New issue

Closed

opened 2026-10-26 22:37:13 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:13 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/187
Author: @mattmillerai
Created: 8/20/2026
Status: ✅ Merged
Merged: 8/20/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6507-sync-workflows-ref-docs

📝 Commits (2)

9f81730 docs(callers): sync workflows_ref rows and prose to the required-no-default reality (BE-6507)
8d4dc9f docs(callers): keep the groom workflows_ref pin; scope the fail-fast claim (BE-6507)

📊 Changes

7 files changed (+110 additions, -40 deletions)

View changed files

📝 CONTRIBUTING.md (+5 -4)
📝 README.md (+18 -8)
📝 docs/callers/README.md (+57 -15)
📝 docs/callers/agents-md-integrity.md (+1 -1)
📝 docs/callers/cursor-review.md (+1 -1)
📝 docs/callers/groom.md (+27 -10)
📝 docs/callers/pr-size.md (+1 -1)

📄 Description

ELI-5

Four caller docs still said the workflows_ref input "defaults to main". It hasn't for a while: on nine of the ten workflows that take it, the input is required: with no default and the run stops with an ::error:: if you leave it out. This PR makes the docs say what the workflows actually do. Docs only — no workflow YAML changed.

The tenth is groom, and it is the interesting one — see "The groom question", below.

What changed

docs/callers/pr-size.md, docs/callers/cursor-review.md, docs/callers/agents-md-integrity.md — the workflows_ref inputs-table row now follows the house style already shipped at docs/callers/pr-risk.md: — (**required**), no default on purpose, each keeping its own trailing clause about what loads from the ref (tool build / review prompts / checker script). cursor-review.md additionally scopes the fail-fast promise (see below).
docs/callers/groom.md — row, inputs preamble, caller snippet and the two "keep it in lock-step" footguns rewritten. Net effect: keep pinning workflows_ref on groom callers, with the reason stated.
docs/callers/README.md — the ### workflows_ref must match your pin section rewritten. "It defaults to main" and the "usually harmless, occasionally not, and always confusing" framing are gone; in their place, which workflows declare the input at all, the two tiers of value checking that actually run (warn-on-non-SHA vs reject-unless-merged-40-hex), the limits of "it fails fast", and groom.
README.md, CONTRIBUTING.md — the same stale claims outside docs/.

The groom question — settled, and it changed this PR

The first revision of this PR told every groom caller to drop its explicit workflows_ref pin, on the strength of BE-4169's auto-derive: groom's asset checkouts are ${{ inputs.workflows_ref || github.job_workflow_sha }}, so an omitted input was supposed to fall back to the commit uses: resolved to. The body flagged that groom.yml:879 asserts the opposite and said the question could not be settled from the repo alone.

The review panel raised it as High, and it is settled now — the pessimistic side is right:

GitHub's contexts reference does not list job_workflow_sha as a github context property. The documented commit-SHA-of-the-current-job's-workflow-file value is job.workflow_sha.
actionlint 1.7.12 rejects it: property "job_workflow_sha" is not defined in object type {…} — and the object type it prints contains workflow_sha but no job_workflow_sha.
This repo already says so in three places — groom.yml's CLI-pin comment (~L879), .github/groom/README.md (~L508), pr-risk.yml (~L41/435/549, which names the actionlint rejection explicitly).
groom.yml itself works around it: the agent CLI pin checkout uses job.workflow_sha, specifically to avoid the trap.

Actions expands an unknown context property to '', and actions/checkout with an empty ref takes the default branch. So an omitted workflows_ref today loads the briefs, ledger.py and interval.py from main into jobs holding ANTHROPIC_API_KEY and the App token. BE-4169's premise is stale.

So this PR took the panel's second option — keep documenting the explicit pin — and records why the derived ref cannot replace it yet. The workflow-side fix is deferred (below).

Review findings addressed

All six panel threads were valid and are fixed:

Sev	Finding	Fix
🟠 High	Docs told groom callers to drop the pin, contradicting three in-repo statements that `github.job_workflow_sha` is empty	Reverted to explicit-pin advice across all four pages, with the evidence stated
🟡 Med	"the run fails fast" is not universal — `cursor-review`'s `Prior-review ledger` job carries no guard by design, and a label-gate skip means guarded jobs never evaluate theirs	Scoped the claim in `cursor-review.md`, `docs/callers/README.md`, `README.md`
🟢 Low	"override … must equal your `uses:` SHA" is unsatisfiable (a branch is not a SHA; an equal override is a no-op)	Clause removed from both pages
🟢 Low	"It is on you to keep it in lock-step" misdescribes the fleet — `bump-callers.sh` rewrites both pins in one pass	Rewritten; added that an override moves briefs+ledger but not the agent CLI pin
🟢 Low	"Every OTHER workflow here requires it" overshoots — five documented workflows don't declare the input, and GitHub rejects an undeclared `with:` key at startup	Both lists now enumerate declare / don't-declare explicitly
⚪ Nit	The quoted `::error::workflows_ref is required…` is wrong for `pr-risk`/`pr-derisk`, which have no empty-value branch	Attributed to the five that emit it; the shape-error fall-through documented for the other two

Method

Each page was written from the merged workflow_call input declaration, re-verified this round by parsing all ten declarations out of .github/workflows/*.yml:

Workflow	`required`	`default`	Runtime guard
`pr-size`, `cursor-review`, `agents-md-integrity`, `coderabbit-config-validate`, `refresh-reviewers`	`true`	none	empty → `::error::workflows_ref is required…` + `exit 1`; non-40-hex → `::warning::`
`pr-risk`, `pr-derisk`, `pr-area-label`, `linear-ticket`	`true`	none	rejects anything not a merged full 40-hex SHA of this repo. `pr-area-label`/`linear-ticket` name the empty case; `pr-risk`/`pr-derisk` have no empty branch, so `''` falls through to the shape error
`groom`	`false`	`''`	none — checkouts are `${{ inputs.workflows_ref \|\| github.job_workflow_sha }}`, which does not resolve

Verification

Every one of the six findings re-derived from source before fixing, not taken on the panel's word: the ten input declarations parsed out of the workflow YAML; the guard ::error::/::warning:: strings grepped per workflow; cursor-review.yml's ledger job confirmed guard-free with ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} and its guarded siblings confirmed if: needs.gate.outputs.should_run == 'true'; bump-callers.sh confirmed to rewrite workflows_ref: alongside uses:; docs/callers/*.md cross-checked against which workflows declare the input.
github.job_workflow_sha checked against GitHub's contexts reference and executed through actionlint 1.7.12 on a minimal reusable workflow — both spellings tested, output quoted above.
Unit suites (unaffected by a docs diff, run as a smoke check): workflow-pins 78 ok, agents-md-integrity 46 ok, groom 365 ok (1 skipped), cursor-review 194 ok. shellcheck -x on bump-callers.sh + its tests: clean.
Pre-existing and untouched: check_agents_md.py --root . fails on this repo's own AGENTS.md being 273 lines against a 200-line ceiling. Identical on main — not introduced here, and not a check this repo's CI runs.

Judgment calls

The docs now contradict groom.yml's own input description, which still reads "LEAVE UNSET (the default)", as does its header caller example. That is deliberate: the description is wrong, and correcting it is a workflow change (which forces a GROOM_CALLERS re-bump), so it belongs to the follow-up rather than to a docs-only PR. Called out in the thread reply so it is not mistaken for an oversight.
The pinning example block was kept but re-pointed at cursor-review.yml. Unchanged from the first revision.
Two edits outside the ticket's edit-site list (README.md, CONTRIBUTING.md) carry the same stale claim the ticket exists to remove, are docs, and are not under .github/.
coderabbit-config-validate.md's row was left alone. It reads *(required)* — not house style, but not wrong.

Not fixed here (why this is `Refs`, not `Closes`)

The workflow-side fix is deferred to a follow-up ticket. Moving groom.yml's 7 asset checkouts and cursor-review.yml's ledger checkout onto job.workflow_sha behind an empty-ref guard also means updating check_workflow_pins.py's BE-4169 exemption regex (which hardcodes the github.job_workflow_sha spelling) and its tests, AGENTS.md, .github/groom/README.md and groom.yml's own input description — then re-bumping the affected fleets. All under .github/, explicitly out of this ticket's scope. Once it lands, these pages can go back to "leave it unset".
docs/callers/pr-derisk.md documents workflows_ref in prose but has no row for it in its inputs table. An omission, not a false statement; left for the same follow-up.

Unexercised artifacts

No workflow run was triggered. The github.job_workflow_sha question is settled from GitHub's own documentation, actionlint's schema and this repo's four internal statements rather than from a live consumer run — convergent and authoritative, but not an observed run.
No Linear access from this session.

Refs BE-6507

Provenance

Authored by: agent-work loop
Verified: GitHub contexts reference + actionlint 1.7.12 both confirm github.job_workflow_sha is not a github context property and job.workflow_sha is the documented accessor; all ten workflows_ref declarations and every guard string re-parsed from the merged workflow YAML; cursor-review.yml's ledger job confirmed guard-free; bump-callers.sh confirmed to rewrite both pins in one pass. Unittest suites: workflow-pins 78 ok, agents-md-integrity 46 ok, groom 365 ok/1 skipped, cursor-review 194 ok; shellcheck -x clean. check_agents_md.py --root . fails on a pre-existing 273-line AGENTS.md, identical on main.
Deviations: the first revision's central advice — "drop the workflows_ref pin on groom callers" — was reversed after the review panel's High finding was verified against GitHub's documentation; the docs now keep the explicit pin. This leaves the docs deliberately contradicting groom.yml's own input description until the deferred workflow fix lands. Two docs files outside the ticket's edit-site list were also synced, and the pinning example was re-pointed from groom.yml to cursor-review.yml.

_{🔄 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/187 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6507-sync-workflows-ref-docs` --- ### 📝 Commits (2) - [`9f81730`](https://github.com/Comfy-Org/github-workflows/commit/9f817306b863bbdece45c4e5c3959ad99013cf4a) docs(callers): sync workflows_ref rows and prose to the required-no-default reality (BE-6507) - [`8d4dc9f`](https://github.com/Comfy-Org/github-workflows/commit/8d4dc9f0194727408f2f3b3b9603e3b6f1bc3b38) docs(callers): keep the groom workflows_ref pin; scope the fail-fast claim (BE-6507) ### 📊 Changes **7 files changed** (+110 additions, -40 deletions) <details> <summary>View changed files</summary> 📝 `CONTRIBUTING.md` (+5 -4) 📝 `README.md` (+18 -8) 📝 `docs/callers/README.md` (+57 -15) 📝 `docs/callers/agents-md-integrity.md` (+1 -1) 📝 `docs/callers/cursor-review.md` (+1 -1) 📝 `docs/callers/groom.md` (+27 -10) 📝 `docs/callers/pr-size.md` (+1 -1) </details> ### 📄 Description ## ELI-5 Four caller docs still said the `workflows_ref` input "defaults to `main`". It hasn't for a while: on nine of the ten workflows that take it, the input is `required:` with no default and the run stops with an `::error::` if you leave it out. This PR makes the docs say what the workflows actually do. Docs only — no workflow YAML changed. The tenth is `groom`, and it is the interesting one — see "The groom question", below. ## What changed - `docs/callers/pr-size.md`, `docs/callers/cursor-review.md`, `docs/callers/agents-md-integrity.md` — the `workflows_ref` inputs-table row now follows the house style already shipped at `docs/callers/pr-risk.md`: `— (**required**)`, no default on purpose, each keeping its own trailing clause about *what* loads from the ref (tool build / review prompts / checker script). `cursor-review.md` additionally scopes the fail-fast promise (see below). - `docs/callers/groom.md` — row, inputs preamble, caller snippet and the two "keep it in lock-step" footguns rewritten. Net effect: **keep pinning `workflows_ref` on groom callers**, with the reason stated. - `docs/callers/README.md` — the `### workflows_ref must match your pin` section rewritten. "It defaults to `main`" and the "usually harmless, occasionally not, and always confusing" framing are gone; in their place, which workflows declare the input at all, the two tiers of value checking that actually run (warn-on-non-SHA vs reject-unless-merged-40-hex), the limits of "it fails fast", and groom. - `README.md`, `CONTRIBUTING.md` — the same stale claims outside `docs/`. ## The groom question — settled, and it changed this PR The first revision of this PR told every groom caller to **drop** its explicit `workflows_ref` pin, on the strength of BE-4169's auto-derive: groom's asset checkouts are `${{ inputs.workflows_ref || github.job_workflow_sha }}`, so an omitted input was supposed to fall back to the commit `uses:` resolved to. The body flagged that `groom.yml:879` asserts the opposite and said the question could not be settled from the repo alone. The review panel raised it as **High**, and it is settled now — the pessimistic side is right: - **GitHub's contexts reference** does not list `job_workflow_sha` as a `github` context property. The documented commit-SHA-of-the-current-job's-workflow-file value is **`job.workflow_sha`**. - **`actionlint` 1.7.12** rejects it: `property "job_workflow_sha" is not defined in object type {…}` — and the object type it prints contains `workflow_sha` but no `job_workflow_sha`. - **This repo already says so in three places** — `groom.yml`'s CLI-pin comment (~L879), `.github/groom/README.md` (~L508), `pr-risk.yml` (~L41/435/549, which names the `actionlint` rejection explicitly). - **`groom.yml` itself works around it**: the agent CLI pin checkout uses `job.workflow_sha`, specifically to avoid the trap. Actions expands an unknown context property to `''`, and `actions/checkout` with an empty `ref` takes the default branch. So an omitted `workflows_ref` today loads the briefs, `ledger.py` and `interval.py` from **`main`** into jobs holding `ANTHROPIC_API_KEY` and the App token. BE-4169's premise is stale. So this PR took the panel's second option — **keep documenting the explicit pin** — and records why the derived ref cannot replace it yet. The workflow-side fix is deferred (below). ## Review findings addressed All six panel threads were valid and are fixed: | Sev | Finding | Fix | |---|---|---| | 🟠 High | Docs told groom callers to drop the pin, contradicting three in-repo statements that `github.job_workflow_sha` is empty | Reverted to explicit-pin advice across all four pages, with the evidence stated | | 🟡 Med | "the run fails fast" is not universal — `cursor-review`'s `Prior-review ledger` job carries no guard by design, and a label-gate skip means guarded jobs never evaluate theirs | Scoped the claim in `cursor-review.md`, `docs/callers/README.md`, `README.md` | | 🟢 Low | "override … must equal your `uses:` SHA" is unsatisfiable (a branch is not a SHA; an equal override is a no-op) | Clause removed from both pages | | 🟢 Low | "It is on you to keep it in lock-step" misdescribes the fleet — `bump-callers.sh` rewrites both pins in one pass | Rewritten; added that an override moves briefs+ledger but **not** the agent CLI pin | | 🟢 Low | "Every OTHER workflow here requires it" overshoots — five documented workflows don't declare the input, and GitHub rejects an undeclared `with:` key at startup | Both lists now enumerate declare / don't-declare explicitly | | ⚪ Nit | The quoted `::error::workflows_ref is required…` is wrong for `pr-risk`/`pr-derisk`, which have no empty-value branch | Attributed to the five that emit it; the shape-error fall-through documented for the other two | ## Method Each page was written from the **merged** `workflow_call` input declaration, re-verified this round by parsing all ten declarations out of `.github/workflows/*.yml`: | Workflow | `required` | `default` | Runtime guard | |---|---|---|---| | `pr-size`, `cursor-review`, `agents-md-integrity`, `coderabbit-config-validate`, `refresh-reviewers` | `true` | none | empty → `::error::workflows_ref is required…` + `exit 1`; non-40-hex → `::warning::` | | `pr-risk`, `pr-derisk`, `pr-area-label`, `linear-ticket` | `true` | none | rejects anything not a merged full 40-hex SHA of this repo. `pr-area-label`/`linear-ticket` name the empty case; `pr-risk`/`pr-derisk` have no empty branch, so `''` falls through to the shape error | | `groom` | `false` | `''` | none — checkouts are `${{ inputs.workflows_ref \|\| github.job_workflow_sha }}`, which does not resolve | ## Verification - Every one of the six findings re-derived from source before fixing, not taken on the panel's word: the ten input declarations parsed out of the workflow YAML; the guard `::error::`/`::warning::` strings grepped per workflow; `cursor-review.yml`'s `ledger` job confirmed guard-free with `ref: ${{ inputs.workflows_ref || github.job_workflow_sha }}` and its guarded siblings confirmed `if: needs.gate.outputs.should_run == 'true'`; `bump-callers.sh` confirmed to rewrite `workflows_ref:` alongside `uses:`; `docs/callers/*.md` cross-checked against which workflows declare the input. - `github.job_workflow_sha` checked against GitHub's contexts reference **and** executed through `actionlint` 1.7.12 on a minimal reusable workflow — both spellings tested, output quoted above. - Unit suites (unaffected by a docs diff, run as a smoke check): workflow-pins 78 ok, agents-md-integrity 46 ok, groom 365 ok (1 skipped), cursor-review 194 ok. `shellcheck -x` on `bump-callers.sh` + its tests: clean. - Pre-existing and untouched: `check_agents_md.py --root .` fails on this repo's own `AGENTS.md` being 273 lines against a 200-line ceiling. Identical on `main` — not introduced here, and not a check this repo's CI runs. ## Judgment calls - **The docs now contradict `groom.yml`'s own input description**, which still reads "LEAVE UNSET (the default)", as does its header caller example. That is deliberate: the description is wrong, and correcting it is a workflow change (which forces a `GROOM_CALLERS` re-bump), so it belongs to the follow-up rather than to a docs-only PR. Called out in the thread reply so it is not mistaken for an oversight. - **The pinning example block was kept but re-pointed at `cursor-review.yml`.** Unchanged from the first revision. - **Two edits outside the ticket's edit-site list** (`README.md`, `CONTRIBUTING.md`) carry the same stale claim the ticket exists to remove, are docs, and are not under `.github/`. - **`coderabbit-config-validate.md`'s row was left alone.** It reads `*(required)*` — not house style, but not wrong. ## Not fixed here (why this is `Refs`, not `Closes`) - **The workflow-side fix is deferred to a follow-up ticket.** Moving `groom.yml`'s 7 asset checkouts and `cursor-review.yml`'s ledger checkout onto `job.workflow_sha` behind an empty-ref guard also means updating `check_workflow_pins.py`'s BE-4169 exemption regex (which hardcodes the `github.job_workflow_sha` spelling) and its tests, `AGENTS.md`, `.github/groom/README.md` and `groom.yml`'s own input description — then re-bumping the affected fleets. All under `.github/`, explicitly out of this ticket's scope. Once it lands, these pages can go back to "leave it unset". - `docs/callers/pr-derisk.md` documents `workflows_ref` in prose but has no row for it in its inputs table. An omission, not a false statement; left for the same follow-up. ## Unexercised artifacts - No workflow run was triggered. The `github.job_workflow_sha` question is settled from GitHub's own documentation, `actionlint`'s schema and this repo's four internal statements rather than from a live consumer run — convergent and authoritative, but not an observed run. - No Linear access from this session. Refs BE-6507 ## Provenance - **Authored by:** agent-work loop - **Verified:** GitHub contexts reference + `actionlint` 1.7.12 both confirm `github.job_workflow_sha` is not a `github` context property and `job.workflow_sha` is the documented accessor; all ten `workflows_ref` declarations and every guard string re-parsed from the merged workflow YAML; `cursor-review.yml`'s `ledger` job confirmed guard-free; `bump-callers.sh` confirmed to rewrite both pins in one pass. Unittest suites: workflow-pins 78 ok, agents-md-integrity 46 ok, groom 365 ok/1 skipped, cursor-review 194 ok; `shellcheck -x` clean. `check_agents_md.py --root .` fails on a pre-existing 273-line `AGENTS.md`, identical on `main`. - **Deviations:** the first revision's central advice — "drop the `workflows_ref` pin on groom callers" — was **reversed** after the review panel's High finding was verified against GitHub's documentation; the docs now keep the explicit pin. This leaves the docs deliberately contradicting `groom.yml`'s own input description until the deferred workflow fix lands. Two docs files outside the ticket's edit-site list were also synced, and the pinning example was re-pointed from `groom.yml` to `cursor-review.yml`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:13 +00:00