Skip to content

feat(governance): add seller policy and change-right contracts - #6794

Open
bokelley wants to merge 26 commits into
mainfrom
seller-governance-policies
Open

feat(governance): add seller policy and change-right contracts#6794
bokelley wants to merge 26 commits into
mainfrom
seller-governance-policies

Conversation

@bokelley

@bokelley bokelley commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add structured seller acceptance-policy discovery backed by immutable, digest-pinned registry profiles and platform political-advertising policies
  • bind buyer change rights to accepted proposal terms, with fail-closed constraint enforcement and an explicit AdCP 3.1 projection
  • retain bounded seller disposition evidence in governance audit logs without granting sellers authority over buyer governance
  • advertise and enforce seller governance-agent acceptance with safe opaque rejection

Security and compatibility

  • accepted proposal terms are the authorization ceiling; product declarations may only narrow them
  • policy and profile versions are immutable; canonical digests and guarded migrations prevent same-version mutation
  • rejected governance-agent endpoints and credentials are never echoed, contacted, or persisted
  • audit error evidence is JSON-only and bounded by bytes, depth, and container size
  • wire versions use release precision (3.1); the released 3.1.19 artifact remains the compatibility validator

Validation

  • repository build, schema/compliance generation, and package build
  • 908 focused runtime and unit assertions
  • seller-governance contract suite
  • schema, JSON Schema, migration, registry-completeness, OpenAPI, and one-of audits
  • TypeScript typecheck and compliance storyboard linting
  • code, protocol, and security expert reviews

SDK follow-ups

Closes #6749
Closes #6750
Closes #6757
Closes #6758

Closes #6823

@lukasz-pubx

Copy link
Copy Markdown

Reviewed at da00e20 as author of #6757/#6758: both are implemented faithfully to their amended, triage-resolved states — vocabulary mapping, optional origin, bounded buyer-attributed audit copy, adcp.governance_enforcement placement, any_of matchers, GOVERNANCE_UNAVAILABLE for unresolvable verification, hygiene and non-discrimination MUSTs — and several hardenings go beyond what the RFCs asked (origin-only safe reflection, binding-pinned verification evidence, seller_policy_ref kept out of the registry entity space).

Two small items:

  1. Per rfc-process.mdx, the decision records for RFC: structured seller acceptance-policy discovery backed by the policy registry #6749 / RFC: bind buyer change rights into accepted proposal terms #6750 / RFC: typed seller dispositions in the campaign-governance audit record #6757 / RFC: seller acceptance criteria for campaign governance-agent bindings #6758 should be posted (the drafted memos look ready to ratify) before merge, and the final PR should carry Closes rather than Refs.
  2. A one-liner in sync_governance.mdx noting that disclosed rejections reflect origins while matchers may carry paths — so buyers should compare full matcher entries, not origins — would prevent a minor false-match misread.

Supportive of landing this.

…matchers may carry paths

Buyers reconciling a disclosed rejection against their any_of[] matcher
list should compare full matcher entries, not origins — an agent_url
matcher may include a path and therefore be narrower than the disclosed
origin, causing a false match on origin-only comparison.

Suggested by @lukasz-pubx in review of #6794.
@bokelley

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, @lukasz-pubx.

Item 2 — sync_governance.mdx clarifier: applied in bf70312. Added the following sentence after the attempted_agent_origin stripping note:

Buyers reconciling a disclosed rejection against their any_of[] matcher list should compare full matcher entries, not origins — an agent_url matcher may include a path and therefore be narrower than the disclosed origin.

Item 1 — decision records + Closes: both are author actions for @bokelley before merge. The decision memos for #6749 / #6750 / #6757 / #6758 need to be ratified and committed to governance/decisions/ per rfc-process.mdx (ratification is a human act on the WG memo). RefsCloses is likewise a call the author makes once they confirm each issue is fully resolved by this PR's scope.


Generated by Claude Code

@bokelley
bokelley marked this pull request as ready for review August 24, 2026 03:48
Comment thread static/schemas/source/governance/report-plan-outcome-request.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 paths touched without required approval.

