feat(protocol): draft account change feed - #6811
Conversation
| supportedCanonicalFormatsCapability, | ||
| } from '../task-handlers.js'; | ||
| import { GET_PRODUCTS_REJECTED_ADCP_VERSION, supportsGetProductsRejected, type TrainingContext } from '../types.js'; | ||
| import { GET_PRODUCTS_REJECTED_ADCP_VERSION, supportsAccountChangeFeed, supportsGetProductsRejected, type TrainingContext } from '../types.js'; |
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths require human/CODEOWNERS approval.
This PR modifies files under static/schemas/source/**, which are under a hard, path-based approval gate (gated_paths: true). The required review status is not APPROVED (unknown/none), so decision-table row 2 fires: the gate is not satisfied and the outcome must be escalate.
The reviewer surfaced no Critical/High/Medium findings — this is a clean additive 3.2 draft (new list_account_changes task, account.change_recorded notification, CURSOR_EXPIRED code, change_feed capability). Schema↔docs coherence, changeset scope/type (correctly additive/Normative), oneOf disjointness, and released-artifact immutability were all verified. Absent the gate, this would approve.
Human/CODEOWNERS review is required on the gated schema-source files before merge.
Escalation reasons:
- Gated paths touched without an APPROVED review:
static/schemas/source/account/list-account-changes-request.json,list-account-changes-response.json,core/account-change-recorded-webhook.json,core/account-change.json,core/account.json,core/notification-config.json,enums/error-code.json,enums/notification-type.json,index.json,protocol/get-adcp-capabilities-response.json— CODEOWNERS/human approval required.
Why human review
- Gated schema-source files changed (static/schemas/source/**) but review_decision is not APPROVED — human/CODEOWNERS approval required (decision rule row 2).
- Affected gated files include list-account-changes-request/response.json, account-change*.json, account.json, notification-config.json, error-code.json, notification-type.json, index.json, and get-adcp-capabilities-response.json.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/account/list-account-changes-request.json (added) matches
static/schemas/source/**; static/schemas/source/account/list-account-changes-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/account-change-recorded-webhook.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/account-change.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/account.json (modified) matchesstatic/schemas/source/**; static/schemas/source/core/notification-config.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/notification-type.json (modified) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/protocol/get-adcp-capabilities-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'unknown', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
| "description": "Read a durable, ordered feed of material changes to AdCP-visible state for one account. Use a latest checkpoint before snapshot bootstrap, then drain from that cursor to close the bootstrap race.", | ||
| "x-tool-summary": "List durable changes to authoritative AdCP-visible state for one account.", | ||
| "type": "object", | ||
| "allOf": [ |
There was a problem hiding this comment.
Interoperability concern: this request schema has a top-level allOf. Anthropic MCP tool registration rejects top-level input combinators, which can make the entire tool catalog unavailable rather than only this task. Can the generated SDK/tool schema flatten the version-envelope properties and enforce the cursor/starting-position exclusion at runtime, with a conformance assertion that the emitted MCP inputSchema is a plain root object?
|
|
||
| Current authorization controls both snapshots and history. Sellers filter | ||
| inaccessible resource changes without revealing their existence or count, but | ||
| still advance the scoped cursor across filtered records. Cursors are bound to |
There was a problem hiding this comment.
Principal binding prevents cross-principal replay, but what happens when the same principal authorization scope expands? A cursor may already have advanced across changes that were filtered while inaccessible; granting access later would make those resources visible without replaying their earlier changes. I think the contract needs an authorization/scope epoch bound into the cursor, or a normative requirement to invalidate the cursor and force snapshot rebootstrap whenever visible scope changes.
| 1. reflect the resulting current state on that authoritative read regardless | ||
| of whether the change originated through AdCP, a seller UI or API, seller | ||
| automation, another authorized principal, or a connected platform; | ||
| 2. append one immutable account change record in the same commit boundary; |
There was a problem hiding this comment.
Could we define the minimum invalidation granularity? One committed operation can update a media buy, several packages, and creative assignments. “One record” is ambiguous between one per transaction, one per resource, and one per repair-read closure. My preference is at least one record per independently repairable authoritative identity, allowing same-commit changes to share an optional batch_id and allowing nested package changes to coalesce under a media-buy invalidation only when get_media_buys fully repairs them.
| The seller-wide capability lists resource types the seller can support. | ||
| `list_account_changes.source_coverage[]` reports account-specific source | ||
| health with a source kind, status, resource types, optional coverage start, | ||
| last successful sync, and observed-through watermark. |
There was a problem hiding this comment.
current needs an operational definition if buyers are expected to distinguish feed catch-up from upstream freshness. Could the capability/coverage contract declare a poll cadence or stale_after, so last_successful_sync_at and observed_through can be evaluated mechanically? Otherwise a connector last observed days ago can still self-label current without violating a testable threshold.
|
SDK adopter follow-up filed: adcontextprotocol/adcp-client#2669. It covers generated types/enums, a typed |
|
Review at Overall: strong draft — the tree delivers what the RFC promises, and it's wired unusually completely. The notification enum's own meta-rule (anchor + Four findings: 1. blocker — the coverage matrix promises wholesale change records the schema cannot express
2. warning — cursor/filter mismatch has no defined disposition, and the silent branch loses data permanentlyThe cursor is "bound to the normalized filter" (request schema; task doc), but nothing defines what a seller MUST do when a cursor arrives with different filters than it was minted under. Two conformant implementations diverge: one rejects with 3. warning — the commit-boundary invariant is unimplementable for non-mediated changes as literally writtenInvariant item 2: "append one immutable account change record in the same commit boundary." For a seller whose authoritative reads pass through to the underlying ad server, a seller-UI or connected-platform change is visible on 4. warning — the ratified "durable seller-initiated subscription is 4.0 work" statements survive untouched and now misleadThree normative sentences this PR doesn't modify: Notes
|
Summary
list_account_changesdurable account feed,account.change_recordedwake-up, capability declaration, cursor-expiry recovery, privacy boundary, and 90-day retention floorRFC/epic: #6810
Why a new feed
webhook_activity[]is delivery-attempt diagnostics, task history covers AdCP operations, and snapshots only answer current state. None can durably enumerate changes made while the buyer is idle. The feed is ordered metadata plus a safe repair-read hint; authoritative reads remain current truth.Scope boundary
The completeness claim covers committed material changes to account-scoped control-plane fields recoverable through an advertised authoritative read. It excludes raw audience members/events, per-impression delivery accumulation, naturally moving forecasts, validation failures, dry runs, exact idempotency replays, and webhook delivery attempts.
Reviewer focus
This draft is ready for protocol review. The highest-value questions are:
an advertised authoritative read, but not high-volume data-plane events?
list_account_changesfeed preferable to per-resource feedsor notification-only convergence?
account.change_recordednotification to every active subscriber, includingchanges initiated synchronously through AdCP?
CURSOR_EXPIREDrebootstrap sufficient?
may advertise each resource family?
while 3.2 ships only source-neutral snapshots and invalidations?
The SDK/storage mechanics below are merge gates for the reference deployment,
not blockers to reviewing those protocol decisions. The SDK already provides a
generic
PostgresStateStore; the seller runtime still owns durable orderedrecords, retention, cursor checkpoints, and atomic state/change commits. A
specialized reusable SDK change-feed store can follow once the wire contract is
accepted.
Draft / merge gates
This PR is intentionally draft and MUST NOT merge until:
advertising the capability in its production deployment; an in-memory
reference cannot claim 90-day retention
account.change_recordednotification value; the currently installed SDK rejects registration before regenerationcursor-expiry recovery (the reference unit suite already covers fanout,
pause/removal, re-emission identity, total ordering, concurrent append,
filtered scanning, empty-tail checkpoints, and 90-day expiry)
The training seller now advertises only
creativecoverage. Its shared-account fixture proves that a creative added through a connected platform while the buyer is idle appears both inlist_account_changesand the authoritativelist_creativesread. Account, media-buy, money, and reporting coverage remain explicitly unadvertised until their mutation paths and snapshots meet the same bar.Within creative coverage, the reference implementation now exercises
connected-platform create and status modification, direct
sync_creatives,inline media-buy create/update, purge, shared-account readback, no-op replay
suppression, cross-account isolation, and subscriber fanout. The learning lab
uses both external creation and external modification so this is visibly a
shared account, not a buyer-exclusive sandbox.
Security decisions
repair.taskis a closed allowlist of reads and is only a hint; buyers construct and validate requests locally and never dispatch feed-provided argumentsValidation
npm run test:schemasnpm run typechecknpm run test:docs-navnpm run test:tenant-routingnpm run test:sdk-runner-capability-gatesnpm run test:error-code-driftnpx vitest run --config server/vitest.config.ts server/tests/unit/account-change-feed.test.tsnpx vitest run --config server/vitest.config.ts server/tests/unit/training-agent.test.tsnpm run test:oneof-discriminatorsnpm run test:error-codesgit diff --check