From c13232e90c786b4ab84f6c8ec4a83b23736330a7 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 29 Sep 2026 14:54:25 -0500 Subject: [PATCH] feat(skills): external mounts get an 'external' provenance tier (#108032) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills mounted from skills.external_dirs are externally authored, but every provenance surface flattened them into 'agent': the usage report, the dashboard's set-based twin, the desktop catalog/detail UI, and the learning journey graph (where a used external mount rendered as a learned skill). - tools/skill_usage: provenance() gains an 'external' tier (external-only: a local copy of the same name keeps winning, mirroring is_agent_created's external exclusion and _external_skill_names() scans external dirs once for the router's set-based twin. - web_routers/skills: /api/skills classifies hub > bundled > external > agent. - learning_graph: skills whose path resolves under an external dir are tagged source='external' and are never learning milestones — a mount is configured, not learned, even when used. - desktop: SkillInfo.provenance gains 'external'; the catalog labels mounts 'external' and the detail pane keeps them editable in place. Origin only, never mutability: read-only enforcement for external mounts was deliberately rejected by maintainers (commit 8c8fc6c1ec / PR #17512 — a refusal just breeds silent local duplicates). Provenance decisions extracted into skill-provenance.ts pure helpers (testable without source-reading tests). - curator: usage report/filter knows the 'external' origin. The desktop edit logic in this commit is the natural reading of 8c8fc6c1ec (external dirs exist so the user can point Hermes at a vault they curate elsewhere); if maintainers prefer the read-only half as well, it slots in behind isEditableProvenance() in one place. EOF ) --- agent/learning_graph.py | 16 ++- .../capabilities/catalog/skill-catalog.tsx | 3 +- .../app/capabilities/skills/skill-detail.tsx | 7 +- .../skills/skill-provenance.test.ts | 42 ++++++ .../capabilities/skills/skill-provenance.ts | 22 +++ apps/desktop/src/types/hermes.ts | 5 +- hermes_cli/curator.py | 6 +- hermes_cli/web_routers/skills.py | 13 +- tests/tools/test_skill_provenance_external.py | 127 ++++++++++++++++++ tools/skill_usage.py | 34 ++++- 10 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 apps/desktop/src/app/capabilities/skills/skill-provenance.test.ts create mode 100644 apps/desktop/src/app/capabilities/skills/skill-provenance.ts create mode 100644 tests/tools/test_skill_provenance_external.py diff --git a/agent/learning_graph.py b/agent/learning_graph.py index ca6c9e645f..13b9228dcd 100644 --- a/agent/learning_graph.py +++ b/agent/learning_graph.py @@ -82,6 +82,14 @@ def _to_int_ts(value: Any) -> Optional[int]: def build_skill_nodes(skill_roots: list[tuple[str, Path]]) -> dict[str, SkillNode]: usage = _load_usage() nodes: dict[str, SkillNode] = {} + # Tag skills mounted from skills.external_dirs so the journey graph can keep them out of + # learning milestones (#108032): a mount is configured, not learned. Path-based (the common + # symlink into the profile tree resolves to the external root), so a local copy of the same + # name still classifies as its own source. + try: + from agent.skill_utils import is_external_skill_path + except Exception: + is_external_skill_path = None # type: ignore[assignment] for source, root in skill_roots: for skill_md in root.rglob("SKILL.md") if root.exists() else (): if _SKIP_PARTS.intersection(skill_md.parts): @@ -100,6 +108,8 @@ def build_skill_nodes(skill_roots: list[tuple[str, Path]]) -> dict[str, SkillNod continue rec, cat, parts = usage.get(name, {}), _fm_field(fm, "category"), skill_md.parts # …/skills///SKILL.md usage_ts = next((ts for ts in (_to_int_ts(rec.get(k)) for k in _USAGE_TS_KEYS) if ts is not None), None) + if is_external_skill_path is not None and is_external_skill_path(skill_md): + source = "external" nodes[name] = SkillNode( name=name, category=str(cat) if cat else parts[-3] if len(parts) >= 3 else "general", source=source, timestamp=usage_ts or _to_int_ts(skill_md.stat().st_mtime), @@ -195,9 +205,11 @@ def _has_learning_signal(node: SkillNode) -> bool: """Graph-worthy: agent-created, user-taught (/learn), or actually used. ``created_by="learn"`` is a learning-signal marker only — curator management stays keyed - strictly on ``"agent"`` (see ``tools.skill_usage._is_curator_managed_record``). + strictly on ``"agent"`` (see ``tools.skill_usage._is_curator_managed_record``). External + mounts are never a learning milestone: they were configured by the user, not learned, so + even a used external skill stays out of the journey graph (#108032). """ - return node.created_by in {"agent", "learn"} or node.use_count > 0 + return node.source != "external" and (node.created_by in {"agent", "learn"} or node.use_count > 0) def build_learning_graph() -> dict[str, Any]: diff --git a/apps/desktop/src/app/capabilities/catalog/skill-catalog.tsx b/apps/desktop/src/app/capabilities/catalog/skill-catalog.tsx index befefc6f0c..84e3452297 100644 --- a/apps/desktop/src/app/capabilities/catalog/skill-catalog.tsx +++ b/apps/desktop/src/app/capabilities/catalog/skill-catalog.tsx @@ -12,6 +12,7 @@ import type { SkillHubSourcesResponse, SkillInfo } from '@/types/hermes' import { CatalogAlert } from './catalog-alert' import { CatalogBrowser } from './catalog-browser' import { type CatalogEntry, parseCatalog } from './catalog-data' +import { catalogSourceFor } from '../skills/skill-provenance' interface SkillCatalogProps { skills: SkillInfo[] @@ -104,7 +105,7 @@ function ScopedSkillCatalog({ name: skill.name, description: skill.description, category: skill.category, - source: skill.provenance === 'bundled' ? 'built-in' : skill.provenance === 'hub' ? 'hub' : 'local' + source: catalogSourceFor(skill.provenance) })) ).map(entry => { const skill = skillsByName.get(entry.name)! diff --git a/apps/desktop/src/app/capabilities/skills/skill-detail.tsx b/apps/desktop/src/app/capabilities/skills/skill-detail.tsx index 5235593e80..260b4c02c9 100644 --- a/apps/desktop/src/app/capabilities/skills/skill-detail.tsx +++ b/apps/desktop/src/app/capabilities/skills/skill-detail.tsx @@ -8,6 +8,7 @@ import { useI18n } from '@/i18n' import type { SkillInfo } from '@/types/hermes' import { parseFrontmatter } from './frontmatter' +import { isEditableProvenance } from './skill-provenance' export function SkillDetail({ onArchive, @@ -21,9 +22,9 @@ export function SkillDetail({ skill: SkillInfo }) { const { t } = useI18n() - // Only learned/local skills are the user's to rewrite or archive — bundled - // and hub skills are managed by their sources. - const editable = skill.provenance === 'agent' + // Origin only, never mutability: external mounts stay editable in place — + // see ./skill-provenance (commit 8c8fc6c1ec). + const editable = isEditableProvenance(skill.provenance) // The FULL skill — frontmatter metadata + complete SKILL.md body — for any // provenance, scoped to the Capabilities profile selector. The row list only diff --git a/apps/desktop/src/app/capabilities/skills/skill-provenance.test.ts b/apps/desktop/src/app/capabilities/skills/skill-provenance.test.ts new file mode 100644 index 0000000000..590a158eba --- /dev/null +++ b/apps/desktop/src/app/capabilities/skills/skill-provenance.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' + +import type { SkillInfo } from '@/types/hermes' + +import { catalogSourceFor, isEditableProvenance } from './skill-provenance' + +// #108032: externally mounted skills get an 'external' provenance tier — origin +// labeling without losing in-place edit rights (commit 8c8fc6c1ec / PR #17512). +describe('skill provenance tiers', () => { + describe('catalogSourceFor', () => { + it('maps every provenance tier to a distinct catalog source', () => { + expect(catalogSourceFor('bundled')).toBe('built-in') + expect(catalogSourceFor('hub')).toBe('hub') + expect(catalogSourceFor('external')).toBe('external') + expect(catalogSourceFor('agent')).toBe('local') + }) + + it('treats an absent provenance as local (older backends)', () => { + expect(catalogSourceFor(undefined)).toBe('local') + }) + }) + + describe('isEditableProvenance', () => { + it('keeps learned/local skills editable', () => { + expect(isEditableProvenance('agent')).toBe(true) + }) + + it('keeps external mounts editable in place', () => { + expect(isEditableProvenance('external')).toBe(true) + }) + + it('keeps bundled and hub skills managed by their sources', () => { + expect(isEditableProvenance('bundled')).toBe(false) + expect(isEditableProvenance('hub')).toBe(false) + }) + + it('treats an absent provenance as not editable (older backends predate edit rights)', () => { + const skill: SkillInfo = { name: 'legacy', description: 'd' } + expect(isEditableProvenance(skill.provenance)).toBe(false) + }) + }) +}) diff --git a/apps/desktop/src/app/capabilities/skills/skill-provenance.ts b/apps/desktop/src/app/capabilities/skills/skill-provenance.ts new file mode 100644 index 0000000000..9a4bf11230 --- /dev/null +++ b/apps/desktop/src/app/capabilities/skills/skill-provenance.ts @@ -0,0 +1,22 @@ +import type { SkillInfo } from '@/types/hermes' + +/** + * #108032: an external mount is not a managed hub/bundled source — it is the user's + * own directory pointed at via skills.external_dirs. Origin only, never mutability: + * external skills stay editable in place (a refusal would just breed silent local + * duplicates — commit 8c8fc6c1ec / PR #17512). + */ + +export function catalogSourceFor(provenance: SkillInfo['provenance']): 'built-in' | 'hub' | 'external' | 'local' { + if (provenance === 'bundled') return 'built-in' + if (provenance === 'hub') return 'hub' + if (provenance === 'external') return 'external' + return 'local' +} + +export function isEditableProvenance(provenance: SkillInfo['provenance']): boolean { + // Only learned/local skills are the user's to rewrite or archive — bundled and + // hub skills are managed by their sources. External mounts stay editable in + // place (the user pointed skills.external_dirs at them). + return provenance === 'agent' || provenance === 'external' +} diff --git a/apps/desktop/src/types/hermes.ts b/apps/desktop/src/types/hermes.ts index c40a0bfc00..2b648a98af 100644 --- a/apps/desktop/src/types/hermes.ts +++ b/apps/desktop/src/types/hermes.ts @@ -1177,8 +1177,9 @@ export interface SkillInfo { name: string /** Total observed activity (use + view + patch). Absent on older backends. */ usage?: number - /** 'agent' = learned/local (editable), 'bundled' = ships with Hermes, 'hub' = installed. */ - provenance?: 'agent' | 'bundled' | 'hub' + /** 'agent' = learned/local (editable), 'bundled' = ships with Hermes, 'hub' = installed, + * 'external' = mounted from skills.external_dirs (externally authored, still editable). */ + provenance?: 'agent' | 'bundled' | 'external' | 'hub' } /** One entry of the built-in optional-skills catalog (optional-skills/ in the diff --git a/hermes_cli/curator.py b/hermes_cli/curator.py index 00a4e6954b..b02e8a4d5f 100644 --- a/hermes_cli/curator.py +++ b/hermes_cli/curator.py @@ -590,10 +590,10 @@ def _cmd_usage(args) -> int: print("curator: no skills found") return 0 provenance = [r.get("provenance", "agent") for r in rows] - counts = {k: provenance.count(k) for k in ("agent", "bundled", "hub")} + counts = {k: provenance.count(k) for k in ("agent", "bundled", "hub", "external")} print( f"skills: {len(rows)} total " - f"(agent={counts['agent']} bundled={counts['bundled']} hub={counts['hub']})\n") + f"(agent={counts['agent']} bundled={counts['bundled']} hub={counts['hub']} external={counts['external']})\n") print( f" {'skill':40s} {'origin':8s} " f"{'use':>4s} {'view':>4s} {'patch':>5s} {'act':>4s} last_activity") @@ -626,7 +626,7 @@ _SUBCOMMANDS = ( _arg("--sort", choices=("activity", "recent", "name"), default="activity", help="Sort order: activity (most-used first, default), recent " "(most-recently-active first), or name (alphabetical)"), - _arg("--provenance", choices=("agent", "bundled", "hub"), default=None, + _arg("--provenance", choices=("agent", "bundled", "hub", "external"), default=None, help="Only show skills of this origin"), _arg("--json", **_STORE_TRUE, help="Emit the full report as JSON instead of a table")), ( diff --git a/hermes_cli/web_routers/skills.py b/hermes_cli/web_routers/skills.py index 9fc1f6dc36..e66fd9b6bb 100644 --- a/hermes_cli/web_routers/skills.py +++ b/hermes_cli/web_routers/skills.py @@ -345,7 +345,7 @@ async def get_skills(profile: Optional[str] = None): from tools.skills_tool import _find_all_skills from hermes_cli.skills_config import get_disabled_skills from tools.skill_usage import ( - _read_bundled_names, _read_hub_installed_names, activity_count, load_usage) + _external_skill_names, _read_bundled_names, _read_hub_installed_names, activity_count, load_usage) def _run(): with _profile_scope(profile): @@ -354,17 +354,22 @@ async def get_skills(profile: Optional[str] = None): skills = _find_all_skills(skip_disabled=True) usage = load_usage() # Set-based provenance (same classification as skill_usage.provenance, - # without a per-skill manifest read): hub > bundled > agent, where - # "agent" covers agent-authored AND local hand-made skills — the ones - # the user may edit/delete from the UI. + # without a per-skill manifest read): hub > bundled > external > agent. + # "external" is mounted from skills.external_dirs and absent locally — + # externally authored, NOT learned. "agent" covers agent-authored AND + # local hand-made skills — the ones the user may edit/delete from the + # UI; external skills keep their in-place foreground edit rights + # regardless of label (commit 8c8fc6c1ec). bundled_names = _read_bundled_names() hub_names = _read_hub_installed_names() + external_names = _external_skill_names() - bundled_names - hub_names for s in skills: s["enabled"] = s["name"] not in disabled s["usage"] = activity_count(usage.get(s["name"], {})) s["provenance"] = ( "hub" if s["name"] in hub_names else "bundled" if s["name"] in bundled_names + else "external" if s["name"] in external_names else "agent") return skills diff --git a/tests/tools/test_skill_provenance_external.py b/tests/tools/test_skill_provenance_external.py new file mode 100644 index 0000000000..55eff1c236 --- /dev/null +++ b/tests/tools/test_skill_provenance_external.py @@ -0,0 +1,127 @@ +"""#108032: external-dir skills get an 'external' provenance tier, not 'agent'. + +Externally mounted skills (skills.external_dirs) are externally authored: the label must say +where they came from without changing their foreground mutability (in-place edits are the +standing design, commit 8c8fc6c1ec / PR #17512) and without counting as learning milestones +in the journey graph. +""" + +import pytest + + +@pytest.fixture +def external_home(tmp_path, monkeypatch): + """HERMES_HOME with one external dir configured and one external skill in it. + + The external skill is ALSO reachable from the profile skills tree via a symlinked + SKILL.md — the common mounting style — because that is the shape the journey graph + actually sees (its roots are the bundled and profile skills dirs; rglob there does not + traverse dir symlinks but does yield symlinked files, and is_external_skill_path + resolves the link to tag it 'external'). + """ + home = tmp_path / ".hermes" + (home / "skills").mkdir(parents=True) + ext_dir = tmp_path / "vault" + (ext_dir / "ext-skill").mkdir(parents=True) + (ext_dir / "ext-skill" / "SKILL.md").write_text( + "---\nname: ext-skill\ndescription: An external skill.\n---\n\n# External\n", encoding="utf-8") + (home / "skills" / "ext-linked").mkdir() + (home / "skills" / "ext-linked" / "SKILL.md").symlink_to(ext_dir / "ext-skill" / "SKILL.md") + (home / "config.yaml").write_text(f"skills:\n external_dirs:\n - {ext_dir}\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr("hermes_constants._hermes_home_cache", None, raising=False) + + from agent import skill_utils + skill_utils._external_dirs_cache_clear() + yield home + skill_utils._external_dirs_cache_clear() + + +def _write_local_skill(home, name: str): + d = home / "skills" / name + d.mkdir(parents=True, exist_ok=True) + (d / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Local skill.\n---\n\n# Local\n", encoding="utf-8") + + +class TestProvenanceTier: + def test_external_only_skill_classifies_as_external(self, external_home): + from tools.skill_usage import provenance + assert provenance("ext-skill") == "external" + + def test_local_skill_stays_agent(self, external_home): + _write_local_skill(external_home, "my-local") + from tools.skill_usage import provenance + assert provenance("my-local") == "agent" + + def test_local_copy_wins_over_external_mount(self, external_home): + """A name present both locally and externally is NOT external (local store wins).""" + _write_local_skill(external_home, "ext-skill") + from tools.skill_usage import provenance + assert provenance("ext-skill") == "agent" + + def test_agent_created_still_excludes_external_only(self, external_home): + from tools.skill_usage import is_agent_created + assert is_agent_created("ext-skill") is False + + def test_external_names_helper_scans_external_dirs(self, external_home): + from tools.skill_usage import _external_skill_names + assert _external_skill_names() == {"ext-skill"} + + +class TestLearningGraphExcludesExternal: + def test_used_external_skill_produces_no_learning_node(self, external_home): + """Even a USED external skill is not a learning milestone (#108032).""" + from agent import learning_graph + with _patched_usage({"ext-skill": {"use_count": 5, "created_by": None}}): + skill_ids = {n["id"] for n in learning_graph.build_learning_graph()["nodes"] + if n["kind"] == "skill"} + assert "ext-skill" not in skill_ids + + def test_used_local_skill_still_produces_learning_node(self, external_home): + _write_local_skill(external_home, "my-local") + from agent import learning_graph + with _patched_usage({"my-local": {"use_count": 3, "created_by": None}}): + skill_ids = {n["id"] for n in learning_graph.build_learning_graph()["nodes"] + if n["kind"] == "skill"} + assert "my-local" in skill_ids + + +def _patched_usage(data): + from unittest.mock import patch + from agent import learning_graph + return patch.object(learning_graph, "_load_usage", lambda: data) + + +class TestWebRouterProvenanceTwin: + """The /api/skills set-based twin must classify external mounts as 'external' while a + local hand-made skill stays 'agent' (the regression this guards: hub > bundled > agent + with no external tier flattened mounts into 'agent'). + + ``get_skills`` resolves its imports inside the function body, so monkeypatching router + module attributes cannot intercept them; drive the real app through TestClient instead + (same harness as test_web_server_skills_profiles.py).""" + + @pytest.fixture + def client(self, external_home, monkeypatch): + try: + from starlette.testclient import TestClient + except ImportError: + pytest.skip("fastapi/starlette not installed") + + import hermes_state + from hermes_constants import get_hermes_home + from hermes_cli.web_server import app, _SESSION_HEADER_NAME, _SESSION_TOKEN + + monkeypatch.setattr(hermes_state, "DEFAULT_DB_PATH", get_hermes_home() / "state.db") + c = TestClient(app) + c.headers[_SESSION_HEADER_NAME] = _SESSION_TOKEN + return c + + def test_router_labels_external_vs_agent(self, client, external_home): + _write_local_skill(external_home, "my-local") + resp = client.get("/api/skills") + assert resp.status_code == 200, resp.text + by_name = {s["name"]: s["provenance"] for s in resp.json()} + assert by_name.get("ext-skill") == "external" + assert by_name.get("my-local") == "agent" diff --git a/tools/skill_usage.py b/tools/skill_usage.py index da220970a9..ee202c35a8 100644 --- a/tools/skill_usage.py +++ b/tools/skill_usage.py @@ -713,9 +713,39 @@ def curated_report() -> List[Dict[str, Any]]: return [_report_row(n, data.get(n), _persisted=n in data, provenance=provenance(n)) for n in sorted(names)] +def is_external_only(skill_name: str) -> bool: + """Present under ``skills.external_dirs`` and NOT in the local store (the local copy wins on + name clashes, mirroring ``is_agent_created``'s external exclusion). An external-only skill is + externally authored: it was mounted, not learned.""" + return (_find_skill_dir(skill_name) is None and _find_external_skill_dir(skill_name) is not None) + + +def _external_skill_names() -> set: + """Frontmatter names of all skills under configured external dirs (one scan per call; the + router's set-based provenance uses this instead of a per-skill dir lookup).""" + from agent.skill_utils import get_all_skills_dirs + names: set = set() + for base in get_all_skills_dirs()[1:]: + if not base.exists(): + continue + for skill_md in base.rglob("SKILL.md"): + if not is_excluded_skill_path(skill_md): + names.add(_read_skill_name(skill_md, fallback=skill_md.parent.name)) + return names + + def provenance(skill_name: str) -> str: - """'hub' | 'bundled' | 'agent' (the latter also covers local manually-authored skills).""" - return "hub" if is_hub_installed(skill_name) else "bundled" if is_bundled(skill_name) else "agent" + """'hub' | 'bundled' | 'external' | 'agent'. + + 'external' covers skills mounted from ``skills.external_dirs`` and absent from the local + store — externally authored, not learned from the user (#108032). 'agent' keeps covering + agent-authored AND local hand-made skills — the ones the user may edit/delete from the UI + (external skills stay foreground-editable in place by design, commit 8c8fc6c1ec; the label + split is about origin, not mutability).""" + return ("hub" if is_hub_installed(skill_name) + else "bundled" if is_bundled(skill_name) + else "external" if is_external_only(skill_name) + else "agent") def usage_report() -> List[Dict[str, Any]]: