feat(shell-projection): add exec* dispatch and platform* file I/O seam (#3465) (#3470)

* feat(shell-projection): add exec* dispatch and platform* file I/O seam (Phase 1, #3465) Extends shell-command-projection.cjs with two new sections: Subprocess dispatch: - execGit(args, opts) → { exitCode, stdout, stderr } - execNpm(args, opts) → same; owns shell:true on Windows (npm.cmd) - execTool(program, args, opts) → same; ENOENT → exitCode 127, no throw - probeTty(opts) → string | null; returns null on Windows and non-tty Platform file I/O: - normalizeContent(filePath, content) → { content, encoding }; pure function; .md → full normalizeMd pass; other → CRLF→LF + trailing newline - platformWriteSync(filePath, content, opts) → ensureDir + normalizeContent + atomic write - platformReadSync(filePath, opts) → null on ENOENT; throws when required:true - platformEnsureDir(dirPath) → mkdirSync recursive, idempotent Absorbs normalizeMd fence-tracking logic from core.cjs (no circular dep). Existing rendering exports untouched. core.cjs compat exports unchanged until Phase 4. Updates CONTEXT.md with Shell Command Projection Module canonical definition. 31 new behavioral tests; full suite green. Closes #3465 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(context): record shell-projection expansion session learnings * chore(changeset): add entry for shell-projection I/O seam (#3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(adr): add ADR-0010 skill-surface budget module and ADR-0011 review default reviewers (Phase 1, #3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(context): record phase 1 rebase and PR session learnings (#3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(shell-projection): drop substring match on stderr — assert typed shape only The lint-no-source-grep rule prohibits substring matching on stderr (or any test-output text). The exitCode === 127 assertion already proves the ENOENT path; the stderr content was implementation-detail of the OS. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(pr3470): address CodeRabbit findings and minimal-core drift * docs(adr0010): align profile interface with phase-1 scope * docs(adr): index newly added 0010/0011 ADR drafts in README enh-3271 invariant requires every file in docs/adr/ to be linked from the README. Adds entries for the three ADR drafts committed earlier in this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-13 17:12:53 -04:00
parent cc503ea725
commit ba625c0978
13 changed files with 1052 additions and 18 deletions
--- a/.changeset/shell-projection-io-seam.md
+++ b/.changeset/shell-projection-io-seam.md
@@ -0,0 +1,5 @@
+---
+type: Changed
+pr: 3470
+---
+Add subprocess dispatch (`execGit`, `execNpm`, `execTool`, `probeTty`) and platform file I/O seam (`platformWriteSync`, `platformReadSync`, `platformEnsureDir`, `normalizeContent`) to `shell-command-projection.cjs`. Single seam for all OS-facing I/O — phase 1 of cross-platform hardening. See #3465.
--- a/CONTEXT.md
+++ b/CONTEXT.md
@@ -613,3 +613,63 @@ After stripping prose @-refs, some command `<process>` blocks retained bolded "*
 `DEFECT.GENERATIVE-PRIORITY=these defect classes share a common root: parallel implementations diverge silently because no parity test enforces equality at the test layer`
 `DEFECT.GENERATIVE-FIX=for any new constant/array/parser shared between CJS and SDK (or between two workflow surfaces), the same commit MUST add a parity assertion that fails when the two diverge`
 `DEFECT.GENERATIVE-EXEMPLAR=tests/config-schema-sdk-parity.test.cjs (asserts SDK VALID_CONFIG_KEYS == CJS VALID_CONFIG_KEYS); tests/bug-3298-phase-dir-prefix-drift-in-workflows.test.cjs (asserts every workflow surface uses expected_phase_dir)`
+
+### Shell Command Projection Module
+Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (`execGit`, `execNpm`, `execTool`, `probeTty`), and platform file I/O (`platformWriteSync`, `platformReadSync`, `platformEnsureDir`). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. Lives in `get-shit-done/bin/lib/shell-command-projection.cjs`. See ADR-0009.
+
+## Session learnings
+
+### 2026-05-13 — Shell Command Projection Module expansion (issues #3465–#3468)
+
+- scope: `shell-command-projection.cjs` extended to own subprocess dispatch + platform file I/O
+- ADR-0009 "does not execute" constraint superseded; ADR-0010 File Operation Engine superseded
+- new exports: `execGit`, `execNpm`, `execTool`, `probeTty`, `normalizeContent`, `platformWriteSync`, `platformReadSync`, `platformEnsureDir`
+- result shape invariant: all `exec*` return `{ exitCode, stdout, stderr }`; never throw on non-zero exit
+- platform policy owned at seam: `shell: process.platform === 'win32'` lives only in `execNpm`; `probeTty` returns `null` on Windows
+- normalization policy: `platformWriteSync` owns full `normalizeMd` for `.md`; CRLF→LF + trailing newline for all others; callers must not pre-call `normalizeMd`
+- `normalizeContent(filePath, content)` is the pure typed surface tests assert on — no file content read-back in tests (CONTRIBUTING.md rule)
+- `_normalizeMd` is re-implemented inline (not imported from `core.cjs`) to avoid circular dep
+- `atomicWriteFileSync`, `safeReadFile`, `normalizeMd` remain in `core.cjs` exports until Phase 4 (#3468)
+- phase gate: no call site migration until Phase 1 branch merged; Phase 2 (#3466) targets 6 subprocess files; Phase 3 (#3467) targets 15 fs files (215 call sites); Phase 4 (#3468) removes compat exports
+- branch: `feat/3465-shell-projection-platform-io-seam`
+- test file: `tests/shell-command-projection-dispatch.test.cjs` — 31 behavioral tests, node:test + node:assert/strict, no source-grep
+
+### 2026-05-13 — CodeRabbit guard + merge recovery (PR #3464)
+
+- scope: `gsd-build/get-shit-done` only; run all `gh` checks with `--repo gsd-build/get-shit-done`.
+- invariant: review completion requires all three gates:
+  - CI required checks green
+  - CodeRabbit green
+  - GraphQL unresolved review threads = `0`
+- failure mode: `mergeStateStatus=DIRTY` can exist even when CodeRabbit + thread count are clean.
+  - remediation: rebase/replay onto latest `origin/main` before treating PR as merge-ready.
+- failure mode: primary worktree rebase blocked by unrelated untracked files.
+  - remediation: use isolated worktree seeded from `origin/main`, replay feature commits there, then `push --force-with-lease` to PR head.
+- replay conflict learned seam:
+  - file: `get-shit-done/bin/lib/config.cjs`
+  - keep both behaviors during conflict resolution:
+    - existing `ship.pr_body_sections` and `workflow.human_verify_mode` validations
+    - new `review.default_reviewers` normalization path
+- post-rebase CI drift classes to verify immediately:
+  - SDK schema parity (`tests/config-schema-sdk-parity.test.cjs`)
+  - docs inventory counts (`tests/inventory-counts.test.cjs`)
+  - inventory manifest sync (`tests/inventory-manifest-sync.test.cjs`)
+  - slash namespace invariant (`tests/bug-2543-gsd-slash-namespace.test.cjs`)
+- concrete regressions fixed in this session:
+  - add `review.default_reviewers` to `sdk/src/query/config-schema.ts`
+  - add `review-reviewer-selection.cjs` row + count update in `docs/INVENTORY.md`
+  - regenerate `docs/INVENTORY-MANIFEST.json`
+  - replace legacy `/gsd-review` mention with canonical `/gsd:review` in source comments
+
+### 2026-05-13 — Phase 1 rebase + PR open (#3465)
+
+- branch: `feat/3465-shell-projection-platform-io-seam`; PR: #3470 on `gsd-build/get-shit-done`
+- rebase pattern: 4 sibling-branch commits (#3464) were skipped as "patch contents already upstream" — expected when a feature branch is cut before a sibling merges to main
+- untracked files block rebase: `git stash --include-untracked` before `git rebase origin/main`
+- add/add conflict resolution: when HEAD side is empty (feature didn't have the file) and ours has the content, `git checkout --theirs <file>` is correct
+- content conflict resolution: `git checkout --ours CONTEXT.md` + manual append of our new sections — preserves main's full content while adding our additions
+- `docs/research/` was untracked pre-rebase but came in from main during rebase — already tracked, no action required
+- force-push after rebase: `git push --force-with-lease origin <branch>` (not `--force`)
+- stash pop can fail if main brought in the same files: drop the stash with `git stash drop` when files are already present
+- PR hook requires reading all listed contribution files before `gh pr create` will execute — including `pull_request_template.md`, all typed templates, and all issue templates
+- changeset type for seam additions: `Changed` (not `Added`) — expands existing module, not a new standalone feature
--- a/bin/install.js
+++ b/bin/install.js
--- a/docs/adr/0010-skill-surface-budget-module.md
+++ b/docs/adr/0010-skill-surface-budget-module.md
@@ -0,0 +1,148 @@
+# Skill Surface Budget Module owns install-time skill listing curation
+
+- **Status:** Proposed
+- **Date:** 2026-05-12
+
+We propose extending the existing install profile seam (`get-shit-done/bin/lib/install-profiles.cjs`) into a **Skill Surface Budget Module** that owns which subset of GSD's 66 skills is written to the runtime config dirs, and that owns the per-skill `requires:` dependency manifest used to keep that subset closed under cross-skill references. GSD currently ships a binary `--minimal` / full toggle; runtimes that enumerate skills (Claude Code, OpenCode, etc.) cap the `<available_skills>` system-prompt block at `skillListingBudgetFraction` of the context window (default 1% = ~2k tokens at 200k), and GSD alone consumes ~60% of that cap (#3408). Further description shrinkage is unavailable — `scripts/lint-descriptions.cjs` already enforces a hard 100-char ceiling and the mean is 72.5 chars. The remaining lever is surfacing fewer skills, which requires a typed profile model plus a dependency manifest, not more ad-hoc allowlists.
+
+## Decision
+
+- Add a **Skill Surface Budget Module** by extending `get-shit-done/bin/lib/install-profiles.cjs` as the single owner for which `commands/gsd/*.md` and `agents/gsd-*.md` files are staged into the per-runtime copy pipeline.
+- Replace the single `MINIMAL_SKILL_ALLOWLIST` constant with a typed `PROFILES` map keyed by profile name. Each profile is a *base set* of skills; the module computes the **transitive closure** over each skill's declared `requires:` set before staging.
+- Add a `requires:` frontmatter field to every skill whose body references another GSD skill. The dependency graph in the research memo (`docs/research/2026-05-12-skill-surface-budget.md` §3.1) is the migration spec for this pass.
+- Extend `bin/install.js` argument parsing to accept `--profile=<name>` and `--profile=<name1>,<name2>` (composable). Preserve `--minimal` / `--core-only` as aliases for `--profile=core`. Default install (no flag) remains `full` for back-compat.
+- Persist the active profile to `~/.claude/skills/.gsd-profile` (and runtime-equivalent locations) so `gsd update` re-applies the same profile instead of expanding silently to full.
+- Add `scripts/lint-skill-deps.cjs` and wire it into the existing `npm run lint:descriptions` pretest gate. The lint fails if:
+  - a skill body references another skill not in its `requires:` set, or
+  - any profile would ship a skill whose `requires:` closure is not satisfied.
+- Keep the **interactive install picker** behind the same `AskUserQuestion`-style flow already used for runtime/location selection. Non-interactive installs (CI, `npx --yes`) fall back to `--profile=full` unless overridden.
+
+## Initial Scope
+
+First migration slice should land the profile model and one new tier above `core`:
+
+1. Profile map (typed): `core` (current minimal, 7 skills including `phase`), `standard` (~13 skills covering the audit + main-loop + utility floor), `full` (current default, 66 skills).
+2. `requires:` frontmatter added to the **hot nodes** of the dependency graph first: `phase` (38 callers), `review` (11), `config` (7), `progress` (5), `update` (5). These are the skills whose absence silently breaks others, so they need explicit `required_by` audit before any profile narrows them out.
+3. Confirm-and-lock the latent bug fix surfaced by the audit: **`phase` is referenced by 38 skills and now belongs in `MINIMAL_SKILL_ALLOWLIST` / `PROFILES.core`.** Keep explicit coverage in minimal/core tests so this cannot regress.
+4. CLI surface: `--profile=`, comma-composed profiles, `--profile=help` listing each profile's contents and token cost.
+5. Profile marker persistence + `gsd update` re-application.
+
+It should **not** in the first pass:
+
+- Build a runtime enable/disable surface (`/gsd:surface`). Track as a follow-up ADR (see "Open questions").
+- Split GSD into multiple npm packages. The packaging-level alternative was considered and rejected — see research memo §4 Option F.
+- Consolidate further skills (e.g. collapsing `*-phase` into a dispatcher). Track separately as IA cleanup; orthogonal to surface curation.
+
+## Migration Inventory
+
+### `get-shit-done/bin/lib/install-profiles.cjs`
+
+- Replace `MINIMAL_SKILL_ALLOWLIST` Object.freeze constant with `PROFILES` Object.freeze map of profile-name → base skill set.
+- Replace `isMinimalMode(mode)` with `resolveProfile(mode)` returning a typed `{name, skills: Set, agents: Set}` after transitive-closure computation.
+- Replace `shouldInstallSkill(name, mode)` with `shouldInstallSkill(name, resolvedProfile)`.
+- Replace `stageSkillsForMode(srcDir, mode)` with `stageSkillsForProfile(srcDir, resolvedProfile)`. Add a sibling `stageAgentsForProfile` since this module now owns agent staging too (current `--minimal` skips agents wholesale; tiered profiles need finer control).
+- Keep the existing exit-cleanup machinery (`STAGED_DIRS`, `ensureExitCleanup`) unchanged — the bug surface it covers is the same.
+
+### `bin/install.js`
+
+These call sites should migrate behind the Skill Surface Budget Module:
+
+- `--minimal` / `--core-only` flag parsing — `bin/install.js:123-124`
+- `_effectiveInstallMode` plumbing + `isMinimalMode()` checks — `bin/install.js:7634-8465` (passes through to per-runtime copy fns)
+- minimal-agent skip block — `bin/install.js:8167-8207` (becomes "skip agents not in profile")
+- runtime-specific copy entry points that consume `stageSkillsForMode` — 13 sites per the existing comment in `install-profiles.cjs`
+- usage help block — `bin/install.js:508` (add `--profile=` documentation)
+
+### Frontmatter changes
+
+- Add `requires:` field to every skill in `commands/gsd/*.md` whose body references another GSD skill. Audit data lists the full set (`docs/research/2026-05-12-skill-surface-budget.md` §3.1). Estimate: 25-30 files touched in Phase 1.
+- Field is optional. Absence = "no GSD-skill dependencies." `lint-skill-deps.cjs` enforces consistency, not presence.
+
+### New: `scripts/lint-skill-deps.cjs`
+
+- Walks `commands/gsd/*.md`, parses `requires:`, walks the body for `gsd:<name>` or `\b<stem>\b` references to other skills (same matching rules documented in `docs/research/2026-05-12-skill-surface-budget.md` §3.1).
+- Fails CI if `requires:` set ≠ actual references (modulo ignore-list for prose mentions that aren't actual dispatches).
+- Walks `PROFILES` from `install-profiles.cjs`, fails if any profile's transitive closure references a skill not in the profile.
+- Wires into `npm run lint:descriptions` (or as a sibling `lint:skill-deps`) and `pretest`.
+
+### Profile marker
+
+- New `~/.claude/skills/.gsd-profile` (and per-runtime equivalents under `.codex/`, `.cursor/`, etc. as enumerated in `install.js`) containing the active profile name.
+- Installer Migration Module (ADR-0008) gains a one-shot migration: if marker absent and skills dir matches `core` exactly, write `core`; otherwise write `full`. Migrations are idempotent per existing module contract.
+
+### Tests expected to move with the seam
+
+- `tests/install-profiles-*.test.cjs` (any existing) — extend to assert profile resolution, transitive closure, and `--profile=core,standard` composition.
+- New `tests/skill-surface-budget-*.test.cjs` covering:
+  - profile closure: a profile that lists `discuss-phase` must transitively include `phase` if `discuss-phase` requires it
+  - lint failures: a skill body that references an un-required skill makes `lint:skill-deps` fail
+  - marker persistence: `gsd install --profile=standard` followed by `gsd update` preserves `standard`
+  - minimal back-compat: `--minimal` resolves to `--profile=core` and emits the same file set as today (modulo the `phase`-inclusion bug fix)
+
+## Interface sketch
+
+The module should accept typed profile intent and return a typed resolved profile:
+
+```js
+// install-profiles.cjs (extended)
+resolveProfile({
+  modes: ['core' | 'standard' | 'full'],
+  skillsManifest: ManifestMap,   // parsed `requires:` graph
+})
+// → { name: 'standard', skills: Set<string>, agents: Set<string> }
+```
+
+Profile composition: `--profile=core,standard` resolves to `union(closure(core), closure(standard))`. `--profile=full` is the identity profile (every skill).
+
+```js
+stageSkillsForProfile(srcDir, resolvedProfile)  // returns staged dir path
+stageAgentsForProfile(srcAgentsDir, resolvedProfile)  // new
+```
+
+Profile marker IO is typed too, not stringly:
+
+```js
+readActiveProfile(runtimeConfigDir) // → 'core' | 'standard' | 'full' | null
+writeActiveProfile(runtimeConfigDir, profileName)
+```
+
+Per-skill frontmatter contract:
+
+```yaml
+---
+name: gsd:plan-phase
+description: ...
+requires: [phase, discuss-phase]   # GSD skills only; not Claude Code primitives
+---
+```
+
+`requires:` lists *GSD* skills (file stems). It does not include Claude Code built-ins (`Read`, `Bash`, etc.) — those continue to live in `allowed-tools:` per existing convention.
+
+## Consequences
+
+- The skill-set written by the installer becomes a typed first-class artifact, not a side effect of file copies + an allowlist constant. ADR-0008 (Installer Migration Module) gains a clean handle for safe profile migrations on upgrade.
+- `gsd update` stops silently re-expanding a `--minimal` install to full — a current foot-gun documented inline in `install-profiles.cjs` (its module-level comment recommends `gsd update` without `--minimal` to "expand to the full surface"; that path remains available, but the default `gsd update` now respects the recorded profile).
+- The `requires:` manifest creates a new authoring obligation (~30 files in Phase 1), enforced by CI. Skill authors who add a `/gsd:phase` reference in a new skill body have to update `requires:`. The lint script keeps drift low-cost.
+- The `phase`-in-minimal latent gap (research memo §3.1) gets resolved as a side effect of adopting closure-based profile resolution — `phase` is auto-included whenever any minimal-loop skill `requires:` it.
+- First-time install UX gains a profile picker. The default remains `full` for non-interactive (`npx --yes`) installs, so back-compat for CI scripts is preserved.
+- The module becomes the canonical place to land future Anthropic platform features (lazy descriptions, per-plugin budgets, `.disabled` toggles — see Open Questions). It does not, in this ADR, *use* those features.
+- If accepted, `CONTEXT.md` should gain a canonical **Skill Surface Budget Module** entry alongside the existing seam entries, and future architecture reviews should treat ad-hoc `commands/gsd/` filtering outside this seam as drift.
+
+## Open questions
+
+- Whether the Phase-2 runtime `/gsd:surface` command (research memo §4 Option B) should be its own ADR or an amendment to this one. Leaning **separate ADR** because it introduces persistent runtime state outside the install pipeline.
+- Profile naming bikeshed. `core / standard / full` is the working proposal. Alternatives surveyed: `minimal / recommended / everything`, functional names (`planning, audit, research`). Settle in the implementation PR after a contributor poll.
+- Whether the `requires:` field should also be consumed by `/gsd:help` to render a "skills you have installed and what depends on what" graph. Likely yes, but out of scope for this ADR.
+- Whether to keep `phase` explicitly listed in `core` forever vs relying purely on closure semantics. Current recommendation: keep explicit listing because minimal mode has a back-compat allowlist path.
+- Whether telemetry (opt-in) is worth proposing to inform where the `standard` profile line goes. Without it, the cut points are author-intuition. Track separately; not a blocker.
+- Whether the Anthropic platform asks (research memo §6 — lazy descriptions, per-plugin budgets, dependency-aware listing, `.disabled` toggles) should be filed before or after this ADR ships. Recommendation: file as a feedback bundle when ADR is accepted, so we ship Phase 1 unilaterally and platform improvements compose on top.
+
+## References
+
+- Feature issue: `#3408`
+- Research input: `docs/research/2026-05-12-skill-surface-budget.md`
+- Existing seam being extended: `get-shit-done/bin/lib/install-profiles.cjs`
+- Description budget enforcement: `scripts/lint-descriptions.cjs`
+- Installer dispatch site: `bin/install.js:123-124`, `:8167-8207`
+- See `0008-installer-migration-module.md` (the migration that records the profile marker lives here)
+- See `0005-sdk-architecture-seam-map.md` (the seam map this module joins)
--- a/docs/adr/0011-review-default-reviewers-prd.md
+++ b/docs/adr/0011-review-default-reviewers-prd.md
@@ -0,0 +1,246 @@
+# PRD — `review.default_reviewers` config key for `/gsd-review` reviewer selection
+
+- **Status:** Draft
+- **Date:** 2026-05-13
+- **Issue:** `#3079`
+- **Related ADR:** `0011-review-default-reviewers.md`
+
+> This PRD is filed alongside its ADR under `docs/adr/` for co-location. The repo does not yet have a `docs/prd/` directory; if maintainers prefer one, this file can move there with the `0011-` prefix preserved.
+
+## TL;DR
+
+`/gsd-review` with no flags fans out to **every** detected CLI reviewer (Claude, Codex, Cursor, Gemini, OpenCode, plus local model servers such as ollama, lm-studio, llama.cpp). For users with many backends installed, this wastes wall-clock on timeouts and burns tokens on reviewers they don't want for routine work. Add a `review.default_reviewers` key under the existing `review.*` namespace in `.planning/config.json` that scopes the no-flag default to a user-chosen subset. Absent key preserves today's behavior. `--all` and individual flags continue to work unchanged. Follows GSD's **absent = enabled** convention.
+
+## Problem Statement
+
+GSD's `/gsd-review` workflow treats "no flags" as "invoke every CLI we can detect" (`workflows/review.md` line 52). That default is fine at install time — it makes the feature discoverable — but it's the wrong default for any user who has accumulated multiple reviewer CLIs plus local model servers. Each review probes up to ~10 backends, including ones that are slow, expensive, redundant for the change at hand, or not actually running (timeout waits on ollama, lm-studio, llama.cpp when the daemon is off).
+
+The only existing workaround is editing `workflows/review.md` in place. That patch gets clobbered on every `/gsd-update`, requiring `/gsd-update --reapply` to restore. There is no machine-readable record of the user's intent — every machine the user works on needs the same patch reapplied. The issue reporter (`#3079`) and presumably others are paying a "tax" on every review that is purely a default-selection problem.
+
+This is a small change with broad reach: it lands in a hot-path workflow that power users run many times per day.
+
+## Goals
+
+- Eliminate the recurring local-patch tax for multi-CLI users. A user who consistently wants only Gemini + Codex for routine reviews should be able to set that once and forget it.
+- Cut median wall-clock time of a no-flag `/gsd-review` on multi-CLI machines (target: ≥40% reduction for users with ≥4 detected CLIs).
+- Keep the change non-breaking. Absent config = today's behavior; nothing changes for the install-day experience.
+- Match GSD's established config conventions (`review.models.*`, `review.*_host`, **absent = enabled**, namespacing under `review.*`) so users don't have to learn a new pattern.
+- Stay one-line-shaped. The implementation should be a config read plus an intersection with the detected set — no new commands, no schema overhaul, no migration.
+
+## Non-Goals
+
+- **Per-phase or per-task reviewer routing** (e.g., "use Codex on Rust phases, Gemini on docs"). Useful, but a separate, larger design — track as a future ADR.
+- **Reviewer scoring, weighting, or ensemble logic.** This is about *which* reviewers run, not *how* their output is aggregated.
+- **Auto-detecting the "best" default reviewers based on usage history.** Out of scope; we want explicit user intent, not silent behavior drift.
+- **Changing the `--all` semantics or the individual flag set** (`--gemini`, `--codex`, `--cursor`, …). They keep their current meaning.
+- **A new top-level config namespace.** Reviewers belong under the existing `review.*` namespace.
+- **GUI / TUI editing of the key in v1.** Editing the JSON file directly (or via `/gsd-settings` / `/gsd-config --integrations` if maintainers choose to support it later) is sufficient.
+
+## Users & Use Cases
+
+### Primary persona: "Multi-CLI power user"
+
+A developer who has installed multiple coding CLIs (e.g., Claude Code, Codex, Gemini CLI, Cursor, OpenCode, Kilo) plus one or more local inference servers. They run `/gsd-review` frequently — sometimes dozens of times per day during a sprint — and have a stable mental model of which 1–3 reviewers actually add signal for their day-to-day work.
+
+### Secondary persona: "Single-CLI user with a sometimes-on local server"
+
+Has Claude + ollama installed. Wants reviews from Claude every time, and from ollama only when explicitly asked. Today, every `/gsd-review` pays the ollama timeout cost when ollama isn't running.
+
+### Secondary persona: "Cost-sensitive team lead"
+
+Routine reviews should hit cheap/local reviewers; pre-merge reviews should hit the expensive ones via `--all` or explicit flags. Wants a config-level expression of "the cheap subset is my default."
+
+### Use cases this enables
+
+- "I only want Gemini + Codex for routine reviews." → set `review.default_reviewers: ["gemini", "codex"]`.
+- "I want Claude only by default, and I'll opt into the others with flags." → set `["claude"]`.
+- "I want today's behavior." → leave the key absent.
+- "I want today's behavior just this once" on a configured project → `/gsd-review --all`.
+
+## User Stories
+
+Grouped by persona, ordered roughly by frequency.
+
+**Multi-CLI power user**
+
+- As a multi-CLI user, I want to declare which reviewers run by default so that `/gsd-review` doesn't probe backends I don't use.
+- As a multi-CLI user, I want my preference to survive `/gsd-update` so I don't have to keep re-patching `workflows/review.md`.
+- As a multi-CLI user, I want `--all` to still work so I can opt into a full review pre-merge without un-setting my config.
+- As a multi-CLI user, I want individual flags (`--gemini`, `--cursor`, …) to keep working regardless of my default so ad-hoc runs aren't constrained by the default.
+
+**Single-CLI user with sometimes-on local server**
+
+- As a user with intermittent local servers, I want my default to exclude them so a stopped daemon doesn't cost me a 30-second timeout on every review.
+
+**Cost-sensitive team lead**
+
+- As a team lead, I want to commit `.planning/config.json` to the repo so everyone on the team gets the same review defaults.
+
+**New user**
+
+- As a new user, I want today's behavior preserved so the feature still "just works" out of the box without config.
+
+**Edge cases**
+
+- As a user with a typo in my default list, I want a clear warning that names an unknown reviewer rather than a silent skip.
+- As a user whose configured reviewer is no longer installed, I want a clear note that it was dropped from this run.
+- As a user who lists only reviewers that aren't installed, I want a clear error or a documented fallback (see Open Questions).
+
+## Requirements
+
+### Must-Have (P0)
+
+- **P0-1. New config key.** `review.default_reviewers` is `string[]`. Each element validates against the existing slug pattern `^[a-zA-Z0-9_-]+$`. Schema parser accepts the key; rejects non-array or non-string-element values with a clear error. Empty array `[]` behavior is decided per Q-1.
+- **P0-2. No-flag honors the key.** Given the key is `["gemini", "codex"]` and both are detected, running `/gsd-review` invokes only Gemini and Codex.
+- **P0-3. Absent key preserves current behavior.** Given the key is unset, running `/gsd-review` runs every detected reviewer, identical to today.
+- **P0-4. `--all` overrides the config.** Given the key is `["gemini"]`, running `/gsd-review --all` invokes every detected reviewer. Verbose mode shows which reviewers came from `--all` vs. the default.
+- **P0-5. Individual flags override the config.** Given the key is `["gemini"]`, running `/gsd-review --cursor` invokes only Cursor. Running `/gsd-review --gemini --codex` invokes exactly those two regardless of the default.
+- **P0-6. Graceful slug handling.** Unknown slug → start-of-run warning naming the offending slug; run continues with valid entries. Known slug but undetected → info-level note; run continues. Zero post-filter selections → error per Q-1.
+- **P0-7. Docs updated.** `docs/CONFIGURATION.md` gets a `review.*` subsection (or an extension of an existing one) covering the new key. `workflows/review.md` references the key in the no-flag branch. Schema example at the top of `docs/CONFIGURATION.md` includes the key.
+- **P0-8. Tests.** Unit and integration coverage per the test list in the ADR's **Tests expected to move with the seam** section.
+
+### Nice-to-Have (P1)
+
+- **P1-1.** `/gsd-config --integrations` extends to set `review.default_reviewers` interactively, aligning with the existing interactive config flow for reviewers.
+- **P1-2.** `--no-default` flag that runs the full detected set without `--all` semantics — slightly different intent expression. Drop if equivalent to `--all` (Q-2).
+- **P1-3.** Verbose-mode "selection source" line in `/gsd-review` output: `Running reviewers (default): gemini, codex (set in .planning/config.json)`.
+- **P1-4.** Per-command override env var (`GSD_REVIEW_DEFAULT=...`) for CI scenarios where mutating `config.json` is undesirable.
+
+### Future Considerations (P2)
+
+- **P2-1.** Per-phase or per-file-type reviewer profiles (`review.profiles.frontend: ["claude", "cursor"]`). The shape of `default_reviewers` is deliberately chosen not to foreclose this — a future `review.profiles.*` map can coexist.
+- **P2-2.** Reviewer "groups" or aliases (`review.groups.cheap = ["ollama", "gemini-flash"]`). Same — leave room under `review.*`.
+- **P2-3.** Auto-suggestion that detects repeated flag patterns and offers to persist them. Natural follow-up but explicitly out of scope here.
+
+## Behavior Specification
+
+### Precedence (highest first)
+
+1. Individual reviewer flags (`--gemini`, `--codex`, `--cursor`, …) — always win.
+2. `--all` — full detected set, ignores config.
+3. `review.default_reviewers` in config — subset, intersected with detected set.
+4. No config, no flags — full detected set (today's behavior).
+
+This matches the principle of least surprise: explicit user input (flags) always wins over persisted preference (config), and persisted preference only fills the gap when the user hasn't said anything else.
+
+### Resolution pseudocode
+
+```text
+detected = detect_clis()                  # unchanged
+if any individual flag passed:
+    selected = flags_to_set(flags) ∩ detected
+elif --all:
+    selected = detected
+elif config.review.default_reviewers is set:
+    valid    = filter(config.review.default_reviewers, is_known_slug)
+    # warn on each invalid slug
+    selected = valid ∩ detected
+    # info on each valid-but-undetected slug
+    if selected is empty:
+        error with actionable message      # see Q-1
+else:
+    selected = detected                    # today's behavior
+```
+
+### Validation
+
+- Slug pattern: `^[a-zA-Z0-9_-]+$` (already in use for `review.models.<cli>`).
+- Type: JSON array of strings; anything else → schema error at config load.
+- Empty array: see Q-1 (proposed: schema error).
+- Slug case normalization: lowercase-on-read.
+- Duplicates: de-dup silently.
+
+### Logging
+
+- One line per `/gsd-review` start identifying the source of selection (default config / `--all` / explicit flags / no config). Surfaced under `--verbose` per Q-5.
+- Slug warnings/infos as described in P0-6.
+
+## Success Metrics
+
+### Leading indicators (1–4 weeks post-release)
+
+- **Adoption proxy.** Count of `.planning/config.json` files containing `review.default_reviewers` (only countable if/when GSD ever ships opt-in telemetry; otherwise qualitative via Discussions).
+- **Issue echo.** Closure of `#3079` and zero new issues reporting the same wipe-on-update problem within 60 days.
+- **Patch-removal proxy.** Maintainer observes no further PRs or Discussion threads about patching `workflows/review.md` defaults within 60 days.
+
+### Lagging indicators (1–3 months post-release)
+
+- **Median wall-clock per `/gsd-review`** on machines with ≥4 detected CLIs (self-reported or telemetry). Target: ≥40% reduction for users who opt in.
+- **User-perceived signal-to-noise** on review output (qualitative; gather via GitHub Discussions or a single follow-up question on the issue).
+
+### Measurement notes
+
+GSD doesn't ship usage telemetry today. Most of these metrics rely on qualitative signal: issue activity, Discussion threads, and a follow-up on `#3079`. That's appropriate for a config addition of this size — we don't need a metrics pipeline to validate it.
+
+## Edge Cases & Error Handling
+
+- **Config key missing** → today's behavior (all detected).
+- **Config key is `[]`** → schema error per proposed Q-1 resolution. Message: `review.default_reviewers is empty; remove the key to use the default-all behavior or list at least one reviewer.`
+- **Config key contains an unknown slug** → warn, drop the unknown entry, continue with the rest.
+- **Config key contains a known slug not detected on this host** → info, drop, continue.
+- **Config key contains only undetected slugs** → error with actionable message: `All configured default reviewers are missing on this host: [...]. Install at least one, or pass --all / specific flags.`
+- **Config key is malformed (e.g., string instead of array)** → schema error at config load, with file path and line number where the parser supports it.
+- **Slug case sensitivity** → lowercase-normalize on read; document this.
+- **Duplicates in the array** → de-dup silently.
+- **User passes `--all` and individual flags together** → existing behavior preserved; this change does not alter that interaction. Confirm in tests.
+
+## Open Questions
+
+- **Q-1. Empty-array semantics.** Should `review.default_reviewers: []` be a schema error, or should it fall back to "all detected"? *Proposal: schema error.* **Blocking.** Affects schema validation and tests.
+- **Q-2. `--no-default` flag.** Is this meaningfully different from `--all`? *Proposal: drop unless implementation surfaces a concrete difference.*
+- **Q-3. `/gsd-config --integrations` integration.** Land in this pass or as a fast follow? *Proposal: fast follow; depends on contributor bandwidth.*
+- **Q-4. Slug case handling.** Lowercase-on-read (proposed) or exact-match enforcement at the schema layer?
+- **Q-5. Verbose-mode "selection source" line.** Always-on or only under `--verbose`? *Proposal: `--verbose` only.*
+- **Q-6. Cross-runtime sanity.** Any cross-runtime concerns for Codex, OpenCode, Gemini CLI, or Kilo given the existing `resolve_model_ids: "omit"` pattern? *Proposal: none expected — the change operates on detection, not runtime; add at least one non-Claude integration test to confirm.*
+
+## Rollout Plan
+
+This is a small, additive, non-breaking change. No migration is required.
+
+1. **Implementation** (one PR) — schema addition, resolution logic, unit + integration tests per P0-8, docs update per P0-7.
+2. **Pre-release sanity** — dogfood on a multi-CLI setup; confirm `--all` and individual flags still behave.
+3. **Release** in the next minor version (no semver-major bump needed — additive).
+4. **Changelog & announcement** — call out in release notes; link to `#3079`; show the two-line config example.
+5. **Monitor** `#3079` and any new issues mentioning "default reviewers" or "review.md patch" for 60 days.
+6. **Optional fast follow** — P1-1 (`/gsd-config --integrations` integration) if there's contributor bandwidth.
+
+## Timeline Considerations
+
+- No hard deadlines. Quality-of-life fix, not contractual or compliance-driven.
+- No dependencies on other in-flight work.
+- Size: estimated ≤1 day of engineering for implementation + tests + docs.
+
+## Out-of-Scope (Restated)
+
+- No new commands.
+- No new top-level config namespaces.
+- No changes to `--all` or individual flag semantics.
+- No reviewer-output aggregation changes.
+- No per-phase reviewer profiles in v1 — the namespace is left open.
+- No GUI/TUI editing of the key in v1.
+
+## Appendix A: Example config
+
+```json
+{
+  "review": {
+    "default_reviewers": ["gemini", "codex"]
+  }
+}
+```
+
+With this config, `/gsd-review` invokes only Gemini and Codex. `/gsd-review --all` invokes every detected reviewer. `/gsd-review --cursor` invokes only Cursor.
+
+## Appendix B: Glossary
+
+- **Reviewer / CLI / backend.** Any code-review-capable CLI or model server GSD can invoke (Claude, Codex, Cursor, Gemini, OpenCode, Kilo, ollama, lm-studio, llama.cpp, …).
+- **Detected set.** The list of reviewers `detect_clis` finds on the current host at review time.
+- **Slug.** The lowercase short name of a reviewer used in flags and config (e.g., `gemini`, `codex`).
+- **"Absent = enabled" pattern.** GSD's convention that missing config keys default to a sensible enabled state. Here, missing `review.default_reviewers` means "all detected."
+
+## References
+
+- Feature issue: `#3079`
+- Configuration reference: `docs/CONFIGURATION.md` — `review.models.<cli>`, `review.*_host`, and the **absent = enabled** pattern
+- Workflow file owning the no-flag branch: `workflows/review.md` (line 52)
+- Companion ADR: `0011-review-default-reviewers.md`
--- a/docs/adr/0011-review-default-reviewers.md
+++ b/docs/adr/0011-review-default-reviewers.md
@@ -0,0 +1,147 @@
+# `review.default_reviewers` config key scopes the no-flag `/gsd-review` fan-out
+
+- **Status:** Proposed
+- **Date:** 2026-05-13
+
+We propose adding a `review.default_reviewers` key to `.planning/config.json` that scopes the no-flag default of `/gsd-review` to a user-chosen subset of detected CLI reviewers. Today the no-flag branch of `workflows/review.md` (line 52) invokes **all available** CLIs, which for multi-CLI users plus local model servers (ollama, lm-studio, llama.cpp) means probing up to ~10 backends per review, paying timeout costs on servers that aren't running and burning tokens on reviewers the user doesn't want for routine work (`#3079`). The only workaround today is patching `workflows/review.md` in place; that patch is wiped on every `/gsd-update` and requires `/gsd-update --reapply` to restore, with no machine-readable record of intent. The proposed key sits inside the existing `review.*` namespace (alongside `review.models.<cli>` and `review.*_host`), follows GSD's **absent = enabled** config philosophy, and is implementable as a one-line config read plus an intersection on the detected reviewer set.
+
+## Decision
+
+- Add **`review.default_reviewers`** to the `config.json` schema as `string[]`, validated against the existing CLI slug pattern `^[a-zA-Z0-9_-]+$` (the same pattern used for `review.models.<cli>` slugs).
+- When the key is **present**, the no-flag branch of `/gsd-review` invokes only the reviewers listed in the key, intersected with the host's `detect_clis` result.
+- When the key is **absent**, today's behavior is preserved: every detected reviewer runs. This matches the **absent = enabled** pattern documented in `docs/CONFIGURATION.md`.
+- **`--all`** continues to mean "every detected reviewer" and ignores the config key.
+- **Individual reviewer flags** (`--gemini`, `--codex`, `--cursor`, `--claude`, `--opencode`, …) continue to win over both config and `--all`.
+- Resolution lives in the `detect_clis` step of `workflows/review.md`: detect first, then filter by `review.default_reviewers` only on the no-flag branch.
+- Unknown slugs in the key emit a warning and are dropped; valid-but-undetected slugs emit an info-level note and are dropped; an all-undetected post-filter set emits an actionable error (see "Open questions" Q-1 for empty-array semantics).
+- Slug comparison is lowercase-normalized on read. The schema pattern already accepts mixed case, so normalization is forgiving without making the pattern itself stricter.
+- No new top-level config namespace, no new command, no new flag in v1.
+
+**Precedence (highest first):**
+
+1. Individual reviewer flags
+2. `--all`
+3. `review.default_reviewers`
+4. No config, no flags → today's behavior (all detected)
+
+## Initial Scope
+
+First slice should land the config plumbing and the no-flag branch behavior without expanding into adjacent reviewer-selection design:
+
+1. Schema addition for `review.default_reviewers` in the config loader; validation as `string[]` with slug pattern; lowercase normalization; clear schema errors for non-array / non-string-element values.
+2. Filter step inside `workflows/review.md` `detect_clis` no-flag branch:
+   - intersect `detected ∩ default_reviewers`
+   - emit a single-line "selection source" log identifying which path was taken (default config / `--all` / explicit flags / no config)
+   - warn on unknown slugs; info on valid-but-undetected slugs; error if the post-filter selection is empty
+3. Docs update: extend `docs/CONFIGURATION.md` with a `review.*` subsection documenting the key, allowed values, defaults, and override precedence; add the key to the schema block at the top of that file; update `workflows/review.md` to reference the key in the no-flag branch.
+4. Tests: config parsing (valid / empty / malformed); `detect_clis` intersection; integration coverage of no-flag honors config, `--all` overrides, individual flags override, unknown slug warns, only-undetected slugs errors.
+5. Release notes entry calling out the new key with the two-line config example.
+
+It should **not** in the first pass:
+
+- Add `--no-default` or any new CLI flag (track in "Open questions" Q-2 — likely equivalent to `--all`).
+- Add per-phase or per-file-type reviewer profiles (`review.profiles.*`). The namespace is left open for this as a future ADR (see "Open questions" Q-3).
+- Add reviewer "groups" or aliases (`review.groups.cheap = [...]`). Same — namespace deliberately left open.
+- Auto-suggest defaults from usage history. Different design philosophy (silent behavior drift); explicitly out of scope.
+- Extend `/gsd-config --integrations` to set the key interactively. Track as a fast follow (see "Open questions" Q-4).
+- Change `--all` semantics or the individual flag set.
+
+## Migration Inventory
+
+### `workflows/review.md`
+
+- `detect_clis` step, no-flag branch (current line 52: "No flags → include all available") — replace with: "No flags → if `review.default_reviewers` is set, intersect detected with the listed slugs; otherwise include all detected."
+- Verbose / debug output path — emit one line identifying the selection source.
+
+### Config loader
+
+- Add `review.default_reviewers: string[]` to the JSON schema for `.planning/config.json`. Pattern per element: `^[a-zA-Z0-9_-]+$`. Slug list de-dup on read; lowercase-normalize on read.
+- Surface schema errors at config load (file path + line number where the parser supports it), matching the existing handling for other malformed `review.*` keys.
+
+### `docs/CONFIGURATION.md`
+
+- Add `review.default_reviewers` to the **Full Schema** code block at the top of the file as an optional array key under a `"review": { ... }` object.
+- Add a new **Reviewer Selection** subsection (or extend the existing `review.*` section if one exists) covering: purpose, type, default, precedence vs. `--all` and individual flags, edge-case behavior (unknown slugs, undetected slugs, empty result).
+- Cross-reference `workflows/review.md` for how the key is consumed at review time.
+
+### Tests expected to move with the seam
+
+- New `tests/review-default-reviewers-config.test.cjs`:
+  - valid `["gemini", "codex"]` parses; lowercase normalization works
+  - `[]` schema decision per Q-1 (proposed: parse-time error)
+  - non-array → schema error
+  - non-string element → schema error
+  - element failing slug pattern → schema error
+- New `tests/review-default-reviewers-resolution.test.cjs`:
+  - no flags + key set, both slugs detected → exactly those reviewers invoked
+  - no flags + key set, one slug undetected → info logged, remaining slug invoked
+  - no flags + key set, all slugs undetected → actionable error, nothing invoked
+  - no flags + key set, unknown slug present → warning logged, unknowns dropped, rest invoked
+  - `--all` + key set → every detected reviewer invoked, key ignored
+  - `--gemini` + key set to `["codex"]` → only Gemini invoked
+  - no flags + key absent → every detected reviewer invoked (back-compat)
+- Extend existing `/gsd-review` integration tests to cover the new selection-source log line.
+
+### Schema doc cross-reference
+
+- Update the schema example at the top of `docs/CONFIGURATION.md` to include `"review": { "default_reviewers": ["gemini", "codex"] }` so the key is discoverable from the canonical schema view.
+
+## Example config
+
+```json
+{
+  "review": {
+    "default_reviewers": ["gemini", "codex"]
+  }
+}
+```
+
+With this set, `/gsd-review` (no flags) invokes only Gemini and Codex. `/gsd-review --all` invokes every detected reviewer. `/gsd-review --cursor` invokes only Cursor. Today's behavior is preserved by simply omitting the key.
+
+## Resolution pseudocode
+
+```text
+detected = detect_clis()                  # unchanged
+if any individual flag passed:
+    selected = flags_to_set(flags) ∩ detected
+elif --all:
+    selected = detected
+elif config.review.default_reviewers is set:
+    valid    = filter(config.review.default_reviewers, is_known_slug)
+    # warn on each invalid slug
+    selected = valid ∩ detected
+    # info on each valid-but-undetected slug
+    if selected is empty:
+        error with actionable message      # see Q-1
+else:
+    selected = detected                    # today's behavior
+log_selection_source(selected, source)
+```
+
+## Consequences
+
+- Multi-CLI users can stop patching `workflows/review.md`; the patch class that `/gsd-update` wipes goes away for this case.
+- Teams can commit `.planning/config.json` and share a default reviewer set across machines and contributors, without forking the workflow file.
+- `/gsd-review` wall-clock time drops on machines where detection probes idle local model servers — the timeout cost on stopped daemons is no longer paid on every routine review.
+- Schema surface grows by one optional key; doc maintenance and the test matrix grow by a small fixed amount.
+- The `review.*` namespace stays internally consistent. Future `review.profiles.*` or `review.groups.*` can coexist with `review.default_reviewers` without renaming.
+- Cross-runtime impact is minimal: the change operates on the detection layer in `detect_clis`, not on any per-runtime adapter. The existing `resolve_model_ids: "omit"` path used by non-Claude runtimes (Codex, OpenCode, Gemini CLI, Kilo) is unaffected.
+- One additional surface area for bug reports — primarily edge interactions between the key and `--all` / individual flags, which the test plan covers.
+- If telemetry is ever opt-in for `.planning/config.json` shape, adoption of this key becomes a useful signal for whether to invest in the richer profiles design (see Q-3).
+
+## Open questions
+
+- **Q-1.** Should `review.default_reviewers: []` be a schema error, or should it fall back to "all detected"? *Proposal: schema error.* Rationale: users who want "all detected" can simply omit the key (more readable); `[]` looks like a typo or programmatic mistake; surfacing the ambiguity is more helpful than silently swallowing it. **Blocking** — affects schema validation and tests.
+- **Q-2.** Is a new `--no-default` flag warranted, or is it equivalent to `--all` for this use case? *Proposal: drop unless a concrete difference surfaces during implementation.* Non-blocking.
+- **Q-3.** Do we want to commit now to leaving `review.profiles.*` open as a future namespace, or is that premature? *Proposal: leave open; document the intent in `docs/CONFIGURATION.md` so the next contributor doesn't pick a conflicting key.* Non-blocking.
+- **Q-4.** Should `/gsd-config --integrations` learn the new key in this pass, or as a fast follow once the schema + resolution land? *Proposal: fast follow.* Non-blocking; depends on contributor bandwidth.
+- **Q-5.** Should the verbose-mode "selection source" line ship in the first pass, or only behind `--verbose`? *Proposal: behind `--verbose`.* Non-blocking.
+- **Q-6.** Slug normalization: lowercase-on-read (proposed) vs. exact-match enforcement at the schema layer. *Proposal: normalize.* Non-blocking; document either way.
+
+## References
+
+- Feature issue: `#3079`
+- Configuration reference: `docs/CONFIGURATION.md` — `review.models.<cli>`, `review.*_host`, and the **absent = enabled** pattern
+- Workflow file owning the no-flag branch: `workflows/review.md` (line 52)
+- Existing slug validation pattern: `^[a-zA-Z0-9_-]+$` (used for `review.models.<cli>` keys)
+- Related PRD: `0011-review-default-reviewers-prd.md`
--- a/docs/adr/README.md
+++ b/docs/adr/README.md
@@ -18,7 +18,10 @@ Each ADR documents one architectural decision: what was decided, why, and what c
 | [0008-installer-migration-module.md](0008-installer-migration-module.md) | Installer Migration Module owns install-time upgrade safety | Accepted |
 | [0009-shell-command-projection-module.md](0009-shell-command-projection-module.md) | Shell Command Projection Module owns runtime-aware OS command rendering | Accepted |
 | [0010-file-operation-engine-module.md](0010-file-operation-engine-module.md) | File Operation Engine Module owns safe runtime/config file mutations | Proposed |
+| [0010-skill-surface-budget-module.md](0010-skill-surface-budget-module.md) | Skill Surface Budget Module — earlier draft superseded by ADR-0011 | Superseded by 0011 |
 | [0011-skill-surface-budget-module.md](0011-skill-surface-budget-module.md) | Skill Surface Budget Module owns install-time profile staging and runtime surface control | Accepted |
+| [0011-review-default-reviewers.md](0011-review-default-reviewers.md) | Review default-reviewers selection policy for /gsd:review | Accepted |
+| [0011-review-default-reviewers-prd.md](0011-review-default-reviewers-prd.md) | PRD for review.default_reviewers feature (#3464) | Reference |

 ## Seam map

--- a/get-shit-done/bin/lib/install-profiles.cjs
+++ b/get-shit-done/bin/lib/install-profiles.cjs
@@ -60,6 +60,7 @@ const PROFILES = Object.freeze({
    'discuss-phase',
    'plan-phase',
    'execute-phase',
+    'phase',
    'help',
    'update',
  ]),
--- a/get-shit-done/bin/lib/shell-command-projection.cjs
+++ b/get-shit-done/bin/lib/shell-command-projection.cjs
@@ -1,6 +1,8 @@
 'use strict';

 const path = require('path');
+const fs = require('fs');
+const { spawnSync, execFileSync } = require('child_process');

 /**
 * Shell Command Projection Module
@@ -352,6 +354,155 @@ function formatSdkPathDiagnostic({ shimDir, platform, runDir }) {
  return { shimLocationLine, actionLines, shellActions, npxNoteLines, isNpx, isWin32 };
 }

+// ─── Subprocess dispatch ──────────────────────────────────────────────────────
+
+function _spawnResult(result, program) {
+  if (result.error && result.error.code === 'ENOENT') {
+    return { exitCode: 127, stdout: '', stderr: `${program}: not found` };
+  }
+  return {
+    exitCode: result.status ?? 1,
+    stdout: (result.stdout ?? '').toString().trim(),
+    stderr: (result.stderr ?? '').toString().trim(),
+  };
+}
+
+function execGit(args, opts = {}) {
+  const result = spawnSync('git', args, {
+    cwd: opts.cwd,
+    encoding: 'utf-8',
+    stdio: 'pipe',
+    timeout: opts.timeout ?? 10_000,
+  });
+  return _spawnResult(result, 'git');
+}
+
+function execNpm(args, opts = {}) {
+  const result = spawnSync('npm', args, {
+    cwd: opts.cwd,
+    shell: process.platform === 'win32',
+    encoding: 'utf-8',
+    stdio: ['ignore', 'pipe', 'pipe'],
+    timeout: opts.timeout ?? 15_000,
+  });
+  return _spawnResult(result, 'npm');
+}
+
+function execTool(program, args, opts = {}) {
+  const result = spawnSync(program, args, {
+    cwd: opts.cwd,
+    encoding: 'utf-8',
+    stdio: 'pipe',
+    timeout: opts.timeout ?? 30_000,
+  });
+  return _spawnResult(result, program);
+}
+
+function probeTty(opts = {}) {
+  const platform = opts.platform ?? process.platform;
+  if (platform === 'win32') return null;
+  try {
+    const ttyPath = execFileSync('tty', [], {
+      encoding: 'utf-8',
+      stdio: ['inherit', 'pipe', 'ignore'],
+    }).trim();
+    if (!ttyPath || ttyPath === 'not a tty') return null;
+    return ttyPath;
+  } catch {
+    return null;
+  }
+}
+
+// ─── Platform file I/O ────────────────────────────────────────────────────────
+
+function _normalizeMd(content) {
+  if (!content || typeof content !== 'string') return content;
+  let text = content.replace(/\r\n/g, '\n');
+  const lines = text.split('\n');
+  const result = [];
+  const fenceRegex = /^```/;
+  const insideFence = new Array(lines.length);
+  let fenceOpen = false;
+  for (let i = 0; i < lines.length; i++) {
+    if (fenceRegex.test(lines[i].trimEnd())) {
+      if (fenceOpen) {
+        insideFence[i] = false;
+        fenceOpen = false;
+      } else {
+        insideFence[i] = false;
+        fenceOpen = true;
+      }
+    } else {
+      insideFence[i] = fenceOpen;
+    }
+  }
+  for (let i = 0; i < lines.length; i++) {
+    const line = lines[i];
+    const prev = i > 0 ? lines[i - 1] : '';
+    const prevTrimmed = prev.trimEnd();
+    const trimmed = line.trimEnd();
+    const isFenceLine = fenceRegex.test(trimmed);
+    if (/^#{1,6}\s/.test(trimmed) && i > 0 && prevTrimmed !== '' && prevTrimmed !== '---') result.push('');
+    if (isFenceLine && i > 0 && prevTrimmed !== '' && !insideFence[i] && (i === 0 || !insideFence[i - 1] || isFenceLine)) {
+      if (i === 0 || !insideFence[i - 1]) result.push('');
+    }
+    if (/^(\s*[-*+]\s|\s*\d+\.\s)/.test(line) && i > 0 && prevTrimmed !== '' && !/^(\s*[-*+]\s|\s*\d+\.\s)/.test(prev) && prevTrimmed !== '---') result.push('');
+    result.push(line);
+    if (/^#{1,6}\s/.test(trimmed) && i < lines.length - 1 && (lines[i + 1] ?? '').trimEnd() !== '') result.push('');
+    if (/^```\s*$/.test(trimmed) && i > 0 && insideFence[i - 1] && i < lines.length - 1 && (lines[i + 1] ?? '').trimEnd() !== '') result.push('');
+    if (/^(\s*[-*+]\s|\s*\d+\.\s)/.test(line) && i < lines.length - 1) {
+      const next = lines[i + 1];
+      if (next !== undefined && next.trimEnd() !== '' && !/^(\s*[-*+]\s|\s*\d+\.\s)/.test(next) && !/^\s/.test(next)) result.push('');
+    }
+  }
+  text = result.join('\n');
+  text = text.replace(/\n{3,}/g, '\n\n');
+  text = text.replace(/\n*$/, '\n');
+  return text;
+}
+
+function normalizeContent(filePath, content, opts = {}) {
+  const encoding = opts.encoding ?? 'utf-8';
+  const isMd = path.extname(filePath).toLowerCase() === '.md';
+  let normalized;
+  if (isMd) {
+    normalized = _normalizeMd(content);
+  } else {
+    normalized = (content ?? '').replace(/\r\n/g, '\n').replace(/\n*$/, '\n');
+  }
+  return { content: normalized, encoding };
+}
+
+function platformWriteSync(filePath, content, opts = {}) {
+  const { content: normalized, encoding } = normalizeContent(filePath, content, opts);
+  fs.mkdirSync(path.dirname(filePath), { recursive: true });
+  const tmpPath = filePath + '.tmp.' + process.pid;
+  try {
+    fs.writeFileSync(tmpPath, normalized, encoding);
+    fs.renameSync(tmpPath, filePath);
+  } catch {
+    try { fs.unlinkSync(tmpPath); } catch { /* already gone */ }
+    fs.writeFileSync(filePath, normalized, encoding);
+  }
+}
+
+function platformReadSync(filePath, opts = {}) {
+  const encoding = opts.encoding ?? 'utf-8';
+  try {
+    return fs.readFileSync(filePath, encoding);
+  } catch (err) {
+    if (err.code === 'ENOENT') {
+      if (opts.required) throw err;
+      return null;
+    }
+    throw err;
+  }
+}
+
+function platformEnsureDir(dirPath) {
+  fs.mkdirSync(dirPath, { recursive: true });
+}
+
 module.exports = {
  hookCommandNeedsPowerShellCallOperator,
  formatHookCommandForRuntime,
@@ -370,4 +521,12 @@ module.exports = {
  projectPersistentPathExportActions,
  buildWindowsShimTriple,
  formatSdkPathDiagnostic,
+  execGit,
+  execNpm,
+  execTool,
+  probeTty,
+  normalizeContent,
+  platformWriteSync,
+  platformReadSync,
+  platformEnsureDir,
 };
--- a/tests/install-minimal-backcompat.test.cjs
+++ b/tests/install-minimal-backcompat.test.cjs
@@ -22,7 +22,7 @@ const INSTALL_SCRIPT = path.join(__dirname, '..', 'bin', 'install.js');
 const MANIFEST_NAME = 'gsd-file-manifest.json';

 describe('install-minimal-backcompat: PROFILES.core matches MINIMAL_SKILL_ALLOWLIST', () => {
-  test('PROFILES.core contains the same 6 skills as MINIMAL_SKILL_ALLOWLIST', () => {
+  test('PROFILES.core contains the same 7 skills as MINIMAL_SKILL_ALLOWLIST', () => {
    assert.deepStrictEqual(
      [...PROFILES.core].sort(),
      [...MINIMAL_SKILL_ALLOWLIST].sort(),
@@ -59,10 +59,10 @@ describe('install-minimal-backcompat: --minimal and --profile=core produce the s
    }
  }

-  test('--minimal produces mode "minimal" with exactly 6 skills', () => {
+  test('--minimal produces mode "minimal" with exactly 7 skills', () => {
    const r = installAndGetManifest(['--minimal']);
    assert.strictEqual(r.mode, 'minimal');
-    assert.strictEqual(r.skillCount, 6);
+    assert.strictEqual(r.skillCount, 7);
  });

  test('--minimal writes .gsd-profile marker with "core"', () => {
--- a/tests/install-minimal.test.cjs
+++ b/tests/install-minimal.test.cjs
@@ -45,6 +45,7 @@ describe('install-profiles: MINIMAL_SKILL_ALLOWLIST', () => {
        'execute-phase',
        'help',
        'new-project',
+        'phase',
        'plan-phase',
        'update',
      ],
@@ -108,6 +109,7 @@ describe('install-profiles: stageSkillsForMode', () => {
    fs.writeFileSync(path.join(tmp, 'do.md'), '# do\n');
    fs.writeFileSync(path.join(tmp, 'help.md'), '# help\n');
    fs.writeFileSync(path.join(tmp, 'new-project.md'), '# new-project\n');
+    fs.writeFileSync(path.join(tmp, 'phase.md'), '# phase\n');
    fs.writeFileSync(path.join(tmp, 'discuss-phase.md'), '# discuss-phase\n');
    fs.writeFileSync(path.join(tmp, 'update.md'), '# update\n');
    fs.writeFileSync(path.join(tmp, 'progress.md'), '# progress\n');
@@ -136,6 +138,7 @@ describe('install-profiles: stageSkillsForMode', () => {
        'execute-phase.md',
        'help.md',
        'new-project.md',
+        'phase.md',
        'plan-phase.md',
        'update.md',
      ]);
@@ -539,21 +542,21 @@ describe('install: manifest records mode for both profiles', () => {
  test('default install records mode: "full" with the full skill+agent count', () => {
    const r = manifestModeAfterInstall([]);
    assert.strictEqual(r.mode, 'full');
-    assert.ok(r.skillCount > 6, `full install should have >6 skills, got ${r.skillCount}`);
+    assert.ok(r.skillCount > 7, `full install should have >7 skills, got ${r.skillCount}`);
    assert.ok(r.agentCount > 0, `full install should have agents, got ${r.agentCount}`);
  });

-  test('--minimal records mode: "minimal" with exactly 6 skills and 0 agents', () => {
+  test('--minimal records mode: "minimal" with exactly 7 skills and 0 agents', () => {
    const r = manifestModeAfterInstall(['--minimal']);
    assert.strictEqual(r.mode, 'minimal');
-    assert.strictEqual(r.skillCount, 6);
+    assert.strictEqual(r.skillCount, 7);
    assert.strictEqual(r.agentCount, 0);
  });

  test('--core-only is an alias for --minimal', () => {
    const r = manifestModeAfterInstall(['--core-only']);
    assert.strictEqual(r.mode, 'minimal');
-    assert.strictEqual(r.skillCount, 6);
+    assert.strictEqual(r.skillCount, 7);
    assert.strictEqual(r.agentCount, 0);
  });
 });
--- a/tests/install-profiles-resolve.test.cjs
+++ b/tests/install-profiles-resolve.test.cjs
@@ -26,7 +26,7 @@ describe('PROFILES map', () => {
    assert.ok('full' in PROFILES, 'PROFILES.full missing');
  });

-  test('PROFILES.core contains the 6 main-loop skills', () => {
+  test('PROFILES.core contains the 7 main-loop skills (including phase)', () => {
    const core = PROFILES.core;
    assert.ok(Array.isArray(core), 'core should be an array');
    const sorted = [...core].sort();
@@ -35,6 +35,7 @@ describe('PROFILES map', () => {
      'execute-phase',
      'help',
      'new-project',
+      'phase',
      'plan-phase',
      'update',
    ]);
@@ -66,20 +67,18 @@ describe('resolveProfile', () => {
    assert.strictEqual(result.skills, '*');
  });

-  test('resolves core profile — returns 6+ skills (closure adds phase)', () => {
+  test('resolves core profile — returns 7+ skills', () => {
    const manifest = loadSkillsManifest(REAL_COMMANDS_DIR);
    const result = resolveProfile({ modes: ['core'], manifest });
    assert.strictEqual(result.name, 'core');
    assert.ok(result.skills instanceof Set, 'skills should be a Set');
-    // core has 6 base; closure adds phase (referenced by discuss/plan/execute/new-project)
-    // and config (referenced by discuss-phase, new-project), and more
-    assert.ok(result.skills.size >= 6, `core closure should have >=6 skills, got ${result.skills.size}`);
-    // All 6 base skills must be present
+    // core has 7 base skills.
+    assert.ok(result.skills.size >= 7, `core closure should have >=7 skills, got ${result.skills.size}`);
+    // All base skills must be present
    for (const s of PROFILES.core) {
      assert.ok(result.skills.has(s), `core closure should include ${s}`);
    }
-    // phase must be included via closure (discuss-phase, plan-phase, etc. require it)
-    assert.ok(result.skills.has('phase'), 'core closure must include phase (required by discuss/plan/execute-phase)');
+    assert.ok(result.skills.has('phase'), 'core closure must include phase');
  });

  test('resolves standard profile — returns superset of core', () => {
--- a/tests/shell-command-projection-dispatch.test.cjs
+++ b/tests/shell-command-projection-dispatch.test.cjs
@@ -0,0 +1,263 @@
+'use strict';
+
+const { describe, test, beforeEach, afterEach } = require('node:test');
+const assert = require('node:assert/strict');
+const path = require('node:path');
+const fs = require('node:fs');
+
+const {
+  execGit,
+  execNpm,
+  execTool,
+  probeTty,
+  normalizeContent,
+  platformWriteSync,
+  platformReadSync,
+  platformEnsureDir,
+} = require(path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'shell-command-projection.cjs'));
+
+const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs');
+
+// ─── execGit ─────────────────────────────────────────────────────────────────
+
+describe('execGit', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempGitProject(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('returns { exitCode, stdout, stderr } shape', () => {
+    const result = execGit(['--version']);
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'exitCode'), 'missing exitCode');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stdout'), 'missing stdout');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stderr'), 'missing stderr');
+  });
+
+  test('exitCode 0 for successful command', () => {
+    const result = execGit(['--version']);
+    assert.strictEqual(result.exitCode, 0);
+  });
+
+  test('stdout contains version string for --version', () => {
+    const result = execGit(['--version']);
+    assert.strictEqual(typeof result.stdout, 'string');
+    assert.ok(result.stdout.length > 0, 'stdout should not be empty for git --version');
+  });
+
+  test('exitCode non-zero for failing command — does not throw', () => {
+    const result = execGit(['status', '--porcelain'], { cwd: '/tmp/definitely-not-a-git-repo-8675309' });
+    assert.notStrictEqual(result.exitCode, 0);
+  });
+
+  test('respects cwd option', () => {
+    const result = execGit(['status', '--porcelain'], { cwd: tmpDir });
+    assert.strictEqual(result.exitCode, 0);
+  });
+});
+
+// ─── execNpm ─────────────────────────────────────────────────────────────────
+
+describe('execNpm', () => {
+  test('returns { exitCode, stdout, stderr } shape', () => {
+    const result = execNpm(['--version']);
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'exitCode'), 'missing exitCode');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stdout'), 'missing stdout');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stderr'), 'missing stderr');
+  });
+
+  test('exitCode 0 for npm --version', () => {
+    const result = execNpm(['--version']);
+    assert.strictEqual(result.exitCode, 0);
+  });
+
+  test('stdout is non-empty for npm --version', () => {
+    const result = execNpm(['--version']);
+    assert.ok(result.stdout.trim().length > 0);
+  });
+});
+
+// ─── execTool ────────────────────────────────────────────────────────────────
+
+describe('execTool', () => {
+  test('returns { exitCode, stdout, stderr } shape for known program', () => {
+    const result = execTool('node', ['--version']);
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'exitCode'), 'missing exitCode');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stdout'), 'missing stdout');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'stderr'), 'missing stderr');
+  });
+
+  test('exitCode 0 for node --version', () => {
+    const result = execTool('node', ['--version']);
+    assert.strictEqual(result.exitCode, 0);
+  });
+
+  test('exitCode 127 and no throw when program does not exist', () => {
+    const result = execTool('definitely-not-a-real-program-8675309', []);
+    assert.strictEqual(result.exitCode, 127);
+    assert.strictEqual(result.stdout, '');
+    assert.strictEqual(typeof result.stderr, 'string');
+  });
+});
+
+// ─── probeTty ────────────────────────────────────────────────────────────────
+
+describe('probeTty', () => {
+  test('returns string or null — never throws', () => {
+    const result = probeTty();
+    assert.ok(result === null || typeof result === 'string', `expected string|null, got ${typeof result}`);
+  });
+
+  test('returns null when platform is win32', () => {
+    const result = probeTty({ platform: 'win32' });
+    assert.strictEqual(result, null);
+  });
+});
+
+// ─── normalizeContent ────────────────────────────────────────────────────────
+
+describe('normalizeContent', () => {
+  test('returns { content, encoding } shape', () => {
+    const result = normalizeContent('file.md', 'hello\n');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'content'), 'missing content');
+    assert.ok(Object.prototype.hasOwnProperty.call(result, 'encoding'), 'missing encoding');
+  });
+
+  test('normalizes CRLF to LF for .md files', () => {
+    const result = normalizeContent('file.md', 'line1\r\nline2\r\n');
+    assert.ok(!result.content.includes('\r\n'), 'CRLF should be normalized to LF');
+  });
+
+  test('normalizes CRLF to LF for non-.md files', () => {
+    const result = normalizeContent('file.json', '{"a":1}\r\n');
+    assert.ok(!result.content.includes('\r\n'), 'CRLF should be normalized to LF');
+  });
+
+  test('enforces single trailing newline for .md files', () => {
+    const result = normalizeContent('file.md', 'hello');
+    assert.ok(result.content.endsWith('\n'), 'should end with newline');
+    assert.ok(!result.content.endsWith('\n\n'), 'should not end with double newline');
+  });
+
+  test('enforces single trailing newline for non-.md files', () => {
+    const result = normalizeContent('file.txt', 'hello');
+    assert.ok(result.content.endsWith('\n'));
+    assert.ok(!result.content.endsWith('\n\n'));
+  });
+
+  test('applies full markdownlint normalization for .md files — blank line before heading', () => {
+    const input = [
+      '# Title',
+      'paragraph',
+      '## Section',
+    ].join('\n');
+    const result = normalizeContent('file.md', input);
+    assert.ok(result.content.includes('\n\n## Section'), 'MD022: blank line before heading');
+  });
+
+  test('does NOT apply markdown structural rules to non-.md files', () => {
+    const input = 'paragraph\n## Not a heading in json\n';
+    const result = normalizeContent('file.json', input);
+    assert.strictEqual(result.content, input);
+  });
+
+  test('encoding defaults to utf-8', () => {
+    const result = normalizeContent('file.md', 'hello\n');
+    assert.strictEqual(result.encoding, 'utf-8');
+  });
+});
+
+// ─── platformWriteSync ───────────────────────────────────────────────────────
+
+describe('platformWriteSync', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempDir(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('written file exists and is a regular file', () => {
+    const filePath = path.join(tmpDir, 'output.md');
+    platformWriteSync(filePath, '# Hello\n');
+    assert.ok(fs.statSync(filePath).isFile());
+  });
+
+  test('written file has non-zero size', () => {
+    const filePath = path.join(tmpDir, 'output.md');
+    platformWriteSync(filePath, '# Hello\n');
+    assert.ok(fs.statSync(filePath).size > 0);
+  });
+
+  test('creates parent directory if absent', () => {
+    const filePath = path.join(tmpDir, 'nested', 'deep', 'output.md');
+    platformWriteSync(filePath, '# Hello\n');
+    assert.ok(fs.statSync(filePath).isFile());
+  });
+
+  test('mtime advances on re-write', (t) => {
+    const filePath = path.join(tmpDir, 'output.md');
+    platformWriteSync(filePath, '# First\n');
+    const mtimeBefore = fs.statSync(filePath).mtimeMs;
+    // Small delay to ensure mtime differs
+    const start = Date.now();
+    while (Date.now() - start < 10) { /* busy wait */ }
+    platformWriteSync(filePath, '# Second\n');
+    const mtimeAfter = fs.statSync(filePath).mtimeMs;
+    assert.ok(mtimeAfter >= mtimeBefore, 'mtime should advance or stay same on re-write');
+  });
+
+  test('no temp file left on disk after successful write', () => {
+    const filePath = path.join(tmpDir, 'output.md');
+    platformWriteSync(filePath, '# Hello\n');
+    const tmpFiles = fs.readdirSync(tmpDir).filter(f => f.includes('.tmp.'));
+    assert.strictEqual(tmpFiles.length, 0, 'no temp files should remain');
+  });
+});
+
+// ─── platformReadSync ────────────────────────────────────────────────────────
+
+describe('platformReadSync', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempDir(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('returns null for missing file when required is false (default)', () => {
+    const result = platformReadSync(path.join(tmpDir, 'nonexistent.md'));
+    assert.strictEqual(result, null);
+  });
+
+  test('throws for missing file when required is true', () => {
+    assert.throws(
+      () => platformReadSync(path.join(tmpDir, 'nonexistent.md'), { required: true }),
+      /ENOENT/,
+    );
+  });
+
+  test('returns string content for existing file', () => {
+    const filePath = path.join(tmpDir, 'existing.md');
+    fs.writeFileSync(filePath, '# Hello\n', 'utf-8');
+    const result = platformReadSync(filePath);
+    assert.strictEqual(typeof result, 'string');
+    assert.ok(result.length > 0);
+  });
+});
+
+// ─── platformEnsureDir ───────────────────────────────────────────────────────
+
+describe('platformEnsureDir', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempDir(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('creates directory if absent', () => {
+    const dirPath = path.join(tmpDir, 'new', 'nested', 'dir');
+    platformEnsureDir(dirPath);
+    assert.ok(fs.statSync(dirPath).isDirectory());
+  });
+
+  test('no error when directory already exists — idempotent', () => {
+    const dirPath = path.join(tmpDir, 'existing');
+    fs.mkdirSync(dirPath);
+    assert.doesNotThrow(() => platformEnsureDir(dirPath));
+  });
+});