Skip to content

fix(compat): warn when brand_manifest non-standard path is silently flattened to domain#686

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-683-brand-manifest-path-warning
May 12, 2026
Merged

fix(compat): warn when brand_manifest non-standard path is silently flattened to domain#686
bokelley merged 1 commit into
mainfrom
claude/issue-683-brand-manifest-path-warning

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #683

Summary

After PR #679, the v2.5→v3 adapter correctly extracts the hostname from brand_manifest URLs, but silently drops any non-standard path. A buyer hosting at https://cdn.acmecorp.com/brand-manifest.json gets translated to {domain: "cdn.acmecorp.com"}, and the v3 seller then fetches https://cdn.acmecorp.com/.well-known/brand.json — which typically 404s with no diagnostic signal in the SDK.

This PR adds a logging.WARNING at both adapter call sites (get_products.adapt_request and _media_buy_helpers.adapt_brand_manifest_to_brand) when the input URL carries a non-standard path. No behavior change; pure observability improvement.

Design decisions:

  • Emitted at call sites, not in extract_brand_domain — the utility is stateless string transformation; diagnostic responsibility belongs to the callers that have request context.
  • Per-URL dedup via module-level set — prevents log saturation for high-RPS v2.5 buyers sending the same manifest URL on every request.
  • Bare-domain guard (parsed.hostname is not None) — urlparse returns hostname=None for schemeless strings like "acme.com", which would otherwise false-positive since urlparse("acme.com").path == "acme.com".
  • Both original URL and extracted domain in message — operator needs to see what the v3 seller will actually fetch.

What was tested

  • pytest tests/test_legacy_adapter_v2_5_get_products.py tests/test_legacy_adapter_v2_5_media_buy.py -v52 passed
  • ruff check src/adcp/compat/ — no issues
  • mypy src/adcp/compat/ — no issues in compat files (pre-existing pydantic stubs missing in env; unrelated to this change)

New tests cover: CDN URL warns, standard /.well-known/brand.json does not warn, bare domain does not warn, same URL warns exactly once (dedup).

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

  • extract_brand_domain (in _url.py) also calls urlparse internally, so both call sites do a second urlparse to read .hostname and .path. Harmless but redundant — refactoring extract_brand_domain to accept/return a ParseResult would avoid the double call.
  • Dedup key is the raw (unstripped) manifest string; urlparse is called on manifest.strip(). A manifest string with surrounding whitespace would escape dedup. Pathological input, no real-world impact.
  • Warning logic is duplicated verbatim between get_products.py and _media_buy_helpers.py. A _warn_if_nonstandard_brand_manifest_path(manifest, domain) helper in _media_buy_helpers.py would unify them (dx-expert's main nit).

Pre-PR review

  • code-reviewer: approved — no blockers; double urlparse call and dedup-key normalization noted as nits
  • dx-expert: approved — merge-ready; code duplication between call sites is the main nit (15-line extraction, not a blocker)

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_01X44xueoHmfHRYeKL8giv2F


Generated by Claude Code

…to domain

Resolves silent information loss introduced by the v2.5→v3 adapter: a
brand_manifest URL like https://cdn.acmecorp.com/brand-manifest.json is
correctly translated to BrandReference.domain but the path is dropped,
so v3 sellers will attempt https://cdn.acmecorp.com/.well-known/brand.json
which typically 404s with no signal to the operator.

Both adapter call sites (get_products.adapt_request and
_media_buy_helpers.adapt_brand_manifest_to_brand) now emit a
logging.WARNING when urlparse detects a non-standard path. The warning
is deduplicated per URL via a module-level set so high-RPS buyers do not
saturate log pipelines. Bare-domain inputs (no scheme, urlparse returns
hostname=None) are guarded against false positives.

Fixes #683.

https://claude.ai/code/session_01X44xueoHmfHRYeKL8giv2F
@bokelley bokelley force-pushed the claude/issue-683-brand-manifest-path-warning branch from 274fb93 to fdfdd1b Compare May 12, 2026 01:27
@bokelley bokelley marked this pull request as ready for review May 12, 2026 01:31
@bokelley
Copy link
Copy Markdown
Contributor Author

Independent expert review (post-triage) + rebase summary

code-reviewer (fresh): approved — no blockers. Confirmed warning threshold (parsed.path not in {"", "/", "/.well-known/brand.json"}) is correct, dedup logic correct. Real (non-blocking) concern: _brand_manifest_path_warned is an unbounded set — slow memory leak risk on a high-RPS server with cache-busted manifest URLs. Worth a follow-up; not a hard blocker since per-deployment cardinality of distinct brand_manifest URLs is typically small.

ad-tech-protocol-expert: approved. Confirmed:

  • v3 BrandReference is hostname-only by design (schemas/cache/3.0/core/brand-ref.json, additionalProperties: false); silent flattening is unavoidable.
  • warn is the right call for an adapter at the v2.5→v3 seam — raising would reject spec-valid v2.5 input and violate the adapter role.
  • Both call sites covered; no other URL→hostname patterns in the v2.5 compat layer.

Rebase notes:

Rebased onto current main (which now includes #685 / inline-object handling). The rebase took the union: string-branch keeps the warning; dict-branch handling from #685 is preserved unchanged.

Did not extend the warning to the inline-object's url field — that's the same information loss but is filed as follow-up #687 to keep this PR's scope contained.

CI: 15/15 green on the rebased branch. Merging.

@bokelley bokelley merged commit 0568d2e into main May 12, 2026
16 checks passed
@bokelley bokelley deleted the claude/issue-683-brand-manifest-path-warning branch May 12, 2026 01:31
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 with non-standard path is silently flattened to domain only

2 participants