pre-pr-self-review

Stars6

Forks1

UpdatedJuly 10, 2026 at 16:30

Self-review checklist - plus an agentic review pass (a read-only reviewer persona over the diff) - before opening a PR. Use before invoking `create-pr`, when reviewing your own draft, or when a reviewer flags issues that should have been caught earlier. Codifies recurring mistakes from multi-targeted polyfill work - missing tests for new public surface, unchecked length sums, null-pointer foot-guns from `MemoryMarshal.GetReference` on empty spans, drift from `ArgumentNullException.ThrowIfNull` and `checked()` conventions, TFM phrasing errors, and stale PR descriptions.

Installation

Install with Codex or Claude Copy this prompt, paste it into Codex, Claude, or another assistant, and let it review the skill page and install it for you.

Source

JeremyKuhne

JeremyKuhne/touki

View GitHub Repository View Creator Repositories

Download

Related occupationsSOC

Based on SOC occupation classification

Software Quality Assurance Analysts and TestersComputer and Mathematical Occupations·SOC 15-1253

File Explorer

3 files

SKILL.md

readonly

More from this repository

same repository

filtrace

JeremyKuhne/touki

Analyze .NET CPU, allocation, exception, GC, JIT, and wall-clock (thread-time) data in .nettrace, .etl, and speedscope files with the filtrace CLI or MCP server. Use when a user asks where time or allocation volume goes in a trace or benchmark, which method or source line is hot, why a run regressed against a baseline, what a captured .nettrace / .etl contains, or to rank / drill / diff / export a profile - including profiling .NET Framework (net481) via ETW, where an EventPipe ranking would mislead. Also covers capturing the trace first - choosing EventPipe vs ETW, elevation, and the recording tool (dotnet-trace, BenchmarkDotNet, PerfView, wpr).

2026-07-166

performance-testing

JeremyKuhne/touki

Author and run BenchmarkDotNet performance tests in a multi-targeted .NET library's perf project, and translate a user's outcome-shaped performance question into a measurement. Use when adding new benchmarks, running existing ones, comparing implementations, profiling to find which method or source line dominates, evaluating allocations / memory usage, reading the generated code (sharplab / DisassemblyDiagnoser / HardwareCounters), or when a user asks how long something takes, how much memory it uses, where time is spent, or to help make a method faster - which this skill turns into a scenario, a benchmark, and a drill-down.

2026-07-166

agent-files-review

JeremyKuhne/touki

Review changes to AI-agent customization files (AGENTS.md, .github/copilot-instructions.md, *.instructions.md, *.prompt.md, *.agent.md, SKILL.md, the validator, the CI workflow). Use when asked to review or validate agent-file changes, fix CI failures from the agent-files workflow, or audit a draft of any of these files.

2026-07-156

address-pr-feedback

JeremyKuhne/touki

Address feedback on an existing pull request - review comments, requested changes, CI failures, or any post-PR follow-up work. Use when the user says "address the review", "fix the comments", "address Copilot's feedback", "fix the CI failure", or any similar phrasing. Distinct from `create-pr`, which covers opening the *initial* PR.

2026-07-106

create-pr

JeremyKuhne/touki

Create a pull request for the current changes. Use when asked to "make a PR", "open a pull request", "push and PR", or otherwise publish in-progress work for review. Ensures changes are on a non-`main` branch, commits are made and pushed, and the PR targets `upstream/main` when an `upstream` remote exists, otherwise `origin/main`.

2026-07-106

code-comprehension

JeremyKuhne/touki

Evidence-based readability and cognitive-load review of code. Use when asked to "review this for readability", "is this too complex", "reduce nesting or cognitive load", "what is a reasonable method length / parameter count / nesting depth", or when judging whether code will be hard to understand. Screens code against research-backed thresholds and prioritizes the factors that actually drive comprehension - naming first, then structure that loads working memory.

2026-07-106

compatibility	Requires git plus the repository's build, test, and agent-file validation commands.
description	Self-review checklist - plus an agentic review pass (a read-only reviewer persona over the diff) - before opening a PR. Use before invoking `create-pr`, when reviewing your own draft, or when a reviewer flags issues that should have been caught earlier. Codifies recurring mistakes from multi-targeted polyfill work - missing tests for new public surface, unchecked length sums, null-pointer foot-guns from `MemoryMarshal.GetReference` on empty spans, drift from `ArgumentNullException.ThrowIfNull` and `checked()` conventions, TFM phrasing errors, and stale PR descriptions.
license	MIT
metadata	{"applicability":"universal","binding":"optional-overlay","github-path":"skills/pre-pr-self-review","github-pinned":"v0.10.0","github-ref":"refs/tags/v0.10.0","github-repo":"https://github.com/JeremyKuhne/agent-skills","github-tree-sha":"880e117910c69bb909f59a3dfea81cbbf33ac047","maturity":"canary","portability":"portable","related":"create-pr, address-pr-feedback, security-review, performance-testing","requires":"none","risk":"local-write"}
name	pre-pr-self-review

Pre-PR self-review

If overlay.md exists beside this file, read it before acting; it contains repository-specific bindings. This core remains usable without it.

Run this checklist before invoking the create-pr skill. Each item is a question your code or PR body must answer. Update the skill whenever a reviewer flags something not yet listed.

This skill pairs with several others a consuming repo wires concretely in its overlay: a polyfill-dotnet-api skill (the source-preference and design rules this checklist validates), create-pr (the workflow this precedes), address-pr-feedback (the follow-up that re-runs this checklist), performance-testing (benchmark authoring required when a perf claim drives a change), framework-jit-optimization (net481 RyuJIT tradeoffs cited in the polyfill-correctness items), agent-files-review (for changes under .agents/, AGENTS.md, or .github/copilot-instructions.md), and security-review (the security-specific subset - abusive-input handling, length / integer overflow, allocation and algorithmic DoS, argument validation, and every use of unsafe / Unsafe.* / MemoryMarshal.* / Marshal.* or any BCL API whose docs say "unsafe" or "caller must"). Invoke security-review alongside this checklist for any change that adds or modifies a member accepting caller-supplied data, or that touches one of those caller-validated constructs - the common case, not a niche.

Agentic review pass

Before walking the checklist, run an automated review pass over the diff so the reviewer-bot class of findings - correctness edge cases, hand-rolled parser/format pitfalls, CI and supply-chain hygiene, and doc-vs-behavior drift - surfaces locally instead of across PR rounds. Spawn a read-only pre-PR reviewer persona (a consuming repo wires the concrete agent in its overlay) over the working diff, triage its findings (valid / nit / judgment call / likely false positive), fix the valid ones, and re-run at most once when the fixes were non-trivial. Keep it bounded: a same-class model nitpicks indefinitely, so two passes is the cap and the deterministic gates - tests, lint, the format validators - stay the source of truth. The checklist below is the human complement: the recurring, domain-specific mistakes an agent pass tends to miss.

1. Tests cover every new branch

For each new public (or InternalsVisibleTo-internal) member:

Search the test projects for the symbol; no hits = missing test.
Polyfills in the Framework-only tree: tests run on both TFMs. Wrap polyfill-only paths (subclass fallbacks, null-receiver guards) in #if NETFRAMEWORK.
Runtime type-check fast paths (typeof(T) == obj.GetType()): test the fast path and a subclass override.
Generic primitive specializations: every specialized branch needs a test. Don't rely on byte/int covering bool/sbyte/short/ ushort/uint/long/ulong - each has independent ref reinterpretation.
Security-sensitive APIs (FixedTimeEquals, hex decode): cover equal, differing-content, length mismatch, both empty, one empty, and a long span where only the last byte differs.
Allocating APIs (Concat, ToHexString): include an OverflowException test on the length sum.

Test hygiene for the tests themselves - the recurring miss list that costs the most review rounds on coverage-only PRs:

Test method names start with the method under test. MethodName_StateUnderTest_ExpectedBehavior per the repo's test conventions. ReadOnlySpan_Empty_ReturnsEmpty is wrong; SliceAtNull_ReadOnlySpan_Empty_ReturnsEmpty is right.
Every IDisposable test local uses using or try/finally. A temp-folder helper, a matcher handle, a pooled-list rental - a bare local leaks the resource when an assertion fails. Use the try/finally pattern when the test itself exercises explicit Dispose().
Don't hard-code InvariantCulture for APIs that use CurrentCulture. Provider-less formatting helpers generally format with CurrentCulture. Asserting against InvariantCulture-formatted strings makes the test locale-dependent.

2. Polyfill / framework correctness

For any change in the Framework-only tree (a polyfill or a framework-only fast path), walk these items (a consuming repo may keep the per-item detail and code patterns in a polyfill-correctness overlay companion):

Empty / null spans handled before unsafe interop (empty source, empty destination, both empty; exception type cross-checked).
Multi-input length sums wrapped in checked().
Throw helpers use the standard BCL exceptions, not custom types.
Span overloads stay allocation-free by default (document any trade-off in <remarks>).
Behavior parity with the modern BCL (exception type and message family, edge cases, type-exact fast paths).
Performance claims name the JIT (net481 RyuJIT vs modern .NET RyuJIT) and are measured or explicitly marked unmeasured.

If the change is not in the Framework-only tree, skip to §3.

3. PR description matches reality

TFM phrasing: name the polyfill's target TFM (the framework target, e.g. net472) distinctly from the TFM the tests merely run on (e.g. net481). Do not call a net472-targeted polyfill "net481-only".
File list, test counts, and perf numbers all reflect the current diff. Re-run after every commit; do not paste numbers from an earlier iteration.
Walk each bullet of the description against the diff before pushing. If the body says "covers Foo with cases A/B/C", search the diff for tests named Foo_… and confirm A, B, and C are all there. Review rounds have been lost to descriptions claiming a case (a Span<char> "null at end", a double-dispose test) that was not actually in the diff.
"Deliberately deferred" entries match what's actually absent from the working tree.

4. Final audit before staging

git status --short - delete leftover probe / scratch files; confirm every listed file belongs in the change set.
git diff --check - whitespace.
Rebase onto the canonical main if the branch trails it. Use upstream/main when working from a fork (the canonical repo lives at upstream), origin/main when cloning the canonical repo directly. Recently-merged sister PRs may have introduced files this PR cross-references; running off a stale base point makes those links look broken to an offline link check that gates .agents/**, AGENTS.md, and *.instructions.md changes. A PR once lost a review round to exactly this.
For changes that touch .agents/, AGENTS.md, or .github/copilot-instructions.md, also run the repo's agent-file link checker (see the agent-files-review skill for options, including changed-only and base-ref modes).
Build both TFMs.
Run the test suite in both Debug and Release. Release-mode RyuJIT inlining surfaces bugs Debug doesn't - e.g. [AggressiveInlining] + Unsafe.As<T, byte>(ref param) propagates the caller's int-promoted argument into the comparison immediate (cmp ecx, 0xFFFFFFFF instead of cmp ecx, 0xFF) for negative signed-primitive inputs on net481, but only in Release. Mask explicitly with & 0xFF / & 0xFFFF. See the polyfill-dotnet-api and framework-jit-optimization skills.
Stage by path, never git add -A / git add . when the working tree spans more than one logical change. If topics are intermingled, ask before staging.

5. Failing CI is a stop, not a sprint

When a build / test / CI run fails on a PR:

Diagnose.
Prepare the fix in the working tree.
Describe what changed, why, and any risks.
Stop and wait for explicit approval before commit/push.

Stacked rapid-fire fix commits are how perf regressions and unrelated sweep-ups get into history.

6. Update this skill

If a reviewer flags something not in this checklist, add it. If the review touched .agents/ files, also update via the agent-files-review workflow so the validator and any CI mirror stay in sync.