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 && (
+