[PR #292] [MERGED] feat(workflow-pins): fail CI when a docs/callers workflows_ref row contradicts the declaration #291

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6508-docs-callers-workflows-ref-cross-check

📝 Commits (2)

6f4dc40 feat(workflow-pins): cross-check docs/callers workflows_ref rows against the declaration
56504ab fix(workflow-pins): harden the docs/callers workflows_ref cross-check

📊 Changes

5 files changed (+432 additions, -1 deletions)

View changed files

📝 .github/workflow-pins/README.md (+12 -0)
📝 .github/workflow-pins/check_workflow_pins.py (+155 -1)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+258 -0)
📝 .github/workflows/test-workflow-pins.yml (+6 -0)
📝 docs/callers/pr-derisk.md (+1 -0)

📄 Description

ELI-5

The workflow-pins lint already keeps every reusable workflow's workflows_ref input from carrying a default: (a default would let a caller pin uses: to a fixed commit yet still load this repo's scripts from a moving branch — BE-5546). But the human-readable setup guides in docs/callers/ have their own little "inputs" table, and nothing stopped a table there from saying workflows_ref defaults to main even though the workflow forbids it. A caller who copies that wrong row reopens the exact hole the lint closes. This PR teaches the lint to read those docs tables too and fail CI when a page contradicts the declaration.

What this does

Adds a third check to check_workflow_pins.py. For every reusable workflow that declares a workflows_ref workflow_call input as required: true with no default: (the same state the existing no-default lint parses), it finds the matching docs/callers/<name>.md page and, if that page exists, locates its inputs-table row for workflows_ref:

If the row's default column names a literal ref (main, a SHA, a <placeholder>, anything other than a required/no-default marker like — or required), the run fails, naming the docs file, the row's line number, and the workflow declaration it contradicts.
If the page exists but has no workflows_ref row at all, that is a hard error too — absence of the row must not read the same as "not applicable", matching the discipline the checker already uses for a workflow that consumes the input but whose declaration it cannot locate.
A page that does not exist under the workflow's own name is skipped (not every reusable has a same-named guide).

Only the required-no-default direction is asserted, so an optional / auto-derived input — groom.yml's default: '' self-pin — is deliberately left alone and its docs may keep documenting a default. The parser is text-level like the rest of the file (no PyYAML), skips fenced ```yaml caller examples and prose so neither is mistaken for the table row, and is wired into check_dir so test-workflow-pins.yml picks it up with no workflow-file change.

Acceptance

Reverting a compliant docs row to | workflows_ref | main | … | on this branch makes the pins lint fail with, e.g.:

::error file=docs/callers/pr-risk.md,line=120::the workflows_ref inputs-table row documents a default of main, but pr-risk.yml declares workflows_ref as required: true with no default. … See BE-6508.

Restoring the row returns the run to green.

Corpus swept

Of the 11 workflows the lint tracks, 10 are required-no-default (groom.yml is the optional default: '' self-pin and is excluded). Of those 10: 8 already carried a compliant — (**required**) / *(required)* row and pass unchanged; docs/callers/pr-derisk.md had no workflows_ref row (fixed here); and refresh-reviewers.yml has no page under its own name (see Residual).

Tests

