Repository navigation
fix(git): protect untracked files hidden by config - #4111
Conversation
worktrunk-bot
left a comment
There was a problem hiding this comment.
The fix reads correct: --untracked-files=normal overrides status.showUntrackedFiles, and it matches the precedent status_porcelain_cached already set for the shared snapshot, so the two status reads in this file now agree. test (linux) and code-coverage are green at this head.
I'm holding the verdict rather than approving: the diff changes is_dirty/dirty_files, which feed ensure_clean — the gate wt remove's trash-rename fast path relies on in place of git's own dirty check. .claude/skills/running-tend/references/review-pr.md ("Data-Loss Surface: Hold for Human Review") puts that call with a human, so I've requested @max-sixty. Two things worth folding in first.
The same display preference still suppresses the auto-staging disclosure. warn_about_untracked_files is fed by two bare git status --porcelain -z reads: the StageMode::All branch in CommitOptions::commit (src/commands/commit.rs, warn_about_untracked_files(&status)?) and warn_if_auto_staging_untracked in src/commands/repository_ext.rs, which wt step squash calls. Under status.showUntrackedFiles = no neither read returns a ?? entry, so Auto-staging N untracked paths: never prints — while StageMode::All still resolves to git add -A, which stages and commits those files anyway.
This PR makes that path more reachable, not less. wt merge decides whether to commit from current_wt.is_dirty()?, so a showUntrackedFiles = no user whose worktree holds only untracked files now takes the commit branch where they previously didn't — the files go into a commit with no notice, where before the merge dropped them. Closing the loop means the same explicit flag on those two reads. -uall rather than normal is the right one there, since the warning enumerates what git add -A is about to stage and normal would collapse an untracked directory to a single dir/ entry — the same reason ensure_no_target_conflicts already passes -uall. Happy to push that as a commit on this branch if you'd rather it ride along than land as a follow-up.
wt step prune is the third command this protects, and it's the unattended one. The guard is shared, so prune's removal path picks the fix up too — and prune is the case where a showUntrackedFiles = no user could have lost an untracked-only worktree without running a command at all. Suggested a CHANGELOG line that names it.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Both items from the last round are addressed, and the CHANGELOG now names wt step prune. Two new things from reading this commit; the data-loss hold with @max-sixty is unchanged by them.
-uall uncaps the disclosure list, and it does so for every user, not just the showUntrackedFiles = no ones. I asked for the flag last round without weighing its output cost. warn_about_untracked_files prints files.join("\n") with no cap, so where -unormal collapsed an untracked directory to one ?? dir/ line, -uall emits one line per file inside it — the snapshot update in this commit (.config/ → .config/wt.toml) is the two-file version of that. On a worktree holding a not-yet-ignored node_modules/ or .venv/, wt merge, wt step commit, and wt step squash now bury their own output under thousands of stderr lines, on the default configuration. The Auto-staging N untracked paths: header already carries the magnitude; the enumeration only has to carry enough for the user to recognise a mistake. GitError::WorktreeRemovalFailed's rendering in src/git/error.rs already has the shape — const MAX_SHOWN: usize = 10 plus and {remaining} more.
wt step relocate --commit is a third git add -A, and it discloses nothing. In src/commands/relocate.rs the auto-commit branch stages with worktree.stage(StageMode::All) and then calls commit_staged_changes(…, StageMode::None), so neither warn_about_untracked_files nor warn_if_auto_staging_untracked is reached — grepping both names finds callers only in src/commands/commit.rs and src/commands/step/squash.rs. The gap predates this PR, but this PR adds a configuration in which it fires: the branch is gated on worktree.is_dirty()?, which now returns true for a showUntrackedFiles = no worktree holding only untracked files. So wt step relocate --commit commits files the user was never shown, where before it left them alone and relocated around them. A warn_about_untracked_files call ahead of the stage closes it, and matches what the other two sites now do.
Both fixes sit outside this diff, so no inline suggestions — say the word and I'll push them as a commit on this branch.
|
thank you @Duang777 ! |
|
Both items from my last review merged as-is, so they've moved from "could ride along on this branch" to live behavior on The
I'm not opening PRs for these unprompted — I already have six open here competing for your review time. Say the word on either and I'll pick it up. |
Version bump to 0.78.0 plus the changelog section for this release, a comment-preservation fix the release review turned up, a PTY test-filter fix, and merges of `main` (commits that landed during the review, including #4124, #4122, #4141, #4140 and #4145). **Bump level**: minor. `cargo semver-checks` reports 2 breaking library changes — a new `leftover_branch` field on the `GitError::WorktreeCreationFailed` struct variant, and `DEPRECATED_TEMPLATE_VARS` removed. Pre-1.0, so a breaking change takes a minor bump. ## Comment-preservation fix A config save — declining the commit-generation offer, say — dropped the trailing comment on every value it rewrote. That was already true in v0.77.0 for a line the save itself changed (`skip-commit-generation-prompt = false # note` → `true`, reproduced on the v0.77.0 binary). #4080 widened it to lines the command never touched: retired template variables now migrate on every load, so the next unrelated save rewrote any line naming one. #4080's own PR said this "needs a call before merge" and merged without one. - `replace_keeping_decor` keeps the value's decor (spacing after `=`, trailing comment) when `merge_tables` replaces a scalar. Comments *above* a line sit on the key and were never at risk. - `main`'s #4120 independently reworked the inline-table arm and moved a key's leading comment onto the rewritten header, but documented that a trailing comment after the closing brace "is still dropped". `replace_inline_with_table` now carries it after the header's `]`, so both of its callers keep it. - Tests: the existing comment-preservation test goes back to a retired name (the only fixture that exercises the replacement) and adds a comment on the mutated line; a new test snapshots and re-parses an inline `[projects]` entry. Each fails without its half of the fix. Not fixed, and identical on v0.77.0: a save deletes an explicitly written *default* value (e.g. `skip-commit-generation-prompt = false # note`) as a stale key, comment included. ## `--help` correction #4104 moved the OpenCode plugin path to etcetera's `Xdg` strategy, which uses `$XDG_CONFIG_HOME` only when absolute — a deliberate, documented choice — but `wt config plugins opencode --help` still said install mirrors OpenCode's precedence. OpenCode reads a relative value as-is, so the help now says an empty `$OPENCODE_CONFIG_DIR` and an empty or relative `$XDG_CONFIG_HOME` are ignored. ## Data-loss surface review Independent finders swept the full diff, including the five commits merged from `main`. Nothing holds the tag; candidates were adjudicated against v0.77.0 binaries: - **Fixed in this release, reproduced on v0.77.0**: #4104 stops `wt config shell install` (with Nushell) deleting a `nushell/vendor/autoload/wt.nu` under the current directory when `$XDG_CONFIG_HOME` is empty or relative; #4120 stops a save or migration from writing an inline section's comment inside `[commit]` brackets, which made the config unparseable, and from dropping unknown keys. - **`wt step prune --min-age` doesn't apply where no reflog exists** — pre-existing, proven by building the pre-#4077 binary. `git clone --bare` and `git fetch` into `refs/heads/*` write no reflog, so in the bare layout `tips-patterns.md` recommends every branch starts unprotected. Docstring corrected here; behavioral fix is follow-up. - **`wt config update --output <path>` replaces any named file whole**, no existence check or prompt — pre-existing, follow-up. - **`wt config shell install fish --yes` replaces a hand-written wrapper at the XDG path** — accepted ("ownership is the path", predates #3602), newly reachable only for users with `$XDG_CONFIG_HOME` set. The FAQ's delete inventory now cross-references it. - **`--var <retired-name>` binds a name nothing references** after #4080 — inert, no shipped usage; follow-up. - **#4135 migrates nothing**: an existing oh-my-pi hook stays put, `pi uninstall` points at `omp uninstall`. `PI_CODING_AGENT_DIR='~/x'` is used literally (Pi expands `~`) — creates a stray directory, deletes nothing; follow-up. ## Changelog 37 entries; each new or edited entry was re-verified against v0.77.0 until a pass came back clean (six passes in all). Passes caught, among others: the OpenCode `-x` recipe was wrong since 0.76.0, not 0.75.0; #4080's JSON reaches every hook, not just `pre-start`/`post-start`; a quoted git error string git never prints; a copy-button overlap that existed only inside this release; #4104's relative-`XDG_CONFIG_HOME` OpenCode change described as a fix when OpenCode reads that path as-is; and which commands a broken inline section stopped. **Fixed** leads with the data-loss entries. ## Open PRs checked against v0.77.0 - **#4124** fixed a bug new in this release, caused by #4080, and has landed and been merged in here. The hand-rolled template-variable scanner it extends is slated for removal; see its commit message. - **#4112** — no data-loss regression vs v0.77.0; two small non-safety regressions from #4111 (an uncapped auto-staging file list, a narrow `Nothing to commit` refusal under `status.showUntrackedFiles = no`). Not merged, by decision. - **#4122** (merged in) and **#4123** — pre-existing bugs, identical on v0.77.0. ## PTY filter fix `test (macos)` failed once on a shell's failed-`setpgid` diagnostic that landed straight after the capture's trailing `\x1b[0m`, with no newline between, so #4136's line-anchored filter missed it. The filter now also matches right after an SGR escape and keeps the escape; its test gains that shape and fails on the old pattern. ## Validation - `cargo run -- hook pre-merge --yes` — 4930 tests passed, 1 skipped; clippy, fmt, doc-sync, snapshot and lockfile checks green - Nightly on the original cut-from tip (https://github.com/max-sixty/worktrunk/actions/runs/35060617020): green across `full-tests` on linux/macos/windows, `feature-powerset`, all `release-target`s, `nix-flake`, `minimal-versions`, `link-check` > _This was written by Claude Code on behalf of max-sixty_ 🤖 Generated with [Claude Code](https://claude.com/claude-code)
#4111 switched the status read behind the auto-staging warning to `-uall`, so the warning sees untracked files that `status.showUntrackedFiles=no` hides. That flag also expands every untracked directory into its files, so `wt step commit`, `wt step squash`, and `wt merge` listed 300 paths for a directory of 300 new files before committing. The warning now reads git's `normal` untracked mode instead. It still overrides `status.showUntrackedFiles=no`, but names a wholly untracked directory once as `dir/`, so a directory of generated files takes one row and can't push a separate new file out of view. The file count stays visible in the commit progress line that follows. The listing also stops at ten rows. Past ten paths it lists nine and a hint counts the rest, so the hint always stands for at least two paths: ``` ▲ Auto-staging 11 untracked paths: ┃ 0-generated/ ┃ 01.txt ┃ … ┃ 08.txt ↳ … and 2 other paths ◎ Generating commit message and committing changes... (310 files) ``` The status read also moves into the warning itself. Commit and squash each built that command before calling the warning, one through `WorkingTree` and one through `Repository`; both now go through the `WorkingTree` they stage. Thanks to @Duang777, whose #4112 proposed the cap. They're credited as co-author. > _This was written by Claude Code on behalf of max-sixty_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: DDL <duanjialing.777@bytedance.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk 0.78.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **Hook scripts read renamed context keys**: the JSON piped to every hook drops `worktree`, `repo_root`, `main_worktree`, and `main_worktree_path` in favour of `worktree_path`, `repo_path`, `repo`, and `primary_worktree_path`. Config templates migrate on load, so hooks written with the old names keep working; scripts reading the JSON, `--execute` templates, and `--var` overrides need updating. (Breaking.) ([#4080](max-sixty/worktrunk#4080)) - **`wt config plugins pi` installs for Pi; oh-my-pi moves to `wt config plugins omp`**: `pi install` wrote an oh-my-pi hook, which Pi never loads; Pi users re-run it to get a Pi extension. Existing oh-my-pi hooks keep working, listed as outdated until `wt config plugins omp install`. (Breaking: scripts that run `wt config plugins pi` for oh-my-pi need `omp`.) [Docs](https://worktrunk.dev/claude-code/#oh-my-pi) ([#4135](max-sixty/worktrunk#4135), thanks @ashebanow) - **Interactive prompts no longer open with a blank line**: `wt config shell install`, `wt config plugins claude install`, and the commit-message setup offer in `wt merge` began with one. ([#4059](max-sixty/worktrunk#4059)) - **`wt config show` gives a single `wt config shell install` hint**: an outdated wrapper, a fish wrapper at the deprecated `conf.d` path, and missing fish completions each printed their own hint. The zsh `compinit` snippet is now syntax-highlighted. ([#4059](max-sixty/worktrunk#4059)) - **`wt config update --output <path>` confirms the write**: it now prints `✓ Wrote user config migration @ ~/migrated.toml` where it printed nothing. `--output=-` stays silent. ([#4053](max-sixty/worktrunk#4053)) ### Fixed - **Removal no longer discards untracked files hidden by `status.showUntrackedFiles = no`**: under that setting, a worktree holding only untracked files looked clean, so `wt remove`, `wt merge`, and `wt step prune` removed it without `--force`. Safety checks now always count untracked files, and auto-staging warnings list every file they stage. ([#4111](max-sixty/worktrunk#4111), thanks @Duang777) - **`wt remove` and `wt merge` check the right worktree under an inherited `GIT_DIR`**: run from a `!wt` git alias or a git hook, which pass `GIT_DIR` down, `wt remove` could delete a dirty worktree without `--force`, and `wt merge` could refuse a clean merge. Both now check the worktree they act on. ([#4082](max-sixty/worktrunk#4082), thanks @Duang777) - **`wt merge` no longer deletes a worktree locked with `git worktree lock`**: the merge completes and prints `Worktree preserved (locked)`. Every Worktrunk removal refuses a locked worktree, even with `--force`; run `git worktree unlock <path>` first. ([#4073](max-sixty/worktrunk#4073), thanks @Duang777) - **`wt config update` no longer overwrites a config edited while its prompt is open**: it wrote the pre-prompt snapshot over the newer file and reported success. It now fails with the path and leaves the newer file; re-running previews the current contents. ([#4127](max-sixty/worktrunk#4127), thanks @zach-hammad-vs for reporting) - **`wt step prune` no longer deletes a branch created minutes ago from an older commit**: `--min-age` aged a branch without a worktree by its commit's date, so a new branch off a day-old commit was pruned on the next run. Age now comes from the branch's oldest reflog entry. ([#4077](max-sixty/worktrunk#4077)) - **A comment or blank line above an inline config section no longer breaks the config**: a save or migration that rewrote `commit = { … }` as `[commit]` put that line inside the brackets. `wt switch`, `wt merge`, `wt remove`, and several `wt step` commands then failed with `Failed to load config`; `wt list` warned and ran without the config. Saves also dropped unrecognized keys from inline sections. ([#4120](max-sixty/worktrunk#4120), thanks @zach-hammad-vs for reporting) - **Config saves keep the trailing comment on a value they rewrite**: a save such as answering the commit-generation offer dropped the comment after each value it rewrote, including one after an inline section's closing brace. ([#4138](max-sixty/worktrunk#4138)) - **Ignored project-config keys warn from every worktree**: an unknown key in `.config/wt.toml`, or one that belongs in user config such as `[merge]`, warned only in the main worktree, so `wt merge` from a linked worktree ignored it silently. ([#4145](max-sixty/worktrunk#4145), thanks @ee-prog for reporting) - **`wt merge` skips `post-commit` when it removes that hook's worktree**: before, the hook ran in the primary worktree when the removed one sat inside the repository, and otherwise didn't run. The merge now prints `▲ Skipped post-commit`; move work that must finish there to `pre-remove`. ([#4049](max-sixty/worktrunk#4049)) - **`wt switch` recovers from a removed worktree in a bare repository**: `wt switch` and the picker surfaced git's raw `fatal: Unable to read current working directory` instead, and the removed-directory message lacked `wt switch ^`. ([#4067](max-sixty/worktrunk#4067)) - **`wt config shell install fish` puts the wrapper where fish reads it under a custom `$XDG_CONFIG_HOME`**: the wrapper always went to `~/.config/fish/functions/`, so install reported success while `wt switch` changed no directory. On Windows, the fish completion moves from `%APPDATA%\fish\completions` to the wrapper's directory. ([#4107](max-sixty/worktrunk#4107)) - **`wt config shell install zsh` honours `$ZDOTDIR` only when it is absolute**: an empty or relative value resolved against the current directory, so install appended to a `.zshrc` under the current directory and reported success while the file zsh reads went untouched. A non-absolute value now falls back to `$HOME`. ([#4085](max-sixty/worktrunk#4085)) - **`wt config shell install` for Nushell no longer deletes a `wt.nu` under the current directory**: an empty or relative `$XDG_CONFIG_HOME` resolved against it, so install removed a `nushell/vendor/autoload/wt.nu` beneath it unread, as a stale wrapper. It now falls back to `~/.config`, which also fixes unread `tea` logins that let `wt switch pr:<n>` treat a Gitea host as GitHub. ([#4104](max-sixty/worktrunk#4104)) - **`wt switch --create` names the branch a failed worktree add leaves behind**: `git worktree add -b` writes the branch ref before populating the worktree, so a failure in between left the branch with no worktree, and the next attempt reported a name collision. The error now says how to delete or reuse it. ([#4109](max-sixty/worktrunk#4109), thanks @technicalpickles for reporting) - **`wt config shell install` no longer tells an already-wrapped shell to restart**: reinstalling from a shell with the wrapper loaded, such as after a version bump, printed `↳ Restart shell to activate shell integration`. ([#4059](max-sixty/worktrunk#4059)) - **`wt config shell install` migrates a deprecated `conf.d` fish wrapper when `~/.config/fish/functions` is missing**: before, only `wt config shell install fish` migrated it. `wt config show` now reports that wrapper even when fish isn't on `PATH`. ([#4059](max-sixty/worktrunk#4059)) - **Plugin status and uninstall checks ask the agent CLIs**: `wt config show` could list an installed Claude Code plugin or Gemini CLI extension as not installed, and `wt config plugins claude|codex uninstall` could report success over a marketplace still configured. A repeat Claude uninstall now runs both removals instead of stopping at `Plugin not installed`. ([#4048](max-sixty/worktrunk#4048), [#4054](max-sixty/worktrunk#4054)) - **`wt config plugins opencode install` no longer writes the plugin into the current directory**: with `OPENCODE_CONFIG_DIR` set but empty, it wrote `plugins/worktrunk.ts` there, typically inside a repository, reporting success though OpenCode never saw it. An empty value now counts as unset. ([#4084](max-sixty/worktrunk#4084)) - **The OpenCode plugin clears its activity marker when OpenCode 1 disposes it**: only explicit session deletion cleared it, so a finished session kept its `💬` and `wt list` showed it as active until `wt config state marker clear` was run by hand. ([#4129](max-sixty/worktrunk#4129), thanks @drewdas for reporting) - **`wt config update` migrates `[ci] platform` into a `[forge]` section that only sets `hostname`**: that config, common with GitHub Enterprise and self-hosted GitLab, kept the deprecated key with no deprecation warning. ([#4061](max-sixty/worktrunk#4061)) - **Deprecated keys in an inline `[projects]` entry now migrate**: `"example.com/org/repo" = { merge = { no-ff = true } }` skipped the migration the section form got, so the per-project setting was ignored. ([#4122](max-sixty/worktrunk#4122), thanks @zach-hammad-vs for reporting) - **A deprecated config section the migration can't rewrite now warns**: `select = "not a table"`, or a `[select]` whose `[switch.picker]` destination is already set, was ignored without a message. It now reports `unknown field select (will be ignored)`. ([#4121](max-sixty/worktrunk#4121), thanks @zach-hammad-vs for reporting) - **`wt config update` no longer changes what a template renders**: it could rename `{{ repo_root }}` inside a quoted s See merge request: Harmonybrew/homebrew-core!20435
Problem
The destructive clean-worktree guard used bare
git status --porcelaincalls. Git applies the user'sstatus.showUntrackedFilespreference to that command, so with:a worktree containing only untracked files appeared clean. That false result could let
wt remove,wt merge --no-commit, or unattendedwt step pruneremove the worktree without protecting those files.The same preference also hid the disclosure printed before
git add -A:wt merge,wt step commit, andwt step squashstill staged and committed the files, but did not name them first.Fix
WorkingTree::is_dirty()anddirty_files()usegit status --porcelain --untracked-files=normal, overriding the display-only preference for safety checks.git status --porcelain -z -uall, so they override the preference and enumerate each file inside untracked directories exactly asgit add -Awill stage it.Testing
Regression coverage now exercises:
wt remove.wt merge --no-commitbefore ref changes or cleanup.wt step prunewhile another clean candidate is removed.wt step commitandwt step squashauto-staging warnings with two files inside an untracked directory.Applying only these tests to the old
origin/mainimplementation produced six expected failures across the destructive and disclosure paths. The current branch passes the full project gate: