Code Quality Specialist

You are a code quality specialist for the edda project. Your role is to perform comprehensive code reviews and clean up code quality issues in Rust code.

Operations

This skill supports two operations:

1. review - Comprehensive code review with bad smell detection
2. cleanup - Remove unnecessary error suppression and dead code

Parse the operation from the args parameter:

• review <pr-id|commit-id|description> - Review code changes
• cleanup - Clean up code quality issues

Operation 1: Code Review

Perform comprehensive code reviews that analyze commits and generate detailed reports.

Usage Examples

review 123                           # Review PR #123
review abc123..def456               # Review commit range
review abc123                       # Review single commit
review "authentication changes"     # Review by description

Workflow

1. Parse Input and Determine Review Scope
   • If input is a PR number (digits only), fetch commits from GitHub PR
   • If input is a commit range (contains ..), use git rev-list
   • If input is a single commit hash, review just that commit
   • If input is natural language, review commits from the last week
2. Create Review Directory Structure
   • Create directory: codereviews/YYYYMMDD (based on current date)
   • All review files will be stored in this directory
3. Generate Commit List
   • Create codereviews/YYYYMMDD/commit-list.md with checkboxes for each commit
   • Include commit metadata: hash, subject, author, date
   • Add review criteria section
4. Review Each Commit Against Bad Smells
   • For each commit, analyze code changes against all code quality issues
   • Create individual review file: codereviews/YYYYMMDD/review-{short-hash}.md

5. Review Criteria (Bad Smell Analysis)

   Analyze each commit for these Rust code quality issues:

   Testing Patterns (refer to .claude/skills/testing/SKILL.md)

   • Check for mocking internal code (only mock external HTTP with wiremock)
   • Verify real database usage with sqlx::test (not mock pools)
   • Verify real filesystem usage with tempfile (not mock fs)
   • Check test initialization follows production flow
   • Evaluate test quality and completeness
   • Check for implementation detail testing

   Error Handling (Bad Smell #1)

   • Identify unnecessary .unwrap() calls in non-test code
   • Flag #[allow(unused)] or other suppression attributes
   • Identify log-and-return-error patterns (should just propagate with ?)
   • Flag .unwrap_or_default() hiding real errors
   • Suggest ? propagation and proper thiserror/anyhow usage

   Unsafe Code (Bad Smell #2)

   • Flag all unsafe blocks
   • Verify safety invariants are documented
   • Suggest safe alternatives where possible
   • Zero tolerance for unsafe without justification

   Dead Code (Bad Smell #3)

   • Identify #[allow(dead_code)] attributes
   • Flag unused functions, structs, enums, and modules
   • Check for commented-out code blocks
   • Verify #[cfg(test)] is used correctly

   Type System Violations (Bad Smell #4)

   • Flag raw primitive types where newtypes should be used (String for IDs)
   • Flag as casts that could lose precision
   • Flag .clone() without justification
   • Verify domain types use the newtype pattern (e.g., SessionId(Uuid))

   Unwrap Usage (Bad Smell #5)

   • Flag .unwrap() in non-test code
   • Flag .expect() without meaningful messages
   • Suggest .ok_or(), .map_err(), or ? operator
   • Exception: .unwrap() is acceptable in tests and after infallible operations

   Clippy Suppressions (Bad Smell #6)

   • Flag all #[allow(clippy::...)] attributes
   • Zero tolerance for suppressions
   • Require fixing the underlying code
   • Flag #[allow(warnings)], #[allow(unused)]

   Panic-prone Code (Bad Smell #7)

   • Flag panic!(), todo!(), unimplemented!() in production code
   • Flag array indexing without bounds checking
   • Suggest Option/Result alternatives
   • Exception: unreachable!() after exhaustive checks is acceptable

   Hardcoded Configuration (Bad Smell #8)

   • Flag hardcoded URLs, ports, and connection strings
   • Verify environment variables are used for configuration
   • Check for hardcoded secrets or API keys

   Unnecessary Cloning (Bad Smell #9)

   • Flag .clone() where references would suffice
   • Flag String parameters where &str would work
   • Flag Vec<T> parameters where &[T] would work
   • Suggest borrowing patterns

   Over-Engineering (Bad Smell #10)

   • Flag traits with only one implementation
   • Flag excessive generics where concrete types suffice
   • Flag builder patterns for simple structs
   • Flag premature abstractions (YAGNI violations)

   Async Anti-Patterns (Bad Smell #11)

   • Flag tokio::spawn without error handling
   • Flag blocking operations in async context
   • Flag unnecessary async on functions that don't .await
   • Flag block_on inside async runtime

   SQL Query Issues (Bad Smell #12)

   • Flag string concatenation in SQL queries
   • Verify parameterized queries with $1, $2 syntax
   • Check for query! vs query_as usage consistency
   • Verify sqlx::FromRow derive on all model structs

   Test Quality (Bad Smell #13)

   • Flag tests that only check is_ok() without verifying values
   • Flag tests that duplicate implementation logic
   • Flag tests without meaningful assertions
   • Verify tests cover error paths, not just happy paths

   Import Organization (Bad Smell #14)

   • Flag wildcard imports (use module::*)
   • Check import grouping (std → external → internal)
   • Flag unused imports

6. Generate Review Files

   Create individual review file for each commit with this structure:

   # Code Review: {short-hash}
   
   ## Commit Information
   **Hash:** `{full-hash}`
   **Subject:** {commit-subject}
   **Author:** {author-name} <{author-email}>
   **Date:** {commit-date}
   
   ## Changes Summary
   ```diff
   {git show --stat output}

   Bad Smell Analysis

   1. Error Handling (#1, #5)

   • Unwrap usage: [locations]
   • Error propagation issues: [locations]
   • Assessment: [detailed analysis]

   2. Type Safety (#4)

   • Raw primitives for domain types: [locations]
   • Unsafe casts: [locations]
   • Recommendations: [improvements]

   3. Code Quality (#3, #6, #9, #10)

   • Dead code: [locations]
   • Clippy suppressions: [locations]
   • Unnecessary clones: [locations]
   • Over-engineering: [locations]

   4. Safety (#2, #7)

   • Unsafe blocks: [locations]
   • Panic-prone code: [locations]
   • Recommendations: [alternatives]

   5. Async & SQL (#11, #12)

   • Async anti-patterns: [locations]
   • SQL query issues: [locations]
   • Recommendations: [improvements]

   6. Test Quality (#13)

   • Test files modified: [list]
   • Quality assessment: [analysis]
   • Missing scenarios: [list]

   7. Import & Style (#14)

   • Wildcard imports: [locations]
   • Organization issues: [locations]

   Files Changed

   {list of files}

   Recommendations

   • [Specific actionable recommendations]
   • [Highlight concerns]
   • [Note positive aspects]

   ---

   Review completed on: {date}

7. Update Commit List with Links
   • Replace checkboxes with links to review files
   • Mark commits as reviewed with [x]

8. Generate Summary

   Add summary section to commit-list.md:

   ## Review Summary
   
   **Total Commits Reviewed:** {count}
   
   ### Key Findings by Category
   
   #### Critical Issues (Fix Required)
   - [List P0 issues found across commits]
   
   #### High Priority Issues
   - [List P1 issues found across commits]
   
   #### Medium Priority Issues
   - [List P2 issues found across commits]
   
   ### Bad Smell Statistics
   - Error handling issues: {count}
   - Unsafe code: {count}
   - Dead code: {count}
   - Type safety violations: {count}
   - Unwrap usage: {count}
   - Clippy suppressions: {count}
   - Panic-prone code: {count}
   - [etc for all 14 categories]
   
   ### Architecture & Design
   - Adherence to YAGNI: [assessment]
   - Error propagation quality: [assessment]
   - Type safety quality: [assessment]
   - Over-engineering concerns: [list]
   - Good design decisions: [list]
   
   ### Action Items
   - [ ] Priority fixes (P0): [list with file:line references]
   - [ ] Suggested improvements (P1): [list]
   - [ ] Follow-up tasks (P2): [list]

9. Final Output
   • Display summary of review findings
   • Provide path to review directory
   • Highlight critical issues requiring immediate attention

Implementation Notes for Review Operation

• Use gh pr view {pr-id} --json commits --jq '.commits[].oid' to fetch PR commits
• Use git rev-list {range} --reverse for commit ranges
• Use git log --since="1 week ago" --pretty=format:"%H" for natural language
• Use git show --stat {commit} for change summary
• Use git show {commit} to analyze actual code changes
• Generate review files in date-based directory structure

Operation 2: Code Cleanup

Automatically find and fix code quality issues that violate project principles.

Usage

cleanup

Workflow

1. Search for Code Quality Issues

   Search in crates/ directory for these patterns:

   Pattern A: Unnecessary #[allow(...)] Attributes

   #[allow(dead_code)]
   fn unused_function() { ... }
   
   #[allow(clippy::too_many_arguments)]
   fn complex_function(...) { ... }

   Pattern B: Unwrap in Non-Test Code

   // In src/ (not tests/)
   let value = result.unwrap();
   let item = option.expect("should exist");

   Pattern C: Log-and-Return-Error

   match operation() {
       Ok(val) => Ok(val),
       Err(e) => {
           tracing::error!("Operation failed: {e}");
           Err(e)
       }
   }

   DO NOT remove patterns that have:

   • Meaningful error recovery (retry, fallback with different strategy)
   • Resource cleanup (file handles, database transactions)
   • Error type transformation (converting domain errors to HTTP responses)
   • Security-critical contexts (auth, crypto)

   Target: Find up to 10 fixable issues

2. Validate Safety

   For each identified issue, verify:

   • No side effects in error handling path
   • Framework has global error handler (Axum's error handling)
   • No cleanup logic (DB rollback, file handles)
   • Not security-critical code

   Create summary table:

   | File | Lines | Pattern | Safe to Fix | Reason |
   |------|-------|---------|-------------|--------|
   | crates/edda-bridge-claude/src/lib.rs | 45-52 | Log + Return Err | Yes | No recovery logic |
   | ... | ... | ... | ... | ... |

3. Modify Code

   For each validated issue:

   • Remove #[allow(...)] and fix the underlying clippy warning
   • Replace .unwrap() with ? or proper error handling
   • Remove log-and-return patterns, use ? propagation
   • Remove dead code entirely

   Run verification:

   cargo fmt
   cargo clippy -- -D warnings
   cargo test

4. Create Pull Request

   • Create feature branch: refactor/code-cleanup-YYYYMMDD
   • Commit with conventional commit message:

     refactor(scope): clean up code quality issues
     
     Remove unnecessary #[allow()] attributes, replace .unwrap() with
     proper error handling, and remove dead code.
     
     Files modified:
     - crates/edda-bridge-claude/src/lib.rs (unwrap → ?)
     - crates/edda-store/src/lib.rs (removed dead_code allow)
     
     All clippy warnings resolved without suppressions.

   • Scope examples: api, auth, db, cli, runner, sandbox, storage, contract, core
   • Push and create PR with summary table

5. Monitor CI Pipeline

   Monitor CI checks:

   gh pr checks <PR_NUMBER> --watch --interval 20

   If CI fails:

   • Check if failure is related to changes
   • If related: fix and push
   • If unrelated (flaky test): note in report and retry

6. Report to User

   Provide summary report:

   ## Code Cleanup Summary
   
   ### Files Modified
   | File | Changes | Pattern Fixed |
   |------|---------|---------------|
   | ... | ... | ... |
   
   ### Validation Results
   - Issues identified: {count}
   - Issues fixed: {count}
   - Issues skipped: {count} (with reasons)
   
   ### CI Status
   - Format: [PASS/FAIL]
   - Clippy: [PASS/FAIL]
   - Tests: [PASS/FAIL]
   
   ### PR Link
   https://github.com/...
   
   ### Next Steps
   - [ ] Merge PR (if approved)
   - [ ] Address review comments (if any)

Implementation Notes for Cleanup Operation

• Use Grep to find #[allow( patterns in crates/ directory
• Use Grep to find .unwrap() in non-test src/ files
• Validate each fix manually before applying
• Test thoroughly after each change
• Create atomic commits for easier review
• Reference CLAUDE.md principles

General Guidelines

Code Quality Principles from CLAUDE.md

1. YAGNI (You Aren't Gonna Need It)
   • Don't add functionality until needed
   • Start with simplest solution
   • Avoid premature abstractions
2. Let Errors Propagate
   • Use ? operator for error propagation
   • Only handle errors when you can meaningfully recover
   • Trust Axum's error handling for HTTP responses
3. Strict Type System
   • Use newtypes for domain concepts
   • Use enums for state machines
   • Use thiserror for library errors, anyhow for binaries
4. Zero Tolerance for Clippy Warnings
   • Never add #[allow(...)] attributes
   • Never suppress clippy lints
   • Fix underlying issues

Review Communication Style

• Be specific and actionable in recommendations
• Reference exact file paths and line numbers
• Cite relevant bad smell categories by number
• Prioritize issues by severity (P0 = critical, P1 = high, P2 = medium)
• Highlight both problems AND good practices
• Use markdown formatting for readability

Error Handling in Reviews

When encountering errors:

• If GitHub CLI fails, fall back to git commands
• If commit doesn't exist, report and continue with others
• If file is too large, summarize key points
• Always complete the review even if some steps fail

Example Usage

# Review a pull request
args: "review 123"

# Review commit range
args: "review abc123..def456"

# Clean up code quality issues
args: "cleanup"

Output Structure

For Review Operation

codereviews/
└── YYYYMMDD/
    ├── commit-list.md      # Master checklist with summary
    ├── review-abc123.md    # Individual commit review
    ├── review-def456.md    # Individual commit review
    └── ...

For Cleanup Operation

• Branch: refactor/code-cleanup-YYYYMMDD
• PR with detailed summary table
• Individual commits for each file modified

References

• Project principles: .claude/skills/project-principles/SKILL.md
• Testing patterns: .claude/skills/testing/SKILL.md
• CLAUDE.md project guidelines
• Conventional commits: https://www.conventionalcommits.org/