Skip to content

feat: Add claims check and opportunistic head-vs-base probes to github-pr-review #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jimcadden wants to merge 1 commit into from
Closed

feat: Add claims check and opportunistic head-vs-base probes to github-pr-review #25

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 102 additions & 1 deletion skills/github-pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,96 @@ This check catches a class of bugs where a new code path was written against a p
data layout that differs from the actual deployment (e.g. design doc says key X lives in
CM-A, but existing deployments have it in CM-B).

### 3.11 Claims Check

Always check for PRs that change code behavior:

| Check | What |
|-------|------|
| Claims listed | List every fix or behavior the PR's title, body, commit messages and new test names say it delivers |
| Target case | Each claim holds in the **exact** case it targets, not only a nearby easy case |
| Sibling variants | The same claim holds for variants of that case (other units, locales, transports, API shapes) |
| Test proves claim | The new test fails without the fix **and** asserts what the user sees (e.g. rendered output), not only an intermediate value |

A claim that does not hold in its target case is **must-fix** (see §4.1): the
user-visible behavior the PR says is fixed is still wrong, and merging would
ship a false "fixed" plus a test that passes over the bug. This holds even when
`main` was equally broken — severity comes from what the user sees, not from a
comparison with `main`.

### 3.12 Behavioral Probes (head vs base)

**Concrete-case rule:** every finding — from this section, §3.11, or any other
check — needs a concrete case: "this input or situation leads to this wrong
result." No case, no finding. Reading the diff is not evidence; if a claim can
be checked by running the code, run it.

For PRs that change parsing, decoding, formatting, arithmetic or other
input-handling code, probe head against base **when the tooling is already
present** in the environment (an interpreter, test runner, or build tool
already on `PATH`). Probes are opportunistic, never mandatory:

- Use only tooling that is already installed. **Never install, download, or
substitute tools to make a probe run.** Fetching a toolchain mid-review to
satisfy a probe requirement is reward hacking; a probe that only runs after
the reviewer acquires new tooling is not trustworthy evidence.
- If the toolchain the repo needs is absent (e.g. no `go` on `PATH` for a Go
PR), name it in the summary's **Not checked** list (§4.2) and fall back to
§3.11's claims check plus the concrete-case rule applied to the code as
written.

When tooling is present, build the PR head and its merge base and run the same
probe inputs against both:

```bash
# Precondition: $LOG_DIR is set (see Context-Safe Execution) and points outside
# the repo or into a git-ignored path — never into tracked files.
BASE_SHA=$(gh pr view <number> --json baseRefOid --jq .baseRefOid)
HEAD_SHA=$(gh pr view <number> --json headRefOid --jq .headRefOid)
git fetch origin "$BASE_SHA" "$HEAD_SHA"
git worktree add --detach "$LOG_DIR/base" "$BASE_SHA"
git worktree add --detach "$LOG_DIR/head" "$HEAD_SHA"
trap 'git worktree remove --force "$LOG_DIR/base" 2>/dev/null;
git worktree remove --force "$LOG_DIR/head" 2>/dev/null;
git worktree prune' EXIT
# Write one small probe (test or script), run it in both trees, diff the outputs.
```

The `trap` removes the worktrees and prunes their registrations even when a
probe fails — never leave detached checkouts behind.

Choose probes from what the diff touches:

| Diff touches | Probe with |
|--------------|-----------|
| A decoder reading new fields from client/upstream input | The field with a wrong JSON type (object for string, string for number), missing, `null` |
| Numeric formatting or math | Boundary values: `0`, negative, max/min of the type, clamped/saturated values |
| Fixed-width output (TUI, tables, logs) | The widest real value; assert display width ≤ column width |
| Error or fallback paths | The input that triggers the fallback, and confirm the outcome is visible |

Treat a behavior difference between base and head as a finding, stated as a
concrete case. If base accepted an input and head now rejects it, or one bad
element now fails a whole document, the user-visible behavior is wrong —
**must-fix** — even when the spec allows the stricter behavior, because
clients that don't follow the spec exist. Check the blast radius too: in
protocols that resend history (chat-style APIs), one rejected message can
break every later request in the conversation.

#### Worked examples (§3.11, §3.12)

Both come from reviews where the checklist passed but the behavior was wrong:

- **A claimed fix that does not hold (§3.11):** a PR adds a `+` marker to a
fixed-width table cell for clamped totals. The unit test checks the returned
string and passes, but the clamped value is wider than the column, so the
table truncates the cell and the `+` never reaches the screen. The probe:
render the real cell and assert its display width.
- **Input base handled, head rejects (§3.12):** a PR starts decoding
`tool_calls[].function.arguments` into a typed `string`. A client sending an
object makes the whole request fail to decode, where base, which never read
the field, parsed it fine. The probe: send one off-spec body against head
and base and diff the outcomes.

## Phase 4: Draft Review

Present proposed review to user for approval before posting.
Expand All @@ -392,7 +482,11 @@ Present proposed review to user for approval before posting.
```

Severity levels:
- **must-fix** - Blocks merge. Security issues, broken functionality, missing sign-off.
- **must-fix** - Blocks merge. The user-visible behavior is wrong: security
issues, broken functionality, missing sign-off, a claimed fix that does not
hold in its target case (§3.11), or input that base handled and head now
rejects or mishandles (§3.12). Severity comes from what the user sees;
"not a regression vs `main`" does not downgrade it.
- **suggestion** - Should fix but not blocking. Better patterns, missing tests.
- **nit** - Trivial. Style, naming, minor improvements.

Expand All @@ -409,8 +503,15 @@ Do NOT include praise comments — they clutter the review without adding value.
**Agent/IDE config (.claude/.vscode)**: none / FLAGGED (list paths)
**Commits**: N commits, all signed-off: yes/no
**CI status**: passing/failing/pending
**Not checked**:
- <probe or area skipped> — <why, naming the missing toolchain if that is the reason>
```

The **Not checked** list is auditable, not optional: name every probe or area
you did not run and why (e.g. `- head-vs-base probes — no Go toolchain on
PATH; §3.12 forbids installing one`). If nothing was skipped, write
`**Not checked**: none`.

Map `<association>` to its §1.6 trust tier so the line is self-explanatory:
`MEMBER`/`OWNER`/`COLLABORATOR` → **maintainer**, `CONTRIBUTOR` → **returning
external**, `FIRST_TIME_CONTRIBUTOR`/`FIRST_TIMER`/`NONE` → **first-time** (e.g.
Expand Down
Loading