fix(kanban): route notifications via owning profile + wake creator agent (salvage #54872) - #447
Conversation
|
Review Complete Files Reviewed: 20 By Severity:
This PR adds cross-profile kanban wake notifications and i18n message templates, but contains a profile isolation leak, silent event-consumption bugs, and a cursor advance race condition that permanently loses wake events on delivery failure. Files Reviewed (20 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (78/100) — 1 high finding, 2 medium, 1 low · 248 LOC across 20 files
Summary
PR #447 introduces kanban wake notification support with cross-profile routing and i18n message templates across 17 locales. The diff spans the kanban watcher, session context, gateway run loop, kanban tools, and locale YAML files.
High-Severity Issues
Profile Isolation Leak (gateway/kanban_watchers.py:256)
The cross-profile delivery guard only checks that _owner_adapters exists but not that the specific platform adapter is present. When absent, the fallback at line 320 silently uses the default profile's adapter, leaking task notifications across isolated profiles.
Cursor Advance Race (gateway/kanban_watchers.py:471)
The cursor is advanced at line 471 before wake injection at line 521. If adapter.handle_message() raises (adapter became unavailable), the exception is caught and logged, but the cursor is already advanced — events are permanently consumed without waking the agent.
Medium-Severity Issues
Missing Event Handlers (gateway/kanban_watchers.py:167)
archived and unblocked were added to TERMINAL_KINDS but no message formatting branches exist; events are silently consumed.
Cross-Profile Platform Exclusion (gateway/kanban_watchers.py:201)
active_platforms is built only from the default profile's adapters, silently dropping cross-profile subscriptions on platforms the default profile doesn't have.
Markdown Injection (gateway/kanban_watchers.py:361)
User-controlled fields (task.title, reason, error, new_status) are interpolated unescaped into f-strings sent to Markdown-rendering platforms.
Low-Severity Issues
Locale Punctuation (locales/ja.yaml:159, locales/zh-hant.yaml:159)
Japanese and Traditional Chinese status_joiner uses ASCII comma-space instead of native enumeration punctuation.
| @@ -250,11 +254,13 @@ def _collect(): | |||
| for sub in subs: | |||
| owner_profile = sub.get("notifier_profile") or None | |||
| if owner_profile and owner_profile != notifier_profile: | |||
| logger.debug( | |||
| "kanban notifier: subscription for %s owned by profile %s; current profile %s skipping", | |||
| sub.get("task_id"), owner_profile, notifier_profile, | |||
| ) | |||
| continue | |||
| _owner_adapters = getattr(self, "_profile_adapters", {}).get(owner_profile) | |||
| if not _owner_adapters: | |||
| logger.debug( | |||
| "kanban notifier: subscription for %s owned by profile %s; current profile %s has no adapter for it, skipping", | |||
| sub.get("task_id"), owner_profile, notifier_profile, | |||
| ) | |||
| continue | |||
| platform = (sub.get("platform") or "").lower() | |||
| if platform not in active_platforms: | |||
| logger.debug( | |||
| @@ -304,7 +310,14 @@ def _collect(): | |||
| self._kanban_advance, sub, d["cursor"], board_slug, | |||
| ) | |||
| continue | |||
| adapter = self.adapters.get(plat) | |||
| sub_profile = sub.get("notifier_profile") or "" | |||
| adapter = None | |||
| if sub_profile: | |||
| _profile_map = getattr(self, "_profile_adapters", {}).get(sub_profile) | |||
| if _profile_map: | |||
| adapter = _profile_map.get(plat) | |||
| if adapter is None: | |||
| adapter = self.adapters.get(plat) | |||
| if adapter is None: | |||
| logger.debug( | |||
| "kanban notifier: adapter %s disconnected before delivery for %s; rewinding claim", | |||
| @@ -319,6 +332,7 @@ def _collect(): | |||
| ) | |||
| continue | |||
| title = (task.title if task else sub["task_id"])[:120] | |||
| board_tag = f"[{board_slug}] " if board_slug else "" | |||
| for ev in d["events"]: | |||
| kind = ev.kind | |||
| # Identity prefix: attribute terminal pings to the | |||
| @@ -345,35 +359,40 @@ def _collect(): | |||
| r = lines[0][:160] if lines else task.result[:160] | |||
| handoff = f"\n{r}" | |||
| msg = ( | |||
| f"✔ {tag}Kanban {sub['task_id']} done" | |||
| f"✔ {board_tag}{tag}Kanban {sub['task_id']} done" | |||
| f" — {title}{handoff}" | |||
| ) | |||
| elif kind == "blocked": | |||
| reason = "" | |||
| if ev.payload and ev.payload.get("reason"): | |||
| reason = f": {str(ev.payload['reason'])[:160]}" | |||
| msg = f"⏸ {tag}Kanban {sub['task_id']} blocked{reason}" | |||
| msg = f"⏸ {board_tag}{tag}Kanban {sub['task_id']} blocked{reason}" | |||
| elif kind == "gave_up": | |||
| err = "" | |||
| if ev.payload and ev.payload.get("error"): | |||
| err = f"\n{str(ev.payload['error'])[:200]}" | |||
| msg = ( | |||
| f"✖ {tag}Kanban {sub['task_id']} gave up " | |||
| f"✖ {board_tag}{tag}Kanban {sub['task_id']} gave up " | |||
| f"after repeated spawn failures{err}" | |||
| ) | |||
| elif kind == "crashed": | |||
| msg = ( | |||
| f"✖ {tag}Kanban {sub['task_id']} worker crashed " | |||
| f"✖ {board_tag}{tag}Kanban {sub['task_id']} worker crashed " | |||
| f"(pid gone); dispatcher will retry" | |||
| ) | |||
| elif kind == "timed_out": | |||
| limit = 0 | |||
| if ev.payload and ev.payload.get("limit_seconds"): | |||
| limit = int(ev.payload["limit_seconds"]) | |||
| msg = ( | |||
| f"⏱ {tag}Kanban {sub['task_id']} timed out " | |||
| f"⏱ {board_tag}{tag}Kanban {sub['task_id']} timed out " | |||
| f"(max_runtime={limit}s); will retry" | |||
| ) | |||
| elif kind == "status": | |||
| new_status = "" | |||
| if ev.payload and ev.payload.get("status"): | |||
| new_status = str(ev.payload["status"]) | |||
| msg = f"🔄 {board_tag}{tag}Kanban {sub['task_id']} → {new_status}" | |||
| else: | |||
| continue | |||
There was a problem hiding this comment.
🟡 Missing message handlers for 'archived' and 'unblocked' event kinds in kanban notification dispatch (bug)
The TERMINAL_KINDS tuple at line 167 was expanded to include 'archived' and 'unblocked' event kinds, causing claim_unseen_events_for_sub (line 271) to fetch these events from the kanban event queue and atomically advance the cursor past them. However, the per-event message formatting block (lines 343-397) handles only six kinds: 'completed', 'blocked', 'gave_up', 'crashed', 'timed_out', and 'status'. Both 'archived' and 'unblocked' fall through to the else: continue at lines 396-397, producing no notification message and making no adapter.send() call.
After the loop completes (via the for...else at line 467, which triggers since no break occurred), the cursor is advanced at lines 471-472, permanently marking these events as delivered. The subscriber never learns about the task archive or unblock. This is a mismatch between the event kinds the notifier claims and the kinds it knows how to format.
Evidence from independent scanners: both the correctness scanner and the domain-kanban-workflow scanner independently identified this gap.
💡 Suggestion: Add message formatting handlers for 'archived' and 'unblocked' event kinds before the else: continue at line 396. For example: elif kind == 'archived': msg = f'📦 {board_tag}{tag}Kanban {sub["task_id"]} archived' and elif kind == 'unblocked': msg = f'▶ {board_tag}{tag}Kanban {sub["task_id"]} unblocked'. Alternatively, if notification for these kinds is not intended, remove them from TERMINAL_KINDS at line 167 to avoid silently consuming events.
📋 Prompt for AI Agents
In gateway/kanban_watchers.py, in the message dispatch loop around lines 391-396, add two new elif branches before the else: continue:
elif kind == "archived":
msg = f"📦 {board_tag}{tag}Kanban {sub['task_id']} archived"
elif kind == "unblocked":
msg = f"▶ {board_tag}{tag}Kanban {sub['task_id']} unblocked"If these event kinds should NOT produce user-visible notifications, instead remove 'archived' and 'unblocked' from the TERMINAL_KINDS tuple at line 167. The current state of claiming the events without delivering any notification is the worst of both options.
| if owner_profile and owner_profile != notifier_profile: | ||
| logger.debug( | ||
| "kanban notifier: subscription for %s owned by profile %s; current profile %s skipping", | ||
| sub.get("task_id"), owner_profile, notifier_profile, | ||
| ) | ||
| continue | ||
| _owner_adapters = getattr(self, "_profile_adapters", {}).get(owner_profile) | ||
| if not _owner_adapters: | ||
| logger.debug( | ||
| "kanban notifier: subscription for %s owned by profile %s; current profile %s has no adapter for it, skipping", | ||
| sub.get("task_id"), owner_profile, notifier_profile, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
🟠 Kanban notification adapter fallback leaks profile isolation in multiplexed gateway (security)
In gateway/kanban_watchers.py, the _kanban_notifier_watcher method's _collect closure resolves which adapter delivers kanban notifications. For cross-profile subscriptions, the code at lines 255-263 guards against attempting delivery when the notifier profile has no adapters for the owner profile:
owner_profile = sub.get("notifier_profile") or None
if owner_profile and owner_profile != notifier_profile:
_owner_adapters = getattr(self, "_profile_adapters", {}).get(owner_profile)
if not _owner_adapters:
continue # skip
platform = (sub.get("platform") or "").lower() # line 264, AFTER guardThe bug: The guard only checks that _owner_adapters is truthy (has ANY adapters), not that the specific platform's adapter exists within it. If profile B has a Discord adapter connected but NOT Telegram, and a subscription wants Telegram delivery, the guard passes (profile B's adapter map is truthy), and the delivery proceeds.
Later, in the delivery loop (lines 313-320), the adapter is resolved:
adapter = None
if sub_profile:
_profile_map = getattr(self, "_profile_adapters", {}).get(sub_profile)
if _profile_map:
adapter = _profile_map.get(plat)
if adapter is None:
adapter = self.adapters.get(plat) # FALLBACK: default profile's adapterSince _profile_map.get(plat) returns None (profile B has no Telegram adapter), the fallback at line 320 uses the default profile's Telegram adapter. The notification — including task title, status, assignee, result summary, and any artifacts — is delivered through the wrong profile's adapter, leaking task information across profile boundaries. The subsequent wakeup injection (lines 505-521) further creates a SessionSource with the subscription's profile and calls adapter.handle_message() on the wrong adapter.
Impact: In a multiplexed gateway serving multiple profiles, kanban task notifications and results leak from one profile into another profile's messaging channels.
💡 Suggestion: Tighten the early-skip guard in _collect() to also verify the specific platform adapter exists in the owner's profile map. Move the platform string extraction above the guard, parse it into a Platform enum, and check plat in _owner_adapters. This prevents cross-profile subscriptions from proceeding to delivery when the required platform adapter isn't available in the owner profile's adapter set.
📋 Prompt for AI Agents
In gateway/kanban_watchers.py, in the _kanban_notifier_watcher method, the _collect closure around lines 255-264, restructure the cross-profile guard to also check for the specific platform adapter:
-
Move
platform = (sub.get("platform") or "").lower()(currently line 264) ABOVE the profile guard block (before line 255). -
In the profile guard block (lines 256-263), after verifying
_owner_adaptersexists, parse the platform string into a Platform enum and check it's present:
owner_profile = sub.get("notifier_profile") or None
if owner_profile and owner_profile != notifier_profile:
_owner_adapters = getattr(self, "_profile_adapters", {}).get(owner_profile)
if not _owner_adapters:
logger.debug(...)
continue
try:
_owner_plat = _Platform(platform)
except ValueError:
continue
if _owner_plat not in _owner_adapters:
logger.debug(
"kanban notifier: subscription for %s owned by profile %s on %s; no adapter for that platform, skipping",
sub.get("task_id"), owner_profile, platform,
)
continueThis ensures cross-profile subscriptions are only processed when the notifier profile has the SPECIFIC platform adapter for the subscription owner.
| timed_out: "timed out; dispatcher will retry" | ||
| blocked: "blocked; needs attention" | ||
| status_default: "status changed" | ||
| status_joiner: ", " |
There was a problem hiding this comment.
🟢 Japanese and Traditional Chinese locales use ASCII comma-space as status_joiner instead of correct native punctuation (bug)
In the kanban wake i18n section, the status_joiner concatenates multiple wake status strings (e.g., 'timed out' + 'gave up') when more than one terminal event fires in a single notifier tick. The locale files for Japanese (ja.yaml line 159) and Traditional Chinese (zh-hant.yaml line 159) both define status_joiner: ", " — an ASCII comma followed by a space.
- Japanese enumeration uses the ideographic comma
、(U+3001), not ASCII,with a space. - Traditional Chinese enumeration uses the fullwidth comma
，(U+FF0C), matching Simplified Chinese. - Simplified Chinese (
zh.yamlline 159) already correctly uses"，"(U+FF0C).
The result is typographically incorrect output for Japanese and Traditional Chinese users whenever multiple wake events consolidate into the {status} placeholder in the wake message template.
💡 Suggestion: Change ja.yaml's status_joiner from ", " to "、" (U+3001). Change zh-hant.yaml's status_joiner from ", " to "，" (U+FF0C, matching zh.yaml).
📋 Prompt for AI Agents
In locales/ja.yaml line 159, change status_joiner: ", " to status_joiner: "、" (ideographic comma, U+3001). In locales/zh-hant.yaml line 159, change status_joiner: ", " to status_joiner: "，" (fullwidth comma, U+FF0C). This aligns the joiner punctuation with native Japanese and Traditional Chinese enumeration conventions.
| msg = ( | ||
| f"✔ {tag}Kanban {sub['task_id']} done" | ||
| f"✔ {board_tag}{tag}Kanban {sub['task_id']} done" | ||
| f" — {title}{handoff}" | ||
| ) | ||
| elif kind == "blocked": | ||
| reason = "" | ||
| if ev.payload and ev.payload.get("reason"): | ||
| reason = f": {str(ev.payload['reason'])[:160]}" | ||
| msg = f"⏸ {tag}Kanban {sub['task_id']} blocked{reason}" | ||
| msg = f"⏸ {board_tag}{tag}Kanban {sub['task_id']} blocked{reason}" | ||
| elif kind == "gave_up": | ||
| err = "" | ||
| if ev.payload and ev.payload.get("error"): | ||
| err = f"\n{str(ev.payload['error'])[:200]}" | ||
| msg = ( | ||
| f"✖ {tag}Kanban {sub['task_id']} gave up " | ||
| f"✖ {board_tag}{tag}Kanban {sub['task_id']} gave up " | ||
| f"after repeated spawn failures{err}" | ||
| ) | ||
| elif kind == "crashed": | ||
| msg = ( | ||
| f"✖ {tag}Kanban {sub['task_id']} worker crashed " | ||
| f"✖ {board_tag}{tag}Kanban {sub['task_id']} worker crashed " | ||
| f"(pid gone); dispatcher will retry" | ||
| ) | ||
| elif kind == "timed_out": | ||
| limit = 0 | ||
| if ev.payload and ev.payload.get("limit_seconds"): | ||
| limit = int(ev.payload["limit_seconds"]) | ||
| msg = ( | ||
| f"⏱ {tag}Kanban {sub['task_id']} timed out " | ||
| f"⏱ {board_tag}{tag}Kanban {sub['task_id']} timed out " | ||
| f"(max_runtime={limit}s); will retry" | ||
| ) | ||
| elif kind == "status": | ||
| new_status = "" | ||
| if ev.payload and ev.payload.get("status"): | ||
| new_status = str(ev.payload["status"]) | ||
| msg = f"🔄 {board_tag}{tag}Kanban {sub['task_id']} → {new_status}" |
There was a problem hiding this comment.
🟡 Markdown injection in kanban notification messages via unescaped user-controlled fields (security)
The _kanban_notifier_watcher method builds notification messages using f-strings that embed user-controlled values without Markdown escaping:
title(line 334): extracted fromtask.title— a user-controlled field set via the kanban dashboard PATCH API.reason(line 368): fromev.payload['reason']— the block reason set by a kanban worker or user.error(line 373): fromev.payload['error']— error text from task execution.new_status(line 394): fromev.payload['status']— status changes from dashboard drag-drop.
These values are interpolated into f-strings at lines 361-395 and sent to messaging platforms via adapter.send() (line 406). Platforms like Telegram, Discord, and Slack render Markdown in messages. An attacker who can influence the task title (e.g., by PATCHing a task through the kanban dashboard API) could inject Markdown formatting — bold, italic, strikethrough, or clickable links — into notification messages delivered to other users subscribed to the task.
Example attack: Setting a task title to *URGENT* click [here](https://evil.com) to approve renders as formatted bold text with a clickable phishing link in the notification delivered to all subscribers.
Data flow: plugin_api.py PATCH /tasks/:id → kanban DB → kanban_watchers.py:281 get_task() → line 334 title extraction → lines 361-395 f-string interpolation → line 406 adapter.send().
💡 Suggestion: Escape Markdown-special characters in user-controlled values (title, reason, error, new_status) before interpolating them into notification messages. Apply Markdown escaping appropriate for each target platform via the adapter's message formatting utilities, or use a shared Markdown-escaping helper. At minimum, escape the characters *_[]()~>#+-=|{}.!` to prevent formatting injection.
📋 Prompt for AI Agents
In gateway/kanban_watchers.py, around lines 361-395, before interpolating user-controlled values into notification f-strings, apply Markdown escaping. For each value that originates from user input (title, reason, error, new_status), pass it through a Markdown escaping function. For Telegram (MarkdownV2), escape _*[]()~>#+-=|{}.!with a preceding backslash. For other platforms, apply their appropriate escaping. A simple approach: define a helper_escape_md(s: str) -> strthat escapes the common Markdown special characters, and wrap each user-value before interpolation:title=_escape_md(title), reason=_escape_md(reason)`, etc.
1 participant
Summary
In a
multiplex_profilesgateway, kanban task notifications now deliver via the owning profile's bot adapter (not always the default profile's), and terminal task events wake the creating agent with a synthetic internal message so it can act on the result.Salvage of NousResearch#54872 by @zmlgit onto current
main(the original branch was ~600 commits behind). Contributor authorship preserved via rebase.Root cause
_maybe_auto_subscribestampednotifier_profilefromos.environ["HERMES_PROFILE"], which is unset in the gateway main process — multiplex profiles use ContextVars, not env vars. The notifier then filtered out cross-profile subscriptions and always delivered via the default adapter, which may not be in the task's chat ([230002] Bot can NOT be out of the chat).Changes
session_context.py: addHERMES_SESSION_PROFILEContextVar (profile=kwarg onset_session_vars, backward-compatible default"").run.py: stampsource.profileat dispatch time.kanban_tools.py:_maybe_auto_subscribereads profile from the ContextVar first, falls back toos.environ(kanban workers).kanban_watchers.py: prefer_profile_adapters[notifier_profile]for delivery; relax the profile skip-filter to only skip when no adapter exists for the owning profile; extendTERMINAL_KINDS; wake the creator agent oncompleted/gave_up/crashed/timed_out/blockedvia aMessageEvent(internal=True)dispatched through the normalhandle_message()pipeline.locales/*.yaml: i18n the wake messages (addresses reviewer feedback on the original PR).Validation
maintest_session_context_inheritance.py+test_local_env_session_leak.py+test_kanban_tools.pyset_session_varscallers"")handle_message, matches existing precedent)Credit: @zmlgit (张满良). Reviewer feedback on hardcoded Chinese text was addressed by the contributor's own follow-up commit (i18n).
Closes NousResearch#54872
Mirror-of: NousResearch#56531
NousResearch#56531