Fix TID validation rejecting valid TIDs starting with a-j - #695
Merged
MarshalX merged 5 commits intoJul 18, 2026
Conversation
The previous check (ord(v[0]) & 0x40) was intended to enforce that the high bit of the decoded 64-bit TID value is 0, but it tested the ASCII encoding of the first character instead. All lowercase letters have the 0x40 bit set in ASCII, so every spec-valid TID starting with a-j was rejected (only digit-leading TIDs passed). Encode the constraint in the regex instead, mirroring the reference implementation: the first character must be one of 234567abcdefghij. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 16, 2026
Open
Contributor
Author
|
Follow-up on the interop gap mentioned above: I've proposed the missing vectors upstream in bluesky-social/atproto#5243. It adds three letter-first valid TIDs ( |
MarshalX
reviewed
Jul 17, 2026
MarshalX
left a comment
MarshalX
left a comment
Owner
There was a problem hiding this comment.
Thanks for catching this and for the fix! I left a few inline comments, all minor
Happy to merge once the docstring wording is adjusted
Contributor
Author
|
All updated per comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi! Great work in this SDK!
While writing spec-conformance tests for an AT Protocol project I'm working on, I used this library's validators as a cross-check and hit a case where we disagreed on a valid TID. I believe the first-character check in validate_tid has a subtle bug:
if not TID_RE.match(v) or (ord(v[0]) & 0x40):The intent (per the TID spec) is that the high bit of the decoded 64-bit value must be 0, which restricts the first character to 234567abcdefghij. But ord(v[0]) & 0x40 tests bit 6 of the character's ASCII encoding, and every lowercase letter has that bit set — so all spec-valid TIDs starting with a–j are rejected:
This slipped past CI because the interop test files don't include a letter-leading valid TID. This is true of the upstream bluesky-social/atproto vectors too (I checked; the vendored copies here are identical to upstream), so the suite was rightfully green. I'm thinking of proposing the missing vector upstream as well.
The fix encodes the constraint in the regex itself, matching the reference implementation in @atproto/syntax tid.ts character-for-character (
/^[234567abcdefghij][234567abcdefghijklmnopqrstuvwxyz]{12}$/), and removes the ASCII-bit check. Added parametrized tests for valid first characters (a, j) and invalid ones (k, z, 1, 8). The valid-case tests fail on main and pass with this change. Full suite: 445 passed.Happy to adjust anything for naming, test placement, whatever fits your conventions best. Thanks!