Skip to content

feat(reporting): add viewed-seconds distributions - #6792

Merged
bokelley merged 17 commits into
mainfrom
review-important-screenshot
Aug 26, 2026
Merged

feat(reporting): add viewed-seconds distributions#6792
bokelley merged 17 commits into
mainfrom
review-important-screenshot

Conversation

@bokelley

@bokelley bokelley commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add optional viewed_seconds_percentiles and viewed_seconds_histogram distributions under viewability
  • define shared measurable-population, nearest-rank percentile, half-open histogram, and histogram-combination semantics
  • expose both structured carriers as explicit requestable metric identities while keeping them nested, non-aggregatable, and unsortable
  • validate distribution structure and cross-value semantics before the reference seller mutates delivery state, then reconcile committed identities at package grain
  • compact bundled discovery-schema definition keys so the expanded metric vocabulary remains within the existing tools/list context budget
  • document the reporting contract and add a minor changeset for the current 3.2 beta line; this explicitly overrides the issue's earlier 3.3 deferral

Closes #6769

Validation

  • npm run test:schemas
  • npm run test:composed
  • npm run build:schemas
  • npm run test:mcp-schema-projection
  • npx vitest run server/tests/unit/product-discovery-schema-parity.test.ts --reporter=verbose
  • npx vitest run server/tests/unit/delivery-metrics-semantics.test.ts server/tests/unit/comply-test-controller.test.ts --reporter=verbose
  • node --test tests/metric-identity-coherence.test.cjs
  • git diff --check

@bokelley
bokelley enabled auto-merge (squash) August 23, 2026 13:10

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema paths require human/CODEOWNERS review.

This PR modifies five files under static/schemas/source/**, which fall under a hard, deterministic approval gate (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so per decision rule row 2 the gate is not satisfied and the outcome must be escalate.

The reviewer's assessment is clean: this is an additive reporting change adding two optional viewed-seconds distributions (percentiles, histogram) under viewability, exposed as unsortable structured metric identities. The changeset is correct (minor), schema↔docs coherence holds across all four doc/enum surfaces, structured identities are correctly excluded from sort-metric, no dist mutation, no new undiscriminated oneOf, and new tests cover the rejection paths. No blocking findings were surfaced.

Despite the clean diff, the deterministic path gate is non-overridable without a real approval. A human/CODEOWNERS reviewer must approve the schema-source changes before this can merge.

Escalation reasons

  • Touches gated schema-source files (delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) matching static/schemas/source/** while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required.

Why human review

  • Modifies gated files under static/schemas/source/** (delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2).
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

@aao-secretariat aao-secretariat Bot added the ladon/needs-human-review Ladon has escalated this PR for human review. label Aug 23, 2026
@bokelley
bokelley requested a review from rachitm022 August 23, 2026 13:25

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema paths require human/CODEOWNERS approval.

This PR adds optional, additive viewed-seconds percentile/histogram distributions to viewability reporting (minor changeset, Normative/additive). The reviewer found the schema, docs, semantic validation, and reconciliation coherent and thoroughly tested, with container subsumption correctly tightened to numeric leaves only. No Critical/High/Medium findings.

Escalation reasons

  • This PR modifies files under a hard, non-overridable approval gate (static/schemas/source/**): compliance/comply-test-controller-request.json, core/committed-metric.json, core/delivery-metric-aggregate.json, core/delivery-metrics.json, core/format.json, core/product-filters.json, core/reporting-webhook.json, enums/available-metric.json. The current review_decision is REVIEW_REQUIRED, not APPROVED (decision rule row 2). Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded.

Prior decision was also escalate for the same gated-paths reason. The diff remains clean with no actionable findings — the escalation persists solely because the deterministic approval gate is still unsatisfied.

Why human review

  • Modifies gated files under static/schemas/source/** (delivery-metrics.json, delivery-metric-aggregate.json, committed-metric.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json, comply-test-controller-request.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2).
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/compliance/comply-test-controller-request.json (modified) matches static/schemas/source/**; static/schemas/source/core/committed-metric.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metric-aggregate.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

…enshot

# Conflicts:
#	static/schemas/source/enums/available-metric.json

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema files require human/CODEOWNERS approval.

This PR modifies files under static/schemas/source/**, which are under a hard, non-overridable approval gate (gated_paths: true). The current GitHub review_decision is REVIEW_REQUIRED, not APPROVED, so decision-table row 2 fires: Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded.

No critical/high/medium findings were surfaced in this run. The reviewer confirmed the delta since the prior pass is largely a merge of main (#6621) into the branch — the changed hunks (view-threshold-basis, time_based_views schema/enum, banner script, contract test) already exist identically in the base SHA and are not novel here. The one genuine conflict-resolution site, the merged available-metric.json description, coherently combines this PR's viewed_seconds distribution language with main's time_based_views container language and stays consistent with the schema. No new schema/docs coherence, changeset, or oneOf-discriminator defect.

This is consistent with the prior escalate decision, which was driven by the same gated-paths gate rather than by any code defect. The gate lifts the moment a human/CODEOWNERS approval flips review_decision to APPROVED.

Escalation reasons

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2).

Why human review

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2). The gate lifts once review_decision becomes APPROVED.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/compliance/comply-test-controller-request.json (modified) matches static/schemas/source/**; static/schemas/source/core/committed-metric.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metric-aggregate.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

…enshot

# Conflicts:
#	static/schemas/source/core/delivery-metric-aggregate.json

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema paths require human/CODEOWNERS approval.

This PR modifies files under static/schemas/source/**, which sit behind a hard, deterministic approval gate (gated_paths: true). The current GitHub review decision is REVIEW_REQUIRED, not APPROVED, so decision-rule row 2 fires and Ladon cannot auto-approve regardless of how clean the diff is.

The reviewer reported a clean pass: the closed 5-key vendor-scope qualifier is added symmetrically across every hand-maintained metric copy, delivery-metric-aggregate's previously-empty vendor qualifier is fixed, container tokens are barred from scalar aggregate rows, vendor row uniqueness is re-keyed to (vendor, metric_id, qualifier), and a new parity test enforces the closed key set against canonical-metric-qualifier.json. All $ref targets resolve; schema, create_media_buy docs, and the compliance scenario agree; a changeset is present. No blocking or medium findings.

Because the underlying diff carries no critical/high/medium findings, the escalation is purely to obtain the required human/CODEOWNERS approval on the gated schema files. The gate lifts the moment review_decision becomes APPROVED, at which point this clean diff can fall through to approve.

Escalation reasons

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (row 2).

Why human review

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2). The gate lifts once review_decision becomes APPROVED.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/compliance/comply-test-controller-request.json (modified) matches static/schemas/source/**; static/schemas/source/core/committed-metric.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metric-aggregate.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

…enshot

# Conflicts:
#	static/schemas/source/core/reporting-webhook.json

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema paths require human/CODEOWNERS approval.

This PR modifies files under static/schemas/source/**, which are under a hard, deterministic approval gate (gated_paths: true), while review_decision is REVIEW_REQUIRED — not APPROVED. Decision rule row 2 forces escalation until a human/CODEOWNERS approval is recorded.

Escalation reasons

  • Gated files touched (review not yet APPROVED): static/schemas/source/compliance/comply-test-controller-request.json, static/schemas/source/core/committed-metric.json, static/schemas/source/core/delivery-metric-aggregate.json, static/schemas/source/core/delivery-metrics.json, static/schemas/source/core/format.json, static/schemas/source/core/product-filters.json, static/schemas/source/core/reporting-webhook.json, static/schemas/source/enums/available-metric.json. Human/CODEOWNERS approval required before merge.

The delta itself is clean: it adds an optional requested_metrics array to get_media_buy_delivery with matching docs table row, a "Requesting a metric subset" section, response/webhook parity updates, a contract test, and a changeset. Schema↔docs↔changeset↔tests are internally coherent; no Critical/High/Medium findings. The gate is purely path-based and lifts the moment review_decision becomes APPROVED.

Why human review

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2). The gate lifts once review_decision becomes APPROVED.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/compliance/comply-test-controller-request.json (modified) matches static/schemas/source/**; static/schemas/source/core/committed-metric.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metric-aggregate.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Escalate to human review

Escalate — gated schema paths require human/CODEOWNERS approval before merge.

The reviewer reports a clean pass: PR #6792 adds optional, addressable viewed-seconds percentile and histogram distributions under viewability. This is an additive Normative (non-breaking) change on the 3.2 beta line carrying a correct minor changeset. Schema↔docs coherence verified across available-metric.json, delivery-metrics.json (with dependencies/if-then population guards), committed-metric.json, format/product-filters/reporting-webhook subsumption prose, and delivery-metric-aggregate.json exclusion. No new undiscriminated oneOf; delivery-metric-aggregate.json uses allOf[$ref, not.enum] and is covered by tests/metric-identity-coherence.test.cjs. No Critical/High/Medium findings.

Despite the clean diff, this cannot auto-approve. The PR modifies files under static/schemas/source/**, which are under a hard, non-overridable approval gate (gated_paths: true), and the current GitHub review decision is REVIEW_REQUIRED, not APPROVED (decision rule row 2). This is enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

Escalation reasons

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge. The gate lifts once review_decision becomes APPROVED.

Note: this sustains the prior escalate purely on the deterministic gate, not on any new finding — the reviewer surfaced nothing actionable in this run.

Why human review

  • Modifies gated files under static/schemas/source/** (comply-test-controller-request.json, committed-metric.json, delivery-metric-aggregate.json, delivery-metrics.json, format.json, product-filters.json, reporting-webhook.json, available-metric.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge (decision rule row 2). The gate lifts once review_decision becomes APPROVED.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/compliance/comply-test-controller-request.json (modified) matches static/schemas/source/**; static/schemas/source/core/committed-metric.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metric-aggregate.json (modified) matches static/schemas/source/**; static/schemas/source/core/delivery-metrics.json (modified) matches static/schemas/source/**; static/schemas/source/core/format.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-filters.json (modified) matches static/schemas/source/**; static/schemas/source/core/reporting-webhook.json (modified) matches static/schemas/source/**; static/schemas/source/enums/available-metric.json (modified) matches static/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.

@damiendonnelly

Copy link
Copy Markdown

Issue author here. This delivers what #6769 asked for, and the places where it tightens the proposal are the right calls: pinning nearest-rank so two sellers cannot report different "p50"s from the same data, gating histogram combination on the exact boundary sequence plus vendor plus standard, and keeping the distributions out of container subsumption so a buyer opts into a non-aggregatable carrier explicitly.

On whether the normative text is buildable, which we offered to report on: the load-bearing sentence is that both carriers summarize "the per-impression in-view durations whose arithmetic mean is reported in viewed_seconds". That single phrase closes the one real implementation trap, computing percentiles over viewable-only impressions and silently inflating every value. Worth protecting through any future edit.

One clarification that could use a sentence in the docs: buckets are required to be ordered and non-overlapping but not contiguous. Together with the sum-equals-measurable_impressions rule, a gap between buckets is only legal when zero impressions fall in it. That is coherent, but it is currently implied rather than stated, and an independent validator author could reasonably enforce either contiguity or lossy gaps. Saying it explicitly would prevent both mistakes.

Since this lands as a minor on the 3.2 line, we will implement both carriers on our delivery reporting (first-party in-view duration to the MRC display threshold) once merged, and report back if anything in the text turns out ambiguous in practice.

The viewed_seconds_histogram spec required buckets to be ordered and
non-overlapping but did not explicitly state whether contiguity was
required. The sum-equals-measurable_impressions constraint makes gaps
implicit (a gap is only valid when no impressions fall within it), but
an independent validator author could reasonably enforce either strict
contiguity or permit lossy gaps.

This commit makes the rule explicit in three places:
- schema description: adds a sentence after the sum constraint
- verifier_constraints.viewed_seconds_histogram_bounds: notes that gaps
  are permitted only when zero impressions fall in the gap, enforced by
  the population rule rather than a separate contiguity check
- optimization-reporting.mdx: adds the same note to the viewability
  metric bullet

Prompted by implementer feedback on PR #6792.
@bokelley

Copy link
Copy Markdown
Contributor Author

Thanks for the careful read and the implementation commitment — and for flagging the load-bearing sentence on measurable-population scope. That's worth protecting and we'll keep an eye on it through edits.

The non-contiguity point is a real gap. I've pushed a follow-up commit (5a61036) that makes it explicit in three places:

  • Schema description (viewed_seconds_histogram): adds a sentence after the sum constraint — "Buckets need not be contiguous: a gap between consecutive bucket boundaries is permitted when no impressions fall within that range — the sum constraint enforces this implicitly, and validators MUST NOT independently require contiguity."
  • Verifier constraint (viewed_seconds_histogram_bounds): extends the constraint summary with "gaps between buckets are permitted only when zero impressions fall in the gap (enforced by the sum-equals-measurable_impressions rule, not a separate contiguity check)" so a validator author reading only the constraint block gets the same signal.
  • optimization-reporting.mdx: adds the equivalent note to the viewability metric bullet.

The text now says both mistakes out loud — don't enforce contiguity, and don't allow a gap to swallow impressions — so an independent validator author shouldn't need to infer either.


Generated by Claude Code

@aao-secretariat

Copy link
Copy Markdown
Contributor

Ladon cannot review this PR until merge conflicts are resolved.

…enshot

# Conflicts:
#	server/src/training-agent/task-handlers.ts
@bokelley
bokelley disabled auto-merge August 26, 2026 11:35
@bokelley
bokelley merged commit ba81c91 into main Aug 26, 2026
57 checks passed
@bokelley
bokelley deleted the review-important-screenshot branch August 26, 2026 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ladon/needs-human-review Ladon has escalated this PR for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optional duration distribution alongside viewability.viewed_seconds

3 participants