diff --git a/.devcontainer/.dockerignore b/.devcontainer/.dockerignore new file mode 100644 index 00000000..adf7cc00 --- /dev/null +++ b/.devcontainer/.dockerignore @@ -0,0 +1,53 @@ +# What the prebuild tag is allowed to notice. +# +# devpod hashes the build context as well as the build config, and with a +# Dockerfile that COPYs nothing it hashes the *whole* directory: every file +# under the context, keyed by path and CRC32, with only its own feature-staging +# folder excluded (pkg/util/hash.DirectoryHash, reached from +# pkg/devcontainer/config.CalculatePrebuildHash). There was no .dockerignore in +# here, so the context half of the hash was "every byte in .devcontainer/", +# which is a wider claim than the tag is meant to make: the tag is supposed to +# move when the image would differ, and three kinds of file in here cannot +# change the image at all. +# +# Measured on devpod 0.26.1, amd64, against this repository's `.devcontainer/` +# at 4db3427: appending one comment line to claude-code/README.md and one to +# claude-code/init-host.sh moved the tag from +# devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7 -- which CI had published, so it +# pulled -- to devpod-90e9641b9d3661f681e640eeedb3a410, which nothing has ever +# published. That is a multi-minute local build, per launch, on every branch +# carrying the edit, bought with a prose change. +# +# Nothing here is excluded from the *build* in any meaningful sense, which is +# what makes this safe rather than a trade: the Dockerfile has no COPY and no +# ADD, so the context tar is not read at all. The one guard against that +# changing is a test -- test_the_dockerfile_copies_nothing_out_of_the_context. + +# Prose. The feature's README and troubleshooting guide are here because the +# devcontainer Features spec wants them beside the feature they document, not +# because anything reads them at build time. +**/*.md + +# The host-side hook. `initializeCommand` runs on the developer's machine +# before the container exists, and devpod already agrees it is not a build +# input: normalizeConfigForHash clears DevContainerActions, which is where the +# command lives, out of the config half of the hash. It then hashed the script +# itself straight back in as context. Excluding it here is what makes the two +# halves say the same thing. +claude-code/init-host.sh + +# The manifest itself, for the same reason and with the most to gain from it. +# devpod does not read devcontainer.json out of the context -- it read the file +# to get here -- and the config half of the hash already carries every build +# input it declares: `name`, `features`, `overrideFeatureInstallOrder`, `image`, +# `dockerfile`, `context` and the whole of `build`. Hashing the raw bytes on top +# of that adds nothing except the comments, and this file is mostly comments: +# `abf6d9b` moved the tag with ~30 lines of them beside a one-line mount change. +# +# It also stops the raw bytes overruling devpod on the fields it deliberately +# drops -- `mounts`, `containerEnv`, `postCreateCommand`, `customizations` -- +# all of which devpod applies when it creates the container, so a prebuilt image +# plus a changed one of them is correct and was being rebuilt for nothing. +# Verified both directions: a comment-only edit leaves the tag alone, while +# changing `build.cacheFrom` or adding a feature still moves it. +devcontainer.json diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 4db3dcef..f4cec7cf 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -9,16 +9,22 @@ // is a hash *of the build context* as well as of the config // (pkg/devcontainer/config/prebuild.go). With the root as context that // hash moved on every commit to any file in the repository, so a - // prebuilt image could never match one. Scoped here it moves when - // `.devcontainer/**` moves, which is exactly when the image has to be - // rebuilt -- this Dockerfile and the local `claude-code` feature's - // scripts included, since they sit inside the context and are hashed - // with it. + // prebuilt image could never match one. // - // The root `.dockerignore` is no longer read for this build (docker - // looks for one inside the context); its patterns existed to keep - // `.pixi/` and `rust/target/` out of a root-sized context, and nothing - // of that shape is in here. + // Scoping it here narrowed that to "every commit under + // `.devcontainer/**`", which is close to right but not the same claim as + // "every commit that changes the image", and the gap cost real launches. + // A Dockerfile with no COPY leaves devpod's includeFiles empty, and it + // then hashes the whole directory -- so this file's own comments, the + // feature's README, and the host-side `initializeCommand` were all + // hashed into the tag, none of which the image can see. + // `.dockerignore`, below, is what closes it; read that file for the + // measurement and the reasoning. + // + // The root `.dockerignore` is not read for this build -- docker looks + // for one inside the context, and there is one there now. The root + // file's patterns existed to keep `.pixi/` and `rust/target/` out of a + // root-sized context, and nothing of that shape is in here. "context": ".", // Best-effort layer cache for builders that know nothing about devpod diff --git a/.github/workflows/devcontainer-prebuild.yml b/.github/workflows/devcontainer-prebuild.yml index 9195b125..3dfc90cf 100644 --- a/.github/workflows/devcontainer-prebuild.yml +++ b/.github/workflows/devcontainer-prebuild.yml @@ -29,11 +29,20 @@ name: Devcontainer prebuild on: push: branches: [ "main" ] - # Exactly the inputs to the hash. `build.context` is `.devcontainer`, so the - # hashed context is this directory and nothing else -- which is what makes a - # path filter honest here rather than an optimisation that silently ships a - # stale image. A commit that changes `devlaunch/`, `rust/` or `pyproject.toml` - # cannot change the tag, so there is nothing for this workflow to do. + # A superset of the inputs to the hash, which is the safe direction to err in. + # `build.context` is `.devcontainer`, so nothing outside this directory can + # move the tag -- a commit to `devlaunch/`, `rust/` or `pyproject.toml` has + # nothing for this workflow to do. Inside it, `.devcontainer/.dockerignore` + # narrows the hash further, to the files the image is actually built from, so + # some commits this filter catches cannot move the tag either. Those cost a + # registry lookup and nothing else: `devpod build` finds the prebuild already + # published and returns without rebuilding or repushing. + # + # Narrowing the filter to match the hash exactly is the change not to make. + # It would mean restating `.dockerignore`'s patterns here, in a second place, + # inverted -- and the failure mode of getting that wrong is a commit that + # moved the tag with no image published at it, which is a silent return to + # local builds for everyone on `main`. # # The workflow file itself is in the list because a change to how the image # is built or tagged should produce one. diff --git a/CHANGELOG.md b/CHANGELOG.md index b6d6652c..1d6682dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Editing prose under `.devcontainer/` no longer throws away the prebuilt + container.** Opening this repository with `dl` pulls a published image instead + of building one, and it had gone back to building. The prebuild tag is a hash + of the build context, and with a Dockerfile that `COPY`s nothing devpod hashes + the *whole* context directory -- so the feature's `README.md`, its + `TROUBLESHOOTING.md`, the host-side `initializeCommand`, and this manifest's + own comments were all hashed into a tag that is supposed to move only when the + image would differ. None of the four can reach the image. + + Measured on devpod 0.26.1, amd64, at `4db3427`: one comment line appended to + `claude-code/README.md` and one to `claude-code/init-host.sh` moved the tag + from `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7`, which CI had published and + every launch pulled, to `devpod-90e9641b9d3661f681e640eeedb3a410`, which + nothing had ever built. The cost was several minutes per launch, on every + branch carrying the edit, and the only symptom was that opening a container + was slow again -- devpod treats an unmatched tag as a cache miss and builds + locally without complaint. + + `.devcontainer/.dockerignore` is the fix, and the narrowing is checked in both + directions rather than only the useful one. Prose, the host hook and + `devcontainer.json` stop moving the tag; the Dockerfile, + `claude-code/install.sh`, `claude-code/devcontainer-feature.json` and the + build inputs the manifest declares still do -- `build.cacheFrom` and an added + feature were both confirmed to move it, since devpod hashes those from the + parsed config rather than from the file. An exclusion too wide would be the + worse bug: a launch pulling an image built from a Dockerfile the branch no + longer has, coming up fine and being the wrong container. + + **One rebuild is still owed.** The `.dockerignore` is itself part of the + context, so this commit moves the tag once; launches build locally until + `devcontainer-prebuild.yml` republishes on `main`. + ## [0.35.0] - 2026-09-09 ### Added diff --git a/docs/development.md b/docs/development.md index 0a85a029..91b8947b 100644 --- a/docs/development.md +++ b/docs/development.md @@ -396,14 +396,26 @@ Two consequences worth knowing: what makes the tag usable. The Dockerfile copies nothing out of the context, so the root was never needed, but it was hashed, which meant a different tag on every commit to any file and a prebuilt image that never matched one. - Scoped to `.devcontainer`, the tag moves when `.devcontainer/**` moves, the - Dockerfile and the local feature's scripts included. -- **A commit whose `.devcontainer/` differs from the last prebuild builds - locally.** That is the correct answer rather than a gap: the alternative is a - container built from something other than what the branch asks for. The pull - comes back once the change is on `main`. What the tag does not promise is the - converse, that one `.devcontainer/` tree always yields one image; see "What - the prebuild tag does not promise" below. +- **Inside `.devcontainer/`, only the files the image is built from are + hashed**, and `.devcontainer/.dockerignore` is what draws that line. Scoping + the context to this directory was not enough on its own: a Dockerfile with no + `COPY` leaves devpod's `includeFiles` empty and it hashes the whole directory, + so the feature's `README.md`, its `TROUBLESHOOTING.md`, the host-side + `initializeCommand` and `devcontainer.json`'s own comments all moved the tag + without changing the image. Measured on devpod 0.26.1 at `4db3427`: one + comment line appended to `claude-code/README.md` and one to + `claude-code/init-host.sh` moved the tag off the published + `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7` and onto a tag nothing had ever + built. What still moves it: the Dockerfile, `claude-code/install.sh`, + `claude-code/devcontainer-feature.json`, and the build inputs + `devcontainer.json` declares -- `build`, `features`, `image`, `name` -- which + devpod hashes from the parsed config rather than from the file. +- **A commit that changes what the image is built from builds locally until it + reaches `main`.** That is the correct answer rather than a gap: the + alternative is a container built from something other than what the branch + asks for. The pull comes back once the change is on `main`. What the tag does + not promise is the converse, that one `.devcontainer/` tree always yields one + image; see "What the prebuild tag does not promise" below. `.github/workflows/devcontainer-prebuild.yml` publishes it, on pushes to `main` that touch `.devcontainer/**` and on manual dispatch. Its path filter is exactly diff --git a/test/unit/test_devcontainer_manifest.py b/test/unit/test_devcontainer_manifest.py index 5dea4911..070b4d57 100644 --- a/test/unit/test_devcontainer_manifest.py +++ b/test/unit/test_devcontainer_manifest.py @@ -46,6 +46,45 @@ PIXI_GLOBAL_INSTALL = "pixi global install " DOCKERFILE = REPO_ROOT / ".devcontainer" / "Dockerfile" +CONTEXT_DIR = REPO_ROOT / ".devcontainer" +CONTEXT_DOCKERIGNORE = CONTEXT_DIR / ".dockerignore" + +# Every file under the build context, split by whether the *image* can see it. +# +# devpod hashes the context as well as the config, and with includeFiles empty -- +# which is what a Dockerfile with no COPY leaves it -- it hashes the whole +# directory (pkg/util/hash.DirectoryHash). So anything in here that the build +# cannot read is a file whose edits throw the published prebuild away and buy +# nothing, and `.dockerignore` is what keeps it out of the hash. +# +# Split by hand rather than derived, because "can the image see this" is not a +# property of the path: `install.sh` and `devcontainer-feature.json` are the +# local feature devpod bakes in, while `init-host.sh` sits beside them and runs +# on the host. The two sets together have to account for every file present, so +# a file added here is a test failure until somebody says which side it is on. +CONTEXT_BUILD_INPUTS = frozenset( + { + "Dockerfile", + "claude-code/install.sh", + "claude-code/devcontainer-feature.json", + } +) +CONTEXT_NON_BUILD_INPUTS = frozenset( + { + # Prose, here because the Features spec wants it beside its feature. + "claude-code/README.md", + "claude-code/TROUBLESHOOTING.md", + # `initializeCommand`, which runs on the host before the container + # exists. devpod clears DevContainerActions out of the config half of + # the hash for exactly that reason, then hashed the script back in as + # context. + "claude-code/init-host.sh", + # devpod read this file to get here; it does not read it out of the + # context, and the config half of the hash already carries every build + # input it declares. + "devcontainer.json", + } +) LOCKFILE = REPO_ROOT / "pixi.lock" #: Lock-file format version -> the lowest pixi release that can read it. #: @@ -202,6 +241,42 @@ def test_a_readonly_spelling_the_membership_test_misreads_is_refused(flag): parse_mount(f"source=/a,target=/b,type=bind,{flag}") +def dockerignore_patterns() -> list: + """The `.dockerignore` lines devpod's pattern matcher will actually see. + + Comments and blank lines are dropped, which is what `ignorefile.ReadAll` + does before the patterns reach `patternmatcher`. Measured on devpod 0.26.1: + with this file's three patterns the debug line reports `excludeCount=4`, the + fourth being devpod's own feature-staging folder, which it appends itself. + """ + lines = CONTEXT_DOCKERIGNORE.read_text().splitlines() + return [line.strip() for line in lines if line.strip() and not line.startswith("#")] + + +def excluded_from_the_context_hash(rel_path: str) -> bool: + """Whether `.dockerignore` keeps `rel_path` out of the context hash. + + A deliberately small matcher, covering only the two pattern forms this + repository's `.dockerignore` uses: `**/*.ext`, which docker matches at any + depth including the top level, and an exact context-relative path. Any other + form raises rather than returning False, so this cannot quietly report "not + excluded" about a pattern it does not understand -- which is the one way a + guard like this fails without saying so. + """ + for pattern in dockerignore_patterns(): + if pattern.startswith("**/*."): + if rel_path.endswith(pattern[len("**/*") :]): + return True + elif "*" in pattern or "?" in pattern or "[" in pattern: + raise AssertionError( + f"{CONTEXT_DOCKERIGNORE.name} pattern {pattern!r} is a glob form this " + "test cannot evaluate; teach it the form or express the exclusion as a path" + ) + elif rel_path == pattern or rel_path.startswith(pattern + "/"): + return True + return False + + def test_devcontainer_manifest_is_this_repos_and_parses(devcontainer): """Comments stripped, this file is JSON, and it is the manifest we mean. @@ -866,3 +941,136 @@ def test_the_host_hook_heals_the_socket_it_is_mounted_from(devcontainer, mounts) f"init-host.sh does not heal a stale mount at ${variable}, which is what " "the manifest binds the agent socket from" ) + + +def test_the_dockerfile_copies_nothing_out_of_the_build_context(): + """The premise every exclusion below rests on. + + `.dockerignore` is safe here only because the build never reads the context: + no `COPY`, no `ADD`, so the tar devpod assembles is opened by nothing. Add + one and the excluded files stop being invisible to the image and start being + *missing* from it -- a build that fails, or worse, a stale prebuild that + matches a context it no longer describes. + + So this is the test to read first when `.dockerignore` looks wrong: it is the + claim, and the exclusions are downstream of it. + """ + instructions = [ + line.split(maxsplit=1)[0].upper() + for line in DOCKERFILE.read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#") + ] + assert not {"COPY", "ADD"} & set(instructions), ( + f"{DOCKERFILE.name} now reads the build context, which .dockerignore is filtering; " + "either the copied paths have to be un-excluded or the exclusions have to go" + ) + + +def test_every_file_in_the_build_context_is_classified_as_a_build_input_or_not(): + """A file added under `.devcontainer/` has to be put on one side or the other. + + Left unclassified it defaults to the expensive answer -- hashed, so its edits + move the prebuild tag -- and defaults to it silently, because nothing about a + launch changes except that it takes minutes again. Failing here is the cheap + version of that conversation. + + `.dockerignore` itself is neither: it is what draws the line, and it is + hashed, correctly, since changing what the hash can see should move the tag. + """ + present = { + str(path.relative_to(CONTEXT_DIR)) + for path in CONTEXT_DIR.rglob("*") + if path.is_file() and path.name != ".dockerignore" + } + classified = CONTEXT_BUILD_INPUTS | CONTEXT_NON_BUILD_INPUTS + assert present == classified, ( + f"the build context holds {sorted(present - classified)} that no test has " + f"classified, and expects {sorted(classified - present)} that are not there; " + "add each to CONTEXT_BUILD_INPUTS or CONTEXT_NON_BUILD_INPUTS" + ) + + +def test_the_prebuild_tag_cannot_move_for_a_file_the_image_never_sees(): + """The fix for a prebuild that stopped being pulled. + + Measured on devpod 0.26.1, amd64, at 4db3427: appending a comment line to + `claude-code/README.md` and another to `claude-code/init-host.sh` moved the + tag from `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7`, which CI had published + and every launch pulled, to `devpod-90e9641b9d3661f681e640eeedb3a410`, which + nothing had ever published. Neither file can reach the image. The cost was a + full local build per launch, on every branch carrying the edit, and the only + symptom was that opening a container was slow again. + """ + for rel_path in sorted(CONTEXT_NON_BUILD_INPUTS): + assert excluded_from_the_context_hash(rel_path), ( + f"{rel_path} cannot reach the image but is still hashed into the prebuild " + f"tag; {CONTEXT_DOCKERIGNORE.name} has to exclude it or editing it throws " + "the published prebuild away" + ) + + +def test_the_prebuild_tag_still_moves_for_every_file_the_image_is_built_from(): + """The other half, and the one that makes the exclusions worth having. + + An exclusion too wide is worse than none: the tag stops moving when the image + would genuinely differ, so a launch pulls an image built from a Dockerfile or + a feature installer that is no longer the one on the branch. That failure is + quieter than a slow build and much harder to attribute -- the container comes + up, it is simply not the container the branch asks for. + """ + for rel_path in sorted(CONTEXT_BUILD_INPUTS): + assert not excluded_from_the_context_hash(rel_path), ( + f"{rel_path} is built into the image but {CONTEXT_DOCKERIGNORE.name} keeps it " + "out of the prebuild tag, so a launch can pull an image that predates it" + ) + + +def test_the_excluded_manifest_leaves_its_build_inputs_in_the_hash(devcontainer): + """Excluding `devcontainer.json` is only safe while devpod hashes what it declares. + + `normalizeConfigForHash` keeps `name`, `features`, `overrideFeatureInstallOrder`, + `image`, `dockerfile`, `context` and the whole of `build`, and clears the rest -- + `mounts`, `containerEnv`, `postCreateCommand`, `customizations` -- because devpod + applies those when it creates the container rather than when it builds the image. + So the config half of the hash already carries every build input this file has, + and hashing its raw bytes on top added only the comments, which this manifest is + mostly made of. + + What this test holds is the boundary: every key in here is either one devpod + hashes or one it deliberately applies at create time. A key that is neither + would be a build input the tag could not see, which is the stale-image failure + the test above describes, reached from the other direction. + """ + assert "devcontainer.json" in dockerignore_patterns(), ( + "this test is about the manifest being excluded from the context hash, and it " + "is not excluded; either restore the exclusion or drop this test" + ) + hashed = { + "name", + "features", + "overrideFeatureInstallOrder", + "image", + "dockerfile", + "context", + "build", + } + applied_at_create = { + "mounts", + "containerEnv", + "postCreateCommand", + "customizations", + "initializeCommand", + "runArgs", + "workspaceFolder", + "workspaceMount", + "remoteUser", + "remoteEnv", + "privileged", + "init", + } + unaccounted = set(devcontainer) - hashed - applied_at_create + assert not unaccounted, ( + f"{DEVCONTAINER_JSON.name} declares {sorted(unaccounted)}, which this test cannot " + "say devpod hashes or applies at create time; if any of them affects the image, the " + "manifest can no longer be excluded from the context hash" + )