refactor(server)!: XRPC endpoint hosting — route groups, one NSID source, correct binding #153

Grandiras · 2026-09-24T19:48:26Z

Grandiras commented

2026-09-24 19:48:26 +00:00

Closes #121.

Server-side XRPC hosting (ATProtoNet.Server/Xrpc): route groups, one NSID source, binding by property type, one error path.

What changed

`MapXrpcEndpoints()` returns the `/xrpc` route group

It returns RouteGroupBuilder (from MapGroup("/xrpc")), so .RequireAuthorization(), .RequireRateLimiting(…), .RequireCors(…) and .WithMetadata(…) apply to every XRPC endpoint.
Each endpoint also carries its handler class's attributes as metadata (GetCustomAttributes(inherit: true)). That makes [Authorize], [AllowAnonymous], [EnableRateLimiting] and [RequestSizeLimit] work per handler, and [AllowAnonymous] exempts a handler from a group-wide RequireAuthorization().
Endpoints are plain RequestDelegate endpoints (MapMethods(pattern, [method], RequestDelegate)), so no minimal-API delegate binding runs.

One source of truth for the NSID

IXrpcEndpoint is now static abstract Nsid Nsid { get; } (typed ATProtoNet.Identity.Nsid). Registration reads THandler.Nsid directly. The RuntimeHelpers.GetUninitializedObject lookup is gone, along with its failure on auto-property and field initializers (regression test: AddXrpcEndpoint_AutoPropertyNsid_IsRead).
[XrpcEndpoint] is removed. AddXrpcEndpointsFromAssembly registers every non-abstract, non-generic class implementing IXrpcEndpoint by invoking AddXrpcEndpoint<T> itself, so the two paths are the same code. A test scans the Server assembly and checks that the result equals what AddSpaceAuthority + AddSimpleSpace + AddSpaceRepoHost register (19 endpoints, same NSID, method and handler).
Registration fails fast when a handler implements zero or several endpoint interfaces, when its NSID is null, or when a second handler claims an NSID. Registering the same handler twice is idempotent (TryAddScoped).

Query binding by target property type (X1, F25)

New XrpcQueryBinder<TParams>. It reads the serializer's own contract once per type (wire names, property types, required, property-level converters). Per request it writes the query directly into a reused per-thread UTF-8 buffer as the JSON the contract expects, then deserializes from the span. There is no Dictionary, no polymorphic object serialization and no intermediate JSON string.
A collection property (list, IReadOnlyList<T>, array) always gets an array: ?uris=x binds a one-element list. Repeated keys bind in order. uris[]= is accepted leniently (collections only). A repeated scalar is refused, to avoid parameter-pollution ambiguity.
bool binds without XrpcBooleanConverter. A property that carries a converter still gets the string, as before. Identifier types (Did, Nsid, AtUri, …) go through their own IdentifierJsonConverter, and a missing required parameter or any unbindable value answers InvalidRequest naming the parameter. The serializer's message, which has CLR type names and byte positions, is no longer echoed.
Constructor-bound types (positional records) bind as they do from a body.
Contracts are resolved lazily (on the first request), not at registration: a contract snapshots LexiconTypeRegistry union variants, and an app may register those after its endpoints.

One exception filter, typed error body

XrpcErrorResponse.Handle wraps every endpoint delegate:
- XrpcException answers with its status, name, message and Headers.
- BadHttpRequestException answers with its status; 413 maps to PayloadTooLarge, anything else to InvalidRequest.
- A client abort is swallowed.
- Anything else answers 500 {"error":"InternalServerError","message":"Internal Server Error"} and is logged ([LoggerMessage], category ATProtoNet.Server.Xrpc) with the exception. Its message never reaches the client.
The body is the internal typed XrpcErrorBody. Once the response has started (a blob failing halfway), the exception propagates so the server aborts the response rather than passing off a truncated body as complete.
A handler returning null output is a 500, where it used to write null with a 200.

Unmatched routes: `MethodNotImplemented` fallback

The /xrpc group now holds a fallback, MapFallback("/{nsid}", …), that answers what no endpoint matched, following the reference xrpc-server catch-all:

Request	Status	`error`
An NSID no handler serves	`501`	`MethodNotImplemented` (message `Method Not Implemented`)
A registered NSID with the wrong HTTP method	`405` + `Allow`	`InvalidRequest` (`Incorrect HTTP method (POST), expected GET.`)
A segment that is not an NSID (`/xrpc/foo`)	`400`	`InvalidRequest`

The 405 case has to be written by the fallback. Routing prefers an endpoint that accepts any method (the fallback) over producing its own 405, so once a fallback exists the router's 405 never happens. The fallback checks whether the NSID is registered (case-insensitively, like route matching) and answers 405 with Allow and an XRPC body. The status stays 405 as before; the error name is InvalidRequest, which is what the reference uses for this case (it answers with status 400).
No shadowing. MapFallback orders the fallback at int.MaxValue, after every other endpoint, and it matches only one segment below /xrpc. Registered endpoints and routes the app maps under /xrpc itself (e.g. /xrpc/_health) still answer. /other/…, /com.x.y and /xrpc/a/b remain plain 404s.
Group conventions apply to the fallback too. With .RequireAuthorization() on the group, an anonymous caller probing an unknown NSID gets 401, not a map of which methods exist.
The invalid-segment → 400 row goes slightly beyond "every unclaimed NSID answers 501". It mirrors the reference's invalid xrpc path error; switch it to 501 if you prefer.

Interface gaps

IXrpcProcedure<TOutput>: no input, JSON output.
IXrpcBlobProcedure<TOutput> + XrpcBlobInput(Stream Content, string ContentType, long? ContentLength): an unbuffered request body, for blob-upload-style methods. A missing Content-Type answers InvalidRequest, as in the reference xrpc-server.
Also added IXrpcProcedureVoid (no input, no output). This goes beyond the issue text but fills the same "procedure with no input" gap for deleteSession-shaped methods; see the flag below.
I named the binary shape IXrpcBlobProcedure / XrpcBlobInput, not the research's IXrpcBinaryProcedure, to match the existing IXrpcBlobQuery / XrpcBlobResult.

One generic registration path (debloat 6.1)

The 5 reflection wrappers, the 5-branch type check, the dead Kind enum, and the EnsureRegistry/GetOrCreateRegistry double registration are replaced by one table of shape → (HTTP method, generic factory). There is exactly one MakeGenericMethod per registration, at startup. Requests run with no reflection.
Blob query output streams straight to the response with CopyToAsync, instead of through Results.Stream.

Spaces endpoints (debloat 6.2)

Mechanical: each of the 19 handlers drops [XrpcEndpoint(Nsid = SpaceNsids.X)] and turns public string Nsid => SpaceNsids.X; into public static Nsid Nsid { get; } = Nsid.Parse(SpaceNsids.X);.
NotifyWriteEndpoint passes SpaceNsids.NotifyWrite to the service-auth check (it used the instance Nsid).
The now-false remark on SpaceNsids ("needed at attribute position") is removed. No parameter or model types changed.

Breaking changes (CHANGELOG has migration notes)

NSID declaration: delete [XrpcEndpoint(Nsid = …)] and replace public string Nsid => "…"; with public static Nsid Nsid { get; } = Nsid.Parse("…"); (using ATProtoNet.Identity;). The endpoint interfaces can no longer be generic type arguments (CS8920).
Assembly scanning: AddXrpcEndpointsFromAssembly picks up every IXrpcEndpoint class, and invalid or conflicting handlers throw at registration.
MapXrpcEndpoints() return type: it is now RouteGroupBuilder. app.MapXrpcEndpoints(); compiles unchanged. A Map… call chained onto the result would now map under /xrpc, so move it to the app.

Verification

dotnet build (whole solution): 0 warnings, 0 errors.
Unit tests: 2886 passed, 0 failed, 5 skipped (the skips are the existing Aspire manifest tests). XRPC hosting tests went from 10 to 68 test methods across three files:
- Unmatched routes: an unknown NSID answers 501 MethodNotImplemented for GET and POST. POST to a query and GET to a procedure answer 405 with Allow and an InvalidRequest body. A non-NSID segment answers 400. /other/…, /com.x.y and /xrpc/a/b stay 404. App-mapped /xrpc/_health and /xrpc/com.example.appOwned are not shadowed. Under group RequireAuthorization(), an unknown NSID answers 401 anonymous and 501 authenticated.
- SimpleSpaceEndpointTests.AddMember_IsNoLongerServed now expects 501 MethodNotImplemented for the removed addMember (it expected 404).
- XrpcEndpointTests: every shape end to end; body errors (no body, wrong content type, malformed JSON, wrong field type); blob upload hashing 70 KB; blob download of 100 KB with length; XrpcException status/headers; unknown exception → 500 with no secret in the body and one error log entry holding the exception; 413 → PayloadTooLarge; null output → 500.
- Registration tests: auto-property NSID; each shape's HTTP method; idempotent re-registration; two shapes, null NSID and NSID conflict throw; scan ≡ AddXrpcEndpoint<T>; Server-assembly scan ≡ Spaces Add* registrations; open generics skipped; MapXrpcEndpoints with nothing registered throws.
- XrpcQueryBindingTests: single value → list/array; repeated keys; [] keys; mixed; Did/Nsid/AtUri list binding and rejection; int/bool parse and rejection; List<bool>; repeated scalar refused; unknown keys ignored; JSON-escaping round trip; missing required; positional-record binding; 64 concurrent requests (thread-static buffer reuse).
- XrpcRouteGroupTests: group RequireAuthorization() (401 → 200 with auth), [AllowAnonymous] overriding it, handler [Authorize] without a group convention, group RequireRateLimiting (second call 429), and group WithMetadata reaching the endpoint.
- The existing Spaces endpoint suites (SpaceServerEndpointTests, SimpleSpaceEndpointTests) pass unchanged through the new pipeline.
Binding micro-benchmark (temporary test, not committed; Release, best of 5 × 200k, old = the previous Dictionary → JSON string → Deserialize):

