Claude: changes for issue #93 #97

Merged
Grandiras merged 2 commits from claude/issue-93 into main 2026-08-20 21:45:02 +00:00
Collaborator

Closes #93.

Closes #93.
test: integration tests for spaces against a real permissioned-data PDS
All checks were successful
CI / pds-integration (pull_request) Successful in 22s
CI / build-and-test (pull_request) Successful in 53s
c39d3731a9
#89 landed the permissioned data protocol with 212 unit tests, every one of
which stubbed the HTTP layer. That proves the SDK agrees with a reading of the
specification, not that a server accepts what it sends.

These 26 tests talk to a live space host, behind a [RequiresSpacesFact] gate
that skips unless ATPROTO_TEST_SPACES=true, so CI is unaffected:

- SpaceCredentialTests: the delegation-token -> credential exchange end to end,
  including that the server accepts the htu the SDK signs into its DPoP proof.
  Then the refusals that make a credential worth something - a replayed token,
  a token for another space, a proof from another key, a proof for another
  host, a bearer presentation, and SpaceDeleted on renewal after deleteSpace.
- SpaceRepoSyncTests: the CAR round trip, checking the SDK's LtHash, commit
  context encoding, MAC, and canonical DAG-CBOR ordering against a real
  implementation; then incremental sync over a real oplog, divergence
  detection, and the fallback when since is no longer serviceable.
- SimpleSpacePolicyTests: a non-member refused under member-list, an app
  refused under #allowList, revocation at the next renewal, and the repo
  boundary between two accounts on one host.

No PDS release serves com.atproto.space.*, so docs/testing-spaces.md documents
standing a host up from bluesky-social/atproto#5187 - which is what this was
verified against: 26 passed, and the whole suite skips with no environment.

closes #93
Author
Collaborator

🤖 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/324); 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-97/run-324.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Test/docs-only PR adding integration tests for the permissioned-data (space) protocol landed in #89, gated behind [RequiresSpacesFact]/ATPROTO_TEST_SPACES=true, plus docs/testing-spaces.md explaining how to stand up a dev-network host since no PDS release yet serves com.atproto.space.*.

I checked the test code against the actual production API surfaces it exercises (SpaceCredentialProvider, SpaceSyncer, SpaceRepoCursor, ISpaceRepoStore, SpaceRepoCar.Verify, PdsAdminClient, DidResolver/PlcClient, AtProtoCrypto.ImportCompressedPublicKey/ToDidKey) and constructor signatures, delegate shapes, and error constants (SpaceErrors.*) all line up correctly — nothing here looks like it would fail to compile or silently pass for the wrong reason.

SpaceNetworkFixture provisioning/teardown, the RequiresSpacesFactAttribute skip logic, and the TestConfig env-var wiring all mirror the existing AuthenticatedClientFixture / RequiresJetstreamFact / RequiresPdsAdminFact patterns already established in this test project, including the same

Emitted as JSON this job could not parse, so it is not line-anchored; the raw object follows.

{"verdict": "approve", "summary": "Test/docs-only PR adding integration tests for the permissioned-data (space) protocol landed in #89, gated behind `[RequiresSpacesFact]`/`ATPROTO_TEST_SPACES=true`, plus `docs/testing-spaces.md` explaining how to stand up a dev-network host since no PDS release yet serves `com.atproto.space.*`.\n\nI checked the test code against the actual production API surfaces it exercises (`SpaceCredentialProvider`, `SpaceSyncer`, `SpaceRepoCursor`, `ISpaceRepoStore`, `SpaceRepoCar.Verify`, `PdsAdminClient`, `DidResolver`/`PlcClient`, `AtProtoCrypto.ImportCompressedPublicKey`/`ToDidKey`) and constructor signatures, delegate shapes, and error constants (`SpaceErrors.*`) all line up correctly — nothing here looks like it would fail to compile or silently pass for the wrong reason.\n\n`SpaceNetworkFixture` provisioning/teardown, the `RequiresSpacesFactAttribute` skip logic, and the `TestConfig` env-var wiring all mirror the existing `AuthenticatedClientFixture` / `RequiresJetstreamFact` / `RequiresPdsAdminFact` patterns already established in this test project, including the same "fixture always initializes even for skipped tests" characteristic that the codebase already accepts elsewhere (integration tests never run in CI, only the unit-test project does). The base58 decode / secp256k1 point-compression helper for the dev network's legacy verification-method format looks correct, and the negative-path tests (replayed delegation token, wrong-space token, wrong-key proof, wrong-host `htu`, bearer-vs-DPoP, post-deletion) are exactly the assertions the PR description claims are only reachable against a live server.\n\nCHANGELOG, CLAUDE.md, and docs cross-links were all updated appropriately in the same commit per the repo's contribution rules. No issues found — good to merge.", "findings": []}

⏱ 2m 20s · 22 turns · tokens: 63.5k in / 10.9k out (+1564.6k cache-read)
📜 Full session transcript

Test/docs-only PR adding integration tests for the permissioned-data (space) protocol landed in #89, gated behind `[RequiresSpacesFact]`/`ATPROTO_TEST_SPACES=true`, plus `docs/testing-spaces.md` explaining how to stand up a dev-network host since no PDS release yet serves `com.atproto.space.*`. I checked the test code against the actual production API surfaces it exercises (`SpaceCredentialProvider`, `SpaceSyncer`, `SpaceRepoCursor`, `ISpaceRepoStore`, `SpaceRepoCar.Verify`, `PdsAdminClient`, `DidResolver`/`PlcClient`, `AtProtoCrypto.ImportCompressedPublicKey`/`ToDidKey`) and constructor signatures, delegate shapes, and error constants (`SpaceErrors.*`) all line up correctly — nothing here looks like it would fail to compile or silently pass for the wrong reason. `SpaceNetworkFixture` provisioning/teardown, the `RequiresSpacesFactAttribute` skip logic, and the `TestConfig` env-var wiring all mirror the existing `AuthenticatedClientFixture` / `RequiresJetstreamFact` / `RequiresPdsAdminFact` patterns already established in this test project, including the same _Emitted as JSON this job could not parse, so it is not line-anchored; the raw object follows._ ```json {"verdict": "approve", "summary": "Test/docs-only PR adding integration tests for the permissioned-data (space) protocol landed in #89, gated behind `[RequiresSpacesFact]`/`ATPROTO_TEST_SPACES=true`, plus `docs/testing-spaces.md` explaining how to stand up a dev-network host since no PDS release yet serves `com.atproto.space.*`.\n\nI checked the test code against the actual production API surfaces it exercises (`SpaceCredentialProvider`, `SpaceSyncer`, `SpaceRepoCursor`, `ISpaceRepoStore`, `SpaceRepoCar.Verify`, `PdsAdminClient`, `DidResolver`/`PlcClient`, `AtProtoCrypto.ImportCompressedPublicKey`/`ToDidKey`) and constructor signatures, delegate shapes, and error constants (`SpaceErrors.*`) all line up correctly — nothing here looks like it would fail to compile or silently pass for the wrong reason.\n\n`SpaceNetworkFixture` provisioning/teardown, the `RequiresSpacesFactAttribute` skip logic, and the `TestConfig` env-var wiring all mirror the existing `AuthenticatedClientFixture` / `RequiresJetstreamFact` / `RequiresPdsAdminFact` patterns already established in this test project, including the same "fixture always initializes even for skipped tests" characteristic that the codebase already accepts elsewhere (integration tests never run in CI, only the unit-test project does). The base58 decode / secp256k1 point-compression helper for the dev network's legacy verification-method format looks correct, and the negative-path tests (replayed delegation token, wrong-space token, wrong-key proof, wrong-host `htu`, bearer-vs-DPoP, post-deletion) are exactly the assertions the PR description claims are only reachable against a live server.\n\nCHANGELOG, CLAUDE.md, and docs cross-links were all updated appropriately in the same commit per the repo's contribution rules. No issues found — good to merge.", "findings": []} ``` --- ⏱ 2m 20s · 22 turns · tokens: 63.5k in / 10.9k out (+1564.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-97/run-324.md) <!-- claude-reviewed-head:c39d3731a9b5e63e3e62214efb0e56c4ed64cc47 -->
Author
Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

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

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/325); the reply will link the full session transcript. <!-- claude-run:comment --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-97/run-325.md) — updating every ~5 minutes while the task runs.
Author
Collaborator

The review is an approval with an empty findings array — there is nothing in it to act on. The prose reads as a verification report ("No issues found — good to merge"), not as a list of requested changes, so I made no code changes and pushed nothing. claude/issue-93 still points at c39d373, the same commit the reviewer looked at.

What I did do was re-verify the branch independently, since a test-only PR that never runs in CI is exactly the kind that can rot without anyone noticing:

  • dotnet build tests/ATProtoNet.IntegrationTests/ -p:EnableSourceControlManagerQueries=false — succeeded, 0 warnings, 0 errors. This is the meaningful check for this PR: the new SpaceCredentialTests, SpaceRepoSyncTests, SimpleSpacePolicyTests and SpaceNetworkFixture compile against the current com.atproto.space.* production surface, so the reviewer's claim that the constructor signatures, delegate shapes and SpaceErrors.* constants line up is confirmed by the compiler and not just by reading.
  • dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false1511 passed, 0 failed, 5 skipped (the skips are the pre-existing Aspire manifest tests). Unchanged by this PR, as expected for a branch that only touches the integration project and docs.

The new tests themselves stay skipped without ATPROTO_TEST_SPACES=true and a dev-network host, which is the point of the gating — docs/testing-spaces.md on this branch covers standing one up. I did not try to provision that here, so the space tests remain unexercised end-to-end; that limitation is inherent to the PR and was already stated in its description.

No follow-up issues filed — nothing surfaced that belongs in one.


⏱ 1m 3s · 4 turns · tokens: 14.4k in / 1.4k out (+103.0k cache-read)
📜 Full session transcript

The review is an approval with an empty `findings` array — there is nothing in it to act on. The prose reads as a verification report ("No issues found — good to merge"), not as a list of requested changes, so I made no code changes and pushed nothing. `claude/issue-93` still points at `c39d373`, the same commit the reviewer looked at. What I did do was re-verify the branch independently, since a test-only PR that never runs in CI is exactly the kind that can rot without anyone noticing: - `dotnet build tests/ATProtoNet.IntegrationTests/ -p:EnableSourceControlManagerQueries=false` — succeeded, **0 warnings, 0 errors**. This is the meaningful check for this PR: the new `SpaceCredentialTests`, `SpaceRepoSyncTests`, `SimpleSpacePolicyTests` and `SpaceNetworkFixture` compile against the current `com.atproto.space.*` production surface, so the reviewer's claim that the constructor signatures, delegate shapes and `SpaceErrors.*` constants line up is confirmed by the compiler and not just by reading. - `dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false` — **1511 passed, 0 failed, 5 skipped** (the skips are the pre-existing Aspire manifest tests). Unchanged by this PR, as expected for a branch that only touches the integration project and docs. The new tests themselves stay skipped without `ATPROTO_TEST_SPACES=true` and a dev-network host, which is the point of the gating — `docs/testing-spaces.md` on this branch covers standing one up. I did not try to provision that here, so the space tests remain unexercised end-to-end; that limitation is inherent to the PR and was already stated in its description. No follow-up issues filed — nothing surfaced that belongs in one. --- ⏱ 1m 3s · 4 turns · tokens: 14.4k in / 1.4k out (+103.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-97/run-325.md)
Owner

@claude-bot Please fix the merge conflict

@claude-bot Please fix the merge conflict
Author
Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

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

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/327); the reply will link the full session transcript. <!-- claude-run:comment --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-97/run-327.md) — updating every ~5 minutes while the task runs.
Merge main into claude/issue-93
All checks were successful
CI / pds-integration (pull_request) Successful in 26s
CI / build-and-test (pull_request) Successful in 56s
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
679124967b
Resolves the CHANGELOG.md conflict: main's `atproto-lexgen` space-type
entry (issue #92) and this branch's space integration-test entry
(issue #93) both landed at the top of `### Added`, so both are kept,
newest first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Collaborator

Merged main into claude/issue-93 and pushed — the PR is conflict-free now. New head is 6791249.

The conflict was one hunk in CHANGELOG.md. Both sides inserted a new bullet at the top of ### Added on the same line: main got the atproto-lexgen space-type entry from #96 (issue #92), this branch has the space integration-test entry (issue #93). Nothing was dropped — I kept both, newest-merging first (#93, then #92, then the existing #89 spaces entry below), matching how main has been stacking entries.

Everything else auto-merged. The two branches barely overlap: main touched tools/ATProtoNet.LexiconGenerator/*, AtProtoScopes.cs and the unit-test project; this branch touched only tests/ATProtoNet.IntegrationTests/* and docs. docs/spaces.md merged cleanly (main didn't touch it; the +5 lines are this branch's cross-link to testing-spaces.md).

One interaction worth checking, since main's #94 change makes AtProtoScopes.Repo(..., RepoAction.None) throw rather than silently grant full write: the new integration tests don't call it — grep -rn "RepoAction.None" tests/ATProtoNet.IntegrationTests/ returns nothing — so the merge doesn't turn any of the new tests into a runtime throw.

Verified on the merged tree:

  • dotnet build -p:EnableSourceControlManagerQueries=false0 warnings, 0 errors across the whole solution, integration-test project included. That's the check that matters here: the space tests still compile against main's post-#92/#94 surface.
  • dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false1531 passed, 0 failed, 5 skipped. Up from the 1511 I reported before the merge; the extra 20 are main's new lexgen tests, and the 5 skips are the pre-existing Aspire manifest ones.

The space tests themselves remain skipped without ATPROTO_TEST_SPACES=true and a dev-network host, unchanged by this merge — I didn't provision one, so they're still unexercised end-to-end here, exactly as the PR description states.

No new issues filed — nothing surfaced that belongs in one.


⏱ 1m 43s · 13 turns · tokens: 20.1k in / 3.4k out (+425.3k cache-read)
📜 Full session transcript

Merged `main` into `claude/issue-93` and pushed — the PR is conflict-free now. New head is `6791249`. **The conflict was one hunk in `CHANGELOG.md`.** Both sides inserted a new bullet at the top of `### Added` on the same line: main got the `atproto-lexgen` space-type entry from #96 (issue #92), this branch has the space integration-test entry (issue #93). Nothing was dropped — I kept both, newest-merging first (#93, then #92, then the existing #89 spaces entry below), matching how main has been stacking entries. Everything else auto-merged. The two branches barely overlap: main touched `tools/ATProtoNet.LexiconGenerator/*`, `AtProtoScopes.cs` and the unit-test project; this branch touched only `tests/ATProtoNet.IntegrationTests/*` and docs. `docs/spaces.md` merged cleanly (main didn't touch it; the +5 lines are this branch's cross-link to `testing-spaces.md`). One interaction worth checking, since main's #94 change makes `AtProtoScopes.Repo(..., RepoAction.None)` throw rather than silently grant full write: the new integration tests don't call it — `grep -rn "RepoAction.None" tests/ATProtoNet.IntegrationTests/` returns nothing — so the merge doesn't turn any of the new tests into a runtime throw. Verified on the merged tree: - `dotnet build -p:EnableSourceControlManagerQueries=false` — **0 warnings, 0 errors** across the whole solution, integration-test project included. That's the check that matters here: the space tests still compile against main's post-#92/#94 surface. - `dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false` — **1531 passed, 0 failed, 5 skipped**. Up from the 1511 I reported before the merge; the extra 20 are main's new lexgen tests, and the 5 skips are the pre-existing Aspire manifest ones. The space tests themselves remain skipped without `ATPROTO_TEST_SPACES=true` and a dev-network host, unchanged by this merge — I didn't provision one, so they're still unexercised end-to-end here, exactly as the PR description states. No new issues filed — nothing surfaced that belongs in one. --- ⏱ 1m 43s · 13 turns · tokens: 20.1k in / 3.4k out (+425.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-97/run-327.md)
Author
Collaborator

🤖 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/328); 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-97/run-328.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Verdict: approve — no blocking issues found.

Re-checked this PR's diff against the current source. Re-verified the production API surfaces the new integration tests call — PdsAdminClient (constructor overloads, CreateAccountAsync/DeleteAccountAsync/DescribeServerAsync, PdsAdminOptions.AllowInsecureHttp), SpaceCredentialProvider's constructor parameter order (client, options, httpClient, didResolver, logger), SpaceSyncer's constructor and SyncRepoAsync/RecoverAsync, SpaceRepoCursor's two constructors and Rev/Commit members, ISpaceRepoStore, and SpaceSyncOutcome — all line up exactly with how the tests use them. IAsyncLifetime.InitializeAsync/DisposeAsync return ValueTask, consistent with xunit.v3 (confirmed via the package reference and the existing AuthenticatedClientFixture), so SpaceNetworkFixture implementing the interface the same way is correct, not a bug.

Nothing here would fail to compile, silently pass for the wrong reason, or regress CI (still fully gated behind [RequiresSpacesFact]/ATPROTO_TEST_SPACES=true, which nothing in this diff touches). CHANGELOG, CLAUDE.md, and docs cross-links are all updated in the same commit as required. No issues found; good to merge.


⏱ 0m 36s · 8 turns · tokens: 47.1k in / 3.0k out (+519.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-checked this PR's diff against the current source. Re-verified the production API surfaces the new integration tests call — `PdsAdminClient` (constructor overloads, `CreateAccountAsync`/`DeleteAccountAsync`/`DescribeServerAsync`, `PdsAdminOptions.AllowInsecureHttp`), `SpaceCredentialProvider`'s constructor parameter order (`client, options, httpClient, didResolver, logger`), `SpaceSyncer`'s constructor and `SyncRepoAsync`/`RecoverAsync`, `SpaceRepoCursor`'s two constructors and `Rev`/`Commit` members, `ISpaceRepoStore`, and `SpaceSyncOutcome` — all line up exactly with how the tests use them. `IAsyncLifetime.InitializeAsync`/`DisposeAsync` return `ValueTask`, consistent with xunit.v3 (confirmed via the package reference and the existing `AuthenticatedClientFixture`), so `SpaceNetworkFixture` implementing the interface the same way is correct, not a bug. Nothing here would fail to compile, silently pass for the wrong reason, or regress CI (still fully gated behind `[RequiresSpacesFact]`/`ATPROTO_TEST_SPACES=true`, which nothing in this diff touches). CHANGELOG, CLAUDE.md, and docs cross-links are all updated in the same commit as required. No issues found; good to merge. --- ⏱ 0m 36s · 8 turns · tokens: 47.1k in / 3.0k out (+519.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-97/run-328.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:679124967bf9919dc374643aa76d414df9b58f6e -->
Grandiras deleted branch claude/issue-93 2026-08-20 21:45:02 +00:00
Grandiras referenced this pull request from a commit 2026-08-20 21:45:02 +00:00
Sign in to join this conversation.
No description provided.