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
