fix: materialize skill resources to disk for subprocess access (#271)

* fix: materialize skill resources to disk for subprocess access

Skill-bundled scripts stored in skill_resources were loaded into memory
but never written to disk, causing FileNotFoundError when the model
tried to execute them. Write resources to a per-workstream temp directory
on skill load, expose via SKILL_RESOURCES_DIR env var and PATH, clean up
on skill change or session close.

* fix: pre-flight validation warns when skill references missing resources

Scan rendered skill content for path references (scripts/foo.py, etc.)
and compare against bundled skill_resources. Warn via on_info if any
referenced paths are not bundled, so operators see the gap at skill
activation rather than at runtime FileNotFoundError.

* fix: address PR #271 review feedback

- Fix trailing colon in PATH when $PATH is empty (cwd-on-PATH risk)
- Move try/except inside per-resource loop so one bad write doesn't
  abort all resources
- Explicit encoding="utf-8" for deterministic writes across locales

* fix: address PR #271 review round 2

- Normalize available paths in _validate_skill_resources() to match
  referenced paths (both sides use os.path.normpath now)
- Fix flaky traversal test: assert inside base dir, not escaped path

tests/test_skill_resource_materialization.py

@@ -0,0 +1,479 @@
+"""Tests for skill resource materialization to disk.
+
+Verifies that skill-bundled resources (scripts, references, assets) stored
+in the ``skill_resources`` table are written to a temp directory when a
+skill is loaded, exposed via ``SKILL_RESOURCES_DIR`` env var and ``PATH``,
+and cleaned up on skill change or session close.
+"""
+
+from __future__ import annotations
+
+import os
+import stat
+from typing import Any
+from unittest.mock import MagicMock
+
+from turnstone.core.session import ChatSession
+from turnstone.core.storage._registry import get_storage
+
+# ---------------------------------------------------------------------------
+# Helpers (mirrors test_skills.py)
+# ---------------------------------------------------------------------------
+
+
+class NullUI:
+    """UI adapter that discards all output."""
+
+    def on_thinking_start(self):
+        pass
+
+    def on_thinking_stop(self):
+        pass
+
+    def on_reasoning_token(self, text):
+        pass
+
+    def on_content_token(self, text):
+        pass
+
+    def on_stream_end(self):
+        pass
+
+    def approve_tools(self, items):
+        return True, None
+
+    def on_tool_result(self, call_id, name, output, **kwargs):
+        pass
+
+    def on_tool_output_chunk(self, call_id, chunk):
+        pass
+
+    def on_status(self, usage, context_window, effort):
+        pass
+
+    def on_plan_review(self, content):
+        return ""
+
+    def on_info(self, message):
+        pass
+
+    def on_error(self, message):
+        pass
+
+    def on_state_change(self, state):
+        pass
+
+    def on_rename(self, name):
+        pass
+
+    def on_output_warning(self, call_id, assessment):
+        pass
+
+
+def _make_session(**kwargs: Any) -> ChatSession:
+    defaults: dict[str, Any] = dict(
+        client=MagicMock(),
+        model="test-model",
+        ui=NullUI(),
+        instructions=None,
+        temperature=0.5,
+        max_tokens=4096,
+        tool_timeout=30,
+    )
+    defaults.update(kwargs)
+    return ChatSession(**defaults)
+
+
+def _create_skill(db: Any, skill_id: str, name: str, content: str, **kw: Any) -> None:
+    db.create_prompt_template(
+        template_id=skill_id,
+        name=name,
+        category=kw.get("category", "general"),
+        content=content,
+        variables=kw.get("variables", "[]"),
+        is_default=kw.get("is_default", False),
+        org_id="",
+        created_by="test",
+        origin="manual",
+        mcp_server="",
+        readonly=False,
+        description="",
+        tags="[]",
+        source_url="",
+        version="1.0.0",
+        author="",
+        activation=kw.get("activation", "named"),
+        token_estimate=0,
+        model="",
+        auto_approve=False,
+        temperature=None,
+        reasoning_effort="",
+        max_tokens=None,
+        token_budget=0,
+        agent_max_turns=None,
+        notify_on_complete="{}",
+        enabled=True,
+        allowed_tools="[]",
+        priority=0,
+    )
+
+
+def _sys_content(session: ChatSession) -> str:
+    msgs = [m for m in session.system_messages if m["role"] == "system"]
+    assert msgs
+    return msgs[0]["content"]
+
+
+# ---------------------------------------------------------------------------
+# Tests
+# ---------------------------------------------------------------------------
+
+
+class TestMaterializeResources:
+    def test_materialize_creates_files(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "test-skill", "Use the scripts.")
+        db.create_skill_resource("r1", "s1", "scripts/helper.py", "print('hello')")
+        db.create_skill_resource("r2", "s1", "references/api.md", "# API")
+
+        session = _make_session(skill="test-skill")
+        assert session._skill_resources_dir is not None
+        base = session._skill_resources_dir
+        assert os.path.isdir(base)
+
+        helper = os.path.join(base, "scripts", "helper.py")
+        assert os.path.isfile(helper)
+        with open(helper) as f:
+            assert f.read() == "print('hello')"
+
+        api_md = os.path.join(base, "references", "api.md")
+        assert os.path.isfile(api_md)
+        with open(api_md) as f:
+            assert f.read() == "# API"
+
+        session.close()
+
+    def test_scripts_executable(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "exec-skill", "Run scripts/run.sh")
+        db.create_skill_resource("r1", "s1", "scripts/run.sh", "#!/bin/bash\necho hi")
+
+        session = _make_session(skill="exec-skill")
+        base = session._skill_resources_dir
+        run_sh = os.path.join(base, "scripts", "run.sh")
+        mode = os.stat(run_sh).st_mode
+        assert mode & stat.S_IXUSR  # owner execute
+        session.close()
+
+    def test_non_scripts_not_executable(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "ref-skill", "Read references/guide.md")
+        db.create_skill_resource("r1", "s1", "references/guide.md", "# Guide")
+
+        session = _make_session(skill="ref-skill")
+        base = session._skill_resources_dir
+        guide = os.path.join(base, "references", "guide.md")
+        mode = os.stat(guide).st_mode
+        assert not (mode & stat.S_IXUSR)  # not executable
+        session.close()
+
+    def test_cleanup_on_close(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "cleanup-skill", "content")
+        db.create_skill_resource("r1", "s1", "scripts/a.py", "code")
+
+        session = _make_session(skill="cleanup-skill")
+        base = session._skill_resources_dir
+        assert os.path.isdir(base)
+
+        session.close()
+        assert not os.path.exists(base)
+        assert session._skill_resources_dir is None
+
+    def test_cleanup_on_skill_switch(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "skill-a", "Skill A")
+        db.create_skill_resource("r1", "s1", "scripts/a.py", "code_a")
+        _create_skill(db, "s2", "skill-b", "Skill B")
+        db.create_skill_resource("r2", "s2", "scripts/b.py", "code_b")
+
+        session = _make_session(skill="skill-a")
+        dir_a = session._skill_resources_dir
+        assert os.path.isfile(os.path.join(dir_a, "scripts", "a.py"))
+
+        session.set_skill("skill-b")
+        dir_b = session._skill_resources_dir
+        assert dir_b != dir_a
+        assert not os.path.exists(dir_a)
+        assert os.path.isfile(os.path.join(dir_b, "scripts", "b.py"))
+
+        session.close()
+
+    def test_cleanup_on_skill_clear(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "clear-skill", "content")
+        db.create_skill_resource("r1", "s1", "scripts/x.py", "code")
+
+        session = _make_session(skill="clear-skill")
+        base = session._skill_resources_dir
+        assert os.path.isdir(base)
+
+        session.set_skill(None)
+        assert not os.path.exists(base)
+        assert session._skill_resources_dir is None
+
+        session.close()
+
+    def test_empty_resources_no_dir(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "no-res-skill", "content")
+        # No resources added
+
+        session = _make_session(skill="no-res-skill")
+        assert session._skill_resources_dir is None
+        session.close()
+
+    def test_no_skill_no_dir(self, tmp_db):
+        session = _make_session()
+        assert session._skill_resources_dir is None
+        session.close()
+
+    def test_path_traversal_rejected(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "traversal-skill", "content")
+        # Inject a malicious path directly into storage
+        db.create_skill_resource("r1", "s1", "../etc/passwd", "bad content")
+        db.create_skill_resource("r2", "s1", "scripts/good.py", "good content")
+
+        session = _make_session(skill="traversal-skill")
+        base = session._skill_resources_dir
+        # The traversal path must not be written inside the resources dir
+        assert not os.path.exists(os.path.join(base, "etc"))
+        # The good resource should still be materialized
+        assert os.path.isfile(os.path.join(base, "scripts", "good.py"))
+        session.close()
+
+
+class TestSkillResourceEnv:
+    def test_env_with_resources(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "env-skill", "content")
+        db.create_skill_resource("r1", "s1", "scripts/tool.py", "code")
+
+        session = _make_session(skill="env-skill")
+        env = session._skill_resource_env()
+        assert env["SKILL_RESOURCES_DIR"] == session._skill_resources_dir
+        assert "PATH" in env
+        scripts_dir = os.path.join(session._skill_resources_dir, "scripts")
+        assert env["PATH"].startswith(scripts_dir + ":")
+        session.close()
+
+    def test_env_without_scripts_dir(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "no-scripts-skill", "content")
+        db.create_skill_resource("r1", "s1", "references/doc.md", "# Doc")
+
+        session = _make_session(skill="no-scripts-skill")
+        env = session._skill_resource_env()
+        assert "SKILL_RESOURCES_DIR" in env
+        # No scripts/ subdir so PATH should not be overridden
+        assert "PATH" not in env
+        session.close()
+
+    def test_env_empty_when_no_resources(self, tmp_db):
+        session = _make_session()
+        assert session._skill_resource_env() == {}
+        session.close()
+
+
+class TestSystemMessageHint:
+    def test_hint_present_when_resources_exist(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "hint-skill", "Use the bundled scripts.")
+        db.create_skill_resource("r1", "s1", "scripts/run.py", "code")
+
+        session = _make_session(skill="hint-skill")
+        content = _sys_content(session)
+        assert "$SKILL_RESOURCES_DIR" in content
+        assert "scripts/ are on PATH" in content
+        session.close()
+
+    def test_no_hint_when_no_resources(self, tmp_db):
+        db = get_storage()
+        _create_skill(db, "s1", "plain-skill", "No resources here.")
+
+        session = _make_session(skill="plain-skill")
+        content = _sys_content(session)
+        assert "SKILL_RESOURCES_DIR" not in content
+        session.close()
+
+
+class TestMaterializeEdgeCases:
+    def test_all_resources_rejected_no_dir(self, tmp_db):
+        """When every resource fails path validation, no temp dir is left."""
+        db = get_storage()
+        _create_skill(db, "s1", "all-bad", "content")
+        db.create_skill_resource("r1", "s1", "../escape", "bad")
+        db.create_skill_resource("r2", "s1", "/absolute", "bad")
+
+        session = _make_session(skill="all-bad")
+        assert session._skill_resources_dir is None
+        session.close()
+
+    def test_dot_path_rejected(self, tmp_db):
+        """A bare '.' path is rejected rather than crashing."""
+        db = get_storage()
+        _create_skill(db, "s1", "dot-skill", "content")
+        db.create_skill_resource("r1", "s1", ".", "bad")
+        db.create_skill_resource("r2", "s1", "scripts/ok.py", "good")
+
+        session = _make_session(skill="dot-skill")
+        base = session._skill_resources_dir
+        assert os.path.isfile(os.path.join(base, "scripts", "ok.py"))
+        session.close()
+
+    def test_empty_path_rejected(self, tmp_db):
+        """An empty string path is rejected."""
+        db = get_storage()
+        _create_skill(db, "s1", "empty-skill", "content")
+        db.create_skill_resource("r1", "s1", "", "bad")
+        db.create_skill_resource("r2", "s1", "scripts/ok.py", "good")
+
+        session = _make_session(skill="empty-skill")
+        assert session._skill_resources_dir is not None
+        session.close()
+
+    def test_nested_traversal_rejected(self, tmp_db):
+        """Traversal hidden inside a valid prefix is still caught."""
+        db = get_storage()
+        _create_skill(db, "s1", "nested-skill", "content")
+        db.create_skill_resource("r1", "s1", "scripts/../../../etc/passwd", "bad")
+        db.create_skill_resource("r2", "s1", "scripts/ok.py", "good")
+
+        session = _make_session(skill="nested-skill")
+        base = session._skill_resources_dir
+        assert not os.path.exists(os.path.join(base, "etc"))
+        assert os.path.isfile(os.path.join(base, "scripts", "ok.py"))
+        session.close()
+
+    def test_double_close_idempotent(self, tmp_db):
+        """Calling close() twice does not raise."""
+        db = get_storage()
+        _create_skill(db, "s1", "double-skill", "content")
+        db.create_skill_resource("r1", "s1", "scripts/x.py", "code")
+
+        session = _make_session(skill="double-skill")
+        session.close()
+        session.close()  # must not raise
+
+
+class TestPreflightValidation:
+    def test_missing_resource_warns(self, tmp_db):
+        """Skill content references a script not in resources."""
+        db = get_storage()
+        _create_skill(db, "s1", "warn-skill", "Run scripts/missing.py to start.")
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="warn-skill")
+        ui.on_info.assert_called_once()
+        msg = ui.on_info.call_args[0][0]
+        assert "scripts/missing.py" in msg
+        assert "warn-skill" in msg
+        session.close()
+
+    def test_all_resources_present_no_warn(self, tmp_db):
+        """No warning when all referenced paths are bundled."""
+        db = get_storage()
+        _create_skill(db, "s1", "ok-skill", "Run scripts/helper.py for help.")
+        db.create_skill_resource("r1", "s1", "scripts/helper.py", "print('hi')")
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="ok-skill")
+        ui.on_info.assert_not_called()
+        session.close()
+
+    def test_no_references_no_warn(self, tmp_db):
+        """Skill content with no resource paths triggers no validation warning."""
+        db = get_storage()
+        _create_skill(db, "s1", "plain-skill", "Just a plain skill with no paths.")
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="plain-skill")
+        ui.on_info.assert_not_called()
+        session.close()
+
+    def test_multiple_missing_warns_once(self, tmp_db):
+        """Multiple missing resources produce a single warning listing all."""
+        db = get_storage()
+        _create_skill(
+            db,
+            "s1",
+            "multi-skill",
+            "Use scripts/a.py and scripts/b.sh to process references/guide.md",
+        )
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="multi-skill")
+        ui.on_info.assert_called_once()
+        msg = ui.on_info.call_args[0][0]
+        assert "3 resource(s)" in msg
+        assert "scripts/a.py" in msg
+        assert "scripts/b.sh" in msg
+        assert "references/guide.md" in msg
+        session.close()
+
+    def test_validation_skipped_no_skill(self, tmp_db):
+        """No crash or warning when no skill is active."""
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui)
+        ui.on_info.assert_not_called()
+        session.close()
+
+    def test_json_extension_not_truncated(self, tmp_db):
+        """assets/config.json should match as .json, not .js."""
+        db = get_storage()
+        _create_skill(db, "s1", "json-skill", "Load assets/config.json for settings.")
+        db.create_skill_resource("r1", "s1", "assets/config.json", "{}")
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="json-skill")
+        ui.on_info.assert_not_called()
+        session.close()
+
+    def test_compound_prefix_not_matched(self, tmp_db):
+        """'myscripts/tool.py' should not match as 'scripts/tool.py'."""
+        db = get_storage()
+        _create_skill(
+            db,
+            "s1",
+            "compound-skill",
+            "The myscripts/tool.py file is unrelated.",
+        )
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="compound-skill")
+        ui.on_info.assert_not_called()
+        session.close()
+
+    def test_extension_suffix_not_matched(self, tmp_db):
+        """'scripts/tool.python' should not match as 'scripts/tool.py'."""
+        db = get_storage()
+        _create_skill(
+            db,
+            "s1",
+            "suffix-skill",
+            "Run scripts/tool.python to start.",
+        )
+
+        ui = NullUI()
+        ui.on_info = MagicMock()
+        session = _make_session(ui=ui, skill="suffix-skill")
+        ui.on_info.assert_not_called()
+        session.close()

