From 8030b01a2ad6b82ee4c0434ba6f9726d0d1de380 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:50:49 -0700 Subject: [PATCH] fix(kanban): harden durable artifact handoff --- gateway/platforms/base.py | 23 +++ hermes_cli/kanban_db.py | 151 ++++++++++++++++-- plugins/kanban/dashboard/plugin_api.py | 2 +- tests/gateway/test_platform_base.py | 22 +++ tests/hermes_cli/test_kanban_db.py | 50 ++++++ tests/tools/test_kanban_tools.py | 25 +++ tools/kanban_tools.py | 13 +- website/docs/user-guide/features/kanban.md | 2 +- .../current/user-guide/features/kanban.md | 2 +- 9 files changed, 275 insertions(+), 15 deletions(-) diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index 21ca4af6bb4d..14a7bc336972 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -1072,10 +1072,33 @@ def _profile_cache_roots() -> List[Path]: return roots +def _kanban_attachment_roots() -> List[Path]: + """Return durable Kanban attachment roots without importing kanban_db.""" + override = os.environ.get("HERMES_KANBAN_ATTACHMENTS_ROOT", "").strip() + if override: + return [Path(override).expanduser()] + home_override = os.environ.get("HERMES_KANBAN_HOME", "").strip() + root = Path(home_override).expanduser() if home_override else _HERMES_ROOT + roots = [root / "kanban" / "attachments"] + boards_root = root / "kanban" / "boards" + try: + board_dirs = [ + path for path in boards_root.iterdir() + if path.is_dir() and not path.is_symlink() + and re.fullmatch(r"[a-z0-9][a-z0-9_-]{0,63}", path.name) + and (path / "kanban.db").is_file() + ] + except OSError: + return roots + roots.extend(path / "attachments" for path in board_dirs) + return roots + + def _media_delivery_allowed_roots() -> List[Path]: """Return roots from which model-emitted local media may be delivered.""" roots = [Path(root) for root in MEDIA_DELIVERY_SAFE_ROOTS] roots.extend(_profile_cache_roots()) + roots.extend(_kanban_attachment_roots()) extra_roots = os.environ.get(MEDIA_DELIVERY_ALLOW_DIRS_ENV, "") for chunk in extra_roots.split(os.pathsep): for raw_root in chunk.split(","): diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c7c894493bbe..7cee6feaf3c6 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -135,6 +135,7 @@ BLOCK_RECURRENCE_LIMIT = 2 VALID_WORKSPACE_KINDS = {"scratch", "worktree", "dir"} KNOWN_TOOLSET_NAMES = frozenset(name.casefold() for name in get_toolset_names()) _IS_WINDOWS = sys.platform == "win32" +KANBAN_ATTACHMENT_MAX_BYTES = 25 * 1024 * 1024 def _fire_kanban_lifecycle_hook(event: str, task_id: str, **fields: Any) -> None: @@ -3975,6 +3976,10 @@ class HallucinatedCardsError(ValueError): ) +class ArtifactPreservationError(RuntimeError): + """Raised when a declared scratch deliverable cannot be preserved.""" + + def complete_task( conn: sqlite3.Connection, task_id: str, @@ -4042,6 +4047,9 @@ def complete_task( else: verified_cards = [] + metadata = _merge_completion_prose_artifacts( + conn, task_id, metadata, summary=summary, result=result, + ) with write_txn(conn): if expected_run_id is None: cur = conn.execute( @@ -4082,6 +4090,16 @@ def complete_task( return False if isinstance(metadata, dict): _persist_scratch_completion_artifacts(conn, task_id, metadata) + for stored_path in metadata.pop("_staged_artifacts", []): + path = Path(stored_path) + _insert_completion_attachment( + conn, + task_id, + filename=path.name, + stored_path=str(path), + size=path.stat().st_size, + created_at=now, + ) run_id = _end_run( conn, task_id, outcome="completed", status="done", @@ -4176,6 +4194,54 @@ def complete_task( # Workspace / tmux cleanup # --------------------------------------------------------------------------- + +def _merge_completion_prose_artifacts( + conn: sqlite3.Connection, + task_id: str, + metadata: Optional[dict], + *, + summary: Optional[str], + result: Optional[str], +) -> Optional[dict]: + """Promote existing scratch files named in legacy completion prose. + + ``artifacts=[...]`` is preferred. Older workers only wrote an absolute + deliverable path in ``summary``/``result``; discover it while scratch still + exists so cleanup cannot erase the file the user was promised. + """ + 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 metadata + workspace = Path(row["workspace_path"]).expanduser() + if not _is_managed_scratch_path(workspace): + return metadata + text = "\n".join(part for part in (summary, result) if part) + if not text: + return metadata + prefix = re.escape(str(workspace)) + discovered: list[str] = [] + for match in re.finditer(prefix + r"(?:[/\\][^\s`\"'<>]+)", text): + raw = match.group(0).rstrip(".,;:!?)]}") + candidate = Path(raw) + if candidate.is_file(): + discovered.append(str(candidate)) + if not discovered: + return metadata + updated = dict(metadata) if isinstance(metadata, dict) else {} + existing = updated.get("artifacts") + merged = list(existing) if isinstance(existing, (list, tuple)) else [] + seen = {str(path) for path in merged} + for path in discovered: + if path not in seen: + merged.append(path) + seen.add(path) + updated["artifacts"] = merged + return updated + + def _persist_scratch_completion_artifacts( conn: sqlite3.Connection, task_id: str, @@ -4208,6 +4274,17 @@ def _persist_scratch_completion_artifacts( used_destinations: set[Path] = set() changed = False + def _discard_copies() -> None: + for copied in used_destinations: + try: + copied.unlink(missing_ok=True) + except OSError: + pass + try: + attachment_dir.rmdir() + except OSError: + pass + for item in raw_artifacts: artifact = str(item).strip() if isinstance(item, str) else "" if not artifact: @@ -4219,29 +4296,83 @@ def _persist_scratch_completion_artifacts( persisted.append(artifact) continue - if not src.is_file() or not resolved_src.is_relative_to(workspace_root): + if not resolved_src.is_relative_to(workspace_root): persisted.append(artifact) continue + if not src.is_file(): + _discard_copies() + raise ArtifactPreservationError( + f"declared scratch artifact is unavailable or not a regular file: {artifact}" + ) + + size = resolved_src.stat().st_size + if size > KANBAN_ATTACHMENT_MAX_BYTES: + _discard_copies() + raise ArtifactPreservationError( + f"declared scratch artifact exceeds the " + f"{KANBAN_ATTACHMENT_MAX_BYTES}-byte limit: {artifact}" + ) + + dest: Optional[Path] = None 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) + with resolved_src.open("rb") as source_file, dest.open("xb") as destination_file: + copied = 0 + while chunk := source_file.read(1024 * 1024): + copied += len(chunk) + if copied > KANBAN_ATTACHMENT_MAX_BYTES: + raise ArtifactPreservationError( + f"declared scratch artifact grew beyond the size limit: {artifact}" + ) + destination_file.write(chunk) except Exception as exc: - _log.debug( - "Failed to persist completion artifact for task %s: %s", - task_id, - exc, - ) - persisted.append(artifact) - continue + if dest is not None: + try: + dest.unlink(missing_ok=True) + except OSError: + pass + _discard_copies() + if isinstance(exc, ArtifactPreservationError): + raise + raise ArtifactPreservationError( + f"could not preserve declared scratch artifact {artifact}: {exc}" + ) from exc used_destinations.add(dest) - persisted.append(str(dest)) + persisted.append(str(dest.resolve())) changed = True if changed: metadata["artifacts"] = persisted + metadata["_staged_artifacts"] = [ + path for path in persisted if path.startswith(str(attachment_dir.resolve())) + ] + + +def _insert_completion_attachment( + conn: sqlite3.Connection, + task_id: str, + *, + filename: str, + stored_path: str, + size: int, + created_at: int, +) -> None: + """Record a worker-produced artifact in the existing attachment table.""" + conn.execute( + "INSERT INTO task_attachments " + "(task_id, filename, stored_path, content_type, size, uploaded_by, created_at) " + "VALUES (?, ?, ?, NULL, ?, 'kanban_complete', ?)", + (task_id, filename, stored_path, size, created_at), + ) + _append_event( + conn, + task_id, + "attached", + {"filename": filename, "size": size, "by": "kanban_complete"}, + ) def _unique_attachment_path(directory: Path, filename: str, used: set[Path]) -> Path: diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index 428f372384d6..793f9ab98182 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -646,7 +646,7 @@ def create_task(payload: CreateTaskBody, board: Optional[str] = Query(None)): # Cap a single upload so a runaway request can't fill the disk. 25 MB # comfortably covers PDFs, images, and source docs — the kanban use case. -_MAX_ATTACHMENT_BYTES = 25 * 1024 * 1024 +_MAX_ATTACHMENT_BYTES = kanban_db.KANBAN_ATTACHMENT_MAX_BYTES def _safe_attachment_name(raw: str) -> str: diff --git a/tests/gateway/test_platform_base.py b/tests/gateway/test_platform_base.py index 133f50ee3591..42e750403597 100644 --- a/tests/gateway/test_platform_base.py +++ b/tests/gateway/test_platform_base.py @@ -786,6 +786,28 @@ class TestMediaDeliveryPathValidation: assert BasePlatformAdapter.validate_media_delivery_path(str(media_file)) == str(media_file.resolve()) + def test_allows_stale_kanban_attachment_but_not_neighboring_workspace( + self, tmp_path, monkeypatch, + ): + """Strict mode trusts durable attachments without trusting scratch.""" + self._patch_roots(monkeypatch) + monkeypatch.setenv("HERMES_KANBAN_HOME", str(tmp_path / "hermes")) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + board_root = tmp_path / "hermes" / "kanban" / "boards" / "research" + board_root.mkdir(parents=True) + (board_root / "kanban.db").touch() + attachment = board_root / "attachments" / "t_12345678" / "report.pdf" + scratch = board_root / "workspaces" / "t_12345678" / "notes.txt" + attachment.parent.mkdir(parents=True) + scratch.parent.mkdir(parents=True) + attachment.write_bytes(b"%PDF") + scratch.write_text("private", encoding="utf-8") + + assert BasePlatformAdapter.validate_media_delivery_path(str(attachment)) == str( + attachment.resolve() + ) + assert BasePlatformAdapter.validate_media_delivery_path(str(scratch)) is None + def test_recency_trust_allows_freshly_produced_file(self, tmp_path, monkeypatch): """A PDF the agent just wrote to /tmp should be deliverable. diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 7e9f99e2ceb0..25ed7223129f 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -2392,6 +2392,56 @@ def test_complete_task_persists_scratch_artifacts_before_cleanup(kanban_home): assert str(persisted) != str(artifact) assert run is not None assert run.metadata["artifacts"] == [str(persisted)] + with kb.connect() as conn: + attachments = kb.list_attachments(conn, t) + assert [(a.filename, a.stored_path) for a in attachments] == [ + ("chart.png", str(persisted.resolve())) + ] + + +def test_complete_task_rejects_missing_declared_scratch_artifact(kanban_home): + """A declared scratch deliverable must not disappear behind a false Done.""" + with kb.connect() as conn: + t = kb.create_task(conn, title="missing report") + task = kb.get_task(conn, t) + ws = kb.resolve_workspace(task) + kb.set_workspace_path(conn, t, ws) + missing = ws / "report.md" + + with pytest.raises(kb.ArtifactPreservationError, match="unavailable"): + kb.complete_task( + conn, + t, + result="report complete", + metadata={"artifacts": [str(missing)]}, + ) + + assert kb.get_task(conn, t).status == "ready" + assert kb.list_attachments(conn, t) == [] + assert ws.exists(), "failed completion must keep scratch available for retry" + + +def test_complete_task_preserves_legacy_artifact_path_from_summary(kanban_home): + """Summary-only workers keep the file they tell the user was delivered.""" + with kb.connect() as conn: + t = kb.create_task(conn, title="legacy report") + task = kb.get_task(conn, t) + ws = kb.resolve_workspace(task) + kb.set_workspace_path(conn, t, ws) + report = ws / "report.md" + report.write_text("legacy deliverable", encoding="utf-8") + + assert kb.complete_task( + conn, + t, + summary=f"Task complete — delivered {report}", + ) + run = kb.latest_run(conn, t) + + persisted = Path(run.metadata["artifacts"][0]) + assert not ws.exists() + assert persisted.read_text(encoding="utf-8") == "legacy deliverable" + assert persisted.parent == kb.task_attachments_dir(t) def test_complete_task_leaves_non_scratch_artifact_paths_unchanged( diff --git a/tests/tools/test_kanban_tools.py b/tests/tools/test_kanban_tools.py index cdf8c4cdb5da..5b1d9f6f7556 100644 --- a/tests/tools/test_kanban_tools.py +++ b/tests/tools/test_kanban_tools.py @@ -484,6 +484,31 @@ def test_complete_rejects_non_list_artifacts(worker_env): assert "artifacts must be a list" in err +def test_complete_missing_scratch_artifact_stays_in_flight(worker_env): + """A false deliverable claim must return retry guidance, not mark Done.""" + from hermes_cli import kanban_db as kb + from tools import kanban_tools as kt + + with kb.connect() as conn: + task = kb.get_task(conn, worker_env) + assert task is not None + workspace = kb.resolve_workspace(task) + kb.set_workspace_path(conn, worker_env, workspace) + + output = kt._handle_complete({ + "summary": "report complete", + "artifacts": [str(workspace / "missing-report.md")], + }) + error = json.loads(output).get("error", "") + + assert "could not preserve" in error + assert "still in-flight" in error + assert "retry kanban_complete" in error + with kb.connect() as conn: + assert kb.get_task(conn, worker_env).status == "running" + assert workspace.exists() + + def test_complete_rejects_no_handoff(worker_env): from tools import kanban_tools as kt out = kt._handle_complete({}) diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 2e81f94429e0..5207c756aa6c 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -630,6 +630,13 @@ def _handle_complete(args: dict, **kw) -> str: created_cards=created_cards, expected_run_id=_worker_run_id(tid), ) + except kb.ArtifactPreservationError as artifact_err: + return tool_error( + f"kanban_complete could not preserve the declared artifacts: " + f"{artifact_err}. Your task is still in-flight and its " + f"scratch workspace was kept. Fix the artifact path or " + f"storage error, then retry kanban_complete with the same handoff." + ) except kb.HallucinatedCardsError as hall_err: # Structured rejection — surface the phantom ids so the # worker can retry with a corrected list or drop the @@ -1277,8 +1284,10 @@ KANBAN_COMPLETE_SCHEMA = { "lands with the completion notification. Skip " "intermediate scratch files and references that " "are not the deliverable. The path must exist " - "on disk when the notifier runs; missing files " - "are silently skipped." + "on disk at completion. Files inside a managed scratch " + "workspace are copied to durable task attachments before " + "cleanup; a missing declared scratch artifact keeps the " + "task in-flight so you can fix the path and retry." ), }, "board": _board_schema_prop(), diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index bd8bd27e3bfe..b505ccc9bf4a 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -63,7 +63,7 @@ They coexist: a kanban worker may call `delegate_task` internally during its run - **Link** — `task_links` row recording a parent → child dependency. The dispatcher promotes `todo → ready` when all parents are `done`. - **Comment** — the inter-agent protocol. Agents and humans append comments; when a worker is (re-)spawned it reads the full comment thread as part of its context. - **Workspace** — the directory a worker operates in. Three kinds: - - `scratch` (default) — fresh tmp dir under `~/.hermes/kanban/workspaces//` (or `~/.hermes/kanban/boards//workspaces//` on non-default boards). **Deleted when the task completes** — scratch is ephemeral by design, so the dir is wiped the moment the worker (or `hermes kanban complete `) marks the task done. If you want to keep the worker's output, use `worktree:` or `dir:` instead. The first time a scratch workspace is created on an install, the dispatcher logs a warning and emits a `tip_scratch_workspace` event on the task (visible via `hermes kanban show `). + - `scratch` (default) — fresh tmp dir under `~/.hermes/kanban/workspaces//` (or `~/.hermes/kanban/boards//workspaces//` on non-default boards). **Deleted when the task completes** — scratch is ephemeral by design. Files explicitly declared through `kanban_complete(artifacts=[...])` are copied into durable per-task attachment storage before cleanup; existing deliverable paths in legacy completion summaries receive the same treatment. Other scratch files are removed. A missing declared scratch artifact keeps the task in-flight so the worker can correct the path and retry. Use `worktree:` or `dir:` when the whole workspace should remain available. The first time a scratch workspace is created on an install, the dispatcher logs a warning and emits a `tip_scratch_workspace` event on the task (visible via `hermes kanban show `). - `dir:` — an existing shared directory (Obsidian vault, mail ops dir, per-account folder). **Must be an absolute path.** Relative paths like `dir:../tenants/foo/` are rejected at dispatch because they'd resolve against whatever CWD the dispatcher happens to be in, which is ambiguous and a confused-deputy escape vector. The path is otherwise trusted — it's your box, your filesystem, the worker runs with your uid. This is the trusted-local-user threat model; kanban is single-host by design. **Preserved on completion.** - `worktree` — a git worktree under `.worktrees//` for coding tasks. Use `worktree:` to pin the exact target path. Worker-side `git worktree add` creates it, using `--branch` when provided. **Preserved on completion.** - **Dispatcher** — a long-lived loop that, every N seconds (default 60): reclaims stale claims, reclaims crashed workers (PID gone but TTL not yet expired), promotes ready tasks, atomically claims, spawns assigned profiles. Runs **inside the gateway** by default (`kanban.dispatch_in_gateway: true`). One dispatcher sweeps all boards per tick; workers are spawned with `HERMES_KANBAN_BOARD` pinned so they can't see other boards. After `kanban.failure_limit` consecutive spawn failures on the same task (default: 2) the dispatcher auto-blocks it with the last error as the reason — prevents thrashing on tasks whose profile doesn't exist, workspace can't mount, etc. diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md index 075296d687b3..ee96d46f0280 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md @@ -59,7 +59,7 @@ Hermes Kanban 是一个持久化任务看板，在所有 Hermes 配置文件之 - **Link（链接）** —— `task_links` 行，记录父 → 子依赖关系。当所有父任务变为 `done` 时，调度器将 `todo → ready`。 - **Comment（评论）** —— agent 间协议。Agent 和人类追加评论；当 worker 被（重新）启动时，它将完整的评论线程作为上下文的一部分读取。 - **Workspace（工作区）** —— worker 操作的目录。三种类型： - - `scratch`（默认）—— 在 `~/.hermes/kanban/workspaces//` 下（非默认看板为 `~/.hermes/kanban/boards//workspaces//`）创建的临时目录。**任务完成时删除** —— scratch 是临时性的，worker（或 `hermes kanban complete `）将任务标记为完成的那一刻，目录即被清除。如果想保留 worker 的输出，请使用 `worktree:` 或 `dir:`。在某次安装中首次创建 scratch 工作区时，调度器会记录警告并在任务上发出 `tip_scratch_workspace` 事件（可通过 `hermes kanban show ` 查看）。 + - `scratch`（默认）—— 在 `~/.hermes/kanban/workspaces//` 下（非默认看板为 `~/.hermes/kanban/boards//workspaces//`）创建的临时目录。**任务完成时删除** —— scratch 按设计是临时性的。通过 `kanban_complete(artifacts=[...])` 明确声明的文件会在清理前复制到持久的任务附件存储；旧版完成摘要中已存在的交付文件路径也会得到同样处理。其他 scratch 文件仍会被删除。如果声明的 scratch 交付文件不存在，任务会保持进行中，worker 可修正路径后重试。需要保留整个工作区时，请使用 `worktree:` 或 `dir:`。在某次安装中首次创建 scratch 工作区时，调度器会记录警告并在任务上发出 `tip_scratch_workspace` 事件（可通过 `hermes kanban show ` 查看）。 - `dir:` —— 现有的共享目录（Obsidian vault、邮件运维目录、每账号文件夹）。**必须是绝对路径。** 像 `dir:../tenants/foo/` 这样的相对路径在调度时会被拒绝，因为它们会相对于调度器碰巧所在的 CWD 解析，这是模糊的，也是混淆代理（confused-deputy）逃逸向量。路径本身是受信任的 —— 这是你的机器、你的文件系统，worker 以你的 uid 运行。这是受信任本地用户的威胁模型；kanban 设计为单主机。**完成时保留。** - `worktree` —— 用于编码任务的 git worktree，位于 `.worktrees//` 下。使用 `worktree:` 固定确切的目标路径。Worker 端的 `git worktree add` 创建它，提供 `--branch` 时使用该分支。**完成时保留。** - **Dispatcher（调度器）** —— 一个长期运行的循环，每 N 秒（默认 60 秒）执行一次：回收过期的认领、回收崩溃的 worker（PID 消失但 TTL 尚未过期）、推进就绪任务、原子性认领、启动已分配的配置文件。默认**在 gateway 内部运行**（`kanban.dispatch_in_gateway: true`）。每次 tick 一个调度器扫描所有看板；worker 启动时固定了 `HERMES_KANBAN_BOARD`，因此无法看到其他看板。在同一任务上连续启动失败 `kanban.failure_limit` 次（默认：2）后，调度器会以最后一个错误为原因自动阻塞该任务 —— 防止因配置文件不存在、工作区无法挂载等原因导致的反复抖动。