diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 518e74eb0647..c7c894493bbe 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -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: diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index a9e61f41690a..7e9f99e2ceb0 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -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).