xoai/quality-review

<!-- sage-metadata cost-tier: sonnet activation: mandatory tags: [review, quality, security, performance, maintainability] inputs: [implementation, codebase-context, constitution] outputs: [review-result] -->

Quality Review

Evaluate code craftsmanship — is it clean, secure, maintainable, and performant?

Core Principle: Spec compliance (Gate 1) verifies you built the right thing. Quality review (Gate 3) verifies you built it well. Both are required.

When to Use

After spec review passes (Gate 1) and constitution compliance passes (Gate 2), as Gate 3 in the quality pipeline.

Process

Sub-Agent Delegation (REQUIRED when Task tool available)

Gate 3 REQUIRES sub-agent delegation when Task tool is available. Self-review is the fallback when Task tool is NOT available, not a choice the agent makes.

Step 1: Check Task tool availability.

Step 2 — Task tool available AND independent_gate3 ≠ false:

Announce: "⚡ Running code quality review (sub-agent)..."

Spawn a sub-agent with the following prompt:

You are a code reviewer. You were NOT involved in writing this code.
Review it for quality, security, and maintainability. Be specific.

CRITICAL: You are READ-ONLY. Do NOT modify any files. Do NOT use
Edit or Write tools. Your job is to REPORT findings, not fix them.

INPUTS:
- Changed files: {FILE_LIST}
- Project conventions: {CONVENTIONS_FILE or "none detected"}
- Stack: {DETECTED_STACK or "unknown"}

REVIEW THESE 5 DIMENSIONS:

1. READABILITY: Are names descriptive? Is flow obvious? Are complex
   sections commented with WHY? Is there unnecessary complexity?

2. ERROR HANDLING: Are errors handled, not swallowed? Do error
   messages help diagnose? Are failure paths tested? Are external
   calls protected?

3. SECURITY: Are inputs validated? Is auth checked? Are secrets
   hardcoded? Is user data logged? Are queries parameterized?
   Security issues are ALWAYS critical.

4. PERFORMANCE: Are there N+1 patterns? Unnecessary allocations?
   Large datasets loaded into memory? Only flag OBVIOUS issues —
   no speculative optimization.

5. CONVENTIONS: Does the code match existing project patterns?
   Naming, file structure, style? Is it internally consistent?

CLASSIFY each finding:
- CRITICAL: Security vulnerability or will break in production.
  Must fix. Security issues are ALWAYS critical.
- WARNING: Quality issue. Should fix before shipping.
- SUGGESTION-substantive: Optional improvement. Affects readability,
  maintainability, or future behavior. Can defer.
- SUGGESTION-cosmetic: Style/naming/formatting with equally valid
  alternatives. No behavior change.

FORMAT (strict):
GATE: code-quality
RESULT: PASS | FAIL
CRITICAL: [list with file:line or "None"]
WARNING: [list with file:line or "None"]
SUGGESTION-substantive: [list with file:line or "None"]
SUGGESTION-cosmetic: [list with file:line or "None"]

Be concise. Every finding names a specific file and line.
No generic praise. No vague observations. Just findings.
Security issues found = ALWAYS FAIL.

Present the sub-agent's findings as the Gate 3 result. Do NOT filter, downgrade, or dismiss findings.

Step 3 — Task tool NOT available OR independent_gate3 is false:

Self-review fallback. This is a degraded Gate 3 — make it loud (R29):

• Announce: Sage: independent Gate 3 skipped — Task tool unavailable on this platform. Self-review only; quality chain is degraded. For independent review, run /review.
• When the cause is the Task tool being unavailable (not a config opt-out), append one line to the initiative's decisions.md: [<date>] independent Gate 3 skipped (Task tool unavailable) — code quality self-reviewed only.

Do NOT self-review when Task tool IS available and config allows sub-agent. That defeats the purpose of independent review.

Proceed with self-review using the 5 dimensions below.

Dimension 1: Readability

• Is the code clear to someone unfamiliar with it?
• Are names descriptive and consistent with project conventions?
• Is the logic flow easy to follow? Are complex sections commented?
• Is there unnecessary complexity that could be simplified?

Dimension 2: Error Handling

• Are errors handled, not swallowed? No empty catch blocks.
• Do error messages help diagnose the problem?
• Are failure paths tested?
• Are external call failures handled (network, database, file system)?

Dimension 3: Security

• Are inputs validated and sanitized?
• Is authentication/authorization checked where needed?
• Are secrets hardcoded? (Should use environment variables or secret management)
• Is user data logged inappropriately?
• Are SQL queries parameterized? (No string concatenation)
• Are dependencies from trusted sources with known versions?

Dimension 4: Performance

• Are there obvious N+1 query patterns?
• Are there unnecessary allocations in hot paths?
• Are large datasets loaded into memory when streaming would work?
• Are there missing indexes for frequent queries?
• Only flag OBVIOUS issues — don't micro-optimize speculatively.

Dimension 5: Conventions

• Does the code follow the patterns established in the codebase? (from codebase-scan)
• Is it consistent with project naming, file structure, and style?
• Does it follow the constitution's mandated patterns?

Output

GATE: code-quality
RESULT: PASS | FAIL

FINDINGS:
  Readability: [PASS | issues found]
  Error Handling: [PASS | issues found]
  Security: [PASS | issues found — security issues are always FAIL]
  Performance: [PASS | issues found]
  Conventions: [PASS | issues found]

SEVERITY:
  Critical: [list — these cause FAIL]
  Warning: [list — these are noted but don't block]
  Suggestion: [list — optional improvements]

ACTION: none | fix-and-retry | escalate-to-human

Rules

• Security issues are ALWAYS critical — they cause FAIL regardless of severity assessment.
• Performance opinions must be evidence-based. "This might be slow" is not a finding. "This loads all records into memory for a table that could have millions of rows" is a finding.
• Convention deviations are critical only if they break consistency in a meaningful way. A different variable name style is critical. A slightly different comment format is a suggestion.
• Don't nitpick style when the project has no established style guide. Pick battles.
• Do NOT suggest rewrites. Flag specific issues with specific locations.

Rationalization table

Derived from the RED baseline in TESTS.md. When the Task tool is available, the sub-agent is REQUIRED — the marker ⚡ Running code quality review (sub-agent)... MUST appear; self-review is the fallback, not a choice.

| The excuse (observed) | Why it's wrong | The rule | |---|---|---| | "I can review my own code." | Self-review shares the author's blind spots; Gate 3 exists for an independent pass. | The sub-agent is required when the Task tool is available — self-review is the fallback. | | "It's a small diff, a sub-agent is overkill." | Security and convention breaks hide in small diffs as readily as large ones. | Size is not the condition; Task-tool availability is. | | "Gate 1 already passed." | Gate 1 verifies the right thing was built; Gate 3 verifies it was built well. | Both are required; spec compliance is not quality. | | "Self-review is faster." | Speed bought by dropping independence is exactly the cost Gate 3 exists to prevent. | Self-review only when the Task tool is unavailable or config disables it. |

Review Loop v2 (ledger mode)

Active by DEFAULT (an absent review_loop: block means mode: v2) (loop: orchestration/quality-locked; ledger: sage/runtime/tools/ review.py). When active, the sub-agent prompt's CLASSIFY + FORMAT block above is replaced by the contract below and the 5 dimensions by the perspective passes. With mode: v1 this section is inert.

Output contract (v2 — no verdict)

Include verbatim in the sub-agent prompt:

You do not decide the loop; you report findings. The decision is computed from them.

A critical or major must come with a witness: a failing test you wrote and ran, a concrete repro (input → observed → expected), or an execution trace. If you cannot demonstrate it, report it — it will be recorded as substantive. This is not a penalty; it is the definition of the severities.

An empty finding list is a valid, creditable outcome; you are scored on precision, not volume. Every critical/major must cite the spec clause, constitution rule, or requirement it violates — a finding that cites nothing is capped at substantive automatically, so spend your effort on citations and witnesses, not on quantity.

Findings are ONE fenced ```json block — an array of objects (prose outside it is not parsed):

[{
  "pass": "input-hostility | state-and-flow | security | regression-surface",
  "severity": "critical | major | substantive | cosmetic",
  "cited_rule": "spec §4.2 | constitution:api.3 | null",
  "anchor": {"file": "src/auth.ts", "region": [118, 141]},
  "claim": "one falsifiable sentence",
  "witness": {"kind": "test | repro | trace | none",
              "ref": "path or null", "status": "red | green | n/a"},
  "exit_criteria": "what specifically would make this finding pass"
}]

Where the platform grants sub-agent test execution (Tier-A attested), the reviewer RUNS its witness before reporting status: red.

Perspective passes (round 1, code)

Sequential checklist passes in one dispatch; tag each finding's pass:

1. input-hostility — boundaries, nulls/empties, malformed input, injection, encoding, size limits.
2. state-and-flow — ordering assumptions, concurrency, resource lifecycle (open/close, acquire/release), partial-failure states, error paths that skip cleanup.
3. security — secrets in code, authz on every entry point, unsafe APIs, unparameterized queries, sensitive data in logs. Security findings cite the rule they violate like any other — the severity rubric is the same.
4. regression-surface — the packet's blast-radius neighbors: callers/callees/shared state of changed symbols with no covering test.

Two-phase (rounds >1)

Phase A: verify each open/not-fixed ledger entry (FIXED | NOT-FIXED | DISPUTED-STANDS, evidence required; test witnesses run at current HEAD). Phase B: hunt the revision delta plus Phase-A anchors only — the whole-artifact pass happened at round 1.

Input packet (v2)

Assembled by the dispatching workflow, in order: (1) changed files (delta on rounds >1); (2) deterministic gate outputs verbatim; (3) test output + per-file coverage for touched files; (4) sage-ontology blast radius for changed symbols — when absent, the packet says so (loud degradation); (5) spec/plan excerpts the diff claims to implement; (6) the ledger (open + settled); (7) mutation-survivor report if present.

Failure Modes

• Code is correct but ugly: PASS with suggestions. Correctness > aesthetics.
• Security vulnerability found: ALWAYS FAIL. Even for internal tools. Security is not optional.
• Performance concern is speculative: Note as suggestion, not finding. Don't block on "might be slow."