From cb4f0b6b3c8e1ad06643d86c93f48751449ebe10 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 20 Sep 2026 00:26:44 -0700 Subject: [PATCH 1/2] fix(extensions): set aside an unusable ledger row instead of failing the file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #959. `readLedger` threw for the WHOLE file when any row failed the schema or the api-version gate. List, install, remove, the startup sweep, the scheme handler and every capability check read the ledger first — so one unusable row disabled every installed extension. And because `removeExtension` reads before it writes, Remove rejected too: the only recovery was hand-editing ~/.config/agent-code/extensions.json. A row this build cannot use is now preserved verbatim and set aside. The runnable rows load; the set-aside row is reported with its reason, and the user can remove it from Settings. WHY the raw value is carried rather than 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 go back untouched and the build that can read it gets its row back intact. WHY `writeLedger` re-reads the preserved rows instead of taking them as an argument: they are invisible to every caller by design, so if carrying them were the caller's job, forgetting would silently delete the one row we have the least right to destroy. `dropPreservedId` is the single deliberate exception — uninstalling a quarantined extension — and it takes an id, never a row. Their bundles count as REFERENCED by the sweep. The sweep runs at startup, so without that a single rollback-and-relaunch would delete the extension's code for good. Paths are still built only from an id that independently passes `isValidExtensionId` and a generation that is a uuid; an unvalidated row that cannot be named yields no path rather than a guess, and a protected path is never served or executed. A file that is not JSON, or not an array, still throws. Quarantine preserves a ROW; when reading rows is what failed there is nothing to set aside, and rewriting such a file is how bundles get orphaned. Tests drive the real exported functions against a real ledger file on disk, including the issue's own reproduction. Mutation-checked: restoring the api-version throw fails 5, dropping preserved rows on write fails 2, restoring the schema throw fails 2. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/extensions/install.ts | 16 +- src/main/extensions/ledger.ts | 220 +++++++++++++++--- .../ledgerQuarantine.system.test.ts | 181 ++++++++++++++ src/main/ipc/extensions.ts | 12 +- src/preload/api/extensions.ts | 5 + src/renderer/src/apps/ui/AppsSettingsRow.tsx | 89 ++++++- src/shared/types/extensions.ts | 21 ++ 7 files changed, 506 insertions(+), 38 deletions(-) create mode 100644 src/main/extensions/ledgerQuarantine.system.test.ts diff --git a/src/main/extensions/install.ts b/src/main/extensions/install.ts index ef1150c20..b7d52e7d4 100644 --- a/src/main/extensions/install.ts +++ b/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, preservedBundleDirectories, 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 { 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)) + for (const path of preserved.flatMap(preservedBundleDirectories)) referenced.add(path) const bundlesRoot = join(EXTENSIONS_DIR, '.bundles') let extensionDirs try { extensionDirs = await readdir(bundlesRoot, { withFileTypes: true }) } catch (error) { diff --git a/src/main/extensions/ledger.ts b/src/main/extensions/ledger.ts index d36b84fd2..08688efda 100644 --- a/src/main/extensions/ledger.ts +++ b/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 { +/** + * 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 { 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 { } 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 { + return (await readLedgerContents()).rows } /** @@ -138,14 +216,42 @@ export function withLedgerLock(operation: () => Promise): Promise { return next } -export async function writeLedger(rows: InstalledExtension[]): Promise { +/** + * 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 { + // 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 { * whereas silently hiding the row would leave the user wondering where their * extension went. */ +export async function listQuarantinedExtensions(): Promise { + 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 { return withLedgerLock(async () => { const rows = await readLedger() @@ -204,13 +320,53 @@ export async function removeExtension(id: string): Promise { // uninstall: reporting failure would encourage retrying an already-removed // extension. Saved extension state lives elsewhere and is never touched. await withLedgerLock(async () => { - const rows = await readLedger() + const { rows, preserved } = await readLedgerContents() const previous = rows.find(row => row.manifest.id === id) - await writeLedger(rows.filter(row => row.manifest.id !== id)) + // Uninstalling a QUARANTINED extension is the recovery path #959 exists for: + // the user can see a row this build cannot run and get rid of it from + // Settings, instead of editing JSON in ~/.config. Its bundle directory is + // not reclaimed — the path is built from validated fields, and a preserved + // row has by definition not been validated, so there is no path we are + // entitled to delete. The sweep stops treating it as referenced once the + // row is gone and collects it on the next pass. + const quarantined = preserved.some(row => row.id === id) + await writeLedger( + rows.filter(row => row.manifest.id !== id), + quarantined ? { dropPreservedId: id } : undefined, + ) if (previous) await discardExtensionBundle(previous) }) } +/** + * Every bundle directory a PRESERVED row could be pointing at (#959). + * + * WHY both layouts, and why this is safe despite the row being unvalidated: + * `extensionBundleDirectory` picks one of two shapes depending on whether the + * row has an `installation` generation, and we cannot ask a row this build + * failed to parse which era it is from. The only consumer is the sweep's + * "do not delete this" set, so over-protecting costs at most a stale directory + * until the row is removed, while under-protecting deletes an extension's code. + * + * The id and generation are still validated before being joined — an + * unvalidated row must never be able to name a path, even a protected one, and + * an id that does not pass `isValidExtensionId` yields NO paths rather than a + * guess. A protected path is never served, executed or read from; it is only + * ever compared against candidates for deletion. + */ +export function preservedBundleDirectories(row: PreservedLedgerRow): string[] { + if (row.id === null) return [] + const record = row.raw && typeof row.raw === 'object' ? (row.raw as Record) : null + const installation = record && typeof record.installation === 'object' && record.installation !== null + ? (record.installation as { id?: unknown }).id + : null + const paths = [join(EXTENSIONS_DIR, row.id)] + if (typeof installation === 'string' && /^[0-9a-f-]{36}$/i.test(installation)) { + paths.push(join(EXTENSIONS_DIR, '.bundles', row.id, installation)) + } + return paths +} + /** Construct paths only from validated ledger fields, never a caller's path. */ export function extensionBundleDirectory(row: InstalledExtension): string { return row.installation diff --git a/src/main/extensions/ledgerQuarantine.system.test.ts b/src/main/extensions/ledgerQuarantine.system.test.ts new file mode 100644 index 000000000..fffb48a4c --- /dev/null +++ b/src/main/extensions/ledgerQuarantine.system.test.ts @@ -0,0 +1,181 @@ +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises' +import { tmpdir } from 'os' +import { join } from 'path' +import { afterEach, describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// #959. `readLedger` threw for the WHOLE file when any row failed the schema or +// the api-version gate. List, install, remove, the startup sweep, the scheme +// handler and every capability check read the ledger first — so one unusable +// row disabled every installed extension, and because `removeExtension` reads +// before it writes, Remove rejected too. The only recovery was hand-editing +// ~/.config/agent-code/extensions.json. +// +// SYSTEM tier: a real ledger file on a real temp disk, through the real +// exported functions. The only thing redirected is the state directory. +// --------------------------------------------------------------------------- + +const root = await mkdtemp(join(tmpdir(), 'agent-code-ledger-quarantine-')) +const stateRoot = join(root, 'state') +const ledgerPath = join(stateRoot, 'extensions.json') + +vi.mock('@main/storage/paths.js', () => ({ + STATE_DIR: stateRoot, + EXTENSIONS_DIR: join(stateRoot, 'extensions'), + EXTENSIONS_LOCKFILE: ledgerPath, + EXTENSION_STATE_DIR: join(stateRoot, 'extension-state'), +})) + +const { + listInstalledExtensions, + listQuarantinedExtensions, + preservedBundleDirectories, + readLedger, + readLedgerContents, + removeExtension, + writeLedger, +} = await import('./ledger.js') + +/** A row the current build CAN run. Shaped like one finalizeInstall writes. */ +function validRow(id: string): Record { + return { + manifest: { id, name: id, description: 'fixture', version: '1.0.0', apiVersion: 1, entry: 'dist/index.js' }, + installation: { id: '11111111-2222-4333-8444-555555555555', bundleSha256: 'a'.repeat(64) }, + origin: 'github', + repo: `owner/${id}`, + ref: 'v1.0.0', + sha256: 'b'.repeat(64), + installedAt: 1, + } +} + +/** The issue's own reproduction: a row targeting an api version this build + * does not implement, which is what a ROLLBACK produces. */ +function futureApiRow(id: string): Record { + const row = validRow(id) + ;(row.manifest as Record).apiVersion = 3 + return row +} + +async function seed(rows: unknown[]): Promise { + await mkdir(stateRoot, { recursive: true }) + await writeFile(ledgerPath, `${JSON.stringify(rows, null, 2)}\n`, 'utf8') +} + +async function onDisk(): Promise { + return JSON.parse(await readFile(ledgerPath, 'utf8')) as unknown[] +} + +afterEach(async () => { + await rm(stateRoot, { recursive: true, force: true }) +}) + +describe('one unusable ledger row (#959)', () => { + it('does not stop the other extensions from loading', async () => { + await seed([validRow('timer'), futureApiRow('from-the-future')]) + + const rows = await readLedger() + expect(rows.map(row => row.manifest.id)).toEqual(['timer']) + + // The surface the user actually looks at. + const listed = await listInstalledExtensions() + expect(listed.map(row => row.manifest.id)).toEqual(['timer']) + }) + + it('reports the set-aside row, with the reason, so Settings can show it', async () => { + await seed([validRow('timer'), futureApiRow('from-the-future')]) + const quarantined = await listQuarantinedExtensions() + expect(quarantined).toHaveLength(1) + expect(quarantined[0]!.id).toBe('from-the-future') + expect(quarantined[0]!.reason).toContain('API v3') + }) + + it('lets Remove work on the OTHER extension, and keeps the set-aside row', async () => { + // This is the half that made the bug unrecoverable in-app: removeExtension + // reads the ledger before it writes, so it rejected too. + await seed([validRow('timer'), futureApiRow('from-the-future')]) + await removeExtension('timer') + + expect((await readLedger()).map(row => row.manifest.id)).toEqual([]) + // The row we could not read is still in the file, byte for byte. + const remaining = await onDisk() + expect(remaining).toEqual([futureApiRow('from-the-future')]) + }) + + it('carries the set-aside row through an unrelated write', async () => { + // writeLedger's callers only ever see runnable rows, so forgetting to + // carry a preserved one would silently delete the row we have the least + // right to destroy. + await seed([validRow('timer'), futureApiRow('from-the-future')]) + const { rows } = await readLedgerContents() + await writeLedger(rows) + + const remaining = await onDisk() + expect(remaining).toHaveLength(2) + expect(remaining[1]).toEqual(futureApiRow('from-the-future')) + }) + + it('lets the user remove the set-aside row itself', async () => { + await seed([validRow('timer'), futureApiRow('from-the-future')]) + await removeExtension('from-the-future') + + expect(await onDisk()).toEqual([validRow('timer')]) + expect(await listQuarantinedExtensions()).toEqual([]) + expect((await readLedger()).map(row => row.manifest.id)).toEqual(['timer']) + }) + + it('sets aside a schema-invalid row too, naming it when the id is safe', async () => { + const broken = validRow('broken') + delete broken.sha256 + await seed([validRow('timer'), broken]) + + expect((await readLedger()).map(row => row.manifest.id)).toEqual(['timer']) + const quarantined = await listQuarantinedExtensions() + expect(quarantined).toHaveLength(1) + expect(quarantined[0]!.id).toBe('broken') + }) + + it('reports — but cannot name — a row with no usable id', async () => { + // `id` is only carried when it independently passes isValidExtensionId, so + // a traversal-shaped id can never reach a path or a Remove call. + await seed([validRow('timer'), { manifest: { id: '../escape' }, repo: 'x' }]) + const quarantined = await listQuarantinedExtensions() + expect(quarantined).toHaveLength(1) + expect(quarantined[0]!.id).toBeNull() + }) + + it('still refuses to touch a ledger it cannot read at all', async () => { + // Quarantine preserves a ROW. When reading rows is what failed there is + // nothing to set aside, and rewriting the file is how bundles get orphaned. + await mkdir(stateRoot, { recursive: true }) + await writeFile(ledgerPath, '{ not json', 'utf8') + await expect(readLedger()).rejects.toThrow(/invalid JSON/) + await expect(writeLedger([])).rejects.toThrow(/invalid JSON/) + expect(await readFile(ledgerPath, 'utf8')).toBe('{ not json') + + await writeFile(ledgerPath, '{"rows":[]}', 'utf8') + await expect(readLedger()).rejects.toThrow(/must be an array/) + }) +}) + +describe('a set-aside row keeps its bundle (#959)', () => { + it('protects both possible bundle layouts, and nothing when the id is unusable', () => { + const generation = '11111111-2222-4333-8444-555555555555' + const paths = preservedBundleDirectories({ + raw: { manifest: { id: 'timer' }, installation: { id: generation } }, + id: 'timer', + reason: 'fixture', + }) + // The sweep runs at STARTUP, so under-protecting means one + // rollback-and-relaunch deletes the extension's code for good. + expect(paths.some(path => path.endsWith(join('extensions', 'timer')))).toBe(true) + expect(paths.some(path => path.endsWith(join('.bundles', 'timer', generation)))).toBe(true) + + // An id we could not validate yields NO path rather than a guess. + expect(preservedBundleDirectories({ raw: { manifest: { id: '../escape' } }, id: null, reason: 'x' })).toEqual([]) + // A generation that is not a uuid is not joined either. + expect( + preservedBundleDirectories({ raw: { manifest: { id: 'timer' }, installation: { id: '../..' } }, id: 'timer', reason: 'x' }), + ).toEqual([join(stateRoot, 'extensions', 'timer')]) + }) +}) diff --git a/src/main/ipc/extensions.ts b/src/main/ipc/extensions.ts index 637fc256c..db0aa22b8 100644 --- a/src/main/ipc/extensions.ts +++ b/src/main/ipc/extensions.ts @@ -9,13 +9,14 @@ import { } from '@main/extensions/storage.js' import { installExtension, installExtensionFromPath } from '@main/extensions/install.js' import type { ConsentPrompt } from '@main/extensions/install.js' -import { listInstalledExtensions, onExtensionPublication, removeExtension } from '@main/extensions/ledger.js' +import { listInstalledExtensions, listQuarantinedExtensions, onExtensionPublication, removeExtension } from '@main/extensions/ledger.js' import { installedExtensionCapabilities } from '@main/extensions/grants.js' import { isValidExtensionId } from '@shared/types/extensionId.js' import type { ExtensionCapability, ExtensionInstallResult, ExtensionListEntry, + QuarantinedExtensionEntry, } from '@shared/types/extensions.js' import { withVisibleControls } from '@shared/text/visibleControls.js' @@ -177,6 +178,15 @@ export function registerExtensionsIpc(): void { listInstalledExtensions(), ) + // Rows this build set aside (#959). Separate from `extensions:list` because + // they are NOT installed extensions: nothing here is loaded, activated or + // granted anything, and giving them the same shape would invite a caller to + // treat one as runnable. Settings shows them so the user can see why an + // extension is missing and remove it, which is the whole recovery path. + ipcMain.handle('extensions:list-quarantined', async (): Promise => + listQuarantinedExtensions(), + ) + // WHY install returns a result object instead of rejecting: every failure here is // something the user can act on — wrong repo name, private repo, missing // manifest, unsupported API version, archive too large. An IPC rejection reaches diff --git a/src/preload/api/extensions.ts b/src/preload/api/extensions.ts index 42c7e4fe8..5d30fe147 100644 --- a/src/preload/api/extensions.ts +++ b/src/preload/api/extensions.ts @@ -9,6 +9,7 @@ import type { ExtensionInstallResult, ExtensionListEntry, InstalledExtension, + QuarantinedExtensionEntry, } from '@shared/types/extensions.js' // Extension-app storage and management bridge. @@ -76,6 +77,10 @@ export const extensionsApi = { // extension must never be able to install or remove another extension. They are // called only by the Settings UI. extensionsList: (): Promise => ipcRenderer.invoke('extensions:list'), + + // Ledger rows this build could not use. Never runnable — see the type. + extensionsListQuarantined: (): Promise => + ipcRenderer.invoke('extensions:list-quarantined'), onExtensionsChanged: (handler: (rows: InstalledExtension[]) => void): (() => void) => subscribe('extensions:changed', handler), diff --git a/src/renderer/src/apps/ui/AppsSettingsRow.tsx b/src/renderer/src/apps/ui/AppsSettingsRow.tsx index fc2857a13..2214e4d7e 100644 --- a/src/renderer/src/apps/ui/AppsSettingsRow.tsx +++ b/src/renderer/src/apps/ui/AppsSettingsRow.tsx @@ -3,7 +3,7 @@ import { useCallback, useEffect, useState } from 'react' import { useAppStore } from '@renderer/app-state/hooks' import { forgetRemovedExtension, refreshInstalledExtensions } from '@renderer/apps/host/installedExtensionsState' -import type { ExtensionListEntry } from '@shared/types/extensions' +import type { ExtensionListEntry, QuarantinedExtensionEntry } from '@shared/types/extensions' import { withVisibleControls } from '@shared/text/visibleControls' /** @@ -28,13 +28,25 @@ export function AppsSettingsRow() { const [busy, setBusy] = useState(false) const [error, setError] = useState(null) const [notice, setNotice] = useState(null) + // Ledger rows this build set aside (#959). Local state, not the shared + // catalog: the catalog is the list of RUNNABLE extensions and every consumer + // of it (commands, panes, the frame broker) would have to learn to skip + // these. They exist for one screen, so they live on that screen. + const [quarantined, setQuarantined] = useState([]) const failures = useAppStore(state => state.extensionFailures) const refresh = refreshInstalledExtensions + const refreshQuarantined = useCallback(async () => { + // A failure here must not take the page down with it: the quarantine list + // is diagnostic, and the installed list above is the part the user came for. + try { setQuarantined(await window.api.extensionsListQuarantined()) } catch { setQuarantined([]) } + }, []) + useEffect(() => { void refresh() - }, [refresh]) + void refreshQuarantined() + }, [refresh, refreshQuarantined]) // FIRST install only — the repo comes from the text box, i.e. a string the // user just typed or pasted. That is the whole distinction main's @@ -146,9 +158,35 @@ export function AppsSettingsRow() { // Failure leaves the existing row visible. Success already removed // exactly the acknowledged generation, independently of this refresh. await refresh() + await refreshQuarantined() } }, - [busy, refresh], + [busy, refresh, refreshQuarantined], + ) + + // Uninstall a row this build set aside. Same IPC as any other remove — main + // drops the preserved row by id — but it takes an ID rather than an entry, + // because a quarantined row HAS no validated entry to pass. + const removeQuarantined = useCallback( + async (id: string) => { + if (busy) return + setBusy(true) + setError(null) + setNotice(null) + try { + await window.api.extensionsRemove(id) + setNotice(`Removed ${id}`) + } catch (removeError) { + setError(removeError instanceof Error ? removeError.message : String(removeError)) + } finally { + setBusy(false) + // Both lists: removing the last quarantined row is also what un-breaks + // the installed list on an older build. + await refresh() + await refreshQuarantined() + } + }, + [busy, refresh, refreshQuarantined], ) // "Load unpacked" — install from a local folder via the native picker (main @@ -302,6 +340,51 @@ export function AppsSettingsRow() { ))} )} + + {/* Rows this build set aside (#959). Rendered BELOW the installed list + and visually distinct, because the honest message is "this is not + running" — showing it beside working extensions would read as though + it were one. Before quarantine, a single such row threw for the whole + ledger: every extension stopped loading and Remove rejected too, so + the only way out was editing JSON in ~/.config. */} + {quarantined.length === 0 ? null : ( +
+
+ Set aside — not loaded by this version of Agent Code: +
+ {quarantined.map((row, index) => ( +
+
+ {/* The id is the only field of an unvalidated row we are + entitled to show, and only because it independently passed + isValidExtensionId. Escaped like every other repository- + authored string on an action surface (#1049). */} +
+ {row.id === null ? 'Unreadable extension record' : withVisibleControls(row.id)} +
+
{withVisibleControls(row.reason)}
+
+ {row.id === null ? ( + // Nothing safe to name in a Remove call. Saying so is better + // than a button that cannot work. +
Edit extensions.json to remove
+ ) : ( + + )} +
+ ))} +
+ )} ) } diff --git a/src/shared/types/extensions.ts b/src/shared/types/extensions.ts index dbe03511f..cdbe189d7 100644 --- a/src/shared/types/extensions.ts +++ b/src/shared/types/extensions.ts @@ -197,6 +197,27 @@ export type ExtensionListEntry = InstalledExtension & { present: boolean } +/** + * A ledger row this build set aside instead of running (#959). + * + * WHY it reaches the renderer at all, when its whole point is that it is NOT + * loaded: the user needs to know why an extension they installed is not there, + * and needs a way to remove it. Before quarantine, one such row threw for the + * entire ledger — every extension stopped loading and Remove rejected too, so + * the only recovery was hand-editing `~/.config/agent-code/extensions.json`. + * + * It carries NO manifest, deliberately. The row failed validation, so nothing + * on it may be treated as a name, a path or a version; `id` is present only + * when it independently passes `isValidExtensionId`, and it is used for one + * thing — naming the row in a Remove call. + */ +export type QuarantinedExtensionEntry = { + /** `manifest.id` when it is a valid id, else null (the row can only be reported). */ + id: string | null + /** Why this build rejected the row, in the user's terms. */ + reason: string +} + /** Runtime identity is a committed generation, never just the entry-file hash. * The fallback lets pre-generation installations keep loading until updated. */ export function extensionRevision(entry: InstalledExtension): string { From 6fd015cce279d94b0e51e547a3780ca979b398f9 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 20 Sep 2026 00:59:14 -0700 Subject: [PATCH 2/2] fix(extensions): separate clearing a set-aside row from uninstalling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1061 found three real defects, one of them destructive. 1. A ledger can hold a preserved row AND a runnable row under the same id — a rollback produces exactly that. `removeExtension` computed both independently and let both act, so one call did both things in both directions: clearing the set-aside row uninstalled the working extension and `rm -rf`'d its bundle, and uninstalling the working extension silently destroyed the row we have the least right to destroy. The reviewer reproduced both on disk. Ambiguity between two different objects is not resolvable by a precedence rule, so they are now two operations: `removeExtension` touches only runnable rows, `removeQuarantinedExtension` only preserved ones, with its own IPC channel and preload method. The quarantine remover never deletes a bundle — the path would have to come from fields that failed validation, and once the row is gone the sweep collects the directory on its own. 2. The sweep's preserved-bundle protection had NO test. Deleting the line left all 206 tests passing. The new tests drive the REAL `sweepAbandonedInstallDirectories`, not the helper: covering the helper alone is what let the protection go untested. 3. Protection was narrower than the rationale claimed. It was keyed on an exact path built from the raw row's `installation.id`, which evaporates in the one case the feature exists for: a future build that RESHAPES `installation` writes rows this build both rejects and cannot locate, so a single rollback-and-relaunch deleted the newer build's bundles. Protection is now keyed on the extension ID — validated independently, and the one level of `.bundles//` the sweep itself walks — so it needs no guess about what a future generation looks like. The old flat `EXTENSIONS_DIR/` path went with it: the sweep never considers that level, so it protected nothing while a test asserted it did. The cost is stated in the code: an id that is both preserved and runnable keeps its superseded generations until the preserved row is removed. A stale directory beats deleting code a rolled-forward build needs. The remaining limit is stated rather than papered over: a row whose id does not validate protects nothing, because we cannot name a directory we have no trustworthy name for, and guessing is how the sweep deletes an unrelated tree. Mutation-checked: dropping the sweep protection fails 2, and each direction of the remove collision fails 1. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/extensions/install.ts | 10 +- src/main/extensions/ledger.ts | 99 ++++++++++++------- .../ledgerQuarantine.system.test.ts | 98 ++++++++++++++---- src/main/ipc/extensions.ts | 10 +- src/preload/api/extensions.ts | 5 + src/renderer/src/apps/ui/AppsSettingsRow.tsx | 4 +- 6 files changed, 165 insertions(+), 61 deletions(-) diff --git a/src/main/extensions/install.ts b/src/main/extensions/install.ts index b7d52e7d4..c42017db5 100644 --- a/src/main/extensions/install.ts +++ b/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, preservedBundleDirectories, readLedger, readLedgerContents, 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' /** @@ -725,7 +725,7 @@ export async function sweepAbandonedInstallDirectories(): Promise { // deletion here, never served or executed. const { rows, preserved } = await readLedgerContents() const referenced = new Set(rows.map(extensionBundleDirectory)) - for (const path of preserved.flatMap(preservedBundleDirectories)) referenced.add(path) + const preservedIds = preservedBundleExtensionIds(preserved) const bundlesRoot = join(EXTENSIONS_DIR, '.bundles') let extensionDirs try { extensionDirs = await readdir(bundlesRoot, { withFileTypes: true }) } catch (error) { @@ -735,6 +735,12 @@ export async function sweepAbandonedInstallDirectories(): Promise { 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) diff --git a/src/main/extensions/ledger.ts b/src/main/extensions/ledger.ts index 08688efda..371603528 100644 --- a/src/main/extensions/ledger.ts +++ b/src/main/extensions/ledger.ts @@ -319,52 +319,79 @@ export async function removeExtension(id: string): Promise { // 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, preserved } = await readLedgerContents() + const rows = await readLedger() const previous = rows.find(row => row.manifest.id === id) - // Uninstalling a QUARANTINED extension is the recovery path #959 exists for: - // the user can see a row this build cannot run and get rid of it from - // Settings, instead of editing JSON in ~/.config. Its bundle directory is - // not reclaimed — the path is built from validated fields, and a preserved - // row has by definition not been validated, so there is no path we are - // entitled to delete. The sweep stops treating it as referenced once the - // row is gone and collects it on the next pass. - const quarantined = preserved.some(row => row.id === id) - await writeLedger( - rows.filter(row => row.manifest.id !== id), - quarantined ? { dropPreservedId: id } : undefined, - ) + await writeLedger(rows.filter(row => row.manifest.id !== id)) if (previous) await discardExtensionBundle(previous) }) } /** - * Every bundle directory a PRESERVED row could be pointing at (#959). + * Drop a PRESERVED row, and nothing else. * - * WHY both layouts, and why this is safe despite the row being unvalidated: - * `extensionBundleDirectory` picks one of two shapes depending on whether the - * row has an `installation` generation, and we cannot ask a row this build - * failed to parse which era it is from. The only consumer is the sweep's - * "do not delete this" set, so over-protecting costs at most a stale directory - * until the row is removed, while under-protecting deletes an extension's code. + * 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 { + 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. * - * The id and generation are still validated before being joined — an - * unvalidated row must never be able to name a path, even a protected one, and - * an id that does not pass `isValidExtensionId` yields NO paths rather than a - * guess. A protected path is never served, executed or read from; it is only - * ever compared against candidates for deletion. + * An id is different: it is validated independently by `isValidExtensionId`, + * and it is the ONE level of the `.bundles//` layout the sweep + * itself walks. Protecting the whole `` subtree needs no guess about what a + * future generation looks like. (The earlier flat `EXTENSIONS_DIR/` 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 preservedBundleDirectories(row: PreservedLedgerRow): string[] { - if (row.id === null) return [] - const record = row.raw && typeof row.raw === 'object' ? (row.raw as Record) : null - const installation = record && typeof record.installation === 'object' && record.installation !== null - ? (record.installation as { id?: unknown }).id - : null - const paths = [join(EXTENSIONS_DIR, row.id)] - if (typeof installation === 'string' && /^[0-9a-f-]{36}$/i.test(installation)) { - paths.push(join(EXTENSIONS_DIR, '.bundles', row.id, installation)) - } - return paths +export function preservedBundleExtensionIds(preserved: readonly PreservedLedgerRow[]): Set { + const ids = new Set() + 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. */ diff --git a/src/main/extensions/ledgerQuarantine.system.test.ts b/src/main/extensions/ledgerQuarantine.system.test.ts index fffb48a4c..39e2688c7 100644 --- a/src/main/extensions/ledgerQuarantine.system.test.ts +++ b/src/main/extensions/ledgerQuarantine.system.test.ts @@ -29,12 +29,14 @@ vi.mock('@main/storage/paths.js', () => ({ const { listInstalledExtensions, listQuarantinedExtensions, - preservedBundleDirectories, + preservedBundleExtensionIds, readLedger, readLedgerContents, removeExtension, + removeQuarantinedExtension, writeLedger, } = await import('./ledger.js') +const { sweepAbandonedInstallDirectories } = await import('./install.js') /** A row the current build CAN run. Shaped like one finalizeInstall writes. */ function validRow(id: string): Record { @@ -117,7 +119,7 @@ describe('one unusable ledger row (#959)', () => { it('lets the user remove the set-aside row itself', async () => { await seed([validRow('timer'), futureApiRow('from-the-future')]) - await removeExtension('from-the-future') + await removeQuarantinedExtension('from-the-future') expect(await onDisk()).toEqual([validRow('timer')]) expect(await listQuarantinedExtensions()).toEqual([]) @@ -158,24 +160,78 @@ describe('one unusable ledger row (#959)', () => { }) }) -describe('a set-aside row keeps its bundle (#959)', () => { - it('protects both possible bundle layouts, and nothing when the id is unusable', () => { - const generation = '11111111-2222-4333-8444-555555555555' - const paths = preservedBundleDirectories({ - raw: { manifest: { id: 'timer' }, installation: { id: generation } }, - id: 'timer', - reason: 'fixture', - }) - // The sweep runs at STARTUP, so under-protecting means one - // rollback-and-relaunch deletes the extension's code for good. - expect(paths.some(path => path.endsWith(join('extensions', 'timer')))).toBe(true) - expect(paths.some(path => path.endsWith(join('.bundles', 'timer', generation)))).toBe(true) - - // An id we could not validate yields NO path rather than a guess. - expect(preservedBundleDirectories({ raw: { manifest: { id: '../escape' } }, id: null, reason: 'x' })).toEqual([]) - // A generation that is not a uuid is not joined either. - expect( - preservedBundleDirectories({ raw: { manifest: { id: 'timer' }, installation: { id: '../..' } }, id: 'timer', reason: 'x' }), - ).toEqual([join(stateRoot, 'extensions', 'timer')]) +describe('a set-aside row and a runnable row can share an id (#959 review)', () => { + // A rollback writes a preserved row beside an id this build still runs. The + // two are different objects, so neither operation may act on the other: one + // handler doing whichever it found meant clearing the set-aside row + // uninstalled the working extension and deleted its bundle. + it('removing the set-aside row leaves the running extension and its bundle alone', async () => { + await seed([validRow('timer'), futureApiRow('timer')]) + const bundle = join(stateRoot, 'extensions', '.bundles', 'timer', '11111111-2222-4333-8444-555555555555') + await mkdir(bundle, { recursive: true }) + await writeFile(join(bundle, 'index.js'), 'export function activate() {}', 'utf8') + + await removeQuarantinedExtension('timer') + + expect((await readLedger()).map(row => row.manifest.id)).toEqual(['timer']) + expect(await listQuarantinedExtensions()).toEqual([]) + await expect(readFile(join(bundle, 'index.js'), 'utf8')).resolves.toContain('activate') + }) + + it('uninstalling the running extension leaves the set-aside row in the file', async () => { + await seed([validRow('timer'), futureApiRow('timer')]) + await removeExtension('timer') + + expect((await readLedger())).toEqual([]) + // The row we could not read is still there, byte for byte. + expect(await onDisk()).toEqual([futureApiRow('timer')]) + expect((await listQuarantinedExtensions())[0]!.id).toBe('timer') + }) +}) + +describe('a set-aside row keeps its bundle through the sweep (#959)', () => { + // The change the whole feature turns on: the sweep runs at STARTUP, so a + // single rollback-and-relaunch would otherwise delete the extension's code + // for good. Driven through the REAL sweep, not the helper — covering the + // helper alone left this untested, and deleting the protection changed + // nothing in the suite. + async function bundleAt(id: string, generation: string): Promise { + const dir = join(stateRoot, 'extensions', '.bundles', id, generation) + await mkdir(dir, { recursive: true }) + await writeFile(join(dir, 'index.js'), 'export function activate() {}', 'utf8') + return dir + } + + it('survives the sweep, and is collected once the row is gone', async () => { + await seed([futureApiRow('from-the-future')]) + const dir = await bundleAt('from-the-future', '11111111-2222-4333-8444-555555555555') + + await sweepAbandonedInstallDirectories() + await expect(readFile(join(dir, 'index.js'), 'utf8')).resolves.toContain('activate') + + await removeQuarantinedExtension('from-the-future') + await sweepAbandonedInstallDirectories() + await expect(readFile(join(dir, 'index.js'), 'utf8')).rejects.toThrow() + }) + + it('survives even when this build cannot parse the row\'s generation', async () => { + // THE case the feature exists for: a future build reshapes `installation`, + // so its rows are both rejected AND unlocatable. Protection keyed on the + // exact path evaporated here — precisely when it was needed. + const reshaped = validRow('from-the-future') + reshaped.installation = { generation: 'v2', digest: 'a'.repeat(64) } + await seed([reshaped]) + expect((await listQuarantinedExtensions())[0]!.id).toBe('from-the-future') + + const dir = await bundleAt('from-the-future', 'whatever-a-future-build-writes') + await sweepAbandonedInstallDirectories() + await expect(readFile(join(dir, 'index.js'), 'utf8')).resolves.toContain('activate') + }) + + it('protects only ids it can trust', () => { + expect(preservedBundleExtensionIds([ + { raw: {}, id: 'timer', reason: 'x' }, + { raw: {}, id: null, reason: 'unreadable' }, + ])).toEqual(new Set(['timer'])) }) }) diff --git a/src/main/ipc/extensions.ts b/src/main/ipc/extensions.ts index db0aa22b8..394f716ba 100644 --- a/src/main/ipc/extensions.ts +++ b/src/main/ipc/extensions.ts @@ -9,7 +9,7 @@ import { } from '@main/extensions/storage.js' import { installExtension, installExtensionFromPath } from '@main/extensions/install.js' import type { ConsentPrompt } from '@main/extensions/install.js' -import { listInstalledExtensions, listQuarantinedExtensions, onExtensionPublication, removeExtension } from '@main/extensions/ledger.js' +import { listInstalledExtensions, listQuarantinedExtensions, onExtensionPublication, removeExtension, removeQuarantinedExtension } from '@main/extensions/ledger.js' import { installedExtensionCapabilities } from '@main/extensions/grants.js' import { isValidExtensionId } from '@shared/types/extensionId.js' import type { @@ -312,6 +312,14 @@ export function registerExtensionsIpc(): void { await removeExtension(id) }) + // Clearing a SET-ASIDE row is a different operation from uninstalling an + // extension, not a flag on it (#959 review): a ledger can hold both under one + // id, and one handler doing whichever it found meant clearing the set-aside + // row uninstalled the working extension and deleted its bundle. + ipcMain.handle('extensions:remove-quarantined', async (_evt, id: string): Promise => { + await removeQuarantinedExtension(id) + }) + // Consent belongs to the committed installation, and is checked against a // fresh full-bundle digest. The helper holds the publication lock across both // reads, so an update cannot mix one generation's bytes with another's grant. diff --git a/src/preload/api/extensions.ts b/src/preload/api/extensions.ts index 5d30fe147..c9bd6d21e 100644 --- a/src/preload/api/extensions.ts +++ b/src/preload/api/extensions.ts @@ -108,6 +108,11 @@ export const extensionsApi = { extensionsRemove: (id: string): Promise => ipcRenderer.invoke('extensions:remove', id), + // Clear a set-aside row. A DIFFERENT operation from uninstalling: a ledger + // can hold a preserved row and a runnable one under the same id. + extensionsRemoveQuarantined: (id: string): Promise => + ipcRenderer.invoke('extensions:remove-quarantined', id), + // Reads the set of capabilities a user granted an extension, for the frame broker // to gate Tier 1-3 calls. A HOST method, not part of AgentCodeApiV1 — an extension // must never read (or change) its own or another's grants; only the broker calls it. diff --git a/src/renderer/src/apps/ui/AppsSettingsRow.tsx b/src/renderer/src/apps/ui/AppsSettingsRow.tsx index 2214e4d7e..8a9cded34 100644 --- a/src/renderer/src/apps/ui/AppsSettingsRow.tsx +++ b/src/renderer/src/apps/ui/AppsSettingsRow.tsx @@ -174,7 +174,9 @@ export function AppsSettingsRow() { setError(null) setNotice(null) try { - await window.api.extensionsRemove(id) + // The quarantine-specific call: `extensionsRemove` uninstalls the + // RUNNABLE extension of that id, and a ledger can hold both. + await window.api.extensionsRemoveQuarantined(id) setNotice(`Removed ${id}`) } catch (removeError) { setError(removeError instanceof Error ? removeError.message : String(removeError))