fix(extensions): set aside an unusable ledger row instead of failing the file

src/main/extensions/install.ts

@@ -22,7 +22,7 @@ import { computeBundleHash } from '@main/extensions/bundleHash.js'
 import { BUNDLE_MAX_BYTES, BUNDLE_MAX_DEPTH, BUNDLE_MAX_ENTRIES } from './bundleLimits.js'
 import { githubApiHeaders, resolveGitHubCliToken } from './githubCli.js'
 import { ManifestError, parseExtensionManifest } from '@main/extensions/manifest.js'
-import { discardExtensionBundle, extensionBundleDirectory, readLedger, withLedgerLock, writeLedger } from '@main/extensions/ledger.js'
+import { discardExtensionBundle, extensionBundleDirectory, preservedBundleExtensionIds, readLedger, readLedgerContents, withLedgerLock, writeLedger } from '@main/extensions/ledger.js'
 import type { ExtensionManifest, InstalledExtension } from '@shared/types/extensions.js'
 
 /**
@@ -713,7 +713,19 @@ export async function sweepAbandonedInstallDirectories(): Promise<void> {
         await removeAbandoned(path)
       }
     }
-    const referenced = new Set((await readLedger()).map(extensionBundleDirectory))
+    // Preserved rows count as referenced (#959). A row this build cannot read
+    // still names a bundle on disk, and the build that CAN read it will need
+    // that bundle. Reclaiming it would turn "your extension is set aside until
+    // you roll forward" into "your extension is gone" — and the sweep runs at
+    // startup, so a single rollback-and-relaunch would do it.
+    //
+    // Their directories are computed from the RAW row rather than a validated
+    // one, so this deliberately protects both possible layouts for an id it
+    // cannot fully trust; a preserved row's bundle is only ever protected from
+    // deletion here, never served or executed.
+    const { rows, preserved } = await readLedgerContents()
+    const referenced = new Set(rows.map(extensionBundleDirectory))
+    const preservedIds = preservedBundleExtensionIds(preserved)
     const bundlesRoot = join(EXTENSIONS_DIR, '.bundles')
     let extensionDirs
     try { extensionDirs = await readdir(bundlesRoot, { withFileTypes: true }) } catch (error) {
@@ -723,6 +735,12 @@ export async function sweepAbandonedInstallDirectories(): Promise<void> {
     for (const extension of extensionDirs) {
       // Never follow a hand-created symlink during recursive housekeeping.
       if (!extension.isDirectory()) continue
+      // A preserved row claims this id, so none of its generations may be
+      // collected: we cannot tell WHICH one the row points at without parsing
+      // the field that failed to parse, and the build that can read the row
+      // will need it. Skipping the whole subtree is the only answer that does
+      // not depend on a guess.
+      if (preservedIds.has(extension.name)) continue
       const parent = join(bundlesRoot, extension.name)
       for (const generation of await readdir(parent, { withFileTypes: true })) {
         const path = join(parent, generation.name)

src/main/extensions/ledger.ts

@@ -6,7 +6,7 @@ import { z } from 'zod'
 
 import { EXTENSIONS_DIR, EXTENSIONS_LOCKFILE, STATE_DIR } from '@main/storage/paths.js'
 import { isValidExtensionId } from '@shared/types/extensionId.js'
-import type { ExtensionListEntry, InstalledExtension } from '@shared/types/extensions.js'
+import type { ExtensionListEntry, InstalledExtension, QuarantinedExtensionEntry } from '@shared/types/extensions.js'
 
 import { apiVersionMismatch, extensionManifestSchema } from './manifest.js'
 
@@ -51,12 +51,69 @@ const installedExtensionSchema = z.object({
   installedAt: z.number().finite(),
 })
 
-export async function readLedger(): Promise<InstalledExtension[]> {
+/**
+ * A row the current build cannot use, kept verbatim (#959).
+ *
+ * ── WHY QUARANTINE RATHER THAN THROW ──
+ * Every one of list, install, remove, the startup sweep, the scheme handler and
+ * the capability check reads the ledger first. Throwing for the whole file
+ * meant ONE unusable row disabled every installed extension — and, because
+ * `removeExtension` reads before it writes, it also rejected, taking away the
+ * only in-app way to recover. The user's escape was to hand-edit a JSON file
+ * in `~/.config`.
+ *
+ * ── WHY THE RAW VALUE IS CARRIED, NOT A REPAIRED ONE ──
+ * A row stops validating for reasons this build cannot reason about: an
+ * extension targeting a NEWER api version after a rollback, or a schema a
+ * future build tightened. We do not know what the field we would drop means, so
+ * the bytes are written back untouched and the owning build gets its row back
+ * intact. That is also why `raw` is `unknown`: it is evidence, not data.
+ *
+ * ── WHAT A PRESERVED ROW MUST STILL DO ──
+ * Its bundle counts as REFERENCED by the sweep. Anything else and housekeeping
+ * deletes the code that the row, once valid again, points at — which is exactly
+ * the "existing bundles were preserved" promise the old error message made and
+ * the throw then made impossible to keep.
+ */
+export type PreservedLedgerRow = {
+  /** The row exactly as it sits in the file. Never interpreted. */
+  raw: unknown
+  /** `manifest.id` IF it is a valid id, so Settings can name and remove it.
+   *  Null when even that is unreadable — such a row can only be reported. */
+  id: string | null
+  /** Why this build rejected it, in the user's terms. */
+  reason: string
+}
+
+export type LedgerContents = {
+  rows: InstalledExtension[]
+  preserved: PreservedLedgerRow[]
+}
+
+/** `manifest.id` off an unvalidated row, only when it is a safe id. */
+function preservedId(candidate: unknown): string | null {
+  const manifest = candidate && typeof candidate === 'object'
+    ? (candidate as { manifest?: unknown }).manifest
+    : null
+  const id = manifest && typeof manifest === 'object'
+    ? (manifest as { id?: unknown }).id
+    : null
+  return typeof id === 'string' && isValidExtensionId(id) ? id : null
+}
+
+/**
+ * The whole ledger: what this build can run, and what it is holding for later.
+ *
+ * A file that is not JSON, or not an array, still throws. There are no rows to
+ * quarantine in that case — quarantine preserves a ROW, and reading a row is
+ * the thing that failed. Rewriting such a file is how the bundles get orphaned.
+ */
+export async function readLedgerContents(): Promise<LedgerContents> {
   let raw: string
   try {
     raw = await readFile(EXTENSIONS_LOCKFILE, 'utf8')
   } catch (error) {
-    if ((error as NodeJS.ErrnoException).code === 'ENOENT') return []
+    if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { rows: [], preserved: [] }
     throw error
   }
   let parsed: unknown
@@ -67,36 +124,57 @@ export async function readLedger(): Promise<InstalledExtension[]> {
   }
   if (!Array.isArray(parsed)) throw new Error('The extension install record must be an array.')
   const rows: InstalledExtension[] = []
+  const preserved: PreservedLedgerRow[] = []
   for (const candidate of parsed) {
     const result = installedExtensionSchema.safeParse(candidate)
-    if (result.success) {
-      // The ABI gate, applied at LOAD and not only at install. The schema accepts
-      // any positive apiVersion (it is shared with the install path, which wants
-      // to report a mismatch with a better message than a field error), so a row
-      // written under a host that implemented v1 would otherwise keep loading
-      // against a host that implements v2. Version skew arrives by UPGRADING
-      // AGENT CODE, which involves no install — so an install-time-only check can
-      // never see it.
-      const mismatch = apiVersionMismatch(result.data.manifest.apiVersion)
-      if (mismatch) {
-        throw new Error(`${result.data.manifest.id}: ${mismatch}. Existing bundles were preserved.`)
-      }
-      // Migrate a pre-`origin` row by the one field that actually distinguishes the
-      // two installers: `ref`. The local installer writes the literal 'local' there
-      // (it has no git ref to record), and the GitHub installer writes a tag or a
-      // branch name. A repository whose branch is literally named `local` would be
-      // misread — and would then simply take the Update path it already took before
-      // this field existed, so the migration is never worse than not having it.
-      rows.push({
-        ...result.data,
-        origin: result.data.origin ?? (result.data.ref === 'local' ? 'local' : 'github'),
-      })
-    } else {
+    if (!result.success) {
       // Never turn a bad row into permission to destroy its on-disk generation.
-      throw new Error(`Invalid extension install record: ${result.error.issues[0]?.message ?? 'unknown'}. Existing bundles were preserved.`)
+      preserved.push({
+        raw: candidate,
+        id: preservedId(candidate),
+        reason: `Invalid extension install record: ${result.error.issues[0]?.message ?? 'unknown'}.`,
+      })
+      continue
+    }
+    // The ABI gate, applied at LOAD and not only at install. The schema accepts
+    // any positive apiVersion (it is shared with the install path, which wants
+    // to report a mismatch with a better message than a field error), so a row
+    // written under a host that implemented v1 would otherwise keep loading
+    // against a host that implements v2. Version skew arrives by UPGRADING
+    // AGENT CODE, which involves no install — so an install-time-only check can
+    // never see it.
+    //
+    // Quarantined rather than fatal (#959): a rollback past a future api
+    // version is precisely the case where the row is still CORRECT and this
+    // build is the temporary one. Setting it aside keeps the other extensions
+    // running and keeps the row for the build that can use it.
+    const mismatch = apiVersionMismatch(result.data.manifest.apiVersion)
+    if (mismatch) {
+      preserved.push({
+        raw: candidate,
+        id: result.data.manifest.id,
+        reason: `${result.data.manifest.id}: ${mismatch}.`,
+      })
+      continue
     }
+    // Migrate a pre-`origin` row by the one field that actually distinguishes the
+    // two installers: `ref`. The local installer writes the literal 'local' there
+    // (it has no git ref to record), and the GitHub installer writes a tag or a
+    // branch name. A repository whose branch is literally named `local` would be
+    // misread — and would then simply take the Update path it already took before
+    // this field existed, so the migration is never worse than not having it.
+    rows.push({
+      ...result.data,
+      origin: result.data.origin ?? (result.data.ref === 'local' ? 'local' : 'github'),
+    })
   }
-  return rows
+  return { rows, preserved }
+}
+
+/** The rows this build can actually run. Preserved rows are invisible here by
+ *  design: discovery, activation and capability checks must never see one. */
+export async function readLedger(): Promise<InstalledExtension[]> {
+  return (await readLedgerContents()).rows
 }
 
 /**
@@ -138,14 +216,42 @@ export function withLedgerLock<T>(operation: () => Promise<T>): Promise<T> {
   return next
 }
 
-export async function writeLedger(rows: InstalledExtension[]): Promise<void> {
+/**
+ * Commit `rows` as the runnable ledger, carrying every preserved row through.
+ *
+ * ── WHY THE PRESERVED ROWS ARE RE-READ HERE INSTEAD OF PASSED IN ──
+ * They are invisible to every caller: `readLedger` deliberately hides them, so
+ * `finalizeInstall` and `removeExtension` both do read → mutate → write over
+ * the runnable rows ALONE. If carrying them were the caller's job, forgetting
+ * would silently delete a row this build could not read — the one row we have
+ * the least right to destroy. Re-reading makes losing one impossible rather
+ * than merely unlikely, and every caller already holds the ledger lock, so the
+ * extra read sees exactly the file this write is about to replace.
+ *
+ * `dropPreservedId` is the single deliberate exception: uninstalling a
+ * quarantined extension. It is an id, never a row, so a caller cannot ask for
+ * anything except "the preserved row that calls itself this".
+ */
+export async function writeLedger(
+  rows: InstalledExtension[],
+  options?: { dropPreservedId?: string },
+): Promise<void> {
+  // A ledger that cannot be READ must not be overwritten: that is how bundles
+  // get orphaned. readLedgerContents throws for unparseable JSON, and letting
+  // that throw propagate is the correct outcome for every caller.
+  const preserved = (await readLedgerContents()).preserved
+    .filter(row => options?.dropPreservedId === undefined || row.id !== options.dropPreservedId)
+    .map(row => row.raw)
+
   await mkdir(STATE_DIR, { recursive: true })
   // temp+rename in the same directory, matching workspace.json: an interrupted
   // write must leave the previous ledger intact rather than a truncated file that
   // reads as "nothing installed" and orphans every bundle on disk.
   const tmp = `${EXTENSIONS_LOCKFILE}.tmp-${randomUUID()}`
   try {
-    await writeFile(tmp, `${JSON.stringify(rows, null, 2)}\n`, 'utf8')
+    // Preserved rows go LAST so the runnable ledger stays in its own order and
+    // a quarantined row cannot change the meaning of the rows before it.
+    await writeFile(tmp, `${JSON.stringify([...rows, ...preserved], null, 2)}\n`, 'utf8')
     await rename(tmp, EXTENSIONS_LOCKFILE)
     // The rename already committed. A closed window's send failure must never
     // turn this into a reported install failure or delete the published bundle.
@@ -165,6 +271,16 @@ export async function writeLedger(rows: InstalledExtension[]): Promise<void> {
  * whereas silently hiding the row would leave the user wondering where their
  * extension went.
  */
+export async function listQuarantinedExtensions(): Promise<QuarantinedExtensionEntry[]> {
+  return withLedgerLock(async () => {
+    // The manifest is deliberately NOT carried across: the row failed
+    // validation, so none of its fields may be shown as a name or a version.
+    // The id survives only because it passed `isValidExtensionId` on its own.
+    const { preserved } = await readLedgerContents()
+    return preserved.map(row => ({ id: row.id, reason: row.reason }))
+  })
+}
+
 export async function listInstalledExtensions(): Promise<ExtensionListEntry[]> {
   return withLedgerLock(async () => {
     const rows = await readLedger()
@@ -203,6 +319,17 @@ export async function removeExtension(id: string): Promise<void> {
   // failed file deletion after that point is garbage collection, not a failed
   // uninstall: reporting failure would encourage retrying an already-removed
   // extension. Saved extension state lives elsewhere and is never touched.
+  //
+  // ── THIS TOUCHES ONLY RUNNABLE ROWS (review finding 1) ──
+  // An earlier version also dropped a PRESERVED row with the same id, because
+  // it computed the two independently and let both act. A ledger can hold both
+  // — a rollback writes a preserved row beside an id this build still runs —
+  // and a single Remove then did both things in both directions: clearing the
+  // set-aside row uninstalled the working extension and deleted its bundle,
+  // and uninstalling the working extension silently destroyed the row we have
+  // the least right to destroy. Ambiguity between two different objects is not
+  // resolvable by a precedence rule, so the two are separate operations:
+  // `removeQuarantinedExtension` is the other one.
   await withLedgerLock(async () => {
     const rows = await readLedger()
     const previous = rows.find(row => row.manifest.id === id)
@@ -211,6 +338,62 @@ export async function removeExtension(id: string): Promise<void> {
   })
 }
 
+/**
+ * Drop a PRESERVED row, and nothing else.
+ *
+ * This is the in-app recovery path #959 exists for: a user can clear a row this
+ * build cannot run instead of editing JSON in `~/.config`. It never removes a
+ * runnable row and never deletes a bundle — the path would have to be built
+ * from fields that failed validation, and once the row is gone the sweep stops
+ * protecting the id and collects the directory on its own.
+ */
+export async function removeQuarantinedExtension(id: string): Promise<void> {
+  if (!isValidExtensionId(id)) throw new Error(`invalid extension id: ${id}`)
+  await withLedgerLock(async () => {
+    const { rows, preserved } = await readLedgerContents()
+    if (!preserved.some(row => row.id === id)) return
+    await writeLedger(rows, { dropPreservedId: id })
+  })
+}
+
+/**
+ * The extension ids that PRESERVED rows claim (#959).
+ *
+ * ── WHY IDS AND NOT PATHS (review finding 3) ──
+ * The first version returned exact bundle directories, built from the raw
+ * row's `installation.id`. That protected nothing in the one case the feature
+ * exists for: a future build that RESHAPES `installation` — renames it, nests
+ * it, adds a layout — writes rows this build both rejects AND cannot locate,
+ * so the sweep deleted the newer build's bundles on the first launch after a
+ * rollback. Protection that depends on parsing the very field that failed to
+ * parse is protection that evaporates exactly when it is needed.
+ *
+ * An id is different: it is validated independently by `isValidExtensionId`,
+ * and it is the ONE level of the `.bundles/<id>/<generation>` layout the sweep
+ * itself walks. Protecting the whole `<id>` subtree needs no guess about what a
+ * future generation looks like. (The earlier flat `EXTENSIONS_DIR/<id>` path
+ * was dead weight besides: the sweep never considers that level, so it could
+ * not have protected anything.)
+ *
+ * ── WHAT THIS COSTS ──
+ * If an id is both preserved and runnable, that extension's SUPERSEDED
+ * generations stop being collected until the preserved row is removed. That is
+ * a stale directory, against deleting code a rolled-forward build still needs.
+ * The trade is deliberate and one-directional.
+ *
+ * ── THE LIMIT, STATED ──
+ * A row whose id does not validate protects NOTHING: we cannot name a
+ * directory we have no trustworthy name for, and guessing is how the sweep
+ * would delete an unrelated tree. Such a row survives in the ledger while its
+ * bundle is collected. Settings says so rather than offering a button that
+ * cannot work.
+ */
+export function preservedBundleExtensionIds(preserved: readonly PreservedLedgerRow[]): Set<string> {
+  const ids = new Set<string>()
+  for (const row of preserved) if (row.id !== null) ids.add(row.id)
+  return ids
+}
+
 /** Construct paths only from validated ledger fields, never a caller's path. */
 export function extensionBundleDirectory(row: InstalledExtension): string {
   return row.installation