refactor!: debloat streaming, repo and core wire primitives #182

Grandiras · 2026-09-27T12:58:43Z

Grandiras commented

2026-09-27 12:58:43 +00:00

Part of #180 (pass A: streaming, repo and core wire primitives).

Fewer lines and one way to do each thing in the streaming, repository and wire code, with the same behaviour except where an owner decision removes something. The existing tests are the safety net. Every regression, security and interop test still passes, retargeted where its API moved.

What changed

Streaming (S1, O1, owner decisions)

One stream loop. EventStreamLoop no longer knows about frames. A handler reads raw socket messages, and CborEventStreamHandler<T> reads the {op, t} DAG-CBOR header for the XRPC streams (firehose, labels, chat moderation). JetstreamConsumer (its own copied reconnect loop) and TapChannel.ReadAllAsync (another copy) now run on it. TapChannel keeps its acknowledgements by adapting its duplex connector to the loop's connector seam.
One cursor preamble. CursorTracker.StartAsync replaces the four copies of load, log, Start and flush in the firehose, label, Jetstream and replay consumers. The tracker is IAsyncDisposable, so await using replaces the finally { FlushAsync } blocks.
One XRPC stream endpoint. CborEventStreamHandler.Endpoint(serviceUrl, nsid, cursor) builds the endpoint for the firehose, label and chat moderation streams.
FirehoseClient and JetstreamClient are removed. The consumers cover them: Reconnect.MaxAttempts = 0 reads a single connection, and no CursorStore means no persistence.
FirehoseVerifier is removed, with VerificationResult and TypedFirehoseConsumerOptions.Verifier. VerifyCids keeps the local CID check, now inside the consumer. SyncVerifier is the signature path.
Options split (O1). StreamConsumerOptions keeps ServiceUrl, Reconnect, Logger, OnStreamError and OnEventDropped. The new CursorStreamConsumerOptions adds CursorStore, StreamId and CursorPersistInterval. ChatModerationEventConsumerOptions and TapClientOptions derive from the base: their own copies of the shared settings go, TapClientOptions.ServiceUrl becomes a string like every other stream's, and Tap's OnError becomes the shared OnEventDropped.
The stream contract is documented once, on StreamConsumerOptions: at-least-once delivery, the cursor interval, cancellation and error frames. The six consumer classes, StreamReconnectPolicy, IStreamCursorStore and EventStreamException point to it with <see cref>. docs/firehose.md has one "Delivery, cursors and errors" section, which jetstream.md and tap.md link to.

PLC export stream (P1)

PlcClient.StreamExportAsync runs on StreamSocket. To support that, StreamSocketOptions gains an HttpMessageInvoker (so the identity address policy still applies) and a per-connection message limit. StreamSocket also sends the SDK's User-Agent, which the PLC client already did.
A close with a reason, which is how the PLC directory reports OutdatedCursor, is now an EventStreamException whose Error is the reason. PlcExportStreamException is removed, and the test seam is a StreamConnector.

Repository and wire primitives (R1, M1, C1, D1, W1)

R1. CommitBlock.Read splices sig out in the same strict pass that reads the fields, refuses anything but version 3, and keeps the duplicate-key refusal of the old strict splice.
- CommitBlock.FromCar(car, out reader, expectedRoot) is the one "parse the CAR, find the root commit" preamble for RepoSyncVerifier, RepoSnapshot and RecordProof.
- FirehoseVerifier.ExtractSignedView / MapHeaderLength / WriteMapHeader and RecordProof.ReadCommit are gone. A 20-line CommitBlock.WriteMapHeader keeps the pinned boundary vectors.
M1.
- MstNodeData.ReadKeys(lower, upper) rebuilds a node's keys and checks them (valid, strictly increasing, inside the parent's range) for MerkleSearchTree, PartialMerkleSearchTree and RecordProof. MerkleSearchTree.Deserialize checks ranges now, not an in-order last key. That is equivalent: every subtree is non-empty, so a subtree linked twice still fails.
- MstNodeData.WriteStart, WriteEntry and WriteEnd are the one encoding of a node. ToBytes and the partial tree's Encode both use them, and the partial tree keeps its allocation-free encode. I wrote three steps rather than an instance Write(CborWriter) for that reason.
- One generic IsValidKey<T> serves strings and node bytes.
- DagCborDecoder.IsMalformed replaces six of the seven copied CborContentException catch lists. The seventh is in Spaces/: see Deferred.
C1. Base32Lower.TryDecode is the one canonical base32 decoder. Cid and CidComputation.DecodeCidString both use it, so the latter now refuses non-canonical text too. The internal Cid.FromBytes replaces nine Cid.Parse(EncodeCidToString(x)) round trips, and DagCborEncoder decodes $bytes through LexBase64.
D1. DagCborDecoder.TryValidate is removed. Decode already covers the 100k-nesting, float and malformed-input cases.
W1. AtProtoHttp.WithScheme(uri, webSocket) replaces the five ws↔http converters: the Jetstream endpoint, the archive client, the resync upstream, the Tap channel and the PLC stream.

Options (C-Q4, C-Q5)

RepoResyncOptions keeps only Fetchers.
- Upstream and AllowPrivateNetworks go: pass IRepoFetchers.
- FetchTimeout goes. The fetchers bound their own downloads, and the coordinator keeps the same five-minute per-attempt cap internally.
- The never-set limits become internal defaults with the same values, still settable by tests.
- The default fetchers own their clients, so the coordinator no longer creates one and disposes it.
JetstreamReplayConsumer has one way to configure it. The (options, archiveClient) constructor goes, so the archive client is always built from JetstreamArchiveOptions. JetstreamArchiveOptions.AfterSeq also goes, because ReplayAsync(afterSeq) already takes the resume position. The AfterSeq < BeforeSeq check now runs in ReplayAsync and covers a stored cursor too.

Tests (T2, T4 firehose part, loop collapse)

EventStreamLoopTests holds the loop contract, run through the firehose consumer:
- a theory that the cursor is saved on break, cancellation and a thrown failure;
- attempts exhausted, with and without a last failure;
- the attempt count resets after a frame;
- a failed connection is retried;
- non-retryable failures (refused upgrade, FutureCursor) throw at once;
- a retryable error frame is reported and reconnects.
  The duplicates in the Jetstream (v1/v2), Tap and firehose files go. The consumer-specific ones stay: cursor semantics, the Jetstream exception type, info and error frames, Tap acknowledgements, the chat token.
T2. The two Jetstream URL builder files merge into JetstreamEndpointTests: protocol theories for paths, filters and compression, and one validation theory that now also pins each message.
T4. FirehoseVerifierTests becomes CommitBlockTests: the splice, the header-boundary vectors, and a malformed-commit theory that adds version 2 and duplicate keys. The null, empty and malformed blocks cases become a VerifyCids consumer theory. The ExtractSignedView interop checks in RepoCommitTests, RepoWritePathTests and RecordProofTests now use CommitBlock.Read.
FirehoseVerifierIdentityTests goes with the verifier. RepoSyncVerifierTests already pins the same refetch-once and invalidate rules: refreshes, rotated keys and invalidations.
The Jetstream consumer tests now script wire frames through a shared ScriptedJetstream and JetstreamFrames, so the parser path is covered too.
The PLC stream tests run on a loopback Kestrel server through the real StreamSocket.

