fix: remove webui-side writer to avoid conflict with agent
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)
This commit is contained in:
BonyFish committed
1 parent
9141a55ae0
commit
dc1e369f89
4 files changed
+40
-234
No files matched your search
+24
-6
@@ -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,
|
||||
|
||||
+4
-84
@@ -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 {}
|
||||
@@ -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
|
||||
|
||||
+12
-137
@@ -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"
|
||||
Reference in new issue
Block a user