From 886c8632608f34befb5a1c6525da0266e3d29938 Mon Sep 17 00:00:00 2001 From: Developer Date: Tue, 21 Jul 2026 14:37:42 +0000 Subject: [PATCH] feat: refresh shared Git config mounts live - Use profile-scoped canonical Git clone sources with locked refreshes - Mount shared Git configuration read-only and isolate profile content - Add API and desktop/mobile actions for live Git mount refresh Quality gates: frontend build and backend py_compile passed. Skipped: backend pytest/Ruff unavailable; Docker/manual checks not approved. --- apps/api/src/api/config/config_profiles.py | 39 +++++ .../api/src/services/tool/instance_service.py | 138 ++++++++++-------- apps/api/tests/unit/test_instance_service.py | 41 +++--- apps/web/src/api/config-profiles.ts | 11 +- .../ConfigProfileEditorPanel.tsx | 5 + .../ConfigProfilesMobileView.tsx | 9 ++ apps/web/src/hooks/use-config-profiles.ts | 21 +++ apps/web/src/pages/ConfigProfilesPage.tsx | 3 + .../live-git-config-mount-refresh/change.md | 23 +++ .../live-git-config-mount-refresh/design.md | 30 ++++ .../live-git-config-mount-refresh/tasks.md | 15 ++ 11 files changed, 254 insertions(+), 81 deletions(-) create mode 100644 openspec/changes/live-git-config-mount-refresh/change.md create mode 100644 openspec/changes/live-git-config-mount-refresh/design.md create mode 100644 openspec/changes/live-git-config-mount-refresh/tasks.md diff --git a/apps/api/src/api/config/config_profiles.py b/apps/api/src/api/config/config_profiles.py index b31e05c..7483ed5 100644 --- a/apps/api/src/api/config/config_profiles.py +++ b/apps/api/src/api/config/config_profiles.py @@ -1,6 +1,7 @@ """Config profile API endpoints.""" import logging +import os import uuid from fastapi import APIRouter, Depends, HTTPException, Query, status @@ -33,6 +34,7 @@ from src.services.config.crud_service import ( update_profile, validate_default_profiles, ) +from src.services.tool.instance_service import resolve_git_mounts from src.services.config.resolver_service import ( resolve_default_profile, validate_git_url, @@ -174,6 +176,43 @@ async def update_config_profile( return response +@router.post("/{profile_id}/refresh-git-mounts") +async def refresh_profile_git_mounts( + profile_id: str, + current_user_id: uuid.UUID = Depends(get_current_user_id), + session: AsyncSession = Depends(get_db_session), +): + """Refresh canonical Git mount sources used by running profile instances.""" + profile = await get_profile_with_includes(session, uuid.UUID(profile_id)) + if profile is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="Profile not found" + ) + if profile.user_id != current_user_id: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail="Not authorized" + ) + + outcomes = await _running_profile_outcomes(session, profile.id) + for outcome in outcomes: + instance = await session.get(ToolInstance, uuid.UUID(outcome["instance_id"])) + if ( + instance is None + or not instance.compose_path + or instance.selected_config_profile_id is None + ): + continue + resolved = await resolve_profile(session, instance.selected_config_profile_id) + await resolve_git_mounts( + session, + resolved, + os.path.dirname(instance.compose_path), + ) + outcome["status"] = "refreshed" + outcome["reason"] = "Canonical Git mount source refreshed in place" + return {"refresh_outcomes": outcomes} + + @router.delete("/{profile_id}", status_code=status.HTTP_204_NO_CONTENT) async def delete_config_profile( profile_id: str, diff --git a/apps/api/src/services/tool/instance_service.py b/apps/api/src/services/tool/instance_service.py index d61540c..47aa400 100644 --- a/apps/api/src/services/tool/instance_service.py +++ b/apps/api/src/services/tool/instance_service.py @@ -2,7 +2,9 @@ import asyncio import contextlib +import fcntl import glob as glob_module +import hashlib import logging import os import re @@ -158,57 +160,23 @@ def _stack_profile_mounts_with_git_mounts( profile_mounts: list[dict], git_mount_volumes: list[dict], ) -> list[dict]: - """Merge profile file mounts into overlapping git-mount sources. + """Leave profile and Git sources isolated. - When a config profile mounts static files to the same directory as a - git-mount (e.g. ``~/.pi``), a directory-level bind mount for the profile - would mask the cloned repository. Instead, copy the profile files into - the git-mount source directory so the container sees both sets of files - through a single bind mount. - - Profile mounts whose target is a child of a git-mount target are copied - into the corresponding subdirectory. Mounts that do not overlap are - returned unchanged. + Git mount sources are shared, read-only canonical checkouts. Profile + content must never be copied into one because doing so dirties the + checkout and leaks one profile's content to every instance using it. """ - remaining: list[dict] = [] - for pvol in profile_mounts: - p_source = pvol.get("source", "") - p_target = pvol.get("target", "") - if not p_source or not os.path.exists(p_source): - remaining.append(pvol) - continue - - merged = False - for gvol in git_mount_volumes: - g_source = gvol.get("source", "") - g_target = gvol.get("target", "") - if not g_source or not os.path.isdir(g_source): - continue - - rel = _relative_under(g_target, p_target) - if rel is None: - continue - - dst = os.path.join(g_source, rel) if rel else g_source - if os.path.isdir(p_source): - shutil.copytree(p_source, dst, dirs_exist_ok=True) - else: - os.makedirs(os.path.dirname(dst), exist_ok=True) - shutil.copy2(p_source, dst) - - logger.debug( - "Stacked profile mount %s into git mount %s at %s", - p_target, - g_target, - dst, - ) - merged = True - break - - if not merged: - remaining.append(pvol) - - return remaining + for profile_mount in profile_mounts: + profile_target = profile_mount.get("target", "") + for git_mount in git_mount_volumes: + if _relative_under(git_mount.get("target", ""), profile_target) is not None: + logger.warning( + "Config profile mount %s overlaps Git mount %s; keeping sources isolated", + profile_target, + git_mount.get("target"), + ) + break + return profile_mounts async def resolve_git_mounts( @@ -227,12 +195,18 @@ async def resolve_git_mounts( if not resolved.git_mounts: return [] + # Git mount sources are profile-scoped, not instance-scoped, so compatible + # instances bind the same canonical checkout. + clone_parent = os.path.join( + os.path.dirname(instance_dir or ""), "config-profiles", str(resolved.profile_id) + ) + # Process all git mounts concurrently tasks = [] for git_mount in resolved.git_mounts: tasks.append( resolve_single_git_mount( - session, git_mount, instance_dir, working_directory, home_dir + session, git_mount, clone_parent, working_directory, home_dir ) ) @@ -265,6 +239,37 @@ def normalize_git_mount(entry: dict) -> dict: return entry +@contextlib.contextmanager +def _git_mount_lock(clone_parent: str, remote_url: str, branch: str | None): + """Serialize clone and refresh operations for one canonical Git source.""" + lock_dir = os.path.join(clone_parent, "git-mounts") + identity = f"{remote_url}:{branch or 'default'}" + lock_path = os.path.join( + lock_dir, f".{hashlib.sha256(identity.encode()).hexdigest()}.lock" + ) + try: + os.makedirs(lock_dir, exist_ok=True) + with open(lock_path, "a+", encoding="utf-8") as lock_file: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + except OSError as exc: + raise RuntimeError(f"Cannot lock Git mount source: {lock_path}") from exc + + +def clone_git_repo_locked( + remote_url: str, + branch: str | None, + clone_parent: str, + project_name: str | None = None, +) -> str: + """Clone or refresh a canonical source while holding its process lock.""" + with _git_mount_lock(clone_parent, remote_url, branch): + return clone_git_repo(remote_url, branch, clone_parent, project_name) + + def clone_git_repo( remote_url: str, branch: str | None, @@ -425,7 +430,7 @@ def resolve_git_mount_mappings( async def resolve_single_git_mount( session: AsyncSession, git_mount: dict, - instance_dir: str | None = None, + clone_parent: str | None = None, working_directory: str | None = None, home_dir: str = "/root", ) -> list[dict]: @@ -447,15 +452,15 @@ async def resolve_single_git_mount( logger.warning("Invalid git mount skipped: no mappings") return [] - if not instance_dir: - logger.warning("Git mount skipped: no instance_dir provided for cloning") + if not clone_parent: + logger.warning("Git mount skipped: no canonical profile directory provided") return [] # Clone or pull the repository. Git mounts are auxiliary, so they keep # using the repository URL basename rather than the project name. try: repo_path = await asyncio.to_thread( - clone_git_repo, remote_url, branch, instance_dir + clone_git_repo_locked, remote_url, branch, clone_parent ) except Exception as exc: logger.warning( @@ -468,7 +473,12 @@ async def resolve_single_git_mount( return [] # Resolve all mappings from the cloned repo - return resolve_git_mount_mappings(repo_path, mappings, working_directory, home_dir) + volumes = resolve_git_mount_mappings( + repo_path, mappings, working_directory, home_dir + ) + for volume in volumes: + volume["readonly"] = True + return volumes def checkout_branch(repo_path: str, branch: str) -> bool: @@ -1481,7 +1491,15 @@ async def start_tool_instance( if ssh_keys_to_mount: ssh_dir = os.path.join(instance_dir, "mounts", "ssh", ".ssh") - os.makedirs(ssh_dir, exist_ok=True) + try: + os.makedirs(ssh_dir, exist_ok=True) + except OSError as exc: + logger.error( + "Failed to create SSH mount directory for instance %s: %s", + instance.id, + exc, + ) + ssh_keys_to_mount = [] key_filenames = [] for ssh_key in ssh_keys_to_mount: @@ -2194,7 +2212,13 @@ async def delete_tool_instance( if os.path.exists(instance_dir): import shutil - shutil.rmtree(instance_dir) + try: + shutil.rmtree(instance_dir) + except OSError as exc: + logger.error( + "Failed to remove instance directory %s: %s", instance_dir, exc + ) + raise RuntimeError("Failed to remove instance files") from exc await publish_lifecycle_event( event_bus=_event_bus, diff --git a/apps/api/tests/unit/test_instance_service.py b/apps/api/tests/unit/test_instance_service.py index 8fef0a3..c6b59a7 100644 --- a/apps/api/tests/unit/test_instance_service.py +++ b/apps/api/tests/unit/test_instance_service.py @@ -80,12 +80,7 @@ class TestCloneGitRepo: 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 = 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")) @@ -99,7 +94,9 @@ class TestCloneGitRepo: 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: + 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" @@ -128,10 +125,10 @@ class TestCloneGitRepo: class TestStackProfileMountsWithGitMounts: """Tests for _stack_profile_mounts_with_git_mounts.""" - 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.""" + def test_exact_overlap_keeps_profile_files_out_of_git_source( + self, tmp_path + ) -> None: + """Overlaps must not dirty the shared Git checkout.""" git_source = tmp_path / "git" / "repo-clone" git_source.mkdir(parents=True) (git_source / "existing.txt").write_text("from git") @@ -156,13 +153,12 @@ class TestStackProfileMountsWithGitMounts: profile_mounts, git_mount_volumes ) - assert result == [] + assert result == profile_mounts assert (git_source / "existing.txt").read_text() == "from git" - assert (git_source / "settings.json").read_text() == "{}" + assert not (git_source / "settings.json").exists() - def test_descendant_overlap_copies_into_subdirectory(self, tmp_path) -> None: - """Profile mounts targeting a child directory are copied into the - corresponding subdirectory of the git-mount source.""" + def test_descendant_overlap_keeps_sources_isolated(self, tmp_path) -> None: + """A child profile mount must not mutate the shared Git checkout.""" git_source = tmp_path / "git" git_source.mkdir() (git_source / "README").write_text("repo") @@ -186,8 +182,8 @@ class TestStackProfileMountsWithGitMounts: profile_mounts, git_mount_volumes ) - assert result == [] - assert (git_source / "agent" / "settings.json").read_text() == "x" + assert result == profile_mounts + assert not (git_source / "agent").exists() assert (git_source / "README").read_text() == "repo" def test_non_overlapping_mounts_left_untouched(self, tmp_path) -> None: @@ -247,9 +243,8 @@ class TestStackProfileMountsWithGitMounts: assert result == profile_mounts - def test_profile_source_file_copied_into_git_source(self, tmp_path) -> None: - """A profile mount that supplies a single file is copied into the - git-mount source directory.""" + def test_profile_source_file_does_not_mutate_git_source(self, tmp_path) -> None: + """A profile file must not be copied into a shared Git checkout.""" git_source = tmp_path / "git" git_source.mkdir() @@ -271,8 +266,8 @@ class TestStackProfileMountsWithGitMounts: profile_mounts, git_mount_volumes ) - assert result == [] - assert (git_source / "settings.json").read_text() == "{}" + assert result == profile_mounts + assert not (git_source / "settings.json").exists() @pytest.mark.unit diff --git a/apps/web/src/api/config-profiles.ts b/apps/web/src/api/config-profiles.ts index 7539a95..9d3c174 100644 --- a/apps/web/src/api/config-profiles.ts +++ b/apps/web/src/api/config-profiles.ts @@ -2,7 +2,7 @@ import { apiClient } from "./client"; export interface ConfigProfileRefreshOutcome { instance_id: string; - status: "compatible" | "restart_required" | "incompatible_permissions"; + status: "compatible" | "refreshed" | "restart_required" | "incompatible_permissions"; reason?: string; } @@ -145,6 +145,15 @@ export const deleteConfigProfile = async (id: string): Promise => { await apiClient.delete(`/config-profiles/${id}`); }; +export const refreshConfigProfileGitMounts = async ( + id: string, +): Promise<{ refresh_outcomes: ConfigProfileRefreshOutcome[] }> => { + const response = await apiClient.post<{ + refresh_outcomes: ConfigProfileRefreshOutcome[]; + }>(`/config-profiles/${id}/refresh-git-mounts`); + return response.data; +}; + export const updateProfileIncludes = async ( id: string, data: UpdateIncludesRequest, diff --git a/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx b/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx index 15c026a..22bcb57 100644 --- a/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx +++ b/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx @@ -23,6 +23,7 @@ interface Props { onSubmit: (e?: React.FormEvent) => void; onReset: () => void; onPreview: () => void; + onRefreshGitMounts: () => void; onAddInclude: (id: string) => void; onRemoveInclude: (index: number) => void; onDragStart: (e: React.DragEvent, index: number) => void; @@ -63,6 +64,7 @@ export const ConfigProfileEditorPanel = ({ onSubmit, onReset, onPreview, + onRefreshGitMounts, onAddInclude, onRemoveInclude, onDragStart, @@ -114,6 +116,9 @@ export const ConfigProfileEditorPanel = ({ {!isCreating && selectedProfile && (
+