Skip to content

feat(adcp)!: type media-buy seller follow-ups#142

Merged
bokelley merged 1 commit into
mainfrom
bokelley/argus-followups-seller-agent
May 26, 2026
Merged

feat(adcp)!: type media-buy seller follow-ups#142
bokelley merged 1 commit into
mainfrom
bokelley/argus-followups-seller-agent

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 25, 2026

Summary

Addresses the concrete Argus follow-ups from the Go reference seller harness merge, plus protocol/code/DX expert passes on the SDK surface:

  • replaces the generated CreateMediaBuyResponse = any callback surface with a real CreateMediaBuyResult interface implemented by the three generated schema variants
  • scopes MediaBuyData to the get_media_buys.media_buys[] item shape instead of using it as a create/task catchall
  • routes Config.GetMediaBuys through the generated GetMediaBuysResponse envelope so pagination/errors/context are reachable from normal adcp.Register handlers
  • types GetMediaBuysResponse.MediaBuys as []MediaBuyData and adds PackageStatus for get-media-buys package rows, including creative approvals, pending formats, and delivery snapshots
  • ensures get_media_buys emits media_buys: [] instead of null for empty/nil lists
  • adds typed response helpers for create media buy success/error/submitted variants
  • makes creative assignments typed without losing schema expressiveness: weight: 0 is preserved and vendor-specific assignment fields round-trip via CreativeAssignment.Extra
  • types SyncCreativesRequest.Assignments as []SyncCreativeAssignment
  • changes UpdateMediaBuyRequest.canceled and package updates to pointer booleans; docs now call out that canceled is nil-or-true while resume uses paused:false
  • normalizes package delivery rows to the flat schema shape instead of emitting nested package totals, with a comment for schema-required package spend
  • applies context echo consistently to signals and collection tools
  • makes list_creatives format filtering match the full format ref, not just id
  • weights simulated reported spend consistently by package budget
  • updates README, migration notes, and SDK-local build skills for the current typed request/response names and seller patterns

Notes

The protocol-managed skills/adcp-media-buy bundle still has stale buyer-facing examples for creative assignments/update wrappers/async status. I did not hand-edit that managed skill; it should be fixed upstream or refreshed through the bundle sync. Tracked in #143.

First-class Config.UpdateMediaBuy and Config.ListCreatives hooks are still an additive SDK follow-up. Tracked in #144.

The compliance scenario enum concern appears resolved in the current 3.0.12 schemas in this repo: seed_product, seed_pricing_option, and force_create_media_buy_arm are already present in the request/response scenario enums. I left that as an issue-tracking/spec-sync item rather than changing runtime behavior here.

Validation

  • go test ./...
  • cd adcp && go test ./...
  • cd cmd/router && go test ./...
  • cd targeting/prommetrics && go test ./...
  • cd reference/seller-agent && go test ./...
  • cd reference/context-agent && go test ./...
  • cd e2e && go test ./...
  • cd bench && go test ./...
  • cd adcp/schemas && python3 lint.py --strict
  • git diff --check
  • golangci-lint run ./...
  • cd adcp && golangci-lint run ./...
  • cd reference/seller-agent && golangci-lint run ./...
  • cd reference/context-agent && golangci-lint run ./...
  • ADCP_RUNNER_BIN=adcp ADCP_PORT=3015 STORYBOARD_RESULT_PATH=.context/storyboard-final-dx.json SELLER_LOG_PATH=.context/storyboard-final-dx.log ./scripts/ci/run_storyboard_reference_seller.sh (59/59)

@bokelley bokelley marked this pull request as ready for review May 25, 2026 22:30
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right shape on the typed fields. Breaking-change marker is the block — repo is at adcp-v1.0.0 and release-please will cut v1.1.0 with breaking Go API changes baked in, violating the SemVer contract adopters get by pinning ^v1.0.0.

