fix(repo-hygiene): emit RESTORED>0 warning on git-tree-reset exit-7 failure path (#605)

[kyle-sexton]

Summary

On the git-tree-reset.sh exit-7 clean-failure path (git clean -fdx failed for a non-locked-file cause), the script emitted the machine-readable RestoredTracked: N line but skipped the human-visible WARNING: restored N tracked file(s) message that the success path already prints when RESTORED>0. An operator hitting the failure path could therefore miss that the reparse-point restore guard fired — i.e. that data-loss recovery mutated the working tree back — even though a clean that errors mid-run may have deleted tracked files before failing.

Fix

Mirror the success path's warning onto the exit-7 failure path: under the same RESTORED>0 condition, emit the same WARNING: restored N tracked file(s) deleted via reparse-point traversal (junction/symlink into tracked dir). message (identical wording, same stderr stream) at the point where RestoredTracked: N is emitted alone. Both paths now surface restore-guard activity identically for this signal. No other behavior changes.

Verification

Added test case #12 to git-tree-reset.test.sh. It drives the exit-7 path with a git shim whose faked clean deletes a tracked file (simulating a partial clean that deleted tracked files via reparse-point traversal) and then exits non-zero, so git ls-files --deleted reports the file and the restore guard recovers it (RESTORED=1) on the failure path. It asserts exit 7, RestoredTracked: 1, and that the warning now appears.

Full suite (50 assertions) passes:

PASS: [46] clean failure with restore exits 7
PASS: [47] clean failure emits AppliedClean: failed
PASS: [48] clean failure reports a positive RestoredTracked count
PASS: [49] clean failure path emits the RESTORED>0 warning (parity with success path)
PASS: [50] restore guard recovered the tracked file on the failure path
OK: git-tree-reset.sh tests passed

Negative check — stashing only the source fix and re-running proves the new test is meaningful (it fails without the fix):

FAIL: [49] clean failure path emits the RESTORED>0 warning (parity with success path) — expected WARNING:\ restored\ 1\ tracked\ file\(s\)... in: Upstream: main
FAILED: 1 test(s)

shellcheck on both the script and the test reports no findings.

Closes #605

Related

• fix(repo-hygiene): git-tree-reset exit-7 failure path emits RestoredTracked: N without the RESTORED>0 warning message the success path prints #605 — this fix
• fix(repo-hygiene): git-tree-reset prints Applied* success lines even when git commands failed #395 — family: the AppliedClean false-success gate that introduced the exit-7 failure path this warning-parity fix completes
• fix(repo-hygiene): git-tree-reset mixed locked+non-locked clean failure still falls through to AppliedClean success #602 — sibling follow-up from the same fix(repo-hygiene): git-tree-reset AppliedClean no longer claims success when git clean failed (#395) #590 merge digest (mixed locked + non-locked clean-failure classifier; out of scope here, left as its own deferred issue)

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

[claude]

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

---

PR Review

• Gather context and read changed files
• Review git-tree-reset.sh fix
• Review git-tree-reset.test.sh test coverage
• Review plugin.json version bump
• Review CHANGELOG.md
• Summarize findings

---

Summary

This is a clean, tightly-scoped fix. The 8-line addition to git-tree-reset.sh exactly mirrors the existing success-path warning onto the exit-7 failure path. No behavioral regressions, no new state mutations, well-tested. Recommend merge.

---

Fix correctness — git-tree-reset.sh

The new block at lines 261–263:

if [[ "${RESTORED:-0}" -gt 0 ]]; then
  printf 'WARNING: restored %s tracked file(s) deleted via reparse-point traversal (junction/symlink into tracked dir).\n' "$RESTORED" >&2
fi

is byte-for-byte identical in condition, message format, and stream (>&2) to the success path at lines 271–273. The placement — after the machine-readable output lines, before exit 7 — is correct; the machine-readable RestoredTracked: N lands first for parsers, then the human-visible WARNING follows for operators. No injection risk: $RESTORED is always an integer produced by clean_restore_tracked_deletions, never external input.

The comment block (4 lines) is more verbose than the repo's default style, but the cross-path invariant it explains is genuinely non-obvious and the comment is accurate.

---

Test coverage — git-tree-reset.test.sh

Test #12 (lines 240–279) is well-targeted:

• Shim design is sound. The shim intercepts only clean (non-dry-run, \$* != *-n*) and delegates ls-files, checkout, fetch, and reset to REAL_GIT (defined at line 120, confirmed present). This means clean_restore_tracked_deletions (which calls git ls-files --deleted and git checkout -- <file>) correctly runs against real git and genuinely restores the tracked file.

• Assertion on the WARNING correctly works. The test captures both stdout and stderr into $out via 2>&1, so assert_contains … "WARNING: restored 1 tracked file(s)" will find the stderr warning. Consistent with test fix(markdown-formatter): harden hook + simplify plumbing #11's approach.

• assert_file_exists on $R5/tracked.txt is the key behavioral assertion — it proves the restore guard actually ran and the file is back, not just that the warning was printed.

🟡 Nit: Test #12 doesn't assert assert_not_contains "... AppliedClean: git clean" (verifying no false-success line appears on the failure path). This is already covered by test #11, so the omission is intentional and reasonable — just noting it for completeness.

---

Version bump and CHANGELOG

• 0.4.0 → 0.4.1: correct patch bump for a pure bug fix with no API/behavior change outside the warning gap.
• CHANGELOG entry is accurate, references the issue, and explains both the before and after behavior clearly.

---

No blocking issues found. Ready to merge.

[kyle-sexton]

Post-green review classification

No findings requiring action. Reviewer confirmed the fix is byte-for-byte identical to the success path's existing warning pattern, correctly placed, no injection risk, well-tested (test #12 asserts both the warning text and the actual file restoration). One noted-but-intentional test omission (already covered elsewhere).

CI green (16/16). Flipping to ready for review.

[claude]

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

---

PR Review

• Read changed files and diff
• Review git-tree-reset.sh fix
• Review git-tree-reset.test.sh test coverage
• Review plugin.json version bump
• Review CHANGELOG.md
• Summarize findings

---

Summary

Clean, tightly-scoped fix with good test coverage. The 8-line addition exactly mirrors the existing success-path warning onto the exit-7 failure path with no other behavioral changes. Recommend merge.

---

Fix correctness — git-tree-reset.sh lines 261–264

if [[ "${RESTORED:-0}" -gt 0 ]]; then
  printf 'WARNING: restored %s tracked file(s) deleted via reparse-point traversal (junction/symlink into tracked dir).\n' "$RESTORED" >&2
fi

This is byte-for-byte identical in condition, format string, and stream (>&2) to the success-path block at lines 271–273. Placement is correct: after the machine-readable RestoredTracked: N line (parsers first), before exit 7. No injection risk: $RESTORED is always an integer produced by clean_restore_tracked_deletions at line 239, never external input, and %s is used as the format specifier.

The 4-line comment block is more verbose than the repo's default style, but the cross-path invariant it explains is genuinely non-obvious and worth the prose.

---

Test coverage — git-tree-reset.test.sh test #12

Shim design is correct. The shim intercepts only non-dry-run clean calls ($* != *-n*), delegates all other subcommands (ls-files --deleted, checkout -- <file>, reset, fetch) to $REAL_GIT. This means clean_restore_tracked_deletions runs against real git and genuinely restores the file.

The $R5 expansion inside the heredoc is intentional and correct — <<SHIMEOF (no quoting) expands $R5 to the absolute path at shim-write time, consistent with the shim patterns in tests #9 and #11.

All 5 assertions are meaningful:

• assert_exit 7 — verifies exit code
• assert_contains "AppliedClean: failed" — verifies no false-success line
• assert_contains "RestoredTracked: 1" — verifies machine-readable count
• assert_contains "WARNING: restored 1 tracked file(s)..." — directly targets the parity fix; this assertion fails without the source change (as shown by the negative check in the PR body)
• assert_file_exists "$R5/tracked.txt" — behavioral proof that restore guard actually ran

🟡 Nit (non-blocking): REAL_GIT is first assigned in the test #9 setup block at line 120 and relied upon in test #12. This ordering dependency is pre-existing (test #11 shares it) and not introduced by this PR, but a future reorder of test cases would silently break test #12's shim. Worth noting as technical debt.

---

Version bump and CHANGELOG

• 0.4.0 → 0.4.1: correct patch semver for a pure bug fix with no API or behavioral change outside the warning gap.
• CHANGELOG entry is accurate, references the issue, and explains both the before and after behavior clearly.

---

No blocking issues found. Ready to merge.

[kyle-sexton]

Post-green review classification (round 2)

Second reviewer pass (job run 29729803370, 09:00:35Z) also lands no blocking findings — recommend merge, confirming the earlier classification.

Its one non-blocking nit differs from round 1: it flags that REAL_GIT is first assigned in the test #9 setup block and relied upon by test #12's shim, an ordering dependency a future test reorder could silently break. The reviewer itself notes this is pre-existing (test #11 shares it) and not introduced by this PR — technical debt in the existing test harness, out of scope for this tightly-scoped parity fix. No action required on this branch.

Head unchanged (15aee31). No unresolved review threads. Nothing requiring action.