merge: sort mounts by specificity
This commit is contained in:
@@ -56,6 +56,7 @@ from src.services.docker import (
|
|||||||
get_container_status,
|
get_container_status,
|
||||||
recreate_tunnel,
|
recreate_tunnel,
|
||||||
render_compose_template,
|
render_compose_template,
|
||||||
|
sort_volumes_by_specificity,
|
||||||
start_cloudflared_tunnel,
|
start_cloudflared_tunnel,
|
||||||
stop_cloudflared_tunnel,
|
stop_cloudflared_tunnel,
|
||||||
wait_for_container_running,
|
wait_for_container_running,
|
||||||
@@ -599,6 +600,12 @@ def _modify_compose_file(
|
|||||||
else:
|
else:
|
||||||
service_config["volumes"].append(f"{source}:{target}:{vol_type}")
|
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
|
break # Only modify the first service
|
||||||
|
|
||||||
# Write back
|
# Write back
|
||||||
|
|||||||
@@ -1,12 +1,54 @@
|
|||||||
"""Docker service for managing tool instances."""
|
"""Docker service for managing tool instances."""
|
||||||
|
|
||||||
|
import logging
|
||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
import subprocess
|
import subprocess
|
||||||
import time
|
import time
|
||||||
|
from collections import Counter
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any
|
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:
|
def render_compose_template(template: str, variables: dict[str, Any]) -> str:
|
||||||
"""Render a Docker Compose template with variable substitution.
|
"""Render a Docker Compose template with variable substitution.
|
||||||
|
|||||||
@@ -8,6 +8,8 @@ from typing import Any
|
|||||||
|
|
||||||
import yaml
|
import yaml
|
||||||
|
|
||||||
|
from src.services.docker import sort_volumes_by_specificity
|
||||||
|
|
||||||
|
|
||||||
def resolve_base(manifest: dict) -> dict:
|
def resolve_base(manifest: dict) -> dict:
|
||||||
"""Merge a base definition into a tool manifest.
|
"""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)
|
volumes.append(vol_str)
|
||||||
|
|
||||||
if volumes:
|
if volumes:
|
||||||
service["volumes"] = volumes
|
service["volumes"] = sort_volumes_by_specificity(volumes)
|
||||||
|
|
||||||
compose = {"services": {"app": service}}
|
compose = {"services": {"app": service}}
|
||||||
return yaml.dump(compose, default_flow_style=False)
|
return yaml.dump(compose, default_flow_style=False)
|
||||||
|
|||||||
@@ -2,7 +2,13 @@
|
|||||||
|
|
||||||
from unittest.mock import MagicMock, patch
|
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:
|
class TestGetContainerId:
|
||||||
@@ -50,3 +56,57 @@ class TestGetContainerName:
|
|||||||
result = get_container_name("missing")
|
result = get_container_name("missing")
|
||||||
|
|
||||||
assert result is None
|
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
|
||||||
|
|||||||
@@ -0,0 +1,4 @@
|
|||||||
|
name: mount-specificity-ordering
|
||||||
|
status: completed
|
||||||
|
type: fix
|
||||||
|
priority: high
|
||||||
@@ -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
|
||||||
@@ -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`)
|
||||||
@@ -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
|
||||||
@@ -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.
|
||||||
@@ -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
|
||||||
Reference in New Issue
Block a user