Skip to content

Commit 1e31ffd

Browse files
committed
fix: classify named-volume sources by Docker's name grammar, not a prefix test
_validate_volume_references (Task 14) rejected any colon-form volume source not starting with '.' or '/' as an undeclared named volume, over-catching ~-prefixed bind mounts (~/data:/var) that docker compose config accepts. Classify by matching Docker's own volume-name grammar (stores.NAME_PATTERN) instead, and share it between the per-service shape check and the cross-document declaration check via one _classify_volume helper. A pre-existing Windows drive-letter residual (C:\data) was found while measuring this fix; it needs a bigger change to the shared colon-split, not this grammar swap, so it stays open and catalogued in planning/deferred.md.
1 parent c242809 commit 1e31ffd

9 files changed

Lines changed: 278 additions & 51 deletions

File tree

‎architecture/supported-subset.md‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -724,12 +724,25 @@ boolean-typed exception in this group, validated as an actual bool like
724724

725725
Short syntax only; the long mapping form raises. The `volumes` key itself
726726
must be a list — a bare string raises, rather than being destructured one
727-
character at a time. A `source:target` entry is one of two kinds, told
728-
apart by whether `source` starts with `.` or `/`:
729-
730-
- **Bind mount** (`source` starts with `.` or `/`): the host path, resolved
731-
against `--project-dir` when relative.
732-
- **Named volume** (`source` is a bare identifier, e.g.
727+
character at a time. A `source:target` entry is one of two kinds, told apart
728+
by whether `source` matches Docker's own volume-name grammar
729+
(`stores.NAME_PATTERN`, `^[a-zA-Z0-9][a-zA-Z0-9_.-]*$` — the identical
730+
pattern a secret/config name is checked against):
731+
732+
- **Bind mount** (`source` does not match the name grammar — `./rel`,
733+
`/abs`, a `~`-prefixed home-relative path, or a `${VAR}` reference): the
734+
host path, resolved against `--project-dir` when relative and passed
735+
through verbatim otherwise (a `~`-prefixed source is not expanded by
736+
compose2pod itself — it reaches the emitted script as written, the same as
737+
every other unresolved token). **Known residual:** a Windows drive-letter
738+
source (`C:\data`) is still misclassified as a named volume and rejected —
739+
the shared colon-split (`source, _, _ = volume.partition(":")`) takes the
740+
drive letter alone as `source`, which is itself a valid name-grammar match;
741+
Docker's own parser special-cases the leading `<letter>:\` before ever
742+
comparing it to a name grammar. Catalogued in `planning/deferred.md`, not
743+
fixed (needs a bigger change to the shared split, in both `parsing.py` and
744+
`emit.py`, not a grammar swap).
745+
- **Named volume** (`source` matches the name grammar, e.g.
733746
`pgdata:/var/lib/...`): passed through verbatim as `-v <name>:<target>` —
734747
no format validation, no path translation. Podman creates it implicitly
735748
with default options on first reference (same as plain `podman run -v`,
@@ -746,14 +759,10 @@ apart by whether `source` starts with `.` or `/`:
746759
(`_validate_volume_references`, `parsing.py`, Task 14, mirroring
747760
`_validate_network_references`). A declaration with `external: true` still
748761
counts. A bind mount or an anonymous volume needs no declaration at all —
749-
neither names a volume. A `${VAR}`-carrying source is exempted from the
750-
check entirely: measured against `docker compose config` v5.1.2, Docker
751-
resolves the variable first and then classifies the *result* (a bind-mount
752-
value needs no declaration, a bare-identifier value does) — which rule
753-
applies is a fact about the shell that will later run the generated script,
754-
not about this document, so it does not bind here either way (same
755-
`values.has_variable` carve-out every other host-state-dependent grammar in
756-
`parsing.py` already applies).
762+
neither names a volume, so neither needs the `${VAR}`-carrying-source
763+
carve-out other host-state-dependent grammars in `parsing.py` need either:
764+
`$`, `{`, and `}` are none of them name-grammar characters, so a
765+
`${VAR}`-carrying source is already excluded by the grammar match itself.
757766

758767
A single absolute container path with no `source:target` (e.g.
759768
`- /var/cache/models`) is accepted as an **anonymous volume** and emitted

‎compose2pod/parsing.py‎

Lines changed: 68 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,51 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None:
115115
_HEALTHCHECK_SCALAR_VALIDATORS[key](name, key, healthcheck[key])
116116

117117

118+
def _classify_volume(volume: str) -> tuple[str, str | None]:
119+
r"""Classify one short-syntax volume entry: its kind, and its source when the kind is 'named'.
120+
121+
Returns `("anonymous", None)` for a colon-less entry (a bare container
122+
path), `("named", source)` for a colon-form entry whose source matches
123+
Docker's own volume-name grammar (`stores.NAME_PATTERN`, the identical
124+
pattern a secret/config name is checked against:
125+
`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`), or `("bind", None)` for every other
126+
colon-form entry -- a relative (`./rel`) or absolute (`/abs`) host path, a
127+
`~`-prefixed home-relative path (`~/data`, measured ACCEPT against
128+
`docker compose config` v5.1.2 with no top-level declaration -- Docker
129+
expands it against the invoking user's home directory), or a `${VAR}`
130+
reference (`$`, `{`, `}` are none of them pattern characters either).
131+
132+
Shared by `_validate_service_volumes` (only cares whether an entry is
133+
anonymous, to check its own shape) and `_named_volume_source` (only cares
134+
whether an entry is named, to extract its source for the cross-document
135+
declaration check) -- one function defines the colon-form split exactly
136+
once, rather than each re-deriving it. An earlier version of this
137+
file split named-vs-bind by "does not start with '.' or '/'" instead of
138+
the name grammar, which wrongly swept up every other bind-mount spelling
139+
(tilde in particular) into "named" -- an over-rejection once paired with
140+
the reference check below, since neither needs a top-level declaration.
141+
142+
A genuine Windows drive-letter source (`C:\\data:/var`) is NOT fixed by
143+
this pattern swap: `source, _, _ = volume.partition(":")` splits on the
144+
FIRST colon regardless, so for that entry `source` is just `"C"` -- a
145+
single letter, which is itself a syntactically valid NAME_PATTERN match --
146+
not the full `"C:\\data"` a naive reading of "doesn't match the pattern"
147+
might suggest. Docker's own parser special-cases a leading `<letter>:\\`
148+
to keep the drive letter attached to the source before ever comparing it
149+
to a name grammar; this module (and `emit.py`'s `_volume_flags`, which
150+
shares the same first-colon split for the same reason) does not. Measured,
151+
still REJECTs post-fix -- a pre-existing, uncatalogued residual from when
152+
this check was introduced (`_named_volume_source`'s Task 14 predecessor),
153+
not something this change introduces or was scoped to close.
154+
"""
155+
if ":" not in volume:
156+
return "anonymous", None
157+
source, _, _ = volume.partition(":")
158+
if stores.NAME_PATTERN.fullmatch(source):
159+
return "named", source
160+
return "bind", None
161+
162+
118163
def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None:
119164
"""Check volumes is a list of short bind-mount entries."""
120165
volumes = svc.get("volumes")
@@ -128,35 +173,20 @@ def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None:
128173
if not isinstance(volume, str):
129174
msg = f"service {name!r}: only short volume syntax is supported"
130175
raise UnsupportedComposeError(msg)
131-
if ":" not in volume:
132-
# Anonymous volume: must be an absolute container path.
133-
if not volume.startswith("/"):
134-
msg = f"service {name!r}: anonymous volume '{volume}' must be an absolute path"
135-
raise UnsupportedComposeError(msg)
136-
continue
137-
# Colon-containing volume: bind mount (host path source) or named volume
138-
# (bare identifier source) — both are accepted; podman creates a named
139-
# volume implicitly on first reference.
176+
kind, _ = _classify_volume(volume)
177+
if kind == "anonymous" and not volume.startswith("/"):
178+
msg = f"service {name!r}: anonymous volume '{volume}' must be an absolute path"
179+
raise UnsupportedComposeError(msg)
180+
# A named or bind entry needs no further shape check here -- both are
181+
# accepted; podman creates a named volume implicitly on first
182+
# reference, and _validate_volume_references (below) is the one place
183+
# that cross-checks a named entry's source against a declaration.
140184

141185

142186
def _named_volume_source(volume: str) -> str | None:
143-
"""Return a colon-form volume entry's bare-identifier source, or None if it needs no declaration.
144-
145-
Mirrors the same bind-vs-named split `emit.py`'s `_volume_flags` already
146-
makes when translating a relative bind mount: a colon-form entry's source is
147-
a bind mount when it starts with `.` or `/` (a host path, relative or
148-
absolute) -- Docker requires no top-level declaration for either. No colon at
149-
all means an anonymous volume (a bare container path), also declaration-free.
150-
Anything else is a bare identifier: the *name* of a volume Docker requires to
151-
be declared in the top-level `volumes:` block, even though podman itself
152-
would create it implicitly on first reference regardless.
153-
"""
154-
if ":" not in volume:
155-
return None
156-
source, _, _ = volume.partition(":")
157-
if source.startswith((".", "/")):
158-
return None
159-
return source
187+
"""Return a colon-form volume entry's bare-identifier source, or None if it needs no declaration."""
188+
kind, source = _classify_volume(volume)
189+
return source if kind == "named" else None
160190

161191

162192
def _validate_image(name: str, svc: dict[str, Any]) -> None:
@@ -796,14 +826,17 @@ def _validate_volume_references(compose: dict[str, Any], services: dict[str, Any
796826
is a list of strings, and that a colon-less entry is an absolute path), so
797827
`svc.get("volumes") or []` here is safe to iterate.
798828
799-
A `${VAR}`-carrying source is exempted, matching every other host-state-
800-
dependent grammar in this file (`values.has_variable`): measured, Docker
801-
resolves the variable first and classifies the *result* -- the same
802-
`${NAME}:/var` document ACCEPTS with no declaration when the shell resolves
803-
`NAME` to a bind-mount path (`/abs` or `./rel`) and REJECTS undeclared when
804-
it resolves to a bare identifier. Which rule applies is a fact about the
805-
shell that will later run the generated script, not about this document, so
806-
it does not bind here either way.
829+
A `${VAR}`-carrying source needs no separate carve-out the way other
830+
host-state-dependent grammars in this file need `values.has_variable`:
831+
measured, Docker resolves the variable first and classifies the *result*
832+
-- the same `${NAME}:/var` document ACCEPTS with no declaration when the
833+
shell resolves `NAME` to a bind-mount path (`/abs` or `./rel`) and REJECTS
834+
undeclared when it resolves to a bare identifier, a fact about the shell
835+
that will later run the generated script, not about this document, so it
836+
does not bind here either way. `_named_volume_source` already returns None
837+
for any `${...}`-carrying source unconditionally -- `$`, `{`, and `}` are
838+
none of them `stores.NAME_PATTERN` characters -- so this function never
839+
needs to ask `values.has_variable` itself.
807840
"""
808841
top_volumes = compose.get("volumes")
809842
if top_volumes is not None and not isinstance(top_volumes, dict):
@@ -813,7 +846,7 @@ def _validate_volume_references(compose: dict[str, Any], services: dict[str, Any
813846
for name, svc in services.items():
814847
for volume in svc.get("volumes") or []:
815848
source = _named_volume_source(volume)
816-
if source is None or values.has_variable(source):
849+
if source is None:
817850
continue
818851
if source not in declared:
819852
msg = f"service {name!r}: refers to undefined volume {source!r}"

‎compose2pod/stores.py‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,13 @@
1919

2020

2121
_LONG_FORM_KEYS = {"source", "target", "uid", "gid", "mode"}
22-
_NAME = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
22+
# Docker's shared identifier grammar for a secret/config *name* -- and, per
23+
# `parsing._named_volume_source`, for telling a named-volume reference apart
24+
# from a bind-mount source (a relative or absolute path, or a `~`-prefixed
25+
# home-relative path all fail this pattern, since none of `.`, `/`, `~` is a
26+
# pattern character). Public (no leading underscore) because parsing.py, in a
27+
# different module, reuses it rather than duplicating it.
28+
NAME_PATTERN = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
2329
_ENV_NAME = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*$")
2430

2531

@@ -61,7 +67,7 @@ class StoreKind:
6167

6268

6369
def _validate_def(name: str, definition: Any, kind: StoreKind) -> None: # noqa: ANN401 - Compose values are untyped
64-
if not _NAME.fullmatch(name):
70+
if not NAME_PATTERN.fullmatch(name):
6571
msg = f"{kind.label} name {name!r} must match [a-zA-Z0-9][a-zA-Z0-9_.-]*"
6672
raise UnsupportedComposeError(msg)
6773
if not isinstance(definition, dict):
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
---
2+
summary: Fixes an over-rejection Task 14 (2026-07-15.14) introduced -- `_validate_volume_references` classified any colon-form volume source not starting with `.` or `/` as a named volume requiring a top-level declaration, wrongly rejecting `~`-prefixed bind mounts (`~/data:/var`) Docker accepts. The classifier now matches Docker's own volume-name grammar (`stores.NAME_PATTERN`) instead. 1 new corpus file, 0 hard violations; 1 pre-existing Windows drive-letter residual discovered and catalogued, not fixed (needs a bigger change to the shared colon-split, not this grammar swap).
3+
---
4+
5+
# Change: tilde bind-mount volume-reference parity
6+
7+
**Lane:** lightweight — 2 source files, no new module, no public-API change,
8+
straightforward tests.
9+
10+
## Goal
11+
12+
`volumes: [~/data:/var]` with no top-level `volumes:` block: `docker compose
13+
config` v5.1.2 ACCEPTs it (a bind mount, expanded against the invoking user's
14+
home directory). compose2pod REJECTs it ("refers to undefined volume
15+
'~/data'"). Fix the over-rejection without weakening the undefined-named-
16+
volume rejection Task 14 closed.
17+
18+
## Approach
19+
20+
`_named_volume_source`'s classifier tested "source does not start with `.` or
21+
`/`" to decide "named volume, needs declaring" — true for a bare identifier
22+
(`data`), but also true for every other bind-mount spelling that isn't a dot-
23+
or slash-prefixed path: `~/data`, and (separately, not fixed here — see
24+
Non-goals) a Windows drive path.
25+
26+
Replaced the startswith test with a positive match against
27+
`stores.NAME_PATTERN` (`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`) — the identical
28+
grammar Docker's own compose-go already applies to a secret/config *name*,
29+
reused rather than re-derived, since parsing.py already imports `stores` (no
30+
cycle: `stores.py` never imports `parsing`). Made public (`NAME_PATTERN`, no
31+
leading underscore) since a second module now reuses it.
32+
33+
A source is now a NAMED VOLUME iff it matches this pattern; everything else —
34+
`./rel`, `/abs`, `~/data`, `${VAR}` — is not a name at all, so none of them
35+
need a declaration. The `${VAR}` case needed a separate `values.has_variable`
36+
carve-out before; it doesn't now, since `$`/`{`/`}` are none of them pattern
37+
characters — `_named_volume_source` already returns `None` for any
38+
`${...}`-carrying source unconditionally, so the redundant `has_variable`
39+
check was dropped from `_validate_volume_references`.
40+
41+
**Minor refactor:** `_validate_service_volumes` (the per-service shape check)
42+
and `_named_volume_source` (the cross-document declaration check) both used
43+
to re-derive the colon-form split independently. Extracted one
44+
`_classify_volume(volume) -> ("anonymous" | "named" | "bind", source |
45+
None)` helper both now call, so the split is defined exactly once.
46+
47+
## Files
48+
49+
- `compose2pod/stores.py` — `_NAME` renamed to public `NAME_PATTERN` (one use
50+
site updated); no behavior change.
51+
- `compose2pod/parsing.py` — new `_classify_volume` helper;
52+
`_validate_service_volumes` and `_named_volume_source` both call it instead
53+
of re-deriving the colon-form split; `_validate_volume_references` drops
54+
its now-redundant `values.has_variable` branch.
55+
- `tests/test_parsing.py` — `test_tilde_bind_mount_needs_no_declaration` (new,
56+
TDD red confirmed pre-fix), `test_hyphenated_and_underscored_named_volume_must_be_declared`
57+
(new, confirms the undefined-named-volume rejection is not weakened).
58+
- `tests/conformance/corpus/volume_tilde_bind_no_declaration.yaml` — new.
59+
- `tests/conformance/corpus/volume_windows_drive_letter_bind.yaml` — new
60+
(documents the residual below as a visible `over-reject`, not a hard
61+
violation).
62+
- `tests/conformance/test_corpus.py` —
63+
`test_volume_tilde_bind_is_no_longer_an_over_rejection` (new, asserts the
64+
verdict is exactly `"both-accept"`, mirroring
65+
`test_networks_default_implicit_is_no_longer_an_over_rejection`).
66+
- `planning/deferred.md` — new bullet cataloguing the Windows drive-letter
67+
residual (see Non-goals).
68+
69+
## Non-goals
70+
71+
**Windows drive-letter volume source is NOT fixed here.**
72+
`volumes: ["C:\data:/var"]` with no top-level declaration: Docker ACCEPTs
73+
(measured — compose-go special-cases a leading `<letter>:\` so the drive
74+
letter stays attached to the source, resolving to `{source: C:\data, target:
75+
/var}`). compose2pod still REJECTs ("refers to undefined volume 'C'"),
76+
unchanged by this fix: the shared colon-split (`source, _, _ =
77+
volume.partition(":")`, in both `_classify_volume` here and `emit.py`'s
78+
`_volume_flags`) splits on the *first* colon regardless, so for this entry
79+
`source` is just the single letter `"C"` — itself a syntactically valid
80+
`NAME_PATTERN` match, not the unparseable string a plain reading of "doesn't
81+
match the pattern" would suggest. Closing this needs genuine Windows-drive
82+
detection ahead of the split, in both call sites — a materially bigger change
83+
than this task's grammar swap, and one that also touches real emission
84+
(`emit.py`), not just the gate. Catalogued in `planning/deferred.md` under
85+
"Forms Docker accepts that compose2pod does not yet parse" with a revisit
86+
trigger; discovered while measuring this fix, pre-existing since Task 14
87+
introduced the named-volume reference check, not introduced by this change.
88+
89+
## Testing
90+
91+
**Unit** (TDD): `test_tilde_bind_mount_needs_no_declaration` confirmed
92+
`UnsupportedComposeError` pre-fix (`service 'app': refers to undefined
93+
volume '~/data'`) via manual invocation against pre-fix `parsing.py`, passes
94+
post-fix. `test_hyphenated_and_underscored_named_volume_must_be_declared`
95+
passed both pre- and post-fix (guards against weakening the Task 14 rejection
96+
this task must not touch). Full suite: `just test-ci` — 1239 passed, 100%
97+
line coverage.
98+
99+
**Conformance**: `volume_tilde_bind_no_declaration.yaml` — confirmed
100+
`AssertionError` pre-fix (docker ACCEPTs, we REJECTed) running under
101+
`-m conformance` against pre-fix `parsing.py`; passes post-fix.
102+
`test_volume_tilde_bind_is_no_longer_an_over_rejection` confirmed
103+
`'over-reject' == 'both-accept'` pre-fix, `"both-accept"` post-fix — the
104+
stronger claim a generic corpus run alone would not have failed on.
105+
`volume_windows_drive_letter_bind.yaml` — confirmed REJECT both before and
106+
after this change (unaffected residual, now visible in the harness's
107+
over-rejection summary instead of silently uncaught).
108+
109+
`just test-conformance`: 791 passed, 4 skipped, **0 hard violations**, 13
110+
over-rejections (the 12 `2026-07-15.14` carried forward, plus the new
111+
Windows drive-letter one).
112+
113+
`just lint-ci` and `just check-planning` both clean.
114+
115+
## Risk
116+
117+
Low. Widens acceptance only in the direction the parity rule allows (Docker
118+
accepts, podman can express a bind mount identically regardless of the host
119+
path's spelling); the undefined-named-volume rejection Task 14 closed is
120+
unchanged (still enforced via the same pattern, just tested positively
121+
instead of by exclusion).

0 commit comments

Comments
 (0)