diff --git a/src/beaker_notebook/lib/integrations/skill.py b/src/beaker_notebook/lib/integrations/skill.py index 254a71be..74e31e21 100644 --- a/src/beaker_notebook/lib/integrations/skill.py +++ b/src/beaker_notebook/lib/integrations/skill.py @@ -141,11 +141,22 @@ def parse_skill_md(content: str) -> tuple[dict, str]: return frontmatter, body +#: Directory, relative to the skill root, whose contents are examples rather +#: than plain reference files. Referenced paths under it become +#: SkillExampleResources; see `_build_skill_integration`. +EXAMPLES_DIR = "examples/" + + def extract_file_references(body: str) -> list[str]: """Extract relative file paths referenced in the markdown body. Detects both markdown links [text](path) and backtick-quoted paths like `references/some_file.md` or `scripts/run.py`. + + Paths under ``examples/`` are included. The caller decides what a path + becomes -- `_build_skill_integration` routes them to SkillExampleResources + rather than SkillFileResources -- so this stays a single, uniform pass over + the body. A bare ``examples/`` directory link names no file and is dropped. """ references = [] @@ -153,14 +164,16 @@ def extract_file_references(body: str) -> list[str]: link_pattern = re.compile(r'\[(?:[^\]]*)\]\(([^)]+)\)') for match in link_pattern.finditer(body): path = match.group(1) - if not path.startswith(("http://", "https://", "#", "mailto:")): - # Skip examples/ paths — they are handled as SkillExampleResources - if not path.startswith("examples/") and path != "examples/": - references.append(path) + if path.startswith(("http://", "https://", "#", "mailto:")): + continue + # A link to the directory itself, not to a file in it. + if path.rstrip("/") == EXAMPLES_DIR.rstrip("/"): + continue + references.append(path) # Backtick-quoted file paths: `some/path.ext` # Match paths that contain a / and end with a file extension - backtick_pattern = re.compile(r'`((?:references|scripts|assets)/[^`]+)`') + backtick_pattern = re.compile(r'`((?:references|scripts|assets|examples)/[^`]+)`') for match in backtick_pattern.finditer(body): references.append(match.group(1)) @@ -460,15 +473,24 @@ def _build_skill_integration( content=body, ) + # One pass over the body; the prefix decides which kind of resource a + # referenced path becomes. Examples have their own tier and their own + # loading tool, so they must not also appear as plain files. file_resources = [] + referenced_examples = [] for ref_path in extract_file_references(body): - file_resources.append(SkillFileResource( - integration=skill.uuid, - name=Path(ref_path).name, - relative_path=ref_path, - )) - - example_resources = cls._discover_examples(skill, source_type, base_path, base_url) + if ref_path.startswith(EXAMPLES_DIR): + referenced_examples.append(ref_path) + else: + file_resources.append(SkillFileResource( + integration=skill.uuid, + name=Path(ref_path).name, + relative_path=ref_path, + )) + + example_resources = cls._discover_examples( + skill, source_type, base_path, base_url, referenced=referenced_examples + ) skill.add_resources([metadata_resource, instructions_resource] + file_resources + example_resources) return skill @@ -480,14 +502,31 @@ def _discover_examples( source_type: str, base_path: Optional[str] = None, base_url: Optional[str] = None, + referenced: "Optional[list[str]]" = None, ) -> list[SkillExampleResource]: - """Discover example files from the skill's examples/ directory. + """Discover a skill's examples, from the directory and from the body. + + Two sources, in precedence order: - For local skills, scans the examples/ subdirectory. For remote skills, - examples must be declared in the frontmatter (not yet implemented). - Only reads the first few lines of each file to extract title and description. + 1. **Local directory scan.** Reads enough of each ``examples/*.md`` file + to pull out a title and description. Only possible for local skills. + 2. **Paths referenced in SKILL.md**, passed in as ``referenced``. This is + the only source available to a remote skill, where the directory + cannot be listed over HTTP, and it also picks up local examples the + ``*.md`` glob misses -- a skill shipping ``examples/*.py`` was + previously invisible whether it was local or remote. + + The scan wins on conflict because it carries a real parsed title and + description; a referenced path yields only its filename. That costs + little, because the agent must call ``load_skill_instructions`` -- which + returns the whole body, author's example listing included -- before it + can ask for an example by name. + + Content is never fetched here; it stays tier 3, loaded on demand. """ resources = [] + seen: set[str] = set() + if source_type == "local" and base_path: examples_dir = Path(base_path) / "examples" if examples_dir.is_dir(): @@ -502,8 +541,25 @@ def _discover_examples( description=description, content=None, # Loaded on demand (tier 3) )) + seen.add(example_path.name) except Exception: logger.exception("Failed to parse example: %s", example_path) + + for relative_path in referenced or []: + # Keep the path below examples/ as the filename so nested examples + # round-trip through load_skill_examples and _fetch_file_content. + filename = relative_path[len(EXAMPLES_DIR):] + if not filename or filename in seen: + continue + seen.add(filename) + resources.append(SkillExampleResource( + integration=skill.uuid, + filename=filename, + title=filename, + description="", + content=None, # Loaded on demand (tier 3) + )) + return resources # --- Abstract method implementations --- @@ -662,7 +718,12 @@ async def load_skill_instructions(self, skill_slug: str, agent: AgentRef) -> str if examples: result += "\n\n## Available Code Examples\n" for ex in examples: - result += f"\n- **{ex.filename}**: {ex.title}" + result += f"\n- **{ex.filename}**" + # An example discovered from a reference in the body has no + # title beyond its own filename; printing it twice reads as a + # bug and spends tokens saying nothing. + if ex.title and ex.title != ex.filename: + result += f": {ex.title}" if ex.description: result += f"\n {ex.description}" result += ( diff --git a/tests/integrations/skills/test_skill_provider.py b/tests/integrations/skills/test_skill_provider.py index 581cf26a..ad115094 100644 --- a/tests/integrations/skills/test_skill_provider.py +++ b/tests/integrations/skills/test_skill_provider.py @@ -206,7 +206,12 @@ def test_mixed_references(self): assert "references/patterns.md" in refs assert "scripts/run.py" in refs - def test_examples_paths_excluded(self): + def test_example_paths_included_but_bare_directory_link_dropped(self): + """examples/ files are extracted here; the caller routes them onward. + + A link to the directory itself names no file, so it is dropped rather + than becoming a resource that cannot be loaded. + """ body = ( "See [examples/](examples/) for working examples.\n" "Also see [examples/basic.md](examples/basic.md) for a quick start.\n" @@ -214,9 +219,13 @@ def test_examples_paths_excluded(self): ) refs = extract_file_references(body) assert "examples/" not in refs - assert "examples/basic.md" not in refs + assert "examples/basic.md" in refs assert "references/guide.md" in refs + def test_backtick_example_paths(self): + refs = extract_file_references("Run `examples/quickstart.py` to start.") + assert "examples/quickstart.py" in refs + # --------------------------------------------------------------------------- # SkillIntegrationProvider — local loading @@ -610,6 +619,187 @@ def test_url_with_trailing_slash(self): mock_get.assert_called_once_with("https://example.com/repo/main/SKILL.md", timeout=30) +# --------------------------------------------------------------------------- +# Remote example discovery +# --------------------------------------------------------------------------- + +REMOTE_SKILL_WITH_EXAMPLES_MD = textwrap.dedent("""\ + --- + name: remote-skill + description: A skill served over HTTP. + --- + + # Remote skill + + See [the guide](references/guide.md) for details. + + ## Runnable examples + + - [examples/basic.py](examples/basic.py) — the simplest case + - [examples/advanced.py](examples/advanced.py) — every knob turned + """) + + +def _load_remote_skill_with_examples(): + mock_response = MagicMock() + mock_response.text = REMOTE_SKILL_WITH_EXAMPLES_MD + mock_response.raise_for_status = MagicMock() + with patch("beaker_notebook.lib.integrations.skill.requests.get", return_value=mock_response): + return SkillIntegrationProvider._load_remote_skill("https://example.com/skill/") + + +class TestRemoteExampleDiscovery: + """A remote skill's examples come from its own body. + + Listing a directory over HTTP is not possible, so before this the agent saw + no examples at all for any remotely-loaded skill. + """ + + def test_examples_discovered_from_body(self): + skill = _load_remote_skill_with_examples() + examples = [r for r in skill.resources.values() if isinstance(r, SkillExampleResource)] + assert {e.filename for e in examples} == {"basic.py", "advanced.py"} + + def test_filename_is_the_handle(self): + """The filename is what load_skill_examples is called with.""" + skill = _load_remote_skill_with_examples() + by_name = { + r.filename: r + for r in skill.resources.values() + if isinstance(r, SkillExampleResource) + } + assert by_name["basic.py"].title == "basic.py" + # The author's own gloss already reached the agent with the body, which + # load_skill_instructions returns in full before examples can be asked for. + assert by_name["basic.py"].description == "" + + def test_content_is_not_fetched_during_discovery(self): + """Discovery must stay free; content is tier 3, loaded on demand.""" + skill = _load_remote_skill_with_examples() + examples = [r for r in skill.resources.values() if isinstance(r, SkillExampleResource)] + assert examples and all(e.content is None for e in examples) + + def test_only_one_request_is_made(self): + mock_response = MagicMock() + mock_response.text = REMOTE_SKILL_WITH_EXAMPLES_MD + mock_response.raise_for_status = MagicMock() + with patch( + "beaker_notebook.lib.integrations.skill.requests.get", return_value=mock_response + ) as mock_get: + SkillIntegrationProvider._load_remote_skill("https://example.com/skill/") + mock_get.assert_called_once() + + def test_example_content_fetches_from_the_base_url(self): + skill = _load_remote_skill_with_examples() + provider = _make_provider([]) + mock_response = MagicMock() + mock_response.text = "print('hi')" + mock_response.raise_for_status = MagicMock() + with patch( + "beaker_notebook.lib.integrations.skill.requests.get", return_value=mock_response + ) as mock_get: + content = provider._fetch_file_content(skill, "examples/basic.py") + mock_get.assert_called_once_with( + "https://example.com/skill/examples/basic.py", timeout=30 + ) + assert content == "print('hi')" + + def test_examples_are_not_also_file_resources(self): + """Examples have their own tier; they must not double as reference files.""" + skill = _load_remote_skill_with_examples() + file_paths = { + r.relative_path + for r in skill.resources.values() + if isinstance(r, SkillFileResource) + } + assert file_paths == {"references/guide.md"} + + def test_skill_without_example_links_has_none(self): + mock_response = MagicMock() + mock_response.text = MINIMAL_SKILL_MD + mock_response.raise_for_status = MagicMock() + with patch("beaker_notebook.lib.integrations.skill.requests.get", return_value=mock_response): + skill = SkillIntegrationProvider._load_remote_skill("https://example.com/skill/") + assert not [r for r in skill.resources.values() if isinstance(r, SkillExampleResource)] + + def test_nested_example_path_round_trips(self): + """A nested path keeps enough of itself to be fetched back.""" + md = MINIMAL_SKILL_MD.rstrip() + "\n\n[deep](examples/advanced/tuning.py)\n" + resp = MagicMock(text=md, raise_for_status=MagicMock()) + with patch("beaker_notebook.lib.integrations.skill.requests.get", return_value=resp): + skill = SkillIntegrationProvider._load_remote_skill("https://example.com/skill/") + example = next( + r for r in skill.resources.values() if isinstance(r, SkillExampleResource) + ) + assert example.filename == "advanced/tuning.py" + + provider = _make_provider([]) + fetched = MagicMock(text="x = 1", raise_for_status=MagicMock()) + with patch( + "beaker_notebook.lib.integrations.skill.requests.get", return_value=fetched + ) as mock_get: + provider._fetch_file_content(skill, f"examples/{example.filename}") + mock_get.assert_called_once_with( + "https://example.com/skill/examples/advanced/tuning.py", timeout=30 + ) + + +# --------------------------------------------------------------------------- +# Example discovery: directory scan and body references combined +# --------------------------------------------------------------------------- + +class TestExampleSourcesAreDeduped: + """Local skills draw examples from two sources, which must not collide.""" + + def test_directory_scan_wins_over_body_reference(self, skill_dir: Path): + """The scan carries a real parsed title; a bare reference does not.""" + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + skill_md.read_text() + + "\n\nSee [examples/basic_usage.md](examples/basic_usage.md) too.\n" + ) + skill = SkillIntegrationProvider._load_local_skill(str(skill_dir)) + matching = [ + r for r in skill.resources.values() + if isinstance(r, SkillExampleResource) and r.filename == "basic_usage.md" + ] + assert len(matching) == 1, "example listed twice" + assert matching[0].title == "Basic usage of the full skill" + + def test_body_reference_adds_examples_the_glob_misses(self, skill_dir: Path): + """A skill shipping examples/*.py was previously invisible entirely. + + _discover_examples only globs *.md, so a .py example is found solely + because SKILL.md points at it. + """ + (skill_dir / "examples" / "script_example.py").write_text("print('hi')") + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + skill_md.read_text() + + "\n\n- [examples/script_example.py](examples/script_example.py) - a script\n" + ) + skill = SkillIntegrationProvider._load_local_skill(str(skill_dir)) + names = { + r.filename for r in skill.resources.values() + if isinstance(r, SkillExampleResource) + } + assert names == {"basic_usage.md", "advanced_usage.md", "script_example.py"} + + def test_referenced_examples_are_not_also_file_resources(self, skill_dir: Path): + """Examples have their own tier and must not double as reference files.""" + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + skill_md.read_text() + "\n\n[x](examples/basic_usage.md)\n" + ) + skill = SkillIntegrationProvider._load_local_skill(str(skill_dir)) + file_paths = { + r.relative_path for r in skill.resources.values() + if isinstance(r, SkillFileResource) + } + assert not any(p.startswith("examples/") for p in file_paths) + + + # --------------------------------------------------------------------------- # parse_example_md # --------------------------------------------------------------------------- diff --git a/tests/integrations/skills/test_skill_tools.py b/tests/integrations/skills/test_skill_tools.py index a57ca320..a8fcbd55 100644 --- a/tests/integrations/skills/test_skill_tools.py +++ b/tests/integrations/skills/test_skill_tools.py @@ -9,6 +9,7 @@ import pytest from beaker_notebook.lib.integrations.skill import SkillIntegrationProvider +from beaker_notebook.lib.integrations.types import SkillExampleResource SKILL_MD = textwrap.dedent("""\ @@ -87,6 +88,26 @@ async def test_load_skill_instructions_returns_body(provider): assert "basic.md" in result +async def test_example_listing_does_not_repeat_the_filename_as_its_title(provider): + """An example found via a body reference has no title but its filename. + + Rendering `- **basic.py**: basic.py` reads as a bug and spends tokens + saying nothing, so the title is omitted when it adds nothing. + """ + skill = provider._find_skill_by_slug("my-skill") + for resource in skill.resources.values(): + if isinstance(resource, SkillExampleResource): + resource.title = resource.filename + resource.description = "" + + result = await SkillIntegrationProvider.load_skill_instructions( + provider, skill_slug="my-skill", agent=_agent_ref(), + ) + listing = result.split("## Available Code Examples")[-1] + assert "- **basic.md**" in listing + assert "basic.md**: basic.md" not in listing + + async def test_load_skill_instructions_dedupes_per_session(provider): agent = _agent_ref("session-A") await SkillIntegrationProvider.load_skill_instructions(provider, skill_slug="my-skill", agent=agent)