fix(repo-hygiene): gate git-tree-reset clean on reset success#460
Conversation
The --apply path runs under `set -uo pipefail` (no -e). It ran `git reset --hard "$UPSTREAM"` without checking its exit status, then unconditionally ran `git clean -fdx`. A failed reset dropped straight through to the destructive clean, leaving the tree cleaned but not reset (a partial destructive op). Gate the clean and restore guard on a successful reset: on non-zero reset, print an explicit failure line, emit honest `AppliedReset: failed` / `AppliedClean: none` (no success lines), and exit 5. Document the new exit code in the header, --help, and the tree context doc. Regression test forces a reset failure via a PATH git shim and asserts clean did not run (untracked file survives, no clean success line, exit 5). Verified red against the pre-fix script, green after. Closes #394 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 1m 45s —— View job Code Review
Overall: The fix is correct, minimal, and well-scoped. The
|
| Area | Status |
|---|---|
| Core fix correctness | ✅ Sound |
| Exit code documentation (3 places) | ✅ Complete |
| stdout/stderr split in failure path | ✅ Consistent |
| Regression test shim design | ✅ Correct |
| Positive assertions for honest labels |
No blocking issues. The two missing positive assertions in case 9 are the only improvements worth considering before merge.
Per-change version + changelog convention: record the git-tree-reset clean-gating fix as a 0.3.1 Fixed entry and bump the plugin manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 1m 38s —— View job Code Review
Overall: The fix is correct, minimal, and well-scoped. The version bump commit is clean and appropriate. One pre-existing minor gap from the earlier review pass remains open (two missing positive assertions in case 9) — no new concerns introduced by the
|
| Area | Status |
|---|---|
| Core fix correctness | ✅ Sound |
| Exit code documentation (3 places) | ✅ Complete |
| stdout/stderr split in failure path | ✅ Consistent |
| Regression test shim design | ✅ Correct |
| Positive assertions for honest labels | |
Version bump (0.3.1) |
✅ Correct |
| CHANGELOG entry | ✅ Accurate |
No blocking issues. The two missing positive assertions in case 9 are the only improvements worth considering before merge.
…se 9 Add positive assertions that a failed reset --hard emits the machine-readable labels AppliedReset: failed and AppliedClean: none, completing the contract check the four existing assertions only covered by absence.
|
Claude finished @kyle-sexton's task in 1m 28s —— View job Code Review
Overall: The fix is correct, minimal, and well-scoped. The latest commit (
|
| Assertion | What it verifies |
|---|---|
assert_exit … 5 |
Script aborts with the documented exit code |
assert_contains … "FAILED: git reset --hard" |
Human-readable failure message present |
assert_contains … "AppliedReset: failed" |
Honest machine-readable reset label ✅ (added in 2f50cb1) |
assert_not_contains … "AppliedClean: git clean" |
No false success label for clean |
assert_contains … "AppliedClean: none" |
Honest machine-readable clean label ✅ (added in 2f50cb1) |
assert_file_exists … "scratch.txt" |
Clean did not run (untracked file survived) |
context/git-tree-reset.md — documentation
Placed correctly between the unpushed-commits guard and the restore guard. Wording is accurate and matches the script header and --help. Good.
plugin.json + CHANGELOG.md — version bump (0.3.0 → 0.3.1)
Semver patch is the correct choice for a bug fix. The CHANGELOG entry accurately describes the failure mode, the fix, the honest labels, and exit 5 in a style consistent with existing entries.
Summary
| Area | Status |
|---|---|
| Core fix correctness | ✅ Sound |
| Exit code documentation (3 places) | ✅ Complete |
| stdout/stderr split in failure path | ✅ Consistent |
| Regression test shim design | ✅ Correct |
| Positive assertions for honest labels | ✅ Complete (resolved in 2f50cb1) |
Version bump (0.3.1) |
✅ Correct |
| CHANGELOG entry | ✅ Accurate |
No blocking issues. No suggestions for improvement — the prior review's open point has been addressed.
|
Addressing both Code Review passes (runs 29681230062 and 29681298531) — both carry the same two positive-assertion gaps in
Both assertions added in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f50cb17f9
ℹ️ 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".
…tomic) A failed git reset --hard is not atomic: it can partially modify or remove tracked files before exiting non-zero. The prior message claimed "no files removed," implying an untouched worktree. Report that git clean was skipped and that the reset may have partially modified the tree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 46s —— View job Code Review
Overall: This pass reviews only the final commit (
|
| Assertion | What it verifies |
|---|---|
assert_exit … 5 |
Script exits with documented code |
assert_contains … "FAILED: git reset --hard" |
Human-readable failure message present |
assert_contains … "AppliedReset: failed" |
Honest machine-readable reset label |
assert_not_contains … "AppliedClean: git clean" |
No false success label for clean |
assert_contains … "AppliedClean: none" |
Honest machine-readable clean label |
assert_file_exists … "scratch.txt" |
Clean did not run (untracked file survived) |
The test does not assert the exact wording of the updated failure message beyond the "FAILED: git reset --hard" prefix — that is fine, the behavioral contract (exit 5, clean skipped, honest labels) is fully asserted.
Summary
| Area | Status |
|---|---|
| Core fix correctness | ✅ Sound |
Exit code documentation (header, --help, context doc) |
✅ Complete |
| Honest failure message (non-atomicity caveat) | ✅ Correct (745def6) |
| stdout/stderr split in failure path | ✅ Consistent |
| Regression test shim design | ✅ Correct |
| Positive assertions for honest labels | ✅ Complete |
Version bump (0.3.1) + CHANGELOG |
✅ Accurate |
context/git-tree-reset.md non-atomicity doc |
No blocking issues. One cosmetic doc gap noted, no code changes warranted.
|
Re: latest review at
No open blocking items. PR remains ready to merge at |
|
Follow-up on the deferred |
…r reset --hard @{u} (#542)
## Summary
`plugins/repo-hygiene/skills/clean/scripts/git-tree-reset.sh` (the
`tree` action of `/repo-hygiene:clean`) did a **partial destructive op**
on a branch whose upstream is configured but whose remote-tracking ref
is absent — e.g. a feature branch whose squash-merged PR left the remote
branch deleted and pruned.
When `refs/remotes/<remote>/<branch>` is gone, `git rev-parse
--abbrev-ref '@{u}'` prints the **literal token** `@{u}` instead of a
ref name, and the trailing `| tr -d '\r' || true` pipe masks git's
non-zero exit — so the non-empty guard passes and `UPSTREAM=@{u}`. On
`--apply` this reached `git reset --hard @{u}` → `fatal: ambiguous
argument '@{u}'`, and (before the reset-success gate #460) `git clean
-fdx` still ran, leaving the tree cleaned but not reset.
## Fix
Add a first-class resolution gate immediately after the existing
no-upstream check, **before any label output or destructive op**. It
verifies `@{u}` names a real, resolvable ref and, if not, skips the
repo:
```sh
if ! git rev-parse --verify --quiet '@{u}' >/dev/null 2>&1; then
UNRESOLVED_REMOTE="$(git config "branch.${CURRENT_BRANCH}.remote" 2>/dev/null | tr -d '\r' || true)"
UNRESOLVED_MERGE="$(git config "branch.${CURRENT_BRANCH}.merge" 2>/dev/null | tr -d '\r' || true)"
printf 'Blocked: upstream-unresolved (%s/%s)\n' "${UNRESOLVED_REMOTE:-?}" "${UNRESOLVED_MERGE#refs/heads/}"
printf 'PlannedReset: none\n'
printf 'PlannedClean: none\n'
exit 6
fi
```
- Verifies `@{u}` **directly** (the actual reset target), robust against
the literal-token quirk regardless of what `--abbrev-ref` echoed.
- A local-only upstream (`branch.<name>.remote="."`) still resolves to
its local branch → gate falls through → **resolvable common case is
unchanged** (existing dry-run / apply / default-branch / unpushed /
reset-fail tests all still pass).
- Distinct exit code `6` (vs exit `2` "no upstream"): different
remediation — configured-but-remote-ref-gone (`fetch --prune` / delete
branch) vs never-configured (`push -u`). Mirrors #460's convention of a
distinct code per gate.
- No destructive command (`reset`/`clean`) can run on the unresolved
path — the gate returns before the dry-run preview and before the apply
block.
Also bumps `repo-hygiene` `0.3.1` → `0.3.2` (patch, bug), adds the
CHANGELOG entry, extends the header exit map + `git-tree-reset.md` gate
list, and adds a regression test.
## Verification
**Lint (all clean):**
- `shellcheck --rcfile .shellcheckrc` on `git-tree-reset.sh` and
`git-tree-reset.test.sh` → both clean.
- `typos`, `editorconfig-checker`, `markdownlint-cli2` on changed md +
json → 0 findings.
**Test suite:** `git-tree-reset.test.sh` → 37/37 pass, including the new
pruned-upstream case asserting exit 6, the `Blocked:` message,
`PlannedReset`/`PlannedClean: none`, no `AppliedReset`/`AppliedClean`
line, and the untracked file surviving.
**Empirical before/after** (real bare remote; `feat/gone` pushed, remote
branch deleted + pruned; `--dry-run` to sidestep the reset-guard hook):
BEFORE (pre-fix script) — the smoking gun:
```text
Upstream: @{u}
Blocked: none
PlannedReset: git reset --hard @{u}
PlannedClean: git clean -fdx (+18 preserve excludes)
--- clean preview (git clean -fdxn, default-preserve applied) ---
Would remove scratch.txt
exit=0
```
On `--apply` this hits `fatal: ambiguous argument '@{u}'`.
AFTER (fixed script) — no destructive op planned or run:
```text
Blocked: upstream-unresolved (origin/feat/gone)
PlannedReset: none
PlannedClean: none
exit=6
```
`scratch.txt` still present: yes.
## Related
Closes #393.
- Source: #393.
- Companions in the same `git-tree-reset.sh` gate family: #394, #460
(reset-success gate, merged — this PR coordinates with it: `clean` never
runs when the reset precondition fails), #395, #396. This PR is scoped
strictly to the `@{u}`-resolution gate; the others are noted here, not
touched.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ss when git clean failed (#395) (#590) ## 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 - #395 — this PR. - #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) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…-5 gate (#485) (#670) ## Summary The exit-5 gate bullet in `plugins/repo-hygiene/skills/clean/context/git-tree-reset.md` accurately described the *gating* contract but was incomplete relative to the runtime stderr message: it did not tell a doc-only reader that a failed `reset --hard` may have partially modified tracked files before it failed. This is a one-line doc-consistency fix (patch bump `0.4.1` → `0.4.2`). ## Fix Added a parenthetical to the exit-5 bullet so the context doc is consistent with the exit-5 stderr message the script already emits. ## Verification **Doc — before (`git-tree-reset.md`):** > - Aborts the apply if `reset --hard` fails (exit 5) — `clean` and the restore guard never run, so a failed reset can never leave the tree cleaned but not reset. **Doc — after:** > - Aborts the apply if `reset --hard` fails (exit 5) — `clean` and the restore guard never run, so a failed reset can never leave the tree cleaned but not reset (the reset itself may have partially modified tracked files, since `reset --hard` is not atomic). **Script stderr it stays consistent with (`skills/clean/scripts/git-tree-reset.sh`, exit-5 path):** ```sh if ! git reset --hard "$UPSTREAM"; then printf 'FAILED: git reset --hard %s exited non-zero — aborting apply; git clean skipped. Note: reset --hard is not atomic and may have partially modified tracked files.\n' "$UPSTREAM" >&2 ... exit 5 ``` The added parenthetical ("the reset itself may have partially modified tracked files, since `reset --hard` is not atomic") mirrors the script's stderr wording ("reset --hard is not atomic and may have partially modified tracked files"). No behavioral change; docs only. Closes #485 ## Related - Follow-up to #485 (this issue). - Deferred from PR #460 (fixes #394) — the parenthetical was flagged in the #460 review but left unfixed to preserve that PR's merge-ready head SHA (`745def6`, the commit that added the stderr caveat). 🤖 Generated with a Claude Code implementation subagent (issue #485) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Problem
plugins/repo-hygiene/skills/clean/scripts/git-tree-reset.shruns its--applypath underset -uo pipefail(no-e). It executedgit reset --hard "$UPSTREAM"(~L184) without checking the exit status, then unconditionally rangit clean -fdx(~L187). A failed reset fell straight through to the destructive clean — the working tree got cleaned but not reset (a partial destructive op), and success lines were emitted for a command that failed.Fix
Gate
clean(and the reparse-point restore guard) on a successful reset:reset --hardexit status; on non-zero, skip clean and the restore guard, print an explicitFAILED:line, andexit 5.AppliedReset: failed/AppliedClean: none— never a success line for a command that failed.5in the script header,--help, andcontext/git-tree-reset.md.Scoped strictly to the reset→clean gating and this path's honest failure/success lines; the broader output-integrity sweep remains with companion issue #395.
Test
New regression case (case 9) forces a
reset --hardfailure via a PATHgitshim that intercepts onlyresetand delegates everything else to the real git. It asserts the clean did not run: an untracked fileclean -fdxwould have removed survives, noAppliedClean: git cleansuccess line is emitted, and the script exits 5.Red-green proof: temporarily reintroducing the bug (bare
git reset --hard) turns all four new assertions red (exit 0, untracked file removed, clean success line present); the fix makes them green.Verification
bash plugins/repo-hygiene/skills/clean/scripts/git-tree-reset.test.sh— 25/25 passshellcheck --rcfile=.shellcheckrcclean on both changed shell filesVersion bumped
repo-hygiene0.3.0 → 0.3.1 with a matching## [0.3.1]Fixed changelog entry.Related
@{u}guard issues fix(repo-hygiene): git-tree-reset aborts withfatal: ambiguous argument '@{u}'on branches with deleted upstream #393/fix(repo-hygiene): git-tree-reset dry-run doesn't validate@{u}, so dry-run success != apply success #396 touch the samegit-tree-reset.sherror handling; those land as separate PRs and will rebase on this one.Closes #394
🤖 Generated with Claude Code
https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo