docs(agents): document how to verify a "superseded" closure claim - #1906
Merged
Conversation
"repair, don't close" is already a standing convention, but the *verification method* was never written down -- so a closure whose comment reads convincingly could discard real, tested delta. Records the commands that actually caught one (#1661, closed as "superseded by protected main" while a ~450-line concurrency fix and 3 of 4 cited doctoring docs were provably absent from main): git grep -l "<symbol>" origin/main --, git show origin/main:<path>, and git diff --stat origin/main <head> because a long-lived branch's title records what it was opened for, not what it now contains. Narrowing a PR into successors is the same claim and needs the same proof. Also records why cancel-in-progress: true is safe on a PR-scoped group once an admission job re-verifies the live head -- the SHA-suffix and cancel-in-progress: false workarounds are then unnecessary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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 |
…ion-before-close # Conflicts: # AGENTS.md
2 tasks
…odel audit An independent read-only Codex audit of the first draft found seven factual problems. Corrected here rather than shipping guidance that would mislead: - Dropped the cancel-in-progress bullet entirely. It claimed an in-workflow admission job makes cancel-in-progress: true safe, but workflow-level concurrency cancels before any job runs, so no job can precede it. It also contradicted the existing "put concurrency at workflow scope" guidance, and the SHA-suffix point was already covered two bullets above. - git diff now uses three-dot (origin/main...<head>). Two-dot reports main's own newer commits as phantom deletions by the PR -- precisely wrong for the stale PR this section is about. - git grep gains -F (it is a regex otherwise), and no longer claims absence proves the delta is missing: a successor may have renamed or restructured it, and a match does not prove behaviour was inherited. - git show now says the path exists *now*; a non-zero exit does not mean the content never landed, since it may have landed and later been deleted. - The closure conditions are quoted from the repo's actual documented rule in docs/org-required-workflow-rollout.md instead of an invented four-item list. - Dropped an unnamed "several PRs" claim, and qualified the reopen advice: inconclusive evidence is not disproven succession. Adds the ancestry caveat: git merge-base --is-ancestor answers a different question, and is unreliable in both directions here because this repository mixes squash merges with real merge commits (measured: 153 single-parent vs 47 two-parent over 200 commits). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
#1906 landed "Verifying a 'superseded — closing' claim", which already covers ancestry-vs-content in more depth than my draft's version did (three-dot diff, `git grep -lF`, `git show origin/main:<path>`, and the squash/merge-commit mix that makes `git merge-base --is-ancestor` unreliable in both directions). Removed my duplicate bullet and pointed at that section instead. What remains here is the part a single-PR closure check does not cover: a PR split across several successors fails differently, because no individual successor looks wrong. Added the union question explicitly, and kept the end-to-end check — git-level commands show whether text moved, not whether behaviour is restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
AGENTS.mdalready requires "verified complete successor carryover of every unique valid delta" before retiring a PR (docs/org-required-workflow-rollout.md), but never said how to verify it — so a closure whose comment reads convincingly can discard real, tested delta. This adds the verification method, and the caveats that make each command trustworthy.The motivating case:
.github#1661was closed as "superseded by protected main… later focused PRs also landed the queue/concurrency, telemetry, credential, and review-gate slices represented in this branch." Checking that against the tree instead of reading it:git grep -lF "cancel-superseded-noema-runs" origin/main --returned nothing (a ~450-line Actions concurrency-deadlock fix, with tests, was on no other branch), and 3 of the 4 doctoring docs the PR body cited were absent frommain. It was reopened with that evidence. Nothing in the closing comment's wording distinguished it from a correct one — only running the commands did.This PR was itself corrected by a cross-model audit
The first draft was audited by an independent read-only
codex execpass, which found seven factual problems. That is worth stating plainly, because it is direct evidence for the cross-model review bullet peer 3 landed in#1907:cancel-in-progress: truesafeAGENTS.md's existing "put concurrency at workflow scope" line, and the SHA-suffix point was already covered two bullets above. Bullet dropped entirely.git diff --stat origin/main <head>shows the PR's scopemaingained since the fork point as deletions by the branch, so a stale PR reads as a mass revert. Now three-dot.git show origin/main:<path>failing means it "never landed"git grep -l "<symbol>"proves inheritance-F, with the inference qualified.A peer then corrected the correction: the added ancestry caveat first claimed this repo squash-merges everything, generalized from eight commit titles. A parent-count census settled it — 153 single-parent vs 47 two-parent over 200 commits — so the repo mixes both, and the accurate statement is that
git merge-base --is-ancestoris unreliable in both directions here.What this adds
AGENTS.md— "Verifying a 'superseded — closing' claim": three-dot diff for real scope,git grep -lFandgit showwith their actual evidentiary limits, the ancestry caveat, and a qualified reopen instruction.CLAUDE.md— a pointer plus the two repo-specific traps (two-dot diff,--is-ancestor).Doc-only. No code, no behaviour change.
Test plan
coverage run -m pytest tests -q— 2883 passed, 1 skipped, 21 subtests passedAGENTS.md/CLAUDE.mdcontent — 118 passedCoordination
Part of a five-session split of this documentation work, merging sequentially to avoid conflicts in these two files. Stacked behind peer 3's
#1907(merged, already rebased onto). Peer 2's test-gate section rebases behind this one.🤖 Generated with Claude Code