fix(identity)!: spec-conformant identifier types and a monotonic TID generator #146

Merged

Grandiras merged 1 commit from issue-114-identifiers into main 2026-09-24 18:14:13 +00:00

Conversation 6 Commits 1 Files changed 55 +2371 -699

Grandiras commented 2026-09-24 18:00:56 +00:00

Owner

Copy link

Closes #114.

The identifier types are meant to guarantee validity, but several of them accepted input the atproto specs reject, and Tid.Next() produced out-of-order and duplicate TIDs. This PR makes parsing spec-conformant (checked against the official interop fixtures), adds a monotonic TidGenerator, and replaces the hand-written value-type boilerplate with sealed records, one parse interface and one JSON converter.

Bugs

• Tid.Next() monotonic and collision-free. The new TidGenerator(int? clockId = null, TimeProvider? timeProvider = null) fixes one 10-bit clock id per generator (random by default, RandomNumberGenerator) and advances a microsecond timestamp with a lock-free CAS loop: next = max(now, last + 1). It never repeats and never goes backwards, even when the clock stalls or steps back. It throws InvalidOperationException once the timestamp would overflow 53 bits. Tid.Next(), Tid.NextString() and RecordKey.NewTid() use one internal shared instance.
• Interop syntax fixtures. bluesky-social/atproto-interop-tests syntax/ is vendored verbatim at commit 056e5741bb330757205d6b16db5266fffcae937b (CC0) into tests/ATProtoNet.Tests/Interop/syntax/, with a README recording the origin, commit and license. A nested .editorconfig stops editors from trimming the whitespace, which is part of the test data. SyntaxInteropTests asserts every valid line parses, keeping its text, and every invalid line is rejected by both TryParse and Parse. It covers TID, AT URI, DID, NSID, handle, record key, AT identifier and the invalid CIDs. The parser fixes:
  • Tid: the first character must be in 234567abcdefghij (the high bit stays clear).
  • RecordKey: ^[A-Za-z0-9._:~-]{1,512}$, excluding . and ...
  • Did: the final character may no longer be %.
  • Nsid: the spec's reference regex, which limits the name to 63 characters.
  • AtUri: the restricted Lexicon syntax, at://AUTHORITY[/NSID[/RKEY]]. Each part is validated by its own type, and an authority with an @ prefix is rejected. Trailing slashes, empty or extra segments, queries and fragments are rejected. The limit is 8 KiB.
• Cid validates. It accepts only a lower-case base32 b… CIDv1 with codec 0x71 or 0x55 and a sha2-256 multihash with a 32-byte digest, in canonical encoding (the unused trailing bits must be zero). It exposes CidCodec Codec, ReadOnlyMemory<byte> Digest and byte[] ToBytes() (a copy). Cid.Parse("hello") now throws.
• AtUri typed accessors. The type now has AtIdentifier Repo, Nsid? Collection and RecordKey? RecordKey, all computed once at parse time. Authority (a string) keeps the text as written, while Repo lower-cases a handle. Create(AtIdentifier repo, Nsid? collection = null, RecordKey? rkey = null) builds the URI without re-parsing it, and throws when an rkey is given without a collection.
• JSON converters. An invalid value now fails as a JsonException with a null message, so System.Text.Json writes its standard message with Path, line and position. The inner FormatException names the value and the type. Example: The JSON value could not be converted to ATProtoNet.Identity.AtUri. Path: $.Uri | ….

API shape

• The ten types (Did, Handle, AtIdentifier, Nsid, Tid, RecordKey, Cid, AtUri, SpaceUri, SpaceRecordUri) are now sealed records implementing an internal IIdentifier<TSelf>.
  • The interface declares one static abstract TryCreate(ReadOnlySpan<char>, string?, out TSelf?) and supplies IParsable<T>/ISpanParsable<T> as static virtual default implementations, so each type states its validation once.
  • Every type also implements IComparable<T> (ordinal) and has an explicit conversion from string.
  • Parse is TryParse ? x : throw. It still throws ArgumentException (ArgumentNullException for null). The IParsable members throw FormatException, as that contract expects.
  • Span parsing allocates only on success, and the string overloads reuse the caller's string.
