Skip to content

Commit ad8036e

Browse files
committed
CSHARP-5943: Expand /review-areas to full feature set and require verified findings
TL;DR - /review-areas gains external-clone mode and an `--iterate` review→fix→converge loop (fixer agent commits fixes; stops on two clean passes, max-iterations, or no-op). - Findings now carry two tags `[fix-in-code|external-action][blocking|substantive|nit]` and lead with a bold ≤8-word TL;DR headline; cap tightened to 5 per pass. - Reviewers must reproduce every functional finding by running code before reporting it (and include the repro) — a local MongoDB is always available (`MONGODB_URI` or `mongodb://localhost`), and Atlas tests run when `ATLAS_SEARCH_TESTS_ENABLED` + `ATLAS_SEARCH_URI` are set. - The skill hard-gates on the superpowers plugin (Step 0) and weaves its skills into run framing, fixing, and the convergence check. - Breaking changes are now measured against the latest released NuGet (not `main`); `internal` is never breaking; unsupported-feature exception-type changes are not breaks (api-stability-reviewer + new AGENTS.md "Versioning conventions" section). Details This ports the MongoDB EF Core Provider's review skill feature set (plus its not-yet-merged refinement PR) onto this repo's `/review-areas`, adapting every EF-specific detail to the driver: .claude/commands/review-areas.md (full rewrite) - Three modes: local range, external PR, and external clone (diff a branch in another clone while using this clone's reviewer briefs); `<diff-repo>` is threaded through every git command and file path. - `--iterate [--max-iterations N]`: alternates review and fix passes, narrowing the file set to what the previous fixer touched, carrying forward `[nit]` findings as do-not-re-emit notes, and gating the "clean" call on `superpowers:verification-before-completion`. - Two-tag finding model and a bold TL;DR headline on every finding; report is saved to disk for PR/clone/iterate runs. - Verification requirement: functional findings must be reproduced by running a test or small repro first; repro blocks are included in the report and exempt from the word cap. Driver-accurate harness wording (no auto-testcontainer; the suite is not parallel-safe, so scope repros to the tightest `--filter`). - `[external-action]` is reserved for what genuinely can't run here — CSFLE/QE (`CRYPT_SHARED_LIB_PATH`, KMS vars), external auth mechanisms, and Atlas only when `ATLAS_SEARCH_*` is unset. - Keeps this repo's reviewer table and cross-cutters (security, api-stability, async); fixer guidance uses driver conventions (BOMs, ConfigureAwait(false), CancellationToken/OperationContext CSOT, paired sync/async, the netstandard2.1/net472/net6.0 target set). .claude/agents/api-stability-reviewer.md - `internal` is never breaking regardless of `InternalsVisibleTo` / Moq proxy visibility; baseline is the latest released `v<major>.<minor>.<patch>` tag via `gh release list` (not stale local tags); exception-type changes for unsupported features are not breaks. AGENTS.md - New "Versioning conventions" section documenting the same baseline and not-a-break rules.
1 parent 22940c4 commit ad8036e

3 files changed

Lines changed: 286 additions & 54 deletions

File tree

.claude/agents/api-stability-reviewer.md

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,15 @@ You are the cross-cutting API stability reviewer for the MongoDB C# driver.
99

1010
## Authoritative context
1111

12-
Read the root `AGENTS.md`. The driver follows SemVer; the public surface is everything declared `public`, plus `protected` and `protected internal` members on types that external code can derive from (i.e. any `public` type that is not `sealed`, plus accessible nested public types). `InternalsVisibleTo` in this repo grants visibility only to first-party test/benchmark/analyzer assemblies plus Castle's `DynamicProxyGenAssembly2` (the proxy stub Moq generates at test time); see `AssemblyInfo.cs` in each project. None of those enlarge the SemVer surface — internal types remain internal for SemVer purposes regardless of who can see them.
12+
Read the root `AGENTS.md`. The driver follows SemVer; the public surface is everything declared `public`, plus `protected` and `protected internal` members on types that external code can derive from (i.e. any `public` type that is not `sealed`, plus accessible nested public types). `InternalsVisibleTo` in this repo grants visibility only to first-party test/benchmark/analyzer assemblies plus Castle's `DynamicProxyGenAssembly2` (the proxy stub Moq generates at test time); see `AssemblyInfo.cs` in each project. **Changes to anything `internal` are never breaking — regardless of `InternalsVisibleTo`.** That a test or proxy assembly can see an internal type does not make it part of the public surface; do not flag internal signature, behavior, or visibility changes as breaks. The public surface is strictly what an external consumer can reference without `InternalsVisibleTo`.
13+
14+
## Baseline: the latest released version, not `main`
15+
16+
Breaking changes are measured against the **latest released version of the assembly** (the most recent published NuGet package), **not** against the current state of `main`. The practical consequence: a public API that was added, then changed or removed, *within the current unreleased development cycle* (i.e. it does not exist in the last release) is **not** a break — it never shipped, so no consumer depends on it. Only differences observable to someone upgrading from the last released version count.
17+
18+
**Finding the baseline.** Releases are tagged `v<major>.<minor>.<patch>` (e.g. `v3.9.0`). Do **not** rely on local `git tag` — a clone's tags are frequently stale and miss recent releases. Use the GitHub release list instead: `gh release list --limit 1 --json tagName,isLatest` for the absolute latest, or `gh release list --limit 100 --json tagName` to find the highest released `v<major>.*` relevant to the change.
19+
20+
When in doubt whether a symbol shipped, compare against that tag rather than `main`: `git -C "<diff-repo>" fetch --tags` (if the tag isn't local), then `git -C "<diff-repo>" show <tag>:<path>` or `git -C "<diff-repo>" diff <tag> -- <path>`. If a public symbol is absent at the baseline tag, changing or removing it now is not a break.
1321

1422
## Review focus
1523

@@ -18,7 +26,7 @@ Read the root `AGENTS.md`. The driver follows SemVer; the public surface is ever
1826
- Public types or members removed, renamed, or moved between namespaces.
1927
- Visibility tightening: `public``internal`, `private protected`, or `private` is fully breaking. `public``protected` on a member of an **unsealed** type is still breaking for external **non-derived** consumers, but external derivers retain access — so it's a *partial* break rather than total; treat it as breaking but note the asymmetry in the escalation rationale. (`protected internal` is a *union* of `protected` and `internal`, i.e. wider than either alone — narrowing `public` to `protected internal` is still breaking for non-derived external consumers.) Widening is usually fine but flag if the type wasn't designed for public consumption.
2028
- Attribute changes that affect serialization / binding on public types: `[BsonElement]`, `[BsonId]`, `[BsonIgnore]`, `[BsonRepresentation]`, `[BsonExtraElements]`.
21-
- Exception types thrown by public methods: new types, replaced types, removed types from documented behavior.
29+
- Exception types thrown by public methods: new types, replaced types, removed types from documented behavior. **Exception:** changing the exception type thrown for an *unsupported* feature (a not-yet-implemented LINQ operator, an unsupported serialization mapping, a guard that exists only to reject something the driver does not support) is **not** a break — the thrown type for an unsupported path is not part of the contract. Only the exception type of a *supported, documented* operation matters.
2230
- Interface members added (breaks existing implementers). Default interface methods are **not** an option here: the driver multi-targets `netstandard2.1;net472;net6.0` (see `src/Directory.Build.props`), and `net472` cannot consume DIMs — adding any interface member is a hard break for every multi-target consumer.
2331
- Enum value renames or numeric-value changes (additions are usually safe; flag if used as a wire-encoded discriminator).
2432
- Nullability annotation tightening (`string?``string` non-null, etc.) under nullable contexts. **Forward-looking:** the driver's `src/` projects do not currently enable `<Nullable>` and no source file declares `#nullable enable`, so today this rule applies only when a PR is itself the one introducing `#nullable enable` to a public-API file. Skip flagging unless the diff opts a public type into nullable annotations.
@@ -35,5 +43,11 @@ Read the root `AGENTS.md`. The driver follows SemVer; the public surface is ever
3543
- Any breaking change to a public type or member, regardless of how minor it appears.
3644
- Behavior change of a public method whose signature is unchanged — silent semantic shifts surprise users and have no `[Obsolete]` mitigation (you can only `[Obsolete]` a member you're replacing with a new one, not a member whose contract changed in place).
3745
- Default-value change on a public method.
38-
- Exception-type change for a documented exception.
46+
- Exception-type change for a documented exception thrown by a *supported* operation (not for an unsupported-feature guard — see review focus above).
3947
- Interface member added to any public interface (DIMs are not a mitigation here — see review focus above).
48+
49+
Do **not** escalate (these are not breaks — see the definitions above):
50+
51+
- Changes to anything `internal`, regardless of `InternalsVisibleTo` or who Moq's proxy stub can see.
52+
- A public API added and then changed/removed within the current unreleased cycle — it never shipped, so measure against the latest released version, not `main`.
53+
- A change to the exception type thrown for an unsupported feature (unimplemented LINQ operator, unsupported mapping, reject-guard). Only the exception type of a supported, documented operation is contractual.

0 commit comments

Comments
 (0)