Fix misc issues found by Copilot#37916
Conversation
There was a problem hiding this comment.
Pull request overview
This PR contains a broad set of small fixes across EF Core runtime, providers, tooling, and tests—mostly around logging/output consistency, correctness hardening, and minor refactors.
Changes:
- Updates logging option fragments and corresponding test expectations (e.g.,
NullabilityChecksEnabledin InMemory log fragments). - Hardens several runtime/provider components (e.g., skip navigation validation, index validation relocation, retry/exception handling adjustments).
- Improves tooling output generation (JSON string escaping) and updates internal agent/skill metadata docs.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Tests/DbContextLoggerTests.cs | Updates expected log output to include NullabilityChecksEnabled. |
| test/EFCore.InMemory.FunctionalTests/LoggingInMemoryTest.cs | Updates expected InMemory options fragment. |
| test/EFCore.Design.Tests/Design/Internal/DbContextOperationsTest.cs | Updates expected options string for InMemory context info. |
| src/EFCore/Storage/ValueBuffer.cs | Adjusts equality logic to handle default/empty buffers. |
| src/EFCore/Storage/ExecutionStrategy.cs | Corrects retry logging metadata for sync execution path; refines transaction pattern check. |
| src/EFCore/Properties/CoreStrings.resx | Moves/adds resource strings and normalizes some formatting/ordering. |
| src/EFCore/Properties/CoreStrings.Designer.cs | Regenerates strongly-typed resource accessors/events for updated strings. |
| src/EFCore/Metadata/Internal/SkipNavigation.cs | Adds validation to prevent a skip navigation from being its own inverse. |
| src/EFCore/Metadata/Internal/Index.cs | Adds constructor-time validation for index properties (duplicates/wrong entity). |
| src/EFCore/Metadata/Internal/EntityType.cs | Removes redundant index property validation; tightens exception filtering in seed access. |
| src/EFCore/Infrastructure/CoreOptionsExtension.cs | Changes interceptor storage to lists and materializes concatenations. |
| src/EFCore/Diagnostics/Internal/FormattingDbContextLogger.cs | Avoids double DateTime.Now calls by caching the timestamp. |
| src/EFCore/DbContextOptionsBuilder.cs | Refactors LogTo overloads for event/category filtering efficiency/clarity. |
| src/EFCore/DbContext.cs | Minor cleanup in dispose logic using local lease. |
| src/EFCore.Tools/EFCore.Tools.csproj | Switches project SDK to Microsoft.NET.Sdk. |
| src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs | Updates SQLite regexp UDF to use non-backtracking regex + timeout. |
| src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs | Excludes UnreachableException from wrapping into DbUpdateException. |
| src/EFCore.Relational/Storage/RelationalDatabaseCreator.cs | Narrows exception swallowing in CanConnect() to avoid catching OOM/unreachable. |
| src/EFCore.Relational/Storage/RelationalConnection.cs | Adjusts reset behavior around opened counters/flags. |
| src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs | Simplifies interception flow (removes redundant proceed branch). |
| src/EFCore.Proxies/LazyLoadingProxiesOptionsBuilder.cs | Fixes XML doc reference to the correct proxies extension method. |
| src/EFCore.InMemory/Infrastructure/Internal/InMemoryOptionsExtension.cs | Fixes NullabilityChecksEnabled log fragment emission condition. |
| src/EFCore.Design/Migrations/Design/MigrationsScaffolder.cs | Ensures snapshot directory is created before writing snapshot file. |
| src/EFCore.Cosmos/Storage/Internal/CosmosExecutionStrategy.cs | Adjusts transient error detection for WebException and status handling. |
| src/EFCore.Cosmos/Storage/Internal/CosmosClientWrapper.cs | Disposes response stream properly via using var. |
| src/EFCore.Cosmos/Diagnostics/Internal/CosmosLoggerExtensions.cs | Logs elapsed time using TotalMilliseconds rather than millisecond component. |
| src/ef/Json.cs | Improves JSON string literal escaping for control characters. |
| .github/copilot-instructions.md | Edits Copilot guidance (testing bullets trimmed). |
| .agents/skills/update-pipeline/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/tooling/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/testing/SKILL.md | Renames frontmatter key and adds cross-platform testing note. |
| .agents/skills/sqlite-adonet/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/scaffolding/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/query-pipeline/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/model-building/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/migrations/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/make-skill/SKILL.md | Updates documentation text to use user-invocable. |
| .agents/skills/make-custom-agent/SKILL.md | Updates documentation text to use user-invocable. |
| .agents/skills/dbcontext-and-services/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/cosmos-provider/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/change-tracking/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/bulk-operations/SKILL.md | Renames frontmatter key to user-invocable. |
| .agents/skills/analyzers/SKILL.md | Renames frontmatter key to user-invocable. |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
src/EFCore.Relational/Storage/RelationalConnection.cs:1054
ResetState(disposeDbConnection: false)closes the underlying connection but no longer resets_openedCount/_openedInternallyunlessdisposeDbConnectionis true. If a pooled context is returned while_openedCountis non-zero, subsequentClose()calls may never reach zero (so the connection won't be closed), and_openedInternallymay also be stale. SinceResetStateis intended to return the connection to a clean baseline, consider resetting these counters/flags whenever the connection is closed here, regardless of whether it is disposed.
protected virtual void ResetState(bool disposeDbConnection)
{
CurrentTransaction?.Dispose();
ClearTransactions(clearAmbient: true);
_commandTimeout = _defaultCommandTimeout;
if (_connectionOwned
&& _connection is not null)
{
CloseDbConnection();
if (disposeDbConnection)
{
DisposeDbConnection();
_connection = null;
_openedCount = 0;
_openedInternally = false;
}
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR updates several EF Core runtime behaviors and diagnostics, alongside corresponding test baseline updates and some internal documentation/skill metadata tweaks.
Changes:
- Update InMemory provider option logging to include
NullabilityChecksEnabledwhen enabled, and adjust tests expecting option fragments/log lines. - Improve robustness/correctness in various internals (e.g., safer exception filters, regex timeout, Cosmos response disposal, more accurate elapsed time logging, metadata validation).
- Minor tooling/docs cleanup (JSON literal escaping in
ef, skill frontmatter key rename, copilot instruction trim).
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Tests/DbContextLoggerTests.cs | Update expected context initialization log output to include NullabilityChecksEnabled. |
| test/EFCore.InMemory.FunctionalTests/LoggingInMemoryTest.cs | Update expected default options fragment for InMemory logging. |
| test/EFCore.Design.Tests/Design/Internal/DbContextOperationsTest.cs | Update expected DbContext options string returned by design-time operations. |
| src/EFCore/Storage/ValueBuffer.cs | Adjust ValueBuffer.Equals to handle default/empty buffers (_values == null). |
| src/EFCore/Storage/ExecutionStrategy.cs | Fix retry logging flag to correctly indicate sync execution. |
| src/EFCore/Properties/CoreStrings.resx | Resource updates/reordering and new strings (compiled model provider mismatch, skip nav self-inverse). |
| src/EFCore/Properties/CoreStrings.Designer.cs | Regenerate designer to match resource changes/new APIs. |
| src/EFCore/Metadata/Internal/SkipNavigation.cs | Add guard preventing a skip navigation from being set as its own inverse. |
| src/EFCore/Metadata/Internal/Index.cs | Add validation for duplicate/wrong-entity index properties in Index construction. |
| src/EFCore/Metadata/Internal/EntityType.cs | Move index property validation responsibility and refine exception swallowing filter. |
| src/EFCore/Infrastructure/CoreOptionsExtension.cs | Materialize interceptors into lists (avoid deferred concat) and use collection expressions. |
| src/EFCore/Diagnostics/Internal/FormattingDbContextLogger.cs | Use a single DateTime.Now value for consistent date/time output. |
| src/EFCore/DbContextOptionsBuilder.cs | Refactor LogTo overloads to reduce repeated allocations and clarify control flow. |
| src/EFCore/DbContext.cs | Minor cleanup using local lease consistently. |
| src/EFCore.Tools/EFCore.Tools.csproj | Switch SDK to Microsoft.NET.Sdk for EFCore.Tools packaging project. |
| src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs | Add regex match timeout for regexp to mitigate pathological regex behavior. |
| src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs | Avoid wrapping UnreachableException into DbUpdateException. |
| src/EFCore.Relational/Storage/RelationalDatabaseCreator.cs | Don’t swallow OOM/unreachable exceptions in CanConnect(). |
| src/EFCore.Relational/Storage/RelationalConnection.cs | Adjust ResetState logic (but see review comment re: open counters). |
| src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs | Ensure setters always proceed; simplify flow. |
| src/EFCore.Proxies/LazyLoadingProxiesOptionsBuilder.cs | Fix XML doc reference to the correct extension method. |
| src/EFCore.InMemory/Infrastructure/Internal/InMemoryOptionsExtension.cs | Fix LogFragment emission to append NullabilityChecksEnabled when enabled. |
| src/EFCore.Design/Migrations/Design/MigrationsScaffolder.cs | Ensure snapshot directory is created before writing snapshot. |
| src/EFCore.Cosmos/Storage/Internal/CosmosExecutionStrategy.cs | Avoid NRE when WebException.Response isn’t an HttpWebResponse. |
| src/EFCore.Cosmos/Storage/Internal/CosmosClientWrapper.cs | Dispose Cosmos stream response to avoid resource leaks. |
| src/EFCore.Cosmos/Diagnostics/Internal/CosmosLoggerExtensions.cs | Log total elapsed milliseconds (not the milliseconds component). |
| src/ef/Json.cs | Improve JSON string literal escaping (control chars, common escapes). |
| .github/copilot-instructions.md | Remove/relocate some testing guidance from copilot instructions. |
| .agents/skills/update-pipeline/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/tooling/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/testing/SKILL.md | Rename frontmatter key to user-invocable and add cross-platform testing note. |
| .agents/skills/sqlite-adonet/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/scaffolding/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/query-pipeline/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/model-building/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/migrations/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/make-skill/SKILL.md | Update template wording to user-invocable. |
| .agents/skills/make-custom-agent/SKILL.md | Update guidance wording to user-invocable. |
| .agents/skills/dbcontext-and-services/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/cosmos-provider/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/change-tracking/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/bulk-operations/SKILL.md | Rename frontmatter key to user-invocable. |
| .agents/skills/analyzers/SKILL.md | Rename frontmatter key to user-invocable. |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
roji
left a comment
There was a problem hiding this comment.
LGTM, left comments where I'm not 100% sure and it would be good for you to take another look.
78d8974 to
da98f19
Compare
This pull request primarily updates the YAML frontmatter and documentation for agent skill files to use the correct property name, and makes several improvements and fixes in the Cosmos provider and migration scaffolding code. The most important changes are grouped below.
Documentation and YAML frontmatter consistency
Changed the property name from
user-invokabletouser-invocableacross all agent skill files and related documentation, ensuring consistent terminology and preventing confusion or misconfiguration. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]Added a testing guideline for cross-platform code in
.agents/skills/testing/SKILL.md, instructing agents to verify fixes on both Windows and Linux/macOS.Cosmos provider improvements
CosmosLoggerExtensions.csto useTotalMillisecondsinstead ofMillisecondsfor elapsed time, improving accuracy in diagnostics messages. [1] [2] [3] [4] [5]CosmosClientWrapper.csby disposing the response fromCreateItemStreamAsyncproperly using ausingstatement.CosmosExecutionStrategy.csto safely handle nullHttpWebResponseand treat null status codes as transient, preventing potential crashes and improving retry logic.Migration scaffolding fix
MigrationsScaffolder.cs, preventing errors during migration scaffolding.