• There is one IdentifierJsonConverter<T> in place of the eight copies, and the seven unused internal UnsafeCreate methods are deleted. SpaceUri/SpaceRecordUri now also serialize as JSON strings; before, they serialized as objects that could not be read back.
• Kept as before:
  • Ordinal equality. Handle moved from OrdinalIgnoreCase to ordinal, which is equivalent because handles are always lower-cased.
  • Handle lower-casing and @ stripping.
  • FromDid/FromHandle, Tid.NextString(), FromInt64/ToInt64.
• The implicit conversions that remain cannot throw: T → string and Did/Handle → AtIdentifier now map null to null. Before, they threw a NullReferenceException or built an AtIdentifier with neither DID nor handle.

Breaking changes and migration

• Stricter parsing (behavioral): values that used to parse, and now violate the spec, throw from Parse and from JSON deserialization. Migration: read legacy or foreign data with TryParse and handle false.
• Records (binary break, source-compatible for Parse/TryParse/==/Equals/ToString/conversions). Migration: recompile.
• AtUri: Collection is now Nsid? and RecordKey is now RecordKey? (both convert implicitly to string). Create(AtIdentifier, string?, string?) is replaced by Create(AtIdentifier, Nsid?, RecordKey?). Migration: AtUri.Create(repo, Nsid.Parse(c), RecordKey.Parse(k)), and uri.Collection?.Value wherever a string member was used.
• A converter failure is now a JsonException rather than an ArgumentException. This is listed under Fixed: code that caught ArgumentException around deserialization should catch JsonException.

Verification

• Full solution build: 0 warnings, 0 errors.
• Unit tests: 2,160 passed, 0 failed, 5 skipped (2,165 total; up from 1,713). The fixture and TID-generator tests also pass in Release.
• Interop fixtures: 374 distinct fixture lines are asserted, all passing. Before this change (per the research harness), the SDK wrongly accepted invalid AT URIs, record keys, TIDs, DIDs and NSIDs; handles already passed.
• TidGeneratorTests:
  • 100,000 sequential calls are strictly increasing.
  • 8 threads × 12,500 calls: each caller's sequence is strictly increasing, and all 100,000 values are distinct.
  • With a frozen clock, each call adds exactly +1 µs; a clock stepping back still gives increasing values.
  • The spec's example 3jzfcijpj2z2a is reproduced from its instant and clock id 6.
  • Clock-id range checks and 53-bit overflow are covered.
• CidTests pins real CIDs: the empty MST node bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm and the raw empty-bytes CID. It rejects upper case, CIDv0, dag-pb, BLAKE3, non-canonical padding bits, padding and truncation.
• IdentifierContractTests runs generic T.Parse/T.TryParse (string and span), CompareTo, conversions and null handling over all ten types.

Deliberately deferred / notes

• Fragments: the research table (4.5) suggested parsing an optional #fragment. The current fixtures and the spec's restricted syntax reject every fragment and query, so AtUri rejects them too. The general AT URI syntax (with query and fragment) is not modelled.
• cid_syntax_valid.txt is not asserted. Its lines are generic multiformats CIDs (base58, base16, dag-pb), none of which the atproto data model blesses. No CidParseMode.Lenient was added: nothing in the SDK needs one.
• Datetime and language fixtures: the upstream fixtures also cover datetime_*, language_* and uri_*. They are vendored but unused, because the SDK has no Datetime/Language types yet (see the Interop README).
• Fixture loading: the fixtures are read from the source tree (by walking up from the test binary) rather than copied to the output, because the csproj files belong to a parallel PR. It works for bin/ and artifacts/ layouts.
• Jetstream: JetstreamCommitEvent.Uri now throws, lazily, on an event whose collection or rkey is not spec-valid. The event itself is still delivered.

🤖 Generated with Claude Code

