fix(#2866): Codex installer strips legacy hooks at EOF without trailing newline (#2870)

* fix(#2866): Codex installer strips legacy hooks at end-of-file without trailing newline

The four shape-strip regexes in `bin/install.js` (Codex install path)
required `\r?\n` at end. A stale GSD hook block sitting at end-of-file
without a trailing newline (common — many editors strip them, and the
legacy installer never wrote one) failed every shape, the installer
saw `gsd-check-update` already present, skipped writing the new
Nested-AoT block, and Codex 0.125+ refused to load with
  invalid type: map, expected a sequence in `hooks`

Root cause + fix
================

Each shape's terminator changed from `\r?\n` to `(?:\r?\n|$)`, so
end-of-file is also a valid terminator.

Strip logic was lifted into a new pure helper
`stripStaleGsdHookBlocks(configContent)` that the install pipeline now
calls in place of the inline replace chain. The helper is exported via
the GSD_TEST_MODE module.exports for direct unit-test coverage.

Regression test
===============

`tests/bug-2866-codex-strip-no-trailing-newline.test.cjs` exercises
all four historical shapes (Shape 1 — pre-#1755 gsd-update-check;
Shape 2 — flat [[hooks]]+gsd-check-update; Shape 3 — single
[[hooks.SessionStart]] without nested .hooks; Shape 4 — correct
two-block nested) twice each: once with a trailing newline (regression
guard against the existing behavior) and once at end-of-file without a
trailing newline (the reporter's exact repro).

It also asserts:
- the helper is a no-op when no GSD reference is present, and
- Shape 4 strip does not leave an orphaned [[hooks.SessionStart]]
  header behind (the same ordering invariant the inline code relied on).

The helper is loaded via `package.json` `bin` field, not a hardcoded
path — `tests/bug-2866-codex-strip-no-trailing-newline.test.cjs`
parses package.json and resolves `pkg.bin['get-shit-done-cc']` to
require the installer.

Closes #2866

* test(#2866): assert TOML structure, not raw-text substrings

CodeRabbit caught the strip assertions using `.includes()` against
raw TOML output. Added a small line-structural parseTomlShape() helper
(table headers + dotted-path key/value map, comments stripped) and
rewrote the assertions to:

- Verify no [[hooks.* table header survives the strip
- Verify no key carries a stale gsd-(update|check)-(check|update) value
- Verify history.persistence is preserved as the parsed string "save-all"

Behaviour is unchanged (the strip function under test is not modified).
The assertions now check structural shape rather than substring presence,
which catches re-shaping regressions that text matching would miss.

No new dependencies — the parser is local to the test and handles only
the small well-formed TOML these tests construct.

* refactor(#2866): replace regex hook strip with TOML AST removal

Per CR feedback on PR #2870: the regex-driven `stripStaleGsdHookBlocks`
implementation was fragile to whitespace, indentation, and key-ordering
variations the regression test never exercised. Variations the regex
silently leaked (verified before the rewrite):

- Shape 4 with an extra blank line between parent/child tables
- Shape 2/3 with `command` ordered before `event`
- Shape 3 with an extra `timeout = 5000` key — worse than a leak: the
  regex matched only the command line, leaving `timeout = 5000`
  orphaned outside any TOML table (invalid TOML)
- Tight whitespace `event="SessionStart"` (no spaces around `=`)

The structural rewrite uses the TOML parser already present in this file
(`getTomlTableSections` + `getTomlLineRecords` + `parseTomlValue` +
`removeContentRanges` + `collapseTomlBlankLines`):

1. Find every section whose path is `hooks` or starts with `hooks.`.
2. For each, walk the section's line records and parse `command` values
   structurally — match by basename equality (`gsd-update-check.js` or
   `gsd-check-update.js`), never by regex on raw bytes.
3. Detect orphaned `[[hooks.SessionStart]]` parents: empty body and a
   stale child immediately follows → mark for removal.
4. Extend each removal range backward through any preceding
   `# GSD Hooks` marker line (detected via line records, not text scan).
5. Remove ranges atomically and collapse resulting blank-line runs.

Legacy hook basenames are hoisted to template-literal constants so the
existing `install-hooks-copy.test.cjs` quoted-literal guard continues to
catch accidental *registration* of the inverted filename, while strip
detection (which legitimately needs both names) bypasses it.

Test coverage added: 8 new sub-tests exercising the four whitespace/
ordering variations (with and without trailing newline) plus a
`[[hooks.UserPromptSubmit]]` user-authored hook to guarantee the strip
only touches GSD-managed sections. 20/20 in the file, 5867/5867 in the
full suite.
This commit is contained in:
Tom Boucher
2026-04-29 21:51:58 -04:00
committed by GitHub
parent f2ada8500c
commit ef08a89241
2 changed files with 340 additions and 22 deletions

View File

@@ -2895,6 +2895,129 @@ function stripLeakedGsdCodexSections(content) {
return collapseTomlBlankLines(cleaned);
}
/**
* Strip GSD-managed legacy Codex hook blocks from a config.toml string
* using the TOML AST already used elsewhere in this file
* (`getTomlTableSections` + `removeContentRanges`). The earlier regex-based
* implementation required a precise key order, exact single-space padding
* around `=`, and exactly one blank line between Shape 4's parent/child
* tables — any deviation (an extra blank line, key reorder, an added
* `timeout` key, `event="SessionStart"` without spaces) silently leaked the
* stale block, sometimes corrupting the file by leaving orphaned key=value
* lines outside any table.
*
* The structural approach: find every `hooks*` table whose body contains a
* `command = "...gsd-(check-update|update-check).js"` value, remove its
* exact byte range, and additionally remove any orphaned parent
* `[[hooks.SessionStart]]` whose body becomes empty as a result (Shape 4).
* The leading `# GSD Hooks` header line is swallowed by extending the
* removal range backward through any single preceding comment line.
*
* Pure function, exported for test coverage. Returns the input unchanged
* if no GSD-managed hook section is present.
*/
// Legacy hook command basenames to detect during strip. Template-literal
// form so install-hooks-copy.test.cjs's quoted-literal guard continues to
// catch accidental regressions where someone *registers* the inverted
// `gsd-update-check.js` filename in a Codex hook command.
const STALE_HOOK_BASENAMES = new Set([
`gsd-update-check.js`,
`gsd-check-update.js`,
]);
function stripStaleGsdHookBlocks(configContent) {
const sections = getTomlTableSections(configContent);
const lineRecords = getTomlLineRecords(configContent);
const hookSections = sections.filter(
(s) => s.path === 'hooks' || s.path.startsWith('hooks.')
);
if (hookSections.length === 0) {
return configContent;
}
// A section is GSD-managed if any structural `command` key inside its
// body parses to a string whose basename matches `gsd-(check-update|
// update-check).js`. The TOML line parser already classified each line's
// `keySegments`, so we never inspect raw text — this handles arbitrary
// whitespace, key reordering, and additional keys robustly.
function sectionHasStaleCommand(section) {
const records = lineRecords.filter(
(r) => !r.startsInMultilineString
&& !r.tableHeader
&& r.start >= section.headerEnd
&& r.end + r.eol.length <= section.end
&& r.keySegments
&& r.keySegments.length === 1
&& r.keySegments[0] === 'command'
);
for (const record of records) {
const equalsIndex = findTomlAssignmentEquals(record.text);
if (equalsIndex === -1) continue;
let parsed;
try {
parsed = parseTomlValue(record.text, equalsIndex + 1);
} catch {
continue;
}
if (typeof parsed.value !== 'string') continue;
// Match the basename — Codex configs reference these files by absolute
// path under the user's `.codex/hooks/` directory.
const basename = parsed.value.split(/[\\/]/).pop() || '';
if (STALE_HOOK_BASENAMES.has(basename)) {
return true;
}
}
return false;
}
const stale = new Set(hookSections.filter(sectionHasStaleCommand));
if (stale.size === 0) {
return configContent;
}
// Shape 4: a `[[hooks.SessionStart]]` event-table whose body is empty and
// whose immediately following section is a stale child handler table
// (`[[hooks.SessionStart.hooks]]`) becomes orphaned once the child is
// stripped. Detect emptiness via line records — no key/value lines and no
// non-blank, non-comment text between this section's header and the next.
function sectionBodyHasContent(section) {
return lineRecords.some(
(r) => !r.startsInMultilineString
&& !r.tableHeader
&& r.start >= section.headerEnd
&& r.end + r.eol.length <= section.end
&& r.text.trim() !== ''
&& !r.text.trim().startsWith('#')
);
}
for (let i = 0; i < sections.length; i += 1) {
const parent = sections[i];
if (stale.has(parent)) continue;
if (!parent.array || parent.path !== 'hooks.SessionStart') continue;
if (sectionBodyHasContent(parent)) continue;
const next = sections[i + 1];
if (next && stale.has(next) && next.path.startsWith('hooks.SessionStart.')) {
stale.add(parent);
}
}
// Each removal range starts at the table header. If the immediately
// preceding line is the GSD marker comment `# GSD Hooks` (and is not part
// of an already-removed section), extend the range backward to swallow it
// — preserves cleanliness on round-trip strip+rewrite.
const ranges = [];
for (const section of stale) {
let start = section.start;
const headerLineIdx = lineRecords.findIndex((r) => r.start === section.start);
const prev = headerLineIdx > 0 ? lineRecords[headerLineIdx - 1] : null;
if (prev && !prev.startsInMultilineString && prev.text.trim() === '# GSD Hooks') {
start = prev.start;
}
ranges.push({ start, end: section.end });
}
return collapseTomlBlankLines(removeContentRanges(configContent, ranges));
}
/**
* Migrate legacy Codex [hooks] map format to [[hooks]] array-of-tables format.
*
@@ -7201,28 +7324,7 @@ function install(isGlobal, runtime = 'claude') {
// Shape 2 — flat [[hooks]] + event = "SessionStart" (#2637 era, never correct)
// Shape 4 — correct two-block nested (strip before shape 3 to avoid orphaned header)
// Shape 3 — single-block [[hooks.SessionStart]] without nested .hooks (#2760 era)
if (configContent.includes('gsd-update-check') || configContent.includes('gsd-check-update')) {
// Shape 1
configContent = configContent.replace(
/(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-update-check\.js"\r?\n/gm,
(match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''),
);
// Shape 2
configContent = configContent.replace(
/(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm,
(match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''),
);
// Shape 4 — strip before shape 3 to avoid orphaned header
configContent = configContent.replace(
/(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\n\r?\n\[\[hooks\.SessionStart\.hooks\]\]\r?\ntype = "command"\r?\ncommand = "node [^\r\n]*(?:gsd-check-update|gsd-update-check)\.js"\r?\n/gm,
(match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''),
);
// Shape 3
configContent = configContent.replace(
/(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\ncommand = "node [^\r\n]*(?:gsd-check-update|gsd-update-check)\.js"\r?\n/gm,
(match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''),
);
}
configContent = stripStaleGsdHookBlocks(configContent);
// Migrate legacy [hooks] map format and flat [[hooks]] AoT entries to the
// namespaced [[hooks.<EVENT>]] form after stripping GSD-managed stale blocks.
@@ -8464,6 +8566,7 @@ if (process.env.GSD_TEST_MODE) {
generateCodexConfigBlock,
stripGsdFromCodexConfig,
migrateCodexHooksMapFormat,
stripStaleGsdHookBlocks,
hasUserNamespacedAotHooks,
parseTomlToObject,
validateCodexConfigSchema,

View File

@@ -0,0 +1,215 @@
/**
* Bug #2866: Codex Installer (RC.7) fails to strip legacy flat hooks if
* trailing newline is missing.
*
* The cleanup regexes in `bin/install.js` matched stale GSD hook blocks
* via `\r?\n` at the end. When a stale block sat at end-of-file without
* a trailing newline (very common — many editors strip them, and the
* legacy installer never wrote one), no shape stripped, the installer
* saw `gsd-check-update` already present, skipped writing the new
* Nested-AoT block, and Codex 0.125+ refused to load with
* "invalid type: map, expected a sequence in `hooks`"
*
* Fix: every shape's terminator is now `(?:\r?\n|$)` so end-of-file
* counts as a valid terminator. The strip logic was lifted into a pure
* helper, `stripStaleGsdHookBlocks(configContent)`, exported from
* `bin/install.js` for direct test coverage.
*
* This test parses `package.json` to require `bin/install.js`
* structurally (not by hardcoded path), then drives each historical
* shape through the helper twice — once with a trailing newline, once
* without — and asserts both are stripped.
*/
'use strict';
process.env.GSD_TEST_MODE = '1';
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
const path = require('node:path');
const fs = require('node:fs');
const REPO_ROOT = path.join(__dirname, '..');
const pkg = JSON.parse(fs.readFileSync(path.join(REPO_ROOT, 'package.json'), 'utf-8'));
const installPath = path.resolve(REPO_ROOT, pkg.bin['get-shit-done-cc']);
const { stripStaleGsdHookBlocks } = require(installPath);
/**
* Parse the TOML output line-structurally so assertions check shape, not
* substring presence in raw text. Comments are dropped, table headers are
* recorded, and string-valued keys are captured. Sufficient for the small,
* well-formed TOML produced by these tests.
*/
function parseTomlShape(text) {
const tableHeaders = [];
const keys = new Map(); // dotted path → string value (last-write-wins, fine for these inputs)
let currentTable = '';
for (const rawLine of text.split('\n')) {
const line = rawLine.replace(/(?:^|\s)#.*$/, '').trim();
if (!line) continue;
const tableMatch = line.match(/^\[(\[)?([^\]]+)\]?\]$/);
if (tableMatch) {
currentTable = tableMatch[2];
tableHeaders.push((tableMatch[1] ? '[[' : '[') + currentTable + (tableMatch[1] ? ']]' : ']'));
continue;
}
const kvMatch = line.match(/^([A-Za-z_][\w-]*)\s*=\s*(.*)$/);
if (kvMatch) {
const key = currentTable ? `${currentTable}.${kvMatch[1]}` : kvMatch[1];
const value = kvMatch[2].replace(/^"(.*)"$/, '$1');
keys.set(key, value);
}
}
return { tableHeaders, keys };
}
const SHAPES = {
'Shape 1 (legacy gsd-update-check)': [
'# GSD Hooks',
'[[hooks]]',
'event = "SessionStart"',
'command = "node /Users/USER/.codex/hooks/gsd-update-check.js"',
].join('\n'),
'Shape 2 (flat [[hooks]] + gsd-check-update)': [
'# GSD Hooks',
'[[hooks]]',
'event = "SessionStart"',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
].join('\n'),
'Shape 3 ([[hooks.SessionStart]] without nested .hooks)': [
'# GSD Hooks',
'[[hooks.SessionStart]]',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
].join('\n'),
'Shape 4 (nested [[hooks.SessionStart]] + [[hooks.SessionStart.hooks]])': [
'# GSD Hooks',
'[[hooks.SessionStart]]',
'',
'[[hooks.SessionStart.hooks]]',
'type = "command"',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
].join('\n'),
};
describe('bug-2866: stripStaleGsdHookBlocks handles end-of-file without trailing newline', () => {
test('stripStaleGsdHookBlocks is exported from bin/install.js', () => {
assert.strictEqual(typeof stripStaleGsdHookBlocks, 'function',
'bin/install.js must export stripStaleGsdHookBlocks');
});
function assertStripped(out, shape, scenario) {
const shape_ = parseTomlShape(out);
const hooksTable = shape_.tableHeaders.find((h) => /^\[\[?hooks(\.|]\])/.test(h));
assert.strictEqual(hooksTable, undefined,
`(${shape}, ${scenario}) no hooks table header may remain after strip, got tables: ${shape_.tableHeaders.join(', ')}`);
const staleCmd = [...shape_.keys.entries()].find(([_, v]) =>
/gsd-(update-check|check-update)/.test(v));
assert.strictEqual(staleCmd, undefined,
`(${shape}, ${scenario}) no key may carry a stale gsd-*-update command, got: ${staleCmd && staleCmd.join('=')}`);
assert.strictEqual(shape_.keys.get('history.persistence'), 'save-all',
`(${shape}, ${scenario}) history.persistence must be preserved as "save-all"`);
}
for (const [shape, block] of Object.entries(SHAPES)) {
test(`${shape}: stripped when terminated by trailing newline`, () => {
const input = `[history]\npersistence = "save-all"\n${block}\n`;
assertStripped(stripStaleGsdHookBlocks(input), shape, 'with trailing newline');
});
test(`${shape}: stripped when at end-of-file without trailing newline`, () => {
// The reporter's repro: stale block sits at the very end with no \n.
const input = `[history]\npersistence = "save-all"\n${block}`;
assertStripped(stripStaleGsdHookBlocks(input), shape, 'no trailing newline');
});
}
test('returns input unchanged when no GSD hook block is present', () => {
const benign = '[history]\npersistence = "save-all"\n';
const out = stripStaleGsdHookBlocks(benign);
assert.strictEqual(out, benign, 'helper must be a no-op when no GSD reference exists');
const benignShape = parseTomlShape(out);
assert.strictEqual(benignShape.keys.get('history.persistence'), 'save-all',
'parsed shape must preserve history.persistence');
assert.deepStrictEqual(benignShape.tableHeaders, ['[history]'],
'parsed shape must contain only the [history] table');
});
// The structural rewrite (TOML-AST-driven, not regex-driven) must handle
// whitespace and key-ordering variations that the previous regex missed.
// These cases were silently leaked by the old implementation; one
// (V3) actually corrupted the file by leaving an orphaned key=value line
// outside any table.
const VARIATIONS = {
'extra blank line in Shape 4': [
'# GSD Hooks',
'[[hooks.SessionStart]]',
'',
'',
'[[hooks.SessionStart.hooks]]',
'type = "command"',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
].join('\n'),
'keys reordered (command before event in Shape 2)': [
'# GSD Hooks',
'[[hooks]]',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
'event = "SessionStart"',
].join('\n'),
'extra key alongside command (Shape 3 + timeout)': [
'# GSD Hooks',
'[[hooks.SessionStart]]',
'command = "node /Users/USER/.codex/hooks/gsd-check-update.js"',
'timeout = 5000',
].join('\n'),
'tight whitespace (no spaces around `=`)': [
'# GSD Hooks',
'[[hooks]]',
'event="SessionStart"',
'command="node /Users/USER/.codex/hooks/gsd-check-update.js"',
].join('\n'),
};
for (const [variation, block] of Object.entries(VARIATIONS)) {
test(`variation stripped: ${variation}`, () => {
const input = `[history]\npersistence = "save-all"\n${block}\n`;
assertStripped(stripStaleGsdHookBlocks(input), variation, 'with trailing newline');
});
test(`variation stripped at EOF without trailing newline: ${variation}`, () => {
const input = `[history]\npersistence = "save-all"\n${block}`;
assertStripped(stripStaleGsdHookBlocks(input), variation, 'no trailing newline');
});
}
test('user-authored [[hooks.UserPromptSubmit]] is preserved', () => {
// The structural strip must not touch hook tables that don't carry a
// GSD-managed `gsd-(check-update|update-check).js` command.
const input = [
'[history]',
'persistence = "save-all"',
'[[hooks.UserPromptSubmit]]',
'command = "node /Users/USER/my-hook.js"',
'',
].join('\n');
const out = stripStaleGsdHookBlocks(input);
const shape = parseTomlShape(out);
assert.ok(
shape.tableHeaders.includes('[[hooks.UserPromptSubmit]]'),
`user-authored [[hooks.UserPromptSubmit]] must survive, got: ${shape.tableHeaders.join(', ')}`,
);
assert.strictEqual(
shape.keys.get('hooks.UserPromptSubmit.command'),
'node /Users/USER/my-hook.js',
'user-authored command value must be preserved verbatim',
);
});
test('Shape 4 strip does not leave an orphaned [[hooks.SessionStart]] header', () => {
// Shape 4 is stripped before Shape 3 specifically to avoid this.
const block = SHAPES['Shape 4 (nested [[hooks.SessionStart]] + [[hooks.SessionStart.hooks]])'];
const out = stripStaleGsdHookBlocks(`[history]\npersistence = "save-all"\n${block}`);
const outShape = parseTomlShape(out);
const orphan = outShape.tableHeaders.find((h) => /hooks\.SessionStart/.test(h));
assert.strictEqual(orphan, undefined,
`Shape 4 strip must remove the parent [[hooks.SessionStart]] header too, got tables: ${outShape.tableHeaders.join(', ')}`);
});
});