The breaking surface is real:

  • adcp/types.go:480-487PackageDelivery.Totals field removed. Adopters reading pkg.Totals.Impressions will not compile.
  • adcp/inputs.go:48,66UpdateMediaBuyRequest.Canceled and PackageUpdate.Canceled flip from bool to *bool. Callers passing Canceled: true will not compile.
  • adcp/inputs.go:32,71CreativeAssignments flips from []any to []CreativeAssignment. Callers using inline map[string]any{...} will not compile.
  • adcp/responses.go:23-36MediaBuyResponse no longer flattens ext.* keys to the response root. Adopters using the old shape (or stuffing task_id/message into Ext) see a wire-shape change.

The single commit is feat(adcp): type media-buy seller follow-ups. It needs feat(adcp)!: (or a BREAKING CHANGE: footer) so release-please cuts v2.0.0. This is the MUST-FIX gate AGENTS.md documents — one-line fix at the commit subject.

Things I checked

  • Schema/types coherence: MediaBuyData added to KNOWN_TYPES in adcp/schemas/generate.py and EXEMPT in adcp/schemas/lint.py:79; CreativeAssignment registered in EXPLICIT_SCHEMA at lint.py:124. adcp/types_gen.go regenerated cleanly (drops the old MediaBuyData, updates Package.CreativeAssignments from []any to []CreativeAssignment). Schema bundle is downloaded at build time (adcp/schemas/download.sh) — I trust the author's python3 lint.py --strict run.
  • The decorateMediaBuy path at reference/seller-agent/cmd/seller-agent/main.go:632 clears Ext, but the forced/submitted path at line 310 returns before decorate runs, so adopter-provided ext on submitted buys is preserved.
  • weightedSpend in reference/seller-agent/cmd/seller-agent/main.go:608 is the right fix — old spend / float64(count) over-allocated equal shares across uneven budgets; new code weights by pkg.Budget / totalBudget and the test at main_test.go:328-359 locks it in (100/300 split → 10/30 spend).
  • Context-echo in adcp/seller.go:185-294 is applied symmetrically on success and error paths across all six tools. Clean.
  • MediaBuyData is in both EXEMPT (line 79) and EXPLICIT_SCHEMA (line 123) — the EXPLICIT_SCHEMA entry is now dead code because EXEMPT short-circuits first at lint.py:381. Pre-existing condition that this PR worsens slightly.

Follow-ups (non-blocking — file as issues)

  1. task_id emitted unconditionally on the submitted pathadcp/responses.go:26 builds {"status": "submitted", "task_id": data.TaskID} even when data.TaskID == "". AdCP's submitted envelope marks task_id as required-non-empty; the SDK should guard with if data.TaskID != "" or fail-fast in MediaBuyResponse so adopters can't ship an empty-string task_id. Reference seller always sets it (main.go:310), so no test catches this today.

  2. MediaBuyData in EXEMPT loses drift coverage on the four new envelope fields. Currency, ValidActions, TaskID, Message are not in core/media-buy.json — the PR's design intent is "base schema permits additional response properties." That's plausible (in-tree precedent: MediaBuyListItem carries top-level currency), but MediaBuysResponse at adcp/responses.go:39-42 now returns raw []MediaBuyData items. If the items-schema for get_media_buys.media_buys[] sets additionalProperties: false, the four new top-level keys are wire drift the linter won't catch. Worth a sanity check.

  3. Dead EXPLICIT_SCHEMA entry. adcp/schemas/lint.py:123 maps MediaBuyDatacore/media-buy.json, but MediaBuyData now sits in EXEMPT at line 79; the short-circuit at line 381 means the EXPLICIT_SCHEMA entry never resolves. Drop one or the other.

Minor nits (non-blocking)

  1. formatRefKey separator. reference/seller-agent/cmd/seller-agent/main.go:299 joins AgentURL + "\x00" + ID for a map key. FormatRef is {AgentURL, ID} with both fields strings — comparable, so map[adcp.FormatRef]bool drops the separator entirely. Same correctness, less ceremony.

  2. decorateMediaBuy clears Ext silently. reference/seller-agent/cmd/seller-agent/main.go:632 sets buy.Ext = nil. Today no path routes a forced/submitted buy through decorate, so adopter-provided ext is safe. A one-line comment ("clears Ext deliberately — typed fields are the contract") would prevent a future regression.

  3. MediaBuyData.Status asymmetry. adcp/types.go:443 keeps Status non-omitempty while peer schema-required fields (MediaBuyID:442, TotalBudget:448, Packages:449) gained ,omitempty. Submitted path bypasses the struct so this doesn't bite today, but a zero-value MediaBuyData{} marshals "status": "" to the wire via MediaBuysResponse. Either keep the required fields non-omitempty for symmetry, or document the asymmetry.

