From d2291dffcf68c830b8f899e583f76f0cff5d5d87 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 10:45:58 -0700 Subject: [PATCH 1/4] Add local-strict-review skill and wire it into the pre-PR flow Runs 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. Reuses code-review's Review the Change criteria rather than restating them. 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), drive-pr's Drive Loop and finding disposition, and agent-conduct's Before Claiming Done trigger for PR-bound work. Fixes #1056, Fixes #1057, Fixes #1059 --- .agents/skills/agent-conduct/SKILL.md | 1 + .agents/skills/drive-pr/SKILL.md | 6 +- .agents/skills/local-strict-review/SKILL.md | 66 +++++++++++++++++++ .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 | 66 +++++++++++++++++++ .../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 | 66 +++++++++++++++++++ .github/skills/pr-review-conduct/SKILL.md | 5 +- AGENTS.md | 2 + 15 files changed, 229 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..0e536c85 --- /dev/null +++ b/.agents/skills/local-strict-review/SKILL.md @@ -0,0 +1,66 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target, full file context included, on the strongest model tier the session can reach, before a + unit of work is committed toward a pull request or claimed done. Use this whenever staged or + unstaged work is about to become a commit 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 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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. + +The subagent also reads the full content of every file the diff touches, 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 the branch it + targets, read full surrounding files where the diff hunks alone do not give enough context. +Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing + wider. +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, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. + +## 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 commit 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 + +- After staging or committing a unit of work, 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". +- 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..c1a83828 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -8e452ce0980b2de6 +f80a076dfaafa94f 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..0e536c85 --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -0,0 +1,66 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target, full file context included, on the strongest model tier the session can reach, before a + unit of work is committed toward a pull request or claimed done. Use this whenever staged or + unstaged work is about to become a commit 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 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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. + +The subagent also reads the full content of every file the diff touches, 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 the branch it + targets, read full surrounding files where the diff hunks alone do not give enough context. +Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing + wider. +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, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. + +## 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 commit 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 + +- After staging or committing a unit of work, 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". +- 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..0e536c85 --- /dev/null +++ b/.github/skills/local-strict-review/SKILL.md @@ -0,0 +1,66 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target, full file context included, on the strongest model tier the session can reach, before a + unit of work is committed toward a pull request or claimed done. Use this whenever staged or + unstaged work is about to become a commit 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 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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. + +The subagent also reads the full content of every file the diff touches, 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 the branch it + targets, read full surrounding files where the diff hunks alone do not give enough context. +Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing + wider. +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, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. + +## 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 commit 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 + +- After staging or committing a unit of work, 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". +- 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..46dfee80 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, full file context included, on the strongest model tier the session can reach, before a unit of work is committed 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 068f55820e1f70ad7f95ce688d77ecafbe831a08 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 11:09:31 -0700 Subject: [PATCH 2/4] Fix local-strict-review's diff anchor and Paths bounds per review findings CodeRabbit and Qodo independently found that anchoring on @{u} breaks after the first push: git push -u resets a branch's own upstream tracking ref to itself, so a later run diffs the branch against its own last-pushed tip instead of develop, silently narrowing coverage to the latest push instead of the full accumulated diff. Anchor on origin/develop explicitly instead (fetched fresh first), per repo-worktree's own base-branch rule, with origin/main named as the documented exception for a main-based branch. Also: enumerate and read untracked files explicitly, since git diff never reports a path git add has not touched (Qodo); allow narrowly scoped reads into an unchanged caller or consumer when a finding's proof depends on it, instead of a blanket path ceiling that contradicted the imported code-review criteria (Qodo); stop restating AGENTS.md's model-tier rule text and point at it instead (Qodo); reconcile the trigger-timing wording between the frontmatter and When to Run It, and fix a garbled rationale sentence (CodeRabbit). --- .agents/skills/local-strict-review/SKILL.md | 46 +++++++++++-------- .claude-plugin/fleet-skills/.source-digest | 2 +- .../skills/local-strict-review/SKILL.md | 46 +++++++++++-------- .github/skills/local-strict-review/SKILL.md | 46 +++++++++++-------- AGENTS.md | 2 +- 5 files changed, 80 insertions(+), 62 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 0e536c85..09f8f50c 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -2,29 +2,31 @@ name: local-strict-review description: >- Runs one read-only, adversarial review pass against this branch's current diff against its - target, full file context included, on the strongest model tier the session can reach, before a - unit of work is committed toward a pull request or claimed done. Use this whenever staged or - unstaged work is about to become a commit 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. + 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 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. +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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. -The subagent also reads the full content of every file the diff touches, 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. +`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. @@ -33,10 +35,13 @@ Review criteria are `code-review`'s "Review the Change" section, reused rather t 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 the branch it - targets, read full surrounding files where the diff hunks alone do not give enough context. -Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing - wider. +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/develop 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 @@ -47,15 +52,15 @@ If a rule you were given does not cover what you find, stop and report it. Do no not read a governance file to resolve it. ``` -**Model tier:** the strongest tier this session can reach, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. +**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 commit 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." +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 -- After staging or committing a unit of work, 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 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. @@ -63,4 +68,5 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - 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/.source-digest b/.claude-plugin/fleet-skills/.source-digest index c1a83828..37f89ade 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -f80a076dfaafa94f +1f55feda9ffc57fb diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md index 0e536c85..09f8f50c 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -2,29 +2,31 @@ name: local-strict-review description: >- Runs one read-only, adversarial review pass against this branch's current diff against its - target, full file context included, on the strongest model tier the session can reach, before a - unit of work is committed toward a pull request or claimed done. Use this whenever staged or - unstaged work is about to become a commit 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. + 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 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. +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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. -The subagent also reads the full content of every file the diff touches, 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. +`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. @@ -33,10 +35,13 @@ Review criteria are `code-review`'s "Review the Change" section, reused rather t 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 the branch it - targets, read full surrounding files where the diff hunks alone do not give enough context. -Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing - wider. +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/develop 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 @@ -47,15 +52,15 @@ If a rule you were given does not cover what you find, stop and report it. Do no not read a governance file to resolve it. ``` -**Model tier:** the strongest tier this session can reach, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. +**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 commit 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." +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 -- After staging or committing a unit of work, 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 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. @@ -63,4 +68,5 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - 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/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 0e536c85..09f8f50c 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -2,29 +2,31 @@ name: local-strict-review description: >- Runs one read-only, adversarial review pass against this branch's current diff against its - target, full file context included, on the strongest model tier the session can reach, before a - unit of work is committed toward a pull request or claimed done. Use this whenever staged or - unstaged work is about to become a commit 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. + 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 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. +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 against its merge-base with the branch it targets: `git diff "$(git merge-base @{u} HEAD)"`. That single diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, and it is never empty at any of the moments this skill is invoked from, before the first commit or before a later fix commit alike. Reviewing only the most recent staged hunk would recreate the exact incremental blind spot this skill exists to close, since a fresh review of the full accumulated diff is what catches what per-push review misses. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. -The subagent also reads the full content of every file the diff touches, 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. +`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. @@ -33,10 +35,13 @@ Review criteria are `code-review`'s "Review the Change" section, reused rather t 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 the branch it - targets, read full surrounding files where the diff hunks alone do not give enough context. -Paths: exactly the files `git diff --name-only "$(git merge-base @{u} HEAD)"` lists, nothing - wider. +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/develop 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 @@ -47,15 +52,15 @@ If a rule you were given does not cover what you find, stop and report it. Do no not read a governance file to resolve it. ``` -**Model tier:** the strongest tier this session can reach, the same convention `AGENTS.md` states for delegation generally. Reaching for a weaker tier to satisfy some notion of "a different reviewer" is the tier-down mistake `AGENTS.md` "Never tier down the seat holding the judgment" forbids, extended here to reviewing. 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, even unchanged. +**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 commit 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." +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 -- After staging or committing a unit of work, 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 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. @@ -63,4 +68,5 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - 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.md b/AGENTS.md index 46dfee80..fb38b405 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,4 +110,4 @@ Adding or changing a managed host tool is packaged as `add-host-tool`. It keeps 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, full file context included, on the strongest model tier the session can reach, before a unit of work is committed 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. +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 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 1f0599735ffa8efa828d1d74fb24d71503bd23b0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 11:35:08 -0700 Subject: [PATCH 3/4] Resolve the target-branch placeholder and AGENTS.md scope inconsistency CodeRabbit found that the skill documented main as a valid target but every command hard-coded origin/develop, so a main-based branch would review from the wrong merge-base. Resolve once and reuse it in the fetch and both merge-base commands instead of a literal develop alongside the documented main exception. Also scope AGENTS.md's local-strict-review paragraph to PR-bound work explicitly, matching the skill's own trigger and agent-conduct's wiring, rather than reading as any claimed-done work. --- .agents/skills/local-strict-review/SKILL.md | 4 ++-- .claude-plugin/fleet-skills/.source-digest | 2 +- .../fleet-skills/skills/local-strict-review/SKILL.md | 4 ++-- .github/skills/local-strict-review/SKILL.md | 4 ++-- AGENTS.md | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 09f8f50c..93eda758 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. +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)"`. 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. @@ -37,7 +37,7 @@ 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/develop HEAD)"` and +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 diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 37f89ade..8f4d1d19 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -1f55feda9ffc57fb +e0a0b1db7fc3f9c1 diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md index 09f8f50c..93eda758 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. +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)"`. 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. @@ -37,7 +37,7 @@ 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/develop HEAD)"` and +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 diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 09f8f50c..93eda758 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead: fetch the target first, `git fetch origin develop`, then diff against the merge-base, `git diff "$(git merge-base origin/develop HEAD)"`. 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. +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)"`. 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. @@ -37,7 +37,7 @@ 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/develop HEAD)"` and +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 diff --git a/AGENTS.md b/AGENTS.md index fb38b405..33bf3dce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,4 +110,4 @@ Adding or changing a managed host tool is packaged as `add-host-tool`. It keeps 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 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. +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 99dd1daa775292da183dc6244b158db24f3ac850 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 12:00:26 -0700 Subject: [PATCH 4/4] Fail closed on a fetch failure instead of reviewing a stale target ref CodeRabbit: git fetch origin had no success guard before the merge-base and diff commands ran, so a failed fetch could leave an existing local origin/ ref in place and the review would silently proceed against stale target data. State explicitly that a failed fetch stops the pass rather than falling through. --- .agents/skills/local-strict-review/SKILL.md | 2 +- .claude-plugin/fleet-skills/.source-digest | 2 +- .claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md | 2 +- .github/skills/local-strict-review/SKILL.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 93eda758..7a2eb75f 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## 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)"`. 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. +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. diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 8f4d1d19..ea6d8faa 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -e0a0b1db7fc3f9c1 +fb40d1fd82f81ff3 diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md index 93eda758..7a2eb75f 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## 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)"`. 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. +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. diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 93eda758..7a2eb75f 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## 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)"`. 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. +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.