Add review-loop and ship skills
This commit is contained in:
@@ -0,0 +1,62 @@
|
|||||||
|
---
|
||||||
|
name: review-loop
|
||||||
|
description: Get a pull request reviewed by Patterm reviewer agents, send their findings to whoever is fixing it, re-review with the same reviewers, and give Harry one traffic-light verdict. Use when Harry asks for reviews ("get fable and smartypants to review it", "traffic light review via patterm", "iterate until green"). Never merges.
|
||||||
|
---
|
||||||
|
|
||||||
|
# Review loop
|
||||||
|
|
||||||
|
Runs review rounds on one PR until it is GREEN or Harry has to decide something. It uses the `traffic-light-review` skill as the rubric.
|
||||||
|
|
||||||
|
## Start
|
||||||
|
|
||||||
|
1. Pin the head: record the PR URL, base branch and exact head SHA. Every review round names that SHA, so nobody reviews a moving branch.
|
||||||
|
2. Pick reviewers: use the ones Harry named. If he named none, use `harness_presets`, ask which to use, and wait for his answer. Don't add reviewers he didn't ask for.
|
||||||
|
3. Spawn each reviewer with `spawn_agent` and that preset. Tell them:
|
||||||
|
- the PR, base, head SHA and worktree path;
|
||||||
|
- read-only: no edits, commits, pushes, merges or PR comments;
|
||||||
|
- apply `traffic-light-review` and back every finding with a file and line;
|
||||||
|
- judge the PR against what it set out to do, and list ideas beyond that as follow-ups, not findings;
|
||||||
|
- `report_to_parent` with the verdict and findings.
|
||||||
|
4. Wait with `timer_fire_when_reported_all` on the reviewer IDs. Don't poll output.
|
||||||
|
5. Note the PR's diff size (files and lines changed). You'll compare against it later.
|
||||||
|
|
||||||
|
## Keep the PR the size it started
|
||||||
|
|
||||||
|
Reviewers always find more to suggest. A loop that acts on every suggestion turns a small PR into a large one that is harder to review and riskier to ship. So:
|
||||||
|
|
||||||
|
- Fix bugs, regressions and missing tests for what the PR changes. These are the point of the loop.
|
||||||
|
- Send anything else to follow-ups, such as refactors, new abstractions, extra options, or tidying code the PR didn't touch. A follow-up becomes a todo or a line in the verdict, not a change to this PR.
|
||||||
|
- Ask the implementer for the smallest fix that resolves each finding.
|
||||||
|
- If the diff has grown well beyond where it started (roughly half again), stop and tell Harry before going further. He may prefer to split the PR.
|
||||||
|
|
||||||
|
This doesn't mean ignoring real problems to keep the diff small. A correct fix that needs more code is fine. The test is whether a change is needed for this PR to be right.
|
||||||
|
|
||||||
|
## Each round
|
||||||
|
|
||||||
|
1. Read each report and check every finding against the code yourself. Drop the ones that are wrong, and say why. Move out-of-scope ones to follow-ups.
|
||||||
|
2. Write one verdict for Harry (format below).
|
||||||
|
3. GREEN: stop and report. Merging is Harry's call, or use `ship` if he has already authorised it.
|
||||||
|
4. RED or YELLOW with clear fixes: send the confirmed findings to the implementer, meaning whichever agent owns the branch, or yourself if you wrote it. If it has already reported, `send_message` it and wait for its report.
|
||||||
|
5. After the fixes, pin the new head SHA. Then `send_message` the same reviewers: give them the new SHA and the findings they raised, and ask them to check those fixes and anything new. Reuse the reviewers because they keep their context. Spawn a replacement only if a reviewer's process is gone. If that happens, give the replacement the earlier findings.
|
||||||
|
6. Stop and ask Harry when a fix needs a product or design decision, when reviewers disagree on something that matters, or after three rounds without reaching GREEN.
|
||||||
|
|
||||||
|
## Verdict format
|
||||||
|
|
||||||
|
```
|
||||||
|
<GREEN|YELLOW|RED> — PR #<n> @ <short SHA>
|
||||||
|
Reviewers: <name: verdict>, …
|
||||||
|
Must fix: <finding, file:line> (or "none")
|
||||||
|
Should fix: …
|
||||||
|
Dismissed: <finding — why>
|
||||||
|
Follow-ups: <out-of-scope suggestion> (not in this PR)
|
||||||
|
Diff: <files/lines now> (started at <files/lines>)
|
||||||
|
Needs you: <decision> (only if there is one)
|
||||||
|
```
|
||||||
|
|
||||||
|
Keep it short. Harry wants the answer, not the transcripts.
|
||||||
|
|
||||||
|
## Boundaries
|
||||||
|
|
||||||
|
- Never merge, approve or close the PR.
|
||||||
|
- Reviewers never write code. Only the implementer does.
|
||||||
|
- Leave the reviewers open until the loop ends, then close them.
|
||||||
@@ -0,0 +1,43 @@
|
|||||||
|
---
|
||||||
|
name: ship
|
||||||
|
description: Merge a reviewed PR once Harry has explicitly said to, update the local main checkout, clean up what the task created, and watch the rollout if he asks. Use for "merge it", "ship it", "merged, monitor the rollout", "clean up and get main up to date".
|
||||||
|
---
|
||||||
|
|
||||||
|
# Ship
|
||||||
|
|
||||||
|
Merging, building and deploying are separate steps. Do only the ones Harry asked for, and say which ones happened.
|
||||||
|
|
||||||
|
## 1. Merge (only on explicit authorisation)
|
||||||
|
|
||||||
|
Merge only when Harry has said "merge it", "ship it" or similar about this PR in this conversation. A conditional instruction such as "merge it if it's green" counts once the condition is met. A GREEN review on its own does not. If he says he merged it himself, skip to step 2.
|
||||||
|
|
||||||
|
Before merging, check:
|
||||||
|
- the PR head is the SHA that was reviewed. If commits have landed since, tell Harry and stop;
|
||||||
|
- the latest review is GREEN, or Harry accepted the remaining findings;
|
||||||
|
- CI passes and the PR is mergeable.
|
||||||
|
|
||||||
|
If any check fails, report it and stop. Merge with the remote's own tool (`gh`, `tea`, or the provider MCP) and follow the repo's merge style. If the repo's AGENTS.md or memory describes how merges work there, follow it.
|
||||||
|
|
||||||
|
## 2. Update main
|
||||||
|
|
||||||
|
In the main checkout, fetch and fast-forward main (`git merge --ff-only`). If that would not fast-forward, or the checkout has local changes, stop and say so. If the lockfile changed, reinstall dependencies the way the repo does.
|
||||||
|
|
||||||
|
## 3. Clean up what this task created
|
||||||
|
|
||||||
|
Remove only things this task owns:
|
||||||
|
- the worktree, if no other session is linked to it and it has no uncommitted or unpushed work;
|
||||||
|
- the local branch, once its commits are in main;
|
||||||
|
- the PR's reviewer and implementer agents and dev servers, once they have reported.
|
||||||
|
|
||||||
|
Anything else, such as other agents' worktrees or older branches, needs a list for Harry, not removal.
|
||||||
|
|
||||||
|
## 4. Watch the rollout (only if asked)
|
||||||
|
|
||||||
|
A merge is not a deploy. Find out how this repo deploys, from AGENTS.md, CI config or memory. Watch that deploy until it finishes, then check the signals that matter for the change, such as errors, job success and the metric it was meant to move. Use read-only access.
|
||||||
|
|
||||||
|
Report: deployed or not, at which SHA, what you checked, and anything that looks wrong. If something looks wrong, report it and suggest a fix or revert. Don't act on it yourself.
|
||||||
|
|
||||||
|
## Boundaries
|
||||||
|
|
||||||
|
- No production writes, migrations, backfills, canaries, flag or percentage changes, or config changes unless Harry approves that exact action.
|
||||||
|
- Don't deploy or restart live services as part of shipping, unless Harry asks for it separately.
|
||||||
Reference in New Issue
Block a user