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: ```markdown # 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: ```markdown ## 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 - Mock violations: {count} - Test coverage issues: {count} - Defensive programming: {count} - Dynamic imports: {count} - Type safety issues: {count} - [etc for all 19 categories] ### Mock Usage Summary - Total new mocks: {count} - Direct fetch mocking: {count} violations - Internal code mocking (AP-4): {count} violations - Third-party mocking: {count} (acceptable) ### Test Quality Summary - Test files modified: {count} - Bad test patterns: {count} - Missing coverage areas: [list] ### Architecture & Design - Adherence to YAGNI: [assessment] - Fail-fast violations: {count} - 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 - Cross-reference with `docs/bad-smell.md` for criteria ## Operation 2: Defensive Code Cleanup Automatically find and remove defensive try-catch blocks that violate the "Avoid Defensive Programming" principle. ### Usage ``` cleanup ``` ### Workflow 1. **Search for Removable Try-Catch Blocks** Search in `turbo/` directory for try-catch blocks matching these BAD patterns: **Pattern A: Log + Return Generic Error** ```typescript try { // ... business logic } catch (error) { log.error("...", error); return { status: 500, body: { error: { message: "Internal server error" } } }; } ``` **Pattern B: Silent Failure (return null/undefined)** ```typescript try { // ... logic } catch (error) { console.error("...", error); return null; } ``` **Pattern C: Log and Re-throw Without Recovery** ```typescript try { // ... logic } catch (error) { log.error("...", error); throw error; } ``` **DO NOT remove** try-catch blocks that have: - Meaningful error recovery logic (rollback, cleanup, retry) - Error type categorization (converting domain errors to HTTP responses) - Fire-and-forget patterns for non-critical operations - Per-item error handling in loops (continue processing other items) - Security-critical code where defensive programming is justified Target: Find up to 10 removable try-catch blocks 2. **Validate Safety** For each identified try-catch block, verify: - No side effects in catch block (only logs and returns/throws) - Framework has global error handler - No cleanup logic (DB rollback, file handles, etc.) - No recovery logic (retry, fallback, degradation) - Not security-critical code (auth/crypto) Create summary table: ```markdown | File | Lines | Pattern | Safe to Remove | Reason | |------|-------|---------|----------------|--------| | path/file.ts | 45-52 | Log + Re-throw | Yes | No recovery logic | | ... | ... | ... | ... | ... | ``` 3. **Modify Code** For each validated catch block: - Remove the try-catch wrapper - Update return types if they change (e.g., `Promise<T | null>` → `Promise<T>`) - Remove unused imports (e.g., `logger` if no longer used) - Update callers if needed (e.g., remove null filtering) Run verification: ```bash cd turbo && pnpm turbo run lint cd turbo && pnpm check-types ``` 4. **Create Pull Request** - Create feature branch: `refactor/defensive-code-cleanup-YYYYMMDD` - Commit with conventional commit message: ``` refactor(scope): remove defensive try-catch blocks Remove defensive try-catch blocks that violate the project's "Avoid Defensive Programming" principle. Files modified: - file1.ts (Pattern A: log + generic error) - file2.ts (Pattern C: log + re-throw) Errors now propagate to framework error handlers instead of being caught and logged defensively. ``` - Scope examples: `web`, `cli`, `core`, `runner`, or omit if multiple packages - Push and create PR with summary table 5. **Monitor CI Pipeline** Monitor CI checks: ```bash 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: ```markdown ## Defensive Code Cleanup Summary ### Files Modified | File | Changes | Pattern Removed | |------|---------|-----------------| | ... | ... | ... |