New: DocsDefaultLiteralRefTests (8), FindDocsRowTests (4), DocsCrossCheckTests (6) in tests/test_check_workflow_pins.py, matching the file's string-fixture style — including the ticket's four cases: a docs row claiming main → hit with the right line number; a — (**required**) row → clean; a present page missing the row → hard error; and a workflows_ref in prose or a fenced ```yaml block → not mistaken for the row.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' → 334 passed, 0 failed; python3 .github/workflow-pins/check_workflow_pins.py against .github/workflows → OK (11 workflows, 0 problems); reverted a docs row to main → lint failed naming file+line+workflow, restored → green; the two failing-path CLI smoke steps from test-workflow-pins.yml (reintroduced default, unguarded checkout) still reject and do not leak a docs error into their fixture dirs.
Deviations: none

Residual

refresh-reviewers.yml is uncovered by the docs cross-check. It declares workflows_ref required-no-default, but its caller guide is docs/callers/assign-reviewers.md, not refresh-reviewers.md, so the by-basename mapping (docs/callers/<name>.md) finds no page and skips it. This is consistent with the ticket's "if the page exists" scoping, but it means assign-reviewers.md — which today documents no workflows_ref row at all — is never checked and could drift undetected. A follow-up should either add a workflows_ref row to assign-reviewers.md and teach the checker the refresh-reviewers → assign-reviewers name mapping, or reconcile the page name.
docs/callers/pr-derisk.md needed a workflows_ref row added. It was the one remaining drift the new check flags on the current tree (its "Inputs worth setting" table omitted workflows_ref entirely). I added the row, mirroring pr-risk.md, so this PR's own CI is green. This is arguably the sibling docs-sync ticket's territory rather than this check-only ticket's; it is the minimal docs edit required for the new check to pass on main. If the docs-sync ticket intended pr-derisk.md to stay row-less, the reviewer should decide whether the row or a checker carve-out is preferred.
Unexercised artifact. The blocking sibling docs-sync ticket named in the description could not be read (no Linear access from this environment), so I could not confirm its Done state; I inferred the remaining drift empirically from the checker's output on main instead.

_{🔄 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/292 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6508-docs-callers-workflows-ref-cross-check` --- ### 📝 Commits (2) - [`6f4dc40`](https://github.com/Comfy-Org/github-workflows/commit/6f4dc40194cf4d6fe9eb585efb623431146407d3) feat(workflow-pins): cross-check docs/callers workflows_ref rows against the declaration - [`56504ab`](https://github.com/Comfy-Org/github-workflows/commit/56504ab353cb698f332928b527473b8485835575) fix(workflow-pins): harden the docs/callers workflows_ref cross-check ### 📊 Changes **5 files changed** (+432 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+12 -0) 📝 `.github/workflow-pins/check_workflow_pins.py` (+155 -1) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+258 -0) 📝 `.github/workflows/test-workflow-pins.yml` (+6 -0) 📝 `docs/callers/pr-derisk.md` (+1 -0) </details> ### 📄 Description ## ELI-5 The workflow-pins lint already keeps every reusable workflow's `workflows_ref` input from carrying a `default:` (a default would let a caller pin `uses:` to a fixed commit yet still load this repo's scripts from a moving branch — BE-5546). But the human-readable setup guides in `docs/callers/` have their own little "inputs" table, and nothing stopped a table there from *saying* `workflows_ref` defaults to `main` even though the workflow forbids it. A caller who copies that wrong row reopens the exact hole the lint closes. This PR teaches the lint to read those docs tables too and fail CI when a page contradicts the declaration. ## What this does Adds a third check to `check_workflow_pins.py`. For every reusable workflow that declares a `workflows_ref` `workflow_call` input as `required: true` with **no** `default:` (the same state the existing no-default lint parses), it finds the matching `docs/callers/<name>.md` page and, if that page exists, locates its inputs-table row for `workflows_ref`: - If the row's default column names a **literal ref** (`main`, a SHA, a `<placeholder>`, anything other than a required/no-default marker like `—` or `required`), the run **fails**, naming the docs file, the row's line number, and the workflow declaration it contradicts. - If the page exists but has **no** `workflows_ref` row at all, that is a hard error too — absence of the row must not read the same as "not applicable", matching the discipline the checker already uses for a workflow that consumes the input but whose declaration it cannot locate. - A page that does not exist under the workflow's own name is skipped (not every reusable has a same-named guide). Only the required-no-default direction is asserted, so an optional / auto-derived input — `groom.yml`'s `default: ''` self-pin — is deliberately left alone and its docs may keep documenting a default. The parser is text-level like the rest of the file (no PyYAML), skips fenced ` ```yaml ` caller examples and prose so neither is mistaken for the table row, and is wired into `check_dir` so `test-workflow-pins.yml` picks it up with no workflow-file change. ## Acceptance Reverting a compliant docs row to `| workflows_ref | main | … |` on this branch makes the pins lint fail with, e.g.: `::error file=docs/callers/pr-risk.md,line=120::the workflows_ref inputs-table row documents a default of main, but pr-risk.yml declares workflows_ref as required: true with no default. … See BE-6508.` Restoring the row returns the run to green. ## Corpus swept Of the 11 workflows the lint tracks, 10 are required-no-default (`groom.yml` is the optional `default: ''` self-pin and is excluded). Of those 10: 8 already carried a compliant `— (**required**)` / `*(required)*` row and pass unchanged; `docs/callers/pr-derisk.md` had **no** `workflows_ref` row (fixed here); and `refresh-reviewers.yml` has no page under its own name (see Residual). ## Tests - New: `DocsDefaultLiteralRefTests` (8), `FindDocsRowTests` (4), `DocsCrossCheckTests` (6) in `tests/test_check_workflow_pins.py`, matching the file's string-fixture style — including the ticket's four cases: a docs row claiming `main` → hit with the right line number; a `— (**required**)` row → clean; a present page missing the row → hard error; and a `workflows_ref` in prose or a fenced ` ```yaml ` block → not mistaken for the row. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` → 334 passed, 0 failed; `python3 .github/workflow-pins/check_workflow_pins.py` against `.github/workflows` → OK (11 workflows, 0 problems); reverted a docs row to `main` → lint failed naming file+line+workflow, restored → green; the two failing-path CLI smoke steps from `test-workflow-pins.yml` (reintroduced default, unguarded checkout) still reject and do not leak a docs error into their fixture dirs. - **Deviations:** none ## Residual - **`refresh-reviewers.yml` is uncovered by the docs cross-check.** It declares `workflows_ref` required-no-default, but its caller guide is `docs/callers/assign-reviewers.md`, not `refresh-reviewers.md`, so the by-basename mapping (`docs/callers/<name>.md`) finds no page and skips it. This is consistent with the ticket's "if the page exists" scoping, but it means `assign-reviewers.md` — which today documents no `workflows_ref` row at all — is never checked and could drift undetected. A follow-up should either add a `workflows_ref` row to `assign-reviewers.md` and teach the checker the `refresh-reviewers` → `assign-reviewers` name mapping, or reconcile the page name. - **`docs/callers/pr-derisk.md` needed a `workflows_ref` row added.** It was the one remaining drift the new check flags on the current tree (its "Inputs worth setting" table omitted `workflows_ref` entirely). I added the row, mirroring `pr-risk.md`, so this PR's own CI is green. This is arguably the sibling docs-sync ticket's territory rather than this check-only ticket's; it is the minimal docs edit required for the new check to pass on `main`. If the docs-sync ticket intended `pr-derisk.md` to stay row-less, the reviewer should decide whether the row or a checker carve-out is preferred. - **Unexercised artifact.** The blocking sibling docs-sync ticket named in the description could not be read (no Linear access from this environment), so I could not confirm its Done state; I inferred the remaining drift empirically from the checker's output on `main` instead. --- <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:09 +00:00