Skip to content

fix(compat): handle inline BrandManifest object in v2.5 adapters#685

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-684-brand-manifest-inline-object
May 12, 2026
Merged

fix(compat): handle inline BrandManifest object in v2.5 adapters#685
bokelley merged 2 commits into
mainfrom
claude/issue-684-brand-manifest-inline-object

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #684

Summary

The v2.5 brand-manifest-ref.json schema defines brand_manifest as a oneOf — either a URL string or an inline BrandManifest object. Both adapters (get_products and create_media_buy) only handled the string branch. When a v2.5 buyer sent an inline object, the field was popped but brand was 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:

  • Inline object with url: extract hostname via existing extract_brand_domain(), set brand: {domain}. Guard uses url.strip() to prevent whitespace-only strings producing {"domain": ""}.
  • Inline object without url: silently omit brand and let v3 validation decide. The BrandManifest schema marks url as optional (only name is required), so {"name": "Great Value"} is spec-valid input that cannot yield a hostname — raising here would reject spec-valid payloads.
  • Caller-supplied brand wins over either form of brand_manifest (same precedence as the existing string branch).

update_media_buy intentionally does not translate brand_manifest and is unaffected.

What was tested

  • pytest tests/test_legacy_adapter_v2_5_get_products.py tests/test_legacy_adapter_v2_5_media_buy.py -v52 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 issues
  • mypy src/adcp/compat/legacy/v2_5/ --ignore-missing-imports — no issues

Nits surfaced by pre-PR review (not fixed in this PR)

  • Asymmetric required-field behavior (protocol expert): brand is required in v3 create_media_buy but optional in v3 get_products. When an inline object has no url, the adapter silently omits brand at both sites. For get_products this is genuinely safe; for create_media_buy the 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.
  • Module-level docstrings in create_media_buy.py and get_products.py still say "URL string" for brand_manifest; in-function comments were updated but the top-level module docstring wire-shape tables were not.
  • Mutation-safety test for the dict branch not added (the out = dict(payload) shallow copy is read-only for .get("url"), so no actual mutation risk, but coverage gap exists).

Pre-PR review

  • code-reviewer: approved — no blockers; whitespace-only url guard issue (blocker candidate) fixed before PR creation via url.strip(); nits noted above
  • ad-tech-protocol-expert: approved — non-breaking per spec; oneOf handling 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 above

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01Nz3E58qfSW4X6fdJngTxKt


Generated by Claude Code

claude added 2 commits May 12, 2026 01:10
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
@bokelley
Copy link
Copy Markdown
Contributor Author

Independent expert review (post-triage)

code-reviewer (fresh): approved — no blockers. Confirmed:

  • isinstance(url, str) and url.strip() guard correctly rejects None / non-strings / whitespace-only inputs
  • Silent-skip on missing url is the right call — preserves spec-correct behaviour of letting v3 validation own the missing-brand decision
  • Brand precedence preserved (caller-supplied brand wins regardless of manifest shape) and tested at both call sites
  • 8 new tests, symmetric across both adapters, behaviour-focused

Nit (non-blocking): the four-line dict branch is copy-pasted across get_products and _media_buy_helpers. Borderline extract candidate — would consolidate to _media_buy_helpers.adapt_brand_manifest_to_brand as the single implementation. Not worth blocking; can fold in if a third call site appears.

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.

@bokelley bokelley merged commit edd7d0a into main May 12, 2026
16 checks passed
@bokelley bokelley deleted the claude/issue-684-brand-manifest-inline-object branch May 12, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v2.5 brand_manifest as inline object (not URL) is unhandled by legacy adapter

2 participants