diff --git a/.claude/skills/ship-pr/SKILL.md b/.claude/skills/ship-pr/SKILL.md index 4d6833f..4eb8e69 100644 --- a/.claude/skills/ship-pr/SKILL.md +++ b/.claude/skills/ship-pr/SKILL.md @@ -1,15 +1,19 @@ --- name: ship-pr -description: Use when the user wants to open a pull request for the current branch — phrases like "open a PR", "ship this", "submit PR", "let's send it", "提 PR", "提个 MR". Walks through the canonical post-edit flow: local architecture review → L1 unit tests → push → gh pr create → gh pr merge --auto. Trigger any time the user signals they're done editing and want the change on its way to main; do NOT trigger for `gh pr view` / status checks on existing PRs. +description: Use when the user wants to open a pull request for the current branch — phrases like "open a PR", "ship this", "submit PR", "let's send it", "提 PR", "提个 MR". Walks through the canonical post-edit flow: push → open PR → wait for CI → review the diff → triage findings (self-fix small issues, escalate decisions to the user, merge directly when clean) → local cleanup. Trigger any time the user signals they're done editing and want the change on its way to main; do NOT trigger for `gh pr view` / status checks on existing PRs. --- # Ship a PR for openboot This is the canonical way to move a finished change from a feature branch into `main`. The branch protection rules defined in [docs/MERGE_POLICY.md](../../../docs/MERGE_POLICY.md) -do the gating — this skill makes sure the obvious local checks are done -first and that auto-merge is enabled so the change lands without a human -having to babysit CI. +define the **mechanical** gate (6 required checks). This skill adds the +**inferential** gate on top: after CI is green, the diff gets reviewed, +findings are surfaced, and merge only happens after the user confirms. + +**Auto-merge is intentionally not used.** `gh pr merge --auto` skips the +review step entirely — that defeats the purpose of having a review gate. +The merge command is run from this session, after the user OKs it. ## When to use @@ -17,9 +21,9 @@ Trigger when the user says they're done with a change and want a PR. **Do not** use this for: - Draft PRs / WIP work the user wants reviewed but not merged. Use plain - `gh pr create --draft` and skip auto-merge. -- Changes to `.github/workflows/` or branch protection. Those need human - review on `main`'s ruleset interaction. + `gh pr create --draft` and stop. +- Changes to `.github/workflows/` or branch protection rules. Those need + a human to also poke the GitHub UI. - Release tags (`v*.*.*`). Releases follow `make test-vm-release` first. ## The flow @@ -35,31 +39,17 @@ git log --oneline main..HEAD # commits actually exist If the branch is `main`, stop — make a feature branch first. If there are no commits ahead of `main`, stop — nothing to ship. -### Step 2 — Local review (cheap, catches most issues) - -Invoke the [`architecture-review`](../architecture-review/SKILL.md) skill -on the current diff. Fix anything it flags before moving on. The point of -running it locally is to avoid burning a CI cycle on a violation that -`internal/archtest` would catch in ~15 seconds. - -### Step 3 — L1 locally - -```bash -make test-unit -``` - -This is the same suite the `pre-push` hook would run, so think of it as -"don't ship without doing what the hook does". If `make install-hooks` is -already wired, this step is redundant — but running it explicitly costs -~15s and removes any uncertainty. - -### Step 4 — Push +### Step 2 — Push ```bash git push -u origin "$(git rev-parse --abbrev-ref HEAD)" ``` -### Step 5 — Open the PR +The `pre-push` hook (installed via `make install-hooks`) runs L1 here +automatically — no need to run `make test-unit` separately. If the hook +is not installed, that's the user's choice; CI will still gate. + +### Step 3 — Open the PR ```bash gh pr create --title "" --body "$(cat <<'EOF' @@ -80,78 +70,137 @@ Title rules (from CLAUDE.md): `test:` / `chore:` / `ci:` / `perf:`. - Keep under 70 chars. Detail goes in the body. -### Step 6 — Hand the PR to GitHub's auto-merge +### Step 4 — Wait for CI ```bash -gh pr merge --auto --squash --delete-branch +gh pr checks --watch ``` -GitHub will hold the merge until every required check from -[docs/MERGE_POLICY.md](../../../docs/MERGE_POLICY.md) reports success — -currently: - -- `lint` -- `unit (L1)` -- `integration (L2)` -- `contract schema (L3)` -- `curl|bash smoke` -- `old-cli compat` +This blocks until every check finishes. Typical wall time is 3–10 min +depending on which jobs run. The required checks per +[docs/MERGE_POLICY.md](../../../docs/MERGE_POLICY.md): + +- `lint`, `unit (L1)`, `integration (L2)`, `contract schema (L3)`, + `curl|bash smoke`, `old-cli compat` + +If any required check fails: +- **Stop. Do not proceed to review or merge.** +- Read the failure (`gh run view --log-failed`), explain it to the user, + and ask whether to fix it now. +- Drift sensors (`govulncheck`, `deadcode`, etc.) failing is + informational — flag them but do not block. + +### Step 5 — Review the diff + +Once CI is green, invoke the +[`architecture-review`](../architecture-review/SKILL.md) skill on the +**full PR diff** (`git diff main...HEAD`). CI catches mechanical +violations; this catches the rest — behaviour, design, test coverage, +risk, rollback. + +### Step 6 — Triage the findings + +Every finding goes into one of two buckets. Get this right or the gate +fails open. + +**Self-fixable — fix in this session, then loop back to Step 4.** +Anything where the correction is mechanical and clearly inside the +PR's stated scope: +- typos, formatting, doc rewording for clarity, +- dead code that this PR introduced, +- missing imports, missing test cases for branches this PR adds, +- bug fixes that don't change observable behaviour, +- following through on a rule the user has already stated in this + session (this is the recursive case). + +Fix it, push to the same branch, return to Step 4 to wait for CI again, +then come back to Step 5. Do **not** prompt the user. The point of this +branch is to keep the human's attention budget for decisions that +actually need it. + +**Needs user judgment — surface and stop.** +Anything with a real choice or scope question: +- design decisions, API shape, behaviour changes, +- anything that touches a deliberate prior decision (e.g. an explicit + user instruction or an existing convention in CLAUDE.md), +- anything that would expand the PR beyond its stated scope, +- anything you'd ask a teammate about before pushing. + +Surface the finding with the question made explicit; stop the flow. + +**Clean — proceed straight to merge.** +If CI is green AND the diff review found nothing self-fixable AND +nothing that needs user judgment, skip the "ask the user" step and +merge directly. Asking when there is nothing to decide just burns the +user's attention. + +**Rule of thumb:** would a thoughtful junior engineer file this as a +question, push a follow-up commit, or just merge it? Escalate / fix / +merge accordingly. + +### Step 7 — Merge + +Reached when Step 6 ended in "clean", OR when the user has explicitly +approved a merge after an escalation. -There is no need to `gh pr checks --watch` in the session. If any check -fails, GitHub will refuse to merge and the user will see the failure on -the PR — handle it the next time the user mentions the PR. - -### Step 7 — Tell the user +```bash +gh pr merge --squash --delete-branch +``` -Return the PR URL and one sentence on what was queued. Example: +No `--auto`. No `--admin`. The branch-protection rules still apply — if +something flipped red between Step 4 and now, GitHub will refuse and +we'll loop back to Step 4. -> PR opened: . Auto-merge enabled — will land once the 6 required -> checks pass. +Report the merge to the user as a one-liner ("PR #N merged, branch +deleted, local cleaned up"). Do not ask for confirmation **before** +merging on a clean review — the harness loop is supposed to close +itself when there is nothing to decide. -### Step 8 — Post-merge cleanup (if already merged) +### Step 8 — Local cleanup -Immediately re-check whether the PR has already landed — for small PRs -that hit `MERGEABLE` and pass CI in seconds, `gh pr merge --auto` can -complete before the user sees the PR URL: - -```bash -gh pr view --json state -q .state -``` - -If the output is `MERGED`, the remote branch has been deleted by -`--delete-branch`. Clean up the local side in the same session so the -user doesn't have to: +After merge succeeds, the remote branch was deleted by `--delete-branch`. +Bring local in sync: ```bash git checkout main && git pull --quiet git branch -d "$(git symbolic-ref --quiet --short @{-1} 2>/dev/null)" ``` -If the output is `OPEN`, do nothing here. The +(`@{-1}` refers to the previously checked-out branch — the one we just +merged.) + +If the user closes the session before reaching Step 8, the [`session-start.sh`](../../hooks/session-start.sh) stale-branch sensor -will catch it on the next session and prompt cleanup. +catches it on the next session and prints the same cleanup hint. ## What NOT to do -- **Do not skip step 2.** "It's a small change" is exactly when archtest - violations sneak in. -- **Do not merge without `--auto`** (i.e. plain `gh pr merge`) unless the - user explicitly asks for an immediate merge. Bypassing required checks - is reserved for emergencies and must be documented. -- **Do not use `--admin`.** That bypasses branch protection entirely. +- **Do not use `--auto`.** It skips Step 5 (review) entirely, which is + the whole reason this skill exists. +- **Do not use `--admin`.** That bypasses branch protection. +- **Do not merge before CI completes.** Even if the diff looks trivial, + let `gh pr checks --watch` finish — drift detection sometimes catches + surprising things. - **Do not amend / force-push** after `gh pr create` unless the user asks - — it invalidates in-flight reviews. -- **Do not push directly to `main`.** Branch protection will reject it - anyway, but try anyway and you'll have to clean up the failed push. + — it invalidates in-flight reviews and re-runs CI from scratch. +- **Do not push directly to `main`.** Branch protection will reject it. +- **Do not auto-fix findings that need user judgment.** Anything that + could change behaviour, expand scope, or touches a prior deliberate + choice gets surfaced in Step 6 — not silently committed. Self-fixable + items (typos, missing tests, doc tweaks following a stated rule) are + the exception, not the default. ## Why this is encoded as a skill -Per [docs/HARNESS.md](../../../docs/HARNESS.md), repeated guidance -becomes a control. The post-edit flow ("review → test → push → PR → -auto-merge") was previously oral tradition; encoding it here means: +Per [docs/HARNESS.md](../../../docs/HARNESS.md), repeated guidance becomes +a control. The merge gate has two layers: + +- **Mechanical** (branch protection / required checks) — already + enforced by GitHub. Catches what CI knows how to check. +- **Inferential** (review of the diff) — was previously oral tradition. + Encoding the order here ("CI green THEN review THEN ask THEN merge") + is the harness; without it, the easy thing is auto-merge, which is + cheap and wrong. -1. New agents follow the same recipe without being told. -2. If the recipe changes (e.g. a new required check), this is the one - place to edit, and AGENTS.md points here. -3. Skipped steps become a reviewable diff against this file, not a - judgment call buried in a session transcript. +If the recipe changes (new required check, different review tool), this +is the one place to edit; AGENTS.md points here. diff --git a/AGENTS.md b/AGENTS.md index ef657ec..c4ff07c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,10 +64,15 @@ Project-specific Claude skills live under [`.claude/skills/`](.claude/skills/): - `bootstrap-feature` — how to add a CLI command end-to-end. - `architecture-review` — what to check when reviewing a PR against project invariants. -- `ship-pr` — canonical post-edit flow: local review → L1 → push → - `gh pr create` → `gh pr merge --auto`. Use this instead of calling - `gh pr create` directly so auto-merge against - [`docs/MERGE_POLICY.md`](docs/MERGE_POLICY.md) is consistent. +- `ship-pr` — canonical post-edit flow: push → `gh pr create` → wait + for CI → review the diff → triage (self-fix small stuff, escalate + decisions, merge directly when clean) → `gh pr merge --squash` → + local cleanup. Use this instead of calling `gh pr create` / + `gh pr merge` directly. **Do not use `--auto`** — it skips the + review gate, which is the point of having this skill on top of the + branch-protection rules in [`docs/MERGE_POLICY.md`](docs/MERGE_POLICY.md). + **Do not ask the user to confirm a clean merge** — the loop closes + itself when there is nothing to decide. These are loaded automatically when Claude runs in this repo. diff --git a/docs/HARNESS.md b/docs/HARNESS.md index 6a91c91..5448e75 100644 --- a/docs/HARNESS.md +++ b/docs/HARNESS.md @@ -56,7 +56,7 @@ Three regulation categories: | Feedfwd. | Skills | model-loaded | `.claude/skills/*` | | Feedfwd. | Session-start hook (warm caches, fetch deps) | every Claude session | `.claude/hooks/session-start.sh` | | Feedback (agent) | Stale-branch sensor (suggests cleanup when current branch's upstream is gone) | every Claude session | `.claude/hooks/session-start.sh` | -| Feedfwd. | `ship-pr` skill — canonical PR flow incl. post-merge cleanup | model-loaded | `.claude/skills/ship-pr/SKILL.md` | +| Feedfwd. | `ship-pr` skill — canonical PR flow (push → CI → review → triage: self-fix / escalate / merge directly when clean → cleanup; **no `--auto`**, **no "do you want me to merge?" when clean**) | model-loaded | `.claude/skills/ship-pr/SKILL.md` | | Feedback (agent) | `go vet` on edited package | after every Edit/Write/MultiEdit | `.claude/hooks/post-tool-use.sh` | | Feedback (agent) | `go vet ./...` + archtest | end of every Claude turn (if .go dirty) | `.claude/hooks/stop.sh` | | Maint. | `golangci-lint` on the staged diff | local git pre-commit | `scripts/hooks/pre-commit` |