Repository navigation
feat: add timeout result policy for Codex function tools - #5605
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at be4737f. Static review covered the public API and exports, captured settings and selected MCP adapters, deadline ownership, joined async/sync cleanup, parent cancellation, formatter admission and failures, late-result suppression, and root/child/parallel invocation coverage. No actionable findings. Hosted CI is still running with no failures observed; no local code or tests were run.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be4737f9f1
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Superseding my earlier approval: I missed the new child-task boundary issues and have now independently confirmed the existing findings. Request changes at be4737f.
- Release the completed body task after extracting its outcome, before validation/after-hook failures can retain it and expose rejected callback payloads through traceback locals: #5605 (comment)
- Preserve non-cancellation body cleanup failures in the owning state when parent cancellation wins. _close_after_error attaches the failure to CancelledError, but the outer handler discards that cancellation without retaining the cleanup error: #5605 (comment)
- Capture process-control exceptions inside the child task and transport them to the parent; SystemExit/KeyboardInterrupt must not escape the task and abort the event loop: #5605 (comment)
Please add regressions for these boundaries. Referencing existing discussions rather than duplicating inline findings. Static review only; no candidate code or tests run. Hosted CI remains pending.
be4737f to
85898e2
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 85898e2. All three earlier findings are resolved: the child task returns None and keeps output in the parent cell cleared on validation failure; cancellation and revoked admission retain non-cancellation cleanup failures in owner state; BaseException is transported out of the child without process-control exits escaping asyncio, then handled by the existing owning invocation boundary. Reviewed the exact old/new source changes separately from the rebased documentation base, along with privacy, cancellation/close, and process-control regression coverage. No additional actionable findings. Hosted CI is still running with no failures observed. Static review only; no local code or tests run.
Summary
This pull request adds a timeout result policy for Codex function tools. Set
timeout_behavior='error_as_result'withcallback_timeoutto return failed text when a callback exceeds its deadline.timeout_error_functioncan format that text using the tool context and a typedToolTimeoutError. The default still raises an exception and fails the turn; the new error remains aTimeoutError.The SDK waits for entered callback cleanup before it formats the timeout result. Failed timeout output bypasses success validation and the
afterhook. Parent cancellation prevents late output and retains callback cleanup failures. The body task delivers failures to its owner without retaining rejected return values or letting process-control exceptions escape into the event loop. A timeout does not undo side effects or forcibly stop work that ignores cancellation. Selected MCP function-tool adapters retain these settings. The change includes an example and updates the public API snapshot without changing release metadata.Test plan
Issue number
No linked GitHub issue.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md