Worktree family hardening: fix P0 repo deletion, unify batch/exit-code/JSON contracts#178
Merged
Conversation
WHY: The 2026-06-03 wtc/wtl/wtdel exercise report catalogued 20 DX findings. Empirical re-verification in throwaway sandboxes changed the picture materially: one finding is false (DX-1 — --base accepts relative refs; the test repo just lacked HEAD~3), two are by-design but under-documented (the -y/-f confirmation tier model), and the report missed a P0: `hug wtdel -p <main-worktree> -f` run from a linked worktree rm -rf's the entire main repository after git correctly refuses to remove it. Designing from the report without verification would have fixed the wrong things. WHAT: Approved design covering safety hardening (main-worktree guard in all wtdel modes, abolition of the blind rm -rf fallback, scoped pruning, dead-code removal), a validate-all-then-execute batch model, a documented 3-tier confirmation language, family exit codes (0/1/2/3), --json/-q scriptability for wtc/wtdel, wtl staleness visibility, and flag unification (-p/--path, -B/--with-branch). HOW: Four contract decisions were answered explicitly by the maintainer (batch model, -y semantics, exit codes, single-PR delivery) and are recorded in §2 so future readers know they were deliberate, not incidental. IMPACT: This spec is the single source of truth for the implementation commits that follow; each cites its sections. Lesson encoded: exercise reports are hypotheses — re-verify findings empirically before designing fixes (DX-1 was disproven by a 5-commit sandbox in seconds, and the worst bug in the family was found only by trying the report's own "workaround" suggestion in a sandbox). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…f fallback, scoped prune
WHY: `wtdel -p <main-repo> --force` deleted the entire main repository. Three
independent bugs conspired:
1. Path-mode (-p) had NO main-worktree guard. Branch-mode refused main at
resolve time, but -p bypassed that resolver entirely and reached the
removal step unguarded. Git then refused ("is a main working tree"),
which triggered bug #2.
2. When git refused, the rm -rf fallback treated every refusal as cruft and
bulldozed the target — including the main repo. The P0 path was:
`wtdel -p <main> --force` → git refuses → rm -rf <main> → total loss.
3. Stale-flow used global `git wt prune` per-removal, which silently
erased every OTHER stale entry as a side effect.
WHAT: Three surgical safety fixes:
- main_worktree_of_gitdir(): new lib function that resolves the main
working tree for an EXPLICIT gitdir. Unlike resolve_main_worktree_path
(which anchors to CWD), this function anchors to the target's own gitdir
— the only correct answer for submodule topologies where CWD's gitdir
differs from the target wt's owning gitdir.
- prune_worktree_entry(): new lib function that deletes exactly one admin
directory ($gitdir/worktrees/<id>) by matching the gitdir file's content
against <wt-path>/.git. Replaces global `git wt prune` which
destroyed all stale entries indiscriminately.
- Removed rm -rf fallback entirely. Git refuses removals for PRINCIPLED
reasons (main wt, locks, submodules). Hug now surfaces git's
reason instead of overriding it. Added a submodule double-force retry
(git demands --force --force for wts containing submodules) as the
sole exception — still git-managed, never a blind filesystem delete.
HOW: resolve_main_worktree_path() refactored into a thin CWD adapter that
delegates to main_worktree_of_gitdir(). The wtdel path loop gains a P0 guard
after gitdir resolution: compare the target path against the target's own
main wt (not CWD's). The stale flow uses prune_worktree_entry instead
of global prune. The execute block replaces rm-rf with error reporting.
IMPACT: Main-repo deletion P0 eliminated. Unrelated stale entries survive
when a different wt is removed. Submodule wts get the correct
gitdir-anchored main check. All 133 existing tests continue to pass;
9 new tests cover the three safety fixes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
WHY: remove_worktree(), create_worktree(), and create_branch_if_needed() had zero callers — git-wtdel/git-wtc reimplement their logic inline — and remove_worktree() carried its own copy of the rm -rf fallback abolished in the previous commit. Dead code with a live footgun is the worst kind. WHAT: Deleted all three function bodies and their doc comments from hug-git-worktree. Deleted the 7 test blocks that exercised them (3 create_worktree + 4 remove_worktree) from test_hug-git-worktree.bats. HOW: grep-verified zero references remain outside comments/history. Test count dropped from 74→67 in lib tests (7 deleted, 0 regressions). IMPACT: No behavior change. Removes the residual rm -rf hazard that could confuse future developers into thinking it was safe to call these functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WHY: Every hug command used exit 1 for every failure mode — usage errors, safety blocks, and operational failures were indistinguishable. Shell scripts wrapping hug (CI pipelines, Make targets, dotfiles) could not tell "bad flag" from "refused for safety" from "git error". The danger-tier -y rejection was particularly misleading: it signalled generic failure (exit 1) when the real semantic was "blocked by policy", which callers should handle differently. WHAT: Introduced four named exit-code constants (HUG_EX_OK/FAIL/USAGE/BLOCKED) in hug-output and two thin helper wrappers (error_usage → exit 2, error_blocked → exit 3). Changed prompt_confirm_danger's HUG_YES rejection from exit 1 to exit 3 ($HUG_EX_BLOCKED). Added the confirmation-tier matrix to hug-confirm's file header documenting how -y/-f interact with safe/warn/ danger tiers. Updated the prompt_confirm_warn docstring to explain WHY -y covers warn-tier (recoverable destructive) but not danger-tier. HOW: Constants are defined at lib load time in hug-output (before any function that might use them). error_usage/error_blocked delegate to the existing error() which already accepts an optional exit-code argument — no duplication. The tier matrix in hug-confirm is a reference table, not executable code, so it has zero runtime cost. Existing tests for HUG_YES + danger used assert_failure (any non-zero) and continue to pass; a new test explicitly asserts exit 3. IMPACT: Scripts can now branch on exit codes (2 = usage, 3 = safety-blocked) while `if hug …` still works for simple success/failure checks. The danger-tier -y rejection is unchanged for interactive users — it still refuses, just with a more specific exit code that automated callers can react to. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…n, -q WHY: The old wtdel processed worktrees one-at-a-time — remove #1, then try #2 (even if #1 failed), etc. A batch with one bad target would partially succeed, leaving the user with an inconsistent state. There was no way to script wtdel (no --json, no quiet mode, no exit code distinction between "not found" and "blocked"). WHAT: Restructured wtdel into 4 phases: RESOLVE → PRE-FLIGHT → CONFIRM → EXECUTE. Every target is classified first (ok/stale/dirty/blocked/etc); if ANY is blocked or invalid, NOTHING is removed. One confirmation covers the whole batch. Added --json for scriptable output, -q for quiet mode. Exit codes: 2=usage, 3=blocked, 1=operational error. HOW: New wtdel_classify_target() in hug-git-worktree classifies targets without mutation. git-wtdel builds plan arrays, validates the whole plan, then executes. JSON emitted via hug-json helpers. getopt gains q/quiet/json. PITFALL — interactive-menu guard: The old code populated worktree_paths[] from branch_names[] BEFORE the interactive-menu check, so an empty worktree_paths meant "no targets at all." The new code keeps branch_names and worktree_paths separate (Phase 1 resolves branch_names into plan records directly), so the guard must check BOTH arrays: if [[ ${#worktree_paths[@]} -eq 0 && ${#branch_names[@]} -eq 0 ]]; then Missing the branch_names check causes all positional-branch invocations to fall through to the interactive menu — silent and wrong. IMPACT: Batch removals are now atomic (all-or-nothing). Scripts can parse --json output and branch on exit codes. -q enables automation-friendly output. Breaking change: mixed-validity batches now remove NOTHING (was best-effort). The new behavior is safer and more predictable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…s, --json, -q, exit codes WHY: wtc was the only worktree command without HUG_FORCE env support, without scriptability flags (--json/-q), and without exit-code distinction between usage errors and safety blocks. Its -f/--dry-run mutual exclusion blocked legitimate use cases (preview with force-override semantics). Branch-in-use errors didn't name the holding worktree, forcing users to run `hug wtl` separately. WHAT: Added HUG_FORCE env support (parity with wtdel). Removed the -f/--dry-run mutual exclusion — they now compose naturally. Added -p/--path for explicit worktree path, -B/--with-branch as a discoverable alias for --new, --json for machine-readable output, -q for quiet mode. Upgraded error guards to use error_blocked (exit 3) for branch-in-use and main-checkout cases, error_usage (exit 2) for bad arguments and getopt failures. Branch-in-use guard now names the holding worktree path and suggests the removal command. HOW: Extended getopt with new short options (B, q, p:) and long options (with-branch, quiet, json, path:). hug-json is already sourced via hug-common's library chain, so json_escape/to_json_nested are available without an extra source line. -p/--path validation runs after the positional-arg $# case block so it can detect the conflict between positional and flag paths. --json emits to stdout after all chatter has gone to stderr, keeping the output pipe-safe. HUG_QUIET guard wraps only the decorative summary blocks (DRY RUN and SUMMARY) while leaving info/success/tip calls outside — those already respect HUG_QUIET internally. IMPACT: Scripts can now create worktrees programmatically with --json output and HUG_FORCE env var. -B provides a more discoverable alias for --new (mnemonic: "with [this] branch"). Exit codes distinguish usage errors (2) from safety blocks (3), enabling callers to branch on $? for retry/abort logic. Fixed stale integration test assertions for wtdel's pre-flight model (dirty worktree and current-worktree guards). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ils in JSON, usage exit 2
WHY: When a worktree directory was deleted externally (rm -rf), wtl still
showed the old commit hash — indistinguishable from a healthy worktree at a
glance. Scripts consuming --json had no structured way to detect stale
entries. Usage errors (unknown flags, missing values) exited 1 (general
error) instead of 2 (usage error), inconsistent with the rest of the worktree
family (wtdel, wtc) which adopted exit 2 in earlier tasks.
WHAT:
- Stale worktree rows now render "(gone)" instead of the commit hash
- wtl --json gains "missing" (bool) and "dirty_details" (array) fields
- Three usage-error sites in git-wtl now route through error_usage (exit 2)
- Existing pytest mocks updated to target _check_worktree_dirty_details
(parse_worktree_list now calls it directly instead of the thin wrapper)
HOW:
- git-wtl: after extracting the commit hash in each rendering loop (filtered
and unfiltered), check [[ -d "$path" ]] and replace commit with "gone" if
the directory is absent. The existing printf format ($commit) then renders
it as (gone) — no new legend symbol, no column shift.
- worktree.py: added `missing` (bool) and `dirty_details` (tuple) fields to
WorktreeInfo. Extracted _dirty_detail_labels() helper that converts a
WorktreeDirtyInfo into a tuple of category labels ("staged", "unstaged",
"untracked") — DRY between parse_worktree_list and the dirty subcommand.
Propagated through WorktreeList parallel arrays and to_json().
- Changed parse_worktree_list to call _check_worktree_dirty_details directly
(not _check_worktree_dirty wrapper) to get both the dirty boolean AND the
categorized labels in a single subprocess round-trip — avoids 3 extra git
invocations per worktree.
- error_usage already existed in hug-output (exit 2 with HUG_EX_USAGE);
simply replaced the three error()+exit 1 patterns.
LESSONS:
- BATS refute_output --regexp spans newlines (.* matches \n), making
per-line negative assertions unreliable. Use `grep -E` in an if/fail
pattern for line-scoped negative checks.
- When adding fields to a dataclass with existing tests, the mock target
must match the actual call site — changing from _check_worktree_dirty to
_check_worktree_dirty_details requires updating ALL @patch decorators.
IMPACT: Users can immediately see which worktrees are gone from the listing.
Scripts can filter on missing/dirty_details in JSON output. Consistent exit
codes across the worktree family.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
…ing guide, CHANGELOG WHY: The three worktree commands (wtc/wtdel/wtl) had inconsistent help text, no shared safety language, no exit code documentation, and no scripting guide. The CHANGELOG had no entry for the worktree family hardening. Users could not discover the safety model or understand exit codes from help alone. WHAT: Unified SAFETY and EXIT CODES sections across wtc/wtdel/wtl help text. Updated CHANGELOG with [Unreleased] Fixed/Changed/Added sections documenting the hardening. Expanded docs/commands/worktree.md with safety tier table, family-wide exit codes, scripting guide (--json/-q recipes), stale worktree lifecycle, and deferred --detach reference. Filed #177 for the --detach follow-up. HOW: Help text in all three commands now follows the same structure: OPTIONS, DESCRIPTION, EXAMPLES, SAFETY, EXIT CODES, SEE ALSO. The -y flag description explicitly states it is NOT sufficient for dangerous operations — that wording is identical across wtc and wtdel. EXIT CODES uses the same 0/1/2/3 convention documented in the codebase. IMPACT: Users discover the safety model and exit codes from --help. Scripts have a documented JSON schema with missing/dirty_details fields and scripting patterns (-q, -p -b recipe). The CHANGELOG provides a reviewable record of all behavioral changes. The deferred --detach issue gives a clear landing spot for future work. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WHY: The implementation commits were formatted per-editor; the project's canonical formatter (make sanitize) normalized trailing-comment spacing and a few other whitespace conventions. WHAT: shfmt and ruff whitespace normalization on the touched files only (git-wtc, git-wtdel, worktree.py, test_worktree.py). No logic changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9a873e8c3
ℹ️ 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".
WHY: Two defensive gaps surfaced during Codex review of the worktree
family hardening PR:
1. wtdel: `hug wtdel feat feat --force` resolves both specs to the same
path. Phase 1 (RESOLVE) appends two plan records; Phase 4 (EXECUTE)
removes the first successfully, then the second fails with "not a
working tree" — a partial removal that directly violates the
all-or-nothing batch contract this PR introduced.
2. wtc: `-p ""` (easy from `-p "$UNSET_VAR"`) sets flag_path to empty,
then `[[ -n "$flag_path" ]]` evaluates false, so the command silently
falls back to auto-generating a path — creating the worktree somewhere
the caller never intended.
WHAT:
- wtdel: Post-RESOLVE dedup by resolved path (empty paths dedup by spec).
Duplicates emit a warning and are dropped; the batch proceeds with
unique entries only.
- wtc: Reject `-p/--path` with an empty or missing value as a usage
error (exit 2), caught at parse time before any worktree logic runs.
HOW:
- Dedup uses an associative array keyed on resolved path (or spec for
no-worktree entries), placed between Phase 1 and Phase 2. A compaction
loop rebuilds the six parallel plan arrays to only kept indices.
- The empty-value guard is a `[[ -z "${2:-}" ]]` check inside getopt's
`-p|--path` case arm — fails fast before `$2` is consumed.
IMPACT: The batch contract is now structurally enforced against duplicate
targets. Empty `-p` is a parse-time error instead of a silent misroute.
Zero behavioral change for well-formed invocations.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✅ Eliminate the P0 where
wtdel -p <main-repo> --forcedeleted the entire main repository (no main-WT guard + rm -rf fallback bulldozing git's refusal). Unify wtc/wtdel/wtl on one batch model (validate-all-then-execute), one exit-code convention (0/1/2/3), and shared--json/-qscriptability.🟢 Behavioral change worth a look: mixed-validity batches now remove NOTHING instead of best-effort partial removal (safer, but scripts relying on partial success need updating). 327 worktree-family BATS + 943 pytest green. Design doc: https://github.com/elifarley/hug-scm/blob/main/mgmt/plans/2026-06-12-worktree-family-hardening-design.md
What changed, per command:
wtdel:--force --forceprune_worktree_entry(removing worktree A no longer destroys unrelated stale entry B)wtc:HUG_FORCEenv support,-fcomposes with--dry-run(was mutually exclusive)-p/--path,-B/--with-branch,--json,-qwtl:(gone)instead of the commit hash--jsongainsmissing+dirty_detailsfieldsFamily-wide:
HUG_EX_OK/FAIL/USAGE/BLOCKEDconstants +error_usage(2)/error_blocked(3) in hug-output-yon danger-tier now exits 3 (was 1)Usage examples
Details: root cause and deferred work
The P0 had three contributing factors: (1) path-mode
-phad no main-worktree guard, so-p <main>reached the removal step; (2) when git refused removal ("is a main working tree"), the rm -rf fallback deleted the entire repo; (3) the stale flow used a global prune, destroying unrelated metadata. All three fixed.--detachworktrees deferred to #177.