code-quality

Code Quality Specialist

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

Operations

This skill supports two operations:

1. review - Comprehensive code review with bad smell detection
2. cleanup - Remove defensive try-catch blocks

Your args are: $ARGUMENTS

Parse the operation from the args above:

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

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

   • Read the bad smell documentation from docs/bad-smell.md
   • For testing-related changes, read testing spec from docs/testing.md
   • For React, ccstate, cache, Store, ref, or resource-lifecycle changes, read docs/cache.md
   • 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 code quality issues:

   Testing Patterns (refer to docs/testing.md)

   • Check for AP-4 violations (mocking internal code with relative paths)
   • Verify MSW usage for HTTP mocking (not direct fetch mocking)
   • Verify real filesystem usage (not fs mocks)
   • Check test initialization follows production flow
   • Evaluate test quality and completeness
   • Check for fake timers, partial mocks, implementation detail testing
   • Verify mocks are reset through the package's standard centralized cleanup

   React, ccstate, Cache, and Resource Lifecycles (refer to docs/cache.md)

   • Keep React render pure and do not allocate signal identities during render
   • Reject unbounded lifetime caches and state whose owner outlives its domain
   • Avoid duplicate mutable sources of truth and parallel state machines
   • Verify callback-ref stability and preserve onRef cleanup returns
   • Require symmetric teardown for listeners, timers, observers, object URLs, editors, subscriptions, and async work
   • Inspect helper, chaining, and nested-callback shapes that can evade lint

   Error Handling (Bad Smell #3)

   • Identify unnecessary try/catch blocks
   • Flag defensive programming patterns:
     • Log + return generic error
     • Silent failure (return null/undefined)
     • Log and re-throw without recovery
   • Suggest fail-fast improvements

   Interface Changes (Bad Smell #4)

   • Document new/modified public interfaces
   • Highlight breaking changes
   • Review API design decisions

   Deployment Compatibility

   • Read docs/deployment-compatibility.md when changes touch frontend/backend, runner/backend, queue payloads, or persisted state
   • Verify old frontend requests still work with the new backend while open browser pages keep already-loaded code
   • Verify old runner requests still work with the new backend while old runners drain active runs
   • Verify new frontend or runner code can tolerate old backend responses during rollout when deployment order can overlap
   • Flag one-shot protocol flips that require all deployable surfaces to update at exactly the same time
   • Ensure temporary compatibility logic has an explicit cleanup condition or follow-up issue

   Timer and Delay Analysis (Bad Smell #5)

   • Identify artificial delays in production code
   • Flag useFakeTimers/advanceTimers in tests
   • Flag timeout increases to pass tests
   • Suggest deterministic alternatives

   Dynamic Imports (Bad Smell #6)

   • Flag all dynamic import() usage
   • Suggest static import alternatives
   • Zero tolerance unless truly justified

   Database Mocking in Route Tests (Bad Smell #7)

   • Flag database or internal-service mocking in apps/api route tests
   • Verify real database connections are used

   Test Mock Cleanup (Bad Smell #8)

   • Verify mock cleanup follows the package convention (resetApiTestMocks, Vitest clearMocks, or dedicated test helpers)
   • Check for potential mock state leakage

   TypeScript any Usage (Bad Smell #9)

   • Flag all any type usage
   • Suggest unknown with type narrowing

   Artificial Delays in Tests (Bad Smell #10)

   • Flag setTimeout, sleep, delay in tests
   • Flag fake timer usage
   • Suggest proper async/await patterns

   Hardcoded URLs (Bad Smell #11)

   • Flag hardcoded URLs and environment values
   • Verify usage of env() configuration

   Direct Database Operations in Tests (Bad Smell #12)

   • Flag direct DB operations for test setup
   • Suggest using API endpoints instead

   Fallback Patterns (Bad Smell #13)

   • Flag fallback/recovery logic
   • Suggest fail-fast alternatives
   • Verify configuration errors fail visibly

   Lint/Type Suppressions (Bad Smell #14)

   • Flag eslint-disable, @ts-ignore, @ts-nocheck
   • Zero tolerance for suppressions
   • Require fixing root cause

   Bad Tests (Bad Smell #15)

   • Flag tests that only verify mocks
   • Flag tests that duplicate implementation
   • Flag over-testing of error responses and schemas
   • Flag testing UI implementation details
   • Flag testing specific UI text content

   Mocking Internal Code - AP-4 (Bad Smell #16)

   • Flag vi.mock() of relative paths (../../ or ../)
   • Flag mocking of globalThis.services.db
   • Flag mocking of internal services
   • Only accept mocking of third-party node_modules packages

   Filesystem Mocks (Bad Smell #17)

   • Flag filesystem mocking in tests
   • Suggest using real filesystem with temp directories
   • Note: One known exception in ip-pool.test.ts (technical debt)

   Unit Tests for Internal Functions (Bad Smell #18)

   • Flag test files that directly import and test internal/private functions
   • Tests should only exercise public entry points (API routes, CLI commands, exported module interfaces)
   • Internal logic should be covered indirectly through integration tests
   • Only integration tests are acceptable — no unit tests for internal functions

   Test Initialization Flow (Bad Smell #19)

   • Flag tests that bypass production initialization flow
   • Platform page tests must use detachedSetupPage() or equivalent production initialization
   • Tests should not manually construct internal state that production code initializes differently
   • Test setup should mirror how the code actually runs in production

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. Mock Analysis (Bad Smell #1, #16)

   • New mocks found: [list]
   • Direct fetch mocking: [yes/no + locations]
   • Internal code mocking: [yes/no + locations]
   • Assessment: [detailed analysis]

   2. Test Coverage (Bad Smell #2, #15)

   • Test files modified: [list]
   • Quality assessment: [analysis]
   • Bad test patterns: [list issues]
   • Missing scenarios: [list]

   3. Error Handling (Bad Smell #3, #13)

   • Try/catch blocks: [locations]
   • Defensive patterns: [list violations]
   • Fallback patterns: [list violations]
   • Recommendations: [improvements]

   4. Interface Changes (Bad Smell #4)

   • New/modified interfaces: [list]
   • Breaking changes: [list]
   • API design review: [assessment]

   5. Timer and Delay Analysis (Bad Smell #5, #10)

   • Timer usage: [locations]
   • Fake timer usage: [locations]
   • Artificial delays: [locations]
   • Recommendations: [alternatives]

   6. Code Quality Issues

   • Dynamic imports (Bad Smell #6): [locations]
   • TypeScript any (Bad Smell #9): [locations]
   • Hardcoded URLs (Bad Smell #11): [locations]
   • Lint suppressions (Bad Smell #14): [locations]

   7. Test Infrastructure Issues

   • Database mocking (Bad Smell #7): [locations]
   • Mock cleanup (Bad Smell #8): [assessment]
   • Direct DB ops (Bad Smell #12): [locations]
   • Filesystem mocking (Bad Smell #17): [locations]
   • Unit tests for internals (Bad Smell #18): [locations]
   • Test initialization bypass (Bad Smell #19): [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 i
   

Content truncated.