docs: the four ways a CI gate can be unable to fail - #7049
Conversation
All four look fine on the Actions page and none can turn a merge red. Each has bitten this repo, three inside one week: 1. continue-on-error: true -- gc-stress, for months 2. not a required context -- gc-stress again; #6925's regression landed visibly red and survived three merges 3. unconditional cancel-in-progress -- gc-ratchet had three consecutive main runs cancelled, zero executed 4. the gate runs, its subject doesn't -- the dangerous one, because the job is genuinely green. PERRY_GC_FORCE_EVACUATE was inert for every gc()-driven test (#6942/#6946); the matrix --pressure knob disabled the path it measured (#7024); its moved= counter summed two collectors so a cell could pass having run zero copying minors (#7025) The rule that follows from (4): a gate must assert its subject was live, not merely that nothing threw. Corollary noted for (2): a new gate has never been green, so promoting it to required immediately blocks every open PR. Run it once, then promote -- leaving that second step undone is how (2) happens.
📝 WalkthroughWalkthroughAdds project guidance and a changelog entry documenting four CI gate failure patterns, plus a rule to run a gate before making it required. ChangesCI gate guidance
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CLAUDE.md (1)
205-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep detailed incident history out of
CLAUDE.md.The issue chronology and references (
#6925,#6942/#6946,#7024,#7025) belong inchangelog.d/; keep this section focused on the four reusable rules and the required-subject assertion.🤖 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 `@CLAUDE.md` around lines 205 - 214, Update the “Four ways a gate can be unable to fail” section in CLAUDE.md to remove incident chronology, repository-specific job names, and issue references such as `#6925`, `#6942/`#6946, `#7024`, and `#7025`. Retain only the four reusable failure modes, the guidance to assert the gate’s subject was live, and the general rule to run a new gate once before making it required.Source: Coding guidelines
🤖 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.
Inline comments:
In `@CLAUDE.md`:
- Around line 205-214: Revise the cancellation guidance in the four-way gate
checklist so it no longer claims cancellation cannot turn a merge red; state
that required checks can block merging when cancelled, and that cancellation
must be scoped to pull_request so required checks on main are allowed to queue.
Apply the same wording correction in CLAUDE.md (the cancellation item around
lines 205-214) and changelog.d/7047-gate-cannot-fail-patterns.md (lines 1-12);
update both sites directly.
- Around line 205-214: Before merging this documentation change to main, add the
required changelog fragment, update the version field under [workspace.package]
in Cargo.toml, and update the Current Version line in CLAUDE.md to the new
version.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 205-214: Update the “Four ways a gate can be unable to fail”
section in CLAUDE.md to remove incident chronology, repository-specific job
names, and issue references such as `#6925`, `#6942/`#6946, `#7024`, and `#7025`. Retain
only the four reusable failure modes, the guidance to assert the gate’s subject
was live, and the general rule to run a new gate once before making it required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c37dafd-538b-4141-8c79-0c513eccb181
📒 Files selected for processing (2)
CLAUDE.mdchangelog.d/7047-gate-cannot-fail-patterns.md
| ### ★ Four ways a gate can be unable to fail | ||
|
|
||
| All four look fine on the Actions page. None can turn a merge red. When adding or reviewing a gate, check all four — each has bitten this repo, three of them within one week: | ||
|
|
||
| 1. **`continue-on-error: true`** — `gc-stress` carried it for months while being the only job covering GC correctness. | ||
| 2. **Not in branch protection's required contexts** — `gc-stress` again. This is why #6925's `PERRY_PTR_SHAPE_LOCALS=0` regression landed visibly red and survived three merges. A job that reports failure without blocking is documentation, not a gate. | ||
| 3. **`concurrency` with unconditional `cancel-in-progress`** — on a branch with a slow runner queue, every new merge cancels the previous run before it reaches a runner. `gc-ratchet` had three consecutive `main` runs cancelled, zero executed. Scope cancellation to `pull_request` and let `main` runs queue. | ||
| 4. **The gate runs but its subject never did** — the most dangerous, because the job is genuinely green. `PERRY_GC_FORCE_EVACUATE` was inert for every `gc()`-driven test (#6942/#6946); the matrix's `--pressure` knob disabled the very path it was measuring (#7024); its `moved=` counter summed two different collectors, so a cell could pass having run zero copying minors (#7025). **A gate must assert its subject was live**, not merely that nothing threw — e.g. `copied_objects > 0` before a green verdict. | ||
|
|
||
| Corollary: a *new* gate has never been green, so promoting it to required immediately blocks every open PR. Run it once, then promote. Leaving that second step undone is how (2) happens. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
GitHub status checks required branches cancelled check blocks merge not satisfied pending
💡 Result:
On GitHub, a required status check must reach a successful conclusion for a pull request to be merged into a protected branch [1][2]. If a required status check is in a "cancelled" or "pending" state, it will block the merge [3][4][5]. The following details clarify how these states affect merging: Pending: A status check remains in a "pending" state if it has not yet reported a result, or if the workflow was skipped (e.g., due to path or branch filtering) [4][6]. Because the check has not reached a successful conclusion, it blocks the merge [3][4]. Cancelled: If a required status check is cancelled before it completes, its conclusion is "cancelled" [2]. This does not satisfy the requirement for a successful, skipped, or neutral conclusion, and therefore it blocks the merge [1][2][5]. To resolve these issues, you must ensure the required check is triggered and reaches a successful, skipped, or neutral conclusion [1][2][3]. If a check is stuck in "pending" or "cancelled," you may need to: - Rerun the workflow or check to ensure it completes successfully [2][5]. - Verify that your workflow filters (such as paths or branches) are not causing the check to be skipped unexpectedly [4][6]. - Ensure that the required check name matches exactly what is defined in your branch protection rules [3][5]. - If you are using merge queues, ensure your workflows include the merge_group event trigger [7][8][6].
Citations:
- 1: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches?require-status-checks-before-merging=
- 2: https://docs.github.com/en/pull-requests/reference/status-checks
- 3: https://adhdecode.com/debugging/github-actions/branch-protection-required-status-check-failed/
- 4: https://docs.github.com/en/enterprise-server@3.4/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/troubleshooting-required-status-checks
- 5: https://adhdecode.com/debugging/github-actions/required-status-check-failed/
- 6: https://docs.github.com/en/enterprise-server@3.13/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks
- 7: https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks
- 8: https://docs.github.com/en/enterprise-cloud@latest/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks
Correct the cancellation wording for required checks.
A cancelled required status check can block merging because it is not a successful/skipped/neutral conclusion. Reword the cancellation item to say the rule is to scope cancellation so required checks on main are not cancelled, rather than claiming it universally cannot turn a merge red. Apply this to both CLAUDE.md and changelog.d/7047-gate-cannot-fail-patterns.md.
📍 Affects 2 files
CLAUDE.md#L205-L214(this comment)changelog.d/7047-gate-cannot-fail-patterns.md#L1-L12
🤖 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 `@CLAUDE.md` around lines 205 - 214, Revise the cancellation guidance in the
four-way gate checklist so it no longer claims cancellation cannot turn a merge
red; state that required checks can block merging when cancelled, and that
cancellation must be scoped to pull_request so required checks on main are
allowed to queue. Apply the same wording correction in CLAUDE.md (the
cancellation item around lines 205-214) and
changelog.d/7047-gate-cannot-fail-patterns.md (lines 1-12); update both sites
directly.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '^\[workspace\.package\]|^version\s*=' Cargo.toml
rg -n '^Current Version' CLAUDE.md
git diff --name-only -- Cargo.toml CLAUDE.md changelog.d/7047-gate-cannot-fail-patterns.mdRepository: PerryTS/perry
Length of output: 200
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Top-level tracked files:"
git ls-files | sed -n '1,80p'
echo
echo "CLAUDE.md current version occurrences:"
rg -n -C 2 '^current version|^Current Version' CLAUDE.md || true
echo
echo "changelog.d contents count and names:"
git ls-files 'changelog.d/*' | sed -n '1,120p' | wc -l
git ls-files 'changelog.d/*' | sed -n '1,160p'
echo
echo "Relevant git status and diff names:"
git status --short
git diff --name-only -- Cargo.toml CLAUDE.md changelog.d/ 2>/dev/null || true
git diff --cached --name-only -- Cargo.toml CLAUDE.md changelog.d/ 2>/dev/null || true
echo
echo "Search references to gate patterns or added lines:"
rg -n "Four ways a gate can be unable to fail|PERRY_GC_FORCE_EVACUATE|gc-ratchet|continue-on-error: true|concurrency|cancel-in-progress|required contexts" CLAUDE.md CHANGELOG.md changelog.d Cargo.toml 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 28604
Add the required version metadata before merging to main.
This Markdown-only change is not tied to a Cargo.toml version bump. Include the changelog fragment, update [workspace.package].version, and update CLAUDE.md’s Current Version line as part of the merge commit.
🤖 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 `@CLAUDE.md` around lines 205 - 214, Before merging this documentation change
to main, add the required changelog fragment, update the version field under
[workspace.package] in Cargo.toml, and update the Current Version line in
CLAUDE.md to the new version.
Source: Coding guidelines
1 participant
A
CLAUDE.mdaddition under "CI gates that surprise people". No code.Why
Four structurally different ways a gate can be unable to fail have now bitten this repo — three of them inside one week. They all look fine on the Actions page and none can turn a merge red, so each was found by accident rather than by anyone noticing the gate was toothless.
continue-on-error: truegc-stress, for months, while being the only job covering GC correctnessgc-stressagain — why #6925'sPERRY_PTR_SHAPE_LOCALS=0regression landed visibly red and survived three mergesconcurrency+ unconditionalcancel-in-progressgc-ratchethad three consecutivemainruns cancelled, zero executed, because every new merge cancelled the previous one before it reached a runner on a 40-minute queue(4) is worth stating separately because it is invisible to every review process we have.
PERRY_GC_FORCE_EVACUATEwas inert for everygc()-driven test — it is read only on the minor path whilegc()runs a full mark-sweep (#6942/#6946). The GC matrix's--pressureknob disabled the very path it was measuring (#7024). Itsmoved=counter summed two different collectors, so arequires=movecell could pass having run zero copying minors (#7025).The rule that follows: a gate must assert its subject was live, not merely that nothing threw — e.g.
copied_objects > 0before a green verdict.And a corollary for (2): a new gate has never been green, so promoting it to required immediately would block every open PR. Run it once, then promote. Leaving that second step undone is exactly how (2) happens —
gc-ratchetis in that state right now.Scope
Documentation only.
CLAUDE.mdis 219 lines, well under the 2000-line cap.Note
scripts/check_file_size.shexits 1 on this branch for 8 pre-existing files unrelated to this change (typed_abi.rs,index_get.rs,native_module.rs,array/generic.rs,layout_trace.rs,callable_export_check.rs,callable_exports.rs,constants.rs) — that is one of the four stackedlintfailures currently blocking every merge, and is not addressed here.Summary by CodeRabbit