From dc1e369f89ad5171dc0e8513ba677ee09a53b5c2 Mon Sep 17 00:00:00 2001 From: BonyFish Date: Wed, 27 May 2026 20:57:32 +0800 Subject: [PATCH] fix: remove webui-side writer to avoid conflict with agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .usage.json file is owned by hermes-agent (tools/skill_usage.py). This change removes the webui-side increment logic to avoid: 1. File ownership conflict - both writing to same file 2. Schema mismatch - agent uses ISO strings, webui used floats 3. Concurrency issues - agent uses fcntl locks, webui had no locking 4. Double-counting - agent already increments counters server-side Changes: - api/skill_usage.py: keep only read_skill_usage(), remove increment functions - api/streaming.py: remove skill usage counter hook - api/routes.py: adapt response to pass through agent's format as-is, with defensive coercion for None values and metadata preservation - tests/test_skill_usage.py: remove increment tests (17→7 cases) --- api/routes.py | 30 ++++++-- api/skill_usage.py | 88 +--------------------- api/streaming.py | 7 -- tests/test_skill_usage.py | 149 +++----------------------------------- 4 files changed, 40 insertions(+), 234 deletions(-) diff --git a/api/routes.py b/api/routes.py index eef14ddd..f797cacb 100644 --- a/api/routes.py +++ b/api/routes.py @@ -4791,14 +4791,32 @@ def handle_get(handler, parsed) -> bool: if parsed.path == "/api/skills/usage": from api.skill_usage import read_skill_usage raw = read_skill_usage(_active_skills_dir()) - usage = { - k: {"use_count": v.get("use_count", 0), "view_count": v.get("view_count", 0), "patch_count": v.get("patch_count", 0)} - for k, v in raw.items() - } if isinstance(raw, dict) else {} + # Pass through agent's format as-is; defensive coercion for fields + usage = {} + if isinstance(raw, dict): + for k, v in raw.items(): + if not isinstance(v, dict): + usage[k] = {"use_count": 0, "view_count": 0, "patch_count": 0} + continue + usage[k] = { + "use_count": (int(v["use_count"]) if v.get("use_count") is not None else 0), + "view_count": (int(v["view_count"]) if v.get("view_count") is not None else 0), + "patch_count": (int(v["patch_count"]) if v.get("patch_count") is not None else 0), + } + # Preserve agent's metadata (timestamps, state, etc.) + for meta_key in v: + if meta_key not in usage[k]: + usage[k][meta_key] = v[meta_key] skills_data = _skills_list_from_dir(_active_skills_dir()).get("skills", []) skill_names = sorted({s["name"] for s in skills_data}) - total = sum(e["use_count"] for e in usage.values()) - unique = sum(1 for e in usage.values() if e["use_count"] > 0 or e["patch_count"] > 0) + total = sum( + e.get("use_count", 0) + e.get("view_count", 0) + e.get("patch_count", 0) + for e in usage.values() + ) + unique = sum( + 1 for e in usage.values() + if e.get("use_count", 0) > 0 or e.get("view_count", 0) > 0 or e.get("patch_count", 0) > 0 + ) return j(handler, { "usage": usage, "skill_names": skill_names, diff --git a/api/skill_usage.py b/api/skill_usage.py index 88f5ec74..8d4e923a 100644 --- a/api/skill_usage.py +++ b/api/skill_usage.py @@ -1,16 +1,13 @@ -# ── Skill usage counter (file-based, best-effort) ── +# ── Skill usage reader (read-only) ── +# Note: .usage.json is written by hermes-agent (tools/skill_usage.py). +# WebUI only reads to display usage stats in Insights page. import json import logging -import os -import threading -import time - from pathlib import Path logger = logging.getLogger(__name__) -_SKILL_TOOL_NAMES = frozenset({"skill_view", "skill_manage", "skill_patch"}) _USAGE_FILE = ".usage.json" @@ -32,81 +29,4 @@ def read_skill_usage(skills_dir: Path) -> dict: return {} except (json.JSONDecodeError, OSError) as exc: logger.debug("Failed to read .usage.json: %s", exc) - return {} - - -def _ensure_skill_entry(usage: dict, name: str) -> dict: - """Return the metadata dict for *name*, creating a default one if missing.""" - entry = usage.get(name) - if not isinstance(entry, dict): - entry = {"use_count": 0, "view_count": 0, "patch_count": 0} - usage[name] = entry - return entry - - -def increment_skill_usage(skills_dir: Path, tool_calls: list[dict]) -> None: - """Scan *tool_calls* for skill_view/skill_manage invocations and bump - the corresponding counters in .usage.json atomically. - - - ``skill_view`` → increments ``view_count``, sets ``last_viewed_at`` - - ``skill_manage`` → increments ``use_count``, sets ``last_used_at`` - - This is a best-effort helper — failures are logged at DEBUG level and - silently swallowed so they never block the calling stream handler. - """ - if not tool_calls: - return - - updates: list[tuple[str, str]] = [] - for tc in tool_calls: - if not isinstance(tc, dict): - continue - tname = tc.get("name") or tc.get("tool_name") or "" - if tname not in _SKILL_TOOL_NAMES: - continue - args = tc.get("args") or tc.get("input") or {} - if isinstance(args, dict): - skill_name = args.get("name") - if skill_name and isinstance(skill_name, str): - if tname == "skill_view": - action = "view" - elif tname == "skill_patch": - action = "patch" - else: - action = "use" - updates.append((skill_name, action)) - - if not updates: - return - - usage = read_skill_usage(skills_dir) - now = time.time() - - for name, action in updates: - entry = _ensure_skill_entry(usage, name) - if action == "use": - entry["use_count"] = entry.get("use_count", 0) + 1 - entry["last_used_at"] = now - elif action == "patch": - entry["patch_count"] = entry.get("patch_count", 0) + 1 - entry["last_patched_at"] = now - else: - entry["view_count"] = entry.get("view_count", 0) + 1 - entry["last_viewed_at"] = now - - usage_path = skills_dir / _USAGE_FILE - tmp_path = usage_path.with_suffix( - f".tmp.{os.getpid()}.{threading.current_thread().ident}" - ) - try: - tmp_path.write_text( - json.dumps(usage, ensure_ascii=False, indent=2, sort_keys=True), - encoding="utf-8", - ) - tmp_path.replace(usage_path) - except OSError as exc: - logger.debug("Failed to atomically write .usage.json: %s", exc) - try: - tmp_path.unlink(missing_ok=True) - except OSError: - pass + return {} \ No newline at end of file diff --git a/api/streaming.py b/api/streaming.py index d5ced63d..52d93af1 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -5492,13 +5492,6 @@ def _run_agent_streaming( mark_turn_completed(s.session_id, agent=agent) except Exception: logger.debug("Memory lifecycle mark failed for session %s", s.session_id, exc_info=True) - # ── Skill usage counter (non-blocking, best-effort) ── - try: - from api.skill_usage import increment_skill_usage - from api.routes import _active_skills_dir - increment_skill_usage(_active_skills_dir(), s.tool_calls or []) - except Exception: - logger.debug("Skill usage counter update failed", exc_info=True) # Sync to state.db for /insights (opt-in setting) try: from api.config import load_settings as _load_settings diff --git a/tests/test_skill_usage.py b/tests/test_skill_usage.py index 90e2debd..8820173b 100644 --- a/tests/test_skill_usage.py +++ b/tests/test_skill_usage.py @@ -1,24 +1,19 @@ -"""Tests for api/skill_usage.py — file-based skill usage counter. +"""Tests for api/skill_usage.py — .usage.json reader. Covers: - read_skill_usage with various .usage.json states - - increment_skill_usage with skill_view/skill_manage tool_calls - - Non-skill tools and missing name args are correctly ignored - - GET /api/skills/usage route presence + - GET /api/skills/usage route presence (read-only, agent writes the file) """ import json import re from pathlib import Path -from api.skill_usage import increment_skill_usage, read_skill_usage +from api.skill_usage import read_skill_usage _ROUTES = Path(__file__).resolve().parent.parent / "api" / "routes.py" -# ── read_skill_usage ────────────────────────────────────────────────────────── - - class TestReadSkillUsage: def test_read_empty(self, tmp_path): """File does not exist -> returns {}.""" @@ -33,13 +28,13 @@ class TestReadSkillUsage: (tmp_path / ".usage.json").write_text(json.dumps(data), encoding="utf-8") assert read_skill_usage(tmp_path) == data - def test_read_actual_format(self, tmp_path): - """Actual production format with full metadata is accepted.""" + def test_read_agent_format(self, tmp_path): + """Agent-side format (ISO timestamps) is accepted.""" data = { "dev-workflow": { "use_count": 77, "view_count": 77, - "last_used_at": 1712345678.0, + "last_used_at": "2024-04-05T20:54:38Z", "state": "active", }, } @@ -57,110 +52,6 @@ class TestReadSkillUsage: assert read_skill_usage(tmp_path) == {} -# ── increment_skill_usage ───────────────────────────────────────────────────── - - -def _skill_tc(name: str) -> dict: - return {"name": "skill_view", "args": {"name": name}} - - -def _skill_manage_tc(name: str) -> dict: - return {"name": "skill_manage", "args": {"name": name}} - - -def _get_counter(result: dict, name: str, key: str) -> int: - entry = result.get(name, {}) - return entry.get(key, 0) if isinstance(entry, dict) else 0 - - -class TestIncrementSkillUsage: - def test_skill_view_increments_view_count(self, tmp_path): - """A single skill_view call increments view_count to 1.""" - increment_skill_usage(tmp_path, [_skill_tc("research-arxiv")]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "view_count") == 1 - - def test_skill_manage_increments_use_count(self, tmp_path): - """A single skill_manage call increments use_count to 1.""" - increment_skill_usage(tmp_path, [_skill_manage_tc("research-arxiv")]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "use_count") == 1 - - def test_view_and_manage_tracked_separately(self, tmp_path): - """skill_view bumps view_count; skill_manage bumps use_count.""" - increment_skill_usage(tmp_path, [ - _skill_tc("research-arxiv"), - _skill_manage_tc("research-arxiv"), - ]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "view_count") == 1 - assert _get_counter(result, "research-arxiv", "use_count") == 1 - - def test_accumulates(self, tmp_path): - """Same skill called twice -> count = 2.""" - for _ in range(2): - increment_skill_usage(tmp_path, [_skill_manage_tc("research-arxiv")]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "use_count") == 2 - - def test_multiple_skills_in_one_batch(self, tmp_path): - """Multiple skills in a single tool_calls list are all counted.""" - increment_skill_usage(tmp_path, [ - _skill_manage_tc("research-arxiv"), - _skill_manage_tc("hermes-agent"), - _skill_manage_tc("research-arxiv"), - ]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "use_count") == 2 - assert _get_counter(result, "hermes-agent", "use_count") == 1 - - def test_non_skill_tools_ignored(self, tmp_path): - """Terminal, web_search, etc. not counted.""" - increment_skill_usage(tmp_path, [ - {"name": "terminal", "args": {"command": "ls"}}, - {"name": "web_search", "args": {"query": "test"}}, - _skill_manage_tc("github-pr-workflow"), - ]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "github-pr-workflow", "use_count") == 1 - - def test_skill_manage_without_name(self, tmp_path): - """skill_manage with no name arg -> skipped.""" - increment_skill_usage(tmp_path, [ - {"name": "skill_manage", "args": {}}, - _skill_manage_tc("research-arxiv"), - ]) - result = read_skill_usage(tmp_path) - assert _get_counter(result, "research-arxiv", "use_count") == 1 - - def test_empty_tool_calls(self, tmp_path): - """Empty list -> no file written.""" - increment_skill_usage(tmp_path, []) - assert not (tmp_path / ".usage.json").exists() - - def test_preserves_existing_metadata(self, tmp_path): - """Existing metadata fields are preserved after increment.""" - original = { - "dev-workflow": { - "use_count": 77, - "view_count": 50, - "state": "active", - "pinned": False, - }, - } - (tmp_path / ".usage.json").write_text(json.dumps(original), encoding="utf-8") - increment_skill_usage(tmp_path, [_skill_manage_tc("dev-workflow")]) - result = read_skill_usage(tmp_path) - entry = result.get("dev-workflow", {}) - assert entry["use_count"] == 78 - assert entry["view_count"] == 50 - assert entry["state"] == "active" - assert entry["pinned"] is False - - -# ── API route presence ──────────────────────────────────────────────────────── - - class TestApiSkillsUsageRoute: def test_route_handler_present(self): """routes.py contains a handler for GET /api/skills/usage.""" @@ -175,25 +66,9 @@ class TestApiSkillsUsageRoute: def test_route_returns_usage_structure(self): """The route response shape includes usage/skill_names/total_invocations.""" src = _ROUTES.read_text(encoding="utf-8") - match = re.search( - r'return j\(handler,\s*\{[^}]*"usage"[^}]*"skill_names"[^}]*' - r'"total_invocations"[^}]*"unique_skills_used"[^}]*\}\)', - src, - ) - assert match, ( - "Expected /api/skills/usage to return {usage, skill_names, " - "total_invocations, unique_skills_used}" - ) - - -# ── Streaming hook presence ─────────────────────────────────────────────────── - - -class TestStreamingHook: - def test_skill_usage_counter_called_in_streaming(self): - """streaming.py imports and calls increment_skill_usage.""" - from api import streaming as _s - content = Path(_s.__file__).read_text(encoding="utf-8") - assert "increment_skill_usage" in content, ( - "Missing increment_skill_usage call in streaming.py" - ) + # Find the /api/skills/usage handler block and check for key fields + block_match = re.search(r'if parsed\.path == "/api/skills/usage":.*?(?=\n if parsed|$)', src, re.DOTALL) + assert block_match, "Missing /api/skills/usage handler block" + block = block_match.group() + assert '"usage"' in block and '"skill_names"' in block, "Missing usage or skill_names in response" + assert '"total_invocations"' in block and '"unique_skills_used"' in block, "Missing total_invocations or unique_skills_used" \ No newline at end of file