feat(governance): reconcile delivery and plan adjustments - #6305
Conversation
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated schema paths modified without required approval.
This PR touches multiple files under static/schemas/source/** (gated paths), and review_decision is REVIEW_REQUIRED — not APPROVED. Per decision-table row 2, a gated-paths PR cannot auto-approve until a human/CODEOWNERS approval lands.
The reviewer found no Critical, High, or Medium findings: the AdCP 3.2 campaign-governance addition (new report_plan_adjustment task, attributed delivery reconciliation, gross_commitment/verified_net_cost accounting, expanded audit state) is coherent across schema and docs, the double-charge guard and adjustment attribution/authorization logic are correct, and the minor changeset is right because every governance schema is x-status: experimental. Extensive new test coverage is present. Only one Low note (delivery-metrics block ordering before the plan-existence guard, unreachable in practice) — left out of the gate.
Absent blocking findings this would approve, but the hard gated-paths gate requires explicit human/CODEOWNERS review before merge.
Escalation reasons:
- Gated files under
static/schemas/source/**modified/added (governance schemas + core x-entity-types + index) withreview_decision: REVIEW_REQUIRED— human/CODEOWNERS approval required before merge.
Why human review
- Gated paths touched (static/schemas/source/** — governance schemas, x-entity-types.json, index.json) and review_decision is REVIEW_REQUIRED, not APPROVED — hard approval gate requires human/CODEOWNERS review before merge.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/x-entity-types.json (modified) matches
static/schemas/source/**; static/schemas/source/governance/check-governance-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/check-governance-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/get-plan-audit-logs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/sync-plans-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/index.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.
Proposal review
VerdictThe tree delivers every claim in the description, and the design quality is high: the new two-action Named merits:
The Findings1. warning — exact-match comparison makes The spec defines disagreement implicitly as inequality: governance "compares period, cumulative spend, currency" and "If the records disagree, governance MUST: mark the governed action Failure narrative: a buyer's ad server measures $12,489.37 against the seller's $12,500 statement — a routine sub-1% discrepancy that is the norm in delivery measurement, not evidence of equivocation. Every Ask — any of: (a) define a materiality rule (e.g., plan-configurable 2. warning — breaking reshape of experimental surfaces without the migration note the experimental contract requires Two surfaces change incompatibly: the base Failure narrative: an existing delivery-phase seller upgrades and every delivery report is schema-rejected with no in-repo migration guidance. Ask — add a migration note (before/after payloads for both surfaces) in the changeset or a docs migration section, or state in the PR that the note and 6-week notice will ship with the 3.2 release notes. 3. warning — new normative surface has zero agent-observable compliance coverage Issue #6304's acceptance criteria: "Cover cancellation, partial decommitment, refund, credit, makegood, duplicate replay, conflicting replay, over-adjustment, and unauthorized reporter cases in conformance tests." The PR covers these well in training-agent unit tests (661 lines), but Ask — add storyboard scenarios (adjustment happy path + at least the over-adjustment and unauthorized-reporter rejections, and one dispute/closure probe in the delivery-monitor specialism), or declare the deferral and its tracking issue in the PR. 4. warning — the headline use case (cancellation before delivery) has an undocumented prerequisite "A decommitment also requires a canonical seller delivery statement" ( Ask — document the zero-statement pre-flight-cancellation flow with an example, or explicitly define the disposition when no statement exists. 5. note — "credible" in Schema: "Maximum credible seller or buyer amount while evidence is unresolved"; prose: "use the highest credible reported amount." If the rule is 6. note — seller correction path while a period is open is implicit Reconciliation is specified buyer-side ("The buyer submits a newer observation against the applicable canonical seller statement"), and closure prohibits new statements for a closed period — but whether a seller may issue a corrected statement (new 7. note — this PR decides all three questions #6304 explicitly reserved for the WG Separate task (yes), roles (seller reports / plan owner reviews), and gross-vs-net accounting (per-plan No blockers: the internal docs↔schema↔example↔test coherence is unusually good (digest math, schema conditionals, and x-entity wiring all check out). |
|
Thanks @lukasz-pubx — the digest-math verification and authority-table analysis are exactly the kind of independent review this surface needed. Surfacing these for @bokelley below, organized by what needs a design decision vs. what's a ready-to-apply text fix. Finding 1 — Dispute semantics for
|
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — hard approval gate (gated paths) unsatisfied, and a Breaking-class protocol change is present.
This PR modifies published protocol source under static/schemas/source/** (governance schemas, x-entity-types.json, index.json), which is under a hard, non-overridable approval gate. gated_paths: true and review_decision: REVIEW_REQUIRED (not APPROVED) → decision-table row 2 forces escalate. Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
Additionally, the reviewer notes the changeset's Migration section justifies "Breaking downgrades to Normative" for two x-status: experimental surfaces via a minor changeset. Per the repo's "Breaking-class changes are never auto-approved" hard gate, ratification is a human act — escalate unless review is APPROVED.
Escalation reasons
- Gated paths touched (static/schemas/source/** — governance schemas, x-entity-types.json, index.json) and review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS review required before merge.
- Breaking-class protocol change to experimental surfaces (per changeset Migration section) is never auto-approved; requires human ratification.
Findings
No critical/high/medium findings this pass. Delta is confined to documentation clarifications (conservative_exposure wording, corrected-statement paragraph, cancel-before-delivery prerequisite) and a Migration section — all internally consistent. The escalation is driven by the deterministic gate and the breaking-change governance gate, not by any actionable code defect.
Why human review
- Gated paths touched (static/schemas/source/** — governance schemas, x-entity-types.json, index.json) and review_decision is REVIEW_REQUIRED, not APPROVED — hard approval gate requires human/CODEOWNERS review before merge.
- Changeset Migration section documents Breaking-class changes to two x-status: experimental surfaces (Breaking downgrades to Normative); Breaking-class changes are never auto-approved and require human ratification unless review_decision is APPROVED.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/x-entity-types.json (modified) matches
static/schemas/source/**; static/schemas/source/governance/check-governance-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/check-governance-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/get-plan-audit-logs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/sync-plans-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/index.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 paths touched without an approving review.
This PR modifies/adds files under static/schemas/source/** (governance schemas, core/x-entity-types.json, index.json), which sit 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 and the outcome must be escalate.
The reviewer's delta pass surfaced no critical/high/medium findings — the only change since the prior escalate touches scripts/lint-storyboard-scoping.cjs, adding report_plan_adjustment to the exempt set, and its exemption rationale (plan_id unconditionally required) matches the new schema. The diff is mechanically clean, but a clean diff does not lift the gate.
Additionally, per the repo's "Breaking-class changes are never auto-approved" hard gate, the prior pass noted Breaking-class changes to x-status: experimental surfaces documented in the changeset Migration section. Ratification is a human act and requires review_decision: APPROVED.
Escalation reasons
- Gated paths (
static/schemas/source/**) touched andreview_decisionisREVIEW_REQUIRED, notAPPROVED— human/CODEOWNERS approval required before merge. - Breaking-class protocol changes require human ratification unless
review_decisionisAPPROVED.
Human/CODEOWNERS approval will lift the gate and allow this to fall through to a normal approve on the next pass.
Why human review
- Gated paths touched (static/schemas/source/** — governance schemas, core/x-entity-types.json, index.json) and review_decision is REVIEW_REQUIRED, not APPROVED — hard approval gate requires human/CODEOWNERS review before merge.
- Breaking-class changes to x-status: experimental surfaces (per changeset Migration section) are never auto-approved; ratification requires human approval unless review_decision is APPROVED.
- This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/x-entity-types.json (modified) matches
static/schemas/source/**; static/schemas/source/governance/check-governance-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/check-governance-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/get-plan-audit-logs-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-adjustment-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/report-plan-outcome-response.json (modified) matchesstatic/schemas/source/**; static/schemas/source/governance/sync-plans-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/index.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.
3bc374b to
d950ef0
Compare
…authority gaps Rewrite the add-governance-adjustments changeset migration section to name the correct schema (report-plan-outcome-request.json, not check-governance-response.json), correct the delivery_metrics required field count to 7, document the third breaking change (check_id and governance_context now required for outcome: "delivery"), and replace the unverifiable before/after doc-example claim with inline before/after snippets. Bring get-plan-audit-logs-response.json entries[].delivery out of its deprecated stub state to match the MDX description already in place, giving it the buyer observation's optional field shape without adding a required array (audit surfaces retain historical entries). Make the get_plan_audit_logs example internally consistent: a disputed/open governed action next to an accepted $25,000 decommitment read as a contradiction of the spec's own blocking rule. Sharpen the two same-named adjustments_verified fields (budget: dollars, makegoods contribute zero; summary: count, makegoods included) so the wire-compatible name carries an unambiguous unit and makegood treatment in both the schema and the docs table. Add normative statements the implementation already enforces but the spec didn't say: superseded-statement rejection, adjustment disputes as terminal records with a new-ID correction path, the cumulative-verified- adjustment cap, that "disputed governance period remains open" covers any reporting period, and that only the plan owner may set period_closed. Mirrored briefly into report_plan_adjustment.mdx and report_plan_outcome.mdx. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…overage Addresses verified review findings on the campaign-governance adjustment lifecycle (PR #6305): - Disputed adjustments no longer consume the cumulative-adjustment cap, so a seller can re-report a corrected amount after a buyer dispute. Accept-time also re-validates verified-sum <= commitment. - Delivery observations must bind to the canonical (latest-sequence) seller statement for the binding + reporting period, not whichever check the buyer happened to name. - Dispute-blocks-acceptance considers every open reporting period for a binding, not just the globally latest delivery outcome. - Period closure is a plan-owner-only authority; delivery observations may be reported by the plan owner or the original intent caller, including through the plan-resolution fallback. - Delivery observation_id dedup is scoped to the authenticated reporter (reporterCaller), not the plan owner. - Re-syncing a plan without accounting_mode preserves the existing plan's mode instead of silently resetting to gross_commitment. - Cap checks at commitment boundaries use an epsilon-tolerant comparison so exact-boundary floats aren't spuriously rejected. - The adjustment-review accept path builds the full response (including the throwable plan-summary computation) before mutating the ledger and the review idempotency cache. Test and compliance coverage: - Regression tests for all fixes plus a golden digest vector transcribed from the check_governance documentation example, digest-mismatch rejection, pre-flight cancellation, missing-statement decommitment review, statement sequence regression, and an accounting-mode-by-adjustment-type matrix. - governance-delivery-monitor storyboard phases for the adjustment lifecycle: report/accept happy path, over-adjustment rejection, unauthorized-reporter rejection, and dispute-blocks-acceptance with closed_unresolved unblocking. - Schema-validation vectors for report-plan-adjustment request/response conditionals and the delivery_metrics statement-digest requirement. - The changeset migration snippet uses a placeholder statement_digest instead of reusing the doc example's real digest against a different payload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- docs/building/by-layer/L1/security.mdx and docs/building/operating/transport-errors.mdx used HTML comments, which the MDX parser rejects; use MDX comments. - scripts/x-entity-field-map.json declared evidence_id twice after the governance-adjustment entries landed; JSON last-wins already resolved it to __scope_specific__, so drop the shadowed audience_evidence entry (evidence_id is scope-specific per the map's own comment). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up status on the findings in this thread, as of Applied earlier (in Applied at Still open — finding 1 (dispute semantics), proposed resolution for WG review: option (b) source-type split, hardened against manufactured variance by moving the protection into the math rather than the blocking state. |
|
Clean closure on findings 2–7. The option (b) hardened proposal for finding 1 is technically sound — a few notes that may help the WG evaluation: The ceiling math is incentive-compatible. Using The digest invariant closes the subtle equivocation path correctly. RFC 8785/JCS SHA-256 covers the full canonical payload including What implementing option (b) requires:
The Generated by Claude Code |
Resolves the remaining open review question on dispute semantics with the WG-proposed option (b) plus a conservative decommitment ceiling: - A forwarded seller_statement_copy whose statement ID or digest does not match the canonical statement is seller equivocation: disputed, and blocks adjustment acceptance while the period is open. - A forwarded copy whose identity matches but whose values differ is internally inconsistent (the digest covers period, spend, currency) and is rejected as VALIDATION_ERROR rather than recorded. - A buyer_measurement with matching period and currency but different cumulative_spend is expected measurement noise: recorded as the new non-blocking measurement_variance status with a warning finding. - A buyer_measurement whose period or currency differs is incomparable and remains disputed. - The verified-decommitment ceiling uses the conservative delivered figure, commitment - max(latest seller-stated, latest buyer-observed cumulative spend), so a seller understating its statement gains no decommitment room and a buyer inflating an observation only shrinks its own restorable headroom. Manufactured variance can freeze value, never free it. - Closing a period over either kind of disagreement records closed_unresolved; only disputed blocks acceptance. Outcome-response findings now retain their details payloads. Storyboard coverage converts the dispute probe to a forwarded-copy digest mismatch and adds a measurement-variance non-blocking phase; schema vectors cover the new enum value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Finding 1 is now resolved and implemented at |
Summary
report_plan_adjustmentbilateral: the seller reports immutable evidence and the authenticated plan owner accepts or disputes it before any economic or ledger effectgross_commitment(default) and opt-inverified_net_costaccounting modes while retaining original commitments for anti-fragmentation thresholdsWhy
The original adjustment design left an important trust question implicit: a seller could report one delivery amount to governance and another to the buyer, then attempt to use an adjustment to manufacture headroom. The protocol needs to preserve who asserted each fact, detect disagreement, and prevent either side's later report from silently overwriting the other.
Governance is an operational control plane, not a billing ledger. The governance ledger is authoritative for authorization and headroom; it is not independent proof of physical delivery, a refund, or the final payable amount. Seller delivery is a seller-attributed statement, buyer measurement is a buyer-attributed observation, and a commercial adjustment becomes verified only through seller report plus plan-owner review.
Behavior and impact
disputed, retain both values, and use the higher credible amount asconservative_exposureclosed_unresolved, freezes both parties' evidence for that period, and stops the historical mismatch from blocking later governance; it does not settle an invoice or assert billing truthopenandunmatched, without carrying the old buyer observation into the current-period summaryverified_net_costmode. Makegoods never restore cash headroomProposed WG resolutions
Issue #6304 carried
needs-wg-reviewon three structural questions. This PR proposes specific resolutions so ratification is explicit rather than incidental to merge:report_plan_adjustmenttask — yes; the bilateral report/review lifecycle is distinct enough fromreport_plan_outcometo warrant its own task surface.action: "report"to submit evidence; the authenticated plan owner usesaction: "review"to accept or dispute. Neither party is unilaterally authoritative.budget.accounting_modefield; default isgross_commitment(headroom restored only for decommitments); opt-inverified_net_costrestores headroom for refunds and credits as well.Validation
npm run test:schemas(28 schema checks plus deprecation metadata), including explicit period-close request/response casesnpm run test:storyboard-sample-request-schemanpm run test:examples(75 examples)npm run test:docs-navnpm run test:oneof-discriminatorsCloses #6304
Review follow-ups applied on-branch
A second review pass (schemas/docs read + adversarial implementation review + test-coverage audit) surfaced issues that are now fixed in three commits:
Reference implementation (
fix(governance): close adjustment lifecycle gaps...):CONFLICT.observation_idreplay/conflict detection is scoped to the authenticated reporter.accounting_modepreserves the existing mode; cap checks are epsilon-tolerant at float boundaries; the review accept path builds the full response before mutating the ledger and idempotency cache.Docs/schemas (
docs(governance): fix migration note...): the changeset migration note now names the correct schemas and required fields (including the previously omittedcheck_id/governance_contextbreak) with before/after payloads;get-plan-audit-logs-response.jsonentries[].deliveryis modeled instead of a deprecated stub; the audit-logs example no longer shows a verified adjustment coexisting with an open dispute; new normative sentences cover dispute terminality, canonical-statement binding, any-open-period blocking, and closure authority.Coverage:
governance-delivery-monitorstoryboards now probe the adjustment lifecycle (report→accept happy path, over-adjustment rejection, unauthorized reporter, dispute-blocks-acceptance withclosed_unresolvedunblocking); schema-validation vectors exercise thereport_plan_adjustmentrequest/response conditionals and thedelivery_metricsdigest requirement; unit tests add a golden digest vector transcribed from thecheck_governancedoc example, digest-mismatch rejection, pre-flight zero-spend cancellation, missing-statement decommitment review, sequence regression, and an accounting-mode-by-type matrix (755 tests green).Dispute semantics (resolved per WG option b, implemented at
306ad185): disagreement handling splits by evidence source. A forwardedseller_statement_copywhose statement ID or digest mismatches the canonical statement is seller equivocation —disputed, blocks adjustment acceptance while the period is open. A forwarded copy whose identity matches but whose values differ is internally inconsistent (the digest covers period, spend, currency) and rejects asVALIDATION_ERROR. Abuyer_measurementwith matching period/currency but differentcumulative_spendis recorded as the non-blockingmeasurement_variance; a structural mismatch (period or currency) staysdisputed. The verified-decommitment ceiling uses the conservative delivered figure —commitment − max(latest seller-stated, latest buyer-observed cumulative spend)— so a seller understating its statement gains no decommitment room and a buyer inflating an observation only shrinks its own restorable headroom: manufactured variance can freeze value, never free it. Covered by unit tests (763 green), a storyboard variance phase, and schema vectors for the new enum value.