fix: close the structural-key gate#53
Merged
Merged
Conversation
Check that each element in env_file list is a string, consistent with _validate_service_volumes. Prevents TypeError from reaching emit when env_file contains non-string values like env_file: [5].
'image' is a structural key with no KeySpec, so a service with neither 'image' nor 'build' reached image_for and crashed with KeyError: 'image'; a non-string image (e.g. image: 5) reached shell.py and crashed with TypeError. Both now raise UnsupportedComposeError at validate() instead.
'command' is command_tokens' sibling of 'entrypoint', which already has _validate_entrypoint; command had no validator at all. A non- string/non-list command (e.g. command: 5) crashed emit with TypeError: 'int' object is not iterable. A mapping command was worse: it was silently accepted and then mis-emitted -- only the mapping's key reached podman run, the value was dropped. Both now raise UnsupportedComposeError at validate(), mirroring _validate_entrypoint.
_validate_tmpfs only checked the outer shape (string or list); a non-string element (e.g. tmpfs: [5]) still reached emit and crashed in shell.py. Carries the same element-level check already applied to env_file (commit 61084dc) over to tmpfs.
validate() itself walked services.items() before checking its shape, so services: "app" or services: ["app"] crashed raw with AttributeError: 'str'/'list' object has no attribute 'items' inside validate(), not even reaching emit.
networks.<name>.aliases was extended into the hostname/alias set
without a shape check, so networks: {default: {aliases: "abc"}} was
destructured character-wise and silently emitted --add-host
a:127.0.0.1 --add-host b:127.0.0.1 --add-host c:127.0.0.1 -- the same
class of bug already fixed for volumes. aliases must now be a list
of strings.
Only 'interval' was shape-checked (via interval_seconds); timeout,
retries, and start_period were passed through unchecked into
str(value), so healthcheck: {retries: {a: 1}} was silently accepted
and emitted the literal garbage --health-retries "{'a': 1}". All
three now must be a number or string, reusing keys.is_number.
POD_NAME_PATTERN was only checked in emit_script, so the other public entry point, referenced_variables, skipped it entirely. Moves the check into _plan -- the single traversal both entry points project from -- alongside the _validate_options call already there, so both public entry points are guarded by the same check and message.
Updates architecture/supported-subset.md's shape contracts for image, command, tmpfs, network aliases, healthcheck scalars, and top-level services to match the code. Updates the change file's summary and body to describe the realized, broader result -- the gate is closed as a class, not just the original four holes.
_validate_command rejected an explicit null command (command: null in YAML), but emit.command_tokens accepts it to mean "no command." This was over-rejection compared to the other validators (_validate_environment, _validate_tmpfs, etc.) which skip None correctly. Now _validate_command skips None but still rejects genuinely wrong shapes (int, mapping).
Expand is the chokepoint every emitted token flows through on its way to shell.to_shell/variable_names, both of which crash raw (TypeError) on a non-str value. Any per-key validator gap that let a non-str leak into emit used to surface as an unhandled crash instead of a clean UnsupportedComposeError, no matter which key was responsible. Guard it once at construction instead of trusting every call site.
healthcheck.test was validated for mapping shape and key names but never for the test value's own shape, so a malformed test (e.g. a nested list as CMD-SHELL's argument) reached emit and crashed raw instead of failing at the gate. Tighten health_cmd() itself to check CMD-SHELL/CMD argument types (dropping the ty: ignore that was hiding the gap) and call it from _validate_service_healthcheck so the same check runs at validate() time, not only when emit.py calls it later.
_validate_list and validate_map only checked the outer list/dict shape, so a non-string list element or non-scalar map value reached emit and got str()'d/repr()'d straight into the script -- exit 0, garbage output, no error. The commonest trigger is the classic Compose YAML slip of mixing list and map form (`- KEY: value` instead of `- KEY=value`). Tighten both shared validators to check elements/values, which covers labels, annotations, cap_add, cap_drop, group_add, security_opt, devices, extra_hosts, and environment in one edit (all route through one of the two). command/entrypoint don't go through the registry, so add the matching list-element check locally in parsing.py. List elements must be strings; map values must be a string, number, or null (null keeps its existing per-key meaning -- host-passthrough for environment, empty label for labels/annotations).
depends_on's list (short) form passed entries straight into
dict.fromkeys, which crashed with a raw TypeError (unhashable type)
on the same list/map YAML slip (`- db: {condition: ...}` instead of
a bare service name) already fixed for environment/command/labels.
Unlike those, this one crashed from inside validate() itself, via
graph.depends_on's normalization. Reject it with the same local style
depends_on already uses for its other malformed shapes.
_validate_tmpfs and _validate_env_file were the same "string or list of strings" rule written twice with slightly different messages. Extract _validate_string_or_string_list and quote tmpfs's key name in its message to match every other key's error format.
The change file's "rejects every malformed structural-key shape" claim was false when three more review rounds kept finding the same defect class -- update it to describe what actually closed this round (the Expand type-guard chokepoint, healthcheck.test's shape, depends_on's list-element shape, and the shared list/map validator tightening) and state precisely what remains scoped rather than re-claiming victory. architecture/supported-subset.md: fix the false command/entrypoint null equivalence (command: null is accepted, entrypoint: null is rejected -- pre-existing, unchanged here), and document the new rejections and the Expand guard's role.
Reject a non-string `target` in service secret/config long-form references with UnsupportedComposeError instead of silently accepting it and letting it pass through to the emitted script. Both secrets and configs now validate that `target`, when present, must be a string. Configs continue to require that string targets be absolute paths. Update architecture/supported-subset.md to document that `target` must be a string when present in long-form store references.
resolve_extends runs ahead of validate() (called from cli.py before the
gate), so its own list-to-mapping normalization for depends_on needs its
own element check. Without it, a list-of-mapping depends_on entry (the
same list/map YAML slip graph.py already rejects cleanly outside
extends) crashed raw with TypeError: unhashable type: 'dict' building
{dep: {} for dep in value}.
pairs_to_mapping backs the merge policy for environment/labels/ annotations/ulimits under extends. Its list branch used str(item) on each element, so a non-string element (the same list/map YAML slip validate() rejects everywhere else) was laundered into a mapping key instead of being rejected -- the merged service came out well-formed enough for validate() to accept and emit to render a literal Python repr as a flag value.
validate() assumed every mapping key is a str, but PyYAML routinely produces int/bool keys (a bare 3: or, under YAML 1.1, on:/off:). A non-string key crashed raw wherever the gate first sorted or regex- matched the mapping: the top-level document, a service's own keys, a healthcheck's keys, and a secrets/configs definition name. One shared helper, keys.require_string_keys, closes all four sites with a single clean UnsupportedComposeError naming the offending key.
environment/labels/annotations/extra_hosts map values must be a string,
number, boolean, or null again -- the branch's list/map hardening had
started rejecting a bool value outright, an over-rejection regression
versus `docker compose config`, which normalizes `DEBUG: true` to the
string "true" rather than refusing it. key_value_pairs/extra_host_pairs
now render a bool lowercase ('true'/'false') instead of leaking
Python's str(True) == 'True' into the flag value.
_extends_target's sorted(unknown) crashed raw (TypeError: '<' not supported between instances of 'str' and 'int') when a malformed 'extends' mapping mixed a non-string key with a string one. extends.py runs ahead of validate()'s gate, so this is exactly the kind of hostile input the gate never gets a turn on. Part of the extends.py audit C1 calls for.
bool IS an int in Python, so _validate_ulimits's isinstance(spec, int | str) let a boolean nofile/soft/hard through, and emit rendered the literal Python repr (--ulimit "nofile=True"). Unlike environment's boolean (which Docker normalizes to a string), a boolean ulimit has no sensible normalization, so it is rejected -- following the precedent stores._check_long_form_scalars already sets for uid/gid/mode.
C2 closed the 4-5 sites where the gate itself walks a mapping's keys directly, but sorted(unknown) at every "reject unrecognized keys" call site has the identical crash: a set of mixed-type keys (a non-string key alongside a string one) is unorderable. Reproduced and fixed the remaining sites that are reachable through validate() on a hostile document: a secret/config definition's own keys and a long-form service reference's own keys (compose2pod/stores.py), and deploy / deploy.resources / deploy.resources.limits / deploy.resources.reservations (compose2pod/resources.py). Same require_string_keys helper C2 introduced, called one step earlier so the non-string key is reported before the unrecognized-keys check ever runs.
planning/changes/2026-07-13.10-close-structural-key-gate.md's frontmatter and Round 3 text claimed the raw-crash half was closed "no matter which key is responsible" -- false, since extends.py runs ahead of validate() and was never audited, and non-string mapping keys crash through a different mechanism (sorted()/startswith) than the Expand chokepoint covers. Added a Round 4 section stating precisely what this round verified closed, what remains unverified, and recording the I2 boolean normalization ruling as a deliberate design decision. architecture/supported-subset.md: documented the string-keys requirement, boolean map-value normalization, ulimits' boolean rejection, and fixed the stale claim that labels/annotations/ulimits list-form merges under extends are refused rather than coerced.
…weep The gate's non-string-mapping-key guard was six-turned-ten hand-placed require_string_keys() calls, wired into whichever mappings some other check happened to sorted()/startswith() -- never a walk of the document. Two holes followed: a mapping whose keys get f-string-interpolated straight into a flag value (environment/labels/annotations/sysctls/ extra_hosts/ulimits) leaked a YAML-1.1 bool's or int's Python repr into the emitted script instead of raising, and the services mapping's own keys (a service name) were never checked at all, reaching --add-host and --name verbatim. Replace the hand-placement with one recursive sweep, parsing._require_string_keys_deep, run once at the top of validate() before every other check: it walks the whole compose document and requires every mapping key, at every depth, to be a string, skipping recursion into x--prefixed subtrees (their arbitrary payloads are accepted and ignored by design). This closes both holes without any change to keys.py/pod.py, the modules that actually interpolate the keys, and covers any future structural key by construction rather than by someone remembering to wire in a call. Delete the three hand-placed calls the sweep provably subsumes (top-level document, service body, healthcheck -- all reached only through validate(), confirmed by deleting them and rerunning the suite). Keep the seven in resources.py/stores.py: their tests call validate_deploy()/stores.validate() directly, bypassing validate()'s sweep, and crash raw without their own guard. Confirmed resolve_extends() -- which cli.py runs ahead of validate() -- still passes every hostile-key shape through unchanged rather than crashing raw, so no extends.py change was needed this round.
The non-string-key sweep's x- skip is syntactic (key.startswith("x-"))
but its rationale is semantic ("a subtree we ignore"); on the services
mapping's own keys those diverge, since a service name is an identifier,
not an extension-field marker. validate() iterates services.items() with
no x- filter, so a service literally named x-web is a real service whose
body was escaping the sweep entirely -- reaching a raw TypeError crash via
sorted(svc)/sorted(healthcheck), or silently leaking a YAML-1.1 bareword
key's Python repr into the emitted script (`-e "True=1"`) with validate()
returning clean.
_sweep_document/_sweep_service replace the single _require_string_keys_deep
call with an orchestration that always sweeps every services/secrets/configs
entry by name, regardless of what the name looks like -- the same fix
applies to store definitions, whose names aren't excluded from `x-` either.
Also restores the two require_string_keys calls Round 5 deleted from
_validate_service/_validate_service_healthcheck as belt-and-braces, matching
the "own contract independent of caller" reasoning already used for
resources.validate_deploy/stores.validate.
The non-string-key sweep walked the whole document, skipping only x- prefixed subtrees. build's contents and the ignored top-level networks/volumes blocks are not x-prefixed, so the sweep rejected non-string keys inside them too, even though compose2pod never reads either region: image_for never reads a service's build contents (the CI image always wins), and top-level networks/volumes are accepted and warned, never inspected past the membership check. A non-string key there can never reach the generated script, so rejecting one was pure over-rejection of input Docker accepts. _sweep_service now skips 'build' alongside the service's own x- keys. Everything the sweep previously rejected correctly -- environment/labels/ ulimits/service names/store names, etc. -- is still rejected identically; only build/top-level networks/volumes moved from rejected to accepted.
_require_string_keys_deep's `elif isinstance(node, list)` branch was reachable (line-covered by existing tests exercising service-level secrets/configs references) but nothing asserted on its effect, so deleting the branch left every other test green -- "does the sweep recurse into lists-of-mappings?" was unfalsifiable by the suite. Confirmed by deleting the branch: the new test goes red because the sweep's own message stops appearing and stores.py's independent, differently-worded check (unaffected by the deletion) raises instead, which the test's match= no longer matches. Branch restored afterward, confirmed byte-identical to before this test was added.
architecture/supported-subset.md's non-string-mapping-key bullet claimed the sweep applied "uniformly to every mapping in the document" -- false in both directions after this round's fixes: a service named x-* is swept (the bullet never distinguished a NAME from a content key), and build's contents / top-level networks/volumes are explicitly not swept (never read, so a non-string key there can't reach the script). Rewrite states the true scope: which regions are swept, which are skipped and why, and that a service named x-* is a service. planning/changes/2026-07-13.10-close-structural-key-gate.md gets a Round 6 section recording both findings, the fix, and the anchor-merged-from-x- block edge case re-verified against the fix.
condition not in DEPENDS_ON_CONDITIONS (parsing._validate_depends_on) hashes its operand, so an unhashable condition (e.g. a dict or list) crashed raw with TypeError: unhashable type instead of failing clean. Checked in graph.depends_on, which already owns every other depends_on shape check, so every caller -- not just validate() -- gets the same protection.
_health_flags keyed --health-timeout/--health-retries/--health-start-period off key presence, not value, so an explicit `timeout: null` (already accepted by validate()) emitted the literal string 'None' into the generated script. Ruling: null means unset, matching `docker compose config` and how this package already treats a null environment/volumes/ command value -- omit the flag entirely, keyed off `.get(key) is not None`.
…fields _sweep_service handed depends_on/networks/ulimits's whole mapping form to _require_string_keys_deep, whose x- skip is only valid for content keys (per its own docstring) -- a dependency/network/ulimit literally named x-foo was treated as an extension field, so a malformed subtree under it escaped the sweep. _sweep_identifier_map checks these identifiers with require_string_keys (no x- skip) and hands only each identifier's own value to the ordinary x--skipping deep walk, honoring the contract instead of changing it. Also adds a test pinning the x- skip inside _require_string_keys_deep itself, which no existing test would catch if deleted (deleting the line was verified to turn the new test red; restoring turns it green).
CLI-unreachable (argparse enforces str/int) but a library caller can construct EmitOptions directly: a non-string artifact crashed raw on the ':' membership test, and a non-int allow_exit_codes entry is interpolated unquoted into the generated `case "$rc" in ...)` pattern -- shell injection, not just a crash. _validate_options gains one guard per field.
architecture/supported-subset.md: makes the depends_on section's raw-crash
promise true for the long-form condition case too (C1); documents the
null-healthcheck-scalar-means-unset ruling (C2) in place of the stale
"rejects a mapping/list" claim; documents identifier-keyed service keys
(depends_on/networks/ulimits) getting the same name-not-content sweep
treatment as service/store names (I1); documents the EmitOptions.artifacts/
allow_exit_codes guards (M2). Drops the healthcheck section's changelog
voice ("previously ... crashed raw", "used to reach emit_script()") --
that history now lives only in planning/changes (M1).
planning/changes/2026-07-13.10-close-structural-key-gate.md: adds a Round 7
section recording all five findings, the C2 ruling alongside I2's existing
boolean-normalization ruling, and the re-verified DO-NOT-OVER-REJECT
scenarios.
emit_script and referenced_variables are both public exports that project
_plan's traversal, so a library caller could reach either without ever
calling validate(). Doing so on a malformed document raised a raw
KeyError/TypeError, or silently emitted a corrupted flag value
(user: {a: 1} -> --user "{'a': 1}").
_plan now validates the document itself -- the one place neither entry
point can route around -- discarding the warnings, which cli.py already
prints from its own pass. validate() only reads and returns, so the
repeated pass on the CLI path is side-effect-free.
This closes the gap decisions/2026-07-10-reject-parse-dont-validate.md
asserted was already closed; that record is corrected accordingly. Its
conclusion stands and is now actually supported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the gate's central contract: anything malformed raises
UnsupportedComposeError; nothing malformed reaches the emitter to crash raw or be silently mis-emitted.Spawned by
planning/audits/2026-07-13-docs-and-gate-audit.md(#52). Design:planning/changes/2026-07-13.10-close-structural-key-gate.md.Why
decisions/2026-07-10-reject-parse-dont-validate.mdrejects a typed-model refactor and justifies itself on the premise that this invariant already holds. It didn't — and the false premise survived because every hardening pass fixed readers reached throughvalidate(), while nobody checked whethervalidate()was reachable fromemit_script's own call graph. It wasn't.What was actually broken
Two failure classes, both CLI-reachable:
Raw crashes.
environment: "FOO=bar"→AttributeError.env_file: 5→TypeError.depends_on: {db: {condition: {a: 1}}}→TypeErrorfrom insidevalidate(). A service with noimage/build→KeyError.Silent corruption — exit 0, garbage in the script. The worse half:
volumes: "/data:/data"was iterated character-wise;volumes: "/"was silently accepted and emitted-v "/".environment: {DEBUG: true}emitted-e "DEBUG=True"— a Pythonrepr().on:into a bool, soenvironment: {on: 1}emitted-e "True=1", and a service namedon:emitted--name test-pod-True.healthcheck: {timeout: null}emitted--health-timeout None.The two structural causes
Patching keys one at a time kept finding more. The fix is structural:
resolve_extends()runs beforevalidate()incli.py, soextends.pysat ahead of the gate entirely and had never been hardened. It also laundered bad values into well-formed shapes the gate then waved through.x-blocks,buildcontents, and the ignored top-levelnetworks/volumes, since a key that cannot reach the script cannot corrupt it.Plus an
Expand.__post_init__type guard as the chokepoint for the raw-crash half, and_plannow callsvalidate()itself, so both public emit entry points are safe by construction rather than by call-order convention.decisions/2026-07-10is corrected accordingly — its conclusion stands and is now actually supported.Deliberate design rulings (documented with rationale)
DEBUG: true→-e DEBUG=true.{3306: db}is refused though Docker accepts it. Normalizing would not reproduce Docker (YAML 1.2 keepsona string →on=1; normalizing Python's bool givestrue=1, a different wrong answer).docker compose config.Verification
549 tests at 100% line coverage;
lint-ciandcheck-planningclean; integration suite green against real Podman 6.0.1.Seven adversarial whole-branch review rounds. The final round ran every service key × 19 hostile values through
resolve_extends → validate → emit_script, probed shell injection through every user-controlled value, and killed 15/15 planted mutants — returning zero code findings.🤖 Generated with Claude Code