Query Old New

listRecords, 6 scalars incl. a converter-bound bool 786 ns / 1424 B 549 ns / 456 B

2-element list + 2 scalars 743 ns / 1992 B 547 ns / 1016 B

Deferred / flagged

IXrpcProcedureVoid (non-generic) is extra public API beyond the issue checklist. It is small and fills the no-input/no-output gap; drop it if unwanted.
Not added: a binary-input procedure with no output (e.g. importRepo), and subscriptions (IXrpcSubscription, research 13.5). Neither is in the issue checklist.
401/403 from authorization and 429 from rate limiting are written by those middlewares, not in the XRPC envelope. The docs point to the scheme's challenge / the limiter's OnRejected.
XrpcBooleanConverter and the Spaces handler classes stay public; making them internal is #133 (13.7). Only the converter's remark was updated, since routing no longer needs it.
No trim/AOT annotations were added (the Server project has no trim analysis enabled). The remaining reflection is startup-only: one GetInterfaces + MakeGenericMethod per handler, plus assembly scanning.

Review follow-up

The first review (COMMENT) raised two findings. Both are fixed in the same commit.

NSID conflicts are now checked case-insensitively. XrpcEndpointRegistry.Add compared Nsid records ordinally, while routing and the fallback's lookup are case-insensitive. So com.example.simpleQuery and com.example.SimpleQuery both registered, and MapXrpcEndpoints() then crashed with an unhandled ArgumentException from the fallback's dictionary. The check is now OrdinalIgnoreCase and throws the usual InvalidOperationException, naming both handlers and both spellings. Test: AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict. The CHANGELOG bullet and the docs now say conflicts are compared case-insensitively.
The fallback now goes through the same exception wrapper as every endpoint. XrpcErrorResponse.Handle now takes any RequestDelegate rather than a registration. The unmatched-route fallback is wrapped exactly like the handlers, so an unexpected exception there answers the 500 envelope and is logged. Since the fallback has no single NSID, the log entry now names the request instead: XRPC request {Method} {Path} failed with an unhandled exception. Test: Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt.

After the fix, the build has 0 warnings and 0 errors, and unit tests show 2886 passed, 0 failed, 5 skipped.

🤖 Generated with Claude Code

Closes #121. Server-side XRPC hosting (`ATProtoNet.Server/Xrpc`): route groups, one NSID source, binding by property type, one error path. ## What changed ### `MapXrpcEndpoints()` returns the `/xrpc` route group - It returns `RouteGroupBuilder` (from `MapGroup("/xrpc")`), so `.RequireAuthorization()`, `.RequireRateLimiting(…)`, `.RequireCors(…)` and `.WithMetadata(…)` apply to every XRPC endpoint. - Each endpoint also carries its handler class's attributes as metadata (`GetCustomAttributes(inherit: true)`). That makes `[Authorize]`, `[AllowAnonymous]`, `[EnableRateLimiting]` and `[RequestSizeLimit]` work per handler, and `[AllowAnonymous]` exempts a handler from a group-wide `RequireAuthorization()`. - Endpoints are plain `RequestDelegate` endpoints (`MapMethods(pattern, [method], RequestDelegate)`), so no minimal-API delegate binding runs. ### One source of truth for the NSID - `IXrpcEndpoint` is now `static abstract Nsid Nsid { get; }` (typed `ATProtoNet.Identity.Nsid`). Registration reads `THandler.Nsid` directly. The `RuntimeHelpers.GetUninitializedObject` lookup is gone, along with its failure on auto-property and field initializers (regression test: `AddXrpcEndpoint_AutoPropertyNsid_IsRead`). - `[XrpcEndpoint]` is removed. `AddXrpcEndpointsFromAssembly` registers every non-abstract, non-generic class implementing `IXrpcEndpoint` by invoking `AddXrpcEndpoint<T>` itself, so the two paths are the same code. A test scans the Server assembly and checks that the result equals what `AddSpaceAuthority` + `AddSimpleSpace` + `AddSpaceRepoHost` register (19 endpoints, same NSID, method and handler). - Registration fails fast when a handler implements zero or several endpoint interfaces, when its NSID is null, or when a second handler claims an NSID. Registering the same handler twice is idempotent (`TryAddScoped`). ### Query binding by target property type (X1, F25) - New `XrpcQueryBinder<TParams>`. It reads the serializer's own contract once per type (wire names, property types, `required`, property-level converters). Per request it writes the query directly into a reused per-thread UTF-8 buffer as the JSON the contract expects, then deserializes from the span. There is no `Dictionary`, no polymorphic `object` serialization and no intermediate JSON string. - A collection property (list, `IReadOnlyList<T>`, array) always gets an array: `?uris=x` binds a one-element list. Repeated keys bind in order. `uris[]=` is accepted leniently (collections only). A repeated scalar is refused, to avoid parameter-pollution ambiguity. - `bool` binds without `XrpcBooleanConverter`. A property that carries a converter still gets the string, as before. Identifier types (`Did`, `Nsid`, `AtUri`, …) go through their own `IdentifierJsonConverter`, and a missing `required` parameter or any unbindable value answers `InvalidRequest` naming the parameter. The serializer's message, which has CLR type names and byte positions, is no longer echoed. - Constructor-bound types (positional records) bind as they do from a body. - Contracts are resolved lazily (on the first request), not at registration: a contract snapshots `LexiconTypeRegistry` union variants, and an app may register those after its endpoints. ### One exception filter, typed error body - `XrpcErrorResponse.Handle` wraps every endpoint delegate: - `XrpcException` answers with its status, name, message and `Headers`. - `BadHttpRequestException` answers with its status; 413 maps to `PayloadTooLarge`, anything else to `InvalidRequest`. - A client abort is swallowed. - Anything else answers `500 {"error":"InternalServerError","message":"Internal Server Error"}` and is logged (`[LoggerMessage]`, category `ATProtoNet.Server.Xrpc`) with the exception. Its message never reaches the client. - The body is the internal typed `XrpcErrorBody`. Once the response has started (a blob failing halfway), the exception propagates so the server aborts the response rather than passing off a truncated body as complete. - A handler returning `null` output is a 500, where it used to write `null` with a 200. ### Unmatched routes: `MethodNotImplemented` fallback The `/xrpc` group now holds a fallback, `MapFallback("/{nsid}", …)`, that answers what no endpoint matched, following the reference `xrpc-server` catch-all: | Request | Status | `error` | |---|---|---| | An NSID no handler serves | `501` | `MethodNotImplemented` (message `Method Not Implemented`) | | A registered NSID with the wrong HTTP method | `405` + `Allow` | `InvalidRequest` (`Incorrect HTTP method (POST), expected GET.`) | | A segment that is not an NSID (`/xrpc/foo`) | `400` | `InvalidRequest` | - **The 405 case has to be written by the fallback.** Routing prefers an endpoint that accepts any method (the fallback) over producing its own 405, so once a fallback exists the router's 405 never happens. The fallback checks whether the NSID is registered (case-insensitively, like route matching) and answers 405 with `Allow` and an XRPC body. The status stays 405 as before; the error name is `InvalidRequest`, which is what the reference uses for this case (it answers with status 400). - **No shadowing.** `MapFallback` orders the fallback at `int.MaxValue`, after every other endpoint, and it matches only one segment below `/xrpc`. Registered endpoints and routes the app maps under `/xrpc` itself (e.g. `/xrpc/_health`) still answer. `/other/…`, `/com.x.y` and `/xrpc/a/b` remain plain 404s. - **Group conventions apply to the fallback too.** With `.RequireAuthorization()` on the group, an anonymous caller probing an unknown NSID gets 401, not a map of which methods exist. - The invalid-segment → 400 row goes slightly beyond "every unclaimed NSID answers 501". It mirrors the reference's `invalid xrpc path` error; switch it to 501 if you prefer. ### Interface gaps - `IXrpcProcedure<TOutput>`: no input, JSON output. - `IXrpcBlobProcedure<TOutput>` + `XrpcBlobInput(Stream Content, string ContentType, long? ContentLength)`: an unbuffered request body, for blob-upload-style methods. A missing `Content-Type` answers `InvalidRequest`, as in the reference `xrpc-server`. - Also added **`IXrpcProcedureVoid`** (no input, no output). This goes beyond the issue text but fills the same "procedure with no input" gap for `deleteSession`-shaped methods; see the flag below. - I named the binary shape `IXrpcBlobProcedure` / `XrpcBlobInput`, not the research's `IXrpcBinaryProcedure`, to match the existing `IXrpcBlobQuery` / `XrpcBlobResult`. ### One generic registration path (debloat 6.1) - The 5 reflection wrappers, the 5-branch type check, the dead `Kind` enum, and the `EnsureRegistry`/`GetOrCreateRegistry` double registration are replaced by one table of shape → (HTTP method, generic factory). There is exactly one `MakeGenericMethod` per registration, at startup. Requests run with no reflection. - Blob query output streams straight to the response with `CopyToAsync`, instead of through `Results.Stream`. ### Spaces endpoints (debloat 6.2) - Mechanical: each of the 19 handlers drops `[XrpcEndpoint(Nsid = SpaceNsids.X)]` and turns `public string Nsid => SpaceNsids.X;` into `public static Nsid Nsid { get; } = Nsid.Parse(SpaceNsids.X);`. - `NotifyWriteEndpoint` passes `SpaceNsids.NotifyWrite` to the service-auth check (it used the instance `Nsid`). - The now-false remark on `SpaceNsids` ("needed at attribute position") is removed. No parameter or model types changed. ## Breaking changes (CHANGELOG has migration notes) - **NSID declaration:** delete `[XrpcEndpoint(Nsid = …)]` and replace `public string Nsid => "…";` with `public static Nsid Nsid { get; } = Nsid.Parse("…");` (`using ATProtoNet.Identity;`). The endpoint interfaces can no longer be generic type arguments (CS8920). - **Assembly scanning:** `AddXrpcEndpointsFromAssembly` picks up every `IXrpcEndpoint` class, and invalid or conflicting handlers throw at registration. - **`MapXrpcEndpoints()` return type:** it is now `RouteGroupBuilder`. `app.MapXrpcEndpoints();` compiles unchanged. A `Map…` call chained onto the result would now map under `/xrpc`, so move it to the app. ## Verification - `dotnet build` (whole solution): **0 warnings, 0 errors**. - Unit tests: **2886 passed, 0 failed, 5 skipped** (the skips are the existing Aspire manifest tests). XRPC hosting tests went from 10 to 68 test methods across three files: - Unmatched routes: an unknown NSID answers 501 `MethodNotImplemented` for GET and POST. POST to a query and GET to a procedure answer 405 with `Allow` and an `InvalidRequest` body. A non-NSID segment answers 400. `/other/…`, `/com.x.y` and `/xrpc/a/b` stay 404. App-mapped `/xrpc/_health` and `/xrpc/com.example.appOwned` are not shadowed. Under group `RequireAuthorization()`, an unknown NSID answers 401 anonymous and 501 authenticated. - `SimpleSpaceEndpointTests.AddMember_IsNoLongerServed` now expects 501 `MethodNotImplemented` for the removed `addMember` (it expected 404). - `XrpcEndpointTests`: every shape end to end; body errors (no body, wrong content type, malformed JSON, wrong field type); blob upload hashing 70 KB; blob download of 100 KB with length; `XrpcException` status/headers; unknown exception → 500 with no secret in the body and one error log entry holding the exception; 413 → `PayloadTooLarge`; null output → 500. - Registration tests: auto-property NSID; each shape's HTTP method; idempotent re-registration; two shapes, null NSID and NSID conflict throw; scan ≡ `AddXrpcEndpoint<T>`; Server-assembly scan ≡ Spaces `Add*` registrations; open generics skipped; `MapXrpcEndpoints` with nothing registered throws. - `XrpcQueryBindingTests`: single value → list/array; repeated keys; `[]` keys; mixed; `Did`/`Nsid`/`AtUri` list binding and rejection; int/bool parse and rejection; `List<bool>`; repeated scalar refused; unknown keys ignored; JSON-escaping round trip; missing `required`; positional-record binding; 64 concurrent requests (thread-static buffer reuse). - `XrpcRouteGroupTests`: group `RequireAuthorization()` (401 → 200 with auth), `[AllowAnonymous]` overriding it, handler `[Authorize]` without a group convention, group `RequireRateLimiting` (second call 429), and group `WithMetadata` reaching the endpoint. - The existing Spaces endpoint suites (`SpaceServerEndpointTests`, `SimpleSpaceEndpointTests`) pass unchanged through the new pipeline. - Binding micro-benchmark (temporary test, not committed; Release, best of 5 × 200k, old = the previous `Dictionary` → JSON string → `Deserialize`): | Query | Old | New | |---|---|---| | `listRecords`, 6 scalars incl. a converter-bound bool | 786 ns / 1424 B | 549 ns / 456 B | | 2-element list + 2 scalars | 743 ns / 1992 B | 547 ns / 1016 B | ## Deferred / flagged - **`IXrpcProcedureVoid` (non-generic) is extra public API** beyond the issue checklist. It is small and fills the no-input/no-output gap; drop it if unwanted. - **Not added:** a binary-input procedure with no output (e.g. `importRepo`), and subscriptions (`IXrpcSubscription`, research 13.5). Neither is in the issue checklist. - **401/403 from authorization and 429 from rate limiting** are written by those middlewares, not in the XRPC envelope. The docs point to the scheme's challenge / the limiter's `OnRejected`. - `XrpcBooleanConverter` and the Spaces handler classes stay public; making them internal is #133 (13.7). Only the converter's remark was updated, since routing no longer needs it. - No trim/AOT annotations were added (the Server project has no trim analysis enabled). The remaining reflection is startup-only: one `GetInterfaces` + `MakeGenericMethod` per handler, plus assembly scanning. ## Review follow-up The first review (COMMENT) raised two findings. Both are fixed in the same commit. 1. **NSID conflicts are now checked case-insensitively.** `XrpcEndpointRegistry.Add` compared `Nsid` records ordinally, while routing and the fallback's lookup are case-insensitive. So `com.example.simpleQuery` and `com.example.SimpleQuery` both registered, and `MapXrpcEndpoints()` then crashed with an unhandled `ArgumentException` from the fallback's dictionary. The check is now `OrdinalIgnoreCase` and throws the usual `InvalidOperationException`, naming both handlers and both spellings. Test: `AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict`. The CHANGELOG bullet and the docs now say conflicts are compared case-insensitively. 2. **The fallback now goes through the same exception wrapper as every endpoint.** `XrpcErrorResponse.Handle` now takes any `RequestDelegate` rather than a registration. The unmatched-route fallback is wrapped exactly like the handlers, so an unexpected exception there answers the 500 envelope and is logged. Since the fallback has no single NSID, the log entry now names the request instead: `XRPC request {Method} {Path} failed with an unhandled exception.` Test: `Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt`. After the fix, the build has 0 warnings and 0 errors, and unit tests show 2886 passed, 0 failed, 5 skipped. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-24 19:48:26 +00:00

refactor(server)!: XRPC endpoint hosting — route groups, one NSID source, correct binding (closes #121 )

CI / pds-integration (pull_request) Successful in 25s

Details

CI / build-and-test (pull_request) Successful in 59s

Details

23b0d0c7c1

MapXrpcEndpoints() returned the builder it was called on, so no convention
(RequireAuthorization, rate limiting, metadata) could be applied to XRPC
endpoints. It now returns the /xrpc route group, and each endpoint carries its
handler class's attributes as metadata, so [Authorize] and [AllowAnonymous]
work per handler. The group's fallback answers /xrpc/{nsid} requests nothing
matched the way the reference xrpc-server does: 501 MethodNotImplemented for
an unknown NSID, and 405 with Allow and an InvalidRequest body for a known
NSID called with the wrong method.

An endpoint's NSID was declared twice (attribute and instance property), and
the property was read from an uninitialized object, which fails for
auto-property and field initializers. It is now one static abstract Nsid on
IXrpcEndpoint. Assembly scanning calls AddXrpcEndpoint<T> itself, so the two
registration paths cannot disagree. Registration rejects handlers with no
shape, several shapes, a null NSID, or a conflicting NSID.

Query parameters were bound by serializing a Dictionary to a JSON string, so a
single ?uris=x became a string and failed to bind to a list. They now bind by
the declared property type, from the serializer's contract, written straight
into a reused buffer. Identifier types go through their parsers, and failures
answer InvalidRequest naming the parameter.

Every exception now goes through one filter that writes the XRPC envelope.
Unknown ones answer InternalServerError without their message and are logged.
New shapes: IXrpcProcedure<TOutput>, IXrpcProcedureVoid, and
IXrpcBlobProcedure<TOutput> with XrpcBlobInput. The five reflection wrappers
collapse into one table and one MakeGenericMethod per registration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

opencode-bot commented

2026-09-24 19:48:29 +00:00

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-2026-09-24T19-48-28-570Z.md) — updating every ~5 minutes while the review runs.

Grandiras commented

2026-09-24 19:48:30 +00:00

@claude /review

claude-bot commented

2026-09-24 19:48:34 +00:00

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/589); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-589.md) — updating every ~5 minutes while the task runs.

claude-reviewer reviewed

2026-09-24 19:52:00 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a substantial, well-executed refactor. The static-abstract-NSID approach fixes a real bug (uninitialized-object reads failing for auto-properties), the query binder is a solid design (reads the serializer's own contract instead of duplicating type logic), and the exception-handling/fallback-route design closely and correctly follows the described ASP.NET Core routing semantics (a route with no HTTP-method constraint suppresses the router's own 405, so the fallback has to produce it — this matches actual ASP.NET Core HttpMethodMatcherPolicy behavior). Test coverage is extensive and specifically targets the tricky bits (thread-static buffer reuse under concurrency, group convention propagation to the fallback, idempotent registration, etc.).

I traced through the registration/dedup logic, the query-binding JSON round-trip, the error envelope writing, and the blob streaming paths and didn't find a functional break in the common cases. One real, if narrow, correctness gap: NSID uniqueness is enforced case-sensitively at registration time (Nsid is a record with ordinal Value equality), but the unmatched-route lookup builds its dictionary with StringComparer.OrdinalIgnoreCase. Two handlers registered for NSIDs that differ only by case would both pass XrpcEndpointRegistry.Add (record equality says they're different), then blow up with an unhandled ArgumentException from ToDictionary inside MapXrpcEndpoints() — a confusing crash, not the fail-fast InvalidOperationException message the rest of the registration path promises. Given NSIDs are conventionally globally unique, this is unlikely to bite anyone in practice, but it's worth a one-line comparer fix in Add() for consistency.

Minor secondary note (not blocking): XrpcErrorResponse.Unmatched's fallback delegate is not wrapped by XrpcErrorResponse.Handle, so an unexpected exception thrown inside it (however unlikely, given the logic is pure) would bypass the XRPC JSON envelope and the ATProtoNet.Server.Xrpc logging path that every other endpoint failure goes through.

