Skip to content

Commit 2c6bdc2

Browse files
committed
docs(quality): update all instruction/prompt/skill docs with 2026 learnings
- testing.instructions.md: 0-fatals gate row; Minimum>=90%/Prefer>=95% coverage; flaky test prevention (timestamp strip, CorporateGuard stub, ParseArgs nullable guard) - cicd.instructions.md: canonical action versions table (v6/v5/v7); fix Standard CI pattern (per-project tests, paths-ignore, MSBUILDDISABLENODEREUSE, FORCE_JAVASCRIPT_ACTIONS_TO_NODE24, remove --no-build from test steps) - csharp.instructions.md: add NOLINT/HACK/csharpier-ignore to fully enumerate all forbidden suppression/waiver patterns; add them to What NOT to Do - fix-quality.prompt.md: add 0-fatals bullet; expand suppression list to all variants - code-review.prompt.md: expand suppression checklist to all variants; add coverage >=95% pref - write-tests.prompt.md: >=95% coverage preference; CorporateGuard.StubCorporate req; timestamp-stripping requirement for generated output comparisons - debug-fix/SKILL.md: add CRLF/.runsettings XML error; GUI.Tests --no-build failure; PublishTrimmed IL2026 error; strengthen forbidden patterns to all waiver variants - testing/SKILL.md: 0-fatals row; Preferred >=95% column; full suppression list; new Flaky Test Prevention section (timestamp strip, perf budget, CorporateGuard stub)
1 parent 84ecff2 commit 2c6bdc2

8 files changed

Lines changed: 275 additions & 56 deletions

File tree

.github/instructions/cicd.instructions.md

Lines changed: 62 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,31 @@ to GitHub Releases as soon as the version is tagged.
4242
- Run on both push to main and pull_request
4343
- Windows-only project — run on `windows-latest`
4444

45+
## Canonical Action Versions (verified 2026-04-07)
46+
47+
> **Before adding or bumping any action, verify the version exists on the action's GitHub releases page.**
48+
> Pinning a non-existent version silently fails the CI step.
49+
50+
| Action | Stable Version |
51+
|--------|---------------|
52+
| `actions/checkout` | `@v6` |
53+
| `actions/setup-dotnet` | `@v5` |
54+
| `actions/cache` | `@v5` |
55+
| `actions/upload-artifact` | `@v7` |
56+
| `codecov/codecov-action` | `@v6` |
57+
| `github/codeql-action/init` | `@v4` |
58+
| `github/codeql-action/analyze` | `@v4` |
59+
| `github/codeql-action/upload-sarif` | `@v4` |
60+
| `actions/dependency-review-action` | `@v4` |
61+
| `actions/labeler` | `@v6` |
62+
| `actions/github-script` | `@v8` |
63+
| `actions/stale` | `@v10` |
64+
65+
```powershell
66+
# Verify a version exists before pinning:
67+
gh release list --repo actions/upload-artifact --limit 5
68+
```
69+
4570
## Standard .NET CI Workflow Pattern
4671

4772
```yaml
@@ -50,6 +75,12 @@ name: CI
5075
on:
5176
push:
5277
branches: [main]
78+
paths-ignore:
79+
- 'docs/**'
80+
- '**.md'
81+
- '**.svg'
82+
- '**.txt'
83+
- '.github/instructions/**'
5384
pull_request:
5485
branches: [main]
5586

@@ -59,16 +90,19 @@ permissions:
5990
jobs:
6091
build-and-test:
6192
runs-on: windows-latest
93+
env:
94+
MSBUILDDISABLENODEREUSE: 1
95+
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
6296

6397
steps:
64-
- uses: actions/checkout@v4
98+
- uses: actions/checkout@v6
6599

66-
- uses: actions/setup-dotnet@v4
100+
- uses: actions/setup-dotnet@v5
67101
with:
68102
dotnet-version: "10.0.x"
69103

70104
- name: Cache NuGet
71-
uses: actions/cache@v4
105+
uses: actions/cache@v5
72106
with:
73107
path: ~/.nuget/packages
74108
key: ${{ runner.os }}-nuget-${{ hashFiles('**/*.csproj') }}
@@ -78,19 +112,26 @@ jobs:
78112
run: dotnet restore RegiLattice.sln
79113

80114
- name: Build
81-
run: dotnet build RegiLattice.sln -c Release --no-restore
115+
run: dotnet build RegiLattice.sln -c Release --no-restore --verbosity minimal
116+
117+
# Run each project individually to avoid cross-assembly file-write races
118+
- name: Test (Core)
119+
run: dotnet test tests/RegiLattice.Core.Tests/RegiLattice.Core.Tests.csproj -c Release --no-restore --settings tests/.runsettings --blame-hang-timeout 60s --collect:"XPlat Code Coverage" --logger "console;verbosity=normal"
120+
121+
- name: Test (CLI)
122+
run: dotnet test tests/RegiLattice.CLI.Tests/RegiLattice.CLI.Tests.csproj -c Release --no-restore --settings tests/.runsettings --blame-hang-timeout 60s --logger "console;verbosity=normal"
82123

83-
- name: Test with coverage
84-
run: dotnet test RegiLattice.sln -c Release --no-build --collect:"XPlat Code Coverage" --logger "console;verbosity=normal"
124+
- name: Test (GUI)
125+
run: dotnet test tests/RegiLattice.GUI.Tests/RegiLattice.GUI.Tests.csproj -c Release --no-restore --settings tests/.runsettings --blame-hang-timeout 60s --logger "console;verbosity=normal"
85126

86127
- name: Upload coverage artifact
87-
uses: actions/upload-artifact@v4
128+
uses: actions/upload-artifact@v7
88129
with:
89130
name: coverage-report
90131
path: "**/coverage.cobertura.xml"
91132

92133
- name: Upload coverage to Codecov
93-
uses: codecov/codecov-action@v5
134+
uses: codecov/codecov-action@v6
94135
with:
95136
token: ${{ secrets.CODECOV_TOKEN }}
96137
files: "**/coverage.cobertura.xml"
@@ -100,11 +141,14 @@ jobs:
100141
needs: build-and-test
101142
if: github.ref == 'refs/heads/main' && github.event_name == 'push'
102143
runs-on: windows-latest
144+
env:
145+
MSBUILDDISABLENODEREUSE: 1
146+
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
103147

104148
steps:
105-
- uses: actions/checkout@v4
149+
- uses: actions/checkout@v6
106150

107-
- uses: actions/setup-dotnet@v4
151+
- uses: actions/setup-dotnet@v5
108152
with:
109153
dotnet-version: "10.0.x"
110154

@@ -115,7 +159,7 @@ jobs:
115159
-p:PublishSingleFile=true -p:IncludeNativeLibrariesForSelfExtract=true
116160
117161
- name: Upload artifact
118-
uses: actions/upload-artifact@v4
162+
uses: actions/upload-artifact@v7
119163
with:
120164
name: RegiLattice-win-x64
121165
path: src/RegiLattice.GUI/bin/Release/net10.0-windows/win-x64/publish/
@@ -204,13 +248,17 @@ Use: `gh issue close <N> --repo RajwanYair/RegiLattice --comment "CI now green a
204248
## CI Best Practices
205249

206250
- **NuGet cache**: key on `.csproj` hash to invalidate on dependency changes
207-
- **Build once, test from build**: use `--no-build` in test step after `dotnet build`
251+
- **Build once, test without `--no-build`**: omit `--no-build` to let each test step do a fast incremental build — safer than relying on a prior Build step to produce all DLLs, especially for GUI.Tests which requires Windows SDK runtime packs
208252
- **Per-project test runs — MANDATORY**: NEVER use `dotnet test RegiLattice.sln`. Always run each
209253
test project individually (Core → CLI → GUI). `dotnet test RegiLattice.sln` spawns Core.Tests and
210254
GUI.Tests concurrently as separate processes, both writing to the same `compliance-history.json`
211255
file, causing non-deterministic failures. See `tests/.runsettings` MaxCpuCount note.
212-
- **Codecov**: use `codecov-action@v5`; set `fail_ci_if_error: false`
256+
- **`MSBUILDDISABLENODEREUSE: 1`**: Set at job level in every CI job — prevents MSBuild worker nodes from holding file locks across builds on the OneDrive-hosted workspace (MSB3492)
257+
- **`FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true`**: Set at job level — ensures JS-based actions (checkout, cache, codecov) run on Node 24 without deprecation warnings
258+
- **`paths-ignore` for docs-only changes**: Add to `on.push` to avoid a full build/test run when only `.md`, `.svg`, `.txt`, or instruction files changed
259+
- **Codecov**: use `codecov-action@v6`; set `fail_ci_if_error: false`
213260
- **Windows-only**: no matrix needed — single `windows-latest` runner
214261
- **Self-contained publish**: `-r win-x64 --self-contained true -p:PublishSingleFile=true`
215-
- **Stryker mutation testing**: runs from `src/RegiLattice.Core/` directory with `STRYKER_BUILD=1`; explicit `<TargetFramework>` required in Core and Core.Tests .csproj files for Buildalyzer compatibility
262+
- **Stryker mutation testing**: runs from `src/RegiLattice.Core/` directory with `STRYKER_BUILD=1`; explicit `<TargetFramework>` required in Core and Core.Tests .csproj files for Buildalyzer compatibility; move to **weekly schedule** (cron) to avoid adding ~15 min per push
216263
- **`dotnet build` verbosity**: NEVER use `-q`/`--verbosity quiet` — causes question-build aborts; use no flag or `--verbosity minimal`
264+
- **Action version verification**: verify every action version exists before committing — a non-existent tag silently fails the step

