Treat full check list as CI gate when --required is empty (#3844)#3904
Conversation
When `gh pr checks <PR> --required` reports no required checks, the
required-readiness probe is vacuously green because this repo's `main`
branch protection defines zero required status-check contexts. Agents
following the letter of the docs could report CI-ready without inspecting
any real check.
Amend the CI-readiness polling guidance in three workflow docs so that an
empty `--required` result means falling back to the full `gh pr checks`
list as the readiness gate (not advisory):
- .agents/workflows/pr-processing.md (CI Polling And Live State section
and the mirrored "Before merge" review-agent paragraph)
- .agents/skills/pr-batch/SKILL.md ("Before merge" review-agent paragraph)
- .agents/workflows/adversarial-pr-review.md (Ground Truth Commands prose
and the Independent Review Prompt gather list)
Implements option (b) from #3844. Configuring required status-check
contexts in branch protection (option a) is a maintainer admin action and
is left as a follow-up recommendation.
Closes #3844
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR updates three agent workflow/skill documents to require using the full ChangesCI Readiness Gate Documentation Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review — PR #3904Summary: Correct and well-motivated fix. The 1. Hardcoded "currently" creates a maintenance hazard (medium)The fix embeds a repo-specific fact — "this repo's Two ways to fix this:
2. Formatting inconsistency — buried mid-paragraph rule is easy to miss (low)The new sentence in the "Before merge" paragraph (in both Minor observations
Verdict: Approve after addressing the staleness concern (issue 1). Issue 2 is style-only and can be addressed in a follow-up. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Greptile SummaryThis is a documentation-only PR that hardens CI-readiness polling guidance across three agent workflow/skill files. It closes a logic gap where
Confidence Score: 4/5Documentation-only change with no code paths affected; safe to merge with minor wording consistency worth addressing. The fallback rule is correctly placed and consistently worded across two of the three touch-points, but the gather-list bullet in adversarial-pr-review.md drops the 'require those checks to pass' enforcement clause, meaning an agent executing only that path could observe the fallback condition without acting on it as a hard gate. Additionally, the rule embeds a static assertion about current branch-protection state rather than testing the observable output condition, which will silently become stale if a maintainer later configures required checks. adversarial-pr-review.md L79 — the gather-list bullet is weaker than its prose counterpart and should include 'require those checks to pass'. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent polls CI readiness] --> B[Run: gh pr checks PR --required]
B --> C{Any required checks returned?}
C -- Yes --> D[Use --required checks as readiness gate]
C -- No / empty --> E[Fallback: treat full gh pr checks PR list as gate]
D --> F{All required checks pass?}
E --> G{All full-list checks pass?}
F -- Yes --> H[CI-ready]
F -- No --> I[Not CI-ready]
G -- Yes --> H
G -- No --> I
Reviews (1): Last reviewed commit: "Treat full check list as CI gate when --..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.agents/workflows/pr-processing.md (2)
275-275: ⚡ Quick winRepo-specific assertion spans three files and will become stale if branch protection changes.
The parenthetical "(this repo's
maincurrently defines zero required status-check contexts, so--requiredis vacuously green)" appears in all three workflow/skill files and embeds a configuration fact that will become outdated if maintainers later implement option (a) from issue#3844(configure required status checks in branch protection). Rephrase to make the guidance conditional without asserting the current repo state:-If `gh pr checks <PR> --required` reports no required checks (this repo's `main` currently defines zero required status-check contexts, so `--required` is vacuously green), do NOT treat that as CI-ready: +If `gh pr checks <PR> --required` reports no required checks (because the base branch defines zero required status-check contexts, making `--required` vacuously green), do NOT treat that as CI-ready:This single change should be applied consistently to all five occurrences across the three files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/workflows/pr-processing.md at line 275, The guidance text embeds a repo-specific assertion "(this repo's `main` currently defines zero required status-check contexts, so `--required` is vacuously green)" which will become stale; update each occurrence (the parenthetical phrase present in the PR-processing workflow and the two related workflow/skill files — five total occurrences across three files) to a conditional phrasing that avoids asserting current branch-protection state, e.g. replace it with "if this repo has no required status-check contexts, `--required` may report green; otherwise use the full `gh pr checks <PR>` list" (or similar) so the guidance remains correct regardless of future branch-protection changes. Ensure the same rewording is applied consistently to all five occurrences.
526-529: ⚡ Quick winClarify behavior when the full check list is also empty.
The guidance requires using the full
gh pr checks <PR>list as the readiness gate when--requiredis vacuously green, but doesn't specify what agents should do if the full list is also empty (e.g., for a repo with no CI configured at all). Consider adding explicit guidance for that edge case.📝 Suggested addition
If `gh pr checks <PR> --required` reports no required checks (this repo's `main` currently defines zero required status-check contexts, so `--required` is vacuously green), do NOT treat that as CI-ready. Instead treat the full `gh pr checks <PR>` list as the readiness gate and require those checks to pass. +If the full list is also empty, treat the PR as having no CI coverage and +document that state in the readiness report or merge evidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/workflows/pr-processing.md around lines 526 - 529, Add an explicit rule handling the edge case where both `gh pr checks <PR> --required` and the full `gh pr checks <PR>` return empty lists: update the guidance to say that this indicates no CI is configured and agents must not treat the PR as CI-ready; instead the agent should pause automated gating and either (a) require explicit human/maintainer approval before merging, or (b) create a clear notification/ticket asking maintainers to confirm merge policy. Reference the commands `gh pr checks <PR> --required` and `gh pr checks <PR>` in the new sentence so readers know exactly when this branch applies..agents/workflows/adversarial-pr-review.md (1)
79-79: ⚡ Quick winSame edge case applies: empty full check list.
As noted in the pr-processing.md review, the guidance doesn't specify behavior when both
--requiredand the full check list are empty. The same clarification would be valuable here for completeness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/workflows/adversarial-pr-review.md at line 79, Add an explicit rule to .agents/workflows/adversarial-pr-review.md clarifying the CI-checks edge case: when both the required-checks command `gh pr checks <PR> --required` and the full checks list from `gh pr checks <PR>` return empty, state that this should be interpreted as "no CI checks required" and the CI readiness gate is considered passed (or specify the alternative fallback you prefer), and update the prose near the existing sentence about `--required` to include this behavior so readers know how the workflow handles an empty full-check list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.agents/workflows/adversarial-pr-review.md:
- Line 79: Add an explicit rule to .agents/workflows/adversarial-pr-review.md
clarifying the CI-checks edge case: when both the required-checks command `gh pr
checks <PR> --required` and the full checks list from `gh pr checks <PR>` return
empty, state that this should be interpreted as "no CI checks required" and the
CI readiness gate is considered passed (or specify the alternative fallback you
prefer), and update the prose near the existing sentence about `--required` to
include this behavior so readers know how the workflow handles an empty
full-check list.
In @.agents/workflows/pr-processing.md:
- Line 275: The guidance text embeds a repo-specific assertion "(this repo's
`main` currently defines zero required status-check contexts, so `--required` is
vacuously green)" which will become stale; update each occurrence (the
parenthetical phrase present in the PR-processing workflow and the two related
workflow/skill files — five total occurrences across three files) to a
conditional phrasing that avoids asserting current branch-protection state, e.g.
replace it with "if this repo has no required status-check contexts,
`--required` may report green; otherwise use the full `gh pr checks <PR>` list"
(or similar) so the guidance remains correct regardless of future
branch-protection changes. Ensure the same rewording is applied consistently to
all five occurrences.
- Around line 526-529: Add an explicit rule handling the edge case where both
`gh pr checks <PR> --required` and the full `gh pr checks <PR>` return empty
lists: update the guidance to say that this indicates no CI is configured and
agents must not treat the PR as CI-ready; instead the agent should pause
automated gating and either (a) require explicit human/maintainer approval
before merging, or (b) create a clear notification/ticket asking maintainers to
confirm merge policy. Reference the commands `gh pr checks <PR> --required` and
`gh pr checks <PR>` in the new sentence so readers know exactly when this branch
applies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d23cce1-ec92-43f2-b278-acc8f6675c21
📒 Files selected for processing (3)
.agents/skills/pr-batch/SKILL.md.agents/workflows/adversarial-pr-review.md.agents/workflows/pr-processing.md
Address advisory review threads on PR #3904 (CodeRabbit approved): - Make the fallback rule runtime-conditioned only ("if `--required` reports no required checks"). The repo-specific standing-fact aside ("`main` defines zero required status-check contexts, so `--required` is vacuously green") was repeated in five places and would silently drift if #3844 option (a) is ever implemented. Drop it from four of the five occurrences; keep it once as an auditable #3844 cross-reference in the canonical pr-processing.md standalone block. - adversarial-pr-review.md gather-list bullet now ends "and require those checks to pass" so the gather list enforces the gate as strongly as the prose above it. - SKILL.md: break the fallback out into its own standalone paragraph immediately after the "Before merge" block, mirroring the standalone treatment in adversarial-pr-review.md and pr-processing.md, instead of burying it mid-paragraph. Doc-only. Refs #3844. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review posted via inline comment tool — see below for details. |
|
Strengths: (1) Correct fix for the vacuous-green --required problem. (2) Consistent three-file application. (3) Self-expiring note in CI Polling section is good hygiene. Issues: (1) Edge case: if gh pr checks PR (the fallback) also returns empty, the fallback is itself vacuously green -- add a note to treat that as UNKNOWN. (2) Style inconsistency: SKILL.md uses a clean standalone paragraph, but pr-processing.md buries the same rule inline in the already-dense Before-merge mega-paragraph. (3) Minor: the self-expiring note about this fallback disappearing once option (a) is configured is only in the CI Polling section of pr-processing.md -- worth a one-liner in adversarial-pr-review.md and SKILL.md too. No blockers -- doc-only, no code/security/perf impact. |
| as CI-ready. Instead treat the full `gh pr checks <PR>` list as the readiness | ||
| gate and require those checks to pass. (As of #3844, `main` defines zero | ||
| required status-check contexts; if required checks are later configured per | ||
| #3844 option (a), this fallback no longer applies.) |
There was a problem hiding this comment.
Edge case not covered: if gh pr checks <PR> (the fallback) also returns an empty list — e.g., before any workflow has been triggered on a brand-new PR — the fallback is itself vacuously green. Suggest adding a sentence:
| #3844 option (a), this fallback no longer applies.) | |
| If `gh pr checks <PR> --required` reports no required checks, do NOT treat that | |
| as CI-ready. Instead treat the full `gh pr checks <PR>` list as the readiness | |
| gate and require those checks to pass; if that list is also empty, treat CI | |
| state as `UNKNOWN`. (As of #3844, `main` defines zero |
This prevents agents from falsely declaring readiness on PRs where checks haven't started yet.
|
|
||
| Before merge, wait for requested or configured review agents such as Claude, CodeRabbit, Greptile, Cursor Bugbot, and Codex review to finish for the current head SHA. Poll CI with bounded commands and timeouts; use narrow required-check commands such as `gh pr checks <PR> --required` for required CI readiness, then also fetch all checks or explicit review-agent checks so non-required reviewers are not hidden. Avoid long-lived `gh ... --watch`. Ignore superseded cancelled workflow rows unless they are current required checks or current configured review-agent checks. If live state cannot be verified, report it as `UNKNOWN` instead of guessing. AI review systems are advisory unless they identify a confirmed blocker: correctness regression, failing test, security issue, API contract break, data-loss risk, or missing required maintainer approval. Their approvals, positive issue comments, and "no actionable comments" summaries are useful evidence, but they do not count as required GitHub approval objects. For high-risk or concurrent-batch PRs, run or request the adversarial PR review workflow in `.agents/workflows/adversarial-pr-review.md`. A completed check is not enough when review comments exist: classify and resolve or explicitly waive actionable findings before merging. Treat untriaged `BLOCKING`, `Must Fix`, `MUST-FIX`, `Changes Requested`, correctness, security, regression, compatibility, and missing-changelog findings as merge blockers unless a maintainer explicitly waives them. | ||
|
|
||
| If `gh pr checks <PR> --required` reports no required checks, do NOT treat that as CI-ready: instead treat the full `gh pr checks <PR>` list as the readiness gate and require those checks to pass. |
There was a problem hiding this comment.
Same empty-list edge case applies here too. Suggest:
| If `gh pr checks <PR> --required` reports no required checks, do NOT treat that as CI-ready: instead treat the full `gh pr checks <PR>` list as the readiness gate and require those checks to pass. | |
| If `gh pr checks <PR> --required` reports no required checks, do NOT treat that as CI-ready: instead treat the full `gh pr checks <PR>` list as the readiness gate and require those checks to pass; if that list is also empty, treat CI state as `UNKNOWN`. |
…t-workflow-docs * origin/main: Treat full check list as CI gate when --required is empty (#3844) (#3904) docs: triage cleanup sweep for #3843 (SKILL.md closeout wording + AGENTS.md rubocop example) (#3905) Revert Pro-side RSC FOUC CSS wrapping from #3587 (#3860) Pin prerelease gems in create app # Conflicts: # .agents/skills/pr-batch/SKILL.md # .agents/workflows/pr-processing.md
1 participant
Summary
Closes #3844.
Workflow docs routed "required CI readiness" through
gh pr checks <PR> --required, butmainbranch protection defines zero required status-check contexts (gh api repos/shakacode/react_on_rails/branches/main/protection/required_status_checks→"contexts": [], "checks": []). So--requiredis vacuously green, and an agent following the letter of the docs could report CI-ready without inspecting any real check.This PR implements option (b) from the issue: amend the CI-readiness polling guidance so that when
--requiredreports no required checks, agents must treat the fullgh pr checks <PR>list as the readiness gate (not advisory).Changes (3 files, doc-only)
.agents/workflows/pr-processing.md— added the--required-empty fallback rule in theCI Polling And Live Statesection (after thegh pr checkscode block, ~L526) and in the mirrored "Before merge, wait for requested or configured review agents…" paragraph (~L275)..agents/skills/pr-batch/SKILL.md— amended the "Before merge…" review-agent paragraph (~L115) with the same fallback rule. The mirrored wording is kept consistent withpr-processing.md. TheCoordinator Closeout Laneregion (~L204) was intentionally not touched..agents/workflows/adversarial-pr-review.md— applied the same amendment to theGround Truth Commandsprose (~L29, the--requiredreference added by Align adversarial review CI polling guidance #3794) and to theIndependent Review Promptgather list (~L79).Conditional added (adapted per file):
Maintainer recommendation (NOT done here — recommendation only)
A maintainer SHOULD separately consider option (a) from #3844: configure the key checks (build / lint / claude-review or equivalent) as required status-check contexts in
mainbranch protection, so--requiredmeans something. That is a maintainer admin action and is intentionally not performed in this PR; this PR only hardens the agent-facing docs (option b).Validation
Doc-only / markdown change — low risk. Heavy
codex review//simplifygates are N/A for this scope (doc-only, no code paths). Verified:npx prettier --checkon all 3 files → "All matched files use Prettier code style!"trailing-newlineshook → all files have proper trailing newlinesmarkdown-links(offline lychee) → 1 OK, 0 errorsprettierhook could not run in the isolated worktree becausepnpm execcannot resolve the prettier binary there; formatting was instead verified directly withnpx prettier --check, which passes.)Concurrent batch note
Concurrent batch: the PR for #3843 also edits SKILL.md at a different region (~L204); whichever PR merges second should
git pull --rebase origin mainand resolve.🤖 Generated with Claude Code
Note
Low Risk
Markdown-only workflow guidance with no runtime, API, or branch-protection changes.
Overview
Closes #3844 by hardening agent workflow docs so CI readiness is not inferred from an empty
gh pr checks <PR> --requiredresult (this repo’smaincurrently has no required status-check contexts, so--requiredcan look green without validating real runs).The same fallback rule is added in three places: when
--requiredreports no required checks, agents must treat the fullgh pr checks <PR>output as the readiness gate and require those checks to pass. Updates land inpr-processing.md(pre-merge polling paragraph and CI Polling And Live State, including a note that the fallback goes away if maintainers later configure required checks per issue option (a)),pr-batch/SKILL.md(Goal Prompt “Before merge” block), andadversarial-pr-review.md(ground-truth commands and independent review gather list).Reviewed by Cursor Bugbot for commit f99ee85. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores