fix(upgrade): detect failed extraction and stop leaking temp directories

.claude/agent-memory/archgate-developer/MEMORY.md

@@ -25,13 +25,15 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code
 - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout <file>`, which discards all uncommitted work in that file, including the fix under test.
 - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not.
 - **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point.
-- **Confirm a fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. Build such fixtures from `String.fromCharCode` and print the file back (`cat -A`) first.
+- **Confirm a probe or fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. A quoted `<<'EOF'` heredoc through the Bash tool is not reliably literal either: a backslash-heavy regex probe run that way reported no match for a pattern that matches fine. Write any escape-sensitive probe to a real file with the Write tool instead of inlining it in a shell command, build such fixtures from `String.fromCodePoint` (oxlint's `unicorn/prefer-code-point` rejects `fromCharCode`), and print the file back (`cat -A`) first.
 - **A Sentry issue whose `install_path` is a local worktree is an artifact of that worktree, not a user report.** Confirm provenance from `install_path`, `is_ci`, and the timestamp — then still ask whether the error was classified correctly, because reaching Sentry at all claims archgate has a bug rather than the caller.
 - **`--update-snapshots` is never on its own the fix for a failing snapshot.** `tests/helpers/__snapshots__/rules-shim.test.ts.snap` is the entire `rules.d.ts` a governed project receives, and its diff is the review artifact — read every hunk and confirm it follows from an intended `src/formats/rules.ts` edit before regenerating. Deleting the file routes around nothing: Bun fails a missing snapshot whenever `CI` is set, and passes it locally.
 - **`actionlint` silently skips its shellcheck-backed checks when `shellcheck` is not on PATH.** A Windows dev machine typically has none, so a local run exits 0 while CI (ubuntu-latest) fails on a `run:` block — SC2086 on an unquoted variable is the common one. A Git Bash `PATH` entry must use the Unix-style form (`/c/Users/...`), or the lookup silently fails.
 - **Splitting a test file for `oxlint`'s 500-line `max-lines` cap: add a sibling `<name>-<suffix>.test.ts`, don't trim coverage.** Precedent: `check-max-warnings.test.ts` beside `check.test.ts`; followed again for `reporter-strict.test.ts`, `sync-strict.test.ts`, and the `*-strict.test.ts` integration files.
 - **`typescript/no-unnecessary-condition` does not flag a string-literal union compared against a literal outside it.** With `typeAware: true`, a narrowed `"a" | "b"` tested against `""` passes clean while a `string !== undefined` control in the same function is flagged at once. Narrowing a type does not hand the dead comparison to the linter, so a clean lint run is not evidence either way.
 - **Sizing a change's blast radius by grep: search each token separately, don't require them on one line.** A pattern requiring `"check"` and `"--json"` on the same line missed `tests/commands/check.test.ts`, where they sit on adjacent lines — surfacing only when `bun run validate` failed after a "complete" migration.
+- **A `test.skipIf(process.platform === "win32")` test passes vacuously on this machine — run it under WSL before believing it.** A skipped test reports as passing, so an assertion that never executes reads exactly like a verified one. Bun installs in WSL Ubuntu via `curl -fsSL https://bun.sh/install | bash`, then `wsl.exe -d Ubuntu -- bash -lc 'cd /mnt/e/... && ~/.bun/bin/bun test <files>'` runs the Linux-only cases against the Windows checkout. Fire-test there too — a skipped fire-test proves nothing.
+- **Run WSL against every test file a change touches, not just the one being written.** Compare the Windows and Linux `skip` counts: a change to a helper's signature broke three `tests/commands/upgrade-action.test.ts` cases that Windows skips, and checking only the helper's own test file missed them until CI. Two caveats: `/mnt/e` fails ~120 subprocess-integration tests (`review-context`, `stream-guards`, `session-context` spawn the CLI) that pass on real CI Linux, so a full-suite WSL run cannot be read as a gate — target the affected files and judge failures by whether they touch your change; and coverage from `/mnt/e` runs is not comparable to CI's.
 - **Genuine OS-level EPIPE cannot be arranged from bun:test on Windows** — a spawned child's `stdout.cancel()` leaves the child's pipe open, and Git-Bash `cmd | true` pipelines may never break the pipe even unguarded (so a passing fire-test there is inconclusive, not proof). Synthesize `process.stdout.emit("error", err)` with `code: "EPIPE"` instead, as in `tests/integration/stream-guards.test.ts`. A real break IS reproducible locally with a sustained writer piped to `head -c 100`.
 - **`archgate review-context`'s `--base` diffs against the local `main`/`origin/main` ref, which can be stale-but-tree-identical after a squash merge** — same content, different hash, inflating `allChangedFiles` with the last merged PR's files. Fix with `git fetch origin` + explicit `--base origin/main` (the only option in a worktree, where `fetch origin main:main` is refused because `main` is checked out in the primary tree). Only when the current branch IS the stale `main` and `git diff origin/main HEAD --stat` is empty may `git reset origin/main` (never `--hard`) realign it.
 - **`docs/public/llms-full.txt` is auto-regenerated by the `update-llms.yaml` PR workflow whenever `docs/src/content/docs/**` changes** — a bot commit lands on the branch shortly after pushing docs edits. Never hand-edit it; `git pull` before continuing, and when CodeRabbit flags stale wording inside it, fix the source `.mdx`.