⏱ 3m 15s · 6 turns · tokens: 102.3k in / 21.1k out (+475.8k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a substantial, well-executed refactor. The static-abstract-NSID approach fixes a real bug (uninitialized-object reads failing for auto-properties), the query binder is a solid design (reads the serializer's own contract instead of duplicating type logic), and the exception-handling/fallback-route design closely and correctly follows the described ASP.NET Core routing semantics (a route with no HTTP-method constraint suppresses the router's own 405, so the fallback has to produce it — this matches actual ASP.NET Core `HttpMethodMatcherPolicy` behavior). Test coverage is extensive and specifically targets the tricky bits (thread-static buffer reuse under concurrency, group convention propagation to the fallback, idempotent registration, etc.). I traced through the registration/dedup logic, the query-binding JSON round-trip, the error envelope writing, and the blob streaming paths and didn't find a functional break in the common cases. One real, if narrow, correctness gap: NSID uniqueness is enforced case-sensitively at registration time (`Nsid` is a record with ordinal `Value` equality), but the unmatched-route lookup builds its dictionary with `StringComparer.OrdinalIgnoreCase`. Two handlers registered for NSIDs that differ only by case would both pass `XrpcEndpointRegistry.Add` (record equality says they're different), then blow up with an unhandled `ArgumentException` from `ToDictionary` inside `MapXrpcEndpoints()` — a confusing crash, not the fail-fast `InvalidOperationException` message the rest of the registration path promises. Given NSIDs are conventionally globally unique, this is unlikely to bite anyone in practice, but it's worth a one-line comparer fix in `Add()` for consistency. Minor secondary note (not blocking): `XrpcErrorResponse.Unmatched`'s fallback delegate is not wrapped by `XrpcErrorResponse.Handle`, so an unexpected exception thrown inside it (however unlikely, given the logic is pure) would bypass the XRPC JSON envelope and the `ATProtoNet.Server.Xrpc` logging path that every other endpoint failure goes through. --- ⏱ 3m 15s · 6 turns · tokens: 102.3k in / 21.1k out (+475.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-589.md)

src/ATProtoNet.Server/Xrpc/XrpcErrorResponse.cs Outdated

					
				@ -0,0 +59,4 @@

				    public static RequestDelegate Unmatched(IEnumerable<XrpcEndpointRegistration> registrations)

				    {

				        // Case-insensitive, as route matching of the registered paths is.

				        var methods = registrations.ToDictionary(r => r.Nsid.Value, r => r.HttpMethod, StringComparer.OrdinalIgnoreCase);

XrpcEndpointRegistry.Add (in XrpcEndpointRegistration.cs) treats two NSIDs as conflicting only when they are equal under Nsid's ordinal, case-sensitive record equality, so two handlers registered for NSIDs differing only by case both succeed at registration time. This line then builds methods with StringComparer.OrdinalIgnoreCase, so ToDictionary throws an unhandled ArgumentException ("An item with the same key has already been added") the first time MapXrpcEndpoints() runs against such a registry — instead of the clear InvalidOperationException the rest of the registration path uses for conflicts. Either make Add()'s conflict check case-insensitive (matching what route matching and this lookup actually do), or build this dictionary with the same comparer used for uniqueness.

`XrpcEndpointRegistry.Add` (in `XrpcEndpointRegistration.cs`) treats two NSIDs as conflicting only when they are equal under `Nsid`'s ordinal, case-sensitive record equality, so two handlers registered for NSIDs differing only by case both succeed at registration time. This line then builds `methods` with `StringComparer.OrdinalIgnoreCase`, so `ToDictionary` throws an unhandled `ArgumentException` ("An item with the same key has already been added") the first time `MapXrpcEndpoints()` runs against such a registry — instead of the clear `InvalidOperationException` the rest of the registration path uses for conflicts. Either make `Add()`'s conflict check case-insensitive (matching what route matching and this lookup actually do), or build this dictionary with the same comparer used for uniqueness.

Grandiras commented

2026-09-24 19:55:19 +00:00

Both review findings are addressed in 20cbade:

Registration now treats NSIDs that differ only by case as duplicates, matching routing and the unmatched lookup. It throws the same clear InvalidOperationException (test AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict).
The unmatched-route fallback is wrapped by XrpcErrorResponse.Handle, so its failures get the envelope and are logged (test Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt).

@claude /review

Both review findings are addressed in `20cbade`: 1. Registration now treats NSIDs that differ only by case as duplicates, matching routing and the unmatched lookup. It throws the same clear `InvalidOperationException` (test `AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict`). 2. The unmatched-route fallback is wrapped by `XrpcErrorResponse.Handle`, so its failures get the envelope and are logged (test `Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt`). @claude /review

Grandiras force-pushed issue-121-xrpc-hosting from 23b0d0c7c1

CI / pds-integration (pull_request) Successful in 25s

Details

CI / build-and-test (pull_request) Successful in 59s

Details

to 20cbade4db

CI / pds-integration (pull_request) Successful in 27s

Details

CI / build-and-test (pull_request) Successful in 58s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s

Details

2026-09-24 19:55:20 +00:00

Compare

claude-bot commented

2026-09-24 19:55:22 +00:00

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/590); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-590.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-24 19:56:08 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Re-review: both findings from my previous COMMENT pass are fixed in this commit, each with a targeted regression test.

