fix(repo-hygiene): git-tree-reset AppliedClean no longer claims success when git clean failed (#395)

[kyle-sexton]

Summary

git-tree-reset.sh --apply printed AppliedClean: git clean -fdx … unconditionally, regardless of whether git clean actually succeeded — a false success signal that misleads any operator or automation keying off that line to conclude the tree reached a known-good state.

Scope note: the sibling AppliedReset false-success and the unresolvable-@{u} gate that issue #395 references were already closed by #460 (reset-failure aborts clean, exit 5) and #542 (upstream-unresolved block, exit 6). This PR fixes the one remaining unguarded case — AppliedClean — so #395's acceptance criteria are fully met.

Fix

• Capture git clean -fdx's exit status (CLEAN_RC), read on the line immediately after the capture so $? isn't clobbered (the script runs set -uo pipefail, no -e).
• Gate the AppliedClean success line on the real outcome. A non-zero clean exit whose cause is locked/in-use files is the expected non-fatal case — git clean returns non-zero when it fails to remove any path, and that case is already reported honestly via Unremovable: — so it is not treated as failure (avoids regressing the deliberate locked-file handling on Windows). Only a non-zero exit with no failed to remove warnings is a genuine failure: it emits an explicit FAILED: line, AppliedClean: failed, and exits 7 instead of a success line.
• Emit the AppliedReset: success line as soon as reset --hard genuinely succeeds — before clean — so a subsequent clean failure still surfaces the truthful reset outcome rather than swallowing it.
• Run the reparse-point restore guard on the failure path too (data-loss guard: a clean that errored mid-run may still have deleted tracked files first; safe because reset --hard ran first).
• Documented exit 7 in the script header, usage(), and the clean skill's context/git-tree-reset.md gates list.

Verification

New test case #11 (git-tree-reset.test.sh) forces a genuine clean failure via a PATH shim that intercepts only git clean (delegating every other subcommand — crucially reset — to the real git, so the reset genuinely succeeds). The full suite passes, including the 6 new assertions:

PASS: [38] clean failure exits 7
PASS: [39] clean failure reports failure
PASS: [40] clean failure emits AppliedClean: failed
PASS: [41] clean failure emits no clean success line
PASS: [42] clean failure still reports the successful reset
PASS: [43] clean failure leaves untracked intact (clean did not silently succeed)
OK: git-tree-reset.sh tests passed

(All 43 tests pass; the pre-existing reset-failure suite #22–27 and upstream-unresolved suite #28–37 continue to pass — no regression. shellcheck is clean on both files.)

Live --apply run against a repo where git clean fails after a successful reset — the report is now honest and exits non-zero:

AppliedReset: git reset --hard main
FAILED: git clean -fdx exited 1 (non-locked-file cause) — untracked removal incomplete; reset --hard already applied.
fatal: simulated git clean failure
AppliedClean: failed
RestoredTracked: 0
EXIT=7
scratch.txt still present (clean did NOT silently succeed): YES

Before this fix the same run printed AppliedClean: git clean -fdx and exited 0.

Closes #395

Related

• fix(repo-hygiene): git-tree-reset prints Applied* success lines even when git commands failed #395 — this PR.
• fix(repo-hygiene): git-tree-reset dry-run doesn't validate @{u}, so dry-run success != apply success #396 — sibling issue on the same script (dry-run does not validate @{u} upstream). Not addressed here to avoid a merge collision on the same lines; a separate cycle picks it up.

🤖 Generated with a Claude Code implementation subagent (issue #395)

[claude]

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

---

PR Review — fix(repo-hygiene): git-tree-reset AppliedClean false-success

• Gather context (PR diff, changed files)
• Review git-tree-reset.sh (core logic change)
• Review git-tree-reset.test.sh (new test case fix(markdown-formatter): harden hook + simplify plumbing #11)
• Review docs / metadata (plugin.json, CHANGELOG.md, git-tree-reset.md)
• Assess security, correctness, and CLAUDE.md alignment
• Post findings

---

Verdict: Approve with observations. The fix is correct, the exit-code path is clean, and the tests genuinely exercise the failure path. A few minor observations below — none are blocking.

---

Correctness

CLEAN_RC capture (git-tree-reset.sh:232–233)

CLEAN_STDERR="$(git clean -fdx "${PRESERVE_ARGS[@]}" 2>&1 >/dev/null)"
CLEAN_RC=$?

The redirect order 2>&1 >/dev/null inside $(...) is correct: stderr flows to the command-substitution capture pipe (captured in CLEAN_STDERR) while stdout is discarded. CLEAN_RC is read on the immediately following line before $? can be clobbered — good.

Failure gate logic (git-tree-reset.sh:247)

if [[ "$CLEAN_RC" -ne 0 && "${UNREMOVABLE:-0}" -eq 0 ]]; then

Sound: non-zero exit with no locked-file warnings → genuine failure (exit 7). Non-zero exit with locked-file warnings → expected non-fatal case, falls through to the success path. The design intent is clear.

AppliedReset: before clean (git-tree-reset.sh:226) — emitting the success line before git clean ensures a subsequent clean failure can't swallow the truthful reset outcome. Good ordering.

Restore guard on failure path (git-tree-reset.sh:239) — guard runs unconditionally before the failure gate, so reparse-point casualties are recovered even when clean errors mid-run. Correct.

---

Observations (non-blocking)

1. Mixed locked + non-locked failure edge case

If git clean exits non-zero with both locked-file warnings and other unrelated errors (e.g. a permission denial on a non-locked path), UNREMOVABLE > 0 and the script falls through to the success path — suppressing the non-locked error. This is an existing design decision (the locked-file heuristic was in place before this PR), so not a regression, but worth a comment in the relevant block for the next reader.

2. Unremovable: absent on exit-7 path

The success path emits Unremovable: 0 (line 257). The failure path doesn't emit it at all — which is technically consistent since UNREMOVABLE=0 by definition when exit 7 is taken. But a consumer parsing AppliedClean: failed alongside a missing Unremovable: label may be surprised. Emitting Unremovable: 0 before exit 7 would make the API surface uniform. Fix this →

3. Potential blank stderr line when CLEAN_STDERR is empty

git-tree-reset.sh:249:

printf '%s\n' "$CLEAN_STDERR" >&2

If git clean exits non-zero but writes nothing to stderr, this emits a bare newline to stderr. Cosmetic, but [[ -n "$CLEAN_STDERR" ]] guard would tighten it.

4. Test #11 — missing RestoredTracked: assertion

The failure path emits RestoredTracked: %s (line 251) but the new test case doesn't assert it. The six assertions that exist are solid; this would just round out the contract. Minor coverage gap.

---

Test shim design

The git-clean-shim intercepts git clean but passes git clean -fdxn (dry-run) through ("\$*" != *-n*), which correctly avoids breaking the --dry-run code path if it were ever exercised. The shim emits to stderr (>&2) matching git's real behavior. Well designed.

---

CLAUDE.md / plugin conventions

• Repo-agnostic ✓ — no hardcoded paths introduced
• Versioned ✓ — 0.3.2 → 0.3.3 in plugin.json
• Exit-code table updated in header, usage(), and git-tree-reset.md ✓
• CHANGELOG entry is accurate and clearly scoped ✓
• Security: no new command injection vectors; all variables quoted/arrayed; grep -c ... || true correctly neutralizes pipefail on zero-match ✓

---

Summary: Solid fix. The false-success signal is gone, the failure surface is documented, and the test exercises the path end-to-end with a real git reset. The observations above are all minor; the PR is ready to merge as-is.

[claude]

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

---

PR Review — fix(repo-hygiene): git-tree-reset AppliedClean false-success

• Gather context (PR diff, changed files)
• Review git-tree-reset.sh (core logic changes)
• Review git-tree-reset.test.sh (new test case fix(markdown-formatter): harden hook + simplify plumbing #11)
• Review docs / metadata (plugin.json, CHANGELOG.md, git-tree-reset.md)
• Assess security, correctness, and CLAUDE.md alignment
• Post findings

---

Verdict: Approve. The two-commit series is correct. Commit 2256dc4 closes the false-success signal; commit 449c049 addresses three of the four observations from the prior round (blank-stderr guard, Unremovable: 0 on failure path, and the RestoredTracked:/Unremovable: test assertions). One prior observation (#1 — mixed locked+non-locked edge case) remains as a documented known limitation.

---

Correctness

CLEAN_RC capture (line 232–233)

CLEAN_STDERR="$(git clean -fdx "${PRESERVE_ARGS[@]}" 2>&1 >/dev/null)"
CLEAN_RC=$?

Redirect order is correct inside $(...): 2>&1 routes stderr to the capture pipe; >/dev/null then discards stdout. CLEAN_RC is read on the immediately following line before $? can be clobbered.

Failure gate (line 250)

if [[ "$CLEAN_RC" -ne 0 && "${UNREMOVABLE:-0}" -eq 0 ]]; then

Sound: non-zero exit + no locked-file warnings → genuine failure → exit 7. Non-zero exit + locked-file warnings → non-fatal, falls through to success path. The known-limitation comment (mixed locked+non-locked) is present and correctly deferred.

AppliedReset: before clean (line 226) — truthful reset outcome is always visible even when clean fails. Good ordering decision.

Restore guard runs unconditionally (line 239) — tracked-file recovery runs before the failure gate, so a partially-run clean that deleted tracked files mid-run is still corrected. Correct.

[[ -n "$CLEAN_STDERR" ]] guard (line 252) — blank-stderr observation addressed; no bare newline emitted when git clean exits non-zero silently.

Unremovable: 0 on failure path (line 255) — uniform apply-path API surface observation addressed.

---

Test case #11

Eight assertions (up from the six in the PR body, the 449c049 commit added RestoredTracked: and Unremovable: assertions):

• Exit code, FAILED: line, AppliedClean: failed, no success line, AppliedReset:, RestoredTracked: 0, Unremovable: 0, untracked file survival.

The $REAL_GIT variable is defined at line 120 (from test case #9) and remains in scope for test case #11 — no re-resolution needed. The heredoc delimiter SHIMEOF is unquoted, so $REAL_GIT is correctly expanded at write-time and embedded as a literal path in the shim.

The shim's dry-run pass-through ("\$*" != *-n*) correctly avoids breaking the --dry-run code path.

---

Minor observation (non-blocking)

RestoredTracked: N warning suppressed on exit-7 path — the success path emits a WARNING: restored N tracked file(s)... message when RESTORED > 0 (line 263–265), but the exit-7 path doesn't. If the restore guard fires on the failure path (tracked files recovered after a mid-run clean error), the output would include RestoredTracked: N with no accompanying warning explaining what was recovered. Very edge case; if deferred, a code comment in the failure branch would help future readers. Fix this →

---

CLAUDE.md / plugin conventions

• Repo-agnostic ✓ — no hardcoded paths
• Versioned ✓ — 0.3.2 → 0.3.3 in plugin.json
• Exit-code table updated in header, usage(), and git-tree-reset.md ✓
• CHANGELOG entry accurate and clearly scoped ✓
• Security: no new injection vectors; all variables quoted/arrayed; grep -c ... || true correctly neutralizes pipefail ✓

---

Ready to merge.

[kyle-sexton]

Review observation classification

Thanks for the thorough review. Verdict was "Approve with observations"; all four are triaged below. Applied in 449c0495.

#	Finding	Severity	Classification	Action
2	Unremovable: label absent on the exit-7 (clean-failure) path — success path emits it, failure path did not	Minor (API-surface uniformity)	Fixed	Added printf 'Unremovable: %s\n' "0" before exit 7, so every apply-path outcome emits the same label set. Test #11 now asserts it.
3	Bare newline to stderr when git clean fails with empty CLEAN_STDERR	Cosmetic	Fixed	Guarded the echo: [[ -n "$CLEAN_STDERR" ]] && printf '%s\n' "$CLEAN_STDERR" >&2.
4	Test #11 missing a RestoredTracked: assertion for the failure path	Minor (coverage gap)	Fixed	Added RestoredTracked: 0 and Unremovable: 0 assertions (suite now 45 assertions, all green).
1	Mixed locked + non-locked clean failure falls through to the success path (UNREMOVABLE>0 wins), suppressing the unrelated error	Minor (pre-existing design)	Non-blocking, deferred	The locked-file heuristic predates this PR and this is not a regression, so a stricter classifier is out of scope here. Documented the limitation inline at the gate so the next reader sees the trade-off; a follow-up can tighten it if the mixed case is observed in practice.

Verification after the fix: full suite passes (OK: git-tree-reset.sh tests passed, 45/45), shellcheck clean on both files.

🤖 Generated with a Claude Code implementation subagent (issue #395)

[claude]

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

---

PR Review — fix(repo-hygiene): git-tree-reset AppliedClean false-success

• Gather context (PR diff, changed files)
• Review git-tree-reset.sh (core logic changes)
• Review git-tree-reset.test.sh (new test case fix(markdown-formatter): harden hook + simplify plumbing #11)
• Review docs / metadata (plugin.json, CHANGELOG.md, git-tree-reset.md)
• Assess security, correctness, and CLAUDE.md alignment
• Post findings

---

Verdict: Approve. This is the third pass over this branch; prior rounds (commits 2256dc4 and 449c049) closed all four observations. I read the code independently and my analysis agrees with the prior verdicts. The fix is correct, the test exercises the real failure path, and the API surface is consistent. Notes below.

---

Correctness

CLEAN_RC capture (lines 232–233)

CLEAN_STDERR="$(git clean -fdx "${PRESERVE_ARGS[@]}" 2>&1 >/dev/null)"
CLEAN_RC=$?

Redirect order inside $(...) is correct: 2>&1 routes stderr to the pipe that $() captures; >/dev/null then discards stdout. Net: stderr is captured in CLEAN_STDERR, stdout is discarded. CLEAN_RC is read on the immediately following line, before $? can be clobbered — sound.

PRESERVE_ARGS expansion with set -u (line 232): PRESERVE_ARGS is always populated by mapfile -t at line 152 (empty arrays don't trigger -u), so the [@] expansion is safe even when no preserve args apply.

UNREMOVABLE via grep (line 234): grep -c 'failed to remove' || true correctly neutralizes pipefail on zero-match. A silent git clean failure (CLEAN_STDERR empty, CLEAN_RC non-zero) still yields UNREMOVABLE=0 and correctly enters the exit-7 path.

Failure gate logic (line 250): CLEAN_RC -ne 0 && UNREMOVABLE -eq 0 → exit 7. The known-limitation comment at lines 241–249 is clear about the mixed locked+non-locked edge case (pre-existing heuristic, not a regression).

AppliedReset: before clean (line 226): truthful reset outcome is emitted before clean runs, so a later clean failure cannot swallow it.

Restore guard before the failure gate (line 239): guard runs unconditionally — tracked files deleted mid-run by a partially-executed clean are still recovered on both the success and failure paths.

Blank-stderr guard (line 252): [[ -n "$CLEAN_STDERR" ]] && printf '%s\n' "$CLEAN_STDERR" >&2 — no bare newline when git clean fails silently.

Unremovable: 0 on failure path (line 255): uniform apply-path label set regardless of outcome.

---

Test case #11

Eight assertions cover the full contract of the exit-7 path:

Assertion	What it proves
Exit code 7	Correct error classification
FAILED: git clean -fdx	Failure line present
AppliedClean: failed	No false-success label
No AppliedClean: git clean	False-success line definitively absent
AppliedReset: git reset --hard	Reset outcome survives clean failure
RestoredTracked: 0	Restore guard ran; output label present
Unremovable: 0	Uniform API surface label present
Untracked file survives	Clean did not silently succeed

The shim (lines 219–227) intercepts only non-dry-run git clean calls ("\$*" != *-n*) and delegates everything else — crucially reset — to $REAL_GIT, so the reset is real and its success output is genuine. $REAL_GIT was resolved at line 120 and the unquoted heredoc delimiter correctly expands it into the shim at write-time.

---

Residual observation (non-blocking, not new)

WARNING: restored N tracked file(s) absent on exit-7 path — The success path emits a warning when RESTORED > 0 (lines 263–265); the failure path doesn't. If the restore guard fires during a mid-run clean failure, the output will contain RestoredTracked: N with no accompanying warning. Very edge-case — the test verifies RestoredTracked: 0 for the simulated path. A code comment in the failure branch would help future readers. Fix this →

---

CLAUDE.md / plugin conventions

• Repo-agnostic ✓ — no hardcoded paths
• Versioned ✓ — 0.3.2 → 0.3.3 in plugin.json
• Exit-code table updated in header, usage(), and git-tree-reset.md ✓
• CHANGELOG entry accurate, clearly scoped, and complete ✓
• Security: no new injection vectors; all variables quoted/arrayed ✓

---

Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 449c04954b

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