diff --git a/plugins/repo-hygiene/CHANGELOG.md b/plugins/repo-hygiene/CHANGELOG.md index efe574d350..53cf50c21e 100644 --- a/plugins/repo-hygiene/CHANGELOG.md +++ b/plugins/repo-hygiene/CHANGELOG.md @@ -23,6 +23,32 @@ All notable changes to the `repo-hygiene` plugin are documented here. Format fol --include-caches` build-tier flow (`SKILL.md` §3) must repeat `--include-caches` on the `--apply --manifest ` step too, or the folded-in `caches` entries are rejected instead of removed. +- **`clean-batch.sh --apply` now validates the batch plan against the requested + `--tier` before touching disk.** The apply-time `--tier` was informational only — + dispatch keyed purely on each plan line's `REPO`/`GITDIR` kind — so a stale or + swapped plan 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`). Apply now pre-scans the + plan and refuses it atomically (usage error, nothing removed, no apply banner) when + a record the requested tier does not authorize is present: a `build`-class REPO + record under `--tier caches`, a `caches` record under `build`, or a `GITDIR` record + under a non-git tier. The removal set is unchanged (every path was still gated at + plan creation); the fix closes the scope-misrepresentation between the `--tier` flag + and what apply actually removes. The check is bidirectional: because `--tier all` + authorizes both record kinds, a narrower plan would pass every per-record test and + then run only half the tier (a `build` plan skipping every prune, a `git` plan + skipping every build removal), so a non-empty plan applied with `--tier all` must + carry both a `REPO` and a `GITDIR` record. An empty plan stays a no-op. Presence + is satisfied only by a structurally well-formed record: a truncated `GITDIR` line + naming no representative worktree (or a `REPO` line naming no manifest) names no + target, so counting it would let a narrower plan clear the `all` requirement and + then print `Tier: all` with `gitdirs=1` while performing no Git cleanup at all. + Such a record now also fails closed per-record at apply — reported as `malformed + plan record`, counted in `failed=` and never in `gitdirs=`, instead of being + reported as a store that vanished after the dry-run. **Caller-visible:** a plan + carrying a malformed record of the kind the `all` tier still needs is now refused + atomically (exit 2, nothing removed) where it previously applied its other half + and exited 1. (#1081) ## [0.7.1] diff --git a/plugins/repo-hygiene/skills/clean/context/clean-batch.md b/plugins/repo-hygiene/skills/clean/context/clean-batch.md index 1e6ac85d82..74829f6d9b 100644 --- a/plugins/repo-hygiene/skills/clean/context/clean-batch.md +++ b/plugins/repo-hygiene/skills/clean/context/clean-batch.md @@ -102,6 +102,28 @@ fleet safe to sweep: a repo that vanished after the dry-run applies idempotently (its manifest paths are already gone); a repo that appeared is not in the plan, so it is never touched. Do not re-enumerate at apply — pass the plan back. +Apply also validates the plan against the requested `--tier` before touching disk: +the plan must have been built for the same tier. A plan whose records the tier does +not authorize — a `build` REPO record (which folds caches) under `--tier caches`, a +`caches` record under `build`, or a `GITDIR` record under a non-git tier — is +refused atomically (usage error, nothing removed, no apply banner) so the `--tier` +flag can never under-report the scope of what a swapped or stale plan removes. + +The check runs in both directions. `all` authorizes both record kinds, so a +narrower plan would clear every per-record test and then run only part of the tier +— a `build` plan (no `GITDIR` records) applied with `--tier all` would skip every +prune, a `git` plan (no `REPO` records) would skip every build removal. A non-empty +plan applied with `--tier all` must therefore carry both kinds, or it is refused +the same way. An empty plan plans nothing for either kind and stays a no-op. + +Only a structurally well-formed record satisfies that both-kinds requirement — a +`GITDIR` line naming no representative worktree, or a `REPO` line naming no +manifest, names no target and so cannot stand in for the tier half it belongs to. +A malformed record is a different error class from a wrong-tier plan: the plan is +not refused wholesale, but the record fails closed per-record at apply (structural +corruption, exit 1, counted in `failed=` and never in `gitdirs=`) rather than being +reported as a store that vanished after the dry-run. + ### Per-repo outcome Each repo emits `Repo:` / `Outcome:` / `Reason:`. Outcomes: `would-clean` diff --git a/plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh b/plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh index 6478b06451..7fc8e1eb6c 100755 --- a/plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh +++ b/plugins/repo-hygiene/skills/clean/scripts/clean-batch.sh @@ -30,8 +30,10 @@ # Default: --dry-run. # # Exit: 0 ran to completion (skips/blocks are normal outcomes); -# 1 one or more repos failed mid-apply (a child rm failure); -# 2 usage/validation error (no tier, no repos, bad flag, apply without plan). +# 1 one or more repos failed mid-apply (a child rm failure, or a structurally +# corrupt plan record failed closed); +# 2 usage/validation error (no tier, no repos, bad flag, apply without plan, +# or a plan whose records do not match the requested --tier). set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -76,8 +78,12 @@ Gate: --apply --batch-plan P apply the gated plan P from a prior dry-run. Required: apply without --batch-plan is a usage error (the gate is mandatory). - Prints `Summary: removed=N failed=M bytes=K` (plus gitdirs=G - for the git/all tiers). Exits non-zero if any repo failed. + P must have been built for the SAME --tier: a plan whose + records the requested tier does not authorize (e.g. a build + plan under --tier caches, or a GITDIR record under a non-git + tier) is refused before anything is removed. Prints `Summary: + removed=N failed=M bytes=K` (plus gitdirs=G for the git/all + tiers). Exits non-zero if any repo failed. Exit: 0 ran to completion; 1 a repo failed mid-apply; 2 usage error. EOF @@ -163,6 +169,35 @@ tier_has_git() { [[ "$TIER" == git || "$TIER" == all ]]; } # Manifest child token written into the plan (apply picks the script from it). manifest_child_token() { [[ "$TIER" == caches ]] && printf 'caches' || printf 'build'; } +# The REPO manifest token the requested tier AUTHORIZES at apply, or empty for a +# tier that authorizes no REPO record (git). Distinct from manifest_child_token, +# which never returns empty and would wrongly authorize a build REPO record under +# --tier git: authorization must fail closed for a tier that plans no REPO work. +tier_repo_token() { + case "$TIER" in + caches) printf 'caches' ;; + build | all) printf 'build' ;; + *) printf '' ;; + esac +} + +# Is a plan record structurally well-formed — does it carry the fields the dry-run +# writer emits? Shared by the tier pre-scan and the apply loop so both agree on +# what counts as a record: a truncated or hand-edited line (a GITDIR naming no +# representative worktree, a REPO naming no manifest) must neither satisfy the +# `all` both-kinds-present requirement nor reach a child. Field SHAPE only — +# whether the referenced manifest still EXISTS is deliberately not judged here, +# because a manifest that vanished after the dry-run is a per-record runtime +# failure the apply loop reports (exit 1), not a plan built for the wrong tier, and +# judging it in the pre-scan would refuse the whole apply with the wrong diagnosis. +plan_repo_record_wellformed() { + local top="$1" token="$2" manifest="$3" + [[ -n "$top" && ("$token" == caches || "$token" == build) && -n "$manifest" ]] +} +# A GITDIR record's only load-bearing field is the representative worktree toplevel +# to cd into; its common-dir key is informational. +plan_gitdir_record_wellformed() { [[ -n "$1" ]]; } + # --------------------------------------------------------------------------- # APPLY: consume the gated plan only (never re-enumerate). # --------------------------------------------------------------------------- @@ -170,6 +205,60 @@ if [[ "$DRY_RUN" -eq 0 ]]; then [[ -n "$BATCH_PLAN_ARG" ]] || fail_usage "--apply requires --batch-plan from a prior --dry-run" [[ -f "$BATCH_PLAN_ARG" ]] || fail_usage "batch plan not found: $BATCH_PLAN_ARG" + # Tier-authorization pre-scan: refuse the whole apply — before the banner, before + # touching any disk — if the plan carries a record the requested --tier does not + # authorize. A plan built for a broader tier (e.g. a build plan, whose REPO + # records fold caches) applied under a narrower --tier caches would otherwise + # execute the broader gated content while the banner names the narrower tier — + # scope misrepresentation. Atomic refusal (exit 2, nothing removed, no banner) + # makes that structurally impossible. A malformed/unrecognized token is NOT judged + # here; the apply loop fails those closed per-record (exit 1) as structural + # corruption, a different error class from a well-formed plan for the wrong tier. + # + # Authorization alone is only half the match: `all` authorizes BOTH record kinds, + # so a narrower plan (a `build` plan with no GITDIR records, or a `git` plan with + # no REPO records) would pass every per-record test and then apply only part of + # the requested tier while the banner named `all`. The same misrepresentation, + # from the other direction. So the plan's record-kind SET must also be the set the + # tier plans: `all` requires both kinds present. Presence is required only when + # the plan carries records at all — a fleet where every repo was skipped or + # blocked plans nothing for either kind, and an empty plan removes nothing under + # any tier. + # + # Presence is satisfied only by a STRUCTURALLY WELL-FORMED record. A truncated + # line names no target, so counting it would let a narrower plan clear the `all` + # requirement on a record that removes nothing — apply would print `Tier: all` and + # `gitdirs=1` while performing no Git cleanup, the same misrepresentation this + # pre-scan exists to close. The record KIND is still authorization-judged + # regardless of its fields: a GITDIR line under a non-git tier is a wrong-tier + # plan however malformed it is. + PLAN_HAS_REPO=0 + PLAN_HAS_GITDIR=0 + while IFS=$'\t' read -r kind a b c; do + [[ -n "$kind" ]] || continue + case "$kind" in + REPO) + [[ "$b" == caches || "$b" == build ]] || continue + if [[ "$b" != "$(tier_repo_token)" ]]; then + fail_usage "plan record does not match --tier $TIER: a '$b' REPO record is not authorized (plan built for a different tier?). Re-run --dry-run --tier $TIER." + fi + plan_repo_record_wellformed "$a" "$b" "$c" && PLAN_HAS_REPO=1 + ;; + GITDIR) + tier_has_git || fail_usage "plan record does not match --tier $TIER: a GITDIR record requires --tier git or all (plan built for a different tier?). Re-run --dry-run --tier $TIER." + plan_gitdir_record_wellformed "$a" && PLAN_HAS_GITDIR=1 + ;; + *) ;; + esac + done <"$BATCH_PLAN_ARG" + + if [[ "$TIER" == all && "$PLAN_HAS_REPO" -ne "$PLAN_HAS_GITDIR" ]]; then + if [[ "$PLAN_HAS_GITDIR" -eq 0 ]]; then + fail_usage "plan record does not match --tier all: the plan carries no well-formed GITDIR record, so applying it would skip the git tier entirely (plan built for a different tier?). Re-run --dry-run --tier all." + fi + fail_usage "plan record does not match --tier all: the plan carries no well-formed REPO record, so applying it would skip the build tier entirely (plan built for a different tier?). Re-run --dry-run --tier all." + fi + printf 'Fleet Clean (apply)\n' printf 'Tier: %s\n' "$TIER" printf '%s\n' '---' @@ -188,7 +277,9 @@ if [[ "$DRY_RUN" -eq 0 ]]; then # field would pass --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. Fail closed — the batch contract is "apply the plan only". - if [[ -z "$a" || ("$b" != caches && "$b" != build) || -z "$c" || ! -f "$c" ]]; then + # The field-shape half is the pre-scan's predicate; the manifest must also + # still be on disk by the time apply reads it. + if ! plan_repo_record_wellformed "$a" "$b" "$c" || [[ ! -f "$c" ]]; then batch_emit "${a:-}" failed "malformed plan record (tier='$b' manifest='$c')" FAILED=$((FAILED + 1)) continue @@ -229,7 +320,17 @@ if [[ "$DRY_RUN" -eq 0 ]]; then fi ;; GITDIR) - # a=representative worktree toplevel (b=common-dir key, informational) + # a=representative worktree toplevel (b=common-dir key, informational). + # Validate before counting, as the REPO arm does: a truncated line names no + # representative, so an empty `a` would count toward gitdirs= and then read as + # `! -d` — reporting a prune "skipped (vanished)" for a store that never had a + # target, and a gitdirs= tally larger than the prunes actually attempted. Fail + # closed as structural corruption instead. + if ! plan_gitdir_record_wellformed "$a"; then + batch_emit "" failed "malformed plan record (GITDIR names no representative worktree)" + FAILED=$((FAILED + 1)) + continue + fi GITDIRS=$((GITDIRS + 1)) if [[ ! -d "$a" ]]; then batch_emit "$a" skipped "vanished after dry-run (gone from fleet)" diff --git a/plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh b/plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh index 6e7cead0a4..aaf7e42777 100755 --- a/plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh +++ b/plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh @@ -150,6 +150,138 @@ assert_exit "malformed REPO record (empty manifest) fails closed (exit 1)" 1 "$r assert_contains "malformed record reported" "$out" "malformed plan record" assert_file_exists "live repo cache NOT re-walked/removed" "$R7/.pytest_cache/x" +# --- 4d. tier-authorization: a plan built for a BROADER tier must be refused when +# applied under a NARROWER --tier, before anything is removed. The exact +# repro: a `--tier build` dry-run plan (REPO/build records that fold caches) +# applied with `--tier caches` must NOT remove bin/ OR .pytest_cache/. --- +R8="$(mkrepo r8)" +out="$(bash "$BATCH" --tier build --repo "$R8")" +PLAN_R8="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +rc=0 +out="$(bash "$BATCH" --tier caches --apply --batch-plan "$PLAN_R8" 2>&1)" || rc=$? +assert_exit "build plan under --tier caches is refused (exit 2)" 2 "$rc" +assert_contains "tier-mismatch refusal reported" "$out" "does not match --tier caches" +assert_not_contains "no apply banner printed on tier mismatch" "$out" "Fleet Clean (apply)" +assert_file_exists "build dir NOT removed by mismatched apply" "$R8/bin/b" +assert_file_exists "cache NOT removed by mismatched apply" "$R8/.pytest_cache/x" + +# --- 4e. tier-authorization: a GITDIR record must be refused under a non-git tier +# (gate the GITDIR arm on a git-bearing tier). --- +GR2="$(mkrepo gitrepo2)" +out="$(bash "$BATCH" --tier git --repo "$GR2")" +GPLAN2="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +rc=0 +out="$(bash "$BATCH" --tier caches --apply --batch-plan "$GPLAN2" 2>&1)" || rc=$? +assert_exit "git plan under --tier caches is refused (exit 2)" 2 "$rc" +assert_contains "GITDIR-under-non-git refusal reported" "$out" "GITDIR record requires --tier git or all" + +# --- 4f. tier-authorization is not over-strict: an `all` plan (REPO/build + GITDIR) +# applied under --tier all authorizes BOTH record kinds and runs them. --- +AR2="$(mkrepo allrepo2)" +out="$(bash "$BATCH" --tier all --repo "$AR2")" +APLAN2="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$APLAN2" 2>&1)" || rc=$? +assert_exit "all plan under --tier all applies (exit 0)" 0 "$rc" +assert_contains "all apply cleans the build/caches manifest" "$out" "Outcome: cleaned" +assert_contains "all apply prunes the shared object store" "$out" "Outcome: pruned" +assert_file_absent "all apply removed build dir" "$AR2/bin/b" +assert_file_absent "all apply removed cache" "$AR2/.pytest_cache/x" + +# --- 4g. tier-authorization runs in BOTH directions: `all` authorizes both record +# kinds, so a NARROWER plan passes every per-record test while applying only +# part of the requested tier. A `build` plan (no GITDIR) under --tier all +# would clean build artifacts and silently skip every prune; a `git` plan +# (no REPO) would prune and silently skip every build removal. Both are the +# same scope misrepresentation as 4d, from the other side. --- +AR3="$(mkrepo allrepo3)" +out="$(bash "$BATCH" --tier build --repo "$AR3")" +BPLAN3="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$BPLAN3" 2>&1)" || rc=$? +assert_exit "build plan under --tier all is refused (exit 2)" 2 "$rc" +assert_contains "missing-GITDIR refusal reported" "$out" "carries no well-formed GITDIR record" +assert_not_contains "no apply banner on missing-GITDIR refusal" "$out" "Fleet Clean (apply)" +assert_file_exists "build dir NOT removed by under-scoped all apply" "$AR3/bin/b" + +AR4="$(mkrepo allrepo4)" +out="$(bash "$BATCH" --tier git --repo "$AR4")" +GPLAN4="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$GPLAN4" 2>&1)" || rc=$? +assert_exit "git plan under --tier all is refused (exit 2)" 2 "$rc" +assert_contains "missing-REPO refusal reported" "$out" "carries no well-formed REPO record" +assert_not_contains "no apply banner on missing-REPO refusal" "$out" "Fleet Clean (apply)" + +# An EMPTY plan plans nothing for either kind and removes nothing, so the presence +# requirement must not turn it into an error under --tier all. +EMPTYPLAN="$TEST_TMPDIR/empty-all.plan" +: >"$EMPTYPLAN" +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$EMPTYPLAN" 2>&1)" || rc=$? +assert_exit "empty plan under --tier all applies as a no-op (exit 0)" 0 "$rc" +assert_contains "empty all apply reports gitdirs=0" "$out" "gitdirs=0" + +# --- 4h. only a STRUCTURALLY WELL-FORMED record satisfies the `all` both-kinds +# requirement. A truncated record (`GITDIR\t\t`, `REPO\t\tbuild\t`) names no +# target, so counting it as presence would let a narrower plan clear 4g's +# check on a record that removes nothing — apply would print `Tier: all` and +# a gitdirs= tally while performing no cleanup for that half. The apply loop +# fails such a record closed (exit 1, structural corruption) — a distinct +# class from 4d/4g's well-formed-but-wrong-tier plan (exit 2, atomic). --- +AR5="$(mkrepo allrepo5)" +out="$(bash "$BATCH" --tier build --repo "$AR5")" +BPLAN5="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +printf 'GITDIR\t\t\n' >>"$BPLAN5" # truncated: no representative worktree +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$BPLAN5" 2>&1)" || rc=$? +assert_exit "truncated GITDIR does not satisfy --tier all (exit 2)" 2 "$rc" +assert_contains "truncated GITDIR still reported as missing" "$out" "carries no well-formed GITDIR record" +assert_not_contains "no apply banner on truncated-GITDIR refusal" "$out" "Fleet Clean (apply)" +assert_file_exists "build dir NOT removed by truncated-GITDIR all apply" "$AR5/bin/b" + +# The mirror: a truncated REPO record must not satisfy the build half either. The +# reachable truncation is a missing MANIFEST field (as in 4c) — tab is IFS +# whitespace, so `read` collapses consecutive tabs and an empty leading field +# cannot survive plan parsing. +AR6="$(mkrepo allrepo6)" +out="$(bash "$BATCH" --tier git --repo "$AR6")" +GPLAN6="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +printf 'REPO\t%s\tbuild\t\n' "$AR6" >>"$GPLAN6" # truncated: no manifest path +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$GPLAN6" 2>&1)" || rc=$? +assert_exit "truncated REPO does not satisfy --tier all (exit 2)" 2 "$rc" +assert_contains "truncated REPO still reported as missing" "$out" "carries no well-formed REPO record" +assert_not_contains "no apply banner on truncated-REPO refusal" "$out" "Fleet Clean (apply)" +assert_file_exists "nothing pruned/removed by truncated-REPO all apply" "$AR6/bin/b" + +# The apply-loop half, on the path the `all` presence check does not gate: a +# truncated GITDIR under --tier git must fail closed and count NO gitdir — never +# exit 0 with gitdirs=1 for a prune that had no target. +TRUNCPLAN="$TEST_TMPDIR/trunc-gitdir.plan" +printf 'GITDIR\t\t\n' >"$TRUNCPLAN" +rc=0 +out="$(bash "$BATCH" --tier git --apply --batch-plan "$TRUNCPLAN" 2>&1)" || rc=$? +assert_exit "truncated GITDIR fails closed under --tier git (exit 1)" 1 "$rc" +assert_contains "truncated GITDIR reported malformed" "$out" "malformed plan record" +assert_contains "truncated GITDIR counted as a failure, not a gitdir" "$out" "failed=1 bytes=0 gitdirs=0" + +# Not over-strict: one valid GITDIR satisfies presence even alongside a malformed +# sibling — the valid store is still pruned, the malformed record still fails closed. +AR7="$(mkrepo allrepo7)" +out="$(bash "$BATCH" --tier all --repo "$AR7")" +APLAN7="$(sed -n 's/^BatchPlan: //p' <<<"$out")" +printf 'GITDIR\t\t\n' >>"$APLAN7" +rc=0 +out="$(bash "$BATCH" --tier all --apply --batch-plan "$APLAN7" 2>&1)" || rc=$? +assert_exit "valid GITDIR + malformed sibling applies then fails closed (exit 1)" 1 "$rc" +assert_contains "presence satisfied by the valid record (apply ran)" "$out" "Fleet Clean (apply)" +assert_contains "valid store still pruned" "$out" "Outcome: pruned" +assert_contains "malformed sibling reported" "$out" "malformed plan record" +assert_contains "only the valid GITDIR counted" "$out" "failed=1" +assert_contains "malformed sibling adds no gitdir" "$out" "gitdirs=1" +assert_file_absent "valid REPO record still applied" "$AR7/bin/b" + # --- 5. skip list + unmatched skip --- out="$(bash "$BATCH" --tier caches --repo "$R1" "$R2" --skip r2 --skip nosuchrepo)" assert_contains "skip-listed repo skipped" "$out" "skip-list (r2)"