diff --git a/tests/tools/test_skills_sync_client.py b/tests/tools/test_skills_sync_client.py index fd9b71fb01..a6fdcdb884 100644 --- a/tests/tools/test_skills_sync_client.py +++ b/tests/tools/test_skills_sync_client.py @@ -430,22 +430,22 @@ class TestObjectBuilding: class TestMergeDecision: def test_no_change(self): - assert ssc._merge_skill("b", "b", "b") == "either" + assert ssc.merge_skill("b", "b", "b") == "either" def test_ours_only_changed(self): - assert ssc._merge_skill("b", "o", "b") == "ours" + assert ssc.merge_skill("b", "o", "b") == "ours" def test_theirs_only_changed(self): - assert ssc._merge_skill("b", "b", "t") == "theirs" + assert ssc.merge_skill("b", "b", "t") == "theirs" def test_both_converged(self): - assert ssc._merge_skill("b", "x", "x") == "either" + assert ssc.merge_skill("b", "x", "x") == "either" def test_true_overlap(self): - assert ssc._merge_skill("b", "o", "t") == "overlap" + assert ssc.merge_skill("b", "o", "t") == "overlap" def test_deleted_both(self): - assert ssc._merge_skill(None, None, None) == "none" + assert ssc.merge_skill(None, None, None) == "none" # --------------------------------------------------------------------------- @@ -683,7 +683,7 @@ class TestSyncManifest: # The manifest is a root-level BLOB, not a skill subtree, so the skill # walk must not surface it as a skill. - trees = ssc._skill_trees_of_root(client, root_hash) + trees = ssc.skill_trees_of_root(client, root_hash) assert "sync-manifest" not in trees assert set(trees) == {"alpha", "devops/beta"} diff --git a/tools/skills_sync_client.py b/tools/skills_sync_client.py index df5ba80050..858bbc1f46 100644 --- a/tools/skills_sync_client.py +++ b/tools/skills_sync_client.py @@ -1,18 +1,12 @@ #!/usr/bin/env python3 -"""Skill Sync client -- the low-level sync layer. - -Builds content-addressed objects from local skills and talks the sync wire contract (push -objects + CAS a ref, pull the owner's HEAD, three-way merge on a 409). Driven by the debounced -push hook in ``skill_manage``, ``maybe_pull_skills`` at the curator tick sites, and ``hermes -sync``. Lives under tools/ (NOT hermes_cli/) so it never imports the CLI at module load; -``skills_sync_client_wire`` and ``skills_sync_client_org`` are re-exported here. -ACCESS GATE (pre-launch): sync is INERT unless the signed-in user is a Nous admin, read off the -``tool_gateway_admin`` JWT claim (NAS's misleading name for the global portal-admin permission; -replace with a real entitlement before shipping). OPT-IN DEFAULT (provisional): nothing syncs -unless the user marks a skill for sync -- local intent is the ``sync`` flag in ``.usage.json``, -the DURABLE cross-device state is the committed ``sync-manifest`` blob in the plane. Only -agent-created + user-authored skills under ``~/.hermes/skills/`` are eligible. -""" +"""Skill Sync client -- the low-level sync layer (push objects + CAS a ref, pull the owner's +HEAD, three-way merge on a 409). Driven by the debounced ``skill_manage`` push hook, the curator +tick ``maybe_pull_skills`` and ``hermes sync``. Lives under tools/ so it never imports the CLI at +module load; ``skills_sync_client_wire`` / ``skills_sync_client_org`` are re-exported here. +ACCESS GATE (pre-launch): INERT unless the user is a Nous admin per the ``tool_gateway_admin`` +JWT claim (NAS's misleading name for the global portal-admin permission; replace before shipping). +OPT-IN DEFAULT (provisional): local intent is the ``sync`` flag in ``.usage.json``; the DURABLE +cross-device state is the ``sync-manifest`` blob in the plane. Only ~/.hermes/skills/ skills qualify.""" from __future__ import annotations @@ -24,22 +18,16 @@ from pathlib import Path, PurePosixPath from typing import Any, Callable, Dict, List, Optional, Tuple from tools.skills_sync_client_wire import ( # noqa: F401 (re-exports) - ARTIFACT_TYPE_SKILL, DEFAULT_MAX_OBJECT_BYTES, KIND_BLOB, KIND_COMMIT, KIND_TREE, MODE_DIR, - MODE_EXEC, MODE_FILE, SYNC_MANIFEST_ENTRY_NAME, SYNC_MANIFEST_TYPE, SYNC_MANIFEST_VERSION, - WIRE_VERSION, ObjectSet, SyncClient, SyncConflict, SyncError, _check_version, - assemble_root_from_skill_trees, build_commit, build_root_tree, build_sync_manifest_bytes, - build_tree, canonical_json_bytes, materialize_tree, merge_skill, nest_skill_tree, - parse_sync_manifest, read_manifest_of_root, read_ref_hash, root_tree_of_commit, - skill_trees_of_root, wire_address) + DEFAULT_MAX_OBJECT_BYTES, KIND_BLOB, KIND_COMMIT, KIND_TREE, MODE_EXEC, MODE_FILE, ObjectSet, + SyncClient, SyncConflict, SyncError, _check_version, assemble_root_from_skill_trees, build_commit, + build_root_tree, build_sync_manifest_bytes, build_tree, canonical_json_bytes, materialize_tree, + merge_skill, nest_skill_tree, parse_sync_manifest, read_manifest_of_root, read_ref_hash, + root_tree_of_commit, skill_trees_of_root, wire_address) logger = logging.getLogger(__name__) -_merge_skill = merge_skill -_skill_trees_of_root = skill_trees_of_root - -# Identity & access gate. The bearer comes from resolve_nous_runtime_credentials() (file lock, -# host allowlist, refresh); its payload is decoded unverified to read the gate claim, whose -# wire name is NAS's and means "Nous admin" (Permissions.ADMIN_ACCESS). +# Gate claim (NAS's wire name; means "Nous admin" / Permissions.ADMIN_ACCESS). The bearer comes +# from resolve_nous_runtime_credentials(); its payload is decoded unverified to read this. NOUS_ADMIN_CLAIM = "tool_gateway_admin" @@ -48,13 +36,11 @@ class SyncInertError(RuntimeError): def resolve_identity() -> Dict[str, Any]: - """``{api_key, base_url, owner, nous_admin, claims}``; SyncInertError if not logged in / no - bearer. ``owner`` is advisory for local ref naming only -- the server derives the real owner - from the bearer. The JWT is decoded WITHOUT signature verification: safe because the claims - never drive authz here (the server re-verifies), only whether to attempt sync at all.""" + """``{api_key, base_url, owner, nous_admin, claims}``; SyncInertError if not logged in / no bearer. + ``owner`` is advisory (local ref naming; the server derives the real one). The JWT is decoded + WITHOUT verification: safe, the claims only decide whether to attempt sync, never authz.""" try: from hermes_cli.auth import resolve_nous_runtime_credentials - creds = resolve_nous_runtime_credentials() or {} except Exception as e: raise SyncInertError(f"no Nous credentials: {e}") from e @@ -63,22 +49,18 @@ def resolve_identity() -> Dict[str, Any]: raise SyncInertError("no bearer token available") try: import jwt # PyJWT, a core dependency - claims = jwt.decode(api_key, options={"verify_signature": False, "verify_exp": False}) or {} except Exception as e: logger.debug("skills_sync_client: JWT payload decode failed: %s", e) claims = {} owner = claims.get("sub") or claims.get("privy_did") or claims.get("tid") or "unknown" - return { - "api_key": api_key, "base_url": creds.get("base_url"), "owner": str(owner), - "nous_admin": claims.get(NOUS_ADMIN_CLAIM) is True, "claims": claims} + return {"api_key": api_key, "base_url": creds.get("base_url"), "owner": str(owner), + "nous_admin": claims.get(NOUS_ADMIN_CLAIM) is True, "claims": claims} -# Configuration -- env-first so a Hermes Cloud instance can enable sync purely through the -# environment. Every knob: HERMES_SYNC_ env -> config.yaml ``sync.`` -> default -# (keys: base_url = the sync plane, NOT the inference URL; enabled; default_opt_in; org_auto_propose). - -#: Production Skill Sync plane; a normal user configures nothing. +# Configuration -- env-first so Hermes Cloud can enable sync via environment alone. Every knob: +# HERMES_SYNC_ env -> config.yaml ``sync.`` -> default (base_url = the sync plane, NOT +# the inference URL; enabled; default_opt_in; org_auto_propose). DEFAULT_SYNC_BASE_URL = "https://gateway-gateway.nousresearch.com" _TRUE = {"1", "true", "yes", "on"} @@ -89,7 +71,6 @@ def _sync_config(key: str) -> Any: """``sync.`` from config.yaml, or None. Lazy import: must not import the CLI at module load.""" try: from hermes_cli.config import load_config - return ((load_config() or {}).get("sync") or {}).get(key) except Exception as e: logger.debug("skills_sync_client: config sync.%s read failed: %s", key, e) @@ -110,10 +91,8 @@ def resolve_sync_base_url() -> Optional[str]: def _parse_bool(value: Any) -> Optional[bool]: """Parse a config/env bool; None if unrecognized so callers fall through to the next layer.""" - if isinstance(value, bool): + if isinstance(value, bool) or value is None: return value - if value is None: - return None s = str(value).strip().lower() return True if s in _TRUE else False if s in _FALSE else None @@ -148,7 +127,6 @@ def sync_default_opt_in() -> bool: def _skills_dir() -> Path: from hermes_constants import get_hermes_home - return get_hermes_home() / "skills" @@ -177,9 +155,8 @@ def _skill_rel_path(skill_name: str) -> Optional[PurePosixPath]: def is_sync_eligible(skill_name: str) -> bool: - """Candidate for sync (before the opt-in check): present locally, NOT bundled, NOT hub-installed, - NOT external, NOT under the ``_org/`` mirror (enterprise content must never ride a personal - push). Mirrors the curator's exclusions (tools/skill_usage.py).""" + """Sync candidate (before opt-in): local, NOT bundled/hub-installed/external, NOT under ``_org/`` + (enterprise content never rides a personal push). Mirrors the curator's exclusions.""" try: from tools.skill_usage import is_bundled, is_hub_installed, _find_skill_dir from agent.skill_utils import is_external_skill_path @@ -195,9 +172,8 @@ def is_sync_eligible(skill_name: str) -> bool: def list_synced_skill_names() -> List[str]: - """Names of skills that should sync (sorted, deduped), per ``sync_default_opt_in()``: opt-in -> - eligible skills whose usage record has ``sync: true``; opt-out -> every eligible skill unless - its record has ``sync: false``.""" + """Sorted skill names that should sync: opt-in -> eligible skills with ``sync: true``; + opt-out (``sync_default_opt_in()``) -> every eligible skill unless ``sync: false``.""" try: from tools.skill_usage import load_usage except Exception: @@ -212,8 +188,7 @@ def list_synced_skill_names() -> List[str]: def _all_local_skill_names() -> List[str]: - """Every locally-present skill name (a dir under ~/.hermes/skills/ with a ``SKILL.md``; - frontmatter ``name`` falling back to the dir name). Eligibility is applied by the caller.""" + """Every local skill name (dir under ~/.hermes/skills/ with SKILL.md; frontmatter ``name`` or dir name).""" names: List[str] = [] root = _skills_dir() try: @@ -221,12 +196,9 @@ def _all_local_skill_names() -> List[str]: if skill_md.is_symlink(): continue name = skill_md.parent.name - try: + with suppress(Exception): from tools.skill_usage import _read_skill_name - name = _read_skill_name(skill_md, name) - except Exception: - pass if name: names.append(name) except OSError as e: @@ -248,7 +220,6 @@ def _adopt_manifest_opt_ins(remote_manifest: Optional[Dict[str, bool]]) -> List[ return adopted try: from tools.skill_usage import set_sync, is_curation_eligible, is_sync_enabled - for sname, enabled in remote_manifest.items(): if enabled and is_curation_eligible(sname) and not is_sync_enabled(sname): set_sync(sname, True) @@ -264,7 +235,6 @@ def _default_device_label() -> str: """Short hostname + random suffix (two machines can share a hostname); bare uuid if unusable.""" import socket import uuid - try: host = socket.gethostname() or "" except OSError: @@ -274,17 +244,12 @@ def _default_device_label() -> str: def stable_device_id() -> str: - """Stable per-device label persisted at ~/.hermes/skills/.sync_device_id. An existing file - always wins; otherwise seeded from HERMES_SYNC_DEVICE_NAME (first use only; lets Hermes Cloud - name hosted instances) or a friendly default, then persisted so a later ``set_device_name()`` wins.""" - path = _skills_dir() / ".sync_device_id" - try: - if path.exists(): - val = path.read_text(encoding="utf-8").strip() - if val: - return val - except OSError: - pass + """Per-device label at ~/.hermes/skills/.sync_device_id. An existing file always wins; else seeded + from HERMES_SYNC_DEVICE_NAME (first use only, for Hermes Cloud) or a friendly default, then persisted.""" + with suppress(OSError): + val = _device_id_path().read_text(encoding="utf-8").strip() + if val: + return val val = (os.environ.get("HERMES_SYNC_DEVICE_NAME") or "").strip() or _default_device_label() try: _write_device_id(val) @@ -293,10 +258,13 @@ def stable_device_id() -> str: return val +def _device_id_path() -> Path: + return _skills_dir() / ".sync_device_id" + + def _write_device_id(val: str) -> None: - path = _skills_dir() / ".sync_device_id" - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(val, encoding="utf-8") + _device_id_path().parent.mkdir(parents=True, exist_ok=True) + _device_id_path().write_text(val, encoding="utf-8") def set_device_name(name: str) -> str: @@ -308,9 +276,8 @@ def set_device_name(name: str) -> str: return cleaned -# Local sync STATE: the last HEAD we pushed/pulled + the root tree at that point (FULL-digest -# namespace). Distinct from the bundled manifest (skills_sync.py) and from the plane's -# `sync-manifest` object. Lives at ~/.hermes/skills/.sync_state; legacy `.sync_manifest` migrates on read. +# Local sync STATE: last HEAD pushed/pulled + its root tree (FULL-digest namespace). Distinct from +# the bundled manifest (skills_sync.py) and the plane's `sync-manifest`. ~/.hermes/skills/.sync_state. _EMPTY_STATE: Dict[str, Any] = {"head": None, "skills": {}} @@ -326,60 +293,46 @@ def _load_state_file(path: Path, what: str = "sync state read") -> Optional[Dict except (OSError, json.JSONDecodeError) as e: logger.debug("skills_sync_client: %s failed: %s", what, e) return None - if not isinstance(data, dict): - return None - data.setdefault("head", None) - data.setdefault("skills", {}) - return data + return {**_EMPTY_STATE, **data} if isinstance(data, dict) else None def read_sync_state() -> Dict[str, Any]: - """``{"head": "sha256:...|null", "skills": {...}}``; a default on missing/corrupt. If - ``.sync_state`` is absent but the legacy ``.sync_manifest`` exists it is read, rewritten to - the new path and removed, so no head record is lost.""" + """``{"head": "sha256:...|null", "skills": {...}}``; a default on missing/corrupt. A legacy + ``.sync_manifest`` is migrated to ``.sync_state`` on read so no head record is lost.""" path = _sync_state_path() if path.exists(): return _load_state_file(path) or dict(_EMPTY_STATE) legacy = _skills_dir() / ".sync_manifest" - if legacy.exists(): - data = _load_state_file(legacy, "legacy sync state migrate") - if data is not None: - write_sync_state(data) - with suppress(OSError): - legacy.unlink() - return data - return dict(_EMPTY_STATE) + data = _load_state_file(legacy, "legacy sync state migrate") if legacy.exists() else None + if data is not None: + write_sync_state(data) + with suppress(OSError): + legacy.unlink() + return data if data is not None else dict(_EMPTY_STATE) def write_sync_state(data: Dict[str, Any]) -> None: """Write the local sync state atomically. Best-effort.""" try: from tools.skill_usage import _atomic_write - - _atomic_write( - _sync_state_path(), ".sync_state_", - lambda f: json.dump(data, f, indent=2, sort_keys=True, ensure_ascii=False)) + _atomic_write(_sync_state_path(), ".sync_state_", + lambda f: json.dump(data, f, indent=2, sort_keys=True, ensure_ascii=False)) except Exception as e: logger.debug("skills_sync_client: sync state write failed: %s", e) def _record_head(state: Dict[str, Any], head: str, root: str) -> None: - state["head"] = head - state["root"] = root + state.update(head=head, root=root) write_sync_state(state) -# Profile snapshot -- the root tree mirrors each synced skill's relative path under -# ~/.hermes/skills/ (category dirs become intermediate trees). +# Profile snapshot -- the root tree mirrors each skill's relative path (categories = intermediate trees). -def snapshot_profile( - skill_names: List[str], *, max_object_bytes: int = DEFAULT_MAX_OBJECT_BYTES -) -> Tuple[ObjectSet, str, Dict[str, str]]: - """Build all objects for *skill_names* + the profile-root tree -> ``(objects, root_tree_hash, - {skill_name: tree_hash})``. Skills whose blobs exceed *max_object_bytes* are skipped (logged). - The root also carries the ``sync-manifest`` blob listing every included skill as enabled.""" +def snapshot_profile(skill_names: List[str], *, max_object_bytes: int = DEFAULT_MAX_OBJECT_BYTES, + ) -> Tuple[ObjectSet, str, Dict[str, str]]: + """All objects for *skill_names* + profile root -> ``(objects, root_hash, {name: tree_hash})``. + Oversized skills are skipped (logged); the root carries a ``sync-manifest`` blob of included skills.""" from tools.skill_usage import _find_skill_dir - objects = ObjectSet() skill_tree_map: Dict[str, str] = {} root: Dict[str, Any] = {} @@ -405,9 +358,8 @@ def user_head_ref(owner: str) -> str: return f"refs/user/{owner}/HEAD" -def _personal_client( - identity: Optional[Dict[str, Any]], client: Optional[SyncClient] -) -> Tuple[Dict[str, Any], Optional[SyncClient]]: +def _personal_client(identity: Optional[Dict[str, Any]], client: Optional[SyncClient], + ) -> Tuple[Dict[str, Any], Optional[SyncClient]]: """Identity + client for a personal sync op; ``client`` is None when no base URL is configured.""" identity = identity if identity is not None else resolve_identity() if client is None: @@ -419,15 +371,10 @@ def _personal_client( _NO_BASE_URL = {"ok": False, "reason": "no sync base url configured", "noop": True} -def push_skills( - client: Optional[SyncClient] = None, - *, - skill_names: Optional[List[str]] = None, - identity: Optional[Dict[str, Any]] = None, - message: str = "hermes skill sync") -> Dict[str, Any]: - """Push opted-in skills to ``refs/user//HEAD``: upload new objects, CAS HEAD. A 409 with - an actual head -> three-way merge + one retry; a 409 against a NON-EXISTENT ref (stale local - head, e.g. from another plane) -> redo the CAS as a create. Never raises for inert/no-op cases.""" +def push_skills(client: Optional[SyncClient] = None, *, skill_names: Optional[List[str]] = None, + identity: Optional[Dict[str, Any]] = None, message: str = "hermes skill sync") -> Dict[str, Any]: + """Push opted-in skills to ``refs/user//HEAD`` (upload objects, CAS HEAD). 409 with an actual + head -> three-way merge + one retry; 409 on a NON-EXISTENT ref (stale local head) -> CAS as a create.""" identity, client = _personal_client(identity, client) if client is None: return dict(_NO_BASE_URL) @@ -466,18 +413,11 @@ def push_skills( return result -def _resolve_push_conflict( - client: SyncClient, - identity: Dict[str, Any], - actual_head: str, - our_root: str, - our_commit: str, - objects: ObjectSet, - message: str, - base_head: Optional[str]) -> Dict[str, Any]: - """Three-way merge per skill against the base we forked from (see merge_skill): each side - changing a DIFFERENT skill -> merge commit (2 parents) + CAS retry; both changing the SAME - skill -> TRUE OVERLAP -> written to refs/user//conflict/ for out-of-band resolution.""" +def _resolve_push_conflict(client: SyncClient, identity: Dict[str, Any], actual_head: str, our_root: str, + our_commit: str, objects: ObjectSet, message: str, base_head: Optional[str], + ) -> Dict[str, Any]: + """Per-skill three-way merge against the forked base (merge_skill): different skills changed -> + merge commit + CAS retry; SAME skill -> OVERLAP -> refs/user//conflict/ for out-of-band.""" owner = identity["owner"] ours_trees = skill_trees_of_root(client, our_root) theirs_trees = skill_trees_of_root(client, root_tree_of_commit(client, actual_head)) @@ -535,12 +475,10 @@ def _next_conflict_index(client: SyncClient, owner: str) -> int: return (max(used) + 1) if used else 1 -def pull_skills( - client: Optional[SyncClient] = None, *, identity: Optional[Dict[str, Any]] = None -) -> Dict[str, Any]: - """Pull the owner's HEAD and materialize opted-in skills under ~/.hermes/skills/ if it advanced - past our recorded head. Opt-in intent is first adopted FROM the plane manifest, then only - opted-in paths are written so a pull never resurrects a skill the user hasn't chosen.""" +def pull_skills(client: Optional[SyncClient] = None, *, identity: Optional[Dict[str, Any]] = None, + ) -> Dict[str, Any]: + """Pull the owner's HEAD (if it advanced) and materialize opted-in skills. Opt-ins are first + adopted FROM the plane manifest; only opted-in paths are written (never resurrects a skill).""" identity, client = _personal_client(identity, client) if client is None: return dict(_NO_BASE_URL) @@ -565,12 +503,10 @@ def pull_skills( return {"ok": True, "head": head, "updated": sorted(updated), "opt_in_adopted": sorted(adopted)} -# Gated public entrypoints (gate-and-swallow, like the curator's maybe_run_curator): best-effort, -# never raise, return a result dict or None. +# Gated public entrypoints (gate-and-swallow, like maybe_run_curator): never raise; dict or None. def _gate_and_swallow(op: str, run: Callable[[Dict[str, Any]], Optional[Dict[str, Any]]]): - """Run *run(identity)* only if every background-sync gate holds (Nous admin, feature on, base - URL); None when inert or on any error.""" + """Run *run(identity)* only if all gates hold (Nous admin, feature on, base URL); None if inert/error.""" try: identity = resolve_identity() if not identity.get("nous_admin") or not sync_feature_enabled() or not resolve_sync_base_url(): @@ -583,10 +519,8 @@ def _gate_and_swallow(op: str, run: Callable[[Dict[str, Any]], Optional[Dict[str def maybe_push_skills(*, message: str = "hermes skill sync") -> Optional[Dict[str, Any]]: """Best-effort push (debounced skill_manage hook). Never raises.""" - return _gate_and_swallow( - "maybe_push_skills", - lambda identity: push_skills(identity=identity, message=message) if list_synced_skill_names() else None, - ) + return _gate_and_swallow("maybe_push_skills", lambda identity: push_skills( + identity=identity, message=message) if list_synced_skill_names() else None) def maybe_pull_skills() -> Optional[Dict[str, Any]]: @@ -595,8 +529,7 @@ def maybe_pull_skills() -> Optional[Dict[str, Any]]: def sync_status() -> Dict[str, Any]: - """Status snapshot for ``hermes sync status``. Never raises. ``org_available`` False means the - account isn't in a shared organisation (org workflow does not apply), not that anything is broken.""" + """Snapshot for ``hermes sync status``; never raises. ``org_available`` False = not in a shared org.""" status: Dict[str, Any] = { "nous_admin": False, "logged_in": False, "feature_enabled": sync_feature_enabled(), "default_opt_in": sync_default_opt_in(), "base_url": resolve_sync_base_url(), @@ -627,11 +560,8 @@ def sync_status() -> Dict[str, Any]: return status -# Org-shared skills live in skills_sync_client_org; imported last because that module reads -# this module's state lazily. +# Imported last: skills_sync_client_org reads this module's state lazily. from tools.skills_sync_client_org import ( # noqa: E402,F401 (re-exports) - ORG_DIR_NAME, _ORG_CAS_MAX_ATTEMPTS, _clear_active_org_marker, _org_baseline_path, - _read_org_baseline, _read_org_head, _skill_dir_fingerprint, _write_active_org_marker, - _write_org_baseline, _write_org_provenance, list_locally_modified_org_skills, - list_org_skill_names, maybe_pull_org_skills, org_head_ref, org_skill_is_locally_modified, - propose_skill, pull_org_skills, resolve_org_identity) + ORG_DIR_NAME, _skill_dir_fingerprint, _write_org_baseline, list_locally_modified_org_skills, + list_org_skill_names, maybe_pull_org_skills, org_skill_is_locally_modified, propose_skill, + pull_org_skills, resolve_org_identity) diff --git a/tools/skills_sync_client_org.py b/tools/skills_sync_client_org.py index 66b8e15590..a8369f9721 100644 --- a/tools/skills_sync_client_org.py +++ b/tools/skills_sync_client_org.py @@ -1,13 +1,10 @@ """Org-shared skills: org pull + propose (``~/.hermes/skills/_org//``). - -Org skills live under a DISTINCT local namespace (read-only to the runtime; a local edit is a -personal fork until proposed). The canonical set is ``refs/org//HEAD`` -- the SAME -object model as personal sync. PERSONAL-ORG GATE: NAS stamps ``org_role`` ONLY for multi-member -orgs; no claim => pull/propose raise SyncInertError and personal sync is untouched. -``propose_skill`` must stay non-interactive (automation will drive it). Module state -(``_skills_dir``, ``_org_dir``, base URL, device id) stays in ``tools.skills_sync_client`` and -is read lazily so tests can monkeypatch it. -""" +Org skills live in a DISTINCT local namespace (read-only to the runtime; a local edit is a personal +fork until proposed); the canonical set is ``refs/org//HEAD`` with the SAME object model. +PERSONAL-ORG GATE: NAS stamps ``org_role`` ONLY for multi-member orgs; no claim => pull/propose +raise SyncInertError and personal sync is untouched. ``propose_skill`` must stay non-interactive. +Module state (``_skills_dir``, ``_org_dir``, base URL, device id) stays in ``tools.skills_sync_client`` +and is read lazily so tests can monkeypatch it.""" from __future__ import annotations @@ -34,7 +31,6 @@ _ORG_CAS_MAX_ATTEMPTS = 5 def _ssc(): from tools import skills_sync_client - return skills_sync_client @@ -43,20 +39,17 @@ def org_head_ref(org_id: str) -> str: def resolve_org_identity() -> Dict[str, Any]: - """``resolve_identity()`` extended with ``org_id`` + ``org_role``. Raises SyncInertError when - the token carries no ``org_role`` claim (personal org / old issuer): unavailable, NOT an error.""" + """``resolve_identity()`` + ``org_id``/``org_role``; SyncInertError without an ``org_role`` claim + (personal org / old issuer): org sync unavailable, NOT an error.""" from tools.skills_sync_client import SyncInertError, resolve_identity - identity = resolve_identity() claims = identity.get("claims") or {} - org_id = claims.get("org_id") - org_role = claims.get("org_role") + org_id, org_role = claims.get("org_id"), claims.get("org_role") if not org_id: raise SyncInertError("no organisation associated with this account") if not isinstance(org_role, str) or not org_role: raise SyncInertError("this account isn't a member of a shared organisation") - identity["org_id"] = str(org_id) - identity["org_role"] = org_role + identity.update(org_id=str(org_id), org_role=org_role) return identity @@ -99,8 +92,7 @@ def _write_sidecar(what: str, path_fn: Callable[[], Path], text: str) -> None: def _skill_dir_fingerprint(path: Path) -> str: - """Stable content hash of a materialized skill dir (sorted relative path + bytes, - independent of filesystem order and mtimes). "" on read failure.""" + """Content hash of a skill dir (sorted relative path + bytes; mtime-independent). "" on read failure.""" h = hashlib.sha256() try: for f in sorted(p for p in path.rglob("*") if p.is_file()): @@ -117,28 +109,25 @@ def _skill_dir_fingerprint(path: Path) -> str: def _sidecar_path(org_id: Optional[str], const: str) -> Path: """``/>`` (org-level when org_id is None).""" import agent.skill_utils as sku - return (_mirror_root(org_id) if org_id else _ssc()._org_dir()) / getattr(sku, const) -def _org_baseline_path(org_id: str) -> Path: - """Sidecar recording the upstream fingerprint of each mirrored skill.""" - return _sidecar_path(org_id, "ORG_BASELINE_FILE") - - def _read_org_baseline(org_id: str) -> Dict[str, Any]: + """The baseline sidecar: upstream fingerprint + tree of each mirrored skill ({} if absent).""" try: - return json.loads(_org_baseline_path(org_id).read_text(encoding="utf-8")) + return json.loads(_sidecar_path(org_id, "ORG_BASELINE_FILE").read_text(encoding="utf-8")) except Exception: return {} def _write_org_baseline(org_id: str, baseline: Dict[str, Any]) -> None: - _write_sidecar("baseline", lambda: _org_baseline_path(org_id), json.dumps(baseline, indent=2, sort_keys=True)) + _write_sidecar("baseline", lambda: _sidecar_path(org_id, "ORG_BASELINE_FILE"), + json.dumps(baseline, indent=2, sort_keys=True)) def _write_org_provenance(org_id: str, data: Dict[str, Any]) -> None: - _write_sidecar("org provenance", lambda: _sidecar_path(org_id, "ORG_PROVENANCE_FILE"), json.dumps(data, indent=2)) + _write_sidecar("org provenance", lambda: _sidecar_path(org_id, "ORG_PROVENANCE_FILE"), + json.dumps(data, indent=2)) def _write_active_org_marker(org_id: str) -> None: @@ -152,16 +141,14 @@ def _clear_active_org_marker() -> None: marker = _sidecar_path(None, "ORG_ACTIVE_MARKER") if marker.exists(): marker.unlink() - logger.info( - "skills_sync_client: cleared active-org marker " - "(token has no org workflow); org skills no longer resolve") + logger.info("skills_sync_client: cleared active-org marker " + "(token has no org workflow); org skills no longer resolve") except Exception as e: logger.debug("skills_sync_client: marker clear failed: %s", e) def org_skill_is_locally_modified(skill_rel_path: str, org_id: str) -> bool: - """True when the local copy of an org skill differs from what upstream sent. No recorded - baseline (pre-existing mirror) => unmodified; the next pull records one.""" + """Local copy differs from upstream's fingerprint. No baseline (pre-existing mirror) => unmodified.""" dest = _mirror_root(org_id) / PurePosixPath(skill_rel_path) if not dest.is_dir(): return False @@ -174,7 +161,6 @@ def list_locally_modified_org_skills(org_id: Optional[str] = None) -> List[str]: """Org skills with local edits that upstream has not seen.""" try: from agent.skill_utils import read_active_org_id - org_id = org_id or read_active_org_id(_ssc()._skills_dir()) if not org_id: return [] @@ -189,7 +175,6 @@ def list_org_skill_names() -> List[str]: names: List[str] = [] try: from agent.skill_utils import read_active_org_id - org_id = read_active_org_id(_ssc()._skills_dir()) root = _mirror_root(org_id) if org_id else None if root and root.is_dir(): @@ -204,12 +189,10 @@ def list_org_skill_names() -> List[str]: # Pull / propose -def pull_org_skills( - client: Optional[SyncClient] = None, *, identity: Optional[Dict[str, Any]] = None -) -> Dict[str, Any]: - """Pull the org canonical set into the local mirror (fast-forward only; no client merge on - the org path). A mirrored skill with LOCAL edits is never clobbered: skipped, and reported - in ``conflicted`` when upstream also moved; the member's change of record is ``propose_skill``. +def pull_org_skills(client: Optional[SyncClient] = None, *, identity: Optional[Dict[str, Any]] = None, + ) -> Dict[str, Any]: + """Pull the org canonical set into the mirror (fast-forward only, no client merge). A skill with + LOCAL edits is never clobbered: skipped, and in ``conflicted`` when upstream also moved. Returns ``{ok, org_id, head, updated, conflicted}``.""" identity = identity or resolve_org_identity() if "org_id" not in identity: @@ -218,8 +201,8 @@ def pull_org_skills( org_id = identity["org_id"] head = _read_org_head(client, org_id) - # Token-gated marker: written HERE because this runs only after the token's org_id + org_role - # were verified. A stale mirror from a previous org stops resolving on a pull under another org. + # Marker written HERE: only after the token's org_id + org_role were verified, so a stale mirror + # from a previous org stops resolving on a pull under another org. _write_active_org_marker(org_id) if not head: return {"ok": True, "org_id": org_id, "head": None, "updated": []} @@ -254,28 +237,18 @@ def pull_org_skills( }) _write_org_baseline(org_id, baseline) if conflicted: - logger.warning( - "skills_sync_client: %d org skill(s) have local edits AND upstream " - "changes; left untouched: %s", len(conflicted), ", ".join(conflicted)) + logger.warning("skills_sync_client: %d org skill(s) have local edits AND upstream " + "changes; left untouched: %s", len(conflicted), ", ".join(conflicted)) return {"ok": True, "org_id": org_id, "head": head, "updated": updated, "conflicted": conflicted} -def propose_skill( - skill_name: str, - client: Optional[SyncClient] = None, - *, - identity: Optional[Dict[str, Any]] = None, - message: Optional[str] = None) -> Dict[str, Any]: - """Propose a local (personal) skill's content to the org canonical set. - - Snapshots the skill dir as an org-scoped commit splicing that ONE skill subtree into the - current org HEAD (proposals are per-skill deltas, never a wholesale replace), uploads with - ``?scope=org``, then CAS-es the org HEAD: ADMIN/OWNER -> server merges -> ``{ok, merged: - True}``; MEMBER -> 202 proposal -> ``{ok, proposal_pending: True, proposal_id, ref}``, never - presented as live. If HEAD moves between read and CAS the skill is re-spliced onto the NEW - head (not replayed from the old root, which would drop the other member's skill) up to - ``_ORG_CAS_MAX_ATTEMPTS`` times. - """ +def propose_skill(skill_name: str, client: Optional[SyncClient] = None, *, + identity: Optional[Dict[str, Any]] = None, message: Optional[str] = None) -> Dict[str, Any]: + """Propose a local skill to the org canonical set: an org-scoped commit splicing that ONE skill + subtree into the current org HEAD (per-skill deltas, never a wholesale replace), uploaded with + ``?scope=org``, then CAS. ADMIN/OWNER -> ``{ok, merged: True}``; MEMBER -> 202 -> ``{ok, + proposal_pending: True, proposal_id, ref}`` (never presented as live). If HEAD moves before the + CAS the skill is re-spliced onto the NEW head (replaying the old root would drop others' skills).""" ssc = _ssc() identity, client, caps = _org_client(identity, client) org_id = identity["org_id"] @@ -292,9 +265,8 @@ def propose_skill( skill_tree = build_tree(skill_dir, objects, max_object_bytes=max_bytes) for attempt in range(1, _ORG_CAS_MAX_ATTEMPTS + 1): base_head = _read_org_head(client, org_id) - skill_map = ( - skill_trees_of_root(client, root_tree_of_commit(client, base_head, org_scope=True), org_scope=True) - if base_head else {}) + skill_map = {} if not base_head else skill_trees_of_root( + client, root_tree_of_commit(client, base_head, org_scope=True), org_scope=True) skill_map[str(rel)] = skill_tree root_hash = assemble_root_from_skill_trees(skill_map, objects) commit_hash = build_commit( @@ -307,27 +279,23 @@ def propose_skill( break except SyncConflict as conflict: if attempt >= _ORG_CAS_MAX_ATTEMPTS: - raise SyncError( - "the organisation's skills changed while this was being " - f"proposed, and {attempt} attempts to catch up all lost " - "the race — run the command again", - status=409, - ) from conflict - logger.debug("propose_skill: org HEAD moved (actual=%r), re-splicing (attempt %d)", conflict.actual, attempt) + raise SyncError("the organisation's skills changed while this was being " + f"proposed, and {attempt} attempts to catch up all lost " + "the race — run the command again", status=409) from conflict + logger.debug("propose_skill: org HEAD moved (actual=%r), re-splicing (attempt %d)", + conflict.actual, attempt) if result.get("proposal_pending"): - return { - "ok": True, "proposal_pending": True, "proposal_id": result.get("proposal_id"), - "ref": result.get("ref"), "commit": commit_hash, "org_id": org_id} - return {"ok": True, "merged": True, "head": result.get("hash", commit_hash), "commit": commit_hash, "org_id": org_id} + return {"ok": True, "proposal_pending": True, "proposal_id": result.get("proposal_id"), + "ref": result.get("ref"), "commit": commit_hash, "org_id": org_id} + return {"ok": True, "merged": True, "head": result.get("hash", commit_hash), + "commit": commit_hash, "org_id": org_id} def maybe_pull_org_skills() -> Optional[Dict[str, Any]]: - """Best-effort org pull if all gates hold (logged in, org_role claim, feature enabled, base - URL). Never raises; None when inert. Marker hygiene: when the token VERIFIABLY lacks the org - claim (personal org / left the org) the active-org marker is cleared so mirrored org skills - stop resolving; when identity cannot be resolved at all (offline, logged out) the marker is - left alone -- offline grace keeps pulled org skills working.""" + """Best-effort org pull if all gates hold; never raises, None when inert. Marker hygiene: a token + that VERIFIABLY lacks the org claim clears the active-org marker (org skills stop resolving); an + unresolvable identity (offline, logged out) leaves it alone so pulled org skills keep working.""" ssc = _ssc() try: identity = resolve_org_identity() diff --git a/tools/skills_sync_client_wire.py b/tools/skills_sync_client_wire.py index 73a5631777..3e1d6100d8 100644 --- a/tools/skills_sync_client_wire.py +++ b/tools/skills_sync_client_wire.py @@ -51,9 +51,8 @@ def wire_address(data: bytes) -> str: def canonical_json_bytes(obj: Dict[str, Any]) -> bytes: - """Canonical JSON for tree/commit hashing: UTF-8, sorted keys, no whitespace, no trailing - newline. Arrays must already be in contract order (tree entries by name, commit parents by - significance). Client and server MUST agree byte-for-byte or a push fails ``422 hash_mismatch``.""" + """Canonical JSON for hashing: UTF-8, sorted keys, no whitespace/trailing newline; arrays already in + contract order. Client and server MUST agree byte-for-byte or a push fails ``422 hash_mismatch``.""" return json.dumps(obj, sort_keys=True, separators=(",", ":"), ensure_ascii=False).encode("utf-8") @@ -66,9 +65,8 @@ def build_sync_manifest_bytes(skills: Dict[str, bool]) -> bytes: def parse_sync_manifest(data: bytes) -> Optional[Dict[str, bool]]: - """Parse ``sync-manifest`` bytes into ``{name: enabled}``, or None if malformed. - Strict (mirrors gateway-gateway ``parseSyncManifest``): unknown type, version != 1, non-list - skills or a malformed entry all reject -- a bad manifest must not read as "nothing opted in".""" + """``{name: enabled}`` from ``sync-manifest`` bytes, or None if malformed. Strict (mirrors gateway + ``parseSyncManifest``): a bad manifest must not read as "nothing opted in".""" try: value = json.loads(data.decode("utf-8")) except Exception: @@ -80,9 +78,7 @@ def parse_sync_manifest(data: bytes) -> Optional[Dict[str, bool]]: return None out: Dict[str, bool] = {} for raw in value["skills"]: - if not isinstance(raw, dict): - return None - name, enabled = raw.get("name"), raw.get("enabled") + name, enabled = (raw.get("name"), raw.get("enabled")) if isinstance(raw, dict) else (None, None) if not isinstance(name, str) or not name or not isinstance(enabled, bool): return None out[name] = enabled @@ -125,9 +121,8 @@ def _file_mode(path: Path) -> str: def build_tree(dir_path: Path, objects: ObjectSet, *, max_object_bytes: int) -> str: - """Recursively build objects for *dir_path*; return the tree address. Files -> blobs, subdirs - -> nested trees; symlinks/special files are skipped (contract: no symlinks). A blob over - *max_object_bytes* raises ValueError so the caller can surface / skip the artifact (server -> 413).""" + """Build objects for *dir_path* recursively; return the tree address. Symlinks/special files are + skipped (contract). A blob over *max_object_bytes* raises ValueError (server would 413).""" entries: List[Dict[str, str]] = [] for child in sorted(dir_path.iterdir(), key=lambda p: p.name): if child.is_symlink(): @@ -143,11 +138,9 @@ def build_tree(dir_path: Path, objects: ObjectSet, *, max_object_bytes: int) -> return _add_tree(entries, objects) -def build_commit( - tree_hash: str, parents: List[str], *, owner: str, device: str, message: str, - objects: ObjectSet, ts: Optional[str] = None) -> str: - """Build a commit object and return its address. ``parents``: 0 for the first commit, 1 for - an edit, 2 for a merge (parents[0] = base fast-forwarded from, parents[1] = other head).""" +def build_commit(tree_hash: str, parents: List[str], *, owner: str, device: str, message: str, + objects: ObjectSet, ts: Optional[str] = None) -> str: + """Commit object address. ``parents``: 0 first commit, 1 edit, 2 merge (base first, other head second).""" return objects.add(KIND_COMMIT, canonical_json_bytes({ "type": KIND_COMMIT, "tree": tree_hash, "parents": list(parents), "author": {"owner": owner, "device": device}, @@ -156,9 +149,8 @@ def build_commit( def build_root_tree(node: Dict[str, Any], objects: ObjectSet, *, manifest_hash: Optional[str] = None) -> str: - """Canonicalize a nested ``{name: {"__tree__": hash} | subdict}`` root into trees. - ``manifest_hash`` (top level only) adds the root ``sync-manifest`` BLOB entry; it cannot - collide with a skill dir because skill entries are trees.""" + """Canonicalize a nested ``{name: {"__tree__": hash} | subdict}`` root into trees. ``manifest_hash`` + adds the root ``sync-manifest`` BLOB entry (cannot collide with a skill dir: those are trees).""" entries: List[Dict[str, str]] = [] for name, child in node.items(): if isinstance(child, dict) and "__tree__" in child and len(child) == 1: @@ -179,8 +171,8 @@ def nest_skill_tree(root: Dict[str, Any], rel_parts: Tuple[str, ...], tree_hash: def assemble_root_from_skill_trees(skill_trees: Dict[str, str], objects: ObjectSet) -> str: - """Profile-root tree from ``{posix_rel_path: tree_hash}``. The skill trees are assumed already - durable (either side of a merge / the org HEAD); only the new intermediate/root trees are added.""" + """Profile-root tree from ``{posix_rel_path: tree_hash}``; skill trees are assumed durable, only + the new intermediate/root trees are added.""" root: Dict[str, Any] = {} for path, tree_hash in skill_trees.items(): nest_skill_tree(root, PurePosixPath(path).parts, tree_hash) @@ -198,9 +190,8 @@ class SyncError(RuntimeError): class SyncConflict(RuntimeError): - """CAS lost (409). NOT a rejection -- pushed objects are already durable. ``actual`` is the - current head to merge against, or None when the ref does not exist server-side (the server - sends ""): "retry as a create"; it must never be fetched as an object -- normalized here.""" + """CAS lost (409); NOT a rejection, pushed objects are durable. ``actual`` = head to merge against, + or None when the ref does not exist (server sends ""): retry as a create, never fetch it.""" def __init__(self, actual: Optional[str]): self.actual: Optional[str] = actual or None @@ -223,19 +214,15 @@ def _body(r) -> Dict[str, Any]: class SyncClient: - """Sync client bound to a base URL + Nous bearer. - - Org refs/objects live behind SEPARATE ``org/`` routes, not a prefix filter: the personal - routes are hard-scoped to the token's owner and would silently answer an org query with - personal data. Callers reading org content MUST pass ``org_scope=True`` on every hop. - """ + """Sync client bound to a base URL + Nous bearer. Org refs/objects live behind SEPARATE ``org/`` + routes: the personal routes are hard-scoped to the token's owner and would silently answer an + org query with personal data, so org readers MUST pass ``org_scope=True`` on every hop.""" def __init__(self, base_url: str, api_key: str, *, timeout: float = 30.0): self.base = base_url.rstrip("/") self.api_key = api_key self.timeout = timeout import requests # core dependency - self._session = requests.Session() self._session.headers["Authorization"] = f"Bearer {api_key}" @@ -243,8 +230,8 @@ class SyncClient: return f"{self.base}/v1/sync/{path.lstrip('/')}" def _request(self, method: str, path: str, op: str, *, ok=(200,), errors: Optional[Dict[int, Any]] = None, **kw): - """One wire call. Raises SyncError unless the status is in *ok*; *errors* maps specific - statuses to a message (str or ``fn(response)``), anything else gets ``" failed: "``.""" + """One wire call; SyncError unless the status is in *ok*. *errors* maps a status to a message + (str or ``fn(response)``); anything else gets ``" failed: "``.""" r = self._session.request(method, self._url(path), timeout=self.timeout, **kw) if r.status_code in ok: return r @@ -285,11 +272,9 @@ class SyncClient: return self._get_json_of_kind(tree_hash, KIND_TREE, org_scope) def put_objects(self, objects: Dict[str, Tuple[str, bytes]], *, org_scope: bool = False) -> Dict[str, Any]: - """POST objects -- batch multipart upload: field name = the claimed ``sha256:``, - ``filename`` = object type, body = raw bytes (the contract forbids base64-in-JSON). The - server recomputes every hash and rejects the whole batch with 422 on mismatch; known - hashes are idempotent no-ops. ``org_scope`` adds ``?scope=org`` so objects land - org-readable (required before an org CAS/propose).""" + """POST objects as multipart (field = claimed ``sha256:``, filename = kind, body = raw + bytes; no base64-in-JSON). The server rehashes and 422s the whole batch on mismatch; known + hashes are no-ops. ``org_scope`` adds ``?scope=org`` (required before an org CAS/propose).""" files = [(h, (kind, data, "application/octet-stream")) for h, (kind, data) in objects.items()] r = self._request( "POST", "objects", "put_objects", ok=(200, 201), files=files, @@ -298,9 +283,8 @@ class SyncClient: return _body(r) def cas_ref(self, name: str, from_hash: Optional[str], to_hash: str) -> Dict[str, Any]: - """POST refs/:name -- atomic compare-and-swap. Raises SyncConflict on 409. A non-admin - member's CAS on an org HEAD is converted server-side to a proposal (202) and surfaced as - ``{"proposal_pending": True, ...}``: SUCCESS-shaped but never to be presented as live/merged.""" + """POST refs/:name -- atomic CAS; SyncConflict on 409. A member's CAS on an org HEAD becomes a + proposal (202) -> ``{"proposal_pending": True, ...}``: success-shaped, never present as live.""" r = self._request( "POST", f"refs/{name}", "cas_ref", ok=(200, 202, 409), json={"from": from_hash, "to": to_hash}, errors={403: "forbidden (403) -- owner/permission"}) @@ -324,8 +308,7 @@ def root_tree_of_commit(client: SyncClient, commit_hash: str, *, org_scope: bool def skill_trees_of_root(client: SyncClient, root_tree_hash: str, *, org_scope: bool = False) -> Dict[str, str]: - """Flatten a profile-root tree into ``{posix_rel_path: skill_tree_hash}``: a skill tree is any - subtree containing a ``SKILL.md`` blob, keyed by its path so category nesting is preserved.""" + """``{posix_rel_path: skill_tree_hash}`` for every subtree containing a ``SKILL.md`` blob.""" result: Dict[str, str] = {} def _walk(tree_hash: str, prefix: str) -> None: @@ -342,8 +325,7 @@ def skill_trees_of_root(client: SyncClient, root_tree_hash: str, *, org_scope: b def read_manifest_of_root(client: SyncClient, root_tree_hash: str) -> Optional[Dict[str, bool]]: - """``{name: enabled}`` from the root-level ``sync-manifest`` blob, or None if absent/malformed. - This is how a device learns another device's opt-ins.""" + """``{name: enabled}`` from the root ``sync-manifest`` blob (how a device learns another's opt-ins).""" try: tree = client.get_tree_json(root_tree_hash) except Exception as e: @@ -361,9 +343,8 @@ def read_manifest_of_root(client: SyncClient, root_tree_hash: str) -> Optional[D def materialize_tree(client: SyncClient, tree_hash: str, dest: Path, *, org_scope: bool = False) -> None: - """Write the tree at *tree_hash* into *dest* (created if needed): blobs -> files (+x restored - for ``exec``), trees -> subdirectories. Does NOT delete files absent from the tree (caller - decides). Refuses path traversal.""" + """Write the tree into *dest*: blobs -> files (+x for ``exec``), trees -> subdirs. Does NOT delete + files absent from the tree (caller decides). Refuses path traversal.""" dest.mkdir(parents=True, exist_ok=True) for entry in client.get_tree_json(tree_hash, org_scope=org_scope).get("entries", []): name = entry.get("name", "") @@ -383,9 +364,8 @@ def materialize_tree(client: SyncClient, tree_hash: str, dest: Path, *, org_scop def merge_skill(base: Optional[str], ours: Optional[str], theirs: Optional[str]) -> str: - """Three-way decision for one skill's tree hash: ``ours`` / ``theirs`` / ``either`` / ``overlap`` - / ``none``. A side "modified" the skill when its hash differs from the common base (same - semantics as skills_sync.py's origin/user/incoming block).""" + """Three-way decision: ``ours``/``theirs``/``either``/``overlap``/``none``. A side "modified" the + skill when its hash differs from the base (same semantics as skills_sync.py).""" if ours == theirs: return "either" if ours is not None else "none" if theirs == base: # only we moved diff --git a/tools/tirith_security.py b/tools/tirith_security.py index 02eb43b8f5..2f775d098b 100644 --- a/tools/tirith_security.py +++ b/tools/tirith_security.py @@ -22,6 +22,7 @@ import tempfile import threading import time import urllib.request +from contextlib import suppress from hermes_constants import get_hermes_home @@ -93,10 +94,8 @@ def _record_tirith_crash() -> None: _crash_count += 1 if _crash_count >= _CRASH_LIMIT: _circuit_open = True - logger.warning( - "tirith circuit breaker opened after %d consecutive failures; " - "disabling for the rest of the process", - _crash_count) + logger.warning("tirith circuit breaker opened after %d consecutive failures; " + "disabling for the rest of the process", _crash_count) def _warn_once(key: str, message: str, *args) -> None: @@ -115,10 +114,8 @@ def _reset_spawn_warning_state() -> None: def _cached_path() -> str | None: - """The path resolved on a previous call, or None if unresolved/failed.""" - if _resolved_path is None or _resolved_path is _INSTALL_FAILED: - return None - return _resolved_path + """The path resolved on a previous call, or None if unresolved (None) / failed (_INSTALL_FAILED).""" + return _resolved_path or None def _set_resolved(path: str) -> None: @@ -153,32 +150,26 @@ def _is_install_failed_on_disk() -> bool: """True if a recent install failure was persisted and is still non-retryable. A 'cosign_missing' marker is auto-cleared once cosign appears on PATH.""" reason = _read_failure_reason() - if reason is None: - return False if reason == "cosign_missing" and shutil.which("cosign"): _clear_install_failed() return False - return True + return reason is not None def _mark_install_failed(reason: str = ""): """Persist install failure to disk; ``reason`` is a short retryability tag.""" - try: + with suppress(OSError): p = _failure_marker_path() os.makedirs(os.path.dirname(p), exist_ok=True) with open(p, "w", encoding="utf-8") as f: f.write(reason) - except OSError: - pass def _clear_install_failed(): """Remove the failure marker and reset warn-once state after a successful install.""" _reset_spawn_warning_state() - try: + with suppress(OSError): os.unlink(_failure_marker_path()) - except OSError: - pass def _disk_marker_blocks_install() -> bool: @@ -291,8 +282,7 @@ def _verify_checksum(archive_path: str, checksums_path: str, archive_name: str) actual = sha.hexdigest() if actual != expected: logger.warning("Checksum mismatch: expected %s, got %s", expected, actual) - return False - return True + return actual == expected def _extract_tirith_binary(tar: tarfile.TarFile, dest_dir: str, log) -> tuple[str | None, str]: @@ -360,10 +350,8 @@ def _install_tirith(*, log_failures: bool = True) -> tuple[str | None, str]: try: shutil.copy(src, dest) except OSError: - try: + with suppress(OSError): os.unlink(dest) - except OSError: - pass return None, "cross_device_copy_failed" os.chmod(dest, os.stat(dest).st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) logger.info("tirith installed to %s (%s)", dest, "cosign + SHA-256" if cosign_verified else "SHA-256 only") @@ -487,8 +475,8 @@ def ensure_installed(*, log_failures: bool = True): if found or not may_install or _disk_marker_blocks_install(): return found if _install_thread is None or not _install_thread.is_alive(): - _install_thread = threading.Thread( - target=_background_install, kwargs={"log_failures": log_failures}, daemon=True) + _install_thread = threading.Thread(target=_background_install, daemon=True, + kwargs={"log_failures": log_failures}) _install_thread.start() return None # not available yet; commands fail-open until ready @@ -538,7 +526,8 @@ def check_command_security(command: str) -> dict: except OSError as exc: # FileNotFoundError / PermissionError / exec format error: dedupe by (class, errno) # so each failure mode surfaces once, not per command. - _warn_once(f"tirith_spawn_failed:{type(exc).__name__}:{getattr(exc, 'errno', '')}", "tirith spawn failed: %s", exc) + spawn_key = f"tirith_spawn_failed:{type(exc).__name__}:{getattr(exc, 'errno', '')}" + _warn_once(spawn_key, "tirith spawn failed: %s", exc) _record_tirith_crash() return _fail(fail_open, f"tirith unavailable: {exc}", f"tirith spawn failed (fail-closed): {exc}") except subprocess.TimeoutExpired: @@ -552,7 +541,8 @@ def check_command_security(command: str) -> dict: # Unknown exit code (includes signal-killed, e.g. -11): respect fail_open. logger.warning("tirith returned unexpected exit code %d", exit_code) _record_tirith_crash() - return _fail(fail_open, f"tirith exit code {exit_code} (fail-open)", f"tirith exit code {exit_code} (fail-closed)") + return _fail(fail_open, f"tirith exit code {exit_code} (fail-open)", + f"tirith exit code {exit_code} (fail-closed)") if action == "allow": _crash_count = 0 # successful execution resets the circuit breaker # JSON enriches findings/summary; a parse failure never changes the verdict.