.github/instructions/csharp.instructions.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,17 @@ When the compiler or analyzer emits a diagnostic, **fix the root cause**. The fo
3737
// NOSONAR
3838
// NCA
3939
// ReSharper disable ...
40+
// NOLINT
41+
// HACK: (used to sidestep a quality check)
42+
```
43+
44+
**Inline quality gate waivers are also forbidden** — they are indistinguishable from suppressions:
45+
46+
```csharp
47+
// ❌ FORBIDDEN — lint ignore / bypass comments
48+
// csharpier-ignore
49+
// dotnet-stryker-exclude
50+
// coverage: ignore
4051
```
4152

4253
**Resolution patterns** (fix instead of suppress):
@@ -266,6 +277,8 @@ public async Task<string> ExportJsonAsync(string path)
266277
- Don't catch `Exception` without re-throwing or explicit justification
267278
- Don't use `#pragma warning disable` — fix the root cause instead
268279
- Don't use `[SuppressMessage(...)]` — fix the root cause instead
280+
- Don't use `// NOSONAR`, `// NCA`, `// ReSharper disable`, `// NOLINT`, or any inline suppression/waiver pattern
269281
- Don't leave `TODO` or `FIXME` comments — complete the work or open a GitHub Issue
270282
- Don't use `null!` to suppress CS8618 — use `required` + `init` or assign a real default
271283
- Don't use `!` (null-forgiving) unless the value is provably non-null at that point
284+
- Don't skip tests with `[Fact(Skip=...)]` or `[Theory(Skip=...)]` — fix the test or the code it tests

.github/instructions/testing.instructions.md

Lines changed: 87 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -189,19 +189,20 @@ reportgenerator -reports:"**/coverage.cobertura.xml" -targetdir:"htmlcov" -repor
189189

190190
### Coverage Targets
191191

192-
> **Target: ≥90% line coverage on all testable components. Prefer higher. Branch coverage at 60%+.**
193-
> Any PR that drops line coverage below 90% on `RegiLattice.Core.Tests` must be accompanied by a
194-
> documented justification. Coverage regressions from the baseline are treated as build failures.
195-
196-
| Component | Target | Notes |
197-
| ------------------ | --------- | ------------------------------------------------------------ |
198-
| TweakDef model | **100%** | Pure logic, fully testable with declarative assertions |
199-
| TweakEngine | **90%+** | Core business logic — all public API paths must be tested |
200-
| RegistrySession | **85%+** | DryRun mode enables safe testing of all write/read paths |
201-
| Services | **90%+** | All services with deterministic logic must have full tests |
202-
| GUI (Theme) | **90%+** | Theme records are pure data with no external dependencies |
203-
| GUI (Forms) | **60%+** | WinForms UI event handling is hard to unit test |
204-
| **Overall (Core)** | **≥90%** | **Line coverage gate enforced in CI via Codecov threshold** |
192+
> **Minimum: ≥90% line coverage (enforced gate). Preferred: ≥95% where achievable. Branch coverage ≥60%.**
193+
> Any PR that drops line coverage below 90% on `RegiLattice.Core.Tests` is blocked.
194+
> Coverage regressions from the baseline are treated as build failures and must not be merged.
195+
> Strive for 95%+ on all new code — 90% is the floor, not the goal.
196+
197+
| Component | Minimum | Preferred | Notes |
198+
| ------------------ | --------- | --------- | ------------------------------------------------------------ |
199+
| TweakDef model | **100%** | **100%** | Pure logic, fully testable with declarative assertions |
200+
| TweakEngine | **90%+** | **95%+** | Core business logic — all public API paths must be tested |
201+
| RegistrySession | **85%+** | **92%+** | DryRun mode enables safe testing of all write/read paths |
202+
| Services | **90%+** | **95%+** | All services with deterministic logic must have full tests |
203+
| GUI (Theme) | **90%+** | **95%+** | Theme records are pure data with no external dependencies |
204+
| GUI (Forms) | **60%+** | **75%+** | WinForms UI event handling is hard to unit test |
205+
| **Overall (Core)** | **≥90%** | **≥95%** | **Line coverage gate enforced in CI via Codecov threshold** |
205206

