Skip to content

fix(media-buy): close vendor metric accountability loop - #6228

Open
bokelley wants to merge 2 commits into
mainfrom
fix-issue-3519
Open

fix(media-buy): close vendor metric accountability loop#6228
bokelley wants to merge 2 commits into
mainfrom
fix-issue-3519

Conversation

@bokelley

@bokelley bokelley commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • persist seller-stamped per-package metric commitments and expose them on readback
  • reconcile unified standard and vendor missing_metrics, including product capability fallback
  • isolate simulated vendor values and deferrals by package and make controller updates success-atomic
  • capability-gate and strengthen the vendor metric accountability storyboard

Why

Vendor metric commitments had no reliable in-band audit path. The reference seller also treated simulated vendor values as media-buy scoped, which could leak one package's value into another package carrying the same metric key.

This closes the accountability loop while preserving the unified missing_metrics schema introduced after the original issue proposal.

Impact

Buyers can read back the binding reporting contract and identify overdue standard or vendor metrics per package. Sellers that do not advertise media_buy.features.committed_metrics_supported are not evaluated by the optional conformance scenario.

Validation

  • npm run typecheck
  • affected server unit suites: 623 passing tests
  • npm run test:schemas: 27 passing validations
  • npm run build:schemas
  • npm run build:compliance
  • storyboard sample-request, response, and context-output lints
  • current and AdCP 3.0 compatibility storyboard matrices across all tenants

The repository pre-commit hook's full server suite exceeded its local 240-second timeout under workspace load; no assertion failure was reported. The scoped suites and all other gates above passed.

Closes #3519

@bokelley bokelley changed the title [codex] Close vendor metric accountability loop fix(media-buy): close vendor metric accountability loop Aug 5, 2026
@bokelley bokelley closed this Aug 5, 2026
@bokelley bokelley reopened this Aug 5, 2026
@bokelley
bokelley marked this pull request as ready for review August 5, 2026 11:36
Comment thread server/src/training-agent/task-handlers.ts

@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 path touched, human/CODEOWNERS review required.

Escalation reasons

  • static/schemas/source/compliance/comply-test-controller-request.json (modified) matches the hard approval gate static/schemas/source/**, and review_decision is REVIEW_REQUIRED (not APPROVED). Human/CODEOWNERS approval is required before this can merge.

Medium findings (non-blocking, for the human reviewer)

  • server/src/training-agent/task-handlers.ts:6502reporting_period reflects the requested window but delivery totals are still measured to now, creating a reporting-window inconsistency.

Context checked

  • Schema change is four additive optional fields on the compliance test-controller schema; reviewer found no schema↔docs drift, no oneOf discriminator regression, no changeset scope/type problem (changeset present), and no released-artifact mutation.
  • No critical/high findings. High-risk flag is a (modified) schema file; the sole medium finding is not on that modified file (row 5 not triggered) and is not in data-loss/schema/infra category (row 4 not triggered).

Decision table: row 1 no (no critical/high), row 2 fires (gated_paths: true AND review_decisionAPPROVED) → escalate.

Medium findings

  • server/src/training-agent/task-handlers.ts:6502 — reporting_period reflects requested window but delivery totals still measured to now

Why human review

  • Modifies gated path static/schemas/source/compliance/comply-test-controller-request.json (matches static/schemas/source/**) while 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/compliance/comply-test-controller-request.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 5, 2026

@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 path requires human/CODEOWNERS approval.

This PR modifies static/schemas/source/compliance/comply-test-controller-request.json, which matches the hard, non-overridable approval gate glob static/schemas/source/**. The gate is only satisfied when review_decision is exactly APPROVED; here it is unknown/none, so the gate is NOT satisfied and the outcome must be escalate (decision table row 2).

On the merits, this is a net improvement: the delta is a one-line targeted fix — elapsed pacing now measures to reportingEnd (req.end_date, defaulting to now) instead of wall-clock now, so computed delivery totals scale to the requested reporting cutoff. This resolves the prior medium finding (task-handlers.ts:6502 — reporting_period reflected the requested window while totals were paced to now). A new deterministic test binds the behavior and the default no-end_date path is unchanged. No new findings in this run.

Because the prior escalation was driven solely by the gated-path/review-status gate (not by an unresolved finding), and that gate condition still holds, escalation persists purely on the gate. Once a human/CODEOWNERS approval is recorded (review_decision: APPROVED), this clean diff falls through to a normal approve.

Escalation reason: Modifies gated path static/schemas/source/compliance/comply-test-controller-request.json (matches static/schemas/source/**) while review_decision is not APPROVED — human/CODEOWNERS approval required before merge.

Why human review

  • Modifies gated path static/schemas/source/compliance/comply-test-controller-request.json (matches static/schemas/source/**) while review_decision is not APPROVED — human/CODEOWNERS approval required before merge.
  • 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/**) 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.

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.

missing_vendor_metrics on by_package — vendor-metric audit symmetry

1 participant