fix: bound native Stop hook pointer loops#3238
Conversation
Signed-off-by: Foad Abo Dahood <32059146+masterfoad@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Red-team review — PR #3238 (fix: bound native Stop hook pointer loops)
Verdict: MERGE_READY
Reviewed the full diff and the surrounding native Stop / session-pointer authorization state machine adversarially against head 8a3b2b1763683cef076abb3ce47a0b2ecc5bc114. The change is scoped to the unauthorized Stop failure branch in dispatchCodexNativeHook (src/scripts/codex-native-hook.ts:20457-20477) and is a genuine loop-bounding fix, not an authorization weakening.
Findings
1. foreign-cwd no-op on first Stop is correct, not a bypass. The failure branch is reached only when allowImplicitSessionSideEffects === false. A foreign-cwd pointer (isSessionStateAuthoritativeForCwd → sameFilePath mismatch) has authority over another workspace and cannot authorize this cwd's Stop at all; blocking on it was a false block that looped forever (the pointer status never changes across replays). A legitimately active session in this cwd is authoritative-for-cwd → usable, so it routes to buildStopHookOutput (persistence enforced there), never the failure branch. No active-workflow persistence is reachable to bypass.
2. No attacker/stale-pointer persistence bypass. Ralph/team/autopilot/Ultragoal continuation Stop-blocks all live on the usable/absent/authorized path. Forging stop_hook_active only lets the forger's own unauthorized Stop terminate; it cannot reach or mutate an authorized session's persistence. The unmatched (session_scope_unmatched) case still leaves the authoritative pointer untouched.
3. stop_hook_active replay suppression is safely scoped. Each hook invocation reads its own payload; the flag is per-Codex-stop-attempt and resets on genuine progress. The failure branch emits only one kind of block, so the simpler raw check (vs. the signature-scoped maybeReturnRepeatableStopOutput on the usable path) cannot suppress an unrelated later Stop — a fresh Stop carries stop_hook_active=false and re-emits the diagnostic. No cross-session state is mutated.
4. One truthful diagnostic preserved. malformed / identity-indeterminate / session_scope_unmatched each block once on the first Stop (stopReason asserted in tests) then no-op on the Codex-marked replay. foreign-cwd intentionally emits no block (it is another workspace's pointer) — consistent with its exclusion from the diagnostic set.
5. Native-stop-state durability/non-mutation is correct. The failure branch never persists a signature; regression tests assert native-stop-state.json is not created and a pre-existing file is byte-identical before/after.
6. Safe interaction with adjacent work. #3237 (paused Stop guidance) and #3235 (live pointer replacement rejection) both operate on the usable path — #3235 is the complementary mitigation preventing a live pointer from being flipped to foreign-cwd. Exact pane/cwd authority (#3145) and Windows/path normalization hold: sameFilePath canonicalizes via realpathSync.native, and the symlink-alias authority test confirms the real cwd is never misclassified as foreign.
Verification
codex-native-hook.test.js: 573/573 pass (includes the new foreign-cwd no-op, malformed/identity-indeterminate replay-bound, and unmatched-replay cases).session.test.js+prompt-session-provenance.test.js: 69/69 pass (pointer classification, symlink authority, live-replacement rejection).- Clean
tscbuild; docs contract row updated accurately. - Hosted CI green for exact head
8a3b2b1: required checkCI Statuspass; Typecheck / Lint / Docs / Ralph Persistence Gate / Coverage Gate / all Node test lanes pass.
Reviewed serially; no subagents/teams spawned (host cgroup pressure honored). Contract change treated as adversarial and independently verified against source and tests rather than reporter claims.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Summary
Testing
npm run buildnode --test --test-concurrency=1 --test-name-pattern='owner-env terminal|bounds Stop replays|bounds Stop replays when session.json is malformed|no-ops Stop when session.json points to another worktree|session-scoped team id' dist/scripts/__tests__/codex-native-hook.test.jsnpm run lintnpm run check:no-unusedgit diff --check{}