Repository navigation
fix: preserve completed tool outputs when a sibling fails - #5332
jbeckwith-oai wants to merge 9 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (3)
ℹ️ 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. |
![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: af69e3739e
ℹ️ 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".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 39324dd. I found a passing-input timing case that still loses completed side effects; the inline comment describes the trigger and regression test.
I also confirmed two existing findings against this commit:
- Supplied non-streamed RunState is not synchronized. After an approved interruption plus accepted
state.add_input(...), the generated/session arrays are copied before the next turn. This handler appends completed pairs and the response only to local arrays, so serializing the supplied state after the tool error loses them. Even without added input, the session and raw-response copies lag. Synchronize the recovery state before rethrowing. - Streamed replay loses the tool-use snapshot. The new error branch bypasses the normal tracker snapshot. A first turn with
tool_choice="required"can retain completed outputs but restore an empty tracker, forcing another tool call on resume instead of resetting tool choice.
Please make partial-turn acceptance update the complete recovery checkpoint, including transcript, response and tool-use state, through the existing state owners. The two runners currently update different subsets of that state. The earlier non-streamed input-admission comment does not reproduce here: resumed turns set all_input_guardrails to an empty list.
Validation: source review of the four-file diff, lifecycle/state owners and regression tests. GitHub reports 23 successful checks and two skipped readiness checks at this head. I did not run repository workloads. The added tests cover the basic failure modes, but not the delayed passing input verdict or these resume-state cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e334fbc96
ℹ️ 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".
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8493f588e1
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8493f588e1
ℹ️ 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".
| def _publish_completed_tools() -> None: | ||
| # Accepted server responses already have their own resumable checkpoint. | ||
| if on_tool_execution_error is not None and not server_manages_conversation: | ||
| retained_items = _completed_tool_step_items(new_step_items, completed_outputs) |
There was a problem hiding this comment.
When conversation_id or response chaining is active on a follow-up turn, pending_input_admission_items is empty, so the accepted response does not receive the special interruption checkpoint referenced by this comment. This unconditional server-managed exclusion then prevents a successful local sibling's output from reaching error details or RunState; the server owns the tool call in its response chain but does not own the client-executed output, so resuming cannot submit that completed result and may repeat the local side effect. Publish local completed outputs on these turns or checkpoint every accepted server response before excluding this path.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this thread open as a separate scope item. This PR deliberately preserves existing server-managed recovery and limits its new partial-history publication to client-managed turns. The follow-up path described here needs a server-conversation checkpoint/replay contract: the server owns response calls, while local outputs still need submission. Reusing the client-managed publication path or checkpointing every server response without that analysis risks duplicate replay. The current change does not claim to fix that path; the PR description now states the boundary. This remains unresolved, not a false positive or an accepted-risk dismissal.
There was a problem hiding this comment.
The reviewed scope is now approved: this PR fixes client-managed history and preserves the existing server-managed recovery path. The follow-up-turn gap remains separate work and is stated in the PR description. A server already owns failed sibling calls, so reusing the pruned client transcript is insufficient; the follow-up needs explicit unresolved-invocation and unsent-output recovery semantics. Leaving this thread open because 57a2c71 does not fix that gap.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 959ebbb16a
ℹ️ 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".
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
The non-streaming runner still loses completed tool history when a default parallel input guardrail is pending; see the inline finding.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 5a2bc6993d
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| if pending: | ||
| task.cancel() | ||
| # Bound cancellation cleanup too: application hooks may suppress cancellation. | ||
| await asyncio.wait((task,), timeout=_FUNCTION_TOOL_POST_INVOKE_WAIT_SECONDS) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When a prompt-exposed host configures a native tool whose custom_data_extractor or on_tool_end catches CancelledError, a model response can pair that tool with a failing function-tool category. This branch calls task.cancel() but ignores whether the second timed asyncio.wait() still returns the task in pending; the run then raises while the finalizer remains live with its run context. Repeating such requests accumulates detached tasks and enables service-level exhaustion. Fresh evidence beyond the prior finalizer comment is that the new cancellation wait still has no terminal-state check. Keep ownership until termination or use an isolation boundary that can be killed.
SECURITY.md reference: SECURITY.md:L29-L35
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the specific cancellation-resistant case with a synthetic public Runner probe: the callback receives cancellation, suppresses it, and remains pending after the bounded cleanup returns. The probe explicitly releases it afterward to avoid leaving work behind. This is not fixed by the earlier cancel-then-wait change.
The existing tool lifecycle contract requires bounded draining and prompt parent cancellation; it does not guarantee forcible termination of application callbacks that suppress cancellation. An asyncio Task cannot forcibly terminate such a coroutine. Waiting until it exits can hang the run indefinitely, and guaranteeing termination through isolation or a new ownership/supervision policy is a broader design change. Checking pending again alone would not terminate it.
Leaving this thread open, without dismissal or accepted-risk status, and asking the human for the scope decision. The independent Python 3.10 cancellation-marker correction is continuing through review and verification; it does not claim to remediate this limitation.
There was a problem hiding this comment.
The reviewed scope now explicitly defers hard termination of cancellation-resistant application callbacks. Commit 57a2c71 seals client recovery snapshots against late callback mutation, but it does not stop a coroutine that suppresses cancellation or bound aggregate retained tasks across repeated requests. Cleanup remains bounded to preserve prompt cancellation. This limitation is explicit in the PR description; this thread remains open and is not being represented as remediated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c177ae685
ℹ️ 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".
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
A completed side effect can still be forgotten when custom-data finalization exceeds the bounded wait; see the inline finding.
| while not release.is_set(): | ||
| try: | ||
| await release.wait() | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57a2c7122e
ℹ️ 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".
| on_response_accepted=_commit_pending_server_response, | ||
| on_response_hooks_started=_mark_response_hooks_started, | ||
| run_state=run_state, | ||
| on_tool_execution_error=partial_tool_results.append, |
There was a problem hiding this comment.
When a parallel input guardrail trips after a tool failure has been selected but while that tool batch is still draining post-invocation work, the non-streaming invocation wires only the partial-output callback and never receives the on_tool_error_selected signal used by the streamed path. The guardrail task can therefore finish first, causing the dedicated tripwire branch to cancel model_task and raise InputGuardrailTripwireTriggered, whereas the same ordering in run_streamed() preserves the selected tool error; this hides the actual tool failure and makes error handling depend on runner mode. Wire the selection signal through this path and apply the same precedence before handling a late tripwire.
AGENTS.md reference: AGENTS.md:L116-L116
Useful? React with 👍 / 👎.
| except BaseException: | ||
| accepting_outputs = False | ||
| if run_state is not None and not server_manages_conversation: | ||
| run_state._generated_items = _snapshot_tool_outputs( | ||
| run_state._generated_items, committed_tool_outputs | ||
| ) |
There was a problem hiding this comment.
When an interrupted response resumes multiple approved tools and one completes before another raises an AgentsException, this failure path snapshots the completed output only into run_state._generated_items. The non-streaming runner still builds RunErrorDetails.new_items from its pre-resume session_items, and the streaming runner updates _model_input_items but not new_items, so both public error details omit the accepted output even though the supplied RunState contains it. Synchronize the public/session view from the checkpoint before re-raising so diagnostics accurately expose the side effect that recovery will skip.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 57a2c7122e
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| while not self._input_guardrail_queue.empty(): | ||
| guardrail_result = self._input_guardrail_queue.get_nowait() | ||
| if guardrail_result.output.tripwire_triggered: | ||
| if guardrail_result.output.tripwire_triggered and not self._tool_error_selected: |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Fresh evidence beyond the earlier stream-completion thread: when streaming with parallel input guardrails, a model-selected tool that propagates CancelledError sets _tool_error_selected; this condition then consumes a late tripwire without storing InputGuardrailTripwireTriggered. Because _check_errors() also ignores a cancelled run-loop task, stream_events() ends normally. A host relying on its documented exception to reject untrusted input can treat the request as a clean completion. Preserve the tripwire whenever the selected tool failure has no reportable stream exception.
SECURITY.md reference: SECURITY.md:L38-L38
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
This branch has not been deployed
3 participants
Summary
This pull request fixes completed tool outputs disappearing when another tool in the same turn fails. Client-managed runs retain accepted call/output pairs, required reasoning and replay ancestry in Session history, error details, streamed results, and existing RunState recovery. The run still reports the failure, and rejected input or tool output is not published.
Accepted output is recorded after tool output guardrails and provider conversion, before optional custom-data extraction and end hooks. Failed runs may therefore retain an output with incomplete optional metadata. Local recovery snapshots detach replay payloads and SDK metadata from late callbacks while preserving stream occurrence markers and provider-owned response identities. Successful runs retain their existing metadata behavior. Existing Session reconciliation handles failed writes; there is no new public API or persisted schema.
Scope and limitations:
conversation_id/ response chaining) keeps its existing recovery path. The known local-output gap on follow-up failures is separate work.Test plan
Issue number
Closes #5327
Checks
.agents/skills/code-change-verification/scripts/run.sh