Claude: changes for issue #83 #84
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!84
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-83"
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?
Closes #83.
🤖 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: request changes — blocking issues below.
The mechanical parts of this PR (URL building, cursor tracking, reconnect logic, the v1/v2 dispatch in
JetstreamEventParser) are internally consistent and well covered by the new unit tests — the resume/replay semantics, theWantedKinds/WantedCollectionsconflict validation, and the dictionary-header parsing inJetstreamDictionaryClientall check out against their own test suites.The bigger concern is the premise of the whole change. I'm not aware of a real, publicly documented "Jetstream v2" wire protocol matching what's described here — a lexicon-based XRPC endpoint
network.bsky.jetstream.subscribeEventsunder anxrpc.v1.jsonsubprotocol, an "atproto proposal 0015", hosts renamed from the well-knownjetstream1/jetstream2naming to barejetstream.us-east/jetstream.us-west, and anetwork.bsky.jetstream.getZstdDictionaryendpoint. If this protocol doesn't actually exist (or doesn't exist in this shape), the SDK is shipping a large, entirely non-functional surface (new client, new event types, new exception type, ~650 lines of docs) that will simply fail to connect against real infrastructure. The CHANGELOG also asserts this was "Verified against the livejetstream.us-east/jetstream.us-westinstances", but the diff contains no integration test exercising real infrastructure (only scripted/mocked unit tests) — that claim is unverifiable from the change itself and, if the protocol is fictional, is false. Please confirm this against an authoritative source (an actual Bluesky/atproto spec or announcement) before merging, since this is the kind of claim that's very expensive to walk back once published in a NuGet package's CHANGELOG and public API.Minor: in
JetstreamEventParser.ParseCommitFields, therootparameter is unused in the function body (both call sites pass it but it's never read), and the new XML doc comment on the method claims the collection NSID is "outside" the commit object when it's actually read fromcommititself, same as before — worth a cleanup pass.⏱ 2m 30s · 1 turns · tokens: 63.9k in / 15.8k out (+24.5k cache-read)
📜 Full session transcript
@ -9,6 +9,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0### Added- **Jetstream v2 is now supported alongside v1** (Issue #83) — Jetstream's second wire protocol serves at `/xrpc/network.bsky.jetstream.subscribeEvents` under the `xrpc.v1.json` subprotocol (atproto proposal 0015) and differs from the frozen v1 wire in almost every particular: a self-describing `{"$type":"message","payload":{…}}` envelope per frame, flat commit fields instead of a nested `commit` object, filters renamed `collections` / `dids` and joined by a new `kinds`, a monotonic sequence number as the cursor instead of a `time_us` timestamp, a `sync` event kind v1 never emits, and out-of-band `#info` / `error` frames. `JetstreamClient` and `JetstreamConsumer` speak both, selected by the new `JetstreamConsumerOptions.Protocol`This entry describes a "Jetstream v2" wire protocol (new lexicon
network.bsky.jetstream.subscribeEvents,xrpc.v1.jsonsubprotocol, "atproto proposal 0015", renamed public hosts, a versioned zstd dictionary endpoint) that I can't corroborate as a real, documented AT Protocol/Bluesky service. If it doesn't exist as described, this whole feature — client code, new exception type, new hostnames, and the extensivedocs/jetstream.mdrewrite — is dead on arrival against real servers. The closing bullet's claim of having been "Verified against the livejetstream.us-east/jetstream.us-westinstances" is also not backed by anything in this diff (only mocked/scripted unit tests are added, no integration test hitting real infrastructure). Please confirm the protocol's existence/shape against an authoritative source before merging, and either add real integration coverage or soften/remove the verification claim.@ -125,1 +229,4 @@Cursor = cursor,Collection = collection,RKey = rkey,Operation = operation,rootis accepted but never read insideParseCommitFields(collection/rkey/operation/cid/record are all pulled fromcommit, at both v1 and v2 call sites). Combined with the new doc comment's claim that the collection NSID lives "outside" the commit object — which isn't true, it's read fromcommitjust like before — this parameter and comment are misleading dead weight. Worth dropping the parameter and fixing the comment.@claude-bot /review Please re-review, you should now have access to the internet and therefore be able to verify the existance of this new Jetstream v2 API.
🤖 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.
@claude-bot /review Please re-review, you should now have access to the internet and therefore be able to verify the existance of this new Jetstream v2 API.
🤖 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: request changes — blocking issues below.
No new commits have landed on this branch since my previous review (branch tip
42ec39d, 2026-08-14T01:42Z, predates my prior review at 01:45Z) — this is the same diff, unchanged. Both blocking points from that review still stand.Unresolved and confirmed via web research this pass: I checked the official
bluesky-social/jetstreamGitHub repo and README, and searched for independent documentation of the wire protocol. The real, documented Jetstream protocol matches only what this PR calls "v1": a/subscribeWebSocket endpoint,wantedCollections/wantedDidsquery params, akindfield per message, and anoptions_updatemechanism for live filter changes. I found no corroboration for any of: anetwork.bsky.jetstream.subscribeEventsXRPC lexicon, anxrpc.v1.jsonWebSocket subprotocol, "atproto proposal 0015", anetwork.bsky.jetstream.getZstdDictionaryendpoint, akindsfilter,JetstreamSyncEvent/sync messages, or the renamed hostsjetstream.us-east.bsky.network/jetstream.us-west.bsky.network(as opposed to the realjetstream1/jetstream2naming). The official repo README describes Jetstream as an archive/replay/streaming service but doesn't reference any v2 wire protocol matching this shape.This reinforces rather than resolves my prior concern: if this protocol doesn't exist as described, this PR ships a large, non-functional public API surface (new client, exception type, endpoint constants, ~650 lines of docs) under a CHANGELOG claim of having been "verified against the live
jetstream.us-east/jetstream.us-westinstances" — a claim the diff still has no integration test to back (only mocked/scripted unit tests). Please confirm the protocol's existence against an authoritative atproto/Bluesky source before merging, or soften/remove the verification claim and clearly mark this as speculative/forward-looking if the protocol is genuinely unreleased-but-planned.The minor cleanup from last time is also still present:
JetstreamEventParser.ParseCommitFields'srootparameter remains unused in the body (onlycommitis read), unchanged from before — not blocking on its own.Keeping this at request_changes since the primary blocking concern was not addressed (no new commits since it was raised).
⏱ 3m 30s · 16 turns · tokens: 69.5k in / 4.8k out (+1155.0k cache-read)
📜 Full session transcript
@ -9,6 +9,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0### Added- **Jetstream v2 is now supported alongside v1** (Issue #83) — Jetstream's second wire protocol serves at `/xrpc/network.bsky.jetstream.subscribeEvents` under the `xrpc.v1.json` subprotocol (atproto proposal 0015) and differs from the frozen v1 wire in almost every particular: a self-describing `{"$type":"message","payload":{…}}` envelope per frame, flat commit fields instead of a nested `commit` object, filters renamed `collections` / `dids` and joined by a new `kinds`, a monotonic sequence number as the cursor instead of a `time_us` timestamp, a `sync` event kind v1 never emits, and out-of-band `#info` / `error` frames. `JetstreamClient` and `JetstreamConsumer` speak both, selected by the new `JetstreamConsumerOptions.Protocol`Same concern as my prior review, still unaddressed (no commits landed since then): I cannot find any authoritative source (official
bluesky-social/jetstreamrepo/README, Bluesky docs, or independent third-party Jetstream client docs) describing a "Jetstream v2" protocol matching this shape —network.bsky.jetstream.subscribeEventsXRPC lexicon,xrpc.v1.jsonsubprotocol, "atproto proposal 0015",getZstdDictionary, akindsfilter, sync events, or hosts renamed tojetstream.us-east.bsky.network/jetstream.us-west.bsky.network. Everything I could verify (repo README, search results for the real wire format) matches only what this PR calls v1:/subscribe,wantedCollections/wantedDids,kind,jetstream1/jetstream2hostnames. If v2 as described here doesn't exist, this changelog's claim of having been "Verified against the livejetstream.us-east/jetstream.us-westinstances" is false, and the entire v2 code path (client, exception type, docs) will fail to connect against real infrastructure. Please cite an authoritative source (atproto spec repo, Bluesky announcement) confirming this before merging.@claude-bot /review https://atproto.com/blog/introducing-bluesky-protocol-services What about this then?
🤖 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 of the same commit (
42ec39d, no new commits since my last two passes) — but this time I independently verified the protocol claim that blocked the previous two reviews, and it checks out. I fetched the actual Bluesky documentation site (bsky.network/docs/jetstream,/jetstream-sdk,/jetstream-replay,/jetstream-self-host) directly rather than relying on search summaries, and confirmed via verbatim page content — not paraphrase — that: the v2 live-tail path is/xrpc/network.bsky.jetstream.subscribeEventsunder thexrpc.v1.jsonsubprotocol; the envelope is{"$type":"message","payload":{...}}; filters are namedcollections/dids/kinds; commit/identity/account/sync are the four event kinds; the cursor isseq, replayed inclusively; a collection filter constrains commit events only; the public v2 hosts are literallyjetstream.us-east.bsky.network/jetstream.us-west.bsky.networkwith the olderjetstream1/jetstream2hosts serving the frozen v1 wire at/subscribe; and the replay archive API is exactlyplanSnapshot/listSegments/getSegment/getBlock, gated by an API key, with a documented 36-hour lookback window and a 400 when backfill falls outside it. This matches the PR's implementation, CHANGELOG description, and docs rewrite in remarkable and specific detail — this is not a fictional protocol. My prior blocking concern is resolved.One caveat surfaced by the same research, worth a non-blocking follow-up: I could not find the
getZstdDictionaryXRPC method, dictionary versioning, or the specific error/info names (ConsumerTooSlow,OutdatedCursor,UnknownZstdDictionary) anywhere in the official docs I fetched, even though every other v2 detail was corroborated precisely. That doesn't mean this part is wrong — it may simply live on a page I didn't reach (e.g. an SDK internals page or the HTTP reference SPA, which didn't render for me) — but given how much of this PR's documented surface (JetstreamDictionaryClient,ZstdDictionaryId,JetstreamConnectExceptionreasons) rests on it, it's worth a quick confirmation pass against the real server (or the linked HTTP reference) in a follow-up, rather than blocking this PR on it. Also still true: the CHANGELOG's claim of having been "verified against the live instances" isn't backed by an integration test in this diff, but that's consistent with this repo's existing pattern of gating integration tests behind[RequiresPdsFactAttribute]-style attributes and isn't in itself a defect.The mechanical review from my first pass stands: URL building, cursor tracking, reconnect/rewind logic, and the v1/v2 parser dispatch are internally consistent and well covered by the new unit tests. The minor unused-
root-parameter / stale-doc-comment nit inParseCommitFieldsis still present but is cosmetic, not correctness-affecting.Since the blocking issue from both prior passes is resolved, this moves to approve.
⏱ 4m 2s · 24 turns · tokens: 85.2k in / 13.7k out (+2051.1k cache-read)
📜 Full session transcript
@ -9,6 +9,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0### Added- **Jetstream v2 is now supported alongside v1** (Issue #83) — Jetstream's second wire protocol serves at `/xrpc/network.bsky.jetstream.subscribeEvents` under the `xrpc.v1.json` subprotocol (atproto proposal 0015) and differs from the frozen v1 wire in almost every particular: a self-describing `{"$type":"message","payload":{…}}` envelope per frame, flat commit fields instead of a nested `commit` object, filters renamed `collections` / `dids` and joined by a new `kinds`, a monotonic sequence number as the cursor instead of a `time_us` timestamp, a `sync` event kind v1 never emits, and out-of-band `#info` / `error` frames. `JetstreamClient` and `JetstreamConsumer` speak both, selected by the new `JetstreamConsumerOptions.Protocol`I independently verified the core v2 protocol claims here (endpoint path, subprotocol, envelope shape, filter names, hostnames, cursor semantics, and the replay method names) against the real Bluesky docs site and they check out precisely — this resolves the concern from my prior two reviews. One piece I could not corroborate from any doc page I reached is the
network.bsky.jetstream.getZstdDictionarymethod and dictionary versioning described later in this entry and implemented inJetstreamDictionaryClient. Given everything else checked out, I don't think this blocks merging, but it's worth a follow-up confirmation against the HTTP reference or a live call before leaning on it too heavily.@ -125,1 +229,4 @@Cursor = cursor,Collection = collection,RKey = rkey,Operation = operation,Non-blocking carryover from my first review:
rootis still accepted but never read in the body ofParseCommitFields(both call sites only usecommit), and the new doc comment above it says the collection NSID lives "outside" the commit object, which isn't accurate — it's read fromcommitat both call sites. Worth a quick cleanup, not merge-blocking.@claude-bot Please pick up the review findings (the actual ones :D)
🤖 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.
Picked up both actual findings and pushed to
claude/issue-83(89950ab).1.
ParseCommitFieldscleanup (src/ATProtoNet/Streaming/JetstreamEventParser.cs:190) — dropped the unusedrootparameter and rewrote the doc comment, which claimed the collection NSID lives outside the commit object. It doesn't: both wires name the commit fields identically, only the enclosing element differs (nestedcommiton v1, the payload itself on v2).2. The "verified against the live instances" CHANGELOG claim — now backed by tests instead of an assertion. Added
tests/ATProtoNet.IntegrationTests/JetstreamV2Tests.cswith a new[RequiresJetstreamFact]gate (ATPROTO_TEST_JETSTREAM=true, host overridable viaATPROTO_JETSTREAM_URL), following the existingRequiresPdsFactpattern — no PDS and no credentials needed, only outbound internet, so CI keeps skipping them. Eight tests: commit parsing, thekindsandcollectionsfilters, monotonic sequence cursors, inclusive cursor replay, the pre-upgrade 400s, the dictionary fetch, and the v1 wire a v2 host still serves. The CHANGELOG bullet now describes those tests rather than claiming an unverifiable manual run; CLAUDE.md documents the new env var.On the
getZstdDictionarycaveat from the approving review — I probed the live server directly, and it's real:GET /xrpc/network.bsky.jetstream.getZstdDictionary→ 200, 65536 bytes,x-zstd-dictionary-id: 20260811. The payload starts with the zstd dictionary magic37 a4 30 ecfollowed bycb 27 35 01— little-endian20260811, i.e. exactly the header's ID, which confirmsJetstreamDictionaryClient's "read the ID out of the dictionary's own header" approach.subscribeEvents?zstdDictionary=1with a real WebSocket handshake →400 {"error":"UnknownZstdDictionary","message":"unknown zstd dictionary id 1; current dictionary id is 20260811 ..."}; with the correct ID →101 Switching Protocols,sec-websocket-protocol: xrpc.v1.json, compressed binary frames.?cursor=1→400 {"error":"CursorTooOld", ...}; a malformed collection →400 {"error":"InvalidRequest", ...}. Note the ordering: cursor and filter validation happen before the upgrade check, dictionary validation after it — all three still pre-upgrade, as the code assumes.{"$type":"message","payload":{"$type":"network.bsky.jetstream.subscribeEvents#identity",...,"seq":24707882424,"time":"2026-08-14T02:11:17.103502Z"}}.Verified: unit tests 1237 passed / 0 failed / 5 skipped; the new integration tests 8 passed against
jetstream.us-eastwith the env var set, and 8 skipped without it. (The container had no .NET SDK — I installed 10.0.400 to build and run.)⏱ 5m 46s · 38 turns · tokens: 70.5k in / 18.0k out (+2016.0k cache-read)
📜 Full session transcript
@claude-bot /review Please re-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.
New commit
89950ab(after my last approval of42ec39d) directly addresses the two follow-up notes from my previous pass:tests/ATProtoNet.IntegrationTests/JetstreamV2Tests.csis added, gated by the new[RequiresJetstreamFact]/ATPROTO_TEST_JETSTREAM=true(mirroring the existingRequiresPdsFactpattern, skips by default in CI since CI has no outbound internet). It exercises exactly what the CHANGELOG now claims: commit parsing with sequence cursors, thekinds/collectionsfilters, inclusive cursor replay, the pre-upgrade 400s forCursorTooOldandUnknownZstdDictionary, thegetZstdDictionaryfetch (including that the returned dictionary's self-reported ID round-trips), and the v1 wire still served by a v2 host. The CHANGELOG bullet was reworded from an unverifiable "Verified against the live instances" assertion to point at this concrete, runnable test class — this resolves the gap I flagged for thegetZstdDictionary/dictionary-versioning surface specifically, since it's now the thing exercised against the real server.ParseCommitFieldsno longer takes the unusedrootparameter, and its doc comment no longer claims the collection NSID lives "outside" the commit object.I independently confirmed via
git log/git showthat this commit is genuinely present onclaude/issue-83and is included in the diff under review, not just described. Combined with my prior pass's independent verification of the core v2 protocol claims (endpoint path, subprotocol, envelope, filter names, hostnames, cursor semantics) against the real Bluesky docs, I have no remaining blocking concerns. The rest of the implementation (URL building, cursor tracking/reconnect logic, v1/v2 parser dispatch) remains internally consistent and well covered by unit tests, as noted previously.⏱ 0m 31s · 8 turns · tokens: 84.0k in / 2.1k out (+733.0k cache-read)
📜 Full session transcript