[codex] Add dimensional forecast point fields#5061
Conversation
There was a problem hiding this comment.
LGTM. Additive optional fields on ForecastPoint, minor changeset is the right call. Follow-ups below.
Things I checked
forecast-vendor-metric-value.jsonis a clean ForecastRange-swapped twin ofcore/vendor-metric-value.json— same envelope, same required set[vendor, metric_id, value], sameadditionalProperties: false. Onlyvalueandmeasurable_impressionschange shape.forecast-point.json:48-95viewabilitymirrorsdelivery-metrics.json:284-318field-for-field with ForecastRange swaps.additionalProperties: truematches the delivery side.oneOfinforecast-point-dimensions.jsonis disjoint: each branch gated bykindconst +additionalProperties: false. Discriminator pattern thatscripts/audit-oneof.mjsaccepts — no baseline regression.kindvalues (geo,placement,device_type,device_platform,audience) line up 1:1 with the existingreporting_dimensionskeys inmedia-buy/get-media-buy-delivery-request.json:96,132,149,166,184. The doc's post-purchase reconciliation claim is load-bearing.- Buyer workflow mapping in
media-products.mdxis grounded:targeting_overlay.geo_countries/geo_regions/geo_metros/geo_postal_areas,audience_include, device overlay fields,creative_assignments[].placement_refsall exist on the schemas referenced. x-entity: "placement"registration is correct — compositepublisher_domain + placement_idnamespace, not product-scoped. Applied consistently inplacement-ref.jsonandplacement.json.viewable_ratelower bound inherited fromforecast-range.jsonminimum: 0; theallOfonly needs to addmaximum: 1. No drift.- Changeset is
minor. Base schema already hadadditionalProperties: true; new fields are all optional. Right type.
Follow-ups (non-blocking — file as issues)
systemis free-form on forecast geo rows.forecast-point-dimensions.json:21-24typessystemas bare"type": "string", but the reporting-side counterpart atmedia-buy/get-media-buy-delivery-request.json:105-114usesoneOf(metro-system, postal-system), andcore/targeting.json:50-52,98-101$refs those same enums. Forecast rows can emit asystemvalue buyers cannot reconcile against targeting/reporting. Tighten to the sameoneOffor parity.- Forecast viewability requires
standard; delivery viewability does not.forecast-point.json:87-95enforces "any of measurable_impressions / viewable_impressions / viewable_rate / viewed_seconds present →standardrequired."delivery-metrics.json:284-318carries no equivalent invariant. The stricter forecast-side rule is defensible (MRC vs GroupM rows are not interchangeable, as the doc says), but the asymmetry means a row that round-trips forecast → delivery loses the guarantee. Consider whether to backport the invariant to delivery-metrics. - The
if/thenrequiringsystemfor metro/postal_area is stricter here than at peer sites.forecast-point-dimensions.json:35-50enforces conditionalsystemrequirement. The same constraint is prose-only atget-media-buy-delivery-response.jsonby_geoandcore/product-filters.jsonrequired_geo_targeting. Either backfill the conditional there or note the asymmetry.
Minor nits (non-blocking)
- Empty
viewability: {}validates.forecast-point.json:87-95— thenot(anyOf required)branch accepts an empty object. Probably intended (same effect as omitting the field), but worth a one-line schema comment so a future reader doesn't read it as a bug. viewable_rate.allOf[1]lacks"type": "object".forecast-point.json:65-74— works in practice because the only value reaching that constraint is already an object, butdelivery-metrics.jsonstatesminimum: 0andmaximum: 1inline as one block. The split-across-allOf form is more clever than it needs to be.
Approving on the strength of the dimensional-row design (one product, many rows beats product-per-country fanout) plus the mirror discipline between forecast and delivery shapes. Safe to merge.
|
Non-blocking follow-up notes from review:
|
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape for what this is solving — dimensions as a kind-discriminated array lets one product expose placement×country availability without product-per-slice expansion, and the canonical-id parity with reporting dimensions and targeting overlays is intact.
Things I checked
forecast-point-dimensions.jsononeOfuseskindwithconstvalues across all five variants andrequired: ["kind", ...]on every branch. Classifies asdiscriminatedperscripts/audit-oneof.mjs:188-215— no baseline entry needed, and absent fromscripts/oneof-discriminators.baseline.json.- Canonical identifier parity:
geo_level/geo_code/system,device_type,device_platform,audience_id+audience_sourceall match the reporting-dimensions block inget-media-buy-delivery-response.jsonand the targeting overlay surfaces.placement_refmatchescreative_assignments[].placement_refs. viewabilitymirror parity vsdelivery-metrics.viewabilityatdelivery-metrics.json:284-318. New forecast version adds the ratchet atforecast-point.json:87-95requiringstandardwhenever any rate field is present — branch A (no rate field) and branch B (standard present) correctly cover the cases.viewable_rateallOfatforecast-point.json:64-76correctly boundslow/mid/highto[0, 1]via override of the baseforecast-range.jsonnumeric range.forecast-vendor-metric-value.jsonmirrorsvendor-metric-value.json— same required[vendor, metric_id, value], sameadditionalProperties: false,breakdownpassthrough preserved. Only delta isvalueandmeasurable_impressionsupgraded toForecastRange. Right call.- Geo
if-thenrequiringsystemwhengeo_levelismetroorpostal_areaworks givengeo_levelis required at top level. - Docs example at
media-products.mdx:1094-1166validates against the schemas. Both example points includestandard: "mrc"whenever a viewability rate field is emitted — the new ratchet is honored in the doc itself. - Buyer-workflow mapping at
media-products.mdx:1170-1176:creative_assignments[].placement_refsas creative-routing-only is consistent withcreative-assignment.json— placement narrowing happens at package targeting time, not at creative assignment. - Changeset
minoris correct: every new field is optional and additive. No required-field flips, no enum removals, no shape changes to existing surfaces. x-entity: placementannotation on bothplacement.json:16andplacement-ref.json:16follows the established pattern (annotate every site carrying the identifier, dedup is the lint's job).
Follow-ups (non-blocking — file as issues)
- Placement dimension shape diverges from delivery
by_placement.forecast-point-dimensions.json:65-72uses a structuredplacement_ref(publisher-scoped), butget-media-buy-delivery-response.jsonby_placementstill emits a bareplacement_idstring. Forecast shape is the better one — multi-publisher-safe and aligned withcreative_assignments[].placement_refs— but the asymmetry means buyer agents can't cross-walk forecast→delivery rows by identical key extraction. Worth a follow-up to upgradeby_placement.placement_idtoplacement_ref, or an explicit note that the asymmetry is deliberate during transition. standard-required ratchet asymmetric vs delivery-metrics.forecast-point.viewabilityrequiresstandardwhen any rate field is present (forecast-point.json:87-95);delivery-metrics.viewabilitykeepsstandardoptional unconditionally (delivery-metrics.json:284-318). Reasonable to enforce up-front at forecast time and rely on the seller carrying the same standard into delivery, but the inconsistency lets a buyer compare a forecasted MRC viewable_rate against an unlabeled delivered viewable_rate. Either tighten delivery-metrics in a separate change or document the divergence as intentional.uniqueItemsdoes not enforce per-kind uniqueness ondimensions.forecast-point-dimensions.json:6-8— two items with the samekindbut different codes (e.g.,geo:USandgeo:CAon one point) deep-compare as distinct and passuniqueItems. Two geos in one intersection is not a meaningful row. Either add a per-kind uniqueness constraint or document in the description that intersection items must each have a distinctkind.audience_idselectability constraint is only in prose. The docs at line 1173 correctly note that informational audience rows don't automatically become targeting handles, butforecast-point-dimensions.json:111-135doesn't carry that constraint into the schema description. Mirror the prose into the schema description so buyer agents reading the schema in isolation don't try to liftkind: "audience"rows intoaudience_include.
Minor nits (non-blocking)
- Viewability
additionalProperties: trueplus openanyOfleaves a typo escape hatch.forecast-point.json:48-96— a payload with a typo likeviewablerate(missing underscore) would pass branch A of theanyOfbecause none of the spelled rate fields are present, sostandardis not enforced. Either tighten toadditionalProperties: falseon the viewability block, or accept that the open shape is intentional for forward-compatible vendor extensions and the typo case is a buyer-side conformance bug. - Empty
placement_idis allowed.placement-ref.json:13-17has nominLengthonplacement_id, so aplacement_ref: { placement_id: \"\" }validates. Pre-existing, not introduced here, but inherited by the new placement dimension variant. - Redundant
type: stringinside the geoif.forecast-point-dimensions.json:42-46— the outergeo_level$refalready constrains to the enum; the nestedtype: stringis harmless but redundant.
Approved.
|
Resolved the follow-up concerns in
Validation and pre-push checks passed; PR body has the full list. |
There was a problem hiding this comment.
Request changes. The dimensional ForecastPoint shape is the right design — array of intersection items with kind as the set key, geo if/then constraining system to the existing metro/postal enums, ForecastRange-shaped viewability and vendor metrics, product_id for proposal-level allocation mapping. That part is clean. The block is the bundled tightening of delivery-metrics.viewability shipped under a minor changeset.
static/schemas/source/core/delivery-metrics.json:318-331 adds an anyOf that requires standard whenever measurable_impressions, viewable_impressions, viewable_rate, or viewed_seconds is present. Published dist/schemas/3.0.12/core/delivery-metrics.json had standard as a plain optional property with no conformance gate. That is an optional→required flip on an existing public field. Per MUST FIX #5 and the playbook semver rules at lines 295-298, an optional→required flip on a published field is a major change. .changeset/5058-forecast-point-dimensions.md:2 declares minor; main is in pre mode so this ships as 3.1.0-beta.N, not 4.0.0. A 3.0-conformant seller emitting viewability: { measurable_impressions: N, viewable_rate: r } without standard becomes schema-invalid against 3.1.x without a major-version migration note. The same wire-break is duplicated in static/schemas/source/compliance/comply-test-controller-request.json:696-708 for simulate_delivery.params.viewability, so any existing harness fixture that omits standard will also break.
I agree that MRC and GroupM rows are materially different and standard SHOULD be present. The data-quality argument doesn't change the semver math.
Fix options
- Split the delivery-metrics tightening into its own
majorchangeset with a migration note. The forecast-point additions stayminor. This is the cleanest path. - Weaken to SHOULD in prose, drop the
anyOffromdelivery-metrics.jsonandcomply-test-controller-request.json. The new forecast-pointviewabilityanyOf atforecast-point.json:91-100is fine to keep — that's a new field, additive. Only the delivery-side tightening is the wire-break.
Either is acceptable. Pick one.
Things I checked
- Geo
if/thencomposes correctly under draft-07.not.required.systemcorrectly rejects{kind:"geo", geo_level:"country", geo_code:"US", system:"us_zip"}; the metro/postal branches' nestedsystem: $refenums layer on top of the variant-levelsystem: {type:string}so metro+us_ziprejects.forecast-point-dimensions.json:42-99. oneOfdiscriminator soundness — every variant declareskindasconstwithrequired:["kind"]and distinct values.scripts/audit-oneof.mjsconst-property path classifies asdiscriminated. No baseline ratchet needed.x-adcp-validation.unique_item_properties: ["kind"]atforecast-point-dimensions.json:6-9is docs-only — AJV won't enforce it. The buyer-sidedimensions sorted by kindrow-identity claim indocs/media-buy/product-discovery/media-products.mdx:984is therefore stronger than the wire enforces.- Docs/schema coherence on the array shape — the JSON examples at
media-products.mdx:1095-1167use thedimensions: [...]array form consistently. The earlier object form from the first commit is gone. product_idsemantics are clear atforecast-point.json:19-23andmedia-products.mdx:1178— populated when a proposal-level dimensional row maps to one allocation, omitted for aggregates spanning multiple products.x-entity: placementadded toplacement-ref.json:16,placement.json:16,x-entity-types.json:50. Composite identity documented in the type entry; matches thevendor_metricprecedent.
Follow-ups (non-blocking — file as issues)
- Test 9 in
tests/schema-validation.test.cjs:444-479is thin. It validates thex-adcp-validationannotation and exercises a hand-rolledhasRepeatedKindhelper. It does not compileforecast-point-dimensions.jsonwith AJV and exercise theif/then/not.required.systemcountry/region branches, the metro/postalsystemenum mismatch rejection, or the new viewabilityanyOfgate against valid/rejected fixtures. The PR description claims those AJV runs happened locally; the fixtures aren't in the repo. Add a fixture-based AJV pass so the conformance gates are CI-enforced. x-adcp-validation.unique_item_propertiesis unenforced at the wire. Either ship a custom AJV keyword that conformance tooling consumes, or update the docs to saykinduniqueness is buyer-side normalization, not a schema-enforced invariant. The current state has docs claiming MUST NOT and a wire that silently passes the violation.
Reblock comes off once the delivery-metrics + comply-test-controller tightening is either pulled out into its own major or weakened to SHOULD.
|
Addressed the semver block in
Validation, precommit, and pre-push docs checks passed. |
There was a problem hiding this comment.
Approving. Additive change with the right shape: optional new fields, properly-discriminated oneOf, structural enforcement on the new field where it matters, SHOULD-only on the legacy delivery side for 3.x compatibility. minor is the right semver bump.
Things I checked
- Additivity. No required-field flips, no enum value removal, no type changes on existing fields.
product_id,dimensions,viewability,vendor_metric_valuesare all optional new properties onForecastPoint.minormatches. - oneOf discriminator.
forecast-point-dimensions.json:14-183uses per-variantkind: constwith five distinct values — this is the const-property branchscripts/audit-oneof.mjs:190-215classifies asdiscriminated. No baseline entry needed;scripts/oneof-discriminators.baseline.jsoncorrectly has none. Matches theactivation-key.jsonprecedent. - Viewability gate asymmetry.
forecast-point.json:92-100is structurally stricter (anyOf forcesstandardwhen any measured value is present);delivery-metrics.json:286stays SHOULD-only. Right asymmetry — new field can enforce, legacy stays 3.x-compatible, andpackage.committed_metrics.qualifier.viewability_standard(core/package.json:106-108) is the existing contract-side disambiguator. - Geo dimension parity with existing vocabularies. Metro/postal system enums in
media-products.mdx:984matchenums/metro-system.jsonandenums/postal-system.jsonexactly. Geo dimension shape mirrors the existingby_geobreakdown rows atmedia-buy/get-media-buy-delivery-response.json:734-770, so cross-protocol mapping is consistent. - Forecast viewable_rate composition.
allOfofforecast-range.jsonplus per-propertymaximum:1atforecast-point.json:69-79— composes correctly. - Unique-by-
kindis non-structural and acknowledged. Draft-07 cannot express it;x-adcp-validation.unique_item_properties: ["kind"]plus normative MUST in the description is the right pattern and matchesget-products-response.json:301andget-signals-response.json:170precedent. Test asserts the rule via a JS helper (tests/schema-validation.test.cjs:533-557), not via AJV — fine, but make that obligation explicit downstream. - Changeset.
.changeset/5058-forecast-point-dimensions.mdexists, typeminor, prose describes the wire impact correctly.
Follow-ups (non-blocking — file as issues)
- Rename leaks. This PR retitles "Placement Targeting" → "Placement Routing" in
media-buys/index.mdx:147-159and rewires thecreative-assignment.jsondescriptions to sayplacement_refsdoes not narrow inventory. Four present-tense docs sites still describe the same field with the old framing:docs/media-buy/product-discovery/media-products.mdx:200("Omitting placement targeting")docs/media-buy/product-discovery/media-products.mdx:309— broken link: text "Placement Targeting" + anchor#creative-assignment-and-placement-targeting, but the heading is now "...Placement Routing". Pre-push Mintlify check passed per the PR body; worth re-confirming the slug resolves after rename.docs/media-buy/task-reference/create_media_buy.mdx:199docs/media-buy/task-reference/update_media_buy.mdx:241docs/creative/template-format-ids.mdx:592
Either sweep these in a follow-up or scope the rename to headings only.release-notes.mdx:1586describing historical 3.0 behavior can stay.
- x-entity coverage is partial. PR registers
placementand annotatesplacement-ref.json:16andplacement.json:16, but skips the three remainingplacement_idsites:core/placement-definition.json:8,media-buy/get-media-buy-delivery-response.json:884,tmp/context-match-request.json:52. Alsoscripts/x-entity-field-map.jsonwas not updated to mapplacement_id → placementfor future automated sweeps. Not a CI break; follow-up sweep. - Audience entity scope.
forecast-point-dimensions.json:166carriesx-entity: "audience", butx-entity-types.json:50describesaudienceas buyer-managed (CRM, lookalike seed, suppression) while forecast dimensions can beplatform,third_party,lookalike,retargeting, orunknownperaudience-source.json. Either broaden the entity description or split intoaudience_segment. - Row-identity recipe blurs parent vs point.
media-products.mdx:984andget_products.mdx:346list(forecast_range_unit, budget if present, product_id if present, dimensions sorted by kind).forecast_range_unitlives onDeliveryForecast(delivery-forecast.json:16), not onForecastPoint. Make the parent/child boundary explicit so implementers don't go looking for the field on the point. - Test coverage gaps. Untested cases worth adding:
viewable_rate.mid > 1rejection (the newmaximum:1allOf atforecast-point.json:69-79); region-levelgeo_codepattern (^[A-Z]{2}-[A-Z0-9]{1,3}$); region withsystempopulated (thenot:{required:["system"]}gate); cross-kind contamination viaadditionalProperties:false(e.g.,{kind:"geo", placement_name:"foo"}); device_type / device_platform / audience branch validity.
Minor nits (non-blocking)
- Long paragraph in
media-products.mdx:984. 15+ lines covering metro/postal systems, intersection semantics, normalization key, and the no-repeat rule. Splittinggeo_level→ system into a sub-bullet list would help scanability. - Region
geo_codedescription.forecast-point-dimensions.json:34says "ISO 3166-2 with country prefix" but the pattern is^[A-Z]{2}-[A-Z0-9]{1,3}$. Worth mentioning the 1-3 alphanumeric subdivision constraint inline. vendor_metric_value.breakdownis opaque.forecast-vendor-metric-value.json:30-33hasadditionalProperties:trueand the prose says sub-values SHOULD be ForecastRange; schema doesn't hint at this. Leaving it permissive is fine for vendor flexibility — flagging only so it's a known choice.
Ship it once the rename leaks are tracked. The schema work is the right shape.
Summary
Adds dimensional and measurement-aware forecast point fields for #5058.
ForecastPoint.dimensionsas an array of dimension constraints, so a single point can represent intersections such as placement x country, placement x device, or geo x audiencekindvariants for geo, placement, device type, device platform, and audience dimensionssystem, metro usesmetro-system, and postal usespostal-systemkindvalues a conformance error via normative schema/docs language plusx-adcp-validation.unique_item_properties: ["kind"]and test coverageviewabilityranges and forecastedvendor_metric_valuesranges independent ofpricing_optionsviewability.standardwhenever forecast viewability values are present; deliveryviewability.standardremains optional for 3.x compatibility and is documented as SHOULDForecastPoint.product_idso proposal-level dimensional rows can map back to executable product allocationsx-entitycoverage and a minor changesetValidation
npm run test:schemasnpm run test:examplesnpm run test:oneof-discriminatorsnpm run test:json-schema -- --file docs/media-buy/product-discovery/media-products.mdxnpm run test:json-schema -- --file docs/media-buy/task-reference/get_products.mdxnpm run test:json-schema -- --file docs/building/by-layer/L3/comply-test-controller.mdxtest:schemasfor forecast geo valid/rejected cases, forecast viewability standard requirement, and delivery/compliance 3.x compatibility withoutstandardnpm run build:schemasgit diff --checknpm run test:unit,npm run test:test-dynamic-imports,npm run test:callapi-state-change,npm run typecheckNotes
The array shape is preserved for readable intersections, while repeated
kindvalues are now a conformance error handled by normative docs and custom validation metadata because JSON Schema draft-07 cannot express unique-by-property for this array form. The delivery-side viewability schema tightening was removed to keep this PR minor-compatible; only the new forecast viewability field enforcesstandardstructurally.