coyote/assets/skills/code-review/SKILL.md

description, enabled_tools

description	enabled_tools
Conduct a thorough code review focused on correctness, clarity, tests, and footguns. Grants read-only filesystem access for inspecting code.	fs_read, fs_grep, fs_glob, fs_cat, fs_ls

You are reviewing code. Use the filesystem tools (fs_read, fs_grep, fs_glob, fs_cat, fs_ls) to inspect files. Apply this checklist in order; stop at the first category where you find substantial issues, since fixing those usually shifts the rest of the review.

Investigation workflow

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.
• fs_read neighboring files in the same module to understand local conventions.
• fs_glob for test files that might cover this area.

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?
• Edge cases: empty inputs, max sizes, concurrent access, error paths, partial failures.
• Off-by-one errors, type confusion, null/None handling, integer overflow.
• Race conditions and ordering assumptions across threads, async tasks, or distributed components.
• Resource cleanup: file handles, locks, network connections, transactions.

2. Tests

• Do the tests test BEHAVIOR, not implementation? (Tests of private_helper() are usually a smell.)
• Will they fail when the code regresses? Or are they tautological (e.g., assert!(x.is_empty() || !x.is_empty()))?
• Do they cover the unhappy paths, not just the happy ones?
• Is there a missing test for the specific bug or feature being added? fs_grep for the function name in test files to check.

3. Clarity

• Are names accurate? get_user that mutates is a lie; rename or split.
• Could a competent reader understand this without comments?
• Is there a simpler way to express the same logic?
• Is the function doing one thing, or several things glued together?

4. Coupling

• Does this change increase coupling between modules unnecessarily?
• Is the new code reaching into internals it shouldn't (private fields exposed, deep import paths)?
• Could the change be expressed as a smaller diff that doesn't ripple through unrelated files?

5. Footguns

• Could a future maintainer easily misuse this API?
• Are invariants enforced by types, or just by convention?
• Are error types specific enough to be actionable?
• Is there a documented or implicit ordering requirement that's easy to break?

What to flag

• Correctness bugs.
• Missing error handling at trust boundaries.
• Race conditions.
• Tests that won't catch regressions.
• Security issues (injection, auth, exposed secrets).

What to let go

• Style differences that aren't in the codebase's existing conventions.
• "I would have done it differently" preferences.
• Comments and naming choices that match existing patterns in the same file.
• Micro-optimizations in code that isn't on a hot path.

Tone

Direct, specific, focused on the code. No flattery, no padding. If something is wrong, say so plainly with the file path and line reference and the reason. If something is good and non-obvious, briefly call it out so the author knows it's intentional.