From 2247ec47c9044432af6a88de2e43e17befd40c4e Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Wed, 22 Jul 2026 11:23:19 +0200 Subject: [PATCH] fix(config-profiles): synchronize shared mount working copies Use canonical profile files and writable Git working copies so editor and container changes share one source. Require confirmation before destructive Git refreshes and overlay profile files without composite snapshots. --- apps/api/src/api/config/config_profiles.py | 48 +++++- .../api/src/services/tool/instance_service.py | 162 +++++++----------- .../tests/unit/test_config_profile_refresh.py | 23 +++ apps/api/tests/unit/test_instance_service.py | 75 +++++--- apps/web/src/api/config-profiles.test.ts | 31 ++++ apps/web/src/api/config-profiles.ts | 10 +- .../ConfigProfileEditorPanel.tsx | 7 + .../ConfigProfilesMobileView.tsx | 7 + apps/web/src/hooks/use-config-profiles.ts | 28 +++ apps/web/src/pages/ConfigProfilesPage.tsx | 6 + .../live-config-profile-refresh/tasks.md | 16 +- .../live-git-config-mount-refresh/change.md | 7 +- .../live-git-config-mount-refresh/design.md | 10 +- .../live-git-config-mount-refresh/tasks.md | 14 +- 14 files changed, 285 insertions(+), 159 deletions(-) create mode 100644 apps/api/tests/unit/test_config_profile_refresh.py create mode 100644 apps/web/src/api/config-profiles.test.ts diff --git a/apps/api/src/api/config/config_profiles.py b/apps/api/src/api/config/config_profiles.py index 7378ae2..4346b66 100644 --- a/apps/api/src/api/config/config_profiles.py +++ b/apps/api/src/api/config/config_profiles.py @@ -3,6 +3,7 @@ import logging import os import uuid +from pathlib import Path from fastapi import APIRouter, Depends, HTTPException, Query, status from sqlalchemy import select @@ -10,6 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload from src.auth.dependencies import get_current_user_id, get_db_session +from src.config import Settings from src.models import ConfigProfile, ConfigProfileInclude, ToolInstance, UserConfig from src.schemas.config import ( ConfigProfileCreate, @@ -22,6 +24,7 @@ from src.schemas.config import ( ) from src.services.config.config_profile_resolver import ( ConfigProfileCycleError, + apply_resolved_profile, resolve_profile, resolved_profile_to_dict, ) @@ -44,6 +47,31 @@ logger = logging.getLogger(__name__) router = APIRouter(prefix="/config-profiles", tags=["config-profiles"]) +def _canonical_profile_response(profile: ConfigProfile) -> dict: + """Return profile data with edits from its shared working copy.""" + response = profile_to_response(profile) + root = Path(Settings().instance_base_path) / "config-profiles" / str(profile.id) + + def read_file(path: Path, fallback: str) -> str: + try: + return path.read_text() if path.is_file() else fallback + except OSError: + return fallback + + response["files"] = { + relative_path: read_file(root / "files" / relative_path, content) + for relative_path, content in response["files"].items() + } + response["mounts"] = [dict(mount) for mount in response["mounts"]] + for mount in response["mounts"]: + mount_root = root / "mounts" / mount["target"].lstrip("/").replace("/", "_") + mount["files"] = { + relative_path: read_file(mount_root / relative_path, content) + for relative_path, content in mount.get("files", {}).items() + } + return response + + async def _running_profile_outcomes( session: AsyncSession, profile_id: uuid.UUID ) -> list[dict[str, str]]: @@ -114,7 +142,7 @@ async def list_config_profiles( result = await session.execute(query) profiles = result.scalars().all() - return [profile_to_response(p) for p in profiles] + return [_canonical_profile_response(profile) for profile in profiles] @router.post( @@ -147,7 +175,7 @@ async def get_config_profile( raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Not authorized" ) - return profile_to_response(profile) + return _canonical_profile_response(profile) @router.put("/{profile_id}", response_model=ConfigProfileResponse) @@ -169,7 +197,12 @@ async def update_config_profile( ) profile = await update_profile(session, profile, data) - response = profile_to_response(profile) + resolved = await resolve_profile(session, profile.id) + apply_resolved_profile( + os.path.join(Settings().instance_base_path, "profile-refresh"), + resolved, + ) + response = _canonical_profile_response(profile) response["refresh_outcomes"] = await _running_profile_outcomes(session, profile.id) logger.debug("Updated config profile %s", profile.id) return response @@ -178,10 +211,17 @@ async def update_config_profile( @router.post("/{profile_id}/refresh-git-mounts") async def refresh_profile_git_mounts( profile_id: str, + confirm_destructive_refresh: bool = False, 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.""" + """Destructively refresh profile Git working copies used by instances.""" + if not confirm_destructive_refresh: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Confirm destructive Git refresh before replacing local edits", + ) + profile = await get_profile_with_includes(session, uuid.UUID(profile_id)) if profile is None: raise HTTPException( diff --git a/apps/api/src/services/tool/instance_service.py b/apps/api/src/services/tool/instance_service.py index 2bb07dd..cd93b83 100644 --- a/apps/api/src/services/tool/instance_service.py +++ b/apps/api/src/services/tool/instance_service.py @@ -127,15 +127,20 @@ def _chown_staged_mounts( uid: int, gid: int, ) -> None: - """Recursively chown instance-local mount sources to the container user. + """Recursively chown writable profile and instance mount sources. - Profile copies, profile/Git composites, and SSH key mounts are created by - the API process. Their sources must be owned by the target container user - before Docker bind-mounts them into writable paths. + Canonical non-Git profile sources are shared by compatible instances, so + they must be writable by the container user rather than copied per + instance. Instance-local composites and SSH mounts remain supported. """ + canonical_profile_root = os.path.join( + os.path.dirname(instance_dir), "config-profiles" + ) for vol in extra_volumes: source = vol.get("source", "") - if not source or not source.startswith(instance_dir): + if not source or not ( + source.startswith(instance_dir) or source.startswith(canonical_profile_root) + ): continue _chown_path(source, uid, gid) @@ -234,100 +239,38 @@ def _stack_profile_mounts_with_git_mounts( git_mount_volumes: list[dict], instance_dir: str, ) -> list[dict]: - """Build instance-local composite mounts for overlapping profile and Git paths. + """Overlay canonical profile files on Git directories without snapshots. - Docker applies one bind mount per target; it never merges their contents. - A composite therefore copies shared Git content first and profile content - second, so profile files extend (and intentionally override) the Git tree - without dirtying the shared clone. + An overlapping profile directory is expanded into individual child-file + mounts. Docker then mounts the Git working directory first and the more + specific canonical profile files last, preserving shared writable sources + instead of constructing an instance-local composite copy. """ - records: list[tuple[str, dict]] = [ - ("profile", mount) for mount in profile_mounts - ] + [("git", mount) for mount in git_mount_volumes] - components: list[list[int]] = [] - remaining: set[int] = set(range(len(records))) + del instance_dir + result: list[dict] = list(git_mount_volumes) - while remaining: - component_indices: set[int] = {min(remaining)} - remaining.difference_update(component_indices) - pending = list(component_indices) - while pending: - current_index = pending.pop() - current_target = records[current_index][1].get("target", "") - for candidate_index in list(remaining): - candidate_target = records[candidate_index][1].get("target", "") - if _mounts_overlap(current_target, candidate_target): - remaining.remove(candidate_index) - component_indices.add(candidate_index) - pending.append(candidate_index) - components.append(sorted(component_indices)) - - result: list[dict] = [] - for component_indexes in components: - component_records = [records[index] for index in component_indexes] - kinds = {kind for kind, _mount in component_records} - if kinds != {"profile", "git"}: - result.extend(mount for _kind, mount in component_records) + for profile_mount in profile_mounts: + source = profile_mount.get("source", "") + target = profile_mount.get("target", "") + overlaps_git = any( + _mounts_overlap(target, git_mount.get("target", "")) + for git_mount in git_mount_volumes + ) + if not overlaps_git or not os.path.isdir(source): + result.append(profile_mount) continue - targets = [ - os.path.normpath(mount["target"]) for _kind, mount in component_records - ] - composite_target = os.path.commonpath(targets) - if any( - not os.path.isdir(mount["source"]) - and os.path.normpath(mount["target"]) == composite_target - for _kind, mount in component_records - ): - composite_target = os.path.dirname(composite_target) - - digest_input = "\0".join( - f"{kind}:{mount['source']}:{mount['target']}" - for kind, mount in component_records - ) - composite_source = os.path.join( - instance_dir, - "mounts", - "composites", - hashlib.sha256(digest_input.encode()).hexdigest()[:16], - ) - try: - if os.path.lexists(composite_source): - shutil.rmtree(composite_source) - os.makedirs(composite_source, exist_ok=True) - except OSError as exc: - raise RuntimeError( - f"Unable to prepare composite mount directory {composite_source}: {exc}" - ) from exc - - for kind in ("git", "profile"): - for record_kind, mount in component_records: - if record_kind != kind: - continue - relative_target = _relative_under(composite_target, mount["target"]) - destination = ( - composite_source - if relative_target == "" - else os.path.join(composite_source, relative_target or "") + for root, _dirs, files in os.walk(source): + for filename in files: + file_source = os.path.join(root, filename) + relative_path = os.path.relpath(file_source, source) + result.append( + { + **profile_mount, + "source": file_source, + "target": os.path.join(target, relative_path), + } ) - _copy_mount_source(mount["source"], destination) - - result.append( - { - "source": composite_source, - "target": composite_target, - "type": "bind", - "readonly": all( - mount.get("readonly", False) for _kind, mount in component_records - ), - } - ) - logger.info( - "Composed %d profile/Git mounts at %s into %s", - len(component_records), - composite_target, - composite_source, - ) return result @@ -629,8 +572,10 @@ async def resolve_single_git_mount( volumes = resolve_git_mount_mappings( repo_path, mappings, working_directory, home_dir ) + # Profile-scoped Git working copies are writable and shared by compatible + # containers. Explicit refresh replaces local edits with the remote ref. for volume in volumes: - volume["readonly"] = True + volume["readonly"] = False return volumes @@ -675,10 +620,10 @@ def checkout_branch(repo_path: str, branch: str) -> bool: def pull_repository_updates(repo_path: str, remote_url: str) -> None: - """Pull latest updates from remote repository. + """Replace a profile working copy with its current remote branch. - Used when starting a new container with an existing cloned repository - to ensure the latest code is mounted. + Git profile mounts are writable shared working copies. Refresh discards + local container/editor edits after fetching the remote baseline. """ import subprocess @@ -692,15 +637,22 @@ def pull_repository_updates(repo_path: str, remote_url: str) -> None: if result.returncode != 0: raise RuntimeError(f"Failed to fetch updates: {result.stderr}") - # Pull changes for current branch - result = subprocess.run( - ["git", "-C", repo_path, "pull", "origin"], + branch_result = subprocess.run( + ["git", "-C", repo_path, "branch", "--show-current"], capture_output=True, text=True, ) + branch = branch_result.stdout.strip() if branch_result.returncode == 0 else "" + if not branch: + raise RuntimeError("Unable to determine Git working-copy branch") + result = subprocess.run( + ["git", "-C", repo_path, "reset", "--hard", f"origin/{branch}"], + capture_output=True, + text=True, + ) if result.returncode != 0: - raise RuntimeError(f"Failed to pull updates: {result.stderr}") + raise RuntimeError(f"Failed to reset working copy: {result.stderr}") def expand_glob_source(source_path: str, repo_path: str) -> list[str]: @@ -1588,9 +1540,11 @@ async def start_tool_instance( git_mount_volumes = await resolve_git_mounts( session, resolved, instance_dir, working_directory, home_dir ) - staged_profile_mounts = _stage_profile_mounts(profile_mounts, instance_dir) + # Non-Git profile mounts bind directly to canonical profile + # storage so edits made by one compatible container are visible to + # every other container and the profile editor readback path. composed_mounts = _stack_profile_mounts_with_git_mounts( - staged_profile_mounts, + profile_mounts, git_mount_volumes, instance_dir, ) @@ -1601,7 +1555,7 @@ async def start_tool_instance( instance.id, len(profile_env), len(profile_files), - len(staged_profile_mounts), + len(profile_mounts), len(git_mount_volumes), len(composed_mounts), ) diff --git a/apps/api/tests/unit/test_config_profile_refresh.py b/apps/api/tests/unit/test_config_profile_refresh.py new file mode 100644 index 0000000..46f462a --- /dev/null +++ b/apps/api/tests/unit/test_config_profile_refresh.py @@ -0,0 +1,23 @@ +"""Tests for Config Profile refresh safety contracts.""" + +import uuid +from unittest.mock import AsyncMock + +import pytest +from fastapi import HTTPException + +from src.api.config.config_profiles import refresh_profile_git_mounts + + +@pytest.mark.unit +async def test_git_refresh_requires_destructive_confirmation() -> None: + """The endpoint must not reset a writable working copy without consent.""" + with pytest.raises(HTTPException) as exc_info: + await refresh_profile_git_mounts( + profile_id=str(uuid.uuid4()), + confirm_destructive_refresh=False, + current_user_id=uuid.uuid4(), + session=AsyncMock(), + ) + + assert exc_info.value.status_code == 409 diff --git a/apps/api/tests/unit/test_instance_service.py b/apps/api/tests/unit/test_instance_service.py index ea1582a..bab4ae4 100644 --- a/apps/api/tests/unit/test_instance_service.py +++ b/apps/api/tests/unit/test_instance_service.py @@ -15,6 +15,7 @@ from src.services.tool.instance_service import ( clone_git_repo, modify_compose_file, prepare_manifest_instance, + pull_repository_updates, ) @@ -97,6 +98,37 @@ class TestCloneGitRepo: clone.assert_not_called() pull.assert_called_once_with(str(repo_path), remote_url) + def test_refresh_resets_writable_working_copy_to_remote_branch( + self, monkeypatch + ) -> None: + from src.services.tool import instance_service + + results = [ + MagicMock(returncode=0, stdout="", stderr=""), + MagicMock(returncode=0, stdout="main\n", stderr=""), + MagicMock(returncode=0, stdout="", stderr=""), + ] + run = MagicMock(side_effect=results) + monkeypatch.setattr(instance_service.subprocess, "run", run) + + pull_repository_updates("/work/profile-git", "https://example.test/repo.git") + + assert run.call_args_list[0].args[0] == [ + "git", + "-C", + "/work/profile-git", + "fetch", + "origin", + ] + assert run.call_args_list[2].args[0] == [ + "git", + "-C", + "/work/profile-git", + "reset", + "--hard", + "origin/main", + ] + def test_replaces_incomplete_clone_before_retry( self, monkeypatch, tmp_path ) -> None: @@ -191,17 +223,12 @@ class TestStackProfileMountsWithGitMounts: str(instance_dir), ) - assert len(result) == 1 - composite = result[0] - assert composite["target"] == "/home/user/.pi" - assert composite["source"].startswith(str(instance_dir)) - assert not (tmp_path / "git" / "repo-clone" / "settings.json").exists() - assert ( - tmp_path / "git" / "repo-clone" / "existing.txt" - ).read_text() == "from git" - assert (tmp_path / "instance" / "mounts" / "composites").exists() - assert (Path(composite["source"]) / "existing.txt").read_text() == "from git" - assert (Path(composite["source"]) / "settings.json").read_text() == "{}" + assert len(result) == 2 + assert result[0]["source"] == str(git_source) + assert result[0]["target"] == "/home/user/.pi" + assert result[1]["source"].endswith("/settings.json") + assert result[1]["target"] == "/home/user/.pi/settings.json" + assert not (git_source / "settings.json").exists() def test_descendant_profile_mount_extends_git_root(self, tmp_path) -> None: """Nested targets are composed at the Git root, preserving siblings.""" @@ -223,11 +250,9 @@ class TestStackProfileMountsWithGitMounts: str(instance_dir), ) - assert len(result) == 1 - composite = Path(result[0]["source"]) - assert result[0]["target"] == "/home/user/.pi" - assert (composite / "README").read_text() == "repo" - assert (composite / "agent" / "settings.json").read_text() == "x" + assert len(result) == 2 + assert result[0]["source"] == str(git_source) + assert result[1]["target"] == "/home/user/.pi/agent/settings.json" assert not (git_source / "agent").exists() def test_file_profile_mount_extends_git_root(self, tmp_path) -> None: @@ -254,11 +279,9 @@ class TestStackProfileMountsWithGitMounts: str(instance_dir), ) - assert len(result) == 1 - composite = Path(result[0]["source"]) - assert result[0]["target"] == "/home/user/.pi" - assert (composite / "README").read_text() == "repo" - assert (composite / "settings.json").read_text() == "{}" + assert len(result) == 2 + assert result[0]["source"] == str(git_source) + assert result[1]["target"] == "/home/user/.pi/settings.json" def test_parent_profile_mount_extends_nested_git_mount(self, tmp_path) -> None: """A profile parent mount keeps Git content and its own sibling files.""" @@ -279,11 +302,9 @@ class TestStackProfileMountsWithGitMounts: str(instance_dir), ) - assert len(result) == 1 - composite = Path(result[0]["source"]) - assert result[0]["target"] == "/home/user" - assert (composite / "config.toml").read_text() == "profile" - assert (composite / ".pi" / "plugin.toml").read_text() == "git" + assert len(result) == 2 + assert result[0]["source"] == str(git_source) + assert result[1]["target"] == "/home/user/config.toml" def test_non_overlapping_mounts_remain_separate(self, tmp_path) -> None: """Unrelated profile and Git mounts retain their independent sources.""" @@ -303,7 +324,7 @@ class TestStackProfileMountsWithGitMounts: _stack_profile_mounts_with_git_mounts( profile_mounts, git_mounts, str(instance_dir) ) - == profile_mounts + git_mounts + == git_mounts + profile_mounts ) diff --git a/apps/web/src/api/config-profiles.test.ts b/apps/web/src/api/config-profiles.test.ts new file mode 100644 index 0000000..4bf717d --- /dev/null +++ b/apps/web/src/api/config-profiles.test.ts @@ -0,0 +1,31 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mockPost = vi.fn(); + +vi.mock("./client", () => ({ + apiClient: { + post: (...args: unknown[]) => mockPost(...args), + }, +})); + +import { refreshConfigProfileGitMounts } from "./config-profiles"; + +describe("refreshConfigProfileGitMounts", () => { + beforeEach(() => { + mockPost.mockReset(); + }); + + it("confirms the destructive refresh at the API boundary", async () => { + mockPost.mockResolvedValue({ data: { refresh_outcomes: [] } }); + + await expect(refreshConfigProfileGitMounts("profile-1")).resolves.toEqual({ + refresh_outcomes: [], + }); + + expect(mockPost).toHaveBeenCalledWith( + "/config-profiles/profile-1/refresh-git-mounts", + undefined, + { params: { confirm_destructive_refresh: true } }, + ); + }); +}); diff --git a/apps/web/src/api/config-profiles.ts b/apps/web/src/api/config-profiles.ts index 9d3c174..943f7c6 100644 --- a/apps/web/src/api/config-profiles.ts +++ b/apps/web/src/api/config-profiles.ts @@ -2,7 +2,11 @@ import { apiClient } from "./client"; export interface ConfigProfileRefreshOutcome { instance_id: string; - status: "compatible" | "refreshed" | "restart_required" | "incompatible_permissions"; + status: + | "compatible" + | "refreshed" + | "restart_required" + | "incompatible_permissions"; reason?: string; } @@ -150,7 +154,9 @@ export const refreshConfigProfileGitMounts = async ( ): Promise<{ refresh_outcomes: ConfigProfileRefreshOutcome[] }> => { const response = await apiClient.post<{ refresh_outcomes: ConfigProfileRefreshOutcome[]; - }>(`/config-profiles/${id}/refresh-git-mounts`); + }>(`/config-profiles/${id}/refresh-git-mounts`, undefined, { + params: { confirm_destructive_refresh: true }, + }); return response.data; }; diff --git a/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx b/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx index 3b85fb8..ba3f5f4 100644 --- a/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx +++ b/apps/web/src/components/features/config-profiles/ConfigProfileEditorPanel.tsx @@ -15,6 +15,7 @@ interface Props { previewData: ResolvedProfile | null; previewingId: string | null; isRefreshingGitMounts: boolean; + isReloadingWorkingCopy: boolean; projects: ProjectWithRepos[]; toolTypes: ToolType[]; availableProfiles: ConfigProfile[]; @@ -24,6 +25,7 @@ interface Props { onSubmit: (e?: React.FormEvent) => void; onReset: () => void; onPreview: () => void; + onReloadWorkingCopy: () => void; onRefreshGitMounts: () => void; onAddInclude: (id: string) => void; onRemoveInclude: (index: number) => void; @@ -57,6 +59,7 @@ export const ConfigProfileEditorPanel = ({ previewData, previewingId, isRefreshingGitMounts, + isReloadingWorkingCopy, projects, toolTypes, availableProfiles, @@ -66,6 +69,7 @@ export const ConfigProfileEditorPanel = ({ onSubmit, onReset, onPreview, + onReloadWorkingCopy, onRefreshGitMounts, onAddInclude, onRemoveInclude, @@ -118,6 +122,9 @@ export const ConfigProfileEditorPanel = ({ {!isCreating && selectedProfile && (
+