Skip to content

fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols#609

Closed
bokelley wants to merge 1 commit intomainfrom
fix/account-dispatch
Closed

fix(decisioning): wire sync_accounts/list_accounts dispatch to AccountStore Protocols#609
bokelley wants to merge 1 commit intomainfrom
fix/account-dispatch

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 9, 2026

Problem

PlatformHandler.sync_accounts and PlatformHandler.list_accounts are inherited as _not_supported stubs from ADCPHandler. Every adopter that implements AccountStoreUpsert / AccountStoreList on their platform's accounts store is invisible on the wire — both tools return OPERATION_NOT_SUPPORTED regardless of what the store declares.

Compared to e.g. provide_performance_feedback in the same handler, which properly threads through _invoke_platform_method to platform.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, NOT platform.<method>, since LazyPlatformRouter._ACCOUNT_STORE_METHODS already excludes account ops from per-tenant delegation:

async def sync_accounts(self, params, context=None):
    upsert = getattr(self._platform.accounts, "upsert", None)
    if upsert is None:
        return self._not_supported("sync_accounts")
    tool_ctx = context or ToolContext()
    account = await self._resolve_account(None, tool_ctx)
    ctx = self._build_ctx(tool_ctx, account)
    resolve_ctx = self._make_resolve_context(tool_ctx, "sync_accounts")
    result = _call_with_optional_ctx(upsert, params, ctx=resolve_ctx)
    if inspect.isawaitable(result):
        result = await result
    return cast("SyncAccountsResponse", result)

The ResolveContext carries the caller's AuthInfo and 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.

Both upsert and list are OPTIONAL on the AccountStore. The shims surface OPERATION_NOT_SUPPORTED (via _not_supported) when the store doesn't expose them — distinct from AttributeError, which is what an unguarded getattr().() would produce.

Wire advertisement

Adds _ACCOUNT_ADVERTISED_TOOLS = {"sync_accounts", "list_accounts"} and unions it into every sales-* entry of SPECIALISM_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.
  • New positive: sync_accounts / list_accounts route through to platform.accounts.upsert / .list with ResolveContext.tool_name set correctly.
  • New negative: stores that don't implement the optional Protocols surface OPERATION_NOT_SUPPORTED via NotImplementedResponse — distinct from AttributeError from an unguarded getattr chain.

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/setup against an agent that implements the optional store Protocols will see the scenario graduate from skipped (missing_tool) to graded the moment they pull this in. Verified end-to-end against bokelley/salesagent@4e62024cMCP 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.

…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.
@bokelley
Copy link
Copy Markdown
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.

@bokelley bokelley closed this May 10, 2026
@bokelley
Copy link
Copy Markdown
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>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant