Add code-reviewer skill with conditional rule files

.github/skills/code-reviewer/SKILL.md

@@ -0,0 +1,124 @@
+---
+name: code-reviewer
+description: >-
+  Review dotnes PRs against established rules. Trigger on "review this PR",
+  a GitHub PR URL, or code review requests. Checks transpiler correctness,
+  6502 assembly, NES program conventions, MSBuild integration, snapshot tests,
+  C# patterns, and AI-generated code pitfalls.
+---
+
+# dotnes PR Reviewer
+
+Review PRs against guidelines for the dotnes transpiler — a tool that converts .NET IL into 6502 machine code to produce NES ROMs.
+
+## Review Mindset
+
+Be polite but skeptical. Prioritize bugs, correctness regressions, and transpiler safety over style nitpicks. **3 important comments > 15 nitpicks.**
+
+This is a compiler/transpiler project — correctness is paramount. A subtle bug in opcode emission can produce a ROM that silently does the wrong thing, and the only way to catch it may be running in an emulator. Treat every change to `Transpiler.cs`, `IL2NESWriter.cs`, `NESWriter.cs`, `BuiltInSubroutines.cs`, or `Program6502.cs` with extra scrutiny.
+
+Flag severity clearly in every comment:
+- ❌ **error** — Must fix before merge. Bugs, incorrect 6502 emission, broken snapshot tests, security issues.
+- ⚠️ **warning** — Should fix. Performance issues, missing test coverage, inconsistency with patterns.
+- 💡 **suggestion** — Consider changing. Style, readability, optional improvements.
+
+**Every review should produce at least one inline comment.** Even clean PRs have opportunities for improvement — missing edge-case tests, documentation gaps, or code consolidation. Use 💡 suggestions for these. Only omit inline comments for truly trivial PRs (1-line typo fix, dependency bump).
+
+## Workflow
+
+### 1. Identify the PR
+
+If triggered from an agentic workflow (slash command on a PR), use the PR from the event context. Otherwise, extract `owner`, `repo`, `pr_number` from a URL or reference provided by the user.
+Formats: `https://github.com/{owner}/{repo}/pull/{number}`, `{owner}/{repo}#{number}`, or bare number (defaults to `jonathanpeppers/dotnes`).
+
+### 2. Gather context (before reading PR description)
+
+```
+gh pr diff {number} --repo {owner}/{repo}
+gh pr view {number} --repo {owner}/{repo} --json files
+```
+
+For each changed file, read the **full source file** (not just the diff) to understand surrounding invariants, call patterns, and data flow. If the change modifies a public/internal API or utility, search for callers. Check whether sibling types need the same fix.
+
+**Form an independent assessment** of what the change does and what problems it has *before* reading the PR description.
+
+### 3. Incorporate PR narrative and reconcile
+
+```
+gh pr view {number} --repo {owner}/{repo} --json title,body
+```
+
+Now read the PR description and linked issues. Treat them as claims to verify, not facts to accept. Where your independent reading disagrees with the PR description, investigate further. If the PR claims a performance improvement, require evidence. If it claims a bug fix, verify the bug exists and the fix addresses root cause — not symptoms.
+
+### 4. Check CI status
+
+```
+gh pr checks {number} --repo {owner}/{repo}
+```
+
+Review the CI results. **Never post ✅ LGTM if any required CI check is failing or if the code doesn't build.** If CI is failing:
+- Investigate the failure.
+- If the failure is caused by the PR's code changes, flag it as ❌ error.
+- If the failure is a known infrastructure issue or pre-existing flake unrelated to the PR, note it in the summary but still use ⚠️ Needs Changes — the PR isn't mergeable until CI is green.
+
+### 5. Load review rules
+
+Based on the file types identified in step 2, read the appropriate rule files from this skill's `references/` directory.
+
+**Always load:**
+- `references/repo-conventions.md` — Formatting, style, and patterns specific to dotnes.
+- `references/ai-pitfalls.md` — Common AI-generated code mistakes.
+
+**Conditionally load based on changed file types:**
+- `references/csharp-rules.md` — When any `.cs` files changed. Covers nullable, async, error handling, performance, and code organization.
+- `references/transpiler-rules.md` — When files under `src/dotnes.tasks/` changed, especially `Transpiler.cs`, `IL2NESWriter.cs`, `NESWriter.cs`, `BuiltInSubroutines.cs`, or `Program6502.cs`. The core of this project.
+- `references/nes-program-rules.md` — When files under `samples/` changed, or when `NESLib.cs` changed, or when the diff contains NES API calls (e.g., `pal_col`, `ppu_on_all`, `oam_spr`). Covers NES program constraints and neslib API usage.
+- `references/testing-rules.md` — When test files changed (files under `src/dotnes.tests/`) or when transpiler changes lack corresponding test additions.
+- `references/msbuild-rules.md` — When `.targets`, `.props`, or `.csproj` files changed, or when `TranspileToNES.cs` changed.
+- `references/security-rules.md` — When any code files changed (C# or MSBuild).
+
+### 6. Analyze the diff
+
+For each changed file, check against the loaded review rules. Record issues as:
+
+```json
+{ "path": "src/Example.cs", "line": 42, "side": "RIGHT", "body": "..." }
+```
+
+**What to look for (in priority order):**
+1. **Transpiler correctness** — Wrong opcodes, incorrect address modes, broken label resolution, ROM layout changes
+2. **Snapshot regressions** — Changes that would alter `.verified.bin` output for unchanged samples
+3. **Bugs & correctness** — Logic errors, off-by-one, null dereferences
+4. **Missing tests** — Transpiler changes without `RoslynTests`, new samples without snapshot data
+5. **Performance** — Unnecessary allocations, O(n²) patterns in hot transpiler paths
+6. **Code duplication** — Near-identical methods that should be consolidated
+7. **Documentation** — Misleading comments, undocumented behavioral decisions, missing `docs/msbuild-properties.md` updates
+
+Constraints:
+- Only comment on added/modified lines in the diff — the API rejects out-of-range lines.
+- `line` = line number in the NEW file (right side). Double-check against the diff.
+- One issue per comment.
+- **Don't pile on.** If the same issue appears many times, flag it once with a note listing all affected files.
+- **Don't flag what CI catches.** Skip compiler errors, formatting the linter will catch, etc.
+- **Avoid false positives.** Verify the concern actually applies given the full context. If unsure, phrase it as a question rather than a firm claim.
+
+### 7. Post the review
+
+Post your findings directly:
+
+- **Inline comments** on specific lines of the diff with the severity, category, and explanation.
+- **Review summary** with the overall verdict (✅ LGTM, ⚠️ Needs Changes, or ❌ Reject), issue counts by severity, and positive callouts.
+
+If no issues found **and CI is green**, submit with at most one or two 💡 suggestions and a positive summary. Truly trivial PRs (dependency bumps, 1-line typo fixes) may have no inline comments.
+
+**Copilot-authored PRs:** If the PR author is `Copilot` (the GitHub Copilot coding agent) and the verdict is ⚠️ Needs Changes or ❌ Reject, prefix the review summary with `@copilot ` so the comment automatically triggers Copilot to address the feedback. Do NOT add the prefix for ✅ LGTM verdicts.
+
+## Comment format
+
+```
+🤖 {severity} **{Category}** — {What's wrong and what to do instead.}
+```
+
+Where `{severity}` is ❌, ⚠️, or 💡.
+
+**Categories:** Transpiler correctness · 6502 emission · ROM layout · Snapshot integrity · NES program · neslib API · MSBuild · Nullable · Async pattern · Error handling · Performance · Code organization · Testing · YAGNI · API design · Documentation · Security

.github/skills/code-reviewer/references/ai-pitfalls.md

@@ -0,0 +1,26 @@
+# AI Code Generation Pitfalls
+
+Patterns that AI-generated code consistently gets wrong in this repository.
+Always loaded during reviews.
+
+---
+
+## Common AI Mistakes
+
+| Pattern | What to watch for |
+|---------|------------------|
+| **Reinventing the wheel** | AI creates new infrastructure instead of using existing utilities. ALWAYS check if a similar utility exists before accepting new wrapper code. This is the most expensive AI pattern — hundreds of lines of plausible code that duplicates what's already there. |
+| **Over-engineering** | Speculative helper classes, unused overloads, "for testability" abstractions nobody asked for. If no caller needs it today, remove it. |
+| **Swallowed errors** | AI catch blocks love to eat exceptions silently. Check EVERY catch block. Also check that the MSBuild task returns `!Log.HasLoggedErrors`. |
+| **Null-forgiving operator (`!`)** | The postfix `!` null-forgiving operator (e.g., `foo!.Bar`) should be avoided. If the value can be null, add a proper null check. If it can't be null, make the type non-nullable. AI frequently sprinkles `!` to silence the compiler. Note: this rule is about the postfix `!` operator, not the logical negation `!` (e.g., `if (!someBool)`). Exception: `NESLib.cs` methods use `throw null!` by design — that's the reference assembly pattern. |
+| **`string.Empty` and `Array.Empty<T>()`** | AI defaults to these. Use `""` and `[]` instead. The `.editorconfig` enforces `IDE0300` as an error for collection expressions. |
+| **Sloppy structure** | Multiple types in one file, block-scoped namespaces, `#region` directives (except in `BuiltInSubroutines.cs`), classes where records would do. New helpers marked `public` when `internal` suffices. |
+| **Wrong NES assumptions** | AI confidently generates NES code using BCL types (`string`, `List<T>`, LINQ), classes/objects, or heap allocation — none of which work on a 6502. NES programs are pure procedural code with fixed-size byte arrays and zero-page variables only. |
+| **Redefining neslib constants** | AI creates local `const byte PAD_A = 0x01` or similar redefinitions of enums/constants already provided by neslib (`PAD`, `MASK`, PPU registers, APU constants). Use the built-in definitions directly — redefinitions lead to bugs when values are wrong. |
+| **Adding implementations to NESLib.cs** | AI tries to add real method bodies to `NESLib.cs`. Every method there must be `=> throw null!` — the transpiler maps method names to 6502 subroutines, it never runs the C# body. |
+| **Unused parameters** | AI adds `CancellationToken` or other parameters but never observes them. If a parameter is accepted, it must be used. |
+| **Confidently wrong 6502 facts** | AI makes authoritative claims about 6502 behavior (addressing modes, flag effects, cycle counts) that are wrong. Always verify 6502 claims against the [instruction set reference](https://www.masswerk.at/6502/6502_instruction_set.html). |
+| **Docs describe intent not reality** | AI doc comments often describe what the code *should* do, not what it *actually* does. Review doc comments against the implementation. |
+| **Filler words in docs** | "So" at the start of a sentence adds nothing. "Basically" and "essentially" are padding. Be direct. |
+| **`git commit --amend`** | AI uses `--amend` on commits. Always create new commits — the maintainer will squash as needed. |
+| **Modifying `.verified.bin` files** | AI updates snapshot files to make tests pass after a code change. The `.verified.bin` files are the source of truth — if a code change produces different bytes for an unchanged sample, the code is wrong, not the snapshot. Only update snapshots when the sample's `Program.cs` itself changed. |

.github/skills/code-reviewer/references/csharp-rules.md

@@ -0,0 +1,53 @@
+# C# Review Rules
+
+General C# guidance applicable across this repository.
+
+---
+
+## Nullable Reference Types
+
+| Check | What to look for |
+|-------|-----------------|
+| **Nullable is project-level** | Nullable is enabled via `<Nullable>enable</Nullable>` in project files. Per-file `#nullable enable` is unnecessary. |
+| **Avoid `!` (null-forgiving operator)** | The postfix `!` null-forgiving operator should be avoided. Add proper null checks or make the type non-nullable. Exception: `NESLib.cs` uses `throw null!` by design. |
+| **`ArgumentNullException.ThrowIfNull`** | Use `ArgumentNullException.ThrowIfNull(param)` for parameter validation — but only in `net10.0`-targeting projects. The `dotnes.tasks` project targets `netstandard2.0` where this API is unavailable; use explicit `if (x is null) throw new ArgumentNullException(nameof(x));` there instead. |
+
+---
+
+## Error Handling
+
+| Check | What to look for |
+|-------|-----------------|
+| **No empty catch blocks** | Every `catch` must capture the `Exception` and log it (or rethrow). No silent swallowing. |
+| **Fail fast on critical ops** | If a critical operation fails (file not found, invalid IL), throw immediately. Silently continuing leads to confusing downstream failures or broken ROMs. |
+| **Include actionable details in exceptions** | Use `nameof` for parameter names. Include the unsupported value or unexpected type. Never throw empty exceptions. |
+| **Challenge exception swallowing** | When a PR adds `catch { continue; }` or `catch { return null; }`, question whether the exception is truly expected or masking a deeper problem. |
+
+---
+
+## Performance
+
+| Check | What to look for |
+|-------|-----------------|
+| **Avoid unnecessary allocations** | Don't create intermediate collections when LINQ chaining or a single pass would do. Char arrays for `string.Split()` should be `static readonly` fields. |
+| **`HashSet.Add()` already handles duplicates** | Calling `.Contains()` before `.Add()` does the hash lookup twice. Just call `.Add()`. |
+| **Don't wrap a value in an interpolated string** | `$"{someString}"` creates an unnecessary `string.Format` call when `someString` is already a string. |
+| **Pre-allocate collections when size is known** | Use `new List<T>(capacity)` or `new Dictionary<TK, TV>(count)` when the size is known or estimable. |
+| **Avoid closures in hot paths** | Lambdas that capture local variables allocate a closure object on every call. In the transpiler's main loop or frequently-called emit methods, extract the lambda to a static method or cache the delegate. |
+| **Place cheap checks before expensive ones** | In validation chains, test simple conditions (null checks, boolean flags) before allocating strings or doing I/O. Short-circuit with `&&`/`||`. |
+| **Watch for O(n²)** | Nested loops over the same or related collections, repeated `.Contains()` on a `List<T>`, or LINQ `.Where()` inside a loop are O(n²). Switch to `HashSet<T>` or `Dictionary<TK, TV>` for lookups. |
+| **Use `.Ordinal` for identifier comparisons** | `.Ordinal` is faster than `.OrdinalIgnoreCase`. Use `.OrdinalIgnoreCase` only for filesystem paths. IL method names, opcode strings, and label names should use `.Ordinal`. |
+
+---
+
+## Code Organization
+
+| Check | What to look for |
+|-------|-----------------|
+| **One type per file** | Each public class, struct, enum, or interface must be in its own `.cs` file named after the type. Partial classes are fine (e.g., `Transpiler` is `partial`). |
+| **Use `record` for data types** | Immutable data-carrier types should be `record` types — they get value equality, `ToString()`, and deconstruction for free. |
+| **Remove unused code** | Dead methods, speculative helpers, and code "for later" should be removed. Ship only what's needed. No commented-out code — Git has history. |
+| **New helpers default to `internal`** | New utility methods should be `internal` unless a confirmed external consumer needs them. Use `InternalsVisibleTo` for test access (already configured in this project). |
+| **Reduce indentation with early returns** | Invert conditions and `return`/`continue` early so the main logic has less nesting. |
+| **Don't initialize fields to default values** | `bool flag = false;` and `int count = 0;` are noise. The CLR zero-initializes all fields. Only assign when the initial value is non-default. |
+| **Well-named constants over magic numbers** | `if (address > 0xFFFF)` is fine for 6502 address space (well-known boundary). But buffer sizes, retry counts, and obscure thresholds should be named constants. |

.github/skills/code-reviewer/references/msbuild-rules.md

@@ -0,0 +1,38 @@
+# MSBuild Review Rules
+
+MSBuild task and target guidance for the dotnes build infrastructure.
+
+---
+
+## MSBuild Task (TranspileToNES.cs)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Return `!Log.HasLoggedErrors`** | `Execute()` must return `!Log.HasLoggedErrors`. Do not return `true`/`false` directly — it bypasses the centralized error-tracking mechanism. |
+| **`[Required]` properties need defaults** | `[Required]` properties must have a default value: `public string Foo { get; set; } = "";` or `public string[] Bar { get; set; } = [];`. Non-`[Required]` properties should be nullable where appropriate. |
+| **Dispose resources** | The transpiler creates `PEReader`, `StreamReader`, and other disposable objects. Verify `using` statements are present. Leaked file handles prevent rebuilds on Windows. |
+| **Logger null-object pattern** | `Logger ??= DiagnosticLogging ? new MSBuildLogger(Log) : null;` — the logger is null when diagnostic logging is off. Code that uses `Logger` must handle the null case (the `ILogger?` interface is nullable). |
+
+---
+
+## MSBuild Targets (dotnes.targets / dotnes.props)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Incremental builds (`Inputs`/`Outputs`)** | The `Transpile` target must have `Inputs` and `Outputs` so MSBuild can skip it when nothing changed. Inputs include `$(TargetPath)`, `@(NESAssembly)`, and the properties stamp file. |
+| **Properties stamp file** | A `_WriteNESPropertiesStamp` target writes NES property values to a stamp file. This ensures changing a property like `NESBattery` retriggers transpilation. If a new MSBuild property is added, it must be included in this stamp file. |
+| **`TranspileDependsOn` extensibility** | The `Transpile` target uses `$(TranspileDependsOn)` for ordering. New dependencies should be added via this property, not `BeforeTargets`/`AfterTargets`. |
+| **XML indentation** | MSBuild/XML files use 2 spaces for indentation (per `.editorconfig`), not tabs. |
+| **`NoStdLib=true`** | `dotnes.props` sets `NoStdLib=true` to prevent BCL references. If a change removes or weakens this, NES programs could accidentally reference BCL types that the transpiler can't handle. |
+| **`Optimize=true`** | `dotnes.props` forces Release-mode IL optimization. The transpiler expects optimized IL patterns. Debug IL has different instruction sequences (extra `nop`, different branch patterns) that may not be handled. |
+
+---
+
+## Adding New MSBuild Properties
+
+| Check | What to look for |
+|-------|-----------------|
+| **Document in `docs/msbuild-properties.md`** | Every new public MSBuild property must be documented with: property name, type, default value, description, and an XML example. |
+| **Add to properties stamp** | If the property affects transpilation output, add it to the `_WriteNESPropertiesStamp` target so incremental builds correctly retrigger. |
+| **Wire through TranspileToNES.cs** | The MSBuild property must be passed to the task as a property, and the task must forward it to the `Transpiler` constructor. |
+| **Test both values** | If the property is boolean, test with both `true` and `false`. If it has enumerated values (like `NESMirroring`), test all valid values. |