[PR #435] [groom] Three load-bearing comments have rotted where the comment is the specification #434

New issue

Open

opened 2026-10-26 22:39:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented 2026-10-26 22:39:47 +00:00

Owner

Copy link

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/435
Author: @cloud-code-bot[bot]
Created: 10/17/2026
Status: 🔄 Open

Base: main ← Head: groom/github-workflows-whole-repo-github-cursor-review-39021195499-0

---

📝 Commits (2)

• 866312d groom: Three load-bearing comments have rotted where the comment is the specification
• 1037195 groom: correct the auto-rereview gate, auto-retry cases and its best-effort re-read in the comments

📊 Changes

4 files changed (+60 additions, -18 deletions)

View changed files

📝 .github/cursor-review/auto-approve.py (+42 -1)
📝 .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+14 -13)
📝 .github/workflows/cursor-review.yml (+3 -3)
📝 AGENTS.md (+1 -1)

📄 Description

🤖 Auto-built by the groom sweep — this is a machine-proposed refactor. It runs full CI + cursor-review and requires human review; it is NOT auto-merged. Ranked from a CONFIRMED groom finding. · run

ELI5

Some of the comments in this repo are the specification — they tell the next reader (human or agent) what the code is supposed to do. Three of them had drifted away from the code they describe: a test's prose said there were "eight" copies of a safety check when the file actually has seventeen, the auto-approve script's header said it has "five" commands when it has eight, and the repo's orientation doc still called a workflow "planned" that has been shipped and wired up for a while. This PR only rewrites that prose (plus one test method name and one failure string), so nothing the workflows do changes — it just stops the documentation from lying to the next reader.

What changed

• .github/cursor-review/tests/test_workflow_ref_guard_parity.py — removed every spelled-out count so the number cannot rot again: the module docstring now talks about "one literal copy per guarded job" and "exactly EXPECTED_GUARDS guard steps" instead of "eight"; test_exactly_eight_guard_steps is renamed test_exactly_the_expected_number_of_guard_steps; the parity failure message interpolates len(blocks) instead of saying "the eight copies are load-bearing"; and the case-insensitivity comment now says "every guarded job". EXPECTED_GUARDS = 17 and every assertion are untouched.
• .github/cursor-review/auto-approve.py — the module header now says "Eight subcommands" and documents the three that were missing (auto-rereview, approve-only, auto-retry) in the same style as the existing sections, placed in the order main() registers them. approve-only's entry says go (not eligible) is what stands auto-rereview down, matching the workflow's gate. auto-retry's entry covers both retry cases and says its PR re-read is best-effort. The existing "each run from a job that checks out NO PR code" clause was re-checked against cursor-review.yml: all three of those jobs check out only Comfy-Org/github-workflows at inputs.workflows_ref, so the clause still holds.
• .github/workflows/cursor-review.yml — comment only: the approve-only job's header said auto-rereview stands down on eligible, but the gate reads go.
• AGENTS.md — .github/cursor-approve/'s entry no longer calls cursor-approve.yml "planned"; that workflow exists and cursor-review.yml already calls it.

Why

This repo treats comments as the design artefact, so a stale one is actively misleading rather than merely untidy. The test file's prose is doc rot inside the mechanism built to catch drift: a reader trusting "eight" would conclude that nine of the seventeen Require a pinned workflows_ref guards are unaccounted for. The auto-approve.py header is the CLI contract of the module that decides whether a bot approves a PR, and three of its subcommands appeared nowhere in it. AGENTS.md is the first file an onboarding agent is told to read.

Nothing executable changed: the edits are comments, docstrings, one assertion message and one test method name (unittest discovers tests by the test_ prefix, and nothing else in the repo references the old name). No behaviour could change as a result, and the guard-parity suite still asserts exactly what it did before.

Verifier rationale

Problem

This repo treats comments as the design artefact, which makes a stale one actively misleading. Three are stale, all verified against the code:

1. .github/cursor-review/tests/test_workflow_ref_guard_parity.py — doc rot inside the anti-drift mechanism itself. The module docstring says "eight literal copies" (:6), "exactly EIGHT guard steps" (:13), "all eight env: + run: blocks" (:15) and "the literal bytes of eight blocks" (:27); the test method is named test_exactly_eight_guard_steps (:122); the failure message says "the eight copies are load-bearing" (:140); and :194 mentions "all eight jobs". Meanwhile EXPECTED_GUARDS = 17 (:43), and grep -c 'name: Require a pinned workflows_ref' .github/workflows/cursor-review.yml returns 17. The constant and the file are correct; every piece of prose around them is not.

2. .github/cursor-review/auto-approve.py:6 — "Five subcommands, each run from a job that checks out NO PR code". main() registers eight: decide (:4658), dismiss-stale (:4711), round-cap (:4723), approve-external (:4729), auto-rereview (:4762), approve-only (:4770), auto-retry (:4785), withdraw (:4789). The docstring documents five (decide, dismiss-stale, round-cap, approve-external, withdraw); auto-rereview, approve-only and auto-retry appear nowhere in the header. This is the entry-point docstring of the module that decides whether a bot approves a PR.

3. AGENTS.md:54 — "the pure verdict aggregator behind the planned cursor-approve.yml". .github/workflows/cursor-approve.yml exists (549 lines) and is invoked from .github/workflows/cursor-review.yml:8218 (uses: ./.github/workflows/cursor-approve.yml, the approve-start-early job). The finder cited AGENTS.md:53 and cursor-review.yml:8180; the actual lines are 54 and 8218 (8180 is the job header). AGENTS.md is the first file an onboarding agent reads, and the subject of its own integrity checker.

Fix

1. In test_workflow_ref_guard_parity.py, replace every spelled-out "eight" with the value — e.g. interpolate EXPECTED_GUARDS into the failure messages, rename test_exactly_eight_guard_steps to test_exactly_the_expected_number_of_guard_steps, and reword the docstring so no count is written in words. The count cannot then rot again.
2. Change auto-approve.py's "Five subcommands" to "Eight subcommands" and add the three missing sections (auto-rereview, approve-only, auto-retry) in the header's existing style. Check the "each run from a job that checks out NO PR code" clause still holds for all eight before asserting it.
3. Change AGENTS.md:54's "the planned cursor-approve.yml" to describe it as shipped.

Steelman

Comment fixes are not a refactor and risk being bikeshedding. These three clear that bar narrowly and for one reason each: the first is a false specification living inside the mechanism built to stop drift (a reader trusting it would conclude nine of the seventeen guards are unaccounted for); the second is the contract of the auto-approval engine's CLI; the third is the repo's orientation document, which an agent is explicitly told to read first.

Risk

None — prose only, no behaviour change. Item 1 touches a test's strings and one method name; run the suite to confirm it still passes.

Provenance

Verified:

• .github/cursor-review/tests unittest suite passes (1732 tests) at the current head
• .github/workflow-pins tests and check_workflow_pins.py pass; check_agents_md.py --root . passes (pre-existing length/CODEOWNERS warnings only)
• check-org-repo-literals.sh passes
• The three panel findings were checked against cursor-review.yml's auto-rereview gate and against cmd_auto_retry / auto_retry_eligible

Deviations:

• Besides the planned three files, also fixed the same stale eligible claim in the approve-only job comment in cursor-review.yml (comment only)
• The last commit has Skip-caller-bump: true: the whole PR is comments, docstrings and test wording, so no consumer behaviour changes

---

_{🔄 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/435 **Author:** [@cloud-code-bot[bot]](https://github.com/apps/cloud-code-bot) **Created:** 10/17/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `groom/github-workflows-whole-repo-github-cursor-review-39021195499-0` --- ### 📝 Commits (2) - [`866312d`](https://github.com/Comfy-Org/github-workflows/commit/866312dc95d18617038e36c71e3f658eaa0c7fe0) groom: Three load-bearing comments have rotted where the comment is the specification - [`1037195`](https://github.com/Comfy-Org/github-workflows/commit/103719578c90368f240a55d146db86ebf18998b1) groom: correct the auto-rereview gate, auto-retry cases and its best-effort re-read in the comments ### 📊 Changes **4 files changed** (+60 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/auto-approve.py` (+42 -1) 📝 `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+14 -13) 📝 `.github/workflows/cursor-review.yml` (+3 -3) 📝 `AGENTS.md` (+1 -1) </details> ### 📄 Description > 🤖 **Auto-built by the groom sweep** — this is a machine-proposed refactor. It runs full CI + cursor-review and **requires human review; it is NOT auto-merged.** Ranked from a CONFIRMED groom finding. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/39021195499) ## ELI5 Some of the comments in this repo are the specification — they tell the next reader (human or agent) what the code is supposed to do. Three of them had drifted away from the code they describe: a test's prose said there were "eight" copies of a safety check when the file actually has seventeen, the auto-approve script's header said it has "five" commands when it has eight, and the repo's orientation doc still called a workflow "planned" that has been shipped and wired up for a while. This PR only rewrites that prose (plus one test method name and one failure string), so nothing the workflows do changes — it just stops the documentation from lying to the next reader. ## What changed - `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` — removed every spelled-out count so the number cannot rot again: the module docstring now talks about "one literal copy per guarded job" and "exactly `EXPECTED_GUARDS` guard steps" instead of "eight"; `test_exactly_eight_guard_steps` is renamed `test_exactly_the_expected_number_of_guard_steps`; the parity failure message interpolates `len(blocks)` instead of saying "the eight copies are load-bearing"; and the case-insensitivity comment now says "every guarded job". `EXPECTED_GUARDS = 17` and every assertion are untouched. - `.github/cursor-review/auto-approve.py` — the module header now says "Eight subcommands" and documents the three that were missing (`auto-rereview`, `approve-only`, `auto-retry`) in the same style as the existing sections, placed in the order `main()` registers them. `approve-only`'s entry says `go` (not `eligible`) is what stands `auto-rereview` down, matching the workflow's gate. `auto-retry`'s entry covers both retry cases and says its PR re-read is best-effort. The existing "each run from a job that checks out NO PR code" clause was re-checked against `cursor-review.yml`: all three of those jobs check out only `Comfy-Org/github-workflows` at `inputs.workflows_ref`, so the clause still holds. - `.github/workflows/cursor-review.yml` — comment only: the `approve-only` job's header said `auto-rereview` stands down on `eligible`, but the gate reads `go`. - `AGENTS.md` — `.github/cursor-approve/`'s entry no longer calls `cursor-approve.yml` "planned"; that workflow exists and `cursor-review.yml` already calls it. ## Why This repo treats comments as the design artefact, so a stale one is actively misleading rather than merely untidy. The test file's prose is doc rot *inside* the mechanism built to catch drift: a reader trusting "eight" would conclude that nine of the seventeen `Require a pinned workflows_ref` guards are unaccounted for. The `auto-approve.py` header is the CLI contract of the module that decides whether a bot approves a PR, and three of its subcommands appeared nowhere in it. `AGENTS.md` is the first file an onboarding agent is told to read. Nothing executable changed: the edits are comments, docstrings, one assertion message and one test method name (unittest discovers tests by the `test_` prefix, and nothing else in the repo references the old name). No behaviour could change as a result, and the guard-parity suite still asserts exactly what it did before. <details> <summary><strong>Verifier rationale</strong></summary> ## Problem This repo treats comments as the design artefact, which makes a stale one actively misleading. Three are stale, all verified against the code: **1. `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` — doc rot inside the anti-drift mechanism itself.** The module docstring says "eight literal copies" (`:6`), "exactly EIGHT guard steps" (`:13`), "all eight `env:` + `run:` blocks" (`:15`) and "the literal bytes of eight blocks" (`:27`); the test method is named `test_exactly_eight_guard_steps` (`:122`); the failure message says "the eight copies are load-bearing" (`:140`); and `:194` mentions "all eight jobs". Meanwhile `EXPECTED_GUARDS = 17` (`:43`), and `grep -c 'name: Require a pinned workflows_ref' .github/workflows/cursor-review.yml` returns **17**. The constant and the file are correct; every piece of prose around them is not. **2. `.github/cursor-review/auto-approve.py:6` — "Five subcommands, each run from a job that checks out NO PR code".** `main()` registers **eight**: `decide` (`:4658`), `dismiss-stale` (`:4711`), `round-cap` (`:4723`), `approve-external` (`:4729`), `auto-rereview` (`:4762`), `approve-only` (`:4770`), `auto-retry` (`:4785`), `withdraw` (`:4789`). The docstring documents five (`decide`, `dismiss-stale`, `round-cap`, `approve-external`, `withdraw`); `auto-rereview`, `approve-only` and `auto-retry` appear nowhere in the header. This is the entry-point docstring of the module that decides whether a bot approves a PR. **3. `AGENTS.md:54` — "the pure verdict aggregator behind the planned `cursor-approve.yml`".** `.github/workflows/cursor-approve.yml` exists (549 lines) and is invoked from `.github/workflows/cursor-review.yml:8218` (`uses: ./.github/workflows/cursor-approve.yml`, the `approve-start-early` job). The finder cited `AGENTS.md:53` and `cursor-review.yml:8180`; the actual lines are 54 and 8218 (8180 is the job header). AGENTS.md is the first file an onboarding agent reads, and the subject of its own integrity checker. ## Fix 1. In `test_workflow_ref_guard_parity.py`, replace every spelled-out "eight" with the value — e.g. interpolate `EXPECTED_GUARDS` into the failure messages, rename `test_exactly_eight_guard_steps` to `test_exactly_the_expected_number_of_guard_steps`, and reword the docstring so no count is written in words. The count cannot then rot again. 2. Change `auto-approve.py`'s "Five subcommands" to "Eight subcommands" and add the three missing sections (`auto-rereview`, `approve-only`, `auto-retry`) in the header's existing style. Check the "each run from a job that checks out NO PR code" clause still holds for all eight before asserting it. 3. Change AGENTS.md:54's "the planned `cursor-approve.yml`" to describe it as shipped. ## Steelman Comment fixes are not a refactor and risk being bikeshedding. These three clear that bar narrowly and for one reason each: the first is a false specification living *inside* the mechanism built to stop drift (a reader trusting it would conclude nine of the seventeen guards are unaccounted for); the second is the contract of the auto-approval engine's CLI; the third is the repo's orientation document, which an agent is explicitly told to read first. ## Risk None — prose only, no behaviour change. Item 1 touches a test's strings and one method name; run the suite to confirm it still passes. </details> ## Provenance **Verified:** - `.github/cursor-review/tests` unittest suite passes (1732 tests) at the current head - `.github/workflow-pins` tests and `check_workflow_pins.py` pass; `check_agents_md.py --root .` passes (pre-existing length/CODEOWNERS warnings only) - `check-org-repo-literals.sh` passes - The three panel findings were checked against `cursor-review.yml`'s `auto-rereview` gate and against `cmd_auto_retry` / `auto_retry_eligible` **Deviations:** - Besides the planned three files, also fixed the same stale `eligible` claim in the `approve-only` job comment in `cursor-review.yml` (comment only) - The last commit has `Skip-caller-bump: true`: the whole PR is comments, docstrings and test wording, so no consumer behaviour changes <!-- groom-signature: Z2l0aHViLXdvcmtmbG93czp3aG9sZS1yZXBvOmdpdGh1Yi1jdXJzb3ItcmV2aWV3LWF1dG8tYXBwcm92ZS1weQ== --> --- _{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

bsakaguchi added the

pull-request

label 2026-10-26 22:39:47 +00:00

bsakaguchi referenced this issue 2026-10-26 22:39:47 +00:00

[PR #434] [MERGED] feat(cursor-review): axes_alongside_panel — run cursor-approve's start phase and the gating axes beside the panel #435

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

fix/cursor-review-post-retry-5xx

ci/bump-cursor-review

ci/refresh-coderabbit-schema

ci/bump-cursor-cli-pin

dependabot/github_actions/github-actions-43d9b7ee8a

dependabot/pip/dot-github/coderabbit-config/rpds-py-2026.9.1

codex/cursor-review-fields

codex/cursor-review-history-shape

codex/cursor-label-shape

codex/cursor-immediate-handoff

codex/cursor-preserve-rounds

codex/cursor-count-superseded

codex/cursor-three-review

codex/cursor-fallback-finding-identity

codex/readiness-human-identity

codex/readiness-reviewer-permissions

codex/readiness-approval-incomplete

codex/cursor-review-evaluation

codex/readiness-check-failures

codex/readiness-shadow-report

codex/readiness-review-inventory

codex/readiness-from-risk

codex/pr-risk-report-snapshot

codex/pr-risk-report-only

codex/risk-report-base-revision

fix/cursor-review-account-outage-guard

groom/github-workflows-whole-repo-github-cursor-review-39021195499-0

matt/direct-lane-error-visibility

recovery/worktrees/20261008T012922Z-github-workflows-pr178-dirty-3c1b68f78aaf

matt/cursor-review-auto-approve

fix/detect-unreviewed-human-approvals

anupreet-comfy/be-17523-runs-on-input

christian-byrne/cifix-16923-groom-finder-bounds

christian-byrne/fix-linear-ticket-post-merge

chore/comfy-cli-codeowners-team-allowlist

matt/be-4405-groom-extra-denied-paths

cx/risk-dispute-command

cx/risk-label-map-variable

cx/named-risk-tiers

luke/area-label-bump-doc

matt/be-5508-pr-risk-shadow

matt/be-4005-bump-cursor-review-auto-label-callers

agent/cursor-review-xl

matt/be-3160-judge-parse-hardening

matt/auto-label-runs-on

matt/cursor-review-self-hostable

matt/be-3046-cursor-review-callers-var

matt/reviewers-team-slug-expansion

matt/cursor-review-judge-reformat-retry

matt/fix-bump-trailing-newline

matt/bump-kimi-k2.7-code

matt/be-1916-harden-judge-parse

matt/be-1891-cursor-review-blocking

matt/bump-callers-workflow-dispatch

matt/fix-bump-callers-yaml-parse

matt/be-1812-story-11-cursor-review-reusable-workflow-optional-dedicated

fix/cursor-review-fork-pr-403

ci/auto-bump-cursor-review-callers

chore/cursor-review-opus-4-8

matt/auto-label-open-ready

chore/ignore-pycache-cursor-review

docs/cursor-review-readme

feat/cursor-review-auto-label-reusable

chore/cursor-review-deroster

v1

Labels

Clear labels   

cursor-review-catalog-drift

  

groom

  

groom

  

pull-request

Mirrored from GitHub Pull Request

No labels

cursor-review-catalog-drift

groom

groom

pull-request

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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#434

No description provided.