feat(lexicon)!: lexicon resolution and publishing, permission sets in atproto-lexgen #169

Merged

Grandiras merged 2 commits from issue-130-lexicon into main 2026-09-26 15:55:40 +00:00

Conversation 8 Commits 2 Files changed 44 +5367 -2238

Grandiras commented 2026-09-26 15:48:07 +00:00

Owner

Copy link

Closes #130.

Lexicon resolution and publication per the Lexicon spec, permission sets per the permission spec, and a reworked atproto-lexgen.

Runtime (ATProtoNet)

• com.atproto.lexicon.* — LexiconSchemaRecord (com.atproto.lexicon.schema, rkey = NSID; defs kept as raw JSON, unknown fields round-trip) and ResolvedLexicon ({uri, cid, schema}, the resolveLexicon output) in Lexicon/Com/AtProto/Lexicon/LexiconModels.cs; LexiconClient (client.Lexicon.ResolveLexiconAsync) in LexiconClient.cs, registered on AtProtoClient. XrpcErrors.LexiconNotFound added.
• ILexiconResolver / LexiconResolver — matches @atproto/lex-resolver 0.3.0:
  • DNS _lexicon. + the NSID authority reversed (name segment dropped), exactly one did= TXT value (two identical ones fail too, as in the reference), no hierarchical walk;
  • DID → PDS + #atproto key through the injected IDidResolver;
  • com.atproto.sync.getRecord + RecordProof.Verify (the reference verifies the commit proof, so this does too); then $type, lexicon == 1, id == nsid, non-empty defs.
  • Reuses the identity layer: the DoH TXT lookup was extracted from HandleResolver into internal Identity/DnsTxtLookup.cs (HandleResolver now calls it; behaviour unchanged), the IdentityResolverOptions DoH endpoint/timeout, and the SSRF-safe handler (IdentityNetworkPolicy); a non-HTTPS PDS is refused unless AllowPrivateNetworks.
  • ResolveAuthorityAsync(nsid) and ResolveAsync(nsid, authority) (the reference's didAuthority) are public; errors are LexiconResolutionException with a Kind (AuthorityNotFound, ResolutionFailed, NotFound, InvalidRecord, NotPermissionSet).
• CachingLexiconResolver — same model as CachingDidResolver (stale-while-revalidate, failure TTL, LRU capacity, shared in-flight fetch, InvalidateAsync). Defaults: stale 5 min (upstream oauth-provider's LEXICON_REFRESH_FREQUENCY), expire 24 h (the permission spec's upper bound), failures 1 min; a failed refresh keeps serving the stale schema until expiry, as the permission spec asks.
• LexiconClient is also an ILexiconResolver (explicit implementation) for the service path; it checks id == nsid and maps LexiconNotFound.
• AddAtProtoIdentity also registers ILexiconResolver (a caching resolver over the shared IDidResolver).
• Permission-set existence check (design choice, flagged): AtProtoScopes belongs to the parallel OAuth PR #167, so the check lives on the resolver side: ILexiconResolver.ResolvePermissionSetAsync(Nsid) and ResolveIncludeScopeAsync(string scope) (parses include:<nsid> / include?nsid=), returning the typed LexiconPermissionSet. Usable with the scope AtProtoScopes.Include(...) builds; AtProtoScopes itself is untouched.

atproto-lexgen

• Permission sets — LexiconSchema parses permission-set (title, title:lang, detail, detail:lang, permissions); csharp emits public static class <Name> { const string Nsid; static string Include(string? aud = null) }, forwarding to AtProtoScopes.Include (same signature on main and in #167). Generated code was compiled against the SDK with warnings as errors.
• lint (new) — Validation/LexiconLinter.cs: permission sets (only repo/rpc; blob/account/identity error; no wildcards; collection/lxm must be in the set's own NSID group or below, same check as IncludeScope.isParentAuthorityOf; action ∈ create/update/delete, unique; rpc aud is * xor inheritAud: true, never a service DID; unknown resources/parameters warn since servers drop them), plus valid id, non-empty defs, primary types named main, the removed null type anywhere, and */non-NSID in space collections. --strict fails on warnings.
• publish (rewritten) — lints (errors stop it), signs in (--identifier/ATPROTO_IDENTIFIER; password only from ATPROTO_PASSWORD or a no-echo prompt, --password is rejected without echoing; --pds/ATPROTO_PDS_URL, else resolved from the handle/DID), diffs each schema against the published record (identical → SAME; any breaking change refuses the whole run, nothing written, unless --force), putRecord with rkey = NSID and swapRecord on update, logs out, then checks each authority's _lexicon TXT record and prints OK / MISSING (with the record to create) / MISMATCH / UNKNOWN.
• resolve <nsid>... (new) — verified resolution through the SDK resolver; prints the Lexicon file (without $type) or writes --output <dir>/<nsid path>.json; --did skips DNS.
• migrate removed — evaluated: the run mode applied add/remove/rename operations to local JSON files, and scaffold mode only printed stubs. The evolution rules forbid removing/renaming/retyping fields and require new NSIDs for breaking changes, so there is nothing for it to migrate. The command now prints why and points to diff --strict.
• Debloat — removed TypeMapper.GetLexiconType/InferStringFormat/ToCamelCase/NsidToFilePath/ParseTypeValue (→ SplitRef), CSharpEmitter.Emit(LexiconDocument), LexiconEmitter.JsonOptions, the whole Migrations/ folder; the three directory loaders, the two assembly→document loops and the duplicate options are now Schema/LexiconLoader.cs + LexiconJson.
• null def type removed from the emitter's vocabulary (warns "removed from the Lexicon language"); space defs: */non-NSID collections are left out of the emitted declaration with a warning (the emitted Nsid.Parse would throw); name length follows the proposal (no limit; the SpaceTypeDeclaration.Name doc's "1–64" was dropped).
• Tool now references ATProtoNet (publish/resolve use it). To keep lexicon/diff --assembly binding the inspected assembly to its own dependencies, it is loaded into a collectible AssemblyLoadContext (InspectionLoadContext); framework assemblies stay shared so JsonPropertyNameAttribute matching still works. The packed tool contains ATProtoNet.dll and its 5 small dependencies.
• Program.cs testability — RunAsync(args, CommandContext) with substitutable output, environment, secret prompt and network (ILexgenNetwork/ILexiconRepository); Main binds the console.

Breaking changes (migration)

• atproto-lexgen publish no longer copies to a directory; --output/--baseline/--assembly/--no-bump are gone and explain the new command. Migration: keep published copies in version control and use diff --strict; publish with publish --input <dir> --identifier <handle> + ATPROTO_PASSWORD.
• atproto-lexgen migrate and the ATProtoNet.LexiconGenerator.Migrations types are removed. Migration: none needed under the evolution rules; gate changes with diff --strict.
• Removed tool helpers: CSharpEmitter.Emit(doc) → EmitAll([doc]), LexiconEmitter.JsonOptions → LexiconJson.WriteOptions, TypeMapper.ParseTypeValue → SplitRef; the others had no callers.

Verification

• Unit tests: 4341 passed, 0 failed, 5 skipped (LexiconMigrationTests and LexiconPublisherTests were deleted with the code they tested). Full solution build: 0 warnings, 0 errors.
• Pinned reference vector tests/ATProtoNet.Tests/Lexicon/TestData/lexicon-resolution.json, generated with @atproto/repo 0.10.14 / @atproto/crypto 0.5.5 and checked with @atproto/lex-resolver 0.3.0 (generator + README alongside, deterministic): a 44-record signed repo; post and authBasic resolve (URI/CID match), mismatch (id ≠ rkey), badType (wrong $type) and missing (absence proof) fail — the SDK agrees on all five, and rejects the proof under another signing key.
• New tests: LexiconResolverTests (vector, DNS name/spec examples, no hierarchical walk, exactly-one-DID, DoH failure/disabled/configured endpoint, DID/PDS/HTTPS failures, RecordNotFound, timeout, SSRF refusal, permission set + include scope parsing, DI), CachingLexiconResolverTests, LexiconClientTests, LexiconLinterTests (spec example set clean + every rule), CSharpEmitterTests additions, and LexiconGeneratorProgramTests (66 in-process tests: every command's help/unknown option/happy/error paths; publish create/same/update/breaking/force/non-lexicon record/DNS statuses/lint refusal/password handling; resolve print/write/--did/failure).
• Live check (not in CI): atproto-lexgen resolve site.standard.document site.standard.authFull app.bsky.authFullApp chat.bsky.authFullChatClient resolved and verified all four from the real network, a non-existent authority failed with AuthorityNotFound, and lint passed the four published sets with 0 errors/0 warnings. dotnet pack of the tool succeeds.

Notes / deferred

• Lexicon document validation is structural only ($type, version, id, defs); full lex-document schema validation (L4) is out of scope.
• LexiconDiffer does not compare permission-set contents yet (added/removed permissions are not reported by diff/publish); a small follow-up.
• CLAUDE.md still describes the tool as "…schema diffing, and migrations" — not edited here; worth updating.
• Edits outside the Lexicon area: Identity/HandleResolver.cs (DoH code moved to DnsTxtLookup.cs), Http/XrpcErrors.cs (+1 constant), AtProtoClient.cs (+1 property), Server/Extensions/IdentityServiceCollectionExtensions.cs (+ILexiconResolver), Spaces/SpaceTypeDeclaration.cs (one doc line), and allow-list entries in Conventions/* and LexiconDriftTests.Allowlists.cs.

🤖 Generated with Claude Code

Closes #130. Lexicon resolution and publication per the [Lexicon spec](https://atproto.com/specs/lexicon#lexicon-publication-and-resolution), permission sets per the [permission spec](https://atproto.com/specs/permission#permission-sets), and a reworked `atproto-lexgen`. ## Runtime (`ATProtoNet`) - **`com.atproto.lexicon.*`** — `LexiconSchemaRecord` (`com.atproto.lexicon.schema`, rkey = NSID; defs kept as raw JSON, unknown fields round-trip) and `ResolvedLexicon` (`{uri, cid, schema}`, the `resolveLexicon` output) in `Lexicon/Com/AtProto/Lexicon/LexiconModels.cs`; `LexiconClient` (`client.Lexicon.ResolveLexiconAsync`) in `LexiconClient.cs`, registered on `AtProtoClient`. `XrpcErrors.LexiconNotFound` added. - **`ILexiconResolver` / `LexiconResolver`** — matches `@atproto/lex-resolver` 0.3.0: - DNS `_lexicon.` + the NSID authority reversed (name segment dropped), exactly one `did=` TXT value (two identical ones fail too, as in the reference), **no hierarchical walk**; - DID → PDS + `#atproto` key through the injected `IDidResolver`; - `com.atproto.sync.getRecord` + `RecordProof.Verify` (the reference verifies the commit proof, so this does too); then `$type`, `lexicon == 1`, `id == nsid`, non-empty `defs`. - Reuses the identity layer: the DoH TXT lookup was extracted from `HandleResolver` into internal `Identity/DnsTxtLookup.cs` (HandleResolver now calls it; behaviour unchanged), the `IdentityResolverOptions` DoH endpoint/timeout, and the SSRF-safe handler (`IdentityNetworkPolicy`); a non-HTTPS PDS is refused unless `AllowPrivateNetworks`. - `ResolveAuthorityAsync(nsid)` and `ResolveAsync(nsid, authority)` (the reference's `didAuthority`) are public; errors are `LexiconResolutionException` with a `Kind` (`AuthorityNotFound`, `ResolutionFailed`, `NotFound`, `InvalidRecord`, `NotPermissionSet`). - **`CachingLexiconResolver`** — same model as `CachingDidResolver` (stale-while-revalidate, failure TTL, LRU capacity, shared in-flight fetch, `InvalidateAsync`). Defaults: stale 5 min (upstream oauth-provider's `LEXICON_REFRESH_FREQUENCY`), expire 24 h (the permission spec's upper bound), failures 1 min; a failed refresh keeps serving the stale schema until expiry, as the permission spec asks. - **`LexiconClient` is also an `ILexiconResolver`** (explicit implementation) for the service path; it checks `id == nsid` and maps `LexiconNotFound`. - **`AddAtProtoIdentity`** also registers `ILexiconResolver` (a caching resolver over the shared `IDidResolver`). - **Permission-set existence check (design choice, flagged):** `AtProtoScopes` belongs to the parallel OAuth PR #167, so the check lives on the resolver side: `ILexiconResolver.ResolvePermissionSetAsync(Nsid)` and `ResolveIncludeScopeAsync(string scope)` (parses `include:<nsid>` / `include?nsid=`), returning the typed `LexiconPermissionSet`. Usable with the scope `AtProtoScopes.Include(...)` builds; `AtProtoScopes` itself is untouched. ## `atproto-lexgen` - **Permission sets** — `LexiconSchema` parses `permission-set` (`title`, `title:lang`, `detail`, `detail:lang`, `permissions`); `csharp` emits `public static class <Name> { const string Nsid; static string Include(string? aud = null) }`, forwarding to `AtProtoScopes.Include` (same signature on `main` and in #167). Generated code was compiled against the SDK with warnings as errors. - **`lint` (new)** — `Validation/LexiconLinter.cs`: permission sets (only `repo`/`rpc`; `blob`/`account`/`identity` error; no wildcards; `collection`/`lxm` must be in the set's own NSID group or below, same check as `IncludeScope.isParentAuthorityOf`; `action` ∈ create/update/delete, unique; rpc `aud` is `*` xor `inheritAud: true`, never a service DID; unknown resources/parameters warn since servers drop them), plus valid `id`, non-empty `defs`, primary types named `main`, the removed `null` type anywhere, and `*`/non-NSID in space `collections`. `--strict` fails on warnings. - **`publish` (rewritten)** — lints (errors stop it), signs in (`--identifier`/`ATPROTO_IDENTIFIER`; password only from `ATPROTO_PASSWORD` or a no-echo prompt, `--password` is rejected without echoing; `--pds`/`ATPROTO_PDS_URL`, else resolved from the handle/DID), diffs each schema against the published record (identical → `SAME`; any breaking change refuses the whole run, nothing written, unless `--force`), `putRecord` with rkey = NSID and `swapRecord` on update, logs out, then checks each authority's `_lexicon` TXT record and prints `OK` / `MISSING` (with the record to create) / `MISMATCH` / `UNKNOWN`. - **`resolve <nsid>...` (new)** — verified resolution through the SDK resolver; prints the Lexicon file (without `$type`) or writes `--output <dir>/<nsid path>.json`; `--did` skips DNS. - **`migrate` removed** — evaluated: the run mode applied add/remove/rename operations to local JSON files, and scaffold mode only printed stubs. The evolution rules forbid removing/renaming/retyping fields and require new NSIDs for breaking changes, so there is nothing for it to migrate. The command now prints why and points to `diff --strict`. - **Debloat** — removed `TypeMapper.GetLexiconType`/`InferStringFormat`/`ToCamelCase`/`NsidToFilePath`/`ParseTypeValue` (→ `SplitRef`), `CSharpEmitter.Emit(LexiconDocument)`, `LexiconEmitter.JsonOptions`, the whole `Migrations/` folder; the three directory loaders, the two assembly→document loops and the duplicate options are now `Schema/LexiconLoader.cs` + `LexiconJson`. - **`null` def type** removed from the emitter's vocabulary (warns "removed from the Lexicon language"); **space** defs: `*`/non-NSID `collections` are left out of the emitted declaration with a warning (the emitted `Nsid.Parse` would throw); `name` length follows the proposal (no limit; the `SpaceTypeDeclaration.Name` doc's "1–64" was dropped). - **Tool now references `ATProtoNet`** (publish/resolve use it). To keep `lexicon`/`diff --assembly` binding the inspected assembly to its own dependencies, it is loaded into a collectible `AssemblyLoadContext` (`InspectionLoadContext`); framework assemblies stay shared so `JsonPropertyNameAttribute` matching still works. The packed tool contains `ATProtoNet.dll` and its 5 small dependencies. - **`Program.cs` testability** — `RunAsync(args, CommandContext)` with substitutable output, environment, secret prompt and network (`ILexgenNetwork`/`ILexiconRepository`); `Main` binds the console. ## Breaking changes (migration) - `atproto-lexgen publish` no longer copies to a directory; `--output`/`--baseline`/`--assembly`/`--no-bump` are gone and explain the new command. Migration: keep published copies in version control and use `diff --strict`; publish with `publish --input <dir> --identifier <handle>` + `ATPROTO_PASSWORD`. - `atproto-lexgen migrate` and the `ATProtoNet.LexiconGenerator.Migrations` types are removed. Migration: none needed under the evolution rules; gate changes with `diff --strict`. - Removed tool helpers: `CSharpEmitter.Emit(doc)` → `EmitAll([doc])`, `LexiconEmitter.JsonOptions` → `LexiconJson.WriteOptions`, `TypeMapper.ParseTypeValue` → `SplitRef`; the others had no callers. ## Verification - Unit tests: **4341 passed, 0 failed, 5 skipped** (`LexiconMigrationTests` and `LexiconPublisherTests` were deleted with the code they tested). Full solution build: **0 warnings, 0 errors**. - **Pinned reference vector** `tests/ATProtoNet.Tests/Lexicon/TestData/lexicon-resolution.json`, generated with `@atproto/repo` 0.10.14 / `@atproto/crypto` 0.5.5 and checked with `@atproto/lex-resolver` 0.3.0 (generator + README alongside, deterministic): a 44-record signed repo; `post` and `authBasic` resolve (URI/CID match), `mismatch` (id ≠ rkey), `badType` (wrong `$type`) and `missing` (absence proof) fail — the SDK agrees on all five, and rejects the proof under another signing key. - New tests: `LexiconResolverTests` (vector, DNS name/spec examples, no hierarchical walk, exactly-one-DID, DoH failure/disabled/configured endpoint, DID/PDS/HTTPS failures, RecordNotFound, timeout, SSRF refusal, permission set + include scope parsing, DI), `CachingLexiconResolverTests`, `LexiconClientTests`, `LexiconLinterTests` (spec example set clean + every rule), `CSharpEmitterTests` additions, and `LexiconGeneratorProgramTests` (66 in-process tests: every command's help/unknown option/happy/error paths; publish create/same/update/breaking/force/non-lexicon record/DNS statuses/lint refusal/password handling; resolve print/write/--did/failure). - **Live check** (not in CI): `atproto-lexgen resolve site.standard.document site.standard.authFull app.bsky.authFullApp chat.bsky.authFullChatClient` resolved and verified all four from the real network, a non-existent authority failed with `AuthorityNotFound`, and `lint` passed the four published sets with 0 errors/0 warnings. `dotnet pack` of the tool succeeds. ## Notes / deferred - Lexicon *document* validation is structural only (`$type`, version, `id`, `defs`); full lex-document schema validation (L4) is out of scope. - `LexiconDiffer` does not compare permission-set contents yet (added/removed permissions are not reported by `diff`/`publish`); a small follow-up. - `CLAUDE.md` still describes the tool as "…schema diffing, and migrations" — not edited here; worth updating. - Edits outside the Lexicon area: `Identity/HandleResolver.cs` (DoH code moved to `DnsTxtLookup.cs`), `Http/XrpcErrors.cs` (+1 constant), `AtProtoClient.cs` (+1 property), `Server/Extensions/IdentityServiceCollectionExtensions.cs` (+`ILexiconResolver`), `Spaces/SpaceTypeDeclaration.cs` (one doc line), and allow-list entries in `Conventions/*` and `LexiconDriftTests.Allowlists.cs`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-09-26 15:48:07 +00:00

feat(lexicon)!: lexicon resolution and publishing, permission sets in atproto-lexgen (closes #130) ... 837b3c0586

The SDK could not resolve a published Lexicon, and atproto-lexgen ignored
permission sets and "published" by copying files into a folder.

Runtime: LexiconResolver resolves an NSID as the Lexicon spec and
@atproto/lex-resolver do (the _lexicon.<authority> TXT record, exactly one
did=, no hierarchical walk; the DID's PDS and key; the schema record fetched
with sync.getRecord and verified against its commit proof), reusing the
identity resolver, its DNS-over-HTTPS lookup (now shared with
HandleResolver) and its SSRF policy. CachingLexiconResolver caches any
resolver, client.Lexicon wraps com.atproto.lexicon.resolveLexicon, and
ResolvePermissionSetAsync / ResolveIncludeScopeAsync check that an include:
scope's set exists. Pinned against a vector from the reference resolver.

atproto-lexgen: permission-set defs generate an NSID constant and an
Include(aud) helper; the new lint command checks the permission and Lexicon
rules; publish writes com.atproto.lexicon.schema records to a PDS (password
from the environment or a prompt, never an argument), refuses breaking
changes and reports the _lexicon DNS records; resolve fetches schemas.
migrate is removed (the evolution rules leave nothing to migrate), as are
the dead helpers, the duplicated loaders and options, and the null type.
Program.cs runs in process against a substitutable context and is tested.

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

opencode-bot commented 2026-09-26 15:48:08 +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-169/run-2026-09-26T15-48-08-539Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented 2026-09-26 15:48:10 +00:00

Collaborator

Copy link

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T20:46:56.945Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T20:46:56.945Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f2198f664ffehyYrbLC8fxkLiV -->

Grandiras commented 2026-09-26 15:48:18 +00:00

Author

Owner

Copy link

@claude /review — large; the description covers the CLI breaking changes and the pinned reference vector. The diff will be truncated, so please read these in full from the branch: src/ATProtoNet/Lexicon/Com/AtProto/Lexicon/LexiconResolver.cs (DNS _lexicon lookup with no hierarchical walk, DID to PDS, sync.getRecord with RecordProof.Verify, the NSID/id match, SSRF policy), CachingLexiconResolver.cs (TTL, negative caching, single-flight, bounds), LexiconModels.cs, src/ATProtoNet/Identity/DnsTxtLookup.cs plus the HandleResolver.cs change (behaviour must be unchanged), tools/ATProtoNet.LexiconGenerator/Validation/LexiconLinter.cs (the permission-set rules from the Lexicon spec), Publishing/LexiconPublisher.cs and Publishing/LexgenNetwork.cs (credential handling: env or no-echo prompt only, never echoed or logged), and Program.cs. Compare with @atproto/lex-resolver. Focus on resolution correctness and spoofing (a record served from the wrong repo, or an id or NSID mismatch), cache poisoning, credential leaks in the CLI, and whether the removed migrate or deleted helpers are still referenced by docs or code.

@claude /review — large; the description covers the CLI breaking changes and the pinned reference vector. The diff will be truncated, so please read these in full from the branch: `src/ATProtoNet/Lexicon/Com/AtProto/Lexicon/LexiconResolver.cs` (DNS `_lexicon` lookup with no hierarchical walk, DID to PDS, `sync.getRecord` with `RecordProof.Verify`, the NSID/`id` match, SSRF policy), `CachingLexiconResolver.cs` (TTL, negative caching, single-flight, bounds), `LexiconModels.cs`, `src/ATProtoNet/Identity/DnsTxtLookup.cs` plus the `HandleResolver.cs` change (behaviour must be unchanged), `tools/ATProtoNet.LexiconGenerator/Validation/LexiconLinter.cs` (the permission-set rules from the Lexicon spec), `Publishing/LexiconPublisher.cs` and `Publishing/LexgenNetwork.cs` (credential handling: env or no-echo prompt only, never echoed or logged), and `Program.cs`. Compare with `@atproto/lex-resolver`. Focus on resolution correctness and spoofing (a record served from the wrong repo, or an id or NSID mismatch), cache poisoning, credential leaks in the CLI, and whether the removed `migrate` or deleted helpers are still referenced by docs or code.

claude-bot commented 2026-09-26 15:48:23 +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/615); 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-169/run-615.md) — updating every ~5 minutes while the task runs.

Grandiras force-pushed issue-130-lexicon from 837b3c0586 to b47454c53a 2026-09-26 15:51:13 +00:00 Compare

claude-reviewer approved these changes 2026-09-26 15:51:40 +00:00

Dismissed

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Large, well-scoped PR implementing Lexicon resolution/publication and permission sets, plus a substantial atproto-lexgen rework. I read the full diff (not just the truncated PR text) via git diff main...origin/issue-130-lexicon, including the files cut off in the description: LexiconLinter.cs, LexiconPublisher.cs, LexgenNetwork.cs, and the relevant parts of Program.cs.

What I checked closely:

• LexiconResolver: DNS name derivation (_lexicon.<reversed-authority>), exactly-one-did= enforcement, PDS/signing-key lookup, HTTPS-unless-AllowPrivateNetworks gate, sync.getRecord + RecordProof verification, and the Validate checks (lexicon==1, id==nsid, non-empty defs). Matches the described spec behavior and the reference-vector tests (mismatch/badType/missing all map to the right LexiconResolutionErrorKind).
• CachingLexiconResolver: fresh/stale/expired/failure-TTL state machine, shared in-flight fetch with detach-on-invalidate, LRU eviction, and the "failed refresh keeps serving stale until expiry" behavior — traced through the logic and it's internally consistent (the RunFetchAsync failure branch correctly distinguishes "still-fresh entry, keep serving" from "no usable entry, store fresh failure").
• LexiconClient as an ILexiconResolver: correctly re-validates the service's answer (id == nsid) rather than trusting it blindly, and maps LexiconNotFound distinctly from other failures.
• LexiconLinter: permission-set namespace containment (CheckNsids via authority-prefix + dot boundary, avoiding the feed vs feed2 false-positive), wildcard/action/audience checks, and the null-type / space-collection warnings all look correct.
• LexiconPublisher / LexgenNetwork: the DNS authority check correctly translates LexiconResolutionErrorKind.AuthorityNotFound into null ("missing") rather than letting it propagate as a hard failure — that seam is exactly right for publish's DNS-status report. The whole-run breaking-change refusal (nothing written unless --force) is applied before any PutAsync call, so there's no partial-write case.
• Program.cs publish path: passwords are never accepted as an argument, only ATPROTO_PASSWORD or a non-echoing prompt — good security hygiene, matches the PR's own description.

Minor, non-blocking observations (not filed as line findings since they don't rise to correctness bugs):

• LexiconPublisher.PublishAsync reads the currently-published record (GetAsync) and later writes with swapRecord: current?.Cid; for a brand-new NSID current is null so the write goes through as an unconditional create, meaning a genuinely concurrent first-publish from another process could be silently overwritten (no swapRecord guard against "must not exist"). This is a narrow race in a single-operator CLI tool and consistent with how putRecord is used elsewhere in the codebase, so I wouldn't block on it.
• atproto-lexgen publish --pds accepts a plain http:// URL as well as https://, presumably for local/dev PDS instances; worth confirming that's intentional given the account password is sent to that endpoint during SignInAsync.

No correctness bugs found in the areas I was able to review in full. Given the scope (44 files, ~5300 insertions) I prioritized the security- and correctness-sensitive surfaces (resolver verification, caching semantics, permission-set validation, publish/sign-in flow) over the code-generation templating and the very large LexiconGeneratorProgramTests file, which I skimmed rather than read line-by-line — the tests it adds appear commensurate with the described behavior. I did not have network access to independently re-verify the pinned @atproto/lex-resolver reference vector, but the resolver logic matches the algorithm as documented in the PR and in docs/did-resolution.md.

---

⏱ 3m 4s · 23 turns · tokens: 120.2k in / 14.8k out (+2775.7k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Large, well-scoped PR implementing Lexicon resolution/publication and permission sets, plus a substantial `atproto-lexgen` rework. I read the full diff (not just the truncated PR text) via `git diff main...origin/issue-130-lexicon`, including the files cut off in the description: `LexiconLinter.cs`, `LexiconPublisher.cs`, `LexgenNetwork.cs`, and the relevant parts of `Program.cs`. What I checked closely: - `LexiconResolver`: DNS name derivation (`_lexicon.<reversed-authority>`), exactly-one-`did=` enforcement, PDS/signing-key lookup, HTTPS-unless-`AllowPrivateNetworks` gate, `sync.getRecord` + `RecordProof` verification, and the `Validate` checks (`lexicon==1`, `id==nsid`, non-empty `defs`). Matches the described spec behavior and the reference-vector tests (mismatch/badType/missing all map to the right `LexiconResolutionErrorKind`). - `CachingLexiconResolver`: fresh/stale/expired/failure-TTL state machine, shared in-flight fetch with detach-on-invalidate, LRU eviction, and the "failed refresh keeps serving stale until expiry" behavior — traced through the logic and it's internally consistent (the `RunFetchAsync` failure branch correctly distinguishes "still-fresh entry, keep serving" from "no usable entry, store fresh failure"). - `LexiconClient` as an `ILexiconResolver`: correctly re-validates the service's answer (`id == nsid`) rather than trusting it blindly, and maps `LexiconNotFound` distinctly from other failures. - `LexiconLinter`: permission-set namespace containment (`CheckNsids` via authority-prefix + dot boundary, avoiding the `feed` vs `feed2` false-positive), wildcard/action/audience checks, and the `null`-type / space-collection warnings all look correct. - `LexiconPublisher` / `LexgenNetwork`: the DNS authority check correctly translates `LexiconResolutionErrorKind.AuthorityNotFound` into `null` ("missing") rather than letting it propagate as a hard failure — that seam is exactly right for `publish`'s DNS-status report. The whole-run breaking-change refusal (nothing written unless `--force`) is applied before any `PutAsync` call, so there's no partial-write case. - `Program.cs` publish path: passwords are never accepted as an argument, only `ATPROTO_PASSWORD` or a non-echoing prompt — good security hygiene, matches the PR's own description. Minor, non-blocking observations (not filed as line findings since they don't rise to correctness bugs): - `LexiconPublisher.PublishAsync` reads the currently-published record (`GetAsync`) and later writes with `swapRecord: current?.Cid`; for a brand-new NSID `current` is null so the write goes through as an unconditional create, meaning a genuinely concurrent first-publish from another process could be silently overwritten (no `swapRecord` guard against "must not exist"). This is a narrow race in a single-operator CLI tool and consistent with how `putRecord` is used elsewhere in the codebase, so I wouldn't block on it. - `atproto-lexgen publish --pds` accepts a plain `http://` URL as well as `https://`, presumably for local/dev PDS instances; worth confirming that's intentional given the account password is sent to that endpoint during `SignInAsync`. No correctness bugs found in the areas I was able to review in full. Given the scope (44 files, ~5300 insertions) I prioritized the security- and correctness-sensitive surfaces (resolver verification, caching semantics, permission-set validation, publish/sign-in flow) over the code-generation templating and the very large `LexiconGeneratorProgramTests` file, which I skimmed rather than read line-by-line — the tests it adds appear commensurate with the described behavior. I did not have network access to independently re-verify the pinned `@atproto/lex-resolver` reference vector, but the resolver logic matches the algorithm as documented in the PR and in `docs/did-resolution.md`. --- ⏱ 3m 4s · 23 turns · tokens: 120.2k in / 14.8k out (+2775.7k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-169/run-615.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:837b3c058643fd13aa1168b9862ae213d92efcf6 -->

Grandiras commented 2026-09-26 15:54:21 +00:00

Author

Owner

Copy link

Addressed both review observations in 2c-commit fix(lexgen) on top of the approved patch:

• publish --pds (and a PDS looked up from the DID document) must be HTTPS; plain HTTP is only allowed on loopback, so the password is never sent in plaintext off-machine (SdkLexgenNetwork.MaySignInAt, with tests).
• A first publish now uses createRecord (which fails if the record appeared meanwhile) instead of an unguarded putRecord; replacements keep swapRecord.

@claude /review — please review the new commit on top of the previously approved one (the earlier patch is unchanged apart from a CHANGELOG-only rebase onto main, which verify-rebase confirmed byte-identical outside CHANGELOG.md).

Addressed both review observations in 2c-commit `fix(lexgen)` on top of the approved patch: - `publish --pds` (and a PDS looked up from the DID document) must be HTTPS; plain HTTP is only allowed on loopback, so the password is never sent in plaintext off-machine (`SdkLexgenNetwork.MaySignInAt`, with tests). - A first publish now uses `createRecord` (which fails if the record appeared meanwhile) instead of an unguarded `putRecord`; replacements keep `swapRecord`. @claude /review — please review the new commit on top of the previously approved one (the earlier patch is unchanged apart from a CHANGELOG-only rebase onto main, which `verify-rebase` confirmed byte-identical outside CHANGELOG.md).

Grandiras referenced this pull request from a commit 2026-09-26 15:54:21 +00:00

fix(lexgen): publish sends the password only over HTTPS and creates new schemas (#130)

Grandiras added 1 commit 2026-09-26 15:54:21 +00:00

fix(lexgen): publish sends the password only over HTTPS and creates new schemas (#130) ... e4863def29

Review follow-ups on #169. `publish --pds http://…` sent the account
password in plain text to any host; it now requires HTTPS, allowing
plain HTTP only on loopback for a local PDS, and holds a PDS looked up
from the DID document to the same rule. A schema published for the first
time is written with createRecord, so a concurrent first publish fails
instead of being silently overwritten; replacements keep swapRecord.

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

claude-bot commented 2026-09-26 15:54:24 +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/616); 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-169/run-616.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-09-26 15:55:20 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Re-review: one new commit (e4863de) landed since my last pass, and it is a direct, well-targeted follow-up to both non-blocking notes I raised previously:

1. Password-over-plain-HTTP — publish --pds http://… used to send the account password to any host that accepted an http(s) URL. Now SdkLexgenNetwork.MaySignInAt requires HTTPS, allowing plain HTTP only when Uri.IsLoopback is true (a local dev PDS), and the same rule is applied to a PDS discovered via DID-document lookup (ResolveAsync + MaySignInAt check before signing in), not just the --pds flag. New tests (Publish_PlainHttpPdsOffThisMachine_IsRefusedBeforeThePasswordIsSent, MaySignInAt_OnlyHttpsOrLoopbackHttp) cover the https/loopback-http/off-machine-http/ftp matrix, including verifying no sign-in attempt (_network.SignIns empty) occurs when refused.
2. First-publish race — PutAsync now branches on replacing: null (no previously published record) goes through CreateRecordAsync, which fails if a record already exists, instead of the previous unconditional PutRecordAsync/putRecord that could silently overwrite a concurrent first publish; an existing record still goes through PutRecordAsync with swapRecord, so replacement remains guarded.

CHANGELOG and docs/lexicon-codegen.md were updated in the same commit to describe both behaviors, consistent with the repo's changelog-in-same-commit convention. I diffed this commit in full and it's small, surgical, and doesn't touch any other resolver/linter/caching logic I reviewed previously (which I already verified carries over unchanged in this diff).

No remaining issues. Approving.

---

⏱ 0m 39s · 10 turns · tokens: 87.9k in / 2.4k out (+980.1k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: one new commit (e4863de) landed since my last pass, and it is a direct, well-targeted follow-up to both non-blocking notes I raised previously: 1. **Password-over-plain-HTTP** — `publish --pds http://…` used to send the account password to any host that accepted an http(s) URL. Now `SdkLexgenNetwork.MaySignInAt` requires HTTPS, allowing plain HTTP only when `Uri.IsLoopback` is true (a local dev PDS), and the same rule is applied to a PDS discovered via DID-document lookup (`ResolveAsync` + `MaySignInAt` check before signing in), not just the `--pds` flag. New tests (`Publish_PlainHttpPdsOffThisMachine_IsRefusedBeforeThePasswordIsSent`, `MaySignInAt_OnlyHttpsOrLoopbackHttp`) cover the https/loopback-http/off-machine-http/ftp matrix, including verifying no sign-in attempt (`_network.SignIns` empty) occurs when refused. 2. **First-publish race** — `PutAsync` now branches on `replacing`: `null` (no previously published record) goes through `CreateRecordAsync`, which fails if a record already exists, instead of the previous unconditional `PutRecordAsync`/`putRecord` that could silently overwrite a concurrent first publish; an existing record still goes through `PutRecordAsync` with `swapRecord`, so replacement remains guarded. CHANGELOG and `docs/lexicon-codegen.md` were updated in the same commit to describe both behaviors, consistent with the repo's changelog-in-same-commit convention. I diffed this commit in full and it's small, surgical, and doesn't touch any other resolver/linter/caching logic I reviewed previously (which I already verified carries over unchanged in this diff). No remaining issues. Approving. --- ⏱ 0m 39s · 10 turns · tokens: 87.9k in / 2.4k out (+980.1k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-169/run-616.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:e4863def29760b8d4f0fe24b8534006fee8767f9 -->

Grandiras merged commit ef506f15f1 into main 2026-09-26 15:55:40 +00:00

Grandiras referenced this pull request from a commit 2026-09-26 15:55:40 +00:00

feat(lexicon)!: lexicon resolution and publishing, permission sets in atproto-lexgen (#169)

Grandiras deleted branch issue-130-lexicon 2026-09-26 15:55:41 +00:00

Grandiras referenced this pull request 2026-09-26 16:41:08 +00:00

fix(spaces)!: protocol hardening, performance and a smaller server surface #172

Sign in to join this conversation.

Reviewers

No reviewers

claude-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

4 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!169

No description provided.