fix(repo-hygiene): clean-build --apply honors --include-caches gate

[kyle-sexton]

This was generated by AI during work-loop execution.

Closes #1076

Summary

clean-build.sh --apply accepted caches-tier manifest entries even without
--include-caches: both apply paths (the resume-from-manifest early return
and the normal build-then-apply flow) passed a hardcoded "build caches"
allow-list to clean_apply_manifest regardless of the flag, so a build-only
apply could still remove cache targets from a caller-supplied or stale
manifest — defeating the tier-isolation guard on the documented --manifest
surface.

The allowed classes are now derived from the apply invocation's own
--include-caches flag (build only when unset, build caches when set).
A build-only apply now rejects a caches manifest line as
Rejected (wrong tier) and leaves the target in place.

Caller-visible: the tier gate now tracks the apply call's own flag, not
whatever built the manifest. The documented SKILL.md §3 build-tier flow
(clean-build.sh --include-caches at dry-run, then apply the same manifest)
must repeat --include-caches on the apply step too, or the folded-in
caches entries get rejected instead of removed. clean-batch.sh already
passed --include-caches correctly on its build-tier apply, so it needed no
change. SKILL.md and clean-build.sh's usage text are updated to document
this explicitly.

Test plan

• Added a TDD regression case to clean-build.test.sh: a build-only
  --apply --manifest <path> against a manifest containing a caches line
  now asserts Rejected (wrong tier): .pytest_cache, failed=1, exit 1, and
  the cache target left in place; the same manifest line is accepted once
  --include-caches is added.
• Verified the new case is a real regression guard, not a tautology: stashed
  the clean-build.sh fix and reran the suite — 5 assertions failed
  (including the new case) against the pre-fix script; restored the fix and
  reran clean — all pass.
• Updated the pre-existing manifest round-trip test (dry-run
  --include-caches → apply) to also pass --include-caches on its apply
  calls, since that round trip now needs it to remove both tiers.
• bash plugins/repo-hygiene/skills/clean/scripts/clean-build.test.sh — 27/27
  pass.
• bash plugins/repo-hygiene/skills/clean/scripts/clean-batch.test.sh — all
  pass (no regression in the batch caller that already passes
  --include-caches correctly).
• bash plugins/repo-hygiene/skills/clean/scripts/clean-caches.test.sh — all
  pass (unaffected sibling script, checked for regressions).
• shellcheck clean on the modified clean-build.sh.

Related

N/A

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 2m 12s —— View job

---

Code Review

• Gather context and understand the PR
• Review clean-build.sh — core fix logic
• Review clean-build.test.sh — regression test coverage
• Review SKILL.md — documentation update
• Review plugin.json — version bump
• Review CHANGELOG.md — changelog entry
• Post findings

---

Summary

This is a well-targeted fix. The root cause was correctly identified: both --apply --manifest paths in clean-build.sh called clean_apply_manifest with a hardcoded "build caches" allow-list, bypassing clean_apply_manifest's own wrong-tier gate when a caller-supplied or stale manifest carried caches lines without --include-caches being set. The fix is minimal (one variable, two substitutions), correctly placed before both apply paths, and well-tested.

---

Findings

clean-build.sh — Core fix

Lines 86–91: Comment block is longer than necessary

The comment is 5 lines and re-states the problem the fix solves in detail. The closing part (otherwise a build-only apply (no --include-caches) would still accept caches lines…) is already described in the CHANGELOG and SKILL.md. A shorter inline note pointing at the invariant is enough:

# Derived once; both apply paths must use this — never a fixed "build caches",
# or a build-only apply still accepts caches lines from a caller-supplied manifest.
ALLOWED_CLASSES="build"
[[ "$INCLUDE_CACHES" -eq 1 ]] && ALLOWED_CLASSES="build caches"

Minor; the current wording is correct, just verbose for production code. permalink

Lines 132 (no-manifest --apply path): Fix is consistent, though this path wasn't actually buggy

When --apply runs without --manifest, clean_plan only writes caches entries to the in-memory manifest when INCLUDE_CACHES=1 (because clean_add_candidates caches … is gated on that flag). So the hardcoded "build caches" on this path never accepted caches entries a plan didn't already write. The substitution to $ALLOWED_CLASSES is still the right call — it makes the two paths consistent and future-proof — but the CHANGELOG entry overstates the impact slightly by saying both paths were "bypassing" the guard. The --manifest path was the actual attack surface; the no-manifest path was a latent inconsistency. permalink

clean-build.test.sh — Regression tests

Overall: The new b3 regression test is solid. It covers rejection message, failed=1 in the summary, exit code, and file left in place — the four things that matter for the guard. The positive case (same manifest accepted with --include-caches) completes the pair.

Lines 103, 132: Resume and positive-apply lack exit-code assertions

The resume test (run_b2 --apply --include-caches --manifest "$MANI") captures $rc and asserts the exit, but the b3 positive-apply call at line 132 doesn't. Low-stakes since the file-absence assertion would fail anyway on a non-zero exit, but adding rc=$? + assert_exit would be consistent with every other apply assertion in the file. permalink

SKILL.md

Line 106: Appended sentence is correct but makes an already-long paragraph denser

The added sentence correctly documents the breaking change and is bolded for visibility. No correctness issue. A future cleanup might split this paragraph into a brief flow block + a build-tier note: callout, but that's out of scope here.

CHANGELOG.md and plugin.json

Both are correct. Version bump 0.7.1 → 0.7.2 is appropriate for a bug fix. The CHANGELOG entry is detailed and calls out the caller-visible behavior change clearly. The slight overstatement about the no-manifest path is minor.

---

Verdict

Approve. The fix is correct and minimal. The test regression guard is thorough (the PR description confirms it fails against the pre-fix script). Documentation and CHANGELOG are accurate. The notes above are minor polish items, not blockers.

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

Security Review

• Gather PR diff and changed files
• Review clean-build.sh for injection surfaces, logic flaws, path traversal
• Review clean-build.test.sh for test integrity issues
• Review plugin.json and SKILL.md for config/contract changes
• Synthesize and report findings

---

Findings

SUGGESTION · Low confidence — Unquoted $* in run_b3() shell wrapper

File: plugins/repo-hygiene/skills/clean/scripts/clean-build.test.sh L120–122

run_b3() {
  bash -c "cd '$TEST_TMPDIR/b3' && bash '$BUILD' $*"
}

$* is expanded unquoted inside the double-quoted bash -c "…" string. Any positional parameter that contains shell metacharacters (;, &&, $(…), etc.) would be interpreted as shell syntax rather than arguments — allowing command injection in the child shell. Similarly, a single quote inside $TEST_TMPDIR or $BUILD would break the surrounding '…' quoting.

In practice the immediate risk is very low: all call-sites pass only hardcoded string constants (--apply, --manifest, --include-caches) and paths produced by mktemp -d (which on Linux generates alphanumeric-only suffixes). But the same structural flaw exists in the pre-existing run_build() and run_b2() wrappers and this PR adds a third copy rather than fixing the pattern. If these wrappers are ever extended to accept test-supplied or environment-derived strings the latent surface becomes live.

The idiomatic fix is:

run_b3() {
  bash -c "cd %q && bash %q \"\$@\"" _ "$TEST_TMPDIR/b3" "$BUILD" -- "$@"
}
# or more readably:
run_b3() {
  (cd "$TEST_TMPDIR/b3" && bash "$BUILD" "$@")
}

The subshell form ((cd … && …)) eliminates the bash -c quoting problem entirely and is already safe.

---

Clean areas

Core fix (clean-build.sh): The ALLOWED_CLASSES derivation is correct and closes the tier-bypass as intended. INCLUDE_CACHES is always 0 or 1 (integer literal, never user-supplied data), so ALLOWED_CLASSES is always one of exactly two known string values — no injection surface. Both apply paths (--apply --manifest and the normal walk-then-apply path) now use the same derived value, which is the right fix.

clean_apply_manifest gate (pre-existing, unchanged): The tier check at line 595 of clean-common.sh (" $allowed " != *" $class "* ) is a bash glob, not a rm argument, and $class values are already restricted to caches or build by clean_manifest_writable_target's regex whitelist. No widening introduced.

Manifest input validation (pre-existing, unchanged): clean_manifest_rel_safe, clean_path_has_pruned_segment, clean_path_has_reparse_ancestor, and clean_manifest_target_valid all remain in place as guards before any rm -rf. This PR does not touch those guards and does not introduce new paths to rm -rf.

plugin.json, CHANGELOG.md, SKILL.md: Version bump, changelog entry, and documentation update only — no new permissions, no new config keys, no new MCP surface, no supply-chain changes.

Test regression guard: The new test fixture (b3) correctly exercises the wrong-tier rejection path. The manifest is written with printf to a known path under $TEST_TMPDIR, so there is no risk of the manifest content influencing the shell execution context.

---

No critical or important security issues. One suggestion on a latent shell-injection pattern in the new test wrapper; the fix itself is sound.