Repository navigation
fix(deploy): prune unused runtime schema history - #7496
Conversation
Exact-head terminal verificationIndependent read-only review at head
The PR remains intentionally Draft with auto-merge disabled. Human maintainer/CODEOWNER approval is still required before a ready transition, merge, or deployment. |
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| const { require: requireTypeScript } = require("tsx/cjs/api"); | ||
| const sourcePath = join(repoRoot, "server", "src", "training-agent", "schema-compat.ts"); | ||
| const { RETAINED_SCHEMA_BUNDLES: retained } = requireTypeScript(sourcePath, __filename); | ||
| if (!retained || typeof retained !== "object" || Array.isArray(retained) |
There was a problem hiding this comment.
MUST FIX: const { RETAINED_SCHEMA_BUNDLES: retained } = requireTypeScript(sourcePath, ...) reads an export that does not exist. At base/head, server/src/training-agent/schema-compat.ts exports only the singular RETAINED_SCHEMA_BUNDLE ('3.2.0-beta.6'); RETAINED_SCHEMA_BUNDLES appears nowhere in the repo, and this PR's 8 files do not add it. retained is therefore undefined and L67-69 throws must export a nonempty RETAINED_SCHEMA_BUNDLES map.
Two consequences, both blocking: the Dockerfile runtime-artifacts stage (RUN node scripts/prune-runtime-artifacts.cjs ...) exits non-zero, failing every image build on the deploy path; and npm run test:runtime-artifacts (build-check.yml) fails on its first test, since runtimeArtifactManifest(repoRoot) resolves retainedTrainingSchemaVersions against the real tree. The PR body's "retained training anchors 3.2.0-beta.6 and 3.2.0-rc.7" and passing-build proof describe a tree that carries this export — it is missing from the PR. Ship the schema-compat.ts change that exports RETAINED_SCHEMA_BUNDLES, or point the script at the existing RETAINED_SCHEMA_BUNDLE.
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
- scripts/prune-runtime-artifacts.cjs:66 — Prune script destructures
RETAINED_SCHEMA_BUNDLESfrom schema-compat.ts, but that export does not exist (base/head only export the singularRETAINED_SCHEMA_BUNDLE), and none of the PR's 8 files add it. The prune step throws, breaking the Dockerfile runtime-artifacts stage andnpm run test:runtime-artifacts. The dependency the PR body's passing-build proof assumes is absent from the submission.
Row 1 of the decision table fires: a high-severity finding is present, so the outcome is request-changes. Fix: either add the RETAINED_SCHEMA_BUNDLES export to schema-compat.ts or correct the destructure to the existing singular RETAINED_SCHEMA_BUNDLE.
Otherwise the pruning design is sound — fail-closed, well-tested, prunes only the in-image dist/schemas copy, leaves immutable git/R2 release records intact, and correctly requires no changeset for a deploy-only change.
Blocking findings
- scripts/prune-runtime-artifacts.cjs:66 — Prune script reads nonexistent RETAINED_SCHEMA_BUNDLES export; breaks image build and CI
| # and the training-agent checkpoint. Prune the builder copy so repository | ||
| # release records are never deleted or rewritten. | ||
| FROM builder AS runtime-artifacts | ||
| RUN node scripts/prune-runtime-artifacts.cjs \ |
There was a problem hiding this comment.
MUST FIX: Pruning the 35 omitted schema versions from the Fly image changes the wire for pinned immutable URLs if the Node server still serves /schemas/. schemas-middleware.ts:462-493 does not 404 a missing pinned version — resolvePinnedFallback silently rewrites /schemas/3.0.5/... to the nearest lower kept version on the same major and serves different bytes with Cache-Control: public, no-cache, must-revalidate instead of the exact immutable bytes. On main the image carries all 86 versions, so these are exact hits. After this prune, every adopter who pinned one of the 35 omitted paths — exactly the population the immutability rule protects — gets silently substituted schema content. The PR body's "confirm routing before deploying" is procedural, not a guard. Fail-closed: do not deploy this image as the /schemas/ origin unless the public route is bound to the artifact-cdn worker (which carries all versions); the discovery surfaces already advertise only the kept subset, so the exposure is direct pinned requests.
| } | ||
|
|
||
| function run() { | ||
| const repoRoot = resolve(process.argv[2] ?? join(__dirname, "..")); |
There was a problem hiding this comment.
Medium: run() defaults repoRoot to the real checkout, and the only guard — assertAdcpRepoRoot — passes on the real repo rather than blocking it. So node scripts/prune-runtime-artifacts.cjs run anywhere in the working tree will rmSync the 35 omitted dist/schemas/<version> directories and rewrite dist/schemas/index.json in place — deleting committed, immutable released artifacts from the checkout. Reversible via git, but surprising for a script whose guard exists to confirm it's pointed at a real adcp checkout. Consider a --dry-run default, or require an explicit opt-in (env flag / /app sentinel) so the destructive path only runs inside the Docker build stage it's designed for.
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
- Dockerfile:24 — Pruned runtime image silently serves wrong schema bytes for pinned immutable URLs. After pruning the local dist/schemas tree to a 51-of-86 closure, the Node schemas-middleware does not 404 the 35 omitted pinned versions; it falls through to serving the nearest lower kept version with a no-cache header. This breaks the immutable-URL contract for pinning adopters unless the public /schemas/ origin is bound to the artifact-cdn worker. The PR's "confirm routing" note is procedural, not an in-code guard — the served-wrong-bytes behavior is a reproducible customer-contract break on a load-bearing path (high).
Medium findings
- scripts/prune-runtime-artifacts.cjs:244 — prune script defaults to the real checkout and its guard enables rather than blocks destructive in-place deletion of released artifacts. Fail-closed verification (verify before/after prune) is the right shape and should be adopted.
Decision table: Row 1 fires (a high-severity finding is present) → request-changes. Note: gated_paths is true with review_decision=REVIEW_REQUIRED, which would independently force escalate under Row 2, but Row 1 wins top-down. Human/CODEOWNERS approval is still required on the .github/workflows/** changes before this can merge.
Blocking findings
- Dockerfile:24 — Pruned image silently serves nearest-lower kept schema version for 35 omitted pinned immutable URLs instead of 404ing, breaking the immutable-URL pinning contract; the routing note is procedural, not an in-code guard
Medium findings
- scripts/prune-runtime-artifacts.cjs:244 — prune script defaults to real checkout and deletes released artifacts in place
Keep both image-size fixes: hard-link dedup in the builder, then main's runtime-artifacts prune stage (#7496). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The runtime image carried schema releases that no current docs snapshot, hosted-compliance bundle or retained training checkpoint needs. This PR builds a disposable runtime schema copy and omits only that unused history. It keeps all48 published hosted-compliance pairs, all4 configured docs snapshots, discovery/latest/major/minor aliases, and both retained training anchors (
3.2.0-beta.6and3.2.0-rc.7). Missing or noncanonical required anchors fail before pruning. Current closure retains51 of86 schema releases and omits35 from the image copy. Immutable Git/R2 releases, protocol tarballs, source schemas and compliance/docs history are untouched.The Dockerfile preserves main's cache sanitization, compiled-asset guards and 7424 MiB application ceiling. Builder and final-image smokes use the actual pinned SDK15.2.0 to load/index every published compliance version and initialize every configured docs snapshot. CI path filters include the closure/smoke inputs. The new contents-read-only default-image workflow builds the exact reviewed head with actual Docker defaults, verifies every configured cache, records image/root/app/cache sizes and runs final-image smokes. It has no secrets, registry publication or deployment step; checkout credentials are not persisted. Its initial PR trigger is limited to the workflow path, with later exact-SHA manual dispatch supported.
Current head:
c238b56188d7a4832bdc1c653b565268c15d259c. Production source is unchanged from qualified parent3dd3e27; the two follow-ups only add/fix the footprint workflow. The original draft head remains in history; this updates the same PR/branch without a force push or duplicate PR.Validation:
sha256:6191a37d7bd13cc60f8b676cd24b37d081f9b0d75f9f5069f2b83b8a202490aa. Both builder and final-image docs4/SDK48 smokes passed. Exact source SHA, image inspection, sizes and smoke logs are retained in the workflow artifact.Direct Fly history narrows to the runtime closure. Read-only history verification now covers every omitted schema asset: all23,246 committed JSON files across all35 omitted schema versions returned HTTP200 with exact Git-verified bytes from the candidate Worker. All35 version indexes also matched committed bytes on both the public site and candidate. This proves candidate historical-schema content, not the active public route binding or complete compliance/protocol-tar cutover; confirm routing before deploying the pruned Fly image. No deployment or live route/settings change was performed.
Remain Draft; auto-merge off. Human maintainer/workflow review and deployment/CDN routing confirmation remain required. The full default-cache image size/smoke proof is complete.
Existing workspace.