diff --git a/api/route_approvals.py b/api/route_approvals.py index 84e43346c19..aedd15677c5 100644 --- a/api/route_approvals.py +++ b/api/route_approvals.py @@ -3,6 +3,8 @@ State-extraction prelude to the routes.py split tracked in #1907. Extracts approval state, not handlers, by design. """ +import json +import logging import queue import threading import uuid @@ -10,30 +12,32 @@ from api.session_events import publish_session_list_changed +logger = logging.getLogger(__name__) + # Approval system (optional -- graceful fallback if agent not available) try: from tools.approval import ( submit_pending as _submit_pending_raw, - approve_session, - approve_permanent, - save_permanent_allowlist, + approve_session, # noqa: F401 — re-exported for api.routes backward compat + approve_permanent, # noqa: F401 — re-exported for api.routes backward compat + save_permanent_allowlist, # noqa: F401 — re-exported for api.routes backward compat is_approved, _pending, _lock, _permanent_approved, _gateway_queues, - resolve_gateway_approval, + resolve_gateway_approval, # noqa: F401 — re-exported for api.routes backward compat enable_session_yolo, disable_session_yolo, is_session_yolo_enabled, ) except ImportError: _submit_pending_raw = lambda *a, **k: None - approve_session = lambda *a, **k: None - approve_permanent = lambda *a, **k: None - save_permanent_allowlist = lambda *a, **k: None + approve_session = lambda *a, **k: None # noqa: F401 — re-export for api.routes + approve_permanent = lambda *a, **k: None # noqa: F401 — re-export for api.routes + save_permanent_allowlist = lambda *a, **k: None # noqa: F401 — re-export for api.routes is_approved = lambda *a, **k: True - resolve_gateway_approval = lambda *a, **k: 0 + resolve_gateway_approval = lambda *a, **k: 0 # noqa: F401 — re-export for api.routes enable_session_yolo = lambda *a, **k: None disable_session_yolo = lambda *a, **k: None is_session_yolo_enabled = lambda *a, **k: False @@ -536,6 +540,7 @@ def retire_gateway_pending_mirror( if not retired and not gateway_queue_changed: head, total, changed = reconcile_gateway_pending_mirror_locked(session_key) _approval_sse_notify_locked(session_key, head, total) + _relay_child_change_to_parent_locked(session_key) if changed: publish_session_list_changed("attention_resolved") return changed @@ -552,6 +557,7 @@ def retire_gateway_pending_mirror( _pending.pop(session_key, None) head, total, _changed = reconcile_gateway_pending_mirror_locked(session_key) _approval_sse_notify_locked(session_key, head, total) + _relay_child_change_to_parent_locked(session_key) publish_session_list_changed("attention_resolved") return True @@ -766,6 +772,7 @@ def submit_gateway_pending_mirror(session_key: str, approval: dict) -> tuple[dic _normalize_pending_queue_locked(session_key).append(mirror_entry) head, total, _changed = reconcile_gateway_pending_mirror_locked(session_key) _approval_sse_notify_locked(session_key, head, total) + _relay_child_change_to_parent_locked(session_key) publish_session_list_changed("attention_pending") return (dict(head) if head else None), total @@ -789,6 +796,7 @@ def resolve_gateway_pending_local( _gateway_queues.pop(session_key, None) head, total, _changed = reconcile_gateway_pending_mirror_locked(session_key) _approval_sse_notify_locked(session_key, head, total) + _relay_child_change_to_parent_locked(session_key) if target is None: return 0, head, total target.result = choice @@ -841,6 +849,7 @@ def resolve_gateway_pending_local_no_run_mirror( _pending.pop(session_key, None) head, total, _changed = reconcile_gateway_pending_mirror_locked(session_key) _approval_sse_notify_locked(session_key, head, total) + _relay_child_change_to_parent_locked(session_key) target.result = choice if reason: target.reason = reason @@ -935,6 +944,11 @@ def submit_pending(session_key: str, approval: dict) -> None: """ entry = dict(approval) entry.setdefault("approval_id", uuid.uuid4().hex) + if _is_child_approval_key(session_key): + # Bind the queue entry to the profile/state-db that enqueued it so a + # cross-profile process-global queue can never surface profile A's + # pending command under profile B's parent (#6961 r3 MUST-FIX 2). + entry["_child_provenance"] = _child_provenance_current() with _lock: queue_list = _normalize_pending_queue_locked(session_key) queue_list.append(entry) @@ -944,9 +958,435 @@ def submit_pending(session_key: str, approval: dict) -> None: # submit_pending calls can't deliver out-of-order (T2's later # notify arriving before T1's earlier notify with a stale count). _approval_sse_notify_locked(session_key, head, total) + # A child-key enqueue must also reach the parent's SSE subscribers + # (#6961 #6): the parent stream only gets the initial snapshot with + # the child, so later child enqueues would go stale for pure-SSE + # consumers. + _relay_child_change_to_parent_locked(session_key) publish_session_list_changed("attention_pending") # NOTE: We do NOT call _submit_pending_raw here — that function overwrites # _pending[session_key] with a single dict, which would undo the list we just # built. The gateway blocking path uses _gateway_queues (a separate mechanism # managed by check_all_command_guards / register_gateway_notify), which is # unaffected by _pending. The _pending dict is only used for UI polling. + + +# ── Delegated-child approval routing (agent approval-key rebinding contract) ─ +# The agent rebinds a delegated child's approval authority to a child-owned +# key ("subagent:") at the worker boundary (hermes-agent +# PR #82009, the agent-side root-cause fix for nesquena/hermes-webui#6100). +# The WebUI previously only ever read/resolved approvals under the parent +# WebUI session key, so a dangerous child command enqueued under the child key +# was never surfaced and the child retried forever — "approval gets stuck" +# (nesquena/hermes-webui#6943). These helpers route child-key approvals into +# the parent session's UI and resolve them back under the child's key. +_CHILD_APPROVAL_KEY_PREFIX = "subagent:" + +# Read-only marker for child approvals surfaced under a parent session +# (#6961 r3 MUST-FIX 1): child projections get this non-empty sentinel as +# their approval_id so they are never mistaken for an actionable parent +# approval. The legacy resolver rejects it and the frontend renders the +# card inert. +_READ_ONLY_CHILD_SENTINEL = "__read_only_child__:" + +# child id -> parent session key, scoped by canonical state-db/profile path. +# Populated lazily on first sight of a child approval key (state.db fallback +# below); seeded directly by tests via seed_child_parent(). +# +# The cache key includes the canonical state-db path so one profile's child +# resolution can never poison another profile's identical child id, and only +# POSITIVE lookups are cached — a failed/missing lookup is never stored, so a +# late state.db write is picked up on the next call instead of permanently +# resolving to None (#6961 #4). +_child_approval_parents: dict[tuple[str, str], str] = {} + + +def _child_parent_cache_key(child_session_id: str) -> tuple[str, str]: + """Canonical cache key: (state-db/profile path, child session id).""" + from api.models import _active_state_db_path + try: + db_path = _active_state_db_path() + except Exception: + db_path = None + return (str(db_path or ""), child_session_id) + + +def _child_provenance_current() -> str: + """Canonical identity of the currently-active profile's state DB. + + Child queue entries are bound to this identity when enqueued + (``submit_pending``), and ``pending_head_for_session_locked`` filters the + projection by it, so a process-global queue shared across profiles can + never surface profile A's pending command under profile B's parent + (#6961 r3 MUST-FIX 2). + """ + try: + from api.models import _active_state_db_path + return str(_active_state_db_path() or "").strip() + except Exception: + return "" + + +def seed_child_parent(child_session_id: str, parent_session_id: str) -> None: + """Record a child->parent mapping (used by tests and early wiring).""" + with _lock: + _child_approval_parents[_child_parent_cache_key(child_session_id)] = parent_session_id + + +def invalidate_child_parent_cache(child_session_id: str | None = None) -> None: + """Drop cached child->parent mappings (all, or for one child id). + + Call when a child's ownership may have changed (e.g. the state.db row + lands after an early lookup, or the child is re-parented) so the next + lookup re-reads the authoritative row instead of a stale positive. + """ + with _lock: + if child_session_id is None: + _child_approval_parents.clear() + return + for key in [k for k in _child_approval_parents if k[1] == child_session_id]: + _child_approval_parents.pop(key, None) + + +def _is_child_approval_key(key: str) -> bool: + """True when *key* is a delegated-child approval key (``subagent:`` prefix).""" + return isinstance(key, str) and key.startswith(_CHILD_APPROVAL_KEY_PREFIX) + + +def _child_parent_session_id(child_session_id: str) -> str | None: + """Return the parent WebUI session key for a delegated child session id. + + Consults the in-process cache first (keyed by canonical state-db/profile + path + child id), then falls back to the state.db signals the sidebar uses + to identify delegated children (#5307): the ``model_config._delegate_from`` + marker is authoritative, and ``source='subagent'`` + ``parent_session_id`` + is the legacy signal. Only POSITIVE lookups are cached; a failed or + missing lookup is never cached, so a late state.db write is observed on + the next call and one profile's miss cannot poison another's identical + child id (#6961 #4). Any failure resolves to ``None`` (fail-closed: an + unassociated child approval is never surfaced in a session that does not + own it). + """ + cache_key = _child_parent_cache_key(child_session_id) + cached = _child_approval_parents.get(cache_key) + if cached is not None: + return cached + parent: str | None = None + try: + from api.models import _active_state_db_path + from contextlib import closing + from pathlib import Path + import sqlite3 as _sqlite + + db_path = _active_state_db_path() + if db_path and Path(str(db_path)).exists(): + with closing(_sqlite.connect(str(db_path))) as conn: + row = conn.execute( + "SELECT parent_session_id, model_config, source FROM sessions WHERE id = ?", + (child_session_id,), + ).fetchone() + if row: + parent_session_id, raw_model_config, source = row + model_config = {} + config_authoritative = False + if raw_model_config is None: + # Absence of config is authoritative: nothing to parse. + config_authoritative = True + elif isinstance(raw_model_config, str): + if raw_model_config.strip(): + try: + parsed = json.loads(raw_model_config) + if isinstance(parsed, dict): + model_config = parsed + config_authoritative = True + except (TypeError, ValueError): + # Malformed JSON: never fall through to the + # physical-parent branch (#6961 r3 MUST-FIX 3). + config_authoritative = False + else: + config_authoritative = True + elif isinstance(raw_model_config, dict): + model_config = raw_model_config + config_authoritative = True + if "_delegate_from" in model_config: + # Key PRESENCE is authoritative (#6961 r4 #2): an + # explicitly present marker — even empty/null/non-string — + # declares the child's lineage and must NEVER fall through + # to the physical-parent fallback. Only a non-empty string + # value yields a parent; everything else fails closed. + delegate_from_value = model_config.get("_delegate_from") + if isinstance(delegate_from_value, str) and delegate_from_value.strip(): + parent = delegate_from_value.strip() + else: + parent = None + elif config_authoritative and str(source or "").strip().lower() == "subagent": + parent = str(parent_session_id or "").strip() or None + except Exception: + logger.debug("child approval parent lookup failed", exc_info=True) + if parent: + _child_approval_parents[cache_key] = parent + return parent + + +def child_approval_keys_for_session_locked(session_key: str) -> list[str]: + """Return every approval key that belongs to *session_key*. + + Includes the session's own key plus any delegated-child keys + (``subagent:``) whose recorded parent is this session. + + CALLER MUST HOLD `_lock`. Scans only keys that actually carry a pending + entry, so the child->parent mapping stays lazy and costs nothing when no + child approval is live. + """ + keys = [session_key] + seen = {session_key} + for candidate in list(_gateway_queues.keys()) + list(_pending.keys()): + if not _is_child_approval_key(candidate) or candidate in seen: + continue + seen.add(candidate) + child_id = candidate[len(_CHILD_APPROVAL_KEY_PREFIX):] + if _child_parent_session_id(child_id) == session_key: + keys.append(candidate) + return keys + + +def _stable_entry_key(entry: dict) -> str | None: + """Return a stable dedupe identity for one approval entry, or ``None``. + + Mirrors parked in ``_pending`` and their live ``_gateway_queues`` + counterparts are the same approval surfaced twice; dedupe by stable + approval id first, then by the gateway mirror token when the id is + missing (legacy no-id entries). Entries with neither id nor token are + returned as ``None`` and are never deduped (each one is unique). + """ + approval_id = str(entry.get("approval_id") or "").strip() + if approval_id: + return f"id:{approval_id}" + token = str( + entry.get(_GATEWAY_MIRROR_TOKEN) + or entry.get(_GATEWAY_ENTRY_DATA_TOKEN_KEY) + or "" + ).strip() + if token: + return f"token:{token}" + return None + + +def _queue_entries_locked(key: str) -> list[dict]: + """Return the pending entries for *key* as a list of dicts. + + Tolerates the agent's legacy single-dict ``_pending`` value and folds in + live gateway-queue heads (``_ApprovalEntry.data`` payloads). The same + approval can appear both as a ``_pending`` mirror and as a live gateway + entry; entries are deduped by stable approval id / mirror token so the + aggregate count never double-counts one approval (#6961 #5). + + CALLER MUST HOLD `_lock`. + """ + entries: list[dict] = [] + seen: set[str] = set() + + def _append(entry: dict) -> None: + stable = _stable_entry_key(entry) + if stable is not None: + if stable in seen: + return + seen.add(stable) + entries.append(dict(entry)) + + q = _pending.get(key) + if isinstance(q, list): + for entry in q: + _append(entry) + elif q: + _append(q) + for entry in _gateway_queues.get(key) or []: + raw = getattr(entry, "data", None) or {} + if raw: + _append(raw) + return entries + + +def pending_head_for_session_locked(session_key: str) -> tuple[dict | None, int]: + """Return ``(head, total)`` of every pending approval visible for *session_key*. + + The session's own queue comes first, then any delegated-child approvals + routed to this session under the agent#82009 child-key contract (fixes + nesquena/hermes-webui#6943: child approvals were never surfaced, leaving + the child retrying forever). + + CALLER MUST HOLD `_lock`. + """ + entries = _queue_entries_locked(session_key) + current_prov = _child_provenance_current() + for child_key in child_approval_keys_for_session_locked(session_key): + if child_key == session_key: + continue + for entry in _queue_entries_locked(child_key): + entry_prov = str(entry.get("_child_provenance") or "").strip() + if not entry_prov or not current_prov or entry_prov != current_prov: + # Cross-profile guard (#6961 r3 MUST-FIX 2): a child entry + # parked under the same key by another profile's process-global + # queue must never be projected here. Unknown provenance fails + # closed too (#6961 r5 #2): child approvals only arrive via + # submit_pending / the gateway wrap, which bind provenance on + # enqueue, so an empty stamp on EITHER side means state-db + # resolution failed — equality of two empty strings must never + # authorize a projection. + continue + # Mark child projections explicitly read-only (#6961 r3 + # MUST-FIX 1): non-empty sentinel identity (never null/absent), so + # the frontend renders the card inert and the legacy resolver + # rejects it instead of signalling the PARENT's approval. + marked = dict(entry) + marked["approval_id"] = f"{_READ_ONLY_CHILD_SENTINEL}{child_key}" + marked["read_only"] = True + entries.append(marked) + if not entries: + return None, 0 + return dict(entries[0]), len(entries) + + +def _relay_child_change_to_parent_locked(child_key: str) -> None: + """Publish the parent's aggregate head/count after an owned child change. + + The parent SSE subscriber only ever receives the initial snapshot that + includes the child (routes.py `_handle_approval_sse_stream`); later child + enqueue/resolve events under the child key never reached the parent's + stream, so a pure-SSE consumer went stale until the 1.5s HTTP poll fired + (#6961 #6). Whenever an owned child queue changes, push the aggregate + parent head/count to the parent's subscribers. + + CALLER MUST HOLD `_lock`. No-op for non-child keys, unassociated children, + and parents without live SSE subscribers. + """ + if not _is_child_approval_key(child_key): + return + child_id = child_key[len(_CHILD_APPROVAL_KEY_PREFIX):] + parent = _child_parent_session_id(child_id) + if not parent: + return + if not _approval_sse_subscribers.get(parent): + return + head, total = pending_head_for_session_locked(parent) + _approval_sse_notify_locked(parent, head, total) + + +# ── Raw producer provenance boundary (installed Agent enqueue path) ───────── +# The installed Agent's own `tools.approval.submit_pending()` writes a no-ID +# raw dict straight into process-global `_pending` without ever calling the +# WebUI wrapper above, so child-key entries enqueued by the REAL producer +# carried no `_child_provenance` and the projector filtered them out — the +# child approval stayed invisible even though it was pending (#6961 r4 #1). +# Because the WebUI shares the agent module in-process (`_pending`/`_lock`/ +# `_gateway_queues` are the same objects), the fix is to bind the boundary at +# import time: wrap the raw producer so every child-key enqueue gets canonical +# profile/state-db provenance stamped (the same identity the WebUI wrapper +# injects) and the owning parent's aggregate SSE update is relayed — the exact +# treatment the wrapper gives. Gateway entries get the same stamp so mirrors +# preserve it. The wrap is a no-op when the agent module is not importable +# (agent-less WebUI test environments fall back to the stub `_pending`). +_RAW_PRODUCER_BOUND_ATTR = "_webui_raw_producer_provenance_bound" + + +def _wrap_raw_submit_pending(module) -> bool: + """Stamp provenance + relay parent SSE on the raw Agent `submit_pending`.""" + original = module.submit_pending + if getattr(original, _RAW_PRODUCER_BOUND_ATTR, False): + return False + + def _raw_submit_pending_bound(session_key, approval): + if _is_child_approval_key(session_key) and isinstance(approval, dict): + entry = dict(approval) + entry["_child_provenance"] = _child_provenance_current() + original(session_key, entry) + try: + with _lock: + _relay_child_change_to_parent_locked(session_key) + except Exception: + logger.debug("raw child approval SSE relay failed", exc_info=True) + return + return original(session_key, approval) + + _raw_submit_pending_bound.__name__ = original.__name__ + _raw_submit_pending_bound.__doc__ = original.__doc__ + setattr(_raw_submit_pending_bound, _RAW_PRODUCER_BOUND_ATTR, True) + module.submit_pending = _raw_submit_pending_bound + return True + + +def _wrap_raw_gateway_enqueue(module) -> bool: + """Stamp provenance on child-key gateway entries enqueued by the Agent and + publish the parent's aggregate while the entry is actually parked. + + The gateway path parks `_ApprovalEntry(approval_data)` in + `_gateway_queues`; stamping the data dict before the original runs means + the entry's `.data` (same object) carries provenance and every mirror that + copies it preserves the identity. + + The Agent's `_await_gateway_decision` invokes `notify_cb` AFTER the entry + is parked and only removes it right before returning, so a relay in + `finally` alone would fire after the pending entry was already gone — the + parent SSE subscriber would see the removal but never the pending state + (#6961 r5 #1). Wrapping `notify_cb` publishes the parent's initial + aggregate at the exact moment the child entry is visible, before the + worker blocks; the `finally` relay is retained for the removal. + """ + original = getattr(module, "_await_gateway_decision", None) + if original is None or getattr(original, _RAW_PRODUCER_BOUND_ATTR, False): + return False + + def _raw_gateway_decision_bound(session_key, notify_cb, approval_data, *args, **kwargs): + child_path = _is_child_approval_key(session_key) + if child_path and isinstance(approval_data, dict): + approval_data = dict(approval_data) + approval_data.setdefault("_child_provenance", _child_provenance_current()) + if callable(notify_cb): + orig_notify = notify_cb + + def _relay_then_notify(payload): + # The entry is parked RIGHT NOW (the Agent appends it to + # _gateway_queues before invoking notify_cb): publish the + # parent's initial aggregate, then forward the + # notification unchanged. + try: + with _lock: + _relay_child_change_to_parent_locked(session_key) + except Exception: + logger.debug("raw gateway child approval SSE relay failed", exc_info=True) + return orig_notify(payload) + + _relay_then_notify.__name__ = getattr(orig_notify, "__name__", "notify_cb") + notify_cb = _relay_then_notify + try: + return original(session_key, notify_cb, approval_data, *args, **kwargs) + finally: + if child_path: + try: + with _lock: + _relay_child_change_to_parent_locked(session_key) + except Exception: + logger.debug("raw gateway child approval SSE relay failed", exc_info=True) + + _raw_gateway_decision_bound.__name__ = original.__name__ + _raw_gateway_decision_bound.__doc__ = original.__doc__ + setattr(_raw_gateway_decision_bound, _RAW_PRODUCER_BOUND_ATTR, True) + module._await_gateway_decision = _raw_gateway_decision_bound + return True + + +def _install_raw_producer_provenance_hook() -> bool: + """Bind canonical profile/state-db provenance at the installed Agent's raw + enqueue boundary (#6961 r4 #1). No-op when the agent module is absent. + """ + try: + import tools.approval as _tools_approval + except Exception: + return False + bound = _wrap_raw_submit_pending(_tools_approval) + bound = _wrap_raw_gateway_enqueue(_tools_approval) or bound + return bound + + +_install_raw_producer_provenance_hook() + diff --git a/api/routes.py b/api/routes.py index 920a3663299..9d153cf79d8 100644 --- a/api/routes.py +++ b/api/routes.py @@ -10612,6 +10612,9 @@ def _resolve_from_rows(rows: list) -> str | None: set_session_yolo_enabled, submit_gateway_pending_mirror, submit_pending, + pending_head_for_session_locked, + child_approval_keys_for_session_locked, + _queue_entries_locked, ) # Clarify prompts (optional -- graceful fallback if agent not available) @@ -10636,14 +10639,12 @@ def _resolve_from_rows(rows: list) -> str | None: def _session_attention_summary(session_id: str) -> dict | None: """Return sidebar attention metadata for pending approval/clarify work.""" - approval_count = 0 with _lock: reconcile_gateway_pending_mirror_locked(session_id) - queue_list = _pending.get(session_id) - if isinstance(queue_list, list): - approval_count = len(queue_list) - elif queue_list: - approval_count = 1 + # One aggregate projection on every path (own queue + delegated-child + # queues, deduped by stable approval id) so a parent-with-1 + + # child-with-1 lights the dot with count 2, not 1 (#6961 #5). + _head, approval_count = pending_head_for_session_locked(session_id) if approval_count > 0: return { "kind": "approval", @@ -20969,27 +20970,11 @@ def _read_anchored_file_bytes(ws_root: Path, target: Path) -> bytes: def _handle_approval_pending(handler, parsed): sid = parse_qs(parsed.query).get("session_id", [""])[0] with _lock: - _head, _total, _changed = reconcile_gateway_pending_mirror_locked(sid) - queue = _pending.get(sid) - # Support both the new list format and a legacy single-dict value. - if isinstance(queue, list): - p = queue[0] if queue else None - total = len(queue) - elif queue: - p = queue - total = 1 - else: - p = None - total = 0 - if p is None: - gw_queue = _gateway_queues.get(sid) or [] - if gw_queue: - raw = getattr(gw_queue[0], "data", None) or {} - if raw: - p = raw - total = len(gw_queue) - else: - logger.warning("Gateway queue entry for %s has no .data attribute", sid) + reconcile_gateway_pending_mirror_locked(sid) + # One aggregate projection on every path (own queue + delegated-child + # queues, deduped by stable approval id) so a parent-with-1 + + # child-with-1 reports count 2, not 1 (#6961 #5). + p, total = pending_head_for_session_locked(sid) if p: return j(handler, {"pending": dict(p), "pending_count": total}) return j(handler, {"pending": None, "pending_count": 0}) @@ -21018,13 +21003,10 @@ def _handle_approval_sse_stream(handler, parsed): with _lock: _approval_sse_subscribers.setdefault(sid, []).append(q) reconcile_gateway_pending_mirror_locked(sid) - q_list = _pending.get(sid) - if isinstance(q_list, list): - initial_pending = dict(q_list[0]) if q_list else None - initial_count = len(q_list) - elif q_list: - initial_pending = dict(q_list) - initial_count = 1 + # One aggregate projection on every path (own queue + delegated-child + # queues, deduped by stable approval id) so a parent-with-1 + + # child-with-1 opens the stream with count 2, not 1 (#6961 #5). + initial_pending, initial_count = pending_head_for_session_locked(sid) handler.send_response(200) handler.send_header('Content-Type', 'text/event-stream; charset=utf-8') @@ -25865,6 +25847,11 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str, run_id: st Slice 3b keeps the RuntimeAdapter as a protocol translator: it delegates to this legacy helper rather than owning approval queues or callback state. """ + if approval_id and str(approval_id).startswith("__read_only_child__:"): + # Read-only child projection — never resolvable through the parent's + # legacy FIFO resolver (#6961 r3 MUST-FIX 1). The child stays pending; + # signalling here would resolve the PARENT's approval instead. + return False # Pop the targeted entry from the pending queue by approval_id. Old clients # that omit approval_id still resolve the oldest entry for compatibility. pending = None @@ -26010,9 +25997,20 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str, run_id: st gateway_resolved = resolve_gateway_approval(sid, choice, resolve_all=False) or 0 elif not approval_id: gateway_resolved = resolve_gateway_approval(sid, choice, resolve_all=False) or 0 + # Delegated-child approvals (#6943): the agent parks a child's approval + # under "subagent:" (agent#82009 contract). The WebUI + # surfaces those entries read-only under the parent session key; the + # coordinated exact-entry resolve plus agent-side waiter wakeup lands in + # the follow-up gated on the agent contract. A parent click therefore + # reports the count truthfully without claiming it resolved the child. # Keep the historical no-id response path truthy for old clients/tests while # making stale explicit ids bounded as not-active for Slice 3b. - resolved = bool(pending) or bool(gateway_resolved) or bool(local_gateway_resolved) or not bool(approval_id) + resolved = ( + bool(pending) + or bool(gateway_resolved) + or bool(local_gateway_resolved) + or not bool(approval_id) + ) if resolved: publish_session_list_changed("attention_resolved") return resolved @@ -26329,7 +26327,16 @@ def _session_has_pending_approval(sid: str) -> bool: elif queue: return True gw_queue = _gateway_queues.get(sid) - return bool(gw_queue) + if gw_queue: + return True + # A delegated-child approval parked under a child key (agent#82009 + # contract) is still live work for this session (#6943). + for child_key in child_approval_keys_for_session_locked(sid): + if child_key == sid: + continue + if _queue_entries_locked(child_key): + return True + return False def _handle_approval_respond(handler, body): @@ -26340,6 +26347,19 @@ def _handle_approval_respond(handler, body): if choice not in ("once", "session", "always", "deny"): return bad(handler, f"Invalid choice: {choice}") approval_id = body.get("approval_id", "") + + if approval_id and str(approval_id).startswith("__read_only_child__:"): + # Read-only child projection — reject BEFORE any resolver side effect + # (#6961 r4 #4): the sentinel must never reach the gateway relay, the + # local no-run mirror resolver, or the legacy FIFO path. Frontend + # guards already make the card inert; this is the server-side + # belt-and-braces so a crafted/legacy client cannot resolve a parent + # approval by answering a surfaced child card. + return j( + handler, + {"ok": False, "choice": choice, "error": "read_only_child_not_resolvable"}, + status=409, + ) enable_yolo = body.get("yolo") is True requested_run_id = str(body.get("run_id") or "").strip() requested_mirror_token = str(body.get("mirror_token") or "").strip() diff --git a/static/messages.js b/static/messages.js index 77d8cf08090..f597d0261f9 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7313,6 +7313,12 @@ function _updateYoloPill() { } async function toggleYoloFromApproval() { + // Read-only child projection — Skip all / YOLO must be inert (#6961 r4 #4): + // the card cannot be answered through the parent session, and YOLO would + // mutate the parent session without the user approving THIS command. + if (_approvalCurrentId && _approvalCurrentId.indexOf(_READ_ONLY_APPROVAL_PREFIX) === 0) { + return; + } const owner = _captureApprovalResponseOwner(); if (!owner) return false; return !!(await respondApproval('once', {yolo: true, owner})); @@ -7395,6 +7401,10 @@ function hideApprovalCard(force=false) { // Track session_id of the active approval so respond goes to the right session let _approvalSessionId = null; let _approvalCurrentId = null; // approval_id of the card currently shown +// Read-only child-approval projections carry this non-empty sentinel as their +// approval_id (#6961 r3): the card must render inert — every approval control +// disabled — so it can never be answered through the parent's resolver. +const _READ_ONLY_APPROVAL_PREFIX = "__read_only_child__:"; let _approvalPendingBySession = new Map(); let _approvalResponding = null; let _approvalClearedOwner = null; @@ -7599,6 +7609,11 @@ function _setApprovalControlsDisabled(choice, disabled) { b.classList.remove("loading"); } }); + // #6961 r4 #4: the Skip all / YOLO control must be inert on read-only + // (surfaced-child) cards and while a response is in flight — it mutates the + // parent session and must never be reachable from an answered card. + const skipAll = $("approvalSkipAll"); + if (skipAll) skipAll.disabled = !!disabled; } function showApprovalForSession(sid, pending, pendingCount) { @@ -7651,9 +7666,13 @@ function showApprovalCard(pending, pendingCount) { card.classList.remove("collapsed"); } const responding = _approvalResponseMatches(sid, _approvalCurrentId); + // Read-only child projections (#6961 r3): render the card inert — every + // approval control disabled — so it can never be answered through the + // parent's resolver. Never focus the "Allow once" button either. + const readOnly = !!(_approvalCurrentId && _approvalCurrentId.indexOf(_READ_ONLY_APPROVAL_PREFIX) === 0); _setApprovalControlsDisabled( - responding ? (_approvalResponding.controlChoice || _approvalResponding.choice) : null, - responding, + readOnly ? null : (responding ? (_approvalResponding.controlChoice || _approvalResponding.choice) : null), + readOnly || responding, ); _setPromptFlyoutHidden(card, false); card.classList.add("visible"); @@ -7661,7 +7680,7 @@ function showApprovalCard(pending, pendingCount) { _syncApprovalTranscriptSpace(card, {immediate: true}); if (typeof applyLocaleToDOM === "function") applyLocaleToDOM(); const onceBtn = $("approvalBtnOnce"); - if (onceBtn && document.activeElement !== $('msg')) { + if (onceBtn && !readOnly && document.activeElement !== $('msg')) { setTimeout(() => onceBtn.focus({preventScroll: true}), 50); } if (typeof syncTopbar === 'function') syncTopbar(); @@ -7750,6 +7769,11 @@ function toggleApprovalCardCollapsed(forceCollapsed) { } async function respondApproval(choice, options = {}) { + if (_approvalCurrentId && _approvalCurrentId.indexOf(_READ_ONLY_APPROVAL_PREFIX) === 0) { + // Read-only child projection — never resolvable from the frontend + // (#6961 r3 MUST-FIX 1). Belt-and-braces alongside the disabled controls. + return; + } const owner = options.owner || _captureApprovalResponseOwner(); if (!_approvalResponseOwnerIsCurrent(owner)) return false; const {sid, approvalId} = owner; diff --git a/tests/test_6961_child_approval_read_only.py b/tests/test_6961_child_approval_read_only.py new file mode 100644 index 00000000000..9ed747b5d12 --- /dev/null +++ b/tests/test_6961_child_approval_read_only.py @@ -0,0 +1,612 @@ +"""Regression tests for #6961 round-3 re-gate (read-only end-to-end child approvals). + +The r3 review found three MUST-FIXes: +1. (CORE) The surfaced child card was actionable and routed into the PARENT's + legacy FIFO resolver — production child entries have no approval_id, so a + click signalled the parent's approval while the child stayed pending. + Fix: child projections carry a non-empty read-only sentinel approval_id and + the legacy resolver rejects it. +2. (SILENT) Cross-profile leak: _pending/_gateway_queues are process-global, + so an identical child id across two profiles could surface profile A's + pending command under profile B's parent. Fix: entries are bound to the + enqueuing profile's state-db and the projection filters by that identity. +3. (SILENT) Malformed model_config fell through to the wrong physical parent. + Fix: only the authoritative (parsed/absent) config may take the + physical-parent fallback; malformed JSON fails closed. +""" +import json +import pathlib +import re +import sqlite3 +import sys + +import pytest + +REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve() +sys.path.insert(0, str(REPO_ROOT)) + +# Import order matters: `api.routes` loads `api.config`, which appends the +# discovered hermes-agent dir to sys.path (api/config.py). Importing it FIRST +# means `api.route_approvals` binds the REAL `tools.approval` module +# (`_pending`/`_lock`/`_gateway_queues` shared in-process) instead of the +# no-agent stub fallback — the same binding the production server gets, and +# the one the raw-producer regressions below must exercise. +from api import routes as r # noqa: E402 +from api import route_approvals as ra # noqa: E402 + +_SENTINEL = "__read_only_child__:" + + +def _clear(*session_keys: str) -> None: + with ra._lock: + for key in session_keys: + r._pending.pop(key, None) + r._gateway_queues.pop(key, None) + ra._child_approval_parents.clear() + + +def _seed(parent: str, child: str) -> str: + child_key = f"subagent:{child}" + _clear(parent, child_key) + ra.seed_child_parent(child, parent) + return child_key + + +# --------------------------------------------------------------------------- +# MUST-FIX 1 (CORE) — read-only sentinel projection +# --------------------------------------------------------------------------- + +def test_child_projection_carries_read_only_sentinel(): + """The projected child head must be inert: sentinel id + read_only flag.""" + parent = "test-6961-sentinel-parent" + child = "test-6961-sentinel-child" + child_key = _seed(parent, child) + try: + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert head is not None + assert head["command"] == "childcmd" + assert head["read_only"] is True, "child projection must be flagged read-only" + assert str(head.get("approval_id") or "").startswith(_SENTINEL), ( + "child projection must carry the non-empty read-only sentinel id, " + "never the raw null/absent production approval_id" + ) + assert total == 1 + # The underlying child entry keeps its own identity under the child key. + with ra._lock: + raw = r._pending[child_key][0] + assert not str(raw.get("approval_id") or "").startswith(_SENTINEL) + finally: + _clear(parent, child_key) + + +def test_legacy_resolver_rejects_read_only_sentinel(): + """A sentinel approval_id must never resolve through the parent resolver.""" + parent = "test-6961-resolver-parent" + child = "test-6961-resolver-child" + child_key = _seed(parent, child) + try: + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with ra._lock: + head, _total = ra.pending_head_for_session_locked(parent) + assert head is not None + sentinel_id = str(head["approval_id"]) + # Attempting to answer the surfaced child through the parent's legacy + # FIFO resolver must fail closed (return False, nothing resolved). + resolved = r._resolve_approval_legacy(parent, sentinel_id, "once") + assert resolved is False + # The child entry stays pending untouched. + with ra._lock: + assert len(r._pending[child_key]) == 1 + finally: + _clear(parent, child_key) + + +def test_frontend_disables_controls_for_read_only_card(): + """messages.js must disable every approval control for sentinel cards.""" + src = pathlib.Path(REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") + assert "_READ_ONLY_APPROVAL_PREFIX" in src + assert "_setApprovalControlsDisabled(" in src + # The read-only branch must disable controls unconditionally. + assert "readOnly || responding" in src, ( + "read-only cards must disable every approval control" + ) + # respondApproval must refuse sentinel ids outright. + assert "_READ_ONLY_APPROVAL_PREFIX) === 0" in src + + +# --------------------------------------------------------------------------- +# MUST-FIX 2 (SILENT) — cross-profile provenance filter +# --------------------------------------------------------------------------- + +def test_cross_profile_child_entry_not_projected(monkeypatch): + """A child entry parked by profile A must never surface under profile B.""" + parent = "test-6961-prov-parent" + child = "test-6961-prov-child" + child_key = _seed(parent, child) + try: + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "profileA-db") + r.submit_pending( + child_key, + {"command": "acmd", "pattern_key": "ap", "pattern_keys": ["ap"], "description": "ad"}, + ) + # Same process, profile B active: the entry must not be projected. + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "profileB-db") + with ra._lock: + head_b, total_b = ra.pending_head_for_session_locked(parent) + assert head_b is None and total_b == 0, ( + "cross-profile child entry must be filtered from the projection" + ) + # Back on profile A the entry is visible again. + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "profileA-db") + with ra._lock: + head_a, total_a = ra.pending_head_for_session_locked(parent) + assert head_a is not None and total_a == 1 + finally: + monkeypatch.setattr(ra, "_child_provenance_current", ra._child_provenance_current.__wrapped__ if hasattr(ra._child_provenance_current, "__wrapped__") else ra._child_provenance_current) + _clear(parent, child_key) + + +# --------------------------------------------------------------------------- +# MUST-FIX 3 (SILENT) — malformed model_config fails closed +# --------------------------------------------------------------------------- + +def test_malformed_model_config_fails_closed(tmp_path, monkeypatch): + """Malformed model_config must never fall through to the physical parent.""" + db_path = tmp_path / "state.db" + conn = sqlite3.connect(str(db_path)) + try: + conn.execute("CREATE TABLE sessions (id TEXT PRIMARY KEY, parent_session_id TEXT, model_config TEXT, source TEXT)") + conn.execute( + "INSERT INTO sessions VALUES (?, ?, ?, ?)", + ("malformed-child", "physical-parent", "{not-valid-json", "subagent"), + ) + conn.commit() + finally: + conn.close() + + monkeypatch.setattr(ra, "_child_parent_cache_key", lambda child: ("test-malformed-db", child)) + from api import models as api_models + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: str(db_path)) + with ra._lock: + ra._child_approval_parents.clear() + try: + parent = ra._child_parent_session_id("malformed-child") + assert parent is None, ( + "malformed model_config must fail closed (no physical-parent fallback)" + ) + finally: + with ra._lock: + ra._child_approval_parents.clear() + + +def test_valid_delegate_from_still_resolves(tmp_path, monkeypatch): + """A well-formed _delegate_from must still win over the physical parent.""" + db_path = tmp_path / "state.db" + conn = sqlite3.connect(str(db_path)) + try: + conn.execute("CREATE TABLE sessions (id TEXT PRIMARY KEY, parent_session_id TEXT, model_config TEXT, source TEXT)") + conn.execute( + "INSERT INTO sessions VALUES (?, ?, ?, ?)", + ("good-child", "physical-parent", json.dumps({"_delegate_from": "logical-parent"}), "subagent"), + ) + conn.commit() + finally: + conn.close() + + monkeypatch.setattr(ra, "_child_parent_cache_key", lambda child: ("test-good-db", child)) + from api import models as api_models + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: str(db_path)) + with ra._lock: + ra._child_approval_parents.clear() + try: + parent = ra._child_parent_session_id("good-child") + assert parent == "logical-parent" + finally: + with ra._lock: + ra._child_approval_parents.clear() + + +# --------------------------------------------------------------------------- +# MUST-FIX 2 (r4) — explicit-empty/null lineage fails open → key presence +# --------------------------------------------------------------------------- + +def _lineage_row(db_path, child_id, raw_config, source, physical_parent): + """Insert one lineage-matrix row into a fresh state.db.""" + conn = sqlite3.connect(str(db_path)) + try: + conn.execute("CREATE TABLE IF NOT EXISTS sessions (id TEXT PRIMARY KEY, parent_session_id TEXT, model_config TEXT, source TEXT)") + conn.execute( + "INSERT OR REPLACE INTO sessions VALUES (?, ?, ?, ?)", + (child_id, physical_parent, raw_config, source), + ) + conn.commit() + finally: + conn.close() + + +@pytest.mark.parametrize( + "raw_config,source,physical_parent,expected", + [ + # Genuinely absent marker + legacy subagent signal → physical parent. + (None, "subagent", "physical-parent", "physical-parent"), + (json.dumps({"other": 1}), "subagent", "physical-parent", "physical-parent"), + # Present non-empty string marker wins. + (json.dumps({"_delegate_from": "logical-parent"}), "subagent", "physical-parent", "logical-parent"), + # Explicitly present EMPTY marker: authoritative fail-closed, never the + # physical-parent fallback (#6961 r4 #2). + (json.dumps({"_delegate_from": ""}), "subagent", "physical-parent", None), + # Explicitly present NULL marker: fail closed. + (json.dumps({"_delegate_from": None}), "subagent", "physical-parent", None), + # Explicitly present non-string marker: fail closed. + (json.dumps({"_delegate_from": 123}), "subagent", "physical-parent", None), + (json.dumps({"_delegate_from": []}), "subagent", "physical-parent", None), + # Malformed / non-dict config: fail closed (no physical-parent fallback). + ("{not-valid-json", "subagent", "physical-parent", None), + (json.dumps([1, 2]), "subagent", "physical-parent", None), + # Absent marker + non-subagent source: no parent. + (None, "api_server", "physical-parent", None), + ], +) +def test_lineage_matrix_fails_closed(tmp_path, monkeypatch, raw_config, source, physical_parent, expected): + """_delegate_from key PRESENCE must be distinguished from absence. + + An explicitly present empty/null/non-string marker declares the child's + lineage and must never fall through to the physical-parent fallback; only + a genuinely absent marker may take the legacy `source='subagent'` path. + """ + db_path = tmp_path / "state.db" + child_id = f"lineage-{abs(hash((str(raw_config), source)))}-{len(list(tmp_path.iterdir()))}" + _lineage_row(db_path, child_id, raw_config, source, physical_parent) + monkeypatch.setattr(ra, "_child_parent_cache_key", lambda child: ("test-lineage-db", child)) + from api import models as api_models + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: str(db_path)) + with ra._lock: + ra._child_approval_parents.clear() + try: + parent = ra._child_parent_session_id(child_id) + assert parent == expected, ( + f"raw_config={raw_config!r} source={source!r} -> {parent!r}, expected {expected!r}" + ) + finally: + with ra._lock: + ra._child_approval_parents.clear() + + +# --------------------------------------------------------------------------- +# MUST-FIX 1 (r4) — real raw-producer provenance + raw SSE relay +# --------------------------------------------------------------------------- + +def _raw_tools_approval(): + """The installed Agent's raw `tools.approval` module, or skip.""" + return pytest.importorskip("tools.approval") + + +def test_raw_producer_child_enqueue_gets_provenance_and_surfaces(): + """The REAL Agent `tools.approval.submit_pending()` must stamp canonical + provenance on child-key entries so the projector surfaces them. + + The r3 projector filters child entries by `_child_provenance`; the raw + producer never called the WebUI wrapper, so the real child row was + filtered instead of surfaced. The import-time boundary hook must make the + raw path equivalent to the wrapper path (#6961 r4 #1). + """ + ta = _raw_tools_approval() + parent = "test-6961-raw-parent" + child = "test-6961-raw-child" + child_key = _seed(parent, child) + try: + ta.submit_pending( + child_key, + {"command": "rawchildcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + # The raw entry itself now carries the enqueuing profile's provenance. + with ra._lock: + raw = ra._pending[child_key] + assert isinstance(raw, dict), "raw producer stores a legacy single dict" + assert str(raw.get("_child_provenance") or "").strip(), ( + "raw child enqueue must stamp _child_provenance (found empty)" + ) + # And the projector now surfaces it instead of filtering it out. + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert total == 1 + assert head is not None and head["command"] == "rawchildcmd" + assert head["read_only"] is True + assert str(head.get("approval_id") or "").startswith(_SENTINEL) + finally: + _clear(parent, child_key) + + +def test_raw_producer_relays_parent_sse_subscriber(): + """A raw Agent child enqueue must push the aggregate to the parent SSE + subscriber — the relay was previously only wired into the WebUI wrapper + path (#6961 r4 #3).""" + ta = _raw_tools_approval() + parent = "test-6961-raw-sse-parent" + child = "test-6961-raw-sse-child" + child_key = _seed(parent, child) + q = ra._approval_sse_subscribe(parent) + try: + ta.submit_pending( + child_key, + {"command": "rawssechild", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + payload = q.get(timeout=2) + assert payload["pending_count"] == 1 + assert payload["pending"]["command"] == "rawssechild" + assert str(payload["pending"].get("approval_id") or "").startswith(_SENTINEL) + finally: + ra._approval_sse_unsubscribe(parent, q) + _clear(parent, child_key) + + +def test_raw_producer_two_profile_same_child_id(monkeypatch): + """Raw enqueue under profile A must never surface under profile B, even + with the same child id (process-global queue, raw producer path).""" + ta = _raw_tools_approval() + parent = "test-6961-raw-prov-parent" + child = "test-6961-raw-prov-child" + child_key = _seed(parent, child) + try: + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "rawProfileA-db") + ta.submit_pending( + child_key, + {"command": "rawacmd", "pattern_key": "ap", "pattern_keys": ["ap"], "description": "ad"}, + ) + # Same process, profile B active: filtered from the projection. + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "rawProfileB-db") + with ra._lock: + head_b, total_b = ra.pending_head_for_session_locked(parent) + assert head_b is None and total_b == 0, ( + "raw cross-profile child entry must be filtered from the projection" + ) + # Back on profile A the raw entry is visible again. + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "rawProfileA-db") + with ra._lock: + head_a, total_a = ra.pending_head_for_session_locked(parent) + assert head_a is not None and total_a == 1 + finally: + _clear(parent, child_key) + + +# --------------------------------------------------------------------------- +# MUST-FIX 1 (r4) — sentinel with a SIMULTANEOUS parent approval +# --------------------------------------------------------------------------- + +def test_sentinel_does_not_resolve_simultaneous_parent_approval(): + """Answering a surfaced child card (sentinel id) while the parent has its + OWN pending approval must resolve nothing — the parent approval stays + intact and the child stays pending (#6961 r4 #4).""" + parent = "test-6961-simul-parent" + child = "test-6961-simul-child" + child_key = _seed(parent, child) + try: + r.submit_pending( + parent, + {"command": "parentcmd", "pattern_key": "pp", "pattern_keys": ["pp"], "description": "pd"}, + ) + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert total == 2, "parent-own + child must both be visible" + assert head["command"] == "parentcmd" + sentinel_id = f"{_SENTINEL}{child_key}" + + # Legacy resolver: sentinel must fail closed and resolve NOTHING. + resolved = r._resolve_approval_legacy(parent, sentinel_id, "once") + assert resolved is False + with ra._lock: + assert len(r._pending[parent]) == 1, "parent approval must stay pending" + assert len(r._pending[child_key]) == 1, "child entry must stay pending" + + # HTTP respond handler: sentinel rejected before any resolver side + # effect (409), parent approval still untouched. + import io + captured_status = {} + handler = type("H", (), { + "wfile": io.BytesIO(), + "send_response": lambda self, s: captured_status.__setitem__("status", s), + "send_header": lambda self, k, v: None, + "end_headers": lambda self: None, + })() + r._handle_approval_respond( + handler, + {"session_id": parent, "choice": "once", "approval_id": sentinel_id}, + ) + assert captured_status.get("status") == 409, ( + "respond with a sentinel approval_id must be rejected with 409" + ) + response_body = json.loads(handler.wfile.getvalue().decode("utf-8")) + assert response_body.get("ok") is False + with ra._lock: + assert len(r._pending[parent]) == 1, ( + "respond with sentinel must not consume the parent approval" + ) + finally: + _clear(parent, child_key) + + +# --------------------------------------------------------------------------- +# MUST-FIX 4 (r4) — full-control frontend regressions (Skip all / YOLO) +# --------------------------------------------------------------------------- + +def test_frontend_disables_skip_all_for_read_only_card(): + """Every card action must be inert on read-only cards — including Skip all + / YOLO, which previously stayed wired to toggleYoloFromApproval() and + mutated the parent session (#6961 r4 #4).""" + src = pathlib.Path(REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") + # The disabled set must now include the Skip all / YOLO control. + assert '"approvalSkipAll"' in src, ( + "_setApprovalControlsDisabled must reference the Skip all button" + ) + assert "skipAll.disabled = !!disabled" in src, ( + "the Skip all button must be disabled together with the other controls" + ) + # toggleYoloFromApproval must refuse read-only sentinel cards outright. + assert "async function toggleYoloFromApproval()" in src + func_start = src.index("async function toggleYoloFromApproval()") + toggle_body = src[func_start:func_start + 700] + assert "_READ_ONLY_APPROVAL_PREFIX) === 0" in toggle_body, ( + "toggleYoloFromApproval must early-return on read-only sentinel cards" + ) + assert "return;" in toggle_body.split("const sid = S.session")[0], ( + "the sentinel guard must return BEFORE the /api/session/yolo call" + ) + # Belt-and-braces: respondApproval also still refuses the sentinel. + assert src.count("_READ_ONLY_APPROVAL_PREFIX) === 0") >= 2 + + +# --------------------------------------------------------------------------- +# Round-5 regressions — unknown provenance fails closed + gateway initial +# pending relay (#6961 r5) +# --------------------------------------------------------------------------- + +def test_unknown_provenance_fails_closed_when_resolution_empty(monkeypatch): + """state-db resolution failing on BOTH sides (entry_prov == current_prov + == "") must fail closed: equality of two empty strings never authorizes a + child projection (#6961 r5 #2).""" + ta = _raw_tools_approval() + parent = "test-6961-empty-prov-parent" + child = "test-6961-empty-prov-child" + child_key = _seed(parent, child) + try: + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "") + ta.submit_pending( + child_key, + {"command": "noprovcmd", "pattern_key": "np", "pattern_keys": ["np"], "description": "nd"}, + ) + # The raw entry really was stamped empty (resolution failed at enqueue). + with ra._lock: + raw = ra._pending[child_key] + assert isinstance(raw, dict) + assert not str(raw.get("_child_provenance") or "").strip(), ( + "raw enqueue with failed resolution must stamp empty provenance" + ) + # Empty == empty must NOT authorize the projection. + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert head is None and total == 0, ( + "unknown provenance (empty on both sides) must fail closed" + ) + finally: + _clear(parent, child_key) + + +def test_provenance_requires_both_sides_non_empty(monkeypatch): + """A child entry is only projected when BOTH the entry and the current + provenance are non-empty AND equal — an empty stamp on either side fails + closed even when the other side is known (#6961 r5 #2).""" + parent = "test-6961-both-side-parent" + child = "test-6961-both-side-child" + child_key = _seed(parent, child) + try: + # Entry has no provenance at all, current is known -> filtered. + with ra._lock: + ra._pending[child_key] = [ + {"command": "stale", "pattern_key": "s", "pattern_keys": ["s"], "description": "sd"} + ] + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "known-db") + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert head is None and total == 0, ( + "empty entry provenance must fail closed even with a known current side" + ) + # Entry is known, current resolution returns empty -> filtered. + with ra._lock: + ra._pending[child_key] = [ + {"command": "stale", "pattern_key": "s", "pattern_keys": ["s"], "description": "sd", + "_child_provenance": "known-db"} + ] + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "") + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert head is None and total == 0, ( + "empty current provenance (state-db resolution failure) must fail closed" + ) + # Sanity: both known and equal still authorizes. + with ra._lock: + ra._pending[child_key] = [ + {"command": "stale", "pattern_key": "s", "pattern_keys": ["s"], "description": "sd", + "_child_provenance": "known-db"} + ] + monkeypatch.setattr(ra, "_child_provenance_current", lambda: "known-db") + with ra._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert head is not None and total == 1 + finally: + _clear(parent, child_key) + + +def test_gateway_child_enqueue_relays_initial_pending_to_parent_sse(): + """A child-key gateway enqueue must publish the parent's INITIAL aggregate + while the worker is still blocked (entry parked), not only the removal + after `_await_gateway_decision` returns (#6961 r5 #1).""" + import threading + + ta = _raw_tools_approval() + parent = "test-6961-gw-sse-parent" + child = "test-6961-gw-sse-child" + child_key = _seed(parent, child) + q = ra._approval_sse_subscribe(parent) + notified = [] + t = None + try: + def _worker(): + ta._await_gateway_decision( + child_key, + notified.append, + {"command": "gwchild", "pattern_key": "gp", "pattern_keys": ["gp"], "description": "gd"}, + surface="gateway", + ) + + t = threading.Thread(target=_worker, daemon=True) + t.start() + # The worker parks the entry and invokes notify_cb BEFORE blocking; the + # wrapped callback must already have relayed the parent's aggregate. + payload = q.get(timeout=5) + assert payload["pending_count"] == 1, ( + "parent must see the child pending while the gateway worker is blocked" + ) + assert payload["pending"]["command"] == "gwchild" + assert payload["pending"]["read_only"] is True + assert str(payload["pending"].get("approval_id") or "").startswith(_SENTINEL) + # The original notify_cb still fires, with the stamped data. + assert notified and notified[0]["command"] == "gwchild" + assert str(notified[0].get("_child_provenance") or "").strip(), ( + "gateway approval data must carry provenance" + ) + # The worker is provably still blocked with the entry parked. + with ra._lock: + parked = list(ta._gateway_queues.get(child_key, [])) + assert len(parked) == 1, "worker must still be blocked with the entry parked" + assert t.is_alive() + + # Resolve the entry: the worker unblocks, drops the entry, and the + # retained finally relay publishes the removal to the parent. + parked[0].result = "once" + parked[0].event.set() + t.join(timeout=5) + assert not t.is_alive(), "resolving the gateway entry must unblock the worker" + removal = q.get(timeout=2) + assert removal["pending_count"] == 0 and removal["pending"] is None, ( + "resolving the gateway entry must relay the removal to the parent" + ) + finally: + if t is not None and t.is_alive(): + with ra._lock: + for entry in ta._gateway_queues.get(child_key, []): + entry.event.set() + t.join(timeout=2) + ra._approval_sse_unsubscribe(parent, q) + _clear(parent, child_key) diff --git a/tests/test_approval_queue.py b/tests/test_approval_queue.py index 4de98b72233..6307065216f 100644 --- a/tests/test_approval_queue.py +++ b/tests/test_approval_queue.py @@ -248,3 +248,397 @@ def test_stale_explicit_approval_id_does_not_pop_oldest_entry(): with r._lock: r._pending.pop(sid, None) + + +# --------------------------------------------------------------------------- +# Delegated-child approval routing (#6943) +# +# The agent rebinds a delegated child's approval authority to a child-owned +# key "subagent:" (hermes-agent #82009 contract). These +# tests prove the WebUI surfaces those child-key approvals under the parent +# session key. The coordinated exact-entry resolve plus agent-side waiter +# wakeup lands in the follow-up gated on the agent contract. +# --------------------------------------------------------------------------- + +def _seed_child_parent(child_session_id: str, parent_session_id: str) -> None: + from api import route_approvals as ra + + ra.seed_child_parent(child_session_id, parent_session_id) + + +def _clear_approval_state(*session_keys: str) -> None: + from api import routes as r + from api import route_approvals as ra + + with ra._lock: + for key in session_keys: + r._pending.pop(key, None) + r._gateway_queues.pop(key, None) + ra._child_approval_parents.clear() + + +def test_child_approval_surfaced_under_parent_key(): + """A child-key approval must be visible when polling the parent session.""" + from api import routes as r + + parent = "test-child-parent-surfaced" + child = "test-child-surfaced" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with r._lock: + head, total = r.pending_head_for_session_locked(parent) + assert head is not None, "child approval must be surfaced under parent key" + assert head["command"] == "childcmd" + assert total == 1 + # The parent's own queue stays untouched; the entry lives under the + # child key so the agent-side child resolution still finds it. + with r._lock: + assert not r._pending.get(parent) + assert len(r._pending[child_key]) == 1 + finally: + _clear_approval_state(parent, child_key) + + +def test_parent_approval_unchanged_by_child_routing(): + """A normal parent-key approval resolves exactly as before the fix.""" + from api import routes as r + + parent = "test-child-parent-normal" + child = "test-child-normal" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + r.submit_pending( + parent, + {"command": "parentcmd", "pattern_key": "pp", "pattern_keys": ["pp"], "description": "pd"}, + ) + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with r._lock: + parent_aid = r._pending[parent][0]["approval_id"] + child_aid = r._pending[child_key][0]["approval_id"] + + # Parent head must be its own approval, not the child's. + with r._lock: + head, total = r.pending_head_for_session_locked(parent) + assert head["approval_id"] == parent_aid + assert total == 2 + + assert r._resolve_approval_legacy(parent, parent_aid, "once") is True + with r._lock: + assert parent not in r._pending + assert len(r._pending[child_key]) == 1 + assert r._pending[child_key][0]["approval_id"] == child_aid + # A stale explicit child id must not resolve the unrelated parent head + # (#527 guard) — and here nothing is pending for the parent at all. + assert r._resolve_approval_legacy(parent, "missing-id", "deny") is False + finally: + _clear_approval_state(parent, child_key) + + +def test_session_has_pending_approval_sees_child_approval(): + """_session_has_pending_approval must count child-key work as live.""" + from api import routes as r + + parent = "test-child-parent-haspending" + child = "test-child-haspending" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + assert r._session_has_pending_approval(parent) is False + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + assert r._session_has_pending_approval(parent) is True + finally: + _clear_approval_state(parent, child_key) + + +def test_attention_summary_lights_for_child_approval(): + """The sidebar attention dot must light when only a child approval is live.""" + from api import routes as r + + parent = "test-child-parent-attn" + child = "test-child-attn" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + assert r._session_attention_summary(parent) is None + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + summary = r._session_attention_summary(parent) + assert summary is not None + assert summary["kind"] == "approval" + assert summary["count"] == 1 + finally: + _clear_approval_state(parent, child_key) + + +def test_unassociated_child_approval_not_surfaced(): + """A child key with no recorded parent must fail closed (never surfaced).""" + from api import routes as r + + parent = "test-child-parent-unassoc" + child = "test-child-unassoc" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + try: + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + with r._lock: + head, total = r.pending_head_for_session_locked(parent) + assert head is None + assert total == 0 + with r._lock: + aid = r._pending[child_key][0]["approval_id"] + # Explicit-id respond must fail closed: the entry stays parked under + # the child key, never surfaced into an unrelated parent session. + assert r._resolve_approval_legacy(parent, aid, "once") is False + with r._lock: + assert child_key in r._pending, "unassociated child entry must stay parked" + finally: + _clear_approval_state(parent, child_key) + + +# --------------------------------------------------------------------------- +# Read/surface half of the #6961 review: cache scoping (#4), aggregate count +# on all three surface paths (#5), and child-change SSE relay to the parent +# (#6). The resolve half (#1/#2/#3) stays in the follow-up gated on the agent +# contract, exactly as the maintainer suggested. +# --------------------------------------------------------------------------- + +def test_child_parent_cache_scoped_by_state_db_profile(monkeypatch): + """#4: a cached parent lookup must never leak across state-db profiles.""" + import pathlib + from api import route_approvals as ra + from api import models as api_models + + child = "test-child-cache-scope" + profile_a = pathlib.Path("/tmp/__profile_a__/state.db") + profile_b = pathlib.Path("/tmp/__profile_b__/state.db") + _clear_approval_state() + try: + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: profile_a) + ra.seed_child_parent(child, "parent-a") + assert ra._child_parent_session_id(child) == "parent-a" + + # Switching profile must NOT see profile A's cached parent — the + # entry is keyed by canonical state-db path + child id (#6961 #4). + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: profile_b) + assert ra._child_parent_session_id(child) is None + + # Profile B can record its own mapping independently. + ra.seed_child_parent(child, "parent-b") + assert ra._child_parent_session_id(child) == "parent-b" + + # Back on profile A, the original mapping is still intact. + monkeypatch.setattr(api_models, "_active_state_db_path", lambda: profile_a) + assert ra._child_parent_session_id(child) == "parent-a" + finally: + monkeypatch.undo() + _clear_approval_state() + + +def test_child_parent_cache_does_not_cache_misses(): + """#4: a failed lookup must not be cached, so a late DB write is seen.""" + from api import route_approvals as ra + + child = "test-child-cache-miss" + _clear_approval_state() + try: + # Unknown child -> None, and NOT cached (no negative-cache poison). + assert ra._child_parent_session_id(child) is None + assert ra._child_parent_cache_key(child) not in ra._child_approval_parents + # After the mapping is seeded (simulating a late state.db write), + # the next lookup succeeds — the miss was not cached. + ra.seed_child_parent(child, "parent-late") + assert ra._child_parent_session_id(child) == "parent-late" + finally: + _clear_approval_state() + + +def test_child_parent_cache_invalidates_on_ownership_change(): + """#4: invalidate_child_parent_cache must drop stale positives.""" + from api import route_approvals as ra + + child = "test-child-cache-invalidate" + _clear_approval_state() + try: + ra.seed_child_parent(child, "parent-old") + assert ra._child_parent_session_id(child) == "parent-old" + ra.invalidate_child_parent_cache(child) + assert ra._child_parent_cache_key(child) not in ra._child_approval_parents + ra.seed_child_parent(child, "parent-new") + assert ra._child_parent_session_id(child) == "parent-new" + # Full clear also works. + ra.invalidate_child_parent_cache() + assert not ra._child_approval_parents + finally: + _clear_approval_state() + + +def test_aggregate_count_includes_child_when_parent_has_approval(): + """#5: parent-1 + child-1 must count 2 on every surface path.""" + from api import routes as r + from api import route_approvals as ra + + parent = "test-child-parent-aggr" + child = "test-child-aggr" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + r.submit_pending( + parent, + {"command": "parentcmd", "pattern_key": "pp", "pattern_keys": ["pp"], "description": "pd"}, + ) + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + # Aggregate projection: count 2 (parent 1 + child 1). + with r._lock: + head, total = ra.pending_head_for_session_locked(parent) + assert total == 2 + assert head["command"] == "parentcmd" # own head stays first + # Attention summary: count 2, not 1. + summary = r._session_attention_summary(parent) + assert summary is not None + assert summary["kind"] == "approval" + assert summary["count"] == 2 + finally: + _clear_approval_state(parent, child_key) + + +def test_aggregate_dedupes_mirror_and_gateway_representation(): + """#5: the same approval in _pending (mirror) and _gateway_queues counts once.""" + from api import routes as r + from api import route_approvals as ra + + parent = "test-child-parent-dedupe" + _clear_approval_state(parent) + try: + r.submit_pending( + parent, + {"command": "cmd", "pattern_key": "pk", "pattern_keys": ["pk"], "description": "d"}, + ) + with r._lock: + q = r._pending[parent] + aid = q[0]["approval_id"] + with ra._lock: + # Simulate the gateway mirror representation of the SAME approval + # parked in _gateway_queues (data carries the same approval_id). + entry = type("Entry", (), {"data": {"command": "cmd", "approval_id": aid}})() + r._gateway_queues.setdefault(parent, []).append(entry) + head, total = ra.pending_head_for_session_locked(parent) + assert total == 1, "mirror + live gateway entry for one approval must dedupe to 1" + assert head["approval_id"] == aid + finally: + _clear_approval_state(parent) + + +def test_polling_endpoint_reports_aggregate_count(): + """#5: _handle_approval_pending must report 2 for parent-1 + child-1.""" + from urllib.parse import urlparse + import io + from api import routes as r + + parent = "test-child-parent-poll" + child = "test-child-poll" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + r.submit_pending( + parent, + {"command": "parentcmd", "pattern_key": "pp", "pattern_keys": ["pp"], "description": "pd"}, + ) + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + handler = type("H", (), { + "wfile": io.BytesIO(), + "send_response": lambda self, s: None, + "send_header": lambda self, k, v: None, + "end_headers": lambda self: None, + })() + r._handle_approval_pending(handler, urlparse(f"/api/approval/pending?session_id={parent}")) + import json as _json + body = _json.loads(handler.wfile.getvalue().decode("utf-8")) + assert body["pending_count"] == 2 + assert body["pending"]["command"] == "parentcmd" + finally: + _clear_approval_state(parent, child_key) + + +def test_sse_initial_snapshot_reports_aggregate_count(): + """#5: SSE initial snapshot must include child approvals with count 2.""" + from api import routes as r + from api import route_approvals as ra + + parent = "test-child-parent-sseinit" + child = "test-child-sseinit" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + r.submit_pending( + parent, + {"command": "parentcmd", "pattern_key": "pp", "pattern_keys": ["pp"], "description": "pd"}, + ) + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + # The stream handler's initial snapshot uses the same aggregate + # projection as the polling endpoint. + with r._lock: + r.reconcile_gateway_pending_mirror_locked(parent) + initial_pending, initial_count = ra.pending_head_for_session_locked(parent) + assert initial_count == 2 + assert initial_pending["command"] == "parentcmd" + finally: + _clear_approval_state(parent, child_key) + + +def test_parent_sse_subscriber_receives_child_enqueue(): + """#6: a parent SSE subscriber must get a push when a child approval lands.""" + from api import routes as r + from api import route_approvals as ra + + parent = "test-child-parent-sse-enqueue" + child = "test-child-sse-enqueue" + child_key = f"subagent:{child}" + _clear_approval_state(parent, child_key) + _seed_child_parent(child, parent) + try: + q = ra._approval_sse_subscribe(parent) + try: + r.submit_pending( + child_key, + {"command": "childcmd", "pattern_key": "cp", "pattern_keys": ["cp"], "description": "cd"}, + ) + payload = q.get(timeout=2) + assert payload["pending_count"] == 1 + assert payload["pending"]["command"] == "childcmd" + finally: + ra._approval_sse_unsubscribe(parent, q) + finally: + _clear_approval_state(parent, child_key) diff --git a/tests/test_pr1350_sse_atomic_subscribe.py b/tests/test_pr1350_sse_atomic_subscribe.py index fad3ea299a7..3db025e32ca 100644 --- a/tests/test_pr1350_sse_atomic_subscribe.py +++ b/tests/test_pr1350_sse_atomic_subscribe.py @@ -66,11 +66,13 @@ def _handler_body() -> str: def test_snapshot_taken_under_lock(): - """The initial _pending snapshot must be guarded by `with _lock:`.""" + """The initial snapshot must be guarded by `with _lock:`.""" lock_body = _extract_lock_block(_handler_body()) assert lock_body, "_handle_approval_sse_stream must contain a `with _lock:` block" - assert "_pending.get(sid)" in lock_body, \ - "Initial snapshot of _pending must be read inside the `with _lock:` block" + assert "pending_head_for_session_locked(sid)" in lock_body, ( + "Initial aggregate snapshot (own + delegated-child queues) must be " + "read inside the `with _lock:` block" + ) def test_subscriber_registered_inside_lock(): @@ -89,10 +91,10 @@ def test_subscribe_before_snapshot_in_lock(): assert lock_body, "Handler must contain a `with _lock:` block" sub_idx = lock_body.find("_approval_sse_subscribers") - snap_idx = lock_body.find("_pending.get(sid)") + snap_idx = lock_body.find("pending_head_for_session_locked(sid)") assert sub_idx != -1, "Subscriber registration must be inside the lock" - assert snap_idx != -1, "Snapshot read must be inside the lock" + assert snap_idx != -1, "Aggregate snapshot read must be inside the lock" assert sub_idx < snap_idx, ( "Subscriber registration must come BEFORE the snapshot read inside the lock. " "Otherwise an approval arriving between subscribe and snapshot is silently dropped."