Breaking changes (each has a CHANGELOG bullet and a migration-guide entry)

Removed or changed	Migration
`FirehoseClient` (with `SubscribeLabelsAsync`), `JetstreamClient`	`TypedFirehoseConsumer` / `LabelStreamConsumer` / `JetstreamConsumer` `.ConsumeAsync(cursor)`; for a single connection, set `Reconnect = new StreamReconnectPolicy { MaxAttempts = 0 }`
`FirehoseVerifier`, `VerificationResult`, `TypedFirehoseConsumerOptions.Verifier`	`SyncVerifier = new RepoSyncVerifier()` (in-memory state by default), plus `Resync`; `VerifyCids` for CIDs alone
`DagCborDecoder.TryValidate`	`DagCborDecoder.Decode` and catch `FormatException`; key order is no longer checked
`JetstreamReplayConsumer(options, archiveClient)`, `JetstreamArchiveOptions.AfterSeq`	configure through `JetstreamArchiveOptions`; `ReplayAsync(afterSeq)`
`StreamConsumerOptions.CursorStore` / `StreamId` / `CursorPersistInterval` moved to `CursorStreamConsumerOptions`	recompile (source compatible)

These are new in 0.7 and never shipped, so their Unreleased Added bullets are amended instead of getting breaking bullets:

PlcExportStreamException: StreamExportAsync now throws EventStreamException with Error set to the close reason, and a connect failure is an EventStreamException instead of DidResolutionException.
TapClientOptions.OnError / Uri ServiceUrl.
The chat moderation options' own copies of the shared settings.
The RepoResyncOptions knobs.

Existing Unreleased bullets and docs that named the removed types were brought up to date.

PublicAPI.Shipped.txt: 67 lines removed, and the eight CursorStreamConsumerOptions lines added. Unshipped.txt is empty.

Behaviour changes worth knowing

A server that closes a stream abnormally with a reason now fails that connection with an EventStreamException whose Error is the reason. Before, it was a silent close. A retryable reason still reconnects; a FutureCursor close is no longer retried forever.
On a consumer, a frame that is skipped (an info frame or an unreadable one) resets the reconnect count, as it already did on the firehose loop. Before, only a delivered event reset it on the Jetstream consumer.
CidComputation.DecodeCidString and TryDecodeCidString refuse upper case, padding and non-canonical trailing bits. These have a Changed bullet.

Verification

dotnet build: 0 warnings, 0 errors (full solution, including doc snippets, samples and integration tests).
Unit tests (before the review fixes): 5027 passed, 0 failed, 5 skipped, run twice. The baseline on origin/main was 5059 / 0 / 5; the difference is the collapsed duplicates listed above.

Performance: a Stopwatch harness, Release build, a scripted connector with no I/O, and interleaved A/B runs of origin/main against this branch. The machine is shared: 12 threads at a load average of about 6 from other agents, so round-to-round noise is ±5%. The table shows the best of three runs of 31 rounds each. Allocations are per run.

Path	origin/main	this branch
Firehose, 100k `#commit` frames, parse	215.9 ms, 419.1 MiB	219.9 ms, 419.1 MiB
Firehose, 100k frames, collection filter miss (CBOR scan)	57.3 ms, 88.5 MiB	58.1 ms, 88.5 MiB
Firehose Sync 1.1, 2k verified commits (signature, inversion)	503.5 ms, 88.9 MiB	497.8 ms, 87.8 MiB
Jetstream v2 live, 100k JSON frames	159.3 ms, 152.6 MiB	163.0 ms, 152.6 MiB

Allocations are unchanged on every path, and the Sync 1.1 path is about 1% faster now that the commit block is read in one pass. The loop paths measure 1–2% slower, inside the noise. That fits the one or two extra virtual calls per frame that sharing the loop costs. The Jetstream archive decode (JetstreamSegmentReader) is untouched.

Line counts (`loc.py origin/main HEAD`)

Area	code	xmldoc	comments
`src/ATProtoNet/Streaming`	4563 → 4008	2041 → 1834	181 → 134
`src/ATProtoNet/Repo`	2363 → 2211	743 → 770	129 → 121
`src/ATProtoNet/Identity`	2564 → 2460	1350 → 1336	101 → 100
`src/ATProtoNet/Tap`	458 → 405	187 → 192	3 → 2
`src/ATProtoNet/Http`	1175 → 1192	728 → 732	40 → 40
src total	−847	−186	−57
`tests/ATProtoNet.Tests`	49571 → 48825 (−746)	1470 → 1474	1847 → 1812
`docs`	12818 → 12736 (−82)
`PublicAPI.*.txt`	9983 → 9916 (−67)

Review fixes

Six new commits on top of 967439a. Each code fix has a regression test that fails without it; I checked by running the test against the committed source.

A finished bounded replay can run again (848d897). BeforeSeq is inclusive, so a completed snapshot leaves its store at exactly BeforeSeq, and a second run threw. A start at or above BeforeSeq, explicit or stored, now ends the replay with nothing to read, which is main's result. It also no longer spends a metered plan request on an empty window. Test: BoundedSnapshot_RunAgainWithItsStore_EndsWithNothingToRead.
A frame that does not decompress fails the connection (330a2cd), as on main. Decompression moved out of the handler into the consumer's connector, which is where the removed JetstreamClient did it. I chose this over skipping the frame:
- a dictionary mismatch breaks every frame, and skipping would silently drop the whole stream, while failing the connection surfaces it when the policy gives up;
- a one-off corrupt frame is fetched again on reconnect from the last cursor rather than lost.
Test: ConsumeAsync_FrameThatDoesNotDecompress_FailsTheConnectionAndReconnects.
Commit blocks check their links and refuse trailing bytes (7fa09b5). This flaw is also on main, so it gets a Security CHANGELOG bullet.
- data, and prev when it is not null, must be valid CIDs. Before, a commit signed with the repository's own key and carrying a malformed data made Cid.FromBytes throw out of TypedFirehoseConsumer.
- Bytes after the commit map are refused, because they would change the block's CID without touching its signature.
Tests: three new malformed rows in CommitBlockTests, a null prev accepted, a valid prev kept in the signed bytes, and VerifySyncAsync_SignedCommitWhoseDataIsNotACid_IsInvalidRatherThanThrowing.
Jetstream cursor coverage restored (d6cadc5):
- v1: stored cursor, explicit cursor over the store, cancellation persisting the final cursor. The live start is already asserted in YieldsEventsFromSource.
- v2: stored sequence number (replayed and dropped), and progress persisted before a refused reconnect throws.
Single-connection wording (676810c). With MaxAttempts = 0 the end of the connection throws; the CHANGELOG bullet, jetstream.md and StreamReconnectPolicy.MaxAttempts now say so.
DagCborDecoder.IsMalformed no longer matches ArgumentNullException (9fb842e), with a comment on why ArgumentException counts as malformed input in those parse-only blocks. Test: IsMalformed_CountsWhatTheBytesCauseAndNotABug.

