[malibio]

What

Codex gets what the Claude Code and OpenCode plugins deliver: the project, orientation, consent rules and skill list at session start; a note when a skill changes; and a refusal of the next tool call once the watched task is finished elsewhere.

• Codex runs each hook event as its own command, so the shared session module gains snapshot() and a restore argument, and plugins/codex/hook.ts keeps the state in a file per thread. The session logic is reused, not copied.
• The installer registers one handler per event in ~/.codex/hooks.json (other entries untouched; uninstall removes only its own), prints the /hooks trust step, and keeps the instructions block for sessions whose hooks are not trusted.
• A hook cannot set a command's environment, so the CLI names a write's journal after CODEX_THREAD_ID when NODESPACE_WRITE_JOURNAL is unset. The hook's session_id is the same id.
• scripts/eval/harness/codex.ts drives the scenario; trace.ts reads Codex events.

Live result (Codex 0.162.1)

Probe, mid-session standard edit, and task-finished-elsewhere all held; the session's own writes were not read as foreign. Details are in the issue comment.

Not in this PR

Pi and Antigravity drivers and runs, Antigravity hook research, and the status duty (Codex has no status line; only a systemMessage is emitted). These stay open on #3748.

Tests

bun run test:changed: scripts, skill, types and node-types checks pass. One nodespace-agent PTY test (ultra_short_lived_processes_are_pruned) failed under load and passes alone; it is unrelated to this change.

🤖 Generated with Claude Code

[malibio]

Pragmatic review: verdict approve with improvements, no blockers. Full findings are in the review below.

[malibio]

Code Review Summary

Verdict: APPROVE WITH IMPROVEMENTS (no blockers; this is a net improvement and the core design is sound). Posted as a comment review because the PR is self-authored.

The shape is right: one hook command per event, state in a per-thread JSON file, the shared session module reused via snapshot() / restored rather than copied, user hooks preserved, a malformed hooks.json left untouched, the instructions block kept for untrusted-hook sessions, and the /hooks trust step printed. The rule checks pass: no compat code, no suppression, no raw console in production code, no node-type or lifecycle literals, no Sync-boundary markers, and the README is an existing package README. The Rust change is small and well tested.

Acceptance criteria (Codex part of #3748):

• Install and uninstall give and remove hooks: met (codex-hooks.test.ts).
• Project, orientation, consent rules and skill list at start: met per the live run; the hook-process test only covers the unreachable path.
• Skill edited mid-session announced on the next prompt: met per the live run; no automated test through the hook process.
• Task finished elsewhere refuses the next call: met per the live run, and at the session level by the snapshot round-trip test.
• Installer states the /hooks step and the block remains: met.
• State survives between invocations and shared logic is reused: met, with the caveats below.
• Status duty settled and recorded: met (not available, systemMessage only). The issue comment says the systemMessage is "not yet observed in the UI", the one unverified claim.
• Codex driver and Codex column: met (issue comment). Pi, Antigravity and the completed table correctly remain open.

Findings

Critical Issues

None.

Suggested Improvements

• [Improvement] packages/skill/plugins/codex/hook.ts, SessionEnd vs resume. The issue comment records that SessionEnd fires per codex exec process, and the handler deletes the state file. The next codex exec resume <thread> then arrives as SessionStart with source: resume and saved === null, so the isContinued branch is never taken and session.start() runs from scratch. That resets fetched, listVersion, item and blocked, and start() calls endJournal(), wiping the thread's journal (the session's own earlier writes). The scenario passed, presumably because the item is re-learned within a turn, but a watched item that spans two exec invocations loses its watch and a refusal is forgotten across turns. The continuity logic and the SessionEnd cleanup contradict each other. Suggest: on SessionEnd, call session.end() for the journal but keep the state file (the 7-day prune bounds it), and add a test that drives SessionStart(startup), SessionEnd, SessionStart(resume) and asserts the state carries over.
• [Improvement] hook.ts, concurrent tool calls. Codex can run tool calls in parallel, and every PreToolUse/PostToolUse is load, mutate, write the whole file. The per-pid temp name makes the rename atomic and never torn, but it is last-writer-wins: a PreToolUse that loaded before a PostToolUse saved can overwrite the updated fetched / item / lastCheckedAt, or drop a pendingNote or a refusal (blocked). The consequence is a missed or repeated note, or a refusal that lands one call late, not corruption. Cheapest mitigation: skip the write when the snapshot equals what was loaded (PreToolUse is the hot path and usually changes nothing, which also removes most of the race window); a lock file or re-read-and-merge would close the rest. At minimum record the limitation in the README.
• [Improvement] hook.ts load(). parsed.session as unknown as SessionState is an unchecked cast. A wrong-typed field (a missing one is handled by ??) throws inside createSession/session.*, is swallowed by main, and the session silently never works again until the file is pruned. Validate the few fields, or treat a throw on restore as "no state" and rewrite. Per project policy no shape-migration code: a plain reset is the right behaviour.
• [Improvement] hook.ts ensureStarted. It starts a session only when no state file exists. A session whose first start() was unreachable / no-cli (daemon not up yet) is saved with isReached: false and is never retried for the thread's life, because saved is non-null from then on. Retrying start() when isReached === false (rate-limited by the watch interval) is cheap. Also note a mid-session start() (hooks trusted late) calls endJournal() under the thread id and discards writes already journaled; harmless today because the item is not yet known, but worth a comment.
• [Improvement] Tests: not everything asserted is exercised. codex-hooks.test.ts runs the real script only for SessionStart (unreachable), UserPromptSubmit ({}), SessionEnd and a bad id. The hook's own output shapes are never asserted through the process: permissionDecision: "deny" plus reason, PostToolUse additionalContext from pendingNote, the UserPromptSubmit note + section join, and shellLineOf / outputOf (the Bash tool name and tool_response shapes). Those are exactly the contract with Codex. A fake nodespace on PATH (the test already fakes git) would let a test drive PreToolUse to a deny and PostToolUse to a note. The command wrapper (command -v bun / node --experimental-strip-types branch, and quoting of a path containing a space) is also untested; the test runs hook.ts directly with node. The bad-id test also asserts only empty output, not that no file was written.
• [Improvement] packages/cli/src/journal.rs, precedence and nesting. The precedence is correct and tested (NODESPACE_WRITE_JOURNAL wins, empty falls through). The residual mis-attribution risk is the inverse of the one the README documents: a non-Codex harness started from a shell inside a Codex command inherits CODEX_THREAD_ID, and any harness whose plugin variable is unset (plugin absent or untrusted) journals its writes under the Codex thread's file, where the Codex hook reads them as its own and can suppress a genuine foreign-change refusal. Low probability and bounded, but the variable is read unconditionally for every CLI write. Document it next to the constant (the comment currently says only that hooks cannot set the variable), or scope it.
• [Improvement] packages/skill/src/installer.ts. (1) Handlers are found by command.includes('"<script path>"'). If CODEX_HOME moved since the last install, the old handlers carry a different path and are neither replaced nor removed by uninstall, and Codex then warns on every event for a missing script. Uninstall already reads the record's plugin root for files; use the recorded path for the hook match too. (2) The script path is interpolated into a sh command inside double quotes, so a CODEX_HOME containing ", $ or a backtick is interpreted by the shell. Single-quote with '\'' escaping, or reject such a path. (3) needsHookTrust is set on every install with a hooks file, so the /hooks message prints again on a no-op reinstall; set it only when the handler set changed.

Nitpicks

• Nit: removeHooksFile deletes hooks.json when it holds only {"hooks": {}}, which may be a file the user created empty. The install record could say whether the file existed beforehand.
• Nit: scripts/eval/harness/codex.ts does realpathSync(process.env.NODESPACE_HOME ?? ""); an unset variable throws a bare ENOENT. scenario.ts already enforces it; use the validated home.
• Nit: main and load() swallow every error silently. Never breaking Codex is right; a debug-gated stderr line would make a dead hook diagnosable.
• Nit: the driver uses the user's real CODEX_HOME (with their other hooks) and sets network_access=true. The rationale (a copy of the credential is a credential) is documented and the widening is narrow (NodeSpace home and .git), so this is acceptable. Deleting NODESPACE_WRITE_JOURNAL from the child env is correct for the reason stated.

Checked and sound

• Session id is validated against ^[A-Za-z0-9_-]{1,128}$ before it forms a path (no traversal); the Rust side applies is_session_id too.
• State is written to a temp file in the same directory and renamed, so a reader never sees a partial file.
• Output shapes match the contract: hookSpecificOutput.hookEventName + additionalContext, PreToolUse permissionDecision: "deny" with a reason, top-level systemMessage. SessionEnd exists in the 0.162.1 binary, so registering it is valid.
• Restored fields: lastCheckedAt is epoch ms from host.now() and comparable across processes; blocked and opening are intentionally carried and cleared by prompt(); nothing restored is unsafe beyond the unvalidated-cast point.
• Per-call cost: the CLI is spawned only when the watch interval has elapsed; the unconditional state write on every PreToolUse is the main avoidable cost.

[malibio]

Review addressed (87d9334)

Addressed

• SessionEnd fires per codex exec process and wiped the state a resume continues from: the handler is gone, and a state file no event touched for a week is pruned. A resume test covers it.
• Concurrent tool calls: a write that changes nothing is skipped, so it cannot undo another call's note, refusal or fetched skills. Two writes that both change something can still race; that is documented as last-writer-wins, bounded by the next prompt re-deriving most state.
• A restored state file is validated; an unusable one starts the session again.
• A prompt starts a session that could not reach NodeSpace. A tool call does not: that would cost every call a timeout while the daemon is down.
• The script path is single-quoted in the hook command.
• Hook-process tests over a fake nodespace and git, through the registered shell command: the deny output shape, PostToolUse additionalContext delivered once, the prompt note with the list, resume, a missing CLI, an unusable state file.
• The nested-harness journal caveat is documented both ways; the driver reads the scenario home instead of an unset env; NODESPACE_HOOK_DEBUG prints a swallowed error.

Skipped

• Handlers left behind when CODEX_HOME moves: they sit in the old home's hooks.json, which Codex no longer reads. Revisit if the hooks file ever stops moving with the home.
• The trust line printed on a no-op reinstall: whether the hooks are trusted is not knowable from the installer, so the reminder is the safe default.
• hooks.json holding only {"hooks": {}} deleted on uninstall: it is the file's only content, and recreating it is a no-op.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: Dropping SessionEnd and changing what a restore trusts are behaviour changes to how state lives across processes, and several fixes interlock (what is written, when it is skipped, when a session starts). The new tests pin them, but a second pair of eyes should check the interaction.

[malibio]

Re-review of 87d9334 (Codex hooks)

Verdict: APPROVE. Every earlier finding is addressed or skipped with a sound reason. Two small things are worth a follow-up in this PR or the next; neither blocks.

Earlier findings

Finding	Status
SessionEnd wiped state and journal that codex resume continues from	Fixed: handler, registration and README row gone; resume test added. Pruning moved to a 7-day mtime sweep.
Concurrent tool calls clobbering each other's state	Fixed for the no-change case (see 1 below for one hole). Changing-write races remain last-writer-wins, documented.
Restored state unvalidated	Fixed: isState.
Server unreachable at start never retried	Fixed for prompts, deliberately not tool calls.
Double-quoted path in the command	Fixed (see 3 for the old form).
Tests did not run the registered command	Fixed (see 4).
Skipped items (CODEX_HOME moves, trust line on no-op reinstall, hooks.json emptied)	Agree with all three.

Findings

Improvement

1. Skip-unchanged-write hides the write that keeps a resumed file alive (plugins/codex/hook.ts, SessionStart branch). load() reads the file, then pruneStale() runs, then save(..., before). For resume/compact on a thread idle more than 7 days, pruneStale deletes the very file just loaded (its mtime is old), the session is continued unchanged, text === before, and save is skipped. Result: the file is gone, the next event finds no state and silently restarts the session, dropping blocked, pendingNote and the held item. Before this commit the unconditional write masked it. Fix: have pruneStale skip path (or do it before load), and add a test with an aged file. Everywhere else the skip is sound: before is the exact text this process read, so it only skips when this process added nothing, and a rename is atomic so a half-written read cannot occur. The one cost is that an unchanged file's mtime no longer refreshes, which only matters at the 7-day boundary above.
2. isState is shallower than its comment. skills and fetched are checked as arrays and item/project as records, but not their members. A stale-shaped item (for example without parts or fields) passes and throws inside checkItem. The throw is swallowed in main, no state is saved, and every later event fails the same way, silently, until the file is a week old. Pre-release this only matters across a state-shape change, and the answer is "reset", so I would not widen the validator; but the doc comment ("a stale shape starts the session again") is stronger than what it enforces. Either narrow the comment or check item.id/item.parts/item.fields and skills[].id. isState does not reject any state snapshot() produces.

