Release: snapshot stale-JOB_DATA + env-scrub fix (ucho MNG-1622/1702)

src/router/worker-snapshots.ts

@@ -13,6 +13,156 @@ import { registerSnapshot } from './snapshot-manager.js';
 
 const docker = new Docker();
 
+/**
+ * Env-var keys that must NEVER be baked into a committed snapshot image.
+ *
+ * `docker commit` preserves the container's `Config.Env` (every `-e` from the
+ * spawn) into the new image. Two problems if left unscrubbed:
+ *
+ *  1. **Correctness (ucho/MNG-1622 + MNG-1702).** A run that passes its payload
+ *     INLINE bakes `JOB_DATA=<json>` into the snapshot. A later run for the same
+ *     work item whose payload is large is OFFLOADED (only `JOB_DATA_REDIS_KEY`
+ *     is set, not `JOB_DATA`), and `docker run -e JOB_DATA_REDIS_KEY=...` does
+ *     not clear the baked `JOB_DATA`. The worker then read the stale baked
+ *     payload and ran the wrong (prior) agent. The primary fix is worker-side
+ *     (`resolveRawJobData` prefers the Redis key); stripping job env here removes
+ *     the stale artifact at the source too.
+ *  2. **Security.** The spawn env carries `DATABASE_URL`, `REDIS_URL`, the
+ *     project's GitHub/Linear/OpenAI/etc. credentials, and the Claude OAuth
+ *     token. Baking them means anyone with Docker/registry access to a
+ *     `cascade-snapshot-*` image can read every project secret via
+ *     `docker image inspect`.
+ *
+ * Static deny-set covers job + infra-secret keys; per-project credential names
+ * are dynamic and enumerated at runtime from `CASCADE_CREDENTIAL_KEYS`.
+ */
+const SNAPSHOT_ENV_DENYLIST: ReadonlySet<string> = new Set([
+	'JOB_DATA',
+	'JOB_DATA_REDIS_KEY',
+	'JOB_ID',
+	'JOB_TYPE',
+	'DATABASE_URL',
+	'DATABASE_SSL',
+	'DATABASE_CA_CERT',
+	'REDIS_URL',
+	'CREDENTIAL_MASTER_KEY',
+	'CASCADE_CREDENTIAL_KEYS',
+	'CLAUDE_CODE_OAUTH_TOKEN',
+	'CASCADE_POSTGRES_HOST',
+	'CASCADE_POSTGRES_PORT',
+	'CASCADE_SNAPSHOT_REUSE',
+	'CASCADE_SNAPSHOT_ENABLED',
+]);
+
+/** Parse the `KEY` out of a `KEY=VALUE` env line (split on the FIRST `=` only). */
+function envKey(line: string): string {
+	const eq = line.indexOf('=');
+	return eq === -1 ? line : line.slice(0, eq);
+}
+
+/**
+ * Filter a container's `Config.Env` down to what is safe to bake into a snapshot
+ * image: drop the static deny-set plus every dynamic project-credential name
+ * listed in `CASCADE_CREDENTIAL_KEYS`. Everything else (PATH, NODE_*, LOG_LEVEL,
+ * SENTRY_*, PLAYWRIGHT_BROWSERS_PATH, CASCADE_DASHBOARD_URL, …) is PRESERVED so
+ * the snapshot still boots. Pure and total; splits on the first `=` so JSON /
+ * connection-string values containing `=` are handled.
+ */
+export function scrubSnapshotEnv(env: string[], extraCredentialKeys: string[] = []): string[] {
+	const deny = new Set<string>(SNAPSHOT_ENV_DENYLIST);
+	for (const k of extraCredentialKeys) {
+		const trimmed = k.trim();
+		if (trimmed) deny.add(trimmed);
+	}
+	return env.filter((line) => !deny.has(envKey(line)));
+}
+
+/** Extract the dynamic project-credential key names from a container's env. */
+function extractCredentialKeys(env: string[]): string[] {
+	const line = env.find((e) => e.startsWith('CASCADE_CREDENTIAL_KEYS='));
+	if (!line) return [];
+	return line
+		.slice('CASCADE_CREDENTIAL_KEYS='.length)
+		.split(',')
+		.map((s) => s.trim())
+		.filter(Boolean);
+}
+
+/**
+ * Compute the `changes` (Dockerfile `ENV KEY=` instructions) that BLANK the
+ * value of every deny-listed / credential env var actually present in `env`.
+ *
+ * WHY blank-via-changes and not a scrubbed `Env` body: `docker commit` cannot
+ * REMOVE an env var. The `POST /commit` body's `Env` list does not replace the
+ * container's env — moby re-appends every container env var whose key is absent
+ * from the body, so a "scrubbed Env body" is a **silent no-op** (verified
+ * against a live daemon: `JOB_DATA` / secrets survive unchanged, byte-identical
+ * to a bare commit). The supported mechanism is the `changes` param — Dockerfile
+ * instructions applied to the committed image — where `ENV KEY=` sets the value
+ * to empty. Empty `JOB_DATA` is falsy (the worker ignores it), and an emptied
+ * secret carries no value to leak. `Cmd` / `Entrypoint` / `WorkingDir` and every
+ * other env var are preserved by the daemon (a bare commit keeps the full
+ * container config; `changes` only overlays the named ENV keys). Only keys
+ * PRESENT in the env are blanked, so no spurious empty vars are introduced.
+ */
+export function buildSnapshotEnvScrubChanges(env: string[]): string[] {
+	const deny = new Set<string>(SNAPSHOT_ENV_DENYLIST);
+	for (const k of extractCredentialKeys(env)) deny.add(k);
+	const present = new Set<string>();
+	for (const line of env) {
+		const key = envKey(line);
+		if (deny.has(key)) present.add(key);
+	}
+	return [...present].map((key) => `ENV ${key}=`);
+}
+
+/**
+ * Commit `container` to `imageName` with its job + secret env vars blanked.
+ *
+ * Inspects the container's live `Config.Env`, then commits with `changes` that
+ * empty the value of every deny-listed / credential key present (see
+ * `buildSnapshotEnvScrubChanges` for why `changes` and not an `Env` body — the
+ * latter is a proven no-op). `Cmd`/`Entrypoint`/`WorkingDir`/all other env are
+ * preserved, so a reused snapshot still boots.
+ *
+ * If inspect fails or the env is empty, falls back to a bare commit (an
+ * unscrubbed but working snapshot) and captures Sentry under
+ * `snapshot_env_scrub_inspect_failed` so the regression to baking secrets is
+ * loud rather than silent.
+ */
+async function commitScrubbed(
+	container: Docker.Container,
+	repo: string,
+	imageName: string,
+): Promise<void> {
+	let env: string[] | undefined;
+	try {
+		const info = (await container.inspect()) as { Config?: { Env?: string[] } } | undefined;
+		env = info?.Config?.Env;
+	} catch (inspectErr) {
+		captureException(inspectErr, {
+			tags: { source: 'snapshot_env_scrub_inspect_failed' },
+			extra: { imageName },
+			level: 'warning',
+		});
+	}
+
+	if (Array.isArray(env) && env.length > 0) {
+		const changes = buildSnapshotEnvScrubChanges(env);
+		if (changes.length > 0) {
+			await container.commit({ repo, tag: 'latest', changes });
+			logger.info('[WorkerManager] Snapshot committed with blanked job/secret env', {
+				imageName,
+				blankedKeys: changes.length,
+			});
+			return;
+		}
+	}
+
+	// inspect unavailable / empty env / nothing to blank → bare config-preserving commit.
+	await container.commit({ repo, tag: 'latest' });
+}
+
 /**
  * Build a stable Docker image name for a snapshot.
  * Uses a sanitised project+workItem key so it's valid as a Docker image tag.
@@ -56,7 +206,7 @@ export async function commitWorkerSnapshot(
 	const imageName = buildWorkerSnapshotImageName(projectId, workItemId);
 	try {
 		const container = docker.getContainer(containerId);
-		await container.commit({ repo: imageName.split(':')[0], tag: 'latest' });
+		await commitScrubbed(container, imageName.split(':')[0], imageName);
 		const imageSize = await inspectImageSizeBestEffort(imageName);
 		registerSnapshot(projectId, workItemId, imageName, imageSize);
 		logger.info('[WorkerManager] Committed container to snapshot image:', {

src/worker-entry.ts

@@ -29,7 +29,7 @@ import {
 	extractTrelloContext,
 	generateAckMessage,
 } from './router/ackMessageGenerator.js';
-import { readOffloadedJobData } from './router/job-data-offload.js';
+import { buildJobDataRedisKey, readOffloadedJobData } from './router/job-data-offload.js';
 import { dispatchPMAck } from './router/pm-ack-dispatch.js';
 import { captureException, flush, setTag } from './sentry.js';
 import {
@@ -460,29 +460,56 @@ export async function dispatchJob(
  * with "argument list too long". Must run before scrubSensitiveEnv() strips
  * REDIS_URL. Exits the process with a clear, grep-able reason on any failure —
  * never the cryptic exec crash, never a payload-less worker.
+ *
+ * The Redis key is authoritative ONLY when it names THIS job's payload. The
+ * router sets EXACTLY ONE of JOB_DATA / JOB_DATA_REDIS_KEY per spawn
+ * (worker-env.ts is an if/else), and JOB_ID is set fresh on every spawn while the
+ * offload key embeds that jobId (`buildJobDataRedisKey`). But `docker commit`
+ * bakes the committed container's ENV into the snapshot image, and
+ * `docker run -e ...` does NOT clear a baked key that this run doesn't re-set. A
+ * reused snapshot can therefore carry a stale co-present channel from a PRIOR run
+ * of the same work item. Disambiguate both directions by matching the key to
+ * JOB_ID:
+ *
+ *  - Forward case — this run is OFFLOADED (fresh JOB_DATA_REDIS_KEY) but the
+ *    snapshot baked a stale `JOB_DATA=<json>` from a prior INLINE run. The key
+ *    matches JOB_ID → read Redis, ignoring the stale inline value. Reading inline
+ *    first silently ran the wrong (prior) agent (prod incident ucho/MNG-1622 +
+ *    MNG-1702: a 'splitting' run reused a 'planning' snapshot and re-ran planning,
+ *    producing no story cards).
+ *  - Reverse case — this run is INLINE (fresh JOB_DATA) but the snapshot baked a
+ *    stale `JOB_DATA_REDIS_KEY=<priorJobId>` from a prior OFFLOADED run. The key
+ *    does NOT match JOB_ID → it is a stale baked artifact whose key the prior run
+ *    already deleted from Redis; reading it would throw and crash the worker.
+ *    Ignore it and fall through to this run's fresh inline JOB_DATA.
  */
 async function resolveRawJobData(): Promise<string> {
-	const inline = process.env.JOB_DATA;
-	if (inline) return inline;
-
 	const key = process.env.JOB_DATA_REDIS_KEY;
-	if (!key) {
-		// Defensive: main() validates that JOB_DATA or JOB_DATA_REDIS_KEY is present.
-		const err = new Error('JOB_DATA could not be resolved from env or Redis');
-		console.error(`[Worker] ${err.message}`);
-		captureException(err, { tags: { source: 'worker_env' } });
-		await flush();
-		process.exit(1);
+	const jobId = process.env.JOB_ID;
+	// Trust the key only when it names THIS job's payload; a mismatched key is a
+	// stale artifact baked into a reused snapshot (see fn doc, reverse case).
+	if (key && jobId && key === buildJobDataRedisKey(jobId)) {
+		try {
+			return await readOffloadedJobData(key);
+		} catch (err) {
+			console.error('[Worker] Failed to read offloaded JOB_DATA from Redis:', err);
+			captureException(err, { tags: { source: 'worker_job_data_redis_read' } });
+			await flush();
+			process.exit(1);
+		}
 	}
 
-	try {
-		return await readOffloadedJobData(key);
-	} catch (err) {
-		console.error('[Worker] Failed to read offloaded JOB_DATA from Redis:', err);
-		captureException(err, { tags: { source: 'worker_job_data_redis_read' } });
-		await flush();
-		process.exit(1);
-	}
+	const inline = process.env.JOB_DATA;
+	if (inline) return inline;
+
+	// Defensive: main() validates that JOB_DATA or JOB_DATA_REDIS_KEY is present.
+	// Reaching here means the only channel set is a stale baked key (no matching
+	// JOB_ID) with no fresh inline fallback — the router set neither for this run.
+	const err = new Error('JOB_DATA could not be resolved from env or Redis');
+	console.error(`[Worker] ${err.message}`);
+	captureException(err, { tags: { source: 'worker_env' } });
+	await flush();
+	process.exit(1);
 }
 
 export async function main(): Promise<void> {