fix what the review found: signals, identity, the profile the wrapper nearly used

Ten findings from a review of the first commit. The ones that bite in normal use:

An unchecked mktemp left EPHEMERAL empty, which becomes `--user-data-dir=` — and Chrome reads that as
"use the default profile", i.e. the browser the user actually signs into. `set -u` does not catch it,
because the variable is set, just empty. Now checked and refused. That branch also never exec'd and
had no way to reach the browser with a signal; it now backgrounds, records the pid, and traps.

Signal handlers registered AFTER a build that takes minutes, so a pm2 restart during a first start
killed this process and left cargo compiling as an orphan — then started a second one on the same
target directory. Handlers now register first and shutdown kills the build.

`which('iris')` treated presence as identity. A truncated binary from an interrupted install is
present, reports ready, and never rebuilds. It now runs `--version` and checks — the same lesson
os-deps.ts already learned as `identify`.

Cookies were copied without Cookies-wal. These are WAL databases, so the newest session may exist
only in the log — copy the .db alone and the capture is signed out for exactly the account that just
signed in. PLUGIN.md had this backwards, citing WAL as the reason it was safe; corrected there too.

Also: --password-store=basic was missing from the branch that actually runs, working only because
chromiumoxide's DEFAULT_ARGS happens to supply it; seeding failures were silent, so the skill's
"sign in again" advice was offered for an unreadable profile it cannot fix; only --user-data-dir=X was
detected and not the space-separated form; installWrapper was unguarded at top level and crash-looped
on an unwritable ~/.local/bin; and it truncated the wrapper in place rather than renaming over it.

The searchPath copy is gone — the platform now exports locateBinary and this asks it. That copy is
what made the manifest probe refuse a rustup-only machine the sidecar could build on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

sidecar/index.ts

@@ -1,6 +1,7 @@
-import { chmodSync, copyFileSync, existsSync, mkdirSync } from 'node:fs';
+import { chmodSync, copyFileSync, existsSync, mkdirSync, renameSync } from 'node:fs';
 import { homedir } from 'node:os';
 import { join } from 'node:path';
+import { locateBinary } from '@@/plugins/os-deps';
 
 // The iris sidecar: a builder wearing a supervisor's clothes.
 //
@@ -21,20 +22,30 @@ const MSRV: readonly [number, number] = [1, 88];
 const home = homedir();
 const localBin = join(home, '.local', 'bin');
 
-// `Bun.which` alone reads the PATH this process started with, which under PM2 is minimal. Look where
-// things actually are: our install target first, rustup's home second, then whatever PATH says.
-const searchPath = (): string => `${localBin}:${join(home, '.cargo', 'bin')}:${process.env.PATH ?? ''}`;
-const which = (binary: string): string | null => Bun.which(binary, { PATH: searchPath() });
+// The platform's own resolver, not a copy of it. It asks the OWNER's login shell where a binary is,
+// which is the only source that knows about rustup's `~/.cargo/bin`, or anything else a per-user
+// installer added — this file used to guess that list and disagreed with the manifest probe, so a
+// rustup-only machine was refused the install and then built fine.
+const which = (binary: string): string | null => locateBinary(binary);
 
 const log = (message: string): void => console.log(`[${name}] ${message}`);
 
-/** The wrapper ships in this repository; agents need it on a path they can name. Overwrite = upgrade. */
+/**
+ * The wrapper ships in this repository; agents need it on a path they can name. Overwrite = upgrade.
+ *
+ * Written to a temporary name and RENAMED over the target, because `copyFileSync` truncates in place: a
+ * restart during a capture would have the running shell read a half-written script, which fails in
+ * whatever way the truncation happens to produce. Rename is atomic within a filesystem, so a reader
+ * holds either the old file or the new one. Same reasoning as `installRelease` in os-deps.ts.
+ */
 function installWrapper(): void {
   const source = join(import.meta.dir, '..', 'bin', 'iris-browser');
   const target = join(localBin, 'iris-browser');
+  const staging = `${target}.new`;
   mkdirSync(localBin, { recursive: true });
-  copyFileSync(source, target);
-  chmodSync(target, 0o755);
+  copyFileSync(source, staging);
+  chmodSync(staging, 0o755);
+  renameSync(staging, target);
   log(`iris-browser installed to ${target}`);
 }
 
@@ -59,13 +70,20 @@ const meetsMsrv = ([major, minor]: [number, number]): boolean =>
  * `--locked` builds against upstream's committed lockfile rather than re-resolving dependencies at
  * install time — the platform's frozen-lockfile rule, honoured one language over.
  */
+let building: Bun.Subprocess | null = null;
+
 async function build(cargo: string): Promise<boolean> {
   log(`building ${CRATE} — first start only, takes a few minutes`);
   const proc = Bun.spawn([cargo, 'install', CRATE, '--locked', '--root', join(home, '.local')], {
     stdout: 'inherit',
     stderr: 'inherit',
   });
+  // Remembered so `shutdown` can kill it. A cargo build spawns rustc processes that keep going long after
+  // this process exits — `pm2 restart` during a build would otherwise leave the old compile running and
+  // start a second one on top of it, both writing the same target directory.
+  building = proc;
   const code = await proc.exited;
+  building = null;
   if (code !== 0) {
     log(`build failed (cargo exited ${code}) — read the output above; restart this process to retry`);
     return false;
@@ -73,13 +91,37 @@ async function build(cargo: string): Promise<boolean> {
   return true;
 }
 
+/**
+ * Is the binary named `iris` actually ours?
+ *
+ * Presence is not identity. `iris` is a common enough name for a program, and a half-written file from a
+ * cargo install that was killed mid-copy is also present — both make `which` succeed and both make this
+ * sidecar report ready and never rebuild. The platform learned this once already: `os-deps.ts` grew
+ * `identify` for exactly it, with the note that "a namesake on PATH is not the dependency".
+ *
+ * Cheap, and only on start.
+ */
+function isOurIris(path: string): boolean {
+  try {
+    const out = Bun.spawnSync([path, '--version'], { stdout: 'pipe', stderr: 'pipe', timeout: 5_000 });
+    const text = `${new TextDecoder().decode(out.stdout)}${new TextDecoder().decode(out.stderr)}`;
+    return text.toLowerCase().includes('iris');
+  } catch {
+    // Will not execute at all — a truncated or wrong-architecture file. No more useful than a missing one.
+    return false;
+  }
+}
+
 /** True when the binary is usable. Says why not, with the fix, when it is not. */
 async function ensureIris(): Promise<boolean> {
   const present = which('iris');
-  if (present) {
+  if (present && isOurIris(present)) {
     log(`iris ready at ${present}`);
     return true;
   }
+  if (present) {
+    log(`${present} does not identify as iris — rebuilding over it`);
+  }
 
   const cargo = which('cargo');
   if (!cargo) {
@@ -108,14 +150,44 @@ async function ensureIris(): Promise<boolean> {
 }
 
 log(`started (pid ${process.pid})`);
-installWrapper();
+
+// ── Signals FIRST ──
+//
+// `ensureIris()` below can take minutes, and until these are registered a SIGTERM is handled by Bun's
+// default, which kills this process and leaves the cargo build running as an orphan. `pm2 restart` during
+// a first start is not an unusual thing to do — it is what somebody does when they think it is stuck.
+let beat: ReturnType<typeof setInterval> | null = null;
+
+const shutdown = (signal: string): void => {
+  log(`${signal} — exiting`);
+  if (beat) clearInterval(beat);
+  if (building) {
+    log('killing the in-progress cargo build');
+    building.kill();
+  }
+  process.exit(0);
+};
+
+process.on('SIGTERM', () => shutdown('SIGTERM'));
+process.on('SIGINT', () => shutdown('SIGINT'));
+
+// Guarded, in a file whose every other failure logs and continues. An unwritable `~/.local/bin` — wrong
+// ownership after an install run as the wrong user is the way this happens — threw here at top level and
+// crash-looped the sidecar under PM2, which reports "errored" and says nothing about a directory.
+try {
+  installWrapper();
+} catch (err) {
+  log(`could not install iris-browser: ${err instanceof Error ? err.message : err}`);
+  log(`check that ${localBin} exists and is writable; captures will fail until it is`);
+}
+
 let ready = await ensureIris();
 
 // The heartbeat re-probes rather than repeating a remembered answer: a binary deleted out from under us
 // shows up here, and a rustup installed after a refusal turns the next restart into a build — which the
 // log has already told the owner. Building inside the interval is deliberately avoided: a periodic
 // compile that nobody asked for is not a heartbeat.
-const beat = setInterval(() => {
+beat = setInterval(() => {
   const present = which('iris');
   if (present && !ready) {
     ready = true;
@@ -128,11 +200,4 @@ const beat = setInterval(() => {
   }
 }, 300_000);
 
-const shutdown = (signal: string): void => {
-  log(`${signal} — exiting`);
-  clearInterval(beat);
-  process.exit(0);
-};
 
-process.on('SIGTERM', () => shutdown('SIGTERM'));
-process.on('SIGINT', () => shutdown('SIGINT'));