Add evidence-based SqlClient code review skill by cheenamalhotra · Pull Request #4736 · dotnet/SqlClient

cheenamalhotra · 2026-09-23T17:00:04Z

Description

Add a shared review skill informed by dotnet/runtime, dotnet/efcore, and microsoft/mssql-rs, with pinned sources.
Cover driver correctness, compatibility, pooling, TDS, async paths, and regression coverage.
Adapt mssql-rs lessons on vacuous tests, versioned parity evidence, partial-send failures, and measured performance to SqlClient's contracts.
Add concise finding examples and safeguards for automated publication, duplicate findings, and stale reviews.
Route the existing code-review prompt through the skill with explicit read-only tools. No driver behavior changes or new automation.

Issues

N/A

Testing

Self-reviewed and addressed findings. Checked frontmatter, local links, reference links, whitespace, and read-only prompt scoping across all five Markdown files. Verified the four pinned mssql-rs sources. Driver tests are not applicable to this documentation-only change.

Guidelines

Tests added or updated: N/A; documentation-only
Public API changes documented: N/A
Verified against customer repro: N/A
Ensure no breaking changes introduced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🟡 Changes recommended

Removing explicit tool scoping broadens prompt permissions and contradicts the stated read-only, no-automation-change scope.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Restore explicit least-privilege tool scoping · New

What changed in this PR

Adds a reusable, evidence-based SqlClient review skill and routes the existing review prompt through it.

Changes:

Defines review workflow, driver checks, and publication safeguards.
Documents pinned upstream sources and reporting examples.
Simplifies the existing review prompt to use the shared skill.

File	Description
`.github/skills/sqlclient-code-review/SKILL.md`	Defines the review procedure.
`.github/skills/sqlclient-code-review/references/sources.md`	Records sources and adaptations.
`.github/skills/sqlclient-code-review/references/reporting.md`	Defines reporting and publication rules.
`.github/skills/sqlclient-code-review/references/driver-checks.md`	Adds SqlClient-specific review guidance.
`.github/prompts/code-review.prompt.md`	Routes reviews through the skill.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🔵 Needs a closer look

The driver-check reference incorrectly labels abbreviated paths as repository-relative, which can misdirect automated reviews.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Restore explicit least-privilege tool scoping

Previously missed (1)

In code that hasn't changed since last review

Incorrect repository-relative paths in file location table

.github/skills/sqlclient-code-review/references/driver-checks.md:5

The statement that all entries below are repository-relative is incorrect. For example, the table lists ConnectionPool/ChannelDbConnectionPool.cs, but the file is under src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/; several other entries are similarly abbreviated. This can send automated reviewers to nonexistent paths, so describe these as search starting points or expand them to full repository-relative paths.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🟡 Changes recommended

The prompt loads review instructions from the untrusted workspace instead of a trusted base revision.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Resolve review instructions from a trusted base, not the worktree · New

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🟢 Approval recommended

The previous tool-scoping and trust-boundary concerns are addressed, with no remaining actionable defects found.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Resolve review instructions from a trusted base, not the worktree

priyankatiwari08

Docs/skill-only change. Two things:

Prompt says report the review as partial rather than using a terminal fallback, but SKILL.md says prefer gh for GitHub reads when available and documents an npx @microsoft/learn-cli fallback. Reconcile — as written an agent gets contradictory guidance on shell use.
See inline on the tools list.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🟢 Approval recommended

The documentation-only changes are internally consistent, scoped appropriately, and address the prior tool and trust-boundary feedback.

Review effort: Balanced
Findings: None

mdaigle

There's too much in here to read. But a few points did stick out that didn't make much sense. I suspect we can probably cut a lot of this out? We can adjust it over time

priyankatiwari08

