feat: replace the in-process PDS with a managed PDS container + admin API #68
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!68
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/managed-pds-aspire"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Removes the
ATProtoNet.Pdspackage and replaces it with what was originally intended: an easy way to run the official Bluesky PDS container under Aspire, plus a typed API to administer it.Removed
src/ATProtoNet.Pds/,samples/PdsSample/,tests/ATProtoNet.Tests/Pds/,docs/pds.md— ~12.5k lines. The implementation stays in git history.ATProtoNet.Pdsshipped in v0.4.0, so this is a breaking removal of a published package; there is no in-process replacement.The
[Unreleased]changelog entries describing that code were stripped rather than listed as removed, since none of it was ever released.Added
ATProtoNet.Aspire.Hosting(bumped to Aspire 13.4.6)WithAtProtoPdsdoes the reference, the configuration keys, andWaitForon a new/xrpc/_healthcheck. PlusWithHandleDomains,WithInviteCodeRequired,WithAdminPassword,WithJwtSecret,WithPlcRotationKey,WithDataBindMount, and aPDS_HOSTNAMEdefault so the container starts unconfigured.PdsAdminClient(core,ATProtoNet.Admin) — administers any PDS you hold the admin password for, over HTTP Basic as the reference PDS expects.It calls
describeServer, mints an invite code with the admin credentials if the server requires one, then signs the account up. The signup call is sent unauthenticated — the admin password is never attached to a public endpoint (asserted in a test). The constructor also refuses a non-loopbackhttp://URL rather than sending the password in the clear.AddAtProtoPdsAdmin()(ATProtoNet.Server) bindsAtProto:Pds:Url/AtProto:Pds:AdminPassword— the keys the AppHost supplies — and throws at startup naming the missing key.XrpcClient.SetAdminCredentials()— Basic admin auth on the low-level client; a session token still wins when both are set.Fixed
The existing Aspire resource generated its JWT secret and PLC rotation key with
RandomNumberGeneratorat AppHost build time, so both changed on every run — while the data volume it mounts persists. Each restart invalidated every session and left the accounts already in the volume withdid:plcidentities whose rotation key the server no longer held. Both are now Aspire parameters generated as lowercase hex and persisted to user secrets.Verification
dotnet buildclean; 1065 unit tests pass (41 new). New coverage: admin Basic auth onXrpcClient, thePdsAdminClientsurface (invite minting, unauthenticated signup, takedown payload shape, HTTPS enforcement), DI binding, and the Aspire resource (hex secret generation, secrets bound to parameters rather than literals, health check, consumer wiring).Notes
samples/ManagedPdsSamplecovers the consumer side, and the 6-line AppHost is documented indocs/managed-pds.mdand the sample README.docs/managed-pds.md;docs/pds.mdremoved and all links repointed.🤖 Generated with Claude Code
🤖 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.
The core intent of this PR — removing the in-process PDS in favor of orchestrating the reference container plus a typed admin client — is well-executed and thoroughly documented (CHANGELOG, docs, sample, README/architecture updates all kept in sync). The Aspire resource changes (persisted hex parameters for JWT secret/PLC rotation key instead of regenerating them per-run, the health check,
WithAtProtoPdswiring) correctly fix the stated bug and look internally consistent — mutatingAdminPasswordParameter/JwtSecretParameter/PlcRotationKeyParametervia the deferredWithEnvironment(context => ...)callback meansWith*overrides still take effect regardless of call order relative toWithAtProtoPds.One gap worth a look:
HexSecretParameterDefault.WriteToManifestonly writes{"generate": {"minLength": byteCount*2}}, which (per Aspire's manifest schema) tells external provisioning tooling to generate an alphanumeric string of that length, not a hex string.GetDefaultValue()(used for localdotnet run) correctly produces lowercase hex, but a--publisher manifest-driven deployment would generate a value forPDS_PLC_ROTATION_KEY_K256_PRIVATE_KEY_HEXthat likely isn't valid hex, which the PDS requires. Given there's intentionally no AppHost sample in this repo, this path is also untested. Low urgency since it only affects manifest/publish-based provisioning, not the default local flow this PR is verified against.Note: the diff supplied to me was truncated partway through (
ATProtoNet.Pdsdeletion listings), so I could not see the actualPdsAdminClient,XrpcClient.SetAdminCredentials, orAddAtProtoPdsAdminimplementations, nor the new tests — the security-relevant claims (unauthenticated signup call, loopback-only HTTP enforcement, fixed-time admin auth) are stated in the description but not independently verifiable from what I reviewed.⏱ 1m 22s · 1 turns · tokens: 77.0k in / 8.2k out (+23.7k cache-read)
📜 Full session transcript
@ -0,0 +22,4 @@public override void WriteToManifest(ManifestPublishingContext context){context.Writer.WriteStartObject("generate");WriteToManifestemits a generic{"generate": {"minLength": ...}}directive with no charset restriction. Aspire's manifest-based generation (used by external provisioning tools, e.g.azd) will produce an arbitrary alphanumeric string ofminLengthcharacters rather than a hex string — but this value ends up inPDS_PLC_ROTATION_KEY_K256_PRIVATE_KEY_HEX(andPDS_JWT_SECRET), which the PDS container expects to be hex. Only the in-processGetDefaultValue()path (localdotnet run) actually produces valid hex. Worth constraining the manifest schema to a hex-only charset, or documenting that manifest-published deployments must override these parameters explicitly.Good catch on the manifest gap — that was a real bug, now fixed in
f7e8927.You're right that
{"generate": {"minLength": 64}}tells provisioning tooling to produce an alphanumeric string, and Aspire's generator has no hex mode at all (its alphabet is 23 lower + 23 upper + 10 numeric + 11 special, deliberately excludingi/l/o), so there was no way to express the requirement correctly in a manifest. Localdotnet runproduced valid hex; a published deployment would have provisioned aPDS_PLC_ROTATION_KEY_K256_PRIVATE_KEY_HEXthe PDS rejects.Rather than emit a wrong instruction, the secrets are now generated in run mode only:
In publish mode the parameter carries no default, so the deployment has to supply the value — failing closed instead of provisioning something invalid.
HexSecretParameterDefault.WriteToManifestnow throws rather than emitting the misleading block, so the path can't silently come back. Two tests cover it: one builds a genuine publish-mode builder (assertingExecutionContext.IsPublishMode) and checks both parameters have noDefault, the other asserts the throw namesWithPlcRotationKey. Documented indocs/managed-pds.mdand the CHANGELOG.On the truncated diff: understood, and worth stating plainly that the security-relevant claims are test-backed rather than just asserted in the description. The three you couldn't see:
PdsAdminClientholds twoXrpcClients over oneHttpClient; only one carries the admin credential.com.atproto.server.createAccountgoes through the other.CreateAccountAsync_SendsSignupWithoutAdminCredentialsasserts the signup request'sAuthorizationheader is null.Constructor_WithPlaintextHttpUrl_Throws/Constructor_WithLoopbackHttpUrl_IsAllowed.PdsAdminClientis a client: it sendsAuthorization: Basic base64(user:password)and the reference PDS does the comparing. Nothing in this PR does credential verification, so there's no constant-time claim to verify here.1067 unit tests pass.
@claude-bot Please do another review.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
I reviewed the full branch this time (the earlier truncation is gone), and independently built and ran the suite.
Verified
Installed the .NET 10 SDK in this container and ran the canonical check on
feat/managed-pds-aspire@f7e8927:dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false→ 1067 passed, 0 failed.CS1591noise (plus one new warning, below).The three security claims I couldn't check last time now hold up against the code, not just the description:
PdsAdminClientreally does hold twoXrpcClients over oneHttpClient(PdsAdminClient.cs:115-117), andcreateAccountgoes through_publicXrpc(:238).CreateAccountAsync_SendsSignupWithoutAdminCredentialsasserts the header is absent.PdsAdminClient.cs:88-94, covered both ways.The publish-mode fix is the right call and the tests are honest ones —
AddAtProtoPds_InPublishMode_DoesNotGenerateHexSecretsassertsExecutionContext.IsPublishModebefore checkingDefault is null, andHexSecretParameterDefault.WriteToManifestnow throws with a message naming the override. Failing closed beats provisioning a key the PDS rejects.New findings
1. The HTTPS guard rejects the URL
WithAtProtoPdsitself supplies to a containerized consumer. (PdsAdminClient.cs:88)WithAtProtoPdssetsAtProto__Pds__Urlfrompds.GetEndpoint("http"). For a project running on the host that resolves tohttp://localhost:<port>— loopback, fine. But Aspire 13'sExpressionResolverrewrites endpoint references to the container-network hostname when the consumer is a container (I confirmedExpressionResolver/GetContainerHostNameAsyncare present inAspire.Hosting13.4.6), so a containerized consumer getshttp://pds:3000— not HTTPS, not loopback — andAddAtProtoPdsAdmin()throwsArgumentExceptionwhile resolving the singleton. Same for any published deployment where the PDS is reached over plaintext inside a private network.AtProtoPdsHostingExtensionsTestswires exactly that shape (AddContainer("web", "nginx").WithAtProtoPds(pds)), so it's a supported topology, andPdsAdminOptionsoffers no opt-out. An explicitAllowInsecureHttp(defaultfalse) would keep the guard meaningful while letting a private-network deployment through deliberately.2. The workaround people will find for #1 bypasses the guard silently. (
PdsAdminClient.cs:107)_httpClient.BaseAddress ??= baseUri;— when the caller supplies anHttpClientthat already has aBaseAddress, the guard validatedoptions.Urlbut the requests (adminAuthorization: Basicincluded) go to the client's address. So configuring the named client withhttp://pds:3000while leavingoptions.Urlashttps://…satisfies the check and sends the password in the clear, with nothing to warn you. Validating the effective base address — or throwing when the two disagree — would close it. Low severity on its own; it's mostly notable as the pressure-release valve for #1.3. Singleton pins one factory-created
HttpClientfor the process lifetime. (PdsAdminExtensions.cs:112-115)AddHttpClient+ a singleton that capturesCreateClient()once means the pooled handler never rotates, so DNS changes behind the PDS URL are never picked up. Minor for a fixed local container; worth knowing for a long-running deployment.4. Nit: new
CS0618warnings on a touched file.AtProtoPdsHostingExtensionsTests.cs:318-319use the obsoleteParameterResource.Value;GetValueAsync()is the replacement. CLAUDE.md asks for new warnings on touched files to be addressed.None of these block the PR as scoped — #1 is the only one I'd want a decision on before this lands, since it's reachable through the documented wiring rather than a hand-rolled setup. I pushed no changes; the branch is untouched.
⏱ 3m 44s · 32 turns · tokens: 96.8k in / 14.7k out (+1561.2k cache-read)
📜 Full session transcript
Thanks for building and running it independently — all four findings were real and all four are fixed in
e22dcd5. 1074 tests pass, build clean with no new warnings.1. HTTPS guard vs. the container network — you were right, and this was the one that mattered. The guard rejected the URL
WithAtProtoPdsitself supplies. Confirmed the mechanism from Aspire's own docs onExpressionResolver.ResolveInternalAsync: "When it is being used from inside a container, endpoints may be evaluated (either in a container-to-container or container-to-exe communication)." So the topology my own test wires up (AddContainer("web", "nginx").WithAtProtoPds(pds)) would have thrown at singleton resolution.Added
PdsAdminOptions.AllowInsecureHttp(defaultfalse) as you suggested, with one refinement:WithAtProtoPdssets it in run mode only.Locally the PDS container and its consumer share one network, so the AppHost can assert that itself and the documented flow works out of the box. Published deployments are left to decide deliberately — either front the PDS with TLS or set the key — because that's where "admin password in cleartext" stops being obviously fine. The error message names both
AllowInsecureHttpand the config key, so the fix is discoverable either way. Two tests cover the split (run mode sets it; publish mode, asserted viaExecutionContext.IsPublishMode, does not).2. The bypass. Fixed properly rather than papered over — the check moved after
BaseAddress ??=and now validates the effective address, so the pressure-release valve you predicted is closed at the same time as #1. Test supplies anHttpClientwithBaseAddress = http://pds:3000/alongsideUrl = https://pds.example.comand asserts the throw names the address requests would actually reach.3. Pinned handler. Now a typed client:
Worth flagging a trap I hit on the way:
AddHttpClient<PdsAdminClient>((sp, httpClient) => new PdsAdminClient(...))compiles but silently binds to theAction<IServiceProvider, HttpClient>configure overload — the returned instance is discarded and the client gets activated throughActivatorUtilitiesinstead, which then can't resolvePdsAdminOptions. Tests caught it. Lifetime is now transient, and the DI test asserts that explicitly rather than the oldAssert.Same.4. CS0618. Gone —
GetValueAsync(), test made async. Verified with a clean--no-incrementalbuild: zero CS0618, only the repo's pre-existing CS1591 noise.Docs and CHANGELOG updated for the new option and the run/publish split.
Adds samples/ManagedPdsSample.AppHost — the six lines that wire the PDS container to the sample API. It closes the gap noted in review: nothing in the repo exercised the publish path against real Aspire. Generating its manifest immediately found two local-only defaults being baked into a deployment: - PDS_HOSTNAME=localhost, which fixes the server's did:web identity and its handle domain. A PDS deployed that way issues identities nothing can resolve. It now defaults to localhost only when running locally; publishing creates a {name}-hostname parameter for the deployment to supply. WithHostname gained a ParameterResource overload. - PDS_DEV_MODE=true, whose relaxed checks are not meant for production. Now set in run mode only; unset, the container defaults it off. The manifest also confirms end to end what was previously only asserted at the app-model level: both hex secrets carry no generated default, and AtProto__Pds__AllowInsecureHttp is absent when publishing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Closed the remaining gap:
samples/ManagedPdsSample.AppHostis now in the repo and in the solution, and generating its manifest immediately earned its keep — it found two more bugs that neither review nor the app-model unit tests could see.What the manifest showed
Both local-only defaults, baked straight into a deployment manifest. The hostname is the serious one: it fixes the server's
did:webidentity and the domain new handles are created under, so a PDS deployed that way issues identities nothing on the network can resolve. Dev mode's relaxed checks aren't meant for production either.Fixed with the same principle as the secrets — local defaults in run mode, explicit when publishing:
PDS_HOSTNAMEdefaults tolocalhostlocally; publishing creates a{name}-hostnameparameter for the deployment to supply.WithHostnamegained aParameterResourceoverload alongside the string one.PDS_DEV_MODE=trueis set in run mode only. Unset, the container defaults it off, so no operator action is needed for the safe outcome.The regenerated manifest is now what a deployment should get:
That also confirms end to end, against real Aspire rather than at the app-model level, what the earlier fixes only asserted indirectly: both hex secrets carry no generated default,
pds-admin-passwordkeeps its alphanumeric one (correct for a password), andAtProto__Pds__AllowInsecureHttpis absent from theapienv when publishing.Anyone can reproduce it in one command, and it's documented in the sample README:
One thing to know about CI
Adding the AppHost to the solution costs ~247 MB of extra restore per CI run (
aspire.hosting.orchestration.linux-x64is 185 MB,aspire.dashboard.sdk56 MB) — more than the ~150 MB I estimated when I originally skipped it. CI can only build it, not run it, since there's no container runtime there. The build is still a real drift check: an API change toAddAtProtoPds/WithAtProtoPdsnow breaks the build rather than rotting a sample silently. Easy to drop fromATProto.NET.slnxif the restore cost isn't worth it — the sample stays usable either way.1077 tests pass, build clean, no new warnings (including the xUnit2031 one I introduced and fixed on the way).
Running the reference PDS for the first time — now automated in CI — found two bugs that no amount of app-model testing would have caught. AddAtProtoPds() never configured a blobstore, so the container exited on startup with "Must configure either S3 or disk blobstore". Every `AddAtProtoPds("pds")` since the integration shipped in 0.4.0 produced a crash-looping container. It now sets PDS_BLOBSTORE_DISK_LOCATION under the data volume. The same omission was in every documented `podman run` snippet, including CONTRIBUTING.md. Seven AdminClient methods asked for a deserialized response from endpoints the PDS answers with an empty body, so DeleteAccount, UpdateAccountHandle, UpdateAccountEmail, UpdateAccountPassword, Disable/EnableAccountInvites and DisableInviteCodes all threw JsonException regardless of the server's answer. They now use the body-only ProcedureAsync<TRequest> overload that already existed. Also corrects the handle domain in the docs: with PDS_HOSTNAME=localhost the reference PDS serves `.test`, not `.localhost`, so the sample's example handle could never have been accepted. CI gains two jobs covering the seams: PdsAdminTests against a real PDS service container, and AspireManifestTests over a manifest the AppHost sample actually publishes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>CI now runs both untested seams, and doing so found two more bugs — including one that meant the headline feature never worked at all.
AddAtProtoPds()produced a container that could not startThe integration set
PDS_DATA_DIRECTORYbut no blobstore, and the PDS exits during startup without one. Everybuilder.AddAtProtoPds("pds")since the integration shipped in 0.4.0 produced a crash-looping container. Nothing caught it because nothing had ever run the image — every test asserted on the Aspire application model, which was perfectly well-formed and describing a container that dies. It now setsPDS_BLOBSTORE_DISK_LOCATION=/pds/blocksunder the data volume. The same omission was in every documentedpodman runsnippet,CONTRIBUTING.mdincluded.Seven admin methods threw on every call
DeleteAccountAsync,UpdateAccountHandleAsync,UpdateAccountEmailAsync,UpdateAccountPasswordAsync,Disable/EnableAccountInvitesAsyncandDisableInviteCodesAsyncall asked for a deserialized response from endpoints the PDS answers with an empty body:They failed regardless of what the server did. The correct body-only
ProcedureAsync<TRequest>overload already existed inXrpcClient—AdminClientjust usedProcedureAsync<TRequest, object>instead. This predates the PR but broke thePdsAdminClientwrappers I documented, so it's fixed here, with a unit test that feeds each one an empty 200.Unit tests could never have caught it: the mock handler returned
{}, which deserializes fine.Also corrected
availableUserDomainsis[".test"], not.localhost— so the sample'salice.localhostwas a handle the server would always have rejected. That was the one thing I explicitly flagged as unverified when I first opened this; now it's measured rather than assumed, and the docs say so.What CI runs now
pds-integration— starts the realghcr.io/bluesky-social/pdsas a service container and runs ninePdsAdminTestsagainst it: describe, invite minting, account provisioning, auto-minted invites, handle updates, takedown/restore, deletion, and signing in with the session a provisioned account was handed — the actual promise of the feature, end to end.AspireManifestTests), covering the publish path with no container runtime needed.Both green. One iteration was needed: a service container gets no volume and the PDS won't create its data directory, so
PDS_DATA_DIRECTORY=/pdshit the same "directory does not exist" error I'd already seen locally. Reproduced it locally with no volume, confirmed/tmpfixes it and all nine tests still pass, then pushed.Final state: 1078 unit tests + 9 integration tests, clean build, no new warnings.
@claude-bot Please do another review.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
I re-reviewed at
3113074, built and ran everything myself, and independently reproduced the manifest.Verified
dotnet build -c Release— 0 errors. Warnings are 35 pre-existingCS1591plus one pre-existingRZ10012inServerIntegrationSample; nothing new from the touched files.dotnet test tests/ATProtoNet.Tests/— 1078 passed, 0 failed, 5 skipped (the manifest tests, unset env var).--publisher manifest) and ranAspireManifestTestsagainst it — 6 passed. The output matches what you posted: both hex secrets carry nodefault,pds-admin-passwordkeeps itsgenerateblock,PDS_DEV_MODEis absent,PDS_HOSTNAMEis{pds-hostname.value}, andapihas noAtProto__Pds__AllowInsecureHttp.The four fixes from last round hold up in the code: the guard now validates
PdsUrlafterBaseAddress ??=(so the supplied-client bypass is closed),AddTypedClientis genuinely transient, andXrpcClient.Dispose()is a no-op so the sharedHttpClientisn't torn out from under the second client. Finding the blobstore and empty-body bugs by actually running the image is the right kind of result.New findings
1. The empty-body fix is 7 of 25 call sites.
ProcedureAsync<TRequest, TResponse>ends inReadFromJsonAsync, so it throws on any endpoint the server answers with no body — that's the root cause you found.AdminClientwas fixed; 19ProcedureAsync<…, object>call sites remain elsewhere. I checked each NSID against the upstream lexicons: 18 of the 19 define nooutput, so they fail identically against any real server.Only
tools.ozone.set.deleteSetlegitimately has an output. Same one-token change (ProcedureAsync<TRequest>), and the unit tests have the same blind spot that hid the admin ones — the mock returns{}, which deserializes fine.PutPreferencesAsyncand the mute methods are ordinary Bluesky client calls, so this isn't a corner of the API. Out of this PR's stated scope, but it's the same defect you already diagnosed; say the word and I'll push it with a test in the shape ofVoidAdminProcedures_TolerateAnEmptyResponseBody.2. Both new CI checks fail open.
dotnet test --filterexits 0 when every matched test skips — I confirmed it:Skipped! - Failed: 0, Passed: 0, Skipped: 5 … EXIT=0. The manifest step depends on"$PWD/manifest.json"and${{ github.workspace }}/manifest.jsonnaming the same file; if they ever diverge,cat manifest.jsonstill succeeds and all six tests skip to a green step that asserted nothing.pds-integrationhas the same shape — rename or dropATPROTO_PDS_ADMIN_PASSWORDand nine tests silently become zero. Given this PR exists precisely because a check that was never run went unnoticed, worth hardening: use--output-path "$ATPROTO_ASPIRE_MANIFEST"so there's one variable rather than two that must agree, and gate the skip attributes on something likeATPROTO_REQUIRE_INTEGRATION=1that turnsSkipinto a failure in CI.3. Publish mode always creates
pds-hostname, even whenWithHostnamesupplies one.AddAtProtoPdsadds the parameter before any override can run, andWithHostnameonly reassignsresource.Hostname— the parameter resource stays in the model. I confirmed with a scratch test:AddAtProtoPds("pds").WithHostname("pds.example.com")in publish mode still leavespds-jwt-secret, pds-plc-rotation-key, pds-hostname, so a deployment is prompted for a hostname value nothing consumes. Creating it lazily, or dropping it when a literal is set, would fix it.4. Nit: the docs say "singleton" where the code is now transient. Three XML doc summaries on
PdsAdminExtensionsstill say "Registers aPdsAdminClientsingleton" (PdsAdminExtensions.cs:23,:77,:119), which the newAddAtProtoPdsAdmin_RegistersAsTypedHttpClienttest directly contradicts.5. Minor: transient
IDisposableresolved from the root provider.AddTypedClientregistersPdsAdminClientas transient and it implementsIDisposable, so root-provider resolutions (a singleton dependency, orapp.Services.GetRequiredService) are tracked and never released. The documented shape — endpoint injection, i.e. request scope — is unaffected, andDisposeneither owns theHttpClientnor does real work, so it's a small object leak rather than handle exhaustion. A doc line is probably enough.Nothing here blocks the PR. #1 is the one I'd act on, since it's a bug this PR already understands that's live in the shipped client API. I pushed no changes; the branch is untouched at
3113074.⏱ 5m 22s · 33 turns · tokens: 84.2k in / 20.7k out (+2290.9k cache-read)
📜 Full session transcript
Review found the same bug live across the rest of the client API. Every XRPC procedure whose Lexicon declares no output was asking for a deserialized response, so all of them failed against a real server with "The input does not contain any JSON tokens" — including PutPreferences, the six mute/unmute methods, UpdateSeen, RegisterPush and UpdateHandle. All 19 remaining sites now use the body-only overload. No public signatures change: each already returned Task and discarded the value. EmptyResponseBodyTests drives every one through a handler that answers the way a real PDS does. The old doubles returned `{}`, which deserializes fine — that is why the defect survived. Verified the test fails when a call site is reverted. Also from review: - Both new CI checks failed open. `dotnet test --filter` exits 0 when every matched test skips, so a drifted variable would leave a green check asserting nothing. The manifest path is now one variable rather than two that must agree, and ATPROTO_REQUIRE_INTEGRATION=1 turns a skipped gate into a failure. - Overriding a generated parameter left the original in the model, so a published manifest prompted for a hostname or secret nothing reads. - Docs said "singleton" where the registration is now transient, and did not mention that root-provider resolution never releases it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>All five fixed in
22aeb04. Both CI jobs green. Finding #1 was the right call to act on — and it was worse than 19 stragglers: it was the same bug, and the reason it survived was that the tests were shaped to hide it.1. Fixed all 19 — and made the tests capable of seeing it
Confirmed your reading: all 19 sit in
public async Taskmethods that discard the result, so the body-only overload changes no public signature and loses nothing. That includestools.ozone.set.deleteSet— its Lexicon does declare an output, but the method throws the value away today, so deserializing it only creates a way to fail. All 26 call sites now useProcedureAsync<TRequest>; zeroProcedureAsync<…, object>remain insrc/.The more useful part is
EmptyResponseBodyTests: a[Theory]over all 19 NSIDs driving realAtProtoClientcalls through a handler that answers 200 with an empty body, the way a real PDS does. As you noted, the old doubles returned{}— which deserializes fine and is precisely why 26 broken methods looked tested. I verified the new test isn't vacuous by reverting one call site and watching that case fail.It then caught a mistake of mine within the hour: undoing that probe with
git checkout <file>also discarded the six uncommittedGraphClientfixes, and the suite failed on exactly those six NSIDs.2. Both CI checks failed open — confirmed and closed
Reproduced it:
Skipped! - Failed: 0, Passed: 0, Skipped: 5 … exit=0.Took both of your suggestions.
--output-path "$ATPROTO_ASPIRE_MANIFEST"means one variable rather than two that must agree, andATPROTO_REQUIRE_INTEGRATION=1on both jobs turns a skipped gate into a failure. Verified in both directions:You put your finger on the thing that makes this PR embarrassing rather than merely buggy: it exists because a check nobody ran went unnoticed, and I'd added two more of the same shape.
3. Orphaned parameter — and it wasn't only the hostname
Confirmed, and the same mechanism affects
WithJwtSecretandWithPlcRotationKey: all are created before any override can run, so a superseded one stayed in the model and appeared as a manifest input nothing reads. Overrides now drop the parameter they replace (Replace(...)), covered by a[Theory]across all three in publish mode.4 & 5. Docs
Three summaries now say "typed
HttpClient" rather than "singleton", and both the XML docs anddocs/managed-pds.mdnote that it's transient: inject it into an endpoint or controller, where the request scope releases it; root-provider resolution keeps every instance alive until shutdown.Verification
build-and-testandpds-integrationboth green.@claude-bot Please do another review pass.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
I reviewed at
22aeb04, built and ran the suite myself, and probed two claims empirically.Verified
dotnet test tests/ATProtoNet.Tests/→ 1101 passed, 0 failed, 5 skipped (the manifest tests, gate off). Build clean.ProcedureAsync<…, object>remain insrc/— I checked every surviving two-type-arg call site by hand and all 35 name a real response type.EmptyResponseBodyTestsis the right shape: an empty 200, not{}, driven through realAtProtoClientmethods.Replacehelper does drop the superseded resource, and the run/publish split onPDS_DEV_MODE,PDS_HOSTNAMEand the hex secrets reads correctly.Disposecorrectly leaves a suppliedHttpClientalone.New findings
1.
WithDataBindMountproduces a container that can't start — the same failure class as the blobstore bug. (AtProtoPdsHostingExtensions.cs:113,:351)AddAtProtoPdsunconditionally adds.WithVolume($"{name}-data", "/pds"), andWithDataBindMountadds a bind mount at the same target. I ran it:Both annotations survive into the container spec. Docker and Podman both reject two mounts on one destination (
Duplicate mount point: /pds), sobuilder.AddAtProtoPds("pds").WithDataBindMount("./pds-data")never comes up — and that's the exact line indocs/managed-pds.md:103, commented "host directory instead of a volume", which is precisely what it doesn't do. Aspire's own integrations treatWithDataVolume()/WithDataBindMount()as mutually exclusive opt-ins for this reason; here the volume is always on. Fix is to drop the volume annotation when a bind mount is supplied. I couldn't run a container in this box to show the daemon rejecting it, which is itself the point: the newpds-integrationjob doesn't touch Aspire, so nothing in CI would catch this either.2. The CI hardening closed env-var drift but not filter drift. (
.forgejo/workflows/ci.yml:41,:93)ATPROTO_REQUIRE_INTEGRATION=1correctly turns a missing prerequisite into a failure. But the other way these steps can match nothing is the filter itself, and I confirmed that still exits 0:Rename
AspireManifestTestsorPdsAdminTestsand both steps stay green having asserted nothing — the same failure mode, reached from a different direction. One flag fixes it, verified:3. Nits.
IntegrationGate.ThrowIfRequired(IntegrationGate.cs:44) is never called —AspireManifestTests.Resources()hand-rolls the throw instead.EmptyResponseBodyTests._handleris assigned twice (field initializer at:22, then:28), so one handler is allocated and orphaned.EveryVoidProcedureIsCoveredassertsCount == 19, which only fires if someone edits that list — it can't detect a new void procedure added to a Lexicon client, which is the case it's described as guarding._httpClient.BaseAddress ??= baseUrisilently wins overoptions.Urlwhen both are set. Safe now that the guard validates the effective address, but an admin client quietly talking to a different server than configured could use a log warning.#1 is the one I'd act on before merge — it's a documented method on a new public API that cannot work. Say the word and I'll push it with a test asserting a single mount at
/pds, plus theTreatNoTestsAsErrorflag for #2. I pushed no changes; the branch is untouched at22aeb04.⏱ 5m 12s · 41 turns · tokens: 83.2k in / 18.0k out (+2674.1k cache-read)
📜 Full session transcript
Split done, per the review's offer to keep a clean boundary.
main: 19 call sites plusEmptyResponseBodyTests. 1418 unit tests pass.AdminClientsites only, which the managed-PDS work depends on and which the live-PDS integration tests here cover. It no longer changes the behaviour ofGraphClient,ActorClient,NotificationClient,IdentityClient,SyncClient, or the Ozone clients.The two sets of call sites are disjoint, so they can merge in either order; both touch
CHANGELOG.md, which is the only conflict to expect.Re-verified after the split: 1081 unit tests pass here, and the nine
PdsAdminTestsstill pass against a livebluesky-social/pdscontainer withATPROTO_REQUIRE_INTEGRATION=1, so the admin surface remains covered end to end by its own seven fixes. CI green on both branches.One consequence worth flagging:
fix/empty-response-bodyis based onmain, so it runsmain's workflow — plainbuild-and-test, without thepds-integrationjob or the fail-open hardening, since both arrive with this PR. #70 is therefore verified by unit tests and a local run rather than by the container job. If this PR merges first, a rebase would put #70 under the fuller CI.AddAtProtoPds always mounts a named volume at /pds, and WithDataBindMount added a second mount on the same destination. Both annotations reached the container spec, which the runtime rejects — confirmed with podman: Error: /pds: duplicate mount destination (exit 125) So the line documented in docs/managed-pds.md, commented "host directory instead of a volume", never came up. Supplying a data mount now replaces the default rather than adding to it, and WithDataVolume(name?) is added as the counterpart. Running the fixed single-mount container surfaced a second, separate trap: on an SELinux host a bind-mounted directory needs the container label or the PDS exits with "SqliteError: unable to open database file". Aspire mounts without relabelling, so that is documented on the method and in the guide rather than worked around. Also from review: - CI hardening closed env-var drift but not filter drift: `dotnet test --filter` also exits 0 when it matches nothing, so a renamed test class would leave a green step. Verified, and closed with TreatNoTestsAsError=true on both steps. - Dropped IntegrationGate.ThrowIfRequired, which nothing called. - The plaintext-HTTP guard runs at first resolution, not startup; said so on the API and corrected the CHANGELOG's "throws at startup". - A supplied HttpClient's BaseAddress silently wins over the configured URL; now logged as a warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>All fixed in
478d2e2, plus the two test nits in #70 (e6a3401). Both branches green.1.
WithDataBindMount— confirmed against a real runtimeYou couldn't run a container to show the daemon rejecting it; I could, and it does:
So
AddAtProtoPds("pds").WithDataBindMount("./pds-data")— the exact line indocs/managed-pds.md— could never start. Supplying a data mount now replaces the default rather than adding to it, andWithDataVolume(name?)is added as the counterpart (rename the volume, or restore it after a bind mount). Both covered by tests asserting a single mount at/pdsof the expected type.Running the fixed version surfaced a second, independent trap. With one bind mount the container starts and then dies:
Not the mount count — SELinux. On Fedora/RHEL a bind-mounted directory needs the container label, and Aspire's
WithBindMountdoesn't relabel. Identical container with-v …:/pds:Zcomes up healthy. There's no SELinux option on Aspire's API, so it's documented on the method and in the guide (chcon -Rt container_file_t, or stay on the default volume, which the runtime labels for you) rather than papered over. Worth knowing that the default path — a named volume — was never affected.2. Filter drift — verified and closed
Both steps now carry the flag. Your framing was the useful part:
ATPROTO_REQUIRE_INTEGRATIONclosed missing prerequisite, and this closes matched nothing — same green-but-empty outcome, different route. The comment in the workflow now names both.3. Nits
IntegrationGate.ThrowIfRequired— deleted; nothing called it._handlerdouble assignment — fixed in #70.EveryVoidProcedureIsCovered— you're right that it couldn't detect what its comment claimed. Replaced withEveryListedProcedureIsDispatchable, which checks what's actually verifiable (no unmapped or duplicated NSID) and says plainly that keeping the list current is manual. Fixed in #70.BaseAddress ??=— now logs a warning when the supplied client's address differs from the configured URL.Verification
Build clean, no new warnings. 1083 unit tests, 5 skipped. Manifest regenerated and asserted with the gate on and
TreatNoTestsAsError: 6 passed. 9 integration tests against a live PDS, same flags: 9 passed. CI green on both branches.@claude-bot Please do another review.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
I reviewed at
478d2e2, installed the SDK, built and ran everything myself, and probed the new code empirically rather than reading it.Verified
dotnet build -c Release— 0 errors. Distinct warning codes:CS1591(the repo's XML-doc noise), plusCS1573atXrpcClient.cs:597,CS0419inPlcOperationBuilder.cs,RZ10012inServerIntegrationSample. All three non-CS1591 ones are pre-existing onmain— theCS1573traces toea508ab(theproxyOverrideparameter), not to this PR. No new warnings.dotnet test tests/ATProtoNet.Tests/→ 1083 passed, 0 failed, 5 skipped.AspireManifestTestswithATPROTO_REQUIRE_INTEGRATION=1→ 6 passed. Output matches what you posted byte for byte: both hex secrets with nodefault,pds-admin-passwordkeeping itsgenerateblock,PDS_DEV_MODEabsent,PDS_HOSTNAMEas{pds-hostname.value},PDS_BLOBSTORE_DISK_LOCATIONpresent, and noAtProto__Pds__AllowInsecureHttponapi.TreatNoTestsAsErrordoes what you said — measured, not assumed: bogus filter → exit 1 with the flag, exit 0 without.AddAtProtoPds("pds").WithDataBindMount("./pds-data")leaves exactly one annotation (BindMount → /pds) andPDS_BLOBSTORE_DISK_LOCATIONstill resolves to/pds/blocks. I also checked the ordering hazard the fix could have introduced —WithProductionMode()afterAddAtProtoPdsstill resolvesPDS_DEV_MODE=falsein run mode, so the run-mode default doesn't win.AdminClient's seven void methods are onProcedureAsync<TRequest>; exactly 19ProcedureAsync<…, object>remain, matching the split to #69.XrpcClient.Dispose()is a no-op and nothing anywhere mutatesDefaultRequestHeaders, so the one-HttpClient-two-XrpcClients arrangement is sound.ATProtoNet.Pdsreferences outside the CHANGELOG's own history.No container runtime in this box, so the nine
PdsAdminTestsremain verified by you and CI, not by me.Findings
1.
Replacealso removes a caller-supplied parameter when an override is applied twice. (AtProtoPdsHostingExtensions.cs:139)Replaceremoves whatevercurrentis, and after the first overridecurrentis the caller's own parameter rather than the auto-created one. Measured:Same for
WithJwtSecret,WithPlcRotationKey, andWithHostname. Severity is low — theParameterResourceobject still resolves wherever it's referenced, and the manifest writer re-adds referenced parameters lazily — so the visible damage is a parameter missing from the dashboard and from the model for anything that enumerates it. But it's reachable from ordinary layering (a helper sets a default, a caller overrides it later), and the removal was only ever meant to target the parameterAddAtProtoPdscreated. Remembering which three those are, and removing only those, closes it.2. Worth knowing:
WithAdminPassword's parameter drop works for a different reason than the other two.ParameterResourceBuilderExtensions.CreateDefaultPasswordParameternever adds its parameter tobuilder.Resources— I confirmed a publish-modeAddAtProtoPds("pds")yieldspds, pds-hostname, pds-jwt-secret, pds-plc-rotation-keyand nothing else;pds-admin-passwordreaches the manifest only because the writer adds referenced parameters lazily. SoReplace'sResources.Removeis a no-op there, and the superseded parameter stays out of the manifest because nothing references it. The outcome is right; the mechanism isn't the one the[Theory](correctly scoped to the other three) demonstrates. Not a defect — just a load-bearing implementation detail of Aspire that the comment onReplacedoesn't mention.3. Nits.
CHANGELOG.md:69has a run-on: "…the correspondingPdsAdminClientwrappers The same defect affects 19 further procedures…" — missing a period.With*methods doArgumentNullException.ThrowIfNull(builder);WithHandleDomains,WithPlcUrl,WithAppView,WithCrawlers,WithProductionMode,WithInviteCodeRequired,WithBlobUploadLimit,WithReportService, andWithEmaildon't. Cosmetic inconsistency.PdsAdminClientappends its User-Agent to a suppliedHttpClientviaTryParseAdd, so constructing two clients over one hand-managedHttpClientaccumulates duplicate UA tokens. Unreachable through the DI path, since the factory hands out a freshHttpClientper resolution.AtProto__Pds__Urlis{pds.bindings.http.url}, alwayshttp://. So every published deployment of the documented wiring needs one deliberate operator action — TLS in front, orAllowInsecureHttp. That's the fail-closed design anddocs/managed-pds.mdstates it; I mention it only because the manifest makes it unconditional.Nothing here blocks the PR. #1 is the only one I'd bother fixing, and it's small. Say the word and I'll push it with a test that a second override leaves the first caller's parameter in the model, plus the CHANGELOG punctuation. I pushed no changes; the branch is untouched at
478d2e2.⏱ 7m 54s · 43 turns · tokens: 122.3k in / 31.3k out (+4036.5k cache-read)
📜 Full session transcript
@claude-bot /review
🤖 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.
✅ Verdict: approve — no blocking issues found.
Re-review: the one substantive issue from my previous pass —
HexSecretParameterDefault.WriteToManifestpotentially emitting a non-hex-constrainedgeneratedirective for a value the PDS parses as hex — is resolved. Publish mode now creates the JWT-secret/PLC-rotation-key parameters via a plainAddParameter(name, secret: true)with no default at all (so a deployment must supply the value), andWriteToManifeston the hex-generating default is now unreachable and throws defensively rather than emitting something misleading. This is verified directly against a real Aspire manifest by the newAspireManifestTests(assertingPDS_HOSTNAMEis a manifest reference,PDS_DEV_MODEis absent, the blobstore location is set, the two hex secrets carry no generated default, the admin password still does, and the consumer doesn't getAllowInsecureHttpin publish mode) plus a newpds-integrationCI job that runsPdsAdminTestsagainst the realghcr.io/bluesky-social/pdscontainer. I confirmedPdsAdminClientin the actual source matches its documented behavior (unauthenticated signup call, loopback/HTTPS enforcement validated against the effectiveHttpClient.BaseAddress, automatic invite-code minting). TheReplace()helper for overriding auto-created parameters (hostname/admin password/JWT secret/rotation key) correctly removes the supersededParameterResourcefrom the application model regardless of call order, since the parameters are all constructed up front.WithDataBindMount/WithDataVolumecorrectly clear the prior mount before adding a new one, fixing the duplicate-mount-destination bug described in the changelog.One accuracy nit, not blocking: the PR description's Notes section says "No AppHost sample project" was added to avoid the ~150MB Aspire AppHost SDK restore cost in CI, but the diff does add
samples/ManagedPdsSample.AppHost, wires it into the.slnx, and CI now builds and runs it every run. Flagged as a finding so the description can be corrected or the tradeoff reconsidered — it doesn't affect correctness of the shipped code.⏱ 1m 46s · 9 turns · tokens: 89.3k in / 8.3k out (+803.5k cache-read)
📜 Full session transcript
@ -29,2 +29,4 @@- name: Run unit testsrun: dotnet test tests/ATProtoNet.Tests/ --no-build --configuration Release --verbosity normal# Publishing is the path least covered by unit tests: it is where local-onlyThe PR description's Notes section says: "No AppHost sample project — the Aspire AppHost SDK pulls ~150 MB of orchestration/dashboard packages into every CI restore." But the diff adds
samples/ManagedPdsSample.AppHost(referencingAspire.Hosting.AppHost13.4.6), registers it inATProto.NET.slnx, and this CI job now restores/builds it as part of the main solution build and executes it in the "Verify Aspire publish manifest" step. So the description states the opposite tradeoff of what actually shipped — worth updating the PR description (or reconsidering whether the AppHost project belongs in the default solution/build) so it doesn't misrepresent the CI cost impact to future readers. Not a functional defect — the wiring itself is correct and well-tested.