diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 347366e6a6ff..a78804be724f 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -483,7 +483,14 @@ def test_collision_prints_reset_hint(self, tmp_path, capsys): assert "hermes skills reset new-skill" in captured def test_nonexistent_bundled_dir(self, tmp_path): - with patch("tools.skills_sync._get_bundled_dir", return_value=tmp_path / "nope"): + # Patch SKILLS_DIR to a clean tmp path so the marker check inside + # sync_skills() doesn't pick up the developer's live HERMES_HOME + # opt-out marker and short-circuit before the missing-bundled-dir + # path runs. + skills_dir = tmp_path / "profile" / "skills" + skills_dir.mkdir(parents=True) + with patch("tools.skills_sync._get_bundled_dir", return_value=tmp_path / "nope"), \ + patch("tools.skills_sync.SKILLS_DIR", skills_dir): result = sync_skills(quiet=True) assert result == { "copied": [], "updated": [], "skipped": 0, @@ -732,3 +739,63 @@ def test_reset_no_op_when_already_clean(self, tmp_path): post_manifest = _read_manifest() assert "google-workspace" in post_manifest assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists() + + +class TestNoBundledSkillsMarker: + """Regression: sync_skills must honor the .no-bundled-skills opt-out marker. + + PR #15 added the marker check at every documented caller but the + DESCRIPTION.md copy loop inside sync_skills() still ran unconditionally + when sync_skills() was called directly (e.g. seed_profile_skills() on a + profile created before the marker was written). That left stale category + DESCRIPTION.md stubs in opted-out profile skill dirs. + """ + + def _setup_bundled(self, tmp_path): + bundled = tmp_path / "bundled_skills" + (bundled / "apple").mkdir(parents=True) + (bundled / "apple" / "DESCRIPTION.md").write_text( + "---\ndescription: Apple\n---\n" + ) + (bundled / "apple" / "imessage").mkdir() + (bundled / "apple" / "imessage" / "SKILL.md").write_text( + "---\nname: imessage\n---\n# iMessage\n" + ) + return bundled + + def test_marker_skips_skill_and_description_copy(self, tmp_path): + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "profile" / "skills" + skills_dir.mkdir(parents=True) + manifest_file = skills_dir / ".bundled_manifest" + # Marker lives at HERMES_HOME root (skills_dir.parent in this test). + from hermes_cli.profiles import NO_BUNDLED_SKILLS_MARKER + (skills_dir.parent / NO_BUNDLED_SKILLS_MARKER).write_text("opt-out\n") + + with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \ + patch("tools.skills_sync.SKILLS_DIR", skills_dir), \ + patch("tools.skills_sync.MANIFEST_FILE", manifest_file): + result = sync_skills(quiet=True) + + assert result.get("skipped_opt_out") is True + assert result["copied"] == [] + # The bug we're guarding: no DESCRIPTION.md must land in the profile. + assert not (skills_dir / "apple" / "DESCRIPTION.md").exists() + # And no SKILL.md either. + assert not (skills_dir / "apple" / "imessage" / "SKILL.md").exists() + + def test_no_marker_proceeds_normally(self, tmp_path): + """Sanity check: without the marker sync still copies as before.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "profile" / "skills" + skills_dir.mkdir(parents=True) + manifest_file = skills_dir / ".bundled_manifest" + + with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \ + patch("tools.skills_sync.SKILLS_DIR", skills_dir), \ + patch("tools.skills_sync.MANIFEST_FILE", manifest_file): + result = sync_skills(quiet=True) + + assert result.get("skipped_opt_out") is not True + assert (skills_dir / "apple" / "DESCRIPTION.md").exists() + assert (skills_dir / "apple" / "imessage" / "SKILL.md").exists() diff --git a/tools/skills_sync.py b/tools/skills_sync.py index fb95898f84db..5fd67e9930e8 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -176,10 +176,39 @@ def sync_skills(quiet: bool = False) -> dict: """ Sync bundled skills into ~/.hermes/skills/ using the manifest. + Honors the ``.no-bundled-skills`` opt-out marker at ``HERMES_HOME``: when + present, the entire sync (SKILL.md *and* category DESCRIPTION.md files) + is skipped. This is defense-in-depth — every documented caller (gateway + startup, ``hermes update``, ``seed_profile_skills``) is supposed to check + the marker before calling us, but historically the DESCRIPTION.md + loop below ran unconditionally even when the SKILL.md path was + correctly bypassed at the caller, leaving stale category stubs in + opted-out profile skills/ dirs (see PR #15 follow-up). + Returns: dict with keys: copied (list), updated (list), skipped (int), - user_modified (list), cleaned (list), total_bundled (int) + user_modified (list), cleaned (list), total_bundled (int). + When the opt-out marker is set, returns a zero-effect dict with + ``skipped_opt_out=True`` so callers can distinguish "skipped due to + marker" from "nothing to do". """ + # Defense-in-depth marker check. SKILLS_DIR's parent is HERMES_HOME by + # construction (module-level constant), so resolving the marker relative + # to it works regardless of how callers patch globals in tests. + try: + from hermes_cli.profiles import NO_BUNDLED_SKILLS_MARKER + marker = SKILLS_DIR.parent / NO_BUNDLED_SKILLS_MARKER + if marker.exists(): + return { + "copied": [], "updated": [], "skipped": 0, + "user_modified": [], "cleaned": [], "total_bundled": 0, + "skipped_opt_out": True, + } + except Exception: + # Best-effort — if the import fails (circular import edge cases, + # partial installs) fall through to the normal sync path. + pass + bundled_dir = _get_bundled_dir() if not bundled_dir.exists(): return {