nw-sc-review-dimensions

Code Quality Critique Dimensions

When invoked in review mode, apply these critique dimensions to production code and tests.

Persona shift: from implementer (build solutions) to independent peer reviewer (critique solutions). Focus: detect implementation bias | test quality issues | acceptance criteria coverage gaps. Mindset: fresh perspective with critical analysis - assume nothing, verify everything.

Return complete YAML feedback to calling agent for display to user.

Dimension 1: Implementation Bias Detection

Over-Engineering (YAGNI Violations)

Pattern: features, abstractions, or infrastructure without corresponding acceptance criteria.

Examples: Caching layer without performance AC | Generic framework for single use case | Premature abstraction before Rule of Three | Design patterns without demonstrated complexity need | Infrastructure (queues, workers) without scale requirement.

Detection: Compare implementation against AC | Check if feature requested by stakeholder or assumed by developer | Verify performance requirements exist before optimization | Validate abstractions serve 3+ concrete cases.

Severity: Medium to High.

Premature Optimization

Pattern: performance optimization without measurement proving necessity.

Examples: Custom caching without latency tests showing need | Complex O(log n) algorithms when simple O(n) meets AC | Memory optimizations without profiling data | Database denormalization without query analysis.

Detection: check for performance tests | verify AC specify thresholds | look for profiling data. Severity: Medium.

Solving Assumed Problems

Pattern: implementing solutions for problems not in acceptance criteria.

Examples: Multi-tenancy when AC specify single-tenant | Internationalization when AC require English only | Audit logging when AC don't mention compliance.

Detection: map each feature to corresponding AC, flag features without traceability. Severity: Medium to High.

Dimension 2: Test Quality Validation

Implementation Coupling

Pattern: tests depend on implementation details, preventing refactoring.

Examples: Mocking domain objects or application services (violates port-boundary policy) | Asserting on private methods/fields/internal state | Tests break on refactoring despite behavior unchanged | Tests duplicate production logic to verify correctness.

Detection: Check if mocks used inside hexagon (domain/application layers) - VIOLATION | Verify tests call only public interfaces | Confirm tests validate observable behavior, not implementation | Check if Extract Method refactoring would break tests.

Severity: CRITICAL.

Shared Mutable State

Pattern: tests share state causing flakiness, order dependencies, parallel execution failures.

Examples: Database state not reset between tests | Static variables mutated across tests | File system state persists between tests | In-memory caches shared across test methods.

Detection: Run tests in random order - do they still pass? | Run tests in parallel - do they fail? | Check for test setup/teardown creating isolated state | Look for static fields, shared fixtures, class-level state.

Severity: HIGH.

Port-Boundary Violations

Test doubles policy follows the port-boundary rules defined in the tdd-methodology skill.

Severity: HIGH to CRITICAL.

Testing Theater Detection

Pattern: tests creating illusion of safety without verifying real behavior. Single most dangerous test quality issue -- undetected Testing Theater causes catastrophic production failures because the team believes code is tested when it is not.

Concrete patterns to detect:

| Pattern | Detection | Severity | |---------|-----------|----------| | Zero-assertion test | Test method contains no assert statement at all | BLOCKER | | Tautological assertion | assert result is not None, assert isinstance(...), assert True, assertEqual(x, x) as primary assertion | BLOCKER | | Mock-dominated test | Test mocks the SUT or mocks return the expected value directly -- removing production code still passes | BLOCKER | | Circular verification | Test recomputes expected value using same formula as production code | BLOCKER | | Always-green test | try/except wrapping assertions, empty except blocks, bare pass in test body | BLOCKER | | Fully-mocked SUT | Every dependency of the SUT is mocked -- test verifies mock wiring, not behavior | BLOCKER | | Implementation-mirroring | Assertions only on assert_called_once_with / call counts without behavioral outcome check | HIGH | | Assertion-free smoke test | Code executes but asserts only that no exception was thrown (unless that IS the stated requirement) | BLOCKER | | Misleading test name | Test name says "validates X" or "rejects X" but assertion checks unrelated Y | HIGH | | Hardcoded magic oracle | Expected values are unexplained magic numbers not traceable to business rules or AC | HIGH |

Review checklist (apply to every new/modified test):

1. Delete the production code this test covers -- does the test fail? If not: THEATER.
2. Introduce a logic bug (wrong calculation, swapped condition) -- does the test catch it? If not: THEATER.
3. Is every expected value traceable to an acceptance criterion or business rule? If not: SUSPICIOUS.
4. Does the test assert on observable behavior (return values, state changes, side effects at port boundaries)? If it only asserts on types, existence, or internal calls: THEATER.

Severity: BLOCKER for zero-assertion, tautological, mock-dominated, circular, always-green, fully-mocked SUT, and assertion-free patterns. HIGH for implementation-mirroring, misleading names, and hardcoded oracles. A test suite with Theater is worse than no tests -- it creates false confidence.

Dimension 3: Completeness Validation

Missing Acceptance Criteria Coverage

Pattern: not all acceptance criteria have corresponding test coverage.

Detection: Map each AC to test cases | Check coverage report shows 100% AC mapped to tests | Verify error paths have test coverage | Confirm edge cases explicitly tested.

Severity: CRITICAL.

Inadequate Error Scenario Coverage

Pattern: only happy path tested, error handling untested.

Examples: No test for network timeout during payment | No test for database unavailable | No test for invalid input validation | No test for business rule violations.

Detection: count happy path vs error path tests | check exception types have triggering tests. Severity: HIGH.

Dimension 4: RPP Code Smell Detection

Refactoring Priority Premise -- cascading 6-level review. Apply RPP cascade rule: scan and report lower levels first. Do not report L3+ issues until L1-L2 are clean.

RPP Cascade Rule

1. Scan L1 first. If L1 smells found -> report only L1, stop.
2. If L1 clean -> scan L2. If L2 smells found -> report L1 clean + L2 findings, stop.
3. Continue ascending only when lower levels are verified clean.
4. Exception: if --from and --to parameters specified, scan only that range.

L1: Readability (Severity: LOW)

| Smell | Detection Pattern | |-------|-------------------| | Dead Code | Unused imports, unreachable branches, commented-out code | | How-Comments | Comments explaining what code does (not why) | | Magic Strings/Numbers | Literal values without named constants | | Variable/Method Scope | Variables declared far from use, overly broad scope | | Lazy Class | Classes with < 3 methods or single delegation | | Speculative Generality | Abstractions without 3+ concrete uses |

L2: Complexity (Severity: MEDIUM)

| Smell | Detection Pattern | |-------|-------------------| | Long Method | > 20 lines or multiple responsibilities | | Duplicated Code | Same structure in 2+ places (>= 3 lines) | | Complex Conditionals | Nested if/else > 2 levels, boolean expressions with 3+ terms |

L3: Responsibilities (Severity: HIGH)

| Smell | Detection Pattern | |-------|-------------------| | Large Class | > 300 lines or > 10 methods | | Feat