chore: sync AdCP 3.1 rc4#2123
Merged
Merged
Conversation
f35b556 to
80b03ba
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape for an rc-tracking bump: the only hand-written code change is two lines of compat-cast in preflight.ts:653-655 with the spec-lineage comment baked in, and the rest is mechanical regeneration plus a CI-resilience fallback in scripts/sync-schemas.ts.
Things I checked
.changeset/adcp-3-1-rc4.mdpresent, typeminor. Defensible — rc4 narrowsMediaBuyActionMode(removesrequires_proposal), but the runtime cast atpreflight.ts:655preserves behavior for legacy emitters and this is rc→rc prerelease churn, not a GA contract change.requires_proposallives on the buyer-side switch arm, not the wire response shape.ADCP_VERSION(3.1.0-rc.3→3.1.0-rc.4) andpackage.json#adcp_versionmove together.package.json#versionuntouched — changesets keep ownership.sync-version.ts:99appends'3.1.0-rc.4'toCOMPATIBLE_PREFIX;version.tsregenerated andCOMPATIBLE_ADCP_VERSIONSextends through3.1-rc.4. 3.0.x compat surface preserved.scripts/sync-schemas.tsfallback threadsbaseUrlthroughsyncFromTarball/syncSchemasPerFile/downloadSchemacleanly.verifyCosignSignaturealready receivesbaseUrlfor sidecar lookup — sidecars get resolved against the same origin as the bundle.- All generated surfaces stamp
schemas/cache/3.1.0-rc.4/consistently — entity-hydration, wire-spec-fields, manifest, schemas, tools, enums all agree on the pin. No mixed-version drift.
Follow-ups (non-blocking — file as issues)
scripts/sync-schemas.ts:543-551— fallback catches every throw, including cosign and sha256 failures. Ifadcontextprotocol.orgever serves a bundle that fails cosign or sha256 verification, the script silently retries againstraw.githubusercontent.com/.../dist, where missing sidecars trip theverifyCosignSignature"predates signing — checksum-only" branch atscripts/sync-schemas.ts:191-194. Net effect: a genuine signature failure on the canonical mirror is masked by an unsigned re-fetch. Scope the catch to network/404-class errors only — letcosign verify-blob failedandTarball sha256 mismatchpropagate. Minimum viable: re-throw iferr.messagematches/cosign|sha256 mismatch/i.ADCP_GITHUB_FALLBACK=0opt-out is undocumented. Only escape hatch when the fallback misbehaves; mention it in the comment block above the try/catch at minimum.- rc4
requires_proposalremoval — is there a new wire signal the SDK should dispatch on? The compat cast atpreflight.ts:655keeps the old code path alive for legacy emitters, which is correct. But if rc4 sellers now signal proposal-required via a different surface (a new error code oncreate_media_buy, a different mode value, anexthint),recoveryForModeMismatchwon't pick it up. Worth confirming againstschemas/cache/3.1.0-rc.4/media-buy/and the upstream spec PR that did the removal — link the spec PR in a follow-up if there's no new dispatch site needed.
Minor nits (non-blocking)
- Warn message overstates the failure mode.
scripts/sync-schemas.ts:550reads "was not reachable from adcontextprotocol.org" but the catch fires on any throw — including signature/checksum failures. Either narrow the catch (preferred, see follow-up #1) or say "sync against adcontextprotocol.org failed".
LGTM.
There was a problem hiding this comment.
LGTM. Faithful schema-pin bump tracking rc4 with a load-bearing compat shim for sellers still emitting requires_proposal.
Things I checked
MediaBuyActionModecorrectly drops'requires_proposal'atsrc/lib/types/core.generated.ts:4664;signed_responseonVerifyBrandClaimSuccesslands as a required additive field with a well-typedResponsePayloadJWSEnvelopeshape.- The runtime compat shim at
src/lib/media-buy/preflight.ts:655(entry.mode as MediaBuyActionMode | 'requires_proposal') keeps the recovery path working for rc.3-era sellers — wire-compat preserved while the type narrows. scripts/sync-version.ts:99extendsCOMPATIBLE_PREFIXcorrectly. No other dispatch table needed updating.- Cosign verification at
scripts/sync-schemas.ts:177-243is threaded through the newbaseUrlparameter — checksum and signature checks still run against whichever mirror is serving the bundle. The GitHub-raw fallback is opt-out-able viaADCP_GITHUB_FALLBACK=0, and thetry/catchretry only triggers when the primary mirror is the canonicaladcontextprotocol.org(no double-fallback against customADCP_BASE_URLoverrides — right call). - Changeset is
minor, which matches the established beta-track convention for AdCP RC pin bumps (adcp-3-1-beta-5,adcp-3-1-beta-7both shippedminoron the8.x.x-beta.NNline).
Follow-ups (non-blocking — file as issues)
src/lib/errors/index.ts:514declares an inlinemodeunion that still includes'requires_proposal'as a current value, andbuildModeMismatchRecoveryat line 541 switches on it as a present-tense case. Pre-existing — this PR didn't touch it — but the analogous surface topreflight.ts:655, and now diverges fromMediaBuyActionMode. Worth a follow-up to mirror the cast trick (or narrow the union and accept the legacy literal explicitly) so the SDK's two error-reasoning paths agree about what the rc.4 wire claims.scripts/sync-schemas.ts:539-549— the fallback retry isn't transactional across the tworeplaceTreecalls. If attempt-1 succeeds onschemas/(line 379) but fails oncompliance/(line 380), the catch retries and the new.previoussnapshot of schemas captures the attempt-1 partial, clobbering the genuine prior. Narrow window (committed cache is also in git, so recovery isgit checkout), but the schema-diff tool could be misled. Either snapshot both trees atomically before either rename, or treat any post-replaceTreefailure as fatal.- Changeset prose mentions the recovery-path tolerance but not that
MediaBuyActionModelost a union member. Worth a sentence so adopters with switch statements on the type know to expect a now-unreachable case.
Minor nits (non-blocking)
- Cosign log message at
scripts/sync-schemas.ts:192. When the GitHub mirror serves the tarball but not.sig/.crt, the existing "upstream predates signing" message reads wrong — the cause is "this mirror doesn't carry sidecars," not version age. SurfacebaseUrlin the log. - Comment accuracy at
src/lib/media-buy/preflight.ts:653. "Removed from the rc4+ mode enum" — removed in rc.4, not "rc4+". Audit-trail nit.
Safe to merge.
80b03ba to
59c6288
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Schema diff
Compared schemas/cache/3.1.0-rc.3 to schemas/cache/3.1.0-rc.4. Notable wire changes: signed_response is required on successful brand claim verification responses, pricing_currencies is available on product filters, requires_proposal is removed from media-buy action modes, signal definition enrichment is added.