fix(smoke): validate remote mktemp -d before using it as an rm -rf target

package.json

@@ -28,6 +28,7 @@
     "bin",
     "dist",
     "scripts/apply-cloud-migrations.mjs",
+    "scripts/lib/remote-temp-dir.mjs",
     "scripts/smoke-machine-sync-release.mjs",
     "scripts/smoke-machines-adapter.mjs",
     "scripts/smoke-open-files-installed-boundary.mjs",

scripts/lib/remote-temp-dir.mjs

@@ -0,0 +1,156 @@
+import { posix } from 'node:path';
+
+/**
+ * Validate a path that will later be handed to `rm -rf` on a remote host.
+ *
+ * What this actually defends against - stated precisely, because an earlier version of this
+ * comment described an unreachable code path:
+ *
+ * `runRemote` goes through `runChecked`, which throws on a non-zero remote exit. A *failing*
+ * `mktemp -d` exits 1, so it aborts the run and never yields an empty string. The reachable
+ * hole is the remote exiting **zero** with stdout that is not the path: an ssh `Banner`, a
+ * chatty login profile, or a `ForceCommand` wrapper prepends lines, and a wrapper can mask
+ * the real exit status entirely. Whatever lands in that variable is then passed as a
+ * `--peer-workspace` argv element and, more importantly, interpolated into the remote shell
+ * strings `cd <value> && knowledge ...` and `rm -rf <value>`.
+ *
+ * That is the same class as the 2026-07-24 incident - a delete target taken on faith from a
+ * command's output - which is why the check belongs at the assignment rather than relying on
+ * `shellQuote` downstream to neutralise a value that should never have been accepted.
+ *
+ * Accepts only what this exact template could have produced: `mktemp` replaces the trailing
+ * run of `X` in place, so the result is the template's fixed prefix followed by a single
+ * non-empty path component. Checking against the template's parent directory alone is not
+ * enough - it would accept any unrelated directory sitting beside the temp dir - so a
+ * template whose final component is nothing but `X` (`/tmp/XXXXXX`) is refused outright:
+ * its prefix is a bare directory, which would silently degrade this into exactly that
+ * parent-only check. The bound is derived from the template, so it stays correct if the
+ * template moves.
+ *
+ * @param {string} remote Remote host, used in the error message only.
+ * @param {unknown} dir Trimmed stdout of the remote `mktemp -d`.
+ * @param {string} template Absolute template passed to `mktemp -d`, ending in `XXX...`.
+ * @returns {string} The validated directory.
+ * @throws {Error} If the value is anything other than a directory that template could create.
+ */
+export function assertRemoteTempDir(remote, dir, template) {
+  const fail = (why) => {
+    throw new Error(
+      `Refusing to use remote temp dir from ${remote}: ${why}. `
+      + `Template ${template}, got ${JSON.stringify(dir)}. `
+      + 'This value is used as an `rm -rf` target on the remote host.'
+    );
+  };
+
+  if (typeof template !== 'string' || !template.startsWith('/')) {
+    throw new Error(`Remote temp dir template must be an absolute path, got ${JSON.stringify(template)}.`);
+  }
+  // A template containing `.` or `..` makes the derived bound meaningless, and the template
+  // is built from a caller-supplied version string. Require it to be already normalized
+  // rather than normalizing silently, so a surprising template is loud instead of accepted.
+  if (posix.normalize(template) !== template) {
+    throw new Error(
+      `Remote temp dir template must be normalized (no "." or ".." segments), got ${JSON.stringify(template)}.`
+    );
+  }
+  // POSIX mktemp requires at least three trailing X in the final component.
+  const match = template.match(/^(.*[^X])(X{3,})$/);
+  if (!match) {
+    throw new Error(`Remote temp dir template must end in at least three X, got ${JSON.stringify(template)}.`);
+  }
+  const [, prefix, xRun] = match;
+  // Without a fixed part in the final component, `prefix` is a bare directory and every
+  // sibling under it would validate - the parent-only check this function exists to avoid.
+  if (prefix.endsWith('/')) {
+    throw new Error(
+      'Remote temp dir template must have a fixed prefix in its final component '
+      + `(e.g. /tmp/knowledge-XXXXXX, not /tmp/XXXXXX), got ${JSON.stringify(template)}.`
+    );
+  }
+
+  if (typeof dir !== 'string' || dir === '') fail('mktemp -d produced no path');
+  if (/[\r\n]/.test(dir)) fail('path spans multiple lines');
+  // Redundant while the template is absolute and normalized (prefix then starts with `/`),
+  // kept as a cheap invariant so a future change to the template rules cannot quietly admit
+  // a relative delete target.
+  if (!dir.startsWith('/')) fail('path is not absolute');
+  if (!dir.startsWith(prefix)) fail(`path does not start with the template prefix ${prefix}`);
+
+  const suffix = dir.slice(prefix.length);
+  if (suffix === '') fail('path is the bare template prefix, so mktemp created nothing');
+  if (suffix.includes('/')) fail('path descends below the directory the template would create');
+  // mktemp replaces the X run with exactly that many characters from [A-Za-z0-9]. Requiring
+  // the same shape is what makes "only what this template could have produced" literally
+  // true, rather than "anything under the prefix without a slash in it".
+  if (suffix.length !== xRun.length) {
+    fail(`path suffix is ${suffix.length} characters, but the template's X run is ${xRun.length}`);
+  }
+  if (!/^[A-Za-z0-9]+$/.test(suffix)) fail('path suffix contains characters mktemp never generates');
+
+  return dir;
+}
+
+/**
+ * Create a remote temp dir, returning only a path this exact template could have produced.
+ *
+ * `makeTempDir` is injected rather than imported so this module never spawns a process - the
+ * published script stays the only thing that talks to ssh - and, more to the point, so a test
+ * can drive THIS function, the one that decides whether the validator runs at all. Exercising
+ * `assertRemoteTempDir` on its own proves the validator works but pins nothing about it being
+ * wired in: deleting the call below used to leave the entire suite green.
+ *
+ * @param {string} remote Remote host.
+ * @param {string} template Absolute template passed to `mktemp -d`, ending in `XXX...`.
+ * @param {(remote: string, template: string) => string} makeTempDir Runs `mktemp -d <template>`
+ *   on `remote` and returns its stdout. Expected to throw on a non-zero remote exit.
+ * @returns {string} The validated directory.
+ * @throws {Error} If the value is anything other than a directory that template could create.
+ */
+export function createRemoteTempDir(remote, template, makeTempDir) {
+  const raw = makeTempDir(remote, template);
+  // A non-string reaches the guard as-is rather than dying on `.trim()`, so a wrapper returning
+  // something unexpected produces the explanatory refusal instead of a bare TypeError.
+  const dir = typeof raw === 'string' ? raw.trim() : raw;
+  return assertRemoteTempDir(remote, dir, template);
+}
+
+/**
+ * Delete a remote temp dir, but only after re-checking that the value is still something
+ * `template` could have produced, and without ever throwing.
+ *
+ * That re-check is the last gate in front of a recursive delete on another machine, which is
+ * why it lives here rather than inline in the caller's `finally`: the property worth pinning is
+ * a negative - `deleteDir` must NOT be called when the value does not validate - and a negative
+ * is unobservable while the gate sits in a cleanup block no test can drive.
+ *
+ * Callers use this from `finally`, where a throw would replace the in-flight exception and skip
+ * the remaining cleanup, so every step is contained and problems are handed to `report`.
+ *
+ * @param {string} remote Remote host.
+ * @param {string|null|undefined} dir Candidate delete target; nullish means there is nothing to
+ *   clean up, which is normal when creation itself failed.
+ * @param {string} template The template `dir` was created from.
+ * @param {{
+ *   deleteDir: (dir: string) => void,
+ *   report: (what: string, error: unknown) => void,
+ * }} io `deleteDir` performs the remote delete; `report` records a cleanup problem and must not
+ *   throw.
+ * @returns {boolean} Whether the delete was attempted.
+ */
+export function removeRemoteTempDir(remote, dir, template, { deleteDir, report }) {
+  if (dir === null || dir === undefined) return false;
+
+  try {
+    assertRemoteTempDir(remote, dir, template);
+  } catch (error) {
+    report('refusing remote cleanup, temp dir no longer validates', error);
+    return false;
+  }
+
+  try {
+    deleteDir(dir);
+  } catch (error) {
+    report(`remote cleanup of ${dir} could not be spawned`, error);
+  }
+  return true;
+}

scripts/smoke-machine-sync-release.mjs

@@ -1,10 +1,11 @@
 #!/usr/bin/env bun
 
 import { spawnSync } from 'node:child_process';
-import { chmodSync, cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
+import { chmodSync, cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { dirname, join, relative, resolve } from 'node:path';
 import { fileURLToPath } from 'node:url';
+import { createRemoteTempDir, removeRemoteTempDir } from './lib/remote-temp-dir.mjs';
 
 const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..');
 const packageJson = JSON.parse(readFileSync(join(repoRoot, 'package.json'), 'utf8'));
@@ -122,6 +123,55 @@ function runRemote(remote, command, options = {}) {
   return runChecked('ssh', [remote, command], options);
 }
 
+/**
+ * Report a cleanup problem without ever throwing.
+ *
+ * Used only from `finally` blocks, where a throw REPLACES the in-flight exception from the
+ * try block and skips the remaining cleanup, so the reporter itself is contained rather than
+ * only the statements it reports on.
+ *
+ * The try/catch covers a synchronous throw (a non-string argument, a closed fd). It does NOT
+ * cover EPIPE: measured on Linux, writing to a stderr pipe whose reader has exited raises an
+ * asynchronous 'error' event that terminates the process, and no catch here can intercept
+ * that. An earlier version of this comment claimed otherwise; the residual risk of the
+ * process dying mid-cleanup is real and is not addressed by this function.
+ */
+function reportCleanupProblem(what, error) {
+  try {
+    process.stderr.write(`[smoke] ${what}: ${error instanceof Error ? error.message : String(error)}\n`);
+  } catch {
+    // Nothing can be done about a failed diagnostic, and it must not mask the real error.
+  }
+}
+
+/** Remove a directory from a `finally` block without ever throwing. */
+function removeTempDirSafely(dir, label) {
+  if (dir === null || dir === undefined) return;
+  try {
+    rmSync(dir, { recursive: true, force: true });
+  } catch (error) {
+    reportCleanupProblem(`could not remove ${label} ${dir}`, error);
+  }
+}
+
+/** `mktemp -d <template>` on the remote host; throws on a non-zero remote exit. */
+function remoteMktemp(remote, template) {
+  return runRemote(remote, `mktemp -d ${shellQuote(template)}`);
+}
+
+/** `rm -rf <dir>` on the remote host, reporting a non-zero exit rather than discarding it. */
+function remoteRemoveDir(remote, dir) {
+  // A failed remote delete leaves the temp dir behind; `run` swallows the exit status, so it is
+  // surfaced here rather than discarded.
+  const cleanup = run('ssh', [remote, `rm -rf ${shellQuote(dir)}`]);
+  if (cleanup.status !== 0) {
+    reportCleanupProblem(
+      `remote cleanup of ${dir} on ${remote} exited ${cleanup.status}; it may still exist`,
+      cleanup.stderr.trim() || 'no stderr'
+    );
+  }
+}
+
 function parseJsonOutput(label, raw) {
   try {
     return JSON.parse(raw);
@@ -410,11 +460,18 @@ function syncMachineArgs(options, remoteDir, runOptions = {}) {
 }
 
 function runSyncSmoke(options, runOptions = {}) {
-  const localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`));
-  const remoteDir = runRemote(options.remote, `mktemp -d ${shellQuote(`/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`)}`).trim();
+  // Both temp dirs are created INSIDE the try, with the finally null-guarding each one, so
+  // whichever creation succeeds is always cleaned up. Creating either one outside only moves
+  // the leak around: remote-first leaks a remote dir when mkdtempSync fails, local-first
+  // leaks a local dir when the remote dir is refused.
+  const remoteTemplate = `/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`;
+  let remoteDir = null;
+  let localDir = null;
   const localCommandOptions = runOptions.localCommandOptions ?? {};
   const learnCommandOptions = runOptions.learnCommandOptions ?? {};
   try {
+    remoteDir = createRemoteTempDir(options.remote, remoteTemplate, remoteMktemp);
+    localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`));
     knowledgeJson(localDir, ['db', 'init', '--scope', 'project', '--json'], localCommandOptions);
     remoteKnowledgeJson(options.remote, remoteDir, ['db', 'init', '--scope', 'project', '--json']);
 
@@ -575,8 +632,19 @@ function runSyncSmoke(options, runOptions = {}) {
     return summary;
   } finally {
     if (!options.keepTemp) {
-      rmSync(localDir, { recursive: true, force: true });
-      run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]);
+      // EVERY statement in this finally is individually contained, including `report` itself.
+      // A throw here would replace the in-flight exception from the try block - turning a real
+      // smoke failure into a confusing cleanup error - and would skip the remaining cleanup.
+
+      removeTempDirSafely(localDir, 'local temp dir');
+      // Null-guarded, re-validated and contained inside the helper, which is what makes the
+      // re-check reachable by a test. remoteDir is assigned once from an already-validated
+      // value, so the re-check cannot currently fail; it exists so a future refactor that makes
+      // the value reassignable is caught there rather than at the `rm -rf`.
+      removeRemoteTempDir(options.remote, remoteDir, remoteTemplate, {
+        deleteDir: (dir) => remoteRemoveDir(options.remote, dir),
+        report: reportCleanupProblem,
+      });
     }
   }
 }
@@ -589,7 +657,7 @@ function runNoMachinesSyncSmoke(options, dirs) {
     try {
       probe = knowledgeJson(probeDir, ['machines', 'topology', '--no-tailscale', '--json'], { env: runner.env });
     } finally {
-      if (!options.keepTemp) rmSync(probeDir, { recursive: true, force: true });
+      if (!options.keepTemp) removeTempDirSafely(probeDir, 'probe dir');
     }
     if (probe.adapter?.implementation !== 'disabled' || probe.adapter?.available !== false) {
       throw new Error(`no-machines probe did not disable adapter: ${JSON.stringify(probe.adapter)}`);
@@ -619,7 +687,7 @@ function runNoMachinesSyncSmoke(options, dirs) {
       sync,
     };
   } finally {
-    if (!options.keepTemp) rmSync(runner.app_dir, { recursive: true, force: true });
+    if (!options.keepTemp) removeTempDirSafely(runner.app_dir, 'runner app dir');
   }
 }
 
@@ -631,7 +699,7 @@ function runNoMachinesRegistrySyncSmoke(options, dirs) {
     try {
       probe = knowledgeJson(probeDir, ['machines', 'topology', '--no-tailscale', '--json'], { env: runner.env });
     } finally {
-      if (!options.keepTemp) rmSync(probeDir, { recursive: true, force: true });
+      if (!options.keepTemp) removeTempDirSafely(probeDir, 'probe dir');
     }
     if (probe.adapter?.implementation !== 'disabled' || probe.adapter?.available !== false) {
       throw new Error(`no-machines registry probe did not disable adapter: ${JSON.stringify(probe.adapter)}`);
@@ -674,7 +742,7 @@ function runNoMachinesRegistrySyncSmoke(options, dirs) {
       sync,
     };
   } finally {
-    if (!options.keepTemp) rmSync(runner.app_dir, { recursive: true, force: true });
+    if (!options.keepTemp) removeTempDirSafely(runner.app_dir, 'runner app dir');
   }
 }
 