Docs-only. Content is accurate against the current tree (verified src/Microsoft.Data.SqlClient/ref/ exists and netcore/ref//netfx/ref/ do not; all referenced policy/instruction files resolve). Two follow-ups:

The skill tells reviewers to read .github/copilot-instructions.md / architecture.instructions.md as trusted policy, but those still claim netcore/ref/ and netfx/ref/ are the active ref surface. Reviewers get contradictory guidance until those are corrected.
run_secret_scanning was dropped from the prompt allowlist, so the secret-scanning inspection step in driver-checks.md has no tool behind it in this prompt.

priyankatiwari08 · 2026-09-25T06:38:30Z

+| Surface | Starting point |
+| --- | --- |
+| Driver implementation | `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/` |
+| Public API and build selection | `src/Microsoft.Data.SqlClient/ref/`, implementation/ref `.csproj` files and imported build files |


Correct for the current tree, but .github/copilot-instructions.md and architecture.instructions.md still say netcore/ref/ and netfx/ref/ are active. Since the skill directs reviewers to those as trusted policy, they should be updated here or in a follow-up.

Updated the Copilot, architecture, and API-design instructions to use the unified src/Microsoft.Data.SqlClient/ref/ project and sources. Removed the stale legacy-layout warning from the skill.

priyankatiwari08 · 2026-09-25T06:38:30Z

+
+- Inspect added code, configuration, samples, and comments for embedded credentials;
+  follow `.github/instructions/secrets.instructions.md` without reproducing suspected
+  secrets in findings. Inspect available current-head secret-scanning results.


The code-review prompt no longer lists github/run_secret_scanning, and nothing else in its allowlist reads scanning results — so this step is always a verification gap under that prompt. Worth stating that explicitly here.

Made this explicit: the draft prompt cannot run scans or retrieve scanning alerts. It reports a verification gap unless redacted results for the reviewed revision are supplied by the user or trusted CI; manual inspection is not a scan.

priyankatiwari08 · 2026-09-25T06:38:30Z

+argument-hint: <PR number, PR URL, branch name, or local changes>
 agent: agent
-tools: ['github/search_issues', 'github/pull_request_read', 'github/get_file_contents', 'github/run_secret_scanning', 'read/readFile', 'search']
+tools: ['github/search_issues', 'github/issue_read', 'github/pull_request_read', 'github/get_file_contents', 'read/readFile', 'search/changes', 'search/codebase', 'search/fileSearch', 'search/listDirectory', 'search/textSearch', 'search/usages', 'vscode/askQuestions']


github/run_secret_scanning was removed but no read path for scan results replaced it, so the secret-scanning guidance in the skill is unreachable from this prompt.

The prompt now states that its allowlist has no scan execution or alert-read tool. It accepts supplied, redacted results or reports a gap, rather than implying it can fetch them; no additional permissions were added.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot

Copilot review overview

🟢 Approval recommended

The documentation-only changes are internally consistent, local links resolve, and prior tool-scoping concerns are addressed.

Review effort: Balanced
Findings: None

Add evidence-based SqlClient code review skill

1a9c75d

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

cheenamalhotra requested review from a team and a balanced review from Copilot September 23, 2026 17:00

github-project-automation Bot added this to SqlClient Board Sep 23, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Sep 23, 2026

cheenamalhotra moved this from To triage to In review in SqlClient Board Sep 23, 2026

Copilot started reviewing on behalf of cheenamalhotra September 23, 2026 17:00 View session

cheenamalhotra added this to the 8.0.0-preview1 milestone Sep 23, 2026

github-actions Bot assigned mdaigle and priyankatiwari08 Sep 23, 2026

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread .github/prompts/code-review.prompt.md

cheenamalhotra assigned paulmedynski Sep 23, 2026

Restore read-only tool scope for code review prompt

0496eeb

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot AI review requested due to automatic review settings September 23, 2026 17:09

Copilot started reviewing on behalf of cheenamalhotra September 23, 2026 17:10 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Adapt mssql-rs review lessons for SqlClient

db89a9a

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot AI review requested due to automatic review settings September 23, 2026 17:29

Copilot started reviewing on behalf of cheenamalhotra September 23, 2026 17:30 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread .github/prompts/code-review.prompt.md Outdated

Pin code review guidance to trusted policy sources

c1bec69

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot AI review requested due to automatic review settings September 23, 2026 17:36

Copilot started reviewing on behalf of cheenamalhotra September 23, 2026 17:36 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

cheenamalhotra enabled auto-merge (squash) September 23, 2026 18:01

priyankatiwari08 reviewed Sep 24, 2026

View reviewed changes

Comment thread .github/prompts/code-review.prompt.md Outdated

Constrain review lookups to authorized tool scopes

111f98f

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot AI review requested due to automatic review settings September 24, 2026 15:15

Copilot started reviewing on behalf of cheenamalhotra September 24, 2026 15:16 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

cheenamalhotra requested a review from priyankatiwari08 September 24, 2026 15:21

cheenamalhotra added Hotfix 7.1.1 PRs targeting main that should be backported to release/7.1 for 7.1.1. Hotfix 7.0.4 Hotfix 6.1.8 labels Sep 24, 2026

mdaigle previously approved these changes Sep 24, 2026

View reviewed changes

priyankatiwari08 reviewed Sep 25, 2026

View reviewed changes

Align review guidance with unified references and scan access

1e4aff9

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e511be1-16f3-4ab0-a8d3-51efbde72763

Copilot AI review requested due to automatic review settings September 25, 2026 07:44

cheenamalhotra dismissed mdaigle’s stale review via 1e4aff9 September 25, 2026 07:44

Copilot started reviewing on behalf of cheenamalhotra September 25, 2026 07:45 View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

priyankatiwari08 approved these changes Sep 25, 2026

View reviewed changes

paulmedynski removed the Hotfix 7.1.1 PRs targeting main that should be backported to release/7.1 for 7.1.1. label Sep 25, 2026

Conversation

cheenamalhotra commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Testing

Guidelines

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

priyankatiwari08 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

mdaigle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyankatiwari08 left a comment

Choose a reason for hiding this comment

Uh oh!

priyankatiwari08 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

priyankatiwari08 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

priyankatiwari08 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cheenamalhotra commented Sep 23, 2026 •

edited

Loading

mdaigle left a comment •

edited

Loading