feat(skills): org-skill namespace — token-gated discovery, fail-loud collisions, provenance (M2)
Implements the agreed design (2026-07-23): org skills are FIRST-CLASS (bare names) with three hard companions. 1. TOKEN-GATED RESOLUTION: _org/<org_id>/ mirrors resolve ONLY while marked active. pull_org_skills (which runs only after the token's org_id+org_role verified) writes _org/.active_org; discovery (iter_skill_index_files, _find_skill_dir, snapshot manifest) prunes every other mirror. Leave the org (verified personal token in maybe_pull_org_skills) => marker cleared => org skills stop resolving; offline => marker untouched (grace). Snapshot manifest includes the marker so org switches invalidate the prompt snapshot; _SKILLS_SNAPSHOT_VERSION bumped to 2. 2. FAIL-LOUD COLLISIONS: listing pass unified across snapshot/scan paths; a personal/org name clash flags BOTH entries '[name collision — load via category path]' — neither side silently wins (personal-wins = silent divergence from the org set; org-wins = shadowed personal work). skill_view's existing multi-candidate refusal already rejects the ambiguous bare name. 3. PROVENANCE: org entries list under an org:<org_id> category with '[org-shared: by <author>]' tags; skill_view prepends a load-time header (org, author, as-of + read-only/fork-and-propose guidance) INTO the content the model consumes, plus an org_provenance result field. Author comes from the pull-time .org-provenance.json sidecar (HEAD commit author — token-verified at push by the plane's author_mismatch guard, gg #166). 4. READ-ONLY MIRROR: skill_manage patch/edit/delete/write_file refuse org- mirror targets with fork-and-propose guidance; org skills are curation- exempt (is_curation_eligible False — the org HEAD owns them). Tests: 11 new (tests/agent/test_org_skill_namespace.py) covering gating, stale-mirror pruning, org-switch flip, snapshot provenance, listing labels, both-sides collision flags, read-only guard, curation exemption; 448 green across skills/prompt/sync suites. Live E2E (real modules, temp HERMES_HOME, mock plane): merge -> pull -> marker+sidecar -> labeled listing -> exactly-2 collision flags -> load-time header -> edit refused -> marker cleared => org skills vanish, personal survive.
This commit is contained in:
Ben Barclay
+90
-11
@@ -19,13 +19,18 @@ from typing import Optional
|
||||
from agent.runtime_cwd import resolve_agent_cwd
|
||||
from agent.skill_utils import (
|
||||
EXCLUDED_SKILL_DIRS,
|
||||
ORG_ACTIVE_MARKER,
|
||||
ORG_MIRROR_DIR_NAME,
|
||||
ORG_PROVENANCE_FILE,
|
||||
SKILL_SUPPORT_DIRS,
|
||||
extract_skill_conditions,
|
||||
extract_skill_description,
|
||||
get_all_skills_dirs,
|
||||
get_disabled_skill_names,
|
||||
iter_skill_index_files,
|
||||
org_id_of_path,
|
||||
parse_frontmatter,
|
||||
read_active_org_id,
|
||||
skill_matches_environment,
|
||||
skill_matches_platform,
|
||||
skill_matches_platform_list,
|
||||
@@ -1310,7 +1315,9 @@ def drain_truncation_warnings() -> list:
|
||||
_SKILLS_PROMPT_CACHE_MAX = 8
|
||||
_SKILLS_PROMPT_CACHE: OrderedDict[tuple, str] = OrderedDict()
|
||||
_SKILLS_PROMPT_CACHE_LOCK = threading.Lock()
|
||||
_SKILLS_SNAPSHOT_VERSION = 1
|
||||
# v2: entries gained org provenance fields (org_id/org_author/rel_dir) for M2
|
||||
# org-shared skills; older snapshots are discarded and rebuilt.
|
||||
_SKILLS_SNAPSHOT_VERSION = 2
|
||||
|
||||
|
||||
def _skills_prompt_snapshot_path() -> Path:
|
||||
@@ -1329,13 +1336,32 @@ def clear_skills_system_prompt_cache(*, clear_snapshot: bool = False) -> None:
|
||||
|
||||
|
||||
def _build_skills_manifest(skills_dir: Path) -> dict[str, list[int]]:
|
||||
"""Build an mtime/size manifest of all SKILL.md and DESCRIPTION.md files."""
|
||||
"""Build an mtime/size manifest of all SKILL.md and DESCRIPTION.md files.
|
||||
|
||||
Org mirrors (M2): only the ACTIVE org's mirror participates, and the
|
||||
``.active_org`` marker itself is included — so switching/leaving an org
|
||||
invalidates the snapshot even when no SKILL.md changed.
|
||||
"""
|
||||
manifest: dict[str, list[int]] = {}
|
||||
skills_dir_str = str(skills_dir)
|
||||
base = os.path.join(skills_dir_str, "")
|
||||
prefix_len = len(base)
|
||||
active_org = read_active_org_id(skills_dir)
|
||||
org_root = os.path.join(skills_dir_str, ORG_MIRROR_DIR_NAME)
|
||||
marker_path = os.path.join(org_root, ORG_ACTIVE_MARKER)
|
||||
try:
|
||||
st = os.stat(marker_path)
|
||||
manifest[ORG_MIRROR_DIR_NAME + "/" + ORG_ACTIVE_MARKER] = [
|
||||
int(st.st_mtime), int(st.st_size),
|
||||
]
|
||||
except OSError:
|
||||
pass
|
||||
for root, dirs, files in os.walk(skills_dir_str, followlinks=True):
|
||||
has_skill_md = "SKILL.md" in files
|
||||
if root == skills_dir_str and ORG_MIRROR_DIR_NAME in dirs and active_org is None:
|
||||
dirs.remove(ORG_MIRROR_DIR_NAME)
|
||||
elif root == org_root:
|
||||
dirs[:] = [d for d in dirs if d == active_org]
|
||||
dirs[:] = [
|
||||
d
|
||||
for d in dirs
|
||||
@@ -1400,6 +1426,15 @@ def _build_snapshot_entry(
|
||||
"""Build a serialisable metadata dict for one skill."""
|
||||
rel_path = skill_file.relative_to(skills_dir)
|
||||
parts = rel_path.parts
|
||||
|
||||
# M2 org mirror: strip the `_org/<org_id>/` prefix so category/name derive
|
||||
# from the path WITHIN the mirror (same shape the org tree was built
|
||||
# from), and record provenance for labeling + fail-loud collisions.
|
||||
org_id: str | None = None
|
||||
if len(parts) >= 3 and parts[0] == ORG_MIRROR_DIR_NAME:
|
||||
org_id = parts[1]
|
||||
parts = parts[2:]
|
||||
|
||||
if len(parts) >= 2:
|
||||
skill_name = parts[-2]
|
||||
category = "/".join(parts[:-2]) if len(parts) > 2 else parts[0]
|
||||
@@ -1411,7 +1446,7 @@ def _build_snapshot_entry(
|
||||
if isinstance(platforms, str):
|
||||
platforms = [platforms]
|
||||
|
||||
return {
|
||||
entry = {
|
||||
"skill_name": skill_name,
|
||||
"category": category,
|
||||
"frontmatter_name": str(frontmatter.get("name", skill_name)),
|
||||
@@ -1419,6 +1454,22 @@ def _build_snapshot_entry(
|
||||
"platforms": [str(p).strip() for p in platforms if str(p).strip()],
|
||||
"conditions": extract_skill_conditions(frontmatter),
|
||||
}
|
||||
if org_id:
|
||||
entry["org_id"] = org_id
|
||||
# Author from the pull-time provenance sidecar (token-verified at
|
||||
# push by the plane's author_mismatch guard). Best-effort.
|
||||
try:
|
||||
import json as _json
|
||||
|
||||
prov_path = (
|
||||
skills_dir / ORG_MIRROR_DIR_NAME / org_id / ORG_PROVENANCE_FILE
|
||||
)
|
||||
prov = _json.loads(prov_path.read_text(encoding="utf-8"))
|
||||
device = str(prov.get("author_device") or "")
|
||||
entry["org_author"] = device or str(prov.get("author_user_id") or "")
|
||||
except Exception:
|
||||
entry["org_author"] = ""
|
||||
return entry
|
||||
|
||||
|
||||
# =========================================================================
|
||||
@@ -1554,6 +1605,10 @@ def build_skills_system_prompt(
|
||||
|
||||
skills_by_category: dict[str, list[tuple[str, str]]] = {}
|
||||
category_descriptions: dict[str, str] = {}
|
||||
# Unified visible-entry list (both paths) so the org labeling +
|
||||
# fail-loud collision pass below runs identically for snapshot and scan.
|
||||
visible_entries: list[dict] = []
|
||||
skill_entries: list[dict] = []
|
||||
|
||||
if snapshot is not None:
|
||||
# Fast path: use pre-parsed metadata from disk
|
||||
@@ -1561,7 +1616,6 @@ def build_skills_system_prompt(
|
||||
if not isinstance(entry, dict):
|
||||
continue
|
||||
skill_name = entry.get("skill_name") or ""
|
||||
category = entry.get("category") or "general"
|
||||
frontmatter_name = entry.get("frontmatter_name") or skill_name
|
||||
platforms = entry.get("platforms") or []
|
||||
if not skill_matches_platform_list(platforms):
|
||||
@@ -1574,16 +1628,13 @@ def build_skills_system_prompt(
|
||||
available_toolsets,
|
||||
):
|
||||
continue
|
||||
skills_by_category.setdefault(category, []).append(
|
||||
(frontmatter_name, entry.get("description", ""))
|
||||
)
|
||||
visible_entries.append(entry)
|
||||
category_descriptions = {
|
||||
str(k): str(v)
|
||||
for k, v in (snapshot.get("category_descriptions") or {}).items()
|
||||
}
|
||||
else:
|
||||
# Cold path: full filesystem scan + write snapshot for next time
|
||||
skill_entries: list[dict] = []
|
||||
for skill_file in iter_skill_index_files(skills_dir, "SKILL.md"):
|
||||
is_compatible, frontmatter, desc = _parse_skill_file(skill_file)
|
||||
entry = _build_snapshot_entry(skill_file, skills_dir, frontmatter, desc)
|
||||
@@ -1599,10 +1650,38 @@ def build_skills_system_prompt(
|
||||
available_toolsets,
|
||||
):
|
||||
continue
|
||||
skills_by_category.setdefault(entry["category"], []).append(
|
||||
(entry["frontmatter_name"], entry["description"])
|
||||
)
|
||||
visible_entries.append(entry)
|
||||
|
||||
# ── M2 org labeling + FAIL-LOUD collisions ─────────────────────────
|
||||
# An org skill lists with an explicit provenance tag. When a personal and
|
||||
# an org skill share a name, NEITHER silently wins: both list qualified
|
||||
# (personal keeps the bare name is the wrong default — silent divergence
|
||||
# from the org set; org winning silently shadows the user's own work) —
|
||||
# so both entries carry a [name collision] flag and skill_view refuses
|
||||
# the ambiguous bare name (its existing multi-candidate guard).
|
||||
name_owners: dict[str, set[str]] = {}
|
||||
for entry in visible_entries:
|
||||
fm = entry.get("frontmatter_name") or entry.get("skill_name") or ""
|
||||
kind = "org" if entry.get("org_id") else "personal"
|
||||
name_owners.setdefault(fm, set()).add(kind)
|
||||
for entry in visible_entries:
|
||||
fm = entry.get("frontmatter_name") or entry.get("skill_name") or ""
|
||||
desc = entry.get("description", "")
|
||||
org_id = entry.get("org_id")
|
||||
collided = len(name_owners.get(fm, set())) > 1
|
||||
if org_id:
|
||||
author = entry.get("org_author") or ""
|
||||
tag = f"[org-shared{': by ' + author if author else ''}]"
|
||||
desc = f"{tag} {desc}".strip()
|
||||
category = f"org:{org_id}"
|
||||
else:
|
||||
category = entry.get("category") or "general"
|
||||
if collided:
|
||||
desc = f"[name collision — also exists {'personally' if org_id else 'in your org'}; load via category path] {desc}".strip()
|
||||
skills_by_category.setdefault(category, []).append((fm, desc))
|
||||
|
||||
if snapshot is None:
|
||||
# (continuation of the cold path below: category descriptions + write)
|
||||
# Read category-level DESCRIPTION.md files
|
||||
for desc_file in iter_skill_index_files(skills_dir, "DESCRIPTION.md"):
|
||||
try:
|
||||
|
||||
@@ -49,6 +49,52 @@ EXCLUDED_SKILL_DIRS = frozenset(
|
||||
# archive workflow preserves a complete old skill package under references/.
|
||||
SKILL_SUPPORT_DIRS = frozenset(("references", "templates", "assets", "scripts"))
|
||||
|
||||
# ── M2 org-shared skills (hsp-1-contract.md §11) ───────────────────────────
|
||||
# Org mirrors live under ~/.hermes/skills/_org/<org_id>/. Resolution is
|
||||
# TOKEN-GATED via a marker file the sync client writes after verifying the
|
||||
# token (skills_sync_client.pull_org_skills): only the marked org's mirror is
|
||||
# scanned. No marker ⇒ no org skills load. The marker is plain data (org_id
|
||||
# string) so this module stays import-light; the VERIFICATION lives in the
|
||||
# sync client, which is the only writer. Offline grace: the marker persists,
|
||||
# so already-pulled org skills keep working without connectivity; a VERIFIED
|
||||
# org change (or personal-org token) rewrites/removes it.
|
||||
|
||||
ORG_MIRROR_DIR_NAME = "_org"
|
||||
ORG_ACTIVE_MARKER = ".active_org"
|
||||
ORG_PROVENANCE_FILE = ".org-provenance.json"
|
||||
|
||||
|
||||
def read_active_org_id(skills_dir: Path) -> Optional[str]:
|
||||
"""The org id whose mirror may resolve, or None (no org skills load)."""
|
||||
try:
|
||||
marker = skills_dir / ORG_MIRROR_DIR_NAME / ORG_ACTIVE_MARKER
|
||||
if not marker.exists():
|
||||
return None
|
||||
val = marker.read_text(encoding="utf-8").strip()
|
||||
return val or None
|
||||
except OSError:
|
||||
return None
|
||||
|
||||
|
||||
def is_org_mirror_path(path, skills_dir: Path) -> bool:
|
||||
"""True when *path* is inside the org mirror (``_org/``)."""
|
||||
try:
|
||||
rel = Path(path).resolve().relative_to(Path(skills_dir).resolve())
|
||||
except (OSError, ValueError):
|
||||
return False
|
||||
return bool(rel.parts) and rel.parts[0] == ORG_MIRROR_DIR_NAME
|
||||
|
||||
|
||||
def org_id_of_path(path, skills_dir: Path) -> Optional[str]:
|
||||
"""The ``<org_id>`` segment for a path under ``_org/<org_id>/...``."""
|
||||
try:
|
||||
rel = Path(path).resolve().relative_to(Path(skills_dir).resolve())
|
||||
except (OSError, ValueError):
|
||||
return None
|
||||
if len(rel.parts) >= 2 and rel.parts[0] == ORG_MIRROR_DIR_NAME:
|
||||
return rel.parts[1]
|
||||
return None
|
||||
|
||||
|
||||
def is_excluded_skill_path(path) -> bool:
|
||||
"""True if *path* should be skipped by active skill scanners.
|
||||
@@ -802,11 +848,24 @@ def iter_skill_index_files(skills_dir: Path, filename: str):
|
||||
scripts) can contain arbitrary markdown and even archived package
|
||||
``SKILL.md`` files, but they are progressive-disclosure data loaded through
|
||||
``skill_view(..., file_path=...)`` rather than active skill roots.
|
||||
|
||||
M2 org mirrors (``_org/``): TOKEN-GATED resolution. Only the active org's
|
||||
subdir (per the sync-client-written ``.active_org`` marker) is walked;
|
||||
every other ``_org/<id>/`` (stale mirror from a previous org, or no
|
||||
marker at all) is pruned — leave an org and its skills stop resolving,
|
||||
without any manual cleanup.
|
||||
"""
|
||||
skills_dir_str = str(skills_dir)
|
||||
active_org = read_active_org_id(skills_dir)
|
||||
org_root = os.path.join(skills_dir_str, ORG_MIRROR_DIR_NAME)
|
||||
matches: list[str] = []
|
||||
for root, dirs, files in os.walk(skills_dir_str, followlinks=True):
|
||||
has_skill_md = "SKILL.md" in files
|
||||
if root == skills_dir_str and ORG_MIRROR_DIR_NAME in dirs and active_org is None:
|
||||
dirs.remove(ORG_MIRROR_DIR_NAME)
|
||||
elif root == org_root:
|
||||
# Inside _org/: descend ONLY into the active org's mirror.
|
||||
dirs[:] = [d for d in dirs if d == active_org]
|
||||
dirs[:] = [
|
||||
d
|
||||
for d in dirs
|
||||
|
||||
@@ -0,0 +1,176 @@
|
||||
"""M2 org-skill namespace: token-gated resolution, provenance, collisions.
|
||||
|
||||
Covers the design agreed 2026-07-23 (bare-name first-class org skills):
|
||||
1. TOKEN-GATED discovery — only the `.active_org`-marked mirror resolves;
|
||||
stale mirrors and marker-less trees never load.
|
||||
2. Fail-loud collisions — a personal/org name clash lists BOTH sides flagged;
|
||||
skill_view's existing multi-candidate guard refuses the bare name.
|
||||
3. Load-time provenance header — org skill content announces org + author.
|
||||
4. Org mirrors are read-only (skill_manage guards) and curation-exempt.
|
||||
"""
|
||||
|
||||
import json
|
||||
|
||||
import pytest
|
||||
|
||||
from agent import skill_utils as sku
|
||||
from agent.prompt_builder import _build_snapshot_entry
|
||||
|
||||
|
||||
def _mk_skill(root, rel, name=None, body="# body\n"):
|
||||
d = root
|
||||
for part in rel.split("/"):
|
||||
d = d / part
|
||||
d.mkdir(parents=True, exist_ok=True)
|
||||
(d / "SKILL.md").write_text(
|
||||
f"---\nname: {name or rel.split('/')[-1]}\ndescription: d\n---\n{body}",
|
||||
encoding="utf-8",
|
||||
)
|
||||
return d
|
||||
|
||||
|
||||
def _mark_active(skills, org_id):
|
||||
org_root = skills / sku.ORG_MIRROR_DIR_NAME
|
||||
org_root.mkdir(parents=True, exist_ok=True)
|
||||
(org_root / sku.ORG_ACTIVE_MARKER).write_text(org_id, encoding="utf-8")
|
||||
|
||||
|
||||
class TestTokenGatedDiscovery:
|
||||
def test_no_marker_no_org_skills(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
_mk_skill(skills, "personal-a")
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
found = [p.parent.name for p in sku.iter_skill_index_files(skills, "SKILL.md")]
|
||||
assert "personal-a" in found
|
||||
assert "shared-x" not in found # unmarked mirror never resolves
|
||||
|
||||
def test_marker_gates_to_active_org_only(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-OLD/stale-y", name="stale-y")
|
||||
_mark_active(skills, "org-1")
|
||||
found = [p.parent.name for p in sku.iter_skill_index_files(skills, "SKILL.md")]
|
||||
assert "shared-x" in found
|
||||
assert "stale-y" not in found # stale mirror pruned at resolution
|
||||
|
||||
def test_switching_org_flips_resolution(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-2/other-z", name="other-z")
|
||||
_mark_active(skills, "org-2")
|
||||
found = [p.parent.name for p in sku.iter_skill_index_files(skills, "SKILL.md")]
|
||||
assert found and "other-z" in found and "shared-x" not in found
|
||||
|
||||
def test_helpers(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
d = _mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-9/cat/sk", name="sk")
|
||||
assert sku.is_org_mirror_path(d, skills) is True
|
||||
assert sku.org_id_of_path(d, skills) == "org-9"
|
||||
p = _mk_skill(skills, "plain")
|
||||
assert sku.is_org_mirror_path(p, skills) is False
|
||||
assert sku.read_active_org_id(skills) is None
|
||||
_mark_active(skills, "org-9")
|
||||
assert sku.read_active_org_id(skills) == "org-9"
|
||||
|
||||
|
||||
class TestSnapshotEntryProvenance:
|
||||
def test_org_entry_strips_prefix_and_carries_provenance(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
d = _mk_skill(
|
||||
skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/devops/beta", name="beta"
|
||||
)
|
||||
(skills / sku.ORG_MIRROR_DIR_NAME / "org-1" / sku.ORG_PROVENANCE_FILE).write_text(
|
||||
json.dumps(
|
||||
{"author_device": "bens-macbook-a1b2c3", "author_user_id": "u1"}
|
||||
),
|
||||
encoding="utf-8",
|
||||
)
|
||||
entry = _build_snapshot_entry(d / "SKILL.md", skills, {"name": "beta"}, "d")
|
||||
assert entry["org_id"] == "org-1"
|
||||
assert entry["org_author"] == "bens-macbook-a1b2c3"
|
||||
# Category derives from the path WITHIN the mirror, not _org/org-1/...
|
||||
assert entry["category"] == "devops"
|
||||
assert entry["skill_name"] == "beta"
|
||||
|
||||
def test_personal_entry_unchanged(self, tmp_path):
|
||||
skills = tmp_path / "skills"
|
||||
d = _mk_skill(skills, "devops/beta", name="beta")
|
||||
entry = _build_snapshot_entry(d / "SKILL.md", skills, {"name": "beta"}, "d")
|
||||
assert "org_id" not in entry
|
||||
assert entry["category"] == "devops"
|
||||
|
||||
|
||||
class TestListingCollisionsAndLabels:
|
||||
def _render(self, tmp_path, monkeypatch):
|
||||
from agent import prompt_builder as pb
|
||||
|
||||
skills = tmp_path / "skills"
|
||||
skills.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setattr(pb, "get_skills_dir", lambda: skills, raising=True)
|
||||
monkeypatch.setattr(
|
||||
pb, "get_all_skills_dirs", lambda: [skills], raising=True
|
||||
)
|
||||
monkeypatch.setattr(pb, "get_disabled_skill_names", lambda *a, **k: set())
|
||||
monkeypatch.setattr(
|
||||
pb, "_skills_prompt_snapshot_path", lambda: tmp_path / "snap.json"
|
||||
)
|
||||
pb.clear_skills_system_prompt_cache()
|
||||
return skills, pb
|
||||
|
||||
def test_org_skill_listed_with_provenance_tag(self, tmp_path, monkeypatch):
|
||||
skills, pb = self._render(tmp_path, monkeypatch)
|
||||
_mk_skill(skills, "personal-a")
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
(skills / sku.ORG_MIRROR_DIR_NAME / "org-1" / sku.ORG_PROVENANCE_FILE).write_text(
|
||||
json.dumps({"author_device": "bens-macbook"}), encoding="utf-8"
|
||||
)
|
||||
_mark_active(skills, "org-1")
|
||||
out = pb.build_skills_system_prompt()
|
||||
assert "org:org-1" in out
|
||||
assert "[org-shared: by bens-macbook]" in out
|
||||
assert "personal-a" in out
|
||||
|
||||
def test_collision_flags_both_sides(self, tmp_path, monkeypatch):
|
||||
skills, pb = self._render(tmp_path, monkeypatch)
|
||||
_mk_skill(skills, "k8s-debug", body="personal version\n")
|
||||
_mk_skill(
|
||||
skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/k8s-debug", name="k8s-debug"
|
||||
)
|
||||
_mark_active(skills, "org-1")
|
||||
out = pb.build_skills_system_prompt()
|
||||
# BOTH entries flagged — neither silently wins.
|
||||
assert out.count("[name collision") == 2
|
||||
|
||||
def test_no_collision_flag_when_unique(self, tmp_path, monkeypatch):
|
||||
skills, pb = self._render(tmp_path, monkeypatch)
|
||||
_mk_skill(skills, "personal-a")
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
_mark_active(skills, "org-1")
|
||||
out = pb.build_skills_system_prompt()
|
||||
assert "[name collision" not in out
|
||||
|
||||
|
||||
class TestOrgMirrorReadOnly:
|
||||
def test_skill_manage_patch_refuses_org_mirror(self, tmp_path, monkeypatch):
|
||||
from tools import skill_manager_tool as smt
|
||||
|
||||
skills = tmp_path / "skills"
|
||||
_mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
_mark_active(skills, "org-1")
|
||||
monkeypatch.setattr(smt, "_skills_dir", lambda: skills)
|
||||
from agent import skill_utils as _sku
|
||||
monkeypatch.setattr(
|
||||
_sku, "get_all_skills_dirs", lambda: [skills], raising=True
|
||||
)
|
||||
result = smt._patch_skill("shared-x", "body", "hacked")
|
||||
assert result["success"] is False
|
||||
assert "ORG-SHARED" in result["error"]
|
||||
assert "propose" in result["error"]
|
||||
|
||||
def test_curation_exempt(self, tmp_path, monkeypatch):
|
||||
from tools import skill_usage as su
|
||||
|
||||
skills = tmp_path / "skills"
|
||||
d = _mk_skill(skills, f"{sku.ORG_MIRROR_DIR_NAME}/org-1/shared-x", name="shared-x")
|
||||
monkeypatch.setattr(su, "_skills_dir", lambda: skills)
|
||||
assert su.is_curation_eligible("shared-x", d) is False
|
||||
@@ -622,6 +622,34 @@ def _find_skill(name: str) -> Optional[Dict[str, Any]]:
|
||||
return None
|
||||
|
||||
|
||||
def _org_mirror_write_guard(name: str, skill_path: Path, action: str) -> Optional[Dict[str, Any]]:
|
||||
"""Refuse writes to org-mirror skills (M2, contract §11.11 / design §7.1).
|
||||
|
||||
The ``_org/`` mirror is materialized FROM the org HEAD and overwritten on
|
||||
every pull — a local edit would be silently lost AND would misrepresent
|
||||
admin-approved shared content. The change path is: fork into a personal
|
||||
skill, edit, then ``hermes skills propose``.
|
||||
"""
|
||||
try:
|
||||
from agent.skill_utils import is_org_mirror_path
|
||||
|
||||
if is_org_mirror_path(skill_path, _skills_dir()):
|
||||
return {
|
||||
"success": False,
|
||||
"error": (
|
||||
f"Refusing {action} for '{name}': it is an ORG-SHARED "
|
||||
"skill (read-only mirror of your org's approved set; "
|
||||
"local edits are overwritten on every org pull). To "
|
||||
"change it: copy it to a personal skill, edit that, then "
|
||||
"`hermes skills propose <name>` so an org admin can "
|
||||
"review and approve."
|
||||
),
|
||||
}
|
||||
except Exception:
|
||||
logger.debug("org mirror guard lookup failed for %s", name, exc_info=True)
|
||||
return None
|
||||
|
||||
|
||||
def _find_skill_in_other_profiles(name: str) -> List[Tuple[str, Path]]:
|
||||
"""Look for ``name`` under SKILL.md across OTHER Hermes profiles.
|
||||
|
||||
@@ -891,6 +919,9 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]:
|
||||
existing = _find_skill(name)
|
||||
if not existing:
|
||||
return {"success": False, "error": _skill_not_found_error(name)}
|
||||
org_guard = _org_mirror_write_guard(name, existing["path"], "edit")
|
||||
if org_guard:
|
||||
return org_guard
|
||||
guard = _background_review_write_guard(name, existing["path"], "edit")
|
||||
if guard:
|
||||
return guard
|
||||
@@ -953,6 +984,9 @@ def _patch_skill(
|
||||
return {"success": False, "error": _skill_not_found_error(name)}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
org_guard = _org_mirror_write_guard(name, skill_dir, "patch")
|
||||
if org_guard:
|
||||
return org_guard
|
||||
guard = _background_review_write_guard(name, skill_dir, "patch")
|
||||
if guard:
|
||||
return guard
|
||||
@@ -1059,6 +1093,9 @@ def _delete_skill(name: str, absorbed_into: Optional[str] = None) -> Dict[str, A
|
||||
existing = _find_skill(name)
|
||||
if not existing:
|
||||
return {"success": False, "error": _skill_not_found_error(name)}
|
||||
org_guard = _org_mirror_write_guard(name, existing["path"], "delete")
|
||||
if org_guard:
|
||||
return org_guard
|
||||
guard = _background_review_write_guard(name, existing["path"], "delete")
|
||||
if guard:
|
||||
return guard
|
||||
@@ -1176,6 +1213,9 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]:
|
||||
existing = _find_skill(name)
|
||||
if not existing:
|
||||
return {"success": False, "error": _skill_not_found_error(name, " Create it first with action='create'.")}
|
||||
org_guard = _org_mirror_write_guard(name, existing["path"], "write_file")
|
||||
if org_guard:
|
||||
return org_guard
|
||||
guard = _background_review_write_guard(name, existing["path"], "write_file")
|
||||
if guard:
|
||||
return guard
|
||||
|
||||
+14
-4
@@ -450,12 +450,18 @@ def is_curation_eligible(skill_name: str, skill_path: Optional[Path] = None) ->
|
||||
Agent-created skills are always eligible. Bundled built-ins become eligible
|
||||
only when ``curator.prune_builtins`` is enabled. Hub-installed and external
|
||||
skill-dir skills are NEVER eligible — they have an external upstream owner.
|
||||
Org-mirror skills (``_org/``) are NEVER eligible — the org HEAD owns them;
|
||||
curation happens via propose → approve, not local archive/consolidate.
|
||||
Protected built-ins (``PROTECTED_BUILTIN_SKILLS``) are NEVER eligible
|
||||
regardless of any flag — they back load-bearing UX and must never be
|
||||
archived or consolidated.
|
||||
"""
|
||||
from agent.skill_utils import is_org_mirror_path
|
||||
|
||||
if skill_path is not None and is_external_skill_path(skill_path):
|
||||
return False
|
||||
if skill_path is not None and is_org_mirror_path(skill_path, _skills_dir()):
|
||||
return False
|
||||
if is_protected_builtin(skill_name):
|
||||
return False
|
||||
if is_hub_installed(skill_name):
|
||||
@@ -464,6 +470,8 @@ def is_curation_eligible(skill_name: str, skill_path: Optional[Path] = None) ->
|
||||
return _prune_builtins_enabled()
|
||||
local_dir = _find_skill_dir(skill_name)
|
||||
if local_dir is not None:
|
||||
if is_org_mirror_path(local_dir, _skills_dir()):
|
||||
return False
|
||||
return not is_external_skill_path(local_dir)
|
||||
if _find_external_skill_dir(skill_name) is not None:
|
||||
return False
|
||||
@@ -853,14 +861,16 @@ def _find_skill_dir(skill_name: str) -> Optional[Path]:
|
||||
"""Locate the directory for a skill by its frontmatter `name:` field.
|
||||
|
||||
Handles both flat (~/.hermes/skills/<skill>/SKILL.md) and category-nested
|
||||
(~/.hermes/skills/<category>/<skill>/SKILL.md) layouts.
|
||||
(~/.hermes/skills/<category>/<skill>/SKILL.md) layouts. Uses the gated
|
||||
index iterator so M2 org mirrors resolve ONLY for the active org
|
||||
(stale ``_org/<other>/`` trees never match).
|
||||
"""
|
||||
base = _skills_dir()
|
||||
if not base.exists():
|
||||
return None
|
||||
for skill_md in base.rglob("SKILL.md"):
|
||||
if is_excluded_skill_path(skill_md):
|
||||
continue
|
||||
from agent.skill_utils import iter_skill_index_files
|
||||
|
||||
for skill_md in iter_skill_index_files(base, "SKILL.md"):
|
||||
if is_external_skill_path(skill_md):
|
||||
continue
|
||||
if _read_skill_name(skill_md, fallback=skill_md.parent.name) == skill_name:
|
||||
|
||||
@@ -1671,10 +1671,17 @@ def pull_org_skills(
|
||||
head = next(
|
||||
(r["hash"] for r in refs if r.get("name") == org_head_ref(org_id)), None
|
||||
)
|
||||
# TOKEN-GATED resolution marker (agent/skill_utils.read_active_org_id):
|
||||
# written HERE because this function only runs after resolve_org_identity
|
||||
# verified the token's org_id + org_role. Discovery scans only the marked
|
||||
# org's mirror, so a stale mirror from a previous org stops resolving the
|
||||
# moment a pull runs under a different org — no manual cleanup.
|
||||
_write_active_org_marker(org_id)
|
||||
if not head:
|
||||
return {"ok": True, "org_id": org_id, "head": None, "updated": []}
|
||||
|
||||
root_tree = _root_tree_of_commit(client, head)
|
||||
head_commit = client.get_commit_json(head)
|
||||
root_tree = head_commit["tree"]
|
||||
skill_trees = _skill_trees_of_root(client, root_tree)
|
||||
|
||||
dest_root = _org_dir() / org_id
|
||||
@@ -1695,9 +1702,49 @@ def pull_org_skills(
|
||||
rel_path,
|
||||
e,
|
||||
)
|
||||
# Provenance sidecar for the load-time header (skill_view): the HEAD
|
||||
# commit's author is TOKEN-VERIFIED at push time by the plane
|
||||
# (author_mismatch guard, gateway-gateway #166) — trustworthy to display.
|
||||
_write_org_provenance(
|
||||
org_id,
|
||||
{
|
||||
"org_id": org_id,
|
||||
"head": head,
|
||||
"author_user_id": (head_commit.get("author") or {}).get("owner", ""),
|
||||
"author_device": (head_commit.get("author") or {}).get("device", ""),
|
||||
"ts": head_commit.get("ts", ""),
|
||||
"skills": updated,
|
||||
},
|
||||
)
|
||||
return {"ok": True, "org_id": org_id, "head": head, "updated": updated}
|
||||
|
||||
|
||||
def _write_active_org_marker(org_id: str) -> None:
|
||||
"""Record which org's mirror may resolve (best-effort, never raises)."""
|
||||
try:
|
||||
from agent.skill_utils import ORG_ACTIVE_MARKER
|
||||
|
||||
root = _org_dir()
|
||||
root.mkdir(parents=True, exist_ok=True)
|
||||
(root / ORG_ACTIVE_MARKER).write_text(org_id, encoding="utf-8")
|
||||
except Exception as e:
|
||||
logger.debug("skills_sync_client: active-org marker write failed: %s", e)
|
||||
|
||||
|
||||
def _write_org_provenance(org_id: str, data: Dict[str, Any]) -> None:
|
||||
"""Persist the org HEAD provenance sidecar (best-effort, never raises)."""
|
||||
try:
|
||||
from agent.skill_utils import ORG_PROVENANCE_FILE
|
||||
|
||||
dest = _org_dir() / org_id
|
||||
dest.mkdir(parents=True, exist_ok=True)
|
||||
(dest / ORG_PROVENANCE_FILE).write_text(
|
||||
json.dumps(data, indent=2), encoding="utf-8"
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug("skills_sync_client: org provenance write failed: %s", e)
|
||||
|
||||
|
||||
def propose_skill(
|
||||
skill_name: str,
|
||||
client: Optional["HSPClient"] = None,
|
||||
@@ -1797,9 +1844,31 @@ def maybe_pull_org_skills() -> Optional[Dict[str, Any]]:
|
||||
org), feature enabled, base URL configured. Personal orgs are inert here
|
||||
by construction — resolve_org_identity raises SyncInertError without the
|
||||
claim.
|
||||
|
||||
Marker hygiene: when the token VERIFIABLY lacks the org claim (logged in,
|
||||
personal org / left the org), the active-org marker is cleared so
|
||||
previously-mirrored org skills stop resolving. When we simply cannot
|
||||
resolve identity (offline, logged out), the marker is left alone —
|
||||
offline grace keeps already-pulled org skills working.
|
||||
"""
|
||||
try:
|
||||
identity = resolve_org_identity()
|
||||
except SyncInertError:
|
||||
# Distinguish "verifiably personal/left-org" from "can't tell".
|
||||
try:
|
||||
base_identity = resolve_identity()
|
||||
claims = base_identity.get("claims") or {}
|
||||
if not claims.get("org_role"):
|
||||
_clear_active_org_marker()
|
||||
except Exception:
|
||||
pass # offline/logged out — keep offline grace
|
||||
return None
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"skills_sync_client: maybe_pull_org_skills inert/failed: %s", e
|
||||
)
|
||||
return None
|
||||
try:
|
||||
if not sync_feature_enabled():
|
||||
return None
|
||||
if not resolve_sync_base_url():
|
||||
@@ -1810,3 +1879,19 @@ def maybe_pull_org_skills() -> Optional[Dict[str, Any]]:
|
||||
"skills_sync_client: maybe_pull_org_skills inert/failed: %s", e
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
def _clear_active_org_marker() -> None:
|
||||
"""Remove the active-org marker (org skills stop resolving)."""
|
||||
try:
|
||||
from agent.skill_utils import ORG_ACTIVE_MARKER
|
||||
|
||||
marker = _org_dir() / ORG_ACTIVE_MARKER
|
||||
if marker.exists():
|
||||
marker.unlink()
|
||||
logger.info(
|
||||
"skills_sync_client: cleared active-org marker "
|
||||
"(token has no org workflow); org skills no longer resolve"
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug("skills_sync_client: marker clear failed: %s", e)
|
||||
|
||||
@@ -1561,6 +1561,69 @@ def skill_view(
|
||||
"Could not preprocess skill content for %s", skill_name, exc_info=True
|
||||
)
|
||||
|
||||
# ── M2 org provenance header (load-time) ──────────────────────────
|
||||
# An org-shared skill announces its provenance IN the returned content
|
||||
# — the moment the model consumes it — not only in the listing. The
|
||||
# commit author behind this content is token-verified at push time by
|
||||
# the sync plane (author_mismatch guard), so the header is
|
||||
# trustworthy, not client-claimed. Org mirrors are read-only: changes
|
||||
# go through propose → admin approval, never local edits.
|
||||
org_provenance = None
|
||||
if skill_dir:
|
||||
try:
|
||||
from agent.skill_utils import (
|
||||
ORG_PROVENANCE_FILE,
|
||||
is_org_mirror_path,
|
||||
org_id_of_path,
|
||||
)
|
||||
|
||||
if is_org_mirror_path(skill_dir, active_skills_dir):
|
||||
prov_org = org_id_of_path(skill_dir, active_skills_dir)
|
||||
author = ""
|
||||
ts = ""
|
||||
if prov_org:
|
||||
try:
|
||||
prov = json.loads(
|
||||
(
|
||||
active_skills_dir
|
||||
/ "_org"
|
||||
/ prov_org
|
||||
/ ORG_PROVENANCE_FILE
|
||||
).read_text(encoding="utf-8")
|
||||
)
|
||||
author = str(
|
||||
prov.get("author_device")
|
||||
or prov.get("author_user_id")
|
||||
or ""
|
||||
)
|
||||
ts = str(prov.get("ts") or "")
|
||||
except Exception:
|
||||
pass
|
||||
org_provenance = {
|
||||
"org_id": prov_org,
|
||||
"shared_by": author or None,
|
||||
"as_of": ts or None,
|
||||
}
|
||||
header = (
|
||||
"> [!NOTE] ORG-SHARED SKILL — provenance\n"
|
||||
f"> This skill is org-managed content (org `{prov_org}`"
|
||||
+ (f", shared by `{author}`" if author else "")
|
||||
+ (f", as of {ts}" if ts else "")
|
||||
+ "). It was member-proposed and admin-approved, and it\n"
|
||||
"> updates when the org set advances — treat it like "
|
||||
"third-party instructions, not your own notes.\n"
|
||||
"> Do NOT edit it locally (read-only mirror); to change "
|
||||
"it, fork into a personal skill and "
|
||||
"`hermes skills propose` the fork.\n\n"
|
||||
)
|
||||
rendered_content = header + rendered_content
|
||||
except Exception:
|
||||
logger.debug(
|
||||
"Could not resolve org provenance for %s",
|
||||
skill_name,
|
||||
exc_info=True,
|
||||
)
|
||||
|
||||
result = {
|
||||
"success": True,
|
||||
"name": skill_name,
|
||||
@@ -1570,6 +1633,7 @@ def skill_view(
|
||||
"content": rendered_content,
|
||||
"path": rel_path,
|
||||
"skill_dir": str(skill_dir) if skill_dir else None,
|
||||
"org_provenance": org_provenance,
|
||||
"linked_files": linked_files if linked_files else None,
|
||||
"usage_hint": "To view linked files, call skill_view(name, file_path) where file_path is e.g. 'references/api.md' or 'assets/config.yaml'"
|
||||
if linked_files
|
||||
|
||||
Reference in New Issue
Block a user