[GH-ISSUE #259] [groom] Declared-vs-documented input drift is machine-checked for 1 of the 16 reusable workflows #9

New issue

Open

opened 2026-10-26 22:34:28 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:34:28 +00:00

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Sep 3, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/259

Groom auto-builder — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · run

Verified

16 workflow files declare a top-level workflow_call: block (agents-md-integrity, assign-prs-to-author, assign-reviewers, coderabbit-config-validate, cursor-review-auto-label, cursor-review, detect-unreviewed-merge, groom, linear-ticket, pr-area-label, pr-derisk, pr-risk, pr-size, public-repo-hygiene, refresh-reviewers, stale). Exactly one - cursor-review - has a drift test: .github/cursor-review/tests/test_workflow_inputs_docs.py asserts set equality between on.workflow_call.inputs, the knob table in .github/cursor-review/README.md and the Inputs table in docs/callers/cursor-review.md. Nothing else compares a workflow's inputs to its docs (grep -rl docs/callers over .github returns only that test plus two unrelated comment mentions).

AGENTS.md:132-136 records the stake and the precedent: GitHub rejects an unknown input at startup, so a phantom input in the docs is a broken caller for whoever copies it - with cursor-review's blocking: input as the worked example, deleted in #31, documented in three places for weeks, restored by BE-4691. Each input surface is written down three or four times (the workflow block, the header caller example, docs/callers/<name>.md, and the README catalog row - README.md:24, the pr-risk row, is a single table cell over 5,000 characters).

Scope note the finder missed

docs/callers/ holds 15 pages, not 16: refresh-reviewers.yml declares workflow_call but has no caller page. And the third leg of the cursor-review test (a directory README knob table) exists only for cursor-review. So the generalisation is over (workflow file, docs/callers page) PAIRS with an optional third leg, and it needs an explicit decision for refresh-reviewers - add a page, or record it as intentionally uncatalogued.

Fix

Parameterise test_workflow_inputs_docs.py over a table of (workflow, docs page, optional directory-README section) triples and move it somewhere shared (e.g. .github/workflow-pins/tests/ or a new docs-drift directory), so adding a reusable workflow adds a row rather than a bespoke test. Keep the existing self-checks - each scanner asserts a non-empty result and the presence of workflows_ref - since they are what makes generalising safe. Test-only; expect the first run to surface real drift across the 15 currently unguarded workflows, and budget for fixing it.

Steelman

Strict set equality across 16 workflows may produce false failures on legitimate presentational choices (a deliberately undocumented internal input, a table split across sections), which trains people to weaken the check. Counter: the assertion is over input NAMES only, it is already proven non-annoying on the most input-heavy workflow in the repo, and the failure it prevents is a zero-log startup_failure in someone else's repo.

Originally created by @cloud-code-bot[bot] on GitHub (Sep 3, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/259 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/33739264302) ## Verified 16 workflow files declare a top-level `workflow_call:` block (agents-md-integrity, assign-prs-to-author, assign-reviewers, coderabbit-config-validate, cursor-review-auto-label, cursor-review, detect-unreviewed-merge, groom, linear-ticket, pr-area-label, pr-derisk, pr-risk, pr-size, public-repo-hygiene, refresh-reviewers, stale). Exactly one - cursor-review - has a drift test: `.github/cursor-review/tests/test_workflow_inputs_docs.py` asserts set equality between `on.workflow_call.inputs`, the knob table in `.github/cursor-review/README.md` and the Inputs table in `docs/callers/cursor-review.md`. Nothing else compares a workflow's inputs to its docs (`grep -rl docs/callers` over `.github` returns only that test plus two unrelated comment mentions). `AGENTS.md:132-136` records the stake and the precedent: GitHub rejects an unknown input at startup, so a phantom input in the docs is a broken caller for whoever copies it - with cursor-review's `blocking:` input as the worked example, deleted in #31, documented in three places for weeks, restored by BE-4691. Each input surface is written down three or four times (the workflow block, the header caller example, `docs/callers/<name>.md`, and the README catalog row - `README.md:24`, the pr-risk row, is a single table cell over 5,000 characters). ## Scope note the finder missed `docs/callers/` holds 15 pages, not 16: `refresh-reviewers.yml` declares `workflow_call` but has no caller page. And the third leg of the cursor-review test (a directory README knob table) exists only for cursor-review. So the generalisation is over (workflow file, docs/callers page) PAIRS with an optional third leg, and it needs an explicit decision for refresh-reviewers - add a page, or record it as intentionally uncatalogued. ## Fix Parameterise `test_workflow_inputs_docs.py` over a table of (workflow, docs page, optional directory-README section) triples and move it somewhere shared (e.g. `.github/workflow-pins/tests/` or a new docs-drift directory), so adding a reusable workflow adds a row rather than a bespoke test. Keep the existing self-checks - each scanner asserts a non-empty result and the presence of `workflows_ref` - since they are what makes generalising safe. Test-only; expect the first run to surface real drift across the 15 currently unguarded workflows, and budget for fixing it. ## Steelman Strict set equality across 16 workflows may produce false failures on legitimate presentational choices (a deliberately undocumented internal input, a table split across sections), which trains people to weaken the check. Counter: the assertion is over input NAMES only, it is already proven non-annoying on the most input-heavy workflow in the repo, and the failure it prevents is a zero-log startup_failure in someone else's repo.

bsakaguchi added the

groom

label

2026-10-26 22:34:28 +00:00

bsakaguchi commented

2026-10-26 22:34:30 +00:00

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: still valid, but the headline count is out of date. Leaving this open, tracked internally.

Verified against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b.

What changed since filing:

It is now 2 of 16, not 1 of 16. .github/refresh-reviewers/tests/test_inputs_docs.py landed in #298 and was extended in #299. It is a second full input-set drift test — and a near-copy of the cursor-review one rather than a generalisation of it. Its own docstring says so: "Deliberately parsed WITHOUT PyYAML, like the cursor-review model (test_workflow_inputs_docs.py)".
Scope note 5 is wholly resolved. docs/callers/refresh-reviewers.md landed in #296, so docs/callers/ is now 1:1 with the 16 reusables. The "add a page, or record it as intentionally uncatalogued" decision was made in favour of adding a page.

What has not changed — the substance of the issue:

The count of reusables declaring workflow_call is still exactly 16, and your list is byte-identical to what is on main today. (A naive grep returns 19; test-pr-derisk.yml and test-pr-risk.yml mention it only in header prose and test-workflow-pins.yml inside heredoc fixtures.)
14 of the 16 still have no full input-set test, and no shared or parameterised harness exists. Both existing tests hardcode their workflow/docs/README paths at module top.
AGENTS.md:132-136 is still exact, worked example and all.

One clarification worth recording, because it is easy to assume otherwise: the workflows_ref docs cross-check that landed in #292 does not cover this. check_workflow_pins.py:52 sets INPUT_NAME = "workflows_ref" and :1839 compiles a regex hardcoded to that one name. It asserts that a caller guide has a workflows_ref row and that the row's Default cell is not a literal ref — it never enumerates on.workflow_call.inputs. A page documenting a phantom blocking: input, or omitting a real one, passes it clean. It is also gated on the workflow having no default:, so it reaches about 4 workflows, not 16.

So your proposed fix — parameterise over (workflow, docs page, optional README section) triples and move it somewhere shared — is untouched, and is now more attractive: there are two hand-rolled parsers (141 and 536 lines) to fold into one table, and the newer one already carries the richer assertions a generalised version should inherit (Default column, example with: blocks, partial-README one-way mode).

Tracked internally as an investigation first, because the thing nobody knows yet is how much real drift the first run surfaces across the 14 unguarded workflows — which decides whether this is one PR or a sequence, and whether strict set equality is viable everywhere or needs an exemption mechanism. Your steelman is the reason for that ordering.

Reopen or comment if you think the rescoping loses something.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: still valid, but the headline count is out of date. Leaving this open, tracked internally.** Verified against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. **What changed since filing:** - **It is now 2 of 16, not 1 of 16.** `.github/refresh-reviewers/tests/test_inputs_docs.py` landed in #298 and was extended in #299. It is a second full input-set drift test — and a near-copy of the cursor-review one rather than a generalisation of it. Its own docstring says so: "Deliberately parsed WITHOUT PyYAML, like the cursor-review model (test_workflow_inputs_docs.py)". - **Scope note 5 is wholly resolved.** `docs/callers/refresh-reviewers.md` landed in #296, so `docs/callers/` is now 1:1 with the 16 reusables. The "add a page, or record it as intentionally uncatalogued" decision was made in favour of adding a page. **What has not changed — the substance of the issue:** - The count of reusables declaring `workflow_call` is still exactly 16, and your list is byte-identical to what is on main today. (A naive grep returns 19; `test-pr-derisk.yml` and `test-pr-risk.yml` mention it only in header prose and `test-workflow-pins.yml` inside heredoc fixtures.) - **14 of the 16 still have no full input-set test**, and no shared or parameterised harness exists. Both existing tests hardcode their workflow/docs/README paths at module top. - `AGENTS.md:132-136` is still exact, worked example and all. **One clarification worth recording**, because it is easy to assume otherwise: the `workflows_ref` docs cross-check that landed in #292 does **not** cover this. `check_workflow_pins.py:52` sets `INPUT_NAME = "workflows_ref"` and `:1839` compiles a regex hardcoded to that one name. It asserts that a caller guide has a `workflows_ref` row and that the row's Default cell is not a literal ref — it never enumerates `on.workflow_call.inputs`. A page documenting a phantom `blocking:` input, or omitting a real one, passes it clean. It is also gated on the workflow having no `default:`, so it reaches about 4 workflows, not 16. So your proposed fix — parameterise over (workflow, docs page, optional README section) triples and move it somewhere shared — is untouched, and is now *more* attractive: there are two hand-rolled parsers (141 and 536 lines) to fold into one table, and the newer one already carries the richer assertions a generalised version should inherit (Default column, example `with:` blocks, partial-README one-way mode). Tracked internally as an investigation first, because the thing nobody knows yet is how much real drift the first run surfaces across the 14 unguarded workflows — which decides whether this is one PR or a sequence, and whether strict set equality is viable everywhere or needs an exemption mechanism. Your steelman is the reason for that ordering. Reopen or comment if you think the rescoping loses something. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:27 +00:00

[PR #9] [MERGED] ci: bump cursor-review panel and judge to Opus 4.8 #19