From 0952aa8217792e9f6bd2512c13be65ff95b4e0b1 Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Fri, 29 May 2026 11:35:27 +0200 Subject: [PATCH] fix: sort mount volumes by specificity to prevent parent mounts hiding children When git repo mounts and regular file mounts have overlapping target paths, broader parent mounts hide deeper child mounts because Docker Compose applies volumes in array order. - Add sort_volumes_by_specificity() to docker.py: - Sorts by target path depth (parent paths first, child paths last) - Logs warnings for duplicate targets - Handles :bind and :ro suffixes correctly - Integrate into manifest flow (compile_compose): - Sorts manifest mounts + EXTRA_VOLUMES before writing compose - Integrate into legacy flow (_modify_compose_file): - Sorts after appending extra_volumes to existing template volumes - Add 6 unit tests covering parent/child ordering, stable sort, type suffixes, empty list, single volume, and duplicate warnings. Quality gates: pytest (214 passed, 6 pre-existing), tsc --noEmit (clean) --- apps/api/src/api/tool_instances.py | 7 + apps/api/src/services/docker.py | 42 ++++++ apps/api/src/services/manifest_compiler.py | 4 +- apps/api/tests/unit/test_docker_service.py | 62 ++++++++- .../mount-specificity-ordering/.openspec.yaml | 4 + .../designs/mount-specificity-ordering.md | 55 ++++++++ .../mount-specificity-ordering.md | 130 ++++++++++++++++++ .../proposals/mount-specificity-ordering.md | 19 +++ openspec/specs/mount-specificity-ordering.md | 45 ++++++ openspec/tasks/mount-specificity-ordering.md | 12 ++ 10 files changed, 378 insertions(+), 2 deletions(-) create mode 100644 openspec/changes/mount-specificity-ordering/.openspec.yaml create mode 100644 openspec/designs/mount-specificity-ordering.md create mode 100644 openspec/explorations/mount-specificity-ordering.md create mode 100644 openspec/proposals/mount-specificity-ordering.md create mode 100644 openspec/specs/mount-specificity-ordering.md create mode 100644 openspec/tasks/mount-specificity-ordering.md 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