diff --git a/gateway/browser_control_artifacts.py b/gateway/browser_control_artifacts.py index 7b31875d07..732eb06c53 100644 --- a/gateway/browser_control_artifacts.py +++ b/gateway/browser_control_artifacts.py @@ -1,27 +1,9 @@ -"""One-shot artifact transport for browser control (Gateway side). - -Transport-neutral store core for bounded browser-control artifacts -(screenshots, PDFs, uploads). It knows nothing about aiohttp: the routes in -:mod:`gateway.platforms.api_server` authenticate callers and rate-limit, then -hand bytes here. The controller WebSocket is a command channel, not a file -pipe — frames carry only ``artifact_id`` strings and the bytes live on disk -under a controlled root for a short TTL. - -Contract (tests/gateway/test_browser_control_artifacts.py): - -- Server-minted ``[0-9a-f]{32}`` ids resolved strictly inside the root; - client filenames are metadata only, never paths. -- Exact size and MIME caps enforced before any disk write. -- SHA-256 recorded in the receipt and re-verified by ``load``/``validate``. -- ``load`` requires the exact scope key and consumes atomically; - ``validate`` checks existence/TTL/scope without consuming. -- No overwrite: an id collision is retried with a fresh id. -- ``prune_expired`` removes expired entries; the API server sweeps on demand. - -Thread-safety: the in-memory index is lock-guarded; files are written to a -temp name and atomically renamed so a concurrent ``load`` never sees a -partial artifact. -""" +"""One-shot artifact transport for browser control (Gateway side); :mod:`gateway.platforms.api_server` +authenticates and rate-limits, then hands bytes here. Controller frames carry only server-minted +``[0-9a-f]{32}`` ids (client filenames are metadata, never paths); bytes live under a controlled root +for a short TTL. Size/MIME caps apply before any write; SHA-256 is re-verified on read; ``load`` needs +the exact scope key and consumes atomically. The index is lock-guarded and files are temp-written then +renamed so readers never see partials.""" from __future__ import annotations @@ -43,10 +25,8 @@ DEFAULT_ARTIFACT_TTL_SECONDS = 300.0 DEFAULT_MAX_ARTIFACT_BYTES = 10 * 1024 * 1024 #: Exact allowlist — parameterized/unknown variants are rejected. DEFAULT_ALLOWED_MIME_TYPES = frozenset({ - "application/json", "application/pdf", "image/gif", "image/jpeg", - "image/png", "image/webp", "text/plain", + "application/json", "application/pdf", "image/gif", "image/jpeg", "image/png", "image/webp", "text/plain", }) -_ARTIFACT_ID_HEX = 32 _ARTIFACT_ID_RE = re.compile(r"^[0-9a-f]{32}$") _TEMP_SUFFIX = ".tmp" @@ -86,7 +66,6 @@ class ArtifactTraversal(ArtifactError): @dataclass(frozen=True) class ArtifactReceipt: """Provenance record returned to the caller of ``store``.""" - artifact_id: str sha256: str size_bytes: int @@ -99,35 +78,22 @@ class ArtifactReceipt: def to_dict(self, *, download_path: str = "") -> dict[str, Any]: """Serialize to the wire receipt (never contains file paths).""" - receipt = { - "artifact_id": self.artifact_id, - "sha256": self.sha256, - "size_bytes": self.size_bytes, - "content_type": self.content_type, - "filename": self.filename, - "created_at": self.created_at, - "expires_at": self.expires_at, - "ttl_seconds": self.ttl_seconds, - "one_shot": True, + return { + "artifact_id": self.artifact_id, "sha256": self.sha256, "size_bytes": self.size_bytes, + "content_type": self.content_type, "filename": self.filename, "created_at": self.created_at, + "expires_at": self.expires_at, "ttl_seconds": self.ttl_seconds, "one_shot": True, + **({"download_path": download_path} if download_path else {}), } - if download_path: - receipt["download_path"] = download_path - return receipt def artifact_scope_key(scope: Any) -> str: """Derive the stable scope key an artifact is bound to. - Only principal (mandatory) + transport family participate. ``session_id`` - is deliberately EXCLUDED: HTTP artifact routes authenticate by API key and - can't resolve a session, while broker dispatch always carries one — hashing - it would make upload and dispatch never compose. Ids are unguessable and - downloads one-shot, so cross-session reuse within one principal is by - design. Capabilities/optional ids are excluded so a reconnect keeps its - artifacts. - """ - principal = "" - family = "" + Only principal (mandatory) + transport family participate. ``session_id`` is deliberately EXCLUDED: HTTP + artifact routes authenticate by API key and can't resolve a session while broker dispatch always carries + one, so hashing it would make upload and dispatch never compose (ids are unguessable and downloads + one-shot). Capabilities/optional ids are excluded so a reconnect keeps its artifacts.""" + principal = family = "" try: principal = str(getattr(scope, "principal_id", "") or "") family = str(getattr(scope, "transport_family", "") or "") @@ -136,12 +102,7 @@ def artifact_scope_key(scope: Any) -> str: if not principal: # Fail closed: only an authenticated principal may mint artifacts. raise ArtifactError("artifact scope must carry a resolved principal") - material = f"{principal}\x00{family}".encode("utf-8") - return hashlib.sha256(material).hexdigest() - - -def _sha256(data: bytes) -> str: - return hashlib.sha256(data).hexdigest() + return hashlib.sha256(f"{principal}\x00{family}".encode("utf-8")).hexdigest() @dataclass @@ -152,14 +113,8 @@ class _ArtifactEntry: class ArtifactStore: """Thread-safe, TTL-bounded, scope-bound one-shot artifact store.""" - - def __init__( - self, root: Path, *, - ttl_seconds: float = DEFAULT_ARTIFACT_TTL_SECONDS, - max_bytes: int = DEFAULT_MAX_ARTIFACT_BYTES, - allowed_mime_types: frozenset = DEFAULT_ALLOWED_MIME_TYPES, - clock: Optional[Callable[[], float]] = None, - ) -> None: + def __init__(self, root: Path, *, ttl_seconds: float = DEFAULT_ARTIFACT_TTL_SECONDS, max_bytes: int = DEFAULT_MAX_ARTIFACT_BYTES, + allowed_mime_types: frozenset = DEFAULT_ALLOWED_MIME_TYPES, clock: Optional[Callable[[], float]] = None) -> None: self._root = Path(root) self._root.mkdir(parents=True, exist_ok=True) self._ttl_seconds = max(1.0, float(ttl_seconds)) @@ -173,11 +128,7 @@ class ArtifactStore: self._sweep_orphan_files() def _sweep_orphan_files(self) -> None: - """Delete on-disk files with no live index entry. - - Only names matching the minted 32-hex id shape or the ``*.tmp`` - staging suffix are touched; anything else in the directory is left. - """ + """Delete on-disk files with no live index entry; only minted-id-shaped and ``*.tmp`` names are touched.""" try: candidates = list(self._root.iterdir()) except OSError: @@ -185,18 +136,10 @@ class ArtifactStore: with self._lock: live = set(self._entries) for path in candidates: - name = path.name - is_temp = name.endswith(_TEMP_SUFFIX) - if not path.is_file() or not (is_temp or _ARTIFACT_ID_RE.fullmatch(name)): - continue - if not is_temp and name in live: - continue - with contextlib.suppress(OSError): - path.unlink(missing_ok=True) - - # ------------------------------------------------------------------ - # Public API - # ------------------------------------------------------------------ + orphan = path.name.endswith(_TEMP_SUFFIX) or (_ARTIFACT_ID_RE.fullmatch(path.name) and path.name not in live) + if path.is_file() and orphan: + with contextlib.suppress(OSError): + path.unlink(missing_ok=True) @property def root(self) -> Path: @@ -212,11 +155,7 @@ class ArtifactStore: return self._allowed_mime_types def store(self, data: bytes, *, filename: str, content_type: str, scope: Any) -> ArtifactReceipt: - """Validate and store one artifact, returning its provenance receipt. - - Raises :class:`ArtifactTooLarge` / :class:`ArtifactMimeRejected` - before any disk write; :class:`ArtifactError` on an unresolved scope. - """ + """Validate and store one artifact, returning its receipt; size/MIME rejections fire before any disk write.""" size = len(data) if size > self._max_bytes: raise ArtifactTooLarge(f"artifact is {size} bytes; cap is {self._max_bytes}") @@ -225,23 +164,19 @@ class ArtifactStore: raise ArtifactMimeRejected(f"content type {content_type!r} is outside the exact allowlist") scope_key = artifact_scope_key(scope) now = self._clock() - # Mint a fresh id; retry on an astronomically unlikely collision. while True: - artifact_id = secrets.token_hex(_ARTIFACT_ID_HEX // 2) + artifact_id = secrets.token_hex(16) target = self._artifact_path(artifact_id) with self._lock: - if artifact_id in self._entries or target.exists(): - continue - receipt = ArtifactReceipt( - artifact_id=artifact_id, sha256=_sha256(data), size_bytes=size, - content_type=normalized_type, filename=_bounded_filename(filename), - created_at=now, expires_at=now + self._ttl_seconds, - ttl_seconds=self._ttl_seconds, scope_key=scope_key, - ) - self._entries[artifact_id] = _ArtifactEntry(receipt=receipt, path=target) - break - + if artifact_id not in self._entries and not target.exists(): + receipt = ArtifactReceipt( + artifact_id=artifact_id, sha256=hashlib.sha256(data).hexdigest(), size_bytes=size, + content_type=normalized_type, filename=_bounded_filename(filename), created_at=now, + expires_at=now + self._ttl_seconds, ttl_seconds=self._ttl_seconds, scope_key=scope_key, + ) + self._entries[artifact_id] = _ArtifactEntry(receipt=receipt, path=target) + break # Temp + atomic rename so readers never observe a partial artifact. temp = target.with_name(f"{target.name}{_TEMP_SUFFIX}") try: @@ -259,48 +194,39 @@ class ArtifactStore: return receipt def validate(self, artifact_id: str, *, scope: Any) -> ArtifactReceipt: - """Return the receipt when the artifact is live for ``scope`` - (existence, TTL, scope) without consuming it; raises otherwise.""" + """Receipt when the artifact is live for ``scope`` (existence, TTL, scope), without consuming.""" return self._entry_for(artifact_id, scope=scope).receipt def load(self, artifact_id: str, *, scope: Any) -> tuple[bytes, ArtifactReceipt]: - """One-shot download: verify, read, checksum, then consume. - - A second ``load`` raises :class:`ArtifactNotFound`. A checksum - mismatch raises :class:`ArtifactChecksumMismatch` without consuming. - """ + """One-shot download: verify, read, checksum, then consume (a checksum mismatch does not consume).""" with self._lock: entry = self._entry_for(artifact_id, scope=scope) - path = entry.path - if not path.exists(): + if not entry.path.exists(): self._entries.pop(artifact_id, None) raise ArtifactNotFound(f"artifact {artifact_id!r} is gone") try: - data = path.read_bytes() + data = entry.path.read_bytes() except OSError as exc: raise ArtifactError(f"artifact read failed: {exc}") from exc - if _sha256(data) != entry.receipt.sha256: + if hashlib.sha256(data).hexdigest() != entry.receipt.sha256: raise ArtifactChecksumMismatch(f"artifact {artifact_id!r} failed SHA-256 validation") # Drop the index entry first so a concurrent load fails closed. self._entries.pop(artifact_id, None) try: - path.unlink(missing_ok=True) + entry.path.unlink(missing_ok=True) except OSError: logger.warning("artifact %s: file removal failed; TTL sweep will retry", artifact_id) return data, entry.receipt def prune_expired(self, now: Optional[float] = None) -> int: - """Delete every artifact past its TTL (and stale temp files); return - the count removed. Idempotent.""" + """Delete every artifact past its TTL (and stale temp files); return the count removed.""" now = self._clock() if now is None else float(now) with self._lock: removed = self._prune_expired_locked(now) for temp in self._root.glob(f"*{_TEMP_SUFFIX}"): - try: + with contextlib.suppress(OSError): if temp.stat().st_mtime <= now - self._ttl_seconds: temp.unlink(missing_ok=True) - except OSError: - continue return removed def count(self) -> int: @@ -308,9 +234,11 @@ class ArtifactStore: with self._lock: return len(self._entries) - # ------------------------------------------------------------------ - # Internals - # ------------------------------------------------------------------ + def _discard_locked(self, artifact_id: str, path: Path) -> None: + """Drop the index entry and best-effort unlink its file.""" + self._entries.pop(artifact_id, None) + with contextlib.suppress(OSError): + path.unlink(missing_ok=True) def _entry_for(self, artifact_id: str, *, scope: Any) -> _ArtifactEntry: path = self._artifact_path(artifact_id) @@ -326,23 +254,17 @@ class ArtifactStore: if entry is None: raise ArtifactNotFound(f"unknown artifact {artifact_id!r}") if entry.receipt.expires_at <= now: - self._entries.pop(artifact_id, None) - with contextlib.suppress(OSError): - path.unlink(missing_ok=True) + self._discard_locked(artifact_id, path) raise ArtifactExpired(f"artifact {artifact_id!r} expired") if entry.receipt.scope_key != scope_key: raise ArtifactScopeMismatch(f"artifact {artifact_id!r} is bound to a different scope") return entry def _prune_expired_locked(self, now: float) -> int: - removed = 0 - for artifact_id, entry in list(self._entries.items()): - if entry.receipt.expires_at <= now: - self._entries.pop(artifact_id, None) - with contextlib.suppress(OSError): - entry.path.unlink(missing_ok=True) - removed += 1 - return removed + expired = [(aid, e.path) for aid, e in self._entries.items() if e.receipt.expires_at <= now] + for artifact_id, path in expired: + self._discard_locked(artifact_id, path) + return len(expired) def _artifact_path(self, artifact_id: str) -> Path: """Resolve a minted id strictly inside the controlled root.""" @@ -360,32 +282,18 @@ class ArtifactStore: def _normalize_content_type(value: str) -> str: """Return the canonical MIME type, or ``""`` for malformed input.""" - if not isinstance(value, str): - return "" - return value.strip().split(";", 1)[0].strip().lower() + return value.strip().split(";", 1)[0].strip().lower() if isinstance(value, str) else "" def _bounded_filename(value: str, limit: int = 160) -> str: """Sanitize a display-only filename; never used as a filesystem path.""" - if not isinstance(value, str): - return "" - cleaned = value.strip().replace("\\", "_").replace("/", "_") - cleaned = "".join(character for character in cleaned if ord(character) >= 32) - return cleaned[:limit] - - -# ---------------------------------------------------------------------- -# Rate limiting (route-level, per principal) -# ---------------------------------------------------------------------- + cleaned = value.strip().replace("\\", "_").replace("/", "_") if isinstance(value, str) else "" + return "".join(character for character in cleaned if ord(character) >= 32)[:limit] class ArtifactRateLimiter: """Sliding-window per-key limiter; the API server keys it by principal.""" - - def __init__( - self, *, window_seconds: float = 60.0, max_requests: int = 30, - clock: Optional[Callable[[], float]] = None, - ) -> None: + def __init__(self, *, window_seconds: float = 60.0, max_requests: int = 30, clock: Optional[Callable[[], float]] = None) -> None: self._window_seconds = max(1.0, float(window_seconds)) self._max_requests = max(1, int(max_requests)) self._clock = clock if clock is not None else time.time @@ -397,9 +305,8 @@ class ArtifactRateLimiter: if not isinstance(key, str) or not key: return False now = self._clock() - window_start = now - self._window_seconds with self._lock: - hits = [hit for hit in self._hits.get(key, []) if hit > window_start] + hits = [hit for hit in self._hits.get(key, []) if hit > now - self._window_seconds] allowed = len(hits) < self._max_requests if allowed: hits.append(now) diff --git a/gateway/browser_control_broker.py b/gateway/browser_control_broker.py index 5319873a75..58e2d7c672 100644 --- a/gateway/browser_control_broker.py +++ b/gateway/browser_control_broker.py @@ -1,34 +1,10 @@ -"""Transport-neutral browser-control broker core. - -Binds an identity-scoped *controller* (the party driving a browser) to -*callers* (agents talking to it over any transport) without knowing HTTP, -WebSocket, or any wire format. The browser is a stateful single-owner -resource while the agent side is multi-tenant and multi-transport, so every -rule here makes a class of violation structurally impossible: - -- Registration tickets are short-lived, single-use, identity-bound, and - ``secrets``-random; ``consume_ticket`` exchanges one exactly once. The - ticket is the only cross-transport credential minted here; transports - decide how to carry it. -- ``select`` matches on *every* stable identity field (principal, profile, - session, controller id, browser profile id, transport family) plus the - controller's currently negotiated capability set; partial matches yield - ``None``. A same-identity reconnect renegotiates capabilities in place. -- One pending command per command id; ``complete`` is single-shot (late - completion after cancel/detach returns ``False``). -- ``cancel`` aborts only the command matching scope + tool_call_id. -- ``detach`` fails closed: pending work of that scope raises - :class:`ControllerCancelled` instead of racing a late ``complete``. -- ``disconnect`` marks a transport offline without cancelling running work; - re-attaching the same identity refreshes the callback and may still - deliver the original result. Detach, cancel, identity replacement, and - timeout remain terminal. - -Thread-safety: all state transitions happen under one reentrant lock; the -send callback is invoked *outside* it so a controller may synchronously -``complete`` from inside its own send, and waiters park on per-command -events, never on the broker lock. -""" +"""Transport-neutral browser-control broker core: binds an identity-scoped *controller* +(the party driving a browser) to *callers* on any transport. Tickets are short-lived, +single-use, identity-bound and consumed exactly once; ``select`` matches every stable +identity field plus the current capability set; ``complete`` is single-shot; ``detach`` +fails pending work closed while ``disconnect`` only marks the transport offline. State +changes happen under one RLock; the send callback runs *outside* it so a controller may +``complete`` from inside its own send.""" from __future__ import annotations @@ -48,39 +24,18 @@ DEFAULT_TICKET_TTL = 30.0 DEFAULT_COMMAND_TIMEOUT = 30.0 #: Maximum cancel frames retained while a same-identity controller is offline. MAX_DEFERRED_CANCELS = 512 - -#: Current wire protocol version. Registration requires this exact integer; -#: booleans are rejected even though ``bool`` subclasses ``int``. +#: Current wire protocol version; registration requires this exact int (bools rejected). BROWSER_CONTROL_PROTOCOL_VERSION = 1 -#: Exact controller capability allowlist shared by every transport. Raw CDP, -#: script evaluation, console access, and other privileged surfaces stay out. -BROWSER_CONTROL_CAPABILITIES = frozenset( - { - "controller.noop", - "browser_back", - "browser_click", - "browser_navigate", - "browser_press", - "browser_screenshot", - "browser_scroll", - "browser_snapshot", - "browser_tab_activate", - "browser_tabs", - "browser_type", - } -) - -#: Privileged capabilities (JS eval, raw CDP): fail-closed unless Developer -#: Mode (``browser.extension_control.developer_mode``) is on AND the -#: controller explicitly negotiated them. +#: Exact controller capability allowlist shared by every transport (raw CDP/eval/console stay out). +BROWSER_CONTROL_CAPABILITIES = frozenset({ + "controller.noop", "browser_back", "browser_click", "browser_navigate", "browser_press", "browser_screenshot", + "browser_scroll", "browser_snapshot", "browser_tab_activate", "browser_tabs", "browser_type", +}) +#: Privileged capabilities: fail-closed unless Developer Mode is on AND explicitly negotiated. BROWSER_CONTROL_DEVELOPER_CAPABILITIES = frozenset({"browser_cdp", "browser_evaluate"}) - -#: Artifact transport capabilities. Non-developer because payloads never ride -#: in controller frames: only a store-validated ``artifact_id`` is dispatched. -BROWSER_CONTROL_ARTIFACT_CAPABILITIES = frozenset( - {"browser_artifact_download", "browser_artifact_upload"} -) +#: Artifact transport capabilities; non-developer because only a store-validated ``artifact_id`` travels. +BROWSER_CONTROL_ARTIFACT_CAPABILITIES = frozenset({"browser_artifact_download", "browser_artifact_upload"}) #: Wire method names for controller frames; transports carry them verbatim. FRAME_COMMAND = "browser.controller.command" @@ -96,22 +51,14 @@ def _extension_control_flag(config: Optional[dict], key: str) -> bool: """Read ``browser.extension_control.`` as a literal ``True`` (default off).""" if config is None: try: - # Hot path (every browser tool call / check_fn): the read-only - # loader skips load_config()'s deepcopy; we never mutate. + # Hot path (every browser tool call): the read-only loader skips load_config()'s deepcopy. from hermes_cli.config import load_config_readonly - config = load_config_readonly() except Exception: return False - if not isinstance(config, dict): - return False - browser = config.get("browser") - if not isinstance(browser, dict): - return False - extension_control = browser.get("extension_control") - if not isinstance(extension_control, dict): - return False - return extension_control.get(key, False) is True + browser = config.get("browser") if isinstance(config, dict) else None + extension_control = browser.get("extension_control") if isinstance(browser, dict) else None + return isinstance(extension_control, dict) and extension_control.get(key, False) is True def browser_control_developer_mode(config: Optional[dict] = None) -> bool: @@ -124,25 +71,13 @@ def browser_control_enabled(config: Optional[dict] = None) -> bool: return _extension_control_flag(config, "enabled") -def filter_browser_control_capabilities( - value: Any, - *, - developer_mode: Optional[bool] = None, -) -> frozenset: - """Return the permitted subset of a JSON/RPC capability list. - - Non-list input has no capabilities; unknown/non-string entries are - dropped (registration rejects an empty returned set). Developer - capabilities pass only when Developer Mode is on (passed in, else read - from live config). - """ +def filter_browser_control_capabilities(value: Any, *, developer_mode: Optional[bool] = None) -> frozenset: + """Permitted subset of a capability list (non-list -> empty); developer caps only in Developer Mode.""" if not isinstance(value, list): return frozenset() allowed = BROWSER_CONTROL_CAPABILITIES | BROWSER_CONTROL_ARTIFACT_CAPABILITIES - if developer_mode is None: - developer_mode = browser_control_developer_mode() - if developer_mode is True: - allowed = allowed | BROWSER_CONTROL_DEVELOPER_CAPABILITIES + if (browser_control_developer_mode() if developer_mode is None else developer_mode) is True: + allowed |= BROWSER_CONTROL_DEVELOPER_CAPABILITIES return frozenset(c for c in value if isinstance(c, str) and c in allowed) @@ -173,7 +108,6 @@ class ControllerRejected(BrowserControlError): @dataclass(frozen=True) class ControllerScope: """Exact controller identity plus capability set; equality is over all fields.""" - principal_id: Optional[str] = None profile_id: Optional[str] = None session_id: Optional[str] = None @@ -183,26 +117,17 @@ class ControllerScope: capabilities: frozenset = frozenset() -def _scope_identity(scope: ControllerScope) -> tuple: - """Stable identity fields, excluding negotiated capabilities.""" - return ( - scope.principal_id, - scope.profile_id, - scope.session_id, - scope.controller_id, - scope.browser_profile_id, - scope.transport_family, - ) +#: Stable identity fields; negotiated ``capabilities`` are deliberately excluded. +_IDENTITY_FIELDS = ("principal_id", "profile_id", "session_id", "controller_id", "browser_profile_id", "transport_family") def _same_scope_identity(first: ControllerScope, second: ControllerScope) -> bool: - return _scope_identity(first) == _scope_identity(second) + return all(getattr(first, name) == getattr(second, name) for name in _IDENTITY_FIELDS) @dataclass(frozen=True) class Ticket: """Opaque, single-use registration credential.""" - value: str expires_at: float @@ -221,9 +146,7 @@ class _Controller: owner: Any = None connected: bool = True deferred_cancels: list[dict] = field(default_factory=list) - # Serializes command/cancel writes with detach or replacement. Broker - # state is never held while waiting on it, so a transport callback may - # synchronously complete() without deadlocking. + # Serializes command/cancel writes with detach or replacement; never held with broker state. send_lock: threading.Lock = field(default_factory=threading.Lock) @@ -239,20 +162,14 @@ class _PendingCommand: result: Any = None +def _cancel_frame(pending: _PendingCommand) -> dict: + return {"method": FRAME_CANCEL, "params": {"command_id": pending.command_id, "tool_call_id": pending.tool_call_id}} + + class BrowserControlBroker: - """Thread-safe broker core binding controllers to callers. - - ``clock`` is injectable (default ``time.monotonic``) so tests pin expiry. - """ - - def __init__( - self, - *, - ticket_ttl: float = DEFAULT_TICKET_TTL, - command_timeout: float = DEFAULT_COMMAND_TIMEOUT, - clock: Optional[Callable[[], float]] = None, - developer_mode: Optional[bool] = None, - ) -> None: + """Thread-safe broker core; ``clock`` is injectable (default ``time.monotonic``).""" + def __init__(self, *, ticket_ttl: float = DEFAULT_TICKET_TTL, command_timeout: float = DEFAULT_COMMAND_TIMEOUT, + clock: Optional[Callable[[], float]] = None, developer_mode: Optional[bool] = None) -> None: self._ticket_ttl = ticket_ttl self._command_timeout = command_timeout self._clock = clock if clock is not None else time.monotonic @@ -260,19 +177,15 @@ class BrowserControlBroker: self._tickets: Dict[str, _TicketRecord] = {} self._controllers: Dict[ControllerScope, _Controller] = {} self._pending: Dict[str, _PendingCommand] = {} - # None defers to live config on every selection so flipping - # developer_mode off REVOKES raw CDP/eval from attached controllers - # without restart; an explicit bool pins the gate (tests, multi-tenant). - self._developer_mode_pinned: Optional[bool] = ( - None if developer_mode is None else developer_mode is True - ) + # None defers to live config on every selection (so flipping developer_mode off REVOKES + # raw CDP/eval from attached controllers without restart); a bool pins the gate. + self._developer_mode_pinned: Optional[bool] = None if developer_mode is None else developer_mode is True # Artifact stores keyed by profile id; ``None`` is the default slot. - # Per-profile stores keep multiplex profile A from pinning profile B - # to A's physical root. self._artifact_stores: Dict[Optional[str], Any] = {} - def _developer_mode_now(self) -> bool: - """Current Developer Mode authority (live config unless pinned).""" + @property + def developer_mode(self) -> bool: + """Whether privileged capabilities may be selected/dispatched (live config unless pinned).""" if self._developer_mode_pinned is not None: return self._developer_mode_pinned try: @@ -280,55 +193,29 @@ class BrowserControlBroker: except Exception: return False - @property - def developer_mode(self) -> bool: - """Whether privileged capabilities may be selected/dispatched.""" - return self._developer_mode_now() - - def attach_artifact_store( - self, store: Any, *, profile_id: Optional[str] = None - ) -> None: - """Attach a store exposing ``validate(artifact_id, *, scope) -> receipt`` - (raising the artifacts module's :class:`ArtifactError` subclasses). - - ``profile_id`` scopes the store to one profile on multiplex hosts; - ``None`` registers the default store. ``store=None`` clears the slot. - Artifact actions without a resolvable store fail closed. - """ + def attach_artifact_store(self, store: Any, *, profile_id: Optional[str] = None) -> None: + """Attach a store exposing ``validate(artifact_id, *, scope) -> receipt`` for one profile + (``None`` = default slot); ``store=None`` clears the slot. Artifact actions fail closed without one.""" if store is None: self._artifact_stores.pop(profile_id, None) - return - self._artifact_stores[profile_id] = store + else: + self._artifact_stores[profile_id] = store def _artifact_store_for_scope(self, scope: "ControllerScope") -> Any: - """Exact profile-scoped store, else the default (``None``) slot so - single-profile hosts keep the historical one-store behaviour.""" - profile = getattr(scope, "profile_id", None) or None - store = self._artifact_stores.get(profile) + store = self._artifact_stores.get(getattr(scope, "profile_id", None) or None) return store if store is not None else self._artifact_stores.get(None) - # ------------------------------------------------------------------ - # Registration tickets - # ------------------------------------------------------------------ - def mint_ticket(self, scope: ControllerScope) -> Ticket: """Mint a short-lived, single-use ticket bound to ``scope``.""" now = self._clock() with self._lock: - for stale in [v for v, rec in self._tickets.items() if rec.expires_at <= now]: - del self._tickets[stale] + self._tickets = {v: rec for v, rec in self._tickets.items() if rec.expires_at > now} value = secrets.token_urlsafe(32) - record = _TicketRecord(scope=scope, expires_at=now + self._ticket_ttl) - self._tickets[value] = record + self._tickets[value] = record = _TicketRecord(scope=scope, expires_at=now + self._ticket_ttl) return Ticket(value=value, expires_at=record.expires_at) def consume_ticket(self, value: str) -> ControllerScope: - """Exchange a ticket for its scope exactly once. - - Raises :class:`ControllerTicketInvalid` for unknown, already-consumed, - or expired tickets; expiry is checked against the live clock at - consume time. - """ + """Exchange a ticket for its scope exactly once; unknown/consumed/expired -> ControllerTicketInvalid.""" now = self._clock() with self._lock: record = self._tickets.get(value) @@ -341,52 +228,26 @@ class BrowserControlBroker: record.consumed = True return record.scope - # ------------------------------------------------------------------ - # Controller registration / selection - # ------------------------------------------------------------------ - def _controller_for_identity_locked(self, scope: ControllerScope) -> Optional[_Controller]: """Attached controller sharing ``scope``'s stable identity (any capabilities).""" - return next( - (c for c in self._controllers.values() if _same_scope_identity(c.scope, scope)), - None, - ) + return next((c for c in self._controllers.values() if _same_scope_identity(c.scope, scope)), None) - def _lane_scopes_locked(self, session_id: str, principal_id: str, family: str) -> list[ControllerScope]: - """Attached scopes bound to one session lane (session + principal + transport).""" - return [ - scope - for scope in self._controllers - if scope.session_id == session_id - and scope.principal_id == principal_id - and scope.transport_family == family - ] + def _live_controller(self, scope: ControllerScope) -> Optional[_Controller]: + with self._lock: + controller = self._controller_for_identity_locked(scope) + return controller if controller is not None and controller.connected else None - def attach( - self, - scope: ControllerScope, - send: Callable[[dict], None], - *, - owner: Any = None, - ) -> None: - """Attach or refresh the controller for one stable identity. - - A same-identity reconnect refreshes the send callback and negotiated - capabilities without cancelling pending work. A different controller - or browser profile in the same authenticated session lane - hard-replaces the previous identity. - """ + def attach(self, scope: ControllerScope, send: Callable[[dict], None], *, owner: Any = None) -> None: + """Attach or refresh the controller for one stable identity: a same-identity reconnect refreshes send + and capabilities without cancelling pending work; a different controller/browser profile in the same + authenticated session lane hard-replaces it.""" while True: with self._lock: existing = self._controller_for_identity_locked(scope) lane_scopes = [ - candidate - for candidate in self._controllers - if candidate.principal_id == scope.principal_id - and candidate.profile_id == scope.profile_id - and candidate.session_id == scope.session_id - and candidate.transport_family == scope.transport_family - and not _same_scope_identity(candidate, scope) + c for c in self._controllers if not _same_scope_identity(c, scope) + and (c.principal_id, c.profile_id, c.session_id, c.transport_family) + == (scope.principal_id, scope.profile_id, scope.session_id, scope.transport_family) ] if existing is None and not lane_scopes: self._controllers[scope] = _Controller(scope=scope, send=send, owner=owner) @@ -404,69 +265,41 @@ class BrowserControlBroker: if self._controllers.get(existing.scope) is not existing: continue self._controllers.pop(existing.scope, None) - existing.scope = scope - existing.send = send - existing.owner = owner - existing.connected = False - for pending in self._pending.values(): - if _same_scope_identity(pending.scope, scope): - pending.scope = scope - deferred = list(existing.deferred_cancels) - existing.deferred_cancels.clear() + existing.scope, existing.send, existing.owner, existing.connected = scope, send, owner, False + for pending in self._pending_for_scope_locked(scope): + pending.scope = scope + deferred, existing.deferred_cancels = existing.deferred_cancels, [] self._controllers[scope] = existing - unsent: list[dict] = [] for index, frame in enumerate(deferred): try: send(frame) + continue except Exception: logger.exception("failed to flush deferred browser-controller cancel") - unsent = deferred[index:] - break - if unsent: with self._lock: if self._controllers.get(scope) is existing: - existing.deferred_cancels = unsent[-MAX_DEFERRED_CANCELS:] - raise ConnectionError( - "browser controller reconnect could not flush deferred cancels" - ) + existing.deferred_cancels = deferred[index:][-MAX_DEFERRED_CANCELS:] + raise ConnectionError("browser controller reconnect could not flush deferred cancels") with self._lock: if self._controllers.get(scope) is existing: existing.connected = True return def select(self, scope: ControllerScope, capability: str) -> Optional[_Controller]: - """Return the connected controller matching identity and capability. - - The caller's capabilities are not authoritative: identity is matched, - then the controller's *current* negotiated set is checked. Offline - controllers never accept new dispatches. Developer capabilities are - additionally gated on the LIVE Developer Mode flag (unless pinned). - """ - if capability in BROWSER_CONTROL_DEVELOPER_CAPABILITIES and not self._developer_mode_now(): + """Connected controller matching identity whose *current* negotiated set holds ``capability`` (the + caller's set is not authoritative); developer capabilities are also gated on LIVE Developer Mode.""" + if capability in BROWSER_CONTROL_DEVELOPER_CAPABILITIES and not self.developer_mode: return None - with self._lock: - controller = self._controller_for_identity_locked(scope) - if controller is None or not controller.connected: - return None - return controller if capability in controller.scope.capabilities else None + controller = self._live_controller(scope) + return controller if controller is not None and capability in controller.scope.capabilities else None def is_owner(self, scope: ControllerScope, owner: Any) -> bool: - """Whether ``owner`` is the exact live transport for ``scope``. + """Whether ``owner`` is the exact live transport for ``scope`` (capability-independent).""" + controller = self._live_controller(scope) + return controller is not None and controller.owner is owner - Independent of capabilities, so a least-privilege controller need not - request ``controller.noop`` merely to heartbeat or complete an action. - """ - with self._lock: - controller = self._controller_for_identity_locked(scope) - return controller is not None and controller.connected and controller.owner is owner - - def disconnect( - self, - scope: ControllerScope, - *, - owner: Any = _OWNER_UNSET, - ) -> bool: + def disconnect(self, scope: ControllerScope, *, owner: Any = _OWNER_UNSET) -> bool: """Mark one exact controller transport offline without cancelling work.""" with self._lock: controller = self._controller_for_identity_locked(scope) @@ -474,37 +307,21 @@ class BrowserControlBroker: return False with controller.send_lock: with self._lock: - if self._controllers.get(controller.scope) is not controller: + owned = owner is _OWNER_UNSET or controller.owner is owner + if self._controllers.get(controller.scope) is not controller or not owned: return False - if owner is not _OWNER_UNSET and controller.owner is not owner: - return False - controller.connected = False - controller.owner = None + controller.connected, controller.owner = False, None return True - def detach( - self, - scope: ControllerScope, - *, - owner: Any = _OWNER_UNSET, - notify_controller: bool = True, - ) -> None: - """Remove the controller for ``scope`` and fail its pending work closed. - - Pending commands resolve cancelled (dispatchers raise - :class:`ControllerCancelled`); a late ``complete`` returns ``False``. - """ + def detach(self, scope: ControllerScope, *, owner: Any = _OWNER_UNSET, notify_controller: bool = True) -> None: + """Remove the controller for ``scope`` and fail its pending work closed (ControllerCancelled).""" with self._lock: controller = self._controllers.get(scope) - if controller is None: - return - if owner is not _OWNER_UNSET and controller.owner != owner: + if controller is None or (owner is not _OWNER_UNSET and controller.owner != owner): return with controller.send_lock: with self._lock: - if self._controllers.get(scope) is not controller: - return - if owner is not _OWNER_UNSET and controller.owner != owner: + if self._controllers.get(scope) is not controller or (owner is not _OWNER_UNSET and controller.owner != owner): return self._controllers.pop(scope, None) pendings = self._pending_for_scope_locked(scope) @@ -515,65 +332,31 @@ class BrowserControlBroker: if notify_controller: self._emit_cancel_frames(controller, pendings) - # ------------------------------------------------------------------ - # Command lifecycle - # ------------------------------------------------------------------ - def dispatch( - self, - scope: ControllerScope, - *, - action: str, - arguments: Optional[dict] = None, - tool_call_id: Optional[str] = None, + self, scope: ControllerScope, *, action: str, arguments: Optional[dict] = None, tool_call_id: Optional[str] = None, ) -> Any: - """Send one controller command and block for its completion. - - Raises :class:`ControllerUnavailable` (no exact match), - :class:`ControllerCancelled`, :class:`ControllerTimeout`, or - :class:`ControllerRejected` (``ok=False``). Artifact actions also - require an attached store and an approved ``artifact_id`` argument; - only the id travels in the frame, never the payload. - """ + """Send one controller command and block for completion; raises ControllerUnavailable/Cancelled/Timeout/ + Rejected. Artifact actions also need an attached store and an approved ``artifact_id`` (only the id travels).""" controller = self.select(scope, action) if controller is None: - raise ControllerUnavailable( - f"no controller for scope {scope!r} with capability {action!r}" - ) - + raise ControllerUnavailable(f"no controller for scope {scope!r} with capability {action!r}") arguments = dict(arguments or {}) if action in BROWSER_CONTROL_ARTIFACT_CAPABILITIES: self._validate_artifact_reference(scope, action, arguments) - command_id = secrets.token_hex(16) - frame = { - "method": FRAME_COMMAND, - "params": { - "command_id": command_id, - "action": action, - "arguments": arguments, - "controller_id": scope.controller_id, - "browser_profile_id": scope.browser_profile_id, - "tool_call_id": tool_call_id, - }, - } - pending = _PendingCommand( - scope=controller.scope, - command_id=command_id, - tool_call_id=tool_call_id, - ) + frame = {"method": FRAME_COMMAND, "params": { + "command_id": command_id, "action": action, "arguments": arguments, "controller_id": scope.controller_id, + "browser_profile_id": scope.browser_profile_id, "tool_call_id": tool_call_id, + }} + pending = _PendingCommand(scope=controller.scope, command_id=command_id, tool_call_id=tool_call_id) with controller.send_lock: with self._lock: # select() ran outside the send lock; revalidate the live # controller so disconnect/replacement can't strand a command. - attached = self._controller_for_identity_locked(scope) - if attached is not controller or not controller.connected: - raise ControllerUnavailable( - f"controller for scope {scope!r} detached before dispatch" - ) + if self._controller_for_identity_locked(scope) is not controller or not controller.connected: + raise ControllerUnavailable(f"controller for scope {scope!r} detached before dispatch") pending.scope = controller.scope self._pending[command_id] = pending - try: controller.send(frame) except Exception: @@ -583,14 +366,13 @@ class BrowserControlBroker: raise if not pending.event.wait(timeout=self._command_timeout): - timed_out = False with self._lock: # Event.wait() may return False at the exact boundary where a # completion already won and removed the pending command. - if not pending.done and self._pending.get(command_id) is pending: + timed_out = not pending.done and self._pending.get(command_id) is pending + if timed_out: pending.done = True del self._pending[command_id] - timed_out = True if timed_out: with controller.send_lock: with self._lock: @@ -600,97 +382,48 @@ class BrowserControlBroker: active = None if active is not None: self._emit_cancel_frames(active, [pending]) - raise ControllerTimeout( - f"controller did not complete command {command_id!r} " - f"within {self._command_timeout}s" - ) - + raise ControllerTimeout(f"controller did not complete command {command_id!r} within {self._command_timeout}s") if pending.cancelled: raise ControllerCancelled(f"command {command_id!r} was cancelled") if not pending.ok: - raise ControllerRejected( - f"controller rejected command {command_id!r}: {pending.result!r}" - ) + raise ControllerRejected(f"controller rejected command {command_id!r}: {pending.result!r}") return pending.result - def complete( - self, - command_id: str, - *, - scope: Optional[ControllerScope] = None, - ok: bool, - result: Any = None, - ) -> bool: - """Resolve a pending command by id; ``False`` when none is pending. - - Safe from inside the controller's own ``send`` callback. Late - completions after ``cancel``/``detach`` are ignored (``False``). - """ + def complete(self, command_id: str, *, scope: Optional[ControllerScope] = None, ok: bool, result: Any = None) -> bool: + """Resolve a pending command by id; ``False`` when none is pending. Safe from inside the send callback.""" with self._lock: pending = self._pending.get(command_id) - if pending is None or pending.done: + if pending is None or pending.done or (scope is not None and pending.scope != scope): return False - if scope is not None and pending.scope != scope: - return False - pending.done = True - pending.ok = ok is True - pending.result = result + pending.done, pending.ok, pending.result = True, ok is True, result del self._pending[command_id] pending.event.set() return True def cancel(self, scope: ControllerScope, *, tool_call_id: Optional[str]) -> bool: - """Cancel exactly the pending command matching ``scope`` + tool_call_id. - - Emits one cancel frame; returns ``False`` when nothing matched so - transports can answer idempotently. - """ - with self._lock: - controller = self._controller_for_identity_locked(scope) - if controller is None or not controller.connected: + """Cancel the pending command matching ``scope`` + tool_call_id (one cancel frame); ``False`` if none.""" + controller = self._live_controller(scope) + if controller is None: return False with controller.send_lock: with self._lock: - attached = self._controller_for_identity_locked(scope) - if attached is not controller or not controller.connected: + if self._controller_for_identity_locked(scope) is not controller or not controller.connected: return False - target = next( - ( - p - for p in self._pending.values() - if _same_scope_identity(p.scope, scope) - and p.tool_call_id == tool_call_id - and not p.done - ), - None, - ) + target = next((p for p in self._pending_for_scope_locked(scope) + if p.tool_call_id == tool_call_id and not p.done), None) if target is None: return False self._resolve_pending(target, cancelled=True) self._emit_cancel_frames(controller, [target]) return True - # ------------------------------------------------------------------ - # Internals (callers hold the lock unless noted) - # ------------------------------------------------------------------ - def _resolve_pending(self, pending: _PendingCommand, *, cancelled: bool) -> None: - pending.cancelled = cancelled - pending.done = True + pending.cancelled, pending.done = cancelled, True del self._pending[pending.command_id] pending.event.set() - def _validate_artifact_reference( - self, - scope: ControllerScope, - action: str, - arguments: dict, - ) -> None: - """Fail closed unless ``arguments`` carries a store-approved artifact id. - - Missing store/id, traversal, expiry, checksum, or scope mismatch all - surface as :class:`ControllerRejected` before any frame is emitted. - """ + def _validate_artifact_reference(self, scope: ControllerScope, action: str, arguments: dict) -> None: + """Fail closed unless ``arguments`` carries a store-approved artifact id (failures -> ControllerRejected).""" store = self._artifact_store_for_scope(scope) if store is None: raise ControllerRejected(f"{action} requires an attached artifact store") @@ -702,22 +435,10 @@ class BrowserControlBroker: except ControllerRejected: raise except Exception as exc: - raise ControllerRejected( - f"{action} rejected artifact reference {artifact_id!r}: {exc}" - ) from exc - - @staticmethod - def _cancel_frame(pending: _PendingCommand) -> dict: - return { - "method": FRAME_CANCEL, - "params": { - "command_id": pending.command_id, - "tool_call_id": pending.tool_call_id, - }, - } + raise ControllerRejected(f"{action} rejected artifact reference {artifact_id!r}: {exc}") from exc def _defer_cancel_locked(self, controller: _Controller, pending: _PendingCommand) -> None: - controller.deferred_cancels.append(self._cancel_frame(pending)) + controller.deferred_cancels.append(_cancel_frame(pending)) if len(controller.deferred_cancels) > MAX_DEFERRED_CANCELS: del controller.deferred_cancels[:-MAX_DEFERRED_CANCELS] @@ -728,65 +449,30 @@ class BrowserControlBroker: """Send cancel frames (caller holds ``send_lock``, never the broker lock).""" for pending in pendings: try: - controller.send(self._cancel_frame(pending)) + controller.send(_cancel_frame(pending)) except Exception: - logger.exception( - "failed to emit cancel frame for command %r", pending.command_id - ) + logger.exception("failed to emit cancel frame for command %r", pending.command_id) - # ------------------------------------------------------------------ - # Session-lane lookups / bulk teardown - # ------------------------------------------------------------------ - - @staticmethod - def _lane_key(session_id, task_id, principal_id, transport_family) -> Optional[tuple[str, str, str]]: - target = str(session_id or task_id or "").strip() - principal = str(principal_id or "").strip() - family = str(transport_family or "").strip() - if not target or not principal or not family: - return None - return target, principal, family - - def scope_for_session( - self, - *, - session_id: Optional[str] = None, - task_id: Optional[str] = None, - principal_id: Optional[str] = None, - transport_family: Optional[str] = None, - ) -> Optional[ControllerScope]: - """Return one unambiguous attached scope for a server-owned session. - - The public session id is only a hint; the caller must also supply its - server-derived principal and transport family. Missing identity, no - match, or multiple matches fail closed rather than picking by order. - """ - key = self._lane_key(session_id, task_id, principal_id, transport_family) - if key is None: - return None + def _lane_scopes(self, session_id, task_id, principal_id, transport_family) -> list[ControllerScope]: + """Attached scopes bound to one session lane (session + principal + transport).""" + key = tuple(str(v or "").strip() for v in (session_id or task_id, principal_id, transport_family)) + if not all(key): + return [] with self._lock: - matches = self._lane_scopes_locked(*key) + return [s for s in self._controllers if (s.session_id, s.principal_id, s.transport_family) == key] + + def scope_for_session(self, *, session_id: Optional[str] = None, task_id: Optional[str] = None, + principal_id: Optional[str] = None, transport_family: Optional[str] = None) -> Optional[ControllerScope]: + """One unambiguous attached scope for a server-owned session (session id is only a hint; the caller + supplies its server-derived principal + transport family). Missing/ambiguous identity fails closed.""" + matches = self._lane_scopes(session_id, task_id, principal_id, transport_family) return matches[0] if len(matches) == 1 else None - def lane_registered( - self, - *, - session_id: Optional[str] = None, - task_id: Optional[str] = None, - principal_id: Optional[str] = None, - transport_family: Optional[str] = None, - ) -> bool: - """Whether ANY controller (even offline) registered for this lane. - - Distinguishes "lane bound but currently unavailable" (fail closed; - the extension lane stays authoritative) from "never registered" - (caller keeps the legacy backend). Ambiguous lanes report True. - """ - key = self._lane_key(session_id, task_id, principal_id, transport_family) - if key is None: - return False - with self._lock: - return bool(self._lane_scopes_locked(*key)) + def lane_registered(self, *, session_id: Optional[str] = None, task_id: Optional[str] = None, + principal_id: Optional[str] = None, transport_family: Optional[str] = None) -> bool: + """Whether ANY controller (even offline) registered for this lane: "bound but unavailable" fails closed + vs "never registered" (caller keeps the legacy backend). Ambiguous lanes report True.""" + return bool(self._lane_scopes(session_id, task_id, principal_id, transport_family)) def disconnect_owner(self, owner: Any) -> int: """Mark every controller owned by one lost transport offline.""" diff --git a/gateway/control_socket.py b/gateway/control_socket.py index 668ac22608..6a767a7a01 100644 --- a/gateway/control_socket.py +++ b/gateway/control_socket.py @@ -1,18 +1,10 @@ -"""Gateway control socket — the gateway-owned local coordination surface. - -Instead of process-table scans or ``gateway_state.json`` (which can outlive -its writer), the gateway serves a local-only socket answering versioned JSON -verbs (``identify``, ``status``). A connectable socket with a well-formed -``identify`` answer IS liveness — no PID-reuse heuristics. - -Transport — never a TCP port; filesystem/pipe ACLs are the auth boundary. -POSIX: ``$HERMES_HOME/gateway.sock``, or a temp-dir socket plus a -``gateway.sock.path`` pointer file when the home path exceeds ``sun_path``. -Windows: named pipe ``\\\\.\\pipe\\hermes-gateway-``. - -Wire contract: ONE request per connection — one JSON line in, one out, then -the server closes (no keep-alive/pipelining). Consumers PREFER the socket and -fall back to the state-file/scan layer when it doesn't answer. +"""Gateway control socket — the gateway-owned local coordination surface: a local-only socket answering +versioned JSON verbs (``identify``, ``status``). A connectable socket with a well-formed ``identify`` +answer IS liveness — no PID-reuse heuristics. Never a TCP port: filesystem/pipe ACLs are the auth +boundary. POSIX: ``$HERMES_HOME/gateway.sock`` (or a temp-dir socket + ``gateway.sock.path`` pointer +file when the home path exceeds ``sun_path``); Windows: named pipe ``\\\\.\\pipe\\hermes-gateway-``. +Wire contract: ONE request per connection — one JSON line in, one out, then the server closes. +Consumers PREFER the socket and fall back to the state-file/scan layer when it doesn't answer. """ from __future__ import annotations @@ -33,28 +25,18 @@ from typing import Any, Callable, Optional logger = logging.getLogger(__name__) CONTROL_PROTOCOL_VERSION = 1 - _SOCKET_FILENAME = "gateway.sock" _POINTER_FILENAME = "gateway.sock.path" _IS_WINDOWS = sys.platform == "win32" - -# sun_path limit is 104 on macOS/BSD, 108 on Linux; leave margin for the NUL. -_MAX_UNIX_PATH = 100 - +_MAX_UNIX_PATH = 100 # sun_path limit is 104 on macOS/BSD, 108 on Linux; margin for the NUL # Single-line JSON in/out; bounded so a misbehaving peer can't balloon memory. _MAX_REQUEST_BYTES = 64 * 1024 _MAX_RESPONSE_BYTES = 512 * 1024 - _DEFAULT_CLIENT_TIMEOUT = 2.0 -# --------------------------------------------------------------------------- -# Path resolution (shared by server and client) -# --------------------------------------------------------------------------- - def _home_hash(home: Path) -> str: - canonical = os.path.normcase(str(Path(home).expanduser().resolve(strict=False))) - return hashlib.sha256(canonical.encode("utf-8")).hexdigest()[:16] + return hashlib.sha256(os.path.normcase(str(Path(home).expanduser().resolve(strict=False))).encode("utf-8")).hexdigest()[:16] def windows_pipe_name(home: Path) -> str: @@ -67,29 +49,18 @@ def _fits_sun_path(path: Path) -> bool: def _fallback_socket_path(home: Path) -> Path: - """Short temp-dir path for homes whose direct socket path exceeds sun_path. - - Tries ``tempfile.gettempdir()`` then ``/tmp`` (POSIX). If nothing fits the - tempdir candidate is returned anyway — bind fails non-fatally and - consumers use the scan layer. - """ + """Short temp-dir path for homes whose direct socket path exceeds sun_path: ``tempfile.gettempdir()`` + then ``/tmp`` (POSIX); if nothing fits the tempdir candidate is returned anyway — bind fails + non-fatally and consumers use the scan layer.""" name = f"hermes-gw-{_home_hash(home)}.sock" - candidates = [Path(tempfile.gettempdir()) / name] - if not _IS_WINDOWS: - candidates.append(Path("/tmp") / name) - for candidate in candidates: - if _fits_sun_path(candidate): - return candidate - return candidates[0] + candidates = [Path(tempfile.gettempdir()) / name] + ([] if _IS_WINDOWS else [Path("/tmp") / name]) + return next((c for c in candidates if _fits_sun_path(c)), candidates[0]) def resolve_server_socket_path(home: Path) -> tuple[Path, Optional[Path]]: - """Return ``(bind_path, pointer_file)``; pointer_file is non-None only - when the temp-dir fallback is in use and the server must record it.""" + """Return ``(bind_path, pointer_file)``; pointer_file is set only for the temp-dir fallback.""" direct = Path(home) / _SOCKET_FILENAME - if _fits_sun_path(direct): - return direct, None - return _fallback_socket_path(home), Path(home) / _POINTER_FILENAME + return (direct, None) if _fits_sun_path(direct) else (_fallback_socket_path(home), Path(home) / _POINTER_FILENAME) def resolve_client_socket_path(home: Path) -> Optional[Path]: @@ -97,98 +68,60 @@ def resolve_client_socket_path(home: Path) -> Optional[Path]: direct = Path(home) / _SOCKET_FILENAME if direct.exists(): return direct - pointer = Path(home) / _POINTER_FILENAME - try: - if pointer.is_file(): - target = pointer.read_text(encoding="utf-8").strip() - if target and Path(target).exists(): - return Path(target) - except OSError: - pass + with contextlib.suppress(OSError): + pointer = Path(home) / _POINTER_FILENAME + target = pointer.read_text(encoding="utf-8").strip() if pointer.is_file() else "" + if target and Path(target).exists(): + return Path(target) return None -# --------------------------------------------------------------------------- -# Default payload builders (import-light; overridable at wiring time) -# --------------------------------------------------------------------------- - def _detect_supervisor() -> str: - """Supervisor kind for THIS process, declared from its own launch env - (not inferred outside-in like ``_detect_supervisor_for_pid``).""" + """Supervisor kind for THIS process from its own launch env (not inferred outside-in).""" env = os.environ if env.get("INVOCATION_ID"): return "systemd" - if sys.platform == "darwin" and ( - env.get("XPC_SERVICE_NAME", "").startswith("ai.hermes") - or env.get("LAUNCHD_SOCKET") - ): + if sys.platform == "darwin" and (env.get("XPC_SERVICE_NAME", "").startswith("ai.hermes") + or env.get("LAUNCHD_SOCKET")): return "launchd" if env.get("HERMES_DESKTOP_MANAGED"): return "desktop" - if "--external-supervisor" in sys.argv: - return "external" - return "manual" + return "external" if "--external-supervisor" in sys.argv else "manual" def build_identify_payload() -> dict[str, Any]: """Default ``identify`` answer, built from gateway.status primitives.""" - from gateway.status import ( - _build_pid_record, - _get_code_identity_fields, - _profile_label_for_home, - read_runtime_status, - ) - + from gateway.status import _build_pid_record, _get_code_identity_fields, _profile_label_for_home, read_runtime_status record = _build_pid_record() payload: dict[str, Any] = { "protocol": CONTROL_PROTOCOL_VERSION, **{k: record.get(k) for k in ("kind", "pid", "start_time", "hermes_home")}, "profile": _profile_label_for_home(record.get("hermes_home") or ""), - "supervisor": _detect_supervisor(), - } - payload.update(_get_code_identity_fields()) - # served_profiles (multiplex mode) is stamped into runtime status by the runner. - try: + "supervisor": _detect_supervisor(), **_get_code_identity_fields()} + with contextlib.suppress(Exception): + # served_profiles (multiplex mode) is stamped into runtime status by the runner. served = (read_runtime_status() or {}).get("served_profiles") if isinstance(served, list) and served: payload["served_profiles"] = served - except Exception: - pass return payload def build_status_payload() -> dict[str, Any]: """Default ``status`` answer — current runtime status, answered live.""" from gateway.status import read_runtime_status + return {**(read_runtime_status() or {}), "protocol": CONTROL_PROTOCOL_VERSION, + "answered_at": time.time(), "answering_pid": os.getpid()} - payload = dict(read_runtime_status() or {}) - payload["protocol"] = CONTROL_PROTOCOL_VERSION - payload["answered_at"] = time.time() - payload["answering_pid"] = os.getpid() - return payload - - -# --------------------------------------------------------------------------- -# Server -# --------------------------------------------------------------------------- class GatewayControlServer: - """Gateway-owned control socket server (identify/status, v1). + """Gateway-owned control socket server (identify/status, v1): ``start()`` after the PID-file claim, + ``stop()`` on shutdown. All failures are non-fatal — the gateway never refuses to serve messaging + because its control socket couldn't bind; consumers fall back to the scan layer.""" - ``start()`` after the PID-file claim, ``stop()`` on shutdown. All failures - are non-fatal — the gateway never refuses to serve messaging because its - control socket couldn't bind; consumers fall back to the scan layer. - """ - - def __init__( - self, - home: Optional[Path] = None, - *, - verb_handlers: Optional[dict[str, Callable[[], dict[str, Any]]]] = None, - ) -> None: + def __init__(self, home: Optional[Path] = None, *, + verb_handlers: Optional[dict[str, Callable[[], dict[str, Any]]]] = None) -> None: if home is None: from gateway.status import _get_process_hermes_home - home = _get_process_hermes_home() self._home = Path(home) self._server: Optional[asyncio.AbstractServer] = None @@ -196,33 +129,24 @@ class GatewayControlServer: self._bind_path: Optional[Path] = None self._pointer_file: Optional[Path] = None self._handlers: dict[str, Callable[[], dict[str, Any]]] = { - "identify": build_identify_payload, - "status": build_status_payload, - } - if verb_handlers: - self._handlers.update(verb_handlers) - - # -- lifecycle --------------------------------------------------------- + "identify": build_identify_payload, "status": build_status_payload, **(verb_handlers or {})} async def start(self) -> bool: """Bind and start serving. Returns True on success, False otherwise.""" try: - if _IS_WINDOWS: - return await self._start_windows() - return await self._start_posix() + return await (self._start_windows() if _IS_WINDOWS else self._start_posix()) except Exception as exc: logger.warning("Gateway control socket failed to start (non-fatal): %s", exc) return False async def _start_posix(self) -> bool: bind_path, pointer_file = resolve_server_socket_path(self._home) - # We only get here after winning the PID-file O_EXCL race, so any - # existing file is stale or a collision — never a live sibling. + # We only get here after winning the PID-file O_EXCL race, so any existing + # file is stale or a collision — never a live sibling. with contextlib.suppress(OSError): if bind_path.exists(): bind_path.unlink() - # Restrictive umask so the socket is never world-connectable, even - # for the instant before chmod. + # Restrictive umask so the socket is never world-connectable, even for the instant before chmod. old_umask = os.umask(0o177) try: self._server = await asyncio.start_unix_server(self._handle_connection, path=str(bind_path)) @@ -241,10 +165,8 @@ class GatewayControlServer: loop = asyncio.get_running_loop() start_serving_pipe = getattr(loop, "start_serving_pipe", None) if start_serving_pipe is None: - logger.debug( - "Event loop %s has no start_serving_pipe — control socket " - "disabled (selector loop on Windows).", type(loop).__name__, - ) + logger.debug("Event loop %s has no start_serving_pipe — control socket " + "disabled (selector loop on Windows).", type(loop).__name__) return False pipe_name = windows_pipe_name(self._home) servers = await start_serving_pipe(lambda: _PipeControlProtocol(self), pipe_name) @@ -258,48 +180,34 @@ class GatewayControlServer: self._server.close() with contextlib.suppress(Exception): await self._server.wait_closed() - self._server = None if self._pipe_server is not None: with contextlib.suppress(Exception): self._pipe_server.close() - self._pipe_server = None + self._server = self._pipe_server = None self.cleanup_files() def cleanup_files(self) -> None: """Best-effort removal of socket + pointer files (atexit-safe).""" - for path in (self._bind_path, self._pointer_file): - if path is not None: - with contextlib.suppress(OSError): - path.unlink(missing_ok=True) - - # -- request handling ---------------------------------------------------- + for path in filter(None, (self._bind_path, self._pointer_file)): + with contextlib.suppress(OSError): + path.unlink(missing_ok=True) def handle_request_line(self, raw: bytes) -> bytes: - """One JSON request line -> one JSON response line. Never raises. - - Shared by the POSIX stream handler and the Windows pipe protocol. - """ + """One JSON request line -> one JSON response line. Never raises (shared by POSIX + pipe).""" request_id: Any = None try: request = json.loads(raw.decode("utf-8")) if not isinstance(request, dict): raise ValueError("request must be a JSON object") - request_id = request.get("id") - verb = request.get("verb") + request_id, verb = request.get("id"), request.get("verb") handler = self._handlers.get(verb) if isinstance(verb, str) else None if handler is None: - response: dict[str, Any] = { - "ok": False, "error": f"unknown verb: {verb!r}", - "protocol": CONTROL_PROTOCOL_VERSION, - "supported_verbs": sorted(self._handlers), - } + response: dict[str, Any] = {"ok": False, "error": f"unknown verb: {verb!r}", + "protocol": CONTROL_PROTOCOL_VERSION, "supported_verbs": sorted(self._handlers)} else: response = {"ok": True, "protocol": CONTROL_PROTOCOL_VERSION, "result": handler()} except Exception as exc: - response = { - "ok": False, "error": f"{type(exc).__name__}: {exc}", - "protocol": CONTROL_PROTOCOL_VERSION, - } + response = {"ok": False, "error": f"{type(exc).__name__}: {exc}", "protocol": CONTROL_PROTOCOL_VERSION} if request_id is not None: response["id"] = request_id try: @@ -315,10 +223,10 @@ class GatewayControlServer: raw = await asyncio.wait_for(reader.readline(), timeout=_DEFAULT_CLIENT_TIMEOUT) if not raw or len(raw) > _MAX_REQUEST_BYTES: return - # Handlers read disk; keep that off the loop that drives every - # platform adapter so a fast-polling consumer can't stall heartbeats. - loop = asyncio.get_running_loop() - response = await loop.run_in_executor(None, self.handle_request_line, raw.rstrip(b"\n")) + # Handlers read disk; keep that off the loop that drives every platform + # adapter so a fast-polling consumer can't stall heartbeats. + response = await asyncio.get_running_loop().run_in_executor( + None, self.handle_request_line, raw.rstrip(b"\n")) writer.write(response) await writer.drain() except (asyncio.TimeoutError, ConnectionError, OSError): @@ -332,7 +240,6 @@ class GatewayControlServer: class _PipeControlProtocol(asyncio.Protocol): """One-shot request/response protocol for the Windows named pipe.""" - def __init__(self, server: GatewayControlServer) -> None: self._server = server self._transport: Any = None @@ -345,43 +252,25 @@ class _PipeControlProtocol(asyncio.Protocol): self._buffer.extend(data) if len(self._buffer) > _MAX_REQUEST_BYTES: self._transport.close() - return - if b"\n" in self._buffer: - line, _, _ = bytes(self._buffer).partition(b"\n") + elif b"\n" in self._buffer: try: - self._transport.write(self._server.handle_request_line(line)) + self._transport.write(self._server.handle_request_line(bytes(self._buffer).partition(b"\n")[0])) finally: self._transport.close() -# --------------------------------------------------------------------------- -# Client (synchronous — used by CLI/updater consumers) -# --------------------------------------------------------------------------- - -def query_gateway_control( - home: Path, verb: str, *, timeout: float = _DEFAULT_CLIENT_TIMEOUT -) -> Optional[dict[str, Any]]: - """Ask the gateway serving ``home`` a control verb; None when unanswered. - - Returns the verb's ``result`` payload. Any failure (no/stale socket, - timeout, malformed answer, ``ok: false``) returns None so callers fall - back to the scan layer. Never raises. - """ +def query_gateway_control(home: Path, verb: str, *, timeout: float = _DEFAULT_CLIENT_TIMEOUT) -> Optional[dict[str, Any]]: + """Ask the gateway serving ``home`` a control verb; returns its ``result`` payload. Any failure (no/stale + socket, timeout, malformed answer, ``ok: false``) returns None so callers fall back to the scan layer. + Never raises.""" request = json.dumps({"verb": verb, "id": 1, "protocol": CONTROL_PROTOCOL_VERSION}).encode("utf-8") + b"\n" query = _query_windows_pipe if _IS_WINDOWS else _query_unix_socket try: raw = query(Path(home), request, timeout) + response = json.loads(raw.decode("utf-8")) if raw else None except Exception: return None - if not raw: - return None - try: - response = json.loads(raw.decode("utf-8")) - except (ValueError, UnicodeDecodeError): - return None - if not isinstance(response, dict) or response.get("ok") is not True: - return None - result = response.get("result") + result = response.get("result") if isinstance(response, dict) and response.get("ok") is True else None return result if isinstance(result, dict) else None @@ -390,32 +279,25 @@ def _read_response_line(read: Callable[[], bytes], deadline: float) -> Optional[ chunks: list[bytes] = [] while time.monotonic() < deadline: chunk = read() - if not chunk: - break chunks.append(chunk) - if b"\n" in chunk: + if not chunk or b"\n" in chunk: break if sum(len(c) for c in chunks) > _MAX_RESPONSE_BYTES: return None - line, _, _ = b"".join(chunks).partition(b"\n") - return line or None + return b"".join(chunks).partition(b"\n")[0] or None def _query_unix_socket(home: Path, request: bytes, timeout: float) -> Optional[bytes]: path = resolve_client_socket_path(home) if path is None: return None - with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock: + # OSError covers ConnectionRefusedError / FileNotFoundError on connect and socket.timeout on read. + with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock, contextlib.suppress(OSError): sock.settimeout(timeout) - try: - sock.connect(str(path)) - except (ConnectionRefusedError, FileNotFoundError, OSError): - return None + sock.connect(str(path)) sock.sendall(request) - try: - return _read_response_line(lambda: sock.recv(65536), time.monotonic() + timeout) - except socket.timeout: - return None + return _read_response_line(lambda: sock.recv(65536), time.monotonic() + timeout) + return None def _query_windows_pipe(home: Path, request: bytes, timeout: float) -> Optional[bytes]: # pragma: no cover - wine2e lane @@ -446,10 +328,7 @@ def identify_gateway(home: Path, *, timeout: float = _DEFAULT_CLIENT_TIMEOUT) -> def pause_gateway_for_update(home: Path, *, timeout: float = _DEFAULT_CLIENT_TIMEOUT) -> Optional[dict[str, Any]]: - """Ask the gateway serving ``home`` to drain and exit for an update. - - Returns the ACK ``{"pausing", "already_stopping", "pid", "drain_timeout"}`` - or None when no gateway answers (old gateway without the verb, no/dead - socket) — the caller then uses the legacy signal/tree-kill pause path. - """ + """Ask the gateway serving ``home`` to drain and exit for an update. Returns the ACK ``{"pausing", + "already_stopping", "pid", "drain_timeout"}`` or None when no gateway answers (old gateway without + the verb, no/dead socket) — the caller then uses the legacy signal/tree-kill pause path.""" return query_gateway_control(home, "pause-for-update", timeout=timeout) diff --git a/gateway/dead_targets.py b/gateway/dead_targets.py index b0ffa3cdd6..2b47b3083b 100644 --- a/gateway/dead_targets.py +++ b/gateway/dead_targets.py @@ -1,18 +1,9 @@ -"""Persistent registry of delivery targets that are confirmed unreachable. - -When a platform reports a target chat is permanently gone (deleted group, -bot kicked/blocked, deactivated user), re-sending on every cron tick wastes a -send against flood control and spams logs. The delivery layer short-circuits -targets proven dead; any later successful send clears the flag (self-healing). - -Scope is deliberately narrow: only *whole-chat* deaths (``forbidden`` and -chat-level ``not_found``) are recorded. Thread/topic-level ``not_found`` is -NOT — adapters self-heal that by retrying without ``reply_to``, and a deleted -topic does not mean the parent chat is dead. - -Storage is a JSON file under the active profile's HERMES_HOME (each profile -keeps its own dead set). Reads/writes are best-effort: a corrupt or unwritable -file degrades to in-memory-only rather than raising on the delivery path. +"""Persistent registry of delivery targets confirmed unreachable. Re-sending to a permanently gone chat +(deleted group, bot kicked/blocked, deactivated user) every cron tick wastes flood-control budget; delivery +short-circuits targets proven dead and any later successful send clears the flag. Only *whole-chat* deaths +(``forbidden``, chat-level ``not_found``) are recorded: adapters self-heal thread/topic-level ``not_found`` +by retrying without ``reply_to``. Storage is a per-profile JSON file; reads/writes are best-effort (a +corrupt or unwritable file degrades to in-memory-only rather than raising on the delivery path). """ from __future__ import annotations @@ -28,8 +19,7 @@ from hermes_cli.config import get_hermes_home logger = logging.getLogger(__name__) -# Error kinds (gateway.platforms.base.classify_send_error) meaning the whole -# chat is unreachable, not a transient or thread-level problem. +# classify_send_error kinds meaning the whole chat is unreachable (not transient / thread-level). _DEAD_ERROR_KINDS = frozenset({"forbidden", "not_found"}) @@ -38,29 +28,35 @@ def _normalize(platform: str, chat_id: str) -> str: return f"{str(platform).strip().lower()}:{str(chat_id).strip()}" -class DeadTargetRegistry: - """Thread-safe, persistent set of confirmed-dead targets keyed ``platform:chat_id``. +def classify_dead_error(error_text: Optional[str]) -> Optional[str]: + """Best-effort dead-target error_kind from a raised error's text, else None. ``_deliver_to_platform`` + raises on hard failure (no SendResult), so ``deliver()`` only has the exception string. ``not_found`` + collapses chat-level and thread/topic/message-level failures: only a whole-chat not_found means the + target is dead — a deleted forum topic or edited-away message must not mark the entire chat dead.""" + if not error_text: + return None + try: + from .platforms.base import classify_send_error, is_chat_level_not_found + except Exception: # pragma: no cover - import guard + return None + kind = classify_send_error(None, error_text=error_text) + dead = kind in _DEAD_ERROR_KINDS and (kind != "not_found" or is_chat_level_not_found(error_text=error_text)) + return kind if dead else None - Each entry stores reason + timestamp for observability; :meth:`clear` - (called on a successful send) removes the flag. - """ + +class DeadTargetRegistry: + """Thread-safe, persistent set of confirmed-dead targets keyed ``platform:chat_id``. Each entry stores + reason + timestamp for observability; :meth:`clear` (called on a successful send) removes the flag.""" def __init__(self, path: Optional[Path] = None) -> None: self._lock = threading.RLock() self._dead: Dict[str, Dict[str, object]] = {} self._path = path if path is not None else get_hermes_home() / "gateway" / "dead_targets.json" - self._load() - - def _load(self) -> None: try: - if self._path.exists(): - raw = json.loads(self._path.read_text(encoding="utf-8")) - if isinstance(raw, dict): - self._dead = {k: v for k, v in raw.items() if isinstance(v, dict)} + raw = json.loads(self._path.read_text(encoding="utf-8")) if self._path.exists() else {} + self._dead = {k: v for k, v in raw.items() if isinstance(v, dict)} if isinstance(raw, dict) else {} except (OSError, ValueError) as exc: - logger.debug("dead_targets: could not load %s (%s) — starting empty", - self._path, exc) - self._dead = {} + logger.debug("dead_targets: could not load %s (%s) — starting empty", self._path, exc) def _flush_locked(self) -> None: try: @@ -68,42 +64,26 @@ class DeadTargetRegistry: tmp = self._path.with_suffix(self._path.suffix + ".tmp") tmp.write_text(json.dumps(self._dead, indent=2), encoding="utf-8") tmp.replace(self._path) - except OSError as exc: - # Best-effort: keep in-memory state, never break delivery. + except OSError as exc: # best-effort: keep in-memory state, never break delivery logger.debug("dead_targets: could not persist %s (%s)", self._path, exc) - @staticmethod - def is_dead_error_kind(error_kind: Optional[str]) -> bool: - """True when ``error_kind`` denotes a permanent whole-chat death.""" - return bool(error_kind) and error_kind in _DEAD_ERROR_KINDS - def is_dead(self, platform: str, chat_id: Optional[str]) -> bool: - if not chat_id: - return False with self._lock: - return _normalize(platform, chat_id) in self._dead + return bool(chat_id) and _normalize(platform, chat_id) in self._dead - def mark_dead(self, platform: str, chat_id: Optional[str], - reason: str = "") -> bool: + def mark_dead(self, platform: str, chat_id: Optional[str], reason: str = "") -> bool: """Record a target as confirmed-dead. Returns True if newly added.""" if not chat_id: return False key = _normalize(platform, chat_id) with self._lock: existed = key in self._dead - self._dead[key] = { - "platform": str(platform).strip().lower(), - "chat_id": str(chat_id), - "reason": str(reason)[:200], - "marked_at": time.time(), - } + self._dead[key] = {"platform": str(platform).strip().lower(), "chat_id": str(chat_id), + "reason": str(reason)[:200], "marked_at": time.time()} self._flush_locked() if not existed: - logger.info( - "dead_targets: marked %s as unreachable (%s) — future deliveries " - "to this target will be skipped until a send succeeds", - key, reason or "no reason given", - ) + logger.info("dead_targets: marked %s as unreachable (%s) — future deliveries " + "to this target will be skipped until a send succeeds", key, reason or "no reason given") return not existed def clear(self, platform: str, chat_id: Optional[str]) -> bool: @@ -112,9 +92,8 @@ class DeadTargetRegistry: return False key = _normalize(platform, chat_id) with self._lock: - if key in self._dead: - del self._dead[key] - self._flush_locked() - logger.info("dead_targets: cleared %s (delivery succeeded again)", key) - return True - return False + if self._dead.pop(key, None) is None: + return False + self._flush_locked() + logger.info("dead_targets: cleared %s (delivery succeeded again)", key) + return True diff --git a/gateway/delivery.py b/gateway/delivery.py index e5ded9c5f8..b8478da44f 100644 --- a/gateway/delivery.py +++ b/gateway/delivery.py @@ -1,10 +1,5 @@ -""" -Delivery routing for cron job outputs and agent responses. - -Routes messages by target: explicit ("telegram:123456789"), platform home -channel ("telegram"), origin (back to where the job was created), or local -(saved to files). -""" +"""Delivery routing for cron job outputs and agent responses, by target: explicit ("telegram:123456789"), +platform home channel ("telegram"), origin (back to where the job was created), or local (files).""" import logging import os @@ -16,43 +11,35 @@ from typing import Dict, List, Optional, Any from hermes_cli.config import get_hermes_home +from .config import Platform, GatewayConfig, PlatformConfig +from .session import SessionSource +from .dead_targets import DeadTargetRegistry, classify_dead_error + logger = logging.getLogger(__name__) -# Cap before gateway-level truncation of cron output for non-chunking platform -# delivery. Telegram's hard API limit is 4096; the headroom covers the "full -# output saved to …" footer. Adapters that split long messages natively -# (BasePlatformAdapter.splits_long_messages) bypass this entirely. +# Cap before gateway-level truncation of cron output for non-chunking platform delivery. Telegram's hard +# API limit is 4096; the headroom covers the "full output saved to …" footer. Adapters that split long +# messages natively (splits_long_messages) bypass this entirely. MAX_PLATFORM_OUTPUT = 4000 - -# Matches strings that are *only* a "silence" narration with optional markdown -# wrappers: *(silent)*, _silent_, `silent`, (silent), silent, 🔇, a bare ".", -# "…", and marker-padded variants. Anchored so substantive messages that merely -# *contain* the word "silent" never match. +# Matches strings that are *only* a "silence" narration with optional markdown wrappers (*(silent)*, +# _silent_, 🔇, a bare ".", "…"). Anchored so messages that merely *contain* "silent" never match. _SILENCE_NARRATION = re.compile( r'^[\s*_~`]*\(?\s*(silent|silence|no\s+response|no\s+reply)\s*\.?\)?[\s*_~`]*$' r'|^[\s*_~`]*[\U0001F507\.\u2026]+[\s*_~`]*$', re.IGNORECASE, ) +_THREAD_ROUTING_KEYS = ("thread_id", "message_thread_id", "direct_messages_topic_id", "telegram_direct_messages_topic_id") def _is_silence_narration(content: Optional[str]) -> bool: """True when ``content`` is *only* a silence-narration token (length-guarded).""" - if not content: - return False - stripped = content.strip() - if not stripped or len(stripped) > 64: - return False - return bool(_SILENCE_NARRATION.match(stripped)) - -from .config import Platform, GatewayConfig, PlatformConfig -from .session import SessionSource -from .dead_targets import DeadTargetRegistry + stripped = content.strip() if content else "" + return bool(stripped) and len(stripped) <= 64 and bool(_SILENCE_NARRATION.match(stripped)) @dataclass(frozen=True) class DeliveryTransport: """Resolved live transport for one logical delivery platform.""" - adapter: Any config: Optional[PlatformConfig] transport_platform: Platform @@ -61,128 +48,59 @@ class DeliveryTransport: def is_relay(self) -> bool: return self.transport_platform == Platform.RELAY - async def send( - self, - logical_platform: Platform, - chat_id: str, - content: str, - metadata: Optional[Dict[str, Any]], - ) -> Any: + async def send(self, logical_platform: Platform, chat_id: str, content: str, + metadata: Optional[Dict[str, Any]]) -> Any: """Send through this transport while preserving the logical platform.""" - if self.is_relay: - return await self.adapter.send_for_platform( - logical_platform, chat_id, content, metadata=metadata, - ) - return await self.adapter.send(chat_id, content, metadata=metadata) + return await (self.adapter.send_for_platform(logical_platform, chat_id, content, metadata=metadata) + if self.is_relay else self.adapter.send(chat_id, content, metadata=metadata)) -def resolve_delivery_transport( - platform: Platform, - config: GatewayConfig, - adapters: Optional[Dict[Platform, Any]], -) -> Optional[DeliveryTransport]: - """Resolve a logical platform to its live delivery transport. - - A concrete native adapter always wins. Relay is eligible only when its - authenticated transport explicitly advertises that it fronts the logical - platform, so restart-time delivery is independent of per-chat caches without - letting Relay hijack unrelated platform targets. - """ +def resolve_delivery_transport(platform: Platform, config: GatewayConfig, + adapters: Optional[Dict[Platform, Any]]) -> Optional[DeliveryTransport]: + """Resolve a logical platform to its live delivery transport. A concrete native adapter always wins; + Relay is eligible only when its authenticated transport explicitly advertises that it fronts the + logical platform, so restart-time delivery is independent of per-chat caches without letting Relay + hijack unrelated platform targets.""" live_adapters = adapters or {} - native = live_adapters.get(platform) - native_config = config.platforms.get(platform) + native, native_config = live_adapters.get(platform), config.platforms.get(platform) # Explicitly supplied live adapters with no config block are honored, but an # explicitly disabled native adapter never shadows an enabled Relay transport. if native is not None and (native_config is None or native_config.enabled): return DeliveryTransport(native, native_config, platform) - - relay = live_adapters.get(Platform.RELAY) - relay_config = config.platforms.get(Platform.RELAY) + relay, relay_config = live_adapters.get(Platform.RELAY), config.platforms.get(Platform.RELAY) fronts_platform = getattr(relay, "fronts_platform", None) - if ( - relay is not None - and (relay_config is None or relay_config.enabled) - and callable(fronts_platform) - and fronts_platform(platform) - ): + if (relay is not None and (relay_config is None or relay_config.enabled) + and callable(fronts_platform) and fronts_platform(platform)): return DeliveryTransport(relay, relay_config, Platform.RELAY) return None -def _as_int(value: Any) -> Optional[int]: - try: - return int(value) - except (TypeError, ValueError): - return None - - def looks_like_telegram_private_chat_id(chat_id: Optional[str]) -> bool: - """True when ``chat_id`` is a positive int — Telegram's private-chat shape. - - Groups/channels/supergroups use negative IDs. Single source of truth for the - heuristic, reused by the handoff seed path in ``gateway/run.py`` so - handoff-created DM topics key the same way as inbound DM-topic messages. - """ - if chat_id is None: + """True when ``chat_id`` is a positive int — Telegram's private-chat shape (groups/channels are negative). + Single source of truth, reused by the handoff seed path in ``gateway/run.py`` so handoff-created DM + topics key the same way as inbound DM-topic messages.""" + try: + return int(chat_id) > 0 + except (TypeError, ValueError): return False - parsed = _as_int(chat_id) - return parsed is not None and parsed > 0 def _looks_like_int(value: Optional[str]) -> bool: - return value is not None and _as_int(value) is not None - - -def _result_field(result: Any, name: str, default: Any = None) -> Any: - """Read ``name`` from a SendResult object or a plain result dict.""" - if isinstance(result, dict): - return result.get(name, default) - return getattr(result, name, default) - - -def _send_result_failed(result: Any) -> bool: - return _result_field(result, "success", True) is False + try: + return int(value) is not None + except (TypeError, ValueError): + return False def _send_result_error(result: Any) -> Optional[str]: - error = _result_field(result, "error") - return str(error) if error else None - - -def _is_thread_not_found_delivery_error(result: Any) -> bool: - error = _send_result_error(result) - return bool(error and "thread not found" in error.lower()) - - -def _classify_dead_from_error_text(error_text: Optional[str]) -> Optional[str]: - """Best-effort dead-target classification from a raised error's text. - - ``_deliver_to_platform`` raises on hard failure (no SendResult), so the - ``deliver()`` loop only has the exception string; reuse the platform-neutral - classifier to recover the error_kind from it. - """ - if not error_text: - return None - try: - from .platforms.base import classify_send_error, is_chat_level_not_found - except Exception: # pragma: no cover - import guard - return None - kind = classify_send_error(None, error_text=error_text) - if not DeadTargetRegistry.is_dead_error_kind(kind): - return None - # ``not_found`` collapses chat-level and thread/topic/message-level failures. - # Only a whole-chat not_found means the target is dead — a deleted forum - # topic or an edited-away message must not mark the entire chat (and all its - # future deliveries) dead (see gateway.dead_targets). - if kind == "not_found" and not is_chat_level_not_found(error_text=error_text): - return None - return kind + """Error string of a failed SendResult object / plain result dict ("" if none), or None on success.""" + get = result.get if isinstance(result, dict) else (lambda name, default=None: getattr(result, name, default)) + return None if get("success", True) is not False else str(get("error") or "") @dataclass class DeliveryTarget: - """A single delivery target: "origin", "local", "telegram" (home channel), - or "telegram:123456[:thread]" (specific chat).""" + """One target: "origin", "local", "telegram" (home channel) or "telegram:123456[:thread]".""" platform: Platform chat_id: Optional[str] = None # None means use home channel thread_id: Optional[str] = None @@ -192,38 +110,18 @@ class DeliveryTarget: @classmethod def parse(cls, target: str, origin: Optional[SessionSource] = None) -> "DeliveryTarget": """Parse "origin" | "local" | "" | ":[:]".""" - target_stripped = target.strip() - target_lower = target_stripped.lower() - - if target_lower == "origin": - if origin: - return cls( - platform=origin.platform, - chat_id=origin.chat_id, - thread_id=origin.thread_id, - is_origin=True, - ) - return cls(platform=Platform.LOCAL, is_origin=True) - - if target_lower == "local": - return cls(platform=Platform.LOCAL) - - # Platform names are case-insensitive; chat/thread ids keep original case. - # Unknown platforms are treated as local. - if ":" in target_stripped: - parts = target_stripped.split(":", 2) - chat_id = parts[1] if len(parts) > 1 else None - thread_id = parts[2] if len(parts) > 2 else None - try: - platform = Platform(parts[0].lower()) - except ValueError: - return cls(platform=Platform.LOCAL) - return cls(platform=platform, chat_id=chat_id, thread_id=thread_id, is_explicit=True) - + target = target.strip() + if target.lower() == "origin": + return (cls(platform=origin.platform, chat_id=origin.chat_id, thread_id=origin.thread_id, is_origin=True) + if origin else cls(platform=Platform.LOCAL, is_origin=True)) + # Platform names are case-insensitive; chat/thread ids keep case. Unknown platforms -> local. + parts = target.split(":", 2) try: - return cls(platform=Platform(target_lower)) + platform = Platform(parts[0].lower()) except ValueError: return cls(platform=Platform.LOCAL) + return (cls(platform=platform, chat_id=parts[1], thread_id=parts[2] if len(parts) > 2 else None, is_explicit=True) + if len(parts) > 1 else cls(platform=platform)) def to_string(self) -> str: """Convert back to string format.""" @@ -231,23 +129,16 @@ class DeliveryTarget: return "origin" if self.platform == Platform.LOCAL: return "local" - if self.chat_id and self.thread_id: - return f"{self.platform.value}:{self.chat_id}:{self.thread_id}" - if self.chat_id: - return f"{self.platform.value}:{self.chat_id}" - return self.platform.value + parts = [self.platform.value, self.chat_id, self.thread_id if self.chat_id else None] + return ":".join(p for p in parts if p) async def _ensure_named_dm_topic(adapter: Any, chat_id: str, name: str, *, refresh: bool) -> str: """Create (or force-recreate) a named Telegram private DM topic; return its thread id.""" - verb = "refresh" if refresh else "create" - ensure_dm_topic = getattr(adapter, "ensure_dm_topic", None) + verb, ensure_dm_topic = "refresh" if refresh else "create", getattr(adapter, "ensure_dm_topic", None) if ensure_dm_topic is None: raise RuntimeError(f"Telegram adapter cannot {verb} named private DM topics") - if refresh: - thread_id = await ensure_dm_topic(chat_id, name, force_create=True) - else: - thread_id = await ensure_dm_topic(chat_id, name) + thread_id = await ensure_dm_topic(chat_id, name, **({"force_create": True} if refresh else {})) if not thread_id: raise RuntimeError(f"Failed to {verb} Telegram private DM topic '{name}'") return str(thread_id) @@ -257,251 +148,154 @@ class DeliveryRouter: """Resolves delivery targets and dispatches messages to platform adapters.""" def __init__(self, config: GatewayConfig, adapters: Dict[Platform, Any] = None, - dead_targets: Optional[DeadTargetRegistry] = None): - """``dead_targets``: shared registry of confirmed-unreachable targets; - a profile-local registry is created when omitted.""" + dead_targets: Optional[DeadTargetRegistry] = None): # profile-local registry when omitted self.config = config self.adapters = adapters or {} self.output_dir = get_hermes_home() / "cron" / "output" self.dead_targets = dead_targets or DeadTargetRegistry() - async def deliver( - self, - content: str, - targets: List[DeliveryTarget], - job_id: Optional[str] = None, - job_name: Optional[str] = None, - metadata: Optional[Dict[str, Any]] = None - ) -> Dict[str, Any]: + async def deliver(self, content: str, targets: List[DeliveryTarget], job_id: Optional[str] = None, + job_name: Optional[str] = None, metadata: Optional[Dict[str, Any]] = None) -> Dict[str, Any]: """Deliver content to all targets; returns per-target results keyed by target string.""" results = {} - for target in targets: - # Skip targets proven permanently unreachable (deleted group, blocked - # bot, deactivated user) — re-sending each tick wastes flood-control - # budget. Self-healing: a later successful send clears the flag. - # LOCAL/origin-without-chat targets are never dead-tracked. + # Skip targets proven permanently unreachable (deleted group, blocked bot, deactivated user) — + # re-sending each tick wastes flood-control budget. Self-healing: a later successful send + # clears the flag. LOCAL/origin-without-chat targets are never dead-tracked. tracked = target.platform != Platform.LOCAL and target.chat_id if tracked and self.dead_targets.is_dead(target.platform.value, target.chat_id): - logger.info( - "Skipping delivery to known-dead target %s:%s " - "(send to it again to clear)", - target.platform.value, target.chat_id, - ) - results[target.to_string()] = { - "success": False, - "skipped": "dead_target", - "error": "target previously confirmed unreachable", - } + logger.info("Skipping delivery to known-dead target %s:%s (send to it again to clear)", + target.platform.value, target.chat_id) + results[target.to_string()] = {"success": False, "skipped": "dead_target", + "error": "target previously confirmed unreachable"} continue try: if target.platform == Platform.LOCAL: result = self._deliver_local(content, job_id, job_name, metadata) else: result = await self._deliver_to_platform(target, content, metadata) - if target.chat_id and not _send_result_failed(result): + if target.chat_id and _send_result_error(result) is None: self.dead_targets.clear(target.platform.value, target.chat_id) results[target.to_string()] = {"success": True, "result": result} except Exception as e: - # Hard failures raise. Record a whole-chat death so future - # deliveries short-circuit. - if tracked: - dead_kind = _classify_dead_from_error_text(str(e)) - if dead_kind: - self.dead_targets.mark_dead( - target.platform.value, target.chat_id, - reason=f"{dead_kind}: {str(e)[:120]}", - ) + # Hard failures raise. Record a whole-chat death so future deliveries short-circuit. + dead_kind = classify_dead_error(str(e)) if tracked else None + if dead_kind: + self.dead_targets.mark_dead(target.platform.value, target.chat_id, + reason=f"{dead_kind}: {str(e)[:120]}") results[target.to_string()] = {"success": False, "error": str(e)} - return results - def _deliver_local( - self, - content: str, - job_id: Optional[str], - job_name: Optional[str], - metadata: Optional[Dict[str, Any]] - ) -> Dict[str, Any]: + def _deliver_local(self, content: str, job_id: Optional[str], job_name: Optional[str], + metadata: Optional[Dict[str, Any]]) -> Dict[str, Any]: """Save content to local files.""" timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") output_path = self.output_dir / (job_id or "misc") / f"{timestamp}.md" output_path.parent.mkdir(parents=True, exist_ok=True) - - lines = [ - f"# {job_name}" if job_name else "# Delivery Output", - "", - f"**Timestamp:** {datetime.now().strftime('%Y-%m-%d %H:%M:%S')}", - ] - if job_id: - lines.append(f"**Job ID:** {job_id}") - for key, value in (metadata or {}).items(): - lines.append(f"**{key}:** {value}") - lines += ["", "---", "", content] - + lines = [f"# {job_name}" if job_name else "# Delivery Output", "", + f"**Timestamp:** {datetime.now().strftime('%Y-%m-%d %H:%M:%S')}"] + lines += [f"**Job ID:** {job_id}"] if job_id else [] + lines += [f"**{key}:** {value}" for key, value in (metadata or {}).items()] + ["", "---", "", content] output_path.write_text("\n".join(lines), encoding="utf-8") return {"path": str(output_path), "timestamp": timestamp} def _save_full_output(self, content: str, job_id: str) -> Path: """Save full cron output to disk and return the file path.""" timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") - out_dir = get_hermes_home() / "cron" / "output" - out_dir.mkdir(parents=True, exist_ok=True) - path = out_dir / f"{job_id}_{timestamp}.txt" + path = get_hermes_home() / "cron" / "output" / f"{job_id}_{timestamp}.txt" + path.parent.mkdir(parents=True, exist_ok=True) path.write_text(content, encoding="utf-8") return path def _filter_silence_narration_enabled(self) -> bool: - """``HERMES_FILTER_SILENCE_NARRATION`` env overrides the - ``gateway.filter_silence_narration`` config flag (default True).""" + """``HERMES_FILTER_SILENCE_NARRATION`` env overrides the ``gateway.filter_silence_narration`` flag.""" env = os.getenv("HERMES_FILTER_SILENCE_NARRATION") - if env is not None: - return env.strip().lower() in ("1", "true", "yes", "on") - return bool(getattr(self.config, "filter_silence_narration", True)) + return (bool(getattr(self.config, "filter_silence_narration", True)) if env is None + else env.strip().lower() in ("1", "true", "yes", "on")) def _cap_oversized_output(self, adapter: Any, content: str, job_id: str) -> str: - """Audit-save oversized cron output; truncate it for non-chunking adapters. - - Two independent decisions: (1) above MAX_PLATFORM_OUTPUT the full output - is always written to disk as an audit trail, best-effort — a failed save - (full disk, permissions) never blocks delivery; (2) non-chunking adapters - get the content truncated with a footer pointing to the saved file, while - ``splits_long_messages`` adapters receive the full payload. - """ + """Audit-save oversized cron output; truncate it for non-chunking adapters. Above MAX_PLATFORM_OUTPUT + the full output is always written to disk as an audit trail, best-effort — a failed save (full disk, + permissions) never blocks delivery. Non-chunking adapters then get the content truncated with a + footer pointing to the saved file; ``splits_long_messages`` adapters receive the full payload.""" if len(content) <= MAX_PLATFORM_OUTPUT: return content saved_path: Optional[Path] = None try: saved_path = self._save_full_output(content, job_id) except OSError as exc: - logger.warning( - "Audit save failed for cron output (%d chars, job=%s): %s — " - "delivery proceeds without audit copy", - len(content), job_id, exc, - ) - + logger.warning("Audit save failed for cron output (%d chars, job=%s): %s — " + "delivery proceeds without audit copy", len(content), job_id, exc) if getattr(adapter, "splits_long_messages", False): if saved_path: - logger.info( - "Cron output preserved for chunking adapter (%d chars) — " - "full output saved to %s", - len(content), saved_path, - ) + logger.info("Cron output preserved for chunking adapter (%d chars) — " + "full output saved to %s", len(content), saved_path) return content - # The footer needs a valid path: if the best-effort save failed, retry # (a failure now is a real delivery problem and propagates). - if saved_path is None: - saved_path = self._save_full_output(content, job_id) + saved_path = saved_path or self._save_full_output(content, job_id) footer = f"\n\n... [truncated, full output saved to {saved_path}]" - visible = max(0, MAX_PLATFORM_OUTPUT - len(footer)) - logger.info( - "Cron output truncated (%d chars) — full output: %s", - len(content), saved_path, - ) - return content[:visible] + footer + logger.info("Cron output truncated (%d chars) — full output: %s", len(content), saved_path) + return content[:max(0, MAX_PLATFORM_OUTPUT - len(footer))] + footer - async def _deliver_to_platform( - self, - target: DeliveryTarget, - content: str, - metadata: Optional[Dict[str, Any]] - ) -> Dict[str, Any]: + async def _deliver_to_platform(self, target: DeliveryTarget, content: str, + metadata: Optional[Dict[str, Any]]) -> Dict[str, Any]: """Deliver content to a messaging platform.""" transport = resolve_delivery_transport(target.platform, self.config, self.adapters) if transport is None: raise ValueError(f"No adapter configured for {target.platform.value}") - adapter = transport.adapter - if not target.chat_id: raise ValueError(f"No chat ID for {target.platform.value} delivery") + adapter = transport.adapter + content = self._cap_oversized_output(adapter, content, (metadata or {}).get("job_id", "unknown")) - content = self._cap_oversized_output( - adapter, content, (metadata or {}).get("job_id", "unknown") - ) - - # Substrate-level anti-loop guard: drop hallucinated "silence narration" - # (*(silent)*, 🔇, a bare ".") before it reaches any adapter — in - # bot-to-bot channels these mirror back and forth until a model crashes - # with "no content after all retries"; prompt rules drift across - # providers, so this single chokepoint covers every platform. Local/file - # delivery is never filtered (saved silence has no loop risk). - # Cron output is an ARTIFACT, not model chatter: a legitimately terse job - # ("...", a single 🔇) has no mirror loop, and dropping it while returning - # success is how a cron gets logged as delivered with nothing on the - # wire. Cron sends carry job_id in metadata; everything else is filtered. + # Substrate-level anti-loop guard: drop hallucinated "silence narration" (*(silent)*, 🔇, a bare ".") + # before it reaches any adapter — in bot-to-bot channels these mirror back and forth until a model + # crashes with "no content after all retries"; prompt rules drift across providers, so this single + # chokepoint covers every platform. Local/file delivery is never filtered (saved silence has no loop + # risk). Cron output is an ARTIFACT, not model chatter: a legitimately terse job ("...", a single 🔇) + # has no mirror loop, and dropping it while returning success is how a cron gets logged as delivered + # with nothing on the wire. Cron sends carry job_id in metadata; everything else is filtered. is_cron_artifact = "job_id" in (metadata or {}) - if ( - self._filter_silence_narration_enabled() - and not is_cron_artifact - and _is_silence_narration(content) - ): - logger.warning( - "Dropped silence-narration outbound to %s (chat=%s): %r", - target.platform.value, - target.chat_id, - content[:40], - ) - return { - "success": True, - "filtered": "silence_narration", - "delivered": False, - } + if self._filter_silence_narration_enabled() and not is_cron_artifact and _is_silence_narration(content): + logger.warning("Dropped silence-narration outbound to %s (chat=%s): %r", + target.platform.value, target.chat_id, content[:40]) + return {"success": True, "filtered": "silence_narration", "delivered": False} send_metadata = dict(metadata or {}) - if transport.is_relay: - home = self.config.get_home_channel(target.platform) - if home is not None and home.chat_id == target.chat_id: - if home.user_id: - send_metadata["user_id"] = home.user_id - if home.scope_id: - send_metadata["scope_id"] = home.scope_id + home = self.config.get_home_channel(target.platform) if transport.is_relay else None + if home is not None and home.chat_id == target.chat_id: + send_metadata.update({k: v for k, v in (("user_id", home.user_id), ("scope_id", home.scope_id)) if v}) # Caller-supplied thread routing always wins over target.thread_id. - named_telegram_private_topic_name: Optional[str] = None + named_topic: Optional[str] = None # named Telegram private topic created for this send thread_id = target.thread_id - thread_unrouted = thread_id and not any( - key in send_metadata - for key in ("thread_id", "message_thread_id", - "direct_messages_topic_id", "telegram_direct_messages_topic_id") - ) - if thread_unrouted: - is_telegram_private = ( - target.platform == Platform.TELEGRAM - and looks_like_telegram_private_chat_id(target.chat_id) - ) - if is_telegram_private and not _looks_like_int(thread_id): - # Named topic: create via createForumTopic, use message_thread_id directly. - named_telegram_private_topic_name = thread_id - send_metadata["thread_id"] = await _ensure_named_dm_topic( - adapter, target.chat_id, thread_id, refresh=False - ) - send_metadata["telegram_dm_topic_created_for_send"] = True - elif is_telegram_private: - # Legacy numeric private topic ids not created by this send path - # need a reply anchor to stay visible in the requested lane. - if send_metadata.get("telegram_reply_to_message_id") is None: + if thread_id and not any(key in send_metadata for key in _THREAD_ROUTING_KEYS): + send_metadata["thread_id"] = thread_id + if target.platform == Platform.TELEGRAM and looks_like_telegram_private_chat_id(target.chat_id): + if not _looks_like_int(thread_id): + # Named topic: create via createForumTopic, use message_thread_id directly. + named_topic = thread_id + send_metadata["thread_id"] = await _ensure_named_dm_topic(adapter, target.chat_id, thread_id, refresh=False) + send_metadata["telegram_dm_topic_created_for_send"] = True + elif send_metadata.get("telegram_reply_to_message_id") is None: + # Legacy numeric private topic ids not created by this send path need a reply + # anchor to stay visible in the requested lane. raise RuntimeError( "Telegram private DM topic delivery requires telegram_reply_to_message_id; " "send to the bare chat or provide a reply anchor" ) - send_metadata["thread_id"] = thread_id - send_metadata["telegram_dm_topic_reply_fallback"] = True - else: - send_metadata["thread_id"] = thread_id + else: + send_metadata["telegram_dm_topic_reply_fallback"] = True - result = await transport.send( - target.platform, target.chat_id, content, metadata=send_metadata or None, - ) - if _send_result_failed(result): - if named_telegram_private_topic_name and _is_thread_not_found_delivery_error(result): - send_metadata["thread_id"] = await _ensure_named_dm_topic( - adapter, target.chat_id, named_telegram_private_topic_name, refresh=True - ) - send_metadata["telegram_dm_topic_created_for_send"] = True - result = await transport.send( - target.platform, target.chat_id, content, metadata=send_metadata or None, - ) - if _send_result_failed(result): - raise RuntimeError(_send_result_error(result) or f"{target.platform.value} delivery failed") + for retry in (False, True): + result = await transport.send(target.platform, target.chat_id, content, metadata=send_metadata or None) + error = _send_result_error(result) + if retry or error is None or not named_topic or "thread not found" not in error.lower(): + break + # The named topic vanished under us: recreate it once and resend. + send_metadata["thread_id"] = await _ensure_named_dm_topic(adapter, target.chat_id, named_topic, refresh=True) + send_metadata["telegram_dm_topic_created_for_send"] = True + if error is not None: + raise RuntimeError(error or f"{target.platform.value} delivery failed") return result diff --git a/gateway/delivery_ledger.py b/gateway/delivery_ledger.py index 7d5fa26d64..fac8e2b219 100644 --- a/gateway/delivery_ledger.py +++ b/gateway/delivery_ledger.py @@ -1,37 +1,13 @@ -"""Durable delivery-obligation ledger for gateway final responses. - -A final agent response generated but not yet confirmed-delivered is the one -artifact the gateway can lose without a trace: the turn already burned its -tokens, the text exists only in a Python local, and a crash / restart between -finalize and platform ACK drops it silently. - -Each outbound final response gets a small durable row in the shared ``state.db`` -(same conventions as ``tools.async_delegation`` — WAL, owner pid + process-start -liveness, bounded retention). The gateway writes three checkpoints: - - record_obligation() state='pending' before any send attempt - mark_attempting() state='attempting' immediately before the await - mark_delivered() / state='delivered' only on SendResult.success - mark_failed() state='failed' on a definitive rejection - -On startup ``sweep_recoverable()`` claims rows whose owner is dead and hands -them back for redelivery. After an adapter reconnects without a restart, -``sweep_failed_for_runtime()`` may claim only the same live process's -allowlisted transient failures. Crash semantics are explicit about ambiguity -(never silently resend an ambiguous send): - -- ``pending`` — send never started: redeliver plainly, no dup risk. -- ``attempting`` — crashed mid-await: the platform MAY already have it. - Redelivered WITH a visible recovered-reply marker (honest at-least-once). -- ``failed`` — definitively rejected once; restart is a natural retry - boundary. Also carries the marker. -- ``delivered`` — nothing to do; retention prunes. - -Poison rows cannot spin: attempts are capped and stale rows expire, both -transitioning to ``abandoned`` (kept briefly, then pruned). - -Everything here is best-effort: ledger failures must never block or delay an -actual send. Callers wrap every call in try/except. +"""Durable delivery-obligation ledger for gateway final responses (rows in the shared ``state.db``; +WAL, owner pid + process-start liveness, bounded retention) so a crash between finalize and +platform ACK cannot lose a response silently. Checkpoints: record_obligation() 'pending' before +any send | mark_attempting() 'attempting' right before the await | mark_delivered() 'delivered' +only on SendResult.success | mark_failed() 'failed' on a definitive rejection. Crash semantics +(never silently resend an ambiguous send): pending = never started, redeliver plainly; attempting += crashed mid-await, platform MAY have it, redeliver WITH a visible recovered marker; failed = +rejected once, restart is a retry boundary, also marked; delivered = prune. Attempts are capped +and stale rows expire, both -> 'abandoned' (kept briefly, then pruned). Everything is +best-effort: ledger failures must never block a send; callers wrap every call in try/except. """ from __future__ import annotations @@ -42,13 +18,12 @@ import os import sqlite3 import threading import time -from contextlib import contextmanager +from contextlib import closing, contextmanager from typing import Any, Dict, Iterator, List, Optional from hermes_constants import get_hermes_home logger = logging.getLogger(__name__) - _DB_LOCK = threading.Lock() # Redelivery policy knobs (deliberately not config — the ledger is gated by @@ -58,23 +33,15 @@ STALE_AFTER_SECONDS = 24 * 60 * 60 _RETENTION_SECONDS = 7 * 24 * 60 * 60 _MAX_ROWS = 500 -# Visible prefix for redeliveries that might duplicate an already-received -# message (crash mid-send / post-rejection retry). Honest at-least-once. -RECOVERED_MARKER = ( - "♻️ Recovered reply — the gateway restarted during delivery, " - "so this may be a duplicate:\n\n" -) +# Visible prefixes for redeliveries that might duplicate an already-received message (crash mid-send / +# post-rejection retry) — honest at-least-once. Runtime recovery uses a distinct marker: no restart +# occurred, but a network rejection's acknowledgement can still have been lost independently. +RECOVERED_MARKER = "♻️ Recovered reply — the gateway restarted during delivery, so this may be a duplicate:\n\n" +RECONNECTED_MARKER = ("♻️ Recovered reply — the messaging platform reconnected after the original " + "delivery failed, so this may be a duplicate:\n\n") -# Runtime recovery uses a distinct marker: no restart occurred, but a network -# rejection's acknowledgement can still have been lost independently. -RECONNECTED_MARKER = ( - "♻️ Recovered reply — the messaging platform reconnected after the original " - "delivery failed, so this may be a duplicate:\n\n" -) - -# Runtime replay is fail-closed: only errors whose send contract proves they are -# transient reconnect failures. Permanent rejects (blocked bot, bad auth, missing -# chat) must not be retried merely because an adapter reconnected. +# Runtime replay is fail-closed: only errors whose send contract proves they are transient reconnect +# failures. Permanent rejects (blocked bot, bad auth, missing chat) must not be retried on reconnect. _RUNTIME_RETRYABLE_ERRORS = frozenset({"send_path_degraded"}) @@ -89,15 +56,13 @@ def _connect() -> sqlite3.Connection: try: _initialize_schema(conn) except Exception: - # A PRAGMA/DDL failure after connect() must not leak the connection. - conn.close() + conn.close() # a PRAGMA/DDL failure after connect() must not leak the connection raise return conn def _initialize_schema(conn: sqlite3.Connection) -> None: from hermes_state import apply_wal_with_fallback - apply_wal_with_fallback(conn, db_label="state.db (delivery_ledger)") conn.execute( """CREATE TABLE IF NOT EXISTS delivery_obligations ( @@ -117,8 +82,7 @@ def _initialize_schema(conn: sqlite3.Connection) -> None: adapter_profile TEXT )""" ) - columns = {row[1] for row in conn.execute("PRAGMA table_info(delivery_obligations)")} - if "adapter_profile" not in columns: + if "adapter_profile" not in {row[1] for row in conn.execute("PRAGMA table_info(delivery_obligations)")}: try: conn.execute("ALTER TABLE delivery_obligations ADD COLUMN adapter_profile TEXT") except sqlite3.OperationalError as exc: @@ -129,29 +93,25 @@ def _initialize_schema(conn: sqlite3.Connection) -> None: @contextmanager def _transaction() -> Iterator[sqlite3.Connection]: - """Open a connection, commit/rollback on exit, and ALWAYS close it. - - ``sqlite3.Connection`` as a context manager only commits/rolls back; it does - not close. ``with _connect()`` alone would leak a connection (and its WAL/SHM - fds) per call until GC — ``record_obligation`` runs on every outbound final - response, so this ledger would exhaust ``RLIMIT_NOFILE`` on a long-running gateway. - """ + """Open a connection, commit/rollback on exit, and ALWAYS close it: ``sqlite3.Connection`` as a + context manager only commits/rolls back, so ``with _connect()`` alone leaks a connection (and its + WAL/SHM fds) per call — ``record_obligation`` runs on every final response; exhausts RLIMIT_NOFILE.""" conn = _connect() + with closing(conn), conn: + yield conn + + +def _start_time(pid: int) -> Optional[int]: try: - with conn: - yield conn - finally: - conn.close() + from gateway.status import get_process_start_time # lazy: tests monkeypatch gateway.status + return get_process_start_time(pid) + except Exception: + return None def _owner_stamp() -> tuple[int, Optional[int]]: pid = os.getpid() - try: - from gateway.status import get_process_start_time - - return pid, get_process_start_time(pid) - except Exception: - return pid, None + return pid, _start_time(pid) def _owner_alive(pid: Any, started_at: Any) -> bool: @@ -162,18 +122,11 @@ def _owner_alive(pid: Any, started_at: Any) -> bool: pid = int(pid) except (TypeError, ValueError): return False - try: - from gateway.status import get_process_start_time - - current_start = get_process_start_time(pid) - except Exception: - current_start = None + current_start = _start_time(pid) if current_start is None: - # Start time unreadable: alive iff the pid exists. Route through the - # cross-platform probe — ``os.kill(pid, 0)`` on Windows is NOT a no-op - # (bpo-14484: it maps to ``GenerateConsoleCtrlEvent(0, pid)`` and could - # Ctrl+C the gateway's own console group). ``_pid_exists`` keeps the - # EPERM-means-alive semantics (pid exists but is owned by another user). + # Start time unreadable: alive iff the pid exists. Route through the cross-platform probe — on Windows + # ``os.kill(pid, 0)`` is NOT a no-op (bpo-14484: it maps to ``GenerateConsoleCtrlEvent(0, pid)`` and could + # Ctrl+C the gateway's own console group). ``_pid_exists`` keeps EPERM-means-alive (pid owned by another user). try: from gateway.status import _pid_exists except Exception: @@ -181,47 +134,29 @@ def _owner_alive(pid: Any, started_at: Any) -> bool: return False # never fall back to a raw sig-0 probe on Windows try: os.kill(pid, 0) # windows-footgun: ok — POSIX-only fallback branch - except ProcessLookupError: - return False - except PermissionError: return True - except OSError: - return False - return True + except OSError as exc: # incl. ProcessLookupError; EPERM means the pid exists + return isinstance(exc, PermissionError) try: return bool(_pid_exists(pid)) except Exception: return False - if started_at is None: - return True try: - return int(current_start) == int(started_at) + return started_at is None or int(current_start) == int(started_at) except (TypeError, ValueError): return True def compute_obligation_id(session_key: str, message_ref: str, content: str) -> str: - """Stable id: same turn + same content re-records idempotently, while distinct - threads/topics on one chat never collide (session_key carries platform, chat and - thread; ``message_ref`` is the triggering inbound message id).""" - payload = f"{session_key}|{message_ref}|{content}" - return hashlib.sha256(payload.encode("utf-8", "replace")).hexdigest()[:24] + """Stable id: same turn + same content re-records idempotently, while distinct threads/topics on one + chat never collide (session_key carries platform/chat/thread; ``message_ref`` = inbound message id).""" + return hashlib.sha256(f"{session_key}|{message_ref}|{content}".encode("utf-8", "replace")).hexdigest()[:24] -def record_obligation( - *, - obligation_id: str, - session_key: str, - platform: str, - chat_id: str, - thread_id: Optional[str], - content: str, - adapter_profile: Optional[str] = None, -) -> None: +def record_obligation(*, obligation_id: str, session_key: str, platform: str, chat_id: str, + thread_id: Optional[str], content: str, adapter_profile: Optional[str] = None) -> None: """Record a final response as owed to the platform (state='pending').""" - now = time.time() - stored_profile = str(adapter_profile).strip() if adapter_profile else "default" - pid, started = _owner_stamp() + now, (pid, started) = time.time(), _owner_stamp() with _DB_LOCK, _transaction() as conn: conn.execute( """INSERT OR REPLACE INTO delivery_obligations @@ -229,10 +164,8 @@ def record_obligation( content, state, attempts, created_at, updated_at, owner_pid, owner_started_at, adapter_profile) VALUES (?, ?, ?, ?, ?, ?, 'pending', 0, ?, ?, ?, ?, ?)""", - (obligation_id, session_key, platform, str(chat_id), - str(thread_id) if thread_id else None, content, now, now, - pid, started, stored_profile), - ) + (obligation_id, session_key, platform, str(chat_id), str(thread_id) if thread_id else None, + content, now, now, pid, started, str(adapter_profile).strip() if adapter_profile else "default")) _prune() @@ -251,11 +184,9 @@ def mark_failed(obligation_id: str, error: str = "") -> None: def release_runtime_claim(obligation_id: str, error: str = "") -> bool: """Return an unsent runtime claim to ``failed`` without spending an attempt. - Runtime recovery claims before clearing ``resume_pending`` so two reconnect - paths cannot send the same row. If the session flag cannot be cleared, no - send was attempted and the claim must not consume the redelivery budget. - Fail-closed to the exact current process instance and ``attempting`` state. - """ + Runtime recovery claims before clearing ``resume_pending`` so two reconnect paths cannot send the + same row; if the flag cannot be cleared no send was attempted and the claim must not consume the + redelivery budget. Fail-closed to the exact current process instance and ``attempting`` state.""" pid, started = _owner_stamp() if started is None: return False @@ -267,9 +198,7 @@ def release_runtime_claim(obligation_id: str, error: str = "") -> bool: updated_at=?, last_error=? WHERE obligation_id=? AND state='attempting' AND owner_pid IS ? AND owner_started_at IS ?""", - (time.time(), error[:500] if error else None, - obligation_id, pid, started), - ) + (time.time(), error[:500] if error else None, obligation_id, pid, started)) return bool(cursor.rowcount) @@ -279,39 +208,31 @@ def _update_state(obligation_id: str, state: str, error: str = "") -> None: """UPDATE delivery_obligations SET state=?, updated_at=?, last_error=? WHERE obligation_id=?""", - (state, time.time(), error[:500] if error else None, obligation_id), - ) + (state, time.time(), error[:500] if error else None, obligation_id)) -def _exhausted(attempts: int, created_at: float, now: float) -> bool: - return attempts >= MAX_ATTEMPTS or (now - created_at) > STALE_AFTER_SECONDS +def _claimed_row(oid, session_key, platform, chat_id, thread_id, content, attempts, profile, *, + needs_marker: bool, runtime: bool = False) -> Dict[str, Any]: + """Claimed-row dict handed back for redelivery; ``runtime`` adds the reconnect-marker fields.""" + return {"obligation_id": oid, "session_key": session_key, "platform": platform, "chat_id": chat_id, + "thread_id": thread_id, "content": content, "needs_marker": needs_marker, + **({"marker": RECONNECTED_MARKER} if runtime else {}), "profile": profile, + **({"runtime_recovery": True} if runtime else {}), "attempts": attempts + 1} -def sweep_recoverable( - now: Optional[float] = None, - *, - deliverable_platforms: Optional[set] = None, - deliverable_targets: Optional[set] = None, -) -> List[Dict[str, Any]]: +def sweep_recoverable(now: Optional[float] = None, *, deliverable_platforms: Optional[set] = None, + deliverable_targets: Optional[set] = None) -> List[Dict[str, Any]]: """Claim undelivered rows owned by dead processes; return them for redelivery. - Claiming atomically re-stamps the owner to THIS process and increments - ``attempts`` (the UPDATE is guarded on the previous owner stamp, so a second - gateway racing the same sweep cannot double-claim). Rows over the attempts - cap or stale cutoff transition to 'abandoned' instead of being returned. - - ``deliverable_platforms`` restricts claiming to platforms the caller can - actually send on this boot: ``attempts`` is the redelivery budget and must - only be spent on a real send, else a platform that failed to connect burns - one attempt per boot and hits the cap having never been sent once. Rows for - absent platforms are left untouched (the stale cutoff still bounds them). - - ``deliverable_targets`` further scopes multiplexed gateways by exact - ``(platform, adapter_profile)`` so one connected bot cannot spend another - disconnected bot's retry budget. - """ - now = now if now is not None else time.time() - pid, started = _owner_stamp() + Claiming atomically re-stamps the owner to THIS process and increments ``attempts`` (the UPDATE is + guarded on the previous owner stamp, so a second gateway racing the same sweep cannot double-claim). + Rows over the attempts cap or stale cutoff become 'abandoned'. ``deliverable_platforms`` restricts + claiming to platforms the caller can send on this boot: ``attempts`` is the redelivery budget and + must only be spent on a real send, else a platform that failed to connect burns one attempt per boot + and hits the cap having never been sent once (the stale cutoff still bounds untouched rows). + ``deliverable_targets`` further scopes multiplexed gateways by exact ``(platform, adapter_profile)`` + so one connected bot cannot spend another disconnected bot's retry budget.""" + now, (pid, started) = now if now is not None else time.time(), _owner_stamp() claimed: List[Dict[str, Any]] = [] with _DB_LOCK, _transaction() as conn: rows = conn.execute( @@ -321,73 +242,45 @@ def sweep_recoverable( FROM delivery_obligations WHERE state IN ('pending', 'attempting', 'failed')""" ).fetchall() - for (oid, session_key, platform, chat_id, thread_id, content, state, - attempts, created_at, owner_pid, owner_started_at, - adapter_profile) in rows: + for (oid, session_key, platform, chat_id, thread_id, content, state, attempts, created_at, + owner_pid, owner_started_at, adapter_profile) in rows: if _owner_alive(owner_pid, owner_started_at): continue # a live gateway still owns this row - if _exhausted(attempts, created_at, now): + if attempts >= MAX_ATTEMPTS or (now - created_at) > STALE_AFTER_SECONDS: # exhausted -> abandoned conn.execute( """UPDATE delivery_obligations - SET state='abandoned', updated_at=? WHERE obligation_id=?""", - (now, oid), - ) + SET state='abandoned', updated_at=? WHERE obligation_id=?""", (now, oid)) continue - if deliverable_platforms is not None and platform not in deliverable_platforms: + if ((deliverable_platforms is not None and platform not in deliverable_platforms) + or (deliverable_targets is not None and (platform, adapter_profile) not in deliverable_targets)): continue # no adapter this boot — claiming would spend an attempt on a no-op - if deliverable_targets is not None and (platform, adapter_profile) not in deliverable_targets: - continue cursor = conn.execute( """UPDATE delivery_obligations SET owner_pid=?, owner_started_at=?, attempts=attempts+1, updated_at=? WHERE obligation_id=? AND (owner_pid IS ? OR owner_pid=?)""", - (pid, started, now, oid, owner_pid, owner_pid), - ) - if cursor.rowcount: - claimed.append({ - "obligation_id": oid, - "session_key": session_key, - "platform": platform, - "chat_id": chat_id, - "thread_id": thread_id, - "content": content, - # pending = never started, redeliver plainly; else carry marker. - "needs_marker": state != "pending", - "profile": adapter_profile, - "attempts": attempts + 1, - }) + (pid, started, now, oid, owner_pid, owner_pid)) + if cursor.rowcount: # pending = never started, redeliver plainly; else carry marker + claimed.append(_claimed_row(oid, session_key, platform, chat_id, thread_id, content, attempts, + adapter_profile, needs_marker=state != "pending")) return claimed -def sweep_failed_for_runtime( - platform: str, - now: Optional[float] = None, - *, - profile: Optional[str] = None, -) -> List[Dict[str, Any]]: +def sweep_failed_for_runtime(platform: str, now: Optional[float] = None, *, + profile: Optional[str] = None) -> List[Dict[str, Any]]: """Claim this process's reconnect-retryable failed rows for one adapter. - ``profile`` scopes multiplexed gateways to the bot identity that owned the - failed send; ``None`` means the primary/default adapter. The persisted - adapter owner is independent of the routed session namespace. Unowned rows - and rows owned by another process are left for the startup/dead-owner sweep. - - Startup recovery ignores rows owned by a live gateway, so a final response - rejected with ``send_path_degraded`` would stay stranded when only the - adapter reconnects. This sweep closes that gap without weakening ownership: - only rows stamped to this exact process instance, only allowlisted transient - errors, same attempts/staleness bounds, every update guarded by the prior - owner stamp and ``failed`` state. Claimed rows always carry the reconnect - marker because the failed send's acknowledgement is not safe to infer. - """ - now = now if now is not None else time.time() - pid, started = _owner_stamp() - if started is None: - # PID alone cannot distinguish this process from a stale row left after - # PID reuse. Runtime replay is optional, so fail closed; startup - # recovery remains the durable fallback. - return [] + ``profile`` scopes multiplexed gateways to the bot identity that owned the failed send (``None`` = + primary/default adapter); unowned rows and rows owned by another process are left for the + startup/dead-owner sweep. Startup recovery ignores rows owned by a live gateway, so a response + rejected with ``send_path_degraded`` would stay stranded when only the adapter reconnects; this closes + that gap without weakening ownership: only rows stamped to this exact process instance, only + allowlisted transient errors, same attempts/staleness bounds, every update guarded by the prior owner + stamp and ``failed`` state. Claimed rows always carry the reconnect marker (the failed send's ack is + not safe to infer).""" + now, (pid, started) = now if now is not None else time.time(), _owner_stamp() + if started is None: # PID alone cannot distinguish this process from a stale row left after PID + return [] # reuse; runtime replay is optional, so fail closed (startup recovery remains). expected_profile = "default" if not profile or profile == "default" else str(profile) claimed: List[Dict[str, Any]] = [] with _DB_LOCK, _transaction() as conn: @@ -396,66 +289,41 @@ def sweep_failed_for_runtime( content, attempts, created_at, owner_pid, owner_started_at, last_error, adapter_profile FROM delivery_obligations - WHERE state='failed' AND platform=?""", - (platform,), - ).fetchall() - for (oid, session_key, row_platform, chat_id, thread_id, content, - attempts, created_at, owner_pid, owner_started_at, last_error, - adapter_profile) in rows: - if adapter_profile != expected_profile: - continue + WHERE state='failed' AND platform=?""", (platform,)).fetchall() + for (oid, session_key, row_platform, chat_id, thread_id, content, attempts, created_at, + owner_pid, owner_started_at, last_error, adapter_profile) in rows: # Exact process-start matching prevents PID reuse from stealing work. - if owner_pid != pid or owner_started_at != started: + if (adapter_profile != expected_profile or owner_pid != pid or owner_started_at != started + or str(last_error or "").strip().lower() not in _RUNTIME_RETRYABLE_ERRORS): continue - if str(last_error or "").strip().lower() not in _RUNTIME_RETRYABLE_ERRORS: - continue - owner_guard = (oid, owner_pid, owner_started_at) - if _exhausted(attempts, created_at, now): + owner_guard = (now, oid, owner_pid, owner_started_at) + if attempts >= MAX_ATTEMPTS or (now - created_at) > STALE_AFTER_SECONDS: # exhausted -> abandoned conn.execute( """UPDATE delivery_obligations SET state='abandoned', updated_at=? WHERE obligation_id=? AND state='failed' - AND owner_pid IS ? AND owner_started_at IS ?""", - (now, *owner_guard), - ) + AND owner_pid IS ? AND owner_started_at IS ?""", owner_guard) continue cursor = conn.execute( """UPDATE delivery_obligations SET state='attempting', attempts=attempts+1, updated_at=? WHERE obligation_id=? AND state='failed' - AND owner_pid IS ? AND owner_started_at IS ?""", - (now, *owner_guard), - ) + AND owner_pid IS ? AND owner_started_at IS ?""", owner_guard) if cursor.rowcount: - claimed.append({ - "obligation_id": oid, - "session_key": session_key, - "platform": row_platform, - "chat_id": chat_id, - "thread_id": thread_id, - "content": content, - "needs_marker": True, - "marker": RECONNECTED_MARKER, - "profile": adapter_profile, - "runtime_recovery": True, - "attempts": attempts + 1, - }) + claimed.append(_claimed_row(oid, session_key, row_platform, chat_id, thread_id, content, + attempts, adapter_profile, needs_marker=True, runtime=True)) return claimed def _prune(now: Optional[float] = None) -> None: now = now if now is not None else time.time() - cutoff = now - _RETENTION_SECONDS try: with _transaction() as conn: conn.execute( """DELETE FROM delivery_obligations - WHERE state IN ('delivered', 'abandoned') AND updated_at < ?""", - (cutoff,), - ) + WHERE state IN ('delivered', 'abandoned') AND updated_at < ?""", (now - _RETENTION_SECONDS,)) total = conn.execute("SELECT COUNT(*) FROM delivery_obligations").fetchone()[0] - excess = max(0, total - _MAX_ROWS) - if excess: + if total > _MAX_ROWS: conn.execute( """DELETE FROM delivery_obligations WHERE obligation_id IN ( SELECT obligation_id FROM delivery_obligations @@ -464,9 +332,7 @@ def _prune(now: Optional[float] = None) -> None: WHEN 'abandoned' THEN 1 ELSE 2 END, updated_at ASC - LIMIT ?)""", - (excess,), - ) + LIMIT ?)""", (total - _MAX_ROWS,)) except Exception: logger.debug("delivery ledger prune failed", exc_info=True) @@ -476,12 +342,8 @@ def ledger_enabled(config: Optional[Dict[str, Any]] = None) -> bool: try: if config is None: from hermes_cli.config import load_config - config = load_config() - gw = config.get("gateway") or {} - value = gw.get("delivery_ledger", True) - if isinstance(value, str): - return value.strip().lower() not in {"false", "0", "no", "off"} - return bool(value) + value = (config.get("gateway") or {}).get("delivery_ledger", True) + return value.strip().lower() not in {"false", "0", "no", "off"} if isinstance(value, str) else bool(value) except Exception: return True diff --git a/gateway/kanban_watchers.py b/gateway/kanban_watchers.py index 5929061f89..f38e6ee422 100644 --- a/gateway/kanban_watchers.py +++ b/gateway/kanban_watchers.py @@ -3,19 +3,27 @@ Background loops that subscribe to kanban boards, deliver notifications and artifacts, and drive the multi-agent dispatcher. They use only ``self`` state, so they live on a mixin ``GatewayRunner`` inherits. Per-tick work lives in -``kanban_watchers_notifier`` / ``kanban_watchers_dispatcher``. +``kanban_watchers_notifier`` / ``kanban_watchers_dispatcher``; shared plumbing +in ``kanban_watchers_common``. """ from __future__ import annotations import asyncio -import contextlib import os import time from pathlib import Path -from typing import Any, Callable, Optional +from typing import Any, Optional -from gateway.kanban_watchers_common import _to_thread_process_service, logger # noqa: F401 (tests import via origin) +from gateway.kanban_watchers_common import ( # noqa: F401 (tests import via origin) + _acquire_singleton_lock, + _kanban_dispatch_allowed, + _release_singleton_lock, + _resolve_auto_decompose_settings, + _gc_retention_days, + _to_thread_process_service, + logger, +) from gateway.kanban_watchers_notifier import ( # noqa: F401 (_wake_scope_id: tests import via origin) _KanbanNotification, _notifier_collect, @@ -27,100 +35,16 @@ from gateway.kanban_watchers_dispatcher import ( _resolve_dispatcher_settings, ) - -def _resolve_auto_decompose_settings( - load_config: Callable[[], Any], -) -> "tuple[bool, int]": - """Resolve the live (enabled, per_tick) auto-decompose settings. - - Read fresh every dispatcher tick so flipping ``kanban.auto_decompose: - false`` stops runaway fan-out on the next tick without a restart. Fails - safe: a config read error returns ``(False, 3)`` rather than re-enabling - a feature the user turned off. ``per_tick`` is clamped to ``>= 1``. - """ - try: - cfg = load_config() - except Exception: - return False, 3 - kcfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} - enabled = bool(kcfg.get("auto_decompose", True)) - try: - per_tick = int(kcfg.get("auto_decompose_per_tick", 3) or 3) - except (TypeError, ValueError): - per_tick = 3 - return enabled, max(per_tick, 1) - - -def _kanban_dispatch_allowed() -> bool: - """Return False while the global emergency stop (`hermes pause`) is engaged. - - Checked every tick before spawning, so a pause applies on the next tick; - in-flight workers are never touched. Fails open if estop is unimportable. - """ - try: - from agent.estop import check_paused - except ImportError: - return True - return not check_paused("kanban", logger) - - -def _acquire_singleton_lock(lock_path) -> "tuple[Optional[object], str]": - """Take the exclusive, non-blocking advisory lock for the sole dispatcher. - - Only one gateway machine-wide may run the embedded dispatcher: concurrent - dispatchers double reclaim frequency and claim events, and with - ``wal_autocheckpoint=0`` concurrent manual checkpoints can corrupt index - pages. ``dispatch_in_gateway`` is the primary control; this is the backstop. - - Returns ``(handle, "held")`` (caller must release via - :func:`_release_singleton_lock`), ``(None, "contended")`` when another - process holds it (caller must NOT dispatch), or ``(None, "unavailable")`` - when locking cannot be performed (caller falls back to config control). - """ - try: - from gateway.status import _try_acquire_file_lock # deferred; same package - except ImportError: - return None, "unavailable" - try: - Path(lock_path).parent.mkdir(parents=True, exist_ok=True) - handle = open(str(lock_path), "a+", encoding="utf-8") - except OSError: - return None, "unavailable" - if not _try_acquire_file_lock(handle): - handle.close() - return None, "contended" - return handle, "held" - - -def _release_singleton_lock(handle) -> None: - """Release a lock acquired via :func:`_acquire_singleton_lock`.""" - if handle is None: - return - try: - from gateway.status import _release_file_lock - _release_file_lock(handle) - except Exception: - pass - with contextlib.suppress(Exception): - handle.close() - - -def _gc_retention_days() -> int: - """``kanban.done_sub_retention_days`` (default 30; 0 disables), re-read per sweep; fails safe to 30.""" - try: - from hermes_cli.config import load_config as _load_cfg - - _kanban_cfg = (_load_cfg() or {}).get("kanban") or {} - return int(_kanban_cfg.get("done_sub_retention_days", 30)) - except Exception: - return 30 +_IMAGE_EXTS = {".png", ".jpg", ".jpeg", ".gif", ".webp"} +_VIDEO_EXTS = {".mp4", ".mov", ".avi", ".mkv", ".webm", ".3gp"} +_GC_INTERVAL_SECONDS = 3600.0 +_HEALTH_WINDOW = 6 class GatewayKanbanWatchersMixin: """Kanban watcher / notifier / dispatcher loops for GatewayRunner.""" def _owns_kanban_dispatcher_lock(self) -> bool: - """Return whether this gateway currently owns the singleton lock.""" return getattr(self, "_kanban_dispatcher_lock_handle", None) is not None def _release_kanban_dispatcher_lock(self) -> None: @@ -141,15 +65,12 @@ class GatewayKanbanWatchersMixin: """Poll ``kanban_notify_subs`` and deliver terminal events to users. Per subscription, claims ``task_events`` newer than the stored cursor - (kinds in TERMINAL_KINDS), sends one message per event to - ``(platform, chat_id, thread_id)``, then advances the cursor. The - subscription is removed only when the task is ``archived``: ``done`` - is reversible (review/continuation), so the cursor — not unsubscribing - — is the dedup mechanism. Earlier unsub-on-terminal silently dropped - users when the dispatcher respawned a crashed task. - - All SQLite work runs in a thread; one tick's failure never stops the - next. Iterates every board on disk per tick. + (kinds in TERMINAL_KINDS), sends one message per event, then advances + the cursor. The subscription is removed only when the task is + ``archived``: ``done`` is reversible, so the cursor — not unsubscribing + — is the dedup mechanism (unsub-on-terminal dropped users when the + dispatcher respawned a crashed task). All SQLite work runs in a thread; + one tick's failure never stops the next. """ from gateway.config import Platform as _Platform try: @@ -158,23 +79,18 @@ class GatewayKanbanWatchersMixin: logger.warning("kanban notifier: kanban_db not importable; notifier disabled") return - sub_fail_counts: dict[tuple, int] = getattr( - self, "_kanban_sub_fail_counts", {} - ) + sub_fail_counts: dict[tuple, int] = getattr(self, "_kanban_sub_fail_counts", {}) self._kanban_sub_fail_counts = sub_fail_counts - notifier_profile = getattr(self, "_kanban_notifier_profile", None) - if not notifier_profile: - notifier_profile = self._active_profile_name() - self._kanban_notifier_profile = notifier_profile + notifier_profile = getattr(self, "_kanban_notifier_profile", None) or self._active_profile_name() + self._kanban_notifier_profile = notifier_profile # Initial delay so the gateway can finish wiring adapters. await asyncio.sleep(5) # Stale done-sub GC: subs survive ``done``, so boards that never # archive would accumulate rows scanned every tick. One DELETE per - # board, at startup and at most hourly. - _GC_INTERVAL_SECONDS = 3600.0 - _gc_next_at = 0.0 # 0 → sweep on the first tick after startup + # board, at startup (0 → first tick) and at most hourly. + _gc_next_at = 0.0 while self._running: try: @@ -186,9 +102,7 @@ class GatewayKanbanWatchersMixin: deliveries = await asyncio.to_thread( _notifier_collect, self, _kb, - notifier_profile=notifier_profile, - gc_due=_gc_due, - gc_retention_days=_retention, + notifier_profile=notifier_profile, gc_due=_gc_due, gc_retention_days=_retention, ) for d in deliveries: await _KanbanNotification( @@ -204,47 +118,23 @@ class GatewayKanbanWatchersMixin: conn = _kb.connect(board=board) try: getattr(_kb, op)( - conn, - task_id=sub["task_id"], - platform=sub["platform"], - chat_id=sub["chat_id"], - thread_id=sub.get("thread_id") or "", - **extra, + conn, task_id=sub["task_id"], platform=sub["platform"], chat_id=sub["chat_id"], + thread_id=sub.get("thread_id") or "", **extra, ) finally: conn.close() - def _kanban_advance( - self, sub: dict, cursor: int, board: Optional[str] = None, - ) -> None: - """Advance a subscription's cursor on the board that owns it.""" + def _kanban_advance(self, sub: dict, cursor: int, board: Optional[str] = None) -> None: self._kanban_sub_op(board, "advance_notify_cursor", sub, new_cursor=cursor) def _kanban_unsub(self, sub: dict, board: Optional[str] = None) -> None: self._kanban_sub_op(board, "remove_notify_sub", sub) - def _kanban_rewind( - self, - sub: dict, - claimed_cursor: int, - old_cursor: int, - board: Optional[str] = None, - ) -> None: + def _kanban_rewind(self, sub: dict, claimed_cursor: int, old_cursor: int, board: Optional[str] = None) -> None: """Undo a claimed notification cursor after send failure.""" - self._kanban_sub_op( - board, "rewind_notify_cursor", sub, - claimed_cursor=claimed_cursor, old_cursor=old_cursor, - ) + self._kanban_sub_op(board, "rewind_notify_cursor", sub, claimed_cursor=claimed_cursor, old_cursor=old_cursor) - async def _deliver_kanban_artifacts( - self, - *, - adapter, - chat_id: str, - metadata: dict, - event_payload: Optional[dict], - task, - ) -> None: + async def _deliver_kanban_artifacts(self, *, adapter, chat_id: str, metadata: dict, event_payload: Optional[dict], task) -> None: """Upload artifact files referenced by a completed kanban task. Sources, in priority order: ``event_payload['artifacts']``, @@ -252,36 +142,21 @@ class GatewayKanbanWatchersMixin: deduplicated, missing files are skipped (may be mentioned for reference only), and upload errors are logged, never raised. """ - candidates: list[str] = [] - seen: set[str] = set() - - def _add(path: str) -> None: - if not path: - return - expanded = os.path.expanduser(path) - if expanded in seen or not os.path.isfile(expanded): - return - seen.add(expanded) - candidates.append(expanded) - + raw_paths: list[str] = [] if isinstance(event_payload, dict): raw = event_payload.get("artifacts") if isinstance(raw, (list, tuple)): - for item in raw: - if isinstance(item, str): - _add(item) - + raw_paths += [item for item in raw if isinstance(item, str)] summary = event_payload.get("summary") if isinstance(summary, str) and summary: - paths, _ = adapter.extract_local_files(summary) - for p in paths: - _add(p) - + raw_paths += adapter.extract_local_files(summary)[0] if task is not None and getattr(task, "result", None): - paths, _ = adapter.extract_local_files(str(task.result)) - for p in paths: - _add(p) - + raw_paths += adapter.extract_local_files(str(task.result))[0] + candidates: list[str] = [] + for path in raw_paths: + expanded = os.path.expanduser(path) if path else "" + if expanded and expanded not in candidates and os.path.isfile(expanded): + candidates.append(expanded) if not candidates: return @@ -290,41 +165,73 @@ class GatewayKanbanWatchersMixin: if not candidates: return - _IMAGE_EXTS = {".png", ".jpg", ".jpeg", ".gif", ".webp"} - _VIDEO_EXTS = {".mp4", ".mov", ".avi", ".mkv", ".webm", ".3gp"} - from urllib.parse import quote as _quote # Images ride one send_multiple_images call (batch uploads on Signal/Slack). image_paths = [p for p in candidates if Path(p).suffix.lower() in _IMAGE_EXTS] other_paths = [p for p in candidates if Path(p).suffix.lower() not in _IMAGE_EXTS] - if image_paths: try: batch = [(f"file://{_quote(p)}", "") for p in image_paths] - await adapter.send_multiple_images( - chat_id=chat_id, images=batch, metadata=metadata, - ) + await adapter.send_multiple_images(chat_id=chat_id, images=batch, metadata=metadata) except Exception as exc: - logger.warning( - "kanban notifier: image batch upload failed: %s", exc, - ) - + logger.warning("kanban notifier: image batch upload failed: %s", exc) for path in other_paths: try: if Path(path).suffix.lower() in _VIDEO_EXTS: - await adapter.send_video( - chat_id=chat_id, video_path=path, metadata=metadata, - ) + await adapter.send_video(chat_id=chat_id, video_path=path, metadata=metadata) else: - await adapter.send_document( - chat_id=chat_id, file_path=path, metadata=metadata, - ) + await adapter.send_document(chat_id=chat_id, file_path=path, metadata=metadata) except Exception as exc: - logger.warning( - "kanban notifier: artifact upload (%s) failed: %s", - path, exc, - ) + logger.warning("kanban notifier: artifact upload (%s) failed: %s", path, exc) + + def _kanban_dispatcher_boot(self) -> Optional[tuple]: + """Resolve config, kanban_db and the singleton lock; None when the dispatcher must not run. + + Config is read once at boot (restart to apply), except the auto-decompose + toggle which is re-read every tick. The env var is an escape hatch to + disable without editing YAML. + """ + try: + from hermes_cli.config import load_config as _load_config + except Exception: + logger.warning("kanban dispatcher: config loader unavailable; disabled") + return None + env_override = os.environ.get("HERMES_KANBAN_DISPATCH_IN_GATEWAY", "").strip().lower() + if env_override in {"0", "false", "no", "off"}: + logger.info("kanban dispatcher: disabled via HERMES_KANBAN_DISPATCH_IN_GATEWAY env") + return None + try: + cfg = _load_config() + except Exception as exc: + logger.warning("kanban dispatcher: cannot load config (%s); disabled", exc) + return None + kanban_cfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} + if not kanban_cfg.get("dispatch_in_gateway", True): + logger.info("kanban dispatcher: disabled via config kanban.dispatch_in_gateway=false") + return None + try: + from hermes_cli import kanban_db as _kb + except Exception: + logger.warning("kanban dispatcher: kanban_db not importable; dispatcher disabled") + return None + + # Single-dispatcher backstop (see _acquire_singleton_lock). The lock + # lives at the machine-global kanban root, so it serialises ALL gateways. + self._kanban_dispatcher_lock_handle = None + _lock_path = _kb.kanban_home() / "kanban" / ".dispatcher.lock" + _lock_handle, _lock_state = _acquire_singleton_lock(_lock_path) + if _lock_state == "contended": + logger.info("kanban dispatcher: another gateway already holds the dispatcher " + "lock (%s); this gateway will NOT dispatch.", _lock_path) + return None + if _lock_state == "held": + self._kanban_dispatcher_lock_handle = _lock_handle # hold for process lifetime + logger.info("kanban dispatcher: holding singleton dispatcher lock (%s)", _lock_path) + else: + logger.warning("kanban dispatcher: advisory lock unavailable at %s; proceeding " + "on config control alone.", _lock_path) + return _load_config, _kb, kanban_cfg async def _kanban_dispatcher_watcher(self) -> None: """Embedded kanban dispatcher — one tick every `dispatch_interval_seconds`. @@ -335,57 +242,10 @@ class GatewayKanbanWatchersMixin: failure never stops the next. Shutdown: ``self._running`` is checked between ticks and the in-flight ``to_thread`` returns on its own. """ - # Config is read once at boot (restart to apply), except the - # auto-decompose toggle which is re-read every tick. The env var is - # an escape hatch to disable without editing YAML. - try: - from hermes_cli.config import load_config as _load_config - except Exception: - logger.warning("kanban dispatcher: config loader unavailable; disabled") + boot = self._kanban_dispatcher_boot() + if boot is None: return - env_override = os.environ.get("HERMES_KANBAN_DISPATCH_IN_GATEWAY", "").strip().lower() - if env_override in {"0", "false", "no", "off"}: - logger.info("kanban dispatcher: disabled via HERMES_KANBAN_DISPATCH_IN_GATEWAY env") - return - - try: - cfg = _load_config() - except Exception as exc: - logger.warning("kanban dispatcher: cannot load config (%s); disabled", exc) - return - kanban_cfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} - if not kanban_cfg.get("dispatch_in_gateway", True): - logger.info( - "kanban dispatcher: disabled via config kanban.dispatch_in_gateway=false" - ) - return - - try: - from hermes_cli import kanban_db as _kb - except Exception: - logger.warning("kanban dispatcher: kanban_db not importable; dispatcher disabled") - return - - # Single-dispatcher backstop (see _acquire_singleton_lock). The lock - # lives at the machine-global kanban root, so it serialises ALL gateways. - self._kanban_dispatcher_lock_handle = None - _lock_path = _kb.kanban_home() / "kanban" / ".dispatcher.lock" - _lock_handle, _lock_state = _acquire_singleton_lock(_lock_path) - if _lock_state == "contended": - logger.info( - "kanban dispatcher: another gateway already holds the dispatcher " - "lock (%s); this gateway will NOT dispatch.", _lock_path, - ) - return - if _lock_state == "held": - self._kanban_dispatcher_lock_handle = _lock_handle # hold for process lifetime - logger.info("kanban dispatcher: holding singleton dispatcher lock (%s)", _lock_path) - else: - logger.warning( - "kanban dispatcher: advisory lock unavailable at %s; proceeding " - "on config control alone.", _lock_path, - ) - + _load_config, _kb, kanban_cfg = boot settings = _resolve_dispatcher_settings(kanban_cfg, _kb) interval = settings.interval @@ -395,25 +255,18 @@ class GatewayKanbanWatchersMixin: # Health telemetry (mirrors `_cmd_daemon`): warn when the ready queue # is non-empty but spawns are 0 for N consecutive ticks — usually a # broken PATH, missing venv, or credential loss. - HEALTH_WINDOW = 6 bad_ticks = 0 last_warn_at = 0 dispatcher = _KanbanDispatcher(_kb, settings) - logger.info( - "kanban dispatcher: embedded in gateway (interval=%.1fs)", interval - ) + logger.info("kanban dispatcher: embedded in gateway (interval=%.1fs)", interval) while self._running: try: # Reap zombies before per-board work so a board DB failure # cannot block cleanup of unrelated workers. pids = await _to_thread_process_service(_kb.reap_worker_zombies) if pids: - logger.info( - "kanban dispatcher: reaped %d zombie worker(s), pids=%s", - len(pids), - pids, - ) + logger.info("kanban dispatcher: reaped %d zombie worker(s), pids=%s", len(pids), pids) except Exception: logger.exception("kanban dispatcher: zombie reaper failed") @@ -430,20 +283,18 @@ class GatewayKanbanWatchersMixin: await _to_thread_process_service(dispatcher.auto_decompose_tick, _ad_per_tick) results = await _to_thread_process_service(dispatcher.tick_once) any_spawned = _log_spawn_results(results) - # Health telemetry (aggregate across boards) ready_pending = await _to_thread_process_service(dispatcher.ready_nonempty) bad_ticks = bad_ticks + 1 if ready_pending and not any_spawned else 0 - if bad_ticks >= HEALTH_WINDOW: - now = int(time.time()) - if now - last_warn_at >= 300: - logger.warning( - "kanban dispatcher stuck: ready queue non-empty for " - "%d consecutive ticks but 0 workers spawned. Check " - "profile health (venv, PATH, credentials) and " - "`hermes kanban list --status ready`.", - bad_ticks, - ) - last_warn_at = now + now = int(time.time()) + if bad_ticks >= _HEALTH_WINDOW and now - last_warn_at >= 300: + logger.warning( + "kanban dispatcher stuck: ready queue non-empty for " + "%d consecutive ticks but 0 workers spawned. Check " + "profile health (venv, PATH, credentials) and " + "`hermes kanban list --status ready`.", + bad_ticks, + ) + last_warn_at = now except asyncio.CancelledError: logger.debug("kanban dispatcher: cancelled") self._release_kanban_dispatcher_lock() diff --git a/gateway/kanban_watchers_common.py b/gateway/kanban_watchers_common.py index 22833575ee..167e24d49b 100644 --- a/gateway/kanban_watchers_common.py +++ b/gateway/kanban_watchers_common.py @@ -1,11 +1,18 @@ -"""Plumbing shared by the kanban notifier and dispatcher loops.""" +"""Plumbing shared by the kanban notifier and dispatcher loops. + +Thread offload, board enumeration, live-config coercers and the singleton +dispatcher lock live here so the notifier, dispatcher and mixin modules read +them from one place. +""" from __future__ import annotations import asyncio +import contextlib import logging from contextvars import Context -from typing import Any, Callable +from pathlib import Path +from typing import Any, Callable, Optional # Keep the logger name run.py used so extracted log records are unchanged. logger = logging.getLogger("gateway.run") @@ -33,3 +40,105 @@ def _list_boards(kb: Any) -> list: return kb.list_boards(include_archived=False) except Exception: return [kb.read_board_metadata(kb.DEFAULT_BOARD)] + + +def _board_slugs(kb: Any) -> list: + return [b.get("slug") or kb.DEFAULT_BOARD for b in _list_boards(kb)] + + +def _positive_int_setting(kanban_cfg: dict, key: str) -> Optional[int]: + """Parse an optional ``kanban.`` int cap; None when unset or invalid (< 1 is invalid).""" + raw = kanban_cfg.get(key) + if raw is None: + return None + try: + value = int(raw) + except (TypeError, ValueError): + logger.warning("kanban dispatcher: invalid kanban.%s=%r; ignoring", key, raw) + return None + if value < 1: + logger.warning("kanban dispatcher: kanban.%s=%r is below 1; ignoring", key, raw) + return None + logger.info("kanban dispatcher: %s=%d", key, value) + return value + + +def _resolve_auto_decompose_settings(load_config: Callable[[], Any]) -> "tuple[bool, int]": + """Live (enabled, per_tick) auto-decompose settings, re-read every dispatcher tick. + + Fails safe: a config read error returns ``(False, 3)`` rather than + re-enabling a feature the user turned off. ``per_tick`` is clamped to ``>= 1``. + """ + try: + cfg = load_config() + except Exception: + return False, 3 + kcfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} + try: + per_tick = int(kcfg.get("auto_decompose_per_tick", 3) or 3) + except (TypeError, ValueError): + per_tick = 3 + return bool(kcfg.get("auto_decompose", True)), max(per_tick, 1) + + +def _gc_retention_days() -> int: + """``kanban.done_sub_retention_days`` (default 30; 0 disables), re-read per sweep; fails safe to 30.""" + try: + from hermes_cli.config import load_config + + return int(((load_config() or {}).get("kanban") or {}).get("done_sub_retention_days", 30)) + except Exception: + return 30 + + +def _kanban_dispatch_allowed() -> bool: + """False while the global emergency stop (`hermes pause`) is engaged. + + Checked every tick before spawning, so a pause applies on the next tick; + in-flight workers are never touched. Fails open if estop is unimportable. + """ + try: + from agent.estop import check_paused + except ImportError: + return True + return not check_paused("kanban", logger) + + +def _acquire_singleton_lock(lock_path) -> "tuple[Optional[object], str]": + """Take the exclusive, non-blocking advisory lock for the sole dispatcher. + + Only one gateway machine-wide may run the embedded dispatcher: concurrent + dispatchers double reclaim frequency and claim events, and with + ``wal_autocheckpoint=0`` concurrent manual checkpoints can corrupt index + pages. ``dispatch_in_gateway`` is the primary control; this is the backstop. + + Returns ``(handle, "held")`` (release via :func:`_release_singleton_lock`), + ``(None, "contended")`` when another process holds it (caller must NOT + dispatch), or ``(None, "unavailable")`` when locking cannot be performed + (caller falls back to config control). + """ + try: + from gateway.status import _try_acquire_file_lock # deferred; same package + except ImportError: + return None, "unavailable" + try: + Path(lock_path).parent.mkdir(parents=True, exist_ok=True) + handle = open(str(lock_path), "a+", encoding="utf-8") + except OSError: + return None, "unavailable" + if not _try_acquire_file_lock(handle): + handle.close() + return None, "contended" + return handle, "held" + + +def _release_singleton_lock(handle) -> None: + """Release a lock acquired via :func:`_acquire_singleton_lock`.""" + if handle is None: + return + with contextlib.suppress(Exception): + from gateway.status import _release_file_lock + + _release_file_lock(handle) + with contextlib.suppress(Exception): + handle.close() diff --git a/gateway/kanban_watchers_dispatcher.py b/gateway/kanban_watchers_dispatcher.py index b768906245..28688b6068 100644 --- a/gateway/kanban_watchers_dispatcher.py +++ b/gateway/kanban_watchers_dispatcher.py @@ -11,10 +11,10 @@ import contextlib import os import sqlite3 import time -from dataclasses import dataclass +from dataclasses import asdict, dataclass from typing import Any, Optional -from gateway.kanban_watchers_common import _list_boards, logger +from gateway.kanban_watchers_common import _board_slugs, _positive_int_setting, logger _CORRUPT_DB_MARKERS = ("file is not a database", "database disk image is malformed") @@ -33,32 +33,13 @@ class _DispatcherSettings: max_in_progress_per_profile: Optional[int] -def _positive_int_setting(kanban_cfg: dict, key: str) -> Optional[int]: - """Parse an optional ``kanban.`` int cap; None when unset or invalid (< 1 is invalid).""" - raw = kanban_cfg.get(key) - if raw is None: - return None - try: - value = int(raw) - except (TypeError, ValueError): - logger.warning("kanban dispatcher: invalid kanban.%s=%r; ignoring", key, raw) - return None - if value < 1: - logger.warning("kanban dispatcher: kanban.%s=%r is below 1; ignoring", key, raw) - return None - logger.info("kanban dispatcher: %s=%d", key, value) - return value - - def _resolve_dispatcher_settings(kanban_cfg: dict, kb: Any) -> _DispatcherSettings: """Parse and log the dispatcher settings in their established order.""" try: interval = float(kanban_cfg.get("dispatch_interval_seconds", 60) or 60) except (ValueError, TypeError): - logger.warning( - "kanban dispatcher: invalid dispatch_interval_seconds=%r, using default 60", - kanban_cfg.get("dispatch_interval_seconds"), - ) + logger.warning("kanban dispatcher: invalid dispatch_interval_seconds=%r, using default 60", + kanban_cfg.get("dispatch_interval_seconds")) interval = 60.0 interval = max(interval, 1.0) # sanity floor — tighter than this is a footgun @@ -66,10 +47,9 @@ def _resolve_dispatcher_settings(kanban_cfg: dict, kb: Any) -> _DispatcherSettin if max_spawn is not None: logger.info("kanban dispatcher: max_spawn=%s", max_spawn) - # Cap simultaneously running tasks so slow workers don't pile up and - # time out. Explicit config wins; otherwise a memory-derived default - # (unbounded fan-out swap-thrashes small hosts), or None where total - # memory can't be read. + # Cap simultaneously running tasks so slow workers don't pile up and time + # out. Explicit config wins; otherwise a memory-derived default (unbounded + # fan-out swap-thrashes small hosts), or None where total memory can't be read. max_in_progress = _positive_int_setting(kanban_cfg, "max_in_progress") effective_max_in_progress = kb.resolve_max_in_progress(max_in_progress) if max_in_progress is None and effective_max_in_progress is not None: @@ -84,18 +64,12 @@ def _resolve_dispatcher_settings(kanban_cfg: dict, kb: Any) -> _DispatcherSettin try: failure_limit = int(raw_failure_limit) except (TypeError, ValueError): - logger.warning( - "kanban dispatcher: invalid kanban.failure_limit=%r; using default %d", - raw_failure_limit, - kb.DEFAULT_FAILURE_LIMIT, - ) + logger.warning("kanban dispatcher: invalid kanban.failure_limit=%r; using default %d", + raw_failure_limit, kb.DEFAULT_FAILURE_LIMIT) failure_limit = kb.DEFAULT_FAILURE_LIMIT if failure_limit < 1: - logger.warning( - "kanban dispatcher: kanban.failure_limit=%r is below 1; using default %d", - raw_failure_limit, - kb.DEFAULT_FAILURE_LIMIT, - ) + logger.warning("kanban dispatcher: kanban.failure_limit=%r is below 1; using default %d", + raw_failure_limit, kb.DEFAULT_FAILURE_LIMIT) failure_limit = kb.DEFAULT_FAILURE_LIMIT # 0 disables stale detection. @@ -103,22 +77,16 @@ def _resolve_dispatcher_settings(kanban_cfg: dict, kb: Any) -> _DispatcherSettin try: stale_timeout_seconds = int(raw_stale or 0) except (TypeError, ValueError): - logger.warning( - "kanban dispatcher: invalid kanban.dispatch_stale_timeout_seconds=%r; " - "disabling stale detection", - raw_stale, - ) + logger.warning("kanban dispatcher: invalid kanban.dispatch_stale_timeout_seconds=%r; " + "disabling stale detection", raw_stale) stale_timeout_seconds = 0 # Fallback profile for tasks created without an assignee (e.g. via the # dashboard). Empty (the schema default) keeps skipping them. default_assignee = (kanban_cfg.get("default_assignee") or "").strip() or None if default_assignee: - logger.info( - "kanban dispatcher: default_assignee=%r (unassigned ready tasks " - "will route to this profile)", - default_assignee, - ) + logger.info("kanban dispatcher: default_assignee=%r (unassigned ready tasks " + "will route to this profile)", default_assignee) return _DispatcherSettings( interval=interval, @@ -150,12 +118,10 @@ class _KanbanDispatcher: def __init__(self, kb: Any, settings: _DispatcherSettings) -> None: self.kb = kb self.settings = settings - self.disabled_corrupt_boards: dict[ - str, tuple[tuple[str, int | None, int | None], float] - ] = {} + self.disabled_corrupt_boards: dict[str, tuple[tuple[str, int | None, int | None], float]] = {} def _board_slugs(self) -> list: - return [b.get("slug") or self.kb.DEFAULT_BOARD for b in _list_boards(self.kb)] + return _board_slugs(self.kb) def board_db_fingerprint(self, slug: str) -> tuple[str, int | None, int | None]: path = self.kb.kanban_db_path(slug) @@ -173,10 +139,7 @@ class _KanbanDispatcher: corrupt_guard_error = getattr(self.kb, "KanbanDbCorruptError", None) if corrupt_guard_error is not None and isinstance(exc, corrupt_guard_error): return True - if not isinstance(exc, sqlite3.DatabaseError): - return False - msg = str(exc).lower() - return any(marker in msg for marker in _CORRUPT_DB_MARKERS) + return isinstance(exc, sqlite3.DatabaseError) and any(m in str(exc).lower() for m in _CORRUPT_DB_MARKERS) def _quarantine_lifted(self, slug: str, fingerprint: tuple) -> bool: """Return False while *slug* stays quarantined; lift (and log) otherwise.""" @@ -188,17 +151,10 @@ class _KanbanDispatcher: if disabled_fingerprint == fingerprint and age < self.CORRUPT_BOARD_RETRY_AFTER_SECONDS: return False if disabled_fingerprint == fingerprint: - logger.info( - "kanban dispatcher: board %s database fingerprint unchanged " - "after %.0fs quarantine; retrying dispatch", - slug, - age, - ) + logger.info("kanban dispatcher: board %s database fingerprint unchanged " + "after %.0fs quarantine; retrying dispatch", slug, age) else: - logger.info( - "kanban dispatcher: board %s database changed; retrying dispatch", - slug, - ) + logger.info("kanban dispatcher: board %s database changed; retrying dispatch", slug) self.disabled_corrupt_boards.pop(slug, None) return True @@ -212,22 +168,12 @@ class _KanbanDispatcher: fingerprint = self.board_db_fingerprint(slug) if not self._quarantine_lifted(slug, fingerprint): return None - s = self.settings + kwargs = {k: v for k, v in asdict(self.settings).items() if k != "interval"} try: # No explicit init_db(): connect() runs the migration once per # process (see the matching note in the notifier collector). conn = self.kb.connect(board=slug) - return self.kb.dispatch_once( - conn, - board=slug, - max_spawn=s.max_spawn, - max_in_progress=s.max_in_progress, - failure_limit=s.failure_limit, - stale_timeout_seconds=s.stale_timeout_seconds, - default_assignee=s.default_assignee, - max_in_progress_per_profile=s.max_in_progress_per_profile, - reconcile_orphans=s.reconcile_orphans, - ) + return self.kb.dispatch_once(conn, board=slug, **kwargs) except Exception as exc: if self.is_corrupt_board_db_error(exc): self.disabled_corrupt_boards[slug] = (fingerprint, time.monotonic()) @@ -237,8 +183,7 @@ class _KanbanDispatcher: "the file changes, the gateway restarts, or the " "quarantine timer expires. Move or restore the file, " "then run `hermes kanban init` if you need a fresh board.", - slug, - fingerprint[0], + slug, fingerprint[0], ) return None logger.exception("kanban dispatcher: tick failed on board %s", slug) @@ -253,14 +198,13 @@ class _KanbanDispatcher: return [(slug, self.tick_once_for_board(slug)) for slug in self._board_slugs()] def ready_nonempty(self) -> bool: - """Is there a ready+assigned+unclaimed task on ANY board that the - dispatcher would actually spawn for? + """Is there a ready+assigned+unclaimed task on ANY board the dispatcher would spawn for? - Control-plane lanes (e.g. ``orion-cc``) are pulled by terminals - via ``claim_task`` and never spawnable — a queue full of those is - "correctly idle", not "stuck". The review column is probed only - when review dispatch is on (same gate as the dispatcher): a task - waiting for a human reviewer is idle, not stuck. + Control-plane lanes (e.g. ``orion-cc``) are pulled by terminals via + ``claim_task`` and never spawnable — a queue full of those is + "correctly idle", not "stuck". The review column is probed only when + review dispatch is on (same gate as the dispatcher): a task waiting + for a human reviewer is idle, not stuck. """ kb = self.kb _review_probe = kb.review_dispatch_enabled() @@ -268,9 +212,7 @@ class _KanbanDispatcher: conn = None try: conn = kb.connect(board=slug) - if kb.has_spawnable_ready(conn): - return True - if _review_probe and kb.has_spawnable_review(conn): + if kb.has_spawnable_ready(conn) or (_review_probe and kb.has_spawnable_review(conn)): return True except Exception: continue @@ -281,17 +223,15 @@ class _KanbanDispatcher: return False def auto_decompose_tick(self, auto_decompose_per_tick: int) -> int: - """Auto-decompose up to N triage tasks across all boards into - ready workgraphs before dispatch fans out; the per-tick cap keeps - a bulk triage load from burst-spending the aux LLM. Returns the - number decomposed/specified. + """Auto-decompose up to N triage tasks across all boards into ready workgraphs. + + Runs before dispatch fans out; the per-tick cap keeps a bulk triage + load from burst-spending the aux LLM. Returns the number decomposed. """ try: from hermes_cli import kanban_decompose as _decomp except Exception as exc: # pragma: no cover - logger.warning( - "kanban auto-decompose: import failed (%s); skipping", exc, - ) + logger.warning("kanban auto-decompose: import failed (%s); skipping", exc) return 0 attempted = 0 successes = 0 @@ -306,10 +246,7 @@ class _KanbanDispatcher: try: triage_ids = _decomp.list_triage_ids() except Exception as exc: - logger.debug( - "kanban auto-decompose: list_triage_ids failed on board %s (%s)", - slug, exc, - ) + logger.debug("kanban auto-decompose: list_triage_ids failed on board %s (%s)", slug, exc) triage_ids = [] for tid in triage_ids: if attempted >= auto_decompose_per_tick: @@ -329,28 +266,16 @@ class _KanbanDispatcher: try: outcome = _decomp.decompose_task(tid, author="auto-decomposer") except Exception: - logger.exception( - "kanban auto-decompose: decompose_task crashed on %s", - tid, - ) + logger.exception("kanban auto-decompose: decompose_task crashed on %s", tid) return 0 if not outcome.ok: # Common no-op reasons (no aux client) must not spam logs every tick. - logger.debug( - "kanban auto-decompose [%s]: %s skipped: %s", - slug, tid, outcome.reason, - ) + logger.debug("kanban auto-decompose [%s]: %s skipped: %s", slug, tid, outcome.reason) return 0 if outcome.fanout and outcome.child_ids: - logger.info( - "kanban auto-decompose [%s]: %s → %d children", - slug, tid, len(outcome.child_ids), - ) + logger.info("kanban auto-decompose [%s]: %s → %d children", slug, tid, len(outcome.child_ids)) else: - logger.info( - "kanban auto-decompose [%s]: %s → single task (no fanout)", - slug, tid, - ) + logger.info("kanban auto-decompose [%s]: %s → single task (no fanout)", slug, tid) return 1 @@ -364,9 +289,7 @@ def _log_spawn_results(results: Optional[list]) -> bool: logger.info( "kanban dispatcher [%s]: spawned=%d reclaimed=%d " "crashed=%d timed_out=%d promoted=%d auto_blocked=%d", - slug, - len(res.spawned), - res.reclaimed, + slug, len(res.spawned), res.reclaimed, len(res.crashed) if hasattr(res.crashed, "__len__") else 0, len(res.timed_out) if hasattr(res.timed_out, "__len__") else 0, res.promoted, diff --git a/gateway/kanban_watchers_notifier.py b/gateway/kanban_watchers_notifier.py index 2abb00890e..128b7ec037 100644 --- a/gateway/kanban_watchers_notifier.py +++ b/gateway/kanban_watchers_notifier.py @@ -21,34 +21,21 @@ from gateway.kanban_watchers_common import _list_boards, _to_thread_process_serv TERMINAL_KINDS = ("completed", "blocked", "gave_up", "crashed", "timed_out", "status", "archived", "unblocked", "block_loop_detected", "review_requested", "changes_requested") # Kinds that hand a decision back to the origin, which must take a turn. # status/archived/unblocked are bookkeeping. -_WAKE_KINDS = ( - "completed", "gave_up", "crashed", "timed_out", - "blocked", "review_requested", "changes_requested", - "block_loop_detected", -) -# Consecutive send failures (adapter raised OR reported -# SendResult(success=False)) before a sub is dropped as a dead chat. -# 12 ≈ 60s at the 5s cadence: a transient API outage must not -# permanently unsubscribe a live review-gate channel. +_WAKE_KINDS = ("completed", "gave_up", "crashed", "timed_out", "blocked", "review_requested", "changes_requested", "block_loop_detected") +# Consecutive send failures (adapter raised OR reported SendResult(success=False)) +# before a sub is dropped as a dead chat. 12 ≈ 60s at the 5s cadence: a transient +# API outage must not permanently unsubscribe a live review-gate channel. MAX_SEND_FAILURES = 12 -_LOCAL_PATH_RE = re.compile( - r"(? str: """Return a mobile-friendly review reason safe for external delivery.""" from agent.redact import redact_sensitive_text - reason = redact_sensitive_text( - "" if value is None else str(value), - force=True, - redact_url_credentials=True, - ) - reason = _LOCAL_PATH_RE.sub("[local path]", reason) - reason = " ".join(reason.split()) + reason = redact_sensitive_text("" if value is None else str(value), force=True, redact_url_credentials=True) + reason = " ".join(_LOCAL_PATH_RE.sub("[local path]", reason).split()) if len(reason) > limit: reason = reason[: limit - 1].rstrip() + "…" return reason @@ -70,21 +57,15 @@ def _wake_scope_id(adapter: Any, sub: dict) -> Optional[str]: if value: return str(value) resolver = getattr(adapter, "scope_id_for_chat", None) - if callable(resolver): - try: - resolved = resolver(str(sub.get("chat_id") or "")) - except Exception as exc: - # An adapter-side lookup failure yields no scope, never an error. - logger.debug( - "kanban notifier: scope lookup failed for chat %s: %s", - sub.get("chat_id"), - exc, - exc_info=True, - ) - return None - if resolved: - return str(resolved) - return None + if not callable(resolver): + return None + try: + resolved = resolver(str(sub.get("chat_id") or "")) + except Exception as exc: + # An adapter-side lookup failure yields no scope, never an error. + logger.debug("kanban notifier: scope lookup failed for chat %s: %s", sub.get("chat_id"), exc, exc_info=True) + return None + return str(resolved) if resolved else None def _platform_names(mapping: Any) -> set[str]: @@ -92,260 +73,177 @@ def _platform_names(mapping: Any) -> set[str]: return {getattr(platform, "value", str(platform)).lower() for platform in mapping} -# --------------------------------------------------------------------------- -# Collection (runs in a worker thread) -# --------------------------------------------------------------------------- +# --- Collection (runs in a worker thread) --- -def _notifier_collect( - runner: Any, - kb: Any, - *, - notifier_profile: Optional[str], - gc_due: bool, - gc_retention_days: int, -) -> list[dict]: +class _Collector: + """One tick's claim state: which profiles/platforms this gateway serves and the GC gate.""" + + def __init__(self, runner: Any, kb: Any, *, notifier_profile: Optional[str], gc_due: bool, gc_retention_days: int) -> None: + self.kb = kb + self.notifier_profile = notifier_profile + self.gc_due = gc_due + self.gc_retention_days = gc_retention_days + self.deliveries: list[dict] = [] + self.include_unowned = runner._owns_kanban_dispatcher_lock() + self.profile_adapters = getattr(runner, "_profile_adapters", {}) + self.notifier_profiles = {notifier_profile} + self.notifier_profiles.update(str(p).strip() for p in self.profile_adapters if str(p).strip()) + # Include every platform any secondary profile has live. This is only a + # coarse pre-filter; the precise per-profile check (_authorization_adapter, + # no default fallback) runs at delivery and rewinds the claim if it + # resolves to None. An unclaimed event never retries, so dropping a + # secondary-profile sub here would lose it. + self.active_platforms = _platform_names(runner.adapters).union( + *(_platform_names(m) for m in self.profile_adapters.values())) + + def collect(self) -> list[dict]: + if not self.active_platforms: + logger.debug("kanban notifier: no connected adapters; skipping tick") + return self.deliveries + # Poll each resolved DB path once: several slugs can map to one DB when + # HERMES_KANBAN_DB pins the board path. + kb = self.kb + seen_db_paths: set[str] = set() + for board_meta in _list_boards(kb): + slug = board_meta.get("slug") or kb.DEFAULT_BOARD + db_path = board_meta.get("db_path") + try: + resolved_db_path = str(Path(db_path).expanduser().resolve()) if db_path else str(kb.kanban_db_path(slug).resolve()) + except Exception: + resolved_db_path = f"slug:{slug}" + if resolved_db_path in seen_db_paths: + logger.debug("kanban notifier: skipping duplicate board slug %s for DB %s", slug, resolved_db_path) + continue + seen_db_paths.add(resolved_db_path) + self.collect_board(slug) + return self.deliveries + + def _board_has_subs(self, slug: str) -> bool: + """Cheap read-only probe before the writable connect() (schema init, WAL + sidecars, checkpoints); a probe failure falls back to the writable open.""" + try: + count = self.kb.count_notify_subs( + board=slug, notifier_profiles=self.notifier_profiles, include_unowned=self.include_unowned) + except Exception as exc: + logger.debug("kanban notifier: read-only subscription probe failed " + "for board %s (%s); falling back to writable open", slug, exc) + return True + if count == 0: + logger.debug("kanban notifier: board %s has no subscriptions owned by %s; skipping open", + slug, sorted(self.notifier_profiles)) + return count != 0 + + def _gc_stale_subs(self, conn: Any, slug: str) -> None: + """Best-effort stale-sub sweep: a failed sweep never blocks delivery; the next hourly gate retries.""" + try: + _purged = self.kb.purge_stale_done_notify_subs(conn, max_age_days=self.gc_retention_days) + if _purged: + logger.info("kanban notifier: purged %d stale done/blocked-task subscription(s) on board %s (retention %dd)", + _purged, slug, self.gc_retention_days) + except Exception as _gc_exc: + logger.debug("kanban notifier: stale-sub GC failed for board %s: %s", slug, _gc_exc) + + def _claim_for_sub(self, conn: Any, slug: str, sub: dict) -> Optional[dict]: + """Claim one subscription's unseen events; None when skipped or nothing new.""" + owner_profile = sub.get("notifier_profile") or None + if owner_profile and owner_profile != self.notifier_profile and not self.profile_adapters.get(owner_profile): + logger.debug("kanban notifier: subscription for %s owned by profile %s; current profile %s has no adapter for it, skipping", + sub.get("task_id"), owner_profile, self.notifier_profile) + return None + platform = (sub.get("platform") or "").lower() + if platform not in self.active_platforms: + logger.debug("kanban notifier: subscription for %s on %s skipped; adapter not connected", + sub.get("task_id"), platform or "") + return None + old_cursor, cursor, events = self.kb.claim_unseen_events_for_sub( + conn, task_id=sub["task_id"], platform=sub["platform"], chat_id=sub["chat_id"], + thread_id=sub.get("thread_id") or "", kinds=TERMINAL_KINDS, + ) + if not events: + return None + task = self.kb.get_task(conn, sub["task_id"]) + logger.debug("kanban notifier: claimed %d event(s) for %s on board %s cursor %s→%s", + len(events), sub["task_id"], slug, old_cursor, cursor) + return {"sub": sub, "old_cursor": old_cursor, "cursor": cursor, "events": events, "task": task, "board": slug} + + def collect_board(self, slug: str) -> None: + """Claim events on one board, appending delivery dicts to ``deliveries``.""" + if not self._board_has_subs(slug): + return + kb = self.kb + try: + conn = kb.connect(board=slug) + except Exception as exc: + logger.debug("kanban notifier: cannot open board %s: %s", slug, exc) + return + try: + if self.gc_due: + self._gc_stale_subs(conn, slug) + # No explicit init_db(): connect() already runs the migration once per + # process, and init_db() would re-run it on a second connection racing + # the first. + subs = kb.list_notify_subs(conn, notifier_profiles=self.notifier_profiles, include_unowned=self.include_unowned) + if not subs: + logger.debug("kanban notifier: board %s has no subscriptions", slug) + for sub in subs: + try: + claimed = self._claim_for_sub(conn, slug, sub) + if claimed is not None: + self.deliveries.append(claimed) + except Exception as sub_exc: + # One bad subscription must not block the rest of the tick. + logger.warning("kanban notifier: subscription for %s on board %s failed: %s", + sub.get("task_id"), slug, sub_exc) + finally: + conn.close() + + +def _notifier_collect(runner: Any, kb: Any, *, notifier_profile: Optional[str], gc_due: bool, gc_retention_days: int) -> list[dict]: """Claim unseen terminal events for every owned subscription on every board. Each gateway polls only subscriptions owned by profiles whose adapters it hosts; legacy rows without a profile stamp are visible only to the process holding the singleton dispatcher lock. """ - deliveries: list[dict] = [] - include_unowned = runner._owns_kanban_dispatcher_lock() - profile_adapters = getattr(runner, "_profile_adapters", {}) - notifier_profiles = {notifier_profile} - notifier_profiles.update( - str(profile).strip() for profile in profile_adapters if str(profile).strip() - ) - active_platforms = _platform_names(runner.adapters) - # Include every platform any secondary profile has live. This is only a - # coarse pre-filter; the precise per-profile check (_authorization_adapter, - # no default fallback) runs at delivery and rewinds the claim if it - # resolves to None. An unclaimed event never retries, so dropping a - # secondary-profile sub here would lose it. - for _profile_adapter_map in profile_adapters.values(): - active_platforms.update(_platform_names(_profile_adapter_map)) - if not active_platforms: - logger.debug("kanban notifier: no connected adapters; skipping tick") - return deliveries - - # Poll each resolved DB path once: several slugs can map to one DB when - # HERMES_KANBAN_DB pins the board path. - seen_db_paths: set[str] = set() - for board_meta in _list_boards(kb): - slug = board_meta.get("slug") or kb.DEFAULT_BOARD - db_path = board_meta.get("db_path") - try: - resolved_db_path = str(Path(db_path).expanduser().resolve()) if db_path else str(kb.kanban_db_path(slug).resolve()) - except Exception: - resolved_db_path = f"slug:{slug}" - if resolved_db_path in seen_db_paths: - logger.debug( - "kanban notifier: skipping duplicate board slug %s for DB %s", - slug, resolved_db_path, - ) - continue - seen_db_paths.add(resolved_db_path) - _notifier_collect_board( - kb, slug, deliveries, - notifier_profile=notifier_profile, - notifier_profiles=notifier_profiles, - include_unowned=include_unowned, - profile_adapters=profile_adapters, - active_platforms=active_platforms, - gc_due=gc_due, - gc_retention_days=gc_retention_days, - ) - return deliveries + return _Collector( + runner, kb, notifier_profile=notifier_profile, gc_due=gc_due, gc_retention_days=gc_retention_days, + ).collect() -def _notifier_collect_board( - kb: Any, - slug: str, - deliveries: list[dict], - *, - notifier_profile: Optional[str], - notifier_profiles: set, - include_unowned: bool, - profile_adapters: dict, - active_platforms: set[str], - gc_due: bool, - gc_retention_days: int, -) -> None: - """Claim events on one board, appending delivery dicts to *deliveries*.""" - # Cheap read-only probe before the writable connect() (schema init, WAL - # sidecars, checkpoints) — a board with no subscriptions has nothing to notify. - try: - if kb.count_notify_subs( - board=slug, - notifier_profiles=notifier_profiles, - include_unowned=include_unowned, - ) == 0: - logger.debug( - "kanban notifier: board %s has no subscriptions owned by %s; skipping open", - slug, sorted(notifier_profiles), - ) - return - except Exception as exc: - logger.debug( - "kanban notifier: read-only subscription probe failed " - "for board %s (%s); falling back to writable open", - slug, exc, - ) - try: - conn = kb.connect(board=slug) - except Exception as exc: - logger.debug("kanban notifier: cannot open board %s: %s", slug, exc) - return - try: - if gc_due: - # Best-effort: a failed sweep never blocks delivery; the next - # hourly gate retries. - try: - _purged = kb.purge_stale_done_notify_subs(conn, max_age_days=gc_retention_days) - if _purged: - logger.info( - "kanban notifier: purged %d stale done/blocked-task subscription(s) on board %s (retention %dd)", - _purged, slug, gc_retention_days, - ) - except Exception as _gc_exc: - logger.debug( - "kanban notifier: stale-sub GC failed for board %s: %s", - slug, _gc_exc, - ) - # No explicit init_db(): connect() already runs the migration once per - # process, and init_db() would re-run it on a second connection racing - # the first. - subs = kb.list_notify_subs( - conn, - notifier_profiles=notifier_profiles, - include_unowned=include_unowned, - ) - if not subs: - logger.debug("kanban notifier: board %s has no subscriptions", slug) - for sub in subs: - try: - owner_profile = sub.get("notifier_profile") or None - if owner_profile and owner_profile != notifier_profile and not profile_adapters.get(owner_profile): - logger.debug( - "kanban notifier: subscription for %s owned by profile %s; current profile %s has no adapter for it, skipping", - sub.get("task_id"), owner_profile, notifier_profile, - ) - continue - platform = (sub.get("platform") or "").lower() - if platform not in active_platforms: - logger.debug( - "kanban notifier: subscription for %s on %s skipped; adapter not connected", - sub.get("task_id"), platform or "", - ) - continue - old_cursor, cursor, events = kb.claim_unseen_events_for_sub( - conn, - task_id=sub["task_id"], - platform=sub["platform"], - chat_id=sub["chat_id"], - thread_id=sub.get("thread_id") or "", - kinds=TERMINAL_KINDS, - ) - if not events: - continue - task = kb.get_task(conn, sub["task_id"]) - logger.debug( - "kanban notifier: claimed %d event(s) for %s on board %s cursor %s→%s", - len(events), sub["task_id"], slug, old_cursor, cursor, - ) - deliveries.append({ - "sub": sub, - "old_cursor": old_cursor, - "cursor": cursor, - "events": events, - "task": task, - "board": slug, - }) - except Exception as sub_exc: - # One bad subscription must not block the rest of the tick. - logger.warning( - "kanban notifier: subscription for %s on board %s failed: %s", - sub.get("task_id"), slug, sub_exc, - ) - finally: - conn.close() - - -# --------------------------------------------------------------------------- -# Per-event message formatting: kind -> (msg, wake_handoff, wake_review_detail) -# --------------------------------------------------------------------------- -# ``None`` for handoff / review_detail leaves the accumulated wake value -# untouched. ``_payload(ev, key)`` is the shared "payload present and truthy" read. +# --- Per-event message formatting: kind -> (msg, wake_handoff, wake_review_detail) --- +# ``None`` for handoff / review_detail leaves the accumulated wake value untouched. def _payload(ev: Any, key: str) -> Any: + """Shared "payload present and truthy" read.""" return ev.payload.get(key) if ev.payload and ev.payload.get(key) else None +def _clip(ev: Any, key: str, fmt: str, limit: int) -> str: + """``fmt`` applied to the truncated payload value, or ``""`` when absent.""" + value = _payload(ev, key) + return fmt.format(str(value)[:limit]) if value else "" + + +_NL = "\n{}" + + def _first_line(text: str, limit: int) -> str: lines = text.strip().splitlines() return lines[0][:limit] if lines else text[:limit] def _fmt_completed(ev, n) -> tuple: - # Prefer the run summary from the event payload; fall back to - # task.result for legacy rows. - handoff = "" + # Prefer the run summary from the event payload; fall back to task.result for legacy rows. wake_handoff = None payload_summary = _payload(ev, "summary") if payload_summary: wake_handoff = _first_line(str(payload_summary), 200) - handoff = f"\n{wake_handoff}" elif n.task and n.task.result: wake_handoff = _first_line(n.task.result, 160) - handoff = f"\n{wake_handoff}" - msg = ( - f"✔ {n.board_tag}{n.tag}Kanban {n.task_id} done" - f" — {n.title}{handoff}" - ) - return msg, wake_handoff, None - - -def _fmt_blocked(ev, n) -> tuple: - reason = _payload(ev, "reason") - reason = f": {str(reason)[:160]}" if reason else "" - return f"⏸ {n.board_tag}{n.tag}Kanban {n.task_id} blocked{reason}", None, None - - -def _fmt_gave_up(ev, n) -> tuple: - err = _payload(ev, "error") - err = f"\n{str(err)[:200]}" if err else "" - msg = ( - f"✖ {n.board_tag}{n.tag}Kanban {n.task_id} gave up " - f"after repeated spawn failures{err}" - ) - return msg, None, None - - -def _fmt_crashed(ev, n) -> tuple: - msg = ( - f"✖ {n.board_tag}{n.tag}Kanban {n.task_id} worker crashed " - f"(pid gone); dispatcher will retry" - ) - return msg, None, None - - -def _fmt_timed_out(ev, n) -> tuple: - limit = _payload(ev, "limit_seconds") - limit = int(limit) if limit else 0 - msg = ( - f"⏱ {n.board_tag}{n.tag}Kanban {n.task_id} timed out " - f"(max_runtime={limit}s); will retry" - ) - return msg, None, None - - -def _fmt_status(ev, n) -> tuple: - new_status = _payload(ev, "status") - new_status = str(new_status) if new_status else "" - return f"🔄 {n.board_tag}{n.tag}Kanban {n.task_id} → {new_status}", None, None + handoff = f"\n{wake_handoff}" if wake_handoff is not None else "" + return f"✔ {n.head} done — {n.title}{handoff}", wake_handoff, None def _fmt_review_requested(ev, n) -> tuple: @@ -358,11 +256,7 @@ def _fmt_review_requested(ev, n) -> tuple: summary = str(summary) handoff = f"\n{summary[:200]}" wake_handoff = _first_line(summary, 200) - msg = ( - f"👀 {n.board_tag}{n.tag}Kanban {n.task_id} ready for review" - f" — {n.title}{handoff}" - ) - return msg, wake_handoff, None + return f"👀 {n.head} ready for review — {n.title}{handoff}", wake_handoff, None def _fmt_changes_requested(ev, n) -> tuple: @@ -371,51 +265,40 @@ def _fmt_changes_requested(ev, n) -> tuple: reviewer = _safe_review_reason(payload.get("reviewer"), 48) implementer = _safe_review_reason(payload.get("implementer"), 48) reason_text = reason or "reviewer feedback requires changes" - provenance = "" - if reviewer: - provenance += f" — reviewer @{reviewer}" + provenance = f" — reviewer @{reviewer}" if reviewer else "" if implementer: provenance += f" → implementer @{implementer}" - msg = ( - f"🛑 {n.board_tag}Kanban {n.task_id} review requested " - f"changes/BLOCK: {reason_text}{provenance}" - ) + msg = f"🛑 {n.board_tag}Kanban {n.task_id} review requested changes/BLOCK: {reason_text}{provenance}" return msg, None, reason_text -def _fmt_block_loop_detected(ev, n) -> tuple: - # Re-blocked for the same cause past the limit and routed to `triage` - # for a human. It emits no blocked/status event, so ping loudly here. - reason = _payload(ev, "reason") - reason = f": {str(reason)[:160]}" if reason else "" - recurrences = ev.payload.get("recurrences") if ev.payload else None - rc = f" (blocked {recurrences}x for the same cause)" if recurrences else "" - msg = ( - f"🛑 {n.board_tag}{n.tag}Kanban {n.task_id} routed to TRIAGE" - f" — needs a human decision{rc}{reason}" - ) - return msg, None, None - - # archived / unblocked are claimed (so the cursor advances past them) but # intentionally silent (no formatter), and excluded from _WAKE_KINDS so they # never wake the creator. _EVENT_FORMATTERS: dict[str, Callable[[Any, "_KanbanNotification"], tuple]] = { "completed": _fmt_completed, - "blocked": _fmt_blocked, - "gave_up": _fmt_gave_up, - "crashed": _fmt_crashed, - "timed_out": _fmt_timed_out, - "status": _fmt_status, + "blocked": lambda ev, n: (f"⏸ {n.head} blocked{_clip(ev, 'reason', ': {}', 160)}", None, None), + "gave_up": lambda ev, n: ( + f"✖ {n.head} gave up after repeated spawn failures{_clip(ev, 'error', _NL, 200)}", None, None, + ), + "crashed": lambda ev, n: (f"✖ {n.head} worker crashed (pid gone); dispatcher will retry", None, None), + "timed_out": lambda ev, n: ( + f"⏱ {n.head} timed out (max_runtime={int(_payload(ev, 'limit_seconds') or 0)}s); will retry", None, None, + ), + "status": lambda ev, n: (f"🔄 {n.head} → {_payload(ev, 'status') or ''}", None, None), "review_requested": _fmt_review_requested, "changes_requested": _fmt_changes_requested, - "block_loop_detected": _fmt_block_loop_detected, + # Re-blocked for the same cause past the limit and routed to `triage` for a + # human. It emits no blocked/status event, so ping loudly here. + "block_loop_detected": lambda ev, n: ( + f"🛑 {n.head} routed to TRIAGE — needs a human decision" + f"{_clip(ev, 'recurrences', ' (blocked {}x for the same cause)', 200)}{_clip(ev, 'reason', ': {}', 160)}", + None, None, + ), } -# --------------------------------------------------------------------------- -# Delivery of one claimed batch (one subscription, N events) -# --------------------------------------------------------------------------- +# --- Delivery of one claimed batch (one subscription, N events) --- class _KanbanNotification: @@ -443,8 +326,8 @@ class _KanbanNotification: self.title = (task.title if task else sub["task_id"])[:120] self.board_tag = f"[{self.board_slug}] " if self.board_slug else "" # Attribute the ping to the worker that did the work. - who = task.assignee if task and task.assignee else None - self.tag = f"@{who} " if who else "" + tag = f"@{task.assignee} " if task and task.assignee else "" + self.head = f"{self.board_tag}{tag}Kanban {self.task_id}" # The wake self-post path needs the key even when every event was skipped. self.sub_key = (sub["task_id"], sub["platform"], sub["chat_id"], sub.get("thread_id") or "") mode = sub.get("delivery_mode") or "notify" @@ -452,14 +335,11 @@ class _KanbanNotification: self.send_passive = mode != "wake" # Worker handoff carried into the synthetic wake turn so the woken # creator doesn't re-decompose work already on the board. - self.wake_handoff = "" - self.wake_review_detail = "" + self.wake_handoff = self.wake_review_detail = self.session_key = self.synth = "" self.plat: Any = None self.adapter: Any = None self.is_push_adapter = True self.wake_kinds: set = set() - self.session_key = "" - self.synth = "" # -- cursor / subscription ops (blocking, run in a fresh-context thread) -- @@ -490,11 +370,8 @@ class _KanbanNotification: await self.rewind() async def _wake_failed(self, fmt: str, exc: Exception) -> None: - await self.delivery_failed( - fmt, (self.task_id,), - "kanban notifier: dropping subscription %s on %s after %d consecutive wake failures", - exc, True, - ) + drop_fmt = "kanban notifier: dropping subscription %s on %s after %d consecutive wake failures" + await self.delivery_failed(fmt, (self.task_id,), drop_fmt, exc, True) # -- formatting -- @@ -513,11 +390,7 @@ class _KanbanNotification: def build_wake_text(self) -> None: """Set ``wake_kinds`` / ``session_key`` / ``synth`` for the wake paths.""" task, sub = self.task, self.sub - self.wake_kinds = ( - {ev.kind for ev in self.d["events"] if ev.kind in _WAKE_KINDS} - if self.wake_agent - else set() - ) + self.wake_kinds = {ev.kind for ev in self.d["events"] if ev.kind in _WAKE_KINDS} if self.wake_agent else set() if not self.wake_kinds: return if self.is_push_adapter: @@ -532,11 +405,8 @@ class _KanbanNotification: _status = t("gateway.kanban.wake.status_joiner").join(_parts) or t("gateway.kanban.wake.status_default") synth = t( "gateway.kanban.wake.message", - task_id=sub["task_id"], - status=_status, - title=self.title, - assignee=task.assignee if task else "", - board=self.board_slug, + task_id=sub["task_id"], status=_status, title=self.title, + assignee=task.assignee if task else "", board=self.board_slug, ) # Label as an automatic notification and carry the handoff so the # creator inspects the board instead of re-decomposing. @@ -547,36 +417,32 @@ class _KanbanNotification: self.synth = synth + "\n\n" + t("gateway.kanban.wake.guidance") def _log_woke(self) -> None: - logger.info( - "kanban notifier: woke agent for %s on %s/%s profile=%s events=%s", - self.task_id, self.platform_str, self.sub["chat_id"], self.sub_profile or "default", self.wake_kinds, - ) + logger.info("kanban notifier: woke agent for %s on %s/%s profile=%s events=%s", + self.task_id, self.platform_str, self.sub["chat_id"], self.sub_profile or "default", self.wake_kinds) - async def push_wake(self) -> None: - """Wake the creator session behind a push adapter; raises on failure.""" - from gateway.session import SessionSource + async def wake(self) -> None: + """Wake the creator session (raises on failure): push adapters get a full SessionSource, non-push a raw self-post.""" from gateway.wake import deliver_wake sub = self.sub - # Rebuild the creator's real session scope from the persisted - # chat_type: build_session_key() keys DMs differently from - # group/thread, so a hardcoded "group" mis-routed DM/thread creators - # into a fresh session. Legacy rows may carry chat_type in - # delivery_metadata; last resort is "group". A mismatch only degrades - # to a fresh session. + if not self.is_push_adapter: + await deliver_wake(self.adapter, text=self.synth, session_id=self.session_key) + self._log_woke() + return + from gateway.session import SessionSource + # Rebuild the creator's real session scope from the persisted chat_type: + # build_session_key() keys DMs differently from group/thread, so a + # hardcoded "group" mis-routed DM/thread creators into a fresh session. + # Legacy rows may carry chat_type in delivery_metadata; last resort is + # "group". A mismatch only degrades to a fresh session. _chat_type = str(sub.get("chat_type") or "").strip() if not _chat_type: _delivery_meta = sub.get("delivery_metadata") if isinstance(_delivery_meta, dict): _chat_type = str(_delivery_meta.get("chat_type") or "").strip() _source = SessionSource( - platform=self.plat, - chat_id=sub["chat_id"], - chat_type=_chat_type or "group", - thread_id=sub.get("thread_id") or None, - user_id=sub.get("user_id"), - user_id_alt=sub.get("user_id_alt"), - profile=self.sub_profile or None, - scope_id=_wake_scope_id(self.adapter, sub), + platform=self.plat, chat_id=sub["chat_id"], chat_type=_chat_type or "group", + thread_id=sub.get("thread_id") or None, user_id=sub.get("user_id"), user_id_alt=sub.get("user_id_alt"), + profile=self.sub_profile or None, scope_id=_wake_scope_id(self.adapter, sub), ) await deliver_wake(self.adapter, text=self.synth, session_id=self.session_key, source=_source) self._log_woke() @@ -593,30 +459,19 @@ class _KanbanNotification: # (else the event is lost); None / non-SendResult keeps the # "no exception == delivered" contract. if getattr(_send_res, "success", True) is False: - raise RuntimeError( - "adapter send() reported failure: " - f"{getattr(_send_res, 'error', None) or 'unknown error'}" - ) - logger.debug( - "kanban notifier: delivered %s event for %s to %s/%s on board %s", - ev.kind, self.task_id, self.platform_str, sub["chat_id"], self.board_slug, - ) + raise RuntimeError(f"adapter send() reported failure: {getattr(_send_res, 'error', None) or 'unknown error'}") + logger.debug("kanban notifier: delivered %s event for %s to %s/%s on board %s", + ev.kind, self.task_id, self.platform_str, sub["chat_id"], self.board_slug) # Upload artifact paths from the completion payload / legacy result as # native files. Only on ``completed`` so retries never spam attachments. if ev.kind == "completed": try: await self.runner._deliver_kanban_artifacts( - adapter=adapter, - chat_id=sub["chat_id"], - metadata=metadata, - event_payload=getattr(ev, "payload", None), - task=self.task, + adapter=adapter, chat_id=sub["chat_id"], metadata=metadata, + event_payload=getattr(ev, "payload", None), task=self.task, ) except Exception as art_exc: - logger.debug( - "kanban notifier: artifact delivery for %s failed: %s", - self.task_id, art_exc, - ) + logger.debug("kanban notifier: artifact delivery for %s failed: %s", self.task_id, art_exc) async def _send_pings(self) -> bool: """Send every text ping; False when a send failed (claim already rewound/dropped).""" @@ -630,10 +485,8 @@ class _KanbanNotification: # IS the delivery and resolves the failure counter. if not self.is_push_adapter and self.wake_agent: logger.debug( - "kanban notifier: adapter %s has no push " - "channel; skipping text ping for %s, relying " - "on wake self-post instead", - self.platform_str, self.task_id, + "kanban notifier: adapter %s has no push channel; skipping text ping for %s, relying " + "on wake self-post instead", self.platform_str, self.task_id, ) continue if not self.send_passive: @@ -644,10 +497,8 @@ class _KanbanNotification: self.clear_failures() except Exception as exc: await self.delivery_failed( - "kanban notifier: send failed for %s on %s (attempt %d/%d): %s", - (self.task_id, self.platform_str), - "kanban notifier: dropping subscription %s on %s after %d consecutive send failures", - exc, False, + "kanban notifier: send failed for %s on %s (attempt %d/%d): %s", (self.task_id, self.platform_str), + "kanban notifier: dropping subscription %s on %s after %d consecutive send failures", exc, False, ) return False return True @@ -664,10 +515,8 @@ class _KanbanNotification: # default) has no adapter. adapter = self.runner._authorization_adapter(self.plat, self.sub_profile or None) if adapter is None: - logger.debug( - "kanban notifier: adapter %s disconnected before delivery for %s; rewinding claim", - self.platform_str, self.task_id, - ) + logger.debug("kanban notifier: adapter %s disconnected before delivery for %s; rewinding claim", + self.platform_str, self.task_id) await self.rewind() return self.adapter = adapter @@ -677,29 +526,21 @@ class _KanbanNotification: if not await self._send_pings(): return # All text pings delivered (or skipped for non-push / wake-only). - task_terminal = self.task and self.task.status == "archived" self.build_wake_text() wake_kinds, is_push = self.wake_kinds, self.is_push_adapter - if not is_push and wake_kinds and self.session_key: - # Self-post IS the delivery: must succeed BEFORE the cursor advances. - from gateway.wake import deliver_wake + # Non-push self-post, or wake-only push sub: the wake IS the delivery + # and must succeed BEFORE the cursor advances. + if wake_kinds and (not self.send_passive if is_push else bool(self.session_key)): try: - await deliver_wake(adapter, text=self.synth, session_id=self.session_key) - self._log_woke() + await self.wake() self.clear_failures() except Exception as _wk_err: - await self._wake_failed("kanban notifier: wake self-post failed for %s (attempt %d/%d): %s", _wk_err) - return - - if is_push and not self.send_passive and wake_kinds: - # Wake-only push sub: the wake is the sole delivery and must - # succeed BEFORE the cursor advances. - try: - await self.push_wake() - self.clear_failures() - except Exception as _wk_err: - await self._wake_failed("kanban notifier: wake-only delivery failed for %s (attempt %d/%d): %s", _wk_err) + await self._wake_failed( + "kanban notifier: wake-only delivery failed for %s (attempt %d/%d): %s" if is_push + else "kanban notifier: wake self-post failed for %s (attempt %d/%d): %s", + _wk_err, + ) return # Delivery complete: advance the cursor (the dedup mechanism). @@ -711,12 +552,9 @@ class _KanbanNotification: # advanced; the wake stays best-effort, but log at WARNING so a # persistently failing wake is visible. try: - await self.push_wake() + await self.wake() except Exception as _wk_err: - logger.warning( - "kanban notifier: wakeup injection failed for %s: %s", - self.task_id, _wk_err, exc_info=True, - ) + logger.warning("kanban notifier: wakeup injection failed for %s: %s", self.task_id, _wk_err, exc_info=True) # Unsubscribe only on archive; ``done`` is reversible. - if task_terminal: + if self.task and self.task.status == "archived": await self.unsub() diff --git a/gateway/rich_sent_store.py b/gateway/rich_sent_store.py index ebd6a1a71a..645c542db7 100644 --- a/gateway/rich_sent_store.py +++ b/gateway/rich_sent_store.py @@ -1,13 +1,8 @@ -"""Local index of text we've sent via ``sendRichMessage`` (Bot API 10.1). - -Telegram does NOT echo a rich message's content back in ``reply_to_message`` -(``.text``/``.caption`` empty, ``.api_kwargs`` None), so replies to rich sends -arrive with no quotable text. We remember ``message_id -> text`` at send time -and look it up by ``reply_to_id`` on inbound. This module is the single source -of truth for that index. - -Best-effort and dependency-free: every operation swallows errors and degrades -to a no-op / ``None`` so it can never break a send or an inbound message. +"""Local index of text we've sent via ``sendRichMessage`` (Bot API 10.1). Telegram does NOT echo a rich +message's content back in ``reply_to_message`` (``.text``/``.caption`` empty, ``.api_kwargs`` None), so +replies to rich sends arrive with no quotable text; we remember ``message_id -> text`` at send time and +look it up by ``reply_to_id`` on inbound. Best-effort and dependency-free: every operation swallows +errors and degrades to a no-op / ``None`` so it can never break a send or an inbound message. """ from __future__ import annotations @@ -22,16 +17,10 @@ _MAX_TEXT_CHARS = 2000 def _store_path() -> str: - # get_hermes_home() honors the active profile override. - from hermes_constants import get_hermes_home - + from hermes_constants import get_hermes_home # honors the active profile override return os.path.join(str(get_hermes_home()), "state", "rich_sent_index.json") -def _key(chat_id, message_id) -> str: - return f"{chat_id}:{message_id}" - - def _load(path: str) -> dict: try: with open(path, "r", encoding="utf-8") as fh: @@ -49,10 +38,9 @@ def record(chat_id, message_id, text: Optional[str]) -> None: try: os.makedirs(os.path.dirname(path), exist_ok=True) data = _load(path) - data[_key(chat_id, message_id)] = {"t": text[:_MAX_TEXT_CHARS], "ts": int(time.time())} + data[f"{chat_id}:{message_id}"] = {"t": text[:_MAX_TEXT_CHARS], "ts": int(time.time())} if len(data) > _MAX_ENTRIES: # trim oldest by timestamp - by_age = sorted(data.items(), key=lambda kv: kv[1].get("ts", 0)) - for k, _ in by_age[: len(data) - _MAX_ENTRIES]: + for k, _ in sorted(data.items(), key=lambda kv: kv[1].get("ts", 0))[: len(data) - _MAX_ENTRIES]: data.pop(k, None) tmp = f"{path}.tmp.{os.getpid()}" with open(tmp, "w", encoding="utf-8") as fh: @@ -66,5 +54,5 @@ def lookup(chat_id, message_id) -> Optional[str]: """Return stored text for ``(chat_id, message_id)`` or ``None``.""" if message_id is None or chat_id is None: return None - entry = _load(_store_path()).get(_key(chat_id, message_id)) + entry = _load(_store_path()).get(f"{chat_id}:{message_id}") return (entry.get("t") or None) if isinstance(entry, dict) else None