This PR modifies protocol source under static/schemas/source/** (and .agents/**), which are under a hard, deterministic approval gate (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so the gate is NOT satisfied and the PR cannot be auto-approved (decision table row 2).

The reviewer findings are clean: the change relaxes error.code from required to optional on the newly-added, unreleased reported-outcome-error.json schema — a non-breaking loosening on an unreleased surface — and it is propagated coherently across schema, docs (report_plan_outcome.mdx), the governance handler, and both server and .cjs tests, with a minor changeset covering the additive surface. No critical/high/medium findings were reported.

Escalation reasons:

  • Touches gated protocol-source files (e.g. static/schemas/source/governance/reported-outcome-error.json, static/schemas/source/enums/error-code.json, and many others under static/schemas/source/**, plus .agents/sdk-shim-ledger.json) that require human/CODEOWNERS approval; review_decision is REVIEW_REQUIRED.

Once a human/CODEOWNERS approval sets review_decision to APPROVED, this PR falls through to a normal clean approve on the current findings.

Why human review

  • Gated paths touched (static/schemas/source/** and .agents/**) — hard approval gate not satisfied: review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS review required.
  • This PR touches a path under a hard, non-overridable approval gate (.agents/sdk-shim-ledger.json (modified) matches .agents/**; static/schemas/source/account/sync-governance-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-available-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-allowed-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product.json (modified) matches static/schemas/source/**; static/schemas/source/core/sla-window.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/enums/canonical-media-buy-action-mode.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/media-buy-action-mode.json (modified) matches static/schemas/source/**; static/schemas/source/error-details/governance-agent-not-accepted.json (added) matches static/schemas/source/**; static/schemas/source/error-details/policy-violation.json (modified) matches static/schemas/source/**; static/schemas/source/governance/accepted-governance-agents.json (added) matches static/schemas/source/**; static/schemas/source/governance/get-plan-audit-logs-response.json (modified) matches static/schemas/source/**; static/schemas/source/governance/policy-category-definition.json (modified) matches static/schemas/source/**; static/schemas/source/governance/policy-entry.json (modified) matches static/schemas/source/**; static/schemas/source/governance/report-plan-outcome-request.json (modified) matches static/schemas/source/**; static/schemas/source/governance/reported-outcome-error.json (added) matches static/schemas/source/**; static/schemas/source/index.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/acceptance-context.json (added) matches static/schemas/source/**; static/schemas/source/media-

…(truncated)…

@bokelley

Copy link
Copy Markdown
Contributor Author

Issue #6823 proposes a new docs/governance/sell-side-governance.mdx section covering spend-approval framing and asymmetric authority (the split-authority model, typed seller dispositions in audit trails, HITL as an implementation detail). Same governance surface as this PR — consider folding the doc section in before merge or confirm as a follow-up.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

WG ratification is now recorded for all four RFCs:

The decision records are committed in 6060673, formal decision comments are posted on each RFC, the needs-wg-review labels are removed, and the final PR references now use Closes as requested. The sync_governance matcher/origin clarification remains in place from bf70312.

@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 paths touched without required review approval.

This PR modifies files under hard approval gates (static/schemas/source/** and .agents/**) — the protocol wire-shape source of truth. gated_paths is true and review_decision is REVIEW_REQUIRED (not APPROVED), so decision-table row 2 forces escalation regardless of how clean the diff is.

The reviewer found no critical/high/medium findings this pass. The delta since the prior escalate adds only four well-formed governance decision records (DR-0015–DR-0018), whose factual claims check out against the schema/docs/SDK changes. This is a net-clean iteration, but the hard gate cannot be auto-satisfied by Ladon.

Additionally, this PR introduces new protocol schema fields, new error codes (GOVERNANCE_AGENT_NOT_ACCEPTED, GOVERNANCE_UNAVAILABLE), new media-buy acceptance-policy schemas, and new enum values — breaking/normative-class protocol surface changes whose ratification is an explicit human act per the constitution's breaking-class hard gate. Human/CODEOWNERS review is required before merge.

Escalation reasons

  • Gated paths touched (static/schemas/source/**, .agents/sdk-shim-ledger.json) — hard approval gate not satisfied: review_decision is REVIEW_REQUIRED, not APPROVED.
  • Protocol wire-shape changes (new schemas, fields, error codes, enum values) require human ratification per the breaking-class governance gate.

Why human review

  • Gated paths touched (static/schemas/source/** and .agents/sdk-shim-ledger.json) — hard approval gate not satisfied: review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval required.
  • Protocol wire-shape surface changed (new acceptance-policy schemas, new error codes, new enum values, new required/optional fields) — breaking-class ratification is a human act per constitution; never auto-approved absent APPROVED review.
  • This PR touches a path under a hard, non-overridable approval gate (.agents/sdk-shim-ledger.json (modified) matches .agents/**; static/schemas/source/account/sync-governance-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-available-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product-allowed-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product.json (modified) matches static/schemas/source/**; static/schemas/source/core/sla-window.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/enums/canonical-media-buy-action-mode.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/media-buy-action-mode.json (modified) matches static/schemas/source/**; static/schemas/source/error-details/governance-agent-not-accepted.json (added) matches static/schemas/source/**; static/schemas/source/error-details/policy-violation.json (modified) matches static/schemas/source/**; static/schemas/source/governance/accepted-governance-agents.json (added) matches static/schemas/source/**; static/schemas/source/governance/get-plan-audit-logs-response.json (modified) matches static/schemas/source/**; static/schemas/source/governance/policy-category-definition.json (modified) matches static/schemas/source/**; static/schemas/source/governance/policy-entry.json (modified) matches static/schemas/source/**; static/schemas/source/governance/report-plan-outcome-request

…(truncated)…

@bokelley

Copy link
Copy Markdown
Contributor Author

Addressed the #6823 documentation note in 27ff9e1. The new sell-side governance boundaries page makes the spend-approval asymmetry explicit and separates buyer authority, seller acceptance, negotiated change rights, and seller-internal review. It also documents the seller-disposition/audit and governance-agent dependency boundaries introduced by this PR. The PR now carries Closes #6823.

…icies

# Conflicts:
#	docs.json
#	server/src/training-agent/task-handlers.ts
#	server/src/training-agent/tenants/router.ts
#	server/src/training-agent/tenants/tenant-smoke.test.ts
#	tests/docs-nav-validation.test.cjs
Comment thread server/src/training-agent/tenants/router.ts Fixed

@KonstantinMirin KonstantinMirin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — PR #6794

Overview — This PR closes #6749/#6750/#6757/#6758/#6823, and it picked the right shape for all four RFCs. Facets landed on the generic policy-category-definition.json with a category-interpolating validator. change_terms went inside commercial_terms, so the existing terms_digest covers it. #6757 re-scopes three existing error codes and adds one optional origin rather than a new code. #6758's matcher union is a real discriminated any_of, with unresolvable verification routed to GOVERNANCE_UNAVAILABLE. The problem is reachability, not design: every surface this PR adds is gated on >= 3.2-beta.7, and the reference seller's highest negotiable release is 3.2-beta.6, so none of it is reachable on any transport — and the diff moved two previously wire-level tests off the wire to compensate. Three of #6749/#6750's own test-shaped acceptance criteria are unmet, and the page that closes #6823 has no inbound link. The design is sound; what follows is making it reachable and provable.

Should fix

1. The wire gate cannot open, and the proofs moved off the wire instead of the wire moving.

supportsSellerGovernanceDiscovery() returns true only for >= 3.2-beta.7 (server/src/training-agent/types.ts:64, a hardcoded prerelease >= 7). The highest release in SUPPORTED_RELEASE_VERSIONS is 3.2-beta.6 (task-handlers.ts:2633, tenants/router.ts:135), and resolveServedAdcpVersion rejects an unlisted prerelease outright — no caller can open the gate. That one predicate gates media_buy.acceptance_policy_discovery, adcp.governance_enforcement.accepted_governance_agents, and the sync_governance rejection itself (account-handlers.ts:1594). grep -rn '3\.2-beta\.7' server/src returns exactly one match: SELLER_GOVERNANCE_DISCOVERY_ADCP_VERSION at types.ts:29, which nothing reads. The threshold is written twice — once as a named constant, once as a magic 7 inside a character-for-character clone of supportsGetProductsRejected — so the two can only agree by coincidence, and they already disagree with the servable set.

The consequences are visible in the diff. Both new storyboards gate on capabilities the seller never emits, so the runner skips them not_applicable, which run-storyboards.ts:932 counts as clean. training-agent.test.ts converted two tests from simulateCallTool(server, 'get_adcp_capabilities', …) to direct handleGetAdcpCapabilities({}, { …, servedAdcpVersion: '3.2-beta.7' }) calls, and the sole assertion of #6758's credential-non-echo MUST calls handleSyncGovernance() in-process (account-handlers.test.ts:1073) while every sibling sync_governance test in that file goes through simulateCallTool. Meanwhile tenant-smoke.test.ts asserts on the real HTTP route that the capability blocks are undefined and that binding https://untrusted-governance.example/mcp returns status: 'synced' with the endpoint persisted. Those wire tests are correct — they document that the feature is off.

Root: a wire-gated feature with two writers for its checkpoint, and a checkpoint the server does not serve. Nothing in the build can observe that a gated capability is unreachable, because an unsatisfiable requires_capability is indistinguishable from one that is legitimately not applicable.

Fix: add 3.2-beta.7 to the release list and advance CURRENT_ADCP_VERSION / TRAINING_AGENT_CURRENT_ADCP_VERSION to it (the immutability of the published 3.2-beta.6 bundle is why a new checkpoint is needed, not a reason to ship an unreachable one) — or lower the gate to >= 6 if the checkpoint genuinely cannot move here. Either way, express both predicates as one-liners over a single atLeastAdcpVersion(served, minimum) comparator driven by the exported *_ADCP_VERSION constants, reusing the parseAdcpReleaseVersion / compareAdcpReleaseVersions pair this PR already extended at task-handlers.ts:2673. Then revert the two converted tests to simulateCallTool and drop the servedAdcpVersion: '3.2-beta.7' injections. If the checkpoint cannot move, the two scenarios must not be listed in requires_scenarios yet — registering a scenario whose gate can never open is a false green.

2. ~300 lines of new change-term enforcement sit in a branch that excludes exactly the state they handle, and the dead copy is fail-open.

task-handlers.ts:18479-18787 is guarded by !(acceptedCommercialTerms && Object.hasOwn(acceptedCommercialTerms, 'change_terms')) and then reads acceptedTerms.change_terms off the same object at :18485:

const acceptedCommercialTerms = mediaBuy?.acceptedProposal?.commercial_terms as unknown as Record<string, unknown> | undefined;
if (mediaBuy?.acceptedProposal && args.revision === mediaBuy.revision
    && !(acceptedCommercialTerms && Object.hasOwn(acceptedCommercialTerms, 'change_terms'))) {
  const acceptedTerms = mediaBuy.acceptedProposal.commercial_terms as unknown as Record<string, unknown>;
  const acceptedChangeTerms = Array.isArray(acceptedTerms.change_terms) ?  : [];

So acceptedChangeTerms is always [], changeTermByAction is always empty, permitsChange() always returns false, and budgetConstraintViolation() always returns undefined at its first guard. Every new call site (:18565, :18619, :18723, and the permitsChange guards at :18577, :18630, :18738, :18768) lies inside that branch. The REQUOTE_REQUIRED behavior the storyboard grades comes from the other implementation — rejectUnavailableActionenforceAcceptedChangeConstraint, invoked earlier in the same function at :18471.

The two copies have already diverged in the unsafe direction. enforceBudgetConstraint returns cannot_preflight when the current value is missing or a money constraint has a mismatched currency (:4384, :4348). The inline copy drops the bound to undefined — that is, allows — in both cases (:18502, :18506-18510). One rule, two evaluators, one of them permissive on unknown.

Root: the mutation path carries two parallel authorization systems. The correct one is already wired for control_media_buy; the PR copied the new rule inward and then added a guard that made the copy dead.

Fix: delete acceptedChangeTerms / changeTermByAction / permitsChange / budgetConstraintViolation and the permitsChange guards from handleControlMediaBuyUnlocked, leaving that block as the legacy no-terms envelope check it is guarded to be. Anything genuinely wanted from it becomes a case in enforceBudgetConstraint, with a control_media_buy step in change_rights_state_projection.yaml that reaches it.

3. The seller advertises the acceptance contract but implements no evaluator, so #6749's preflight loop is exercised by nobody.

acceptance_context: true is advertised at both capability sites (task-handlers.ts:15222, tenants/router.ts:852) and read nowhere — grep -rn acceptance_context server/src returns those two literals and one test type declaration, zero reads. The schema says that flag exists precisely to distinguish a seller that interprets the context "from a legacy seller that does not interpret it", and docs/protocol/get_adcp_capabilities.mdx:446 says true "confirms the seller evaluates the buyer's structured … facts". The reference seller makes exactly the claim the flag exists to deny it.

The conformance artifacts grade the declaration, not the evaluation. acceptance_policy_discovery.yaml:118 sends subject_facets: ["federal_election"], which is not a facet of political_advertising or of any other category (candidate_or_party, election_or_ballot_measure, issue_advocacy, legislation_or_regulation, government_information, political_merchandise, incidental_or_news_reference), so it matches no rule in the profile the very next assertion discovers. Nothing catches it — x-entity: governance_policy_category_facet is only pattern-checked. The scenario's only product-side assertion is that a product echoes back its own seeded acceptance_policy_profile_ids.

Two of #6749's acceptance criteria fall out of this. "The schema can also encode one non-political conditional category" has no artifact. subject_category is political_advertising at all 13 occurrences across tests/, static/registry/, and the storyboards, and static/registry/policy-categories/pharmaceutical_advertising.json has no facets. "Conformance tests cover overlapping rules and the precedence of product-specific policy over seller defaults" has no artifact either. The restrictive-composition rule is normative in docs/media-buy/media-buys/policy-compliance.mdx, and no step and no test exercises it. Invert the precedence rule in any implementation and the whole suite stays green.

Fix: implement the resolver over the already-published catalog — it is a pure function of profile plus context — and grade that in acceptance_policy_discovery.yaml: a context selecting a prohibited rule must project differently from one selecting conditional, and a context matching no published rule must resolve to unknown, never allowed. Correct the facet to a registry value that actually selects a rule. Add facets to one non-political category with a conditional profile case in tests/seller-governance-policy-surfaces.test.cjs. Advertise and enforce are one decision: until the seller evaluates the context, do not advertise the flag — an absent capability already means unknown, which is the fail-closed reading the schema prescribes.

4. One capability contract, two writers — and the author built the right pattern one property away.

task-handlers.ts:15218 and tenants/router.ts:848 each hand-assemble the same four-field acceptance_policy_discovery object from the same three imported constants, with different gates: the handler adds ctx.tenantId === 'sales' || ctx.tenantId == null, the router has no null-tenant arm. The sibling value added in the same hunks does it right — TRAINING_ACCEPTED_GOVERNANCE_AGENTS (account-handlers.ts:48) is one exported constant consumed at both sites. The grading consequence is concrete: the byte-digest recomputation test (training-agent.test.ts:15563-15588) covers only the handler copy, while the router copy — the path that serves the sales tenant over HTTP — is asserted only negatively (tenant-smoke.test.ts:611, :634: toBeUndefined()).

The same shape repeats twice more in this diff. router.ts:135 keeps a byte-identical private copy of SUPPORTED_RELEASE_VERSIONS, so adding '3.1' took four synchronized edits. router.ts:441-471 hand-maintains the "is this scenario handled in-process" chain that comply-test-controller.ts:1205-1221 and :1370-1374 already compute; the two lists have drifted, and registering one scenario in this PR took four edits across two layers.

Root: projectTenantCapabilities re-parses an already-serialized MCP response buffer (router.ts:705-733) and rewrites capability content. That inversion predates this PR, but this PR made it a second author of a new contract instead of leaving it a pure selector. The missing abstraction is a capability model owned by the business layer; today the model exists only as literals inside two serializers.

Fix: export builders with declared return interfaces next to TRAINING_ACCEPTED_GOVERNANCE_AGENTSacceptancePolicyDiscoveryCapability(servedVersion, tenantId) and governanceEnforcementCapability(tasks, servedVersion) — each owning its own version and tenant gate, so both writers reduce to one spread and a future schema field is a compile error at both sites. Export the release list and import it in the router. Export the controller's local-scenario predicate. Extend the existing byte-digest assertion to the router projection so the served path is the pinned one.

5. static/schemas/source/** is the declared single writer of every wire contract, but nothing downstream derives from it — so each contract is re-authored by hand in TypeScript.

One disease, four surfaces:

  • Types. change-term.json is a closed object and change-term-constraints.json a discriminated union, yet the reference seller re-implements both as Record<string, unknown> + isRecord() probes across deriveAvailableActionsFromAcceptedChangeTerms, enforceAcceptedChangeConstraint, enforceBudgetConstraint, and proposalChangeTermsForPurchases. "Does this buy have negotiated change terms" is hand-rolled four times (:3936, :4247, :18478, :18486), each re-deriving the absent/non-array/present tri-state the fail-closed semantics depend on. MediaBuyProductAllowedActionState.constraints is Record<string, unknown> (types.ts:568); the action-mode union is spelled inline four times and duplicated across two value-identical schema enums, so adding seller_managed took six parallel edits. One regression makes this a defect rather than a style note. rejectUnavailableAction's return type was widened from { errors: TaskError[]; context?: unknown } | null to Record<string, unknown> | null (:4246), while all four sibling builders in the same diff keep the concrete type. It is then invoked for control_media_buy with args as unknown as UpdateMediaBuyArgs (:18471) — a cast the type system cannot check, and one that is false in general.
  • Tool schema and runtime validator. governance/reported-outcome-error.json is transcribed by hand into REPORTED_OUTCOME_ERROR_INPUT_SCHEMA (governance-handlers.ts:44-101) and re-implemented a third time imperatively in cloneReportedOutcomeError (:107-232). The MCP copy's internal $refs are absolute into the embedding document (#/properties/error/definitions/…), so it only resolves while spliced at exactly properties.error. The 16 KiB byte cap exists only in the imperative copy, so the published contract never states the bound the seller enforces. The new test compiles GOVERNANCE_TOOLS' own inputSchema and validates it against itself.
  • Repository and API model. deserializePublishedPolicy(row: any) (policies-db.ts:109-144) reconstructs the public Policy field-by-field out of untyped JSONB with ?? null defaults and synthesizes domain state the row does not carry (source_type: 'registry', review_status: 'approved'). acceptance_profile is Record<string, unknown> / z.record(...) (policies-db.ts:20, registry.ts:1039) although its shape is fully declared by this same PR in media-buy/acceptance-policy-profile.json and validated key-by-key by check-registry-completeness.cjs — and PolicySchema feeds the published OpenAPI, so consumers get additionalProperties: true for a closed, digest-pinned shape. issuer, added in the same hunk, gets a concrete type; the profile does not.
  • Field-name lists. COMPACT_PRODUCT_FIELDS (task-handlers.ts:6231) is a third hand-kept copy of the product-field vocabulary and names acceptance_policy_refs, a field that exists in no schema, no doc, no registry, and no test.

Root: no derivation step runs from the generated schema artifacts into the reference seller — not for types, not for MCP tool schemas, not for the database and API model. Hand-mirroring is therefore the path of least resistance, and this PR added four concepts at once. "The SDK snapshot lags the schema" is the reason, not a justification: types.ts already hosts MediaBuyAvailableActionState for exactly this purpose. Widening to Record<string, unknown> is the same missing-contract smell one notch up — it still gives callers no key or value checking.

Fix, one surface at a time:

  • Declare ChangeTerm, ChangeTermConstraints (discriminated on kind), ChangeTermServiceMode and MEDIA_BUY_ACTION_MODES in types.ts, mirroring the schemas. Parse once at the trust boundary (parseChangeTerms(commercialTerms): ChangeTerm[]), let every downstream function take the typed value, and delete the per-field probes.
  • Restore the concrete rejection type (type MediaBuyActionRejection = { errors: TaskError[]; context?: unknown }) and pass a domain AttemptedMediaBuyActionEntry[] instead of the cast.
  • Load the generated reported-outcome-error.json once and splice it under definitions with self-relative pointers. Validate req.error with the Ajv-compiled schema on all transports. Leave cloneReportedOutcomeError responsible only for what JSON Schema cannot express — byte cap, prototype and accessor hygiene, and safe deep clone — and state the byte bound in the schema description.
  • Derive Policy from the Zod schema, parse canonical_content with a declared CanonicalPolicyDocument so a malformed publication fails loudly, and type acceptance_profile with the profile shape.
  • Delete acceptance_policy_refs and derive COMPACT_PRODUCT_FIELDS from the canonical-product schema's property names.
  • Have canonical-media-buy-action-mode.json $ref/allOf-narrow the base enum, so a member cannot land in one file and not the other.

6. Assertions that cannot fail, and proofs that never run.

Five instances, one mechanism:

  • change_rights_state_projection.yaml:217 writes the expected code under code: where every other check: error_code entry in the corpus uses value: — 96 value: against exactly one code:, the one this PR adds, on #6750's fail-closed branch. The error-code lint and the contradictions lint both harvest only value/allowed_values, so the step asserts nothing and ACTION_NOT_ALLOWED is never checked against the canonical enum. Both siblings in the same file use value:, and this step also omits their expect_error: true / negative_path:.
  • negative_path: wrong_status (:314) and constraint_exceeded (:350) are outside the documented schema_invalid | payload_well_formed enum. lint-storyboard-sample-request-schema.cjs:344-348 treats anything that is not payload_well_formed as skip-schema-validation, so the static shape check is off for both steps — and both send deliberately well-formed payloads rejected on state/bounds grounds, which is the payload_well_formed case.
  • scripts/run-storyboards-matrix.sh is edited by this diff, but min_clean/min_passed and the REQUIRED_CLEAN_/REQUIRED_EXACT_CURRENT_SALES lists are untouched while four scenarios join requires_scenarios. git diff main...HEAD -- scripts/run-storyboards-matrix.sh | grep -E 'min_passed|REQUIRED_' returns nothing. The repo already has the right instrument — compact_product_lifecycle:9:0 is pinned exactly — and the new scenarios were not enrolled.
  • tests/seller-governance-policy-surfaces.test.cjs (400 lines, 8 tests) is the proof artifact for all four RFCs' schema semantics and is referenced by nothing: grep -rn seller-governance-policy-surfaces over the head returns zero hits outside the file. Sibling .cjs suites run only because package.json names them, and the one glob runner excludes **/*.test.cjs. Delete origin from policy-violation.json and no pipeline notices.
  • Two guards pass on their own output. policy-publication-resolution.test.ts asserts resolvePolicy returned the values its own stub supplied, plus two SQL-substring checks that pass on a semantically wrong query — delete the fail-closed if (version && policy.version !== version) return null and both tests stay green. And check-registry-completeness.cjs:174 has no negative case: make profileContentDigest() return profile.content_digest and the suite stays green, which turns every stored digest self-attesting.

