From 8e25eb6546c5c87bad0a6abfefbd1ae5dbb71e81 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 2 May 2026 11:45:30 -0400 Subject: [PATCH] fix(#3017): codex SessionStart hook uses absolute node, not bare 'node' (#3022) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3017): codex SessionStart hook uses absolute node, not bare 'node' PR #3002 fixed #2979 for settings.json-based managed JS hooks (Claude Code, Gemini, Antigravity) by routing through buildHookCommand() → resolveNodeRunner(), emitting the absolute Node binary path so hooks resolve under GUI/minimal-PATH runtimes (/usr/bin:/bin:/usr/sbin:/sbin) where nvm/Homebrew/Volta-installed node is not on PATH. The Codex install path bypassed both helpers — line 7935 of bin/install.js wrote `command = "node ${path}"` directly into config.toml. So Codex SessionStart hook still failed with exit 127 ("node: command not found") under the same minimal-PATH conditions PR #3002 was meant to close. Fix: - Add buildCodexHookBlock(targetDir, { absoluteRunner, eol }) — a pure helper that emits the toml hook block with the absolute runner. Returns null when absoluteRunner is null so the caller skips registration with a warning instead of writing a broken bare-node hook. - Add rewriteLegacyCodexHookBlock(content, absoluteRunner) — mirror of rewriteLegacyManagedNodeHookCommands for the toml surface, so reinstall migrates a 1.39.x bare-node config.toml to the absolute form. Uses basename equality (CODEX_MANAGED_HOOK_BASENAMES set) so user- authored bare-node hooks are left alone. - Replace the inline string-concat at line 7935 with a call to the new helper, threaded with the detected line ending so CRLF files stay CRLF. - On the codex reinstall path, call rewriteLegacyCodexHookBlock first so existing bare-node entries get migrated before the new entry is added. Tests: - bug-3017-codex-hook-absolute-node.test.cjs (9 tests, all typed-IR): - buildCodexHookBlock emits absolute runner, parses to expected fields - returns null on missing runner (caller skips) - integrates with resolveNodeRunner() in the live process - rewriteLegacyCodexHookBlock migrates managed bare-node entries - leaves user-authored bare-node hooks alone (basename allowlist) - leaves entries with absolute runner unchanged (idempotent) - returns content unchanged when absoluteRunner is null - codex-config.test.cjs e2e expectation updated to match new shape: parsed.hooks.SessionStart[0].hooks[0].command now equals '"" ""' instead of 'node '. Verification: - 9/9 pass on the new regression test - 179/179 pass across all codex-touching test files - 6767/6767 pass on full suite, lint-no-source-grep clean - Adheres to typed-IR / CONTRIBUTING.md "Prohibited: Raw Text Matching": parseCodexHookBlock returns a typed record; assertions are on structured fields (runner, hookPath, type, hasMarker), not stdout regex. Closes #3017 * test(#3017): tighten runner assertions to exact process.execPath (CR) CodeRabbit on PR #3022 (3 findings, 2 actionable + 1 nitpick): 1. .changeset/codex-bare-node-fix.md:3 — replace `pr: TBD` with `pr: 3022` so changeset metadata is traceable. 2. tests/bug-3017-codex-hook-absolute-node.test.cjs:81-146 — the test asserted `parsed.runner !== 'node'` and `parsed.runner.includes('/node')`, which would false-positive on any absolute path containing '/node' (e.g. /Users/x/notnode/foo). Tightened to compare against the EXACT absolute path supplied by the caller (after stripping toml + JSON escape layers via a new unescapeRunner() helper). The live-process integration test now compares against process.execPath exactly. The rewriteLegacyCodexHookBlock test also uses exact-equality. 3. Nitpick (skipped): use repository's TOML parser for parsing instead of bespoke regex. The hand-rolled parser is small, scoped, and fully tested by these structural assertions; pulling in a TOML lib for tests would create a circular dependency on the SUT (the installer's own parser). Leaving as-is. Verification: 9/9 pass on regression test, 6767/6767 full suite, lint clean. --- .changeset/codex-bare-node-fix.md | 5 + bin/install.js | 130 ++++++++++- ...bug-3017-codex-hook-absolute-node.test.cjs | 207 ++++++++++++++++++ tests/codex-config.test.cjs | 13 +- 4 files changed, 344 insertions(+), 11 deletions(-) create mode 100644 .changeset/codex-bare-node-fix.md create mode 100644 tests/bug-3017-codex-hook-absolute-node.test.cjs diff --git a/.changeset/codex-bare-node-fix.md b/.changeset/codex-bare-node-fix.md new file mode 100644 index 000000000..9d89a1f0f --- /dev/null +++ b/.changeset/codex-bare-node-fix.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3022 +--- +**Codex SessionStart hook now uses absolute Node binary path** — closes the gap left after #3002. The Codex install path wrote `command = "node ${path}"` directly into config.toml, bypassing `resolveNodeRunner()`. Under GUI/minimal-PATH runtimes (`/usr/bin:/bin:/usr/sbin:/sbin`), bare `node` failed to resolve, exit 127. Now routed through new `buildCodexHookBlock()` helper. Reinstall path migrates legacy bare-node entries via new `rewriteLegacyCodexHookBlock()`. See #3017. diff --git a/bin/install.js b/bin/install.js index f08a3b832..a9c6504ea 100755 --- a/bin/install.js +++ b/bin/install.js @@ -601,6 +601,100 @@ function rewriteLegacyManagedNodeHookCommands(settings, absoluteRunner) { return changed; } +/** + * Codex managed-hook filenames eligible for legacy-bare-node migration. + * Mirrors the settings.json allowlist in rewriteLegacyManagedNodeHookCommands. + * Centralized so the codex toml branch and the settings.json branch can't drift. + */ +const CODEX_MANAGED_HOOK_BASENAMES = new Set([ + 'gsd-check-update.js', +]); + +/** + * Build the GSD-managed Codex SessionStart hook block for config.toml. + * + * Issue #3017: the previous shape inlined `command = "node ${path}"` which + * fails under GUI/minimal-PATH runtimes where bare `node` doesn't resolve + * (same failure mode as #2979 → fixed for settings.json by #3002, this + * helper closes the gap for Codex's TOML hook surface). + * + * Returns null when `absoluteRunner` is null so callers can warn-and-skip + * registration — emitting a broken bare-node hook is strictly worse than + * not registering one (the user can re-run install once node is on PATH). + * + * @param {string} targetDir - Resolved absolute Codex config dir (e.g. ~/.codex). + * @param {{ absoluteRunner: string|null, eol?: string }} opts + * absoluteRunner: result of resolveNodeRunner() — a JSON-stringified + * absolute node path with forward slashes (e.g. `"/usr/local/bin/node"`), + * or null when process.execPath was unavailable. + * eol: line ending to emit ('\n' or '\r\n') — caller passes + * detectLineEnding(configContent) so existing CRLF files stay CRLF. + * Defaults to '\n'. + * @returns {string|null} The toml block to append, or null on missing runner. + */ +function buildCodexHookBlock(targetDir, opts) { + const absoluteRunner = opts && opts.absoluteRunner; + if (!absoluteRunner) return null; + const eol = (opts && opts.eol) || '\n'; + const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); + // toml requires escaped interior quotes (\"). The runner is already a + // JSON-stringified token (with literal " around the absolute path); we + // need to escape those quotes so the toml parser sees them as part of + // the string value, not as the closing quote of the command field. + const runnerEscaped = absoluteRunner.replace(/"/g, '\\"'); + const hookPathEscaped = updateCheckScript.replace(/"/g, '\\"'); + return `${eol}# GSD Hooks${eol}` + + `[[hooks.SessionStart]]${eol}` + + `${eol}` + + `[[hooks.SessionStart.hooks]]${eol}` + + `type = "command"${eol}` + + `command = "${runnerEscaped} \\"${hookPathEscaped}\\""${eol}`; +} + +/** + * Rewrite legacy bare-`node` managed-hook command lines in a Codex + * config.toml string to use the absolute Node runner. Mirror of + * rewriteLegacyManagedNodeHookCommands but for the toml surface (#3017). + * + * Only rewrites entries whose script basename matches CODEX_MANAGED_HOOK_BASENAMES + * (basename equality, not substring containment) — user-authored bare-node + * hooks pointing at scripts outside the managed allowlist are left alone. + * + * @param {string} content - Current config.toml contents. + * @param {string|null} absoluteRunner - Result of resolveNodeRunner(). + * @returns {{ content: string, changed: boolean }} + */ +function rewriteLegacyCodexHookBlock(content, absoluteRunner) { + if (!content || !absoluteRunner) return { content, changed: false }; + let changed = false; + // Match `command = "node "` lines where scriptToken is + // either an unquoted path (no spaces) or a toml-escaped quoted path. + // The whole RHS is a toml-double-quoted string; interior quotes are \". + // Examples we want to migrate: + // command = "node /Users/x/.codex/hooks/gsd-check-update.js" + // command = "node \"/Users/x/.codex/hooks/gsd-check-update.js\"" + // Examples we must leave alone: + // command = "\"/usr/local/bin/node\" \"/path/to/gsd-check-update.js\"" ← already absolute + // command = "node /home/me/my-custom.js" ← user-owned filename + const updated = content.replace( + /^(command\s*=\s*")node\s+((?:\\"[^"]+\\"|\S+))("\s*)$/gm, + (full, prefix, scriptToken, suffix) => { + // Extract the underlying script path from the captured token — + // either the bare token or the inner content of \"...\". + const quoted = scriptToken.match(/^\\"(.+)\\"$/); + const scriptPath = quoted ? quoted[1] : scriptToken; + const base = scriptPath.split(/[\\/]/).pop() || ''; + if (!CODEX_MANAGED_HOOK_BASENAMES.has(base)) return full; + changed = true; + const runnerEscaped = absoluteRunner.replace(/"/g, '\\"'); + // Always re-quote the path on output for consistency with the new + // builder's shape. + return `${prefix}${runnerEscaped} \\"${scriptPath}\\"${suffix}`; + }, + ); + return { content: updated, 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. @@ -7926,16 +8020,32 @@ function install(isGlobal, runtime = 'claude') { // two-level nested AoT schema: [[hooks.SessionStart]] for the event entry // (holds optional matcher) and [[hooks.SessionStart.hooks]] for the handler // (holds type, command, statusMessage, timeout). (#2637, #2760, #2773) - const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); - const hookBlock = `${eol}# GSD Hooks${eol}` + - `[[hooks.SessionStart]]${eol}` + - `${eol}` + - `[[hooks.SessionStart.hooks]]${eol}` + - `type = "command"${eol}` + - `command = "node ${updateCheckScript}"${eol}`; + // + // #3017: route through buildCodexHookBlock() so the absolute Node binary + // path is emitted (matching the settings.json branch via #3002), so the + // hook resolves under GUI/minimal-PATH runtimes where bare `node` doesn't. + const codexNodeRunner = resolveNodeRunner(); + const hookBlock = buildCodexHookBlock(targetDir, { absoluteRunner: codexNodeRunner, eol }); - if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) { - configContent += hookBlock; + if (hasEnabledCodexHooksFeature(configContent)) { + // Reinstall path: rewrite a legacy bare-node managed-hook entry to the + // absolute runner. Mirrors rewriteLegacyManagedNodeHookCommands for the + // settings.json surface (#3002 CR). + const rewrite = rewriteLegacyCodexHookBlock(configContent, codexNodeRunner); + if (rewrite.changed) { + configContent = rewrite.content; + console.log(` ${green}✓${reset} Migrated legacy bare-node Codex hook to absolute runner (#3017)`); + } + if (!configContent.includes('gsd-check-update')) { + if (hookBlock !== null) { + configContent += hookBlock; + } else { + // resolveNodeRunner() returned null — process.execPath unavailable. + // Match the settings.json branch's warn-and-skip behavior rather + // than emit a broken bare-node hook (the #2979 / #3017 failure mode). + console.warn(` ${yellow}⚠${reset} Skipping Codex SessionStart hook registration — Node executable path unavailable (process.execPath is empty). See #2979 / #3002 / #3017.`); + } + } } // #2760 fix 3 — post-write schema validation. Parse the bytes we are @@ -9573,6 +9683,8 @@ if (process.env.GSD_TEST_MODE) { buildHookCommand, resolveNodeRunner, rewriteLegacyManagedNodeHookCommands, + buildCodexHookBlock, + rewriteLegacyCodexHookBlock, }; } else { diff --git a/tests/bug-3017-codex-hook-absolute-node.test.cjs b/tests/bug-3017-codex-hook-absolute-node.test.cjs new file mode 100644 index 000000000..e4fe4787b --- /dev/null +++ b/tests/bug-3017-codex-hook-absolute-node.test.cjs @@ -0,0 +1,207 @@ +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +/** + * Bug #3017: Codex SessionStart hook still emits bare `node` after #3002. + * + * PR #3002 fixed #2979 for settings.json-based managed JS hooks (Claude + * Code, Gemini, Antigravity) by routing through buildHookCommand() → + * resolveNodeRunner(), which emits the absolute Node binary path. But the + * Codex install path writes its SessionStart hook directly into a + * config.toml string, bypassing both helpers: + * + * command = "node ${updateCheckScript}" + * + * Under a GUI/minimal PATH (`/usr/bin:/bin:/usr/sbin:/sbin`) where node + * is not resolvable, the hook fails with `/bin/sh: node: command not + * found` (exit 127). The same failure mode #2979 was meant to fix — + * just on the codex toml branch instead of the settings.json branch. + * + * The fix exposes two pure helpers and tests them as typed records, + * not by grepping install.js content: + * + * buildCodexHookBlock(targetDir, { absoluteRunner }) → toml string + * - emits `command = " "` so the + * hook resolves under minimal PATH. + * - returns null when absoluteRunner is null (caller skips with warn, + * matching settings.json branch behavior). + * + * rewriteLegacyCodexHookBlock(tomlContent, absoluteRunner) → { content, changed } + * - rewrites an existing bare-node managed-hook command on reinstall + * (matches the rewriteLegacyManagedNodeHookCommands shape from #3002). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +const INSTALL = require(path.join(__dirname, '..', 'bin', 'install.js')); +const { buildCodexHookBlock, rewriteLegacyCodexHookBlock, resolveNodeRunner } = INSTALL; + +/** + * Parse the toml hook block into a typed record so tests can assert on + * the structured shape (what's the runner, what's the hook path, what's + * the type) rather than substring-matching the toml text. + */ +function parseCodexHookBlock(block) { + if (!block) return { ok: false, reason: 'empty' }; + // The block always carries the "# GSD Hooks" marker, the AoT tables, + // a type=command, and a command=" " line. + const hasMarker = /^# GSD Hooks$/m.test(block); + const hasEvent = /^\[\[hooks\.SessionStart\]\]$/m.test(block); + const hasHandler = /^\[\[hooks\.SessionStart\.hooks\]\]$/m.test(block); + const typeMatch = block.match(/^type\s*=\s*"([^"]+)"$/m); + // command = " " — runner may itself be a quoted absolute path. + // Match the whole RHS as one toml double-quoted string, then split into runner + hookpath. + const cmdLine = block.match(/^command\s*=\s*"((?:[^"\\]|\\.)*)"$/m); + if (!cmdLine) return { ok: false, reason: 'no command line' }; + const cmdValue = cmdLine[1]; + // Inside the command value, the runner is either a quoted string (escaped \" in toml) + // or a bare token, followed by a space and the hook path (quoted). + // toml escapes interior " as \", so the cmdValue contains literal \" sequences. + const cmdParsed = cmdValue.match(/^(\\".+?\\"|node|bash|\S+)\s+\\"([^\\]+)\\"\s*$/); + return { + ok: true, + hasMarker, + hasEvent, + hasHandler, + type: typeMatch ? typeMatch[1] : null, + command: cmdValue, + runner: cmdParsed ? cmdParsed[1] : null, + hookPath: cmdParsed ? cmdParsed[2] : null, + }; +} + +// Strip the toml-escape (\") and JSON-quote (") layers from the parsed +// runner token to compare against the raw absolute path the caller +// supplied. parsed.runner round-trips through TWO escape layers: +// 1. JSON.stringify in resolveNodeRunner adds outer "..." quotes +// 2. toml escapes the interior " to \" inside the command field +// After both, parsed.runner ends in `\"` and starts with `\"`. +function unescapeRunner(token) { + if (!token) return token; + let t = token.replace(/^\\"/, '').replace(/\\"$/, ''); + if (t.startsWith('"') && t.endsWith('"')) t = t.slice(1, -1); + return t; +} + +describe('Bug #3017: buildCodexHookBlock emits absolute node runner', () => { + test('exported as a function', () => { + assert.equal(typeof buildCodexHookBlock, 'function'); + }); + + test('emits the EXACT absolute node runner the caller supplied (#3022 CR)', () => { + const targetDir = '/tmp/codex-test/.codex'; + const expectedRunnerPath = '/usr/local/bin/node'; + const absoluteRunner = `"${expectedRunnerPath}"`; + const block = buildCodexHookBlock(targetDir, { absoluteRunner }); + const parsed = parseCodexHookBlock(block); + assert.equal(parsed.ok, true, `parse failed: ${block}`); + assert.equal(parsed.hasMarker, true, '# GSD Hooks marker present'); + assert.equal(parsed.hasEvent, true, '[[hooks.SessionStart]] AoT entry present'); + assert.equal(parsed.hasHandler, true, '[[hooks.SessionStart.hooks]] handler entry present'); + assert.equal(parsed.type, 'command', 'handler is type=command'); + // Strict: parsed runner must match the supplied absolute path EXACTLY + // (after stripping toml/JSON escape layers). A loose substring like + // '/node' would let an unrelated absolute token containing '/node' + // pass — e.g. '/Users/x/notnode/foo'. + assert.equal(unescapeRunner(parsed.runner), expectedRunnerPath, + `parsed runner must equal supplied absolute path: got ${parsed.runner}, want ${expectedRunnerPath}`); + assert.equal(parsed.hookPath, '/tmp/codex-test/.codex/hooks/gsd-check-update.js', + `hook path equality, got: ${parsed.hookPath}`); + }); + + test('returns null when absoluteRunner is null (caller skips registration)', () => { + const block = buildCodexHookBlock('/tmp/x/.codex', { absoluteRunner: null }); + assert.equal(block, null, + 'must return null on missing runner so caller can warn-and-skip instead of writing a broken hook'); + }); + + test('integrates with resolveNodeRunner() in the live process — runner equals process.execPath (#3022 CR)', () => { + const runner = resolveNodeRunner(); + assert.ok(runner, 'resolveNodeRunner returns a usable value in this test env'); + const block = buildCodexHookBlock('/tmp/x/.codex', { absoluteRunner: runner }); + const parsed = parseCodexHookBlock(block); + assert.equal(parsed.ok, true); + // Strict canonical-runner equality: the parsed runner (after + // stripping toml + JSON escape layers) must be exactly process.execPath + // (forward-slashed, since resolveNodeRunner normalizes that way). + const expected = process.execPath.replace(/\\/g, '/'); + assert.equal(unescapeRunner(parsed.runner), expected, + `parsed runner must equal process.execPath, got: ${parsed.runner}, want: ${expected}`); + }); +}); + +describe('Bug #3017: rewriteLegacyCodexHookBlock migrates bare-node on reinstall', () => { + test('exported as a function', () => { + assert.equal(typeof rewriteLegacyCodexHookBlock, 'function'); + }); + + test('rewrites a bare-node managed-hook command to the absolute runner', () => { + const before = [ + '[model]', + 'name = "o3"', + '', + '# GSD Hooks', + '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "node /Users/x/.codex/hooks/gsd-check-update.js"', + '', + ].join('\n'); + const expectedRunnerPath = '/usr/local/bin/node'; + const runner = `"${expectedRunnerPath}"`; + const result = rewriteLegacyCodexHookBlock(before, runner); + assert.equal(result.changed, true, 'must report change=true'); + // The migrated command must use the EXACT absolute runner the caller + // supplied (#3022 CR — was previously asserting a loose '/node' + // substring which let unrelated absolute paths pass). + const parsed = parseCodexHookBlock(result.content); + assert.equal(parsed.ok, true); + assert.equal(unescapeRunner(parsed.runner), expectedRunnerPath, + `runner must equal supplied absolute path: ${parsed.runner}`); + assert.equal(parsed.hookPath, '/Users/x/.codex/hooks/gsd-check-update.js'); + // Non-GSD content (the [model] block) must be preserved verbatim. + assert.ok(result.content.includes('[model]')); + assert.ok(result.content.includes('name = "o3"')); + }); + + test('does NOT touch a managed-hook entry that already uses an absolute runner', () => { + const already = [ + '# GSD Hooks', + '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "\\"/usr/local/bin/node\\" /Users/x/.codex/hooks/gsd-check-update.js"', + '', + ].join('\n'); + const result = rewriteLegacyCodexHookBlock(already, '"/usr/local/bin/node"'); + assert.equal(result.changed, false); + assert.equal(result.content, already); + }); + + test('does NOT touch user-authored bare-node hooks (filename not in managed allowlist)', () => { + const userOwned = [ + '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "node /home/me/my-custom-codex-hook.js"', + '', + ].join('\n'); + const result = rewriteLegacyCodexHookBlock(userOwned, '"/usr/local/bin/node"'); + assert.equal(result.changed, false, + 'user-authored hooks must be left alone; only managed gsd-* hooks are migrated'); + assert.equal(result.content, userOwned); + }); + + test('returns content unchanged when absoluteRunner is null', () => { + const before = 'command = "node /path/to/gsd-check-update.js"'; + const result = rewriteLegacyCodexHookBlock(before, null); + assert.equal(result.changed, false); + assert.equal(result.content, before); + }); +}); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index c7f662621..a53c75c39 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -1434,10 +1434,19 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(parsed.hooks && Array.isArray(parsed.hooks.SessionStart), 'writes [[hooks.SessionStart]] AoT'); assert.ok(Array.isArray(parsed.hooks.SessionStart[0].hooks), 'writes [[hooks.SessionStart.hooks]] sub-table'); assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command', 'handler type is "command"'); + // #3017: handler command now uses the absolute Node binary path so + // GUI/minimal-PATH runtimes can resolve it. The shape is + // "" "" + // where is process.execPath (forward-slashed) + // and the hook path is also quoted. Same Node process runs the test + // and the installer, so process.execPath matches at both ends. + const expectedRunner = process.execPath.replace(/\\/g, '/'); + const expectedHookPath = path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); + const expectedCommand = `"${expectedRunner}" "${expectedHookPath}"`; assert.strictEqual( parsed.hooks.SessionStart[0].hooks[0].command, - 'node ' + path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'), - 'handler command must be the exact absolute path to gsd-check-update.js' + expectedCommand, + 'handler command must use absolute node runner pointing at gsd-check-update.js (#3017)' ); assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] AoT emitted'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'writes one codex_hooks key');