Restore pinned Codex deny-floor adapter - #1538
Conversation
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
|
@codex review Please review exact head |
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent adversarial review — exact head 6a09f435ace8b44ec6b18ca89e9c66a1b8ac72b6 against base 2daeee9449cbdc454436df2a7acc702d3ecf5034.
Verdict:
- CRITICAL: 0
- HIGH: 0
- MEDIUM: 0
- LOW: 0
The bounded review/fix round closed all six HIGH findings found during review: whitespace-only stderr acceptance, inherited Python import-path influence, dispatcher replacement between identity check and execution, malformed event/CWD acceptance, unreviewed producer-checkout execution, and Windows installed-pin drift being silently skipped.
Exact-head evidence I ran or directly reconciled:
py -3 -B -m unittest discover -s scripts/agent_hooks -p "test_*.py" -v: 24/24 passed, zero skips; this includes the real Windows allow and attributed force-push-deny handler path under hostileHOME,GH_HOST,PYTHONPATH, and strippedPATH.py -3 -B scripts/agent_hooks/smoke_test.py: passed.- Docs governance, golden-principles, and GitHub-operations governance: passed.
- JSON/TOML parsing, PowerShell parsing, POSIX
bash -n, andgit diff --check: passed. - All six commits carry
Signed-off-bytrailers; reviewed worktree is clean. - Live producer main is
c056a77d22ecdcdfb2389c4c83487eb54061f5aa; installed normalized dispatcher SHA-256 is524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b, floor 1.6.19. Clean isolated producer smoke passed 2171/2171, exit 0.
No code, security, fail-closed, test, documentation, workflow, or merge-risk finding remains from this independent lens.
This review deliberately does not satisfy the remaining external gates: exact-head hosted CI/CI Extended/CodeQL/comment triage and the maintainer-run fresh exact-root Codex /hooks inspection plus allowed git status and Taskdeck-attributed non-writing deny canary. Do not merge until those gates are directly proven.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a09f435ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| executable, | ||
| "-I", | ||
| "-B", | ||
| str(dispatcher), |
There was a problem hiding this comment.
Execute the verified dispatcher bytes
During a dispatcher deployment/rollback or hostile A→B→A replacement, the pre-check can hash reviewed file A, while this subprocess reopens the pathname and executes unreviewed file B, after which the post-check sees restored file A. If B exits successfully with empty stdout, attribute_dispatcher_output() returns None and the Bash command is allowed; a deterministic swap-run-restore reproduction produced exactly that result while the adapter tests remained green. Execute an immutable snapshot or descriptor containing the bytes that were actually verified rather than re-resolving the path.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| "command": "expected=524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b; dispatcher=$HOME/.claude/hooks/dispatch.py; wrapper=.codex/invoke_deny_floor.sh; /bin/sh \"$wrapper\" --dispatcher-suffix .claude/hooks/dispatch.py --event pre --runtime codex", | ||
| "commandWindows": "$expected='524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b'; $dispatcher=$env:USERPROFILE+'/.claude/hooks/dispatch.py'; $wrapper='.codex/invoke_deny_floor.ps1'; & $wrapper --dispatcher-suffix .claude/hooks/dispatch.py --event pre --runtime codex", |
There was a problem hiding this comment.
Pin the repo-owned adapter in the trusted hook
After /hooks trusts this definition, a checkout, deployment, or permitted non-Bash file edit can replace either relative launcher or deny_floor_adapter.py without changing the trusted hook command, because the embedded expected value is unused and neither launcher verifies the repo-owned files. Replacing only the adapter with a successful empty-output program made this exact handler return exit code 0 with empty stdout for a force-push payload, which is the canonical allow result. Bind the trusted definition to reviewed launcher and adapter digests, or invoke an immutable/content-addressed adapter.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| python=$(command -v python3 2>/dev/null) || deny | ||
| [ -n "$python" ] || deny | ||
|
|
||
| exec "$python" -I -B "$core" "$@" |
There was a problem hiding this comment.
Convert POSIX interpreter failures into an explicit denial
If Python disappears or becomes non-executable after command -v, or the adapter fails during startup/import in a partial deployment, this exec replaces the shell and exits with only a nonzero status and stderr, never invoking deny() or emitting deny JSON. Codex reserves exit code 2 with a nonempty reason or valid deny JSON for a blocking PreToolUse result, so this ordinary hook-process failure can let the pending command continue; an invalid adapter fixture reproduced exit code 1 with zero stdout bytes. Keep the shell alive, capture the adapter status, and convert every nonzero result to an explicit denial as the Windows launcher already does.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "expected=524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b; dispatcher=$HOME/.claude/hooks/dispatch.py; wrapper=.codex/invoke_deny_floor.sh; /bin/sh \"$wrapper\" --dispatcher-suffix .claude/hooks/dispatch.py --event pre --runtime codex", |
There was a problem hiding this comment.
Strip loader injection variables before creating hook processes
With shell_environment_policy.inherit = "all", an inherited LD_PRELOAD, LD_AUDIT, or equivalent loader variable takes effect when this /bin/sh process is created, before the launcher can execute its unset statements. A preload constructor that exited the shell successfully made this exact force-push handler return code 0 with empty stdout and stderr—the canonical allow result—without modifying the adapter or dispatcher. Exclude dynamic-loader injection variables from the environment used to create hook processes and cover the exact handler with a hostile-environment regression.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| { | ||
| "type": "command", | ||
| "command": "expected=524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b; dispatcher=$HOME/.claude/hooks/dispatch.py; wrapper=.codex/invoke_deny_floor.sh; /bin/sh \"$wrapper\" --dispatcher-suffix .claude/hooks/dispatch.py --event pre --runtime codex", | ||
| "commandWindows": "$expected='524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b'; $dispatcher=$env:USERPROFILE+'/.claude/hooks/dispatch.py'; $wrapper='.codex/invoke_deny_floor.ps1'; & $wrapper --dispatcher-suffix .claude/hooks/dispatch.py --event pre --runtime codex", |
There was a problem hiding this comment.
Resolve the Windows wrapper from the hook source
When Codex is started below the repository root, it still discovers this root hook but launches it with the session subdirectory as its process cwd; this was reproduced with installed codex-cli 0.144.0-alpha.4 using -C <repo>/sub. The relative $wrapper therefore resolves to <repo>/sub/.codex/invoke_deny_floor.ps1, and PowerShell's & failure occurs before the wrapper's try/catch, producing no deny JSON and an ordinary exit code 1 rather than Codex's blocking exit-2 path. Resolve the wrapper from the hook source or project root and fail closed inline if it cannot be invoked.
AGENTS.md reference: AGENTS.md:L106-L107
Useful? React with 👍 / 👎.
Summary
Chris0Jeky/agent-harness@c056a77d22ecdcdfb2389c4c83487eb54061f5aa, floor1.6.19 (2026-07-27), normalized dispatcher SHA-256524fed5ae6630313f4a000a6e9a8c7deb7b1a7a6d424913d8b12c4536b79a97b.origin/main.Closes #1456
Supersedes #1457
Verification
py -3 -I -B -m unittest discover -s scripts/agent_hooks -p "test_*.py"— 24/24 passed, zero skips.agent-harness@c056a77d...checkout — 2171/2171 passed, exit 0.bash -n,git diff --check, and DCO across all six commits passed.6a09f435ace8b44ec6b18ca89e9c66a1b8ac72b6(pending GitHub execution).Preserved red evidence
The first reviewed-producer smoke run against floor 1.6.18 exited 1 after about 304 seconds at
smoke_test.py:5774: the long-lived Codex process had inherited the retiredGIT_CONFIG_GLOBALsetting. Running the identical producer bytes with only that child environment variable removed passed 2166/2166. The fresh-session Taskdeck configuration removes that variable; the documented gate fails if it remains inherited. After the producer advanced to current floor 1.6.19, an isolated clean run passed 2171/2171.The producer audit's general-hygiene command exits 1 for three unrelated pre-existing findings (the global
CLAUDE.mdandAGENTS.mdline budgets, plus the existing Taskdeck profile path). Its deny-floor proof fields are current: zero unproven roots and declared-versus-real identity matched.Review
One independent adversarial round found six HIGH defects: whitespace-only stderr acceptance, Python import-path influence, a dispatcher replacement race, malformed event/cwd acceptance, dirty producer execution risk, and a Windows installed-pin skip. All six were fixed. A final exact-head review at
6a09f435...reported 0 CRITICAL, 0 HIGH, 0 MEDIUM, and 0 LOW findings.Documentation
docs/STATUS.md,docs/IMPLEMENTATION_MASTERPLAN.md, anddocs/TESTING_GUIDE.mdwith the current producer identity and fail-fast runtime ritual..codex/README.mdfor the reviewed adapter contract.OUTSTANDING_TASKS.mdunchanged.Tracking / merge gates
Reviewwith derived priorityPriority I./hooks, run the allowed controlgit status --short, then run a non-writing force-push dry-run canary and confirm the denial is attributed toTaskdeck Codex deny-floor adapter.CI Workflow Validation
This PR changes
.github/workflows/reusable-docs-governance.yml, so CI Extended/actionlint is a mandatory hosted gate. It must be green at this exact head before merge.Risk Notes
Static checks, Claude hook evidence, worktree-local execution, and old runtime sessions do not satisfy the final Codex hook activation gate. The PR must remain unmerged until hosted checks, all comments/threads, project fields, and the fresh exact-CWD human runtime canary are complete.