fix: audit-found bugs across guard, bridge, and memory system

bridge/core/sdk_text.py

@@ -122,12 +122,14 @@ def _format_ask_user_question(tool_input: dict):
     """
     lines: list = []
 
-    for q in tool_input.get("questions", []):
+    for q in tool_input.get("questions") or []:
+        if not isinstance(q, dict):
+            continue
         question = q.get("question", "")
         if question:
             lines.append(question)
 
-        options = q.get("options", [])
+        options = q.get("options") or []
 
         if options:
             lines.append("")

bridge/core/streaming.py

@@ -202,6 +202,13 @@ async def update_draft(self, draft: DraftState, new_text: str) -> bool:
                 return True
             logger.error(f"Failed to update draft {draft.message_id}: {e}")
             return False
+        except Exception as e:
+            # Match create_draft's resilience: a non-Telegram transport error
+            # (OSError, etc.) during an edit must not propagate out through the
+            # reader loop and abort message processing mid-stream. Treat it as a
+            # failed update and carry on.
+            logger.error(f"Failed to update draft {draft.message_id}: {e}")
+            return False
 
     def should_update(self, draft: DraftState, new_char_count: int) -> bool:
         """Check if draft should be updated based on thresholds"""
@@ -538,6 +545,13 @@ async def update_if_needed(self, new_text_chunk: str) -> bool:
             boundary = self._find_split_boundary(self.accumulated_text)
             seed = self._first_draft_prefix() + self.accumulated_text[:boundary]
             await self.create_draft(seed)
+            # create_draft returns None and leaves drafts empty on a failed send.
+            # Without a first draft, handle_overflow() below returns immediately
+            # without consuming accumulated_text, so the overflow `while` would
+            # spin forever with no await point and hang the whole event loop.
+            # Bail this round instead; the next chunk retries the first send.
+            if not self.drafts:
+                return False
 
         # Split off complete drafts while the buffer exceeds the per-bubble size.
         while len(self.accumulated_text) >= self.max_bubble_chars:

bridge/core/task_queue.py

@@ -109,7 +109,15 @@ async def wrapped_task():
                         # Re-raise to ensure cancellation propagates.
                         raise
                     finally:
-                        self._active.pop(key, None)
+                        # Identity-guarded clear: with up to max_inflight tasks
+                        # per key running concurrently (the lock is released
+                        # before the task body runs), a bare pop would let an
+                        # earlier task delete a *later* task's active slot —
+                        # leaving a still-running task invisible to the priority
+                        # /stop and /revert paths. Only clear the slot when it
+                        # still points at THIS task.
+                        if self._active.get(key) is current_task:
+                            self._active.pop(key, None)
 
                 accepted_task = asyncio.create_task(wrapped_task())
                 self._track(key, accepted_task)

bridge/core/ui.py

@@ -60,6 +60,12 @@ def format_relative_time(timestamp: str) -> str:
 
     try:
         dt = datetime.fromisoformat(timestamp.replace("Z", "+00:00"))
+        # A timestamp with no "Z" and no explicit offset parses as tz-naive;
+        # subtracting it from a tz-aware `now` raises TypeError, so every such
+        # button would silently fall back to a raw date. Assume UTC for naive
+        # inputs so relative formatting keeps working.
+        if dt.tzinfo is None:
+            dt = dt.replace(tzinfo=timezone.utc)
         now = datetime.now(timezone.utc)
         diff = now - dt
 
@@ -142,7 +148,7 @@ def build_history_keyboard(
     page_messages = messages[start_idx:end_idx]
 
     buttons = []
-    for msg in page_messages:
+    for offset, msg in enumerate(page_messages):
         # Format relative time
         timestamp = msg.get("timestamp", "")
         time_str = format_relative_time(timestamp)
@@ -155,8 +161,11 @@ def build_history_keyboard(
         # Format button label with relative time
         label = f"💬 {time_str} {preview}"
 
-        # Callback data: revert:select:{index}
-        cb_data = f"revert:select:{msg['index']}"
+        # Callback data: revert:select:{index}. Fall back to the computed
+        # absolute position when a record lacks an explicit "index" so a single
+        # malformed entry can't crash the whole keyboard (leaving /revert blank).
+        index = msg.get("index", start_idx + offset)
+        cb_data = f"revert:select:{index}"
         buttons.append([InlineKeyboardButton(label, callback_data=cb_data)])
 
     # Add pagination buttons if needed

bridge/tests/test_orphan_reaper.py

@@ -113,6 +113,16 @@ def test_rejects_one_part(self):
     def test_case_insensitive(self):
         self.assertTrue(self._check(["Node", "Claude"]))
 
+    def test_argv_list_with_spaced_node_path(self):
+        # Regression: a node binary under a path with a space must still be
+        # detected. The old code joined argv with spaces then re-split, turning
+        # "/opt/my node/bin/node" into two tokens and missing the orphan. The
+        # argv-list form preserves the boundary.
+        argv = ["/opt/my node/bin/node", "/home/x/.npm/claude", "--print"]
+        self.assertTrue(self.mod._is_node_claude(argv))
+        # The lossy space-joined form is exactly what used to fail.
+        self.assertFalse(self.mod._is_node_claude(" ".join(argv)))
+
 
 # ---------------------------------------------------------------------------
 # Unit tests for _is_orphaned_claude_process

bridge/tests/test_task_queue.py

@@ -127,6 +127,49 @@ async def test_clear_unknown_key_is_zero(self):
         q = UserTaskQueue()
         self.assertEqual(q.clear("nobody"), 0)
 
+    async def test_earlier_task_finishing_keeps_later_active_slot(self):
+        # Regression: with two concurrent tasks under the same key, the first to
+        # finish used to pop the shared _active slot unconditionally, erasing the
+        # still-running later task so /stop could no longer cancel it.
+        q = UserTaskQueue(max_inflight=3)
+        a_started = asyncio.Event()
+        a_release = asyncio.Event()
+        b_started = asyncio.Event()
+        b_release = asyncio.Event()
+
+        async def run_a():
+            a_started.set()
+            await a_release.wait()
+
+        async def run_b():
+            b_started.set()
+            await b_release.wait()
+
+        async def overflow():
+            raise AssertionError("unexpected overflow")
+
+        await q.enqueue("u", run_a, overflow)
+        await asyncio.wait_for(a_started.wait(), 1)
+        await q.enqueue("u", run_b, overflow)
+        await asyncio.wait_for(b_started.wait(), 1)
+
+        # B started last, so it owns the active slot.
+        b_task = q.active("u")
+        self.assertIsNotNone(b_task)
+
+        # A finishes first. Its finally must NOT clear B's slot.
+        a_release.set()
+        await asyncio.sleep(0)
+        await asyncio.sleep(0)
+        self.assertIs(q.active("u"), b_task)
+
+        # B is still cancellable via the active slot.
+        q.active("u").cancel()
+        b_release.set()
+        await asyncio.sleep(0)
+        await asyncio.sleep(0)
+        self.assertIsNone(q.active("u"))
+
 
 if __name__ == "__main__":
     unittest.main()

bridge/utils/orphan_reaper.py

@@ -106,18 +106,37 @@ def _cmdline_of(pid: int) -> str:
     return raw.replace(b"\x00", b" ").decode("utf-8", errors="replace").strip()
 
 
-def _is_node_claude(cmdline: str) -> bool:
+def _argv_of(pid: int) -> list[str]:
+    """Return the process argv as a list, preserving argument boundaries.
+
+    /proc/<pid>/cmdline is NUL-separated, so splitting on ``\\x00`` keeps each
+    argument intact even when a path contains spaces (e.g. a node binary under
+    ``/opt/my node/bin/node``). Detection must use this rather than a
+    space-joined string, which would mis-split such a path and miss the orphan.
+    """
+    raw = _read_bytes(f"/proc/{pid}/cmdline")
+    if raw is None:
+        return []
+    return [
+        a for a in raw.decode("utf-8", errors="replace").split("\x00") if a
+    ]
+
+
+def _is_node_claude(cmdline) -> bool:
     """
