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..f0871d50 --- /dev/null +++ b/skills/process/code-review/SKILL.md @@ -0,0 +1,165 @@ +--- +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** — 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 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 `. + +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. + +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: + +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 — 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. + +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 |