Files
msd-core/docs/adr/0009-shell-command-projection-module.md
Tom Boucher 1e091d2bcb refactor(shell-projection): remove deprecated wrappers + finalize ADRs (Phase 4, #3468) (#3484)
* refactor(shell-projection): remove deprecated wrappers + finalize ADRs (Phase 4, #3468)

Final phase of the shell-command-projection expansion. Removes the legacy
core.cjs wrappers (`atomicWriteFileSync`, `safeReadFile`, `normalizeMd`)
now that every call site lives behind the seam, plus three Phase-3
stragglers (`graphify.cjs`, `template.cjs`, dead import in
`profile-pipeline.cjs`).

Documentation:
- ADR-0009: addendum noting Phase 1–4 scope expansion (subprocess +
  file I/O ownership), supersession of "does not execute" constraint,
  and resolution of open Q4.
- ADR-0010: status changed to Superseded by ADR-0009 with explanation.
- CONTEXT.md "Shell Command Projection Module" entry already current
  from Phase 1 — no edit needed.

Tests:
- `tests/atomic-write.test.cjs` deleted — wrapper it tested is gone;
  `atomic-write-coverage.test.cjs` (Phase 3) covers platformWriteSync.
- `tests/core.test.cjs::safeReadFile` + `::normalizeMd` describes
  deleted — wrappers are gone.
- `tests/concurrency-safety.test.cjs` normalizeMd suite (behavioral /
  perf / snapshot) repointed via 2-line shim at the seam's
  `normalizeContent` — full regression coverage preserved.

Test result: 9059/9041/18 — exact pre-Phase-4 baseline. All 18
failures are pre-existing path-with-spaces local-env issues.

Closes #3468

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(shell-projection): migrate remaining raw fs.writeFileSync sites (Phase 4, #3468)

Sweeps the 7 raw fs.writeFileSync call sites that bypassed the seam through Phase 3,
folding them into platformWriteSync. Net -14 lines: deletes the local writeFileAtomicSync
helper in installer-migrations.cjs and collapses surface.cjs's manual tmp+rename into a
single seam call.

Sites migrated:
- drift.cjs (1) — frontmatter write
- learnings.cjs (1) — learning record JSON write
- install-profiles.cjs (1) — profile marker write (collapsed redundant mkdir)
- gsd2-import.cjs (1) — imported file write (collapsed redundant mkdir)
- surface.cjs (1) — surface state write (replaced manual tmp+rename block)
- installer-migrations.cjs (3) — journal init/finalize + rewrite-json action;
  deleted private writeFileAtomicSync helper and its three call sites

Two sites intentionally retained outside the seam:
- planning-workspace.cjs:241 — workspace lock (wx-flag atomic-create; previously excluded by Phase 3)
- installer-migrations.cjs:220 — install migration lock (fd write into wx-opened handle)
- writeInstallState (installer-migrations.cjs) — strict atomic contract for install state;
  the seam's fallback-to-direct-write on rename failure would silently violate the
  invariant that install state must never be left half-written. Inline tmp+rename with
  rethrow keeps the original guarantee.

Tests: 9059 / 9041 / 18 — exactly the pre-Phase-4 baseline; 18 failures are the
pre-existing path-with-spaces local-env issues, identical files as before.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(installer-migrations): use strict atomic write for rollback install-state restore

The rollback path was restoring INSTALL_STATE via platformWriteSync, which falls
back to a direct write on rename failure and would silently violate the
half-written invariant that the install-state contract guarantees elsewhere.

Extracts the strict tmp+rename logic from writeInstallState into a shared
atomicWriteInstallState(configDir, content) helper and routes both
writeInstallState and rollbackAppliedMigrationResult through it. Preserves the
existing null-handling (rmSync when previousInstallStateBytes === null) and
existing failure-collection (failures.push on caught errors).

Byte-faithful restore: previousInstallStateBytes is written as-is (no JSON
parse round-trip), preserving the exact prior file contents on restore.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-13 20:46:02 -04:00

132 lines
7.6 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Shell Command Projection Module owns runtime-aware OS command rendering
- **Status:** Accepted
- **Date:** 2026-05-12
We propose introducing a Shell Command Projection Module that owns projection from typed command intent to concrete shell/runtime-specific command text. GSD currently hand-builds hook commands, PATH repair commands, shim scripts, and other serialized OS-facing command strings across installer call sites. That drift has repeatedly produced cross-shell regressions (`#2376`, `#2979`, `#3002`, `#3011`, `#3181`, `#3393`, `#3413`). The proposed seam concentrates quoting, path-style, and runtime-wrapper policy in one module while keeping real subprocess execution on array-arg/non-shell paths.
## Decision
- Add a **Shell Command Projection Module** under `get-shit-done/bin/lib/` as the single owner for runtime-aware command-text rendering.
- Feed the module typed inputs (`platform`, `shell`, `runtime`, executable token, args, path policy) instead of prebuilt shell strings.
- Keep callers as thin Adapters that request projected text for:
- managed hook commands in `settings.json`
- managed hook commands in runtime config TOML/JSON surfaces
- user-facing PATH repair / setup instructions
- generated shim / wrapper script text written to disk
- Keep internal subprocess execution (`spawnSync`, `execFileSync`, SDK query dispatch) outside this seam. The module does **not** become a generic command runner.
- Make runtime-specific wrappers explicit policy at the seam (for example, emit PowerShell call-operator prefixes only for shells/runtimes that require them).
- Make path-style projection explicit policy at the seam (`native Windows`, `POSIX slash`, `$HOME`-relative, `project-dir-relative`, etc.).
- Prefer typed IR outputs that tests can assert against directly, then render text at the final Adapter.
## Initial Scope
First migration slice should cover installer/runtime surfaces already proving this bug class:
1. managed JS and `.sh` hook command construction
2. managed hook rewrite / normalization on reinstall
3. Codex hook block command rendering
4. PATH diagnostic action-line rendering
5. Windows shim / wrapper script text builders
6. local-install hook command rendering (`$CLAUDE_PROJECT_DIR` vs cwd-relative runtime paths)
It should **not** in the first pass expand into workflow markdown ` ```bash` blocks or replace safe internal subprocess APIs with shell-string execution.
## Migration Inventory
### `bin/install.js`
These call sites should migrate behind the Shell Command Projection Module:
- `formatHookCommandForShell()` — `bin/install.js:605-608`
- `formatManagedHookScriptToken()` — `bin/install.js:610-614`
- `rewriteLegacyManagedNodeHookCommands()` projection path — `bin/install.js:639-718`
- `buildCodexHookBlock()` — `bin/install.js:752-768`
- `rewriteLegacyCodexHookBlock()` — `bin/install.js:784-812`
- `buildHookCommand()` — `bin/install.js:827-857`
- `localCmd()` builder — `bin/install.js:8855-8857`
- per-hook command constructors — `bin/install.js:8858-8875`, `9065-9067`, `9090-9092`, `9119-9121`, `9143-9145`, `9173-9176`
- legacy PATH hint strings — `bin/install.js:9764-9768`
- `formatSdkPathDiagnostic()` render path — `bin/install.js:10075-10084`, builder at `10532-10582`
- `buildWindowsShimTriple()` — `bin/install.js:10487-10510`
### Tests expected to move with the seam
- `tests/bug-2979-hook-absolute-node.test.cjs`
- `tests/sh-hook-paths.test.cjs`
- `tests/bug-3011-sdk-path-diagnostic.test.cjs`
- `tests/bug-3017-codex-hook-absolute-node.test.cjs`
- `tests/bug-2376-opencode-windows-home-path.test.cjs`
- `tests/bug-3020-install-shell-path-probe.test.cjs`
- `tests/bug-3359-stale-gsd-sdk-path-version.test.cjs`
## Interface sketch
The module should accept typed command intent rather than concatenated shell fragments. Example shape:
```js
projectShellCommand({
platform: 'linux' | 'darwin' | 'win32',
shell: 'bash' | 'zsh' | 'cmd' | 'pwsh',
runtime: 'claude' | 'gemini' | 'codex' | 'opencode' | 'copilot' | 'antigravity' | 'generic',
executable: { kind: 'node' | 'bash' | 'pwsh' | 'literal', token: '...' },
args: ['...'],
pathStyle: 'native' | 'posix' | 'home-relative' | 'project-relative',
})
```
For user-facing multi-line guidance, the seam should return typed action IR first, then let the installer print it:
```js
projectShellActions({ intent: 'prepend-path', platform, targetDir, runtime })
```
For generated shim/wrapper files, the seam should own script text rendering too:
```js
projectShellScript({ shell: 'cmd' | 'pwsh' | 'sh', executable, argsTemplate })
```
## Consequences
- Quoting, slash-direction, wrapper-prefix, and variable-expansion policy become local to one module.
- Installer/runtime call sites become thinner and stop inventing sibling string builders.
- Windows runtime-specific regressions become easier to classify as seam bugs instead of one-off installer bugs.
- Tests can assert against typed projection IR instead of source-grepping ad-hoc string concatenation sites.
- The first implementation will move a broad installer surface, so scope discipline matters: start with installer/runtime projection only, not every shell string in the repo.
- If accepted, `CONTEXT.md` should gain a canonical **Shell Command Projection Module** entry and future architecture reviews should treat out-of-seam command rendering as drift.
## Open questions
- Whether `hooks.shell_preference` from `#3082` should become an input policy consumed by this module or remain a higher-level runtime config concern.
- Whether Windows Git Bash should be modeled as explicit `shell: 'bash'` + `platform: 'win32'` or as a distinct shell target.
- Whether existing shim/script builders should migrate in the first pass or follow immediately after the hook/diagnostic path is stable.
- Whether the seam should live entirely in installer land or later become shared with other runtime-output surfaces outside `bin/install.js`.
## References
- Feature issue: `#3439`
- Related bug history: `#2376`, `#2979`, `#3002`, `#3011`, `#3017`, `#3020`, `#3082`, `#3181`, `#3393`, `#3413`
- See `0005-sdk-architecture-seam-map.md`
- See `0008-installer-migration-module.md`
## Update — 2026-05-13 (Phases 1–4 expansion, `#3465`–`#3468`)
The seam grew beyond the original "rendering only" scope. The "does not become a generic command runner" and "does not replace safe internal subprocess APIs" constraints (Decision §17, Initial Scope §33) were intentionally superseded.
**Scope now owned by `shell-command-projection.cjs`:**
- runtime-aware command-text rendering (original ADR scope)
- subprocess dispatch — `execGit`, `execNpm`, `execTool`, `probeTty` (Phase 2, `#3466`)
- platform file I/O — `platformWriteSync`, `platformReadSync`, `platformEnsureDir`, `normalizeContent` (Phase 3, `#3467`)
- legacy wrappers `atomicWriteFileSync` / `safeReadFile` / `normalizeMd` removed from `core.cjs` (Phase 4, `#3468`)
**Result-shape invariant:** all `exec*` return `{ exitCode, stdout, stderr }` and never throw on non-zero exit. Platform-conditional logic (`shell: process.platform === 'win32'`, `probeTty` Windows null return, `.md`-aware normalization) lives only at the seam.
**Open question resolutions:**
- Q4 (installer-only vs shared seam): **resolved — shared.** The seam lives in `get-shit-done/bin/lib/`, consumed by installer, planning workflow, and every fs/subprocess call site across the tool.
- Q1, Q2, Q3 (`hooks.shell_preference`, Windows Git Bash modeling, shim/script builder migration timing): unresolved, carried forward as projection-design concerns independent of the I/O expansion.
See CONTEXT.md "Shell Command Projection Module" entry for the canonical current-state description.