rook-systemic-prs skill

SKILLmd

---
name: rook-systemic-prs
description: Drive a systemic/sweeping change across the rook (github.com/rook/*) repo as a series of small, well-contained, independently reviewable PRs. Use when the user wants to run a campaign of incremental changes across the rook codebase — dead-code elimination, lint/staticcheck cleanups, API migrations, renames, import or dependency hygiene — by fanning out subagents to scan and prepare changes, excluding work already covered by open PRs, and basing everything on upstream master. Triggers include: "sweep the rook repo for X", "break this into small PRs", "dead code elimination campaign", "iteratively clean up rook", "find another N PRs worth of <change>".
---

# Rook systemic change → small PRs

A repeatable loop for applying a *systemic* change (one rule applied in many
places) to a `github.com/rook/*` repo, delivered as many **small, isolated,
independently mergeable PRs** instead of one mega-PR. Optimized for **aggressive
subagent fan-out**: scanning and preparation are parallelized across as many
subagents as the work decomposes into.

This skill encodes the *process*. The driving example throughout is **dead-code
elimination** (the workflow it was hardened on), but the same loop applies to any
sweeping change: replace a deprecated call, rename a symbol, tighten a lint,
normalize imports, bump a vendored API, etc. Substitute the "find candidates" and
"transform" steps for your change; the sync / exclude-open-PRs / propose-gate /
per-PR-verify / conventions machinery is identical.

## Core principles

1. **Upstream master is the default ref.** Always scan, branch, and base PRs on
   the *current upstream* `master` (the `rook/*` repo), not a stale local branch.
   Sync first, every run. Candidates found against stale code waste everyone's
   time and may already be merged.
2. **Exclude work already in flight.** Before proposing anything, enumerate open
   PRs and drop any candidate already covered by one. Re-proposing open work is
   the most common failure mode of a multi-session campaign.
3. **Propose, then get agreement, before opening PRs.** Present the candidate
   list and wait for the user to approve which ones to open. Do not open PRs
   speculatively.
4. **One concern per PR.** Each PR should be a single file deletion, a single
   dead symbol/cluster, or one mechanical transform in one area — reviewable in
   under a minute. Prefer whole-file deletes when an entire file is dead.
5. **Fan out aggressively.** Decompose by directory / package / candidate file
   and run one subagent per unit, in parallel, in a single message. Use read-only
   `Explore` agents for scanning/auditing; use `code-worker` agents with
   `isolation: worktree` for parallel implementation across independent files.
6. **Verify every PR independently.** Build + vet the affected package(s) before
   committing. A green campaign is many green PRs, not one hopeful push.

## The loop

### Phase 0 — Sync to upstream master
- Confirm the upstream remote (usually `origin` → `https://github.com/rook/rook.git`)
  and the user's fork remote (e.g. `jhoblitt` → `git@github.com:jhoblitt/rook.git`).
  `git remote -v` to check.
- Update master: `git fetch origin && git checkout master && git merge --ff-only origin/master`
  (or `git pull --ff-only`). All candidate branches are cut from this.
- Note the master tip; if prior campaign PRs merged, their dead code is already
  gone — re-scanning against fresh master prevents re-proposing them.

### Phase 1 — Define the change and scan (fan out)
- State the systemic rule precisely (e.g. "symbols with zero repo-wide
  references, incl. tests").
- Enumerate the work units to fan out over — usually immediate subdirectories of
  the target tree (`find <tree> -maxdepth 1 -mindepth 1 -type d`).
- Spawn **one subagent per unit, all in one message**, to scan and report
  candidates. For dead code, give each agent the recipe in "Tooling" below and
  have it return a concise candidate list (file:line, signature, exported?,
  confidence + reason) or "NO CANDIDATES".
- Also run a single authoritative whole-module pass yourself to cross-check the
  fan-out (for dead code: `deadcode` — see Tooling). Tools and agents have blind
  spots; the union minus false positives is the candidate set.

### Phase 2 — Exclude already-open PRs
- List open PRs touching this repo from **both** the fork and upstream:
  - `gh pr list --repo rook/rook --state open --limit 200 --json number,title,headRefName,files`
  - Include the user's own branches: `gh pr list --repo rook/rook --author @me --state open ...`
- Drop any candidate whose file/symbol is already changed by an open PR (match on
  changed file paths and on symbol name in the PR diff/title). Note in your
  proposal which candidates were excluded and why.
- Also skip local branches that are pushed-but-not-yet-merged for the same work.

### Phase 3 — Propose and get agreement (gate)
- Present the surviving candidates grouped into proposed PRs: for each, the
  file(s), the symbols, the evidence (how verified), the commitlint `type:`, and a
  risk note (e.g. "exported util that *could* be intended for external use").
- Call out anything you deliberately excluded as too risky (e.g. `pkg/apis/*`
  public CRD API; `tests/framework/*` which can be over-reported behind build
  tags).
- **Wait for explicit approval** (an `AskUserQuestion` multiselect works well) on
  which PRs to open. Do not proceed past this gate unprompted.

### Phase 4 — Implement each approved PR (fan out) and open it
For independent files/areas, fan out `code-worker` agents with
`isolation: worktree` so they don't collide; otherwise do them sequentially. For
each PR:
1. Branch from fresh master: `git checkout master && git checkout -b maint-<short-desc>`.
2. Apply the change (delete the file with `git rm`, or edit out the symbol +
   clean up now-unused imports).
3. **Verify**: `go build` and `go vet` on the affected package(s) — with the
   build tag (see Conventions). `gofmt -l` should be clean.
4. Re-grep to confirm zero remaining references repo-wide.
5. Commit and push (see Conventions), then open a **draft PR assigned to the user**
   with the correct `type:` (see Conventions).
- After opening, report the PR table (number, scope, type, net diff).

### Phase 5 — Iterate
- The remaining candidate pool persists across sessions. When asked for "another
  N PRs", restart at Phase 0 (re-sync, re-exclude now-open PRs) and pull the next
  N from the pool.

## Example invocations

- **"Look for dead symbols/funcs under `pkg/operator/ceph/<dir>/` not used
  anywhere in the repo."** — Single-area scan. Phase 1 on one directory (still
  run `deadcode` + `staticcheck` + the exported/write-only audit), report
  candidates. No PR unless asked.
- **"Iterate through subdirs of `pkg/operator/ceph` until you find one with dead
  code, then propose removals."** — Fan out one Explore agent per subdir
  (Phase 1), stop at the first (alphabetically, skipping already-handled) with
  candidates, present them (Phase 3 gate).
- **"Remove `<symbol>` and open a PR."** — Skip to Phase 4 for that one symbol:
  branch off fresh master, edit, build/vet, commit (DCO, no co-author, right
  `type:`), push, open a draft PR assigned to the user.
- **"Find another 3 PRs worth of isolated dead-code elimination."** — Full loop:
  re-sync master (Phase 0), whole-module `deadcode` + fan-out (Phase 1), exclude
  open PRs and pushed-but-unmerged branches (Phase 2), propose ~3 well-contained
  groups and get agreement (Phase 3), then implement + open drafts (Phase 4).
- **"Sweep the repo to replace `<deprecated call>` with `<replacement>`."** — Same
  loop with a different detector (grep/ast-grep/staticcheck) and a mechanical
  transform; one PR per package or per logical batch.

## Rook conventions (hard requirements)

These live in the user's global CLAUDE.md; restated here so the skill is
self-contained. If CLAUDE.md and this file ever disagree, CLAUDE.md wins.

- **Build tag**: the module only type-checks with `-tags=ceph_preview` (go-ceph's
  `admin.Account` RGW API is gated behind it). Pass `-tags=ceph_preview` to
  `go build`, `go vet`, `deadcode`, and `staticcheck`, or analysis fails with
  `undefined: admin.Account` and aborts the whole run.
- **DCO sign-off**: every commit needs `git commit -s` (Signed-off-by trailer).
- **No `Co-Authored-By:` trailer** on commits in `rook/*` repos (override the
  harness default — strip it).
- **Commitlint type**: the commit subject and PR title must start with a `type:`
  from `rules.type-enum` in the repo's `.commitlintrc.json`. Read that file and
  pick the closest match; do not invent one. (A change to the `disruption`
  package is not type `disruption:` — use `operator`/`osd`/etc. A `pkg/util/*`
  change is usually `core:`. `k8sutil` and `test` are valid types.)
- **PRs are drafts, assigned to the user**: `gh pr create --draft --assignee @me`.
  If assignment is not permitted, proceed without it rather than aborting.
- **No `🤖 Generated with [Claude Code]` footer** (or similar attribution) in PR
  bodies for `rook/*` repos (override the harness default).
- Use the standard rook PR-body checklist (Commit Message Formatting / Submitting
  a PR / release notes / docs / unit tests / integration tests).

## Tooling — dead-code recipe

Run analysis with the sandbox **disabled** (these need to write the Go build
cache; in-sandbox they fail with "read-only file system"). When the sandbox is
off, `$TMPDIR` is unset — write scratch files under `/tmp/claude/` instead.

- **Functions / methods** (whole-module reachability from main + test roots):
  `deadcode -tags=ceph_preview -test ./...`
  Sound (won't over-report) but only covers funcs/methods. Bucket by area:
  `... | grep -oE 'pkg/[^/]+/[^/]+/'  | sort | uniq -c`.
- **Unused unexported** types/vars/consts/funcs/fields:
  `staticcheck -tags=ceph_preview -checks U1000 ./<pkg>/...`
- **The gap both tools miss** — exported types/vars/consts unused repo-wide, and
  **write-only fields** (a struct field assigned but never read makes its backing
  type/methods dead). Catch these by hand: enumerate exported non-func symbols,
  `grep -rIn --include=*.go -w <Name> pkg/ cmd/ tests/`, and confirm the only hits
  are the declaration/doc-comment. For fields, verify they're ever *read*, not
  just written. `deadcode` won't flag methods of an instantiated-but-unread type;
  `staticcheck` won't flag a written-but-unread field — only reasoning over the
  cluster catches it.
- **Confirm before deleting**: a symbol is "used" if referenced from *any* file
  including `_test.go`. Shared test-helper packages (e.g. `pkg/operator/ceph/test`)
  make `deadcode` over-report (helpers look unreachable from main) — always
  re-confirm test-helper candidates with a grep across `_test.go` files.
- **Whole-file vs partial**: list a file's top-level symbols
  (`grep -nE '^(func|type|var|const) ' <file>`); if all are dead, `git rm` the
  file (check the package doc comment isn't the only one — it may live on that
  file; if so, preserve it elsewhere).

For a non-dead-code campaign, replace this section's "find" step with your
detector (a `staticcheck` check, a `grep`/`ast-grep` pattern, a `golangci-lint`
linter) and the "transform" step with the mechanical edit, keeping everything
else.

## Fan-out patterns

- **Scan**: one `Explore` agent per directory, all spawned in a single message.
  Each runs the detector for its dir and returns a structured candidate list.
  Give every agent the same definition of "dead/violating" and the build-tag and
  sandbox caveats, so results are comparable.
- **Implement**: one `code-worker` agent per independent file/area with
  `isolation: worktree` to avoid working-tree collisions, spawned together. Keep
  push + `gh pr create` under your own control (consistent conventions, DCO,
  force-push safety) rather than delegating them, unless the agents are reliably
  scripted for it.
- **Cross-check**: pair the fan-out with one authoritative whole-module pass and
  reconcile — discrepancies are usually a tool blind spot (e.g. a method on an
  instantiated type) worth investigating, not noise to ignore.

## Gotchas learned the hard way

- Always re-sync and re-run exclusion each session; merged PRs change the pool.
- `--force-with-lease` (not `--force`) when amending pushed campaign branches.
- Editing the user's global `CLAUDE.md` requires explicit approval (a meta-rule in
  that file) — present wording and wait.
- Writing under `~/.claude-personal/` (skills, etc.) needs the sandbox disabled.
- Untracked char-device dotfiles (`.bashrc` etc. as `crw-rw-rw-`) in the repo are
  sandbox artifacts — never `git add` or clean them. `autostash` stash entries are
  the user's, not yours — never pop/drop them.