Repository navigation
Python: isolate FIDES security state per session - #8137
Eduard van Valkenburg (eavanvalkenburg) wants to merge 2 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new state-accessor behavior breaks an existing public call pattern despite the PR declaring no breaking change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves FIDES security state into AgentSession to prevent cross-conversation leakage.
Changes:
- Adds durable session-scoped labels, variables, audits, and approvals.
- Uses task-local middleware and preserves middleware configuration.
- Updates documentation, samples, and regression tests.
File summaries
| File | Description |
|---|---|
security.py |
Implements session-scoped FIDES state. |
test_security.py |
Tests isolation, restoration, concurrency, and serialization. |
FIDES_DEVELOPER_GUIDE.md |
Documents session-aware accessors. |
email_security_example.py |
Reads audit state from the active session. |
repo_confidentiality_example.py |
Creates and passes an explicit session. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if session is None and self._provider_state_used: | ||
| raise ValueError("session is required after SecureAgentConfig is used as a context provider") |
|
Superseded by #8138 using an in-repository head branch; native GitHub stacks do not support cross-fork heads. |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (2 commit(s)): e912fce745d4, 53e1ce19f123
Model: gpt-5.6-sol-fast
Overview
The PR moves cumulative labels, hidden variables, audit records, and pending approvals into session-owned state, with strong owner checks, strict durable serialization, task-local middleware binding, and restoration tests. Explicit sessions are isolated and resumable, but the new lifecycle makes security state from omitted-session runs unreachable and leaves several public middleware state APIs silently attached to a detached standalone scope.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/security.py
| Returns: | ||
| List of violation records, or empty list if policy enforcement disabled. | ||
| """ | ||
| if session is None and self._provider_state_used: |
There was a problem hiding this comment.
When Agent.run() is called without a session, the framework creates a private generated session, but this new gate then rejects config.get_audit_log() and the caller has no session object it can pass instead. Existing session-optional callers therefore lose access to that run's policy audit state. Please keep this workflow retrievable, for example by exposing a per-run session/state handle, while still preventing reads from an unrelated standalone scope.
| def _middleware_for_scope(self, scope: _SecurityScope) -> list[FunctionMiddleware]: | ||
| """Clone the current overridable middleware stack into one scope.""" | ||
| return [ | ||
| middleware._clone_for_scope(scope) # pyright: ignore[reportPrivateUsage] |
There was a problem hiding this comment.
These scoped clones leave the public config.label_tracker and config.policy_enforcer handles attached to standalone state. After a provider run, calls such as config.label_tracker.reset_context_label() succeed but do not reset the session, so the next run can remain tainted and keep blocking tools; there is no session-aware replacement for context-label reset, metadata access, or audit clearing. Please route these stateful public operations through an explicit session scope, or make the detached handles fail loudly instead of silently reading or mutating the wrong state.
Motivation & Context
FIDES middleware currently stores cumulative labels, hidden variables, audit records, and pending policy approvals on reusable middleware instances. Hosts that share one agent across conversations can therefore leak security state between sessions. This layer moves FIDES state into the owning
AgentSessionboundary while preserving standalone middleware behavior.Description & Review Guide
Related Issue
Part of #7455. This is the bottom layer of a four-PR FIDES hardening stack.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and the title prefix in sync automatically.