update after comments

Author: VascoSch92

openhands-sdk/openhands/sdk/skills/fork.py

@@ -8,13 +8,24 @@
 
 from __future__ import annotations
 
+import re
 from pathlib import Path
 from typing import TYPE_CHECKING
 
 from openhands.sdk.logger import get_logger
 from openhands.sdk.skills.execute import render_content_with_commands
 
 
+# Skill names are not guaranteed to be filesystem-safe:
+# - Legacy skills derive their name from a relative path, which can contain
+#   "/" (e.g. "subdir/my_skill") and would create unintended nested dirs.
+# - Programmatic Skill(name=...) has no name validator; a crafted name like
+#   "../../etc/passwd" would escape the forks directory.
+# AgentSkills-format skills ARE validated (lowercase [a-z0-9-]) but we
+# sanitize unconditionally to keep the fork persistence layout predictable.
+_UNSAFE_PATH_CHARS = re.compile(r"[^a-zA-Z0-9_-]")
+
+
 if TYPE_CHECKING:
     from openhands.sdk.agent.base import AgentBase
     from openhands.sdk.skills.skill import Skill
@@ -47,19 +58,23 @@ def run_skill_forked(
         working_dir=Path(working_dir) if working_dir else None,
     )
 
-    # Strip skills from the subagent's context so forked skills cannot
-    # re-trigger and cause infinite recursion. Everything else (system_message_suffix,
-    # secrets, datetime) is preserved.
-    sub_agent_context = (
-        agent.agent_context.model_copy(update={"skills": []})
-        if agent.agent_context is not None
-        else None
-    )
+    # Strip fork-context skills from the subagent so forked skills cannot
+    # re-trigger themselves (direct recursion) or each other (A → B → A loops).
+    # Inline skills are kept: they only inject static content and are safe to
+    # reuse inside a forked subagent. Everything else on agent_context
+    # (system_message_suffix, secrets, datetime) is preserved.
+    parent_context = agent.agent_context
+    if parent_context is not None:
+        safe_skills = [s for s in parent_context.skills if s.context != "fork"]
+        sub_agent_context = parent_context.model_copy(update={"skills": safe_skills})
+    else:
+        sub_agent_context = None
     sub_agent = agent.model_copy(update={"agent_context": sub_agent_context})
 
     fork_persistence_dir: str | None = None
     if persistence_dir is not None:
-        fork_persistence_dir = str(Path(persistence_dir) / "forks" / skill.name)
+        safe_name = _UNSAFE_PATH_CHARS.sub("_", skill.name)
+        fork_persistence_dir = str(Path(persistence_dir) / "forks" / safe_name)
 
     sub_conv = Conversation(
         agent=sub_agent,
@@ -79,5 +94,5 @@ def run_skill_forked(
     finally:
         try:
             sub_conv.close()
-        except Exception:
-            logger.debug("Ignoring error closing forked sub-conversation")
+        except Exception as e:
+            logger.debug("Ignoring error closing forked sub-conversation: %s", e)

tests/sdk/skills/test_skill_fork_execution.py

@@ -153,15 +153,18 @@ def test_fork_skill_persistence_dir_forwarded(persistence_dir):
 
 
 @pytest.mark.parametrize(
-    "persistence_dir, expected",
+    "persistence_dir, skill_name, expected",
     [
-        (None, None),
-        ("/state/abc123", "/state/abc123/forks/ddebug"),
+        (None, "ddebug", None),
+        ("/state/abc123", "ddebug", "/state/abc123/forks/ddebug"),
+        # Path-unsafe characters are sanitized to avoid nested dirs or traversal
+        ("/state/abc123", "subdir/my_skill", "/state/abc123/forks/subdir_my_skill"),
+        ("/state/abc123", "../evil", "/state/abc123/forks/___evil"),
     ],
 )
-def test_fork_persistence_dir_path_construction(persistence_dir, expected):
-    """builds <persistence_dir>/forks/<skill.name> before passing to Conversation."""
-    skill = _fork_skill()
+def test_fork_persistence_dir_path_construction(persistence_dir, skill_name, expected):
+    """builds <persistence_dir>/forks/<safe_name> before passing to Conversation."""
+    skill = _fork_skill(name=skill_name)
     mock_agent = MagicMock()
     mock_agent.agent_context = None
 
@@ -204,3 +207,37 @@ def test_fork_skill_fallback_when_agent_or_working_dir_missing(
     assert result is not None
     content, _ = result
     assert "inline fallback content" in content.text
+
+
+def test_subagent_context_keeps_inline_skills_drops_forks():
+    """Forked subagent retains inline skills but not other fork skills,
+    and preserves system_message_suffix."""
+    fork_skill = _fork_skill(name="ddebug")
+    other_fork = _fork_skill(name="other_fork")
+    inline_skill = Skill(
+        name="inline_helper",
+        content="inline body",
+        trigger=KeywordTrigger(keywords=["helper"]),
+        context="inline",
+    )
+    parent_ctx = AgentContext(
+        skills=[fork_skill, other_fork, inline_skill],
+        system_message_suffix="preserve me",
+    )
+    mock_agent = MagicMock()
+    mock_agent.agent_context = parent_ctx
+    # model_copy on the agent itself: capture what gets passed
+    mock_agent.model_copy.return_value = MagicMock()
+
+    with patch(
+        "openhands.sdk.conversation.conversation.Conversation"
+    ) as MockConversation:
+        mock_conv = MagicMock()
+        mock_conv.state.events = []
+        MockConversation.return_value = mock_conv
+        run_skill_forked(fork_skill, mock_agent, "/workspace")
+
+    _, agent_copy_kwargs = mock_agent.model_copy.call_args
+    sub_ctx = agent_copy_kwargs["update"]["agent_context"]
+    assert [s.name for s in sub_ctx.skills] == ["inline_helper"]
+    assert sub_ctx.system_message_suffix == "preserve me"