feat(shell-projection): centralize managed hook command policy (#3450)

* feat(shell-projection): route persistent PATH hints through projection seam

* refactor(shell-projection): unify managed hook command policy

* fix(install): guard malformed settings hooks during uninstall

* chore(changeset): add fragment for pr 3450

* fix(install): guard malformed settings hook entries
This commit is contained in:
Tom Boucher
2026-05-12 20:15:43 -04:00
committed by GitHub
parent 16f5009270
commit bf3c736029
10 changed files with 341 additions and 42 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 3450
---
**Stabilized shell-command projection integration and uninstall hook cleanup** — rebasing the ADR-0009 projection work onto `main` now preserves the typed PATH-action seam while keeping the persistent PATH-export adapter used by installer messaging. It also hardens `settings.json` hook cleanup in `bin/install.js` to skip malformed/non-command hook entries before calling managed-hook classification, preventing uninstall-time crashes when mixed hook shapes are present.

View File

@@ -9,11 +9,13 @@ const {
buildWindowsShimTriple: buildWindowsShimTripleFromProjection,
formatSdkPathDiagnostic: formatSdkPathDiagnosticFromProjection,
isManagedHookBasename,
isManagedHookCommand,
projectLocalHookPrefix,
projectLegacySettingsHookCommand,
projectManagedHookCommand,
projectPathActionProjection,
projectPortableHookBaseDir,
projectPersistentPathExportActions,
projectShellCommandText,
projectCodexHookTomlCommand,
} = require('../get-shit-done/bin/lib/shell-command-projection.cjs');
@@ -3276,14 +3278,6 @@ function stripLeakedGsdCodexSections(content) {
* Pure function, exported for test coverage. Returns the input unchanged
* if no GSD-managed hook section is present.
*/
// Legacy hook command basenames to detect during strip. Template-literal
// form so install-hooks-copy.test.cjs's quoted-literal guard continues to
// catch accidental regressions where someone *registers* the inverted
// `gsd-update-check.js` filename in a Codex hook command.
const STALE_HOOK_BASENAMES = new Set([
`gsd-update-check.js`,
`gsd-check-update.js`,
]);
function stripStaleGsdHookBlocks(configContent) {
const sections = getTomlTableSections(configContent);
const lineRecords = getTomlLineRecords(configContent);
@@ -3319,10 +3313,10 @@ function stripStaleGsdHookBlocks(configContent) {
continue;
}
if (typeof parsed.value !== 'string') continue;
// Match the basename — Codex configs reference these files by absolute
// path under the user's `.codex/hooks/` directory.
const basename = parsed.value.split(/[\\/]/).pop() || '';
if (STALE_HOOK_BASENAMES.has(basename)) {
if (isManagedHookCommand(parsed.value, {
surface: 'codex-toml',
includeLegacyAliases: true,
})) {
return true;
}
}
@@ -6718,22 +6712,19 @@ function uninstall(isGlobal, runtime = 'claude') {
// Remove GSD hooks from settings — per-hook granularity to preserve
// user hooks that share an entry with a GSD hook (#1755 followup)
const isGsdHookCommand = (cmd) =>
cmd && (cmd.includes('gsd-check-update') || cmd.includes('gsd-statusline') ||
cmd.includes('gsd-session-state') || cmd.includes('gsd-context-monitor') ||
cmd.includes('gsd-phase-boundary') || cmd.includes('gsd-prompt-guard') ||
cmd.includes('gsd-read-guard') || cmd.includes('gsd-read-injection-scanner') ||
cmd.includes('gsd-update-banner') ||
cmd.includes('gsd-validate-commit') || cmd.includes('gsd-workflow-guard'));
for (const eventName of ['SessionStart', 'PostToolUse', 'AfterTool', 'PreToolUse', 'BeforeTool']) {
if (settings.hooks && settings.hooks[eventName]) {
const before = JSON.stringify(settings.hooks[eventName]);
settings.hooks[eventName] = settings.hooks[eventName]
.map(entry => {
if (!entry.hooks || !Array.isArray(entry.hooks)) return entry;
if (!entry || typeof entry !== 'object' || !Array.isArray(entry.hooks)) return entry;
// Filter out individual GSD hooks, keep user hooks
entry.hooks = entry.hooks.filter(h => !isGsdHookCommand(h.command));
entry.hooks = entry.hooks.filter((h) => {
if (!h || typeof h.command !== 'string') return true;
return !isManagedHookCommand(h.command, {
surface: 'settings-json',
});
});
return entry.hooks.length > 0 ? entry : null;
})
.filter(Boolean);
@@ -9757,7 +9748,8 @@ function homePathCoveredByRc(globalBin, homeDir, rcFileNames) {
* - a diagnostic "already covered via rc file" note, if an rc file has
* `export PATH="$HOME/…/bin:$PATH"` (or equivalent) and the user just
* needs to reopen their shell
* - the absolute `echo 'export PATH="…:$PATH"' >> ~/.zshrc` suggestion,
* - projected shell actions that append `export PATH="…:$PATH"` to
* `~/.zshrc` / `~/.bashrc` when neither PATH nor rc files cover globalBin
* if neither PATH nor any rc file covers globalBin
*
* Exported for tests; the installer calls this from finishInstall.
@@ -9786,14 +9778,13 @@ function maybeSuggestPathExport(globalBin, homeDir) {
console.log('');
console.log(` ${yellow}⚠${reset} ${bold}${globalBin}${reset} is not on your PATH.`);
console.log(` Add it with one of:`);
const projected = projectPathActionProjection({
mode: 'persist',
const projected = projectPersistentPathExportActions({
targetDir: globalBin,
platform: process.platform,
});
for (const action of projected.shellActions) {
const label = action.label ? `${action.label}: ` : '';
console.log(` ${cyan}${label}${action.command}${reset}`);
const labelPrefix = action.label ? `${action.label}: ` : '';
console.log(` ${cyan}${labelPrefix}${action.command}${reset}`);
}
console.log('');
}

View File

@@ -1,6 +1,6 @@
# Shell Command Projection Module owns runtime-aware OS command rendering
- **Status:** Proposed
- **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.

View File

@@ -0,0 +1,102 @@
# File Operation Engine Module owns safe runtime/config file mutations
- **Status:** Proposed
- **Date:** 2026-05-12
We propose introducing a File Operation Engine Module that owns policy for managed file reads, writes, deletes, locks, backups, and rollbacks across installer, migration, and planning surfaces. Today, file mutation behavior is duplicated across `bin/install.js`, `get-shit-done/bin/lib/installer-migrations.cjs`, and multiple planning modules, with drift in atomic-write guarantees, path safety checks, and ownership classification.
This ADR also captures where Shell Command Projection Module policy should be consumed or expanded for hook-command-specific file mutations, so shell command drift and file mutation drift do not evolve as separate bug classes.
## Decision
- Add a **File Operation Engine Module** under `get-shit-done/bin/lib/` as the single seam for file mutation safety policy.
- Keep command-text projection in the Shell Command Projection Module (ADR-0009), but route projection-adjacent hook file mutations through shared managed-hook ownership policy.
- Move file operation adapters to the new seam in two tracks:
- **Track A (projection-adjacent):** runtime config hook-command detection/rewrite/delete paths consume shared managed-hook policy from the projection seam.
- **Track B (solution-wide):** shared file operation engine owns atomic write, path containment, lock behavior, rollback bookkeeping, and best-effort cleanup policy.
- Keep internal subprocess execution out of this seam (same boundary as ADR-0009): this is a file operation seam, not a command runner.
## Initial Scope
1. Unify managed-hook ownership classification used by install/uninstall/migration hook config rewrites.
2. Unify atomic write behavior currently duplicated in installer/core/migration paths.
3. Unify lock-file lifecycle policy used by planning workspace and installer migration journal flows.
4. Expose typed file mutation plan IR for tests (`rewrite-json`, `rewrite-text` with format (`toml`/`markdown`/`plain`), `delete-file`, `backup-file`, `restore-file`, `ensure-dir`).
## Migration Inventory
### Projection-adjacent file mutation drift (Track A)
- `bin/install.js`
- hook cleanup command detection (`isGsdHookCommand`)
- stale Codex hook strip basenames (`STALE_HOOK_BASENAMES`)
- settings/config hook entry prune/rewrite paths
- `get-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs`
- `isManagedCodexHookCommand` regex/path detection duplicated from installer-owned hook policy
- `get-shit-done/bin/lib/shell-command-projection.cjs`
- `isManagedHookBasename` already owns part of this policy and should become the canonical owner
### Solution-wide file operation drift (Track B)
- `bin/install.js`
- local `atomicWriteFileSync` and temp cleanup registry
- large inlined read/modify/write + backup/rollback logic for runtime config and hooks
- `get-shit-done/bin/lib/core.cjs`
- `atomicWriteFileSync` helper diverges in fallback behavior from installer/migration variants
- `get-shit-done/bin/lib/installer-migrations.cjs`
- separate `writeFileAtomicSync`, rollback journaling, lock handling, and containment checks
- `get-shit-done/bin/lib/planning-workspace.cjs` and `get-shit-done/bin/lib/state.cjs`
- duplicated lock-file create/release/remove patterns and best-effort cleanup semantics
- `get-shit-done/bin/lib/roadmap.cjs`, `phase.cjs`, `milestone.cjs`, `frontmatter.cjs`, `drift.cjs`
- direct read/modify/write flows with inconsistent atomicity and normalization policy application
## Interface sketch
The File Operation Engine Module should expose typed mutation planning and execution helpers:
```js
planFileMutations({
rootDir,
operations: [
{ type: 'rewrite-json', relPath, mutate },
{ type: 'rewrite-text', relPath, mutate, format: 'toml' | 'markdown' | 'plain' },
{ type: 'delete-file', relPath },
{ type: 'ensure-dir', relPath },
],
ownership: { mode: 'managed-only' | 'allow-user', classifier },
})
```
```js
applyFileMutationPlan({
plan,
atomic: true,
rollback: true,
lock: { scope: 'config' | 'planning', id: '...' },
})
```
For projection-adjacent paths, adapters should consume projection policy:
```js
isManagedHookCommand(commandText, { surface, configDir })
```
## Consequences
- File mutation safety policy becomes local to one module, reducing drift across installer/migration/planning paths.
- Shell command projection and hook ownership classification stay aligned at one seam family.
- Tests can assert typed mutation IR and reason codes instead of source-grep and duplicated predicate mirrors.
- Initial migration is broad; sequencing should prioritize projection-adjacent hook config paths first, then converge atomic-write and lock semantics.
## Open questions
- Whether lock semantics should be one shared policy for installer + planning, or two adapters over one lock primitive.
- Whether SDK query write paths should consume the same engine in the first pass or follow after CJS convergence.
- Whether file mutation telemetry (per-op reason codes and rollback events) should be required for all engine adapters.
## References
- ADR-0008: `0008-installer-migration-module.md`
- ADR-0009: `0009-shell-command-projection-module.md`
- Related bug history: `#1755`, `#2866`, `#2979`, `#3002`, `#3017`, `#3439`

View File

@@ -16,7 +16,8 @@ Each ADR documents one architectural decision: what was decided, why, and what c
| [0006-planning-path-projection-module.md](0006-planning-path-projection-module.md) | Planning Path Projection Module for SDK query handlers | Accepted |
| [0007-sdk-package-seam-module.md](0007-sdk-package-seam-module.md) | SDK Package Seam Module owns SDK-to-get-shit-done-cc compatibility | Accepted |
| [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 | Proposed |
| [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 |
## Seam map
@@ -28,3 +29,7 @@ ADR 0008 documents the Installer Migration Module for safe install-time moves, r
ADR 0009 documents the Shell Command Projection Module seam for runtime-aware
projection of installer-owned command text and projection IR.
ADR 0010 documents the File Operation Engine Module seam for converging
installer/migration/planning file mutation safety policy, and its relationship
to ADR 0009 hook-command ownership policy.

View File

@@ -1,6 +1,6 @@
'use strict';
const path = require('path');
const { isManagedHookCommand } = require('../shell-command-projection.cjs');
function isStructurallyEmpty(value) {
if (value === null || value === undefined) return true;
@@ -11,12 +11,11 @@ function isStructurallyEmpty(value) {
}
function isManagedCodexHookCommand(command, configDir) {
if (typeof command !== 'string') return false;
if (typeof configDir !== 'string' || configDir.length === 0) return false;
const normalizedCommand = command.replace(/\\/g, '/');
const managedHooksDir = `${path.join(configDir, 'hooks').replace(/\\/g, '/')}/`;
if (!normalizedCommand.includes(managedHooksDir)) return false;
return /(^|[\\/\s"'])(gsd-check-update\.js|gsd-update-check\.js)(?=$|[\s"'])/.test(normalizedCommand);
return isManagedHookCommand(command, {
surface: 'codex-hooks-json',
includeLegacyAliases: true,
configDir,
});
}
function pruneLegacyCodexHooksJsonValue(value, configDir) {

View File

@@ -1,5 +1,7 @@
'use strict';
const path = require('path');
/**
* Shell Command Projection Module
*
@@ -99,6 +101,37 @@ const MANAGED_HOOK_BASENAMES_BY_SURFACE = {
]),
};
const MANAGED_HOOK_COMMAND_BASENAMES_BY_SURFACE = {
'settings-json': new Set([
'gsd-check-update.js',
'gsd-statusline.js',
'gsd-context-monitor.js',
'gsd-prompt-guard.js',
'gsd-read-guard.js',
'gsd-read-injection-scanner.js',
'gsd-update-banner.js',
'gsd-workflow-guard.js',
'gsd-session-state.sh',
'gsd-validate-commit.sh',
'gsd-phase-boundary.sh',
]),
'codex-toml': new Set([
'gsd-check-update.js',
]),
'codex-hooks-json': new Set([
'gsd-check-update.js',
]),
};
const LEGACY_MANAGED_HOOK_ALIASES_BY_SURFACE = {
'codex-toml': new Set([
'gsd-update-check.js',
]),
'codex-hooks-json': new Set([
'gsd-update-check.js',
]),
};
function managedHookSurfaceSet(surface = 'settings-json') {
return MANAGED_HOOK_BASENAMES_BY_SURFACE[surface] || MANAGED_HOOK_BASENAMES_BY_SURFACE['settings-json'];
}
@@ -110,6 +143,36 @@ function isManagedHookBasename(scriptPathOrBasename, opts = {}) {
return managedHookSurfaceSet(surface).has(basename);
}
function managedHookCommandSurfaceSet(surface = 'settings-json', includeLegacyAliases = false) {
const base = MANAGED_HOOK_COMMAND_BASENAMES_BY_SURFACE[surface]
|| MANAGED_HOOK_COMMAND_BASENAMES_BY_SURFACE['settings-json'];
if (!includeLegacyAliases) return base;
const aliases = LEGACY_MANAGED_HOOK_ALIASES_BY_SURFACE[surface];
if (!aliases || aliases.size === 0) return base;
return new Set([...base, ...aliases]);
}
function isManagedHookCommand(commandText, opts = {}) {
if (typeof commandText !== 'string') return false;
const surface = opts.surface || 'settings-json';
const includeLegacyAliases = opts.includeLegacyAliases === true;
const managedBasenames = managedHookCommandSurfaceSet(surface, includeLegacyAliases);
if (!managedBasenames || managedBasenames.size === 0) return false;
const normalizedCommand = commandText.replace(/\\/g, '/');
if (typeof opts.configDir === 'string' && opts.configDir.length > 0) {
const normalizedHooksDir = `${path.join(opts.configDir, 'hooks').replace(/\\/g, '/')}/`;
if (!normalizedCommand.includes(normalizedHooksDir)) return false;
}
for (const basename of managedBasenames) {
const escapedBasename = basename.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const pattern = new RegExp(`(^|[\\\\/\\s"'` + '`' + `])${escapedBasename}(?=$|[\\s"'` + '`' + `])`);
if (pattern.test(normalizedCommand)) return true;
}
return false;
}
/**
* Projection helper for legacy settings.json hook rewrites.
*
@@ -158,11 +221,9 @@ function escapePowerShellSingleQuoted(value) {
function escapePosixDoubleQuoted(value) {
return String(value).replace(/[\\$"`]/g, '\\$&');
}
function escapeSingleQuotedShellLiteral(value) {
return String(value).replace(/'/g, "'\\''");
}
function renderShellActionLines(shellActions = []) {
return shellActions.map((action) => {
if (!action || !action.command) return '';
@@ -231,8 +292,16 @@ function projectPathActionProjection({
};
}
function projectPersistentPathExportActions({ targetDir, platform = process.platform }) {
const projected = projectPathActionProjection({
mode: 'persist',
targetDir,
platform,
});
return { shellActions: projected.shellActions };
}
function buildWindowsShimTriple(shimSrc) {
const path = require('path');
const shimAbs = path.resolve(shimSrc);
const shimQuoted = JSON.stringify(shimAbs);
const invocation = {
@@ -292,11 +361,13 @@ module.exports = {
projectShellCommandText,
projectManagedHookCommand,
isManagedHookBasename,
isManagedHookCommand,
projectLegacySettingsHookCommand,
escapeTomlDoubleQuotedString,
projectCodexHookTomlCommand,
projectPathActionProjection,
renderShellActionLines,
projectPersistentPathExportActions,
buildWindowsShimTriple,
formatSdkPathDiagnostic,
};

View File

@@ -13,6 +13,7 @@ const {
hookCommandNeedsPowerShellCallOperator,
formatHookCommandForRuntime,
isManagedHookBasename,
isManagedHookCommand,
projectLocalHookPrefix,
projectLegacySettingsHookCommand,
projectPortableHookBaseDir,
@@ -141,4 +142,48 @@ describe('bug #3439: shell projection module owns managed-hook policy and legacy
'/opt/custom/.claude',
);
});
test('isManagedHookCommand classifies managed settings hooks and leaves user commands untouched', () => {
assert.equal(
isManagedHookCommand('"/usr/local/bin/node" "/Users/me/.claude/hooks/gsd-statusline.js"', {
surface: 'settings-json',
}),
true,
);
assert.equal(
isManagedHookCommand('"C:/Program Files/Git/bin/bash.exe" "C:/Users/me/.claude/hooks/gsd-session-state.sh"', {
surface: 'settings-json',
}),
true,
);
assert.equal(
isManagedHookCommand('bash /Users/me/.claude/hooks/custom-lint.sh', {
surface: 'settings-json',
}),
false,
);
});
test('isManagedHookCommand supports codex surfaces and optional legacy alias matching', () => {
const command = '"/usr/local/bin/node" "/Users/me/.codex/hooks/gsd-check-update.js"';
assert.equal(
isManagedHookCommand(command, {
surface: 'codex-toml',
}),
true,
);
assert.equal(
isManagedHookCommand('"/usr/local/bin/node" "/Users/me/.codex/hooks/gsd-update-check.js"', {
surface: 'codex-toml',
}),
false,
);
assert.equal(
isManagedHookCommand('"/usr/local/bin/node" "/Users/me/.codex/hooks/gsd-update-check.js"', {
surface: 'codex-toml',
includeLegacyAliases: true,
}),
true,
);
});
});

View File

@@ -0,0 +1,58 @@
'use strict';
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
const path = require('node:path');
const migration = require(path.join(
__dirname,
'..',
'get-shit-done',
'bin',
'lib',
'installer-migrations',
'002-codex-legacy-hooks-json.cjs',
));
describe('bug #3442: codex legacy hooks.json migration consumes shared managed-hook policy', () => {
test('plan prunes managed codex hook commands including legacy alias', () => {
const configDir = '/Users/me/.codex';
const hooksJson = {
hooks: [
{ command: '"/usr/local/bin/node" "/Users/me/.codex/hooks/gsd-check-update.js"' },
{ command: '"/usr/local/bin/node" "/Users/me/.codex/hooks/gsd-update-check.js"' },
{ command: '"/usr/local/bin/node" "/Users/me/.codex/hooks/custom-hook.js"' },
],
};
const actions = migration.plan({
configDir,
readJson: () => ({ exists: true, error: null, value: hooksJson }),
});
assert.equal(actions.length, 1);
assert.equal(actions[0].type, 'rewrite-json');
assert.equal(actions[0].relPath, 'hooks.json');
assert.deepEqual(actions[0].value, {
hooks: [
{ command: '"/usr/local/bin/node" "/Users/me/.codex/hooks/custom-hook.js"' },
],
});
});
test('plan preserves similarly named commands outside the managed hooks directory', () => {
const configDir = '/Users/me/.codex';
const hooksJson = {
hooks: [
{ command: '"/usr/local/bin/node" "/tmp/other/hooks/gsd-check-update.js"' },
],
};
const actions = migration.plan({
configDir,
readJson: () => ({ exists: true, error: null, value: hooksJson }),
});
assert.deepEqual(actions, []);
});
});

View File

@@ -17,6 +17,14 @@ const os = require('os');
const path = require('path');
const INSTALL_PATH = path.join(__dirname, '..', 'bin', 'install.js');
const PROJECTION_PATH = path.join(
__dirname,
'..',
'get-shit-done',
'bin',
'lib',
'shell-command-projection.cjs',
);
function loadInstaller() {
process.env.GSD_TEST_MODE = '1';
@@ -34,8 +42,10 @@ function cleanup(dir) {
describe('installer HOME-relative PATH detection (#2620)', () => {
let installer;
let projection;
before(() => {
installer = loadInstaller();
projection = require(PROJECTION_PATH);
});
test('homePathCoveredByRc is exported', () => {
@@ -257,10 +267,23 @@ describe('installer HOME-relative PATH detection (#2620)', () => {
}
const joined = logs.join('\n');
assert.ok(
/echo 'export PATH=/.test(joined),
`installer should emit absolute export suggestion when rc does not cover globalBin; got:\n${joined}`,
assert.equal(
typeof projection.projectPersistentPathExportActions,
'function',
'shell command projection module must export projectPersistentPathExportActions',
);
const projected = projection.projectPersistentPathExportActions({
targetDir: globalBin,
platform: process.platform,
});
assert.ok(Array.isArray(projected.shellActions), 'projected.shellActions must be an array');
assert.ok(projected.shellActions.length >= 2, 'expected at least zsh/bash projected actions');
for (const action of projected.shellActions) {
assert.ok(
joined.includes(action.command),
`installer should render projected command "${action.command}". Output:\n${joined}`,
);
}
} finally {
cleanup(home);
}