Skip to content

feat(repo-hygiene): fleet batch mode for the caches/build/git/all tiers - #1064

Merged
kyle-sexton merged 19 commits into
mainfrom
feat/repo-hygiene-fleet-batch
Jul 22, 2026
Merged

feat(repo-hygiene): fleet batch mode for the caches/build/git/all tiers#1064
kyle-sexton merged 19 commits into
mainfrom
feat/repo-hygiene-fleet-batch

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds fleet (batch) mode for the clean skill's selective tiers — caches / build / git / all — so a multi-repo sweep no longer has to be hand-rolled (which the auto-mode classifier blocks). This is the selective-tier sibling of the existing tree-batch.

A new clean-batch.sh --tier <caches|build|git|all> orchestrator runs the single-repo tiers across a repo set behind ONE confirmation gate. It runs no removal itself — every per-repo action delegates to the unchanged single-repo child (clean-caches.sh, clean-build.sh, git-prune.sh), so every child gate (protection classes, submodule/reparse guards, the dry-run manifest + re-stat staleness guard) is reused verbatim.

New action spellings: caches-batch / build-batch / git-batch / all-batch (plus *-fleet aliases), resolved by resolve-clean-action.sh.

Design

  • One --tier orchestrator, not four scripts. The batch-plan format, the single batch-wide gate, and the apply-from-plan logic are one cohesive contract; the tier is a data difference (which child + manifest-mode vs gitdir-mode), not a structural one. The resolver maps the four spellings to clean-batch.sh --tier X.
  • The batch plan IS the gated set. --dry-run writes a plan (REPO lines → per-repo child manifest; GITDIR lines → unique shared object stores) and prints BatchPlan: <path> + aggregate Summary: repos=N planned=P bytes=K. --apply --batch-plan <path> acts on that plan ONLY and errors without it. This is the fleet-level analogue of the child's per-repo manifest staleness guard: a repo that vanished after the dry-run applies idempotently (paths already gone); a repo that appeared is not in the plan, so it is never touched.
  • Central path normalization (lib/batch-common.sh): ghq list -p backslash paths → the git-friendly D:/repos/... forward-slash form once (backslashes break xargs and [[ -d ]]; git check-ignore rejects MSYS /d/… forms).
  • Shared-object-store dedup: the git tier groups repos by unique git rev-parse --git-common-dir and prunes each store once (from a representative worktree cwd), not once per linked worktree.
  • Shared plumbing in lib/batch-common.sh. tree-batch predates this module and keeps its own inline copy of read-lines / resolve-dedup / emit; migrating it onto batch-common.sh is a deliberate fast-follow (kept out of scope to keep this diff off feat(repo-hygiene): single pruned walk + dry-run manifest + apply summary #1023's churn and tree-batch's large test suite). Recorded as a decision (noted in the lib header), not silence.

Spec deviations (recorded)

  • Batch git = prune / gc / remote-prune only. Branch audit/deletion is excluded because interactive per-branch deletion can't sit behind one fleet-wide gate. Therefore all-batch = build + git-prune, no branch audit — matching the single-repo all (build + git, no tree).
  • tree is not batched here — the destructive tier has its own batch form (tree-batch) with a dirty guard.

Independent review

An independent reviewer (fresh context) audited the apply path; fixes landed here:

  • Fail-closed aggregate counter — a per-repo child that exits non-zero without a parseable Summary (e.g. a repo that lost its .git mid-sweep) now counts as a failure, so the batch exits 1 instead of silently reporting failed=0 / exit 0.
  • Fleet-alias safety routing — a selective tier token co-occurring with a fleet indicator (clean caches across all repos, caches fleet, prune git across the fleet) routes to the non-destructive <tier>-batch, never the destructive tree-batch; the ambiguous bare "fleet" trigger was dropped from the skill description.
  • Batch-plan write guard — the dry-run creates the plan directory and verifies the plan is writable, failing loudly instead of printing BatchPlan / Summary and exiting 0 with no plan.
  • Apply summary gates the gitdirs= field to the git/all tiers; the gitdir first-seen-representative limitation is documented.

Testing

  • New clean-batch.test.sh and lib/batch-common.test.sh, TDD-first; resolver gains routing cases including the fleet-safety ones. All repo-hygiene *.test.sh green; shellcheck --rcfile .shellcheckrc, shfmt -i 2, and markdownlint clean. Re-run against this PR's rebased (hardened) single-repo children.
  • Verified with real multi-repo dry-runs (never --apply) over a local ghq fleet: caches-batch over 3 repos; git-batch over a main clone + its linked worktree correctly reports gitdirs=1 (shared object store deduped); node_modules present in a repo is correctly preserved (0 planned).

Related

Closes #994

kyle-sexton and others added 16 commits July 22, 2026 12:35
…mary

Rework the clean skill's build/caches enumeration and apply flow as one change.

#993 — the selective caches/build tiers ran ~10 unpruned full-tree `find`
walks per repo; the `! -path` exclusions filtered output but did not `-prune`,
so every walk re-descended `.git/`, `node_modules/`, and `.venv/`. A single
pruned walk per tier (`clean_enumerate`) now prunes those three trees once and
`-print`s all dir-name and file-glob matches, then classifies on the result
list. Measured on a large .NET + node repo (Windows/NTFS): one pruned walk incl.
`du` sizing ~17s vs a 10-walk unpruned dry-run that exceeded 10 min (killed).

#995 — the dry-run now writes a session-scoped manifest
(`<class>\t<bytes>\t<relpath>` per eligible target), prints `Manifest: <path>`
and `Summary: planned=N bytes=K` so the gate can state reclaimable space, and
`--apply --manifest <path>` consumes it with a re-stat + re-classify staleness
guard instead of re-walking. Resume = re-run the same command (already-gone
entries are idempotent). `--apply` without a manifest builds one then applies it,
preserving the standalone CLI contract; `--include-caches` folds the caches tier
into the one build manifest (no subprocess).

#1002 — each `--apply` ends with `Summary: removed=N failed=M bytes=K` and exits
non-zero when failed>0, so a fleet sweep no longer needs per-log grepping.

Cross-tier nested-target dedup drops any eligible path under an eligible ancestor
so byte totals never double-count. scan.sh keeps the old walk pattern for now,
tracked as TODO(#1011).

Closes #993
Closes #995
Closes #1002

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…ding

The manifest is a consumed contract (#994 parses it), so assert the exact
class<TAB>bytes<TAB>relpath shape per tier rather than substrings, and cover
--include-caches folding both a build and a caches entry into one manifest.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
`clean-build.sh --apply` ran `dotnet clean <solution>` before removing bin/obj
wholesale, but the universal artifact removal already deletes everything the
driver would — running it first was pure overhead: a full MSBuild evaluation
(minutes on a large solution) that also re-created obj/ evaluation artifacts. One
walk + rm is strictly faster and equally complete. Removes the driver and its
`Planned: dotnet clean …` (dry-run) / `DRIVER_FAILED:` (apply) output markers,
and the now-stale driver references in the config/ecosystem/README docs.

Closes #999

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
The repo's comment-hygiene gate forbids tracker/issue references and TODO
markers in code comments; version control, the CHANGELOG, and the tracker own
that history. Strips the `(#NNN)` refs and the scan.sh `TODO(#…)` note from the
clean scripts and their comments — no behavior change. The scan.sh migration
deferral remains tracked by its own issue and this PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…edup

The `--apply --manifest <path>` surface is documented and caller-supplied, so
`clean_apply_manifest` now treats the manifest as untrusted input:

- Containment: an entry whose repo-relative path is absolute or traverses a
  parent (`..`, either separator) is rejected before any stat/rm and counted as
  a failure — an entry like `../outside` can no longer make `rm -rf` escape the
  repository.
- Byte field as data: the size field is validated as an unsigned decimal before
  it reaches Bash arithmetic (which evaluates array subscripts recursively), so
  a crafted value cannot execute embedded command substitution.
- Fail closed: `--apply --manifest <missing>` now exits non-zero with a clear
  message instead of printing `removed=0 failed=0` and exiting 0, which would let
  automation treat a mistyped path as a successful sweep.

Also replaces the O(n^2) nested-target dedup with an O(n log n) sort pass: each
path is sorted under a trailing-'/' key so an ancestor sorts immediately before
all its descendants (and never swallows a sibling like `buildstuff`), then a
single scan drops anything under the last kept ancestor — the dry-run no longer
spends minutes deduplicating a monorepo's thousands of candidates before it can
show the confirmation plan.

Regression tests cover containment rejection, arithmetic-injection refusal via a
sentinel, and the missing-manifest fail-closed exit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…rules

The manifest containment guard stopped `..`/absolute escapes but still trusted
that any in-repo path a manifest listed was a real cleanup target. A caller-
supplied or concurrently-altered entry such as `caches\t1\tnotes` therefore
passed the protection gate (an ordinary untracked dir is unprotected) and was
removed while the run reported success.

`clean_apply_manifest` now takes the tier's allowed classes and, per entry,
rejects (fail closed, counted as a failure) any entry whose class the tier does
not produce, and any path that is not a legitimate target for its class per the
same candidate rules enumeration uses to find targets (explicit repo-root paths,
a recognized dir-name leaf, or a file-glob leaf). clean-caches accepts only
`caches`; clean-build accepts `build` and the caches it folds in.

Regression tests cover a non-target untracked dir (`notes/`) and a wrong-tier
entry — both rejected, preserved, non-zero exit. The rm-failure case now targets
a valid explicit cache so it exercises the genuine Unremovable branch rather than
target rejection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…rwrite

Two more hardenings of the untrusted --manifest surface:

- Target type: manifest target validation now also checks the filesystem type
  each candidate rule emits (dir names / explicit dirs are found -type d, globs /
  explicit files -type f). A regular file merely named `bin` (or `__pycache__`)
  can no longer be removed by a crafted `build\t1\timportant/bin` — the dry-run
  could never have planned it. The existence check now precedes validation so a
  resumed, already-removed entry stays an idempotent no-op rather than a
  rejection.
- Manifest overwrite: `--manifest <path>` is a caller-supplied input, so the
  dry-run build refuses to truncate an existing NON-manifest file (e.g. a mistyped
  `~/.config/app/settings`) and exits non-zero instead of silently erasing it;
  an absent path or an existing manifest-format file is still (re)written.

Regression tests cover a file-as-dir-name target rejection and the
refuse-to-overwrite guard (including that a real manifest is still rewritable).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…orever

A trailing `--manifest` (e.g. `clean-caches.sh --apply --manifest`) hung: `$2`
is empty so `shift 2` fails on the single remaining arg, and without `set -e`
`$#` is unchanged, so the case loop reprocesses `--manifest` forever. Both entry
scripts now require a value to follow `--manifest` and exit with the documented
usage error (2) otherwise. Regression test guards with a `timeout` so a
regression fails loudly instead of spinning.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
Enumeration prunes .git/node_modules/.venv and never emits a target inside them,
but the apply path did not re-exclude them: a manifest entry such as
`caches\t1\t.git/objects/__pycache__` passed every guard (its basename is a valid
target name and .git is not in the protected-substring set) and was removed —
removing files under .git can corrupt the repository.

Lift the prune set into a single CLEAN_PRUNE_DIRS SSOT that the single-walk
enumeration builds its `-prune` alternation from AND the apply path rejects
against, so the prune set is enforced uniformly on both sides. Any manifest entry
with a pruned segment is now rejected (fail closed). Regression test covers a
`.git/...` entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
… records

Two manifest-parsing gaps that let a bad manifest report success:

- A valid final record with no trailing newline (common in caller-written
  files) was dropped: `read` returns nonzero at EOF so the loop body never ran,
  and `--apply` reported `removed=0 failed=0` while leaving the target in place.
  The loop now processes a nonempty final record via `|| [[ -n … ]]`.
- A nonblank but truncated record missing the path field (e.g. `caches<TAB>1`,
  or a partial write by a concurrent process) was silently skipped. It is now a
  malformed-record failure (fail closed); only genuinely blank lines are ignored.

Regression tests cover both.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…targets

Two more manifest-surface hardenings, both keeping apply to exactly what the
dry-run could have emitted:

- Manifest creation: clean_manifest_path only truncated its target and never
  checked the result, so `--dry-run --manifest <unwritable-or-missing-parent>`
  printed a `Manifest:`/`Summary: planned=` plan for a file that was never
  written, handing apply a bogus path. It now fails (non-zero, prints nothing on
  stdout) when the file cannot be created.
- Symlinked targets: the enumerator matches with `find -type d`/`-type f`, which
  never follow a symlink or Windows junction, but Bash `[[ -d ]]`/`[[ -f ]]` do.
  Target validation now requires a plain dir/file (not a symlink/reparse point),
  so a manifest naming a link (or a path swapped for one after the dry-run) is
  rejected instead of having its link removed.

Regression tests cover the uncreatable-manifest failure and the symlinked-target
rejection (the latter skips where the FS cannot create a symlink).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
…ly parity

Keeps the dry-run plan and the apply outcome faithful to each other and to the
filesystem:

- Apply loop fd isolation (root-cause bug): a per-entry validator shells out to
  `fsutil` (the reparse-point check), which drains stdin. Read on fd 0, it
  swallowed the rest of the manifest — every entry after the first was silently
  skipped, so `clean-build --apply --include-caches` removed the first target and
  reported success while leaving the rest. The loop now reads the manifest on a
  dedicated fd (3), and the reparse check gives fsutil its own `</dev/null`.
- Reclaimed bytes are re-measured from the filesystem at removal time instead of
  trusting the manifest's dry-run byte field: the target may have changed since
  the plan, and a caller-supplied value never reaches the summary arithmetic (no
  overflow, no evaluation of untrusted input).
- Dry-run/apply parity for explicit caches: an explicit cache path that is a file
  or symlink (not the plain dir `find` would emit) is filtered at planning, so
  the advertised plan is always applyable rather than being rejected at --apply.
- Unencodable paths: a candidate whose path contains a tab or newline cannot be
  encoded in the tab-delimited, newline-terminated manifest, so it is skipped
  with a warning rather than written as a record that maps to the wrong path.

Regression tests cover the file-named explicit cache, the recomputed bytes, and
the unencodable-path skip; the existing include-caches apply test now exercises
the multi-entry loop that the fd bug broke.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
Comment-hygiene forbids tracker references in code comments; reword to name the
downstream consumer generically.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
`typos` flagged `applyable` and `mis-mapped` in the new comments; reword to
plain phrasing ("the plan can still be applied", "not mapped to a wrong path").

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
The `..`/absolute containment guard and the final-component reparse check did not
cover a symlinked ANCESTOR: a manifest naming `link/__pycache__`, where `link`
points outside the repo, passed every check and `rm -rf` followed the ancestor
symlink to delete the external directory while reporting success. Enumeration
uses `find -type d`, which never descends a symlinked dir, so such a path is
never one the dry-run emitted.

Apply now walks every component of `root/rel` and rejects the entry (fail closed)
if any ancestor or the target is a symlink/reparse point — junction-aware via the
existing fsutil check, so it also catches Windows reparse-point ancestors that
`realpath`/`-L` miss. Regression test covers a symlinked ancestor pointing
outside the repo (skips where the FS cannot create a symlink).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
Add clean-batch.sh, a single orchestrator that runs the selective clean
tiers across a set of repositories behind one confirmation gate — the
selective-tier sibling of tree-batch. It runs no removal itself: every
per-repo action delegates to the unchanged single-repo child
(clean-caches.sh, clean-build.sh, git-prune.sh), reusing every child gate
(protection classes, submodule/reparse guards, the dry-run manifest +
re-stat staleness guard) verbatim.

The batch plan is the gated set. --dry-run writes a plan (REPO lines that
point at per-repo child manifests; GITDIR lines for unique shared object
stores) and prints BatchPlan + an aggregate Summary. --apply --batch-plan
consumes that plan only and errors without it, so a live fleet that races
the sweep is tolerated exactly: a vanished repo applies idempotently, an
appeared repo is never touched, and a repo that lost its .git mid-apply
fails the batch closed.

Central path normalization + shared-object-store dedup live in
lib/batch-common.sh: ghq list -p backslash paths normalize once to the
git-friendly D:/repos/... form, and the git tier prunes each unique
git-common-dir once rather than per linked worktree. New action spellings
caches-batch / build-batch / git-batch / all-batch (plus *-fleet aliases)
resolve to it; a selective tier token co-occurring with a fleet indicator
routes to the non-destructive batch form rather than the destructive
tree-batch.

Deviations: batch git is prune/gc/remote-prune only (branch audit stays
per-repo — interactive deletion can't sit behind one gate), so all-batch =
build + git-prune; tree is not batched here (tree-batch owns that).

Closes #994

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 42s —— View job


PR Review: feat(repo-hygiene) fleet batch mode

  • Gather context and understand the request
  • Read key changed files (clean-batch.sh, batch-common.sh, clean-common.sh, resolve-clean-action.sh)
  • Review for correctness, error handling, and security
  • Check test coverage
  • Review documentation and skill changes
  • Post review findings

Overall this is a well-designed, thoughtful piece of work. The one-gate/plan-is-the-set contract is sound, the fail-closed aggregate counter is correct, and the gitdir dedup is the right call. The documentation is thorough. I found a few issues ranging from a latent bug to a missing test case.

P1 — Child dry-run failure is not detected; broken REPO line is written to the plan

clean-batch.sh:326–338

out="$(cd "$top" && bash "$child" --dry-run --manifest "$manifest" "${extra[@]}" 2>&1)"
r="$(printf '%s\n' "$out" | sed -n 's/^Summary: //p' | head -1)"
planned="${planned:-0}"   # ← silent default if child failed
...
printf 'REPO\t%s\t%s\t%s\n' "$top" "$tok" "$manifest" >>"$PLAN"  # ← always written

If the manifest child exits non-zero (say, a permission error mid-walk), the script silently defaults planned=0, writes the REPO plan line, and exits dry-run with exit 0. On --apply, the child is then invoked with --apply --manifest <empty-or-missing-file> and fails — but now the user has confirmed what they thought was a clean dry-run. The dry-run should surface per-repo child failures (analogous to the fail-closed logic in the apply path at line 211).

Suggested fix: capture rc=$? after the child dry-run, and if rc != 0, emit batch_emit "$top" failed "child dry-run failed (rc=$rc)", increment a FAILED counter, and skip writing the REPO plan line. The dry-run would then exit non-zero, and the apply would never see a broken plan entry for that repo.

Fix this →


P2 — batch_add_gitdir rc=1 (unresolvable --git-common-dir) silently misreports as "deduped"

clean-batch.sh:346–351, batch-common.sh:109–124

batch_add_gitdir has three return codes: 0 (new), 2 (duplicate), 1 (could not resolve --git-common-dir). The caller only checks for 0:

if batch_add_gitdir "$top"; then       # ← rc=0 only
  # ... write GITDIR line
else
  reason_parts+=("git: shared object store (deduped with a sibling worktree)")
  # ← rc=1 AND rc=2 both land here — wrong message for rc=1
fi

When rc=1, no GITDIR line is written, git prune for that repo is silently skipped, and the reason reported to the user is "deduped with a sibling worktree" — factually wrong. In practice, rc=1 requires git rev-parse --git-common-dir to silently fail on a repo that already passed batch_resolve_repos, which is unlikely but possible (e.g., a corrupted .git). A third branch would make the failure explicit:

rc=0; batch_add_gitdir "$top" || rc=$?
if   [[ "$rc" -eq 0 ]]; then
  ...
elif [[ "$rc" -eq 2 ]]; then
  reason_parts+=("git: shared object store (deduped with a sibling worktree)")
else
  reason_parts+=("git: could not resolve shared object store — prune skipped")
  batch_emit "$top" blocked "could not resolve --git-common-dir"
fi

Fix this →


P3 — batch_add_gitdir fallback produces non-canonical keys on git < 2.26, defeating worktree dedup

batch-common.sh:112–115

common="$(git -C "$top" rev-parse --path-format=absolute --git-common-dir 2>/dev/null | tr -d '\r')"
if [[ -z "$common" ]]; then
  common="$(git -C "$top" rev-parse --git-common-dir 2>/dev/null | tr -d '\r')"
  [[ -n "$common" && "$common" != /* && "$common" != ?:/* ]] && common="$top/$common"
fi

When --path-format=absolute is unavailable (git < 2.26, released April 2020), the fallback joins $top with a potentially relative path (e.g., ../../.git). For a linked worktree, two paths like /repos/a/.git/worktrees/wt1/../../.. and /repos/a/.git/worktrees/wt2/../../.. would produce different string keys after clean_path_key, even though they name the same directory. Git gc would then run once per worktree instead of once per store — the exact problem this dedup is designed to prevent.

realpath (GNU coreutils, also available in MSYS2) or cd "$common" && pwd would canonicalize the path before keying:

common="$(cd "$common" 2>/dev/null && pwd || printf '%s' "$common")"

Fix this →


P4 — all-batch apply path is not integration-tested

clean-batch.test.sh:155–163

Test 8 (the all tier) verifies the dry-run plan contains both REPO and GITDIR lines, but never calls --apply --batch-plan. The combined REPO+GITDIR apply path — the only code path that exercises both BUILD_CHILD and GIT_CHILD in a single apply loop — is untested. A follow-up apply call (analogous to test 3 for caches or test in §6 for build) would cover it:

out="$(bash "$BATCH" --tier all --apply --batch-plan "$APLAN")"
assert_contains "all apply cleaned build artifacts" "$out" "Outcome: cleaned"
assert_contains "all apply pruned git store" "$out" "Outcome: pruned"
assert_file_absent "build dir removed by all apply" "$AR/bin/b"

Fix this →


P5 — resolve_one called twice per token in the fleet-routing loop

resolve-clean-action.sh:108–110

for token in "$@"; do
  case "$(resolve_one "$(printf '%s' "$token" | tr '[:upper:]' '[:lower:]')")" in
  caches | build | git)
    t="$(resolve_one "$(printf '%s' "$token" | tr '[:upper:]' '[:lower:]')")"  # ← redundant

resolve_one is a pure function; the second call with identical input is wasteful. One assignment before the case eliminates both forks and a needless subshell:

for token in "$@"; do
  lower="$(printf '%s' "$token" | tr '[:upper:]' '[:lower:]')"
  t="$(resolve_one "$lower")"
  case "$t" in
  caches | build | git)
    ...
  esac
done

This is minor, but given the resolver runs on every skill invocation it's a clean win.

Fix this →


Minor nit: predictable temp dir fallback

clean-batch.sh:266

PLAN_DIR="$(mktemp -d 2>/dev/null)" || PLAN_DIR="${TMPDIR:-/tmp}/clean-batch.$$"

The $$-based fallback is predictable. mktemp -d should never fail on a POSIX system, so this is not a real risk, but the existing siblings (clean-manifest.sh, git-tree-reset-batch.sh) use the same pattern, so keeping it consistent is fine. Just noting it for awareness.


Architecture — what looks good

  • One gate, plan-is-the-set contract is well-designed and correctly enforced: --apply errors without --batch-plan, and the plan write is guarded before BatchPlan:/Summary: are printed.
  • Fail-closed aggregate counter (line 211): correctly handles a child that exits non-zero without a parseable Summary line (the scenario described in the PR body).
  • Gitdir dedup is sound in structure; the key collision issue (P3) only surfaces on very old git.
  • Fleet-alias safety routing in resolve-clean-action.sh (has_fleet_indicator + selective-tier priority) is correct, clean, and well-tested.
  • clean-common.sh fd-3 manifest reader and the unencodable-path rejection are solid defensive patterns.
  • CLEAN_GUARD_ACK=1 prefixing is correctly delegated to the hook layer, not replicated in clean-batch.sh itself.
  • The deferred tree-batchbatch-common.sh migration is properly noted in the lib header rather than left as silent debt.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 49s —— View job


Security Review — PR #1064

  • Gather PR diff and changed files
  • Audit shell scripts for command injection / path traversal
  • Audit batch orchestration logic for access-control gaps
  • Audit plan-file handling and path normalization
  • Review GitHub Actions patterns (if any)
  • Compile and post findings

Scope: New files clean-batch.sh, lib/batch-common.sh, additions to lib/clean-common.sh (the manifest engine), changes to resolve-clean-action.sh, and refactored clean-caches.sh / clean-build.sh.

Overall assessment

The PR's security posture is strong. The manifest engine in lib/clean-common.sh implements thorough defense-in-depth: every apply entry is checked for ..-traversal, absolute paths, pruned-tree membership, symlinked ancestors, and type-validity before rm -rf fires. The batch orchestrator is a thin pass-through that adds no new removal capability — all destructive work stays in the existing children. No critical or high-severity findings.

Two lower-severity items follow.


SUGGESTION — Predictable temp-dir fallback opens plan-file race

Confidence: PLAUSIBLE

clean-batch.sh line 266

PLAN_DIR="$(mktemp -d 2>/dev/null)" || PLAN_DIR="${TMPDIR:-/tmp}/clean-batch.$$"

When mktemp -d fails (rare but possible on full /tmp or a race), the fallback uses $$ — the current PID — which is predictable. A local attacker who knows the PID (e.g., by monitoring /proc) could pre-create /tmp/clean-batch.<pid>/ before the script runs. mkdir -p on the pre-existing directory succeeds silently, then : >"$PLAN" creates (or truncates) the plan file inside the attacker-controlled directory. On a subsequent --apply, the script would consume the attacker's crafted plan.

Impact is limited by the robust apply-side validation — every REPO and GITDIR entry in the plan is only acted on with fully-quoted arguments and the child scripts re-validate all paths before any rm. But a crafted plan can direct cd to attacker-chosen directories and invoke git-prune.sh, which runs git gc, git worktree prune, and git remote prune in that directory. Those ops are non-destructive to file content but will pack/prune loose objects in an unintended repo.

Mitigation options:

  • Fail hard if mktemp -d returns empty (remove the || fallback entirely), or emit a clear error and exit 2.
  • If a human-readable fallback path must remain, use mkdir -m 0700 to create it atomically and fail if it already exists.

SUGGESTION — No reparse-point guard on repo toplevel in apply path

Confidence: PLAUSIBLE

clean-batch.sh line 187 and line 225

# REPO branch
if [[ ! -d "$a" ]]; then
  ...continue...
fi
out="$(cd "$a" && bash "$child" --apply --manifest "$c" "${extra[@]}" 2>&1)"

# GITDIR branch
if [[ ! -d "$a" ]]; then
  ...continue...
fi
out="$(cd "$a" && bash "$GIT_CHILD" --apply 2>&1)"

The only pre-flight check before cd "$a" is [[ -d "$a" ]], which follows symlinks. If $a (a repo toplevel recorded at dry-run time) is swapped for a symlink pointing to a different directory between dry-run and apply — a local TOCTOU — the script would cd into the wrong location and run child scripts there.

For REPO lines the impact is bounded: the caches/build children immediately re-run git rev-parse --show-toplevel and re-root themselves, and the manifest entries are fully re-validated with clean_manifest_target_valid + clean_path_has_reparse_ancestor before any removal. So file deletion in an unintended repo via this vector would require that repo to also contain eligible cache/build targets matching the manifest — unlikely.

For GITDIR lines the impact is wider: git-prune.sh --apply runs git worktree prune, git remote prune origin, and git gc --auto in whatever directory $a resolves to, without the manifest-engine's re-validation layer. A symlink swap here causes those ops to run in an unintended repository (packfile changes, worktree stale-ref pruning).

Mitigation: Apply the same clean_path_is_reparse_point guard that already exists for manifest entries to $a before cd:

if clean_path_is_reparse_point "$a"; then
  batch_emit "$a" skipped "toplevel became a reparse point (symlink swap?)"
  continue
fi

Non-findings noted (for completeness)

  • eval "$op" in git-prune.sh line 67: Pre-existing code, not introduced by this PR. GIT_PRUNE_OPS is a static array defined in cleanup-paths.sh with no user-input dataflow.
  • --batch-plan FILE truncates existing files without format check: Intentional documented behavior (the flag means "write plan here"). Contrast with the child manifest's clean_manifest_writable_target guard — that guard exists because --manifest is also the apply flag; the batch plan is write-only at dry-run time.
  • Manifest class allowed-check with empty $class: An empty class field passes the " $allowed " != *" $class "* glob check but is correctly rejected downstream by clean_manifest_target_valid's *) return 1 fallthrough. Defense-in-depth works.
  • clean_plan sort pipe with paths: Tabs and newlines in paths are filtered before elig_abs is populated; no path injection into the sort.
  • resolve-clean-action.sh fleet routing: Pattern matching only, no command execution on user input.
  • No GitHub Actions changes in this PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35e4075280

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-caches.test.sh Outdated
Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh Outdated
Comment thread plugins/repo-hygiene/skills/clean/scripts/resolve-clean-action.sh
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier).

Fold PR #1023's now-merged single-repo manifest work (squashed to main) into
the fleet-batch branch, and land the CI fixes: shebang exec bits on the new
scripts, corrected shellcheck source directives, and generic example paths in
the batch-common test.
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d0c564d18

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh
Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh
…ifests, safe all-fleet routing

Address Codex review on the fleet-batch orchestrator:

- Apply validates each REPO plan record (non-empty toplevel, caches|build tier,
  and an existing manifest file) before invoking the child. A truncated or
  hand-edited line with an empty manifest field previously passed --manifest ""
  to the child, which reads that as "no manifest" and re-walks the live repo —
  removing artifacts never shown in the gated dry-run. Now fails closed.
- Dry-run fails closed on a non-zero child: the repo is reported blocked and left
  out of the plan instead of writing a REPO line with a silent planned=0.
- Per-repo manifest names are prefixed with the plan index so two keys that
  differ only by punctuation the sanitizer collapses (repo-a vs repo_a) never
  share a manifest file.
- A non-reset all-tier fleet request (clean all repos, sweep across all repos)
  resolves to the selective all-batch, not the destructive tree-batch; only
  reset/fresh-pull phrasing keeps tree-batch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 29047f0e29

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh
A typo'd --batch-plan path pointing at an existing ordinary file would be
truncated by the plan redirect before any validation, irreversibly destroying
unrelated user data. The dry-run now refuses an existing target unless it is
already a batch plan (every non-empty line a REPO/GITDIR tab-record) — the
resumable case — mirroring the child --manifest non-manifest-overwrite guard.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0c7f8f7b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh
@kyle-sexton
kyle-sexton merged commit cc5a208 into main Jul 22, 2026
25 checks passed
@kyle-sexton
kyle-sexton deleted the feat/repo-hygiene-fleet-batch branch July 22, 2026 21:30
kyle-sexton added a commit that referenced this pull request Jul 22, 2026
…ght-resolver

Batch C's fleet-batch mode merged to main first (first-ready-wins). Rebase this
PR's version to 0.7.0 over C's 0.6.0 and reorder the CHANGELOG so 0.7.0 (stash
audit, worktree/no-upstream branch classes, resolver notes, single-walk scan)
sits above 0.6.0 (fleet batch). Shared-file conflicts resolved to carry BOTH
feature sets: SKILL.md description/argument-hint list the stash action and the
caches/build/git/all batch forms; resolve-clean-action.sh keeps the action-token
note short-circuit and adopts C's reset-intent split (tree-batch vs all-batch).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016D1oCPX8LaUKnLUi3TXmih
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
…d --tier (#1081) (#1088)

## Summary

`clean-batch.sh --apply --batch-plan <path>` dispatched purely on each
plan line's
kind (`REPO`/`GITDIR`); the apply-time `--tier` flag was informational
only (it set
the banner and whether the summary reports `gitdirs=`). A stale or
swapped plan
therefore executed its full gated content while the banner named a
narrower tier —
e.g. a `--tier build` dry-run plan applied with `--tier caches` removed
both `bin/`
and `.pytest_cache/` while printing `Tier: caches`. Bounded defect:
every removed
path was still enumerated and confirmation-gated at plan creation, so
nothing
un-gated is ever removed — the flaw is scope *misrepresentation* at
apply time.

## Fix

Per the triage decision on #1081 (validate, do not drop the flag),
`--apply` now
pre-scans the whole plan **before the banner and before touching any
disk** and
refuses it atomically (usage error, exit 2, nothing removed, no apply
banner) when a
record the requested `--tier` does not authorize is present:

| Record | Authorized under |
| --- | --- |
| `REPO` token `caches` | `--tier caches` |
| `REPO` token `build` | `--tier build`, `--tier all` |
| `GITDIR` | `--tier git`, `--tier all` |

This rejects a `build`-class REPO record under `--tier caches` and gates
the
`GITDIR` arm on a git-bearing tier, exactly as the triage resolved.
Atomic refusal
(vs. per-record rejection) means the apply banner is never printed for a
mismatched
plan, so the misrepresentation is structurally impossible rather than
merely caught
after some records already ran. A malformed/unrecognized token is still
handled as
before by the apply loop's per-record fail-closed guard (exit 1) — a
distinct error
class (structural corruption) from a well-formed plan built for the
wrong tier.

Authorization uses a dedicated `tier_repo_token` helper (empty for the
git tier) —
deliberately **not** the existing `manifest_child_token`, which returns
`build` for
git and would wrongly authorize a build REPO record under `--tier git`.

Header / `usage` / `clean-batch.md` exit-taxonomy and gated-set docs
updated to match.

## Verification

All commands run in the worktree on this branch.

Full test suite (57 assertions, incl. 3 new tier-authorization tests) +
shellcheck:

```
$ shellcheck plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh && echo CLEAN
CLEAN
$ bash plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh | tail -8
PASS: [35] build plan under --tier caches is refused (exit 2)
PASS: [36] tier-mismatch refusal reported
PASS: [37] no apply banner printed on tier mismatch
PASS: [38] build dir NOT removed by mismatched apply
PASS: [39] cache NOT removed by mismatched apply
PASS: [40] git plan under --tier caches is refused (exit 2)
PASS: [41] GITDIR-under-non-git refusal reported
PASS: [42] all plan under --tier all applies (exit 0)
...
clean-batch.test.sh: all passed
```

Exact issue repro (a `--tier build` dry-run plan applied with `--tier
caches`) —
both `bin/` and `.pytest_cache/` survive:

```
=== plan contents (REPO/build record folds caches) ===
REPO	<demo>	build	<tmp>/000-....manifest

=== 2. --tier caches --apply --batch-plan <build-plan>  (the bug scenario) ===
clean-batch.sh: plan record does not match --tier caches: a 'build' REPO record is not authorized (plan built for a different tier?). Re-run --dry-run --tier caches.
exit=2
=== 3. artifacts still present? ===
<demo>/.pytest_cache/x
<demo>/bin/b
BOTH SURVIVE -- scope misrepresentation prevented
```

Version: `plugins/repo-hygiene` bumped 0.7.1 → 0.7.2 (patch bugfix) with
a matching
CHANGELOG `[0.7.2]` `Fixed` entry. `main` published its own doc-only
`0.7.1` while
this branch was open, so this fix takes `0.7.2` — otherwise consumers
already on the
published `0.7.1` would never see it. `main`'s `[0.7.1]` entry is
preserved verbatim.

## Related

- #994 (fleet batch mode, PR #1064) — where the plan-as-gated-set
contract landed;
  this issue is the deferred Codex P2 finding from that review.
- The triage comment on #1081 resolved the validate-vs-drop-flag fork in
favor of
validation ("reject `build`-class REPO records under `--tier caches`;
gate the
`GITDIR` arm on a git-bearing tier — veto before merge"); this PR
implements that
  direction exactly.

Closes #1081

Work-class: C3 (bug-fix-shaped) — attended triage 2026-07-23,
operator-ratified. 🤖

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repo-hygiene: no batch mode for caches/build/git tiers — fleet sweep forces hand-rolled orchestration

1 participant