hooks: per-handler failClosed and PreToolUse on write_stdin by thedancingdeveloper · Pull Request #6 · TheDancingDeveloper-org/codex

thedancingdeveloper · 2026-10-09T11:03:44Z

Summary

WI-1129. failClosed is a per-handler opt-in on command hooks, defaulting to false. When true, every PreToolUse failure mode blocks the tool with a reason the model sees instead of failing open: a non-0/2 exit, a missing status code, a spawn failure (missing or unreadable command), a timeout, a crash or signal, invalid JSON, and unreadable output. Handlers without the flag are unchanged.

A non-empty write_stdin now emits a PreToolUse payload as tool_name: Bash with the stdin text as command, so text typed into an open shell is reviewed before it reaches the process. Empty background polls emit nothing. A block is returned as a tool error before the handler runs, so it stops the write.

Test plan

cargo test -p codex-hooks --lib (191 passed), including one test per failure mode plus the fail-open regression
cargo test -p codex-config --lib (361 passed), including the failClosed default and round trip
cargo test -p codex-core --lib unified_exec::tests (38 passed), covering non-empty and empty write_stdin

thedancingdeveloper

Review (epic lead, WI-1129, security-critical). Blockers on head e2a25ef:

failClosed silently does nothing on an async handler. The conversion is gated on handler.can_apply_control_effects(), which is execution_mode() == Sync (hooks/src/engine/mod.rs:159). A managed hook written as async = true, failClosed = true therefore fails open with no warning, which is exactly the misconfiguration this flag exists to prevent. Fix at discovery: when failClosed is set, either force the handler to Sync, or refuse the combination loudly with record_load_failure and a clear message. Don't skip the hook silently, because a skipped guard also fails open. Add a test.
write_stdin PreToolUse doesn't carry the shell's directory. The payload is {command: chars} only, so a guard judges typed text against the turn cwd. That is the WI-1128 gap reopened for the stdin path. Example:
- exec_command {cmd: "bash", tty: true, workdir: "/other/dirty-repo"}
- then write_stdin "git checkout -- .\n"
- The hook sees the turn cwd (clean start repo) and allows it, and the dirty repo is wiped.
Fix: include "workdir", the cwd the exec process was started in, looked up by session_id/process_id in the unified-exec process manager (store it at spawn if it isn't kept today). If the process can't be found, emit "workdir": null, and make the gitguard hook treat a Bash payload with null workdir conservatively (I'll require that on the msp-klaudia side). Document the residual in the PR: a cd typed inside the shell isn't tracked.

Verified:

The opt-in defaults to false, and handlers without the flag fail open exactly as before (regression test over six failure kinds).
Each failure mode (non-0/2 exit, exit 2 without a reason, no status code, spawn failure, timeout, signal, invalid JSON, non-UTF-8 output) becomes Blocked with a reason the model sees. One test each.
failClosed propagates through every Command discovery path, including managed requirements.toml.
An empty write_stdin (a poll) emits nothing. A block stops the write.

Non-blocking:

MCP-tool hooks hard-code fail_closed: false, and the field only exists on Command handlers. Say so in the config doc comment.
Note in the PR that a hook which never runs (untrusted user or project hook, or the hooks feature disabled) can't fail closed. Managed hooks are trusted automatically, which is why WI-1146 installs gitguard there.

thedancingdeveloper

Review (epic lead, WI-1129, security-critical). One blocker on head 52bbb57, now rebased on fork/main 99a1e42.

Resolved:

Async + failClosed. The handler is forced to run synchronously, with a load warning, and fail_closed_forces_an_async_handler_to_run_synchronously covers it. It never fails open.
write_stdin carries workdir. It is the exec process's start cwd, or null when the process is gone, the cd residual is documented, and start_cwd_for_process is tested.

Blocker:

The sync-over-async lookup in exec_process_start_cwd is fragile, and bigger than it needs to be. It does tokio::spawn, then a helper thread with recv_timeout(2s), then join(), and join() blocks the calling runtime thread for up to 2 s.
- On a current-thread runtime (e.g. a #[tokio::test] default) the spawned lookup can't run while the thread is blocked. It always times out, so every non-empty write_stdin stalls for 2 s and gets null.
- Outside a runtime context, tokio::spawn panics.
The production runtimes look multi-threaded, so this works today, but it depends on that. process_store is a tokio::sync::Mutex, and its try_lock() is synchronous. Replace the whole helper with:
```
self.process_store.try_lock().ok()?.processes.get(&id).map(|e| e.cwd.clone())
```
That is a sync start_cwd_for_process_now. On contention it returns None, so the payload gets "workdir": null, which the gitguard hook treats conservatively (WI-1136 follow-up). No spawn, no thread, no stall, and about 20 fewer lines. Keep the existing test, and add one asserting a non-blocking None while the lock is held.

Merge condition: fork-ci green on the new head. Run 37925927681 was pending at review.

thedancingdeveloper

Review: no blockers (head ae7b7b4)

The last blocker is fixed. start_cwd_for_process is now synchronous over process_store.try_lock() and returns None (so "workdir": null) when the process is gone or the store is busy. The spawn, thread and join helper is gone. A test shows the lookup returns None within 200 ms while the lock is held.

Summary for WI-1129:

failClosed is an opt-in, default false, and forces async handlers to run synchronously.
Every failure mode blocks, with a test each. Handlers without the flag fail open exactly as before.
A non-empty write_stdin runs PreToolUse as Bash with the process's start cwd, or null.

The WI-1136 follow-up (gitguard treating null workdir conservatively) is recorded on WI-1136.

Merge condition (security-critical): fork-ci run 37928042460 on this head is green.

thedancingdeveloper

Review: no blockers (head 19fc871)

The only change since ae7b7b4 is rustfmt: import order, and a line wrap in a test. Everything in the previous review holds. Merge condition (security-critical): fork-ci run 37928905215 on this head is green.

thedancingdeveloper

Review: no blockers (head e367439)

The only change since 19fc871 is rustfmt in the hooks crate, which fixes the fmt failure in run 37928905215. No logic changed, so the security review stands. Merge condition: fork-ci run 37929929778 on this head is green.

thedancingdeveloper

Review (epic lead). One blocker on head 72e7308: a scope issue, not a logic one.

Commit 72e7308 edits upstream code unrelated to WI-1129 (app-server/src/message_processor.rs:371, a redundant binding) only to get fork-ci's clippy -D warnings green.
- That lint debt exists upstream at the tag. Clippy only flags it because this PR touches the codex-app-server package (config_processor.rs), so fork-ci clippies the whole package.
- Fixing upstream debt inside our patches grows the carried diff and adds rebase conflicts. This hunk is also 13 lines from message_processor.rs:358, where WI-1131 swaps the instructions provider, so the instructions lane would conflict with it.
- Fix: drop 72e7308 from this PR. fork-ci is changing to fail only on clippy warnings in files the PR changes (requested from codex-fork-infra under WI-1123). Once that lands on fork/main, rebase, and CI judges #6 on its own lines.

The security content is unchanged and still has no blockers (see the review at ae7b7b4 and its rustfmt-only follow-ups).

thedancingdeveloper · 2026-10-09T13:34:50Z

fork-ci now only fails clippy on diagnostics in files the PR changes (PR #11, head 5f9d3efe8b, rebased onto current fork/main so it is a single commit). The let_and_return in message_processor.rs:372 is on fork/main and not in this PR's diff, so it will no longer fail the check.

Once #11 is merged, please rebase this branch onto fork/main without commit 72e73083 (the lint fix). The probe that must fail is run 37937788126 on fork/ci-lint, which adds one let_and_return in a file the branch itself changes.

thedancingdeveloper

Review: no blockers (head d7f233c)

This is a rebase onto fork/main 219664f. git range-diff shows the five WI-1129 commits identical (=) to the reviewed ones up to e367439, and 72e7308 is dropped, so message_processor.rs is no longer touched. That resolves the scope blocker. The security review of failClosed and write_stdin stands. Merge condition (security-critical): fork-ci green on this head. Upstream lint debt in unchanged files no longer counts.

thedancingdeveloper

Review: blocker (head d7f233c)

fork-ci run 37947113324 fails cargo test -p codex-app-server-protocol: schema_fixtures_tests::{json_schema,typescript_schema,experimental_precomputed_exports}_fixtures_match_generated. WI-1129 adds failClosed to ConfiguredHookHandler (v2/config.rs), so the generated TS and JSON fixtures change (+failClosed: boolean in v2/ConfiguredHookHandler.ts), but the committed fixtures weren't regenerated. Regenerate them with the repo's fixture writer (see app-server-protocol/src/schema_fixtures_tests.rs for the command or env var it expects), commit the fixture diff in its own commit, and check that it touches only the hook-handler types. The security content is unchanged.

thedancingdeveloper

Review: no blockers (head 3d5d91b)

The fixture blocker is fixed. The one new commit, 3d5d91b, regenerates the generated schema artefacts, and every textual hunk is the new failClosed field:

the JSON schemas (default: false, boolean, described);
v2/ConfiguredHookHandler.ts;
the Python SDK's generated fail_closed alias;
the precomputed .zst export, which is binary and gains 111 bytes.

The five WI-1129 commits are unchanged from d7f233c, and the security review stands. Merge condition (security-critical): fork-ci run 37953281365 on this head is green.

thedancingdeveloper

Review: no blockers (head bfefd48)

Rebased onto fork/main feaf20e. git range-diff shows the six previously reviewed commits identical (=). The two new commits are both fixture regeneration for failClosed only:

f945633: the experimental precomputed app-server export (.zst, binary, +110 bytes);
bfefd48: core/config.schema.json gains the failClosed boolean, defaulting to false. Its description also covers the non-blocking notes from my earlier review: MCP-tool hooks always fail open, a hook that never runs can't fail closed, and async is forced to run synchronously.

The security review of WI-1129 stands. Merge condition: fork-ci green on this head.

thedancingdeveloper

Review: no blockers (head e040b6c)

This is a rebase onto fork/main ef97503, which includes #17's workspace bin build. git range-diff against the previously reviewed head shows every commit identical (=): 8 commits; the WI-1129 security review stands. Merge condition: fork-ci green on this head.

cargo test -p runs the app-server and core integration suites without the serialization, retries and slow timeouts in codex-rs/.config/nextest.toml, so #6 and #10 fail tests that are unrelated to their changes. Mirror rust-ci-full-nextest-platform.yml: install bubblewrap, install cargo-nextest 0.9.103 from the same taiki-e/install-action pin, and run `cargo nextest run -p` so that config applies. RUST_MIN_STACK and the workspace bin build stay. The cli skip moves from a cargo --skip to a nextest filterset read from .github/fork-ci-known-failures.txt. That file lists only tests shown to fail on unmodified fork/main, each with the baseline run that proved it, and it starts empty until that baseline exists.

These four tests fail on PRs #6 and #12 in files those PRs do not touch, and they are green on the fork/main baseline. List them so fork-ci can keep reporting real regressions.

failClosed is opt-in and defaults to false. When set, every PreToolUse failure mode (non-0/2 exit, missing status, spawn failure, timeout, signal, invalid JSON, unreadable output) blocks the tool with a reason the model sees. Handlers without the flag still fail open. A non-empty write_stdin is typed into the open shell, so it now runs PreToolUse as tool_name Bash with the stdin text as command. Empty polls stay unreviewed. A block stops the write before it reaches the process.

An async handler that sets failClosed now runs synchronously, with a warning, instead of failing open silently. The block can only be applied before the tool starts, so skipping the hook would leave no guard. A non-empty write_stdin payload now carries the exec process's start cwd as workdir, looked up by session_id, or null when the process is gone. A cd typed into the shell is not tracked, and the hook config documents that MCP tool hooks and hooks that never run cannot fail closed.

The write_stdin hook reads the process store with try_lock, so a busy store reports workdir as null instead of blocking a runtime worker. The previous lookup spawned a task and joined it from another thread, which deadlocked on a current-thread runtime and panicked outside one. Also fills the two HookHandlerConfig literals CI found in the tui and app-server tests.

cargo fmt wants the HookToolName import grouped with the other tool imports and the long json! argument wrapped.

cargo fmt reflows the fail-closed condition, the async warning assertion, and the timeout test call, and orders the test imports.

ConfiguredHookHandler gained failClosed, so the vendored TypeScript, JSON schema, precomputed export and Python SDK type are regenerated from it.

The experimental precomputed export embeds ConfiguredHookHandler and still omitted failClosed.

The config.toml schema embeds the command hook handler, which now carries failClosed.

thedancingdeveloper · 2026-10-12T21:16:04Z

Superseded: these commits were cherry-picked into #22 (fork features, merged to fork/main as c2d74d2) and ship in release fork-c2d74d2d9. Closing so the work isn't merged twice. The branch is kept.