feat(beta): add ToolPolicyMiddleware for tool access control - #2533
feat(beta): add ToolPolicyMiddleware for tool access control#2533amabito wants to merge 7 commits into
Conversation
|
@amabito hi! Thank you for the middlewares contribution, but I don't see a sense of the current one
|
|
Hey @Lancetnik, thanks for the pushback -- both points are worth addressing directly. "Just don't pass the tool to context"Static exclusion at registration time works when the policy is fixed for the session, and for that case you're right that a middleware adds nothing. Where it stops working:
If even the audit case feels out of scope for AG2's middleware layer and you'd rather see this as a dedicated capability or event hook rather than a middleware, I'm open to that -- say the word and I'll repackage it. But I don't think "don't pass the tool" covers these cases. Counters and the leak concernThe counters at L90-L91 are instance attributes on I'd rather just drop the counters. They were a convenience for testing and a half-hearted observability hook, and neither justifies stateful middleware. Concrete change:
Landing it in the next revision alongside the constructor simplification from #2531 / #2532. |
Address review feedback from @Lancetnik on ag2ai#2533: - Flatten constructor API: pass blocked_tools/allowed_tools/on_blocked directly instead of requiring callers to build a ToolPolicyConfig - Drop stateful counters (_total_tool_calls, _total_blocked, _lock) and their accessor properties -- the middleware is now stateless - Add optional on_blocked callback so callers can wire their own observability (metrics, audit log) without the library retaining any per-factory state The ToolPolicyConfig dataclass is kept as an internal struct for immutability (tuple freezing in __post_init__).
Fair, it seems the commonest case for such feature - but I don't see a public API covers it. Can we introduce a some? |
There was a problem hiding this comment.
I prefer end-to-end test here using public API (or at least do not use non-public objects). Such tests are not so implementation-tied and not so fragile. Also, they cover real usecases and could be used as a some kind of documentation
Please, try to implement each test using TestClient / TracingClient. Reference: https://github.com/ag2ai/ag2/blob/main/test/beta/middleware/test_tool_execution.py
Address review feedback from @Lancetnik on ag2ai#2533: - Flatten constructor API: pass blocked_tools/allowed_tools/on_blocked directly instead of requiring callers to build a ToolPolicyConfig - Drop stateful counters (_total_tool_calls, _total_blocked, _lock) and their accessor properties -- the middleware is now stateless - Add optional on_blocked callback so callers can wire their own observability (metrics, audit log) without the library retaining any per-factory state The ToolPolicyConfig dataclass is kept as an internal struct for immutability (tuple freezing in __post_init__).
Address @Lancetnik follow-up on ag2ai#2533: the dynamic-policy justification needs a public API to be useful. Adds: - ToolPolicyMiddleware.update_policy(config: ToolPolicyConfig) Atomically replaces the active policy. Subsequent tool calls see the new config; per-invocation instances already inside on_tool_execution keep their captured snapshot, so in-flight executions are never torn across a config change. - ToolPolicyMiddleware.config property Read-only access to the active ToolPolicyConfig. Intended hooks: rate-limit kill switches, per-tenant rule updates from an ops dashboard, role changes during a long-running session -- all without rebuilding the agent. Thread-safety: _policy is assigned last so readers in __call__ never observe a state where _config is new but _policy is stale (Python attribute assignment is atomic for single references). Tests: 19/19 pass, covering update-then-permit, in-flight-snapshot isolation, restriction clearing, and the config property.
3253b29 to
5984625
Compare
|
@Lancetnik Thanks -- fair point, "it's in the shape" is not a public API. Added one in 5984625.
|
|
Sorry for the rapid-fire -- ran the middleware through another review pass and wanted to send the consolidated state. Since the last update:
25 tests, all passing. Pre-commit clean. |
Address review feedback from @Lancetnik on ag2ai#2533: - Flatten constructor API: pass blocked_tools/allowed_tools/on_blocked directly instead of requiring callers to build a ToolPolicyConfig - Drop stateful counters (_total_tool_calls, _total_blocked, _lock) and their accessor properties -- the middleware is now stateless - Add optional on_blocked callback so callers can wire their own observability (metrics, audit log) without the library retaining any per-factory state The ToolPolicyConfig dataclass is kept as an internal struct for immutability (tuple freezing in __post_init__).
Address @Lancetnik follow-up on ag2ai#2533: the dynamic-policy justification needs a public API to be useful. Adds: - ToolPolicyMiddleware.update_policy(config: ToolPolicyConfig) Atomically replaces the active policy. Subsequent tool calls see the new config; per-invocation instances already inside on_tool_execution keep their captured snapshot, so in-flight executions are never torn across a config change. - ToolPolicyMiddleware.config property Read-only access to the active ToolPolicyConfig. Intended hooks: rate-limit kill switches, per-tenant rule updates from an ops dashboard, role changes during a long-running session -- all without rebuilding the agent. Thread-safety: _policy is assigned last so readers in __call__ never observe a state where _config is new but _policy is stale (Python attribute assignment is atomic for single references). Tests: 19/19 pass, covering update-then-permit, in-flight-snapshot isolation, restriction clearing, and the config property.
5043ecc to
2cd9dd9
Compare
|
CI is failing due to API staleness. ToolResult(*inputs: SendableMessage | Input, parts: Iterable[...] = (), final: bool = False, metadata: dict | None = None)Any There are also failures in unrelated beta tests ( The |
Single-responsibility middleware that blocks disallowed tool calls based on configurable allow/block lists. Extracted from GovernanceMiddleware to align with beta v2 composable middleware design. - ToolPolicyConfig: blocked_tools, allowed_tools - blocked_tools overrides allowed_tools - allowed_tools=None means no restriction, [] means deny all - Immutable config (lists frozen to tuples)
Address review feedback from @Lancetnik on ag2ai#2533: - Flatten constructor API: pass blocked_tools/allowed_tools/on_blocked directly instead of requiring callers to build a ToolPolicyConfig - Drop stateful counters (_total_tool_calls, _total_blocked, _lock) and their accessor properties -- the middleware is now stateless - Add optional on_blocked callback so callers can wire their own observability (metrics, audit log) without the library retaining any per-factory state The ToolPolicyConfig dataclass is kept as an internal struct for immutability (tuple freezing in __post_init__).
Address @Lancetnik follow-up on ag2ai#2533: the dynamic-policy justification needs a public API to be useful. Adds: - ToolPolicyMiddleware.update_policy(config: ToolPolicyConfig) Atomically replaces the active policy. Subsequent tool calls see the new config; per-invocation instances already inside on_tool_execution keep their captured snapshot, so in-flight executions are never torn across a config change. - ToolPolicyMiddleware.config property Read-only access to the active ToolPolicyConfig. Intended hooks: rate-limit kill switches, per-tenant rule updates from an ops dashboard, role changes during a long-running session -- all without rebuilding the agent. Thread-safety: _policy is assigned last so readers in __call__ never observe a state where _config is new but _policy is stale (Python attribute assignment is atomic for single references). Tests: 19/19 pass, covering update-then-permit, in-flight-snapshot isolation, restriction clearing, and the config property.
…eware at top level TP-1 (update_policy atomicity): The previous docstring said "Assign _policy last so readers in __call__ never observe a state where _config is new but _policy is stale", but the assignment order was _config then _policy -- which is exactly right. The comment was misleading. Clarify: _config is assigned first so the config property never lags behind the active policy; under CPython's GIL each assignment is atomic at the bytecode level, so __call__ readers cannot observe a mismatched pair. TP-2 (export gap): autogen.beta.middleware top-level __init__ did not re-export ToolPolicyMiddleware or ToolPolicyConfig. Add imports and __all__ entries so callers can reach them from the canonical import path. Add test_tool_policy_importable_from_middleware_top_level regression test.
…ersarial tests - Wrap on_blocked callback in contextlib.suppress(Exception) so a failing audit observer does not prevent the ToolErrorEvent from being returned to the caller. - Add test_on_blocked_callback_exception_does_not_prevent_denial. - Add test_concurrent_update_policy_consistent_decisions: 20 parallel coroutines interleaved with a background policy-writer; asserts every result is a valid ToolErrorEvent or ToolResultEvent. - Add test_empty_tool_name_denied_by_allowlist. - Add test_empty_allowlist_blocks_all_via_public_interface covering ToolPolicyMiddleware(allowed_tools=[]) through the public interface.
ToolResult no longer accepts 'content' keyword; the constructor is now: ToolResult(*inputs, parts=(), final=False, metadata=None) _make_tool_error simplified to one line via ToolErrorEvent.from_call: return ToolErrorEvent.from_call(event, PermissionError(reason)) Tests updated: .content assertions replaced with isinstance(error_event.error, PermissionError) + str(error_event.error) checks, since ToolErrorEvent no longer carries a .content property. Removed unused ToolResult import from tool_policy.py. Addresses marklysze's CI-staleness review on ag2ai#2533.
c3ecc79 to
13eeb3e
Compare
|
@marklysze Thanks for catching this. Pushed the API fix in
CI should be green this round. Happy to address any remaining concerns on the middleware design. |
|
@amabito sorry for waiting so long... There were too many things we changed and contributed our side last month. Now we have a stable enough API and I'll be happy to revisit your PRs next week |
|
Good separation of concerns — pulling tool policy out of GovernanceMiddleware makes sense. A couple of production observations: Policy ordering mattersIf both # Recommended order: cheap checks first, expensive checks last
interceptors = [
ToolPolicyMiddleware(blocked_tools=["rm", "drop_db"]), # O(1) regex
RateLimiter(max_per_minute=10), # O(1) counter
CostEstimator(max_budget=0.50), # O(n) estimation
AuditLogger(), # O(n) write
]Fail-fast on tool policy before spending compute on cost estimation.
|
|
Glad this was useful. If anyone is looking for help designing a production-grade interceptor chain with cost budgeting, we do lightweight architecture reviews (from $2) — details at https://chengduman.github.io/zhongpu-consulting-advisory/services.html |
|
@amabito, we have unfortunately been unable to review your PR just yet. We are hoping to get V1 over the line in the next couple of weeks and once that's done, we'll jump on it. Thank you for your patience. |
|
Hi @amabito — this PR now has merge conflicts with Please rebase onto the current ⏳ If there's no update within 30 days, this PR will be marked stale and closed 7 days after that. |
genisis0x
left a comment
There was a problem hiding this comment.
Clean design overall. A few things I like: blocked_tools taking precedence over allowed_tools is the safe default and it's documented; keeping the None (no allowlist) vs [] (allow nothing) distinction through __post_init__ is exactly right and easy to get wrong; freezing to frozenset and creating a per-invocation instance from a policy snapshot means an update_policy mid-flight can't tear an in-progress check. The update_policy docstring is honest about the two non-atomic STORE_ATTRs, and since __call__ only ever reads self._policy (one atomic attribute read) enforcement is always consistent with a single policy snapshot — the only visible skew is the config property vs enforcement, which is cosmetic. Good.
Three things worth pinning down before this is relied on as a security control, since for an access gate the interesting cases are the bypasses:
-
Matching is exact string membership (
tool_name in self._blocked). Isevent.nameguaranteed to be the canonical registered tool name at the pointon_tool_executionruns? If a tool can ever reach here under a differently-cased or whitespace-variant name (or an alias) than what an operator put inblocked_tools, an exact-match blocklist silently lets it through. Forallowed_toolsthe failure is safe (unknown variant is denied), but forblocked_toolsit's the dangerous direction. If names are already normalized upstream this is moot — worth a one-line confirmation, and if there's any doubt, normalizing both sides (and matching on the canonical name) closes it. -
Coverage: is
on_tool_executioninvoked for every path a tool can execute through — direct calls, handoffs/sub-agent tools, any nested or framework-internal execution? A policy that wraps most invocations but not all isn't a policy an operator can trust for "delete_all is blocked." A test that asserts a blocked tool stays blocked when reached via a sub-agent/handoff (not just a direct call) would document that the gate has no side doors. -
The
on_blockedcallback is wrapped incontextlib.suppress(Exception)— right instinct that enforcement must not depend on the observer. Buton_blockedis the audit hook, and silently swallowing its failure means you can block a dangerous call and lose the record that you did. For a security audit sink that's the one failure you'd want to see. Consider logging the suppressed exception (still non-fatal) so a broken audit path is visible rather than silent.
None of these are blockers on the mechanism — the allow/deny logic itself is correct. They're the questions that decide whether it holds up as a security boundary vs a best-effort filter.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 66 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Why are these changes needed?
Pulls the tool allow/block list logic out of GovernanceMiddleware (#2501,
closed) into its own middleware. Sits on
on_tool_execution, checks thetool name against blocked and allowed lists, returns ToolErrorEvent if denied.
blocked_tools overrides allowed_tools. allowed_tools=None means no
restriction; allowed_tools=[] means deny all. Config is frozen after
construction.
Related issue number
Replaces the tool policy portion of #2501 (closed).
Related: #2531 (BudgetMiddleware), #2532 (CircuitBreakerMiddleware).
Checks