[bilby91]

Fixes the two findings the DAP review raised on #146 (dap-finding 3890 HIGH, 3887 MEDIUM). #146 was merged while that review was still BLOCKING, so both issues are on main. The lockfile is unreleased (still under [Unreleased]).

1. Frozen builds trusted pre-baked features (HIGH)

Problem: a feature the base image's devcontainer.metadata label satisfied was skipped, and its existing lock entry was copied over. Frozen mode therefore accepted a base image built from a different artifact, or one the lockfile had no entry for.

The data was already there: this library has written every feature's resolvedRef into the label since v0.1.0. ParseMetadataLabel dropped it, and so did carrying base entries into a rebuilt label, despite that path's "lossless" comment.

Fix: reuse the existing label match and add the lockfile as a second condition.

• Keep the artifact: config.FeatureMetadata.ResolvedRef now survives parsing and rebuilds.
• Read the lockfile first: layerFeatures reads it before matching the label. markAlreadyInstalled accepts a baked feature only if featureLock.acceptsBaked agrees:
  • its artifact is the pinned one; or
  • with no pin, its artifact is recorded, and it then gets locked.
• Everything else is fetched: a different or unrecorded artifact is fetched at its pin and installed over the base image.
• Commit: an accepted baked feature keeps its pin rather than being re-locked from the label, because the label doesn't record dependsOn.
• LockfileDisabled: keeps the old label-only match.

Effect on the pre-baked fast path: none for images this library built, since they record resolvedRef. Labels from other tools record no artifact. The reference CLI's labels never matched here anyway, because their id is the full reference, not the bare id this path compares.

2. Removing the last feature skipped the lockfile (MEDIUM)

Problem: layerFeatures returned before touching the lockfile when no features were declared.

Fix: with no features, commitNoFeatures still reconciles an existing lockfile:

• default and upgrade modes empty it;
• frozen mode reports the stale entries;
• with no lockfile, nothing is created, as with the reference CLI.

Testing

• TestLockfile_BakedFeature covers six cases: pinned artifact, other artifact, unrecorded artifact, no pin (locks the baked artifact), no pin in frozen mode, and disabled mode.
• TestLockfile_NoFeatures covers stale entries in frozen and default modes, and that no lockfile gets created.
• TestParseMetadataLabel_ResolvedRefSurvivesRebuild checks the digest survives parsing and a rebuild on top.
• Regression check: with acceptsBaked and commitNoFeatures reverted to the old behaviour, these tests fail.
• Fixture update: TestUp_PrebakedImageSkipsFeatureFetch's label fixture now carries a resolvedRef, like real labels do.
• Suites: go vet, lint and go test -race ./... pass. The pre-baked, metadata, build and lockfile integration tests pass against a local daemon.

🤖 Generated with Claude Code

[dap-code-review-by-crunchloop]

Code Review — 131bbeb · ADVISORY

Upgrade-mode correctness: the lockfile fix still lets an already-baked artifact short-circuit resolution in LockfileUpgrade, then records that artifact as the new pin, so upgrade can downgrade the lock instead of refreshing it.

	Finding	Anchor
🟡 MEDIUM	Upgrade mode can replace a newer pin with an older baked artifact core/correctness	lockfile.go:83

What I reviewed

Coverage: Reviewed the exact requested commit 131bbebdabba1e47af1142b3a1f8492f28f2b89b (one commit, nine changed files). Worked correctness and interface-contract behavior for baked-feature matching, lockfile reconciliation, and no-feature configurations; checked their consumers in up.go and build.go, the lockfile integrity implementation, and relevant unit tests. Checked design and changelog claims and repository guidance at base 25711e1bb5b50e4438cf3e05d3ef993f6a7a4e1a; .dap/code-review.yml and AGENTS.md are absent, so the repository mechanism guidance in .dap/review/engineering.md was used alongside the core prompt. The change is backend-neutral and introduces no applicable state transaction, retry path, or user-facing feature needing evidence capture. Runtime/tests were not executed, per review constraints.

---

_{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 — 67a01cc · ADVISORY

One MEDIUM interface-contract defect remains: accepting an unpinned baked feature can write a lock entry without its declared dependsOn metadata. The earlier upgrade-mode finding is resolved on this head because upgrade mode rejects baked matches and fetches the requested feature afresh.

	Finding	Anchor
🟡 MEDIUM	Unpinned baked features skip the lockfile's dependency metadata core/correctness	lockfile.go:88

What I reviewed

Coverage: Reviewed exact head 67a01cce99a1b727d28a081efbe87caf375d9e52 (2 commits, 9 changed files). Checked correctness and interface-contract behavior in baked-feature matching, lockfile generation, and no-feature reconciliation; compared the lockfile shape with the reference CLI and examined the updated tests. Checked optional metadata-label compatibility: older labels without resolvedRef fall back to fetch, and older readers can ignore the added field. No .dap/code-review.yml exists at the governing base SHA; applied the repository guidance in .dap/review/engineering.md alongside the core review dimensions. Did not execute tests or the system under review. Redelivery, concurrency, and resource-lifecycle dimensions do not apply to this change.

---

_{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]

🟡 MEDIUM · core/correctness

Unpinned baked features skip the lockfile's dependency metadata

The serialized lockfile is consumed by the reference-compatible feature lockfile path: at devcontainers/cli v0.87.0 src/spec-configuration/lockfile.ts:26-35 writes each feature's declared dependsOn, and containerFeaturesOrder.ts:414-435 resolves those dependencies while preparing a future feature install. In this repository feature.Lock at feature/lockfile.go:155-164 serializes only f.Metadata.DependsOn; the accepted baked metadata has no such field, and Lockfile.Equal/Changed compare it at feature/lockfile.go:113-124.

Fix — When an unpinned baked feature is accepted, retain its declared dependency list from its feature manifest (for example by resolving the manifest or preserving the relevant dependency data in the image label) before generating the lock entry; alternatively fetch the feature artifact to build a complete lock entry.

Witness

At lockfile.go:87-88 acceptsBaked returns true for a baked artifact when no lock pin exists. markAlreadyInstalled assigns the parsed label entry to f.Metadata; metadataEntry does not include dependsOn, while fetched feature metadata does (feature/metadata.go:23-38, feature/parse.go:63-65,92-98). commit then passes this metadata to feature.Lock, whose DependsOn is constructed from f.Metadata.DependsOn (feature/lockfile.go:155-164), and writes the resulting entry. A real feature declaring dependsOn thus gets an empty dependsOn in newly generated lockfile after its first baked-image acceptance without a prior pin. CLI v0.87.0 lockfile generation writes keys from the manifest (/tmp/devcontainers-cli/src/spec-configuration/lockfile.ts:25-35), and its ordering resolver reads those declarations (containerFeaturesOrder.ts:414-435).

[bilby91]

Valid for this commit taken alone. It's resolved by #151, which is stacked on this PR and merges right after it: there, layerFeatures fills in a baked feature's dependsOn before lock.commit runs. It comes from the label, which now records dependsOn, or for a label written before that, from the feature's metadata fetched at its recorded artifact. So feature.Lock builds the entry from the full list. The lockfile is unreleased, so main won't ship with the gap. I'm adding a test on #151 that asserts an unpinned baked feature's lock entry carries its dependsOn.

[dap-code-review-by-crunchloop]

I can't settle this from the review record alone — Settling this requires reading the stacked branch #151 — layerFeatures, the label schema, and the new test — which is not in this record; the finding stands on the sequence this diff shows.

To have me read the code and take another look, comment @dap-code-review-by-crunchloop review. I also understand @dap-code-review-by-crunchloop pause and @dap-code-review-by-crunchloop resume.