diff --git a/PLUGIN.md b/PLUGIN.md index 954624f..a7ed45e 100644 --- a/PLUGIN.md +++ b/PLUGIN.md @@ -57,7 +57,12 @@ This contract predates the desktop plugin. When that plugin lands, this path is 3. **Members.** The wrapper isolates per-user by `$HOME`, but the binary lands in the *owner's* `~/.local/bin`, and a confined member's agent cannot read it there. If members should capture, the binary needs a shared home. Deferred until the desktop plugin defines the member story. -4. **Cookie copy under a live browser.** Copying a SQLite file mid-write can catch a torn state; in - practice the cookie DB is WAL and short-lived reads are fine, but "in practice" is not verified. +4. **Cookie copy under a live browser.** Copying a SQLite file mid-write can catch a torn state. Being + WAL does not make this safe — it is what makes the copy INCOMPLETE, because a recent write may live + only in `Cookies-wal` until the browser checkpoints it. This originally read "the cookie DB is WAL and + short-lived reads are fine", which had the reasoning backwards: the wrapper copied `Cookies` alone, so + a session created moments earlier was the one guaranteed to be missing. `-wal` is now copied alongside + each database (`-shm` deliberately is not — it is live shared memory, rebuilt on open). A torn read of + the pair is still possible under a live browser and is still unverified. 5. **`permissions: []`** is honest today (nothing platform-gated to protect) but means the owner cannot dock-gate or role-gate the plugin's existence. Revisit if an api/ or web/ arrives. diff --git a/bin/iris-browser b/bin/iris-browser index d660df0..8c81015 100755 --- a/bin/iris-browser +++ b/bin/iris-browser @@ -36,34 +36,85 @@ fi # Cookies plus the key that decrypts them. Login Data rides along so "remembered" form logins survive. # Everything else in a profile is cache and preference noise a capture does not need. +# +# ── The -wal files are not optional ── +# +# These are SQLite databases in WAL mode, and a recent write may live ONLY in the write-ahead log: the +# browser has not checkpointed it into the .db yet. Copying `Cookies` without `Cookies-wal` therefore +# produces a profile that is missing exactly the newest sessions — sign in, capture immediately, and the +# capture is signed out, which reads as "sessions do not work" rather than as a missing file. +# +# `-shm` is deliberately NOT copied: it is shared memory for live coordination between processes, it is +# rebuilt from the WAL on open, and a stale one is worse than none. +# +# Failures are REPORTED, not swallowed. The skill tells the user to sign in again when a capture comes +# back logged out; that advice is useless if the real cause was an unreadable profile, and silence is +# what made the two indistinguishable. +seed_one() { + [ -f "$1" ] || return 0 + cp "$1" "$2" 2>/dev/null || echo "iris-browser: could not copy $(basename "$1") — captures may be signed out" >&2 +} + seed_sessions() { dest="$1" - [ -d "$PROFILE/Default" ] || return 0 - mkdir -p "$dest/Default" - cp "$PROFILE/Local State" "$dest/Local State" 2>/dev/null || true - cp "$PROFILE/Default/Cookies" "$dest/Default/Cookies" 2>/dev/null || true - cp "$PROFILE/Default/Login Data" "$dest/Default/Login Data" 2>/dev/null || true + [ -d "$PROFILE/Default" ] || { + echo "iris-browser: no profile at $PROFILE — captures will be signed out until you sign in there" >&2 + return 0 + } + mkdir -p "$dest/Default" || return 0 + seed_one "$PROFILE/Local State" "$dest/Local State" + for db in "Cookies" "Login Data"; do + seed_one "$PROFILE/Default/$db" "$dest/Default/$db" + seed_one "$PROFILE/Default/$db-wal" "$dest/Default/$db-wal" + done } # Did the launcher already choose a profile directory? +# +# Both spellings. Chrome accepts `--user-data-dir=X` and `--user-data-dir X`, and matching only the first +# would send us down the ephemeral path while the launcher used its own directory — two profiles, and the +# sessions seeded into the one nobody opened. CALLER_DIR="" +take_next=0 for arg in "$@"; do + if [ "$take_next" -eq 1 ]; then + CALLER_DIR="$arg" + take_next=0 + continue + fi case "$arg" in --user-data-dir=*) CALLER_DIR="${arg#--user-data-dir=}" ;; + --user-data-dir) take_next=1 ;; esac done +# --password-store=basic keeps cookie encryption portable between the profile and the copy: without it a +# keyring-backed browser writes cookies that the copied `Local State` key cannot decrypt, and the capture +# is signed out for a reason nothing reports. Passed on BOTH paths — chromiumoxide's own DEFAULT_ARGS +# happen to include it today, which is not something this script should depend on. if [ -n "$CALLER_DIR" ]; then seed_sessions "$CALLER_DIR" - exec "$BROWSER" "$@" + exec "$BROWSER" "$@" --password-store=basic fi -EPHEMERAL="$(mktemp -d "${TMPDIR:-/tmp}/iris-profile.XXXXXX")" +# CHECKED. `set -u` does not catch this: mktemp failing leaves EPHEMERAL an empty string, which becomes +# `--user-data-dir=` — and Chrome reads an empty value as "use the default profile", i.e. the user's real +# one. A capture would then run in, and write to, the browser they actually use. +if ! EPHEMERAL="$(mktemp -d "${TMPDIR:-/tmp}/iris-profile.XXXXXX")" || [ -z "$EPHEMERAL" ]; then + echo "iris-browser: could not create a temporary profile directory — refusing to launch" >&2 + exit 1 +fi trap 'rm -rf "$EPHEMERAL"' EXIT INT TERM seed_sessions "$EPHEMERAL" -# --password-store=basic keeps cookie encryption portable between the profile and the copy — without it a -# keyring-backed browser writes cookies the copy cannot decrypt. Appended AFTER the launcher's args, and -# so is our --user-data-dir, because for a repeated switch Chrome honours the last occurrence. -"$BROWSER" "$@" --user-data-dir="$EPHEMERAL" --password-store=basic +# NOT exec'd, unlike the branch above, because the trap has to outlive the browser to remove the profile — +# and a profile holding copies of the user's cookies is not something to leave in /tmp. The cost is that +# this shell stays alive as the parent; `wait` is what makes a signal to it reach the browser rather than +# killing the wrapper and orphaning the child. +# +# Our --user-data-dir goes AFTER the launcher's args: for a repeated switch Chrome honours the last one. +"$BROWSER" "$@" --user-data-dir="$EPHEMERAL" --password-store=basic & +BROWSER_PID=$! +trap 'kill "$BROWSER_PID" 2>/dev/null; rm -rf "$EPHEMERAL"' EXIT INT TERM +wait "$BROWSER_PID" exit $? diff --git a/sidecar/index.ts b/sidecar/index.ts index fa4c5b2..ac8d4ef 100644 --- a/sidecar/index.ts +++ b/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 { 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 { 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 { 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 { } 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 | 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'));