Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

refactor(server)!: EF Core package split, no Server dependencies, and an AtProto builder for DI #174

Merged
Grandiras merged 13 commits from issue-136-packages-di into main 2026-09-26 22:09:38 +00:00
Conversation 10 Commits 13 Files changed 83 +2566 -1235
Grandiras commented 2026-09-26 21:36:59 +00:00
Owner
Copy link

Closes #136.

Second half of #136: the Packages and DI sections. The first half (Auth, OAuth hosting, Blazor widgets) was #173.

Packages

  • ATProtoNet.Server.EntityFrameworkCore is its own package again. Every EF Core store and DbContext moved out of ATProtoNet.Server with git mv, namespace ATProtoNet.Server.EntityFrameworkCore unchanged: the session store (EfCoreAtProtoSessionStore<T>, AtProtoTokenDbContext), the JTI replay store (EfCoreJtiReplayStore<T>, JtiReplayDbContext), the space stores (EfCoreSpaceAuthorityStore<T>, EfCoreSimpleSpaceStore<T>, SpaceDbContext), and the repo-sync state store (EfCoreRepoSyncStateStore<T>, RepoSyncStateDbContext), with their Configure…Model helpers and Add… methods. The project is in ATProto.NET.slnx, packs (checked: dotnet pack gives a nuspec that depends on ATProtoNet.Server and Microsoft.EntityFrameworkCore.Relational 10.0.10, with the framework reference and the README), and gets InternalsVisibleTo from core (AtProtoSessionJson) and Server (SpacePaging). Its tests moved to tests/ATProtoNet.Tests/Server/EntityFrameworkCore/; the EF InMemory and Sqlite versions in the test project still match the src EF version (10.0.10).
  • The Redis replay store is gone: RedisSpaceReplayStore, its extensions, StackExchange.Redis, its unit and integration tests, [RequiresRedisFact] and ATPROTO_REDIS_URL in TestInfrastructure.cs. docs/spaces.md has a new "A replay store on Redis" section with the whole SET key value NX EX ttl IJtiReplayStore (about 15 lines) and its Replace registration; the service-auth docs (docs/xrpc-handlers.md) link to it.
  • Microsoft.Extensions.Http.Resilience is gone. The builder exposes the IHttpClientBuilder (IAtProtoBuilder.HttpClient) so apps add their own. docs/server.md and docs/aspire.md say the SDK must not be combined with a handler that retries non-idempotent POSTs or replays DPoP proofs. They also show how to remove the handler that Aspire service defaults add to every client, with RemoveAllResilienceHandlers() (experimental, EXTEXP0001) on the SDK's client and the OAuth login's client.
  • Result: ATProtoNet.Server has no NuGet dependencies. dotnet list src/ATProtoNet.Server package reports "No packages were found"; the csproj has only the core ProjectReference and FrameworkReference Microsoft.AspNetCore.App.
Package Before (NuGet dependencies) After
ATProtoNet Caching.Abstractions, Logging.Abstractions, System.Formats.Cbor unchanged
ATProtoNet.Server ATProtoNet, EntityFrameworkCore.Relational 10.0.10, Http.Resilience 10.8.0, StackExchange.Redis 2.13.17 (+ ASP.NET Core framework) ATProtoNet only (+ ASP.NET Core framework)
ATProtoNet.Server.EntityFrameworkCore not published since 0.4.0 ATProtoNet.Server, EntityFrameworkCore.Relational 10.0.10 (+ ASP.NET Core framework)
ATProtoNet.Blazor ATProtoNet, ATProtoNet.Server unchanged
ATProtoNet.Aspire.Hosting Aspire.Hosting, Aspire.Hosting.PostgreSQL 13.4.6 unchanged

