From e700e5ffdbf86c774c9c477a0485c4e1df0b944d Mon Sep 17 00:00:00 2001 From: unsupportedpastels Date: Tue, 29 Sep 2026 02:31:51 +0000 Subject: [PATCH] fix(checkpoints): honor Git pathspecs in nested repository guard Resolve selective restores against a disposable index before any checkpoint mutation, preserving unrelated-file recovery and refusing uncaptured gitlinks. Keep NUL-delimited filenames lossless, including surrogateescaped POSIX bytes. Add real-Git regression coverage and document the capture limitation. Fixes #127073 Salvages #127077 Co-authored-by: Yuan Li --- tests/tools/test_checkpoint_gitlinks.py | 143 ++++++++++++++++++ tools/checkpoint_manager.py | 33 +++- .../user-guide/checkpoints-and-rollback.md | 15 ++ 3 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 tests/tools/test_checkpoint_gitlinks.py diff --git a/tests/tools/test_checkpoint_gitlinks.py b/tests/tools/test_checkpoint_gitlinks.py new file mode 100644 index 0000000000..5a92c540e7 --- /dev/null +++ b/tests/tools/test_checkpoint_gitlinks.py @@ -0,0 +1,143 @@ +"""Real-Git rollback contracts for checkpoints containing uncaptured gitlinks.""" +import os +import shutil +import subprocess + +import pytest + +from tools import checkpoint_manager as cm + + +@pytest.fixture +def checkpoint(tmp_path, monkeypatch): + monkeypatch.setattr(cm, "CHECKPOINT_BASE", tmp_path / "checkpoints") + work = tmp_path / "workspace" + work.mkdir() + nested = work / "tool" + nested.mkdir() + env = {**os.environ, "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1"} + for args in (["init", "-q"], ["config", "user.name", "Test"], + ["config", "user.email", "test@example.invalid"]): + subprocess.run(["git", *args], cwd=nested, env=env, check=True, capture_output=True) + (nested / "main.py").write_text("committed\n") + subprocess.run(["git", "add", "."], cwd=nested, env=env, check=True, capture_output=True) + subprocess.run(["git", "commit", "-qm", "seed"], cwd=nested, env=env, check=True, capture_output=True) + (nested / "main.py").write_text("user work not captured\n") + (nested / "untracked.txt").write_text("not captured either\n") + (work / "notes.txt").write_text("before\n") + mgr = cm.CheckpointManager(enabled=True) + assert mgr.ensure_checkpoint(str(work), "baseline") + commit = mgr.list_checkpoints(str(work))[0]["hash"] + ok, tree, _ = cm._run_git(["ls-tree", "-r", "-z", commit], cm._store_path(), str(work)) + assert ok and "160000 commit " in tree + (work / "notes.txt").write_text("after\n") + (nested / "main.py").write_text("agent overwrite\n") + return mgr, work, commit + + +def durable_state(work): + """Include refs, indexes, ledger and working files, not transient lock files.""" + roots = [work, cm._store_path()] + return {(str(root), str(path.relative_to(root))): path.read_bytes() if path.is_file() else None + for root in roots for path in root.rglob("*")} + + +@pytest.mark.parametrize("safe", [False, True]) +@pytest.mark.parametrize("spec", [None, ".", "tool", "tool/", "tool/main.py", "*", + "t*", ":(glob)*", ":(top,glob)**", ":(icase)TOOL"]) +def test_refusal_is_nonmutating(checkpoint, safe, spec): + mgr, work, commit = checkpoint + before = durable_state(work) + result = mgr.restore(str(work), commit, file_path=spec, safe=safe) + assert result["success"] is False + assert result["nested_repositories"] == ["tool"] + assert "not captured" in result["error"] + assert durable_state(work) == before + + +@pytest.mark.parametrize("safe", [False, True]) +@pytest.mark.parametrize("spec", ["notes.txt", "*.txt", ":(glob)*.txt", ":(exclude)tool", + ":(literal)notes.txt"]) +def test_unrelated_selection_still_restores(checkpoint, safe, spec): + mgr, work, commit = checkpoint + result = mgr.restore(str(work), commit, file_path=spec, safe=safe) + assert result["success"] is True, result + assert (work / "notes.txt").read_text() == "before\n" + assert (work / "tool" / "main.py").read_text() == "agent overwrite\n" + + +@pytest.mark.parametrize("name", ["space name.txt", "unicode-雪.txt", + pytest.param("tab\tline\n.txt", marks=pytest.mark.skipif(os.name == "nt", reason="POSIX control characters")), + pytest.param(os.fsdecode(b"bad-\xff.txt"), + marks=pytest.mark.skipif(os.name != "posix", reason="POSIX filename bytes"))]) +def test_unusual_filename_does_not_break_tree_inspection(checkpoint, name): + mgr, work, _ = checkpoint + target = work / name + target.write_text("original\n") + mgr.new_turn() + assert mgr.ensure_checkpoint(str(work), "unusual filename") + commit = mgr.list_checkpoints(str(work))[0]["hash"] + target.write_text("changed\n") + result = mgr.restore(str(work), commit, file_path=name) + assert result["success"] is True, result + assert target.read_text() == "original\n" + + +@pytest.mark.parametrize("safe", [False, True]) +@pytest.mark.parametrize("spec", [None, "*", "tool/main.py"]) +def test_deleted_nested_repository_is_not_reported_restored(checkpoint, safe, spec): + mgr, work, commit = checkpoint + shutil.rmtree(work / "tool") + before = durable_state(work) + result = mgr.restore(str(work), commit, file_path=spec, safe=safe) + assert result["success"] is False + assert result["nested_repositories"] == ["tool"] + assert not (work / "tool").exists() + assert durable_state(work) == before + + +@pytest.mark.parametrize("name", ["nested space", "nested-雪", "vendor/tool", + pytest.param("nested\tline\n", marks=pytest.mark.skipif(os.name == "nt", reason="POSIX control characters")), + pytest.param(os.fsdecode(b"nested-\xff"), + marks=pytest.mark.skipif(os.name != "posix", reason="POSIX filename bytes"))]) +def test_gitlink_name_roundtrips_in_refusal(checkpoint, name): + mgr, work, _ = checkpoint + (work / name).parent.mkdir(parents=True, exist_ok=True) + (work / "tool").rename(work / name) + mgr.new_turn() + assert mgr.ensure_checkpoint(str(work), "renamed nested repository") + commit = mgr.list_checkpoints(str(work))[0]["hash"] + before = durable_state(work) + result = mgr.restore(str(work), commit, file_path="*") + assert result["success"] is False + assert result["nested_repositories"] == [name] + assert durable_state(work) == before + + +@pytest.mark.parametrize("spec", [":(literal)tool/main.py", ":(glob)tool/*.py", ":(invalid)tool"]) +def test_unresolvable_selection_is_nonmutating(checkpoint, spec): + mgr, work, commit = checkpoint + before = durable_state(work) + result = mgr.restore(str(work), commit, file_path=spec) + assert result["success"] is False + assert durable_state(work) == before + + +@pytest.mark.parametrize("command", ["read-tree", "ls-files"]) +def test_inspection_failure_leaves_files_and_history_untouched(checkpoint, monkeypatch, command): + mgr, work, commit = checkpoint + run_git = cm._run_git + + def fail_inspection(args, *a, **kw): + if args[0] == command: + return False, "", "inspection unavailable" + return run_git(args, *a, **kw) + + monkeypatch.setattr(cm, "_run_git", fail_inspection) + before = durable_state(work) + result = mgr.restore(str(work), commit, file_path="*") + assert result["success"] is False + assert "inspection unavailable" in result["error"] + assert durable_state(work) == before + assert not list(cm._store_path().glob("restore-inspect-*")) diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index 91d212c3fb..269e52b9d3 100644 --- a/tools/checkpoint_manager.py +++ b/tools/checkpoint_manager.py @@ -55,6 +55,7 @@ import os import re import shutil import subprocess +import tempfile import time from pathlib import Path, PurePosixPath from hermes_constants import get_hermes_home @@ -1149,9 +1150,9 @@ class CheckpointManager: if not ok: return {"success": False, "error": f"Could not inspect checkpoint: {err}"} nested_repos = [ - record.split(b"\t", 1)[1].decode(errors="replace") - for record in tree_out.encode().split(b"\x00") - if record.startswith(b"160000 commit ") and b"\t" in record + record.split("\t", 1)[1] + for record in tree_out.split("\x00") + if record.startswith("160000 commit ") and "\t" in record ] if nested_repos: blocked_repos = nested_repos @@ -1176,6 +1177,32 @@ class CheckpointManager: if _scope_intersects_nested_repo(repo) ] + # Literal paths below a gitlink have no tree entry to match. + # Keep the explicit refusal above, but let Git resolve all + # other selections exactly as checkout does (glob, icase, + # top, exclude, literal, etc.). ls-tree lacks that pathspec + # support. A disposable index avoids touching the project's + # index, refs or files before we know rollback is possible. + if not blocked_repos: + with tempfile.TemporaryDirectory(prefix="restore-inspect-", dir=store) as scratch: + inspect_index = Path(scratch) / "index" + ok, _, err = _run_git( + ["read-tree", commit_hash], store, abs_dir, + index_file=inspect_index, + ) + if ok: + ok, selected, err = _run_git( + ["ls-files", "--stage", "--error-unmatch", "-z", "--", file_path], + store, abs_dir, index_file=inspect_index, + ) + if not ok: + return {"success": False, "error": f"Could not inspect restore selection: {err}"} + blocked_repos = [ + record.split("\t", 1)[1] + for record in selected.split("\x00") + if record.startswith("160000 ") and "\t" in record + ] + if blocked_repos: paths = ", ".join(blocked_repos) return { diff --git a/website/docs/user-guide/checkpoints-and-rollback.md b/website/docs/user-guide/checkpoints-and-rollback.md index dd732f53aa..edf11cfef2 100644 --- a/website/docs/user-guide/checkpoints-and-rollback.md +++ b/website/docs/user-guide/checkpoints-and-rollback.md @@ -234,6 +234,21 @@ Restore just one file from a checkpoint without affecting the rest of the direct ## Safety and Performance Guards +### Nested Git repositories + +A checkpoint of a parent directory can store a nested repository as a Git +**gitlink** (a commit reference), not a copy of its files. Recursive capture of +nested repositories is not supported: their uncommitted edits and untracked +files are not recoverable from that parent checkpoint. + +If the selected checkpoint contains gitlinks, a full rollback (including +`--all`) is refused before changing files or creating a pre-rollback snapshot. +Selecting a nested repository, a file below it, or a Git pathspec matching it +also refuses the restore rather than reporting success for uncaptured files. +You can still restore unrelated captured files, for example +`/rollback 1 notes.txt`. Keep separate backups or checkpoints taken directly +from the nested repository's own working directory. + ### Container Backends With a container terminal backend (`docker`, `singularity`, `modal`, `daytona`, `vercel_sandbox`, or a container plugin), file paths belong to the sandbox rather than the host. Hermes therefore does not take checkpoints or record the agent-write ledger for those paths, and `/rollback` explains the limitation: it still lists existing host checkpoints but refuses diff and restore, on the CLI and in messaging-gateway chats alike; `/diff session` answers with the same reason. The TUI and Desktop behave the same: `/rollback list` still works while `/rollback diff` and `/rollback ` are refused with that reason. Local and SSH backends are unaffected. To point `terminal.cwd` at the container-side view of a mounted directory see [`terminal.docker_mount_cwd_to_workspace`](./configuration.md).