fix: resolve project variable in create_tool_instance
This commit is contained in:
@@ -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.shared.ssh_keys import prepare_ssh_key_files
|
||||||
from src.services.instance.event_bus import InstanceEventBus
|
from src.services.instance.event_bus import InstanceEventBus
|
||||||
from src.services.instance.lifecycle_hooks import publish_lifecycle_event
|
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__)
|
logger = logging.getLogger(__name__)
|
||||||
_event_bus = InstanceEventBus()
|
_event_bus = InstanceEventBus()
|
||||||
@@ -281,9 +280,7 @@ def clone_git_repo(
|
|||||||
# Include the branch in the hash so different branches of the same repo
|
# Include the branch in the hash so different branches of the same repo
|
||||||
# get separate clone directories and cannot race each other.
|
# get separate clone directories and cannot race each other.
|
||||||
branch_segment = branch or "default"
|
branch_segment = branch or "default"
|
||||||
url_hash = hashlib.md5(
|
url_hash = hashlib.md5(f"{remote_url}:{branch_segment}".encode()).hexdigest()[:12]
|
||||||
f"{remote_url}:{branch_segment}".encode()
|
|
||||||
).hexdigest()[:12]
|
|
||||||
repo_name = remote_url.split("/")[-1].replace(".git", "") or "repo"
|
repo_name = remote_url.split("/")[-1].replace(".git", "") or "repo"
|
||||||
clone_dir = os.path.join(clone_parent, "git-mounts", f"{repo_name}-{url_hash}")
|
clone_dir = os.path.join(clone_parent, "git-mounts", f"{repo_name}-{url_hash}")
|
||||||
repo_path = clone_repository(
|
repo_path = clone_repository(
|
||||||
@@ -1028,7 +1025,9 @@ async def prepare_manifest_instance(
|
|||||||
workspace: Workspace | None = None
|
workspace: Workspace | None = None
|
||||||
if instance.workspace_id:
|
if instance.workspace_id:
|
||||||
workspace = await session.get(Workspace, 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 = (
|
repo_name = (
|
||||||
_get_repository_mount_name(project, repo, workspace)
|
_get_repository_mount_name(project, repo, workspace)
|
||||||
if project and repo
|
if project and repo
|
||||||
@@ -1087,6 +1086,13 @@ async def create_tool_instance(
|
|||||||
if repo is None or repo.project_id != project_id:
|
if repo is None or repo.project_id != project_id:
|
||||||
raise ValueError("repository not found")
|
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_id = uuid.UUID(data.tool_type_id)
|
||||||
tool_type = await session.get(ToolType, tool_type_id)
|
tool_type = await session.get(ToolType, tool_type_id)
|
||||||
if tool_type is None:
|
if tool_type is None:
|
||||||
@@ -1120,7 +1126,6 @@ async def create_tool_instance(
|
|||||||
if data.display_name:
|
if data.display_name:
|
||||||
instance_display = data.display_name
|
instance_display = data.display_name
|
||||||
else:
|
else:
|
||||||
project = await session.get(Project, project_id)
|
|
||||||
project_name = project.name if project else "Unknown"
|
project_name = project.name if project else "Unknown"
|
||||||
scope_name = workspace.name if workspace else repo.name
|
scope_name = workspace.name if workspace else repo.name
|
||||||
auto_name = f"{project_name} / {scope_name} / {tool_type.display_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
|
# Fetch tool type early to determine home directory and container user
|
||||||
tool_type = await session.get(ToolType, instance.tool_type_id)
|
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_uid = 0
|
||||||
container_gid = 0
|
container_gid = 0
|
||||||
if tool_type and tool_type.definition_type == "manifest" and tool_type.manifest_id:
|
if tool_type and tool_type.definition_type == "manifest" and tool_type.manifest_id:
|
||||||
@@ -1583,8 +1590,16 @@ async def start_tool_instance(
|
|||||||
else:
|
else:
|
||||||
# ── LEGACY FLOW ──────────────────────────────────────────
|
# ── LEGACY FLOW ──────────────────────────────────────────
|
||||||
if instance.clone_mode == "clone" and not instance.workspace_id:
|
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_project = (
|
||||||
clone_name = _slugify_directory_name(clone_project.name) if clone_project else "repo-clone"
|
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_path = os.path.join(instance_dir, clone_name)
|
||||||
|
|
||||||
repo = await session.get(GitRepository, instance.repository_id)
|
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.
|
# Ensure the legacy compose mounts the project-named clone directory.
|
||||||
extra_volumes.append({
|
extra_volumes.append(
|
||||||
"source": repo_path,
|
{
|
||||||
"target": f"{home_dir}/{clone_name}",
|
"source": repo_path,
|
||||||
"type": "bind",
|
"target": f"{home_dir}/{clone_name}",
|
||||||
})
|
"type": "bind",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
if port_override or start_command or working_directory or extra_volumes:
|
if port_override or start_command or working_directory or extra_volumes:
|
||||||
modify_compose_file(
|
modify_compose_file(
|
||||||
@@ -2124,8 +2141,16 @@ async def delete_tool_instance(
|
|||||||
os.path.dirname(instance.compose_path) if instance.compose_path else None
|
os.path.dirname(instance.compose_path) if instance.compose_path else None
|
||||||
)
|
)
|
||||||
if instance_dir:
|
if instance_dir:
|
||||||
clone_project = await session.get(Project, instance.project_id) if instance.project_id else None
|
clone_project = (
|
||||||
clone_name = _slugify_directory_name(clone_project.name) if clone_project else "repo-clone"
|
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)
|
clone_path = os.path.join(instance_dir, clone_name)
|
||||||
if not os.path.exists(clone_path):
|
if not os.path.exists(clone_path):
|
||||||
clone_path = os.path.join(instance_dir, "repo-clone")
|
clone_path = os.path.join(instance_dir, "repo-clone")
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
"""Unit tests for the tool instance service."""
|
"""Unit tests for the tool instance service."""
|
||||||
|
|
||||||
|
import uuid
|
||||||
from unittest.mock import MagicMock, AsyncMock
|
from unittest.mock import MagicMock, AsyncMock
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
@@ -70,9 +71,7 @@ class TestGetRepositoryMountName:
|
|||||||
class TestStackProfileMountsWithGitMounts:
|
class TestStackProfileMountsWithGitMounts:
|
||||||
"""Tests for _stack_profile_mounts_with_git_mounts."""
|
"""Tests for _stack_profile_mounts_with_git_mounts."""
|
||||||
|
|
||||||
def test_exact_overlap_merges_profile_files_into_git_source(
|
def test_exact_overlap_merges_profile_files_into_git_source(self, tmp_path) -> None:
|
||||||
self, tmp_path
|
|
||||||
) -> None:
|
|
||||||
"""When a profile mount targets the same directory as a git mount,
|
"""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
|
the profile files should be copied into the git-mount source so the
|
||||||
container sees both sets of files through one bind mount."""
|
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
|
instance_service.subprocess.run = original_run
|
||||||
|
|
||||||
assert "WORKSPACE_NAME: acme" in compose_content
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user