From 3d3313c0ee99f308e19766444535db3cc20b109e Mon Sep 17 00:00:00 2001 From: lilleman Date: Wed, 5 Aug 2026 19:17:07 +0200 Subject: [PATCH] Document the shadowing risk and the leftover dir; close two holes in the mount guard --- .dockerignore | 2 ++ AGENTS.md | 7 ++++++- README.md | 7 +++++++ src/compose.test.ts | 15 ++++++++++----- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/.dockerignore b/.dockerignore index b5ac416..196eef9 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,4 +1,6 @@ .git +# Load-bearing: deps live at /node_modules, so a builder's stray copy would bake in at +# /app/node_modules and shadow them for every consumer of the image, production included. node_modules npm-debug.log *.log diff --git a/AGENTS.md b/AGENTS.md index 203072a..7dea587 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -329,7 +329,12 @@ them. Revisit only if the stated reason stops holding. as `/node_modules` no longer resolves, so `src/ui/icons.test.ts` locates lucide-static by specifier via `import.meta.resolve`; and `npm install` must not run in `/app` or it recreates the problem — README → Extending the core documents `--package-lock-only` plus `--user` instead, which - is also why the image sets `npm_config_cache` (the host uid has no home dir here). Decided 2026-08-05. + is also why the image sets `npm_config_cache` (the host uid has no home dir here). Nothing masks + that path any more, so anything at `/app/node_modules` now **shadows `/node_modules` silently** — + wrong dependency code, no warning, and CI stays green because a fresh clone has none. An empty + leftover dir is harmless (resolution falls through); a populated one from the superseded root + `npm install` needs `sudo` to remove, which is worse than the bug this replaced. `.dockerignore`'s + `node_modules` line is what keeps a builder's stray copy out of the image. Decided 2026-08-05. ## Docker only — no host tooling diff --git a/README.md b/README.md index 4ed6368..a64dd5d 100644 --- a/README.md +++ b/README.md @@ -1768,6 +1768,13 @@ so for now the error names the rule it tripped instead. `users:`/`groups:`/`oauth2-clients:` × `read`/`write`, so a copy taken before this needs re-copying. `ADMIN_PERMISSIONS` is held to the same rule, but an unusable value there is dropped with a warning rather than failing the boot. See [Naming a permission](#naming-a-permission). +- **Deps moved to `/node_modules`, above `/app`** (2026-08-05). The dev override no longer mounts an + anonymous volume at `/app/node_modules`, so after pulling, delete the empty directory the daemon + left behind: `rmdir node_modules` — no `sudo`, it is empty. `docker volume prune` reclaims the + orphaned volumes. Keep that path clear: anything there shadows the image's deps silently, and a + populated one is root-owned and needs `sudo` to remove. Add a dependency with the + `--package-lock-only` command in [Extending the core](#extending-the-core), never a bare + `npm install` in `/app`. ## Observability diff --git a/src/compose.test.ts b/src/compose.test.ts index fe53874..535c2a8 100644 --- a/src/compose.test.ts +++ b/src/compose.test.ts @@ -94,14 +94,19 @@ test("deps live above WORKDIR, so no mount creates a root-owned dir in the check // a volume at /app/node_modules leaves a root-owned node_modules/ in the developer's own checkout // (dev bind-mounts `.:/app`). Installing above /app lets Node resolve upward instead — nothing to // shadow, so nothing to mount over. - const beforeWorkdir = read("Dockerfile").split("WORKDIR /app")[0]!; + const dockerfile = read("Dockerfile"); + // Asserted, not assumed: split() returns the whole file when the marker is missing, which would + // silently widen "before WORKDIR" to "anywhere". + assert.ok(dockerfile.includes("WORKDIR /app"), "the app dir is /app"); + const beforeWorkdir = dockerfile.split("WORKDIR /app")[0]!; assert.match(beforeWorkdir, /npm ci/, "npm ci runs before WORKDIR /app"); assert.match(beforeWorkdir, /mv\s+node_modules\s+\/node_modules/, "and its tree lands at /node_modules"); - const composeFiles = readdirSync(new URL("../e2e-tests", import.meta.url)) - .filter((f) => f.startsWith("compose.")) - .map((f) => `e2e-tests/${f}`); - for (const f of ["compose.yml", "compose.override.yml", ...composeFiles]) + const composeFiles = (dir: string) => + readdirSync(new URL(`../${dir}`, import.meta.url)) + .filter((f) => f.startsWith("compose.") && f.endsWith(".yml")) + .map((f) => `${dir}${f}`); + for (const f of [...composeFiles(""), ...composeFiles("e2e-tests/")]) assert.ok(!read(f).includes("/app/node_modules"), `${f} mounts nothing at /app/node_modules`); });