206207
### Coverage by TweakKind
207208

@@ -231,15 +232,18 @@ These components require external tools, network, or system state that cannot be
231232

232233
Every test run and every CI build must meet these conditions before merging:
233234

234-
| Gate | Requirement |
235-
| ---------------------- | -------------------------------------------------------------------- |
236-
| Build warnings | **0**`TreatWarningsAsErrors=true`; any warning blocks CI |
237-
| Build errors | **0** — hard fail |
238-
| Test failures | **0** — all 3,230+ tests must pass |
239-
| Skipped tests | **0**`[Fact(Skip=...)]` and `[Theory(Skip=...)]` are forbidden |
240-
| Inline suppressions | **0**`#pragma warning disable` / `[SuppressMessage]` are forbidden |
241-
| TODO / FIXME in tests | **0** — open a GitHub Issue instead |
242-
| Line coverage (Core) | **≥90%** — Codecov enforced; PR dropped below gate = block |
235+
| Gate | Requirement |
236+
| ---------------------- | ------------------------------------------------------------------------------------- |
237+
| Build fatals | **0** — hard CI fail |
238+
| Build errors | **0** — hard CI fail |
239+
| Build warnings | **0**`TreatWarningsAsErrors=true`; any warning blocks CI |
240+
| Test failures | **0** — all 3,230+ tests must pass |
241+
| Skipped tests | **0**`[Fact(Skip=...)]` and `[Theory(Skip=...)]` are forbidden |
242+
| Inline suppressions | **0**`#pragma warning disable`, `[SuppressMessage]`, `// NOSONAR`, `// NCA`, |
243+
| | `// ReSharper disable`, `// NOLINT` — all forbidden; fix root cause instead |
244+
| TODO / FIXME in tests | **0** — deferred work belongs in a GitHub Issue, not inline comments |
245+
| Waivers / lint ignores | **0** — no inline quality gate bypasses of any kind |
246+
| Line coverage (Core) | **≥90%** minimum (gate enforced) — **≥95% preferred** |
243247

244248
```powershell
245249
# Verify the full quality gate locally before every commit:
@@ -249,6 +253,62 @@ dotnet test tests/RegiLattice.CLI.Tests/RegiLattice.CLI.Tests.csproj --settings
249253
dotnet test tests/RegiLattice.GUI.Tests/RegiLattice.GUI.Tests.csproj --settings tests/.runsettings --blame-hang-timeout 60s
250254
```
251255

256+
## Flaky Test Prevention
257+
258+
### Strip timestamps before comparing generated output
259+
260+
Any method that embeds `DateTime.Now` internally can straddle a clock second when called twice:
261+
262+
```csharp
263+
// ❌ BAD — can fail at second boundaries (non-deterministic)
264+
Assert.Equal(gen.Build(map), File.ReadAllText(path));
265+
266+
// ✅ GOOD — strip timestamps before comparing
267+
static string StripTimestamp(string s) =>
268+
System.Text.RegularExpressions.Regex.Replace(
269+
s, @"\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}", "TIMESTAMP");
270+
Assert.Equal(StripTimestamp(gen.Build(map)), StripTimestamp(File.ReadAllText(path)));
271+
```
272+
273+
### Performance budget comments are mandatory
274+
275+
```csharp
276+
// ✅ GOOD — document the budget with context so future changes are deliberate
277+
// Budget: 150ms — search over 7,000+ tweaks with synonym expansion; ~60ms on dev machine.
278+
// Raise threshold if tweak count exceeds 10,000.
279+
Assert.True(sw.ElapsedMilliseconds < 150, $"Search took {sw.ElapsedMilliseconds}ms (budget: 150ms)");
280+
```
281+
282+
### CorporateGuard isolation in test fixtures
283+
284+
Any test that touches code paths reaching `CorporateGuard` **must** stub it to avoid Intune/SCCM
285+
registry read latency on corporate machines:
286+
287+
```csharp
288+
public sealed class MyTestFixture : IDisposable
289+
{
290+
public MyTestFixture() => CorporateGuard.StubCorporate = false;
291+
public void Dispose() => CorporateGuard.StubCorporate = null;
292+
}
293+
```
294+
295+
### `ParseArgs()` nullable guard — always `Assert.NotNull` first
296+
297+
`Program.ParseArgs()` returns `CliArgs?`. Accessing any property without a null guard emits CS8602:
298+
299+
```csharp
300+
// ❌ BAD — CS8602: dereference of possibly null reference
301+
var a = ParseArgs(new[] { "--list" });
302+
Assert.True(a.ShowList);
303+
304+
// ✅ GOOD — Assert.NotNull satisfies nullable flow analysis
305+
var a = ParseArgs(new[] { "--list" });
306+
Assert.NotNull(a);
307+
Assert.True(a.ShowList);
308+
```
309+
310+
---
311+
252312
## What NOT to Do in Tests
253313

254314
- Don't test implementation details — test behaviour
@@ -259,6 +319,9 @@ dotnet test tests/RegiLattice.GUI.Tests/RegiLattice.GUI.Tests.csproj --settings
259319
- Don't create real registry keys in tests — use `DryRun = true` on RegistrySession
260320
- Don't create actual WinForms windows in CI — test data models and logic only
261321
- Don't use `Assert.True(condition)` when a specific assertion exists (e.g., `Assert.Equal`, `Assert.Contains`)
262-
- Don't use `[Fact(Skip=...)]` or `[Theory(Skip=...)]` — fix the test or the code it tests; skips are forbidden
263-
- Don't use `#pragma warning disable` in test code — all warnings must be fixed
322+
- Don't use `[Fact(Skip=...)]` or `[Theory(Skip=...)]` — fix the test or the code it tests; skips are **forbidden**
323+
- Don't use `#pragma warning disable`, `[SuppressMessage]`, `// NOSONAR`, or any inline suppression — fix root cause
264324
- Don't use inline assertion workarounds (`Assert.Equal(expected, actual)` inverted) to hide test logic errors
325+
- Don't use waivers or lint-ignore comments — there are no circumstances where these are acceptable in tests
326+
- Don't use `DateTime.Now` in two separate calls when comparing generated output — strip timestamps instead
327+
- Don't hardcode wall-clock performance budgets without a comment stating the tweak count at time of writing

.github/prompts/code-review.prompt.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,11 @@ Selection: `${selection}`
1717
### Build Quality Gate
1818

1919
- [ ] Build produces **0 fatals, 0 errors, 0 warnings** (`TreatWarningsAsErrors=true`)
20-
- [ ] No `#pragma warning disable` / `[SuppressMessage]` / `// NOSONAR` suppressions
20+
- [ ] No `#pragma warning disable` / `[SuppressMessage]` / `// NOSONAR` / `// NCA` / `// ReSharper disable` / `// NOLINT` suppressions
21+
- [ ] No inline quality gate waivers (`// csharpier-ignore`, `// coverage: ignore`, `// HACK:` to bypass checks)
2122
- [ ] No `TODO` or `FIXME` comments (open GitHub Issues instead)
2223
- [ ] No `[Fact(Skip=...)]` or `[Theory(Skip=...)]` in tests
23-
- [ ] Core test coverage ≥90% for new/changed code
24+
- [ ] Core test coverage ≥90% for new/changed code (prefer ≥95%)
2425

2526
### Security (OWASP Top 10)
2627

.github/prompts/fix-quality.prompt.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,13 @@ Fix all quality issues in: `${file}`
99

1010
## Quality Gate (MUST achieve before done)
1111

12+
- **0 build fatals** — no fatal MSBuild or compiler errors
1213
- **0 build warnings**`TreatWarningsAsErrors=true` is global; every warning is a build error
1314
- **0 build errors**
14-
- **No suppressions allowed**`#pragma warning disable`, `[SuppressMessage]`, `// NOSONAR` are **FORBIDDEN** — fix the root cause instead
15+
- **No suppressions or waivers allowed** — the following are **FORBIDDEN** — fix the root cause instead:
16+
- `#pragma warning disable` / `[SuppressMessage]` / `// NOSONAR` / `// NCA`
17+
- `// ReSharper disable` / `// NOLINT` / `// csharpier-ignore` / `// coverage: ignore`
18+
- `// HACK:` (when used to bypass a quality check)
1519
- **No TODO / FIXME comments** — open a GitHub Issue instead; note it in the commit footer
1620

1721
## What to Fix

0 commit comments

Comments
 (0)