From abadf6c5532b137333eac4cb0eb4965b569462ad Mon Sep 17 00:00:00 2001 From: Dan Childers Date: Fri, 17 Jul 2026 14:35:52 -0400 Subject: [PATCH] refactor: centralize skills_install_dir in Settings The path `Path(tempfile.gettempdir()) / "forge" / "skills"` was hardcoded identically in agent.py, worker.py, and runner.py. Define it once as a derived property on Settings and import at each call site. Also adds a comment in runner.py noting the assumption that worker and runner share the same host filesystem for skill mounts. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Dan Childers --- src/forge/config.py | 8 ++ src/forge/integrations/agents/agent.py | 4 +- src/forge/orchestrator/worker.py | 7 +- src/forge/sandbox/runner.py | 7 +- src/forge/skills/orchestrator.py | 22 ++-- src/forge/skills/resolver.py | 32 ++++-- tests/unit/integrations/agents/test_agent.py | 2 +- tests/unit/orchestrator/test_worker.py | 14 ++- tests/unit/skills/test_orchestrator.py | 112 +++++++++++++++++++ tests/unit/skills/test_resolver.py | 54 +++++++++ 10 files changed, 238 insertions(+), 24 deletions(-) diff --git a/src/forge/config.py b/src/forge/config.py index e1bc7db9..adb0eaad 100644 --- a/src/forge/config.py +++ b/src/forge/config.py @@ -1,7 +1,9 @@ """Configuration management using Pydantic settings.""" import logging +import tempfile from functools import cached_property, lru_cache +from pathlib import Path from typing import TYPE_CHECKING, Literal from pydantic import Field, SecretStr @@ -255,6 +257,12 @@ def detect_model_provider(model_name: str) -> str: default="skills/", description="Base directory for skill resolution. The resolver finds skills/default/ and skills/{project}/ under this path.", ) + + @property + def skills_install_dir(self) -> Path: + """Directory for runtime-fetched skill packages.""" + return Path(tempfile.gettempdir()) / "forge" / "skills" + container_langchain_verbose: bool = Field( default=False, description="Enable LangChain verbose/debug logging in container", diff --git a/src/forge/integrations/agents/agent.py b/src/forge/integrations/agents/agent.py index 5ac52fd6..06952e01 100644 --- a/src/forge/integrations/agents/agent.py +++ b/src/forge/integrations/agents/agent.py @@ -214,7 +214,9 @@ def _get_skill_paths(self, ticket_key: str | None = None) -> list[str]: fallback to skills/default/ for any skill not overridden by the project. """ skills_dir = PROJECT_ROOT / self.settings.skills_dir.rstrip("/") - paths = resolve_skill_paths(ticket_key or "", skills_dir) + paths = resolve_skill_paths( + ticket_key or "", skills_dir, skills_install_dir=self.settings.skills_install_dir + ) logger.debug(f"Using skill paths: {paths}") return paths diff --git a/src/forge/orchestrator/worker.py b/src/forge/orchestrator/worker.py index dc1a0da9..a78e14b2 100644 --- a/src/forge/orchestrator/worker.py +++ b/src/forge/orchestrator/worker.py @@ -241,7 +241,12 @@ async def _process_workflow(self, message: QueueMessage) -> None: project_key = extract_project_key(ticket_key) jira_client = JiraClient() skills_dir = Path(self.settings.skills_dir) - await ensure_skills(project_key, jira_client, skills_dir) + await ensure_skills( + project_key, + jira_client, + skills_dir, + skills_install_dir=self.settings.skills_install_dir, + ) except Exception: logger.warning( "Skill synchronisation failed for %s; continuing with workflow.", diff --git a/src/forge/sandbox/runner.py b/src/forge/sandbox/runner.py index a422ef91..e0313ec0 100644 --- a/src/forge/sandbox/runner.py +++ b/src/forge/sandbox/runner.py @@ -196,8 +196,13 @@ def _get_skill_mounts( - container_paths: Comma-separated paths for AGENT_SKILL_PATHS env var """ skills_dir = Path.cwd() / self.settings.skills_dir.rstrip("/") + # Assumes worker and runner share the same host filesystem — skills_install_dir + # is populated by the worker and mounted into the container from the host. host_paths = [ - Path(p.rstrip("/")) for p in resolve_skill_paths(ticket_key or "", skills_dir) + Path(p.rstrip("/")) + for p in resolve_skill_paths( + ticket_key or "", skills_dir, skills_install_dir=self.settings.skills_install_dir + ) ] mounts = [] diff --git a/src/forge/skills/orchestrator.py b/src/forge/skills/orchestrator.py index 0b9b0c51..655dcecb 100644 --- a/src/forge/skills/orchestrator.py +++ b/src/forge/skills/orchestrator.py @@ -31,6 +31,8 @@ async def ensure_skills( project_key: str, jira_client: JiraClient, skills_dir: Path, + *, + skills_install_dir: Path | None = None, ) -> None: """Ensure all skills configured for *project_key* are installed. @@ -39,15 +41,16 @@ async def ensure_skills( and installs any entries whose SHA has changed or that are not yet installed. - The target directory for skills is ``skills_dir//`` - where ```` is the lowercase form of *project_key*. - Args: project_key: Jira project key (e.g., ``"MYPROJ"``). jira_client: Authenticated Jira client used to read the project property. - skills_dir: Root directory where skills are installed. The lock file - is read from ``skills_dir/skills.lock``. + skills_dir: Root directory for source-tracked skills (e.g. + ``skills/default/``). Used as the install target only when + *skills_install_dir* is not provided. + skills_install_dir: When provided, fetched skills and the lock file are + written here instead of *skills_dir*. Keeps runtime artifacts + out of the source tree. """ # ------------------------------------------------------------------ # 1. Fetch the forge.skills configuration from Jira. @@ -69,15 +72,18 @@ async def ensure_skills( return # ------------------------------------------------------------------ - # 2. Read the current lock file. + # 2. Determine install root (skills_install_dir if provided, else skills_dir). # ------------------------------------------------------------------ - lock_path = skills_dir / "skills.lock" + install_root = skills_install_dir if skills_install_dir is not None else skills_dir + install_root.mkdir(parents=True, exist_ok=True) + + lock_path = install_root / "skills.lock" lock = read_lock_file(lock_path) # ------------------------------------------------------------------ # 3. Determine the target directory (lowercase project key). # ------------------------------------------------------------------ - target_dir = skills_dir / project_key.lower() + target_dir = install_root / project_key.lower() # ------------------------------------------------------------------ # 4. Process each SkillEntry. diff --git a/src/forge/skills/resolver.py b/src/forge/skills/resolver.py index b404d12c..0bca8ded 100644 --- a/src/forge/skills/resolver.py +++ b/src/forge/skills/resolver.py @@ -4,12 +4,21 @@ logger = logging.getLogger(__name__) -def resolve_skill_paths(ticket_key: str, skills_dir: Path) -> list[str]: +def resolve_skill_paths( + ticket_key: str, + skills_dir: Path, + *, + skills_install_dir: Path | None = None, +) -> list[str]: """Return ordered skill source paths for Deep Agents. Deep Agents loads sources in order and deduplicates by skill name, - with later sources overriding earlier ones (last wins). Default comes - first; project override comes last so it wins on name collision. + with later sources overriding earlier ones (last wins). + + Resolution order (lowest to highest priority): + 1. ``skills_dir/default/`` — committed default skills + 2. ``skills_dir/{project}/`` — committed project overrides + 3. ``skills_install_dir/{project}/`` — runtime-fetched project skills """ default_dir = skills_dir / "default" @@ -18,11 +27,20 @@ def resolve_skill_paths(ticket_key: str, skills_dir: Path) -> list[str]: return [str(default_dir) + "/"] project = ticket_key.split("-")[0].lower() + paths: list[str] = [str(default_dir) + "/"] + override_dir = skills_dir / project + if override_dir.is_dir(): + paths.append(str(override_dir) + "/") + logger.info(f"Skills: committed override active for '{project}' ({override_dir})") - if not override_dir.is_dir(): + if skills_install_dir is not None: + cached_dir = skills_install_dir / project + if cached_dir.is_dir(): + paths.append(str(cached_dir) + "/") + logger.info(f"Skills: fetched skills active for '{project}' ({cached_dir})") + + if len(paths) == 1: logger.info(f"Skills: default only (no override for project '{project}')") - return [str(default_dir) + "/"] - logger.info(f"Skills: project override active for '{project}' ({override_dir})") - return [str(default_dir) + "/", str(override_dir) + "/"] + return paths diff --git a/tests/unit/integrations/agents/test_agent.py b/tests/unit/integrations/agents/test_agent.py index f179eb91..f3e78830 100644 --- a/tests/unit/integrations/agents/test_agent.py +++ b/tests/unit/integrations/agents/test_agent.py @@ -95,5 +95,5 @@ def test_get_skill_paths_returns_default_without_ticket_key(): mock_resolver.return_value = ["skills/default/"] result = agent._get_skill_paths(None) - mock_resolver.assert_called_once_with("", ANY) + mock_resolver.assert_called_once_with("", ANY, skills_install_dir=ANY) assert result == ["skills/default/"] diff --git a/tests/unit/orchestrator/test_worker.py b/tests/unit/orchestrator/test_worker.py index 30a7fe62..cfe65e4b 100644 --- a/tests/unit/orchestrator/test_worker.py +++ b/tests/unit/orchestrator/test_worker.py @@ -553,7 +553,7 @@ async def test_ensure_skills_receives_correct_project_key( """Project key extracted from ticket key is passed to ensure_skills.""" received: dict = {} - async def fake_ensure_skills(project_key, _jira_client, _skills_dir) -> None: + async def fake_ensure_skills(project_key, _jira_client, _skills_dir, **_kw) -> None: received["project_key"] = project_key with ( @@ -570,11 +570,14 @@ async def fake_ensure_skills(project_key, _jira_client, _skills_dir) -> None: async def test_ensure_skills_receives_skills_dir_from_settings( self, worker: OrchestratorWorker, jira_message: QueueMessage ): - """skills_dir passed to ensure_skills comes from settings.skills_dir.""" + """skills_dir and skills_install_dir passed to ensure_skills come from settings.""" received: dict = {} - async def fake_ensure_skills(_project_key, _jira_client, skills_dir) -> None: + async def fake_ensure_skills( + _project_key, _jira_client, skills_dir, *, skills_install_dir=None + ) -> None: received["skills_dir"] = skills_dir + received["skills_install_dir"] = skills_install_dir worker.settings.skills_dir = "custom/skills" @@ -587,6 +590,7 @@ async def fake_ensure_skills(_project_key, _jira_client, skills_dir) -> None: await worker._process_workflow(jira_message) assert received["skills_dir"] == Path("custom/skills") + assert received["skills_install_dir"] == worker.settings.skills_install_dir @pytest.mark.asyncio async def test_workflow_continues_when_ensure_skills_raises( @@ -653,7 +657,7 @@ async def test_jira_client_instantiated_for_ensure_skills( received: dict = {} fake_client_instance = MagicMock() - async def fake_ensure_skills(_project_key, jira_client, _skills_dir) -> None: + async def fake_ensure_skills(_project_key, jira_client, _skills_dir, **_kw) -> None: received["jira_client"] = jira_client with ( @@ -677,7 +681,7 @@ async def test_ensure_skills_skipped_gracefully_when_forge_skills_not_set( """ ensure_skills_called = False - async def fake_ensure_skills_no_property(project_key, jira_client, _skills_dir) -> None: + async def fake_ensure_skills_no_property(project_key, jira_client, _skills_dir, **_kw) -> None: """Simulate ensure_skills when forge.skills property is absent (returns None).""" nonlocal ensure_skills_called ensure_skills_called = True diff --git a/tests/unit/skills/test_orchestrator.py b/tests/unit/skills/test_orchestrator.py index e6c4e650..8c2980f9 100644 --- a/tests/unit/skills/test_orchestrator.py +++ b/tests/unit/skills/test_orchestrator.py @@ -566,3 +566,115 @@ async def test_handles_empty_list_from_invalid_json(self, tmp_path: Path, caplog mock_clone.assert_not_called() mock_update_lock.assert_not_called() assert "is empty" in caplog.text + + +# --------------------------------------------------------------------------- +# skills_install_dir support +# --------------------------------------------------------------------------- + + +class TestEnsureSkillsInstallDir: + @pytest.mark.asyncio + async def test_lock_file_written_to_skills_install_dir(self, tmp_path: Path) -> None: + """When skills_install_dir is provided, lock file is written there instead of skills_dir.""" + skills = tmp_path / "skills" + skills.mkdir() + cache = tmp_path / "cache" + + entry = _make_skill_entry() + jira_client = _make_jira_client([entry]) + + fake_clone = tmp_path / "clone" + (fake_clone / "skills").mkdir(parents=True) + + mock_cm = MagicMock() + mock_cm.__aenter__ = AsyncMock(return_value=fake_clone) + mock_cm.__aexit__ = AsyncMock(return_value=False) + + with ( + patch( + "forge.skills.orchestrator.resolve_ref_sha", + new=AsyncMock(return_value=RESOLVED_SHA), + ), + patch("forge.skills.orchestrator.should_fetch_entry", return_value=True), + patch("forge.skills.orchestrator.clone_context", return_value=mock_cm), + patch( + "forge.skills.orchestrator.install_path_mode", + return_value=["skill-a"], + ), + patch("forge.skills.orchestrator.update_lock_file") as mock_update_lock, + patch( + "forge.skills.orchestrator.read_lock_file", + return_value=LockFile(), + ), + ): + await ensure_skills(PROJECT_KEY, jira_client, skills, skills_install_dir=cache) + + lock_path_used = mock_update_lock.call_args[0][0] + assert lock_path_used == cache / "skills.lock" + assert "skills.lock" not in [p.name for p in skills.iterdir()] + + @pytest.mark.asyncio + async def test_skills_installed_to_skills_install_dir(self, tmp_path: Path) -> None: + """When skills_install_dir is provided, skills install to skills_install_dir/{project}.""" + skills = tmp_path / "skills" + skills.mkdir() + cache = tmp_path / "cache" + + entry = _make_skill_entry() + jira_client = _make_jira_client([entry]) + + fake_clone = tmp_path / "clone" + (fake_clone / "skills").mkdir(parents=True) + + mock_cm = MagicMock() + mock_cm.__aenter__ = AsyncMock(return_value=fake_clone) + mock_cm.__aexit__ = AsyncMock(return_value=False) + + captured_target_dir: list[Path] = [] + + def fake_install(_source_dir: Path, target_dir: Path) -> list[str]: + captured_target_dir.append(target_dir) + return ["skill-a"] + + with ( + patch( + "forge.skills.orchestrator.resolve_ref_sha", + new=AsyncMock(return_value=RESOLVED_SHA), + ), + patch("forge.skills.orchestrator.should_fetch_entry", return_value=True), + patch("forge.skills.orchestrator.clone_context", return_value=mock_cm), + patch( + "forge.skills.orchestrator.install_path_mode", + side_effect=fake_install, + ), + patch("forge.skills.orchestrator.update_lock_file"), + patch( + "forge.skills.orchestrator.read_lock_file", + return_value=LockFile(), + ), + ): + await ensure_skills(PROJECT_KEY, jira_client, skills, skills_install_dir=cache) + + assert captured_target_dir[0] == cache / "myproj" + + @pytest.mark.asyncio + async def test_skills_install_dir_created_if_missing(self, tmp_path: Path) -> None: + """skills_install_dir is created automatically when it does not exist.""" + cache = tmp_path / "nonexistent" / "cache" + assert not cache.exists() + + entry = _make_skill_entry() + jira_client = _make_jira_client([entry]) + + with ( + patch( + "forge.skills.orchestrator.resolve_ref_sha", + new=AsyncMock(return_value=RESOLVED_SHA), + ), + patch("forge.skills.orchestrator.should_fetch_entry", return_value=False), + patch("forge.skills.orchestrator.clone_context"), + ): + await ensure_skills(PROJECT_KEY, jira_client, tmp_path, skills_install_dir=cache) + + assert cache.is_dir() diff --git a/tests/unit/skills/test_resolver.py b/tests/unit/skills/test_resolver.py index e262a663..a88bea3b 100644 --- a/tests/unit/skills/test_resolver.py +++ b/tests/unit/skills/test_resolver.py @@ -49,3 +49,57 @@ def test_override_path_is_file_returns_default(skills_dir: Path) -> None: (skills_dir / "proj").touch() # file, not a directory result = resolve_skill_paths("PROJ-123", skills_dir) assert result == [str(skills_dir / "default") + "/"] + + +# --------------------------------------------------------------------------- +# skills_install_dir support +# --------------------------------------------------------------------------- + + +def test_skills_install_dir_appended_after_committed_override( + skills_dir: Path, tmp_path: Path +) -> None: + """When both committed override and cached skills exist, all three paths returned.""" + (skills_dir / "proj").mkdir() + cache = tmp_path / "cache" + (cache / "proj").mkdir(parents=True) + + result = resolve_skill_paths("PROJ-123", skills_dir, skills_install_dir=cache) + assert result == [ + str(skills_dir / "default") + "/", + str(skills_dir / "proj") + "/", + str(cache / "proj") + "/", + ] + + +def test_skills_install_dir_only_fetched(skills_dir: Path, tmp_path: Path) -> None: + """When only skills_install_dir has a project dir, returns default + cached.""" + cache = tmp_path / "cache" + (cache / "proj").mkdir(parents=True) + + result = resolve_skill_paths("PROJ-123", skills_dir, skills_install_dir=cache) + assert result == [ + str(skills_dir / "default") + "/", + str(cache / "proj") + "/", + ] + + +def test_skills_install_dir_none_preserves_old_behavior(skills_dir: Path) -> None: + """When skills_install_dir is None, behavior matches original (no extra paths).""" + (skills_dir / "proj").mkdir() + result = resolve_skill_paths("PROJ-123", skills_dir, skills_install_dir=None) + assert result == [ + str(skills_dir / "default") + "/", + str(skills_dir / "proj") + "/", + ] + + +def test_skills_install_dir_nonexistent_project_returns_default( + skills_dir: Path, tmp_path: Path +) -> None: + """When skills_install_dir exists but has no project subdir, returns default only.""" + cache = tmp_path / "cache" + cache.mkdir() + + result = resolve_skill_paths("PROJ-123", skills_dir, skills_install_dir=cache) + assert result == [str(skills_dir / "default") + "/"]