Root: every guard in the proof pipeline is a hand-maintained list or an unvalidated key — suites are opt-in by editing package.json, scenarios are pinned by editing a shell array, matcher keys and negative_path values are unchecked vocabulary. The default state of a new proof is dormant, and a typo degrades an assertion to a no-op rather than failing loudly.

Fix the five instances, then close the class that produced them.

Repair the instances:

  • Change code: to value: on that step and add expect_error and negative_path alongside it.
  • Set negative_path: payload_well_formed on both out-of-enum steps.
  • Register the .cjs suite in a test: script.
  • Enroll all four scenarios in REQUIRED_EXACT_CURRENT_SALES with their exact P/S tuples. The exact form catches a degradation into skips; required-clean does not. Raise the sales and governance min_passed floors by the steps the scenarios contribute, and mirror both edits in .github/workflows/training-agent-storyboards.yml per the keep-in-sync header.
  • Add the behavioral assertions to the resolver test: no publication row plus a current row at a different version resolves to null, and a digest or canonical mismatch is not authoritative. Assert the parameter array rather than the SQL text.
  • Add the missing negative case for the profile-digest comparison.

Close the class:

  • Extend lint-storyboard-check-enum.cjs to reject a validation object that carries no recognized matcher key, and to validate negative_path against its declared enum.
  • Add a guard asserting that every tests/*.test.cjs file appears in some test:* script.

The sibling available_actions.yaml family carries the same out-of-enum negative_path values, which is why the fix belongs in the lint.

7. Every safety branch is graded off the wire or not at all — the storyboards grade the accepted path.

  • #6750's "an omitted action is not a negotiated right" has no scenario. grep -rn not_supported_on_buy static/compliance/source/ returns nothing; the branch exists at task-handlers.ts:4276-4280 and is pinned only by comply-test-controller.test.ts:1001. The one scenario asserting not_supported_on_product is the pre-existing available_actions.yaml, which grades the product-template path, not the accepted-terms path. #6750's acceptance criteria name not-negotiated explicitly.
  • #6757's bounded-evidence family — over-byte, over-depth, over-wide details, over-long property name, invalid recovery, invalid classification_source, and "the ledger is not mutated" — is asserted entirely by direct handleReportPlanOutcome(...) calls. Nothing proves a buyer on the wire receives VALIDATION_ERROR and that outcomes_reported stays 0.
  • #6758's credential/URL non-echo MUST is asserted by one in-process handleSyncGovernance call (see finding 1). The storyboard asserts only field_absent: accounts[0].governance_agents, never that https://untrusted-governance.example/mcp and the submitted credential are absent from the response.
  • #6757's one new normative field is graded nowhere on the wire: failed_outcome_audit_persistence.yaml reports details: { disposition: "declined", disclosure: "opaque" } — two keys that appear in no schema — and asserts on disclosure. grep -rn origin static/compliance/source/protocols/governance/ returns nothing. An implementation can pass conformance while never emitting the field #6757 was filed to add.
  • The disclosed arm of governance-agent-not-accepted.json is produced by nothing: grep -rn attempted_agent_origin server/src static/compliance returns no matches. The recovery path the error code's own suggestion instructs buyers to follow — "select a governance agent matching error.details.accepted_governance_agents" — exists only as an Ajv shape assertion.

Fix by adding the missing steps to the scenarios that already exist:

  • Add one update_targeting attempt against change_rights_state_buy asserting ACTION_NOT_ALLOWED with errors[0].details.reason == "not_supported_on_buy". The fixture already carries a closed five-entry change_terms list.
  • Add one over-bound report_plan_outcome step with expect_error: true and value: VALIDATION_ERROR, followed by a get_plan_audit_logs step asserting that the ledger is unchanged.
  • Assert on the rejection step that the submitted URL host and credential appear nowhere in the response, and route the non-echo unit test through simulateCallTool.
  • Report details: { origin: "seller", seller_policy_ref: … } and assert entries[*].error.details.origin. Keep a second outcome carrying the bare { origin: "seller" } so the scenario grades the genuinely opaque form too, and drop the invented disposition key.
  • Have the seller return the disclosed arm for its already-public allowlist and grade both arms, asserting that attempted_agent_origin equals the stripped origin and not the submitted URL.

8. RFC 8785 canonicalization was forked, and the fork both mints and verifies immutable digests.

scripts/check-registry-completeness.cjs:32-63 adds a from-scratch JCS canonicalize / digest / policyContentDigest / profileContentDigest. Those functions mint every content_digest baked into the immutable publication migration (generate-policy-publication-migration.cjs:22,124553_publish_political_acceptance_policies.sql) and every self-referential digest in static/registry/policies/*.json. The only test of those digests imports the same functions (tests/seller-governance-policy-surfaces.test.cjs:7, :115), so it proves the artifact matches its own producer and nothing about RFC 8785. The runtime and the documented consumer obligation use a different implementation — canonicalize from @adcp/sdk (plan-hash.ts:15, task-handlers.ts proposalTermsDigest), and registry.ts:1041 tells consumers to "canonicalize this object with RFC 8785 before verifying content_digest".

Root: canonicalization is a contract primitive with no shared home, so the validation layer grew its own — and that fork was promoted to the authority for a content-addressed, immutable-by-trigger registry. Publications are immutable, so a divergence in number formatting, escape selection, or key ordering ships as a permanently unverifiable published digest.

Reconsider the port itself before choosing how to test it. @adcp/sdk already implements RFC 8785, and the runtime and the documented consumer obligation both use it. Porting a second copy into the validation layer buys nothing the dependency does not already provide, and it costs a permanent maintenance obligation: every JCS edge case — number formatting, escape selection, key ordering, lone surrogates — becomes this repo's to track and to keep in step with the implementation buyers actually run. A spec-conformance primitive is the worst candidate for a hand-port, because the cost of a subtle divergence lands on immutable published digests that no later commit can correct.

Fix: use the existing implementation. Put one JCS module in one place, import it from the scripts, the server, and the tests, and back it with the canonicalize dependency — a thin CJS wrapper if the script boundary needs one. Then delete the fork.

Keep the local copy only if the CJS/ESM boundary genuinely blocks a direct import, and treat that as a temporary state with a tracked removal, not a resting place. In that case, add a cross-check asserting that digest(x) equals the SDK-canonicalized SHA-256 over a divergence-prone fixture set (non-ASCII, surrogate pairs, nested key ordering, integers against floats, -0, deep nesting), plus every checked-in static/registry/policies/*.json. training-agent.test.ts:15577-15581 already does this correctly for the catalog digest by recomputing from the served bytes, and is the pattern to copy.

9. New concepts and normative rules are restated per surface instead of declared once and referenced.

  • change_term_id plus the deprecated terms_ref block (with its deprecated / x-deprecated-in / x-removed-in and 130-character description) is pasted verbatim into all three oneOf branches of core/canonical-media-buy-action.json (lines 26/27, 41/42, 56/57) and once more in media-buy-available-action.json. A 4.0 removal needs four identical edits.
  • acceptance_policy_profile_ids is defined inline on both core/product.json:512 and core/canonical-product.json:77, and the descriptions have already diverged — only one states "the most restrictive matching disposition wins", so a reader of the compact product gets a weaker contract. The field name is also added to two separate selectable-field enums.
  • reported-outcome-error.json:45 re-declares code at maxLength: 128 where the canonical core/error.json says 64, so the audit copy can validly retain a code the wire could never have carried.
  • The rule #6750 exists to establish — accepted terms are the ceiling — is stated in four places with two modal strengths: proposal-negotiation.mdx:470 says MUST NOT, update_media_buy.mdx:61 says "cannot", plus control_media_buy.mdx and product-discovery/index.mdx:209. A conformance reader cannot tell which is the requirement.
  • docs/governance/sell-side-governance.mdx, the page that closes #6823, is unreachable. The docs.json nav entry added in 27ff9e1 no longer exists at the head (an upstream restructure to versioned dist/docs/... routes landed in between), and grep -rn sell-side-governance over the tree returns zero hits. docs-nav-validation.test.cjs passes, so the build is fine — the issue closes with a page nothing links to.
  • GOVERNANCE_AGENT_NOT_ACCEPTED's enumDescriptions entry (enums/error-code.json:187) is the only governance-family description with zero backticked symbols — GOVERNANCE_UNAVAILABLE has 12, GOVERNANCE_DENIED 54, PROVENANCE_VERIFIER_NOT_ACCEPTED 24 — so its accordion in docs/snippets/compliance-error-codes.mdx renders sync_governance, error.details, and GOVERNANCE_UNAVAILABLE as plain prose while every neighbour renders them as code.

Root: this repo's standard is that each new concept is its own $id'd schema $ref'd from every surface that carries it, and each normative rule has one home the task pages link to (as the sibling pages already do for url-canonicalization.mdx). This PR inlined per surface, which forks the contract at authoring time — and the fork has already produced a weaker contract for compact-product readers and a bound that contradicts core/error.json.

Fix each restatement at its source:

  • Extract the shared action fields into an $id'd definition and compose it per branch with allOf, so the discriminator survives.
  • Give both product schemas one $ref'd acceptance-policy-profile-ids field schema, and have get-products-request.json's fields enum compose product-fields.json rather than restate it.
  • Set code to maxLength: 64, or state in the description why the audit copy is deliberately wider.
  • Demote three of the four prose statements to pointers at the one normative home.
  • Cross-link the governance page from docs/governance/overview.mdx, docs/governance/campaign/specification.mdx, and docs/accounts/tasks/sync_governance.mdx.
  • Put the symbols in the error-code description and its errorRecovery suggestion in code font.

10. The v6 facade writes a false account.sandbox to steer an internal partition, and the falsified ref outlives the request.

server/src/training-agent/v6-sales-platform.ts:355-376 rewrites an incoming account.sandbox: true to false for static: principals so that sessionKeyFromArgs (state.ts:718-723, which keys on canonical.sandbox === false) takes its shared-partition branch. The comment concedes the value is untrue. But the rewritten AccountRef is what the business layer receives, persists onto media-buy and creative state (task-handlers.ts:578, :12625, :13626) and compares with accountRefsOverlap (:13721) — an authority-scoping comparison. The stored account disposition depends on which entry point wrote it.

Root: the session-partition key is expressed as a mutated wire model instead of a derived key. An adapter layer is authoring a domain fact.

Fix: leave the AccountRef untouched and move the decision into key derivation — sessionKeyFromArgs, or a partitionHintFor(principal, ref) it consumes, treats public static: principals as the shared open partition explicitly, taking the hint as a parameter. The facade passes a hint; it never writes a false field value into a model other layers trust.

11. The new shim-ledger entry has no working reverse gear, and the mutations it authorizes fail open.

.agents/sdk-shim-ledger.json:43-59 takes the reach-in ledger from 2 entries to 3, authorizing a string patch against compiled vendor JS anchored on the whitespace-exact literal ' "cancellation_terms"\n]);' (scripts/overlay-compliance-cache.sh). lint-sdk-shims.cjs states the bar: shims should be "visible, owned, tied to an upstream ask, and removed when the SDK grows a public API". This entry fails two halves of that.

The entry's upstream is adcp-client#2664. Verified with gh issue view: #2664, #2665, adcp-client-python#1067, and #1069 are all OPEN and unassigned, so nobody is obligated to do the work that deletes the entry. And #2664's body ("feat(media-buy): unified product, proposal, and live-buy action assessment") never mentions the negotiation verifier, dist/lib/negotiation/verification.js, or its hand-maintained commercial_terms allowlist — closing #2664 in full does not satisfy the stated removalCondition. That is a permanent entry wearing a "status": "temporary" label.

Separately, the two new addLiteralToConstUnion(...) mutations at scripts/overlay-compliance-cache.sh:210-211 reuse a helper that returns silently when its anchors move (if (declarationStart < 0) return;), so a future SDK bump turns the ledgered shim into a no-op and the failure resurfaces as an opaque storyboard validation error. The same diff shows the correct shape 25 lines later: patchProposalVerificationFile re-checks its post-condition and throws.

Fix the ownership and the failure mode together:

  • Self-assign #2664, #2665, adcp-client-python#1067, and #1069, so the parking spots are owned.
  • Either add the concrete item to #2664 — "derive the negotiation verifier's commercial_terms field allowlist from the generated schema so additive optional fields need no consumer patch" — or file a dedicated issue and re-point upstream and removalCondition at it.
  • Convert addLiteralToConstUnion and addLiteralToFirstActionUnion from return to throw when the const, the union terminator, or the afterValue anchor is missing, so every ledgered mutation proves that it applied.

12. One documented URL-canonicalization rule, two implementations.

docs/reference/url-canonicalization.mdx gains a row (added by this diff) asserting that governance_agents[].url vs accepted_governance_agents.any_of[].agent_url is governed by the same eight-step algorithm as the preceding provenance-verifier row. In the reference seller the two rows run different code with observably different output. The new path calls canonicalTargetUri from @adcp/sdk/signing (account-handlers.ts:1575). The accepted_verifiers allowlist still uses the local canonicalizeAgentUrl (task-handlers.ts:4824), which lowercases scheme and host and strips a trailing slash, but strips no default ports and normalizes no dot-segments — despite its own doc comment claiming it does. An allowlist entry that matches for a creative verifier can fail to match for a governance agent.

The same constant also normalizes twice: TRAINING_ACCEPTED_GOVERNANCE_AGENTS advertises TRAINING_ACCEPTED_GOVERNANCE_AGENT_URLS verbatim (account-handlers.ts:49) while enforcement builds its set as TRAINING_ACCEPTED_GOVERNANCE_AGENT_URLS.map(canonicalTargetUri) (:1589), rebuilt on every iteration of the per-account loop. Today's three literals survive canonicalization unchanged, so nothing is broken yet — add a fourth with a default port, an uppercase host or a trailing slash and the seller advertises one string and enforces another. The constant's own doc comment calls itself "one source of truth for what is advertised and what is enforced"; it is single only at the string level.

Fix: route both allowlist comparisons through one canonicalizer implementing the documented algorithm, with the https/userinfo rejection layered on top for the governance path, and delete canonicalizeAgentUrl. Canonicalize the allowlist once at module scope and derive both the advertised matchers and the enforcement set from that one array.

Notes / prior-review follow-ups

  • The prior rounds all landed: DR-0015…DR-0018 are committed with Closes references (@lukasz-pubx item 1), the sync_governance.mdx matcher-vs-origin clarifier is in place (item 2), and error.code optionality is restored with both schema and runtime regressions (@bokelley's thread on report-plan-outcome-request.json:192). None of those is re-raised.
  • Out of scope: projectTenantCapabilities re-parsing an already-serialized MCP response buffer (tenants/router.ts:705-733) is a pre-existing inverted boundary. Reworking it is separate architectural work outside this diff — finding 4 asks only that the contract this PR adds there not acquire a second author.
  • Out of scope: run-storyboards.ts dispatches all ~170 storyboards over MCP only, and there is no A2A/REST/ADK storyboard runner in the tree. That is a pre-existing property of the harness shared by every scenario, so it is not charged here — but it does mean none of the four new scenarios prove cross-transport serialization.
  • Out of scope: the real root of the compiled-JS overlay is upstream — @adcp/sdk's negotiation verifier keeps a hand-maintained commercial_terms field allowlist duplicating knowledge the generated schema already carries, so every additive optional field needs a monkey-patch. That fix lives in another repository; only the in-repo half is asked for in finding 11.

Comment thread server/src/training-agent/types.ts Outdated
Comment thread server/src/training-agent/task-handlers.ts
Comment thread server/src/training-agent/task-handlers.ts Outdated
Comment thread server/src/training-agent/task-handlers.ts Outdated
Comment thread server/src/training-agent/task-handlers.ts Outdated
Comment thread static/schemas/source/enums/error-code.json Outdated
Comment thread static/schemas/source/core/canonical-media-buy-action.json Outdated
Comment thread static/schemas/source/core/canonical-product.json
Comment thread server/src/training-agent/account-handlers.ts Outdated
Comment thread docs/governance/sell-side-governance.mdx
@lukasz-pubx

Copy link
Copy Markdown

As author of #6757/#6758 (now DR-0017/DR-0018), a prioritization note on the review above: four of the findings determine whether those two ratified contracts are demonstrated by this PR rather than merely present, and I've verified each at 55fa62b:

  1. Gate reachability (types.ts:64 vs SUPPORTED_RELEASE_VERSIONS max 3.2-beta.6) — both new capability surfaces are currently unreachable, so acceptance_policy_discovery.yaml and governance_agent_binding_acceptance.yaml skip as not_applicable and count clean. As it stands, the observable proofs behind DR-0017/DR-0018 execute zero steps.
  2. tests/seller-governance-policy-surfaces.test.cjs runs nowhere — verified zero references outside the file. It is the proof artifact for the origin field and matcher schema semantics; deleting origin from policy-violation.json today would trip no gate.
  3. governance_agent_binding_acceptance.yaml gating on the reference implementation's private allowlist entry — a conformant seller with a different (equally valid) any_of[] skips the scenario, so GOVERNANCE_AGENT_NOT_ACCEPTED is graded for exactly one agent. Gating on presence of the declaration, as the review suggests, matches how the sibling sync_governance scenarios bind.
  4. reported-outcome-error.json code at maxLength: 128 vs core/error.json's tighter wire boundRFC: typed seller dispositions in the campaign-governance audit record #6757's contract frames the audit record as a buyer-attributed copy of the seller response; a copy that can validly retain a code the wire could never have carried weakens that. Either align the bound or state in the description that the audit copy deliberately accepts a wider value.

No position from me on the remaining implementation findings — though the drifted duplicate budget evaluator (allows on currency mismatch where the canonical helper fails closed) and the second RFC 8785 implementation minting immutable digests look like the two with real blast radius.

None of this changes my support for landing the PR once the gates above make the new surfaces observable.

@bokelley

Copy link
Copy Markdown
Contributor Author

All four prioritization points map to open review threads posted today at 55fa62b:

  1. Gate reachabilitytypes.ts:64 and acceptance_policy_discovery.yaml:16. Design call for @bokelley: enrol 3.2-beta.7 in SUPPORTED_RELEASE_VERSIONS (advancing CURRENT_ADCP_VERSION), or lower the gate to ≥ 3.2-beta.6. Once the threshold is settled I can consolidate the comparator onto atLeastAdcpVersion and push.
  2. Test not wiredtests/seller-governance-policy-surfaces.test.cjs:1. Fix requires adding a test:seller-governance-policy-surfaces script and chaining it into test in package.json. This edit must be applied directly on the branch — outside agent scope.
  3. Storyboard gatinggovernance_agent_binding_acceptance.yaml:18. Fix is clear from the review thread: gate on presence of adcp.governance_enforcement.accepted_governance_agents.any_of (no contains), bind the accepted endpoint via $context.governance_agent_url matching the six sibling sync_governance scenarios. Can push this once item 1's reachability is unblocked.
  4. maxLength mismatchreported-outcome-error.json:45. Design call for @bokelley: set maxLength: 64 to match core/error.json's wire bound, or add a description stating the audit copy intentionally accepts a wider value than the wire permits.

Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Addressed the full review set in cc9061d and resolved all 32 review threads.

For the four prioritized proof gaps:

  • Seller-governance discovery now activates at 3.2-beta.6, the highest served beta, through the shared version comparator.
  • The seller-governance policy surface suite is registered in package.json and chained into the canonical test stages.
  • Governance-agent binding conformance gates on the feature declaration, uses the scenario context URL, and executes against the reference seller.
  • Reported outcome error codes now use the canonical 64-character wire bound, with the runtime/MCP schema loaded from the same source schema.

The broader review is also incorporated: duplicated capability/version/budget/digest logic was consolidated; proposal terms fail closed as the authority ceiling; opaque conditions, compound actions, aggregate package rights, and seller-managed paths were hardened; policy publications are append-only and digest-verifiable; registry/profile/schema composition is pinned and linted; SDK follow-ups are assigned; and the sell-side governance boundary documentation is linked from the main governance surfaces.

Verification completed locally: full build/static worker, 4/4 canonical server-unit shards (6,810 tests), schema suite, JSON docs, migrations, OpenAPI, registry completeness, and both current and 3.0 storyboard matrices. The four new exact scenarios execute with zero skips: change rights 8P, acceptance discovery 3P, governance-agent binding 4P, and failed-outcome audit persistence 4P.

@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 — hard approval gate not satisfied.

This PR touches files under gated paths (static/schemas/source/**, .agents/sdk-shim-ledger.json, .github/workflows/**) and review_decision is REVIEW_REQUIRED, not APPROVED. Per decision rule row 2, the outcome must be escalate (no critical/high findings exist to make this request-changes).

Additionally, this is a breaking-class governance surface change per repo context: new protocol schemas (acceptance-policy family), new error codes (GOVERNANCE_AGENT_NOT_ACCEPTED), new enum values, and new discriminated oneOf arms. Even though the reviewer found the surface uniformly additive and correctly discriminated with a correct minor changeset, protocol wire-shape ratification on this class of change is a human act per the constitution and is never auto-approved absent an APPROVED review.

Escalation reasons

  • Gated paths touched (static/schemas/source/**, .agents/sdk-shim-ledger.json, .github/workflows/training-agent-storyboards.yml) — hard approval gate; review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval required.
  • Protocol wire-shape surface expanded (new acceptance-policy schemas, new error code, new enum values, new optional fields, new oneOf arms) — governance/breaking-class ratification requires explicit human sign-off.

No Critical/High/Medium code findings were reported; migrations 557/558 are additive/transactional, schema↔docs coherence holds, and terms_ref is deprecated-with-compat rather than removed. The escalation is driven purely by the unsatisfied approval gate — once a human/CODEOWNERS approval is recorded, this can fall through to a normal decision.

Why human review

  • Gated paths touched (static/schemas/source/**, .agents/sdk-shim-ledger.json, .github/workflows/training-agent-storyboards.yml) — hard approval gate not satisfied: review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval required.
  • Breaking-class protocol surface change (new acceptance-policy schemas, new GOVERNANCE_AGENT_NOT_ACCEPTED error code, new enum values, new optional fields, new discriminated oneOf arms) — ratification is a human act per the constitution; never auto-approved absent an APPROVED review.
  • This PR touches a path under a hard, non-overridable approval gate (.agents/sdk-shim-ledger.json (modified) matches .agents/**; .github/workflows/training-agent-storyboards.yml (modified) matches .github/workflows/**; static/schemas/source/account/sync-governance-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/acceptance-policy-profile-ids.json (added) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action-fields.json (added) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-available-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-change-term-id.json (added) matches static/schemas/source/**; static/schemas/source/core/media-buy-legacy-terms-ref.json (added) matches static/schemas/source/**; static/schemas/source/core/product-allowed-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product.json (modified) matches static/schemas/source/**; static/schemas/source/core/sla-window.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/enums/canonical-media-buy-action-mode.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.j

…(truncated)…

@bokelley

Copy link
Copy Markdown
Contributor Author

Final CI follow-up: merged current main, fixed the tenant capability schema assertion, compacted redundant enum types in MCP prompt views to restore the 400 KiB budget, and aligned storyboard floor tests with the ratcheted 571/161 step floors. The replacement run is fully green (all checks successful or neutral), the branch is clean/mergeable, Ladon added no new findings, and all review threads are resolved. The only remaining GitHub block is the required human approval.

@bokelley

Copy link
Copy Markdown
Contributor Author

Final merge/CI follow-up at 76f5901:

  • Merged the latest origin/main (through 38c1719) with no conflicts.
  • Fixed the controller storyboard request to include the required sandbox assertion.
  • Aligned the GitHub /sales exact-count ratchet with the now-executable governance-agent binding scenario (5P / 0S).
  • All implementation checks are green: 57 passed, including build/static, typecheck, CodeQL, migrations/Postgres, Docker, docs smoke, canonical and server shards, and all current + 3.0 storyboard surfaces. Current /sales is 130/130 clean with 572 passing steps.
  • The code, protocol, and security expert reviews are merge-ready, and there are zero unresolved review threads.

The only remaining failed check is external reviewer infrastructure: Ladon completed with no buffered inline comments, then failed because it did not create its findings JSON. This reproduced on two reruns and has no repository finding to address. GitHub also still reports REVIEW_REQUIRED, so a human/CODEOWNERS approval is needed to record the WG approval and clear the hard gate.

@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 — hard approval gate not satisfied and Breaking-class governance change requires human ratification.

No blocking findings — the reviewer's subsequent-pass review of the delta since f58408d surfaced no critical/high/medium findings. The protocol-surface changes were verified as coherent: the new condition_unresolved reason value aligns across action-not-allowed-reason.json, error-code.json, control-media-buy-request.json, and the corresponding docs/compliance scenario (no schema↔docs drift); the minor changeset is correct for additive enum/fields retaining 3.1 compatibility; policies-db.ts async conversions and fail-closed resolvePolicy are correct; the acceptance_profile requirements oneOf is discriminated by a kind const. This is a clean diff.

Why escalate, not approve:

  • Gated paths (row 2): gated_paths: true (touches static/schemas/source/**, .agents/**, .github/workflows/**) and review_decision: REVIEW_REQUIRED — NOT APPROVED. The hard, non-overridable approval gate is not satisfied. Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded.
  • Breaking-class governance surface: New acceptance-policy schemas, new GOVERNANCE_AGENT_NOT_ACCEPTED error code, new enum values, new discriminated oneOf arms, and new decision records DR-0015..0018. Per the constitution's Breaking-class hard gate, ratification is a human act and is never auto-approved absent an APPROVED review.
  • Prior decision was also escalate for the same gate reasons; the gate condition persists unchanged.

Escalation reasons:

  • Gated files under static/schemas/source/**, .agents/sdk-shim-ledger.json, and .github/workflows/training-agent-storyboards.yml require human/CODEOWNERS approval; review_decision is REVIEW_REQUIRED.
  • Breaking-class / Normative governance protocol change (new schemas, error code, enum values, DR-0015..0018) — ratification is a human act.

Why human review

  • Gated paths touched (static/schemas/source/**, .agents/sdk-shim-ledger.json, .github/workflows/training-agent-storyboards.yml) — hard approval gate not satisfied: review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval required.
  • Breaking-class governance protocol surface change (new acceptance-policy schemas, new GOVERNANCE_AGENT_NOT_ACCEPTED error code, new enum values, new discriminated oneOf arms, DR-0015..0018) — ratification is a human act per the constitution; never auto-approved absent an APPROVED review.
  • This PR touches a path under a hard, non-overridable approval gate (.agents/sdk-shim-ledger.json (modified) matches .agents/**; .github/workflows/training-agent-storyboards.yml (modified) matches .github/workflows/**; static/schemas/source/account/sync-governance-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/acceptance-policy-profile-ids.json (added) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action-fields.json (added) matches static/schemas/source/**; static/schemas/source/core/canonical-media-buy-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/canonical-product.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-available-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/media-buy-change-term-id.json (added) matches static/schemas/source/**; static/schemas/source/core/media-buy-legacy-terms-ref.json (added) matches static/schemas/source/**; static/schemas/source/core/product-allowed-action.json (modified) matches static/schemas/source/**; static/schemas/source/core/product.json (modified) matches static/schemas/source/**; static/schemas/source/core/sla-window.json (modified) matches `static/schemas/so

…(truncated)…

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

3 participants