From b5e961ebe9f3d83132b124d80222881a6c1a9c5b Mon Sep 17 00:00:00 2001 From: Developer Date: Fri, 17 Jul 2026 20:53:37 +0000 Subject: [PATCH] fix: reuse cached config profile git mounts - Reuse valid deterministic git mount clones on repeated starts - Remove incomplete clone destinations before retrying - Add regression coverage for cached and partial clones - Document OpenSpec change fix-config-profile-git-mount-clone-reuse Quality gates: pytest (13 passed), ruff, mypy --- .../api/src/services/tool/instance_service.py | 34 ++++++----- apps/api/tests/unit/test_instance_service.py | 57 +++++++++++++++++++ openspec/changes/.pi-map.index.md | 3 + .../.pi-map.index.md | 20 +++++++ .../.pi-map.md | 20 +++++++ .../change.md | 27 +++++++++ .../tasks.md | 9 +++ 7 files changed, 155 insertions(+), 15 deletions(-) create mode 100644 openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md create mode 100644 openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md create mode 100644 openspec/changes/fix-config-profile-git-mount-clone-reuse/change.md create mode 100644 openspec/changes/fix-config-profile-git-mount-clone-reuse/tasks.md diff --git a/apps/api/src/services/tool/instance_service.py b/apps/api/src/services/tool/instance_service.py index 82ab4c4..5f81441 100644 --- a/apps/api/src/services/tool/instance_service.py +++ b/apps/api/src/services/tool/instance_service.py @@ -283,16 +283,27 @@ def clone_git_repo( 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( - remote_url, - None, # No SSH key for now - can be added later - clone_dir, - branch or "main", - project_name=project_name, - ) + clone_name = _slugify_directory_name(project_name) if project_name else "repo-clone" + repo_path = os.path.join(clone_dir, clone_name) - if not os.path.exists(repo_path): + if os.path.isdir(os.path.join(repo_path, ".git")): + # Reuse the deterministic per-repository cache on repeated starts. try: + pull_repository_updates(repo_path, remote_url) + logger.debug("Pulled updates for git mount %s", remote_url) + except Exception as exc: + logger.warning("Failed to pull updates for %s: %s", remote_url, exc) + else: + try: + # A failed clone can leave its destination behind. Remove only the + # computed clone path so the next start can retry cleanly. + if os.path.lexists(repo_path): + logger.warning("Removing incomplete git mount clone at %s", repo_path) + if os.path.isdir(repo_path) and not os.path.islink(repo_path): + shutil.rmtree(repo_path) + else: + os.unlink(repo_path) + os.makedirs(clone_dir, exist_ok=True) repo_path = clone_repository( remote_url, @@ -305,13 +316,6 @@ def clone_git_repo( except Exception as exc: logger.warning("Clone failed for git mount %s: %s", remote_url, exc) raise - else: - # Repo exists - pull latest updates - try: - pull_repository_updates(repo_path, remote_url) - logger.debug("Pulled updates for git mount %s", remote_url) - except Exception as exc: - logger.warning("Failed to pull updates for %s: %s", remote_url, exc) # Handle branch checkout if specified if branch and repo_path: diff --git a/apps/api/tests/unit/test_instance_service.py b/apps/api/tests/unit/test_instance_service.py index 63457c5..8fef0a3 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 hashlib import uuid from unittest.mock import MagicMock, AsyncMock @@ -8,6 +9,7 @@ import pytest from src.services.tool.instance_service import ( _get_repository_mount_name, _stack_profile_mounts_with_git_mounts, + clone_git_repo, modify_compose_file, prepare_manifest_instance, ) @@ -67,6 +69,61 @@ class TestGetRepositoryMountName: assert _get_repository_mount_name(project, repo) == "project-v2-0" +@pytest.mark.unit +class TestCloneGitRepo: + """Regression tests for reusable config-profile git mount clones.""" + + def test_reuses_existing_clone(self, monkeypatch, tmp_path) -> None: + from src.services.tool import instance_service + + remote_url = "https://gitlab.com/example/dotfiles" + branch = None + clone_parent = str(tmp_path) + url_hash = hashlib.md5(f"{remote_url}:default".encode()).hexdigest()[:12] + repo_path = ( + tmp_path + / "git-mounts" + / f"dotfiles-{url_hash}" + / "repo-clone" + ) + (repo_path / ".git").mkdir(parents=True) + + clone = MagicMock(side_effect=AssertionError("existing clone must be reused")) + pull = MagicMock() + monkeypatch.setattr(instance_service, "clone_repository", clone) + monkeypatch.setattr(instance_service, "pull_repository_updates", pull) + + result = clone_git_repo(remote_url, branch, clone_parent) + + assert result == str(repo_path) + clone.assert_not_called() + pull.assert_called_once_with(str(repo_path), remote_url) + + def test_replaces_incomplete_clone_before_retry(self, monkeypatch, tmp_path) -> None: + from src.services.tool import instance_service + + remote_url = "https://gitlab.com/example/dotfiles" + url_hash = hashlib.md5(f"{remote_url}:main".encode()).hexdigest()[:12] + clone_dir = tmp_path / "git-mounts" / f"dotfiles-{url_hash}" + repo_path = clone_dir / "repo-clone" + repo_path.mkdir(parents=True) + (repo_path / "partial-file").write_text("incomplete") + + def clone(_url, _key, destination, _branch, project_name=None): + assert destination == str(clone_dir) + assert project_name is None + assert not repo_path.exists() + (repo_path / ".git").mkdir(parents=True) + return str(repo_path) + + monkeypatch.setattr(instance_service, "clone_repository", clone) + + result = clone_git_repo(remote_url, "main", str(tmp_path)) + + assert result == str(repo_path) + assert (repo_path / ".git").is_dir() + + @pytest.mark.unit class TestStackProfileMountsWithGitMounts: """Tests for _stack_profile_mounts_with_git_mounts.""" diff --git a/openspec/changes/.pi-map.index.md b/openspec/changes/.pi-map.index.md index 68f75ba..ab6883a 100644 --- a/openspec/changes/.pi-map.index.md +++ b/openspec/changes/.pi-map.index.md @@ -10,6 +10,9 @@ map: openspec/.pi-map.md - openspec/changes/archive index: openspec/changes/archive/.pi-map.index.md map: openspec/changes/archive/.pi-map.md +- openspec/changes/fix-config-profile-git-mount-clone-reuse + index: openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md + map: openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md - openspec/changes/fix-container-status-false-positive index: openspec/changes/fix-container-status-false-positive/.pi-map.index.md map: openspec/changes/fix-container-status-false-positive/.pi-map.md diff --git a/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md b/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md new file mode 100644 index 0000000..ffaaaff --- /dev/null +++ b/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md @@ -0,0 +1,20 @@ +# openspec/changes/fix-config-profile-git-mount-clone-reuse (index) +dir: openspec/changes/fix-config-profile-git-mount-clone-reuse + +## role +Documents and tracks a bug fix for reusing cached Config Profile git mount clones during tool instance startup. +## parent +index: openspec/changes/.pi-map.index.md +map: openspec/changes/.pi-map.md +## children +- +## files +- change.md +- tasks.md +## links +index: openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md +map: openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md +## workflows +- +## dirty +- diff --git a/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md b/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md new file mode 100644 index 0000000..e388c6e --- /dev/null +++ b/openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.md @@ -0,0 +1,20 @@ +# openspec/changes/fix-config-profile-git-mount-clone-reuse +dir: openspec/changes/fix-config-profile-git-mount-clone-reuse + +index: openspec/changes/fix-config-profile-git-mount-clone-reuse/.pi-map.index.md + +## role +Documents and tracks a bug fix for reusing cached Config Profile git mount clones during tool instance startup. +## files +- change.md | Describes the cached clone regression, required behavior, implementation scope, and verification plan. +- tasks.md | Tracks investigation, regression testing, implementation, project-map maintenance, verification, and commit status. +## arch +Documentation-only OpenSpec change package with separate change rationale and implementation task checklist. +## tags +config, profile, git, mount, clone, cache, startup, fix +## symbols +- +## workflows +- +## dirty +- diff --git a/openspec/changes/fix-config-profile-git-mount-clone-reuse/change.md b/openspec/changes/fix-config-profile-git-mount-clone-reuse/change.md new file mode 100644 index 0000000..2487513 --- /dev/null +++ b/openspec/changes/fix-config-profile-git-mount-clone-reuse/change.md @@ -0,0 +1,27 @@ +# Fix config profile git mount clone reuse + +## Problem + +Starting a tool instance with a Config Profile git mount can omit the mount when the repository was already cloned into the instance cache. The startup log reports that the destination path already exists and is not an empty directory. + +## Root cause + +`clone_git_repo()` computes a deterministic cache directory but calls `clone_repository()` before checking whether that directory already contains a clone. `git clone` therefore fails on repeated starts or overlapping start requests. `resolve_single_git_mount()` treats auxiliary mount failures as non-blocking, so startup continues without the configured volume. + +## Required behavior + +1. A valid existing git mount clone must be reused and updated instead of cloned again. +2. A missing clone must still be created normally. +3. An incomplete clone directory must not permanently prevent a later retry. +4. A clone/update failure remains non-blocking at the git mount resolver boundary. + +## Scope + +- Correct clone-cache handling in `apps/api/src/services/tool/instance_service.py`. +- Add focused regression tests in `apps/api/tests/unit/test_instance_service.py`. +- No API, database, frontend, or Docker Compose contract changes. + +## Verification + +- Targeted `pytest` for git mount clone reuse and instance service tests. +- Ruff and mypy checks for changed backend files. diff --git a/openspec/changes/fix-config-profile-git-mount-clone-reuse/tasks.md b/openspec/changes/fix-config-profile-git-mount-clone-reuse/tasks.md new file mode 100644 index 0000000..8054663 --- /dev/null +++ b/openspec/changes/fix-config-profile-git-mount-clone-reuse/tasks.md @@ -0,0 +1,9 @@ +# Tasks: fix config profile git mount clone reuse + +- [x] Capture the failing runtime trace from an affected tool session. +- [x] Add a regression test proving a valid cached clone is reused. +- [x] Update `clone_git_repo()` to check the deterministic clone path before cloning. +- [x] Recover safely from an incomplete clone directory. +- [x] Run targeted backend tests and quality checks. +- [x] Update project maps and validate map freshness. +- [x] Commit the verified fix.