[bilby91]

Closes #147. Stacked on #146 (lockfile): pulled-in features are locked too. The base is feat/feature-lockfile, so this diff shows only the dependsOn change. Retarget it to main once #146 merges.

Problem

dependsOn only ordered features that devcontainer.json already declared. An undeclared dependency was skipped silently, so a feature that relies on one was installed without it. The spec makes dependsOn a hard dependency, installed recursively with the options it names. The reference CLI does this in computeDependsOnInstallationOrder.

Design decisions

Recorded in design/features.md §5.1 and §8.2.

1. No new structures. layerFeatures' fetch loop appends each fetched feature's dependsOn entries and fetches them in turn. Lockfile pinning, option validation and defaults apply exactly as for declared features. Pulled-in features are locked under their dependsOn key, as the CLI does.
2. Duplicates follow the spec's feature equality: same digest and same options.
   • A repeated key with the same options is skipped before fetching, which also ends cycles.
   • The same content under another reference (:1 vs :1.0) is dropped after fetching.
   • The same feature with different options is installed once per option set. The ordering graph maps an id to all of its entries, and each dependsOn edge points at the entry with the requested options.
3. Options are compared after defaults are applied. This follows the spec's "options executed against the Feature"; the CLI compares options as written. No file format depends on this.
4. Behaviour change: two declared references to the same digest with the same options now install once instead of twice. This matches the spec and the CLI.
5. Base-image matches: a dependency the base image's metadata label already satisfies is skipped without fetching. markAlreadyInstalled's match is factored into bakedFeature for this.
6. Compose inputs hash: it now also covers a local dependency's content, which the declaring feature's content doesn't capture. Projects without a local dependency hash exactly as before.
7. OCI references are lowercased before parsing, as the CLI's getRef does. Published dependsOn keys with uppercase letters (e.g. ghcr.io/codspace/dependson/A) failed to parse before.

Not in this PR

• Uncompressed tarballs: the HTTPS fetcher doesn't accept an uncompressed tar named .tgz, which one of the CLI's fixtures uses. The integration test leaves that feature out.
• overrideFeatureInstallOrder is ignored for fetched features, because layerFeatures re-sorts them with Order(…, nil). A throwaway test confirmed it. This is pre-existing and also affects declared features, so the README's ✅ for it is inaccurate. It's separate work.

Testing

• Unit tests cover:

  • a chain and a diamond;
  • dedupe against a declared feature (4 cases);
  • a cycle through a pulled-in feature;
  • lockfile entries and frozen mode;
  • a dependency already in the base image;
  • a local→local dependency;
  • the compose inputs hash.

  Each fails with the change disabled.

• Integration: TestDependsOn_MatchesCLILockfile builds the CLI's own lockfile-dependson fixture in frozen mode against real ghcr.io, checked against the CLI's expected lockfile.

• go vet, lint and go test -race ./... pass. The integration suite passes, excluding the Docker-in-Docker tests that were broken on main at the time (since fixed in test(integration): fix docker-in-docker tests on ubuntu-26.04 runners #149).

🤖 Generated with Claude Code

[dap-code-review-by-crunchloop]

Code Review — b4a57e2 · ADVISORY

The new recursive dependsOn path can lose declared dependency aliases during content deduplication, and bypasses lockfile artifact validation or transitive expansion when the base image label marks a dependency baked. These leave lockfile pins incomplete and can omit required dependency features on upgrade-era base images.

	Finding	Anchor
🟡 MEDIUM	Dedupe drops the dependsOn alias before it can be locked core/version-compatibility	up.go:1415
🟡 MEDIUM	Baked dependencies can bypass lockfile digest validation core/version-compatibility	up.go:473
🟡 MEDIUM	A baked dependency prevents discovery of its transitive dependencies core/correctness	up.go:474

What I reviewed

Coverage: Reviewed the exact head b4a57e2a28eed794fc64f8bab6e484af8c5859d5 (one commit; 16 changed files relative to lockfile base 45c0015c262fb870cef8107c4a3e7c31d581b7a8). Worked correctness, interface-contract, version-compatibility, and repository mechanisms R5 (reference-CLI divergence) and R8 (tests), plus compose reattach hash behavior. Checked feature resolution, option merging, DAG ordering, build-context use, lockfile pin/commit flow, and baked-label handling. The baked transitive omission is also explicitly described in the new design text as intentional, but that assumption fails for base images made before this behavior shipped. No .dap/code-review.yml or AGENTS.md exists at directives base; applied the repository's .dap/review/engineering.md at that SHA and the core review dimensions. Did not run tests or execute the code. access-control, untrusted-input, and secret-exposure mechanisms were not implicated by the diff; state-integrity and redelivery are not applicable.

---

_{ADVISORY — findings worth reading, none of them blocking. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.}

[dap-code-review-by-crunchloop]

Code Review — 419fdce · ADVISORY

No newly changed behavior was found: the reviewed head has the same tree as the prior head that produced the three already-open findings, so those findings remain open and were not resubmitted.

	Finding	Anchor
🟡 MEDIUM	Dedupe drops the dependsOn alias before it can be locked core/version-compatibility	up.go:1415
🟡 MEDIUM	Baked dependencies can bypass lockfile digest validation core/version-compatibility	up.go:473
🟡 MEDIUM	A baked dependency prevents discovery of its transitive dependencies core/correctness	up.go:474

What I reviewed

Coverage: Verified the exact requested SHA (419fdcefb53254a7054c6162d21004c754671b7c), current PR scope (one commit, 16 files), and the governing repository guidance at base SHA 864709a381a47798a949e16138d066cbd9f4916f; .dap/code-review.yml is absent, and .dap/review/engineering.md supplies the repository-specific mechanisms. The relevant feature-resolution, dependency-graph, lockfile, base-image-label, and compose reattach paths and their tests were read. Correctness and version-compatibility were considered; the prior three findings remain open on unchanged code. No visual evidence applies, and no tests or system code were executed.

---

_{ADVISORY — findings worth reading, none of them blocking. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.}

[dap-code-review-by-crunchloop]

Code Review — 680f353 · ADVISORY

The implementation fixes the three carried findings: duplicate aliases are retained for lockfile commits, baked artifact digests are checked against pins, and dependencies of baked features are traversed. One correctness issue remains: a baked dependency can be treated as satisfying a request even when the request's options differ from those used to build the base image, so those options are silently not applied.

	Finding	Anchor
🟡 MEDIUM	Baked dependsOn features can be reused with the wrong options core/correctness	up.go:475

What I reviewed

Coverage: Reviewed the exact head 680f353bb8f092e44cce80b29ba63e83739a0201 (2 commits, 21 changed files relative to the PR base); its merge base with main is 25711e1bb5b50e4438cf3e05d3ef993f6a7a4e1a. Checked out the exact SHA and verified HEAD. Read the PR rationale, the governing .dap/review/engineering.md at directives base 25711e1bb5b50e4438cf3e05d3ef993f6a7a4e1a, and its README, CONTRIBUTING, and feature design guidance; .dap/code-review.yml is absent. Reviewed feature dependency resolution, graph ordering, baked-image matching and metadata labels, lockfile acceptance/commit behavior, and consumers that skip baked feature installation. Verified and resolved all three carried findings. Relevant mechanisms: correctness, version-compatibility, interface-contract, test-coverage, and operability. No backend-specific path change warranted a path-parity finding; conditional state-integrity and redelivery do not apply. I did not reach unrelated feature/cache and runtime paths.

---

_{ADVISORY — findings worth reading, none of them blocking. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.}

[bilby91]

@dap-code-review-by-crunchloop review

[dap-code-review-by-crunchloop]

On it. A review of the current head is on its way. This branch's automatic review rounds are back, so I'll review new pushes again.

[dap-code-review-by-crunchloop]

Code Review — 9a4e5c2 · BLOCKING

The change introduces a baked-dependency option mismatch and a compose reattach hash gap for local dependencies of remote features; both can produce or retain behavior inconsistent with the newly resolved dependencies. I also found an untrusted-input path where fetched feature metadata can cause local directories outside the project to be read into the build context.

	Finding	Anchor
🔴 CRITICAL	Remote feature metadata can traverse into local files during feature installation core/untrusted-input	up.go:472
🟡 MEDIUM	A baked dependency is reused despite different requested options core/correctness	up.go:475
🟡 MEDIUM	Compose reattach misses local dependencies of remote features core/correctness	up_compose_reattach.go:143
🟡 MEDIUM	Remote dependency metadata can read and package local feature directories outside the project core/untrusted-input	up.go:472

What I reviewed

Coverage: Reviewed the exact head 9a4e5c2b3176d7a990ddaeba61a7f22cb6cc6577: 3 commits and 21 changed files relative to main at ceaba2070fa6783221a83bdebf3a142470b3e066. .dap/code-review.yml is absent at directives base 67a01cce99a1b727d28a081efbe87caf375d9e52; used the repository mechanism catalogue at .dap/review/engineering.md and read the related design and contribution guidance. Worked correctness, untrusted-input, version-compatibility, and test-coverage while tracing dependency expansion, baked metadata and lock checks, duplicate aliases, the feature build context, and compose reattach hashing. Verified the old baked lock-pin mismatch and transitive-dependency discovery findings are fixed in this head; reviewed the option-mismatch rebuttal and found the new dependsOn path exposes it. Did not run the system or tests; examined tests as source. Did not independently review unrelated low-risk README/changelog and mechanical API-renaming hunks in depth. No visual evidence applies.

---

_{BLOCKING — at least one finding is at or above this repository's blocking severity. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.}

[dap-code-review-by-crunchloop]

Code Review — 4811e0a · CLEAN

No new defect is established in the final commit; the prior path-confinement, baked-dependency-option and compose-hash claims no longer hold. I dismissed or resolved the carried findings after checking the relevant code at 4811e0a; the two open arguments without an explicit reply were adjudicated against the code as well.

What I reviewed

Coverage: Reviewed the 5-commit, 21-file change against exact head 4811e0aecb4e81f0166eb4316efd1c84480500c1 (same SHA as HEAD), focusing on correctness, untrusted-input, version-compatibility, compose reattach hashing and dependency graph/lock behavior. Confirmed local dependency confinement and transitive local-content hashing, baked dependencies now fetched rather than accepted from base-image labels, and dedupe aliases retained for lockfile commit. Read applicable repository guidance at directives base 67a01cce99a1b727d28a081efbe87caf375d9e52; .dap/code-review.yml is absent, and the applicable standard is .dap/review/engineering.md plus CONTRIBUTING.md and design/features.md. The eight supplied GitHub comment IDs returned 404 individually; the available pull-request review comments and their replies were read via the PR comments endpoint and used to check the carried claims. No system code or tests were executed. Other behavior outside these paths was not exhaustively reviewed.

---

_{CLEAN — this round found nothing. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.}

[dap-code-review-by-crunchloop]

No open findings on 4811e0a.

_{The DAP Code Review check run remains the verdict of record. This approval is dismissed if a finding reopens or a later head is not clean.}