diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 062284a46049..9332dd43859c 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -700,6 +700,71 @@ class TestSyncSkills: assert "old-skill" not in result.get("user_modified", []) assert result["skipped"] >= 1 + def test_unchanged_skill_does_not_hash_user_copy(self, tmp_path): + """An unchanged bundled origin must not read the bind-mounted copy.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + user_skill = skills_dir / "old-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("# Old") + origin_hash = _dir_hash(bundled / "old-skill") + manifest_file.write_text(f"old-skill:{origin_hash}\n") + real_dir_hash = _dir_hash + + def reject_user_hash(directory): + if directory == user_skill: + pytest.fail("unchanged sync read the user skill tree") + return real_dir_hash(directory) + + with self._patches(bundled, skills_dir, manifest_file), \ + patch("tools.skills_sync._index_active_skills", return_value={}), \ + patch("tools.skills_sync._dir_hash", side_effect=reject_user_hash): + result = sync_skills(quiet=True) + + assert result["skipped"] >= 1 + + def test_unchanged_skills_do_not_build_rename_index(self, tmp_path): + """Rename recovery should not scan the active tree when every dest exists.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + user_skill = skills_dir / "old-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("# Old") + origin_hash = _dir_hash(bundled / "old-skill") + manifest_file.write_text(f"old-skill:{origin_hash}\n") + + with self._patches(bundled, skills_dir, manifest_file), \ + patch( + "tools.skills_sync._index_active_skills", + side_effect=AssertionError("rename index scanned eagerly"), + ): + result = sync_skills(quiet=True) + + assert result["skipped"] >= 1 + + def test_fast_path_defers_user_hash_until_bundled_update(self, tmp_path): + """Local edits remain protected when a later bundled update arrives.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + user_skill = skills_dir / "old-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("# Old") + origin_hash = _dir_hash(bundled / "old-skill") + manifest_file.write_text(f"old-skill:{origin_hash}\n") + (user_skill / "SKILL.md").write_text("# My local edit") + + with self._patches(bundled, skills_dir, manifest_file): + unchanged = sync_skills(quiet=True) + (bundled / "old-skill" / "SKILL.md").write_text("# Upstream v2") + changed = sync_skills(quiet=True) + + assert "old-skill" not in unchanged["user_modified"] + assert "old-skill" in changed["user_modified"] + assert (user_skill / "SKILL.md").read_text() == "# My local edit" + def test_v1_manifest_migration_sets_baseline(self, tmp_path): """v1 manifest entries (no hash) should set baseline from user's current copy.""" bundled = self._setup_bundled(tmp_path) @@ -948,6 +1013,68 @@ class TestSyncSkills: assert entry["source"] == "official" assert entry["install_path"] == "mlops/chroma" + def test_optional_backfill_scans_active_tree_once(self, tmp_path): + """Missing optional candidates must share one active-tree index.""" + bundled = self._setup_bundled(tmp_path) + optional = tmp_path / "optional-skills" + for name in ("optional-a", "optional-b", "optional-c"): + skill = optional / "category" / name + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text(f"---\nname: {name}\n---\n") + + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + real_rglob = Path.rglob + active_tree_scans = 0 + + def count_active_tree_scans(path, pattern): + nonlocal active_tree_scans + if path == skills_dir: + active_tree_scans += 1 + return real_rglob(path, pattern) + + with self._patches(bundled, skills_dir, manifest_file), \ + patch("tools.skills_sync._get_optional_dir", return_value=optional), \ + patch.object(Path, "rglob", count_active_tree_scans): + sync_skills(quiet=True) + + assert active_tree_scans <= 1 + + def test_optional_backfill_skips_already_tracked_skill_before_hashing(self, tmp_path): + """Existing hub provenance must bypass bind-mounted content hashing.""" + bundled = self._setup_bundled(tmp_path) + optional = tmp_path / "optional-skills" + optional_skill = optional / "category" / "tracked-skill" + optional_skill.mkdir(parents=True) + (optional_skill / "SKILL.md").write_text("# tracked\n") + + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + active = skills_dir / "category" / "tracked-skill" + active.mkdir(parents=True) + (active / "SKILL.md").write_text("# tracked\n") + lock_path = skills_dir / ".hub" / "lock.json" + lock_path.parent.mkdir(parents=True) + lock_path.write_text(json.dumps({ + "version": 1, + "installed": { + "tracked-skill": {"install_path": "category/tracked-skill"}, + }, + })) + real_dir_hash = _dir_hash + + def reject_active_hash(directory): + if directory == active: + pytest.fail("tracked optional skill was hashed again") + return real_dir_hash(directory) + + with self._patches(bundled, skills_dir, manifest_file), \ + patch("tools.skills_sync._get_optional_dir", return_value=optional), \ + patch("tools.skills_sync._dir_hash", side_effect=reject_active_hash): + result = sync_skills(quiet=True) + + assert result["optional_provenance_backfilled"] == [] + def test_relocated_backfill_still_requires_identical_content(self, tmp_path): """The name fallback must not weaken the content check. diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 288aa2dac51e..0a8106d6690b 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -12,9 +12,10 @@ Old v1 manifests (plain names without hashes) are auto-migrated. Update logic: - NEW skills (not in manifest): copied to user dir, origin hash recorded. - EXISTING skills (in manifest, present in user dir): - * If user copy matches origin hash: user hasn't modified it → safe to - update from bundled if bundled changed. New origin hash recorded. - * If user copy differs from origin hash: user customized it → SKIP. + * If bundled still matches origin hash: no update → skip without reading + the user copy. + * If bundled changed and user copy matches origin hash: safe to update. + * If bundled changed and user copy differs: user customized it → SKIP. - DELETED by user (in manifest, absent from user dir): respected, not re-added. - REMOVED from bundled (in manifest, gone from repo): cleaned from manifest. @@ -410,7 +411,28 @@ def restore_official_optional_skill(name: str, *, restore: bool = False) -> dict } -def _find_installed_skill_dir_by_name(skill_dir_name: str) -> Optional[Path]: +def _index_installed_skill_dirs_by_name() -> Dict[str, List[Path]]: + """Index installed skills by directory name with one active-tree scan.""" + index: Dict[str, List[Path]] = {} + if not SKILLS_DIR.exists(): + return index + for skill_md in SKILLS_DIR.rglob("SKILL.md"): + if is_excluded_skill_path(skill_md): + continue + candidate = skill_md.parent + # Never reach outside the skills tree (symlinked/external dirs). + try: + candidate.resolve().relative_to(SKILLS_DIR.resolve()) + except (OSError, ValueError): + continue + index.setdefault(candidate.name, []).append(candidate) + return index + + +def _find_installed_skill_dir_by_name( + skill_dir_name: str, + installed_index: Optional[Dict[str, List[Path]]] = None, +) -> Optional[Path]: """Locate an installed skill directory by its directory name. Used only as a fallback when the repo-derived install path doesn't exist in @@ -422,19 +444,9 @@ def _find_installed_skill_dir_by_name(skill_dir_name: str) -> Optional[Path]: """ if not skill_dir_name or not SKILLS_DIR.exists(): return None - matches: List[Path] = [] - for skill_md in SKILLS_DIR.rglob("SKILL.md"): - if is_excluded_skill_path(skill_md): - continue - candidate = skill_md.parent - if candidate.name != skill_dir_name: - continue - # Never reach outside the skills tree (symlinked/external dirs). - try: - candidate.resolve().relative_to(SKILLS_DIR.resolve()) - except (OSError, ValueError): - continue - matches.append(candidate) + if installed_index is None: + installed_index = _index_installed_skill_dirs_by_name() + matches = installed_index.get(skill_dir_name, []) if len(matches) != 1: return None return matches[0] @@ -467,6 +479,7 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: backfilled: List[str] = [] changed = False + installed_dir_index: Optional[Dict[str, List[Path]]] = None for skill_md in sorted(optional_dir.rglob("SKILL.md")): if is_excluded_skill_path(skill_md): continue @@ -476,6 +489,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: except ValueError as e: logger.debug("Skipping optional skill with unsafe path %s: %s", src, e) continue + lock_name = src.name + if lock_name in installed or install_path in existing_paths: + continue dest = SKILLS_DIR / Path(*install_path.split("/")) if not dest.exists() or not dest.is_dir(): # The active tree may hold the same skill under a DIFFERENT @@ -486,7 +502,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: # silently skips them forever. Fall back to a unique # same-directory-name match anywhere in the tree, then still # require a byte-identical hash below before claiming provenance. - dest = _find_installed_skill_dir_by_name(src.name) + if installed_dir_index is None: + installed_dir_index = _index_installed_skill_dirs_by_name() + dest = _find_installed_skill_dir_by_name(src.name, installed_dir_index) if dest is None: continue try: @@ -494,11 +512,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: except ValueError as e: logger.debug("Skipping relocated optional skill %s: %s", dest, e) continue - if _dir_hash(dest) != _dir_hash(src): + if install_path in existing_paths: continue - - lock_name = src.name - if lock_name in installed or install_path in existing_paths: + if _dir_hash(dest) != _dir_hash(src): continue timestamp = datetime.now(timezone.utc).isoformat() @@ -694,9 +710,10 @@ def sync_skills(quiet: bool = False) -> dict: # Index of skills already provided by external_dirs (skip writing them) external_index = _build_external_skill_index() shadowed_by_external: List[str] = [] - # Rename recovery indexes, built once per sync (see _recover_renamed_skill). - active_index = _index_active_skills() - hub_paths = _read_hub_install_paths() + # Rename recovery indexes are expensive on host bind mounts. Build them + # only if a tracked skill is actually missing from its canonical path. + active_index: Optional[Dict[str, List[Path]]] = None + hub_paths: Optional[Set[str]] = None copied = [] updated = [] @@ -742,12 +759,15 @@ def sync_skills(quiet: bool = False) -> dict: # "in manifest but not on disk" branch below misreads the skill as # user-deleted, stranding the old copy at its stale path forever. if not dest.exists() and skill_name in manifest: + if active_index is None: + active_index = _index_active_skills() + hub_paths = _read_hub_install_paths() _moved_from = _recover_renamed_skill( skill_name, manifest.get(skill_name, ""), dest, active_index, - hub_paths, + hub_paths or set(), quiet, ) if _moved_from: @@ -816,6 +836,16 @@ def sync_skills(quiet: bool = False) -> dict: elif dest.exists(): # ── Existing skill — in manifest AND on disk ── origin_hash = manifest.get(skill_name, "") + + # If the bundled source still matches the version recorded when + # it was installed, there is no update to apply. Avoid recursively + # hashing the user's copy just to rediscover that fact; when the + # bundled source changes, the normal user-modification check below + # still protects local edits before any overwrite. + if origin_hash and bundled_hash == origin_hash: + skipped += 1 + continue + user_hash = _dir_hash(dest) if not origin_hash: