Skip to content

fix(release): recover a release's schema and compliance trees on the CDN - #8351

Merged
bokelley merged 4 commits into
mainfrom
fix/8347-recover-cdn-version-trees
Oct 20, 2026
Merged

bokelley merged 4 commits into
mainfrom
fix/8347-recover-cdn-version-trees

Conversation

@bokelley

@bokelley bokelley commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Closes #8347.

The bug

schemas/3.0.27/, schemas/3.0.28/, compliance/3.0.27/ and compliance/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 resolvePinnedFallback resolves a pinned semver with no published directory to the nearest release at-or-below it on the line, so right now:

$ curl -s https://adcontextprotocol.org/schemas/3.0.28/index.json | grep adcp_version
  "adcp_version": "3.0.26",

200 OK, wrong schemas, no error. /compliance/3.0.27/ and /compliance/3.0.28/ do the same. The only outward tells are cache-control: public, no-cache, must-revalidate instead of the immutable policy, and the ::warning title=Schema versions missing from the CDN that #8324's pointer builder now logs on every deploy while it keeps v3.0 pinned to 3.0.26.

Nothing could fix it. release.yml publishes only the current checkout's package version (check-release-state.cjs committed requires the local, tested and original committed versions to be equal); --latest-only excludes every semver path; live bulk backfill is disabled (#7507/#7510). RELEASING.md told that warning's reader to "recover those releases per RELEASING.md", and then said the stable-only recover-protocol-cdn.yml "recovers four protocol files, not schemas/compliance".

The fix

recover-protocol-cdn.yml gains a surfaces choice input. protocol is the default and is byte-identical to today's behaviour; all additionally recovers the immutable schemas/<version>/ and compliance/<version>/ trees via scripts/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.cjs stages dist/protocol/<v>.tgz by copying dist/schemas/<v> and dist/compliance/<v> verbatim, so scripts/check-protocol-bundle-parity.cjs proves 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 timestamp pinGeneratedAt re-pins in schemas/manifest.json and compliance/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:

  • the fence, parity checker and recovery script are copied to $RUNNER_TEMP before git checkout --detach v<version>, so recovery logic never comes from the artifact being recovered;
  • published bytes come from 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);
  • writes are --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;
  • the current-branch publication fence runs before and after every object;
  • then the public origin is fetched and every object must match byte-for-byte and be served with the immutable cache policy. Byte equality alone cannot tell a recovered directory from a fallback to a neighbour whose files happen to agree — the cache policy is the exactPinnedHit signal, 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 into npm run test:release-workflow) runs the real script against a stubbed aws/git and a real local HTTP origin answering real curl. Mutation-checked: removing the parity proof, the ETag column, the git cat-file read, 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 no index.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 because package.json is in FULL_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:

Stage Result
scripts/check-image-quality.sh pass
npm run test:unit 79 files, 1260 tests
npm run test:test-dynamic-imports pass
npm run test:format-identity-boundaries pass
npm run test:callapi-state-change pass
precommit:server-unit (6 shards) 773 files, ~12.9k tests
npm run typecheck pass

Nothing 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 because package.json is in STORYBOARD_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.yml is 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-reviewer and security-reviewer both ran before this PR opened. Neither found a blocker; everything they raised is fixed or answered in the code. The substantive ones:

  • git archive is not the committed bytes. It applies the tag's own .gitattributes, so export-subst splices the commit subject into its output — reproduced. Switched to git cat-file blob, which is attribute-immune and is what backfill-cdn-artifacts.sh already uses. Regression-tested.
  • The cache-policy check was not sufficient on its own. server/src/schemas-middleware.ts stamps 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 the ETag to 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.
  • No curl timeouts, so a stalled server could hang the job forever and make the retry loop dead code. Bounded.
  • Unbounded generated_at fraction 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.
  • Dropped backfill controls restored: per-object digest re-verification between the parity proof and each write, and a content-type assertion on objects that already exist (right bytes under the wrong type is the same silent-wrong-answer failure, and no-overwrite means it cannot be repaired here).
  • created + verified == total asserted before reporting success.
  • Also: NUL-safe path validation, bucket-name validation, the awk IGNORECASE portability trap (removed with the old probe), and the adcp-<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 the v3/v3.0 alias moves as soon as the first object lands — the same progressive-publication window release.yml already 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

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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@aao-secretariat aao-secretariat Bot added the ladon/needs-human-review Ladon has escalated this PR for human review. label Oct 19, 2026
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>
bokelley and others added 2 commits October 19, 2026 23:38
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>

Copy link
Copy Markdown
Contributor Author

Thanks — addressed the non-blocking follow-up in 6a4cf17: the upload loop's head-object, s3 cp and put-object calls now carry explicit --cli-connect-timeout 20 --cli-read-timeout 120, matching the bound already on the verification fetches. The loop makes three S3 calls per object across ~770 objects, so a stalled socket on CLI defaults would have hung publication rather than failing it.

On the escalation: agreed, and expected. .github/workflows/recover-protocol-cdn.yml is the point of the change, so the CODEOWNERS gate is the correct outcome — this needs a human approval before merge.

Two things a reviewer may want to know:

  • Nothing is recovered by merging this. It only adds the capability. Recovering 3.0.27 and 3.0.28 is a separate maintainer dispatch, deliberately not performed here.
  • The dispatch, once approved, is gh workflow run recover-protocol-cdn.yml --ref main -f version=3.0.27 -f surfaces=all, then the same for 3.0.28. surfaces defaults to protocol, so existing dispatches are unchanged.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@bokelley
bokelley merged commit f5f9aed into main Oct 20, 2026
60 checks passed
@bokelley
bokelley deleted the fix/8347-recover-cdn-version-trees branch October 20, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ladon/needs-human-review Ladon has escalated this PR for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No recovery path for schemas/<version> and compliance/<version> trees missing from the artifact CDN (3.0.27, 3.0.28)

2 participants