Notes (no action needed)

• ensureStarted(isPrompt): no loop; at most one start() per prompt, and only while isReached is false. Cost per such prompt is --version plus diagnostics, 5s each, inside the 30s hook timeout. isReached never goes back to false except through start(), so a once-reached session is never restarted by this path. The mid-thread start() (file lost or rejected by isState) does run endJournal and zeroes fetched, item and blocked; the thread's own earlier writes then read as foreign until the next check re-baselines. That is the intended "start again" and is bounded.
• The recovery start passes opens: false, so a task launched while the daemon was down is never opened once the daemon returns. Acceptable; say so in the comment if intended.
• Journals are now only pruned when some session starts and journal end runs. A resume after more than an hour idle with another session start in between loses the journal, so own writes made after the last check could read as foreign. Narrow.

3. Old double-quoted handlers (packages/skill/src/installer.ts)

quotedPath is correct, including a path with ' (' becomes '\'') and $, backticks, spaces. But withoutOurs matches '<path>', so a handler written by the previous commit ("<path>") is not recognised: a reinstall appends the new handlers and leaves the old, so each event runs the hook twice (racing writes, doubled CLI calls), and uninstall leaves them. The old SessionEnd handler also stays but is now a harmless no-op. Not a concern for users (nothing shipped), but ~/.codex/hooks.json on the author's machine holds exactly the double-quoted form: remove it by hand (or uninstall with the previous commit's build) before reinstalling. No compatibility code is warranted.

4. Tests (packages/skill/src/tests/codex-hooks.test.ts)

• They do run the registered command: sh -c <command> per event via execFileSync, one process each, state carried only through the file, HOME and NODESPACE_HOME in the temp dir, a stub nodespace and git first on a minimal PATH, and a symlinked interpreter so a machine-installed nodespace is not found. The deny shape, once-only PostToolUse delivery, resume, the unusable-file path and the missing-CLI retry are covered.
• Machine dependence is small: /usr/bin:/bin for sh/cat/printf and Node 22.6 or later (or bun) for --experimental-strip-types. A /usr/bin/nodespace would break the missing-CLI test.
• Nit: no test pins the skip-unchanged write (delete the skip and nothing fails) or the stale prune (which would have caught finding 1). quotedPath is exercised only on a plain path; one case with ' and a space, executed through sh, would pin the escaping.

Nothing new introduced

The NODESPACE_HOOK_DEBUG stderr path, the nested-harness README note and the driver's nodespaceHome read are fine.

[malibio]

Re-review of 87d9334, verdict APPROVE. All earlier findings are addressed or soundly skipped. Two non-blocking improvements are in the summary comment: (1) pruneStale can delete the state file of a thread resumed after more than 7 days and the skip-unchanged save then hides the write; (2) isState is shallower than its comment claims. The author's hooks.json still holds the old double-quoted handlers and needs a manual clean before reinstall.

[malibio]

Re-review addressed (d554b03)

Addressed

• Improvement 1 (a resume after a week idle read the file, pruned it, then skipped the write): pruneStale skips the thread being continued, and an unchanged save touches the file, so its age is the thread's idle time. A test with an aged file covers it; it fails with the fix removed.
• Improvement 2: isState now checks the members a restored session reads (item id, paths, parts and fields; skill ids; project id). A test with an item missing its parts starts the session again.
• Tests: the registered command run in a home whose path holds a space and a single quote.

Not changed

• Old-form (double-quoted) handlers: nothing has shipped, and the only install is the author's own ~/.codex/hooks.json, cleaned by hand. No compatibility code.
• opens: false on the recovery start: a task launched while the daemon was down is not opened later. Intended; the user's next prompt is the entry point.
• A /usr/bin/nodespace on the test machine would break the missing-CLI test: the PATH holds only the interpreter's directory and the system's.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Two targeted fixes to what the re-review found, each pinned by a test (one mutation-checked), and one new test. No new behaviour beyond what was approved. The suite passes (281 skill tests; quality:fix clean).