cDAC review instructions: require doc updates for contract/data-descriptor changes#130532
Open
max-charlamb wants to merge 2 commits into
Open
cDAC review instructions: require doc updates for contract/data-descriptor changes#130532max-charlamb wants to merge 2 commits into
max-charlamb wants to merge 2 commits into
Conversation
…iptor changes Copilot PR reviews for cDAC changes have repeatedly missed cases where docs/design/datacontracts/<Name>.md needed to be updated alongside the code. Add an explicit "Documentation updates" section listing the code-change patterns that require a matching doc update, and broaden applyTo so the instructions load for docs/design/datacontracts/** and every src/coreclr/**/datadescriptor/** directory (vm, gc, nativeaot/Runtime). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the cDAC folder-specific Copilot review instructions so they also apply to cDAC contract specification docs and CoreCLR data-descriptor sources, and adds an explicit checklist to require corresponding docs/design/datacontracts/<Name>.md updates when contract/data-descriptor-related code changes.
Changes:
- Expanded the
applyToscope to includedocs/design/datacontracts/**andsrc/coreclr/**/datadescriptor/**in addition tosrc/native/managed/cdac/**. - Added a short “Documentation updates” section enumerating contract/data-descriptor change patterns that must be accompanied by doc updates, with explicit exclusions.
rcj1
reviewed
Jul 10, 2026
| - **`Contracts/Contracts/<Name>_<N>.cs`** — a new version file (needs a new `## Version N` section), or a semantic change to an existing version's algorithm. Pure refactors don't need doc updates. | ||
| - **New contract** (new `I<Name>.cs` + `<Name>_1.cs`) — needs a new `docs/design/datacontracts/<Name>.md`. | ||
| - **`src/coreclr/**/datadescriptor/**`** — every consuming contract's doc must reflect added/removed/renamed types, fields, or globals. | ||
| - **New `Contracts/Data/<Type>.cs`** or new `Target.ReadGlobal*` in a contract impl — the consuming contract's doc must document it. |
Contributor
There was a problem hiding this comment.
This might trigger extraneously when the doc is in a more prose style. I think we already have the changes we want captured in “semantic change to an existing version’s algorithm”.
steveisok
approved these changes
Jul 10, 2026
…or wording Per review feedback (rcj1): the Data/<Type>.cs + Target.ReadGlobal* rule was redundant with 'semantic change to an existing version's algorithm' and risked false positives on contract docs written in prose style (rather than a strict 'list every data structure' style). Drop it, and soften the datadescriptor bullet from 'must reflect' to 'may need a matching update ... check whichever contracts' algorithms are affected' for the same reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+40
to
+44
| - **`Abstractions/Contracts/I<Name>.cs`** — added, removed, or renamed methods, or new exposed types/enums. | ||
| - **`Contracts/Contracts/<Name>_<N>.cs`** — a new version file (needs a new `## Version N` section), or a semantic change to an existing version's algorithm. Pure refactors don't need doc updates. | ||
| - **New contract** (new `I<Name>.cs` + `<Name>_1.cs`) — needs a new `docs/design/datacontracts/<Name>.md`. | ||
| - **`src/coreclr/**/datadescriptor/**`** — added, removed, or renamed types, fields, or globals may need a matching update in every consuming contract's doc; check whichever contracts' algorithms are affected. | ||
|
|
steveisok
approved these changes
Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves the cDAC Copilot PR-review instructions so reviews consistently flag missing doc updates for contract and data-descriptor changes.
Motivation
Copilot PR reviews on cDAC changes have repeatedly missed cases where
docs/design/datacontracts/<Name>.mdshould have been updated alongside the code. The doc underdocs/design/datacontracts/is the authoritative spec of each contract (1:1 withAbstractions/Contracts/I<Name>.cs) and it drifts silently when reviewers don't check it.Change
Two edits to
.github/instructions/cdac.instructions.md:Broaden
applyToso these instructions load for PRs that touch:src/native/managed/cdac/**(existing)docs/design/datacontracts/**(new)src/coreclr/**/datadescriptor/**(new -- catches all three datadescriptor dirs:vm/,gc/,nativeaot/Runtime/)Add a "Documentation updates" section with a short list of change patterns that MUST have a matching doc update:
Abstractions/Contracts/I<Name>.cs-- add/remove/rename methods, new exposed types/enumsContracts/Contracts/<Name>_<N>.cs-- new version file, or semantic change to an existing version's algorithm (pure refactors excluded)docs/design/datacontracts/<Name>.mdsrc/coreclr/**/datadescriptor/**-- consuming contract's doc must reflect the changeContracts/Data/<Type>.csor newTarget.ReadGlobal*-- doc must document itWith an explicit exclusion list (pure refactors, test-only, restored-behavior bug fixes,
Legacy/**, build/CI).Kept the section deliberately short (~12 lines) -- shorter instruction blocks have produced better reviewer behavior than longer ones.
Note
This PR was drafted with assistance from GitHub Copilot.