feat(server): pluggable PushNotificationConfigStore on A2A (#225)#232
Merged
feat(server): pluggable PushNotificationConfigStore on A2A (#225)#232
Conversation
Second of three A2A hooks salesagent needs to drop their 2,288-LOC custom A2A server. #224 shipped TaskStore; this adds push-notif config persistence. #226 (per-skill middleware) closes out the set. API - create_a2a_server(..., push_config_store=...) threads the store into DefaultRequestHandler(push_config_store=...). - serve(..., transport="a2a", push_config_store=...) surfaces the kwarg end-to-end. - Default is None — matches a2a-sdk's "push-notif disabled" posture. Sellers opt in explicitly, which is the right default: push-notif config endpoints vanish until a store is wired, rather than silently swallowing subscriptions in a non-durable buffer. Reference impl — SqlitePushNotificationConfigStore - Extends examples/a2a_db_tasks.py so the durable-A2A-agent reference stays in one file. - Schema: (scope, task_id, config_id) composite PK. Configs serialised as JSON. - Scoping via a module-level ContextVar (_current_push_config_scope) the seller's auth middleware populates per request. a2a-sdk's PushNotificationConfigStore ABC doesn't pass a ServerCallContext to its methods (unlike TaskStore), so out-of-band scoping is the only available option — documented loudly in the docstring and handler-authoring guide. - Same chmod 0o600 / rollback-on-exception hardening as SqliteTaskStore. Tests — tests/test_a2a_server.py (24 passing, +3 from this PR) - test_create_a2a_server_omits_push_config_store_by_default asserts _push_config_store is None (the a2a-sdk semantic). - test_create_a2a_server_accepts_custom_push_config_store: custom store threads through. - test_sqlite_push_config_store_isolates_scopes_by_contextvar loads the reference impl via importlib and asserts tenant B can't see or delete tenant A's push-notif registration. Pins the security claim in the example's docstring so a regression fails the example, not the docs. Docs — docs/handler-authoring.md - New "Durable push-notification config storage" subsection with the SqlitePushNotificationConfigStore recipe. - Scoping-caveat call-out explains the ABC's missing-context gap and why ContextVar is the compensating control. - Known-gaps list shrinks: #225 removed, only #226 left before A2A reaches MCP parity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on, loud anonymous fallback, SSRF/secret-storage docs Addresses code-reviewer + security-reviewer findings on PR #232. CODE — delete misleading inline comment + branching - src/adcp/server/a2a_server.py: the "only forward when explicitly provided" branch was premised on claims that DefaultRequestHandler defaults push_config_store to an in-memory store and that passing None disables push-notifs in some a2a-sdk versions. Neither claim is true — DefaultRequestHandler stores push_config_store verbatim and None == omitted. Pass unconditionally, delete the dead branch and the wrong comment. EXAMPLE — SqlitePushNotificationConfigStore hardening - scope_provider injection: takes Callable[[], str | None] in __init__ so sellers wire their own auth integration without reaching into the module-level `_current_push_config_scope` ContextVar. Default provider reads the existing ContextVar — current adopters unbroken. - Anonymous-scope UserWarning: emitted once per store instance the first time scope_provider returns None. Silent fall-through was the multi-tenant footgun security review flagged — operators shipping the example to prod without wiring auth got one giant shared webhook pool with no signal. - config_id collision fix: two clients registering on the same task without an explicit id used to collide on the PK via INSERT OR REPLACE. Now synthesises a UUID per call. DOCS — threats the reference example teaches - examples/a2a_db_tasks.py module docstring: new "Security model — push-notification config store adds two threats tenant-scoping alone does NOT address" covering SSRF on webhook URLs and plaintext storage of auth secrets. - SqlitePushNotificationConfigStore docstring: explicit callout for the ContextVar-across-background-asyncio.Task fragility on the push-notif sender path. - docs/handler-authoring.md "Durable push-notification config storage" section expands from one paragraph to three numbered MUSTs: (1) URL allowlist, (2) secret-at-rest handling, (3) per-principal scoping. Parallel structure to the TaskStore MUSTs above. Operator-facing failure modes subsection explains why the UserWarning is a P0 signal. TESTS — tests/test_a2a_server.py (27 passing, +3 this round) - test_custom_push_config_store_receives_sets_from_handler: behavioral test mirroring #230's TaskStore analog. Drives on_set_task_push_notification_config through the extracted handler and asserts the recording store's .sets grew. Defends against a2a-sdk renaming `_push_config_store`. - test_sqlite_push_config_store_warns_once_on_anonymous_scope: locks the warning contract — fires exactly once per store instance. - test_sqlite_push_config_store_synthesises_config_id_when_omitted: two sets on same task without explicit id produce two rows. Follow-ups (not blocking this PR): URL allowlist helper shipped from adcp.server; field-level envelope encryption for webhook secrets; BasePushNotificationSender subclass that re-sets the ContextVar on the background sender path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
Apr 20, 2026
Code reviewer: - _accepts_context_kwarg now rejects positional-only `context`. A method like `def fn(self, context, /, ...)` would previously register as opted-in but raise TypeError when the dispatcher passes `context=ctx` by keyword. - Documented the functools.wraps caveat — inspect.signature follows __wrapped__, which is the intended contract but worth noting. - Added tests: positional-only context, @functools.wraps-preserving decorator (both legacy and modern signatures), inherited override from an intermediate base class via mixin. Security reviewer (L1): - TestHeaderMiddleware docs example now resets the ContextVar token in finally. Without reset(token) the set value leaks into the next request reusing the asyncio task — same shape as the PR #232 cross- tenant idempotency scoping issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #225 (Phase 2 PR-O). Second of three A2A hooks salesagent needs to drop their 2,288-LOC custom A2A server. With #224 (TaskStore) landed and #226 (per-skill middleware) next, A2A adoption reaches parity with MCP for production agents.
Test plan
Follow-ups
🤖 Generated with Claude Code