Skip to content

ci: self-edit guard reads what the pull request changes, not its copy of the file - #273

Merged
Babissimo merged 3 commits into
mainfrom
worktree-ci-guard-uses-pr-diff
Aug 27, 2026
Merged

Babissimo merged 3 commits into
mainfrom
worktree-ci-guard-uses-pr-diff

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

The self-edit guard mirrors the action's refusal to run on a pull request that edits its reviewer, so the completion check below it does not fail a pull request over a review that was never allowed to happen. It decided that by hashing the checked-out workflow and comparing it to the copy on the default branch.

A branch carries its own copy of every file. One cut before this file last changed still holds the old copy, untouched, so the hashes differ and the guard reads a stale branch as an edit. It was testing branch age, not what the pull request does.

The misfire skips the completion check, which is the only thing telling a finished review from one that stopped partway, and which exists because 7 of 35 sampled runs stopped partway. The pull request then goes green whether or not it was reviewed, under a notice saying it edits a file it does not touch.

Measured

The three pull requests open when #245 landed, all reviewed after it:

head's copy of this file guard tripped completion check
#270 current, branch rebased past the merge no ran, logged Review completed
#271 pre-#245 yes skipped
#272 pre-#245 yes skipped

#272 changes 11 files, none under .github. Its review did finish, 5 of 5 ticked, but nothing checked that. #271 is the only pull request carrying two foldable comments, so under the old guard the one case that could exercise the fold is a case where the fold never runs.

It clears as branches pull in main, which is why #270 behaves. So it is a window rather than a permanent break, but the window reopens every time this file changes.

The change

Ask GraphQL which paths the pull request changes and match them against WORKFLOW_PATH. A stale branch answers correctly, because it genuinely is not changing that file.

Names only, rather than gh pr diff --name-only, which fetches the whole patch to read them: 133,752 bytes against 359 for the same answer on #272. Size is not the objection. The API refuses to generate a diff at all once one grows large enough, so the guard would stop running on precisely the bulk changes it most wants to see.

That caps at 100 files per page, so hasNextPage rides the first line of the same output and warns when the guard has read only part of the list. Position separates it from the paths, so no filename can be read as the flag.

Also here:

  • The failure branch quotes gh's reason, having previously sent it to /dev/null. A rate limit, a diff the API would not build, and a token without pull-requests: read read identically and want different responses. Newlines are flattened first, or the workflow command truncates at the first one.
  • WORKFLOW_PATH replaces three hard-coded copies of this file's own path with one env var the fold's origin check reads too. One of the three was a git hash-object call that would have failed the step with a bare git error on a rename.
  • The fold's copy is read inside fold_superseded rather than at module scope. Only the fold uses it, and the rule stated in the file is that folding must never redden a review that passed; at module scope a missing value raised before the try that enforces that.

This pull request will not be reviewed, and that is the point

It edits claude-code-review.yml, so the action refuses itself and the guard skips the completion check. That is the guard working: this is a genuine self-edit, which is the case it exists for. Watch the run for the ::notice:: rather than for a review.

Which also means the change cannot be exercised in its own CI. Verified instead by parsing the step out of the YAML and running it against the live API: #272 reaches the gate, #245 skips as it must, and an unknown number warns with gh's reason on one line and then reaches the gate, so a failed lookup fails towards demanding a review rather than passing unreviewed. The block also passes bash -n, and the Python it wraps still compiles.

Ticket

86cbacz09 planned to "extend the self-edit guard to hash the script as well as the workflow" when the Python moves to .github/scripts/. There is no hashing left to extend, so its description is amended: the extension is one more exact match against the same path list, and reintroducing hashing would put this misfire back on two files that misfire independently.

The fold itself is still unexercised. It needs a pull request that does not edit this workflow, pushed twice.

Babissimo and others added 3 commits August 27, 2026 14:37
The self-edit guard mirrors the action's own refusal to run on a pull
request that edits its reviewer, so the completion check below does not
fail a pull request over a review that was never allowed to happen. It
decided that by hashing the checked-out workflow and comparing it to the
copy on the default branch.

A branch carries its own copy of every file. One cut before this file
last changed still holds the old copy, untouched, so the hashes differ
and the guard reads a stale branch as an edit. It was testing branch age,
not what the pull request does.

That misfire skips the completion check, which is the only thing telling
a finished review from one that stopped partway, and which exists because
7 of 35 sampled runs stopped partway. The pull request then goes green
whether or not it was reviewed, under a notice saying it edits a file it
does not touch.

Measured on the three pull requests open when #245 landed: #270, rebased
past the merge, kept its gate and logged "Review completed"; #271 and
#272, both cut earlier, carry the pre-#245 blob and both tripped the
guard. #272 changes 11 files, none under .github. Its review did finish,
5 of 5 ticked, but nothing checked that.

Asking gh which files the pull request changes answers the actual
question, and a stale branch answers it correctly. Verified by running
the block against #272 (reaches the gate), #245 (skips, as it must), and
an unknown number (warns, then reaches the gate, so a lookup failure
fails towards demanding a review rather than passing unreviewed).

This also drops two of the three copies of this file's own path. The one
that remains is an env var the fold's origin check now reads too, so a
rename is one edit rather than three, one of which used to fail the step
with a bare git error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit, applied.

The guard asked `gh pr diff --name-only`, which fetches the entire patch
and reads the names off it: 133,752 bytes to learn 11 filenames on #272.
Size is not the objection. The diff endpoint refuses outright once a diff
grows large enough, so the guard would stop running on precisely the bulk
changes it most wants to see, and the branch would fall through to the
warning. GraphQL answers the same question in 359 bytes, has no such
ceiling, and `gh api graphql` is already the idiom two calls below.

That caps at 100 files per page, so hasNextPage now rides on the first
line of the same output and warns when the guard has only seen part of
the list. Position separates it from the paths, so no filename can be
read as the flag.

The failure branch quoted no reason, having sent stderr to /dev/null. A
rate limit, a diff the API would not build and a token without
pull-requests: read all read identically, and they want different
responses. Newlines are flattened first, or the workflow command would
truncate at the first one.

WORKFLOW moved from a module-level env lookup back inside
fold_superseded, as a local. Only the fold uses it, and the file's rule
is that folding must never redden a review that passed; at module scope a
missing WORKFLOW_PATH raised KeyError before the try/except that enforces
that. It was a literal until the previous commit, so this restores the
property rather than adding one. 86cbacz09 plans to move this Python to
.github/scripts/, which is exactly when an env var stops being guaranteed.

Verified as before, by running the block against #272 (reaches the gate),
#245 (skips), and an unknown number (warns with gh's reason on one line,
then reaches the gate).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo marked this pull request as ready for review August 27, 2026 22:13
@Babissimo
Babissimo merged commit 78e8d81 into main Aug 27, 2026
15 checks passed
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