Merge PR #7312 (contributor head aeab2fb99f)

api/profiles.py

@@ -1866,7 +1866,18 @@ 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): 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

tests/test_issue7305_profiles_fallback.py

@@ -0,0 +1,230 @@
+"""#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 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
+
+# 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 _expected_walked() -> set[str]:
+    return {
+        rel
+        for rel, (_, _, outcome) in SKILLS_TREE.items()
+        if outcome != WALK_PRUNED
+    }
+
+
+@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" or str(name).startswith("agent."):
+            raise ImportError(f"{name} not mounted (simulated two-container Docker)")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", _no_agent)
+
+
+@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 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["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 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;
+    - 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")}
+
+    # 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 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), (compatible, expected_compatible)
+    assert compatible < len(on_disk)
+    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)