pr-auditor

Files (1)

• SKILL.md

SKILL.md (first lines)

# PR auditor

Gate a merge the way a skeptical staff engineer would: decide whether this branch is safe to merge, and justify the verdict from the code's actual behavior.

An AI coding agent wrote this branch.
So the pull request description, the commit messages, the code comments, the test names, and the green checks are **claims** rather than facts, each one asserted by the agent about its own work.
Confirm or refute every claim against the code.

Three rules hold for the whole run.

* **Blunt.** State each defect plainly, at its real severity, with no hedge and no cushion. Praise only what is technically load-bearing. A softened finding is a finding nobody fixes.
* **Reproduce.** Prefer reproducing a defect over reasoning about it: run the test, run the script, revert the hunk, grep for the caller. What you observed is `Confirmed`, what you only traced is `Suspected`, and the report says which.
* **Report only.** Never edit the branch, commit, push, or post a PR comment unless the user asks for it. Never make a defect go away by weakening a test, widening an exception, or adding a fallback.


## Step 1: Establish the target and the intended behavior

Resolve four inputs: the branch or pull request under audit, the base branch, the linked issue or spec, and any extra context the user gave.
Default to the current branch against `main`.
Name whatever you assumed and proceed; do not stall on a missing input.

```bash
PR=123 # the pull request number under audit

gh auth status
gh pr view "$PR" --json title,body,baseRefName,headRefName,files,commits
gh pr diff "$PR"
```

Fall back to `git log --oneline origin/main..HEAD` and `git diff origin/main...HEAD` when there is no pull request, or when `gh` is unavailable or unauthenticated.
With no shell at all, treat the diff presented to you as complete, and record in the report that every command gate below became an inspection.

The description says what the agent believes it built; the issue or spec says what was asked for.
Where the two disagree is the first place a plausible implementation hides.

Done when you can state the intended behavior and the change actually made in one sentence each, and have named every input you assumed.


## Step 2: Read past the diff

A diff shows what changed and never whether the change is right.
For every symbol the diff touches, read four things outside the changed hunks.

* **Callers.** `grep -rn '<symbol>' --exclude-dir=node_modules .` for each renamed, retyped, or removed symbol, so a signature change cannot silently strand one.
* **The whole module,** not the hunk, because the invariant a change breaks usually lives above or below it.
* **The nearest existing implementation of the same kind of thing,** which is the convention this change is supposed to match.
* **The tests that cover it,** and what they actually assert.

Done when every changed symbol has its callers enumerated, and you can name the repository convention each changed file is supposed to follow.


## Step 3: Run the gates

An unrun gate is not a pass, and a passing gate is not a correct change.
Run each command from the repository root, or record the gate as unverified.

| Gate                   | Catches                                                                                                               | Note                                                                          |
| ---------------------- | --------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------- |
| `pnpm check-content`   | A content page missing its EN or JA counterpart, or missing the `title`, `description`, or `localization` frontmatter | The bilingual pairing gate; run it after any change under `contents/`         |
| `pnpm lint-check`      | A Prettier or markdownlint violation, reported without writing fixes                                                  | Iterate on one file or folder with `pnpm lint-target <path>`                  |
| `pnpm lint-naming`     | A file or folder name that breaks the naming rules (kebab-case, `notes/` date prefix, `.yaml` not `.yml`)             | Runs `skills/file-folder-name-linter/scripts/lint-names.mjs`                  |
| `pnpm test`            | Everything above, plus a stale `docs/contents-structure.md`, a VitePress build break, and a sitemap defect            | The composite gate; reach for it when the diff is wide                        |
| `node <script> --help` | A helper script whose usage output drifted from its flags                                                             | Also confirms the notes section and version history that `AGENTS.md` requires |

`pnpm lint` and `pnpm tree` write files, so run them only on a clean tree, read the resulting diff as the finding, and restore the tree with `git restore .` afterward.
There is no type checker and no unit test suite in this repository: the scripts are plain Node.js ES modules, so the composite `pnpm test` run and the `--help` runs are the closest behavioral gates.

A failing gate is a finding only when this branch caused it, so reproduce it on the base ref before reporting it.

```bash
git status --porcelain # must print nothing before you switch refs
git switch --detach origin/main
pnpm test # or whichever gate failed
git switch -
```

A failure that reproduces on the base is pre-existing; say so, and keep it out of the verdict.

Done when every gate above has either run or been recorded as unverified, and every failure has been classified as introduced or pre-existing.


## Step 4: Audit the change

Two passes: the lenses, then the failure modes that plausible code hides behind.


### The lenses

| Lens              | Look for                                                                                                                             |
| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------ |
| Correctness       | A misread requirement, a logic error, an invalid assumption, an unhandled edge case, an off-by-one                                   |
| Regression        | Behavior an existing caller depends on, changed or removed, and a breaking change to a public interface or a file another tool reads |
| State and timing  | A race, a stale cache, an unawaited promise, an order dependence between steps                                                       |
| Failure paths     | Error handling, recovery, resource cleanup, and what happens when an external call fails or returns garbage                          |
| Types and APIs    | A type-safety hole, and a library or platform API used against its documented contract                                               |
| Security and data | Unsafe handling of user input or external data, an exposed secret, and an authentication or authorization mistake                    |
| Performance       | A new pass over a large tree, a filesystem or network call inside a loop, work moved onto a hot path                                 |
| Reach             | Accessibility, internationalization, and every platform and browser the project supports                                             |
| Maintainability   | Duplicated logic, dead code, a speculative abstraction, naming that fights the repository's terminology                              |
| Scope             | Any change the stated goal does not require                                                                                          |
| Tests             | Coverage that is missing, weak, misleading, or coupled to the implementation instead of the behavior                                 |
| Docs              | A comment, `README.md`, `AGENTS.md` section, or skill file that no longer matches the code                                           |


### Where plausible code fails

**Plausible** is the failure mode, not the standard.
An AI agent optimizes for code that reads as correct, so the defects it leaves are the ones that survive a skim.
Hunt each of these by name.

* Code that satisfies the wording of the task and not the requirement behind it.
* An invented API, configuration field, flag, or library behavior. Check each against the installed source under `node_modules` or the vendor's documentation, never against how the name reads.
* A `try`/`catch`, `?.`, or `|| default` that turns a defect into a silent success.
* A second implementation of a utility the repository already has. Grep for the behavior before accepting a new helper.
* Validation the caller already performed, layered on again.
* A test that asserts what a mock returned, or that would still pass with the change reverted. Revert the hunk, rerun the test, and see whether it fails.
* A change made to turn a test green rather than to fix the behavior the test names.
* An abstraction, refactor, or rename the task did not require.
* A partial migration: the new path added, the old path left live, and callers split between the two.

From ahandsel/tokyo-geek, MIT.

More from ahandsel/tokyo-geek