feat(governance): reconcile delivery and plan adjustments - #6305
feat(governance): reconcile delivery and plan adjustments#6305bokelley wants to merge 6 commits into
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
|
…, 4, 5, 6 - Finding 4: document zero-spend delivery statement prerequisite for pre-flight decommitments in report_plan_adjustment.mdx - Finding 5: replace "highest credible" with explicit max() rule in specification.mdx and align conservative_exposure schema description - Finding 6: add seller correction path (higher-sequence statement) for open periods in specification.mdx - Finding 2: add Migration section to changeset per experimental-status contract (delivery object shape change + delivery_metrics required field expansion from 1 → 8), noting 3.2 beta as the notice vehicle Design decisions from Findings 1 and 3 (dispute semantics and storyboard coverage) remain open for @bokelley. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012TmnQ7bcUnvm8C1jDWQxFF
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.
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