Skip to content

Enhance expert reviewer guidelines with additional checks. #14255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AR-May merged 2 commits into from
Jul 3, 2026
Merged

Enhance expert reviewer guidelines with additional checks. #14255

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
089d5f5
Enhance expert reviewer guidelines with additional checks for abstrac…
AR-May Jul 3, 2026
661d6cc
Potential fix for pull request finding
AR-May Jul 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/agents/expert-reviewer.agent.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ See `../../documentation/wiki/Microsoft.Build.Framework.md`.
5. For bug fix PRs, read the original issue and feature PR discussions to understand the design intent. Verify the fix aligns with it.
6. When code works around an API limitation (try/catch chains, TOCTOU patterns, fallback sequences), check whether a better API exists in already-referenced packages that would eliminate the workaround.
7. When a pattern is borrowed from another codebase or context, verify its assumptions still hold in the new context.
8. Respect abstraction layers: a layer should not depend on the internals or concrete types of another, and logic should live in the layer responsible for it. Flag dependency inversions (a lower layer depending on a higher one), leaks (one layer's implementation details exposed across its boundary), and misplaced logic (a decision made outside the layer that owns it), even when functionally correct.
9. When code is placed in a particular spot because some constraint supposedly requires it, confirm that constraint is real before accepting the placement. A justification that holds for one path may not hold here, and a cleaner location may be available.

**CHECK — Flag if:**
- [ ] Large feature PR with no linked spec
Expand All @@ -241,6 +243,7 @@ See `../../documentation/wiki/Microsoft.Build.Framework.md`.
- [ ] Fix contradicts design intent established in original feature discussions
- [ ] Workaround for an API limitation when a better API is available in existing dependencies
- [ ] Pattern borrowed from a different context without validating its assumptions apply here
- [ ] Abstraction-layer violation: dependency inversion, a leak across a boundary, or logic placed outside the layer that owns it

---

Expand Down Expand Up @@ -312,14 +315,16 @@ See `../../documentation/wiki/Nodes-Orchestration.md`, `../../documentation/spec

**Rules:**
1. Use clear, descriptive names. Avoid abbreviations unless universally understood (e.g., `PRE` for `ProjectRootElement`).
2. Be consistent with surrounding code naming.
3. Test methods: `MethodUnderTest_Scenario_ExpectedResult`.
2. Names should describe what a member is or does, not why it was introduced. Names should not be misleading.
3. Be consistent with surrounding code naming.
4. Test methods: `MethodUnderTest_Scenario_ExpectedResult`.

**CHECK — Flag if:**
- [ ] Ambiguous names (`data`, `result`, `temp`, `flag`)
- [ ] Naming inconsistent with adjacent code
- [ ] Boolean parameter meaning unclear
- [ ] Test method names don't describe what they test
- [ ] Misleading names: a name implies semantics different from what the member actually represents or does

---

Expand Down
Loading