Skip to content

fix(schema): include async response refs in release bundles#5166

Merged
bokelley merged 1 commit into
mainfrom
review-prs-and-issues
May 29, 2026
Merged

fix(schema): include async response refs in release bundles#5166
bokelley merged 1 commit into
mainfrom
review-prs-and-issues

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

Fixes the schema release build so generated bundles include core/async-response-refs/ copies of async response schemas. This lets SDK/compliance validators pre-register async response refs before compiling core/async-response-data.json, even when original *-async-response-* tool response files are skipped during core pre-registration.

This is forward-only: existing exact-version bundles such as 3.1.0-rc.1 remain immutable. The patch changeset should cause the next Version Packages cut to publish 3.1.0-rc.2 with the corrected bundle.

Changes

  • Add copyAsyncResponseRefsToCore() to scripts/build-schemas.cjs and run it for release and latest schema builds.
  • Add a regression test for the SDK-style compile path that skips original async response files and relies on core/async-response-refs/.
  • Wire the regression test into npm test.
  • Add a patch changeset for the next RC.
  • Clarify the existing staging-script comment now that the issue applies beyond older 3.0.x bundles.

Validation

  • npm run test:build-schemas-async-response-refs
  • npm run build:schemas
  • npm run test:schemas
  • Precommit hook: test:unit, test:test-dynamic-imports, test:callapi-state-change, typecheck
  • Pre-push hook: current storyboard matrix and 3.0 storyboard compatibility matrix

Refs #5161

@bokelley bokelley marked this pull request as ready for review May 29, 2026 16:48
Copy link
Copy Markdown
Contributor

@aao-release-bot aao-release-bot Bot left a comment

Choose a reason for hiding this comment

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

LGTM. Forward-only fix with the right shape: SDK loader cooperation moves upstream into the published bundle instead of living only in stage-sdk-schema-bundle.sh.

Things I checked

  • copyAsyncResponseRefsToCore at scripts/build-schemas.cjs:1021-1057 — walk is contained to targetDir, skip predicates for bundled/ and the destination core/async-response-refs/ exit recursion cleanly, the core/ file-prefix skip is belt-and-suspenders (basename regex already filters), idempotent via pre-delete + ensureDir. No path-traversal surface.
  • No double-bundling. generateBundledSchemas reads from SOURCE_DIR (see callers at scripts/build-schemas.cjs:1899, :1922, :1983), so duplicated files in targetDir/core/async-response-refs/ are never seen by the bundler.
  • Call sites: release branch (:1921), latest-after-release (:1955), dev/latest (:2009). All three call after copyAndTransformSchemas and before generateBundledSchemas. Correct ordering.
  • npm test wiring: defined once at package.json:71, included once in the aggregate chain at :88, placed next to the other build-schemas-* tests.
  • Changeset is patch. Consistent with .changeset/prepare-3-1-rc-mode.md precedent for release-process changes. Per docs/reference/versioning.mdx, this adds no new $ids, fields, tools, or enum values — patch is the right tier.
  • 3.1.0-rc.1 immutability preserved. Forward-only to 3.1.0-rc.2.
  • audit-oneof.mjs only walks static/schemas/source/ and inspects oneOf structure — unaffected. In-repo validators (tests/json-schema-validation.test.cjs:116, tests/canonical-fixture-validation.test.cjs:73) already tolerate duplicate $id registration.
  • Test at tests/build-schemas-async-response-refs.test.cjs faithfully simulates the @adcp/client 8.1.0-beta.12 loader pattern — isOriginalAsyncResponse mirrors the skip list, refs are registered under core/async-response-refs/, target compiles. Tmp cleanup via finally + rmSync(..., { recursive: true, force: true }).

Follow-ups (non-blocking — file as issues)

  • Third-party validator duplicate-$id exposure. The mitigation co-locates dupes under a path the SDK loader treats as fragments — it's a soft contract that relies on consumer cooperation, not on the bundle being self-consistent. Naive validators (Python jsonschema, Go gojsonschema, ajv-cli in strict mode) that walk the published bundle and unconditionally register every .json will throw on the second $id registration. Worth a one-line aside in docs/reference/release-notes.mdx for 3.1.0-rc.2 clarifying that core/async-response-refs/ is a redundant SDK-compat directory, not a new schema namespace — the registry isn't auto-enumerated from disk so the new dir won't appear in index.json, and that asymmetry is the exact thing implementers will need explained.
  • Test exercises a synthetic fixture, not the real pipeline. Current test hand-rolls schemas under a tmp dir; it doesn't run copyAndTransformSchemascopyAsyncResponseRefsToCore end-to-end. If anyone adds a post-copy transform between those two steps later, copies could silently diverge from originals. A follow-up assertion that walks the actual dist/schemas/{version}/ output and confirms JSON.parse(original) deepEqual JSON.parse(copy) for each duplicated file would close that gap.
  • Sunset tracking. Carrying a permanent shadow directory of every async-response schema is a smell. File a tracking issue to rip this out when the SDK loader stops skipping response-tool files, or when core/async-response-data.json is restructured to not $ref tool response schemas directly. Pin it to a 4.0 milestone.

Minor nits (non-blocking)

  1. Inconsistent success logging across call sites. Release branch logs the count (scripts/build-schemas.cjs:1922-1923), dev/latest branch logs the count (:2009-2010), but the latest-after-release branch at :1955 calls silently. Cosmetic, but a future reader will wonder why the middle call site is quieter than its siblings.
  2. Walks into core/ unnecessarily. The directory walk descends into core/ and relies on the basename regex + file-prefix skip to drop everything inside. A directory-skip on core/ would be marginally tidier. Trivial.

Approving. Forward-only RC cut is the right path; once 3.1.0-rc.2 ships, the staging shell script becomes belt-and-suspenders rather than load-bearing.

@bokelley bokelley merged commit cd71e45 into main May 29, 2026
29 checks passed
@bokelley bokelley deleted the review-prs-and-issues branch May 29, 2026 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant