From 41ddcf42d294ed435b38eb168ef7553ab74aa632 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 12:24:15 -0700 Subject: [PATCH 1/4] Add local-strict-review skill and wire it into the pre-PR flow (#1065) ## Summary Adds a new fleet skill, `local-strict-review`: one read-only, adversarial review pass against a branch's current diff against its merge-base, full file context included, on the strongest model tier reachable, before a unit of work is committed toward a pull request or claimed done. It reuses `code-review`'s "Review the Change" criteria rather than restating them, and owns only this local, pre-PR moment. Wires it into the three moments it exists to cover: - `pr-review-conduct`'s Expected review loop, before the first push and before any fix push under outcome 1. - `drive-pr`'s Drive Loop step 2 and its finding-disposition mapping. - `agent-conduct`'s "Before Claiming Done" trigger, for PR-bound work specifically. `AGENTS.md` "Where the Rules Live" gets one new closing-paragraph sentence introducing the skill, per the `skill-lifecycle` doc-packaging pattern for new content. Regenerated `.github/skills/` and `.claude-plugin/fleet-skills/` via `python3 scripts/build_dist.py`; `--check` is clean and `scripts/tests/test_build_dist.py` passes. ## Design notes - The diff anchor is the branch's diff against `git merge-base @{u} HEAD`, not `git diff --staged` as literally proposed in #1056. Staging is consumed by the commit that must precede any push, so a staged-only anchor would be empty at every call site the wiring above invokes it from. The merge-base anchor also reviews the whole accumulated branch diff rather than only the latest increment, which is what the issue's own evidence section says incremental per-push review misses. - Model selection follows the fleet's existing model-tier convention (`AGENTS.md` "Match the model tier to the judgment... state the tier in the delegation itself") rather than naming a literal model ID that would go stale. - `code-review`'s "Publish Every Finding" section (PR-comment posting, severity-labeled titles, a `fleet-review` coverage marker) is deliberately not imported: this pass has no PR to post to, so its own report contract replaces that section rather than extending it. - Findings dispose per `pr-review-conduct`'s five outcomes; a finding raised and not fixed is never the agent's own call to leave, per outcome 3. ## Scope Folds in #1057 (chain local checks before opening a PR / pushing a fix) and #1059 (`agent-conduct`'s claiming-done trigger). #1058 (CodeRabbit/Qodo comment-coverage gaps) and #1060 (bot-silence vs. budget-stop diagnosis) are a different subject and intentionally out of scope here. Fixes #1056 Fixes #1057 Fixes #1059 ## Verification - `python3 scripts/build_dist.py --check` clean. - `python3 scripts/tests/test_build_dist.py` passes (28 tests). - Full local gate (`ruff`, `mypy`, the `scripts/tests` and `spec`/`host-setup` self-tests, `build_dist.py --check`, `repo_gate.py`, `prose_lint.py`'s default and `charset-unknown` checks, JSON validation, `spec/validate.py`, `docker_lint.py`) all green. - Dogfooded the new skill itself: dispatched an Opus-tier adversarial pass against this PR's own staged diff before the first push. It found real defects in the first draft (the staged-diff anchor being empty at its own call sites, a lossy summary of `pr-review-conduct`'s five outcomes, a briefing-shape violation, a Markdown loose-list bug, a heading-casing miss), all fixed before this push. ## Summary by CodeRabbit - **New Features** - Added a read-only, adversarial review pass covering complete branch changes, touched files, and untracked files. - Reviews now check for coercion issues, race conditions, and platform-specific behavior. - **Documentation** - Documented review execution, finding handling, and completion validation requirements. - Registered the review capability across supported skill integrations. - **Process Improvements** - Required local review before pushing changes, opening pull requests, pushing fixes, or declaring PR-bound work complete. --- .agents/skills/agent-conduct/SKILL.md | 1 + .agents/skills/drive-pr/SKILL.md | 6 +- .agents/skills/local-strict-review/SKILL.md | 72 +++++++++++++++++++ .agents/skills/pr-review-conduct/SKILL.md | 5 +- .../fleet-skills/.claude-plugin/plugin.json | 1 + .claude-plugin/fleet-skills/.source-digest | 2 +- .../skills/agent-conduct/SKILL.md | 1 + .../fleet-skills/skills/drive-pr/SKILL.md | 6 +- .../skills/local-strict-review/SKILL.md | 72 +++++++++++++++++++ .../skills/pr-review-conduct/SKILL.md | 5 +- .github/skills/agent-conduct/SKILL.md | 1 + .github/skills/drive-pr/SKILL.md | 6 +- .github/skills/local-strict-review/SKILL.md | 72 +++++++++++++++++++ .github/skills/pr-review-conduct/SKILL.md | 5 +- AGENTS.md | 2 + 15 files changed, 247 insertions(+), 10 deletions(-) create mode 100644 .agents/skills/local-strict-review/SKILL.md create mode 100644 .claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md create mode 100644 .github/skills/local-strict-review/SKILL.md diff --git a/.agents/skills/agent-conduct/SKILL.md b/.agents/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.agents/skills/agent-conduct/SKILL.md +++ b/.agents/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.agents/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json index b1b9ed80..e7913205 100644 --- a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json +++ b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json @@ -17,6 +17,7 @@ "./skills/drive-pr", "./skills/fleet-conformance-check", "./skills/git-commit-conventions", + "./skills/local-strict-review", "./skills/merge-and-release", "./skills/operational-vs-release-workflow", "./skills/pr-review-conduct", diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 3fbd8ae8..ea6d8faa 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -8e452ce0980b2de6 +fb40d1fd82f81ff3 diff --git a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.github/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/AGENTS.md b/AGENTS.md index e5073477..33bf3dce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,3 +109,5 @@ Some of the rules above are also packaged as Claude Code / opencode / Codex Skil Adding or changing a managed host tool is packaged as `add-host-tool`. It keeps the cross-platform contract, installer, documentation, test, and native-verification surfaces together. Driving a pull request through its review loop, from a feature branch into `develop` and, when asked, on to a mergeable `develop -> main` promotion PR, disposing of every reviewer finding along the way per `pr-review-conduct`, is packaged as `drive-pr`, new content rather than a rule extracted from a section. Merging a ready promotion PR and dispatching the release it unblocks, refreshing this machine's installed Skills first when the repo is this hub, is `merge-and-release`, its own new-content package, invoked separately from `drive-pr` so the promotion merge and the release dispatch each keep their own explicit go-ahead. + +Running one read-only, adversarial review pass against a branch's current diff against its target branch, full file context included, on the strongest model tier the session can reach, before a unit of PR-bound work is pushed toward a pull request or claimed done, is packaged as `local-strict-review`, new content rather than a rule extracted from a section. `drive-pr`, `pr-review-conduct`, and `agent-conduct` each reference it at the moment they already govern, rather than restating what it does. From 5a93698a384d2e2453ab2da3d5d5b2bcd0438414 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 15:27:17 -0700 Subject: [PATCH 2/4] Clarify Copilot-scoped review_on_head, read CodeRabbit/Qodo comment-only findings (#1067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1066. Closes #1058. ## #1066 - review_on_head=NO misread as "no review covers this head" `review_on_head` in `pr_review.py`'s digest names Copilot's own coverage specifically (the reviewer this script requests and waits for), never "no review of any kind covers this head". A tracked other reviewer (CodeRabbit, Qodo) can carry the exact head commit under `other_reviewed`, with an empty review body and no new threads, its own ordinary "reviewed, nothing to flag" shape, not a missing review. That distinction existed in the code already but was easy to misread from the digest line alone. Clarified in `pr_review.py`'s own docstring and in `pr-review-conduct`'s Merge Gate (item 2). ## #1058 - CodeRabbit/Qodo findings that reach no thread Gives `pr_review.py` the equivalent of Copilot's suppressed-comments handling for the two other trialed reviewers: - **CodeRabbit's "outside diff range" findings** are collapsed into the review body rather than raised as an inline review comment, so they open no `reviewThreads` entry either. Read via a generalized `marker_blocks` helper (shared with the existing `suppressed_blocks`), surfaced as `cr_outside_diff=N (on_head=X earlier=Y)` in the digest and as detail lines. - **Qodo's numbered findings** live entirely in its "Code Review by Qodo" PR-level comment; its formal review carries an empty body on every round observed. Read and filtered by Qodo's own `Resolved`/`Dismissed` self-tracked badge, surfaced as `qodo_open=N` (a fast pre-triage signal per the runbook, not a substitute for reading the finding). `pr-review-conduct`'s Merge Gate (item 3) now requires triaging both the same way it already requires for Copilot's suppressed findings. ## Review history on this branch The first commit implemented the above. Before opening this PR, `local-strict-review` (the new skill from #1065/#1056) ran an adversarial pass against the full branch diff and found six real issues, all fixed in the second commit: a multi-finding CodeRabbit section silently rendering only its first finding despite the count reporting the true total, blockquote-stripping corrupting quoted shell/code content in the rendered output, an unanchored `QODO_BADGE` regex misreading a finding titled about this script's own `isResolved` identifier as self-resolved, missing window-blind handling for `qodo_open`, hardcoded login literals duplicating `OTHER_REVIEWERS`, and one vacuous test assertion. Regression tests cover each. All local gates pass: `ruff check`/`format`, `mypy`, the full `scripts/tests` suite (873 tests), `prose_lint.py --diff`, `repo_gate.py --check eol`/`eol-coverage`, `build_dist.py --check`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **New Features** * Expanded merge review coverage to include CodeRabbit and Qodo advisory reviews. * Recognizes qualifying reviews for the current change, including reviews with no findings. * Detects findings in outside-diff comments and comment-only reviews. * Verifies resolved or dismissed findings before allowing merges. * Review summaries now report findings, truncated reviews, and unknown review windows more clearly. * **Tests** * Added coverage for nested review content, multiple findings, reviewer-specific results, and resolved statuses. --- .agents/skills/pr-review-conduct/SKILL.md | 13 +- .claude-plugin/fleet-skills/.source-digest | 2 +- .../skills/pr-review-conduct/SKILL.md | 13 +- .github/skills/pr-review-conduct/SKILL.md | 13 +- scripts/pr_review.py | 338 +++++++++++++++--- scripts/tests/test_pr_review.py | 330 ++++++++++++++++- 6 files changed, 657 insertions(+), 52 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 84ec8159..810955eb 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -41,11 +41,20 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. + the PR outright, or say it read only part of the changed files. `pr_review.py`'s + `review_on_head` names Copilot's own coverage specifically, the currently required reviewer, + not "no review of any kind covers this head": a trialed advisory reviewer (CodeRabbit, + Qodo) carrying the exact head under `other_reviewed`, with an empty review body and no new + threads, is its own ordinary "reviewed, nothing to flag" shape, not a missing review (#1066). 3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered. Those appear in no thread, so polling threads - alone reports a clean pass while they stand. + alone reports a clean pass while they stand. The same holds for CodeRabbit's own + "outside diff range" comments (`cr_outside_diff` in `pr_review.py`'s digest) and for Qodo's + comment-only findings (`qodo_open`): neither opens a `reviewThreads` entry either, so + give each one the same triage the low-confidence findings above already get (#1058). Qodo's own + `Resolved`/`Dismissed` self-tracked badge is a fast pre-triage signal, not a substitute for + reading the finding, spot-verify against `gh pr diff` rather than trusting it outright. 4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own. File an issue naming it and quoting the body, rather than guessing what the new wording diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index ea6d8faa..c7e837e4 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -fb40d1fd82f81ff3 +8cab4f8afbf9a439 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 84ec8159..810955eb 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -41,11 +41,20 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. + the PR outright, or say it read only part of the changed files. `pr_review.py`'s + `review_on_head` names Copilot's own coverage specifically, the currently required reviewer, + not "no review of any kind covers this head": a trialed advisory reviewer (CodeRabbit, + Qodo) carrying the exact head under `other_reviewed`, with an empty review body and no new + threads, is its own ordinary "reviewed, nothing to flag" shape, not a missing review (#1066). 3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered. Those appear in no thread, so polling threads - alone reports a clean pass while they stand. + alone reports a clean pass while they stand. The same holds for CodeRabbit's own + "outside diff range" comments (`cr_outside_diff` in `pr_review.py`'s digest) and for Qodo's + comment-only findings (`qodo_open`): neither opens a `reviewThreads` entry either, so + give each one the same triage the low-confidence findings above already get (#1058). Qodo's own + `Resolved`/`Dismissed` self-tracked badge is a fast pre-triage signal, not a substitute for + reading the finding, spot-verify against `gh pr diff` rather than trusting it outright. 4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own. File an issue naming it and quoting the body, rather than guessing what the new wording diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 84ec8159..810955eb 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -41,11 +41,20 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed from a green merge-state. A push makes checks go green *before* the re-review lands, and the matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. + the PR outright, or say it read only part of the changed files. `pr_review.py`'s + `review_on_head` names Copilot's own coverage specifically, the currently required reviewer, + not "no review of any kind covers this head": a trialed advisory reviewer (CodeRabbit, + Qodo) carrying the exact head under `other_reviewed`, with an empty review body and no new + threads, is its own ordinary "reviewed, nothing to flag" shape, not a missing review (#1066). 3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered. Those appear in no thread, so polling threads - alone reports a clean pass while they stand. + alone reports a clean pass while they stand. The same holds for CodeRabbit's own + "outside diff range" comments (`cr_outside_diff` in `pr_review.py`'s digest) and for Qodo's + comment-only findings (`qodo_open`): neither opens a `reviewThreads` entry either, so + give each one the same triage the low-confidence findings above already get (#1058). Qodo's own + `Resolved`/`Dismissed` self-tracked badge is a fast pre-triage signal, not a substitute for + reading the finding, spot-verify against `gh pr diff` rather than trusting it outright. 4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own. File an issue naming it and quoting the body, rather than guessing what the new wording diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 3ca13f6d..efbdb5d7 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -18,9 +18,16 @@ review findings between them. Read-only. Exit 0 = every reference resolves, 70 = one does not, 71 = there were references and none could be read, so nothing was decided. status One digest line, any unresolved threads, and any suppressed findings. Read-only. - Exit 0 = no review covers the head yet, or every output shape is recognized and the - round covering the head read the whole diff. Use `wait` when review presence is the - condition, since `status` reports an absent review without treating it as a failure. + Exit 0 = no Copilot review covers the head yet, or every output shape is recognized + and the round covering the head read the whole diff. `review_on_head` and `rounds=` + in the digest name Copilot's own coverage specifically, the reviewer this script + requests and waits for, never "no review of any kind covers this head": a + tracked other reviewer, named under `other_reviewed` below, can carry the exact head + commit while Copilot's own `review_on_head` still reads `NO`, and an empty body from + that other reviewer on that head is its own ordinary "reviewed, nothing to flag" + shape, the same reading an empty-bodied Copilot round already gets, not a gap. + Use `wait` when review presence is the condition, since `status` reports an absent + review without treating it as a failure. 42 = that round read fewer files than the pull request changed, so part of the diff has no review at all. Measured over four pull requests and seven rounds here, a re-request never cleared one and no round ever recovered, so this is a state to @@ -43,15 +50,34 @@ further pagination, so a pull request carrying more than that undercounts silently past that point: both fields print a trailing `+` and a `THREADS TRUNCATED` block follows, naming the gap rather than leaving either count to be trusted as whole - (#973). Reading past the cut needs `reply`'s own paginated walk instead. - Neither is read past that: no coverage, no refusal, no suppressed-finding parsing, no - `wait`/request support, each being its own format and its own future task. Where - either has posted anything at all on the current head, `other_reviewed` names it, - identity and commit only, no verdict read from what it said. `other_rate_limited` - names one whose newest comment or review carries its own rate-limit marker instead, - a structural `` convention rather than free-text - prose (observed on CodeRabbit, ptr727/Blog #110), so reading it needs no per-bot - wording model the way Copilot's quota refusal needed `QUOTA` built for its own text. + (#973). Reading past the cut needs `reply`'s own paginated walk instead. `suppressed=` + and `cr_outside_diff=` carry the identical marker over the separate 100-review page + they are both read from, a `REVIEWS TRUNCATED` block following, since an unanswered + finding on a round old enough to fall out of that window is exactly as invisible. + Coverage, refusal, and `wait`/request support stay Copilot-only, each being its own + format and its own future task per bot. Where either has posted anything at all on + the current head, `other_reviewed` names it, identity and commit only, no verdict + read from what it said. `other_rate_limited` names one whose newest comment or review + carries its own rate-limit marker instead, a structural + `` convention rather than free-text prose (observed on + CodeRabbit, ptr727/Blog #110), so reading it needs no per-bot wording model the way + Copilot's quota refusal needed `QUOTA` built for its own text. + Two of their own finding shapes are read, though, each its own blind spot a thread + poll alone cannot see. `cr_outside_diff=N (on_head=X earlier=Y)` counts + CodeRabbit's own "outside diff range" findings, collapsed into the review body rather + than raised as an inline review comment because the finding sits on a line outside + the pull request's changed hunks. Printed only once CodeRabbit has raised one, on any + round, so a repository not trialing it stays silent. `qodo_open=N` counts Qodo's own + numbered findings that carry neither its `Resolved` nor `Dismissed` self-tracked + badge: Qodo's formal review carries an empty body on every round observed, so its + findings are read from its "Code Review by Qodo" PR-level comment instead, not + head-scoped since a comment carries no commit. Printed as `0` once Qodo has posted + that comment at all, since a `0` is itself a reading, `unknown` where its findings + comment specifically could be sitting behind the 100-comment window even while a + paired `PR Summary by Qodo` stays visible, so its silence cannot be told from it + never having commented, and absent only where it genuinely never has. Qodo's own + badge is a fast pre-triage signal, not a substitute for reading the finding: + spot-verify against `gh pr diff` rather than trusting it outright. reply Answer one thread selected by its text, and resolve it on request. Exists because the hand-run form keeps failing the same way: a node id typed into a mutation, which resolves globally and so writes to a real thread somewhere @@ -129,13 +155,15 @@ REVIEWER = "copilot-pull-request-reviewer" # Other review bots this repository has trialed alongside Copilot. -# Tracked at the identity level only, login and commit oid, never body prose. -# No coverage parsing, no refusal reading, no wait/request support here. -# Each of those writes its own findings in its own format, and doing that well is a separate task per bot. +# Tracked at the identity level only, login and commit oid, never body prose, except where a reader below names one explicitly. +# Each format read here is its own reader, and doing that well is a separate task per bot. # What generalizes without reading any of their prose is thread resolution. # An open thread blocks a ruleset-gated merge whoever opened it, and `status`'s `unresolved=0` once silently hid a CodeRabbit/qodo thread that did block one (PR #915, ptr727/ProjectTemplate). # Login spellings are read off this repository's own history (`gh pr view --json reviews,comments`) rather than guessed. -OTHER_REVIEWERS = ("coderabbitai", "qodo-code-review") +CODERABBIT_LOGIN = "coderabbitai" +QODO_LOGIN = "qodo-code-review" +# Named rather than inlined at each of their own readers below, so a login rename updates one spelling instead of silently leaving a hardcoded copy matching nothing. +OTHER_REVIEWERS = (CODERABBIT_LOGIN, QODO_LOGIN) KNOWN_REVIEWERS = (REVIEWER, *OTHER_REVIEWERS) # A check dispatched to a runner and not begun, in the spellings a CheckRun carries. @@ -166,6 +194,8 @@ # The alternation is the runbook's, since the heading wording has changed once already. # Matching one phrasing alone reports zero on a review that has them. SUPPRESSED = re.compile(r"Suppressed comments|low confidence", re.IGNORECASE) +# CodeRabbit's own equivalent, collapsed into the review body like `SUPPRESSED` rather than raised as an inline comment. +CR_OUTSIDE_DIFF = re.compile(r"Outside diff range comments", re.IGNORECASE) # A refusal declines the round as a formal review carrying the head and no threads. # That is the clean pass byte for byte, so every coverage check passes over a round that never ran. # The alternation is the runbook's for the same reason the one above is. @@ -186,6 +216,20 @@ # The service name is captured rather than assumed. # A second bot using the same auto-generated-comment convention is read without a new pattern, and one that does not use it stays unread rather than guessed at. RATE_LIMITED = re.compile(r"auto-generated comment:\s*rate limited by\s*(\S+?)\s*-->") +# Qodo's formal review object carries an empty body on every review checked, confirmed across roughly 60. +# Its findings ride a PR-level comment instead. +# Two are posted per round, "PR Summary by Qodo" (an overview, no findings) and this one, told apart by its own heading. +# Anchored to the `

` tag the heading itself wears, not a bare substring: the summary comment's own prose can mention the phrase without being the comment it names. +QODO_REVIEW_HEADING = re.compile(r"

\s*Code Review by Qodo\s*

", re.IGNORECASE) +# Qodo nests a Description/Code/Relevance/Evidence/Agent-prompt `` under each of its own numbered findings. +# Only the numbered heading itself is a finding, told apart from those by starting with `N.` the way none of the nested ones do. +QODO_FINDING = re.compile(r"\s*\d+\.\s") +# Qodo's own self-tracked disposition, re-checked against the current head on its own schedule. +# Present on a finding it has re-verified as addressed or intentionally dismissed, absent on one still open. +# A fast pre-triage signal per the runbook, not a substitute for reading the finding: spot-verify against `gh pr diff` rather than trusting it outright. +# The glyph is required rather than left unanchored, since a bare word match reads any finding whose own title quotes the identifier `Resolved` or `Dismissed` in its own `` tag as carrying the badge, closing an open finding on the strength of its own title. +# Escaped rather than typed literally (check mark U+2713, ballot x U+2717), keeping this source inside the ASCII charset rule that governs the repository. +QODO_BADGE = re.compile(r"[^<]*(?:\u2713 Resolved|\u2717 Dismissed)[^<]*") # A round states how much of the diff it read on a line of its own. # A round that read part of it is the clean pass elsewhere, same commit and threads and digest. # Five such rounds landed across three merged pull requests here. @@ -297,7 +341,6 @@ # It is a literal rather than the pull request's own repository. # That one is where the shape was seen, not where the reader failing on it lives. HUB = "ptr727/ProjectTemplate" -DETAILS = re.compile(r"
(.*?)
", re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r"(.*?)", re.DOTALL | re.IGNORECASE) TAGS = re.compile(r"", re.IGNORECASE) COUNT = re.compile(r"\((\d+)\)") @@ -708,18 +751,22 @@ def answered_outside_review(pr: dict) -> dict | None: return newest if (newest.get("createdAt") or "") > latest_review else None -def window_blind(pr: dict, field: str) -> bool: - """True where the reviewer's own nodes can sit behind the window, so the view cannot decide. +def window_blind(pr: dict, field: str, login: str = REVIEWER) -> bool: + """True where this login's own nodes can sit behind the window, so the view cannot decide. - Each query reads the newest nodes rather than the reviewer's, so ordinary traffic is what + `login` defaults to the fully-modeled reviewer, so every existing caller keeps reading + Copilot's own blind spot unchanged. A caller reading another known reviewer's own window, + Qodo's `comments` connection carrying its findings comment, passes it explicitly. + + Each query reads the newest nodes rather than this login's, so ordinary traffic is what pushes theirs out of reach. Nodes arrive in creation order, so anything behind the window is - older than everything inside it: one of the reviewer's in view bounds every hidden one as + older than everything inside it: one of this login's in view bounds every hidden one as older still, which settles the question rather than leaving it open. `hasPreviousPage` is what says anything is back there at all, since a full window and a window holding the lot are the same length. """ older = ((pr.get(field) or {}).get("pageInfo") or {}).get("hasPreviousPage") - return bool(older) and not reviewer_nodes(pr, field) + return bool(older) and not reviewer_nodes(pr, field, login) def threads_truncated(pr: dict) -> bool: @@ -737,6 +784,22 @@ def threads_truncated(pr: dict) -> bool: return bool(((pr.get("reviewThreads") or {}).get("pageInfo") or {}).get("hasNextPage")) +def reviews_truncated(pr: dict) -> bool: + """True where the `reviews` connection's own page cut off before this pull request's actual + round count, so a suppressed or outside-diff-range finding raised on an older, windowed-out + round is missing from `suppressed=`/`cr_outside_diff=` below rather than merely counted as + stale. + + Unlike `window_blind`, which settles a coverage or dating question once even one of a + login's own reviews sits in view, a finding count needs every round read, not just a + recent-enough one: an unread round can carry a finding of its own regardless of whether a + newer round from the same login is visible. `hasPreviousPage` on this `last`-windowed + connection means the reviews left unread are the oldest, exactly where an unanswered + finding is most likely to still sit unresolved. + """ + return bool(((pr.get("reviews") or {}).get("pageInfo") or {}).get("hasPreviousPage")) + + def refusal_of(node: dict) -> str: """The review's body where its opening line says the reviewer did not review, otherwise empty. @@ -1384,41 +1447,169 @@ def heading_of(block: str) -> str: return m.group(1) if m and head.lower().startswith(" list[str]: - """Return the review body's low-confidence sections, each sliced from its own heading. +# A blockquote marker (`>` per line, Markdown's own quoting convention) sits in front of every line CodeRabbit wraps its outside-diff section in. +# `HEADING`'s `\s*` does not match `>`, so a heading under one reads as prose without this stripped first. +# Copilot's own shapes carry no such prefix, so this is only ever asked of CodeRabbit's own marker below, never of `SUPPRESSED`. +BLOCKQUOTE = re.compile(r"^>+\s?") +# `
(.*?)
` lazily pairs each open with the *next* close, which is the innermost one once a shape nests, silently losing everything the outer wrapper still carries after it. +# CodeRabbit's outside-diff section does exactly that: a file wrapper nested inside the section heading, itself wrapping a per-finding "Prompt for AI Agents" block three levels deep. +DETAILS_TAG = re.compile(r"]*)?>|", re.IGNORECASE) - The section has worn three shapes so far: its own `
` wrapper, a bare heading in the - body, and a Markdown heading nested inside the `Review details` wrapper. The wrapper is the - part that keeps moving, so each region is scanned line by line for the heading rather than - read for a wrapper's summary: a nested heading is not a summary, and stripping the wrappers - to look for it outside deletes the very region it sits in. That is the pair that reported - `suppressed=0` over a body carrying `### Suppressed comments (2)`. + +def details_regions(body: str) -> tuple[list[str], str]: + """Every top-level `
...
` region's own content, plus what is left once + every top-level region is removed whole. + """ + regions: list[str] = [] + leftover: list[str] = [] + depth = 0 + region_start = 0 + cursor = 0 + for m in DETAILS_TAG.finditer(body): + opening = not m.group().startswith(" 0: + depth -= 1 + if depth == 0: + regions.append(body[region_start : m.start()]) + cursor = m.end() + leftover.append(body[cursor:]) + return regions, "".join(leftover) + + +def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = False) -> list[str]: + """Return the review body's sections whose own heading matches `marker`, each sliced from + that heading through to the end of its own region. + + Shared by `suppressed_blocks` (Copilot's low-confidence findings) and `outside_diff_blocks` + (CodeRabbit's outside-diff-range findings): both are a review body collapsing real + findings into a block that raises no `reviewThreads` entry, so a thread poll alone reports a + clean pass over either. + + The section has worn three shapes so far, all against `SUPPRESSED`: its own `
` + wrapper, a bare heading in the body, and a Markdown heading nested inside the `Review + details` wrapper. The wrapper is the part that keeps moving, so each region is scanned line + by line for the heading rather than read for a wrapper's summary: a nested heading is not a + summary, and stripping the wrappers to look for it outside deletes the very region it sits + in. That is the pair that reported `suppressed=0` over a body carrying `### Suppressed + comments (2)`. The heading carries the match rather than the body text, since a review whose prose discusses - suppressed findings is not itself carrying any. A region ends the block, so a section is not - read on into the file table that follows it. + the marker's own wording is not itself carrying a block. A region ends the block, so a + section is not read on into the file table that follows it. + + `strip_blockquote` is read on a copy used only to find the heading line, never on what is + returned: CodeRabbit's own outside-diff section wraps its lines in a Markdown blockquote, + which `BLOCKQUOTE` strips for detection, but a finding quoting code (`>&2 echo`, `>> $LOG`) + would otherwise be corrupted by that same strip once it landed in the printed digest. + Left off by default, so `suppressed_blocks` reads exactly as it always has. """ if not body: return [] # Each wrapper's contents, plus what is left outside them all, so a heading is found anywhere. - # The regions do not overlap, since what the sub deletes is exactly what the findall keeps. - regions = DETAILS.findall(body) + [DETAILS.sub("", body)] + # The regions do not overlap, since `details_regions` removes exactly what it returns. + regions, leftover = details_regions(body) + regions = [*regions, leftover] blocks = [] for region in regions: - lines = region.splitlines() - for i, line in enumerate(lines): - if SUPPRESSED.search(line) and (HEADING.match(line) or COUNT.search(line)): - blocks.append("\n".join(lines[i:])) + raw_lines = region.splitlines() + scan_lines = [BLOCKQUOTE.sub("", ln) for ln in raw_lines] if strip_blockquote else raw_lines + for i, line in enumerate(scan_lines): + if marker.search(line) and (HEADING.match(line) or COUNT.search(line)): + blocks.append("\n".join(raw_lines[i:])) break return blocks +def suppressed_blocks(body: str) -> list[str]: + """Copilot's own low-confidence findings, collapsed rather than raised as inline threads. + + See `marker_blocks` for the shared reading and the shape history behind it. + """ + return marker_blocks(body, SUPPRESSED) + + +def outside_diff_blocks(body: str) -> list[str]: + """CodeRabbit's own outside-diff-range findings: a real finding on a line outside the pull + request's changed hunks, which GitHub cannot attach as an inline review comment, so + CodeRabbit collapses it into the review body instead. Observed corpus, ptr727/ProjectTemplate + PR #1053: a `...Outside diff range comments (N)` heading, nested one + file-level `
` deep in turn nesting a per-finding "Prompt for AI Agents" block, + wrapped in the review's own blockquote. + + Reads a section spanning several files or several findings in one file just as reliably as a + single finding, since `details_regions` counts the nesting rather than pairing the first + open it meets with the first close. + """ + return marker_blocks(body, CR_OUTSIDE_DIFF, strip_blockquote=True) + + def finding_count(block: str) -> int: """The heading's `(N)`, floored at one, since a block reported as zero reads as a clean pass.""" m = COUNT.search(heading_of(block)) return max(int(m.group(1)), 1) if m else 1 +def qodo_review_comment(pr: dict) -> dict | None: + """The reviewer's newest `Code Review by Qodo` comment, where its actual findings live. + + Its formal review object carries an empty body on every review checked, confirmed across + roughly 60. The findings ride a PR-level comment instead, alongside a second one, `PR + Summary by Qodo`, that carries none, told apart by this comment's own heading. Comments carry + no commit, so this is read by recency rather than by head, the same limitation + `answered_outside_review` already has for Copilot's own plain comments. + """ + comments = [ + c + for c in reviewer_nodes(pr, "comments", QODO_LOGIN) + if QODO_REVIEW_HEADING.search(c.get("body") or "") + ] + return max(comments, key=lambda c: c.get("createdAt") or "", default=None) + + +def qodo_open_findings(body: str) -> list[str]: + """Each numbered finding in a `Code Review by Qodo` comment that carries neither Qodo's own + `Resolved` nor `Dismissed` self-tracked badge, so it is still open. + + Qodo nests each finding's own Description/Code/Relevance/Evidence/Agent-prompt sections + under `` tags of their own, so every `` in the comment is read rather than + only the outermost ones, and only the numbered heading itself, matched by `QODO_FINDING`, + counts as a finding. + """ + if not body: + return [] + return [ + s.strip() + for s in SUMMARY.findall(body) + if QODO_FINDING.match(s) and not QODO_BADGE.search(s) + ] + + +def qodo_comments_blind(pr: dict) -> bool: + """True where Qodo's own `Code Review by Qodo` comment specifically can be sitting behind + the comments window, so `qodo_open` cannot tell "never commented" from "commented, unseen". + + `window_blind(pr, "comments", QODO_LOGIN)` alone is not enough: it clears the moment any + Qodo comment is visible, including the paired `PR Summary by Qodo` that carries no findings + of its own, so a findings comment old enough to fall out of the window would read as though + Qodo had never posted one at all. A second check closes that gap: an older page exists, some + Qodo comment is visible, yet `qodo_review_comment` still finds none matching the findings + heading among what is visible, so the one that matters is the one sitting behind the cut. + """ + if window_blind(pr, "comments", QODO_LOGIN): + return True + older = ((pr.get("comments") or {}).get("pageInfo") or {}).get("hasPreviousPage") + return ( + bool(older) + and bool(reviewer_nodes(pr, "comments", QODO_LOGIN)) + and not qodo_review_comment(pr) + ) + + def thread_author(t: dict) -> str: """The login on a thread's opening comment, or `""` where there is none to read. @@ -1468,6 +1659,8 @@ def digest( # True where the connection cut off before this pull request's actual thread count (#973). # Read here rather than inline below, since both the summary line and the explanatory block need it. truncated = threads_truncated(pr) + # Same reasoning, the `reviews` connection rather than `reviewThreads`, feeding `suppressed=` and `cr_outside_diff=` below. + revs_truncated = reviews_truncated(pr) # Any known reviewer's own thread, not only Copilot's. # An open thread blocks a ruleset-gated merge whoever opened it, and counting Copilot's alone hid a CodeRabbit/qodo thread that did block one (PR #915). # `thread_author` carries the deleted-account default this needs. @@ -1511,7 +1704,9 @@ def digest( ok, total = checks_tally(checks) stuck = checks_stuck(checks, now, grace, stall) # Which other known reviewers have posted anything at all on this exact head, identity and commit only, no body read. - # Whether it is a clean pass, a finding, or a refusal of its own is each bot's own prose to parse, which this script does not do for any reviewer but Copilot. + # Whether it is a clean pass, a finding, or a refusal of its own is each bot's own prose to parse. + # Two shapes of that are read below rather than left as each bot's own future task: CodeRabbit's outside-diff-range blocks and Qodo's comment-only findings. + # Coverage and refusal reading stay Copilot-only, each of the others writing its own findings in its own format. # Omitted entirely where none has, so a repository not trialing either stays silent. other_on_head = [ login @@ -1525,6 +1720,19 @@ def digest( other_limited = { login: name for login in OTHER_REVIEWERS if (name := rate_limited_by(pr, login)) } + # CodeRabbit's own outside-diff-range findings, the identical blind spot `blocks` above reads for Copilot. + # A real finding collapsed into the review body rather than raised as an inline review comment opens no `reviewThreads` entry either. + # Read every round, not only the head, for the same reason `blocks` is: a finding nobody replied to must not drop out of the digest the moment a later push supersedes its own round. + cr_revs = reviewer_nodes(pr, "reviews", CODERABBIT_LOGIN) + cr_blocks = [(n, b) for n in cr_revs for b in outside_diff_blocks(n.get("body") or "")] + cr_on_head_blocks = [b for n, b in cr_blocks if (n.get("commit") or {}).get("oid") == head] + cr_stale = sum(finding_count(b) for n, b in cr_blocks) - sum( + finding_count(b) for b in cr_on_head_blocks + ) + # Qodo's own comment-only findings: its formal review carries no body at all on any round checked, so its numbered findings are read from its `Code Review by Qodo` PR comment instead. + # Comments carry no commit, so this is read by recency, not head-scoped the way `cr_blocks` above is. + qodo_comment = qodo_review_comment(pr) + qodo_open = qodo_open_findings(qodo_comment.get("body") or "") if qodo_comment else [] lines = [ # The repository leads the line, since a number alone reads as correct anywhere. # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. @@ -1557,9 +1765,27 @@ def digest( # `unresolved` is drawn from the same truncated `threads` list, so a cut page can hide an open thread exactly as easily as it hides a resolved one, and both counts carry the marker. f"threads={len(threads)}{'+' if truncated else ''} " f"unresolved={len(unresolved)}{'+' if truncated else ''}{breakdown} " - f"suppressed={sum(finding_count(b) for n, b in blocks)} " + # A trailing `+` on either count says the same as it does on `threads=`/`unresolved=` above: a round old enough to fall out of the `reviews` window is a round its own finding cannot be read from. + f"suppressed={sum(finding_count(b) for n, b in blocks)}{'+' if revs_truncated else ''} " f"(on_head={sum(finding_count(b) for b in on_head_blocks)} earlier={stale}) " - f"answered_outside_review={answered} " + # Present where CodeRabbit has raised at least one outside-diff-range finding on any round, or the truncated window means one could exist unseen. + # A repository not trialing it, on an untruncated window, stays silent rather than printing a permanent `cr_outside_diff=0`. + + ( + f"cr_outside_diff={sum(finding_count(b) for n, b in cr_blocks)}{'+' if revs_truncated else ''} " + f"(on_head={sum(finding_count(b) for b in cr_on_head_blocks)} earlier={cr_stale}) " + if cr_blocks or revs_truncated + else "" + ) + # Present once Qodo has posted a `Code Review by Qodo` comment at all, `0` included. + # A `0` here is itself a reading, Qodo reviewed and left nothing open, rather than silence about whether it reviewed. + # `unknown` where its findings comment specifically can be sitting behind the window, so its silence here cannot be told apart from it never having commented. + # Absent only where `qodo_comments_blind` clears it and it genuinely never has. + + ( + f"qodo_open={len(qodo_open)} " + if qodo_comment + else ("qodo_open=unknown " if qodo_comments_blind(pr) else "") + ) + + f"answered_outside_review={answered} " f"requested={'yes' if reviewer_requested(pr) else 'no'} " f"merge={pr.get('mergeStateStatus')} " # `merge=BLOCKED` names no cause and is worn by every gate alike. @@ -1678,6 +1904,14 @@ def digest( "open. `threads=` and `unresolved=` above undercount. Read the rest with " "`reply`'s own paginated walk before trusting either number" ) + if revs_truncated: + lines.append( + " REVIEWS TRUNCATED: this pull request carries more reviews than the 100 read " + "here, and the connection reads newest-first, so the ones cut off are the " + "oldest, exactly where an unanswered suppressed or outside-diff-range finding is " + "most likely to still sit unresolved. `suppressed=` and `cr_outside_diff=` above " + "undercount" + ) if answer: # Printed whole for the same reason a suppressed finding is, since it reaches no thread. # Its wording is the only thing separating a refusal from an ordinary remark. @@ -1720,6 +1954,28 @@ def digest( ) # Indentation is kept, since a block carries fenced code a flattened line would garble. lines += [f" {ln.rstrip()}" for ln in TAGS.sub("", b).splitlines() if ln.strip()] + for n, b in cr_blocks: + # Mirrors the `SUPPRESSED` rendering above, its own reasons applying identically here. + sha = ((n.get("commit") or {}).get("oid") or "")[:8] + if not sha: + where = "commit unknown, treat as outstanding" + elif sha == head[:8]: + where = "on head" + else: + where = f"raised on {sha}, earlier round" + lines.append( + f" CODERABBIT OUTSIDE-DIFF ({where}): no thread to resolve, " + "answer it in the PR conversation quoting the finding" + ) + lines += [f" {ln.rstrip()}" for ln in TAGS.sub("", b).splitlines() if ln.strip()] + for finding in qodo_open: + # Not head-scoped, since the comment it comes from carries no commit at all. + lines.append( + " QODO OPEN FINDING: neither Resolved nor Dismissed, no thread to resolve, " + "answer it in the PR conversation quoting the finding, and spot-verify Qodo's " + "own badge against `gh pr diff` rather than trusting it outright" + ) + lines.append(f" {' '.join(finding.split())}") if seen is not None: lines[0] += f" new={new}" return "\n".join(lines), len(unresolved) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 44a760d2..a8728261 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -586,12 +586,14 @@ def test_a_body_is_flattened_and_bounded(self) -> None: class TestOtherReviewers(GqlCase): """Thread resolution and head-presence, generalized past Copilot to the other review bots - this repository has trialed: identity and commit only, no prose parsed for either one. + this repository has trialed: identity and commit only, no prose parsed for either one here. `status`'s `unresolved=0` used to hide a CodeRabbit/qodo thread that still blocked a ruleset-gated merge (PR #915, ptr727/ProjectTemplate), since only Copilot's own threads - counted. Coverage, refusal, and suppressed-finding reading stay Copilot-only: each of the - others writes its own findings in its own format, which is its own future task per bot. + counted. Coverage and refusal reading stay Copilot-only: `review_on_head` above names + Copilot's own coverage specifically, the reviewer this script requests and waits for, not + "no review of any kind covers this head" (#1066). CodeRabbit's outside-diff-range findings + and Qodo's comment-only findings are each read too, in their own shape, by `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below (#1058). """ def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict: @@ -874,6 +876,314 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No out, _ = pr_review.digest("o", "r", 7) self.assertIn("suppressed=0", out) + def test_a_truncated_reviews_window_marks_suppressed_as_undercounting(self) -> None: + """An older round old enough to fall out of the 100-review window can carry a finding + of its own that `suppressed=` then has no way to read, the same blind spot `threads=` + already carries its own `+` marker for.""" + self.answer(payload([review()], older_reviews=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("suppressed=0+", out) + self.assertIn("REVIEWS TRUNCATED", out) + + def test_an_untruncated_reviews_window_carries_no_marker(self) -> None: + self.answer(payload([review()], older_reviews=False)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("suppressed=0 ", out) + self.assertNotIn("REVIEWS TRUNCATED", out) + + +# Trimmed from CodeRabbit's own review body on ptr727/ProjectTemplate PR #1053, one file and one finding rather than the two files the live round carried. +# Its warning emoji is dropped, since this file's own charset rule keeps its literal source ASCII. +# The leading blockquote (`>` per line) and the file-level `
` nesting are both kept, since both are what the reader has to see through. +# CodeRabbit wraps its own outside-diff section in a blockquote, and the section nests one file-level wrapper deep before the finding. +def cr_outside_diff_body(count: int = 1, path: str = "a.py", finding: str = "Off by one.") -> str: + return ( + f"{OVERVIEW}\n" + "> [!CAUTION]\n" + "> Some comments are outside the diff and can't be posted inline due to platform " + "limitations.\n" + "> \n" + ">
\n" + f"> Outside diff range comments ({count})
\n" + "> \n" + ">
\n" + f"> {path} (1)
\n" + "> \n" + f"> `12-14`: **{finding}**\n" + "> \n" + ">
\n" + "> \n" + ">
\n" + ) + + +# Two findings under one file, each nested one level deeper in its own "Prompt for AI Agents" `
` block, the exact shape PR #1053's own round carries. +# A lazy `
` pairing loses everything after: it stops at the first finding's own nested block, not the file's close. +def cr_outside_diff_body_multi(findings: list[str], path: str = "a.py") -> str: + entries = "".join( + f"> `{line}-{line}`: **{text}**\n" + "> \n" + ">
\n" + "> Prompt for AI Agents\n" + "> \n" + "> ```\n" + f"> Fix: {text}\n" + "> ```\n" + "> \n" + ">
\n" + "> \n" + for line, text in enumerate(findings, start=12) + ) + return ( + f"{OVERVIEW}\n" + ">
\n" + f"> Outside diff range comments ({len(findings)})
\n" + "> \n" + ">
\n" + f"> {path} ({len(findings)})
\n" + "> \n" + f"{entries}" + ">
\n" + "> \n" + ">
\n" + ) + + +class TestCodeRabbitOutsideDiff(GqlCase): + """CodeRabbit's own equivalent of the blind spot `TestSuppressed` covers for Copilot.""" + + def cr_review(self, oid: str = HEAD, body: str = "") -> dict: + return { + "author": {"login": "coderabbitai"}, + "state": "COMMENTED", + "commit": {"oid": oid}, + "body": body, + "submittedAt": LATE, + } + + def test_a_block_on_the_head_counts_and_prints_the_finding(self) -> None: + self.answer(payload([review(), self.cr_review(body=cr_outside_diff_body())])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("cr_outside_diff=1 (on_head=1 earlier=0)", out) + self.assertIn("CODERABBIT OUTSIDE-DIFF (on head)", out) + self.assertIn("Off by one.", out) + + def test_the_count_is_the_headings_own_rather_than_floored_to_one(self) -> None: + self.answer(payload([review(), self.cr_review(body=cr_outside_diff_body(count=3))])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("cr_outside_diff=3 (on_head=3 earlier=0)", out) + + def test_a_block_on_an_earlier_round_is_still_reported_rather_than_dropped(self) -> None: + """A push must not retire an unanswered finding, the same rule `TestSuppressed` holds.""" + self.answer( + payload( + [ + review(), + self.cr_review(oid=OLD, body=cr_outside_diff_body()), + self.cr_review(), + ] + ) + ) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("cr_outside_diff=1 (on_head=0 earlier=1)", out) + self.assertIn("raised on", out) + + def test_cr_outside_diff_is_absent_where_none_has_been_raised(self) -> None: + """The common case, a clean round or a repository not trialing it, stays silent.""" + self.answer(payload([review(), self.cr_review(body="Walkthrough prose, no findings.")])) + out, _ = pr_review.digest("o", "r", 7) + self.assertNotIn("cr_outside_diff", out) + + def test_a_copilot_review_carrying_the_phrase_is_not_a_coderabbit_finding(self) -> None: + """The reader is scoped to CodeRabbit's own reviews, not every reviewer's body.""" + self.answer(payload([review(body=cr_outside_diff_body())])) + out, _ = pr_review.digest("o", "r", 7) + self.assertNotIn("cr_outside_diff", out) + + def test_a_truncated_reviews_window_surfaces_cr_outside_diff_even_at_zero(self) -> None: + """Silence on `cr_outside_diff` reads as "nothing to triage", so an older CodeRabbit + round old enough to fall out of the window must not stay silent just because none of + the rounds still in view happen to carry a finding of their own.""" + self.answer(payload([review()], older_reviews=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("cr_outside_diff=0+", out) + self.assertIn("REVIEWS TRUNCATED", out) + + def test_every_finding_in_a_multi_finding_section_is_captured(self) -> None: + """The shape a lazy `
` pairing loses: PR #1053's own round nests a per-finding + "Prompt for AI Agents" block, and a second finding sitting after that nested block's own + close used to fall outside the captured region entirely, `cr_outside_diff=2` printing + only the first.""" + body = cr_outside_diff_body_multi(["First off-by-one.", "Second off-by-one."]) + self.answer(payload([review(), self.cr_review(body=body)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("cr_outside_diff=2 (on_head=2 earlier=0)", out) + self.assertIn("First off-by-one.", out) + self.assertIn("Second off-by-one.", out) + + def test_a_finding_quoting_shell_redirects_is_not_corrupted_by_the_blockquote_strip( + self, + ) -> None: + """`BLOCKQUOTE` is read on a copy for detection only, never on the returned block: a + naive strip once turned `>&2 echo` into `&2 echo` in the printed finding.""" + body = cr_outside_diff_body(finding="Missing >&2 echo failed on error.") + self.answer(payload([review(), self.cr_review(body=body)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn(">&2 echo failed", out) + + +# Trimmed from Qodo's own "Code Review by Qodo" comment on ptr727/ProjectTemplate PR #1062, one numbered finding rather than the two the live comment carried. +# Its nested Description/Code/Relevance/Evidence/Agent-prompt sub-summaries are kept, since those are exactly what a naive `` scan would miscount as findings of their own. +# The badge's check mark is escaped (U+2713) rather than typed literally, keeping this file's own literal source inside the ASCII charset rule that governs the repository. +def qodo_review_body(resolved: bool = False, heading: str = "Bad naming") -> str: + badge = " \u2713 Resolved" if resolved else "" + label = f"{heading}" if resolved else heading + return ( + "

Code Review by Qodo

\n\n" + "Bugs (1)\n\n" + "
\n" + f" 1. {label}{badge} Bug\n\n" + "
\n\n" + ">
\n>Description\n>
\n>\n>
\n"
+        ">The name does not describe what the function does.\n>
\n>
\n\n" + ">
\n>Code\n>
\n>\n" + ">a.py[12]\n>
\n\n" + ">
\n>Relevance\n>
\n>\n" + ">
Recent history accepts naming corrections.
\n>
\n\n" + "
\n
\n" + ) + + +class TestQodoOpenFindings(GqlCase): + """Qodo's own comment-only findings, its formal review carrying an empty body on every round.""" + + def test_an_open_finding_counts_and_prints_without_its_nested_subsections(self) -> None: + self.answer( + payload( + [review()], comments=[comment(login="qodo-code-review", body=qodo_review_body())] + ) + ) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=1", out) + self.assertIn("QODO OPEN FINDING", out) + self.assertIn("Bad naming", out) + self.assertNotIn("Description", out) + self.assertNotIn("Relevance", out) + + def test_a_resolved_finding_does_not_count_as_open(self) -> None: + self.answer( + payload( + [review()], + comments=[comment(login="qodo-code-review", body=qodo_review_body(resolved=True))], + ) + ) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=0", out) + self.assertNotIn("QODO OPEN FINDING", out) + + def test_qodo_open_prints_zero_rather_than_staying_silent_once_it_has_commented(self) -> None: + """A `0` here is its own reading, Qodo reviewed and left nothing open, not silence.""" + self.answer( + payload( + [review()], + comments=[comment(login="qodo-code-review", body=qodo_review_body(resolved=True))], + ) + ) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=0", out) + + def test_qodo_open_is_absent_where_it_has_never_commented(self) -> None: + self.answer(payload([review()])) + out, _ = pr_review.digest("o", "r", 7) + self.assertNotIn("qodo_open", out) + + def test_the_pr_summary_comment_is_not_read_as_the_findings_comment(self) -> None: + """Qodo posts two comments per round, and only `Code Review by Qodo` carries findings.""" + summary = "

PR Summary by Qodo

\n\nAdds a thing.\n" + self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=summary)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertNotIn("qodo_open", out) + + def test_the_newest_findings_comment_wins_on_a_re_reviewed_pull_request(self) -> None: + self.answer( + payload( + [review()], + comments=[ + comment(login="qodo-code-review", at=EARLY, body=qodo_review_body()), + comment( + login="qodo-code-review", at=LATE, body=qodo_review_body(resolved=True) + ), + ], + ) + ) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=0", out) + + def test_a_finding_titled_about_the_badge_word_itself_is_not_read_as_carrying_it(self) -> None: + """An unanchored match previously read this script's own `isResolved` identifier, quoted + in a finding's title, as the badge, closing an open finding on its own title.""" + body = qodo_review_body(heading="isResolved handling is inconsistent") + self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=1", out) + self.assertIn("isResolved handling is inconsistent", out) + + def test_qodo_open_is_unknown_rather_than_absent_where_its_comment_can_be_behind_the_window( + self, + ) -> None: + """`qodo_open` absent means "never commented", and a window with no Qodo comment in view + cannot tell that from Qodo's own comment simply sitting behind it.""" + full = [comment(login="ptr727") for _ in range(pr_review.WINDOW)] + self.answer(payload([review()], comments=full, older=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=unknown", out) + + def test_qodo_open_is_absent_rather_than_unknown_once_the_window_is_not_blind(self) -> None: + full = [comment(login="ptr727") for _ in range(pr_review.WINDOW)] + self.answer(payload([review()], comments=full, older=False)) + out, _ = pr_review.digest("o", "r", 7) + self.assertNotIn("qodo_open", out) + + def test_qodo_open_is_unknown_where_only_the_paired_summary_comment_is_in_view(self) -> None: + """A visible `PR Summary by Qodo` clears plain `window_blind`, which settles for any of + Qodo's own comments, but says nothing about the findings comment specifically: an older + `Code Review by Qodo` can still be the one that fell out of the window.""" + summary = "

PR Summary by Qodo

\n\nAdds a thing.\n" + full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ + comment(login="qodo-code-review", body=summary) + ] + self.answer(payload([review()], comments=full, older=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=unknown", out) + + def test_a_summary_comment_mentioning_the_findings_heading_in_prose_is_not_selected( + self, + ) -> None: + """`QODO_REVIEW_HEADING` is anchored to the `

` tag the real heading wears, not a + bare substring: a `PR Summary by Qodo` comment whose own prose happens to mention + "Code Review by Qodo" must not be mistaken for the findings comment itself, which + would read a genuinely hidden findings comment as `qodo_open=0` rather than + `unknown`.""" + summary = "

PR Summary by Qodo

\n\nA Code Review by Qodo will follow shortly.\n" + full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ + comment(login="qodo-code-review", body=summary) + ] + self.answer(payload([review()], comments=full, older=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=unknown", out) + self.assertNotIn("qodo_open=0 ", out) + + def test_a_finding_titled_with_the_bare_badge_word_and_no_glyph_is_not_read_as_the_badge( + self, + ) -> None: + """`QODO_BADGE` requires the check mark or cross Qodo's own badge carries, not just the + word: a finding's own title quoting `Resolved` with no glyph is not Qodo's + badge, only something adjacent enough to be mistaken for it on the word alone.""" + body = qodo_review_body(heading="Resolved flag ignored on retry") + self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("qodo_open=1", out) + class TestRefusal(GqlCase): """The review that says it did not review, which carries the head and covers nothing. @@ -3470,7 +3780,19 @@ def test_gh_being_absent_does_not_raise(self) -> None: class TestContract(unittest.TestCase): def test_status_documents_its_no_review_success_case(self) -> None: - self.assertIn("Exit 0 = no review covers the head yet", pr_review.__doc__ or "") + self.assertIn("Exit 0 = no Copilot review covers the head yet", pr_review.__doc__ or "") + + def test_status_documents_review_on_head_as_copilot_scoped(self) -> None: + """`review_on_head=NO` alongside a genuine `other_reviewed` head is a scoping fact, not + a coverage gap, the exact confusion that reached this docstring as a filed issue.""" + doc = pr_review.__doc__ or "" + self.assertIn('never "no review of any kind covers this head"', doc) + self.assertIn("not a gap", doc) + + def test_status_documents_the_coderabbit_and_qodo_finding_fields(self) -> None: + doc = pr_review.__doc__ or "" + self.assertIn("cr_outside_diff=N (on_head=X earlier=Y)", doc) + self.assertIn("qodo_open=N", doc) def test_the_runbook_bootstraps_the_review_skill(self) -> None: """Copilot reaches the provider-independent review contract from its always-on file.""" From 76c87cf6f06314cde9834af2c3f0e1b1a725c493 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 17:06:03 -0700 Subject: [PATCH 3/4] Trim implementation-detail docstrings Qodo flagged on PR #1068 (#1069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #1067, prompted by Qodo's round on the develop -> main promotion PR (#1068). Qodo flagged three real style issues in content #1067 introduced: - `marker_blocks`' docstring recorded implementation history/mechanics rather than its return contract. - Two new test classes carried one-line class-summary docstrings, which `comment-and-doc-style` forbids regardless of length. - A test docstring used historical "previously read" framing instead of a present-tense invariant. `local-strict-review` against this diff (dispatched before this push) caught three more real issues in my own first pass at fixing those three: an inaccurate mechanism claim in a rewritten docstring (said "word-boundaried", the actual mechanism is the required glyph), an inverted rationale in a new inline comment, and a dangling docstring cross-reference this diff itself created. All fixed, all covered by the existing 306-test suite passing unchanged. All local gates pass: `ruff check`/`format`, `mypy`, the full `scripts/tests` suite, `prose_lint.py --diff`, `repo_gate.py --check eol`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Documentation** * Clarified documentation for shared review-comment block extraction and blockquote handling. * Simplified documentation for suppressed review content. * **Tests** * Refined test descriptions for review findings, including requirements for resolved-status indicators. --- scripts/pr_review.py | 28 +++++-------------------- scripts/tests/test_pr_review.py | 37 ++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 38 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index efbdb5d7..20687ab7 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1485,28 +1485,8 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F """Return the review body's sections whose own heading matches `marker`, each sliced from that heading through to the end of its own region. - Shared by `suppressed_blocks` (Copilot's low-confidence findings) and `outside_diff_blocks` - (CodeRabbit's outside-diff-range findings): both are a review body collapsing real - findings into a block that raises no `reviewThreads` entry, so a thread poll alone reports a - clean pass over either. - - The section has worn three shapes so far, all against `SUPPRESSED`: its own `
` - wrapper, a bare heading in the body, and a Markdown heading nested inside the `Review - details` wrapper. The wrapper is the part that keeps moving, so each region is scanned line - by line for the heading rather than read for a wrapper's summary: a nested heading is not a - summary, and stripping the wrappers to look for it outside deletes the very region it sits - in. That is the pair that reported `suppressed=0` over a body carrying `### Suppressed - comments (2)`. - - The heading carries the match rather than the body text, since a review whose prose discusses - the marker's own wording is not itself carrying a block. A region ends the block, so a - section is not read on into the file table that follows it. - - `strip_blockquote` is read on a copy used only to find the heading line, never on what is - returned: CodeRabbit's own outside-diff section wraps its lines in a Markdown blockquote, - which `BLOCKQUOTE` strips for detection, but a finding quoting code (`>&2 echo`, `>> $LOG`) - would otherwise be corrupted by that same strip once it landed in the printed digest. - Left off by default, so `suppressed_blocks` reads exactly as it always has. + Shared by `suppressed_blocks` and `outside_diff_blocks`. `strip_blockquote` affects heading + detection only, never the returned content. """ if not body: return [] @@ -1517,6 +1497,8 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F blocks = [] for region in regions: raw_lines = region.splitlines() + # Scanned line by line rather than read from a wrapper's own summary. + # A heading can sit in the wrapper's body instead, where reading only the summary would miss it. scan_lines = [BLOCKQUOTE.sub("", ln) for ln in raw_lines] if strip_blockquote else raw_lines for i, line in enumerate(scan_lines): if marker.search(line) and (HEADING.match(line) or COUNT.search(line)): @@ -1528,7 +1510,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F def suppressed_blocks(body: str) -> list[str]: """Copilot's own low-confidence findings, collapsed rather than raised as inline threads. - See `marker_blocks` for the shared reading and the shape history behind it. + See `marker_blocks` for the shared reading. """ return marker_blocks(body, SUPPRESSED) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index a8728261..ae6137be 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -879,7 +879,8 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No def test_a_truncated_reviews_window_marks_suppressed_as_undercounting(self) -> None: """An older round old enough to fall out of the 100-review window can carry a finding of its own that `suppressed=` then has no way to read, the same blind spot `threads=` - already carries its own `+` marker for.""" + already carries its own `+` marker for. + """ self.answer(payload([review()], older_reviews=True)) out, _ = pr_review.digest("o", "r", 7) self.assertIn("suppressed=0+", out) @@ -950,8 +951,6 @@ def cr_outside_diff_body_multi(findings: list[str], path: str = "a.py") -> str: class TestCodeRabbitOutsideDiff(GqlCase): - """CodeRabbit's own equivalent of the blind spot `TestSuppressed` covers for Copilot.""" - def cr_review(self, oid: str = HEAD, body: str = "") -> dict: return { "author": {"login": "coderabbitai"}, @@ -1003,7 +1002,8 @@ def test_a_copilot_review_carrying_the_phrase_is_not_a_coderabbit_finding(self) def test_a_truncated_reviews_window_surfaces_cr_outside_diff_even_at_zero(self) -> None: """Silence on `cr_outside_diff` reads as "nothing to triage", so an older CodeRabbit round old enough to fall out of the window must not stay silent just because none of - the rounds still in view happen to carry a finding of their own.""" + the rounds still in view happen to carry a finding of their own. + """ self.answer(payload([review()], older_reviews=True)) out, _ = pr_review.digest("o", "r", 7) self.assertIn("cr_outside_diff=0+", out) @@ -1013,7 +1013,8 @@ def test_every_finding_in_a_multi_finding_section_is_captured(self) -> None: """The shape a lazy `
` pairing loses: PR #1053's own round nests a per-finding "Prompt for AI Agents" block, and a second finding sitting after that nested block's own close used to fall outside the captured region entirely, `cr_outside_diff=2` printing - only the first.""" + only the first. + """ body = cr_outside_diff_body_multi(["First off-by-one.", "Second off-by-one."]) self.answer(payload([review(), self.cr_review(body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1025,7 +1026,8 @@ def test_a_finding_quoting_shell_redirects_is_not_corrupted_by_the_blockquote_st self, ) -> None: """`BLOCKQUOTE` is read on a copy for detection only, never on the returned block: a - naive strip once turned `>&2 echo` into `&2 echo` in the printed finding.""" + naive strip once turned `>&2 echo` into `&2 echo` in the printed finding. + """ body = cr_outside_diff_body(finding="Missing >&2 echo failed on error.") self.answer(payload([review(), self.cr_review(body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1055,8 +1057,6 @@ def qodo_review_body(resolved: bool = False, heading: str = "Bad naming") -> str class TestQodoOpenFindings(GqlCase): - """Qodo's own comment-only findings, its formal review carrying an empty body on every round.""" - def test_an_open_finding_counts_and_prints_without_its_nested_subsections(self) -> None: self.answer( payload( @@ -1120,8 +1120,10 @@ def test_the_newest_findings_comment_wins_on_a_re_reviewed_pull_request(self) -> self.assertIn("qodo_open=0", out) def test_a_finding_titled_about_the_badge_word_itself_is_not_read_as_carrying_it(self) -> None: - """An unanchored match previously read this script's own `isResolved` identifier, quoted - in a finding's title, as the badge, closing an open finding on its own title.""" + """The required glyph, not the bare word, decides. This script's own `isResolved` + identifier, quoted in a finding's title, is not the badge and does not close an open + finding. + """ body = qodo_review_body(heading="isResolved handling is inconsistent") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1132,7 +1134,8 @@ def test_qodo_open_is_unknown_rather_than_absent_where_its_comment_can_be_behind self, ) -> None: """`qodo_open` absent means "never commented", and a window with no Qodo comment in view - cannot tell that from Qodo's own comment simply sitting behind it.""" + cannot tell that from Qodo's own comment simply sitting behind it. + """ full = [comment(login="ptr727") for _ in range(pr_review.WINDOW)] self.answer(payload([review()], comments=full, older=True)) out, _ = pr_review.digest("o", "r", 7) @@ -1147,7 +1150,8 @@ def test_qodo_open_is_absent_rather_than_unknown_once_the_window_is_not_blind(se def test_qodo_open_is_unknown_where_only_the_paired_summary_comment_is_in_view(self) -> None: """A visible `PR Summary by Qodo` clears plain `window_blind`, which settles for any of Qodo's own comments, but says nothing about the findings comment specifically: an older - `Code Review by Qodo` can still be the one that fell out of the window.""" + `Code Review by Qodo` can still be the one that fell out of the window. + """ summary = "

PR Summary by Qodo

\n\nAdds a thing.\n" full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ comment(login="qodo-code-review", body=summary) @@ -1163,7 +1167,8 @@ def test_a_summary_comment_mentioning_the_findings_heading_in_prose_is_not_selec bare substring: a `PR Summary by Qodo` comment whose own prose happens to mention "Code Review by Qodo" must not be mistaken for the findings comment itself, which would read a genuinely hidden findings comment as `qodo_open=0` rather than - `unknown`.""" + `unknown`. + """ summary = "

PR Summary by Qodo

\n\nA Code Review by Qodo will follow shortly.\n" full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ comment(login="qodo-code-review", body=summary) @@ -1178,7 +1183,8 @@ def test_a_finding_titled_with_the_bare_badge_word_and_no_glyph_is_not_read_as_t ) -> None: """`QODO_BADGE` requires the check mark or cross Qodo's own badge carries, not just the word: a finding's own title quoting `Resolved` with no glyph is not Qodo's - badge, only something adjacent enough to be mistaken for it on the word alone.""" + badge, only something adjacent enough to be mistaken for it on the word alone. + """ body = qodo_review_body(heading="Resolved flag ignored on retry") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -3784,7 +3790,8 @@ def test_status_documents_its_no_review_success_case(self) -> None: def test_status_documents_review_on_head_as_copilot_scoped(self) -> None: """`review_on_head=NO` alongside a genuine `other_reviewed` head is a scoping fact, not - a coverage gap, the exact confusion that reached this docstring as a filed issue.""" + a coverage gap, the exact confusion that reached this docstring as a filed issue. + """ doc = pr_review.__doc__ or "" self.assertIn('never "no review of any kind covers this head"', doc) self.assertIn("not a gap", doc) From 659b2819add9928017f72fe419acc93ea2406dec Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 17:48:17 -0700 Subject: [PATCH 4/4] Fix docstring drift Copilot caught on PR #1068 (#1072) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #1067/#1069, prompted by Copilot's round on the develop -> main promotion PR (#1068). Copilot flagged two real docstring/comment inaccuracies: - The `status` docstring's `cr_outside_diff` paragraph claimed it prints only once CodeRabbit has raised a finding, but `digest()` also prints it (as `cr_outside_diff=0+`) once the reviews window is truncated, regardless of what's currently visible. - `TestOtherReviewers`' docstring claimed "identity and commit only", contradicting its own rate-limit-marker tests, which read comment/review body content. `local-strict-review` against this diff (dispatched before this push) caught three follow-on issues in that same rewrite (a garden-path modifier, a reword that read as the opposite of what it meant, and a comment trim from the same review round that had dropped load-bearing rationale), all fixed. All covered by the existing 306-test suite passing unchanged. All local gates pass: `ruff check`/`format`, `mypy`, the full `scripts/tests` suite, `prose_lint.py --diff`, `repo_gate.py --check eol`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * CodeRabbit findings are now reported when the review window is truncated, even if no visible findings were detected. * Truncated results display the count as `0+` when applicable, making incomplete review results clearer. * **Documentation** * Clarified Qodo parser behavior and automated review-bot coverage. * Documented which review checks apply to different automated reviewers without changing parsing behavior. --- scripts/pr_review.py | 22 +++++++++------------- scripts/tests/test_pr_review.py | 11 ++++++----- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 20687ab7..489abdbb 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -66,8 +66,11 @@ poll alone cannot see. `cr_outside_diff=N (on_head=X earlier=Y)` counts CodeRabbit's own "outside diff range" findings, collapsed into the review body rather than raised as an inline review comment because the finding sits on a line outside - the pull request's changed hunks. Printed only once CodeRabbit has raised one, on any - round, so a repository not trialing it stays silent. `qodo_open=N` counts Qodo's own + the pull request's changed hunks. Printed once CodeRabbit has raised one, on any + round, or once the reviews window is truncated, since an older round could then + still carry one unseen (`cr_outside_diff=0+`). Stays silent otherwise, whether that + silence means no finding was raised or CodeRabbit was never trialed at all. + `qodo_open=N` counts Qodo's own numbered findings that carry neither its `Resolved` nor `Dismissed` self-tracked badge: Qodo's formal review carries an empty body on every round observed, so its findings are read from its "Code Review by Qodo" PR-level comment instead, not @@ -216,19 +219,12 @@ # The service name is captured rather than assumed. # A second bot using the same auto-generated-comment convention is read without a new pattern, and one that does not use it stays unread rather than guessed at. RATE_LIMITED = re.compile(r"auto-generated comment:\s*rate limited by\s*(\S+?)\s*-->") -# Qodo's formal review object carries an empty body on every review checked, confirmed across roughly 60. -# Its findings ride a PR-level comment instead. -# Two are posted per round, "PR Summary by Qodo" (an overview, no findings) and this one, told apart by its own heading. -# Anchored to the `

` tag the heading itself wears, not a bare substring: the summary comment's own prose can mention the phrase without being the comment it names. +# Anchored to the `

` tag rather than a bare substring, since prose elsewhere can mention the phrase without being the comment it names. QODO_REVIEW_HEADING = re.compile(r"

\s*Code Review by Qodo\s*

", re.IGNORECASE) -# Qodo nests a Description/Code/Relevance/Evidence/Agent-prompt `` under each of its own numbered findings. -# Only the numbered heading itself is a finding, told apart from those by starting with `N.` the way none of the nested ones do. +# Only the numbered heading counts as a finding, told apart from Qodo's own nested sub-summaries by starting with a number and a period (`1.`, `2.`, ...). QODO_FINDING = re.compile(r"\s*\d+\.\s") -# Qodo's own self-tracked disposition, re-checked against the current head on its own schedule. -# Present on a finding it has re-verified as addressed or intentionally dismissed, absent on one still open. -# A fast pre-triage signal per the runbook, not a substitute for reading the finding: spot-verify against `gh pr diff` rather than trusting it outright. -# The glyph is required rather than left unanchored, since a bare word match reads any finding whose own title quotes the identifier `Resolved` or `Dismissed` in its own `` tag as carrying the badge, closing an open finding on the strength of its own title. -# Escaped rather than typed literally (check mark U+2713, ballot x U+2717), keeping this source inside the ASCII charset rule that governs the repository. +# A finding's own title can quote `Resolved`/`Dismissed` without carrying the badge, so the glyph is required rather than just the word. +# Escaped (U+2713, U+2717) rather than typed literally, per the repository's ASCII charset rule. QODO_BADGE = re.compile(r"[^<]*(?:\u2713 Resolved|\u2717 Dismissed)[^<]*") # A round states how much of the diff it read on a line of its own. # A round that read part of it is the clean pass elsewhere, same commit and threads and digest. diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index ae6137be..6df4ed4c 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -585,19 +585,20 @@ def test_a_body_is_flattened_and_bounded(self) -> None: class TestOtherReviewers(GqlCase): - """Thread resolution and head-presence, generalized past Copilot to the other review bots - this repository has trialed: identity and commit only, no prose parsed for either one here. + """Thread resolution, head-presence, and the rate-limit marker's own structural pattern, + generalized past Copilot to the other review bots this repository has trialed. Free-text + prose parsing for CodeRabbit's outside-diff findings and Qodo's comment-only findings lives + in `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below, not here. `status`'s `unresolved=0` used to hide a CodeRabbit/qodo thread that still blocked a ruleset-gated merge (PR #915, ptr727/ProjectTemplate), since only Copilot's own threads counted. Coverage and refusal reading stay Copilot-only: `review_on_head` above names Copilot's own coverage specifically, the reviewer this script requests and waits for, not - "no review of any kind covers this head" (#1066). CodeRabbit's outside-diff-range findings - and Qodo's comment-only findings are each read too, in their own shape, by `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below (#1058). + "no review of any kind covers this head" (#1066). """ def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict: - """A minimal review node for a tracked non-Copilot reviewer: identity and commit only.""" + """A review node for a tracked non-Copilot reviewer, `body` empty unless a case needs it.""" return { "author": {"login": login}, "state": "COMMENTED",