diff --git a/apps/api/src/services/tool/instance_service.py b/apps/api/src/services/tool/instance_service.py index 47aa400..2bb07dd 100644 --- a/apps/api/src/services/tool/instance_service.py +++ b/apps/api/src/services/tool/instance_service.py @@ -127,11 +127,11 @@ def _chown_staged_mounts( uid: int, gid: int, ) -> None: - """Recursively chown staged mount sources to the container user. + """Recursively chown instance-local mount sources to the container user. - Config-profile mounts, git mounts, and SSH key mounts are staged under - instance_dir by the API process (root). Without this, the container - user cannot write into bind-mounted directories such as ~/.config. + 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. """ for vol in extra_volumes: source = vol.get("source", "") @@ -156,27 +156,180 @@ def _relative_under(parent: str, child: str) -> str | None: return None +def _stage_profile_mounts(profile_mounts: list[dict], instance_dir: str) -> list[dict]: + """Copy profile bind sources into an instance-local, writable staging area.""" + staged_mounts: list[dict] = [] + staging_root = os.path.join(instance_dir, "mounts", "profiles") + + for mount in profile_mounts: + source = mount.get("source", "") + target = mount.get("target", "") + if not source or not target: + continue + if not os.path.exists(source): + logger.warning("Skipping missing config profile mount source: %s", source) + continue + + digest = hashlib.sha256(f"{source}\0{target}".encode()).hexdigest()[:16] + staged_source = os.path.join(staging_root, digest) + try: + if os.path.lexists(staged_source): + if os.path.isdir(staged_source): + shutil.rmtree(staged_source) + else: + os.unlink(staged_source) + os.makedirs(os.path.dirname(staged_source), exist_ok=True) + + if os.path.isdir(source): + shutil.copytree(source, staged_source, symlinks=True) + else: + shutil.copy2(source, staged_source, follow_symlinks=False) + except OSError as exc: + logger.error("Failed to stage config profile mount %s: %s", source, exc) + continue + + staged_mount = dict(mount) + staged_mount["source"] = staged_source + staged_mounts.append(staged_mount) + + return staged_mounts + + +def _mounts_overlap(first_target: str, second_target: str) -> bool: + """Return whether two normalized container mount targets intersect.""" + return ( + _relative_under(first_target, second_target) is not None + or _relative_under(second_target, first_target) is not None + ) + + +def _copy_mount_source(source: str, destination: str) -> None: + """Copy a bind-mount source into its destination in a composite tree.""" + try: + if os.path.isdir(source): + os.makedirs(destination, exist_ok=True) + for entry in os.listdir(source): + source_entry = os.path.join(source, entry) + destination_entry = os.path.join(destination, entry) + if os.path.isdir(source_entry): + shutil.copytree( + source_entry, + destination_entry, + dirs_exist_ok=True, + symlinks=True, + ) + else: + os.makedirs(os.path.dirname(destination_entry), exist_ok=True) + shutil.copy2(source_entry, destination_entry, follow_symlinks=False) + return + + os.makedirs(os.path.dirname(destination), exist_ok=True) + shutil.copy2(source, destination, follow_symlinks=False) + except OSError as exc: + raise RuntimeError(f"Unable to compose mount source {source}: {exc}") from exc + + def _stack_profile_mounts_with_git_mounts( profile_mounts: list[dict], git_mount_volumes: list[dict], + instance_dir: str, ) -> list[dict]: - """Leave profile and Git sources isolated. + """Build instance-local composite mounts for overlapping profile and Git paths. - 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. + 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. """ - 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"), + 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))) + + 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) + 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 "") ) - break - return profile_mounts + _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 async def resolve_git_mounts( @@ -1435,22 +1588,22 @@ async def start_tool_instance( git_mount_volumes = await resolve_git_mounts( session, resolved, instance_dir, working_directory, home_dir ) - # Stack static file mounts on top of git repo mounts so they do - # not mask each other when they target the same directory. - stacked_profile_mounts = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes + staged_profile_mounts = _stage_profile_mounts(profile_mounts, instance_dir) + composed_mounts = _stack_profile_mounts_with_git_mounts( + staged_profile_mounts, + git_mount_volumes, + instance_dir, ) - extra_volumes.extend(stacked_profile_mounts) - extra_volumes.extend(git_mount_volumes) + extra_volumes.extend(composed_mounts) logger.debug( - "Applied config profile %s to instance %s (env=%d, files=%d, mounts=%d, git_mounts=%d, stacked=%d)", + "Applied config profile %s to instance %s (env=%d, files=%d, profile_mounts=%d, git_mounts=%d, composed_mounts=%d)", resolved.profile_name, instance.id, len(profile_env), len(profile_files), - len(profile_mounts), + len(staged_profile_mounts), len(git_mount_volumes), - len(profile_mounts) - len(stacked_profile_mounts), + len(composed_mounts), ) except ConfigProfileCycleError as exc: logger.error( diff --git a/apps/api/tests/unit/test_instance_service.py b/apps/api/tests/unit/test_instance_service.py index c6b59a7..ea1582a 100644 --- a/apps/api/tests/unit/test_instance_service.py +++ b/apps/api/tests/unit/test_instance_service.py @@ -2,13 +2,16 @@ import hashlib import uuid +from pathlib import Path from unittest.mock import MagicMock, AsyncMock import pytest from src.services.tool.instance_service import ( + _chown_staged_mounts, _get_repository_mount_name, _stack_profile_mounts_with_git_mounts, + _stage_profile_mounts, clone_git_repo, modify_compose_file, prepare_manifest_instance, @@ -123,151 +126,185 @@ class TestCloneGitRepo: @pytest.mark.unit class TestStackProfileMountsWithGitMounts: - """Tests for _stack_profile_mounts_with_git_mounts.""" + """Tests for composing profile and Git mounts without Docker masking.""" - def test_exact_overlap_keeps_profile_files_out_of_git_source( - self, tmp_path + def test_stages_profile_source_under_instance_directory(self, tmp_path) -> None: + """Profile sources must be instance-local before ownership is fixed.""" + instance_dir = tmp_path / "instance" + profile_source = tmp_path / "profile" / "settings.json" + profile_source.parent.mkdir(parents=True) + profile_source.write_text("{}") + + staged = _stage_profile_mounts( + [{"source": str(profile_source), "target": "/home/user/.pi/settings.json"}], + str(instance_dir), + ) + + assert staged[0]["source"].startswith(str(instance_dir)) + assert staged[0]["source"] != str(profile_source) + assert Path(staged[0]["source"]).read_text() == "{}" + + def test_staged_profile_source_is_chowned_for_container_user( + self, monkeypatch, tmp_path ) -> None: - """Overlaps must not dirty the shared Git checkout.""" + """The ownership pass must include the instance-local profile copy.""" + from src.services.tool import instance_service + + instance_dir = tmp_path / "instance" + profile_source = tmp_path / "profile" + profile_source.mkdir() + (profile_source / "settings.json").write_text("{}") + staged = _stage_profile_mounts( + [{"source": str(profile_source), "target": "/home/user/.pi"}], + str(instance_dir), + ) + chown = MagicMock() + monkeypatch.setattr(instance_service, "_chown_path", chown) + + _chown_staged_mounts(staged, str(instance_dir), 1000, 1000) + + chown.assert_called_once_with(staged[0]["source"], 1000, 1000) + + def test_exact_overlap_creates_instance_local_composite(self, tmp_path) -> None: + """Profile files extend a Git root without mutating its shared clone.""" + instance_dir = tmp_path / "instance" git_source = tmp_path / "git" / "repo-clone" git_source.mkdir(parents=True) (git_source / "existing.txt").write_text("from git") - - profile_source = tmp_path / "profile" / "home_user_.pi" - profile_source.mkdir(parents=True) + profile_source = tmp_path / "profile" + profile_source.mkdir() (profile_source / "settings.json").write_text("{}") - profile_mounts = [ - { - "source": str(profile_source), - "target": "/home/user/.pi", - "type": "bind", - "readonly": False, - } - ] - git_mount_volumes = [ - {"source": str(git_source), "target": "/home/user/.pi", "type": "bind"} - ] - + profile_mounts = _stage_profile_mounts( + [ + { + "source": str(profile_source), + "target": "/home/user/.pi", + "type": "bind", + } + ], + str(instance_dir), + ) result = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes + profile_mounts, + [{"source": str(git_source), "target": "/home/user/.pi", "type": "bind"}], + str(instance_dir), ) - assert result == profile_mounts - assert (git_source / "existing.txt").read_text() == "from git" - assert not (git_source / "settings.json").exists() + 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() == "{}" - def test_descendant_overlap_keeps_sources_isolated(self, tmp_path) -> None: - """A child profile mount must not mutate the shared Git checkout.""" + def test_descendant_profile_mount_extends_git_root(self, tmp_path) -> None: + """Nested targets are composed at the Git root, preserving siblings.""" + instance_dir = tmp_path / "instance" git_source = tmp_path / "git" git_source.mkdir() (git_source / "README").write_text("repo") - - profile_source = tmp_path / "profile" / "agent" - profile_source.mkdir(parents=True) + profile_source = tmp_path / "profile" + profile_source.mkdir() (profile_source / "settings.json").write_text("x") - profile_mounts = [ - { - "source": str(profile_source), - "target": "/home/user/.pi/agent", - "type": "bind", - } - ] - git_mount_volumes = [ - {"source": str(git_source), "target": "/home/user/.pi", "type": "bind"} - ] - + profile_mounts = _stage_profile_mounts( + [{"source": str(profile_source), "target": "/home/user/.pi/agent"}], + str(instance_dir), + ) result = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes + profile_mounts, + [{"source": str(git_source), "target": "/home/user/.pi", "type": "bind"}], + str(instance_dir), ) - assert result == profile_mounts + 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 not (git_source / "agent").exists() - assert (git_source / "README").read_text() == "repo" - def test_non_overlapping_mounts_left_untouched(self, tmp_path) -> None: - """Profile mounts that do not overlap a git mount are returned as-is.""" + def test_file_profile_mount_extends_git_root(self, tmp_path) -> None: + """A file bind mount is composed into the Git directory, not masked.""" + instance_dir = tmp_path / "instance" git_source = tmp_path / "git" git_source.mkdir() - - profile_source = tmp_path / "profile" - profile_source.mkdir() - (profile_source / "config").write_text("c") - - profile_mounts = [ - { - "source": str(profile_source), - "target": "/home/user/.config", - "type": "bind", - } - ] - git_mount_volumes = [ - {"source": str(git_source), "target": "/home/user/.pi", "type": "bind"} - ] - - result = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes - ) - - assert result == profile_mounts - - def test_git_source_file_does_not_consume_profile_mount(self, tmp_path) -> None: - """If the overlapping git-mount source is a file, the profile mount - cannot be merged and must be kept.""" - git_source = tmp_path / "file.txt" - git_source.write_text("file") - - profile_source = tmp_path / "profile" - profile_source.mkdir() - (profile_source / "settings.json").write_text("{}") - - profile_mounts = [ - { - "source": str(profile_source), - "target": "/home/user/.pi", - "type": "bind", - } - ] - git_mount_volumes = [ - { - "source": str(git_source), - "target": "/home/user/.pi/file.txt", - "type": "bind", - } - ] - - result = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes - ) - - assert result == profile_mounts - - 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() - + (git_source / "README").write_text("repo") profile_source = tmp_path / "settings.json" profile_source.write_text("{}") - profile_mounts = [ - { - "source": str(profile_source), - "target": "/home/user/.pi/settings.json", - "type": "bind", - } - ] - git_mount_volumes = [ - {"source": str(git_source), "target": "/home/user/.pi", "type": "bind"} - ] - + profile_mounts = _stage_profile_mounts( + [ + { + "source": str(profile_source), + "target": "/home/user/.pi/settings.json", + } + ], + str(instance_dir), + ) result = _stack_profile_mounts_with_git_mounts( - profile_mounts, git_mount_volumes + profile_mounts, + [{"source": str(git_source), "target": "/home/user/.pi", "type": "bind"}], + str(instance_dir), ) - assert result == profile_mounts - assert not (git_source / "settings.json").exists() + 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() == "{}" + + 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.""" + instance_dir = tmp_path / "instance" + git_source = tmp_path / "git" + git_source.mkdir() + (git_source / "plugin.toml").write_text("git") + profile_source = tmp_path / "profile" + profile_source.mkdir() + (profile_source / "config.toml").write_text("profile") + + profile_mounts = _stage_profile_mounts( + [{"source": str(profile_source), "target": "/home/user"}], str(instance_dir) + ) + result = _stack_profile_mounts_with_git_mounts( + profile_mounts, + [{"source": str(git_source), "target": "/home/user/.pi", "type": "bind"}], + 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" + + def test_non_overlapping_mounts_remain_separate(self, tmp_path) -> None: + """Unrelated profile and Git mounts retain their independent sources.""" + instance_dir = tmp_path / "instance" + git_source = tmp_path / "git" + git_source.mkdir() + profile_source = tmp_path / "profile" + profile_source.mkdir() + + profile_mounts = _stage_profile_mounts( + [{"source": str(profile_source), "target": "/home/user/.config"}], + str(instance_dir), + ) + git_mounts = [{"source": str(git_source), "target": "/home/user/.pi"}] + + assert ( + _stack_profile_mounts_with_git_mounts( + profile_mounts, git_mounts, str(instance_dir) + ) + == profile_mounts + git_mounts + ) @pytest.mark.unit diff --git a/openspec/changes/fix-pi-container-mount-permissions/change.md b/openspec/changes/fix-pi-container-mount-permissions/change.md index f54e641..d3543aa 100644 --- a/openspec/changes/fix-pi-container-mount-permissions/change.md +++ b/openspec/changes/fix-pi-container-mount-permissions/change.md @@ -12,6 +12,7 @@ After implementing configurable tool container home directories, new `pi-agent` 4. `npm_global` packages are installed with `RUN npm install -g ...` as root into the system npm prefix, so the non-root container user cannot update them. 5. Once the repo mount moves out of `/workspace`, the generated `/workspace` compatibility symlink is created in the image as root. The non-root entrypoint cannot replace it (write permission is required on `/`), so container startup fails. 6. Older images baked a literal `{{WORKSPACE_NAME}}` directory into `/home/user`, which survives alongside the real repo-named mount directory. +7. Config-profile file mounts use canonical profile storage owned by the API process. A non-root container user therefore cannot write to writable bind mounts. When a directory-level profile mount and a Git mount share or nest under the same target, Docker bind mounting masks the earlier source rather than merging their files. ## Fix @@ -40,7 +41,9 @@ After implementing configurable tool container home directories, new `pi-agent` 6. Remove the explicit repo mount from the built-in `pi-agent` manifest so the repo mount is synthesized by `compile_compose` rather than depending on tool config. Add a follow-up Alembic data migration that strips the `source_type: repo` mount from the manifest. 7. Add `_get_repository_mount_name()` helper. When the instance is bound to a workspace, the helper returns the basename of `workspace.path`. For legacy repo-only instances it falls back to parsing the remote URL like `git clone` would, then to the user-provided repository name. 8. Switch workspace storage layout to `/data/working-copies/{workspace_id}/{repo_name}/` so `git clone` creates the repo-named directory naturally, making `workspace.path.basename` the correct container mount name. This replaces the previous `/data/working-copies/{repo_id}/{workspace_name}/` layout. -9. Update unit tests for the new behavior. +9. Stage every config-profile bind-mount source into the instance directory before compose generation. This makes writable mounts user-owned without changing the shared canonical profile source. +10. Replace overlapping profile and Git bind mounts with a per-instance composite source. The composite copies Git content first and profile content second, preserving Git siblings while allowing profile files to override matching paths; it is then chowned with the other staged mounts. This removes duplicate/nested Docker mounts rather than relying on mount order to merge them. +11. Update unit tests for the new behavior. ## Affected files diff --git a/openspec/changes/fix-pi-container-mount-permissions/tasks.md b/openspec/changes/fix-pi-container-mount-permissions/tasks.md index f00e0af..88a1e24 100644 --- a/openspec/changes/fix-pi-container-mount-permissions/tasks.md +++ b/openspec/changes/fix-pi-container-mount-permissions/tasks.md @@ -12,5 +12,8 @@ - [x] Remove compose-level `user: 0:0` override so entrypoint can drop privileges - [x] Pass manifest-declared container user to terminal sessions via `docker exec --user` - [x] Update unit tests for container user/terminal changes +- [x] Stage profile bind mounts per instance so writable sources are owned by the container user +- [x] Composite overlapping profile and Git mounts into one per-instance bind source +- [x] Add regression tests for mount composition, ownership staging, and mount order - [ ] Run quality gates for container user/terminal changes - [ ] Commit and push