# Review Code

> Test-quality review skill for unhappy paths, real assertions, DB/E2E coverage, lifecycle edges, and production-grade verification gaps.

- Skill: `matrixorigin/review-code` (Agent Skill)
- Install (CLI): `npx skillmds@latest add matrixorigin/review-code`
- Raw SKILL.md: https://api.skillmd.com/api/skills/matrixorigin/review-code/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: AI & ML
- Author: matrixorigin (https://skillmd.com/u/matrixorigin)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/matrixorigin/review-code

---


# Review Code: Test Quality

This is not a general code review. Answer one question: do the tests prove the
changed behavior, especially when things go wrong?

Hard rule: flag production control flow that depends on natural-language text
matching. Safety, admission, routing, blocking, retry, recovery, evaluation, and
state transitions must use typed fields such as enums, `ErrorKind`,
`result_class`, `exit_semantics`, structured JSON, protocol parsers, AST/token
parsers, or exact machine-owned sentinel fields. Text matching is acceptable
only for UI display/search, rendered-text tests, or explicitly named legacy
`fallback` parsers that are not the primary decision path.

## Task

$ARGUMENTS

## Step 1: Resolve Changed Behavior

```bash
git status --short
git diff --stat
git diff --name-only
```

For each changed behavior, identify:

- public entrypoint or caller;
- state/persistence side effect;
- error/cancellation path;
- existing test file, if any.

## Step 2: Look For Required Test Evidence

| Change signal | Required evidence |
| --- | --- |
| Public function/API/CLI command | Happy path plus at least one invalid input/error path |
| DB write or projection | Test asserts persisted state, not just `Ok(())` |
| Restore/sync/checkpoint | Test covers missing, stale, duplicate, or partial data |
| Auth/permission/capability | Denied case and allowed case |
| State machine/lifecycle | Out-of-order, double-submit, retry, cancellation, terminal-state behavior; every consumer uses the producer-owned status vocabulary |
| Async task/channel/lock | Cancellation/timeout/cleanup or bounded queue behavior |
| Fanout/background work | Partial child completion does not wake/synthesize; lookup miss is not terminal; one canonical group wake after settlement |
| Prompt/tool/skill selection | Test proves the selection rule, not just string presence |

Use `rg` to find tests before reading them:

```bash
rg -n "<function|type|route|event_name>" crates tests web packages --glob '!target/**'
```

## Step 3: Classify Gaps

Missing test:

- A reachable changed behavior has no test at the owning layer.
- A persistence mutation has no state assertion.
- An unhappy path is the main risk and only happy paths are tested.

Weak test:

- Only checks `is_ok()` / `is_err()` with no effect assertion.
- Mocks away the behavior being changed.
- Tests a helper while the bug lives at the integration boundary.
- Snapshot/string test is brittle and does not assert the contract.

Covered:

- Test reaches the public path, asserts the relevant state/output, and includes at least one failure/lifecycle edge when that edge is material.
- Async journey tests assert causal counts (model requests, terminal transitions,
  durable rows, and wake boundaries), not merely final rendered text or elapsed
  time. At least one real transport/public entrypoint test complements producer
  unit tests when the change crosses CLI/server/edge topology.

## Output Contract

```text
Missing Tests:
- <file:line> <behavior not proven, consequence>

Weak Tests:
- <test file:line> <why it can pass while behavior is broken>

Covered:
- <behavior> proven by <test>

Suggested Checks:
- <exact command(s), usually cargo test -p <crate> <filter>>
```

If coverage is adequate, say so and name the tests that prove it.