Conversation
…gs Line (#1709) Answers #1692 and #1699, the two defects in how `pr_review.py` reads a `ccr-overview-v2` review body. ## #1692, the unclearable coverage block Five of ten rounds in that format state no file coverage at all, so exit 45 fired on a state no re-request has ever cleared, and the maintainer waived it every time. A round that states none now carries the newest round that states some. **The carry is bounded on the change set.** It carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying: one pull request in a promotion chain held its marker across a push whose change set stayed the same three files, and the next lost it when its change set grew from one file to three. It refuses where either compare cannot be read, because a bound that falls open when it cannot be measured is no bound. Exit 45 therefore now covers three states, and its own message says which the run reached: nothing ever stated coverage, the change set moved since the round that did, or the comparison could not be read. `changed_at` is memoized for the run. The digest and the verdict each compute the bound, and unmemoized they did so from four independent network reads, so a transient failure in one pair and not the other printed a carried block beside an exit code that refused the carry, and the reverse. The reverse is the fail-open direction, since a wait keys on the exit code. ## #1699, the per-severity findings line The line was read as its first number, so a round stating two of one severity and three of another reported two findings and `unlisted_findings` computed a shortfall of zero. Every count the markup did not swallow is now summed. Summing overstates and never understates. A line stating a grand total and then the severities it decomposes into is summed with its own parts, and `12 findings across 4 files` sums to sixteen. That is the named cost of the direction: an overstated shortfall prints a block a reader checks against the body and blocks no merge, where a suppressed one closes the gate over findings nobody answered. The markup is masked to one sentinel before any count is read, so a badge's own attribute digits and a link's anchor id are not counts. Under the mask, a run of more than four digits is an identifier rather than a count, which is the second guard for a mask that terminates early on a bracket a URL carries itself, and which also stops `int` being handed a digit run long enough to raise. ## Prose these changed `.github/copilot-instructions.md` still required the head round to state coverage itself, which the carry made false. The Merge Gate in `pr-review-conduct` and `GOVERNANCE.md` "PR Review Etiquette" were never updated for the carry at all. ## Verification Three adversarial local review passes, each on the strongest tier this session reaches, raising 9, 13 and 8 findings, every one dispositioned. The second pass is the one worth naming: the first fix had the digest and the verdict each compute the bound from their own network reads, so a transient failure in one and not the other let a carry the digest had just refused read as clean through the exit code a wait keys on. The full suite is green at 1604 tests, and the two bound cases were confirmed to fail with the bound reverted. `ruff format`, `ruff check`, `mypy`, `repo_gate --check eol`, `spec/validate.py` and `build_dist.py --check` all pass. The prose gate reports `comment-added` alone, which the `comments` label on this pull request clears. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Review completion now correctly carries coverage from an earlier round only when the changed files remain identical. * Reviews are blocked when coverage cannot be verified, the change set has moved, or findings and coverage data are incomplete. * Finding totals are parsed more accurately across severity counts, markup, links, and “none” values. * **Documentation** * Updated review guidance and script documentation to explain carried coverage, blocking conditions, and revised exit statuses. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes merge-gating logic and parsing of untrusted review bodies in pr_review.py, so a human should do a final risk check despite the strong test coverage.
Review effort: Lite
Findings: 1
What changed in this PR
This promotion PR brings develop to main, carrying the ccr-overview-v2 reader fixes that make pr_review.py (a) carry forward the newest stated coverage when the head round states none, bounded by an unchanged changed-file set, and (b) sum multi-severity finding counts instead of reading only the first number. It also updates the review guidance docs and adds/strengthens tests to keep the new behavior off the network and order-independent.
Changes:
- Update
scripts/pr_review.pyto (1) bound carried coverage on a REST-verified unchanged change set and (2) sum findings totals across severity splits while masking markup/identifiers. - Expand
scripts/tests/test_pr_review.pywith coverage-carry and findings-summing cases, plus a basesetUpinvariant that prevents accidental REST/network reads. - Update review-loop documentation (
scripts/README.md,GOVERNANCE.md,.github/copilot-instructions.md, and thepr-review-conductskill text) to describe the new carry behavior and exit-code semantics.
| File | Description |
|---|---|
| scripts/tests/test_pr_review.py | Adds comprehensive regression tests for carried coverage, summed findings parsing, and enforces test isolation from REST reads. |
| scripts/README.md | Documents updated status/wait semantics for carried coverage and summed findings interpretation. |
| scripts/pr_review.py | Implements bounded coverage carry via REST compare + memoization and robust findings total parsing for ccr-overview-v2. |
| GOVERNANCE.md | Updates PR review etiquette to allow bounded coverage carry when the head round states none. |
| .github/skills/pr-review-conduct/SKILL.md | Generated skill copy updated to reflect the bounded-coverage-carry rule. |
| .github/copilot-instructions.md | Updates Copilot review completion guidance to match bounded carry and new blocking conditions. |
| .claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md | Generated plugin skill copy updated to reflect the bounded-coverage-carry rule. |
| .claude-plugin/fleet-skills/.source-digests/pr-review-conduct | Updates the source digest for the regenerated skill distribution. |
| .agents/skills/pr-review-conduct/SKILL.md | Source skill text updated to describe bounded-coverage carry for head rounds that state no coverage. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+66
to
67
| Re-run the loop after every fix | ||
| push. Never infer review completion from `mergeStateStatus: CLEAN`. |
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
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.
Promotes
developtomain, carrying one change: theccr-overview-v2reader fixes from #1709.What it carries
The coverage statement now carries forward, bounded on the change set. A round in that format states no file coverage five times in ten, so exit 45 fired on a state no re-request has ever reliably cleared. A round that states none now carries the newest round that states some, and only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying. It refuses where either compare cannot be read, because a bound that falls open when it cannot be measured is no bound. Exit 45 now names three states rather than one, and its own message says which the run reached.
The per-severity findings line is summed rather than read as its first number. A round stating two of one severity and three of another reported two findings, so
unlisted_findingscomputed a shortfall of zero and a withheld finding went unreported. Summing overstates and never understates, and the named cost is that12 findings across 4 filessums to sixteen. An overstated shortfall prints a block a reader checks against the body and blocks no merge, where a suppressed one closes the gate over findings nobody answered.The runbook, the
pr-review-conductMerge Gate, andGOVERNANCE.md"PR Review Etiquette" are updated for the carry, which none of them described.Review record on #1709
Four Copilot rounds and one CodeRabbit round. The final round reads
coverage=full, "Approval recommended", zero findings, zero threads, zero suppressed, 8/8 checks.CodeRabbit raised one body-only nitpick that opened no thread, and it was real: two test classes overrode
setUpwithout chaining, so neither installed thegh_restpatch that keeps a case off the network nor the memo clear this change adds. Fixed as a class invariant rather than in the two instances, and answered with the fixing SHA. CodeRabbit is rate limited on the final head, so its green check there is not coverage and it is advisory in any case.Three adversarial local review passes raised 9, 13 and 8 findings, every one dispositioned. The second pass caught a fail-open this change had introduced while closing another: the digest and the verdict each computed the bound from their own network reads, so a transient failure in one and not the other let a carry the digest had just refused read as clean through the exit code a wait keys on.
Closes #1692
Closes #1699
🤖 Generated with Claude Code