From 58bfae9d6a3771b7001ecb993b7e064b9ac53043 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 11 Jun 2026 17:21:04 -0400 Subject: [PATCH] =?UTF-8?q?refactor(#1059):=20phase=205f-1=20=E2=80=94=20e?= =?UTF-8?q?xtract=20standalone=20hook-surface=20writers=20into=20a=20modul?= =?UTF-8?q?e=20=E2=80=94=20ADR-857/1016=20(#1064)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(#1059): phase 5f-1 — extract standalone hook-surface writers into a module Extract the structurally-isolated hook-surface writer functions (cline/cursor/ copilot/codex-hooks-json + buildHookCommand + atomicWriteFileSync + node/bash runner resolvers) out of bin/install.js into a new src/runtime-hooks-surface.cts module (-693 LOC from install.js). Behavior-preserving: install.js requires + re-exports the moved functions (module.exports surface preserved); no descriptor reads, no behavior change. Prerequisite for the descriptor-drive (5f-2), mirroring ADR-3660's artifactLayout extract→drive split. Review caught + fixed 3 coupling issues: (HIGH) the module's atomicWriteFileSync dropped the shared __atomicWrittenTmps temp-tracking → now ONE shared set (module owns it, install.js aliases it, both cleanups read it); (drift) buildHookCommand called resolveNodeRunner(opts) vs the original resolveNodeRunner() → reverted; two source-grep tests (workflow-guard, sh-hook-paths) that scanned install.js for the moved functions → made behavioral/non-vacuous; duplicate runner resolvers consolidated. Settings-json hook block (~648 LOC) deferred to 5f-1b; descriptor-drive to 5f-2. New-module checklist done. ~62 hook test files green; gsd-test 17592/0. Closes #1059 Co-Authored-By: Claude Opus 4.8 * chore(#1059): reconcile CLI Modules count after merging next (uat-predicate) Merging current next (which added uat-predicate.cjs via #247) alongside this branch's runtime-hooks-surface.cjs put the filesystem at 107 bin/lib modules, but both sides had independently bumped the INVENTORY headline 105→106 so the merge under-counted. Set "CLI Modules (107 shipped)" + regenerate INVENTORY-MANIFEST.json. Both module rows already present. Fixes inventory-counts.test.cjs (the only CI red). Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .gitignore | 1 + CONTEXT.md | 3 + bin/install.js | 920 +--------------- docs/ARCHITECTURE.md | 1 + docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 3 +- eslint.config.mjs | 1 + src/runtime-hooks-surface.cts | 1140 ++++++++++++++++++++ tests/sh-hook-paths.test.cjs | 82 +- tests/workflow-guard-registration.test.cjs | 133 ++- 10 files changed, 1365 insertions(+), 920 deletions(-) create mode 100644 src/runtime-hooks-surface.cts diff --git a/.gitignore b/.gitignore index 08e407f21..3cca0f941 100644 --- a/.gitignore +++ b/.gitignore @@ -130,6 +130,7 @@ build/ /gsd-core/bin/lib/planning-workspace.cjs /gsd-core/bin/lib/runtime-artifact-layout.cjs /gsd-core/bin/lib/runtime-config-adapter-registry.cjs +/gsd-core/bin/lib/runtime-hooks-surface.cjs /gsd-core/bin/lib/command-routing-hub.cjs /gsd-core/bin/lib/core.cjs /gsd-core/bin/lib/core-utils.cjs diff --git a/CONTEXT.md b/CONTEXT.md index 4dad44981..f443ea245 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -175,6 +175,9 @@ A Capability whose integration shape brings its own external process, service, o `RULESET.CAPABILITY.step-additive-gate-blocks=a `step` hook is purely additive (invoke skill + produce artifacts, NEVER halts the host); host-blocking preconditions are `gate`s (blocking:true, onError:halt); runtime/mode context (auto/chain vs manual) self-gates IN THE SKILL, not via `when` (config-only). §5.6 = plan:pre step (ui-phase; skill self-gates on frontend+pipeline, auto-fires only in pipelines) + a NEW plan:pre gate (frontend-and-no-UI-SPEC → halt, when:workflow.ui_safety_gate); the loop.render-hooks dispatch template handles steps AND gates. Resolves #1022.` +### Runtime Hooks Surface Module +Standalone hook-surface writer module extracted from `bin/install.js` as ADR-857 phase 5f-1 (behavior-preserving relocation, no logic change). Owns: Cline rules-body/agents-md/pre-tool-use hook generation (`buildClineRulesBody`, `buildClineAgentsMdBody`, `buildClinePreToolUseHook`, `mergeGsdAgentsMd`, `writeClineArtifacts`); Cursor `hooks.json` lifecycle (`buildCursorHookEntry`, `isManagedCursorHookEntry`, `reconcileCursorHooksJson`, `writeCursorHooksJson`, `removeCursorHooksJson`); Copilot session-hook config (`buildCopilotHookConfig`, `writeCopilotHookConfig`); Codex hook-block and event management (`buildCodexHookBlock`, `rewriteLegacyCodexHookBlock`, `reconcileCodexHooksJsonEvent`, `reconcileCodexHooksJsonSessionStart`, `ensureCodexHooksJsonSessionStart`, `ensureCodexHooksJsonEvent`, `removeCodexHooksJsonEvent`, `removeCodexHooksJsonSessionStart`, `buildCodexHookWindowsShimIR`); and shared hook command helpers (`buildHookCommand`, `rewriteLegacyManagedNodeHookCommands`, `normalizeNodePath`, `resolveNodeRunner`). `bin/install.js` delegates to this module via thin wrappers and re-exports its functions unchanged so existing tests require no modification. Source: `src/runtime-hooks-surface.cts`. Built output: `gsd-core/bin/lib/runtime-hooks-surface.cjs`. + ### Runtime Config Adapter Registry Module owning the explicit per-runtime config-mutation dispatch table for the installer. `resolveRuntimeConfigIntent(runtime)` projects a typed config intent — `installSurface` (`settings-json` | `codex-toml` | `copilot-instructions` | `cline-rules` | `cursor-hooks-json` | `profile-marker-only`), `writesSharedSettings` (the `finishInstall` shared-settings write gate), and `finishPermissionWriter` (`opencode` | `kilo` | none) — that `bin/install.js` dispatches on instead of inline `runtime === '...'` branching. Owns adapter selection only: it performs no filesystem IO and does not execute config mutations (the install/finishInstall handlers and the per-runtime writers do that). Unknown runtimes fail loudly with a `TypeError`, guarded by an `Object.hasOwn` own-property check so prototype-chain keys (`__proto__`, `constructor`) also throw. Realizes the adapter-selection half of the Runtime Install Policy Module boundary. Source: `gsd-core/bin/lib/runtime-config-adapter-registry.cjs`. See ADR-58, #60. diff --git a/bin/install.js b/bin/install.js index 5fcd0298c..a1004efa9 100755 --- a/bin/install.js +++ b/bin/install.js @@ -45,6 +45,11 @@ const { resolveRuntimeConfigIntent } = require('../gsd-core/bin/lib/runtime-conf const { HOOKS_TO_COPY: _HOOKS_TO_COPY } = require('../scripts/build-hooks.js'); const INSTALLED_HOOK_FILES = new Set(_HOOKS_TO_COPY); +// ADR-857 phase 5f-1: hook-surface writer functions extracted to a dedicated module. +// bin/install.js re-exports everything from hooksSurface so existing callers +// (require('../bin/install.js').writeCursorHooksJson etc.) continue to work. +const hooksSurface = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + /** * Runtimes that register hyphen-form `name:` per #2808 AND copy agent bodies * verbatim (only branding swaps, no namespace conversion), so retired @@ -622,215 +627,15 @@ function computePathPrefix({ isGlobal, isOpencode, isWindowsHost: _isWindowsHost return `${resolvedTarget}/`; } -/** - * Normalize a raw `process.execPath` to a stable, upgrade-safe node binary - * path. On Homebrew installs, `process.execPath` resolves symlinks and returns - * the versioned Cellar path (e.g. - * `/usr/local/Cellar/node/25.8.1/bin/node`). Baking that path into hook - * commands causes `dyld: Library not loaded` errors after `brew upgrade node` - * because the shared libraries referenced by the Cellar binary have changed - * SOVERSION. (#3181) - * - * The stable Homebrew symlinks (`/usr/local/bin/node` for Intel, - * `/opt/homebrew/bin/node` for Apple Silicon) survive upgrades — Homebrew - * re-points them atomically. We prefer those when a Cellar path is detected. - * - * Non-Homebrew installs (NVM, system node, Windows, etc.) are returned as-is. - */ -function normalizeNodePath(execPath, opts) { - if (!execPath) return execPath; - const env = (opts && opts.env) || process.env; - const existsSync = (opts && opts.existsSync) || fs.existsSync; - - // fnm multishell shim: C:/Users//AppData/Local/fnm_multishells/_/node.exe - // These are per-shell-session ephemeral directories that fnm cleans up on shell exit. - // Probe the stable fnm alias paths instead so baked hook commands survive shell restarts. - // (#977) - // TODO: Volta (~/.volta/bin/node → ~/.volta/tools/image/node//bin/node) and - // nvm-windows (AppData/Roaming/nvm//node.exe) have analogous issues — future work. - const normalizedForMatch = execPath.replace(/\\/g, '/'); - if (/\/fnm_multishells\/[0-9]+_[0-9]+\/node(\.exe)?$/i.test(normalizedForMatch)) { - const candidates = []; - if (env.FNM_DIR) { - // Preferred: alias installed directly under FNM_DIR/aliases/default/ - candidates.push(`${env.FNM_DIR}/aliases/default/node.exe`); - // POSIX layout (no .exe) - candidates.push(`${env.FNM_DIR}/aliases/default/bin/node`); - } - if (env.APPDATA) { - // Fallback: fnm default location on Windows when FNM_DIR is not set - candidates.push(`${env.APPDATA}/fnm/aliases/default/node.exe`); - } - for (const candidate of candidates) { - if (existsSync(candidate)) return candidate; - } - // No stable alias found — return raw path unchanged (graceful fallback) - return execPath; - } - - // Intel Homebrew: /usr/local/Cellar/node//bin/node - // or /usr/local/Cellar/node@20//bin/node - if (/^\/usr\/local\/Cellar\/node(@\d+)?\/[^/]+\/bin\/node(\.exe)?$/.test(execPath)) { - return '/usr/local/bin/node'; - } - // Apple Silicon Homebrew: /opt/homebrew/Cellar/node//bin/node - // or /opt/homebrew/Cellar/node@18//bin/node - if (/^\/opt\/homebrew\/Cellar\/node(@\d+)?\/[^/]+\/bin\/node(\.exe)?$/.test(execPath)) { - return '/opt/homebrew/bin/node'; - } - return execPath; -} - -/** - * Resolve the absolute path to the node binary running the installer. - * Used as the runner for .js hooks so they execute in GUI/minimal-PATH - * runtimes (Gemini, Antigravity, Codex CLIs launched from a Finder - * shortcut etc.) where bare `node` is not on `/usr/bin:/bin:/usr/sbin:/sbin` - * and the hook would fail with `node: command not found` (#2979). - * - * Returns a forward-slash-normalized, double-quoted path so the emitted - * command is shell-safe across POSIX and Windows. `process.execPath` - * gives the absolute path of the node binary actively running the - * installer — that is the version the user just installed under, and - * the right default runtime for hooks invoked under the same install. - * - * When `process.execPath` is a versioned Homebrew Cellar path, the stable - * Homebrew symlink is returned instead to survive `brew upgrade node` (#3181). - * - * When `process.execPath` is an ephemeral fnm multishell shim, the stable fnm - * alias path is returned instead so managed hook commands survive shell restarts - * (#977). An optional `opts` bag (`{ env, existsSync }`) is accepted for - * testability; production callers omit it to get real env / fs. - */ -function resolveNodeRunner(opts) { - const execPath = typeof process.execPath === 'string' ? process.execPath : ''; - if (!execPath) return null; - const stablePath = normalizeNodePath(execPath, opts); - // JSON.stringify produces a properly escaped double-quoted shell token, - // safe for paths containing spaces or unusual characters. - return JSON.stringify(stablePath.replace(/\\/g, '/')); -} - -/** - * Rewrite legacy `node .../gsd-*.js` command strings in settings.hooks to use - * the absolute Node binary path (#2979 follow-up: CR feedback on #3002). - * - * The original #2979 fix only emitted absolute paths for *newly registered* - * hooks. Pre-existing entries kept their bare `node ` prefix on reinstall, - * which left them broken under minimal-PATH GUI runtimes — exactly the - * failure mode the original fix was meant to close. This walker normalizes - * any managed-hook entry whose command starts with bare `node ` to - * `