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 <dskwelmcy@163.com>
This commit is contained in:
unsupportedpastels
committed by
Mark S.
Mark S.
parent
19714b1d93
commit
e700e5ffdb
143
tests/tools/test_checkpoint_gitlinks.py
Normal file
143
tests/tools/test_checkpoint_gitlinks.py
Normal file
@@ -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-*"))
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 <N>` 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).
|
||||
|
||||
Reference in New Issue
Block a user