fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols#609
Closed
fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols#609
Conversation
…tStore Protocols Closes adcp-client#1631. ``PlatformHandler.sync_accounts`` and ``PlatformHandler.list_accounts`` were inherited as ``_not_supported`` stubs from ``ADCPHandler`` — every adopter that implements :class:`AccountStoreUpsert` / :class:`AccountStoreList` on their platform's ``accounts`` store was invisible on the wire, returning ``OPERATION_NOT_SUPPORTED`` for both tools regardless of what they declared. The dispatch path now mirrors the documented design: * ``PlatformHandler.sync_accounts(params, ctx)`` → ``platform.accounts.upsert(params, ctx=ResolveContext(...))`` * ``PlatformHandler.list_accounts(params, ctx)`` → ``platform.accounts.list(params, ctx=ResolveContext(...))`` The ``ResolveContext`` carries the caller's :class:`AuthInfo` and resolved :class:`BuyerAgent` (when a registry is wired) — same threading :meth:`AccountStore.resolve` already uses, so adopter impls that implement principal-keyed gates (e.g. spec ``BILLING_NOT_PERMITTED_FOR_AGENT``) read the principal off the same context. Account methods aren't part of any per-specialism Protocol — they live on the ``AccountStore`` Protocol surface per ``LazyPlatformRouter._ACCOUNT_STORE_METHODS``. Both ``upsert`` and ``list`` are OPTIONAL on the store; the new shims surface ``OPERATION_NOT_SUPPORTED`` (via ``_not_supported``) when the store doesn't expose them, rather than ``AttributeError``. Wire advertisement adds a new ``_ACCOUNT_ADVERTISED_TOOLS`` set, unioned into every ``sales-*`` entry of ``SPECIALISM_TO_ADVERTISED_TOOLS`` (every sales-* claim already implies an account roster — buyers must reach it). The override- detection filter still drops the tools when the platform's store doesn't implement the corresponding Protocol method. Tests: * ``test_advertised_tools_covers_every_specialism_wire_tool`` extended with the two new tools. * New positive tests: ``sync_accounts`` and ``list_accounts`` route through to ``platform.accounts.upsert`` / ``.list`` with the ``ResolveContext.tool_name`` set correctly. * New negative tests: stores that don't implement the optional Protocols surface ``OPERATION_NOT_SUPPORTED`` via the framework's ``NotImplementedResponse`` — distinct from ``AttributeError``. Storyboard impact for downstream adopters: scenarios in ``media_buy_seller/refine_products/setup`` (which require either ``sync_accounts`` or ``list_accounts``) graduate from skipped to graded the moment the adopter wires their store's optional Protocol methods.
Contributor
Author
|
Superseded by #610 — same core wiring, but strictly better: per-instance filter that drops the tool when the store doesn't expose the optional Protocol (with logger.info breadcrumb), wire-envelope auto-wrapping with credential scrubbing, filter-dict projection for list, plus authoring docs and credential-smuggling test coverage. Closing in favor. |
Contributor
Author
|
Acknowledged — noting the closure in favor of #610. No action needed here. Triaged by Claude Code. Session: https://claude.ai/code/session_0146e4vWbsnkKQ7WqUvLw3ue Generated by Claude Code |
bokelley
added a commit
that referenced
this pull request
May 10, 2026
…tStore Protocols (#610) * fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols PlatformHandler.sync_accounts and PlatformHandler.list_accounts were inherited as _not_supported stubs from ADCPHandler — every adopter implementing AccountStoreUpsert / AccountStoreList saw their account roster invisible on the wire regardless of what the store declared. This wires real shims that route through platform.accounts.upsert / .list with a ResolveContext carrying the caller's verified AuthInfo and registry-resolved BuyerAgent (same threading AccountStore.resolve already uses, so adopter impls implementing principal-keyed gates — e.g. spec BILLING_NOT_PERMITTED_FOR_AGENT — read the principal off the canonical context). Wire advertisement: _ACCOUNT_ADVERTISED_TOOLS = {"sync_accounts", "list_accounts"} unioned into every sales-* entry of SPECIALISM_TO_ADVERTISED_TOOLS. The per-instance filter at advertised_tools_for_instance drops the tool when the store doesn't expose the corresponding optional Protocol method (with a one-line logger.info breadcrumb on first drop), so adopters who haven't wired the optional Protocols don't over-advertise. Defense-in-depth: shims project results through strip_credentials_from_wire_result on the final envelope so loose dicts and Pydantic extra='allow' rows can't smuggle governance_agents[i].authentication or billing_entity.bank past the typed projections. Closes #609. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(examples): sales-proposal-mode seller emits spec-shape sync_accounts rows The example's _SingleTenantAccounts.upsert returned rows shaped as ``{ref, account, operation}`` — a custom layout that worked while the framework's sync_accounts dispatch was a no_supported stub but trips schema output validation now that the dispatch actually invokes accounts.upsert. Reshape each row to the wire schema per schemas/cache/account/sync-accounts-response.json: ``{account_id, brand, operator, action, status, ...}``. The framework wraps the list as ``{"accounts": [...]}``. Verified end-to-end against the running seller: storyboard runner's sync_accounts step now passes output validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(decisioning): unblock sync_accounts/list_accounts bootstrap on explicit-mode stores The shims called ``await self._resolve_account(None, tool_ctx)`` purely to side-effect-stash the registry-resolved buyer-agent on ``ctx.metadata``. For ``AccountStore.resolution == "explicit"`` impls without a principal/subdomain fallback, that no-ref resolve raised ``ACCOUNT_NOT_FOUND`` — deadlocking the bootstrap path that uses ``sync_accounts`` to populate the store in the first place. Extract the side-effect into a ``_prime_auth_context`` helper that runs the buyer-agent registry gate (suspended/blocked rejection still fires) WITHOUT calling ``AccountStore.resolve``. Use it in both account-roster shims; ``_resolve_account`` delegates to it for the buyer-agent stash. Also: - Warn on unexpected ``upsert`` / ``list`` return shapes (None, tuple, bare string) so adopter contract violations aren't silent — the credential scrubber relies on dict-or-list shape. - Document that the per-tenant handler created by ``LazyPlatformRouter`` emits the dropped-tool boot log per (tenant, tool) — intentional, since stores can be wired differently per tenant. - Doc nit: ``AccountStoreList.list`` example shows binding ``filter`` to ``filter_`` to avoid shadowing the builtin in the method body. Test additions: - explicit-mode bootstrap regression: shim must NOT call ``ExplicitAccounts.resolve(None, ...)`` first. - unexpected-shape return triggers the new ``logger.warning``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
bokelley
added a commit
to bokelley/salesagent
that referenced
this pull request
May 10, 2026
…_accounts (#313) * fix(storyboard): unblock pending_creatives, surface status, drop null asset fields, get-by-id ignores default-active gate Knocks out three of the four open AdCP storyboard ``media_buy_seller`` failures the launch-readiness check picks up. - closes #272: ``update_media_buy`` now transitions a buy out of ``pending_creatives`` once at least one ``creative_assignments`` entry is processed, regardless of creative review state — per the spec / storyboard ``pending_creatives_to_start`` flow, the buy state machine is decoupled from creative approval state. The success response also populates the spec-defined ``status`` field via the same ``_compute_status`` the read path uses, so the storyboard's ``status in [pending_start, active]`` assertion fires on the response envelope, not just the next ``get_media_buys`` poll. Best-effort — fetch failures fall back to ``status=None`` rather than break the response. - closes #273 (follow-up): ``get_media_buys`` now skips the default ``active``-only status gate when the buyer explicitly names ``media_buy_ids`` *and* didn't supply an explicit ``status_filter``. An explicit ``status_filter`` still narrows results, preserving the by-id-with-status-filter contract. Storyboard ``inventory_list_targeting/get_after_create`` reads back a freshly-created (``pending_creatives``) buy by id; gating on ``active`` returned ``[]`` and the property_list/collection_list assertions all failed downstream. Mirrors the same change in ``_project_gam_buys`` so projected buys behave the same way. - closes #274: ``list_creatives`` strips null-valued optional fields from each asset value before serialisation. The bundled ``list-creatives-response.json`` schema declares ``ImageAsset.format``/``alt_text``/``provenance`` as ``type: string``/``type: object`` (NOT nullable), so emitting ``format: null`` violates the asset oneOf and the storyboard validator rejects with ``Invalid input`` at ``/creatives/0/assets/<key>``. Pydantic ImageAsset allows null on those fields, but the strict spec is what we have to satisfy on the wire. Plus a test ergonomics nit: ``scripts/storyboard-check.sh`` now honors a ``BETWEEN_PROTOCOLS_HOOK`` env var that runs between MCP and A2A. Storyboard scenarios mutate state (``update_swap_lists``) and reuse idempotency keys per scenario; running both transports against the same DB causes the second protocol's idempotency replay to return the cached create response while reading the post-update DB state, surfacing as bogus ``verify_create_persisted`` failures. Local runs against a Docker stack should pass a SQL-truncate hook; remote runs leave it unset (a no-op). Storyboard result on a fresh ``docker compose up -d`` with the hook: AGENT_URL=http://localhost:8123 AGENT_TOKEN=ci-test-token \ NO_SANDBOX=1 ALLOW_HTTP=1 BETWEEN_PROTOCOLS_HOOK="..." \ ./scripts/storyboard-check.sh → MCP: 31 passed, 0 failed, 28 skipped → A2A: 26 passed, 3 failed, 30 skipped The remaining A2A failures (#275: TERMS_REJECTED / MEDIA_BUY_NOT_FOUND / PACKAGE_NOT_FOUND not surfacing) reproduce a client-side bug in ``@adcp/sdk@6.10.0`` — its A2A response unwrapper rejects any task whose ``status.state != "completed"``, so structured ``adcp_error`` envelopes on failed tasks get dropped before reaching the validator. SDK 6.13+ already fixed this; ``npx -y @adcp/sdk`` defaults to 6.10.0 today because of the npm registry's minimum-package- age suppression. Our server response is spec-compliant; the bug is upstream of the agent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(storyboard-check): aggregate skip causes inline in summary The SDK's always-on summary only reports skip *counts*, not *causes*, so "30 skipped" doesn't tell you whether you're missing one tool or ten. Post-process the --json output and print a "Skip causes" block grouped by reason (with affected scenarios) so launch-readiness signal is actionable without grovelling through the full ComplianceResult. Switches the runner from --format json (human-readable) to --json (structured ComplianceResult) so the post-processor has something to read. Reads from the same /tmp/storyboard-<proto>.json file the script already captures. Filed upstream as adcontextprotocol/adcp-client#1623; this is a local workaround until that lands. Companion issue adcontextprotocol/adcp-client#1624 tracks the related "skip vs. fail" classification gap (missing required-by-spec tools should fail, not skip). Sample output: ── A2A ── Steps: 26 passed, 3 failed, 30 skipped Failing steps: - measurement_terms_rejected/create_media_buy_aggressive_terms ... Skip causes: [14] missing_test_controller Affected: create_media_buy_async, delivery_reporting, ... [ 1] missing_tool: sync_accounts Affected: refine_products Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(accounts): wire sync_accounts/list_accounts to the wire (closes-the-skip) Existing impls in src/core/tools/accounts.py have been complete since PR prebid#1170 but the MCP/A2A wrappers got deleted with the legacy stack (PR #17) and never got ported to the new core/platforms delegate path. adcp 4.5.0's PlatformHandler also ships sync_accounts / list_accounts as ``_not_supported`` stubs — so even an adopter that overrides them on the platform is invisible on the wire. The storyboard's ``refine_products/setup`` skipped against us with ``missing_tool: sync_accounts`` because of this gap. Wire path now: request → SpecDefaultsMiddleware (idempotency_key default) → MCP/A2A tool dispatch → PlatformHandler.sync_accounts ← rebound by polyfill → platform.accounts.upsert(params, ctx) → SalesagentAccountStore.upsert → src.core.tools.accounts._sync_accounts_impl Pieces: - core/platforms/account_polyfill.py — runtime patch of ``PlatformHandler.sync_accounts`` / ``list_accounts`` to delegate to the platform's ``accounts`` store (where the framework's ``AccountStoreUpsert``/``AccountStoreList`` Protocols live; the framework's stub returns ``_not_supported`` instead). Also extends ``_HANDLER_TOOLS["PlatformHandler"]`` and every ``sales-*`` entry of ``SPECIALISM_TO_ADVERTISED_TOOLS`` so the per-instance specialism filter doesn't strip the two tools back out of ``tools/list``. Imported as a side-effect from ``core/main.py`` so the patch applies before ``serve()`` constructs the handler. Drop once the framework wires this natively — file an upstream issue against adcontextprotocol/adcp-client. - core/stores/accounts.py — ``SalesagentAccountStore`` grows ``upsert`` / ``list`` Protocol methods that forward to the existing ``_sync_accounts_impl`` / ``_list_accounts_impl``. Identity comes from the auth chain's ContextVars (same source ``_build_identity`` already uses); ``ResolveContext.agent`` is consulted as a fallback for non- HTTP entry points (lifespan, tests). - core/middleware/spec_defaults.py — ``sync_accounts`` doesn't take an ``account`` field (the request IS the account discovery surface), so splitting the auth-fill defaults into ``_AUTH_FILLED_TOOLS`` (account + idempotency_key) and ``_IDEMPOTENCY_ONLY_TOOLS`` (sync_accounts). - core/platforms/{mock,gam}.py — small comment pointing at the polyfill, plus dropping the now-unused delegate forwarders. Account dispatch flows through ``accounts`` (the AccountStore) per ``LazyPlatformRouter._ACCOUNT_STORE_METHODS``, not the per-platform delegation path. Storyboard impact: Before: MCP 31 passed / 0 failed / 28 skipped with [1] missing_tool: sync_accounts (refine_products) After: MCP 32 passed / 0 failed / 27 skipped (skip-cause cleared) Verified end-to-end against ``@adcp/sdk@6.15.2`` on both transports — ``refine_products/setup`` graduates to graded; ``tools/list`` advertises sync_accounts + list_accounts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(polyfill): point at upstream PR + removal path for the account dispatch monkey-patch The fix landed upstream as adcontextprotocol/adcp-client-python#609. Update the module docstring with the removal path so the next person reading this knows when (and how) to drop the patch — bump adcp to the release containing prebid#609 and delete this module + its import in ``core/main.py``. * chore(accounts): forward-compat upsert/list signatures with adcp-client-python#610 PR prebid#610 supersedes the upstream fix I sent (prebid#609). The post-prebid#610 ``AccountStoreUpsert`` / ``AccountStoreList`` contract is: * ``upsert(refs: list[AccountReference], ctx: ResolveContext)`` * ``list(filter: dict | None, ctx: ResolveContext)`` vs. our polyfill's ``upsert(params: SyncAccountsRequest, ctx)`` / ``list(params: ListAccountsRequest, ctx)``. To make the eventual adcp bump a one-line ``adcp >= X.Y`` change with no code rewrite, ``SalesagentAccountStore.upsert`` / ``.list`` now accept either shape and normalise inside ``_coerce_sync_accounts_payload`` / ``_coerce_list_accounts_payload``: * ``list[AccountReference]`` → ``SyncAccountsRequest(accounts=...)`` with a synthesised idempotency key * ``dict`` filter for list → ``ListAccountsRequest(**filter)`` * Empty filter dict / ``None`` → ``None`` (impl's no-filter path) * Pre-shaped Pydantic / dict requests pass through unchanged (today's polyfill behavior preserved) Polyfill docstring updated to point at prebid#610 instead of the closed prebid#609, and to note that the store-side signatures are already future-compat so nothing else needs to change at bump time except the requirement version. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(deps): bump adcp >= 4.6.1 and drop account dispatch polyfill adcp 4.6.1 ships adcontextprotocol/adcp-client-python#610, which wires ``PlatformHandler.sync_accounts`` / ``list_accounts`` to dispatch through the AccountStore Protocols (``accounts.upsert`` / ``accounts.list``) natively. The salesagent monkey-patch we added in ``core/platforms/account_polyfill.py`` is no longer needed: * delete the polyfill module * drop the load-bearing import in ``core/main.py`` * trim "see polyfill" references in the platform modules and ``core/stores/accounts.py`` The store-side ``upsert`` / ``list`` methods on :class:`SalesagentAccountStore` keep their dual-shape coercion (refs list / filter dict) — that's the framework's contract under prebid#610, no change needed on this side. Verified end-to-end: * Container: ``adcp 4.6.1`` confirmed * Boot log: ``mcp server advertising 12 of 12 tools`` (was 10/10 before the polyfill landed; would still be 10 if the framework filter were dropping these). No polyfill breadcrumb. * Storyboard against ``@adcp/sdk@6.15.2``: MCP: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK A2A: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(docker): LOCKFILE_HASH build arg invalidates install layer on uv.lock change Symptom: ``docker compose build && docker compose up -d`` after a ``uv lock --upgrade-package <foo>`` would silently keep the old version of the bumped package inside the container. Hit this bumping ``adcp 4.5.0 -> 4.6.1`` — only ``docker rmi`` + full rebuild picked it up. The BuildKit cache-mount on ``/cache/uv`` plus the ``COPY pyproject.toml uv.lock`` layer hash short-circuited the install layer even though the lockfile content had changed. Fix: thread the uv.lock content sha256 as a build arg into the ``RUN uv sync --frozen`` step. When the lockfile content changes, the ARG value changes, the layer cache key changes, the install step re-runs against the fresh lockfile. Wrap the dev workflow in two make targets so the next person doesn't need to remember the incantation: make compose-build # builds adcp-server with LOCKFILE_HASH wired in make compose-up # build + force-recreate adcp-server CLAUDE.md adds a breadcrumb pointing at the make target after a dependency bump. The existing ``docker compose up -d`` still works for non-lockfile- changing rebuilds (source code edits) — those paths invalidate the ``COPY . .`` layer in the runtime stage and don't go through the install step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(storyboard): expert-review pass — drop "unassigned" sentinel, harden identity, narrow except, fail-fast on missing LOCKFILE_HASH Bundles the substantive findings from three parallel reviews (code-reviewer / security-reviewer / ad-tech-protocol-expert) on the branch before opening the PR. Protocol (HIGH from ad-tech-protocol-expert): - ``_build_sync_result`` no longer emits the literal ``"unassigned"`` sentinel string for ``account_id``. Per ``sync-accounts-response.json`` 3.0.6+, ``account_id`` is optional on rejected/failed entries — the library type already declares ``str | None`` and ``exclude_none=True`` drops the field on the wire. Buyers could otherwise round-trip ``"unassigned"`` as a real account ID into ``create_media_buy``. Plumbed the real ``account_id`` (or ``existing.account_id`` / ``account_id`` / ``db_acct.account_id``) through the five success call sites that previously relied on the sentinel. - ``_check_domain_validity`` now exempts reserved RFC 2606 TLDs (``.test`` / ``.invalid`` / ``.example`` / ``.localhost``) when ``ADCP_TESTING=true`` — the AdCP storyboards intentionally use ``acmeoutdoor.example`` etc. as test domains, so the ``refine_products/sync_accounts`` setup step would otherwise fail with ``INVALID_DOMAIN`` on every compliance run. Security (Consider from security-reviewer + Should-Fix #1 from code-reviewer): - Drop the ``getattr(ctx.agent, "tenant_id", None)`` fallback in ``SalesagentAccountStore._identity_from_ctx``. Dead code today (``BuyerAgent`` doesn't carry ``tenant_id``) but a non-obvious trust- boundary widening if the framework or a custom registry ever adds that attribute. ``BearerTokenAuthMiddleware`` is the canonical gate. Code quality (Should-Fix from code-reviewer): - ``SalesagentAccountStore._identity_from_ctx``: stamp the actual protocol via ``current_transport.get()`` instead of hard-coding ``"mcp"``. Mirrors ``core/platforms/_delegate.py:_build_identity``. No behavioral change today (no protocol-aware code branches on ``identity.protocol`` for sync_accounts/list_accounts), but closes a future-misroute trap. - ``_coerce_sync_accounts_payload``: synthesised ``idempotency_key`` uses ``uuid.uuid4()`` instead of ``id(payload)``. The Python object address is non-deterministic and reusable as soon as the list is GC'd; under concurrent retries that hit a recycled address the impl could treat distinct calls as duplicates. - ``_update_media_buy_impl`` status-compute ``except`` narrowed from bare ``Exception`` to ``(SQLAlchemyError, ValueError, TypeError, StopIteration)``. Real prod errors (DB I/O, bad date math) and unit-test fixture artefacts (mocked datetimes, exhausted mock side_effects) are caught; ``AttributeError`` / ``KeyError`` / enum drift in ``_compute_status`` now intentionally bubble. Listing (refinement of #274 fix): - ``listing.py`` null-strip switched from "drop every null" to an allowlist of fields the bundled spec schema declares non-nullable (``format``, ``alt_text``, ``provenance``, ``mime_type``, ``container_format``). Future fields that meaningfully use ``null`` as a sentinel won't be silently elided. Docker (Should-Fix #6 from code-reviewer): - ``Dockerfile`` ``LOCKFILE_HASH`` ARG default changed from ``unset`` to ``set-this-build-arg`` and the install layer fails fast when the value is unchanged. Bare ``docker compose build`` callers who skip ``make compose-build`` get a clear error instead of silently reusing a stale install layer. - ``.github/workflows/test.yml`` and ``publish-image.yml`` thread ``LOCKFILE_HASH=$(shasum -a 256 uv.lock | awk '{print $1}')`` so CI doesn't trip the new gate. Other code-reviewer nits: - ``scripts/storyboard-check.sh``: drop unused ``Counter`` import. - ``media_buy_update.py``: clarify in-source why ``_determine_media_buy_status(creatives_approved=True)`` is hard-coded — spec text says ``pending_creatives`` clears on attachment, not on review approval. Tracked separately as a follow-up to align ``_determine_media_buy_status`` for the create-path (filed as #296). Storyboard verified against ``@adcp/sdk@6.15.2``: MCP: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK A2A: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK Upstream issues filed: - adcontextprotocol/adcp-client-python#622 (nullable asset fields) - adcontextprotocol/adcp-client-python#623 (optional ``account`` in typed dispatcher) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(docker): thread LOCKFILE_HASH into db-init build args too The Dockerfile's fail-fast on missing LOCKFILE_HASH was hitting db-init's build because docker-compose.yml only wired the build arg under adcp-server.build.args. Both services build from the same Dockerfile, so both need the arg threaded — without it db-init exits 1 inside the install step. Caught by CI on PR #313 (Quickstart Docker Compose Test). Also verified storyboard pass on @adcp/sdk@6.17.0: MCP: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK A2A: 32 passed, 0 failed, 27 skipped — STORYBOARD-OK Note: 6.17.0 ships native skip-cause aggregation in the always-on summary (per adcp-client#1623 feature request). The local post- processor in scripts/storyboard-check.sh becomes a no-op-equivalent on 6.17+ but doesn't conflict — it parses the same data from --json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ci): unbreak Lint + E2E after the LOCKFILE_HASH gate Three CI failures from PR #313's previous push, all caused by the new LOCKFILE_HASH fail-fast in the Dockerfile catching paths I missed: * **Lint & Type Check**: ``src/services/delivery_webhook_scheduler.py:381`` carried a ``# type: ignore[arg-type]`` for an adcp 4.5.0 type that 4.6.1 loosened. mypy now flags it as ``[unused-ignore]``. Drop the comment. * **E2E Tests**: ``tests/e2e/conftest.py`` shells out to ``docker-compose -f docker-compose.e2e.yml build`` directly with no LOCKFILE_HASH, hitting the fail-fast at every E2E suite startup. Compute the hash on the runner (``hashlib.sha256(uv.lock)``) and thread it into the subprocess env. * **docker-compose.e2e.yml**: didn't have the build-args block. Add it so the ARG actually flows from compose env to the Dockerfile build. * **Integration (media-buy)**: cascading "Database is unhealthy" circuit-breaker errors mid-run — pre-existing flaky pattern unrelated to this branch (passed on most runs in the same job, then failed on the simulator-restart teardown). Not addressed here; will retry CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(e2e): UnboundLocalError on conftest's LOCKFILE_HASH path Last commit's nested ``from pathlib import Path`` inside the ``docker_services_e2e`` fixture shadowed the module-level ``Path`` binding for the entire function (Python scoping rule: any name bound in a function body is treated as local everywhere in that body, even before the binding line). The earlier ``Path(".env")`` at line 114 then raised ``UnboundLocalError`` and every E2E test failed at fixture setup. Move ``hashlib`` import to module level alongside the existing ``Path`` import so the LOCKFILE_HASH branch uses the unshadowed bindings. Caught by CI on PR #313 (E2E Tests, fail at fixture setup). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- 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
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.
Problem
PlatformHandler.sync_accountsandPlatformHandler.list_accountsare inherited as_not_supportedstubs fromADCPHandler. Every adopter that implementsAccountStoreUpsert/AccountStoreListon their platform'saccountsstore is invisible on the wire — both tools returnOPERATION_NOT_SUPPORTEDregardless of what the store declares.Compared to e.g.
provide_performance_feedbackin the same handler, which properly threads through_invoke_platform_methodtoplatform.provide_performance_feedback(params, ctx). The account dispatchers were never given the equivalent treatment.Tracked at adcontextprotocol/adcp-client#1631.
Fix
Wire real shims that route through the documented design —
AccountStore.upsert/.list, NOTplatform.<method>, sinceLazyPlatformRouter._ACCOUNT_STORE_METHODSalready excludes account ops from per-tenant delegation:The
ResolveContextcarries the caller'sAuthInfoand resolvedBuyerAgent— same threadingAccountStore.resolvealready uses, so adopter impls implementing principal-keyed gates (e.g. specBILLING_NOT_PERMITTED_FOR_AGENT) read the principal off the canonical context.Both
upsertandlistare OPTIONAL on the AccountStore. The shims surfaceOPERATION_NOT_SUPPORTED(via_not_supported) when the store doesn't expose them — distinct fromAttributeError, which is what an unguardedgetattr().()would produce.Wire advertisement
Adds
_ACCOUNT_ADVERTISED_TOOLS = {"sync_accounts", "list_accounts"}and unions it into everysales-*entry ofSPECIALISM_TO_ADVERTISED_TOOLS(every sales-* claim implies an account roster — per spec, buyers must be able to reach it). The override-detection filter still drops the tools when the platform's store doesn't expose the corresponding Protocol method, so adopters who don't yet implement them aren't suddenly over-advertising.Tests
test_advertised_tools_covers_every_specialism_wire_tool— extended with the two new tools.sync_accounts/list_accountsroute through toplatform.accounts.upsert/.listwithResolveContext.tool_nameset correctly.OPERATION_NOT_SUPPORTEDviaNotImplementedResponse— distinct fromAttributeErrorfrom an unguardedgetattrchain.877 tests pass locally (
pytest tests/ -k "decisioning or account or handler or platform").Storyboard impact
Downstream sellers running the AdCP storyboard
media_buy_seller/refine_products/setupagainst an agent that implements the optional store Protocols will see the scenario graduate fromskipped (missing_tool)to graded the moment they pull this in. Verified end-to-end against bokelley/salesagent@4e62024c —MCP 31→32 passed, 28→27 skipped.Side note
Tracked salesagent monkey-patch that this PR replaces: https://github.com/bokelley/salesagent/blob/main/core/platforms/account_polyfill.py — happy to drop the polyfill once this lands.