Skip to content

fix(run): report tool guardrail results when a run fails - #4127

Closed
LHMQ878 wants to merge 10 commits into
openai:mainfrom
LHMQ878:fix/tool-guardrail-results-run-error-details
Closed

fix(run): report tool guardrail results when a run fails#4127
LHMQ878 wants to merge 10 commits into
openai:mainfrom
LHMQ878:fix/tool-guardrail-results-run-error-details

Conversation

@LHMQ878

@LHMQ878 LHMQ878 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Problem

RunResult exposes tool_input_guardrail_results and tool_output_guardrail_results, but RunErrorDetails declares neither:

https://github.com/openai/openai-agents-python/blob/9d894a90/src/agents/exceptions.py#L31-L43

So every tool guardrail result collected during a run becomes unreachable the moment the run raises — including from MaxTurnsExceeded, which is exactly when you want to know why the agent kept looping. Both construction sites (run.py:1596, run_loop.py:1309) already have the accumulated lists in local scope; they just aren't passed.

This is the same gap #4071 / #4090 closed for input/output guardrails, and #4097 closed for tool guardrails on successful streamed runs.

Repro

An agent whose tool is gated by a reject_content input guardrail. Since reject_content lets the run continue, the model keeps retrying and the run ends on max turns.

@function_tool
def blocked(query: str) -> str:
    return "should-not-run"

async def _gate(data: ToolInputGuardrailData) -> ToolGuardrailFunctionOutput:
    return ToolGuardrailFunctionOutput.reject_content(
        message="policy: blocked", output_info={"reason": "pii"}
    )

blocked.tool_input_guardrails = [ToolInputGuardrail(guardrail_function=_gate, name="pii_gate")]

On main (9d894a90):

run / completes              SUCCESS  tool_in_gr=3  names=['pii_gate', 'pii_gate', 'pii_gate']
run / MaxTurnsExceeded       run_data=set   tool_input_gr=<MISSING>  tool_output_gr=<MISSING>
streamed / MaxTurnsExceeded  run_data=set   tool_input_gr=<MISSING>  tool_output_gr=<MISSING>

The identical guardrail activity is fully visible when the run completes and entirely lost when it fails. str(run_data) doesn't mention tool guardrails either.

With this PR, both failing cases report all three pii_gate results, and run / run_streamed agree.

Changes

RunErrorDetails gains the two fields; the rest is making sure results actually reach it on each failure path. Review on this PR surfaced five further paths that dropped them, all reproduced before fixing:

  • RunErrorDetails: add both fields, defaulting to empty lists via field(default_factory=list) so existing constructor calls (including the three in tests/) keep working.
  • Populate at all three sites: run.py (non-streaming), run_internal/run_loop.py (streamed), and RunResultStreaming._create_error_details.
  • A tripwire aborts its own turn before a SingleStepResult exists, so the run-wide accumulators never see the triggering result. The function-tool executor now records its in-flight results on the propagating exception and _create_error_details merges them in.
  • A sibling tool failure cancels the function-tool side mid-flight. _execute_tool_plan gathers function tools alongside custom/shell/computer tools, so a non-function tool raising never passes through the executor's own recording path. The parallel branch now owns the accumulator lists and passes them in, so results survive the cancellation.
  • A parallel input guardrail trip discards the overlapped model turn even though its tool guardrails may already have run. Fixed on all three sub-paths: the cancel path, the no-cancel path (Temporal replay compatibility, which also discards the turn), and the streamed path, where run_data was frozen while the turn was still running and is now refreshed once the run loop settles.
  • pretty_print_run_error_details: report the two counts, matching its siblings.
  • docs/guardrails.md: one sentence next to the existing run_data paragraph. Translations left to make translate_docs.

Two implementation notes, both learned the hard way:

  • Partial results cannot be harvested from a cancelled task. awaiting one raises a fresh CancelledError, so anything the worker attached to its own exception is unreachable. Both fixes above therefore invert ownership — the caller creates the lists and passes them down.
  • _record_tool_guardrail_partials de-dupes by identity, not equality, since two runs of the same guardrail produce equal-looking results. That makes the overlapping recording paths safe to combine.

Tests

43 tests in tests/test_tool_guardrails.py, the newer ones parametrized over run / run_streamed where the paths differ:

  • input / output guardrail results reported on failure
  • a raising tool guardrail still reports results from earlier turns, and the result that triggered the tripwire
  • a sibling tool failure still reports the function tool's guardrail results
  • a parallel input tripwire reports the overlapped turn's results — separately for a completed turn, a cancelled turn, and the no-cancel path
  • a parallel guardrail that raises (rather than tripping) reports the overlapped turn's results too — the run-only gap fixed in e0ec7f5b
  • a streamed tripwire's run_data agrees with the streamed object once the turn settles
  • negative control: fields stay empty when no tool guardrails run

Two properties the assertions deliberately enforce, because without them these tests pass vacuously:

  • each one asserts the tool guardrail actually ran before comparing lists, so it cannot pass by both sides being trivially empty
  • the streamed test uses a slow consumer. A fast async for ... : pass parks on the empty event queue while the tool call is in flight, so the details happen to be built after the results land and the bug hides.

Every new test was confirmed to fail on the parent commit before its fix.

Verification

  • tests/test_tool_guardrails.py: 43 passed
  • ruff check / ruff format --check: clean
  • pyright on all changed files: 0 errors
  • mypy: no errors in any file this PR touches (pre-existing errors on main are all in unrelated sandbox / optional-dep modules)
  • Full suite A/B'd against the merge base on Windows, compared as JUnit XML test-ID sets rather than counts (the raw count is flaky here — 68/69/70 observed on one tree): 69 failures before, 69 after, identical sets — nothing added, nothing masked. The failures are pre-existing tracing-processor and guardrail-cancellation timing cases, reproducible on the merge base with nothing applied (tests/test_run_state.py can't be collected on Windows).
  • Merged current main to resolve a conflict with fix(run): keep input item order when collapsing duplicates #4140, which added an agent_span argument to run_single_turn.

Review follow-up (377a0067, 27086ef6, e0ec7f5b, e0546cd4, 56ca637c)

Five further gaps found in review, each reproduced with a probe before being fixed and each with an A/B showing which case is load-bearing:

  1. 377a0067 — the no-cancel path (Temporal replay compatibility) skipped a turn that was still pending when the tripwire arrived. Its tool guardrail results live in a local inside the tool executor until the turn settles, so awaiting it is what makes them reachable — and, since this configuration has opted out of cancelling, it is also what keeps the turn from outliving the run. The existing test used tool_sleep=0.0, so model_task.done() was always true and the pending branch was never exercised; it is now parametrized over turn_already_done / turn_still_pending.
  2. 27086ef6 — a sibling tool failure left the function-tool coroutine running. asyncio.gather propagates the sibling exception immediately, so the function-tool side kept executing past the end of the failed run. It is now held as a task and cancelled-then-drained on failure. The other half of that report — that results appended "milliseconds later" were lost — did not survive the probe: a cancelled tool body correctly never runs its output guardrail, so the empty list was accurate rather than truncated.
  3. e0ec7f5b — a parallel guardrail that fails by raising discards the turn just as a tripwire does, but only the tripwire branch harvested it. run reported []. Regression is parametrized over run / run_streamed so the parity stays pinned.
  4. e0546cd4 — the streamed sibling of (3), which my own test for (3) hid: it let the tool turn finish before the guardrail failed, so run_streamed looked correct. With the raise landing mid-tool-call it reports [] too, for a second reason. run_input_guardrails_with_queue cancels the run loop on a raise (a tripwire lets the turn complete), and Task.exception() on a cancelled task hands back a fresh CancelledError, so the partials recorded on the aborting error are unreachable by the time the details are built — marking them stale alone would refresh from empty accumulators. Instrumenting the turn shows the data is alive inside the task (CancelledError: in=['in_ALLOW']) and gone outside it. Fix is both halves: _capture_streamed_turn_tool_guardrail_partials copies the partials out from inside the task onto the streamed result, which outlives the cancellation — the same shape as _capture_turn_tool_guardrail_partials on the non-streaming path, with the streamed result as the owner instead of a caller-owned list — and the raised branch now marks the stored details stale so they rebuild once the loop settles. Merging dedups by id(), matching _record_tool_guardrail_partials, since two runs of one guardrail compare equal. The new regression is event-gated rather than sleep-raced, parametrized over both paths, and fails exactly the streamed row at e0ec7f5b (assert [] == ['input_allows']); it also pins the output guardrail list as empty, since that guardrail genuinely never ran. Full suite re-A/B'd: 69 failed / 5502 passed / 76 skipped, JUnit failure-ID set identical to the parent.
  5. 56ca637c — a review round on (4) caught that the three bookkeeping fields this PR adds (_stored_exception_details_stale and the two partial lists) were declared as init fields, shifting _cancel_mode, _last_processed_response and interruptions three positional slots to the right. All 41 constructor parameters are POSITIONAL_OR_KEYWORD and tests/test_source_compat_constructors.py pins the v0.7.0 positional prefix, so this is a released surface. None of the three is ever passed by a caller, so they are now init=False, matching _active_stream_consumers / _stream_consumers_stopped / _current_agent_ref on the same class. A new compat test pins the fields after the internal state; reverting just the init=False markers fails it with assert 'none' == 'immediate', the value meant for _cancel_mode binding to _stored_exception_details_stale. Suite: 69 / 5503 / 76, failure-ID set identical to base.

