Skip to content

Commit cb59f84

Browse files
gladjohnCopilot
gladjohn
and
Copilot
authored
Fix Copilot review noise: restructure instructions for 4K limit (#5971)
* Fix Copilot review noise: restructure instructions for 4K limit Copilot code review only reads the first 4,000 characters of copilot-instructions.md. The previous layout consumed this budget with Copilot Chat skills documentation, so the reviewer never saw any coding rules — causing 328 false-positive comments across 40 PRs. Changes: - Move code review rules to the first 2,500 chars (review scope, repo-specific false-positive suppressions, coding standards) - Push Chat/Agent skills documentation below the 4K cutoff - Add .github/instructions/tests.instructions.md with test-specific rules (RunOn attribute, MSTest patterns, deterministic tests) - Add .github/instructions/src.instructions.md with source code rules (namespace resolution, reflection ban, GetOrAdd pattern) Path-specific instruction files are not subject to the 4K limit and provide targeted guidance per file context. Resolves #5967 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: MSTest v4, strip coding standards from src.instructions (defer to .editorconfig) - Fix MSTest SDK v3 → v4 in copilot-instructions.md and tests.instructions.md (bgavrilMS suggestion) - Remove coding standards section from src.instructions.md — enforced via .editorconfig (bgavrilMS) - Keep only MSAL-specific rules (dependencies) and scope/reviewer rules - Resolves conflicting reflection guidance flagged by Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 55c4940 commit cb59f84

3 files changed

Lines changed: 120 additions & 16 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 58 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,68 @@
1-
Carefully review all markdown documents in the ../.clinerules folder. Those are your custom instructions.
1+
# Code Review Rules
22

3-
---
3+
These rules apply to Copilot code review. Read all rules before commenting.
44

5-
# Code Review Rules
5+
## Review scope
6+
7+
- Only comment on lines added or modified in the PR diff
8+
- Do not comment on pre-existing code unless the PR directly introduces the issue
9+
- Do not comment on style, formatting, or indentation
10+
- Focus exclusively on: bugs, security issues, logic errors, API contract violations
11+
- If unsure whether something is a bug, do not comment
12+
- Prefer no comment over a speculative comment
13+
- Do not re-post a comment already made on an earlier commit in the same PR
14+
15+
## Repo-specific patterns — do NOT flag these
16+
17+
These patterns are correct in this repo. Do not suggest changes:
18+
19+
- `[RunOn]` inherits from `TestMethodAttribute`. Do not flag as missing `[TestMethod]`
20+
- `Client.AppConfig.X` resolves via parent namespace `Microsoft.Identity`. Do not flag as unresolved namespace
21+
- `Assert.IsTrue(bool?)` is a valid MSTest overload. Do not flag nullable bool as a type mismatch
22+
- `Assert.DoesNotContain(substring, value)` — MSTest v4 signature is substring first, value second
23+
- `ConfigureAwait(false)` is intentional in library code. Do not suggest removal
24+
25+
## ConcurrentDictionary.GetOrAdd — always use factory delegate
26+
27+
`GetOrAdd(key, value)` eagerly evaluates the value arg. Flag any call where the second argument is not a delegate/lambda/method group:
628

7-
## ConcurrentDictionary.GetOrAdd — always use the factory delegate overload
29+
- Bad: `pool.GetOrAdd(key, new ExpensiveObject());`
30+
- Good: `pool.GetOrAdd(key, _ => new ExpensiveObject());`
831

9-
`ConcurrentDictionary.GetOrAdd(key, value)` **eagerly evaluates** the second argument before checking the dictionary. If the value is a constructor call or method invocation (e.g., `new HttpClient(...)`, `CreateFoo()`), a new object is created and discarded on every cache hit.
32+
## C# coding standards
1033

11-
**Bad** — object created on every call, discarded on cache hit:
12-
```csharp
13-
pool.GetOrAdd(key, new ExpensiveObject());
14-
pool.GetOrAdd(key, CreateExpensiveObject());
15-
```
34+
- Use `is null` / `is not null` instead of `== null` / `!= null`
35+
- No reflection in product code (`/src`). Acceptable in tests
36+
- Static fields: `s_camelCase` (e.g., `s_knownHosts`)
37+
- Ordinal string comparisons for protocol values, identifiers, cache keys
38+
- Validate inputs at method boundaries (fail fast with specific exception types)
39+
- Do not include secrets/tokens/PII in exception messages or logs
40+
- Use `nameof` instead of string literals for member names
1641

17-
**Good** — factory lambda only runs on cache miss:
18-
```csharp
19-
pool.GetOrAdd(key, _ => new ExpensiveObject());
20-
pool.GetOrAdd(key, _ => CreateExpensiveObject());
21-
```
42+
## Testing standards
2243

23-
When reviewing code, flag any `GetOrAdd` call where the second argument is **not** a delegate/lambda/method group. All other usages of GetOrAdd(key, value) should be justified.
44+
- MSTest SDK v4 with NSubstitute for mocking
45+
- Use `// Arrange`, `// Act`, `// Assert` comments
46+
- Prefer deterministic tests (no timing flakiness)
47+
48+
## Public API changes
49+
50+
- Update `PublicAPI.Unshipped.txt` for any public API additions/removals
51+
- XML doc comments required on all public APIs
52+
- Maintain backward compatibility
53+
54+
## MSAL-specific rules
55+
56+
- Use certificate-based auth over client secrets when possible
57+
- Use async APIs consistently
58+
- Keep dependencies minimal and well-justified
59+
60+
---
61+
62+
<!-- Everything below this line is for Copilot Chat and Copilot Agent only. -->
63+
<!-- Copilot code review reads only the first 4,000 characters of this file. -->
64+
65+
Carefully review all markdown documents in the ../.clinerules folder. Those are your custom instructions.
2466

2567
---
2668

‎.github/instructions/src.instructions.md‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
# Source code review rules
2+
3+
Applies to: src/**/*.cs
4+
5+
These rules apply when reviewing production source code in this repository.
6+
7+
## Namespace resolution — do NOT flag
8+
9+
- `Client.AppConfig.X`, `Client.Internal.X`, and similar short namespace references resolve via parent namespace `Microsoft.Identity`. Do not flag as unresolved.
10+
11+
## ConcurrentDictionary.GetOrAdd
12+
13+
`GetOrAdd(key, value)` eagerly evaluates the value argument. Flag any call where the second argument is not a delegate/lambda/method group:
14+
15+
- Bad: `pool.GetOrAdd(key, new ExpensiveObject());`
16+
- Good: `pool.GetOrAdd(key, _ => new ExpensiveObject());`
17+
18+
## `ConfigureAwait(false)`
19+
20+
`ConfigureAwait(false)` is intentional in library code. Do not suggest removal.
21+
22+
## Public API changes
23+
24+
- Any public API additions or removals must be reflected in `PublicAPI.Unshipped.txt`
25+
- XML doc comments are required on all public APIs
26+
- Maintain backward compatibility — breaking changes require explicit justification
27+
28+
## MSAL-specific
29+
30+
- Keep dependencies minimal and well-justified
31+
32+
## Scope
33+
34+
- Only comment on source code added or modified in the PR diff
35+
- Do not comment on pre-existing code unless the PR directly introduces the issue
36+
- Do not comment on style or formatting — coding standards are enforced via .editorconfig
37+
- Focus on: bugs, security issues, logic errors, API contract violations

‎.github/instructions/tests.instructions.md‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Test file review rules
2+
3+
Applies to: tests/**/*.cs
4+
5+
These rules apply when reviewing test files in this repository.
6+
7+
## Test framework patterns — do NOT flag
8+
9+
- `[RunOn]` inherits from `TestMethodAttribute` (see `tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/TargetFramework.cs` line 15). Tests decorated with `[RunOn]` WILL be discovered by MSTest. Do not flag as missing `[TestMethod]`.
10+
- `Assert.IsTrue(bool?)` is a valid MSTest overload. Do not flag nullable bool arguments as type mismatches.
11+
- `Assert.DoesNotContain(substring, value)` — in MSTest v4, the first argument is the substring and the second is the value to search. Do not suggest swapping arguments.
12+
- `Assert.HasCount(expected, collection)` — valid MSTest v4 assertion. Do not suggest `Assert.AreEqual` for count checks.
13+
14+
## Test conventions
15+
16+
- Use MSTest SDK v4 with NSubstitute for mocking
17+
- Use `// Arrange`, `// Act`, `// Assert` comments
18+
- Prefer deterministic tests: avoid `Thread.Sleep`, timing dependencies, or environment-specific behavior
19+
- Copy existing style in nearby files for test method names
20+
21+
## Scope
22+
23+
- Only comment on test code that is added or modified in the PR diff
24+
- Do not comment on pre-existing test patterns or style
25+
- Do not re-post comments already made on earlier commits

0 commit comments

Comments
 (0)