chore: ConfigureAwait, comment compaction and public API tracking #177
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!177
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-139-chore"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #139.
Four mechanical, behavior-preserving passes over
src/(library projects only —tests/andsamples/are untouched except a couple of trivialusingremovals noted below).1.
ConfigureAwait(false)on every library awaitsrc/.editorconfigenablesCA2007(dotnet_diagnostic.CA2007.severity = error), andsrc/Directory.Build.propsaddsCA2007toWarningsAsErrorsalongsideCS1591, so a missedConfigureAwait(false)fails the build the same way an undocumented public member does.dotnet format analyzers --diagnostics CA2007per project, then hand-reviewed. ~518 call sites gained.ConfigureAwait(false), coveringawait,await foreach(on the enumerable) andawait using(on the disposable).ATProtoNet.Blazorcomponents are untouched.ComposePost,FeedView,LoginForm,PostCardandProfileCardlive in.razorfiles and callStateHasChanged-equivalent UI updates after their awaits; CA2007 does not fire on Razor's generated code (confirmed empirically — the analyzer skips it), so no config was needed to exempt them.AtProtoUserClientAccessor, the project's one non-component service with awaits, tookConfigureAwait(false)throughout: nothing after its awaits touches circuit state.await using var x = await Expr();, the automated fixer configures both the inner task await and the outer implicitDisposeAsyncawait by wrapping the whole initializer a second time — which changesx's declared type toConfiguredAsyncDisposable, breaking any later use of its real members (9 compile errors surfaced this inATProtoNet/ATProtoNet.Server, another 31 in EF Core'sCreateDbContextAsynccall sites). Fixed by keepingConfigureAwait(false)only on the explicit task await and suppressing CA2007 for the now-unconfigured disposal with a one-line-reasoned#pragma warning disable/restore CA2007(35 sites total). This is a legitimate C# limitation (there is no way to configure a disposal-only await without changing the declared type in that combined statement form), not a policy choice.2. Comment compaction
/// <summary>blocks (single-sentence body, ~120–130 chars) onto one line, across 275+ files.// ── Title ──────...), same total width as the original box, across 41 files.<param>tags that only restated the parameter's name in words (e.g.<param name="cancellationToken">Cancellation token.</param>,<param name="space">The space.</param>), determined algorithmically (description, minus a leading article and trailing period, equals the parameter name split into words) so only true tautologies were removed — anything carrying real information ("The DID of the repository.","Cancellation token to stop consuming.") was left alone.src/Directory.Build.propsaddsNoWarn CS1573, since dropping some but not all of a member's<param>tags would otherwise warn.previously…,used to be…,was changed…) and obvious-code narration: none found. The codebase already followedCLAUDE.md's existing comment conventions.3. Public API tracking
Microsoft.CodeAnalysis.PublicApiAnalyzers(5.6.0) tosrc/Directory.Build.props, scoped to packable projects,PrivateAssets="all"(confirmed it does not leak into the published.nuspec).PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtadded to all five packable projects:ATProtoNet(8,966 lines),ATProtoNet.Server(630),ATProtoNet.Server.EntityFrameworkCore(155),ATProtoNet.Blazor(165, including Razor components'[Parameter]properties and lifecycle overrides — see below),ATProtoNet.Aspire.Hosting(62).Shipped.txtis the pre-0.7.0 baseline (generated via the analyzer's own RS0016 code fix);Unshipped.txtis empty.RS0016(undeclared public member) andRS0017(removed public member) are promoted to build errors inWarningsAsErrors, next toCS1591/CA2007.RS0026/RS0027(the "overloads with optional parameters" backcompat lint, also shipped in this package) are silenced: the SDK already has many such overloads by design (a trailingCancellationToken, mostly), and redesigning that is unrelated to this issue.ATProtoNet.Blazor's Razor components are genuinely tracked, unlike with CA2007. RS0016 does not skip Razor-generated code, and correctly so — a component's[Parameter]properties and event callbacks are real public surface.dotnet format analyzersitself can't reach that generated code (it only edits physical source files), so those 94 symbols were added toPublicAPI.Shipped.txtby hand from the build's own error list. The generatedBuildRenderTreeoverride also reportsRS0041("oblivious reference types"); an.editorconfigseverity override can't reach it either (its diagnostics are keyed to theobj/source-generator output path, not the source tree's.editorconfighierarchy), so it's suppressed with<NoWarn>RS0041</NoWarn>directly inATProtoNet.Blazor.csproj.tools/ATProtoNet.LexiconGeneratordeliberately has noPublicAPI.*.txt. It packs as adotnet tool(PackAsTool), consumed through its CLI (atproto-lexgen), not by referencing its assembly as a library — there is no consumer contract for RS0016/RS0017 to protect, even though the assembly does have many public types.CONTRIBUTING.mdgains a short "Public API tracking" section describing the Shipped/Unshipped workflow (adding API → Unshipped; a release moves Unshipped → Shipped).4. Unused
usingdirectivesIDE0005at warning severity insrc/.editorconfigand randotnet format style --diagnostics IDE0005across everysrc/project, theLexiconGeneratortool, and thesamples/projects not owned by the parallel test-suite work. The codebase was already close to clean: 9 staleusings removed across 9 files (one each).tests/**was left alone entirely, per this issue's scope-discipline note (the parallel #137 test-suite agent is editing many test files there).Breaking changes
None. All four passes are behavior-preserving by construction (comment/whitespace/analyzer-config only, plus
.ConfigureAwait(false)which does not change return values or exception behavior — only which context a continuation resumes on).Verified
dotnet build -p:EnableSourceControlManagerQueries=false): 0 warnings, 0 errors.dotnet test tests/ATProtoNet.Tests/: 5,045 passed, 5 skipped, 0 failed — run twice, identical both times.dotnet pack -c Releasesucceeds for all five packable projects (ATProtoNet,ATProtoNet.Server,ATProtoNet.Server.EntityFrameworkCore,ATProtoNet.Blazor,ATProtoNet.Aspire.Hosting).Microsoft.CodeAnalysis.PublicApiAnalyzersdoes not appear as a dependency in the packed.nuspec(PrivateAssets="all"works as intended).src/is a//////comment line, a blank line, or the newNoWarnentry's own explanatory<!-- -->comment inDirectory.Build.props— no code line changed.Deferred / flagged
tests/**andsamples/**usings beyond the ones already clean were not swept exhaustively, per scope discipline around the parallel #137 (tests) work.ATProtoNet.Blazor._Imports(a public class Razor generates from_Imports.razor, which carries no real API). This reflects the actual current surface rather than something introduced by this issue; tightening component visibility (e.g. via code-behind + explicit access modifiers) is a separate, non-mechanical change outside this issue's scope.🤖 Generated with Claude Code
Adds Microsoft.CodeAnalysis.PublicApiAnalyzers to every packable project (ATProtoNet, ATProtoNet.Server, ATProtoNet.Server.EntityFrameworkCore, ATProtoNet.Blazor, ATProtoNet.Aspire.Hosting) via src/Directory.Build.props, with PublicAPI.Shipped.txt seeded from the analyzer's own RS0016 code fix as the pre-0.7.0 baseline and PublicAPI.Unshipped.txt left empty. RS0016 (undeclared public member) and RS0017 (removed public member) are promoted to build errors alongside CS1591 and CA2007, so a future API change that isn't reflected in Unshipped.txt fails the build. RS0026/RS0027 (overloads-with-optional-parameters backcompat lint) are silenced: the SDK already has many such overloads by design (a trailing CancellationToken, mostly), and redesigning that is unrelated to this issue's public-API-tracking goal. The tool project, tools/ATProtoNet.LexiconGenerator, is deliberately left out: it packs as a dotnet tool (PackAsTool), consumed by its CLI surface, not by referencing its assembly, so there is no consumer contract for RS0016/RS0017 to protect. ATProtoNet.Blazor's components are Razor-generated code, which this analyzer (unlike CA2007) does not skip: their [Parameter] properties, event callbacks and lifecycle overrides are genuinely part of the package's public surface and are included in the baseline. The generated BuildRenderTree override reports RS0041 ("oblivious reference types") that an .editorconfig severity override cannot reach (its diagnostics are keyed to the obj/ source-generator output, not the .editorconfig hierarchy under the source tree), so it is suppressed with <NoWarn> in the project file instead. CONTRIBUTING.md gains a short "Public API tracking" section describing the Shipped/Unshipped workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>Mechanical pass over src/ comments, no behavior change: - Collapse a three-line /// <summary> block onto one line when its body is a single sentence that fits in ~120-130 chars (1,679 blocks across 275 files). - Collapse three-line box-drawing section dividers into the one-line style already used elsewhere in the codebase, same total width (193 blocks across 41 files). - Drop <param> tags that only restate the parameter's name in words (e.g. "Cancellation token." on `cancellationToken`, "The space." on `space`) - 683 tags across roughly 100 files. NoWarn CS1573 in src/Directory.Build.props, since dropping some but not all of a member's <param> tags would otherwise warn. Grepped for history-style comments ("previously…", "used to be…", "was changed…") and obvious-code narration: none were found: the codebase already followed CLAUDE.md's existing comment conventions before this pass. Nothing was reflowed or reworded. Verified: every changed line in the src/ diff is a /// or // comment line (or the new NoWarn entry's own explanatory comment); full solution build is 0 warnings/0 errors; unit tests unaffected (5045 passed, 5 skipped, run twice). No CHANGELOG entry: comment-only, no user-visible or API effect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>Mechanical-diff verification (orchestrator, scripted over
git show -U0of each commit after the rebase onto main):6f6281c), 70 .cs files. With whitespace normalised,.ConfigureAwait(false)stripped and#pragmalines ignored, every file's removed code equals its added code, so there are no other code changes.448261c). Onlyusinglines removed, nothing added, plus the IDE0005 line insrc/.editorconfig.50edefa). Every changed line in a .cs file is a//or///line or blank. The only non-.cs change isNoWarn CS1573insrc/Directory.Build.props.e43bd4e). Touches onlyPublicAPI.*.txt,Directory.Build.props,.editorconfig, the Blazor csproj (RS0041 NoWarn), CONTRIBUTING and CHANGELOG.--no-incremental) has 0 warnings and 0 errors, 5,058 unit tests pass, anddotnet packproduces all 6 packages.@claude /review. The diff is huge but mechanical (see the verification above), so please don't try to read all of it. Focus on:
src/.editorconfigandsrc/Directory.Build.props: the CA2007, IDE0005 and RS0016/RS0017 severities are scoped tosrc/only and don't leak into tests or samples; PublicApiAnalyzers isPrivateAssets=alland absent from the nuspec.await using var x = await …sites that carry a#pragma warning disable CA2007with a reason. Is each justified?ATProtoNet.Blazor: the Razor components are untouched, andAtProtoUserClientAccessor'sConfigureAwait(false)is safe because nothing after an await needs the circuit's context.🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
🤖 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.
⏳ 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-27T04:03:28.622Z) — nothing to re-trigger.
💬 Verdict: comment — notes only, nothing blocking.
This is a large, mechanical, well-tested pass (ConfigureAwait, comment compaction, public API tracking, unused usings) and the diff is consistent with what the description promises: ConfigureAwait(false) is applied correctly (including the documented
await usingcombined-statement workaround, which correctly configures only the explicit task await and leaves the implicit disposal await under a scoped#pragma warning disable/restore CA2007, so the declared type of the disposable is preserved), the dropped<param>tags all look like genuine restatement-of-the-name tautologies, and the PublicAPI.Shipped.txt additions look mechanically generated. No behavior changes are visible in the diff.One real issue:
CHANGELOG.mdnow has two near-duplicate bullets about the same[RequiresFact]/[RequiresTheory]enum-driven attribute change under### Changed— the pre-existing one (tagged#137) and a new one this PR adds (also tagged#137, not#139) that just adds the[RequiresTheory(...)]mention. That entry describes work from a different, already-merged issue and doesn't belong to this PR's own change set; it reads like a stray artifact from a branch merge/rebase rather than an intentional edit. Worth squashing into the original bullet (or dropping) before merge, per this repo's own changelog-hygiene rule in CLAUDE.md.⏱ 0m 57s · 1 turns · tokens: 82.2k in / 5.0k out (+23.9k cache-read)
📜 Full session transcript
@ -329,6 +330,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0- **The client factory's per-user clients take `UserAgent` and `RateLimit` from `AddAtProto(o => …)`** — they were created with default options whatever the registration said. The other client options describe the registered client's own session and do not apply to them: each addresses its user's PDS and refreshes on demand under the factory's coordinator (#136)- **One enum-driven `[RequiresFact]` gates every environment-dependent integration test** — `RequiresPdsFactAttribute`, `RequiresBlueskyFactAttribute`, `RequiresPdsAdminFactAttribute`, `RequiresJetstreamFactAttribute`, `RequiresJetstreamArchiveFactAttribute` and `RequiresSpacesFactAttribute` are replaced by `[RequiresFact(IntegrationRequirement.X)]`; `RequiresRedisFactAttribute` is unchanged. `ATPROTO_REQUIRE_INTEGRATION=1` now also covers the PDS and Bluesky requirements, which previously skipped silently instead of failing when their prerequisites were missing. Migration: replace the old attribute names with `[RequiresFact(IntegrationRequirement.<Name>)]` (#137)- **`AuthenticatedClientFixture` no longer needs a pre-existing account** — like `SpaceNetworkFixture`, it now provisions and tears down its own throwaway account through the admin API, gated on `ATPROTO_PDS_ADMIN_PASSWORD` instead of `ATPROTO_TEST_HANDLE` / `ATPROTO_TEST_PASSWORD`. Setting both env vars still overrides it, signing in with that account instead. The CI PDS-integration job now also runs `AuthenticationTests`, `RepositoryTests`, `CustomRecordTests` and `IdentityResolutionTests` against the reference PDS on every PR, not just `PdsAdminTests` (#137)- **One enum-driven `[RequiresFact]` / `[RequiresTheory]` gates every environment-dependent integration test** — `RequiresPdsFactAttribute`, `RequiresBlueskyFactAttribute`, `RequiresPdsAdminFactAttribute`, `RequiresJetstreamFactAttribute`, `RequiresJetstreamArchiveFactAttribute` and `RequiresSpacesFactAttribute` are replaced by `[RequiresFact(IntegrationRequirement.X)]` (and a `[RequiresTheory(...)]` counterpart); `RequiresRedisFactAttribute` is unchanged. `ATPROTO_REQUIRE_INTEGRATION=1` now also covers the PDS and Bluesky requirements, which previously skipped silently instead of failing when their prerequisites were missing. Migration: replace the old attribute names with `[RequiresFact(IntegrationRequirement.<Name>)]` (#137)This bullet duplicates the existing
[RequiresFact]/[RequiresTheory]entry a few lines above (also under### Changed, tagged#137). Both describe the same attribute consolidation; this one only adds the[RequiresTheory(...)]mention and is tagged#137, not#139, even though it's being introduced by the #139 PR. This looks like a merge artifact rather than an intentional change belonging to this PR's scope (ConfigureAwait/comments/PublicAPI/usings) — worth merging into the original bullet or dropping.🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
⏳ 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-27T09:03:59.073Z) — nothing to re-trigger.
🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
⏳ 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-27T14:04:29.524Z) — nothing to re-trigger.
@claude /review, please re-review. The duplicated #137 bullet you flagged was a rebase artifact: my CHANGELOG merge kept the pre-#176 wording alongside the version #176 amended.
6d552bbremoves it, so the branch's[Unreleased]now differs from main only by this PR's own two bullets, which I checked with a script. The merge script now drops a stale copy of any bullet that upstream has edited. Nothing else changed since50edefa.🤖 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 — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review runs.
⏳ 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-27T15:53:37.817Z) — nothing to re-trigger.
✅ Verdict: approve — no blocking issues found.
Re-review confirms the one blocking issue from my last pass — the duplicate
[RequiresFact]/[RequiresTheory]CHANGELOG bullet under### Changed(a rebase artifact from #137/#176) — is now fixed by a dedicated follow-up commit (6d552bb, "docs(changelog): drop a #137 bullet duplicated by the rebase"), which removes the stray duplicate and keeps the single correct bullet plus this PR's own newConfigureAwaitbullet. I diffed that commit directly and confirmed the current CHANGELOG.md has no duplication left.The remaining four commits (ConfigureAwait pass, comment compaction, PublicAPI tracking, unused usings) are unchanged from my prior pass, where I already verified: correct handling of the
await usingcombined-statement CA2007 fixer bug (explicit task await configured, implicit disposal await left under a scoped#pragma warning disable/restore CA2007, preserving the disposable's declared type), genuine tautology-only<param>removals, and mechanically-generatedPublicAPI.Shipped.txtadditions. No behavior changes are visible in the diff, consistent with the PR's own claim of being purely mechanical.No new issues found. Approving.
⏱ 0m 48s · 13 turns · tokens: 94.1k in / 3.2k out (+1337.6k cache-read)
📜 Full session transcript