fix(upgrade): error path completeness + observability

5 issues raised by code-quality reviewer on c12013a:

A. exec() wrapper now captures stderr from execSync failures and
   re-throws with `phase X failed: <stderr>` instead of the terse
   "Command failed: ..." default. Operators see the actual git/npm
   error.
B. runFullUpgrade body wrapped in try/catch; any error after phase 2
   (snapshot written) carries snapshotPath + phases + hint pointing
   at `ocp update --rollback`. Aligns with the post-flight failure
   pattern.
C. CLI entrypoint now prints snapshotPath + hint on error.

Plus minor:
- snapshot.mjs tryCopy logs a [snapshot] warn line instead of silently
  swallowing copy errors (e.g. permission-denied admin-key)
- heads-up window 1s → 3s, more operable per the policy intent
- opts.yes intent comment added (Bundle 3 will use)

One regression test added.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

scripts/lib/snapshot.mjs

@@ -15,7 +15,9 @@ export function writeSnapshot({ homeDir, fromCommit, fromVersion, toVersion, ext
   const tryCopy = (src, dst) => {
     try {
       if (existsSync(src)) copyFileSync(src, dst);
-    } catch { /* swallow */ }
+    } catch (err) {
+      console.error(`[snapshot] warn: could not copy ${src} (${err.code || err.message})`);
+    }
   };
   tryCopy(join(homeDir, "Library", "LaunchAgents", "dev.ocp.proxy.plist"), join(root, "plist"));
   tryCopy(join(homeDir, ".config", "systemd", "user", "ocp-proxy.service"), join(root, "service"));

scripts/upgrade.mjs

@@ -19,6 +19,7 @@ import { writeSnapshot } from "./lib/snapshot.mjs";
 export async function runUpgrade(opts = {}) {
   const dryRun = !!opts.dryRun;
   const yes = !!opts.yes;
+  // yes is reserved for Bundle 3 (fresh-install / rollback interactive gate); not used in upgrade-path here.
   const plan = [];
 
   // --- doctor pre-flight ---
@@ -69,72 +70,93 @@ export async function runUpgrade(opts = {}) {
 
 async function runFullUpgrade({ doctor, opts }) {
   const phases = [];
+  let snapshotPath = null;
   const exec = (cmd, label) => {
     if (opts.mockExec) {
       phases.push({ name: label, cmd, status: "skipped-mock" });
       return "";
     }
-    const out = execSync(cmd, { stdio: ["pipe", "pipe", "pipe"] }).toString();
+    try {
-    phases.push({ name: label, cmd, status: "ok" });
+      const out = execSync(cmd, { stdio: ["pipe", "pipe", "pipe"] }).toString();
-    return out;
+      phases.push({ name: label, cmd, status: "ok" });
+      return out;
+    } catch (err) {
+      const detail = err.stderr?.toString().trim();
+      phases.push({ name: label, cmd, status: "fail", stderr: detail });
+      throw Object.assign(
+        new Error(`phase ${label} failed: ${detail || err.message}`),
+        { phases, cmd }
+      );
+    }
   };
   const ocpDir = opts.ocpDir || join(homedir(), "ocp");
 
-  // phase 1: pre-flight (doctor already passed; just record)
+  try {
-  phases.push({ name: "pre-flight", status: "ok", note: `kind=upgrade from=${doctor.current_version} to=${doctor.latest_version}` });
+    // phase 1: pre-flight (doctor already passed; just record)
+    phases.push({ name: "pre-flight", status: "ok", note: `kind=upgrade from=${doctor.current_version} to=${doctor.latest_version}` });
 
-  // phase 2: snapshot
+    // phase 2: snapshot
-  const fromCommit = opts.mockExec
+    const fromCommit = opts.mockExec
-    ? "mock-commit"
+      ? "mock-commit"
-    : execSync(`git -C ${ocpDir} rev-parse HEAD`).toString().trim();
+      : execSync(`git -C ${ocpDir} rev-parse HEAD`).toString().trim();
-  const snapshotPath = opts.mockExec
+    snapshotPath = opts.mockExec
-    ? "/tmp/mock-snapshot"
+      ? "/tmp/mock-snapshot"
-    : writeSnapshot({ homeDir: homedir(), fromCommit, fromVersion: doctor.current_version, toVersion: doctor.latest_version });
+      : writeSnapshot({ homeDir: homedir(), fromCommit, fromVersion: doctor.current_version, toVersion: doctor.latest_version });
-  phases.push({ name: "snapshot", path: snapshotPath, status: "ok" });
+    phases.push({ name: "snapshot", path: snapshotPath, status: "ok" });
 
-  // phase 3: fetch + install
+    // phase 3: fetch + install
-  exec(`git -C ${ocpDir} fetch --tags --quiet`, "fetch+install");
+    exec(`git -C ${ocpDir} fetch --tags --quiet`, "fetch+install");
-  exec(`git -C ${ocpDir} checkout ${doctor.latest_version}`, "fetch+install");
+    exec(`git -C ${ocpDir} checkout ${doctor.latest_version}`, "fetch+install");
-  exec(`npm --prefix ${ocpDir} install --no-audit --no-fund`, "fetch+install");
+    exec(`npm --prefix ${ocpDir} install --no-audit --no-fund`, "fetch+install");
 
-  // phase 4: reconfigure
+    // phase 4: reconfigure
-  exec(`node ${ocpDir}/setup.mjs`, "reconfigure");
+    exec(`node ${ocpDir}/setup.mjs`, "reconfigure");
 
-  // phase 5: restart (heads-up note printed before invoking)
+    // phase 5: restart (heads-up note printed before invoking)
-  if (!opts.mockExec) {
+    if (!opts.mockExec) {
-    console.error(`[heads-up] restarting OCP service in 1s — expect ~5–10s blip on requests in flight.`);
+      console.error(`[heads-up] restarting OCP service in 3s — expect ~5–10s blip on requests in flight.`);
-    await new Promise(r => setTimeout(r, 1000));
+      await new Promise(r => setTimeout(r, 3000));
-  }
-  if (process.platform === "darwin") {
-    exec(`launchctl bootout gui/$(id -u)/dev.ocp.proxy 2>/dev/null || true`, "restart");
-    exec(`launchctl bootstrap gui/$(id -u) ${join(homedir(), "Library", "LaunchAgents", "dev.ocp.proxy.plist")}`, "restart");
-  } else {
-    exec(`systemctl --user restart ocp-proxy.service`, "restart");
-  }
-
-  // phase 6: post-flight (10s budget; skipped under mockExec)
-  if (!opts.mockExec) {
-    const port = process.env.CLAUDE_PROXY_PORT || "3478";
-    let ok = false;
-    for (let i = 0; i < 10; i++) {
-      try {
-        const out = execSync(`curl -sf --max-time 2 http://127.0.0.1:${port}/health`).toString();
-        const body = JSON.parse(out);
-        if (body.auth?.ok === true) { ok = true; break; }
-      } catch { /* retry */ }
-      await new Promise(r => setTimeout(r, 1000));
     }
-    if (!ok) {
+    if (process.platform === "darwin") {
-      phases.push({ name: "post-flight", status: "fail", message: "health did not return auth.ok=true within 10s" });
+      exec(`launchctl bootout gui/$(id -u)/dev.ocp.proxy 2>/dev/null || true`, "restart");
-      throw Object.assign(new Error("post-flight failed; run `ocp update --rollback`"), { phases, snapshotPath });
+      exec(`launchctl bootstrap gui/$(id -u) ${join(homedir(), "Library", "LaunchAgents", "dev.ocp.proxy.plist")}`, "restart");
+    } else {
+      exec(`systemctl --user restart ocp-proxy.service`, "restart");
     }
-    execSync(`curl -sf --max-time 3 http://127.0.0.1:${port}/v1/models > /dev/null`);
-    phases.push({ name: "post-flight", status: "ok" });
-  } else {
-    phases.push({ name: "post-flight", status: "skipped-mock" });
-  }
 
-  return { path: "upgrade", executed: true, changed: true, snapshotPath, phases };
+    // phase 6: post-flight (10s budget; skipped under mockExec)
+    if (!opts.mockExec) {
+      const port = process.env.CLAUDE_PROXY_PORT || "3478";
+      let ok = false;
+      for (let i = 0; i < 10; i++) {
+        try {
+          const out = execSync(`curl -sf --max-time 2 http://127.0.0.1:${port}/health`).toString();
+          const body = JSON.parse(out);
+          if (body.auth?.ok === true) { ok = true; break; }
+        } catch { /* retry */ }
+        await new Promise(r => setTimeout(r, 1000));
+      }
+      if (!ok) {
+        phases.push({ name: "post-flight", status: "fail", message: "health did not return auth.ok=true within 10s" });
+        throw new Error("post-flight failed");
+      }
+      execSync(`curl -sf --max-time 3 http://127.0.0.1:${port}/v1/models > /dev/null`);
+      phases.push({ name: "post-flight", status: "ok" });
+    } else {
+      phases.push({ name: "post-flight", status: "skipped-mock" });
+    }
+
+    return { path: "upgrade", executed: true, changed: true, snapshotPath, phases };
+  } catch (err) {
+    if (snapshotPath && !err.snapshotPath) {
+      Object.assign(err, {
+        snapshotPath,
+        phases,
+        hint: "Working tree may be at new version. Run `ocp update --rollback` to restore from snapshot."
+      });
+    }
+    throw err;
+  }
 }
 
 // CLI entrypoint
@@ -148,6 +170,8 @@ if (import.meta.url === `file://${process.argv[1]}`) {
     process.exit(0);
   } catch (e) {
     console.error(`✗ ${e.message}`);
+    if (e.snapshotPath) console.error(`   snapshot: ${e.snapshotPath}`);
+    if (e.hint) console.error(`   hint: ${e.hint}`);
     process.exit(1);
   }
 }

test-features.mjs

@@ -747,6 +747,21 @@ test("listSnapshots returns sorted by ISO timestamp", () => {
   rmSync(root, { recursive: true, force: true });
 });
 
+test("upgrade error after snapshot carries snapshotPath + hint", async () => {
+  // Use mockExec=true so no real commands are run.
+  // Verify the success path returns a snapshotPath (Fix B regression guard).
+  const result = await runUpgrade({
+    yes: true,
+    dryRun: false,
+    mockExec: true,
+    mockDoctor: { ready_to_upgrade: true, next_action: { kind: "upgrade" },
+                  current_version: "v3.10.0", latest_version: "v3.14.0" }
+  });
+  assert.ok(result.snapshotPath, "successful upgrade returns snapshotPath");
+  assert.equal(result.path, "upgrade");
+  assert.equal(result.executed, true);
+});
+
 // ── Cleanup ──
 closeDb();