Skip to content

fix(kanban): confine completion artifacts to task workspace - #86610

Open
enzo-adami wants to merge 3 commits into
NousResearch:mainfrom
enzo-adami:agent/confine-kanban-completion-artifacts
Open

fix(kanban): confine completion artifacts to task workspace#86610
enzo-adami wants to merge 3 commits into
NousResearch:mainfrom
enzo-adami:agent/confine-kanban-completion-artifacts

Conversation

@enzo-adami

@enzo-adami enzo-adami commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Require every declared completion artifact to exist and resolve inside its task workspace.
  • Stage accepted artifacts from scratch, directory, and worktree workspaces into the task's durable attachment directory before emitting the completion event.
  • Open the staging source with O_NOFOLLOW where the platform provides it, then compare the opened descriptor's device/inode and validate its type and size before copying.
  • Pin every durable destination component with descriptor-relative O_DIRECTORY | O_NOFOLLOW traversal, create files with O_EXCL, and perform rollback cleanup relative to the pinned task-directory descriptor.
  • Fail closed on pre-planted symlinks at either the attachments root or per-task directory, for the default board and named boards. The non-dir_fd fallback also rejects symlinked components with lstat.
  • Restrict notifier adapters to staged task-attachment paths; worker-workspace paths and symlinked attachment roots are never passed to an upload adapter.
  • Derive attachment ownership from the open board database rather than the ambient/current board.
  • Preserve legacy prose-discovered deliverables by staging them through the same path, and keep post-staging missing-file handling fail-open in the notifier.
  • Update the kanban_complete schema text to state the workspace and staging contract.

Why

The completion path previously accepted host paths outside the task workspace. A first confinement pass still left two check/use boundaries:

  1. after resolving a workspace source, the notifier adapter could reopen a replaced pathname;
  2. attachment_dir.mkdir(parents=True, exist_ok=True) accepted a pre-planted directory symlink and redirected the durable copy outside Hermes storage.

The second issue was reproduced deterministically on both the default and a named board: completion succeeded and wrote report.pdf through a symlink into an external directory. Destination traversal and file creation are now relative to pinned directory descriptors, so replacing the visible pathname cannot redirect the open. The notifier independently refuses a symlinked durable root.

Rejected declarations or unsafe destinations roll back task completion so the worker can correct the path and retry. If a staged attachment disappears after completion, the notifier skips it without stopping delivery of the remaining artifacts.

Validation

Reproduced before the fixes:

  • absolute-path escape, in-workspace symlink escape, and source substitution during staging;
  • notifier check/use race that uploaded b"host secret";
  • explicit named-board completion staging into the ambient default board;
  • pre-planted attachments-root and task-directory symlink redirection;
  • legacy missing-file notifier test failing after the stricter completion contract.

After the fixes:

145 passed, 0 failed, 1 skipped (unrelated Windows-only test on macOS)
destination symlink matrix: 8/8 pass
ruff: All checks passed
git diff --check: clean

The test set covers Kanban completion and boards, gateway watcher delivery, tool handling, notifier behavior, attachment persistence, worker image extraction, delegated-child isolation, collision-safe durable names, and native/fallback destination traversal.

Follow-up

#82908 returns the newly durable artifact paths directly from kanban_complete, which is useful for Desktop artifact indexing but is not required for confinement correctness. It is intentionally not absorbed here so this security fix stays focused; it should be rebased on this PR (or implemented separately after merge).

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 15, 2026
@enzo-adami
enzo-adami marked this pull request as ready for review August 15, 2026 03:35
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(kanban): confine completion artifacts to task workspace

  1. gateway/kanban_watchers.py _deliver_kanban_artifacts — when task_id is falsy/absent, allowed_roots stays empty and every candidate is silently dropped (only a per-candidate warning log). Callers that don't pass a task object now have all artifacts suppressed with no error surfaced, a behavior change from before. Consider failing loudly (or explicitly opting in) so a future caller doesn't regress delivery without noticing.
  2. The same function wraps the task_attachments_dir resolution in except (OSError, RuntimeError, ValueError): pass — if the durable root can't be resolved (permissions, missing home), artifacts are dropped again. An aggregated warning naming the count of skipped artifacts would make confinement failures observable.
  3. hermes_cli/kanban_db.py _persist_completion_artifacts_connection_board() derives the board from PRAGMA database_list, an indirect ownership signal. If kanban_home is a symlink or the DB path layout differs, the fallback get_current_board() can return a different board than the task's actual one, staging attachments under the wrong board. Passing the board explicitly through the call chain (the connection already knows it) would be more robust than re-deriving it.
  4. _open_pinned_attachment_directory fallback path — when dir_fd is unsupported, os.mkdir(current, mode=0o700) leaves a pre-existing directory's permissions untouched (mode is only applied at creation). Consider os.chmod after mkdir (or at least an lstat + chmod when the component pre-exists) to uphold the 0o700 guarantee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants