Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .agents/skills/agent-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<owner>/<repo>/contents/<path>?ref=<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`.

Expand Down
6 changes: 4 additions & 2 deletions .agents/skills/drive-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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
Expand Down Expand Up @@ -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).
Expand Down
72 changes: 72 additions & 0 deletions .agents/skills/local-strict-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -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 `<target>` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin <target>`, and diff against the merge-base, `git diff "$(git merge-base origin/<target> HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/<target>` 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 `<target>` 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/<target> 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.
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
```

**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`.
18 changes: 15 additions & 3 deletions .agents/skills/pr-review-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,20 @@ visible comments, routinely still carries a finding nobody has answered. Treatin
2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed
from a green merge-state. A push makes checks go green *before* the re-review lands, and the
matched review is **read**, not just counted. A review can carry the head SHA and still decline
the PR outright, or say it read only part of the changed files.
the PR outright, or say it read only part of the changed files. `pr_review.py`'s
`review_on_head` names Copilot's own coverage specifically, the currently required reviewer,
not "no review of any kind covers this head": a trialed advisory reviewer (CodeRabbit,
Qodo) carrying the exact head under `other_reviewed`, with an empty review body and no new
threads, is its own ordinary "reviewed, nothing to flag" shape, not a missing review (#1066).
3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which
have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed
in the review body investigated and answered. Those appear in no thread, so polling threads
alone reports a clean pass while they stand.
alone reports a clean pass while they stand. The same holds for CodeRabbit's own
"outside diff range" comments (`cr_outside_diff` in `pr_review.py`'s digest) and for Qodo's
comment-only findings (`qodo_open`): neither opens a `reviewThreads` entry either, so
give each one the same triage the low-confidence findings above already get (#1058). Qodo's own
`Resolved`/`Dismissed` self-tracked badge is a fast pre-triage signal, not a substitute for
reading the finding, spot-verify against `gh pr diff` rather than trusting it outright.
4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved
section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own.
File an issue naming it and quoting the body, rather than guessing what the new wording
Expand All @@ -69,6 +78,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
Expand All @@ -94,7 +105,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.
Expand Down
1 change: 1 addition & 0 deletions .claude-plugin/fleet-skills/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion .claude-plugin/fleet-skills/.source-digest
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8e452ce0980b2de6
8cab4f8afbf9a439
1 change: 1 addition & 0 deletions .claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<owner>/<repo>/contents/<path>?ref=<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`.

Expand Down
6 changes: 4 additions & 2 deletions .claude-plugin/fleet-skills/skills/drive-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Expand Down
Loading