fix(handlers): preserve full SyncAccountsRequest in sync_accounts store dispatch#797
Closed
bokelley wants to merge 2 commits into
Closed
fix(handlers): preserve full SyncAccountsRequest in sync_accounts store dispatch#797bokelley wants to merge 2 commits into
bokelley wants to merge 2 commits into
Conversation
PlatformHandler.sync_accounts was extracting only params.accounts before calling AccountStore.upsert, silently dropping push_notification_config (webhook URL registration), delete_missing, dry_run, and all other request-envelope fields. Adds AccountStoreUpsert.sync_accounts(params, ctx) as the preferred dispatch surface — receives the full SyncAccountsRequest so the store can persist push_notification_config and honor all envelope flags. Falls back to the existing upsert(refs, ctx) for backward compatibility. Also updates the capability-advertisement gate and platform_router denylist to recognise the new method. Fixes #794
Follow-up to previous commit: split AccountStoreSyncAccounts off AccountStoreUpsert as a separate Protocol so existing isinstance checks against AccountStoreUpsert continue to work for upsert-only stores. Also fixes stale warning message in _project_sync_accounts and exports AccountStoreSyncAccounts from adcp.decisioning. Fixes #794
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| carries the raw transport-level credential. For billing gates | ||
| the registry-resolved identity is canonical. | ||
| """ | ||
| ... |
Contributor
Author
Contributor
Author
|
Acknowledged — closing this draft as superseded by #796. No further action needed on this PR. Triaged by Claude Code. Session: https://claude.ai/code/session_0156fBdTLABmzN1TvWCEiKLj Generated by Claude Code |
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
2 participants
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.
Closes #794
Summary
PlatformHandler.sync_accountswas extracting onlyparams.accountsbefore callingAccountStore.upsert, silently droppingpush_notification_config(the buyer's webhook URL for async account-status notifications),delete_missing,dry_run, and all otherSyncAccountsRequestenvelope fields. A buyer callingsync_accountswith a webhook URL to register would get a success response, but the webhook was never persisted.Fix: Adds a new
AccountStoreSyncAccountsProtocol with async_accounts(params: SyncAccountsRequest, ctx)method that receives the full wire request. The dispatch shim checks forsync_accountsfirst (new path) and falls back to the existingupsert(refs, ctx)(legacy path) when absent — no changes required from existingupsertimplementors.AccountStoreSyncAccountsis a separate Protocol fromAccountStoreUpsert(not added to it) so thatisinstance(store, AccountStoreUpsert)continues to returnTruefor existingupsert-only stores.What was tested
pytest tests/test_decisioning_handler_shims.py tests/test_platform_router.py— 92 passedmypy src/adcp/— 811 files, no issuesruff check src/adcp/decisioning/— cleanNew tests:
test_sync_accounts_full_request_routes_to_sync_accounts_method— assertsparams is reqandpush_notification_config.urlsurvives to the store impltest_sync_accounts_prefers_sync_accounts_over_upsert— verifies dispatch priority when both methods are presenttest_advertised_tools_for_instance_includes_sync_accounts_with_sync_accounts_only_store— regression guard for the capability-advertisement gate (previously checked only forupsert)Pre-PR review
isinstanceconcern (addressed by split-Protocol design) and stale warning message (fixed)sync_accounts(mirrors wire verb, clearest for adopters and coding agents)Nits surfaced (not fixed in this PR):
_call_with_optional_ctxdocstring "Used by" list doesn't mentionAccountStoreSyncAccounts.sync_accountselse: return self._not_supported(...)branch is unreachable (logic is correct; the guard above already covers it)_make_resolve_contextdocstring should mentionsync_accountsalongsideupsert/listSession: https://claude.ai/code/session_01LFsNBjCJ4LztRMAaJvAjA4
Generated by Claude Code