fix(sync): stop schema syncs from clobbering the primary pin's shared surfaces#2353
Merged
Merged
Conversation
… surfaces sync-schemas wrote non-version-scoped, checked-in state on every run, so syncing a side-bundle version (3.0.12, v2.5, the opt-in beta) disturbed the primary pin (ADCP_VERSION, currently 3.1.2): - Protocol skills (skills/adcp-*): a side-bundle sync overwrote them with an older version's content. Gate the skill write behind includeSharedSurfaces, which defaults on only for the primary pin. - latest/ cache pointer: a standalone side-bundle sync silently repointed it, making the SDK validate against the older version by default. Gate it the same way (both syncFromTarball and the per-file fallback). - schemas/registry/registry.yaml: owned by generate-registry-types --sync from a different upstream (agenticadvertising.org) that runs ahead of the pinned protocol bundle. sync-schemas copying the tarball's stale copy only downgraded it and dirtied every working tree. Stop writing it here entirely. Removes the fragile restoreFromHead/RESTORE_PATHS workaround in the beta sync. Fixes #2352 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Approving. Ties three non-version-scoped surfaces to the primary pin instead of letting whichever side-bundle synced last win — the right shape: version-scoped state stays per-version, shared state tracks one owner.
Things I checked
- Registry ownership claim is real.
sync-schemasno longer writesschemas/registry/registry.yaml. Verified the sole owner isgenerate-registry-types --sync, which pulls from a different upstream (scripts/generate-registry-types.ts:15→https://agenticadvertising.org/openapi/registry.yaml). Both CI gates run it aftersync-schemasand thengit diff --exit-codethe committed file:package.json:357(ci:schema-check) andscripts/ci-validate.js:117+:129. The committed-in-sync invariant is unchanged. This also quietly fixes a latent publish bug —prepublishOnly→sync-schemas:allused to overwrite the authoritative committed registry with the staler tarball copy right before publish; now it leaves the committed file alone. - Gate defaults to fail-safe.
sync()computesincludeSharedSurfaces = options.includeSharedSurfaces ?? adcpVersion === primaryPin(scripts/sync-schemas.ts~L556).===binds tighter than??, so this reads?? (adcpVersion === primaryPin)— correct. CLIsync-schemas -- 3.0.12→false; baresync-schemasand explicit-- 3.1.2→true. Side-bundle syncs leave skills andlatest/alone; the primary pin still refreshes both. - Cross-file contract matches. The beta script's
syncSchemas(BETA_VERSION, { includeSharedSurfaces: primary === BETA_VERSION })lines up with the newsync(version?, options)signature (export { sync as syncSchemas },scripts/sync-schemas.ts:574). When the beta is the primary pin,includeSharedSurfacesistrueand skills get written — the intended-bump case is preserved. - No dangling refs after the removals.
REGISTRY_SPEC_PATHand its only uses are gone;copyFileSync/mkdirSyncstay imported because they're used elsewhere (sync-schemas.ts:274, severalmkdirSyncsites). In the beta script,restoreFromHead/RESTORE_PATHSare fully removed butspawnSyncsurvives for the GitHub-dist fallback (L129) andexistsSyncsurvives inhasBetaCache. - Per-file fallback gated consistently.
syncSchemasPerFilenever wrote skills or registry; it now gates only thelatest/pointer behind the same flag. Consistent. - Changeset present and correctly typed.
.changeset/sync-schemas-shared-surfaces.mdis apatch. This is build-tooling that shapes published content (skills, cache, registry), and the change restores intended behavior with no API/export surface change —patchis the right call. MUST-FIX #5 satisfied.
Follow-ups (non-blocking — file as issues)
- The
RESTORE_PATHSdeletion removes the one place that enumerated "shared surfacessyncSchemastouches." The invariant now lives implicitly in theincludeSharedSurfacesgates. If a future surface gets added tosyncFromTarball, there's no list forcing the author to gate it. A one-line assertion or test that a3.0.12sync leavesskills/adcp-*andschemas/cache/latestbyte-identical would lock the contract down.
Minor nits (non-blocking)
- Renaming the inner
syncWithBaseparamoptions→fallbackis good hygiene now that the outersyncowns anoptionsparam (scripts/sync-schemas.ts~L540). No behavior change, avoids a shadow. Noting it so a future reader doesn't "fix" it back.
Removing a hand-maintained git checkout HEAD restore list and replacing it with a gate that defaults off for side-bundles is the correct direction — fail-closed beats papering over the clobber after the fact.
LGTM. Follow-up noted below.
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.
Fixes #2352
Problem
scripts/sync-schemas.tswrote non-version-scoped, checked-in state on every run, so syncing a side-bundle version (3.0.12,v2.5, the opt-in3.1.0-beta.7) disturbed the primary pin (ADCP_VERSION, currently3.1.2). Becausenpm run sync-schemas:allsyncs several versions in sequence, contributors kept seeing phantom diffs and reverting files they never touched.Three shared surfaces were affected:
skills/adcp-*) — stored in one shared folder (no per-version copy). A3.0.12sync overwrote the pinned3.1.2skills with older content. The beta script papered over this with a fragilegit checkout HEADrestore (RESTORE_PATHS) that ran after a network fetch and needed a hand-maintained path list.latest/cache pointer — a standalonesync-schemas -- 3.0.12silently repointedschemas/cache/latestat the old version, making the SDK validate against it by default. (Surfaced in review.)schemas/registry/registry.yaml— not really part of the versioned bundle. Its real owner isscripts/generate-registry-types.ts --sync, which downloads it fromhttps://agenticadvertising.org/openapi/registry.yaml(a live spec ahead of the pinned tarball).sync-schemascopying the tarball's staler copy only downgraded the committed file.Fix
All three shared surfaces are now tied to the primary pin:
syncFromTarballgains anincludeSharedSurfacesflag (threaded into the per-file fallback too). It defaults on only when the synced version equalsADCP_VERSION. Skill writes and thelatest/pointer are gated behind it, so side-bundle syncs leave them alone.registry.yamlwrite is removed fromsync-schemasentirely —generate-registry-types --syncis its sole owner.restoreFromHead/RESTORE_PATHSworkaround is deleted fromscripts/sync-3-1-beta-schemas.ts; nothing gets clobbered anymore, so ordering insync-schemas:allno longer matters.Not affected
schemas/cache/<version>/, untouched.build-*,call-adcp-agent) — already never overwritten.ci:schema-checkandci-validate.jsalready rungenerate-registry-types --syncaftersync-schemas, so the committedregistry.yamlinvariant is unchanged;prepublishOnlyships skills that deterministically track the primary pin.Verification
3.1.2): still writes skills,latest → 3.1.2, registry untouched.3.0.12): skills unchanged,lateststays3.1.2, registry untouched.generate-registry-types --syncconfirmed as registry's sole owner (zero diff vs committed).