diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 3ffc277abc0f6b..c023ec96cf10f3 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,16 +1,10 @@ -When reviewing pull requests, use the `code-review` skill unless the user has stated they will review the changes themselves. +**Don't claim more than you verified.** Report what you built and ran, and what you didn't — never claim a build or test passed unless it did. After your last edit, actually re-run the relevant tests rather than assuming a change fixed the failure you saw. -**Any code you commit MUST compile, and new and existing tests related to the change MUST pass.** +Scale the effort to the risk. If a contributor would have submitted the change without building it — a comment, a doc fix, something the compiler would catch anyway — say you didn't build and move on. Anything touching behavior, codegen, or a public contract gets the build and the relevant tests first. -You MUST make your best effort to ensure any code changes satisfy those criteria before committing. If for any reason you were unable to build or test code changes, you MUST report that. You MUST NOT claim success unless all builds and tests pass as described above. +Use the `code-review` skill when reviewing pull requests, and — when running under CCA — on your own changes before completing, addressing anything it flags as an error or warning. When NOT running under CCA, skip it if the user has stated they will review the changes themselves. -If you make code changes, do not complete without checking the relevant code builds and relevant tests still pass after the last edits you make. Do not simply assume that your changes fix test failures you see, actually build and run those tests again to confirm. - -When running under CCA and before completing, use the `code-review` skill to review your code changes. Any issues flagged as errors or warnings should be addressed before the task is considered complete. - -When NOT running under CCA, skip the `code-review` skill if the user has stated they will review the changes themselves. - -Before making changes to a directory, search for `README.md` files in that directory and its parent directories up to the repository root. Read any you find — they contain conventions, patterns, and architectural context relevant to your work. +When starting work in an unfamiliar directory, search for `README.md` files in it and its parents up to the repository root. Read any you find — they contain conventions, patterns, and architectural context relevant to your work. If the changes are intended to improve performance, or if they could negatively impact performance, use the `performance-benchmark` skill to validate the impact before completing. @@ -18,7 +12,7 @@ When writing or reviewing SIMD / hardware-intrinsics code (anything using `Vecto You MUST follow all code-formatting and naming conventions defined in [`.editorconfig`](/.editorconfig). -In addition to the rules enforced by `.editorconfig`, you SHOULD: +In addition to the rules enforced by `.editorconfig`, when writing C# you SHOULD: - Prefer file-scoped namespace declarations and single-line using directives. - Ensure that the final return statement of a method is on its own line. @@ -28,19 +22,33 @@ In addition to the rules enforced by `.editorconfig`, you SHOULD: - Trust the C# null annotations and don't add null checks when the type system says a value cannot be null. - Prefer `?.` if applicable (e.g. `scope?.Dispose()`). - Use `ObjectDisposedException.ThrowIf` where applicable. -- When adding new unit tests, strongly prefer to add them to existing test code files rather than creating new code files. -- When adding new test files, examine the directory structure of sibling tests first. Some test directories use flat files (e.g., `GCEvents.cs` alongside `GCEvents.csproj`) while others use per-test subdirectories. Match the existing convention. -- When working with tests, look for `README.md` files along the directory hierarchy (starting from the test's directory and walking up). These contain build, run, and authoring guidance specific to that test area. -- When adding new unit tests, avoid adding a regression comment citing a GitHub issue or PR number unless explicitly asked to include such information. -- When writing tests, prefer using `[Theory]` with multiple data sources (like `[InlineData]` or `[MemberData]`) over multiple duplicative `[Fact]` methods. Fewer test methods that validate more inputs are better than many similar test methods. - If you add new code files, ensure they are listed in the csproj file (if other files in that folder are listed there) so they build. +- When adding XML documentation to APIs, follow the guidelines at [`docs.prompt.md`](/.github/prompts/docs.prompt.md). + +When writing or modifying tests, you SHOULD: + +- Strongly prefer to add new unit tests to existing test code files rather than creating new code files. +- When adding new test files, examine the directory structure of sibling tests first. Some test directories use flat files (e.g., `GCEvents.cs` alongside `GCEvents.csproj`) while others use per-test subdirectories. Match the existing convention. +- Avoid adding a regression comment citing a GitHub issue or PR number unless explicitly asked to include such information. +- Prefer using `[Theory]` with multiple data sources (like `[InlineData]` or `[MemberData]`) over multiple duplicative `[Fact]` methods. Fewer test methods that validate more inputs are better than many similar test methods. - When running tests, if possible use filters and check test run counts, or look at test logs, to ensure they actually ran. - Do not finish work with any tests commented out or disabled that were not previously commented out or disabled. -- When writing tests, do not emit "Act", "Arrange" or "Assert" comments. -- For markdown (`.md`) files, ensure there is no trailing whitespace at the end of any line. -- When adding XML documentation to APIs, follow the guidelines at [`docs.prompt.md`](/.github/prompts/docs.prompt.md). +- Do not emit "Act", "Arrange" or "Assert" comments. + +For markdown (`.md`) files, ensure there is no trailing whitespace at the end of any line. -When NOT running under CCA, guidance for creating commits and pushing changes: +## Pull Requests + +- **One concern per PR.** Split large or mixed changes. Do large refactorings and mechanical renames in their own PR, separate from logic changes. +- **New public API requires an approved proposal before submission** — PRs adding unapproved API will be closed. Use the `api-proposal` skill; until approval lands the API stays `internal` in any submitted PR. A proposal's prototype branch is exempt and keeps its surface public — it's evidence, not a submission. +- **Core component changes should start with an issue.** Changes to the host, VM, or JIT need a GitHub issue describing the problem and motivation first. +- **Put the measurements in the description** for performance changes — BenchmarkDotNet results, or codegen and instruction-count evidence for low-level work. +- **Behavioral changes need breaking-change documentation**, even prerelease-to-prerelease. Use the `breaking-change-doc` skill. +- **Merge to main first, then `/backport`.** Servicing backports are limited to security bugs, regressions, and reliability issues, and should be small targeted fixes rather than refactorings. +- **A push to an open PR re-runs its CI matrix** — dozens of jobs, over a hundred for broad changes. For anything non-trivial, validate locally rather than using CI to find out whether it builds, and batch fixes into one push. Branches with no PR trigger nothing, as do changes confined to `**.md`, `docs/*`, or `.github/*`. +- **Treat review feedback as a sample, not a list.** A reviewer flags examples of a problem, not every instance. Grep for the rest of the class and fix it in the same push, and answer a whole round of comments at once rather than pushing per comment. + +When NOT running under CCA, for commits and pushes: - Never squash and force push unless explicitly instructed. Always push incremental commits on top of previous PR changes. - Never push to an active PR without being explicitly asked, even in autopilot/yolo mode. Always wait for explicit instruction to push. @@ -50,238 +58,23 @@ When NOT running under CCA, guidance for creating commits and pushing changes: ## AI-Generated Content Disclosure -When posting any content to GitHub under a user's credentials — opening PRs, creating issues, commenting on PRs or issues, posting review comments, or any other public-facing action — and the account is **not** a dedicated "copilot" or "bot" account/app (e.g., `github-actions[bot]`, `copilot`), you **MUST** include a concise, visible note (e.g. a `> [!NOTE]` alert) at the bottom of the content indicating the content was AI/Copilot-generated. - -This applies to all GitHub interactions: PR descriptions, issue bodies, comments, review comments, etc. Exceptions: -- The account is a recognized bot or Copilot app account (e.g., `github-actions[bot]`, `copilot`), where the AI origin is already apparent from the account identity. -- The user explicitly asks you to omit the disclosure. +When posting to GitHub under a user's credentials — PR descriptions, issue bodies, comments, review comments, or any other public-facing action — you **MUST** add a concise, visible note (e.g. a `> [!NOTE]` alert) at the bottom of the content indicating it was AI/Copilot-generated. Skip it only when posting from a recognized bot or Copilot app account (e.g. `github-actions[bot]`, `copilot`), where the AI origin is already apparent from the account identity, or when the user explicitly asks you to omit it. --- -# Building & Testing in dotnet/runtime - -## Baseline Build - -A successful baseline build of the affected component is required for incremental builds and tests. Without it you'll hit "missing testhost" and "shared framework" errors that cost 20+ minutes per occurrence. - -### When running under CCA — MANDATORY WITH ONE EXCEPTION - -⚠️ **RULE: You MUST complete the baseline build BEFORE making any code changes.** The CCA environment is fresh; there are no pre-existing artifacts, and incremental builds will fail in ways that waste significant compute. Skipping this step IS a task failure — do not proceed with code changes until the baseline build succeeds. - -If the baseline build fails, STOP, report the failure, and do not attempt to work around it. - -⚠️ **EXCEPTION:** If you are on a feature branch with commits upstream of main, and you encounter a build failure in your attempt to run a baseline build, this rule does **NOT** apply. Make any code changes necessary to fix the build. Once said changes are committed , return to requiring a baseline build. - -### When running under CLI (interactive) — flexible - -A baseline may already exist in the user's workspace from prior work. Don't re-run a 40-minute baseline if the existing one is usable. Apply this rule (works for human users and for local agents driving the CLI): - -1. **Check the sentinel artifact** for the work you're about to do. Each component-specific workflow below lists a sentinel path under `artifacts/`. If it's missing, run the baseline before proceeding. -2. **Otherwise, attempt the incremental work.** If a build or test step then fails with a baseline-missing signature from [Troubleshooting](#troubleshooting) (e.g., "shared framework must be built", "testhost" missing, `FileNotFoundException` on shared framework paths), run the baseline once and retry. Do not loop. -3. **Honor explicit user signals when offered.** If the user (or a driving agent) volunteered "just built" / "skip baseline", trust it and skip step 1's check. If they said "fresh checkout" / "no baseline", run the baseline up front without probing. - -If you're uncertain which mode you're in, follow the CCA rule. - -The remaining steps below apply in both modes whenever a baseline build is actually being performed. - -### Step 1: Identify Your Component - -Based on file paths you will modify: - -| Files Changed | Component | -|---------------|-----------| -| `src/coreclr/` | CoreCLR | -| `src/mono/` | Mono | -| `src/libraries/` (no Browser/WASM or WASI targets) | Libraries | -| `src/libraries/` with Browser/WASM or WASI targets in the affected `.csproj` | WASM/WASI Libraries | -| `src/native/corehost/`, `src/installer/` | Host | -| `src/tools` | Tools | -| `src/native/managed` | Tools | -| `src/tasks` | Build Tasks | -| `src/tests` | Runtime Tests | - -**WASM/WASI Library Detection:** A change under `src/libraries/` is WASM/WASI-relevant if the library's `.csproj` has explicit Browser/WASM or WASI targets (`TargetFrameworks`, `TARGET_BROWSER`, `TARGET_WASI` constants, or `Condition` attributes referencing `browser`/`wasi`), **and** the changed file is not excluded from those targets via `Condition` on `` or ``. - -### Step 2: Run the Baseline Build (from repo root) - -From the repo root, run the appropriate build command on the branch you intend to modify. The baseline reflects whatever is in your working tree at that moment, so: - -- If you're baselining up front (CCA, or CLI with a fresh checkout), ensure HEAD is clean — no uncommitted changes. -- If you're baselining after a probe failure and already have work-in-progress changes, either stash them first or accept that the baseline incorporates those changes. - -| Component | Command | -|-----------|---------| -| **CoreCLR** | `./build.sh clr+libs+host` | -| **Mono** | `./build.sh mono+libs` | -| **Libraries** | `./build.sh clr+libs -rc release` | -| **WASM Libraries** | `./build.sh mono+libs -os browser` | -| **Host** | `./build.sh clr+libs+host -rc release -lc release` | -| **Tools** | `./build.sh clr+libs -rc release` | -| **Build Tasks** | `./build.sh clr+libs -rc release` | -| **Runtime Tests** | `./build.sh clr+libs -lc release -rc checked` | - -For System.Private.CoreLib changes, use `-rc checked` instead of `-rc release` for asserts. - -⏱️ **This build can take up to 40 minutes.** Do not cancel unless no output for 5+ minutes. - -### Step 3: Configure Environment - -```bash -export PATH="$(pwd)/.dotnet:$PATH" -dotnet --version # Should match sdk.version in global.json -``` - -**If the baseline build fails, report the failure and stop** before proceeding with changes that depend on it. - ---- - -## Component-Specific Workflows - -These workflows assume a usable baseline build exists for the component (either freshly produced per the section above, or already present in the user's workspace under CLI use). Each workflow lists a **Baseline sentinel** — a path under `artifacts/` whose absence indicates the baseline is missing and must be run before proceeding. All commands must complete with exit code 0, and all tests must pass with zero failures. - -### Libraries (Most Common) - -**Baseline sentinel (for tests):** `artifacts/bin/testhost/` and `artifacts/bin/microsoft.netcore.app.runtime.//`. (Building a single library typically works without a baseline; running its tests does not.) - -**Build and test a specific library:** -```bash -cd src/libraries/ -dotnet build -dotnet build /t:test ./tests/.csproj -``` - -Test projects are typically at: `tests/.Tests.csproj` or `tests/.Tests/.Tests.csproj`, or under `tests/FunctionalTests/`, `tests/UnitTests/`, etc. Use `find tests -name '*.Tests.csproj'` to discover them. +## Tool Use -**Test all libraries:** `./build.sh libs.tests -test -rc release` +Issue independent tool calls together in one response rather than one at a time. Every round trip re-sends the whole conversation as cached input — measured at roughly half the cost of a call before it does any work — so fewer, wider steps beat many narrow ones. -**System.Private.CoreLib:** Rebuild with `./build.sh clr.corelib+clr.nativecorelib+libs.pretest -rc checked` +Redirect long-running commands to a log and poll a bounded view — a tail, a grep for errors, or a status sentinel. Re-reading a running command's output re-sends it from the start every time, so repeatedly checking a long build costs far more than the check is worth. Check the outcome, not the process. -Before completing, ensure ALL tests for affected libraries pass. - -### CoreCLR - -**Baseline sentinel:** `artifacts/bin/coreclr/../` for incremental runtime builds; `artifacts/tests/coreclr/../Tests/Core_Root/` for running tests. - -**Test:** `cd src/tests && ./build.sh && ./run.sh` - -### Mono - -**Baseline sentinel:** `artifacts/bin/mono/../` for incremental runtime builds; `artifacts/tests/coreclr/../Tests/Core_Root/` for running tests (Mono tests reuse the Core_Root layout). - -**Test:** ```bash -./build.sh clr.host -cd src/tests -./build.sh mono debug /p:LibrariesConfiguration=debug -./run.sh + > out.log 2>&1; echo "exit=$?" > out.status # bash + *> out.log; "exit=$LASTEXITCODE" | Out-File out.status # PowerShell -- $? is a [bool] here ``` -### WASM Libraries - -**Baseline sentinel:** `artifacts/bin/microsoft.netcore.app.runtime.browser-wasm//`. - -**Build:** `./build.sh libs -os browser` - -**Test:** `./build.sh libs.tests -test -os browser` - -### Host - -**Baseline sentinel:** `artifacts/bin/coreclr/../` and `artifacts/bin/testhost/` (host build/tests need both clr and libs in place). - -**Build:** `./build.sh host -rc release -lc release` - -**Test:** `./build.sh host.tests -rc release -lc release -test` - -### Tools - -**Baseline sentinel:** `artifacts/bin/coreclr/../` and `artifacts/bin/testhost/`. - -**Build:** `./build.sh tools+tools.ilasm` - -**Test:** `./build.sh tools+tools.ilasm+tools.illinktests+tools.cdactests -test` - -### Build Tasks - -**Baseline sentinel:** none required for `./build.sh tasks` — it's self-contained. If you go on to consume the tasks from a workflow that does need a baseline (e.g., libraries tests), apply that workflow's sentinel instead. - -**Build:** `./build.sh tasks` - -### Runtime Tests - -**Baseline sentinel:** `artifacts/tests/coreclr/../Tests/Core_Root/` (required to run individual tests; produced by the baseline build plus `src/tests/build.sh -GenerateLayoutOnly`). - -Subdirectories under `src/tests/` may contain `README.md` files with -area-specific guidance (e.g., EventPipe test patterns). - -**Build all tests:** -```bash -./build.sh clr+libs -lc release -rc checked -./src/tests/build.sh checked -./src/tests/run.sh checked -``` - -**Build a single test project** (path is relative to the repo root): -```bash -# Use -priority1 ("-Priority 1" on Windows) for tests with 1, -# otherwise the build silently reports "0 test projects" and builds nothing. -src/tests/build.sh -Test tracing/eventpipe/eventsvalidation/GCEvents.csproj x64 Release -priority1 -``` - -Other useful flags (run `src/tests/build.sh -h` for the full list): - -| Flag | Description | -|------|-------------| -| `-Test ` | Build one project | -| `-Dir ` | Build all projects in a directory | -| `-Tree ` | Build a subtree recursively | -| `-priority1` (`-Priority 1` on Windows) | Include priority 1 tests | -| `-GenerateLayoutOnly` | Generate Core_Root layout only | - -**Generate Core_Root layout** (required before running individual tests): -```bash -src/tests/build.sh -GenerateLayoutOnly x64 Release -``` - -**Run a single test:** -```bash -export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/.x64.Release/Tests/Core_Root -cd artifacts/tests/coreclr/.x64.Release// -$CORE_ROOT/corerun .dll -# Exit code 100 = pass, any other value = fail. -``` - ---- - -## Adding new tests - -When creating a regression test for a bug fix: - -1. **Verify the test FAILS without the fix** — build and run against the unfixed code. -2. **Verify the test PASSES with the fix** — apply the fix, rebuild, and run again. -3. If the fix is not yet merged locally, manually apply the minimal changes from the PR/commit to verify. - -Do not mark a regression test task as complete until both conditions are confirmed. - -## Troubleshooting - -| Error | Solution | -|-------|----------| -| "shared framework must be built" | Run baseline build: `./build.sh clr+libs -rc release` | -| "testhost" missing / FileNotFoundException | Run baseline build first (Step 2 above) | -| Build timeout | Wait up to 40 min; only fail if no output for 5 min | -| "Target does not exist" | Avoid specifying a target framework; the build will auto-select `$(NetCoreAppCurrent)` | -| "0 test projects" after `build.sh -Test` | The test has `` > 0; add `-priority1` to the build command | - -**When reporting failures:** Include logs from `artifacts/log/` and console output for diagnostics. - -**Windows:** Use `build.cmd` instead of `build.sh`. - ---- +Fetch narrowly: `gh run view --log-failed` over `--log`, `--json`/`--jq` to project only the fields needed, `git diff --stat` before the full diff. Quiet what doesn't detect a non-TTY: `curl -sS`, `--quiet` on `git clone`/`fetch`/`checkout`. MSBuild and `dotnet` already detect it — no flags needed. -## Reference +## Building & Testing -- [Build Libraries](/docs/workflow/building/libraries/README.md) · [Test Libraries](/docs/workflow/testing/libraries/testing.md) -- [Build CoreCLR](/docs/workflow/building/coreclr/README.md) · [Test CoreCLR](/docs/workflow/testing/coreclr/testing.md) -- [Build Mono](/docs/workflow/building/mono/README.md) · [Test Mono](/docs/workflow/testing/mono/testing.md) -- [WASM Build](/docs/workflow/building/libraries/webassembly-instructions.md) · [WASM Test](/docs/workflow/testing/libraries/testing-wasm.md) -- [Host Tests](/docs/workflow/testing/host/testing.md) +**Before running any build or test command, use the `build-and-test` skill** — don't guess the commands. Under CCA, invoke it **before making any code changes**; a missing or incorrect baseline build costs 20-40 minutes to recover from. diff --git a/.github/instructions/conventions.instructions.md b/.github/instructions/conventions.instructions.md new file mode 100644 index 00000000000000..bf04b8c3798044 --- /dev/null +++ b/.github/instructions/conventions.instructions.md @@ -0,0 +1,57 @@ +--- +applyTo: "src/**" +--- + +# General Conventions (all source areas) + +Code conventions for any change under `src/`, applied when authoring and when reviewing. Also +apply the language file for the code in question (`csharp`, `native`), `tests` for test changes, +and any matching area file (`core-runtime`, `jit`, `system-net-*`, `extensions-*`, `compression`, +`cdac`). Where a more specific file conflicts with a general one, the more specific file wins. + +Pull-request process — scope, benchmark evidence, API approval, backport — is in +[`copilot-instructions.md`](/.github/copilot-instructions.md). Build and test workflow is in the +`build-and-test` skill. Review-only criteria are in `.github/skills/code-review/pr-assessment.md`. + +## Change Scope & Justification + +- **Prefer the simplest solution that works.** The burden of proof is on the more complex approach. Unnecessary abstraction, extra indirection, and elaborate solutions for marginal gains are a cost, not a feature. +- **Justify each addition.** New code, APIs, abstractions, and flags create a permanent maintenance obligation. If an addition can be avoided without sacrificing correctness or meaningful capability, avoid it. +- **Fix root cause, not symptoms or workarounds.** Investigate and fix the root cause rather than adding workarounds or suppressing warnings. Revert broken commits before layering fixes. +- **Don't bundle unrelated changes.** Keep each change to a single concern: no drive-by refactoring, no whitespace noise, no build artifacts. Large refactorings and mechanical renames belong in their own change, separate from logic changes. + +## Consistency with Codebase Patterns + +### Code Reuse & Deduplication + +- **Extract duplicated logic into shared helper methods.** Fix improvements inside shared helpers so all callers benefit. +- **Move shared code to shared files, not duplicated across runtimes.** When identical code exists across CoreCLR and NativeAOT, move it to the shared partition (using `#if !MONO` if needed). +- **Use existing APIs instead of creating parallel ones.** Before introducing new types, enums, or helpers, check if existing ones serve the same purpose. Fix existing utilities rather than introducing duplicates. +- **Delete dead code and unused declarations aggressively.** Remove dead code, unnecessary wrappers, obsolete fields, and unused variables when encountered or when the only caller changes. Also remove helper methods, enum values, function declarations, and resx strings left unused by a removal. + +### Established Conventions + +- **Store error strings in `.resx`, not inline code.** Reference via the `SR` class. When removing code that uses a resx string, delete the unused string entry. +- **Preserve existing alphabetical ordering in modified lists.** When a PR adds or reorders entries in an alphabetized list—especially items within a `.csproj` item group, such as `Compile`, `ProjectReference`, and `PackageReference`—verify that the changed entries preserve the surrounding order. Flag only ordering regressions introduced by the PR; do not require unrelated cleanup of pre-existing unsorted entries. This also applies to lists of areas, configuration entries, resx entries, entrypoint/export lists, and ref source members. +- **Don't modify auto-generated files or `eng/common` manually.** Change the generator or source definition instead. Files in `eng/common` are synced from dotnet/arcade. +- **Use `DOTNET_` prefix for environment variables, not `COMPlus_`.** New runtime environment variables must use `DOTNET_` exclusively. +- **Match existing style in modified files.** The existing style in a file takes precedence over general guidelines. Do not change existing code for style alone. + +### Runtime-Specific Patterns + +- **Consider NativeAOT parity for runtime changes.** When changing CoreCLR behavior, verify whether the same change is needed for NativeAOT. Note: Mono and CoreCLR native code conventions differ significantly — do not assume they share the same rules. +- **Keep interpreter behavior consistent with the regular JIT.** Follow the same patterns, naming, error codes (`CORJIT_BADCODE`), and macros (`NO_WAY`). Use `FEATURE_INTERPRETER` guards. +- **Source generators: no file locks, diagnostics from analyzers only.** Generators should bypass invalid state gracefully. A separate analyzer should produce diagnostics. +- **Ref assembly conventions.** No `using` directives (fully qualify types), empty method bodies or `throw null`, genapi-style formatting, alphabetical member order. TFM-specific APIs go in separate files. + +## Documentation & Comments + +- **Comments should explain why, not restate code.** Delete comments like `// Get the types` that just duplicate the code in English. Don't include historical context about why code changed. +- **Delete or update obsolete comments when corresponding code changes.** Stale comments describing old behavior are worse than no comments. Update them when you touch the relevant code; leave unrelated stale comments to a dedicated cleanup pass. +- **Track deferred work with GitHub issues and searchable TODOs.** Reference a tracking issue in TODO comments with a consistent prefix (e.g., `TODO-Async:`). Remove ancient TODOs that will never be addressed. +- **Don't duplicate comments on interface implementations.** Documentation comments belong on the interface definition. Implementations should use `` to avoid divergence. +- **Add XML doc comments on all new public APIs.** These seed the official API documentation on learn.microsoft.com. Properties should start with "Gets the ..." or "Gets or sets the ...". Do not add XML docs to test code. +- **Use SHA-specific or commit-based links in documentation.** Don't use branch-relative links that break when files move. +- **Reference specs and authoritative sources in implementation code.** When parsing signatures and metadata, cite the relevant spec section (e.g., ECMA-335). Link to relevant RFCs, papers, or repo-specific documentation (such as the ECMA-335 augments maintained in this repo). This applies broadly, not just to ECMA-335. +- **Use established terminology in user-facing text.** Do not expose internal type names, private field names, or codenames like "Roslyn" in public docs or error messages. +- **Retain copyright headers and license information.** All C# and C++ source files must include the standard license header, including test files. When porting from other projects, retain original copyright and update THIRD-PARTY-NOTICES.TXT. diff --git a/.github/instructions/review-core-runtime.instructions.md b/.github/instructions/core-runtime.instructions.md similarity index 52% rename from .github/instructions/review-core-runtime.instructions.md rename to .github/instructions/core-runtime.instructions.md index 78bf9577d86c1f..73d9a177073887 100644 --- a/.github/instructions/review-core-runtime.instructions.md +++ b/.github/instructions/core-runtime.instructions.md @@ -2,15 +2,10 @@ applyTo: "src/coreclr/**,src/native/corehost/**" --- -# Code Review -- Core runtime +# Core runtime -Rules for reviewing CoreCLR and native host changes. Also apply `review-all-src`, the language -file (`review-csharp` or `review-native`), `review-all-tests` for test changes, and `jit` for -JIT changes. - -These are review criteria. During code authoring or local experimentation, treat PR-level gates -such as motivation, benchmark evidence, and issue prerequisites as preparation guidance for a -ready-for-review PR, not as reasons to block exploratory work unless the user asks for review. +Conventions for CoreCLR and native host changes. Also apply `conventions`, the language file +(`csharp` or `native`), `tests` for test changes, and `jit` for JIT changes. ## Correctness & Safety @@ -20,7 +15,3 @@ ready-for-review PR, not as reasons to block exploratory work unless the user as ## Performance & Allocations - **Avoid LINQ and records in low-level compiler codebases.** In CG2/ILC and AOT tools, use direct loops instead of LINQ and readonly structs instead of records. Use concrete types over interfaces in private code. - -## PR Prerequisites - -- **Start core component changes with an issue.** Changes to host, VM, or JIT should start with a GitHub issue describing the problem and motivation before submitting a PR. diff --git a/.github/instructions/review-csharp.instructions.md b/.github/instructions/csharp.instructions.md similarity index 79% rename from .github/instructions/review-csharp.instructions.md rename to .github/instructions/csharp.instructions.md index 036cb82dae5ae3..0031ca96326238 100644 --- a/.github/instructions/review-csharp.instructions.md +++ b/.github/instructions/csharp.instructions.md @@ -2,16 +2,11 @@ applyTo: "**/*.cs" --- -# Code Review -- C# (managed code) +# C# (managed code) -Rules for reviewing C# changes across `src/`. Also apply `review-all-src` (all changes), -`review-all-tests` (test files), and any matching area file (`review-core-runtime`, `jit`, -`system-net-*`, `extensions-*`, `compression`, `cdac`). Native runtime code is covered by -`review-native`. - -These are review criteria. During code authoring or local experimentation, treat PR-level gates -such as motivation, benchmark evidence, and issue prerequisites as preparation guidance for a -ready-for-review PR, not as reasons to block exploratory work unless the user asks for review. +Conventions for C# changes across `src/`. Also apply `conventions` (all changes), `tests` (test +files), and any matching area file (`core-runtime`, `jit`, `system-net-*`, `extensions-*`, +`compression`, `cdac`). Native runtime code is covered by `native`. ## Correctness & Safety @@ -22,7 +17,7 @@ ready-for-review PR, not as reasons to block exploratory work unless the user as - **Include actionable details in exception messages.** Use `nameof` for parameter names. Include the unsupported type or unexpected value. Never throw empty exceptions. - **Initialize output parameters in all code paths.** When a method has `out` parameters or pointer outputs (`bytesWritten`, `numLocals`), ensure they are initialized to a defined value in all error paths. - **Use `ThrowIf` helpers over manual checks.** Use `ArgumentOutOfRangeException.ThrowIfNegative`, `ObjectDisposedException.ThrowIf`, etc. instead of manual if-then-throw patterns. -- **Challenge exception swallowing that masks unexpected errors.** When a PR adds try/catch blocks that silently discard exceptions (`catch { continue; }`, `catch { return null; }`), question whether the exception represents a truly expected, recoverable condition or an unexpected error signaling a deeper problem (race conditions, memory corruption, build environment issues). Silently catching exceptions that "shouldn't happen" hides root causes and makes debugging harder. The default disposition should be to let unexpected exceptions propagate or fail fast so the real issue gets investigated. +- **Don't swallow exceptions that mask unexpected errors.** Before adding a try/catch that silently discards exceptions (`catch { continue; }`, `catch { return null; }`), establish that the exception is a truly expected, recoverable condition rather than an unexpected error signaling a deeper problem (race conditions, memory corruption, build environment issues). Silently catching exceptions that "shouldn't happen" hides root causes and makes debugging harder. Let unexpected exceptions propagate or fail fast so the real issue gets investigated. ### Thread Safety @@ -38,22 +33,17 @@ ready-for-review PR, not as reasons to block exploratory work unless the user as ### Correctness Patterns -- **Fix root cause, not symptoms or workarounds.** Investigate and fix the root cause rather than adding workarounds or suppressing warnings. Revert broken commits before layering fixes. -- **Prefer safe code over unsafe micro-optimizations.** Do not introduce `Unsafe.As`, `Unsafe.AsRef`, or raw pointers without demonstrable performance need. Prefer Span-based APIs. If performance is the issue, prefer fixing the JIT. +- **Prefer safe code over unsafe micro-optimizations.** Do not introduce `Unsafe.As`, `Unsafe.AsRef`, or raw pointers without demonstrable performance need. Prefer Span-based APIs. If performance is the issue, prefer fixing the JIT. Never convert safe code to unsafe as a side effect of an unrelated functional change. - **Use `Unsafe.BitCast` for same-size type punning between blittable types.** Prefer `Unsafe.BitCast` over `Unsafe.As` for type punning between unmanaged value types of the same size. For common cases, prefer safe alternatives (e.g., `BitConverter.SingleToInt32Bits` for `float`→`int`). -- **Scope creep: don't bundle cleanup into unrelated changes.** When the focus is a functional change, don't also convert safe code to unsafe or refactor for micro-optimizations. Keep those in separate PRs. -- **Delete dead code and unnecessary wrappers.** Remove dead code, unnecessary wrappers, obsolete fields, and unused variables when encountered or when the only caller changes. - **Handle `SafeHandle.IsInvalid` before `Dispose`.** Check `IsInvalid` (not null) on returned SafeHandles. Get the exception before calling `Dispose`, since Dispose might clear the error state. -- **Seal classes when `Equals` uses exact type matching.** If a class implements `Equals` with `GetType()` comparison, flag this as a potential bug if the class is unsealed — the solution is usually to seal the class, but don't automatically recommend sealing as the fix. Raise it as a warning for the author to evaluate. +- **Seal classes when `Equals` uses exact type matching.** If a class implements `Equals` with `GetType()` comparison, treat this as a potential bug when the class is unsealed. Sealing is usually the right fix, but not always — evaluate rather than applying it reflexively. - **Use `Environment.ProcessPath` and `AppContext.BaseDirectory`.** Use these instead of `Process.GetCurrentProcess().MainModule?.FileName` and `Assembly.Location` for NativeAOT/single-file compatibility. - **File name casing must match csproj references exactly.** Linux is case-sensitive. New source files must be listed in the `.csproj` if other files in that folder are explicitly listed. -- **Backport targeted fixes, not refactorings.** When backporting to servicing branches, create small targeted fixes. Backporting large refactorings introduces unnecessary risk. ## Performance & Allocations ### Measurement & Evidence -- **Performance changes require benchmark evidence.** Include BenchmarkDotNet results before merging. Prefer local BenchmarkDotNet runs first, especially for experimental/iterative work. EgorBot runs on an individual's personal account and is not billed like Copilot usage — only recommend it when explicitly requested, or for a final cross-architecture (x64/arm64) confirmation that cannot be reproduced locally. - **Justify binary size increases with real-world measurements.** Changes that increase binary size require measured wall-clock improvements on real-world apps, not just instruction counts. - **Avoid premature optimization with object pools and caches.** Do not introduce global caches or object pools without evidence they are needed. Prefer making the underlying operation faster. @@ -86,8 +76,8 @@ ready-for-review PR, not as reasons to block exploratory work unless the user as ## API Design & Contracts -- **New public APIs require approved proposals before PR submission.** All new API surface must go through API review. PRs adding unapproved APIs will be closed. The implementation should match what was approved, though it is explicitly allowed to defer portions of an approved API to incremental follow-up PRs, and an implementor may opt to exclude specific API members for technical reasons without needing re-approval unless the exclusion significantly impacts the design. When new public API surface is detected, the API approval verification procedure (`.github/skills/code-review/api-approval-check.md`) is executed to enforce this rule. -- **Use `internal` for new APIs pending API review.** If the API is needed immediately for implementation, mark it `internal` and file a review request separately. +- **Implementation must match the approved API shape.** Deferring portions of an approved API to incremental follow-up PRs is explicitly allowed, as is excluding specific members for technical reasons — unless the exclusion significantly impacts the design. +- **Use `internal` for new APIs pending API review.** If the API is needed immediately for implementation, mark it `internal` and file a review request separately. This governs code being submitted — an `api-proposal` prototype branch keeps the surface public so ref source generation can extract it. - **Parameter names must match between ref and src.** Renaming a public API parameter (including case changes) is a breaking change affecting named arguments and late-bound scenarios. - **Align exception types and validation order across platforms.** Validate arguments first (`ArgumentNullException`, then `ArgumentException`), then `PNSE`, then `ObjectDisposedException`, then perform the operation. Throw the same exception types on all platforms. - **`Try` APIs should return `false` only for the common expected failure.** Throw for everything else (corruption, permissions, invalid arguments). Try methods must always throw on invalid arguments. diff --git a/.github/instructions/review-native.instructions.md b/.github/instructions/native.instructions.md similarity index 89% rename from .github/instructions/review-native.instructions.md rename to .github/instructions/native.instructions.md index 9859fb63361482..e52a089f92db0f 100644 --- a/.github/instructions/review-native.instructions.md +++ b/.github/instructions/native.instructions.md @@ -2,16 +2,11 @@ applyTo: "**/*.c,**/*.cc,**/*.cpp,**/*.cxx,**/*.h,**/*.hpp,**/*.inc,**/*.S,**/*.s,**/*.asm" --- -# Code Review -- Native code (C/C++/asm) & interop +# Native code (C/C++/asm) & interop -Rules for reviewing native runtime code (CoreCLR VM, JIT, `src/native`, Mono native). Also apply -`review-all-src`, `review-all-tests` for test changes, and `review-core-runtime` for CoreCLR and -native host changes. For JIT specifics see `jit`; for networking interop see -`system-net-interop`. - -These are review criteria. During code authoring or local experimentation, treat PR-level gates -such as motivation, benchmark evidence, and issue prerequisites as preparation guidance for a -ready-for-review PR, not as reasons to block exploratory work unless the user asks for review. +Conventions for native runtime code (CoreCLR VM, JIT, `src/native`, Mono native). Also apply +`conventions`, `tests` for test changes, and `core-runtime` for CoreCLR and native host changes. +For JIT specifics see `jit`; for networking interop see `system-net-interop`. ## Correctness & Safety diff --git a/.github/instructions/review-all-src.instructions.md b/.github/instructions/review-all-src.instructions.md deleted file mode 100644 index 6b169fc9653dd9..00000000000000 --- a/.github/instructions/review-all-src.instructions.md +++ /dev/null @@ -1,104 +0,0 @@ ---- -applyTo: "src/**" ---- - -# Code Review -- General Guidance (all source areas) - -Cross-cutting review criteria for any change under `src/`. Also apply the language file for the -code under review (`review-csharp`, `review-native`), `review-all-tests` for test changes, and -any matching area file (`review-core-runtime`, `jit`, `system-net-*`, `extensions-*`, -`compression`, `cdac`). Where a more specific file conflicts with a general one, the more -specific file wins. - -**Reviewer mindset:** Be polite but very skeptical. Your job is to help speed the review process for maintainers, which includes not only finding problems the PR author may have missed but also questioning the value of the PR in its entirety. Treat the PR description and linked issues as claims to verify, not facts to accept. Question the stated direction, probe edge cases, and don't hesitate to flag concerns even when unsure. - -These are review criteria. During code authoring or local experimentation, treat PR-level gates -such as motivation, benchmark evidence, and issue prerequisites as preparation guidance for a -ready-for-review PR, not as reasons to block exploratory work unless the user asks for review. - -## Holistic PR Assessment - -Before reviewing individual lines of code, evaluate the PR as a whole. Consider whether the change is justified, whether it takes the right approach, and whether it will be a net positive for the codebase. - -### Motivation & Justification - -- **Every PR must articulate what problem it solves and why.** Don't accept vague or absent motivation. Ask "What's the rationale?" if none is provided. However, when the PR links to an approved API proposal, accepted issue, or prior discussion that already establishes motivation, referencing that is sufficient — don't demand the author re-state what's already documented. -- **Challenge every addition with "Do we need this?"** New code, APIs, abstractions, and flags must justify their existence. If an addition can be avoided without sacrificing correctness or meaningful capability, it should be. -- **Demand real-world use cases and customer scenarios.** Hypothetical benefits are insufficient motivation for expanding API surface area or adding features. Require evidence that real users need this. - -### Evidence & Data - -- **Require measurable performance data before accepting optimization PRs.** Demand BenchmarkDotNet results or equivalent proof — never accept performance claims at face value. Prefer local BenchmarkDotNet runs first, especially for experimental/iterative work. EgorBot runs on an individual's personal account and is not billed like Copilot usage — only recommend it when explicitly requested, or for a final cross-architecture (x64/arm64) confirmation that cannot be reproduced locally. -- **Distinguish real performance wins from micro-benchmark noise.** Trivial benchmarks with predictable inputs overstate gains from jump tables, branch elimination, and similar tricks. Require evidence from realistic inputs representative of actual workloads. Note that "realistic" does not always mean "varied" — many real-world collections are small (under 64 elements), and data distributions are often domain-specific and non-uniform. -- **Performance claims in low-level or hardware-guided code may not need benchmarks.** When code follows official hardware vendor optimization recommendations or well-established algorithmic improvements, the systemic reasoning may be sufficient evidence. Microbenchmarks for such changes can be misleading because they don't capture system-level effects. -- **Investigate and explain regressions before merging.** Even if a PR shows a net improvement, regressions in specific scenarios must be understood and explicitly addressed — not hand-waved. - -### Approach & Alternatives - -- **Check whether the PR solves the right problem at the right layer.** Look for whether it addresses root cause or applies a band-aid. Prefer fixing the actual source of an issue over adding workarounds to production code. -- **When a PR takes a fundamentally wrong approach, redirect early.** Don't iterate on implementation details of a flawed design. Push back on the overall direction before the contributor invests more time. -- **Ask "Why not just X?" — always prefer the simplest solution.** When a PR uses a complex approach, challenge it with the simplest alternative that could work. The burden of proof is on the complex solution. - -### Cost-Benefit & Complexity - -- **Explicitly weigh whether the change is a net positive.** A performance trade-off that shifts costs around is not automatically beneficial. Demand clarity that the change is a win in the typical configuration, not just in a narrow scenario. -- **Reject overengineering — complexity is a first-class cost.** Unnecessary abstraction, extra indirections, and elaborate solutions for marginal gains are actively rejected. -- **Every addition creates a maintenance obligation.** Long-term maintenance cost outweighs short-term convenience. Code that is hard to maintain, increases surface area, or creates technical debt needs stronger justification. - -### Scope & Focus - -- **Require large or mixed PRs to be split into focused changes.** Each PR should address one concern. Mixed concerns make review harder and increase regression risk. -- **Defer tangential improvements to follow-up PRs.** Police scope creep by asking contributors to separate concerns. Even good ideas should wait if they're not part of the PR's core purpose. - -### Risk & Compatibility - -- **Flag breaking changes and require formal process.** Any behavioral change that could affect downstream consumers needs documentation, API review, and explicit approval — even when the change improves the codebase internally. -- **Assess regression risk proportional to the change's blast radius.** High-risk changes to stable code need proportionally higher value and more thorough validation. - -### Codebase Fit & History - -- **Ensure new code matches existing patterns and conventions.** Deviations from established patterns create confusion and inconsistency. If a rename or restructuring is warranted, do it uniformly in a dedicated PR — not piecemeal. -- **Check whether a similar approach has been tried and rejected before.** If a prior attempt didn't work, require a clear explanation of what's different this time. - -## Consistency with Codebase Patterns - -### PR Hygiene - -- **Keep PRs focused on their stated scope.** No accidental file modifications, no unrelated refactoring, no whitespace noise, no build artifacts. Each PR should serve a single purpose. -- **Do large refactorings and renames in separate PRs.** Separate no-diff refactors from functional changes. Mechanical renames should be separate from logic changes. -- **Merge to main first, then backport to release branches.** Use the `/backport` command. Backports to servicing are limited to security bugs, regressions, and reliability issues. Note: the reviewer should never invoke `/backport` itself — only recommend it when appropriate. - -### Code Reuse & Deduplication - -- **Extract duplicated logic into shared helper methods.** Fix improvements inside shared helpers so all callers benefit. -- **Move shared code to shared files, not duplicated across runtimes.** When identical code exists across CoreCLR and NativeAOT, move it to the shared partition (using `#if !MONO` if needed). -- **Use existing APIs instead of creating parallel ones.** Before introducing new types, enums, or helpers, check if existing ones serve the same purpose. Fix existing utilities rather than introducing duplicates. -- **Delete dead code and unused declarations aggressively.** When removing code, also remove helper methods, enum values, function declarations, and resx strings that are no longer used. - -### Established Conventions - -- **Store error strings in `.resx`, not inline code.** Reference via the `SR` class. When removing code that uses a resx string, delete the unused string entry. -- **Preserve existing alphabetical ordering in modified lists.** When a PR adds or reorders entries in an alphabetized list—especially items within a `.csproj` item group, such as `Compile`, `ProjectReference`, and `PackageReference`—verify that the changed entries preserve the surrounding order. Flag only ordering regressions introduced by the PR; do not require unrelated cleanup of pre-existing unsorted entries. This also applies to lists of areas, configuration entries, resx entries, entrypoint/export lists, and ref source members. -- **Don't modify auto-generated files or `eng/common` manually.** Change the generator or source definition instead. Files in `eng/common` are synced from dotnet/arcade. -- **Use `DOTNET_` prefix for environment variables, not `COMPlus_`.** New runtime environment variables must use `DOTNET_` exclusively. -- **Match existing style in modified files.** The existing style in a file takes precedence over general guidelines. Do not change existing code for style alone. - -### Runtime-Specific Patterns - -- **Consider NativeAOT parity for runtime changes.** When changing CoreCLR behavior, verify whether the same change is needed for NativeAOT. Note: Mono and CoreCLR native code conventions differ significantly — do not assume they share the same rules. -- **Keep interpreter behavior consistent with the regular JIT.** Follow the same patterns, naming, error codes (`CORJIT_BADCODE`), and macros (`NO_WAY`). Use `FEATURE_INTERPRETER` guards. -- **Source generators: no file locks, diagnostics from analyzers only.** Generators should bypass invalid state gracefully. A separate analyzer should produce diagnostics. -- **Ref assembly conventions.** No `using` directives (fully qualify types), empty method bodies or `throw null`, genapi-style formatting, alphabetical member order. TFM-specific APIs go in separate files. - -## Documentation & Comments - -- **Comments should explain why, not restate code.** Delete comments like `// Get the types` that just duplicate the code in English. Don't include historical context about why code changed. -- **Delete or update obsolete comments when corresponding code changes.** Stale comments describing old behavior are worse than no comments. Only flag obsolete comments when the relevant code is being touched or the PR is an explicit cleanup pass. -- **Track deferred work with GitHub issues and searchable TODOs.** Reference a tracking issue in TODO comments with a consistent prefix (e.g., `TODO-Async:`). Remove ancient TODOs that will never be addressed. -- **Don't duplicate comments on interface implementations.** Documentation comments belong on the interface definition. Implementations should use `` to avoid divergence. -- **Add XML doc comments on all new public APIs.** These seed the official API documentation on learn.microsoft.com. Properties should start with "Gets the ..." or "Gets or sets the ...". Do not add XML docs to test code. -- **Use SHA-specific or commit-based links in documentation.** Don't use branch-relative links that break when files move. -- **Reference specs and authoritative sources in implementation code.** When parsing signatures and metadata, cite the relevant spec section (e.g., ECMA-335). Link to relevant RFCs, papers, or repo-specific documentation (such as the ECMA-335 augments maintained in this repo). This applies broadly, not just to ECMA-335. -- **File breaking change documentation for behavioral changes.** Open an issue in dotnet/docs using the template, send notification to the .NET Breaking Change Notification DL. Applies even to prerelease-to-prerelease changes. -- **Use established terminology in user-facing text.** Do not expose internal type names, private field names, or codenames like "Roslyn" in public docs or error messages. -- **Retain copyright headers and license information.** All C# and C++ source files must include the standard license header, including test files. When porting from other projects, retain original copyright and update THIRD-PARTY-NOTICES.TXT. diff --git a/.github/instructions/review-all-tests.instructions.md b/.github/instructions/tests.instructions.md similarity index 77% rename from .github/instructions/review-all-tests.instructions.md rename to .github/instructions/tests.instructions.md index 9813c35a421804..5ad4e8fdc9d648 100644 --- a/.github/instructions/review-all-tests.instructions.md +++ b/.github/instructions/tests.instructions.md @@ -2,18 +2,14 @@ applyTo: "src/tests/**,**/tests/**" --- -# Code Review -- Tests +# Tests -Rules for reviewing test changes. Also apply `review-all-src` plus the language file for the code -under test (`review-csharp` or `review-native`). Note that test conventions differ across areas: -libraries tests (`src/libraries/**/tests/`) typically use xUnit with `[Fact]`/`[Theory]` and -`Assert.*`; JIT/runtime tests (`src/tests/`) often use a `return 100` success convention and -have different naming/priority requirements. Apply area-specific rules when they conflict with -general guidance below. - -These are review criteria. During code authoring or local experimentation, treat PR-level gates -such as motivation, benchmark evidence, and issue prerequisites as preparation guidance for a -ready-for-review PR, not as reasons to block exploratory work unless the user asks for review. +Conventions for test changes. Also apply `conventions` plus the language file for the code under +test (`csharp` or `native`). Note that test conventions differ across areas: libraries tests +(`src/libraries/**/tests/`) typically use xUnit with `[Fact]`/`[Theory]` and `Assert.*`; +JIT/runtime tests (`src/tests/`) often use a `return 100` success convention and have different +naming/priority requirements. Apply area-specific rules when they conflict with general guidance +below. ## Testing diff --git a/.github/skills/api-proposal/SKILL.md b/.github/skills/api-proposal/SKILL.md index 841fabeb875e54..6949afab51a0da 100644 --- a/.github/skills/api-proposal/SKILL.md +++ b/.github/skills/api-proposal/SKILL.md @@ -102,7 +102,8 @@ The skill contains baked-in examples and guidelines for writing good proposals ( 1. Create a new branch: `api-proposal/`. The prototype must be kept as a **single commit** on this branch. - Commit the initial prototype as a single commit. - - When addressing review feedback, amend the existing commit or squash locally and force-push so that the branch history remains a single commit. + - When addressing feedback **before publication**, amend the existing commit or squash locally and force-push so the branch history remains a single commit. This is the one place the repo's no-force-push rule doesn't apply — nothing links to the commit yet. + - After publication, use Phase 6 instead: a force-push invalidates the commit URL the proposal links to. 2. Implement the API surface with: - Complete triple-slash XML documentation on all public members @@ -133,11 +134,11 @@ The skill contains baked-in examples and guidelines for writing good proposals ( #### Prototype Validation (all steps required) -> **Prerequisite:** Follow the build and test workflow in [`copilot-instructions.md`](../../copilot-instructions.md) — complete the baseline build, configure the environment, and use the component-specific workflow for the target library. All build and test steps below assume the baseline build has already succeeded. +> **Prerequisite:** Follow the build and test workflow in the `build-and-test` skill — complete the baseline build, configure the environment, and use the component-specific workflow for the target library. All build and test steps below assume the baseline build has already succeeded. **Step 1: Build and test** -Build the src and test projects, then run all tests for the target library using the workflow described in `copilot-instructions.md`. All tests must pass with zero failures. +Build the src and test projects, then run all tests for the target library using the workflow described in the `build-and-test` skill. All tests must pass with zero failures. Building the test project separately is critical for detecting **source breaking changes** that ApiCompat won't catch: - New overloads/extension methods causing wrong method binding in existing code @@ -316,7 +317,7 @@ Present the complete draft to the user for review. Iterate based on feedback bef #### Step 1: Push and capture commit URL -Commit prototype changes and push the branch to the user's fork (default) or ask for an alternative remote. Capture the commit URL for inclusion in the proposal (e.g., `https://github.com///commit/`). +Commit the prototype changes, then push the branch to the user's fork — confirm first unless running non-interactively (see Step 2), or ask for an alternative remote. Capture the commit URL for inclusion in the proposal (e.g., `https://github.com///commit/`). #### Step 2: Non-interactive mode (Copilot Coding Agent) diff --git a/.github/skills/build-and-test/SKILL.md b/.github/skills/build-and-test/SKILL.md new file mode 100644 index 00000000000000..9f3603f3555d29 --- /dev/null +++ b/.github/skills/build-and-test/SKILL.md @@ -0,0 +1,132 @@ +--- +name: build-and-test +description: > + Build and test dotnet/runtime locally. Covers the mandatory baseline build, + per-component build and test commands, Windows/PowerShell equivalents, running + individual tests, and troubleshooting. USE FOR: any build or test invocation in + this repo, and before making code changes under CCA. DO NOT USE FOR: CI pipeline + triage (use ci-pipeline-monitor) or benchmarking (use performance-benchmark). +--- + +# Building & Testing in dotnet/runtime + +## Shell Conventions + +Commands below are written for bash. On Windows, use the `.cmd` entrypoints and PowerShell equivalents: + +| bash | Windows (PowerShell) | +|------|----------------------| +| `./build.sh ` | `.\build.cmd ` | +| `src/tests/build.sh ` | `src\tests\build.cmd ` | +| `src/tests/run.sh ` | `src\tests\run.cmd ` | +| `export FOO=` | `$env:FOO = ''` | +| `$CORE_ROOT/corerun .dll` | `& "$env:CORE_ROOT\corerun.exe" .dll` | +| `find -name ''` | `Get-ChildItem -Recurse -Filter ''` | +| `tail -20 ` | `Get-Content -Tail 20` | +| `grep '' ` | `Select-String -Path -Pattern ''` | + +Some arguments also differ in casing: `-priority1` on bash is `-Priority 1` on Windows. + +## Baseline Build + +A successful baseline build of the affected component is required for incremental builds and tests. Without it you'll hit "missing testhost" and "shared framework" errors that cost 20+ minutes per occurrence. If a baseline build fails, STOP, report the failure, and do not attempt to work around it. + +### When a baseline is required + +- **Under CCA — always, before changing any of the product code in the table below.** ⚠️ The environment is fresh, so there are no pre-existing artifacts and incremental builds fail in ways that waste significant compute. Skipping this step IS a task failure. Changes outside those paths — docs, markdown, workflow and instruction files — don't need one. **Exception:** if you are on a feature branch with commits upstream of main and the baseline build fails, make whatever code changes are needed to fix the build, then resume requiring a baseline. +- **Under CLI (interactive) — only when needed.** A usable baseline may already exist from prior work; don't re-run a 40-minute build unnecessarily. Check the component's [baseline sentinel](#baseline-sentinels) and build if it is missing. Otherwise attempt the work, and if it fails with a baseline-missing signature from [Troubleshooting](#troubleshooting), run the baseline once and retry — do not loop. Trust volunteered user signals ("just built", "fresh checkout") over probing. +- **Unsure which mode you're in?** Follow the CCA rule. + +### Step 1: Build the Baseline (from repo root) + +Pick the row matching the files you will modify: + +| Files Changed | Component | Baseline Build | +|---------------|-----------|----------------| +| `src/coreclr/` | CoreCLR | `./build.sh clr+libs+host` | +| `src/mono/` | Mono | `./build.sh mono+libs` | +| `src/libraries/` (no Browser/WASM or WASI targets) | Libraries | `./build.sh clr+libs -rc release` | +| `src/libraries/` with Browser/WASM or WASI targets in the affected `.csproj` | WASM/WASI Libraries | `./build.sh mono+libs -os browser` | +| `src/native/corehost/`, `src/installer/` | Host | `./build.sh clr+libs+host -rc release -lc release` | +| `src/tools`, `src/native/managed` | Tools | `./build.sh clr+libs -rc release` | +| `src/tasks` | Build Tasks | None — `./build.sh tasks` is self-contained | +| `src/tests` | Runtime Tests | `./build.sh clr+libs -lc release -rc checked` | + +**WASM/WASI Library Detection:** A change under `src/libraries/` is WASM/WASI-relevant if the library's `.csproj` has explicit Browser/WASM or WASI targets (`TargetFrameworks`, `TARGET_BROWSER`, `TARGET_WASI` constants, or `Condition` attributes referencing `browser`/`wasi`), **and** the changed file is not excluded from those targets via `Condition` on `` or ``. + +For System.Private.CoreLib changes, use `-rc checked` instead of `-rc release` for asserts. + +Build on the branch you intend to modify — the baseline reflects your working tree at that moment. Baselining up front requires a clean HEAD; baselining after a probe failure means either stashing work-in-progress changes first or accepting that the baseline incorporates them. + +⏱️ **This build can take up to 40 minutes.** Do not cancel unless no output for 5+ minutes. + +Redirect it to a log and poll a bounded view rather than watching the console: + +```bash +./build.sh clr+libs -rc release > artifacts/build.log 2>&1; echo "exit=$?" > artifacts/build.status +``` + +Then check `artifacts/build.status`, `tail -20 artifacts/build.log`, or grep the log for `: error`. +`artifacts/log/` holds the per-project binlogs for reporting failures. + +### Baseline sentinels + +A path under `artifacts/` whose absence means the baseline is missing. + +| Component | Sentinel | +|-----------|----------| +| Libraries | `artifacts/bin/testhost/` and `artifacts/bin/microsoft.netcore.app.runtime.//`. Building a single library usually works without one; running its tests does not. | +| CoreCLR | `artifacts/bin/coreclr/../`, plus `artifacts/tests/coreclr/../Tests/Core_Root/` to run tests | +| Mono | `artifacts/bin/mono/../`, plus `artifacts/tests/coreclr/../Tests/Core_Root/` to run tests (Mono reuses the Core_Root layout) | +| WASM Libraries | `artifacts/bin/microsoft.netcore.app.runtime.browser-wasm//` | +| Host | `artifacts/bin/coreclr/../` and `artifacts/bin/testhost/` | +| Tools | `artifacts/bin/coreclr/../` and `artifacts/bin/testhost/` | +| Build Tasks | None — `./build.sh tasks` is self-contained | +| Runtime Tests | `artifacts/tests/coreclr/../Tests/Core_Root/`, produced by the baseline build plus `src/tests/build.sh -GenerateLayoutOnly` | + +### Step 2: Configure Environment + +```bash +export PATH="$(pwd)/.dotnet:$PATH" +dotnet --version # Should match sdk.version in global.json +``` + +On Windows: `$env:PATH = "$PWD\.dotnet;$env:PATH"` + +--- + +## Component Workflows + +Read only the file for the component you are working on. All commands must complete with exit +code 0, and all tests must pass with zero failures. + +| Component | Workflow | +|-----------|----------| +| Libraries, WASM/WASI Libraries | [`components/libraries.md`](components/libraries.md) | +| CoreCLR, Mono | [`components/runtime.md`](components/runtime.md) | +| Runtime Tests (`src/tests`) | [`components/runtime-tests.md`](components/runtime-tests.md) | +| Host, Tools, Build Tasks | [`components/host-and-tools.md`](components/host-and-tools.md) | + +--- + +## Adding new tests + +When creating a regression test for a bug fix: + +1. **Verify the test FAILS without the fix** — build and run against the unfixed code. +2. **Verify the test PASSES with the fix** — apply the fix, rebuild, and run again. +3. If the fix is not yet merged locally, manually apply the minimal changes from the PR/commit to verify. + +Do not mark a regression test task as complete until both conditions are confirmed. + +## Troubleshooting + +| Error | Solution | +|-------|----------| +| "shared framework must be built" | Run baseline build: `./build.sh clr+libs -rc release` | +| "testhost" missing / FileNotFoundException | Run baseline build first (Step 1 above) | +| Build timeout | Wait up to 40 min; only fail if no output for 5 min | +| "Target does not exist" | Avoid specifying a target framework; the build will auto-select `$(NetCoreAppCurrent)` | +| "0 test projects" after `build.sh -Test` | The test has `` > 0; add `-priority1` to the build command | + +**When reporting failures:** Include logs from `artifacts/log/` and console output for diagnostics. diff --git a/.github/skills/build-and-test/components/host-and-tools.md b/.github/skills/build-and-test/components/host-and-tools.md new file mode 100644 index 00000000000000..7ade65942835d9 --- /dev/null +++ b/.github/skills/build-and-test/components/host-and-tools.md @@ -0,0 +1,27 @@ +# Host, Tools, and Build Tasks + +Covers `src/native/corehost/`, `src/installer/`, `src/tools`, `src/native/managed`, and +`src/tasks`. Confirm the component's [baseline sentinel](../SKILL.md#baseline-sentinels) first. + +## Host + +**Build:** `./build.sh host -rc release -lc release` + +**Test:** `./build.sh host.tests -rc release -lc release -test` + +## Tools + +**Build:** `./build.sh tools+tools.ilasm` + +**Test:** `./build.sh tools+tools.ilasm+tools.illinktests+tools.cdactests -test` + +## Build Tasks + +**Build:** `./build.sh tasks` + +No baseline is required — this is self-contained. If you go on to consume the tasks from a +workflow that does need one (e.g. libraries tests), apply that workflow's sentinel instead. + +## Reference + +- [Host Tests](/docs/workflow/testing/host/testing.md) diff --git a/.github/skills/build-and-test/components/libraries.md b/.github/skills/build-and-test/components/libraries.md new file mode 100644 index 00000000000000..beac1b19fe638b --- /dev/null +++ b/.github/skills/build-and-test/components/libraries.md @@ -0,0 +1,36 @@ +# Libraries + +Covers `src/libraries/`, including Browser/WASM and WASI targets. Confirm the component's +[baseline sentinel](../SKILL.md#baseline-sentinels) before running tests. + +## Libraries (most common) + +**Build and test a specific library:** + +```bash +cd src/libraries/ +dotnet build +dotnet build /t:test ./tests/.csproj +``` + +Test projects are typically at `tests/.Tests.csproj` or +`tests/.Tests/.Tests.csproj`, or under `tests/FunctionalTests/`, +`tests/UnitTests/`, etc. Use `find tests -name '*.Tests.csproj'` to discover them. + +Before completing, ensure ALL tests for the affected libraries pass — not just the one project +you touched. + +**Test all libraries:** `./build.sh libs.tests -test -rc release` + +**System.Private.CoreLib:** rebuild with `./build.sh clr.corelib+clr.nativecorelib+libs.pretest -rc checked` + +## WASM / WASI Libraries + +**Build:** `./build.sh libs -os browser` + +**Test:** `./build.sh libs.tests -test -os browser` + +## Reference + +- [Build Libraries](/docs/workflow/building/libraries/README.md) · [Test Libraries](/docs/workflow/testing/libraries/testing.md) +- [WASM Build](/docs/workflow/building/libraries/webassembly-instructions.md) · [WASM Test](/docs/workflow/testing/libraries/testing-wasm.md) diff --git a/.github/skills/build-and-test/components/runtime-tests.md b/.github/skills/build-and-test/components/runtime-tests.md new file mode 100644 index 00000000000000..54d1656c8ff080 --- /dev/null +++ b/.github/skills/build-and-test/components/runtime-tests.md @@ -0,0 +1,51 @@ +# Runtime Tests + +Covers `src/tests/`. Confirm the component's [baseline sentinel](../SKILL.md#baseline-sentinels) +first — running an individual test additionally requires the Core_Root layout. + +Subdirectories under `src/tests/` may contain `README.md` files with area-specific guidance +(e.g. EventPipe test patterns). + +**Build all tests:** + +```bash +./build.sh clr+libs -lc release -rc checked +./src/tests/build.sh checked +./src/tests/run.sh checked +``` + +**Build a single test project** (path is relative to the repo root): + +```bash +# Use -priority1 ("-Priority 1" on Windows) for tests with 1, +# otherwise the build silently reports "0 test projects" and builds nothing. +src/tests/build.sh -Test tracing/eventpipe/eventsvalidation/GCEvents.csproj x64 Release -priority1 +``` + +Other useful flags (run `src/tests/build.sh -h` for the full list): + +| Flag | Description | +|------|-------------| +| `-Test ` | Build one project | +| `-Dir ` | Build all projects in a directory | +| `-Tree ` | Build a subtree recursively | +| `-priority1` (`-Priority 1` on Windows) | Include priority 1 tests | +| `-GenerateLayoutOnly` | Generate Core_Root layout only | + +**Generate Core_Root layout** (required before running individual tests): + +```bash +src/tests/build.sh -GenerateLayoutOnly x64 Release +``` + +**Run a single test:** + +```bash +export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/.x64.Release/Tests/Core_Root +cd artifacts/tests/coreclr/.x64.Release// +$CORE_ROOT/corerun .dll +# Exit code 100 = pass, any other value = fail. +``` + +On Windows: `$env:CORE_ROOT = "$PWD\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root"`, +then run `& "$env:CORE_ROOT\corerun.exe" .dll` from the test's output directory. diff --git a/.github/skills/build-and-test/components/runtime.md b/.github/skills/build-and-test/components/runtime.md new file mode 100644 index 00000000000000..4bc9fe15a13d27 --- /dev/null +++ b/.github/skills/build-and-test/components/runtime.md @@ -0,0 +1,26 @@ +# CoreCLR and Mono + +Covers `src/coreclr/` and `src/mono/`. Confirm the component's +[baseline sentinel](../SKILL.md#baseline-sentinels) first. + +## CoreCLR + +**Test:** `cd src/tests && ./build.sh && ./run.sh` + +## Mono + +**Test:** + +```bash +./build.sh clr.host +cd src/tests +./build.sh mono debug /p:LibrariesConfiguration=debug +./run.sh +``` + +For building or running an individual runtime test, see [`runtime-tests.md`](runtime-tests.md). + +## Reference + +- [Build CoreCLR](/docs/workflow/building/coreclr/README.md) · [Test CoreCLR](/docs/workflow/testing/coreclr/testing.md) +- [Build Mono](/docs/workflow/building/mono/README.md) · [Test Mono](/docs/workflow/testing/mono/testing.md) diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 57c6c3c58f2212..2fd41e4b3f6413 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -21,14 +21,14 @@ Use this skill when: ### Step 0: Load Relevant Instructions -Before analyzing anything, load any and all instructions under `.github/instructions` that are relevant to the code changes, as indicated by the frontmatter. If conflict arises between said custom instructions and the instructions in this skill, the custom instructions supersede instructions in this skill. +Before analyzing anything, load `.github/skills/code-review/pr-assessment.md` -- the holistic criteria you will assess against -- plus any and all instructions under `.github/instructions` that are relevant to the code changes, as indicated by the frontmatter. If conflict arises between said custom instructions and the instructions in this skill, the custom instructions supersede instructions in this skill. ### Step 1: Gather Code Context (No PR Narrative Yet) Before analyzing anything, collect as much relevant **code** context as you can. **Critically, do NOT read the PR description, linked issues, or existing review comments yet.** You must form your own independent assessment of what the code does, why it might be needed, what problems it has, and whether the approach is sound — before being exposed to the author's framing. Reading the author's narrative first anchors your judgment and makes you less likely to find real problems. 1. **Diff and file list**: Fetch the full diff and the list of changed files. -2. **Full source files**: For every changed file, read the **entire source file** (not just the diff hunks). You need the surrounding code to understand invariants, locking protocols, call patterns, and data flow. Diff-only review is the #1 cause of false positives and missed issues. +2. **Full source files**: For every changed file, read well beyond the diff hunks — diff-only review is the #1 cause of false positives and missed issues. You need the surrounding code to understand invariants, locking protocols, call patterns, and data flow. Read the whole file when it is a reasonable size; for the very large ones this repo has (`morph.cpp` alone is ~137k tokens) read the enclosing functions, their callers, and the types involved instead of paging through the entire file. 3. **Consumers and callers**: If the change modifies a public/internal API, a type that others depend on, or a virtual/interface method, search for how consumers use the functionality. Grep for callers, usages, and test sites. Understanding how the code is consumed reveals whether the change could break existing behavior or violate caller assumptions. 4. **Sibling types and related code**: If the change fixes a bug or adds a pattern in one type, check whether sibling types (e.g., other abstraction implementations, other collection types, platform-specific variants) have the same issue or need the same fix. Fetch and read those files too. 5. **Key utility/helper files**: If the diff calls into shared utilities, read those to understand the contracts (thread-safety, idempotency, etc.). @@ -37,7 +37,7 @@ Before analyzing anything, collect as much relevant **code** context as you can. - Changes to `ref/` assembly source files (the strongest signal — these define the public API contract) - New `public` members (methods, properties, types, enum values) in `src/` files - Note whether new public API was detected. If it was, you **MUST** load and execute the API approval verification procedure during Step 4. Read the file `.github/skills/code-review/api-approval-check.md` (relative to the repository root) and follow its instructions. Do not skip this step — it is blocking. - + ### Step 2: Discover Area-Specific Agents - If the environment supports sub-agent or task invocation, study the **review** agents that actually exist in `.github/agents`. Their yaml frontmatter descriptions tell when they apply. @@ -60,7 +60,7 @@ Based **only** on the code context gathered above (without the PR description or 3. **Is this the right approach?** Would a simpler alternative be more consistent with the codebase? Could the goal be achieved with existing functionality? Are there correctness, performance, or safety concerns? 4. **What problems do you see?** Identify bugs, edge cases, missing validation, thread-safety issues, performance regressions, API design problems, test gaps, and anything else that concerns you. -Write down your independent assessment before proceeding. You must produce a holistic assessment (using the criteria from the applicable `.github/instructions/*.instructions.md` files for the diff) at this stage. +Write down your independent assessment before proceeding. You must produce a holistic assessment (using the criteria in `.github/skills/code-review/pr-assessment.md`) at this stage. ### Step 4: Incorporate PR Narrative and Reconcile @@ -97,7 +97,7 @@ Now read the PR description, labels, linked issues (in full), author information ## Multi-Model Review -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. +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. Each sub-agent re-reads the diff and the surrounding files, so this multiplies the cost of the review by the number of models — worth it for a substantial or risky change, not for a one-line fix. **How to execute (when supported):** 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. **Model selection rules:** @@ -179,20 +179,23 @@ The summary verdict **must** be consistent with the findings in the body. Follow ## Where the Review Rules Live -The detailed review rules -- correctness, performance, API design, style, testing, -documentation, native/interop, and the Holistic PR Assessment criteria -- are maintained -as path-specific instruction files under `.github/instructions/` so that the built-in -Copilot code reviewer and this skill share a single source of truth. **You MUST load the -files whose `applyTo` paths match the diff and treat them as the rule set for this -review**, in addition to the process above. +Review rules are split by activity: + +- **`.github/skills/code-review/pr-assessment.md`** -- the Holistic PR Assessment criteria: + reviewer mindset, Motivation, Evidence, Approach, Cost-Benefit, Scope, Risk, and Codebase Fit. + **Always load this.** Use it to write the Motivation, Approach, and Summary fields of your output. +- **`.github/instructions/*.instructions.md`** -- the code conventions themselves, shared with code + authoring and with the built-in Copilot code reviewer so there is a single source of truth. + **You MUST load the files whose `applyTo` paths match the diff and treat them as the rule set + for this review**, in addition to the process above. Load, based on the paths in the diff: -- **`src/**` changed:** `.github/instructions/review-all-src.instructions.md` -- reviewer mindset, the Holistic PR Assessment criteria (Motivation, Evidence, Approach, Cost-Benefit, Scope, Risk, Codebase Fit), correctness philosophy, PR hygiene, consistency, and documentation. Use these criteria to write the Motivation, Approach, and Summary fields in your output. -- **`**/*.cs` changed:** `.github/instructions/review-csharp.instructions.md` -- C# error handling, thread safety, security, correctness, performance/allocation, API design, and style rules. -- **Native files (`*.c` / `*.cpp` / `*.h` / `*.inc` / `*.S` / `*.asm`) changed:** `.github/instructions/review-native.instructions.md` -- C++ style, VM/JIT contracts, GC protection, platform defines, and interop/marshalling rules. -- **Test files (`**/tests/**`, `src/tests/**`) changed:** `.github/instructions/review-all-tests.instructions.md` -- testing conventions and regression-test requirements. -- **Area matches:** also load any matching area file under `.github/instructions/` (for example `.github/instructions/review-core-runtime.instructions.md`, `.github/instructions/jit.instructions.md`, `.github/instructions/system-net-*.instructions.md`, `.github/instructions/extensions-*.instructions.md`, `.github/instructions/compression.instructions.md`, `.github/instructions/cdac.instructions.md`). These stack on top of the language rules. An area instruction file does not imply that a corresponding agent exists; invoke an area **agent** under `.github/agents/` only when it actually exists and applies, as described in Step 2. +- **`src/**` changed:** `.github/instructions/conventions.instructions.md` -- change scope, code reuse and deduplication, established conventions, runtime-specific patterns, documentation and comments. +- **`**/*.cs` changed:** `.github/instructions/csharp.instructions.md` -- C# error handling, thread safety, security, correctness, performance/allocation, API design, and style rules. +- **Native files (`*.c` / `*.cpp` / `*.h` / `*.inc` / `*.S` / `*.asm`) changed:** `.github/instructions/native.instructions.md` -- C++ style, VM/JIT contracts, GC protection, platform defines, and interop/marshalling rules. +- **Test files (`**/tests/**`, `src/tests/**`) changed:** `.github/instructions/tests.instructions.md` -- testing conventions and regression-test requirements. +- **Area matches:** also load any matching area file under `.github/instructions/` (for example `.github/instructions/core-runtime.instructions.md`, `.github/instructions/jit.instructions.md`, `.github/instructions/system-net-*.instructions.md`, `.github/instructions/extensions-*.instructions.md`, `.github/instructions/compression.instructions.md`, `.github/instructions/cdac.instructions.md`). These stack on top of the language rules. An area instruction file does not imply that a corresponding agent exists; invoke an area **agent** under `.github/agents/` only when it actually exists and applies, as described in Step 2. - **Content matches (not path-based):** if the diff uses `Vector128`/`Vector256`/`Vector512`, `Vector`, or `System.Runtime.Intrinsics.*` anywhere, apply the `vectorization` skill in addition to the above. SIMD code appears in arbitrary library files, so this trigger is keyed on content, not folder. If a rule in a more specific file conflicts with a general one, the more specific file diff --git a/.github/skills/code-review/pr-assessment.md b/.github/skills/code-review/pr-assessment.md new file mode 100644 index 00000000000000..03b9b8c1128430 --- /dev/null +++ b/.github/skills/code-review/pr-assessment.md @@ -0,0 +1,53 @@ +# Holistic PR Assessment + +Review-only criteria: these apply when assessing a pull request, not when authoring code. The +coding rules themselves live in the path-specific instruction files under `.github/instructions/` +(see `## Where the Review Rules Live` in `SKILL.md`). + +Use the criteria below to write the Motivation, Approach, and Summary fields of your review output. + +**Reviewer mindset:** Be polite but very skeptical. Your job is to help speed the review process for maintainers, which includes not only finding problems the PR author may have missed but also questioning the value of the PR in its entirety. Treat the PR description and linked issues as claims to verify, not facts to accept. Question the stated direction, probe edge cases, and don't hesitate to flag concerns even when unsure. + +Before reviewing individual lines of code, evaluate the PR as a whole. Consider whether the change is justified, whether it takes the right approach, and whether it will be a net positive for the codebase. + +## Motivation & Justification + +- **Every PR must articulate what problem it solves and why.** Don't accept vague or absent motivation. Ask "What's the rationale?" if none is provided. However, when the PR links to an approved API proposal, accepted issue, or prior discussion that already establishes motivation, referencing that is sufficient — don't demand the author re-state what's already documented. +- **Challenge every addition with "Do we need this?"** New code, APIs, abstractions, and flags must justify their existence. If an addition can be avoided without sacrificing correctness or meaningful capability, it should be. +- **Demand real-world use cases and customer scenarios.** Hypothetical benefits are insufficient motivation for expanding API surface area or adding features. Require evidence that real users need this. +- **Core component changes should start with an issue.** Changes to host, VM, or JIT should start with a GitHub issue describing the problem and motivation before submitting a PR. + +## Evidence & Data + +- **Require measurable performance data before accepting optimization PRs.** Demand BenchmarkDotNet results or equivalent proof — never accept performance claims at face value. Prefer local BenchmarkDotNet runs first, especially for experimental/iterative work. EgorBot runs on an individual's personal account and is not billed like Copilot usage — only recommend it when explicitly requested, or for a final cross-architecture (x64/arm64) confirmation that cannot be reproduced locally. +- **Distinguish real performance wins from micro-benchmark noise.** Trivial benchmarks with predictable inputs overstate gains from jump tables, branch elimination, and similar tricks. Require evidence from realistic inputs representative of actual workloads. Note that "realistic" does not always mean "varied" — many real-world collections are small (under 64 elements), and data distributions are often domain-specific and non-uniform. +- **Performance claims in low-level or hardware-guided code may not need benchmarks.** When code follows official hardware vendor optimization recommendations or well-established algorithmic improvements, the systemic reasoning may be sufficient evidence. Microbenchmarks for such changes can be misleading because they don't capture system-level effects. +- **Investigate and explain regressions before merging.** Even if a PR shows a net improvement, regressions in specific scenarios must be understood and explicitly addressed — not hand-waved. + +## Approach & Alternatives + +- **Check whether the PR solves the right problem at the right layer.** Look for whether it addresses root cause or applies a band-aid. Prefer fixing the actual source of an issue over adding workarounds to production code. +- **When a PR takes a fundamentally wrong approach, redirect early.** Don't iterate on implementation details of a flawed design. Push back on the overall direction before the contributor invests more time. +- **Ask "Why not just X?" — always prefer the simplest solution.** When a PR uses a complex approach, challenge it with the simplest alternative that could work. The burden of proof is on the complex solution. + +## Cost-Benefit & Complexity + +- **Explicitly weigh whether the change is a net positive.** A performance trade-off that shifts costs around is not automatically beneficial. Demand clarity that the change is a win in the typical configuration, not just in a narrow scenario. +- **Reject overengineering — complexity is a first-class cost.** Unnecessary abstraction, extra indirections, and elaborate solutions for marginal gains are actively rejected. +- **Every addition creates a maintenance obligation.** Long-term maintenance cost outweighs short-term convenience. Code that is hard to maintain, increases surface area, or creates technical debt needs stronger justification. + +## Scope & Focus + +- **Require large or mixed PRs to be split into focused changes.** Each PR should address one concern. Mixed concerns make review harder and increase regression risk. +- **Defer tangential improvements to follow-up PRs.** Police scope creep by asking contributors to separate concerns. Even good ideas should wait if they're not part of the PR's core purpose. + +## Risk & Compatibility + +- **Flag breaking changes and require formal process.** Any behavioral change that could affect downstream consumers needs documentation, API review, and explicit approval — even when the change improves the codebase internally. +- **Assess regression risk proportional to the change's blast radius.** High-risk changes to stable code need proportionally higher value and more thorough validation. +- **Never invoke `/backport` yourself.** Recommend it when appropriate and leave the command to a maintainer. + +## Codebase Fit & History + +- **Ensure new code matches existing patterns and conventions.** Deviations from established patterns create confusion and inconsistency. If a rename or restructuring is warranted, do it uniformly in a dedicated PR — not piecemeal. +- **Check whether a similar approach has been tried and rejected before.** If a prior attempt didn't work, require a clear explanation of what's different this time. diff --git a/.github/skills/system-net-review/SKILL.md b/.github/skills/system-net-review/SKILL.md index 65294e49a636a4..78375eb89197f0 100644 --- a/.github/skills/system-net-review/SKILL.md +++ b/.github/skills/system-net-review/SKILL.md @@ -9,7 +9,7 @@ This skill provides **implementation guidance** when writing or modifying code u ## Scope -Applies to all `System.Net.*` libraries, `System.Private.Uri`, and shared networking code in `Common/src`. For general dotnet/runtime conventions (style, builds, testing workflow), defer to the repo-level `copilot-instructions.md` and the `code-review` skill. +Applies to all `System.Net.*` libraries, `System.Private.Uri`, and shared networking code in `Common/src`. For general dotnet/runtime conventions, defer to the path-scoped files under `.github/instructions/` (style, tests), the `build-and-test` skill (build and test workflow), and the `code-review` skill. ### Review Delegation @@ -263,7 +263,7 @@ System.Net functional tests use in-process loopback servers to avoid real networ - Tests must clean up all sockets, streams, and servers — wrap in `using`/`await using`. - Stress tests live under `tests/StressTests/`; don't add long-running loops to functional tests. -- General test conventions (prefer existing files, `[Theory]` over `[Fact]`) are in `copilot-instructions.md`. +- General test conventions (prefer existing files, `[Theory]` over `[Fact]`) are in `.github/instructions/tests.instructions.md`. --- diff --git a/.github/skills/update-os-coverage/SKILL.md b/.github/skills/update-os-coverage/SKILL.md index ca4fcaba69aa93..e899c67a207204 100644 --- a/.github/skills/update-os-coverage/SKILL.md +++ b/.github/skills/update-os-coverage/SKILL.md @@ -15,7 +15,7 @@ Update OS version references in Helix queue definition files. These files contro ## Prerequisites -> **Baseline build not required:** This skill is for YAML/docs-style queue and image reference updates, not product code changes. Do **not** start with the repo-wide baseline build workflow from [`copilot-instructions.md`](../../copilot-instructions.md) unless the task expands beyond image / queue metadata into code changes that actually need build or test validation. +> **Baseline build not required:** This skill is for YAML/docs-style queue and image reference updates, not product code changes. Do **not** start with the repo-wide baseline build workflow from the `build-and-test` skill unless the task expands beyond image / queue metadata into code changes that actually need build or test validation. ## When to use diff --git a/.github/skills/vectorization/SKILL.md b/.github/skills/vectorization/SKILL.md index fa1ac2fda760e4..5dce6175ecaea7 100644 --- a/.github/skills/vectorization/SKILL.md +++ b/.github/skills/vectorization/SKILL.md @@ -76,9 +76,8 @@ This skill distills what to actually enforce when authoring or reviewing vectori - **Toggle acceleration via environment variables** (can't be done at the unit-test level): run the suite with no overrides, with `DOTNET_EnableAVX2=0` (disables `Vector256`), and with `DOTNET_EnableHWIntrinsic=0` (disables all intrinsics down to the software fallback). Build the - affected library and run its test project per the build/test workflow in - [`.github/copilot-instructions.md`](/.github/copilot-instructions.md), with the relevant - `DOTNET_Enable*` variable set in the environment. + affected library and run its test project per the build/test workflow in the `build-and-test` + skill, with the relevant `DOTNET_Enable*` variable set in the environment. - **Guard against out-of-bounds reads with `BoundedMemory`.** [`BoundedMemory.Allocate(count)`](/src/libraries/Common/tests/TestUtilities/System/Buffers/BoundedMemory.Creation.cs) places a no-access page immediately after the buffer (use `PoisonPagePlacement.Before` for