Add code-review skill for cross-agent diff review

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",

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 |
 

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 |

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
+   <file>` 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 <default> <branch>`), then `git diff
+   <merge-base>...<branch>`.
+3. **A path** — `git diff HEAD -- <path>` (covers staged and unstaged
+   changes to the path in one call — plain `git diff -- <path>` 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 <number>`.
+
+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.

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.

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 |