One further suggestion from that round was not taken: rebuilding the error details for a raised guardrail whose exception already carries run_data (a nested Runner.run failure). The run_data is None guard predates this PR (#4004, applied uniformly to all three branches in _check_errors) and preserves the inner run's details on purpose; A/B confirms behaviour is unchanged from the merge base. Overwriting them would replace the failed run's last_agent / raw_responses / input with the observing run's, which loses more than it recovers. Reasoning is in the review thread — if the nested case should merge rather than preserve, that is a cross-cutting RunErrorDetails ownership change and belongs in its own PR.

`RunResult` exposes `tool_input_guardrail_results` and
`tool_output_guardrail_results`, but `RunErrorDetails` declared neither, so
every tool guardrail result collected during a run became unreachable the
moment the run raised — even though both construction sites already had the
accumulated lists in scope.

This is the same gap openai#4071 and openai#4090 closed for input/output guardrails, and
openai#4097 closed for tool guardrails on successful streamed runs.

Add both fields to `RunErrorDetails` (defaulting to empty lists, so existing
constructor calls keep working) and populate them from the non-streaming path
(`run.py`), the streamed path (`run_internal/run_loop.py`), and
`RunResultStreaming._create_error_details`. Also report the two counts in
`pretty_print_run_error_details`, matching its siblings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a636c69b9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/run.py Outdated
Comment on lines +1604 to +1605
tool_input_guardrail_results=tool_input_guardrail_results,
tool_output_guardrail_results=tool_output_guardrail_results,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the triggering tool guardrail result

When a ToolInputGuardrailTripwireTriggered or ToolOutputGuardrailTripwireTriggered is raised during the current tool batch, these run-wide lists have not been extended yet because that only happens after execute_tools_and_side_effects() returns a SingleStepResult. As a result, a first-turn tool guardrail tripwire still produces exc.run_data.tool_*_guardrail_results == [] (and later tripwires omit the triggering result), even though _execute_tool_*_guardrails already appended that result locally; the streaming path has the same gap because it also publishes only completed-turn accumulators.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and this was the headline case of the PR — thanks. Fixed in aceb1e1.

Reproduced before the fix on both paths (a single raising guardrail, first turn):

### input guardrail raises, turn 1
   run       trip=in_RAISES   in=[]  out=[]
   streamed  trip=in_RAISES   in=[]  out=[]
### output guardrail raises, turn 1
   run       trip=out_RAISES  in=[]  out=[]
   streamed  trip=out_RAISES  in=[]  out=[]

Your diagnosis is exactly right: _execute_tool_{input,output}_guardrails appends to self.tool_{input,output}_guardrail_results on the _FunctionToolBatchExecutor, and execute() only returns those lists on the success path — so a tripwire aborts the turn before execute_tools_and_side_effects() builds a SingleStepResult, and the run-wide accumulators never see them.

The results do still exist at raise time, on the executor instance. So rather than re-plumbing return values through execute_function_tool_callstool_planningturn_resolution (which would mean changing several tuple signatures on a path that has both a gather and a sequential branch), I record them onto the propagating exception in execute() and merge them in at the three RunErrorDetails construction sites.

After:

### input guardrail raises, turn 1
   run       trip=in_RAISES   in=['in_RAISES']    out=[]
   streamed  trip=in_RAISES   in=['in_RAISES']    out=[]
### output guardrail raises, turn 1
   run       trip=out_RAISES  in=[]  out=['out_RAISES']
   streamed  trip=out_RAISES  in=[]  out=['out_RAISES']

Two details worth flagging:

  • Dedup is by identity, not equality. ToolInputGuardrailResult is a dataclass, so two distinct runs of the same guardrail compare equal; not in recorded would have silently dropped legitimate repeats. Uses id() instead.
  • The catch is except BaseException, so the partials are also preserved when the turn aborts for an unrelated reason mid-batch, and the existing asyncio.CancelledError branch keeps its propagating_failure semantics.

New tests (parametrized over run / run_streamed) cover the triggering result on both pipelines, guardrails that passed earlier in the same batch (['input_allows', 'input_raises']), and a two-turn case asserting the completed-turn accumulator and the aborted turn's partials each appear exactly once (['output_rejects', 'output_raises']) — that last one is the regression test for double-counting.

Verification: tests/test_tool_guardrails.py 32 passed; ruff clean; pyright 0 errors across all changed files; full suite A/B'd against the merge base on Windows shows no new failures (57 vs 59 baseline — the two differences are the known flaky guardrail-cancellation and realtime-cancel timing tests, which fail intermittently on main).

Addresses the Codex review on openai#4127. The run-wide accumulators are only
extended once `execute_tools_and_side_effects()` returns a `SingleStepResult`,
so a tool guardrail that raises aborts the turn first and its own result — plus
any guardrail results already collected in that same batch — never reach
`RunErrorDetails`. A first-turn tripwire still reported `[]`, which is the
headline case of this PR.

The results do exist at raise time, on the `_FunctionToolBatchExecutor`
instance. Record them onto the propagating exception there, then merge them in
at the three `RunErrorDetails` construction sites. Dedup is by identity, not
equality: two distinct runs of the same guardrail produce equal results and
both should be reported.

Adds 8 tests, parametrized over run/run_streamed: the triggering result is
reported for both the input and output pipelines, guardrails that passed
earlier in the same batch are included, and a two-turn case asserts the
completed-turn accumulator and the aborted turn's partials appear exactly once
each.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aceb1e1bb6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/run.py
Comment on lines +1606 to +1608
tool_input_guardrail_results=(
tool_input_guardrail_results + _tool_input_guardrail_partials(exc)
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve tool guardrails from parallel model tasks

When a parallel input guardrail trips after the model task has already completed a guarded function-tool turn, or is cancelled after running some tool guardrails, the asyncio.gather() tripwire path discards that model task's SingleStepResult/cancellation partials and re-raises the input-tripwire exception. This new RunErrorDetails population only merges the run-wide accumulators plus partials attached to that tripwire, so exc.run_data.tool_input_guardrail_results stays empty even though the overlapped model side already collected tool guardrail results; harvest the completed/cancelled model task before raising the tripwire.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 5140b87. Reproduced with a run_in_parallel input guardrail that sleeps 0.2s before tripping, so the model turn completes a guarded tool call first:

Before (aceb1e1b):  run_data set, tool_input_guardrail_results = []
After  (5140b872):  run_data set, tool_input_guardrail_results = ['input_rejects']
   (tool guardrail confirmed to have actually run: 1 time, in both cases)

The test asserts the guardrail ran at least once before comparing the lists, so it cannot pass by both sides being trivially empty.

The tripwire handler now captures the model task's outcome and carries its results onto the exception:

                                except InputGuardrailTripwireTriggered as tripwire_exc:
                                    if should_cancel_parallel_model_task_on_input_guardrail_trip():
                                        ...
                                        model_outcome: Sequence[Any] = await asyncio.gather(
                                            model_task, return_exceptions=True
                                        )
                                    else:
                                        model_outcome = ()
                                    _record_parallel_model_task_guardrail_partials(
                                        tripwire_exc, model_outcome
                                    )

_record_parallel_model_task_guardrail_partials handles both shapes you described: a task that completed a guarded turn is read via its SingleStepResult's tool_*_guardrail_results, and a task cancelled mid-turn is read via the partials the function-tool executor recorded on its exception.

On the streaming claim — I checked it and the streamed path does not have this gap. Running the same repro through Runner.run_streamed reports ['input_rejects'] even against aceb1e1b, because streamed_result.tool_input_guardrail_results is published incrementally rather than only on turn completion. The new test is parametrized over both modes anyway; the streamed case passes before and after, which documents the asymmetry rather than hiding it.

ruff, ruff format, mypy and pyright clean on all four changed files. The full suite A/Bs at 56 failures on the merge base vs 55 with the change; the differing entries (test_ctxmanager_spans and the two test_interrupt_* realtime tests) all fail in isolation with no changes applied — pre-existing order-dependent trace-ID failures.

Comment on lines +1523 to +1527
except BaseException as exc:
# The turn aborts before a SingleStepResult exists, so the run-wide accumulators would
# never see the results collected here - including a triggering tripwire result.
self._record_guardrail_partials(exc)
raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Carry function guardrails across tool-plan failures

This only records partials when the function-tool executor itself raises. In a mixed tool response, _execute_tool_plan() runs function tools in asyncio.gather() alongside custom/shell/computer tools; if a guarded function tool completes normally and a concurrent non-function tool then raises, the exception never passes through this block, so RunErrorDetails is built with empty tool guardrail lists even though those guardrails already ran. Publish completed function-tool guardrail results before propagating sibling failures, or harvest them from the gathered task.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 5140b87. Reproduced with a guarded function tool and a LocalShellTool whose executor raises, both in one model response:

Before (aceb1e1b):  run_data set, tool_input_guardrail_results = []      (guardrail ran 1x)
After  (5140b872):  run_data set, tool_input_guardrail_results = ['input_rejects']

Worth noting for anyone reading this later: the sibling failure has to be an AgentsException subclass to see it at all. A plain RuntimeError from the shell executor means no RunErrorDetails is ever constructed, so the lists are not merely empty — run_data is None. That is pre-existing behaviour and out of scope here.

Harvesting from the gathered task did not work, and the reason is worth recording. By the time the sibling's exception propagates out of asyncio.gather, the function-tool task is not done — gather cancels it, and awaiting a cancelled task raises a fresh CancelledError, so the partials the executor attached to its own exception object are unreachable:

HARVEST: done=False cancelled=False
DRAIN got CancelledError in=0 out=0

So instead of reading the results out afterwards, the caller now owns the lists:

        function_tool_input_results: list[ToolInputGuardrailResult] = []
        function_tool_output_results: list[ToolOutputGuardrailResult] = []
        ...
                execute_function_tool_calls(
                    ...
                    tool_input_guardrail_results=function_tool_input_results,
                    tool_output_guardrail_results=function_tool_output_results,
                ),

execute_function_tool_calls and _FunctionToolBatchExecutor take these as optional parameters and accumulate into them when provided, defaulting to fresh lists otherwise, so every other caller is unaffected. Results collected before the cancellation stay readable even though the return tuple never arrives, and the except clause copies them onto the propagating error. _record_tool_guardrail_partials de-dupes by identity, so this is safe in the case where the function side recorded the same results itself.

test_sibling_tool_failure_reports_function_tool_guardrail_results fails against aceb1e1b with Right contains one more item: 'input_rejects'.

…arallel guardrail failures

Two paths discarded tool guardrail results that had already been collected:

- _execute_tool_plan() gathers function tools alongside custom/shell/computer
  tools. When a non-function tool raised, the function-tool side was cancelled
  mid-flight and never returned its tuple, so guardrails that had already run
  were dropped. The accumulator lists are now owned by the caller, so results
  survive the cancellation and are recorded on the propagating exception.

- When a parallel input guardrail trips, the overlapped model task's
  SingleStepResult is discarded even though its tool guardrails may already have
  run. Those results are now harvested from the completed result, or from the
  partials the function-tool executor recorded if the task was cancelled.

The streamed path already reported these, since it publishes into the streamed
result incrementally rather than only on turn completion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5140b87241

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/run.py Outdated
Comment on lines +1326 to +1327
else:
model_outcome = ()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Harvest completed model results when not cancelling

When should_cancel_parallel_model_task_on_input_guardrail_trip() is false, such as the Temporal replay-compatibility path already covered by the existing guardrail tests, this branch always drops model_outcome even if model_task has already finished a guarded tool turn before the parallel input guardrail trips. The outer RunErrorDetails construction can only merge tool guardrail partials from model_outcome, so those completed tool_*_guardrail_results are still omitted from the raised tripwire; fresh evidence in this revision is the new else: model_outcome = () path, so harvest model_task.result() when the task is already done before falling back to empty.

AGENTS.md reference: AGENTS.md:L120-L120

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e2bcc7. Reproduced by patching should_cancel_parallel_model_task_on_input_guardrail_trip to return False, with a guarded tool call that completes before the parallel guardrail trips:

Before (5140b872):  run_data set, runs=1, tool_input_guardrail_results = []
After  (3e2bcc71):  run_data set, runs=1, tool_input_guardrail_results = ['input_rejects']

The branch now harvests an already-finished task instead of dropping it:

model_outcome: Sequence[Any] = ()
if should_cancel_parallel_model_task_on_input_guardrail_trip():
    if not model_task.done():
        model_task.cancel()
    model_outcome = await asyncio.gather(model_task, return_exceptions=True)
elif model_task.done():
    # Not cancelling (e.g. Temporal replay compatibility) still discards the turn, so
    # harvest a turn that already finished instead of dropping its guardrail results.
    model_outcome = await asyncio.gather(model_task, return_exceptions=True)

Guarded on done() deliberately: in the no-cancel path an unfinished task must be left alone, since awaiting it would defeat the point of not cancelling. Covered by test_parallel_tripwire_reports_guardrails_without_cancelling_the_turn, which asserts the tool guardrail actually ran before comparing the lists so it cannot pass by both sides being trivially empty.

Comment thread src/agents/run.py Outdated
Comment on lines +1323 to +1325
model_outcome: Sequence[Any] = await asyncio.gather(
model_task, return_exceptions=True
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve partials across task cancellation

When the parallel input guardrail cancels model_task after function-tool guardrails have already appended results, the executor records those partials on the CancelledError it catches, but asyncio.gather(..., return_exceptions=True) returns a fresh CancelledError for a cancelled task rather than the annotated exception object. As a result, _record_parallel_model_task_guardrail_partials() sees no partial attributes and the tripwire's run_data.tool_*_guardrail_results is still empty for this cancellation path; fresh evidence in this revision is that the new harvest path reads partials from the gathered cancellation object.

AGENTS.md reference: AGENTS.md:L120-L120

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e2bcc7. This one is correct, and it is the same trap that bit me earlier on this PR from the other direction. Reproduced with an allowing tool guardrail and a tool body that sleeps past the tripwire, so the turn is genuinely cancelled mid-flight rather than completing:

Before (5140b872):  run_data set, runs=1, tool_input_guardrail_results = []
After  (3e2bcc71):  run_data set, runs=1, tool_input_guardrail_results = ['input_allows']

Worth recording precisely where the annotation is lost, because it is narrower than "propagation drops it". Tracing run_single_turn from inside run.py's own frame shows the exception there does still carry the partials:

SEEN AT run.py FRAME: [('CancelledError', ['input_allows'])]

So it is specifically the task boundary — gather synthesising a fresh CancelledError for a cancelled task — that discards them, not the unwinding. That makes the fix local: read them inside the task, into lists the awaiting side owns.

async def _capture_turn_tool_guardrail_partials(turn, tool_input_guardrail_results, tool_output_guardrail_results):
    try:
        return await turn
    except BaseException as exc:
        tool_input_guardrail_results.extend(_tool_input_guardrail_partials(exc))
        tool_output_guardrail_results.extend(_tool_output_guardrail_partials(exc))
        raise

_record_parallel_model_task_guardrail_partials now records those caller-owned lists in addition to whatever it reads from model_outcome; _record_tool_guardrail_partials de-dupes by identity, so the overlap with a completed result is harmless. Covered by test_parallel_tripwire_reports_guardrails_from_a_cancelled_turn.

Comment thread src/agents/result.py
Comment on lines 905 to +906
tripwire_exc = InputGuardrailTripwireTriggered(guardrail_result)
tripwire_exc.run_data = self._create_error_details()
tripwire_exc.run_data = self._create_error_details(tripwire_exc)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rebuild streamed tripwire run_data after the turn settles

When a streamed run's parallel input guardrail tripwire is dequeued while the current function-tool turn is still running, this eagerly freezes tripwire_exc.run_data before stream_events() enters its finally block and awaits run_loop_task. If that turn then finishes and _accumulate_tool_guardrail_results() appends the tool guardrail results, _check_errors() keeps the already-stored exception and never refreshes its run_data, so the raised streamed tripwire can still report empty tool_*_guardrail_results; defer or recompute the details after the run loop has settled.

AGENTS.md reference: AGENTS.md:L120-L120

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e2bcc7 — but it only reproduces under a specific consumer pattern, which is worth spelling out since a naive repro looks like a non-repro.

With a fast consumer (async for _ in streamed.stream_events(): pass) the loop parks on the empty event queue while the tool call is in flight, so _check_errors() does not run until after the turn has already accumulated, and the reported list is correct by luck. Making the consumer yield between events (await asyncio.sleep(0.08)) exposes exactly the ordering you describe. Instrumenting _accumulate_tool_guardrail_results and _create_error_details:

('tool_guardrail_ran',    2.047)
('tripwire_queued',       2.078)
('create_error_details',  2.141, 'InputGuardrailTripwireTriggered', [])          <- frozen here
('accumulate',            2.656, ['input_allows'])                              <- results land after
PROBE run_data in= []
STREAMED OBJ in= ['input_allows']                                               <- object and exception disagreed

Fix: mark the details provisional when a tripwire is dequeued, and rebuild them in the finally block after await self._await_task_safely(self.run_loop_task), where _check_errors() is already re-run:

('create_error_details', 2.781, 'InputGuardrailTripwireTriggered', ['input_allows'])
PROBE run_data in= ['input_allows']

Only refreshing (never replacing) the stored exception, and only when it already had run_data, so the exception identity and the "first stored exception wins" behaviour are untouched. test_streamed_parallel_tripwire_details_refresh_after_the_turn_settles uses the slow consumer and additionally asserts the exception's list matches streamed.tool_input_guardrail_results — that cross-check is what the earlier fast-consumer test could not catch.

Verification for all three P2s in this round: each new test fails on 5140b87 (assert [] == ['input_allows']) and passes on 3e2bcc7; tests/test_tool_guardrails.py 38 passed; full suite A/B is 68 failures on the merge base vs 68 with the change (+3 passed, exactly the new tests), so no regressions. ruff and mypy clean on run.py and result.py.

LHMQ878 added 2 commits August 3, 2026 14:50
A parallel input guardrail trip discards the overlapped model turn, dropping the
tool guardrails that turn already ran in three ways:

- The no-cancel path (Temporal replay compatibility) never harvested the model
  task, even when it had already finished a guarded tool turn.
- On the cancel path, `asyncio.gather` returns a fresh `CancelledError` for a
  cancelled task, so the partials the function-tool executor recorded on the
  original exception were unreachable. Capture them inside the task instead, into
  lists owned by the caller that builds `RunErrorDetails`.
- A streamed tripwire froze `run_data` while the turn was still running, so
  results appended afterwards never reached the raised exception. Refresh the
  stored details once the run loop settles.
…sults-run-error-details

# Conflicts:
#	src/agents/run.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2954c3ce42

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

shell_results,
apply_patch_results,
local_shell_results,
) = await asyncio.gather(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Drain sibling tool tasks before raising

When a non-function tool fails before a slower guarded function tool has finished, asyncio.gather propagates the sibling exception immediately and leaves the function-tool awaitable running; this handler then copies function_tool_input_results/function_tool_output_results at that instant and raises, so guardrail results appended milliseconds later are omitted from RunErrorDetails and the tool work can continue after the run has already failed. Fresh evidence in this revision is that the final fix still records the shared partial lists immediately after asyncio.gather raises instead of cancelling/awaiting the function-tool side before building the error details.

AGENTS.md reference: AGENTS.md:L128-L128

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 27086ef6, though the mechanism turned out to be split differently than described — worth spelling out, because it changes what the test has to assert.

My first probe was invalid: I used a FunctionTool as the "non-function sibling", which goes through the same executor and so never reaches the asyncio.gather branch at all. Re-probed with a real non-function sibling (LocalShellTool whose executor raises) alongside a slow guarded function tool:

                       base                fixed
at raise            in_ran=1 out_ran=0   in_ran=1 out_ran=0
reported input      ['in_ran']           ['in_ran']
reported output     []                   []
after run settled   in_ran=1 out_ran=1   in_ran=1 out_ran=0
                                 ^ leak            ^ drained

So, taking the two halves separately:

The leak is real — the last row is the one that matters. asyncio.gather propagates the sibling exception immediately and abandons the function-tool coroutine, which kept running to completion and executed its output guardrail after Runner.run had already raised. Fixed by holding it as a task and draining it in the handler:

        function_tool_task = asyncio.ensure_future(execute_function_tool_calls(...))
        try:
            ... = await asyncio.gather(function_tool_task, ...)
        except BaseException as exc:
            if not function_tool_task.done():
                function_tool_task.cancel()
            await asyncio.gather(function_tool_task, return_exceptions=True)
            _record_tool_guardrail_partials(exc, ...)
            raise

The "results appended milliseconds later are omitted" half does not survive the probe as a lost-data claim. Once the tool is cancelled mid-body its output guardrail correctly never runs, so reported output: [] is accurate rather than truncated — there is no result to report. The input guardrail result was already reported on both sides. The fix is the leak, not a recovery of missing results, and reported output is [] before and after by design.

One test note, since it bit me: the pre-existing test_sibling_tool_failure_reports_function_tool_guardrail_results uses a rejecting input guardrail, and a rejected call never runs the tool body — so there was never anything in flight for the sibling failure to abandon, and it passed while the leak was live. The new test allows the input, uses a 1.0s body, and asserts that nothing runs after the failure:

without the fix: 1 failed, 1 passed
    tool work continued after the run failed: ['tool_body', 'output_guardrail']
with the fix:    2 passed

Full suite: 69 failed / 5498 passed vs 69 failed / 5474 passed at main, identical failing test-ID sets (JUnit XML comparison).

Comment thread src/agents/run.py Outdated
model_outcome = await asyncio.gather(
model_task, return_exceptions=True
)
elif model_task.done():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve pending no-cancel turn guardrails

In the Temporal/replay compatibility path where should_cancel_parallel_model_task_on_input_guardrail_trip() is false, a parallel input tripwire can arrive while the model task is still pending after a function-tool guardrail has already run, such as a slow tool body after a fast allowing input guardrail. Because this branch only harvests when model_task.done() is already true, and _capture_turn_tool_guardrail_partials() only copies partials when the model task exits, the raised tripwire still reports empty tool_*_guardrail_results; fresh evidence in this revision is the remaining elif model_task.done() fallback to an empty model_outcome for pending no-cancel turns.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 377a0067. This one was real and my previous round had left exactly the hole you describe.

Repro — a parallel input guardrail tripping while a slow tool body is still in flight, tool input guardrail already having run:

                    before            after
no_cancel=False  -> ['input_allows']  ['input_allows']
no_cancel=True   -> []            <-  ['input_allows']

runs=1 in both rows, so the guardrail definitely ran and the result was simply dropped.

The reason elif model_task.done() couldn't be widened into a peek is that the partials are not reachable from run.py while the task is pending: they live in a local inside execute_tool_calls_and_side_effects' parallel branch (function_tool_input_results) and only surface when the turn settles, either as a SingleStepResult or recorded on the propagating exception. So the fix awaits the turn rather than skipping it:

                                    else:
                                        # ... A turn that is still pending is awaited rather than
                                        # skipped: its tool guardrail results live in a local
                                        # inside the tool executor until it settles, and this
                                        # configuration has opted out of cancelling it, so letting
                                        # it finish is also what keeps it from outliving the run.
                                        model_outcome = await asyncio.gather(
                                            model_task, return_exceptions=True
                                        )

Awaiting is also the behaviour this branch already wants: should_cancel_parallel_model_task_on_input_guardrail_trip() == False means the caller has explicitly opted out of cancelling the turn, so letting it complete is what keeps it from outliving the run — the alternative would leave it orphaned.

Test-wise, the existing no-cancel test used tool_sleep=0.0, i.e. a turn that had already settled, which is why it passed while this was broken. It's now parametrized over turn_already_done / turn_still_pending; only the pending case fails without the change:

without the fix: 1 failed, 1 passed
with the fix:    2 passed

Full suite: 69 failed / 5497 passed vs 69 failed / 5474 passed at main, identical failing test-ID sets compared as JUnit XML.

LHMQ878 and others added 2 commits August 3, 2026 16:27
In the Temporal/replay compatibility path the turn is not cancelled, but the
branch only harvested it when `model_task.done()` was already true. A tripwire
arriving while a slow tool body is still running therefore reported empty
tool guardrail results even though the tool's input guardrail had run.

The results are not reachable from here while the task is pending -- they live
in a local inside the tool executor until the turn settles -- so await it
instead of skipping. That is consistent with the branch's own intent: this
configuration has explicitly opted out of cancelling the turn, so letting it
finish is also what keeps it from outliving the run.

Parametrizes the existing no-cancel test over settled/pending. Only the pending
case fails without this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`asyncio.gather` propagates a sibling tool's exception as soon as it is raised
and leaves the function-tool coroutine running, so recording its partials right
there both read a list that was still being appended to and let the tool body
keep executing after the run had already failed.

Hold the function-tool call as a task so the handler can cancel and drain it
before reading the shared partial lists. The existing sibling test used a
rejecting input guardrail, which prevents the tool body from ever running, so
nothing was left in flight for it to catch; the new test allows the input and
uses a slow body, and asserts no tool work happens after the failure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27086ef6cf

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/run.py
model_task,
)
except InputGuardrailTripwireTriggered:
except InputGuardrailTripwireTriggered as tripwire_exc:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Harvest model partials for guardrail exceptions

In non-streaming runs where a parallel input guardrail raises a non-tripwire AgentsException (for example, a guardrail function raises UserError) after the overlapped model turn has already run function-tool guardrails, this tripwire-only branch is skipped; the following except BaseException drains model_task but discards its result/partials. The outer RunErrorDetails merge then only sees partials attached to the guardrail exception, so run_data.tool_*_guardrail_results stays empty despite those guardrails having run; harvest the drained model outcome for non-tripwire parallel guardrail failures as well.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in e0ec7f5b. Probe with an allowing tool input/output guardrail pair, a parallel input guardrail that sleeps 0.4s then raises, and a run counter so the test can't pass vacuously:

                          guardrails that actually ran   reported_in   reported_out
before, UserError         ['in_ALLOW', 'out_ALLOW']      []            []
before, tripwire (ctrl)   ['in_ALLOW', 'out_ALLOW']      ['in_ALLOW']  ['out_ALLOW']
after,  UserError         ['in_ALLOW', 'out_ALLOW']      ['in_ALLOW']  ['out_ALLOW']
after,  tripwire (ctrl)   ['in_ALLOW', 'out_ALLOW']      ['in_ALLOW']  ['out_ALLOW']

Same result with ModelBehaviorError. The except BaseException branch now harvests the drained outcome the same way the tripwire branch does — a guardrail failing by raising discards the turn for the same reason, so it should report it for the same reason.

One thing worth noting on the shape of the fix: that branch drains gather(guardrail_task, model_task), so the outcome sequence includes the guardrail task's own result too. That's harmless — _record_parallel_model_task_guardrail_partials no-ops on entries with nothing to contribute, and _record_tool_guardrail_partials dedups by id(), so the exception that already carries its partials isn't double-counted. I added that to the docstring rather than filtering the sequence, since filtering would just re-encode which task is which.

The streamed path was already correct here — I checked before changing anything, and run_streamed reported ['in_ALLOW'] / ['out_ALLOW'] at 27086ef6. This was a run-only gap.

Regression: test_parallel_guardrail_raising_reports_guardrails_from_the_discarded_turn, parametrized over non_streamed/streamed. A/B at 27086ef6: exactly the non_streamed case fails (assert [] == ['input_allows']), the streamed case passes both ways — which matches the probe and keeps the parity pinned going forward.

Full suite A/B vs main (bdc294fc): 69 failed / 5500 passed / 76 skipped, JUnit failure-ID sets identical. ruff check, ruff format --check clean; mypy reports only the pre-existing sandbox/util/tar_utils.py:161 error, unrelated to this change.

A parallel input guardrail can fail without tripping - by raising - and that
discards the overlapped model turn exactly as a tripwire does. Only the
tripwire branch harvested it, so a turn whose tool guardrails had already run
reported `tool_input_guardrail_results == []` on the resulting error.

Harvest the drained outcome on the non-tripwire path too. Results are
deduplicated by identity, so an exception already carrying its own partials is
not double-counted.

The streamed path was already correct here; the regression covers both so the
parity stays pinned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0ec7f5b40

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/result.py
Comment on lines 1015 to +1016
if isinstance(in_guard_exc, AgentsException) and in_guard_exc.run_data is None:
in_guard_exc.run_data = self._create_error_details()
in_guard_exc.run_data = self._create_error_details(in_guard_exc)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh streamed data for raised input guardrails

When a streamed run has a run_in_parallel input guardrail raise an AgentsException after function-tool guardrails have already run but before the tool turn returns, this path freezes in_guard_exc.run_data from the current RunResultStreaming state before the run loop has either accumulated or harvested the in-flight tool guardrail results. Unlike the queued tripwire branch above, it never marks those details stale, and run_input_guardrails_with_queue() cancels the run loop on the raised guardrail exception, so the final exception can still report empty tool_*_guardrail_results for the cancelled turn; please harvest the in-flight turn partials or rebuild these details after settlement for raised parallel guardrail failures too.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and fixed in e0546cd4. My earlier regression let the tool turn finish before the guardrail failed, which is why the streamed case passed and I reported the gap as run-only. With the failure landing mid-tool-call the streamed path does lose the results, exactly as described.

Probe matrix — a tool that holds the turn open on an event, an allowing tool input/output guardrail pair, and a run counter so no row can pass vacuously. slow_tool=True means the guardrail fails while the tool call is still in flight:

                                          reported in / out        guardrails that ran
BEFORE
  streamed  tripwire      slow_tool=True   ['in_ALLOW'] ['out_ALLOW']  ['in_ALLOW','out_ALLOW']
  streamed  UserError     slow_tool=True   []           []             ['in_ALLOW']         <-- bug
  streamed  ModelBehavior slow_tool=True   []           []             ['in_ALLOW']         <-- bug
  streamed  UserError     slow_tool=False  ['in_ALLOW'] ['out_ALLOW']  ['in_ALLOW','out_ALLOW']
  non_str   UserError     slow_tool=True   ['in_ALLOW'] []             ['in_ALLOW']
AFTER
  streamed  UserError     slow_tool=True   ['in_ALLOW'] []             ['in_ALLOW']
  streamed  ModelBehavior slow_tool=True   ['in_ALLOW'] []             ['in_ALLOW']
  (every other row unchanged)

The tripwire row directly above the bug is the control: same timing, same tool, and it reports correctly — so the difference is the branch, not the schedule.

Marking the details stale is necessary but not sufficient, which is the part worth flagging. run_input_guardrails_with_queue cancels the run loop on a raised guardrail (a tripwire lets the turn complete instead), so refreshing from the current state would just rebuild from empty accumulators. Instrumenting run_single_turn_streamed shows where the data actually dies:

seen inside the run loop task : CancelledError: in=['in_ALLOW'] out=[]
run_loop_task.cancelled()     : True   -> Task.exception() raises; the annotated error is gone
streamed_result accumulators  : [] []  -> _accumulate_tool_guardrail_results never ran

So the partials exist, but only inside the task. The fix has both halves:

  1. _capture_streamed_turn_tool_guardrail_partials wraps the turn and copies the partials onto the streamed result, which outlives the cancellation. This is the same shape as _capture_turn_tool_guardrail_partials on the non-streaming path — the owner is just the streamed result rather than a local list.
  2. The raised-guardrail branch in _check_errors now sets _stored_exception_details_stale = True, so _refresh_stale_error_details() rebuilds after _await_task_safely(run_loop_task) settles.

Merging dedups by id() rather than equality, matching _record_tool_guardrail_partials, so a turn whose aborting error did survive the boundary isn't counted twice — visible above in the tripwire rows staying at exactly one entry each.

Regression: test_parallel_guardrail_raising_mid_turn_reports_the_guardrails_that_ran, parametrized over non_streamed/streamed, using an asyncio.Event rather than a sleep race so the ordering is deterministic. A/B at e0ec7f5b: exactly the streamed row fails (AssertionError: assert [] == ['input_allows']) and non_streamed passes both ways — matching the probe. It also asserts the output guardrail list stays empty, since that guardrail genuinely never ran.

Full suite A/B: 69 failed / 5502 passed / 76 skipped, JUnit failure-ID sets identical to e0ec7f5b (no regressions, no accidental fixes). ruff check and ruff format --check clean; mypy reports the same 40 pre-existing errors in 7 unrelated files before and after, none in result.py or run_loop.py.

Good catch — this was the streamed sibling of the same branch-parity gap, and my own test's timing hid it.

A parallel input guardrail that fails by *raising* cancels the streamed run loop,
unlike a tripwire, which lets the turn finish. The aborted turn records its tool
guardrail partials on the error that propagates out, but `asyncio` replaces a
cancelled task's exception with a fresh `CancelledError`, so by the time
`_check_errors()` builds `RunErrorDetails` they are gone - the run reported no
tool guardrails despite having run one.

Copy them out from inside the run loop task, onto the streamed result that
outlives the cancellation, and mark the stored exception's details stale in the
raised-guardrail branch so they are rebuilt once the loop settles. The tripwire
branch already did the latter; this is the sibling that was missing it.

The non-streaming path solves the same problem with caller-owned lists in
`_capture_turn_tool_guardrail_partials`; the owner here is the streamed result.
Merging dedups by identity, matching `_record_tool_guardrail_partials`, so a
turn described by both paths is not double-counted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0546cd418

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/result.py
# the turn's tool guardrail results are still in flight here for the same
# reason a tripwire's are. Refresh once the loop has settled and the
# aborted turn has copied its partials out.
self._stored_exception_details_stale = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh stale details even when run_data exists

When a streamed run_in_parallel input guardrail raises an AgentsException that already has run_data (for example it propagates a nested Runner.run failure) after the model turn has run a function-tool guardrail, this stale marker is skipped because it is inside the in_guard_exc.run_data is None branch. Fresh evidence after the previous fix is that the new cancelled-turn partials now live on streamed_result._turn_tool_*_partials, but _refresh_stale_error_details() is never invoked for pre-populated run_data, so the raised outer error still reports the nested details and omits the tool guardrail that ran in the outer streamed turn; rebuild/refresh the details for this raised-guardrail path even when replacing existing run_data.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not making this change, because the probe says the current behaviour is intended rather than a gap — but the scenario is real and reachable, so here is what I found.

Built the exact case: a streamed run_in_parallel input guardrail that runs a nested Runner.run, whose own tool guardrail runs and whose tool then raises, so the AgentsException propagating into the outer _check_errors already carries run_data. The outer turn has run its own function-tool guardrail by then.

guardrails that ran      : ['OUTER_in_ALLOW', 'INNER_in_ALLOW']
exception                : ModelBehaviorError | inner run blew up
run_data present         : True
run_data.last_agent      : inner        <-- the nested run's details
run_data tool in / out   : ['INNER_in_ALLOW'] []

So the precondition is reachable and the outer turn's OUTER_in_ALLOW is indeed absent. The question is whether that's a defect, and I don't think it is:

The run_data is None guard is deliberate and predates this PR. It arrived in #4004 (3142f3ac, "cancel streamed models when input guardrails fail") and is applied uniformly across all three branches in _check_errors — run loop, input guardrails, output guardrails — not just the one this PR touches. Its purpose is that an exception which already describes a failed run keeps its own details rather than having them replaced by the details of whatever outer run happened to observe it.

A/B at the merge base confirms this PR didn't change it. run_data.last_agent is inner both at bdc294fc and at e0546cd4; only the field the probe reads is new. Rebuilding the details here, as suggested, would overwrite the nested run's last_agent, raw_responses and input with the outer run's — turning a report about the run that actually failed into a report about its caller, and losing INNER_in_ALLOW in the process. That trades one missing guardrail result for a strictly worse loss.

It is also not specific to the raised-guardrail path. Any nested-run exception surfacing through any of the three branches behaves identically. If the intended semantics really are "merge the observing run's tool guardrails into a nested run's details," that's a deliberate cross-cutting change to RunErrorDetails ownership — new field or a documented merge policy — and it belongs in its own PR against all three branches, not smuggled into this one where it would be invisible to reviewers looking at a guardrail fix.

Happy to file that as a separate issue if maintainers think the nested case should merge rather than preserve. Marking this one as working-as-intended for the scope of this PR.

The sibling comment on constructor ordering was correct and is fixed in 56ca637c.

Comment thread src/agents/result.py Outdated
_triggered_input_guardrail_result: InputGuardrailResult | None = field(default=None, repr=False)
_output_guardrails_task: asyncio.Task[Any] | None = field(default=None, repr=False)
_stored_exception: Exception | None = field(default=None, repr=False)
_stored_exception_details_stale: bool = field(default=False, repr=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve streaming constructor positional order

Because RunResultStreaming is an exported dataclass constructor, inserting these new internal fields here shifts the positional meaning of the existing fields that follow, such as _cancel_mode, _last_processed_response, and interruptions. Callers/tests that pass those optional fields positionally now bind values to _stored_exception_details_stale or the partial-list fields instead; make these new bookkeeping fields init=False or append them after the existing init fields so released positional call patterns keep working.

AGENTS.md reference: AGENTS.md:L80-L84

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 56ca637c — and it was worse than reported: this PR added three init fields ahead of _cancel_mode, not two (_stored_exception_details_stale came in with the first commit).

Positional index dump before the fix:

   22  _stored_exception
   23  _stored_exception_details_stale        <-- added by this PR
   24  _turn_tool_input_guardrail_partials    <-- added by this PR
   25  _turn_tool_output_guardrail_partials   <-- added by this PR
   26  _cancel_mode              (was 23)
   27  _last_processed_response  (was 24)
   28  interruptions             (was 25)

All 41 parameters are POSITIONAL_OR_KEYWORD, so the shift is real rather than theoretical — and tests/test_source_compat_constructors.py shows this repo treats that ordering as a released surface, with tests pinning the v0.7.0 positional prefix.

None of the three is ever passed by a caller (grep finds only the declarations plus internal reads in result.py and run_loop.py), so they're now init=False, matching the internal state already on this class — _active_stream_consumers, _stream_consumers_stopped, _current_agent_ref.

Also added test_run_result_streaming_positional_binding_survives_internal_bookkeeping_fields, which pins the fields after the new internal state so a future bookkeeping field can't silently take a released slot. A/B: reverting just the three init=False markers fails it with AssertionError: assert 'none' == 'immediate' — the value intended for _cancel_mode landing on _stored_exception_details_stale, exactly the binding you described.

Full suite re-run: 69 failed / 5503 passed / 76 skipped, JUnit failure-ID set identical to both the merge base and e0546cd4 (collected count +1, the new test). ruff check / ruff format --check clean.

Good catch — worth flagging that the pre-existing init=False fields on this class were the right pattern and I should have followed them from the start.

`RunResultStreaming` is an exported dataclass whose positional argument order is pinned by
tests/test_source_compat_constructors.py. The three bookkeeping fields added by this PR for
error-detail refreshing were declared as init fields, which shifted `_cancel_mode`,
`_last_processed_response` and `interruptions` three slots to the right - a caller passing
`_cancel_mode` positionally would bind it to `_stored_exception_details_stale` instead.

None of the three is ever passed by a caller, so mark them `init=False`, matching the other
internal state on the same class (`_active_stream_consumers`, `_stream_consumers_stopped`).

Adds a compat test pinning the fields that sit *after* the new internal state, so a future
bookkeeping field cannot silently take a released positional slot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LHMQ878

LHMQ878 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebase/merge status check, since main has moved four commits since this branch's base and one of them lands in the same file.

686d041b (#4130, "synchronize after-turn cancellation with event consumption") also touches src/agents/result.py, and adds _active_stream_consumers / _stream_consumers_stopped to RunResultStreaming — both as init=False. That is the same convention this branch adopted in 56ca637c, so the two changes agree on how new internal state should be added to that dataclass rather than competing.

I verified the combination rather than trusting the "mergeable" flag:

  • git merge c546ca12 into 56ca637c auto-merges with no conflicts (git diff --diff-filter=U is empty), including run_internal/run_loop.py, which 9af785b1 (fix(run): stop emitting handoff calls as streamed tool_called events #4146) also modified.
  • Full suite on the merge result: 5449 passed / 133 failed. Suite on c546ca12 alone: 5422 passed / 131 failed.
  • The +2 delta is tests/test_tracing.py::test_spans_with_setters and tests/test_tracing_errors.py::test_multiple_final_output_doesnt_error. Both are pre-existing and order-dependent, not caused by this branch: run in isolation at c546ca12 with no other tests, they fail identically with the same KeyError in tests/testing_processor.py:155 (parent = nodes[(trace_id, parent_id)]). Running the two tracing files together gives 12 failed / 14 passed at both heads — byte-identical counts. They pass in main's full-suite run only because span state from earlier tests happens to populate nodes; the merge shifts collection order enough to expose that.

So: no conflict, no regression attributable to this branch. Happy to push the merge (or rebase, if you prefer a linear history) whenever it's useful — I've left the branch at 56ca637c for now so the review threads above stay anchored to the commits they discuss.

One note on this repo's environment for anyone reproducing the numbers: tests/test_run_state.py isn't collectable on Windows, and a handful of the baseline failures are network-dependent (ConnectionError out of tests/extensions), which is why the absolute failure count is high on both sides. The A/B is the meaningful part, not the raw totals.

@seratch

seratch commented Aug 5, 2026

Copy link
Copy Markdown
Member

Thanks again for working on this here. Your contribution is included in #4180 via co-authored-by credit.

@seratch seratch closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants