feat(media-buy): add identifier-based place targeting - #6093
Conversation
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths require human/CODEOWNERS approval and review is not yet APPROVED.
Verdict: This PR touches the published protocol surface under static/schemas/source/**, which is under a hard, deterministic approval gate (gated_paths: true), and review_decision is REVIEW_REQUIRED (not APPROVED). Per decision table row 2, this must escalate.
The change itself looks sound: an additive minor introducing identifier-based place targeting (new geo_places/geo_places_exclude overlays, capability declaration, get_products filters, a resolver contract, PLACE_TARGET_UNAVAILABLE, and compliance coverage). Schema↔docs coherence holds across targeting/get_products/capabilities/error-code surfaces, no undiscriminated oneOf, dist/** untouched, changeset is a correct minor for a non-breaking addition, and draft-07-inexpressible constraints are documented via x-adcp-validation and enforced in tests. No critical/high findings.
Medium findings (1, non-blocking on their own):
static/compliance/source/protocols/media-buy/scenarios/geo_place_targeting.yaml:567— Compliance overlap probe only exercises same-version include/exclude, never the cross-version case the spec singles out.
Escalation reason: Human/CODEOWNERS approval is required before merge because the PR modifies gated protocol-schema files under static/schemas/source/** and review_decision is not APPROVED.
Medium findings
- static/compliance/source/protocols/media-buy/scenarios/geo_place_targeting.yaml:567 — Compliance overlap probe never exercises the cross-version include/exclude case the spec singles out
Why human review
- Gated paths: PR modifies protocol schema files under static/schemas/source/** (added: geo-place-*.json, geo-targeting-level.json; modified: product-filters.json, targeting.json, error-code.json, index.json, get-media-buys-response.json, get-adcp-capabilities-response.json) and review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/geo-place-area.json (added) matches
static/schemas/source/**; static/schemas/source/core/geo-place-catalog-capability.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-catalog-entry.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-resolver.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-support.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-system.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-type.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/get-geo-place-resolution-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/get-geo-place-resolution-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matchesstatic/schemas/source/**; static/schemas/source/core/targeting.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/geo-targeting-level.json (added) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/get-media-buys-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/protocol/get-adcp-capabilities-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths require human/CODEOWNERS approval.
This PR touches multiple files under static/schemas/source/**, which is a hard, deterministic approval gate (gated_paths: true). The current review_decision is not APPROVED (unknown/none), so the gate is NOT satisfied and per decision-table row 2 the outcome must be escalate. This is also a protocol wire-shape change (new geo-place schemas, new enum, targeting/product-filters/error-code modifications) — a Breaking/Normative-class change whose ratification is a human act.
The prior Medium finding (compliance overlap probe never exercising the cross-version include/exclude case) has been resolved: the delta adds the reject_cross_version_place_overlap probe exercising the same-place include-under-v0 / exclude-under-v1 → INVALID_REQUEST case, with matching context_outputs and requires_capability gating. No new findings in the current run — this is a clean net improvement.
However, the diff continues to touch gated schema paths and no human/CODEOWNERS approval is recorded, so the escalation persists on the deterministic gate alone (not as a "still has issues" judgment). A human/CODEOWNERS review is required before merge.
Escalation reasons
- Gated paths under
static/schemas/source/**(added geo-place-*.json, geo-targeting-level.json; modified product-filters.json, targeting.json, error-code.json, index.json, get-media-buys-response.json, get-adcp-capabilities-response.json) andreview_decisionis not APPROVED — human/CODEOWNERS approval required. - Breaking/Normative-class protocol wire-shape change — ratification is a human act.
Why human review
- Gated paths: PR modifies protocol schema files under static/schemas/source/** and review_decision is not APPROVED — human/CODEOWNERS approval required before merge.
- Breaking/Normative-class protocol wire-shape change (new geo-place schemas, new geo-targeting-level enum, targeting/product-filters/error-code changes) — ratification is a human act per the constitution hard gate.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/geo-place-area.json (added) matches
static/schemas/source/**; static/schemas/source/core/geo-place-catalog-capability.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-catalog-entry.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-resolver.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-support.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-system.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/geo-place-type.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/get-geo-place-resolution-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/get-geo-place-resolution-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matchesstatic/schemas/source/**; static/schemas/source/core/targeting.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/geo-targeting-level.json (added) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/get-media-buys-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/protocol/get-adcp-capabilities-response.json (modified) matchesstatic/schemas/source/**) and the current GitHub review decision is 'unknown', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
|
Strong support for the motivation here — structured 1.
|
|
All three concerns are well-founded. Here's the synthesis. 1. Forecast granularity gap — clarify intent, don't gate The asymmetry is real, but it follows the existing overlay model. Option (c) stated explicitly is the right call — not gating, which would couple independent problems and delay adoption with no benefit. One sentence in
Side issue surfaced: 2. Filter-not-fail vs. MUST-reject — fix before merge This is a real interop gap. The MUST-reject sentence in Two sentences needed in the
Plus one sentence on 3. Storyboard brief-mode gap — fix before merge Confirmed: @bokelley — actionable pre-merge items are (2) and (3). The normative statement in (1) is a clarity add on a deferral Generated by Claude Code |
- Clarify pricing/forecast granularity in targeting.mdx: firm pricing is unconditional on overlay granularity for geo_places, consistent with geo_metros and geo_postal_areas precedent - Add TARGETING_TOO_NARROW to error-code.json enum with description and enumMetadata (correctable recovery) — reconciles drift with create_media_buy.mdx - Add filter-not-fail rules to filters.places description in product-filters.json: unsupported system MUST yield no-match (not error); MUST-reject applies only to targeting_overlay.geo_places on spend-committing operations; system_version is informational at discovery time - Add brief-mode filters.places storyboard phase in geo_place_targeting.yaml: positive probe (covered place returns product) and negative probe (uncovered place returns empty — not error) to verify ANY/intersection semantics are observable by the compliance harness
|
Applied all four items from the pre-merge review in commit 3c78787: 1. 2.
3.
4. Generated by Claude Code |
|
Ladon cannot review this PR until merge conflicts are resolved. |
- Clarify pricing/forecast granularity in targeting.mdx: firm pricing is unconditional on overlay granularity for geo_places, consistent with geo_metros and geo_postal_areas precedent - Add TARGETING_TOO_NARROW to error-code.json enum with description and enumMetadata (correctable recovery) — reconciles drift with create_media_buy.mdx - Add filter-not-fail rules to filters.places description in product-filters.json: unsupported system MUST yield no-match (not error); MUST-reject applies only to targeting_overlay.geo_places on spend-committing operations; system_version is informational at discovery time - Add brief-mode filters.places storyboard phase in geo_place_targeting.yaml: positive probe (covered place returns product) and negative probe (uncovered place returns empty — not error) to verify ANY/intersection semantics are observable by the compliance harness
0d0bc72 to
295f578
Compare
# Conflicts: # docs/media-buy/task-reference/create_media_buy.mdx # docs/media-buy/task-reference/get_products.mdx # docs/protocol/get_adcp_capabilities.mdx # package.json # skills/adcp-media-buy/SKILL.md # static/schemas/source/core/creative-assignment.json # static/schemas/source/core/product.json # static/schemas/source/media-buy/get-products-request.json
…o exec/pr-6093 # Conflicts: # docs/media-buy/task-reference/create_media_buy.mdx # docs/media-buy/task-reference/get_products.mdx # skills/adcp-media-buy/SKILL.md # static/schemas/source/core/creative-assignment.json # static/schemas/source/media-buy/get-products-request.json
What changed
geo_placesandgeo_places_excludetargeting using stable IDs, explicit countries, identifier systems, catalog versions, and place types.get_productsplace coverage and capability filters so buyers can discover support before creating a buy.PLACE_TARGET_UNAVAILABLEfor correctable catalog-rollover failures.Why
Raw city names are ambiguous, localized, and unstable across platforms. Buyers need a portable way to express named-place intent without silently targeting the wrong Bergen, Springfield, municipality, borough, or neighborhood.
The capability shape uses a country-keyed type map to avoid Cartesian false positives. Registered numeric systems have exact semantics, while owner-controlled HTTPS URIs provide collision-safe extensions for private catalogs and types.
Review
Independent product, protocol-interoperability, and JSON Schema experts reviewed the implementation. Their findings on resolver disambiguation, cross-version overlap, exclusion-only behavior, catalog rollover, deterministic compliance, and error recovery were addressed; all three report no remaining blockers.
Validation
Closes #5588.