golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#3017): codex SessionStart hook uses absolute node, not bare 'node' (#3022)

Browse Source 
* 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
  '"<process.execPath>" "<hookPath>"' instead of 'node <hookPath>'.

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.
This commit is contained in:
Tom Boucher
2026-05-02 11:45:30 -04:00
committed by GitHub
parent f2decefede
commit 8e25eb6546
4 changed files with 344 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

207
tests/bug-3017-codex-hook-absolute-node.test.cjs Normal file
View File

@@ -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 = "<absoluteRunner> <quoted hook path>"` 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="<runner> <quoted-hook-path>" 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> <hookpath>" — 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);
});
});