feat(labeler): aggregator service binding + read-only client (W8.4 slice-3 foundation)#2070
Conversation
Add the AGGREGATOR service binding to the labeler Worker and a typed, read-only client over the aggregator's XRPC surface (getPackage, getLatestRelease, listReleases). Fetch-over-binding matching the perf-monitor pattern; the aggregator stays HTTP/XRPC-only with no RPC entrypoint. Each method does exactly one fetch per call (no caching/retry/memoization, per W10.4), sends no atproto-accept-labelers header so internal reads see the unfiltered view, and URL-encodes every param value against fixed-constant NSIDs. XRPC NotFound resolves to null; other non-2xx and transport failures throw. Stub the binding in vitest.config.ts so the workerd pool can start.
…red aggregator reads Omitting the header applies the aggregator's default trusted-labeler redaction, so a redacted subject presents as NotFound/null — silently blinding the labeler's own analysis to the subjects it most needs to see. Send a blank value to opt out. Documents the analysis-only boundary and the checksum re-verification downstream.
|
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 924 lines across 5 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. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | f135583 | Jul 16 2026, 11:53 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | f135583 | Jul 16 2026, 11:53 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | f135583 | Jul 16 2026, 11:54 AM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR lands a small, well-scoped foundation: a read-only AggregatorClient for the labeler that reaches the aggregator over a Cloudflare service binding, plus the binding declaration, generated types, and a stub for tests. The approach mirrors the existing infra/perf-monitor pattern and the code is careful about fixed NSIDs, URL-encoding all caller-supplied query values, mapping NotFound to null, and throwing everything else.
The blank-atproto-accept-labelers design is also correctly reasoned: the aggregator’s request-policy.ts treats an absent header as “use the deployment’s default trusted-labeler redaction” and an empty header as “no accepted labelers → no redaction,” so the labeler must send "" to avoid being blinded to redacted subjects. That logic is protected by existing aggregator tests, but only through SELF.fetch (an in-process Request), not the actual binding transport. This is the main open correctness gap.
I checked the changed files, the aggregator read/policy handlers, the registry-lexicons types, the test coverage, and the generated-type diff. pnpm typecheck/pnpm test are claimed clean and I have no static reason to doubt them. I do not think this should block merge, but it should not move to consumer wiring until the binding-level header survival is proven.
Headline: solid foundation, but the critical transport property is still assumed, not demonstrated.
| /** One fetch, no retry. `NotFound` → `null`; any other non-2xx or a | ||
| * transport failure throws. */ | ||
| async #query<T>(nsid: string, url: string): Promise<T | null> { | ||
| const response = await this.#fetcher.fetch(url, { | ||
| headers: { [ACCEPT_LABELERS_HEADER]: "" }, |
There was a problem hiding this comment.
[needs fixing] The unfiltered-read strategy depends on an empty-valued atproto-accept-labelers header surviving the service-binding transport. The aggregator’s own tests prove the parser handles an empty header correctly, but they use SELF.fetch, which is an in-process Request. They do not prove the header is still present after the labeler calls env.AGGREGATOR.fetch(url, { headers: { [ACCEPT_LABELERS_HEADER]: "" } }).
If Cloudflare’s service-binding transport drops empty-valued headers, the aggregator sees an absent header, falls back to its default trusted-labeler redaction, and redacted/taken-down subjects return NotFound → null. The labeler would then be silently blinded to exactly the subjects it most needs to see.
Add a workerd-level test in apps/labeler/test that exercises the real env.AGGREGATOR binding (or a serviceBindings stub in vitest.config.ts) and asserts the inbound Request carries atproto-accept-labelers: "".
| if (response.ok) { | ||
| return response.json<T>(); |
There was a problem hiding this comment.
[suggestion] The client casts the response body directly to the registry-lexicons type via response.json<T>() without runtime validation. The module doc explains this away because the aggregator is a first-party binding and downstream code verifies artifact checksums, but the view shape itself (field presence, cursors, etc.) is never checked at the trust boundary. A malformed or unexpectedly-shaped response could propagate into labeler analysis code before any checksum verification runs.
Consider validating against the generated output schema before returning, at least for the fresh-read path:
const raw = await response.json();
const view = AggregatorGetPackage.mainSchema.output.schema.parse(raw);
return view as AggregatorDefs.PackageView;This would turn aggregator contract violations into explicit, catchable parse errors at the client boundary.
| */ | ||
|
|
||
| import type { AggregatorDefs, AggregatorListReleases } from "@emdash-cms/registry-lexicons"; | ||
|
|
There was a problem hiding this comment.
[suggestion] ACCEPT_LABELERS_HEADER is defined locally here and also locally in apps/aggregator/src/routes/xrpc/request-policy.ts. Since @emdash-cms/registry-moderation is already a dependency of the labeler app and owns the labeler-header parsing logic, consider exporting the constant from packages/registry-moderation/src/labeler-headers.ts and importing it in both places so the two sides of the binding cannot drift out of sync.
github-actions
Bot
added
review/awaiting-author
…ator binding transport
github-actions
Bot
added
review/needs-rereview
|
Addressed the review:
Declining the two suggestions, with rationale:
~ 🤖 Claude Fable 5 |
There was a problem hiding this comment.
Approach judgment: this is the right foundation for the slice. The labeler needs a first-party, read-only path to the aggregator, and a fetch-over-service-binding client is the idiomatic Cloudflare pattern (mirroring infra/perf-monitor). The deliberately blank atproto-accept-labelers header is correctly reasoned: the aggregator's request-policy.ts treats an absent header as "use the deployment's default trusted-labeler redaction" and an empty header as "no accepted labelers → no redaction", so the labeler must send "" to avoid being blinded to redacted subjects. The code is careful about fixed NSIDs, URL-encoding every caller-supplied value, mapping only NotFound → null, and throwing everything else.
What's improved since the prior review: the PR now adds an AGGREGATOR service-binding stub in vitest.config.ts and a binding-transport test that proves miniflare preserves the empty-valued header. That closes the local-runtime gap.
Remaining concerns (non-blocking for this foundation):
- The binding-transport test still only exercises miniflare's stub, not the actual
workerdservice-binding hop. The PR description already flags this as a follow-up for consumer wiring, but the unfiltered-read contract truly depends on the header surviving production transport. - Regenerating
worker-configuration.d.tsfor the single newAGGREGATORbinding pulled in a large amount of unrelated workerd runtime type churn, making the diff noisier than necessary. listReleasespasses an empty cursor string through ascursor=, which the aggregator will decode as a malformed cursor and reject with 400.
Overall: solid, scoped foundation. Don't move to consumer wiring without a real-binding integration test that proves the blank header survives the service-binding hop in workerd.
| serviceBindings: { | ||
| AGGREGATOR: (request) => { | ||
| const raw = request.headers.get("atproto-accept-labelers"); | ||
| const marker = raw === null ? "absent" : raw === "" ? "empty" : `value:${raw}`; | ||
| return new Response("AGGREGATOR is stubbed in tests", { | ||
| status: 501, | ||
| headers: { "x-test-accept-labelers": marker }, | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[suggestion] The isolated AGGREGATOR stub and the AGGREGATOR binding transport test confirm that miniflare preserves an empty-valued atproto-accept-labelers header, but they don't exercise the actual workerd service-binding hop. The PR description already flags the follow-up ("verify via a real-binding integration test that the blank header survives the service-binding transport"), and it remains a correctness assumption: if the production binding drops or normalizes empty-valued headers, the aggregator falls back to default redaction and the labeler is silently blinded to the redacted subjects it most needs to see. Non-blocking for this foundation, but make sure a real-binding/wrangler integration test lands before this client is wired into any consumer that depends on the unfiltered view.
| cursor?: string, | ||
| ): Promise<AggregatorListReleases.$output | null> { | ||
| const params: Record<string, string> = { did, package: pkg }; | ||
| if (cursor !== undefined) params["cursor"] = cursor; |
There was a problem hiding this comment.
[suggestion] listReleases only omits cursor when the argument is undefined; an empty string is still passed through as cursor=. The aggregator's decodeListCursor treats a provided empty value as malformed and returns 400 InvalidRequest (only an absent cursor means "first page"). Consider treating an empty cursor the same as no cursor:
| if (cursor !== undefined) params["cursor"] = cursor; | |
| if (cursor) params["cursor"] = cursor; |
Or, if callers are expected to only pass opaque aggregator cursors, document that in the method's JSDoc.
| // Generated by Wrangler by running `wrangler types` (hash: ccfd2a4d903b4c76107d4323cc123869) | ||
| // Runtime types generated with workerd@1.20260611.1 2026-02-24 nodejs_compat | ||
| // Generated by Wrangler by running `wrangler types` (hash: e21c438219f0e3a8c154bb09b450524b) | ||
| // Runtime types generated with workerd@1.20260708.1 2026-02-24 nodejs_compat |
There was a problem hiding this comment.
[suggestion] Regenerating this file for the single new AGGREGATOR binding pulled in a lot of unrelated workerd runtime type churn (new jurisdiction hints, image-transformation field reordering, ExecutionContext/Workflow changes, etc.). The only semantic addition is AGGREGATOR: Fetcher. If the new runtime types are required by a wrangler/version bump in the base branch, that's fine, but consider keeping runtime type updates in a separate dependency/tooling PR so this foundation diff stays focused on the binding and client.
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Approach judgment: This is the right foundation for the W8.4 slice and W10.4 read path. The aggregator is HTTP/XRPC-only, so a fetch-over-service-binding client is the idiomatic EmDash/Cloudflare pattern. The deliberately blank atproto-accept-labelers header is correctly reasoned: the aggregator's request-policy.ts resolves a missing header to the deployment's default trusted-labeler policy (redact: true), while parseAcceptLabelersHeader("") returns an empty accepted set, which filterAvailable keeps empty and which label-enforcement.ts treats as a no-op, so only a blank value gives the labeler the genuinely unfiltered view it needs for moderation analysis. The re-exposure risk is bounded at the serving layer, consistent with the ratified sync.getRecord decisions.
What's improved since the prior review: The empty-cursor bug is fixed (if (cursor) now prevents sending cursor= to listReleases), and the PR adds a miniflare serviceBindings stub plus a binding-transport test that proves an empty-valued header survives at least the local runtime hop.
What I checked: I read the new client and tests, the vitest and wrangler config changes, the generated type file diff, and the aggregator's request-policy.ts / label-enforcement.ts to verify the blank-header semantics. I confirmed the package is private so no changeset is required. No SQL, UI, localization, or public API surface is involved.
Headline conclusion: Clean, scoped foundation. No blocking issues. I do have two non-blocking observations to keep on record:
- Production transport caveat remains: the binding-transport test still exercises miniflare only; the real concern is whether
workerddrops empty-valued headers on an actual service-binding hop. You call this out already as a follow-up for the consumer-wiring slice, and it remains the right place to prove the contract with a real-binding integration test. - Generated-type churn: regenerating
worker-configuration.d.tsfor one binding pulled in a lot of unrelated workerd runtime churn. See the anchored suggestion.
Recommended: ship this slice, but don't move to consumer wiring without the real-binding integration test that proves the blank header survives in workerd.
| NOTIFICATION_HASH_PEPPER: string; | ||
| LABEL_SUBSCRIPTION: DurableObjectNamespace<import("./src/index").LabelSubscriptionDO>; | ||
| LABELER_DISCOVERY_DO: DurableObjectNamespace<import("./src/index").LabelerDiscoveryDO>; | ||
| AGGREGATOR: Fetcher /* emdash-aggregator */; |
There was a problem hiding this comment.
[suggestion] The only intentional change in this generated file is the new AGGREGATOR binding, but the regeneration also pulled in unrelated workerd runtime type churn (ExecutionContext.tracing accessor change, new CloudflareAccessContext, extra DO jurisdictions/location hints, DurableObjectFacets.clone, container exec types, etc.).
This makes the diff noisier than necessary and blurs scope discipline. If feasible, regenerate against the same workerd version the integration branch already uses, or commit only the AGGREGATOR binding line plus the header hash update; revert the rest so the diff stays focused on this slice.
github-actions
Bot
added
review/approved
ascorbic
merged commit 201cdc0
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Adds the read-only labeler→aggregator service binding and a typed client — the foundation the ratified W8.4 slice-3 read path (and W10.4 slice A contact resolution) build on. No consumer wiring yet; this lands the mechanism.
The aggregator is an HTTP/XRPC-only module Worker (no RPC entrypoint), so consumption is fetch-over-binding, matching the repo's one existing service-binding example (
infra/perf-monitor).AggregatorClientwraps theAGGREGATORFetcherwith three methods over the existing XRPC reads —getPackage,getLatestRelease,listReleases— typed against@emdash-cms/registry-lexiconsview types. NSIDs are fixed per-method constants; only query-param values carry caller data and every value isencodeURIComponent-encoded, so no caller data can reach the host/path. One fetch per call, no caching (honors W10.4's fresh-read-at-notification rule). XRPCNotFound→null; any other non-2xx or a transport failure throws (callers treat a throw as transient).Moderation-semantics note (flagged for maintainer): the client sends a blank
atproto-accept-labelersheader to obtain a genuinely unfiltered view. This is deliberate and counter-intuitive: omitting the header makes the aggregator apply its default trusted-labeler redaction, so a redacted/taken-down subject returnsNotFound→nulland silently blinds the labeler's own analysis to the subjects it most needs to see. The labeler is the moderation authority performing analysis, so it must read unfiltered. The re-exposure risk (an unfiltered read reaching a public surface) is bounded at the serving layer, not here — see the ratified sync.getRecord / artifact-mirror serving decisions. This was caught by an adversarial review pass (the original code omitted the header under a comment claiming the opposite).Part of the plugin-registry labelling service (RFC #694, umbrella #1909). Targets the integration branch.
Type of change
Covered by the approved RFC #694 umbrella.
Checklist
pnpm typecheckpassespnpm lintpasses (zero diagnostics in changed files; pre-existing baseline unchanged)pnpm testpasses (aggregator-client 11/11; full labeler suite green)pnpm formathas been run (touched paths; worktree path is gitignored so local oxfmt skips it — CI format-checks the branch checkout)@emdash-cms/labeleris unpublished (private: true)AI-generated code disclosure
Screenshots / test output
vitest run test/aggregator-client.test.ts→ 11/11 pass (URL/param encoding per method, typed parse, NotFound→null, 5xx and transport-failure throw, and the blank-header assertion). A note for the consumer-wiring slices: verify via a real-binding integration test that the blank header survives the service-binding transport and yields the unfiltered view — if empty-valued headers are dropped in transport, escalate to an explicit aggregator internal-read affordance.Try 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/labeler-aggregator-binding. Updated automatically when the playground redeploys.