Python: Bound abandoned PolicyEnforcement pending approvals - #8203
Shivani . (Shivani767) wants to merge 1 commit into
Conversation
…t#7890) Cap pending policy-approval bindings with FIFO eviction (max_pending_policy_approvals, default 256) and TTL expiry (pending_policy_approval_ttl, default 1h, None disables) on top of the microsoft#6966 call-binding isolation. Clear pending state on explicit rejection via FunctionMiddlewarePipeline.notify_rejected_approvals -> discard_rejected_policy_approvals, threaded with invocation_session so rejection cleanup requires the same session_key match as grants. Expose both bounds through SecureAgentConfig. Tests: max-size eviction, TTL discard, resolver-path rejection cleanup, cross-session rejection isolation, config forwarding.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
| # different function, changed arguments, a different security label, or a different session. | ||
| self._pending_policy_approvals: dict[str, _PendingPolicyApproval] = {} | ||
| # OrderedDict preserves insertion order so abandoned entries can be evicted FIFO (#7890). | ||
| self._pending_policy_approvals: OrderedDict[str, _PendingPolicyApproval] = OrderedDict() |
There was a problem hiding this comment.
Could we rebase this onto the session-scoped policy state from #8138 before applying the bounds? This head still keeps one OrderedDict on the shared middleware and is 246 commits behind main, so sessions using the same call_id overwrite each other's pending bindings and one busy session can evict another session's valid approvals. The current base stores these records in self._scope.pending_approvals under occurrence-aware approval IDs, so the TTL, cap, and rejection cleanup need to operate on that representation rather than restore instance-global state.
There was a problem hiding this comment.
Thanks Evan Mattson (@moonbox3) — you were right, and this turned out to be decisive.
While preparing the rebase onto current main I found that #8142 has since landed exactly this design on top of the #8138 session-scoped state: pending approvals now live in self._scope.pending_approvals, keyed by occurrence-aware approval IDs, with a per-scope FIFO cap (max_pending_approvals, default 256) and TTL expiry (pending_approval_ttl, default 1h, None disables) that prune on access/write and survive session serialization. Rejection/non-grant cleanup runs through the authenticated AG-UI rejection and cancellation paths.
Since that supersedes the bounds work here (the instance-global OrderedDict no longer exists upstream), this PR is now redundant and I'm closing it as superseded by #8142 rather than re-proposing a duplicate. Thanks again for the detailed review, and for the guidance on #7893 — it directly shaped the approach that ultimately landed.
| for middleware in self._middleware: | ||
| discard = getattr(middleware, "discard_rejected_policy_approvals", None) | ||
| if callable(discard): | ||
| discard(responses, invocation_session) |
There was a problem hiding this comment.
Would it make sense to declare a default no-op rejection hook on FunctionMiddleware instead of discovering discard_rejected_policy_approvals by name? As written, middleware authors have to know the exact private method name, argument order, and synchronous-callback requirement, while typos or an async implementation fail silently. An explicit on_function_approval_response(response, session) method would keep the lifecycle contract visible and type-checked while letting PolicyEnforcementFunctionMiddleware own the cleanup behavior.
There was a problem hiding this comment.
Great suggestion — an explicit, documented hook is much safer than duck-typed discovery (typos and async mismatches currently fail silently, exactly as you note).
It became moot for this PR: while rebasing onto current main I found that #8142 already landed the approval lifecycle work, with lifecycle observation intentionally kept private to the FIDES middleware and rejection/cancellation notification wired through the authenticated AG-UI paths instead of a core pipeline hook.
If a core-level on_function_approval_response(response, session) hook on FunctionMiddleware is ever desirable — so any host, not just AG-UI, can release pending bindings eagerly — I'd be glad to raise that as a separate issue/PR building on your sketch. Closing this PR as superseded by #8142. Thanks for the review!
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Closing as superseded by #8142, which merged the issue fix on the session-scoped, occurrence-aware FIDES approval model and includes FIFO bounds, TTL expiry, authenticated core and AG-UI non-grant cleanup, and safe replacement-approval recovery. |
Fixes #7890 (supersedes closed #7893, rebased onto current upstream/main past the FIDES per-session isolation refactor #8138).
Motivation
PolicyEnforcementFunctionMiddleware retains pending policy-approval bindings until consumed. With approval_on_violation=True, abandoned/rejected/never-returned approvals accumulate without bound on long-lived middleware instances.
Changes
Tests (168 passed in test_security.py)