code-quality

You are the **local code-quality pre-check** for `microsoft/winappcli`. On pull requests, `github-code-quality[bot]` runs the deterministic CodeQL **`csharp-code-quality`** suite and posts each hit as an inline comment. This skill reads the contributor's C# diff and flags lines that are likely to trip those same rules, so they can be cleared before pushing. Scope is **C# only** — the language the bot rule-checks in this repo. **Be honest about what this is.** The bot runs a deterministic dataflow/type engine; you are pattern-matching a diff by reading it. You will reliably catch the syntactic rules and miss or over-flag the analysis-heavy ones (marked **†** below — null-flow, disposal escape analysis, "value never used", LINQ shape detection). Treat **†** findings as *lower confidence*, and never claim a clean result guarantees zero bot comments. It does not — it means nothing *obvious* is left. This also does not cover the bot's separate AI-powered pass, or C/C++. Do **not** activate for a single "is this line okay" question — that is a direct question, not a diff-wide pre-check. ## 1. Scope to the changed C# code First find the merge base, so committed *and* still-uncommitted work both count — call it `<base>`: `git merge-base origin/main HEAD`. - **Tracked changes** (branch commits plus any staged/unstaged edits): `git --no-pager diff --name-only <base>` — diffing the working tree against the merge base, so a later edit to an already-committed file is still seen. - **Untracked new files:** `git ls-files --others --exclude-standard -- "src/*.cs"` — `git diff` cannot emit hunks for these; read them directly with `view`. Keep only `*.cs` under `src\`, and drop files matching the repo's CodeQL exclusion `**/*Tests/*.cs` — a `.cs` file **directly inside** a directory whose name ends in `Tests` (e.g. `src\WinApp.Cli.Tests\Foo.cs`). Mirror that glob exactly: a file nested deeper, such as `src\...\WinApp.Cli.Tests\TestApps\App.xaml.cs`, does **not** match it, so the bot may scan it — keep those in scope. If nothing survives, say so and stop. Then read the changed lines. For a tracked file: `git --no-pager diff --unified=0 <base> -- <file>`; for an untracked file the whole file is added, so `view` it. Either way `view` each changed region for context. **Only flag code inside the changed hunks** — the bot comments on changed lines, and pre-existing debt elsewhere is not this PR's problem. ## 2. Check the diff against the rule set For every changed hunk, walk the rule tables below and flag each line that matches. For a match, record the `cs/...` id, the exact `file:line`, the offending snippet, and the smallest fix. Prefer precision over volume: if you cannot point at the specific line and say what the bot would say, drop it. Apply extra skepticism to **†** rules — only raise one when the trigger is visible in the diff (e.g. a `new HttpRequestMessage(...)` with no `using` and no `return`/field assignment), not on a hunch. ### Exception handling | Rule | Flags → do instead | |---|---| | `cs/catch-of-all-exceptions` | `catch (Exception)` (or bare `catch`) → catch the specific type you can handle | | `cs/catch-nullreferenceexception` | catching `NullReferenceException` → fix the deref; don't catch it | | `cs/empty-catch-block` | `catch {}` that swallows → handle, log, or let it propagate | | `cs/rethrown-exception-variable` | `throw ex;` (resets stack) → `throw;` | ### Null & dereference — **†** | Rule | Flags → do instead | |---|---| | `cs/dereferenced-value-may-be-null` **†** | using a value that a prior branch/API can leave null → null-check or `?.` | | `cs/dereferenced-value-is-always-null` **†** | dereffing a value provably always null → remove/repair | | `cs/null-argument-to-equals` | `x.Equals(null)` → `x is null` | ### Disposal & resources | Rule | Flags → do instead | |---|---| | `cs/local-not-disposed` **†** | a local `IDisposable` you own, never disposed → `using` | | `cs/missed-using-statement` **†** | `Dispose()` in a `finally` you could express as `using` → `using` | ### Equality & hashing | Rule | Flags → do instead | |---|---| | `cs/gethashcode-is-not-defined` | overriding `Equals` without `GetHashCode` → override both | | `cs/useless-gethashcode-call` | `GetHashCode()` on a small integral type (e.g. `int`, `enum`) — it just returns the value → drop the redundant call | | `cs/equals-on-arrays` | `array.Equals(other)` (reference eq) → `SequenceEqual` | | `cs/equals-on-unrelated-types` | `Equals` across incomparable types → compare compatible types | | `cs/unchecked-cast-in-equals` | unchecked cast of the arg in an `Equals` override → `is`/`as` guard | | `cs/recursive-equals-call` | `Equals` calling itself unboundedly → fix the recursion | | `cs/reference-equality-on-valuetypes` | `ReferenceEquals` on value types (always false) → `==`/`Equals` | | `cs/equality-on-floats` | `==` on `double`/`float` → compare with a tolerance | | `cs/comparison-of-identical-expressions` | a comparison operator with identical operands (`x == x`, `a < a`) → remove or fix the typo | ### Dead & useless code | Rule | Flags → do instead | |---|---| | `cs/empty-block` | empty `if`/loop body → remove or fill | | `cs/useless-if-statement` | `if` whose branches do the same thing → collapse | | `cs/constant-condition` **†** | condition provably always true/false → remove the dead path | | `cs/simplifiable-boolean-expression` | `x ? true : false`, `!(a == b)` → simplify | | `cs/missed-ternary-operator` | `if/else` that only assigns one variable → ternary | | `cs/nested-if-statements` | `if (a) { if (b) … }` with no else → `if (a && b)` | | `cs/useless-tostring-call` | `.ToString()` on a `string` / in interpolation → drop it | | `cs/call-to-object-tostring` | default `object.ToString()` (type name) → override or format explicitly | | `cs/self-assignment` | `x = x;` → remove (or fix the intended target) | | `cs/unused-label` | a `label:` never targeted → remove | | `cs/unused-property-value` **†** | setter that ignores `value` → use it | | `cs/empty-collection` **†** | a collection created empty and then read/used but never given any elements → add elements or drop the reads | | `cs/unused-collection` **†** | a collection filled but never read → remove or use | ### LINQ opportunities — **†** | Rule | Flags → do instead | |---|---| | `cs/linq/missed-where` **†** | `foreach` with a filtering `if` → `.Where(...)` | | `cs/linq/missed-select` **†** | `foreach` that only projects → `.Select(...)` | | `cs/linq/missed-all` **†** | loop checking a condition holds for all → `.All(...)` | | `cs/linq/missed-cast` **†** | loop casting every element → `.Cast<T>()` | | `cs/linq/missed-oftype` **†** | loop filtering by type → `.OfType<T>()` | | `cs/linq/useless-select` | `.Select(x => x)` → drop it | ### Collections & indexing | Rule | Flags → do instead | |---|---| | `cs/inefficient-containskey` | `ContainsKey` then indexer → `TryGetValue` | | `cs/test-for-negative-container-size` | `Count < 0` / `Length < 0` (never true) → fix the check | | `cs/index-out-of-bounds` **†** | off-by-one vs `Length`/`Count` → use `< Length` | | `cs/nested-loops-with-same-variable` | nested `for`/`foreach` that reuse the same counter (inner loop also updates `i`) → use a distinct loop variable | ### Concurrency & locking | Rule | Flags → do instead | |---|---| | `cs/lock-this` | `lock (this)` → lock a private `object` | | `cs/empty-lock-statement` | `lock (x) {}` → remove or fill | | `cs/unsafe-sync-on-field` | locking a field that is **reassigned inside the lock** (threads then lock different objects) → lock a private `readonly` object | | `cs/inconsistent-lock-sequence` **†** | acquiring locks in differing orders → fix ordering | | `cs/locked-wait` **†** | a blocking `.Wait()` (`Task`/`WaitHandle`) while holding a lock — **not** `Monitor.Wait` on the locked object, which correctly requires it → don't block on unrelated waits inside the lock | | `cs/non-short-circuit` | non-short-circuit `&`/`\|` on bools whose right operand has a risky access (e.g. a possible null-deref) that `&&`/`\|\|` would guard → short-circuit | ### `this`, casts & inheritance | Rule | Flags → do instead | |---|---| | `cs/cast-of-this-to-type-parameter` | casting `this` to a type param → rethink the API | | `cs/downcast-of-this` | downcasting `this` to a derived type → virtual method | | `cs/type-test-of-this` | `this is DerivedType` → virtual method | | `cs/impossible-array-cast` | array cast that always fails at runtime → fix the type | | `cs/class-name-matches-base-class` | subclass named like its base → rename | | `cs/field-masks-base-field` | field shadowing a base field → rename or reuse | | `cs/local-shadows-member` | local hiding a field/property/param → rename | ### API & design smells | Rule | Flags → do instead | |---|---| | `cs/path-combine` **†** | `Path.Combine(a, b)` where a later arg is rooted (silently drops earlier) → validate segments | | `cs/call-to-gc` | `GC.Collect()` → remove; let the runtime manage it | | `cs/call-to-obsolete-method` | calling `[Obsolete]` API → use the replacement | | `cs/call-to-unmanaged-code`, `cs/unmanaged-code` | P/Invoke / unsafe surface → review necessity (often expected for CsWin32) | | `cs/class-implements-icloneable` | `ICloneable` (ambiguous deep/shallow) → a typed clone method | | `cs/expose-implementation` **†** | returning/storing a reference to an internal mutable array/collection that is **later modified** through the alias → return a copy or read-only view | | `cs/static-field-written-by-instance` | instance method writing a static field → rethink ownership | | `cs/missed-readonly-modifier` **†** | a private field written only at its declaration and/or in the (static) constructor, never elsewhere → `readonly` | | `cs/loss-of-precision` | integer division/multiplication whose result is converted to `float`/`double`/`decimal` — precision is already lost before the conversion (e.g. `(double)(a / b)`) → cast an operand first | | `cs/invalid-string-formatting` | format string vs args mismatch → fix placeholders | | `cs/string-concatenation-in-loop` **†** | `s += …` in a loop → `StringBuilder` | | `cs/stringbuilder-creation-in-loop` **†** | `new StringBuilder()` inside a loop → hoist it out **only if** it doesn't escape the iteration, calling `.Clear()` at the start of each pass; otherwise leave it | | `cs/stringbuilder-initialized-with-character` | `new StringBuilder('x')` (that's capacity!) → `.Append('x')` or `"x"` | | `cs/asp/response-write` | `Response.Write` of a single block → not relevant to this CLI; skip unless ASP code appears | ## 3. Report Print to stdout. No file output and **no fixes** unless asked. ```markdown # Code Quality pre-check — <head> vs origin/main (csharp-code-quality, static) ## Verdict <nothing obvious | N likely finding(s)> — <one plain sentence> ## Likely bot comments ### <rule id> — <short plain title> <mark (lower confidence) for † rules> - **What it flags:** <one sentence> - **Where:** `<path>:<line>` - **Show me:** <the snippet from the diff> - **Smallest fix:** <least-complex change> <Repeat per finding, or "None obvious on your changed lines."> ## Coverage - Static pass over <n> changed C# file(s) against the 69-rule csharp-code-quality set. - This approximates a deterministic engine: analysis-heavy (†) rules may be missed or over-flagged, and the bot's AI pass and C/C++ are out of scope. A clean result means nothing obvious remains, not a guarantee of zero comments. ``` ## If asked to fix Apply the **smallest** change that clears each finding on the changed lines, one rule at a time. Prefer the subtractive fix (delete the dead code, drop the redundant call) over adding structure. Do not touch pre-existing findings unless explicitly asked. ## Maintaining this skill The tables above are a **point-in-time snapshot** of the `csharp-code-quality` suite (69 rules). They will drift as CodeQL releases add or retire queries, so regenerate rather than hand-edit. With the CodeQL bundle available, the current membership is: ```powershell codeql resolve queries codeql/csharp-queries:codeql-suites/csharp-code-quality.qls # each resolved .ql file's header carries its @id / @name / @precision ``` If that list diverges from the tables, reconcile it — add new rules, remove retired ones — and keep the **†** markers on the flow/type-analysis queries so their lower confidence stays visible. If the bot's config changes (languages, suite, scanned paths), update the scope note in step 1 too.