From f4261c229b4a1572bb1fe0f0cfdb5dbc9362edca Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:18:31 -0700 Subject: [PATCH 1/7] refactor(hermes_cli): compact active_sessions lease registry (775->~695 LOC), unify lease-drop/session-held helpers --- hermes_cli/active_sessions.py | 236 ++++++++++++---------------------- 1 file changed, 81 insertions(+), 155 deletions(-) diff --git a/hermes_cli/active_sessions.py b/hermes_cli/active_sessions.py index 015b8a636f..a375f8567a 100644 --- a/hermes_cli/active_sessions.py +++ b/hermes_cli/active_sessions.py @@ -1,6 +1,6 @@ """Cross-process active chat session leases. -The session database records persisted conversations. This module records +The session database records persisted conversations; this module records currently open chat surfaces, including idle CLI/TUI sessions that have not written a transcript row yet. """ @@ -31,14 +31,9 @@ def coerce_max_concurrent_sessions(value: Any, key: str = "max_concurrent_sessio """Return a positive integer cap, or None when disabled/invalid.""" if value is None: return None - if isinstance(value, bool): - logger.warning( - "Ignoring invalid %s=%r (expected a positive integer; 0/null disables)", - key, - value, - ) - return None try: + if isinstance(value, bool): + raise ValueError(value) if isinstance(value, float): if not value.is_integer(): raise ValueError(value) @@ -49,14 +44,10 @@ def coerce_max_concurrent_sessions(value: Any, key: str = "max_concurrent_sessio parsed = int(value) except (TypeError, ValueError): logger.warning( - "Ignoring invalid %s=%r (expected a positive integer; 0/null disables)", - key, - value, + "Ignoring invalid %s=%r (expected a positive integer; 0/null disables)", key, value ) return None - if parsed <= 0: - return None - return parsed + return parsed if parsed > 0 else None def resolve_max_concurrent_sessions(config: Any) -> Optional[int]: @@ -103,9 +94,7 @@ def summarize_holders(entries: list[dict[str, Any]]) -> str: def active_session_limit_message( - active_count: int, - max_sessions: int, - entries: Optional[list[dict[str, Any]]] = None, + active_count: int, max_sessions: int, entries: Optional[list[dict[str, Any]]] = None ) -> str: # Name the holders: slots are shared across CLI, desktop/TUI and gateway, # so the rejected surface is usually NOT the one squatting on them. @@ -117,10 +106,6 @@ def active_session_limit_message( ) -def _registry_home(registry_home: str | Path | None = None) -> Path: - return Path(registry_home) if registry_home is not None else Path(get_hermes_home()) - - # Machine-readable refusal reasons: the reason is the contract, the message is # for people. Capacity = "busy, come back later"; ownership = "this session # has a live owner and writing would interleave with theirs". @@ -161,9 +146,7 @@ def _is_same_writer(entry: dict[str, Any], metadata: Optional[dict[str, Any]]) - return False existing_live = str((entry.get("metadata") or {}).get("live_session_id") or "") incoming_live = str((metadata or {}).get("live_session_id") or "") - if not existing_live or not incoming_live: - return False - return existing_live == incoming_live + return bool(existing_live and incoming_live) and existing_live == incoming_live def session_already_owned_message(session_id: str, entry: dict[str, Any]) -> str: @@ -178,21 +161,20 @@ def session_already_owned_message(session_id: str, entry: dict[str, Any]) -> str ) -def _state_dir(registry_home: str | Path | None = None) -> Path: - return _registry_home(registry_home) / "runtime" +def _registry_home(registry_home: str | Path | None = None) -> Path: + return Path(registry_home) if registry_home is not None else Path(get_hermes_home()) def _state_path(registry_home: str | Path | None = None) -> Path: - return _state_dir(registry_home) / "active_sessions.json" + return _registry_home(registry_home) / "runtime" / "active_sessions.json" def _lock_path(registry_home: str | Path | None = None) -> Path: - return _state_dir(registry_home) / "active_sessions.lock" + return _registry_home(registry_home) / "runtime" / "active_sessions.lock" def _lease_paths( - lease: Optional["ActiveSessionLease"] = None, - registry_home: str | Path | None = None, + lease: Optional["ActiveSessionLease"] = None, registry_home: str | Path | None = None ) -> tuple[Path, Path]: if lease is not None and lease.state_path is not None and lease.lock_path is not None: return lease.state_path, lease.lock_path @@ -243,6 +225,9 @@ class _FileLock: def _read_entries(path: Path, *, strict: bool = False) -> list[dict[str, Any]]: + def invalid(what: str) -> ActiveSessionRegistryError: + return ActiveSessionRegistryError(f"active session registry {what}: {path}") + try: with open(path, "r", encoding="utf-8") as fh: data = json.load(fh) @@ -250,25 +235,19 @@ def _read_entries(path: Path, *, strict: bool = False) -> list[dict[str, Any]]: return [] except Exception as exc: if strict: - raise ActiveSessionRegistryError( - f"active session registry unreadable: {path}" - ) from exc + raise invalid("unreadable") from exc logger.warning("Ignoring corrupt active session registry at %s", path) return [] entries = data.get("entries") if isinstance(data, dict) else data if not isinstance(entries, list): if strict: - raise ActiveSessionRegistryError( - f"active session registry has invalid shape: {path}" - ) + raise invalid("has invalid shape") return [] valid = [entry for entry in entries if isinstance(entry, dict)] if not strict: return valid if len(valid) != len(entries): - raise ActiveSessionRegistryError( - f"active session registry contains invalid entries: {path}" - ) + raise invalid("contains invalid entries") seen_leases: set[str] = set() for entry in valid: lease_id = entry.get("lease_id") @@ -285,9 +264,7 @@ def _read_entries(path: Path, *, strict: bool = False) -> list[dict[str, Any]]: (lambda: not _valid_process_start(entry.get("process_start_time")), "an invalid process start time"), ): if bad(): - raise ActiveSessionRegistryError( - f"active session registry contains {what}: {path}" - ) + raise invalid(f"contains {what}") seen_leases.add(lease_id) return valid @@ -357,18 +334,19 @@ def _pid_liveness(pid: Any, process_start_time: Any = None, *, lenient: bool = F ``lenient`` never returns None: an unparseable pid or a failed existence probe counts as dead, an unreadable current start time as alive. """ + unknown_dead = False if lenient else None try: pid_int = int(pid) except (TypeError, ValueError): - return False if lenient else None + return unknown_dead if pid_int <= 0: - return False if lenient else None + return unknown_dead try: from gateway.status import _pid_exists exists = bool(_pid_exists(pid_int)) except Exception: - return False if lenient else None + return unknown_dead if not exists: return False expected_start = _optional_float(process_start_time) @@ -380,26 +358,17 @@ def _pid_liveness(pid: Any, process_start_time: Any = None, *, lenient: bool = F return abs(current_start - expected_start) < 0.001 -def _pid_alive(pid: Any, process_start_time: Any = None) -> bool: - return bool(_pid_liveness(pid, process_start_time, lenient=True)) - - -def _prune_dead( - entries: list[dict[str, Any]], *, strict: bool = False -) -> list[dict[str, Any]]: +def _prune_dead(entries: list[dict[str, Any]], *, strict: bool = False) -> list[dict[str, Any]]: + """Keep entries whose owner is alive; tracked/strict entries must be provably so.""" live: list[dict[str, Any]] = [] for entry in entries: - tracked = bool(entry.get("track_liveness")) - if strict or tracked: - state = _pid_liveness(entry.get("pid"), entry.get("process_start_time")) - if state is None: - raise ActiveSessionRegistryError( - "active session owner liveness is unknown" - ) - if state: - live.append(entry) - continue - if _pid_alive(entry.get("pid"), entry.get("process_start_time")): + tracked = strict or bool(entry.get("track_liveness")) + state = _pid_liveness( + entry.get("pid"), entry.get("process_start_time"), lenient=not tracked + ) + if state is None: + raise ActiveSessionRegistryError("active session owner liveness is unknown") + if state: live.append(entry) return live @@ -428,8 +397,19 @@ def _clean_metadata(metadata: dict[str, Any]) -> dict[str, Any]: return {str(k): v for k, v in metadata.items() if isinstance(k, str)} -def _without_lease(entries: list[dict[str, Any]], lease_id: str) -> list[dict[str, Any]]: - return [e for e in entries if str(e.get("lease_id") or "") != lease_id] +def _drop_lease( + state_path: Path, entries: list[dict[str, Any]], lease_id: str +) -> list[dict[str, Any]]: + """Remove ``lease_id`` from ``entries``, writing the registry only if it was present.""" + kept = [e for e in entries if str(e.get("lease_id") or "") != lease_id] + if len(kept) != len(entries): + _write_entries(state_path, kept) + return kept + + +def _holds_session(entries: list[dict[str, Any]], session_id: str) -> bool: + target = str(session_id or "") + return bool(target) and any(str(e.get("session_id") or "") == target for e in entries) def _read_live_entries( @@ -503,28 +483,17 @@ def try_acquire_active_session( # refuses empty session ids): hand back a no-op lease. if not key and not track_liveness: return ActiveSessionLease( - lease_id=lease_id, - session_id=key, - surface=str(surface), - enabled=False, + lease_id=lease_id, session_id=key, surface=str(surface), enabled=False ), None entry = _lease_entry( - lease_id=lease_id, - session_id=key, - surface=str(surface), - metadata=metadata, + lease_id=lease_id, session_id=key, surface=str(surface), metadata=metadata, track_liveness=track_liveness, ) - state_path, lock_path = _lease_paths(registry_home=registry_home) lease = ActiveSessionLease( - lease_id=lease_id, - session_id=key, - surface=str(surface), - state_path=state_path, - lock_path=lock_path, - track_liveness=track_liveness, + lease_id=lease_id, session_id=key, surface=str(surface), state_path=state_path, + lock_path=lock_path, track_liveness=track_liveness, ) with _FileLock(lock_path): # A capacity cap could degrade open; exclusivity cannot: "could not @@ -536,11 +505,9 @@ def try_acquire_active_session( ) if loaded is None: return None, ActiveSessionRefusal( - ( - "Hermes could not read the active-session registry at " - f"{state_path}, so it cannot prove this session has no other " - "live owner. Fix or remove that file and try again." - ), + "Hermes could not read the active-session registry at " + f"{state_path}, so it cannot prove this session has no other " + "live owner. Fix or remove that file and try again.", SESSION_COORDINATION_UNAVAILABLE, ) raw_entries, entries = loaded @@ -555,7 +522,6 @@ def try_acquire_active_session( for index, existing in enumerate(entries): if str(existing.get("session_id") or "") != key: continue - # The same writer is not a second writer: a live session that # leaked its lease reference would otherwise be fenced out of # its own session permanently (pruning only removes entries @@ -564,34 +530,27 @@ def try_acquire_active_session( entries[index] = entry _write_entries(state_path, entries) return lease, None - _write_entries(state_path, entries) logger.info( "Refused active session %s: already held by pid=%s surface=%s", - key, - existing.get("pid"), - existing.get("surface"), + key, existing.get("pid"), existing.get("surface"), ) return None, ActiveSessionRefusal( - session_already_owned_message(key, existing), - SESSION_NOT_OWNED, + session_already_owned_message(key, existing), SESSION_NOT_OWNED ) # Capacity second, and only when an operator asked for one. - if max_sessions is not None: + if max_sessions is not None and len(entries) >= max_sessions: active_count = len(entries) - if active_count >= max_sessions: - _write_entries(state_path, entries) - logger.info( - "Active session limit reached: active=%d max=%d surface=%s", - active_count, - max_sessions, - surface, - ) - return None, ActiveSessionRefusal( - active_session_limit_message(active_count, max_sessions, entries), - MAX_CONCURRENT_SESSIONS, - ) + _write_entries(state_path, entries) + logger.info( + "Active session limit reached: active=%d max=%d surface=%s", + active_count, max_sessions, surface, + ) + return None, ActiveSessionRefusal( + active_session_limit_message(active_count, max_sessions, entries), + MAX_CONCURRENT_SESSIONS, + ) entries.append(entry) _write_entries(state_path, entries) @@ -600,7 +559,7 @@ def try_acquire_active_session( def release_active_session(lease: ActiveSessionLease) -> None: # Prefer the registry the lease was acquired against: the caller may be - # running under a profile HERMES_HOME override (#85431). + # running under a profile HERMES_HOME override. state_path, lock_path = _lease_paths(lease) with _FileLock(lock_path): if lease.released: @@ -611,24 +570,16 @@ def release_active_session(lease: ActiveSessionLease) -> None: "releasing an untracked lease", ) if loaded is not None: - entries = loaded[1] - kept = _without_lease(entries, lease.lease_id) - if len(kept) != len(entries): - _write_entries(state_path, kept) + _drop_lease(state_path, loaded[1], lease.lease_id) lease.released = True def transfer_active_session( - lease: ActiveSessionLease, - *, - session_id: str, - metadata: Optional[dict[str, Any]] = None, + lease: ActiveSessionLease, *, session_id: str, metadata: Optional[dict[str, Any]] = None ) -> bool: """Move an existing lease to a new session id without dropping the slot.""" new_session_id = str(session_id or "") - if not new_session_id: - return False - if lease.released: + if not new_session_id or lease.released: return False if not lease.enabled: lease.session_id = new_session_id @@ -659,15 +610,10 @@ def transfer_active_session( updated = True break if not updated and lease.track_liveness: - entries.append( - _lease_entry( - lease_id=lease.lease_id, - session_id=new_session_id, - surface=lease.surface, - metadata=metadata, - track_liveness=True, - ) - ) + entries.append(_lease_entry( + lease_id=lease.lease_id, session_id=new_session_id, surface=lease.surface, + metadata=metadata, track_liveness=True, + )) updated = True if updated: _write_entries(state_path, entries) @@ -698,10 +644,8 @@ def release_orphaned_leases(live_lease_ids: set[str]) -> int: return 0 entries = loaded[1] kept = [ - entry - for entry in entries - if entry.get("pid") != pid - or str(entry.get("lease_id") or "") in live_lease_ids + entry for entry in entries + if entry.get("pid") != pid or str(entry.get("lease_id") or "") in live_lease_ids ] dropped = len(entries) - len(kept) if dropped: @@ -724,52 +668,34 @@ def active_session_registry_snapshot( @contextmanager def active_session_liveness_guard( - session_id: str, - *, - registry_home: str | Path | None = None, + session_id: str, *, registry_home: str | Path | None = None ) -> Iterator[bool]: """Hold the registry lock while reporting whether ``session_id`` is leased, so no new backend can acquire a lease between the check and the caller's ``end_session`` write.""" - target = str(session_id or "") state_path, lock_path = _lease_paths(registry_home=registry_home) with _FileLock(lock_path): entries = _prune_dead(_read_entries(state_path, strict=True), strict=True) _write_entries(state_path, entries) - yield bool(target) and any( - str(entry.get("session_id") or "") == target for entry in entries - ) + yield _holds_session(entries, session_id) @contextmanager def release_active_session_liveness_guard( - lease: ActiveSessionLease, - session_id: str, + lease: ActiveSessionLease, session_id: str ) -> Iterator[bool]: """Remove ``lease`` and hold its registry lock through a lifecycle write, making cleanup one atomic ownership decision (release, check siblings, end the durable row) before any new backend can reopen it.""" if not lease.enabled or lease.released: - with active_session_liveness_guard( - session_id, registry_home=_registry_home_for_lease(lease) - ) as active: + home = lease.state_path.parent.parent if lease.state_path is not None else None + with active_session_liveness_guard(session_id, registry_home=home) as active: yield active return - target = str(session_id or "") state_path, lock_path = _lease_paths(lease) with _FileLock(lock_path): entries = _prune_dead(_read_entries(state_path, strict=True), strict=True) - kept = _without_lease(entries, lease.lease_id) - if len(kept) != len(entries): - _write_entries(state_path, kept) + kept = _drop_lease(state_path, entries, lease.lease_id) lease.released = True - yield bool(target) and any( - str(entry.get("session_id") or "") == target for entry in kept - ) - - -def _registry_home_for_lease(lease: ActiveSessionLease) -> Path | None: - if lease.state_path is None: - return None - return lease.state_path.parent.parent + yield _holds_session(kept, session_id) From a68d82b5e1824d1aea111eaf06f941623c26301a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:47:03 -0700 Subject: [PATCH 2/7] refactor(hermes_cli): active_sessions signature/docstring compaction (695->676 LOC) --- hermes_cli/active_sessions.py | 99 +++++++++++++---------------------- 1 file changed, 37 insertions(+), 62 deletions(-) diff --git a/hermes_cli/active_sessions.py b/hermes_cli/active_sessions.py index a375f8567a..f3f5531dea 100644 --- a/hermes_cli/active_sessions.py +++ b/hermes_cli/active_sessions.py @@ -106,25 +106,24 @@ def active_session_limit_message( ) -# Machine-readable refusal reasons: the reason is the contract, the message is -# for people. Capacity = "busy, come back later"; ownership = "this session -# has a live owner and writing would interleave with theirs". +# Machine-readable refusal reasons (the reason is the contract, the message is for +# people). Capacity = "busy, come back later"; ownership = "a live owner exists and +# writing would interleave with theirs". SESSION_NOT_OWNED = "SESSION_NOT_OWNED" MAX_CONCURRENT_SESSIONS = "MAX_CONCURRENT_SESSIONS" -# Ownership could not be PROVEN (registry unreadable/corrupt). Deliberately -# distinct from SESSION_NOT_OWNED: collapsing "can't tell" into a silent -# go-ahead is the fail-open hole that let two writers share one session. +# Ownership could not be PROVEN (registry unreadable/corrupt). Deliberately distinct +# from SESSION_NOT_OWNED: treating "can't tell" as a go-ahead is the fail-open hole +# that let two writers share one session. SESSION_COORDINATION_UNAVAILABLE = "SESSION_COORDINATION_UNAVAILABLE" -# Advertised through the gateway. A module constant, not a config flag: it is -# true because try_acquire_active_session checks atomically, so it cannot drift -# out of step with the enforcement without this file changing. +# Advertised through the gateway. A module constant, not a config flag: it holds +# because try_acquire_active_session checks atomically, so it cannot drift from the +# enforcement without this file changing. PER_SESSION_EXCLUSIVE_SUBMIT = True class ActiveSessionRefusal(str): - """A refusal message (``str`` subclass, so existing callers are untouched) - that also carries a machine-readable ``reason``.""" + """Refusal message (a ``str``, so callers are untouched) with a machine-readable ``reason``.""" reason: str @@ -136,9 +135,8 @@ class ActiveSessionRefusal(str): def _is_same_writer(entry: dict[str, Any], metadata: Optional[dict[str, Any]]) -> bool: """True when an existing lease belongs to the very caller re-acquiring it. - Identity is (pid, live_session_id): pid alone would let two live sessions in - one process steal each other's lease; the live id alone would let another - process with an equal id do the same.""" + Identity is (pid, live_session_id): pid alone lets two live sessions in one process + steal each other's lease; the live id alone lets another process with an equal id.""" try: if int(entry.get("pid") or -1) != os.getpid(): return False @@ -186,12 +184,10 @@ def _flock(fh, *, lock: bool) -> None: """Exclusive whole-file lock/unlock on ``fh`` (fcntl on POSIX, msvcrt on Windows).""" if os.name == "nt": import msvcrt - fh.seek(0) msvcrt.locking(fh.fileno(), msvcrt.LK_LOCK if lock else msvcrt.LK_UNLCK, 1) else: import fcntl - fcntl.flock(fh.fileno(), fcntl.LOCK_EX if lock else fcntl.LOCK_UN) @@ -309,11 +305,10 @@ def _write_entries(path: Path, entries: list[dict[str, Any]]) -> None: def _process_start_time(pid: int) -> Optional[float]: - # Pair pid with process create_time when psutil can read it, so a recycled - # pid does not keep a stale lease alive indefinitely. + # Pair pid with create_time when psutil can read it, so a recycled pid does not + # keep a stale lease alive indefinitely. try: import psutil # type: ignore - return float(psutil.Process(pid).create_time()) except Exception: return None @@ -329,11 +324,8 @@ def _optional_float(value: Any) -> Optional[float]: def _pid_liveness(pid: Any, process_start_time: Any = None, *, lenient: bool = False) -> Optional[bool]: - """Return True/False for live/dead, or None when liveness is unknowable. - - ``lenient`` never returns None: an unparseable pid or a failed existence - probe counts as dead, an unreadable current start time as alive. - """ + """True/False for live/dead, or None when unknowable. ``lenient`` never returns None: + an unparseable pid or failed existence probe counts as dead, an unreadable start as alive.""" unknown_dead = False if lenient else None try: pid_int = int(pid) @@ -343,7 +335,6 @@ def _pid_liveness(pid: Any, process_start_time: Any = None, *, lenient: bool = F return unknown_dead try: from gateway.status import _pid_exists - exists = bool(_pid_exists(pid_int)) except Exception: return unknown_dead @@ -380,9 +371,9 @@ class ActiveSessionLease: surface: str enabled: bool = True released: bool = False - # Pinned at acquisition: a lease acquired under the root HERMES_HOME must - # release against the same registry even when release() runs inside a - # profile-home override, or phantom leases fill the session cap. + # Pinned at acquisition: a lease taken under the root HERMES_HOME must release + # against the same registry even inside a profile-home override, or phantom + # leases fill the session cap. state_path: Optional[Path] = None lock_path: Optional[Path] = None track_liveness: bool = False @@ -417,9 +408,8 @@ def _read_live_entries( ) -> Optional[tuple[list[dict[str, Any]], list[dict[str, Any]]]]: """``(raw, pruned)`` from the registry, or None when it is unreadable. - Liveness-tracked callers re-raise instead: they must not proceed on an - unprovable registry. Untracked callers get ``warn`` logged and decide - how to degrade themselves. + Liveness-tracked callers re-raise instead (they must not proceed on an unprovable + registry); untracked callers get ``warn`` logged and decide how to degrade. """ try: raw_entries = _read_entries(state_path, strict=True) @@ -432,12 +422,8 @@ def _read_live_entries( def _lease_entry( - *, - lease_id: str, - session_id: str, - surface: str, - metadata: Optional[dict[str, Any]] = None, - track_liveness: bool = False, + *, lease_id: str, session_id: str, surface: str, + metadata: Optional[dict[str, Any]] = None, track_liveness: bool = False, ) -> dict[str, Any]: now = time.time() entry: dict[str, Any] = { @@ -457,23 +443,15 @@ def _lease_entry( def try_acquire_active_session( - *, - session_id: str, - surface: str, - config: Any, - metadata: Optional[dict[str, Any]] = None, - registry_home: str | Path | None = None, - track_liveness: bool = False, + *, session_id: str, surface: str, config: Any, metadata: Optional[dict[str, Any]] = None, + registry_home: str | Path | None = None, track_liveness: bool = False, ) -> tuple[Optional[ActiveSessionLease], Optional[str]]: - """Acquire an active-session slot. + """Acquire an active-session slot: ``(lease, None)`` or ``(None, ActiveSessionRefusal)``. - Per-session exclusivity is CORRECTNESS, enforced unconditionally: at most - one live owner per stored session. ``max_concurrent_sessions`` is resource - POLICY and applies only when configured. ``registry_home`` lets - profile-scoped backends share the owning profile's registry. - - Returns ``(lease, None)`` or ``(None, ActiveSessionRefusal)``. Ownership - uncertainty fails CLOSED with ``SESSION_COORDINATION_UNAVAILABLE``. + Per-session exclusivity is CORRECTNESS, enforced unconditionally (at most one live + owner per stored session); ``max_concurrent_sessions`` is resource POLICY, applied + only when configured. ``registry_home`` lets profile-scoped backends share the owning + profile's registry. Ownership uncertainty fails CLOSED (SESSION_COORDINATION_UNAVAILABLE). """ max_sessions = resolve_max_concurrent_sessions(config) lease_id = uuid.uuid4().hex @@ -624,10 +602,9 @@ def transfer_active_session( def release_orphaned_leases(live_lease_ids: set[str]) -> int: """Drop this process's registry entries that no live session owns. - ``_prune_dead`` only reclaims leases of dead processes; a days-long server - never trips it, so a lease whose session skipped teardown is held until - restart. The owning process is the only authority on its own leases — - exact, no heartbeat on the turn path, no staleness threshold. + ``_prune_dead`` only reclaims leases of dead processes, so on a days-long server a + lease whose session skipped teardown is held until restart. The owning process is the + only authority on its own leases — exact, no heartbeat on the turn path, no threshold. """ pid = os.getpid() state_path = _state_path() @@ -670,9 +647,8 @@ def active_session_registry_snapshot( def active_session_liveness_guard( session_id: str, *, registry_home: str | Path | None = None ) -> Iterator[bool]: - """Hold the registry lock while reporting whether ``session_id`` is leased, - so no new backend can acquire a lease between the check and the caller's - ``end_session`` write.""" + """Hold the registry lock while reporting whether ``session_id`` is leased, so no + new backend can acquire a lease between the check and the caller's ``end_session``.""" state_path, lock_path = _lease_paths(registry_home=registry_home) with _FileLock(lock_path): entries = _prune_dead(_read_entries(state_path, strict=True), strict=True) @@ -684,9 +660,8 @@ def active_session_liveness_guard( def release_active_session_liveness_guard( lease: ActiveSessionLease, session_id: str ) -> Iterator[bool]: - """Remove ``lease`` and hold its registry lock through a lifecycle write, - making cleanup one atomic ownership decision (release, check siblings, - end the durable row) before any new backend can reopen it.""" + """Remove ``lease`` and hold its registry lock through a lifecycle write, making + cleanup one atomic decision (release, check siblings, end the durable row).""" if not lease.enabled or lease.released: home = lease.state_path.parent.parent if lease.state_path is not None else None with active_session_liveness_guard(session_id, registry_home=home) as active: From c375d0ed5e59c37e1f9b300f485fc99a45cdea18 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:00:34 -0700 Subject: [PATCH 3/7] refactor(hermes_cli): simplify agent_import/agent_plugins (1369->1020 LOC): inline single-use helpers, unify permission/memory import paths, dispatch-driven report --- hermes_cli/agent_import.py | 614 +++++++++++------------------------- hermes_cli/agent_plugins.py | 261 +++++---------- 2 files changed, 263 insertions(+), 612 deletions(-) diff --git a/hermes_cli/agent_import.py b/hermes_cli/agent_import.py index 3e6d61907a..5b01e0fff3 100644 --- a/hermes_cli/agent_import.py +++ b/hermes_cli/agent_import.py @@ -1,8 +1,8 @@ """hermes import-agent — import Claude Code / Codex CLI setups into Hermes. -Secrets are NEVER imported: credential files (.credentials.json, auth.json) are ignored, and MCP -server env vars with secret-looking names (KEY, TOKEN, SECRET, PASSWORD, ...) are stripped and -reported so the user can re-add them deliberately via ``hermes setup`` or config.yaml. +Secrets are NEVER imported: credential files are never read, and MCP env vars with secret-looking +names (KEY, TOKEN, SECRET, PASSWORD, ...) are stripped and reported so the user re-adds them via +``hermes setup`` or config.yaml. """ from __future__ import annotations @@ -13,42 +13,28 @@ import re import shutil import sys import time +import tomllib from pathlib import Path from typing import Any, Dict, List, Optional, Sequence, Tuple +import yaml + from utils import atomic_write_text, atomic_yaml_write logger = logging.getLogger(__name__) -# Same entry delimiter as the Hermes memory store and the openclaw migration -# script — memories/MEMORY.md entries are separated by bare "§" lines. +# Entry delimiter of the Hermes memory store (memories/MEMORY.md) and the openclaw script. ENTRY_DELIMITER = "\n§\n" - -# Character budget for merged memory files (matches the openclaw script's -# default memory limit). +# Character budget for merged memory files (openclaw script default). MEMORY_CHAR_LIMIT = 20_000 - SUPPORTED_AGENTS = ("claude-code", "codex") - -_AGENT_DEFAULT_DIRS = { - "claude-code": ".claude", - "codex": ".codex", -} - -_SKILL_CATEGORY = { - "claude-code": "claude-code-imports", - "codex": "codex-imports", -} +_AGENT_DEFAULT_DIRS = {"claude-code": ".claude", "codex": ".codex"} +_SKILL_CATEGORY = {"claude-code": "claude-code-imports", "codex": "codex-imports"} # Env var names that look like credentials — never copied into config.yaml. _SECRET_KEY_RE = re.compile( r"(?:^|_)(?:API[_-]?KEY|APIKEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|" - r"AUTH|PRIVATE[_-]?KEY|ACCESS[_-]?KEY)(?:_|$)|KEY$", - re.IGNORECASE, -) - -# Files inside the source tree that hold credentials — never read. -_CREDENTIAL_FILENAMES = (".credentials.json", "auth.json", "credentials.json") + r"AUTH|PRIVATE[_-]?KEY|ACCESS[_-]?KEY)(?:_|$)|KEY$", re.IGNORECASE) def is_secret_key(key: str) -> bool: @@ -65,92 +51,59 @@ def read_text(path: Path) -> str: class ConfigReadError(RuntimeError): - """An existing config file is present but cannot be read or parsed. - - Signals that a read-modify-write round trip must be abandoned: the caller has no idea what the - file holds, so writing a merged result back would replace real settings with only the keys it - merged. - """ + """Existing config file present but unreadable/unparseable: the read-modify-write round trip + must be abandoned, else the merged result would replace real settings with only merged keys.""" def load_yaml_file(path: Path) -> Dict[str, Any]: - """Load a YAML mapping, distinguishing "absent" from "unreadable". - - - Absent, or present but empty -> ``{}``; first-time creation still works. - Present but - unreadable, unparseable, or not a mapping -> raise :class:`ConfigReadError` so the caller - refuses and leaves the file byte-identical. - """ - import yaml - + """Load a YAML mapping: absent/empty -> ``{}``; unreadable/unparseable/non-mapping -> + :class:`ConfigReadError` so the caller refuses and leaves the file byte-identical.""" if not path.exists(): return {} fix_hint = "Fix it with `hermes config edit` (or move it aside), then re-run the import." + + def refusal(detail: str) -> ConfigReadError: + return ConfigReadError(f"Refusing to overwrite {path}: {detail}") + try: raw = read_text(path) except OSError as exc: - raise ConfigReadError( - f"Refusing to overwrite {path}: the existing file cannot be read " - f"({exc}). Fix the file permissions or move it aside first." - ) from exc + raise refusal(f"the existing file cannot be read ({exc}). " + "Fix the file permissions or move it aside first.") from exc try: data = yaml.safe_load(raw) except yaml.YAMLError as exc: - raise ConfigReadError( - f"Refusing to overwrite {path}: the existing file is not valid YAML ({exc}). {fix_hint}" - ) from exc - # An empty file parses to None — a legitimate state with nothing to lose. - if data is None: + raise refusal(f"the existing file is not valid YAML ({exc}). {fix_hint}") from exc + if data is None: # empty file — a legitimate state with nothing to lose return {} if not isinstance(data, dict): - raise ConfigReadError( - f"Refusing to overwrite {path}: expected the existing file to hold a " - f"YAML mapping but found {type(data).__name__}. {fix_hint}" - ) + raise refusal(f"expected the existing file to hold a YAML mapping but found " + f"{type(data).__name__}. {fix_hint}") return data def dump_yaml_file(path: Path, data: Dict[str, Any]) -> None: - """Write ``data`` as YAML atomically (temp file + fsync + rename). - - Only ever reached after :func:`load_yaml_file` has successfully read the same path, so the - mapping being written is the real file's content plus the merged section — never a silently- - empty stand-in. - """ + """Atomic YAML write; only reached after :func:`load_yaml_file` succeeded on the same path.""" atomic_yaml_write(path, data) -# --------------------------------------------------------------------------- -# Memory-entry primitives (ported from openclaw_to_hermes.py) -# --------------------------------------------------------------------------- - def extract_markdown_entries(text: str) -> List[str]: - """Split a markdown document into individual memory entries. - - Headings become context prefixes; bullets and paragraphs become entries; code blocks and - tables are skipped. - """ + """Split markdown into memory entries: headings become context prefixes; bullets and + paragraphs become entries; code blocks and tables are skipped.""" entries: List[str] = [] headings: List[str] = [] paragraph_lines: List[str] = [] - def context_prefix() -> str: - filtered = [ - h for h in headings - if h and not re.search( - r"\b(MEMORY|USER|SOUL|AGENTS|TOOLS|IDENTITY|CLAUDE)\.md\b", - h, re.I, - ) - ] - return " > ".join(filtered) - def add_entry(content: str) -> None: - prefix = context_prefix() + prefix = " > ".join( + h for h in headings + if h and not re.search(r"\b(MEMORY|USER|SOUL|AGENTS|TOOLS|IDENTITY|CLAUDE)\.md\b", + h, re.I)) entries.append(f"{prefix}: {content}" if prefix else content) def flush_paragraph() -> None: - nonlocal paragraph_lines block = " ".join(line.strip() for line in paragraph_lines).strip() - paragraph_lines = [] + paragraph_lines.clear() if block: add_entry(block) @@ -158,98 +111,54 @@ def extract_markdown_entries(text: str) -> List[str]: for raw_line in (text or "").splitlines(): line = raw_line.rstrip() stripped = line.strip() - if stripped.startswith("```"): in_code_block = not in_code_block flush_paragraph() continue if in_code_block: continue - heading_match = re.match(r"^(#{1,6})\s+(.*\S)\s*$", stripped) if heading_match: flush_paragraph() - level = len(heading_match.group(1)) - value = heading_match.group(2).strip() - while len(headings) >= level: - headings.pop() - headings.append(value) + # Drop deeper (and same-level) headings, then push this one. + headings[len(heading_match.group(1)) - 1:] = [heading_match.group(2).strip()] continue - bullet_match = re.match(r"^\s*(?:[-*]|\d+\.)\s+(.*\S)\s*$", line) if bullet_match: flush_paragraph() add_entry(bullet_match.group(1).strip()) continue - - if not stripped: - flush_paragraph() + if not stripped or (stripped.startswith("|") and stripped.endswith("|")): + flush_paragraph() # blank line or table row ends the paragraph continue - - if stripped.startswith("|") and stripped.endswith("|"): - flush_paragraph() - continue - paragraph_lines.append(stripped) flush_paragraph() - deduped: List[str] = [] seen = set() for entry in entries: normalized = normalize_text(entry) - if not normalized or normalized in seen: - continue - seen.add(normalized) - deduped.append(entry.strip()) + if normalized and normalized not in seen: + seen.add(normalized) + deduped.append(entry.strip()) return deduped def parse_existing_memory_entries(path: Path) -> List[str]: - """Parse the DESTINATION memory store into entries. - - ``memories/MEMORY.md`` is the entry-delimited store written by ``MemoryStore._write_file`` - (tools/memory_tool.py), not a markdown document, so this splits on ``ENTRY_DELIMITER`` only — - exactly what ``MemoryStore._parse_entries`` does. - - Do NOT fall back to :func:`extract_markdown_entries` here. That extractor is correct for - CLAUDE.md / AGENTS.md *sources*, but it drops fenced code blocks and table rows and splits a - block into one entry per bullet — and the merged result is written straight back over the user's - store, so the loss is permanent. - """ + """Parse the DESTINATION memory store (``ENTRY_DELIMITER``-split, as ``MemoryStore`` does). + Do NOT fall back to :func:`extract_markdown_entries`: it drops code blocks/table rows and + splits bullets, and the merged result overwrites the user's store — the loss is permanent.""" if not path.exists(): return [] - raw = read_text(path) - if not raw.strip(): - return [] - return [e.strip() for e in raw.split(ENTRY_DELIMITER) if e.strip()] + return [e.strip() for e in read_text(path).split(ENTRY_DELIMITER) if e.strip()] -def backup_memory_file(path: Path) -> Optional[Path]: - """Snapshot ``path`` before a destructive rewrite; return the backup path. - - Restores parity with the openclaw migration script this module was ported from, which calls - ``maybe_backup(destination)`` before rewriting a memory store. Uses the same - ``.bak.`` naming as ``MemoryStore._backup_drifted_file``. Returns None when there - is nothing to back up. - """ - if not path.exists(): - return None - backup = path.with_suffix(path.suffix + f".bak.{int(time.time())}") - shutil.copy2(path, backup) - return backup - - -def merge_entries( - existing: Sequence[str], - incoming: Sequence[str], - limit: int, -) -> Tuple[List[str], Dict[str, int]]: +def merge_entries(existing: Sequence[str], incoming: Sequence[str], + limit: int) -> Tuple[List[str], Dict[str, int]]: merged = list(existing) seen = {normalize_text(e) for e in existing if e.strip()} stats = {"existing": len(existing), "added": 0, "duplicates": 0, "overflowed": 0} - - current_len = len(ENTRY_DELIMITER.join(merged)) if merged else 0 + current_len = len(ENTRY_DELIMITER.join(merged)) for entry in incoming: normalized = normalize_text(entry) if not normalized: @@ -257,10 +166,8 @@ def merge_entries( if normalized in seen: stats["duplicates"] += 1 continue - candidate_len = ( - len(entry) if not merged - else current_len + len(ENTRY_DELIMITER) + len(entry) - ) + candidate_len = (len(entry) if not merged + else current_len + len(ENTRY_DELIMITER) + len(entry)) if candidate_len > limit: stats["overflowed"] += 1 continue @@ -271,43 +178,22 @@ def merge_entries( return merged, stats -# --------------------------------------------------------------------------- -# Claude Code permission rules → Hermes command patterns -# --------------------------------------------------------------------------- - _BASH_RULE_RE = re.compile(r"^Bash\((?P.*)\)$") def claude_rule_to_command_pattern(rule: str) -> Optional[str]: - """Convert a Claude Code ``Bash(...)`` permission rule into a Hermes glob. - - ``Bash(npm run test:*)`` -> ``npm run test*`` (Claude ':*' is a prefix match). Bare ``Bash`` - and non-Bash rules (``Read(...)``, ``WebFetch(...)``) return None: the former is too broad to - import, the latter gate Claude-specific tools with no command-allowlist equivalent. - """ - rule = (rule or "").strip() - m = _BASH_RULE_RE.match(rule) - if not m: - return None - inner = m.group("inner").strip() + """``Bash(npm run test:*)`` -> ``npm run test*`` (Claude ':*' is a prefix match). Bare ``Bash`` + (too broad) and non-Bash rules (Claude-only tools, no allowlist equivalent) return None.""" + m = _BASH_RULE_RE.match((rule or "").strip()) + inner = m.group("inner").strip() if m else "" if not inner: return None - if inner.endswith(":*"): - inner = inner[:-2] + "*" - return inner - - -# --------------------------------------------------------------------------- -# Detection -# --------------------------------------------------------------------------- - -def default_source_dir(agent: str) -> Path: - return Path.home() / _AGENT_DEFAULT_DIRS[agent] + return inner[:-2] + "*" if inner.endswith(":*") else inner def detect_agents() -> List[str]: """Return the list of supported agents whose default dirs exist.""" - return [a for a in SUPPORTED_AGENTS if default_source_dir(a).is_dir()] + return [a for a in SUPPORTED_AGENTS if (Path.home() / _AGENT_DEFAULT_DIRS[a]).is_dir()] def sanitize_mcp_env(env: Any) -> Tuple[Dict[str, str], List[str]]: @@ -318,13 +204,6 @@ def sanitize_mcp_env(env: Any) -> Tuple[Dict[str, str], List[str]]: return kept, [str(k) for k in env if str(k) not in kept] -def _copy_skill_dir(skill_dir: Path, destination: Path) -> None: - destination.parent.mkdir(parents=True, exist_ok=True) - if destination.exists(): - shutil.rmtree(destination) - shutil.copytree(skill_dir, destination) - - def _translate_mcp_server(name: str, srv: Dict[str, Any]) -> Tuple[Dict[str, Any], List[str]]: """Map one Claude/Codex MCP server entry to Hermes shape; returns (server, stripped secret paths).""" hermes_srv: Dict[str, Any] = {} @@ -343,35 +222,20 @@ def _translate_mcp_server(name: str, srv: Dict[str, Any]) -> Tuple[Dict[str, Any hermes_srv["url"] = srv["url"] headers = srv.get("headers") if isinstance(headers, dict): - kept_headers = { - k: v for k, v in headers.items() - if not is_secret_key(str(k)) and "authorization" not in str(k).lower() - } + kept_headers = {k: v for k, v in headers.items() + if not is_secret_key(str(k)) and "authorization" not in str(k).lower()} if kept_headers: hermes_srv["headers"] = kept_headers stripped.extend(f"mcp_servers.{name}.headers.{k}" for k in headers if k not in kept_headers) return hermes_srv, stripped -# --------------------------------------------------------------------------- -# Importer -# --------------------------------------------------------------------------- - class AgentImporter: - """Detect/parse/map/apply importer for a single agent source tree. + """Detect/parse/map/apply importer for one agent source tree. ``execute=False`` runs the full + plan without touching disk; every item is recorded as imported/skipped/conflict/error.""" - ``execute=False`` runs the full plan without touching disk (dry run). Every item is recorded - as imported/skipped/conflict/error with a reason so the CLI can print a per-item report. - """ - - def __init__( - self, - agent: str, - source_root: Path, - target_root: Path, - execute: bool = False, - overwrite: bool = False, - ) -> None: + def __init__(self, agent: str, source_root: Path, target_root: Path, + execute: bool = False, overwrite: bool = False) -> None: if agent not in SUPPORTED_AGENTS: raise ValueError(f"Unsupported agent: {agent!r}") self.agent = agent @@ -382,27 +246,16 @@ class AgentImporter: self.items: List[Dict[str, Any]] = [] self.stripped_secrets: List[str] = [] - # -- reporting --------------------------------------------------------- - def record(self, kind: str, source, destination, status: str, reason: str = "", **details) -> None: - item: Dict[str, Any] = { - "kind": kind, - "source": str(source) if source else None, - "destination": str(destination) if destination else None, - "status": status, - "reason": reason, - } - item.update(details) - self.items.append(item) + self.items.append({"kind": kind, "source": str(source) if source else None, + "destination": str(destination) if destination else None, + "status": status, "reason": reason, **details}) - def load_target_config(self, kind: str, source, destination: Path - ) -> Optional[Dict[str, Any]]: - """Read the destination config.yaml, or record a refusal and return None. - - Deliberately runs in dry-run too: ``--dry-run`` must report the refusal, not preview an - ``imported`` that would destroy the config. - """ + def load_target_config(self, kind: str, source, + destination: Path) -> Optional[Dict[str, Any]]: + """Read the destination config.yaml, or record a refusal and return None. Runs in dry-run + too: ``--dry-run`` must report the refusal, not preview an ``imported`` that destroys it.""" try: return load_yaml_file(destination) except ConfigReadError as exc: @@ -412,88 +265,74 @@ class AgentImporter: def apply(self, kind: str, source, destination, would: str, action, details: Optional[Dict[str, Any]] = None) -> None: """Record ``imported`` (reason ``would`` in dry-run); in execute mode run ``action`` first. - ``action`` may add keys to ``details`` (shared by reference) and returns an error string - to record ``error`` instead, or None on success. - """ + (recorded as ``error``) or None.""" details = details or {} - if not self.execute: - self.record(kind, source, destination, "imported", would, **details) - return - error = action() - self.record(kind, source, destination, "error" if error else "imported", error or "", **details) + error = action() if self.execute else None + self.record(kind, source, destination, "error" if error else "imported", + error or ("" if self.execute else would), **details) def build_report(self) -> Dict[str, Any]: summary = {"imported": 0, "skipped": 0, "conflict": 0, "error": 0} for item in self.items: summary[item["status"]] = summary.get(item["status"], 0) + 1 - report: Dict[str, Any] = { - "agent": self.agent, - "source": str(self.source_root), - "target": str(self.target_root), - "dry_run": not self.execute, - "items": self.items, - "summary": summary, - } + report: Dict[str, Any] = {"agent": self.agent, "source": str(self.source_root), + "target": str(self.target_root), "dry_run": not self.execute, + "items": self.items, "summary": summary} if self.stripped_secrets: report["stripped_secrets"] = sorted(set(self.stripped_secrets)) return report - # -- orchestration ----------------------------------------------------- - def run(self) -> Dict[str, Any]: if not self.source_root.is_dir(): self.record("source", self.source_root, None, "error", "Source directory does not exist") - return self.build_report() - {"claude-code": self._run_claude_code, "codex": self._run_codex}[self.agent]() + else: + {"claude-code": self._run_claude_code, "codex": self._run_codex}[self.agent]() return self.build_report() def _run_claude_code(self) -> None: - settings = self._load_claude_settings() - self.import_context_file(self.source_root / "CLAUDE.md", kind="claude-md") - self.import_permission_allowlist(settings) - self.import_permission_denylist(settings) - self.import_mcp_servers(self._claude_mcp_servers(settings), kind="mcp-servers") + settings = self._load_source_mapping( + "settings", self.source_root / "settings.json", json.loads, self._JSON_ERRORS, + record_missing=True, non_mapping_error="settings.json is not a JSON object") + self.import_context_file(self.source_root / "CLAUDE.md", "claude-md") + self._import_permission_rules(settings, "allow") + self._import_permission_rules(settings, "deny") + # mcpServers: ~/.claude.json (preferred; lives NEXT TO ~/.claude/) then settings.json + claude_json = self._load_source_mapping( + "mcp-servers", self.source_root.parent / ".claude.json", json.loads, self._JSON_ERRORS) + servers: Dict[str, Any] = {} + for source in (claude_json.get("mcpServers"), settings.get("mcpServers")): + if isinstance(source, dict): + for name, srv in source.items(): + servers.setdefault(name, srv) + self.import_mcp_servers(servers, kind="mcp-servers") self.import_skills(self.source_root / "skills") commands_dir = self.source_root / "commands" if commands_dir.is_dir() and any(commands_dir.glob("*.md")): - self.record( - "slash-commands", commands_dir, None, "skipped", - "Claude slash commands have no direct Hermes equivalent — " - "consider converting them into skills", - ) + self.record("slash-commands", commands_dir, None, "skipped", + "Claude slash commands have no direct Hermes equivalent — " + "consider converting them into skills") def _run_codex(self) -> None: - config = self._load_codex_config() - self.import_context_file(self.source_root / "AGENTS.md", kind="agents-md") + config = self._load_source_mapping("config", self.source_root / "config.toml", + tomllib.loads, Exception, record_missing=True) + self.import_context_file(self.source_root / "AGENTS.md", "agents-md") mcp = config.get("mcp_servers") self.import_mcp_servers(mcp if isinstance(mcp, dict) else {}, kind="mcp-servers") - self.import_memories_dir(self.source_root / "memories") + memories_dir = self.source_root / "memories" + self._import_markdown_files( + "memories", memories_dir, + sorted(memories_dir.glob("*.md")) if memories_dir.is_dir() else None, + "No memories directory found") self.import_skills(self.source_root / "skills") - # -- parsers (fail soft: bad files become per-item error records) ------- - _JSON_ERRORS = (json.JSONDecodeError, OSError) - def _load_claude_settings(self) -> Dict[str, Any]: - return self._load_source_mapping( - "settings", self.source_root / "settings.json", json.loads, self._JSON_ERRORS, - record_missing=True, non_mapping_error="settings.json is not a JSON object", - ) - - def _load_codex_config(self) -> Dict[str, Any]: - import tomllib - - return self._load_source_mapping( - "config", self.source_root / "config.toml", tomllib.loads, Exception, - record_missing=True, - ) - def _load_source_mapping(self, kind: str, path: Path, parse, errors, *, record_missing: bool = False, non_mapping_error: str = "") -> Dict[str, Any]: - """Parse ``path`` into a mapping; problems become per-item error records, result ``{}``.""" + """Parse ``path`` into a mapping; problems become per-item error records and ``{}``.""" if not path.exists(): if record_missing: self.record(kind, None, None, "skipped", f"No {path.name} found") @@ -509,46 +348,20 @@ class AgentImporter: self.record(kind, path, None, "error", non_mapping_error) return {} - def _claude_mcp_servers(self, settings: Dict[str, Any]) -> Dict[str, Any]: - """Collect mcpServers from ~/.claude.json (preferred) and settings.json.""" - # ~/.claude.json lives NEXT TO ~/.claude/, not inside it - data = self._load_source_mapping( - "mcp-servers", self.source_root.parent / ".claude.json", json.loads, self._JSON_ERRORS) - servers: Dict[str, Any] = {} - if isinstance(data.get("mcpServers"), dict): - servers.update(data["mcpServers"]) - from_settings = settings.get("mcpServers") - if isinstance(from_settings, dict): - for name, srv in from_settings.items(): - servers.setdefault(name, srv) - return servers - - # -- mappers ------------------------------------------------------------- - def import_context_file(self, source: Path, kind: str) -> None: """CLAUDE.md / AGENTS.md → memory entries in memories/MEMORY.md.""" - if not source.exists(): - self.record(kind, None, self.target_root / "memories" / "MEMORY.md", - "skipped", f"No {source.name} found") - return - self._import_markdown_files(kind, source, [source], single_file=True) - - def import_memories_dir(self, memories_dir: Path) -> None: - """codex memories/*.md → memory entries in memories/MEMORY.md.""" - if not memories_dir.is_dir(): - self.record("memories", None, self.target_root / "memories" / "MEMORY.md", - "skipped", "No memories directory found") - return - self._import_markdown_files("memories", memories_dir, sorted(memories_dir.glob("*.md"))) - - def _import_markdown_files(self, kind: str, source: Path, files: List[Path], - single_file: bool = False) -> None: - """Extract entries from ``files`` and merge them into memories/MEMORY.md. + self._import_markdown_files(kind, source, [source] if source.exists() else None, + f"No {source.name} found", single_file=True) + def _import_markdown_files(self, kind: str, source: Path, files: Optional[List[Path]], + missing_reason: str, single_file: bool = False) -> None: + """Extract entries from ``files`` (None = source missing) and merge into memories/MEMORY.md. An unreadable file records an error; a directory import then still reports "no entries" - when nothing was extracted, while a single-file import stops at the error. - """ + when nothing was extracted, while a single-file import stops at the error.""" destination = self.target_root / "memories" / "MEMORY.md" + if files is None: + self.record(kind, None, destination, "skipped", missing_reason) + return incoming: List[str] = [] failed = False for md_file in files: @@ -561,35 +374,29 @@ class AgentImporter: if not (failed and single_file): self.record(kind, source, destination, "skipped", "No importable entries found") return - self._merge_memory_entries(kind, source, destination, incoming) - - def _merge_memory_entries(self, kind: str, source: Path, - destination: Path, incoming: List[str]) -> None: existing = parse_existing_memory_entries(destination) merged, stats = merge_entries(existing, incoming, MEMORY_CHAR_LIMIT) - details = { - "existing_entries": stats["existing"], - "added_entries": stats["added"], - "duplicate_entries": stats["duplicates"], - "overflowed_entries": stats["overflowed"], - } + details = {"existing_entries": stats["existing"], "added_entries": stats["added"], + "duplicate_entries": stats["duplicates"], + "overflowed_entries": stats["overflowed"]} if stats["added"] == 0: self.record(kind, source, destination, "skipped", "No new entries to import", **details) return def write() -> Optional[str]: destination.parent.mkdir(parents=True, exist_ok=True) + # Snapshot first (``.bak.``, as MemoryStore does); never rewrite the + # store when the safety net failed. + step = "back up existing" try: - backup = backup_memory_file(destination) - except OSError as exc: - # Never rewrite the store when the safety net failed. - return f"Could not back up existing memory file: {exc}" - if backup is not None: - details["backup"] = str(backup) - try: + if destination.exists(): + backup = destination.with_suffix(f"{destination.suffix}.bak.{int(time.time())}") + shutil.copy2(destination, backup) + details["backup"] = str(backup) + step = "write merged" atomic_write_text(destination, ENTRY_DELIMITER.join(merged) + ("\n" if merged else "")) except OSError as exc: - return f"Could not write merged memory file: {exc}" + return f"Could not {step} memory file: {exc}" return None self.apply(kind, source, destination, "Would merge entries", write, details) @@ -597,18 +404,10 @@ class AgentImporter: # (settings key, item kind, config path, dry-run tracks unmapped rules) _PERMISSION_RULES = { "allow": ("command-allowlist", ("command_allowlist",), True), - "deny": ("command-denylist", ("approvals", "deny"), False), - } - - def import_permission_allowlist(self, settings: Dict[str, Any]) -> None: - """settings.json permissions.allow → config.yaml command_allowlist.""" - self._import_permission_rules(settings, "allow") - - def import_permission_denylist(self, settings: Dict[str, Any]) -> None: - """settings.json permissions.deny → config.yaml approvals.deny.""" - self._import_permission_rules(settings, "deny") + "deny": ("command-denylist", ("approvals", "deny"), False)} def _import_permission_rules(self, settings: Dict[str, Any], key: str) -> None: + """settings.json permissions.allow/deny → config.yaml command_allowlist / approvals.deny.""" kind, config_path, track_unmapped = self._PERMISSION_RULES[key] label = f"settings.json permissions.{key}" destination = self.target_root / "config.yaml" @@ -617,18 +416,9 @@ class AgentImporter: if not isinstance(rules, list) or not rules: self.record(kind, None, destination, "skipped", f"No permissions.{key} rules found") return - - patterns: List[str] = [] - skipped_rules: List[str] = [] - for rule in rules: - if not isinstance(rule, str): - continue - pattern = claude_rule_to_command_pattern(rule) - if pattern: - patterns.append(pattern) - else: - skipped_rules.append(rule) - patterns = sorted(dict.fromkeys(patterns)) + mapped = [(r, claude_rule_to_command_pattern(r)) for r in rules if isinstance(r, str)] + patterns = sorted(dict.fromkeys(p for _, p in mapped if p)) + skipped_rules = [r for r, p in mapped if not p] unmapped: Dict[str, Any] = {"unmapped_rules": skipped_rules} if track_unmapped else {} if not patterns: self.record(kind, None, destination, "skipped", @@ -636,7 +426,6 @@ class AgentImporter: return if not skipped_rules: unmapped = {} - config = self.load_target_config(kind, label, destination) if config is None: return @@ -644,18 +433,15 @@ class AgentImporter: parent: Dict[str, Any] = config for part in config_path[:-1]: child = parent.get(part) - if not isinstance(child, dict): - child = {} - parent[part] = child - parent = child + parent[part] = parent = child if isinstance(child, dict) else {} current = parent.get(config_path[-1], []) - if not isinstance(current, list): - current = [] + current = current if isinstance(current, list) else [] merged = sorted(dict.fromkeys(list(current) + patterns)) added = [p for p in merged if p not in current] if not added: self.record(kind, label, destination, "skipped", "All patterns already present") return + def write() -> None: parent[config_path[-1]] = merged dump_yaml_file(destination, config) @@ -669,13 +455,11 @@ class AgentImporter: if not servers: self.record(kind, None, destination, "skipped", "No MCP servers found") return - config = self.load_target_config(kind, None, destination) if config is None: return existing = config.get("mcp_servers") - if not isinstance(existing, dict): - existing = {} + existing = existing if isinstance(existing, dict) else {} added = 0 for name, srv in servers.items(): if not isinstance(srv, dict): @@ -685,32 +469,26 @@ class AgentImporter: self.record(kind, name, f"mcp_servers.{name}", "conflict", "MCP server already exists in Hermes config") continue - hermes_srv, stripped = _translate_mcp_server(name, srv) self.stripped_secrets.extend(stripped) if not hermes_srv: self.record(kind, name, None, "skipped", "Server has neither a command nor a url") continue - existing[name] = hermes_srv added += 1 self.record(kind, name, f"config.yaml mcp_servers.{name}", "imported") - if added > 0 and self.execute: config["mcp_servers"] = existing dump_yaml_file(destination, config) def import_skills(self, source_root: Path) -> None: """skills//SKILL.md dirs → HERMES_HOME/skills//.""" - category = _SKILL_CATEGORY[self.agent] - destination_root = self.target_root / "skills" / category + destination_root = self.target_root / "skills" / _SKILL_CATEGORY[self.agent] if not source_root.is_dir(): self.record("skills", None, destination_root, "skipped", "No skills directory found") return - skill_dirs = [ - p for p in sorted(source_root.iterdir()) - if p.is_dir() and (p / "SKILL.md").exists() - ] + skill_dirs = [p for p in sorted(source_root.iterdir()) + if p.is_dir() and (p / "SKILL.md").exists()] if not skill_dirs: self.record("skills", source_root, destination_root, "skipped", "No skills with SKILL.md found") @@ -721,26 +499,25 @@ class AgentImporter: self.record("skill", skill_dir, destination, "conflict", "Destination skill already exists") continue - self.apply("skill", skill_dir, destination, "Would copy skill directory", - lambda: _copy_skill_dir(skill_dir, destination)) + def copy(skill_dir=skill_dir, destination=destination) -> None: + destination.parent.mkdir(parents=True, exist_ok=True) + if destination.exists(): + shutil.rmtree(destination) + shutil.copytree(skill_dir, destination) + + self.apply("skill", skill_dir, destination, "Would copy skill directory", copy) -# --------------------------------------------------------------------------- -# CLI entry point -# --------------------------------------------------------------------------- def import_agent_command(args) -> None: """Handle ``hermes import-agent`` (invoked from hermes_cli.main).""" from hermes_cli.config import get_config_path, load_config, save_config from hermes_constants import get_hermes_home - from hermes_cli.setup import ( - Colors, color, print_header, print_info, print_success, print_error, prompt_yes_no, - ) + from hermes_cli.setup import (Colors, color, print_header, print_info, print_success, + print_error, prompt_yes_no) - agent, explicit_source = args.agent, args.source - dry_run, overwrite, auto_yes = args.dry_run, args.overwrite, args.yes + agent, explicit_source, overwrite = args.agent, args.source, args.overwrite - # -- detect -------------------------------------------------------------- if agent is None: detected = detect_agents() if not detected: @@ -754,20 +531,17 @@ def import_agent_command(args) -> None: print_info("Pick one: hermes import-agent claude-code or hermes import-agent codex") return agent = detected[0] - - source_dir = Path(explicit_source) if explicit_source else default_source_dir(agent) + source_dir = Path(explicit_source or Path.home() / _AGENT_DEFAULT_DIRS[agent]) print() print(color("┌─────────────────────────────────────────────────────────┐", Colors.MAGENTA)) print(color("│ ⚕ Hermes — Import From Another Agent │", Colors.MAGENTA)) print(color("└─────────────────────────────────────────────────────────┘", Colors.MAGENTA)) - if not source_dir.is_dir(): print() print_error(f"Agent directory not found: {source_dir}") print_info(f"Specify a custom path: hermes import-agent {agent} --source /path/to/{_AGENT_DEFAULT_DIRS[agent]}") return - hermes_home = get_hermes_home() print() print_header("Import Settings") @@ -776,48 +550,41 @@ def import_agent_command(args) -> None: print_info(f"Target: {hermes_home}") print_info(f"Overwrite: {'yes' if overwrite else 'no (skip conflicts)'}") print_info("Secrets: never imported — run 'hermes setup' for credentials") - # Ensure config.yaml exists before the import tries to merge into it - config_path = get_config_path() - if not config_path.exists(): + if not get_config_path().exists(): save_config(load_config()) - # -- Phase 1: preview (always) -------------------------------------------- - def run_import(execute: bool) -> Dict[str, Any]: - return AgentImporter( - agent=agent, - source_root=source_dir.resolve(), - target_root=hermes_home.resolve(), - execute=execute, - overwrite=overwrite, - ).run() + def run_import(execute: bool, phase: str) -> Optional[Dict[str, Any]]: + """Run the importer; on failure print the error and return None.""" + try: + return AgentImporter(agent, source_dir.resolve(), hermes_home.resolve(), + execute=execute, overwrite=overwrite).run() + except Exception as e: + print() + print_error(f"Import{phase} failed: {e}") + logger.debug(f"import-agent{phase} error", exc_info=True) + return None - try: - preview = run_import(execute=False) - except Exception as e: - print() - print_error(f"Import preview failed: {e}") - logger.debug("import-agent preview error", exc_info=True) + # Phase 1: preview (always) + preview = run_import(False, " preview") + if preview is None: return - summary = preview.get("summary", {}) if summary.get("imported", 0) == 0 and summary.get("conflict", 0) == 0: print() print_info(f"Nothing to import from {agent}.") print_import_report(preview, dry_run=True) return - print() print_header(f"Import Preview — {summary.get('imported', 0)} item(s) would be imported") print_info("No changes have been made yet. Review the list below:") print_import_report(preview, dry_run=True) - - if dry_run: + if args.dry_run: return - # -- Phase 2: confirm and execute ----------------------------------------- + # Phase 2: confirm and execute print() - if not auto_yes: + if not args.yes: if not sys.stdin.isatty(): print_info("Non-interactive session — preview only.") print_info(f"To execute, re-run with: hermes import-agent {agent} --yes") @@ -825,15 +592,9 @@ def import_agent_command(args) -> None: if not prompt_yes_no("Proceed with import?", default=True): print_info("Import cancelled.") return - - try: - report = run_import(execute=True) - except Exception as e: - print() - print_error(f"Import failed: {e}") - logger.debug("import-agent error", exc_info=True) + report = run_import(True, "") + if report is None: return - print_import_report(report, dry_run=False) print() print_success("Import complete.") @@ -845,47 +606,36 @@ def print_import_report(report: Dict[str, Any], dry_run: bool) -> None: """Print a formatted per-item import report (claw-migrate style).""" from hermes_cli.setup import Colors, color, print_header, print_info - summary = report.get("summary", {}) print() + print_header("Dry Run Results" if dry_run else "Import Results") if dry_run: - print_header("Dry Run Results") print_info("No files were modified. This is a preview of what would happen.") - else: - print_header("Import Results") print() - - items = report.get("items", []) + # (status, colour, group heading, summary label) groups = ( - ("imported", Colors.GREEN, "✓ Would import" if dry_run else "✓ Imported"), - ("conflict", Colors.YELLOW, "⚠ Conflicts (skipped — use --overwrite to force)"), - ("skipped", Colors.DIM, "─ Skipped"), - ("error", Colors.RED, "✗ Errors"), - ) - for status, col, label in groups: - group_items = [i for i in items if i.get("status") == status] + ("imported", Colors.GREEN, "✓ Would import" if dry_run else "✓ Imported", + "would import" if dry_run else "imported"), + ("conflict", Colors.YELLOW, "⚠ Conflicts (skipped — use --overwrite to force)", + "conflict(s)"), + ("skipped", Colors.DIM, "─ Skipped", "skipped"), + ("error", Colors.RED, "✗ Errors", "error(s)")) + for status, col, label, _ in groups: + group_items = [i for i in report.get("items", []) if i.get("status") == status] if not group_items: continue print(color(f" {label}:", col)) for item in group_items: - tail = ( - "→ " + str(item.get("destination") or "").replace(str(Path.home()), "~") - if status == "imported" else f" {item.get('reason', '')}" - ) + tail = ("→ " + str(item.get("destination") or "").replace(str(Path.home()), "~") + if status == "imported" else f" {item.get('reason', '')}") print(f" {item.get('kind', 'unknown'):<22s} {tail}") print() - - stripped = report.get("stripped_secrets") or [] - if stripped: + if stripped := report.get("stripped_secrets"): print(color(" ⚷ Secrets stripped (never imported):", Colors.YELLOW)) for name in stripped: print(f" {name}") print_info("Re-add credentials deliberately via 'hermes setup' or ~/.hermes/.env.") print() - - labels = ( - ("imported", "would import" if dry_run else "imported"), - ("conflict", "conflict(s)"), ("skipped", "skipped"), ("error", "error(s)"), - ) - parts = [f"{summary[k]} {label}" for k, label in labels if summary.get(k)] + summary = report.get("summary", {}) + parts = [f"{summary[k]} {label}" for k, _, _, label in groups if summary.get(k)] if parts: print_info(f"Summary: {', '.join(parts)}") diff --git a/hermes_cli/agent_plugins.py b/hermes_cli/agent_plugins.py index 6428f80315..01366b6011 100644 --- a/hermes_cli/agent_plugins.py +++ b/hermes_cli/agent_plugins.py @@ -2,23 +2,22 @@ from __future__ import annotations +import ipaddress import json import os import re from dataclasses import dataclass from pathlib import Path from typing import Any, Dict, Mapping, Tuple +from urllib.parse import urlsplit from agent.skill_utils import yaml_load - PLUGIN_SCHEMA_V1 = "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json" MCP_SCHEMA_V1 = "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json" -_PLUGIN_FIELDS = { - "$schema", "name", "version", "description", "author", "homepage", "repository", "license", - "keywords", "extensions", -} +_PLUGIN_FIELDS = {"$schema", "name", "version", "description", "author", "homepage", "repository", + "license", "keywords", "extensions"} _AUTHOR_FIELDS = {"name", "email", "url"} _STDIO_FIELDS = {"type", "command", "args", "env", "cwd"} _REMOTE_FIELDS = {"type", "url", "headers"} @@ -62,8 +61,7 @@ class AgentPluginPackage: def _inside(path: Path, root: Path) -> bool: try: - path.resolve(strict=False).relative_to(root.resolve(strict=True)) - return True + return path.resolve(strict=False).is_relative_to(root.resolve(strict=True)) except (OSError, RuntimeError, ValueError): return False @@ -92,52 +90,38 @@ def _validate_manifest(root: Path) -> tuple[dict, list[AgentPluginDiagnostic]]: raise AgentPluginError("plugin.json must be a regular file within the plugin root") manifest = _read_json_object(manifest_path, label="plugin.json") diagnostics: list[AgentPluginDiagnostic] = [] - for field in sorted(set(manifest) - _PLUGIN_FIELDS): diagnostics.append(AgentPluginDiagnostic("manifest", f"ignored unknown top-level field: {field}")) manifest.pop(field) - if manifest.get("$schema") != PLUGIN_SCHEMA_V1: raise AgentPluginError("plugin.json declares an unsupported or missing Agent Plugins schema") name = manifest.get("name") if not isinstance(name, str) or not 1 <= len(name) <= 64 or _PLUGIN_NAME_RE.fullmatch(name) is None: raise AgentPluginError("plugin.json name does not satisfy v1 constraints") - for field in ("version", "description", "homepage", "repository", "license"): if field in manifest and not isinstance(manifest[field], str): raise AgentPluginError(f"plugin.json {field} must be a string") - - if "keywords" in manifest: - keywords = manifest["keywords"] - if not isinstance(keywords, list) or not _all_str(keywords): - raise AgentPluginError("plugin.json keywords must be an array of strings") - - if "author" in manifest: - author = manifest["author"] - if not isinstance(author, dict): - raise AgentPluginError("plugin.json author must be an object") - if set(author) - _AUTHOR_FIELDS or not _all_str(author.values()): - raise AgentPluginError("plugin.json author may contain only string name, email, and url fields") - - if "extensions" in manifest: - extensions = manifest["extensions"] - if not isinstance(extensions, dict): - diagnostics.append(AgentPluginDiagnostic("manifest", "ignored non-object extensions field")) - manifest.pop("extensions") - elif any(not isinstance(value, dict) for value in extensions.values()): - raise AgentPluginError("plugin.json extension namespace values must be objects") - + keywords = manifest.get("keywords", []) + if not isinstance(keywords, list) or not _all_str(keywords): + raise AgentPluginError("plugin.json keywords must be an array of strings") + author = manifest.get("author", {}) + if not isinstance(author, dict): + raise AgentPluginError("plugin.json author must be an object") + if set(author) - _AUTHOR_FIELDS or not _all_str(author.values()): + raise AgentPluginError("plugin.json author may contain only string name, email, and url fields") + extensions = manifest.get("extensions", {}) + if not isinstance(extensions, dict): + diagnostics.append(AgentPluginDiagnostic("manifest", "ignored non-object extensions field")) + manifest.pop("extensions") + elif any(not isinstance(value, dict) for value in extensions.values()): + raise AgentPluginError("plugin.json extension namespace values must be objects") return manifest, diagnostics def _valid_skill_frontmatter(frontmatter: Mapping[str, Any], directory_name: str) -> str | None: name = frontmatter.get("name") - if ( - not isinstance(name, str) - or name != directory_name - or not 1 <= len(name) <= 64 - or _SKILL_NAME_RE.fullmatch(name) is None - ): + if (not isinstance(name, str) or name != directory_name or not 1 <= len(name) <= 64 + or _SKILL_NAME_RE.fullmatch(name) is None): return "name must match the directory and satisfy Agent Skills constraints" description = frontmatter.get("description") if not isinstance(description, str) or not 1 <= len(description) <= 1024: @@ -184,15 +168,11 @@ def _discover_skills(root: Path, diagnostics: list[AgentPluginDiagnostic]) -> tu except OSError as exc: diagnostics.append(AgentPluginDiagnostic("skills", f"cannot list skills: {exc}")) return () - skills: list[AgentPluginSkill] = [] for child in children: skill_md = child / "SKILL.md" if not child.is_dir() or not skill_md.exists(): continue - scope = f"skill:{child.name}" - error: str | None = None - frontmatter: dict = {} if not _inside(skill_md, root) or not skill_md.is_file(): error = "SKILL.md must be a regular in-root file" else: @@ -203,17 +183,12 @@ def _discover_skills(root: Path, diagnostics: list[AgentPluginDiagnostic]) -> tu else: error = _valid_skill_frontmatter(frontmatter, child.name) if error: - diagnostics.append(AgentPluginDiagnostic(scope, error)) + diagnostics.append(AgentPluginDiagnostic(f"skill:{child.name}", error)) continue - skills.append( - AgentPluginSkill( - name=child.name, - description=frontmatter["description"], - root=child.resolve(strict=True), - skill_md=skill_md.resolve(strict=True), - frontmatter=dict(frontmatter), - ) - ) + skills.append(AgentPluginSkill( + name=child.name, description=frontmatter["description"], + root=child.resolve(strict=True), skill_md=skill_md.resolve(strict=True), + frontmatter=dict(frontmatter))) return tuple(skills) @@ -222,9 +197,8 @@ def _expand(value: str, plugin_root: Path, data_root: Path) -> str: return _PLACEHOLDER_RE.sub(lambda match: replacements[match.group(1)], value) -def _resolve_scoped_path( - value: str, plugin_root: Path, data_root: Path, *, expand_placeholders: bool = True -) -> Path: +def _resolve_scoped_path(value: str, plugin_root: Path, data_root: Path, *, + expand_placeholders: bool = True) -> Path: expanded = _expand(value, plugin_root, data_root) if expand_placeholders else value if value.startswith("./"): base, candidate = plugin_root, plugin_root / expanded[2:] @@ -243,28 +217,17 @@ def _resolve_scoped_path( def _validate_headers(headers: object) -> bool: - if headers is None: - return True if not isinstance(headers, dict): - return False + return headers is None names = [name for name in headers if isinstance(name, str) and _HEADER_NAME_RE.fullmatch(name)] - return ( - len(names) == len(headers) - and len({name.lower() for name in names}) == len(names) - and all(isinstance(v, str) and "\r" not in v and "\n" not in v for v in headers.values()) - ) + values = headers.values() + return (len(names) == len(headers) and len({name.lower() for name in names}) == len(names) + and all(isinstance(v, str) and "\r" not in v and "\n" not in v for v in values)) def _validate_remote_url(url: object) -> str: - """Validate a portable remote MCP URL per the v1 spec and return it. - - Rules (Agent Plugins v1 §7.2.1): absolute http(s) URL, no user information, no fragment; non- - loopback endpoints must use HTTPS. HTTP is allowed only when the host is exactly ``localhost`` - or an IP literal in a loopback range. No placeholder or environment expansion is performed. - """ - - from urllib.parse import urlsplit - + """Validate a portable remote MCP URL per Agent Plugins v1 §7.2.1 and return it: absolute + http(s), no user info, no fragment; HTTP only for ``localhost``/loopback IP. No expansion.""" if not isinstance(url, str) or not url: raise ValueError("url must be a non-empty string") try: @@ -282,8 +245,6 @@ def _validate_remote_url(url: object) -> str: if not host: raise ValueError("url must have a host") if scheme == "http" and host != "localhost": - import ipaddress - try: loopback = ipaddress.ip_address(host).is_loopback except ValueError: @@ -294,13 +255,8 @@ def _validate_remote_url(url: object) -> str: def _translate_remote(config: Mapping[str, Any]) -> Dict[str, Any]: - """Translate a portable ``streamable-http`` entry into native MCP config. - - ``strict_redirect_headers`` tells the runtime to drop configured headers on any cross-origin - redirect, as the v1 spec requires for portable packages (no forwarding to another origin - without explicit user authorization). - """ - + """Translate a portable ``streamable-http`` entry into native MCP config. The v1 spec requires + ``strict_redirect_headers``: drop configured headers on any cross-origin redirect.""" if set(config) - _REMOTE_FIELDS: raise ValueError("unknown remote field") url = _validate_remote_url(config.get("url")) @@ -312,122 +268,83 @@ def _translate_remote(config: Mapping[str, Any]) -> Dict[str, Any]: return translated -def _translate_stdio(config: Mapping[str, Any], plugin_root: Path, data_root: Path) -> Dict[str, Any]: +def _translate_stdio(config: Mapping[str, Any], plugin_root: Path, data_root: Path, + create_data: bool = False) -> Dict[str, Any]: if set(config) - _STDIO_FIELDS: raise ValueError("unknown stdio field") command = config.get("command") if not isinstance(command, str) or not command or "\x00" in command: raise ValueError("command must be a non-empty executable token") if command.startswith("./"): - command_value = str(_resolve_scoped_path(command, plugin_root, data_root, expand_placeholders=False)) + command = str(_resolve_scoped_path(command, plugin_root, data_root, + expand_placeholders=False)) elif any(character.isspace() for character in command): raise ValueError("command must contain one executable token") elif "/" in command or "\\" in command or command in {".", ".."}: raise ValueError("command must be a bare executable or begin with ./") - else: - command_value = command - args = config.get("args", []) if not isinstance(args, list) or not _all_str(args): raise ValueError("args must be an array of strings") env = config.get("env", {}) if not _str_map(env): raise ValueError("env must map string keys to string values") - env_keys = {key.upper() if os.name == "nt" else key for key in env} - if "PLUGIN_ROOT" in env_keys or "PLUGIN_DATA" in env_keys: + if {"PLUGIN_ROOT", "PLUGIN_DATA"} & {key.upper() if os.name == "nt" else key for key in env}: raise ValueError("PLUGIN_ROOT and PLUGIN_DATA are reserved") - cwd = config.get("cwd") - if cwd is None: - cwd_value = plugin_root - elif not isinstance(cwd, str): + if cwd is not None and not isinstance(cwd, str): raise ValueError("cwd must be a string") - else: - cwd_value = _resolve_scoped_path(cwd, plugin_root, data_root) - - translated_env = {key: _expand(value, plugin_root, data_root) for key, value in env.items()} - translated_env["PLUGIN_ROOT"] = str(plugin_root) - translated_env["PLUGIN_DATA"] = str(data_root) - return { - "command": command_value, - "args": [_expand(value, plugin_root, data_root) for value in args], - "env": translated_env, - "cwd": str(cwd_value), - } - - -def _translate_stdio_server(server: Mapping[str, Any], root: Path, data_root: Path, create_data: bool) -> Dict[str, Any]: - translated_server = _translate_stdio(server, root, data_root) + cwd_path = plugin_root if cwd is None else _resolve_scoped_path(cwd, plugin_root, data_root) if create_data: data_root.mkdir(parents=True, exist_ok=True) - cwd_path = Path(translated_server["cwd"]) - # The MCP client starts stdio servers with this cwd. Create only data-root - # descendants; plugin-root paths remain package-owned and are never made - # writable as a side effect of discovery. + # The MCP client starts stdio servers with this cwd. Create only data-root descendants; + # plugin-root paths remain package-owned and are never made writable by discovery. if cwd_path.is_relative_to(data_root): cwd_path.mkdir(parents=True, exist_ok=True) - return translated_server + translated_env = {**{key: _expand(value, plugin_root, data_root) for key, value in env.items()}, + "PLUGIN_ROOT": str(plugin_root), "PLUGIN_DATA": str(data_root)} + return {"command": command, "args": [_expand(value, plugin_root, data_root) for value in args], + "env": translated_env, "cwd": str(cwd_path)} def _reject_sse(server: Mapping[str, Any]) -> None: - if ( - set(server) - _REMOTE_FIELDS - or not isinstance(server.get("url"), str) - or not server.get("url") - or not _validate_headers(server.get("headers")) - ): + if (set(server) - _REMOTE_FIELDS or not isinstance(server.get("url"), str) + or not server.get("url") or not _validate_headers(server.get("headers"))): raise ValueError("invalid remote entry") raise ValueError("portable sse transport is not supported") -def _discover_mcp( - root: Path, - data_root: Path, - diagnostics: list[AgentPluginDiagnostic], - *, - create_data: bool = True, -) -> Dict[str, Dict[str, Any]]: +def _discover_mcp(root: Path, data_root: Path, diagnostics: list[AgentPluginDiagnostic], *, + create_data: bool = True) -> Dict[str, Dict[str, Any]]: mcp_path = root / "mcp.json" if not mcp_path.exists() and not mcp_path.is_symlink(): return {} - - def _fail(message: str) -> Dict[str, Dict[str, Any]]: - diagnostics.append(AgentPluginDiagnostic("mcp", message)) - return {} - - if not _inside(mcp_path, root) or not mcp_path.is_file(): - return _fail("mcp.json must be a regular in-root file") - try: + try: # file-level problems (incl. AgentPluginError from the JSON read) -> one "mcp" diagnostic + if not _inside(mcp_path, root) or not mcp_path.is_file(): + raise ValueError("mcp.json must be a regular in-root file") config = _read_json_object(mcp_path, label="mcp.json") - except AgentPluginError as exc: - return _fail(str(exc)) - if set(config) != {"$schema", "mcpServers"}: - return _fail("mcp.json has an invalid top-level shape") - if config.get("$schema") != MCP_SCHEMA_V1: - return _fail("mcp.json declares an unsupported schema") - servers = config.get("mcpServers") - if not isinstance(servers, dict): - return _fail("mcpServers must be an object") - - translators = { - "stdio": lambda server: _translate_stdio_server(server, root, data_root, create_data), - "streamable-http": _translate_remote, - "sse": _reject_sse, - } + if set(config) != {"$schema", "mcpServers"}: + raise ValueError("mcp.json has an invalid top-level shape") + if config.get("$schema") != MCP_SCHEMA_V1: + raise ValueError("mcp.json declares an unsupported schema") + servers = config.get("mcpServers") + if not isinstance(servers, dict): + raise ValueError("mcpServers must be an object") + except ValueError as exc: + diagnostics.append(AgentPluginDiagnostic("mcp", str(exc))) + return {} + translators = {"stdio": lambda server: _translate_stdio(server, root, data_root, create_data), + "streamable-http": _translate_remote, "sse": _reject_sse} translated: Dict[str, Dict[str, Any]] = {} for name, server in servers.items(): - scope = f"mcp:{name}" - if not isinstance(name, str) or not name or not isinstance(server, dict): - diagnostics.append(AgentPluginDiagnostic(scope, "invalid server entry")) - continue - translate = translators.get(server.get("type")) - if translate is None: - diagnostics.append(AgentPluginDiagnostic(scope, "unknown MCP server type")) - continue try: + if not isinstance(name, str) or not name or not isinstance(server, dict): + raise ValueError("invalid server entry") + translate = translators.get(server.get("type")) + if translate is None: + raise ValueError("unknown MCP server type") translated[name] = translate(server) except (OSError, ValueError) as exc: - diagnostics.append(AgentPluginDiagnostic(scope, str(exc))) + diagnostics.append(AgentPluginDiagnostic(f"mcp:{name}", str(exc))) return translated @@ -440,39 +357,23 @@ def _validate_root(plugin_root: Path) -> tuple[Path, dict, list[AgentPluginDiagn def load_agent_plugin(plugin_root: Path, data_root: Path) -> AgentPluginPackage: """Validate and translate one installed Agent Plugins v1 package.""" - root, manifest, diagnostics = _validate_root(plugin_root) resolved_data = Path(data_root).resolve(strict=False) - skills = _discover_skills(root, diagnostics) - mcp_servers = _discover_mcp(root, resolved_data, diagnostics) - return AgentPluginPackage( - name=manifest["name"], - version=manifest.get("version", ""), - description=manifest.get("description", ""), - root=root, - data_root=resolved_data, - manifest=dict(manifest), - skills=skills, - mcp_servers=mcp_servers, - diagnostics=tuple(diagnostics), - ) + return AgentPluginPackage( # skills are discovered before MCP: diagnostics keep that order + name=manifest["name"], version=manifest.get("version", ""), + description=manifest.get("description", ""), root=root, data_root=resolved_data, + manifest=dict(manifest), skills=_discover_skills(root, diagnostics), + mcp_servers=_discover_mcp(root, resolved_data, diagnostics), diagnostics=tuple(diagnostics)) def read_agent_plugin_manifest(plugin_root: Path) -> tuple[dict, tuple[AgentPluginDiagnostic, ...]]: """Validate only root ``plugin.json`` without discovering components.""" - _root, manifest, diagnostics = _validate_root(plugin_root) return manifest, tuple(diagnostics) def has_enabled_agent_plugin_mcp(raw_config: Mapping[str, Any]) -> bool: - """Compatibility wrapper for the shared PluginManager MCP probe. - - Directory scanning belongs to :mod:`hermes_cli.plugins` so startup gating and full plugin - discovery cannot drift apart. Keep this import-compatible entry point for callers that used the - original helper. - """ - + """Import-compatible wrapper for the shared PluginManager MCP probe; directory scanning lives + in :mod:`hermes_cli.plugins` so startup gating and full plugin discovery cannot drift apart.""" from hermes_cli.plugins import has_enabled_agent_plugin_mcp as _probe - return _probe(raw_config) From 3ce59eb2509d9c04e70a30ad746645dc2b1df300 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:14:33 -0700 Subject: [PATCH 4/7] refactor(hermes_cli): simplify browser_connect (1326->994 LOC): dead try_launch_chrome_debug, unify secure-snapshot/tcp-probe/candidate-path helpers, extract locked-profile message + profile-tree copy --- hermes_cli/browser_connect.py | 1076 ++++++++++++--------------------- 1 file changed, 372 insertions(+), 704 deletions(-) diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index a2b1322584..847c8cf7d9 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -1,8 +1,14 @@ -"""Shared helpers for attaching Hermes to a local Chromium-family CDP port.""" +"""Shared helpers for attaching Hermes to a local Chromium-family CDP port. + +Resolves the default Chromium browser + real profile dir, snapshots that profile for the +``browser.use_real_profile`` consent path, and discovers/launches a debug browser on a +loopback CDP port (dual-stack: IPv4 first, then IPv6). +""" from __future__ import annotations import contextlib +import json import logging import ntpath import os @@ -11,8 +17,12 @@ import posixpath import re import shlex import shutil +import socket +import sqlite3 import subprocess import time +import urllib.parse +import urllib.request from dataclasses import dataclass, field from pathlib import Path @@ -20,14 +30,13 @@ from hermes_constants import get_hermes_home logger = logging.getLogger(__name__) - DEFAULT_BROWSER_CDP_PORT = 9222 DEFAULT_BROWSER_CDP_URL = f"http://127.0.0.1:{DEFAULT_BROWSER_CDP_PORT}" + @dataclass(frozen=True) class _Browser: """Per-browser install/profile locations for one Chromium-family product.""" - key: str mac_app: str mac_support: tuple[str, ...] # under ~/Library/Application Support @@ -37,218 +46,141 @@ class _Browser: linux_bins: tuple[str, ...] # shutil.which names on Linux linux_paths: tuple[str, ...] # known absolute install paths linux_config: str # under $XDG_CONFIG_HOME - # PATH names tried by chromium_executable() when they differ from - # linux_bins (channel/alias binaries are launch candidates only). + # PATH names tried by chromium_executable() when they differ from linux_bins + # (channel/alias binaries are launch candidates only). linux_exec: tuple[str, ...] | None = None -# Launch-candidate order (chrome, chromium, brave, brave-origin, edge) is the -# tuple order. ``brave-origin`` is Brave's standalone paid build: same Chromium -# core, but a fully distinct install identity (BraveSoftware/Brave-Origin -# product path, ``BraveOHTML`` ProgId, ``com.brave.Browser.origin`` bundle id) -# so it side-by-side installs with regular Brave — its profile is NOT under -# Brave-Browser and must never be conflated with the ``brave`` key (a "brave" -# lookup resolving to the Origin binary would drive the wrong browser's profile). +# Launch-candidate order (chrome, chromium, brave, brave-origin, edge) is the tuple +# order. ``brave-origin`` is Brave's standalone paid build: same Chromium core but a +# fully distinct install identity (Brave-Origin product path, ``BraveOHTML`` ProgId, +# ``com.brave.Browser.origin`` bundle id) that installs side-by-side with Brave. Its +# profile is NOT under Brave-Browser and must never be conflated with the ``brave`` +# key — a "brave" lookup resolving to the Origin binary drives the wrong profile. +# Positional layout per entry: key, mac_app / mac_support, win_bins / win_install / +# win_profile / linux_bins / linux_paths / linux_config [, linux_exec]. _BROWSERS = ( _Browser( - "chrome", - "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", - ("Google", "Chrome"), - ("chrome.exe", "chrome"), + "chrome", "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", + ("Google", "Chrome"), ("chrome.exe", "chrome"), (("Google", "Chrome", "Application", "chrome.exe"),), ("Google", "Chrome", "User Data"), ("google-chrome", "google-chrome-stable"), ("/opt/google/chrome/chrome", "/usr/bin/google-chrome", "/usr/bin/google-chrome-stable"), - "google-chrome", - ), + "google-chrome"), _Browser( - "chromium", - "/Applications/Chromium.app/Contents/MacOS/Chromium", - ("Chromium",), - ("chromium.exe", "chromium"), + "chromium", "/Applications/Chromium.app/Contents/MacOS/Chromium", + ("Chromium",), ("chromium.exe", "chromium"), (("Chromium", "Application", "chrome.exe"), ("Chromium", "Application", "chromium.exe")), ("Chromium", "User Data"), ("chromium-browser", "chromium"), ("/usr/bin/chromium-browser", "/usr/bin/chromium"), - "chromium", - ), + "chromium"), _Browser( - "brave", - "/Applications/Brave Browser.app/Contents/MacOS/Brave Browser", - ("BraveSoftware", "Brave-Browser"), - ("brave.exe", "brave"), + "brave", "/Applications/Brave Browser.app/Contents/MacOS/Brave Browser", + ("BraveSoftware", "Brave-Browser"), ("brave.exe", "brave"), (("BraveSoftware", "Brave-Browser", "Application", "brave.exe"),), ("BraveSoftware", "Brave-Browser", "User Data"), ("brave-browser", "brave-browser-stable", "brave"), - ( - "/usr/bin/brave-browser", - "/usr/bin/brave-browser-stable", - "/usr/bin/brave", - "/snap/bin/brave", - "/opt/brave.com/brave/brave-browser", - "/opt/brave.com/brave/brave", - "/opt/brave-bin/brave", - ), - "BraveSoftware/Brave-Browser", - ), + ("/usr/bin/brave-browser", "/usr/bin/brave-browser-stable", "/usr/bin/brave", + "/snap/bin/brave", "/opt/brave.com/brave/brave-browser", "/opt/brave.com/brave/brave", + "/opt/brave-bin/brave"), + "BraveSoftware/Brave-Browser"), _Browser( - "brave-origin", - "/Applications/Brave Origin.app/Contents/MacOS/Brave Origin", - ("BraveSoftware", "Brave-Origin"), - ("brave-origin.exe", "brave-origin"), - ( - ("BraveSoftware", "Brave-Origin", "Application", "brave.exe"), - ("BraveSoftware", "Brave-Origin", "Application", "brave-origin.exe"), - ), + "brave-origin", "/Applications/Brave Origin.app/Contents/MacOS/Brave Origin", + ("BraveSoftware", "Brave-Origin"), ("brave-origin.exe", "brave-origin"), + (("BraveSoftware", "Brave-Origin", "Application", "brave.exe"), + ("BraveSoftware", "Brave-Origin", "Application", "brave-origin.exe")), ("BraveSoftware", "Brave-Origin", "User Data"), ("brave-origin", "brave-origin-nightly"), - ( - "/usr/bin/brave-origin", - "/opt/brave.com/brave-origin/brave-origin", - "/opt/brave.com/brave-origin-nightly/brave-origin", - ), - "BraveSoftware/Brave-Origin", - linux_exec=("brave-origin",), - ), + ("/usr/bin/brave-origin", "/opt/brave.com/brave-origin/brave-origin", + "/opt/brave.com/brave-origin-nightly/brave-origin"), + "BraveSoftware/Brave-Origin", linux_exec=("brave-origin",)), _Browser( - "edge", - "/Applications/Microsoft Edge.app/Contents/MacOS/Microsoft Edge", - ("Microsoft Edge",), - ("msedge.exe", "msedge"), + "edge", "/Applications/Microsoft Edge.app/Contents/MacOS/Microsoft Edge", + ("Microsoft Edge",), ("msedge.exe", "msedge"), (("Microsoft", "Edge", "Application", "msedge.exe"),), ("Microsoft", "Edge", "User Data"), ("microsoft-edge", "microsoft-edge-stable", "msedge"), - ( - "/usr/bin/microsoft-edge", - "/usr/bin/microsoft-edge-stable", - "/opt/microsoft/msedge/microsoft-edge", - "/opt/microsoft/msedge/msedge", - ), - "microsoft-edge", - linux_exec=("microsoft-edge", "microsoft-edge-stable"), - ), + ("/usr/bin/microsoft-edge", "/usr/bin/microsoft-edge-stable", + "/opt/microsoft/msedge/microsoft-edge", "/opt/microsoft/msedge/msedge"), + "microsoft-edge", linux_exec=("microsoft-edge", "microsoft-edge-stable")), ) _BROWSER_BY_KEY = {b.key: b for b in _BROWSERS} -# --------------------------------------------------------------------------- -# Real-profile (default Chromium) resolution -# -# Used by the browser tool's ``browser.use_real_profile`` consent path: when a -# local Chromium is launched, point agent-browser at the user's REAL default -# browser profile (``--profile `` + ``--executable-path``) so -# their live logins/cookies are available. Only Chromium-family browsers are -# supported; a non-Chromium default (Firefox, Safari) resolves to None and the -# caller fails closed with a clear message. -# --------------------------------------------------------------------------- +# --- Default-Chromium resolution (``browser.use_real_profile``) ------------------------ +# Only Chromium-family browsers are supported; a non-Chromium default (Firefox, Safari) +# resolves to None and the caller fails closed. Each platform table is a STABLE map plus a +# CHANNEL list: Beta/Dev/Canary are recognized but unsupported (their profiles live in dirs +# the tables don't carry) and are matched FIRST so they fail closed instead of being +# swallowed into the stable family — driving the wrong profile is a wrong-principal bug +# (#95549 invariant). -# Windows UserChoice ProgId prefixes → canonical browser key. Matched -# case-insensitively by prefix so version suffixes (e.g. ``ChromeHTML.X``) -# still resolve to STABLE. Pre-release channels have their own ProgIds and -# MUST be matched first (see _WINDOWS_CHANNEL_PROGIDS) so they are never -# swallowed into the stable family — driving the wrong profile is a -# wrong-principal bug (#95549 invariant). +# Windows UserChoice ProgId prefixes → key. Case-insensitive PREFIX match so +# version suffixes (``ChromeHTML.X``) still resolve. _WINDOWS_PROGID_MAP = ( - ("chromehtml", "chrome"), - ("msedgehtm", "edge"), - # Brave Origin stable is ``BraveOHTML`` (brave-core install_static). Listed - # before ``bravehtml`` for clarity; the prefixes don't collide either way. - ("braveohtml", "brave-origin"), - ("bravehtml", "brave"), - ("chromiumhtm", "chromium"), -) + ("chromehtml", "chrome"), ("msedgehtm", "edge"), + ("braveohtml", "brave-origin"), # Brave Origin stable (brave-core install_static) + ("bravehtml", "brave"), ("chromiumhtm", "chromium")) -# Pre-release ProgId prefixes we recognize but do NOT support (their profiles -# live in channel-specific dirs the resolver tables don't carry). Matched -# BEFORE the stable map; a hit fails closed rather than resolving to stable. -# ``ChromeBHTML`` = Beta, ``ChromeDHTML`` = Dev, ``ChromeSSHTML`` = Canary -# (SxS); ``MSEdgeBHTML`` / ``MSEdgeDHTML`` / ``MSEdgeCHTML`` = Edge channels. +# ``ChromeBHTML`` = Beta, ``ChromeDHTML`` = Dev, ``ChromeSSHTML`` = Canary (SxS); +# ``MSEdge[BDC]HTML`` = Edge channels; Brave Origin Beta=BraveOBHTML, Dev=BraveODHTML, +# Nightly/SxS=BraveOSHTM (no trailing L — 10-char cap). _WINDOWS_CHANNEL_PROGIDS = ( "chromebhtml", "chromedhtml", "chromesshtml", "chromecanaryhtml", "msedgebhtml", "msedgedhtml", "msedgechtml", "bravebetahtml", "bravenightlyhtml", - # Brave Origin channels (brave-core install_static): Beta=BraveOBHTML, - # Dev=BraveODHTML, Nightly/SxS=BraveOSHTM (no trailing L — 10-char cap). - "braveobhtml", "braveodhtml", "braveoshtm", -) + "braveobhtml", "braveodhtml", "braveoshtm") -# Linux xdg default-web-browser .desktop name fragments → canonical STABLE key. -# Includes the Flatpak application ids (``com.google.Chrome.desktop`` etc.), -# which share none of the native package name fragments. Anchored so a channel -# .desktop (``google-chrome-beta``, ``com.google.chrome.beta``) does NOT match -# the stable fragment — channels are caught by _LINUX_CHANNEL_FRAGMENTS first. +# Linux xdg default-web-browser .desktop name fragments → key (SUBSTRING match), +# including Flatpak application ids (``com.google.Chrome.desktop``). _LINUX_DESKTOP_MAP = ( - ("google-chrome", "chrome"), - ("com.google.chrome", "chrome"), - ("chromium", "chromium"), - # ORDER MATTERS: ``brave-origin.desktop`` contains the bare ``brave`` - # fragment, so the substring scan must hit the Origin entry first — - # otherwise an Origin default resolves to stable Brave and real-profile - # mode drives a DIFFERENT browser's profile (wrong-principal, #95549). - ("brave-origin", "brave-origin"), - ("brave", "brave"), - ("microsoft-edge", "edge"), - ("com.microsoft.edge", "edge"), - ("msedge", "edge"), -) + ("google-chrome", "chrome"), ("com.google.chrome", "chrome"), ("chromium", "chromium"), + # ORDER MATTERS: ``brave-origin.desktop`` contains the bare ``brave`` fragment, + # so the substring scan must hit the Origin entry first (#95549). + ("brave-origin", "brave-origin"), ("brave", "brave"), + ("microsoft-edge", "edge"), ("com.microsoft.edge", "edge"), ("msedge", "edge")) -# Non-stable Linux channel .desktop fragments — recognized, unsupported. -# Checked before the stable map; a hit fails closed. _LINUX_CHANNEL_FRAGMENTS = ( "google-chrome-beta", "google-chrome-unstable", "google-chrome-canary", "com.google.chrome.beta", "com.google.chrome.dev", "com.google.chrome.canary", "microsoft-edge-beta", "microsoft-edge-dev", "microsoft-edge-canary", "brave-browser-beta", "brave-browser-nightly", "brave-browser-dev", - "brave-origin-beta", "brave-origin-nightly", "brave-origin-dev", -) + "brave-origin-beta", "brave-origin-nightly", "brave-origin-dev") # Where sandboxed Linux packages keep the profile instead of $XDG_CONFIG_HOME. -_LINUX_FLATPAK_IDS = { - "chrome": "com.google.Chrome", - "chromium": "org.chromium.Chromium", - "brave": "com.brave.Browser", - "edge": "com.microsoft.Edge", -} +_LINUX_FLATPAK_IDS = {"chrome": "com.google.Chrome", "chromium": "org.chromium.Chromium", + "brave": "com.brave.Browser", "edge": "com.microsoft.Edge"} _LINUX_SNAP_PROFILE_PARTS = { "chromium": ("snap", "chromium", "common", "chromium"), - "brave": ("snap", "brave", "current", ".config", "BraveSoftware", "Brave-Browser"), -} + "brave": ("snap", "brave", "current", ".config", "BraveSoftware", "Brave-Browser")} -# macOS LaunchServices bundle-id → canonical STABLE key. EXACT match (not -# prefix): ``com.google.chrome.beta`` must not be read as ``com.google.chrome``. +# macOS LaunchServices bundle-id → key. EXACT match (not prefix): ``com.google.chrome.beta`` +# must not be read as ``com.google.chrome``, nor ``com.brave.browser.origin`` (Homebrew +# cask id for Brave Origin) as plain ``com.brave.browser``. _DARWIN_BUNDLE_MAP = ( - ("com.google.chrome", "chrome"), - ("com.microsoft.edgemac", "edge"), - ("com.brave.browser", "brave"), - # Brave Origin reuses the Brave bundle id with an ``.origin`` suffix - # (Homebrew cask: com.brave.Browser.origin). Exact matching keeps it from - # ever being read as plain ``com.brave.browser``. - ("com.brave.browser.origin", "brave-origin"), - ("org.chromium.chromium", "chromium"), -) + ("com.google.chrome", "chrome"), ("com.microsoft.edgemac", "edge"), + ("com.brave.browser", "brave"), ("com.brave.browser.origin", "brave-origin"), + ("org.chromium.chromium", "chromium")) -# Non-stable macOS channel bundle ids — recognized, unsupported. Checked first. _DARWIN_CHANNEL_BUNDLES = ( "com.google.chrome.beta", "com.google.chrome.dev", "com.google.chrome.canary", "com.microsoft.edgemac.beta", "com.microsoft.edgemac.dev", "com.microsoft.edgemac.canary", "com.brave.browser.beta", "com.brave.browser.nightly", "com.brave.browser.origin.beta", "com.brave.browser.origin.dev", - "com.brave.browser.origin.nightly", -) + "com.brave.browser.origin.nightly") -# Sentinel returned when the OS default is a recognized-but-unsupported -# Chromium CHANNEL (Beta/Dev/Canary). Distinct from None (non-Chromium) so the -# caller fails closed with a channel-specific message instead of driving the -# stable profile of a different account. +# Sentinel for a recognized-but-unsupported Chromium CHANNEL default. Distinct from +# None (non-Chromium) so the caller can give a channel-specific message. UNSUPPORTED_CHANNEL = "__unsupported_channel__" def real_profile_data_dir(browser: str, system: str | None = None) -> str | None: - """Return the default user-data-dir for a Chromium ``browser`` on ``system``. + """Return the default user-data-dir for a Chromium ``browser`` on ``system`` (None if unknown). - Returns None for unknown browsers. On Linux the native ($XDG_CONFIG_HOME), snap and Flatpak - locations are tried and the first existing one wins; the native path is returned when none - exists so the caller's error names it. Darwin/Windows paths are not stat'ed. + Linux tries native ($XDG_CONFIG_HOME), snap and Flatpak; first existing wins, else the native + path so the caller's error names it. Darwin/Windows paths are not stat'ed. """ b = _BROWSER_BY_KEY.get(browser) if b is None: @@ -260,20 +192,15 @@ def real_profile_data_dir(browser: str, system: str | None = None) -> str | None if system == "Windows": local = os.environ.get("LOCALAPPDATA") or ntpath.join(home, "AppData", "Local") return ntpath.join(local, *b.win_profile) - # Linux / other POSIX config = os.environ.get("XDG_CONFIG_HOME") or posixpath.join(home, ".config") linux_parts = b.linux_config.split("/") candidates = [posixpath.join(config, *linux_parts)] - snap_parts = _LINUX_SNAP_PROFILE_PARTS.get(browser) - if snap_parts: - candidates.append(posixpath.join(home, *snap_parts)) - flatpak_id = _LINUX_FLATPAK_IDS.get(browser) - if flatpak_id: - candidates.append(posixpath.join(home, ".var", "app", flatpak_id, "config", *linux_parts)) - for candidate in candidates: - if os.path.isdir(candidate): - return candidate - return candidates[0] + if browser in _LINUX_SNAP_PROFILE_PARTS: + candidates.append(posixpath.join(home, *_LINUX_SNAP_PROFILE_PARTS[browser])) + if browser in _LINUX_FLATPAK_IDS: + candidates.append(posixpath.join(home, ".var", "app", _LINUX_FLATPAK_IDS[browser], "config", + *linux_parts)) + return next((c for c in candidates if os.path.isdir(c)), candidates[0]) def _first_present(paths) -> str | None: @@ -295,8 +222,7 @@ def chromium_executable(browser: str, system: str | None = None) -> str | None: bases = [ os.environ.get("PROGRAMFILES", r"C:\Program Files"), os.environ.get("PROGRAMFILES(X86)", r"C:\Program Files (x86)"), - os.environ.get("LOCALAPPDATA", str(Path.home() / "AppData" / "Local")), - ] + os.environ.get("LOCALAPPDATA", str(Path.home() / "AppData" / "Local"))] return _first_present(os.path.join(base, *parts) for base in bases for parts in b.win_install) # Linux: PATH lookup first, then the known absolute install paths. for name in b.linux_exec or b.linux_bins: @@ -324,71 +250,50 @@ def _classify_default(value: str, channels, table, match) -> str | None: def _detect_default_windows() -> str | None: try: import winreg # type: ignore - except Exception: - return None - try: + key = winreg.OpenKey( winreg.HKEY_CURRENT_USER, - r"Software\Microsoft\Windows\Shell\Associations\UrlAssociations\https\UserChoice", - ) + r"Software\Microsoft\Windows\Shell\Associations\UrlAssociations\https\UserChoice") prog_id, _ = winreg.QueryValueEx(key, "ProgId") winreg.CloseKey(key) - except Exception: + except Exception: # non-Windows host (no winreg) or unreadable key return None - low = str(prog_id or "").lower() - return _classify_default(low, _WINDOWS_CHANNEL_PROGIDS, _WINDOWS_PROGID_MAP, str.startswith) + return _classify_default(str(prog_id or "").lower(), _WINDOWS_CHANNEL_PROGIDS, + _WINDOWS_PROGID_MAP, str.startswith) def _run_stdout(argv: list[str]) -> str | None: try: - return subprocess.run( - argv, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5 - ).stdout + return subprocess.run(argv, capture_output=True, text=True, encoding="utf-8", + errors="replace", timeout=5).stdout except Exception: return None -_LS_HANDLERS_READER = ( - "defaults", - "read", - "com.apple.LaunchServices/com.apple.launchservices.secure", - "LSHandlers", -) - - def _launchservices_https_handler(dump: str) -> str | None: """Return the bundle id registered for the ``https`` URL scheme. - ``dump`` is the ``defaults read … LSHandlers`` output: an array of ``{ … }`` dictionaries, one - per handler. Only the entry whose ``LSHandlerURLScheme`` is ``https`` counts — a browser - registered for another scheme or a file type must not be mistaken for the default. + ``dump`` is ``defaults read … LSHandlers`` output: an array of ``{ … }`` dicts, one per handler. + Only the ``LSHandlerURLScheme = https`` entry counts — a browser registered for another scheme + or a file type must not be mistaken for the default. """ entries: list[str] = [] - depth = 0 - buf: list[str] = [] + depth, buf = 0, [] for ch in dump: - if ch == "{": - depth += 1 - if depth == 1: - buf = [] - continue - elif ch == "}": - depth -= 1 - if depth == 0: - entries.append("".join(buf)) - continue - if depth >= 1: + depth += (ch == "{") - (ch == "}") + if ch == "{" and depth == 1: + buf = [] + elif ch == "}" and depth == 0: + entries.append("".join(buf)) + elif depth >= 1: buf.append(ch) for entry in entries: low = entry.lower() if not re.search(r'lshandlerurlscheme\s*=\s*"?https"?\s*;', low): continue - # Strip the nested LSHandlerPreferredVersions block first: on macOS 26 - # it carries a VERSION NUMBER (e.g. LSHandlerRoleAll = "7559.97";), not - # the "-" placeholder older releases used. Left in, the role regex below - # would match that version before the real bundle id sitting at the - # entry's own level and return "7559.97" — which maps to no browser, so - # detection fails on a machine whose default IS Chrome (PR #95620 review). + # Strip the nested LSHandlerPreferredVersions block first: on macOS 26 it carries + # a VERSION NUMBER (LSHandlerRoleAll = "7559.97";), not the "-" placeholder older + # releases used, and the role regex below would return it instead of the bundle id. low = re.sub(r"lshandlerpreferredversions\s*=\s*\{[^}]*\}\s*;", "", low) # The real bundle id is the first non-"-" role value at this level. for role in re.findall(r'lshandlerrole(?:all|viewer)\s*=\s*"?([a-z0-9.\-]+)"?\s*;', low): @@ -399,106 +304,67 @@ def _launchservices_https_handler(dump: str) -> str | None: def _detect_default_darwin() -> str | None: - out = _run_stdout(list(_LS_HANDLERS_READER)) + out = _run_stdout(["defaults", "read", + "com.apple.LaunchServices/com.apple.launchservices.secure", "LSHandlers"]) if out is None: return None bundle = _launchservices_https_handler(out) if not bundle: return None - # Exact match. A non-Chromium https handler (Safari, Firefox, Arc, …) or an - # unknown channel bundle fails closed: no "first installed Chromium wins" - # fallback — that would drive a browser the user never made their default. + # Exact match. A non-Chromium https handler (Safari, Firefox, Arc, …) or an unknown + # channel bundle fails closed: no "first installed Chromium wins" fallback. return _classify_default(bundle.lower(), _DARWIN_CHANNEL_BUNDLES, _DARWIN_BUNDLE_MAP, str.__eq__) def _detect_default_linux() -> str | None: out = (_run_stdout(["xdg-settings", "get", "default-web-browser"]) or "").strip().lower() - # Substring match; channels first because ``google-chrome-beta.desktop`` - # contains the stable ``google-chrome`` fragment. + # Substring match; channels first because ``google-chrome-beta.desktop`` contains + # the stable ``google-chrome`` fragment. return _classify_default(out, _LINUX_CHANNEL_FRAGMENTS, _LINUX_DESKTOP_MAP, str.__contains__) def detect_default_chromium(system: str | None = None) -> str | None: """Return the canonical key of the default Chromium browser, or None.""" - system = system or platform.system() - if system == "Windows": - return _detect_default_windows() - if system == "Darwin": - return _detect_default_darwin() - return _detect_default_linux() + detect = {"Windows": _detect_default_windows, "Darwin": _detect_default_darwin} + return detect.get(system or platform.system(), _detect_default_linux)() -# --------------------------------------------------------------------------- -# Real-profile SNAPSHOT launch -# -# The consent path (``browser.use_real_profile``) never drives the live -# default user-data-dir. Chromium ≥136 (Google-branded builds) refuses -# remote debugging on the default dir no matter who launches it, and the -# live dir is usually held by the user's running browser (SingletonLock). -# Instead we snapshot the real profile into ``~/.hermes/browser-profile/`` -# — a non-default dir Chrome will happily debug, that never contends with -# the user's browser — launch the user's real binary on the copy with a -# devtools port, and hand the CDP URL to whichever browser lane is active -# (Browser Use CLI or the built-in tools). We launch the browser ourselves -# precisely so NO mock-keychain/basic-store switches are added: cookies -# encrypted with the OS keyring (gnome-keyring / kwallet / macOS Keychain) -# decrypt exactly like they do in the user's own browser. -# --------------------------------------------------------------------------- +# --- Real-profile SNAPSHOT launch ------------------------------------------------------- +# Never drive the live default user-data-dir: Chromium ≥136 (Google builds) refuses remote +# debugging on it, and the user's running browser holds it (SingletonLock). Instead snapshot +# the real profile into ``~/.hermes/browser-profile/`` and launch the user's real binary on +# the copy — with NO mock-keychain/basic-store switches, so OS-keyring-encrypted cookies +# decrypt exactly as in the user's own browser. -# Directory names excluded from the profile snapshot: caches/telemetry AND the -# heavy, replay-prone state that hangs a fresh Chromium's renderer (extensions -# and their service workers spin up on launch and wedge JS eval; IndexedDB / -# GPUCache add hundreds of MB for nothing). We keep ONLY auth/login state -# (cookies, Login Data, Web Data, Preferences, Local State) — the point of the -# feature — which turns a multi-hundred-MB profile into a few MB. +# Excluded from the snapshot: caches/telemetry AND replay-prone state that hangs a fresh +# Chromium's renderer (extensions + service workers spin up on launch and wedge JS eval; +# IndexedDB/GPUCache add hundreds of MB). Only auth/login state is kept. _SNAPSHOT_IGNORES = ( "*Cache*", # Cache, Code Cache, GPUCache, GrShaderCache, ShaderCache, GraphiteDawnCache, component_crx_cache, ... "Extensions", # wallets/etc.: 100s of MB, and hang the renderer headless "Extension*", # Extension State, Extension Rules, Extension Scripts "Local Extension Settings", "Service Worker", # replays on launch → wedges the renderer - "IndexedDB", - "Crash Reports", - "Crashpad", - "BrowserMetrics*", - "Snapshots", - "OptimizationGuide*", - "optimization_guide_model_store", - "Safe Browsing", - "SafetyTips", - "OnDeviceHeadSuggestModel", - "segmentation_platform", - "Sync Data", - "Shared Dictionary", + "IndexedDB", "Crash Reports", "Crashpad", "BrowserMetrics*", "Snapshots", + "OptimizationGuide*", "optimization_guide_model_store", "Safe Browsing", "SafetyTips", + "OnDeviceHeadSuggestModel", "segmentation_platform", "Sync Data", "Shared Dictionary", "History*", # large; not needed for auth "Favicons*", "Singleton*", # live-instance symlinks; never valid in a copy - "RunningChromeVersion", - "SingletonSocket", - "*.tmp", + "RunningChromeVersion", "SingletonSocket", "*.tmp", "*-journal", # SQLite rollback journals — sidecars of the auth DBs, "*-wal", # which are copied via online-backup; a stale sidecar "*-shm", # next to a backed-up DB corrupts it. "BrowserMetrics-spare.pma", ) -# Small, auth-bearing files re-synced from the live profile on EVERY consented -# launch (the full tree is only copied when the snapshot doesn't exist yet). -# Paths here are RELATIVE TO A PROFILE DIR (Default, "Profile 6", …) — the -# caller resolves which source profile is active and mirrors these into the -# copy's ``Default`` so the launched Chromium (which opens ``Default``) lands -# on the user's real signed-in session. No ``-journal``/``-wal`` sidecars: the -# SQLite DBs are copied via the online-backup API (see _copy_auth_file), which -# produces a self-contained DB with committed state folded in — copying a -# stale raw journal on top of that would corrupt it. +# Auth-bearing files re-synced from the live profile on EVERY consented launch (the full +# tree is copied only when the snapshot doesn't exist yet). RELATIVE TO A PROFILE DIR; the +# caller mirrors them into the copy's ``Default``. No ``-journal``/``-wal`` sidecars: the +# DBs come via sqlite online-backup (committed state folded in); a stale journal corrupts. _AUTH_REFRESH_PROFILE_FILES = ( - "Cookies", - "Network/Cookies", - "Login Data", - "Login Data For Account", - "Web Data", - "Preferences", -) + "Cookies", "Network/Cookies", "Login Data", "Login Data For Account", "Web Data", "Preferences") + def real_profile_copy_dir(browser: str) -> str: """Return the hermes-owned snapshot dir for ``browser``'s real profile.""" @@ -506,112 +372,72 @@ def real_profile_copy_dir(browser: str) -> str: def _last_used_profile(src: str) -> str: - """Return the profile dir Chrome last used (``Local State`` → profile.last_used). + """Profile dir Chrome last used (``Local State`` → profile.last_used), else ``Default``. - Chromium opens ``Default`` inside a user-data-dir unless told otherwise, but the user's signed- - in session usually lives in whichever profile they actually browse (``Profile 6`` etc.). We read - that here and mirror its auth into the copy's ``Default`` so the launched browser is signed in. + The user's signed-in session usually lives in the profile they actually browse (``Profile 6``), + not ``Default``; its auth is what gets mirrored into the copy's ``Default``. """ - import json - try: with open(os.path.join(src, "Local State"), encoding="utf-8", errors="replace") as fh: state = json.load(fh) last = ((state.get("profile") or {}).get("last_used")) or "Default" except (OSError, ValueError, AttributeError): last = "Default" - if not isinstance(last, str) or not os.path.isdir(os.path.join(src, last)): - return "Default" - return last + return last if isinstance(last, str) and os.path.isdir(os.path.join(src, last)) else "Default" -def _secure_snapshot_root(path: str) -> None: - """Lock down a snapshot dir through Hermes' canonical secret-store policy. +def _secure_snapshot(path: str, *, contents: bool = False) -> None: + """Lock down a snapshot dir (or, with ``contents``, everything INSIDE it) as a secret store. - The snapshot holds copies of the user's Cookies / Login Data, so it is a credential store and - must get the same owner-only permissions (and managed-mode / NixOS group-share carve-out, - HERMES_UID/GID ownership) as every other Hermes secret dir — via - ``hermes_cli.config._secure_dir``, not a bespoke chmod. Best-effort: never blocks a launch. - """ - try: - from hermes_cli.config import _secure_dir - - _secure_dir(path) - except Exception as e: - logger.debug("could not secure real-profile snapshot dir %s: %s", path, e) - - -def _secure_snapshot_contents(dst: str) -> None: - """Owner-only modes for every file/dir INSIDE the snapshot (#96729). - - ``_secure_snapshot_root`` covers the top-level dirs, but ``copy2`` preserves Chrome's 0644 file - modes and ``sqlite3`` backups create umask files, so live session cookies landed world-readable - — a real exposure under the ``HERMES_HOME_MODE`` hatch. Reconciled via ``_secure_dir`` / - ``_secure_file`` on EVERY pass so older snapshots heal too. Best-effort: never blocks a launch. + The snapshot holds the user's Cookies / Login Data, so it gets the same owner-only permissions + (managed-mode / NixOS group-share carve-out, HERMES_UID/GID ownership) as every other Hermes + secret dir — via ``_secure_dir``/``_secure_file``, not a bespoke chmod. Contents matter too + (#96729): ``copy2`` keeps Chrome's 0644 modes and sqlite backups land umask-wide, so cookies + were world-readable under the ``HERMES_HOME_MODE`` hatch. Best-effort; never blocks a launch. """ try: from hermes_cli.config import _secure_dir, _secure_file - - for root, dirs, files in os.walk(dst): + if not contents: + _secure_dir(path) + return + for root, dirs, files in os.walk(path): for d in dirs: _secure_dir(os.path.join(root, d)) for f in files: _secure_file(os.path.join(root, f)) except Exception as e: - logger.debug("could not secure real-profile snapshot contents %s: %s", dst, e) + logger.debug("could not secure real-profile snapshot %s %s: %s", + "contents" if contents else "dir", path, e) -# Auth files that are SQLite databases: on Windows a running Chrome holds these -# with an exclusive lock, so a raw file copy raises WinError 32 ("being used by -# another process") and a naive best-effort skip leaves the copy signed-out. -# These are copied via SQLite's online-backup API instead, which reads a -# consistent committed snapshot while the lock is held. Matched by basename. -_SQLITE_AUTH_DBS = frozenset({ - "Cookies", "Login Data", "Login Data For Account", "Web Data", -}) +# Auth files that are SQLite databases: on Windows a running Chrome holds these with an +# exclusive lock, so a raw copy raises WinError 32 and a best-effort skip leaves the copy +# signed-out. They are copied via SQLite's online-backup API instead. Matched by basename. +_SQLITE_AUTH_DBS = frozenset({"Cookies", "Login Data", "Login Data For Account", "Web Data"}) def _copy_auth_file(src_file: str, dst_file: str) -> bool: - """Copy one auth file, lock-aware. Returns True on success. + """Copy one auth file, lock-aware; True on success. - For SQLite DBs (Cookies/Login Data/…), use the online-backup API so the copy works even while - the browser holds the file's write lock (Windows). Everything else is a plain copy. A DB whose - backup fails falls through to a raw copy attempt; only if BOTH fail do we report failure to the - caller. + SQLite DBs use the online-backup API (works under a Windows write lock) and fall through to a + raw copy if that fails; everything else is a plain copy. Failure only if BOTH fail. """ os.makedirs(os.path.dirname(dst_file), exist_ok=True) if os.path.basename(src_file) in _SQLITE_AUTH_DBS: - # On a live Chrome on macOS the profile holds - # its DBs in a state where mode=ro WITHOUT immutable=1 can hang the - # connect/backup indefinitely (the sqlite busy-timeout never fires - # because the block happens inside lock negotiation). immutable=1 - # reads instantly and is correct here: we want a committed snapshot of - # a file another process owns, not coordinated writes. A torn read - # raises → falls through to the plain-copy fallback below. - for uri in ( - f"file:{src_file}?mode=ro&immutable=1", - f"file:{src_file}?mode=ro", - ): + # With a live Chrome on macOS, mode=ro WITHOUT immutable=1 can hang connect/backup + # forever (blocked inside lock negotiation, so the busy-timeout never fires). + # immutable=1 reads instantly and is correct: we want a committed snapshot, not + # coordinated writes. A torn read raises → next mode, then the plain-copy fallback. + for uri in (f"file:{src_file}?mode=ro&immutable=1", f"file:{src_file}?mode=ro"): try: - import sqlite3 - - # Short busy timeout so a truly wedged DB fails fast rather - # than hanging the launch. - source = sqlite3.connect(uri, uri=True, timeout=5) - try: - out = sqlite3.connect(dst_file) - try: - with out: - source.backup(out) - finally: - out.close() - finally: - source.close() + # Short busy timeout so a truly wedged DB fails fast rather than hanging. + with contextlib.closing(sqlite3.connect(uri, uri=True, timeout=5)) as source: + with contextlib.closing(sqlite3.connect(dst_file)) as out, out: + source.backup(out) return True except Exception as e: logger.debug("real-profile: sqlite-backup of %s failed (%s); trying next mode", src_file, e) - # Non-DB file, or DB whose backup failed: raw copy. try: shutil.copy2(src_file, dst_file) return True @@ -621,12 +447,9 @@ def _copy_auth_file(src_file: str, dst_file: str) -> bool: def _mirror_profile_auth(src: str, dst: str, source_profile: str) -> int: - """Copy ``source_profile``'s auth files into the copy's ``Default`` slot. + """Mirror ``source_profile``'s auth files into the copy's ``Default`` (agent-browser opens it). - agent-browser launches ``Default`` in the copied user-data-dir, so mirroring the active - profile's cookies/logins/prefs there is what makes the session actually signed in when the real - session lives in a non-Default profile. Lock-aware on Windows. Returns the number of DB auth - files that could NOT be copied (0 = clean). + Returns the number of DB auth files that could NOT be copied (0 = clean). """ dst_default = os.path.join(dst, "Default") failed_dbs = 0 @@ -641,28 +464,21 @@ def _mirror_profile_auth(src: str, dst: str, source_profile: str) -> int: _SNAPSHOT_DONE_MARKER = ".hermes-snapshot-complete" - -# Prefix stamped on the "profile is locked" error so the calling layer can -# recognize it as the specific needs-the-browser-closed condition (vs a generic -# snapshot failure) and surface the close-with-approval flow. +# Prefix stamped on the "profile is locked" error so the calling layer can recognize the +# needs-the-browser-closed condition and surface the close-with-approval flow. _PROFILE_LOCKED_PREFIX = "[profile-locked] " -def _profile_cookie_db(src: str, source_profile: str) -> str | None: - """Path to the active profile's cookie DB (modern Network/ first).""" - return _first_present( - os.path.join(src, source_profile, rel) for rel in (os.path.join("Network", "Cookies"), "Cookies") - ) - - def _profile_is_locked(src: str, source_profile: str) -> bool: """True when the active profile's cookie DB can't be opened (browser running). - On Windows a running browser holds Cookies with a deny-all share mode, so a plain open raises - PermissionError. This is a FAST one-open probe used to fail closed BEFORE the heavy snapshot so - a locked profile can never hang the launch. POSIX has no mandatory locking, so it returns False. + On Windows a running browser holds Cookies deny-all, so open raises PermissionError. This FAST + probe fails closed BEFORE the heavy snapshot so a locked profile can never hang the launch. + POSIX has no mandatory locking, so it returns False. """ - db = _profile_cookie_db(src, source_profile) + db = _first_present( # modern Network/ location first + os.path.join(src, source_profile, rel) + for rel in (os.path.join("Network", "Cookies"), "Cookies")) if not db: return False # nothing to lock; let the copy path handle "no cookies" try: @@ -670,8 +486,7 @@ def _profile_is_locked(src: str, source_profile: str) -> bool: return False except PermissionError: return True - except OSError: - # Other errors (transient) — don't declare locked; let the copy try. + except OSError: # transient — don't declare locked; let the copy try return False @@ -679,46 +494,38 @@ def _browser_setting(key: str): """Read ``browser.`` from raw config; None when unset/unreadable.""" try: from hermes_cli.config import read_raw_config - browser_cfg = read_raw_config().get("browser", {}) - if isinstance(browser_cfg, dict): - return browser_cfg.get(key) + return browser_cfg.get(key) if isinstance(browser_cfg, dict) else None except Exception as e: logger.debug("could not read %s: %s", key, e) - return None + return None def _real_profile_pin() -> str | None: """Pinned source profile dir name from ``browser.real_profile_pin``. - Natively the snapshot follows Chrome's ``profile.last_used``, which on a machine with work and - personal profiles can silently hand the agent the wrong identity. When set (e.g. ``"Profile - 2"``) that profile is ALWAYS copied; unset keeps the native last_used behavior. + Unset, the snapshot follows Chrome's ``profile.last_used``, which with work + personal profiles + can hand the agent the wrong identity. When set (``"Profile 2"``) that profile is ALWAYS copied. """ pin = _browser_setting("real_profile_pin") - if isinstance(pin, str) and pin.strip(): - return pin.strip() - return None + return pin.strip() if isinstance(pin, str) and pin.strip() else None def _resolve_source_profile(src: str) -> tuple[str | None, str | None]: - """Resolve which source profile to copy: pin first, else last_used. + """Return ``(profile_dir_name, error)``: the pin if set, else last_used. - Returns ``(profile_dir_name, error)``. A configured pin that does not exist under ``src`` FAILS - CLOSED with a fixable message — falling back to last_used would silently browse as the wrong - identity, which is the exact wrong-principal bug this pin exists to prevent. + A pin that does not exist under ``src`` FAILS CLOSED — falling back to last_used would + silently browse as the wrong identity, the exact wrong-principal bug the pin prevents. """ pin = _real_profile_pin() if pin: if os.path.isdir(os.path.join(src, pin)): return pin, None return None, ( - f"browser.real_profile_pin is set to '{pin}' but that profile " - f"directory does not exist under {src!r}. Profile directories are " - "named like 'Default' or 'Profile 2' — list them with: " - f"ls {src!r}. Fix the pin, or remove it to fall back to the " - "last-used profile." - ) + f"browser.real_profile_pin is set to '{pin}' but that profile directory does not " + f"exist under {src!r}. Profile directories are named like 'Default' or 'Profile 2' " + f"— list them with: ls {src!r}. Fix the pin, or remove it to fall back to the " + "last-used profile.") return _last_used_profile(src), None @@ -728,11 +535,10 @@ def _real_profile_autoclose() -> bool: def _processes_holding_profile(src: str): - """Yield (psutil.Process) instances holding the user-data-dir ``src`` open. + """Yield psutil.Process instances holding the user-data-dir ``src`` open. - Identity discipline mirrors the daemon reaper: a process qualifies only when it's a Chromium- - family binary AND its command line references THIS user-data-dir — so we never terminate an - unrelated same-PID process. Any ambiguity (unreadable cmdline) is skipped, fail-closed. + A process qualifies only when it's a Chromium-family binary AND its cmdline references THIS + user-data-dir — never an unrelated same-PID process. Unreadable cmdline is skipped. """ try: import psutil @@ -741,8 +547,7 @@ def _processes_holding_profile(src: str): norm = os.path.normcase(os.path.normpath(src)) browser_bins = ( "chrome", "chrome.exe", "chromium", "chromium.exe", "chrome_crashpad", - "brave", "brave.exe", "msedge", "msedge.exe", "google chrome", - ) + "brave", "brave.exe", "msedge", "msedge.exe", "google chrome") for proc in psutil.process_iter(["name", "cmdline"]): try: name = (proc.info.get("name") or "").lower() @@ -750,46 +555,35 @@ def _processes_holding_profile(src: str): joined = " ".join(cmd) except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): continue - if not any(b in name for b in browser_bins): - # Some platforms report a generic name; also accept when the binary - # in argv[0] looks like a browser. - argv0 = (cmd[0].lower() if cmd else "") - if not any(b in argv0 for b in browser_bins): - continue - # Binding: the exact user-data-dir must appear in the cmdline - # (--user-data-dir=), normalized for case/separators. - if norm not in os.path.normcase(os.path.normpath(joined)) and \ - f"--user-data-dir={src}".lower() not in joined.lower(): + argv0 = cmd[0].lower() if cmd else "" # some platforms report a generic name + if not any(b in name or b in argv0 for b in browser_bins): continue - yield proc + # Binding: the exact user-data-dir must appear in the cmdline, normalized. + if (norm in os.path.normcase(os.path.normpath(joined)) + or f"--user-data-dir={src}".lower() in joined.lower()): + yield proc def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool, str]: """Terminate the browser process tree holding ``src`` and wait for release. - CONSENTED, DESTRUCTIVE. Only call after the user has agreed to close their browser — it - terminates every Chromium-family process bound to this exact user-data-dir (graceful terminate, - then kill), so unsaved tab/form state in that browser is lost. + CONSENTED, DESTRUCTIVE: terminates (then kills) every Chromium-family process bound to this + exact user-data-dir, losing unsaved tab/form state. Only call after the user agreed. """ try: import psutil except ImportError: return False, "psutil unavailable — cannot close the browser automatically." - procs = list(_processes_holding_profile(src)) if not procs: - # Nothing we can see holds it. Either already closed, or the holder is - # a different user / unreadable — caller re-probes the lock. + # Already closed, or the holder is a different user / unreadable — caller re-probes. return False, "no matching browser process found holding the profile." - # Include child processes (renderers, GPU, crashpad) for a full tree kill. gone_errs = (psutil.NoSuchProcess, psutil.AccessDenied) - targets = [] + targets = list(procs) for p in procs: - targets.append(p) with contextlib.suppress(*gone_errs): targets.extend(p.children(recursive=True)) - # Graceful terminate first. for p in targets: with contextlib.suppress(*gone_errs): p.terminate() @@ -798,7 +592,6 @@ def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool with contextlib.suppress(*gone_errs): p.kill() psutil.wait_procs(alive, timeout=3.0) - # The lock releases slightly after the process exits on Windows; poll. source_profile = _resolve_source_profile(src)[0] or _last_used_profile(src) deadline = time.monotonic() + timeout @@ -808,32 +601,26 @@ def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool time.sleep(0.5) return False, ( "closed the browser processes but the profile is still locked — " - "another instance may have relaunched (background/tray mode)." - ) + "another instance may have relaunched (background/tray mode).") def _sync_local_state(src: str, dst: str, source_profile: str) -> None: """Copy ``Local State`` into the snapshot and rewrite it for the single ``Default`` profile. - Cheap; always re-synced so last_used etc. stay current. The copy contains ONLY the mirrored - Default dir, but a verbatim Local State still names the SOURCE profile (last_used="Profile 2", - info_cache listing Profile 2/4/7), so Chrome would open a missing profile dir and start SIGNED - OUT. CRITICAL: Default's identity entry must be the SOURCE profile's entry (name + Google - account), not the source's own "Default" entry — the Default DIR holds the source profile's - cookies. A mismatch makes Chrome demand a "Continue as " reconciliation on every launch. + A verbatim Local State still names the SOURCE profile (last_used="Profile 2", info_cache of + Profile 2/4/7) that the copy doesn't contain, so Chrome would start SIGNED OUT. CRITICAL: the + Default entry must be the SOURCE profile's identity (name + account) because the Default DIR + holds its cookies; a mismatch makes Chrome demand "Continue as " on every launch. """ - ls_src = os.path.join(src, "Local State") - ls_dst = os.path.join(dst, "Local State") + ls_src, ls_dst = os.path.join(src, "Local State"), os.path.join(dst, "Local State") if os.path.isfile(ls_src): try: shutil.copy2(ls_src, ls_dst) except OSError as e: logger.debug("real-profile snapshot: skipped Local State: %s", e) try: - import json as _json - with open(ls_dst, encoding="utf-8") as fh: - state = _json.load(fh) + state = json.load(fh) prof = state.get("profile") if isinstance(prof, dict): cache = prof.get("info_cache") @@ -844,119 +631,95 @@ def _sync_local_state(src: str, dst: str, source_profile: str) -> None: prof["last_used"] = "Default" prof["last_active_profiles"] = ["Default"] with open(ls_dst, "w", encoding="utf-8") as fh: - _json.dump(state, fh) + json.dump(state, fh) except (OSError, ValueError) as e: logger.debug("real-profile snapshot: could not normalize Local State: %s", e) +def _locked_profile_error(browser: str) -> str: + """Fail-closed message for a locked profile; offers auto-close only when consent arms it. + + NEVER kill from here: closing the user's browser is destructive and must be an explicit, + per-attempt, user-approved step. A still-locked retry blocks again — no auto-retry, no loop. + """ + if _real_profile_autoclose(): + msg = ( + f"{browser} is running and has its profile locked, so its login data can't be copied " + "yet. Hermes can close it for you (this quits the browser — you'll lose unsaved " + "tabs). Ask the user to confirm, then close it and retry; if it's still locked after " + "that, they must fully quit it (including any background/tray instance).") + else: + msg = ( + f"{browser} is running and has its profile locked, so its login data can't be copied. " + "Fully quit the browser (including any background/tray instance) and retry, or turn " + "browser.use_real_profile off. (Enable browser.real_profile_autoclose to let Hermes " + "offer to close it for you.)") + return _PROFILE_LOCKED_PREFIX + msg + + +def _copy_profile_tree(src: str, dst: str, source_profile: str) -> None: + """Fresh (or torn-and-rebuilding) copy of the ACTIVE profile dir into the copy's Default. + + Excludes caches AND the SQLite auth DBs (a raw copytree of a file a running Chrome holds open + raises on Windows); ``_mirror_profile_auth`` copies the DBs lock-aware instead. + """ + dst_default = os.path.join(dst, "Default") + try: + shutil.rmtree(dst_default, ignore_errors=True) + shutil.copytree( + os.path.join(src, source_profile), + dst_default, + dirs_exist_ok=True, + symlinks=False, + ignore=shutil.ignore_patterns(*_SNAPSHOT_IGNORES, *_SQLITE_AUTH_DBS), + ignore_dangling_symlinks=True) + except shutil.Error as multi: + # Per-file failures (browser mid-write) are non-fatal. + logger.info( + "real-profile snapshot: %d file(s) skipped copying %s/%s", + len(multi.args[0]) if multi.args else 0, src, source_profile) + + def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | None, str | None]: - """Snapshot ``browser``'s real ACTIVE profile into the hermes copy dir. + """Snapshot ``browser``'s real ACTIVE profile into the hermes copy dir; returns ``(dst, err)``. - Copies only what the launched browser needs: the user-data-dir's ``Local State`` plus the auth- - bearing files of the profile the user actually browses (``Local State → profile.last_used``, - e.g. ``Profile 6``), mirrored into the copy's ``Default`` — which is what agent-browser opens. - - A ``.hermes-snapshot-complete`` marker is written only after a copy fully succeeds; a - torn/interrupted first copy (disk full, Ctrl+C) therefore never looks "already populated" on the - next run — it is redone from scratch. + Copies ``Local State`` plus the active profile's auth files into the copy's ``Default``. The + completion marker is written only after a full success, so a torn first copy (disk full, + Ctrl+C) never looks "already populated" — it is redone from scratch. """ src = src or real_profile_data_dir(browser) if not src or not os.path.isdir(src): return None, ( f"profile directory for '{browser}' was not found ({src!r}). " - "Launch that browser at least once, or turn browser.use_real_profile off." - ) + "Launch that browser at least once, or turn browser.use_real_profile off.") source_profile, resolve_err = _resolve_source_profile(src) if resolve_err or not source_profile: return None, resolve_err dst = real_profile_copy_dir(browser) - # Fast lock probe BEFORE any copy: a running browser holds the cookie DB - # deny-all (Windows), and a blocking file op on it can hang the launch for - # minutes. On POSIX this never trips (no mandatory locking) so - # copy-while-running still works. + # Fast lock probe BEFORE any copy: a blocking file op on a Windows-locked cookie DB can + # hang the launch for minutes. Never trips on POSIX, so copy-while-running still works. if _profile_is_locked(src, source_profile): - # NEVER kill from here. Closing the user's browser is destructive and - # must be an explicit, per-attempt, user-approved step — not a silent - # side effect of a snapshot. So we always BLOCK when locked and let the - # agent decide whether to ask the user to close it (only offered when - # browser.real_profile_autoclose arms the capability). A subsequent - # attempt that is still locked blocks again — no auto-retry, no loop. - if _real_profile_autoclose(): - msg = ( - f"{browser} is running and has its profile locked, so its login " - "data can't be copied yet. Hermes can close it for you " - "(this quits the browser — you'll lose unsaved tabs). Ask the " - "user to confirm, then close it and retry; if it's still locked " - "after that, they must fully quit it (including any " - "background/tray instance)." - ) - else: - msg = ( - f"{browser} is running and has its profile locked, so its login " - "data can't be copied. Fully quit the browser (including any " - "background/tray instance) and retry, or turn " - "browser.use_real_profile off. (Enable " - "browser.real_profile_autoclose to let Hermes offer to close it " - "for you.)" - ) - return None, _PROFILE_LOCKED_PREFIX + msg + return None, _locked_profile_error(browser) marker = os.path.join(dst, _SNAPSHOT_DONE_MARKER) - # Only a copy that previously COMPLETED counts as populated. A half-written - # tree (no marker) is treated as absent and rebuilt — otherwise a torn first - # copy poisons freshness forever and only ever gets auth overlays. + # Only a copy that previously COMPLETED counts as populated; a half-written tree is + # rebuilt — otherwise a torn first copy poisons freshness forever. populated = os.path.isfile(marker) try: os.makedirs(dst, exist_ok=True) - # Secure the snapshot dir AND its browser-profile parent on EVERY - # launch: a failed first attempt or an older-build dir must still - # converge to owner-only perms; the parent enumerates every browser we - # hold cookies for. - parent = os.path.dirname(dst) - if parent: - _secure_snapshot_root(parent) - _secure_snapshot_root(dst) - + # Secure the snapshot dir AND its browser-profile parent on EVERY launch so a failed + # first attempt or an older-build dir still converges to owner-only perms. + if os.path.dirname(dst): + _secure_snapshot(os.path.dirname(dst)) + _secure_snapshot(dst) _sync_local_state(src, dst, source_profile) - if not populated: - # Fresh (or torn-and-rebuilding): drop any partial Default and copy - # the ACTIVE profile's full dir (minus caches AND the locked auth - # DBs) into the copy's Default. The SQLite auth DBs are excluded - # here because a raw copytree of a file a running Chrome holds open - # raises on Windows; they are copied lock-aware by - # _mirror_profile_auth below (sqlite online-backup). - dst_default = os.path.join(dst, "Default") - try: - shutil.rmtree(dst_default, ignore_errors=True) - shutil.copytree( - os.path.join(src, source_profile), - dst_default, - dirs_exist_ok=True, - symlinks=False, - ignore=shutil.ignore_patterns(*_SNAPSHOT_IGNORES, *_SQLITE_AUTH_DBS), - ignore_dangling_symlinks=True, - ) - except shutil.Error as multi: - # Per-file failures (browser mid-write) are non-fatal. - logger.info( - "real-profile snapshot: %d file(s) skipped copying %s/%s", - len(multi.args[0]) if multi.args else 0, src, source_profile, - ) - - # Both paths: copy the active profile's auth DBs into Default, - # lock-aware (sqlite online-backup) so a running Chrome on Windows - # doesn't block them. This is also the per-launch fresh-login re-sync. + _copy_profile_tree(src, dst, source_profile) + # Both paths: lock-aware auth DB copy into Default — also the per-launch re-sync. failed_dbs = _mirror_profile_auth(src, dst, source_profile) - if failed_dbs: - # We could not read the user's cookie/login DBs at all — even the - # online-backup fallback failed. Rather than launch a silently - # signed-out session, fail closed with an actionable message. - return None, ( - f"could not read the '{browser}' profile's login data " - f"({failed_dbs} database(s) locked). Close {browser} and retry, " - "or turn browser.use_real_profile off." - ) - + if failed_dbs: # even online-backup failed: never launch a silently signed-out session + return None, (f"could not read the '{browser}' profile's login data ({failed_dbs} " + f"database(s) locked). Close {browser} and retry, or turn " + "browser.use_real_profile off.") # Never carry live-instance leftovers into the copy. for leftover in ("SingletonLock", "SingletonSocket", "SingletonCookie"): with contextlib.suppress(OSError): @@ -967,75 +730,53 @@ def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | N fh.write(source_profile) except OSError as e: logger.debug("real-profile snapshot: could not write done marker: %s", e) - # Owner-only modes for everything the copies above created — copy2 - # preserves Chrome's 0644 and sqlite backup files land umask-wide; - # these are the user's session cookies (#96729). Runs AFTER the marker - # write so the marker itself is covered, and on every pass so - # snapshots from older builds heal on their next launch. - _secure_snapshot_contents(dst) + # AFTER the marker write so the marker itself is covered; every pass, so old snapshots heal. + _secure_snapshot(dst, contents=True) except OSError as e: return None, f"could not snapshot the '{browser}' profile into {dst}: {e}" return dst, None def cleanup_real_profile_snapshots() -> None: - """Delete the whole real-profile snapshot store (all copied credentials). - - Called when consent is OFF: the copied Cookies / Login Data must not outlive the toggle. Best- - effort and idempotent — missing dir is fine. - """ + """Delete the whole real-profile snapshot store when consent is OFF (idempotent).""" root = str(get_hermes_home() / "browser-profile") - try: - if os.path.isdir(root): - shutil.rmtree(root, ignore_errors=True) - logger.info("real-profile: removed snapshot store %s (consent off)", root) - except OSError as e: - logger.debug("real-profile cleanup failed for %s: %s", root, e) + if os.path.isdir(root): + shutil.rmtree(root, ignore_errors=True) + logger.info("real-profile: removed snapshot store %s (consent off)", root) + + +def _debug_candidate_paths(system: str): + """Yield possible debug-browser binaries in launch order (may include None/missing).""" + install_bases = (os.environ.get("ProgramFiles"), os.environ.get("ProgramFiles(x86)"), + os.environ.get("LOCALAPPDATA")) + for b in _BROWSERS: + if system == "Darwin": + yield b.mac_app + elif system == "Windows": + yield from map(shutil.which, b.win_bins) + for base in filter(None, install_bases): + for parts in b.win_install: + yield os.path.join(base, *parts) + else: + yield from map(shutil.which, b.linux_bins) + yield from b.linux_paths + if system not in ("Darwin", "Windows"): + # WSL: Windows installs under ``/mnt/c/...`` are POSIX paths regardless of the host + # OS, so join with posixpath (os.path.join would emit backslashes on nt). + for b in _BROWSERS: + for base in ("/mnt/c/Program Files", "/mnt/c/Program Files (x86)"): + for parts in b.win_install: + yield posixpath.join(base, *parts) def get_chrome_debug_candidates(system: str) -> list[str]: candidates: list[str] = [] seen: set[str] = set() - - def add(path: str | None) -> None: - if not path: - return - normalized = os.path.normcase(os.path.normpath(path)) - if normalized in seen or not os.path.isfile(path): - return - candidates.append(path) - seen.add(normalized) - - if system == "Darwin": - for b in _BROWSERS: - add(b.mac_app) - return candidates - - if system == "Windows": - install_bases = ( - os.environ.get("ProgramFiles"), - os.environ.get("ProgramFiles(x86)"), - os.environ.get("LOCALAPPDATA"), - ) - for b in _BROWSERS: - for name in b.win_bins: - add(shutil.which(name)) - for base in filter(None, install_bases): - for parts in b.win_install: - add(os.path.join(base, *parts)) - return candidates - - for b in _BROWSERS: - for name in b.linux_bins: - add(shutil.which(name)) - for path in b.linux_paths: - add(path) - # WSL: Windows installs under ``/mnt/c/...`` are POSIX paths regardless of - # the host OS, so join with posixpath (os.path.join would emit backslashes on nt). - for b in _BROWSERS: - for base in ("/mnt/c/Program Files", "/mnt/c/Program Files (x86)"): - for parts in b.win_install: - add(posixpath.join(base, *parts)) + for path in _debug_candidate_paths(system): + normalized = os.path.normcase(os.path.normpath(path)) if path else None + if path and normalized not in seen and os.path.isfile(path): + candidates.append(path) + seen.add(normalized) return candidates @@ -1044,39 +785,30 @@ def chrome_debug_data_dir() -> str: def _chrome_debug_args(port: int) -> list[str]: - return [ - f"--remote-debugging-port={port}", - f"--user-data-dir={chrome_debug_data_dir()}", - "--no-first-run", - "--no-default-browser-check", - ] + return [f"--remote-debugging-port={port}", f"--user-data-dir={chrome_debug_data_dir()}", + "--no-first-run", "--no-default-browser-check"] + + +def _tcp_open(host: str, port: int, timeout: float) -> bool: + try: + with socket.create_connection((host, port), timeout=timeout): + return True + except OSError: + return False def is_browser_debug_ready(url: str, timeout: float = 1.0) -> bool: """Return True when ``url`` exposes a reachable Chrome DevTools endpoint.""" - import socket - import urllib.request - from urllib.parse import urlparse - - parsed = urlparse(url if "://" in url else f"http://{url}") + parsed = urllib.parse.urlparse(url if "://" in url else f"http://{url}") try: port = parsed.port or (443 if parsed.scheme in {"https", "wss"} else 80) except ValueError: return False - if parsed.scheme in {"ws", "wss"} and parsed.path.startswith("/devtools/browser/"): - if not parsed.hostname: - return False - try: - with socket.create_connection((parsed.hostname, port), timeout=timeout): - return True - except OSError: - return False - + return bool(parsed.hostname) and _tcp_open(parsed.hostname, port, timeout) scheme = {"ws": "http", "wss": "https"}.get(parsed.scheme, parsed.scheme) if scheme not in {"http", "https"} or not parsed.netloc: return False - root = f"{scheme}://{parsed.netloc}".rstrip("/") for probe in (f"{root}/json/version", f"{root}/json"): try: @@ -1088,21 +820,15 @@ def is_browser_debug_ready(url: str, timeout: float = 1.0) -> bool: return False -# Both loopback literals: Windows (and some Linux setups) can hand the IPv4 -# loopback to one process and the IPv6 loopback to another. Chrome asked to -# bind :9222 while e.g. VS Code's js-debug holds 127.0.0.1:9222 will come up -# on [::1]:9222 only — reachable, but invisible to an IPv4-only probe. +# Both loopback literals, IPv4 FIRST: Windows (and some Linux setups) can hand each loopback +# to a different process — Chrome asked to bind :9222 while VS Code's js-debug holds +# 127.0.0.1:9222 comes up on [::1]:9222 only, invisible to an IPv4-only probe. _LOOPBACK_PROBE_HOSTS = ("127.0.0.1", "[::1]") _LOOPBACK_SOCKET_HOSTS = ("127.0.0.1", "::1") def discover_local_cdp_url(port: int, timeout: float = 1.0) -> str | None: - """Return the first loopback URL (IPv4 first, then IPv6) speaking CDP. - - Dual-stack discovery: when another application squats the IPv4 loopback on ``port``, a debug - browser launched with ``--remote-debugging-port`` may bind only ``[::1]``. Probing both literals - finds it either way. Returns ``None`` when neither loopback exposes a CDP discovery endpoint. - """ + """Return the first loopback URL (IPv4 first, then IPv6) speaking CDP, else None.""" for host in _LOOPBACK_PROBE_HOSTS: url = f"http://{host}:{port}" if is_browser_debug_ready(url, timeout=timeout): @@ -1111,43 +837,28 @@ def discover_local_cdp_url(port: int, timeout: float = 1.0) -> str | None: def local_port_in_use(port: int, timeout: float = 0.5) -> bool: - """Return True when either loopback accepts TCP on ``port``. + """True when either loopback accepts TCP on ``port``. - Callers use this AFTER a failed CDP probe to distinguish "port is free, we can launch a browser - on it" from "another application (IDE debugger, dev server) is squatting the port and a launch - would fight it". + Used AFTER a failed CDP probe to tell "port is free, launch here" from "another application + (IDE debugger, dev server) is squatting the port and a launch would fight it". """ - import socket - - for host in _LOOPBACK_SOCKET_HOSTS: - try: - with socket.create_connection((host, port), timeout=timeout): - return True - except OSError: - continue - return False + return any(_tcp_open(host, port, timeout) for host in _LOOPBACK_SOCKET_HOSTS) def find_free_debug_port(preferred: int = DEFAULT_BROWSER_CDP_PORT, attempts: int = 10) -> int: - """Return the first port after ``preferred`` bindable on both loopbacks. - - Used when ``preferred`` is occupied by a non-CDP application: rather than launching a browser - into a bind conflict, pick a nearby free port. Falls back to ``preferred + 1`` if nothing binds - (the launch will then fail with a clear browser-side error instead of silently doing nothing). - """ - import socket - + """First port after ``preferred`` bindable on both loopbacks; ``preferred + 1`` if none binds + (the launch then fails with a clear browser-side error instead of silently doing nothing).""" + def bindable(family, host, port) -> bool: + try: + with socket.socket(family, socket.SOCK_STREAM) as sock: + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + sock.bind((host, port)) + return True + except OSError: + return False + loopbacks = ((socket.AF_INET, "127.0.0.1"), (socket.AF_INET6, "::1")) for port in range(preferred + 1, preferred + 1 + attempts): - bindable = True - for family, host in ((socket.AF_INET, "127.0.0.1"), (socket.AF_INET6, "::1")): - try: - with socket.socket(family, socket.SOCK_STREAM) as sock: - sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) - sock.bind((host, port)) - except OSError: - bindable = False - break - if bindable: + if all(bindable(family, host, port) for family, host in loopbacks): return port return preferred + 1 @@ -1155,53 +866,39 @@ def find_free_debug_port(preferred: int = DEFAULT_BROWSER_CDP_PORT, attempts: in def manual_chrome_debug_command(port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> str | None: system = system or platform.system() candidates = get_chrome_debug_candidates(system) - if candidates: argv = [candidates[0], *_chrome_debug_args(port)] return subprocess.list2cmdline(argv) if system == "Windows" else shlex.join(argv) - if system == "Darwin": - data_dir = chrome_debug_data_dir() - return ( - f'open -a "Google Chrome" --args --remote-debugging-port={port} ' - f'--user-data-dir="{data_dir}" --no-first-run --no-default-browser-check' - ) - + return (f'open -a "Google Chrome" --args --remote-debugging-port={port} ' + f'--user-data-dir="{chrome_debug_data_dir()}" --no-first-run ' + "--no-default-browser-check") return None def _detach_kwargs(system: str) -> dict: if system != "Windows": return {"start_new_session": True} - flags = getattr(subprocess, "DETACHED_PROCESS", 0) | getattr( - subprocess, "CREATE_NEW_PROCESS_GROUP", 0 - ) + flags = (getattr(subprocess, "DETACHED_PROCESS", 0) + | getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0)) return {"creationflags": flags} if flags else {} def _wait_for_browser_debug_ready_or_exit( - proc: subprocess.Popen, - port: int, - timeout: float = 2.0, - interval: float = 0.1, -) -> str: - """Classify a launched browser as ready, exited, or still starting. + proc: subprocess.Popen, port: int, timeout: float = 2.0, interval: float = 0.1) -> str: + """Classify a launched browser as "ready", "exited" or "starting". - We only need to wait long enough to catch the common failure mode where a candidate binary - exists but exits immediately before exposing the CDP port. Slower browsers can still finish - starting after this grace window. + The grace window only needs to catch a candidate that exits immediately before exposing the + CDP port; slower browsers may still finish starting after "starting" is returned. """ deadline = time.monotonic() + timeout - while time.monotonic() < deadline: - # Dual-stack: a squatter on the IPv4 loopback can push the browser - # to bind [::1] only — check both so a successful launch is seen. + # Dual-stack: a squatter on the IPv4 loopback can push the browser to bind [::1] only. if discover_local_cdp_url(port, timeout=min(interval, 0.2)): return "ready" if proc.poll() is not None: return "exited" time.sleep(interval) - return "starting" @@ -1212,7 +909,6 @@ _STDERR_TAIL_LIMIT = 2000 @dataclass class LaunchAttempt: """Outcome of one candidate-binary launch attempt.""" - binary: str state: str # "ready" | "starting" | "exited" | "spawn-failed" returncode: int | None = None @@ -1221,13 +917,8 @@ class LaunchAttempt: @dataclass class ChromeDebugLaunch: - """Structured result of ``launch_chrome_debug``. - - ``launched`` mirrors the legacy boolean contract: a launch command was executed and the browser - is ready or still starting (it does NOT guarantee the CDP port ever opens). ``attempts`` carries - per-candidate diagnostics so callers can explain *why* nothing came up. - """ - + """Result of ``launch_chrome_debug``: ``launched`` = a browser was spawned and is ready or + still starting (NOT a guarantee the CDP port ever opens); ``attempts`` explains *why* not.""" launched: bool = False attempts: list[LaunchAttempt] = field(default_factory=list) @@ -1241,34 +932,29 @@ class ChromeDebugLaunch: f"{name} exited immediately without opening the debug port — an already-running " f"{name} instance likely absorbed the launch (Chromium's single-instance " "behavior). Close ALL of its processes (including background/tray instances) " - "and retry /browser connect." - ) + "and retry /browser connect.") for attempt in self.attempts: if attempt.state == "exited" and attempt.stderr_tail: return ( f"{os.path.basename(attempt.binary)} exited before the debug port opened: " - f"{attempt.stderr_tail.splitlines()[-1].strip()}" - ) + f"{attempt.stderr_tail.splitlines()[-1].strip()}") return None def _read_stderr_tail(path: str) -> str: try: with open(path, "rb") as fh: - data = fh.read() - return data[-_STDERR_TAIL_LIMIT:].decode("utf-8", errors="replace").strip() + return fh.read()[-_STDERR_TAIL_LIMIT:].decode("utf-8", errors="replace").strip() except OSError: return "" def launch_chrome_debug( - port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None -) -> ChromeDebugLaunch: - """Launch a Chromium-family browser with remote debugging, with diagnostics. + port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> ChromeDebugLaunch: + """Launch a Chromium-family browser with remote debugging, trying each candidate in turn. - Tries each detected candidate binary in turn. A candidate that exits before the CDP port opens - (crash, singleton forward to an existing instance, bad profile dir) is logged — with exit code - and a stderr tail — and the next candidate is tried. + A candidate that exits before the CDP port opens (crash, singleton forward to an existing + instance, bad profile dir) is logged with exit code + stderr tail and the next one is tried. """ system = system or platform.system() result = ChromeDebugLaunch() @@ -1280,47 +966,29 @@ def launch_chrome_debug( data_dir = chrome_debug_data_dir() os.makedirs(data_dir, exist_ok=True) stderr_path = os.path.join(data_dir, _LAUNCH_STDERR_LOG) - for candidate in candidates: try: with open(stderr_path, "wb") as stderr_file: proc = subprocess.Popen( [candidate, *_chrome_debug_args(port)], - stdout=subprocess.DEVNULL, - stderr=stderr_file, - **_detach_kwargs(system), - ) + stdout=subprocess.DEVNULL, stderr=stderr_file, **_detach_kwargs(system)) except Exception as exc: result.attempts.append(LaunchAttempt(binary=candidate, state="spawn-failed")) logger.info("browser debug launch: failed to spawn %s: %s", candidate, exc) continue - logger.info( "browser debug launch: spawned %s (pid=%s) with --remote-debugging-port=%d", - candidate, - getattr(proc, "pid", None), - port, - ) + candidate, getattr(proc, "pid", None), port) state = _wait_for_browser_debug_ready_or_exit(proc, port) attempt = LaunchAttempt(binary=candidate, state=state) result.attempts.append(attempt) - if state != "exited": result.launched = True return result - attempt.returncode = getattr(proc, "returncode", None) attempt.stderr_tail = _read_stderr_tail(stderr_path) logger.warning( "browser debug launch: %s exited (code=%s) before port %d opened%s", - candidate, - attempt.returncode, - port, - f"; stderr tail: {attempt.stderr_tail}" if attempt.stderr_tail else "", - ) - + candidate, attempt.returncode, port, + f"; stderr tail: {attempt.stderr_tail}" if attempt.stderr_tail else "") return result - - -def try_launch_chrome_debug(port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> bool: - return launch_chrome_debug(port, system).launched From 9cb873ad0108327911db803dde9af4025a9a8432 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:20:16 -0700 Subject: [PATCH 5/7] refactor(hermes_cli): active_sessions refusal helper, Counter holders, single-pass transfer (676->656 LOC) --- hermes_cli/active_sessions.py | 88 ++++++++++++++--------------------- 1 file changed, 34 insertions(+), 54 deletions(-) diff --git a/hermes_cli/active_sessions.py b/hermes_cli/active_sessions.py index f3f5531dea..57c774ffa3 100644 --- a/hermes_cli/active_sessions.py +++ b/hermes_cli/active_sessions.py @@ -9,11 +9,12 @@ from __future__ import annotations import json import logging +import collections import math import os import time import uuid -from contextlib import contextmanager +from contextlib import contextmanager, suppress from dataclasses import dataclass from pathlib import Path from typing import Any, Iterator, Optional @@ -32,16 +33,9 @@ def coerce_max_concurrent_sessions(value: Any, key: str = "max_concurrent_sessio if value is None: return None try: - if isinstance(value, bool): + if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): raise ValueError(value) - if isinstance(value, float): - if not value.is_integer(): - raise ValueError(value) - parsed = int(value) - elif isinstance(value, str): - parsed = int(value.strip(), 10) - else: - parsed = int(value) + parsed = int(value.strip(), 10) if isinstance(value, str) else int(value) except (TypeError, ValueError): logger.warning( "Ignoring invalid %s=%r (expected a positive integer; 0/null disables)", key, value @@ -79,10 +73,7 @@ def summarize_holders(entries: list[dict[str, Any]]) -> str: """Compact "who is holding the slots" phrase, e.g. ``desktop x4, cli``.""" if not entries: return "" - counts: dict[str, int] = {} - for entry in entries: - surface = str(entry.get("surface") or "unknown") - counts[surface] = counts.get(surface, 0) + 1 + counts = collections.Counter(str(e.get("surface") or "unknown") for e in entries) held = ", ".join( f"{surface} x{n}" if n > 1 else surface for surface, n in sorted(counts.items(), key=lambda kv: (-kv[1], kv[0])) @@ -177,7 +168,7 @@ def _lease_paths( if lease is not None and lease.state_path is not None and lease.lock_path is not None: return lease.state_path, lease.lock_path home = _registry_home(registry_home) - return _state_path(home), _lock_path(home) + return home / "runtime" / "active_sessions.json", home / "runtime" / "active_sessions.lock" def _flock(fh, *, lock: bool) -> None: @@ -208,16 +199,11 @@ class _FileLock: return self def __exit__(self, exc_type, exc, tb): - if self._fh is None: - return - try: - _flock(self._fh, lock=False) - except Exception: - pass - try: - self._fh.close() - finally: - self._fh = None + fh, self._fh = self._fh, None + if fh is not None: + with suppress(Exception): + _flock(fh, lock=False) + fh.close() def _read_entries(path: Path, *, strict: bool = False) -> list[dict[str, Any]]: @@ -330,7 +316,7 @@ def _pid_liveness(pid: Any, process_start_time: Any = None, *, lenient: bool = F try: pid_int = int(pid) except (TypeError, ValueError): - return unknown_dead + pid_int = 0 if pid_int <= 0: return unknown_dead try: @@ -493,6 +479,11 @@ def try_acquire_active_session( if pruned: logger.info("Pruned %d stale active session lease(s)", pruned) + def refuse(message: str, reason: str, log: str, *args) -> tuple[None, ActiveSessionRefusal]: + _write_entries(state_path, entries) # persist the prune even when refusing + logger.info(log, *args) + return None, ActiveSessionRefusal(message, reason) + # Correctness first, under the same lock that just pruned dead owners. # An empty key is exempt: treating "" as an identity would make every # unsaved draft exclude every other one. @@ -508,26 +499,19 @@ def try_acquire_active_session( entries[index] = entry _write_entries(state_path, entries) return lease, None - _write_entries(state_path, entries) - logger.info( + return refuse( + session_already_owned_message(key, existing), SESSION_NOT_OWNED, "Refused active session %s: already held by pid=%s surface=%s", key, existing.get("pid"), existing.get("surface"), ) - return None, ActiveSessionRefusal( - session_already_owned_message(key, existing), SESSION_NOT_OWNED - ) # Capacity second, and only when an operator asked for one. if max_sessions is not None and len(entries) >= max_sessions: - active_count = len(entries) - _write_entries(state_path, entries) - logger.info( - "Active session limit reached: active=%d max=%d surface=%s", - active_count, max_sessions, surface, - ) - return None, ActiveSessionRefusal( - active_session_limit_message(active_count, max_sessions, entries), + return refuse( + active_session_limit_message(len(entries), max_sessions, entries), MAX_CONCURRENT_SESSIONS, + "Active session limit reached: active=%d max=%d surface=%s", + len(entries), max_sessions, surface, ) entries.append(entry) _write_entries(state_path, entries) @@ -577,26 +561,22 @@ def transfer_active_session( if loaded is None: return False entries = loaded[1] - updated = False - for entry in entries: - if str(entry.get("lease_id") or "") != lease.lease_id: - continue - entry["session_id"] = new_session_id - entry["updated_at"] = time.time() + own = next((e for e in entries if str(e.get("lease_id") or "") == lease.lease_id), None) + if own is not None: + own["session_id"] = new_session_id + own["updated_at"] = time.time() if metadata: - entry["metadata"] = _clean_metadata(metadata) - updated = True - break - if not updated and lease.track_liveness: + own["metadata"] = _clean_metadata(metadata) + elif lease.track_liveness: entries.append(_lease_entry( lease_id=lease.lease_id, session_id=new_session_id, surface=lease.surface, metadata=metadata, track_liveness=True, )) - updated = True - if updated: - _write_entries(state_path, entries) - lease.session_id = new_session_id - return updated + else: + return False + _write_entries(state_path, entries) + lease.session_id = new_session_id + return True def release_orphaned_leases(live_lease_ids: set[str]) -> int: From 7f3dabe75131f337b4f30d3053f972d848e88e2d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:43:58 -0700 Subject: [PATCH 6/7] refactor(hermes_cli): browser_connect predicate/next() collapses (994->965 LOC) --- hermes_cli/browser_connect.py | 80 ++++++++++++----------------------- 1 file changed, 26 insertions(+), 54 deletions(-) diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index 847c8cf7d9..dd227a2556 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -204,10 +204,7 @@ def real_profile_data_dir(browser: str, system: str | None = None) -> str | None def _first_present(paths) -> str | None: - for p in paths: - if p and os.path.isfile(p): - return p - return None + return next((p for p in paths if p and os.path.isfile(p)), None) def chromium_executable(browser: str, system: str | None = None) -> str | None: @@ -225,11 +222,8 @@ def chromium_executable(browser: str, system: str | None = None) -> str | None: os.environ.get("LOCALAPPDATA", str(Path.home() / "AppData" / "Local"))] return _first_present(os.path.join(base, *parts) for base in bases for parts in b.win_install) # Linux: PATH lookup first, then the known absolute install paths. - for name in b.linux_exec or b.linux_bins: - found = shutil.which(name) - if found: - return found - return _first_present(b.linux_paths) + found = next(filter(None, map(shutil.which, b.linux_exec or b.linux_bins)), None) + return found or _first_present(b.linux_paths) def _classify_default(value: str, channels, table, match) -> str | None: @@ -238,13 +232,9 @@ def _classify_default(value: str, channels, table, match) -> str | None: Channels are checked FIRST: a recognized Beta/Dev/Canary identifier must fail closed (UNSUPPORTED_CHANNEL), never fall through to a stable match and drive the stable profile. """ - for chan in channels: - if match(value, chan): - return UNSUPPORTED_CHANNEL - for frag, browser in table: - if match(value, frag): - return browser - return None + if any(match(value, chan) for chan in channels): + return UNSUPPORTED_CHANNEL + return next((browser for frag, browser in table if match(value, frag)), None) def _detect_default_windows() -> str | None: @@ -296,19 +286,15 @@ def _launchservices_https_handler(dump: str) -> str | None: # releases used, and the role regex below would return it instead of the bundle id. low = re.sub(r"lshandlerpreferredversions\s*=\s*\{[^}]*\}\s*;", "", low) # The real bundle id is the first non-"-" role value at this level. - for role in re.findall(r'lshandlerrole(?:all|viewer)\s*=\s*"?([a-z0-9.\-]+)"?\s*;', low): - if role != "-": - return role - return None + roles = re.findall(r'lshandlerrole(?:all|viewer)\s*=\s*"?([a-z0-9.\-]+)"?\s*;', low) + return next((role for role in roles if role != "-"), None) return None def _detect_default_darwin() -> str | None: out = _run_stdout(["defaults", "read", "com.apple.LaunchServices/com.apple.launchservices.secure", "LSHandlers"]) - if out is None: - return None - bundle = _launchservices_https_handler(out) + bundle = _launchservices_https_handler(out) if out is not None else None if not bundle: return None # Exact match. A non-Chromium https handler (Safari, Firefox, Arc, …) or an unknown @@ -451,15 +437,11 @@ def _mirror_profile_auth(src: str, dst: str, source_profile: str) -> int: Returns the number of DB auth files that could NOT be copied (0 = clean). """ - dst_default = os.path.join(dst, "Default") failed_dbs = 0 for rel in _AUTH_REFRESH_PROFILE_FILES: s = os.path.join(src, source_profile, rel) - if not os.path.isfile(s): - continue - ok = _copy_auth_file(s, os.path.join(dst_default, rel)) - if not ok and os.path.basename(rel) in _SQLITE_AUTH_DBS: - failed_dbs += 1 + if os.path.isfile(s) and not _copy_auth_file(s, os.path.join(dst, "Default", rel)): + failed_dbs += os.path.basename(rel) in _SQLITE_AUTH_DBS return failed_dbs @@ -484,10 +466,8 @@ def _profile_is_locked(src: str, source_profile: str) -> bool: try: with open(db, "rb"): return False - except PermissionError: - return True - except OSError: # transient — don't declare locked; let the copy try - return False + except OSError as e: # other OSErrors are transient — don't declare locked; let the copy try + return isinstance(e, PermissionError) def _browser_setting(key: str): @@ -587,7 +567,7 @@ def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool for p in targets: with contextlib.suppress(*gone_errs): p.terminate() - _gone, alive = psutil.wait_procs(targets, timeout=min(timeout, 8.0)) + alive = psutil.wait_procs(targets, timeout=min(timeout, 8.0))[1] for p in alive: with contextlib.suppress(*gone_errs): p.kill() @@ -708,9 +688,8 @@ def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | N os.makedirs(dst, exist_ok=True) # Secure the snapshot dir AND its browser-profile parent on EVERY launch so a failed # first attempt or an older-build dir still converges to owner-only perms. - if os.path.dirname(dst): - _secure_snapshot(os.path.dirname(dst)) - _secure_snapshot(dst) + for path in filter(None, (os.path.dirname(dst), dst)): + _secure_snapshot(path) _sync_local_state(src, dst, source_profile) if not populated: _copy_profile_tree(src, dst, source_profile) @@ -770,14 +749,12 @@ def _debug_candidate_paths(system: str): def get_chrome_debug_candidates(system: str) -> list[str]: - candidates: list[str] = [] - seen: set[str] = set() - for path in _debug_candidate_paths(system): - normalized = os.path.normcase(os.path.normpath(path)) if path else None - if path and normalized not in seen and os.path.isfile(path): - candidates.append(path) - seen.add(normalized) - return candidates + candidates: dict[str, str] = {} # normalized -> first path seen (dedupe, keep order) + for path in filter(None, _debug_candidate_paths(system)): + normalized = os.path.normcase(os.path.normpath(path)) + if normalized not in candidates and os.path.isfile(path): + candidates[normalized] = path + return list(candidates.values()) def chrome_debug_data_dir() -> str: @@ -829,11 +806,8 @@ _LOOPBACK_SOCKET_HOSTS = ("127.0.0.1", "::1") def discover_local_cdp_url(port: int, timeout: float = 1.0) -> str | None: """Return the first loopback URL (IPv4 first, then IPv6) speaking CDP, else None.""" - for host in _LOOPBACK_PROBE_HOSTS: - url = f"http://{host}:{port}" - if is_browser_debug_ready(url, timeout=timeout): - return url - return None + urls = (f"http://{host}:{port}" for host in _LOOPBACK_PROBE_HOSTS) + return next((url for url in urls if is_browser_debug_ready(url, timeout=timeout)), None) def local_port_in_use(port: int, timeout: float = 0.5) -> bool: @@ -857,10 +831,8 @@ def find_free_debug_port(preferred: int = DEFAULT_BROWSER_CDP_PORT, attempts: in except OSError: return False loopbacks = ((socket.AF_INET, "127.0.0.1"), (socket.AF_INET6, "::1")) - for port in range(preferred + 1, preferred + 1 + attempts): - if all(bindable(family, host, port) for family, host in loopbacks): - return port - return preferred + 1 + ports = range(preferred + 1, preferred + 1 + attempts) + return next((p for p in ports if all(bindable(f, h, p) for f, h in loopbacks)), preferred + 1) def manual_chrome_debug_command(port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> str | None: From 27287e6c88bdc0b6724c639c8cfa834f4ab8cc0c Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:54:48 -0700 Subject: [PATCH 7/7] refactor(hermes_cli): browser_connect docstring compaction (965->919 LOC) --- hermes_cli/browser_connect.py | 143 ++++++++++++---------------------- 1 file changed, 48 insertions(+), 95 deletions(-) diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index dd227a2556..201a5ca71c 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -177,11 +177,9 @@ UNSUPPORTED_CHANNEL = "__unsupported_channel__" def real_profile_data_dir(browser: str, system: str | None = None) -> str | None: - """Return the default user-data-dir for a Chromium ``browser`` on ``system`` (None if unknown). - - Linux tries native ($XDG_CONFIG_HOME), snap and Flatpak; first existing wins, else the native - path so the caller's error names it. Darwin/Windows paths are not stat'ed. - """ + """Default user-data-dir for ``browser`` on ``system`` (None if unknown). Linux tries native + ($XDG_CONFIG_HOME), snap and Flatpak — first existing wins, else native so the caller's + error names it. Darwin/Windows paths are not stat'ed.""" b = _BROWSER_BY_KEY.get(browser) if b is None: return None @@ -227,11 +225,8 @@ def chromium_executable(browser: str, system: str | None = None) -> str | None: def _classify_default(value: str, channels, table, match) -> str | None: - """Map an OS default-browser identifier to a canonical key. - - Channels are checked FIRST: a recognized Beta/Dev/Canary identifier must fail closed - (UNSUPPORTED_CHANNEL), never fall through to a stable match and drive the stable profile. - """ + """Map an OS default-browser identifier to a canonical key. Channels are checked FIRST: a + Beta/Dev/Canary id must fail closed (UNSUPPORTED_CHANNEL), never match the stable profile.""" if any(match(value, chan) for chan in channels): return UNSUPPORTED_CHANNEL return next((browser for frag, browser in table if match(value, frag)), None) @@ -261,12 +256,9 @@ def _run_stdout(argv: list[str]) -> str | None: def _launchservices_https_handler(dump: str) -> str | None: - """Return the bundle id registered for the ``https`` URL scheme. - - ``dump`` is ``defaults read … LSHandlers`` output: an array of ``{ … }`` dicts, one per handler. - Only the ``LSHandlerURLScheme = https`` entry counts — a browser registered for another scheme - or a file type must not be mistaken for the default. - """ + """Bundle id registered for ``https`` in ``defaults read … LSHandlers`` output (an array of + ``{ … }`` dicts). Only the ``LSHandlerURLScheme = https`` entry counts — a browser + registered for another scheme or a file type must not be mistaken for the default.""" entries: list[str] = [] depth, buf = 0, [] for ch in dump: @@ -358,11 +350,8 @@ def real_profile_copy_dir(browser: str) -> str: def _last_used_profile(src: str) -> str: - """Profile dir Chrome last used (``Local State`` → profile.last_used), else ``Default``. - - The user's signed-in session usually lives in the profile they actually browse (``Profile 6``), - not ``Default``; its auth is what gets mirrored into the copy's ``Default``. - """ + """Profile dir Chrome last used (``Local State`` → profile.last_used), else ``Default``. The + signed-in session usually lives in the profile actually browsed (``Profile 6``), not Default.""" try: with open(os.path.join(src, "Local State"), encoding="utf-8", errors="replace") as fh: state = json.load(fh) @@ -374,13 +363,11 @@ def _last_used_profile(src: str) -> str: def _secure_snapshot(path: str, *, contents: bool = False) -> None: """Lock down a snapshot dir (or, with ``contents``, everything INSIDE it) as a secret store. - - The snapshot holds the user's Cookies / Login Data, so it gets the same owner-only permissions - (managed-mode / NixOS group-share carve-out, HERMES_UID/GID ownership) as every other Hermes - secret dir — via ``_secure_dir``/``_secure_file``, not a bespoke chmod. Contents matter too - (#96729): ``copy2`` keeps Chrome's 0644 modes and sqlite backups land umask-wide, so cookies - were world-readable under the ``HERMES_HOME_MODE`` hatch. Best-effort; never blocks a launch. - """ + It holds the user's Cookies / Login Data, so it gets the same owner-only perms (managed-mode / + NixOS group-share carve-out, HERMES_UID/GID) as every Hermes secret dir — via ``_secure_dir``/ + ``_secure_file``, not a bespoke chmod. Contents matter too (#96729): ``copy2`` keeps Chrome's + 0644 and sqlite backups land umask-wide, so cookies were world-readable under the + ``HERMES_HOME_MODE`` hatch. Best-effort; never blocks a launch.""" try: from hermes_cli.config import _secure_dir, _secure_file if not contents: @@ -403,11 +390,8 @@ _SQLITE_AUTH_DBS = frozenset({"Cookies", "Login Data", "Login Data For Account", def _copy_auth_file(src_file: str, dst_file: str) -> bool: - """Copy one auth file, lock-aware; True on success. - - SQLite DBs use the online-backup API (works under a Windows write lock) and fall through to a - raw copy if that fails; everything else is a plain copy. Failure only if BOTH fail. - """ + """Copy one auth file, lock-aware; True on success. SQLite DBs use the online-backup API (works + under a Windows write lock), falling through to a raw copy; failure only if BOTH fail.""" os.makedirs(os.path.dirname(dst_file), exist_ok=True) if os.path.basename(src_file) in _SQLITE_AUTH_DBS: # With a live Chrome on macOS, mode=ro WITHOUT immutable=1 can hang connect/backup @@ -433,10 +417,8 @@ def _copy_auth_file(src_file: str, dst_file: str) -> bool: def _mirror_profile_auth(src: str, dst: str, source_profile: str) -> int: - """Mirror ``source_profile``'s auth files into the copy's ``Default`` (agent-browser opens it). - - Returns the number of DB auth files that could NOT be copied (0 = clean). - """ + """Mirror ``source_profile``'s auth files into the copy's ``Default`` (agent-browser opens it); + returns the number of DB auth files that could NOT be copied (0 = clean).""" failed_dbs = 0 for rel in _AUTH_REFRESH_PROFILE_FILES: s = os.path.join(src, source_profile, rel) @@ -452,12 +434,9 @@ _PROFILE_LOCKED_PREFIX = "[profile-locked] " def _profile_is_locked(src: str, source_profile: str) -> bool: - """True when the active profile's cookie DB can't be opened (browser running). - - On Windows a running browser holds Cookies deny-all, so open raises PermissionError. This FAST - probe fails closed BEFORE the heavy snapshot so a locked profile can never hang the launch. - POSIX has no mandatory locking, so it returns False. - """ + """True when the active profile's cookie DB can't be opened (browser running). On Windows a + running browser holds Cookies deny-all (PermissionError); this FAST probe fails closed BEFORE + the heavy snapshot so a locked profile never hangs the launch. Always False on POSIX.""" db = _first_present( # modern Network/ location first os.path.join(src, source_profile, rel) for rel in (os.path.join("Network", "Cookies"), "Cookies")) @@ -482,21 +461,16 @@ def _browser_setting(key: str): def _real_profile_pin() -> str | None: - """Pinned source profile dir name from ``browser.real_profile_pin``. - - Unset, the snapshot follows Chrome's ``profile.last_used``, which with work + personal profiles - can hand the agent the wrong identity. When set (``"Profile 2"``) that profile is ALWAYS copied. + """Pinned source profile dir name from ``browser.real_profile_pin`` (ALWAYS copied when set). + Unset, the snapshot follows ``profile.last_used``, which can hand the agent the wrong identity. """ pin = _browser_setting("real_profile_pin") return pin.strip() if isinstance(pin, str) and pin.strip() else None def _resolve_source_profile(src: str) -> tuple[str | None, str | None]: - """Return ``(profile_dir_name, error)``: the pin if set, else last_used. - - A pin that does not exist under ``src`` FAILS CLOSED — falling back to last_used would - silently browse as the wrong identity, the exact wrong-principal bug the pin prevents. - """ + """``(profile_dir_name, error)``: the pin if set, else last_used. A pin missing under ``src`` + FAILS CLOSED — falling back would silently browse as the wrong identity (wrong-principal).""" pin = _real_profile_pin() if pin: if os.path.isdir(os.path.join(src, pin)): @@ -515,11 +489,9 @@ def _real_profile_autoclose() -> bool: def _processes_holding_profile(src: str): - """Yield psutil.Process instances holding the user-data-dir ``src`` open. - - A process qualifies only when it's a Chromium-family binary AND its cmdline references THIS - user-data-dir — never an unrelated same-PID process. Unreadable cmdline is skipped. - """ + """Yield psutil.Process instances holding ``src`` open: Chromium-family binaries whose + cmdline references THIS user-data-dir — never an unrelated same-PID process. An unreadable + cmdline is skipped.""" try: import psutil except ImportError: # hard dep; defensive @@ -545,11 +517,8 @@ def _processes_holding_profile(src: str): def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool, str]: - """Terminate the browser process tree holding ``src`` and wait for release. - - CONSENTED, DESTRUCTIVE: terminates (then kills) every Chromium-family process bound to this - exact user-data-dir, losing unsaved tab/form state. Only call after the user agreed. - """ + """Terminate the browser process tree holding ``src`` and wait for release. CONSENTED, + DESTRUCTIVE (unsaved tab/form state is lost): only call after the user agreed.""" try: import psutil except ImportError: @@ -586,11 +555,9 @@ def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool def _sync_local_state(src: str, dst: str, source_profile: str) -> None: """Copy ``Local State`` into the snapshot and rewrite it for the single ``Default`` profile. - - A verbatim Local State still names the SOURCE profile (last_used="Profile 2", info_cache of - Profile 2/4/7) that the copy doesn't contain, so Chrome would start SIGNED OUT. CRITICAL: the - Default entry must be the SOURCE profile's identity (name + account) because the Default DIR - holds its cookies; a mismatch makes Chrome demand "Continue as " on every launch. + Verbatim it names the SOURCE profile (last_used="Profile 2", info_cache of 2/4/7) the copy + lacks, so Chrome would start SIGNED OUT. CRITICAL: Default's entry must be the SOURCE profile's + identity (Default DIR holds its cookies), else Chrome demands "Continue as " each launch. """ ls_src, ls_dst = os.path.join(src, "Local State"), os.path.join(dst, "Local State") if os.path.isfile(ls_src): @@ -618,10 +585,8 @@ def _sync_local_state(src: str, dst: str, source_profile: str) -> None: def _locked_profile_error(browser: str) -> str: """Fail-closed message for a locked profile; offers auto-close only when consent arms it. - - NEVER kill from here: closing the user's browser is destructive and must be an explicit, - per-attempt, user-approved step. A still-locked retry blocks again — no auto-retry, no loop. - """ + NEVER kill from here: closing the browser is destructive and must be an explicit per-attempt, + user-approved step. A still-locked retry blocks again — no auto-retry, no loop.""" if _real_profile_autoclose(): msg = ( f"{browser} is running and has its profile locked, so its login data can't be copied " @@ -638,11 +603,9 @@ def _locked_profile_error(browser: str) -> str: def _copy_profile_tree(src: str, dst: str, source_profile: str) -> None: - """Fresh (or torn-and-rebuilding) copy of the ACTIVE profile dir into the copy's Default. - - Excludes caches AND the SQLite auth DBs (a raw copytree of a file a running Chrome holds open - raises on Windows); ``_mirror_profile_auth`` copies the DBs lock-aware instead. - """ + """Fresh (or torn-and-rebuilding) copy of the ACTIVE profile dir into the copy's Default, + minus caches AND the SQLite auth DBs (raw copytree of a Chrome-held file raises on Windows); + ``_mirror_profile_auth`` copies the DBs lock-aware instead.""" dst_default = os.path.join(dst, "Default") try: shutil.rmtree(dst_default, ignore_errors=True) @@ -662,11 +625,9 @@ def _copy_profile_tree(src: str, dst: str, source_profile: str) -> None: def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | None, str | None]: """Snapshot ``browser``'s real ACTIVE profile into the hermes copy dir; returns ``(dst, err)``. - Copies ``Local State`` plus the active profile's auth files into the copy's ``Default``. The - completion marker is written only after a full success, so a torn first copy (disk full, - Ctrl+C) never looks "already populated" — it is redone from scratch. - """ + completion marker is written only after full success, so a torn first copy (disk full, Ctrl+C) + never looks "already populated" — it is redone from scratch.""" src = src or real_profile_data_dir(browser) if not src or not os.path.isdir(src): return None, ( @@ -811,11 +772,8 @@ def discover_local_cdp_url(port: int, timeout: float = 1.0) -> str | None: def local_port_in_use(port: int, timeout: float = 0.5) -> bool: - """True when either loopback accepts TCP on ``port``. - - Used AFTER a failed CDP probe to tell "port is free, launch here" from "another application - (IDE debugger, dev server) is squatting the port and a launch would fight it". - """ + """True when either loopback accepts TCP on ``port``. Used AFTER a failed CDP probe to tell + "port is free, launch here" from "another application is squatting it; a launch would fight".""" return any(_tcp_open(host, port, timeout) for host in _LOOPBACK_SOCKET_HOSTS) @@ -858,11 +816,8 @@ def _detach_kwargs(system: str) -> dict: def _wait_for_browser_debug_ready_or_exit( proc: subprocess.Popen, port: int, timeout: float = 2.0, interval: float = 0.1) -> str: - """Classify a launched browser as "ready", "exited" or "starting". - - The grace window only needs to catch a candidate that exits immediately before exposing the - CDP port; slower browsers may still finish starting after "starting" is returned. - """ + """Classify a launched browser as "ready", "exited" or "starting". The grace window only needs + to catch a candidate that exits immediately; slower browsers may still finish starting later.""" deadline = time.monotonic() + timeout while time.monotonic() < deadline: # Dual-stack: a squatter on the IPv4 loopback can push the browser to bind [::1] only. @@ -923,11 +878,9 @@ def _read_stderr_tail(path: str) -> str: def launch_chrome_debug( port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> ChromeDebugLaunch: - """Launch a Chromium-family browser with remote debugging, trying each candidate in turn. - - A candidate that exits before the CDP port opens (crash, singleton forward to an existing - instance, bad profile dir) is logged with exit code + stderr tail and the next one is tried. - """ + """Launch a Chromium-family browser with remote debugging, trying each candidate in turn. One + that exits before the CDP port opens (crash, singleton forward, bad profile dir) is logged with + exit code + stderr tail and the next is tried.""" system = system or platform.system() result = ChromeDebugLaunch() candidates = get_chrome_debug_candidates(system)