Closes #114. The identifier types are meant to guarantee validity, but several of them accepted input the atproto specs reject, and `Tid.Next()` produced out-of-order and duplicate TIDs. This PR makes parsing spec-conformant (checked against the official interop fixtures), adds a monotonic `TidGenerator`, and replaces the hand-written value-type boilerplate with sealed records, one parse interface and one JSON converter. ## Bugs - **`Tid.Next()` monotonic and collision-free.** The new `TidGenerator(int? clockId = null, TimeProvider? timeProvider = null)` fixes one 10-bit clock id per generator (random by default, `RandomNumberGenerator`) and advances a microsecond timestamp with a lock-free CAS loop: `next = max(now, last + 1)`. It never repeats and never goes backwards, even when the clock stalls or steps back. It throws `InvalidOperationException` once the timestamp would overflow 53 bits. `Tid.Next()`, `Tid.NextString()` and `RecordKey.NewTid()` use one internal shared instance. - **Interop syntax fixtures.** `bluesky-social/atproto-interop-tests` `syntax/` is vendored verbatim at commit `056e5741bb330757205d6b16db5266fffcae937b` (CC0) into `tests/ATProtoNet.Tests/Interop/syntax/`, with a README recording the origin, commit and license. A nested `.editorconfig` stops editors from trimming the whitespace, which is part of the test data. `SyntaxInteropTests` asserts every valid line parses, keeping its text, and every invalid line is rejected by both `TryParse` and `Parse`. It covers TID, AT URI, DID, NSID, handle, record key, AT identifier and the invalid CIDs. The parser fixes: - `Tid`: the first character must be in `234567abcdefghij` (the high bit stays clear). - `RecordKey`: `^[A-Za-z0-9._:~-]{1,512}$`, excluding `.` and `..`. - `Did`: the final character may no longer be `%`. - `Nsid`: the spec's reference regex, which limits the name to 63 characters. - `AtUri`: the restricted Lexicon syntax, `at://AUTHORITY[/NSID[/RKEY]]`. Each part is validated by its own type, and an authority with an `@` prefix is rejected. Trailing slashes, empty or extra segments, queries and fragments are rejected. The limit is 8 KiB. - **`Cid` validates.** It accepts only a lower-case base32 `b…` CIDv1 with codec `0x71` or `0x55` and a sha2-256 multihash with a 32-byte digest, in canonical encoding (the unused trailing bits must be zero). It exposes `CidCodec Codec`, `ReadOnlyMemory<byte> Digest` and `byte[] ToBytes()` (a copy). `Cid.Parse("hello")` now throws. - **`AtUri` typed accessors.** The type now has `AtIdentifier Repo`, `Nsid? Collection` and `RecordKey? RecordKey`, all computed once at parse time. `Authority` (a string) keeps the text as written, while `Repo` lower-cases a handle. `Create(AtIdentifier repo, Nsid? collection = null, RecordKey? rkey = null)` builds the URI without re-parsing it, and throws when an rkey is given without a collection. - **JSON converters.** An invalid value now fails as a `JsonException` with a null message, so System.Text.Json writes its standard message with `Path`, line and position. The inner `FormatException` names the value and the type. Example: `The JSON value could not be converted to ATProtoNet.Identity.AtUri. Path: $.Uri | …`. ## API shape - The ten types (`Did`, `Handle`, `AtIdentifier`, `Nsid`, `Tid`, `RecordKey`, `Cid`, `AtUri`, `SpaceUri`, `SpaceRecordUri`) are now `sealed record`s implementing an internal `IIdentifier<TSelf>`. - The interface declares one `static abstract TryCreate(ReadOnlySpan<char>, string?, out TSelf?)` and supplies `IParsable<T>`/`ISpanParsable<T>` as static virtual default implementations, so each type states its validation once. - Every type also implements `IComparable<T>` (ordinal) and has an explicit conversion from `string`. - `Parse` is `TryParse ? x : throw`. It still throws `ArgumentException` (`ArgumentNullException` for null). The `IParsable` members throw `FormatException`, as that contract expects. - Span parsing allocates only on success, and the string overloads reuse the caller's string. - There is one `IdentifierJsonConverter<T>` in place of the eight copies, and the seven unused internal `UnsafeCreate` methods are deleted. `SpaceUri`/`SpaceRecordUri` now also serialize as JSON strings; before, they serialized as objects that could not be read back. - Kept as before: - Ordinal equality. `Handle` moved from `OrdinalIgnoreCase` to ordinal, which is equivalent because handles are always lower-cased. - Handle lower-casing and `@` stripping. - `FromDid`/`FromHandle`, `Tid.NextString()`, `FromInt64`/`ToInt64`. - The implicit conversions that remain cannot throw: `T → string` and `Did`/`Handle → AtIdentifier` now map `null` to `null`. Before, they threw a `NullReferenceException` or built an `AtIdentifier` with neither DID nor handle. ## Breaking changes and migration - **Stricter parsing** (behavioral): values that used to parse, and now violate the spec, throw from `Parse` and from JSON deserialization. **Migration:** read legacy or foreign data with `TryParse` and handle `false`. - **Records** (binary break, source-compatible for `Parse`/`TryParse`/`==`/`Equals`/`ToString`/conversions). **Migration:** recompile. - **`AtUri`**: `Collection` is now `Nsid?` and `RecordKey` is now `RecordKey?` (both convert implicitly to `string`). `Create(AtIdentifier, string?, string?)` is replaced by `Create(AtIdentifier, Nsid?, RecordKey?)`. **Migration:** `AtUri.Create(repo, Nsid.Parse(c), RecordKey.Parse(k))`, and `uri.Collection?.Value` wherever a `string` member was used. - A converter failure is now a `JsonException` rather than an `ArgumentException`. This is listed under *Fixed*: code that caught `ArgumentException` around deserialization should catch `JsonException`. ## Verification - Full solution build: 0 warnings, 0 errors. - Unit tests: 2,160 passed, 0 failed, 5 skipped (2,165 total; up from 1,713). The fixture and TID-generator tests also pass in Release. - Interop fixtures: 374 distinct fixture lines are asserted, all passing. Before this change (per the research harness), the SDK wrongly accepted invalid AT URIs, record keys, TIDs, DIDs and NSIDs; handles already passed. - `TidGeneratorTests`: - 100,000 sequential calls are strictly increasing. - 8 threads × 12,500 calls: each caller's sequence is strictly increasing, and all 100,000 values are distinct. - With a frozen clock, each call adds exactly +1 µs; a clock stepping back still gives increasing values. - The spec's example `3jzfcijpj2z2a` is reproduced from its instant and clock id 6. - Clock-id range checks and 53-bit overflow are covered. - `CidTests` pins real CIDs: the empty MST node `bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm` and the raw empty-bytes CID. It rejects upper case, CIDv0, dag-pb, BLAKE3, non-canonical padding bits, padding and truncation. - `IdentifierContractTests` runs generic `T.Parse`/`T.TryParse` (string and span), `CompareTo`, conversions and null handling over all ten types. ## Deliberately deferred / notes - **Fragments:** the research table (4.5) suggested parsing an optional `#fragment`. The current fixtures and the spec's restricted syntax reject every fragment and query, so `AtUri` rejects them too. The general AT URI syntax (with query and fragment) is not modelled. - **`cid_syntax_valid.txt` is not asserted.** Its lines are generic multiformats CIDs (base58, base16, dag-pb), none of which the atproto data model blesses. No `CidParseMode.Lenient` was added: nothing in the SDK needs one. - **Datetime and language fixtures:** the upstream fixtures also cover `datetime_*`, `language_*` and `uri_*`. They are vendored but unused, because the SDK has no `Datetime`/`Language` types yet (see the Interop README). - **Fixture loading:** the fixtures are read from the source tree (by walking up from the test binary) rather than copied to the output, because the csproj files belong to a parallel PR. It works for `bin/` and `artifacts/` layouts. - **Jetstream:** `JetstreamCommitEvent.Uri` now throws, lazily, on an event whose collection or rkey is not spec-valid. The event itself is still delivered. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-09-24 18:00:56 +00:00

fix(identity)!: spec-conformant identifier types and a monotonic TID generator (closes #114) ... 0c28320060

The identifier types are meant to guarantee validity, but several accepted
input the atproto specs reject, and Tid.Next() added a random clock id to a
millisecond timestamp on every call, so TIDs came out of order and collided.

- TidGenerator: fixed clock id per generator, lock-free CAS over the last
  microsecond timestamp, TimeProvider-injectable. Tid.Next() and
  RecordKey.NewTid() use a shared instance.
- Parsing follows the specs and is checked against every line of the
  vendored atproto-interop-tests syntax fixtures (TID, AT URI, DID, NSID,
  handle, record key, AT identifier, invalid CIDs). AtUri uses the
  restricted Lexicon syntax: validated authority/collection/rkey, no
  trailing slash, query or fragment.
- Cid validates a base32 CIDv1 (DRISL or raw, SHA-256) and exposes Codec,
  Digest and ToBytes().
- AtUri exposes Nsid? Collection, RecordKey? RecordKey and a Repo parsed
  once; Create takes typed components.
- The identifier types are sealed records sharing one internal parse
  interface that supplies IParsable/ISpanParsable, backing a single generic
  JSON converter that fails with a JsonException carrying the path.

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

Grandiras commented 2026-09-24 18:01:01 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

claude-bot commented 2026-09-24 18:01:06 +00:00

Collaborator

Copy link

🤖 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/580); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-146/run-580.md) — updating every ~5 minutes while the task runs.

opencode-bot commented 2026-09-24 18:03:30 +00:00

Collaborator

Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-146/run-2026-09-24T18-03-30-233Z.md) — updating every ~5 minutes while the review runs.

Grandiras force-pushed issue-114-identifiers from 0c28320060 to 5c7c779655 2026-09-24 18:05:14 +00:00 Compare

claude-reviewer approved these changes 2026-09-24 18:07:35 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

This is a large, careful rewrite of the identifier types and I did not find correctness bugs after tracing through the trickiest parts.

Things I specifically checked and found correct:

• AtUri's manual path splitting correctly rejects trailing slashes, extra segments, queries and fragments by relying on the fact that /?# are not in the Did/Handle/Nsid/RecordKey character classes, so a segment containing one of those characters fails its own type's regex rather than needing extra AtUri-level checks.
• AtIdentifier.TryCreateStrict (used only for AT URI authorities) correctly rejects a leading @, while the public AtIdentifier.Parse/TryParse intentionally keeps the lenient @-stripping behavior via Handle.TryCreate — two different entry points for two different contracts, which is a deliberate and consistent design rather than an inconsistency.
• Cid's base32 decoder uses a 12-bit (0xFFF) scratch buffer without ever explicitly clearing already-extracted bits; I worked through several iterations by hand and confirmed the modular arithmetic is still correct (a standard fixed-width bit-accumulator idiom: 12 bits is exactly 7 pending + 5 new, so the mask always discards only already-consumed bits before they could corrupt a later extraction). The canonical-encoding check (buffer & ((1<<bits)-1) == 0) correctly rejects non-zero padding bits.
• Records that override Equals/GetHashCode explicitly (AtUri, Cid) avoid the pitfall of default record equality touching internal fields like Cid's byte array or AtUri's derived Repo/Collection/RecordKey. Records that rely on default record equality (Did, Handle, Nsid, Tid, RecordKey, AtIdentifier) only have Value (or, for AtIdentifier, Did/Handle) as their actual stored fields, so the compiler-generated equality ends up equivalent to the old hand-written ordinal comparison — including Handle's move from OrdinalIgnoreCase to ordinal, which is safe precisely because Value is already lower-cased at construction.
• TidGenerator's CAS loop (next = max(now, last + 1)) is correct for monotonicity across threads and clock stalls/steps, and the 53-bit overflow check throwing after already committing the bad value to _lastTimestamp is the right behavior (a real overflow is permanent, so failing permanently afterward is correct, not a bug).
• The regex changes (Did no longer ending in %/:, Nsid name capped at 63 chars, RecordKey's tightened character class, Tid's high-bit-clear first character) all match the PR's stated rules and are backed by regression tests.

One very minor nit, not worth blocking on: the PR description states span-based parsing "allocates only on success," but AtUri's IIdentifier<AtUri>.TryCreate can allocate a string via span.ToString() even when the body ultimately fails to parse (once the scheme/length pre-check passes), and SpaceUri/SpaceRecordUri's TryCreate always allocates on the span path regardless of outcome, since their engines are string-based. Purely a documentation precision issue, not a functional one.

---

⏱ 6m 17s · 1 turns · tokens: 78.3k in / 36.2k out (+23.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, careful rewrite of the identifier types and I did not find correctness bugs after tracing through the trickiest parts. Things I specifically checked and found correct: - `AtUri`'s manual path splitting correctly rejects trailing slashes, extra segments, queries and fragments by relying on the fact that `/?#` are not in the Did/Handle/Nsid/RecordKey character classes, so a segment containing one of those characters fails its own type's regex rather than needing extra AtUri-level checks. - `AtIdentifier.TryCreateStrict` (used only for AT URI authorities) correctly rejects a leading `@`, while the public `AtIdentifier.Parse`/`TryParse` intentionally keeps the lenient `@`-stripping behavior via `Handle.TryCreate` — two different entry points for two different contracts, which is a deliberate and consistent design rather than an inconsistency. - `Cid`'s base32 decoder uses a 12-bit (`0xFFF`) scratch buffer without ever explicitly clearing already-extracted bits; I worked through several iterations by hand and confirmed the modular arithmetic is still correct (a standard fixed-width bit-accumulator idiom: 12 bits is exactly `7 pending + 5 new`, so the mask always discards only already-consumed bits before they could corrupt a later extraction). The canonical-encoding check (`buffer & ((1<<bits)-1) == 0`) correctly rejects non-zero padding bits. - Records that override `Equals`/`GetHashCode` explicitly (`AtUri`, `Cid`) avoid the pitfall of default record equality touching internal fields like `Cid`'s byte array or `AtUri`'s derived `Repo`/`Collection`/`RecordKey`. Records that rely on default record equality (`Did`, `Handle`, `Nsid`, `Tid`, `RecordKey`, `AtIdentifier`) only have `Value` (or, for `AtIdentifier`, `Did`/`Handle`) as their actual stored fields, so the compiler-generated equality ends up equivalent to the old hand-written ordinal comparison — including `Handle`'s move from `OrdinalIgnoreCase` to ordinal, which is safe precisely because `Value` is already lower-cased at construction. - `TidGenerator`'s CAS loop (`next = max(now, last + 1)`) is correct for monotonicity across threads and clock stalls/steps, and the 53-bit overflow check throwing after already committing the bad value to `_lastTimestamp` is the right behavior (a real overflow is permanent, so failing permanently afterward is correct, not a bug). - The regex changes (Did no longer ending in `%`/`:`, Nsid name capped at 63 chars, RecordKey's tightened character class, Tid's high-bit-clear first character) all match the PR's stated rules and are backed by regression tests. One very minor nit, not worth blocking on: the PR description states span-based parsing "allocates only on success," but `AtUri`'s `IIdentifier<AtUri>.TryCreate` can allocate a string via `span.ToString()` even when the body ultimately fails to parse (once the scheme/length pre-check passes), and `SpaceUri`/`SpaceRecordUri`'s `TryCreate` always allocates on the span path regardless of outcome, since their engines are string-based. Purely a documentation precision issue, not a functional one. --- ⏱ 6m 17s · 1 turns · tokens: 78.3k in / 36.2k out (+23.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-146/run-580.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:0c28320060e8db13e3c68c67527dc3663cf50167 -->

Grandiras force-pushed issue-114-identifiers from 5c7c779655 to c3be6a8d56 2026-09-24 18:11:29 +00:00 Compare

Grandiras commented 2026-09-24 18:14:11 +00:00

Author

Owner

Copy link

Rebased onto main twice to resolve CHANGELOG-only conflicts (#143, #148, #147 merged). verify-rebase: the PR's patch outside CHANGELOG.md is byte-identical between the approved head 0c283200 and the new head c3be6a8d (3164 changed lines, hunk offsets ignored). CI is green on the new head, so the approval carries over (maintainer-approved policy for CHANGELOG-only rebases).

Rebased onto main twice to resolve CHANGELOG-only conflicts (#143, #148, #147 merged). `verify-rebase`: the PR's patch outside `CHANGELOG.md` is byte-identical between the approved head `0c283200` and the new head `c3be6a8d` (3164 changed lines, hunk offsets ignored). CI is green on the new head, so the approval carries over (maintainer-approved policy for CHANGELOG-only rebases).

Grandiras merged commit cb878d60d6 into main 2026-09-24 18:14:13 +00:00

Grandiras deleted branch issue-114-identifiers 2026-09-24 18:14:13 +00:00

Grandiras referenced this pull request from a commit 2026-09-24 18:14:13 +00:00

fix(identity)!: spec-conformant identifier types and a monotonic TID generator (#146)

Grandiras referenced this pull request 2026-09-24 18:16:58 +00:00

feat(serialization)!: forward-compatible models — open unions and unknown-field round-tripping #149

opencode-reviewer approved these changes 2026-09-24 18:19:18 +00:00

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Solid, well-tested change. I verified the implementation against the atproto reference patterns and the pinned upstream fixtures rather than taking the description on faith:

• TidGenerator — the CAS loop is correct: next = max(now, previous + 1) with Volatile.Read + Interlocked.CompareExchange gives strictly increasing, distinct TIDs per generator even when the clock stalls or steps back, and per-call now is hoisted correctly. Epoch conversion, clock-id range, and the 53-bit overflow path all check out; the spec example 3jzfcijpj2z2a decodes to 2023-06-30T15:03:01.887…Z as claimed.
• Cid — the hand-rolled base32 decoder is correct (12-bit buffer holds the max 7 leftover + 5 new bits; 58 chars yield exactly 36 bytes; canonical trailing-bits check makes the string form unique), and every line of cid_syntax_invalid.txt fails by construction.
• AtUri — splitting on / and validating each part genuinely rejects fragments, queries, trailing slashes and extra/empty segments, because the DID/handle/NSID/record-key patterns exclude /?# and whitespace; Create's no-re-parse argument holds (max component lengths sum to ~2.9 KiB).
• Fixtures — confirmed verbatim from upstream commit 056e5741bb33… (the tree exists upstream; aturi_syntax_valid.txt, tid_syntax_*, recordkey_syntax_*, cid_syntax_invalid.txt match byte-for-byte, including upstream's own quirks). The source-tree walk works for both bin/ and artifacts/ layouts.
• Regexes match the reference patterns: DID ^did:[a-z]+:[a-zA-Z0-9._:%-]*[a-zA-Z0-9._-]$, NSID 63-char name limit, TID first char in the lower half of base32-sortable, record key [A-Za-z0-9._:~-]{1,512} minus ./...
• Jetstream — unparseable CIDs/DIDs are tolerated and the event is still delivered; Uri throws lazily as documented. The strict-CID change doesn't break the CAR/MST paths, which work with binary CIDs; the only affected path (RepoCommit.Cid) previously produced a silent garbage CID for non-blessed blocks and now throws, which is the intended fix.

Minor remarks (non-blocking, no action required to merge):

1. TidGenerator.Next() commits the overflowing value to _lastTimestamp before throwing, so once the 53-bit limit is hit the generator throws on every later call, even if the clock is corrected back within range. Unreachable until year 2255, and arguably intended for a broken generator — but checking next > MaxTimestamp before the CAS would let a clock correction recover it.
2. Tid.FromInt64 documents "the high bit must be clear" but doesn't reject values ≥ 2^63; such a TID round-trips through FromInt64/ToInt64 yet is rejected by Parse (pre-existing, unchanged by this PR).
3. IdentifierJsonConverter<T>.Read calls reader.GetString() without checking the token kind, so a non-string token (e.g. a number where a CID is expected) throws InvalidOperationException rather than JsonException — pre-existing behavior, unchanged here; worth a follow-up if you want uniform failures.

The breaking changes are properly documented in the CHANGELOG with migration notes, and the JSON converter change to JsonException is correctly classified as a fix.

---

⏱ 15m 47s · 38 steps · $0.1728 · tokens: 247.8k in / 44.1k out (+3786.6k cache-read) · Go: 5h 2% · wk 96% · mo 81%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Solid, well-tested change. I verified the implementation against the atproto reference patterns and the pinned upstream fixtures rather than taking the description on faith: - **`TidGenerator`** — the CAS loop is correct: `next = max(now, previous + 1)` with `Volatile.Read` + `Interlocked.CompareExchange` gives strictly increasing, distinct TIDs per generator even when the clock stalls or steps back, and per-call `now` is hoisted correctly. Epoch conversion, clock-id range, and the 53-bit overflow path all check out; the spec example `3jzfcijpj2z2a` decodes to 2023-06-30T15:03:01.887…Z as claimed. - **`Cid`** — the hand-rolled base32 decoder is correct (12-bit buffer holds the max 7 leftover + 5 new bits; 58 chars yield exactly 36 bytes; canonical trailing-bits check makes the string form unique), and every line of `cid_syntax_invalid.txt` fails by construction. - **`AtUri`** — splitting on `/` and validating each part genuinely rejects fragments, queries, trailing slashes and extra/empty segments, because the DID/handle/NSID/record-key patterns exclude `/?#` and whitespace; `Create`'s no-re-parse argument holds (max component lengths sum to ~2.9 KiB). - **Fixtures** — confirmed verbatim from upstream commit `056e5741bb33…` (the tree exists upstream; `aturi_syntax_valid.txt`, `tid_syntax_*`, `recordkey_syntax_*`, `cid_syntax_invalid.txt` match byte-for-byte, including upstream's own quirks). The source-tree walk works for both `bin/` and `artifacts/` layouts. - **Regexes** match the reference patterns: DID `^did:[a-z]+:[a-zA-Z0-9._:%-]*[a-zA-Z0-9._-]$`, NSID 63-char name limit, TID first char in the lower half of base32-sortable, record key `[A-Za-z0-9._:~-]{1,512}` minus `.`/`..`. - **Jetstream** — unparseable CIDs/DIDs are tolerated and the event is still delivered; `Uri` throws lazily as documented. The strict-CID change doesn't break the CAR/MST paths, which work with binary CIDs; the only affected path (`RepoCommit.Cid`) previously produced a silent garbage CID for non-blessed blocks and now throws, which is the intended fix. Minor remarks (non-blocking, no action required to merge): 1. `TidGenerator.Next()` commits the overflowing value to `_lastTimestamp` before throwing, so once the 53-bit limit is hit the generator throws on every later call, even if the clock is corrected back within range. Unreachable until year 2255, and arguably intended for a broken generator — but checking `next > MaxTimestamp` before the CAS would let a clock correction recover it. 2. `Tid.FromInt64` documents "the high bit must be clear" but doesn't reject values ≥ 2^63; such a TID round-trips through `FromInt64`/`ToInt64` yet is rejected by `Parse` (pre-existing, unchanged by this PR). 3. `IdentifierJsonConverter<T>.Read` calls `reader.GetString()` without checking the token kind, so a non-string token (e.g. a number where a CID is expected) throws `InvalidOperationException` rather than `JsonException` — pre-existing behavior, unchanged here; worth a follow-up if you want uniform failures. The breaking changes are properly documented in the CHANGELOG with migration notes, and the JSON converter change to `JsonException` is correctly classified as a fix. --- ⏱ 15m 47s · 38 steps · $0.1728 · tokens: 247.8k in / 44.1k out (+3786.6k cache-read) · Go: 5h 2% · wk 96% · mo 81% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-146/run-2026-09-24T18-03-30-233Z.md) <!-- opencode-reviewed-head:0c28320060e8db13e3c68c67527dc3663cf50167 --> <!-- opencode-verdict:approve -->

Sign in to join this conversation.

Reviewers

No reviewers

claude-reviewer

opencode-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

5 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!146

No description provided.