diff --git a/.github/skills/code-reviewer/SKILL.md b/.github/skills/code-reviewer/SKILL.md
new file mode 100644
index 00000000..c3190538
--- /dev/null
+++ b/.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
diff --git a/.github/skills/code-reviewer/references/ai-pitfalls.md b/.github/skills/code-reviewer/references/ai-pitfalls.md
new file mode 100644
index 00000000..2d5c611c
--- /dev/null
+++ b/.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. |
diff --git a/.github/skills/code-reviewer/references/csharp-rules.md b/.github/skills/code-reviewer/references/csharp-rules.md
new file mode 100644
index 00000000..ea8a6a8a
--- /dev/null
+++ b/.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. |
diff --git a/.github/skills/code-reviewer/references/msbuild-rules.md b/.github/skills/code-reviewer/references/msbuild-rules.md
new file mode 100644
index 00000000..b2a5d2b7
--- /dev/null
+++ b/.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. |
diff --git a/.github/skills/code-reviewer/references/nes-program-rules.md b/.github/skills/code-reviewer/references/nes-program-rules.md
new file mode 100644
index 00000000..9fc607c6
--- /dev/null
+++ b/.github/skills/code-reviewer/references/nes-program-rules.md
@@ -0,0 +1,55 @@
+# NES Program Review Rules
+
+Rules for reviewing NES program samples (code under `samples/`) and changes to
+the NESLib API (`src/neslib/NESLib.cs`).
+
+---
+
+## NES Program Constraints
+
+NES programs run on a 6502 processor with 2KB RAM, no operating system, and no
+garbage collector. The transpiler supports a strict subset of C#.
+
+| Check | What to look for |
+|-------|-----------------|
+| **Must end with `while (true) ;`** | Every NES program must end with an infinite loop. The NES has no concept of "exit" — without the loop, the CPU executes garbage bytes past the end of your code. |
+| **Top-level statements only** | Programs use top-level statements (no `class Program { static void Main() }`). User-defined static methods with parameters and return values are supported. |
+| **No BCL types** | `string`, `List<T>`, `Dictionary<K,V>`, LINQ, `Console.WriteLine`, and all Base Class Library types are unavailable. Programs can only use `byte`, `ushort`, `int`, `bool`, byte arrays (`byte[]`), ushort arrays (`ushort[]`), and NESLib API calls. |
+| **No classes or objects** | No `new`, no heap allocation, no reference types (except arrays which are ROM tables). Everything is value types and static methods. |
+| **No string manipulation** | The NES has no string support. Text display uses tile indices via `vram_put`/`vram_write` or the `set_vram_update` mechanism. |
+| **Fixed-size byte arrays** | `byte[]` initializers become ROM data tables. They are read-only and baked into the ROM at compile time. Don't try to modify them at runtime. |
+
+---
+
+## NESLib API Usage
+
+| Check | What to look for |
+|-------|-----------------|
+| **Never redefine neslib constants** | The `PAD` enum (`PAD.A`, `PAD.UP`, `PAD.LEFT`, etc.), `MASK` constants, PPU register constants, and APU constants are all built-in. Do NOT create local `const byte PAD_A = 0x01` or similar redefinitions. Use the built-in definitions directly. |
+| **NESLib.cs methods are stubs only** | Every method in `NESLib.cs` must be `=> throw null!`. If someone adds a real implementation, that's wrong — the transpiler uses method name lookup, never the C# body. |
+| **Adding new NESLib methods** | When adding a new NES API: (1) add a stub method in `NESLib.cs` with `throw null!`, (2) implement the 6502 subroutine in `BuiltInSubroutines.cs`, (3) wire it up in the transpiler so `UsedMethods` tracking works. Missing any step means the method either won't compile, won't emit code, or will emit a `JSR` to nowhere. |
+| **Palette data sizes** | `pal_all` expects exactly 32 bytes. `pal_bg` and `pal_spr` expect exactly 16 bytes. Wrong sizes produce garbled colors. |
+| **Sprite limits** | The NES supports 64 sprites (256 bytes of OAM). Exceeding this silently overwrites previous sprites. The `oam_spr` return value tracks the OAM index — verify it's checked. |
+| **PPU timing** | VRAM writes (`vram_put`, `vram_adr`, `vram_write`) must happen during vblank (between `ppu_wait_nmi` and `ppu_on_all`/`ppu_on_bg`/`ppu_on_spr`). Writing to VRAM while rendering is active produces graphical glitches. |
+
+---
+
+## Sample Project Conventions
+
+| Check | What to look for |
+|-------|-----------------|
+| **Every sample needs a `.csproj`** | Samples must have a project file that references `dotnes`. Check that `<PackageReference Include="dotnes" />` is present. |
+| **CHR ROM files** | Most samples need a `chr_*.s` file for graphics. The test infrastructure looks for `chr_{samplename}.s` first, then falls back to `chr_generic.s`. If a sample uses custom tiles, it needs its own CHR file. |
+| **Test coverage** | Every sample should have a corresponding `[InlineData]` entry in `TranspilerTests.Write` with a `.verified.bin` snapshot. Missing test data means the sample can silently break. |
+| **`README.md` or comments** | Non-trivial samples should explain what they demonstrate (scrolling, bank switching, music, etc.) so future contributors understand the purpose. |
+
+---
+
+## Music Architecture
+
+| Check | What to look for |
+|-------|-----------------|
+| **Music subroutines go before `main()`** | In the ROM layout, `music_tick` and `start_music` are emitted before the `main` entry point to match cc65's layout. Don't reorder these. |
+| **Note table format** | `set_music_pulse_table(ushort[])` and `set_music_triangle_table(ushort[])` store note frequencies as interleaved lo/hi byte pairs. The transpiler handles the byte interleaving — the C# code provides a plain `ushort[]`. |
+| **`apu_init()` before `start_music()`** | The APU must be initialized before starting music playback. Calling `start_music` without `apu_init` produces undefined APU state. |
+| **`music_tick()` in the main loop** | `music_tick()` (the built-in music engine) must be called every frame (inside the `while (true)` loop, typically after `ppu_wait_nmi`). Missing it causes music to stall. Note: `music_play(byte)` is the separate FamiTone2 extern — don't confuse the two. |
diff --git a/.github/skills/code-reviewer/references/repo-conventions.md b/.github/skills/code-reviewer/references/repo-conventions.md
new file mode 100644
index 00000000..85cbb20d
--- /dev/null
+++ b/.github/skills/code-reviewer/references/repo-conventions.md
@@ -0,0 +1,64 @@
+# Repo Conventions
+
+Patterns, naming, and style rules specific to the dotnes repository. Always
+loaded during reviews.
+
+---
+
+## Formatting & Style
+
+This project uses standard C# formatting with spaces (not Mono style). The
+`.editorconfig` defines the conventions.
+
+| Check | What to look for |
+|-------|-----------------|
+| **Spaces, not tabs** | Indentation uses spaces (4 spaces for C#, 2 spaces for XML/MSBuild). |
+| **Standard C# spacing** | No space before `(` in method calls: `Foo()`, not `Foo ()`. No space before `[` in array access: `array[0]`, not `array [0]`. |
+| **Allman braces** | Opening braces go on their own line (`csharp_new_line_before_open_brace = all`). |
+| **File-scoped namespaces** | Use `namespace Foo;` not `namespace Foo { }`. |
+| **No `#region`/`#endregion`** | Region directives hide code and make reviews harder. Remove them. Exception: `BuiltInSubroutines.cs` uses `#region` to group large blocks of related subroutines — this is acceptable there. |
+| **Minimal diffs** | Don't leave random empty lines. Preserve existing formatting and comments in files you didn't write. |
+| **`[]` not `Array.Empty<T>()`** | Use collection expressions (`[]`) for empty arrays. The `.editorconfig` enforces `IDE0300` as an error. |
+| **Separate import groups** | `using` directives are grouped: system namespaces first, then others, with blank lines between groups (`dotnet_separate_import_directive_groups = true`). |
+| **`readonly` fields** | Fields that are only assigned in the constructor should be `readonly` (enforced as warning by `.editorconfig`). |
+| **Private field naming** | Private fields use `_camelCase` prefix. Private static fields use `s_camelCase` prefix. Constants use `PascalCase`. |
+
+---
+
+## Architecture Awareness
+
+Understanding the transpilation pipeline is essential for reviewing changes.
+
+| Check | What to look for |
+|-------|-----------------|
+| **Reference assembly pattern** | `neslib` methods have **no implementations** — they all `throw null!`. The transpiler looks up method names to emit 6502 subroutine calls. If someone adds real logic to `NESLib.cs`, that's wrong — it should stay as stubs. |
+| **Transpiler is single-pass** | `Transpiler.cs` reads IL in a single forward pass. Changes that assume random access or multiple passes over the IL stream will break. |
+| **Block-based assembly** | `BuiltInSubroutines.cs` creates `Block` objects with label-based forward references. `Program6502.cs` resolves addresses. Changes here affect ROM layout for ALL samples. |
+| **IL2NESWriter emits 6502** | This is the core IL → 6502 mapping. Every `case` in the instruction switch must emit correct 6502 bytes. A wrong opcode here silently produces a broken ROM. |
+
+---
+
+## Patterns & Conventions
+
+| Check | What to look for |
+|-------|-----------------|
+| **Use existing utilities** | Check `Transpiler`, `NESWriter`, `IL2NESWriter`, `AssemblyReader`, `NESConstants` before writing new helpers. |
+| **Return `!Log.HasLoggedErrors`** | The MSBuild task `TranspileToNES.Execute()` must return `!Log.HasLoggedErrors`, not `true`/`false` directly. |
+| **`ILogger` for diagnostics** | Use `ILogger` (the project's own interface), not `Console.WriteLine` or `Debug.WriteLine`. Diagnostic output is controlled by `NESDiagnosticLogging`. |
+| **Comments explain "why", not "what"** | `// increment i` adds nothing. `// skip the NES header — 16 bytes` explains intent. |
+| **Track TODOs as issues** | A `// TODO` hidden in code will be forgotten. File an issue and reference it in the comment. |
+| **Remove stale comments** | If the code changed, update the comment. Comments describing old behavior are misleading. |
+| **Update `docs/msbuild-properties.md`** | When adding new public MSBuild properties, always add documentation with property name, type, default value, description, and an XML example. |
+| **Don't commit to `main`** | All changes must go through pull requests — no direct pushes to `main`. |
+
+---
+
+## Code Review Anti-Patterns
+
+These are specific patterns that have been flagged in past reviews. **Do NOT suggest these changes:**
+
+| Anti-pattern | Why it's wrong |
+|-------------|---------------|
+| **Using `.Where()` when the loop body needs `out` parameters from `TryResolve`** | The `Where()` clause calls `TryResolve` twice (slower) and can't capture the `out` parameter. Use a `foreach` with `if (TryResolve(..., out var x))` instead. |
+| **Renaming parameters to avoid shadowing field names** | A parameter `local` in `WriteStloc(Local local)` is fine even though there's a field `readonly ushort local = 0x325`. The types are different and context is clear. Don't create noise renames. |
+| **"Simplifying" conditional logic without understanding semantic coupling** | `if (needsDecsp4 && usedMethods.Contains("pad_poll"))` adds `PadTrigger`/`PadState` blocks intentionally — `pad_trigger` and `pad_state` are internal implementation dependencies of `pad_poll`, not independently user-facing. Don't suggest checking them individually. |
diff --git a/.github/skills/code-reviewer/references/security-rules.md b/.github/skills/code-reviewer/references/security-rules.md
new file mode 100644
index 00000000..4d043399
--- /dev/null
+++ b/.github/skills/code-reviewer/references/security-rules.md
@@ -0,0 +1,38 @@
+# Security Review Rules
+
+Security checklist for code reviews. Applicable when any code files change.
+
+---
+
+## File & Path Safety
+
+| Check | What to look for |
+|-------|-----------------|
+| **Path traversal** | `StartsWith()` checks on paths must normalize with `Path.GetFullPath()` first. A path like `C:\safe\..\evil` bypasses naive prefix checks. |
+| **File overwrite protection** | The transpiler writes `.nes` files to `$(OutputPath)`. Verify the output path is validated and doesn't allow writing outside the expected build directory. |
+
+---
+
+## Process & Command Safety
+
+| Check | What to look for |
+|-------|-----------------|
+| **No command injection** | If any code spawns processes (e.g., running Mesen for testing), arguments must not be interpolated from user input. Use `ArgumentList` or separate argument arrays. |
+
+---
+
+## Supply Chain
+
+| Check | What to look for |
+|-------|-----------------|
+| **Review NuGet dependency changes** | If `PackageReference` versions change, verify the update is intentional and from a trusted source. Check for known vulnerabilities. |
+| **Assembly file integrity** | `.s` assembly files are included in the ROM verbatim. A malicious `.s` file could contain arbitrary 6502 code. Verify that new or modified `.s` files contain expected content. |
+
+---
+
+## Data Integrity
+
+| Check | What to look for |
+|-------|-----------------|
+| **ROM output determinism** | The same source code must always produce the same ROM bytes. Non-deterministic output (timestamps, random seeds, GUID-based ordering) in the transpiler would break snapshot tests and make builds unreproducible. |
+| **No secrets in ROM** | The ROM is a binary artifact that can be freely distributed. Verify no API keys, tokens, file paths, or other sensitive data are baked into the ROM output. |
diff --git a/.github/skills/code-reviewer/references/testing-rules.md b/.github/skills/code-reviewer/references/testing-rules.md
new file mode 100644
index 00000000..60e467f0
--- /dev/null
+++ b/.github/skills/code-reviewer/references/testing-rules.md
@@ -0,0 +1,57 @@
+# Testing Review Rules
+
+Testing conventions for the dotnes test suite (`src/dotnes.tests/`).
+
+---
+
+## Testing Framework
+
+This project uses **xUnit** with **Verify** snapshots. Tests are run with
+`dotnet test` (which always rebuilds — never use `--no-build`).
+
+---
+
+## Snapshot Testing
+
+| Check | What to look for |
+|-------|-----------------|
+| **`.verified.bin` files are the source of truth** | For existing samples, the `.verified.bin` files contain the expected ROM output byte-for-byte. Any code change that causes `TranspilerTests.Write` to produce different bytes for an **unchanged** sample is WRONG — fix the code, not the snapshot. |
+| **Only update snapshots when the sample changes** | When a sample's `Program.cs` is modified (new features, bug fixes), rebuild its test DLLs and update the `.verified.bin` to match the new expected output. Document why the snapshot changed in the commit message. |
+| **New samples need snapshots** | Every new sample added to `samples/` must have a corresponding `[InlineData]` entry in `TranspilerTests.Write` and a `.verified.bin` file. Missing snapshots mean the sample can silently break. |
+| **Per-sample CHR ROM** | Tests look for `chr_{name}.s` in the `Data/` folder first, falling back to `chr_generic.s`. The music sample uses an empty CHR. If a new sample has custom graphics, add its CHR file to `Data/`. |
+
+---
+
+## RoslynTests (Preferred for New Features)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Prefer RoslynTests over new samples** | When adding or testing new IL opcode support, write `RoslynTests` instead of creating new `samples/` directories and pre-compiled DLLs. RoslynTests are self-contained (C# source + assertions in one method) and easier to maintain. |
+| **Use `GetProgramBytes(source)`** | `GetProgramBytes` compiles C# via Roslyn and transpiles to 6502 bytes. Assert on the hex output with `Assert.Contains`. This tests the full pipeline end-to-end. |
+| **Use `AssertProgram(source, expectedAssembly)`** | `AssertProgram` checks the exact 6502 assembly output of the main block. Use this for precise byte-level verification of new opcode support. |
+| **C# source includes implicit usings** | `RoslynTests` automatically prepends `using NES;using static NES.NESLib;`. Don't include these in the test source strings. |
+
+---
+
+## TranspilerTests
+
+| Check | What to look for |
+|-------|-----------------|
+| **Debug and Release DLLs** | `TranspilerTests.Write` tests both Debug and Release builds via `[InlineData("name", true)]` and `[InlineData("name", false)]`. Both configurations must produce the same ROM output. |
+| **`ReadStaticVoidMain` tests** | These verify IL parsing produces the expected instruction sequence. They use `.verified.txt` files (text, not binary). |
+| **Test data DLLs in `Data/`** | Pre-compiled DLLs live in `src/dotnes.tests/Data/`. When adding new test cases (legacy approach), compile the sample, copy the `.dll` to `Data/Debug/` and `Data/Release/`, and add `[InlineData]` entries. |
+
+---
+
+## General Testing Patterns
+
+| Check | What to look for |
+|-------|-----------------|
+| **Bug fixes need regression tests** | Every PR that fixes a bug should include a test that fails without the fix and passes with it. If the PR says "fixes #N" but adds no test, ask for one. |
+| **Test assertions must be specific** | `Assert.NotNull(result)` doesn't tell you what went wrong. Prefer `Assert.Equal(expected, actual)` or `Assert.Contains(expectedHex, actualHex)` for richer failure messages. |
+| **Prefer byte-for-byte equality over substring checks** | When testing that two code paths produce the same 6502 output, use `Assert.Equal(expectedBytes, actualBytes)` — not `Assert.Contains` on individual byte patterns. Substring/pattern checks can pass even when the outputs differ in critical ways (wrong ordering, extra instructions, missing instructions). |
+| **Bound-check before indexing emitted bytes** | Tests that index into emitted byte arrays (e.g., `bytes[clcIndex + 2]`) should verify the index is in bounds first. A regression that removes instructions can turn a meaningful assertion failure into a confusing `IndexOutOfRangeException`. |
+| **Assert locality, not just existence** | When checking that instruction X appears after instruction Y, don't just search the entire byte array. Constrain the search window (e.g., "within 3 instructions after Y") so the test catches cases where X exists but in the wrong position — like a JMP that's 50 bytes away instead of immediately after the target. |
+| **Test edge cases** | Empty byte arrays, single-instruction programs, maximum-size ROMs, boundary values for addressing modes (0, 255, 256), and programs with many local variables should all be considered. |
+| **Deterministic test data** | Tests should not depend on system locale, timezone, or current date. ROM output must be fully deterministic. |
+| **xUnit conventions** | Use `[Fact]` for single-case tests, `[Theory]` with `[InlineData]` for parameterized tests. Use constructor injection for `ITestOutputHelper`. |
diff --git a/.github/skills/code-reviewer/references/transpiler-rules.md b/.github/skills/code-reviewer/references/transpiler-rules.md
new file mode 100644
index 00000000..5a393309
--- /dev/null
+++ b/.github/skills/code-reviewer/references/transpiler-rules.md
@@ -0,0 +1,71 @@
+# Transpiler Review Rules
+
+Rules specific to the transpilation pipeline: `Transpiler.cs`,
+`IL2NESWriter.cs`, `NESWriter.cs`, `BuiltInSubroutines.cs`, and
+`Program6502.cs`. These are the most critical files in the project — bugs here
+produce silently broken ROMs.
+
+---
+
+## IL Reading (Transpiler.cs)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Single-pass constraint** | The transpiler reads IL in a single forward pass. Changes that assume random access to the IL stream, or that require re-reading previously consumed instructions, will break the pipeline. |
+| **UsedMethods tracking** | The transpiler tracks which NESLib methods are used (`UsedMethods` set) to decide which built-in subroutines to include in the ROM. If a new NESLib method is added but never registered in `UsedMethods`, the subroutine won't be emitted and the ROM will contain a `JSR` to an undefined address. |
+| **UserMethods vs built-in methods** | User-defined methods (from the sample's `Program.cs`) go into `UserMethods`. NESLib API calls go into `UsedMethods`. Confusing these produces wrong code — user methods get inlined 6502, while NESLib calls become `JSR` to built-in subroutines. |
+| **ExternMethods** | Methods declared as `static extern` are resolved to labels in external `.s` assembly files using cc65 convention (`_name`). Verify the label naming is correct. |
+| **Method metadata** | `UserMethodMetadata` tracks arg count, return value, and parameter types. If this metadata is wrong, the caller/callee will disagree on stack layout — corrupting zero-page variables. |
+| **ReadStaticVoidMain completeness** | When adding support for new IL patterns (e.g., new opcodes, new calling conventions), verify that `ReadStaticVoidMain` correctly populates all the data structures downstream consumers need. |
+
+---
+
+## 6502 Emission (IL2NESWriter.cs)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Correct opcode bytes** | Every 6502 instruction has a specific opcode byte for each addressing mode. `LDA immediate` is `$A9`, `LDA zero-page` is `$A5`, `LDA absolute` is `$AD`. Using the wrong addressing mode produces the wrong opcode byte — the ROM will execute the wrong instruction. Verify against the [6502 reference](https://www.masswerk.at/6502/6502_instruction_set.html). |
+| **Addressing mode matches operand** | Immediate operands are 1 byte (0-255). Zero-page addresses are 1 byte ($00-$FF). Absolute addresses are 2 bytes (lo, hi). If an address is > $FF but emitted as zero-page, the high byte is silently lost. |
+| **Branch offset calculation** | Relative branches (`BEQ`, `BNE`, `BPL`, `BMI`, `BCC`, `BCS`) use signed 8-bit offsets from the *next* instruction. An off-by-one in the offset jumps to the wrong location. Verify branch targets carefully. |
+| **Label resolution** | `EmitWithLabel` defers address resolution to `Program6502`. If a label name is misspelled or doesn't match the `Block` name, the address won't resolve and the ROM will have garbage bytes at that location. |
+| **Stack discipline** | The 6502 stack is only 256 bytes ($0100-$01FF). Unbalanced `PHA`/`PLA` or `JSR`/`RTS` will corrupt the stack. Every `JSR` must have a corresponding `RTS`. Every `PHA` must be balanced with `PLA` on all code paths. |
+| **Local variable allocation** | Local variables are allocated starting at `$0325` (`LocalStackBase` in `NESConstants.cs`). This is NES RAM, not zero page. Each new local gets a RAM slot (1 byte for `byte`, 2 bytes for `ushort`/`short`). If two variables share a slot when they shouldn't, they'll overwrite each other. Verify allocation doesn't overlap with NESLib's reserved RAM usage. |
+| **Flag side effects** | Many 6502 instructions implicitly modify the N (negative) and Z (zero) flags. Code that depends on flags from a previous instruction must not have intervening instructions that clobber those flags. Common trap: `LDA` sets N/Z, but a `STA` between the `LDA` and the `BEQ` doesn't change flags (safe). `LDX` between them would clobber flags (unsafe). |
+
+---
+
+## cc65 Software Stack (pusha/popa)
+
+The transpiler uses a *software stack* (cc65's `pusha`/`popa` subroutines) on
+top of the hardware 6502 stack. This is the most common source of subtle bugs.
+
+| Check | What to look for |
+|-------|-----------------|
+| **pusha/popa balance** | Every `pusha` (push accumulator to cc65 stack) must be paired with a `popa` on all code paths. If an intrinsic sets `argsAlreadyPopped = true` but conditionally pops (e.g., `if (Stack.Count > 0) Stack.Pop()`), the call-site stack can become imbalanced. Verify both the normal and edge-case paths balance. |
+| **Single-slot state variables** | The transpiler uses single-slot fields like `_pendingLeaveTarget`, `_savedConstantViaPusha`, and `_runtimeValueInA` to track state across IL instructions. A single slot can only hold one value — if multiple IL sequences need the same slot concurrently (e.g., nested `leave` targets, overlapping pusha chains), the second write silently overwrites the first. When adding or modifying these, consider whether the slot can be entered from multiple paths. |
+| **State flag interactions** | Flags like `_savedConstantViaPusha` and `_runtimeValueInA` interact with each other. Widening one flag's scope (e.g., making popa trigger in more cases) can conflict with other paths that use the same cc65 stack slot for different purposes (function-call arguments vs arithmetic operands). Trace through all paths that read the flag. |
+| **Backward instruction scans** | The transpiler sometimes scans backward through previously emitted bytes to find/modify/remove instructions (e.g., removing a preceding `LDA` when inlining an intrinsic, or patching branch offsets). These scans are inherently fragile — they assume a specific instruction sequence (e.g., "5 consecutive `ldc` instructions") that may break if the C# compiler changes its IL emission patterns. When reviewing backward scans: (1) check that the scan has a failure mode (throws, not silently produces wrong code), (2) document the assumed IL pattern. |
+| **Conditional operation tracking** | Patterns like `hasOr`/`orMask`, `hasAnd`/`andMask` pair a boolean flag with a value. The flag may be set in cases where the value isn't populated (e.g., non-immediate operands), leading to operations applied with a stale or default mask. Verify that the flag and value are always set together. |
+
+---
+
+## Built-in Subroutines (BuiltInSubroutines.cs)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Match the cc65 reference** | Every subroutine should produce byte-identical output to the cc65 reference implementation where possible. Use `python scripts/compare_rom.py` to verify. |
+| **Block naming** | Block names must match the label names used in `EmitWithLabel` calls. `nameof(_exit)` produces `"_exit"` — make sure the C# field/method name matches the assembly label convention. |
+| **Emit chain ordering** | `.Emit()` calls are chained and produce bytes in order. A misplaced `.Emit()` shifts all subsequent bytes, breaking the entire subroutine. |
+| **Label references within blocks** | Internal labels (like `"@1"`, `"@2"`) are local to a block. Branch targets referencing these labels must have correct offsets. |
+
+---
+
+## ROM Layout (Program6502.cs / NESWriter.cs)
+
+| Check | What to look for |
+|-------|-----------------|
+| **Block ordering matters** | The order blocks are added to `Program6502` determines their position in the ROM. Music subroutines go before `main()` to match cc65 layout. Reordering blocks shifts all addresses downstream. |
+| **iNES header correctness** | The 16-byte iNES header must correctly encode mapper number, PRG/CHR bank counts, mirroring mode, and battery flag. A wrong header means the emulator misinterprets the ROM format. |
+| **Interrupt vectors** | The last 6 bytes of PRG ROM ($FFFA-$FFFF) contain NMI, RESET, and IRQ vectors. These must point to the correct subroutine addresses. Missing or wrong vectors = the NES doesn't boot. |
+| **CHR ROM alignment** | CHR data must be exactly 8192 bytes per bank. Short or misaligned CHR data produces garbled graphics. |
+| **Address space boundaries** | NES PRG ROM maps to $8000-$FFFF (32KB for NROM). Subroutines that exceed this space won't fit. Watch for programs that grow too large and silently wrap or truncate. |