diff --git a/apps/api/src/api/tool_instances.py b/apps/api/src/api/tool_instances.py index e1d6f6d..c58b26f 100644 --- a/apps/api/src/api/tool_instances.py +++ b/apps/api/src/api/tool_instances.py @@ -56,6 +56,7 @@ from src.services.docker import ( get_container_status, recreate_tunnel, render_compose_template, + sort_volumes_by_specificity, start_cloudflared_tunnel, stop_cloudflared_tunnel, wait_for_container_running, @@ -599,6 +600,12 @@ def _modify_compose_file( else: service_config["volumes"].append(f"{source}:{target}:{vol_type}") + # Sort volumes so parent paths come before child paths + if service_config.get("volumes"): + service_config["volumes"] = sort_volumes_by_specificity( + service_config["volumes"] + ) + break # Only modify the first service # Write back diff --git a/apps/api/src/services/docker.py b/apps/api/src/services/docker.py index 691c9b3..d179d92 100644 --- a/apps/api/src/services/docker.py +++ b/apps/api/src/services/docker.py @@ -1,12 +1,54 @@ """Docker service for managing tool instances.""" +import logging import os import re import subprocess import time +from collections import Counter from pathlib import Path from typing import Any +logger = logging.getLogger(__name__) + + +def sort_volumes_by_specificity(volumes: list[str]) -> list[str]: + """Sort volume strings so parent paths come before child paths. + + Docker Compose mounts volumes in array order. A later mount at a parent + path hides earlier mounts at child paths. By sorting shallow paths first + and deep paths last, deeper (more specific) mounts overlay correctly. + + Volume format: source:target or source:target:type + + Args: + volumes: List of Docker volume mount strings. + + Returns: + Sorted list with parent paths before child paths. + """ + + def _target_depth(vol: str) -> int: + parts = vol.split(":") + if len(parts) < 2: + return 0 + target = parts[1].rstrip("/") + if not target or target == "/": + return 0 + return target.count("/") + + # Detect duplicate targets and warn + targets = [] + for vol in volumes: + parts = vol.split(":") + targets.append(parts[1] if len(parts) > 1 else "") + dupes = [t for t, c in Counter(targets).items() if c > 1] + if dupes: + logger.warning("Duplicate mount targets detected: %s", dupes) + + # Stable sort: parent paths first, child paths last + return sorted(volumes, key=_target_depth) + def render_compose_template(template: str, variables: dict[str, Any]) -> str: """Render a Docker Compose template with variable substitution. diff --git a/apps/api/src/services/manifest_compiler.py b/apps/api/src/services/manifest_compiler.py index 75166b8..11f18a8 100644 --- a/apps/api/src/services/manifest_compiler.py +++ b/apps/api/src/services/manifest_compiler.py @@ -8,6 +8,8 @@ from typing import Any import yaml +from src.services.docker import sort_volumes_by_specificity + def resolve_base(manifest: dict) -> dict: """Merge a base definition into a tool manifest. @@ -303,7 +305,7 @@ def compile_compose(manifest: dict, variables: dict[str, Any]) -> str: volumes.append(vol_str) if volumes: - service["volumes"] = volumes + service["volumes"] = sort_volumes_by_specificity(volumes) compose = {"services": {"app": service}} return yaml.dump(compose, default_flow_style=False) diff --git a/apps/api/tests/unit/test_docker_service.py b/apps/api/tests/unit/test_docker_service.py index ec2fa6a..49ca437 100644 --- a/apps/api/tests/unit/test_docker_service.py +++ b/apps/api/tests/unit/test_docker_service.py @@ -2,7 +2,13 @@ from unittest.mock import MagicMock, patch -from src.services.docker import get_container_id, get_container_name +import logging + +from src.services.docker import ( + get_container_id, + get_container_name, + sort_volumes_by_specificity, +) class TestGetContainerId: @@ -50,3 +56,57 @@ class TestGetContainerName: result = get_container_name("missing") assert result is None + + +class TestSortVolumesBySpecificity: + """Tests for sort_volumes_by_specificity.""" + + def test_parent_before_child(self) -> None: + """A repo mount to /workspace/x should come before a file mount to /workspace/x/y/config.json.""" + volumes = [ + "/repo/x/y/config.json:/workspace/x/y/config.json", + "/repo/x:/workspace/x", + ] + result = sort_volumes_by_specificity(volumes) + assert result[0] == "/repo/x:/workspace/x" + assert result[1] == "/repo/x/y/config.json:/workspace/x/y/config.json" + + def test_stable_sort_for_equal_depth(self) -> None: + """Mounts at the same depth preserve input order.""" + volumes = [ + "/a:/workspace/a", + "/b:/workspace/b", + "/c:/workspace/c", + ] + result = sort_volumes_by_specificity(volumes) + assert result == volumes + + def test_with_type_suffix(self) -> None: + """Volume strings with :bind or :ro suffixes are parsed correctly.""" + volumes = [ + "/repo/x/y/config.json:/workspace/x/y/config.json:bind", + "/repo/x:/workspace/x:bind", + ] + result = sort_volumes_by_specificity(volumes) + assert result[0] == "/repo/x:/workspace/x:bind" + assert result[1] == "/repo/x/y/config.json:/workspace/x/y/config.json:bind" + + def test_empty_list(self) -> None: + """Empty list returns empty list.""" + assert sort_volumes_by_specificity([]) == [] + + def test_single_volume(self) -> None: + """Single volume returns unchanged.""" + volumes = ["/repo:/workspace"] + assert sort_volumes_by_specificity(volumes) == volumes + + def test_duplicate_target_warning(self, caplog) -> None: + """Duplicate targets trigger a warning.""" + with caplog.at_level(logging.WARNING, logger="src.services.docker"): + volumes = [ + "/a:/workspace/x", + "/b:/workspace/x", + ] + sort_volumes_by_specificity(volumes) + assert "Duplicate mount targets detected" in caplog.text + assert "/workspace/x" in caplog.text diff --git a/openspec/changes/mount-specificity-ordering/.openspec.yaml b/openspec/changes/mount-specificity-ordering/.openspec.yaml new file mode 100644 index 0000000..14bec01 --- /dev/null +++ b/openspec/changes/mount-specificity-ordering/.openspec.yaml @@ -0,0 +1,4 @@ +name: mount-specificity-ordering +status: completed +type: fix +priority: high diff --git a/openspec/designs/mount-specificity-ordering.md b/openspec/designs/mount-specificity-ordering.md new file mode 100644 index 0000000..748da12 --- /dev/null +++ b/openspec/designs/mount-specificity-ordering.md @@ -0,0 +1,55 @@ +# Design: Mount Specificity Ordering + +## Helper Function + +Add `sort_volumes_by_specificity` to a shared utilities module. The most appropriate location is `apps/api/src/services/docker.py` since it already contains Docker/Compose helpers, or a new small module. We'll add it to `apps/api/src/services/docker.py` to keep the change minimal. + +## Sorting Logic + +```python +def _target_depth(vol: str) -> int: + parts = vol.split(":") + if len(parts) < 2: + return 0 + target = parts[1].rstrip("/") + if not target or target == "/": + return 0 + return target.count("/") +``` + +Stable sort: `sorted(volumes, key=_target_depth)`. + +## Warning Logic + +```python +def sort_volumes_by_specificity(volumes: list[str]) -> list[str]: + from collections import Counter + targets = [v.split(":")[1] if ":" in v else "" for v in volumes] + duplicates = [t for t, c in Counter(targets).items() if c > 1] + if duplicates: + logger.warning("Duplicate mount targets detected: %s", duplicates) + return sorted(volumes, key=_target_depth) +``` + +## Call Sites + +### manifest_compiler.py +In `compile_compose()`, after building the `volumes` list and before assigning: +```python +from src.services.docker import sort_volumes_by_specificity +volumes = sort_volumes_by_specificity(volumes) +service["volumes"] = volumes +``` + +### tool_instances.py +In `_modify_compose_file()`, after appending `extra_volumes` and before write: +```python +from src.services.docker import sort_volumes_by_specificity +service_config["volumes"] = sort_volumes_by_specificity(service_config["volumes"]) +``` + +## Files Changed +- `apps/api/src/services/docker.py` — add helper +- `apps/api/src/services/manifest_compiler.py` — sort manifest volumes +- `apps/api/src/api/tool_instances.py` — sort legacy volumes +- `apps/api/tests/unit/test_docker_service.py` — add tests diff --git a/openspec/explorations/mount-specificity-ordering.md b/openspec/explorations/mount-specificity-ordering.md new file mode 100644 index 0000000..4561ec8 --- /dev/null +++ b/openspec/explorations/mount-specificity-ordering.md @@ -0,0 +1,130 @@ +# Exploration: Mount Specificity Ordering + +## Problem Statement + +When a tool instance uses both git repo mounts and regular file mounts, overlapping +target paths can cause the broader mount to hide the more specific one. + +Example: +- Git repo mount: `repo/x/` → `/workspace/x` (directory) +- Regular file mount: `config.json` → `/workspace/x/y/config.json` (single file) + +Expected: `/workspace/x/y/config.json` contains the file mount contents. +Actual: The git mount overwrites `/workspace/x`, hiding `/workspace/x/y/config.json`. + +## Root Cause + +Docker Compose mounts volumes in the order they appear in the `volumes` array. +In Linux, a later mount at a parent path hides earlier mounts at child paths. + +Current code ordering: +1. `compile_compose` adds manifest-defined mounts first +2. `EXTRA_VOLUMES` (profile + git mounts) appended after + +Within `EXTRA_VOLUMES` in `start_instance`: +1. `profile_mounts` from `apply_resolved_profile` +2. `git_mount_volumes` from `_resolve_git_mounts` + +Since git mounts are appended after regular mounts, a broad git mount +(e.g. `/workspace/x`) overwrites a specific regular mount +(e.g. `/workspace/x/y/config.json`). + +## Affected Code Paths + +1. **Manifest flow**: `compile_compose()` in `manifest_compiler.py` + - Manifest mounts → `EXTRA_VOLUMES` + - All appended to compose `volumes` list in that order + +2. **Legacy flow**: `_modify_compose_file()` in `tool_instances.py` + - Existing template volumes → `extra_volumes` appended + - `extra_volumes` = profile_mounts + git_mount_volumes + +3. **Both flows**: Volume entries are strings like `source:target` or `source:target:bind` + - No structured sorting happens before write + +## Options + +### Option A: Sort by path depth (recommended) + +Sort all volume entries by target path specificity before writing compose. +- Shorter / parent paths first +- Deeper / child paths last +- Deeper mounts "win" by being layered on top + +**Pros:** +- Simple, predictable rule +- Works for all mount types (manifest, git, profile, template) +- Minimal code change + +**Cons:** +- Sorting by string length is naive (edge cases with similar paths) +- Need proper path-segment counting +- Doesn't handle exact same target conflicts + +### Option B: Detect and warn on overlaps + +Before writing compose, detect when any two mounts have overlapping target paths. +Log a warning and optionally fail fast. + +**Pros:** +- Surfaces conflicts to user early +- No silent data loss + +**Cons:** +- Doesn't actually fix the problem; user has to redesign mounts +- False positives for legitimate use cases (mounting different files into same tree) + +### Option C: Merge overlapping mounts into a single staging directory + +Instead of mounting multiple sources, stage all files into a single merged +directory on disk, then mount that single directory. + +**Pros:** +- Eliminates Docker mount ordering entirely +- Natural specificity: later file writes overwrite earlier ones + +**Cons:** +- Complex to implement correctly +- Git mounts would need to be cloned into staging area +- Breaks live file editing (bind mounts from host) +- Large refactor + +### Option D: Annotate mount specificity and merge in compiler + +Add a `priority` or `specificity` field to mount definitions. +Compiler sorts by this field. + +**Pros:** +- Explicit control + +**Cons:** +- Adds schema complexity +- Users must understand mount ordering +- Overkill for this use case + +## Recommendation + +**Option A** — sort by path depth. + +Rationale: +- Mount specificity should "just work" without user intervention +- Path depth is a natural proxy for specificity +- A parent directory mount is almost always less specific than a child file mount +- Implementation is ~20 lines in the compose write path +- Can be combined with Option B (warn on exact conflicts) for safety + +## Acceptance Criteria + +1. A git repo mount to `/workspace/x` and a regular mount to `/workspace/x/y/config.json` + both work: the config.json file contains the regular mount contents. +2. Multiple overlapping mounts sort consistently (deterministic). +3. Exact same-target conflicts are logged as warnings. +4. Both manifest and legacy flows behave correctly. +5. Unit tests cover overlap scenarios. + +## Files to Modify + +- `apps/api/src/services/manifest_compiler.py` — `compile_compose()` sorting +- `apps/api/src/api/tool_instances.py` — `_modify_compose_file()` sorting +- `apps/api/tests/unit/test_manifest_compiler.py` — new tests +- `apps/api/tests/unit/test_tool_instances.py` — new tests (or `test_tool_instances_legacy.py`) diff --git a/openspec/proposals/mount-specificity-ordering.md b/openspec/proposals/mount-specificity-ordering.md new file mode 100644 index 0000000..648c916 --- /dev/null +++ b/openspec/proposals/mount-specificity-ordering.md @@ -0,0 +1,19 @@ +# Proposal: Mount Specificity Ordering + +## Problem +When git repo mounts and regular file mounts have overlapping target paths, the broader mount hides the more specific one because Docker Compose applies volumes in array order. + +Example: repo → `/workspace/x` (directory) hides file → `/workspace/x/y/config.json`. + +## Solution +Sort all volume entries by target path depth before writing the compose file. Parent paths first, child paths last, so deeper mounts overlay correctly. + +## Scope +- `manifest_compiler.py` — `compile_compose()` +- `tool_instances.py` — `_modify_compose_file()` +- Unit tests for overlap scenarios + +## Impact +- Fixes silent mount hiding +- Deterministic ordering +- No user-facing API or schema changes diff --git a/openspec/specs/mount-specificity-ordering.md b/openspec/specs/mount-specificity-ordering.md new file mode 100644 index 0000000..12cda16 --- /dev/null +++ b/openspec/specs/mount-specificity-ordering.md @@ -0,0 +1,45 @@ +# Spec: Mount Specificity Ordering + +## Requirements + +1. All volume mount entries written to compose files must be sorted by target path depth. +2. Shorter / parent target paths appear **before** deeper / child target paths. +3. Deeper mounts are applied later by Docker, overlaying parent mounts correctly. +4. Exact same-target overlaps are logged as warnings. +5. Works for both manifest and legacy compose generation paths. + +## Algorithm + +```python +def sort_volumes_by_specificity(volumes: list[str]) -> list[str]: + """ + Sort volume strings so parent paths come before child paths. + Volume format: source:target or source:target:type + """ +``` + +1. Parse each volume string to extract the target path (second colon-delimited field). +2. Normalize the target: strip trailing `/`, collapse `//`. +3. Compute depth = number of `/`-separated segments. +4. Sort ascending by depth. Stable sort preserves input order for equal depths. +5. Detect exact same-target strings and log warnings. + +## Compose Format Handling + +- `source:target` → target is second field +- `source:target:bind` → target is second field +- `source:target:ro` → target is second field +- Split by `:` into at most 3 parts. Target is always index 1. + +## Integration Points + +- `compile_compose()` in `manifest_compiler.py`: sort `volumes` list before assigning to `service["volumes"]`. +- `_modify_compose_file()` in `tool_instances.py`: sort `service_config["volumes"]` after appending extra volumes. + +## Tests + +- Repo mount `/workspace/x` + file mount `/workspace/x/y/config.json` → file mount comes after. +- Same depth mounts → stable order preserved. +- Exact same target → warning logged. +- Empty volumes list → no-op. +- Volume with `type` suffix → parsed correctly. diff --git a/openspec/tasks/mount-specificity-ordering.md b/openspec/tasks/mount-specificity-ordering.md new file mode 100644 index 0000000..dc28bd5 --- /dev/null +++ b/openspec/tasks/mount-specificity-ordering.md @@ -0,0 +1,12 @@ +# Tasks: Mount Specificity Ordering + +- [x] Exploration written +- [x] Proposal written +- [x] Spec written +- [x] Design written +- [x] Implement `sort_volumes_by_specificity` in `docker.py` +- [x] Integrate sorting into `compile_compose` (manifest flow) +- [x] Integrate sorting into `_modify_compose_file` (legacy flow) +- [x] Add unit tests for helper and overlap scenarios +- [x] Run quality gates (pytest, tsc) +- [x] Commit and merge