Skip to content

Support Codex hooks feature rename and prevent nested Stop hook recursion#204

Closed
shallwe16623 wants to merge 1 commit into
PolyArch:mainfrom
shallwe16623:fix-codex-hooks-feature-rename
Closed

Support Codex hooks feature rename and prevent nested Stop hook recursion#204
shallwe16623 wants to merge 1 commit into
PolyArch:mainfrom
shallwe16623:fix-codex-hooks-feature-rename

Conversation

@shallwe16623
Copy link
Copy Markdown

Summary

This updates the Codex integration to support the current native hooks feature name, hooks, while retaining compatibility with older Codex builds that expose codex_hooks.

The main behavioral fix is that nested Codex reviewer/helper invocations now disable the active hooks feature. This prevents Humanize's Codex Stop hook from recursively triggering itself when it launches codex review or codex exec.

Problem

Current Codex builds expose lifecycle hooks as the stable hooks feature:

hooks                                   stable             true

Codex docs also describe hooks as the canonical feature key and codex_hooks as a deprecated alias.

Humanize still assumed codex_hooks in several Codex paths:

  • install-time feature detection
  • install-time feature enablement
  • nested Stop-hook codex review / codex exec calls
  • BitLesson helper codex exec calls

That creates two failure modes on current Codex builds:

  1. Installation can reject a Codex CLI that supports native hooks because the script only searches for codex_hooks.
  2. Nested Codex calls may use --disable codex_hooks even though the active feature is hooks. In that case, Stop hooks are not disabled for the nested Codex process, so the nested reviewer/helper can trigger the same Stop hook again when it exits.

Root Cause

The feature was renamed from the older codex_hooks key to the canonical hooks key. The install/runtime scripts did not consistently detect which feature name the installed Codex CLI exposes, and some paths hard-coded codex_hooks.

There was also a related shell robustness issue: several probes used command | grep -q under set -o pipefail. When grep -q exits early after finding a match, the producer can receive SIGPIPE, causing the whole pipeline to look like a failed probe. This can make supported Codex flags/features appear unsupported.

Changes

  • Detect hooks first and fall back to codex_hooks when installing Codex hooks.
  • Enable the detected hooks feature instead of always enabling codex_hooks.
  • Disable the detected hooks feature for nested Stop hook codex review and codex exec calls.
  • Disable the detected hooks feature for BitLesson helper Codex calls.
  • Cache the detected nested-disable feature name per loop instead of caching only a yes/no value.
  • Avoid grep -q in pipefail pipelines for Codex feature/help probes by capturing command output first.
  • Update Codex install docs and Humanize skill docs to mention current and legacy feature names.
  • Update tests to simulate current Codex hooks output and assert nested calls use --disable hooks.

Compatibility

This keeps older Codex compatibility by falling back to codex_hooks when hooks is not present in codex features list.

Validation

  • bash -n scripts/install-codex-hooks.sh hooks/loop-codex-stop-hook.sh scripts/bitlesson-select.sh tests/test-codex-hook-install.sh tests/test-disable-nested-codex-hooks.sh tests/test-bitlesson-select-routing.sh
  • bash tests/test-codex-hook-install.sh
  • bash tests/test-disable-nested-codex-hooks.sh
  • bash tests/test-bitlesson-select-routing.sh

@shallwe16623 shallwe16623 marked this pull request as ready for review June 6, 2026 17:09
@shallwe16623
Copy link
Copy Markdown
Author

Thanks! I noticed after opening this that related fixes already landed on dev in #134 and #194. This PR was based on main because main still hard-codes codex_hooks in the Codex install/runtime paths.

If the expected flow is to merge dev into main, feel free to close this as superseded. If a smaller main backport is useful, I can adjust this PR to match the existing dev implementation more closely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant