diff --git a/FORK-HANDOFF-COMPLETE.md b/FORK-HANDOFF-COMPLETE.md new file mode 100644 index 000000000..320f42945 --- /dev/null +++ b/FORK-HANDOFF-COMPLETE.md @@ -0,0 +1,151 @@ +# Devcontainer Metadata Regression: Complete Fork Handoff + +Date: 2026-05-13 +Prepared by: Copilot investigation agent + +## 1) Which repo to fork +Fork this repository: +- https://github.com/devcontainers/cli + +Why this repo: +- The observed behavior is consistent with the CLI build/metadata assembly path, not the spec intent. +- The likely fix point is in CLI source where metadata is collected and re-written. + +Suggested fork workflow: +1. Fork `devcontainers/cli`. +2. Create branch `fix/preserve-user-dockerfile-metadata`. +3. Add/adjust tests first, then patch implementation. +4. Open PR to upstream `devcontainers/cli` with repro matrix and source trace below. + +## 2) Executive conclusion +Your expectation is standard: image metadata in `devcontainer.metadata` is intended to support this exact use case. + +Observed behavior in tested CLI build paths can drop user Dockerfile metadata entries from the effective final image label. + +Working decision for downstream reliability: +- Treat runtime-critical settings as devcontainer JSON source of truth (workaround branch B1 in downstream repo). +- In parallel, fix/report upstream CLI behavior. + +## 3) Spec intent (cross-check) +Reviewed references: +- devcontainers/spec issue #18 (Dev container metadata in image labels) +- devcontainers/spec PR #95 (Image Metadata Proposal, merged) +- containers.dev spec sections for Image Metadata and Merge Logic + +Intent established by spec/discussion: +1. `devcontainer.metadata` label is an intended standard mechanism. +2. Label may contain object or array snippets, merged at runtime. +3. Mounts merge rule is collected list, with conflict handling by source. +4. Features and image metadata are meant to compose, not erase user intent. + +## 4) Reproduction matrix (executed) +Environment versions: +- devcontainer CLI: 0.87.0 +- Docker: 29.4.3 +- buildx: 0.33.0 +- Node for CLI invocation: 22.11.0 via mise + +### Case A +Build path: docker build only (no devcontainer CLI) +Base: metadata-bearing (`mcr.microsoft.com/devcontainers/base:debian`) +Result: user metadata entry survived. + +### Case B +Build path: devcontainer CLI build, no features +Base: metadata-bearing (`mcr.microsoft.com/devcontainers/base:debian`) +Result: user metadata entry did not survive. + +### Case C +Build path: devcontainer CLI build, with features +Base: metadata-bearing (`mcr.microsoft.com/devcontainers/base:debian`) +Result: user metadata entry did not survive. + +### Case D +Build path: devcontainer CLI build, no features +Base: plain (`debian:bookworm`, no preexisting devcontainer metadata) +Result: user metadata entry survived. + +### Case E +Build path: devcontainer CLI build, with features +Base: plain (`debian:bookworm`) +Result: user metadata entry did not survive. + +Interpretation: +- Feature-enabled CLI path consistently overwrote effective user label metadata in tests. +- CLI no-feature behavior appears sensitive to base metadata state. +- This indicates implementation/path behavior, not a spec prohibition. + +## 5) Minimal repro patterns used +### Repro metadata entry in Dockerfile +- `name: "label-repro"` +- `mounts: ["source=${localEnv:HOME}/.aws,target=/home/vscode/.aws,type=bind"]` +- `postCreateCommand: "echo HELLO_FROM_LABEL"` + +### Devcontainer CLI command pattern +- `mise x nodejs@22.11.0 -- npx -y @devcontainers/cli@latest build --workspace-folder --image-name --no-cache` + +### Inspect commands +- `docker inspect --format '{{ index .Config.Labels "devcontainer.metadata" }}' | jq` +- `docker history --no-trunc | grep -i 'devcontainer.metadata'` + +Note: +- Docker-in-Docker feature on Debian trixie required `"moby": false` for build success in repro. + +## 6) Source-code trace in devcontainers/cli (where it likely breaks) +High-probability break path: +1. Dockerfile config flow calls `buildNamedImageAndExtend(...)` then `buildAndExtendImage(...)`. +2. `buildAndExtendImage(...)` calls `getImageBuildInfoFromDockerfile(...)` before generating wrapper Dockerfile. +3. In `internalGetImageBuildInfoFromDockerfile(...)`, metadata source is derived via `findBaseImage(...)` + `inspectDockerImage(baseImage)` and parsed from that image. +4. Wrapper metadata is then generated from computed metadata/features/config with `getDevcontainerMetadata(...)` and written via `getDevcontainerMetadataLabel(...)` into generated wrapper Dockerfile (`Dockerfile-with-features` / `Dockerfile.extended`). +5. Final image has a later `LABEL devcontainer.metadata=...` write from wrapper path, which becomes effective (label replacement semantics). + +Observed consequence: +- User Dockerfile metadata entry can be absent from final effective label, even when present in image history. + +## 7) What to implement upstream +Primary objective: +- Preserve user Dockerfile metadata entries in effective final `devcontainer.metadata` for CLI build paths, especially with features. + +Implementation direction: +1. Ensure metadata computation for wrapper label includes metadata from the built user Dockerfile image layer (not only resolved base image metadata). +2. Validate behavior across: + - Dockerfile config with features + - Dockerfile config without features + - image-based config extended with features +3. Keep merge rules consistent with spec intent. + +## 8) Tests to add/update in devcontainers/cli +Add regression tests that assert final image effective metadata includes user Dockerfile metadata entry: +1. Dockerfile + feature + metadata-bearing base. +2. Dockerfile + feature + plain base. +3. Dockerfile without feature + metadata-bearing base. + +Assertions: +- Final inspect metadata contains user entry by marker (`name` or unique command). +- Feature metadata still present. +- Mount and lifecycle fields are preserved/merged as expected. + +## 9) Downstream workaround (until upstream fixed) +In downstream repos relying on runtime-critical mounts/hooks: +1. Move runtime-critical fields from Dockerfile label to devcontainer JSON. +2. Keep Dockerfile metadata minimal/non-critical where possible. +3. Add drift tests if maintaining parallel local/release configs. + +## 10) Ready-to-file upstream issue/PR summary +Problem statement: +- `devcontainer build` can produce final `devcontainer.metadata` that excludes user Dockerfile metadata entries in certain build paths, especially when features are involved. + +Expected: +- Final metadata should preserve user Dockerfile metadata and append/merge feature/runtime metadata according to spec merge model. + +Actual: +- Final effective label can reflect wrapper/base/feature entries without user Dockerfile entry. + +Impact: +- Mounts, lifecycle hooks, and other runtime-critical settings encoded in user image metadata may silently disappear. + +Evidence: +- Repro matrix above + inspect/history outputs + version snapshot. + +--- +This document is the single source handoff for the next agent run in a forked `devcontainers/cli` context. diff --git a/src/spec-node/imageMetadata.ts b/src/spec-node/imageMetadata.ts index 60884592e..07415d53c 100644 --- a/src/spec-node/imageMetadata.ts +++ b/src/spec-node/imageMetadata.ts @@ -427,7 +427,19 @@ export async function internalGetImageBuildInfoFromDockerfile(inspectDockerImage const imageDetails = baseImage && await inspectDockerImage(baseImage) || undefined; const dockerfileUser = findUserStatement(dockerfile, dockerBuildArgs, envListToObj(imageDetails?.Config.Env), globalBuildxPlatformArgs, targetStage); const user = dockerfileUser || imageDetails?.Config.User || 'root'; - const metadata = imageDetails ? getImageMetadata(imageDetails, substitute, output) : { config: [], raw: [], substitute }; + const baseMetadata = imageDetails ? getImageMetadata(imageDetails, substitute, output) : { config: [], raw: [], substitute }; + const dockerfileMetadata = getDockerfileMetadata(dockerfileText, output); + const metadata = dockerfileMetadata.length ? { + config: [ + ...baseMetadata.config, + ...dockerfileMetadata.map(substitute), + ], + raw: [ + ...baseMetadata.raw, + ...dockerfileMetadata, + ], + substitute, + } : baseMetadata; return { user, metadata, @@ -473,25 +485,65 @@ function internalGetImageMetadata(imageDetails: ImageDetails | ContainerDetails, }; } -export function internalGetImageMetadata0(imageDetails: ImageDetails | ContainerDetails, output: Log) { - const str = (imageDetails.Config.Labels || {})[imageMetadataLabel]; - if (str) { +function getDockerfileMetadata(dockerfileText: string, output: Log): ImageMetadataEntry[] { + const metadataEntries: ImageMetadataEntry[] = []; + const dockerfileWithoutLineContinuations = dockerfileText.replace(/\\\r?\n/g, ' '); + const labelInstruction = /^\s*LABEL\s+(.+)$/gmi; + let labelMatch: RegExpExecArray | null; + while ((labelMatch = labelInstruction.exec(dockerfileWithoutLineContinuations))) { + const body = labelMatch[1]; + const devcontainerMetadataValue = extractDevcontainerMetadataValueFromLabel(body); + if (!devcontainerMetadataValue) { + continue; + } + metadataEntries.push(...parseImageMetadataLabel(devcontainerMetadataValue, output)); + } + return metadataEntries; +} + +function extractDevcontainerMetadataValueFromLabel(labelInstructionBody: string): string | undefined { + const keyValueRegex = /(?:^|\s)(?:"devcontainer\.metadata"|devcontainer\.metadata)\s*=\s*("(?:\\.|[^"])*"|'(?:\\.|[^'])*')/g; + const match = keyValueRegex.exec(labelInstructionBody); + if (!match) { + return undefined; + } + const token = match[1]; + if (token.startsWith('"')) { try { - const obj = JSON.parse(str); - if (Array.isArray(obj)) { - return obj as ImageMetadataEntry[]; - } - if (obj && typeof obj === 'object') { - return [obj as ImageMetadataEntry]; - } - output.write(`Invalid image metadata: ${str}`); - } catch (err) { - output.write(`Error parsing image metadata: ${err?.message || err}`); + return JSON.parse(token) as string; + } catch { + return undefined; } } + return token.slice(1, -1) + .replace(/\\'/g, "'") + .replace(/\\\\/g, '\\'); +} + +function parseImageMetadataLabel(str: string, output: Log): ImageMetadataEntry[] { + try { + const obj = JSON.parse(str); + if (Array.isArray(obj)) { + return obj as ImageMetadataEntry[]; + } + if (obj && typeof obj === 'object') { + return [obj as ImageMetadataEntry]; + } + output.write(`Invalid image metadata: ${str}`); + } catch (err) { + output.write(`Error parsing image metadata: ${err?.message || err}`); + } return []; } +export function internalGetImageMetadata0(imageDetails: ImageDetails | ContainerDetails, output: Log) { + const str = (imageDetails.Config.Labels || {})[imageMetadataLabel]; + if (!str) { + return []; + } + return parseImageMetadataLabel(str, output); +} + export function getDevcontainerMetadataLabel(devContainerMetadata: SubstitutedConfig) { const metadata = devContainerMetadata.raw; if (!metadata.length) { diff --git a/src/test/dockerfileUtils.test.ts b/src/test/dockerfileUtils.test.ts index 1811f1ce2..e26d45c0a 100644 --- a/src/test/dockerfileUtils.test.ts +++ b/src/test/dockerfileUtils.test.ts @@ -242,6 +242,52 @@ FROM base-\${TARGETARCH} assert.strictEqual(info.metadata.config.length, 0); assert.strictEqual(info.metadata.raw.length, 0); }); + + it('preserves metadata declared in Dockerfile labels', async () => { + const dockerfile = `FROM ubuntu:latest as base +LABEL devcontainer.metadata="{\\"id\\":\\"dockerfile-id\\",\\"postCreateCommand\\":\\"echo test\\"}" +`; + const details: ImageDetails = { + Id: '123', + Config: { + User: 'imageUser', + Env: null, + Labels: { + [imageMetadataLabel]: '[{"id":"base-id"}]' + }, + Entrypoint: null, + Cmd: null + }, + Os: 'linux', + Architecture: 'amd64' + }; + const info = await internalGetImageBuildInfoFromDockerfile(async () => details, dockerfile, {}, undefined, testSubstitute, nullLog, false, { os: 'linux', arch: 'arm64' }, { os: 'linux', arch: 'amd64' }); + assert.strictEqual(info.metadata.raw.length, 2); + assert.strictEqual(info.metadata.raw[0].id, 'base-id'); + assert.strictEqual(info.metadata.raw[1].id, 'dockerfile-id'); + assert.strictEqual(info.metadata.config[1].id, 'dockerfile-id-substituted'); + }); + + it('parses metadata declared with a quoted label key', async () => { + const dockerfile = `FROM ubuntu:latest as base +LABEL "devcontainer.metadata"='{"id":"dockerfile-id-quoted"}' +`; + const details: ImageDetails = { + Id: '123', + Config: { + User: 'imageUser', + Env: null, + Labels: null, + Entrypoint: null, + Cmd: null + }, + Os: 'linux', + Architecture: 'amd64' + }; + const info = await internalGetImageBuildInfoFromDockerfile(async () => details, dockerfile, {}, undefined, testSubstitute, nullLog, false, { os: 'linux', arch: 'arm64' }, { os: 'linux', arch: 'amd64' }); + assert.strictEqual(info.metadata.raw.length, 1); + assert.strictEqual(info.metadata.raw[0].id, 'dockerfile-id-quoted'); + }); }); describe('findBaseImage', () => {