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
Feature Envy	Method uses another object's data more than its own
Inappropriate Intimacy	Classes accessing each other's internals
Data Class	Only fields + getters/setters, no behavior
Message Chain	a.b().c().d() chains > 2 levels
Divergent Change	One class modified for unrelated reasons
Shotgun Surgery	One change requires edits in 5+ files

L4: Abstractions (Severity: HIGH)

Smell	Detection Pattern
Long Parameter List	>= 4 parameters
Data Clumps	Same parameter group in 3+ method signatures
Primitive Obsession	Raw strings/ints for domain concepts
Middle Man	Class only delegates, no own logic

L5-L6: Advanced (report only when L1-L4 clean)

L5: Switch statements -> polymorphism. L6: SOLID violations (Refused Bequest, Parallel Inheritance).

---

Dimension 5: Priority Validation

Validate that the roadmap addresses the LARGEST bottleneck first.

Questions

Q1: Is this the largest bottleneck? Does timing data show this is the PRIMARY problem? Is there a larger problem being ignored?

Q2: Were simpler alternatives considered? Does roadmap include rejected alternatives? Are rejection reasons evidence-based?

Q3: Is constraint prioritization correct? Are user-mentioned constraints quantified by impact? Is a minority constraint dominating the solution?

Q4: Is architecture data-justified? Is the key architectural decision supported by quantitative data?

Failure Conditions

• FAIL if Q1 = NO (wrong problem being addressed)
• FAIL if Q2 = MISSING (no alternatives considered)
• FAIL if Q3 = INVERTED (minority constraint dominating)
• FAIL if Q4 = NO_DATA and this is performance optimization

---

Review Output Format

Every review returns YAML in this format:

review_id: "code_rev_{YYYYMMDD_HHMMSS}"
reviewer: "software-crafter (review mode)"
artifact: "{file paths reviewed}"
iteration: {1 or 2}

strengths:
  - "{Specific positive observation with file:line reference}"

issues_identified:
  implementation_bias:
    - issue: "{Specific pattern detected}"
      severity: "critical|high|medium|low"
      location: "{file:line-range}"
      recommendation: "{Actionable fix}"

  test_quality:
    - issue: "{Test coupling or quality issue}"
      severity: "critical|high"
      location: "{test_file:line}"
      recommendation: "{Fix description}"

  testing_theater:
    - issue: "{Specific theater pattern: zero-assertion|tautological|mock-dominated|circular|always-green|fully-mocked-sut|implementation-mirroring|assertion-free|misleading-name|hardcoded-oracle}"
      severity: "blocker|high"
      location: "{test_file:line}"
      evidence: "{Why this test would still pass if production code were deleted or broken}"
      recommendation: "{Rewrite with behavioral assertion or delete}"

  test_modification:
    - issue: "{Signal: assertion-weakened|expectations-reduced|test-deleted|test-skipped|deferred-fix-comment|assertion-count-decreased}"
      severity: "blocker"
      location: "{test_file:line}"
      before: "{Original assertion/test from RED phase}"
      after: "{Modified assertion/test from GREEN phase}"
      recommendation: "Revert test to RED-phase version, fix implementation"

  escalation_verification:
    escalation_marker_present: true|false
    implementation_attempts: {count}
    po_approval_referenced: true|false|not_applicable
    status: "PASS|BLOCKER"

  completeness:
    - issue: "{Missing coverage}"
      severity: "critical"
      location: "Missing test for AC-{number}"
      recommendation: "{Add test: GIVEN... WHEN... THEN...}"

  code_readability:
    - issue: "{Readability issue}"
      severity: "low"
      location: "{file:line}"
      recommendation: "{Extract methods: {names}}"

  rpp_smells:
    levels_scanned: "L1-L3"
    cascade_stopped_at: "L2"  # null if all clean
    findings:
      - level: "L2"
        smell: "Long Method"
        location: "{file:line}"
        evidence: "42 lines, 3 responsibilities"
        recommendation: "Extract Method: {names}"

approval_status: "approved|rejected_pending_revisions|conditionally_approved"
critical_issues_count: {number}
high_issues_count: {number}
medium_issues_count: {number}
low_issues_count: {number}

---

Severity Classification

Blocker (instant rejection, no discussion): Testing Theater patterns (zero-assertion, tautological, mock-dominated, circular, always-green, fully-mocked SUT, assertion-free) | Test modification to accommodate implementation (G9 violation).

Critical (must resolve before handoff): Test coupling to implementation (prevents refactoring) | Missing AC test coverage | Port-boundary violations in critical paths.

High (strongly recommend resolving): Shared mutable state in tests | Major over-engineering | Inadequate error scenario coverage.

Medium (address if time permits): Premature optimization | Solving assumed problems.

Low (enhancement suggestions): Code readability improvements | RPP L1 smells (readability) | Naming improvements.

RPP Level -> Severity Mapping:

• Low: L1 smells (readability -- dead code, how-comments, magic values, scope, lazy class, speculative generality)
• Medium: L2 smells (complexity -- long method, duplicated code, complex conditionals)
• High: L3-L4 smells (responsibilities, abstractions)
• Critical: unchanged (Testing Theater, port violations, missing AC coverage)

---

Review Iteration Rules

• Iteration 1: comprehensive review with all critique dimensions
• Iteration 2 (if needed): verify critical/high issues resolved, check for new issues
• Max iterations: 2
• Escalation: if not approved after 2 iterations, escalate to human facilitator