fix: follow up unresolved PR76 security findings

agent/kanban_stop.py

@@ -128,15 +128,33 @@ def _tool_call_name(tc: Any) -> str:
 
 
 def session_called_kanban_terminal(messages: Iterable[dict] | None) -> bool:
-    """True if this conversation already invoked a terminal kanban tool."""
+    """True if this conversation has a successful terminal Kanban result."""
+    review_call_ids: set[str] = set()
+    review_tools = {"kanban_request_review", "kanban_request_changes"}
     for msg in filter(lambda m: isinstance(m, dict), messages or ()):
         role = msg.get("role")
-        if role == "assistant" and any(
-            _tool_call_name(tc) in _TERMINAL_KANBAN_TOOLS for tc in msg.get("tool_calls") or []
-        ):
-            return True
-        if role == "tool" and str(msg.get("name") or "") in _TERMINAL_KANBAN_TOOLS:
-            return True
+        if role == "assistant":
+            for tc in msg.get("tool_calls") or []:
+                name = _tool_call_name(tc)
+                if name in {"kanban_complete", "kanban_block"}:
+                    return True
+                if name in review_tools:
+                    call_id = tc.get("id") if isinstance(tc, dict) else getattr(tc, "id", None)
+                    if call_id:
+                        review_call_ids.add(str(call_id))
+        elif role == "tool":
+            name = str(msg.get("name") or "")
+            if name in {"kanban_complete", "kanban_block"}:
+                return True
+            if name in review_tools and (
+                not review_call_ids or str(msg.get("tool_call_id") or "") in review_call_ids
+            ):
+                try:
+                    payload = json.loads(msg.get("content") or "")
+                except (TypeError, ValueError, json.JSONDecodeError):
+                    continue
+                if isinstance(payload, dict) and payload.get("ok") is True:
+                    return True
     return False
 
 

agent/shell_hooks.py

@@ -369,9 +369,17 @@ def _evaluate_result(spec: ShellHookSpec, r: Dict[str, Any]) -> Optional[Dict[st
                        r["returncode"], spec.event, spec.command, stderr[:_STDERR_MESSAGE_LIMIT])
     stdout = (r["stdout"] or "").strip()
     parsed = _parse_response(spec.event, stdout)
-    if parsed is None and fail_closed and stdout and not _is_json_object(stdout):
-        # A fail-closed gate must not silently allow on garbage stdout (e.g. a stack trace).
-        return _fail_closed_block(spec, "unparseable stdout (expected a JSON object)")
+    if parsed is None and fail_closed and (
+        spec.event == "pre_kanban_complete" or (stdout and not _is_json_object(stdout))
+    ):
+        # Completion gates must not silently allow on an empty response; other
+        # fail-closed hooks reject malformed JSON but retain a valid {} no-op.
+        reason = (
+            "unparseable stdout (expected a JSON object)"
+            if stdout
+            else "missing decision"
+        )
+        return _fail_closed_block(spec, reason)
     return parsed
 
 

agent/turn_finalizer.py

@@ -89,17 +89,26 @@ def _record_kanban_guardrail_halt(
         from hermes_cli import kanban_db_dispatch as _kbd
         _conn = _hermes_cli_kanban_db_connect.connect()
         try:
+            failure_kwargs = {
+                "outcome": "crashed",
+                "release_claim": True,
+                "end_run": True,
+                "event_payload_extra": {
+                    "guardrail": code,
+                    "tool_name": tool_name,
+                },
+            }
+            raw_run_id = os.environ.get("HERMES_KANBAN_RUN_ID", "").strip()
+            if raw_run_id.isdigit():
+                failure_kwargs["expected_run_id"] = int(raw_run_id)
+            claim_lock = os.environ.get("HERMES_KANBAN_CLAIM_LOCK", "").strip()
+            if claim_lock:
+                failure_kwargs["expected_claim_lock"] = claim_lock
             _kbd._record_task_failure(
                 _conn,
                 kanban_task,
                 error,
-                outcome="crashed",
-                release_claim=True,
-                end_run=True,
-                event_payload_extra={
-                    "guardrail": code,
-                    "tool_name": tool_name,
-                },
+                **failure_kwargs,
             )
         finally:
             try:

apps/desktop/electron/desktop-background-shutdown.test.ts

@@ -5,68 +5,27 @@
 import { stopDesktopBackgroundServices } from './desktop-background-shutdown'
 
 describe('Desktop background-service shutdown', () => {
-  it('runs the resolved Hermes gateway drain-stop command and waits for exit', async () => {
-    const kill = vi.fn<(signal?: NodeJS.Signals | number) => boolean>(() => true)
-    const child = Object.assign(new EventEmitter(), { kill })
-    const spawnFn = vi.fn(() => child)
-    const resolveBackend = vi.fn(args => ({
-      command: '/runtime/bin/hermes',
-      args,
-      root: '/runtime',
-      env: { RUNTIME_MARKER: '1' },
-      shell: false
-    }))
+  it('does not stop messaging gateways when Desktop quits', async () => {
+    const spawnFn = vi.fn()
 
-    const stopped = stopDesktopBackgroundServices({
-      resolveBackend,
+    await expect(stopDesktopBackgroundServices({
+      resolveBackend: vi.fn(),
       spawnFn,
       env: { HERMES_HOME: '/profiles' },
-      platform: 'linux',
-      timeoutMs: 1_000
-    })
-    child.emit('exit', 0, null)
+      platform: 'linux'
+    })).resolves.toBe(true)
 
-    await expect(stopped).resolves.toBe(true)
-    expect(resolveBackend).toHaveBeenCalledWith(['gateway', 'stop', '--all', '--drain'])
-    expect(spawnFn).toHaveBeenCalledWith(
-      '/runtime/bin/hermes',
-      ['gateway', 'stop', '--all', '--drain'],
-      expect.objectContaining({
-        cwd: '/runtime',
-        env: expect.objectContaining({ HERMES_HOME: '/profiles', RUNTIME_MARKER: '1' }),
-        stdio: 'ignore'
-      })
-    )
-    expect(kill).not.toHaveBeenCalled()
-  })
-
-  it('terminates a drain helper that exceeds the shutdown budget', async () => {
-    vi.useFakeTimers()
-    const kill = vi.fn<(signal?: NodeJS.Signals | number) => boolean>(() => true)
-    const child = Object.assign(new EventEmitter(), { kill })
-
-    const stopped = stopDesktopBackgroundServices({
-      resolveBackend: args => ({ command: 'hermes', args }),
-      spawnFn: () => child,
-      env: {},
-      platform: 'linux',
-      timeoutMs: 25
-    })
-    await vi.advanceTimersByTimeAsync(250)
-    expect(kill).toHaveBeenCalledWith('SIGTERM')
-    child.emit('exit', 0, null)
-    await expect(stopped).resolves.toBe(false)
-    vi.useRealTimers()
+    expect(spawnFn).not.toHaveBeenCalled()
   })
 
   it('boots out the exact Hermes companion launchd job on macOS', async () => {
     const children: EventEmitter[] = []
     const spawnFn = vi.fn(() => {
       const child = Object.assign(new EventEmitter(), {
         kill: vi.fn(() => true)
       })
       children.push(child)
       return child
     })
 
     const stopped = stopDesktopBackgroundServices({
@@ -77,14 +36,12 @@
       uid: 501,
       timeoutMs: 1_000
     })
     expect(spawnFn).toHaveBeenCalledTimes(1)
     children[0].emit('exit', 0, null)
-    await vi.waitFor(() => expect(spawnFn).toHaveBeenCalledTimes(2))
-    children[1].emit('exit', 0, null)
 
     await expect(stopped).resolves.toBe(true)
     expect(spawnFn).toHaveBeenNthCalledWith(
-      2,
+      1,
       '/bin/launchctl',
       ['bootout', 'gui/501/com.local.hermes.companion-backend'],
       expect.objectContaining({ shell: false, stdio: 'ignore' })

apps/desktop/electron/desktop-background-shutdown.ts

@@ -47,12 +47,12 @@
       if (settled) {
         return
       }
       settled = true
 
       if (timer) {
         clearTimeout(timer)
       }
       resolve(ok)
     }
 
     try {
@@ -87,10 +87,9 @@
 }
 
 /**
- * Drain and stop every local supervised gateway before Desktop exits. The
- * drain refuses new chat/cron/Kanban work, waits for in-flight gateway turns
- * and live Kanban workers, then stops supervision. The companion launchd job
- * is unloaded only after that drain helper exits.
+ * Stop Desktop-owned background supervision before Desktop exits. Messaging
+ * gateways are user-owned long-lived services and must survive a UI quit;
+ * only the companion backend launchd job is unloaded here.
  */
 export function stopDesktopBackgroundServices({
   resolveBackend,
@@ -101,28 +100,7 @@
   uid = typeof process.getuid === 'function' ? process.getuid() : -1,
   onError = () => undefined
 }: StopDesktopBackgroundServicesOptions): Promise<boolean> {
-  const backend = resolveBackend(['gateway', 'stop', '--all', '--drain'])
-
-  if (!backend?.command || backend.kind === 'bootstrap-needed') {
-    onError('No runnable local Hermes command was available for gateway stop --all --drain')
-
-    return Promise.resolve(false)
-  }
-
-  const commands: StopCommand[] = [
-    {
-      command: backend.command,
-      args: backend.args || ['gateway', 'stop', '--all', '--drain'],
-      label: 'gateway stop --all --drain',
-      options: {
-        cwd: backend.root || undefined,
-        env: { ...env, ...(backend.env || {}) },
-        shell: Boolean(backend.shell),
-        windowsHide: true,
-        stdio: 'ignore'
-      }
-    }
-  ]
+  const commands: StopCommand[] = []
 
   if (platform === 'darwin' && uid >= 0) {
     commands.push({
@@ -139,21 +117,11 @@
     })
   }
 
-  const [gateway, ...afterDrain] = commands
-
-  // Bound the drain helper too.  Gateway drain intentionally waits for
-  // in-flight turns and workers, but a wedged worker must not make Electron's
-  // before-quit promise immortal.  The timeout path sends SIGTERM and lets the
-  // caller complete teardown with an explicit failure result.
-  return runStopCommand(spawnFn, gateway, timeoutMs, onError).then(async gatewayStopped => {
-    if (!gatewayStopped) {
-      return false
-    }
-
-    const results = await Promise.all(
-      afterDrain.map(command => runStopCommand(spawnFn, command, timeoutMs, onError))
-    )
+  if (commands.length === 0) {
+    return Promise.resolve(true)
+  }
 
-    return results.every(Boolean)
-  })
+  return Promise.all(
+    commands.map(command => runStopCommand(spawnFn, command, timeoutMs, onError))
+  ).then(results => results.every(Boolean))
 }

apps/desktop/src/plugins/kanban/api.ts

@@ -11,6 +11,7 @@
 
 import {
   atom,
+  host,
   type PluginOs,
   type PluginRestOptions,
   type PluginStorage,
@@ -19,7 +20,11 @@ import {
 } from '@hermes/plugin-sdk'
 
 // Native completion notification.
-import { bindCompletionNotify, type CompletionEvent, onKanbanEventsFrame } from './completion-notify'
+import {
+  bindCompletionNotify,
+  type CompletionEvent,
+  onKanbanEventsFrame
+} from './completion-notify'
 import type {
   BoardExportResult,
   BoardImportResult,
@@ -107,6 +112,10 @@ export function applyHeartbeatEvents(board: KanbanBoard, events: CompletionEvent
 
 /** One live `task_events` frame → cache-local heartbeat updates plus one
  *  coalesced refresh for events that can actually change board state. */
+function activeSourceKey(): string {
+  return `${host.state.connectionId.get() ?? 'local'}::${host.state.profile.get() || 'default'}`
+}
+
 function onEventsFrame(slug: string, data: unknown, scheduleBoardRefresh: () => void): void {
   const events = (data as { events?: CompletionEvent[] })?.events
 
@@ -133,7 +142,7 @@ function onEventsFrame(slug: string, data: unknown, scheduleBoardRefresh: () =>
 
   // Completion notification (after invalidation so notify failure
   // never interferes with cache invalidation).
-  void onKanbanEventsFrame(slug, events).catch(() => undefined)
+  void onKanbanEventsFrame(slug, events, activeSourceKey()).catch(() => undefined)
 }
 
 // A persisted, subscribable atom (the structural slice we need — avoids
@@ -187,7 +196,14 @@ export function bindApi(
 
   const open = (slug: string) => {
     close?.()
-    close = socket(slug ? `/events?board=${encodeURIComponent(slug)}` : '/events', data =>
+
+    // Do not put a cursor in the socket URL: pluginSocket may reconnect the
+    // same URL after the active source/profile changes. The notification
+    // module keys its cursor by the source and board, then baselines through
+    // the current REST door, so a new source cannot inherit old ids.
+    const path = slug ? `/events?board=${encodeURIComponent(slug)}` : '/events'
+
+    close = socket(path, data =>
       onEventsFrame(slug, data, scheduleBoardRefresh)
     )
   }

apps/desktop/src/plugins/kanban/completion-notify.test.ts

@@ -21,7 +21,8 @@ interface OsDoor {
 }
 interface Mod {
   bindCompletionNotify(r: Rest, t?: Translate, os?: OsDoor): void
-  onKanbanEventsFrame(slug: string, events?: CompletionEvent[]): Promise<boolean>
+  kanbanEventsSince(slug: string, sourceKey?: string): number | undefined
+  onKanbanEventsFrame(slug: string, events?: CompletionEvent[], sourceKey?: string): Promise<boolean>
 }
 
 const { hostMock } = vi.hoisted(() => ({
@@ -104,6 +105,10 @@ describe('authoritative baseline', () => {
     expect(fired).toBe(true)
     expect(hostMock.notify).toHaveBeenCalledTimes(1)
 
+    // Reconnecting subscribers can pass the accepted high-water mark to the
+    // server, so an event missed between socket frames is replayed safely.
+    expect(m.kanbanEventsSince('smoke')).toBe(101)
+
     // Same event delivered again (duplicate frame) must not re-notify.
     const again = await m.onKanbanEventsFrame('smoke', [ev(101, 'completed', { summary: 'Done' })])
 
@@ -180,6 +185,37 @@ describe('authoritative baseline', () => {
     expect(hostMock.notify).toHaveBeenCalledTimes(1)
   })
 
+  it('rebinds the cursor when the backend or profile changes', async () => {
+    const m = await loadModule()
+    m.bindCompletionNotify(makeRest(() => 100) as never)
+    await m.onKanbanEventsFrame('smoke', [ev(101, 'completed')])
+    expect(hostMock.notify).toHaveBeenCalledTimes(1)
+
+    // The same renderer instance can reconnect to another source whose event
+    // ids overlap. Rebinding must discard the old source's high-water mark.
+    const nextRest = makeRest(() => 200)
+    m.bindCompletionNotify(nextRest as never)
+    const fired = await m.onKanbanEventsFrame('smoke', [ev(150, 'completed')])
+
+    expect(fired).toBe(false)
+    expect(hostMock.notify).toHaveBeenCalledTimes(1)
+    expect(nextRest).toHaveBeenCalledWith('/board?board=smoke')
+  })
+
+  it('keeps cursors independent for the same board across active sources', async () => {
+    let baseline = 100
+    const m = await loadModule()
+    m.bindCompletionNotify(makeRest(() => baseline) as never)
+
+    await m.onKanbanEventsFrame('smoke', [ev(101, 'completed')], 'source-a')
+    baseline = 0
+    await m.onKanbanEventsFrame('smoke', [ev(50, 'completed')], 'source-b')
+
+    expect(hostMock.notify).toHaveBeenCalledTimes(2)
+    expect(m.kanbanEventsSince('smoke', 'source-a')).toBe(101)
+    expect(m.kanbanEventsSince('smoke', 'source-b')).toBe(50)
+  })
+
   it('baseline failure is fail-closed: unknown baseline suppresses, later success binds', async () => {
     let failBoard = true