Python: bind and recover FIDES policy approvals - #8142
Python: bind and recover FIDES policy approvals#8142Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
Conversation
GitHub Actions
Active
GitHub Actions
Failure
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
GitHub Actions
Active
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| else: | ||
| stale_response = await agent.run(resume_message, session=session) | ||
|
|
||
| assert calls == 0 |
| else: | ||
| await agent.run(stale_approval, session=session) | ||
|
|
||
| assert calls == 1 |
There was a problem hiding this comment.
🟡 Changes recommended
Provider-scoped and AG-UI rejection cleanup is incomplete, and capacity validation can be bypassed with NaN.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens Python FIDES approvals across persistence and resume.
Changes:
- Adds occurrence-bound, FIFO/TTL-limited policy approvals.
- Preserves replacement approval requests and balanced history.
- Adds rejection/cancellation cleanup with expanded tests.
File summaries
| File | Description |
|---|---|
agent_framework/security.py |
Adds approval lifecycle and cleanup logic. |
agent_framework/_tools.py |
Handles replacement requests and non-grants. |
agent_framework/_sessions.py |
Preserves unresolved approval controls. |
agent_framework/_middleware.py |
Adds the non-grant notification hook. |
tests/test_security.py |
Tests bounds, expiry, and cleanup. |
tests/core/test_harness_tool_approval.py |
Tests end-to-end reapproval flows. |
tests/core/test_function_invocation_logic.py |
Tests replacement request collection. |
docs/specs/004-python-function-calling-loop.md |
Documents new approval semantics. |
Review details
Suppressed comments (1)
python/packages/core/agent_framework/security.py:2712
- This duplicate validation also accepts
float("nan"), after which the policy middleware's capacity comparison can never evict records. Validate that the public configuration value is an actual integer so the advertised bound cannot be disabled by an untyped runtime value.
if isinstance(max_pending_approvals, bool) or max_pending_approvals < 1:
raise ValueError("max_pending_approvals must be at least 1.")
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if middleware_pipeline is not None and responses_not_granted: | ||
| middleware_pipeline.notify_approval_responses(responses_not_granted, session=invocation_session) |
| if isinstance(max_pending_approvals, bool) or max_pending_approvals < 1: | ||
| raise ValueError("max_pending_approvals must be at least 1.") |
| scope = self._scope_for_session(session) | ||
| self._prune_pending_approvals(scope) |
![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)): c7b777531e77
Model: gpt-5.6-sol-fast
Overview
The PR strengthens FIDES approvals with occurrence-bound identities, durable binding signatures, FIFO/TTL limits, fail-closed reapproval, and streaming/non-streaming coverage. The replacement-request and transcript-normalization paths have concrete guards against stale or replayed authority. One provider-path scope mismatch remains: rejection and cancellation notifications inspect standalone middleware state instead of the fixed SecureAgentConfig scope, so the promised prompt cleanup does not occur.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/security.py
| session: AgentSession | None, | ||
| ) -> None: | ||
| """Discard authenticated non-grants from only their owning security scope.""" | ||
| scope = self._scope_for_session(session) |
There was a problem hiding this comment.
When SecureAgentConfig clones this middleware, pending approvals live in its fixed provider scope (session.state[source_id]), but this lookup uses the inherited standalone state key. An authenticated rejection or cancellation therefore inspects an empty scope and leaves the matching record until TTL pruning or FIFO eviction, contrary to the new cleanup contract. For fixed-scope clones, use the fixed scope here (mirroring _activate_security_scope) while retaining session lookup for reusable standalone middleware.
| return current_binding.binding_key() == pending.binding_key() and self._response_matches_pending( | ||
| approval_response, approval_id, call_id, pending.body_signature | ||
| ) |
There was a problem hiding this comment.
Should a replacement approval carry a fresh generation token? After TTL expiry or FIFO eviction, _request_policy_violation_approval stores a new record under the same occurrence ID, but this check excludes created_at; because the call body and IDs are also unchanged, replaying the original serialized grant satisfies the replacement and executes without a second user decision. Could we rotate a request-generation nonce for each replacement and require it in _response_matches_pending?
| def on_approval_responses( | ||
| self, | ||
| responses: Sequence[Content], | ||
| *, | ||
| session: AgentSession | None, | ||
| ) -> None: |
There was a problem hiding this comment.
Could we keep this approval-lifecycle callback behind a private observer protocol instead of adding it to public FunctionMiddleware? Callers of on_approval_responses must know that responses were rebound to the active session, authenticated against the pending snapshot, and filtered to decisions that will not execute, but the only implementation is experimental PolicyEnforcementFunctionMiddleware, and callable middleware cannot participate. A private capability implemented by the policy middleware and dispatched by FunctionMiddlewarePipeline would keep those rules in the approval subsystem without expanding the stable middleware API.
c7b7775 to
268d0b8
Compare
GitHub Actions
Active
GitHub Actions
Active
Code Coverage OverviewLanguages: Python Python / code-coverage/pythonThe overall line coverage in commit 5e42eb2 in the Show a line coverage summary of the most covered files.
Updated |
Bound FIDES policy approvals per session with FIFO and TTL expiry, clean authenticated non-grants by occurrence, and persist visible replacement approvals so stale grants require a safe second approval.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
268d0b8 to
5e42eb2
Compare
GitHub Actions
Active
GitHub Actions
Active
Motivation & Context
A FIDES policy approval must authorize exactly one reviewed invocation across interruption, persistence, and resume. Reused provider call IDs, stale non-grants, unbounded pending records, or result replacement that drops approval context can otherwise bind authority to the wrong occurrence or make safe recovery impossible.
Description & Review Guide
Related Issue
Fixes #7455. Depends on #8141 and is the top layer of the 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.