From 03738824dec9b46077752e6f8f628c096a5faa1e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 6 Sep 2026 05:46:49 -0400 Subject: [PATCH] enhance(#2586): stop installing Codex context-monitor hooks without metrics (#4367) --- .changeset/patient-quails-greet.md | 5 + CONTEXT.md | 2 +- bin/install.js | 122 +++--- capabilities/codex/capability.json | 6 +- docs/how-to/install-on-your-runtime.md | 7 +- .../host-integration-capability-matrix.md | 2 +- gsd-core/bin/lib/capability-registry.cjs | 12 +- gsd-core/bin/lib/capability-validator.cjs | 5 + src/runtime-hooks-surface.cts | 207 +++++++++- tests/codex-config.test.cjs | 356 +++++++++++++++++- tests/fixtures/install-tree/codex.json | 4 - tests/install-minimal-hooks.test.cjs | 61 +-- 12 files changed, 668 insertions(+), 121 deletions(-) create mode 100644 .changeset/patient-quails-greet.md diff --git a/.changeset/patient-quails-greet.md b/.changeset/patient-quails-greet.md new file mode 100644 index 000000000..e6ad01da0 --- /dev/null +++ b/.changeset/patient-quails-greet.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 4367 +--- +**Codex no longer installs a context-monitor hook that could never fire.** `gsd-context-monitor.js` read a remaining-context bridge file only Claude Code's statusline hook writes, so every one of its Codex hook-event registrations was a guaranteed silent no-op. Fresh Codex installs no longer copy or register it; a reinstall over an older install now removes the stale registrations and the orphaned script. Agent-facing context warnings and phase/lifecycle display are documented as unsupported on Codex until a real metrics producer exists for that runtime. (#2586) diff --git a/CONTEXT.md b/CONTEXT.md index dcb451fef..1f2e6a438 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -452,7 +452,7 @@ A per-agent narrative entry written by `mempalace_diary_write`. GSD's `gsd-mempa The `mempalace.memory_mode` config key controlling how authoritative MemPalace is during recall/capture relative to GSD's native memory. Three wired values: `augment` (default — palace is an additive recall layer; native memory stays authoritative; lowest coupling), `kg_backend` (knowledge-graph queries resolve against MemPalace's temporal graph as the primary source, `.planning/graphs/` as fallback; non-KG drawer recall stays additive), `replace` (recall resolves through the palace as the source of truth, native artifacts as fallback). Every mode is `onError:skip` and default-resilient — an unreachable palace degrades to native memory and GSD keeps writing `.planning/graphs/`, so no mode loses memory. Read at hook-render time; switching is a config change, not a reinstall. Cross-mode migration of existing `.planning/graphs/` into the palace is a separate, not-yet-implemented concern (PRD/ADR §17 open question). See MemPalace Settings in `docs/CONFIGURATION.md`. ### Runtime Hooks Surface Module -Standalone hook-surface writer module extracted from `bin/install.js` as ADR-857 phase 5f-1 (behavior-preserving relocation, no logic change). Owns: Cline rules-body/agents-md/pre-tool-use hook generation (`buildClineRulesBody`, `buildClineAgentsMdBody`, `buildClinePreToolUseHook`, `mergeGsdAgentsMd`, `writeClineArtifacts`); Cursor `hooks.json` lifecycle (`buildCursorHookEntry`, `isManagedCursorHookEntry`, `reconcileCursorHooksJson`, `writeCursorHooksJson`, `removeCursorHooksJson`); Copilot session-hook config (`buildCopilotHookConfig`, `writeCopilotHookConfig`); Codex hook-block and event management (`buildCodexHookBlock`, `rewriteLegacyCodexHookBlock`, `reconcileCodexHooksJsonEvent`, `reconcileCodexHooksJsonSessionStart`, `ensureCodexHooksJsonSessionStart`, `ensureCodexHooksJsonEvent`, `removeCodexHooksJsonEvent`, `removeCodexHooksJsonSessionStart`, `buildCodexHookWindowsShimIR`); Kimi native config.toml `[[hooks]]` lifecycle (`buildKimiHooksTomlBlock`, `stripKimiHooksTomlBlock`, `writeKimiHooksToml`, `removeKimiHooksToml` — #2095 EoS/kimi Upgrade 1, the first genuinely NEW hook surface added post-relocation rather than a behavior-preserving move: kimi's `[[hooks]]` array lives in its own native `config.toml`, resolved by `resolveKimiHooksTomlDir` in Runtime Homes Module to a directory deliberately separate from kimi's GSD configDir, wrapped in `# GSD Hooks BEGIN`/`END` marker comments for idempotent reinstall); and shared hook command helpers (`buildHookCommand`, `rewriteLegacyManagedNodeHookCommands`, `normalizeNodePath`, `resolveNodeRunner`, and — #3662 — `buildNodeRunnerChainToken`, the POSIX-sh runner token that resolves node at hook-fire time for non-portable managed JS hooks, plus the `NODE_RUNNER_RESOLVER_HOOK` basename of the staged `hooks/gsd-node-runner.sh` resolver that portable installs route through with the baked node path as its first argument). `bin/install.js` delegates to this module via thin wrappers and re-exports its functions unchanged so existing tests require no modification. Source: `src/runtime-hooks-surface.cts`. Built output: `gsd-core/bin/lib/runtime-hooks-surface.cjs`. +Standalone hook-surface writer module extracted from `bin/install.js` as ADR-857 phase 5f-1 (behavior-preserving relocation, no logic change). Owns: Cline rules-body/agents-md/pre-tool-use hook generation (`buildClineRulesBody`, `buildClineAgentsMdBody`, `buildClinePreToolUseHook`, `mergeGsdAgentsMd`, `writeClineArtifacts`); Cursor `hooks.json` lifecycle (`buildCursorHookEntry`, `isManagedCursorHookEntry`, `reconcileCursorHooksJson`, `writeCursorHooksJson`, `removeCursorHooksJson`); Copilot session-hook config (`buildCopilotHookConfig`, `writeCopilotHookConfig`); Codex hook-block and event management (`buildCodexHookBlock`, `rewriteLegacyCodexHookBlock`, `reconcileCodexHooksJsonEvent`, `reconcileCodexHooksJsonSessionStart`, `ensureCodexHooksJsonSessionStart`, `removeCodexHooksJsonEvent`, `removeCodexHooksJsonSessionStart`, `buildCodexHookWindowsShimIR`, `cleanupOrphanedCodexContextMonitorScript`, `isGsdOwnedCodexContextMonitorScript`, `hooksJsonReferencesCodexContextMonitor` — #2586, the last three replacing the deleted `ensureCodexHooksJsonEvent`: GSD no longer adds Codex context-monitor hook-event registrations, only recognizes and removes stale ones left by a pre-#2586 install, then deletes the orphaned script/`.cmd` shim once unreferenced and content-verified as GSD-owned); Kimi native config.toml `[[hooks]]` lifecycle (`buildKimiHooksTomlBlock`, `stripKimiHooksTomlBlock`, `writeKimiHooksToml`, `removeKimiHooksToml` — #2095 EoS/kimi Upgrade 1, the first genuinely NEW hook surface added post-relocation rather than a behavior-preserving move: kimi's `[[hooks]]` array lives in its own native `config.toml`, resolved by `resolveKimiHooksTomlDir` in Runtime Homes Module to a directory deliberately separate from kimi's GSD configDir, wrapped in `# GSD Hooks BEGIN`/`END` marker comments for idempotent reinstall); and shared hook command helpers (`buildHookCommand`, `rewriteLegacyManagedNodeHookCommands`, `normalizeNodePath`, `resolveNodeRunner`, and — #3662 — `buildNodeRunnerChainToken`, the POSIX-sh runner token that resolves node at hook-fire time for non-portable managed JS hooks, plus the `NODE_RUNNER_RESOLVER_HOOK` basename of the staged `hooks/gsd-node-runner.sh` resolver that portable installs route through with the baked node path as its first argument). `bin/install.js` delegates to this module via thin wrappers and re-exports its functions unchanged so existing tests require no modification. Source: `src/runtime-hooks-surface.cts`. Built output: `gsd-core/bin/lib/runtime-hooks-surface.cjs`. ### Runtime Config Adapter Registry Module owning the explicit per-runtime config-mutation dispatch table for the installer. `resolveRuntimeConfigIntent(runtime)` projects a typed config intent — `installSurface` (`settings-json` | `codex-toml` | `copilot-instructions` | `cline-rules` | `cursor-hooks-json` | `profile-marker-only`), `writesSharedSettings` (the `finishInstall` shared-settings write gate), and `finishPermissionWriter` (`opencode` | `kilo` | `antigravity` | none) — that `bin/install.js` dispatches on instead of inline `runtime === '...'` branching. Owns adapter selection only: it performs no filesystem IO and does not execute config mutations (the install/finishInstall handlers and the per-runtime writers do that). Unknown runtimes fail loudly with a `TypeError`, guarded by an `Object.hasOwn` own-property check so prototype-chain keys (`__proto__`, `constructor`) also throw. Also exports `resolveInstallPlan(runtime)` — the ADR-58 `InstallPlan` capstone — which collects the install-level descriptor axes (`installSurface`, `writesSharedSettings`, `finishPermissionWriter`, `hookEvents`, `extendedHookEvents`, `hooksSurface`, `sandboxTier`) into one typed `InstallPlan` value consumed by `install()` and `finishInstall()` in `bin/install.js`. `sandboxTier` (`none` | `codex-agent-sandbox`) gates per-agent `sandbox_mode` emission in the codex TOML path and fails loud on a missing/invalid value (#1151). The spatial axes (`configHome`, `artifactLayout`, `commandStyle`) remain behind their self-resolving adapter modules and are not part of the plan; they are the execution adapters. Realizes both the adapter-selection and plan-collection halves of the Runtime Install Policy Module boundary. Source: `gsd-core/bin/lib/runtime-config-adapter-registry.cjs`. See ADR-58, #60. diff --git a/bin/install.js b/bin/install.js index a578aadcc..ade940d41 100755 --- a/bin/install.js +++ b/bin/install.js @@ -1369,34 +1369,13 @@ function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { } /** - * Ensure hooks.json contains exactly one managed GSD hook entry for the given - * Codex event, wired to gsd-context-monitor.js. Preserves user-owned entries. - * - * Used for the new Codex events added in #772: - * SubagentStart — inject context / GSD_AGENT_NAME awareness at subagent open - * Stop — post-session context headroom tracking - * PostToolUse — mirror the Claude Code PostToolUse context monitor - * - * All three events are routed through gsd-context-monitor.js — the same hook - * used for PostToolUse in the Claude Code baseline — so context-headroom - * warnings surface at these key Codex session lifecycle moments. - * - * On Windows (#3426): writes a gsd-context-monitor.cmd shim alongside the .js - * file and uses the .cmd path as the hook command — exactly the same fix as - * SessionStart uses for gsd-check-update — to avoid the bash.exe POSIX-exec - * failure when Codex's hook dispatcher tries to run node.exe through Git Bash. - * - * @param {string} targetDir - * @param {string} eventName - One of 'SubagentStart', 'Stop', 'PostToolUse'. - * @param {{ absoluteRunner: string|null, platform?: NodeJS.Platform }} opts - * @returns {{ changed: boolean, wrote: boolean, path: string }} - */ -function ensureCodexHooksJsonEvent(targetDir, eventName, opts = {}) { - return hooksSurface.ensureCodexHooksJsonEvent(targetDir, eventName, opts); -} - -/** - * Remove a GSD-managed event entry from hooks.json. Called during uninstall. + * Remove a GSD-managed event entry from hooks.json. Called during uninstall, + * and (#2586) unconditionally during install/reinstall to clean up a + * pre-#2586 install's stale gsd-context-monitor.js registrations — GSD no + * longer ADDS entries for these events (see CODEX_HOOKS_TO_COPY / + * cleanupOrphanedCodexContextMonitorScript in bin/install.js's Codex branch), + * only removes recognized ones, so the `ensureCodexHooksJsonEvent` wrapper + * that used to add them was removed as dead code. * * @param {string} targetDir * @param {string} eventName @@ -8485,6 +8464,16 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { console.log(` ${green}✓${reset} Removed managed Codex ${eventName} hook from hooks.json`); } } + // #2586: uninstall's own symmetric half of the orphaned-script cleanup — + // same GSD-owned + unreferenced gate as the install-time call. + const uninstallMonitorCleanup = hooksSurface.cleanupOrphanedCodexContextMonitorScript(targetDir); + for (const deletedPath of uninstallMonitorCleanup.deleted) { + removedCount++; + console.log(` ${green}✓${reset} Removed orphaned Codex hook script (${path.basename(deletedPath)})`); + } + for (const warning of uninstallMonitorCleanup.warnings) { + console.warn(` ${yellow}⚠${reset} Could not remove orphaned Codex hook script ${warning.path}: ${warning.reason}`); + } } // 1a-kimi. Non-layout Kimi side-effect (#2095 EoS/kimi Upgrade 1): kimi's @@ -12323,11 +12312,17 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { // stageTransitiveHookLibs call after the copy loop. The #3579 boundary is // preserved: helpers no staged Codex hook requires (graphify tooling among // them) are still not shipped. + // #2586: gsd-context-monitor.js is deliberately NOT copied for Codex. + // It reads the statusline bridge file (${TMPDIR}/claude-ctx-{session_id}.json) + // written only by hooks/gsd-statusline.js, which Codex never installs — so + // every registered event was a guaranteed silent no-op (readSentinel throws + // ENOENT -> allow(undefined), every invocation, every event, no exceptions). + // A pre-#2586 install's stale copy + hooks.json registrations are cleaned + // up below (see the CODEX_EXTENDED_HOOK_EVENTS loop), not re-added here. const CODEX_HOOKS_TO_COPY = [ 'gsd-check-update.js', 'gsd-check-update-worker.js', 'managed-hooks-registry.cjs', - 'gsd-context-monitor.js', ]; const codexHooksSrc = path.join(src, 'hooks', 'dist'); if (fs.existsSync(codexHooksSrc)) { @@ -12529,35 +12524,48 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { } } - // ── Codex extended hook events (#772, #2088) ───────────────────────── - // Codex CLI stabilised a full hook-event set in rust-v0.137.0. GSD - // registers CODEX_EXTENDED_HOOK_EVENTS (#2088 adds the 6 documented - // events beyond the original #772 three) — all routed through - // gsd-context-monitor.js so context-headroom warnings surface at each - // lifecycle point: SubagentStart/SubagentStop (subagent open/close), - // Stop (final-response), PreToolUse/PostToolUse (tool boundaries), - // PermissionRequest (approval prompts), Pre/PostCompact (context - // compaction), and UserPromptSubmit (per-turn context injection). The - // context-monitor script decides per-payload what to do; unregistered - // events simply never fire. - // - // Guard: only register when the context-monitor file exists and the node - // runner is available — same guards as the SessionStart path above. - const contextMonitorFile = path.join(targetDir, 'hooks', 'gsd-context-monitor.js'); - if (codexNodeRunner && fs.existsSync(contextMonitorFile)) { - for (const codexEvent of CODEX_EXTENDED_HOOK_EVENTS) { - const eventWrite = ensureCodexHooksJsonEvent(targetDir, codexEvent, { - absoluteRunner: codexNodeRunner, - platform: process.platform, - }); - if (eventWrite.wrote) { - console.log(` ${green}✓${reset} Configured Codex hooks (${codexEvent} via hooks.json)`); - } else if (eventWrite.changed) { - console.log(` ${green}✓${reset} Verified Codex hooks (${codexEvent} via hooks.json)`); - } + // #2586: Codex's hook payload carries no context/token-usage field + // (confirmed against codex-rs/hooks/src/schema.rs), so agent-facing + // context warnings and GSD phase/lifecycle display cannot be + // supported on this runtime — state that plainly during install + // rather than silently omitting the capability. Matches + // capabilities/codex/capability.json's hostBehaviors.unsupportedFeatures. + console.log(` ${dim}↳${reset} Codex: agent-facing context warnings and GSD phase/lifecycle display are unsupported (Codex's hook payload has no context-usage metric)`); + + // ── Codex extended hook events (#772, #2088) — REMOVED by #2586 ────── + // gsd-context-monitor.js is no longer copied or registered for Codex + // (see the CODEX_HOOKS_TO_COPY comment above): every one of these + // events was a guaranteed silent no-op, since the metrics bridge file + // it reads is only ever written by Claude's own statusline hook. + // Every event in CODEX_EXTENDED_HOOK_EVENTS is unconditionally + // reconciled here — not gated on the script existing — so a + // pre-#2586 install's stale registrations (exact current shape, or a + // recognized legacy shape via isManagedHookCommand's + // includeLegacyAliases) are stripped on reinstall. Mirrors the + // unconditional uninstall-time loop over the same constant. A + // registration whose command does not match the managed shape (a + // hand-customized entry) survives untouched — see + // reconcileCodexHooksJsonEvent's isManagedHookCommand filter. + for (const codexEvent of CODEX_EXTENDED_HOOK_EVENTS) { + const eventCleanup = removeCodexHooksJsonEvent(targetDir, codexEvent); + if (eventCleanup.changed) { + console.log(` ${green}✓${reset} Removed stale Codex ${codexEvent} context-monitor hook from hooks.json`); } - } else if (!codexNodeRunner) { - console.warn(` ${yellow}⚠${reset} Skipped Codex extended hook-event registration — Node runner unavailable.`); + } + // Delete the orphaned script (+ Windows .cmd shim) left by a + // pre-#2586 install, but ONLY once no surviving hooks.json + // registration under any event still references it, and only when + // the on-disk file is GSD's own (see design doc's Ownership check — + // a content-signature check, not manifest membership, so this works + // on the very first reinstall after upgrading, with no bootstrap + // gap). A deletion failure never reverts the (already safe, + // already-written) hooks.json cleanup above — must-have #8. + const monitorCleanup = hooksSurface.cleanupOrphanedCodexContextMonitorScript(targetDir); + for (const deletedPath of monitorCleanup.deleted) { + console.log(` ${green}✓${reset} Removed orphaned Codex hook script (${path.basename(deletedPath)})`); + } + for (const warning of monitorCleanup.warnings) { + console.warn(` ${yellow}⚠${reset} Could not remove orphaned Codex hook script ${warning.path}: ${warning.reason}`); } // ── end Codex extended hook events ──────────────────────────────────── } diff --git a/capabilities/codex/capability.json b/capabilities/codex/capability.json index 71bcd1f53..56dad8197 100644 --- a/capabilities/codex/capability.json +++ b/capabilities/codex/capability.json @@ -109,7 +109,11 @@ "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex" + "frontmatterDialect": "codex", + "unsupportedFeatures": [ + "context-warnings", + "phase-lifecycle-display" + ] } }, "reviewer": { diff --git a/docs/how-to/install-on-your-runtime.md b/docs/how-to/install-on-your-runtime.md index f829063a7..9dc286e3b 100644 --- a/docs/how-to/install-on-your-runtime.md +++ b/docs/how-to/install-on-your-runtime.md @@ -169,14 +169,13 @@ Skills land in `~/.codex/skills/gsd-*/SKILL.md`. Agents are written as standalon **Hook coverage** -GSD registers the following Codex hook events automatically on install (requires Codex CLI 0.137.0+ for the stable hook-event schema): +GSD registers the following Codex hook event automatically on install (requires Codex CLI 0.137.0+ for the stable hook-event schema): | Event | Hook | Purpose | |---|---|---| | `SessionStart` | `gsd-check-update.js` | Update check at session open; Windows installs also emit a `commandWindows` field pointing to the `.cmd` shim so Codex picks the correct executor on Windows without requiring per-OS config regeneration | -| `SubagentStart` | `gsd-context-monitor.js` | Inject context / GSD_AGENT_NAME awareness at subagent open | -| `Stop` | `gsd-context-monitor.js` | Context headroom tracking before model stop | -| `PostToolUse` | `gsd-context-monitor.js` | Mirror the context-monitor coverage available in Claude Code | + +**Context warnings are not supported on Codex (#2586).** Earlier revisions of GSD also registered `SubagentStart`/`Stop`/`PostToolUse` (plus, briefly, six more events) against `gsd-context-monitor.js` for context-headroom tracking. That hook only produces a warning by reading a remaining-context-percentage bridge file that `gsd-statusline.js` — Claude Code's own statusline mechanism — writes; Codex never installs a statusline writer, so every one of those registrations fired as a guaranteed silent no-op, every invocation, with no exceptions. GSD no longer copies or registers `gsd-context-monitor.js` on a fresh Codex install; a reinstall over an older GSD install removes the stale registrations and the now-unreferenced script automatically. Agent-facing context warnings and GSD phase/lifecycle display remain unsupported capabilities on Codex (see `capabilities/codex/capability.json`) until a real metrics producer exists for this runtime — native Codex `/statusline` configuration is a separate, not-yet-implemented surface. All registered hooks are managed by GSD and are removed cleanly on `--uninstall`. diff --git a/docs/reference/host-integration-capability-matrix.md b/docs/reference/host-integration-capability-matrix.md index 80f36b73a..aedda8f8a 100644 --- a/docs/reference/host-integration-capability-matrix.md +++ b/docs/reference/host-integration-capability-matrix.md @@ -130,7 +130,7 @@ Sources consulted: **GSD integration status — Phase D dogfood complete (#2088, ADR-1239).** Codex installs through the `declarative` embedding adapter (`createDeclarativeAdapter` → `installRuntimeArtifacts`); the hardcoded `runtime === 'codex'`/`isCodex` projection is folded into descriptor-driven `runtime.hostBehaviors`, and install/uninstall output is byte-parity-gated at the time (`tests/fixtures/golden-install-parity/codex.json`; superseded by the differential attribution check, #2724). Three capability upgrades land, each with a test driving the user-reachable surface: - **Skill root** — global skills install to the canonical `$HOME/.agents/skills` (Codex core-skills `loader.rs` user-scope root), not the deprecated `$CODEX_HOME/skills` fallback; local skills install to `/.codex/skills`. The global path is declared via the global skills-kind `home: ".agents"` override, while the local kind intentionally has no home override. Pre-move global installs are migrated (stale `~/.codex/skills/gsd-*` cleaned on both install and uninstall); local installs do not remove `$HOME/.agents/skills` because those skills may be intentionally global. -- **Hook events** — GSD registers all documented `hooks.json` lifecycle events beyond `SessionStart`: `SubagentStart`, `Stop`, `PostToolUse` (#772), plus the six added in #2088 — `PreToolUse`, `PermissionRequest`, `PreCompact`, `PostCompact`, `SubagentStop`, `UserPromptSubmit` — all routed through `gsd-context-monitor.js`. (The descriptor `extendedHookEvents` field reflects the schema-valid cross-runtime subset `SubagentStop`/`Stop`/`PreCompact`; Codex's full event set is codex-hooks-json-native, registered directly in `hooks.json`.) +- **Hook events (corrected #2586)** — GSD registers the `SessionStart` event only, wired to `gsd-check-update.js`. The `SubagentStart`/`Stop`/`PostToolUse` (#772) + six #2088 extended events (`PreToolUse`, `PermissionRequest`, `PreCompact`, `PostCompact`, `SubagentStop`, `UserPromptSubmit`) described in earlier revisions of this doc were routed through `gsd-context-monitor.js`, which reads a remaining-context-percentage bridge file (`${TMPDIR}/claude-ctx-{session_id}.json`) that only `gsd-statusline.js` writes — a Claude-only mechanism Codex never installs. Every one of those events was therefore a guaranteed silent no-op on Codex (confirmed: `readSentinel` throws `ENOENT` on every invocation, unconditionally). #2586 stops copying/registering `gsd-context-monitor.js` on fresh installs and cleans up a pre-#2586 install's stale registrations + orphaned script on reinstall/uninstall. Agent-facing context warnings and GSD phase/lifecycle display are consequently **unsupported on Codex** (`capabilities/codex/capability.json`'s `hostBehaviors.unsupportedFeatures: ["context-warnings","phase-lifecycle-display"]`) — no working warning path existed before this change either, so nothing regresses. Native Codex `/statusline` configuration remains a separate, out-of-scope surface. (The descriptor `extendedHookEvents` field's `SubagentStop`/`Stop`/`PreCompact` value is a pre-existing, unrelated drift against the fuller event set `bin/install.js` used to register — not corrected by #2586.) - **Dispatch tuning** — `[agents] max_depth = 1` is written explicitly into the managed `config.toml` block, pinning the `dispatch.maxDepth: 1` axis instead of relying on codex-cli's implicit default. Because `maxDepth === 1`, `degradationFor` flattens GSD-hosted wave dispatch to single-level even though `dispatch.nested`/`background`/`backgroundDispatch` are all `true`. The block is a bare `[agents]` AgentsToml scalar table; it does **not** carry per-role `[agents.gsd-*]` sub-tables — those pointed `config_file` back at the standalone `agents/gsd-*.toml` files Codex already auto-discovers, so emitting them was a duplicate role registration (Codex logged "Ignoring malformed agent role definition: duplicate agent role name" once per agent) removed in #2406. `validateCodexConfigSchema` permits a known-scalar-only `[agents]` while still rejecting `[[agents]]` and unknown-key forms. Sources consulted: diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 05c338919..cd08d58eb 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -1225,7 +1225,11 @@ const capabilities = { "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex" + "frontmatterDialect": "codex", + "unsupportedFeatures": [ + "context-warnings", + "phase-lifecycle-display" + ] } }, "reviewer": { @@ -6337,7 +6341,11 @@ const runtimes = { "tomlConfigInstall": true, "cleanupSkillSidecars": true, "agentTomlFiles": true, - "frontmatterDialect": "codex" + "frontmatterDialect": "codex", + "unsupportedFeatures": [ + "context-warnings", + "phase-lifecycle-display" + ] } }, "reviewer": { diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 3966b22c0..ff00dc58d 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -1956,6 +1956,11 @@ const KNOWN_HOST_BEHAVIORS = new Set([ 'sourceMarkerFile', 'tomlConfigInstall', 'trackCategoryDescription', + // #2586: declares runtime-level feature axes GSD does not/cannot support on + // this host (e.g. Codex's `["context-warnings","phase-lifecycle-display"]`) + // — present-and-populated / absent-is-unsupported-empty convention, so + // omitting the key on every other runtime carries no inverted meaning. + 'unsupportedFeatures', 'verificationStyle', 'writeCategoryDescription', ]); diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index ee1f60f9a..50d0f6c84 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -923,16 +923,68 @@ interface ReconcileResult { path: string; } +/** + * Lazily require install-engine.cjs's `hasExistingSymlinkBetween` / + * `isSymlinkedDestOptIn` — mirrors user-artifact-staging.cts's + * `_installEngineSymlinkGuard` (same call-time-require rationale: avoid a + * static circular require between install-engine.cts and this module). + */ +interface InstallEngineSymlinkGuard { + hasExistingSymlinkBetween: (root: string, fullPath: string, options?: { allowOptInFollow?: boolean }) => boolean; + isSymlinkedDestOptIn: () => boolean; +} + +function _installEngineSymlinkGuard(): InstallEngineSymlinkGuard { + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-assignment + const mod: InstallEngineSymlinkGuard = require('./install-engine.cjs'); + return mod; +} + function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts: ReconcileCodexOpts = {}): ReconcileResult { const hooksJsonPath = path.join(targetDir, 'hooks.json'); const managedCommand = typeof opts.managedCommand === 'string' ? opts.managedCommand : null; const commandWindows = typeof opts.commandWindows === 'string' ? opts.commandWindows : null; const matcher = typeof opts.matcher === 'string' ? opts.matcher : undefined; const timeout = typeof opts.timeout === 'number' ? opts.timeout : undefined; + // #2586 Major 2: every Codex hooks.json writer funnels through this one + // function, and atomicWriteFileSync's final step is a rename(2) onto + // `hooksJsonPath` — which, when that path is a symlink, REPLACES the + // symlink with a plain file rather than writing through it. Refuse (with + // the same GSD_ALLOW_SYMLINKED_DEST opt-in every other install call site + // honors) before reading or writing, so a symlinked hooks.json is neither + // silently destroyed nor left the caller no escape hatch. + const symlinkGuard = _installEngineSymlinkGuard(); + // The path this function actually reads/writes. Defaults to the nominal + // hooks.json path; reassigned below to the symlink's real target when the + // opt-in is active, so the write lands on the file the user's symlink + // points at instead of clobbering the symlink itself (see note below). + let effectiveHooksJsonPath = hooksJsonPath; + if (fs.existsSync(hooksJsonPath) && fs.lstatSync(hooksJsonPath).isSymbolicLink()) { + if ( + symlinkGuard.hasExistingSymlinkBetween(targetDir, hooksJsonPath, { + allowOptInFollow: symlinkGuard.isSymlinkedDestOptIn(), + }) + ) { + throw new Error( + `hooks.json at "${hooksJsonPath}" contains a symlink the install root "${targetDir}" does not trust — ` + + 'refusing to read or write it. If this is an intentional user-owned symlink layout, re-run with ' + + 'GSD_ALLOW_SYMLINKED_DEST=1.', + ); + } + // hasExistingSymlinkBetween returned false only because the opt-in is + // active (a symlinked leaf always trips it otherwise) — so this IS a + // symlink and we are cleared to follow it. atomicWriteFileSync's final + // step is a rename(2) onto its target, which REPLACES an existing + // symlink at that path rather than writing through it; resolving to the + // real path here makes the read AND the write operate on the symlink's + // target, leaving the symlink itself untouched, matching what "follow" + // is supposed to mean. + effectiveHooksJsonPath = fs.realpathSync(hooksJsonPath); + } let parsed: Record = {}; let currentContent: string | null = null; - if (fs.existsSync(hooksJsonPath)) { - const raw = fs.readFileSync(hooksJsonPath, 'utf8'); + if (fs.existsSync(effectiveHooksJsonPath)) { + const raw = fs.readFileSync(effectiveHooksJsonPath, 'utf8'); currentContent = raw; if (raw.trim()) { try { @@ -965,6 +1017,12 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts } parsed['hooks'] = hookTable; const eventEntries = Array.isArray(hookTable[eventName]) ? (hookTable[eventName] as unknown[]) : []; + // Minor 5 (#2586 review): an event key the user already had, already + // holding an empty array, must survive removal as an empty array — not be + // deleted outright. Deleting is only correct when OUR removal is what + // emptied a previously non-empty array. Tracked before the loop below can + // mutate anything. + const wasArrayEmpty = Array.isArray(hookTable[eventName]) && eventEntries.length === 0; let removedLegacy = false; const sanitizedEntries: unknown[] = []; @@ -1002,6 +1060,10 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts if (sanitizedEntries.length > 0) { hookTable[eventName] = sanitizedEntries; + } else if (wasArrayEmpty) { + // Nothing of ours was ever here to remove — preserve the user's own + // empty array exactly as found (Minor 5). + hookTable[eventName] = []; } else { delete hookTable[eventName]; } @@ -1015,7 +1077,7 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts const changed = currentContent !== nextContent; const shouldWrite = changed && (currentContent !== null || Object.keys(parsed).length > 0); if (shouldWrite) { - atomicWriteFileSync(hooksJsonPath, nextContent, 'utf8'); + atomicWriteFileSync(effectiveHooksJsonPath, nextContent, 'utf8'); } return { changed: changed || removedLegacy, wrote: shouldWrite, path: hooksJsonPath }; @@ -1181,6 +1243,142 @@ function removeCodexHooksJsonSessionStart(targetDir: string): ReconcileResult { return reconcileCodexHooksJsonSessionStart(targetDir, { managedCommand: null }); } +// --------------------------------------------------------------------------- +// #2586: cleanupOrphanedCodexContextMonitorScript +// --------------------------------------------------------------------------- + +interface CleanupCodexContextMonitorResult { + /** Absolute paths of files actually deleted this call. */ + deleted: string[]; + /** {path, reason} for a file that could NOT be deleted (still present). */ + warnings: { path: string; reason: string }[]; + /** True if a surviving hooks.json registration still references the + * script (or its .cmd shim) — in which case nothing was deleted. */ + stillReferenced: boolean; +} + +// Literal, version-stable markers every shipped gsd-context-monitor.js +// carries. Stable across the {{GSD_VERSION}} and runtime-path substitutions +// the Codex copy step applies (#2586 design doc "Ownership check" — a raw +// content hash would differ per runtime/version by construction, so a marker +// check is used instead of manifest-membership, which has a bootstrap gap on +// the exact case that matters most: a pre-#2586 install's manifest never +// recorded this file at all). +const CODEX_CONTEXT_MONITOR_OWNERSHIP_MARKERS = [ + '#!/usr/bin/env node', + '// gsd-hook-version:', + '// Context Monitor - PostToolUse/AfterTool hook', +]; + +function isGsdOwnedCodexContextMonitorScript(filePath: string): boolean { + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch { + return false; + } + // The .cmd shim (buildCodexHookWindowsShimIR) is a tiny generated batch + // wrapper, not the JS file itself — it never carries the JS markers above, + // so it gets its own narrower, still-specific signature: the exact + // "@ECHO OFF" / "@SETLOCAL" preamble the shim generator emits, invoking a + // script path that ends in gsd-context-monitor.js. + if (filePath.endsWith('.cmd')) { + return content.startsWith('@ECHO OFF') && content.includes('@SETLOCAL') + && /gsd-context-monitor\.js/.test(content); + } + return CODEX_CONTEXT_MONITOR_OWNERSHIP_MARKERS.every((marker) => content.includes(marker)); +} + +/** + * Scan every event in hooks.json for a surviving reference to the + * context-monitor script or its Windows .cmd shim, by basename — not scoped + * to CODEX_EXTENDED_HOOK_EVENTS, so a user who hand-registered it under an + * unrelated event key is still detected as "referenced" and the script is + * preserved. + */ +function hooksJsonReferencesCodexContextMonitor(targetDir: string): boolean { + const hooksJsonPath = path.join(targetDir, 'hooks.json'); + if (!fs.existsSync(hooksJsonPath)) return false; + let raw: string; + try { + raw = fs.readFileSync(hooksJsonPath, 'utf8'); + } catch { + return true; // unreadable — conservatively assume referenced, never delete + } + if (!raw.trim()) return false; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return true; // unparseable — conservatively assume referenced + } + if (!parsed || typeof parsed !== 'object') return false; + const hooks = (parsed as Record)['hooks']; + const table = hooks && typeof hooks === 'object' && !Array.isArray(hooks) + ? (hooks as Record) + : (parsed as Record); + for (const key of Object.keys(table)) { + const entries = table[key]; + if (!Array.isArray(entries)) continue; + for (const entry of entries) { + if (!entry || typeof entry !== 'object') continue; + const entryHooks = (entry as Record)['hooks']; + const hookList = Array.isArray(entryHooks) ? entryHooks : [entry]; + for (const hook of hookList) { + if (!hook || typeof hook !== 'object') continue; + const values = [ + (hook as Record)['command'], + (hook as Record)['commandWindows'], + ]; + for (const value of values) { + if (typeof value === 'string' && /gsd-context-monitor(\.js|\.cmd)?/.test(value)) { + return true; + } + } + } + } + } + return false; +} + +/** + * #2586 must-have #4/#8: after hooks.json registrations for + * CODEX_EXTENDED_HOOK_EVENTS have been reconciled away (by the caller, via + * removeCodexHooksJsonEvent), delete `hooks/gsd-context-monitor.js` and its + * `.cmd` shim ONLY when (a) no surviving hooks.json registration under ANY + * event still references either basename, and (b) the on-disk file carries + * GSD's own ownership markers (a user's hand-edited or unrelated file at that + * path is left alone). Each file is deleted independently — a failure + * deleting one is reported as a warning and never rolls back the (already + * safe, already-written) hooks.json deregistration the caller performed + * first. + */ +function cleanupOrphanedCodexContextMonitorScript(targetDir: string): CleanupCodexContextMonitorResult { + const result: CleanupCodexContextMonitorResult = { deleted: [], warnings: [], stillReferenced: false }; + if (hooksJsonReferencesCodexContextMonitor(targetDir)) { + result.stillReferenced = true; + return result; + } + const candidates = [ + path.join(targetDir, 'hooks', 'gsd-context-monitor.js'), + path.join(targetDir, 'hooks', 'gsd-context-monitor.cmd'), + ]; + for (const candidate of candidates) { + if (!fs.existsSync(candidate)) continue; + if (!isGsdOwnedCodexContextMonitorScript(candidate)) continue; + try { + fs.unlinkSync(candidate); + result.deleted.push(candidate); + } catch (err) { + result.warnings.push({ + path: candidate, + reason: err && (err as Error).message ? (err as Error).message : String(err), + }); + } + } + return result; +} + // --------------------------------------------------------------------------- // Shared: buildHookCommand // --------------------------------------------------------------------------- @@ -3070,6 +3268,9 @@ export = { removeCodexHooksJsonEvent, removeCodexHooksJsonSessionStart, buildCodexHookWindowsShimIR, + cleanupOrphanedCodexContextMonitorScript, + isGsdOwnedCodexContextMonitorScript, + hooksJsonReferencesCodexContextMonitor, // Codex TOML buildCodexHookBlock, diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index b5ada431f..f67306355 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -73,6 +73,8 @@ const { CODEX_SANDBOX_HOLDS, parseTomlToObject, validateCodexConfigSchema, + uninstall, + CODEX_EXTENDED_HOOK_EVENTS, } = require('../bin/install.js'); const { resolveNodeRunner } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); @@ -4253,24 +4255,29 @@ describe('Codex install hook configuration (e2e)', () => { // and the managed-hooks registry. // // The Codex install branch in bin/install.js used to allowlist only two of the -// four hook files the shipped build emits (gsd-check-update.js + -// gsd-context-monitor.js), and gated the entire branch on !isMinimalMode so the +// four hook files the shipped build emitted at the time (gsd-check-update.js + +// gsd-context-monitor.js — the latter permanently removed by #2586, see below), +// and gated the entire branch on !isMinimalMode so the // `core` profile installed none of them. The parent SessionStart hook spawn()s // the worker, which require()s the registry — so Codex was wired to a dependency // chain the same installer never delivered. // // These tests drive the real installer (bin/install.js) behaviorally into an -// isolated temp config dir and assert the complete four-file set is delivered +// isolated temp config dir and assert the complete three-file set is delivered // for both profiles, the registry is byte-for-byte, the version stamps resolve // to the installed package version, and unrelated user files are preserved. // +// #2586 reduced the set back to three: gsd-context-monitor.js read a Claude-only +// statusline bridge file Codex never writes, so it was a guaranteed silent no-op +// on every Codex hook event and was dropped from CODEX_HOOKS_TO_COPY for good. +// // Verified non-duplicate: the pre-existing 'Codex install hook configuration // (e2e)' suite above only asserts gsd-check-update.js delivery/wiring — it never -// asserts on gsd-check-update-worker.js, managed-hooks-registry.cjs, or -// gsd-context-monitor.js delivery, the core/full profile matrix, upgrade-refresh, -// byte-for-byte registry copy, idempotency of the four-file set, user-file -// preservation, or the core-profile negative-space (no agent files) — all -// genuinely distinct assertions this fold adds. +// asserts on gsd-check-update-worker.js, managed-hooks-registry.cjs, the +// core/full profile matrix, upgrade-refresh, byte-for-byte registry copy, +// idempotency of the three-file set, user-file preservation, or the +// core-profile negative-space (no agent files) — all genuinely distinct +// assertions this fold adds. 'use strict'; @@ -4298,12 +4305,14 @@ const { INSTALL_TIMEOUT_MS, } = require('./helpers/timeouts.cjs'); -// The four-file hook set the Codex surface must deliver together (#2695). +// The three-file hook set the Codex surface must deliver together (#2695). +// gsd-context-monitor.js was removed from this set by #2586: it read a +// Claude-only statusline bridge file Codex never writes, so it was a +// guaranteed silent no-op on every Codex hook event. const CODEX_HOOK_FILES = [ 'gsd-check-update.js', 'gsd-check-update-worker.js', 'managed-hooks-registry.cjs', - 'gsd-context-monitor.js', ]; // Build hooks/dist before any install runs (the installer copies from there). @@ -4339,9 +4348,9 @@ function runCodexInstall({ profile, preseed }) { // Older-version stamp used to pre-seed an "upgrade" scenario. const OLDER_VERSION = '1.7.0'; -describe('#2695: fresh Codex installs deliver the complete four-file hook set', () => { +describe('#2695: fresh Codex installs deliver the complete three-file hook set', () => { for (const profile of ['core', 'full']) { - test(`fresh --profile=${profile} installs all four hook files`, (t) => { + test(`fresh --profile=${profile} installs all three hook files`, (t) => { const { configDir, result } = runCodexInstall({ profile }); t.after(() => cleanup(configDir)); @@ -4357,8 +4366,8 @@ describe('#2695: fresh Codex installs deliver the complete four-file hook set', } }); -describe('#2695: Codex upgrades refresh all four hook files to the current version', () => { - // Pre-seed all four files stamped at OLDER_VERSION so an upgrade must overwrite them. +describe('#2695: Codex upgrades refresh all three hook files to the current version', () => { + // Pre-seed all three files stamped at OLDER_VERSION so an upgrade must overwrite them. function olderSeed() { const seed = {}; for (const name of CODEX_HOOK_FILES) { @@ -4373,12 +4382,12 @@ describe('#2695: Codex upgrades refresh all four hook files to the current versi } for (const profile of ['core', 'full']) { - test(`--profile=${profile} upgrade refreshes all four hook files`, (t) => { + test(`--profile=${profile} upgrade refreshes all three hook files`, (t) => { const { configDir, result } = runCodexInstall({ profile, preseed: olderSeed() }); t.after(() => cleanup(configDir)); const hooksDir = hooksDirOf(configDir); - // All four must now carry the current version stamp where one exists, and + // All three must now carry the current version stamp where one exists, and // the registry must no longer be the stale sentinel. for (const name of CODEX_HOOK_FILES) { const dest = path.join(hooksDir, name); @@ -4473,8 +4482,8 @@ describe('#2695: unrelated user-owned hook files are preserved', () => { } }); -describe('#2695: re-running the installer is idempotent for the four-file set', () => { - test('a second full install leaves all four files present and correctly stamped', (t) => { +describe('#2695: re-running the installer is idempotent for the three-file set', () => { + test('a second full install leaves all three files present and correctly stamped', (t) => { const first = runCodexInstall({ profile: 'full' }); t.after(() => cleanup(first.configDir)); // Second run into the SAME config dir. @@ -10939,3 +10948,314 @@ describe('bug-2866: stripStaleGsdHookBlocks handles end-of-file without trailing }); }); } + +// ─── #2586: stop installing Codex context-monitor hooks without metrics ──── +// gsd-context-monitor.js reads a statusline bridge file Codex never writes, +// so every registered event was a guaranteed silent no-op. These tests drive +// the REAL install()/uninstall() entry points against a real temp CODEX_HOME +// — never a hand-fabricated manifest (see PR #2709's Blocker 1: its cleanup +// path was unreachable in production because its tests only exercised a +// fixture, not real installer state). +describe('#2586 Codex context-monitor: stop installing, clean up on reinstall', () => { + const { + cleanupOrphanedCodexContextMonitorScript, + isGsdOwnedCodexContextMonitorScript, + hooksJsonReferencesCodexContextMonitor, + } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + + let codexHome; + + beforeEach(() => { + codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-codex-2586-')); + }); + + afterEach(() => { + cleanup(codexHome); + delete process.env.GSD_ALLOW_SYMLINKED_DEST; + }); + + function hooksJsonPath(home) { + return path.join(home, 'hooks.json'); + } + + function readHooksJson(home) { + const raw = fs.readFileSync(hooksJsonPath(home), 'utf8'); + return JSON.parse(raw); + } + + function monitorScriptPath(home) { + return path.join(home, 'hooks', 'gsd-context-monitor.js'); + } + + function monitorCmdShimPath(home) { + return path.join(home, 'hooks', 'gsd-context-monitor.cmd'); + } + + // uninstall(), unlike install(), does not sandbox CODEX_HOME/HOME itself — + // runCodexInstall's own env-restore in its `finally` means those are back + // to the real environment by the time a bare `uninstall(true, 'codex')` + // would run. Mirrors runCodexInstall's own sandboxing exactly so uninstall + // operates on the temp fixture, never the real ~/.codex. + function runCodexUninstall(codexHome, cwd = path.join(__dirname, '..')) { + const previousCodeHome = process.env.CODEX_HOME; + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + const previousCwd = process.cwd(); + process.env.CODEX_HOME = codexHome; + process.env.HOME = codexHome; + process.env.USERPROFILE = codexHome; + try { + process.chdir(cwd); + return uninstall(true, 'codex'); + } finally { + process.chdir(previousCwd); + if (previousCodeHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodeHome; + if (previousHome === undefined) delete process.env.HOME; + else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = previousUserProfile; + } + } + + // The exact shape a real pre-#2586 install would have written: the shipped + // gsd-context-monitor.js content (with its ownership markers intact) plus + // hooks.json registrations for every CODEX_EXTENDED_HOOK_EVENTS member, + // using the same command-projection shape ensureCodexHooksJsonEvent used + // to write. Built from the real resolveNodeRunner() output, not a literal + // guess at the command string, so a drift in projectManagedHookCommand's + // output shape cannot make this fixture silently stop matching reality. + function seedPreExisting2586Install(home) { + fs.mkdirSync(path.join(home, 'hooks'), { recursive: true }); + // A literal fixture carrying the same ownership markers + // isGsdOwnedCodexContextMonitorScript looks for, built as a string + // (never read+string-matched from the real shipped source file — see + // this repo's "no source grep" test rule). + const fixtureContent = [ + '#!/usr/bin/env node', + '// gsd-hook-version: 1.12.0', + '// Context Monitor - PostToolUse/AfterTool hook', + 'process.exit(0);', + '', + ].join('\n'); + fs.writeFileSync(monitorScriptPath(home), fixtureContent, 'utf8'); + const runner = resolveNodeRunner(); + const hooksSurface = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + for (const eventName of CODEX_EXTENDED_HOOK_EVENTS) { + hooksSurface.ensureCodexHooksJsonEvent(home, eventName, { + absoluteRunner: runner, + platform: process.platform, + }); + } + } + + test('fresh install does not copy gsd-context-monitor.js or register any extended event', () => { + runCodexInstall(codexHome); + assert.strictEqual(fs.existsSync(monitorScriptPath(codexHome)), false, + 'gsd-context-monitor.js must not be copied on a fresh Codex install'); + assert.strictEqual(fs.existsSync(monitorCmdShimPath(codexHome)), false); + assert.strictEqual(fs.existsSync(path.join(codexHome, 'hooks', 'lib', 'hook-exit.js')), false, + 'hook-exit.js is only required by gsd-context-monitor.js — nothing else staged should pull it in'); + assert.ok(fs.existsSync(path.join(codexHome, 'hooks', 'gsd-check-update.js')), + 'gsd-check-update.js must still be staged'); + assert.ok(fs.existsSync(path.join(codexHome, 'hooks', 'gsd-check-update-worker.js'))); + assert.ok(fs.existsSync(path.join(codexHome, 'hooks', 'managed-hooks-registry.cjs'))); + if (fs.existsSync(hooksJsonPath(codexHome))) { + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), false); + } + }); + + test('reinstall removes exact pre-#2586 registrations for every extended event and deletes the orphaned script', () => { + runCodexInstall(codexHome); + seedPreExisting2586Install(codexHome); + assert.ok(fs.existsSync(monitorScriptPath(codexHome)), 'fixture sanity: script seeded'); + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), true, 'fixture sanity: registered'); + + runCodexInstall(codexHome); + + assert.strictEqual(fs.existsSync(monitorScriptPath(codexHome)), false, + 'orphaned gsd-context-monitor.js must be removed once unreferenced and GSD-owned'); + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), false, + 'no hooks.json entry may still reference gsd-context-monitor after reinstall'); + // gsd-check-update's own SessionStart registration must survive untouched. + const hooks = readHooksJson(codexHome); + const sessionStart = (hooks.hooks && hooks.hooks.SessionStart) || []; + const hasCheckUpdate = sessionStart.some((entry) => + (entry.hooks || []).some((h) => typeof h.command === 'string' && /gsd-check-update/.test(h.command))); + assert.ok(hasCheckUpdate, 'gsd-check-update SessionStart registration must remain after cleanup'); + }); + + test('reinstall preserves a hand-customized registration and does not delete a still-referenced script', () => { + runCodexInstall(codexHome); + seedPreExisting2586Install(codexHome); + // Hand-edit ONE event's entry to a shape isManagedHookCommand will not + // recognize (wraps the invocation in a shell script it does not know). + const before = readHooksJson(codexHome); + before.hooks.Stop = [{ hooks: [{ type: 'command', command: 'bash -c "/opt/custom/my-wrapper.sh"' }] }]; + fs.writeFileSync(hooksJsonPath(codexHome), JSON.stringify(before, null, 2) + '\n', 'utf8'); + + runCodexInstall(codexHome); + + const after = readHooksJson(codexHome); + assert.deepStrictEqual(after.hooks.Stop, before.hooks.Stop, + 'a hand-customized registration must survive verbatim'); + }); + + test('reinstall leaves unrelated hooks.json events and config.toml keys untouched', () => { + runCodexInstall(codexHome); + let hooks = fs.existsSync(hooksJsonPath(codexHome)) ? readHooksJson(codexHome) : { hooks: {} }; + if (!hooks.hooks) hooks.hooks = {}; + hooks.hooks.UnrelatedEvent = [{ hooks: [{ type: 'command', command: 'echo unrelated' }] }]; + fs.writeFileSync(hooksJsonPath(codexHome), JSON.stringify(hooks, null, 2) + '\n', 'utf8'); + const configPath = path.join(codexHome, 'config.toml'); + const configBefore = fs.readFileSync(configPath, 'utf8') + '\n[my_unrelated_section]\nfoo = "bar"\n'; + fs.writeFileSync(configPath, configBefore, 'utf8'); + + runCodexInstall(codexHome); + + const after = readHooksJson(codexHome); + assert.deepStrictEqual(after.hooks.UnrelatedEvent, hooks.hooks.UnrelatedEvent, + 'an unrelated event array must be untouched'); + const configAfter = fs.readFileSync(configPath, 'utf8'); + assert.ok(configAfter.includes('[my_unrelated_section]\nfoo = "bar"'), + 'unrelated config.toml section must survive a Codex reinstall'); + }); + + test('a user-owned pre-existing EMPTY event array is preserved, not deleted', () => { + runCodexInstall(codexHome); + fs.writeFileSync(hooksJsonPath(codexHome), JSON.stringify({ hooks: { Stop: [] } }, null, 2) + '\n', 'utf8'); + + runCodexInstall(codexHome); + + const after = readHooksJson(codexHome); + assert.ok(Array.isArray(after.hooks.Stop) && after.hooks.Stop.length === 0, + 'an empty array the user already had must not be dropped by cleanup that found nothing GSD-owned to remove'); + }); + + test('symlinked hooks.json aborts before any Codex change, without GSD_ALLOW_SYMLINKED_DEST', () => { + runCodexInstall(codexHome); + const configPath = path.join(codexHome, 'config.toml'); + const configBefore = fs.readFileSync(configPath, 'utf8'); + const realHooksJson = path.join(codexHome, 'real-hooks.json'); + fs.writeFileSync(realHooksJson, JSON.stringify({ hooks: {} }, null, 2) + '\n', 'utf8'); + fs.unlinkSync(hooksJsonPath(codexHome)); + fs.symlinkSync(realHooksJson, hooksJsonPath(codexHome)); + + assert.throws(() => runCodexInstall(codexHome), /symlink/i); + + assert.ok(fs.lstatSync(hooksJsonPath(codexHome)).isSymbolicLink(), + 'the symlink itself must survive an aborted install — never replaced by a plain file'); + assert.strictEqual(fs.readFileSync(configPath, 'utf8'), configBefore, + 'config.toml must be restored to its pre-attempt snapshot on abort'); + }); + + test('symlinked hooks.json is followed when GSD_ALLOW_SYMLINKED_DEST=1', () => { + runCodexInstall(codexHome); + const realHooksJson = path.join(codexHome, 'real-hooks.json'); + fs.writeFileSync(realHooksJson, JSON.stringify({ hooks: {} }, null, 2) + '\n', 'utf8'); + fs.unlinkSync(hooksJsonPath(codexHome)); + fs.symlinkSync(realHooksJson, hooksJsonPath(codexHome)); + process.env.GSD_ALLOW_SYMLINKED_DEST = '1'; + + assert.doesNotThrow(() => runCodexInstall(codexHome)); + + assert.ok(fs.lstatSync(hooksJsonPath(codexHome)).isSymbolicLink(), 'still a symlink afterward'); + assert.ok(fs.existsSync(realHooksJson), 'the symlink target must have been written through'); + }); + + test('cleanupOrphanedCodexContextMonitorScript keeps the hooks.json deregistration when script deletion fails', () => { + runCodexInstall(codexHome); + seedPreExisting2586Install(codexHome); + for (const eventName of CODEX_EXTENDED_HOOK_EVENTS) { + require('../gsd-core/bin/lib/runtime-hooks-surface.cjs').removeCodexHooksJsonEvent(codexHome, eventName); + } + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), false, 'deregistration committed first'); + + const originalUnlinkSync = fs.unlinkSync; + fs.unlinkSync = (target, ...rest) => { + if (typeof target === 'string' && target.includes('gsd-context-monitor')) { + throw Object.assign(new Error('EPERM: simulated'), { code: 'EPERM' }); + } + return originalUnlinkSync.call(fs, target, ...rest); + }; + // Determine which GSD-owned candidates actually exist BEFORE the mocked + // deletion attempt — on Windows, ensureCodexHooksJsonEvent also staged a + // .cmd shim alongside the .js file (see buildCodexHookWindowsShimIR), so + // both deletions fail under the mock above; on POSIX only the .js file + // exists. Asserting against this rather than a hardcoded 1 keeps the row + // meaningful on both platforms instead of just loosening it to "at least + // one" (see CI failure: Windows reported 2 warnings, not 1). + const cmdShimPath = monitorCmdShimPath(codexHome); + const cmdShimExisted = fs.existsSync(cmdShimPath); + let result; + try { + result = cleanupOrphanedCodexContextMonitorScript(codexHome); + } finally { + fs.unlinkSync = originalUnlinkSync; + } + + const expectedWarningCount = cmdShimExisted ? 2 : 1; + assert.strictEqual(result.warnings.length, expectedWarningCount); + assert.ok(result.warnings.some((w) => /gsd-context-monitor\.js$/.test(w.path)), + 'a warning must name the .js script'); + if (cmdShimExisted) { + assert.ok(result.warnings.some((w) => /gsd-context-monitor\.cmd$/.test(w.path)), + 'a warning must name the .cmd shim when Windows staged one'); + assert.ok(fs.existsSync(cmdShimPath), 'the .cmd shim remains on disk since its deletion failed too'); + } + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), false, + 'the already-safe hooks.json deregistration must not be reverted by a script-deletion failure'); + assert.ok(fs.existsSync(monitorScriptPath(codexHome)), 'the file remains on disk since deletion failed'); + }); + + test('isGsdOwnedCodexContextMonitorScript rejects a user file at the same path', () => { + runCodexInstall(codexHome); + fs.mkdirSync(path.join(codexHome, 'hooks'), { recursive: true }); + fs.writeFileSync(monitorScriptPath(codexHome), '#!/usr/bin/env node\nconsole.log("my own script");\n', 'utf8'); + assert.strictEqual(isGsdOwnedCodexContextMonitorScript(monitorScriptPath(codexHome)), false); + }); + + test('uninstall removes recognized registrations and the orphaned script symmetrically with install', () => { + runCodexInstall(codexHome); + seedPreExisting2586Install(codexHome); + + runCodexUninstall(codexHome); + + assert.strictEqual(fs.existsSync(monitorScriptPath(codexHome)), false); + if (fs.existsSync(hooksJsonPath(codexHome))) { + assert.strictEqual(hooksJsonReferencesCodexContextMonitor(codexHome), false); + } + }); + + test('uninstall does not throw on an unmodeled hooks.json event-value shape', () => { + runCodexInstall(codexHome); + fs.writeFileSync(hooksJsonPath(codexHome), JSON.stringify({ hooks: { Stop: 'not-an-array' } }, null, 2) + '\n', 'utf8'); + assert.doesNotThrow(() => runCodexUninstall(codexHome)); + }); + + test('property: reconcileCodexHooksJsonEvent never removes a non-managed-shape command', () => { + const { reconcileCodexHooksJsonEvent } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + fc.assert( + fc.property( + fc.array(fc.string({ minLength: 1, maxLength: 40 }).filter((s) => !/gsd-context-monitor|gsd-check-update/.test(s)), { minLength: 1, maxLength: 5 }), + (customCommands) => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-codex-2586-prop-')); + try { + const seeded = { hooks: { Stop: [{ hooks: customCommands.map((c) => ({ type: 'command', command: c })) }] } }; + fs.writeFileSync(path.join(home, 'hooks.json'), JSON.stringify(seeded, null, 2) + '\n', 'utf8'); + reconcileCodexHooksJsonEvent(home, 'Stop', { managedCommand: null }); + const after = JSON.parse(fs.readFileSync(path.join(home, 'hooks.json'), 'utf8')); + const survivingCommands = ((after.hooks && after.hooks.Stop) || []) + .flatMap((entry) => (entry.hooks || []).map((h) => h.command)); + for (const c of customCommands) { + assert.ok(survivingCommands.includes(c), `non-managed command "${c}" must survive removal`); + } + } finally { + cleanup(home); + } + }, + ), + { numRuns: 25 }, + ); + }); +}); diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index fc98e0899..863e061c5 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -539,10 +539,6 @@ "gsd-core/workflows/verify-work/steps/mvp-uat-framing.md", "hooks/gsd-check-update-worker.js", "hooks/gsd-check-update.js", - "hooks/gsd-context-monitor.js", - "hooks/lib/cli-exit.js", - "hooks/lib/exit-code-registry.js", - "hooks/lib/hook-exit.js", "hooks/managed-hooks-registry.cjs", "hooks/package.json", "scripts/changeset/README.md", diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index 60f7b9409..e02515023 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -2996,31 +2996,21 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ return path.join(configDir, 'hooks'); } - test('the installed context-monitor hook LOADS AND RUNS, not merely exists', () => { + test('#2586: gsd-context-monitor.js is no longer staged for Codex at all', () => { + // Was: "the installed context-monitor hook LOADS AND RUNS, not merely + // exists" — that row's premise (Codex ships this hook) is exactly what + // #2586 removes: its only documented metrics source is Claude's own + // statusline hook, which Codex never installs, so every registered Codex + // event was a guaranteed silent no-op. The #4087 bug class this describe + // block guards (a staged hook requiring an unshipped hooks/lib/ helper) + // remains covered live via Windsurf's own guards — see the + // "#4087 review: Windsurf install..." describe block below, unaffected + // by this change. const hooksDir = installCodex(tmpDir); - const hook = path.join(hooksDir, 'gsd-context-monitor.js'); - assert.ok(fs.existsSync(hook), 'precondition: the hook itself must be staged'); - - // The actual defect. Before the fix this exited 1 with - // "Cannot find module './lib/hook-exit.js'". - // `exitCode`, not `status`: the process seam returns its own shape - // ({outcome, exitCode, stdout, stderr, ...}) and `status` reads undefined — - // which would compare unequal to 0 and pass this row for the wrong reason - // if the polarity were ever flipped. - const result = runNode([hook], { timeoutMs: 30000, input: '{}', env: { ...process.env } }); - assert.strictEqual( - result.outcome, 'exited', - `the hook must run to completion, not time out or be killed. outcome=${result.outcome}`, - ); - assert.strictEqual( - result.exitCode, 0, - 'the installed Codex hook must load and exit 0 — a MODULE_NOT_FOUND at load fires on every ' - + `registered event and is invisible to the installer's own exit code. stderr: ${result.stderr}`, - ); - assert.doesNotMatch( - String(result.stderr || ''), /MODULE_NOT_FOUND|Cannot find module/, - 'no missing-module error may reach stderr', - ); + assert.strictEqual(fs.existsSync(path.join(hooksDir, 'gsd-context-monitor.js')), false, + 'gsd-context-monitor.js must not be staged for Codex post-#2586'); + assert.strictEqual(fs.existsSync(path.join(hooksDir, 'lib')), false, + 'hooks/lib/ must not exist at all — nothing else Codex stages requires a lib/ helper'); }); // AC4: this is the row that stops the bug recurring. It derives the @@ -3052,11 +3042,17 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ if (!/\.(js|cjs)$/.test(entry)) continue; scan(fs.readFileSync(full, 'utf8'), seedRe); } - assert.ok( - required.size > 0, - 'precondition: at least one staged Codex hook must require a ./lib/ helper — if this ever ' - + 'goes to zero the bundle changed and this row silently stops testing anything', - ); + // #2586: gsd-context-monitor.js was the only staged Codex hook requiring + // a ./lib/ helper; it is no longer staged for Codex at all, so the + // dependency closure is correctly empty. This row still proves the + // GRAMMAR holds (whatever IS required must be staged) — it is just that + // "whatever is required" is now the empty set for Codex specifically. + // The non-trivial case (closure size > 0) is covered live by the + // "#4087 review: Windsurf install..." describe block below. + assert.strictEqual(required.size, 0, + 'no staged Codex hook should require a ./lib/ helper post-#2586 — if this becomes non-zero, ' + + 'extend this row (do not just raise the bar back to ">0") so the new dependency stays proven'); + assert.strictEqual(fs.existsSync(libDir), false, 'hooks/lib/ must not exist when nothing requires it'); // Walk to a fixed point, exactly as the installer must. const checked = new Set(); @@ -3161,7 +3157,12 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ const hooksDir = installCodex(tmpDir); const libDir = path.join(hooksDir, 'lib'); const stagedLibs = fs.existsSync(libDir) ? fs.readdirSync(libDir).sort() : []; - assert.ok(stagedLibs.length > 0, 'precondition: some helper must have been staged'); + // #2586: Codex's closure is now legitimately empty (gsd-context-monitor.js, + // the only staged Codex hook that ever required a helper, is no longer + // staged) — the deepStrictEqual below is still the real assertion and + // holds for the empty case too; the non-empty case remains covered live + // by the "#4087 review: Windsurf install..." describe block below. + assert.strictEqual(stagedLibs.length, 0, 'no helpers should be staged for Codex post-#2586'); // Derive the closure independently of the installer. const seedRe = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g;