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 carry a witness — a failing test you wrote > and ran, a concrete repro (input → observed → expected), or an > execution trace — or a citation that resolves: a spec clause, > constitution rule, or requirement that actually exists. Citations are > checked mechanically against the cycle's spec/plan and the > constitution; one that resolves nowhere counts as no citation. A > finding with neither witness nor resolving citation is 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. Spend your effort on witnesses and > resolvable citations, not on quantity. Findings are ONE fenced ```json block — an array of objects (prose outside it is not parsed): ```json [{ "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 ledger entries (`FIXED | NOT-FIXED | DISPUTED-STANDS`, evidence required; test witnesses run at current HEAD). Scope follows `review_loop.phase_a_scope`: `all` (default) verifies every open and not-fixed entry; `fixed` verifies only entries a fix commit claimed via `Sage-Fix` trailers since the last round, plus one full pass on the stopping round — the default stays `all` pending measurement (E17 guards ledger amnesia). A `DISPUTED-STANDS` verdict — or a fixer's `--cannot-reproduce` — does not clear an entry: it becomes a Phase-A dispute that must receive a disposition (defer / reject / fix-now) before any STOP records; it never vanishes from the verdict. 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."