Merge branch 'simp/r3-34-E' into simp/integration3

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"}
 

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_<KEY> env -> config.yaml ``sync.<key>`` -> 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_<KEY> env -> config.yaml ``sync.<key>`` -> 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.<key>`` 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/<owner>/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/<owner>/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/<owner>/conflict/<n> 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/<owner>/conflict/<n> 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)

tools/skills_sync_client_org.py

@@ -1,13 +1,10 @@
 """Org-shared skills: org pull + propose (``~/.hermes/skills/_org/<org_id>/``).
-
-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/<org_id>/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/<org_id>/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:
     """``<mirror>/<agent.skill_utils.<const>>`` (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()

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 ``"<op> failed: <code>"``."""
+        """One wire call; SyncError unless the status is in *ok*. *errors* maps a status to a message
+        (str or ``fn(response)``); anything else gets ``"<op> failed: <code>"``."""
         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:<hex>``,
-        ``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:<hex>``, 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

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.