Skip to content

fix: v2/v3 backwards compat for create_media_buy, update_media_buy, get_products#274

Merged
EmmaLouise2018 merged 5 commits into
mainfrom
EmmaLouise2018/brand-manifest-schema-fix
Feb 26, 2026
Merged

fix: v2/v3 backwards compat for create_media_buy, update_media_buy, get_products#274
EmmaLouise2018 merged 5 commits into
mainfrom
EmmaLouise2018/brand-manifest-schema-fix

Conversation

@EmmaLouise2018

@EmmaLouise2018 EmmaLouise2018 commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Strip legacy brand_manifest before strict Zod validation fires for create_media_buy and update_media_buy — v2.5 callers no longer hit "Request validation failed: Unrecognized key: brand_manifest"
  • Convert brand (BrandReference) → brand_manifest bare domain URL when adapting requests for v2 servers — previously brand passed through and v2 servers rejected it
  • Throw a clear error when proposal_id is sent to a v2 server (proposal mode is v3-only; v2 requires explicit packages)
  • Strip catalog from package items for v2 servers (optimization_goals was already stripped; catalog was missed)
  • Fix dead-code delete: account_idaccount in adaptGetProductsRequestForV2
  • Align brand URL format: get_products and create_media_buy both now use https://<domain> (bare domain) consistently

Test plan

  • New tests in request-validation.test.js: brand_manifest stripped pre-validation for create_media_buy, brand wins when both supplied, mock-based test verifying conversion at the transport layer
  • Updated + new tests in v3-compatibility.test.js: brand → brand_manifest conversion, brand-with-no-domain preserved, proposal_id throws, catalog stripped from packages, adaptUpdateMediaBuyRequestForV2 brand passthrough documented
  • All existing tests pass
  • Build and typecheck clean

…et_products

- Strip brand_manifest before strict Zod validation for create/update media buy
- Convert brand → brand_manifest (bare domain URL) when adapting for v2 servers
- Throw on proposal_id for v2 servers (proposal mode is v3-only)
- Strip catalog from package items for v2 servers (v3-only field)
- Fix dead-code delete: account_id → account in adaptGetProductsRequestForV2
- Align brand URL format: both get_products and create_media_buy use https://<domain>

@nastassiafulconis nastassiafulconis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall approach is solid — lifting brand_manifest normalization to cover more task types and the new v2 adaptation for brand/catalog/proposal_id all make sense. A couple things to fix up before this is good to go though.

Comment thread src/lib/utils/creative-adapter.ts
Comment thread src/lib/utils/creative-adapter.ts
Comment thread src/lib/core/SingleAgentClient.ts
Comment thread test/lib/request-validation.test.js
Comment thread test/lib/v3-compatibility.test.js
@EmmaLouise2018

Copy link
Copy Markdown
Contributor Author

Addressed all 5 review comments — pushed the fixes:

#1 (creative-adapter.ts:97) — brand?.domain guard
Changed brand ? \https://${brand.domain}`brand?.domain ? `https://${brand.domain}`so a brand object without adomainfield doesn't produce"https://undefined"`.

#2 (creative-adapter.ts:98) — preserve brand when no domain
When brand is present but has no domain (so can't be converted to a brand_manifest URL), brand is now re-added to the return value — consistent with adaptGetProductsRequestForV2 which also leaves brand on the object when it can't be converted. Added a test for this case.

#3 (SingleAgentClient.ts:2032) — remove update_media_buy from normalization block
Dropped update_media_buy from the brand_manifest → brand normalization condition. Neither the v2 nor v3 update_media_buy schemas have a brand field, so the normalization was wrong to include it. Added a comment to the condition explaining the deliberate exclusion.

#4 (request-validation.test.js:130) — verify actual conversion, not just no-throw
Added a new test that mocks A2AClient.fromCardUrl to capture the parameters actually sent to the server. It asserts that brand_manifest is not forwarded as an object (i.e., normalization happened). The existing no-throw tests are kept for the validation-layer coverage.

#5 (v3-compatibility.test.js:349) — adaptUpdateMediaBuyRequestForV2 brand tests
Added a test that verifies the adapter passes brand through unchanged and doesn't produce a brand_manifest field — documenting the "no conversion" contract since neither schema has that field.

@nastassiafulconis nastassiafulconis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All 5 comments addressed in ca7fc3b. Looks good to merge.

…lization, and test coverage

- brand?.domain guard prevents "https://undefined" in adaptCreateMediaBuyRequestForV2
- preserve brand in output when it has no domain (consistent with adaptGetProductsRequestForV2)
- remove update_media_buy from brand_manifest normalization (neither v2/v3 schema has brand)
- add mock-based test verifying brand_manifest is not forwarded as an object to the server
- add tests for brand-with-no-domain case and adaptUpdateMediaBuyRequestForV2 brand semantics
@EmmaLouise2018 EmmaLouise2018 force-pushed the EmmaLouise2018/brand-manifest-schema-fix branch from ca7fc3b to 022c840 Compare February 26, 2026 22:39
@EmmaLouise2018 EmmaLouise2018 merged commit fdf23d1 into main Feb 26, 2026
8 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.

3 participants