fix: close developer-facing devcontainer spec surface gaps#57
Merged
Conversation
Forward the CLI-provided docker binary path to the CliDocker instance used for container discovery; previously the flag was accepted only for parity and silently ignored, breaking spec §3 / §5 expectations that `docker_path` is the binary used for `docker inspect`. Drops the matching dead_code attribute, refreshes comments that still claimed the features-based merged-config flow was blocked or a placeholder, and documents why feature entrypoint scripts intentionally stay out of the merged-configuration output (they are chained at container start by build_entrypoint_chain and are not part of the read-configuration data model). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
read-configuration's mergedConfiguration JSON now matches the devcontainers/cli reference (`src/spec-node/imageMetadata.ts` `mergeConfiguration`): the singular `entrypoint`/`onCreateCommand`/ `updateContentCommand`/`postCreateCommand`/`postStartCommand`/ `postAttachCommand` fields are stripped from the merged output and re-emitted as plural arrays (`entrypoints`, `onCreateCommands`, ...) collected in declaration order from each metadata source. The container-label path collects from each label entry; the features path collects per resolved feature then appends the base config's own collected fields (matching upstream `getDevcontainerMetadata` which appends the user config last). The shape helper omits a plural field entirely when no entry contributes a value. This closes the spec-parity gap the in-tree DATA-STRUCTURES.md had papered over by listing only the singular subset; the file is updated to document the upstream-aligned shape. Tests cover: - Container metadata: entrypoints array, plural lifecycle arrays, absence of singular names - No-metadata path: base config lifecycle commands surface as plurals - Direct unit test of apply_upstream_merge_shape edge cases (null skip, omitted-when-empty, non-collected fields preserved) Other spec parity items noticed during this review and NOT fixed here: upstream uses set-union for capAdd/securityOpt and boolean-OR for init/ privileged, while Deacon's ConfigMerger is last-wins per field. These remain follow-up items for issue #56. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConfigMerger::merge_configs was concatenating capAdd and securityOpt across config layers (concat_string_arrays), allowing duplicates to accumulate when the same capability appeared in base + extends + image metadata + features. Upstream devcontainers/cli mergeConfiguration uses unionOrUndefined (`src/spec-node/imageMetadata.ts`) — set-union with first-seen ordering — which is also what `up`'s runtime security path already does via crates/core/src/security.rs. Switching to the existing union_arrays helper brings ConfigMerger in line with both: the cap_add / security_opt entries that surface in read-configuration's mergedConfiguration JSON are now deduped while preserving declaration order across the metadata chain. Tests cover overlapping base/overlay entries, multi-layer fold (extends-like chain), and the end-to-end read-configuration container path with overlapping label entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConfigMerger::merge_configs was deduping mounts via union_arrays, which keys on whole-entry PartialEq. Mounts with the same container-side target but different source/type slipped through as separate entries in mergedConfiguration JSON. Upstream devcontainers/cli mergeConfiguration calls mergeMounts which keys dedupe on mount.target and keeps the LAST occurrence per target in declaration order (`src/spec-node/imageMetadata.ts`). The runtime up path already did this via crates/core/src/mount.rs::merge_mounts; this just brings the read-configuration output path in line. Adds two helpers in crates/core/src/mount.rs: - extract_mount_target: pulls the container-side target from a JSON mount value, supporting object form (target/destination/dst keys) and string form (Docker --mount syntax and short volume syntax). - union_mounts_by_target: per-target last-wins dedupe across two mount lists, preserving original declaration order for survivors; mounts with no extractable target pass through verbatim. ConfigMerger uses the new helper for the mounts field. forwardPorts keeps the existing union_arrays semantics (whole-entry equality is correct there). Tests: - 11 unit tests in mount::target_dedup_tests covering object/string forms, alias keys (dst/destination), volume syntax, intra-list and cross-list dedup, surviving-order preservation, unparseable passthrough, and empty inputs. - End-to-end test in read_configuration::tests verifying the container-label path emits a single mount per target with last occurrence's source winning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConfigMerger::merge_configs was deduping forwardPorts via union_arrays
on PartialEq, which treats PortSpec::Number(3000) and
PortSpec::String("localhost:3000") as distinct entries. Upstream
mergeForwardPorts (devcontainers/cli/src/spec-node/imageMetadata.ts)
normalizes both forms to a `localhost:N` key for set dedup, then
converts surviving `localhost:<u16>` keys back to Number(N) in the
final output. Without normalization the merged JSON could carry the
same port twice in different representations.
Adds ConfigMerger::union_forward_ports that mirrors the upstream
semantics:
- Number(N) and String("localhost:N") share a dedup key and surface
as Number(N).
- All other string forms (e.g. "3000:3000", "127.0.0.1:3000", and
plain "3000") remain distinct from Number(N), as upstream only
normalizes the `localhost:` prefix.
- First-seen wins for ordering, preserving declaration position of
survivors.
ConfigMerger::merge_two_configs now uses union_forward_ports for the
forward_ports field; union_arrays stays in place for other Vec fields
where whole-entry equality is correct.
Tests:
- 5 unit tests in config::tests covering the new helper directly:
Number ↔ "localhost:N" dedup, "localhost:N"-only canonicalization,
plain-number vs Number distinct, order preservation, malformed
localhost passthrough.
- 1 end-to-end test through merge_configs verifying a 3-layer chain
collapses Number / "localhost:N" pairs from any layer.
- 1 read-configuration test verifying the container-label path emits
upstream-shape forwardPorts after dedup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
read-configuration's mergedConfiguration was deep-merging customizations
across the metadata chain into a single object. Upstream
mergeConfiguration (devcontainers/cli/src/spec-node/imageMetadata.ts)
instead collects each contributor's customizations into per-tool arrays
— { vscode: [c_from_entry1, c_from_entry2, ...], jetbrains: [...] } —
leaving merging within each slot to the consuming tool. The upstream
spec docs make this explicit: "Merging is left to the tools."
compute_merged_configuration now tracks each metadata source's
customizations object independently, then applies a final shape
transformation that strips the deep-merged customizations from
ConfigMerger's output and replaces it with the per-tool array form.
Empty contributors are skipped; when no source contributes any
customizations the field is omitted entirely (matching upstream
`Object.keys(customizations).length ? customizations : undefined`).
Source ordering matches upstream's getDevcontainerMetadata /
getImageMetadataFromContainer:
- Container-label path: collect from each label entry only. Base
config does NOT contribute (per pickUpdateableConfigProperties
which omits customizations).
- Features path: collect per feature's metadata.customizations, then
append base config as the trailing entry.
- Empty path: base config only (single-entry arrays).
ConfigMerger's underlying deep-merge of customizations is preserved
for other callers; the per-tool array form is a read-configuration
output transformation only. DATA-STRUCTURES.md is updated to document
the new shape and source ordering.
Tests (5):
- Unit tests for apply_customizations_shape: first-seen tool order,
per-tool entry order, empty-entry skip, deep-merge stripped.
- End-to-end container-label test: array per tool, base config not
contributing.
- End-to-end base-only test: single-entry per-tool arrays.
- End-to-end no-customizations test: field omitted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
compute_merged_configuration's features path was overwriting derived_config.init and derived_config.privileged on every iteration: the LAST feature contributing the field won. Upstream's mergeConfiguration uses imageMetadata.some(entry => entry.X), which is a strict OR across all entries — any Some(true) sticks, regardless of declaration position. The bug-trigger sequence is straightforward: feature A sets init=true, a later feature B sets init=false, derived_config.init ends up Some(false), and the final merged output reports init: false even though a feature explicitly requested init. capAdd / securityOpt already accumulate correctly via Vec::extend + ConfigMerger's union_arrays dedupe (commit 81fb5d5). Extract a small or_merge_bool helper (Option<bool> truth table: identity for None, OR for two Somes) and use it for both init and privileged in the features loop. Helper is private to the module and unit-tested directly — full features-path integration would require mocking the OCI manifest+blob fetch, which is significantly more boilerplate for a one-line accumulation fix. Tests (2): - or_merge_bool truth table covering all (Option<bool>, Option<bool>) combinations. - Regression: simulating the features-loop sequence with a true→false transition confirms OR is applied (last-wins would have returned Some(false)). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConfigMerger::merge_two_configs was using last-wins for
host_requirements (a whole-struct overlay.or(base)), so a config layer
specifying only `cpus: 8` would clobber another layer's `memory: "4GB"`.
Upstream mergeHostRequirements
(devcontainers/cli/src/spec-node/imageMetadata.ts) takes the per-field
max across all metadata entries:
cpus = Math.max(...entries.map(e => e.hostRequirements?.cpus || 0))
memory = Math.max(...entries.map(e => parseBytes(e.hostRequirements?.memory || '0')))
storage = same as memory
gpu = entries.reduce(mergeGpuRequirements, undefined)
mergeGpuRequirements:
- undefined/false either side → other side wins
- both "optional" → "optional"
- otherwise treat each as object form (non-object → {}) and take
max(cores), max(memory) across the pair
Implementation adds private helpers on ConfigMerger:
- merge_host_requirements: orchestrates the pairwise merge
- merge_max_cpus: max in cores, emits ResourceSpec::Number
- merge_max_bytes: max in bytes, emits ResourceSpec::String("<bytes>")
matching upstream's `${memory}` numeric-string output
- merge_gpu_requirements: implements the upstream gpu reduce table
Output shape matches upstream: cpus stays a number, memory/storage are
stringified raw bytes. ResourceSpec parsing handles Deacon's existing
unit set (B/KB/MB/GB/TB decimal + KiB/MiB/GiB/TiB binary). The
unit-interpretation difference vs upstream (which treats "gb" as
binary, not decimal) is a pre-existing parse semantic and is not
addressed here — only the merge algorithm.
Tests (11):
- Per-field max for cpus / memory / storage including order
independence and across mixed units (4GB vs 8GiB)
- Single-side passthrough and both-none → None
- GPU: false yields other side, both "optional" stays "optional",
object-form takes max(cores)/max(memory), bool true promotes to
object form when paired with explicit values
- Multi-layer chain (3 configs) collapses to field-wise max regardless
of which layer contributed each field
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deacon's parse_resource_string was interpreting kb/mb/gb/tb as decimal (10^3, 10^6, 10^9, 10^12), differing from upstream devcontainers/cli parseBytes (src/spec-node/imageMetadata.ts) which treats those same suffixes as binary (2^10, 2^20, 2^30, 2^40). A devcontainer.json with `"memory": "4GB"` therefore produced different byte counts depending on which CLI parsed it — same input, different host-requirements evaluation, different mergedConfiguration output. The short suffixes kb/mb/gb/tb now multiply by 1024 instead of 1000, matching upstream. The explicit-binary aliases kib/mib/gib/tib are retained (they always meant binary and continue to mean binary), so this is a behavior change only for callers who wrote kb/mb/gb/tb expecting decimal. Docker and Linux memory-limit syntax both already use binary for these short forms, so most user-authored values were already being interpreted "correctly" by the rest of the stack — only Deacon's parser was the outlier. Updated tests that asserted specific decimal byte counts: - host_requirements::test_resource_spec_parsing - host_requirements::test_storage_evaluation_with_mock_provider - host_requirements::test_storage_evaluation_insufficient_space - integration_host_requirements::test_resource_spec_parsing_edge_cases - config::test_merge_host_requirements_* (added in a29a54b) - config::test_merge_gpu_object_form_takes_max - config::test_merge_gpu_true_with_object_form_promotes_to_object_max Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pofallon
changed the title
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.
Summary
Closes the developer-facing spec parity gaps tracked in #56. Branch carries 19 commits — 11 from earlier work (already on the branch when this batch started) plus 8 new commits this session aligning Deacon's
read-configuration --include-merged-configurationoutput with the upstreamdevcontainers/climergeConfigurationalgorithm (src/spec-node/imageMetadata.ts).Earlier commits (pre-session, on branch already)
Spec-surface fields: gpu availability flag, host-requirements GPU validation,
waitForlifecycle cutoff,portsAttributesprotocol /elevateIfNeeded, featurecustomizationsexposure, feature mount object-form metadata preservation, port event protocol semantics, devcontainer init metadata merge, devcontainer spec-surface field preservation.This session
Entry-point fixes
--docker-paththrough toCliDocker::with_pathinread-configuration(was accepted but silently ignored).mergedConfigurationoutput shape — now matches upstreammergeConfigurationentrypoints+ plural lifecycle arrays (onCreateCommands,updateContentCommands,postCreateCommands,postStartCommands,postAttachCommands) collected per-entry; singular forms stripped from the base. Deacon was previously dropping featureentrypointentirely.capAdd/securityOptswitched from concat to set-union (via existingunion_arrayshelper).mountsswitched from whole-entryPartialEqdedupe to per-target last-wins (matchesmergeMounts); new helpers incrates/core/src/mount.rs(extract_mount_target,union_mounts_by_target).forwardPortsdedupes withlocalhost:N↔Number(N)normalization (matchesmergeForwardPorts); newunion_forward_portsonConfigMerger.customizationsswitched from deep-merge object to per-tool arrays{ vscode: [c1, c2, ...] }(matchesmergeConfiguration's reduce step and the spec note "Merging is left to the tools"). ConfigMerger's underlying deep-merge is preserved for other internal callers; only the read-configuration JSON output is reshaped.init/privilegednow OR-accumulate across features (was last-wins per feature); extractedor_merge_boolhelper.hostRequirementsswitched from whole-struct last-wins to per-field max forcpus/memory/storageandmergeGpuRequirements-style merge forgpu(matchesmergeHostRequirements).Unit-parsing alignment (final commit)
parse_resource_stringnow interpretskb/mb/gb/tbas binary (powers of 1024) to match upstreamparseBytes. Same input → same byte count on Deacon and the reference CLI.kib/mib/gib/tibare retained as explicit-binary aliases.Validation
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningsmake test-nextest-fast→ 2044 tests passed, 30 skipped (up from 1986 at branch start; +58 new tests across the session)Docs touched
docs/subcommand-specs/completed-specs/read-configuration/DATA-STRUCTURES.mdupdated for the newmergedConfigurationshape (plural lifecycle arrays, per-tool customizations).Test plan
cargo run -- read-configuration --workspace-folder . --include-merged-configurationagainst a devcontainer.json with features + lifecycle commands; verifyentrypoints, plural lifecycle arrays, and per-toolcustomizationsarrays appearcargo run -- read-configuration --container-id <id> --include-merged-configurationagainst a container with adevcontainer.metadatalabel that has overlapping mounts/capAdd; verify dedupe behavior🤖 Generated with Claude Code