DI

  • services.AddAtProto() is fixed and returns an IAtProtoBuilder (Services, HttpClient). Before, AddHttpClient<AtProtoClient>() registered the type as a transient typed client, so the TryAddSingleton never ran. I reproduced the 0.6 registration in a scratch app: every resolution was a new client for https://bsky.social, whatever InstanceUrl said. Now there is one singleton built from IOptions<AtProtoClientOptions>, and a regression test covers it. The registration hangs off the builder:
    • WithLifetime(ServiceLifetime) replaces AddAtProtoScoped.
    • WithOAuth(o => …) (the hosted login from #173) replaces AddAtProtoAuthentication.
    • WithClientFactory() replaces the three AddAtProtoServer overloads. It registers IAtProtoClientFactory, the ISessionRefreshCoordinator, and a store default.
    • WithSessionStore<T>(), WithSessionStore(factory), WithInMemorySessionStore(), WithFileSessionStore(o => …), and WithEfCoreSessionStore<TContext>() from the EF package. The chosen store replaces the default in either order.
    • WithHealthCheck(name, failureStatus, tags).
    • The Aspire builder.AddAtProtoClient(section, configure, connectionName) returns the builder too. It binds AtProtoClientOptions from the section, still honours DisableHealthChecks, and replaces AtProtoClientSettings. It follows the Aspire convention: code over configuration, and a connection string (ConnectionStrings:{connectionName}, as WithReference(pds) supplies it) over the section.
  • Service auth stays on AuthenticationBuilder (AddAuthentication().AddAtProtoServiceAuth(...)). It is an authentication scheme that uses neither the client nor its HttpClient, and it already goes through IOptions with ValidateOnStart, so a builder method would only wrap it.
  • The default session store is in memory, with a startup warning. InMemorySessionStoreWarning (a hosted service) warns when the resolved store is the in-memory default and WithInMemorySessionStore() was not chosen explicitly. Nothing is written to %LocalAppData% unless WithFileSessionStore() asks for it.
  • Every options class goes through IOptions<T> with validation and ValidateOnStart, and binds from configuration. This covers AtProtoClientOptions, AtProtoOAuthServerOptions, FileSessionStoreOptions, IdentityResolverOptions, SpaceServerOptions and PdsAdminOptions. One internal helper (AddValidatedOptions) registers the options, an IValidateOptions that reports the check's message, ValidateOnStart, and the validated instance for the services that take the concrete type. Identity and cache options reuse core's existing Validate(). The OAuth validator absorbs the ClientKeys and ServeClientMetadata checks. AtProtoServiceAuthOptions already did all this.
  • Two options classes stay outside IOptions. TapWebhookOptions belongs to each MapTapWebhook endpoint, not to DI. ServiceAuthVerifierOptions is built internally from the scheme options.
  • One HttpClient-name constant. AtProtoServiceCollectionExtensions.HttpClientName ("ATProtoNet") is used by the registered client, the factory's per-user clients (which used "AtProtoClient") and the Aspire client.
  • The OAuth login uses IHttpClientFactory. WithOAuth() registers AtProtoOAuthExtensions.HttpClientName ("ATProtoNet.OAuth") with the identity fetch policy (public addresses only, no redirects, no proxy), HttpClientTimeout and the SDK's User-Agent, and hands the factory to the AtProtoOAuthService it constructs through an internal property; the public constructor is unchanged. It stays a separate name because its primary handler must be the SSRF-safe one.
  • The loopback-callback note from #173 is resolved. The DI factory now resolves the login's OAuthClient lazily, when a request first needs it for an OAuth session. So resolving IAtProtoClientFactory before the server has started no longer builds the loopback client, and no longer throws. A test covers it.
  • ServiceCollectionExtensions is renamed AtProtoServiceCollectionExtensions.

Breaking changes (migration)

  • The EF Core stores need a package reference: add <PackageReference Include="ATProtoNet.Server.EntityFrameworkCore" />. The namespace, types and tables are unchanged.
  • services.AddAtProtoServer() → services.AddAtProto().WithClientFactory().WithFileSessionStore(). Without .WithFileSessionStore(), sessions are kept in memory and a warning is logged.
  • services.AddAtProtoServer("/dir") → ….WithClientFactory().WithFileSessionStore(o => o.Directory = "/dir").
  • services.AddAtProtoServer<TStore>() → ….WithClientFactory().WithSessionStore<TStore>().
  • services.AddAtProtoAuthentication(o => …) → services.AddAtProto().WithOAuth(o => …).
  • services.AddAtProtoScoped(o => …) → services.AddAtProto(o => …).WithLifetime(ServiceLifetime.Scoped).
  • services.AddAtProto<TStore>(o => …) → services.AddAtProto(o => …).WithSessionStore<TStore>().
  • services.AddAtProtoEfCoreSessionStore<TContext>() → ….WithEfCoreSessionStore<TContext>().
  • builder.AddAtProtoClient(configureSettings: s => …) → builder.AddAtProtoClient(configure: o => …). AtProtoClientSettings is removed. The config keys InstanceUrl, AutoRefreshSession and DisableHealthChecks still work. DisableResilience is gone, and no resilience handler is added.
  • AddAtProtoClient returns IAtProtoBuilder, not IHostApplicationBuilder.
  • Named-client customisations of "AtProtoClient" move to IAtProtoBuilder.HttpClient ("ATProtoNet").
  • AddAtProtoIdentity(configure) and AddAtProtoSpaces(configure) now apply every call's configuration, in order. Before, the first call's options won.
  • Options mistakes fail StartAsync with an OptionsValidationException. AddAtProtoPdsAdmin() used to throw InvalidOperationException from the registration call; the messages still name the configuration key.
  • RedisSpaceReplayStore is removed: copy the snippet from docs/spaces.md#a-replay-store-on-redis.

Verification

  • Unit tests: 5030 passed, 0 failed, 5 skipped. The first half's head b4ae539 had 4991/0/5; 7 Redis unit tests were removed, and the net addition is 46.
  • Full solution build (--no-incremental): 0 warnings, 0 errors. The integration tests compile; they are gated and skip.
  • New tests/ATProtoNet.Tests/Server/AtProtoBuilderTests.cs has 40 tests, one or more for every Add*/With*:
    • resolution and lifetime;
    • the store chosen in either order;
    • the warning fires only for the default;
    • options binding from configuration, ClientMetadata included;
    • ValidateOnStart failures through a started host, for the client, OAuth (7 cases), file store, identity and spaces options;
    • the builder's HttpClient carries the requests;
    • the OAuth named client has the policy handler and timeout;
    • the health check runs in the check's scope with a scoped client;
    • AddJetstream.
  • Other test files:
    • AddAtProtoBlazor resolves one accessor per scope over WithClientFactory() (ValidateOnBuild).
    • The Aspire tests are rewritten: config, callback, connection string, validation, health check, no resilience handler.
    • The PDS admin tests check ValidateOnStart, and that the options-instance overload copies every property.
    • WithEfCoreSessionStore is tested against SQLite.
  • The existing registration tests (spaces, JTI, service auth, XRPC, identity, PDS admin) still pass.
  • Doc snippets compiled in a scratch project against the new API with warnings as errors: the Redis IJtiReplayStore, RemoveAllResilienceHandlers, AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods()), the Configure<T>(section) lines, WithEfCoreSessionStore, and AddAtProtoClient(connectionName:). At runtime, with ConfigureHttpClientDefaults(h => h.AddStandardResilienceHandler()), RemoveAllResilienceHandlers() strips the resilience handler from ATProtoNet and ATProtoNet.OAuth, and other keeps it.
  • samples/ServerIntegrationSample was run on the builder:
    • / and /login return 200.
    • A bad handle redirects to /login?error=handle_resolution_failed.
    • /atproto/login?handle=bsky.app PARs at bsky.social with client_id=http://localhost?redirect_uri=http://127.0.0.1:5010/atproto/callback…, through the ATProtoNet.OAuth named client (its HttpClient logs appear).
    • There is no in-memory warning, since the sample chooses WithInMemorySessionStore() explicitly.

For the owner

  • Forgejo NuGet linkage: the package ID ATProtoNet.Server.EntityFrameworkCore exists from 0.4.0, when it was published orphaned and relinked by hand. A later upload never re-evaluates the link, so check after the 0.7.0 release that it shows on the repository's Packages tab. If it does not, relink it with POST /api/v1/packages/Grandiras/nuget/ATProtoNet.Server.EntityFrameworkCore/-/link/ATProto.NET. RepositoryUrl handling is unchanged, and the packed nuspec carries https://git.grandiras.net/Grandiras/ATProto.NET. If the package was deprecated or unlisted on nuget.org when it was merged in 0.5.0, undo that.
  • CLAUDE.md is stale (left for #138):
    • "DI extensions (AddAtProto, AddAtProtoServer), JWT auth handler".
    • EF Core stores "in TokenStore/EntityFrameworkCore/" of Server, and "merged in from separate packages".
    • The Aspire client's "resilience".
    • "The four src/ projects" (now five).
    • The ATPROTO_REDIS_URL / [RequiresRedisFact] bullet.
    • Blazor "and the OAuth login endpoints registered by MapAtProtoOAuth()".

Deferred / flagged

  • SpaceServerOptions.ServiceDid (Did) does not bind from configuration: the identity types have no TypeConverter. It is documented as set in code; a TypeConverter for Did would be a core change.
  • The in-process refresh coordinator still covers one process (as in #173). Multi-instance deployments need a distributed ISessionRefreshCoordinator; that is documented, not shipped.
  • Other issues' CHANGELOG bullets still name AddAtProtoEfCoreSessionStore, AddAtProto<TSessionStore>, AddAtProtoServer<TSessionStore> and AddAtProtoScoped as they were at the time. The new migration list covers them; I left them unedited. I did edit three #173 bullets that gave migration steps pointing at removed calls.

Review fixes

Eight commits on top of 80f94f1. Each code fix has a regression test, and I reverted each fix to confirm its test then fails. Each fix updates the CHANGELOG in the same commit.

  • Retry guidance (79df5ec, docs).
    • The recommended AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods()) is gone from docs/aspire.md and docs/server.md. Under service defaults it stacked a second handler.
    • The docs now say that "ATProtoNet" is shared with the per-user OAuth clients, so no retry policy is safe on it. A resent request carries a spent DPoP proof, and the defaults' 10 s attempt timeout cuts off large downloads.
    • Retries stay in the SDK (429), or go in a wrapper above it (docs/error-handling.md#retry-pattern); the wrapper is now also told to retry only calls that are safe to repeat.
    • docs/error-handling.md no longer pairs the SDK with a retrying handler. The stale "resilience" promises in docs/index.md and docs/managed-pds.md are fixed.
  • One health check per name (cafd12f). WithHealthCheck replaces a check with the same name. Tests cover AddAtProtoClient().WithHealthCheck() and AddAtProtoClient() twice, and both run /health.
  • Health check failure status (cafd12f). An unreachable PDS reports the registration's FailureStatus (Degraded), not Unhealthy. This also fixes a bug that shipped in 0.6.
  • Code over configuration, whatever the order (6bb2377).
    • The callbacks of every options-registering method are now PostConfigure: AddAtProto, WithOAuth, WithFileSessionStore, identity, spaces and PDS admin.
    • So AddAtProto(o => …) before AddAtProtoClient() or Configure<T>(section) no longer loses to the configuration.
    • Tests cover the client with and without a connection string, and PDS admin.
  • The OAuth login's client (1fe7512).
    • It sends the SDK's User-Agent.
    • The public constructor no longer takes an IHttpClientFactory. Only WithOAuth() hands it over, through an internal property, because only WithOAuth() gives that name the SSRF policy.
    • A service built with ActivatorUtilities from a container without WithOAuth() now keeps its own policy client. The test checks ConnectCallback and AllowAutoRedirect, and it failed before the fix.
    • The AtProtoOAuthServerOptions.HttpClient doc no longer offers "a proxy". For an egress proxy it says to supply the client.
  • Per-user clients get the transport options (a112d11).
    • They take UserAgent and RateLimit from AddAtProto(o => …).
    • The session options (InstanceUrl, AutoRefreshSession, BackgroundRefresh, RefreshCoordinator) stay the factory's. This is documented on WithClientFactory and in docs/server.md.
    • The test uses a 429 with MaxRetries = 0: 1 request instead of 4, carrying MyApp/1.0.
  • ServiceDid (7e7fdf1).
    • AddSpaceAuthority validates it with the options at startup. Before, it threw InvalidOperationException from the startup check.
    • AddAtProtoSpaces() alone still starts without one. The code has always allowed that for verifier-only and repo-host services, so the SpaceServerOptions remark ("always required") is corrected.
  • Store docs (79df5ec). The custom-store example takes an IDbContextFactory, and the WithSessionStore XML docs say the store is a singleton that must not capture a scoped DbContext.
  • Tests (9aa0de4, 1fe7512).
    • The OAuth named client's primary handler has ConnectCallback set and UseProxy off, and AllowPrivateNetworks lifts the address check.
    • The login's AtProtoOAuthService, the client factory and AtProtoClientOptions share one ISessionRefreshCoordinator, whichever of WithOAuth() and WithClientFactory() comes first.

After the fixes: 5045 passed, 0 failed, 5 skipped; --no-incremental build 0 warnings, 0 errors. origin/main is unchanged at f750468.

🤖 Generated with Claude Code

Closes #136. Second half of #136: the **Packages** and **DI** sections. The first half (Auth, OAuth hosting, Blazor widgets) was #173. ## Packages - **`ATProtoNet.Server.EntityFrameworkCore` is its own package again.** Every EF Core store and `DbContext` moved out of `ATProtoNet.Server` with `git mv`, namespace `ATProtoNet.Server.EntityFrameworkCore` unchanged: the session store (`EfCoreAtProtoSessionStore<T>`, `AtProtoTokenDbContext`), the JTI replay store (`EfCoreJtiReplayStore<T>`, `JtiReplayDbContext`), the space stores (`EfCoreSpaceAuthorityStore<T>`, `EfCoreSimpleSpaceStore<T>`, `SpaceDbContext`), and the repo-sync state store (`EfCoreRepoSyncStateStore<T>`, `RepoSyncStateDbContext`), with their `Configure…Model` helpers and `Add…` methods. The project is in `ATProto.NET.slnx`, packs (checked: `dotnet pack` gives a nuspec that depends on `ATProtoNet.Server` and `Microsoft.EntityFrameworkCore.Relational` 10.0.10, with the framework reference and the README), and gets `InternalsVisibleTo` from core (`AtProtoSessionJson`) and Server (`SpacePaging`). Its tests moved to `tests/ATProtoNet.Tests/Server/EntityFrameworkCore/`; the EF InMemory and Sqlite versions in the test project still match the src EF version (10.0.10). - **The Redis replay store is gone**: `RedisSpaceReplayStore`, its extensions, `StackExchange.Redis`, its unit and integration tests, `[RequiresRedisFact]` and `ATPROTO_REDIS_URL` in `TestInfrastructure.cs`. `docs/spaces.md` has a new "A replay store on Redis" section with the whole `SET key value NX EX ttl` `IJtiReplayStore` (about 15 lines) and its `Replace` registration; the service-auth docs (`docs/xrpc-handlers.md`) link to it. - **`Microsoft.Extensions.Http.Resilience` is gone.** The builder exposes the `IHttpClientBuilder` (`IAtProtoBuilder.HttpClient`) so apps add their own. `docs/server.md` and `docs/aspire.md` say the SDK must not be combined with a handler that retries non-idempotent POSTs or replays DPoP proofs. They also show how to remove the handler that Aspire service defaults add to every client, with `RemoveAllResilienceHandlers()` (experimental, `EXTEXP0001`) on the SDK's client and the OAuth login's client. - **Result:** `ATProtoNet.Server` has no NuGet dependencies. `dotnet list src/ATProtoNet.Server package` reports "No packages were found"; the csproj has only the core `ProjectReference` and `FrameworkReference Microsoft.AspNetCore.App`. | Package | Before (NuGet dependencies) | After | |---|---|---| | `ATProtoNet` | Caching.Abstractions, Logging.Abstractions, System.Formats.Cbor | unchanged | | `ATProtoNet.Server` | ATProtoNet, EntityFrameworkCore.Relational 10.0.10, Http.Resilience 10.8.0, StackExchange.Redis 2.13.17 (+ ASP.NET Core framework) | ATProtoNet only (+ ASP.NET Core framework) | | `ATProtoNet.Server.EntityFrameworkCore` | not published since 0.4.0 | ATProtoNet.Server, EntityFrameworkCore.Relational 10.0.10 (+ ASP.NET Core framework) | | `ATProtoNet.Blazor` | ATProtoNet, ATProtoNet.Server | unchanged | | `ATProtoNet.Aspire.Hosting` | Aspire.Hosting, Aspire.Hosting.PostgreSQL 13.4.6 | unchanged | ## DI - **`services.AddAtProto()` is fixed and returns an `IAtProtoBuilder`** (`Services`, `HttpClient`). Before, `AddHttpClient<AtProtoClient>()` registered the type as a transient typed client, so the `TryAddSingleton` never ran. I reproduced the 0.6 registration in a scratch app: every resolution was a new client for `https://bsky.social`, whatever `InstanceUrl` said. Now there is one singleton built from `IOptions<AtProtoClientOptions>`, and a regression test covers it. The registration hangs off the builder: - `WithLifetime(ServiceLifetime)` replaces `AddAtProtoScoped`. - `WithOAuth(o => …)` (the hosted login from #173) replaces `AddAtProtoAuthentication`. - `WithClientFactory()` replaces the three `AddAtProtoServer` overloads. It registers `IAtProtoClientFactory`, the `ISessionRefreshCoordinator`, and a store default. - `WithSessionStore<T>()`, `WithSessionStore(factory)`, `WithInMemorySessionStore()`, `WithFileSessionStore(o => …)`, and `WithEfCoreSessionStore<TContext>()` from the EF package. The chosen store replaces the default in either order. - `WithHealthCheck(name, failureStatus, tags)`. - The Aspire `builder.AddAtProtoClient(section, configure, connectionName)` returns the builder too. It binds `AtProtoClientOptions` from the section, still honours `DisableHealthChecks`, and replaces `AtProtoClientSettings`. It follows the Aspire convention: code over configuration, and a connection string (`ConnectionStrings:{connectionName}`, as `WithReference(pds)` supplies it) over the section. - **Service auth stays on `AuthenticationBuilder`** (`AddAuthentication().AddAtProtoServiceAuth(...)`). It is an authentication scheme that uses neither the client nor its `HttpClient`, and it already goes through `IOptions` with `ValidateOnStart`, so a builder method would only wrap it. - **The default session store is in memory, with a startup warning.** `InMemorySessionStoreWarning` (a hosted service) warns when the resolved store is the in-memory default and `WithInMemorySessionStore()` was not chosen explicitly. Nothing is written to `%LocalAppData%` unless `WithFileSessionStore()` asks for it. - **Every options class goes through `IOptions<T>` with validation and `ValidateOnStart`, and binds from configuration.** This covers `AtProtoClientOptions`, `AtProtoOAuthServerOptions`, `FileSessionStoreOptions`, `IdentityResolverOptions`, `SpaceServerOptions` and `PdsAdminOptions`. One internal helper (`AddValidatedOptions`) registers the options, an `IValidateOptions` that reports the check's message, `ValidateOnStart`, and the validated instance for the services that take the concrete type. Identity and cache options reuse core's existing `Validate()`. The OAuth validator absorbs the `ClientKeys` and `ServeClientMetadata` checks. `AtProtoServiceAuthOptions` already did all this. - **Two options classes stay outside `IOptions`.** `TapWebhookOptions` belongs to each `MapTapWebhook` endpoint, not to DI. `ServiceAuthVerifierOptions` is built internally from the scheme options. - **One HttpClient-name constant.** `AtProtoServiceCollectionExtensions.HttpClientName` (`"ATProtoNet"`) is used by the registered client, the factory's per-user clients (which used `"AtProtoClient"`) and the Aspire client. - **The OAuth login uses `IHttpClientFactory`.** `WithOAuth()` registers `AtProtoOAuthExtensions.HttpClientName` (`"ATProtoNet.OAuth"`) with the identity fetch policy (public addresses only, no redirects, no proxy), `HttpClientTimeout` and the SDK's `User-Agent`, and hands the factory to the `AtProtoOAuthService` it constructs through an internal property; the public constructor is unchanged. It stays a separate name because its primary handler must be the SSRF-safe one. - **The loopback-callback note from #173 is resolved.** The DI factory now resolves the login's `OAuthClient` lazily, when a request first needs it for an OAuth session. So resolving `IAtProtoClientFactory` before the server has started no longer builds the loopback client, and no longer throws. A test covers it. - `ServiceCollectionExtensions` is renamed `AtProtoServiceCollectionExtensions`. ## Breaking changes (migration) - The EF Core stores need a package reference: add `<PackageReference Include="ATProtoNet.Server.EntityFrameworkCore" />`. The namespace, types and tables are unchanged. - `services.AddAtProtoServer()` → `services.AddAtProto().WithClientFactory().WithFileSessionStore()`. Without `.WithFileSessionStore()`, sessions are kept in memory and a warning is logged. - `services.AddAtProtoServer("/dir")` → `….WithClientFactory().WithFileSessionStore(o => o.Directory = "/dir")`. - `services.AddAtProtoServer<TStore>()` → `….WithClientFactory().WithSessionStore<TStore>()`. - `services.AddAtProtoAuthentication(o => …)` → `services.AddAtProto().WithOAuth(o => …)`. - `services.AddAtProtoScoped(o => …)` → `services.AddAtProto(o => …).WithLifetime(ServiceLifetime.Scoped)`. - `services.AddAtProto<TStore>(o => …)` → `services.AddAtProto(o => …).WithSessionStore<TStore>()`. - `services.AddAtProtoEfCoreSessionStore<TContext>()` → `….WithEfCoreSessionStore<TContext>()`. - `builder.AddAtProtoClient(configureSettings: s => …)` → `builder.AddAtProtoClient(configure: o => …)`. `AtProtoClientSettings` is removed. The config keys `InstanceUrl`, `AutoRefreshSession` and `DisableHealthChecks` still work. `DisableResilience` is gone, and no resilience handler is added. - `AddAtProtoClient` returns `IAtProtoBuilder`, not `IHostApplicationBuilder`. - Named-client customisations of `"AtProtoClient"` move to `IAtProtoBuilder.HttpClient` (`"ATProtoNet"`). - `AddAtProtoIdentity(configure)` and `AddAtProtoSpaces(configure)` now apply every call's configuration, in order. Before, the first call's options won. - Options mistakes fail `StartAsync` with an `OptionsValidationException`. `AddAtProtoPdsAdmin()` used to throw `InvalidOperationException` from the registration call; the messages still name the configuration key. - `RedisSpaceReplayStore` is removed: copy the snippet from `docs/spaces.md#a-replay-store-on-redis`. ## Verification - Unit tests: **5030 passed, 0 failed, 5 skipped**. The first half's head `b4ae539` had 4991/0/5; 7 Redis unit tests were removed, and the net addition is 46. - Full solution build (`--no-incremental`): **0 warnings, 0 errors**. The integration tests compile; they are gated and skip. - New `tests/ATProtoNet.Tests/Server/AtProtoBuilderTests.cs` has 40 tests, one or more for every `Add*`/`With*`: - resolution and lifetime; - the store chosen in either order; - the warning fires only for the default; - options binding from configuration, `ClientMetadata` included; - `ValidateOnStart` failures through a started host, for the client, OAuth (7 cases), file store, identity and spaces options; - the builder's `HttpClient` carries the requests; - the OAuth named client has the policy handler and timeout; - the health check runs in the check's scope with a scoped client; - `AddJetstream`. - Other test files: - `AddAtProtoBlazor` resolves one accessor per scope over `WithClientFactory()` (`ValidateOnBuild`). - The Aspire tests are rewritten: config, callback, connection string, validation, health check, no resilience handler. - The PDS admin tests check `ValidateOnStart`, and that the options-instance overload copies every property. - `WithEfCoreSessionStore` is tested against SQLite. - The existing registration tests (spaces, JTI, service auth, XRPC, identity, PDS admin) still pass. - Doc snippets compiled in a scratch project against the new API with warnings as errors: the Redis `IJtiReplayStore`, `RemoveAllResilienceHandlers`, `AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods())`, the `Configure<T>(section)` lines, `WithEfCoreSessionStore`, and `AddAtProtoClient(connectionName:)`. At runtime, with `ConfigureHttpClientDefaults(h => h.AddStandardResilienceHandler())`, `RemoveAllResilienceHandlers()` strips the resilience handler from `ATProtoNet` and `ATProtoNet.OAuth`, and `other` keeps it. - `samples/ServerIntegrationSample` was run on the builder: - `/` and `/login` return 200. - A bad handle redirects to `/login?error=handle_resolution_failed`. - `/atproto/login?handle=bsky.app` PARs at bsky.social with `client_id=http://localhost?redirect_uri=http://127.0.0.1:5010/atproto/callback…`, through the `ATProtoNet.OAuth` named client (its `HttpClient` logs appear). - There is no in-memory warning, since the sample chooses `WithInMemorySessionStore()` explicitly. ## For the owner - **Forgejo NuGet linkage:** the package ID `ATProtoNet.Server.EntityFrameworkCore` exists from 0.4.0, when it was published orphaned and relinked by hand. A later upload never re-evaluates the link, so check after the 0.7.0 release that it shows on the repository's Packages tab. If it does not, relink it with `POST /api/v1/packages/Grandiras/nuget/ATProtoNet.Server.EntityFrameworkCore/-/link/ATProto.NET`. `RepositoryUrl` handling is unchanged, and the packed nuspec carries `https://git.grandiras.net/Grandiras/ATProto.NET`. If the package was deprecated or unlisted on nuget.org when it was merged in 0.5.0, undo that. - **`CLAUDE.md` is stale** (left for #138): - "DI extensions (`AddAtProto`, `AddAtProtoServer`), JWT auth handler". - EF Core stores "in `TokenStore/EntityFrameworkCore/`" of Server, and "merged in from separate packages". - The Aspire client's "resilience". - "The four `src/` projects" (now five). - The `ATPROTO_REDIS_URL` / `[RequiresRedisFact]` bullet. - Blazor "and the OAuth login endpoints registered by `MapAtProtoOAuth()`". ## Deferred / flagged - `SpaceServerOptions.ServiceDid` (`Did`) does not bind from configuration: the identity types have no `TypeConverter`. It is documented as set in code; a `TypeConverter` for `Did` would be a core change. - The in-process refresh coordinator still covers one process (as in #173). Multi-instance deployments need a distributed `ISessionRefreshCoordinator`; that is documented, not shipped. - Other issues' CHANGELOG bullets still name `AddAtProtoEfCoreSessionStore`, `AddAtProto<TSessionStore>`, `AddAtProtoServer<TSessionStore>` and `AddAtProtoScoped` as they were at the time. The new migration list covers them; I left them unedited. I did edit three #173 bullets that gave migration steps pointing at removed calls. ## Review fixes Eight commits on top of `80f94f1`. Each code fix has a regression test, and I reverted each fix to confirm its test then fails. Each fix updates the CHANGELOG in the same commit. - **Retry guidance** (`79df5ec`, docs). - The recommended `AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods())` is gone from `docs/aspire.md` and `docs/server.md`. Under service defaults it stacked a second handler. - The docs now say that `"ATProtoNet"` is shared with the per-user OAuth clients, so no retry policy is safe on it. A resent request carries a spent DPoP proof, and the defaults' 10 s attempt timeout cuts off large downloads. - Retries stay in the SDK (429), or go in a wrapper above it (`docs/error-handling.md#retry-pattern`); the wrapper is now also told to retry only calls that are safe to repeat. - `docs/error-handling.md` no longer pairs the SDK with a retrying handler. The stale "resilience" promises in `docs/index.md` and `docs/managed-pds.md` are fixed. - **One health check per name** (`cafd12f`). `WithHealthCheck` replaces a check with the same name. Tests cover `AddAtProtoClient().WithHealthCheck()` and `AddAtProtoClient()` twice, and both run `/health`. - **Health check failure status** (`cafd12f`). An unreachable PDS reports the registration's `FailureStatus` (`Degraded`), not `Unhealthy`. This also fixes a bug that shipped in 0.6. - **Code over configuration, whatever the order** (`6bb2377`). - The callbacks of every options-registering method are now `PostConfigure`: `AddAtProto`, `WithOAuth`, `WithFileSessionStore`, identity, spaces and PDS admin. - So `AddAtProto(o => …)` before `AddAtProtoClient()` or `Configure<T>(section)` no longer loses to the configuration. - Tests cover the client with and without a connection string, and PDS admin. - **The OAuth login's client** (`1fe7512`). - It sends the SDK's `User-Agent`. - The public constructor no longer takes an `IHttpClientFactory`. Only `WithOAuth()` hands it over, through an internal property, because only `WithOAuth()` gives that name the SSRF policy. - A service built with `ActivatorUtilities` from a container without `WithOAuth()` now keeps its own policy client. The test checks `ConnectCallback` and `AllowAutoRedirect`, and it failed before the fix. - The `AtProtoOAuthServerOptions.HttpClient` doc no longer offers "a proxy". For an egress proxy it says to supply the client. - **Per-user clients get the transport options** (`a112d11`). - They take `UserAgent` and `RateLimit` from `AddAtProto(o => …)`. - The session options (`InstanceUrl`, `AutoRefreshSession`, `BackgroundRefresh`, `RefreshCoordinator`) stay the factory's. This is documented on `WithClientFactory` and in `docs/server.md`. - The test uses a 429 with `MaxRetries = 0`: 1 request instead of 4, carrying `MyApp/1.0`. - **`ServiceDid`** (`7e7fdf1`). - `AddSpaceAuthority` validates it with the options at startup. Before, it threw `InvalidOperationException` from the startup check. - `AddAtProtoSpaces()` alone still starts without one. The code has always allowed that for verifier-only and repo-host services, so the `SpaceServerOptions` remark ("always required") is corrected. - **Store docs** (`79df5ec`). The custom-store example takes an `IDbContextFactory`, and the `WithSessionStore` XML docs say the store is a singleton that must not capture a scoped `DbContext`. - **Tests** (`9aa0de4`, `1fe7512`). - The OAuth named client's primary handler has `ConnectCallback` set and `UseProxy` off, and `AllowPrivateNetworks` lifts the address check. - The login's `AtProtoOAuthService`, the client factory and `AtProtoClientOptions` share one `ISessionRefreshCoordinator`, whichever of `WithOAuth()` and `WithClientFactory()` comes first. After the fixes: **5045 passed, 0 failed, 5 skipped**; `--no-incremental` build 0 warnings, 0 errors. `origin/main` is unchanged at `f750468`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Every EF Core store and DbContext leaves ATProtoNet.Server for its own
package, namespace unchanged, so a service without a database no longer
depends on EF Core. Work in progress on the Packages and DI half of #136.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A Redis-backed IJtiReplayStore is one SET NX EX; docs/spaces.md now
shows the whole class instead of the package depending on
StackExchange.Redis. Work in progress on the Packages and DI half of #136.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AddAtProto() registers one resolvable AtProtoClient from IOptions and
returns a builder with WithLifetime, WithOAuth, WithClientFactory, the
session store choices and WithHealthCheck. The client factory defaults to
an in-memory store with a startup warning instead of writing tokens to
LocalAppData; options classes go through IOptions with ValidateOnStart;
the resilience dependency is gone. Work in progress on #136.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each Add*/With* method resolves what it registers, the options of each are
validated when the host starts, and the in-memory store warning fires only
for the default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(changelog): end the builder migration list with the issue reference (#136)
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 59s
Details
80f94f1bb4 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-26 21:37:01 +00:00
Collaborator
Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-2026-09-26T21-37-00-987Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 21:37:04 +00:00
Collaborator
Copy link

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-27T02:37:01.460Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-27T02:37:01.460Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f20598c9dffeLMMtaKcJDf0qf5 -->
Grandiras commented 2026-09-26 21:37:07 +00:00
Author
Owner
Copy link

@claude /review — the second half of #136. #173 was the first. The PR is large and mostly moves files, so the diff will be truncated. Please read these in full from the branch:

  • src/ATProtoNet.Server/Extensions/IAtProtoBuilder.cs, AtProtoServiceCollectionExtensions.cs, AtProtoBuilderExtensions.cs and AtProtoOptionsRegistration.cs: lifetimes, TryAdd semantics, and whether the client really resolves from the configured options;
  • Authentication/AtProtoOAuthExtensions.cs: the OAuth login on the builder, its named HttpClient, and the lazily resolved OAuthClient;
  • Aspire/AtProtoAspireExtensions.cs and Admin/PdsAdminExtensions.cs: configuration binding and ValidateOnStart;
  • TokenStore/InMemorySessionStoreWarning.cs;
  • the new src/ATProtoNet.Server.EntityFrameworkCore project: csproj, namespaces kept, and packability.
    Focus on captive dependencies (a singleton holding a scoped service), a user's client leaking across requests under WithLifetime, registrations that silently do nothing, options that validate too late, and anything in docs or samples that still names a removed call (AddAtProtoServer, AddAtProtoScoped, AddAtProtoAuthentication, AddAtProtoEfCoreSessionStore, AtProtoClientSettings, Redis).
@claude /review — the second half of #136. #173 was the first. The PR is large and mostly moves files, so the diff will be truncated. Please read these in full from the branch: - `src/ATProtoNet.Server/Extensions/IAtProtoBuilder.cs`, `AtProtoServiceCollectionExtensions.cs`, `AtProtoBuilderExtensions.cs` and `AtProtoOptionsRegistration.cs`: lifetimes, TryAdd semantics, and whether the client really resolves from the configured options; - `Authentication/AtProtoOAuthExtensions.cs`: the OAuth login on the builder, its named HttpClient, and the lazily resolved `OAuthClient`; - `Aspire/AtProtoAspireExtensions.cs` and `Admin/PdsAdminExtensions.cs`: configuration binding and `ValidateOnStart`; - `TokenStore/InMemorySessionStoreWarning.cs`; - the new `src/ATProtoNet.Server.EntityFrameworkCore` project: csproj, namespaces kept, and packability. Focus on captive dependencies (a singleton holding a scoped service), a user's client leaking across requests under `WithLifetime`, registrations that silently do nothing, options that validate too late, and anything in docs or samples that still names a removed call (`AddAtProtoServer`, `AddAtProtoScoped`, `AddAtProtoAuthentication`, `AddAtProtoEfCoreSessionStore`, `AtProtoClientSettings`, Redis).
claude-bot commented 2026-09-26 21:37:10 +00:00
Collaborator
Copy link

🤖 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/625); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-625.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-26 21:41:27 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is the second half of the #136 refactor (packages + DI), and it holds together well on close inspection. I checked out the PR branch's tree (via git show/git grep against origin/issue-136-packages-di, since it wasn't locally checked out) to verify the files the diff only partially showed, in particular AtProtoServiceCollectionExtensions.AddAtProto, IAtProtoBuilder/AtProtoBuilder, InMemorySessionStoreWarning/InMemorySessionStoreChoice, AtProtoClientFactory, and the PdsAdminOptions/PdsAdminExtensions validation path.

Things I specifically verified as correct:

  • Session-store precedence is order-independent as claimed. WithClientFactory() only TryAddSingletons the in-memory default, while every explicit With*SessionStore call does RemoveAll<IAtProtoSessionStore>() + RemoveAll<InMemorySessionStoreChoice>() before adding its own descriptor, so the explicit choice wins regardless of call order, and the in-memory startup warning only fires when no explicit choice marker was registered.
  • Options composition/precedence for AddAtProtoClient (Aspire). The section bind, the connection-string override, and the configure callback are three separate Configure<AtProtoClientOptions> registrations applied in that registration order, so "code wins over connection string wins over section" holds as documented. DisableHealthChecks is deliberately read straight off the raw IConfigurationSection rather than bound onto AtProtoClientOptions — a little asymmetric with InstanceUrl's precedence chain, but it's explicitly documented that way ("read by AddAtProtoClient only"), so not a defect.
  • AtProtoOAuthService's new optional IHttpClientFactory parameter is appended last, so existing positional/DI construction stays source- and binary-compatible; WithOAuth() correctly wires the named ATProtoNet.OAuth client with the identity fetch policy and timeout, and AtProtoOAuthExtensions.ValidateOptions is a reasonable, well-scoped set of checks (route paths, scopes, timeouts, confidential-client key/metadata pairing).
  • AtProtoClientFactory's lazy OAuthClient resolution (Func<OAuthClient?> overload, with the DI registration closing over the singleton-time IServiceProvider) genuinely defers resolution to first use, matching the loopback-callback fix described in the PR.
  • PdsAdminOptions binding via .Bind(section) despite required members is safe: the configuration binder doesn't enforce required at bind time (that's compile-time only), so missing values simply stay default and are caught by the new ValidateOptions/OptionsValidationException path at host startup, consistent with the documented breaking change (InvalidOperationException at registration → OptionsValidationException at startup).
  • InternalsVisibleTo wiring for the new ATProtoNet.Server.EntityFrameworkCore project (from core, for AtProtoSessionJson, and from Server, for SpacePaging) is present as described.
  • File moves (git mv) for the EF Core stores preserve namespaces and only touch doc-comment cross-references (AddAtProtoEfCoreSessionStore → WithEfCoreSessionStore), which is exactly the kind of change that should accompany a namespace-preserving package split.

I didn't find any correctness, security, or omission issues worth blocking on. The CHANGELOG updates are thorough and the breaking-change migration table matches the code changes I could verify. Given the scale of this diff, I leaned on the PR's own extensive verification section (5030 unit tests, full build, doc-snippet compilation, manual sample run) rather than re-deriving all of it myself, but everything I spot-checked lines up with what's claimed.

⏱ 4m 5s · 38 turns · tokens: 113.1k in / 20.1k out (+3170.3k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is the second half of the #136 refactor (packages + DI), and it holds together well on close inspection. I checked out the PR branch's tree (via `git show`/`git grep` against `origin/issue-136-packages-di`, since it wasn't locally checked out) to verify the files the diff only partially showed, in particular `AtProtoServiceCollectionExtensions.AddAtProto`, `IAtProtoBuilder`/`AtProtoBuilder`, `InMemorySessionStoreWarning`/`InMemorySessionStoreChoice`, `AtProtoClientFactory`, and the `PdsAdminOptions`/`PdsAdminExtensions` validation path. Things I specifically verified as correct: - **Session-store precedence is order-independent as claimed.** `WithClientFactory()` only `TryAddSingleton`s the in-memory default, while every explicit `With*SessionStore` call does `RemoveAll<IAtProtoSessionStore>()` + `RemoveAll<InMemorySessionStoreChoice>()` before adding its own descriptor, so the explicit choice wins regardless of call order, and the in-memory startup warning only fires when no explicit choice marker was registered. - **Options composition/precedence for `AddAtProtoClient` (Aspire).** The section bind, the connection-string override, and the `configure` callback are three separate `Configure<AtProtoClientOptions>` registrations applied in that registration order, so "code wins over connection string wins over section" holds as documented. `DisableHealthChecks` is deliberately read straight off the raw `IConfigurationSection` rather than bound onto `AtProtoClientOptions` — a little asymmetric with `InstanceUrl`'s precedence chain, but it's explicitly documented that way ("read by `AddAtProtoClient` only"), so not a defect. - **`AtProtoOAuthService`'s new optional `IHttpClientFactory` parameter** is appended last, so existing positional/DI construction stays source- and binary-compatible; `WithOAuth()` correctly wires the named `ATProtoNet.OAuth` client with the identity fetch policy and timeout, and `AtProtoOAuthExtensions.ValidateOptions` is a reasonable, well-scoped set of checks (route paths, scopes, timeouts, confidential-client key/metadata pairing). - **`AtProtoClientFactory`'s lazy `OAuthClient` resolution** (`Func<OAuthClient?>` overload, with the DI registration closing over the singleton-time `IServiceProvider`) genuinely defers resolution to first use, matching the loopback-callback fix described in the PR. - **`PdsAdminOptions` binding via `.Bind(section)` despite `required` members** is safe: the configuration binder doesn't enforce `required` at bind time (that's compile-time only), so missing values simply stay default and are caught by the new `ValidateOptions`/`OptionsValidationException` path at host startup, consistent with the documented breaking change (`InvalidOperationException` at registration → `OptionsValidationException` at startup). - **`InternalsVisibleTo` wiring** for the new `ATProtoNet.Server.EntityFrameworkCore` project (from core, for `AtProtoSessionJson`, and from Server, for `SpacePaging`) is present as described. - File moves (`git mv`) for the EF Core stores preserve namespaces and only touch doc-comment cross-references (`AddAtProtoEfCoreSessionStore` → `WithEfCoreSessionStore`), which is exactly the kind of change that should accompany a namespace-preserving package split. I didn't find any correctness, security, or omission issues worth blocking on. The CHANGELOG updates are thorough and the breaking-change migration table matches the code changes I could verify. Given the scale of this diff, I leaned on the PR's own extensive verification section (5030 unit tests, full build, doc-snippet compilation, manual sample run) rather than re-deriving all of it myself, but everything I spot-checked lines up with what's claimed. --- ⏱ 4m 5s · 38 turns · tokens: 113.1k in / 20.1k out (+3170.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-625.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:80f94f1bb466bcfab5941e3cafff6a7d5b4bd32c -->
Grandiras commented 2026-09-26 22:05:09 +00:00
Author
Owner
Copy link

@claude /review, please re-review. Seven new commits (cafd12f..79df5ec) on top of the approved 80f94f1 fix an independent adversarial review; see "Review fixes" in the description. Please read those commits in full (git diff 80f94f1..79df5ec):

  • the PDS health check: registered once per name, and it reports the registered failure status;
  • options callbacks now run as PostConfigure, so code beats bound configuration whatever the call order;
  • only WithOAuth gives the login its named client (the public constructor no longer takes IHttpClientFactory, so a hand-built service keeps the SSRF-safe owned client), and that client sends the SDK's User-Agent;
  • per-user clients take UserAgent and RateLimit from AddAtProto;
  • ServiceDid is validated when a space authority is registered;
  • the docs drop the retry recommendation on the shared "ATProtoNet" client (DPoP replay, duplicate POSTs) and show a session store without a captive DbContext.
@claude /review, please re-review. Seven new commits (cafd12f..79df5ec) on top of the approved 80f94f1 fix an independent adversarial review; see "Review fixes" in the description. Please read those commits in full (`git diff 80f94f1..79df5ec`): - the PDS health check: registered once per name, and it reports the registered failure status; - options callbacks now run as `PostConfigure`, so code beats bound configuration whatever the call order; - only `WithOAuth` gives the login its named client (the public constructor no longer takes `IHttpClientFactory`, so a hand-built service keeps the SSRF-safe owned client), and that client sends the SDK's User-Agent; - per-user clients take `UserAgent` and `RateLimit` from `AddAtProto`; - `ServiceDid` is validated when a space authority is registered; - the docs drop the retry recommendation on the shared "ATProtoNet" client (DPoP replay, duplicate POSTs) and show a session store without a captive `DbContext`.
WithHealthCheck replaces a check of the same name, so AddAtProtoClient
followed by WithHealthCheck, or AddAtProtoClient twice, no longer makes the
health check service throw. An unreachable PDS reports the registration's
failure status (Degraded by default) instead of always Unhealthy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The callbacks of the options-registering methods are post-configurations,
so AddAtProto(o => …) before AddAtProtoClient(), or before a
Configure<T>(section), no longer loses to the configuration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The public constructor no longer takes an IHttpClientFactory: a service
built by hand, or by ActivatorUtilities from a container without
WithOAuth(), got the unconfigured named client, which follows redirects
and reaches private addresses. WithOAuth() sets the factory internally,
since it configures that name with the identity fetch policy, and the
named client now carries the SDK's User-Agent like the owned one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The factory created its clients with default options, so a UserAgent or
RateLimit set in AddAtProto(o => …) never reached them. They now take
those two; the session options stay the factory's (on demand, under its
coordinator, at the user's PDS).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AddSpaceAuthority validates SpaceServerOptions.ServiceDid with the other
options when the host starts; a service that only verifies or hosts repos
still starts without one, as the code already allowed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs: no retry policy on the SDK's shared client, a session store without a captive DbContext (#136)
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 57s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
Details
79df5ecf32 
The recommended AddStandardResilienceHandler(DisableForUnsafeHttpMethods)
stacked on the handler Aspire service defaults already add, and the name
it configured is shared with the per-user OAuth clients, whose DPoP proofs
must not be resent. Retries stay in the SDK and above it. The custom store
example takes an IDbContextFactory, since stores are singletons, and the
stale resilience promises are gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot commented 2026-09-26 22:05:12 +00:00
Collaborator
Copy link

🤖 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/626); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-626.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-26 22:07:10 +00:00
Collaborator
Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-2026-09-26T22-07-10-245Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 22:07:13 +00:00
Collaborator
Copy link

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-27T03:07:10.725Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-27T03:07:10.725Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f203df11affepaLm8aNr1Br4vu -->
claude-reviewer approved these changes 2026-09-26 22:08:20 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a re-review after my prior APPROVE on this PR's earlier head (80f94f1). Six "review fixes" commits landed since then (cafd12f, 6bb2377, 1fe7512, a112d11, 7e7fdf1, 9aa0de4) plus a docs-only commit (79df5ec), taking the head to 79df5ec. I read each fix commit's diff directly against the branch (origin/issue-136-packages-di) rather than relying on the PR description, and all six hold up:

  • One health check per name / correct failure status (cafd12f): WithHealthCheck now removes any existing HealthCheckRegistration of the same name from HealthCheckServiceOptions before adding its own, via Configure<HealthCheckServiceOptions>, so AddAtProtoClient() called twice (or followed by WithHealthCheck()) no longer throws in the health check service. AtProtoPdsHealthCheck now returns context.Registration?.FailureStatus ?? HealthStatus.Unhealthy instead of hardcoded Unhealthy. Both have direct tests.
  • Code-over-configuration ordering (6bb2377): the shared AddValidatedOptions helper switched from options.Configure(configure) to options.PostConfigure(configure). Since the options pipeline always runs every Configure before every PostConfigure regardless of registration order, this genuinely makes the callback win whichever order AddAtProto(o=>…), AddAtProtoClient()/Configure<T>(section) and AddAtProtoPdsAdmin() are called in — verified against the added order-flip tests (AddAtProto_ThenAddAtProtoClient_CodeWinsOverConfiguration, the PDS admin equivalent). Because the fix lives in the one shared helper, it correctly propagates to every consumer (WithOAuth, WithFileSessionStore, identity, spaces, PDS admin, AddAtProto itself) without each call site needing its own change.
  • OAuth login client ownership (1fe7512): AtProtoOAuthService's public constructor no longer accepts IHttpClientFactory; the SSRF-safe named-client factory is now an internal init-only PolicyHttpClientFactory property set only by WithOAuth(). This closes the exact gap the description flags: a service built via ActivatorUtilities from a container that merely has AddHttpClient() (no WithOAuth()) can no longer accidentally pick up an unconfigured "ATProtoNet.OAuth" client that follows redirects and reaches private addresses. The named client also now sends the SDK's User-Agent. New tests cover both the isolation and the header.
  • Per-user clients inherit UserAgent/RateLimit (a112d11): AtProtoClientFactory now takes an optional AtProtoClientOptions and copies only UserAgent/RateLimit onto each per-request client's options, leaving session-specific fields (InstanceUrl, AutoRefreshSession, etc.) alone as documented. WithClientFactory() wires sp.GetService<IOptions<AtProtoClientOptions>>()?.Value through.
  • ServiceDid required only for an authority (7e7fdf1): the check moved into AddSpaceAuthority as an AddOptions<SpaceServerOptions>().Validate(...).ValidateOnStart(), scoped to that call only, so AddAtProtoSpaces() alone (verifier/repo-host-only) still starts without a ServiceDid, matching the stated intent.
  • Tests-only and docs-only commits (9aa0de4, 79df5ec): the former adds a two-way (loginFirst/not) test proving WithOAuth() and WithClientFactory() end up sharing one ISessionRefreshCoordinator instance; the latter only touches docs plus one XML-doc addition, consistent with "docs" scope.

I did not re-run the build/test suite in this pass (git network operations required approval I didn't have in this session), but the diffs are small, self-contained, each paired with a targeted regression test as the PR claims, and consistent with the fixes described in the PR body. Nothing here reintroduces or overlaps with the issues from my previous pass. No new findings.

⏱ 2m 50s · 24 turns · tokens: 115.4k in / 8.7k out (+2798.8k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review after my prior APPROVE on this PR's earlier head (80f94f1). Six "review fixes" commits landed since then (cafd12f, 6bb2377, 1fe7512, a112d11, 7e7fdf1, 9aa0de4) plus a docs-only commit (79df5ec), taking the head to 79df5ec. I read each fix commit's diff directly against the branch (`origin/issue-136-packages-di`) rather than relying on the PR description, and all six hold up: - **One health check per name / correct failure status** (`cafd12f`): `WithHealthCheck` now removes any existing `HealthCheckRegistration` of the same name from `HealthCheckServiceOptions` before adding its own, via `Configure<HealthCheckServiceOptions>`, so `AddAtProtoClient()` called twice (or followed by `WithHealthCheck()`) no longer throws in the health check service. `AtProtoPdsHealthCheck` now returns `context.Registration?.FailureStatus ?? HealthStatus.Unhealthy` instead of hardcoded `Unhealthy`. Both have direct tests. - **Code-over-configuration ordering** (`6bb2377`): the shared `AddValidatedOptions` helper switched from `options.Configure(configure)` to `options.PostConfigure(configure)`. Since the options pipeline always runs every `Configure` before every `PostConfigure` regardless of registration order, this genuinely makes the callback win whichever order `AddAtProto(o=>…)`, `AddAtProtoClient()`/`Configure<T>(section)` and `AddAtProtoPdsAdmin()` are called in — verified against the added order-flip tests (`AddAtProto_ThenAddAtProtoClient_CodeWinsOverConfiguration`, the PDS admin equivalent). Because the fix lives in the one shared helper, it correctly propagates to every consumer (`WithOAuth`, `WithFileSessionStore`, identity, spaces, PDS admin, `AddAtProto` itself) without each call site needing its own change. - **OAuth login client ownership** (`1fe7512`): `AtProtoOAuthService`'s public constructor no longer accepts `IHttpClientFactory`; the SSRF-safe named-client factory is now an internal `init`-only `PolicyHttpClientFactory` property set only by `WithOAuth()`. This closes the exact gap the description flags: a service built via `ActivatorUtilities` from a container that merely has `AddHttpClient()` (no `WithOAuth()`) can no longer accidentally pick up an unconfigured `"ATProtoNet.OAuth"` client that follows redirects and reaches private addresses. The named client also now sends the SDK's `User-Agent`. New tests cover both the isolation and the header. - **Per-user clients inherit `UserAgent`/`RateLimit`** (`a112d11`): `AtProtoClientFactory` now takes an optional `AtProtoClientOptions` and copies only `UserAgent`/`RateLimit` onto each per-request client's options, leaving session-specific fields (`InstanceUrl`, `AutoRefreshSession`, etc.) alone as documented. `WithClientFactory()` wires `sp.GetService<IOptions<AtProtoClientOptions>>()?.Value` through. - **`ServiceDid` required only for an authority** (`7e7fdf1`): the check moved into `AddSpaceAuthority` as an `AddOptions<SpaceServerOptions>().Validate(...).ValidateOnStart()`, scoped to that call only, so `AddAtProtoSpaces()` alone (verifier/repo-host-only) still starts without a `ServiceDid`, matching the stated intent. - **Tests-only and docs-only commits** (`9aa0de4`, `79df5ec`): the former adds a two-way (`loginFirst`/not) test proving `WithOAuth()` and `WithClientFactory()` end up sharing one `ISessionRefreshCoordinator` instance; the latter only touches docs plus one XML-doc addition, consistent with "docs" scope. I did not re-run the build/test suite in this pass (git network operations required approval I didn't have in this session), but the diffs are small, self-contained, each paired with a targeted regression test as the PR claims, and consistent with the fixes described in the PR body. Nothing here reintroduces or overlaps with the issues from my previous pass. No new findings. --- ⏱ 2m 50s · 24 turns · tokens: 115.4k in / 8.7k out (+2798.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-174/run-626.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:79df5ecf321f0712d8bde12d6d8daef27d1d0846 -->
Grandiras deleted branch issue-136-packages-di 2026-09-26 22:09:39 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
claude-reviewer
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Grandiras/ATProto.NET!174
No description provided.