From 089d5f5b03bd3c1846371cc7a39eee0adbad1590 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Fri, 3 Jul 2026 15:41:09 +0200 Subject: [PATCH 1/2] Enhance expert reviewer guidelines with additional checks for abstraction layer violations and misleading names --- .github/agents/expert-reviewer.agent.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/agents/expert-reviewer.agent.md b/.github/agents/expert-reviewer.agent.md index e5404aa2d9f..d458a973dfd 100644 --- a/.github/agents/expert-reviewer.agent.md +++ b/.github/agents/expert-reviewer.agent.md @@ -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 @@ -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 --- @@ -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 method name implies an effect different from what it actually does --- From 661d6cc9f5eb63f6af65b17d3e221a6b119a16bc Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:02:00 +0200 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/agents/expert-reviewer.agent.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/agents/expert-reviewer.agent.md b/.github/agents/expert-reviewer.agent.md index d458a973dfd..662221a1e6f 100644 --- a/.github/agents/expert-reviewer.agent.md +++ b/.github/agents/expert-reviewer.agent.md @@ -324,7 +324,7 @@ See `../../documentation/wiki/Nodes-Orchestration.md`, `../../documentation/spec - [ ] Naming inconsistent with adjacent code - [ ] Boolean parameter meaning unclear - [ ] Test method names don't describe what they test -- [ ] Misleading names: a method name implies an effect different from what it actually does +- [ ] Misleading names: a name implies semantics different from what the member actually represents or does ---