From a3ad585fd9db702a8fcfacaf8fe2d5d4994a11dd Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 03:27:02 -0700 Subject: [PATCH] fix: limit skill update change to unusable local installs --- hermes_cli/skills_hub.py | 3 +- tests/tools/test_skills_hub.py | 31 ++---- tests/tools/test_skills_update_budget.py | 112 --------------------- tools/skills_hub_install.py | 51 +--------- website/docs/user-guide/features/skills.md | 2 +- 5 files changed, 13 insertions(+), 186 deletions(-) delete mode 100644 tests/tools/test_skills_update_budget.py diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 186ad98aff..54adef6fdf 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -811,7 +811,8 @@ def do_check(name: Optional[str] = None, console: Optional[Console] = None) -> N orphaned = [entry.get("name", "") for entry in results if entry.get("status") == "orphaned"] if orphaned: c.print(f"[yellow]Orphaned:[/] {', '.join(orphaned)} — lock-file entries whose local " - "directory is gone. Remove them with: hermes skills uninstall \n") + "directory is missing or replaced by a non-directory. For missing directories, " + "remove the stale entry with: hermes skills uninstall \n") def _has_local_edits(installed: dict) -> bool: diff --git a/tests/tools/test_skills_hub.py b/tests/tools/test_skills_hub.py index 310005d1e6..ba4e28d691 100644 --- a/tests/tools/test_skills_hub.py +++ b/tests/tools/test_skills_hub.py @@ -553,14 +553,14 @@ class TestCheckForSkillUpdates: assert results[0]["name"] == "demo-skill" assert results[0]["status"] == "update_available" - @pytest.mark.parametrize("regular_file", [False, True]) - def test_orphaned_entry_reported_without_remote_fetch(self, tmp_path, monkeypatch, regular_file): + @pytest.mark.parametrize("path_kind", ["missing", "regular_file", "unsafe", "corrupt"]) + def test_unusable_entry_reported_without_remote_fetch(self, tmp_path, monkeypatch, path_kind): """A lock-file entry whose install directory no longer exists is reported ``orphaned`` without paying the remote fetch cost (#104291).""" import tools.skills_hub as hub skills_dir = tmp_path / "skills" skills_dir.mkdir() - if regular_file: + if path_kind == "regular_file": (skills_dir / "demo-skill").write_text("not an installed directory") monkeypatch.setattr(hub, "SKILLS_DIR", skills_dir) @@ -570,7 +570,7 @@ class TestCheckForSkillUpdates: "source": "github", "identifier": "owner/repo/demo-skill", "content_hash": "hash", - "install_path": "demo-skill", # never created on disk + "install_path": {"unsafe": "../outside", "corrupt": ["bad"]}.get(path_kind, "demo-skill"), }] source = MagicMock() @@ -579,30 +579,11 @@ class TestCheckForSkillUpdates: results = check_for_skill_updates(lock=lock, sources=[source]) assert len(results) == 1 - assert results[0]["status"] == "orphaned" + expected = "invalid_install" if path_kind in {"unsafe", "corrupt"} else "orphaned" + assert results[0]["status"] == expected assert "bundle" not in results[0] source.fetch.assert_not_called() - def test_hanging_fetch_is_abandoned_after_timeout(self): - """A fetch that outlives its wall-clock bound degrades to no bundle - quickly instead of stalling the whole update run (#104291).""" - from tools.skills_hub_install import _fetch_bundle_bounded - - class _HangingSource: - def source_id(self): - return "github" - - def fetch(self, identifier): - time.sleep(10) - raise AssertionError("fetch should have been abandoned") - - started = time.monotonic() - bundle = _fetch_bundle_bounded(_HangingSource(), "owner/repo/demo-skill", timeout=0.2) - elapsed = time.monotonic() - started - - assert bundle is None - assert elapsed < 5.0 - class TestCreateSourceRouter: def test_url_source_runs_before_github_source(self): diff --git a/tests/tools/test_skills_update_budget.py b/tests/tools/test_skills_update_budget.py deleted file mode 100644 index de139b583d..0000000000 --- a/tests/tools/test_skills_update_budget.py +++ /dev/null @@ -1,112 +0,0 @@ -"""Real HTTP controls for invalid installs and bounded update-check work.""" -import threading -import time -from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer -from urllib.request import urlopen - -import pytest - -from tools import skills_hub_install as install -from tools.skills_hub import HubLockFile - - -@pytest.fixture -def upstream(): - calls = [] - release = threading.Event() - finished = threading.Event() - - class Handler(BaseHTTPRequestHandler): - def log_message(self, format, *args): - pass - - def do_GET(self): - calls.append(self.path) - if self.path == "/blocked": - release.wait(10) - elif self.path == "/paced": - time.sleep(0.1) - self.send_response(200) - self.end_headers() - self.wfile.write(b"ok") - - server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) - thread = threading.Thread(target=server.serve_forever, daemon=True) - thread.start() - - class Source: - def source_id(self): - return "fixture" - - def fetch(self, identifier): - with urlopen(f"http://127.0.0.1:{server.server_port}/{identifier}", timeout=15) as response: - response.read() - finished.set() - return None - - try: - yield Source(), calls, release, finished - finally: - release.set() - server.shutdown() - server.server_close() - thread.join() - - -def record(lock, name, path, identifier="ok"): - lock.record_install(name, "fixture", identifier, "community", "safe", "old", path, ["SKILL.md"]) - - -def test_invalid_install_never_contacts_upstream(tmp_path, monkeypatch, upstream): - from tools import skills_hub as hub - root = tmp_path / "skills" - root.mkdir() - monkeypatch.setattr(hub, "SKILLS_DIR", root) - (root / "healthy").mkdir() - lock = HubLockFile() - record(lock, "invalid", "invalid", "invalid") - record(lock, "healthy", "healthy") - data = lock.load() - data["installed"]["invalid"]["install_path"] = "../outside" - lock.save(data) - source, calls, _, _ = upstream - rows = install.check_for_skill_updates(lock=lock, sources=[source]) - assert [row["status"] for row in rows] == ["invalid_install", "unavailable"] - assert calls == ["/ok"] - - -def test_update_budget_bounds_repeated_workers_and_whole_check(tmp_path, monkeypatch, upstream): - from tools import skills_hub as hub - root = tmp_path / "skills" - root.mkdir() - monkeypatch.setattr(hub, "SKILLS_DIR", root) - monkeypatch.setattr(install, "_FETCH_TIMEOUT_SECONDS", 0.2) - source, calls, release, finished = upstream - lock = HubLockFile() - for index in range(30): - name = f"skill-{index}" - (root / name).mkdir() - record(lock, name, name, "blocked") - try: - started = time.monotonic() - for _ in range(3): - rows = install.check_for_skill_updates(lock=lock, sources=[source]) - assert all(row["status"] == "unavailable" for row in rows) - assert time.monotonic() - started < 2 - assert calls == ["/blocked"] - finally: - release.set() - assert finished.wait(5) - # Wait for fetch's finally to release capacity, without assuming scheduler order. - deadline = time.monotonic() + 5 - while any(t.name == "skills-update-fetch" for t in threading.enumerate()): - assert time.monotonic() < deadline - time.sleep(0.01) - calls.clear() - for index in range(30): - name = f"skill-{index}" - record(lock, name, name, "paced") - started = time.monotonic() - install.check_for_skill_updates(lock=lock, sources=[source]) - assert time.monotonic() - started < 2 - assert 1 <= len(calls) <= 2 diff --git a/tools/skills_hub_install.py b/tools/skills_hub_install.py index e4f0d3cfb9..0272e51ac4 100644 --- a/tools/skills_hub_install.py +++ b/tools/skills_hub_install.py @@ -11,9 +11,6 @@ from __future__ import annotations import logging import hashlib import shutil -import threading -import time -from contextvars import copy_context from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple from agent.skill_utils import is_excluded_skill_path @@ -246,46 +243,6 @@ def bundle_content_hash(bundle: SkillBundle) -> str: _SOURCE_ID_ALIASES = {"skills.sh": "skills-sh"} -_FETCH_TIMEOUT_SECONDS = 30.0 -# Keep capacity occupied until fetch really exits, including after caller timeout. -# Repeated/concurrent checks must not accumulate abandoned credential contexts. -_FETCH_SLOT = threading.BoundedSemaphore(1) - - -def _fetch_bundle_bounded( - src: SkillSource, identifier: str, timeout: float = _FETCH_TIMEOUT_SECONDS, -) -> Optional[SkillBundle]: - """Bound caller waiting, not execution of the synchronous source adapter. - - One process-wide slot bounds lingering workers. While it is occupied, new - checks fail fast rather than enqueue work or retain more request contexts. - Python cannot cancel a running thread; the daemon may finish in background. - """ - deadline = time.monotonic() + timeout - if timeout <= 0 or not _FETCH_SLOT.acquire(blocking=False): - return None - box: Dict[str, Tuple[float, Optional[SkillBundle]]] = {} - - def _run() -> None: - try: - bundle = src.fetch(identifier) - box["result"] = (time.monotonic(), bundle) - except Exception: - logger.debug("Skill update fetch failed for %s", identifier, exc_info=True) - finally: - _FETCH_SLOT.release() - - try: - worker = threading.Thread(target=copy_context().run, args=(_run,), - name="skills-update-fetch", daemon=True) - worker.start() - except Exception: - _FETCH_SLOT.release() - raise - worker.join(max(0.0, deadline - time.monotonic())) - finished, bundle = box.get("result", (float("inf"), None)) - return bundle if finished <= deadline else None - def _source_matches(source: SkillSource, source_name: str) -> bool: return source.source_id() == _SOURCE_ID_ALIASES.get(source_name, source_name) @@ -311,9 +268,6 @@ def check_for_skill_updates( if sources is None: sources = create_source_router(auth=auth) - # A shared remote-wait budget, not N independent 30-second waits. Local - # lock/path/hash work is not cancellable and is outside this wait guarantee. - deadline = time.monotonic() + _FETCH_TIMEOUT_SECONDS results: List[dict] = [] for entry in installed: identifier, source_name = entry.get("identifier", ""), entry.get("source", "") @@ -333,7 +287,10 @@ def check_for_skill_updates( continue bundle = None for src in filter(lambda s: _source_matches(s, source_name), sources): - bundle = _fetch_bundle_bounded(src, identifier, timeout=deadline - time.monotonic()) + try: + bundle = src.fetch(identifier) + except Exception: + bundle = None if bundle: break if not bundle: diff --git a/website/docs/user-guide/features/skills.md b/website/docs/user-guide/features/skills.md index 99fb85d828..a380826956 100644 --- a/website/docs/user-guide/features/skills.md +++ b/website/docs/user-guide/features/skills.md @@ -883,7 +883,7 @@ This uses the stored source identifier plus the current upstream bundle content Checks skip network requests for missing or non-directory installs (`orphaned`) and unsafe or unresolvable recorded paths (`invalid_install`). Missing-directory entries can be removed with `hermes skills uninstall `; invalid paths require inspecting and repairing the active profile's `skills/.hub/lock.json` before retrying. No entries are removed automatically. -Remote fetch waiting shares a 30-second budget across the check, rather than spending 30 seconds per skill. Entries not fetched within that budget report `unavailable`, including healthy entries later in the list; checking a specific name avoids waiting behind earlier entries. This is a caller-wait limit, not cancellation or a strict whole-command deadline: local filesystem work is outside the guarantee. At most one update fetch runs in a process. A timed-out synchronous adapter may continue in its daemon thread with its request context; subsequent checks in that process report `unavailable` while it remains active, rather than accumulating more workers. Capacity returns when the adapter exits; a permanently stuck adapter requires restarting that process. +Valid installs continue to use their source adapter’s existing synchronous fetch and transport timeouts. There is no strict total deadline for an update check: an unreachable or slow source for an existing install can still delay later entries. Skills you have edited locally (the on-disk content no longer matches the hash recorded at install time) are **skipped** by `hermes skills update` so your changes are never silently overwritten. Pass `--force` to replace them with the upstream version anyway.