@@ -793,4 +861,49 @@ function main() {
   outputSummary(summary, options);
 }
 
-main();
+/**
+ * Same file on disk, following symlinks on both sides.
+ *
+ * A lexical `resolve()` comparison is NOT enough, and getting this wrong fails in the silent
+ * direction. Node resolves the main module through symlinks before deriving
+ * `import.meta.url`, so under any symlinked layout - a pnpm-style
+ * `node_modules/@hasna/knowledge -> <store>/package`, or macOS `/tmp -> /private/tmp` in the
+ * documented `npm pack && tar xzf` evidence procedure - argv[1] is the link path while
+ * `import.meta.url` is the real path. They never compare equal, the script does nothing, and
+ * it exits 0: a caller checking only the exit status records a green release smoke that never
+ * ran. Bun keeps the link path on both sides, which is why a bun-only test cannot see it.
+ *
+ * When a path cannot be resolved at all - a virtual filesystem, a file deleted out from under a
+ * running process - this falls back to the lexical comparison rather than returning false. The
+ * defect being fixed is a script that silently does nothing, so the fallback is the pre-existing
+ * behaviour rather than a new way to skip the run, and it can never make `import` execute
+ * `main()`: on import argv[1] is a different file under either comparison.
+ */
+function isSameFile(a, b) {
+  try {
+    return realpathSync(a) === realpathSync(b);
+  } catch {
+    return a === b;
+  }
+}
+
+/**
+ * Run only when invoked directly, never on import.
+ *
+ * `main()` at module scope means merely importing this file performs `bun install -g` locally
+ * AND `ssh <remote> bun install -g` on a fleet host, before any argument is inspected. That is
+ * not hypothetical: it fired during review of this very PR, when the file was imported to test
+ * import resolution, and it installed a package on the reviewer's machine and attempted a
+ * remote install. A published script must not mutate a machine as a side effect of being
+ * loaded. Compared against argv rather than `import.meta.main`, which is a recent addition and
+ * is simply `undefined` on older Node releases - reading it there would turn this script into
+ * exactly the silent no-op described above. (An earlier version of this comment said Node had
+ * no support until v24; measured, v22.22.3 defines it as a boolean. The reason to avoid it is
+ * older runtimes, not v24.)
+ */
+const invokedDirectly = process.argv[1] !== undefined
+  && isSameFile(resolve(process.argv[1]), fileURLToPath(import.meta.url));
+
+if (invokedDirectly) {
+  main();
+}

scripts/validate-public-package.mjs

@@ -20,6 +20,7 @@ const publicDocs = [
 
 const publicScripts = [
   'scripts/apply-cloud-migrations.mjs',
+  'scripts/lib/remote-temp-dir.mjs',
   'scripts/smoke-machine-sync-release.mjs',
   'scripts/smoke-machines-adapter.mjs',
   'scripts/smoke-open-files-installed-boundary.mjs',