fix: XRPC procedures with no output throw JsonException on every call #70
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!70
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/empty-response-body"
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?
Fixes #69.
Every XRPC procedure whose Lexicon declares no output asked for a deserialized response, so the empty body a real server returns failed before the caller saw anything:
They failed on every call, regardless of what the server did. Not a corner of the API —
PutPreferencesAsync, the six mute/unmute methods,UpdateSeenAsync,RegisterPushAsyncandUpdateHandleAsyncare ordinary client calls.Change
19 call sites move from
ProcedureAsync<TRequest, object>to the body-onlyProcedureAsync<TRequest>overload that already existed:IdentityClientupdateHandle,submitPlcOperation,requestPlcOperationSignatureSyncClientnotifyOfUpdate,requestCrawlActorClientputPreferencesGraphClientmuteActor,unmuteActor,muteActorList,unmuteActorList,muteThread,unmuteThreadNotificationClientupdateSeen,registerPushcommunication.deleteTemplate,team.deleteMember,set.addValues,set.deleteValues,set.deleteSetNo public signatures change — every one of these methods already returned
Taskand discarded the value, so the only difference is that they stop throwing.tools.ozone.set.deleteSetis included even though its Lexicon does declare an output: the method throws the value away today, so deserializing it only creates a way to fail.Test
The defect survived because the existing test doubles return
{}, which deserializes perfectly well.EmptyResponseBodyTestsis a[Theory]over all 19 NSIDs driving realAtProtoClientcalls through a handler that answers 200 with an empty body, the way a real server does. Verified non-vacuous: reverting a single call site fails exactly that case.Scope
Split out of #68 at review request. That PR keeps the seven equivalents in
AdminClient, which the managed-PDS work depends on and which its live-PDS integration tests cover. The two sets of call sites are disjoint, so these can merge in either order (both touchCHANGELOG.md).Verification
dotnet buildclean; 1418 unit tests pass (20 new). Found by runningPdsAdminClientagainst a realghcr.io/bluesky-social/pdscontainer for the first time in #68.🤖 Generated with Claude Code
Every procedure whose Lexicon declares no output asked for a deserialized response, so the empty body a real server returns failed with "The input does not contain any JSON tokens" before the caller saw anything. They failed on every call regardless of what the server did. This is not a corner of the API: PutPreferencesAsync, the six mute/unmute methods, UpdateSeen, RegisterPush and UpdateHandle are ordinary client calls. All 19 sites now use the body-only ProcedureAsync<TRequest> overload that already existed. No public signatures change — each of these methods already returned Task and discarded the value. The defect survived because the test doubles returned `{}`, which deserializes perfectly well. EmptyResponseBodyTests drives all 19 NSIDs through a handler that answers 200 with an empty body, the way a real server does; reverting any one call site fails its case. Found by running PdsAdminClient against a real bluesky-social/pds container in #68, which fixes the seven equivalents in AdminClient that the managed-PDS work depends on. closes #69 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>🤖 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.
Correct, well-scoped fix.
ProcedureAsync<TRequest>(verified inXrpcClient.cs:267-284) posts the body and callsEnsureSuccessAsyncwithout touchingresponse.Content, so it's immune to the empty-bodyJsonExceptionthatProcedureAsync<TRequest, object>hit when deserializing a genuinely empty response. All 19 call-site changes match methods that already returnedTaskand discarded the value, so no public API changes as claimed. The newEmptyResponseBodyTestsfixture is a good regression guard — it returns a real empty 200 body (unlike the old{}doubles) and exercises all 19 NSIDs through actualAtProtoClientcalls, plus a coverage-count guard against future void procedures being added uncovered. ThedeleteSetinclusion despite its Lexicon declaring output is reasonable and clearly justified in the description. CHANGELOG entry is present and detailed per repo convention. Nothing to block on.⏱ 0m 19s · 6 turns · tokens: 26.0k in / 1.2k out (+260.4k cache-read)
📜 Full session transcript
Two test nits from the review of #68 land in this PR's file, both fixed in
e6a3401:_handlerwas assigned twice — field initializer and constructor — allocating one handler and orphaning it. Now assigned once.EveryVoidProcedureIsCoveredassertedCount == 19, which only fires if someone edits that list. It could not detect a new void procedure being added to a Lexicon client, which is what its comment claimed to guard.The second is the interesting one, because there is no honest automated version: nothing in the test project can see a Lexicon client gaining a method. Rather than leave a check whose comment overstates it, it now verifies what's actually verifiable — that no listed NSID is unmapped or duplicated (so the list and the dispatch switch can't drift apart) — and says plainly that keeping the list current is manual.
1418 unit tests pass; build clean.
@claude-bot Please fix the small conflict and rebase based on main to get the latest state.
🤖 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.
e6a340104a5106bada90Rebased onto
mainand force-pushed tofix/empty-response-body(now5106bad, sitting directly on4b2c4ce).The conflict
Only
CHANGELOG.mdconflicted, in the### Fixedsection. Two things were tangled there:Seven AdminClient methodsbullet from #68, since they're the same defect and now read as a pair.Issue #66bullet (com.atproto.sync.subscribeReposcould miss events after the handshake) — dropped. It came in as part of this branch's base (41f16e1), but #68 deliberately removed bothIssue #66entries when it deleted the in-process PDS — thePdsSequencerthat bullet describes no longer exists. Re-adding it during the rebase would have documented a fix to deleted code. Confirmed bygit show 4b2c4ce -- CHANGELOG.md, which shows both lines being removed.No code conflicts: #68 touched
AdminClientonly, and these 19 call sites are in the Lexicon clients — the disjointness the PR description claimed held up.Verification
No .NET SDK in this container, so I installed 10.0.302 first.
dotnet build -p:EnableSourceControlManagerQueries=false— 0 errors (1117 warnings, all pre-existing CS1591/RZ10012).dotnet test tests/ATProtoNet.Tests/— 1103 passed, 0 failed, 5 skipped.EmptyResponseBodyTestsspecifically — 20 passed.Two notes on the numbers. The total is 1103, not the 1418 quoted earlier: #68 removed the in-process PDS and its test suite, so that drop is the new base, not this branch. The 5 skips are the environment-gated
AspireManifestTests, which skip when no published Aspire manifest is present — also from #68, unrelated to this change.Diff against main is unchanged in substance: 8 Lexicon client files, the new test file, and 2 added
CHANGELOG.mdlines.⏱ 3m 6s · 29 turns · tokens: 39.7k in / 7.4k out (+1089.4k cache-read)
📜 Full session transcript
@claude /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.
Same fix I approved previously — verified again against
XrpcClient.cs:301-318: the body-onlyProcedureAsync<TRequest>overload never readsresponse.Content, so it can't hit the empty-bodyJsonExceptionthatProcedureAsync<TRequest, object>throws. All 19 call-site changes still line up with methods that already returnedTaskand discarded the value, so no public API changes.EmptyResponseBodyTestsstill exercises all 19 NSIDs with a genuinely empty 200 body. No issues found; nothing new introduced since my last pass.⏱ 0m 26s · 8 turns · tokens: 25.3k in / 1.3k out (+357.6k cache-read)
📜 Full session transcript