agui-dotnet-code-review

1---2name: agui-dotnet-code-review3description: >4  Review C#/.NET code changes to the AG-UI .NET SDK (sdks/dotnet/) against its5  specific conventions and architectural rules — AOT serialization, the6  "no ASP.NET in src/" boundary, the PublicAPI analyzer workflow, wire7  compatibility with the TypeScript reference, and the house style8  (sealed/no-records/ConfigureAwait). Runs a phased, rule-by-rule review.9  USE FOR: reviewing a PR, diff, or branch that touches sdks/dotnet/; checking a10  new event/message type; verifying serialization, package placement, or11  public-API changes in the .NET SDK. DO NOT USE FOR: generic C# style nits12  already enforced by analyzers/EditorConfig; reviewing the TypeScript SDK13  (sdks/typescript/) or Python SDK (sdks/python/); writing new features (only14  flag violations, never rewrite code).15---16 17# AG-UI .NET SDK Code Review18 19Encodes the AG-UI .NET SDK's house rules so a reviewer flags real violations a20generic C# reviewer misses. Authoritative sources: `sdks/dotnet/AGENTS.md`21(house rules) and `sdks/dotnet/docs/architecture.md` (design boundaries). Each22rule cites a real enforcement example in the repo; full BAD→GOOD examples and23per-rule exceptions live in [`references/rules.md`](references/rules.md). The24change-footprint and test rules below distil the `minimize-change-footprint`,25`ensure-test-coverage`, and `ensure-test-quality` review conventions for this SDK.26 27## Reviewer discipline28 29- **High signal-to-noise.** Only flag a genuine violation of a rule below, tied30  to its cited convention. Don't invent nits or restate analyzer output.31- **Verify before flagging.** Read the actual changed code and confirm the rule32  applies. Automated reviewers have high false-positive rates.33- **Scope.** Review only files under `sdks/dotnet/`. Skip generic style already34  enforced by `TreatWarningsAsErrors`, nullable, and EditorConfig.35- **Blast radius.** Fix nothing — this skill only reports. Local convention36  violations get a finding; codebase-wide concerns are noted as out-of-scope37  follow-ups, not per-line nits.38- **Severity:** ❌ must fix (breaks AOT/wire/build/boundary); ⚠️ should fix39  (convention drift); 💡 optional polish.40 41## The review process42 43**Step 0 — Ground truth & file classification.** Derive the package map from44`AGUI.slnx`; skim `AGENTS.md` + `docs/architecture.md`. Classify each changed45file: `production | test | sample | proto | csproj | public-api | docs`. Rules46are gated by class.47 48**Step 1 — Determine and summarize the change set.** Resolve what is under49review, in order: an explicit target (PR number / branch / commit range); else50the current branch vs. its tracking/base branch (`git merge-base <base> HEAD`);51plus **staged, unstaged, and untracked** working-tree changes. Then write a short52summary of what the diff does — new types, wire/protocol changes, public-surface53changes, new dependencies, src-vs-sample placement. This frames every phase and54catches scope creep early.55 56**Step 2 — Walk the rules, phase by phase (A→G).** For each phase, check every57applicable changed file against every rule in that phase. Write each finding58immediately: `file:line`, rule ID, severity, one-line fix. Verify the code first.59 60**Step 3 — Self-validation.** Dedupe; confirm each finding cites a real rule and61a real line; confirm no rule was applied to the wrong file class; drop anything62not verifiable in the actual diff.63 64**Step 4 — Emit the review summary** (see below) — a single human-readable comment,65findings grouped by severity with a verdict and coverage line.66 67## Phase A — Scope / scenarios68 69- `NET-SCOPE-01` Every change traces to a spec requirement or issue `[⚠️]`70- `NET-SCOPE-02` No unrequested capability, configurability, or dependency `[⚠️]`71- `NET-SCOPE-03` A wire/protocol change carries compatibility coverage `[❌]`72- `NET-SCOPE-04` Sample-only behavior stays out of `src/` `[⚠️]`73- `NET-SCOPE-05` Minimal footprint — every diff line serves the task; no unrelated74  refactor, speculative abstraction, or cosmetic churn `[⚠️]`75 76## Phase B — Design / architecture77 78From `docs/architecture.md` "Architectural constraints".79 80- `NET-ARCH-01` No `src/` project references `Microsoft.AspNetCore.App` `[❌]` —81  `git grep "Microsoft.AspNetCore" -- sdks/dotnet/src` must be empty.82- `NET-ARCH-02` Correct package placement (wire→Abstractions, SSE→Formatting,83  proto→Protobuf, client/transport→Client, hosting→Server) `[❌]`84- `NET-ARCH-03` Dependency direction — `Client` and `Server` never reference each85  other `[❌]`. Cite: `src/AGUI.Server/AGUI.Server.csproj`.86- `NET-ARCH-04` `IChatClient` is the only integration point — no bespoke agent87  abstraction `[💡]`88- `NET-ARCH-05` Every type has a single reason to change `[⚠️]`89- `NET-ARCH-06` No interface without multiple implementations or a test-double need `[⚠️]`90- `NET-ARCH-07` Make invalid states unrepresentable (enums/types over bool/string) `[⚠️]`91- `NET-ARCH-08` One class per file; file name matches the type `[⚠️]`92 93## Phase C — Implementation / correctness94 95From `AGENTS.md` "JSON serialization" / "Code style".96 97- `NET-IMPL-01` Every serializable type registered in `AGUIJsonSerializerContext`98  via `[JsonSerializable(typeof(T))]` `[❌]`. Cite:99  `src/AGUI.Abstractions/Serialization/AGUIJsonSerializerContext.cs`.100- `NET-IMPL-02` No `JsonSerializer.Serialize<object>` and no reflection-based101  serialization — go through the source-gen context `[❌]`102- `NET-IMPL-03` Polymorphic JSON uses a hand-written discriminator `JsonConverter<T>` `[❌]`.103  Cite: `src/AGUI.Abstractions/Events/BaseEventJsonConverter.cs`.104- `NET-IMPL-04` Property attribute kit present: explicit `[JsonPropertyName]`, **no**105  `[JsonIgnore(WhenWritingNull)]` on optionals (the context-wide default does that),106  required strings = `string.Empty`, collections = `[]` `[⚠️]`.107  Cite: `src/AGUI.Abstractions/Events/RunStartedEvent.cs`.108- `NET-IMPL-05` `ConfigureAwait(false)` on every `await` in `src/` `[⚠️]`. Cite:109  `src/AGUI.Client/AGUIChatClient.cs`.110- `NET-IMPL-06` `[EnumeratorCancellation]` on the token of any `IAsyncEnumerable<T>`111  method `[⚠️]`. Cite: `src/AGUI.Formatting/SseEventStreamFormatter.cs`.112- `NET-IMPL-07` `ArgumentNullException.ThrowIfNull(...)` for public-API argument113  validation `[⚠️]`114- `NET-IMPL-08` Validate external input at the boundary, not deep in the stack `[⚠️]`115- `NET-IMPL-09` Guard every code path — no silent `default`/fallthrough `[⚠️]`116- `NET-IMPL-10` No swallowed exceptions (empty or log-only `catch`) `[⚠️]`117- `NET-IMPL-11` Never log or expose sensitive data in errors `[❌]`118- `NET-IMPL-12` No dead, commented-out, or impossible-condition defensive code `[⚠️]`119- `NET-IMPL-13` No logic duplicated across the changeset — extract a shared helper120  at 3+ uses (Rule of Three; don't extract for 1–2) `[⚠️]`121- `NET-IMPL-14` Prefer BCL/platform APIs over hand-rolled equivalents `[⚠️]` —122  exception: deliberate AOT-safe hand-written paths (the `JsonElement`↔`Value`123  bridge, the discriminator converters) are intentional, not violations.124 125## Phase D — Wire compatibility126 127- `NET-WIRE-01` Protocol types match the TS reference — honor the128  `// Keep in sync with sdks/typescript/...` markers `[❌]`. Cite:129  `src/AGUI.Abstractions/Events/RunStartedEvent.cs`.130- `NET-WIRE-02` Events are additive — unknown types round-trip via `RawEvent`;131  don't remove or repurpose existing fields `[⚠️]`132- `NET-WIRE-03` Protobuf parity preserved for the supported event set `[❌]`133 134## Phase E — PublicAPI analyzer135 136- `NET-API-01` Any public-surface change updates that project's137  `PublicAPI.Unshipped.txt` (build fails RS0016 otherwise) `[❌]`. Cite:138  `sdks/dotnet/Directory.Build.targets`, `src/AGUI.Abstractions/PublicAPI.Unshipped.txt`.139- `NET-API-02` A new event type completes the full checklist (class in `Events/`140  deriving `BaseEvent`; `Type` → `AGUIEventTypes` constant; `[JsonSerializable]`;141  read case in `BaseEventJsonConverter`; `PublicAPI.Unshipped.txt`; round-trip142  test) `[⚠️]`143 144## Phase F — Style / naming145 146From `AGENTS.md` "Code style" / "Naming" (not all analyzer-enforced).147 148- `NET-STYLE-01` `sealed` on every non-abstract class `[⚠️]`149- `NET-STYLE-02` No `record` types — use `sealed class` with properties `[❌]`150- `NET-STYLE-03` No tuples in public APIs — define a named type `[⚠️]`151- `NET-STYLE-04` Braces always on `if`/`for`/`foreach`/`while` `[⚠️]`152- `NET-STYLE-05` Naming: events `{Name}Event`; discriminators `SCREAMING_SNAKE_CASE`153  constants in `AGUIEventTypes`; outcome/role constants lowercase (never enums);154  options `AGUI{Purpose}Options`; extensions `{Target}Extensions`; tests155  `{TypeUnderTest}Test` `[⚠️]`156- `NET-STYLE-06` DI-extension types use the `Microsoft.Extensions.DependencyInjection`157  namespace; all other types use the project `RootNamespace` with no sub-namespaces `[⚠️]`158- `NET-STYLE-07` No XML docs (`///`) on `internal`/`private` members `[⚠️]`159- `NET-STYLE-08` Don't reformat code you didn't otherwise change `[⚠️]`160 161## Phase G — Tests162 163From `AGENTS.md` "Running tests"; coverage/quality rules distilled from164`ensure-test-coverage` and `ensure-test-quality`.165 166SDK-specific:167- `NET-TEST-01` Serialization tests assert concrete JSON property names via168  `JsonDocument` — not via the deserialized object `[⚠️]`169- `NET-TEST-02` No full-JSON-string comparisons — assert individual properties `[❌]`170- `NET-TEST-03` No reflection to enumerate types or verify membership `[❌]`171- `NET-TEST-04` Wire-affecting change ⇒ compatibility fixture + round-trip in172  `tests/AGUI.Abstractions.UnitTests/Compatibility/` `[⚠️]`173- `NET-TEST-05` New public behavior ⇒ unit test; server-pipeline change ⇒174  integration test (`tests/AGUI.Hosting.AspNetCore.IntegrationTests/`) `[⚠️/💡]`175 176Coverage:177- `NET-TEST-06` A new/changed class with branching logic has a test covering happy178  path, primary error path, and boundary values (null/empty/zero/single) `[⚠️]`179- `NET-TEST-07` Don't test trivial code — DTOs/records with no logic, one-line180  delegations, constant returns `[💡]`181- `NET-TEST-08` Test through DI + `InternalsVisibleTo`, not members made `public`182  for tests `[⚠️]`183 184Quality:185- `NET-TEST-09` Every test asserts a specific observable value — no assertion-free186  tests, no bare `Assert.NotNull`/`True`/`NotEmpty` standing in for the real value `[❌]`187- `NET-TEST-10` Deterministic and isolated — no `Thread.Sleep`/`Task.Delay` for188  synchronization, no execution-order dependence, side effects cleaned up189  (files/ports/env) `[❌]`190- `NET-TEST-11` Test behavior, not implementation — prefer hand-written fakes over191  `mock.Verify(Times.*)` (unless the call count is the spec'd behavior); `[Theory]`192  for data variation, `[Fact]` for behavior; AAA visible inline with factory193  helpers (not shared mutable fixtures) below the tests; no `// TODO` or empty194  test bodies `[⚠️]`195 196## Self-validation197 198- [ ] The change set was resolved (target / tracking branch / working tree) and summarized199- [ ] Every applicable changed file was walked against every phase's rules200- [ ] Each finding cites a real rule ID, file, and line, and was verified in the diff201- [ ] No rule applied to the wrong file class; no duplicates202- [ ] Clean diffs are reported as clean — no padding203 204## Common pitfalls205 206| Pitfall | Solution |207|---------|----------|208| Restating analyzer/EditorConfig output | Only flag rules above that tooling does not enforce |209| Flagging a "missing" registration without checking the context | Open `AGUIJsonSerializerContext.cs` and confirm |210| Treating a sample's ASP.NET usage as a `src/` violation | `NET-ARCH-01` applies to `src/` only |211| Calling an additive new event a wire break | `NET-WIRE-02` — additive is allowed |212| Nitpicking style in files with substantive changes | Focus on the substantive change |213 214## Review summary215 216Produce a single human-readable Markdown comment — a reviewer's summary the author217can read top to bottom. Lead with the verdict, then the findings grouped by218severity (most severe first), each one self-contained.219 220```markdown221## AG-UI .NET SDK code review222 223**Verdict:** <Request changes | Comment | Looks good> — <one-sentence reason>224**Change set:** <branch vs base, e.g. `feat/x` vs `main`> · <N files reviewed>225(+<U untracked>) · **Findings:** ❌ <a> · ⚠️ <b> · 💡 <c>226 227<One short paragraph: what the change does and the overall read.>228 229### ❌ Must fix230- **`src/AGUI.Abstractions/Events/FooEvent.cs:42`** · `NET-IMPL-01` — new event type231  isn't registered in `AGUIJsonSerializerContext`; it fails under AOT.232  **Fix:** add `[JsonSerializable(typeof(FooEvent))]`.233 234### ⚠️ Should fix235- **`src/AGUI.Client/AGUIChatClient.cs:88`** · `NET-IMPL-05` — bare `await` in236  library code. **Fix:** append `.ConfigureAwait(false)`.237 238### 💡 Optional239- **`src/AGUI.Server/StreamAdapter.cs:17`** · `NET-ARCH-04` — bespoke agent240  abstraction; the SDK integrates via `IChatClient`. **Fix:** drop the wrapper.241 242### Coverage243Phases checked: A–G. No findings in: **B Design**, **D Wire**, **E PublicAPI**.244```245 246Rules for the summary:247- **Order** findings by severity (❌ → ⚠️ → 💡), then by file. One bullet per finding:248  bold `file:line`, the `RULE-ID`, a plain-language description, and an italic249  **Fix:** with a one-line remedy. The reader can look the ID up in250  [`references/rules.md`](references/rules.md).251- **Omit empty severity sections.** Always include the **Coverage** line so the252  author sees which phases were clean versus untouched.253- **Clean diff:** skip the severity sections and write a single line —254  `✅ No violations of the AG-UI .NET house rules — checked phases A–G across N files.`255- **Verdict mapping:** any ❌ → *Request changes*; only ⚠️/💡 → *Comment*; none →256  *Looks good*. The skill never approves or blocks automatically — the verdict is257  advisory and the author decides.258 259