Repository navigation
The review guard called a fix awaiting release an attack: a merged dev PR that changed claude-review.yml went red on every dev PR as 'review disabled repo-wide'. A third arm reads the base's copy back to the merged PR that produced it and says 'pending release'; the hostile arm is unchanged and a lookup that dies there fails closed - #3673
Merged
Merged
Conversation
… never ran and the PR did not touch claude-review.yml, the drift arm had one verdict, "review disabled repo-wide", for both a merged dev PR waiting on the next release and an unreviewed edit nobody read. A third arm reads the base branch's copy back to the merged PR that produced it and says "pending release" instead; the hostile arm is unchanged, and a lookup that dies there fails closed PRs to main come only from dev (check-pr-branch.yml), so a fix to the review workflow cannot land on main and dev together outside a release: it merges to dev through a reviewed PR and every dev PR is skipped by claude-code-action until the release ships it. #3657 did exactly that tonight and every dev PR went red for it. The arm now asks provenance, not content: the newest commit on the base touching the file, the merged PR that introduced it, that PR's diff including the file, and the base's blob equal to the blob at its merge. All three hold: warning, exit 0, still a human's review. Any fails, or no PR claims the commit: the existing exit 1, text unchanged. Every read goes through lookup_closed(), a fail-CLOSED twin of the #2309 lookup(), because passing on a 503 would be the false verdict.
erikdarlingdata
added a commit
that referenced
this pull request
Sep 19, 2026
… PRs across the #3653, #3542 and #3691 waves, #3704, #3710, #3712 and #3735, spliced once (#3751) Sixty-five PRs merged to dev since #3672 (the first splice) across two seats - the coordinator's #3653 brains-review residue lanes and the wave-2 worker's #3542 / #3691 PostgreSQL-target engine lanes - and by rule none touched this file; each returned its entry to a buffer. This is the one splice. Text-only; one file; nothing outside ## [Unreleased] and the definition block moves. [Unreleased] gains a ### Changed between ### Added and ### Fixed (Keep-a-Changelog order) for the nine entries whose subject is a designed behaviour that now behaves differently - routing (#3712 confidence chooses the channel), gating (#3692 tempdb persistence, #3734 blocking/LRQ gates, #3724 peak-and-mean), the wire shape (#3719 one error envelope), the review guard's drift arm (#3673), lineage metadata (#3723) and two grading changes (#3693, #3747). Twenty-two new capabilities append to ### Added (the sixteen PostgreSQL-target fact families and rungs, CONFIG_CHANGED, the recurrence label, the V134/V135 columns, the review traps file, the measurement census); thirty-two defects append to ### Fixed. Order within each section is ascending PR number, as #3672 did. Every entry is one line and verbatim from its buffer (trailing whitespace only). Four PR pairs fold into one entry as the buffers wrote them (#3738+#3746, #3683+#3686, #3681+#3685, #3726+#3729); #3695 carries three entries. #3673 has no issue and cites none, like the file's ninety-six other ref-less entries. Five definitions added ([#3691], [#3704], [#3710], [#3712], [#3735]) at the end of the [Unreleased] run, which stays one ascending run (3517..3735). No archive touched; the split was not run. tools/changelog/changelog_archive.py verify: all PASS (1,483 bold entries, floor 1,329; 1,363 distinct refs resolve; 1,372 definitions; CRLF throughout; 409,263 bytes under the ceiling). Client-hygiene grep over the file: zero hits.
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
1 participant
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.
The lie
The
Claude review guardhas a never-ran arm for the case whereclaude-code-actionrefuses to run because.github/workflows/claude-review.ymldiffers from the default branch (cause (1) in the header). It had two verdicts: if THIS PR touches the file, a warning ("a human must review it", exit 0); otherwise::error title=Review disabled repo-wideand exit 1. That second verdict was built for one thing -- an unreviewed copy of the file sitting on a branch -- and tonight it fired on every dev PR for something else entirely: #3657, a reviewed and merged dev PR, changed the file, and dev has not been released to main yet. The arm could not tell the two apart because it never looked at where the base branch's copy CAME FROM. It only knew the file differed.Why this window is a normal state, not an incident
check-pr-branch.ymlallows pull requests to main only from dev. So a fix to the review workflow cannot land on main and dev together outside a release: it merges to dev through a reviewed PR, and from that moment until the next dev -> main release every dev PR is skipped by the action. That is the branching model working as designed. A guard that goes red on all of them for the whole window is a mark people learn to ignore -- which the guard's own header names as the failure that costs more than it catches.The three arms
$WFwas put there by a merged pull request into the base. The read is provenance, not content: the newest commit on$BASEtouching$WF(commits?sha=$BASE&path=$WF), the pull request that introduced that commit (commits/<sha>/pulls, filtered tomerged_at != nullandbase.ref == $BASE, because once a release PR from dev is open the commit is associated with it too), and then three facts held together: it merged into$BASE, its diff includes$WF, and$BASE's blob of$WFequals the blob at its merge commit -- the branch carries exactly what that merge produced and nothing after it. All three hold:::warning title=Review pending releasenaming the PR, a summary line, exit 0. The PR is still NOT reviewed and the message says so.$BASEintroduced the commit (a direct push), when the PR that did does not account for the branch's current copy, or when the base already matches the default branch and nothing visible from here explains the skip. Each of those leaves its own evidence line in the step summary before the existing error.And a fourth outcome that is not a verdict: drift origin unknown. Every read in the new arm goes through
lookup_closed(), a twin of the #2309lookup()helper that differs ONLY in what a failed call becomes.lookup()warns and exits 0 because a lookup failure is not a review verdict;lookup_closed()errors with::error title=Drift origin unknownand exits 1, because in this arm passing IS the false verdict -- a hostile drift waved through on a 503. The two share onegh_capture()so the stderr-isolation and mktemp-tolerance fixes from #2309's review live in one place.Why provenance by commit, not by scanning recent PRs
The obvious shape is to page the most-recently-updated closed PRs into dev and look for one that touched the file. Measured against the live repo two hours after #3657 merged, #3657 was already at index 10 of 30 in that ordering (every PR that gains a comment moves ahead of it), and this repo merges a train a night -- so inside a day the fix falls out of any fixed window and the arm flips back to a false hostile for the rest of the drift window. The commits-with-path read is exact regardless of the branch's age, costs three to five calls instead of up to thirty-one, and
commits/<sha>/pullsis documented to return the merged PR that introduced a commit not yet on the default branch. Thepulls/<n>/filescheck is kept as an independent fact so the verdict never rests on the association alone.Live runs against the repo as it stands tonight
The harness is the guard's
run:block extracted with a YAML parser (the same reading Actions does), helpers and the never-ran arm taken by line range and proven identical bydiffbefore each run.R=erikdarlingdata/PerformanceMonitor DEFAULT_BRANCH=main WF=.github/workflows/claude-review.yml PR=3670(a real dev PR that does not touch the file),never_ran=1preset. Only the stubbed cases replace onegh apiendpoint each; everything else hits the live API.Case 1 -- live,
BASE=dev(dev bloba700b3b9≠ main blob06430dc3; newest dev commit touching the file is1dae2ade, #3657's squash):Case 2 -- live,
BASE=main(base equals default; pending arm skipped, falls to the hostile arm):Case 3a -- hostile, wrong PR (stub:
commits/<sha>/pullsclaims #3668, a real merged dev PR that did NOT touch the file; thefilesand blob reads are live):Case 3b -- hostile, direct push (stub:
commits/<sha>/pullsreturns[]):Case 4 -- fail closed (stub: the commits-with-path call returns HTTP 503):
Regression -- the refactored fail-OPEN
lookup()(stub:pulls/$PR/filesreturns HTTP 502):actionlintandshellcheck -s bashon the extracted block: both clean.What it does NOT do
claude-review.yml. This guard never invokesclaude-code-action(header, lines 24–27), so editing it causes no drift refusal anywhere; this PR is itself Claude-reviewable in principle and is only skipped tonight because of the very window it names.::error, same summary line, same exit 1, and it is reached in every case the new arm cannot positively account for -- including the case where the base already matches the default branch and the skip is unexplained.contents: readandpull-requests: readwere already granted; the contents, commits, and commit-pulls endpoints need nothing more.No issue exists for this; the PR body is the record.