Implement feature installation order and enhance test suite

[prulloac]

This pull request adds support for the installsAfter (soft dependency) and dependsOn (hard dependency) fields to the devcontainer features specification, and introduces comprehensive internal tests to validate feature installation order logic, including handling of dependency resolution, override semantics, and cycle detection. Documentation has also been updated to reflect the new support status for these fields.

Feature dependency enhancements:

• Added installsAfter (soft ordering hint) and dependsOn (hard dependency) fields to the Spec struct in features.go, enabling features to declare installation order preferences and requirements.

Testing and validation:

• Introduced a new internal test suite in install_order_internal_test.go to thoroughly test the dependency and installation order resolution logic, covering cases such as ambiguous references, override precedence, cycle detection, and complex dependency trees.

Documentation updates:

• Updated docs/devcontainer-spec-support.md to indicate support for the overrideFeatureInstallOrder, dependsOn, and installsAfter fields, replacing the previous "unsupported" status. [1] [2]

[Copilot]

Pull request overview

Adds support for devcontainer Feature installation ordering semantics, including a user-specified override order and dependency metadata read from devcontainer-feature.json, and extends the test suite and docs accordingly.

Changes:

• Add overrideFeatureInstallOrder to devcontainer.Spec and implement order resolution (resolveInstallOrder) plus dependency validation (validateDependencies).
• Extend devcontainer/features.Spec to parse installsAfter (soft ordering) and dependsOn (hard dependency).
• Add tests covering override ordering, installsAfter behavior, hard-dependency presence validation, and cycle detection; update spec support documentation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
docs/devcontainer-spec-support.md	Updates documented support matrix for devcontainer properties/features.
devcontainer/features/features.go	Adds JSON fields for installsAfter and dependsOn to feature metadata parsing.
devcontainer/devcontainer.go	Implements feature extraction pre-pass, install-order resolution, and hard dependency validation.
devcontainer/devcontainer_test.go	Adds tests for override ordering, installsAfter ordering, dependsOn validation, and cycle detection.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[michaelvp411]

@johnstcn

[johnstcn]

🤖 Deep Review — multi-reviewer analysis with RED-test validation

Clean refactoring of the monolithic compileFeatures into well-separated phases. The round-based topo sort faithfully implements the devcontainer spec, all new types are unexported (no API lifecycle risk), and the test suite covers an impressive range of edge cases. The spec-compliance work on installsAfter/dependsOn/overrideFeatureInstallOrder is solid.

6 findings (all validated with RED tests), 3 nits. Pre-existing MkdirAll 0o644 issue tracked separately in #506.

---

P1 — Non-deterministic map iteration affects dedup winner

devcontainer.go normalizeFeatureOptions (~L396)

The old code iterated s.Features then called sort.Strings(featureOrder) before processing. The new normalizeFeatureOptions iterates the same map without sorting. The final install order is deterministic (topo-sort handles it), but the worklist traversal order is not — and this matters during the new dedup path. When two raw refs resolve to the same feature ID+version (e.g. foo:1.0.0 and foo:latest), whichever is processed first "wins" and determines the feature directory hash, changing WORKDIR, --mount, and FROM scratch AS names in the Dockerfile.

Validated: RED test calls normalizeFeatureOptions 100 times with the same 12-entry map. Fails on iteration 1 — different permutation every time.

Fix: Sort the worklist returned by normalizeFeatureOptions by .ref before processing in collectFeaturePlan Phase 1. Also sort plan.extracted keys before Phase 2 iteration.

(Go Architect P1, Edge Case Analyst P1, Performance Analyst Obs — 3 independent reviewers)

---

P2 — extractAndRegister performs full OCI download before ID-based dedup

devcontainer.go extractAndRegister (~L298 vs ~L309)

features.Extract() (network I/O + disk write) runs at L298. The ID-based dedup check (p.idToRef[spec.ID]) runs at L309. When dedup fires, the downloaded directory is orphaned and never cleaned up. Each wasted pull is seconds of wall time.

Validated: Code trace confirms execution order: MkdirAll → Extract → ID check → silent return. Test with in-memory registry confirms 2 directories on disk but only 1 tracked in the plan.

Fix: Check p.idToRef before extracting (requires pre-fetching the manifest to learn the ID), or clean up the orphaned directory on the dedup path.

(Performance Analyst P2)

---

P2 — validateBuildContexts returns on first map entry; non-deterministic error

devcontainer.go validateBuildContexts (~L443-446)

for canonical, refs := range p.ambiguousCanonicals {
    return fmt.Errorf("multiple configured features share canonical reference %q ...")
}

Iterates a map, returns immediately on the first entry. Which ambiguity is reported depends on Go's map seed.

Validated: RED test with 5 ambiguous canonicals over 100 iterations observes all 5 distinct error messages.

Fix: Collect all ambiguous entries, sort them, report all in one error (or at least pick the lexicographically first).

(Edge Case Analyst P2, Go Architect P2, Product Reviewer P2, Style Reviewer Nit — 4 reviewers)

---

P2 — DependsOnSatisfied and DependsOnCanonicalRefResolved are fake tests

devcontainer_test.go:452 and devcontainer_test.go:567

Both discard params and assert only require.NoError(t, err). The first passes by alphabetical coincidence ("a" sorts before "b"). The second passes because unresolvable deps are silently skipped (if !ok || !all[predRef] { continue }).

Validated (DependsOnSatisfied): RED test calls resolveInstallOrder with empty DependsOn maps. Returns nil error and alphabetical order — proving NoError is vacuous.

Validated (DependsOnCanonicalRefResolved): RED test passes empty canonicalToRef map. resolveDependencyRef returns ("", false, nil), edge is silently dropped, no error returned. Alphabetical fallback puts by-ref before late — ordering constraint completely ignored.

Fix: Assert ordering in params.DockerfileContent, matching the pattern used in the stronger sibling tests like DependsOnEnforcesInstallOrder.

(Test Auditor P1 — downgraded to P2: false coverage, not a runtime bug)

---

P2 — Feature equality uses ID+Version, silently dropping options on dedup

devcontainer.go extractAndRegister (~L308-316)

The spec requires manifest digest + options equality. The implementation compares only ID+Version. If a user lists the same feature from two refs with different options (e.g. {"lang": "go"} vs {"lang": "rust"}), the second is silently dropped — (false, nil) with no warning.

Validated: RED test registers same feature from two refs with different options. Second returns (false, nil). Only first options survive; second lost with no diagnostic.

Fix: At minimum, warn when dedup fires and options differ. Ideally, compare options as the spec requires.

(Product Reviewer P2)

---

P3 — Self-referencing dependsOn produces misleading false cycle error

devcontainer.go newDepGraph (~L589-594)

If a feature spec contains dependsOn: {"own-id": {}}, addEdge(r, r) increments inDegree[r] by 1. The node can never reach in-degree 0, so topoSortRounds reports "cycle detected." A self-reference is arguably a spec error, but the error message is misleading — there's no multi-node cycle.

Validated: RED test with single feature DependsOn: {"a": {}} (self-ref). resolveInstallOrder returns "cycle detected" error.

Fix: Either detect and skip self-edges in addEdge, or produce a specific error message like "feature %q depends on itself".

(Edge Case Analyst P1, Test Auditor P3)

---

Nits

• devcontainer.go — resolveInstallOrder godoc block runs into depRefResolver type definition with no blank-line separator. godoc will misattribute the documentation.
• devcontainer.go — extractedFeature fields use inconsistent prefix convention: featureRef, featureName, featureDir vs spec, opts, fromDep. The feature prefix is redundant given the type name.
• docs/devcontainer-spec-support.md — Status emoji for overrideFeatureInstallOrder, dependsOn, installsAfter renders as � (U+FFFD). Entire file also has LF→CRLF line ending change inflating the diff.

---

🤖 Generated by Coder deep-review. Will be reviewed by a human.

[johnstcn]

Thanks for the contribution! I think the P1 non-determinism is the main blocker here; the other issues could reasonably be follow-ups.

[mafredri]

Netero first pass (round 1). One concern, two nits, three notes.

The dependency resolution algorithm is well-structured: round-based topological sort with override priority, canonical ref matching, feature equality deduplication, and cycle detection. Test coverage is solid at ~1.8:1 test-to-production ratio across both internal and external tests.

Severity breakdown: 1x P2 (line endings), 2x P3 (dead code, nondeterminism), 3x Notes.

"File converted from LF to CRLF line endings... Every line appears changed in the diff due to the line-ending flip, obscuring the actual content changes" -- Netero, squinting at a 200-line diff that changed 3 icons

🤖 This review was automatically generated with Coder Agents.

[mafredri]

P2 The entire file has been converted from LF to CRLF line endings. No other .md in docs/ uses CRLF. This makes the diff appear as though every line changed, obscuring the actual content changes (three status icon updates). (Netero)

file docs/devcontainer-spec-support.md confirms "with CRLF line terminators". Fix with dos2unix or equivalent before merging.

Additionally, the status icons on line 36 (overrideFeatureInstallOrder), line 100 (version), line 119 (dependsOn), and line 120 (installsAfter) all render as the Unicode replacement character (U+FFFD / "\ufffd") instead of the intended emoji. The prior copilot review flagged this as well. These need to be corrected to the appropriate status emoji.

🤖

[mafredri]

P3 fromDep is written but never read. The field is set during extractAndRegister and propagated through featureWorkItem, but no code path ever reads ef.fromDep after construction. (Netero)

Either remove the field and its plumbing, or use it (e.g., to skip user-option merging on auto-added deps, or to annotate logging).

🤖

[mafredri]

P3 validateBuildContexts iterates p.ambiguousCanonicals and returns on the first entry. If multiple canonicals are ambiguous, which one gets reported depends on Go map iteration order, producing nondeterministic error messages across runs. (Netero)

Not a correctness issue (any single error suffices to reject), but it makes debugging harder when users see different messages on each attempt. Could sort the keys and report the first alphabetically, or collect all ambiguous entries into one message.

🤖

[mafredri]

Note ef := extracted[featureRefRaw] returns nil if the key is missing. The next line ef.spec.Compile(...) would panic. In practice this is safe today because resolveInstallOrder operates on the same key set, but there's no defensive check at the boundary between two independent functions. A one-line nil guard would prevent a confusing nil-pointer panic if a future refactor breaks the invariant. (Netero)

🤖

[mafredri]

Note The prior code had a TODO here: "We should warn that because we were unable to find the remote user, we're going to run as root." This PR deleted it without implementing the warning. The behavior is unchanged (still silently falls back to root), but the signal for the next reader encountering the silent root fallback is now gone. Consider either restoring the TODO or implementing the warning. (Netero)

🤖