Python: bind and recover FIDES policy approvals - #8142
Python: bind and recover FIDES policy approvals#8142Eduard van Valkenburg (eavanvalkenburg) wants to merge 3 commits 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}
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 656da37 in the Show a line coverage summary of the most covered files.
Updated |
268d0b8 to
5e42eb2
Compare
GitHub Actions
Active
GitHub Actions
Active
5e42eb2 to
2099b86
Compare
GitHub Actions
Active
GitHub Actions
Active
2099b86 to
c45522f
Compare
GitHub Actions
Active
c45522f to
51612ec
Compare
GitHub Actions
Active
51612ec to
b0b5156
Compare
GitHub Actions
Active
GitHub Actions
Active
b0b5156 to
b021b5e
Compare
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
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>
Harden request-generation authority, keep lifecycle observation private, and notify fixed-scope FIDES policy middleware from authenticated AG-UI rejection and cancellation paths.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
b021b5e to
42d814f
Compare
GitHub Actions
Active
GitHub Actions
Active
Preserve immutable function occurrences while rotating approval generations, route authenticated AG-UI non-grants through private observers, and cover canonical, legacy, mixed-cancellation, and restored-session flows.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 or interleaved decisions, abandoned approval state, and expired or evicted authority can otherwise bind approval to the wrong occurrence, leak pending records, or allow an old serialized grant to authorize a replacement request.
Description & Review Guide
Related Issue
Fixes #7455 and #7890. 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 title prefix in sync automatically.