feat(release-service): isolate verification egress#1988
Conversation
🦋 Changeset detectedLatest commit: 638f9df The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 638f9df | Jul 13 2026, 06:28 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 638f9df | Jul 13 2026, 06:29 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 638f9df | Jul 13 2026, 06:30 AM |
Scope checkThis PR changes 15,129 lines across 23 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
900b4c9 to
c494fc0
Compare
There was a problem hiding this comment.
This PR adds the isolated release-verifier Worker and the release-service client adapter, which moves artifact/provenance egress out of the public release-service Worker — exactly the shape described in the W5.1a plan. The change is well-scoped: it introduces a new private app, a registry-verification/fetch Worker-safe entry point, a typed service-binding RPC interface, and matching workerd/Node tests. No EmDash core conventions (Lingui, RTL, D1 queries, content-table locale filtering, etc.) are touched.
I checked the new verifier Worker code, the service-binding client in apps/release-service/src/verifier.ts, the registry-verification packaging changes (fetch-entry.ts, tsdown.config.ts, check-packed-output.mjs), the wrangler configs, generated Wrangler types, and the existing fetch.ts implementation. The code is generally clean and correctly fails closed: malformed RPC results, binding rejections, and validation failures are all normalized to a retryable VerifierUnavailableError.
One real gap and one minor suggestion:
- Provenance size vs. RPC message limit.
PROVENANCE_MAX_BYTESis set to exactly1024 * 1024, but the generated worker types repeat a 1MiB Workers RPC message limit. The verifier returns provenance bytes wrapped in aVerificationResult<Uint8Array>over a service-binding RPC call, so the envelope adds overhead. A provenance that is legal under the current constant could still fail to serialize across the RPC boundary. - Observability of verifier failures. The
VerifierUnavailableErrorcatch discards the original RPC/validation error, which will make production "verifier unavailable" incidents hard to diagnose. Preserving the cause on the error object helps logs while keeping client responses unchanged.
I did not run the test suite or build; that claim is unverified from this review.
| import { WorkerEntrypoint } from "cloudflare:workers"; | ||
|
|
||
| export const ARTIFACT_MAX_BYTES = 384 * 1024; | ||
| export const PROVENANCE_MAX_BYTES = 1024 * 1024; |
There was a problem hiding this comment.
[needs fixing] PROVENANCE_MAX_BYTES = 1024 * 1024 sets the provenance cap flush against the Workers RPC message limit noted in the generated worker types ("RPC limit for the batch (1MiB)"). The verifier returns provenance bytes over a typed service-binding RPC as { success: true, value: Uint8Array }, so the wrapper object and RPC envelope add bytes beyond the raw payload. A provenance file that is near but under this limit can pass the safe-fetch size check yet fail to serialize across the RPC boundary, producing an opaque transport error instead of a clean VerificationResult.
Leave headroom for the envelope, and ideally express the constant relative to a documented RPC limit rather than a bare literal:
| export const PROVENANCE_MAX_BYTES = 1024 * 1024; | |
| export const PROVENANCE_MAX_BYTES = 1024 * 1024 - 1024; |
(A named constant like WORKERS_RPC_MESSAGE_LIMIT_BYTES would be even clearer.)
| fetchProvenance(url: string): Promise<unknown>; | ||
| } | ||
|
|
||
| export class VerifierUnavailableError extends Error { |
There was a problem hiding this comment.
[suggestion] The catch in callVerifier replaces every binding/validation failure with a fresh VerifierUnavailableError, throwing away the original cause. That matches the "fail closed" design, but when this bubbles up in production logs there will be no trace of why the verifier was considered unavailable — RPC transport error, malformed response, version-skewed error code, etc.
Consider stashing the original error as cause:
export class VerifierUnavailableError extends Error {
readonly retryable = true;
constructor(cause?: unknown) {
super("Release verifier is unavailable", { cause });
this.name = "VerifierUnavailableError";
}
}and then rethrowing new VerifierUnavailableError(error). This keeps the client response unchanged while giving operators something actionable in the logs.
…into feat/delegated-release-service-16-verifier-worker
|
Addressed both review points in
Verification after updating to integration head ~ Sol 🤖 |
There was a problem hiding this comment.
This is a re-review of the isolated verifier Worker PR. The two findings from the previous review are now addressed:
apps/release-verifier/src/index.tsnow caps provenance bytes at960 * 1024, leaving headroom below the 1 MiB Workers RPC message limit, and the constant/test comment explains the envelope overhead.apps/release-service/src/verifier.tsnow passes the original RPC/validation error as thecauseofVerifierUnavailableError, and tests assert the cause is preserved across malformed responses and binding rejections.
The overall approach is sound: moving artifact/provenance egress to a private Worker behind a typed service binding is exactly the shape described in W5.1a, and the new registry-verification/fetch entry point lets the verifier reuse the existing bounded fetch implementation in Worker-safe packaging.
One real issue remains: the release-service RPC adapter validates verifier error results against a hardcoded allowlist of VERIFICATION_ERROR_CODES. A newer verifier that returns a valid { success: false, error: { code, message } } with a code added after this client was deployed is classified as “malformed” and rethrown as a retryable VerifierUnavailableError. That turns a definitive verification failure into a transient, retryable service error and breaks forward compatibility across the service boundary. The RPC boundary already guarantees the object shape; the client should only validate that code and message are strings, not enumerate every possible code.
I did not run builds or tests; the author’s claim that pnpm build, pnpm typecheck, and suite runs passed is unverified from this review.
| function isVerificationError(value: unknown): value is VerificationError { | ||
| return ( | ||
| isRecord(value) && | ||
| typeof value["code"] === "string" && | ||
| VERIFICATION_ERROR_CODE_SET.has(value["code"]) && |
There was a problem hiding this comment.
[needs fixing] The adapter treats a well-formed { success: false, error: { code, message } } result as malformed whenever the verifier uses an error code not in the client’s hardcoded VERIFICATION_ERROR_CODE_SET. That turns forward-compatible new failure codes (version skew, new policy checks, etc.) into a retryable VerifierUnavailableError, masking a definitive verification failure as transient verifier unavailability and defeating the purpose of a separated service boundary.
The typed service binding already guarantees the result shape, so validate only the shape here, not the specific code.
| function isVerificationError(value: unknown): value is VerificationError { | |
| return ( | |
| isRecord(value) && | |
| typeof value["code"] === "string" && | |
| VERIFICATION_ERROR_CODE_SET.has(value["code"]) && | |
| function isVerificationError(value: unknown): value is VerificationError { | |
| return isRecord(value) && typeof value["code"] === "string" && typeof value["message"] === "string"; | |
| } |
Then remove the now-dead VERIFICATION_ERROR_CODE_SET constant (line 8) and the VERIFICATION_ERROR_CODES import (line 5).
| it.each([ | ||
| null, | ||
| { success: true, value: "not bytes" }, | ||
| { success: false, error: { code: "NEW_VERSION_CODE", message: "version skew" } }, |
There was a problem hiding this comment.
[needs fixing] This test case labels a valid version-skew error result as a “malformed RPC result.” A { success: false, error: { code: "NEW_VERSION_CODE", message: "version skew" } } response has exactly the shape the adapter should pass through; it is not malformed and should not become a VerifierUnavailableError.
Move it out of the malformed list and assert that the adapter returns the error result unchanged, instead of normalizing it to a retryable verifier-unavailable error.
There was a problem hiding this comment.
This PR correctly isolates artifact/provenance egress to a private verifier Worker behind a typed service binding, which is the right shape for W5.1a. The approach reuses the existing bounded fetch implementation via the new @emdash-cms/registry-verification/fetch entry point instead of duplicating it, and the service-binding adapter fails closed on binding or malformed-response errors.
The two prior blocking concerns are resolved:
apps/release-verifier/src/index.tscaps provenance at960 * 1024bytes, with a test and comment explaining the headroom below the Workers RPC 1 MiB limit.apps/release-service/src/verifier.tswraps every non-verifier error inVerifierUnavailableErrorwith the original RPC/validation error preserved ascause, and tests assert that preservation.- The previous allowlist-of-error-codes problem is gone: the adapter now only validates that
{ success: false, error: { code, message } }has stringcode/message, so a newer verifier can return new definitive failure codes without being misclassified as malformed/retryable.
I read the changed files and tests statically; I did not run builds, typecheck, lint, or tests. The author’s claim that those pass is unverified from this review.
One small cleanup remains: apps/release-service/package.json adds a dependency on @emdash-cms/registry-verification that the release-service code never imports. The verifier app uses it, and the service-binding types are generated from the verifier’s Wrangler config, so it appears to be dead weight in the release-service package.
| }, | ||
| "dependencies": { | ||
| "@emdash-cms/registry-verification": "workspace:*", | ||
| "jose": "^6.1.3" |
There was a problem hiding this comment.
[suggestion] The added @emdash-cms/registry-verification dependency is not imported anywhere in apps/release-service (the verifier app imports it, and the service-binding types are generated from ../release-verifier/wrangler.jsonc). It adds unused weight to the lockfile and is out of scope for this package.
| "jose": "^6.1.3" | |
| "dependencies": { | |
| "jose": "^6.1.3" | |
| } |
There was a problem hiding this comment.
Re-review. The isolation approach remains correct: outbound artifact/provenance fetches now happen only in the private release-verifier Worker, accessed from release-service through a typed service binding, and failures are normalized to a retryable VerifierUnavailableError while definitive verifier failures pass through unchanged.
The one cleanup from the previous review is done: apps/release-service/package.json no longer declares an unused runtime dependency on @emdash-cms/registry-verification; only the new release-verifier app depends on the package.
I read the changed files and tests statically; I did not run builds, typecheck, lint, or tests. No new correctness, security, or convention issues stand out in this revision.
What does this PR do?
Adds the private release-verifier Worker for W5.1a of the delegated release service. Artifact and provenance retrieval now crosses a typed service-binding RPC boundary, keeping outbound network access out of the public release-service Worker and failing closed when the verifier is unavailable or returns malformed data.
The Worker reuses the registry-verification package's bounded safe-fetch implementation through a new Worker-safe export, with fixed DoH resolution, redirect revalidation, timeout limits, and separate artifact/provenance size limits.
Part of #1908.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Non-visual change. Verified after rebasing onto the current integration branch:
pnpm buildpnpm typecheckpnpm lintandpnpm lint:jsonTry this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
feat/delegated-release-service-16-verifier-worker. Updated automatically when the playground redeploys.