Repository navigation
fix(release): recover a release's schema and compliance trees on the CDN - #8351
Conversation
schemas/3.0.27, schemas/3.0.28 and both compliance trees never reached the artifact bucket. That does not 404: the worker's pinned-semver fallback resolves the pin to the nearest published release on the line, so those versions answer 200 with 3.0.26 bytes, and the discovery pointers keep v3.0 pinned one release back while every deploy logs the gap. No path could fix it. release.yml publishes only the current checkout's package version, routine deploy excludes every semver path, and live bulk backfill is disabled, so RELEASING.md told that warning's reader to recover releases the repository had no way to recover. Add a surfaces input to recover-protocol-cdn.yml. protocol stays the default and is unchanged; all additionally recovers the immutable version trees. R2 objects are unsigned, so the authority comes from the one signature that exists: build-protocol-tarball.cjs stages the tarball by copying the dist trees verbatim, so proving tree parity against the cosign-verified tarball makes that signature cover every object. The bundle's adcp-<version> root is the version binding, since cosign's identity regexp binds the workflow and not the version. Bytes are read with git cat-file blob rather than git archive, whose output the tag's own .gitattributes can rewrite. Writes are conditional and only where the object is confirmed missing; an existing object must match both bytes and content type. Verification then requires the public origin to return the bytes, the immutable policy and the R2 ETag, because the Fly origin serves the same committed tree under the same policy on a miss and only R2 reports the body's MD5 as its ETag. Closes #8347. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated path requires human/CODEOWNERS approval.
This PR modifies .github/workflows/recover-protocol-cdn.yml, which matches the deterministic gated-path glob .github/workflows/**. gated_paths: true and review_decision is REVIEW_REQUIRED (not APPROVED), so decision-table row 2 fires: the hard approval gate is not satisfied and the outcome cannot be approve or plain comment.
No blocking findings exist. The reviewer found no Critical/High/Medium issues across the full ~1600-line diff. The authority chain (cosign tarball → tree parity with bounded generated_at carve-out → conditional immutable writes with --if-none-match '*', byte/content-type refusal on existing objects, R2-MD5 ETag gate) is sound, no protocol surface is touched (so the absent changeset is correct), and released dist/** artifacts are never mutated.
One non-blocking follow-up (not a finding): the per-object aws s3api head/cp/put-object calls in the upload loop (trees.sh:677-705) lack explicit --cli-read-timeout/--cli-connect-timeout, relying on AWS CLI defaults rather than an explicit bound like the curl verify path.
Escalation reason: .github/workflows/recover-protocol-cdn.yml is under a hard approval gate — a human/CODEOWNERS approval (review_decision: APPROVED) is required before this can merge.
Why human review
- Modifies .github/workflows/recover-protocol-cdn.yml — a gated path under a hard approval gate; requires human/CODEOWNERS review (review_decision is REVIEW_REQUIRED, not APPROVED)
- This PR touches a path under a hard, non-overridable approval gate (.github/workflows/recover-protocol-cdn.yml (modified) matches
.github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
CodeQL flagged the recovery verification fixture: it joined a request path onto the fixture directory and read it, so a traversing request would have escaped the served tree. Resolve the path and require containment, which is also what the real origin does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The containment check still let a request value reach the filesystem, and paired existsSync/readFileSync added a race. Enumerate the served tree once and let a request only select an entry, so an unknown or traversing path has no entry and nothing derives a path from the request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ladon's review noted the upload loop relies on AWS CLI default timeouts while the verification fetches are explicitly bounded. The loop makes three calls per object across hundreds of objects, so one stalled socket would hang publication instead of failing it. Bound them the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — addressed the non-blocking follow-up in 6a4cf17: the upload loop's On the escalation: agreed, and expected. Two things a reviewer may want to know:
|
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated path requires human/CODEOWNERS approval.
This PR modifies .github/workflows/recover-protocol-cdn.yml, which matches the hard approval gate glob .github/workflows/** (gated_paths: true). The current review_decision is REVIEW_REQUIRED, not APPROVED, so the gate is not satisfied and auto-approval is barred regardless of how clean the diff is (decision table row 2).
Escalation reasons
- Modifies
.github/workflows/recover-protocol-cdn.yml— a gated path under a hard, non-overridable approval gate; requires human/CODEOWNERS review (review_decision is REVIEW_REQUIRED, not APPROVED).
The diff itself is clean: the reviewer found no Critical/High/Medium findings. The delta over the prior pass added AWS CLI connect/read timeouts to all three S3 calls in the recovery script's upload loop (closing the operational-readiness gap) and refactored the test fixture to a pre-enumerated Map lookup keyed correctly to listFiles() relative paths. Both changes are sound. Once a human/CODEOWNERS approval is recorded (review_decision: APPROVED), this PR falls through to the normal table and is approvable on its merits.
Why human review
- Modifies .github/workflows/recover-protocol-cdn.yml — a gated path under a hard approval gate matching
.github/workflows/**; requires human/CODEOWNERS review (review_decision is REVIEW_REQUIRED, not APPROVED) - This PR touches a path under a hard, non-overridable approval gate (.github/workflows/recover-protocol-cdn.yml (modified) matches
.github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
Closes #8347.
The bug
schemas/3.0.27/,schemas/3.0.28/,compliance/3.0.27/andcompliance/3.0.28/were never published to the artifact bucket. The protocol tuples for both are there; only the trees are missing.That does not 404. The worker's
resolvePinnedFallbackresolves a pinned semver with no published directory to the nearest release at-or-below it on the line, so right now:200 OK, wrong schemas, no error.
/compliance/3.0.27/and/compliance/3.0.28/do the same. The only outward tells arecache-control: public, no-cache, must-revalidateinstead of the immutable policy, and the::warning title=Schema versions missing from the CDNthat #8324's pointer builder now logs on every deploy while it keepsv3.0pinned to 3.0.26.Nothing could fix it.
release.ymlpublishes only the current checkout's package version (check-release-state.cjs committedrequires the local, tested and original committed versions to be equal);--latest-onlyexcludes every semver path; live bulk backfill is disabled (#7507/#7510).RELEASING.mdtold that warning's reader to "recover those releases per RELEASING.md", and then said the stable-onlyrecover-protocol-cdn.yml"recovers four protocol files, not schemas/compliance".The fix
recover-protocol-cdn.ymlgains asurfaceschoice input.protocolis the default and is byte-identical to today's behaviour;alladditionally recovers the immutableschemas/<version>/andcompliance/<version>/trees viascripts/recover-cdn-version-trees.sh.The authority problem this had to solve: R2 objects are unsigned, and a historical version has no current-checkout authority to lean on. The answer is the one signature that does exist.
build-protocol-tarball.cjsstagesdist/protocol/<v>.tgzby copyingdist/schemas/<v>anddist/compliance/<v>verbatim, soscripts/check-protocol-bundle-parity.cjsproves the trees about to be published are byte-for-byte the trees inside the cosign-verified tarball — making that one signature transitively cover all ~770 objects. The only tolerated divergence is the build timestamppinGeneratedAtre-pins inschemas/manifest.jsonandcompliance/index.json, matched by byte substitution rather than deep JSON equality so a reordered or reformatted file still fails. Verified against the real 3.0.27, 3.0.28, 3.1.29, 3.2.1 and 3.2.5 artifacts.Everything else follows
RELEASING.md's existing model:$RUNNER_TEMPbeforegit checkout --detach v<version>, so recovery logic never comes from the artifact being recovered;git archive HEAD, never the working tree, and only from regular blobs with an unambiguous name and a known published content type (an unmapped extension fails before any network call rather than publishing a mistyped partial tree);--if-none-match '*'and only after a confirmed missing object; an existing object must already match byte-for-byte; an inconclusive HEAD aborts instead of reading as absent; nothing is overwritten, deleted or aliased, and the root discovery pointers stay with the deploy path;exactPinnedHitsignal, so it is checked too.Stable only, no RC releases. The dispatch is idempotent: a complete recovery reports every object as already verified.
Testing
tests/recover-cdn-version-trees.test.cjs(29 tests, wired intonpm run test:release-workflow) runs the real script against a stubbedaws/gitand a real local HTTP origin answering realcurl. Mutation-checked: removing the parity proof, the ETag column, thegit cat-fileread, the content-type assertion, the inconclusive-HEAD branch, the immutable cache policy, or widening the parity exemption to any file each turns the corresponding tests red.Covered: conditional immutable publication with the right content types; existing-object verification vs. refusal to overwrite or to accept a mistyped object; inconclusive HEAD; parity against a changed/extra/missing bundle file; a timestamp rewritten in a file the bundle build never pins; a symlinked tree entry; an
export-subst.gitattributes; a tarball failing its committed checksum; another version's bundle swapped in; a tree with noindex.json; prerelease, mismatched checkout and stale branch refusals; a mid-run branch move leaving exactly the objects written before it; unmapped extension; bad bucket/credentials/mode/retry budget; verification passing, retrying while the CDN catches up, and failing on differing bytes, a fallback policy, an origin-served ETag, a missing ETag and a missing object; plus the workflow wiring.Also run locally:
npm run test:release-workflow(every stage green: 279 + 81 + 29 tests plus the two standalone scripts),node scripts/check-changeset-protocol-scope.cjs origin/main(no changeset — this is release machinery, not protocol surface),node scripts/check-pr-title.cjs.Pre-commit hook: committed with
HUSKY=0, and this is why. The hook runs the full server unit suite becausepackage.jsonis inFULL_SERVER_UNIT_PATTERNS, which takes ~25 minutes here, and this sandbox kills the process long before it finishes. Every stage was instead run individually to completion, all green:scripts/check-image-quality.shnpm run test:unitnpm run test:test-dynamic-importsnpm run test:format-identity-boundariesnpm run test:callapi-state-changeprecommit:server-unit(6 shards)npm run typecheckNothing was skipped, only re-split so each piece fit the sandbox's process lifetime.
Pre-push hook: the first push used
ADCP_SKIP_STORYBOARD_MATRIX=1. The matrix triggered only becausepackage.jsonis inSTORYBOARD_PATHS; this PR touches no training-agent or compliance source, so the matrix has nothing to grade here. It ran for 17 minutes on this loaded sandbox before I skipped it — the exact case the playbook documents the flag for.training-agent-storyboards.ymlis the authoritative gate and runs in CI. Every other pre-push check ran: version sync, changeset policy, and the Mintlify checks (skipped, no docs changes). The follow-up push ran the hook with no overrides at all.Review
code-reviewerandsecurity-reviewerboth ran before this PR opened. Neither found a blocker; everything they raised is fixed or answered in the code. The substantive ones:git archiveis not the committed bytes. It applies the tag's own.gitattributes, soexport-substsplices the commit subject into its output — reproduced. Switched togit cat-file blob, which is attribute-immune and is whatbackfill-cdn-artifacts.shalready uses. Regression-tested.server/src/schemas-middleware.tsstamps the same immutable policy for an exact pinned hit, and the worker passes origin headers through on an R2 miss, so a fallback could have satisfied both the byte compare and the policy check. Verification now also requires theETagto equal the body's MD5, which only an R2 object produces. This was the one finding that would have made the verification step unsound in practice.generated_atfraction in the parity carve-out — that field's bytes are the only ones the signature does not cover, so an unbounded value could have been published permanently under the no-overwrite rule. Bounded by shape and by an absolute byte delta.created + verified == totalasserted before reporting success.IGNORECASEportability trap (removed with the old probe), and theadcp-<version>root directory documented as the load-bearing version binding, since cosign's identity regexp binds the workflow and not the version.Operational caveats are documented in
RELEASING.md: the run is slow (per-object fencing, as release publication does), a branch move can fail a run that already succeeded, and thev3/v3.0alias moves as soon as the first object lands — the same progressive-publication windowrelease.ymlalready has.After merge
This only adds the capability. Recovering 3.0.27 and 3.0.28 is a separate maintainer dispatch and is deliberately not performed here.
🤖 Generated with Claude Code
Open workspace in Conductor