Skip to content

fix(codex): emit injected hook timeouts in seconds - #16254

Open
teamleaderleo wants to merge 8 commits into
mainfrom
codex-hook-timeout-seconds
Open

teamleaderleo wants to merge 8 commits into
mainfrom
codex-hook-timeout-seconds

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Codex hook configuration uses seconds for hooks.*.timeout, but cmux was passing its millisecond delivery values directly. This change emits ceiling-rounded seconds for current injected hooks and keeps an explicit compatibility schema for replaying saved launch arguments that still contain the old millisecond values.

Codex's source stores the field as timeout_sec and enforces it with Duration::from_secs: hook_config.rs and command_runner.rs.

The regression commit 6aaf2b48b5bb3702adf1349f70cfc62511059982 adds coverage for current second-based output and saved millisecond forms. The repair is 4617fd275fb (merged with current main as 959e2f0f3d6).

Validation: python3 scripts/verify-local.py passed all 16 local checks, including Swift syntax. The focused swift test --package-path Packages/macOS/CMUXAgentLaunch --filter CodexHookInjectionStrippingTests command was attempted on Linux but cannot compile this macOS package because the installed toolchain has no Darwin module; no native macOS build was run on this host.

Changelog

Fixed injected Codex hook timeouts so Codex receives seconds while replay still strips legacy millisecond hook prefixes.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes injected Codex hook timeouts so Codex receives seconds instead of milliseconds.

  • Hook configuration now renders ceiling-rounded seconds (treating sub-1000ms values as 1s) from cmux's millisecond policy, matching Codex's timeout_sec contract via a new codexTimeoutValue field on injection events and companions.
  • Replay sanitization keeps explicit millisecond-literal schemas so saved launch arguments from older builds still strip correctly.
  • Tests now require complete timeout values (e.g., timeout=5}) and use content-addressed script paths to exercise timeout units rather than script-name validation.

Written for commit 2003d63. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Codex hook timeouts are now expressed in seconds, rounded up from millisecond values, so configured time limits are interpreted consistently.
    • Existing saved hook configurations that use millisecond timeout values continue to be recognized and handled correctly.
    • Timeout values below one millisecond are treated as one millisecond before conversion, ensuring they render as at least one second.

teamleaderleo and others added 3 commits September 30, 2026 14:05
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex interprets hook timeout values as seconds, so convert cmux's millisecond policy values with ceiling rounding. Preserve exact millisecond schemas for replay sanitization of saved launches.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Merge-main commit by scripts/merge-main.sh.
Merged by scripts/merge-main.sh: origin/main at a4e862d.

Merge-main-previous-head: 4617fd2
Merge-main-base: a4e862d
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Repository guideline files applied to this review (3)
.github/review-bot-rules/test-determinism.md — configured
.github/review-bot-rules/swift-architectural-rethink.md — configured
.github/review-bot-rules/source-control-artifacts.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f2ba8ec4-6ad3-4459-8882-02ffea97c72f

📥 Commits

Reviewing files that changed from the base of the PR and between da1f268 and 2003d63.

📒 Files selected for processing (4)
  • Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionEvent.swift
  • Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift
  • cmuxCLITests/CLICodexHookTimeoutRegressionTests.swift
  • cmuxCLITests/CLICodexQueuedHookContractTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Codex hook events and companions now store rendered timeout values separately from millisecond source values. Configuration generation and sanitizer matching use rendered values. Recognized historical schemas and tests cover legacy millisecond values and current second-based values.

Changes

Codex hook timeout handling

Layer / File(s) Summary
Rendered timeout values
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionEvent.swift
Events and companions store a codexTimeoutValue, defaulted by rounding milliseconds up to seconds with a 1 ms minimum. An optional override is supported. Configuration generation uses the rendered values.
Schema compatibility and sanitization
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexHookInjectionSchema.swift, Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerCodexLaunch.swift, Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexHookInjectionStrippingTests.swift, cmuxCLITests/CLICodexHookTimeoutRegressionTests.swift, cmuxCLITests/CLICodexQueuedHookContractTests.swift
Recognized legacy schemas retain historical millisecond timeout values. Sanitizer matching uses codexTimeoutValue. Tests cover current second-based values and saved hooks with millisecond values.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: austinywang

Merge Risk: ⚪ Minimal · up to 2003d

Current hooks emit timeouts in seconds while saved millisecond-based configurations remain recognizable. No actionable merge-blocking risk was established; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2003d

The change corrects timeout units and preserves replay of older saved launches without adding execution privileges in the inspected paths. Remaining uncertainty concerns native execution, timeout handling, and compatibility when reverting to an older version.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The evidenced exposure is the local Codex launch and its saved argument replay. Timeout changes affect existing hook execution windows, while recognition changes determine which leading configurations are removed. Inspected command construction and replay behavior do not add an execution destination or grant additional authority.

Trust Boundaries and Controls

  • observed — User-controlled hook arguments are not accepted as executable identity proof: replay preserves the captured executable. Prefix removal requires hook enablement, the hook-trust bypass marker, a registered ordered event block, matching timeout literals, and matching commands.
  • observed — The existing inline-command matcher uses substring checks and can recognize crafted commands. That behavior predates this PR: the sanitizer comparison changes only timeout matching. Recognition removes arguments rather than executing them or substituting executable identity, so this existing looseness is not retained as an introduced security concern.

Resilience and Maintainability Implications

  • inferred — Immutable schemas and all-or-nothing prefix matching contain partial-recognition failures: no partially consumed argument list or shared mutable state is committed. Repeating recognition on the same input is deterministic, and an incomplete or foreign block remains untouched by this removal function.
🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: emitting injected Codex hook timeouts in seconds.
Description check ✅ Passed The description clearly explains the problem, resulting behavior, compatibility handling, testing performed, and changelog entry. It omits the template headings for Summary and Testing, the Demo Video…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS: The pull request changes only Codex hook timeout rendering, replay sanitization, and related tests. The authoritative diff contains no Cloud terminal creation, persistent transport, manual rende…
Cmux Swift Actor Isolation ✅ Passed PASS. The production diff changes timeout value handling only. CodexHookInjectionEvent, CodexHookCompanion, and CodexHookInjectionSchema were already immutable Sendable value types in the base…
Cmux Swift Blocking Runtime ✅ Passed The production Swift diff only adds Codex timeout-value conversion, rendering, and replay-schema matching. It adds no semaphores, blocking waits, sleeps, delayed dispatch, polling, main-queue sync, or…
Cmux Browser Automation Off-Main ✅ Passed PASS: The pull request changes only Codex hook timeout rendering, replay sanitization, and related tests. It does not change Sources/TerminalController.swift, ControlCommandExecutionPolicy.swift, …
Cmux Expensive Synchronous Load ✅ Passed The production Swift diff only changes Codex hook timeout rendering and legacy schema matching. It adds no agent-history load, store access, transcript or JSONL parsing, directory scan, per-record sys…
Cmux Cache Substitution Correctness ✅ Passed PASS. The production diff changes Codex hook timeout rendering and replay-schema matching only. It does not replace any fresh file, database, or index read with a cache or opportunistic value. The rep…
Cmux No Hacky Sleeps ✅ Passed PASS: The pull request changes only Swift source and Swift test files. The rule applies to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. The diff adds timeout-value conversion an…
Cmux Algorithmic Complexity ✅ Passed PASS. The production diff only changes fixed Codex hook schemas and timeout rendering. The sanitizer checks 7 statically defined schemas with at most 8 events each, and the new legacy conversion maps …
Cmux Swift Concurrency ✅ Passed PASS: The PR changes synchronous Codex timeout representation and replay matching only. The changed production Swift files add no Dispatch queues, Combine state, completion-handler APIs, or fire-and-f…
Cmux Swift @Concurrent ✅ Passed PASS. The PR changes only synchronous Swift code and test expectations. The changed source contains no async, nonisolated, @concurrent, actor-isolated, UI-isolated, or async I/O code. The added …
Cmux Swift Package Boundaries ✅ Passed The changed production Swift files are inside the existing Packages/macOS/CMUXAgentLaunch SwiftPM target, not the cmux app target. Package.swift defines the CMUXAgentLaunch library and its `CMUX…
Cmux Swiftpm Lockfiles ✅ Passed PASS. The PR changes only Codex Swift source and tests. It does not change a Package.swift dependency declaration, package-local Package.resolved, Xcode project package references, root Xcode Package.…
Cmux Swift Logging ✅ Passed PASS: The production Swift diff only changes Codex timeout rendering and schema matching. It adds no print, debugPrint, dump, NSLog, file/stdout diagnostics, Logger constants, or sensitive-dat…
Cmux User-Facing Error Privacy ✅ Passed PASS — The production diff only changes Codex hook timeout values and replay matching. It adds no user-facing error, alert, API error body, recovery copy, or diagnostic output. The related `inject-arg…
Cmux Full Internationalization ✅ Passed The PR changes Codex hook configuration values, timeout metadata, replay matching, and tests. It adds no user-facing Swift UI text, localization keys, catalogs, Info.plist entries, web messages, or lo…
Cmux Swiftui State Layout ✅ Passed PASS: The Swift diff does not introduce SwiftUI state or layout patterns covered by the rule. It changes plain CodexHookInjectionEvent and CodexHookCompanion value types, schema conversion, saniti…
Cmux Architecture Rethink ✅ Passed PASS. The Swift diff is a small, local timeout-format correctness fix. It adds immutable codexTimeoutValue values, pure conversion and compatibility mapping, and uses the shared `CodexHookInjectionS…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The PR changes Codex hook timeout modeling, schema compatibility, sanitization, and tests only. The authoritative diff adds no NSWindow, NSPanel, NSWindowController, SwiftUI Window, WindowGroup,…
Cmux Source Artifacts ✅ Passed PASS. The authoritative diff contains only six modified Swift source and test files under Packages/macOS/CMUXAgentLaunch/... and cmuxCLITests/.... The patch adds product timeout logic and regressi…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The PR changes only production Swift under Sources/ by adding Codex timeout rendering and replay-compatibility logic. It adds no #if DEBUG or test-build guard, no debug/test-named member, an…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Passes: CI passes on 2003d63732.

CI passes on 2003d63732 (run 37528015416 attempt 1).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way.

teamleaderleo and others added 2 commits September 30, 2026 15:14
Use the content-addressed paths used by cmux's current companion handlers so the timeout-unit regression exercises timeout matching rather than script-name validation.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Verification receipt for PR #16254

Head SHA: b570e9d

Green scoped verification: python3 scripts/verify-local.py passed 16/16 checks, and the focused CMUXAgentLaunch tests passed on hosted macOS. The hosted app compile lane is attributed to an inherited current-main error in CLI/CMUXCLI+AutoNaming.swift:271 (usesTemporaryConfig out of scope), recorded by CI. No auto-merge is enabled.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review at b570e9d, using a review subagent.

Correct:

  • Rounding up with a minimum of 1.
  • A compatibility schema keeps saved millisecond commands strippable on replay (CodexHookInjectionSchema.swift:68).
  • The -c injection was the only emitter left: the persistent install already writes seconds, and the Resources/bin wrapper writes no timeout.
  • Tests cover both forms.

Left:

  • Blocking: four existing tests still expect milliseconds. CLICodexHookTimeoutRegressionTests.swift:152 and :482 (timeout=5000), and CLICodexQueuedHookContractTests.swift:40 (timeout=5000) and :51 (timeout=120000). Update them to the new values.
  • The seconds unit is only asserted. Cite Codex's source or docs for it in the PR body.
  • The macOS compile failure is main's break (CMUXCLI+AutoNaming.swift:271), which fix: restore main compile (OpenCodePaths in CLI, Codex auto-naming scope) #16260 fixes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Fixed in 68308c29c3e: updated the four CLI contract expectations to Codex seconds (5000 → 5, 120000 → 120) in CLICodexHookTimeoutRegressionTests.swift and CLICodexQueuedHookContractTests.swift. The PR body cites Codex’s hook_config.rs and command_runner.rs, which define and enforce timeout in seconds.

Left: the unrelated usesTemporaryConfig compile failure until #16260 lands, as requested.

@teamleaderleo teamleaderleo added bug Something isn't working S2: major A crash, hang, lost state, broken connection, or a regression on a path people use area: agents Agent integrations (Claude Code, Codex, ACP), agent chat, hooks, status difficulty:2 Focused: one package or feature boundary labels Sep 30, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Audit confirmation against current origin/main 579a979: CodexHookInjectionEvent.configValue still renders timeout=(timeoutMs) directly, while the Codex hook contract consumes seconds. The PR’s source scope is therefore valid and distinct from the auto-naming issue. The replay sanitizer’s recognized legacy schemas should remain covered because saved launch argv can contain the old millisecond values.

I added contributor metadata to the PR (bug, S2: major, area: agents, difficulty:2). Current hosted checks are not merge-ready: macOS compile admission, tests, and ci-status are red on this head; no local native build was run.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review of 68308c2: the four tests now say contains("timeout=5") and contains("timeout=120"). The old millisecond output timeout=5000 contains timeout=5, so these still pass if the fix is reverted. Match the whole value, for example timeout=5} or a regex with a word boundary. Also merge main now that #16260 fixed the compile. Left after that: the citation for the seconds unit, if it's not in the body yet.

teamleaderleo and others added 2 commits September 30, 2026 16:48
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Merge-main commit by scripts/merge-main.sh.
Merged by scripts/merge-main.sh: origin/main at b520727.

Merge-main-previous-head: 049e51c
Merge-main-base: b520727
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Fixed in 049e51cf9ec and pushed with main merge at 2003d63732a: the four Codex CLI assertions now include the closing } (timeout=5} / timeout=120}), so timeout=5 cannot match the old timeout=5000. The PR body already contains Codex source citations for the seconds unit.

Left: no remaining review item. The unrelated inherited local swift-syntax failure is from current main and was not chased.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Required checks are still red, so the merge decision is pending the CI fix; leaving this PR open.

1 similar comment
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Required checks are still red, so the merge decision is pending the CI fix; leaving this PR open.

This branch has not been deployed

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

Labels

area: agents Agent integrations (Claude Code, Codex, ACP), agent chat, hooks, status bug Something isn't working difficulty:2 Focused: one package or feature boundary S2: major A crash, hang, lost state, broken connection, or a regression on a path people use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants