diff --git a/apps/api/src/services/tool/instance_service.py b/apps/api/src/services/tool/instance_service.py index 4849185..82ab4c4 100644 --- a/apps/api/src/services/tool/instance_service.py +++ b/apps/api/src/services/tool/instance_service.py @@ -76,7 +76,6 @@ from src.services.shared.readiness_probe import execute_probe from src.services.shared.ssh_keys import prepare_ssh_key_files from src.services.instance.event_bus import InstanceEventBus from src.services.instance.lifecycle_hooks import publish_lifecycle_event -from src.utils.git_url_parser import extract_base_repo_url logger = logging.getLogger(__name__) _event_bus = InstanceEventBus() @@ -281,9 +280,7 @@ def clone_git_repo( # Include the branch in the hash so different branches of the same repo # get separate clone directories and cannot race each other. branch_segment = branch or "default" - url_hash = hashlib.md5( - f"{remote_url}:{branch_segment}".encode() - ).hexdigest()[:12] + url_hash = hashlib.md5(f"{remote_url}:{branch_segment}".encode()).hexdigest()[:12] repo_name = remote_url.split("/")[-1].replace(".git", "") or "repo" clone_dir = os.path.join(clone_parent, "git-mounts", f"{repo_name}-{url_hash}") repo_path = clone_repository( @@ -1028,7 +1025,9 @@ async def prepare_manifest_instance( workspace: Workspace | None = None if instance.workspace_id: workspace = await session.get(Workspace, instance.workspace_id) - project = await session.get(Project, instance.project_id) if instance.project_id else None + project = ( + await session.get(Project, instance.project_id) if instance.project_id else None + ) repo_name = ( _get_repository_mount_name(project, repo, workspace) if project and repo @@ -1087,6 +1086,13 @@ async def create_tool_instance( if repo is None or repo.project_id != project_id: raise ValueError("repository not found") + # Resolve the project up front: it is needed both for auto-generated display + # names and for the in-container mount name (/home/user/{project_name}), + # regardless of the tool definition type below. + project = await session.get(Project, project_id) + if project is None: + raise ValueError("project not found") + tool_type_id = uuid.UUID(data.tool_type_id) tool_type = await session.get(ToolType, tool_type_id) if tool_type is None: @@ -1120,7 +1126,6 @@ async def create_tool_instance( if data.display_name: instance_display = data.display_name else: - project = await session.get(Project, project_id) project_name = project.name if project else "Unknown" scope_name = workspace.name if workspace else repo.name auto_name = f"{project_name} / {scope_name} / {tool_type.display_name}" @@ -1348,7 +1353,9 @@ async def start_tool_instance( # Fetch tool type early to determine home directory and container user tool_type = await session.get(ToolType, instance.tool_type_id) - home_dir = tool_type.home_directory if tool_type and tool_type.home_directory else "/root" + home_dir = ( + tool_type.home_directory if tool_type and tool_type.home_directory else "/root" + ) container_uid = 0 container_gid = 0 if tool_type and tool_type.definition_type == "manifest" and tool_type.manifest_id: @@ -1583,8 +1590,16 @@ async def start_tool_instance( else: # ── LEGACY FLOW ────────────────────────────────────────── if instance.clone_mode == "clone" and not instance.workspace_id: - clone_project = await session.get(Project, instance.project_id) if instance.project_id else None - clone_name = _slugify_directory_name(clone_project.name) if clone_project else "repo-clone" + clone_project = ( + await session.get(Project, instance.project_id) + if instance.project_id + else None + ) + clone_name = ( + _slugify_directory_name(clone_project.name) + if clone_project + else "repo-clone" + ) repo_path = os.path.join(instance_dir, clone_name) repo = await session.get(GitRepository, instance.repository_id) @@ -1613,11 +1628,13 @@ async def start_tool_instance( ) # Ensure the legacy compose mounts the project-named clone directory. - extra_volumes.append({ - "source": repo_path, - "target": f"{home_dir}/{clone_name}", - "type": "bind", - }) + extra_volumes.append( + { + "source": repo_path, + "target": f"{home_dir}/{clone_name}", + "type": "bind", + } + ) if port_override or start_command or working_directory or extra_volumes: modify_compose_file( @@ -2124,8 +2141,16 @@ async def delete_tool_instance( os.path.dirname(instance.compose_path) if instance.compose_path else None ) if instance_dir: - clone_project = await session.get(Project, instance.project_id) if instance.project_id else None - clone_name = _slugify_directory_name(clone_project.name) if clone_project else "repo-clone" + clone_project = ( + await session.get(Project, instance.project_id) + if instance.project_id + else None + ) + clone_name = ( + _slugify_directory_name(clone_project.name) + if clone_project + else "repo-clone" + ) clone_path = os.path.join(instance_dir, clone_name) if not os.path.exists(clone_path): clone_path = os.path.join(instance_dir, "repo-clone") diff --git a/apps/api/tests/unit/test_instance_service.py b/apps/api/tests/unit/test_instance_service.py index 79b1c8c..63457c5 100644 --- a/apps/api/tests/unit/test_instance_service.py +++ b/apps/api/tests/unit/test_instance_service.py @@ -1,5 +1,6 @@ """Unit tests for the tool instance service.""" +import uuid from unittest.mock import MagicMock, AsyncMock import pytest @@ -70,9 +71,7 @@ class TestGetRepositoryMountName: class TestStackProfileMountsWithGitMounts: """Tests for _stack_profile_mounts_with_git_mounts.""" - def test_exact_overlap_merges_profile_files_into_git_source( - self, tmp_path - ) -> None: + def test_exact_overlap_merges_profile_files_into_git_source(self, tmp_path) -> None: """When a profile mount targets the same directory as a git mount, the profile files should be copied into the git-mount source so the container sees both sets of files through one bind mount.""" @@ -310,4 +309,100 @@ async def test_prepare_manifest_instance_uses_workspace_path_basename(): instance_service.subprocess.run = original_run assert "WORKSPACE_NAME: acme" in compose_content - assert "/data/working-copies/workspace-uuid/headquarter:/home/user/acme" in compose_content + assert ( + "/data/working-copies/workspace-uuid/headquarter:/home/user/acme" + in compose_content + ) + + +@pytest.mark.unit +async def test_create_tool_instance_resolves_project_when_display_name_given( + monkeypatch, tmp_path +): + """Regression: create_tool_instance must resolve `project` even when a + display_name is supplied. + + Previously `project` was only fetched inside the auto-display-name branch, + so providing display_name left it unbound and the manifest/dockerfile/compose + branches crashed with UnboundLocalError at `_get_repository_mount_name`. + The mount name must derive from the project name regardless. + """ + from src.schemas.tool import CreateInstanceRequest + from src.services.tool import instance_service + + project_id = uuid.uuid4() + repo_id = uuid.uuid4() + user_id = uuid.uuid4() + tool_type_id = uuid.uuid4() + manifest_id = uuid.uuid4() + + repo = MagicMock() + repo.project_id = project_id + repo.name = "src" + repo.path = "/data/repos/src" + + project = MagicMock() + project.name = "My Project" # slugifies to "my-project" + + tool_type = MagicMock() + tool_type.id = tool_type_id + tool_type.name = "pi-agent" + tool_type.definition_type = "manifest" + tool_type.manifest_id = manifest_id + tool_type.home_directory = None + + manifest_def = MagicMock() + manifest_def.manifest = { + "base_image": "ubuntu:24.04", + "interface_type": "terminal", + "user": {"name": "user", "uid": 1001, "gid": 1001}, + } + manifest_def.base_definition_id = None + + async def session_get(model, _obj_id): + name = getattr(model, "__name__", "") + if name == "GitRepository": + return repo + if name == "Project": + return project + if name == "ToolType": + return tool_type + if name == "ToolDefinitionManifest": + return manifest_def + return None + + session = AsyncMock() + session.get.side_effect = session_get + session.add = MagicMock() + + monkeypatch.setattr( + instance_service, "validate_config_profile", AsyncMock(return_value=None) + ) + monkeypatch.setattr( + instance_service, "ensure_instance_directory", lambda _name: str(tmp_path) + ) + monkeypatch.setattr(instance_service, "find_free_port", lambda: 12345) + monkeypatch.setattr( + instance_service, "compute_image_tag", lambda *a, **k: "img:tag" + ) + compile_compose = MagicMock(return_value="services:\n app: {}") + monkeypatch.setattr(instance_service, "compile_compose", compile_compose) + monkeypatch.setattr(instance_service, "write_compose_file", MagicMock()) + monkeypatch.setattr(instance_service, "publish_lifecycle_event", AsyncMock()) + + data = CreateInstanceRequest( + tool_type_id=str(tool_type_id), display_name="Custom Name" + ) + + instance = await instance_service.create_tool_instance( + session, user_id, project_id, repo_id, data + ) + + # Must not raise UnboundLocalError; instance returned. + assert instance is not None + + # The mount name must derive from project.name, proving `project` was + # resolved on the display_name path (the previous crash site). + variables = compile_compose.call_args.args[1] + assert variables["REPO_NAME"] == "my-project" + assert variables["WORKSPACE_NAME"] == "my-project"