Skip to content

Commit c876951

Browse files
authored
Merge branch 'main' into merge/release/11.0-to-main
2 parents 9e685c7 + ff9c81e commit c876951

547 files changed

Lines changed: 46417 additions & 10143 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.agents/skills/change-tracking/SKILL.md‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,23 @@ Manages entity states and detects changes for `SaveChanges()`.
1010

1111
## Core Components
1212

13-
- `StateManager` — central engine, identity maps, tracks all entities
14-
- `InternalEntityEntry` — per-entity state, property flags, snapshots
15-
- `SnapshotFactoryFactory` subclasses build snapshot factories for change detection
13+
- `StateManager` — owns tracked entries, identity/reference maps, fixup, cascades, notifications, and changed counts
14+
- `InternalEntryBase` and derived classes — own per-entry state, flags, values, snapshots, and ordered state transitions
15+
- `ChangeDetector` — compares current values with baselines and reports changes through entry mutation APIs
16+
- `SnapshotFactoryFactory` subclasses — create passive original- and relationship-value baselines
17+
- `IdentityMap` — permits one active entry per key; shared identity pairs a replacement with the prior `Deleted` entry
1618
- `PropertyAccessorsFactory`, `ClrPropertyGetterFactory` and `ClrPropertySetterFactory` compile property accessors for efficient snapshotting and change detection
1719
- Ordinals in `indices` parameter specify element at each complex collection depth
1820

21+
## Change Detection
22+
23+
- Snapshot and notification strategies both call `SetPropertyModified()` to keep property flags and entity state consistent.
24+
- `SetEntityState()` validates values, updates flags and complex entries, changes state, then runs manager bookkeeping hooks.
25+
26+
## Shared-Identity Replacement
27+
28+
`SharedIdentityEntry` is the invariant that distinguishes an old deleted entry from its replacement with the same identity. When attaching a replacement graph, a deleted nested entry should be reconsidered only when its source entry has a replacement; do not broadly process every deleted node. Likewise, cascading from the old replaced principal must not delete dependents that belong to the replacement graph. Keep synchronous and asynchronous graph-attach paths equivalent and preserve unrelated values in replacement tests so they isolate the identity transition.
29+
1930
## Testing
2031

2132
Unit tests: `test/EFCore.Tests/ChangeTracking/`. Functional tests: `test/EFCore.Specification.Tests/GraphUpdates/`.
@@ -24,7 +35,7 @@ Unit tests: `test/EFCore.Tests/ChangeTracking/`. Functional tests: `test/EFCore.
2435

2536
| Pitfall | Solution |
2637
|---------|----------|
27-
| There is a failure when there is shared identity entry (Added and Deleted) | Add code that checks `SharedIdentityEntry` |
38+
| State-based checks treat every Added/Deleted entry as a replacement | Scope replacement behavior through `SharedIdentityEntry` and the graph source |
2839

2940
## Validation
3041

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
---
2+
name: code-review
3+
description: 'Review EF Core pull requests or local changes for concrete correctness, compatibility, performance, security, and test-coverage problems. Use when asked to review, critique, or check code changes before merge or submission.'
4+
---
5+
6+
# EF Core Code Review
7+
8+
Review pull requests and local working-tree changes without modifying source. Findings must be concrete, evidence-backed problems introduced or exposed by the change, not style preferences or general improvement ideas.
9+
10+
## Scope and Safety
11+
12+
- Review the provided pull request, or staged, unstaged, and untracked local changes. Ask for clarification when both are plausible.
13+
- Never checkout, switch branches, stash, reset, clean, or otherwise mutate the working tree, except for checking out the requested head for review purposes. Never edit source as part of a review.
14+
- For a pull request, record its base, head, and immutable head SHA. If local `HEAD` differs, use read-only repository or GitHub context when sufficient; otherwise ask the user to checkout the requested commit.
15+
- Never submit an `APPROVE` or `REQUEST_CHANGES` event. COMMENT emission follows Step 8.
16+
- Do not use this skill to address existing review comments or implement fixes.
17+
18+
## Workflow
19+
20+
### 1. Establish the Review Surface
21+
22+
1. Load `.github/copilot-instructions.md` and any area skill whose description matches the changed implementation.
23+
2. Obtain the complete diff and changed-file list once. Include staged, unstaged, and untracked files for local reviews.
24+
3. For pull requests, gather the base/head refs and head SHA, but defer the PR body, linked issues, and prior review comments until Step 3.
25+
4. Stop with a clear limitation if the requested change surface cannot be established.
26+
27+
### 2. Form an Independent Assessment
28+
29+
Before reading the author's narrative:
30+
31+
1. Read each changed implementation beyond its diff hunk. Diff-only review is insufficient evidence for a finding. For very large files, read the containing members, their contracts, and relevant state. When many files are changed, use subagents to break down the review into manageable parts and gather context incrementally.
32+
2. Search callers, implementations, overrides, sibling provider paths, and nearby tests. Read shared helpers whose contracts control the changed behavior.
33+
3. When logic moves, compare the old and new input domains. Check the moved member's XML contract as well as regression coverage.
34+
4. Use focused history to explain invariants, reverts, or prior rejected approaches; establish what makes a repeated approach viable now.
35+
5. Describe the old and new behavior, likely motivation, affected callers/providers, and initial concerns in your own words.
36+
37+
### 3. Holistic PR Assessment
38+
39+
Assess the overall approach before refining implementation details:
40+
41+
- Require a concrete need for added code, APIs, abstractions, and flags. Prefer the simplest sufficient alternative; weigh benefits against complexity, surface area, and long-term maintenance.
42+
- Prefer root-cause fixes at the owning layer. Challenge a flawed direction early rather than polishing its implementation.
43+
- Require BenchmarkDotNet results or equivalent measurements for performance claims; prefer focused local runs for experimental or iterative work.
44+
- Explain and address scenario-specific regressions even when aggregate results improve.
45+
- Scale required benefit and validation with regression risk and blast radius, especially for stable code.
46+
47+
### 4. Reconcile Pull Request Context
48+
49+
For pull requests, now read the description, linked issues, and existing review threads. Treat them as claims to verify against the code. Also flag found problems the PR narrative doesn't acknowledge.
50+
51+
If the additional context reveals information that genuinely changes your evaluation, update your holistic assessment accordingly. But do not soften findings just because the PR description sounds reasonable.
52+
53+
### 5. Route Conditional Reviews
54+
55+
- Read [references/api-review.md](references/api-review.md) when the change adds, removes, or changes supported public or protected API outside an `.Internal` namespace and not annotated with `[EntityFrameworkInternal]`.
56+
- Read [references/security-review.md](references/security-review.md) when changed data crosses a documented trust boundary or the change affects serialization, generated code from database metadata, command construction, sensitive-data handling, caches or complexity bounds, assembly loading, elevated operations, or another high-risk path.
57+
58+
### 6. Analyze Behavior and Coverage
59+
60+
For each non-trivial production change, identify:
61+
62+
1. **Changed behavior**: the concrete old and new code paths.
63+
2. **Affected surfaces**: callers, providers, public contracts, generated output, persistence, diagnostics, and concurrency where applicable.
64+
3. **Regression risks**: plausible inputs, interleavings, failures, or compatibility breaks.
65+
4. **Expected coverage**: the focused test that would fail without the change or expose the regression.
66+
5. **Coverage gaps**: affected behavior not exercised by changed or existing tests.
67+
68+
Check correctness, error handling, async behavior, concurrency, state and resource lifetime, compatibility, provider hierarchy behavior, NativeAOT constraints, and performance only where the changed path plausibly makes them relevant.
69+
70+
For exception cleanup around shared or concurrent state, distinguish whether this invocation acquired each resource from whether the resource currently appears held. Cover both a failing owner releasing its acquisition and a failing contender preserving another owner's acquisition, including preexisting nested state. Construct a concrete interleaving without weakening existing reentrancy or nesting behavior.
71+
72+
For new functionality consider other feature areas that might be affected but are not directly touched by the change.
73+
74+
Do not request tests for documentation-only changes, comments, formatting, or demonstrably behavior-preserving mechanical changes. Do not flag issues that the compiler, formatter, or existing analyzers deterministically report.
75+
76+
### 7. Optional Multi-Model Review
77+
78+
When the environment supports launching sub-agents with different models (e.g., the `task` tool with a `model` parameter), run the review in parallel across multiple model families to get diverse perspectives. Different models catch different classes of issues. If the environment does not support this, proceed with a single-model review.
79+
80+
1. Inspect the available model list and select models from 2-3 distinct model families, up to 3 sub-agent models total. If fewer than 2 eligible families are available, use what is available.
81+
- Pick only from models explicitly listed as available in the environment. Do not guess or assume model names.
82+
- Never pick models labeled "mini", "fast", or "cheap" for code review.
83+
- Do not select a model from the same family that is already running the primary review (i.e. the currently used model).
84+
- If multiple models exist in the same family, pick the one with the highest version number. Prefer "-codex" variants over general-purpose.
85+
- If the model supports effort configuration use the one closest to the current effort level.
86+
2. Launch a sub-agent for each selected model in parallel, giving each the same review prompt: the PR diff, the review rules from this skill, and instructions to produce findings in the severity format defined above.
87+
3. Wait for all agents to complete, then synthesize: deduplicate findings that appear across models, elevate issues flagged by multiple models (higher confidence), and include unique findings from individual models that meet the confidence bar. If a sub-agent has not completed after 10 minutes and you have results from other agents, proceed with the results you have.
88+
4. Present a single unified review to the user, noting when an issue was flagged by multiple models. Do not wait for any remaining sub-agents and do not attempt retries if any failed. The review is complete once the report operation succeeds.
89+
90+
### 7. Verify and Report Findings
91+
92+
Before reporting a concern:
93+
94+
- Verify it against surrounding and unchanged code, including caller/callee defenses.
95+
- Identify a realistic failure or user impact. Do not promote a theoretical possibility with negligible likelihood into a finding.
96+
- Verify factual claims about APIs from repository evidence or current documentation; never rely on model memory to claim an API is absent, deprecated, or unavailable.
97+
- Report only test results actually observed. Separate unexecuted validation and residual risk from findings.
98+
99+
Order findings by severity:
100+
101+
- ❌ **Critical**: practical exploitation, arbitrary code execution, widespread data loss, or similarly catastrophic impact.
102+
- 🔴 **High**: merge-blocking correctness, security, compatibility, data-integrity, or concurrency defect.
103+
- ⚠️ **Medium**: concrete defect with limited impact or a meaningful regression gap for changed behavior.
104+
- 🔵 **Low**: minor but real defect worth fixing in this change.
105+
- 💡 **Info**: observations or suggestions that do not constitute a defect but may improve code quality or maintainability.
106+
107+
Each finding must contain:
108+
109+
1. Severity and concise title.
110+
2. Certainty level or confidence in the finding based on the available evidence.
111+
3. Changed file and line or symbol.
112+
4. Concrete failure mode and impact.
113+
5. Evidence that makes the finding actionable.
114+
6. Fix direction and, when relevant, the missing regression-test shape.
115+
116+
Present the holistic assessment before findings. Do not add praise or filler.
117+
118+
**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real>
119+
120+
**Approach**: <1-2 sentences on whether the fix/change takes the right approach>
121+
122+
**Summary**: <✅ LGTM / ⚠️ Needs Human Review / ⚠️ Needs Changes / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points. If "Needs Human Review," explicitly state which findings you are uncertain about and what a human reviewer should focus on.>
123+
124+
### 8. Optionally Post Pull Request Comments
125+
126+
Posting is allowed through either authorization path:
127+
128+
- **Direct invocation**: number the findings, let the user select or edit them, and obtain explicit confirmation before posting.
129+
- **Code-reviewer agent delegation**: an invoking code-reviewer agent may explicitly authorize COMMENT emission and provide the selected findings or complete review payload. That caller contract is sufficient authorization; do not add a redundant confirmation prompt. An analysis-only delegation does not authorize posting.
130+
131+
Immediately before posting, confirm that the pull request is still open and its base, head, and head SHA match the reviewed revision. If they changed, stop and refresh the review context.
132+
133+
Create a pending review, add only authorized findings, and submit it as `COMMENT`. Never approve or request changes. Include a concise AI-generated disclosure when posting under a user's identity rather than a bot identity. Do not post findings that are similar to existing or resolved comments; for multiple defects in the same area, consolidate them into a single finding when appropriate.
134+
135+
## Validation
136+
137+
- Every finding resolves to changed code or a changed contract and cites supporting repository evidence.
138+
- Test requests follow the behavior-impact analysis.
139+
- No source or worktree mutation occurred.
140+
- Any posted review used the reviewed head SHA and COMMENT-only authorization.
141+
142+
## Common Pitfalls
143+
144+
- Reviewing hunks without their callers or provider hierarchy.
145+
- Treating a public CLR member in `.Internal` as supported public API.
146+
- Reporting a security keyword without tracing attacker-controlled data to an impact.
147+
- Inferring that passing broad tests covers the changed failure mode.
148+
- Turning uncertainty into a firm finding instead of investigating or recording a limitation.
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# EF Core API Review
2+
3+
Use this reference only for supported public or protected API changes outside `.Internal` namespaces and without `[EntityFrameworkInternal]` or `[Experimental]`.
4+
5+
Read and apply [EF Core API conventions](conventions.md) to the proposed shape. The [.NET Framework Design Guidelines digest](https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/framework-design-guidelines-digest.md) is background guidance; established EF Core contracts take precedence.
6+
7+
## Review Procedure
8+
9+
1. Enumerate every added, removed, or changed supported member from changed source, using modified API baselines to verify the complete API shape. Include containing types, inherited contracts, implemented interfaces, overload families, and provider counterparts.
10+
2. Establish the current API shape from source and sibling APIs. Do not infer it from names or model memory.
11+
3. Keep abstractions minimal and avoid exposing implementation details.
12+
4. Gate every new API on demonstrated need. Identify a concrete user scenario and verify that an existing API or extension point does not already serve it. If that evidence is absent, recommend preserving the current surface rather than adding an overload. New public surface is permanent maintenance and compatibility cost.
13+
5. Review each member and its type as a coherent family:
14+
- naming and namespace placement;
15+
- nullability, parameter ordering, defaults, and generic constraints;
16+
- parameter and return types, including collection mutability;
17+
- sync/async pairing, `Async` suffixes, and cancellation-token placement and flow;
18+
- overload resolution and optional-parameter ambiguity;
19+
- sealing, virtual members, constructors, interfaces, and intended extensibility;
20+
- defaults and behavior consistency across core, relational, and providers.
21+
For added or changed overloads, compare binding for representative existing source calls before and after the change. Pay particular attention to `params` members, optional parameters, `null`, and arguments that convert to both array and interface parameter types. State the concrete binding and semantic change in the finding; do not merely request overload-resolution tests. For example, a one-argument sequence call may change from passing the sequence as one `params` element to passing its contents through a collection overload with an optional trailing parameter. An additive overload can preserve binaries while silently changing behavior when callers recompile.
22+
6. Check source, binary, and behavioral compatibility. Prefer an additive overload over changing a shipped signature. When replacement is necessary, the old API should be preserved with `ObsoleteAttribute` with a message that points to the replacement unless established release policy permits removal.
23+
7. Require complete XML documentation for the contract, including null behavior, defaults, cancellation, and provider limitations where relevant.
24+
8. Verify implementations and overrides agree with the abstraction contract.
25+
9. Before finishing, account explicitly for demonstrated need, contract documentation, and focused behavioral coverage. Do not recommend an additive overload until the need gate in Step 4 passes. When the proposed change omits documentation or tests for a justified added or replacement API, report that concrete omission; do not let compatibility findings displace this completeness check.
26+
27+
The final review must be explicit. For every proposed additive or replacement API, either reject or defer the new surface because need is not demonstrated or the shape needs to be reworked, or state all of these obligations in the finding:
28+
29+
- the demonstrated user need;
30+
- compatibility of existing source-call binding and behavior;
31+
- complete XML documentation for the public contract; and
32+
- focused behavioral tests for the new shape.
33+
34+
Do not leave these obligations implicit or replace them with a generic request to document or test the API.
35+
36+
## API Baselines
37+
38+
API baseline generation, baseline-file updates, and `EFCore.ApiBaseline.Tests` execution are owned by the repository's test infrastructure. Do not request them, flag their absence, or make them an API-review acceptance condition.
39+
40+
## Findings
41+
42+
Feed API findings into the main code-review format. A useful API finding identifies the affected member, the concrete compatibility or usability consequence, the established sibling pattern or contract, and a viable API shape. Do not emit a separate API report.
43+
44+
Do not report general Framework Design Guideline preferences unless they produce a concrete problem for this API or conflict with established EF Core API conventions.

0 commit comments

Comments
 (0)