diff --git a/.changeset/shell-projection-io-seam.md b/.changeset/shell-projection-io-seam.md new file mode 100644 index 000000000..5f7aa062f --- /dev/null +++ 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. diff --git a/CONTEXT.md b/CONTEXT.md index 3acb3b32a..be1d2bd8f 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -613,3 +613,63 @@ After stripping prose @-refs, some command `` 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 ` 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 ` (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 diff --git a/bin/install.js b/bin/install.js index 349af5b17..801a1af25 100755 --- a/bin/install.js +++ b/bin/install.js @@ -535,7 +535,7 @@ if (hasUninstall) { // Show help if requested if (hasHelp) { - console.log(` ${yellow}Usage:${reset} npx get-shit-done-cc [options]\n\n ${yellow}Options:${reset}\n ${cyan}-g, --global${reset} Install globally (to config directory)\n ${cyan}-l, --local${reset} Install locally (to current directory)\n ${cyan}--claude${reset} Install for Claude Code only\n ${cyan}--opencode${reset} Install for OpenCode only\n ${cyan}--gemini${reset} Install for Gemini only\n ${cyan}--kilo${reset} Install for Kilo only\n ${cyan}--codex${reset} Install for Codex only\n ${cyan}--copilot${reset} Install for Copilot only\n ${cyan}--antigravity${reset} Install for Antigravity only\n ${cyan}--cursor${reset} Install for Cursor only\n ${cyan}--windsurf${reset} Install for Windsurf only\n ${cyan}--augment${reset} Install for Augment only\n ${cyan}--trae${reset} Install for Trae only\n ${cyan}--qwen${reset} Install for Qwen Code only\n ${cyan}--hermes${reset} Install for Hermes Agent only\n ${cyan}--cline${reset} Install for Cline only\n ${cyan}--codebuddy${reset} Install for CodeBuddy only\n ${cyan}--all${reset} Install for all runtimes\n ${cyan}-u, --uninstall${reset} Uninstall GSD (remove all GSD files)\n ${cyan}-c, --config-dir ${reset} Specify custom config directory\n ${cyan}-h, --help${reset} Show this help message\n ${cyan}--force-statusline${reset} Replace existing statusline config\n ${cyan}--portable-hooks${reset} Emit \$HOME-relative hook paths in settings.json\n (for WSL/Docker bind-mount setups; also GSD_PORTABLE_HOOKS=1)\n ${cyan}--profile=${reset} Install a named skill profile. Profiles:\n core — 6 main-loop skills only (~87 desc tokens)\n standard — ~13 skills incl. phase, review, config (~700)\n full — all 66 skills (default)\n Composable: --profile=core,audit installs union of closures.\n Profile is persisted and respected by \`gsd update\`.\n ${cyan}--minimal${reset} Alias for --profile=core (back-compat).\n Cuts cold-start overhead from ~12k tokens to ~700.\n Alias: --core-only.\n\n ${yellow}Examples:${reset}\n ${dim}# Interactive install (prompts for runtime and location)${reset}\n npx get-shit-done-cc\n\n ${dim}# Install for Claude Code globally${reset}\n npx get-shit-done-cc --claude --global\n\n ${dim}# Install for Gemini globally${reset}\n npx get-shit-done-cc --gemini --global\n\n ${dim}# Install for Kilo globally${reset}\n npx get-shit-done-cc --kilo --global\n\n ${dim}# Install for Codex globally${reset}\n npx get-shit-done-cc --codex --global\n\n ${dim}# Install for Copilot globally${reset}\n npx get-shit-done-cc --copilot --global\n\n ${dim}# Install for Copilot locally${reset}\n npx get-shit-done-cc --copilot --local\n\n ${dim}# Install for Antigravity globally${reset}\n npx get-shit-done-cc --antigravity --global\n\n ${dim}# Install for Antigravity locally${reset}\n npx get-shit-done-cc --antigravity --local\n\n ${dim}# Install for Cursor globally${reset}\n npx get-shit-done-cc --cursor --global\n\n ${dim}# Install for Cursor locally${reset}\n npx get-shit-done-cc --cursor --local\n\n ${dim}# Install for Windsurf globally${reset}\n npx get-shit-done-cc --windsurf --global\n\n ${dim}# Install for Windsurf locally${reset}\n npx get-shit-done-cc --windsurf --local\n\n ${dim}# Install for Augment globally${reset}\n npx get-shit-done-cc --augment --global\n\n ${dim}# Install for Augment locally${reset}\n npx get-shit-done-cc --augment --local\n\n ${dim}# Install for Trae globally${reset}\n npx get-shit-done-cc --trae --global\n\n ${dim}# Install for Trae locally${reset}\n npx get-shit-done-cc --trae --local\n\n ${dim}# Install for Hermes Agent globally${reset}\n npx get-shit-done-cc --hermes --global\n\n ${dim}# Install for Hermes Agent locally${reset}\n npx get-shit-done-cc --hermes --local\n\n ${dim}# Install for Cline locally${reset}\n npx get-shit-done-cc --cline --local\n\n ${dim}# Install for CodeBuddy globally${reset}\n npx get-shit-done-cc --codebuddy --global\n\n ${dim}# Install for CodeBuddy locally${reset}\n npx get-shit-done-cc --codebuddy --local\n\n ${dim}# Install for all runtimes globally${reset}\n npx get-shit-done-cc --all --global\n\n ${dim}# Install to custom config directory${reset}\n npx get-shit-done-cc --kilo --global --config-dir ~/.kilo-work\n\n ${dim}# Install to current project only${reset}\n npx get-shit-done-cc --claude --local\n\n ${dim}# Uninstall GSD from Cursor globally${reset}\n npx get-shit-done-cc --cursor --global --uninstall\n\n ${yellow}Notes:${reset}\n The --config-dir option is useful when you have multiple configurations.\n It takes priority over CLAUDE_CONFIG_DIR / OPENCODE_CONFIG_DIR / GEMINI_CONFIG_DIR / KILO_CONFIG_DIR / CODEX_HOME / COPILOT_CONFIG_DIR / ANTIGRAVITY_CONFIG_DIR / CURSOR_CONFIG_DIR / WINDSURF_CONFIG_DIR / AUGMENT_CONFIG_DIR / TRAE_CONFIG_DIR / QWEN_CONFIG_DIR / HERMES_HOME / CLINE_CONFIG_DIR / CODEBUDDY_CONFIG_DIR environment variables.\n`); + console.log(` ${yellow}Usage:${reset} npx get-shit-done-cc [options]\n\n ${yellow}Options:${reset}\n ${cyan}-g, --global${reset} Install globally (to config directory)\n ${cyan}-l, --local${reset} Install locally (to current directory)\n ${cyan}--claude${reset} Install for Claude Code only\n ${cyan}--opencode${reset} Install for OpenCode only\n ${cyan}--gemini${reset} Install for Gemini only\n ${cyan}--kilo${reset} Install for Kilo only\n ${cyan}--codex${reset} Install for Codex only\n ${cyan}--copilot${reset} Install for Copilot only\n ${cyan}--antigravity${reset} Install for Antigravity only\n ${cyan}--cursor${reset} Install for Cursor only\n ${cyan}--windsurf${reset} Install for Windsurf only\n ${cyan}--augment${reset} Install for Augment only\n ${cyan}--trae${reset} Install for Trae only\n ${cyan}--qwen${reset} Install for Qwen Code only\n ${cyan}--hermes${reset} Install for Hermes Agent only\n ${cyan}--cline${reset} Install for Cline only\n ${cyan}--codebuddy${reset} Install for CodeBuddy only\n ${cyan}--all${reset} Install for all runtimes\n ${cyan}-u, --uninstall${reset} Uninstall GSD (remove all GSD files)\n ${cyan}-c, --config-dir ${reset} Specify custom config directory\n ${cyan}-h, --help${reset} Show this help message\n ${cyan}--force-statusline${reset} Replace existing statusline config\n ${cyan}--portable-hooks${reset} Emit \$HOME-relative hook paths in settings.json\n (for WSL/Docker bind-mount setups; also GSD_PORTABLE_HOOKS=1)\n ${cyan}--profile=${reset} Install a named skill profile. Profiles:\n core — 7 main-loop skills incl. phase (~130 desc tokens)\n standard — ~13 skills incl. phase, review, config (~700)\n full — all 66 skills (default)\n Composable: --profile=core,audit installs union of closures.\n Profile is persisted and respected by \`gsd update\`.\n ${cyan}--minimal${reset} Alias for --profile=core (back-compat).\n Cuts cold-start overhead from ~12k tokens to ~700.\n Alias: --core-only.\n\n ${yellow}Examples:${reset}\n ${dim}# Interactive install (prompts for runtime and location)${reset}\n npx get-shit-done-cc\n\n ${dim}# Install for Claude Code globally${reset}\n npx get-shit-done-cc --claude --global\n\n ${dim}# Install for Gemini globally${reset}\n npx get-shit-done-cc --gemini --global\n\n ${dim}# Install for Kilo globally${reset}\n npx get-shit-done-cc --kilo --global\n\n ${dim}# Install for Codex globally${reset}\n npx get-shit-done-cc --codex --global\n\n ${dim}# Install for Copilot globally${reset}\n npx get-shit-done-cc --copilot --global\n\n ${dim}# Install for Copilot locally${reset}\n npx get-shit-done-cc --copilot --local\n\n ${dim}# Install for Antigravity globally${reset}\n npx get-shit-done-cc --antigravity --global\n\n ${dim}# Install for Antigravity locally${reset}\n npx get-shit-done-cc --antigravity --local\n\n ${dim}# Install for Cursor globally${reset}\n npx get-shit-done-cc --cursor --global\n\n ${dim}# Install for Cursor locally${reset}\n npx get-shit-done-cc --cursor --local\n\n ${dim}# Install for Windsurf globally${reset}\n npx get-shit-done-cc --windsurf --global\n\n ${dim}# Install for Windsurf locally${reset}\n npx get-shit-done-cc --windsurf --local\n\n ${dim}# Install for Augment globally${reset}\n npx get-shit-done-cc --augment --global\n\n ${dim}# Install for Augment locally${reset}\n npx get-shit-done-cc --augment --local\n\n ${dim}# Install for Trae globally${reset}\n npx get-shit-done-cc --trae --global\n\n ${dim}# Install for Trae locally${reset}\n npx get-shit-done-cc --trae --local\n\n ${dim}# Install for Hermes Agent globally${reset}\n npx get-shit-done-cc --hermes --global\n\n ${dim}# Install for Hermes Agent locally${reset}\n npx get-shit-done-cc --hermes --local\n\n ${dim}# Install for Cline locally${reset}\n npx get-shit-done-cc --cline --local\n\n ${dim}# Install for CodeBuddy globally${reset}\n npx get-shit-done-cc --codebuddy --global\n\n ${dim}# Install for CodeBuddy locally${reset}\n npx get-shit-done-cc --codebuddy --local\n\n ${dim}# Install for all runtimes globally${reset}\n npx get-shit-done-cc --all --global\n\n ${dim}# Install to custom config directory${reset}\n npx get-shit-done-cc --kilo --global --config-dir ~/.kilo-work\n\n ${dim}# Install to current project only${reset}\n npx get-shit-done-cc --claude --local\n\n ${dim}# Uninstall GSD from Cursor globally${reset}\n npx get-shit-done-cc --cursor --global --uninstall\n\n ${yellow}Notes:${reset}\n The --config-dir option is useful when you have multiple configurations.\n It takes priority over CLAUDE_CONFIG_DIR / OPENCODE_CONFIG_DIR / GEMINI_CONFIG_DIR / KILO_CONFIG_DIR / CODEX_HOME / COPILOT_CONFIG_DIR / ANTIGRAVITY_CONFIG_DIR / CURSOR_CONFIG_DIR / WINDSURF_CONFIG_DIR / AUGMENT_CONFIG_DIR / TRAE_CONFIG_DIR / QWEN_CONFIG_DIR / HERMES_HOME / CLINE_CONFIG_DIR / CODEBUDDY_CONFIG_DIR environment variables.\n`); process.exit(0); } @@ -7613,7 +7613,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { // @-references resolve correctly (#2376 Windows, #2831 macOS/Linux). // gsd update marker re-application (ADR-0010 Deviation 2): // Resolve which profile to use for this runtime's install: - // 1. --minimal / --core-only → back-compat path (stageSkillsForMode keeps strict 6-skill list) + // 1. --minimal / --core-only → back-compat path (stageSkillsForMode keeps strict core allowlist) // 2. Explicit --profile= → use it (overrides any marker) // 3. Marker exists in targetDir → honor it (prevents silent expansion on update) // 4. Else → 'full' (back-compat for fresh non-interactive installs) @@ -7622,7 +7622,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { // differ, the caller may use mostRestrictiveProfile() across the per-runtime // results — here we resolve each runtime independently. // - // Note: --minimal uses stageSkillsForMode (back-compat: strictly 6 skills, no closure). + // Note: --minimal uses stageSkillsForMode (back-compat: strict allowlist, no closure). // Named profiles (--profile=X or marker-driven) use resolveProfile() for transitive closure. const _activeProfileName = hasMinimal ? 'core' // --minimal is a back-compat alias for the core profile; marker records 'core' diff --git a/docs/adr/0010-skill-surface-budget-module.md b/docs/adr/0010-skill-surface-budget-module.md new file mode 100644 index 000000000..250c4a305 --- /dev/null +++ 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 `` 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=` and `--profile=,` (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:` or `\b\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, agents: Set } +``` + +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) diff --git a/docs/adr/0011-review-default-reviewers-prd.md b/docs/adr/0011-review-default-reviewers-prd.md new file mode 100644 index 000000000..79f09f297 --- /dev/null +++ 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.`). +- 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.`, `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` diff --git a/docs/adr/0011-review-default-reviewers.md b/docs/adr/0011-review-default-reviewers.md new file mode 100644 index 000000000..d7832af72 --- /dev/null +++ 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.` 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.` 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.`, `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.` keys) +- Related PRD: `0011-review-default-reviewers-prd.md` diff --git a/docs/adr/README.md b/docs/adr/README.md index 54289bcf5..d4d880788 100644 --- 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 diff --git a/get-shit-done/bin/lib/install-profiles.cjs b/get-shit-done/bin/lib/install-profiles.cjs index 688e867f4..c0cfb7970 100644 --- 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', ]), diff --git a/get-shit-done/bin/lib/shell-command-projection.cjs b/get-shit-done/bin/lib/shell-command-projection.cjs index f909aca7e..82e1ad6c0 100644 --- 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, }; diff --git a/tests/install-minimal-backcompat.test.cjs b/tests/install-minimal-backcompat.test.cjs index 655608893..8a3196f25 100644 --- 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"', () => { diff --git a/tests/install-minimal.test.cjs b/tests/install-minimal.test.cjs index bf3e52683..2c110a922 100644 --- 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); }); }); diff --git a/tests/install-profiles-resolve.test.cjs b/tests/install-profiles-resolve.test.cjs index 85bd715d1..84a394709 100644 --- 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', () => { diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs new file mode 100644 index 000000000..17cc1f50e --- /dev/null +++ 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)); + }); +});