mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(kanban): preserve scratch completion artifacts
This commit is contained in:
Kevin Yin
Teknium
parent
5d524d0427
commit
e6c42b5d80
2 changed files with 260 additions and 48 deletions
|
|
@ -4080,6 +4080,8 @@ def complete_task(
|
|||
)
|
||||
if cur.rowcount != 1:
|
||||
return False
|
||||
if isinstance(metadata, dict):
|
||||
_persist_scratch_completion_artifacts(conn, task_id, metadata)
|
||||
run_id = _end_run(
|
||||
conn, task_id,
|
||||
outcome="completed", status="done",
|
||||
|
|
@ -4174,6 +4176,143 @@ def complete_task(
|
|||
# Workspace / tmux cleanup
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _persist_scratch_completion_artifacts(
|
||||
conn: sqlite3.Connection,
|
||||
task_id: str,
|
||||
metadata: dict,
|
||||
) -> None:
|
||||
"""Copy scratch-workspace completion artifacts before cleanup removes them."""
|
||||
raw_artifacts = metadata.get("artifacts")
|
||||
if not isinstance(raw_artifacts, (list, tuple)):
|
||||
return
|
||||
|
||||
row = conn.execute(
|
||||
"SELECT workspace_kind, workspace_path FROM tasks WHERE id = ?",
|
||||
(task_id,),
|
||||
).fetchone()
|
||||
if not row or row["workspace_kind"] != "scratch" or not row["workspace_path"]:
|
||||
return
|
||||
|
||||
workspace = Path(row["workspace_path"]).expanduser()
|
||||
is_managed, board = _managed_scratch_path_info(workspace)
|
||||
if not is_managed:
|
||||
return
|
||||
|
||||
try:
|
||||
workspace_root = workspace.resolve()
|
||||
except OSError:
|
||||
return
|
||||
|
||||
attachment_dir = task_attachments_dir(task_id, board=board)
|
||||
persisted: list[str] = []
|
||||
used_destinations: set[Path] = set()
|
||||
changed = False
|
||||
|
||||
for item in raw_artifacts:
|
||||
artifact = str(item).strip() if isinstance(item, str) else ""
|
||||
if not artifact:
|
||||
continue
|
||||
src = Path(artifact).expanduser()
|
||||
try:
|
||||
resolved_src = src.resolve()
|
||||
except OSError:
|
||||
persisted.append(artifact)
|
||||
continue
|
||||
|
||||
if not src.is_file() or not resolved_src.is_relative_to(workspace_root):
|
||||
persisted.append(artifact)
|
||||
continue
|
||||
|
||||
try:
|
||||
attachment_dir.mkdir(parents=True, exist_ok=True)
|
||||
dest = _unique_attachment_path(attachment_dir, resolved_src.name, used_destinations)
|
||||
shutil.copy2(resolved_src, dest)
|
||||
except Exception as exc:
|
||||
_log.debug(
|
||||
"Failed to persist completion artifact for task %s: %s",
|
||||
task_id,
|
||||
exc,
|
||||
)
|
||||
persisted.append(artifact)
|
||||
continue
|
||||
|
||||
used_destinations.add(dest)
|
||||
persisted.append(str(dest))
|
||||
changed = True
|
||||
|
||||
if changed:
|
||||
metadata["artifacts"] = persisted
|
||||
|
||||
|
||||
def _unique_attachment_path(directory: Path, filename: str, used: set[Path]) -> Path:
|
||||
"""Return a non-conflicting path under ``directory`` for ``filename``."""
|
||||
safe_name = Path(filename).name or "artifact"
|
||||
candidate = directory / safe_name
|
||||
if candidate not in used and not candidate.exists():
|
||||
return candidate
|
||||
|
||||
stem = Path(safe_name).stem or "artifact"
|
||||
suffix = Path(safe_name).suffix
|
||||
idx = 1
|
||||
while True:
|
||||
candidate = directory / f"{stem}_{idx}{suffix}"
|
||||
if candidate not in used and not candidate.exists():
|
||||
return candidate
|
||||
idx += 1
|
||||
|
||||
|
||||
def _managed_scratch_path_info(p: Path) -> tuple[bool, Optional[str]]:
|
||||
"""Return whether *p* is managed scratch storage and the matching board."""
|
||||
try:
|
||||
p_abs = p.resolve(strict=False)
|
||||
except OSError:
|
||||
return False, None
|
||||
roots: list[tuple[Path, Optional[str]]] = []
|
||||
override = os.environ.get("HERMES_KANBAN_WORKSPACES_ROOT", "").strip()
|
||||
if override:
|
||||
try:
|
||||
roots.append((Path(override).expanduser().resolve(strict=False), None))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
home = kanban_home()
|
||||
except OSError:
|
||||
home = None
|
||||
if home is not None:
|
||||
try:
|
||||
roots.append(((home / "kanban" / "workspaces").resolve(strict=False), DEFAULT_BOARD))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
boards_parent = (home / "kanban" / "boards").resolve(strict=False)
|
||||
except OSError:
|
||||
boards_parent = None
|
||||
if boards_parent is not None:
|
||||
try:
|
||||
entries = list(boards_parent.iterdir())
|
||||
except OSError:
|
||||
entries = []
|
||||
for entry in entries:
|
||||
try:
|
||||
if not entry.is_dir():
|
||||
continue
|
||||
except OSError:
|
||||
continue
|
||||
try:
|
||||
roots.append(((entry / "workspaces").resolve(strict=False), entry.name))
|
||||
except OSError:
|
||||
continue
|
||||
for root, board in roots:
|
||||
if p_abs == root:
|
||||
continue
|
||||
try:
|
||||
if p_abs.is_relative_to(root):
|
||||
return True, board
|
||||
except ValueError:
|
||||
continue
|
||||
return False, None
|
||||
|
||||
|
||||
def _is_managed_scratch_path(p: Path) -> bool:
|
||||
"""Return True iff *p* is a strict descendant of a kanban-managed scratch root.
|
||||
|
||||
|
|
@ -4199,54 +4338,8 @@ def _is_managed_scratch_path(p: Path) -> bool:
|
|||
real source tree can otherwise pair with ``workspace_kind='scratch'`` and
|
||||
cause task completion to delete user data (#28818).
|
||||
"""
|
||||
try:
|
||||
p_abs = p.resolve(strict=False)
|
||||
except OSError:
|
||||
return False
|
||||
roots: list[Path] = []
|
||||
override = os.environ.get("HERMES_KANBAN_WORKSPACES_ROOT", "").strip()
|
||||
if override:
|
||||
try:
|
||||
roots.append(Path(override).expanduser().resolve(strict=False))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
home = kanban_home()
|
||||
except OSError:
|
||||
home = None
|
||||
if home is not None:
|
||||
try:
|
||||
roots.append((home / "kanban" / "workspaces").resolve(strict=False))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
boards_parent = (home / "kanban" / "boards").resolve(strict=False)
|
||||
except OSError:
|
||||
boards_parent = None
|
||||
if boards_parent is not None:
|
||||
try:
|
||||
entries = list(boards_parent.iterdir())
|
||||
except OSError:
|
||||
entries = []
|
||||
for entry in entries:
|
||||
try:
|
||||
if not entry.is_dir():
|
||||
continue
|
||||
except OSError:
|
||||
continue
|
||||
try:
|
||||
roots.append((entry / "workspaces").resolve(strict=False))
|
||||
except OSError:
|
||||
continue
|
||||
for root in roots:
|
||||
if p_abs == root:
|
||||
continue
|
||||
try:
|
||||
if p_abs.is_relative_to(root):
|
||||
return True
|
||||
except ValueError:
|
||||
continue
|
||||
return False
|
||||
is_managed, _board = _managed_scratch_path_info(p)
|
||||
return is_managed
|
||||
|
||||
|
||||
def _cleanup_workspace(conn: sqlite3.Connection, task_id: str) -> None:
|
||||
|
|
|
|||
|
|
@ -2363,6 +2363,125 @@ def test_cleanup_workspace_removes_managed_scratch_dir(kanban_home):
|
|||
assert not ws.exists(), "Hermes-managed scratch dir should be cleaned up"
|
||||
|
||||
|
||||
def test_complete_task_persists_scratch_artifacts_before_cleanup(kanban_home):
|
||||
"""Completion artifacts from scratch workspaces survive workspace cleanup."""
|
||||
with kb.connect() as conn:
|
||||
t = kb.create_task(conn, title="render chart")
|
||||
task = kb.get_task(conn, t)
|
||||
ws = kb.resolve_workspace(task)
|
||||
kb.set_workspace_path(conn, t, ws)
|
||||
artifact = ws / "chart.png"
|
||||
artifact.write_bytes(b"png-bytes")
|
||||
|
||||
assert kb.complete_task(
|
||||
conn,
|
||||
t,
|
||||
result="ok",
|
||||
metadata={"artifacts": [str(artifact)]},
|
||||
)
|
||||
|
||||
completed = [e for e in kb.list_events(conn, t) if e.kind == "completed"][-1]
|
||||
persisted = Path(completed.payload["artifacts"][0])
|
||||
run = kb.latest_run(conn, t)
|
||||
|
||||
assert not ws.exists(), "scratch workspace should still be cleaned up"
|
||||
assert persisted.exists(), "artifact copy should survive scratch cleanup"
|
||||
assert persisted.parent == kb.task_attachments_dir(t)
|
||||
assert persisted.name == "chart.png"
|
||||
assert persisted.read_bytes() == b"png-bytes"
|
||||
assert str(persisted) != str(artifact)
|
||||
assert run is not None
|
||||
assert run.metadata["artifacts"] == [str(persisted)]
|
||||
|
||||
|
||||
def test_complete_task_leaves_non_scratch_artifact_paths_unchanged(
|
||||
kanban_home,
|
||||
tmp_path,
|
||||
):
|
||||
"""Only artifacts inside the managed scratch workspace are copied."""
|
||||
external = tmp_path / "report.md"
|
||||
external.write_text("keep me here", encoding="utf-8")
|
||||
|
||||
with kb.connect() as conn:
|
||||
t = kb.create_task(conn, title="external report")
|
||||
task = kb.get_task(conn, t)
|
||||
ws = kb.resolve_workspace(task)
|
||||
kb.set_workspace_path(conn, t, ws)
|
||||
|
||||
assert kb.complete_task(
|
||||
conn,
|
||||
t,
|
||||
result="ok",
|
||||
metadata={"artifacts": [str(external)]},
|
||||
)
|
||||
|
||||
completed = [e for e in kb.list_events(conn, t) if e.kind == "completed"][-1]
|
||||
run = kb.latest_run(conn, t)
|
||||
|
||||
assert not ws.exists(), "scratch workspace should still be cleaned up"
|
||||
assert external.exists()
|
||||
assert completed.payload["artifacts"] == [str(external)]
|
||||
assert run is not None
|
||||
assert run.metadata["artifacts"] == [str(external)]
|
||||
|
||||
|
||||
def test_complete_task_persists_duplicate_scratch_artifact_names(kanban_home):
|
||||
"""Scratch artifact persistence does not overwrite duplicate basenames."""
|
||||
with kb.connect() as conn:
|
||||
t = kb.create_task(conn, title="render reports")
|
||||
task = kb.get_task(conn, t)
|
||||
ws = kb.resolve_workspace(task)
|
||||
kb.set_workspace_path(conn, t, ws)
|
||||
first = ws / "a" / "report.txt"
|
||||
second = ws / "b" / "report.txt"
|
||||
first.parent.mkdir(parents=True)
|
||||
second.parent.mkdir(parents=True)
|
||||
first.write_text("first", encoding="utf-8")
|
||||
second.write_text("second", encoding="utf-8")
|
||||
|
||||
assert kb.complete_task(
|
||||
conn,
|
||||
t,
|
||||
result="ok",
|
||||
metadata={"artifacts": [str(first), str(second)]},
|
||||
)
|
||||
|
||||
completed = [e for e in kb.list_events(conn, t) if e.kind == "completed"][-1]
|
||||
persisted = [Path(p) for p in completed.payload["artifacts"]]
|
||||
|
||||
assert not ws.exists(), "scratch workspace should still be cleaned up"
|
||||
assert [p.name for p in persisted] == ["report.txt", "report_1.txt"]
|
||||
assert [p.read_text(encoding="utf-8") for p in persisted] == ["first", "second"]
|
||||
assert all(p.parent == kb.task_attachments_dir(t) for p in persisted)
|
||||
|
||||
|
||||
def test_complete_task_persists_board_scratch_artifacts_to_board_attachments(kanban_home):
|
||||
"""Board scratch artifacts are copied under that board's attachment root."""
|
||||
kb.create_board("work-proj")
|
||||
|
||||
with kb.connect(board="work-proj") as conn:
|
||||
t = kb.create_task(conn, title="board chart", board="work-proj")
|
||||
task = kb.get_task(conn, t)
|
||||
ws = kb.resolve_workspace(task, board="work-proj")
|
||||
kb.set_workspace_path(conn, t, ws)
|
||||
artifact = ws / "chart.png"
|
||||
artifact.write_bytes(b"board-png")
|
||||
|
||||
assert kb.complete_task(
|
||||
conn,
|
||||
t,
|
||||
result="ok",
|
||||
metadata={"artifacts": [str(artifact)]},
|
||||
)
|
||||
|
||||
completed = [e for e in kb.list_events(conn, t) if e.kind == "completed"][-1]
|
||||
persisted = Path(completed.payload["artifacts"][0])
|
||||
|
||||
assert not ws.exists(), "board scratch workspace should still be cleaned up"
|
||||
assert persisted.exists()
|
||||
assert persisted.parent == kb.task_attachments_dir(t, board="work-proj")
|
||||
|
||||
|
||||
def test_cleanup_workspace_refuses_path_outside_scratch_root(kanban_home, tmp_path):
|
||||
"""A scratch task with a user path outside the workspaces root must NOT be deleted (#28818).
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue