mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(update): survive undeletable untracked files during autostash (#70161)
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.
This commit is contained in:
Teknium
GitHub
parent
76d4b65d59
commit
fdd3943cb1
2 changed files with 315 additions and 10 deletions
|
|
@ -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())
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue