From d9ef2df29db1c813f46afa572b0d492a7d22ed8f Mon Sep 17 00:00:00 2001 From: Peter Koomen Date: Fri, 24 Jul 2026 12:23:40 -0700 Subject: [PATCH 1/3] Add a merge skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands one PR: rebase onto latest main, review it adversarially with two fable subagents (defects, and AGENTS.md invariant conformance), triage and fix the findings, gate on `ci`, merge. Rebase rather than merge-from-main, so the tree fable reviews is the tree that lands. The driver writes the fixes and the reviewers run on fable — a model reviewing its own fixes is not an adversarial review. Findings are confirmed against real control flow before acting, since adversarial reviewers over-report. Three fix rounds, then it stops and reports. Co-Authored-By: Claude Opus 5 --- .claude/skills/merge/SKILL.md | 106 ++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 .claude/skills/merge/SKILL.md diff --git a/.claude/skills/merge/SKILL.md b/.claude/skills/merge/SKILL.md new file mode 100644 index 0000000..5220d63 --- /dev/null +++ b/.claude/skills/merge/SKILL.md @@ -0,0 +1,106 @@ +--- +name: merge +description: Land a PR — rebase onto latest main, review it adversarially with fable, fix every finding, then merge. Use when asked to merge a PR, land a branch, or ship a change. +--- + +# Merge + +Land one PR on `main`: rebase, review, fix, merge. Takes a PR number +(`/merge 28`); with no argument, use the PR for the current branch +(`gh pr view --json number`). + +## Ground rules + +- **Rebase, never merge-from-main.** The premise is that reviewed code is the code + that lands. A merge commit from `main` means fable reviewed a tree that never + existed. +- **`bun run ci` green is necessary and never sufficient.** Fable's verdict is the + other half of the gate. +- **Never weaken the gate to get a merge.** No skipped tests, loosened assertions, + new ignore rules, or edits to `.github/workflows/ci.yml`. If landing the PR + requires any of that, stop and say so. + +## Procedure + +### 1. Preflight and confirm + +Check `gh auth status`, and that the PR is open and not a draft. Show the user the +PR title, the diff stat, and what you're about to do, then get approval for the +**whole run** — after that, rebase, review, fix, and merge without asking again. + +Re-confirm only if something changes the deal: a conflict resolution you aren't +confident in, or the diff touching release/deploy machinery you didn't flag up front. + +### 2. Rebase onto latest `main` + +Work in a worktree, not the main checkout. Reuse the branch's existing worktree if +`git worktree list` shows one; otherwise +`git worktree add /Users/koomen/git/scratch/scratchwork-worktrees/ `. + +```bash +git fetch origin main +git rebase origin/main +git push --force-with-lease +``` + +Resolve only conflicts whose resolution is mechanical and whose intent on both sides +is unambiguous. Anything where you'd be choosing behavior rather than reconciling +text: `git rebase --abort` and hand it back to the user. + +This is the one failure the rest of the pipeline cannot catch — fable will review +whatever tree you produce and find a misresolution perfectly coherent. + +### 3. Adversarial review with fable + +Spawn **two** subagents with the Task tool, in parallel, both on `claude-fable-5`. +Give each the PR number and title, and `git diff origin/main...HEAD` as the target. + +You are the author of the fixes; they are the reviewers. Keep that split — a model +reviewing its own fixes is not an adversarial review. + +Instruct each to hunt for reasons this should **not** land — a reviewer that returns +"looks good to me" has not done its job — and to report findings as +`{file, line, severity: blocking|nit, claim, failure_scenario}`, empty only if it +genuinely finds nothing after a real search. + +- **Lens 1 — defects.** Correctness, security, regressions, error handling, + concurrency, data loss. Where does this break in production? What input makes it + wrong? +- **Lens 2 — invariant conformance.** Read `AGENTS.md` in full, then check the diff + against the *agent-pass* bullets of all six invariants (same job as the + `check-invariants` skill). CI already mechanizes the greppable cores; the + reviewer's value is the judgment residue. + +### 4. Triage, then fix + +Adversarial reviewers over-report — that's the cost of telling them to attack. +Confirm each finding against the actual code before acting: trace the failure +scenario through the real control flow, and check the premise still holds after the +rebase. Discard what doesn't survive and say what you discarded and why. Don't fix a +phantom to make a reviewer happy. + +Fix every surviving finding, `blocking` and `nit` alike. Then `bun run ci` (needs +Docker for the LocalStack e2e lane), commit, push, and re-run step 3 on the new diff. + +**At most 3 rounds.** If findings still survive, stop and report — a PR that can't +converge in three rounds has a problem this skill is the wrong tool for. + +### 5. Merge + +Wait for the real check on the pushed head — a local `bun run ci` is a prediction, +the workflow run is the fact. `ci` is a required check on `main`, so a red run can't +merge anyway. + +```bash +gh pr checks --watch +gh pr merge --merge --delete-branch +git worktree remove +``` + +Merge commits match this repo's history. + +## Report + +The PR number and title, rebase result, review rounds, findings fixed and findings +discarded (with why), and the final `main` SHA. If you stopped short of merging, say +exactly why and what the user needs to decide. From 458e3cffa6ba74b18769a6f8252ef292c50d3864 Mon Sep 17 00:00:00 2001 From: Peter Koomen Date: Fri, 24 Jul 2026 13:46:04 -0700 Subject: [PATCH 2/3] Fix procedural defects in the merge skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 adversarial review found the runbook would fail in practice. Fixed, in order of severity: - Pin the reviewed SHA and merge with `--match-head-commit`, so a commit pushed after the review cannot land unreviewed. Re-check that main has not moved before merging. - Reorder cleanup: a branch checked out in a worktree cannot be deleted, so `--delete-branch` failed on every run. Remove the worktree first, then delete the branch explicitly. - Fetch the PR branch, not just main; `git worktree add` needs the ref. - `bun install` in the worktree before `bun run ci` — a fresh worktree has no node_modules. - Give the reviewers the checkout directory. A subagent starts in the session cwd, so a reviewer that is not told where to look diffs the wrong tree and reports nothing. - Handle the branch already being checked out, including in the primary checkout, where `git worktree add` refuses outright. - Require the PR base to be main; rebasing a stacked PR onto main would rewrite it and split the review target from the merge destination. - Only remove a worktree this skill created; a reused one may be a long-lived checkout the user is sitting in. - Derive the worktree path from the repo root instead of hardcoding a home directory into a shared repo file. - Use the `fable` model alias; the Agent tool takes sonnet|opus|haiku|fable, not a full model ID. - Defer to the check-invariants skill instead of restating it, per the standing rule against duplication that can drift. - Note that `gh pr checks` errors rather than waits before a run registers. Co-Authored-By: Claude Opus 5 --- .claude/skills/merge/SKILL.md | 100 ++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 23 deletions(-) diff --git a/.claude/skills/merge/SKILL.md b/.claude/skills/merge/SKILL.md index 5220d63..f8317a8 100644 --- a/.claude/skills/merge/SKILL.md +++ b/.claude/skills/merge/SKILL.md @@ -24,23 +24,54 @@ Land one PR on `main`: rebase, review, fix, merge. Takes a PR number ### 1. Preflight and confirm -Check `gh auth status`, and that the PR is open and not a draft. Show the user the -PR title, the diff stat, and what you're about to do, then get approval for the -**whole run** — after that, rebase, review, fix, and merge without asking again. +```bash +gh auth status +gh pr view --json title,isDraft,state,baseRefName,headRefName +``` + +The PR must be open and not a draft. **`baseRefName` must be `main`** — this skill +rebases onto `main` and assumes the merge lands there. On a stacked PR based on +another branch, stop: rebasing it onto `main` would rewrite it to include or drop the +base branch's commits, and the review target, the rebase, and the merge destination +would all disagree. -Re-confirm only if something changes the deal: a conflict resolution you aren't -confident in, or the diff touching release/deploy machinery you didn't flag up front. +Show the user the title, the diff stat, and what you're about to do, then get +approval for the **whole run** — after that, rebase, review, fix, and merge without +asking again. Re-confirm only if something changes the deal: a conflict resolution +you aren't confident in, or the diff touching release/deploy machinery you didn't +flag up front. -### 2. Rebase onto latest `main` +### 2. Get a checkout, then rebase -Work in a worktree, not the main checkout. Reuse the branch's existing worktree if -`git worktree list` shows one; otherwise -`git worktree add /Users/koomen/git/scratch/scratchwork-worktrees/ `. +Find where the branch lives — **do not assume you can create a worktree for it.** +`git worktree add` fails if the branch is already checked out anywhere, including the +primary checkout. + +```bash +git fetch origin main # the branch too: worktree add needs the ref +git worktree list # is already checked out? +``` + +- **Already checked out** (any worktree, including the primary one): use that + directory, and **remember not to remove it in step 5** — it may be a long-lived + checkout, possibly the one the user is sitting in. It must be clean + (`git status --porcelain` empty) or the rebase won't start; if it's dirty, stop and + hand it back rather than stashing the user's work. +- **Not checked out:** create a scratch worktree beside the repo, and remember that + you created it: + + ```bash + ROOT=$(git rev-parse --show-toplevel) + WT="$(dirname "$ROOT")/scratchwork-worktrees/" + git worktree add "$WT" + ``` + +Everything below runs in that directory. Then: ```bash -git fetch origin main git rebase origin/main git push --force-with-lease +bun install --frozen-lockfile # a fresh worktree has no node_modules ``` Resolve only conflicts whose resolution is mechanical and whose intent on both sides @@ -52,8 +83,14 @@ whatever tree you produce and find a misresolution perfectly coherent. ### 3. Adversarial review with fable -Spawn **two** subagents with the Task tool, in parallel, both on `claude-fable-5`. -Give each the PR number and title, and `git diff origin/main...HEAD` as the target. +Spawn **two** subagents with the Agent tool, in parallel, both with `model: "fable"` +(the alias — the model parameter takes `sonnet|opus|haiku|fable`, not a full model +ID). + +Give each one **the checkout directory from step 2** — a subagent starts in the +session's cwd, not your working directory, so a reviewer that isn't told where to +look will diff the wrong tree and cheerfully report nothing. Also give it the PR +number and title, and `git diff origin/main...HEAD` as the target. You are the author of the fixes; they are the reviewers. Keep that split — a model reviewing its own fixes is not an adversarial review. @@ -66,10 +103,9 @@ genuinely finds nothing after a real search. - **Lens 1 — defects.** Correctness, security, regressions, error handling, concurrency, data loss. Where does this break in production? What input makes it wrong? -- **Lens 2 — invariant conformance.** Read `AGENTS.md` in full, then check the diff - against the *agent-pass* bullets of all six invariants (same job as the - `check-invariants` skill). CI already mechanizes the greppable cores; the - reviewer's value is the judgment residue. +- **Lens 2 — invariant conformance.** Run the `check-invariants` skill's procedure + against the diff — that skill is normative for this lens; follow it rather than a + restatement here, and if it and this file ever disagree, it wins. ### 4. Triage, then fix @@ -85,19 +121,37 @@ Docker for the LocalStack e2e lane), commit, push, and re-run step 3 on the new **At most 3 rounds.** If findings still survive, stop and report — a PR that can't converge in three rounds has a problem this skill is the wrong tool for. -### 5. Merge +### 5. Merge the tree you reviewed + +Pin the reviewed commit, and confirm `main` hasn't moved out from under it: + +```bash +REVIEWED=$(git rev-parse HEAD) +git fetch origin main +git rev-list --count HEAD..origin/main # non-zero → main moved; restart at step 2 +``` -Wait for the real check on the pushed head — a local `bun run ci` is a prediction, -the workflow run is the fact. `ci` is a required check on `main`, so a red run can't -merge anyway. +Wait for the real check on that head — a local `bun run ci` is a prediction, the +workflow run is the fact. `gh pr checks` errors rather than waits if GitHub hasn't +registered the run yet, so retry a few times before believing it. ```bash gh pr checks --watch -gh pr merge --merge --delete-branch -git worktree remove +gh pr merge --merge --match-head-commit "$REVIEWED" ``` -Merge commits match this repo's history. +`--match-head-commit` is what makes this honest: if anything landed on the branch +after the review, the merge is refused instead of silently shipping an unreviewed +tree. Merge commits match this repo's history. + +Then clean up, in this order — a branch checked out in a worktree cannot be deleted, +which is why `--delete-branch` is not used above: + +```bash +git worktree remove "$WT" # ONLY if you created it in step 2 +git branch -d +git push origin --delete +``` ## Report From 0863f96e8b8bf99c2914cce30c708e7b348bf684 Mon Sep 17 00:00:00 2001 From: Peter Koomen Date: Fri, 24 Jul 2026 13:51:41 -0700 Subject: [PATCH 3/3] Fix defects the round-1 fixes introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review found the worktree and cleanup machinery was generating most of the failure modes, several of them added by round 1. Cut the machinery rather than patching each bug. - Stop creating worktrees. `git worktree add` fails when the branch is checked out, `--show-toplevel` returns the linked worktree's own root rather than the primary one (so the path nested wrongly), and removing a worktree you are standing in breaks every later command. Use the checkout the branch already lives in. - Stop deleting local branches and worktrees. Deleting a checked-out branch fails, and the checkout may be one the user lives in. Delete the remote branch and report what local state remains. - Never carry shell variables across steps. Each Bash call is a fresh shell, so `$WT` and `$REVIEWED` were empty by the time they were used — which silently neutered `--match-head-commit`. Print and substitute literals instead. - Reconcile the local branch against origin/ before rebasing. Round 1 added a fetch of the branch, which makes `--force-with-lease` compare against the ref it just updated — so a stale local branch would force-push away remote-only commits and the lease would pass. - Resolve the two contradictions the check-invariants deference created: give lens 2 an explicit diff base and findings format that override that skill, so both lenses review the same tree and report the same shape. - Reject fork PRs at preflight rather than failing at the fetch. - Say one lens per reviewer, not both lenses to both. Co-Authored-By: Claude Opus 5 --- .claude/skills/merge/SKILL.md | 76 ++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 29 deletions(-) diff --git a/.claude/skills/merge/SKILL.md b/.claude/skills/merge/SKILL.md index f8317a8..e53af11 100644 --- a/.claude/skills/merge/SKILL.md +++ b/.claude/skills/merge/SKILL.md @@ -26,10 +26,12 @@ Land one PR on `main`: rebase, review, fix, merge. Takes a PR number ```bash gh auth status -gh pr view --json title,isDraft,state,baseRefName,headRefName +gh pr view --json title,isDraft,state,baseRefName,headRefName,isCrossRepository ``` -The PR must be open and not a draft. **`baseRefName` must be `main`** — this skill +The PR must be open and not a draft. **`isCrossRepository` must be false** — a fork's +branch can't be fetched from `origin` or force-pushed back, so step 2 would die +mid-procedure; reject it here instead. **`baseRefName` must be `main`** — this skill rebases onto `main` and assumes the merge lands there. On a stacked PR based on another branch, stop: rebasing it onto `main` would rewrite it to include or drop the base branch's commits, and the review target, the rebase, and the merge destination @@ -43,35 +45,44 @@ flag up front. ### 2. Get a checkout, then rebase -Find where the branch lives — **do not assume you can create a worktree for it.** -`git worktree add` fails if the branch is already checked out anywhere, including the -primary checkout. +**Never write shell variables you intend to use later.** Each Bash call is a fresh +shell — `$WT` and `$REVIEWED` set in one block are empty in the next. Print the value, +then substitute the literal into later commands. + +Find where the branch lives. Do **not** create a worktree: `git worktree add` fails +when the branch is checked out anywhere, `--show-toplevel` returns the wrong root when +you are already inside a linked worktree, and removing a worktree you are standing in +breaks every command after it. ```bash -git fetch origin main # the branch too: worktree add needs the ref +git fetch origin main git worktree list # is already checked out? ``` - **Already checked out** (any worktree, including the primary one): use that - directory, and **remember not to remove it in step 5** — it may be a long-lived - checkout, possibly the one the user is sitting in. It must be clean - (`git status --porcelain` empty) or the rebase won't start; if it's dirty, stop and - hand it back rather than stashing the user's work. -- **Not checked out:** create a scratch worktree beside the repo, and remember that - you created it: + directory. +- **Not checked out:** check it out in the primary checkout — the first line of + `git worktree list` — and note that you must restore it to `main` in step 5. + +Either way the directory must be clean (`git status --porcelain` empty). If it's +dirty, stop and hand it back; never stash the user's work. + +Now reconcile with the remote **before** rebasing: - ```bash - ROOT=$(git rev-parse --show-toplevel) - WT="$(dirname "$ROOT")/scratchwork-worktrees/" - git worktree add "$WT" - ``` +```bash +git rev-list --count ..origin/ # non-zero → local is stale +``` -Everything below runs in that directory. Then: +This check is not optional. Step 2 just fetched `origin/`, which means +`--force-with-lease` will compare against the ref it *just updated* and pass happily +— so a stale local branch would force-push away commits that exist only on the +remote, and nothing downstream would notice. If the count is non-zero, +`git reset --hard origin/` before continuing. ```bash git rebase origin/main git push --force-with-lease -bun install --frozen-lockfile # a fresh worktree has no node_modules +bun install --frozen-lockfile # node_modules may be absent or stale ``` Resolve only conflicts whose resolution is mechanical and whose intent on both sides @@ -85,7 +96,7 @@ whatever tree you produce and find a misresolution perfectly coherent. Spawn **two** subagents with the Agent tool, in parallel, both with `model: "fable"` (the alias — the model parameter takes `sonnet|opus|haiku|fable`, not a full model -ID). +ID). **One lens each**, not both lenses to both agents. Give each one **the checkout directory from step 2** — a subagent starts in the session's cwd, not your working directory, so a reviewer that isn't told where to @@ -104,8 +115,12 @@ genuinely finds nothing after a real search. concurrency, data loss. Where does this break in production? What input makes it wrong? - **Lens 2 — invariant conformance.** Run the `check-invariants` skill's procedure - against the diff — that skill is normative for this lens; follow it rather than a - restatement here, and if it and this file ever disagree, it wins. + against the diff. That skill owns *what to check* — follow its per-invariant + agent-pass bullets rather than a restatement here, so this file can't drift from it. + Two deliberate overrides, which you must state in the reviewer's prompt: diff against + `origin/main...HEAD`, not that skill's `main...HEAD` (local `main` may be stale, and + both lenses must review the same tree); and return the findings JSON above, not that + skill's per-invariant pass/fail report, so triage can consume both lenses the same way. ### 4. Triage, then fix @@ -123,10 +138,10 @@ converge in three rounds has a problem this skill is the wrong tool for. ### 5. Merge the tree you reviewed -Pin the reviewed commit, and confirm `main` hasn't moved out from under it: +Print the reviewed commit and confirm `main` hasn't moved out from under it: ```bash -REVIEWED=$(git rev-parse HEAD) +git rev-parse HEAD # copy this SHA; do not store it in a variable git fetch origin main git rev-list --count HEAD..origin/main # non-zero → main moved; restart at step 2 ``` @@ -137,22 +152,25 @@ registered the run yet, so retry a few times before believing it. ```bash gh pr checks --watch -gh pr merge --merge --match-head-commit "$REVIEWED" +gh pr merge --merge --match-head-commit ``` `--match-head-commit` is what makes this honest: if anything landed on the branch after the review, the merge is refused instead of silently shipping an unreviewed tree. Merge commits match this repo's history. -Then clean up, in this order — a branch checked out in a worktree cannot be deleted, -which is why `--delete-branch` is not used above: +Then clean up the remote only: ```bash -git worktree remove "$WT" # ONLY if you created it in step 2 -git branch -d git push origin --delete ``` +**Leave local state alone.** Deleting the local branch fails while it is checked out, +and the checkout may be one the user lives in — so `--delete-branch` is not used above. +If you checked the branch out in the primary checkout in step 2, restore it +(`git checkout main && git pull`). Then tell the user which local branch and which +directory are still there, so they can clean up if they want to. + ## Report The PR number and title, rebase result, review rounds, findings fixed and findings