Repository navigation
Codex hooks: document layered precedence and guard inject-args against copying user hook groups (#12081) #12141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
f3c1f27
docs: describe Codex hook layering and the wrapper opt-out (#12081)
austinywang c84cc4d
test: guard Codex inject-args against copying user hook groups (#12081)
austinywang 11d4c58
Merge remote-tracking branch 'origin/main' into issue-12081-codex-hoo…
austinywang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| #!/usr/bin/env python3 | ||
| """ | ||
| Regression coverage for Codex hook layering. | ||
|
|
||
| Codex appends cmux's session-flags layer to hooks.json, so copying user-owned | ||
| hook groups into the injected values would register and run them twice (#12081). | ||
| This test guards that contract and the one-cmux-producer-per-event rule. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import os | ||
| from pathlib import Path | ||
| import re | ||
| import subprocess | ||
| import tempfile | ||
|
|
||
| from claude_teams_test_utils import resolve_cmux_cli | ||
|
|
||
|
|
||
| CODEX_EVENTS = ( | ||
| "PreToolUse", | ||
| "PermissionRequest", | ||
| "PostToolUse", | ||
| "PreCompact", | ||
| "PostCompact", | ||
| "SessionStart", | ||
| "SessionEnd", | ||
| "UserPromptSubmit", | ||
| "SubagentStart", | ||
| "SubagentStop", | ||
| "Stop", | ||
| "Interrupt", | ||
| ) | ||
|
|
||
| VALUE_PATTERN = re.compile(r"^hooks\.([A-Za-z]+)=(.*)$") | ||
| SHAPE_PATTERN = re.compile( | ||
| r"^\[\{hooks=\[\{type=\"command\",command='''(?P<cmd>[^']+)''',timeout=\d+\}\]\}\]$" | ||
| ) | ||
|
|
||
|
|
||
| def run_inject_args(cli: str, codex_home: str) -> list[str]: | ||
| env = os.environ.copy() | ||
| env["CODEX_HOME"] = codex_home | ||
| for key in list(env): | ||
| if key.startswith("CMUX_"): | ||
| del env[key] | ||
| result = subprocess.run( | ||
| [cli, "hooks", "codex", "inject-args"], | ||
| env=env, | ||
| check=True, | ||
| stdout=subprocess.PIPE, | ||
| ) | ||
| return [part.decode("utf-8") for part in result.stdout.split(b"\0") if part] | ||
|
|
||
|
|
||
| def user_hook_group(event: str) -> dict[str, object]: | ||
| return { | ||
| "matcher": "", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "user-{}-hook".format(event), | ||
| "timeout": 7, | ||
| } | ||
| ], | ||
| } | ||
|
|
||
|
|
||
| def write_hooks(home: Path, hooks: dict[str, list[dict[str, object]]]) -> bytes: | ||
| home.mkdir(parents=True, exist_ok=True) | ||
| path = home / "hooks.json" | ||
| path.write_text(json.dumps({"hooks": hooks}, indent=2) + "\n", encoding="utf-8") | ||
| return path.read_bytes() | ||
|
|
||
|
|
||
| def emitted_hooks( | ||
| args: list[str], require_session_start: bool = True | ||
| ) -> tuple[dict[str, str], dict[str, str]]: | ||
| if args[:3] != ["--enable", "hooks", "--dangerously-bypass-hook-trust"]: | ||
| raise AssertionError("unexpected activation args: {!r}".format(args[:3])) | ||
| rest = args[3:] | ||
| if len(rest) % 2 != 0 or any(rest[index] != "-c" for index in range(0, len(rest), 2)): | ||
| raise AssertionError("expected only -c/value pairs: {!r}".format(rest)) | ||
|
|
||
| values: dict[str, str] = {} | ||
| commands: dict[str, str] = {} | ||
| for index in range(1, len(rest), 2): | ||
| value = rest[index] | ||
| match = VALUE_PATTERN.fullmatch(value) | ||
| if match is None: | ||
| raise AssertionError("invalid hook value: {!r}".format(value)) | ||
| event, encoded = match.groups() | ||
| if event in values: | ||
| raise AssertionError("duplicate emitted event: {}".format(event)) | ||
| if "user-" in value: | ||
| raise AssertionError("user-owned hook copied into value: {!r}".format(value)) | ||
| shape = SHAPE_PATTERN.fullmatch(encoded) | ||
| if shape is None: | ||
| raise AssertionError("unexpected cmux hook shape: {!r}".format(value)) | ||
| command = shape.group("cmd") | ||
| command_path = Path(command) | ||
| if not ( | ||
| (command_path.name.startswith("cmux-codex-hook-") and command_path.name.endswith(".sh")) | ||
| or "cmux hooks codex" in command | ||
| ): | ||
| raise AssertionError("non-cmux command emitted: {!r}".format(command)) | ||
| values[event] = encoded | ||
| commands[event] = command | ||
| required = {"UserPromptSubmit", "Stop"} | ||
| if require_session_start: | ||
| required.add("SessionStart") | ||
| if not required.issubset(values): | ||
| raise AssertionError("missing required events: {!r}".format(sorted(required - set(values)))) | ||
| return values, commands | ||
|
|
||
|
|
||
| def main() -> int: | ||
| cli = resolve_cmux_cli() | ||
| with tempfile.TemporaryDirectory(prefix="cmux-codex-inject-args-", dir="/tmp") as root: | ||
| root_path = Path(root) | ||
| first_home = root_path / "case-one" | ||
| first_hooks = {event: [user_hook_group(event)] for event in CODEX_EVENTS} | ||
| first_bytes = write_hooks(first_home, first_hooks) | ||
| first_args = run_inject_args(cli, str(first_home)) | ||
| first_values, first_commands = emitted_hooks(first_args) | ||
| if (first_home / "hooks.json").read_bytes() != first_bytes: | ||
| raise AssertionError("case 1 changed hooks.json") | ||
| session_start_command = first_commands["SessionStart"] | ||
|
|
||
| second_home = root_path / "case-two" | ||
| second_hooks = {event: [user_hook_group(event)] for event in CODEX_EVENTS} | ||
| second_hooks["SessionStart"].append( | ||
| {"hooks": [{"type": "command", "command": session_start_command}]} | ||
| ) | ||
| second_bytes = write_hooks(second_home, second_hooks) | ||
| second_args = run_inject_args(cli, str(second_home)) | ||
| second_values, _ = emitted_hooks(second_args, require_session_start=False) | ||
| if "SessionStart" in second_values: | ||
| raise AssertionError("persistent cmux SessionStart handler was not respected") | ||
| if not {"UserPromptSubmit", "Stop"}.issubset(second_values): | ||
| raise AssertionError("case 2 omitted required remaining events") | ||
| if (second_home / "hooks.json").read_bytes() != second_bytes: | ||
| raise AssertionError("case 2 changed hooks.json") | ||
|
|
||
| print("PASS: Codex inject-args preserves user groups and one cmux producer per event") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| try: | ||
| raise SystemExit(main()) | ||
| except Exception as exc: | ||
| print("FAIL: {}".format(exc)) | ||
| raise SystemExit(1) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the complete persistent-handler case.
When persistent cmux handlers cover every event,
emitCodexWrapperInjectArgs()returns before it adds--enable hooksand--dangerously-bypass-hook-trust. The current text says these flags apply to every cmux launch. State that these flags apply only when cmux injects at least one missing event. Make the trust-bypass statement conditional too.Also applies to: 157-157
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid:
emitCodexWrapperInjectArgs()returns before emitting--enable hooksor--dangerously-bypass-hook-trustwhen persistent cmux handlers already cover every event, so both the activation flags and the trust-bypass sentence should be conditional on at least one event needing injection. This PR is being consolidated into #12140 (now a draft here), and I raised the same correction there for its docs section; leaving this thread open rather than resolving it so the disposition stays visible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austinywang, confirmed. The documentation must make both flag behaviors conditional on cmux injecting at least one missing event. Keeping this thread open is appropriate while#12140consolidates the correction.✏️ Learnings added
You are interacting with an AI system.