ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 11 Packages Projects Releases Wiki Activity

fix: limit skill update change to unusable local installs

Browse Source
This commit is contained in:
Teknium
2026-09-07 03:27:02 -07:00
parent 6798a9b8a4
commit a3ad585fd9
5 changed files with 13 additions and 186 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hermes_cli/skills_hub.py
+2 -1
View File
@@ -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 <name>\n")
"directory is missing or replaced by a non-directory. For missing directories, "
"remove the stale entry with: hermes skills uninstall <name>\n")
def _has_local_edits(installed: dict) -> bool:
tests/tools/test_skills_hub.py
+6 -25
View File
@@ -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):
tests/tools/test_skills_update_budget.py
-112
View File
@@ -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
tools/skills_hub_install.py
+4 -47
View File
@@ -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:
website/docs/user-guide/features/skills.md
+1 -1
View File
@@ -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 <name>`; 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.