docs(callers): per-workflow setup guides, README restructure, public-repo hygiene (BE-4680) - #80
Open
mattmillerai wants to merge 5 commits into
Open
docs(callers): per-workflow setup guides, README restructure, public-repo hygiene (BE-4680)#80mattmillerai wants to merge 5 commits into
mattmillerai wants to merge 5 commits into
Conversation
…repo hygiene (BE-4680) The catalog documented what each workflow does and was near-silent on how to wire one up, deferring to "each workflow file's header comment" — 105 KB of YAML in groom's case. Measured against the old README's only example: on: 0 hits (no trigger block — copy-paste never fires) cron: 0 hits concurrency: 0 hits (duplicating groom's group DEADLOCKS the run) CALLERS: 0 hits (enrollment is two steps; the roster step was undocumented) and its permissions (contents+pull-requests read) are short for groom, which needs issues:write — the exact grant that produces a zero-job startup_failure. docs/callers/: a shared contract page (pinning, the startup-time permission rule, workflows_ref lock-step, the concurrency deadlock, the roster step) plus one page per workflow carrying a complete caller, verified against each workflow's actual workflow_call inputs rather than prose. Removes three phantom inputs the README advertised on cursor-review (blocking, runs_on, models — none declared; GitHub rejects unknown inputs at startup) and fixes detect-unreviewed-merge's header example, which pointed at a path that 404s and showed pull_request where every live caller uses push. Public-repo hygiene: MIT LICENSE (the repo had none while inviting outside use), SECURITY.md, CONTRIBUTING.md, and deletes a tracked 3.36 MB arm64 binary that pr-size.yml rebuilds from source and the ubuntu runners cannot execute.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 3 |
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…680) Resolves the cursor-review panel's findings on the new guides. Every fix is a doc/example correction — no workflow behavior changes. - cursor-review: drop the "opt into blocking" claim. `cursor-review.yml` declares no `blocking:` input (inputs are exactly bot_app_id, diff_excludes, diff_size_cap, judge_model, review_label, run_without_label, workflows_ref), so a caller acting on it gets a zero-job startup_failure — the same phantom input this PR's AGENTS.md/README changes removed. Redirect to the real mechanism: mark `<caller job id> / Consolidate panel` a required check. - cursor-review: `run_without_label: true` only widens the reusable's gate; it cannot add events to the caller's `on:`. A caller left on [labeled, unlabeled] silently never reviews ordinary PRs. Document that both must change together. - cursor-review: `diff_excludes` default is not "(none)" — the workflow ships lockfiles/node_modules/.claude/dist/vendor/*.generated.*/*.min.js, and a caller-supplied value replaces the list. - cursor-review: add the fork-skip and one-review-per-HEAD-SHA dedupe gotchas (remove-and-re-add no-ops; dismiss the review to re-run). - assign-reviewers: the App token does NOT buy fork support. `pull_request` withholds secrets from forks, so the token mint hard-fails. Add the same-repo `if:` guard to the example and explain why pull_request_target is not offered (head-sha reviewers.yml read would become an escalation), matching this repo's own ci-assign-reviewers.yml. - cursor-review-auto-label: same fork failure, and the reusable carries no guard of its own — add the `if:` plus a gotcha noting cursor-review skips forks anyway. - agents-md-integrity: reconcile "CLAUDE.md if present" with the `require_shim: true` default, which FAILS a repo that has AGENTS.md and no CLAUDE.md (check_agents_md.py:197). - callers/README: the first copy-pasteable caller omitted `workflows_ref`, contradicting the same file's Pinning section — pin it to the uses: SHA. - callers/README: the verification recipe assumed universal workflow_dispatch + dry_run. Split by trigger shape: dry_run exists only for groom/stale; PR-triggered callers verify via a throwaway PR, and adding workflow_dispatch helps only agents-md-integrity (no event dependency); detect-unreviewed-merge is push-triggered. - pr-size / agents-md-integrity: annotate `branches: [main]` as the reader's default branch (a `master` repo gets no runs and no signal), matching detect-unreviewed-merge.md. - stale: redact `Comfy-Org/cloud#3523` to `your-org/your-repo#123` per AGENTS.md "keep private repo paths/detail out of" public files. Tests: go (check-pr-size) + all four python suites (117 tests) pass; the agents-md-integrity gate passes on this repo; no broken relative links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 5 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
…ards (BE-4680) Second review round. The panel caught a real regression behind the "phantom blocking input" story, plus errors in my own round-1 prose. The blocking gate was NOT "never declared" — it shipped in #16 (BE-1891) as a `blocking:` input plus a fail-closed "Blocking gate" job, and #31 (a judge-extraction fix) deleted both from cursor-review.yml while leaving gate-unresolved.py and all its documentation behind. So: - .github/cursor-review/README.md still told callers to pass `blocking: true` — a zero-job startup_failure for anyone who copied it. Replaced with a regression note; dropped the `blocking` row from the knobs table (and added the missing `run_without_label` row); marked gate-unresolved.py orphaned in the file inventory. This was the missed call site of this PR's own cleanup. - AGENTS.md: corrected "never declared" to what actually happened, and made the durable lesson "deleting an input is a docs change too — grep for its name in the same commit." Withdrew my round-1 merge-gate advice. Marking `Consolidate panel` required does NOT gate anything: GitHub counts a skipped required check as passing, and that job is if:-gated on five conditions (no trigger label, dedupe hit, over diff_size_cap, fork PR, panel skipped) — so it goes green in exactly the cases where no review ran. Documented that as a trap instead. Also fixed my own false claim that "pushing a commit always re-runs": a push clears the dedupe but delivers no event to a caller on `types: [labeled, unlabeled]`, so you still toggle the label. Dependabot is not covered by the fork guards I added in round 1 — its branches are same-repo, but its runs read the Dependabot secret store, not Actions secrets, so the App-token mint still fails. Added `&& github.actor != 'dependabot[bot]'` to the assign-reviewers and cursor-review-auto-label examples and explained why assign-reviewers' own bot-author skip does not save it (that check runs after the mint has failed). Noted the same hazard for cursor-review under run_without_label/synchronize. SECURITY.md claimed the AI workflows run model steps with no write credentials. True for groom and for cursor-review's 8 panel cells (contents: read), but NOT for the judge: Consolidate panel runs the judge model and posts the review in one pull-requests: write job. Scoped the claim so that boundary reads as in-scope for reports rather than known-and-accepted. assign-prs-to-author.md said "Inputs: None" while the workflow exposes `skip-bots` (default true) — the only supported way to assign bot PRs. Tests: go + all four python suites pass; AGENTS.md gate passes; no broken links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 6 |
Panel: 8/8 reviewers contributed findings.
…ult_pool traps (BE-4680) Third review round. Mostly verified-against-source corrections to documented defaults, plus one contradiction my previous commit introduced. - CONTRIBUTING.md contradicted the SECURITY.md wording from d1b6a51 — it still claimed cursor-review's agent jobs "run with contents: read and mint no GitHub token". Scoped it the same way: clean for groom and the 8 panel cells, not clean for the judge (Consolidate panel runs the judge model and posts the review in one pull-requests: write job). - cursor-review concurrency: the recommended group keys on github.event.label.name, which is empty on opened/synchronize/reopened. Once `types:` is widened for run_without_label — which the gotcha I added in d1b6a51 tells people to do — label and plain PR events land in different groups and cannot cancel each other, so a push racing a label toggle runs two panels and posts two reviews (both clear the head-SHA dedupe before either posts). Show the PR-number-only group and explain why. - assign-reviewers: the default_pool gotcha described a fall-through that does not exist. `if (candidates.size === 0)` consults default_pool only when NO rule matched; the author-drop happens afterward. So a matched bucket whose only member is the author yields a non-empty candidate set, loses that person, and dead-ends at "No eligible candidates" without ever reaching default_pool. Documented the real failure and kept the (still valid) advice. Documented defaults corrected against the workflow source: - stale `exempt_pr_labels` omitted `do-not-merge` (present in the real default); noted that supplying a value replaces the list. - groom `themes` was shown as *(none)*; it defaults to the finder brief's five dimensions, so the knob is for NARROWING, not enabling. - callers/README overstated secret handling: omitting a `secrets:` mapping fails at startup, but mapping an unset secret passes an empty string and cursor-review then degrades to an all-empty panel rather than failing red. - .github/cursor-review/README.md linked ../../README.md#usage; this PR renamed that heading to "Quick start". Retargeted to #pinning, which is what the sentence is actually about. - .gitignore ignored check-pr-size but not check-pr-size.exe. Link checking now validates anchors, not just file existence — that is how the stale #usage fragment surfaced. Tests: go + all four python suites pass; AGENTS.md gate passes; no broken links or anchors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
…des-and-public-hygiene # Conflicts: # README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prep for broadening groom adoption. The catalog documented what each workflow does very well and was near-silent on how to wire one up, deferring to "each workflow file's header comment" — which for
groom.ymlis a 105 KB YAML.Adoption in this repo tracks documentation quality almost exactly:
uses:)Measured gaps
The only copy-pasteable example was 9 lines of placeholders. Grep counts against the old README:
on:— 0 hits. No trigger block at all, so copy-pasting it produced a workflow that never fires.cronalso 0 — for the two schedule-driven workflows the README never showed the line that makes them run.concurrency— 0 hits.groom.ymldeclaresconcurrency: groom-${{ github.repository }}withcancel-in-progress: false. A caller that duplicates that group deadlocks the run until timeout — documented only inside a comment in a different file.CALLERS— 0 hits. Enrollment is two steps: the caller PR plus avars.<NAME>_CALLERSroster entry. The second was undocumented, and a repo absent from the roster keeps its original SHA forever.contents: read+pull-requests: read; groom needsissues: write. That is exactly the grant that yields a zero-jobstartup_failurewith no logs.Three phantom inputs
The README advertised
blocking,runs_on, andmodelsoncursor-review.yml. None are declared —blockingandruns_onhave zero occurrences in the file, andmodelsexists only as an internal step output (:310). GitHub rejects an unknown input at startup, so anyone following that row got an invalid workflow.AGENTS.mdrepeated theblocking:claim.(
runs_onis real — oncursor-review-auto-label.yml. The row conflated two workflows.)One path that 404s
detect-unreviewed-merge.yml's header example pointed atComfy-Org/unreviewed-merges/.github/workflows/detector.yml@v1, which does not exist. The live path — used by every real caller — is this repo's own. The same example showedpull_requestwhere all live callers usepushto the default branch.Changes
docs/callers/— a shared contract page (pinning, the startup-time permission rule,workflows_reflock-step, the concurrency deadlock, the roster step) plus one page per workflow. Each carries a complete caller includingon:and the exact permission grant, generated against each workflow's realworkflow_callinputs rather than from prose.README — restructured into a scannable index: one line and a setup link per workflow, with the essay-length cells relocated into the guides. Phantom inputs removed.
Public-repo hygiene
LICENSE— MIT. The repo had none, so despite the README inviting outside use, nobody outside the org legally had any. (Aside:gh repo list --json licenseInforeportsNONEfor all 64 public Comfy-Org repos and is unreliable — the license API shows GPL-3.0 on the core repos and MIT on Comfy-Desktop. GPL was considered and rejected as a poor fit for CI glue that private reposuses:.)SECURITY.md— disclosure path, scope, and the agent credential boundary written down as an explicit security property: model steps hold no write token, a separate job does the writing.CONTRIBUTING.md— the test commands, the "every change here is live-fire" framing, what counts as breaking (adding a required input/secret, or a new nested-job permission — breaking even though no caller YAML changes), and two-step enrollment.scripts/check-pr-size/check-pr-size). Mach-O arm64, swept in accidentally by harden pr-size reusable: batch check-attr, --source probe, git timeouts, comment-token scope #51. It was larger than every other file in the repo combined,pr-size.yml:201rebuilds it from source on every run, and it cannot execute on theubuntu-latestrunners all 31 jobs use. Now gitignored.AGENTS.md— phantomblockingremoved,docs/callers/added to layout + deeper docs, two cross-references fixed for the renamed README sections, and one new convention: document only inputs that exist.Deliberately not included
No CODEOWNERS.
agents-md-integrity.ymlships arequire_codeownersknob and this repo trips its own warn-only check. But adding one auto-requests review from named colleagues and stands up a second routing mechanism alongside the existingassign-reviewers.ymlassignee routing. That is a team decision, not a docs PR's — happy to add it in a follow-up if wanted.Verification
go vetandgo testcleancheck_agents_md.py --root .passes (1 warning: the CODEOWNERS DRI above). AGENTS.md is 141 lines, under the 200 ceiling it enforcesUnblocks the tier-1 groom rollout.
BE-4680
🤖 Generated with Claude Code