diff --git a/ms_agent/skill/__init__.py b/ms_agent/skill/__init__.py index dea0ba194..3894a820e 100644 --- a/ms_agent/skill/__init__.py +++ b/ms_agent/skill/__init__.py @@ -1,5 +1,6 @@ # Copyright (c) ModelScope Contributors. All rights reserved. from .catalog import SkillCatalog +from .discovery import SkillDescriptor from .loader import SkillLoader, load_skills from .prompt_injector import SkillPromptInjector from .safety import SafetyFinding, SkillSafetyReport, SkillSafetyScanner @@ -12,6 +13,7 @@ 'SkillSchema', 'SkillSchemaParser', 'SkillFile', + 'SkillDescriptor', 'SkillLoader', 'load_skills', 'SkillSource', diff --git a/ms_agent/skill/discovery.py b/ms_agent/skill/discovery.py new file mode 100644 index 000000000..0d4be77af --- /dev/null +++ b/ms_agent/skill/discovery.py @@ -0,0 +1,29 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Lightweight discovery primitives for local Agent Skills. + +Discovery intentionally reads only ``SKILL.md``. Full file materialization, +safety scanning, and registration remain the catalog's responsibility. +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from typing import Optional + + +@dataclass(frozen=True) +class SkillDescriptor: + """Metadata discovered from a skill root without walking support files.""" + + skill_id: str + name: str + description: str + content: str + version: str = 'latest' + author: Optional[str] = None + tags: tuple[str, ...] = field(default_factory=tuple) + skill_path: Path = field(default_factory=lambda: Path.cwd().resolve()) + + @property + def key(self) -> str: + return f'{self.skill_id}@{self.version}' diff --git a/ms_agent/skill/loader.py b/ms_agent/skill/loader.py index 2f62e6383..9d3c8b244 100644 --- a/ms_agent/skill/loader.py +++ b/ms_agent/skill/loader.py @@ -4,9 +4,10 @@ import os import re from pathlib import Path -from typing import Dict, List, Optional, Union +from typing import Dict, Iterable, List, Optional, Union from ms_agent.utils.logger import logger +from .discovery import SkillDescriptor from .schema import SkillSchema, SkillSchemaParser @@ -22,6 +23,9 @@ class SkillLoader: def __init__(self): self.loaded_skills: Dict[str, SkillSchema] = {} self.parser = SkillSchemaParser() + self._discovery_cache: Dict[Path, + tuple[tuple[int, int, int], + Optional[SkillDescriptor]]] = {} def load_skills( self, skills: Union[str, List[str], List[SkillSchema]] @@ -80,6 +84,48 @@ def load_skills( return all_skills + def discover_skills( + self, skills: Union[str, List[str]]) -> Dict[str, SkillDescriptor]: + """Discover local skills without traversing their support files. + + The source ordering, hidden-directory rule, maximum nesting depth, + and "skill root is a leaf" rule match :meth:`load_skills`. Only + ``SKILL.md`` is read; callers that execute a skill must still use the + full catalog loading path. + """ + if not skills: + return {} + if isinstance(skills, str): + skill_list = [skills] + elif all(isinstance(skill, str) for skill in skills): + skill_list = skills + else: + raise ValueError('Invalid skills input type.') + + discovered: Dict[str, SkillDescriptor] = {} + active_roots: set[Path] = set() + for value in skill_list: + path = Path(value) + if not path.exists(): + logger.warning(f'Path does not exist: {path} - Skipping.') + continue + roots: Iterable[Path] + if self._is_skill_directory(path): + roots = (path, ) + else: + roots = self._iter_skill_directories(path) + for root in roots: + active_roots.add(root.resolve()) + descriptor = self._discover_single_skill(root) + if descriptor is not None: + discovered[descriptor.key] = descriptor + self._discovery_cache = { + path: cached + for path, cached in self._discovery_cache.items() + if path in active_roots + } + return discovered + def _is_skill_directory(self, path: Path) -> bool: """ Check if a directory is a valid skill directory. @@ -122,10 +168,65 @@ def _load_single_skill(self, skill_dir: Path) -> Optional[SkillSchema]: logger.error(f'Error loading skill ({skill_dir}): {str(e)}') return None + def _discover_single_skill(self, + skill_dir: Path) -> Optional[SkillDescriptor]: + try: + skill_dir = skill_dir.resolve() + skill_md = skill_dir / 'SKILL.md' + file_stat = skill_md.stat() + fingerprint = (file_stat.st_mtime_ns, file_stat.st_ctime_ns, + file_stat.st_size) + cached = self._discovery_cache.get(skill_dir) + if cached is not None and cached[0] == fingerprint: + return cached[1] + + content = skill_md.read_text(encoding='utf-8') + frontmatter = self.parser.parse_yaml_frontmatter(content) + if (not frontmatter or 'name' not in frontmatter + or 'description' not in frontmatter): + self._discovery_cache[skill_dir] = (fingerprint, None) + return None + descriptor = SkillDescriptor( + skill_id=skill_dir.name, + name=frontmatter['name'], + description=frontmatter['description'], + content=content, + version=frontmatter.get('version', 'latest'), + author=frontmatter.get('author'), + tags=tuple(frontmatter.get('tags') or []), + skill_path=skill_dir, + ) + self._discovery_cache[skill_dir] = (fingerprint, descriptor) + return descriptor + except Exception as exc: + logger.error(f'Error discovering skill ({skill_dir}): {exc}') + return None + #: How deep _scan_and_load_skills descends below the scan root. Bounds #: symlink cycles; deep enough for organizational nesting (category dirs). _MAX_SCAN_DEPTH = 5 + def _iter_skill_directories(self, base_path: Path) -> Iterable[Path]: + """Yield skill roots using the same bounded marker walk as loading.""" + if not base_path.is_dir(): + logger.warning(f'Not a valid directory: {base_path}') + return + + def _walk(directory: Path, depth: int) -> Iterable[Path]: + try: + entries = sorted(directory.iterdir()) + except OSError: + return + for item in entries: + if not item.is_dir() or item.name.startswith('.'): + continue + if self._is_skill_directory(item): + yield item + elif depth < self._MAX_SCAN_DEPTH: + yield from _walk(item, depth + 1) + + yield from _walk(base_path, 1) + def _scan_and_load_skills(self, base_path: Path) -> Dict[str, SkillSchema]: """ Recursively scan a tree and load every skill root found. @@ -144,26 +245,10 @@ def _scan_and_load_skills(self, base_path: Path) -> Dict[str, SkillSchema]: """ skills: Dict[str, SkillSchema] = {} - if not base_path.is_dir(): - logger.warning(f'Not a valid directory: {base_path}') - return skills - - def _walk(directory: Path, depth: int) -> None: - try: - entries = sorted(directory.iterdir()) - except OSError: - return - for item in entries: - if not item.is_dir() or item.name.startswith('.'): - continue - if self._is_skill_directory(item): - skill = self._load_single_skill(item) - if skill: - skills[self._get_skill_key(skill=skill)] = skill - elif depth < self._MAX_SCAN_DEPTH: - _walk(item, depth + 1) - - _walk(base_path, 1) + for skill_dir in self._iter_skill_directories(base_path): + skill = self._load_single_skill(skill_dir) + if skill: + skills[self._get_skill_key(skill=skill)] = skill return skills @staticmethod diff --git a/ms_agent/skill/runtime.py b/ms_agent/skill/runtime.py index 42b348f71..12905e59e 100644 --- a/ms_agent/skill/runtime.py +++ b/ms_agent/skill/runtime.py @@ -182,14 +182,18 @@ def reload_all(self) -> None: self._catalog.reload() self._version += 1 - def sync_with_config(self, skills_config) -> bool: + def sync_with_config(self, skills_config, *, force: bool = True) -> bool: """Resync the catalog from an updated skills config; bump the version only when the effective skill surface (inventory, metadata, disabled set) actually changed, so maybe_refresh_system_prompt() rebuilds messages[0] on real change and stays a no-op otherwise. - Returns True when the surface changed. + Existing callers retain the full-resync behavior by default. A host + with an authoritative change tracker may pass ``force=False`` to skip + I/O for a known-clean turn. Returns True when the surface changed. """ + if not force: + return False before = self._surface() self._catalog.resync(skills_config) after = self._surface() diff --git a/ms_agent/skill/schema.py b/ms_agent/skill/schema.py index 4b7ae76ea..bcdc0094c 100644 --- a/ms_agent/skill/schema.py +++ b/ms_agent/skill/schema.py @@ -5,7 +5,10 @@ Defines the data structure and validation logic for Agent Skills. Each Skill is represented as a self-contained directory with metadata. """ +import hashlib +import os import re +import stat import yaml from dataclasses import dataclass, field from pathlib import Path @@ -233,13 +236,17 @@ def is_ignored_path(p: Path) -> bool: Returns: True if path should be ignored, False otherwise """ + return SkillSchemaParser._is_ignored_name(p.name) + + @staticmethod + def _is_ignored_name(name: str) -> bool: ignored_names = { '.DS_Store', '__pycache__', '.git', '.gitignore', '.pytest_cache', '.mypy_cache' } ignored_suffixes = {'.pyc', '.pyo'} - - return (p.name in ignored_names) or (p.suffix in ignored_suffixes) + return ((name in ignored_names) + or (os.path.splitext(name)[1] in ignored_suffixes)) @staticmethod def parse_skill_directory(directory_path: Path) -> Optional[SkillSchema]: @@ -276,31 +283,77 @@ def parse_skill_directory(directory_path: Path) -> Optional[SkillSchema]: scripts = [] references = [] resources = [] + digest = hashlib.sha256() - for file_path in directory_path.rglob('*'): - if file_path.is_file(): - if SkillSchemaParser.is_ignored_path(file_path): + def _walk(current_path: Path, relative_dir: str, + visible_dir: bool) -> None: + # DirEntry keeps type/stat data from the same scandir call. This + # avoids a second filesystem lookup while retaining deterministic + # files-first DFS order (the legacy notice signature's order). + try: + with os.scandir(current_path) as iterator: + entries = sorted(iterator, key=lambda entry: entry.name) + except OSError: + return + directories = [] + for entry in entries: + try: + if entry.is_dir(follow_symlinks=False): + directories.append(entry.name) + continue + file_stat = entry.stat(follow_symlinks=True) + except OSError: + continue + if not stat.S_ISREG(file_stat.st_mode): continue - file_type = file_path.suffix if file_path.suffix else '.unknown' + filename = entry.name + file_path = current_path / filename + relative = ( + filename + if not relative_dir else f'{relative_dir}/{filename}') + if visible_dir and not filename.startswith('.'): + signature_row = (f'{relative}|{file_stat.st_mtime_ns}|' + f'{file_stat.st_size}\n') + digest.update(signature_row.encode()) + + if SkillSchemaParser._is_ignored_name(filename): + continue + suffix = os.path.splitext(filename)[1] + file_type = suffix if suffix else '.unknown' skill_file = SkillFile( - name=file_path.name, + name=filename, type=file_type, path=file_path, - required=(file_path.name == 'SKILL.md')) + required=(filename == 'SKILL.md')) files.append(skill_file) # Get scripts, references and resources if skill_file.type in SUPPORTED_SCRIPT_EXT: scripts.append(skill_file) - elif skill_file.type in ['.md' - ] and skill_file.name != 'SKILL.md': + elif (skill_file.type == '.md' + and skill_file.name != 'SKILL.md'): references.append(skill_file) else: resources.append(skill_file) - return SkillSchema( + # Hidden dirs remain in full materialization because the parser + # historically included their non-ignored support files. Only the + # private change signature prunes them. + for dirname in directories: + child_relative = ( + dirname + if not relative_dir else f'{relative_dir}/{dirname}') + _walk( + current_path / dirname, + child_relative, + visible_dir and not dirname.startswith('.'), + ) + + _walk(directory_path, '', True) + + schema = SkillSchema( skill_id=skill_id, name=frontmatter['name'], description=frontmatter['description'], @@ -314,6 +367,10 @@ def parse_skill_directory(directory_path: Path) -> Optional[SkillSchema]: references=references, resources=resources, ) + # Private, in-memory-only bridge for hosts that already had to perform + # this full traversal. It deliberately does not alter serialization. + schema._files_signature = digest.hexdigest()[:16] + return schema @staticmethod def validate_skill_schema(schema: SkillSchema) -> List[str]: diff --git a/tests/skill/test_live_tree.py b/tests/skill/test_live_tree.py index b7b7f649b..303d83042 100644 --- a/tests/skill/test_live_tree.py +++ b/tests/skill/test_live_tree.py @@ -198,6 +198,27 @@ def test_duplicate_sources_load_once(self, tmp_path): class TestRuntimeSync: + def test_known_clean_sync_skips_catalog_io(self, tmp_path, monkeypatch): + s1 = _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([s1]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + rt = SkillRuntime(catalog=cat) + calls = [] + original_resync = cat.resync + + def _resync(value): + calls.append(value) + return original_resync(value) + + monkeypatch.setattr(cat, 'resync', _resync) + + assert rt.sync_with_config(cfg, force=False) is False + assert calls == [] + + assert rt.sync_with_config(cfg) is False + assert calls == [cfg] + def test_sync_bumps_version_only_on_change(self, tmp_path): s1 = _mk_skill(tmp_path, 'alpha') cfg = _skills_config([s1]) diff --git a/tests/skill/test_skill_discovery.py b/tests/skill/test_skill_discovery.py new file mode 100644 index 000000000..9e0f7477e --- /dev/null +++ b/tests/skill/test_skill_discovery.py @@ -0,0 +1,112 @@ +import hashlib +import os +from pathlib import Path + +from ms_agent.skill.loader import SkillLoader +from ms_agent.skill.schema import SkillSchemaParser + + +def _make_skill(root: Path, name: str, description: str = 'description'): + skill = root / name + skill.mkdir(parents=True) + (skill / 'SKILL.md').write_text( + f'---\nname: {name}\ndescription: {description}\n---\nbody', + encoding='utf-8', + ) + return skill + + +def _legacy_signature(root: Path) -> str: + digest = hashlib.sha256() + for current, dirs, files in os.walk(root): + dirs[:] = sorted(name for name in dirs if not name.startswith('.')) + for name in sorted(name for name in files if not name.startswith('.')): + path = Path(current) / name + try: + file_stat = path.stat() + except OSError: + continue + relative = path.relative_to(root).as_posix() + digest.update( + f'{relative}|{file_stat.st_mtime_ns}|{file_stat.st_size}\n'. + encode()) + return digest.hexdigest()[:16] + + +def test_discovery_reads_metadata_without_support_tree_walk( + tmp_path, monkeypatch): + skill = _make_skill(tmp_path, 'alpha') + (skill / 'references').mkdir() + (skill / 'references' / 'large.md').write_text('payload', encoding='utf-8') + + def _unexpected_rglob(*args, **kwargs): + raise AssertionError( + 'metadata discovery must not traverse support files') + + monkeypatch.setattr(Path, 'rglob', _unexpected_rglob) + discovered = SkillLoader().discover_skills(str(tmp_path)) + + assert list(discovered) == ['alpha@latest'] + descriptor = discovered['alpha@latest'] + assert descriptor.skill_id == 'alpha' + assert descriptor.name == 'alpha' + assert descriptor.skill_path == skill.resolve() + + +def test_discovery_matches_loader_marker_walk_rules(tmp_path): + _make_skill(tmp_path, 'top') + outer = _make_skill(tmp_path / 'group', 'outer') + _make_skill(outer / 'references', 'nested-inside-skill') + _make_skill(tmp_path / '.hidden', 'hidden') + + discovered = SkillLoader().discover_skills(str(tmp_path)) + + assert {item.skill_id for item in discovered.values()} == {'top', 'outer'} + + +def test_discovery_caches_unchanged_skill_markdown(tmp_path, monkeypatch): + _make_skill(tmp_path, 'alpha') + loader = SkillLoader() + original_read_text = Path.read_text + reads = [] + + def _read_text(path, *args, **kwargs): + reads.append(path) + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, 'read_text', _read_text) + assert loader.discover_skills(str(tmp_path)) + assert loader.discover_skills(str(tmp_path)) + + assert reads == [tmp_path / 'alpha' / 'SKILL.md'] + + +def test_full_parse_reuses_exact_legacy_files_signature(tmp_path): + skill = _make_skill(tmp_path, 'alpha') + (skill / 'z.txt').write_text('z', encoding='utf-8') + (skill / 'a').mkdir() + (skill / 'a' / 'b.txt').write_text('b', encoding='utf-8') + (skill / '.hidden').mkdir() + (skill / '.hidden' / 'ignored.txt').write_text('ignored', encoding='utf-8') + (skill / '__pycache__').mkdir() + (skill / '__pycache__' / 'tracked.pyc').write_bytes(b'bytecode') + + schema = SkillSchemaParser.parse_skill_directory(skill) + + assert schema is not None + assert schema._files_signature == _legacy_signature(skill) + + +def test_signature_changes_for_visible_resource_but_not_hidden_file(tmp_path): + skill = _make_skill(tmp_path, 'alpha') + resource = skill / 'reference.txt' + resource.write_text('before', encoding='utf-8') + first = SkillSchemaParser.parse_skill_directory(skill)._files_signature + + resource.write_text('after with different size', encoding='utf-8') + second = SkillSchemaParser.parse_skill_directory(skill)._files_signature + assert second != first + + (skill / '.transient').write_text('ignored', encoding='utf-8') + third = SkillSchemaParser.parse_skill_directory(skill)._files_signature + assert third == second