From e405f8f81d71c608a8f486e051df2b3354e2ed2d Mon Sep 17 00:00:00 2001 From: Ksandr Date: Wed, 19 Aug 2026 01:04:12 +0300 Subject: [PATCH] =?UTF-8?q?feat(skills):=20back=20text=20skills=20with=20S?= =?UTF-8?q?hellTools=20=F0=9F=A4=96=F0=9F=A4=96=F0=9F=A4=96?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ksandr --- CHANGELOG.md | 5 + examples/README.md | 15 ++ examples/quickstart/10_skills.py | 7 +- skills/nooa-tools-and-skills/SKILL.md | 3 +- src/nooa/__init__.py | 3 +- src/nooa/skill.py | 211 ++++++++++++-------------- tests/test_skill_registry_extended.py | 10 ++ tests/unit/test_skill.py | 123 +++++++-------- 8 files changed, 195 insertions(+), 182 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ca9fd132..a221223e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ to follow semantic versioning. inspect `original_type`, `original_error`, and `diagnostic` for worker-side details. - Add composable, context-scoped instrumentation hooks and trace-session scopes so hosts can observe NOOA execution without replacing native tracing. - Initial public release of NVIDIA Object-Oriented Agents (NOOA). +- Text skills now load as regular `Skill` objects with documentation from + `SKILL.md`, a stable `files: list[SkillFile]` manifest, and a skill-root-scoped + `ShellTools` dependency. `TextSkill(path=...)` remains available as the + compatibility constructor; migrate `read_file()` / `run_script()` calls to + `skill.shell.read()` / `skill.shell.run()`. - Security: MCP server configurations no longer expand host environment variables from `${VAR}` placeholders. Trusted caller code must resolve secrets and pass their values explicitly. diff --git a/examples/README.md b/examples/README.md index 7f04664c1..7df82bd9d 100644 --- a/examples/README.md +++ b/examples/README.md @@ -49,6 +49,21 @@ features. Each file is standalone and includes its exact run command. If you are new to NOOA, run examples 1–6 in order. After that, choose by the capability you need rather than treating the remaining files as required steps. +### Text skill API + +`TextSkill(path=...)` returns a regular `Skill` whose documentation comes from +`SKILL.md`. Use its root-relative file manifest and root-scoped `ShellTools`: + +```python +skill = TextSkill(path=ASSETS / "frontend-design") +print([file.path for file in skill.files]) +content = (await skill.shell.read("assets/prompt.txt")).text +output = await skill.shell.run("python3 scripts/check.py") +``` + +The former `read_file()` and `run_script()` helpers are removed. Registry +reloads rebuild the documentation, file manifest, and skill-local shell. + ## Advanced mechanics These examples isolate lower-level extension points. Read the source first; diff --git a/examples/quickstart/10_skills.py b/examples/quickstart/10_skills.py index eb5f7ebf5..29dc4e555 100644 --- a/examples/quickstart/10_skills.py +++ b/examples/quickstart/10_skills.py @@ -16,13 +16,18 @@ class FrontendAgent(Agent, llm=llm): - """Agent with a single file-based skill.""" + """Use self.frontend_design.files to inspect packaged files. + + Use self.frontend_design.shell for file access and script execution. + """ frontend_design: TextSkill def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.frontend_design = TextSkill(path=ASSETS / "frontend-design") + # The model can inspect self.frontend_design.files and use + # self.frontend_design.shell for file access or script execution. async def respond(self, prompt: str) -> str: """Respond to a user message.""" diff --git a/skills/nooa-tools-and-skills/SKILL.md b/skills/nooa-tools-and-skills/SKILL.md index 945a5f4f4..2c61048fc 100644 --- a/skills/nooa-tools-and-skills/SKILL.md +++ b/skills/nooa-tools-and-skills/SKILL.md @@ -69,7 +69,8 @@ class MyAgent(Agent, llm=llm): ``` - Agents see a `# Skills` block (one-liner per skill) in their execution context and call `doc(self.)` for the full guide. -- `TextSkill` exposes `read_file(path)` and `await run_script(name, *args)` for files/scripts bundled with the SKILL.md. +- `TextSkill(path=...)` is a compatibility constructor that returns a regular dynamically generated `Skill`. Its class docstring comes from `SKILL.md`, `files: list[SkillFile]` lists packaged files using stable skill-root-relative paths, and `shell: ShellTools` is scoped to the skill root. +- Read bundled files with `await skill.shell.read(file.path)` and run scripts with `await skill.shell.run("python3 scripts/task.py")`. The old `read_file()` / `run_script()` helpers are intentionally absent; all file and process behavior goes through ShellTools. - SKILL.md frontmatter: required `name`, `description`; optional `compatibility`, `metadata`, `user-invocable`, `allowed-tools` — Claude-Code-compatible format. - `SkillRegistry(agent)` supports explicit `register(...)` / `activate(...)` and bulk `discover_skills_dirs(...)`. Register model-facing `TextSkill` diff --git a/src/nooa/__init__.py b/src/nooa/__init__.py index 0b5a21036..b3157fe2a 100644 --- a/src/nooa/__init__.py +++ b/src/nooa/__init__.py @@ -55,7 +55,7 @@ # Export event filtering from nooa.runtime.event_query import EventQuery # noqa: E402 from nooa.runtime.events import EventsApi # noqa: E402 -from nooa.skill import Skill, TextSkill, get_slash_commands, slash_command # noqa: E402 +from nooa.skill import Skill, SkillFile, TextSkill, get_slash_commands, slash_command # noqa: E402 from nooa.skill_registry import skill_from_module # noqa: E402 # Export storage @@ -153,6 +153,7 @@ def __getattr__(name): # Agent and decorators "Agent", "Skill", + "SkillFile", "TextSkill", "slash_command", "get_slash_commands", diff --git a/src/nooa/skill.py b/src/nooa/skill.py index 8773d7e4c..76330f9ce 100644 --- a/src/nooa/skill.py +++ b/src/nooa/skill.py @@ -4,7 +4,8 @@ import inspect import re -import shlex +from collections.abc import Awaitable +from dataclasses import dataclass from pathlib import Path from typing import Annotated, Any @@ -212,44 +213,30 @@ def _parse_skill_md(path: Path) -> tuple[str, str, str]: return skill_id, description, body.strip() -def _resolve_skill_path(skill_root: Path, relative: str) -> Path: - """Resolve a relative path and verify it stays within skill_root. +@dataclass(frozen=True, slots=True) +class SkillFile: + """A file packaged with a text skill. - Raises: - ValueError: If the path escapes the skill directory. - FileNotFoundError: If the resolved path does not exist. + ``path`` is POSIX-style and relative to the skill root, so it can be + passed directly to the skill's ``shell`` methods on every platform. """ - resolved = (skill_root / relative).resolve() - if not resolved.is_relative_to(skill_root.resolve()): - raise ValueError( - f"Path {relative!r} escapes the skill directory. Only paths within {skill_root} are allowed." - ) - if not resolved.exists(): - raise FileNotFoundError(f"{relative!r} not found in skill directory {skill_root}") - return resolved + path: str -def _build_script_command( - script: Path, args: tuple[str, ...], interpreter: str | None = None -) -> str: - """Build the shell command to run a script. - - Priority: explicit interpreter > shebang line > direct execution. - Pass ``interpreter`` explicitly for scripts without a shebang. - """ - quoted_script = shlex.quote(str(script)) - quoted_args = " ".join(shlex.quote(a) for a in args) - - if interpreter: - cmd = ( - f"{interpreter} {quoted_script} {quoted_args}" - if quoted_args - else f"{interpreter} {quoted_script}" - ) - return cmd.strip() - cmd = f"{quoted_script} {quoted_args}" if quoted_args else quoted_script - return cmd.strip() +def _discover_skill_files(skill_root: Path) -> list[SkillFile]: + """Return a deterministic manifest of regular files contained by a skill.""" + root = skill_root.resolve() + files: list[SkillFile] = [] + for candidate in sorted(root.rglob("*"), key=lambda path: path.relative_to(root).as_posix()): + if not candidate.is_file(): + continue + # A symlink that resolves outside the skill is not part of the package + # and ShellTools would reject it at access time as well. + if not candidate.resolve().is_relative_to(root): + continue + files.append(SkillFile(path=candidate.relative_to(root).as_posix())) + return files class Skill: @@ -311,8 +298,12 @@ def attach(self, agent: Any) -> None: self._agent = agent @hidden - def detach(self) -> None: - """Called when this skill is removed from an agent.""" + def detach(self) -> Awaitable[None] | None: + """Called when this skill is removed from an agent. + + Implementations may release resources asynchronously; SkillRegistry + awaits the returned object when needed. + """ self._agent = None def __dir__(self) -> list[str]: @@ -349,87 +340,81 @@ def source_dir(self) -> Path | None: return source_path -class TextSkill(Skill): - """Skill loaded from a SKILL.md directory. Has id, description, run_script, read_file.""" - - def __init__(self, *, path: Path | str, id: str | None = None): - skill_id, description, body = _parse_skill_md(Path(path)) - docstring = f"{description}\n---\n{body}" - class_name = "".join(word.capitalize() for word in skill_id.split("-")) - self._skill_path = Path(path) - self.__class__ = type( # pyright: ignore[reportAttributeAccessIssue] - class_name, (TextSkill,), {"__doc__": docstring, "_id": id or skill_id} - ) +class _GeneratedTextSkill(Skill): + """Runtime base for data-driven skills constructed from a SKILL.md directory.""" - @property - def id(self) -> str: - return str(type(self)._id) # pyright: ignore[reportAttributeAccessIssue] # _id set dynamically in __init__ + id: str + description: str + files: list[SkillFile] - @property - def source_dir(self) -> Path | None: - return self._skill_path.resolve() + def __init__(self, *, path: Path, skill_id: str, description: str) -> None: + # Import lazily: ShellTools itself subclasses Skill, so importing it at + # module initialization time would create a circular import. + from nooa.tools.shell_tools import ShellTools + + self._skill_path = path.resolve() + self.id = skill_id + self.description = description + self.files = _discover_skill_files(self._skill_path) + self.shell = ShellTools(cwd=str(self._skill_path)) + super().__init__() @property - def description(self) -> str: - doc = type(self).__doc__ or "" - return doc.split("\n---\n")[0].split("\n")[0] + def source_dir(self) -> Path | None: + return self._skill_path - async def run_script( - self, - name: str, - *args: str, - interpreter: str | None = None, - timeout: float = 30.0, - ) -> str: - """Run a script from this skill's scripts/ directory. - - Supports any script type — Python, shell, Ruby, Node.js, Perl, etc. - Scripts with a shebang line run directly. For scripts without one, - pass ``interpreter`` explicitly. - - output = await self.my_skill.run_script("run_eval.py", "--limit", "10") - output = await self.my_skill.run_script("report.sh") - output = await self.my_skill.run_script("query.sql", interpreter="psql -f") - output = await self.my_skill.run_script("process.js", "input.json") - """ - script = _resolve_skill_path(self._skill_path, f"scripts/{name}") - if not script.is_file(): - scripts_dir = self._skill_path / "scripts" - available = ( - sorted(p.name for p in scripts_dir.iterdir() if p.is_file()) - if scripts_dir.is_dir() - else [] - ) - raise FileNotFoundError( - f"Script {name!r} not found in {scripts_dir}. Available: {available}" - ) - - from nooa.tools._bash_session import BashSession - - cmd = _build_script_command(script, args, interpreter=interpreter) - session = BashSession(cwd=self._skill_path) + @hidden + async def detach(self) -> None: + """Release the skill-local shell when the registry replaces this object.""" try: - await session.start() - stdout, stderr, exit_code = await session.run(cmd, timeout=timeout) + await self.shell.close() finally: - await session.close() - - # Format output to match the previous BashResult.__str__() contract - output = stdout - if stderr: - output += f"\n[stderr]\n{stderr}" - if exit_code != 0: - output += f"\n[exit code: {exit_code}]" - return output - - def read_file(self, path: str) -> str: - """Read a file from anywhere within this skill's directory. - - content = self.my_skill.read_file("scripts/run_eval.py") - content = self.my_skill.read_file("assets/prompt.txt") - content = self.my_skill.read_file("SKILL.md") - """ - resolved = _resolve_skill_path(self._skill_path, path) - if not resolved.is_file(): - raise ValueError(f"{path!r} is not a file.") - return resolved.read_text() + super().detach() + + +def _load_text_skill(*, path: Path | str, id: str | None = None) -> Skill: + """Construct a normal Skill object from a SKILL.md directory.""" + from nooa.tools.shell_tools import ShellTools + + skill_path = Path(path).resolve() + skill_id, description, body = _parse_skill_md(skill_path) + docstring = f"{description}\n---\n{body}" + class_name = "".join(word.capitalize() for word in skill_id.split("-")) + generated_class = type( + class_name, + (_GeneratedTextSkill,), + { + "__doc__": docstring, + "__module__": __name__, + "__annotations__": { + "shell": ShellTools, + "files": list[SkillFile], + }, + }, + ) + return generated_class( + path=skill_path, + skill_id=id or skill_id, + description=description, + ) + + +class TextSkill(Skill): + """Compatibility factory for loading a SKILL.md directory as a normal Skill. + + The returned object is a generated ``Skill`` with documentation from + ``SKILL.md``, a skill-root-scoped ``shell``, and a ``files`` manifest. + ``TextSkill`` remains importable so existing construction sites continue + to work, but the returned object has no text-skill-specific tool methods. + """ + + def __new__(cls, *, path: Path | str, id: str | None = None) -> Skill: + if cls is not TextSkill: + return super().__new__(cls) + return _load_text_skill(path=path, id=id) + + def __init__(self, *, path: Path | str, id: str | None = None) -> None: + # ``__new__`` returns a generated Skill, so this initializer is skipped + # for normal TextSkill(...) calls. Keep the signature for introspection + # and fail clearly for unsupported TextSkill subclassing. + raise TypeError("TextSkill is a compatibility factory and cannot be subclassed") diff --git a/tests/test_skill_registry_extended.py b/tests/test_skill_registry_extended.py index a1d96c1b6..0260f0a5a 100644 --- a/tests/test_skill_registry_extended.py +++ b/tests/test_skill_registry_extended.py @@ -459,20 +459,30 @@ async def test_each_agent_reloads_its_own_text_skill(self, tmp_path): skill_dir.mkdir() skill_md = skill_dir / "SKILL.md" skill_md.write_text("---\nname: demo\ndescription: old description\n---\nold body\n") + old_file = skill_dir / "old.txt" + old_file.write_text("old") first = SkillRegistry(_FakeAgent()) second = SkillRegistry(_FakeAgent()) first.discover_skills_dirs([tmp_path]) second.discover_skills_dirs([tmp_path]) try: skill_md.write_text("---\nname: demo\ndescription: new description\n---\nnew body\n") + old_file.unlink() + (skill_dir / "new.txt").write_text("new") assert await first.reload("cmd.demo") == "Reloaded cmd.demo (self.demo)" assert first["cmd.demo"].description == "new description" + assert "new body" in (type(first["cmd.demo"]).__doc__ or "") + assert [file.path for file in first["cmd.demo"].files] == ["SKILL.md", "new.txt"] assert second["cmd.demo"].description == "old description" + assert "old body" in (type(second["cmd.demo"]).__doc__ or "") + assert [file.path for file in second["cmd.demo"].files] == ["SKILL.md", "old.txt"] assert await second.reload("cmd.demo") == "Reloaded cmd.demo (self.demo)" assert second["cmd.demo"].description == "new description" + assert [file.path for file in second["cmd.demo"].files] == ["SKILL.md", "new.txt"] assert first["cmd.demo"] is not second["cmd.demo"] + assert first["cmd.demo"].shell is not second["cmd.demo"].shell finally: await first.aclose() await second.aclose() diff --git a/tests/unit/test_skill.py b/tests/unit/test_skill.py index 61c917e41..563a41bc3 100644 --- a/tests/unit/test_skill.py +++ b/tests/unit/test_skill.py @@ -7,7 +7,9 @@ import pytest -from nooa import Skill, TextSkill +from nooa import Skill, SkillFile, TextSkill +from nooa.agentdoc import doc +from nooa.tools import ShellTools @pytest.fixture @@ -28,9 +30,15 @@ def test_skill_path_loads_id(skill_dir): assert TextSkill(path=skill_dir).id == "git-workflow" +def test_skill_path_accepts_explicit_id(skill_dir): + assert TextSkill(path=skill_dir, id="custom-id").id == "custom-id" + + def test_skill_path_creates_dynamic_subclass(skill_dir): skill = TextSkill(path=skill_dir) assert type(skill).__name__ != "Skill" + assert isinstance(skill, Skill) + assert not isinstance(skill, TextSkill) assert "Best practices for Git" in (type(skill).__doc__ or "") @@ -79,7 +87,7 @@ def test_skill_dir_on_path_skill(skill_dir): assert "id" in dir(skill) -# ── run_script ──────────────────────────────────────────────────────────────── +# ── ShellTools and packaged files ───────────────────────────────────────────── @pytest.fixture @@ -87,85 +95,68 @@ def skill_with_scripts(skill_dir): scripts_dir = skill_dir / "scripts" scripts_dir.mkdir() (scripts_dir / "greet.py").write_text("print('hello from script')") - (scripts_dir / "echo_args.py").write_text("import sys\nprint(' '.join(sys.argv[1:]))") - (scripts_dir / "fail.py").write_text("import sys\nprint('oops')\nsys.exit(1)") - shebang = scripts_dir / "shebang_echo.py" - shebang.write_text("#!/usr/bin/env python3\nimport sys\nprint(' '.join(sys.argv[1:]))") - shebang.chmod(0o755) + assets_dir = skill_dir / "assets" + assets_dir.mkdir() + (assets_dir / "prompt.txt").write_text("prompt") return skill_dir -@pytest.mark.asyncio -async def test_run_script_python(skill_with_scripts): - output = await TextSkill(path=skill_with_scripts).run_script("greet.py", interpreter="python3") - assert "hello from script" in output - - -@pytest.mark.asyncio -async def test_run_script_with_args(skill_with_scripts): - output = await TextSkill(path=skill_with_scripts).run_script( - "echo_args.py", "foo", "bar", interpreter="python3" - ) - assert "foo bar" in output - - -@pytest.mark.asyncio -async def test_run_script_nonzero_exit_in_output(skill_with_scripts): - output = await TextSkill(path=skill_with_scripts).run_script("fail.py", interpreter="python3") - assert "oops" in output - assert "exit code: 1" in output - - -@pytest.mark.asyncio -async def test_run_script_raises_for_missing_script(skill_dir): - with pytest.raises(FileNotFoundError): - await TextSkill(path=skill_dir).run_script("nonexistent.py") - - -@pytest.mark.asyncio -async def test_run_script_raises_when_script_is_a_directory(skill_with_scripts): - # Create scripts/mydir/ — exists but is not a file → triggers the "available" error path - (skill_with_scripts / "scripts" / "mydir").mkdir() - with pytest.raises(FileNotFoundError, match="Available"): - await TextSkill(path=skill_with_scripts).run_script("mydir") +def test_text_skill_injects_skill_root_shell(skill_dir): + skill = TextSkill(path=skill_dir) + assert isinstance(skill.shell, ShellTools) + assert skill.shell.cwd == skill_dir.resolve() @pytest.mark.asyncio -async def test_run_script_with_shebang_and_args(skill_with_scripts): - # No interpreter= — uses shebang directly; covers _build_script_command no-interpreter+args path - output = await TextSkill(path=skill_with_scripts).run_script( - "shebang_echo.py", "hello", "world" - ) - assert "hello world" in output +async def test_text_skill_reads_files_through_shell(skill_dir): + skill = TextSkill(path=skill_dir) + result = await skill.shell.read("SKILL.md") + assert "Best practices for Git" in result.text @pytest.mark.asyncio -async def test_run_script_raises_for_path_traversal(skill_with_scripts): - with pytest.raises(ValueError, match="escapes the skill directory"): - await TextSkill(path=skill_with_scripts).run_script("../../etc/passwd") - - -# ── read_file ───────────────────────────────────────────────────────────────── - - -def test_read_file_returns_content(skill_dir): - assert "Best practices for Git" in TextSkill(path=skill_dir).read_file("SKILL.md") +async def test_text_skill_runs_scripts_through_shell(skill_with_scripts): + skill = TextSkill(path=skill_with_scripts) + try: + output = await skill.shell.run("python3 scripts/greet.py") + assert output.success + assert "hello from script" in output.stdout + finally: + await skill.detach() + + +def test_text_skill_files_are_relative_and_stably_sorted(skill_with_scripts): + skill = TextSkill(path=skill_with_scripts) + assert skill.files == [ + SkillFile(path="SKILL.md"), + SkillFile(path="assets/prompt.txt"), + SkillFile(path="scripts/greet.py"), + ] + + +def test_text_skill_manifest_excludes_external_symlinks(skill_dir, tmp_path): + outside = tmp_path / "outside.txt" + outside.write_text("outside") + (skill_dir / "outside-link.txt").symlink_to(outside) + skill = TextSkill(path=skill_dir) -def test_read_file_raises_for_path_traversal(skill_dir): - with pytest.raises(ValueError, match="escapes the skill directory"): - TextSkill(path=skill_dir).read_file("../../etc/passwd") + assert SkillFile(path="outside-link.txt") not in skill.files -def test_read_file_raises_for_missing_file(skill_dir): - with pytest.raises(FileNotFoundError): - TextSkill(path=skill_dir).read_file("nonexistent.txt") +def test_text_skill_removes_legacy_helper_surface(skill_dir): + skill = TextSkill(path=skill_dir) + assert not hasattr(skill, "read_file") + assert not hasattr(skill, "run_script") -def test_read_file_raises_when_path_is_directory(skill_dir): - (skill_dir / "subdir").mkdir() - with pytest.raises(ValueError, match="is not a file"): - TextSkill(path=skill_dir).read_file("subdir") +def test_text_skill_documentation_exposes_shell_and_files(skill_dir): + rendered = doc(TextSkill(path=skill_dir)) + assert "Best practices for Git" in rendered + assert "shell: ShellTools" in rendered + assert "files: list[SkillFile]" in rendered + assert "read_file" not in rendered + assert "run_script" not in rendered # ── source_dir ────────────────────────────────────────────────────────────────