From 4465468d4dc93708edf279aefe2898cc6a72edde Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 20 Aug 2026 18:07:00 -0400 Subject: [PATCH 1/2] Add code-review skill for cross-agent diff review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code's built-in /code-review has no equivalent on pi/copilot/ opencode, so those sessions have no portable way to review a diff for correctness bugs and reuse/simplification/efficiency cleanups. Ports the dimension set and the address-pr-feedback verify-before-reporting discipline (#251) into a skill that needs only git and a rubric — no MCP or witan dependency, so it works the same everywhere the skill catalog installs. Manually exercised in this session against mcp/servers/witan/witan/ cli/local_dispatch.py's diff (43fc1c6): target resolution and the findings table format both held up. A second-platform run (pi/copilot/ opencode) was not performed live and is still open per the task spec. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Jo8EuXWhoatPDqfp1X7jng --- agent-config.toml | 2 + skills/README.md | 1 + skills/process/README.md | 1 + skills/process/code-review/SKILL.md | 126 ++++++++++++++++++ .../code-review/references/dimensions.md | 68 ++++++++++ .../code-review/references/findings-format.md | 45 +++++++ 6 files changed, 243 insertions(+) create mode 100644 skills/process/code-review/SKILL.md create mode 100644 skills/process/code-review/references/dimensions.md create mode 100644 skills/process/code-review/references/findings-format.md diff --git a/agent-config.toml b/agent-config.toml index cb19d85f..ee66bafd 100644 --- a/agent-config.toml +++ b/agent-config.toml @@ -49,6 +49,7 @@ transport = "streamable-http" [skills] address-pr-feedback = "skills/process/address-pr-feedback/SKILL.md" +code-review = "skills/process/code-review/SKILL.md" create-ol-github-issue = "skills/process/create-ol-github-issue/SKILL.md" create-ol-pull-request = "skills/process/create-ol-pull-request/SKILL.md" create-ol-rfc-discussion = "skills/process/create-ol-rfc-discussion/SKILL.md" @@ -95,6 +96,7 @@ skills = ["dagster-code-location-structure"] inherits = ["universal"] skills = [ "address-pr-feedback", + "code-review", "dependency-pruning", "dependency-updates", "deploy-verification", diff --git a/skills/README.md b/skills/README.md index a996c479..0fa4927f 100644 --- a/skills/README.md +++ b/skills/README.md @@ -41,6 +41,7 @@ Skills are organized by **category**. Each skill lives in | process | [`github-issue-triage`](./process/github-issue-triage/SKILL.md) | Audit open GitHub issues to identify stale, completed, or superseded items using parallel codebase cross-referencing | | process | [`github-pr-triage`](./process/github-pr-triage/SKILL.md) | Categorize open PRs across an org by required action (needs first-pass review, has feedback, approved & ready to merge) and optionally act on them | | process | [`address-pr-feedback`](./process/address-pr-feedback/SKILL.md) | Fetch, categorize, address, and resolve GitHub PR review feedback, with pagination for large/long-running PRs | +| process | [`code-review`](./process/code-review/SKILL.md) | Review a diff, branch, path, or PR for correctness bugs and reuse/simplification/efficiency cleanups, with a verify-before-reporting pass | | process | [`renovate-security-triage`](./process/renovate-security-triage/SKILL.md) | Rank open Renovate PRs in your active repos by security urgency using advisory severity, CVSS, and EPSS (read-only) | | process | [`deploy-verification`](./process/deploy-verification/SKILL.md) | Verify a merged config/infra change actually took effect — CD pipeline, pod rollout, running config, before/after metrics, and unintended-environment scope | diff --git a/skills/process/README.md b/skills/process/README.md index 822e7eaf..31599e5d 100644 --- a/skills/process/README.md +++ b/skills/process/README.md @@ -15,5 +15,6 @@ Workflow skills for interacting with external services and developer processes | [`github-issue-triage`](./github-issue-triage/SKILL.md) | Audit open GitHub issues to identify stale, completed, or superseded items using parallel codebase cross-referencing | | [`github-pr-triage`](./github-pr-triage/SKILL.md) | Categorize open PRs across an org by required action (needs first-pass review, has feedback, approved & ready to merge) and optionally act on them | | [`address-pr-feedback`](./address-pr-feedback/SKILL.md) | Fetch, categorize, address, and resolve GitHub PR review feedback, with pagination for large/long-running PRs | +| [`code-review`](./code-review/SKILL.md) | Review a diff, branch, path, or PR for correctness bugs and reuse/simplification/efficiency cleanups, with a verify-before-reporting pass | | [`renovate-security-triage`](./renovate-security-triage/SKILL.md) | Rank open Renovate PRs in your active repos by security urgency using advisory severity, CVSS, and EPSS (read-only) | | [`deploy-verification`](./deploy-verification/SKILL.md) | Verify a merged config/infra change actually took effect — CD pipeline, pod rollout, running config, before/after metrics, and unintended-environment scope | diff --git a/skills/process/code-review/SKILL.md b/skills/process/code-review/SKILL.md new file mode 100644 index 00000000..6fe368cf --- /dev/null +++ b/skills/process/code-review/SKILL.md @@ -0,0 +1,126 @@ +--- +name: code-review +description: > + Review a diff, branch, path, or PR for correctness bugs and + reuse/simplification/efficiency cleanups, using a verify-before-reporting + pass so findings are checked against the actual code rather than + pattern-matched. Portable across agent platforms (Claude Code, pi, + Copilot, OpenCode) — needs only `git diff` and a rubric, no MCP or witan + dependency. Use this skill when asked to "review this diff", "review my + branch", "code review PR #N", "review the changes", "check this for + bugs", or to review staged/unstaged changes before opening a PR. Report-only + by default; only edits code when the request explicitly says to fix the + findings too. +license: BSD-3-Clause +metadata: + category: process +--- + +# Code Review + +Reviews a diff against four dimensions — correctness, simplification, +efficiency, reuse — and reports findings as a severity-ordered table. Every +finding is re-checked against the actual code before it ships, so the +report doesn't carry a pattern-matched guess dressed up as a bug. + +See [references/dimensions.md](references/dimensions.md) for the four +dimensions with worked examples of a real finding vs. a non-finding. See +[references/findings-format.md](references/findings-format.md) for the +table schema and a full worked example. + +## Scope input + +Resolve what to review from the request, in this order: + +1. **No target given** — `git diff` (unstaged) plus `git diff --staged`, + combined. If both are empty, fall back to `git diff HEAD~1` and say + explicitly that's what's being reviewed instead of silently reporting + nothing. +2. **A branch name** — diff against where it forked from the default + branch, not a plain two-dot diff: detect the default branch (`git + symbolic-ref refs/remotes/origin/HEAD`), find the merge base (`git + merge-base `), then `git diff ...`. +3. **A path** — `git diff -- ` (or the branch-scoped equivalent above + if a branch was also named), same default-branch detection. +4. **A PR number** — when `gh` is available and the repo has a GitHub + remote, `gh pr diff `. + +State which of these applied before reporting findings — "reviewing the +diff between `main` and `feature-x`" — so the reader isn't guessing what +was actually in scope. + +## Depth + +Default to high-confidence findings only — the kind you'd stake your name +on, not a maybe. If the user asks for a deeper pass ("be thorough", "don't +hold back", "look harder"), widen to include findings you're less certain +about, and label those explicitly as lower-confidence in the report rather +than presenting them with the same weight as a confirmed bug. There's no +flag or parameter for this — some platforms this skill runs on have no +argument-passing mechanism, so the depth signal has to come from reading +the request, not from a tier number. + +## Dimensions + +Four dimensions, most severe first when findings are reported: + +1. **Correctness** — a bug: wrong output, a crash, or a concrete input that + fails. +2. **Simplification** — unneeded complexity: premature abstraction, dead + branches, a helper that exists for one caller. +3. **Efficiency** — avoidable extra work: N+1 queries, redundant + recomputation, an unnecessary full scan where an indexed lookup exists. +4. **Reuse** — logic in this diff that duplicates something already in the + repo, that should call the existing implementation instead. + +Full rubric with worked examples: [references/dimensions.md](references/dimensions.md). + +## Verification pass + +Before a finding goes in the final report, re-read the exact lines it +claims are broken. If the second read doesn't reproduce the failure +scenario as written, drop the finding — don't soften it into a maybe and +ship it anyway. The diff itself is the evidence here, already in context, +so this is a re-read, not a separate fetch-and-check pass the way it is +when reviewing a PR discussion. + +For a **reuse** finding specifically, verifying means actually locating the +existing implementation (file:line) — "this probably exists elsewhere" is +not verified; grep for it and cite where. + +## Findings format + +Flat, severity-ordered markdown table: + +| # | Severity | File:Line | Summary | Failure scenario | +|---|----------|-----------|---------|-------------------| + +`Failure scenario` is mandatory and concrete — concrete inputs or state +that produce a wrong output or crash. A row that can't state one is a +suspicion, not a finding, and gets dropped in the verification pass above. +For simplification/efficiency/reuse findings, `Failure scenario` becomes +"what it costs" (the maintenance burden, the extra query, the duplicated +logic's drift risk) rather than a crash. + +No inline fixes in the table — those belong to fix mode only, below, so a +plain review never mutates anything by accident. Full schema and a worked +example: [references/findings-format.md](references/findings-format.md). + +If nothing survives the verification pass, say so plainly ("no +high-confidence findings across the four dimensions") rather than padding +the report with low-confidence guesses to have something to show. + +## Fix mode + +Report-only by default. If the request already says to fix the findings +too ("review and fix", "review this and clean it up"), apply fixes for the +confirmed findings *after* the review is complete and reported — never +mid-review, before the list is final. Findings dropped in the verification +pass are never applied. + +## Environment + +Needs `git` (always) and, for PR-number targets, `gh` authenticated against +the target repo. No MCP server, no witan dependency, no repo-specific +tooling beyond that — the scope-resolution rules above work from any +checkout. diff --git a/skills/process/code-review/references/dimensions.md b/skills/process/code-review/references/dimensions.md new file mode 100644 index 00000000..0978e109 --- /dev/null +++ b/skills/process/code-review/references/dimensions.md @@ -0,0 +1,68 @@ +# Dimensions reference + +The four dimensions the [`code-review`](../SKILL.md) skill checks a diff +against, each with a real finding and a look-alike that isn't one. The +distinction in every pair is the same: a finding names a concrete failure +or cost; a non-finding is a style preference or a hypothetical that doesn't +survive the verification pass. + +## Correctness + +A bug: wrong output, a crash, or a concrete input that fails. + +**Finding:** a function divides by `len(items)` without checking for an +empty list — `items=[]` raises `ZeroDivisionError`, and the caller three +lines up in this same diff passes a filtered list that can legitimately be +empty. + +**Not a finding:** a function assumes its argument is non-negative and +isn't defensive about it, but every call site in the diff (and every +existing call site, checked) passes a value already validated upstream. +There's no concrete input that reaches this function and fails — it's a +hypothetical, not a bug in this diff. + +## Simplification + +Unneeded complexity: premature abstraction, dead branches, a helper that +exists for one caller. + +**Finding:** a new `StrategyFactory` class with a single concrete strategy +registered and no second implementation anywhere in the codebase — the +indirection has no caller that benefits from it today. + +**Not a finding:** a helper function extracted for a single caller because +the calling function was already 80 lines and the extraction makes it +readable. One caller doesn't make an extraction premature if the +alternative is a large function — the complexity metric here is +readability, not caller count. + +## Efficiency + +Avoidable extra work: N+1 queries, redundant recomputation, an unnecessary +full scan where an indexed lookup exists. + +**Finding:** a loop that calls `User.objects.get(id=x)` once per iteration +over a list of 200 ids, where a single `User.objects.filter(id__in=ids)` +would do it in one query — confirmed by reading the loop, not assumed from +the pattern alone (some loops iterate a list already fetched in bulk one +line up). + +**Not a finding:** a function recomputes a value on every call instead of +caching it, but it's called once per request and the computation is O(1) — +there's no measurable cost to point at, just a stylistic preference for +memoization. + +## Reuse + +Logic in this diff that duplicates something already in the repo, that +should call the existing implementation instead. + +**Finding:** a new diff adds a hand-rolled retry-with-backoff loop, and +`utils/retry.py:retry_with_backoff` already implements the same thing with +jitter and a max-attempts cap this new code doesn't have — cite the +existing file:line, not just "this probably exists somewhere." + +**Not a finding:** two functions in the diff both call +`.strip().lower()` on user input before comparing it — two lines of +genuinely trivial logic don't warrant extracting a shared helper; that's +premature abstraction in the other direction. diff --git a/skills/process/code-review/references/findings-format.md b/skills/process/code-review/references/findings-format.md new file mode 100644 index 00000000..ea2a7d8d --- /dev/null +++ b/skills/process/code-review/references/findings-format.md @@ -0,0 +1,45 @@ +# Findings format reference + +The [`code-review`](../SKILL.md) skill reports findings as a flat, +severity-ordered markdown table — most severe first, across all four +dimensions together rather than grouped by dimension, since a single +correctness bug usually matters more to the reader than every +simplification finding combined. + +## Schema + +| # | Severity | File:Line | Summary | Failure scenario | +|---|----------|-----------|---------|-------------------| + +- **#** — row order, most severe first. +- **Severity** — `high` / `medium` / `low`. `high` = confirmed correctness + bug or a cost with clear, near-term impact. `medium` = confirmed but + narrower blast radius (an edge case, a rarely-hit path). `low` = + simplification/style-adjacent, correct either way but worth flagging. +- **File:Line** — exact location, `path/to/file.py:42` — not a range unless + the finding genuinely spans one (a duplicated block, a whole function). +- **Summary** — one sentence, the claim itself, no rationale. +- **Failure scenario** — mandatory, concrete. For correctness: the specific + input or state that produces the wrong output or crash. For + simplification/efficiency/reuse: what it costs (the extra query, the + maintenance burden, the drift risk of duplicated logic) — same column, + reframed rather than left blank. + +A row with no concrete failure scenario doesn't ship — see the +verification pass in [SKILL.md](../SKILL.md#verification-pass). + +## Worked example + +Reviewing a diff that adds a batch-import endpoint: + +| # | Severity | File:Line | Summary | Failure scenario | +|---|----------|-----------|---------|-------------------| +| 1 | high | `importers/batch.py:58` | Unbounded query inside a loop | `import_records()` calls `Account.objects.get(id=r.account_id)` once per record; a 500-record batch issues 500 queries and times out under the request's 30s budget past ~300 records (measured against the existing `/health` timeout config) | +| 2 | medium | `importers/batch.py:12` | Empty batch raises instead of returning a 400 | `records=[]` reaches `records[0]` on line 12 before the loop, raising `IndexError` instead of the validation error the endpoint's other empty-input paths return | +| 3 | low | `importers/batch.py:80` | Hand-rolled retry loop duplicates `utils/retry.py:retry_with_backoff` | No functional bug, but this loop lacks the jitter and max-attempts cap the existing helper has — future drift risk if one gets fixed and not the other | + +If the depth was widened per [SKILL.md](../SKILL.md#depth) (user asked for +a thorough pass), a lower-confidence row still gets `Failure scenario` +filled in, just noted as uncertain in the summary: + +| 4 | low (uncertain) | `importers/batch.py:34` | Possible race if two imports for the same account run concurrently | Not confirmed — no lock or transaction observed around the account balance update; would need a concurrent-request test to verify, flagging for awareness rather than asserting as confirmed | From 8d61e367de61bacff268c948513773e3fe69377e Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 20 Aug 2026 18:16:34 -0400 Subject: [PATCH 2/2] code-review: fix scope-resolution gaps and depth/verification conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's PR review (#270) found three real issues, verified against live git behavior before fixing: - Scope rule 1 (no target) used git diff + git diff --staged only, which never show untracked files — a working tree with just one new file looked empty to both and silently fell back to reviewing HEAD~1 instead. Confirmed with a scratch `git init` + untracked file: both diffs empty. - Scope rule 2's origin/HEAD detection had no fallback. Confirmed a bare `git init` with no remote raises "not a symbolic ref" — a fresh or local-only checkout would break branch-target reviews outright. - Scope rule 3's `git diff -- ` only shows unstaged changes; confirmed a fully-staged change at that path produces an empty diff. Switched to `git diff HEAD -- `. - The verification pass said the diff is "the evidence, already in context" while dimensions.md's own worked examples check callers and grep for existing implementations outside the diff, and the Depth section's promise to include lower-confidence findings on a deeper pass directly contradicted the verification pass's unconditional drop-if-unreproduced rule. Made the default-depth vs. widened-depth distinction explicit in both sections instead of leaving them to silently disagree. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Jo8EuXWhoatPDqfp1X7jng --- skills/process/code-review/SKILL.md | 65 +++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/skills/process/code-review/SKILL.md b/skills/process/code-review/SKILL.md index 6fe368cf..f0871d50 100644 --- a/skills/process/code-review/SKILL.md +++ b/skills/process/code-review/SKILL.md @@ -32,16 +32,33 @@ table schema and a full worked example. Resolve what to review from the request, in this order: -1. **No target given** — `git diff` (unstaged) plus `git diff --staged`, - combined. If both are empty, fall back to `git diff HEAD~1` and say - explicitly that's what's being reviewed instead of silently reporting - nothing. +1. **No target given** — combine three sources: `git diff` (unstaged, + tracked changes), `git diff --staged`, and untracked files. Plain `git + diff`/`git diff --staged` never show untracked files — a working tree + containing only a brand-new file looks empty to both — so check `git + status --porcelain` for `??` entries and include them (`git add -N + ` first makes each show up as an addition in the plain `git diff` + without staging its content). Only fall back to `git diff HEAD~1` when + all three are empty, and say explicitly that's what's being reviewed + instead of silently reporting nothing. 2. **A branch name** — diff against where it forked from the default - branch, not a plain two-dot diff: detect the default branch (`git - symbolic-ref refs/remotes/origin/HEAD`), find the merge base (`git - merge-base `), then `git diff ...`. -3. **A path** — `git diff -- ` (or the branch-scoped equivalent above - if a branch was also named), same default-branch detection. + branch, not a plain two-dot diff: detect the default branch via `git + symbolic-ref refs/remotes/origin/HEAD`. That ref only exists once a + remote's HEAD has been set (`git clone` usually does this, but a + fresh/local-only repo or an unset remote won't have it — verified: a + bare `git init` with no remote raises "not a symbolic ref"); when it's + missing, fall back to `gh repo view --json defaultBranchRef --jq + .defaultBranchRef.name` if `gh` and a GitHub remote are available, + otherwise ask the user which branch to diff against rather than + guessing `main` or `master`. However the default branch was found, find + the merge base (`git merge-base `), then `git diff + ...`. +3. **A path** — `git diff HEAD -- ` (covers staged and unstaged + changes to the path in one call — plain `git diff -- ` shows only + unstaged, so a fully-staged change at that path would otherwise look + like an empty diff), plus the same untracked-file handling as rule 1, + scoped to that path. Same default-branch detection as rule 2 if a + branch was also named. 4. **A PR number** — when `gh` is available and the repo has a GitHub remote, `gh pr diff `. @@ -60,6 +77,15 @@ flag or parameter for this — some platforms this skill runs on have no argument-passing mechanism, so the depth signal has to come from reading the request, not from a tier number. +Widening depth changes what the [verification pass](#verification-pass)'s +drop rule means. At the default depth, a finding that doesn't reproduce +gets dropped, full stop. On a widened pass, a finding that doesn't fully +reproduce is *kept*, not dropped — as long as it's explicitly labeled +lower-confidence and its `Failure scenario` states plainly what's +unconfirmed and why (see the lower-confidence row in +[references/findings-format.md](references/findings-format.md#worked-example)). +The drop-if-unreproduced rule is a default-depth rule, not a universal one. + ## Dimensions Four dimensions, most severe first when findings are reported: @@ -78,11 +104,24 @@ Full rubric with worked examples: [references/dimensions.md](references/dimensio ## Verification pass Before a finding goes in the final report, re-read the exact lines it -claims are broken. If the second read doesn't reproduce the failure +claims are broken — and don't stop at the diff when the finding's +correctness turns on something outside it. A guard may already exist in an +unchanged caller, a changed API may violate a contract defined elsewhere in +the repo, or reproducing the scenario may need a definition the diff +doesn't include. The diff is the starting point, not the whole universe of +evidence — this matches [references/dimensions.md](references/dimensions.md)'s +own examples, which check every existing call site for a correctness +non-finding and require citing a real file:line for a reuse finding, not +just what changed. If a claim depends on code outside the diff's scope +(a different file, a different repo, a library's actual behavior), read +that code before the finding ships; if the code needed to verify a claim +is genuinely out of reach in the time available, that's grounds to drop +the finding at default depth (see [Depth](#depth) for the widened-pass +exception) — not to ship it as confirmed anyway. + +At default depth, if the second read doesn't reproduce the failure scenario as written, drop the finding — don't soften it into a maybe and -ship it anyway. The diff itself is the evidence here, already in context, -so this is a re-read, not a separate fetch-and-check pass the way it is -when reviewing a PR discussion. +ship it anyway. For a **reuse** finding specifically, verifying means actually locating the existing implementation (file:line) — "this probably exists elsewhere" is