Skip to content

fix(kanban): enforce worker task ownership on kanban_link - #72309

Open
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/kanban-link-worker-ownership
Open

fix(kanban): enforce worker task ownership on kanban_link#72309
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/kanban-link-worker-ownership

Conversation

@Frowtek

@Frowtek Frowtek commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

#19534 / #19713 established that a dispatcher-spawned worker may only mutate run-lifecycle state on its own task, and wired _enforce_worker_task_ownership into kanban_complete, kanban_block, kanban_heartbeat, kanban_attach, kanban_attach_url and kanban_unblock. kanban_link was left ungated — and it mutates the child task:

# hermes_cli/kanban_db.py :: link_tasks
if parent_status != "done":
    conn.execute("UPDATE tasks SET status = 'todo' WHERE id = ? AND status = 'ready'", (child_id,))
...
_inherit_notify_subs(conn, child_id, (parent_id,))

So a task-scoped worker — or a prompt-injected one, which is the threat the guard's own docstring names — can pass any child_id and:

  1. stall another tenant's work: a ready task is demoted to todo, so the dispatcher stops promoting it, for as long as the attacker's parent stays un-done;
  2. hijack its notifications: the victim task inherits the attacker's parent's notify subscriptions, so that task's terminal notification is delivered to the attacker's chat.

Reproduced against main (759f68bc2) from a worker context (HERMES_KANBAN_TASK set to the attacker's own task):

victim BEFORE  status: ready | subs: []
kanban_link result: {"ok": true, "parent_id": "t_20172129", "child_id": "t_b41e2dbe"}
victim AFTER   status: todo  | subs: ['ATTACKER-CHAT']

After this change the same call returns the standard scope refusal and the victim is untouched (status: ready | subs: []).

The gate goes on the child end — the task whose state actually changes. That keeps every legitimate flow working:

  • orchestrator profiles (no HERMES_KANBAN_TASK) still rewire the graph freely;
  • a worker can still attach its own task under a parent;
  • spawning follow-up work keeps its documented path, kanban_create(parents=[...]), which the guard's own error message already points workers at.

Note on related work: #68029 proposes kanban.worker_graph_mutations as an opt-in policy that hides kanban_create/kanban_link from workers entirely; by its own summary "existing installs are unchanged by default". This PR is the narrower, on-by-default fix — it restores the ownership invariant the other destructive kanban tools already enforce, and remains correct with that policy either way.

Related Issue

No separate issue filed. Completes the invariant from #19534 (fixed for the sibling tools in #19713).

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/kanban_tools.py (_handle_link): call _enforce_worker_task_ownership(child_id) before connecting, mirroring the sibling handlers, with a comment recording why the child end is the gated one.
  • tests/tools/test_kanban_tools.py: three new tests — a worker cannot link a foreign ready child (asserting it stays ready and inherits no subscriptions), a worker may still link its own task under a parent, and an orchestrator context can still link two foreign tasks.
  • tests/tools/test_kanban_tools.py (test_link_happy_path): this existing test used the worker_env fixture only for its DB setup but incidentally ran with HERMES_KANBAN_TASK set while linking two unrelated tasks, which is now an orchestrator operation — it drops the worker scope the fixture also sets. Flagging the edit explicitly: unlike test_worker_can_comment_on_foreign_task, which documents its permissive policy and asks to fail CI if gated, this test states no intent about worker scope.

How to Test

  1. From a dispatcher-spawned worker (HERMES_KANBAN_TASK = its own task), call kanban_link(parent_id=<own task>, child_id=<another tenant's ready task>).
  2. Before this change the call returns ok: true, the victim task drops from ready to todo, and list_notify_subs(victim) shows the attacker's chat. After, the call is refused and the victim is unchanged.
  3. pytest tests/tools/test_kanban_tools.py -q -> 123 passed (3 new). The cross-tenant test fails on main without the code change (assert True is not True — the link succeeds).
  4. Wider run: pytest tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_core_functionality.py tests/gateway/test_kanban_notifier.py tests/hermes_cli/test_kanban_notify.py -q -> 321 passed, 8 failed; those 8 (protocol-violation streaks, stale detection) are pre-existing on main — verified by re-running them with this change stashed (same 8).
  5. Tested on Ubuntu 24.04.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate — see the feat(kanban): allow orchestrator-only graph mutation policy #68029 note above; no open PR gates kanban_link by default
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the affected suites and they pass (see step 4 for the pre-existing, unrelated failures)
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (restores the documented worker-scope rule; the refusal message already names the supported alternatives)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A (no config keys added or changed)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — N/A (no platform-specific behaviour)
  • I've updated tool descriptions/schemas if I changed tool behavior — the kanban_link schema is unchanged; the refusal surfaces at call time like the sibling tools' does

NousResearch#19534 / NousResearch#19713 established that a dispatcher-spawned worker may only
mutate run-lifecycle state on its own task, and wired
_enforce_worker_task_ownership into kanban_complete / kanban_block /
kanban_heartbeat / kanban_attach / kanban_attach_url / kanban_unblock.
kanban_link was left ungated, and it mutates the child task:

  - link_tasks() demotes a `ready` child back to `todo` when the parent
    isn't done, so the dispatcher stops promoting it;
  - the child then inherits the parent's notify subscriptions.

A task-scoped worker (or a prompt-injected one — the threat named in the
guard's own docstring) can therefore point kanban_link at a sibling or
cross-tenant task and both stall it indefinitely and redirect its terminal
notifications to its own parent's subscribers. Observed on main:

  victim BEFORE  status: ready | subs: []
  kanban_link    -> {"ok": true}
  victim AFTER   status: todo  | subs: ['ATTACKER-CHAT']

Gate the child end — the task whose state changes. Orchestrator profiles
(no HERMES_KANBAN_TASK) keep unrestricted graph edits, a worker can still
attach its own task to a parent, and follow-up work keeps its documented
path via kanban_create(parents=[...]).
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels Jul 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused reproduction and the child-state/notification assertions. The current mutation path is real: tools/kanban_tools.py:1415-1428 forwards any supplied child to link_tasks, and hermes_cli/kanban_db.py:3423-3436 can demote that child and inherit notification subscriptions.

Problems

  • This changes an explicit existing policy, not just an unguarded sibling. The original ownership fix, d3b22b76d8, says kanban_link was deliberately kept unrestricted; current regression guidance repeats that at tests/tools/test_kanban_tools.py:558-564.
  • The updated happy-path test clears HERMES_KANBAN_TASK, so it stops covering the current worker-scoped permissive contract instead of reconciling that policy change.

Suggested changes

  • Please establish the intended graph-mutation policy with maintainers before changing this handler contract. Current documentation treats worker/orchestrator separation for graph edits as a convention (website/docs/user-guide/features/kanban.md:341), and related PR #68029 proposes a configurable alternative.
  • If strict ownership is selected, update the policy comment and docs alongside the tests.

Automated hermes-sweeper review.

Comment thread tools/kanban_tools.py
ownership_err = _enforce_worker_task_ownership(str(child_id))
if ownership_err:
return ownership_err
board = args.get("board")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main deliberately keeps kanban_link unrestricted for task-scoped workers (d3b22b76d8; tests/tools/test_kanban_tools.py:558-564). This guard reverses that policy, so the worker graph-mutation direction needs maintainer agreement before changing the handler contract.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants