fix: derive the extends merge policy from each key's validator#58
Merged
Conversation
resolve_extends runs ahead of validate(), so a merge that normalizes a value
decides what the gate then sees. The policy was a hand-maintained pair of key
sets, and it had drifted from the gate in both directions.
It laundered invalid input into valid: ulimits has no list form and the gate
refuses one, but _merge_map coerced `ulimits: ["nofile=2"]` into
{"nofile": "2"} before the gate ran. Same for every list-only key given a bare
scalar -- cap_add, devices, security_opt, group_add, volumes, secrets, configs
all refuse a scalar standalone and all accepted one via extends.
And it refused valid input: extra_hosts has a list form, accepted standalone,
but rejected on merge.
One rule now: a merge may normalize a value only through a form that key
actually has. Registry keys run spec.validate on both sides before spec.merge,
so the merge's accepted forms ARE the gate's, by construction. Structural
normalizers are narrowed to the forms Compose defines -- scalar only for
tmpfs/env_file, list form only for environment/extra_hosts/depends_on.
extra_hosts gets a real normalizer: its list form is colon-separated, so
pairs_to_mapping (which splits on '=') would have produced a single
{'host:ip': None} key. Splits on the first colon, so IPv6 survives.
A parametrized test pins the invariant for every mergeable key: a form the gate
refuses standalone must be refused through extends too. Verified to go red
without the fix.
…test real The base-side spec.validate reported the base's malformed value under the extending service's name. _merge now takes base_name and attributes each side's failure to the service the value belongs to. The parametrized invariant test was 10/11 vacuous: it paired the list-only keys with a dict, which the old code already refused, instead of the bare scalar that actually laundered. It now uses the laundering shape, covers all 17 mergeable keys, and asserts its own completeness so a new key cannot be added without a case. Against the pre-fix code it goes red on nine keys; before, only one.
…tural attribution The Extends section claimed 'nothing else is coerced' while omitting list-form labels/annotations, which are legitimately normalized through their own validator-approved list form. The 'error always names the owning service' claim had one reachable exception: spec.merge only receives the extending service's name, so a null ulimits in the base is reported against the child. Stated rather than overclaimed. Adds tests pinning base-side attribution on both structural paths (_as_mapping, _as_concat_list), which were correct but unpinned.
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 last item from the audit (#52) — the
extendsmerge-policy asymmetry deferred bydecisions/2026-07-12. Design:planning/changes/2026-07-14.05-extends-merge-policy.md.The asymmetry turned out not to be cosmetic. It was two bugs pointing in opposite directions, both caused by the merge policy being a hand-maintained enumeration rather than derived from what each key actually accepts.
It laundered invalid input into valid
cli.pycallsresolve_extends()beforevalidate(), so a merge that normalizes a value decides what the gate then sees.ulimitshas no list form and the gate refuses one:But through
extends,_merge_mapcoerced that list into{"nofile": "2"}before the gate ran — and it sailed through. Same for every list-only key given a bare scalar:cap_add,cap_drop,security_opt,devices,group_add,volumes,secrets,configsall refuse a scalar standalone and all silently accepted one viaextends. Nine keys in total.And it refused valid input
extra_hostshas a list form, accepted standalone — but a merge rejected it.There was a trap in the obvious fix:
extra_hosts' list form is colon-separated (host:ip), not=. Routing it throughpairs_to_mapping(which splits on=) would have silently produced a single{'host:ip': None}key. It gets a real normalizer, splitting on the first colon so IPv6 survives (myhost:::1→{'myhost': '::1'}).The rule
A merge may normalize a value only through a form that key actually has.
Registry keys run
spec.validateon both sides beforespec.merge, so their accepted forms are the gate's, by construction. Structural keys (noKeySpec) have narrowed normalizers, kept honest by a test asserting the invariant for every mergeable key — plus a companion test asserting the table covers them all, so a new key can't be added without a case. That drift is exactly what this fixes.No structural-key registry:
decisions/2026-07-12stands.Verification
Run against the pre-fix code, the invariant test goes red on nine keys — the laundering it now prevents. Review independently probed all 17 mergeable keys × ~16 shapes on both sides: zero laundering, zero raw crashes, and the branch's refusal set is a strict subset of
main's (no over-rejection).extra_hostsmerges emit--add-hostflags byte-identical to the equivalent non-extends document, IPv6 included.Errors now also name the service the offending value belongs to — a malformed value in the base is reported against the base, not the service extending it.
584 tests at 100% coverage; integration green against real podman.
🤖 Generated with Claude Code