turnstone/core/session.py

@@ -19,6 +19,7 @@ import mimetypes
 import os
 import queue
 import re
+import shutil
 import signal
 import subprocess
 import tempfile
@@ -161,6 +162,13 @@ _IMAGE_SIZE_CAP: int = 4 * 1024 * 1024
 # Upper bound on total skill content injected into system messages
 _MAX_SKILL_CONTENT: int = 32768
 
+# Matches resource paths referenced in skill content (scripts/foo.py, etc.)
+_RESOURCE_PATH_RE = re.compile(
+    r"(?<![/\w-])(?:scripts|references|assets)/[\w./-]+\."
+    r"(?:json|yaml|yml|toml|cfg|ini|py|sh|js|ts|md|txt)"
+    r"(?=[\s)\]}'\"`,;:\x60]|$)"
+)
+
 
 _TEMPLATE_VAR_RE = re.compile(r"\{\{(\w+)\}\}")
 
@@ -417,6 +425,7 @@ class ChatSession:
         self._skill_name: str | None = skill
         self._skill_content: str | None = None
         self._skill_resources: dict[str, str] = {}
+        self._skill_resources_dir: str | None = None
         self._load_skills()
         self._init_system_messages()
         self._save_config()
@@ -583,6 +592,8 @@ class ChatSession:
             else:
                 self._skill_content = None
             self._skill_resources = {}
