/nw-sc-review-dimensions

name: nw-sc-review-dimensions
description: Reviewer critique dimensions for peer review - implementation bias detection, test quality validation, completeness checks, and priority validation
user-invocable: false
disable-model-invocation: true

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 f