.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md

@@ -20,7 +20,11 @@ Four failure modes worth testing for by hand:
 
 **A review agent's verdict on non-English prose is worthless**, and it invents the detail that supports it: an orthography pass over the pt-br docs returned PASS while asserting accents the words do not contain (`depreciadas` "(á)", `governança` "(ã)"). Grep cannot settle a claim about meaning, so verify mechanically instead — does the stripped form still occur, did fenced code blocks change — and leave the language judgement to a human speaker.
 
-**Reproduce a described failure before scheduling work from it**, including when your own scan reports zero. Of three issues one audit derived from memory files, two collapsed to nothing once the failure was actually tested (#517 Go proxy, #518 branch protection); the third was real but larger than described (#516).
+**Reproduce a described failure before scheduling work from it**, including when your own scan reports zero. Of three issues one audit derived from memory files, two collapsed to nothing once the failure was actually tested (#517 Go proxy, #518 branch protection); the third was real but larger than described (#516). A second trio filed from coverage work held the same ratio: #554 was accurate, #555's central claim was false (the branch it called dead is load-bearing), and #556 understated its own bug (the leak it pinned to failure paths happens on success too).
+
+An issue filed by a prior session is unverified in the current checkout, and its own claim of having been verified counts for nothing: #555 opened with "verified empirically against real GNU tar output rather than inferred from reading" and was still wrong about what that output does. Re-run the check yourself.
+
+Test the premise separately from the defect. A report can name a real defect while its stated mechanism is wrong, and a fix built on the stated mechanism removes working code — #555 asked for the deletion of a guard branch that is load-bearing.
 
 And prove a zero is a real zero: `\b` inside a JS template literal is a backspace, not a word boundary, so a regex built that way found no corruption where 69 occurrences sat.
 

src/commands/upgrade.ts

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright 2026 Archgate
-import { existsSync } from "node:fs";
+import { existsSync, rmSync } from "node:fs";
 import { dirname, join } from "node:path";
 import { clearLine, cursorTo } from "node:readline";
 
@@ -278,17 +278,28 @@ async function upgradeBinary(tag: string): Promise<void> {
 
   logDebug("Artifact:", artifact.name, "ext:", artifact.ext);
   const hint = getManualInstallHint();
+  // downloadReleaseBinary creates an extraction directory it cannot remove
+  // itself, so removing it once the binary is installed is the caller's job.
+  let extractDir: string | undefined;
   try {
-    const onProgress = createDownloadProgress();
-    const newBinaryPath = await downloadReleaseBinary(
-      tag,
-      artifact,
-      onProgress
-    );
-    finishDownloadProgress();
-    logDebug("Downloaded binary to:", newBinaryPath);
-    logDebug("Replacing binary:", process.execPath);
-    replaceBinary(process.execPath, newBinaryPath);
+    try {
+      const onProgress = createDownloadProgress();
+      const { binaryPath, tmpDir } = await downloadReleaseBinary(
+        tag,
+        artifact,
+        onProgress
+      );
+      extractDir = tmpDir;
+      finishDownloadProgress();
+      logDebug("Downloaded binary to:", binaryPath);
+      logDebug("Replacing binary:", process.execPath);
+      replaceBinary(process.execPath, binaryPath);
+    } finally {
+      // Runs before the handler below, which ends the process via exitWith().
+      if (extractDir !== undefined) {
+        rmSync(extractDir, { recursive: true, force: true });
+      }
+    }
   } catch (err) {
     if (err instanceof Error && err.name === "ExitPromptError") throw err;
     finishDownloadProgress();

src/helpers/binary-upgrade.ts

@@ -1,7 +1,14 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright 2026 Archgate
 import { createHash } from "node:crypto";
-import { chmodSync, mkdtempSync, renameSync, unlinkSync } from "node:fs";
+import {
+  chmodSync,
+  lstatSync,
+  mkdtempSync,
+  renameSync,
+  rmSync,
+  unlinkSync,
+} from "node:fs";
 import { unlink } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -114,19 +121,49 @@ export type DownloadProgressCallback = (progress: DownloadProgress) => void;
 // Download and extract
 // ---------------------------------------------------------------------------
 
+/**
+ * Run `cmd`, draining both pipes concurrently with the exit code.
+ *
+ * @see ARCH-007 — awaiting one pipe alone leaves the child blocked once it
+ * fills the other, so `proc.exited` never resolves.
+ */
+async function runCapture(
+  cmd: string[]
+): Promise<{ exitCode: number; stdout: string; stderr: string }> {
+  const proc = Bun.spawn(cmd, { stdout: "pipe", stderr: "pipe" });
+  const [stdout, stderr, exitCode] = await Promise.all([
+    new Response(proc.stdout).text(),
+    new Response(proc.stderr).text(),
+    proc.exited,
+  ]);
+  return { exitCode, stdout, stderr };
+}
+
+export interface DownloadedBinary {
+  /** Path to the extracted binary, inside {@link DownloadedBinary.tmpDir}. */
+  binaryPath: string;
+  /**
+   * Extraction directory this call created. The caller removes it once the
+   * binary is installed; deriving it from `binaryPath` instead would delete
+   * whichever directory that path happens to sit in.
+   */
+  tmpDir: string;
+}
+
 /**
  * Download and extract the release binary to a temp directory.
- * Returns the path to the extracted binary.
  *
  * When an `onProgress` callback is provided the response body is streamed
  * so the caller can display incremental progress.  Without the callback the
  * response is buffered in one shot.
+ *
+ * @returns The extracted binary and the directory the caller must remove.
  */
 export async function downloadReleaseBinary(
   tag: string,
   artifact: ArtifactInfo,
   onProgress?: DownloadProgressCallback
-): Promise<string> {
+): Promise<DownloadedBinary> {
   const baseUrl = `https://github.com/${GITHUB_REPO}/releases/download/${tag}`;
   const archiveUrl = `${baseUrl}/${artifact.name}${artifact.ext}`;
   const checksumUrl = `${baseUrl}/${artifact.name}${artifact.ext}.sha256`;
@@ -218,62 +255,92 @@ export async function downloadReleaseBinary(
     logDebug("Checksum verification skipped:", err);
   }
   const tmpDir = mkdtempSync(join(tmpdir(), "archgate-upgrade-"));
-  const archivePath = join(tmpDir, `archgate${artifact.ext}`);
-  logDebug("Extracting archive to:", tmpDir);
+  try {
+    const archivePath = join(tmpDir, `archgate${artifact.ext}`);
+    logDebug("Extracting archive to:", tmpDir);
+
+    await Bun.write(archivePath, buffer);
+
+    if (artifact.ext === ".tar.gz") {
+      // Validate archive entries before extraction to prevent path traversal.
+      // Backslashes are normalized because a member stored as `..\evil` is
+      // listed escaped by GNU tar and literal by bsdtar; both forms reach the
+      // `../` check only after normalization.
+      const list = await runCapture(["tar", "-tzf", archivePath]);
+      if (list.exitCode !== 0) {
+        // An empty listing from a failed run would otherwise read as "no
+        // unsafe entries" and wave the archive through the guard below.
+        throw new UserError(
+          `Failed to read archive listing (tar exit code ${list.exitCode})`
+        );
+      }
 
-  await Bun.write(archivePath, buffer);
+      for (const entry of list.stdout.split("\n").filter(Boolean)) {
+        const normalized = entry.replaceAll("\\", "/").trim();
+        if (
+          normalized.startsWith("/") ||
+          normalized.includes("../") ||
+          normalized === ".."
+        ) {
+          throw new Error(
+            `Unsafe path in release archive: "${entry}" — aborting extraction`
+          );
+        }
+      }
 
-  if (artifact.ext === ".tar.gz") {
-    // Validate archive entries before extraction to prevent path traversal
-    const listProc = Bun.spawn(["tar", "-tzf", archivePath], {
-      stdout: "pipe",
-      stderr: "pipe",
-    });
-    const listing = await new Response(listProc.stdout).text();
-    await listProc.exited;
-
-    for (const entry of listing.split("\n").filter(Boolean)) {
-      const normalized = entry.replaceAll("\\", "/").trim();
-      if (
-        normalized.startsWith("/") ||
-        normalized.includes("../") ||
-        normalized === ".."
-      ) {
-        throw new Error(
-          `Unsafe path in release archive: "${entry}" — aborting extraction`
+      const { exitCode } = await runCapture([
+        "tar",
+        "-xzf",
+        archivePath,
+        "-C",
+        tmpDir,
+      ]);
+      if (exitCode !== 0) {
+        throw new UserError(
+          `Failed to extract archive (tar exit code ${exitCode})`
+        );
+      }
+    } else {
+      // `-ErrorAction Stop` promotes Expand-Archive's non-terminating error to
+      // a terminating one; without it PowerShell exits 0 on a corrupt archive
+      // and extraction failure goes unnoticed.
+      const { exitCode } = await runCapture([
+        "powershell",
+        "-NoProfile",
+        "-Command",
+        `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force -ErrorAction Stop`,
+      ]);
+      if (exitCode !== 0) {
+        throw new UserError(
+          `Failed to extract archive (PowerShell exit code ${exitCode})`
         );
       }
     }
 
-    const proc = Bun.spawn(["tar", "-xzf", archivePath, "-C", tmpDir], {
-      stdout: "pipe",
-      stderr: "pipe",
-    });
-    const exitCode = await proc.exited;
-    if (exitCode !== 0) {
+    const binaryPath = join(tmpDir, artifact.binaryName);
+    // lstat rather than existsSync: the latter follows symlinks and accepts
+    // directories, so an archive member that is either would be installed as
+    // the binary. replaceBinary renames without following, so a symlink would
+    // land in ~/.archgate/bin/ pointing wherever the archive chose.
+    const stats = lstatSync(binaryPath, { throwIfNoEntry: false });
+    if (!stats) {
       throw new UserError(
-        `Failed to extract archive (tar exit code ${exitCode})`
+        `Extraction produced no ${artifact.binaryName} — the downloaded archive is corrupt or incomplete`
       );
     }
-  } else {
-    const proc = Bun.spawn(
-      [
-        "powershell",
-        "-NoProfile",
-        "-Command",
-        `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force`,
-      ],
-      { stdout: "pipe", stderr: "pipe" }
-    );
-    const exitCode = await proc.exited;
-    if (exitCode !== 0) {
+    if (!stats.isFile()) {
       throw new UserError(
-        `Failed to extract archive (PowerShell exit code ${exitCode})`
+        `Extraction produced ${artifact.binaryName} as a ${stats.isSymbolicLink() ? "symbolic link" : "non-regular file"} — refusing to install it`
       );
     }
-  }
 
-  return join(tmpDir, artifact.binaryName);
+    return { binaryPath, tmpDir };
+  } catch (err) {
+    // The caller learns of tmpDir only on success, so it can only clean up
+    // then; every failure has to remove it here.
+    rmSync(tmpDir, { recursive: true, force: true });
+    throw err;
+  }
 }
 
 // ---------------------------------------------------------------------------