From 1e9b231ec63e0990d09bb6efc8937500d27bd81f Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 17:30:26 +0200 Subject: [PATCH 1/4] fix(docker): copy .npmrc before npm ci, pin FROM to the .nvmrc major The Dockerfile ran npm ci before COPY . . landed .npmrc, so engine-strict=true never reached the install: an unsupported base image reproduced a confusing lockfile-sync error instead of a clean EBADENGINE. FROM node:lts-slim also floats independently of .nvmrc/ engines.node and will silently drift once Node 26 becomes LTS. Copy .npmrc alongside package*.json before npm ci, and pin the image to node:24-slim (the .nvmrc major) instead of the floating lts-slim tag. Add a drift-guard unit test asserting the Dockerfile major and .nvmrc major match, so the two can't diverge unnoticed again. Closes #4060 Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- Dockerfile | 6 ++- .../tests/dockerfileToolchain.unit.tests.js | 43 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 scripts/tests/dockerfileToolchain.unit.tests.js diff --git a/Dockerfile b/Dockerfile index a3bbe88a2..44dc0f8bf 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,6 @@ -FROM node:lts-slim +# Pinned to the major declared in .nvmrc (not `lts-slim`, which floats +# independently of it). Kept in sync by scripts/tests/dockerfileToolchain.unit.tests.js. +FROM node:24-slim # switch user USER node @@ -9,7 +11,7 @@ WORKDIR /home/node # Install app dependencies & setup ARG NODE_ENV=production ENV NODE_ENV=${NODE_ENV} -COPY --chown=node:node package*.json ./ +COPY --chown=node:node package*.json .npmrc ./ RUN if [ "$NODE_ENV" = "test" ]; then npm ci --include=dev; else npm ci --omit=dev; fi COPY --chown=node:node . . diff --git a/scripts/tests/dockerfileToolchain.unit.tests.js b/scripts/tests/dockerfileToolchain.unit.tests.js new file mode 100644 index 000000000..8d77790f5 --- /dev/null +++ b/scripts/tests/dockerfileToolchain.unit.tests.js @@ -0,0 +1,43 @@ +/** + * Unit tests for the Dockerfile ↔ toolchain consistency guarded by #4060. + * + * `FROM` cannot read `.nvmrc` at build time, so the Node major is restated + * as a literal in the Dockerfile instead of derived. These tests are the + * drift guard: if the Dockerfile's major and `.nvmrc` are ever edited + * independently, this fails instead of the drift going unnoticed until a + * Node LTS rollover changes what `FROM node:lts-slim` resolves to. + * + * The second test guards the sibling defect from the same issue: `.npmrc` + * (carrying `engine-strict=true`) must be copied into the build context + * before `npm ci` runs, or the engines fail-fast never reaches the + * Docker build. + */ +import { describe, test, expect } from '@jest/globals'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; + +const rootDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const dockerfile = readFileSync(path.join(rootDir, 'Dockerfile'), 'utf8'); + +describe('Dockerfile toolchain pin (#4060)', () => { + test('FROM node:-slim matches the major pinned in .nvmrc', () => { + const nvmrcMajor = readFileSync(path.join(rootDir, '.nvmrc'), 'utf8') + .trim() + .replace(/^v/, '') + .split('.')[0]; + + const match = dockerfile.match(/^FROM node:(\d+)(?:\.\d+)*-slim/m); + expect(match).not.toBeNull(); + expect(match[1]).toBe(nvmrcMajor); + }); + + test('.npmrc is copied into the build context before npm ci runs', () => { + const copyIndex = dockerfile.search(/^COPY[^\n]*\.npmrc[^\n]*$/m); + const npmCiIndex = dockerfile.search(/^RUN[^\n]*npm ci/m); + + expect(copyIndex).toBeGreaterThan(-1); + expect(npmCiIndex).toBeGreaterThan(-1); + expect(copyIndex).toBeLessThan(npmCiIndex); + }); +}); From 8604e5c148eaab7184671fa1414a3f18d7ac793c Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 17:32:25 +0200 Subject: [PATCH 2/4] docs(errors): record the Docker install-path gap left by #4053 Closes #4060 Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- ERRORS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/ERRORS.md b/ERRORS.md index 6ddd39400..9c36473b5 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -37,3 +37,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-07-17] billing/stripe: #3964 fixed the admin (409 abort) and reconcile (skip-comparison) call sites of the shared plan resolver but left `billing.webhook.service.js resolvePlan`'s `?? 'free'` fallback in place -> a webhook for an existing paid org whose Stripe price is unresolvable (unmapped `priceId`, no valid `metadata.planId` — e.g. a manually-sold enterprise price) silently downgraded the org to `'free'` on the next `customer.subscription.updated`/`.created`/`invoice.payment_succeeded` event; a webhook cannot 409-abort mid-flight like the admin path, so the fix instead retains the org's/subscription's already-loaded `.plan` (never a hardcoded `'free'`) and emits `billing.webhook.plan_unresolved` for manual review — only a brand-new subscription with zero prior plan reference (no DB row anywhere for that org) still defaults to `'free'`; see pierreb-devkit/Node#3970 - [2026-07-25] billing: `forceRotateForPlanChange` (week-quota snapshot rotation) was wired only into `handleSubscriptionUpdated`'s plan-change block -> `handleCheckoutCompleted` and both branches of `handleSubscriptionCreated` never rotated the current-week quota snapshot after a mid-week plan activation, so an upgraded org kept the previous plan's weekly quota (often 0 on free→paid) until the next weekly reset; fixed by calling the same non-fatal, logged `forceRotateForPlanChange(organizationId, { preserveUsage: true })` from all three activation call sites, and by making `billing.usage.service.js incrementMeter` read the live plan quota (already fetched for the snapshot write) instead of the potentially-stale `updatedDoc.meterQuota` for overflow decisions — mirrors the existing live-quota display fix in `billing.controller.js`; see pierreb-devkit/Node#3988 - [2026-09-04] deps/engines: `engines.node` declared `">=22.0.0"` while the committed lock was already npm-11-shaped, so `npm ci` under npm 10 (bundled by every Node 22.x) failed mid-install with a confusing `Missing: conventional-commits-filter@6.0.1 from lock file` error instead of a clean engines rejection -> a public package's `engines.node`/`engines.npm` must bound the exact npm major that wrote the lock, not just "a Node version that happens to work today"; pin the toolchain with `.nvmrc` + CI's `setup-node` reading it (`node-version-file`, not a floating `lts/*`) and add `engine-strict=true` to `.npmrc` so an unsupported toolchain fails fast at the engines check instead of at dependency resolution; see pierreb-devkit/Node#4053 +- [2026-09-04] docker: #4053 pinned the toolchain for CI and local installs, but the Dockerfile still copied `package*.json`, ran `npm ci`, and only then `COPY . .` — so `.npmrc` (`engine-strict=true`) landed AFTER the install, and `FROM node:lts-slim` floated independently of `.nvmrc`, so an unsupported base image inside Docker still reproduced the confusing lockfile error instead of a clean `EBADENGINE`, and the image could silently drift onto a newer Node major once it enters LTS -> a toolchain fail-fast fix must be verified in every install path, not just the one exercised by CI; copy `.npmrc` alongside `package*.json` before `npm ci` in every installing stage, and pin `FROM` to the `.nvmrc` major with a unit test checking the two against each other instead of a floating tag; see pierreb-devkit/Node#4060 From ff34d0569edd630adeb65dbb8579afbe60a171e0 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 22:18:33 +0200 Subject: [PATCH 3/4] test(docker): make the toolchain guard stage-aware (#4060) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Dockerfile ↔ .nvmrc/.npmrc drift guard only inspected the first FROM and the first COPY/RUN pair, so a multi-stage split with a correct build stage but a drifted runtime stage (the one that actually ships) passed silently. Split the Dockerfile into per-stage blocks and check every stage: every FROM node:* must match the .nvmrc major, and every stage that runs npm ci must copy .npmrc first in that same stage. Stage-boundary detection is kept separate from image-token parsing so an unrecognized FROM shape (e.g. a --platform flag) throws instead of being silently absorbed into the previous stage — the same blind-spot class the original finding was about. Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- .../tests/dockerfileToolchain.unit.tests.js | 107 ++++++++++++++++-- 1 file changed, 97 insertions(+), 10 deletions(-) diff --git a/scripts/tests/dockerfileToolchain.unit.tests.js b/scripts/tests/dockerfileToolchain.unit.tests.js index 8d77790f5..14dcc7f79 100644 --- a/scripts/tests/dockerfileToolchain.unit.tests.js +++ b/scripts/tests/dockerfileToolchain.unit.tests.js @@ -11,6 +11,33 @@ * (carrying `engine-strict=true`) must be copied into the build context * before `npm ci` runs, or the engines fail-fast never reaches the * Docker build. + * + * Both checks are stage-aware: the Dockerfile is split on every top-level + * `FROM` into per-stage blocks, and each check walks every stage instead of + * only the first. A single-occurrence check would miss a multi-stage build + * where an earlier stage (e.g. a build stage) is correct but a later one + * (e.g. the runtime stage that actually ships) drifts. + * + * Scope, decided deliberately for #4060's review: + * - A `FROM` whose image is not `node:...` (a distroless/scratch runtime, + * or `FROM `) has no Node major to check, so it's + * skipped by the major-pin test — but if that stage runs `npm ci`, it's + * still held to the .npmrc-before-npm-ci rule. + * - The .npmrc check is stage-local only: it does not trace whether a + * stage that `FROM`s a previous *named* stage inherits an already-copied + * `.npmrc` from that parent. Resolving that means walking the FROM + * graph, which is a Dockerfile parser, not a drift guard for one file. + * A stage that runs `npm ci` is expected to COPY `.npmrc` itself; widen + * this if that inheritance pattern shows up for real. + * - A digest-pinned image (`node:24-slim@sha256:...`) is accepted the + * same as its tag-only form for the major check. + * - Stage boundaries are detected separately from image-token parsing: any + * line starting with `FROM` opens a new stage, but if its shape can't be + * parsed (e.g. a `--platform=...` flag, which this guard doesn't + * support) the test throws instead of silently merging it into the + * previous stage — a strict-regex-as-splitter would make an + * unrecognized `FROM` vanish rather than fail, which is the same class + * of blind spot #4060's review flagged in the first place. */ import { describe, test, expect } from '@jest/globals'; import { readFileSync } from 'node:fs'; @@ -20,24 +47,84 @@ import path from 'node:path'; const rootDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); const dockerfile = readFileSync(path.join(rootDir, 'Dockerfile'), 'utf8'); +/** + * Splits the Dockerfile into per-stage blocks. A new stage starts at every + * top-level `FROM` line; each block runs from its `FROM` up to (but not + * including) the next `FROM`, or end of file for the last stage. + * + * Boundary detection (which lines start a stage) is intentionally separate + * from image-token extraction (what the stage's image is): matching both + * in one strict regex would make a `FROM` line the regex doesn't recognize + * silently disappear into the previous stage's block instead of failing — + * exactly the kind of miss this guard exists to prevent. So any line that + * starts with `FROM` opens a stage; if its shape can't then be parsed, the + * test throws rather than guessing. + */ +function getStages(text) { + const fromLineRe = /^FROM\b[^\n]*$/gim; + const imageRe = /^FROM[ \t]+(\S+)(?:[ \t]+AS[ \t]+\S+)?[ \t]*$/i; + + const froms = []; + let match; + while ((match = fromLineRe.exec(text)) !== null) { + const line = match[0]; + const imageMatch = line.match(imageRe); + if (!imageMatch) { + throw new Error( + `Dockerfile has a FROM line the stage-splitter can't parse (e.g. a --platform flag ` + + `isn't supported) — extend getStages() in dockerfileToolchain.unit.tests.js or ` + + `simplify the line: "${line}"`, + ); + } + froms.push({ index: match.index, image: imageMatch[1] }); + } + + return froms.map((from, i) => ({ + image: from.image, + block: text.slice(from.index, i + 1 < froms.length ? froms[i + 1].index : text.length), + })); +} + describe('Dockerfile toolchain pin (#4060)', () => { - test('FROM node:-slim matches the major pinned in .nvmrc', () => { + test('every FROM node:-slim stage matches the major pinned in .nvmrc', () => { const nvmrcMajor = readFileSync(path.join(rootDir, '.nvmrc'), 'utf8') .trim() .replace(/^v/, '') .split('.')[0]; - const match = dockerfile.match(/^FROM node:(\d+)(?:\.\d+)*-slim/m); - expect(match).not.toBeNull(); - expect(match[1]).toBe(nvmrcMajor); + const stages = getStages(dockerfile); + const nodeStages = stages.filter((stage) => /^node:/.test(stage.image)); + expect(nodeStages.length).toBeGreaterThan(0); + + for (const stage of nodeStages) { + const majorMatch = stage.image.match(/^node:(\d+)(?:\.\d+)*-slim(?:@sha256:[0-9a-f]{64})?$/); + if (!majorMatch) { + throw new Error( + `FROM ${stage.image} is a node image but not a pinned "node:-slim" form ` + + `(.nvmrc pins major ${nvmrcMajor}) — a floating tag like "node:lts-slim" defeats the pin`, + ); + } + expect(majorMatch[1]).toBe(nvmrcMajor); + } }); - test('.npmrc is copied into the build context before npm ci runs', () => { - const copyIndex = dockerfile.search(/^COPY[^\n]*\.npmrc[^\n]*$/m); - const npmCiIndex = dockerfile.search(/^RUN[^\n]*npm ci/m); + test('.npmrc is copied before npm ci runs, in every stage that runs npm ci', () => { + const npmCiRe = /^RUN[^\n]*npm ci/m; + const npmrcCopyRe = /^COPY[^\n]*\.npmrc[^\n]*$/m; + + const stages = getStages(dockerfile); + const npmCiStages = stages.filter((stage) => npmCiRe.test(stage.block)); + expect(npmCiStages.length).toBeGreaterThan(0); - expect(copyIndex).toBeGreaterThan(-1); - expect(npmCiIndex).toBeGreaterThan(-1); - expect(copyIndex).toBeLessThan(npmCiIndex); + for (const stage of npmCiStages) { + const npmCiIndex = stage.block.search(npmCiRe); + const copyIndex = stage.block.search(npmrcCopyRe); + if (copyIndex === -1 || copyIndex > npmCiIndex) { + throw new Error( + `Stage "FROM ${stage.image}" runs npm ci without copying .npmrc first in that same ` + + 'stage (engine-strict from .npmrc must be in place before npm ci runs)', + ); + } + } }); }); From 624124f029a1284c9d9f87fd34c334c170b71898 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 22:41:50 +0200 Subject: [PATCH 4/4] docs(test): add @param/@returns to the getStages helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stage-splitter is a named helper, so the repo JSDoc guideline applies to it — the test-file exception covers anonymous test-framework callbacks only. Documents the returned stage shape and the deliberate throw on an unparseable FROM line. Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- scripts/tests/dockerfileToolchain.unit.tests.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/tests/dockerfileToolchain.unit.tests.js b/scripts/tests/dockerfileToolchain.unit.tests.js index 14dcc7f79..589cb6884 100644 --- a/scripts/tests/dockerfileToolchain.unit.tests.js +++ b/scripts/tests/dockerfileToolchain.unit.tests.js @@ -59,6 +59,14 @@ const dockerfile = readFileSync(path.join(rootDir, 'Dockerfile'), 'utf8'); * exactly the kind of miss this guard exists to prevent. So any line that * starts with `FROM` opens a stage; if its shape can't then be parsed, the * test throws rather than guessing. + * + * @param {string} text - the full Dockerfile contents + * @returns {Array<{image: string, block: string}>} one entry per stage, in file + * order: `image` is the raw image token from the `FROM` line (tag or + * digest-pinned, or a prior stage's alias), `block` is that stage's lines up + * to the next `FROM` or end of file. + * @throws {Error} if a `FROM` line's shape cannot be parsed, rather than + * silently folding it into the previous stage. */ function getStages(text) { const fromLineRe = /^FROM\b[^\n]*$/gim;