Instrument AdCP 3.2 across docs, compliance, and training - #6799
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — 3.2 instrumentation PR (beta.4 → beta.5 pin bump, targeting-aware configured products, beta.7 idempotency/retry contract, durable encrypted webhook outbox).
Checks performed:
- No
static/schemas/source/**changes → no wire-shape drift, no oneOf regression, no dist-immutability concern. - Changeset present and scoped to normative reference docs + protocol-adjacent training (minor). Consistent with the non-breaking, additive instrumentation surface.
- New migrations (549/550/551) are additive and fail-closed; judged as production-facing and clean.
- Webhook outbox is framework-resolved principal, per-caller scoped, cross-replica recovery, heavily tested.
- No breaking-class protocol change → hard breaking-change gate does not fire.
gated_paths: false; no no-auto-approve team match; high_risk driven by(modified)docs/reference +(added)migrations only, with no medium finding on a modified high-risk file (row 5 does not fire).
Medium findings (1 — non-blocking):
- server/src/training-agent/task-handlers.ts:6204 — targeting-aware discovery silently truncates at the per-session configured-product cap. Operability concern, category operability (not data-loss/schema/infra), so no row-4 escalation.
Low finding (warn-only webhook recovery terminalization) omitted per severity model.
Decision path: no critical/high (row 1 n/a); gated_paths false (row 2 n/a); no deletions (row 3 n/a); the single medium is operability, not data-loss/schema/infra (row 4 n/a); high_risk modified files carry no medium finding (row 5 n/a); no prior decision (row 6 n/a); no team gate (row 7 n/a); only 1 medium, < 3 (row 8 n/a) → row 9 approve.
Medium findings
- server/src/training-agent/task-handlers.ts:6204 — Targeting-aware discovery silently drops products at session configured-product cap
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
- package.json:30 —
test:schemasdrops two schema-contract tests (stale-rebase clobber). The script silently removes two schema-contract tests added at base by #6620/#6621, pulling them out of CI. This is a reproducible regression that reduces test coverage on the protocol contract surface (high). Restore the two dropped tests to thetest:schemasscript.
Medium findings
- server/src/training-agent/task-handlers.ts:6204 — Configured-product cap silently drops products from discovery. Unhandled edge case worth addressing but non-blocking on its own.
Decision rationale: Row 1 fires — a high severity finding is present, so the outcome is request-changes. Once the test:schemas regression is restored, the remaining single medium finding would fall through to approve. Docs version bumps are internally consistent, no schema source changed, and the new migrations/webhook outbox work looks sound.
Blocking findings
- package.json:30 — test:schemas drops two schema-contract tests (#6620/#6621) from CI (stale-rebase clobber)
Medium findings
- server/src/training-agent/task-handlers.ts:6204 — Configured-product cap silently drops products from discovery
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior blocking findings resolved, no current findings.
What changed since the prior review (request-changes):
- The prior Medium (task-handlers.ts:6204 — silent configured-product drop) is resolved.
applyDiscoveryTargetingnow computes exact net additions BEFORE any session mutation and, on overflow, returns a fail-closed atomicLIMIT_EXCEEDED/correctable error viahandleGetProductsUnlockedinstead of partially configuring or silently truncating. A new test asserts the correctable error shape and thatconfiguredProductsis unchanged after rejection. - The prior High (package.json:30 — test:schemas dropping schema-contract tests) is not part of this delta and is out of scope per subsequent-review scope rules.
Current findings: none.
Gate checks:
- No no-auto-approve team match.
gated_paths: false, so row 2 does not apply (the CHANGES_REQUESTED review_decision only blocks approval when a path/prose hard gate is engaged; none is here). This PR adds instrumentation, docs, migrations, and tests without touchingstatic/schemas/source/**, so schema↔docs coherence and breaking-class governance gates are not triggered.high_risk: true, but all high-risk reference-doc hits are(modified)with no findings, and all migration hits are(added)scaffolding — no(deleted)entry, no medium finding on a modified high-risk file, so rows 3–5 do not fire.- Prior decision was
request-changes(notescalate), so sticky-escalation row 6 does not apply. With zero current findings, rows 1 and 8 do not fire.
Net improvement: the developer addressed the prior blocking feedback and the reviewer surfaced nothing new. Falls through to row 9.
Summary
@adcp/sdk@14.0.0-beta.7/ served AdCP3.2-beta.5and add a release-instrumentation manifest covering all 16 feature families across runtime, compliance, docs, and trainingRelease safety
Verification
Closes #6199
Advances #6556
Advances #6557