quality-review

What this skill does

# Quality Review Skill

## Overview

Stage 2 of two-stage review: Assess code quality, maintainability, and engineering best practices.

**Prerequisite:** Spec review must PASS before quality review.

> **MANDATORY:** Before accepting any rationalization for rubber-stamping code quality, consult `references/rationalization-refutation.md`. Every common excuse is catalogued with a counter-argument and the correct action.

## Triggers

Activate this skill when:
- Spec review has passed
- `/exarchos:review` command (after spec review)
- Ready to assess code quality
- Before synthesis/merge

## Execution Context

This skill runs in a SUBAGENT spawned by the orchestrator, not inline.

The orchestrator provides the state file path, diff output from `exarchos_orchestrate({ action: "review_diff" })`, task ID, and spec review results (must be PASS).

The subagent reads the state file for artifact paths, uses the diff output instead of full files, runs static analysis, performs a code walkthrough, generates a report, and returns the verdict.

### Data Handoff Protocol

The **orchestrator** is responsible for generating the diff before dispatching the quality-review subagent. The subagent does NOT generate its own diff.

**Orchestrator responsibilities:**
1. Generate diff: `git diff main...HEAD` or `git diff main...integration-branch`
2. Pass diff content in the subagent dispatch prompt
3. Include state file path for artifact resolution
4. Include spec review results (must be PASS)

**Subagent responsibilities:**
1. Receive diff content from dispatch prompt (do NOT re-generate)
2. Read state file for design/plan artifact paths
3. Run static analysis and security scripts against the working tree
4. Return structured JSON verdict

### Context-Efficient Input

Instead of reading full files, receive the integrated diff:

```bash
# Generate integrated diff for review (branch stack vs main)
git diff main...HEAD > /tmp/stack-diff.patch

# Alternative: git diff for integration branch
git diff main...integration-branch > /tmp/integration-diff.patch

# Alternative: use gh CLI to get PR diff
# gh pr diff <number>
# Or use GitHub MCP pull_request_read with method "get_diff" if available
```

This reduces context consumption by 80-90% while providing the complete picture.

### Pre-Review Schema Discovery

Before evaluating, query the review strategy runbook to determine the appropriate evaluation approach:

- **Evaluation strategy:** `exarchos_orchestrate({ action: "runbook", id: "review-strategy" })` to determine single-pass vs two-pass evaluation strategy based on diff size and task count.

### Review Scope: Combined Changes

After delegation completes, quality review examines:
- The **complete stack diff** (all task branches vs main), or the **feature/integration-branch** diff when using integration branches
- All changes across all tasks in one view
- The full picture of combined code quality

This enables catching:
- Cross-task SOLID violations
- Duplicate code across task boundaries
- Inconsistent patterns between tasks

## Review Scope

**Quality Review focuses on:**
- Code quality and readability
- SOLID principles
- Error handling
- Test quality
- Performance considerations
- Security basics

**Does NOT re-check:**
- Functional completeness (spec review)
- TDD compliance (spec review)

## Review Process

### Check Quality Signals

Before reviewing, query quality signals for the skill(s) under review:
```
mcp__plugin_exarchos_exarchos__exarchos_view({ action: "code_quality", workflowId: "<featureId>" })
```
- If `regressions` is non-empty, report active quality regressions to the user before proceeding
- If any hint has `confidenceLevel: 'actionable'`, present the `suggestedAction` to the user
- If `gatePassRate < 0.80` for the target skill, warn about degrading quality

### Step 0: Verify Spec Review Passed (MANDATORY)

Before proceeding, confirm spec review passed for all tasks:

```text
action: "get", featureId: "<id>", query: "reviews"
```

If ANY task has `specReview.status !== "pass"`, STOP and return:
```json
{ "verdict": "blocked", "summary": "Spec review not passed — run spec-review first" }
```

### Step 0.5: Verify Review Triage (Conditional — run when delegation phase preceded this review)

If this review follows a delegation phase, verify triage routing:

```typescript
exarchos_orchestrate({
  action: "verify_review_triage",
  featureId: "<feature-id>"
})
```

`featureId` resolves PRs (event-store projection) and `review.routed` events directly from the store — no `.state.json`/`.events.jsonl` needed (INV-1). A legacy file-based workflow may instead pass `stateFile` + `eventStream`.

`passed: true`: triage routing correct — continue to Step 1. `passed: false`: triage issues found — investigate and resolve before proceeding.

### Step 1: Static Analysis + Security + Extended Gates

> **Runbook:** Run quality evaluation gates via runbook:
> `exarchos_orchestrate({ action: "runbook", id: "quality-evaluation" })`
> If runbook unavailable, use `describe` to retrieve gate schemas: `exarchos_orchestrate({ action: "describe", actions: ["check_static_analysis", "check_security_scan", "check_convergence", "check_review_verdict"] })`

Run automated gates via orchestrate actions. See `references/gate-execution.md` for orchestrate action signatures and response handling.

1. `check_static_analysis` — lint, typecheck, quality-check (D2). **Must pass** before continuing.
2. `check_security_scan` — security pattern detection (D1). Include findings in report.
3. Optional D3-D5 gates: `check_context_economy`, `check_operational_resilience`, `check_workflow_determinism` — advisory, feed convergence view.

### Step 2: Test Desiderata Evaluation

Evaluate agent-generated tests against Kent Beck's Test Desiderata. Four properties are critical for agentic code:

| Property | What to check | Flag when |
|---|---|---|
| **Behavioral** | Tests assert on observable behavior, not implementation details | Mock call count assertions, internal state inspection, testing private methods |
| **Structure-insensitive** | Tests survive refactoring without behavioral change | Tests coupled to internal helper method signatures, tests that break when internals are renamed |
| **Deterministic** | Tests produce the same result every run | Uncontrolled `Date.now()`, `Math.random()`, `setTimeout` race conditions, network-dependent tests |
| **Specific** | Test failures pinpoint the cause | `toBeTruthy()` / `toBeDefined()` without additional specific assertions, catch-all tests with vague descriptions |

**Test layer mismatch detection:** Flag unit tests with >3 mocked dependencies as potential layer mismatches — unit tests with many mocks often indicate the test is asserting integration concerns rather than unit logic. Advisory finding: suggest re-classifying as integration test with real collaborators.

Include Test Desiderata findings in the quality review report under a "Test Quality" section. **Output format:** Report Test Desiderata violations as entries in the `issues` array with `category: "test-quality"`.

### Step 3: Generate Report

Use the template from `references/review-report-template.md` to structure the review output.

## Priority Levels

| Priority | Action | Examples |
|----------|--------|----------|
| **HIGH** | Must fix before merge | Security issues, data loss risks |
| **MEDIUM** | Should fix, may defer | SOLID violations, complexity |
| **LOW** | Nice to have | Style preferences, minor refactors |

### Priority Classification Rules

- **HIGH:** security vulnerabilities, data loss risk, API contract breaks, uncaught exception paths
- **MEDIUM:** SOLID violations (LSP, ISP), cyclomatic complexity >15, test coverage <70%
- **LOW:** naming, code style, comment clarity, non-impactful performance

If classification is ambiguous, default to MEDIUM and flag for human decision.

## Fix Loop for HIGH-Priority

If HIGH-priority issues found:

1. Create fix task listing each HIGH fi