From 37d5c510b262b51933f3e8e95952c80cb4abd04a Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:03:35 +0300 Subject: [PATCH 01/43] docs: spec closing the structural-key gate --- ...2026-07-13.10-close-structural-key-gate.md | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 planning/changes/2026-07-13.10-close-structural-key-gate.md diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md new file mode 100644 index 0000000..116f3bc --- /dev/null +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -0,0 +1,92 @@ +--- +summary: validate() now rejects malformed 'environment', 'env_file', and 'volumes', and emit rejects a malformed --artifact, so a bad document fails with UnsupportedComposeError instead of a raw AttributeError/TypeError/ValueError. +--- + +# Design: Close the structural-key gate + +## Summary + +`environment`, `env_file`, and `volumes` are structural keys: they carry no +`KeySpec`, so the `SERVICE_KEYS` validate loop never sees them, and unlike +`tmpfs`/`entrypoint`/`healthcheck` nobody wrote them a validator by hand. +`emit` walks their raw shape and crashes. This adds the three missing shape +validators in `parsing.py` and one options check in `emit._plan`, closing the +gate. No new abstraction. + +## Motivation + +From `audits/2026-07-13-docs-and-gate-audit.md`, all four reproduced against +the code: + +| Input | Today | +|-------|-------| +| `environment: "FOO=bar"` | `AttributeError: 'str' object has no attribute 'items'` | +| `env_file: 5` | `TypeError: 'int' object is not iterable` | +| `volumes: "/data:/data"` | destructured *character-wise*; reports `anonymous volume 'd' must be an absolute path` | +| `--artifact nocolon` | `ValueError: not enough values to unpack` out of `emit.py:209` | + +`volumes` also silently corrupts: `volumes: "/"` is **accepted** and emits +`-v "/"`, because the single character `/` happens to pass the +anonymous-volume check. + +This is not a new requirement. `decisions/2026-07-10-reject-parse-dont-validate.md` +rejects a typed model *on the grounds that this invariant already holds*: + +> **validate() owning every shape emit reads** made the shape-reading functions +> robust: a direct `emit_script(dict)` call on a *malformed* document now fails +> with `UnsupportedComposeError`, not a raw crash. + +That premise is false. This change makes it true again — it does not reopen the +decision. + +## Design + +Three validators in `parsing.py`, each mirroring the existing `_validate_tmpfs`, +wired into `_validate_service`: + +- **`environment`** — reuses the already-public `keys.validate_map`, since + list-or-mapping is exactly its contract. No new primitive. +- **`env_file`** — string or list. +- **`volumes`** — must be a list. Folded into the existing + `_validate_service_volumes` as an up-front shape check, ahead of the per-entry + loop that today iterates a string's characters. + +Each skips a `None` value: an empty `environment:` / `env_file:` / `volumes:` +key is valid Compose and today's emit already tolerates it. + +For `--artifact`, one `_validate_options(options)` called from the top of +`emit._plan`. `_plan` is the single traversal both `emit_script` and +`referenced_variables` project from, so one call site guards both public entry +points. `cli.main` already catches `UnsupportedComposeError` and returns exit 2, +so raising is sufficient — no argparse-level check is added. + +## Non-goals + +- **No structural-key registry.** `decisions/2026-07-12-reject-structural-key-registry.md` + rejects it, and nothing here needs one: three hand-written validators in the + module that already owns the gate is the pattern the codebase established. +- **No `--artifact` check in argparse.** `cli.main`'s existing + `UnsupportedComposeError` handler already yields a clean message and exit 2; + a second check would duplicate it. +- **No content validation** of volume/env_file *values* beyond shape. A + malformed path still surfaces as a podman error at run time, unchanged. + +## Testing + +`just test-ci` at 100%: +- `tests/test_parsing.py`: each of `environment` / `env_file` / `volumes` in a + wrong shape raises `UnsupportedComposeError` at `validate()`; each with a + `None` value is still accepted; the character-wise `volumes: "/"` case is now + rejected rather than silently emitting `-v "/"`. +- `tests/test_emit.py`: `emit_script` and `referenced_variables` both raise + `UnsupportedComposeError` on an `--artifact` with no colon; a valid `SRC:DST` + still emits the `podman cp` line. + +`just lint-ci` and `just check-planning` clean. + +## Risk + +- **A document that used to be accepted is now rejected** (low x low): only for + shapes that already crashed or silently corrupted. `volumes: "/"` is the one + input that changes from "accepted" to "rejected"; it emitted `-v "/"`, which + no one can have depended on. From a4553ba224c66a643f417e8bf31f9245a515d164 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:06:00 +0300 Subject: [PATCH 02/43] fix: reject a malformed 'environment' at the gate --- compose2pod/parsing.py | 9 ++++++++- tests/test_parsing.py | 9 +++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index f7d747d..85aa563 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -6,7 +6,7 @@ from compose2pod.exceptions import UnsupportedComposeError from compose2pod.graph import depends_on, hostnames from compose2pod.healthcheck import has_healthcheck, interval_seconds -from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS +from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, validate_map from compose2pod.pod import uses_pod_options, validate_pod_options from compose2pod.resources import validate_deploy @@ -68,6 +68,12 @@ def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) +def _validate_environment(name: str, svc: dict[str, Any]) -> None: + """Check environment is a list or mapping (a bare string would be walked as .items()).""" + if svc.get("environment") is not None: + validate_map(name, "environment", svc["environment"]) + + def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compose values are untyped """Validate one service; returns warnings, raises UnsupportedComposeError.""" if not isinstance(svc, dict): @@ -88,6 +94,7 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo _validate_service_volumes(name, svc) _validate_entrypoint(name, svc) _validate_tmpfs(name, svc) + _validate_environment(name, svc) validate_deploy(name, svc) validate_pod_options(name, svc) for key, spec in SERVICE_KEYS.items(): diff --git a/tests/test_parsing.py b/tests/test_parsing.py index ea696d6..129ef05 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -356,3 +356,12 @@ def test_deploy_reservations_cpus_rejected_at_gate(self) -> None: svc = {"image": "x", "deploy": {"resources": {"reservations": {"cpus": "0.5"}}}} with pytest.raises(UnsupportedComposeError, match=r"reservations.cpus is not supported"): validate({"services": {"app": svc}}) + + def test_string_environment_rejected_at_gate(self) -> None: + # Structural key with no KeySpec: used to reach emit and crash with + # AttributeError: 'str' object has no attribute 'items'. + with pytest.raises(UnsupportedComposeError, match=r"'environment' must be a list or mapping"): + validate({"services": {"app": {"image": "x", "environment": "FOO=bar"}}}) + + def test_null_environment_is_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "environment": None}}}) == [] From fd48b9d35eb6375dbce0d050ec703f3e02e6dbf9 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:09:24 +0300 Subject: [PATCH 03/43] fix: reject a malformed 'env_file' at the gate --- compose2pod/parsing.py | 9 +++++++++ tests/test_parsing.py | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 85aa563..e08cd74 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -74,6 +74,14 @@ def _validate_environment(name: str, svc: dict[str, Any]) -> None: validate_map(name, "environment", svc["environment"]) +def _validate_env_file(name: str, svc: dict[str, Any]) -> None: + """Check env_file is a string or list (emit iterates it).""" + env_file = svc.get("env_file") + if env_file is not None and not isinstance(env_file, str | list): + msg = f"service {name!r}: 'env_file' must be a string or list" + raise UnsupportedComposeError(msg) + + def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compose values are untyped """Validate one service; returns warnings, raises UnsupportedComposeError.""" if not isinstance(svc, dict): @@ -95,6 +103,7 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo _validate_entrypoint(name, svc) _validate_tmpfs(name, svc) _validate_environment(name, svc) + _validate_env_file(name, svc) validate_deploy(name, svc) validate_pod_options(name, svc) for key, spec in SERVICE_KEYS.items(): diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 129ef05..e9b5284 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -365,3 +365,13 @@ def test_string_environment_rejected_at_gate(self) -> None: def test_null_environment_is_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "environment": None}}}) == [] + + def test_non_string_non_list_env_file_rejected_at_gate(self) -> None: + # Used to reach emit and crash with TypeError: 'int' object is not iterable. + with pytest.raises(UnsupportedComposeError, match=r"'env_file' must be a string or list"): + validate({"services": {"app": {"image": "x", "env_file": 5}}}) + + def test_string_and_list_env_file_are_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "env_file": "tests.env"}}}) == [] + assert validate({"services": {"app": {"image": "x", "env_file": ["a.env", "b.env"]}}}) == [] + assert validate({"services": {"app": {"image": "x", "env_file": None}}}) == [] From 61084dc5d775dd8e6402b48f5d84fcdb87095435 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:13:35 +0300 Subject: [PATCH 04/43] fix: validate env_file list entries are strings at gate Check that each element in env_file list is a string, consistent with _validate_service_volumes. Prevents TypeError from reaching emit when env_file contains non-string values like env_file: [5]. --- compose2pod/parsing.py | 11 +++++++++-- tests/test_parsing.py | 5 +++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index e08cd74..22ea423 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -75,11 +75,18 @@ def _validate_environment(name: str, svc: dict[str, Any]) -> None: def _validate_env_file(name: str, svc: dict[str, Any]) -> None: - """Check env_file is a string or list (emit iterates it).""" + """Check env_file is a string or list of strings (emit iterates it).""" env_file = svc.get("env_file") - if env_file is not None and not isinstance(env_file, str | list): + if env_file is None: + return + if not isinstance(env_file, str | list): msg = f"service {name!r}: 'env_file' must be a string or list" raise UnsupportedComposeError(msg) + if isinstance(env_file, list): + for entry in env_file: + if not isinstance(entry, str): + msg = f"service {name!r}: 'env_file' entry must be a string" + raise UnsupportedComposeError(msg) def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compose values are untyped diff --git a/tests/test_parsing.py b/tests/test_parsing.py index e9b5284..3eed4e7 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -375,3 +375,8 @@ def test_string_and_list_env_file_are_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "env_file": "tests.env"}}}) == [] assert validate({"services": {"app": {"image": "x", "env_file": ["a.env", "b.env"]}}}) == [] assert validate({"services": {"app": {"image": "x", "env_file": None}}}) == [] + + def test_env_file_list_with_non_string_entry_rejected_at_gate(self) -> None: + # Used to reach emit and crash with TypeError: argument should be a str or an os.PathLike object. + with pytest.raises(UnsupportedComposeError, match=r"'env_file' entry must be a string"): + validate({"services": {"app": {"image": "x", "env_file": [5]}}}) From 1802ef264cd21a34077863a73ac31874862c7d77 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:16:28 +0300 Subject: [PATCH 05/43] fix: reject a non-list 'volumes' at the gate --- compose2pod/parsing.py | 11 +++++++++-- tests/test_parsing.py | 13 +++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 22ea423..7316620 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -37,8 +37,15 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None: - """Check volumes use short bind-mount syntax only.""" - for volume in svc.get("volumes") or []: + """Check volumes is a list of short bind-mount entries.""" + volumes = svc.get("volumes") + if volumes is None: + return + if not isinstance(volumes, list): + # A string would be iterated character-wise by the loop below. + msg = f"service {name!r}: 'volumes' must be a list" + raise UnsupportedComposeError(msg) + for volume in volumes: if not isinstance(volume, str): msg = f"service {name!r}: only short volume syntax is supported" raise UnsupportedComposeError(msg) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 3eed4e7..27bdd76 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -84,6 +84,19 @@ def test_relative_anonymous_volume_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="absolute"): validate({"services": {"app": {"image": "x", "volumes": ["./cache"]}}}) + def test_string_volumes_rejected_at_gate(self) -> None: + # Used to be iterated character-wise: "/data:/data" reported the nonsense + # "anonymous volume 'd'", and "/" was silently accepted as -v "/". + with pytest.raises(UnsupportedComposeError, match=r"'volumes' must be a list"): + validate({"services": {"app": {"image": "x", "volumes": "/data:/data"}}}) + + def test_single_slash_string_volumes_rejected_at_gate(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"'volumes' must be a list"): + validate({"services": {"app": {"image": "x", "volumes": "/"}}}) + + def test_null_volumes_is_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "volumes": None}}}) == [] + def test_no_services_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="no services"): validate({"services": {}}) From fc085b9b11addfa275d2d2d207dbbf76bf1ab375 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:19:44 +0300 Subject: [PATCH 06/43] fix: reject a malformed --artifact instead of crashing --- compose2pod/emit.py | 9 +++++++++ tests/test_emit.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/compose2pod/emit.py b/compose2pod/emit.py index f4e4495..6c447fd 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -221,6 +221,14 @@ def _emit_target(lines: list[str], tokens: list[Token], options: EmitOptions) -> lines.append("esac") +def _validate_options(options: EmitOptions) -> None: + """Check option values emit destructures (artifacts are split on ':').""" + for artifact in options.artifacts: + if ":" not in artifact: + msg = f"artifact {artifact!r} must be in SRC:DST form" + raise UnsupportedComposeError(msg) + + def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: """Walk the target's dependency closure once, building the script and its variables. @@ -229,6 +237,7 @@ def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: have their `Expand` variables collected, so the script and the variable list cannot disagree about what the script expands at run time. """ + _validate_options(options) services = compose["services"] hosts = hostnames(services) order = startup_order(services, options.target) diff --git a/tests/test_emit.py b/tests/test_emit.py index ecd68c3..d5cc8fe 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -800,6 +800,36 @@ def test_emit_script_accepts_a_valid_pod_name(self) -> None: assert "podman pod create --name test-pod.1" in script +class TestArtifactValidation: + def _options(self, artifacts: list[str]) -> EmitOptions: + return EmitOptions( + target="application", + ci_image="ci:latest", + command="", + pod="test-pod", + project_dir=".", + artifacts=artifacts, + allow_exit_codes=[], + ) + + def test_artifact_without_colon_raises(self, chats_compose: dict) -> None: + # Used to escape as a raw ValueError: not enough values to unpack. + options = self._options(["nocolon"]) + with pytest.raises(UnsupportedComposeError, match=r"artifact 'nocolon' must be in SRC:DST form"): + emit_script(compose=chats_compose, options=options) + + def test_artifact_without_colon_also_raises_from_referenced_variables(self, chats_compose: dict) -> None: + # The other public entry point projects the same _plan traversal. + options = self._options(["nocolon"]) + with pytest.raises(UnsupportedComposeError, match=r"artifact 'nocolon' must be in SRC:DST form"): + referenced_variables(chats_compose, options) + + def test_valid_artifact_still_emits_podman_cp(self, chats_compose: dict) -> None: + options = self._options(["/srv/out/junit.xml:junit.xml"]) + script = emit_script(compose=chats_compose, options=options) + assert "podman cp test-pod-application:/srv/out/junit.xml junit.xml || true" in script + + class TestPodmanVersionGuard: def _run_header(self, tmp_path: Path, podman_stub_body: str) -> "subprocess.CompletedProcess[str]": assert _SH is not None # sh is a POSIX baseline binary, always present From 8b63be5e10a5e2fae7644a8570576317692094f3 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:23:00 +0300 Subject: [PATCH 07/43] docs: promote the closed gate into architecture/ --- architecture/supported-subset.md | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index c7cd520..1823f4a 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -44,6 +44,13 @@ warns (ignored, behavior-neutral inside a single pod) or raises (`KEY: value`, `KEY:`). A null mapping value (`KEY:`) means "pass `KEY` through from the host", emitted as a bare `-e KEY` exactly like the list form `- KEY`. + The key itself must be a list or mapping; any other shape (e.g. a bare + string) raises at the gate. +- **`env_file`:** a string or a list. Any other shape raises + (`_validate_env_file`, `compose2pod/parsing.py`). Each list element must + itself be a string; a non-string element (e.g. `env_file: [5]`) raises the + same way. Each resolved path is passed through `--project-dir` when + relative, then emitted as a `--env-file` flag (`compose2pod/emit.py`). - **`entrypoint`:** string or list. List form is exec form; string form is shell form (`/bin/sh -c `), mirroring `command`. Emitted as `--entrypoint ` with the remaining tokens placed ahead of the @@ -304,8 +311,10 @@ honors both, refusing loudly on overlap rather than picking a precedence. ## Volumes -Short syntax only; the long mapping form raises. A `source:target` entry is -one of two kinds, told apart by whether `source` starts with `.` or `/`: +Short syntax only; the long mapping form raises. The `volumes` key itself +must be a list — a bare string raises, rather than being destructured one +character at a time. A `source:target` entry is one of two kinds, told +apart by whether `source` starts with `.` or `/`: - **Bind mount** (`source` starts with `.` or `/`): the host path, resolved against `--project-dir` when relative. @@ -500,7 +509,11 @@ check. A braced reference whose text after the name is not one of these operators (e.g. `${FOO!bar}`) is malformed and raises `UnsupportedComposeError` rather than silently dropping the trailing text. Tool/CLI-supplied values (`--project-dir`, `--image`, the pod name, -the `--command` override) are literal and never interpolated. The pod +the `--command` override) are literal and never interpolated. +`--artifact` must be in `SRC:DST` form; a value with no `:` raises +`UnsupportedComposeError` (`_validate_options`, `compose2pod/emit.py`), +guarding library callers of both `emit_script` and `referenced_variables` +as well as the CLI. The pod name is embedded into the pod-create line, the single-quoted `EXIT` trap, and the `-` store names (some of them unquoted), so it must be a shell-inert identifier — `emit_script` validates it against From 76f84d3b30f0a7d601e66288853485a49fbde736 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:36:06 +0300 Subject: [PATCH 08/43] fix: reject a service with no usable image at the gate 'image' is a structural key with no KeySpec, so a service with neither 'image' nor 'build' reached image_for and crashed with KeyError: 'image'; a non-string image (e.g. image: 5) reached shell.py and crashed with TypeError. Both now raise UnsupportedComposeError at validate() instead. --- compose2pod/parsing.py | 14 ++++++++++++++ tests/test_parsing.py | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 7316620..58d3d9e 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -60,6 +60,19 @@ def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None: # volume implicitly on first reference. +def _validate_image(name: str, svc: dict[str, Any]) -> None: + """Check the service has a usable image (image_for reads svc['image'] verbatim when there's no 'build').""" + if "build" in svc: + return + image = svc.get("image") + if image is None: + msg = f"service {name!r}: must set 'image' or 'build'" + raise UnsupportedComposeError(msg) + if not isinstance(image, str): + msg = f"service {name!r}: 'image' must be a string" + raise UnsupportedComposeError(msg) + + def _validate_entrypoint(name: str, svc: dict[str, Any]) -> None: """Check the structural entrypoint key's form (it is not a registry key).""" if "entrypoint" in svc and not isinstance(svc["entrypoint"], str | list): @@ -112,6 +125,7 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo raise UnsupportedComposeError(msg) if isinstance(svc.get("entrypoint"), str) and svc.get("command") is not None: warnings.append(f"service {name!r}: string entrypoint runs via shell; 'command' is ignored") + _validate_image(name, svc) _validate_service_healthcheck(name, svc) _validate_service_volumes(name, svc) _validate_entrypoint(name, svc) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 27bdd76..45f47e4 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -393,3 +393,22 @@ def test_env_file_list_with_non_string_entry_rejected_at_gate(self) -> None: # Used to reach emit and crash with TypeError: argument should be a str or an os.PathLike object. with pytest.raises(UnsupportedComposeError, match=r"'env_file' entry must be a string"): validate({"services": {"app": {"image": "x", "env_file": [5]}}}) + + def test_service_with_neither_image_nor_build_rejected_at_gate(self) -> None: + # Used to reach emit and crash with KeyError: 'image' (image_for). + with pytest.raises(UnsupportedComposeError, match=r"must set 'image' or 'build'"): + validate({"services": {"app": {}}}) + + def test_service_with_build_and_no_image_is_accepted(self) -> None: + # The normal CI case: --image replaces a build section's own image. + assert validate({"services": {"app": {"build": {"context": "."}}}}) == [] + + def test_non_string_image_rejected_at_gate(self) -> None: + # Used to reach emit and crash with TypeError: expected string or bytes-like object, got 'int'. + with pytest.raises(UnsupportedComposeError, match=r"'image' must be a string"): + validate({"services": {"app": {"image": 5}}}) + + def test_non_string_image_with_build_present_is_accepted(self) -> None: + # image_for never reads svc['image'] when 'build' is present, so a + # malformed image alongside 'build' cannot crash emit. + assert validate({"services": {"app": {"image": 5, "build": {"context": "."}}}}) == [] From 04699d3ffdd90679241dd2cac06d1e04087c5f63 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:36:51 +0300 Subject: [PATCH 09/43] fix: reject a malformed 'command' at the gate 'command' is command_tokens' sibling of 'entrypoint', which already has _validate_entrypoint; command had no validator at all. A non- string/non-list command (e.g. command: 5) crashed emit with TypeError: 'int' object is not iterable. A mapping command was worse: it was silently accepted and then mis-emitted -- only the mapping's key reached podman run, the value was dropped. Both now raise UnsupportedComposeError at validate(), mirroring _validate_entrypoint. --- compose2pod/parsing.py | 8 ++++++++ tests/test_parsing.py | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 58d3d9e..c4e588d 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -80,6 +80,13 @@ def _validate_entrypoint(name: str, svc: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) +def _validate_command(name: str, svc: dict[str, Any]) -> None: + """Check the structural command key's form (it is not a registry key).""" + if "command" in svc and not isinstance(svc["command"], str | list): + msg = f"service {name!r}: 'command' must be a string or list" + raise UnsupportedComposeError(msg) + + def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: """Check tmpfs is a string or list.""" tmpfs = svc.get("tmpfs") @@ -129,6 +136,7 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo _validate_service_healthcheck(name, svc) _validate_service_volumes(name, svc) _validate_entrypoint(name, svc) + _validate_command(name, svc) _validate_tmpfs(name, svc) _validate_environment(name, svc) _validate_env_file(name, svc) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 45f47e4..35d2162 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -412,3 +412,21 @@ def test_non_string_image_with_build_present_is_accepted(self) -> None: # image_for never reads svc['image'] when 'build' is present, so a # malformed image alongside 'build' cannot crash emit. assert validate({"services": {"app": {"image": 5, "build": {"context": "."}}}}) == [] + + def test_command_string_or_list_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "command": "run me"}}}) == [] + assert validate({"services": {"app": {"image": "x", "command": ["run", "me"]}}}) == [] + + def test_missing_command_is_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x"}}}) == [] + + def test_non_string_or_list_command_rejected_at_gate(self) -> None: + # Used to reach emit and crash with TypeError: 'int' object is not iterable. + with pytest.raises(UnsupportedComposeError, match=r"'command' must be a string or list"): + validate({"services": {"app": {"image": "x", "command": 5}}}) + + def test_mapping_command_rejected_at_gate(self) -> None: + # Used to be silently accepted and mis-emitted: only the mapping's key + # ('run') reached podman run, the value ('tests') was dropped. + with pytest.raises(UnsupportedComposeError, match=r"'command' must be a string or list"): + validate({"services": {"app": {"image": "x", "command": {"run": "tests"}}}}) From d3d2d87f092dc102136bb5f928b46ee621510ebb Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:37:17 +0300 Subject: [PATCH 10/43] fix: reject tmpfs list entries that aren't strings _validate_tmpfs only checked the outer shape (string or list); a non-string element (e.g. tmpfs: [5]) still reached emit and crashed in shell.py. Carries the same element-level check already applied to env_file (commit 61084dc) over to tmpfs. --- compose2pod/parsing.py | 11 +++++++++-- tests/test_parsing.py | 11 +++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index c4e588d..56f91f7 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -88,11 +88,18 @@ def _validate_command(name: str, svc: dict[str, Any]) -> None: def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: - """Check tmpfs is a string or list.""" + """Check tmpfs is a string or list of strings (emit iterates it).""" tmpfs = svc.get("tmpfs") - if tmpfs is not None and not isinstance(tmpfs, str | list): + if tmpfs is None: + return + if not isinstance(tmpfs, str | list): msg = f"service {name!r}: tmpfs must be a string or list" raise UnsupportedComposeError(msg) + if isinstance(tmpfs, list): + for entry in tmpfs: + if not isinstance(entry, str): + msg = f"service {name!r}: tmpfs entry must be a string" + raise UnsupportedComposeError(msg) def _validate_environment(name: str, svc: dict[str, Any]) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 35d2162..3379da2 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -314,6 +314,17 @@ def test_tmpfs_non_string_or_list_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="tmpfs must be a string or list"): validate({"services": {"app": {"image": "x", "tmpfs": {"a": "b"}}}}) + def test_tmpfs_list_with_non_string_entry_rejected_at_gate(self) -> None: + # Used to reach emit and crash with TypeError (shell.py to_shell/variable_names + # expects a str) -- the same element-level gap already fixed for env_file. + with pytest.raises(UnsupportedComposeError, match="tmpfs entry must be a string"): + validate({"services": {"app": {"image": "x", "tmpfs": [5]}}}) + + def test_tmpfs_string_and_list_of_strings_still_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "tmpfs": "/tmp"}}}) == [] # noqa: S108 + assert validate({"services": {"app": {"image": "x", "tmpfs": ["/tmp", "/run"]}}}) == [] # noqa: S108 + assert validate({"services": {"app": {"image": "x", "tmpfs": None}}}) == [] + def test_non_string_hostname_raises_at_gate(self) -> None: with pytest.raises(UnsupportedComposeError, match="hostname must be a string"): validate({"services": {"app": {"image": "x", "hostname": 5}}}) From 66f5fc1cf868072d33474a0ba75a6c3d73631ebc Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:37:41 +0300 Subject: [PATCH 11/43] fix: reject a non-mapping 'services' value at the gate validate() itself walked services.items() before checking its shape, so services: "app" or services: ["app"] crashed raw with AttributeError: 'str'/'list' object has no attribute 'items' inside validate(), not even reaching emit. --- compose2pod/parsing.py | 3 +++ tests/test_parsing.py | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 56f91f7..9161814 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -186,6 +186,9 @@ def validate(compose: dict[str, Any]) -> list[str]: if "volumes" in compose: warnings.append("ignoring top-level 'volumes' (podman creates named volumes on first reference)") services = compose.get("services") or {} + if not isinstance(services, dict): + msg = f"'services' must be a mapping, got {type(services).__name__}" + raise UnsupportedComposeError(msg) if not services: msg = "no services defined" raise UnsupportedComposeError(msg) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 3379da2..acff8c2 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -101,6 +101,14 @@ def test_no_services_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="no services"): validate({"services": {}}) + def test_non_mapping_services_rejected_at_gate(self) -> None: + # Used to reach services.items() inside validate() itself and crash raw + # with AttributeError: 'str'/'list' object has no attribute 'items'. + with pytest.raises(UnsupportedComposeError, match="'services' must be a mapping"): + validate({"services": "app"}) + with pytest.raises(UnsupportedComposeError, match="'services' must be a mapping"): + validate({"services": ["app"]}) + def test_unknown_top_level_key_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="foo"): validate({"services": {"app": {"image": "x"}}, "foo": {}}) From c5135fd7e25b740196f587bdaecab7977f870584 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:38:28 +0300 Subject: [PATCH 12/43] fix: reject non-list network aliases at the gate networks..aliases was extended into the hostname/alias set without a shape check, so networks: {default: {aliases: "abc"}} was destructured character-wise and silently emitted --add-host a:127.0.0.1 --add-host b:127.0.0.1 --add-host c:127.0.0.1 -- the same class of bug already fixed for volumes. aliases must now be a list of strings. --- compose2pod/graph.py | 9 ++++++++- tests/test_graph.py | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/compose2pod/graph.py b/compose2pod/graph.py index 7c7fed1..4ad0031 100644 --- a/compose2pod/graph.py +++ b/compose2pod/graph.py @@ -39,7 +39,14 @@ def _host_names(name: str, svc: dict[str, Any]) -> list[str]: if isinstance(networks, dict): for network in networks.values(): if isinstance(network, dict): - result.extend(network.get("aliases") or []) + aliases = network.get("aliases") + if aliases is None: + continue + # A string would be iterated character-wise by extend() below. + if not isinstance(aliases, list) or not all(isinstance(alias, str) for alias in aliases): + msg = f"service {name!r}: aliases must be a list of strings" + raise UnsupportedComposeError(msg) + result.extend(aliases) return result diff --git a/tests/test_graph.py b/tests/test_graph.py index e823241..69e2b30 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -65,6 +65,25 @@ def test_networks_list_short_form_and_null_value_accepted(self) -> None: assert hostnames({"app": {"image": "x", "networks": ["n1"]}}) == ["app"] assert hostnames({"app": {"image": "x", "networks": {"default": None}}}) == ["app"] + def test_string_aliases_rejected_instead_of_iterated_character_wise(self) -> None: + # Used to be destructured one character at a time, emitting + # --add-host a:127.0.0.1 --add-host b:127.0.0.1 --add-host c:127.0.0.1 + # for aliases: "abc" -- the same bug already fixed for volumes. + with pytest.raises(UnsupportedComposeError, match="'app': aliases must be a list of strings"): + hostnames({"app": {"image": "x", "networks": {"default": {"aliases": "abc"}}}}) + + def test_non_string_alias_entry_rejected(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'app': aliases must be a list of strings"): + hostnames({"app": {"image": "x", "networks": {"default": {"aliases": [5]}}}}) + + def test_list_aliases_still_accepted(self) -> None: + services = {"app": {"image": "x", "networks": {"default": {"aliases": ["app-alias"]}}}} + assert hostnames(services) == ["app", "app-alias"] + + def test_network_mapping_with_no_aliases_key_contributes_nothing(self) -> None: + services = {"app": {"image": "x", "networks": {"default": {"driver": "bridge"}}}} + assert hostnames(services) == ["app"] + class TestStartupOrder: def test_chats_order(self, chats_compose: dict) -> None: From 7759039d2edb1d34822754367741d7030f8f7ec2 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:39:01 +0300 Subject: [PATCH 13/43] fix: reject non-scalar healthcheck timeout/start_period/retries Only 'interval' was shape-checked (via interval_seconds); timeout, retries, and start_period were passed through unchecked into str(value), so healthcheck: {retries: {a: 1}} was silently accepted and emitted the literal garbage --health-retries "{'a': 1}". All three now must be a number or string, reusing keys.is_number. --- compose2pod/parsing.py | 7 ++++++- tests/test_parsing.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 9161814..7d46895 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -6,7 +6,7 @@ from compose2pod.exceptions import UnsupportedComposeError from compose2pod.graph import depends_on, hostnames from compose2pod.healthcheck import has_healthcheck, interval_seconds -from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, validate_map +from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, is_number, validate_map from compose2pod.pod import uses_pod_options, validate_pod_options from compose2pod.resources import validate_deploy @@ -14,6 +14,7 @@ SUPPORTED_SERVICE_KEYS = set(SERVICE_KEYS) | STRUCTURAL_KEYS IGNORED_SERVICE_KEYS = {"ports", "restart", "stdin_open", "tty", "stop_signal", "stop_grace_period", "profiles"} SUPPORTED_HEALTHCHECK_KEYS = {"test", "interval", "timeout", "retries", "start_period"} +_HEALTHCHECK_SCALAR_KEYS = ("timeout", "retries", "start_period") SUPPORTED_TOP_LEVEL_KEYS = {"services", "version", "name", "networks", "volumes", "secrets", "configs"} DEPENDS_ON_CONDITIONS = {"service_started", "service_healthy", "service_completed_successfully"} @@ -34,6 +35,10 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) if "interval" in healthcheck: interval_seconds(healthcheck["interval"]) + for key in _HEALTHCHECK_SCALAR_KEYS: + if key in healthcheck and healthcheck[key] is not None and not is_number(healthcheck[key]): + msg = f"service {name!r}: healthcheck {key!r} must be a number or string" + raise UnsupportedComposeError(msg) def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index acff8c2..6a56326 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -318,6 +318,34 @@ def test_unparseable_healthcheck_interval_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck interval"): validate(compose) + def test_healthcheck_scalars_accept_ints_and_strings(self) -> None: + compose = { + "services": { + "app": { + "image": "x", + "healthcheck": {"test": "true", "retries": 15, "timeout": "5s", "start_period": "10s"}, + } + } + } + assert validate(compose) == [] + + def test_healthcheck_retries_mapping_rejected_at_gate(self) -> None: + # Used to be silently accepted and mis-emitted as the literal + # --health-retries "{'a': 1}". + compose = {"services": {"app": {"image": "x", "healthcheck": {"test": "true", "retries": {"a": 1}}}}} + with pytest.raises(UnsupportedComposeError, match=r"healthcheck 'retries' must be a number or string"): + validate(compose) + + def test_healthcheck_timeout_list_rejected_at_gate(self) -> None: + compose = {"services": {"app": {"image": "x", "healthcheck": {"test": "true", "timeout": [5]}}}} + with pytest.raises(UnsupportedComposeError, match=r"healthcheck 'timeout' must be a number or string"): + validate(compose) + + def test_healthcheck_start_period_mapping_rejected_at_gate(self) -> None: + compose = {"services": {"app": {"image": "x", "healthcheck": {"test": "true", "start_period": {"a": 1}}}}} + with pytest.raises(UnsupportedComposeError, match=r"healthcheck 'start_period' must be a number or string"): + validate(compose) + def test_tmpfs_non_string_or_list_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="tmpfs must be a string or list"): validate({"services": {"app": {"image": "x", "tmpfs": {"a": "b"}}}}) From 9713e654808f7a147bcc3f27a61223726d25d8f8 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:39:28 +0300 Subject: [PATCH 14/43] refactor: move pod-name validation into _plan POD_NAME_PATTERN was only checked in emit_script, so the other public entry point, referenced_variables, skipped it entirely. Moves the check into _plan -- the single traversal both entry points project from -- alongside the _validate_options call already there, so both public entry points are guarded by the same check and message. --- compose2pod/emit.py | 6 +++--- tests/test_emit.py | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/compose2pod/emit.py b/compose2pod/emit.py index 6c447fd..41910d1 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -238,6 +238,9 @@ def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: list cannot disagree about what the script expands at run time. """ _validate_options(options) + if not POD_NAME_PATTERN.fullmatch(options.pod): + msg = f"invalid pod name {options.pod!r}" + raise UnsupportedComposeError(msg) services = compose["services"] hosts = hostnames(services) order = startup_order(services, options.target) @@ -284,9 +287,6 @@ def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: def emit_script(compose: dict[str, Any], options: EmitOptions) -> str: """Render the full pod test script for `target` and its dependency closure.""" - if not POD_NAME_PATTERN.fullmatch(options.pod): - msg = f"invalid pod name {options.pod!r}" - raise UnsupportedComposeError(msg) return _plan(compose, options).script diff --git a/tests/test_emit.py b/tests/test_emit.py index d5cc8fe..e3c38b7 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -799,6 +799,13 @@ def test_emit_script_accepts_a_valid_pod_name(self) -> None: script = emit_script(compose=doc, options=self._options("test-pod.1")) assert "podman pod create --name test-pod.1" in script + def test_referenced_variables_also_rejects_invalid_pod_names(self) -> None: + # Used to only be checked by emit_script; referenced_variables projects + # the same _plan traversal and skipped the check entirely. + doc = {"services": {"app": {"image": "x"}}} + with pytest.raises(UnsupportedComposeError, match="invalid pod name"): + referenced_variables(doc, self._options("bad name")) + class TestArtifactValidation: def _options(self, artifacts: list[str]) -> EmitOptions: From a98289d58863210f2e46d4d36f818f3365c881ee Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:42:10 +0300 Subject: [PATCH 15/43] docs: document the closed structural-key gate Updates architecture/supported-subset.md's shape contracts for image, command, tmpfs, network aliases, healthcheck scalars, and top-level services to match the code. Updates the change file's summary and body to describe the realized, broader result -- the gate is closed as a class, not just the original four holes. --- architecture/supported-subset.md | 40 +++++- ...2026-07-13.10-close-structural-key-gate.md | 126 ++++++++++-------- 2 files changed, 103 insertions(+), 63 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index 1823f4a..22b42bd 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -8,8 +8,10 @@ warns (ignored, behavior-neutral inside a single pod) or raises ## Top-level keys -- **Supported:** `services` (required, non-empty), `version`, `name`, - `networks`, `volumes`, `secrets`, `configs`. +- **Supported:** `services` (required, non-empty mapping of service name to + service definition — a non-mapping `services` value, e.g. a bare string or + list, raises inside `validate()` itself), `version`, `name`, `networks`, + `volumes`, `secrets`, `configs`. - **Ignored (warns):** `networks` — all services share the pod's single network namespace, so top-level network definitions have no effect. - **Extension fields:** any key prefixed `x-` is accepted and ignored @@ -40,6 +42,17 @@ warns (ignored, behavior-neutral inside a single pod) or raises The remaining keys documented below are **structural keys**, handled outside the registry because their `emit` needs `project_dir`, spans multiple keys, or occupies the image/command slot. +- **`image`:** a string; `image_for` (`compose2pod/emit.py`) reads it verbatim + when the service has no `build`. A service must set at least one of `image` + or `build`; a service with neither, or a non-string `image` while `build` is + absent, raises at the gate (`_validate_image`, `compose2pod/parsing.py`). A + non-string `image` alongside `build` is accepted, since `image_for` never + reads it in that case — the CI image always wins. +- **`command`:** string or list, exactly like `entrypoint`. List form is argv + tokens; string form runs via `/bin/sh -c`. Any other shape (e.g. a mapping) + raises at the gate (`_validate_command`, `compose2pod/parsing.py`) — a + mapping is rejected outright rather than reaching `podman run` with only + its keys emitted as bare tokens and its values silently discarded. - **`environment`:** list form (`- KEY=value`, `- KEY`) or mapping form (`KEY: value`, `KEY:`). A null mapping value (`KEY:`) means "pass `KEY` through from the host", emitted as a bare `-e KEY` exactly like the list @@ -101,9 +114,11 @@ warns (ignored, behavior-neutral inside a single pod) or raises `podman run --tmpfs ` — Compose's short syntax maps directly onto podman's own `--tmpfs CONTAINER-DIR[:OPTIONS]` flag, so no translation is needed. The key itself must be a string or list — a non-string/non-list - value (e.g. a mapping) raises; no format validation beyond that, so a - malformed option string inside an accepted string/list surfaces as a podman - error at run time. + value (e.g. a mapping) raises; each list element must itself be a string, a + non-string element (e.g. `tmpfs: [5]`) raises the same way + (`_validate_tmpfs`, `compose2pod/parsing.py`). No format validation beyond + shape, so a malformed option string inside an accepted string/list surfaces + as a podman error at run time. - **`hostname` and `container_name`:** both are made resolvable to `127.0.0.1` like a network alias (added to the shared `--add-host` set), so other services can reach the service by either name. The pod shares the UTS @@ -120,8 +135,12 @@ warns (ignored, behavior-neutral inside a single pod) or raises contribute). The key must be a list or mapping — anything else raises. A long-form *value* that isn't itself a mapping (e.g. `networks: {default: true}`) is lenient, not rejected: it simply contributes no aliases, since - only a mapping value can carry an `aliases` list (`_host_names`, - `compose2pod/graph.py`). + only a mapping value can carry an `aliases` list. When present, `aliases` + itself must be a list of strings — a non-list value (e.g. a bare string) + would otherwise be destructured character-wise into the resolvable-name + set, and a non-string element crashes downstream the same way a malformed + `volumes`/`tmpfs` element does; both raise at the gate instead + (`_host_names`, `compose2pod/graph.py`). - **Ignored (warns):** `ports`, `restart`, `stdin_open`, `tty`, `stop_signal`, `stop_grace_period`, `profiles` — meaningless or irrelevant inside a single shared-namespace pod. `stop_signal`/`stop_grace_period` are inert because the @@ -305,6 +324,13 @@ honors both, refusing loudly on overlap rather than picking a precedence. Compound durations (`"1h30m"`) and hour suffixes (`"1h"`) are not parsed — each is rejected with an `UnsupportedComposeError` rather than silently truncated or misinterpreted. +- **`timeout`, `retries`, `start_period`:** each must be a number (int or + float) or string when present — the same shape `keys.is_number` enforces + for the legacy resource-limit keys. Each is passed straight through + `str(value)` into its `--health-*` flag with no further parsing, so a + mapping or list (e.g. `retries: {a: 1}`) raises at the gate + (`_validate_service_healthcheck`, `compose2pod/parsing.py`) instead of + emitting a literal Python `repr()` as the flag value. - **Extension fields:** any `x-`-prefixed healthcheck key is accepted and ignored silently. - Everything else raises. diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index 116f3bc..94770b0 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,33 +1,40 @@ --- -summary: validate() now rejects malformed 'environment', 'env_file', and 'volumes', and emit rejects a malformed --artifact, so a bad document fails with UnsupportedComposeError instead of a raw AttributeError/TypeError/ValueError. +summary: validate() rejects every malformed structural-key shape emit can crash or misrender on (image, command, environment, env_file, volumes, tmpfs, services, network aliases, healthcheck scalars), emit rejects a malformed --artifact and now also guards referenced_variables on pod name, so a bad document always fails with UnsupportedComposeError instead of a raw crash or silent mis-emission. --- # Design: Close the structural-key gate ## Summary -`environment`, `env_file`, and `volumes` are structural keys: they carry no -`KeySpec`, so the `SERVICE_KEYS` validate loop never sees them, and unlike -`tmpfs`/`entrypoint`/`healthcheck` nobody wrote them a validator by hand. -`emit` walks their raw shape and crashes. This adds the three missing shape -validators in `parsing.py` and one options check in `emit._plan`, closing the -gate. No new abstraction. +Structural keys — the ones with no `KeySpec` in the `SERVICE_KEYS` registry — +are only as safe as the hand-written validator each one gets, and several had +none. `emit` (and, for `services`, `validate()` itself) walked their raw shape +and either crashed with a raw Python exception or, worse, silently accepted a +malformed value and mis-emitted it. This closes the gate as a class: every +structural key `emit` reads now has a shape validator in `parsing.py` (or, for +the two cases the shape lives outside `parsing.py`'s reach, in the owning +module — `graph.py` for network aliases, `emit.py` for the pod name and +`--artifact`). No new abstraction; each validator follows the existing +`_validate_tmpfs`/`_validate_entrypoint` hand-written pattern. ## Motivation -From `audits/2026-07-13-docs-and-gate-audit.md`, all four reproduced against -the code: - -| Input | Today | -|-------|-------| -| `environment: "FOO=bar"` | `AttributeError: 'str' object has no attribute 'items'` | -| `env_file: 5` | `TypeError: 'int' object is not iterable` | -| `volumes: "/data:/data"` | destructured *character-wise*; reports `anonymous volume 'd' must be an absolute path` | -| `--artifact nocolon` | `ValueError: not enough values to unpack` out of `emit.py:209` | - -`volumes` also silently corrupts: `volumes: "/"` is **accepted** and emits -`-v "/"`, because the single character `/` happens to pass the -anonymous-volume check. +An initial pass (`_validate_service_volumes`, `_validate_environment`, +`_validate_env_file`, `_validate_options`) closed four holes. A follow-up +review found the same defect class still live in more places. All nine +reproduced against the code: + +| Input | Before | Kind | +|-------|--------|------| +| service with neither `image` nor `build` | `KeyError: 'image'` (`emit.py`, `image_for`) | crash | +| `image: 5` | `TypeError` (`shell.py`) | crash | +| `command: 5` | `TypeError: 'int' object is not iterable` (`emit.py`, `command_tokens`) | crash | +| `command: {run: tests}` | accepted; only the mapping's key reaches `podman run`, the value is dropped | silent corruption | +| `tmpfs: [5]` | `TypeError` (`shell.py`) — `_validate_tmpfs` only checked the outer shape | crash | +| `services: "app"` / `services: [...]` | `AttributeError: 'str'/'list' object has no attribute 'items'` inside `validate()` itself | crash | +| `networks: {default: {aliases: "abc"}}` | destructured character-wise; emits `--add-host a:127.0.0.1 --add-host b:127.0.0.1 --add-host c:127.0.0.1` | silent corruption | +| `healthcheck: {retries: {a: 1}}` | accepted; emits the literal `--health-retries "{'a': 1}"` | silent corruption | +| invalid pod name via `referenced_variables` | no check at all — `POD_NAME_PATTERN` was only enforced in `emit_script` | gate bypass | This is not a new requirement. `decisions/2026-07-10-reject-parse-dont-validate.md` rejects a typed model *on the grounds that this invariant already holds*: @@ -36,57 +43,64 @@ rejects a typed model *on the grounds that this invariant already holds*: > robust: a direct `emit_script(dict)` call on a *malformed* document now fails > with `UnsupportedComposeError`, not a raw crash. -That premise is false. This change makes it true again — it does not reopen the -decision. +That premise was still false after the first pass. This change makes it true +— it does not reopen the decision. ## Design -Three validators in `parsing.py`, each mirroring the existing `_validate_tmpfs`, -wired into `_validate_service`: - -- **`environment`** — reuses the already-public `keys.validate_map`, since - list-or-mapping is exactly its contract. No new primitive. -- **`env_file`** — string or list. -- **`volumes`** — must be a list. Folded into the existing - `_validate_service_volumes` as an up-front shape check, ahead of the per-entry - loop that today iterates a string's characters. - -Each skips a `None` value: an empty `environment:` / `env_file:` / `volumes:` -key is valid Compose and today's emit already tolerates it. - -For `--artifact`, one `_validate_options(options)` called from the top of -`emit._plan`. `_plan` is the single traversal both `emit_script` and -`referenced_variables` project from, so one call site guards both public entry -points. `cli.main` already catches `UnsupportedComposeError` and returns exit 2, -so raising is sufficient — no argparse-level check is added. +One validator per finding, wired into `_validate_service` (`parsing.py`) +unless noted: + +- **`image`** — a service needs `image` or `build`; when `build` is absent, + `image` must also be a string (the one path `image_for` actually reads). +- **`command`** — string or list, mirroring `_validate_entrypoint` exactly; + this single check closes both the crash (`command: 5`) and the silent + mapping-drop (`command: {...}`) in one shape rule. +- **`tmpfs`** — extends the existing outer-shape check with the same + element-level string check already applied to `env_file`. +- **`services`** — added directly in `validate()`, ahead of the `.items()` + walk it guards, since this is document-level shape, not a per-service key. +- **network aliases** — added in `graph.py`'s `_host_names`, which already + owns `hostname`/`container_name`/`networks` shape checks: `aliases` must be + a list of strings, the same character-wise-destructuring bug already fixed + for `volumes`. +- **healthcheck scalars** — `timeout`/`retries`/`start_period` must be a + number or string (reusing `keys.is_number`), closing the one gap left after + `interval` was already validated. +- **pod name** — moved from `emit_script` into `_plan`, the single traversal + both `emit_script` and `referenced_variables` project from, so one call + site now guards both public entry points instead of only one. + +Each `None`-valued key still short-circuits to "accepted": an absent or null +structural key is valid Compose and emit already tolerates it. ## Non-goals - **No structural-key registry.** `decisions/2026-07-12-reject-structural-key-registry.md` - rejects it, and nothing here needs one: three hand-written validators in the - module that already owns the gate is the pattern the codebase established. + rejects it, and nothing here needs one: hand-written validators in the + module that already owns each concern is the pattern the codebase + established, now carried consistently across every structural key. - **No `--artifact` check in argparse.** `cli.main`'s existing `UnsupportedComposeError` handler already yields a clean message and exit 2; a second check would duplicate it. -- **No content validation** of volume/env_file *values* beyond shape. A - malformed path still surfaces as a podman error at run time, unchanged. +- **No content validation** beyond shape (e.g. a malformed volume path, or a + malformed tmpfs option string). Those still surface as a podman error at + run time, unchanged. ## Testing -`just test-ci` at 100%: -- `tests/test_parsing.py`: each of `environment` / `env_file` / `volumes` in a - wrong shape raises `UnsupportedComposeError` at `validate()`; each with a - `None` value is still accepted; the character-wise `volumes: "/"` case is now - rejected rather than silently emitting `-v "/"`. -- `tests/test_emit.py`: `emit_script` and `referenced_variables` both raise - `UnsupportedComposeError` on an `--artifact` with no colon; a valid `SRC:DST` - still emits the `podman cp` line. +`just test-ci` at 100%: `tests/test_parsing.py`, `tests/test_graph.py`, and +`tests/test_emit.py` each cover, per finding, the crash/silent-acceptance +case (raises `UnsupportedComposeError`) and the accepted forms alongside it +(`image`+`build` combinations, `command` as string/list/absent, `tmpfs` as +string/list/absent, `aliases` as a list of strings, healthcheck scalars as +both ints and strings). `tests/conftest.py`'s `chats_compose` fixture and +every `tests/integration/` scenario continue to pass unchanged. `just lint-ci` and `just check-planning` clean. ## Risk -- **A document that used to be accepted is now rejected** (low x low): only for - shapes that already crashed or silently corrupted. `volumes: "/"` is the one - input that changes from "accepted" to "rejected"; it emitted `-v "/"`, which - no one can have depended on. +- **A document that used to be accepted is now rejected** (low x low): only + for shapes that already crashed or silently corrupted — nothing that ever + produced a correct script changes behavior. From a71ce421fabe16e1762e2777fe596522d95fcb67 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Mon, 13 Jul 2026 23:47:38 +0300 Subject: [PATCH 16/43] fix: accept null command at the gate _validate_command rejected an explicit null command (command: null in YAML), but emit.command_tokens accepts it to mean "no command." This was over-rejection compared to the other validators (_validate_environment, _validate_tmpfs, etc.) which skip None correctly. Now _validate_command skips None but still rejects genuinely wrong shapes (int, mapping). --- compose2pod/parsing.py | 5 ++++- tests/test_parsing.py | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 7d46895..ac27a9f 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -87,7 +87,10 @@ def _validate_entrypoint(name: str, svc: dict[str, Any]) -> None: def _validate_command(name: str, svc: dict[str, Any]) -> None: """Check the structural command key's form (it is not a registry key).""" - if "command" in svc and not isinstance(svc["command"], str | list): + command = svc.get("command") + if command is None: + return + if not isinstance(command, str | list): msg = f"service {name!r}: 'command' must be a string or list" raise UnsupportedComposeError(msg) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 6a56326..4e66b38 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -477,3 +477,6 @@ def test_mapping_command_rejected_at_gate(self) -> None: # ('run') reached podman run, the value ('tests') was dropped. with pytest.raises(UnsupportedComposeError, match=r"'command' must be a string or list"): validate({"services": {"app": {"image": "x", "command": {"run": "tests"}}}}) + + def test_null_command_is_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "command": None}}}) == [] From c9fff97c8c59de87b07c71b8e13b78f7f2842488 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:00:36 +0300 Subject: [PATCH 17/43] fix: guard Expand.value against non-str at construction Expand is the chokepoint every emitted token flows through on its way to shell.to_shell/variable_names, both of which crash raw (TypeError) on a non-str value. Any per-key validator gap that let a non-str leak into emit used to surface as an unhandled crash instead of a clean UnsupportedComposeError, no matter which key was responsible. Guard it once at construction instead of trusting every call site. --- compose2pod/keys.py | 20 +++++++++++++++++++- tests/test_keys.py | 16 ++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 167624e..0bef6da 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -17,10 +17,28 @@ @dataclasses.dataclass(frozen=True, slots=True, kw_only=True) class Expand: - """A token whose Compose variable references expand at script-run time.""" + """A token whose Compose variable references expand at script-run time. + + Every emitted `Expand` eventually reaches `shell.to_shell`/`variable_names`, + both of which assume `value` is a `str` (they run a compiled regex over + it) and crash raw (a `TypeError`, not `UnsupportedComposeError`) otherwise. + Rather than trust every call site across keys.py/emit.py/pod.py/ + resources.py to have cast or validated first, this is the one chokepoint: + a non-str value is rejected right here, so any per-key validator gap + becomes a clean error instead of a raw crash downstream, no matter which + key leaked it. This is defense-in-depth, not a substitute for validating + shape at the gate -- it only turns an otherwise-unhandled crash into a + clean one; it can't catch a non-str value that a caller has already + coerced to str() (see the silent str()/repr() corruption class instead). + """ value: str + def __post_init__(self) -> None: + if not isinstance(self.value, str): + msg = f"internal: Expand.value must be a string, got {type(self.value).__name__}: {self.value!r}" + raise UnsupportedComposeError(msg) + Token = str | Expand diff --git a/tests/test_keys.py b/tests/test_keys.py index c399b5c..f54cfea 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -4,6 +4,7 @@ from compose2pod.keys import ( SERVICE_KEYS, STRUCTURAL_KEYS, + Expand, _concat_list, _merge_map, _validate_list, @@ -17,6 +18,21 @@ def test_registry_and_structural_keys_are_disjoint() -> None: assert not (set(SERVICE_KEYS) & STRUCTURAL_KEYS) +class TestExpand: + """Expand is the chokepoint every emitted token flows through on its way to to_shell/variable_names.""" + + def test_string_value_is_accepted(self) -> None: + assert Expand(value="ok").value == "ok" + + def test_non_string_value_raises_instead_of_reaching_shell_py_raw(self) -> None: + # Any per-key validator gap that lets a non-str leak into emit used to + # crash raw inside shell.py's re.finditer (a TypeError, not a clean + # UnsupportedComposeError). Guarding construction closes that whole + # class in one place, regardless of which key leaked it. + with pytest.raises(UnsupportedComposeError, match="must be a string"): + Expand(value=123) # ty: ignore[invalid-argument-type] + + def test_supported_service_keys_snapshot() -> None: assert { "image", From 735bb8a40c9f14b0b78dad7e201b9833846bd465 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:01:37 +0300 Subject: [PATCH 18/43] fix: gate healthcheck.test's shape at validate() time healthcheck.test was validated for mapping shape and key names but never for the test value's own shape, so a malformed test (e.g. a nested list as CMD-SHELL's argument) reached emit and crashed raw instead of failing at the gate. Tighten health_cmd() itself to check CMD-SHELL/CMD argument types (dropping the ty: ignore that was hiding the gap) and call it from _validate_service_healthcheck so the same check runs at validate() time, not only when emit.py calls it later. --- compose2pod/healthcheck.py | 6 +++--- compose2pod/parsing.py | 7 ++++++- tests/test_healthcheck.py | 10 ++++++++++ tests/test_parsing.py | 18 ++++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/compose2pod/healthcheck.py b/compose2pod/healthcheck.py index 5456e86..c37b769 100644 --- a/compose2pod/healthcheck.py +++ b/compose2pod/healthcheck.py @@ -27,12 +27,12 @@ def health_cmd(test: object) -> str | None: raise UnsupportedComposeError(msg) kind = test[0] if kind == "CMD-SHELL": - if len(test) < _CMD_MIN_LENGTH: + if len(test) < _CMD_MIN_LENGTH or not isinstance(test[1], str): msg = f"unsupported healthcheck test: {test!r}" raise UnsupportedComposeError(msg) - return test[1] # ty: ignore + return test[1] if kind == "CMD": - if len(test) < _CMD_MIN_LENGTH: + if len(test) < _CMD_MIN_LENGTH or not all(isinstance(item, str) for item in test[1:]): msg = f"unsupported healthcheck test: {test!r}" raise UnsupportedComposeError(msg) return json.dumps(test[1:]) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index ac27a9f..3bc8ea6 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -5,7 +5,7 @@ from compose2pod import stores from compose2pod.exceptions import UnsupportedComposeError from compose2pod.graph import depends_on, hostnames -from compose2pod.healthcheck import has_healthcheck, interval_seconds +from compose2pod.healthcheck import has_healthcheck, health_cmd, interval_seconds from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, is_number, validate_map from compose2pod.pod import uses_pod_options, validate_pod_options from compose2pod.resources import validate_deploy @@ -35,6 +35,11 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) if "interval" in healthcheck: interval_seconds(healthcheck["interval"]) + if "test" in healthcheck: + # health_cmd's return value is unused here -- it raises on any + # unsupported shape, which is all this gate needs (emit.py calls it + # again for the actual --health-cmd value at emit time). + health_cmd(healthcheck["test"]) for key in _HEALTHCHECK_SCALAR_KEYS: if key in healthcheck and healthcheck[key] is not None and not is_number(healthcheck[key]): msg = f"service {name!r}: healthcheck {key!r} must be a number or string" diff --git a/tests/test_healthcheck.py b/tests/test_healthcheck.py index e3e9f6a..ef88c38 100644 --- a/tests/test_healthcheck.py +++ b/tests/test_healthcheck.py @@ -42,6 +42,16 @@ def test_cmd_without_command_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck test"): health_cmd(["CMD"]) + def test_cmd_shell_non_string_argument_raises(self) -> None: + # A nested list where a string is expected: used to reach emit and + # crash raw when the non-string was wrapped in Expand. + with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck test"): + health_cmd(["CMD-SHELL", ["curl", "-f"]]) + + def test_cmd_non_string_argument_raises(self) -> None: + with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck test"): + health_cmd(["CMD", "keydb-cli", 123]) + class TestIntervalSeconds: def test_seconds_suffix(self) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 4e66b38..80e9ac1 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -346,6 +346,24 @@ def test_healthcheck_start_period_mapping_rejected_at_gate(self) -> None: with pytest.raises(UnsupportedComposeError, match=r"healthcheck 'start_period' must be a number or string"): validate(compose) + def test_healthcheck_test_cmd_shell_nested_list_rejected_at_gate(self) -> None: + # Used to reach emit and crash raw: health_cmd() returned test[1] + # (the nested list) unchecked, and it hit shell.py's re.finditer. + compose = { + "services": {"app": {"image": "x", "healthcheck": {"test": ["CMD-SHELL", ["curl", "-f"]]}}}, + } + with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck test"): + validate(compose) + + def test_healthcheck_test_cmd_non_string_argument_rejected_at_gate(self) -> None: + compose = {"services": {"app": {"image": "x", "healthcheck": {"test": ["CMD", 123]}}}} + with pytest.raises(UnsupportedComposeError, match="unsupported healthcheck test"): + validate(compose) + + def test_healthcheck_test_forms_still_accepted(self) -> None: + for test in ("true", "NONE", ["NONE"], ["CMD", "a", "b"], ["CMD-SHELL", "some string"]): + assert validate({"services": {"app": {"image": "x", "healthcheck": {"test": test}}}}) == [] + def test_tmpfs_non_string_or_list_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="tmpfs must be a string or list"): validate({"services": {"app": {"image": "x", "tmpfs": {"a": "b"}}}}) From 09ac975970b86a7d883389c05c08f385f31ee051 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:03:26 +0300 Subject: [PATCH 19/43] fix: reject non-scalar list/map elements across structural keys _validate_list and validate_map only checked the outer list/dict shape, so a non-string list element or non-scalar map value reached emit and got str()'d/repr()'d straight into the script -- exit 0, garbage output, no error. The commonest trigger is the classic Compose YAML slip of mixing list and map form (`- KEY: value` instead of `- KEY=value`). Tighten both shared validators to check elements/values, which covers labels, annotations, cap_add, cap_drop, group_add, security_opt, devices, extra_hosts, and environment in one edit (all route through one of the two). command/entrypoint don't go through the registry, so add the matching list-element check locally in parsing.py. List elements must be strings; map values must be a string, number, or null (null keeps its existing per-key meaning -- host-passthrough for environment, empty label for labels/annotations). --- compose2pod/keys.py | 28 +++++++++++++++++++++++++--- compose2pod/parsing.py | 22 +++++++++++++++++++++- tests/test_keys.py | 41 +++++++++++++++++++++++++++++++++++++++++ tests/test_parsing.py | 41 +++++++++++++++++++++++++++++++++++++++++ tests/test_pod.py | 9 +++++++++ 5 files changed, 137 insertions(+), 4 deletions(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 0bef6da..7e20802 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -85,16 +85,38 @@ def _validate_number(name: str, key: str, value: Any) -> None: # noqa: ANN401 - raise UnsupportedComposeError(msg) +def _validate_list_elements(name: str, key: str, value: list[Any]) -> None: + """Check every list element is a string, so emit can't str() a non-string into the script.""" + for item in value: + if not isinstance(item, str): + msg = f"service {name!r}: '{key}' entries must be strings" + raise UnsupportedComposeError(msg) + + +def _validate_map_values(name: str, key: str, value: dict[str, Any]) -> None: + """Check every map value is a scalar or null, so emit can't repr() a dict/list into the script.""" + for val in value.values(): + if val is not None and not is_number(val): + msg = f"service {name!r}: '{key}' values must be a string, number, or null" + raise UnsupportedComposeError(msg) + + def _validate_list(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON if not isinstance(value, list): msg = f"service {name!r}: '{key}' must be a list" raise UnsupportedComposeError(msg) + _validate_list_elements(name, key, value) def validate_map(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON - if not isinstance(value, list | dict): - msg = f"service {name!r}: '{key}' must be a list or mapping" - raise UnsupportedComposeError(msg) + if isinstance(value, list): + _validate_list_elements(name, key, value) + return + if isinstance(value, dict): + _validate_map_values(name, key, value) + return + msg = f"service {name!r}: '{key}' must be a list or mapping" + raise UnsupportedComposeError(msg) def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 3bc8ea6..e420d68 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -83,11 +83,29 @@ def _validate_image(name: str, svc: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) +def _validate_argv_list(name: str, key: str, value: list[Any]) -> None: + """Check every command/entrypoint list element is a string. + + Without this, a list-of-mapping form (e.g. `command: [{run: tests}]` -- + the same list/map YAML slip as `environment`) was silently accepted and + str()'d into a single mangled podman-run argv token. + """ + for token in value: + if not isinstance(token, str): + msg = f"service {name!r}: '{key}' entries must be strings" + raise UnsupportedComposeError(msg) + + def _validate_entrypoint(name: str, svc: dict[str, Any]) -> None: """Check the structural entrypoint key's form (it is not a registry key).""" - if "entrypoint" in svc and not isinstance(svc["entrypoint"], str | list): + if "entrypoint" not in svc: + return + entrypoint = svc["entrypoint"] + if not isinstance(entrypoint, str | list): msg = f"service {name!r}: 'entrypoint' must be a string or list" raise UnsupportedComposeError(msg) + if isinstance(entrypoint, list): + _validate_argv_list(name, "entrypoint", entrypoint) def _validate_command(name: str, svc: dict[str, Any]) -> None: @@ -98,6 +116,8 @@ def _validate_command(name: str, svc: dict[str, Any]) -> None: if not isinstance(command, str | list): msg = f"service {name!r}: 'command' must be a string or list" raise UnsupportedComposeError(msg) + if isinstance(command, list): + _validate_argv_list(name, "command", command) def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: diff --git a/tests/test_keys.py b/tests/test_keys.py index f54cfea..b82b8a3 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -100,6 +100,47 @@ def test_merge_present_iff_list_or_map_shaped(key: str) -> None: assert (spec.merge is not None) == is_list_or_map_shaped +class TestElementLevelShapeChecks: + """_validate_list/validate_map used to only check the outer list/dict shape. + + A non-string list element or non-scalar map value slipped through and got + str()'d/repr()'d straight into the emitted script -- exit 0, garbage + output, no error anywhere. The commonest trigger is the classic YAML slip + of mixing list and map form (`- KEY: value` instead of `- KEY=value`). + """ + + def test_validate_list_rejects_non_string_element(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'cap_add' entries must be strings"): + _validate_list("web", "cap_add", [{"NET_ADMIN": True}]) + + def test_validate_list_accepts_string_elements(self) -> None: + _validate_list("web", "cap_add", ["NET_ADMIN", "SYS_TIME"]) + + def test_validate_map_rejects_non_string_list_element(self) -> None: + # The realistic trigger: `environment: [{KEY: value}]` instead of `- KEY=value`. + with pytest.raises(UnsupportedComposeError, match="'environment' entries must be strings"): + validate_map("web", "environment", [{"POSTGRES_PASSWORD": "password"}]) + + def test_validate_map_rejects_non_scalar_map_value(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'labels' values must be"): + validate_map("web", "labels", {"team": {"nested": "dict"}}) + + def test_validate_map_rejects_list_valued_map_entry(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'labels' values must be"): + validate_map("web", "labels", {"team": ["a", "b"]}) + + def test_validate_map_accepts_null_value(self) -> None: + # environment: {KEY: null} -> host-passthrough; labels: {KEY: null} -> empty label. + validate_map("web", "environment", {"KEY": None}) + validate_map("web", "labels", {"KEY": None}) + + def test_validate_map_accepts_numeric_value(self) -> None: + validate_map("web", "labels", {"version": 2}) + + def test_validate_map_accepts_string_list_elements(self) -> None: + validate_map("web", "labels", ["team=core", "BARE"]) + + class TestMergeCallables: """Direct tests of the merge policy functions, independent of any caller. diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 80e9ac1..b075b45 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -444,6 +444,33 @@ def test_string_environment_rejected_at_gate(self) -> None: def test_null_environment_is_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "environment": None}}}) == [] + def test_environment_list_of_mapping_rejected_at_gate(self) -> None: + # The commonest Compose YAML slip: `- KEY: value` (list + mapping) + # instead of `- KEY=value`. Used to be silently accepted and emit the + # literal `-e "{'POSTGRES_PASSWORD': 'password'}"` -- exit 0, garbage. + compose = {"services": {"app": {"image": "x", "environment": [{"POSTGRES_PASSWORD": "password"}]}}} + with pytest.raises(UnsupportedComposeError, match="'environment' entries must be strings"): + validate(compose) + + def test_environment_list_and_map_forms_still_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "environment": ["KEY=value", "BARE"]}}}) == [] + assert validate({"services": {"app": {"image": "x", "environment": {"KEY": "value", "BARE": None}}}}) == [] + + def test_labels_annotations_list_of_mapping_rejected_at_gate(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'labels' entries must be strings"): + validate({"services": {"app": {"image": "x", "labels": [{"team": "core"}]}}}) + + def test_labels_map_non_scalar_value_rejected_at_gate(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'labels' values must be"): + validate({"services": {"app": {"image": "x", "labels": {"team": {"nested": "dict"}}}}}) + + def test_labels_null_and_numeric_map_values_still_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "labels": {"empty": None, "version": 2}}}}) == [] + + def test_cap_add_list_of_mapping_rejected_at_gate(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'cap_add' entries must be strings"): + validate({"services": {"app": {"image": "x", "cap_add": [{"NET_ADMIN": True}]}}}) + def test_non_string_non_list_env_file_rejected_at_gate(self) -> None: # Used to reach emit and crash with TypeError: 'int' object is not iterable. with pytest.raises(UnsupportedComposeError, match=r"'env_file' must be a string or list"): @@ -498,3 +525,17 @@ def test_mapping_command_rejected_at_gate(self) -> None: def test_null_command_is_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "command": None}}}) == [] + + def test_command_list_with_mapping_entry_rejected_at_gate(self) -> None: + # Used to be silently accepted: str({'run': 'tests'}) reached podman + # run as a single mangled argv token, e.g. "{'run': 'tests'}". + with pytest.raises(UnsupportedComposeError, match="'command' entries must be strings"): + validate({"services": {"app": {"image": "x", "command": [{"run": "tests"}]}}}) + + def test_entrypoint_list_with_non_string_entry_rejected_at_gate(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'entrypoint' entries must be strings"): + validate({"services": {"app": {"image": "x", "entrypoint": [{"run": "tests"}]}}}) + + def test_command_and_entrypoint_list_of_strings_still_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "command": ["run", "me"]}}}) == [] + assert validate({"services": {"app": {"image": "x", "entrypoint": ["run", "me"]}}}) == [] diff --git a/tests/test_pod.py b/tests/test_pod.py index dea7c21..4d01d93 100644 --- a/tests/test_pod.py +++ b/tests/test_pod.py @@ -42,6 +42,15 @@ def test_sysctls_wrong_type_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="'sysctls' must be a mapping or a list"): validate_pod_options("app", {"image": "x", "sysctls": "net.x=1"}) + def test_extra_hosts_list_of_mapping_rejected(self) -> None: + # Used to be silently accepted and str()'d straight into --add-host. + with pytest.raises(UnsupportedComposeError, match="'extra_hosts' entries must be strings"): + validate_pod_options("app", {"image": "x", "extra_hosts": [{"db": "10.0.0.5"}]}) + + def test_extra_hosts_map_non_scalar_value_rejected(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'extra_hosts' values must be"): + validate_pod_options("app", {"image": "x", "extra_hosts": {"db": ["10.0.0.5"]}}) + class TestUsesPodOptions: def test_true_when_declared(self) -> None: From 935509468d9feff79ae17009b27783162494e268 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:04:27 +0300 Subject: [PATCH 20/43] fix: reject non-string depends_on list entries at the gate depends_on's list (short) form passed entries straight into dict.fromkeys, which crashed with a raw TypeError (unhashable type) on the same list/map YAML slip (`- db: {condition: ...}` instead of a bare service name) already fixed for environment/command/labels. Unlike those, this one crashed from inside validate() itself, via graph.depends_on's normalization. Reject it with the same local style depends_on already uses for its other malformed shapes. --- compose2pod/graph.py | 4 ++++ tests/test_graph.py | 14 ++++++++++++++ tests/test_parsing.py | 13 +++++++++++++ 3 files changed, 31 insertions(+) diff --git a/compose2pod/graph.py b/compose2pod/graph.py index 4ad0031..ad4e45a 100644 --- a/compose2pod/graph.py +++ b/compose2pod/graph.py @@ -9,6 +9,10 @@ def depends_on(svc: dict[str, Any]) -> dict[str, str]: """Normalize dependencies of a service to a name -> condition mapping.""" deps = svc.get("depends_on") or {} if isinstance(deps, list): + for dep in deps: + if not isinstance(dep, str): + msg = f"depends_on entry {dep!r} must be a string" + raise UnsupportedComposeError(msg) return cast(dict[str, str], dict.fromkeys(deps, "service_started")) if not isinstance(deps, dict): msg = "'depends_on' must be a list or mapping" diff --git a/tests/test_graph.py b/tests/test_graph.py index 69e2b30..d9a622d 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -22,6 +22,20 @@ def test_mapping_entry_not_a_mapping_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="depends_on entry 'db' must be a mapping"): depends_on({"depends_on": {"db": "service_healthy"}}) + def test_list_entry_not_a_string_raises(self) -> None: + # Same YAML slip as `environment`/`command`: `- db: {condition: ...}` + # is a hyphen + mapping, not a bare service name. Used to crash raw + # (TypeError: unhashable type: 'dict') from dict.fromkeys inside + # validate() itself, instead of a clean UnsupportedComposeError. + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry .* must be a string"): + depends_on({"depends_on": [{"db": {"condition": "service_healthy"}}]}) + + def test_list_form_string_entries_still_accepted(self) -> None: + assert depends_on({"depends_on": ["db", "keydb"]}) == { + "db": "service_started", + "keydb": "service_started", + } + class TestHostnames: def test_collects_service_names_and_aliases(self, chats_compose: dict) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index b075b45..7041f02 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -391,6 +391,19 @@ def test_malformed_depends_on_raises_at_gate(self) -> None: with pytest.raises(UnsupportedComposeError, match="'depends_on' must be a list or mapping"): validate({"services": {"app": {"image": "x", "depends_on": "db"}}}) + def test_depends_on_list_with_mapping_entry_raises_at_gate(self) -> None: + # Same list/map YAML slip as `environment`/`command`. Used to crash + # raw (TypeError: unhashable type: 'dict') from inside validate() + # itself, via graph.depends_on's dict.fromkeys. + compose = { + "services": { + "app": {"image": "x", "depends_on": [{"db": {"condition": "service_healthy"}}]}, + "db": {"image": "x"}, + }, + } + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry .* must be a string"): + validate(compose) + def test_valid_networks_forms_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "networks": ["n1"]}}}) == [] assert validate({"services": {"app": {"image": "x", "networks": {"default": None}}}}) == [] From f3a773a238fbe51f548785f6a66a6e945804a668 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:05:30 +0300 Subject: [PATCH 21/43] refactor: share tmpfs/env_file's string-or-string-list validator _validate_tmpfs and _validate_env_file were the same "string or list of strings" rule written twice with slightly different messages. Extract _validate_string_or_string_list and quote tmpfs's key name in its message to match every other key's error format. --- compose2pod/parsing.py | 34 ++++++++++++++-------------------- tests/test_parsing.py | 4 ++-- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index e420d68..5599e97 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -120,21 +120,25 @@ def _validate_command(name: str, svc: dict[str, Any]) -> None: _validate_argv_list(name, "command", command) -def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: - """Check tmpfs is a string or list of strings (emit iterates it).""" - tmpfs = svc.get("tmpfs") - if tmpfs is None: +def _validate_string_or_string_list(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Check a key is a string or list of strings (emit iterates it), shared by tmpfs and env_file.""" + if value is None: return - if not isinstance(tmpfs, str | list): - msg = f"service {name!r}: tmpfs must be a string or list" + if not isinstance(value, str | list): + msg = f"service {name!r}: '{key}' must be a string or list" raise UnsupportedComposeError(msg) - if isinstance(tmpfs, list): - for entry in tmpfs: + if isinstance(value, list): + for entry in value: if not isinstance(entry, str): - msg = f"service {name!r}: tmpfs entry must be a string" + msg = f"service {name!r}: '{key}' entry must be a string" raise UnsupportedComposeError(msg) +def _validate_tmpfs(name: str, svc: dict[str, Any]) -> None: + """Check tmpfs is a string or list of strings (emit iterates it).""" + _validate_string_or_string_list(name, "tmpfs", svc.get("tmpfs")) + + def _validate_environment(name: str, svc: dict[str, Any]) -> None: """Check environment is a list or mapping (a bare string would be walked as .items()).""" if svc.get("environment") is not None: @@ -143,17 +147,7 @@ def _validate_environment(name: str, svc: dict[str, Any]) -> None: def _validate_env_file(name: str, svc: dict[str, Any]) -> None: """Check env_file is a string or list of strings (emit iterates it).""" - env_file = svc.get("env_file") - if env_file is None: - return - if not isinstance(env_file, str | list): - msg = f"service {name!r}: 'env_file' must be a string or list" - raise UnsupportedComposeError(msg) - if isinstance(env_file, list): - for entry in env_file: - if not isinstance(entry, str): - msg = f"service {name!r}: 'env_file' entry must be a string" - raise UnsupportedComposeError(msg) + _validate_string_or_string_list(name, "env_file", svc.get("env_file")) def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compose values are untyped diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 7041f02..76b284c 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -365,13 +365,13 @@ def test_healthcheck_test_forms_still_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "healthcheck": {"test": test}}}}) == [] def test_tmpfs_non_string_or_list_raises(self) -> None: - with pytest.raises(UnsupportedComposeError, match="tmpfs must be a string or list"): + with pytest.raises(UnsupportedComposeError, match="'tmpfs' must be a string or list"): validate({"services": {"app": {"image": "x", "tmpfs": {"a": "b"}}}}) def test_tmpfs_list_with_non_string_entry_rejected_at_gate(self) -> None: # Used to reach emit and crash with TypeError (shell.py to_shell/variable_names # expects a str) -- the same element-level gap already fixed for env_file. - with pytest.raises(UnsupportedComposeError, match="tmpfs entry must be a string"): + with pytest.raises(UnsupportedComposeError, match="'tmpfs' entry must be a string"): validate({"services": {"app": {"image": "x", "tmpfs": [5]}}}) def test_tmpfs_string_and_list_of_strings_still_accepted(self) -> None: From ca9600dcfe3d1125ac1920f4d3a6c79e1177fc24 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:09:35 +0300 Subject: [PATCH 22/43] docs: document the Expand chokepoint and this round's gate closures The change file's "rejects every malformed structural-key shape" claim was false when three more review rounds kept finding the same defect class -- update it to describe what actually closed this round (the Expand type-guard chokepoint, healthcheck.test's shape, depends_on's list-element shape, and the shared list/map validator tightening) and state precisely what remains scoped rather than re-claiming victory. architecture/supported-subset.md: fix the false command/entrypoint null equivalence (command: null is accepted, entrypoint: null is rejected -- pre-existing, unchanged here), and document the new rejections and the Expand guard's role. --- architecture/supported-subset.md | 64 ++++++++++-- ...2026-07-13.10-close-structural-key-gate.md | 98 ++++++++++++++++++- 2 files changed, 153 insertions(+), 9 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index 22b42bd..2249e4c 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -42,23 +42,42 @@ warns (ignored, behavior-neutral inside a single pod) or raises The remaining keys documented below are **structural keys**, handled outside the registry because their `emit` needs `project_dir`, spans multiple keys, or occupies the image/command slot. + The list-shaped registry keys (`group_add`, `cap_add`, `cap_drop`, + `security_opt`, `devices`) share one validator, `keys._validate_list`; the + map-shaped ones (`labels`, `annotations`) and the pod-level `extra_hosts` + share `keys.validate_map`. Both require every list element to be a string + and every mapping value to be a string, number, or null — a non-string + element or a non-scalar value (e.g. `cap_add: [{NET_ADMIN: true}]`, a list + entry that is itself a mapping) used to be accepted and `str()`'d/`repr()`'d + straight into the flag value instead of being rejected. - **`image`:** a string; `image_for` (`compose2pod/emit.py`) reads it verbatim when the service has no `build`. A service must set at least one of `image` or `build`; a service with neither, or a non-string `image` while `build` is absent, raises at the gate (`_validate_image`, `compose2pod/parsing.py`). A non-string `image` alongside `build` is accepted, since `image_for` never reads it in that case — the CI image always wins. -- **`command`:** string or list, exactly like `entrypoint`. List form is argv - tokens; string form runs via `/bin/sh -c`. Any other shape (e.g. a mapping) - raises at the gate (`_validate_command`, `compose2pod/parsing.py`) — a - mapping is rejected outright rather than reaching `podman run` with only - its keys emitted as bare tokens and its values silently discarded. +- **`command`:** string or list. List form is argv tokens; string form runs + via `/bin/sh -c`. Any other shape (e.g. a mapping) raises at the gate + (`_validate_command`, `compose2pod/parsing.py`) — a mapping is rejected + outright rather than reaching `podman run` with only its keys emitted as + bare tokens and its values silently discarded. Each list element must + itself be a string; a non-string element (e.g. `command: [{run: tests}]`, + the same list/map YAML slip that trips up `environment`) raises the same + way, rather than being `str()`'d into a single mangled argv token. + `command: null` is accepted, treated the same as an absent `command` — + unlike `entrypoint: null` (see below), a narrower pre-existing divergence + between the two keys. - **`environment`:** list form (`- KEY=value`, `- KEY`) or mapping form (`KEY: value`, `KEY:`). A null mapping value (`KEY:`) means "pass `KEY` through from the host", emitted as a bare `-e KEY` exactly like the list form `- KEY`. The key itself must be a list or mapping; any other shape (e.g. a bare - string) raises at the gate. + string) raises at the gate. List elements must themselves be strings, and + mapping values must be a string, number, or null; the commonest violation + is the classic YAML slip of mixing list and map form + (`environment: [{KEY: value}]` instead of `- KEY=value`), which used to be + accepted and emit the literal `-e "{'KEY': 'value'}"` — both rules are + enforced by the shared `keys.validate_map` (`compose2pod/keys.py`). - **`env_file`:** a string or a list. Any other shape raises (`_validate_env_file`, `compose2pod/parsing.py`). Each list element must itself be a string; a non-string element (e.g. `env_file: [5]`) raises the @@ -68,7 +87,12 @@ warns (ignored, behavior-neutral inside a single pod) or raises shell form (`/bin/sh -c `), mirroring `command`. Emitted as `--entrypoint ` with the remaining tokens placed ahead of the command after the image, so `podman run --entrypoint a IMAGE b ` - runs `a b ` -- no JSON needed. A string (shell-form) entrypoint + runs `a b ` -- no JSON needed. Each list element must itself be a + string, the same rule and rejection as `command`'s list form. Unlike + `command`, `entrypoint: null` raises rather than being treated as absent — + a pre-existing, narrower divergence in how the two keys' validators handle + an explicit `null` (`_validate_command` checks for `None` first and treats + it as "absent"; `_validate_entrypoint` does not). A string (shell-form) entrypoint ignores the service `command`, matching Docker; `validate()` warns when both are set. The target's `--command` override still applies as explicit intent, but when the target has a string entrypoint, the override tokens land after @@ -318,6 +342,16 @@ honors both, refusing loudly on overlap rather than picking a precedence. (`_validate_service_healthcheck`, `compose2pod/parsing.py`) — previously a non-mapping healthcheck reached `.get()` calls downstream and crashed raw instead of failing at the gate. +- **`test`:** a bare string (shell form), `"NONE"` / `["NONE"]` (disabled), + `["CMD", ...]` (exec form), or `["CMD-SHELL", ]`. `health_cmd` + (`compose2pod/healthcheck.py`) is the sole reader and the sole validator — + `_validate_service_healthcheck` (`compose2pod/parsing.py`) calls it at + `validate()` time purely for its shape check (the returned `--health-cmd` + value is discarded there; `emit.py` calls it again to get the value for + real). Any other shape raises, including a `CMD-SHELL` whose argument isn't + a string and a `CMD` whose trailing elements aren't all strings (e.g. + `["CMD-SHELL", ["curl", "-f"]]`) — both used to reach `emit_script()` + unchecked and crash raw. - **`interval`:** parsed to whole seconds by `interval_seconds` (`compose2pod/healthcheck.py`). Supported forms: a bare number of seconds (`30`, `"30"`, `"30s"`), minutes (`"2m"`), and milliseconds (`"500ms"`). @@ -496,6 +530,15 @@ interpolated string leaf into a double-quoted POSIX-shell fragment with the variable references left live, so the generated script's own shell expands them against its runtime environment when the script runs. +`Expand` (`compose2pod/keys.py`) is a frozen dataclass wrapping the `str` +value every interpolated field carries; it rejects a non-`str` value at +construction (`__post_init__`) with `UnsupportedComposeError`, since +`to_shell()`/`variable_names()` both assume a `str` and crash raw otherwise. +This is a chokepoint, not a substitute for validating each key's shape at +`validate()` time: it only converts an already-malformed value into a clean +error one step later, at `emit_script()`, instead of leaving it to crash +inside `shell.py`'s regex matching. + The interpolated set is exactly what `Expand(...)` wraps in `compose2pod/emit.py` and `compose2pod/keys.py` — there is no separate list to maintain by hand, so treat the **service-key registry** @@ -566,7 +609,12 @@ names (short form, each defaulting to `service_started`) or a mapping of service name to a per-dependency mapping (long form, read for `condition`). Anything else — a bare string, a number, a mapping whose value isn't itself a mapping — raises `UnsupportedComposeError` at the gate instead of failing -later with a raw `AttributeError`/`TypeError` when the shape is walked. +later with a raw `AttributeError`/`TypeError` when the shape is walked. Each +short-form list element must itself be a string; a non-string element (e.g. +`depends_on: [{db: {condition: service_healthy}}]`, the same list/map YAML +slip that trips up `environment`/`command`) raises the same way, instead of +crashing raw (`TypeError: unhashable type`) from inside `validate()` itself +when the list was passed to `dict.fromkeys`. ## YAML anchors and merge keys diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index 94770b0..e8daae4 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,5 +1,5 @@ --- -summary: validate() rejects every malformed structural-key shape emit can crash or misrender on (image, command, environment, env_file, volumes, tmpfs, services, network aliases, healthcheck scalars), emit rejects a malformed --artifact and now also guards referenced_variables on pod name, so a bad document always fails with UnsupportedComposeError instead of a raw crash or silent mis-emission. +summary: validate() rejects every malformed structural-key shape found across three review rounds (image, command, entrypoint, environment, env_file, volumes, tmpfs, services, network aliases, healthcheck scalars and test, depends_on list entries, and every list/map key's non-scalar elements/values), emit rejects a malformed --artifact and guards referenced_variables on pod name, and Expand.value now gained a type guard so a leak past any per-key validator fails with UnsupportedComposeError instead of a raw crash, no matter which key is responsible. See "Round 3" below for what this closes as a class versus what remains scoped to this round's findings. --- # Design: Close the structural-key gate @@ -104,3 +104,99 @@ every `tests/integration/` scenario continue to pass unchanged. - **A document that used to be accepted is now rejected** (low x low): only for shapes that already crashed or silently corrupted — nothing that ever produced a correct script changes behavior. + +## Round 3: a chokepoint instead of more enumeration + +Two further review rounds each found more members of the same defect class +above by hand-enumerating keys — proof that hand-enumeration alone is leaky. +This round closes it with a chokepoint where one exists, plus the remaining +per-key gaps: + +| Input | Before | Kind | +|-------|--------|------| +| any `Token`/`Expand` construction site fed a non-`str` value (e.g. a healthcheck `test` shape gap reaching `emit.py`) | raw `TypeError` inside `shell.py`'s `re.finditer`, wherever the leak happened to surface | crash | +| `healthcheck: {test: ["CMD-SHELL", ["curl", "-f"]]}` | `validate()` clean; `emit_script()` crashed (`health_cmd` returned `test[1]` — a list — unchecked, the `# ty: ignore` at `healthcheck.py:33` was hiding exactly this) | crash | +| `environment: [{KEY: value}]` (the classic list/map YAML slip) | accepted; emits the literal `-e "{'KEY': 'value'}"` | silent corruption | +| `command: [{run: tests}]` (same slip, list-of-mapping) | accepted; `str()`'d into a single mangled argv token | silent corruption | +| `labels`/`annotations`/`cap_add`/`cap_drop`/`group_add`/`security_opt`/`devices`/`extra_hosts` — a non-string list element or non-scalar map value | accepted; `str()`/`f"..."`'d straight into the flag value | silent corruption | +| `depends_on: [{db: {condition: service_healthy}}]` (same slip, short form) | raw `TypeError: unhashable type: 'dict'` from `dict.fromkeys` **inside `validate()` itself** | crash | + +### The chokepoint: `Expand.value: str` + +`Expand` is the token type every interpolated value flows through on its way +to `shell.to_shell`/`variable_names`, both of which assume a `str` and crash +raw otherwise. Rather than trust every `Expand(...)` call site across +`keys.py`/`emit.py`/`pod.py`/`resources.py` to have validated or cast first, +`Expand` gained a `__post_init__` that rejects a non-`str` value with +`UnsupportedComposeError`. This converts the entire raw-crash half of the +class into one clean error, in one place, regardless of which key's +per-key validator has a gap — it is why the healthcheck `test` finding above +was already a clean error before its dedicated gate (below) was added. + +This is **defense-in-depth, not a substitute** for gating shape at +`validate()` time: it fires at `emit_script()`, one call later than +`validate()` would, and it cannot help at all against the silent-corruption +half of the class — a value that a caller already coerced with `str()` +(`Expand(value=str(item))`, used throughout `keys.py`/`emit.py`) is a `str` +by the time `Expand` sees it, no matter what it was before. That half is +still closed key-by-key, via the list/map validator tightening and the +`depends_on` fix below. + +### The rest: tightening the two shared list/map validators + +`labels`, `annotations`, `cap_add`, `cap_drop`, `group_add`, `security_opt`, +`devices`, and `extra_hosts` are all either a `SERVICE_KEYS` `_list()`/`_map()` +entry or call `keys.validate_map` directly (`extra_hosts`, `environment`) — +so tightening `_validate_list`/`validate_map` to check list elements and map +values, once, covers all of them plus `environment` in a single edit, rather +than eight near-identical hand-written validators. Rule: a list-shaped key's +elements must be strings (every one of these keys' list forms is Compose +string data — bare capability/device/label/host names or `KEY=value` pairs; +none has a legitimate non-string element), and a map-shaped key's values must +be a string, number, or `null` (`is_number` plus an explicit `None` check) — +`null` keeps its existing per-key meaning (`environment`'s host-passthrough, +`labels`/`annotations`' empty label). `command`/`entrypoint` don't route +through the registry, so their list-element check is added directly in +`parsing.py` (`_validate_argv_list`, shared by both). + +`depends_on`'s list (short) form gets the equivalent element check in +`graph.py`, matching the local style its map form already uses for its own +malformed-shape errors — the one finding in this round whose raw crash +happens **inside `validate()` itself**, not at `emit_script()`. + +### Non-goal (unchanged) + +Still no structural-key registry (`decisions/2026-07-12-reject-structural-key-registry.md`) +and no revisit of that decision — the chokepoint is a type guard on one +existing dataclass, not a callback table. + +### Scope of the "closed as a class" claim + +The `Expand` guard closes the **raw-crash half** of the defect class +completely and structurally: any value that reaches `Expand` non-str now +fails cleanly, regardless of which key or which future key is responsible. +The **silent-corruption half** has no equivalent single chokepoint (a value +already coerced with `str()`/interpolated into an f-string is, by +construction, a `str` — there is nothing later to intercept) and remains +closed **per validated shape**: every list/map-shaped key this round audited +(the `SERVICE_KEYS` `_list()`/`_map()` entries, `extra_hosts`, `environment`, +`command`, `entrypoint`, `depends_on`) is now covered. Keys whose shape +validators were already hardened in earlier rounds — `ulimits`, `sysctls`, +`dns`/`dns_search`/`dns_opt`, `secrets`/`configs` definitions and references, +`deploy.resources` — were not re-audited in this round; nothing in this +round's testing found a gap in them, but they were not exhaustively +re-verified against the silent-corruption pattern either. + +### Testing (Round 3) + +TDD throughout: each finding above got a failing test confirming the raw +crash (or, for silent corruption, confirming `validate()` returned clean and +the corrupted value reached emit) before the fix. `tests/test_keys.py` +(`Expand`, `_validate_list`/`validate_map` element/value checks), +`tests/test_healthcheck.py` and `tests/test_parsing.py` (`test` shape, +`command`/`entrypoint`/`environment`/`labels`/`cap_add` element checks), +`tests/test_graph.py` and `tests/test_parsing.py` (`depends_on` list +elements), and `tests/test_pod.py` (`extra_hosts` element/value checks). +`tests/conftest.py`'s `chats_compose` fixture and every `tests/integration/` +scenario continue to pass unchanged. `just test-ci` at 100% line coverage, +`just lint-ci`, and `just check-planning` all clean. From 2cc588f3586610c87ed34718e1b8b0eb471e4629 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:16:40 +0300 Subject: [PATCH 23/43] fix: reject non-string target in store long-form references Reject a non-string `target` in service secret/config long-form references with UnsupportedComposeError instead of silently accepting it and letting it pass through to the emitted script. Both secrets and configs now validate that `target`, when present, must be a string. Configs continue to require that string targets be absolute paths. Update architecture/supported-subset.md to document that `target` must be a string when present in long-form store references. --- architecture/supported-subset.md | 6 +++--- compose2pod/stores.py | 3 +++ tests/test_stores.py | 12 ++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index 2249e4c..71e8298 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -433,8 +433,8 @@ colon-less entry that is not absolute (e.g. `./cache`) is malformed and raises. `podman run`, assembled per service by `stores.flags` (`compose2pod/stores.py`, called from `emit_script`, `compose2pod/emit.py`), where `target` defaults to the secret's own name when the reference doesn't give one (short form, - or long form without `target`). `uid`/`gid`/`mode` are only added when the - long form gives them explicitly; `mode` renders as a 4-digit octal string + or long form without `target`). When present, `target` must be a string. + `uid`/`gid`/`mode` are only added when the long form gives them explicitly; `mode` renders as a 4-digit octal string when given as a Python int (`0o400` becomes `"0400"`) and passes through verbatim when given as a string (`_flags_for`, `compose2pod/stores.py`). When `uid`/`gid`/`mode` are omitted, podman itself applies its own defaults: the @@ -496,7 +496,7 @@ colon-less entry that is not absolute (e.g. `./cache`) is malformed and raises. Unlike a secret, whose default `target` is its own name (mounted by podman under `/run/secrets/`), a config's default `target` is the container-root absolute path `/` (`CONFIG.default_target`, - `compose2pod/stores.py`). A long-form `target` must be an absolute path + `compose2pod/stores.py`). When present, `target` must be a string. A long-form `target` must be an absolute path (start with `/`); a relative target raises (`CONFIG.require_absolute_target`, checked by `_check_target`, `compose2pod/stores.py`). `uid`/`gid`/`mode` behave exactly as for secrets: diff --git a/compose2pod/stores.py b/compose2pod/stores.py index b9e56c2..c718f6a 100644 --- a/compose2pod/stores.py +++ b/compose2pod/stores.py @@ -95,6 +95,9 @@ def _check_long_form_scalars(name: str, ref: dict[str, Any], kind: StoreKind) -> def _check_target(name: str, ref: dict[str, Any], kind: StoreKind) -> None: + if "target" in ref and not isinstance(ref["target"], str): + msg = f"service {name!r}: {kind.label} target must be a string" + raise UnsupportedComposeError(msg) if kind.require_absolute_target and isinstance(ref.get("target"), str) and not ref["target"].startswith("/"): msg = f"service {name!r}: {kind.label} target {ref['target']!r} must be an absolute path" raise UnsupportedComposeError(msg) diff --git a/tests/test_stores.py b/tests/test_stores.py index 944bc33..e528136 100644 --- a/tests/test_stores.py +++ b/tests/test_stores.py @@ -93,6 +93,12 @@ def test_non_scalar_long_form_value_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="must be an int or string"): stores.validate(_doc("secrets", {"s": {"file": "./a"}}, [{"source": "s", "uid": [1]}])) + def test_non_string_target_rejected(self) -> None: + with pytest.raises(UnsupportedComposeError, match="secret target"): + stores.validate(_doc("secrets", {"s": {"file": "./a"}}, [{"source": "s", "target": 5}])) + with pytest.raises(UnsupportedComposeError, match="secret target"): + stores.validate(_doc("secrets", {"s": {"file": "./a"}}, [{"source": "s", "target": ["t"]}])) + def test_content_in_secret_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="unsupported keys"): stores.validate(_doc("secrets", {"a": {"content": "x"}})) @@ -109,6 +115,12 @@ def test_relative_long_form_target_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match=r"config target 'c.conf' must be an absolute path"): stores.validate(_doc("configs", {"c": {"file": "./c"}}, [{"source": "c", "target": "c.conf"}])) + def test_non_string_target_rejected(self) -> None: + with pytest.raises(UnsupportedComposeError, match="config target"): + stores.validate(_doc("configs", {"c": {"file": "./c"}}, [{"source": "c", "target": 5}])) + with pytest.raises(UnsupportedComposeError, match="config target"): + stores.validate(_doc("configs", {"c": {"file": "./c"}}, [{"source": "c", "target": ["/etc"]}])) + def test_external_rejected_with_config_wording(self) -> None: with pytest.raises(UnsupportedComposeError, match="external configs are not supported"): stores.validate(_doc("configs", {"c": {"external": True}}, ["c"])) From 420dbc7f1d457059b324d8cfb205106e424df460 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:53:13 +0300 Subject: [PATCH 24/43] fix: reject non-string depends_on entries in extends merge resolve_extends runs ahead of validate() (called from cli.py before the gate), so its own list-to-mapping normalization for depends_on needs its own element check. Without it, a list-of-mapping depends_on entry (the same list/map YAML slip graph.py already rejects cleanly outside extends) crashed raw with TypeError: unhashable type: 'dict' building {dep: {} for dep in value}. --- compose2pod/extends.py | 4 ++++ tests/test_extends.py | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/compose2pod/extends.py b/compose2pod/extends.py index 3f4817a..fc813f3 100644 --- a/compose2pod/extends.py +++ b/compose2pod/extends.py @@ -40,6 +40,10 @@ def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN if key == "environment": return pairs_to_mapping(name, key, value) if key == "depends_on": + for dep in value: + if not isinstance(dep, str): + msg = f"service {name!r}: depends_on entry {dep!r} must be a string" + raise UnsupportedComposeError(msg) return {dep: {} for dep in value} msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" raise UnsupportedComposeError(msg) diff --git a/tests/test_extends.py b/tests/test_extends.py index 6316969..4892a27 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -215,6 +215,22 @@ def test_extends_non_dict_base_defers_to_validate(self) -> None: assert "extends" not in result["services"]["web"] assert result["services"]["base"] is None + def test_depends_on_list_with_mapping_entry_raises_instead_of_crashing_raw(self) -> None: + # Without extends, graph.depends_on's own element check catches this + # cleanly. extends.py has its own merge-time normalization + # (_as_mapping) that runs ahead of that check and, before this fix, + # crashed raw building `{dep: {} for dep in value}` -- a dict list + # element isn't hashable. + doc = { + "services": { + "db": {"image": "pg"}, + "base": {"image": "a", "depends_on": [{"db": {"condition": "service_healthy"}}]}, + "web": {"extends": {"service": "base"}, "depends_on": ["db"]}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry .* must be a string"): + resolve_extends(doc) + def test_incompatible_structural_concat_form_is_refused(self) -> None: doc = { "services": { From 5639c90b7cba4b4d9b130021da684511c0a0b1b3 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:54:12 +0300 Subject: [PATCH 25/43] fix: reject non-string list elements in pairs_to_mapping pairs_to_mapping backs the merge policy for environment/labels/ annotations/ulimits under extends. Its list branch used str(item) on each element, so a non-string element (the same list/map YAML slip validate() rejects everywhere else) was laundered into a mapping key instead of being rejected -- the merged service came out well-formed enough for validate() to accept and emit to render a literal Python repr as a flag value. --- compose2pod/keys.py | 5 ++++- tests/test_extends.py | 15 +++++++++++++++ tests/test_keys.py | 12 ++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 7e20802..f6655bf 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -141,7 +141,10 @@ def pairs_to_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa if isinstance(value, list): result: dict[str, Any] = {} for item in value: - pair_key, sep, pair_value = str(item).partition("=") + if not isinstance(item, str): + msg = f"service {name!r}: '{key}' entries must be strings" + raise UnsupportedComposeError(msg) + pair_key, sep, pair_value = item.partition("=") result[pair_key] = pair_value if sep else None return result msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" diff --git a/tests/test_extends.py b/tests/test_extends.py index 4892a27..32051d6 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -231,6 +231,21 @@ def test_depends_on_list_with_mapping_entry_raises_instead_of_crashing_raw(self) with pytest.raises(UnsupportedComposeError, match=r"depends_on entry .* must be a string"): resolve_extends(doc) + def test_labels_list_with_mapping_entry_raises_instead_of_laundering(self) -> None: + # Before this fix, a non-string labels list element on the extending + # (child) side survived the merge: pairs_to_mapping str()'d the dict + # element into a mapping *key* instead of rejecting it, so the merged + # service came out well-formed enough for validate() to accept and + # emit to render the literal --label "{'BAD': 'x'}". + doc = { + "services": { + "base": {"image": "a", "labels": {"OK": "1"}}, + "web": {"extends": {"service": "base"}, "labels": [{"BAD": "x"}]}, + } + } + with pytest.raises(UnsupportedComposeError, match="'labels' entries must be strings"): + resolve_extends(doc) + def test_incompatible_structural_concat_form_is_refused(self) -> None: doc = { "services": { diff --git a/tests/test_keys.py b/tests/test_keys.py index b82b8a3..5aa166f 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -9,6 +9,7 @@ _merge_map, _validate_list, _validate_ulimits, + pairs_to_mapping, validate_map, ) from compose2pod.parsing import SUPPORTED_SERVICE_KEYS @@ -169,3 +170,14 @@ def test_merge_map_normalizes_list_form(self) -> None: def test_merge_map_refuses_incompatible_form(self) -> None: with pytest.raises(UnsupportedComposeError, match="cannot merge 'labels' across incompatible forms"): _merge_map("web", "labels", {"team": "core"}, 5) + + def test_pairs_to_mapping_rejects_non_string_list_element(self) -> None: + # Before this fix, a non-string element was str()'d into a mapping + # *key* instead of rejected -- laundering a malformed list-of-mapping + # value into a well-formed-looking mapping that validate() then + # accepted and emit rendered as a literal Python repr. + with pytest.raises(UnsupportedComposeError, match="'labels' entries must be strings"): + pairs_to_mapping("web", "labels", [{"BAD": "x"}]) + + def test_pairs_to_mapping_accepts_string_list_elements(self) -> None: + assert pairs_to_mapping("web", "labels", ["team=core", "BARE"]) == {"team": "core", "BARE": None} From 116d74dfc9978577ffb253c4383099c9181ac155 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:56:29 +0300 Subject: [PATCH 26/43] fix: reject non-string mapping keys at the gate validate() assumed every mapping key is a str, but PyYAML routinely produces int/bool keys (a bare 3: or, under YAML 1.1, on:/off:). A non-string key crashed raw wherever the gate first sorted or regex- matched the mapping: the top-level document, a service's own keys, a healthcheck's keys, and a secrets/configs definition name. One shared helper, keys.require_string_keys, closes all four sites with a single clean UnsupportedComposeError naming the offending key. --- compose2pod/keys.py | 16 ++++++++++++++++ compose2pod/parsing.py | 5 ++++- compose2pod/stores.py | 3 ++- tests/test_keys.py | 15 +++++++++++++++ tests/test_parsing.py | 21 +++++++++++++++++++++ tests/test_stores.py | 7 +++++++ 6 files changed, 65 insertions(+), 2 deletions(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index f6655bf..2bc6082 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -79,6 +79,22 @@ def is_number(value: Any) -> bool: # noqa: ANN401 - Compose values are untyped return not isinstance(value, bool) and isinstance(value, int | float | str) +def require_string_keys(where: str, mapping: dict[Any, Any]) -> None: + """Check every key of a raw YAML/JSON mapping is a string. + + PyYAML routinely produces non-string keys (a bare `3:` is an int; under + YAML 1.1, a bare `on:`/`off:` is a bool). Every mapping-key consumer + downstream (`sorted()`, `str.startswith`, a compiled regex) assumes + `str` and crashes raw otherwise. This is the one shared check the gate + runs before it reads any of `mapping`'s keys, so a non-string key fails + clean regardless of which mapping it turned up in. + """ + for key in mapping: + if not isinstance(key, str): + msg = f"{where}: key {key!r} must be a string" + raise UnsupportedComposeError(msg) + + def _validate_number(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON if not is_number(value): msg = f"service {name!r}: '{key}' must be a number or string" diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 5599e97..5cf09b1 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -6,7 +6,7 @@ from compose2pod.exceptions import UnsupportedComposeError from compose2pod.graph import depends_on, hostnames from compose2pod.healthcheck import has_healthcheck, health_cmd, interval_seconds -from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, is_number, validate_map +from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS, is_number, require_string_keys, validate_map from compose2pod.pod import uses_pod_options, validate_pod_options from compose2pod.resources import validate_deploy @@ -27,6 +27,7 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: if not isinstance(healthcheck, dict): msg = f"service {name!r}: healthcheck must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: healthcheck", healthcheck) for key in sorted(healthcheck): if key.startswith("x-"): continue @@ -155,6 +156,7 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo if not isinstance(svc, dict): msg = f"service {name!r} must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}", svc) warnings: list[str] = [] for key in sorted(svc): if key.startswith("x-"): @@ -203,6 +205,7 @@ def validate(compose: dict[str, Any]) -> list[str]: if not isinstance(compose, dict): msg = f"compose document must be a mapping, got {type(compose).__name__}" raise UnsupportedComposeError(msg) + require_string_keys("compose document", compose) warnings: list[str] = [] unknown_top = {k for k in compose if k not in SUPPORTED_TOP_LEVEL_KEYS and not k.startswith("x-")} if unknown_top: diff --git a/compose2pod/stores.py b/compose2pod/stores.py index c718f6a..6bca524 100644 --- a/compose2pod/stores.py +++ b/compose2pod/stores.py @@ -13,7 +13,7 @@ from typing import Any from compose2pod.exceptions import UnsupportedComposeError -from compose2pod.keys import Token +from compose2pod.keys import Token, require_string_keys from compose2pod.shell import to_shell, variable_names @@ -128,6 +128,7 @@ def _validate_kind(compose: dict[str, Any], kind: StoreKind) -> None: msg = f"top-level {kind.top_key!r} must be a mapping" raise UnsupportedComposeError(msg) defs = defs or {} + require_string_keys(f"top-level {kind.top_key!r}", defs) for name, definition in defs.items(): _validate_def(name, definition, kind) for name, svc in (compose.get("services") or {}).items(): diff --git a/tests/test_keys.py b/tests/test_keys.py index 5aa166f..cec8710 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -10,6 +10,7 @@ _validate_list, _validate_ulimits, pairs_to_mapping, + require_string_keys, validate_map, ) from compose2pod.parsing import SUPPORTED_SERVICE_KEYS @@ -34,6 +35,20 @@ def test_non_string_value_raises_instead_of_reaching_shell_py_raw(self) -> None: Expand(value=123) # ty: ignore[invalid-argument-type] +class TestRequireStringKeys: + """Shared guard against PyYAML's non-string mapping keys (int, or a YAML-1.1 bool).""" + + def test_all_string_keys_pass(self) -> None: + require_string_keys("compose document", {"a": 1, "b": 2}) + + def test_empty_mapping_passes(self) -> None: + require_string_keys("compose document", {}) + + def test_non_string_key_raises_naming_the_key_and_location(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"compose document: key 3 must be a string"): + require_string_keys("compose document", {3: "x"}) + + def test_supported_service_keys_snapshot() -> None: assert { "image", diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 76b284c..c3905c6 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -113,6 +113,27 @@ def test_unknown_top_level_key_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="foo"): validate({"services": {"app": {"image": "x"}}, "foo": {}}) + def test_non_string_top_level_key_raises_instead_of_crashing_raw(self) -> None: + # PyYAML routinely produces non-string keys (int, or a YAML-1.1 bool + # from a bare `on:`). Used to crash raw: AttributeError: 'int' object + # has no attribute 'startswith', from k.startswith("x-"). + with pytest.raises(UnsupportedComposeError, match="key 3 must be a string"): + validate({3: "x", "services": {"app": {"image": "x"}}}) # ty: ignore[invalid-argument-type] + + def test_non_string_service_key_raises_instead_of_crashing_raw(self) -> None: + # A non-string key *inside* a service mapping (an indent slip) used + # to crash raw: TypeError: '<' not supported between instances of + # 'int' and 'str', from sorted(svc). + with pytest.raises(UnsupportedComposeError, match="key 8080 must be a string"): + validate({"services": {"app": {"image": "x", 8080: 8080}}}) + + def test_non_string_healthcheck_key_raises_instead_of_crashing_raw(self) -> None: + # Same class as the service-key case, one level deeper: used to crash + # raw via sorted(healthcheck). + compose = {"services": {"app": {"image": "x", "healthcheck": {1: "x", "test": "true"}}}} + with pytest.raises(UnsupportedComposeError, match="key 1 must be a string"): + validate(compose) + def test_top_level_networks_is_ignored_with_warning(self) -> None: warnings = validate({"services": {"app": {"image": "x"}}, "networks": {"default": None}}) assert any("networks" in w for w in warnings) diff --git a/tests/test_stores.py b/tests/test_stores.py index e528136..0b4d19f 100644 --- a/tests/test_stores.py +++ b/tests/test_stores.py @@ -60,6 +60,13 @@ def test_top_level_secrets_not_a_mapping_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="top-level 'secrets' must be a mapping"): stores.validate({"services": {"app": {"image": "x"}}, "secrets": ["a"]}) + def test_non_string_secret_name_raises_instead_of_crashing_raw(self) -> None: + # PyYAML can produce a non-string mapping key (e.g. an unquoted `1:`). + # Used to crash raw: TypeError: expected string or bytes-like object, + # got 'int', from _NAME.fullmatch(name). + with pytest.raises(UnsupportedComposeError, match="key 1 must be a string"): + stores.validate(_doc("secrets", {1: {"file": "./s"}})) + def test_non_string_or_mapping_reference_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="entry must be a string or mapping"): stores.validate(_doc("secrets", {"a": {"file": "./a"}}, [123])) From 9a8e30cc3711230d9e23b67f8b4a8893c2e3f648 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 00:59:02 +0300 Subject: [PATCH 27/43] fix: normalize boolean map values like Docker instead of rejecting them environment/labels/annotations/extra_hosts map values must be a string, number, boolean, or null again -- the branch's list/map hardening had started rejecting a bool value outright, an over-rejection regression versus `docker compose config`, which normalizes `DEBUG: true` to the string "true" rather than refusing it. key_value_pairs/extra_host_pairs now render a bool lowercase ('true'/'false') instead of leaking Python's str(True) == 'True' into the flag value. --- compose2pod/keys.py | 20 ++++++++++++++++---- tests/test_emit.py | 18 ++++++++++++++++++ tests/test_keys.py | 5 +++++ tests/test_parsing.py | 5 +++++ tests/test_pod.py | 6 ++++++ 5 files changed, 50 insertions(+), 4 deletions(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 2bc6082..9d5b26f 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -43,6 +43,18 @@ def __post_init__(self) -> None: Token = str | Expand +def _render_scalar(value: Any) -> str: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Render a map scalar value the way `docker compose config` does. + + A bool renders lowercase ('true'/'false'), matching Docker's own + normalization -- Python's str(True) == 'True' would otherwise leak into + the emitted flag value verbatim. + """ + if isinstance(value, bool): + return "true" if value else "false" + return str(value) + + def key_value_pairs(value: list[Any] | dict[str, Any]) -> list[Any]: """Compose list/map key-value section as 'KEY=value' / 'KEY' entries. @@ -51,7 +63,7 @@ def key_value_pairs(value: list[Any] | dict[str, Any]) -> list[Any]: """ if isinstance(value, list): return value - return [key if val is None else f"{key}={val}" for key, val in value.items()] + return [key if val is None else f"{key}={_render_scalar(val)}" for key, val in value.items()] @dataclasses.dataclass(frozen=True, slots=True, kw_only=True) @@ -112,8 +124,8 @@ def _validate_list_elements(name: str, key: str, value: list[Any]) -> None: def _validate_map_values(name: str, key: str, value: dict[str, Any]) -> None: """Check every map value is a scalar or null, so emit can't repr() a dict/list into the script.""" for val in value.values(): - if val is not None and not is_number(val): - msg = f"service {name!r}: '{key}' values must be a string, number, or null" + if val is not None and not is_number(val) and not isinstance(val, bool): + msg = f"service {name!r}: '{key}' values must be a string, number, boolean, or null" raise UnsupportedComposeError(msg) @@ -217,7 +229,7 @@ def extra_host_pairs(value: list[Any] | dict[str, Any]) -> list[Any]: """Compose extra_hosts as 'host:ip' entries; map values keep their colons (IPv6-safe).""" if isinstance(value, list): return value - return [f"{host}:{ip}" for host, ip in value.items()] + return [f"{host}:{_render_scalar(ip)}" for host, ip in value.items()] def _validate_pull_policy(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped diff --git a/tests/test_emit.py b/tests/test_emit.py index e3c38b7..c0ca06f 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -50,6 +50,16 @@ def test_env_map_null_value_is_host_passthrough(self) -> None: flags = run_flags("app", svc, "p", "/builds/x") assert flags[4:8] == ["-e", Expand(value="PASSTHRU"), "-e", Expand(value="SET=v")] + def test_env_map_boolean_value_normalizes_like_docker(self) -> None: + # `environment: {DEBUG: true}` is valid Compose; `docker compose config` + # normalizes it to the string "true". Before this fix, this either + # emitted the raw Python repr "-e DEBUG=True" (silent corruption) or, + # after list/map hardening, was rejected outright (an over-rejection + # regression) -- the maintainer's ruling is to normalize, like Docker. + svc = {"image": "x", "environment": {"DEBUG": True, "VERBOSE": False}} + flags = run_flags("app", svc, "p", "/builds/x") + assert flags[4:8] == ["-e", Expand(value="DEBUG=true"), "-e", Expand(value="VERBOSE=false")] + def test_env_file_and_volume_resolved_against_project_dir(self) -> None: svc = {"image": "x", "env_file": "tests.env", "volumes": [".:/srv/www/"]} flags = run_flags("app", svc, "p", "/builds/chats") @@ -162,6 +172,10 @@ def test_labels_null_value_is_empty_label(self) -> None: flags = run_flags("app", {"image": "x", "labels": {"empty": None}}, "p", "/b") assert flags[4:6] == ["--label", Expand(value="empty")] + def test_labels_boolean_value_normalizes_like_docker(self) -> None: + flags = run_flags("app", {"image": "x", "labels": {"enabled": True}}, "p", "/b") + assert flags[4:6] == ["--label", Expand(value="enabled=true")] + def test_platform_flag(self) -> None: flags = run_flags("app", {"image": "x", "platform": "linux/amd64"}, "p", "/b") assert flags[4:6] == ["--platform", Expand(value="linux/amd64")] @@ -178,6 +192,10 @@ def test_annotations_null_value_is_bare_key(self) -> None: flags = run_flags("app", {"image": "x", "annotations": {"marker": None}}, "p", "/b") assert flags[4:6] == ["--annotation", Expand(value="marker")] + def test_annotations_boolean_value_normalizes_like_docker(self) -> None: + flags = run_flags("app", {"image": "x", "annotations": {"enabled": False}}, "p", "/b") + assert flags[4:6] == ["--annotation", Expand(value="enabled=false")] + def test_labels_still_emit_after_map_flags_refactor(self) -> None: flags = run_flags("app", {"image": "x", "labels": {"team": "api"}}, "p", "/b") assert flags[4:6] == ["--label", Expand(value="team=api")] diff --git a/tests/test_keys.py b/tests/test_keys.py index cec8710..0382e57 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -156,6 +156,11 @@ def test_validate_map_accepts_numeric_value(self) -> None: def test_validate_map_accepts_string_list_elements(self) -> None: validate_map("web", "labels", ["team=core", "BARE"]) + def test_validate_map_accepts_boolean_value(self) -> None: + # docker compose config normalizes `DEBUG: true` to the string "true"; + # a bool map value is valid Compose, not a shape to reject. + validate_map("web", "environment", {"DEBUG": True}) + class TestMergeCallables: """Direct tests of the merge policy functions, independent of any caller. diff --git a/tests/test_parsing.py b/tests/test_parsing.py index c3905c6..f06cd60 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -490,6 +490,11 @@ def test_environment_list_and_map_forms_still_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "environment": ["KEY=value", "BARE"]}}}) == [] assert validate({"services": {"app": {"image": "x", "environment": {"KEY": "value", "BARE": None}}}}) == [] + def test_environment_boolean_map_value_accepted(self) -> None: + # docker compose config normalizes `DEBUG: true` to the string "true"; + # rejecting a bool value here would be an over-rejection, not a fix. + assert validate({"services": {"app": {"image": "x", "environment": {"DEBUG": True}}}}) == [] + def test_labels_annotations_list_of_mapping_rejected_at_gate(self) -> None: with pytest.raises(UnsupportedComposeError, match="'labels' entries must be strings"): validate({"services": {"app": {"image": "x", "labels": [{"team": "core"}]}}}) diff --git a/tests/test_pod.py b/tests/test_pod.py index 4d01d93..c44bb55 100644 --- a/tests/test_pod.py +++ b/tests/test_pod.py @@ -124,6 +124,12 @@ def test_extra_hosts_mapping_form(self) -> None: services = {"a": {"extra_hosts": {"db": "10.0.0.5"}}} assert pod_create_flags(services, ["a"], []) == ["--add-host", Expand(value="db:10.0.0.5")] + def test_extra_hosts_boolean_value_normalizes_like_docker(self) -> None: + # Same boolean-map-value normalization as environment/labels/annotations, + # applied uniformly since extra_hosts is also a validate_map-shaped key. + services = {"a": {"extra_hosts": {"db": True}}} + assert pod_create_flags(services, ["a"], []) == ["--add-host", Expand(value="db:true")] + def test_extra_hosts_ipv6_value_keeps_colons(self) -> None: services = {"a": {"extra_hosts": {"myhost": "2001:db8::1"}}} assert pod_create_flags(services, ["a"], []) == [ From e66712481578aad1f89ac8d29bb8ec0b744034fe Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 08:56:47 +0300 Subject: [PATCH 28/43] fix: sort unsupported extends keys by repr to avoid a raw crash _extends_target's sorted(unknown) crashed raw (TypeError: '<' not supported between instances of 'str' and 'int') when a malformed 'extends' mapping mixed a non-string key with a string one. extends.py runs ahead of validate()'s gate, so this is exactly the kind of hostile input the gate never gets a turn on. Part of the extends.py audit C1 calls for. --- compose2pod/extends.py | 8 +++++++- tests/test_extends.py | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/compose2pod/extends.py b/compose2pod/extends.py index fc813f3..45d884a 100644 --- a/compose2pod/extends.py +++ b/compose2pod/extends.py @@ -24,7 +24,13 @@ def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose value raise UnsupportedComposeError(msg) unknown = set(ext) - {"service"} if unknown: - msg = f"service {name!r}: unsupported 'extends' keys {sorted(unknown)}" + # sorted(unknown) would crash raw (TypeError: '<' not supported...) + # if unknown holds keys of more than one incomparable type -- e.g. an + # int key alongside a str key, which a hostile 'extends' mapping can + # freely produce since this runs ahead of validate()'s gate. Sorting + # by repr keeps the message deterministic without assuming key types + # are mutually comparable. + msg = f"service {name!r}: unsupported 'extends' keys {sorted(unknown, key=repr)}" raise UnsupportedComposeError(msg) service = ext.get("service") if not isinstance(service, str): diff --git a/tests/test_extends.py b/tests/test_extends.py index 32051d6..125e61e 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -165,6 +165,16 @@ def test_unknown_extends_key_is_refused(self) -> None: with pytest.raises(UnsupportedComposeError, match="unsupported 'extends' keys"): resolve_extends(doc) + def test_unknown_extends_keys_with_mixed_types_do_not_crash_raw(self) -> None: + # `sorted(unknown)` used to crash raw -- TypeError: '<' not supported + # between instances of 'str' and 'int' -- once `unknown` held keys of + # more than one incomparable type (a non-string key mixed with a + # string one is exactly what a hostile/malformed 'extends' mapping + # can produce, since 'extends' is read ahead of validate()'s gate). + doc = {"services": {"web": {"extends": {"service": "base", 1: "x", "y": "z"}}, "base": {"image": "x"}}} + with pytest.raises(UnsupportedComposeError, match="unsupported 'extends' keys"): + resolve_extends(doc) + def test_non_string_service_is_refused(self) -> None: doc = {"services": {"web": {"extends": {"service": 5}}}} with pytest.raises(UnsupportedComposeError, match="extends 'service' must be a string"): From 79c4201cb6a7403dffd6ca030c8c949171245c47 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 08:57:31 +0300 Subject: [PATCH 29/43] fix: reject boolean ulimits values instead of leaking them bool IS an int in Python, so _validate_ulimits's isinstance(spec, int | str) let a boolean nofile/soft/hard through, and emit rendered the literal Python repr (--ulimit "nofile=True"). Unlike environment's boolean (which Docker normalizes to a string), a boolean ulimit has no sensible normalization, so it is rejected -- following the precedent stores._check_long_form_scalars already sets for uid/gid/mode. --- compose2pod/keys.py | 11 +++++++++-- tests/test_parsing.py | 12 ++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 9d5b26f..769c05c 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -254,10 +254,17 @@ def _validate_ulimits(name: str, key: str, value: Any) -> None: # noqa: ANN401 if set(spec) != {"soft", "hard"}: msg = f"service {name!r}: ulimit {limit!r} mapping must have exactly 'soft' and 'hard'" raise UnsupportedComposeError(msg) - if not isinstance(spec["soft"], int | str) or not isinstance(spec["hard"], int | str): + # bool IS an int in Python, so a plain `isinstance(..., int | str)` + # would silently let a boolean soft/hard value through. + if any( + isinstance(spec[bound], bool) or not isinstance(spec[bound], int | str) for bound in ("soft", "hard") + ): msg = f"service {name!r}: ulimit {limit!r} 'soft' and 'hard' must be int or str" raise UnsupportedComposeError(msg) - elif not isinstance(spec, int | str): + elif isinstance(spec, bool) or not isinstance(spec, int | str): + # A boolean ulimit is meaningless -- unlike environment's bool + # (which Docker normalizes to a string), there is no sensible + # normalization here, so it is rejected rather than coerced. msg = f"service {name!r}: ulimit {limit!r} must be an int or a soft/hard mapping" raise UnsupportedComposeError(msg) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index f06cd60..d2aca58 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -324,6 +324,18 @@ def test_ulimits_non_scalar_soft_hard_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="'soft' and 'hard' must be int or str"): validate({"services": {"app": {"image": "x", "ulimits": {"nofile": {"soft": [1, 2], "hard": 3}}}}}) + def test_ulimits_boolean_scalar_raises(self) -> None: + # bool IS an int in Python, so isinstance(spec, int | str) let it + # through and emit rendered the literal --ulimit "nofile=True". A + # boolean ulimit is meaningless -- unlike environment's bool (which + # Docker normalizes), there is no sensible ulimit normalization. + with pytest.raises(UnsupportedComposeError, match="must be an int or a soft/hard mapping"): + validate({"services": {"app": {"image": "x", "ulimits": {"nofile": True}}}}) + + def test_ulimits_boolean_soft_hard_raises(self) -> None: + with pytest.raises(UnsupportedComposeError, match="'soft' and 'hard' must be int or str"): + validate({"services": {"app": {"image": "x", "ulimits": {"nofile": {"soft": True, "hard": 100}}}}}) + def test_pull_policy_null_is_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "pull_policy": None}}}) == [] From 70e56975aee9c71e2361f0a87f22fd1189a1a478 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 09:05:11 +0300 Subject: [PATCH 30/43] fix: guard more sorted(unknown-keys) call sites against mixed-type keys C2 closed the 4-5 sites where the gate itself walks a mapping's keys directly, but sorted(unknown) at every "reject unrecognized keys" call site has the identical crash: a set of mixed-type keys (a non-string key alongside a string one) is unorderable. Reproduced and fixed the remaining sites that are reachable through validate() on a hostile document: a secret/config definition's own keys and a long-form service reference's own keys (compose2pod/stores.py), and deploy / deploy.resources / deploy.resources.limits / deploy.resources.reservations (compose2pod/resources.py). Same require_string_keys helper C2 introduced, called one step earlier so the non-string key is reported before the unrecognized-keys check ever runs. --- compose2pod/resources.py | 6 +++++- compose2pod/stores.py | 2 ++ tests/test_resources.py | 23 +++++++++++++++++++++++ tests/test_stores.py | 15 +++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/compose2pod/resources.py b/compose2pod/resources.py index 8864c80..0b2cce5 100644 --- a/compose2pod/resources.py +++ b/compose2pod/resources.py @@ -3,7 +3,7 @@ from typing import Any from compose2pod.exceptions import UnsupportedComposeError -from compose2pod.keys import Expand, Token, is_number +from compose2pod.keys import Expand, Token, is_number, require_string_keys # deploy.resources.limits. -> (podman flag, conflicting legacy key) @@ -22,6 +22,7 @@ def _validate_limits(name: str, svc: dict[str, Any], limits: Any) -> None: # no if not isinstance(limits, dict): msg = f"service {name!r}: deploy.resources.limits must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: deploy.resources.limits", limits) unknown = set(limits) - set(_LIMITS) if unknown: msg = f"service {name!r}: deploy.resources.limits: unsupported keys {sorted(unknown)}" @@ -40,6 +41,7 @@ def _validate_reservations(name: str, svc: dict[str, Any], reservations: Any) -> if not isinstance(reservations, dict): msg = f"service {name!r}: deploy.resources.reservations must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: deploy.resources.reservations", reservations) unknown = set(reservations) - {"cpus", "memory", "devices"} if unknown: msg = f"service {name!r}: deploy.resources.reservations: unsupported keys {sorted(unknown)}" @@ -63,6 +65,7 @@ def validate_deploy(name: str, svc: dict[str, Any]) -> None: if not isinstance(deploy, dict): msg = f"service {name!r}: 'deploy' must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: deploy", deploy) unknown = set(deploy) - {"resources"} if unknown: msg = f"service {name!r}: deploy: only 'resources' is supported (got {sorted(unknown)})" @@ -73,6 +76,7 @@ def validate_deploy(name: str, svc: dict[str, Any]) -> None: if not isinstance(resources, dict): msg = f"service {name!r}: deploy.resources must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: deploy.resources", resources) unknown = set(resources) - {"limits", "reservations"} if unknown: msg = f"service {name!r}: deploy.resources: unsupported keys {sorted(unknown)}" diff --git a/compose2pod/stores.py b/compose2pod/stores.py index 6bca524..67b6369 100644 --- a/compose2pod/stores.py +++ b/compose2pod/stores.py @@ -66,6 +66,7 @@ def _validate_def(name: str, definition: Any, kind: StoreKind) -> None: # noqa: if not isinstance(definition, dict): msg = f"{kind.label} {name!r} must be a mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"{kind.label} {name!r}", definition) unknown = set(definition) - kind.sources if unknown: if "external" in unknown: @@ -109,6 +110,7 @@ def _ref_source(name: str, ref: Any, kind: StoreKind) -> str: # noqa: ANN401 - if not isinstance(ref, dict): msg = f"service {name!r}: {kind.label} entry must be a string or mapping" raise UnsupportedComposeError(msg) + require_string_keys(f"service {name!r}: {kind.label} entry", ref) unknown = set(ref) - _LONG_FORM_KEYS if unknown: msg = f"service {name!r}: unsupported {kind.label} keys {sorted(unknown)}" diff --git a/tests/test_resources.py b/tests/test_resources.py index 8e6ca46..d13b024 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -95,6 +95,29 @@ def test_null_limits_is_noop(self) -> None: def test_null_reservations_is_noop(self) -> None: validate_deploy("app", _svc({"resources": {"reservations": None}})) + def test_deploy_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + # sorted(unknown) used to crash raw (TypeError: '<' not supported + # between instances of 'str' and 'int') once 'deploy's unrecognized + # keys mixed a non-string key with a string one. + with pytest.raises(UnsupportedComposeError, match=r"service 'app': deploy: key 1 must be a string"): + validate_deploy("app", _svc({"resources": {}, 1: "x", "y": "z"})) + + def test_resources_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"service 'app': deploy\.resources: key 1 must be a string"): + validate_deploy("app", _svc({"resources": {1: "x", "y": "z"}})) + + def test_limits_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + with pytest.raises( + UnsupportedComposeError, match=r"service 'app': deploy\.resources\.limits: key 1 must be a string" + ): + validate_deploy("app", _svc({"resources": {"limits": {1: "x", "y": "z"}}})) + + def test_reservations_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + with pytest.raises( + UnsupportedComposeError, match=r"service 'app': deploy\.resources\.reservations: key 1 must be a string" + ): + validate_deploy("app", _svc({"resources": {"reservations": {1: "x", "y": "z"}}})) + class TestDeployResourceFlags: def test_no_deploy_no_flags(self) -> None: diff --git a/tests/test_stores.py b/tests/test_stores.py index 0b4d19f..600cd4b 100644 --- a/tests/test_stores.py +++ b/tests/test_stores.py @@ -67,6 +67,21 @@ def test_non_string_secret_name_raises_instead_of_crashing_raw(self) -> None: with pytest.raises(UnsupportedComposeError, match="key 1 must be a string"): stores.validate(_doc("secrets", {1: {"file": "./s"}})) + def test_definition_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + # sorted(unknown) used to crash raw (TypeError: '<' not supported + # between instances of 'str' and 'int') once a secret/config + # definition's unrecognized keys mixed a non-string key with a + # string one. require_string_keys now catches the non-string key + # before the unknown-keys check is even reached. + with pytest.raises(UnsupportedComposeError, match=r"secret 'a': key 1 must be a string"): + stores.validate(_doc("secrets", {"a": {"file": "./a", 1: "x", "y": "z"}}, ["a"])) + + def test_long_form_reference_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: + # Same class as above, for a long-form service reference's own + # unrecognized keys. + with pytest.raises(UnsupportedComposeError, match=r"secret entry: key 1 must be a string"): + stores.validate(_doc("secrets", {"a": {"file": "./a"}}, [{"source": "a", 1: "x", "y": "z"}])) + def test_non_string_or_mapping_reference_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match="entry must be a string or mapping"): stores.validate(_doc("secrets", {"a": {"file": "./a"}}, [123])) From bb39c7cf8f5f9fb824ca0b07bdef1ede43d29ee8 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 09:07:22 +0300 Subject: [PATCH 31/43] docs: correct overclaiming and document Round 4's gate closures planning/changes/2026-07-13.10-close-structural-key-gate.md's frontmatter and Round 3 text claimed the raw-crash half was closed "no matter which key is responsible" -- false, since extends.py runs ahead of validate() and was never audited, and non-string mapping keys crash through a different mechanism (sorted()/startswith) than the Expand chokepoint covers. Added a Round 4 section stating precisely what this round verified closed, what remains unverified, and recording the I2 boolean normalization ruling as a deliberate design decision. architecture/supported-subset.md: documented the string-keys requirement, boolean map-value normalization, ulimits' boolean rejection, and fixed the stale claim that labels/annotations/ulimits list-form merges under extends are refused rather than coerced. --- architecture/supported-subset.md | 68 +++++-- ...2026-07-13.10-close-structural-key-gate.md | 166 +++++++++++++++++- 2 files changed, 216 insertions(+), 18 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index 71e8298..ceb3a6a 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -17,6 +17,17 @@ warns (ignored, behavior-neutral inside a single pod) or raises - **Extension fields:** any key prefixed `x-` is accepted and ignored silently, per the Compose spec. This is what lets a document hold shared config in a top-level `x-*` block for reuse via YAML anchors. +- **Mapping keys must be strings.** PyYAML routinely produces a non-string + key — a bare `3:` parses as an int, and under YAML 1.1 a bare + `on:`/`off:`/`yes:`/`no:` parses as a bool. Every mapping-key consumer + downstream (`sorted()`, `str.startswith`, the secret/config name regex) + assumes `str` and crashes raw otherwise. `require_string_keys` + (`compose2pod/keys.py`) is the one shared check the gate runs before + reading any mapping's keys — applied to the top-level document itself, + each service, each service's `healthcheck`, and each top-level + `secrets`/`configs` definition block — so a non-string key raises + `UnsupportedComposeError` naming the offending key, regardless of which of + those four mappings it turned up in. - Everything else raises. ## Service keys @@ -46,10 +57,15 @@ warns (ignored, behavior-neutral inside a single pod) or raises `security_opt`, `devices`) share one validator, `keys._validate_list`; the map-shaped ones (`labels`, `annotations`) and the pod-level `extra_hosts` share `keys.validate_map`. Both require every list element to be a string - and every mapping value to be a string, number, or null — a non-string - element or a non-scalar value (e.g. `cap_add: [{NET_ADMIN: true}]`, a list - entry that is itself a mapping) used to be accepted and `str()`'d/`repr()`'d - straight into the flag value instead of being rejected. + and every mapping value to be a string, number, boolean, or null — a + non-string element or a non-scalar value (e.g. `cap_add: [{NET_ADMIN: + true}]`, a list entry that is itself a mapping) is rejected rather than + `str()`'d/`repr()`'d straight into the flag value. A boolean mapping value + (`labels: {enabled: true}`) is deliberately accepted, not rejected: it is + normalized the way `docker compose config` normalizes it, rendered as the + lowercase string `true`/`false` (`keys._render_scalar`, shared by + `key_value_pairs` and `extra_host_pairs`) rather than Python's + `str(True) == "True"`. - **`image`:** a string; `image_for` (`compose2pod/emit.py`) reads it verbatim when the service has no `build`. A service must set at least one of `image` or `build`; a service with neither, or a non-string `image` while `build` is @@ -73,11 +89,14 @@ warns (ignored, behavior-neutral inside a single pod) or raises form `- KEY`. The key itself must be a list or mapping; any other shape (e.g. a bare string) raises at the gate. List elements must themselves be strings, and - mapping values must be a string, number, or null; the commonest violation - is the classic YAML slip of mixing list and map form - (`environment: [{KEY: value}]` instead of `- KEY=value`), which used to be - accepted and emit the literal `-e "{'KEY': 'value'}"` — both rules are - enforced by the shared `keys.validate_map` (`compose2pod/keys.py`). + mapping values must be a string, number, boolean, or null; the commonest + violation is the classic YAML slip of mixing list and map form + (`environment: [{KEY: value}]` instead of `- KEY=value`), rejected rather + than emitting the literal `-e "{'KEY': 'value'}"` — both rules are + enforced by the shared `keys.validate_map` (`compose2pod/keys.py`). A + boolean mapping value (`DEBUG: true`) is valid Compose and is normalized + like Docker: emitted as `-e DEBUG=true` (lowercase), not the Python repr + `-e DEBUG=True`. - **`env_file`:** a string or a list. Any other shape raises (`_validate_env_file`, `compose2pod/parsing.py`). Each list element must itself be a string; a non-string element (e.g. `env_file: [5]`) raises the @@ -126,7 +145,12 @@ warns (ignored, behavior-neutral inside a single pod) or raises `--ulimit nproc=65535`, podman sets soft = hard) or a `{soft, hard}` mapping (`nofile: {soft, hard}` → `--ulimit nofile=soft:hard`). A mapping value must have exactly `soft` and `hard`, each an int or string; other shapes are - rejected. (`sysctls`, by + rejected. A boolean scalar or a boolean `soft`/`hard` bound is rejected too + — `bool` is technically an `int` in Python, so a naive `isinstance(spec, + int | str)` would otherwise let one through and emit the literal + `--ulimit nofile=True`. Unlike `environment`'s boolean (normalized to a + string, see above), a boolean ulimit has no sensible Docker-equivalent + normalization, so it is rejected rather than coerced. (`sysctls`, by contrast, is pod-level rather than per-container — see the Pod-level options section below.) - **`labels`:** list (`- KEY=value` / `- KEY`) or mapping (`KEY: value` / `KEY:`), @@ -217,10 +241,19 @@ warns (ignored, behavior-neutral inside a single pod) or raises list-form `environment`/`depends_on`, or a sequence-concatenate key that is neither a list nor a scalar string. - **Divergences from Compose:** - - Only `environment` and `depends_on` accept list form for the - mapping-merge; the other mapping-merge keys (`labels`, `annotations`, - `extra_hosts`, `ulimits`, `healthcheck`) in list form on a merged side are - refused as an incompatible form rather than silently coerced. + - `environment` and `depends_on` accept list form directly in extends.py's + own merge (`_as_mapping`, `compose2pod/extends.py`); `extra_hosts` and + `healthcheck` do not — list form on a merged side is refused as an + incompatible form for those two. `labels`, `annotations`, and `ulimits` + instead route through their own `SERVICE_KEYS` merge policy + (`_merge_map`, `compose2pod/keys.py`), which uses the same + list-accepting `pairs_to_mapping` normalizer that `environment` uses — + so list form on a merged side *is* coerced to a mapping for these three, + not refused. Every one of these normalizers rejects a non-string list + element (e.g. `labels: [{BAD: "x"}]`) rather than laundering it into a + mapping key via `str()`: `pairs_to_mapping` (`compose2pod/keys.py`) is + the single function both paths share, so this rule holds identically + whichever merge route a key takes. - Short-form `volumes` are concatenated rather than merged by target path; podman resolves duplicate mounts at run time rather than compose2pod deduplicating them at generation time. @@ -614,7 +647,12 @@ short-form list element must itself be a string; a non-string element (e.g. `depends_on: [{db: {condition: service_healthy}}]`, the same list/map YAML slip that trips up `environment`/`command`) raises the same way, instead of crashing raw (`TypeError: unhashable type`) from inside `validate()` itself -when the list was passed to `dict.fromkeys`. +when the list was passed to `dict.fromkeys`. `extends.py`'s own +list-to-mapping normalization (`_as_mapping`, used when merging a +list-form `depends_on` across `extends`) enforces the identical +element-must-be-a-string check before `validate()` ever runs — `extends` +resolution happens ahead of the gate (see Extends, above), so without its +own check this same malformed input crashed raw there instead. ## YAML anchors and merge keys diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index e8daae4..9c91b27 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,5 +1,5 @@ --- -summary: validate() rejects every malformed structural-key shape found across three review rounds (image, command, entrypoint, environment, env_file, volumes, tmpfs, services, network aliases, healthcheck scalars and test, depends_on list entries, and every list/map key's non-scalar elements/values), emit rejects a malformed --artifact and guards referenced_variables on pod name, and Expand.value now gained a type guard so a leak past any per-key validator fails with UnsupportedComposeError instead of a raw crash, no matter which key is responsible. See "Round 3" below for what this closes as a class versus what remains scoped to this round's findings. +summary: validate() rejects every malformed structural-key shape found across four review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, added a document-wide non-string-mapping-key guard, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. See "Round 4" for the current, verified scope of what is closed versus what was not re-audited. --- # Design: Close the structural-key gate @@ -43,8 +43,11 @@ rejects a typed model *on the grounds that this invariant already holds*: > robust: a direct `emit_script(dict)` call on a *malformed* document now fails > with `UnsupportedComposeError`, not a raw crash. -That premise was still false after the first pass. This change makes it true -— it does not reopen the decision. +That premise was still false after the first pass, and, as Round 3 and then +Round 4 below each found on independent review, false again after every round +that believed it had closed it. This document does not reopen the decision — +see "Round 4: extends.py never got a turn at the gate" below for the current, +evidence-based answer to whether the premise now holds. ## Design @@ -187,6 +190,17 @@ validators were already hardened in earlier rounds — `ulimits`, `sysctls`, round's testing found a gap in them, but they were not exhaustively re-verified against the silent-corruption pattern either. +**This "regardless of which key or which future key" claim was itself +incomplete**, in two ways Round 4 found: `Expand` only guards values that +reach `emit_script()` through `validate()`'s normal path, but `cli.py` runs +`resolve_extends()` *before* `validate()` — `compose2pod/extends.py` had its +own raw-crash and silent-coercion surface that this round's chokepoint could +never reach, and was not touched at all in Round 3. Separately, a non-string +*mapping key* (as opposed to a non-string *value*) crashes several places +(`sorted()`, `str.startswith`) before a value ever reaches `Expand` — a +different mechanism the chokepoint does not cover by construction. See Round +4 below for both. + ### Testing (Round 3) TDD throughout: each finding above got a failing test confirming the raw @@ -200,3 +214,149 @@ elements), and `tests/test_pod.py` (`extra_hosts` element/value checks). `tests/conftest.py`'s `chats_compose` fixture and every `tests/integration/` scenario continue to pass unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just check-planning` all clean. + +## Round 4: extends.py never got a turn at the gate + +A fourth review found the structural reason the first three rounds kept +missing things: `cli.py` calls `resolve_extends(compose)` **before** +`validate(compose)`. Every hardening Round 1-3 did lives inside +`validate()`/`emit_script()`; `compose2pod/extends.py` runs ahead of both, on +the raw, un-gated document, and had never been touched on this branch. Two of +this round's five findings live there. + +| Finding | Before | Kind | +|---------|--------|------| +| `resolve_extends` on a `depends_on` list-of-mapping entry (`extends.py`, `_as_mapping`) | raw `TypeError: unhashable type: 'dict'` building `{dep: {} for dep in value}` — the identical input `graph.py` already rejected cleanly *without* `extends` | crash, ahead of the gate | +| `_extends_target`'s `sorted(unknown)` on a malformed `extends:` mapping with mixed-type keys | raw `TypeError: '<' not supported between instances of 'str' and 'int'` | crash, ahead of the gate (found auditing the module per this round's mandate) | +| A malformed child-side `labels`/`annotations`/`environment` list value merged across `extends` (`keys.pairs_to_mapping`) | the merge `str()`'d a non-string list element into a mapping *key*, producing a well-formed-looking mapping that `validate()` then accepted and `emit` rendered as the literal `--label "{'BAD': 'x'}"` | silent corruption laundered past the gate | +| A non-string mapping key anywhere `validate()` reads keys directly — top-level document, a service, a `healthcheck`, a `secrets`/`configs` name | `AttributeError`/`TypeError` from `k.startswith(...)`/`sorted(...)`/a compiled regex, all **inside `validate()` itself** — the gate crashing on its own input, not deferring to it | crash, inside the gate | +| `environment: {DEBUG: true}` (valid Compose; `docker compose config` normalizes to the string `"true"`) | emitted the Python repr `-e "DEBUG=True"` on `main`; over-corrected by an earlier fix on this branch into an outright rejection | silent corruption, then an over-rejection regression | +| `ulimits: {nofile: true}` / `{nofile: {soft: true, hard: 100}}` (`bool` is an `int` in Python) | emitted the literal `--ulimit "nofile=True"` | silent corruption | + +### `extends.py` hardening (C1) + +`_as_mapping`'s `depends_on` list branch gained the same per-element +string check `graph.py`'s own `depends_on` validator already has. Auditing +the rest of the module (as this round's finding required, not just the one +reproduction) found `_extends_target`'s `sorted(unknown)` had the identical +crash class one level up: a malformed `extends:` mapping with a non-string +key mixed among its unrecognized keys is unorderable. Fixed by sorting on +`repr` instead of the raw key. The rest of the module — `_as_list`, +`_merge`, `resolve_extends`'s own traversal — was walked the same way; +every other branch either normalizes without inspecting element types (safe +regardless of what they are) or already raises `UnsupportedComposeError` +cleanly on an incompatible shape. + +### `pairs_to_mapping` laundering (I1) + +`pairs_to_mapping` (`compose2pod/keys.py`) is the single function both +`extends.py`'s `environment`/`depends_on` normalization and +`labels`/`annotations`/`ulimits`' own `SERVICE_KEYS` merge policy +(`_merge_map`) route through. It now rejects a non-string list element +instead of `str()`-ing it into a mapping key, so the same malformed +child-side value that `validate()` already rejected outside `extends` is +rejected identically inside it — closing the gap the gate's normal path +never got a chance to see. + +### Non-string mapping keys (C2) + +`require_string_keys` (`compose2pod/keys.py`) is the one shared check, wired +in ahead of every place the gate walks a raw mapping's keys directly: the +top-level document, each service, each service's `healthcheck`, and each +top-level `secrets`/`configs` definition block. This round additionally +audited every other `sorted()`/`.startswith()`/`.fullmatch()` call site in +the package against a mapping built from raw compose input (a full grep +across `compose2pod/*.py`, not just the four sites the finding named) and +found the same crash one level deeper: `sorted(unknown)` in each +"reject unrecognized keys" check — a secret/config definition's own keys, a +long-form service/config reference's own keys (`compose2pod/stores.py`), and +`deploy`/`deploy.resources`/`deploy.resources.limits`/ +`deploy.resources.reservations` (`compose2pod/resources.py`) — all crash the +same way on a mixed-type key set. `require_string_keys` now guards those +five sites too. + +### Boolean map values normalize like Docker (I2 — design decision) + +**Ruling:** `environment`/`labels`/`annotations`/`extra_hosts` map values that +are booleans are valid Compose (`docker compose config` normalizes +`DEBUG: true` to the string `"true"`) and compose2pod now matches that +normalization rather than rejecting the value or leaking Python's +`str(True) == "True"`. `keys._render_scalar`, shared by `key_value_pairs` +(environment/labels/annotations) and `extra_host_pairs`, renders a bool as +lowercase `"true"`/`"false"`; `keys._validate_map_values` accepts a bool +alongside string/number/null. This reverses an over-rejection this branch +introduced earlier in its own history (list/map hardening tightened +`_validate_map_values` past what Compose actually allows) — the fix is +recorded here as the deliberate, permanent behavior, not a revert. +`pod._sysctl_pairs`'s pre-existing rejection of a boolean `sysctls` value is +untouched: `sysctls` has no Docker string-normalization semantic to match, so +there is nothing to normalize to, unlike the four map-value keys above. + +### Ulimits reject booleans instead of leaking them (I3) + +`bool` is an `int` in Python, so `_validate_ulimits`'s +`isinstance(spec, int | str)` accepted a boolean `nofile`/`soft`/`hard` and +`emit` rendered the literal `--ulimit "nofile=True"`. Unlike environment's +boolean (normalized above), a boolean ulimit has no sensible Docker +equivalent, so it is rejected — following the precedent +`stores._check_long_form_scalars` already set for `uid`/`gid`/`mode`. + +### Is the gate closed now? + +**The raw-crash half, evaluated against every path this round could find:** +`extends.py` (audited fully, not just the one reproduction), every mapping +key `validate()`/`graph.py`/`stores.py`/`resources.py` reads directly (a +grep-verified sweep of every `sorted()`/`.startswith()`/`.fullmatch()` call +site against compose-derived data), and the `Expand` chokepoint from Round 3 +(still structurally true for any value that reaches it) — no crash was found +in any of these on any input tried, including every reproduction this +round's finding described. This is **evidence of closure across everything +tested, not a proof of closure for every possible key**, exactly as Round 3's +"regardless of which key" claim turned out not to be: that claim held for +values flowing through `Expand`, but not for `extends.py` (a different +module entirely, run before the gate) or for mapping keys (a different +mechanism than the value checks `Expand` guards). No further such module or +mechanism is known to exist; none was found by this round's audit. + +**The silent-corruption half:** closed for the shapes this round found +(`labels`/`annotations`/`environment` list elements laundered through +`extends`, and boolean map values for `environment`/`labels`/`annotations`/ +`extra_hosts` now normalized instead of miscoerced). As Round 3 already +noted and this round did not revisit: keys whose validators were hardened in +earlier rounds (`ulimits`'s non-boolean shapes, `sysctls`, +`dns`/`dns_search`/`dns_opt`, `secrets`/`configs` definitions and +references, `deploy.resources`) were not re-audited for the +silent-corruption pattern specifically — Round 4 only re-audited them for +the *raw-crash* pattern (see C2 above), which is a narrower check. + +**What is true, stated plainly:** every reproduction given to this round — +and every one given to Rounds 1-3 before it — now raises +`UnsupportedComposeError` instead of crashing raw or reaching `emit` with a +corrupted value. The decision this file's premise depends on +(`decisions/2026-07-10-reject-parse-dont-validate.md`) is, as of this round, +supported by evidence rather than assumed; it has been wrong twice before on +an identical claim, so it should be read as "true for everything audited and +tested so far," not as a permanent guarantee that requires no further review +when a new structural key or a new pre-gate module is added. + +### Testing (Round 4) + +TDD throughout: every finding above, including the ones found by this +round's own audit rather than given as a reproduction, got a failing test +confirming the exact crash or laundering before the fix, then a green test +after. `tests/test_extends.py` (`depends_on`/`labels` laundering, +mixed-type `extends:` keys), `tests/test_keys.py` +(`pairs_to_mapping`/`require_string_keys`/boolean map-value acceptance), +`tests/test_parsing.py` (non-string top-level/service/healthcheck keys, +boolean `environment`), `tests/test_stores.py` and `tests/test_resources.py` +(mixed-type unknown-key sets), `tests/test_emit.py` and `tests/test_pod.py` +(boolean `environment`/`labels`/`annotations`/`extra_hosts` normalization). +The full acceptance-canary list from this round's brief (`environment` list +and every mapping-value shape including bool, `labels`/`annotations` list +and mapping incl. null, `extends` merging valid list-form +`environment`/`depends_on`, `depends_on` list and mapping, `ulimits` scalar +and soft/hard, `secrets`/`configs` with normal names) was re-verified +directly against `validate()`/`resolve_extends()` before committing, and +`tests/conftest.py`'s `chats_compose` fixture plus every `tests/integration/` +scenario pass unchanged. `just test-ci` at 100% line coverage, `just +lint-ci`, and `just check-planning` all clean. From ff04d9f48d20395f78fdc2660c179f5f8a84b6b4 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 09:38:46 +0300 Subject: [PATCH 32/43] fix: replace hand-placed non-string-key checks with a document-wide sweep The gate's non-string-mapping-key guard was six-turned-ten hand-placed require_string_keys() calls, wired into whichever mappings some other check happened to sorted()/startswith() -- never a walk of the document. Two holes followed: a mapping whose keys get f-string-interpolated straight into a flag value (environment/labels/annotations/sysctls/ extra_hosts/ulimits) leaked a YAML-1.1 bool's or int's Python repr into the emitted script instead of raising, and the services mapping's own keys (a service name) were never checked at all, reaching --add-host and --name verbatim. Replace the hand-placement with one recursive sweep, parsing._require_string_keys_deep, run once at the top of validate() before every other check: it walks the whole compose document and requires every mapping key, at every depth, to be a string, skipping recursion into x--prefixed subtrees (their arbitrary payloads are accepted and ignored by design). This closes both holes without any change to keys.py/pod.py, the modules that actually interpolate the keys, and covers any future structural key by construction rather than by someone remembering to wire in a call. Delete the three hand-placed calls the sweep provably subsumes (top-level document, service body, healthcheck -- all reached only through validate(), confirmed by deleting them and rerunning the suite). Keep the seven in resources.py/stores.py: their tests call validate_deploy()/stores.validate() directly, bypassing validate()'s sweep, and crash raw without their own guard. Confirmed resolve_extends() -- which cli.py runs ahead of validate() -- still passes every hostile-key shape through unchanged rather than crashing raw, so no extends.py change was needed this round. --- architecture/supported-subset.md | 64 ++++-- compose2pod/parsing.py | 67 ++++++- ...2026-07-13.10-close-structural-key-gate.md | 186 +++++++++++++++++- tests/test_cli.py | 26 +++ tests/test_extends.py | 40 ++++ tests/test_parsing.py | 107 ++++++++++ 6 files changed, 475 insertions(+), 15 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index ceb3a6a..aa2c114 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -17,17 +17,59 @@ warns (ignored, behavior-neutral inside a single pod) or raises - **Extension fields:** any key prefixed `x-` is accepted and ignored silently, per the Compose spec. This is what lets a document hold shared config in a top-level `x-*` block for reuse via YAML anchors. -- **Mapping keys must be strings.** PyYAML routinely produces a non-string - key — a bare `3:` parses as an int, and under YAML 1.1 a bare - `on:`/`off:`/`yes:`/`no:` parses as a bool. Every mapping-key consumer - downstream (`sorted()`, `str.startswith`, the secret/config name regex) - assumes `str` and crashes raw otherwise. `require_string_keys` - (`compose2pod/keys.py`) is the one shared check the gate runs before - reading any mapping's keys — applied to the top-level document itself, - each service, each service's `healthcheck`, and each top-level - `secrets`/`configs` definition block — so a non-string key raises - `UnsupportedComposeError` naming the offending key, regardless of which of - those four mappings it turned up in. +- **Every mapping key, at every depth of the whole document, must be a + string.** PyYAML routinely produces a non-string key — a bare `3:` parses + as an int, and under YAML 1.1 a bare `on:`/`off:`/`yes:`/`no:` parses as a + bool. `validate()` walks the entire compose document once, recursively, + before running any other check (`_require_string_keys_deep`, + `compose2pod/parsing.py`, built on `require_string_keys`, + `compose2pod/keys.py`) and rejects a non-string key at any depth — + `UnsupportedComposeError` names the offending key and its location. This + applies uniformly to every mapping in the document (the top-level document + itself, every service body and its nested structures, `healthcheck`, + `deploy` and its nested `resources`/`limits`/`reservations`, every + top-level `secrets`/`configs` definition and every service's long-form + reference, `ulimits` and its nested `{soft, hard}` mappings, and so on) — + it is not a list of specific mappings to keep in sync as new keys are + added. **Exception:** `x-`-prefixed keys' values are not walked, since + Compose extension fields legitimately hold arbitrary payloads (e.g. YAML + anchor sources reused via `<<:`) that compose2pod accepts and ignores by + design; the `x-` key itself is still checked (trivially — its own name has + to look like `x-...`). + + Two distinct downstream consumer classes motivate rejecting a non-string + key up front rather than one key at a time: mapping-key readers that + crash raw otherwise (`sorted()`, `str.startswith`, the secret/config name + regex), and mapping-key consumers that don't crash but f-string- + interpolate the key straight into a flag value, silently leaking a bool's + or int's Python repr into the emitted script instead + (`keys.key_value_pairs` — `environment`/`labels`/`annotations`, + `keys.extra_host_pairs` — `extra_hosts`, `keys._ulimit_args` — `ulimits`, + `pod._sysctl_pairs` — `sysctls`). The second class is silent corruption, + not a crash, and is why a document-wide walk replaced hand-placing a check + at each mapping some other symptom happened to point at. + + This is a deliberate divergence from Docker: `environment: {3306: db}` is + valid Compose and Docker accepts it, but compose2pod refuses it. Docker + parses Compose as YAML 1.2, where a bare `3306`/`on`/`off` stays the + string it looks like, so Docker never observes a non-string key at all. + Normalizing Python's `int`/`bool` back into a key string here would not + reproduce that: unlike a boolean *value* (`DEBUG: true`, normalized to the + string `"true"` like Docker does — see `_render_scalar` below), a boolean + or int *key* has no single correct string form to normalize to (`True` → + `"on"`? `"true"`? `"True"`?). A non-string key is a YAML-1.1 accident, not + intentional Compose; anyone who means the literal string `on` writes + `"on"`. + + A handful of module entry points that are also called directly (not only + reached through `validate()`) keep their own `require_string_keys` call in + addition to the document-wide sweep, as defense at their own boundary: + `resources.validate_deploy`'s four checks (`deploy` and its nested + `resources`/`limits`/`reservations`) and `stores.py`'s three checks (a + secret/config definition's keys, a long-form reference's keys, and the + top-level `secrets`/`configs` block's own keys). These are redundant only + when reached through `validate()`; each is still load-bearing for a caller + that invokes that function directly. - Everything else raises. ## Service keys diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 5cf09b1..31bb995 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -27,7 +27,9 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: if not isinstance(healthcheck, dict): msg = f"service {name!r}: healthcheck must be a mapping" raise UnsupportedComposeError(msg) - require_string_keys(f"service {name!r}: healthcheck", healthcheck) + # Non-string healthcheck keys are already rejected by validate()'s + # document-wide _require_string_keys_deep sweep, which runs before this + # function is ever reached. for key in sorted(healthcheck): if key.startswith("x-"): continue @@ -156,7 +158,9 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo if not isinstance(svc, dict): msg = f"service {name!r} must be a mapping" raise UnsupportedComposeError(msg) - require_string_keys(f"service {name!r}", svc) + # Non-string service-body keys are already rejected by validate()'s + # document-wide _require_string_keys_deep sweep, which runs before this + # function is ever reached. warnings: list[str] = [] for key in sorted(svc): if key.startswith("x-"): @@ -184,6 +188,59 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo return warnings +def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Require every mapping key, at every depth of `node`, to be a string. + + PyYAML routinely produces a non-string key: a bare `3:` parses as an int, + and under YAML 1.1 a bare `on:`/`off:`/`yes:`/`no:` parses as a bool. + Two distinct downstream consumer classes assume `str` keys and break + otherwise: + + - Mapping-key readers that crash raw: `sorted()`, `str.startswith`, the + secret/config name regex. + - Mapping-key consumers that don't crash but f-string-interpolate the key + straight into a flag value, silently leaking the Python repr of a bool + or int into the emitted script (`keys.key_value_pairs` -- environment/ + labels/annotations, `keys.extra_host_pairs`, `keys._ulimit_args`, + `pod._sysctl_pairs`). This class is corruption, not a crash, and is the + one the old hand-placed `require_string_keys` call sites never covered + -- they were only wired into mappings whose keys get sorted or + startswith'd, never the ones whose keys get f-string'd into a flag. + + Walking the whole document once, recursively, from `validate()`'s entry + point closes both classes uniformly regardless of which mapping -- + existing or future -- a non-string key turns up in, rather than trusting + every new structural key's validator to remember to call + `require_string_keys` by hand. + + `x-`-prefixed keys' *values* are not recursed into: Compose extension + fields legitimately hold arbitrary user payloads (e.g. anchor sources + reused via YAML merge keys), and compose2pod accepts and ignores their + contents by design. The `x-` key itself is still checked, trivially: it + is a string by construction (its own name has to look like `x-...`). + + Rejecting a non-string key is a deliberate divergence from Docker for + map-typed *keys* specifically (Docker accepts `environment: {3306: db}`): + Compose is parsed as YAML 1.2, where a bare `on`/`off`/`3306` stays a + string, so Docker never observes a non-string key at all. Normalizing + Python's `bool`/`int` back to a string here would not reproduce that -- + `True` has no single correct string form (`"on"`? `"true"`? `"True"`?) + the way a boolean *value* does (see `keys._render_scalar`, which mirrors + Docker's own `true`/`false` value normalization). A non-string key is a + YAML-1.1 accident, not intentional Compose; anyone who means the literal + string `on` writes `"on"`. + """ + if isinstance(node, dict): + require_string_keys(where, node) + for key, value in node.items(): + if key.startswith("x-"): + continue + _require_string_keys_deep(f"{where}.{key}", value) + elif isinstance(node, list): + for item in node: + _require_string_keys_deep(where, item) + + def _validate_depends_on(services: dict[str, Any]) -> None: """Cross-service depends_on checks: known conditions, service_healthy needs a healthcheck.""" for name, svc in services.items(): @@ -205,7 +262,11 @@ def validate(compose: dict[str, Any]) -> list[str]: if not isinstance(compose, dict): msg = f"compose document must be a mapping, got {type(compose).__name__}" raise UnsupportedComposeError(msg) - require_string_keys("compose document", compose) + # Runs first, ahead of every other check: every later check that reads a + # mapping's keys directly (sorted(), .startswith()) or f-string- + # interpolates one into a flag value can assume every key in the document + # is a string from this point on. + _require_string_keys_deep("compose document", compose) warnings: list[str] = [] unknown_top = {k for k in compose if k not in SUPPORTED_TOP_LEVEL_KEYS and not k.startswith("x-")} if unknown_top: diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index 9c91b27..04894a9 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,5 +1,5 @@ --- -summary: validate() rejects every malformed structural-key shape found across four review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, added a document-wide non-string-mapping-key guard, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. See "Round 4" for the current, verified scope of what is closed versus what was not re-audited. +summary: validate() rejects every malformed structural-key shape found across five review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. Round 4's non-string-mapping-key guard was ten hand-placed require_string_keys() calls across three modules, not document-wide as claimed; Round 5 replaced it with one recursive sweep (parsing._require_string_keys_deep) that walks the whole document ahead of every other check, closing two holes the hand-placed calls missed by construction (a non-string key f-string-interpolated into a flag value, and service names). See "Round 5" for the current, verified scope of what is closed versus what was not re-audited. --- # Design: Close the structural-key gate @@ -360,3 +360,187 @@ directly against `validate()`/`resolve_extends()` before committing, and `tests/conftest.py`'s `chats_compose` fixture plus every `tests/integration/` scenario pass unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just check-planning` all clean. + +**Correction (see Round 5 below):** C2's "document-wide" framing above was +wrong. `require_string_keys` was ten hand-placed calls across three +modules, wired into exactly the mappings whose keys some other check +already `sorted()`s or +`.startswith()`s — never a walk of the document. Two holes followed +directly from that: a mapping whose keys are f-string-interpolated straight +into a flag value (never sorted or startswith'd, so never a candidate for +hand-placement) and the `services` mapping's own keys (nobody had wired a +call in there at all). + +## Round 5: the ten hand-placed calls were the bug behind the bug + +A fifth review found that Round 4's C2 fix was hand enumeration wearing a +"document-wide" label. `require_string_keys` guards ten specific mappings a +developer noticed were unsafe; it does not walk the document. Two classes of +mapping were never candidates for hand-placement in the first place, because +neither one matches the pattern ("this mapping's keys get `sorted()`/ +`.startswith()`'d") that motivated adding a call at the other nine sites: + +| Finding | Before | Kind | +|---------|--------|------| +| `environment`/`labels`/`annotations`/`sysctls`/`extra_hosts`/`ulimits` map with a non-string key (YAML 1.1: a bare `on:`/`off:`/`yes:`/`no:` is a Python `bool`; a bare `3306:` is an `int`) | accepted; the key is f-string-interpolated straight into the flag value -- `-e "True=1"`, `--label "True=v"`, `--annotation "False=v"`, `--sysctl "True=1"`, `--add-host "True:1.2.3.4"`, `--ulimit "True=100"` | silent corruption | +| a non-string key in the `services` mapping itself (a service *name*) | accepted; reaches `--add-host :127.0.0.1` (via `graph.hostnames`, document-wide, independent of the target's dependency closure) and `podman run --name test-pod-` | silent corruption | + +### Why F1 was invisible to the C2 fix's own method + +`key_value_pairs` (`compose2pod/keys.py`, backs `environment`/`labels`/ +`annotations`), `extra_host_pairs` (`compose2pod/keys.py`), `_ulimit_args` +(`compose2pod/keys.py`), and `pod._sysctl_pairs` (`compose2pod/pod.py`) all +read a mapping's keys and interpolate them into an f-string (`f"{key}=..."`, +`f"{host}:..."`, `str(key)`) rather than `sorted()` or `.startswith()`ing +them. C2's audit method was "grep every `sorted()`/`.startswith()`/ +`.fullmatch()` call site against compose-derived data" -- a real, useful +sweep, but one that finds only the raw-crash half of the defect class by +construction. It could never have found F1, because f-string interpolation +of a non-string key doesn't crash at all; it produces a *wrong but +well-formed-looking* flag value, exit 0. This is the same +crash-versus-silent-corruption split the rest of this document already +tracks for map *values* (see "Scope of the 'closed as a class' claim" under +Round 3) -- Round 4 closed it for values but reintroduced exactly that gap +for keys, in the one guard whose stated purpose was closing it for keys. + +### Why F2 was invisible to hand-placement as a method + +Nobody had reason to add a `require_string_keys` call for the `services` +mapping specifically, because nothing reads `services`'s keys with +`sorted()`/`.startswith()`/a regex either -- `graph.hostnames` does +`list(services)` and `validate()` does `services.items()`, neither of which +raises on a non-string key. The service-name check was missing not because +an audit missed one mapping among many, but because hand-placement as a +method only ever finds mappings some other symptom (a crash) already points +at. + +### The fix: one recursive sweep, not more hand-placement + +`_require_string_keys_deep` (`compose2pod/parsing.py`) replaces the three +hand-placed `require_string_keys` calls that lived inside `validate()`'s own +call graph (top-level document, each service body, each `healthcheck`) with +one function, called once at the top of `validate()` before every other +check: it walks the whole document depth-first -- every dict's keys via +`require_string_keys`, every dict's values and every list's items +recursively -- and requires every key at every depth to be a string, +regardless of what mapping it turns up in or what that mapping's keys get +used for downstream. `x-`-prefixed keys' *values* are skipped (not +recursed into) rather than checked, since Compose extension fields +legitimately hold arbitrary payloads (e.g. anchor sources reused via `<<:`) +that compose2pod accepts and ignores by design; the `x-` key itself is +still checked, trivially (its own name has to look like `x-...`). + +Ahead-of-the-gate ordering (the Round 4 lesson) was re-checked directly +against this fix: `cli.py` still runs `resolve_extends()` before +`validate()`, so before wiring the sweep in, every hostile-key shape this +round could construct (a non-string service name, a non-string key inside a +service body with no `extends` involved, a non-string key inside an +`extends` base body, and a non-string key merged into `environment` across +`extends`) was tried directly against `resolve_extends()` in isolation. +None crashed -- Round 4's C1 fix (`_extends_target`'s repr-sorted +unknown-key check) and the fact that `extends.py`'s own traversal never +sorts or startswith's a raw mapping's keys already covered this path, so no +further `extends.py` change was needed this round. That absence-of-crash was +verified empirically (`tests/test_extends.py::TestNonStringKeysAheadOfTheGate`), +not assumed. + +### Are the ten hand-placed calls now dead code? + +Three of the ten -- the top-level document, each service body, and each +`healthcheck` -- guarded mappings the sweep now checks before `validate()` +ever reaches them; they were deleted, and every test that covered them +(`tests/test_parsing.py`'s non-string top-level/service/healthcheck-key +tests) still passes against the sweep's message, which reuses +`require_string_keys` under the hood and so produces the identical `key + must be a string` suffix. + +The other seven -- `deploy`/`deploy.resources`/`deploy.resources.limits`/ +`deploy.resources.reservations` (`compose2pod/resources.py`, four calls) and +each `secrets`/`configs` definition and long-form reference +(`compose2pod/stores.py`, three calls) -- were **not** deleted. Deleting +them was tried directly (verifying the "IF it subsumes" condition rather +than assuming it): +`tests/test_resources.py`'s and `tests/test_stores.py`'s mixed-type-key +tests call `validate_deploy(...)` and `stores.validate(...)` directly, +bypassing `parsing.validate()` and its sweep entirely, and failed with the +raw `TypeError` the hand-placed calls exist to prevent. Those two functions +are public module entry points with their own contract independent of +whichever caller reaches them -- `parsing.validate()` is one caller, not the +only one -- so their own guards stay. + +### Is the gate closed now? + +Restating Round 4's closing paragraph with this round folded in: every +reproduction given to this round -- and every one given to Rounds 1-4 before +it -- now raises `UnsupportedComposeError` instead of crashing raw or +reaching `emit` with a corrupted value. Two things are different in kind +from Round 4's answer, not just in degree: + +- **The non-string-key guard is now actually document-wide**, in the literal + sense Round 4 claimed but didn't build: a new structural key added + anywhere in the document tree in the future gets the check for free, + without anyone remembering to wire in a `require_string_keys` call for it. + This closes the specific failure mode that produced both this round's + findings and Round 4's over-claim about itself. +- **The silent-corruption half of the non-string-*key* class is closed**, + not just audited for the raw-crash half. Round 4's C2 explicitly scoped + itself to "every `sorted()`/`.startswith()`/`.fullmatch()` call site" -- + a method that structurally cannot find an f-string-interpolation site. + The recursive sweep isn't scoped to a consumer pattern at all, so this + distinction no longer matters for non-string *keys* (it still matters for + non-string *values*, which the sweep does not touch and which remain + closed per-validated-shape, as Round 3 left them). + +What remains true, unchanged from Round 4: this is evidence of closure +across everything tested, not a proof of closure for every possible input. +No further gap in the non-string-mapping-key class is known; none was found +by this round's audit, which included re-deriving every one of Round 4's +ten hand-placed call sites from first principles (why was each one added? +what pattern was it protecting against? does the sweep's unconditional walk +cover that pattern?) rather than trusting Round 4's own account of its scope. + +### Documentation debt this round found and paid down + +`architecture/supported-subset.md`'s "Mapping keys must be strings" bullet +scoped itself to the same four mappings Round 4's C2 wired calls into, +omitting the two mappings this round's findings live in and never +mentioning the f-string-interpolation consumer class at all -- the third +overclaim on this branch (after the "document-wide" framing above and an +earlier one Round 4 itself corrected, see "Boolean map values normalize +like Docker" and "Ulimits reject booleans instead of leaking them"). Fixed +in the same commit as the sweep: the bullet now names the sweep, its actual +scope (the whole document, not four mappings), the `x-` exception, the +f-string-interpolation consumer class, and the deliberate divergence from +Docker for non-string map *keys* specifically (Docker/YAML 1.2 never +produces one; `environment: {3306: db}` is valid Compose and is refused +here rather than guessed at, since normalizing Python's `int`/`bool` back to +a key string has no single correct answer the way normalizing a boolean +*value* does). + +### Testing (Round 5) + +TDD: `tests/test_parsing.py::TestRequireStringKeysDeep` reproduces both +findings (`environment`/`labels`/`annotations`/`sysctls`/`extra_hosts`/ +`ulimits`, including the nested `ulimits..{soft,hard}` mapping, and a +non-string service name) against `validate()` directly, confirms a deeper +structural key (`deploy.resources.limits`) and a top-level store definition +name are caught too (proving the sweep isn't scoped to the ten old +hand-placed sites), confirms `x-` subtrees at both the top level and inside +a service keep arbitrary non-string-keyed payloads accepted, and confirms +every acceptance case the brief called out by name still validates clean +(`ulimits` soft/hard, `sysctls` mapping, `extra_hosts` list and map forms, +`environment` null/int/float/bool values, dotted/dashed/underscored service +names). `tests/test_extends.py::TestNonStringKeysAheadOfTheGate` confirms +`resolve_extends()` still passes every one of these hostile shapes through +unchanged rather than crashing raw, ahead of the gate. `tests/test_cli.py` +adds one end-to-end case per finding (YAML input through `main()`, matching +the exact reproduction given to this round) plus one confirming a +non-string key survives `extends` merging and is still rejected cleanly by +the full pipeline. Deleting each of the ten old hand-placed +`require_string_keys` calls was tried directly, one at a time, to decide +"subsumed vs. still load-bearing" by evidence rather than inspection; the +seven that broke a test (`resources.py`'s four deploy-block calls, +`stores.py`'s three secret/config calls) were restored. `tests/conftest.py`'s +`chats_compose` fixture and every `tests/integration/` scenario pass +unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just +check-planning` all clean. diff --git a/tests/test_cli.py b/tests/test_cli.py index 89ff259..6dbe7c7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -221,6 +221,32 @@ def test_extends_non_dict_base_is_clean_error( assert rc == EXIT_USAGE_ERROR assert "must be a mapping" in capsys.readouterr().err + def test_non_string_key_survives_extends_and_is_rejected_cleanly( + self, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch + ) -> None: + # resolve_extends() runs before validate(); confirms the merged + # non-string key reaches validate()'s sweep and fails clean (exit 2) + # instead of crashing raw somewhere in extends.py. + yaml_text = ( + "services:\n" + " base:\n image: j\n environment:\n A: '1'\n" + " app:\n extends: {service: base}\n environment:\n on: '2'\n" + ) + rc = run_main(yaml_text, ["--target", "app", "--image", "i", "--format", "yaml"], monkeypatch) + assert rc == EXIT_USAGE_ERROR + assert "key True must be a string" in capsys.readouterr().err + + def test_non_string_service_name_rejected_cleanly( + self, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch + ) -> None: + # F2: a non-string service name reached --add-host/--name verbatim + # (e.g. `podman run --name test-pod-True ...`) instead of being + # rejected at the gate. + yaml_text = "services:\n app:\n image: i\n on:\n image: j\n" + rc = run_main(yaml_text, ["--target", "app", "--image", "i", "--format", "yaml"], monkeypatch) + assert rc == EXIT_USAGE_ERROR + assert "compose document.services: key True must be a string" in capsys.readouterr().err + def test_yaml_anchor_extension_fields_convert( self, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_extends.py b/tests/test_extends.py index 125e61e..a70c217 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -265,3 +265,43 @@ def test_incompatible_structural_concat_form_is_refused(self) -> None: } with pytest.raises(UnsupportedComposeError, match="cannot merge 'env_file' across incompatible forms"): resolve_extends(doc) + + +class TestNonStringKeysAheadOfTheGate: + """resolve_extends() runs before validate()'s document-wide string-key sweep. + + validate() closes non-string mapping keys as a class (see + parsing._require_string_keys_deep), but it never gets a turn if + resolve_extends() crashes raw on the same input first. None of these + hostile shapes involve a value resolve_extends interprets as anything + other than an opaque dict key or dict value, so each should pass through + unchanged for validate() to reject afterward -- not crash here. + """ + + def test_non_string_service_name_passes_through(self) -> None: + doc = {"services": {True: {"image": "j"}, "app": {"image": "i"}}} + assert resolve_extends(doc) == doc + + def test_non_string_key_in_non_extending_service_body_passes_through(self) -> None: + doc = {"services": {"app": {"image": "i", 3: "x"}}} + assert resolve_extends(doc) == doc + + def test_non_string_key_in_extends_base_body_passes_through(self) -> None: + doc = { + "services": { + "base": {"image": "j", True: "y"}, + "app": {"extends": {"service": "base"}, "image": "i"}, + } + } + merged = resolve_extends(doc) + assert merged["services"]["app"][True] == "y" + + def test_non_string_key_merged_into_environment_across_extends_passes_through(self) -> None: + doc = { + "services": { + "base": {"image": "j", "environment": {"A": "1"}}, + "app": {"extends": {"service": "base"}, "environment": {3: "x"}}, + } + } + merged = resolve_extends(doc) + assert merged["services"]["app"]["environment"] == {"A": "1", 3: "x"} diff --git a/tests/test_parsing.py b/tests/test_parsing.py index d2aca58..1c9a93a 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -589,4 +589,111 @@ def test_entrypoint_list_with_non_string_entry_rejected_at_gate(self) -> None: def test_command_and_entrypoint_list_of_strings_still_accepted(self) -> None: assert validate({"services": {"app": {"image": "x", "command": ["run", "me"]}}}) == [] + + +class TestRequireStringKeysDeep: + """validate()'s document-wide recursive non-string-mapping-key sweep. + + F1: a non-string key in a mapping that gets f-string-interpolated into a + flag value (environment/labels/annotations/sysctls/extra_hosts/ulimits) + used to leak Python's bool/int repr into the emitted script rather than + crash -- the hand-placed require_string_keys call sites never covered + these because none of them sorted() or startswith()'d that key. F2: a + non-string *service name* (a key of the `services` mapping itself) was + never checked at all. Both are closed by one recursive sweep instead of + hand-placing more calls -- see _require_string_keys_deep. + """ + + def test_environment_non_string_key_rejected(self) -> None: + # YAML 1.1: a bare `on:` parses as the Python bool True. + compose = {"services": {"app": {"image": "x", "environment": {True: "1"}}}} + with pytest.raises(UnsupportedComposeError, match=r"environment: key True must be a string"): + validate(compose) + + def test_labels_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "labels": {True: "v"}}}} + with pytest.raises(UnsupportedComposeError, match=r"labels: key True must be a string"): + validate(compose) + + def test_annotations_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "annotations": {False: "v"}}}} + with pytest.raises(UnsupportedComposeError, match=r"annotations: key False must be a string"): + validate(compose) + + def test_sysctls_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "sysctls": {True: 1}}}} + with pytest.raises(UnsupportedComposeError, match=r"sysctls: key True must be a string"): + validate(compose) + + def test_extra_hosts_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "extra_hosts": {True: "1.2.3.4"}}}} + with pytest.raises(UnsupportedComposeError, match=r"extra_hosts: key True must be a string"): + validate(compose) + + def test_ulimits_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "ulimits": {True: 100}}}} + with pytest.raises(UnsupportedComposeError, match=r"ulimits: key True must be a string"): + validate(compose) + + def test_ulimits_nested_soft_hard_non_string_key_rejected(self) -> None: + compose = {"services": {"app": {"image": "x", "ulimits": {"nofile": {True: 1024, "hard": 4096}}}}} + with pytest.raises(UnsupportedComposeError, match=r"ulimits\.nofile: key True must be a string"): + validate(compose) + + def test_non_string_service_name_rejected(self) -> None: + # F2: the `services` mapping's own keys (service names) were never + # checked -- an int/bool name reaches --add-host and --name verbatim. + compose = {"services": {"app": {"image": "i"}, True: {"image": "j"}}} + with pytest.raises(UnsupportedComposeError, match=r"compose document\.services: key True must be a string"): + validate(compose) + + def test_deploy_resources_limits_non_string_key_via_full_pipeline(self) -> None: + # A deeper structural key, reached only by walking the whole + # document -- confirms the sweep isn't limited to the sites the old + # hand-placed calls were wired into. + compose = {"services": {"app": {"image": "x", "deploy": {"resources": {"limits": {True: "1"}}}}}} + with pytest.raises(UnsupportedComposeError, match=r"deploy\.resources\.limits: key True must be a string"): + validate(compose) + + def test_secret_definition_non_string_name_via_full_pipeline(self) -> None: + compose = {"services": {"app": {"image": "x"}}, "secrets": {1: {"file": "./a"}}} + with pytest.raises(UnsupportedComposeError, match=r"compose document\.secrets: key 1 must be a string"): + validate(compose) + + def test_top_level_extension_subtree_with_non_string_keys_is_accepted(self) -> None: + # x- extension fields legitimately hold arbitrary payloads (e.g. + # YAML-anchor sources); their contents are never walked. + compose = {"x-anchors": {1: {"a": "b"}}, "services": {"app": {"image": "x"}}} + assert validate(compose) == [] + + def test_service_level_extension_subtree_with_non_string_keys_is_accepted(self) -> None: + compose = {"services": {"app": {"image": "x", "x-meta": {True: [1, {2: "z"}]}}}} + assert validate(compose) == [] + + def test_ulimits_soft_hard_mapping_still_accepted(self) -> None: + compose = {"services": {"app": {"image": "x", "ulimits": {"nofile": {"soft": 1024, "hard": 4096}}}}} + assert validate(compose) == [] + + def test_sysctls_mapping_still_accepted(self) -> None: + compose = {"services": {"app": {"image": "x", "sysctls": {"net.core.somaxconn": 1024}}}} + assert any("pod-wide" in w for w in validate(compose)) + + def test_extra_hosts_map_and_list_forms_still_accepted(self) -> None: + assert validate({"services": {"app": {"image": "x", "extra_hosts": {"db": "10.0.0.1"}}}}) + assert validate({"services": {"app": {"image": "x", "extra_hosts": ["db:10.0.0.1"]}}}) + + def test_environment_null_int_float_bool_values_still_accepted(self) -> None: + compose = { + "services": { + "app": { + "image": "x", + "environment": {"A": None, "B": 1, "C": 1.5, "D": True}, + } + } + } + assert validate(compose) == [] + + def test_normal_service_names_with_dots_dashes_underscores_accepted(self) -> None: + compose = {"services": {"app.v1": {"image": "x"}, "app-two": {"image": "y"}, "app_3": {"image": "z"}}} + assert validate(compose) == [] assert validate({"services": {"app": {"image": "x", "entrypoint": ["run", "me"]}}}) == [] From 36e816e053e67644787eca0f3a12d85eb912fa0b Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 11:29:48 +0300 Subject: [PATCH 33/43] fix: sweep a service named x-* like any other service The non-string-key sweep's x- skip is syntactic (key.startswith("x-")) but its rationale is semantic ("a subtree we ignore"); on the services mapping's own keys those diverge, since a service name is an identifier, not an extension-field marker. validate() iterates services.items() with no x- filter, so a service literally named x-web is a real service whose body was escaping the sweep entirely -- reaching a raw TypeError crash via sorted(svc)/sorted(healthcheck), or silently leaking a YAML-1.1 bareword key's Python repr into the emitted script (`-e "True=1"`) with validate() returning clean. _sweep_document/_sweep_service replace the single _require_string_keys_deep call with an orchestration that always sweeps every services/secrets/configs entry by name, regardless of what the name looks like -- the same fix applies to store definitions, whose names aren't excluded from `x-` either. Also restores the two require_string_keys calls Round 5 deleted from _validate_service/_validate_service_healthcheck as belt-and-braces, matching the "own contract independent of caller" reasoning already used for resources.validate_deploy/stores.validate. --- compose2pod/parsing.py | 86 +++++++++++++++++++++++++++++++++++------- tests/test_cli.py | 13 +++++++ tests/test_parsing.py | 44 +++++++++++++++++++++ 3 files changed, 129 insertions(+), 14 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 31bb995..9bdba5f 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -27,9 +27,11 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: if not isinstance(healthcheck, dict): msg = f"service {name!r}: healthcheck must be a mapping" raise UnsupportedComposeError(msg) - # Non-string healthcheck keys are already rejected by validate()'s - # document-wide _require_string_keys_deep sweep, which runs before this - # function is ever reached. + # Redundant with validate()'s _sweep_document when reached through + # validate() (the only caller today), but this function has its own + # contract as a module entry point -- belt-and-braces, not load-bearing + # only by luck of the current call graph. + require_string_keys(f"service {name!r}: healthcheck", healthcheck) for key in sorted(healthcheck): if key.startswith("x-"): continue @@ -158,9 +160,11 @@ def _validate_service(name: str, svc: Any) -> list[str]: # noqa: ANN401 - Compo if not isinstance(svc, dict): msg = f"service {name!r} must be a mapping" raise UnsupportedComposeError(msg) - # Non-string service-body keys are already rejected by validate()'s - # document-wide _require_string_keys_deep sweep, which runs before this - # function is ever reached. + # Redundant with validate()'s _sweep_document when reached through + # validate() (the only caller today), but this function has its own + # contract as a module entry point -- belt-and-braces, not load-bearing + # only by luck of the current call graph. + require_string_keys(f"service {name!r}", svc) warnings: list[str] = [] for key in sorted(svc): if key.startswith("x-"): @@ -207,17 +211,17 @@ def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - -- they were only wired into mappings whose keys get sorted or startswith'd, never the ones whose keys get f-string'd into a flag. - Walking the whole document once, recursively, from `validate()`'s entry - point closes both classes uniformly regardless of which mapping -- - existing or future -- a non-string key turns up in, rather than trusting - every new structural key's validator to remember to call - `require_string_keys` by hand. - `x-`-prefixed keys' *values* are not recursed into: Compose extension fields legitimately hold arbitrary user payloads (e.g. anchor sources reused via YAML merge keys), and compose2pod accepts and ignores their contents by design. The `x-` key itself is still checked, trivially: it - is a string by construction (its own name has to look like `x-...`). + is a string by construction (its own name has to look like `x-...`). This + skip is only ever correct for a key that plays the role of "extension + field name" in the node being walked -- callers (`_sweep_document`/ + `_sweep_service`) are responsible for never handing this function a + mapping whose keys are *identifiers* (a service name, a store name) + rather than content keys, since an identifier starting with `x-` is not + an extension field. Rejecting a non-string key is a deliberate divergence from Docker for map-typed *keys* specifically (Docker accepts `environment: {3306: db}`): @@ -241,6 +245,60 @@ def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - _require_string_keys_deep(where, item) +def _sweep_service(name: str, svc: dict[str, Any]) -> None: + """Require every mapping key in one service body to be a string, at every depth. + + The service's own top-level keys are always checked (`require_string_keys` + below), regardless of what the service's *name* looks like -- this + function is only ever called with the real body of a real service, + including one literally named `x-web` (see `_sweep_document`). Only the + service's *own* `x-`-prefixed keys are skipped, same as everywhere else + `x-` is treated as an extension-field marker. + """ + where = f"service {name!r}" + require_string_keys(where, svc) + for key, value in svc.items(): + if key.startswith("x-"): + continue + _require_string_keys_deep(f"{where}.{key}", value) + + +def _sweep_document(compose: dict[str, Any]) -> None: + """Require every mapping key, at every depth of `compose`, to be a string. + + Behaves like calling `_require_string_keys_deep("compose document", + compose)` directly, with one correction: the `services` mapping's own + keys (service *names*), and each top-level `secrets`/`configs` + definition's own name, are always swept regardless of what the name + looks like, rather than treated as extension-field markers when they + start with `x-`. `validate()` iterates `services.items()` with no `x-` + filter, so a service literally named `x-web` is a real service, not an + extension field -- conflating a NAME with a content key let such a + service's whole body escape the sweep entirely (see `_sweep_service`). + `stores._validate_def` accepts a store name matching + `[a-zA-Z0-9][a-zA-Z0-9_.-]*`, which does not exclude one starting `x-` + either, so the same correction applies there. + """ + require_string_keys("compose document", compose) + services = compose.get("services") + if isinstance(services, dict): + require_string_keys("compose document.services", services) + for name, svc in services.items(): + if isinstance(svc, dict): + _sweep_service(name, svc) + for top_key in ("secrets", "configs"): + defs = compose.get(top_key) + if isinstance(defs, dict): + require_string_keys(f"compose document.{top_key}", defs) + for def_name, definition in defs.items(): + if isinstance(definition, dict): + _require_string_keys_deep(f"compose document.{top_key}.{def_name}", definition) + for key, value in compose.items(): + if key in ("services", "secrets", "configs") or key.startswith("x-"): + continue + _require_string_keys_deep(f"compose document.{key}", value) + + def _validate_depends_on(services: dict[str, Any]) -> None: """Cross-service depends_on checks: known conditions, service_healthy needs a healthcheck.""" for name, svc in services.items(): @@ -266,7 +324,7 @@ def validate(compose: dict[str, Any]) -> list[str]: # mapping's keys directly (sorted(), .startswith()) or f-string- # interpolates one into a flag value can assume every key in the document # is a string from this point on. - _require_string_keys_deep("compose document", compose) + _sweep_document(compose) warnings: list[str] = [] unknown_top = {k for k in compose if k not in SUPPORTED_TOP_LEVEL_KEYS and not k.startswith("x-")} if unknown_top: diff --git a/tests/test_cli.py b/tests/test_cli.py index 6dbe7c7..1d79230 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -258,6 +258,19 @@ def test_yaml_anchor_extension_fields_convert( assert rc == 0 assert "podman pod create" in out.out + def test_non_string_key_from_x_block_anchor_merged_into_service_is_rejected( + self, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch + ) -> None: + # PyYAML resolves anchors/merge keys at load time, so content whose + # *source* lived in a skipped x- block lands in the service body as + # ordinary content once merged -- it must still be swept there. + yaml_text = ( + "x-common: &common\n environment:\n on: 1\nservices:\n app:\n image: alpine\n <<: *common\n" + ) + rc = run_main(yaml_text, ["--target", "app", "--image", "i", "--format", "yaml"], monkeypatch) + assert rc == EXIT_USAGE_ERROR + assert "key True must be a string" in capsys.readouterr().err + class TestModuleEntrypoint: def test_python_m_runs(self, chats_compose: dict) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 1c9a93a..5463275 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -697,3 +697,47 @@ def test_normal_service_names_with_dots_dashes_underscores_accepted(self) -> Non compose = {"services": {"app.v1": {"image": "x"}, "app-two": {"image": "y"}, "app_3": {"image": "z"}}} assert validate(compose) == [] assert validate({"services": {"app": {"image": "x", "entrypoint": ["run", "me"]}}}) == [] + + +class TestSweepServiceNamedExtensionPrefix: + """A service literally named `x-web` is a real service, not an extension field. + + `validate()` iterates `services.items()` with no `x-` filter, so a + service named `x-web` is planned and emitted like any other service. + The sweep's `x-` skip is syntactic (`key.startswith("x-")`) but its + rationale is semantic ("a subtree we ignore"); on the `services` + mapping's own keys those two diverge, since a service name is an + identifier, not an extension-field marker. This regression let such a + service's whole body escape the sweep -- see _sweep_service/_sweep_document. + """ + + def test_top_level_body_non_string_key_rejected_cleanly(self) -> None: + # Before the fix: _validate_service's own sorted(svc) crashed raw + # (`TypeError: '<' not supported between instances of 'int' and + # 'str'`) because the sweep skipped this service's body entirely. + compose = {"services": {"x-web": {"image": "alpine", 3306: "db"}}} + with pytest.raises(UnsupportedComposeError, match=r"service 'x-web': key 3306 must be a string"): + validate(compose) + + def test_healthcheck_non_string_key_rejected_cleanly(self) -> None: + # Same escape, reached via _validate_service_healthcheck's own + # sorted(healthcheck) instead. + compose = {"services": {"x-web": {"image": "alpine", "healthcheck": {3: "x"}}}} + with pytest.raises(UnsupportedComposeError, match=r"service 'x-web'\.healthcheck: key 3 must be a string"): + validate(compose) + + def test_nested_map_non_string_key_rejected_cleanly(self) -> None: + # Before the fix: validate() returned [] (no warnings, no raise) and + # emit_script rendered `-e "True=1" --label "True=v"` -- the Python + # repr of the YAML-1.1 bareword keys `on`/`yes`, leaked verbatim. + compose = { + "services": { + "x-web": {"image": "alpine", "environment": {True: 1}, "labels": {True: "v"}}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"service 'x-web'\.environment: key True must be a string"): + validate(compose) + + def test_well_formed_is_accepted_as_a_real_service(self) -> None: + compose = {"services": {"x-web": {"image": "alpine"}}} + assert validate(compose) == [] From a3356cb8f551cce08107f0e3faf19eafdd41177c Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 11:31:11 +0300 Subject: [PATCH 34/43] fix: narrow the sweep to regions validate() actually reads The non-string-key sweep walked the whole document, skipping only x- prefixed subtrees. build's contents and the ignored top-level networks/volumes blocks are not x-prefixed, so the sweep rejected non-string keys inside them too, even though compose2pod never reads either region: image_for never reads a service's build contents (the CI image always wins), and top-level networks/volumes are accepted and warned, never inspected past the membership check. A non-string key there can never reach the generated script, so rejecting one was pure over-rejection of input Docker accepts. _sweep_service now skips 'build' alongside the service's own x- keys. Everything the sweep previously rejected correctly -- environment/labels/ ulimits/service names/store names, etc. -- is still rejected identically; only build/top-level networks/volumes moved from rejected to accepted. --- compose2pod/parsing.py | 65 +++++++++++++++++++++++++++--------------- tests/test_parsing.py | 48 +++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 23 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 9bdba5f..51381eb 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -246,38 +246,61 @@ def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - def _sweep_service(name: str, svc: dict[str, Any]) -> None: - """Require every mapping key in one service body to be a string, at every depth. + """Require every mapping key in one service body to be a string, at every depth compose2pod reads. The service's own top-level keys are always checked (`require_string_keys` below), regardless of what the service's *name* looks like -- this function is only ever called with the real body of a real service, - including one literally named `x-web` (see `_sweep_document`). Only the - service's *own* `x-`-prefixed keys are skipped, same as everywhere else - `x-` is treated as an extension-field marker. + including one literally named `x-web` (see `_sweep_document`). + + Two of the service's own top-level keys are skipped rather than swept, + because compose2pod never reads their contents: `build` (accepted, but + `image_for` never reads its contents -- see + architecture/supported-subset.md) and any `x-`-prefixed key (an + extension field whose value is arbitrary user payload, by design, same + as everywhere else `x-` is skipped). Everything else is swept + recursively, since every other service key's value compose2pod either + reads structurally or emits into the generated script. """ where = f"service {name!r}" require_string_keys(where, svc) for key, value in svc.items(): - if key.startswith("x-"): + if key == "build" or key.startswith("x-"): continue _require_string_keys_deep(f"{where}.{key}", value) def _sweep_document(compose: dict[str, Any]) -> None: - """Require every mapping key, at every depth of `compose`, to be a string. - - Behaves like calling `_require_string_keys_deep("compose document", - compose)` directly, with one correction: the `services` mapping's own - keys (service *names*), and each top-level `secrets`/`configs` - definition's own name, are always swept regardless of what the name - looks like, rather than treated as extension-field markers when they - start with `x-`. `validate()` iterates `services.items()` with no `x-` - filter, so a service literally named `x-web` is a real service, not an - extension field -- conflating a NAME with a content key let such a - service's whole body escape the sweep entirely (see `_sweep_service`). - `stores._validate_def` accepts a store name matching - `[a-zA-Z0-9][a-zA-Z0-9_.-]*`, which does not exclude one starting `x-` - either, so the same correction applies there. + """Require every mapping key to be a string, but only in regions `validate()` actually reads. + + Every later check that reads a mapping's keys directly (`sorted()`, + `.startswith()`) or f-string-interpolates one into a flag value can + assume every key it sees is a string -- provided it only ever reads a + region swept here. + + Swept: + + - The top-level document's own keys. + - The `services` mapping's own keys (service *names*): always, and + regardless of what a name looks like. `validate()` iterates + `services.items()` with no `x-` filter, so a service literally named + `x-web` is a real service, not an extension field -- conflating a + NAME with a content key is exactly the bug this function exists to + not repeat (see `_sweep_service`). + - Each service's body (`_sweep_service`), except `build`'s contents and + the service's own `x-`-prefixed keys -- neither is ever read. + - Each top-level `secrets`/`configs` definition's body: read by + `stores.py`. Swept by name (like `services`, not by the generic + `x-`-skipping walk), for the same reason -- `stores._validate_def` + accepts a store name matching `[a-zA-Z0-9][a-zA-Z0-9_.-]*`, which + does not exclude one starting with `x-`, so a store's *name* must not + be conflated with a content key either. + + Skipped, because compose2pod never reads or emits from them, so a + non-string key there can never reach the generated script: `x-` blocks + (top-level and per-service), `build`'s contents, and the ignored + top-level `networks`/`volumes` blocks (accepted, but never read -- + see architecture/supported-subset.md). """ require_string_keys("compose document", compose) services = compose.get("services") @@ -293,10 +316,6 @@ def _sweep_document(compose: dict[str, Any]) -> None: for def_name, definition in defs.items(): if isinstance(definition, dict): _require_string_keys_deep(f"compose document.{top_key}.{def_name}", definition) - for key, value in compose.items(): - if key in ("services", "secrets", "configs") or key.startswith("x-"): - continue - _require_string_keys_deep(f"compose document.{key}", value) def _validate_depends_on(services: dict[str, Any]) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 5463275..e934a6d 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -741,3 +741,51 @@ def test_nested_map_non_string_key_rejected_cleanly(self) -> None: def test_well_formed_is_accepted_as_a_real_service(self) -> None: compose = {"services": {"x-web": {"image": "alpine"}}} assert validate(compose) == [] + + +class TestSweepSkipsUnreadRegions: + """`build`'s contents and the ignored top-level `networks`/`volumes` blocks are never read. + + compose2pod accepts these regions but never inspects their contents (see + architecture/supported-subset.md), so a non-string key inside them can + never reach the generated script and must not be rejected -- Docker + itself accepts them. The `environment`/other emitted-map divergence + (`{3306: db}` rejected) is unaffected: those keys do reach the script. + """ + + def test_build_contents_non_string_key_accepted(self) -> None: + compose = {"services": {"app": {"build": {"context": ".", "args": {True: 1}}}}} + assert validate(compose) == [] + + def test_top_level_volumes_contents_non_string_key_accepted(self) -> None: + compose = { + "services": {"app": {"image": "alpine"}}, + "volumes": {"data": {"driver_opts": {True: 1}}}, + } + assert any("ignoring top-level 'volumes'" in w for w in validate(compose)) + + def test_top_level_networks_contents_non_string_key_accepted(self) -> None: + compose = { + "services": {"app": {"image": "alpine"}}, + "networks": {"net1": {"driver_opts": {True: 1}}}, + } + assert any("ignoring top-level 'networks'" in w for w in validate(compose)) + + def test_environment_non_string_key_still_rejected(self) -> None: + # The `environment: {3306: db}` divergence from Docker stands: that + # key does reach the emitted script, unlike build/top-level + # networks/volumes above. + compose = {"services": {"app": {"image": "x", "environment": {3306: "db"}}}} + with pytest.raises(UnsupportedComposeError, match=r"environment: key 3306 must be a string"): + validate(compose) + + def test_top_level_secrets_and_configs_stay_swept(self) -> None: + # Unlike build/top-level networks/volumes, secrets and configs ARE + # read (stores.py) and must stay swept, at both the definition and + # the service-reference sides. + compose = { + "services": {"app": {"image": "x", "secrets": [{"source": "s", True: "bogus"}]}}, + "secrets": {"s": {"file": "./a"}}, + } + with pytest.raises(UnsupportedComposeError): + validate(compose) From 306ff926ff5ab1b84e320b0b34f5b6c55aa11500 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 11:32:12 +0300 Subject: [PATCH 35/43] test: assert the sweep actually recurses into lists _require_string_keys_deep's `elif isinstance(node, list)` branch was reachable (line-covered by existing tests exercising service-level secrets/configs references) but nothing asserted on its effect, so deleting the branch left every other test green -- "does the sweep recurse into lists-of-mappings?" was unfalsifiable by the suite. Confirmed by deleting the branch: the new test goes red because the sweep's own message stops appearing and stores.py's independent, differently-worded check (unaffected by the deletion) raises instead, which the test's match= no longer matches. Branch restored afterward, confirmed byte-identical to before this test was added. --- tests/test_parsing.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index e934a6d..fb90403 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -789,3 +789,25 @@ def test_top_level_secrets_and_configs_stay_swept(self) -> None: } with pytest.raises(UnsupportedComposeError): validate(compose) + + +class TestSweepListRecursion: + """The sweep must recurse into a list of mappings, not just nested dicts. + + Regression-proofing for a coverage gap: the sweep's + `elif isinstance(node, list)` branch was reachable but nothing asserted + on its effect, so removing it left all other tests green. This asserts + the sweep's own message specifically (produced only by the list branch + recursing into the ref dict before stores.py's independent, + differently-worded check gets a turn) -- deleting the list branch turns + this test red even though `validate()` still raises overall (via + stores.py), because the raised message no longer matches. + """ + + def test_non_string_key_nested_in_a_list_is_caught_by_the_sweep_itself(self) -> None: + compose = { + "services": {"app": {"image": "x", "secrets": [{"source": "mysecret", 1: "bogus"}]}}, + "secrets": {"mysecret": {"file": "./a"}}, + } + with pytest.raises(UnsupportedComposeError, match=r"service 'app'\.secrets: key 1 must be a string"): + validate(compose) From 7aa2c5c35956ffcdcedb8dcc5b20769f3c8f60c7 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 11:34:10 +0300 Subject: [PATCH 36/43] docs: correct sweep-scope claims in supported-subset.md and the plan architecture/supported-subset.md's non-string-mapping-key bullet claimed the sweep applied "uniformly to every mapping in the document" -- false in both directions after this round's fixes: a service named x-* is swept (the bullet never distinguished a NAME from a content key), and build's contents / top-level networks/volumes are explicitly not swept (never read, so a non-string key there can't reach the script). Rewrite states the true scope: which regions are swept, which are skipped and why, and that a service named x-* is a service. planning/changes/2026-07-13.10-close-structural-key-gate.md gets a Round 6 section recording both findings, the fix, and the anchor-merged-from-x- block edge case re-verified against the fix. --- architecture/supported-subset.md | 115 +++++++----- ...2026-07-13.10-close-structural-key-gate.md | 169 +++++++++++++++++- 2 files changed, 235 insertions(+), 49 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index aa2c114..fd20167 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -17,59 +17,78 @@ warns (ignored, behavior-neutral inside a single pod) or raises - **Extension fields:** any key prefixed `x-` is accepted and ignored silently, per the Compose spec. This is what lets a document hold shared config in a top-level `x-*` block for reuse via YAML anchors. -- **Every mapping key, at every depth of the whole document, must be a - string.** PyYAML routinely produces a non-string key — a bare `3:` parses - as an int, and under YAML 1.1 a bare `on:`/`off:`/`yes:`/`no:` parses as a - bool. `validate()` walks the entire compose document once, recursively, - before running any other check (`_require_string_keys_deep`, +- **Every mapping key must be a string, in every region `validate()` + actually reads from or emits into.** PyYAML routinely produces a + non-string key — a bare `3:` parses as an int, and under YAML 1.1 a bare + `on:`/`off:`/`yes:`/`no:` parses as a bool. `validate()` sweeps these + regions once, recursively, before running any other check + (`_sweep_document`/`_sweep_service`/`_require_string_keys_deep`, `compose2pod/parsing.py`, built on `require_string_keys`, - `compose2pod/keys.py`) and rejects a non-string key at any depth — - `UnsupportedComposeError` names the offending key and its location. This - applies uniformly to every mapping in the document (the top-level document - itself, every service body and its nested structures, `healthcheck`, - `deploy` and its nested `resources`/`limits`/`reservations`, every - top-level `secrets`/`configs` definition and every service's long-form - reference, `ulimits` and its nested `{soft, hard}` mappings, and so on) — - it is not a list of specific mappings to keep in sync as new keys are - added. **Exception:** `x-`-prefixed keys' values are not walked, since - Compose extension fields legitimately hold arbitrary payloads (e.g. YAML - anchor sources reused via `<<:`) that compose2pod accepts and ignores by - design; the `x-` key itself is still checked (trivially — its own name has - to look like `x-...`). + `compose2pod/keys.py`) and rejects a non-string key found in any of + them — `UnsupportedComposeError` names the offending key and its + location. Swept: the top-level document's own keys; the `services` + mapping's own keys (service *names*) — always, regardless of what a name + looks like, since `validate()` treats every entry in `services` as a real + service with no `x-` filter (a service literally named `x-web` is swept + like any other service, not skipped as an extension field); each + service's body — every structural key, `healthcheck`, `deploy` and its + nested `resources`/`limits`/`reservations`, `ulimits` and its nested + `{soft, hard}` mappings, per-service `secrets`/`configs`/`networks` + references, and so on — except `build`'s own contents and the service's + own `x-`-prefixed keys; and each top-level `secrets`/`configs` + definition (read by `stores.py`), by name, for the same reason a service + name is swept regardless of what it looks like. + + **Skipped**, because compose2pod never reads or emits from these + regions, so a non-string key inside one can never reach the generated + script: `x-` blocks (top-level and per-service) — Compose extension + fields legitimately hold arbitrary payloads (e.g. YAML anchor sources + reused via `<<:`) that compose2pod accepts and ignores by design, so + their contents are never walked (the `x-` key itself is still checked, + trivially — its own name has to look like `x-...`); `build`'s own + contents (`context`/`dockerfile`/`args`, never read — see `build` below); + and the ignored top-level `networks`/`volumes` blocks (accepted, but + their contents are never read — see Volumes below and the Pod-level + options section for why top-level `networks` has no effect). Two distinct downstream consumer classes motivate rejecting a non-string - key up front rather than one key at a time: mapping-key readers that - crash raw otherwise (`sorted()`, `str.startswith`, the secret/config name - regex), and mapping-key consumers that don't crash but f-string- - interpolate the key straight into a flag value, silently leaking a bool's - or int's Python repr into the emitted script instead - (`keys.key_value_pairs` — `environment`/`labels`/`annotations`, - `keys.extra_host_pairs` — `extra_hosts`, `keys._ulimit_args` — `ulimits`, - `pod._sysctl_pairs` — `sysctls`). The second class is silent corruption, - not a crash, and is why a document-wide walk replaced hand-placing a check - at each mapping some other symptom happened to point at. - - This is a deliberate divergence from Docker: `environment: {3306: db}` is - valid Compose and Docker accepts it, but compose2pod refuses it. Docker - parses Compose as YAML 1.2, where a bare `3306`/`on`/`off` stays the - string it looks like, so Docker never observes a non-string key at all. - Normalizing Python's `int`/`bool` back into a key string here would not - reproduce that: unlike a boolean *value* (`DEBUG: true`, normalized to the - string `"true"` like Docker does — see `_render_scalar` below), a boolean - or int *key* has no single correct string form to normalize to (`True` → - `"on"`? `"true"`? `"True"`?). A non-string key is a YAML-1.1 accident, not - intentional Compose; anyone who means the literal string `on` writes - `"on"`. + key up front rather than one key at a time within a swept region: + mapping-key readers that crash raw otherwise (`sorted()`, + `str.startswith`, the secret/config name regex), and mapping-key + consumers that don't crash but f-string-interpolate the key straight into + a flag value, silently leaking a bool's or int's Python repr into the + emitted script instead (`keys.key_value_pairs` — `environment`/ + `labels`/`annotations`, `keys.extra_host_pairs` — `extra_hosts`, + `keys._ulimit_args` — `ulimits`, `pod._sysctl_pairs` — `sysctls`). The + second class is silent corruption, not a crash. + + This is a deliberate divergence from Docker for keys that *are* swept: + `environment: {3306: db}` is valid Compose and Docker accepts it, but + compose2pod refuses it. Docker parses Compose as YAML 1.2, where a bare + `3306`/`on`/`off` stays the string it looks like, so Docker never + observes a non-string key at all. Normalizing Python's `int`/`bool` back + into a key string here would not reproduce that: unlike a boolean *value* + (`DEBUG: true`, normalized to the string `"true"` like Docker does — see + `_render_scalar` below), a boolean or int *key* has no single correct + string form to normalize to (`True` → `"on"`? `"true"`? `"True"`?). A + non-string key is a YAML-1.1 accident, not intentional Compose; anyone + who means the literal string `on` writes `"on"`. Docker also accepts a + non-string key in `build`'s `args` or a top-level `volumes` block's + `driver_opts` — since compose2pod never reads either, it accepts them + too, matching Docker rather than diverging from it there. A handful of module entry points that are also called directly (not only - reached through `validate()`) keep their own `require_string_keys` call in - addition to the document-wide sweep, as defense at their own boundary: - `resources.validate_deploy`'s four checks (`deploy` and its nested - `resources`/`limits`/`reservations`) and `stores.py`'s three checks (a - secret/config definition's keys, a long-form reference's keys, and the - top-level `secrets`/`configs` block's own keys). These are redundant only - when reached through `validate()`; each is still load-bearing for a caller - that invokes that function directly. + reached through `validate()`) keep their own `require_string_keys` call + in addition to the sweep, as defense at their own boundary and as + belt-and-braces for two service-level checks the sweep also covers: + `_validate_service` and `_validate_service_healthcheck` + (`compose2pod/parsing.py`), `resources.validate_deploy`'s four checks + (`deploy` and its nested `resources`/`limits`/`reservations`), and + `stores.py`'s three checks (a secret/config definition's keys, a + long-form reference's keys, and the top-level `secrets`/`configs` + block's own keys). These are redundant only when reached through + `validate()`; each is still load-bearing for a caller that invokes that + function directly. - Everything else raises. ## Service keys diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index 04894a9..d02664b 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,5 +1,5 @@ --- -summary: validate() rejects every malformed structural-key shape found across five review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. Round 4's non-string-mapping-key guard was ten hand-placed require_string_keys() calls across three modules, not document-wide as claimed; Round 5 replaced it with one recursive sweep (parsing._require_string_keys_deep) that walks the whole document ahead of every other check, closing two holes the hand-placed calls missed by construction (a non-string key f-string-interpolated into a flag value, and service names). See "Round 5" for the current, verified scope of what is closed versus what was not re-audited. +summary: validate() rejects every malformed structural-key shape found across six review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. Round 4's non-string-mapping-key guard was ten hand-placed require_string_keys() calls across three modules, not document-wide as claimed; Round 5 replaced it with one recursive sweep that walked the whole document ahead of every other check, closing two holes the hand-placed calls missed by construction (a non-string key f-string-interpolated into a flag value, and service names) but introducing two new defects of its own: a service literally named `x-*` escaped the sweep entirely (a regression), and the sweep over-rejected non-string keys in regions never read (`build`'s contents, top-level `networks`/`volumes`). Round 6 scoped the sweep to the regions validate() actually reads (`_sweep_document`/`_sweep_service`, compose2pod/parsing.py), closing both. See "Round 6" for the current, verified scope of what is closed versus what was not re-audited. --- # Design: Close the structural-key gate @@ -544,3 +544,170 @@ seven that broke a test (`resources.py`'s four deploy-block calls, `chats_compose` fixture and every `tests/integration/` scenario pass unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just check-planning` all clean. + +## Round 6: the sweep skipped by syntax, not by what it actually skips + +A sixth review found that Round 5's `_require_string_keys_deep` skipped +recursion on any `x-`-prefixed key, at every depth, unconditionally — +including the `services` mapping's own keys, where "skip this key's +subtree" is the wrong rule: a service name is an identifier, not an +extension-field marker. The same unconditional walk also swept two regions +compose2pod never reads at all, over-rejecting input Docker accepts. + +| Finding | Before | Kind | +|---------|--------|------| +| `services: {x-web: {image: alpine, 3306: db}}` (a service literally named `x-web`) | the sweep skipped `x-web`'s entire body (its name matched the `x-` skip predicate); `_validate_service`'s own `sorted(svc)` then crashed raw on the mixed int/str keys the sweep never checked | crash, regression | +| `services: {x-web: {image: alpine, environment: {on: 1}, labels: {yes: v}}}` | same escape; `validate()` returned clean and `emit_script` rendered `-e "True=1" --label "True=v"` | silent corruption, regression | +| `build: {context: ., args: {on: 1}}` | rejected — `build`'s contents are accepted but never read (`image_for` never reads them; see "Service keys" below) | over-rejection | +| top-level `volumes: {data: {driver_opts: {on: 1}}}` | rejected — top-level `volumes` is accepted but never read (see Volumes, below) | over-rejection | + +### Why F1 (the service-name escape) is a distinct bug from a missing check + +The sweep's skip predicate (`key.startswith("x-")`) is syntactic; its +rationale ("this subtree holds an ignored extension-field payload") is +semantic. Everywhere else the sweep is called, those two coincide: an +`x-`-prefixed key inside a service body, a `healthcheck`, or a top-level +document *is* an extension field by construction. On the `services` +mapping's own keys they diverge — `validate()` iterates `services.items()` +with no `x-` filter (`compose2pod/parsing.py`), so a service named `x-web` +is planned and emitted exactly like `web` would be. The sweep conflated "a +key that looks like `x-...`" with "a key that means `x-...`", and only the +`services` mapping (and, by the identical reasoning, the top-level +`secrets`/`configs` mappings — a store name matching +`stores._NAME`'s `[a-zA-Z0-9][a-zA-Z0-9_.-]*` does not exclude one starting +`x-` either) is a place where those two readings disagree. + +### Why F2 (the over-rejection) happened + +Round 5's sweep started from `validate()`'s call with +`_require_string_keys_deep("compose document", compose)` and walked every +key the document had, skipping only `x-`-prefixed subtrees. `build` and +the top-level `networks`/`volumes` blocks are not `x-`-prefixed, so the +walk swept them too — even though none of the three is ever read: +`image_for` never reads `build`'s contents when present (a service with +`build` always runs the CI image), and top-level `networks`/`volumes` are +accepted-and-warned, never inspected beyond the membership check that +produces the warning. A non-string key inside any of the three can never +reach the generated script, so rejecting one is pure over-rejection — +input Docker accepts that compose2pod refused for no reason tied to its +own behavior. + +### The fix: sweep only the regions `validate()` reads, by construction + +`_sweep_document` (`compose2pod/parsing.py`) replaces the single blind +`_require_string_keys_deep("compose document", compose)` call with an +orchestration that names each region it sweeps, rather than walking +everything and trying to skip the wrong parts by pattern-matching key +names: + +- The top-level document's own keys (`require_string_keys`, shallow — no + recursion into any top-level value happens here). +- The `services` mapping's own keys, via `require_string_keys` (shallow, + no `x-` skip), then every service's body via `_sweep_service` — + called for **every** entry in `services`, regardless of the service's + name. `_sweep_service` skips only two of the service's *own* top-level + keys: `build` (never read) and the service's own `x-`-prefixed keys + (extension fields, never read) — everything else is swept with + `_require_string_keys_deep`, unchanged from Round 5's recursive walk and + still skipping nested `x-` subtrees the same way (a `healthcheck`-level + or `deploy`-level `x-` key is still a genuine extension field). +- Each top-level `secrets`/`configs` block's own keys (`require_string_keys`, + shallow, no `x-` skip — a store name is an identifier, exactly like a + service name), then each definition's body via `_require_string_keys_deep` + — called for every entry, regardless of name, closing the same + identifier-vs-content-key gap for stores that F1 closed for services. + +`build`'s contents and the top-level `networks`/`volumes` blocks are never +passed to `_require_string_keys_deep` at all — not skipped by a predicate +inside the walk, but never reached by the orchestration in the first +place, since nothing in `_sweep_document` recurses into them. + +This round shipped as two commits, so each finding is independently +bisectable: the first restored `_sweep_service`/`_sweep_document`'s +always-sweep-by-name behavior for `services`/`secrets`/`configs` (closing +F1) while still sweeping `build`/top-level `networks`/`volumes` generically +(F2 still open); the second then skipped `build` in `_sweep_service` and +stopped sweeping anything outside `services`/`secrets`/`configs` at the +top level (closing F2), with no further change to the F1 fix. Restoring the +two `require_string_keys` calls Round 5 deleted from `_validate_service` +and `_validate_service_healthcheck` was tried as belt-and-braces alongside +the first commit (not as a substitute for the sweep fix, since the sweep's +own correctness is what closes F1) — both were kept: neither function is +called directly by any test today, but both are module entry points with +the same "own contract independent of caller" reasoning +`resources.validate_deploy`/`stores.validate` already established in +Round 5. + +### Anchors merged from an `x-` block still get swept + +`_sweep_service` receives the service body PyYAML already resolved by +load time — an anchor defined in a top-level `x-` block and merged into a +service via `<<:` lands as ordinary keys in that service's body, not as a +separate `x-`-rooted structure, so it is swept exactly like content the +service author wrote directly. This was re-verified directly (not +assumed): `x-common: &common\n environment:\n on: 1\nservices:\n app:\n <<: *common\n` +raises `UnsupportedComposeError` (`tests/test_cli.py::TestMain::test_non_string_key_from_x_block_anchor_merged_into_service_is_rejected`), +confirming the merge doesn't accidentally inherit the source block's `x-` +skip. + +### Is the gate closed now? + +Restating Round 5's closing paragraph with this round folded in: every +reproduction given to this round — and every one given to Rounds 1-5 +before it — now raises `UnsupportedComposeError` instead of crashing raw, +reaching `emit` with a corrupted value, or rejecting input Docker accepts +and compose2pod never reads. What is different in kind from Round 5's +answer: the sweep's scope is now named explicitly by the orchestration +(`_sweep_document`) rather than derived implicitly from "walk everything, +skip `x-`" — the mechanism that produced both of this round's findings. +This is a narrower, not wider, sweep than Round 5's: everything Round 5 +correctly rejected (non-string keys in `environment`/`labels`/ +`annotations`/`sysctls`/`extra_hosts`/`ulimits`/`deploy.resources.limits`/ +service names/store definition names) is still rejected identically; only +`build`'s contents and top-level `networks`/`volumes` moved from rejected +to accepted, and only a service or store literally named `x-*` moved from +silently-escaping-the-sweep to correctly-swept. + +One known, deliberately unaddressed asymmetry remains: `_sweep_document` +sweeps `secrets`/`configs` definition bodies by name (not by the generic +`x-`-skipping walk), closing the identical name-vs-content-key gap F1 +found for services, as a direct generalization of the same fix rather than +a separately reported finding — nothing in this round's brief asked for +it, but leaving the asymmetry in place after fixing it for services would +have left a known, cheap-to-close inconsistency in the same code being +touched. Per-service `secrets`/`configs`/`networks` references (list-of- +mapping entries) were not given the same by-name treatment because they +have no name of their own to conflate with a content key. + +As in every prior round: this is evidence of closure across everything +tested, not a proof of closure for every possible input. + +### Testing (Round 6) + +TDD throughout: every finding above got a failing test confirming the +exact crash, silent corruption, or over-rejection (run against the tree +before this round's fix) before the fix, then a green test after — +including manually reproducing both `x-web` cases and both over-rejection +cases with ad hoc scripts against the pre-fix tree to confirm the exact +before-state the table above describes, not just asserting the fixed +behavior. `tests/test_parsing.py::TestSweepServiceNamedExtensionPrefix` +(both `x-web` findings, plus confirming a well-formed `x-web` service is +still accepted as a real service), `TestSweepSkipsUnreadRegions` (`build`/ +top-level `networks`/`volumes` acceptance, the standing `environment: +{3306: db}` divergence, and top-level `secrets`/`configs` staying swept), +and `TestSweepListRecursion` (a coverage gap closed: the sweep's +`elif isinstance(node, list)` branch was reachable but nothing asserted on +its effect — deleting it left all 519 pre-round tests green; the new test +asserts the sweep's own message specifically, which stops appearing, and +`pytest.raises(..., match=...)` catches the substituted message from +`stores.py`'s independent check instead). `tests/test_cli.py` adds the +anchor-merged-from-`x-`-block regression case end to end. Every acceptance +case from this round's brief was re-verified directly against `validate()` +before committing: `x-` blocks (top level and per-service) with arbitrary +non-string-keyed and nested list-of-mapping payloads, YAML anchors/`<<:` +merge keys including one sourced from an `x-` block, `build.args`/ +top-level `volumes.driver_opts` with non-string keys, boolean map-value +normalization, null map values, normal service names (dotted/dashed/ +underscored). `tests/conftest.py`'s `chats_compose` fixture and every +`tests/integration/` scenario pass unchanged. `just test-ci` at 100% line +coverage, `just lint-ci`, and `just check-planning` all clean. From ab38d417442449e55b874689604a64f78862b97b Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 13:36:06 +0300 Subject: [PATCH 37/43] fix(graph): reject non-string depends_on condition before set lookup condition not in DEPENDS_ON_CONDITIONS (parsing._validate_depends_on) hashes its operand, so an unhashable condition (e.g. a dict or list) crashed raw with TypeError: unhashable type instead of failing clean. Checked in graph.depends_on, which already owns every other depends_on shape check, so every caller -- not just validate() -- gets the same protection. --- compose2pod/graph.py | 15 ++++++++++++++- tests/test_graph.py | 17 +++++++++++++++++ tests/test_parsing.py | 18 ++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/compose2pod/graph.py b/compose2pod/graph.py index ad4e45a..9b8b0ee 100644 --- a/compose2pod/graph.py +++ b/compose2pod/graph.py @@ -22,7 +22,20 @@ def depends_on(svc: dict[str, Any]) -> dict[str, str]: if not isinstance(spec, dict): msg = f"depends_on entry {dep!r} must be a mapping" raise UnsupportedComposeError(msg) - result[dep] = spec.get("condition", "service_started") + condition = spec.get("condition", "service_started") + if not isinstance(condition, str): + # Callers (parsing._validate_depends_on) test membership in a + # `set` of known condition strings -- `x in a_set` hashes `x`, + # so an unhashable condition (a dict or list) would otherwise + # crash raw with `TypeError: unhashable type` instead of failing + # clean. Checked here, not there: this function already owns + # every other depends_on shape check (list vs mapping, spec must + # be a mapping), so a bad condition type belongs with them, and + # every caller of `depends_on` -- not just validate() -- gets the + # same protection. + msg = f"depends_on entry {dep!r}: condition must be a string" + raise UnsupportedComposeError(msg) + result[dep] = condition return result diff --git a/tests/test_graph.py b/tests/test_graph.py index d9a622d..fec2e6e 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -36,6 +36,23 @@ def test_list_form_string_entries_still_accepted(self) -> None: "keydb": "service_started", } + def test_unhashable_condition_raises_cleanly(self) -> None: + # DEPENDS_ON_CONDITIONS (parsing.py) is a set, and `x in a_set` + # hashes `x` -- an unhashable condition (dict/list) used to crash + # raw (TypeError: unhashable type) instead of failing clean. + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry 'db': condition must be a string"): + depends_on({"depends_on": {"db": {"condition": {"a": 1}}}}) + + def test_list_condition_also_raises_cleanly(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry 'db': condition must be a string"): + depends_on({"depends_on": {"db": {"condition": ["x"]}}}) + + def test_int_condition_raises_cleanly(self) -> None: + # Hashable but still not a valid condition shape -- must not slip + # past this check only to fail confusingly deeper in. + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry 'db': condition must be a string"): + depends_on({"depends_on": {"db": {"condition": 1}}}) + class TestHostnames: def test_collects_service_names_and_aliases(self, chats_compose: dict) -> None: diff --git a/tests/test_parsing.py b/tests/test_parsing.py index fb90403..f50ed0a 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -192,6 +192,24 @@ def test_unknown_depends_on_condition_raises(self) -> None: ): validate(compose) + def test_unhashable_depends_on_condition_raises_cleanly(self) -> None: + # Before the fix: `condition not in DEPENDS_ON_CONDITIONS` + # (parsing._validate_depends_on) crashed raw (TypeError: cannot use + # 'dict' as a set element -- unhashable type: 'dict') because + # DEPENDS_ON_CONDITIONS is a set and `in` hashes its operand; + # graph.depends_on checked the dependency's spec was a mapping but + # never the condition's own type. This is a mapping's *key* being + # a string -- the sweep's non-string-key check is irrelevant here, + # since `{"a": 1}` is a well-formed mapping with a string key. + compose = { + "services": { + "app": {"image": "x", "depends_on": {"db": {"condition": {"a": 1}}}}, + "db": {"image": "y"}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"depends_on entry 'db': condition must be a string"): + validate(compose) + def test_top_level_extension_key_is_accepted(self) -> None: compose = {"x-application-defaults": {"build": {}}, "services": {"app": {"image": "x"}}} assert validate(compose) == [] From b9a2606b26d7e9c448943c89c7f69b77a29f832b Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 13:37:11 +0300 Subject: [PATCH 38/43] fix(emit): treat a null healthcheck scalar as unset _health_flags keyed --health-timeout/--health-retries/--health-start-period off key presence, not value, so an explicit `timeout: null` (already accepted by validate()) emitted the literal string 'None' into the generated script. Ruling: null means unset, matching `docker compose config` and how this package already treats a null environment/volumes/ command value -- omit the flag entirely, keyed off `.get(key) is not None`. --- compose2pod/emit.py | 12 +++++++++--- tests/test_emit.py | 16 ++++++++++++++++ tests/test_parsing.py | 14 ++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/compose2pod/emit.py b/compose2pod/emit.py index 41910d1..93c1a97 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -53,11 +53,17 @@ def _health_flags(healthcheck: dict[str, Any]) -> list[Token]: cmd = health_cmd(healthcheck.get("test")) if cmd is not None: flags += ["--health-cmd", Expand(value=cmd)] - if "timeout" in healthcheck: + # An explicit `null` scalar means unset, not "emit the literal string + # 'None'" -- the same treatment `environment`/`volumes`/`command` + # give a null value elsewhere in this package, and matching `docker + # compose config`, which treats an explicitly-null key as absent. + # Keyed off value, not presence, so `timeout: null` and an omitted + # `timeout` behave identically. + if healthcheck.get("timeout") is not None: flags += ["--health-timeout", str(healthcheck["timeout"])] - if "start_period" in healthcheck: + if healthcheck.get("start_period") is not None: flags += ["--health-start-period", str(healthcheck["start_period"])] - if "retries" in healthcheck: + if healthcheck.get("retries") is not None: flags += ["--health-retries", str(healthcheck["retries"])] return flags diff --git a/tests/test_emit.py b/tests/test_emit.py index c0ca06f..471fa38 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -109,6 +109,22 @@ def test_healthcheck_without_timeout_omits_health_timeout_flag(self) -> None: assert flags[4:6] == ["--health-cmd", Expand(value="true")] assert "--health-timeout" not in flags + def test_healthcheck_explicit_null_scalars_omit_flags(self) -> None: + # An explicit `null` means unset, matching `docker compose config` + # and how this package treats null everywhere else (environment/ + # volumes/command). Before the fix: keyed off key *presence* + # (`"timeout" in healthcheck`), not the value, so an explicit null + # emitted the literal string 'None' as the flag value. + svc = { + "image": "x", + "healthcheck": {"test": "true", "timeout": None, "retries": None, "start_period": None}, + } + flags = run_flags("app", svc, "p", "/b") + assert "--health-timeout" not in flags + assert "--health-retries" not in flags + assert "--health-start-period" not in flags + assert "None" not in [str(f) for f in flags] + def test_user_flag(self) -> None: flags = run_flags("app", {"image": "x", "user": "1000:1000"}, "p", "/b") assert flags[4:6] == ["--user", Expand(value="1000:1000")] diff --git a/tests/test_parsing.py b/tests/test_parsing.py index f50ed0a..891bc94 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -380,6 +380,20 @@ def test_healthcheck_scalars_accept_ints_and_strings(self) -> None: } assert validate(compose) == [] + def test_healthcheck_scalars_accept_explicit_null(self) -> None: + # A null scalar is treated as unset (see emit._health_flags), same + # ruling `environment`/`volumes`/`command` already get for a null + # value -- it must not raise at the gate. + compose = { + "services": { + "app": { + "image": "x", + "healthcheck": {"test": "true", "retries": None, "timeout": None, "start_period": None}, + } + } + } + assert validate(compose) == [] + def test_healthcheck_retries_mapping_rejected_at_gate(self) -> None: # Used to be silently accepted and mis-emitted as the literal # --health-retries "{'a': 1}". From 5af45cbf184bf6c34b8afe478f3b075e93d5a4d2 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 13:37:59 +0300 Subject: [PATCH 39/43] fix(parsing): don't treat identifier-keyed service keys as extension fields _sweep_service handed depends_on/networks/ulimits's whole mapping form to _require_string_keys_deep, whose x- skip is only valid for content keys (per its own docstring) -- a dependency/network/ulimit literally named x-foo was treated as an extension field, so a malformed subtree under it escaped the sweep. _sweep_identifier_map checks these identifiers with require_string_keys (no x- skip) and hands only each identifier's own value to the ordinary x--skipping deep walk, honoring the contract instead of changing it. Also adds a test pinning the x- skip inside _require_string_keys_deep itself, which no existing test would catch if deleted (deleting the line was verified to turn the new test red; restoring turns it green). --- compose2pod/parsing.py | 55 +++++++++++++++++++++++++++++++---- tests/test_parsing.py | 66 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 6 deletions(-) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 51381eb..2c1bbef 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -218,10 +218,15 @@ def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - is a string by construction (its own name has to look like `x-...`). This skip is only ever correct for a key that plays the role of "extension field name" in the node being walked -- callers (`_sweep_document`/ - `_sweep_service`) are responsible for never handing this function a - mapping whose keys are *identifiers* (a service name, a store name) + `_sweep_service`/`_sweep_identifier_map`) are responsible for never + handing this function a mapping whose keys are *identifiers* (a service + name, a store name, a dependency name, a network name, a ulimit name) rather than content keys, since an identifier starting with `x-` is not - an extension field. + an extension field. `_sweep_document` sweeps `services`/`secrets`/ + `configs` names this way already; `_sweep_identifier_map` does the same + for the identifier-keyed *service* keys (`depends_on`, `networks`, + `ulimits`) before handing each identifier's own content to this + function. Rejecting a non-string key is a deliberate divergence from Docker for map-typed *keys* specifically (Docker accepts `environment: {3306: db}`): @@ -245,6 +250,39 @@ def _require_string_keys_deep(where: str, node: Any) -> None: # noqa: ANN401 - _require_string_keys_deep(where, item) +_IDENTIFIER_KEYED_SERVICE_KEYS = {"depends_on", "networks", "ulimits"} + + +def _sweep_identifier_map(where: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped + """Sweep a service key whose mapping form is keyed by another entity's *identifier*. + + `depends_on` (dependency = another service's name), `networks` (a + network's name), and `ulimits` (a resource-limit category's name) all + key their mapping form by an identifier, not a content key -- unlike + `environment`/`labels`/`sysctls`/etc., where the map key itself *is* + content. `_require_string_keys_deep`'s blanket `x-` skip is only correct + for content keys (see its docstring): fed an identifier map directly, it + would treat a dependency/network/ulimit literally named `x-foo` as an + extension field and skip checking its value, the same conflation that + made a service named `x-web` fall through the old sweep. So the + identifiers here are checked with `require_string_keys` (no `x-` skip -- + an identifier starting with `x-` is still a real identifier), and only + each identifier's own *value* -- ordinary content from that point on -- + is handed to the `x-`-skipping deep walk. + + List-form (`depends_on: [...]`/`networks: [...]`) and absent/null values + are not identifier-keyed at all; they fall through to the plain deep + walk unchanged. + """ + full = f"{where}.{key}" + if not isinstance(value, dict): + _require_string_keys_deep(full, value) + return + require_string_keys(full, value) + for identifier, content in value.items(): + _require_string_keys_deep(f"{full}.{identifier}", content) + + def _sweep_service(name: str, svc: dict[str, Any]) -> None: """Require every mapping key in one service body to be a string, at every depth compose2pod reads. @@ -258,15 +296,20 @@ def _sweep_service(name: str, svc: dict[str, Any]) -> None: `image_for` never reads its contents -- see architecture/supported-subset.md) and any `x-`-prefixed key (an extension field whose value is arbitrary user payload, by design, same - as everywhere else `x-` is skipped). Everything else is swept - recursively, since every other service key's value compose2pod either - reads structurally or emits into the generated script. + as everywhere else `x-` is skipped). `depends_on`/`networks`/`ulimits` + are identifier-keyed and go through `_sweep_identifier_map` instead of + the plain deep walk (see there). Everything else is swept recursively, + since every other service key's value compose2pod either reads + structurally or emits into the generated script. """ where = f"service {name!r}" require_string_keys(where, svc) for key, value in svc.items(): if key == "build" or key.startswith("x-"): continue + if key in _IDENTIFIER_KEYED_SERVICE_KEYS: + _sweep_identifier_map(where, key, value) + continue _require_string_keys_deep(f"{where}.{key}", value) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 891bc94..e7647dc 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -730,6 +730,72 @@ def test_normal_service_names_with_dots_dashes_underscores_accepted(self) -> Non assert validate(compose) == [] assert validate({"services": {"app": {"image": "x", "entrypoint": ["run", "me"]}}}) == [] + def test_deep_x_prefixed_key_skip_is_load_bearing(self) -> None: + # Mutant check: deleting `if key.startswith("x-"): continue` from + # _require_string_keys_deep makes this raise (it would recurse into + # the x- key's value and hit the non-string key 3). The existing + # x- tests only cover a top-level x- block (never walked) and a + # service-level x- key (skipped by _sweep_service before it ever + # reaches _require_string_keys_deep) -- neither exercises the skip + # *inside* the deep walk itself, at a nesting level the walk (not + # _sweep_service) is the one doing the skipping. + compose = { + "services": { + "app": { + "image": "x", + "healthcheck": {"test": "true", "x-note": {3: 4}}, + } + } + } + assert validate(compose) == [] + + +class TestIdentifierKeyedServiceKeysNotTreatedAsExtensionFields: + """depends_on/networks/ulimits key their mapping form by an *identifier*, not a content key. + + _require_string_keys_deep's `x-` skip is only correct for content keys + (see its docstring) -- an identifier that happens to start with `x-` is + still a real identifier (a dependency, a network, a ulimit category), + the same distinction that matters for a service literally named + `x-web` (see TestSweepServiceNamedExtensionPrefix). Fed directly to the + deep walk, these identifier-keyed maps would let a malformed subtree + under an `x-`-prefixed identifier escape the sweep entirely -- closed by + _sweep_identifier_map, which checks the identifiers themselves (no `x-` + skip) before handing each identifier's own value to the ordinary + (`x-`-skipping) deep walk. + """ + + def test_dependency_named_extension_prefix_content_rejected(self) -> None: + compose = { + "services": { + "x-dep": {"image": "a"}, + "web": {"image": "b", "depends_on": {"x-dep": {3: 4}}}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"depends_on\.x-dep: key 3 must be a string"): + validate(compose) + + def test_network_named_extension_prefix_content_rejected(self) -> None: + compose = {"services": {"web": {"image": "a", "networks": {"x-net": {3: 4}}}}} + with pytest.raises(UnsupportedComposeError, match=r"networks\.x-net: key 3 must be a string"): + validate(compose) + + def test_ulimit_named_extension_prefix_content_rejected(self) -> None: + compose = {"services": {"web": {"image": "a", "ulimits": {"x-lim": {3: 4}}}}} + with pytest.raises(UnsupportedComposeError, match=r"ulimits\.x-lim: key 3 must be a string"): + validate(compose) + + def test_dependency_named_extension_prefix_well_formed_is_accepted(self) -> None: + # The identifier itself starting with x- is not rejected -- only a + # malformed subtree under it is. + compose = { + "services": { + "x-dep": {"image": "a"}, + "web": {"image": "b", "depends_on": ["x-dep"]}, + } + } + assert validate(compose) == [] + class TestSweepServiceNamedExtensionPrefix: """A service literally named `x-web` is a real service, not an extension field. From d6832d55038834fbb1e9e10e345fd4ab980ef91b Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 13:38:22 +0300 Subject: [PATCH 40/43] fix(emit): validate EmitOptions.artifacts/allow_exit_codes shapes CLI-unreachable (argparse enforces str/int) but a library caller can construct EmitOptions directly: a non-string artifact crashed raw on the ':' membership test, and a non-int allow_exit_codes entry is interpolated unquoted into the generated `case "$rc" in ...)` pattern -- shell injection, not just a crash. _validate_options gains one guard per field. --- compose2pod/emit.py | 16 ++++++++++++++-- tests/test_emit.py | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/compose2pod/emit.py b/compose2pod/emit.py index 93c1a97..6d41021 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -228,11 +228,23 @@ def _emit_target(lines: list[str], tokens: list[Token], options: EmitOptions) -> def _validate_options(options: EmitOptions) -> None: - """Check option values emit destructures (artifacts are split on ':').""" + """Check option values emit destructures or interpolates unquoted. + + CLI-unreachable (argparse enforces `artifact` as `str` and + `allow_exit_codes` as `int`) but a library caller can pass `EmitOptions` + directly: a non-string `artifact` would crash raw on the ':' membership + test below, and a non-int `allow_exit_codes` entry is interpolated + unquoted into the generated `case "$rc" in ...)` pattern -- shell + injection, not just a crash. + """ for artifact in options.artifacts: - if ":" not in artifact: + if not isinstance(artifact, str) or ":" not in artifact: msg = f"artifact {artifact!r} must be in SRC:DST form" raise UnsupportedComposeError(msg) + for code in options.allow_exit_codes: + if isinstance(code, bool) or not isinstance(code, int): + msg = f"allow_exit_codes entry {code!r} must be an int" + raise UnsupportedComposeError(msg) def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: diff --git a/tests/test_emit.py b/tests/test_emit.py index 471fa38..186f53e 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -865,12 +865,57 @@ def test_artifact_without_colon_also_raises_from_referenced_variables(self, chat with pytest.raises(UnsupportedComposeError, match=r"artifact 'nocolon' must be in SRC:DST form"): referenced_variables(chats_compose, options) + def test_non_string_artifact_raises_cleanly(self, chats_compose: dict) -> None: + # CLI-unreachable (argparse enforces str), but a library caller can + # pass EmitOptions directly -- a non-string artifact used to crash + # raw on the ':' membership test (TypeError: argument of type 'int' + # is not iterable) instead of failing clean. + options = self._options([3]) # ty: ignore[invalid-argument-type] + with pytest.raises(UnsupportedComposeError, match=r"artifact 3 must be in SRC:DST form"): + emit_script(compose=chats_compose, options=options) + def test_valid_artifact_still_emits_podman_cp(self, chats_compose: dict) -> None: options = self._options(["/srv/out/junit.xml:junit.xml"]) script = emit_script(compose=chats_compose, options=options) assert "podman cp test-pod-application:/srv/out/junit.xml junit.xml || true" in script +class TestAllowExitCodesValidation: + def _options(self, allow_exit_codes: list[int]) -> EmitOptions: + return EmitOptions( + target="application", + ci_image="ci:latest", + command="", + pod="test-pod", + project_dir=".", + artifacts=[], + allow_exit_codes=allow_exit_codes, + ) + + def test_non_int_allow_exit_code_raises_cleanly(self, chats_compose: dict) -> None: + # CLI-unreachable (argparse enforces int), but a library caller can + # pass EmitOptions directly -- a non-int entry is interpolated + # unquoted into the generated `case "$rc" in ...)` pattern, so an + # unvalidated string is shell injection, not just a crash. + options = self._options( + ['0) ;; *) rm -rf / ;; esac; case "$rc" in 0'] # ty: ignore[invalid-argument-type] + ) + with pytest.raises(UnsupportedComposeError, match=r"allow_exit_codes entry .* must be an int"): + emit_script(compose=chats_compose, options=options) + + def test_bool_allow_exit_code_raises_cleanly(self, chats_compose: dict) -> None: + # bool is an int subclass in Python; True/False are not meaningful + # exit codes and must not slip past an isinstance(..., int) check. + options = self._options([True]) + with pytest.raises(UnsupportedComposeError, match="allow_exit_codes entry True must be an int"): + emit_script(compose=chats_compose, options=options) + + def test_valid_int_allow_exit_codes_still_accepted(self, chats_compose: dict) -> None: + options = self._options([1, 2, 5]) + script = emit_script(compose=chats_compose, options=options) + assert "in\n 0|1|2|5) ;;" in script + + class TestPodmanVersionGuard: def _run_header(self, tmp_path: Path, podman_stub_body: str) -> "subprocess.CompletedProcess[str]": assert _SH is not None # sh is a POSIX baseline binary, always present From 92f08c25787bdf8bb2d45bd00879562d169e1bfb Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 13:38:37 +0300 Subject: [PATCH 41/43] docs: record round 7 findings and rulings, drop changelog voice architecture/supported-subset.md: makes the depends_on section's raw-crash promise true for the long-form condition case too (C1); documents the null-healthcheck-scalar-means-unset ruling (C2) in place of the stale "rejects a mapping/list" claim; documents identifier-keyed service keys (depends_on/networks/ulimits) getting the same name-not-content sweep treatment as service/store names (I1); documents the EmitOptions.artifacts/ allow_exit_codes guards (M2). Drops the healthcheck section's changelog voice ("previously ... crashed raw", "used to reach emit_script()") -- that history now lives only in planning/changes (M1). planning/changes/2026-07-13.10-close-structural-key-gate.md: adds a Round 7 section recording all five findings, the C2 ruling alongside I2's existing boolean-normalization ruling, and the re-verified DO-NOT-OVER-REJECT scenarios. --- architecture/supported-subset.md | 71 ++++++--- ...2026-07-13.10-close-structural-key-gate.md | 139 ++++++++++++++++++ 2 files changed, 190 insertions(+), 20 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index fd20167..59a9a20 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -32,13 +32,23 @@ warns (ignored, behavior-neutral inside a single pod) or raises service with no `x-` filter (a service literally named `x-web` is swept like any other service, not skipped as an extension field); each service's body — every structural key, `healthcheck`, `deploy` and its - nested `resources`/`limits`/`reservations`, `ulimits` and its nested - `{soft, hard}` mappings, per-service `secrets`/`configs`/`networks` - references, and so on — except `build`'s own contents and the service's - own `x-`-prefixed keys; and each top-level `secrets`/`configs` + nested `resources`/`limits`/`reservations`, per-service `secrets`/ + `configs` references, and so on — except `build`'s own contents and the + service's own `x-`-prefixed keys; and each top-level `secrets`/`configs` definition (read by `stores.py`), by name, for the same reason a service name is swept regardless of what it looks like. + Three service keys key their mapping form by another entity's + *identifier* rather than by content — `depends_on` (a dependency's + service name), `networks` (a network's name), and `ulimits` (a + resource-limit category's name) — and get the identical name-not-content + treatment (`_sweep_identifier_map`, `compose2pod/parsing.py`): each + identifier is checked regardless of what it looks like (a dependency + literally named `x-dep` is a real identifier, not an extension field), and + only *its* value — ordinary content from that point on, e.g. `ulimits`' + nested `{soft, hard}` mapping — is swept with the ordinary `x-`-skipping + walk. + **Skipped**, because compose2pod never reads or emits from these regions, so a non-string key inside one can never reach the generated script: `x-` blocks (top-level and per-service) — Compose extension @@ -433,9 +443,7 @@ honors both, refusing loudly on overlap rather than picking a precedence. - **Supported:** `test`, `interval`, `timeout`, `retries`, `start_period`. - A `healthcheck` value that isn't a mapping raises - (`_validate_service_healthcheck`, `compose2pod/parsing.py`) — previously a - non-mapping healthcheck reached `.get()` calls downstream and crashed raw - instead of failing at the gate. + (`_validate_service_healthcheck`, `compose2pod/parsing.py`). - **`test`:** a bare string (shell form), `"NONE"` / `["NONE"]` (disabled), `["CMD", ...]` (exec form), or `["CMD-SHELL", ]`. `health_cmd` (`compose2pod/healthcheck.py`) is the sole reader and the sole validator — @@ -444,21 +452,25 @@ honors both, refusing loudly on overlap rather than picking a precedence. value is discarded there; `emit.py` calls it again to get the value for real). Any other shape raises, including a `CMD-SHELL` whose argument isn't a string and a `CMD` whose trailing elements aren't all strings (e.g. - `["CMD-SHELL", ["curl", "-f"]]`) — both used to reach `emit_script()` - unchecked and crash raw. + `["CMD-SHELL", ["curl", "-f"]]`). - **`interval`:** parsed to whole seconds by `interval_seconds` (`compose2pod/healthcheck.py`). Supported forms: a bare number of seconds (`30`, `"30"`, `"30s"`), minutes (`"2m"`), and milliseconds (`"500ms"`). Compound durations (`"1h30m"`) and hour suffixes (`"1h"`) are not parsed — each is rejected with an `UnsupportedComposeError` rather than silently - truncated or misinterpreted. + truncated or misinterpreted. An explicit `null` (or an absent `interval`) + defaults to 1 second. - **`timeout`, `retries`, `start_period`:** each must be a number (int or - float) or string when present — the same shape `keys.is_number` enforces - for the legacy resource-limit keys. Each is passed straight through - `str(value)` into its `--health-*` flag with no further parsing, so a - mapping or list (e.g. `retries: {a: 1}`) raises at the gate - (`_validate_service_healthcheck`, `compose2pod/parsing.py`) instead of - emitting a literal Python `repr()` as the flag value. + float), a string, or `null` when present — the same shape `keys.is_number` + enforces for the legacy resource-limit keys, plus `null`. A mapping or list + (e.g. `retries: {a: 1}`) raises at the gate (`_validate_service_healthcheck`, + `compose2pod/parsing.py`) rather than reaching its `--health-*` flag as a + literal Python `repr()`. An explicit `null` is treated the same as an + absent key -- unset, so its `--health-*` flag is omitted entirely + (`_health_flags`, `compose2pod/emit.py`, keyed off the *value*, not key + presence). This matches `docker compose config`, which treats an + explicitly-null key as unset, and is the same treatment this package + already gives a null `environment`/`volumes`/`command` value elsewhere. - **Extension fields:** any `x-`-prefixed healthcheck key is accepted and ignored silently. - Everything else raises. @@ -673,10 +685,18 @@ operators (e.g. `${FOO!bar}`) is malformed and raises `UnsupportedComposeError` rather than silently dropping the trailing text. Tool/CLI-supplied values (`--project-dir`, `--image`, the pod name, the `--command` override) are literal and never interpolated. -`--artifact` must be in `SRC:DST` form; a value with no `:` raises -`UnsupportedComposeError` (`_validate_options`, `compose2pod/emit.py`), -guarding library callers of both `emit_script` and `referenced_variables` -as well as the CLI. The pod +`--artifact` must be a string in `SRC:DST` form; a non-string value or one +with no `:` raises `UnsupportedComposeError` (`_validate_options`, +`compose2pod/emit.py`), guarding library callers of both `emit_script` and +`referenced_variables` as well as the CLI (the CLI itself always supplies a +string — this guards a direct `EmitOptions` construction). `allow_exit_codes` +entries must each be an `int` (not `bool`, which is an `int` subclass in +Python but not a meaningful exit code) — `_validate_options` rejects +anything else, since each entry is interpolated unquoted into the generated +`case "$rc" in ...)` pattern; the CLI's `argparse` `type=int` already +guarantees this, so the check exists for a library caller passing +`EmitOptions` directly, where an unvalidated string would be shell injection, +not just a crash. The pod name is embedded into the pod-create line, the single-quoted `EXIT` trap, and the `-` store names (some of them unquoted), so it must be a shell-inert identifier — `emit_script` validates it against @@ -715,6 +735,17 @@ element-must-be-a-string check before `validate()` ever runs — `extends` resolution happens ahead of the gate (see Extends, above), so without its own check this same malformed input crashed raw there instead. +The long form's `condition` value gets the same treatment: it must be a +string (`graph.depends_on`), so a mapping or list condition (e.g. `depends_on: +{db: {condition: {a: 1}}}`) raises `UnsupportedComposeError` there instead of +crashing raw (`TypeError: cannot use 'dict' as a set element -- unhashable +type: 'dict'`) at the `condition not in DEPENDS_ON_CONDITIONS` set-membership +check that follows it in `parsing._validate_depends_on` — `in` against a +`set` hashes its operand, and only a `str` condition ever reaches that check. +This is checked in `graph.py`, not `parsing.py`, so every caller of +`depends_on` — not only `validate()` — gets the same protection, matching +every other depends_on shape check, which already lives there. + ## YAML anchors and merge keys Anchors (`&name` / `*name`) and the merge key (`<<:`) need no handling in diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index d02664b..b472438 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -711,3 +711,142 @@ normalization, null map values, normal service names (dotted/dashed/ underscored). `tests/conftest.py`'s `chats_compose` fixture and every `tests/integration/` scenario pass unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just check-planning` all clean. + +## Round 7: closing the residual gaps a final review found + +A seventh review, run against Round 6's tree, found one more raw crash, one +pre-existing silent-corruption bug the same review made a ruling on, one +contract violation in the deep sweep with a surviving mutant, and two minor +issues (changelog voice in `architecture/`, an unvalidated `EmitOptions` +field). + +| Finding | Before | Kind | +|---------|--------|------| +| `depends_on: {db: {condition: {a: 1}}}` (or any unhashable condition) | `condition not in DEPENDS_ON_CONDITIONS` (`parsing._validate_depends_on`) crashed raw — `in` against a `set` hashes its operand | crash | +| `healthcheck: {timeout: null, retries: null}` (explicit null scalar) | `_validate_service_healthcheck` already allowed `None`, but `emit._health_flags` keyed off key *presence*, not value, so it emitted `--health-timeout None` | silent corruption | +| `services: {x-dep: {...}, web: {depends_on: {x-dep: {3: 4}}}}` (a dependency/network/ulimit literally named `x-...`) | `_sweep_service` handed the whole `depends_on`/`networks`/`ulimits` mapping to `_require_string_keys_deep`, whose `x-` skip is only correct for content keys — the identifier `x-dep` was treated as an extension field and its malformed value never checked | contract violation, latent hole | + +### Unhashable `depends_on` condition (C1) + +Fixed in `graph.depends_on` (`compose2pod/graph.py`), not +`parsing._validate_depends_on`: `depends_on` already owns every other +depends_on shape check (list vs. mapping, each spec must itself be a +mapping), so a bad `condition` type belongs with them rather than split +across two modules. It also means every caller of `depends_on` — not only +`validate()` — gets the same protection; `emit.py` calls `depends_on` +directly too (`_plan`, `run_tokens`), so a library caller invoking +`emit_script` without calling `validate()` first is covered the same way. +A grep across `compose2pod/*.py` for every `in ` check against +compose-derived data found this was the only one testing membership in a +`set` — the module-level description above was verified, not assumed. + +### Null healthcheck scalar means unset, matching Docker (C2 — design decision) + +**Ruling:** an explicit `timeout: null` / `retries: null` / `start_period: +null` is treated as unset — `_health_flags` (`compose2pod/emit.py`) now +checks `healthcheck.get(key) is not None` instead of `key in healthcheck`, +so the flag is omitted the same way it would be for an absent key. This is +not a validate()-side rejection: `_validate_service_healthcheck` already +accepted `None` for these three keys before this round (the `is not None` +guard in its `is_number` check), so the gap was entirely on the emit side. +The ruling matches `docker compose config`, which treats an explicitly-null +key as unset, and matches this package's own existing treatment of a null +`environment`/`volumes`/`command` value elsewhere — recorded here as the +deliberate, permanent behavior, alongside I2's boolean-normalization ruling +above. `interval`'s existing `None` handling (`interval_seconds(None) == 1`, +a default rather than an omitted flag) was checked for consistency and left +alone: `interval` was never one of the three emitted `--health-*` scalar +flags in the first place — it only ever feeds the `wait_healthy` polling +loop — so there is no flag-omission behavior for it to match. + +### Identifier-keyed service keys are not extension fields (I1) + +`_require_string_keys_deep`'s own docstring already said callers must never +hand it a mapping whose keys are *identifiers* rather than content keys — +`_sweep_service` violated that contract for `depends_on`/`networks`/ +`ulimits`, the same identifier-vs-content-key conflation Round 6's F1 fixed +for service names and store definition names, one level deeper. Fixed by +honoring the docstring rather than changing it: `_sweep_identifier_map` +(`compose2pod/parsing.py`) checks each of these three keys' identifiers with +`require_string_keys` (no `x-` skip), then hands only each identifier's own +value to the ordinary `x-`-skipping deep walk. Not exploitable today +(nothing reads keys inside an unswept `depends_on`/`networks`/`ulimits` +subtree), but it was a latent hole with zero test defense and a live +contradiction between the code and its own stated contract. + +A second, independent gap in the same function: the `x-` skip *inside* +`_require_string_keys_deep` had no test that would fail if the skip line +were deleted — the existing `x-` tests covered only a top-level `x-` block +(never walked by the deep function at all) and a service-level `x-` key +(skipped one level up, by `_sweep_service`, before `_require_string_keys_deep` +ever sees it). `TestRequireStringKeysDeep::test_deep_x_prefixed_key_skip_is_load_bearing` +closes this with an `x-` key nested inside a service's `healthcheck` (a +region with no other validator that would independently reject the +malformed subtree) — confirmed by deleting the skip line and watching this +one test go red, before restoring it. + +### Minor: changelog voice in `architecture/`, unvalidated `EmitOptions` fields (M1, M2) + +`architecture/supported-subset.md`'s healthcheck section carried its own +history ("previously a non-mapping healthcheck reached `.get()` calls +downstream and crashed raw", "both used to reach `emit_script()`") — +`architecture/` documents present-tense behavior; that history now lives +only here, in the round that made each change. `EmitOptions.artifacts` +elements and `allow_exit_codes` entries were unvalidated past their type +annotations: a non-string artifact crashed raw on the `":" not in artifact` +check, and a non-int `allow_exit_codes` entry is interpolated unquoted into +the generated `case "$rc" in ...)` pattern — shell injection for a library +caller, not just a crash. Both are CLI-unreachable (`argparse` enforces +`str`/`int`) but reachable through a direct `EmitOptions` construction; +`_validate_options` (`compose2pod/emit.py`), the existing home for the +artifact `:` check, gained one more guard per field. + +### Is the gate closed now? + +Every reproduction given to this round now raises `UnsupportedComposeError` +(C1, I1) or emits the same output an absent key would (C2's ruling — not a +rejection) instead of crashing raw or silently corrupting the emitted +script. Nothing in this round widened what Round 6 already accepted: the +three DO-NOT-OVER-REJECT categories from Round 6 (`x-` blocks with +arbitrary payloads, `build`/top-level `volumes` skipped regions, a service +literally named `x-web`) were re-verified unchanged, and a dependency/ +network/ulimit identifier literally named `x-...` is still accepted as a +real identifier — only a malformed *subtree* under it is now rejected, +matching how a service named `x-web` is real but its malformed body still +isn't. + +One known gap remains, unaddressed by design rather than by oversight: +per-service `secrets`/`configs` references (list-of-mapping entries, e.g. +`secrets: [{source: db, x-note: {...}}]`) are swept as ordinary content, not +as identifier-keyed maps — a long-form reference has no identifier of its +own (`source` is a content key naming another entity, not the reference's +own name), so there is nothing analogous to `x-dep`/`x-net`/`x-lim` to +conflate. + +### Testing (Round 7) + +TDD throughout: each finding got a failing test confirming the exact crash +(C1 — reproduced the raw `TypeError` against the pre-fix tree), silent +corruption (C2 — confirmed `--health-timeout None` in the generated script +before the fix), or acceptance-that-should-be-rejection (I1) before the fix, +then a green test after. `tests/test_graph.py::TestDependsOn` (C1, direct +unit tests plus the pre-existing list-form coverage unchanged) and +`tests/test_parsing.py`'s `test_unhashable_depends_on_condition_raises_cleanly` +(C1, full pipeline). `tests/test_emit.py::TestRunFlags::test_healthcheck_explicit_null_scalars_omit_flags` +and `tests/test_parsing.py::TestValidate::test_healthcheck_scalars_accept_explicit_null` +(C2). `tests/test_parsing.py::TestIdentifierKeyedServiceKeysNotTreatedAsExtensionFields` +(I1(b), all three identifiers, plus confirming an `x-`-prefixed identifier +itself is still accepted) and `TestRequireStringKeysDeep::test_deep_x_prefixed_key_skip_is_load_bearing` +(I1(a), the mutant-kill test). `tests/test_emit.py::TestArtifactValidation` +and the new `TestAllowExitCodesValidation` (M2). Every Round 6 +DO-NOT-OVER-REJECT case was re-verified directly against `validate()`/ +`emit_script()` after all fixes landed, not just re-run as existing tests: +healthcheck with real int/string values, all three `depends_on` forms +including the empty long-form spec, `x-` blocks (top-level, in-service, and +now dependency/network/ulimit-identifier-adjacent) with arbitrary +non-string-keyed payloads, an anchor-from-`x-`-block merge still swept, the +`build`/top-level-`volumes` skipped regions, bool/null/int/float map values, +and a service named `x-web` still swept. `tests/conftest.py`'s +`chats_compose` fixture and every `tests/integration/` scenario pass +unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and +`just check-planning` all clean. From 9b06ab8f909f322898cfbe3fba80711092ca4c86 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 14:20:06 +0300 Subject: [PATCH 42/43] fix: validate the compose document inside _plan emit_script and referenced_variables are both public exports that project _plan's traversal, so a library caller could reach either without ever calling validate(). Doing so on a malformed document raised a raw KeyError/TypeError, or silently emitted a corrupted flag value (user: {a: 1} -> --user "{'a': 1}"). _plan now validates the document itself -- the one place neither entry point can route around -- discarding the warnings, which cli.py already prints from its own pass. validate() only reads and returns, so the repeated pass on the CLI path is side-effect-free. This closes the gap decisions/2026-07-10-reject-parse-dont-validate.md asserted was already closed; that record is corrected accordingly. Its conclusion stands and is now actually supported. --- architecture/supported-subset.md | 11 ++ compose2pod/emit.py | 12 ++ ...2026-07-13.10-close-structural-key-gate.md | 138 +++++++++++++++++- .../2026-07-10-reject-parse-dont-validate.md | 54 +++++-- tests/test_cli.py | 15 ++ tests/test_emit.py | 43 ++++++ 6 files changed, 259 insertions(+), 14 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index 59a9a20..f1a3b33 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -6,6 +6,17 @@ loudly rather than silently dropping behavior. `validate()` warns (ignored, behavior-neutral inside a single pod) or raises `UnsupportedComposeError`. +`emit_script()` and `referenced_variables()` (`compose2pod/emit.py`) are both +public exports, and both project the same internal `_plan` traversal; `_plan` +calls `validate()` itself, before reading anything else out of `compose`, and +discards the returned warnings (the CLI already printed its own copy from its +own `validate()` call — see Variable interpolation, below, for exactly which +notes/warnings the CLI prints and when). A library caller therefore cannot +reach either public entry point with a document `validate()` would reject — +calling `emit_script()`/`referenced_variables()` directly, without calling +`validate()` first, is exactly as safe as the CLI path, by construction of +the shared `_plan` call site, not by convention. + ## Top-level keys - **Supported:** `services` (required, non-empty mapping of service name to diff --git a/compose2pod/emit.py b/compose2pod/emit.py index 6d41021..489d9b0 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -11,6 +11,7 @@ from compose2pod.graph import depends_on, hostnames, startup_order from compose2pod.healthcheck import health_cmd, interval_seconds from compose2pod.keys import SERVICE_KEYS, Expand, Token, key_value_pairs +from compose2pod.parsing import validate from compose2pod.pod import pod_create_flags from compose2pod.resources import deploy_resource_flags from compose2pod.shell import to_shell, variable_names @@ -254,8 +255,19 @@ def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: service's `run_tokens` and the `pod_create_flags` render into lines *and* have their `Expand` variables collected, so the script and the variable list cannot disagree about what the script expands at run time. + + `emit_script` and `referenced_variables` are both public entry points that + project this traversal, so a library caller can reach either one without + ever calling `validate()` first. Validating `compose` here -- and nowhere + else -- makes both safe by construction: this is the one place a caller + cannot route around. `cli.py` also calls `validate()` itself (to print + warnings before emitting), so this repeats that pass; `validate()` only + reads `compose` and returns warnings, so the repeat is side-effect-free. + The warnings from this pass have no channel to reach a caller here -- + `cli.py` already printed its own copy -- so they are discarded on purpose. """ _validate_options(options) + _ = validate(compose) # re-validate; warnings already surfaced by the caller, if any if not POD_NAME_PATTERN.fullmatch(options.pod): msg = f"invalid pod name {options.pod!r}" raise UnsupportedComposeError(msg) diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index b472438..9ffef17 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -1,5 +1,5 @@ --- -summary: validate() rejects every malformed structural-key shape found across six review rounds; Round 4 hardened resolve_extends (compose2pod/extends.py), which cli.py runs before validate() and so never got a turn at the gate, against the same raw-crash and silent-coercion defects the gate itself was closed against, and normalized boolean map values (environment/labels/annotations/extra_hosts) like docker compose config instead of rejecting or repr()-ing them. Round 4's non-string-mapping-key guard was ten hand-placed require_string_keys() calls across three modules, not document-wide as claimed; Round 5 replaced it with one recursive sweep that walked the whole document ahead of every other check, closing two holes the hand-placed calls missed by construction (a non-string key f-string-interpolated into a flag value, and service names) but introducing two new defects of its own: a service literally named `x-*` escaped the sweep entirely (a regression), and the sweep over-rejected non-string keys in regions never read (`build`'s contents, top-level `networks`/`volumes`). Round 6 scoped the sweep to the regions validate() actually reads (`_sweep_document`/`_sweep_service`, compose2pod/parsing.py), closing both. See "Round 6" for the current, verified scope of what is closed versus what was not re-audited. +summary: validate() rejects every malformed structural-key shape found across seven review rounds (see "Round 7" for that scope); Round 8 closes the one remaining path around all of them — emit_script/referenced_variables, both public exports, are reachable directly without validate() ever running, so _plan (compose2pod/emit.py) now calls validate() itself and discards its warnings, making both entry points safe by construction rather than by cli.py's call-order convention. See "Round 8" for the current, verified scope of what is closed versus what was not re-audited. --- # Design: Close the structural-key gate @@ -850,3 +850,139 @@ and a service named `x-web` still swept. `tests/conftest.py`'s `chats_compose` fixture and every `tests/integration/` scenario pass unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and `just check-planning` all clean. + +## Round 8: the gate only guarded callers who called it + +Every prior round hardened `validate()`/`emit_script()` against a malformed +*value* reaching them — but never checked whether `emit_script` and +`referenced_variables` (`compose2pod/emit.py`), both public exports in +`compose2pod.__init__.__all__`, could be reached **without `validate()` +running first at all**. `cli.py` always calls `validate(compose)` before +`emit_script(compose=compose, ...)`, but that is a call-order convention in +one caller, not a property of `emit_script` itself. A library caller who +imports `emit_script`/`referenced_variables` directly and skips `validate()` +hits the identical gate every round above closed — for `cli.py`, but not for +itself. + +| Input | Before | Kind | +|-------|--------|------| +| `emit_script(compose={}, options=o)` | `KeyError: 'services'` (`emit.py`, `_plan`) | crash | +| `emit_script(compose={"services": {"web": {"image": "a", "cap_add": 3}}}, options=o)` | `TypeError: 'int' object is not iterable` (`keys.py`, the list-shaped registry emitter) | crash | +| `emit_script(compose={"services": {"web": {"image": "a", "user": {"a": 1}}}}, options=o)` | accepted; emits the literal `--user "{'a': 1}"` | silent corruption | +| `referenced_variables(compose={}, options=o)` | `KeyError: 'services'` (`emit.py`, `_plan`) — same gap, the other public entry point | crash | + +This is exactly the gap `decisions/2026-07-10-reject-parse-dont-validate.md` +named and then asserted closed: "a direct `emit_script(dict)` call on a +*malformed* document now fails with `UnsupportedComposeError`, not a raw +crash." That claim was false when written and, per Round 4 through Round 7 +above, stayed false through every round that believed it had closed the gate +— because every round hardened callers reached *through* `validate()`, and +none checked whether `validate()` itself was reachable from `emit_script`'s +own call graph. It was not. + +### The fix: `_plan` calls `validate()`, not `cli.py`'s convention + +`_plan` (`compose2pod/emit.py`) is the single traversal both `emit_script` +and `referenced_variables` project from — the same property Round 6's pod-name +check and Round 7's `EmitOptions` checks already relied on to guard both +entry points with one call site. `_plan` now calls `parsing.validate(compose)` +as its second statement (`_validate_options(options)` already ran first, +checking `EmitOptions` rather than `compose`), discarding the returned +warnings explicitly: + +```python +_validate_options(options) +_ = validate(compose) # re-validate; warnings already surfaced by the caller, if any +``` + +This makes both public entry points safe by construction — not because their +readers happen to be robust (the property every prior round chased and +repeatedly found incomplete), but because neither can reach a single line of +`_plan`'s traversal without `compose` having already passed the same gate +`cli.py` runs. `resolve_extends()` is unaffected: it is a separate public +entry point `cli.py` calls *before* `validate()`, by design (see Round 4's +"ahead of the gate" framing) — nothing about closing this gap moves or +duplicates that ordering. + +### No import cycle + +`compose2pod/parsing.py` imports from `stores`, `graph`, `healthcheck`, +`keys`, `pod`, and `resources` — the same six modules `emit.py` already +imports from. None of those six imports `parsing` or `emit` (verified by +grepping every module-level `from compose2pod...`/`import compose2pod...` +line in the package), so `emit.py` importing `validate` from `parsing` +introduces no cycle. The import is added at module level +(`from compose2pod.parsing import validate`, alongside `emit.py`'s other +`compose2pod.*` imports) — no in-function import, matching the repo's +absolute rule. + +### `validate()` is genuinely idempotent + +Calling `validate()` twice more per CLI invocation (once already in +`cli.py`, now once inside `emit_script`'s `_plan` and once inside +`referenced_variables`'s `_plan`) is only safe if `validate()` has no side +effects. Verified, not assumed: `validate()` and every function it calls +(`stores.validate`, `hostnames`, `uses_pod_options`, `_validate_service`, +...) only read `compose` and either raise or append to a `warnings` list +local to that call — a grep across `parsing.py` for mutation +(`compose[...] =`, `.pop`, `.clear`, `.setdefault`) and for printing +(`print`, `sys.std*`) found neither. Re-running `validate()` on the same +document three times in one CLI invocation produces the identical three +independent `warnings` lists and mutates nothing; `cli.py`'s own list is the +only one that ever reaches stderr, and `_plan`'s two internal calls discard +theirs on purpose — the `_ = validate(compose)` assignment above is +deliberate, not an oversight (ty/ruff would otherwise have no way to tell +"discarded intentionally" from "the return value was forgotten"). + +### The CLI still prints each warning exactly once + +Confirmed end to end against `chats_compose` (four warnings from +`application`'s `ports`/`restart`/`stdin_open`/`tty` and two from `db`'s +`ports`/`restart`): running `main()` through the CLI's `_target`/`--image` +path and inspecting `stderr` line-by-line shows each of the six warning +lines exactly once, not three times, despite `validate()` running three +times across `cli.py` + `emit_script` + `referenced_variables`. Locked in by +strengthening `tests/test_cli.py::TestMain::test_json_stdin_success_with_warnings` +to assert `err_lines.count(warning) == 1` for each of the six. + +### No existing test was emitting from a genuinely malformed document + +Every `emit_script`/`referenced_variables` call site across +`tests/test_emit.py` that doesn't already expect `UnsupportedComposeError` +was checked against the new gate by running the full suite, not by +inspection alone: all 114 tests in `tests/test_emit.py`, all 549 tests in +the full non-integration suite, and all 10 `tests/integration/` scenarios +(real podman) pass unchanged. No test document needed fixing — every +compose fixture the file's `TestEmitScript`/`TestReferencedVariables`/ +`TestProfiles`/`TestSecretsLifecycle`/etc. classes construct (including the +less-common shapes: mapping-form `ulimits` with a `{soft, hard}` bound, +mapping-form `extra_hosts`, list-form `dns`/`sysctls`, map-form +`annotations`, `platform`/`devices`/`pull_policy`) was already a document +`validate()` accepts cleanly. + +### Is the gate closed now? + +The decision record's central claim — a direct `emit_script(dict)` call on a +malformed document fails with `UnsupportedComposeError`, not a raw crash — +is true for the first time on this branch, and true by a different +mechanism than every prior round assumed: not because every shape-reading +function was individually hardened (Rounds 1-7's approach, and the one the +decision record credited), but because the one call site both public entry +points share now runs the gate itself before reading anything else. Rounds +1-7 remain necessary, not superseded: `validate()` is only as complete as +those rounds left it, and this round adds no new shape checks of its own — +it closes the one remaining path *around* all of them. + +### Testing (Round 8) + +TDD: `tests/test_emit.py::TestPublicEntryPointsValidateWithoutBeingToldTo` +adds one test per reproduction above (missing `services`, non-list `cap_add`, +non-string `user`, and `referenced_variables` on an empty document), each +confirmed to fail for the exact stated reason (`KeyError`, `TypeError`, "DID +NOT RAISE", `KeyError`) against the pre-fix tree before the `_plan` change, +then green after. `tests/test_cli.py`'s existing +`test_json_stdin_success_with_warnings` strengthened with an exact +once-per-warning count, run against `chats_compose`. `just test-ci` at 100% +line coverage (549 selected, 10 integration deselected), `just lint-ci`, and +`just check-planning` all clean. `just test-integration`: 10/10 passed +against real Podman 6.0.1. diff --git a/planning/decisions/2026-07-10-reject-parse-dont-validate.md b/planning/decisions/2026-07-10-reject-parse-dont-validate.md index d057924..e4ba53c 100644 --- a/planning/decisions/2026-07-10-reject-parse-dont-validate.md +++ b/planning/decisions/2026-07-10-reject-parse-dont-validate.md @@ -1,6 +1,6 @@ --- status: accepted -summary: Do not restructure the validate->emit seam as parse-don't-validate (a typed CheckedDocument that emit consumes); the registry + complete-gate changes already delivered the safety, and the residual type-enforcement gap is CLI-unreachable and not worth a typed-model rewrite plus a breaking API. +summary: Do not restructure the validate->emit seam as parse-don't-validate (a typed CheckedDocument that emit consumes); emit._plan now calls validate() itself, so both public emit entry points are safe by construction, and a typed-model rewrite plus a breaking API would buy nothing further. supersedes: null superseded_by: null --- @@ -25,21 +25,47 @@ Two of the review's candidates then shipped and changed the calculus: - The **service-key registry** (`changes/2026-07-09.08`) single-sourced each declarative key's validate + emit, so `emit` no longer re-derives shape knowledge. -- **validate() owning every shape emit reads** (`changes/2026-07-10.01`) made - the shape-reading functions robust: a direct `emit_script(dict)` call on a - *malformed* document now fails with `UnsupportedComposeError`, not a raw - crash, and `validate()` exercises every shape. +- **validate() owning every shape emit reads** (`changes/2026-07-10.01`) was + believed, at the time this decision was first written, to make the + shape-reading functions robust enough that a direct `emit_script(dict)` + call on a malformed document would fail with `UnsupportedComposeError`, + not a raw crash. That belief was wrong — see the note below. + +**Correction, added when the gap this decision names was actually closed** +(`changes/2026-07-13.10`, "Round 8"): the claim above was false when +written, and stayed false through seven further review rounds that each +hardened another shape-reading function and re-asserted it — because every +round hardened callers reached *through* `validate()`, and none checked +whether `validate()` itself was reachable from `emit_script`/ +`referenced_variables`'s own call graph. It was not: both are public +exports (`compose2pod.__all__`) a library caller can call directly, and +doing so on a malformed document reached a raw `KeyError`/`TypeError`, or +worse, silently emitted a corrupted flag value (e.g. `--user "{'a': 1}"` +for `user: {a: 1}`) — identical to what this decision assumed was already +fixed. The gap was closed not by further hardening individual readers, but +by giving `emit._plan` — the single traversal both public entry points +project from — its own call to `validate(compose)`, discarding the returned +warnings (the CLI already prints its own copy from its own `validate()` +call). This is a *mechanism* difference from what this decision originally +described, not a reopening of it: see below. ## Decision & rationale -After those two changes, parse-don't-validate's *unique* remaining benefit is -narrow: preventing a library caller from calling `emit_script` on a -**valid-but-unvalidated** dict (skipping warnings/normalization). That gap is: +After those two changes — and now, after the Round 8 correction above — +parse-don't-validate's *unique* remaining benefit is narrow: preventing a +library caller from calling `emit_script` on a **valid-but-unvalidated** +dict (skipping warnings/normalization). That gap is: - **CLI-unreachable** — the only product entry point always calls `validate()` before `emit_script()`. -- **Already de-risked for malformed input** — the robust readers reject bad - shapes at emit time regardless of whether `validate` ran. +- **Already de-risked for malformed input** — not because the shape-reading + functions are individually robust against every malformed input (that + claim was false, per the correction above), but because `emit._plan` + (`compose2pod/emit.py`) calls `validate(compose)` itself, before reading + anything else out of `compose`. Both public entry points that project + from `_plan` — `emit_script` and `referenced_variables` — are safe by + construction of that one call site, not by relying on `cli.py`'s + call-order convention or on every reader being individually hardened. Against that marginal gain, the cost is real: a typed `CheckedDocument` for the whole ~30-key subset, rewriting the just-built, 100%-covered registry so its @@ -60,6 +86,8 @@ also fits the zero-dependency, minimal-footprint ethos - A **second `emit` consumer** appears — a distinct output format alongside the pod script, or another module that renders from the compose model — so the typed model would pay back across more than one consumer; or -- the "`emit` only sees validated input" convention actually causes a bug (a - real caller emits unvalidated input and ships wrong output), not a - hypothetical one. +- `emit._plan`'s own `validate()` call (added to close this decision's gap — + see the Round 8 correction above) is bypassed or removed, and a real caller + emits unvalidated input and ships wrong output as a result — not a + hypothetical convention violation, an actual regression in the enforced + invariant. diff --git a/tests/test_cli.py b/tests/test_cli.py index 1d79230..46a0123 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -60,6 +60,21 @@ def test_json_stdin_success_with_warnings( assert out.out.startswith("#!/bin/sh") assert "podman pod create" in out.out assert "compose2pod:" in out.err + # main() calls validate() once for its own warnings, and emit_script / + # referenced_variables each call validate() again internally (closing + # the gate for direct library callers) -- each warning must still + # appear exactly once, not once per internal validate() call. + err_lines = out.err.splitlines() + expected_warnings = [ + "compose2pod: service 'application': ignoring 'ports'", + "compose2pod: service 'application': ignoring 'restart'", + "compose2pod: service 'application': ignoring 'stdin_open'", + "compose2pod: service 'application': ignoring 'tty'", + "compose2pod: service 'db': ignoring 'ports'", + "compose2pod: service 'db': ignoring 'restart'", + ] + for warning in expected_warnings: + assert err_lines.count(warning) == 1, err_lines def test_yaml_stdin_success(self, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch) -> None: yaml_text = "services:\n app:\n image: x\n" diff --git a/tests/test_emit.py b/tests/test_emit.py index 186f53e..4bc639e 100644 --- a/tests/test_emit.py +++ b/tests/test_emit.py @@ -952,3 +952,46 @@ def test_silent_when_podman_version_unparseable(self, tmp_path: Path) -> None: result = self._run_header(tmp_path, "exit 1") assert result.returncode == 0 assert result.stderr == "" + + +class TestPublicEntryPointsValidateWithoutBeingToldTo: + """Both public entry points must reject malformed input on their own. + + emit_script/referenced_variables are public exports; a library caller may + call either directly, skipping validate(). Both project the same `_plan` + traversal, so `_plan` itself must gate malformed input -- these documents + used to reach a raw crash or (worse) silently corrupt output. + """ + + def _options(self, target: str = "web") -> EmitOptions: + return EmitOptions( + target=target, + ci_image="ci", + command="", + pod="p", + project_dir=".", + artifacts=[], + allow_exit_codes=[], + ) + + def test_missing_services_key_raises_cleanly_not_a_keyerror(self) -> None: + with pytest.raises(UnsupportedComposeError): + emit_script(compose={}, options=self._options()) + + def test_non_list_cap_add_raises_cleanly_not_a_typeerror(self) -> None: + compose = {"services": {"web": {"image": "a", "cap_add": 3}}} + with pytest.raises(UnsupportedComposeError): + emit_script(compose=compose, options=self._options()) + + def test_non_string_user_is_rejected_not_silently_stringified(self) -> None: + # Before the _plan gate, this silently emitted the Python repr of the + # dict as the --user value: --user "{'a': 1}" -- corruption, not a crash. + compose = {"services": {"web": {"image": "a", "user": {"a": 1}}}} + with pytest.raises(UnsupportedComposeError): + emit_script(compose=compose, options=self._options()) + + def test_referenced_variables_is_equally_guarded(self) -> None: + # referenced_variables projects the same _plan traversal as emit_script + # and must reject malformed input identically. + with pytest.raises(UnsupportedComposeError): + referenced_variables({}, self._options()) From e65c2498e9a8a3bdf74676937a3374947ab0f6f4 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 14:25:49 +0300 Subject: [PATCH 43/43] docs: correct the public-surface and warning-order claims --- architecture/supported-subset.md | 12 ++++++------ compose2pod/emit.py | 10 +++++----- .../2026-07-13.10-close-structural-key-gate.md | 8 ++++---- .../2026-07-10-reject-parse-dont-validate.md | 6 ++++-- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index f1a3b33..90c771d 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -6,12 +6,12 @@ loudly rather than silently dropping behavior. `validate()` warns (ignored, behavior-neutral inside a single pod) or raises `UnsupportedComposeError`. -`emit_script()` and `referenced_variables()` (`compose2pod/emit.py`) are both -public exports, and both project the same internal `_plan` traversal; `_plan` -calls `validate()` itself, before reading anything else out of `compose`, and -discards the returned warnings (the CLI already printed its own copy from its -own `validate()` call — see Variable interpolation, below, for exactly which -notes/warnings the CLI prints and when). A library caller therefore cannot +`emit_script()` (exported from `compose2pod`) and `referenced_variables()` +(public as `compose2pod.emit.referenced_variables`) both project the same +internal `_plan` traversal; `_plan` calls `validate()` itself, before reading +anything else out of `compose`, and discards the returned warnings (the CLI +surfaces its own copy from its own `validate()` call). A library caller +therefore cannot reach either public entry point with a document `validate()` would reject — calling `emit_script()`/`referenced_variables()` directly, without calling `validate()` first, is exactly as safe as the CLI path, by construction of diff --git a/compose2pod/emit.py b/compose2pod/emit.py index 489d9b0..7e2a479 100644 --- a/compose2pod/emit.py +++ b/compose2pod/emit.py @@ -260,11 +260,11 @@ def _plan(compose: dict[str, Any], options: EmitOptions) -> PlannedScript: project this traversal, so a library caller can reach either one without ever calling `validate()` first. Validating `compose` here -- and nowhere else -- makes both safe by construction: this is the one place a caller - cannot route around. `cli.py` also calls `validate()` itself (to print - warnings before emitting), so this repeats that pass; `validate()` only - reads `compose` and returns warnings, so the repeat is side-effect-free. - The warnings from this pass have no channel to reach a caller here -- - `cli.py` already printed its own copy -- so they are discarded on purpose. + cannot route around. `cli.py` also calls `validate()` itself (to surface + warnings), so this repeats that pass; `validate()` only reads `compose` + and returns warnings, so the repeat is side-effect-free. The warnings from + this pass have no channel to reach a caller here -- `cli.py` surfaces its + own copy from its own call -- so they are discarded on purpose. """ _validate_options(options) _ = validate(compose) # re-validate; warnings already surfaced by the caller, if any diff --git a/planning/changes/2026-07-13.10-close-structural-key-gate.md b/planning/changes/2026-07-13.10-close-structural-key-gate.md index 9ffef17..b8cc457 100644 --- a/planning/changes/2026-07-13.10-close-structural-key-gate.md +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -854,10 +854,10 @@ unchanged. `just test-ci` at 100% line coverage, `just lint-ci`, and ## Round 8: the gate only guarded callers who called it Every prior round hardened `validate()`/`emit_script()` against a malformed -*value* reaching them — but never checked whether `emit_script` and -`referenced_variables` (`compose2pod/emit.py`), both public exports in -`compose2pod.__init__.__all__`, could be reached **without `validate()` -running first at all**. `cli.py` always calls `validate(compose)` before +*value* reaching them — but never checked whether `emit_script` (exported +from `compose2pod`) and `referenced_variables` (public as +`compose2pod.emit.referenced_variables`) could be reached **without +`validate()` running first at all**. `cli.py` always calls `validate(compose)` before `emit_script(compose=compose, ...)`, but that is a call-order convention in one caller, not a property of `emit_script` itself. A library caller who imports `emit_script`/`referenced_variables` directly and skips `validate()` diff --git a/planning/decisions/2026-07-10-reject-parse-dont-validate.md b/planning/decisions/2026-07-10-reject-parse-dont-validate.md index e4ba53c..a86fc23 100644 --- a/planning/decisions/2026-07-10-reject-parse-dont-validate.md +++ b/planning/decisions/2026-07-10-reject-parse-dont-validate.md @@ -37,8 +37,10 @@ written, and stayed false through seven further review rounds that each hardened another shape-reading function and re-asserted it — because every round hardened callers reached *through* `validate()`, and none checked whether `validate()` itself was reachable from `emit_script`/ -`referenced_variables`'s own call graph. It was not: both are public -exports (`compose2pod.__all__`) a library caller can call directly, and +`referenced_variables`'s own call graph. It was not: `emit_script` is +exported from `compose2pod`, `referenced_variables` is public as +`compose2pod.emit.referenced_variables`, and a library caller can call +either directly, and doing so on a malformed document reached a raw `KeyError`/`TypeError`, or worse, silently emitted a corrupted flag value (e.g. `--user "{'a': 1}"` for `user: {a: 1}`) — identical to what this decision assumed was already