From ae6afd3eeff943b31ee83ed9368eeb1e5ec915a0 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Fri, 26 Jun 2026 13:53:54 +0800 Subject: [PATCH] chore: remove repo-local ship-pr skill Now provided by the fullstackjam plugin (fullstackjam:ship-pr), so the in-repo copy under .claude/skills/ is redundant. Drop it and clean up the stale references that pointed at the deleted file: - AGENTS.md: remove the ship-pr bullet from the Skills section - docs/HARNESS.md: remove the ship-pr row from the harness table The historical 'No session-start stale-branch sensor' note in HARNESS.md keeps its ship-pr mention as design rationale (it stands on its own). --- .claude/skills/ship-pr/SKILL.md | 220 -------------------------------- AGENTS.md | 9 -- docs/HARNESS.md | 1 - 3 files changed, 230 deletions(-) delete mode 100644 .claude/skills/ship-pr/SKILL.md diff --git a/.claude/skills/ship-pr/SKILL.md b/.claude/skills/ship-pr/SKILL.md deleted file mode 100644 index 8d2e739..0000000 --- a/.claude/skills/ship-pr/SKILL.md +++ /dev/null @@ -1,220 +0,0 @@ ---- -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: 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) -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 - -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 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` first (see CONTRIBUTING.md "VM E2E setup"). - -## The flow - -### Step 1 — Confirm the branch is shippable - -```bash -git status -sb # clean except expected diff? -git rev-parse --abbrev-ref HEAD # not on main -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 — Push - -```bash -git push -u origin "$(git rev-parse --abbrev-ref HEAD)" -``` - -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 - -Before writing the body, check for a PR template: - -```bash -cat .github/pull_request_template.md 2>/dev/null -``` - -If the file exists, use it as the body structure — fill in its sections -rather than inventing a different format. Do **not** discard the template -by passing a completely custom `--body` that ignores it. If the template -has a cross-repo checklist item (e.g. "does this need a docs update in -openboot.dev?"), answer it honestly. - -If no template exists, fall back to the default structure: - -```bash -gh pr create --title "" --body "$(cat <<'EOF' -## Summary - -- -- - -## Test plan - -- [ ] -EOF -)" -``` - -Title rules (from CLAUDE.md): -- Conventional Commits prefix: `feat:` / `fix:` / `docs:` / `refactor:` / - `test:` / `chore:` / `ci:` / `perf:`. -- Keep under 70 chars. Detail goes in the body. - -### Step 4 — Wait for CI - -```bash -gh pr checks --watch -``` - -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)`, `contract schema (L2)`, `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. - -```bash -gh pr merge --squash --delete-branch -``` - -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. - -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 — Local cleanup - -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)" -``` - -(`@{-1}` refers to the previously checked-out branch — the one we just -merged.) - -This step is part of the loop, not optional. Because merges are -synchronous now (no `--auto`), there is no scenario where a feature -branch should be left checked out after a successful merge. - -## What NOT to do - -- **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 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 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. - -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 0873dd6..95e5fcb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,15 +64,6 @@ 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: 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 a289218..e46d1d9 100644 --- a/docs/HARNESS.md +++ b/docs/HARNESS.md @@ -58,7 +58,6 @@ Three regulation categories: | Feedfwd. | Agent conventions | every AI turn | `CLAUDE.md`, `AGENTS.md` | | Feedfwd. | Skills | model-loaded | `.claude/skills/*` | | Feedfwd. | Session-start hook (warm caches, fetch deps) | every Claude session | `.claude/hooks/session-start.sh` | -| 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` |