Repository navigation
Python: Harden FIDES service history continuation - #9877
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 25f03b1d9354
Model: gpt-5.6-sol
Overview
The PR resolves effective history ownership and moves local history loading ahead of FIDES approval preparation, with strong streaming/non-streaming coverage and explicit rejection of several opaque continuation cases. However, the new fail-closed gate does not recognize every client-declared continuation carrier, treats any local message as a complete reconstruction, and does not bind restored FIDES state to the resumed conversation. The middleware reorder also changes which request is persisted, while the approval preload invokes custom history providers twice before the first service call.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
5 verified findings remained after source verification (3 high, 2 medium) across 2 files. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_agents.py, python/packages/core/agent_framework/security.py
| if ( | ||
| reconstruct_history_unstamped | ||
| and not has_local_history | ||
| and (context.service_session_id is not None or context.options.get("conversation_id") is not None) |
There was a problem hiding this comment.
This fail-closed check misses client-declared continuation carriers. OpenAI treats previous_response_id and conversation as service-side continuations and _agents.py forwards both unchanged, so either option can resume opaque history while this condition remains false and FIDES dispatches without reconstructing its label. Please detect every non-empty option in the client's SERVICE_CONTINUATION_OPTION_NAMES (or pass an explicit effective-continuation signal) before allowing dispatch.
| else _ApprovalReplayAuthority() | ||
| ) | ||
| context.metadata[_APPROVAL_REPLAY_AUTHORITY_KEY] = approval_authority | ||
| has_local_history = any(context.context_messages.get(source_id) for source_id in history_source_ids) |
There was a problem hiding this comment.
The presence of one local message does not establish that the full service transcript was reconstructed. With a partial, retained, or unrelated provider archive plus a service continuation, this becomes true, the fail-closed branch is skipped, and FIDES folds only the visible subset before marking reconstruction complete; opaque service history can then drive tools under an understated label. Only accept local history when its completeness and association with this continuation are established, otherwise fail closed.
| context.metadata[_APPROVAL_REPLAY_AUTHORITY_KEY] = approval_authority | ||
| has_local_history = any(context.context_messages.get(source_id) for source_id in history_source_ids) | ||
| if ( | ||
| reconstruct_history_unstamped |
There was a problem hiding this comment.
Once the session state contains a context label and history_reconstruction_complete=True, this gate never verifies that the label belongs to the continuation being resumed. Because FIDES state and service_session_id/runtime conversation_id can be restored or changed independently, state from one trusted conversation can authorize a different opaque conversation. Persist the normalized continuation identity with the authoritative label and fail closed when the current identity does not match.
| existing_middleware = cast( | ||
| "MiddlewareTypes | Sequence[MiddlewareTypes] | None", effective_client_kwargs.get("middleware") | ||
| ) | ||
| effective_client_kwargs["middleware"] = [ |
There was a problem hiding this comment.
Placing history persistence before all provider middleware makes it snapshot context.messages before downstream middleware can filter or replace the request. The leaf client receives the filtered messages, but HistoryProvider.after_run persists the earlier snapshot, so content deliberately removed before dispatch can still be written durably and downstream additions are omitted. Preserve the early history load needed by FIDES, but capture the messages to persist after provider request middleware has finalized the outbound request.
| for provider in self.context_providers: | ||
| if not isinstance(provider, HistoryProvider) or not provider.load_messages: | ||
| continue | ||
| await provider.before_run( |
There was a problem hiding this comment.
For local per-service-call history, this preload invokes each loadable provider here and the history middleware invokes before_run again for the first model call. Custom HistoryProvider.before_run is explicitly supported and may advance a cursor, consume records, mutate state, or return a different snapshot, so FIDES can authorize one transcript while the model receives another. Reuse this loaded snapshot for the first service call, then reload only for later function-loop calls.
25f03b1 to
4d7c69c
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical flaws allow service continuations to bypass authoritative FIDES history reconstruction and cause provider preparation and model dispatch to use different conversation IDs.
4 open issues
What changed in this PR
This PR adds the service-history continuation layer to the FIDES hardening stack, building on #9875 and #9876. It aims to prepare security state before approval execution and reject service continuations whose history cannot be safely reconstructed.
Changes:
- Resolve effective history ownership and propagate conversation identity into provider preparation.
- Load local history before approval checks and order per-service-call history middleware before provider middleware.
- Add streaming and non-streaming regressions for history ordering, write-only archives, and missing continuation security state.
| File | Description |
|---|---|
python/packages/core/tests/core/test_agents.py |
Adds history-ordering, ownership, and continuation regression tests. |
python/packages/core/agent_framework/security.py |
Adds a guard against service continuation without reconstructable history or authoritative FIDES state. |
python/packages/core/agent_framework/_agents.py |
Propagates conversation identity and history ownership; adjusts history preparation and middleware ordering. |
Review effort: Balanced
| resolved_conversation_id = ( | ||
| session_conversation_id if session_conversation_id is not None else effective_conversation_id | ||
| ) |
| has_local_history = any(context.context_messages.get(source_id) for source_id in history_source_ids) | ||
| if ( | ||
| reconstruct_history_unstamped | ||
| and not has_local_history | ||
| and (context.service_session_id is not None or context.options.get("conversation_id") is not None) |
| raise RuntimeError( | ||
| "Cannot reconstruct service-managed history without an authoritative FIDES context label." | ||
| ) |
| await provider.before_run( | ||
| agent=self, | ||
| session=provider_session, | ||
| context=approval_history_context, | ||
| state=provider_session.state.setdefault(provider.source_id, {}), |
Motivation & Context
FIDES security state must account for the effective history owner before approval execution or model dispatch. Local
history must be loaded before FIDES evaluates an approval or tool call, while service-managed history must not trigger
reads from write-only archival providers. A resumed service conversation with opaque history must fail closed when no
authoritative FIDES context label exists.
This is the third layer of the upstream replacement stack for #9652. It builds on #9875 and #9876 and isolates
service-history ownership, continuation identity, and pre-approval history preparation from the later AG-UI and MCP
carrier layers.
Description & Review Guide
explicit
store,STORES_BY_DEFAULT, andstore=False; propagates the effective session, runtime, or defaultconversation ID into provider preparation; and runs local history loading before FIDES approval preparation.
Per-service-call history middleware now runs before provider middleware so FIDES sees loaded history before function
execution. Service continuations without reconstructable local history or an authoritative FIDES label fail before
model dispatch, including configurations with no history provider or
load_messages=False.streaming and non-streaming runs. Service-owned history skips archival reads but cannot silently continue with
missing security state. There are no public API or breaking changes. Validation passed with:
cd python && uv run pytest -q packages/core/tests/core/test_agents.py -k 'psc_history_load_precedes_fides or history_load_precedes_fides_approval or fides_approval_history_respects_effective_service_history_ownership or fides_service_history_without or fides_effective_conversation_id_without_authoritative_state'(28 passed)cd python && uv run pytest -q packages/core/tests/test_security.py -k 'history_completion_marker_without_context_label_reconstructs or failed_history_load_keeps_conservative_reconstruction_for_retry'(2 passed)cd python && uv run poe syntax -P core -C(passed)cd python && uv run poe pyright -P core(0 errors and 0 warnings)cd python && uv run poe test-typing -P core(mypy, pyrefly, pyright, ty, and zuban passed)cd python && uv run poe test -P core(9,162 passed, 20 skipped, 2 xfailed)three
storemodes, and the fail-closed boundary for session, runtime, and default continuation IDs. This layerintentionally excludes AG-UI changes, MCP result/IFC work, and the replay/approval authority supplied by Python: Harden FIDES replay and approval authority #9876.
Related Issue
Fixes #9652
#9652 is the original source draft. This PR is its service-history and continuation replacement layer, stacked as
#9875 -> #9876 -> this PR, rather than a competing implementation.
AI Assistance
AI assistance details: GitHub Copilot assisted with implementation, regression tests, validation, and this PR
description.
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.