Add code review instructions derived from PR feedback patterns#1646
Draft
SteveL-MSFT wants to merge 3 commits into
Draft
Add code review instructions derived from PR feedback patterns#1646SteveL-MSFT wants to merge 3 commits into
SteveL-MSFT wants to merge 3 commits into
Conversation
Analyzed review comments across 30+ PRs spanning Dec 2025 - Jul 2026 to extract recurring patterns and repo-specific best practices for code reviews: - General: avoid false-positive compile claims, respect test harness vs production distinction, recognize auto-generated files - Rust: cache keys, API visibility, fail-closed security, Windows FFI/COM safety, DLL loading security, what-if correctness, Option return types, schema coherence - Pester tests: cross-platform paths, env var isolation, ACL cleanup, exit code assertions, array comparisons, event subscriber cleanup, cmdlet skip guards - PowerShell adapters: module array returns, $using: in parallel blocks, terminating vs non-terminating error distinction - Design: settings precedence, backward compatibility, semver for resources, canonical property naming conventions, noFiltering semantics - CI/CD: fork permissions, conditional tool install, PS naming conventions - Docs: remove debug prints, locale string accuracy, dead i18n keys Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a repository-specific GitHub Copilot code review instruction set under .github/instructions/ to reduce recurring false positives and encode patterns maintainers commonly enforce across DSC’s Rust, PowerShell/Pester, and JSON changes.
Changes:
- Introduces a new Copilot code-review instruction file with guidance grouped by domain (Rust, PowerShell/Pester, architecture, CI/CD, docs/logging).
- Adds targeted “avoid noise” principles (e.g., CI-validated compile/test claims, test-harness vs production expectations, autogenerated file handling).
Code Coverage ReportNo Rust files were changed in this PR. 🔵 Full Codebase Coverage81% (good)
|
SteveL-MSFT
marked this pull request as draft
July 24, 2026 21:14
Extracted and analyzed 2,733 review comments across all 455 PRs with reviews in this repository (Dec 2025 - Jul 2026). Used parallel analysis agents to synthesize patterns by theme: security, performance, testing, maintainability, and manifest/schema coherence. Key sections: - False-positive avoidance (patterns Copilot should NOT flag) - Security (fail-closed checks, DLL hijacking, policy bypass, secret leaks) - Rust patterns (caching, API visibility, error handling, FFI safety, what-if) - Pester testing (cross-platform, isolation, assertions, structure) - PowerShell adapters (module arrays, parallel scoping, error types) - Design (settings precedence, backward compat, schema coherence) - CI/CD (fork permissions, artifact contracts, control flow) - Documentation (comments, logging, i18n accuracy) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
SteveL-MSFT
force-pushed
the
stevel-msft-add-code-review-instructions
branch
from
July 25, 2026 15:58
98036d8 to
d2be216
Compare
Comment on lines
+8
to
+10
| These instructions guide Copilot when performing code reviews on pull requests in this repository. | ||
| Focus on high-confidence, actionable findings. Do not comment on style, formatting, or trivial issues. | ||
| Derived from analysis of 2,700+ review comments across 455 PRs in this repository. |
Comment on lines
+81
to
+83
| ### Concurrency | ||
|
|
||
| - **`ConcurrentQueue` draining**: Do not loop on `.IsEmpty` followed by `TryDequeue`. Use `while queue.TryDequeue(...)` as the single loop condition. |
| - **Accurate comments**: If code behavior changes, update comments to match. | ||
| - **Log level appropriateness**: Full `PATH` contents at `trace!`, not `debug!`. Never log secrets. | ||
| - **Doc comments matching implementation**: Update doc comments when described behavior doesn't match reality. | ||
| - **Remove debug print statements**: No `println!` debug output in production or test code. Use `debug!`/`trace!` macros. |
…review-instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copilot's automated code reviews sometimes flag false positives (e.g., claiming code won't compile when it does) or miss repo-specific patterns that human reviewers consistently catch. This PR adds a
.github/instructions/code-review.instructions.mdfile to teach Copilot the recurring patterns and best practices specific to this repository.Approach
Analyzed review comments across 30+ merged PRs spanning Dec 2025 -- Jul 2026, focusing on:
The resulting instructions file is organized by domain and covers:
$using:in parallel blocks, terminating vs non-terminating errorsNotes
applyTofrontmatter targets**/*.rs, **/*.ps1, **/*.psm1, **/*.tests.ps1, **/*.jsonto cover the main file types in this repo