|
| 1 | +# Code Review Rules |
| 2 | + |
| 3 | +These rules apply to Copilot code review. Read all rules before commenting. |
| 4 | + |
| 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: |
| 28 | + |
| 29 | +- Bad: `pool.GetOrAdd(key, new ExpensiveObject());` |
| 30 | +- Good: `pool.GetOrAdd(key, _ => new ExpensiveObject());` |
| 31 | + |
| 32 | +## C# coding standards |
| 33 | + |
| 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 |
| 41 | + |
| 42 | +## Testing standards |
| 43 | + |
| 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 | + |
1 | 65 | Carefully review all markdown documents in the ../.clinerules folder. Those are your custom instructions. |
2 | 66 |
|
3 | 67 | --- |
@@ -45,18 +109,24 @@ Copilot will automatically reference and describe: |
45 | 109 | - `@msal-mtls-pop-guidance` - Foundational concepts |
46 | 110 | - `@msal-mtls-pop-vanilla` - Direct token acquisition |
47 | 111 | - `@msal-mtls-pop-fic-two-leg` - Token exchange patterns |
| 112 | +- `@msal-auth-code-flow` - Authorization Code Flow |
| 113 | +- `@msal-client-credentials` - Client Credentials Flow |
| 114 | +- `@msal-obo-flow` - On-Behalf-Of Flow |
48 | 115 |
|
49 | 116 | --- |
50 | 117 |
|
51 | 118 | ## 📚 Available Skills Overview |
52 | 119 |
|
53 | | -This repository contains **three GitHub Agent Skills** for mTLS Proof-of-Possession (PoP) authentication: |
| 120 | +This repository contains **six GitHub Agent Skills** for MSAL.NET authentication: |
54 | 121 |
|
55 | 122 | | Skill | Purpose | Best For | |
56 | 123 | |-------|---------|----------| |
57 | 124 | | **@msal-mtls-pop-guidance** | Foundational concepts, terminology, decision frameworks | Learning the fundamentals, comparing approaches | |
58 | 125 | | **@msal-mtls-pop-vanilla** | Direct single-step token acquisition with complete code | Quick implementation with MSI or Confidential Client | |
59 | 126 | | **@msal-mtls-pop-fic-two-leg** | Two-step token exchange patterns | Complex scenarios requiring token exchange | |
| 127 | +| **@msal-auth-code-flow** | Authorization Code Flow for web apps | User sign-in with server-side backend | |
| 128 | +| **@msal-client-credentials** | Client Credentials Flow for daemons | Service-to-service, no user context | |
| 129 | +| **@msal-obo-flow** | On-Behalf-Of Flow for multi-tier APIs | Propagating user identity through API chain | |
60 | 130 |
|
61 | 131 | --- |
62 | 132 |
|
|
0 commit comments