From fdd3943cb12dddc03ff321daee9e7fe73b84c40a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 23 Jul 2026 09:08:40 -0700 Subject: [PATCH] fix(update): survive undeletable untracked files during autostash (#70161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git stash push --include-untracked exits non-zero when it saved everything but could not DELETE some swept untracked files from the working tree (e.g. a root-owned packaging/ directory left behind by a sudo'd build: 'warning: failed to remove ...: Permission denied'). The updater ran the push with check=True, so this benign partial failure raised CalledProcessError and aborted the whole update before it even fetched — reliably, on every run, for any user with an undeletable untracked path in the checkout. Fix, both ends of the class: - _stash_local_changes_if_needed: probe refs/stash before/after the push. Non-zero push + fresh stash entry = changes are saved; warn, reset the tracked-side leftovers (they're in the stash), and continue the update. Non-zero push + NO stash entry = real failure; keep aborting. - _restore_stashed_changes: on restore, those same undeletable files still sit in the tree, so 'git stash apply' exits 1 with 'already exists, no checkout' even though every tracked change applied and nothing was lost. Classify that stderr shape (strictly — any other error line still routes to the conflict path) as restored instead of resetting the tree and telling the user the restore failed. Repro'd both halves with real git; behavioral E2E test covers stash -> checkout -> restore round-trip with an undeletable dir. --- hermes_cli/main.py | 112 +++++++++++- tests/hermes_cli/test_update_autostash.py | 213 +++++++++++++++++++++- 2 files changed, 315 insertions(+), 10 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index b9bb4737e199..9ca6125fc57f 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7069,18 +7069,72 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st "hermes-update-autostash-%Y%m%d-%H%M%S" ) print("→ Local changes detected — stashing before update...") - subprocess.run( - git_cmd + ["stash", "push", "--include-untracked", "-m", stash_name], - cwd=cwd, - check=True, - ) - stash_ref = subprocess.run( + prev_stash = subprocess.run( git_cmd + ["rev-parse", "--verify", "refs/stash"], cwd=cwd, capture_output=True, text=True, - check=True, ).stdout.strip() + push = subprocess.run( + git_cmd + ["stash", "push", "--include-untracked", "-m", stash_name], + cwd=cwd, + capture_output=True, + text=True, + ) + if push.stdout.strip(): + print(push.stdout.strip()) + stash_probe = subprocess.run( + git_cmd + ["rev-parse", "--verify", "refs/stash"], + cwd=cwd, + capture_output=True, + text=True, + ) + stash_ref = stash_probe.stdout.strip() + stash_created = ( + stash_probe.returncode == 0 and bool(stash_ref) and stash_ref != prev_stash + ) + + if push.returncode != 0: + if stash_created: + # git stash push exits non-zero when it saved everything but could + # not delete some swept untracked files from the working tree + # (e.g. a root-owned directory: "warning: failed to remove ...: + # Permission denied"). The stash entry is complete — the changes + # are safe — so this is not a failure. Leave the undeletable + # files in place and continue the update. + if push.stderr.strip(): + print(push.stderr.strip()) + print( + " ⚠ Some untracked files could not be removed from the " + "working tree (permission denied)." + ) + print( + " They were still saved to the stash and were left in " + "place — the update will continue." + ) + # A partially-failed stash push also aborts its working-tree + # cleanup for TRACKED modifications — they are saved in the stash + # but still dirty the tree, which would break the checkout/pull + # that follows. Safe to reset: everything is in the stash entry. + subprocess.run( + git_cmd + ["reset", "--hard", "HEAD"], + cwd=cwd, + capture_output=True, + ) + else: + # No stash entry was created: the changes were NOT saved. This + # is a real failure — bail out before the update touches HEAD. + print("✗ Could not stash local changes — update aborted.") + if push.stderr.strip(): + print(f" {push.stderr.strip().splitlines()[0]}") + print( + " Commit, stash, or clean up your local changes manually, " + "then re-run `hermes update`." + ) + raise subprocess.CalledProcessError( + push.returncode, push.args, output=push.stdout, stderr=push.stderr + ) + return stash_ref @@ -7116,6 +7170,36 @@ def _print_stash_cleanup_guidance( ) +def _stash_apply_failed_only_on_existing_untracked(stderr: str) -> bool: + """True when a ``git stash apply`` failure is ONLY about untracked files + that already exist in the working tree. + + This is the tail end of the permission-denied autostash class: ``git stash + push --include-untracked`` swept undeletable files (e.g. a root-owned + ``packaging/`` directory) into the stash but could not remove them from + disk. On restore, git applies all tracked changes, then refuses to + overwrite those still-present files (``already exists, no checkout`` / + ``could not restore untracked files from stash``) and exits non-zero even + though nothing was lost. Any other error line (e.g. ``would be + overwritten by merge`` / ``Aborting``) means the tracked apply itself + failed and this returns False. + """ + lines = [ln.strip() for ln in (stderr or "").splitlines() if ln.strip()] + if not lines: + return False + saw_untracked_error = False + for ln in lines: + if "already exists, no checkout" in ln: + saw_untracked_error = True + elif "could not restore untracked files from stash" in ln: + saw_untracked_error = True + elif ln.startswith(("warning:", "hint:")): + continue + else: + return False + return saw_untracked_error + + def _restore_stashed_changes( git_cmd: list[str], cwd: Path, @@ -7158,7 +7242,19 @@ def _restore_stashed_changes( ) has_conflicts = bool(unmerged.stdout.strip()) - if restore.returncode != 0 or has_conflicts: + if restore.returncode != 0 and not has_conflicts and ( + _stash_apply_failed_only_on_existing_untracked(restore.stderr) + ): + # Permission-denied autostash tail end: the tracked changes applied + # cleanly; the only "failure" is untracked files that never left the + # working tree (git could not delete them at stash time, so it now + # refuses to overwrite them). Their content was never touched — + # nothing is lost. Treat as restored. + print( + " ⚠ Some stashed untracked files already exist in the working " + "tree and were kept as-is." + ) + elif restore.returncode != 0 or has_conflicts: print("✗ Update pulled new code, but restoring local changes hit conflicts.") if restore.stdout.strip(): print(restore.stdout.strip()) diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 2797ca19f8fa..5ffdb519bfda 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -77,8 +77,11 @@ def test_stash_local_changes_if_needed_returns_specific_stash_commit(monkeypatch assert stash_ref == "abc123" assert calls[1][0][-2:] == ["ls-files", "--unmerged"] - assert calls[2][0][1:4] == ["stash", "push", "--include-untracked"] - assert calls[3][0][-3:] == ["rev-parse", "--verify", "refs/stash"] + # Pre-push probe of refs/stash (baseline for detecting a fresh entry), + # then the push, then the post-push probe. + assert calls[2][0][-3:] == ["rev-parse", "--verify", "refs/stash"] + assert calls[3][0][1:4] == ["stash", "push", "--include-untracked"] + assert calls[4][0][-3:] == ["rev-parse", "--verify", "refs/stash"] def test_resolve_stash_selector_returns_matching_entry(monkeypatch, tmp_path): @@ -952,3 +955,209 @@ def test_install_method_marker_not_autostashed_by_update(tmp_path): ["git", "status", "--porcelain"], cwd=tmp_path, capture_output=True, text=True ).stdout assert ".install_method" not in status + + +# --------------------------------------------------------------------------- +# Permission-denied autostash class: undeletable untracked files (root-owned +# packaging/ etc.) must not abort the update when the stash entry was created. +# --------------------------------------------------------------------------- + + +def test_stash_push_partial_removal_failure_continues_when_stash_created( + monkeypatch, tmp_path, capsys +): + """git stash push exits 1 ("failed to remove ...: Permission denied") but + the stash entry exists → treat as success, return the new ref.""" + calls = [] + + def fake_run(cmd, **kwargs): + calls.append((cmd, kwargs)) + if cmd[-2:] == ["status", "--porcelain"]: + return SimpleNamespace(stdout=" M x.py\n?? packaging/\n", returncode=0) + if cmd[-2:] == ["ls-files", "--unmerged"]: + return SimpleNamespace(stdout="", returncode=0) + if cmd[-3:] == ["rev-parse", "--verify", "refs/stash"]: + # Before push: no stash. After push: new entry. + probes = [c for c, _ in calls if c[-3:] == ["rev-parse", "--verify", "refs/stash"]] + if len(probes) == 1: + return SimpleNamespace(stdout="", returncode=1) + return SimpleNamespace(stdout="newref123\n", returncode=0) + if cmd[1:4] == ["stash", "push", "--include-untracked"]: + return SimpleNamespace( + stdout="Saved working directory and index state\n", + stderr=( + "warning: failed to remove packaging/homebrew/hermes-agent.rb: " + "Permission denied\n" + ), + returncode=1, + ) + if cmd[1:3] == ["reset", "--hard"]: + return SimpleNamespace(stdout="", stderr="", returncode=0) + raise AssertionError(f"unexpected command: {cmd}") + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + + stash_ref = hermes_main._stash_local_changes_if_needed(["git"], tmp_path) + + assert stash_ref == "newref123" + # Tracked mods are saved in the stash but the failed push leaves them in + # the tree — the follow-up reset must run so the checkout/pull can proceed. + assert any(c[1:3] == ["reset", "--hard"] for c, _ in calls) + out = capsys.readouterr().out + assert "could not be removed" in out + assert "update will continue" in out + + +def test_stash_push_failure_without_stash_entry_still_raises(monkeypatch, tmp_path, capsys): + """git stash push fails AND no stash entry was created → real failure.""" + + def fake_run(cmd, **kwargs): + if cmd[-2:] == ["status", "--porcelain"]: + return SimpleNamespace(stdout=" M x.py\n", returncode=0) + if cmd[-2:] == ["ls-files", "--unmerged"]: + return SimpleNamespace(stdout="", returncode=0) + if cmd[-3:] == ["rev-parse", "--verify", "refs/stash"]: + return SimpleNamespace(stdout="", returncode=1) + if cmd[1:4] == ["stash", "push", "--include-untracked"]: + return SimpleNamespace( + stdout="", stderr="fatal: unable to write new index file\n", + returncode=1, args=cmd, + ) + raise AssertionError(f"unexpected command: {cmd}") + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + + with pytest.raises(CalledProcessError): + hermes_main._stash_local_changes_if_needed(["git"], tmp_path) + out = capsys.readouterr().out + assert "update aborted" in out + + +def test_stash_push_failure_with_preexisting_stash_unchanged_still_raises( + monkeypatch, tmp_path +): + """A pre-existing stash entry must not be mistaken for a fresh save.""" + + def fake_run(cmd, **kwargs): + if cmd[-2:] == ["status", "--porcelain"]: + return SimpleNamespace(stdout=" M x.py\n", returncode=0) + if cmd[-2:] == ["ls-files", "--unmerged"]: + return SimpleNamespace(stdout="", returncode=0) + if cmd[-3:] == ["rev-parse", "--verify", "refs/stash"]: + return SimpleNamespace(stdout="oldref456\n", returncode=0) + if cmd[1:4] == ["stash", "push", "--include-untracked"]: + return SimpleNamespace(stdout="", stderr="boom\n", returncode=1, args=cmd) + raise AssertionError(f"unexpected command: {cmd}") + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + + with pytest.raises(CalledProcessError): + hermes_main._stash_local_changes_if_needed(["git"], tmp_path) + + +def test_stash_apply_untracked_only_failure_detector(): + fn = hermes_main._stash_apply_failed_only_on_existing_untracked + assert fn( + "packaging/homebrew/hermes-agent.rb already exists, no checkout\n" + "error: could not restore untracked files from stash\n" + ) is True + # Tracked-apply failure lines must NOT be classified as benign. + assert fn( + "error: Your local changes to the following files would be overwritten by merge:\n" + "\ttracked.txt\n" + "Please commit your changes or stash them before you merge.\n" + "Aborting\n" + "packaging/homebrew/hermes-agent.rb already exists, no checkout\n" + "error: could not restore untracked files from stash\n" + ) is False + assert fn("") is False + assert fn("warning: something harmless\n") is False + + +def test_restore_treats_existing_untracked_only_failure_as_restored( + monkeypatch, tmp_path, capsys +): + """stash apply rc=1 purely from already-present untracked files → restored, + stash dropped, no destructive reset.""" + calls = [] + + def fake_run(cmd, **kwargs): + calls.append((cmd, kwargs)) + if cmd[1:3] == ["stash", "apply"]: + return SimpleNamespace( + stdout="", + stderr=( + "packaging/homebrew/hermes-agent.rb already exists, no checkout\n" + "error: could not restore untracked files from stash\n" + ), + returncode=1, + ) + if cmd[1:3] == ["diff", "--name-only"]: + return SimpleNamespace(stdout="", stderr="", returncode=0) + if cmd[1:3] == ["stash", "list"]: + return SimpleNamespace(stdout="stash@{0} abc123\n", stderr="", returncode=0) + if cmd[1:3] == ["stash", "drop"]: + return SimpleNamespace(stdout="dropped\n", stderr="", returncode=0) + raise AssertionError(f"unexpected command: {cmd}") + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + + restored = hermes_main._restore_stashed_changes( + ["git"], tmp_path, "abc123", prompt_user=False + ) + + assert restored is True + # No reset --hard in the command stream. + assert not any("reset" in c for c, _ in calls) + out = capsys.readouterr().out + assert "kept as-is" in out + assert "hit conflicts" not in out + + +def test_update_autostash_survives_undeletable_untracked_dir(tmp_path): + """Behavioral E2E of the whole permission-denied class with real git: + root-owned-style undeletable untracked dir → stash succeeds, update-style + reset works, restore round-trips, nothing lost. (#70127 follow-up)""" + import os + import shutil + import subprocess + + if shutil.which("git") is None: + pytest.skip("git not available") + if os.name == "nt": + pytest.skip("POSIX permission semantics") + if os.geteuid() == 0: + pytest.skip("root ignores directory write bits") + + def git(*args, check=True): + return subprocess.run( + ["git", *args], cwd=tmp_path, capture_output=True, text=True, check=check + ) + + git("init", "-q", "-b", "main") + git("config", "user.email", "t@example.com") + git("config", "user.name", "t") + (tmp_path / "tracked.txt").write_text("v1\n") + git("add", "-A") + git("commit", "-qm", "init") + + (tmp_path / "tracked.txt").write_text("v2 local change\n") + pkg = tmp_path / "packaging" / "homebrew" + pkg.mkdir(parents=True) + (pkg / "hermes-agent.rb").write_text("formula\n") + os.chmod(pkg, 0o555) # undeletable contents, like a root-owned dir + try: + stash_ref = hermes_main._stash_local_changes_if_needed(["git"], tmp_path) + assert stash_ref + + # The tracked change is stashed; simulate the updater's checkout window. + assert (tmp_path / "tracked.txt").read_text() == "v1\n" + + restored = hermes_main._restore_stashed_changes( + ["git"], tmp_path, stash_ref, prompt_user=False + ) + assert restored is True + assert (tmp_path / "tracked.txt").read_text() == "v2 local change\n" + assert (pkg / "hermes-agent.rb").read_text() == "formula\n" + finally: + os.chmod(pkg, 0o755)