Retitle the commit feat(adcp)!: type media-buy seller follow-ups. Happy to re-review once the breaking marker is in place.

@bokelley bokelley force-pushed the bokelley/argus-followups-seller-agent branch from e830235 to 97d2c6f Compare May 25, 2026 22:42
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 25, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Typed envelope beats stringly-typed Ext, and the KNOWN_TYPES / EXEMPT / EXPLICIT_SCHEMA story is consistent with AGENTS.md §58–60.

Things I checked

  • feat(adcp)!: on the commit. The wire-shape breaks (PackageDelivery.Totals removed, UpdateMediaBuyRequest.Canceled bool→*bool, MediaBuyData no longer flattens ext) get the breaking marker. Title drops the ! but the commit doesn't, which is what release-please reads.
  • adcp/types_gen.go is a clean regen: the MediaBuyData block is gone (now hand-written in types.go), and Package.CreativeAssignments swaps to []CreativeAssignment. No hand-edits.
  • MediaBuyData is in both KNOWN_TYPES (adcp/schemas/generate.py:34) and EXEMPT (adcp/schemas/lint.py:79). CreativeAssignment is in KNOWN_TYPES (adcp/schemas/generate.py:70) and EXPLICIT_SCHEMA (adcp/schemas/lint.py:123), so strict-parity lint still checks the typed assignment shape against core/creative-assignment.json. The PR validation ran python3 lint.py --strict clean.
  • No callers of Canceled bool remain — four seller-agent tests, all flipped to &canceled.
  • Submitted-path guard at adcp/responses.go:27 (require TaskID) is the right call. An async media-buy without a task ID is unrecoverable on the buyer side.
  • item := *buy in get_media_buys is a value copy — decorateMediaBuy(&item) nilling Ext does not touch the stored buy.
  • weightedSpend(s, b, n*b, n) = s/n — equal-budget case preserves prior split semantics.
  • attachContext rework: each shadowed inner result, out, e := errorToResult(...) returns from within its own if block. No accidental return of the outer name.
  • FormatRef is comparable, valid as a map key. The (agent_url, id) filter is the right shape — same-id-different-agent collisions don't.
  • 11 files, +271/-128. Test plan ran the full Go suite, all sub-packages, schema strict-lint, golangci-lint, and the storyboard reference seller (59/59).

Follow-ups (non-blocking — file as issues)

  • MediaBuyData.TaskID / Message sit on the media-buy struct alongside core resource fields. omitempty keeps them off the wire on the active path today, but architecturally they're response-envelope fields, not media-buy fields. If upstream ever lands a media-buy resource that legitimately needs its own task_id, this collides. Consider splitting MediaBuyData (resource) from a separate CreateMediaBuyEnvelope (envelope) at a future opportunity. ad-tech-protocol-expert flagged the shape, and the trade-off is the lint compromise — MediaBuyData is now in EXEMPT rather than EXPLICIT_SCHEMA, so strict-parity drift detection on the in-spec media-buy fields is off until the split lands.
  • PackageDelivery is now flat (Impressions, Clicks, Spend) while MediaBuyDelivery.Totals stays nested (DeliveryTotals). The asymmetry matches the PR's claim about the spec shape, but PackageDelivery is in EXEMPT, so strict-lint won't catch drift here on the next schema sync. Worth eyeballing against core/package-delivery.json when 3.0.13 lands.
  • forceMediaBuyStatus mutates buy.Status without calling decorateMediaBuy, so the stored buy's ValidActions goes stale until the next get_media_buys refresh re-decorates the copy. Test-controller path only, but inconsistent with every other status mutator in the file.

Minor nits (non-blocking)

  1. Redundant zero-total guard. reference/seller-agent/cmd/seller-agent/main.go:533 keeps if total == 0 { continue } after the refactor, even though weightedSpend now handles totalBudget == 0 internally. Pick one location.
  2. Filter equality uses full FormatRef. reference/seller-agent/cmd/seller-agent/main.go:273 uses map[FormatRef]bool, which compares all five fields. The test only exercises the {agent_url, id} case where the others are zero. If a buyer ever sends a filter with Width/Height set, it will silently fail to match a stored creative whose FormatRef carries sizing metadata. Either narrow the key to {AgentURL, ID} or document the full-struct semantics.
  3. Submitted-path error shape. adcp/responses.go:27 returns a plain fmt.Errorf. The rest of the SDK boundary uses adcp.NewError("INTERNAL_ERROR", ...) to keep the wire-shape consistent. Only one caller sets Status: "submitted" and force_create_media_buy_arm validates TaskID upfront, so this is cosmetic.

LGTM. Follow-ups noted above.

@bokelley bokelley force-pushed the bokelley/argus-followups-seller-agent branch from 97d2c6f to 3503e49 Compare May 26, 2026 01:34
@bokelley bokelley changed the title feat(adcp): type media-buy seller follow-ups feat(adcp)!: type media-buy seller follow-ups May 26, 2026
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 26, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. The Argus-followup story is real: typed valid_actions, typed task_id/message, weight *float64 to preserve 0, Canceled *bool, decorateMediaBuy nukes Ext, context echo plumbed through signals + collection — the right shape on every count. feat(adcp)!: is correctly load-bearing here.

Things I checked

  • feat(adcp)!: marker present on the only commit (3503e493). Canceled bool → *bool, CreativeAssignments []any → []CreativeAssignment, PackageDelivery.Totals removal, and MediaBuysResponse sandbox shape — every breaking flip is named in MIGRATING.md and aligned with the !. Bool(sandbox) always returns non-nil so the wire still emits \"sandbox\": false (adcp/responses.go:70).
  • KNOWN_TYPES / EXEMPT / EXPLICIT_SCHEMA hygiene. MediaBuyData, MediaBuyHistoryEntry, SyncCreativeAssignment added to both KNOWN_TYPES (adcp/schemas/generate.py:34,47) and EXEMPT (adcp/schemas/lint.py:79-80). CreativeAssignment correctly lands in KNOWN_TYPES and EXPLICIT_SCHEMA (lint.py:124) — that's the combination that keeps the generator from emitting it while leaving the linter to diff property drift against core/creative-assignment.json. _assert_exempt_subset_known invariant holds. The json:\"-\" skip in the field parser (lint.py:183) matches the new Extra field exactly.
  • safe_comment rstrip is the source of the ~120 whitespace-only hunks in adcp/types_gen.go. Spot-checked the typed-name promotions (CreateMediaBuySubmitted, Package.CreativeAssignments []CreativeAssignment, SyncCreativesRequest.Assignments []SyncCreativeAssignment) — generator was rerun, not hand-edited.
  • CreativeAssignment round-trip is right shape. weight: 0 survives via *float64, nil pointer omits, vendor fields land in Extra and emit back, typed fields win on key collision (adcp/inputs_test.go). Reference seller migrates to the typed form at every call site.
  • MediaBuyResponse submitted branch rejects empty TaskID with MISSING_FIELD and a Suggestion: pointing at CreateMediaBuySubmittedResponse. Only in-repo submitted-status producer is force_create_media_buy_arm and it now sets TaskID/Message on the typed envelope (reference/seller-agent/cmd/seller-agent/main.go). Fail-closed beats fail-open.
  • decorateMediaBuy zeroing Ext — no in-repo scenario uses MediaBuyData.Ext for legit data; compliance scenarios (seed_product, seed_pricing_option, force_create_media_buy_arm) all go through typed fields. Safe.
  • attachContext is wired on every signals + collection branch, error and success paths both. result, out, e := ... inside the if err != nil blocks declares fresh locals — no outer shadowing leak.
  • weightedSpend has the divide-by-zero guards in the right order (packageCount == 0 short-circuit, then totalBudget == 0 → equal split), parity with the old equal-split when total is zero, and TestDeliveryReporting_SimulateDeliveryWeightsSpendByBudget pins the 100/300 → 10/30 split.
  • Validation block: go test ./..., cd adcp && go test ./..., both reference agents, e2e/, bench/, lint.py --strict, golangci-lint, and storyboard 59/59. Manual storyboard run is checked, not left unchecked.

Follow-ups (non-blocking — file as issues)

  1. MediaBuyData lint coverage regressed. Moving MediaBuyData out of EXPLICIT_SCHEMA into EXEMPT (adcp/schemas/lint.py:79) turns off property-name drift detection against core/media-buy.json. The struct comment in adcp/types.go:434-437 justifies it — the base schema allows additional response properties — but a future schema-side rename (say confirmed_atconfirmation_timestamp) will not trip the linter. Either reinstate an explicit-schema entry that allows extra Go fields, or keep EXEMPT but add a one-line allowlist comment so the next sync is verifiable. (ad-tech-protocol-expert flagged this.)
  2. MediaBuyData makes schema-required fields omitempty. media_buy_id, total_budget, packages are all omitempty on the multi-purpose struct (adcp/types.go:438,444,447). On the sync success path that's a regression vs the generated CreateMediaBuySuccess. A buggy seller that forgets MediaBuyID will emit a payload silently missing a required field instead of \"media_buy_id\": \"\". The submitted path is already split out via CreateMediaBuySubmitted, so the success-success fields could stay required. Either drop omitempty from the required-on-success fields, or split MediaBuyData into success + envelope structs.
  3. update-media-buy-request.json still not in TOOL_SCHEMAS. Pre-existing gap (not introduced here), but UpdateMediaBuyRequest / PackageUpdate are entirely hand-written with zero schema cross-check (adcp/schemas/generate.py:99-168). The *bool flip on Canceled is the right call given the generated Package.Canceled precedent at types_gen.go:1844, but the whole request type rides on the honor system. Worth wiring up.
  4. valid_actions: [] vs omitted, and weight: 0 vs omitted, are protocol semantics this PR locks into the SDK without upstream anchoring. Both are defensible interpretations — [] as a positive "no actions available" claim, 0 as "explicitly paused" — and the reference seller emits [] for terminal statuses consistently (reference/seller-agent/cmd/seller-agent/main.go:624). But neither is codified in the schema descriptions or in skills/adcp-media-buy/SKILL.md. Confirm with the spec authors so downstream buyers don't diverge on the interpretation, then put it on the wire.