After the fixes, the full solution builds with 0 warnings. Unit tests: 5039 passed, 0 failed, 5 skipped, run twice. The benchmark paths are unaffected: fix 2 adds an iterator layer only when a Decompressor is set.

Noticed, not changed: RepoSnapshot.Records calls Cid.FromBytes on MST entry values, which the tree reader does not validate. A snapshot whose tree holds a value that is not a CID therefore throws ArgumentException while the caller enumerates Records. main behaves the same way (Cid.Parse(EncodeCidToString(...))), and the consumer never enumerates it, so this is outside Pass A. The fix is to check entry values in MstNodeData.

Deferred or flagged

Outside this pass's files: one cref in src/ATProtoNet/Lexicon/Com/AtProto/Label/LabelModels.cs (pass C's area) named FirehoseClient.SubscribeLabelsAsync. It now names LabelStreamConsumer.
SpaceRecordCbor's catch list (pass B's area) can use DagCborDecoder.IsMalformed too. I did not touch it.
Left for pass D: RepoDownload.CreateClient and the fetchers' HTTP clients (N1), and the verify-with-refresh helper (V1), which RepoSyncVerifier still copies.
Left for pass F: the lean-doc sweep of internal types. I kept new internal docs short, but did not convert the existing /// docs on internal types.
Unifying the two MST implementations (C-Q7) is deferred by decision.

🤖 Generated with Claude Code