+        self._materialize_skill_resources()
+        self._validate_skill_resources()
 
     def set_skill(self, name: str | None) -> None:
         """Set or clear the active skill."""
@@ -613,6 +624,81 @@ class ChatSession:
             log.warning("skill_resources.load_failed", skill_id=skill_id, exc_info=True)
             return {}
 
+    def _cleanup_skill_resources(self) -> None:
+        """Remove materialized skill resources from disk."""
+        d = self._skill_resources_dir
+        if d is not None:
+            shutil.rmtree(d, ignore_errors=True)
+            self._skill_resources_dir = None
+
+    def _materialize_skill_resources(self) -> None:
+        """Write skill resources to a temp directory for subprocess access."""
+        self._cleanup_skill_resources()
+        if not self._skill_resources:
+            return
+        base = tempfile.mkdtemp(prefix=f"skill-{self._ws_id[:8]}-")
+        written = 0
+        for rel_path, content in self._skill_resources.items():
+            normed = os.path.normpath(rel_path)
+            if not normed or normed == "." or normed.startswith(("..", "/")):
+                log.warning("skill_resources.bad_path", path=rel_path)
+                continue
+            if ".." in normed.split(os.sep):
+                log.warning("skill_resources.bad_path", path=rel_path)
+                continue
+            full = os.path.join(base, normed)
+            if not os.path.realpath(full).startswith(os.path.realpath(base)):
+                log.warning("skill_resources.path_escape", path=rel_path)
+                continue
+            try:
+                os.makedirs(os.path.dirname(full), exist_ok=True)
+                with open(full, "w", encoding="utf-8") as f:
+                    f.write(content)
+                if normed.startswith("scripts/"):
+                    os.chmod(full, 0o755)
+                written += 1
+            except Exception:
+                log.warning("skill_resources.write_failed", path=rel_path, exc_info=True)
+        if written == 0:
+            shutil.rmtree(base, ignore_errors=True)
+            return
+        self._skill_resources_dir = base
+        log.info(
+            "skill_resources.materialized",
+            dir=base,
+            count=written,
+        )
+
+    def _skill_resource_env(self) -> dict[str, str]:
+        """Return extra env vars for bash when skill resources are materialized."""
+        if not self._skill_resources_dir:
+            return {}
+        env: dict[str, str] = {"SKILL_RESOURCES_DIR": self._skill_resources_dir}
+        scripts_dir = os.path.join(self._skill_resources_dir, "scripts")
+        if os.path.isdir(scripts_dir):
+            current_path = os.environ.get("PATH")
+            if current_path:
+                env["PATH"] = scripts_dir + os.pathsep + current_path
+            else:
+                env["PATH"] = scripts_dir
+        return env
+
+    def _validate_skill_resources(self) -> None:
+        """Warn if skill content references resource paths not in skill_resources."""
+        if not self._skill_content or not self._skill_name:
+            return
+        referenced = {os.path.normpath(p) for p in _RESOURCE_PATH_RE.findall(self._skill_content)}
+        if not referenced:
+            return
+        available = {os.path.normpath(p) for p in self._skill_resources}
+        missing = sorted(referenced - available)
+        if missing:
+            log.warning("skill_resources.missing", skill=self._skill_name, paths=missing)
+            self.ui.on_info(
+                f"Skill '{self._skill_name}' references {len(missing)} resource(s) "
+                f"not bundled: {', '.join(missing)}"
+            )
+
     # -- MCP tool refresh ----------------------------------------------------
 
     def _on_mcp_tools_changed(self) -> None:
@@ -747,6 +833,7 @@ class ChatSession:
             self._mcp_prompt_cb = None
         if self._watch_runner:
             self._watch_runner.remove_dispatch_fn(self._ws_id)
+        self._cleanup_skill_resources()
 
     def _handle_mcp_refresh(self, arg: str) -> None:
         """Handle ``/mcp refresh [server]``."""
@@ -1091,6 +1178,12 @@ class ChatSession:
                         "Resource content omitted (total exceeds 8KB). "
                         "Resource files are listed above by path and size."
                     )
+                if self._skill_resources_dir:
+                    lines.append(
+                        "\nResource files are materialized on disk. "
+                        "Scripts in scripts/ are on PATH and can be run by name. "
+                        "All files are under $SKILL_RESOURCES_DIR."
+                    )
                 lines.append("</skill-resources>")
                 dev_parts.append("\n".join(lines))
         # Skill catalog: disclose search-activated skills so the model
@@ -4338,7 +4431,7 @@ class ChatSession:
                     stderr=subprocess.PIPE,
                     text=True,
                     start_new_session=True,
-                    env=scrubbed_env(),
+                    env=scrubbed_env(extra=self._skill_resource_env()),
                 )
                 with self._procs_lock:
                     self._active_procs.add(proc)