Fix two build failures CI caught on the first real run

Neither could fail on this machine: pi needs node >=22.19.0 and this box is below it,
so the loader assertions skip locally and only ran for the first time in CI.

1. A comment broke the build. A JSDoc line reading "the no*/else branches" closed its
   own block comment with the */ inside it, turning the rest of the sentence into code:
   SyntaxError, Unexpected token 'else'. loader.mjs stopped parsing. The pure tests do
   not import it, so nothing local noticed.

   Added test/sources-parse.test.mjs: node --check over every runner source. It needs no
   dependencies and no particular node version, so unlike the loader assertions it runs
   everywhere -- including here, where the tests that would have caught this cannot.
   Verified by reintroducing the bug and watching it fail.

2. `npm i -g` does not make a package importable. The runner did
   `import "@earendil-works/pi-coding-agent"` while the Dockerfile installed it globally;
   node's ESM resolver walks node_modules upward from the importing file and ignores the
   global prefix entirely. NODE_PATH does not help either -- it is CommonJS-only. The
   image built fine and every job would have died on module-not-found.

   Now a local `npm ci --omit=dev` from the lockfile at /app, with /app/node_modules/.bin
   on PATH so the pi and playwright-cli binaries come from that same install -- one pin,
   no chance of the CLI drifting from the imported library. @playwright/cli moves into the
   runner's dependencies for the same reason, which also pins playwright transitively
   rather than resolving it at build time via npx.

Both are the kind of thing only a real build finds, which is the argument for the image
existing before the worker rather than after it.

Author: edgehero

image/Dockerfile

@@ -40,23 +40,36 @@ RUN mkdir -p -m 755 /etc/apt/keyrings \
     && apt-get install -y --no-install-recommends gh \
     && rm -rf /var/lib/apt/lists/*
 
-# The workspace .npmrc's engine-strict does NOT reach a global install: `npm i -g` reads
-# only $HOME/.npmrc and $PREFIX/etc/npmrc, never a project file. Set it explicitly or the
-# node-floor guard silently does not cover the one install that matters most.
+# engine-strict is set explicitly because npm reads it from $HOME/.npmrc and
+# $PREFIX/etc/npmrc but NEVER from a project .npmrc for installs outside a project root.
+# Without this the node-floor guard silently does not cover the install that matters most.
 ENV NPM_CONFIG_ENGINE_STRICT=true
 
-# Exact versions, no ranges. pi breaks between MINORS -- a past regression silently dropped
-# sendUserMessage after newSession, and the package was renamed mid-flight.
-RUN npm install -g --no-fund --no-audit \
-      @earendil-works/pi-coding-agent@0.80.7 \
-      @playwright/cli@0.1.17
+# A LOCAL install from the lockfile -- deliberately not `npm i -g`.
+#
+# A global install puts binaries on PATH but does NOT make a package importable by a bare
+# specifier: node's ESM resolver walks node_modules upward from the importing file and
+# ignores the global prefix entirely. NODE_PATH does not help either -- it is CommonJS-only
+# and the resolver ignores it for ESM. `import "@earendil-works/pi-coding-agent"` from
+# /app/image/runner therefore only resolves against a real node_modules above it.
+#
+# Deps are copied before source so a source edit does not invalidate this layer.
+WORKDIR /app
+COPY package.json package-lock.json ./
+COPY image/runner/package.json ./image/runner/
+RUN npm ci --omit=dev --no-audit --no-fund
+
+# The pi and playwright-cli binaries come from that same install: one pin, one copy on disk,
+# no chance of the CLI and the imported library drifting to different versions.
+ENV PATH="/app/node_modules/.bin:${PATH}"
 
 # Set BEFORE the browser install so root-time install and non-root-time lookup agree.
 # Root installs would otherwise land in /root/.cache/ms-playwright, which the non-root
 # runtime user cannot see -- a direct collision between two of our own constraints.
-# @playwright/cli does NOT install browsers; it is a thin wrapper. The standard installer does.
+# @playwright/cli does NOT install browsers; it is a thin wrapper. The standard installer,
+# pinned transitively by @playwright/cli and resolved from the lockfile, does.
 ENV PLAYWRIGHT_BROWSERS_PATH=/ms-playwright
-RUN npx --yes playwright@1.62.0-alpha-1783623505000 install --with-deps chromium \
+RUN playwright install --with-deps chromium \
     && rm -rf /var/lib/apt/lists/*
 
 # Non-root. CONST-ISOLATION-CONTAINER-PER-JOB.
@@ -71,15 +84,17 @@ RUN useradd --create-home --shell /bin/bash --uid 1001 pi
 # obvious fix. Create and chown explicitly.
 RUN mkdir -p /home/pi/.pi/agent \
     && chown -R pi:pi /home/pi/.pi \
-    && chown -R pi:pi /ms-playwright
+    && chown -R pi:pi /ms-playwright \
+    && chown -R pi:pi /app
 
 # The safety floor. Deliberately NOT at ~/.pi/agent/APPEND_SYSTEM.md: a trusted project's
 # .pi/APPEND_SYSTEM.md shadows that path via an early return in discoverAppendSystemPromptFile,
 # which would delete these rules from the prompt with no error and a job that succeeds. The
 # runner reads this path explicitly instead, so discovery cannot shadow it.
 COPY --chown=pi:pi guardrails/HARD_RULES.md /opt/pi-dispatch/HARD_RULES.md
 
-COPY --chown=pi:pi image/runner /runner
+# Source last: everything above is cacheable and this layer changes on every commit.
+COPY --chown=pi:pi image/runner /app/image/runner
 COPY --chown=pi:pi image/entrypoint.sh /entrypoint.sh
 RUN chmod +x /entrypoint.sh
 

image/entrypoint.sh

@@ -15,4 +15,4 @@
 # "bad interpreter" inside the container: a confusing error with a boring cause.
 set -eu
 
-exec node /runner/run-job.mjs
+exec node /app/image/runner/run-job.mjs

image/runner/package.json

@@ -10,6 +10,7 @@
     "node": ">=22.19.0"
   },
   "dependencies": {
-    "@earendil-works/pi-coding-agent": "0.80.7"
+    "@earendil-works/pi-coding-agent": "0.80.7",
+    "@playwright/cli": "0.1.17"
   }
 }

image/runner/src/loader.mjs

@@ -27,8 +27,8 @@ function readIfExists(path) {
  *   CONST-NO-CONTEXT-FILES-MANDATORY fails open by omission; this is the omission.
  *
  * - `noSkills`/`noExtensions` suppress cwd/package discovery, which would read the
- *   CHECKED-OUT branch -- a fork's branch on a PR-triggered job. The additional*Paths
- *   are merged in both the no*/else branches and are never trust-checked, so they load
+ *   CHECKED-OUT branch -- a fork's branch on a PR-triggered job. The additional paths are
+ *   merged whether or not those flags are set, and are never trust-checked, so they load
  *   exactly what the worker handed over and nothing from the tree. Project trust is
  *   therefore never granted: reload() is called without resolveProjectTrust.
  *

image/runner/test/sources-parse.test.mjs

@@ -0,0 +1,42 @@
+import assert from "node:assert/strict";
+import { execFileSync } from "node:child_process";
+import { readdirSync, statSync } from "node:fs";
+import { join } from "node:path";
+import { fileURLToPath } from "node:url";
+import { test } from "node:test";
+
+/**
+ * Every runner source must parse.
+ *
+ * This exists because of a real bug: a JSDoc line reading "the no*​/else branches" closed
+ * its own block comment, turning prose into code. loader.mjs stopped parsing, and nothing
+ * local caught it -- the pure tests do not import that file, and the tests that do cannot
+ * run here because pi needs a newer node than this machine has. It failed in CI instead.
+ *
+ * `node --check` needs no dependencies and no particular node version, so unlike the loader
+ * assertions this runs EVERYWHERE. Cheap, total, and it would have caught it in a second.
+ */
+const runnerRoot = fileURLToPath(new URL("..", import.meta.url));
+
+function collect(dir, found = []) {
+	for (const entry of readdirSync(dir)) {
+		if (entry === "node_modules") continue;
+		const path = join(dir, entry);
+		if (statSync(path).isDirectory()) collect(path, found);
+		else if (entry.endsWith(".mjs")) found.push(path);
+	}
+	return found;
+}
+
+const sources = collect(runnerRoot);
+
+test("there are sources to check", () => {
+	assert.ok(sources.length >= 4, `expected runner sources, found ${sources.length}`);
+});
+
+for (const source of sources) {
+	test(`parses: ${source.slice(runnerRoot.length).replace(/\\/g, "/")}`, () => {
+		// --check parses without executing or resolving imports, so it works with zero deps.
+		execFileSync(process.execPath, ["--check", source], { stdio: "pipe" });
+	});
+}