Part of #180 (pass A: streaming, repo and core wire primitives). Fewer lines and one way to do each thing in the streaming, repository and wire code, with the same behaviour except where an owner decision removes something. The existing tests are the safety net. Every regression, security and interop test still passes, retargeted where its API moved. ## What changed ### Streaming (S1, O1, owner decisions) - **One stream loop.** `EventStreamLoop` no longer knows about frames. A handler reads raw socket messages, and `CborEventStreamHandler<T>` reads the `{op, t}` DAG-CBOR header for the XRPC streams (firehose, labels, chat moderation). `JetstreamConsumer` (its own copied reconnect loop) and `TapChannel.ReadAllAsync` (another copy) now run on it. `TapChannel` keeps its acknowledgements by adapting its duplex connector to the loop's connector seam. - **One cursor preamble.** `CursorTracker.StartAsync` replaces the four copies of load, log, `Start` and flush in the firehose, label, Jetstream and replay consumers. The tracker is `IAsyncDisposable`, so `await using` replaces the `finally { FlushAsync }` blocks. - **One XRPC stream endpoint.** `CborEventStreamHandler.Endpoint(serviceUrl, nsid, cursor)` builds the endpoint for the firehose, label and chat moderation streams. - **`FirehoseClient` and `JetstreamClient` are removed.** The consumers cover them: `Reconnect.MaxAttempts = 0` reads a single connection, and no `CursorStore` means no persistence. - **`FirehoseVerifier` is removed**, with `VerificationResult` and `TypedFirehoseConsumerOptions.Verifier`. `VerifyCids` keeps the local CID check, now inside the consumer. `SyncVerifier` is the signature path. - **Options split (O1).** `StreamConsumerOptions` keeps `ServiceUrl`, `Reconnect`, `Logger`, `OnStreamError` and `OnEventDropped`. The new `CursorStreamConsumerOptions` adds `CursorStore`, `StreamId` and `CursorPersistInterval`. `ChatModerationEventConsumerOptions` and `TapClientOptions` derive from the base: their own copies of the shared settings go, `TapClientOptions.ServiceUrl` becomes a string like every other stream's, and Tap's `OnError` becomes the shared `OnEventDropped`. - **The stream contract is documented once**, on `StreamConsumerOptions`: at-least-once delivery, the cursor interval, cancellation and error frames. The six consumer classes, `StreamReconnectPolicy`, `IStreamCursorStore` and `EventStreamException` point to it with `<see cref>`. `docs/firehose.md` has one "Delivery, cursors and errors" section, which `jetstream.md` and `tap.md` link to. ### PLC export stream (P1) - `PlcClient.StreamExportAsync` runs on `StreamSocket`. To support that, `StreamSocketOptions` gains an `HttpMessageInvoker` (so the identity address policy still applies) and a per-connection message limit. `StreamSocket` also sends the SDK's `User-Agent`, which the PLC client already did. - A close with a reason, which is how the PLC directory reports `OutdatedCursor`, is now an `EventStreamException` whose `Error` is the reason. `PlcExportStreamException` is removed, and the test seam is a `StreamConnector`. ### Repository and wire primitives (R1, M1, C1, D1, W1) - **R1.** `CommitBlock.Read` splices `sig` out in the same strict pass that reads the fields, refuses anything but version 3, and keeps the duplicate-key refusal of the old strict splice. - `CommitBlock.FromCar(car, out reader, expectedRoot)` is the one "parse the CAR, find the root commit" preamble for `RepoSyncVerifier`, `RepoSnapshot` and `RecordProof`. - `FirehoseVerifier.ExtractSignedView` / `MapHeaderLength` / `WriteMapHeader` and `RecordProof.ReadCommit` are gone. A 20-line `CommitBlock.WriteMapHeader` keeps the pinned boundary vectors. - **M1.** - `MstNodeData.ReadKeys(lower, upper)` rebuilds a node's keys and checks them (valid, strictly increasing, inside the parent's range) for `MerkleSearchTree`, `PartialMerkleSearchTree` and `RecordProof`. `MerkleSearchTree.Deserialize` checks ranges now, not an in-order last key. That is equivalent: every subtree is non-empty, so a subtree linked twice still fails. - `MstNodeData.WriteStart`, `WriteEntry` and `WriteEnd` are the one encoding of a node. `ToBytes` and the partial tree's `Encode` both use them, and the partial tree keeps its allocation-free encode. I wrote three steps rather than an instance `Write(CborWriter)` for that reason. - One generic `IsValidKey<T>` serves strings and node bytes. - `DagCborDecoder.IsMalformed` replaces six of the seven copied `CborContentException` catch lists. The seventh is in `Spaces/`: see Deferred. - **C1.** `Base32Lower.TryDecode` is the one canonical base32 decoder. `Cid` and `CidComputation.DecodeCidString` both use it, so the latter now refuses non-canonical text too. The internal `Cid.FromBytes` replaces nine `Cid.Parse(EncodeCidToString(x))` round trips, and `DagCborEncoder` decodes `$bytes` through `LexBase64`. - **D1.** `DagCborDecoder.TryValidate` is removed. `Decode` already covers the 100k-nesting, float and malformed-input cases. - **W1.** `AtProtoHttp.WithScheme(uri, webSocket)` replaces the five ws↔http converters: the Jetstream endpoint, the archive client, the resync upstream, the Tap channel and the PLC stream. ### Options (C-Q4, C-Q5) - **`RepoResyncOptions`** keeps only `Fetchers`. - `Upstream` and `AllowPrivateNetworks` go: pass `IRepoFetcher`s. - `FetchTimeout` goes. The fetchers bound their own downloads, and the coordinator keeps the same five-minute per-attempt cap internally. - The never-set limits become internal defaults with the same values, still settable by tests. - The default fetchers own their clients, so the coordinator no longer creates one and disposes it. - **`JetstreamReplayConsumer`** has one way to configure it. The `(options, archiveClient)` constructor goes, so the archive client is always built from `JetstreamArchiveOptions`. `JetstreamArchiveOptions.AfterSeq` also goes, because `ReplayAsync(afterSeq)` already takes the resume position. The `AfterSeq < BeforeSeq` check now runs in `ReplayAsync` and covers a stored cursor too. ### Tests (T2, T4 firehose part, loop collapse) - `EventStreamLoopTests` holds the loop contract, run through the firehose consumer: - a theory that the cursor is saved on break, cancellation and a thrown failure; - attempts exhausted, with and without a last failure; - the attempt count resets after a frame; - a failed connection is retried; - non-retryable failures (refused upgrade, `FutureCursor`) throw at once; - a retryable error frame is reported and reconnects. The duplicates in the Jetstream (v1/v2), Tap and firehose files go. The consumer-specific ones stay: cursor semantics, the Jetstream exception type, info and error frames, Tap acknowledgements, the chat token. - **T2.** The two Jetstream URL builder files merge into `JetstreamEndpointTests`: protocol theories for paths, filters and compression, and one validation theory that now also pins each message. - **T4.** `FirehoseVerifierTests` becomes `CommitBlockTests`: the splice, the header-boundary vectors, and a malformed-commit theory that adds version 2 and duplicate keys. The null, empty and malformed blocks cases become a `VerifyCids` consumer theory. The `ExtractSignedView` interop checks in `RepoCommitTests`, `RepoWritePathTests` and `RecordProofTests` now use `CommitBlock.Read`. - `FirehoseVerifierIdentityTests` goes with the verifier. `RepoSyncVerifierTests` already pins the same refetch-once and invalidate rules: refreshes, rotated keys and invalidations. - The Jetstream consumer tests now script wire frames through a shared `ScriptedJetstream` and `JetstreamFrames`, so the parser path is covered too. - The PLC stream tests run on a loopback Kestrel server through the real `StreamSocket`. ## Breaking changes (each has a CHANGELOG bullet and a migration-guide entry) | Removed or changed | Migration | |---|---| | `FirehoseClient` (with `SubscribeLabelsAsync`), `JetstreamClient` | `TypedFirehoseConsumer` / `LabelStreamConsumer` / `JetstreamConsumer` `.ConsumeAsync(cursor)`; for a single connection, set `Reconnect = new StreamReconnectPolicy { MaxAttempts = 0 }` | | `FirehoseVerifier`, `VerificationResult`, `TypedFirehoseConsumerOptions.Verifier` | `SyncVerifier = new RepoSyncVerifier()` (in-memory state by default), plus `Resync`; `VerifyCids` for CIDs alone | | `DagCborDecoder.TryValidate` | `DagCborDecoder.Decode` and catch `FormatException`; key order is no longer checked | | `JetstreamReplayConsumer(options, archiveClient)`, `JetstreamArchiveOptions.AfterSeq` | configure through `JetstreamArchiveOptions`; `ReplayAsync(afterSeq)` | | `StreamConsumerOptions.CursorStore` / `StreamId` / `CursorPersistInterval` moved to `CursorStreamConsumerOptions` | recompile (source compatible) | These are new in 0.7 and never shipped, so their Unreleased *Added* bullets are amended instead of getting breaking bullets: - `PlcExportStreamException`: `StreamExportAsync` now throws `EventStreamException` with `Error` set to the close reason, and a connect failure is an `EventStreamException` instead of `DidResolutionException`. - `TapClientOptions.OnError` / `Uri ServiceUrl`. - The chat moderation options' own copies of the shared settings. - The `RepoResyncOptions` knobs. Existing Unreleased bullets and docs that named the removed types were brought up to date. `PublicAPI.Shipped.txt`: 67 lines removed, and the eight `CursorStreamConsumerOptions` lines added. `Unshipped.txt` is empty. ## Behaviour changes worth knowing - A server that closes a stream abnormally with a reason now fails that connection with an `EventStreamException` whose `Error` is the reason. Before, it was a silent close. A retryable reason still reconnects; a `FutureCursor` close is no longer retried forever. - On a consumer, a frame that is skipped (an info frame or an unreadable one) resets the reconnect count, as it already did on the firehose loop. Before, only a delivered event reset it on the Jetstream consumer. - `CidComputation.DecodeCidString` and `TryDecodeCidString` refuse upper case, padding and non-canonical trailing bits. These have a *Changed* bullet. ## Verification - `dotnet build`: 0 warnings, 0 errors (full solution, including doc snippets, samples and integration tests). - Unit tests (before the review fixes): 5027 passed, 0 failed, 5 skipped, run twice. The baseline on `origin/main` was 5059 / 0 / 5; the difference is the collapsed duplicates listed above. - Performance: a Stopwatch harness, Release build, a scripted connector with no I/O, and interleaved A/B runs of `origin/main` against this branch. The machine is shared: 12 threads at a load average of about 6 from other agents, so round-to-round noise is ±5%. The table shows the best of three runs of 31 rounds each. Allocations are per run. | Path | origin/main | this branch | |---|---|---| | Firehose, 100k `#commit` frames, parse | 215.9 ms, 419.1 MiB | 219.9 ms, 419.1 MiB | | Firehose, 100k frames, collection filter miss (CBOR scan) | 57.3 ms, 88.5 MiB | 58.1 ms, 88.5 MiB | | Firehose Sync 1.1, 2k verified commits (signature, inversion) | 503.5 ms, 88.9 MiB | 497.8 ms, 87.8 MiB | | Jetstream v2 live, 100k JSON frames | 159.3 ms, 152.6 MiB | 163.0 ms, 152.6 MiB | Allocations are unchanged on every path, and the Sync 1.1 path is about 1% faster now that the commit block is read in one pass. The loop paths measure 1–2% slower, inside the noise. That fits the one or two extra virtual calls per frame that sharing the loop costs. The Jetstream archive decode (`JetstreamSegmentReader`) is untouched. ## Line counts (`loc.py origin/main HEAD`) | Area | code | xmldoc | comments | |---|---|---|---| | `src/ATProtoNet/Streaming` | 4563 → 4008 | 2041 → 1834 | 181 → 134 | | `src/ATProtoNet/Repo` | 2363 → 2211 | 743 → 770 | 129 → 121 | | `src/ATProtoNet/Identity` | 2564 → 2460 | 1350 → 1336 | 101 → 100 | | `src/ATProtoNet/Tap` | 458 → 405 | 187 → 192 | 3 → 2 | | `src/ATProtoNet/Http` | 1175 → 1192 | 728 → 732 | 40 → 40 | | **src total** | **−847** | **−186** | **−57** | | `tests/ATProtoNet.Tests` | 49571 → 48825 (−746) | 1470 → 1474 | 1847 → 1812 | | `docs` | 12818 → 12736 (−82) | | | | `PublicAPI.*.txt` | 9983 → 9916 (−67) | | | ## Review fixes Six new commits on top of `967439a`. Each code fix has a regression test that fails without it; I checked by running the test against the committed source. 1. **A finished bounded replay can run again** (`848d897`). `BeforeSeq` is inclusive, so a completed snapshot leaves its store at exactly `BeforeSeq`, and a second run threw. A start at or above `BeforeSeq`, explicit or stored, now ends the replay with nothing to read, which is `main`'s result. It also no longer spends a metered plan request on an empty window. Test: `BoundedSnapshot_RunAgainWithItsStore_EndsWithNothingToRead`. 2. **A frame that does not decompress fails the connection** (`330a2cd`), as on `main`. Decompression moved out of the handler into the consumer's connector, which is where the removed `JetstreamClient` did it. I chose this over skipping the frame: - a dictionary mismatch breaks every frame, and skipping would silently drop the whole stream, while failing the connection surfaces it when the policy gives up; - a one-off corrupt frame is fetched again on reconnect from the last cursor rather than lost. Test: `ConsumeAsync_FrameThatDoesNotDecompress_FailsTheConnectionAndReconnects`. 3. **Commit blocks check their links and refuse trailing bytes** (`7fa09b5`). This flaw is also on `main`, so it gets a *Security* CHANGELOG bullet. - `data`, and `prev` when it is not null, must be valid CIDs. Before, a commit signed with the repository's own key and carrying a malformed `data` made `Cid.FromBytes` throw out of `TypedFirehoseConsumer`. - Bytes after the commit map are refused, because they would change the block's CID without touching its signature. Tests: three new malformed rows in `CommitBlockTests`, a null `prev` accepted, a valid `prev` kept in the signed bytes, and `VerifySyncAsync_SignedCommitWhoseDataIsNotACid_IsInvalidRatherThanThrowing`. 4. **Jetstream cursor coverage restored** (`d6cadc5`): - v1: stored cursor, explicit cursor over the store, cancellation persisting the final cursor. The live start is already asserted in `YieldsEventsFromSource`. - v2: stored sequence number (replayed and dropped), and progress persisted before a refused reconnect throws. 5. **Single-connection wording** (`676810c`). With `MaxAttempts = 0` the end of the connection throws; the CHANGELOG bullet, `jetstream.md` and `StreamReconnectPolicy.MaxAttempts` now say so. 6. **`DagCborDecoder.IsMalformed` no longer matches `ArgumentNullException`** (`9fb842e`), with a comment on why `ArgumentException` counts as malformed input in those parse-only blocks. Test: `IsMalformed_CountsWhatTheBytesCauseAndNotABug`. After the fixes, the full solution builds with 0 warnings. Unit tests: 5039 passed, 0 failed, 5 skipped, run twice. The benchmark paths are unaffected: fix 2 adds an iterator layer only when a `Decompressor` is set. **Noticed, not changed:** `RepoSnapshot.Records` calls `Cid.FromBytes` on MST entry values, which the tree reader does not validate. A snapshot whose tree holds a value that is not a CID therefore throws `ArgumentException` while the caller enumerates `Records`. `main` behaves the same way (`Cid.Parse(EncodeCidToString(...))`), and the consumer never enumerates it, so this is outside Pass A. The fix is to check entry values in `MstNodeData`. ## Deferred or flagged - **Outside this pass's files:** one cref in `src/ATProtoNet/Lexicon/Com/AtProto/Label/LabelModels.cs` (pass C's area) named `FirehoseClient.SubscribeLabelsAsync`. It now names `LabelStreamConsumer`. - **`SpaceRecordCbor`'s catch list** (pass B's area) can use `DagCborDecoder.IsMalformed` too. I did not touch it. - **Left for pass D:** `RepoDownload.CreateClient` and the fetchers' HTTP clients (N1), and the verify-with-refresh helper (V1), which `RepoSyncVerifier` still copies. - **Left for pass F:** the lean-doc sweep of internal types. I kept new internal docs short, but did not convert the existing `///` docs on internal types. - **Unifying the two MST implementations** (C-Q7) is deferred by decision. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 5 commits

