Python: Fix active mixed-pause Host response correlation - #8582
RongJie G (CorgiBoyG) wants to merge 1 commit into
Conversation
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two correlation edge cases can reject valid responses or incorrectly reuse a staged Host result.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Scopes mixed-pause response correlation to the active persisted batch.
Changes:
- Adds response-history watermarks and fail-closed validation.
- Prioritizes occurrence-identified Host results and expands regression coverage.
- Supports full-transcript and delta partial resumes.
| File | Description |
|---|---|
python/packages/core/agent_framework/_tools.py |
Implements active-batch response ownership and replay handling. |
python/packages/core/tests/core/test_function_invocation_logic.py |
Adds mixed-pause ownership and conflict regressions. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if any(historical == candidate_state for historical in historical_responses): | ||
| raise RuntimeError( | ||
| "The supplied response is indistinguishable from history that predates the active " | ||
| "mixed-pause batch." | ||
| ) |
There was a problem hiding this comment.
Fixed in 2e0e317. The historical-payload ambiguity check now applies only when no unique history watermark boundary can be established. When distinct message identities prove the active suffix, an equal current response is accepted. Added a regression covering distinct historical/current message IDs with the same occurrence ID and payload.
| candidate_state = candidate.to_dict() | ||
| for item in items: | ||
| stored_response = item.get("response") | ||
| stored_content = _content_from_state(stored_response) | ||
| if stored_content is None or stored_content.to_dict() != candidate_state: | ||
| continue |
There was a problem hiding this comment.
Fixed in 2e0e317. Matching now distinguishes responses persisted before the current resume from responses matched during this resume. An id-less result equivalent to a preexisting identified Host response fails closed if it could fill another occurrence, while identified responses still reserve their occurrences first. The regression matrix also covers fresh versus partial resumes, stateful versus stateless matching, equal and distinct payloads, both input orders, and approval terminal-result preservation.
f7f6816 to
2e0e317
Compare
GitHub Actions
Active
| max_function_calls = self.function_invocation_configuration.get("max_function_calls") | ||
| max_duration_seconds = self.function_invocation_configuration.get("max_duration_seconds") | ||
| prepared_messages = _copy_messages_for_function_invocation(messages) | ||
| response_ownership_history = _copy_messages_for_function_invocation(messages) |
There was a problem hiding this comment.
RongJie G (@CorgiBoyG) This watermark history is captured before approval replay normalizes prepared_messages, so it can include a resolved function_approval_response that HistoryProvider later filters out. If an approval replay is followed by a new mixed approval/Host batch, the next full-history resume cannot match the stored watermark and equal current/historical Host payloads can fail as indistinguishable. Please base response_ownership_history on the post-_resolve_approval_responses transcript in both streaming and non-streaming paths before storing the batch.
There was a problem hiding this comment.
Addressed in 7d9f0a1. I rebuilt the correlation change on top of the post-#8579 implementation rather than carrying the old approval-normalization path forward. Both streaming and non-streaming now seed ownership from the post-_resolve_approval_responses prepared_messages, and the watermark is refreshed after the creating model response is normalized so a same-turn terminal result cannot satisfy a newly created Host occurrence. The persisted raw and canonical boundaries cover both public full transcripts and HistoryProvider-filtered history while rejecting ambiguous boundaries. Added three-stage streaming/non-streaming regressions for approval replay followed by a mixed batch, equal historical/current Host payloads, metadata-only streaming calls, and the no-current-Host case remaining incomplete. Core and affected package suites pass.
2e0e317 to
7d9f0a1
Compare
GitHub Actions
Active
7d9f0a1 to
90123fd
Compare
GitHub Actions
Active
|
RongJie G (@CorgiBoyG) this is far too large of a change to a core area of the framework, please provide more details about why all these changes are needed to solve the issue, and make sure to use the defined PR template |
90123fd to
b1e9ebe
Compare
GitHub Actions
Active
GitHub Actions
Active
|
Thanks for the feedback. I rebuilt the change from the latest main rather than extending the previous implementation. The PR is now reduced from 1,512 additions / 34 deletions to 162 additions / 19 deletions, with only 51 additions / 3 deletions in production code. The custom session codec, transcript watermark, provider-history handling, state-schema changes, and unrelated corruption hardening have all been removed. The remaining production changes are limited to (1) selecting a content-level active response window from a bound approval or exact Host occurrence and (2) preventing an id-less replay of a previously staged result from filling a sibling occurrence with the same call_id. I also rewrote the PR description using the repository template, including the review guide, impact, issue link, and validation results. The latest Python 3.13 workspace suite passes with 15,461 passed, 414 skipped, and 2 xfailed; the focused invocation suite and static checks also pass. When convenient, could you please take another look at the reduced patch? |
b1e9ebe to
28c5ce7
Compare
GitHub Actions
Active
GitHub Actions
Active
| for content in message.contents | ||
| if content.type in {"function_approval_response", "function_result"} | ||
| for index, content in indexed_contents | ||
| if response_start is not None |
There was a problem hiding this comment.
RongJie G (@CorgiBoyG) A valid Host-first mixed resume can still be dropped here: if the current reply contains an id-less Host result followed by the bound approval response, response_start anchors at the approval and filters out the preceding Host result. The completed batch is then staged as incomplete. Please retain supported current-message id-less Host results that precede a later approval anchor, without admitting older history, and add a Host-first regression.
There was a problem hiding this comment.
Addressed in 99b27f8. The active window now starts at the beginning of the message containing the first bound approval or exact Host occurrence, so an id-less Host result that precedes the approval in the same current message is retained, while matching results in earlier messages remain inert. I added a Host-first regression that includes a prior-message historical result with the same call_id to verify that boundary. During adversarial review I also added a fail-closed guard for conflicting id-less results competing for the same occurrence, plus coverage confirming that an exact occurrence-id result remains authoritative. The branch is rebased onto the latest main, including #8750. Python 3.13 workspace tests pass with 16,365 passed, 414 skipped, and 2 xfailed; the Python 3.11 CI typing matrix passes 205/205 package tasks and 3/3 sample tasks.
28c5ce7 to
99b27f8
Compare
GitHub Actions
Active
GitHub Actions
Active
| if ( | ||
| content.type == "function_approval_response" | ||
| and approval_anchor is None | ||
| and bind_approval_response(content) is not None |
There was a problem hiding this comment.
RongJie G (@CorgiBoyG) This anchor can come from an approval response that was already staged by an earlier partial resume. On a later full-history retry, replaying that old approval moves response_start back before the current reply, so an intervening stale id-less Host result with the active call_id can fill the remaining Host slot before the actual current result. Could the approval and exact-Host anchors ignore pending items that already have a response, with a regression covering a replayed staged approval plus stale and current Host results?
There was a problem hiding this comment.
Addressed in 91c4978. Strong anchors are now derived only from pending approval and exact Host items whose response is still unset. Replays of responses staged by an earlier partial resume remain available to the matcher for equivalence and conflict checks, but they can no longer move the active response window back into stale history. I added regressions for both cases: (1) a replayed staged approval followed by an intervening stale id-less Host result and the current result, and (2) a replayed staged exact Host result with a remaining sibling occurrence sharing the same call_id. Both verify that the stale message remains inert and the final current message completes the batch. The branch is rebased onto the latest main. Python 3.13 workspace tests pass with 16,547 passed, 414 skipped, and 2 xfailed; the Python 3.11 CI typing matrix passes 205/205 package tasks and 3/3 sample tasks.
99b27f8 to
91c4978
Compare
GitHub Actions
Active
GitHub Actions
Active
| default=None, | ||
| ) | ||
| approval_is_staged = any(item.get("kind") == "approval" and item.get("response") is not None for item in items) | ||
| if response_start is None and approval_is_staged and messages: |
There was a problem hiding this comment.
RongJie G (@CorgiBoyG) A Host-first partial resume can still drop an id-less Host result when it arrives in an earlier request than the approval. With no approval or exact-Host anchor yet, this approval_is_staged gate leaves response_start unset, so the result is not persisted; the later approval then cannot recover it unless the caller resends the Host result. Could the current/delta id-less Host result establish the active window even before approval is staged, with a split-resume regression?
Motivation & Context
An active session-backed mixed approval/Host batch can be resumed with a transcript that also contains an older id-less Host result using the same
call_id. The matcher previously considered every response in the supplied transcript, so that historical result could be consumed as an active duplicate or assigned to another active occurrence.This revision was rebuilt on the latest
mainafter review feedback. It reduces the PR from 1,512 additions / 34 deletions to 502 additions / 19 deletions, with 96 additions / 3 deletions in production code. It does not add a session-state schema, transcript watermark, custom serializer, or provider-history protocol, and it does not change the stateless path.Description & Review Guide
_stage_pending_mixed_pause_responses, select an active response window beginning at the message containing the first response that can be tied to the persisted batch: either an approval response bound to the pending request or an exact Host result matching(call_id, occurrence id). This retains supported Host-first contents in the same current message while earlier messages remain inert.call_idin the final message to establish the response window. Historical id-less results in earlier messages remain inert._match_mixed_pause_responses, remember which response slots were already staged before the current resume. An equivalent id-less replay cannot fill a different sibling occurrence that shares the samecall_id; a distinct result can still fill the unique unanswered occurrence._stage_pending_mixed_pause_responses._match_mixed_pause_responses.Validation:
test_function_invocation_logic.py: 382 passed, 1 skippedgit diff --check: passedRelated Issue
Fixes #8573
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.