AgentSkillsCN

review-pr

CTO 审查工作表纪律——发现问题、调整验证器、核实修复、跟踪合并后行动。可通过 /review-pr <number> 调用。

SKILL.md
--- frontmatter
name: review-pr
description: CTO review worksheet discipline — spot problems, tune validators, verify fixes, track post-merge actions. Invoke with /review-pr <number>.

Review PR — CTO Worksheet Discipline

Review a PR with Jon. Two goals simultaneously:

  1. Decide if this PR should merge — does it work, is it wired in, who needs to know
  2. Improve the pipeline — if a gap is found, fix the validator/worker templates so the gap doesn't recur

The PR is a test case for the pipeline, not just a merge candidate.

Invoke: /review-pr <number>

Setup

bash
PR_NUMBER=<number>
REPO_ROOT=$(git rev-parse --show-toplevel)
PR_INFO=$(gh pr view $PR_NUMBER --json title,body,files,labels,headRefName,number)
echo "$PR_INFO" | jq '{title, files: [.files[].path], labels: [.labels[].name]}'
ISSUE=$(echo "$PR_INFO" | jq -r '.body' | grep -oP 'Closes #\K\d+' | head -1)
echo "Issue: #$ISSUE"

Read the diff and the validator's verdict:

bash
gh pr diff $PR_NUMBER
gh issue view $ISSUE --json comments --jq '[.comments[] | select(.body | contains("READY") or contains("NOT READY") or contains("Automated Review"))] | .[-1].body'

Step 1: WWJA — What Would Jon Ask?

Now that you've seen the diff, anticipate Jon's questions:

  • Does it work? Not "the code looks right" — has anyone run it?
  • Is it wired in? Does anything call it? Do intended users know it exists?
  • Dead letter? No consumer, no downstream issue, requester not notified?
  • Current reality? Docs accurate? Paths correct? Agents aware of new tools?
  • Who needs to know? Requester, COS, affected agents?

These questions guide what to look for. Don't present the PR until you can answer them.

Step 2: Initialize the worksheet

This is a LIVING document — you fill it in as the review proceeds, including things Jon catches that you missed. Show it to Jon as you go, not just at the end.

code
**PR #NNN (issue #NNN) — TITLE — Worksheet**

| # | Finding (by whom) | Process gap? | Template fix | Re-run result |
|---|-------------------|-------------|-------------|---------------|
| | | | | |

| Step | Status | Detail |
|---|---|---|
| CTO verify | | |
| Jon's verify (link) | | |
| Approvable? | | |
| Post-merge actions | | |

"By whom" matters — Jon's catches and CTO's catches are both tracked. If Jon is catching things the CTO should have caught, that's a CTO improvement signal.

If the PR is clean: write "Clean — validator and CTO review found no gaps" and proceed to verification.

Step 3: For each finding → is it a process gap?

Not every problem is a validator gap:

  • Code bug (crash, wrong logic) → the PR needs fixing. Not a template issue.
  • Process gap (dead letter, missing browser test, untested credentials) → tune the template.

For process gaps:

  1. Which template?
    • Validator rules: $REPO_ROOT/.claude/ralph/VALIDATION_AGENT_PROMPT.md
    • Worker behavior: build_worker_prompt() in $REPO_ROOT/scripts/gh_listener.py
    • Worker template: $REPO_ROOT/.claude/ralph/CTO_ASSISTANT_TEMPLATE.md
  2. Edit it directly. Prompt edits are not code — don't file an issue.
  3. Re-run the validator:

Write the actual issue and PR numbers into the command — don't use shell variables inside Python strings (they won't substitute). Example for issue 882, PR 918:

bash
python3 -c "
import subprocess
proc = subprocess.Popen(
    ['claude', '--model', 'sonnet', '--print', '--dangerously-skip-permissions', '-p',
     'You are a Validation Agent for issue #882 in jonschull/ERA_Admin. '
     'Your instructions are in: /Users/admin/era2c/.claude/ralph/VALIDATION_AGENT_PROMPT.md '
     'Read that file FIRST. Follow its format exactly. '
     'PR to validate: #918. Issue: #882. '
     'Post your verdict clearly as READY or NOT READY. '
     'You are running in /Users/admin/era2c. Use absolute paths.'],
    cwd='/Users/admin/era2c', stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True,
    start_new_session=True)
stdout, stderr = proc.communicate(timeout=300)
verdict = 'NOT READY' if 'NOT READY' in stdout else 'READY' if 'READY' in stdout else 'UNKNOWN'
print(f'VERDICT: {verdict}')
print(stdout[-1500:])
"
  1. Did it catch the gap? If not, iterate on the template. If it can't be mechanized (e.g., "is this ambitious enough?"), note that and move on.
  2. Update the worksheet row.

Step 4: CTO independent verification

Don't trust the validator — verify yourself:

  • Scripts: Run them end-to-end
  • Web pages: B=~/.claude/skills/gstack/browse/dist/browse && $B goto <url> && $B snapshot -a -o /tmp/review.png — then Read the PNG
  • Agents: Launch in tmux, ask "who are you and what's your mission?" Check tool permissions match CLAUDE.md claims
  • CLI tools: grep intended users' CLAUDE.md for the tool name — if not found, dead letter
  • Config: Check actual system state (launchd, cron, pm2)

Step 5: Jon's verification

Give Jon something clickable — don't silently open things, don't give curl commands:

  • Web pages: file:///path/to/preview.html or the live URL
  • Screenshots: Read the PNG so it displays inline
  • Agents: which tmux session to attach to (tmux attach -t NAME)

Step 6: Fresh look before presenting

Before showing anything to Jon, re-read your own work. Then re-read it again. Each pass catches problems at a higher level of abstraction:

  • First pass: does the text/code work?
  • Second pass: does the design match how things actually work?
  • Third pass: does the system know this exists? Will anyone find it?

If you're writing something new (skill, doc, template), take at least 3 fresh looks. You stop looking too soon — the first draft feels complete because you've been thinking about the content, but you haven't thought about the context.

Step 7: Present and discuss

Show the worksheet as you go — this is a dialogue, not a presentation. Jon will spot things you missed. When he does:

  1. Add his finding to the worksheet (note "Jon" in the "by whom" column)
  2. Ask: is this a process gap? If yes, tune the template immediately
  3. Re-run if appropriate

When ready for a decision, say "Approve?" — never "Want me to merge?"

Step 8: On approval — post-merge

Do not dismiss the PR until ALL of these are done:

  1. Merge: gh pr merge $PR_NUMBER --squash --delete-branch
  2. Post the completed worksheet as a [CTO] Review Worksheet comment on the PR
  3. Post-merge checklist:
    • Who filed the issue? (gh issue view $ISSUE --json author --jq '.author.login') — email them if they're an agent (check for [COS] prefix)
    • Listener need updating? cd ~/era2c-listener && git fetch origin && git reset --hard origin/main
    • Deploys to Render? /deploy-check
    • Templates edited during review? Commit them in a separate PR
  4. Only then: done.

Handling interruptions

When interrupted (incoming mail, Jon's corrections):

  1. Read mail immediately — don't ask
  2. Act on corrections immediately — edit templates, save memories
  3. Return to the worksheet — it's your bookmark

Anti-patterns

  • Filing an issue for a prompt edit — just edit the prompt
  • "READY" without checking if users know the tool exists
  • Curl-testing a web page instead of browser-testing it
  • Presenting without WWJA
  • Dismissing without completing post-merge actions
  • "Want me to merge?" instead of presenting evidence
  • Assuming one problem per PR — keep looking after the first find
  • Filling the worksheet alone and presenting the finished product — it's a dialogue
  • Treating every finding as a validator gap — code bugs are just code bugs