feat!: typed identifiers, AtDatetime and one paginator for com.atproto (1/4 of #119) #154

Grandiras · 2026-09-24T20:08:39Z

Grandiras commented

2026-09-24 20:08:39 +00:00

Part of #119 (1/4: foundation + com.atproto).

Lays the foundation the other three parts build on, and converts the com.atproto.* surface, the AtProtoClient facade, RecordCollection<T>, Session and PdsAdminClient to typed identifiers. Parts 2–4 (app.bsky; chat/ozone/site.standard; spaces and streaming) follow the conventions this PR writes down, in parallel.

How to review this

The diff is large but mostly mechanical. Read these first; everything else applies them.

New types (the only non-mechanical logic)

File	What to check
`src/ATProtoNet/Identity/AtDatetime.cs`	`readonly record struct`. Keeps the original text. Strict parser (`TryCreate`: syntax, calendar, `-00:00`, negative-year rejection) run against the interop fixtures. Lenient wire path (`FromWire`). Instants stored as ticks since year 0000 so valid 0000–0009 dates still order. Equality is ordinal on the text; ordering is by instant, with the text as tie-break.
`src/ATProtoNet/Serialization/IdentityJsonConverters.cs`	`AtDatetimeJsonConverter`: never rejects a string, rejects non-string tokens, and refuses to write `default`.
`src/ATProtoNet/Http/Pagination.cs`	The one cursor loop. Stops on a `null`, empty or already-seen cursor; a cycle A→B→A also ends.
`src/ATProtoNet/Models/CommonModels.cs`	`ICursorPage<out T>` replaces `ICursoredResponse`.
`tests/ATProtoNet.Tests/Conventions/TypedIdentifierGuardTests.cs`	The reflection guard. Scope, heuristic word list, commented `Exceptions`, temporary `PendingConversions`. Stale entries in either list fail the build, so parts 2–4 must delete theirs.
`tools/ATProtoNet.LexiconGenerator/CodeGen/TypeMapper.cs` (+ `CSharpEmitter`, `LexiconEmitter`)	Format → type mapping, `IReadOnlyList<T>`, and the reverse mapping.

Conventions (now in CONTRIBUTING.md → Lexicon models and clients, with before/after examples)

Identifier and datetime formats are typed, never string. Lexicon-required fields are required; optional ones are nullable.
Parameter order: subject identifiers → required inputs → optional filters → limit → cursor → CancellationToken.
Models expose IReadOnlyList<T>; methods take IEnumerable<T>.
Single-use request DTOs are internal.
Every cursored response implements ICursorPage<T>, with Items implemented explicitly so the wire shape does not change.
Enumerate* takes int? pageSize = null and goes through Pagination.

Mechanical parts

Model property types across Lexicon/Com/AtProto/{Repo,Server,Identity,Sync,Admin,Label,Moderation}.
Client signatures.
string.Empty defaults replaced by required.
List<T> replaced by IReadOnlyList<T>.
Call sites in tests, samples and docs, which parse literals with X.Parse("…").
Placeholder test CIDs ("bafy1", "bafytest", …) replaced with real ones, because responses now validate them.

Non-mechanical call-site decisions

AtProtoClient.ApplyOAuthSessionAsync: OAuthSessionResult.Handle falls back to the DID when the handle doesn't verify. The typed Session.Handle takes the handle only when IsHandleVerified is set, and is handle.invalid otherwise. A handle marked verified that doesn't parse throws before any client state changes.
AtProtoAuthenticationHandler built a placeholder Session with an empty DID. Its claims then carried that empty DID, because ResumeSessionAsync keeps the stored DID. The placeholder now takes the token's sub, and the handler rejects a token whose sub is not a DID.

What changed

`AtDatetime`

Preserves the original string, so re-serializing is byte-identical and the CID is stable. It is lenient on read: an invalid value is kept with IsValid == false. TryGetValue recovers near-miss ISO 8601 forms, with no time zone read as UTC, and Value throws when there is no instant.
Strict when constructed from code. Parse and TryParse accept only valid values. Now(), FromDateTimeOffset and FromDateTime write yyyy-MM-ddTHH:mm:ss.fffZ in the invariant culture, truncated to milliseconds.
Implements ISpanParsable<T>, IComparable<T>, IComparable and the comparison operators.
DateTimeKind.Unspecified is treated as UTC, as AtProtoJsonDefaults.FormatTimestamp always documented. FormatTimestamp, NowTimestamp and XrpcParams now all go through AtDatetime. FromDateTimeOffset(DateTime) compiles through .NET's implicit conversion, which reads Unspecified as local time; its docs point to FromDateTime instead.
AtProtoRecord.CreatedAt is AtDatetime? and is still set to the current time on construction.

Pagination

ICursorPage<T> is implemented by every cursored response in these namespaces and by RecordPage<T>: ListRecordsResponse, ListMissingBlobsResponse, ListBlobsResponse, ListReposResponse, ListReposByCollectionResponse, ListHostsResponse, QueryLabelsResponse and GetInviteCodesResponse.
New enumerators: Repo.EnumerateMissingBlobsAsync, Sync.EnumerateBlobsAsync, EnumerateReposAsync, EnumerateReposByCollectionAsync, EnumerateHostsAsync, Label.EnumerateLabelsAsync and Admin.EnumerateInviteCodesAsync.
ListAllRecordsAsync is renamed EnumerateRecordsAsync. RecordCollection<T>.EnumerateAsync and EnumerateFromAsync now use the paginator.

Typed identifiers (models and clients)

Common models:
- StrongRef.Uri and Cid are typed.
- On Label, Src and Cid are typed and Cts and Exp are AtDatetime. Label.Uri stays string: its Lexicon format is uri, so the subject can be a DID or an AT URI.
- BlobLink.Link and CidLink.Link are Cid.
RecordCollection<T>, RecordRef and RecordView<T>: identifiers are typed within the existing shapes. #122 redesigns these types.
The facade:
- Did is Did?, Handle is Handle? and LatestRepoRev is Tid?.
- GetCollection<T>, QueryAsync and ProcedureAsync take an Nsid.
- The Bluesky helpers take AtUri, Cid and Did.
- SetLabelers stays string: its entries can carry ;redact.
Session: Did and Handle are typed and required. Nothing else about the model changes; #124 redesigns it.
com.atproto.*:
- Every identifier and datetime field is typed.
- Client methods take typed identifiers, including Tid? since, Nsid? lxm and Cid? blob.
- New AT URI overloads: RepoClient.GetRecordAsync(AtUri), GetRecordAsync<T>(AtUri) and DeleteRecordAsync(AtUri).
- GetInviteCodesResponse.Codes is typed InviteCode instead of JsonElement.
PdsAdminClient: typed Did, Handle and AtIdentifier parameters. CreatePdsAccountRequest.Handle is a Handle and CreatePdsAccountRequest.Did a Did?.

Conventions applied

Parameter reorders:
- ListRecordsAsync, RecordCollection.ListAsync and ListFromAsync take reverse before limit and cursor.
- ListMissingBlobsAsync takes limit before cursor.
- ModerationClient.CreateReportAsync takes the subject first.
Now internal (23 single-use request DTOs):
- Repo: CreateRecord, PutRecord, DeleteRecord, ApplyWrites.
- Server: CreateSession, CreateAppPassword, RequestPasswordReset, ResetPassword, ConfirmEmail, CreateInviteCode, RevokeAppPassword, ReserveSigningKey.
- Identity: UpdateHandle.
- Sync: NotifyOfUpdate, RequestCrawl.
- Admin (7): AdminDeleteAccount, DisableAccountInvites, EnableAccountInvites, UpdateAccountEmail, UpdateAccountHandle, UpdateAccountPassword, DisableInviteCodes.
- Moderation: CreateReport.
Input collections: ApplyWritesAsync, DisableInviteCodesAsync, GetAccountInfosAsync, QueryLabelsAsync and PostAsync take IEnumerable<T>.

Generator

atproto-lexgen csharp:
- did, handle, at-identifier, at-uri, nsid, cid, record-key, tid and datetime map to the SDK's types, with using ATProtoNet.Identity; added only when needed.
- Arrays become IReadOnlyList<T>.
- uri, language and other formats stay string.
atproto-lexgen lexicon: maps the identifier types and IReadOnlyList<T> back to their formats. Records built on AtProtoRecord still emit createdAt as a datetime.

Guard test

Scope: the public properties, method parameters and constructor parameters of types in ATProtoNet, .Admin, .Auth, .Models, .Lexicon.*, .Spaces.*, .Streaming.* and .Server.Spaces.*.
Match rule: a member fails when its name's last camel-case word is on a list mined from the upstream Lexicons (did, uri, cid, at as in createdAt, rev, by, and so on) and its type is a string or a string sequence.
Pending (temporary): App.Bsky.* (PR 2); Chat.*, Tools.Ozone.* and Site.Standard.* (PR 3); Com.AtProto.Space*, SimpleSpace*, Spaces.*, Streaming.*, Server.Spaces.*, the subscribeRepos event models in SyncModels.cs and Auth.ServiceAuthGenerator (PR 4).
Permanent exceptions, each commented:
- Label.Uri
- SendEmailRequest.Subject
- InviteCode.ForAccount and CreatedBy, and AccountCodes.Account (the PDS writes admin)
- DisableInviteCodesAsync(accounts)
- SetLabelers(labelerDids)

Breaking changes and migration

Literals:
- Did.Parse("did:plc:…"), Nsid.Parse("com.example.todo.item"), RecordKey.Parse("self"), Cid.Parse("bafy…") and AtUri.Parse("at://…") wherever a string literal met a typed parameter.
- Values returned by the API pass through unchanged, and every type still converts implicitly to string.
- A response with an invalid identifier now throws XrpcResponseFormatException.
AtDatetime:
- Set with AtDatetime.Now(), Parse or FromDateTimeOffset.
- Read with .Value or .TryGetValue, and .ToString() returns the text.
- AtProtoRecord.CreatedAt is AtDatetime?.
Parameter reordering: positional calls to ListRecordsAsync, ListAsync/ListFromAsync, ListMissingBlobsAsync and CreateReportAsync no longer compile. Reorder them or pass the arguments by name.
Renames:
- ListAllRecordsAsync is now EnumerateRecordsAsync.
- Enumerators take int? pageSize = null. Pass pageSize: 100 to keep the old RecordCollection page size.
Internalized DTOs: call the client method, which takes the fields as parameters.
ICursoredResponse → ICursorPage<T>, and List<T> → IReadOnlyList<T>: copy a list you need to mutate ([.. page.Records]).
Session: set Did and Handle with typed values. An unverified OAuth handle now reads as handle.invalid.
Generated code: regenerate it, then parse literals as above.

All of these are in CHANGELOG.md under Breaking changes, with Added entries (AtDatetime, ICursorPage/enumerators, the AT URI overloads) and Fixed entries (the repeated-cursor loop, FormatTimestamp with Unspecified, and the auth handler's empty did claim).

Verification

Full solution build: 0 warnings, 0 errors.
Unit tests: 3163 passed, 0 failed, 5 skipped (the Aspire manifest tests, as on main).
New tests:
- AtDatetimeTests:
  - Every line of the vendored datetime_syntax_valid, datetime_syntax_invalid and datetime_parse_invalid interop fixtures: strict accept/reject, plus a byte-for-byte JSON round trip of all of them.
  - The canonical format under Thai, Arabic and Finnish cultures.
  - Unspecified, UTC and local DateTimes.
  - Offsets, including one beyond ±14h, and year 0000.
  - Lenient near-miss forms and unreadable text.
  - default, equality versus ordering, and the ISpanParsable contract.
- PaginationTests:
  - Repeated cursor, cycle, empty cursor, empty page, and cancellation.
  - End to end over HTTP: EnumerateRecordsAsync against a server that always returns the same cursor ends after 2 requests; before, it looped forever. EnumerateBlobsAsync and RecordCollection.EnumerateAsync are covered too.
- TypedRepoClientTests: typed request bodies and query strings, the AT URI overloads and their rejection of a URI without an rkey, an invalid identifier surfacing as XrpcResponseFormatException, and the reordered CreateReportAsync.
- TypedIdentifierGuardTests: the guard, the staleness checks on both lists, and the name classifier.
- Generator: a golden test for every format, the using added only when needed, and the reverse mapping from a typed record.
- OAuth apply: a verified handle is kept. An unverified one, whether a well-formed handle or the DID fallback, reads as handle.invalid. A verified handle that doesn't parse throws and leaves the client untouched.
- Auth handler: a non-DID or missing sub is rejected before the network call.
Integration tests, samples, Server and Blazor all compile. The integration tests skip without a PDS.

Deliberately deferred or flagged

subscribeRepos event models (FirehoseMessage, CommitEvent, RepoOp, … in Lexicon/Com/AtProto/Sync/SyncModels.cs) are the models the firehose reads. Converting them pulls in FirehoseVerifier, TypedFirehoseConsumer and SpaceSyncer, so they stay on PR 4's pending list by type name.
Auth.ServiceAuthGenerator (ServiceDid, lxm) is used almost only by Server.Spaces, so it is on PR 4's pending list.
The spaces and Jetstream archive enumerators still have their own cursor loops, and with them the repeated-cursor bug. PR 4 moves them onto Pagination once their responses implement ICursorPage<T>.
Out of scope for #119:
- DidDocument and the PLC models, including GetRecommendedDidCredentialsResponse and SignPlcOperationRequest (#120).
- The OAuth models and token stores (#124).
- The RecordRef/RecordView redesign (#122).
Left as string on purpose: PdsAdminOptions.AdminIdentifier (bound from configuration), GetServiceAuthAsync(aud) (a DID or did#service), ReserveSigningKeyResponse.SigningKey and CreateAccountRequest.RecoveryKey (did:key with no Lexicon format).
No [Obsolete] forwarders for the old string signatures or ListAllRecordsAsync. This is a clean break, per the owner's decision on #119.
RecordRef for a URI without an rkey: it now throws XrpcResponseFormatException instead of carrying a null RecordKey.
Stricter responses: required on response models means a server that omits a Lexicon-required field now fails deserialization. Before, the field was silently empty.
Edits outside this part's area, minimal and only to compile:
- Lexicon/Site/Standard/StandardSiteClient.cs (PR 3) parses at its RepoClient call sites; its public signatures are unchanged.
- AtProtoClient's Bluesky helpers build app.bsky records (PR 2's models) unchanged.
- Test data in StandardSite*Tests, UnknownFieldTests, GalleryEmbedTests and AtProtoClientUpdateProfileTests now uses valid CIDs and TIDs.

Review follow-up

ApplyOAuthSessionAsync guessed handle verification from whether the value parsed. It now uses the authoritative OAuthSessionResult.IsHandleVerified, as AtProtoOAuthService does: verified → the handle, unverified → handle.invalid. A verified handle that doesn't parse is an ArgumentException instead of a silent downgrade.
The DID and handle are typed before the lock and before any state is swapped, so a malformed session leaves the client as it was.
New tests: a verified handle is kept, an unverified well-formed handle becomes handle.invalid, the DID fallback also becomes handle.invalid, and a verified-but-invalid handle throws with the client untouched.
AtProtoClientFactoryTests now sets IsHandleVerified = true on its token data, because it asserts the real handle.
Unit tests: 3163 passed, 0 failed, 5 skipped. Build: 0 warnings.

🤖 Generated with Claude Code

Part of #119 (1/4: foundation + com.atproto). Lays the foundation the other three parts build on, and converts the `com.atproto.*` surface, the `AtProtoClient` facade, `RecordCollection<T>`, `Session` and `PdsAdminClient` to typed identifiers. Parts 2–4 (`app.bsky`; `chat`/`ozone`/`site.standard`; spaces and streaming) follow the conventions this PR writes down, in parallel. ## How to review this The diff is large but mostly mechanical. Read these first; everything else applies them. **New types (the only non-mechanical logic)** | File | What to check | |---|---| | `src/ATProtoNet/Identity/AtDatetime.cs` | `readonly record struct`. Keeps the original text. Strict parser (`TryCreate`: syntax, calendar, `-00:00`, negative-year rejection) run against the interop fixtures. Lenient wire path (`FromWire`). Instants stored as ticks since year 0000 so valid 0000–0009 dates still order. Equality is ordinal on the text; ordering is by instant, with the text as tie-break. | | `src/ATProtoNet/Serialization/IdentityJsonConverters.cs` | `AtDatetimeJsonConverter`: never rejects a string, rejects non-string tokens, and refuses to write `default`. | | `src/ATProtoNet/Http/Pagination.cs` | The one cursor loop. Stops on a `null`, empty or already-seen cursor; a cycle A→B→A also ends. | | `src/ATProtoNet/Models/CommonModels.cs` | `ICursorPage<out T>` replaces `ICursoredResponse`. | | `tests/ATProtoNet.Tests/Conventions/TypedIdentifierGuardTests.cs` | The reflection guard. Scope, heuristic word list, commented `Exceptions`, temporary `PendingConversions`. Stale entries in either list fail the build, so parts 2–4 must delete theirs. | | `tools/ATProtoNet.LexiconGenerator/CodeGen/TypeMapper.cs` (+ `CSharpEmitter`, `LexiconEmitter`) | Format → type mapping, `IReadOnlyList<T>`, and the reverse mapping. | **Conventions** (now in `CONTRIBUTING.md` → *Lexicon models and clients*, with before/after examples) - Identifier and `datetime` formats are typed, never `string`. Lexicon-required fields are `required`; optional ones are nullable. - Parameter order: subject identifiers → required inputs → optional filters → `limit` → `cursor` → `CancellationToken`. - Models expose `IReadOnlyList<T>`; methods take `IEnumerable<T>`. - Single-use request DTOs are `internal`. - Every cursored response implements `ICursorPage<T>`, with `Items` implemented explicitly so the wire shape does not change. - `Enumerate*` takes `int? pageSize = null` and goes through `Pagination`. **Mechanical parts** - Model property types across `Lexicon/Com/AtProto/{Repo,Server,Identity,Sync,Admin,Label,Moderation}`. - Client signatures. - `string.Empty` defaults replaced by `required`. - `List<T>` replaced by `IReadOnlyList<T>`. - Call sites in tests, samples and docs, which parse literals with `X.Parse("…")`. - Placeholder test CIDs (`"bafy1"`, `"bafytest"`, …) replaced with real ones, because responses now validate them. **Non-mechanical call-site decisions** - `AtProtoClient.ApplyOAuthSessionAsync`: `OAuthSessionResult.Handle` falls back to the DID when the handle doesn't verify. The typed `Session.Handle` takes the handle only when `IsHandleVerified` is set, and is `handle.invalid` otherwise. A handle marked verified that doesn't parse throws before any client state changes. - `AtProtoAuthenticationHandler` built a placeholder `Session` with an empty DID. Its claims then carried that empty DID, because `ResumeSessionAsync` keeps the stored DID. The placeholder now takes the token's `sub`, and the handler rejects a token whose `sub` is not a DID. ## What changed ### `AtDatetime` - Preserves the original string, so re-serializing is byte-identical and the CID is stable. It is lenient on read: an invalid value is kept with `IsValid == false`. `TryGetValue` recovers near-miss ISO 8601 forms, with no time zone read as UTC, and `Value` throws when there is no instant. - Strict when constructed from code. `Parse` and `TryParse` accept only valid values. `Now()`, `FromDateTimeOffset` and `FromDateTime` write `yyyy-MM-ddTHH:mm:ss.fffZ` in the invariant culture, truncated to milliseconds. - Implements `ISpanParsable<T>`, `IComparable<T>`, `IComparable` and the comparison operators. - **`DateTimeKind.Unspecified` is treated as UTC**, as `AtProtoJsonDefaults.FormatTimestamp` always documented. `FormatTimestamp`, `NowTimestamp` and `XrpcParams` now all go through `AtDatetime`. `FromDateTimeOffset(DateTime)` compiles through .NET's implicit conversion, which reads `Unspecified` as local time; its docs point to `FromDateTime` instead. - `AtProtoRecord.CreatedAt` is `AtDatetime?` and is still set to the current time on construction. ### Pagination - `ICursorPage<T>` is implemented by every cursored response in these namespaces and by `RecordPage<T>`: `ListRecordsResponse`, `ListMissingBlobsResponse`, `ListBlobsResponse`, `ListReposResponse`, `ListReposByCollectionResponse`, `ListHostsResponse`, `QueryLabelsResponse` and `GetInviteCodesResponse`. - New enumerators: `Repo.EnumerateMissingBlobsAsync`, `Sync.EnumerateBlobsAsync`, `EnumerateReposAsync`, `EnumerateReposByCollectionAsync`, `EnumerateHostsAsync`, `Label.EnumerateLabelsAsync` and `Admin.EnumerateInviteCodesAsync`. - `ListAllRecordsAsync` is renamed `EnumerateRecordsAsync`. `RecordCollection<T>.EnumerateAsync` and `EnumerateFromAsync` now use the paginator. ### Typed identifiers (models and clients) - **Common models:** - `StrongRef.Uri` and `Cid` are typed. - On `Label`, `Src` and `Cid` are typed and `Cts` and `Exp` are `AtDatetime`. `Label.Uri` stays `string`: its Lexicon format is `uri`, so the subject can be a DID or an AT URI. - `BlobLink.Link` and `CidLink.Link` are `Cid`. - **`RecordCollection<T>`, `RecordRef` and `RecordView<T>`:** identifiers are typed within the existing shapes. #122 redesigns these types. - **The facade:** - `Did` is `Did?`, `Handle` is `Handle?` and `LatestRepoRev` is `Tid?`. - `GetCollection<T>`, `QueryAsync` and `ProcedureAsync` take an `Nsid`. - The Bluesky helpers take `AtUri`, `Cid` and `Did`. - `SetLabelers` stays `string`: its entries can carry `;redact`. - **`Session`:** `Did` and `Handle` are typed and `required`. Nothing else about the model changes; #124 redesigns it. - **`com.atproto.*`:** - Every identifier and `datetime` field is typed. - Client methods take typed identifiers, including `Tid? since`, `Nsid? lxm` and `Cid? blob`. - New AT URI overloads: `RepoClient.GetRecordAsync(AtUri)`, `GetRecordAsync<T>(AtUri)` and `DeleteRecordAsync(AtUri)`. - `GetInviteCodesResponse.Codes` is typed `InviteCode` instead of `JsonElement`. - **`PdsAdminClient`:** typed `Did`, `Handle` and `AtIdentifier` parameters. `CreatePdsAccountRequest.Handle` is a `Handle` and `CreatePdsAccountRequest.Did` a `Did?`. ### Conventions applied - **Parameter reorders:** - `ListRecordsAsync`, `RecordCollection.ListAsync` and `ListFromAsync` take `reverse` before `limit` and `cursor`. - `ListMissingBlobsAsync` takes `limit` before `cursor`. - `ModerationClient.CreateReportAsync` takes the subject first. - **Now internal (23 single-use request DTOs):** - Repo: `CreateRecord`, `PutRecord`, `DeleteRecord`, `ApplyWrites`. - Server: `CreateSession`, `CreateAppPassword`, `RequestPasswordReset`, `ResetPassword`, `ConfirmEmail`, `CreateInviteCode`, `RevokeAppPassword`, `ReserveSigningKey`. - Identity: `UpdateHandle`. - Sync: `NotifyOfUpdate`, `RequestCrawl`. - Admin (7): `AdminDeleteAccount`, `DisableAccountInvites`, `EnableAccountInvites`, `UpdateAccountEmail`, `UpdateAccountHandle`, `UpdateAccountPassword`, `DisableInviteCodes`. - Moderation: `CreateReport`. - **Input collections:** `ApplyWritesAsync`, `DisableInviteCodesAsync`, `GetAccountInfosAsync`, `QueryLabelsAsync` and `PostAsync` take `IEnumerable<T>`. ### Generator - **`atproto-lexgen csharp`:** - `did`, `handle`, `at-identifier`, `at-uri`, `nsid`, `cid`, `record-key`, `tid` and `datetime` map to the SDK's types, with `using ATProtoNet.Identity;` added only when needed. - Arrays become `IReadOnlyList<T>`. - `uri`, `language` and other formats stay `string`. - **`atproto-lexgen lexicon`:** maps the identifier types and `IReadOnlyList<T>` back to their formats. Records built on `AtProtoRecord` still emit `createdAt` as a `datetime`. ### Guard test - **Scope:** the public properties, method parameters and constructor parameters of types in `ATProtoNet`, `.Admin`, `.Auth`, `.Models`, `.Lexicon.*`, `.Spaces.*`, `.Streaming.*` and `.Server.Spaces.*`. - **Match rule:** a member fails when its name's last camel-case word is on a list mined from the upstream Lexicons (`did`, `uri`, `cid`, `at` as in `createdAt`, `rev`, `by`, and so on) and its type is a `string` or a string sequence. - **Pending (temporary):** `App.Bsky.*` (PR 2); `Chat.*`, `Tools.Ozone.*` and `Site.Standard.*` (PR 3); `Com.AtProto.Space*`, `SimpleSpace*`, `Spaces.*`, `Streaming.*`, `Server.Spaces.*`, the `subscribeRepos` event models in `SyncModels.cs` and `Auth.ServiceAuthGenerator` (PR 4). - **Permanent exceptions, each commented:** - `Label.Uri` - `SendEmailRequest.Subject` - `InviteCode.ForAccount` and `CreatedBy`, and `AccountCodes.Account` (the PDS writes `admin`) - `DisableInviteCodesAsync(accounts)` - `SetLabelers(labelerDids)` ## Breaking changes and migration - **Literals:** - `Did.Parse("did:plc:…")`, `Nsid.Parse("com.example.todo.item")`, `RecordKey.Parse("self")`, `Cid.Parse("bafy…")` and `AtUri.Parse("at://…")` wherever a string literal met a typed parameter. - Values returned by the API pass through unchanged, and every type still converts implicitly to `string`. - A response with an invalid identifier now throws `XrpcResponseFormatException`. - **`AtDatetime`:** - Set with `AtDatetime.Now()`, `Parse` or `FromDateTimeOffset`. - Read with `.Value` or `.TryGetValue`, and `.ToString()` returns the text. - `AtProtoRecord.CreatedAt` is `AtDatetime?`. - **Parameter reordering:** positional calls to `ListRecordsAsync`, `ListAsync`/`ListFromAsync`, `ListMissingBlobsAsync` and `CreateReportAsync` no longer compile. Reorder them or pass the arguments by name. - **Renames:** - `ListAllRecordsAsync` is now `EnumerateRecordsAsync`. - Enumerators take `int? pageSize = null`. Pass `pageSize: 100` to keep the old `RecordCollection` page size. - **Internalized DTOs:** call the client method, which takes the fields as parameters. - **`ICursoredResponse` → `ICursorPage<T>`, and `List<T>` → `IReadOnlyList<T>`:** copy a list you need to mutate (`[.. page.Records]`). - **`Session`:** set `Did` and `Handle` with typed values. An unverified OAuth handle now reads as `handle.invalid`. - **Generated code:** regenerate it, then parse literals as above. All of these are in `CHANGELOG.md` under `Breaking changes`, with `Added` entries (`AtDatetime`, `ICursorPage`/enumerators, the AT URI overloads) and `Fixed` entries (the repeated-cursor loop, `FormatTimestamp` with `Unspecified`, and the auth handler's empty `did` claim). ## Verification - Full solution build: **0 warnings, 0 errors**. - Unit tests: **3163 passed, 0 failed, 5 skipped** (the Aspire manifest tests, as on `main`). - **New tests:** - **`AtDatetimeTests`:** - Every line of the vendored `datetime_syntax_valid`, `datetime_syntax_invalid` and `datetime_parse_invalid` interop fixtures: strict accept/reject, plus a byte-for-byte JSON round trip of all of them. - The canonical format under Thai, Arabic and Finnish cultures. - `Unspecified`, UTC and local `DateTime`s. - Offsets, including one beyond ±14h, and year 0000. - Lenient near-miss forms and unreadable text. - `default`, equality versus ordering, and the `ISpanParsable` contract. - **`PaginationTests`:** - Repeated cursor, cycle, empty cursor, empty page, and cancellation. - End to end over HTTP: `EnumerateRecordsAsync` against a server that always returns the same cursor ends after 2 requests; before, it looped forever. `EnumerateBlobsAsync` and `RecordCollection.EnumerateAsync` are covered too. - **`TypedRepoClientTests`:** typed request bodies and query strings, the AT URI overloads and their rejection of a URI without an rkey, an invalid identifier surfacing as `XrpcResponseFormatException`, and the reordered `CreateReportAsync`. - **`TypedIdentifierGuardTests`:** the guard, the staleness checks on both lists, and the name classifier. - **Generator:** a golden test for every format, the `using` added only when needed, and the reverse mapping from a typed record. - **OAuth apply:** a verified handle is kept. An unverified one, whether a well-formed handle or the DID fallback, reads as `handle.invalid`. A verified handle that doesn't parse throws and leaves the client untouched. - **Auth handler:** a non-DID or missing `sub` is rejected before the network call. - Integration tests, samples, Server and Blazor all compile. The integration tests skip without a PDS. ## Deliberately deferred or flagged - **`subscribeRepos` event models** (`FirehoseMessage`, `CommitEvent`, `RepoOp`, … in `Lexicon/Com/AtProto/Sync/SyncModels.cs`) are the models the firehose reads. Converting them pulls in `FirehoseVerifier`, `TypedFirehoseConsumer` and `SpaceSyncer`, so they stay on PR 4's pending list by type name. - **`Auth.ServiceAuthGenerator`** (`ServiceDid`, `lxm`) is used almost only by `Server.Spaces`, so it is on PR 4's pending list. - **The spaces and Jetstream archive enumerators** still have their own cursor loops, and with them the repeated-cursor bug. PR 4 moves them onto `Pagination` once their responses implement `ICursorPage<T>`. - **Out of scope for #119:** - `DidDocument` and the PLC models, including `GetRecommendedDidCredentialsResponse` and `SignPlcOperationRequest` (#120). - The OAuth models and token stores (#124). - The `RecordRef`/`RecordView` redesign (#122). - **Left as `string` on purpose:** `PdsAdminOptions.AdminIdentifier` (bound from configuration), `GetServiceAuthAsync(aud)` (a DID or `did#service`), `ReserveSigningKeyResponse.SigningKey` and `CreateAccountRequest.RecoveryKey` (`did:key` with no Lexicon format). - **No `[Obsolete]` forwarders** for the old string signatures or `ListAllRecordsAsync`. This is a clean break, per the owner's decision on #119. - **`RecordRef` for a URI without an rkey:** it now throws `XrpcResponseFormatException` instead of carrying a null `RecordKey`. - **Stricter responses:** `required` on response models means a server that omits a Lexicon-required field now fails deserialization. Before, the field was silently empty. - **Edits outside this part's area, minimal and only to compile:** - `Lexicon/Site/Standard/StandardSiteClient.cs` (PR 3) parses at its `RepoClient` call sites; its public signatures are unchanged. - `AtProtoClient`'s Bluesky helpers build `app.bsky` records (PR 2's models) unchanged. - Test data in `StandardSite*Tests`, `UnknownFieldTests`, `GalleryEmbedTests` and `AtProtoClientUpdateProfileTests` now uses valid CIDs and TIDs. ## Review follow-up - **`ApplyOAuthSessionAsync` guessed handle verification from whether the value parsed.** It now uses the authoritative `OAuthSessionResult.IsHandleVerified`, as `AtProtoOAuthService` does: verified → the handle, unverified → `handle.invalid`. A verified handle that doesn't parse is an `ArgumentException` instead of a silent downgrade. - The DID and handle are typed before the lock and before any state is swapped, so a malformed session leaves the client as it was. - **New tests:** a verified handle is kept, an unverified well-formed handle becomes `handle.invalid`, the DID fallback also becomes `handle.invalid`, and a verified-but-invalid handle throws with the client untouched. - `AtProtoClientFactoryTests` now sets `IsHandleVerified = true` on its token data, because it asserts the real handle. - Unit tests: 3163 passed, 0 failed, 5 skipped. Build: 0 warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-24 20:08:39 +00:00

feat!: typed identifiers, AtDatetime and one paginator for com.atproto (part of #119 )

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

Details

CI / build-and-test (pull_request) Successful in 59s

Details

a0159fccec

Part 1 of 4 of the typed public API. Lays the foundation the later parts
build on and converts the com.atproto surface:

- AtDatetime, the Lexicon datetime: keeps its text so records re-serialize
  byte for byte (CID-stable), reads leniently, is strict when created from
  code, and is checked against the atproto-interop-tests fixtures.
  FormatTimestamp now treats DateTimeKind.Unspecified as UTC, as documented.
- ICursorPage<T> on every cursored com.atproto response, and one internal
  paginator that stops on a null, empty or repeated cursor; a server that
  repeated its cursor used to drive an endless request loop. ListAll* is
  Enumerate*, with int? pageSize = null, and the missing enumerators exist.
- Typed Did/Handle/AtIdentifier/AtUri/Nsid/Cid/RecordKey/Tid/AtDatetime in
  CommonModels, RecordCollection, the AtProtoClient facade, Session,
  PdsAdminClient and Lexicon/Com/AtProto/{Repo,Server,Identity,Sync,Admin,
  Label,Moderation}; one parameter order; IReadOnlyList<T> outputs and
  IEnumerable<T> inputs; single-use request DTOs internal.
- atproto-lexgen maps string formats to the typed identifiers and arrays to
  IReadOnlyList<T>.
- A reflection guard test fails on any identifier-carrying public member
  typed as string, with a temporary allow-list for the areas parts 2-4
  convert, and CONTRIBUTING.md records the conventions they follow.

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

Grandiras commented

2026-09-24 20:08:47 +00:00

@claude /review — the diff is large and mostly mechanical. Please read these files in full from the branch, since the non-mechanical logic lives there: AtDatetime (new type and its JSON converter), the internal paginator plus ICursorPage<T>, the reflection guard test, AtProtoJsonDefaults.FormatTimestamp (Unspecified-kind handling), and the LexiconGenerator TypeMapper changes. The description has a How to review this section.

@claude /review — the diff is large and mostly mechanical. Please read these files in full from the branch, since the non-mechanical logic lives there: `AtDatetime` (new type and its JSON converter), the internal paginator plus `ICursorPage<T>`, the reflection guard test, `AtProtoJsonDefaults.FormatTimestamp` (Unspecified-kind handling), and the LexiconGenerator `TypeMapper` changes. The description has a *How to review this* section.

claude-bot commented

2026-09-24 20:08:50 +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/591); 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-154/run-591.md) — updating every ~5 minutes while the task runs.

claude-reviewer reviewed

2026-09-24 20:11:43 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a large, carefully-executed mechanical conversion, and the non-mechanical pieces called out in the description hold up under review: AtDatetime's strict/lenient split, calendar math, and instant/text ordering are internally consistent (verified the day-counting formula and the Year1Ticks offset by hand); Pagination.EnumerateAsync correctly stops on null/empty/repeated cursors including cycles; the AtDatetimeJsonConverter never rejects a string on read and refuses default on write; the TypedIdentifierGuardTests heuristic, exception list and pending-list staleness checks are sound; the generator's TypeMapper/LexiconEmitter round-trip identifier formats correctly via reflection; and the AtProtoAuthenticationHandler change correctly threads the token's sub through as the DID rather than trusting a stale placeholder.

One real gap: AtProtoClient.ApplyOAuthSessionAsync derives handle-verification status by trying to parse the handle string, instead of using the IsHandleVerified flag the OAuth client already computed for exactly this purpose (see finding). Everything else — parameter reorders, internalized DTOs, IReadOnlyList<T>/IEnumerable<T> conversions, the ICursorPage<T> wiring, the AT URI overloads and their ArgumentException on a recordless URI, the doc/sample updates — is mechanical and consistent with the stated conventions.

⏱ 2m 38s · 25 turns · tokens: 133.3k in / 12.4k out (+3084.6k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, carefully-executed mechanical conversion, and the non-mechanical pieces called out in the description hold up under review: `AtDatetime`'s strict/lenient split, calendar math, and instant/text ordering are internally consistent (verified the day-counting formula and the `Year1Ticks` offset by hand); `Pagination.EnumerateAsync` correctly stops on null/empty/repeated cursors including cycles; the `AtDatetimeJsonConverter` never rejects a string on read and refuses `default` on write; the `TypedIdentifierGuardTests` heuristic, exception list and pending-list staleness checks are sound; the generator's `TypeMapper`/`LexiconEmitter` round-trip identifier formats correctly via reflection; and the `AtProtoAuthenticationHandler` change correctly threads the token's `sub` through as the DID rather than trusting a stale placeholder. One real gap: `AtProtoClient.ApplyOAuthSessionAsync` derives handle-verification status by trying to parse the handle string, instead of using the `IsHandleVerified` flag the OAuth client already computed for exactly this purpose (see finding). Everything else — parameter reorders, internalized DTOs, `IReadOnlyList<T>`/`IEnumerable<T>` conversions, the `ICursorPage<T>` wiring, the AT URI overloads and their `ArgumentException` on a recordless URI, the doc/sample updates — is mechanical and consistent with the stated conventions. --- ⏱ 2m 38s · 25 turns · tokens: 133.3k in / 12.4k out (+3084.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-154/run-591.md)

src/ATProtoNet/AtProtoClient.cs Outdated

					
				@ -734,3 +752,2 @@

				            {

				                Did = oauthSession.Did,

				                Handle = oauthSession.Handle,

				                Did = Did.Parse(oauthSession.Did),

ApplyOAuthSessionAsync infers whether the OAuth handle verified by trying to parse oauthSession.Handle as a Handle, instead of checking the explicit oauthSession.IsHandleVerified flag the OAuth client already computed for this exact purpose. OAuthSessionResult.IsHandleVerified is documented for this (OAuthClient.cs:793), and the sibling code path in src/ATProtoNet.Blazor/Authentication/AtProtoOAuthService.cs:379 does it the direct way: result.IsHandleVerified ? result.Handle : "handle.invalid". Here it's Handle.TryParse(oauthSession.Handle, out var handle) ? handle : InvalidHandle — this only works today because the unverified fallback value happens to always be the DID string (which can't parse as a Handle). It silently diverges from the authoritative signal: if IsHandleVerified is false but Handle isn't literally the DID (a future OAuthClient change, a different placeholder shape), or a verified handle for some reason fails Handle.TryParse, this produces the wrong result, and the one new test for this (AtProtoClientApplyOAuthTests.ApplyOAuthSessionAsync_UnverifiedHandleFallingBackToTheDid_IsHandleInvalid) only covers the DID-shaped fallback case, not the flag itself. Prefer oauthSession.IsHandleVerified ? Handle.Parse(oauthSession.Handle) : InvalidHandle.

`ApplyOAuthSessionAsync` infers whether the OAuth handle verified by trying to parse `oauthSession.Handle` as a `Handle`, instead of checking the explicit `oauthSession.IsHandleVerified` flag the OAuth client already computed for this exact purpose. `OAuthSessionResult.IsHandleVerified` is documented for this (`OAuthClient.cs:793`), and the sibling code path in `src/ATProtoNet.Blazor/Authentication/AtProtoOAuthService.cs:379` does it the direct way: `result.IsHandleVerified ? result.Handle : "handle.invalid"`. Here it's `Handle.TryParse(oauthSession.Handle, out var handle) ? handle : InvalidHandle` — this only works today because the unverified fallback value happens to always be the DID string (which can't parse as a `Handle`). It silently diverges from the authoritative signal: if `IsHandleVerified` is `false` but `Handle` isn't literally the DID (a future `OAuthClient` change, a different placeholder shape), or a verified handle for some reason fails `Handle.TryParse`, this produces the wrong result, and the one new test for this (`AtProtoClientApplyOAuthTests.ApplyOAuthSessionAsync_UnverifiedHandleFallingBackToTheDid_IsHandleInvalid`) only covers the DID-shaped fallback case, not the flag itself. Prefer `oauthSession.IsHandleVerified ? Handle.Parse(oauthSession.Handle) : InvalidHandle`.

opencode-bot commented

2026-09-24 20:18:32 +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-154/run-2026-09-24T20-18-31-922Z.md) — updating every ~5 minutes while the review runs.

opencode-reviewer approved these changes

2026-09-24 20:27:19 +00:00

Dismissed

opencode-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Good to merge. The only non-mechanical logic is sound:

AtDatetime — I traced TryCreate against every line of the vendored datetime_syntax_valid/syntax_invalid/parse_invalid fixtures (length/character gates, -00:00 rejection, ticks < 0 for negative years, leap-year arithmetic including year 0, fraction-digit padding, offsets to ±23:59). TryGetValue's year-0000 and >±14h-offset normalization is correct and overflow-safe. FromWire's dddd- prefix check plus AssumeUniversal keeps culture-shaped junk out while recovering near-miss ISO 8601, as designed. The converter's never-reject-read / throw-on-default-write split matches the docs, and XrpcParams's DateTime/DateTimeOffset rewrites are behavior-identical to the old code (Unspecified → UTC).
Pagination — the single cursor loop is right: repeated cursor, cycle and empty-cursor all terminate (with HTTP-level tests proving the old infinite loop is gone), and items are yielded before the stop check.
Auth handler — requiring sub to be a DID before the PDS round trip, and seeding the placeholder session with it, fixes the empty-did-claim issue without weakening the delegated validation.
Guard test — the name-classifier theory cases match the word list, the stale-entry checks on Exceptions and PendingConversions work as described, and the permanent exceptions each carry a reason.
Spot-checked the mechanical conversions (Repo/Sync/Admin/Identity/Label/Moderation models and clients, RecordCollection, RecordRef/RecordPage, PdsAdminClient, Session, the facade's Bluesky helpers and static readonly NSIDs, implicit → string? conversions being null-safe for optional parameters, Tid/Cid query-param binding) — all consistent.

Two minor nits, non-blocking:

ListRecordsResponse.Records and ListMissingBlobsResponse.Blobs declare IReadOnlyList<…> { get; init; } = [] rather than required, unlike their siblings (ListBlobsResponse.Cids, ListReposResponse.Repos, GetInviteCodesResponse.Codes). A server omitting the Lexicon-required list now yields an empty page (and, for listRecords, an empty walk) instead of failing deserialization — slightly at odds with the "stricter responses" note in the changelog and the CONTRIBUTING rule. Worth aligning while the surface is being touched anyway.
LabelClient.EnumerateLabelsAsync calls uriPatterns.ToList() before the null-tolerant AddAll, so a null sequence would surface as NRE instead of ArgumentNullException/silently-empty. The parameter is non-nullable by annotation, so this is cosmetic at most.

⏱ 8m 47s · 42 steps · $0.1711 · tokens: 315.8k in / 25.4k out (+3701.5k cache-read) · Go: 5h 7% · wk 98% · mo 82%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Good to merge. The only non-mechanical logic is sound: - **`AtDatetime`** — I traced `TryCreate` against every line of the vendored `datetime_syntax_valid`/`syntax_invalid`/`parse_invalid` fixtures (length/character gates, `-00:00` rejection, `ticks < 0` for negative years, leap-year arithmetic including year 0, fraction-digit padding, offsets to ±23:59). `TryGetValue`'s year-0000 and >±14h-offset normalization is correct and overflow-safe. `FromWire`'s `dddd-` prefix check plus `AssumeUniversal` keeps culture-shaped junk out while recovering near-miss ISO 8601, as designed. The converter's never-reject-read / throw-on-default-write split matches the docs, and `XrpcParams`'s `DateTime`/`DateTimeOffset` rewrites are behavior-identical to the old code (Unspecified → UTC). - **`Pagination`** — the single cursor loop is right: repeated cursor, cycle and empty-cursor all terminate (with HTTP-level tests proving the old infinite loop is gone), and items are yielded before the stop check. - **Auth handler** — requiring `sub` to be a DID before the PDS round trip, and seeding the placeholder session with it, fixes the empty-`did`-claim issue without weakening the delegated validation. - **Guard test** — the name-classifier theory cases match the word list, the stale-entry checks on `Exceptions` and `PendingConversions` work as described, and the permanent exceptions each carry a reason. - Spot-checked the mechanical conversions (`Repo`/`Sync`/`Admin`/`Identity`/`Label`/`Moderation` models and clients, `RecordCollection`, `RecordRef`/`RecordPage`, `PdsAdminClient`, `Session`, the facade's Bluesky helpers and `static readonly` NSIDs, implicit `→ string?` conversions being null-safe for optional parameters, `Tid`/`Cid` query-param binding) — all consistent. Two minor nits, non-blocking: 1. `ListRecordsResponse.Records` and `ListMissingBlobsResponse.Blobs` declare `IReadOnlyList<…> { get; init; } = []` rather than `required`, unlike their siblings (`ListBlobsResponse.Cids`, `ListReposResponse.Repos`, `GetInviteCodesResponse.Codes`). A server omitting the Lexicon-required list now yields an empty page (and, for `listRecords`, an empty walk) instead of failing deserialization — slightly at odds with the "stricter responses" note in the changelog and the CONTRIBUTING rule. Worth aligning while the surface is being touched anyway. 2. `LabelClient.EnumerateLabelsAsync` calls `uriPatterns.ToList()` before the null-tolerant `AddAll`, so a null sequence would surface as NRE instead of `ArgumentNullException`/silently-empty. The parameter is non-nullable by annotation, so this is cosmetic at most. --- ⏱ 8m 47s · 42 steps · $0.1711 · tokens: 315.8k in / 25.4k out (+3701.5k cache-read) · Go: 5h 7% · wk 98% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-154/run-2026-09-24T20-18-31-922Z.md)

Grandiras commented

2026-09-24 22:52:59 +00:00

Addressed the review finding in 974c93b: ApplyOAuthSessionAsync now decides the handle by oauthSession.IsHandleVerified (verified → parsed handle; unverified → handle.invalid). A verified-but-unparseable handle throws before any state changes. New tests cover verified, unverified non-DID-shaped, unverified DID fallback, and verified-unparseable (client left untouched).

@claude /review

Addressed the review finding in `974c93b`: `ApplyOAuthSessionAsync` now decides the handle by `oauthSession.IsHandleVerified` (verified → parsed handle; unverified → `handle.invalid`). A verified-but-unparseable handle throws before any state changes. New tests cover verified, unverified non-DID-shaped, unverified DID fallback, and verified-unparseable (client left untouched). @claude /review

Grandiras force-pushed issue-119-typed-foundation from a0159fccec

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

Details

CI / build-and-test (pull_request) Successful in 59s

Details

to 974c93b958

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

Details

CI / build-and-test (pull_request) Successful in 59s

Details

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

Details

2026-09-24 22:52:59 +00:00

Compare

claude-bot commented

2026-09-24 22:53:02 +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/592); 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-154/run-592.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-24 22:53:58 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Re-review: the one blocking issue from my previous pass is fixed correctly. ApplyOAuthSessionAsync now derives the typed Handle directly from OAuthSessionResult.IsHandleVerified rather than inferring verification by trying to parse the string (src/ATProtoNet/AtProtoClient.cs): unverified → handle.invalid, verified → Handle.Parse, and a verified-but-unparseable handle now throws ArgumentException before any client state is touched (did/handle are computed before _refreshLock is even acquired). This matches the sibling path in AtProtoOAuthService.cs and the PR's stated intent, and is exercised by new cases in AtProtoClientApplyOAuthTests.cs (verified kept, unverified well-formed handle → invalid, DID-shaped fallback → invalid, verified-but-invalid throws) plus AtProtoClientFactoryTests.cs setting IsHandleVerified = true for its typed-handle assertion.

I also spot-checked the surrounding non-mechanical pieces referenced in the PR description for consistency with the current branch state: RecordRef.From(nsid, uri, cid) in RecordCollection.cs matches its call site in AtProtoClient.UpdateProfileAsync, and AtProtoAuthenticationHandler's TryPreValidateJwt correctly threads the token's sub through as the DID (validated as a Did before use) with a handle.invalid placeholder, rejecting tokens whose sub isn't a DID before any PDS round-trip. Nothing new here raises a concern.

No other issues found. Everything else in this large mechanical conversion (typed identifiers, AtDatetime, the pagination loop, ICursorPage<T>, internalized DTOs, parameter reorders) was already reviewed and holds up; this pass only needed to confirm the follow-up fix, which it does cleanly. Approving.

⏱ 0m 44s · 16 turns · tokens: 98.7k in / 3.8k out (+1672.6k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: the one blocking issue from my previous pass is fixed correctly. `ApplyOAuthSessionAsync` now derives the typed `Handle` directly from `OAuthSessionResult.IsHandleVerified` rather than inferring verification by trying to parse the string (`src/ATProtoNet/AtProtoClient.cs`): unverified → `handle.invalid`, verified → `Handle.Parse`, and a verified-but-unparseable handle now throws `ArgumentException` before any client state is touched (`did`/`handle` are computed before `_refreshLock` is even acquired). This matches the sibling path in `AtProtoOAuthService.cs` and the PR's stated intent, and is exercised by new cases in `AtProtoClientApplyOAuthTests.cs` (verified kept, unverified well-formed handle → invalid, DID-shaped fallback → invalid, verified-but-invalid throws) plus `AtProtoClientFactoryTests.cs` setting `IsHandleVerified = true` for its typed-handle assertion. I also spot-checked the surrounding non-mechanical pieces referenced in the PR description for consistency with the current branch state: `RecordRef.From(nsid, uri, cid)` in `RecordCollection.cs` matches its call site in `AtProtoClient.UpdateProfileAsync`, and `AtProtoAuthenticationHandler`'s `TryPreValidateJwt` correctly threads the token's `sub` through as the DID (validated as a `Did` before use) with a `handle.invalid` placeholder, rejecting tokens whose `sub` isn't a DID before any PDS round-trip. Nothing new here raises a concern. No other issues found. Everything else in this large mechanical conversion (typed identifiers, `AtDatetime`, the pagination loop, `ICursorPage<T>`, internalized DTOs, parameter reorders) was already reviewed and holds up; this pass only needed to confirm the follow-up fix, which it does cleanly. Approving. --- ⏱ 0m 44s · 16 turns · tokens: 98.7k in / 3.8k out (+1672.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-154/run-592.md)

opencode-bot commented

2026-09-24 22:54:01 +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-154/run-2026-09-24T22-54-00-807Z.md) — updating every ~5 minutes while the review runs.

Grandiras merged commit 3ca606542b into main

2026-09-24 22:54:09 +00:00

Grandiras deleted branch issue-119-typed-foundation

2026-09-24 22:54:10 +00:00

Grandiras referenced this pull request from a commit

2026-09-24 22:54:10 +00:00

feat!: typed identifiers, AtDatetime and one paginator for com.atproto (#154)

opencode-reviewer approved these changes

2026-09-24 22:55:19 +00:00

opencode-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Good to merge. The blocker from the previous round is resolved: ApplyOAuthSessionAsync now uses the authoritative OAuthSessionResult.IsHandleVerified instead of inferring verification by parsing the handle string. Unverified → handle.invalid, verified → the parsed handle, verified-but-unparseable → ArgumentException thrown before the refresh lock and before any client state is touched. This matches the sibling path in AtProtoOAuthService and is exercised by the four new cases in AtProtoClientApplyOAuthTests (verified kept, unverified well-formed handle → invalid, DID fallback → invalid, verified-but-invalid throws and leaves the client untouched), with AtProtoClientFactoryTests updated to set IsHandleVerified = true. No regressions found in the areas reviewed previously (AtDatetime, the pagination loop, the converter, ICursorPage<T> wiring, the guard test).

Two minor, non-blocking notes carried over from my earlier pass, both cosmetic:

ListRecordsResponse.Records and ListMissingBlobsResponse.Blobs declare IReadOnlyList<…> { get; init; } = [] rather than required, unlike their siblings (ListBlobsResponse.Cids, GetInviteCodesResponse.Codes), so a server omitting the Lexicon-required list yields an empty page instead of failing deserialization. Worth aligning in a later pass.
LabelClient.EnumerateLabelsAsync calls uriPatterns.ToList() before the null-tolerant AddAll, so a null sequence surfaces as NRE rather than ArgumentNullException; the parameter is non-nullable by annotation, so this is cosmetic at most.

⏱ 1m 18s · 14 steps · $0.0384 · tokens: 108.1k in / 3.9k out (+673.7k cache-read) · Go: 5h 7% · wk 98% · mo 82%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Good to merge. The blocker from the previous round is resolved: `ApplyOAuthSessionAsync` now uses the authoritative `OAuthSessionResult.IsHandleVerified` instead of inferring verification by parsing the handle string. Unverified → `handle.invalid`, verified → the parsed handle, verified-but-unparseable → `ArgumentException` thrown before the refresh lock and before any client state is touched. This matches the sibling path in `AtProtoOAuthService` and is exercised by the four new cases in `AtProtoClientApplyOAuthTests` (verified kept, unverified well-formed handle → invalid, DID fallback → invalid, verified-but-invalid throws and leaves the client untouched), with `AtProtoClientFactoryTests` updated to set `IsHandleVerified = true`. No regressions found in the areas reviewed previously (`AtDatetime`, the pagination loop, the converter, `ICursorPage<T>` wiring, the guard test). Two minor, non-blocking notes carried over from my earlier pass, both cosmetic: 1. `ListRecordsResponse.Records` and `ListMissingBlobsResponse.Blobs` declare `IReadOnlyList<…> { get; init; } = []` rather than `required`, unlike their siblings (`ListBlobsResponse.Cids`, `GetInviteCodesResponse.Codes`), so a server omitting the Lexicon-required list yields an empty page instead of failing deserialization. Worth aligning in a later pass. 2. `LabelClient.EnumerateLabelsAsync` calls `uriPatterns.ToList()` before the null-tolerant `AddAll`, so a null sequence surfaces as NRE rather than `ArgumentNullException`; the parameter is non-nullable by annotation, so this is cosmetic at most. --- ⏱ 1m 18s · 14 steps · $0.0384 · tokens: 108.1k in / 3.9k out (+673.7k cache-read) · Go: 5h 7% · wk 98% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-154/run-2026-09-24T22-54-00-807Z.md)

Grandiras referenced this pull request

2026-09-24 23:09:15 +00:00

feat!: typed identifiers, AtDatetime and enumerators for app.bsky (2/4 of #119) #155

Grandiras referenced this pull request

2026-09-24 23:10:09 +00:00

feat!: typed identifiers, AtDatetime and enumerators for chat.bsky, tools.ozone and site.standard (3/4 of #119) #156

Grandiras referenced this pull request

2026-09-24 23:35:48 +00:00

feat!: typed identifiers for Spaces, the space server and streaming (4/4 of #119) #157

Sign in to join this conversation.

No reviewers