fix(compat): handle inline BrandManifest object in v2.5 adapters#685
Merged
Conversation
Both get_products and create_media_buy adapters only handled the URL string branch of v2.5 brand_manifest. The spec defines brand_manifest as a oneOf (URL string OR inline BrandManifest object), so inline objects were silently dropped — brand was never set, causing confusing downstream validation failures. Add elif isinstance(brand_manifest, dict) branch at both sites: extract domain from the object's url field when present; when url is absent (spec-valid per brand-manifest.json which only requires name), brand is omitted and v3 validation decides. Adds 8 new tests covering the dict happy path, no-url pass-through, url-with-path, and brand-wins cases. https://claude.ai/code/session_01Nz3E58qfSW4X6fdJngTxKt
…ranch
url.strip() prevents a whitespace-only url (e.g. " ") from passing the
truthiness check and producing {"domain": ""} via extract_brand_domain,
which would hit a noisy v3 validation error instead of the clean silent-skip
the no-url branch provides.
https://claude.ai/code/session_01Nz3E58qfSW4X6fdJngTxKt
Contributor
Author
Independent expert review (post-triage)code-reviewer (fresh): approved — no blockers. Confirmed:
Nit (non-blocking): the four-line dict branch is copy-pasted across CI: 15/15 green. Merging. Coordination note: PR #686 (issue #683) is concurrently in flight on the same function. Will rebase #686 on top of this and extend the warning to the inline-object branch for coherence. |
This was referenced May 12, 2026
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.
Closes #684
Summary
The v2.5
brand-manifest-ref.jsonschema definesbrand_manifestas aoneOf— either a URL string or an inlineBrandManifestobject. Both adapters (get_productsandcreate_media_buy) only handled the string branch. When a v2.5 buyer sent an inline object, the field was popped butbrandwas never set — causing either a silent no-brand request or a confusing v3 validation failure downstream.This PR adds the dict branch at both call sites:
url: extract hostname via existingextract_brand_domain(), setbrand: {domain}. Guard usesurl.strip()to prevent whitespace-only strings producing{"domain": ""}.url: silently omitbrandand let v3 validation decide. TheBrandManifestschema marksurlas optional (onlynameis required), so{"name": "Great Value"}is spec-valid input that cannot yield a hostname — raising here would reject spec-valid payloads.brandwins over either form ofbrand_manifest(same precedence as the existing string branch).update_media_buyintentionally does not translatebrand_manifestand is unaffected.What was tested
pytest tests/test_legacy_adapter_v2_5_get_products.py tests/test_legacy_adapter_v2_5_media_buy.py -v— 52 passed (8 new tests: inline-with-url, url-with-path, no-url-skips-brand, brand-wins-over-inline, at both call sites)ruff check src/adcp/compat/legacy/v2_5/— no issuesmypy src/adcp/compat/legacy/v2_5/ --ignore-missing-imports— no issuesNits surfaced by pre-PR review (not fixed in this PR)
brandis required in v3create_media_buybut optional in v3get_products. When an inline object has nourl, the adapter silently omitsbrandat both sites. Forget_productsthis is genuinely safe; forcreate_media_buythe request will still fail downstream with a v3 validation error. This is a deliberate "let v3 validate" posture — see discussion in v2.5 brand_manifest as inline object (not URL) is unhandled by legacy adapter #684 — but is worth noting for reviewers.create_media_buy.pyandget_products.pystill say "URL string" forbrand_manifest; in-function comments were updated but the top-level module docstring wire-shape tables were not.out = dict(payload)shallow copy is read-only for.get("url"), so no actual mutation risk, but coverage gap exists).Pre-PR review
urlguard issue (blocker candidate) fixed before PR creation viaurl.strip(); nits noted aboveoneOfhandling correct; silent-skip for no-url is the only defensible behavior for an adapter that cannot fabricate a hostname; nit on asymmetric required-field behavior between call sites noted aboveSession: https://claude.ai/code/session_01Nz3E58qfSW4X6fdJngTxKt
Generated by Claude Code