name: code-review description: Review code changes for quality, spec compliance, and OSS readiness. Use when reviewing staged changes, PRs, or specific files for issues. allowed-tools: Read, Glob, Grep, Bash(git:), Bash(cargo:)

Code Review

Instructions

Target Identification

1. Determine review target:

   • If argument is a file path: Review that specific file
   • If argument is a PR number: Run git diff main...HEAD (or appropriate base branch)
   • If no argument: Run git diff --staged to review staged changes
   • If no staged changes: Run git diff to review unstaged changes

2. If no changes found, inform user and stop

Review Process

3. For each changed file, apply the 5 review perspectives in order:

   1. Root Cause Analysis (本質的対応)
   2. Spec Compliance (仕様準拠)
   3. Test Quality (テスト品質)
   4. OSS Readiness (OSS公開適合性)
   5. Guideline Compliance (ガイドライン準拠)

4. Output results in the format specified below

Review Perspectives

1. Root Cause Analysis (本質的対応)

Detect symptomatic fixes that don't address root causes.

Red Flags

Questions to Raise

• "This change treats the symptom. What is the underlying cause?"
• "Why does this condition need to be checked here?"
• "Is this the right layer to handle this issue?"

2. Spec Compliance (仕様準拠)

Verify implementation matches specifications, not vice versa.

Check Process

1. Identify which spec the code relates to:

   • docs-spec/api/*.md - API specifications
   • docs-spec/architecture.md - Architecture decisions
   • docs-spec/adr/*.md - Architecture Decision Records
   • docs-spec/behavior/*.feature - BDD scenarios

2. Read the relevant spec files

3. Compare implementation against spec:

Critical Questions

• "Does this change require a spec update?"
• "Is the implementation bending the spec for convenience?"
• "Should the spec be changed, or the implementation?"

If spec change is warranted, recommend creating/updating ADR to document why.

3. Test Quality (テスト品質)

Ensure tests verify intended behavior, not accidental implementation details.

Accidental vs Intentional Specification

Tests should verify what the code should do (spec), not what it happens to do (implementation).

Red Flags in Test Code

Test Intent Verification

For each test, ask:

1. What spec does this test verify?

   • Map to docs-spec/behavior/*.feature scenario
   • If no spec exists, should one be created?

2. Would this test fail if the spec is violated?

   • False positive: Test passes when behavior is wrong
   • False negative: Test fails when behavior is correct

3. Would this test still pass after valid refactoring?

   • If implementation changes but behavior stays same, test should pass
   • If test breaks on refactor, it's testing implementation

4. Is the assertion meaningful?

   • assert!(result.is_ok()) - Too weak, what should result contain?
   • assert_eq!(result, expected) - Good if expected is from spec

Test Coverage Quality

Questions to Raise

• "This test asserts implementation detail X. Is X part of the spec?"
• "If we refactor the internals, would this test break incorrectly?"
• "What BDD scenario does this test correspond to?"
• "This test doesn't check the error type, only that an error occurred."

4. OSS Readiness (OSS公開適合性)

Ensure code is safe for public repository.

Security Checks

Code Quality Checks

License Compatibility

• Flag new dependencies without checking license
• Warn about GPL dependencies in MIT project

5. Guideline Compliance (ガイドライン準拠)

Check adherence to CLAUDE.md and project conventions.

Code Style

Architecture Rules

Documentation Sync

If code changes affect:

• Public API → Check if docs-spec/api/*.md needs update
• Module structure → Check if docs-spec/architecture.md needs update
• Design decisions → Check if new ADR is needed

Output Format

# Code Review: [target]

## Summary
- Files reviewed: N
- Issues found: N (Critical: X, Warning: Y, Info: Z)

---

## [filename]

### Critical Issues

#### [Issue Title]
- **Perspective**: Root Cause / Spec Compliance / OSS Readiness / Guidelines
- **Location**: Line X-Y
- **Problem**: Description of the issue
- **Current code**:
  ```rust
  problematic code

• Recommendation: What should be done instead
• Suggested fix:

  improved code
  

Warnings

[Warning Title]

• Perspective: ...
• Location: Line X
• Problem: ...
• Recommendation: ...

Info

• [Line X] Minor suggestion: ...

Spec Sync Required

If implementation changes require spec updates:

Action Items

1. Critical: Fix security issue in auth.rs
2. Warning: Update spec for new parameter
3. Info: Consider refactoring duplicate code

---

## Severity Levels

| Level | Criteria | Action |
|-------|----------|--------|
| Critical | Security risk, spec violation, broken functionality | Must fix before commit |
| Warning | Code smell, missing docs, potential issues | Should fix |
| Info | Style, minor improvements | Consider fixing |

---

## Example Review

Code Review: git diff --staged

Summary

• Files reviewed: 2
• Issues found: 3 (Critical: 1, Warning: 1, Info: 1)

src/network/connection.rs

Critical Issues

Hardcoded API Key

• Perspective: OSS Readiness
• Location: Line 45
• Problem: API key hardcoded in source
• Current code:

  let api_key = "sk-1234567890abcdef";
  

• Recommendation: Use environment variable or config file
• Suggested fix:

  let api_key = std::env::var("API_KEY")
      .expect("API_KEY environment variable required");
  

Warnings

Symptomatic Fix

• Perspective: Root Cause Analysis
• Location: Line 78-82
• Problem: Adding sleep to fix race condition
• Current code:

  tokio::time::sleep(Duration::from_millis(100)).await;
  self.connect().await?;
  

• Recommendation: Investigate why connection needs delay. Consider proper synchronization or retry with backoff.

src/audio/engine.rs

Info

• [Line 23] Consider extracting duplicate buffer initialization into helper function

Action Items

1. Critical: Remove hardcoded API key
2. Warning: Investigate connection race condition
3. Info: Refactor buffer initialization