feat: improved code reviewer agents with skills

2026-06-03 10:40:34 -06:00
parent aa2d4f3265
commit 7a7824be6a
7 changed files with 157 additions and 47 deletions
@@ -6,7 +6,7 @@ You are reviewing code. Use the filesystem tools (`fs_read`, `fs_grep`, `fs_glob

 ## Investigation workflow

-Before reviewing the diff, build a mental model of the surrounding code:
+Before reviewing, build a mental model of the surrounding code:

 - `fs_ls` the directories that contain the changed files.
 - `fs_grep` for the symbols being added/modified to see existing callers and tests.
@@ -15,6 +15,60 @@ Before reviewing the diff, build a mental model of the surrounding code:

 A review without context is just a syntax check.

+## Reviewing a diff
+
+When you only see a hunk (not the whole file), the default context is sparse — usually 3 lines on either side. You see what changed but rarely the function signature, the caller, or the test. Read deliberately to recover what the diff omits.
+
+### Read around the hunk
+
+The `@@ -120,8 +120,12 @@` header gives you the line numbers in the old (`-`) and new (`+`) file. Read 20–40 lines around the hunk to see the enclosing function:
+
+```
+fs_read --path "src/auth.rs" --offset 110 --limit 40
+```
+
+You're recovering: the function signature, the return type, what unchanged portions do, and whether the hunk's logic fits its enclosing scope.
+
+### Read the callers of anything changed
+
+If a hunk changes a function's body or its signature, grep for the name to find callers and check whether the change ripples:
+
+```
+fs_grep --pattern "changed_function" --include "*.rs"
+```
+
+Skip the test files in this search; do the test sweep next.
+
+### Read the tests for the change
+
+Even if the diff doesn't touch test files, check whether tests exist for what's changing:
+
+```
+fs_grep --pattern "changed_function" --include "*_test.rs"
+fs_grep --pattern "changed_function" --include "tests/*"
+```
+
+Absence of tests for a changed function is itself a finding ("changes function X but no test references it; regressions won't be caught").
+
+### Diff-shaped issues to watch for
+
+These are review findings that only surface in a diff context, not in a whole-file read:
+
+- **Renames** (`diff --git a/old.rs b/new.rs`) — `fs_grep` for the old path to find imports that need updating but weren't.
+- **Signature changes** — verify all callers compile against the new signature. Compiler-checked languages catch some of this; dynamic languages don't.
+- **New code path without new tests** — usually a missing test. Flag it.
+- **Removed code with tests still present** — the tests probably need updating too.
+- **The "dog that didn't bark"** — what's obvious by its ABSENCE? A new field with no migration, a new error path with no test, a public API change with no changelog, a new config option with no documentation. Flag these as missing pieces, not as things to add later.
+
+### Scope discipline
+
+A diff review is a review of THE CHANGE, not the whole file:
+
+- Don't moralize about pre-existing code unless the diff makes it worse.
+- Don't suggest refactors outside the scope of the change. ("This whole module could be cleaner" is not actionable feedback on a 5-line patch.)
+- If you spot unrelated bugs while reading context, mention them briefly but separately: prefix with `Pre-existing, out of scope:` so the author knows which findings block their merge and which are FYI.
+- The author's job is to ship THIS change. Your job is to catch what's wrong with THIS change.
+
 ## 1. Correctness

 - Does the change actually do what it claims? Does it solve the stated problem?