diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 40d9b794ed3a..f3605c760122 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -119,17 +119,27 @@ Write the plan to `plan.md` in your session folder (see the session context) and **Post the plan and wait for approval before writing the implementing diff** — the plan is meant to be reviewed now, not after a large diff already exists. -Then implement, keeping the diff **scoped and reviewable** — prefer the smallest change that fully addresses the task over a broad refactor. If the plan changes materially while implementing, update it rather than silently diverging. Only after the plan's **Acceptance** and **Validation** are satisfied (and the Validation Checklist below passes) is the work "done." +Then implement, keeping the diff **scoped and reviewable** — prefer the smallest change that fully addresses the task over a broad refactor. If the plan changes materially while implementing, update it rather than silently diverging. Only after the plan's **Acceptance** and **Validation** are satisfied (and the Definition of Done below passes) is the work "done." -Trivial changes don't need a written plan — go straight to the Validation Checklist. +Trivial changes don't need a written plan — go straight to the Definition of Done. -## Validation Checklist +### Keep changes reviewable -When making changes: -1. **Read `.github/memory/INDEX.md` first.** -2. For non-trivial tasks, read `ARCHITECTURE.md` and `CONVENTIONS.md`, and the `.github/instructions/.instructions.md` for the area you're editing. -3. **Build the specific project(s) modified** (`Compilers.slnf` / `Ide.slnf` / `Razor.slnf` / the project). -4. **Run targeted tests** for affected test project(s). -5. If you edited a `.resx`, run `/t:UpdateXlf`; if you edited Syntax/BoundNodes XML, regenerate code. Update `PublicAPI.Unshipped.txt` for public API changes. -6. Follow existing patterns in similar files. -7. **Doc pass** (mandatory) — run the `update-agent-docs` skill and apply the Doc Update Obligation above. +- Keep each change focused on one coherent concern. Do not mix behavior changes with unrelated cleanup, broad renames, or opportunistic refactoring. +- Split work when parts can be reviewed, validated, merged, or reverted independently; when they affect unrelated areas or owners; or when a preparatory refactoring can land before the behavior change. +- Checkpoint after each independently valid slice rather than accumulating one large unreviewed diff. Each checkpoint must build on the previous one and leave the branch in a coherent state. +- Do not optimize for an arbitrary line-count limit: generated files and mechanical updates can be large. Optimize for reviewer cognitive load, clear intent, and independent validation. +- If a change cannot be split without making it less correct or harder to validate, explain that constraint in the plan and keep the commits logically separated. + +## Definition of Done + +Work is done only when every applicable step below is complete: + +1. **Format:** Run the repository's existing formatter for changed files when applicable. +2. **Lint/analyzers:** Run the smallest existing lint or analyzer command that covers the changed files when applicable. +3. **Build:** Build the specific affected project or solution filter (`Compilers.slnf`, `Ide.slnf`, `Razor.slnf`, or the project). Documentation-only changes do not require a product build. +4. **Targeted tests:** Run the affected test project or focused test filter. Add or update tests when behavior changes; explain when no relevant automated test exists. +5. **Generated/resource/API updates:** Regenerate Syntax/BoundNodes outputs when their XML changes, run `/t:UpdateXlf` after `.resx` edits, and update `PublicAPI.Unshipped.txt` for public API changes. +6. **Diff review:** Review the final diff and confirm it matches the approved plan, contains no unrelated edits, and follows nearby patterns. +7. **Docs:** Run the `update-agent-docs` skill and apply the Doc Update Obligation above. +8. **Final evidence:** Inspect repository status and the final diff, then report the exact validation performed. Do not claim completion while required validation is failing or was silently skipped. diff --git a/.github/memory/CONVENTIONS.md b/.github/memory/CONVENTIONS.md index 27963c8ac732..e7b05f7e2065 100644 --- a/.github/memory/CONVENTIONS.md +++ b/.github/memory/CONVENTIONS.md @@ -41,6 +41,12 @@ var semanticModel = await document.GetSemanticModelAsync(cancellationToken); var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken); ``` +### Reviewable changes and completion + +- Keep each change focused on one coherent concern. Split independently reviewable, validatable, mergeable, or revertible work instead of combining it into a broad diff. +- Judge change size by cognitive load and validation boundaries, not an arbitrary line count; generated and mechanical updates may be large while still representing one focused change. +- A change is complete only after applicable formatting, analyzers, affected builds, targeted tests, generated/resource/API updates, final diff review, and documentation freshness work are complete. The canonical ordered checklist is the **Definition of Done** in `.github/copilot-instructions.md`. + ## Patterns Explicitly Avoided - **No `TODO` or `TODO2` comments** — CI correctness leg flags `TODO`. Track follow-up work as a GitHub issue and link it in code (e.g. `// https://github.com/dotnet/roslyn/issues/NNNN`). Existing `TODO2` markers are a frozen baseline from when enforcement started, not a pattern to follow. diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 9206eefe13f2..ecbcb93c18a3 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -80,6 +80,8 @@ If the PR is a pure or targeted revert of an earlier commit, adjust the review s - **Never assert that something "does not exist," "is deprecated," or "is unavailable" based on training data alone.** Your knowledge has a cutoff date. When uncertain, ask rather than assert. 9. **Ensure code suggestions are valid.** Any code you suggest must be syntactically correct and complete. Ensure any suggestion would result in working code. 10. **Label in-scope vs. follow-up.** Distinguish between issues the PR should fix and out-of-scope improvements. Be explicit when a suggestion is a follow-up rather than a blocker. +11. **Enforce reviewable scope.** Verify that the PR addresses one coherent concern. Flag unrelated cleanup, behavior changes mixed with broad refactoring, or independently mergeable/revertible work combined into one diff. Judge size by reviewer cognitive load and validation boundaries, not an arbitrary line count; generated files and mechanical updates may legitimately be large. +12. **Require validation evidence.** Check that the PR reports the applicable formatting, lint/analyzer, affected build, targeted tests, generated/resource/API updates, and documentation freshness work. Treat missing targeted tests for a behavior change or required validation that was skipped without explanation as merge-blocking. Do not demand product builds or tests for documentation-only changes. ### Step 4: Documentation Freshness Check (sub-agent) @@ -182,6 +184,10 @@ Before reviewing individual lines of code, evaluate the PR as a whole. Consider - **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. +- **Use concrete split signals.** Request a split when changes can be reviewed, validated, merged, or reverted independently; touch unrelated ownership areas; or combine preparatory refactoring with a behavior change. Do not request a split solely because generated or mechanical files make the line count large. + +- **Check the Definition of Done evidence.** Confirm the author identifies the relevant format/lint, affected build, targeted tests, generated/resource/API work, and documentation freshness work. Missing evidence must be supplied or explicitly justified before approval. + - **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. - **Consider separating bug fixes from feature additions.** It's not uncommon for feature work to reveal existing issues. When that happens, consider whether the bug fix should be merged independently of the feature work and directly to the `main` branch. It's also good to record the existence of the bug in an issue if it doesn't already exist, so that the fix can be tracked and backported as needed.