/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.

Supply complete findings and a plain diagnostic judgment.

---

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.

Test Time

Pattern: a test exceeds the default budgets in a feature without a granted derogation, or exceeds its feature's derogation, or adapter fault injection exists in a feature without a granted derogation (`test:feedback-loop-budget` in `nw-cross-cutting-invariants`). Fault injection a granted derogation covers, including one the user elected to run in the default suite, is not a finding.

Detection: Measure every test in the diff with native per-test timing over at least nine sequential runs and judge it on the median, two or more runs over the budget on the same test also being a finding | Compare against the design's budgets and the feature's derogation ADR, if the design cites one | Check each formal law has a fast executable projection in the default suite.

Severity: CRITICAL when no granted derogation covers the overrun; reviewers never grant one.

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 nu