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)
This commit is contained in:
@@ -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`)
|
||||
Reference in New Issue
Block a user