docs(pr-review-loop): encode review-sweep defects found in the 2026-07-23 sweep - #6576
Merged
Conversation
…7-23 sweep Four failure modes from a live authorized-maintainer sweep, each of which produced a defective posted review: - Dispatching Review Agents (new): a subagent's final assistant text is not delivered to the lead — findings arrive only via SendMessage, and charters must say so verbatim. Idle means resumable, not dead. Do not substitute a lead-authored review for a slow agent; three of the sweep's four bad reviews came from doing exactly that, and each was overturned by the agent's report minutes later. - Verifying the Review's Own Evidence (new): the CR grep-verification bar binds the reviewer as hard as the contributor, since the contributor acts on what the review says. Check repo convention before proposing a citation, and read the rule rather than trusting a number in roughly the right section — adjacent subrules routinely cover unrelated single cards (612.5 is Exchange of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a different section from the effect it orders (613.1c, not 612.x). - Judging the seam (new, under Review Bar): "reuses existing machinery" is not "correct seam". Every proliferation heuristic in this file returned clean on a PR that reused the instance-blind checker while the instance-aware authority twenty lines away already cited the rule and dropped half of it. Grep for the rule, not the symbol; two mechanisms for one rule is the defect. - Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's own new variants look like contributor sloppiness and are frequently ours — main adds a new match site after the contributor's last push. Timestamp the failing site against the head before writing that finding. - Review Freshness: stale approval plus armed auto-merge is a safety defect, not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting APPROVED after a new commit lands, so an unreviewed head can sit armed to merge. Worse when the post-approval commit is maintainer-authored.
matthewevans
enabled auto-merge
July 23, 2026 23:36
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe review-loop skill documentation adds policies for delegated-agent reporting, evidence verification, stale-approval safety, compile-failure attribution, and identifying the correct enforcement seam. ChangesReview Loop Policy
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/pr-review-loop/SKILL.md:
- Around line 131-135: Update the push-time lookup in the PR review loop to use
PR timeline or update events that record when the head was pushed, rather than
`.commit.committer.date`; if the implementation intentionally keeps the commit
timestamp, rename its field and related usage to clearly indicate commit time
instead of push time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86726889-0f88-400a-88da-d20e582c22c9
📒 Files selected for processing (1)
.claude/skills/pr-review-loop/SKILL.md
… timestamps Addresses @coderabbitai review on #6576. The finding is correct: `.commit.committer.date` is the head commit's author/ committer metadata, not push time, so a rebase, an amend, or a delayed push all skew a date comparison — and a squash-merged main commit carries a date unrelated to when its content actually landed. The suggested remedy (read push time from PR timeline/update events) would fix the skew but still answers the wrong question. What the check actually needs to know is not "which is newer" but "did the branch ever contain the failing code?" — which `git merge-base --is-ancestor` answers directly and is immune to the entire class of timestamp problems, including ones PR timeline events also get wrong. Replaces the three date greps with: 1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all 2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit that introduced the match site in the branch's history 3. the unchanged PR file-list check Also adds a non-vacuity probe, because a check that returns "not an ancestor" for every input is broken rather than exonerating: a commit known to be in the branch's history (`git merge-base origin/main <headOid>`) must report ancestor. Verified verbatim against the case that motivated the section (#5866): step 1 reports interaction.rs absent at the head, step 2 reports the shared.rs match site not in the branch's history, and the probe reports the real merge-base df2ab2d as an ancestor — so the test discriminates rather than always failing closed. Timestamps stay as human-readable colour in the review comment, relabelled "committed at" rather than "pushed at".
matthewevans
disabled auto-merge
July 24, 2026 00:18
jsdevninja
pushed a commit
to jsdevninja/phase
that referenced
this pull request
Jul 24, 2026
…7-23 sweep (phase-rs#6576) * docs(pr-review-loop): encode review-sweep defects found in the 2026-07-23 sweep Four failure modes from a live authorized-maintainer sweep, each of which produced a defective posted review: - Dispatching Review Agents (new): a subagent's final assistant text is not delivered to the lead — findings arrive only via SendMessage, and charters must say so verbatim. Idle means resumable, not dead. Do not substitute a lead-authored review for a slow agent; three of the sweep's four bad reviews came from doing exactly that, and each was overturned by the agent's report minutes later. - Verifying the Review's Own Evidence (new): the CR grep-verification bar binds the reviewer as hard as the contributor, since the contributor acts on what the review says. Check repo convention before proposing a citation, and read the rule rather than trusting a number in roughly the right section — adjacent subrules routinely cover unrelated single cards (612.5 is Exchange of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a different section from the effect it orders (613.1c, not 612.x). - Judging the seam (new, under Review Bar): "reuses existing machinery" is not "correct seam". Every proliferation heuristic in this file returned clean on a PR that reused the instance-blind checker while the instance-aware authority twenty lines away already cited the rule and dropped half of it. Grep for the rule, not the symbol; two mechanisms for one rule is the defect. - Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's own new variants look like contributor sloppiness and are frequently ours — main adds a new match site after the contributor's last push. Timestamp the failing site against the head before writing that finding. - Review Freshness: stale approval plus armed auto-merge is a safety defect, not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting APPROVED after a new commit lands, so an unreviewed head can sit armed to merge. Worse when the post-approval commit is maintainer-authored. * docs(pr-review-loop): test compile-error attribution by ancestry, not timestamps Addresses @coderabbitai review on phase-rs#6576. The finding is correct: `.commit.committer.date` is the head commit's author/ committer metadata, not push time, so a rebase, an amend, or a delayed push all skew a date comparison — and a squash-merged main commit carries a date unrelated to when its content actually landed. The suggested remedy (read push time from PR timeline/update events) would fix the skew but still answers the wrong question. What the check actually needs to know is not "which is newer" but "did the branch ever contain the failing code?" — which `git merge-base --is-ancestor` answers directly and is immune to the entire class of timestamp problems, including ones PR timeline events also get wrong. Replaces the three date greps with: 1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all 2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit that introduced the match site in the branch's history 3. the unchanged PR file-list check Also adds a non-vacuity probe, because a check that returns "not an ancestor" for every input is broken rather than exonerating: a commit known to be in the branch's history (`git merge-base origin/main <headOid>`) must report ancestor. Verified verbatim against the case that motivated the section (phase-rs#5866): step 1 reports interaction.rs absent at the head, step 2 reports the shared.rs match site not in the branch's history, and the probe reports the real merge-base df2ab2d as an ancestor — so the test discriminates rather than always failing closed. Timestamps stay as human-readable colour in the review comment, relabelled "committed at" rather than "pushed at". --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.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.
Four failure modes from a live authorized-maintainer sweep, each of which
produced a defective posted review:
Dispatching Review Agents (new): a subagent's final assistant text is not
delivered to the lead — findings arrive only via SendMessage, and charters
must say so verbatim. Idle means resumable, not dead. Do not substitute a
lead-authored review for a slow agent; three of the sweep's four bad reviews
came from doing exactly that, and each was overturned by the agent's report
minutes later.
Verifying the Review's Own Evidence (new): the CR grep-verification bar binds
the reviewer as hard as the contributor, since the contributor acts on what
the review says. Check repo convention before proposing a citation, and read
the rule rather than trusting a number in roughly the right section —
adjacent subrules routinely cover unrelated single cards (612.5 is Exchange
of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a
different section from the effect it orders (613.1c, not 612.x).
Judging the seam (new, under Review Bar): "reuses existing machinery" is not
"correct seam". Every proliferation heuristic in this file returned clean on
a PR that reused the instance-blind checker while the instance-aware
authority twenty lines away already cited the rule and dropped half of it.
Grep for the rule, not the symbol; two mechanisms for one rule is the defect.
Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's
own new variants look like contributor sloppiness and are frequently ours —
main adds a new match site after the contributor's last push. Timestamp the
failing site against the head before writing that finding.
Review Freshness: stale approval plus armed auto-merge is a safety defect,
not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting
APPROVED after a new commit lands, so an unreviewed head can sit armed to
merge. Worse when the post-approval commit is maintainer-authored.
Summary by CodeRabbit