2026-09-27 12:58:43 +00:00

refactor(repo): one base32 decoder, Cid.FromBytes, shared CBOR error filter (#180 ) 3908c0dc44

Core wire primitives for the pre-release debloat pass A:

- Base32Lower.TryDecode is the one canonical base32 decoder; Cid and
  CidComputation.DecodeCidString both use it.
- Cid.FromBytes replaces the Cid.Parse(EncodeCidToString(x)) round trips.
- DagCborEncoder decodes $bytes through LexBase64.
- DagCborDecoder.IsMalformed is the one exception filter for malformed CBOR.
- AtProtoHttp.WithScheme switches a URL between http(s) and ws(s); the
  archive client and the resync upstream use it.
- DagCborDecoder.TryValidate is removed (only tests called it).

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

refactor(repo): read commit blocks in one pass, share the MST key reader, drop FirehoseVerifier (#180 ) 4d6a6b7757

- CommitBlock.Read splices out `sig` in the same strict pass that reads the
  fields, and refuses anything but version 3; CommitBlock.FromCar is the one
  "parse the CAR, find the root commit" preamble for RepoSyncVerifier,
  RepoSnapshot and RecordProof.
- MstNodeData.ReadKeys rebuilds and checks a node's keys (valid, strictly
  increasing within the parent's range) for MerkleSearchTree, the partial tree
  and record proofs; MerkleSearchTree.Deserialize now checks ranges instead
  of an in-order last key. MstNodeData's writer steps also encode the partial
  tree's nodes. One generic IsValidKey serves strings and node bytes.
- FirehoseVerifier, VerificationResult and TypedFirehoseConsumerOptions.Verifier
  are removed (owner decision): RepoSyncVerifier covers signatures; VerifyCids
  keeps the local CID check inside the consumer.

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

refactor(streaming)!: one stream loop, fold the single-connection clients into the consumers (#180 ) c636459ef6

- EventStreamLoop is frame-agnostic: a handler reads raw socket messages, and
  a CBOR handler base reads the {op, t} header for the XRPC streams. The
  Jetstream consumer and the Tap channel now run on it instead of their own
  reconnect loops, and CursorTracker.StartAsync replaces four copies of the
  load/start/flush preamble.
- FirehoseClient and JetstreamClient are removed (owner decision): the
  consumers with Reconnect.MaxAttempts = 0 read a single connection.
- StreamConsumerOptions keeps ServiceUrl, Reconnect, Logger and the two hooks;
  CursorStreamConsumerOptions adds the cursor settings. The chat moderation
  and Tap options derive from the base (Tap's OnError is OnEventDropped).
  The stream contract is documented once, on StreamConsumerOptions.
- PlcClient.StreamExportAsync runs on StreamSocket, which gains an invoker,
  a per-connection message limit, the SDK User-Agent and close reasons;
  PlcExportStreamException gives way to EventStreamException.
- AtProtoHttp.WithScheme serves the Jetstream, Tap and PLC endpoints too.
- RepoResyncOptions keeps Fetchers; its limits are internal, and Upstream,
  AllowPrivateNetworks and FetchTimeout go (the fetchers own their clients).
- JetstreamReplayConsumer is configured by its options alone: the
  (options, archiveClient) constructor and JetstreamArchiveOptions.AfterSeq go.
- Tests: generic reconnect tests collapse into EventStreamLoopTests; the two
  Jetstream URL builder files merge into protocol theories.

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

docs(streaming): the reconnect policy and cursor store point at the one stream contract (#180 ) dc32d63696

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

docs(repo): shorter CommitBlock.FromCar summary (#180 )

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

Details

CI / build-and-test (pull_request) Successful in 1m3s

Details

967439a23a

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

opencode-bot commented

2026-09-27 12:58:45 +00:00

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

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

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

opencode-bot commented

2026-09-27 12:58:47 +00:00

⏳ 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-27T17:58:44.877Z) — 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-27T17:58:44.877Z) — nothing to re-trigger.

Grandiras commented

2026-09-27 12:58:53 +00:00

@claude /review — debloat pass A of #180 (streaming, repo, core wire primitives). The diff will be truncated; please read in full from the branch:

Streaming/EventStreamLoop.cs plus the new CBOR-header base handler, and the Jetstream consumer and TapChannel ported onto the loop;
CursorTracker.StartAsync;
the StreamConsumerOptions/CursorStreamConsumerOptions split;
Repo/CommitBlock.cs, which now splices out sig in one strict pass. This is the signed-bytes input for commit signature checks, so it must be byte-exact against the pinned vectors.
CommitBlock.FromCar, MstNodeData.ReadKeys and the writer steps;
Cid.FromBytes and the base32 decoder;
PlcClient on StreamSocket.

What to look for:

Behaviour. Anything a folded client, dropped verifier or collapsed test used to guarantee that is no longer guaranteed: reconnect, backoff, cursor persistence, at-least-once delivery, error frames, cancellation.
Commit parsing. Any laxer parsing of untrusted commit, MST or CAR input.
Behaviour changes listed in the description. A FutureCursor close is no longer retried, a PLC connection failure now throws EventStreamException, and DecodeCidString is now strict. Are these acceptable?

@claude /review — debloat pass A of #180 (streaming, repo, core wire primitives). The diff will be truncated; please read in full from the branch: - `Streaming/EventStreamLoop.cs` plus the new CBOR-header base handler, and the Jetstream consumer and `TapChannel` ported onto the loop; - `CursorTracker.StartAsync`; - the `StreamConsumerOptions`/`CursorStreamConsumerOptions` split; - `Repo/CommitBlock.cs`, which now splices out `sig` in one strict pass. This is the signed-bytes input for commit signature checks, so it must be byte-exact against the pinned vectors. - `CommitBlock.FromCar`, `MstNodeData.ReadKeys` and the writer steps; - `Cid.FromBytes` and the base32 decoder; - `PlcClient` on `StreamSocket`. What to look for: - **Behaviour.** Anything a folded client, dropped verifier or collapsed test used to guarantee that is no longer guaranteed: reconnect, backoff, cursor persistence, at-least-once delivery, error frames, cancellation. - **Commit parsing.** Any laxer parsing of untrusted commit, MST or CAR input. - **Behaviour changes listed in the description.** A `FutureCursor` close is no longer retried, a PLC connection failure now throws `EventStreamException`, and `DecodeCidString` is now strict. Are these acceptable?

claude-bot commented

2026-09-27 12:58:57 +00:00

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

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

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

claude-reviewer reviewed

2026-09-27 13:01:35 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a very large, well-documented mechanical refactor (streaming loop unification, CommitBlock/base32/DagCbor primitive consolidation), and the diff supplied is truncated partway through the repo/wire section, so this review only covers what's visible: Http/AtProtoHttp, Identity/Cid, Identity/PlcClient, Repo/CarReader, Repo/CidComputation, Repo/CommitBlock, Repo/DagCborDecoder, Repo/DagCborEncoder, Repo/MerkleSearchTree (partial), the docs and CHANGELOG/PublicAPI files, and the LabelModels cref fix.

Within that scope the change is careful and internally consistent with its own description:

AtProtoHttp.WithScheme correctly folds five separate ws/http converters into one, preserving scheme/port semantics.
Base32Lower.TryDecode's new bits < 5 check is a genuine correctness improvement over the code it replaces (the removed inline decoder in Cid.cs relied on the caller having already fixed the string length; the shared decoder now independently rejects non-minimal-length/non-canonical base32, which is required since it's now used more generally by CidComputation.DecodeCidString).
I verified via a web search that .NET's CborReader in Strict conformance mode does enforce byte-level map-key uniqueness (confirmed by a related dotnet/aspnetcore issue about semantically duplicate keys with different encodings slipping through bytewise checks), so CommitBlock.Read's claim of "keeps the duplicate-key refusal of the old strict splice" holds for the common case, with the same known edge-case caveat (differently-encoded-but-equal keys) that presumably existed before too.
CommitBlock.Read's manual re-splice of the map header (WriteMapHeader) plus the byte ranges around sig looks correct: it copies bytes before sig's key and everything after sig's value, and rewrites only the map header for entries - 1, preserving byte-for-byte signed content.
PlcClient.StreamExportAsync's move to the shared StreamSocket/Connector loop reads correctly, matches the documented breaking behaviour (a close-with-reason and even a connect failure now surface as EventStreamException instead of PlcExportStreamException/DidResolutionException), and CHANGELOG/migration-guide/docs entries for that are present and consistent.

One mild concern, not blocking: DagCborDecoder.IsMalformed now also treats bare ArgumentException as "malformed input" (see below), which is used at several call sites (CarReader, CommitBlock, DagCborDecoder.Decode itself) to catch-and-rewrap as FormatException. ArgumentOutOfRangeException/ArgumentNullException are both subtypes of ArgumentException, so this could occasionally swallow a genuine internal bug (e.g. a bad slice length from a real coding error) and report it to callers as bad/malformed network or CAR data instead of surfacing as an unhandled exception during development/testing. Given the surrounding readers are all operating on attacker-controlled bytes this is probably intentional and low-risk, but worth a second look since it's a behavior-widening change stacked onto several call sites at once.

I could not review the remaining ~60% of the diff (MstNodeData.ReadKeys internals, RepoSyncVerifier, JetstreamReplayConsumer/JetstreamArchiveOptions, RepoResyncOptions defaults, the EventStreamLoop/CborEventStreamHandler consolidation, and the test changes) because the diff was cut off; nothing in the visible portion points to a problem there, but that part of the PR is unverified by this pass.

⏱ 2m 23s · 4 turns · tokens: 94.5k in / 12.2k out (+353.6k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a very large, well-documented mechanical refactor (streaming loop unification, CommitBlock/base32/DagCbor primitive consolidation), and the diff supplied is truncated partway through the repo/wire section, so this review only covers what's visible: Http/AtProtoHttp, Identity/Cid, Identity/PlcClient, Repo/CarReader, Repo/CidComputation, Repo/CommitBlock, Repo/DagCborDecoder, Repo/DagCborEncoder, Repo/MerkleSearchTree (partial), the docs and CHANGELOG/PublicAPI files, and the LabelModels cref fix. Within that scope the change is careful and internally consistent with its own description: - `AtProtoHttp.WithScheme` correctly folds five separate ws/http converters into one, preserving scheme/port semantics. - `Base32Lower.TryDecode`'s new `bits < 5` check is a genuine correctness improvement over the code it replaces (the removed inline decoder in `Cid.cs` relied on the caller having already fixed the string length; the shared decoder now independently rejects non-minimal-length/non-canonical base32, which is required since it's now used more generally by `CidComputation.DecodeCidString`). - I verified via a web search that .NET's `CborReader` in `Strict` conformance mode does enforce byte-level map-key uniqueness (confirmed by a related dotnet/aspnetcore issue about *semantically* duplicate keys with different encodings slipping through bytewise checks), so `CommitBlock.Read`'s claim of "keeps the duplicate-key refusal of the old strict splice" holds for the common case, with the same known edge-case caveat (differently-encoded-but-equal keys) that presumably existed before too. - `CommitBlock.Read`'s manual re-splice of the map header (`WriteMapHeader`) plus the byte ranges around `sig` looks correct: it copies bytes before `sig`'s key and everything after `sig`'s value, and rewrites only the map header for `entries - 1`, preserving byte-for-byte signed content. - `PlcClient.StreamExportAsync`'s move to the shared `StreamSocket`/`Connector` loop reads correctly, matches the documented breaking behaviour (a close-with-reason and even a connect failure now surface as `EventStreamException` instead of `PlcExportStreamException`/`DidResolutionException`), and CHANGELOG/migration-guide/docs entries for that are present and consistent. One mild concern, not blocking: `DagCborDecoder.IsMalformed` now also treats bare `ArgumentException` as "malformed input" (see below), which is used at several call sites (`CarReader`, `CommitBlock`, `DagCborDecoder.Decode` itself) to catch-and-rewrap as `FormatException`. `ArgumentOutOfRangeException`/`ArgumentNullException` are both subtypes of `ArgumentException`, so this could occasionally swallow a genuine internal bug (e.g. a bad slice length from a real coding error) and report it to callers as bad/malformed network or CAR data instead of surfacing as an unhandled exception during development/testing. Given the surrounding readers are all operating on attacker-controlled bytes this is probably intentional and low-risk, but worth a second look since it's a behavior-widening change stacked onto several call sites at once. I could not review the remaining ~60% of the diff (MstNodeData.ReadKeys internals, RepoSyncVerifier, JetstreamReplayConsumer/JetstreamArchiveOptions, RepoResyncOptions defaults, the EventStreamLoop/CborEventStreamHandler consolidation, and the test changes) because the diff was cut off; nothing in the visible portion points to a problem there, but that part of the PR is unverified by this pass. --- ⏱ 2m 23s · 4 turns · tokens: 94.5k in / 12.2k out (+353.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-182/run-634.md)

src/ATProtoNet/Repo/DagCborDecoder.cs Outdated

					
				@ -159,0 +45,4 @@

				    /// What <see cref="CborReader"/>, and the readers built on it, throw for input that is not

				    /// well-formed or not the shape being read: never a bug in the caller.

				    /// </summary>

				    internal static bool IsMalformed(Exception ex) =>

IsMalformed now also matches plain ArgumentException (and therefore ArgumentOutOfRangeException/ArgumentNullException, both subtypes). It's used at multiple call sites (CarReader.cs, CommitBlock.Read, this file's own Decode) to catch-and-rewrap failures as FormatException for "bad input from the network/CAR". If any of the underlying CborReader/span code ever throws an ArgumentException due to an actual internal bug (not malformed input), it will now be silently reported to the caller as malformed data instead of surfacing as a crash/test failure. Given this only wraps parsers over untrusted bytes it's likely intentional, but worth double-checking that no legitimate caller-side argument validation exceptions can reach these catch sites.

`IsMalformed` now also matches plain `ArgumentException` (and therefore `ArgumentOutOfRangeException`/`ArgumentNullException`, both subtypes). It's used at multiple call sites (`CarReader.cs`, `CommitBlock.Read`, this file's own `Decode`) to catch-and-rewrap failures as `FormatException` for "bad input from the network/CAR". If any of the underlying `CborReader`/span code ever throws an `ArgumentException` due to an actual internal bug (not malformed input), it will now be silently reported to the caller as malformed data instead of surfacing as a crash/test failure. Given this only wraps parsers over untrusted bytes it's likely intentional, but worth double-checking that no legitimate caller-side argument validation exceptions can reach these catch sites.

opencode-bot commented

2026-09-27 15:57:18 +00:00

🤖 Resolving the merge conflicts — the reply will link the full session transcript.

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

🤖 Resolving the merge conflicts — the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-182/run-2026-09-27T15-57-17-271Z.md) — updating every ~5 minutes while the fix runs.

opencode-bot commented

2026-09-27 15:57:20 +00:00

⏳ 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-27T20:54:08.259Z) — 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-27T20:54:08.259Z) — nothing to re-trigger.

Grandiras force-pushed debloat-a-streaming from 967439a23a

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

Details

CI / build-and-test (pull_request) Successful in 1m3s

Details

to ddd8c59551

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

Details

CI / build-and-test (pull_request) Successful in 1m34s

Details

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

Details

2026-09-27 16:17:14 +00:00

Compare

Grandiras commented

2026-09-27 16:17:14 +00:00

@claude /review, please re-review. I rebased onto main after #183 merged. The conflicts were only in PublicAPI.Shipped.txt; I resolved them by applying each commit's own line delta onto main's baseline. Six new commits (after the rebase, the last six on the branch) fix an independent adversarial review and your earlier note. See "Review fixes" in the description:

a bounded replay started at or beyond BeforeSeq ends empty, as on main;
a frame that won't decompress fails the connection, as on main;
CommitBlock.Read validates the data and prev CIDs and refuses bytes after the commit map (Security bullet: main has the same flaw);
the Jetstream cursor-resume and throw-persists tests are restored;
the CHANGELOG line for MaxAttempts = 0 is fixed;
IsMalformed no longer treats ArgumentNullException as malformed, with a comment explaining why ArgumentException counts there.
The adversarial review also compared CommitBlock.Read against the old path on 200,000 mutated blocks with zero mismatches, so it didn't need to change. Please read the fix commits in full.

@claude /review, please re-review. I rebased onto main after #183 merged. The conflicts were only in `PublicAPI.Shipped.txt`; I resolved them by applying each commit's own line delta onto main's baseline. Six new commits (after the rebase, the last six on the branch) fix an independent adversarial review and your earlier note. See "Review fixes" in the description: - a bounded replay started at or beyond `BeforeSeq` ends empty, as on main; - a frame that won't decompress fails the connection, as on main; - `CommitBlock.Read` validates the `data` and `prev` CIDs and refuses bytes after the commit map (Security bullet: main has the same flaw); - the Jetstream cursor-resume and throw-persists tests are restored; - the CHANGELOG line for `MaxAttempts = 0` is fixed; - `IsMalformed` no longer treats `ArgumentNullException` as malformed, with a comment explaining why `ArgumentException` counts there. The adversarial review also compared `CommitBlock.Read` against the old path on 200,000 mutated blocks with zero mismatches, so it didn't need to change. Please read the fix commits in full.

claude-bot commented

2026-09-27 16:17:16 +00:00

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

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

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

opencode-bot commented

2026-09-27 16:18:16 +00:00

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

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

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

opencode-bot commented

2026-09-27 16:18:20 +00:00

⏳ 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-27T21:18:16.208Z) — 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-27T21:18:16.208Z) — nothing to re-trigger.

claude-reviewer approved these changes

2026-09-27 16:19:45 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This is a re-review after six "review fixes" commits landed on top of the version I last looked at. I verified each fix against the actual PR branch (origin/debloat-a-streaming) rather than relying on the (again partially truncated) diff text:

Bounded replay re-run (de3e8a4) — the start >= BeforeSeq check moved from an eager throw to a yield break inside the await using tracker block, so a finished snapshot's store position now ends the replay quietly instead of throwing, and the tracker still gets disposed correctly on that path. Matches main's behavior as claimed.
Jetstream decompression failure (966d61f) — decompression moved from the read-side handler into a connector-wrapping Decompress iterator, so an InvalidDataException now surfaces through connection.MoveNextAsync() inside EventStreamLoop.RunAsync, which is caught by the loop's generic catch (Exception ex) { failure = handler.Failed(ex); break; } and correctly triggers the normal backoff/reconnect path rather than being silently skipped as one bad frame.
Commit link/trailing-bytes validation (af2fe1f) — CommitBlock.Read now validates data and (when present) prev are valid CIDs via Cid.IsValid, and refuses bytes after the map via reader.ReadEndMap() + BytesRemaining > 0. RepoSyncVerifier.TryReadCommit only catches FormatException from CommitBlock.FromCar, which is exactly what these new checks throw, so the fix is correctly wired through the whole verification path (I checked the call sites at RepoSyncVerifier.cs:150/203/359).
ArgumentNullException no longer swallowed (ddd8c59) — this directly resolves the concern I flagged in my previous pass: IsMalformed now excludes ArgumentNullException specifically (ArgumentException and not ArgumentNullException), with a comment explaining the distinction and a new test (IsMalformed_CountsWhatTheBytesCauseAndNotABug) pinning both the inclusion (ArgumentException, ArgumentOutOfRangeException) and exclusion (ArgumentNullException, NullReferenceException) cases.
Jetstream cursor-coverage and single-connection wording fixes are documentation/test-only and read as described.

I also re-checked the shared EventStreamLoop/CborEventStreamHandler/EventStreamFrame plumbing (not covered in my last pass since the diff was truncated there too): the header/error/seq parsing and JSON transcoding are all guarded by the shared DagCborDecoder.IsMalformed filter consistently, and the loop's reconnect/backoff/error-frame handling is straightforward and matches the documented contract.

My previous pass was a non-blocking "comment" (the one concern raised is now fixed), and I found no new issues in this pass. Approving.

⏱ 2m 16s · 33 turns · tokens: 114.7k in / 9.6k out (+4002.4k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review after six "review fixes" commits landed on top of the version I last looked at. I verified each fix against the actual PR branch (`origin/debloat-a-streaming`) rather than relying on the (again partially truncated) diff text: 1. **Bounded replay re-run** (`de3e8a4`) — the `start >= BeforeSeq` check moved from an eager throw to a `yield break` inside the `await using` tracker block, so a finished snapshot's store position now ends the replay quietly instead of throwing, and the tracker still gets disposed correctly on that path. Matches `main`'s behavior as claimed. 2. **Jetstream decompression failure** (`966d61f`) — decompression moved from the read-side handler into a connector-wrapping `Decompress` iterator, so an `InvalidDataException` now surfaces through `connection.MoveNextAsync()` inside `EventStreamLoop.RunAsync`, which is caught by the loop's generic `catch (Exception ex) { failure = handler.Failed(ex); break; }` and correctly triggers the normal backoff/reconnect path rather than being silently skipped as one bad frame. 3. **Commit link/trailing-bytes validation** (`af2fe1f`) — `CommitBlock.Read` now validates `data` and (when present) `prev` are valid CIDs via `Cid.IsValid`, and refuses bytes after the map via `reader.ReadEndMap()` + `BytesRemaining > 0`. `RepoSyncVerifier.TryReadCommit` only catches `FormatException` from `CommitBlock.FromCar`, which is exactly what these new checks throw, so the fix is correctly wired through the whole verification path (I checked the call sites at `RepoSyncVerifier.cs:150/203/359`). 4. **`ArgumentNullException` no longer swallowed** (`ddd8c59`) — this directly resolves the concern I flagged in my previous pass: `IsMalformed` now excludes `ArgumentNullException` specifically (`ArgumentException and not ArgumentNullException`), with a comment explaining the distinction and a new test (`IsMalformed_CountsWhatTheBytesCauseAndNotABug`) pinning both the inclusion (`ArgumentException`, `ArgumentOutOfRangeException`) and exclusion (`ArgumentNullException`, `NullReferenceException`) cases. 5. Jetstream cursor-coverage and single-connection wording fixes are documentation/test-only and read as described. I also re-checked the shared `EventStreamLoop`/`CborEventStreamHandler`/`EventStreamFrame` plumbing (not covered in my last pass since the diff was truncated there too): the header/error/seq parsing and JSON transcoding are all guarded by the shared `DagCborDecoder.IsMalformed` filter consistently, and the loop's reconnect/backoff/error-frame handling is straightforward and matches the documented contract. My previous pass was a non-blocking "comment" (the one concern raised is now fixed), and I found no new issues in this pass. Approving. --- ⏱ 2m 16s · 33 turns · tokens: 114.7k in / 9.6k out (+4002.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-182/run-637.md)

Grandiras commented

2026-09-27 16:20:53 +00:00

Merge check against the updated main (#181 merged after this approval). Forgejo merges ddd8c59 cleanly, with docs/migrating-to-0.7.md, docs/tap.md and PublicAPI.Shipped.txt auto-merged. I tested the merge result locally in Release: 0 warnings (PublicApiAnalyzers included) and 5,000 unit tests passing. CHANGELOG has no duplicate bullets and no markers. Merging on the approval of this head.

Merge check against the updated main (#181 merged after this approval). Forgejo merges ddd8c59 cleanly, with `docs/migrating-to-0.7.md`, `docs/tap.md` and `PublicAPI.Shipped.txt` auto-merged. I tested the merge result locally in Release: 0 warnings (PublicApiAnalyzers included) and 5,000 unit tests passing. CHANGELOG has no duplicate bullets and no markers. Merging on the approval of this head.

Grandiras merged commit da42974856 into main

2026-09-27 16:20:54 +00:00

Grandiras deleted branch debloat-a-streaming

2026-09-27 16:20:55 +00:00

Grandiras referenced this pull request from a commit

2026-09-27 16:20:56 +00:00

refactor!: debloat streaming, repo and core wire primitives (#182)

Grandiras referenced this pull request

2026-09-27 16:20:57 +00:00

refactor!: pre-release debloat pass #180

Grandiras referenced this pull request

2026-09-27 21:27:32 +00:00