feat(measurement): add measurement gateway feedback contract - #6245
feat(measurement): add measurement gateway feedback contract#6245bokelley wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths require human/CODEOWNERS review.
This PR modifies/adds published protocol surface under static/schemas/source/** (a deterministic hard approval gate) and review_decision is REVIEW_REQUIRED, not APPROVED. Per decision-table row 2, the gate is not satisfied and the outcome must be escalate.
The reviewer found no Critical/High/Medium findings: verified wire backwards-compatibility (request required set unchanged; response adds only optional fields; removed core fields were unwired and re-homed), schema↔docs coherence (all enum values resolve, baseline enum matches docs/training tool), and the discriminated oneOf audit is unaffected. The changeset is present and minor-scoped. Mechanically the diff looks clean, but this is a protocol wire-shape change on gated paths that adds new fields and a new experimental measurement_gateway block — a human/CODEOWNERS approval is required before merge.
Escalation reasons
- Touches gated paths
static/schemas/source/**(7 files, incl. new performance-feedback-metric.json and performance-baseline.json; modified performance-feedback.json, index.json, request/response, get-adcp-capabilities-response.json) while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required.
Why human review
- Gated paths touched (static/schemas/source/** — 7 files) and review_decision is REVIEW_REQUIRED, not APPROVED; human/CODEOWNERS approval required before merge.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/performance-feedback-metric.json (added) matches
static/schemas/source/**; static/schemas/source/core/performance-feedback.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/performance-baseline.json (added) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/provide-performance-feedback-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/provide-performance-feedback-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: Request changes
Request changes — 1 blocking finding.
Blocking findings
- static/schemas/source/core/performance-feedback.json:130 — Published core type
core/performance-feedback.jsonis redefined and loses 4 previously-required fields under aminorchangeset. Removing required fields is a Breaking-class wire change and takesmajorper the constitution; shipping it underminoris spec drift (high).
Medium findings
- specs/performance-feedback-contract.md:444 — Spec claims the training-seller response path is tested, but no test exercises the handler branch; a regression here would be silent.
Row 1 of the decision table fires: a high finding is present, so the outcome is request-changes regardless of the gated-paths / high-risk / Breaking-class gates that also apply here. Note that even once the high finding is resolved, this PR touches gated schema paths (gated_paths: true) with review_decision: REVIEW_REQUIRED and carries a Breaking-class protocol change — both of which require human/CODEOWNERS approval (escalate) rather than auto-approve, so a subsequent clean pass will not auto-merge without that review.
Fix: reclassify the changeset as major (Breaking-class ratification is a human act), or preserve the removed required fields to keep the change non-breaking. Add a test covering the training-seller response handler branch.
Blocking findings
- static/schemas/source/core/performance-feedback.json:130 — core/performance-feedback.json redefined and loses 4 required fields under a minor changeset (Breaking-class; requires major changeset)
Medium findings
- specs/performance-feedback-contract.md:444 — Spec claims training-seller response path is tested; no test exercises the handler
|
Implementer question on the account block, since this PR touches that table. We run a sales agent (sell.nofluffadvisory.com) declaring: account: { "require_operator_auth": false, "supported_billing": ["operator"], "sandbox": true }Accounts auto-provision on first use from the buyer's natural key (brand + operator), and The table pairs Separately, while this table is being edited: it lists |
EvgenyAndroid
left a comment
There was a problem hiding this comment.
Asked to review. The contract is the right shape and the receipt-vs-application split is the part worth having. Two things I'd fix before merge, and one of them also clears most of what Ladon is blocking on.
1. measurement.gateway is MUST-declared but doesn't exist in the registry.
get-adcp-capabilities-response.json says orchestrators "MUST include measurement in supported_protocols and measurement.gateway in experimental_features", but docs/reference/experimental-status.mdx isn't in the diff, so the id resolves to nothing. That file calls itself the canonical list, and graduation is defined as removing the id from it, which can't happen for a row that was never added. The last four PRs that minted a feature id registered it in the same PR (#5304, #5305, #5387, #6214). Nothing catches this in CI: experimental_features is validated by pattern only, so this PR's own new test asserts an unregistered id validates, and it does.
Two smaller things in the same area. enums/adcp-protocol.json still says agents implementing measurement must list measurement.core; this PR rewrote that clause in the response schema and in get_adcp_capabilities.mdx but not in the enum, so an orchestrator copying your own gateway example is conformant against one gated file and not the other in the same commit. And the measurement.core why-experimental cell still reads "vendor metric catalog discovery only" after the PR adds produces_performance_feedback and both method arrays to that block.
2. The receipt cluster ships without x-status, and marking it is also the cheapest way out of Ladon's blocker.
measurement and measurement_gateway both carry x-status: experimental. media_buy.performance_feedback doesn't, and neither do application_status, status_reason, or applied_at on the response. execution.trusted_match directly below it does. Per experimental-status.mdx there's no undeclared middle ground, and x-status doesn't inherit through $ref, so the request's new allOf picks up nothing from the capability blocks.
That matters because your own WG question 2 asks whether application_status should ship now or whether sellers should return only feedback_id until there's an async audit path. Unmarked, both ways of answering "no" later, removing the field or shrinking its enum, land in 4.0.
The useful part: if the new surface is marked experimental, most of the Breaking-class objection goes with it, since experimental surfaces can drop required fields inside 3.x under the six-week notice contract.
3. Two corrections on the blocking finding.
The field list is slightly off. applied_at was a property on main but never in required. The fourth field dropped from required is feedback_source, which survives as an optional property.
More usefully, the first suggested fix (reclassify to major) isn't reachable. versioning.mdx:136 bars removing fields without a prior deprecation release, :142 says a 3.x deprecation isn't removed until 4.0, and 4.0 is targeted early 2027 with an 18-month floor. The second option is the only one available now: keep feedback_id, status, and submitted_at on the core type as deprecated: true and non-required through 3.x.
4. The removals aren't disclosed anywhere.
§Compatibility has six bullets and none mentions them. The lead bullet, "Existing request fields and required fields are unchanged", is true of the request via the new allOf but reads as covering the core type, which lost four published properties. §Schema changes describes it only as "becomes the canonical assertion rather than a parallel stored-record model". release-notes.mdx already has a 3.2.0 section and #6214 shipped a migration page in its own feature PR; neither is here. An old→new mapping table in §Compatibility would cover most of it.
5. not_applied is the only disposition with no machine-readable companion.
The orchestrator has to consume it across N sellers to keep the cross-seller audit trail the spec assigns it, and skills/adcp-measurement/SKILL.md tells agents to inspect status_reason, which is a bare string with no code, no maxLength, no PII constraint. Everywhere else AdCP pairs a required $ref'd reason code with bounded free text: core/impairment.json, creative-status-changed-webhook.json, account-status-changed-webhook.json. Suggest enums/performance-feedback-reason-code.json as an open documentary enum plus maxLength: 500 on status_reason. Separately, applied_at's description is a prohibition ("Present only when application_status is applied") and nothing enforces it; the response schema has no if/then at all, where 85 of 690 source schemas use it.
6. methodology is described as producer-scoped but isn't bound to producer.
producer is optional and nothing conditions methodology on it. On the orchestrator→seller hop the authenticated caller is the orchestrator, and the spec explicitly allows per-seller redaction, so forwarding methodology with producer stripped is valid and the seller holds a string it can't scope. dependencies: { methodology: ["producer"] } fixes it, and core/frequency-cap.json already uses draft-07 dependencies with the MCP projection handling it. Worth doing now: optional→required is Breaking once released, and the field is absent from main and v3.1.10 today.
Three smaller ones:
supersedes_feedback_idhas no discovery bit and no acknowledgement. Your own training seller shows the gap:catalog-event-handlers.tsadds the field to the input schema, never reads it, and returnsapplication_status: 'applied'with a freshfeedback_id. Echoingsuperseded_feedback_idin the success arm plus asupports_supersessionbit alongsidereports_application_statuswould close it. Not asking to reopen the revision graph you ruled out.- The
sales-catalog-drivennarrative says "The seller must distinguish receipt from actual optimizer application", the spec says legacy sellers may keep returning onlysuccess, and the response schema scopes the MUST to sellers declaringreports_application_status. Three answers in one PR. The validations block is also unchanged, so none of the new fields is graded; #6218 shipped three vector sets and #5748 shipped two storyboards. evidence.confidence_intervalvalidates with nosample_size. Intentional?sample_size's "when the producer can disclose it" reads like a deliberate allowance, but neither §Evidence nor the task reference says so, and a CI on the performance_index scale isn't interpretable without n. One sentence either way would settle it.
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths without human approval.
This PR modifies the published protocol surface under static/schemas/source/** (7 files, including new performance-feedback-metric.json, performance-baseline.json, and modifications to performance-feedback.json, index.json, both provide-performance-feedback request/response schemas, and get-adcp-capabilities-response.json). gated_paths is true and review_decision is CHANGES_REQUESTED — not APPROVED — so the hard approval gate (decision rule row 2) is not satisfied and the outcome cannot be approve or plain comment.
Iteration credit: Both prior blocking findings are resolved. The [high] performance-feedback.json field-loss concern is addressed — feedback_id/status/submitted_at/applied_at are re-added as deprecated OPTIONAL properties on a standalone sender-side assertion type, a non-breaking deprecation appropriate for the minor changeset. The [medium] untested handler concern is addressed — training-agent.test.ts:1303 now exercises handleProvidePerformanceFeedback with an applied-receipt assertion. The current run surfaces only two [low] findings, no blocking or medium concerns.
The diff is mechanically clean, but the deterministic gated-path gate requires a real human/CODEOWNERS approval before merge. Human review of the schema changes is required to lift the gate.
Escalation reasons:
- Gated schema files under
static/schemas/source/**require human/CODEOWNERS review;review_decisionisCHANGES_REQUESTED, notAPPROVED.
Why human review
- Modifies gated protocol schema files under static/schemas/source/** (performance-feedback.json, index.json, provide-performance-feedback request/response, get-adcp-capabilities-response, plus new metric/baseline schemas); review_decision is CHANGES_REQUESTED, not APPROVED — human/CODEOWNERS review required to satisfy the hard approval gate.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/performance-feedback-metric.json (added) matches
static/schemas/source/**; static/schemas/source/core/performance-feedback.json (modified) matchesstatic/schemas/source/**; static/schemas/source/enums/performance-baseline.json (added) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/provide-performance-feedback-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/provide-performance-feedback-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 'CHANGES_REQUESTED', 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.
# Conflicts: # static/schemas/source/protocol/get-adcp-capabilities-response.json
|
Everything I raised is resolved, most of it more cleanly than I proposed. Splitting the compact assertion into One thing I didn't raise, because it wasn't in the PR when I reviewed it. 3743af5 removes That's the only thing I'd want settled before approving. The rest I'm satisfied with. Two items I'd take as separate issues rather than hold this PR: On WG question 2, now that Separately, thanks for the accounts answer — lazy provisioning under |
|
Two changes pushed in follow-up commit
Your blocking point is resolved. The two follow-up items ( Generated by Claude Code |
EvgenyAndroid
left a comment
There was a problem hiding this comment.
Approving. The body and WG question 4 now match what the PR actually implements, which was the only thing I had outstanding.
Recording the two follow-ups so they don't get lost: status_reason has no machine-readable companion, so a not_applied disposition can't be routed programmatically across sellers; and the sales-catalog-driven validations block is unchanged, so none of the new receipt fields is graded anywhere in the suite. Both are additive against an experimental surface, so neither needs to hold this.
|
My approve is in but it doesn't clear the gate — Needs a dismissal on review 4876414785 from someone with write access. |
|
@bokelley — EvgenyAndroid's read is correct. Ladon's CHANGES_REQUESTED review (ID 4876414785, submitted 2026-08-06T15:42:59Z) flagged field removals in Action needed: dismiss review 4876414785. Suggested dismissal note: "Finding resolved — After dismissal, the Generated by Claude Code |
# Conflicts: # package.json
Finding resolved — core/performance-feedback.json is untouched in the current diff; blocking concern no longer applies.
|
Review at the current head Two merge-specific verifications first, since the merge overlaps this PR's own files: main's new The tree delivers what the description promises, and the earlier round's asks are all genuinely in it — the registry rows, the Three findings I'd want considered, one before merge: 1. The PR silently deletes a ratified privacy MUST from Base description (
Head description:
The rule survives verbatim on sibling schemas ( Ask: restore the uniqueness/anti-correlation sentence (per receiving agent — it composes fine with the new retry sentence) and swap the example key for a non-semantic one; or state explicitly that the relaxation is intentional and why. 2. An Accounts-Protocol rule change rides undisclosed in a measurement changeset. The lazy-provisioning change (new MUSTs in Ask: add a sentence (or a second changeset) naming the lazy-provisioning rule change; splitting it into its own PR is the cleaner alternative but doesn't seem necessary at this stage. 3. The index-direction rule is MUST-shaped in the schema and SHOULD-shaped in the docs.
Under SHOULD, a producer that submits CPA as Ask: make the direction rule MUST for compact-contract producers (baseline present) and align the three copies (schema, task doc, spec md); or state why SHOULD is intended. Two notes, no action needed here: Overall: well-constructed. The schema-enforced capability gates, the untouched core stored-record type, and the closed docs↔schema gap go beyond what earlier review asked for, and |
Finding 1 — restore idempotency_key privacy MUST: the PR dropped the uniqueness/anti-correlation sentence introduced in #1622. Restored as "MUST be unique per receiving agent to prevent cross-agent correlation; use a fresh UUID v4 for each new assertion" in the schema description, the task doc required-fields table, and the two example keys in skills/adcp-measurement/SKILL.md and specs/performance-feedback-contract.md (swapped from the semantic study-scoped key to a UUID). Finding 2 — disclose accounts change in changeset: the lazy-provisioning clarification in docs/protocol/required-tasks.mdx, account-ref.json, and docs/accounts/overview.mdx was not mentioned in the changeset. Added a sentence naming the rule change. Finding 3 — align MUST/SHOULD on index-direction rule: performance-feedback- assertion.json used imperative prose; provide_performance_feedback.mdx used SHOULD. Aligned all four copies (schema description, task doc Baseline section, SKILL.md rule, spec Baseline section) to MUST scoped to compact-contract producers (baseline present).
|
All three findings addressed in follow-up commit Finding 1 — Finding 2 — changeset now names the accounts change. Added a sentence to Finding 3 — MUST/SHOULD mismatch resolved. All four copies aligned to MUST scoped to compact-contract producers (baseline present): schema Generated by Claude Code |
Adds a compact, backwards-compatible performance feedback assertion with explicit baseline, metric identity, provenance, evidence, and maturation semantics.
Introduces distinct experimental capabilities for measurement providers, buyer-orchestrator gateways, and sellers, using two fixed first-tier tasks (
get_media_buy_deliveryfor delivery access andprovide_performance_feedbackfor assertion submission). Webhook and offline paths are explicitly deferred until their registration, credential, payload, and receipt contracts exist.Updates docs, the new measurement skill, training-agent behavior, compliance, and the WG proposal so providers interact only with the orchestrator and the orchestrator controls seller fan-out.
Validated with focused schema and contract tests, 5,273 repository unit tests, typecheck, current and 3.0-compatible storyboard matrices, MCP projection, documentation checks, and compliance builds.