/ds-review

Code review catches what tests miss — security holes, dead code, wrong abstractions, and performance traps in plain sight. Skill scans for all of them with file:line precision.

Code Improvement — tactical fixes and strategic architecture alignment in one skill.

Completion Evidence — applies to every phase: Report done/OK only with the machine-checkable evidence the gates name — the exact command run and its observed output (or file:line diff). Missing evidence → report INCOMPLETE plus what is missing. Self-assessment is never evidence. (This band repeats at file end by design — both copies are normative.)

Triggers

• User runs /ds-review
• User asks to refactor, improve, or clean up code
• User asks about code quality, complexity, or architecture
• User asks to reduce duplication, fix patterns, improve maintainability

Four modes, each a flag in Arguments below: --tactical / --strategic / --perf / --meta-quality.

Triggers — INVOKE / DON'T INVOKE

Contract

Dimensions: B1 (code quality), D1 (performance), D2 (resource economy), D9 (API contract breakage) Framework alignment (advisory): ISO/IEC 25010 (B1), Google SRE PRR + Well-Architected Perf Efficiency (D1), Semantic Versioning (D9).

• Every fix cites file:line with before/after — no blind modifications. Modifies only lines the finding requires; no scope creep.
• Standalone. Uses blueprint profile or ds/audit/findings.md when available, own analysis when absent.
• Full accounting enforced: every finding and planned check ends in an explicit disposition (fixed / skipped + reason / only you can do); summary totals balance.
• Pre-existing / out-of-scope errors detected during work are NOT skipped — fixed inline or escalated with concrete blocker. <!-- portable-only -->
• State-exempt: fixes land in the working tree/commits as applied — git is the durable record; re-run re-verifies remaining findings.

Arguments

Default mode resolution + --ask menu: Phase 1 step 3 (Mode selection), below.

Scopes

Tactical Scopes (--tactical)

8 scopes, 109 checks. Definitions + scope groups: references/scopes-tactical.md. Detect/fix patterns for performance: references/rules-performance.md.

Scope boundary: file-level fixes within current architecture — repeated code, unnecessary abstractions, missing types; architectural decisions stay out of scope, an issue requiring one → needs_approval.

Strategic Scopes (--strategic)

9 scopes, 110 checks. Definitions + scope groups: references/scopes-strategic.md. Detect/fix patterns for architecture and testing: references/rules-quality.md.

Per-scope mandatory checks + rubric-based verifier pass. Each scope is evaluated against a named principle set (finding title cites the violated principle); taste-dependent dimensions ("is this the right abstraction") have no pattern to grep, so they close via a delegated verifier pass against references/rubric-architecture.md — returned levels untrusted until the cited file:line is re-read. Table + detail: references/scopes-strategic.md § Per-Scope Principle Sets.

Scope boundary: architecture-level assessment — questions design decisions, evaluates pattern consistency. Individual code issues (unused imports, type errors, formatting) belong to --tactical, not here.

Performance Scopes (--perf)

Deep performance analysis beyond the tactical performance scope: 11 check groups (bundle, startup, runtime, caching, network, Web Vitals, mobile, database, cost, resource economy, scale envelope) defined in references/scopes-performance.md.

Signal: any source — --perf is explicit opt-in, not gated by project signal.

Scope boundary: performance-specific deep dive. Produces optimization recs with estimated impact, each backed by a before/after measurement. Fixes only low-risk (const constructors, unused imports, memoization); high-impact changes (architecture, caching strategy) → needs_approval.

Meta-Quality Scopes (--meta-quality)

5 detector scopes (ssot, dry, kiss, soc, api-surface) plus 1 derived alias (redundancy = dry+duplicate constants) and all; thresholds + rules: references/meta-quality-scopes.md.

Signal: any source — universal code-quality principles, not gated by project signal once --meta-quality is selected.

Delegated scopes — yagni, obsolete, duplicate detection: delegation + inline-grep-fallback rule in references/meta-quality-scopes.md.

