From ef08a8924105fc153540722cc02545b6cd1d8d13 Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Wed, 29 Apr 2026 21:51:58 -0400
Subject: [PATCH] fix(#2866): Codex installer strips legacy hooks at EOF
 without trailing newline (#2870)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 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.
---
 bin/install.js                                | 147 ++++++++++--
 ...6-codex-strip-no-trailing-newline.test.cjs | 215 ++++++++++++++++++
 2 files changed, 340 insertions(+), 22 deletions(-)
 create mode 100644 tests/bug-2866-codex-strip-no-trailing-newline.test.cjs

diff --git a/bin/install.js b/bin/install.js
index 8523135a2..4d5de9537 100755
--- a/bin/install.js
+++ b/bin/install.js
@@ -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,
diff --git a/tests/bug-2866-codex-strip-no-trailing-newline.test.cjs b/tests/bug-2866-codex-strip-no-trailing-newline.test.cjs
new file mode 100644
index 000000000..8cb233491
--- /dev/null
+++ b/tests/bug-2866-codex-strip-no-trailing-newline.test.cjs
@@ -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(', ')}`);
+  });
+});