Minor nits (non-blocking)

  1. MediaBuyData.MarshalJSON shifts key ordering to alphabetical when ValidActions != nil but keeps struct-declaration order when it's nil (adcp/types.go:465-480). Harmless for JSON consumers, breaks byte-level golden-file diffs. If you want one path one ordering, marshal directly into map[string]any and merge unconditionally.
  2. CreativeAssignment.MarshalJSON treats every known key in isCreativeAssignmentField as a collision even when the typed field is the zero value (adcp/inputs.go:39-56). Setting Weight: nil, Extra: {\"weight\": 99} produces wire {} for the weight key, and a user who tucked placement_ids into Extra by mistake will see it silently dropped. Either let Extra win when the typed field is the zero value, or document this on the field.
  3. CreateMediaBuySuccessResponse is a no-op alias for MediaBuyResponse (adcp/responses.go:48-51). Either delete or document why both names live in the public API — README/MIGRATING only reference one.
  4. adcp/seller.go repeats the result, out, err := X(...); return attachContext(result, input.Context), out, err pattern about a dozen times. A respondWithContext(call, out, err, ctx) helper would shrink the file and make adding context echo to a new tool a one-liner. Refactor candidate.

Approving on the strength of the typed-envelope coverage, the storyboard 59/59, and the schema-lint hygiene — the follow-ups are real but live downstream of one design decision (one MediaBuyData for three response shapes) and the Minors are surgical.

@bokelley bokelley force-pushed the bokelley/argus-followups-seller-agent branch from 3503e49 to dc93b91 Compare May 26, 2026 02:47
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 26, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. The shape is right: typed *bool for tri-state canceled, typed []CreativeAssignment with Extra round-trip for schema-allowed vendor fields, and three-branch dispatch on CreateMediaBuyResponse mapping to the schema oneOf. feat(adcp)!: carries the breaking marker correctly.

Things I checked

  • Conventional-commit breaking marker present. Title feat(adcp)!: type media-buy seller follow-ups and the single commit dc93b91 both carry !. Wire-shape changes (Canceled bool*bool, CreativeAssignments []any[]CreativeAssignment, PackageDelivery.Totals removal, Config.CreateMediaBuy return type) are properly marked.
  • Schema-vs-types coherence. adcp/types_gen.go regenerated in this PR — the comment-only trailing-whitespace churn across ~50 types confirms safe_comment ran with the new .rstrip(). KNOWN_TYPES in adcp/schemas/generate.py:34-35,48,70 lists the four new hand-written types (MediaBuyData, MediaBuyHistoryEntry, SyncCreativeAssignment, CreativeAssignment). FIELD_TYPE_OVERRIDES at generate.py:257 correctly maps ('SyncCreativesRequest', 'assignments') to SyncCreativeAssignment.
  • No protocol-managed skills touched. Only skills/build-creative-agent, skills/build-generative-seller-agent, skills/build-retail-media-agent, skills/build-seller-agent modified — all SDK-local. skills/adcp-* bundle is untouched, which is the right call (those are CODEOWNER-gated and stale buyer examples should fix upstream per #143).
  • CreateMediaBuyResponse union dispatch. adcp/responses.go:24-48 handles pointer + value forms of all three schema oneOf branches (Success / Error / Submitted) and falls through to INVALID_REQUEST with a clear suggestion. ad-tech-protocol-expert confirmed 3.0.12 has no fourth branch — pending states surface via tasks/get against the task_id, not as a separate create_media_buy shape.
  • CreativeAssignment round-trip. adcp/inputs.go:43-104. weight: 0 preserved (typed *float64); typed fields beat Extra collisions; vendor fields round-trip through Extra. adcp/inputs_test.go covers both invariants.
  • MediaBuyData.MarshalJSON empty-array semantics. adcp/types.go:457-472. nil → valid_actions omitted; []string{} → emitted as []. Matches the documented protocol distinction ([] means no actions; omission means seller did not say). Test at adcp/responses_test.go:61-82 exercises the explicit-empty path.
  • weightedSpend numerics. reference/seller-agent/cmd/seller-agent/main.go:564-572. spend == 0 || packageCount == 0 short-circuits; totalBudget == 0 falls back to even split; no divide-by-zero. Test at main_test.go:327-378 asserts the 100/300 budget split lands at 10/30 spend.
  • Forced-arm lock interleaving. reference/seller-agent/cmd/seller-agent/main.go:352-367. Read-and-consume of b.forced is one critical section; createMediaBuyResponse unlocks before calling createMediaBuy so no double-lock. Sound.
  • Bool(false) wire shape preserved. MediaBuysResponse now wraps in MediaBuysData{Sandbox: Bool(sandbox)}. *bool,omitempty only omits on nil, so Bool(false) still emits "sandbox": false — same wire as the old map[string]any{"sandbox": false} path.
  • Test plan executed in full. PR body lists go test ./..., both seller and context agents, e2e, bench, lint.py --strict, golangci-lint across three modules, and the storyboard runner at 59/59. No unchecked items.
  • Expert verdicts. code-reviewer: no blockers, one Medium follow-up (no *CreateMediaBuyError test) and one Low (createMediaBuySubmittedResponse mutates caller's Status). ad-tech-protocol-expert: sound-with-caveats — flags the lint.py regression below. python-expert: well-formed, confirms the same lint.py regression and one latent regex edge case.

Follow-ups (non-blocking — file as issues if not already)

  1. MediaBuyData lint coverage regression — file an issue. adcp/schemas/lint.py:79 adds MediaBuyData to KNOWN_TYPES and the same change drops 'MediaBuyData': 'core/media-buy.json' from EXPLICIT_SCHEMA at lint.py:121. Net effect: the hand-written MediaBuyData struct in adcp/types.go:434-454 is no longer compared against any schema file by lint. MediaBuyHistoryEntry and SyncCreativeAssignment are in the same boat (they're in KNOWN_TYPES with no EXPLICIT_SCHEMA mapping; KNOWN_TYPES lint behavior may exempt them entirely). The new CreativeAssignment entry in EXPLICIT_SCHEMA is the right pattern — apply it to the others so the hand-written get_media_buys item shape stays strictly checked. This is exactly the kind of drift the rest of the PR is trying to prevent.
  2. Spec sanity on the hand-written MediaBuyData shape. ad-tech-protocol-expert flagged two items worth confirming against 3.0.12 core/media-buy.json: (a) is currency actually at the media-buy level or derived from packages, and (b) does history[] live on the item versus the response envelope. If lint coverage is restored per #1 above, this resolves itself the next time python3 lint.py --strict runs.
  3. *CreateMediaBuyError branch is untested. adcp/responses.go:34-37,57-64 sets result.IsError = true on the error variant. Add a test in adcp/responses_test.go that passes &CreateMediaBuyError{Errors: []AdcpError{{Code: "X"}}} and asserts result.IsError == true plus the structured-content shape.
  4. CreativeAssignment.Extra requires the schema to allow additional properties. If 3.0.12's core/creative-assignment.json is additionalProperties: false, vendor fields will fail strict downstream validation even though they round-trip through this SDK. Worth grep'ing the schema once #1 is done.
  5. SyncCreativeAssignment — confirm no action discriminator. Drafts of 3.0.12 carried an optional add/remove discriminator on sync assignments. If that landed, the 4-field struct silently drops it. Worth a one-line lookup against task/creative/sync-creatives-request.json.

Minor nits (non-blocking)

  1. createMediaBuySubmittedResponse mutates the caller's struct. adcp/responses.go:69-72 writes data.Status = "submitted" when empty. Most response helpers don't mutate inputs. Either document this or shallow-copy. Not a correctness bug.
  2. safe_comment rstrip breadth. adcp/schemas/generate.py:277. Bare .rstrip() strips all Unicode whitespace (NBSP, narrow NBSP, ideographic space). Tightening to .rstrip(' \t') matches the actual bug being fixed (description with literal space at column 80). Defensible as-is.
  3. json:"-" skip regex edge case. adcp/schemas/lint.py:184-185. if fm.group(3) == '-' would also match the Go-documented json:"-," escape (literal - field name), incorrectly skipping it. No struct currently uses this escape, so it's latent. Tighten to if fm.group(3) == '-' and fm.group(4) == '' if you want to be strict.

Safe to merge.

@bokelley bokelley force-pushed the bokelley/argus-followups-seller-agent branch from dc93b91 to fd63567 Compare May 26, 2026 03:12
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Right shape, breaking marker correct, migration notes load-bearing. Follow-ups below.

The typed-sum on Config.CreateMediaBuy is the move the schema's been asking for — three branches on the wire, three concrete Go types, one marker interface. MediaBuyData rescoping to a get_media_buys row (vs. the previous create/list/task catchall) was overdue. The flattenExt smuggle is finally gone; Ext is now a real extension field, not a top-level-fields conduit.

Things I checked

  • feat(adcp)!: on the commit covers the wire-API removals: old MediaBuyData shape, MediaBuyResponse(*MediaBuyData) signature, Config.GetMediaBuys return type, flattenExt. release-please will treat this correctly.
  • CreateMediaBuySuccess.Packages stays []Package (adcp/types_gen.go:2240); only MediaBuyData.Packages is []PackageStatus. The create-vs-list distinction is preserved, as the PR description claims.
  • MediaBuysResponse(nil, true) emits media_buys: [], not null (adcp/responses_test.go:84). That was the stated fix.
  • Canceled *bool round-trips nil-vs-true correctly through update_media_buy; MIGRATING.md:9-12 documents the "use Paused: Bool(false) for resume" contract.
  • CreativeAssignment.MarshalJSON typed-fields-win on collision (adcp/inputs_test.go:31). Vendor Extra round-trips for scalar values.
  • attachContext is uniform across the typed handlers in adcp/seller.go — every error path and every success path echoes input.Context.
  • cd adcp/schemas && python3 lint.py --strict per PR validation log; golangci-lint clean across all four modules; storyboard 59/59.
  • Protocol-managed skills (skills/adcp-*, skills/call-adcp-agent) untouched. Only SDK-local skills/build-* modified. CODEOWNERS gate intact.

Follow-ups (non-blocking — file as issues)

  • Schema lint coverage strictly weaker on the rescoped types. adcp/schemas/lint.py:79-81 adds MediaBuyData, MediaBuyHistoryEntry, PackageStatus, PackageCreativeApproval, PackageSnapshot, SyncCreativeAssignment to EXEMPT, and drops MediaBuyData from EXPLICIT_SCHEMA. The hand-written types are now drift-blind against get_media_buys_response.json#/properties/media_buys/items and the package-row subschema. When 3.0.13 lands, upstream field add/remove on those shapes will pass lint.py --strict silently. Either point EXPLICIT_SCHEMA at the subschema (needs a JSON-pointer extension in lint.py) or land a curated-subset diff so additions surface in CI. ad-tech-protocol-expert flagged this; code-reviewer agreed.
  • PackageStatus.MarshalJSON field coverage is hand-rolled. adcp/types.go:497-551 walks 14 base Package fields plus 4 status-only ones. The generated Package has 22+ properties — measurement_terms, performance_standards, format_ids_to_provide, optimization_goals, agency_estimate_number are silently dropped on get_media_buys rows. Intentional per the curated-row narrative, but tied to the lint gap above: if any of those are in the upstream package-row schema, buyers parse a row that's missing fields the seller declared at create time. Worth a cd adcp/schemas && ./download.sh 3.0.12 && diff audit before the next protocol bump.
  • listCreatives format-filter key change. reference/seller-agent/cmd/seller-agent/main.go:259 now keys on the full FormatRef struct. A buyer filtering {agent_url, id} against a creative synced with {agent_url, id, width:300} misses. Reference impl only, but a buyer porting their fixture across PRs will see a behavior change. The PR description frames this as "match the full format ref, not just id," which is the right intent; a sentence on dimension semantics in MIGRATING.md would close the loop.
  • #143 and #144 already filed by the author. Stale buyer-facing examples in the managed skills/adcp-media-buy bundle (upstream fix), and Config.UpdateMediaBuy / Config.ListCreatives SDK helpers (additive). Not in scope here — noting the trail.

Minor nits (non-blocking)

  1. Redundant defaulting on submitted status. adcp/responses.go:73-76 defaults data.Status = "submitted" then errors if it isn't "submitted". After defaulting, the only path that reaches the error branch is a non-empty mismatched status (e.g., caller passing "pending"). Fine, just notable.
  2. TOCTOU window in forced-arm consumption. reference/seller-agent/cmd/seller-agent/main.go:354-369 drops the lock between reading b.forced and calling createMediaBuy (which re-locks). A concurrent force_create_media_buy_arm lands for the next call, not this one. Test fixture; one-line comment would spare a future reader the trace.
  3. CreativeAssignment.Extra round-trip is scalar-only in tests. adcp/inputs_test.go doesn't exercise nested map[string]any or []any values. The any-typed round-trip is standard encoding/json and works, but a test would lock the contract in.

Approving on the strength of the typed-sum shape plus the conventional-commit breaking marker. Safe to merge.

@bokelley bokelley merged commit 9621ce8 into main May 26, 2026
17 checks passed
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