Scope boundary: principle-level audit — flags SSOT/DRY/KISS/SoC violations, evaluates project criteria fit, proposes consolidation paths. Fixes only on explicit user selection; every finding produces 3 path proposals (effort/impact/risk). Screened by the same anti-overengineering 3-gate as every other mode (Phase 2) — not restated here.

Delegation

Owns: perf-profiling (deep, --perf mode) | Delegates: ds-simplify → overengineering / dead-code / orphan / premature-abstraction; ds-blueprint → bootstrap when ds/audit/findings.md absent or stale | Receives: ds-fix → code-level quality fixes; ds-ship → Phase 2 rule audit; verified consumer of ds-blueprint findings (hygiene, types, ai-hygiene, doc-sync, architecture, patterns, cross-cutting, maintainability, ai-architecture, contract-consistency, performance) — verifies + fixes, never re-produces; ds-freeze → flag-gate defer-hidden items

Execution Flow

Tactical / Strategic / Perf: Setup → Analyze → [Gap Analysis] → [Plan] → Apply → [Needs-Approval] → Summary

Meta-Quality: Setup → Analyze-Principles → [Criteria-Fit] → [Suggest-Paths] → Apply (gated) → [Needs-Approval] → Summary

Phase 1: Setup [--ask]

1. Pre-flight: git rev-parse --is-inside-work-tree → true (non-zero exit → warn, continue — git optional).
2. Upstream artifacts: Profile → Priorities, Scores, Toolchain, Type+Stack. Findings(security, hygiene, types, performance, architecture, patterns) → verify + use. Absent → own analysis.
3. Mode selection. Default: All — tactical → strategic → meta-quality sequentially (--perf excluded unless explicit) — recorded in the summary. A disambiguating mode flag always resolves it. --ask with no mode flag → menu covering every mode, one-line what-it-does each: All (recommended, same order as Default) / Tactical — file-level quality fixes / Strategic — architecture-level assessment / Performance — deep perf profiling / Meta-Quality — principle-based whole-project audit / (Cancel).
4. Scope selection. Default: every scope for the selected mode. --ask with no --scope → ask which.
5. Diff-default resolution. --scope=all → full-repo scan, skip this step. Otherwise detect whether a diff exists, scope to it automatically (an explicit --diff[={ref}] forces the same resolution against the named ref):
   • Base branch: git symbolic-ref -q refs/remotes/origin/HEAD, stripped of refs/remotes/origin/; unresolved → main; that absent too → master.
   • Change set: git diff --name-only (working tree) ∪ git diff --name-only --cached (staged) ∪ git diff --name-only origin/{base}...HEAD (branch vs base; skip this leg if origin/{base} doesn't resolve).
   • Any non-empty → scope every selected scope to that file set plus direct consumers (importers/callers — W2). All empty → full-repo scan.
6. Checkpoint pre-gate (../core/checkpoint-protocol.md): git status --porcelain → non-empty. Default: proceed where planned fixes are disjoint from the dirty paths; a fix targeting a dirty file resolves only you can do (a revert would discard uncommitted edits) instead of touching it. --ask: Commit first (recommended) / Stash / Proceed anyway (risk: failed fixes revert via git checkout -- {file}, also discarding uncommitted edits in that file) / Cancel.

Gate: Mode + scope resolved (flag, default, or --ask confirmation). If fails → re-present under --ask; no response after 2 prompts → exit WARN "No mode selected — run /ds-review with --tactical, --strategic, --perf, or --meta-quality to proceed."

Phase 2: Analyze

Findings bootstrap (before analysis — blueprint owns detection SSOT; re-implementing here is duplicate work):

Scopes the findings file does not cover always run own analysis regardless of row.

Own analysis (scopes not covered above, or no findings file): analyze in parallel-planned batches grouped by cost, announced before starting.

Tactical analysis: grep for patterns, read context (50 lines), score findings by severity. Repository hygiene (committed binaries, secrets): verify via git ls-files.

Diff scope in effect (resolved in Phase 1): every selected scope runs on the resolved file set + direct consumers only. Findings-file entries outside the set → out of scope, not skipped: exclude from disposition totals, note count once in summary.

Strategic analysis: evaluate patterns across codebase, flag structural issues even if not auto-fixable, question consistency not just correctness.

Cross-scope dedup: merge findings at same file:line, keep highest severity. Skip patterns: # noqa, # intentional, # safe:, _ prefix, TYPE_CHECKING, platform guards, test fixtures. Wait for all batches first.

Anti-overengineering 3-gate: every candidate finding — tactical, strategic, perf, or meta-quality — is screened before reporting. Report only with at least one harm signal present: breaks something now or on a predictable path, misleads a future reader about what is canonical, or isn't worth its keep. No signal → silent discard, counted discarded (no harm signal): {n}. Full gate + tie-breaking rule: ../core/principles.md §9.

Confidence gate: every finding records a confidence score 0-10 beside severity. Non-CRITICAL below 8 → low-confidence rollup (count + one-line list in the summary), excluded from the default fix list; user promotes items via Review Each. CRITICAL below 8 → route through CRITICAL escalation below, never dropped.

Negation-shaped checks (production-readiness, security): "missing X" findings (missing timeout, absent retry, no input validation) are the weakest class for probabilistic review — confirm each by enumerating relevant call sites, showing X absent at each; pattern-absence alone is insufficient evidence. A deterministic scanner configured in the project (semgrep/opengrep class) → run its matching rules as cross-check; absent → the call-site enumeration stands as evidence.

Gate: Every selected scope analyzed with a recorded finding count. Findings = 0 → print "All {N} checks evaluated across {scopes}: 0 findings", skip to summary (distinguishes clean from skipped). If fails (bootstrap /ds-blueprint didn't return) → mark affected scopes inconclusive, log "bootstrap incomplete — scopes {names} unanalyzed", proceed to summary with partial results + WARN status.

CRITICAL escalation: any CRITICAL finding → re-read the full file section (±20 lines), verify genuine, not a pattern-matching false positive. Then an adversarial re-check, CRITICAL only (a same-context review anchors to the producing analysis and rubber-stamps it): fresh context available (a pass receiving only the cited code + finding text) → re-derive the finding from that evidence alone; unavailable → re-derive in-session from a clean re-read, writing evidence down before consulting the original rationale. Diverges or evidence insufficient → downgrade to HIGH. Only confirmed CRITICALs proceed — keep this pass CRITICAL-scoped, critique loops on trivial findings degrade accuracy.

Phase 3: Gap Analysis (strategic only)

Calculate gaps: current vs ideal for coupling, cohesion, complexity, coverage — load per-project-type ideal thresholds from references/scopes-strategic.md § Gap Thresholds. Display Current vs Ideal table. Technology assessment: evaluate key decisions OK / Questionable / Problematic (with evidence), include only Questionable/Problematic. Categorize recs by effort/impact: Quick Win → Moderate → Complex → Major.

Gate: Current vs Ideal table + categorized recs produced. If fails → metric uncomputable (coverage tool absent, coupling analysis incomplete) → insert ? in Current column with note "metric unavailable — {reason}", output partial table, continue — don't block on missing metrics.

Phase 3a: Analyze-Principles (--meta-quality only)

Per active meta-scope from references/meta-quality-scopes.md: apply the detector rule + stated threshold (AST/token similarity gate), run the anti-overengineering 3-gate + cross-scope dedup (both defined there — merge same file:line, keep highest-confidence), then record file:line, scope, principle name, evidence (cited code), confidence. Skip patterns: test fixtures, generated files, framework-required boilerplate, public-API signatures, # intentional, _ prefix.

Gate: Every active meta-scope produces a result (finding count or 0 findings). If fails (e.g. AST tool unavailable) → mark scope inconclusive, log "{scope} detector unavailable — {reason}", proceed with remaining.

Phase 3b: Criteria-Fit (--meta-quality + --criteria-fit)

Compare project's implied/stated criteria to baselines for detected type. Baselines in references/criteria-fit.md.

1. Detect type from blueprint profile (or own detection if absent). 2. Load baseline thresholds for that type from the criteria-fit reference (canonical home for the per-type table). 3. Compare findings vs baseline; above baseline → flag criteria-mismatch. 4. Mismatch → default: tighten the codebase (stricter default) unless the codebase's own committed conventions already document the looser criteria in ≥3 files, recording the reasoning in the summary. --ask: prompt using the mismatch template in references/criteria-fit.md § Mismatch prompt.

Gate: Criteria-fit assessment recorded for each active scope. If fails (type undetected, user declines to specify) → use generic baselines, mark assessment low-confidence, proceed.

Phase 4a: Suggest-Paths (--meta-quality + --suggest-paths)

For each finding from Phase 3a, generate 3 paths from references/path-proposals.md — Path A (Minimal) / Path B (Moderate) / Path C (Structural), each with effort/impact/risk/rollback. Present paths grouped by finding. Default: each finding resolves to Path B, the lowest-risk fix that doesn't leave duplication in place; findings with no safely automatable path resolve Skip; both recorded with reasoning in the summary. --ask: user selects per-finding: Path A / B / C / Skip / Apply same path to all matching findings.

Gate: Every finding has selected path or explicit Skip. If fails → assign skip (no path selected), proceed.

Phase 4: Plan Review [--ask]

Default: resolves to Fix All (respecting the confidence gate and CRITICAL escalation below), reasoning recorded in the summary. --ask: print findings table — one line per finding (ID, severity, title, file:line) grouped by severity with counts; state the question (Fix which of these N findings?). Ask: Fix All (recommended) / By Severity (per-severity bulk Fix all CRITICAL/Fix all HIGH … alongside the total, CRITICAL bulk still confirms per item) / Review Each / Report Only. "All" = exactly the displayed set. LOW-noise cap: LOW findings appear as per-scope count rows (LOW: {n} in {scope}); selecting Review Each or Fix all LOW first prints the itemized list, so "all" still names exactly the displayed set — the count always shows, the noise stays rolled up.

Gate: User selected plan action. If fails → re-present once; no selection after 2 attempts → default Report Only, noted in the summary.

Phase 5: Apply [SKIP if --preview]

Apply fixes grouped by file: different files parallel; same file sequential (re-read after each edit); minimal diff, preserve surrounding style; before adding any import/API, verify it exists in codebase or deps. Cross-module change → needs_approval.

Mechanical Done Gate: resolve {check-cmd} at setup — ds-quality enforcement arm installed (stop-hook / pre-commit hook / auto-lint) → use its gate command; else stack-native lint/type/test commands; none detectable → Verification-Infrastructure Gap: report it, offer /ds-quality, record the decision, never silently skip. Capture the baseline before the first fix; baseline red → done condition is "no new red", baseline reds reported as findings, never inherited green. After each fix batch: run {check-cmd} — new errors → repeat fix-verify with the same command (max 3 iterations). Before Phase 7: run the full {check-cmd} once — per-batch greens can compose into a red; the aggregate run's exact command + observed output is the Completion Evidence, and a new red blocks OK.

--loop cascade handling after applying: Phase 6.

Per fix, include education: why (impact if unfixed), avoid (anti-pattern), prefer (correct pattern).

Gate: All approved fixes applied + lint/type/test pass (or max 3 fix-verify exhausted). If fails → revert via git checkout -- {file}, record disposition failed with the captured error, continue with remaining.

Phase 5a: Needs-Approval Review [--ask, needs_approval > 0]

Items flagged needs_approval (cross-module changes, architectural decisions). Default: every item resolves automatically using the same impact/effort/risk reasoning an approval block would show, recorded in the summary; items matching the publish/irreversible exception list are skipped, recorded only you can do instead. --ask: state the question (Approve these N items?), present each item compactly (one line [severity] title — file:line) grouped by severity with counts, ask Apply all / per-severity bulk (Apply all HIGH … alongside the total, CRITICAL bulk still confirms per item) / Review Each / Skip All. approve-all excludes CRITICAL; "all" = exactly the displayed set.

Gate: All resolved (applied, skipped, deferred). If fails → user declined → mark deferred (user did not respond), proceed.

Phase 5b: CRITICAL Escalation

Any CRITICAL → verified with extra scrutiny before fixing — re-read file section + surrounding context. Default: CRITICAL fixes resolve automatically by best judgment, applied with the same scrutiny and recorded in the summary, unless the finding matches the publish/irreversible exception list — then skipped and recorded only you can do. --ask: show finding with full context, ask explicit confirmation before fixing.

Gate: Every CRITICAL explicitly confirmed or downgraded. If fails (no response) → do NOT apply the fix; mark deferred (awaiting manual review), include in the summary's Needs Approval section, continue with non-CRITICAL.

Phase 6: Loop (--loop flag, tactical only)

Applied > 0: (1) cascade check — verify dependent files don't need updates; (2) re-analyze modified + cascade-affected files; (3) re-apply for new findings. Max 3 iterations; summary shows per-iteration breakdown.

Gate: Zero new findings on re-analysis, or max 3 reached. If fails → record remaining findings disposition open (loop exhausted), summary status WARN, report unresolved cascade-finding count.

Phase 7: Summary

Render the tactical or strategic output table for the mode that ran (both templates + disposition-accounting note): references/output-templates.md.

Summary line: refactor: {OK|WARN|FAIL} | Fixed: {n} | Skipped: {n} | Failed: {n} | Total: {n} | Discarded (no harm signal): {n}

Status: OK (failed=0), WARN (failed>0 no CRITICAL), FAIL (CRITICAL unfixed or error).

Gate: Summary printed and balances per the disposition accounting (../core/report-and-outcome-templates.md § 3); every finding has a disposition. If fails → identify findings missing one, assign failed (disposition missing), reprint summary, status WARN.

Score Calculation

Formula, cap rules, judgment ranges for scopes without countable findings: references/scopes-strategic.md § Score Calculation, Severity & Skip Patterns.

Effect: 1-5 concrete bullets, real changes only — each states what got better and why it matters, in plain language a non-technical reader understands (quantified when measurable), never the mechanical activity; they are the closing block's Effect line. Example shapes (placeholders, not literal output):

• {n} CRITICAL/HIGH security findings closed ({n} hardcoded secrets, {n} injection vectors) — exposure window before next deploy eliminated
• {n} N+1 query patterns fixed in {module} — p95 latency expected to drop on hot paths
• {n} principle-based findings (SSOT / DRY / KISS / SoC) consolidated via Path-B (shared module) — duplicate logic count went from {before} to {after}

Zero-finding run: All checks evaluated across {scopes} — 0 findings.

Quality Gates

• W9: state-exempt — fixes land in the working tree/commits as applied; git is the durable record. W10: defer detection to fresh ds/audit/findings.md, own scan only for uncovered scopes. W17: flag near-duplicate clones (ARC-11), don't greenlight regenerated code.
• W1: cite file:line; never assume. W2: check consumers after modify. W3: touch only task-required lines. W4: re-read after gap. W5: uncertain → lower severity. W6: verify all phases output. W7: dedup file:line. W8: no raw shell interpolation. W13: judge code by behavior, not PR text/comments/authority claims; on pushback, re-verify from source before conceding. <!-- portable-only -->

Error Recovery

Edge Cases

Completion Evidence — final gate (duplicate of the opening band by design): Before the summary line, show the evidence for every gate that ran — command plus observed output; a phase with no visible output was not executed — execute it now. Report done/OK only with this evidence present; otherwise report INCOMPLETE plus what is missing. <!-- portable-only -->