Skip to content

fix(ci): fail ci-status when the kill switch stopped a review lane - #9836

Merged
kyle-sexton merged 5 commits into
mainfrom
fix/ci-status-kill-switch-stopped-review
Oct 24, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
fix/ci-status-kill-switch-stopped-review

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner rule that review lanes are the gate; raised by GitHub Support

Summary

ci-status treated a review lane the kill switch stopped as not required and passed. While CLAUDE_LANES_DISABLED, CLAUDE_LANES_REPO_DISABLED, or a lane's own CLAUDE_REVIEW_DISABLED / CLAUDE_SECURITY_REVIEW_DISABLED was on, any head pushed got a stopped review, passed ci-status, and native auto-merge could merge it unreviewed. Owner rule: review lanes are the gate; the kill switch pauses work and never waives review. The admin bypass stays the break-glass.

Fix

  • Require both review lanes on this head in .github/workflows/pr-require-checks.yml: the stopped verdict now emits ::error::'<check>' did not run on <head> (<run url>): a kill switch was on when it started. Once it is off, re-run all jobs of <run url>; ci-status re-runs when it finishes. and sets failed=1. The verdict reflects the switch when the review run started. A trusted person re-running that run fires rerun-ci-status (pr-feedback-dispatch.yml), so the fix needs no empty push.
  • The review-run lookup only takes runs whose head_repository.full_name is this repository, the same binding rerun-ci-status applies. Before, a fork's run on the same commit could be the newest and decide the lane. This bug predates the PR. A PR whose head repository is not this repository fails up front with the existing "a fork ... gets no review" line, before any run is read, so the filter never turns a fork PR into "missing".
  • The step's comment block, docs/conventions/pr-pipeline/README.md ("Both lanes review the head") and a pr-pipeline changelog fragment now say the same.

Verification

  • bash scripts/ci-review-lanes.test.sh: with the new tests against the old workflow step, the three kill-switch cases, two fork-run cases (a newer fork run with the same SHA is ignored; a fork run alone leaves the lane missing) and a fork-PR case (fails as a fork, reads no run) each fail before their fix. With every fix in place: PASS=60 FAIL=0.
  • actionlint .github/workflows/pr-require-checks.yml: exit 0.
  • scripts/check-changelog-fragments.sh --check and --check-required origin/main: both pass.
  • The Python suites check-affected --tests selects pass under python3 -m unittest (babysit-prs 2877 OK, contract-clause-coverage 24 OK). pytest is missing on the authoring host, so the pull-request skill's verify gate could not record a pass.
  • Left out: no live kill-switch trip was run. The step body is executed by the test against stub gh.

Related

  • Searched pr-require-checks.yml and the merge-lane scripts for other places that treat a kill-switch stop as a pass and found none. The lanes-App kill-switch step in ci-status already fails. clear-escalations, enable-auto-merge and refire-refused stop on the switch instead of passing. The cost-hold reader in ci-status needs a recorded hold, so a stopped lane cannot clear it.
  • docs/conventions/pr-pipeline/CHANGELOG.md 4.x still describes the old behavior. That is released history and is left as is; the fragment records the change.

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits October 24, 2026 14:25
The review-lanes step in ci-status treated a lane the kill switch
stopped as not required, so a head pushed while CLAUDE_LANES_DISABLED,
CLAUDE_LANES_REPO_DISABLED or a lane's own switch was on passed
ci-status with no review and could auto-merge. A stopped lane now fails
with "'<check>' did not run on <head> (<run>): the kill switch is on.
Re-trigger the PR once it is off." The kill switch pauses review; it
never waives it. The admin bypass stays the break-glass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o's head

The kill-switch verdict reflects the switch when the review run started,
and "re-trigger the PR" invited an empty push. The error now says to
re-run all jobs of the stopped run once the switch is off; a trusted
re-run fires rerun-ci-status, which re-runs ci-status.

The review-run lookup also took a fork's pull_request run on the same
commit when it was the newest. It now keeps only runs whose head
repository is this repository, as rerun-ci-status does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The head-repository filter on the review-run lookup would report a fork
pull request's lanes as missing. When the pull request's head repository
is not this repository, ci-status now prints the existing "a fork ...
gets no review" line for each lane and fails before reading any run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep main's pass-notice and unproven verdicts (#9813) beside this
branch's failing stopped verdict. The fork check reads main's new
three-column lanes list. Main's two kill-switch notice tests now expect
a stop to fail.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 24, 2026 20:44
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T20:45:12.955007Z dc4eff4 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kyle-sexton
kyle-sexton enabled auto-merge October 24, 2026 20:44
@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed dc4eff4de4b5369c59126ca2719d18b761fdedce: 0 finding(s), 0 as inline comments.

No findings. I reviewed all four changed files and the PR context. I did not run anything: the lane has no shell, and the test results in the PR description are author-claimed and unverified by me.

What I checked

  • Kill-switch verdict (.github/workflows/pr-require-checks.yml:4140-4143): stopped now emits an error and sets failed=1. The message names the check, head and run URL, and says how to recover. The killunlisted path that resolves to stopped through the report notice gets the same result.
  • Fork filter: the run lookup now requires .head_repository.full_name == $GITHUB_REPOSITORY (lines 4096-4098), so a fork's run on the same SHA can no longer decide a lane.
  • Fork-PR early exit (lines 4084-4091): it runs after the exempt-author branch, so exempt-author handling is unchanged. $lanes is defined before it. read -r _ _ check gives the full check name on both lane lines. An empty HEAD_REPO (deleted fork) fails as "unknown", which is the safe direction. The step only runs on pull_request events (line 4015), so a merge-group event cannot hit the early exit with an empty HEAD_REPO.
  • Re-run claim: the new error text says a re-run of the stopped review run re-triggers ci-status. rerun-ci-status in pr-feedback-dispatch.yml:321-329 matches both lanes' callers on pull_request runs from this repository and excludes only skipped conclusions, so a completed re-run does reach it.
  • Tests (scripts/ci-review-lanes.test.sh): the run helper now carries head_repository, and HEAD_REPO is passed to the step body. New cases cover a newer fork run being ignored, a fork run alone leaving the lane missing, a fork PR failing as a fork with no /actions/ read, and both lanes stopped. The three old pass-on-stop cases now expect rc=1.
  • Docs and changelog: the workflow comment, docs/conventions/pr-pipeline/README.md and the .changes/pr-pipeline fragment agree with the code. The bump: patch level fits a fix to ci-status behavior.

Closing issue: none. The PR context says "No related issue", so there are no acceptance criteria to check.

Plugin manifests: the diff touches none, so no manifest <field> lines apply.

Trusted paths: no changed file matches the listed globs.

Embedded directives: the PR body contains none.

Cost: nothing here can start a real charge, so there is no hold.

I left out one style point: the README edit joins the kill-switch sentence onto an already long line (docs/conventions/pr-pipeline/README.md:103). A formatter would catch it, so it is below this lane's bar.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed dc4eff4de4b5369c59126ca2719d18b761fdedce: 0 finding(s), 0 as inline comments.

No security issues found in this pull request.

The change tightens the ci-status gate. A review lane that a kill switch stopped now fails the check instead of passing. The review-run lookup now accepts only runs whose head_repository.full_name is this repository, and a pull request whose head is a fork fails up front.

  • Kill-switch verdict (pr-require-checks.yml): the stopped case now sets failed=1. This closes the unreviewed-merge path the description names. Nothing in the diff weakens a gate elsewhere.
  • Fork binding: HEAD_REPO comes from github.event.pull_request.head.repo.full_name. It is passed through env:, not interpolated into the run: script, so there is no expression injection. The ::error:: echo does write the value into a workflow command, but a repository full name can't contain a newline. A null head repository (a deleted fork) gives an empty string, which fails the comparison and so fails closed. A casing mismatch in the comparison would also fail closed, never open.
  • Run filter: the jq filter adds .head_repository.full_name == $repo to the existing path, SHA and event checks. A fork's run on the same commit can no longer be the newest run and decide a lane. This is a hardening change.
  • Test and docs changes: scripts/ci-review-lanes.test.sh, the pr-pipeline README and the changelog fragment only restate the new behavior and add test cases. The tests stub gh, so they run no live code.
  • Instruction surface: the README and comment edits remove no standing instruction. They record a stricter rule, and the workflow step enforces it.
  • Trusted paths: none of the four changed files matches a trusted-path glob.
  • Manifests: no plugin manifest changed, so there are no manifest <field> lines.

The protected-class list in docs/conventions/instruction-exception-register/README.md was not consulted, because no standing instruction was deleted or softened.

The change has no cost or billing effect.

critical-findings: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant