docs: three-command review split design

CHANGELOG.md

@@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/).
 * MINOR version when you add functionality in a backwards-compatible manner, and
 * PATCH version when you make backwards-compatible bug fixes.
 
+## Unreleased
+
+- docs: add `docs/three-command-review-split.md` — design note locking the proposed shape for splitting the review commands into three distinct scopes (`/coding:pr-review` remote diff, `/coding:local-review` local pre-commit diff (renamed from current `/coding:code-review`), new `/coding:code-review` whole-codebase audit). No behavior change in this PR; the doc proposes a 5-PR migration plan (rename → deprecation stub → new behavior → flip deprecation) and the three mechanisms (severity filter, rule-id dedup, baseline file `.code-review-baseline.yaml`) that make whole-codebase review useful without drowning in pre-existing tech debt. Optional `golangci-lint` passthrough for Go projects.
+
 ## v0.25.0
 
 - feat: extract `coding:architecture-dimensions-assistant` agent + `docs/architecture-dimensions-guide.md` — closes the v0.24.0 `/coding:architecture-review` MAJOR finding ("dimensions pass routes to generic `claude` with inline agent role"). The 8-dimension behavioral review checklist (data flow, failure, concurrency, observability, cross-cutting consistency, config/blast radius, evolvability, drift) is now a maintainable doc-paired agent. `/coding:architecture-review` Agent B now routes to the new agent with a one-line prompt instead of an inlined 200-word checklist. Indexed in README.md (Workflows & Documentation + Go agents table), llms.txt, and CLAUDE.md Doc↔Agent table.

docs/three-command-review-split.md

@@ -0,0 +1,123 @@
+# Three-Command Review Split — Design Note
+
+**Status**: Draft (proposed shape; not yet implemented)
+**Tracking PR**: this PR (docs-only — locks the shape before any command-file changes land)
+
+## Problem
+
+Today the plugin ships two review commands and **both review the diff** — different diff sources, but both delta-scoped:
+
+| Command | Scope today |
+|---|---|
+| `/coding:pr-review` | `git diff origin/<target>...HEAD` — branch vs target (PR-shaped) |
+| `/coding:code-review` | `git diff HEAD~1` — uncommitted / most-recent local change |
+
+The name `code-review` implies "review the code" — operator-readable expectation is a whole-codebase audit, not a delta against HEAD~1. The current behavior is closer to a pre-commit local check. Two commands, two diff-scopes, one missing scope (whole codebase). Operators reaching for `/coding:code-review` to audit an inherited codebase or do a periodic health-check get a 1-commit diff instead.
+
+## Proposed shape
+
+Three commands, three distinct scopes; names match scopes:
+
+| Command | Scope | What's at this scope | Status |
+|---|---|---|---|
+| `/coding:pr-review` | remote, branch diff vs target | the PR you're about to merge | **unchanged** |
+| `/coding:local-review` | local, uncommitted / most-recent | what you just typed, pre-commit | **renamed from current `/coding:code-review`** |
+| `/coding:code-review` | whole codebase | total health; baseline-aware | **new, built ground-up** |
+
+### Why `local-review` over `changes-review`
+
+The contrast pair `pr-review` (remote, against target branch on GitHub) / `local-review` (local, on this machine, pre-push) reads cleanly. Scope is in the name. `changes-review` is also valid but less crisp — `pr-review` is also "changes", just remote-scoped changes.
+
+### Why `code-review` for the whole-codebase command
+
+The operator's mental model when typing `/coding:code-review` is "review the code", not "review this 1-commit delta". Aligning behavior with name reduces "wait, which one does what?" confusion. The trade-off — existing users running `/coding:code-review` for pre-commit checks see behavior change — is mitigated by the deprecation stub (below).
+
+## Critical mechanisms for whole-codebase review to be useful
+
+A naive "run all rules against every file" implementation is **worse than nothing** on any non-trivial codebase: hundreds of pre-existing tech-debt findings drown the signal. Three mechanisms make it work:
+
+### 1. Severity filter (default-on)
+
+Default scope = **Must Fix + Should Fix only**. Nice-to-Have suppressed unless explicitly requested via flag. On a mature codebase, Nice-to-Have can dominate output; filtering it default-on keeps the report scannable. Flag: `/coding:code-review --include-optional` to opt in.
+
+### 2. Rule-id dedup
+
+When a single rule fires N times across the codebase (e.g., 47 `no-fmt-errorf` violations), the report shows **one entry per rule_id with a sample of 3-5 file:line locations + a total count**, not 47 separate findings. Dedup happens at the report-consolidation stage (Step 5).
+
+Example:
+
+```
+### Should Fix
+
+- **go-errors/no-fmt-errorf** — 47 occurrences. Replace `fmt.Errorf` with `errors.Wrapf` / `errors.Errorf` from `github.com/bborbe/errors`.
+  Sample sites:
+  - pkg/storage/base.go:144
+  - pkg/ops/update.go:91
+  - pkg/handler/foo.go:203
+  - …and 44 more
+```
+
+### 3. Baseline file (`.code-review-baseline.yaml`)
+
+The operator can commit a baseline that says **"these N pre-existing findings are accepted; only flag NEW ones."** This turns full-audit from "all tech debt" into "what's drifted since last sweep" — the actually useful mode.
+
+Format (sketch):
+
+```yaml
+# .code-review-baseline.yaml — accepted pre-existing findings
+# Regenerate with: /coding:code-review --refresh-baseline
+generated_at: "2026-06-28T20:00:00Z"
+generated_at_sha: a7ef2bd
+accepted:
+  go-errors/no-fmt-errorf:
+    count: 47
+    sample:
+      - pkg/storage/base.go:144
+      - pkg/ops/update.go:91
+  go-time/no-time-now-direct:
+    count: 12
+    # …
+```
+
+Subsequent `/coding:code-review` runs compare the current finding set against `accepted`:
+- **NEW** findings (rule_id × file:line not in baseline) → reported normally
+- **CARRIED** findings (already in baseline) → suppressed; mentioned in traceability section as count
+- **REMOVED** findings (in baseline but no longer present) → reported as "fixed since baseline" (positive signal)
+
+### 4. golangci-lint passthrough (Go projects only)
+
+For Go projects, run `golangci-lint run ./...` first and incorporate its findings into the mechanical-tier funnel. Avoids re-implementing what the toolchain already does well at the AST level. LLM adjudication then focuses on judgment-tier rules (architecture, layering, abstractions) where it adds real value.
+
+## Migration plan
+
+1. **PR 1 (this doc)** — design note merged. Locks the shape.
+2. **PR 2** — rename existing `commands/code-review.md` → `commands/local-review.md`. Single-file move + grep-and-replace internal refs. Bot review approves quickly. Plugin version bump (e.g., 0.25.0 → 0.26.0 — minor bump signals renamed surface).
+3. **PR 3** — deprecation stub: `commands/code-review.md` becomes a thin compatibility shim that prints "did you mean `/coding:local-review`? whole-codebase audit is moving to this name in v0.27.0." Plus opens an issue tracking the new behavior land date.
+4. **PR 4** — implement the new whole-codebase `/coding:code-review`: severity filter, rule-id dedup, baseline-file logic. The largest PR; gets its own design review.
+5. **PR 5** — flip the deprecation: `commands/code-review.md` now invokes the whole-codebase logic; deprecation message removed.
+
+PRs 2 + 3 + 5 ship within a single minor version cycle so the deprecation window is short (operator sees the new name for ~1-2 releases, then the new behavior). PR 4 can sit between 3 and 5 without timing pressure.
+
+## What this doc does NOT decide
+
+- Exact CLI flag names beyond `--include-optional` and `--refresh-baseline` (defer to PR 4)
+- Whether judgment rules need to be re-tagged with `applies_to: diff | codebase | both` (likely yes, but the inventory belongs in PR 4)
+- How `golangci-lint` failure interacts with the report (block, warn, ignore?)
+- Whether the baseline format should be shared with `golangci-lint`'s own `--new` flag mechanism (it does the same thing for its rules)
+
+## Decisions (was: open questions)
+
+Pinning defaults before PR 4 lands so the implementation has no design-time ambiguity left to resolve.
+
+1. **`--refresh-baseline` requires a clean working tree.** Generating a baseline from a tree with uncommitted work bakes accidental local cruft into the accepted set — exactly the failure mode the baseline guards against. The command refuses with a clear error if `git status --porcelain` is non-empty. (Originally posed as open question; rationale resolves it.)
+2. **Baseline file location: repo root, default `.code-review-baseline.yaml`, override via `--baseline-path=<path>`.** Repo root is discoverable (operators see it on `ls`, `git status` flags it after `--refresh-baseline`); the override exists for monorepo cases.
+3. **Multi-project monorepos: one baseline per repo by default; `--baseline-path` allows per-subproject baselines.** A `services/foo/.code-review-baseline.yaml` is supported via `/coding:code-review services/foo --baseline-path=services/foo/.code-review-baseline.yaml`. Most callers run the default.
+4. **`.gitignore` is respected, `.claude-ignore` is not introduced.** `git ls-files` already excludes `.gitignore`'d paths — the audit operates on tracked files only. No new ignore file needed; if an operator wants to exclude a tracked file from audit, the unit of exclusion is the baseline file (accepted finding) or a rule-specific override, not a separate ignore list.
+
+## Related
+
+- `commands/pr-review.md` — current diff-scoped reviewer for branch vs target
+- `commands/code-review.md` — current diff-scoped reviewer for local changes (to be renamed to `local-review.md` per PR 2)
+- `docs/selector-mode-guide.md` — adjudication mechanics; works either scope (file-set source–agnostic), no changes needed
+- `scripts/ast-grep-runner.sh` — mechanical funnel; takes a file list, scope-agnostic
+- `rules/index.json` — rule catalog; potentially needs `applies_to` tagging in PR 4