diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index c7cd520..90c771d 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -6,15 +6,110 @@ loudly rather than silently dropping behavior. `validate()` warns (ignored, behavior-neutral inside a single pod) or raises `UnsupportedComposeError`. +`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 +the shared `_plan` call site, not by convention. + ## 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 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 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 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`, 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 + 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 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 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 @@ -40,15 +135,65 @@ 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, 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 + 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. 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. List elements must themselves be strings, and + 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 + 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 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 @@ -82,7 +227,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:`), @@ -94,9 +244,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 @@ -113,8 +265,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 @@ -167,10 +323,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. @@ -289,23 +454,44 @@ 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 — + `_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"]]`). - **`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), 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. ## 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. @@ -364,8 +550,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 @@ -427,7 +613,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: @@ -461,6 +647,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** @@ -500,7 +695,19 @@ 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 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 @@ -527,7 +734,28 @@ 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`. `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. + +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 diff --git a/compose2pod/emit.py b/compose2pod/emit.py index f4e4495..7e2a479 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 @@ -53,11 +54,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 @@ -221,6 +228,26 @@ 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 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 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: """Walk the target's dependency closure once, building the script and its variables. @@ -228,7 +255,22 @@ 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 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 + 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) @@ -275,9 +317,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/compose2pod/extends.py b/compose2pod/extends.py index 3f4817a..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): @@ -40,6 +46,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/compose2pod/graph.py b/compose2pod/graph.py index 7c7fed1..9b8b0ee 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" @@ -18,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 @@ -39,7 +56,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/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/keys.py b/compose2pod/keys.py index 167624e..769c05c 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -17,14 +17,44 @@ @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 +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. @@ -33,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) @@ -61,22 +91,60 @@ 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" 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) and not isinstance(val, bool): + msg = f"service {name!r}: '{key}' values must be a string, number, boolean, 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 @@ -101,7 +169,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" @@ -158,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 @@ -183,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/compose2pod/parsing.py b/compose2pod/parsing.py index f7d747d..2c1bbef 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -5,8 +5,8 @@ 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.keys import SERVICE_KEYS, STRUCTURAL_KEYS +from compose2pod.healthcheck import has_healthcheck, health_cmd, interval_seconds +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 @@ -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"} @@ -26,6 +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) + # 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 @@ -34,11 +40,27 @@ 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" + raise UnsupportedComposeError(msg) 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) @@ -53,19 +75,84 @@ 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_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_tmpfs(name: str, svc: dict[str, Any]) -> None: - """Check tmpfs is a string or list.""" - tmpfs = svc.get("tmpfs") - if tmpfs is not None and not isinstance(tmpfs, str | list): - msg = f"service {name!r}: tmpfs must be a string or list" +def _validate_command(name: str, svc: dict[str, Any]) -> None: + """Check the structural command key's form (it is not a registry key).""" + 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) + if isinstance(command, list): + _validate_argv_list(name, "command", command) + + +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(value, str | list): + msg = f"service {name!r}: '{key}' must be a string or list" + raise UnsupportedComposeError(msg) + if isinstance(value, list): + for entry in value: + if not isinstance(entry, str): + 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: + 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 of strings (emit iterates it).""" + _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 @@ -73,6 +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) + # 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-"): @@ -84,10 +176,14 @@ 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) + _validate_command(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(): @@ -96,6 +192,175 @@ 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. + + `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-...`). 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`/`_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. `_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}`): + 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) + + +_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. + + 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`). + + 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). `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) + + +def _sweep_document(compose: dict[str, Any]) -> None: + """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") + 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) + + 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(): @@ -117,6 +382,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) + # 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. + _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: @@ -127,6 +397,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/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 b9e56c2..67b6369 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 @@ -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: @@ -95,6 +96,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) @@ -106,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)}" @@ -125,6 +130,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/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..b8cc457 --- /dev/null +++ b/planning/changes/2026-07-13.10-close-structural-key-gate.md @@ -0,0 +1,988 @@ +--- +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 + +## Summary + +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 + +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*: + +> **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 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 + +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: 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** 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`, `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 — 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. + +**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 +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. + +## 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. + +**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. + +## 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. + +## 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. + +## 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` (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()` +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..a86fc23 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,49 @@ 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: `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 +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 +88,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 89ff259..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" @@ -221,6 +236,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: @@ -232,6 +273,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_emit.py b/tests/test_emit.py index ecd68c3..4bc639e 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") @@ -99,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")] @@ -162,6 +188,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 +208,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")] @@ -799,6 +833,88 @@ 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: + 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_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]": @@ -836,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()) diff --git a/tests/test_extends.py b/tests/test_extends.py index 6316969..a70c217 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"): @@ -215,6 +225,37 @@ 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_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": { @@ -224,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_graph.py b/tests/test_graph.py index e823241..fec2e6e 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -22,6 +22,37 @@ 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", + } + + 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: @@ -65,6 +96,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: 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_keys.py b/tests/test_keys.py index c399b5c..0382e57 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -4,10 +4,13 @@ from compose2pod.keys import ( SERVICE_KEYS, STRUCTURAL_KEYS, + Expand, _concat_list, _merge_map, _validate_list, _validate_ulimits, + pairs_to_mapping, + require_string_keys, validate_map, ) from compose2pod.parsing import SUPPORTED_SERVICE_KEYS @@ -17,6 +20,35 @@ 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] + + +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", @@ -84,6 +116,52 @@ 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"]) + + 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. @@ -112,3 +190,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} diff --git a/tests/test_parsing.py b/tests/test_parsing.py index ea696d6..e7647dc 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -84,14 +84,56 @@ 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": {}}) + 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": {}}) + 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) @@ -150,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) == [] @@ -282,6 +342,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}}}) == [] @@ -297,10 +369,81 @@ 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_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}". + 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_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"): + 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}}}) @@ -313,6 +456,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}}}}) == [] @@ -356,3 +512,400 @@ 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}}}) == [] + + 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_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"}]}}}) + + 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"): + 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}}}) == [] + + 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": "."}}}}) == [] + + 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"}}}}) + + 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"]}}}) == [] + + +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"]}}}) == [] + + 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. + + `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) == [] + + +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) + + +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) diff --git a/tests/test_pod.py b/tests/test_pod.py index dea7c21..c44bb55 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: @@ -115,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"], []) == [ 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 944bc33..600cd4b 100644 --- a/tests/test_stores.py +++ b/tests/test_stores.py @@ -60,6 +60,28 @@ 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_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])) @@ -93,6 +115,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 +137,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"]))