-    Return True when ``cmdline`` looks like a ``node claude …`` invocation.
+    Return True when the invocation looks like a ``node claude …`` process.
 
     The bridge spawns claude via the Agent SDK as:
         /path/to/node /path/to/claude --output-format stream-json …
 
     On JS-pinned nodes (e.g. daegyo) ``claude`` is the JS entry-point path,
     so we check that the first argument is ``node`` (basename) and that at
     least one subsequent argument contains the string ``claude``.
+
+    Accepts either an argv list (preferred — preserves boundaries for paths
+    with spaces) or a whitespace-joined string (best-effort, legacy).
     """
-    parts = cmdline.split()
+    parts = cmdline if isinstance(cmdline, list) else cmdline.split()
     if len(parts) < 2:
         return False
     first = parts[0].split("/")[-1].lower()
@@ -178,7 +197,7 @@ def _is_orphaned_claude_process(
     if age < min_age_seconds:
         return False
 
-    return _is_node_claude(_cmdline_of(pid))
+    return _is_node_claude(_argv_of(pid))
 
 
 def find_orphaned_claude_pids(

claude/hooks/guard.sh

@@ -101,7 +101,8 @@ gnp() { grep -Eq "$1" <<<"$cnp"; }  # like gn, but with public keys neutralized
 # The old regex required `git` and `push` to be adjacent, so `git -C x push
 # --force origin main` slipped past the review gate entirely.
 cmd_is_git_push() {
-  local toks; read -ra toks <<<"$c"
+  # Parse the quote-stripped view so a quoted subcommand/flag can't hide the push.
+  local toks; read -ra toks <<<"$cn"
   local n=${#toks[@]} i=0
   while [ "$i" -lt "$n" ] && [ "${toks[$i]}" != "git" ]; do i=$((i+1)); done
   [ "$i" -lt "$n" ] || return 1
@@ -119,22 +120,40 @@ cmd_is_git_push() {
 
 is_forcepush() {
   cmd_is_git_push || return 1
-  g '([[:space:]]-[a-zA-Z]*f\b|--force-with-lease|--force([[:space:]=]|$))' && return 0
-  g '[[:space:]]\+[A-Za-z0-9_./-]+:'                                        && return 0  # +src:dst
-  g '[[:space:]]\+[A-Za-z0-9_./-]+([[:space:]]|$)'                          && return 0  # +branch
+  # Match against the quote-stripped view (gn/cn) so `git push "--force" …` and
+  # `git push origin "+main"` can't slip the gate behind quotes. The short-flag
+  # pattern allows `f` anywhere in a bundled cluster (`-fv`, `-vf`, `-fu`), not
+  # only as the last letter.
+  gn '([[:space:]]-[a-zA-Z]*f[a-zA-Z]*\b|--force-with-lease|--force([[:space:]=]|$))' && return 0
+  gn '[[:space:]]\+[A-Za-z0-9_./-]+:'                                        && return 0  # +src:dst
+  gn '[[:space:]]\+[A-Za-z0-9_./-]+([[:space:]]|$)'                          && return 0  # +branch
   return 1
 }
 
 # 0 = safe (single explicit force-push to a clear non-protected feature branch).
 forcepush_to_feature_branch() {
   # Never safe if the command chains/embeds anything else.
   case "$c" in *';'*|*'&'*|*'|'*|*'`'*|*'$('*|*$'\n'*) return 1;; esac
-  # Exactly one `git push` invocation.
-  [ "$(grep -oE 'git[[:space:]]+push\b' <<<"$c" | wc -l)" -eq 1 ] || return 1
 
-  local toks; read -ra toks <<<"$c"
-  local n=${#toks[@]} i=0 pi=-1
-  while [ "$i" -lt "$n" ]; do [ "${toks[$i]}" = "push" ] && { pi=$i; break; }; i=$((i+1)); done
+  # Parse the quote-stripped view and locate the push subcommand, tolerating the
+  # same global options as cmd_is_git_push (`git -C <dir> push`, `git -c k=v push`).
+  # The old code counted adjacent `git push` with `wc -l`, which returned 0 for
+  # `-C`/`-c` forms and wrongly denied every legitimate feature-branch force-push
+  # made through them.
+  local toks; read -ra toks <<<"$cn"
+  local n=${#toks[@]} i=0
+  while [ "$i" -lt "$n" ] && [ "${toks[$i]}" != "git" ]; do i=$((i+1)); done
+  [ "$i" -lt "$n" ] || return 1
+  i=$((i+1))
+  local pi=-1
+  while [ "$i" -lt "$n" ]; do
+    case "${toks[$i]}" in
+      -C|-c|--git-dir|--work-tree|--namespace|--exec-path|--super-prefix) i=$((i+2)) ;;
+      push) pi=$i; break ;;
+      -*) i=$((i+1)) ;;
+      *) return 1 ;;   # first non-option subcommand isn't push
+    esac
+  done
   [ "$pi" -ge 0 ] || return 1
 
   # Collect positional args after `push`, skipping flags (and value-taking flags).
@@ -154,6 +173,9 @@ forcepush_to_feature_branch() {
   local refspec="${positionals[1]}"
   refspec="${refspec#+}"          # drop leading + (force refspec)
   local dst="${refspec##*:}"      # dst = after last ':' (whole token if no ':')
+  # Normalize a fully-qualified ref down to its branch name so protected targets
+  # written as `refs/heads/main` / `heads/main` are still recognized as `main`.
+  dst="${dst#refs/heads/}"; dst="${dst#heads/}"
   [ -n "$dst" ] || return 1
 
   # Protected / ambiguous destinations stay gated.
@@ -190,20 +212,50 @@ gn '\b(tee|cp|mv|dd|install|rsync|sed|truncate|ln)\b[^|;&]*self-update\.(service
 
 # DB destructive / migration / replay
 gi '\b(DROP[[:space:]]+(TABLE|DATABASE)|TRUNCATE[[:space:]]|FLUSHALL|FLUSHDB)\b'                  && deny "db-destructive" "operator_approval_gated" "$c"
-gi '\b(db:migrate|prisma[[:space:]]+migrate[[:space:]]+(deploy|dev)|alembic[[:space:]]+(upgrade|downgrade)|knex[[:space:]]+migrate)\b' && deny "db-migrate" "operator_approval_gated" "$c"
-g '[[:space:]]replay([[:space:]]|$)'                                                              && deny "replay" "operator_approval_gated" "$c"
+# `db:migrate` only as an actual run invocation (npm/yarn/pnpm/npx run, or make),
+# not the bare token — `grep db:migrate Makefile` used to trip this.
+gi '\b((npm|pnpm|yarn|npx)([[:space:]]+run)?[[:space:]]+db:migrate|make[[:space:]]+db:migrate|prisma[[:space:]]+migrate[[:space:]]+(deploy|dev)|alembic[[:space:]]+(upgrade|downgrade)|knex[[:space:]]+migrate)\b' && deny "db-migrate" "operator_approval_gated" "$c"
+# `replay` only as a broker/worker/gateway subcommand — the bare word matched
+# innocuous greps/filenames like `grep replay app.log` before.
+g '\b(broker|worker|gateway|hermes|a2a|nexus|openclaw)[A-Za-z0-9_-]*[[:space:]]+replay([[:space:]]|$)' && deny "replay" "operator_approval_gated" "$c"
 
 # release / publish / tag-push / repo visibility
-g 'npm[[:space:]]+publish([[:space:]]|$)|gh[[:space:]]+release[[:space:]]+create([[:space:]]|$)|git[[:space:]]+push([[:space:]]|$)[^|;&]*--tags' && deny "release/publish" "operator_review_gated" "$c"
+g '\b(npm|yarn|pnpm)[[:space:]]+publish([[:space:]]|$)|gh[[:space:]]+release[[:space:]]+create([[:space:]]|$)' && deny "release/publish" "operator_review_gated" "$c"
+# tag-push: detect the push subcommand through git global options (-C/-c) just
+# like the force-push gate, then look for --tags/--follow-tags anywhere in it.
+# The old adjacency regex let `git -C /repo push origin --tags` slip the gate.
+if cmd_is_git_push && gn '[[:space:]]--(tags|follow-tags)([[:space:]=]|$)'; then
+  deny "release/publish" "operator_review_gated" "$c"
+fi
 g 'gh[[:space:]]+repo[[:space:]]+edit([[:space:]]|$)[^|;&]*--visibility'                          && deny "repo-visibility" "operator_approval_gated" "$c"
 
-# secret exfil — external transfer of credential files to remote endpoints.
+# secret exfil — external transfer of credential/key FILES to a remote endpoint.
 # Local reads (.env, credentials) are intentionally NOT gated: the operator already has
 # full shell access to the node, so local reads carry no marginal risk. Only network
 # exfil (curl/wget/nc/scp sending secret files to a remote) stays gated.
-gn '\b(curl|wget|nc|ncat|scp|sftp|ftp|rsync)\b[^|;&]*(\.env([^A-Za-z0-9_.-]|$)|\.credentials|\bid_rsa\b|secret|token)' && deny "secret-exfil" "operator_approval_gated" "$c"
+#
+# Order-INDEPENDENT: a network tool anywhere in the command PLUS a credential-file
+# reference anywhere trips the gate. The old single-regex form required the secret
+# token to appear *after* the tool on the same segment, so `cat .env | curl @-`
+# (read the secret, pipe it out) and `base64 key | nc host` slipped straight
+# through. `secret`/`token` are no longer matched as bare words — that blocked
+# ordinary API calls like `curl https://…/token` — only concrete credential files
+# (.env, .credentials, SSH private keys) count. Public keys (…​.pub / …​.pub.pem)
+# are neutralized first so deploying an authorized_keys public key is not gated.
+cexf="$(printf '%s' "$cnp" | sed -E 's#[A-Za-z0-9_./~-]*\.pub(\.pem)?#PUBKEY#g' 2>/dev/null)"
+[ -n "$cexf" ] || cexf="$cnp"
+gexf() { grep -Eq "$1" <<<"$cexf"; }
+_exfil_net='\b(curl|wget|nc|ncat|scp|sftp|ftp|rsync)\b'
+_exfil_secret='(\.env([^A-Za-z0-9_.-]|$)|\.credentials|\bid_(rsa|dsa|ecdsa|ed25519)\b)'
+if gexf "$_exfil_net" && gexf "$_exfil_secret"; then
+  deny "secret-exfil" "operator_approval_gated" "$c"
+fi
 
 # catastrophic rm against absolute / home roots (quote-stripped; long flags too)
-gn '\brm\b([[:space:]]+--?[A-Za-z-]+)*[[:space:]]+(/|~|\$HOME|/root|/etc|/var|/usr|/bin|/lib)([[:space:]/]|$)' && deny "rm-catastrophic" "operator_approval_gated" "$c"
+# The trailing anchor now also accepts `*` so `rm -rf /*` (glob-expands to every
+# top-level dir — as catastrophic as `rm -rf /`) is caught, and `${HOME}` is
+# matched alongside `$HOME`. Relative globs like `rm -rf foo/*` are unaffected:
+# the token right after the flags must still BE a filesystem root.
+gn '\brm\b([[:space:]]+--?[A-Za-z-]+)*[[:space:]]+(/|~|\$HOME|\$\{HOME\}|/root|/etc|/var|/usr|/bin|/lib)([[:space:]/*]|$)' && deny "rm-catastrophic" "operator_approval_gated" "$c"
 
 exit 0