# Pre Pr Self Review

> 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-gu…

- **Type:** Skill
- **Install:** `agentstack add skill-jeremykuhne-agent-skills-pre-pr-self-review`
- **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior
- **Seller:** [JeremyKuhne](https://agentstack.voostack.com/s/jeremykuhne)
- **Installs:** 0
- **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills)
- **Latest version:** 0.1.0
- **License:** MIT
- **Upstream author:** [JeremyKuhne](https://github.com/JeremyKuhne)
- **Source:** https://github.com/JeremyKuhne/agent-skills/tree/main/skills/pre-pr-self-review

## Install

```sh
agentstack add skill-jeremykuhne-agent-skills-pre-pr-self-review
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

# 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 ``).
- 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 section 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` "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(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:

1. Diagnose.
2. Prepare the fix in the working tree.
3. Describe what changed, why, and any risks.
4. **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.

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [JeremyKuhne](https://github.com/JeremyKuhne)
- **Source:** [JeremyKuhne/agent-skills](https://github.com/JeremyKuhne/agent-skills)
- **License:** MIT

Install and usage instructions live in the source repository linked above.

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** no
- **Filesystem access:** no
- **Shell / process execution:** no
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: passed — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-jeremykuhne-agent-skills-pre-pr-self-review
- Seller: https://agentstack.voostack.com/s/jeremykuhne
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.