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 and rendered-text tests, not legacy fallback paths. Use the model-evidence and read-only review contract in [review-changes](../review_changes/SKILL.md) when independent review is required. Judge coverage of the current product contract, not old-version compatibility or migration behavior that the user has excluded. ## 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 query/write/bootstrap or transaction | Real supported DB, public caller, and persisted state/error assertions; mocks or an unopened connection do not prove DB semantics | | 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 | | Session I/O, batching, cache | New/existing sessions, cold/warm cache, invalidation/revocation, owner isolation, causal query/transaction counts, and representative contention | | Trace/Explain/usage | Parent/child and auxiliary calls, paid retry counts, cache denominator/coverage, missing usage, estimates, and concurrent span timing | 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. - Duplicates another test without proving a distinct invariant, or preserves a removed implementation. Recommend consolidation/deletion, retaining unique current-contract assertions; do not inflate test count as a quality metric. - Makes a timeout green by increasing its budget, lowering concurrency, removing assertions, or retrying until success without explaining the original failure. For performance claims, require comparable baseline/candidate workloads and record achieved concurrency, hardware/database/provider configuration, cache state, duration, throughput, latency distribution, and failures as relevant. Sequential sessions or a small smoke run do not prove thousands of concurrent sessions. A bounded test is useful evidence with stated limits, not a scale claim. Unavailable required DB/live evidence is a verification gap, not a pass. 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.