From de25400b708a351e24df6fcb7f28fe99ee0a6191 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 2 May 2026 00:40:09 -0400 Subject: [PATCH] fix(#2979): emit absolute node path in managed hooks for GUI/minimal-PATH runtimes (#3002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2979): emit absolute node path in managed hooks for GUI/minimal-PATH runtimes Installer-emitted hook commands started with bare 'node' which works under interactive shells (nvm/Homebrew/Volta on PATH) but fails in GUI-launched runtimes that start with /usr/bin:/bin:/usr/sbin:/sbin. Every managed JS hook (gsd-check-update, gsd-statusline, gsd-context-monitor, gsd-prompt-guard, gsd-read-guard, gsd-read-injection-scanner, gsd-workflow-guard) failed with /bin/sh: node: command not found — silently disabling update checks, statusline, and security guards. Fix: new resolveNodeRunner() helper returns process.execPath (the absolute path of the Node binary running the installer) forward-slash- normalized and double-quoted. Used in: - buildHookCommand() for global installs (.js runner) - local-install code paths for all 7 managed JS hooks .sh hooks keep bare 'bash' — /bin/bash is in the POSIX standard PATH and always resolves under minimal-PATH GUI launches. Tests: bug-2979-hook-absolute-node.test.cjs parses emitted commands into { runner, hookPath } records and asserts: - resolveNodeRunner returns quoted absolute forward-slash node path - .js hooks emit absolute runner (default and portableHooks modes) - .sh hooks still emit bare 'bash' Closes #2979 * chore(#2979): add changeset fragment for PR #3002 * chore(#2979): add changeset fragment for PR #3002 * fix(#2979): resolveNodeRunner returns null on missing execPath; rewrite legacy bare-node managed hooks (CR feedback) CodeRabbit on PR #3002 caught two issues: 1. resolveNodeRunner fell back to bare 'node' when process.execPath was empty -- recreating the exact #2979 bug. Now returns null. Callers (buildHookCommand and the local-install code paths) check for null and skip registration rather than emit a broken command. 2. The original #2979 fix only updated NEWLY registered hooks. Existing bare-node managed hook entries from pre-#2979 installs stayed broken across reinstalls. New rewriteLegacyManagedNodeHookCommands walks settings.hooks and rewrites any managed-hook entry that starts with bare 'node ' to use the absolute runner. Filename allowlist (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-workflow-guard.js) ensures user-authored bare-node hooks are left untouched. Tests: bug-2979-hook-absolute-node.test.cjs grows by 8 cases: - 5 for the migration walker (rewrites managed entries, leaves quoted- runner entries alone, leaves user-authored entries alone, leaves .sh entries alone, no-ops on null runner). - 2 for resolveNodeRunner returning null on empty execPath. - 1 for buildHookCommand returning null when execPath unavailable. * chore(#3002): drop direct CHANGELOG.md edit; release entry now lives in .changeset/ The changeset-fragment workflow (#2975) renders fragments into CHANGELOG.md at release time. Direct edits to [Unreleased] on each PR caused merge conflicts on every concurrent PR. This commit restores CHANGELOG.md to match origin/main; the release entry for this fix is preserved in the .changeset/*.md fragment(s) on this branch, which the release workflow consolidates. * fix(#2979): guard hook + statusline pushes against null commands (CR follow-up) CodeRabbit on PR #3002 found an outside-diff issue: when resolveNodeRunner() returns null, every dependent *Command becomes null, but the registration sites still pushed { type: 'command', command: null } entries onto settings.hooks. The runtime's hook schema rejects null commands and the failure surfaces as a confusing parse error. Fix: - One unified warning at the top of configureSettings when ANY JS-hook command resolves null (operator sees the cause once instead of per-hook). - Each of the 6 managed JS hook registration if-clauses now guards on the *Command variable being truthy: && updateCheckCommand, && contextMonitorCommand, && promptGuardCommand, && readGuardCommand, && readInjectionScannerCommand, && workflowGuardCommand. - Statusline registration adds an else-if (!statuslineCommand) clause with its own warn before the settings.statusLine write site. Tests: bug-2979-hook-absolute-node.test.cjs grows by 7 cases (6 per-hook structural assertions parsing install.js for the `fs.existsSync() && ` shape, plus 1 statusline guard-precedes-write test). * fix(#2979): defense-in-depth validateHookFields before writeSettings (CR) CodeRabbit on PR #3002 (post-fix-up review): replace source-grep structural tests with behavioral assertions on the settings object. The push-site `&& ` guards (commit ce696c64) prevent null commands from being pushed in the first place. As a defense-in-depth backstop, install.js now runs validateHookFields(settings) right before writeSettings(); validateHookFields already filters {type:'command', command: null} entries (line 5884), so even if my push-site guards ever regress, no null-command entries reach disk. Tests: replaced the 7 install.js source-grep tests with 8 truly behavioral tests: - validateHookFields strips null-command entries for each of the 6 managed JS hook shapes (parameterized by event + matcher) - validateHookFields drops the entry entirely when all its hooks are null-command - validateHookFields preserves agent-type hooks while stripping null-command sibling hooks in the same entry These tests exercise the actual function the production code uses, not its source representation. They survive future refactors of the registration call sites. * fix(#2979): tighten managed-hook migration to basename equality (CR) CodeRabbit on PR #3002 (post-fix-up review): the previous `trimmed.includes(name)` matcher had a false-positive vector. A user-authored hook whose path contained a managed filename as a substring (e.g. /home/me/scripts/wraps-gsd-check-update.js-helper.js) would be unconditionally rewritten with the GSD runner, replacing the user's bare `node` with our absolute path -- silently mutating their hook configuration. Fix: parse the command into with the script-token allowed to be quoted (single or double) or bareword. Extract the path inside quotes, take the basename (handles both forward and backslash separators on Windows), and match against MANAGED_HOOK_FILES via Set.has() — exact equality, not substring. Tests: bug-2979 grows by 4 cases: - user hook with managed-filename-as-substring is NOT rewritten - single-quoted path: rewritten correctly - bareword path: rewritten correctly - Windows backslash path: basename extraction works --- .changeset/jolly-pumas-dance.md | 5 + .changeset/zesty-jays-wake.md | 5 + bin/install.js | 165 ++++++++- tests/bug-2979-hook-absolute-node.test.cjs | 385 +++++++++++++++++++++ 4 files changed, 544 insertions(+), 16 deletions(-) create mode 100644 .changeset/jolly-pumas-dance.md create mode 100644 .changeset/zesty-jays-wake.md create mode 100644 tests/bug-2979-hook-absolute-node.test.cjs diff --git a/.changeset/jolly-pumas-dance.md b/.changeset/jolly-pumas-dance.md new file mode 100644 index 000000000..28b09701e --- /dev/null +++ b/.changeset/jolly-pumas-dance.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2979 +--- +Managed JS hooks now resolve under GUI/minimal-PATH runtimes — installer emits process.execPath (absolute, quoted, forward-slash-normalized) as the runner for every .js hook command instead of bare node. See #2979. diff --git a/.changeset/zesty-jays-wake.md b/.changeset/zesty-jays-wake.md new file mode 100644 index 000000000..28b09701e --- /dev/null +++ b/.changeset/zesty-jays-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2979 +--- +Managed JS hooks now resolve under GUI/minimal-PATH runtimes — installer emits process.execPath (absolute, quoted, forward-slash-normalized) as the runner for every .js hook command instead of bare node. See #2979. diff --git a/bin/install.js b/bin/install.js index 390f912d5..1813274d8 100755 --- a/bin/install.js +++ b/bin/install.js @@ -525,6 +525,82 @@ function computePathPrefix({ isGlobal, isOpencode, isWindowsHost: _isWindowsHost return `${resolvedTarget}/`; } +/** + * 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. + */ +function resolveNodeRunner() { + const execPath = typeof process.execPath === 'string' ? process.execPath : ''; + if (!execPath) return null; + // JSON.stringify produces a properly escaped double-quoted shell token, + // safe for paths containing spaces or unusual characters. + return JSON.stringify(execPath.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 + * `