NSID case-insensitive conflict check — XrpcEndpointRegistry.Add (in the new XrpcEndpointRegistration.cs) now compares Nsid.Value with StringComparison.OrdinalIgnoreCase before allowing registration, matching the OrdinalIgnoreCase dictionary the unmatched-route fallback builds. Two handlers differing only by case now fail fast with a clear InvalidOperationException naming both handlers and both spellings, instead of the previous unhandled ArgumentException from ToDictionary. Verified via AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict.
Fallback wrapped by the same exception handler — XrpcErrorResponse.Handle now takes any RequestDelegate, and MapXrpcEndpoints() wraps the group's MapFallback delegate with it exactly like every registered endpoint, so an unexpected exception in the unmatched-route path now answers the standard 500 envelope and is logged (with a request-scoped message since there's no single NSID to name). Verified via Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt.

I re-traced the registration/dedup path, the fallback's dictionary construction, and the Handle wrapper's exception switch and didn't find any new correctness issues introduced by this fix-up commit. The rest of the refactor (query binder, blob streaming, route-group metadata propagation, generic registration table) is unchanged from the version I reviewed previously and remains sound. No blocking issues remain.

⏱ 0m 30s · 3 turns · tokens: 83.6k in / 2.8k out (+236.5k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: both findings from my previous COMMENT pass are fixed in this commit, each with a targeted regression test. 1. **NSID case-insensitive conflict check** — `XrpcEndpointRegistry.Add` (in the new `XrpcEndpointRegistration.cs`) now compares `Nsid.Value` with `StringComparison.OrdinalIgnoreCase` before allowing registration, matching the `OrdinalIgnoreCase` dictionary the unmatched-route fallback builds. Two handlers differing only by case now fail fast with a clear `InvalidOperationException` naming both handlers and both spellings, instead of the previous unhandled `ArgumentException` from `ToDictionary`. Verified via `AddXrpcEndpoint_NsidDifferingOnlyInCase_ThrowsLikeAnyConflict`. 2. **Fallback wrapped by the same exception handler** — `XrpcErrorResponse.Handle` now takes any `RequestDelegate`, and `MapXrpcEndpoints()` wraps the group's `MapFallback` delegate with it exactly like every registered endpoint, so an unexpected exception in the unmatched-route path now answers the standard 500 envelope and is logged (with a request-scoped message since there's no single NSID to name). Verified via `Handle_AnyDelegate_MapsItsExceptionToTheEnvelopeAndLogsIt`. I re-traced the registration/dedup path, the fallback's dictionary construction, and the `Handle` wrapper's exception switch and didn't find any new correctness issues introduced by this fix-up commit. The rest of the refactor (query binder, blob streaming, route-group metadata propagation, generic registration table) is unchanged from the version I reviewed previously and remains sound. No blocking issues remain. --- ⏱ 0m 30s · 3 turns · tokens: 83.6k in / 2.8k out (+236.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-590.md)

Grandiras merged commit bc6d765bc0 into main

2026-09-24 19:56:47 +00:00

Grandiras deleted branch issue-121-xrpc-hosting

2026-09-24 19:56:48 +00:00

Grandiras referenced this pull request from a commit

2026-09-24 19:56:49 +00:00

refactor(server)!: XRPC endpoint hosting — route groups, one NSID source, correct binding (#153)

opencode-reviewer reviewed

2026-09-24 20:18:31 +00:00

opencode-reviewer left a comment

💬 Verdict: comment — the reviewer did not submit a structured review; the raw output follows.

Let me check the serializer defaults, the exception type, and whether the SDK is available to run the tests.

⏱ 30m 2s · 8 steps · $0.0356 · tokens: 71.2k in / 633 out (+293.8k cache-read) · Go: 5h 6% · wk 98% · mo 81%
📜 Full session transcript

💬 **Verdict: comment** — the reviewer did not submit a structured review; the raw output follows. Let me check the serializer defaults, the exception type, and whether the SDK is available to run the tests. --- ⏱ 30m 2s · 8 steps · $0.0356 · tokens: 71.2k in / 633 out (+293.8k cache-read) · Go: 5h 6% · wk 98% · mo 81% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-153/run-2026-09-24T19-48-28-570Z.md)

opencode-bot commented

2026-09-24 20:18:31 +00:00

🔧 Let opencode-bot fix this — tick the box and opencode-bot picks up the review above, on this PR branch. Nothing runs until you do.

@opencode-bot Address the findings of the latest review on this pull request.

🔧 **Let opencode-bot fix this** — tick the box and opencode-bot picks up the review above, on this PR branch. Nothing runs until you do. - [ ] @opencode-bot Address the findings of the latest review on this pull request.

Sign in to join this conversation.

No reviewers