diff --git a/.agents/skills/agent-conduct/SKILL.md b/.agents/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.agents/skills/agent-conduct/SKILL.md +++ b/.agents/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.agents/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json index b1b9ed80..e7913205 100644 --- a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json +++ b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json @@ -17,6 +17,7 @@ "./skills/drive-pr", "./skills/fleet-conformance-check", "./skills/git-commit-conventions", + "./skills/local-strict-review", "./skills/merge-and-release", "./skills/operational-vs-release-workflow", "./skills/pr-review-conduct", diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 3fbd8ae8..ea6d8faa 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -8e452ce0980b2de6 +fb40d1fd82f81ff3 diff --git a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index 244001b7..060267c4 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -24,6 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. +- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index b52c5522..1e3fb1f7 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -55,7 +55,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Push the branch and open the feature -> develop PR if it does not exist yet. +2. Run `local-strict-review` against the branch's current diff, then push the branch and open + the feature -> develop PR if it does not exist yet. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -101,7 +102,8 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Push the fix, reply with its commit SHA (outcome 1). +- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply + with its commit SHA (outcome 1). - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md new file mode 100644 index 00000000..7a2eb75f --- /dev/null +++ b/.github/skills/local-strict-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: local-strict-review +description: >- + Runs one read-only, adversarial review pass against this branch's current diff against its + target branch, full file context included, on the strongest model tier the session can reach, + before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, + committed, or untracked work is about to be pushed on a PR-bound branch, and whenever + `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for + PR-bound work. Triggers even when the change looks small or the same session already judged its + own diff ready, because a self-review pass judging its own diff inherits its own blind spots, + the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses + `code-review`'s "Review the Change" criteria rather than restating them, and owns only this + local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own + triaging and disposing of what a PR-hosted reviewer finds. +--- + +# Local Strict Review + +## Why This Exists + +A coding agent that finishes a unit of work, judges it ready, and opens the pull request is judging its own diff with the model, and often the blind spots, that wrote it. CodeRabbit, Qodo, and Copilot routinely find real defects that a local pass missed, and each round costs review latency and, for a rate-limited reviewer, shared account-wide quota. A local, full-file-context adversarial pass before the pull request exists catches the same class of defect for a fixed, smaller cost, the same reasoning that already runs local lint before a push instead of waiting for CI. + +## What It Does + +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. + +`git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. + +The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. + +Review criteria are `code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. + +## Running It + +Follow `AGENTS.md` "Context and Delegation Discipline"'s subagent briefing shape: + +```text +Task: adversarial review of this branch's diff against its merge-base with its target branch, + read full surrounding files where the diff hunks alone do not give enough context. +Paths: the files `git diff --name-only "$(git merge-base origin/ HEAD)"` and + `git ls-files --others --exclude-standard` list, mandatory floor. Reading a specific + unchanged caller or consumer beyond that list is in bounds only where a candidate finding's + proof actually depends on it, per code-review's own "follow data and control flow beyond the + edited lines" instruction below, never as an open-ended exploration. +Rules that bind this task: quote `code-review`'s "Review the Change" section into the prompt, + plus flag unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior + differences explicitly. Do not quote "Publish Every Finding", this task's report contract is + the Return line below, not a PR comment or a coverage marker. +Return: one finding per line, file:line, the concrete failure scenario, no severity theater. +Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. +If a rule you were given does not cover what you find, stop and report it. Do not guess, and do + not read a governance file to resolve it. +``` + +**Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. + +## Disposing of Findings + +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." + +## When to Run It + +- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. + +## Mechanics Live Elsewhere + +- Review criteria: `code-review`. +- Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". +- Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. +- Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index ea79a759..84ec8159 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -69,6 +69,8 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. + 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it @@ -94,7 +96,8 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing + the fix, then reply with the fixing commit SHA. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/AGENTS.md b/AGENTS.md index e5073477..33bf3dce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,3 +109,5 @@ Some of the rules above are also packaged as Claude Code / opencode / Codex Skil Adding or changing a managed host tool is packaged as `add-host-tool`. It keeps the cross-platform contract, installer, documentation, test, and native-verification surfaces together. Driving a pull request through its review loop, from a feature branch into `develop` and, when asked, on to a mergeable `develop -> main` promotion PR, disposing of every reviewer finding along the way per `pr-review-conduct`, is packaged as `drive-pr`, new content rather than a rule extracted from a section. Merging a ready promotion PR and dispatching the release it unblocks, refreshing this machine's installed Skills first when the repo is this hub, is `merge-and-release`, its own new-content package, invoked separately from `drive-pr` so the promotion merge and the release dispatch each keep their own explicit go-ahead. + +Running one read-only, adversarial review pass against a branch's current diff against its target branch, full file context included, on the strongest model tier the session can reach, before a unit of PR-bound work is pushed toward a pull request or claimed done, is packaged as `local-strict-review`, new content rather than a rule extracted from a section. `drive-pr`, `pr-review-conduct`, and `agent-conduct` each reference it at the moment they already govern, rather than restating what it does.