From cd516704199e2f152c5c3ed98c264df1a1519e62 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Wed, 26 Aug 2026 10:53:17 -0300 Subject: [PATCH 1/4] fix(profiles): guard agent.skill_utils import in skills-stats fallback (#7305) --- api/profiles.py | 29 +++++++++- tests/test_issue7305_profiles_fallback.py | 64 +++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue7305_profiles_fallback.py diff --git a/api/profiles.py b/api/profiles.py index f5e8432a..ad95993b 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -1862,7 +1862,34 @@ def _compute_profile_skills_stats(profile_dir: Path) -> tuple[int, int]: except Exception: pass - from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform + try: + from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform + except ImportError: + # agent source not mounted (two-container Docker, + # HERMES_WEBUI_CHAT_BACKEND=gateway): the fallback profile path must + # still work — never 500 GET /api/profiles (#7305). Best-effort local + # scan: no platform filtering, minimal frontmatter parse. + def iter_skill_index_files(root, filename="SKILL.md"): + for dirpath, _dirnames, filenames in os.walk(root, followlinks=True): + if filename in filenames: + yield Path(dirpath) / filename + + def parse_frontmatter(content): + data = {} + try: + if content.startswith("---"): + _head, _sep, rest = content.partition("\n---") + fm, _, _body = rest.partition("\n---") + for line in fm.splitlines(): + if ":" in line: + k, v = line.split(":", 1) + data[k.strip()] = v.strip().strip('"\'') + except Exception: + pass + return data, None + + def skill_matches_platform(_frontmatter): + return True seen_names = set() enabled_count = 0 diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py new file mode 100644 index 00000000..c89be6e1 --- /dev/null +++ b/tests/test_issue7305_profiles_fallback.py @@ -0,0 +1,64 @@ +"""#7305: GET /api/profiles must never 500 when agent.skill_utils is not +importable (two-container Docker without the hermes-agent source mounted).""" + +from __future__ import annotations + +import builtins +import importlib +from pathlib import Path + +import pytest + +import api.profiles as profiles + + +def _profile_dir(tmp_path, skill_name="my-skill"): + d = tmp_path / "home" + skills = d / "skills" + (skills / skill_name).mkdir(parents=True) + (skills / skill_name / "SKILL.md").write_text( + "---\nname: " + skill_name + "\ndescription: test skill\n---\nbody\n", + encoding="utf-8", + ) + return d + + +def test_skills_stats_works_without_agent_skill_utils(tmp_path): + """The fallback local scan must serve the count when the agent module is + absent — no ImportError escapes into GET /api/profiles.""" + d = _profile_dir(tmp_path) + real_import = builtins.__import__ + + def no_agent(name, *args, **kwargs): + if name == "agent.skill_utils" or name.startswith("agent."): + raise ImportError("agent not mounted (simulated Docker)") + return real_import(name, *args, **kwargs) + + importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated + try: + # Patch the import used by _get_profile_skills_stats' compute path. + import api.profiles as p2 + orig = p2.__dict__.get("iter_skill_index_files") + # The fallback is installed inside the function; simulate the missing + # module by patching builtins for the module's import machinery. + with pytest.MonkeyPatch.context() as mp: + mp.setattr(builtins, "__import__", no_agent) + enabled, compatible = profiles._get_profile_skills_stats(d) + finally: + pass + assert compatible >= 1, "must count at least the local SKILL.md without agent.skill_utils" + assert enabled >= 1 + + +def test_default_profile_dict_never_raises_without_agent(tmp_path): + """_default_profile_dict() (the fallback row for GET /api/profiles) must + return a dict even with no agent module available.""" + d = _profile_dir(tmp_path) + orig = profiles._DEFAULT_HERMES_HOME + try: + profiles._DEFAULT_HERMES_HOME = d + row = profiles._default_profile_dict() + finally: + profiles._DEFAULT_HERMES_HOME = orig + assert row["name"] == "default" + assert row["skill_count"] >= 1 From d2e470b218bcad28f050e7f3d8392580758887d3 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Wed, 26 Aug 2026 10:59:01 -0300 Subject: [PATCH 2/4] test(profiles): fix ruff F401/F841 in #7305 regression --- tests/test_issue7305_profiles_fallback.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index c89be6e1..c79d403f 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -5,7 +5,6 @@ from __future__ import annotations import builtins import importlib -from pathlib import Path import pytest @@ -37,10 +36,6 @@ def test_skills_stats_works_without_agent_skill_utils(tmp_path): importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated try: # Patch the import used by _get_profile_skills_stats' compute path. - import api.profiles as p2 - orig = p2.__dict__.get("iter_skill_index_files") - # The fallback is installed inside the function; simulate the missing - # module by patching builtins for the module's import machinery. with pytest.MonkeyPatch.context() as mp: mp.setattr(builtins, "__import__", no_agent) enabled, compatible = profiles._get_profile_skills_stats(d) From 86c2252dc80c3f77dffb86ba3874290d0d6b66f4 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 18:08:19 -0300 Subject: [PATCH 3/4] fix(profiles): report skill stats as unknown when agent source is unmounted (#7305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the #7305 regression fix. - Drop the local shadow of ``agent.skill_utils`` (an ``os.walk`` + frontmatter parser + always-True platform filter). A local re-implementation cannot preserve the index walk's directory exclusions, frontmatter-name identity or platform filtering, so it reported inaccurate counts: a SKILL.md declared for another platform was counted as enabled, and support / dependency / VCS / inactive org-mirror files were counted as skills. - The Agent-absent branch now reports the stats as unknown — a stable ``(0, 0)``. GET /api/profiles still cannot 500, and the picker never shows a count it cannot verify (the UI omits the skills line when the total is 0). - Tests now pin that contract: force ``ImportError`` for ``agent.skill_utils`` (and ``hermes_cli``) and assert the unknown-stats result, the fallback row from ``_default_profile_dict()``, and the isolated-mode ``/api/profiles`` path end to end. The fixture reproduces the shapes a partial local scan gets wrong — a directory name differing from the declared ``name``, two directories declaring the same name, a declared name disabled for the webui platform, a platform-incompatible skill, and the paths the index walk prunes (support dirs of a skill package, dependency/VCS/metadata dirs, an inactive org mirror) — and a positive control runs the same tree through the real Agent-backed path so the fixture cannot degrade into a tree with nothing to count. --- api/profiles.py | 32 +-- tests/test_issue7305_profiles_fallback.py | 232 ++++++++++++++++++---- 2 files changed, 201 insertions(+), 63 deletions(-) diff --git a/api/profiles.py b/api/profiles.py index ad95993b..221a2ebc 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -1866,30 +1866,14 @@ def _compute_profile_skills_stats(profile_dir: Path) -> tuple[int, int]: from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform except ImportError: # agent source not mounted (two-container Docker, - # HERMES_WEBUI_CHAT_BACKEND=gateway): the fallback profile path must - # still work — never 500 GET /api/profiles (#7305). Best-effort local - # scan: no platform filtering, minimal frontmatter parse. - def iter_skill_index_files(root, filename="SKILL.md"): - for dirpath, _dirnames, filenames in os.walk(root, followlinks=True): - if filename in filenames: - yield Path(dirpath) / filename - - def parse_frontmatter(content): - data = {} - try: - if content.startswith("---"): - _head, _sep, rest = content.partition("\n---") - fm, _, _body = rest.partition("\n---") - for line in fm.splitlines(): - if ":" in line: - k, v = line.split(":", 1) - data[k.strip()] = v.strip().strip('"\'') - except Exception: - pass - return data, None - - def skill_matches_platform(_frontmatter): - return True + # HERMES_WEBUI_CHAT_BACKEND=gateway): this must never 500 GET + # /api/profiles (#7305). Report the skill stats as unknown — a stable + # (0, 0) — instead of keeping a partial shadow of agent.skill_utils + # here: a local re-implementation cannot preserve the index walk's + # exclusions, frontmatter-name identity or platform filtering, so any + # count it produced would be inaccurate. The UI omits the skills line + # when the total is 0, so the profile picker stays fully usable. + return (0, 0) seen_names = set() enabled_count = 0 diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index c79d403f..8407de62 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -1,59 +1,213 @@ -"""#7305: GET /api/profiles must never 500 when agent.skill_utils is not -importable (two-container Docker without the hermes-agent source mounted).""" +"""#7305: GET /api/profiles must never 500 when the hermes-agent source is not +importable (two-container Docker / HERMES_WEBUI_CHAT_BACKEND=gateway). + +Contract pinned here: when ``agent.skill_utils`` is missing, the WebUI reports +the skill stats as *unknown* — a stable ``(0, 0)`` — instead of counting skills +with a local partial re-implementation of the index walk. The profile picker +still renders, and it never claims a count it cannot verify (the UI omits the +skills line when the total is 0). + +The fixture tree deliberately reproduces the shapes that a partial local scan +gets wrong: a SKILL.md whose declared ``name`` differs from its directory, two +directories declaring the same name, a declared name disabled in the profile +config, a platform-incompatible skill, and the paths the real index walk prunes +(support dirs of a skill package, dependency/VCS/metadata dirs, an inactive +org-mirror). ``test_agent_backed_path_counts_these_shapes_accurately`` is the +positive control: it runs the same tree through the real agent-backed path, so +the fixture cannot silently degrade into "a tree that never had a count to +lose". +""" from __future__ import annotations import builtins -import importlib +import pathlib import pytest import api.profiles as profiles +# How a fixture entry relates to the reported counts. +COUNTED = "counted" # yields a logical skill; contributes to compatible/enabled +WALK_PRUNED = "walk-pruned" # never yielded by agent.skill_utils' index walk +PLATFORM_FILTERED = "platform-filtered" # walked, but incompatible with this host -def _profile_dir(tmp_path, skill_name="my-skill"): - d = tmp_path / "home" - skills = d / "skills" - (skills / skill_name).mkdir(parents=True) - (skills / skill_name / "SKILL.md").write_text( - "---\nname: " + skill_name + "\ndescription: test skill\n---\nbody\n", - encoding="utf-8", - ) - return d +# path -> (declared frontmatter name, platform restriction or None, outcome) +SKILLS_TREE = { + # Declared name differs from the directory name; that declared name is + # disabled for the webui platform in config.yaml below. + "alpha-dir/SKILL.md": ("my-skill", None, COUNTED), + # A second directory declaring the same name — one logical skill. + "beta-dir/SKILL.md": ("my-skill", None, COUNTED), + # Platform-incompatible: must not be offered (or counted) on this host. + "gamma-dir/SKILL.md": ("gamma-skill", "some-other-platform", PLATFORM_FILTERED), + # A plain, enabled skill. + "delta-dir/SKILL.md": ("delta-skill", None, COUNTED), + # Support dir of a skill package: disclosure-only, not a standalone skill. + "delta-dir/references/notes/SKILL.md": ("support-note", None, WALK_PRUNED), + # Dependency / virtualenv / VCS / metadata dirs are excluded wholesale, and + # an org mirror without its .active_org marker is token-gated shut. + "node_modules/dep/SKILL.md": ("vendored-skill", None, WALK_PRUNED), + ".venv/lib/pkg/SKILL.md": ("venv-skill", None, WALK_PRUNED), + ".git/hooks/SKILL.md": ("hook-skill", None, WALK_PRUNED), + "_org/other-org/org-skill/SKILL.md": ("org-skill", None, WALK_PRUNED), +} + +# The one declared name the profile config disables for the webui platform. +DISABLED_NAMES = {"my-skill"} + +CONFIG_YAML = ( + "profile:\n" + " name: default\n" + "skills:\n" + " platform_disabled:\n" + " webui:\n" + " - my-skill\n" +) + +CONFIG_WITHOUT_DISABLE = "profile:\n name: default\n" -def test_skills_stats_works_without_agent_skill_utils(tmp_path): - """The fallback local scan must serve the count when the agent module is - absent — no ImportError escapes into GET /api/profiles.""" - d = _profile_dir(tmp_path) +def _expected_walked() -> set[str]: + return { + rel + for rel, (_, _, outcome) in SKILLS_TREE.items() + if outcome != WALK_PRUNED + } + + +def _expected_names(outcome: str) -> set[str]: + return {name for name, _, entry_outcome in SKILLS_TREE.values() if entry_outcome == outcome} + + +@pytest.fixture() +def agent_absent(monkeypatch): + """Simulate the agent-less deployment: importing anything under ``agent.`` + raises ``ImportError``, even when the source tree is present locally.""" real_import = builtins.__import__ - def no_agent(name, *args, **kwargs): - if name == "agent.skill_utils" or name.startswith("agent."): - raise ImportError("agent not mounted (simulated Docker)") + def _no_agent(name, *args, **kwargs): + if name == "agent" or str(name).startswith("agent."): + raise ImportError(f"{name} not mounted (simulated two-container Docker)") return real_import(name, *args, **kwargs) - importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated - try: - # Patch the import used by _get_profile_skills_stats' compute path. - with pytest.MonkeyPatch.context() as mp: - mp.setattr(builtins, "__import__", no_agent) - enabled, compatible = profiles._get_profile_skills_stats(d) - finally: - pass - assert compatible >= 1, "must count at least the local SKILL.md without agent.skill_utils" - assert enabled >= 1 + monkeypatch.setattr(builtins, "__import__", _no_agent) -def test_default_profile_dict_never_raises_without_agent(tmp_path): +@pytest.fixture() +def hermes_cli_absent(monkeypatch): + """Simulate the same deployment's other half: no ``hermes_cli`` either.""" + real_import = builtins.__import__ + + def _no_cli(name, *args, **kwargs): + if name == "hermes_cli" or str(name).startswith("hermes_cli."): + raise ImportError(f"{name} not mounted (simulated two-container Docker)") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _no_cli) + + +def _write_profile(tmp_path: pathlib.Path, dir_name: str = "home") -> pathlib.Path: + """Materialise SKILLS_TREE + config.yaml as a profile home.""" + home = tmp_path / dir_name + skills = home / "skills" + for rel, (name, platform, _outcome) in SKILLS_TREE.items(): + skill_md = skills / rel + skill_md.parent.mkdir(parents=True, exist_ok=True) + lines = ["---", f"name: {name}", "description: issue 7305 fixture"] + if platform: + lines += ["platforms:", f" - {platform}"] + lines += ["---", "body", ""] + skill_md.write_text("\n".join(lines), encoding="utf-8") + (home / "config.yaml").write_text(CONFIG_YAML, encoding="utf-8") + return home + + +def test_skills_stats_are_unknown_without_agent_skill_utils(tmp_path, agent_absent): + """Without agent.skill_utils the stats are (0, 0) — never a guessed count, + no matter which fixtures a partial local scan would have counted.""" + home = _write_profile(tmp_path) + + assert profiles._get_profile_skills_stats(home) == (0, 0) + + +def test_default_profile_dict_never_raises_without_agent(tmp_path, agent_absent, monkeypatch): """_default_profile_dict() (the fallback row for GET /api/profiles) must - return a dict even with no agent module available.""" - d = _profile_dir(tmp_path) - orig = profiles._DEFAULT_HERMES_HOME - try: - profiles._DEFAULT_HERMES_HOME = d - row = profiles._default_profile_dict() - finally: - profiles._DEFAULT_HERMES_HOME = orig + return a usable row with unknown skill stats instead of raising.""" + home = _write_profile(tmp_path) + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", home) + + row = profiles._default_profile_dict() + assert row["name"] == "default" - assert row["skill_count"] >= 1 + assert row["path"] == str(home) + assert row["is_default"] is True + assert row["skill_count"] == 0 + assert row["enabled_skills"] == 0 + assert row["total_skills"] == 0 + + +def test_list_profiles_api_isolated_mode_without_agent_or_cli( + tmp_path, agent_absent, hermes_cli_absent, monkeypatch +): + """The #7305 path end to end: isolated profile mode with neither + agent.skill_utils nor hermes_cli importable must still return the + default-only row instead of propagating ImportError out of the route.""" + home = _write_profile(tmp_path, dir_name="default") + monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: True) + monkeypatch.setattr(profiles, "_INITIAL_HERMES_HOME", str(home)) + + rows = profiles.list_profiles_api() + + assert [r["name"] for r in rows] == ["default"] + assert rows[0]["is_active"] is True + assert rows[0]["skill_count"] == 0 + assert rows[0]["enabled_skills"] == 0 + assert rows[0]["total_skills"] == 0 + + +def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): + """Positive control (agent source present): the fixture above is a real + semantic trap, not a tree with nothing to count. + + - the index walk yields exactly the non-pruned SKILL.md files, so the + support/dependency/VCS/org-mirror files a naive local ``os.walk`` would + pick up are invisible to the count; + - the platform-incompatible and duplicate-declared-name files do not add to + the compatible total, which therefore stays below the walk's file count; + - identity comes from frontmatter ``name``: the disabled ``my-skill`` (a + declared name, not a directory) is compatible but not enabled; + - dropping that config disable flips it to enabled without changing the + compatible total. + """ + pytest.importorskip("agent.skill_utils") + from agent.skill_utils import iter_skill_index_files + + home = _write_profile(tmp_path) + skills = home / "skills" + + walked = { + p.relative_to(skills).as_posix() for p in iter_skill_index_files(skills, "SKILL.md") + } + on_disk = {p.relative_to(skills).as_posix() for p in skills.rglob("SKILL.md")} + + expected_walked = _expected_walked() + expected_compatible = _expected_names(COUNTED) + expected_enabled = expected_compatible - DISABLED_NAMES + + # The fixture must contain paths a naive scan would over-count... + assert on_disk > walked, "fixture must include SKILL.md files the index walk prunes" + # ...and the walk must still see exactly the paths it is documented to keep. + assert walked == expected_walked + + enabled, compatible = profiles._compute_profile_skills_stats(home) + + assert compatible == len(expected_compatible) + assert compatible < len(walked) + assert enabled == len(expected_enabled) + + (home / "config.yaml").write_text(CONFIG_WITHOUT_DISABLE, encoding="utf-8") + enabled_all, compatible_all = profiles._compute_profile_skills_stats(home) + + assert compatible_all == compatible + assert enabled_all == len(expected_compatible) From aeab2fb99f7bbf484f174663681c28421d9e1364 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 18:27:46 -0300 Subject: [PATCH 4/4] test(profiles): derive skill-count expectations from the agent walk output (#7305) The positive control asserted on_disk > walked, i.e. it froze the agent package's index-walk exclusion list. In an environment whose agent.skill_utils yields every SKILL.md (no pruning) both sets are equal and the assertion failed, which is what broke CI on the previous head. Expectations are now derived from the paths the walk actually yielded, so the test keeps asserting the property that belongs to this code - the compatible total stays strictly below the walked file count via platform filtering and frontmatter-name deduplication - without freezing another package's exclusion policy. --- tests/test_issue7305_profiles_fallback.py | 49 +++++++++++++++-------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index 8407de62..3a3331e3 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -76,10 +76,6 @@ def _expected_walked() -> set[str]: } -def _expected_names(outcome: str) -> set[str]: - return {name for name, _, entry_outcome in SKILLS_TREE.values() if entry_outcome == outcome} - - @pytest.fixture() def agent_absent(monkeypatch): """Simulate the agent-less deployment: importing anything under ``agent.`` @@ -170,11 +166,16 @@ def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): """Positive control (agent source present): the fixture above is a real semantic trap, not a tree with nothing to count. - - the index walk yields exactly the non-pruned SKILL.md files, so the - support/dependency/VCS/org-mirror files a naive local ``os.walk`` would - pick up are invisible to the count; + The fixture's expectations are derived from the paths the agent's index + walk actually yields, because that walk (and its exclusion list) belongs to + the agent package and differs between deployments. What this asserts: + + - the compatible total stays strictly below the walked file count — the + support/dependency/VCS/org-mirror files a naive local ``os.walk`` picks + up are either pruned by the walk or, when the walk keeps them, still not + enough to reach the walk's file count; - the platform-incompatible and duplicate-declared-name files do not add to - the compatible total, which therefore stays below the walk's file count; + the compatible total; - identity comes from frontmatter ``name``: the disabled ``my-skill`` (a declared name, not a directory) is compatible but not enabled; - dropping that config disable flips it to enabled without changing the @@ -191,19 +192,35 @@ def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): } on_disk = {p.relative_to(skills).as_posix() for p in skills.rglob("SKILL.md")} - expected_walked = _expected_walked() - expected_compatible = _expected_names(COUNTED) + # The index walk itself belongs to the agent package and its exclusion list + # is version-dependent, so this test does not freeze it: every expectation + # below is derived from the paths the walk actually yielded. (Removing the + # local mirror of that walk is the point of #7305 — the fallback reported + # (0, 0) instead of guessing.) + assert _expected_walked() <= walked, "the walk must keep the non-pruned skills" + assert walked <= on_disk + + expected_compatible = { + name + for rel, (name, platform, _outcome) in SKILLS_TREE.items() + if rel in walked and platform is None + } expected_enabled = expected_compatible - DISABLED_NAMES - # The fixture must contain paths a naive scan would over-count... - assert on_disk > walked, "fixture must include SKILL.md files the index walk prunes" - # ...and the walk must still see exactly the paths it is documented to keep. - assert walked == expected_walked + # The fixture is a real semantic trap: the compatible total must stay below + # the walked file count. If the walk pruned the support/dependency/VCS/org + # files the count drops because those files are gone; if it did not, the + # count still drops because the platform-incompatible file is filtered out + # and the two directories declaring `my-skill` collapse to one logical + # skill. + assert len(expected_compatible) < len(walked), ( + "fixture must make the compatible total fall below the walked file count" + ) enabled, compatible = profiles._compute_profile_skills_stats(home) - assert compatible == len(expected_compatible) - assert compatible < len(walked) + assert compatible == len(expected_compatible), (compatible, expected_compatible) + assert compatible < len(on_disk) assert enabled == len(expected_enabled) (home / "config.yaml").write_text(CONFIG_WITHOUT_DISABLE, encoding="utf-8")