* 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(<file>) && <command>` 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 `&& <command>` 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 <runner> <script-token> 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
This commit is contained in:
Tom Boucher
165
bin/install.js
165
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
|
||||
* `<absoluteRunner> <script>` while leaving non-managed and non-bare-node
|
||||
* entries (user-authored hooks, shell scripts, etc.) untouched.
|
||||
*
|
||||
* Returns true if any entry was rewritten.
|
||||
*/
|
||||
function rewriteLegacyManagedNodeHookCommands(settings, absoluteRunner) {
|
||||
if (!settings || !settings.hooks || !absoluteRunner) return false;
|
||||
const MANAGED_HOOK_FILES = 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-workflow-guard.js',
|
||||
]);
|
||||
let changed = false;
|
||||
for (const entries of Object.values(settings.hooks)) {
|
||||
if (!Array.isArray(entries)) continue;
|
||||
for (const entry of entries) {
|
||||
if (!entry || !Array.isArray(entry.hooks)) continue;
|
||||
for (const h of entry.hooks) {
|
||||
if (!h || typeof h.command !== 'string') continue;
|
||||
const trimmed = h.command.trim();
|
||||
// Match the EXACT legacy form: `node <script>` with optional quoting.
|
||||
// The previous shape used `trimmed.includes(<filename>)` which would
|
||||
// false-positive on user-authored hooks whose path merely contained
|
||||
// a managed filename as a substring (e.g.
|
||||
// /home/me/scripts/wraps-gsd-check-update.js-and-more.js). #3002 CR.
|
||||
const m = trimmed.match(/^node\s+("([^"]+)"|'([^']+)'|(\S+))\s*$/);
|
||||
if (!m) continue;
|
||||
const scriptToken = m[1];
|
||||
const scriptPath = m[2] || m[3] || m[4] || '';
|
||||
// Take the basename — match against MANAGED_HOOK_FILES by exact
|
||||
// equality, not substring containment. Handles both forward and
|
||||
// backslash separators (Windows).
|
||||
const scriptBase = scriptPath.split(/[\\/]/).pop() || '';
|
||||
if (!MANAGED_HOOK_FILES.has(scriptBase)) continue;
|
||||
h.command = `${absoluteRunner} ${scriptToken}`;
|
||||
changed = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return changed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a hook command path using forward slashes for cross-platform compatibility.
|
||||
* On Windows, $HOME is not expanded by cmd.exe/PowerShell, so we use the actual path.
|
||||
@@ -538,7 +614,16 @@ function computePathPrefix({ isGlobal, isOpencode, isWindowsHost: _isWindowsHost
|
||||
*/
|
||||
function buildHookCommand(configDir, hookName, opts) {
|
||||
if (!opts) opts = {};
|
||||
const runner = hookName.endsWith('.sh') ? 'bash' : 'node';
|
||||
// .sh hooks run under /bin/bash (POSIX std PATH always includes /bin),
|
||||
// so bare `bash` is fine. .js hooks need the absolute node path because
|
||||
// GUI-launched runtimes start with a minimal PATH that does not include
|
||||
// nvm/Homebrew/Volta-installed node binaries (#2979).
|
||||
const nodeRunner = resolveNodeRunner();
|
||||
const runner = hookName.endsWith('.sh') ? 'bash' : nodeRunner;
|
||||
// resolveNodeRunner returns null when process.execPath is unavailable.
|
||||
// Fall through with null so callers can skip registration with a warning
|
||||
// instead of emitting bare `node` (which would recreate the #2979 bug).
|
||||
if (runner === null) return null;
|
||||
|
||||
if (opts.portableHooks) {
|
||||
// Replace the home directory prefix with $HOME so the path works when
|
||||
@@ -7977,6 +8062,14 @@ function install(isGlobal, runtime = 'claude') {
|
||||
return;
|
||||
}
|
||||
const settings = validateHookFields(cleanupOrphanedHooks(rawSettings));
|
||||
// #3002 CR: rewrite legacy `node .../gsd-*.js` command strings carried over
|
||||
// from pre-#2979 installs to use the absolute node binary path. Without this,
|
||||
// existing managed hook entries stay bare-`node`-prefixed across reinstalls
|
||||
// and remain broken under GUI/minimal-PATH runtimes.
|
||||
const settingsRunner = resolveNodeRunner();
|
||||
if (settingsRunner && rewriteLegacyManagedNodeHookCommands(settings, settingsRunner)) {
|
||||
console.log(` ${green}✓${reset} Rewrote legacy bare-node managed-hook commands to absolute path (#2979)`);
|
||||
}
|
||||
// Local installs anchor hook paths so they resolve regardless of cwd (#1906).
|
||||
// Claude Code sets $CLAUDE_PROJECT_DIR; Gemini/Antigravity do not — and on
|
||||
// Windows their own substitution logic doubles the path (#2557). Those runtimes
|
||||
@@ -7985,24 +8078,50 @@ function install(isGlobal, runtime = 'claude') {
|
||||
? dirName
|
||||
: '"$CLAUDE_PROJECT_DIR"/' + dirName;
|
||||
const hookOpts = { portableHooks: hasPortableHooks };
|
||||
// #2979: local-install hook commands also use the absolute node path so
|
||||
// GUI/minimal-PATH runtimes can resolve them. Bare `node` fails when the
|
||||
// host launches the runtime with a stripped PATH (Finder/Antigravity/etc).
|
||||
const localNodeRunner = resolveNodeRunner();
|
||||
// If we cannot resolve an absolute node path AND this is a local install,
|
||||
// skip managed-hook registration. Returning null from buildHookCommand on
|
||||
// global installs has the same effect. Better to skip than to emit a bare
|
||||
// `node` command that recreates the #2979 failure.
|
||||
const localCmd = (hookFile) => localNodeRunner === null
|
||||
? null
|
||||
: localNodeRunner + ' ' + localPrefix + '/hooks/' + hookFile;
|
||||
const statuslineCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-statusline.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-statusline.js';
|
||||
: localCmd('gsd-statusline.js');
|
||||
const updateCheckCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-check-update.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-check-update.js';
|
||||
: localCmd('gsd-check-update.js');
|
||||
const contextMonitorCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-context-monitor.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-context-monitor.js';
|
||||
: localCmd('gsd-context-monitor.js');
|
||||
const promptGuardCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-prompt-guard.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-prompt-guard.js';
|
||||
: localCmd('gsd-prompt-guard.js');
|
||||
const readGuardCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-read-guard.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-read-guard.js';
|
||||
: localCmd('gsd-read-guard.js');
|
||||
const readInjectionScannerCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-read-injection-scanner.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-read-injection-scanner.js';
|
||||
: localCmd('gsd-read-injection-scanner.js');
|
||||
|
||||
// #3002 CR: when resolveNodeRunner() returns null, every dependent JS-hook
|
||||
// command is null too. Emit one warning here so the operator sees the cause
|
||||
// ONCE instead of per-hook. Each registration site below also guards on its
|
||||
// own *Command variable being truthy, so we never write `command: null`
|
||||
// entries to settings.json (which the runtime's hook schema would reject).
|
||||
const anyJsHookCommandNull = !statuslineCommand
|
||||
|| !updateCheckCommand
|
||||
|| !contextMonitorCommand
|
||||
|| !promptGuardCommand
|
||||
|| !readGuardCommand
|
||||
|| !readInjectionScannerCommand;
|
||||
if (anyJsHookCommandNull) {
|
||||
console.warn(` ${yellow}⚠${reset} Skipping managed JS hook registration — Node executable path unavailable (process.execPath is empty). See #2979 / #3002.`);
|
||||
}
|
||||
|
||||
// Enable experimental agents for Gemini CLI (required for custom sub-agents)
|
||||
if (isGemini) {
|
||||
@@ -8033,7 +8152,7 @@ function install(isGlobal, runtime = 'claude') {
|
||||
// copy step produces no files but the registration step ran unconditionally,
|
||||
// causing "hook error" on every tool invocation.
|
||||
const checkUpdateFile = path.join(targetDir, 'hooks', 'gsd-check-update.js');
|
||||
if (!hasGsdUpdateHook && fs.existsSync(checkUpdateFile)) {
|
||||
if (!hasGsdUpdateHook && fs.existsSync(checkUpdateFile) && updateCheckCommand) {
|
||||
settings.hooks.SessionStart.push({
|
||||
hooks: [
|
||||
{
|
||||
@@ -8057,7 +8176,7 @@ function install(isGlobal, runtime = 'claude') {
|
||||
);
|
||||
|
||||
const contextMonitorFile = path.join(targetDir, 'hooks', 'gsd-context-monitor.js');
|
||||
if (!hasContextMonitorHook && fs.existsSync(contextMonitorFile)) {
|
||||
if (!hasContextMonitorHook && fs.existsSync(contextMonitorFile) && contextMonitorCommand) {
|
||||
settings.hooks[postToolEvent].push({
|
||||
matcher: 'Bash|Edit|Write|MultiEdit|Agent|Task',
|
||||
hooks: [
|
||||
@@ -8105,7 +8224,7 @@ function install(isGlobal, runtime = 'claude') {
|
||||
);
|
||||
|
||||
const promptGuardFile = path.join(targetDir, 'hooks', 'gsd-prompt-guard.js');
|
||||
if (!hasPromptGuardHook && fs.existsSync(promptGuardFile)) {
|
||||
if (!hasPromptGuardHook && fs.existsSync(promptGuardFile) && promptGuardCommand) {
|
||||
settings.hooks[preToolEvent].push({
|
||||
matcher: 'Write|Edit',
|
||||
hooks: [
|
||||
@@ -8129,7 +8248,7 @@ function install(isGlobal, runtime = 'claude') {
|
||||
);
|
||||
|
||||
const readGuardFile = path.join(targetDir, 'hooks', 'gsd-read-guard.js');
|
||||
if (!hasReadGuardHook && fs.existsSync(readGuardFile)) {
|
||||
if (!hasReadGuardHook && fs.existsSync(readGuardFile) && readGuardCommand) {
|
||||
settings.hooks[preToolEvent].push({
|
||||
matcher: 'Write|Edit',
|
||||
hooks: [
|
||||
@@ -8153,7 +8272,7 @@ function install(isGlobal, runtime = 'claude') {
|
||||
);
|
||||
|
||||
const readInjectionScannerFile = path.join(targetDir, 'hooks', 'gsd-read-injection-scanner.js');
|
||||
if (!hasReadInjectionScannerHook && fs.existsSync(readInjectionScannerFile)) {
|
||||
if (!hasReadInjectionScannerHook && fs.existsSync(readInjectionScannerFile) && readInjectionScannerCommand) {
|
||||
settings.hooks[postToolEvent].push({
|
||||
matcher: 'Read',
|
||||
hooks: [
|
||||
@@ -8179,13 +8298,13 @@ function install(isGlobal, runtime = 'claude') {
|
||||
// /gsd-quick or /gsd-fast for state-tracked changes. Advisory only.
|
||||
const workflowGuardCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-workflow-guard.js', hookOpts)
|
||||
: 'node ' + localPrefix + '/hooks/gsd-workflow-guard.js';
|
||||
: localCmd('gsd-workflow-guard.js');
|
||||
const hasWorkflowGuardHook = settings.hooks[preToolEvent].some(entry =>
|
||||
entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-workflow-guard'))
|
||||
);
|
||||
|
||||
const workflowGuardFile = path.join(targetDir, 'hooks', 'gsd-workflow-guard.js');
|
||||
if (!hasWorkflowGuardHook && fs.existsSync(workflowGuardFile)) {
|
||||
if (!hasWorkflowGuardHook && fs.existsSync(workflowGuardFile) && workflowGuardCommand) {
|
||||
settings.hooks[preToolEvent].push({
|
||||
matcher: 'Write|Edit',
|
||||
hooks: [
|
||||
@@ -8298,6 +8417,11 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS
|
||||
// any profile-level statusLine the user has configured (#2248).
|
||||
// Pass --force-statusline to override this guard.
|
||||
console.log(` ${yellow}⚠${reset} Skipping statusLine for local install (avoids overriding profile-level settings; use --force-statusline to override)`);
|
||||
} else if (!statuslineCommand) {
|
||||
// #3002 CR: don't write { type: 'command', command: null } — the
|
||||
// runtime's settings schema rejects null commands and the failure
|
||||
// surfaces as a confusing parse error rather than a usable diagnostic.
|
||||
console.warn(` ${yellow}⚠${reset} Skipped statusline registration — Node executable path unavailable (process.execPath is empty). See #2979 / #3002.`);
|
||||
} else {
|
||||
settings.statusLine = {
|
||||
type: 'command',
|
||||
@@ -8307,9 +8431,15 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS
|
||||
}
|
||||
}
|
||||
|
||||
// Write settings when runtime supports settings.json
|
||||
// Write settings when runtime supports settings.json.
|
||||
// #3002 CR: defense-in-depth — re-run validateHookFields right before
|
||||
// serialization. The push-site guards above already skip null-command
|
||||
// entries, but a future regression that bypasses them would still produce
|
||||
// {type: 'command', command: null} items that the runtime hook schema
|
||||
// rejects at parse time. validateHookFields filters those out so the file
|
||||
// we write is always schema-valid.
|
||||
if (!isCodex && !isCopilot && !isKilo && !isCursor && !isWindsurf && !isTrae && !isCline) {
|
||||
writeSettings(settingsPath, settings);
|
||||
writeSettings(settingsPath, validateHookFields(settings));
|
||||
}
|
||||
|
||||
// Configure OpenCode permissions
|
||||
@@ -9307,6 +9437,9 @@ if (process.env.GSD_TEST_MODE) {
|
||||
allRuntimes,
|
||||
parseRuntimeInput,
|
||||
buildRuntimePromptText,
|
||||
buildHookCommand,
|
||||
resolveNodeRunner,
|
||||
rewriteLegacyManagedNodeHookCommands,
|
||||
};
|
||||
} else {
|
||||
|
||||
|
||||
Reference in New Issue
Block a user