review-pr

Files (1)

• SKILL.md

SKILL.md (first lines)

# Deciding on a PR or an issue, Sean's way

Sean maintains waku-agent solo while a community sends PRs against the
`good first issue` list. He decides fast, without reading diffs, and he is a
product manager with a technical background — he wants the decision framed for
him AND he wants to be able to see it with his own eyes.

## The hard rule, before anything else

**NEVER approve, merge, or close a PR without Sean's explicit go-ahead for
THAT specific PR.** Not "I reviewed it and it's good" — an actual yes from him,
per PR, after he has seen the four sections.

Approving a *plan* that says "drain the queue" is NOT approval to merge. Neither
is "go", "sure", or "continue" on a broader task. This was violated on
2026-07-26: six PRs were merged off a plan approval, and Sean had tested none of
them himself. His words: *"i have not even tested them myself so i am concerned."*

Post review comments and change requests freely. Merging is his call.

## Frontend and TUI changes are HIS to test

If the diff touches the dashboard frontend (anything under `waku/ops/static/` —
HTML, CSS, JS) or the TUI/CLI experience, **never recommend merge on your own
testing.** Your screenshots, a clean console and a green suite are not enough
evidence for these. Stand the PR up on port 7778 so the live 7777 is untouched,
then hand him the URL and say which page to open and what to look for. The
fourth section becomes "click this, then tell me to merge."

The dashboard is what he films and what a new user meets first; a layout or copy
regression that reads fine to you can be obviously wrong to him. Backend-only
changes he is happy to take on your testing.

## One item at a time

Never batch. One PR or issue, four sections, then **stop and wait** for his
call. Name people and PRs in words ("@wilenWang's Connections registry"), not
bare numbers — he has said plainly that a wall of numbered items is unreadable:
*"my human brain doesn't have a long context."*

## Output format — exactly these four sections, in this order

```
# PR #<N> — @<author>            (or:  # Issue #<N> — @<author>)

### What this is
Plain language. What the change or report actually is, in a few lines. No diff,
no jargon dump. If it touches a mechanism, say what that mechanism does first.

### Why this is important
What breaks, who is affected, what it unblocks. Prefer the concrete failure over
the abstract benefit: "memory silently stopped growing, every turn, for every
Kimi user" beats "improves robustness". If it matters little, say that too.

### How do I test this
Copy-paste commands he can run himself, in fenced bash blocks, one command per
block. Say what you already ran and what you got, then give him the same command
so he can confirm it. Include the ONE test that would actually catch a
regression, not just the green suite. Dashboard PRs run on a second port so 7777
is untouched:
    WAKU_DASHBOARD_PORT=7778 .venv/bin/python -m waku.ops.dashboard
Say what to look for — the specific visual difference. Offer to drive it with
him rather than handing him a wall of steps.

### Are we ready to merge / reply / close — and why
One recommendation with its reason. Not a menu of options. Merge as-is / merge
with a follow-up we push ourselves / ask for a change / close warmly. Name what
you did NOT verify. End by asking for his call on that one item.
```

English by default. Write the 中文 version only when he asks for it (he repeats
some decisions on camera) — and when he does, write it the way he would say it,
not as a literal translation.

## Non-negotiables

1. **Test before you judge.** Never review from the diff alone. Check the PR out
   in an isolated **git worktree** (never `gh pr checkout` — his dashboard runs
   from the main working tree and a branch switch swaps code under a live demo).
   See the `pr-worktree` skill for setup and teardown, including the
   `set_key`-replaces-your-symlinked-`.env` trap that once left a full copy of
   every API key in `~/Developer/waku-prs/pr18/`.

   Note: linking `.waku` into the worktree makes
   `test_runtime_data_is_ignored.py` fail six ways — the gitignore pattern is
   directory-only and does not match a symlink. That failure is your setup, not
   the PR. Skip the link for a test-only pass.

2. **Prove the regression test bites.** If a PR claims a regression test, revert
   the source change inside the worktree and re-run it. A test that passes on
   the unfixed code is worth nothing, and this is not hypothetical: PR #74's
   test passed on broken code, because both paths returned 0 and the broad
   `except` made them indistinguishable from outside.
   ```bash
   git show main:<the/changed/file.py> > <the/changed/file.py>
   .venv/bin/python -m pytest <its/test/file.py> -q
   git checkout <the/changed/file.py>
   ```
   When it doesn't bite, say so plainly, and prefer pushing the real assertion
   ourselves as a follow-up over making a good contributor round-trip.

3. **Test it, don't just read it.** PR #14 looked correct in review and crashed
   on every message (`asyncio.to_thread` moved `respond()` off the thread that
   owns the SQLite connection). Reviewing from the diff would have merged it.

4. **Run it against real data**, not just the PR's own fixtures — that's how the
   PR #13 bug surfaced (hardcoded `~/.waku/traces`; the real home is `.waku`
   relative to cwd via `load_settings()`).

5. **Check it against the repo's rules** (AGENTS.md and docs/context/conventions.md): stdlib + anthropic/openai
   only — new deps must sit behind an optional extra; tests land in
   `evals/deterministic/`; module + test docstrings in the teaching voice; no
   emojis in any UI surface; the arena must never touch real agent state; and
   the footprint ladder — every registered tool ships in every prompt.

6. **Be honest about depth.** If you ran the tests but did not line-by-line
   audit a 600-line adapter, say so in the fourth section.

## For issues, not PRs

From gxiaopang/ContractGuard-Agent, MIT.

More from gxiaopang/ContractGuard-Agent