From 1247e968bed895baaa80b656b79a4dfab91eb54d Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 19:51:00 -0700 Subject: [PATCH 1/4] test(permissions): pin update_plan purge migration for approval stores --- .../approval-store-migration.test.ts | 194 ++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 src/permission/approval-store-migration.test.ts diff --git a/src/permission/approval-store-migration.test.ts b/src/permission/approval-store-migration.test.ts new file mode 100644 index 000000000..6a291778f --- /dev/null +++ b/src/permission/approval-store-migration.test.ts @@ -0,0 +1,194 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { generateSessionId, sessionDir } from "../session/index.js"; +import { loadSeededApprovals } from "../session/runtime-assembly.js"; +import { normalizeSeededApprovals } from "./authz-grants.js"; +import { migratePersistedApprovalStores } from "./approval-store-migration.js"; + +let cwd = ""; +let home = ""; +let sessionId = ""; + +const sessionStorePath = (): string => + join(sessionDir(cwd, sessionId, home), "permissions.json"); +const projectStorePath = (): string => join(cwd, ".corbits", "permissions.json"); +const globalStorePath = (): string => join(home, ".corbits", "permissions.json"); +const backupPath = (path: string): string => `${path}.bak`; + +async function readJson(path: string): Promise { + return JSON.parse(await readFile(path, "utf-8")) as unknown; +} + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "approval-migration-")); + home = await mkdtemp(join(tmpdir(), "approval-migration-home-")); + sessionId = generateSessionId(); + await mkdir(sessionDir(cwd, sessionId, home), { recursive: true }); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); +}); + +describe("migratePersistedApprovalStores", () => { + test("purges update_plan keys from every store, backs up, and re-runs as a no-op", async () => { + const sessionOriginal = { + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "npm *" }, + { tool: "bash", pattern: "git *" }, + ], + }; + const projectOriginal = { + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "manage_tasks", pattern: "tasks *" }, + ], + }; + const globalOriginal = { + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "git *" }, + ], + providerModels: { + "openai:gpt-5": [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "npm *" }, + ], + "anthropic:opus": [{ tool: "run_shell", pattern: "ls *" }], + }, + }; + await writeFile(sessionStorePath(), JSON.stringify(sessionOriginal)); + await mkdir(join(cwd, ".corbits"), { recursive: true }); + await writeFile(projectStorePath(), JSON.stringify(projectOriginal)); + await mkdir(join(home, ".corbits"), { recursive: true }); + await writeFile(globalStorePath(), JSON.stringify(globalOriginal)); + + const first = await migratePersistedApprovalStores(cwd, sessionId, home); + + expect(first.purged).toBe(4); + expect(first.backups).toHaveLength(3); + + expect(await readJson(sessionStorePath())).toEqual({ + approvals: [ + { tool: "run_shell", pattern: "npm *" }, + { tool: "bash", pattern: "git *" }, + ], + }); + expect(await readJson(projectStorePath())).toEqual({ + approvals: [{ tool: "manage_tasks", pattern: "tasks *" }], + }); + expect(await readJson(globalStorePath())).toEqual({ + approvals: [{ tool: "run_shell", pattern: "git *" }], + providerModels: { + "openai:gpt-5": [{ tool: "run_shell", pattern: "npm *" }], + "anthropic:opus": [{ tool: "run_shell", pattern: "ls *" }], + }, + }); + + for (const [path, original] of [ + [sessionStorePath(), sessionOriginal], + [projectStorePath(), projectOriginal], + [globalStorePath(), globalOriginal], + ] as const) { + expect(await readJson(backupPath(path))).toEqual(original); + } + + const sessionAfterFirst = await readFile(sessionStorePath(), "utf-8"); + const second = await migratePersistedApprovalStores(cwd, sessionId, home); + expect(second.purged).toBe(0); + expect(second.backups).toEqual([]); + expect(await readFile(sessionStorePath(), "utf-8")).toBe(sessionAfterFirst); + }); + + test("leaves clean stores untouched with no backup written", async () => { + const sessionOriginal = { + approvals: [{ tool: "run_shell", pattern: "npm *" }], + }; + await writeFile(sessionStorePath(), JSON.stringify(sessionOriginal)); + + const result = await migratePersistedApprovalStores(cwd, sessionId, home); + + expect(result.purged).toBe(0); + expect(result.backups).toEqual([]); + expect(await readJson(sessionStorePath())).toEqual(sessionOriginal); + await expect( + readFile(backupPath(sessionStorePath()), "utf-8"), + ).rejects.toThrow(); + }); + + test("treats missing and corrupt stores as no-ops", async () => { + await mkdir(join(cwd, ".corbits"), { recursive: true }); + await writeFile(projectStorePath(), "not json{{{"); + + const result = await migratePersistedApprovalStores(cwd, sessionId, home); + + expect(result.purged).toBe(0); + expect(result.backups).toEqual([]); + expect(await readFile(projectStorePath(), "utf-8")).toBe("not json{{{"); + }); + + test("purges exactly the keys the load-time normalizer drops", async () => { + const tools = [ + "update_plan", + "Update_Plan", + "default.update_plan", + "manage_tasks", + "run_shell", + ]; + await writeFile( + sessionStorePath(), + JSON.stringify({ + approvals: tools.map((tool) => ({ tool, pattern: "x *" })), + }), + ); + + const result = await migratePersistedApprovalStores(cwd, sessionId, home); + + const seeded = tools.map((tool) => ({ tool, pattern: "x *" })); + const droppedByNormalizer = seeded.filter( + (approval) => + !normalizeSeededApprovals([approval]).some( + (kept: { tool: string }) => kept.tool === approval.tool, + ), + ); + expect(result.purged).toBe(droppedByNormalizer.length); + expect(result.purged).toBe(3); + const remaining = ( + (await readJson(sessionStorePath())) as { + approvals: { tool: string }[]; + } + ).approvals.map((approval) => approval.tool); + expect(remaining).toEqual(["manage_tasks", "run_shell"]); + }); + + test("seed loading purges on-disk update_plan keys while the normalizer still drops them in memory", async () => { + await writeFile( + sessionStorePath(), + JSON.stringify({ + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "npm *" }, + ], + }), + ); + await mkdir(join(cwd, ".corbits"), { recursive: true }); + await writeFile( + projectStorePath(), + JSON.stringify({ + approvals: [{ tool: "update_plan", pattern: "plan *" }], + }), + ); + + const seeded = await loadSeededApprovals(cwd, sessionId, home); + + expect(seeded).toEqual([{ tool: "run_shell", pattern: "npm *" }]); + expect(await readJson(sessionStorePath())).toEqual({ + approvals: [{ tool: "run_shell", pattern: "npm *" }], + }); + expect(await readJson(projectStorePath())).toEqual({ approvals: [] }); + }); +}); From ff9673c161fc7c51caa9188a869ee945da9f6091 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 19:57:52 -0700 Subject: [PATCH 2/4] feat(permissions): purge persisted update_plan keys with backup-then-rewrite migration --- .../approval-store-migration.test.ts | 13 +- src/permission/approval-store-migration.ts | 173 ++++++++++++++++++ src/session/runtime-assembly.ts | 6 + 3 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 src/permission/approval-store-migration.ts diff --git a/src/permission/approval-store-migration.test.ts b/src/permission/approval-store-migration.test.ts index 6a291778f..d56010108 100644 --- a/src/permission/approval-store-migration.test.ts +++ b/src/permission/approval-store-migration.test.ts @@ -13,8 +13,10 @@ let sessionId = ""; const sessionStorePath = (): string => join(sessionDir(cwd, sessionId, home), "permissions.json"); -const projectStorePath = (): string => join(cwd, ".corbits", "permissions.json"); -const globalStorePath = (): string => join(home, ".corbits", "permissions.json"); +const projectStorePath = (): string => + join(cwd, ".corbits", "permissions.json"); +const globalStorePath = (): string => + join(home, ".corbits", "permissions.json"); const backupPath = (path: string): string => `${path}.bak`; async function readJson(path: string): Promise { @@ -185,7 +187,12 @@ describe("migratePersistedApprovalStores", () => { const seeded = await loadSeededApprovals(cwd, sessionId, home); - expect(seeded).toEqual([{ tool: "run_shell", pattern: "npm *" }]); + expect(seeded).toEqual( + expect.arrayContaining([{ tool: "run_shell", pattern: "npm *" }]), + ); + expect(seeded.some((approval) => approval.tool === "update_plan")).toBe( + false, + ); expect(await readJson(sessionStorePath())).toEqual({ approvals: [{ tool: "run_shell", pattern: "npm *" }], }); diff --git a/src/permission/approval-store-migration.ts b/src/permission/approval-store-migration.ts new file mode 100644 index 000000000..7d71cdf39 --- /dev/null +++ b/src/permission/approval-store-migration.ts @@ -0,0 +1,173 @@ +import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { homedir } from "node:os"; +import { dirname, join } from "node:path"; + +import { getLogger } from "@intx/log"; + +import { canonicalGrantTool } from "../agent/canonical-tool-name.js"; +import { LOG_NAMESPACE_ROOT, SETTINGS_DIR_NAME } from "../branding.js"; +import { sessionDir } from "../session/index.js"; + +const log = getLogger([LOG_NAMESPACE_ROOT, "permission", "approval-migration"]); + +export interface ApprovalStoreMigrationFileResult { + path: string; + purged: number; + backupPath?: string | undefined; +} + +export interface ApprovalStoreMigrationResult { + purged: number; + backups: string[]; + files: ApprovalStoreMigrationFileResult[]; +} + +// canonicalGrantTool is the single owner of "is an update_plan key": it maps +// update_plan (any case, default.-prefixed, or doubled) to null so the +// load-time normalizer drops it fail-closed. The migration delegates to it so +// disk and memory can never disagree about which keys purge. +function isUpdatePlanKey(tool: unknown): boolean { + return typeof tool === "string" && canonicalGrantTool(tool) === null; +} + +function isUpdatePlanEntry(entry: unknown): boolean { + return ( + typeof entry === "object" && + entry !== null && + !Array.isArray(entry) && + isUpdatePlanKey((entry as Record).tool) + ); +} + +function purgeList( + list: unknown, + onPurge: () => void, +): { kept: unknown[]; changed: boolean } { + if (!Array.isArray(list)) return { kept: [], changed: false }; + const kept: unknown[] = []; + for (const entry of list) { + if (isUpdatePlanEntry(entry)) onPurge(); + else kept.push(entry); + } + return { kept, changed: kept.length !== list.length }; +} + +function isFileExistsError(err: unknown): boolean { + return ( + typeof err === "object" && + err !== null && + "code" in err && + (err as { code?: unknown }).code === "EEXIST" + ); +} + +// Purge update_plan keys from one approvals file (session, project, or +// global store shape: an `approvals` array plus, for the global file, a +// `providerModels` map of arrays). Backup-then-rewrite: the pre-migration +// bytes are saved to `.bak` first (an existing backup is kept, so the +// first backup always holds the true original), and a file with nothing to +// left untouched (no backup, byte-identical) so re-runs are no-ops. Entries +// that are not positive update_plan matches are kept verbatim — pure renames +// are never collapsed here; that stays the load-time normalizer's job. +// Missing, unreadable, or corrupt files are no-ops; write failures propagate. +export async function migrateApprovalStoreFile( + path: string, +): Promise { + let raw: string; + try { + raw = await readFile(path, "utf-8"); + } catch { + return { path, purged: 0 }; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw) as unknown; + } catch { + return { path, purged: 0 }; + } + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return { path, purged: 0 }; + } + const record = parsed as Record; + const next: Record = { ...record }; + let purged = 0; + const onPurge = (): void => { + purged += 1; + }; + if (Array.isArray(record.approvals)) { + const { kept, changed } = purgeList(record.approvals, onPurge); + if (changed) next.approvals = kept; + } + const providerModels = record.providerModels; + if ( + typeof providerModels === "object" && + providerModels !== null && + !Array.isArray(providerModels) + ) { + const map = providerModels as Record; + const nextMap: Record = {}; + let mapChanged = false; + for (const [key, list] of Object.entries(map)) { + const { kept, changed } = purgeList(list, onPurge); + if (changed) { + nextMap[key] = kept; + mapChanged = true; + } else { + nextMap[key] = list; + } + } + if (mapChanged) next.providerModels = nextMap; + } + if (purged === 0) return { path, purged: 0 }; + const backupPath = `${path}.bak`; + try { + await writeFile(backupPath, raw, { flag: "wx" }); + } catch (err) { + if (!isFileExistsError(err)) { + log.warn("Skipping approval-store migration for {path}: {error}", { + path, + error: err instanceof Error ? err.message : String(err), + }); + return { path, purged: 0 }; + } + } + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, JSON.stringify(next, null, 2)); + return { path, purged, backupPath }; +} + +// One-time migration over the persisted approval stores (session, project, +// global including provider-model grants): purge on-disk update_plan keys so +// removing the load-time normalizer later cannot resurrect the hole. The +// paths mirror store.ts; the files array in the result keeps them explicit. +// Best-effort and idempotent — never throws, and a clean tree is a no-op. +export async function migratePersistedApprovalStores( + cwd: string, + sessionId: string, + home: string = homedir(), +): Promise { + const paths = [ + join(sessionDir(cwd, sessionId, home), "permissions.json"), + join(cwd, SETTINGS_DIR_NAME, "permissions.json"), + join(home, SETTINGS_DIR_NAME, "permissions.json"), + ]; + const files: ApprovalStoreMigrationFileResult[] = []; + for (const path of paths) { + try { + files.push(await migrateApprovalStoreFile(path)); + } catch (err) { + log.warn("Skipping approval-store migration for {path}: {error}", { + path, + error: err instanceof Error ? err.message : String(err), + }); + files.push({ path, purged: 0 }); + } + } + return { + purged: files.reduce((total, file) => total + file.purged, 0), + backups: files.flatMap((file) => + file.backupPath !== undefined ? [file.backupPath] : [], + ), + files, + }; +} diff --git a/src/session/runtime-assembly.ts b/src/session/runtime-assembly.ts index 296be7c78..d26635bd5 100644 --- a/src/session/runtime-assembly.ts +++ b/src/session/runtime-assembly.ts @@ -38,6 +38,7 @@ import { type PluginModule, } from "../plugins/loader.js"; import { isPluginModuleEnabled } from "../plugins/register.js"; +import { migratePersistedApprovalStores } from "../permission/approval-store-migration.js"; import { formatPendingProjectApprovals, loadApprovals, @@ -137,6 +138,11 @@ export async function loadSeededApprovals( home?: string, opts?: { onPendingProjectGrants?: ((text: string) => void) | undefined }, ): Promise { + // One-time migration: purge persisted update_plan keys (dropped at load by + // normalizeSeededApprovals but never rewritten) so removing the normalizer + // later cannot resurrect them. Best-effort and idempotent — a clean tree is + // a no-op, and the normalizer stays as defense-in-depth regardless. + await migratePersistedApprovalStores(cwd, sessionId, home); const sessionApprovals = await loadApprovals(cwd, sessionId, home); const [ projectApprovals, From fa2663985fd920f8d6a2f374cba164d6748f9fb2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 20:12:55 -0700 Subject: [PATCH 3/4] fix(permissions): harden approval migration call site and writes Guard the session-start migration call so a future throw degrades to log-and-continue, and route the migration rewrite through the shared chained tmp+rename writer so a concurrent grant mint serializes with the purge instead of losing an update. --- .../approval-store-migration.test.ts | 69 ++++++++++ src/permission/approval-store-migration.ts | 130 +++++++++--------- src/permission/store.ts | 15 +- .../runtime-assembly-migration.test.ts | 62 +++++++++ src/session/runtime-assembly.ts | 11 +- 5 files changed, 218 insertions(+), 69 deletions(-) create mode 100644 src/session/runtime-assembly-migration.test.ts diff --git a/src/permission/approval-store-migration.test.ts b/src/permission/approval-store-migration.test.ts index d56010108..79d37f858 100644 --- a/src/permission/approval-store-migration.test.ts +++ b/src/permission/approval-store-migration.test.ts @@ -6,6 +6,7 @@ import { generateSessionId, sessionDir } from "../session/index.js"; import { loadSeededApprovals } from "../session/runtime-assembly.js"; import { normalizeSeededApprovals } from "./authz-grants.js"; import { migratePersistedApprovalStores } from "./approval-store-migration.js"; +import { saveGlobalApproval } from "./store.js"; let cwd = ""; let home = ""; @@ -167,6 +168,74 @@ describe("migratePersistedApprovalStores", () => { expect(remaining).toEqual(["manage_tasks", "run_shell"]); }); + test("a grant minted while the migration runs is not lost and the file stays valid", async () => { + await mkdir(join(home, ".corbits"), { recursive: true }); + await writeFile( + globalStorePath(), + JSON.stringify({ + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "git *" }, + ], + }), + ); + + const minted = { tool: "run_shell", pattern: "npm *" }; + const [result] = await Promise.all([ + migratePersistedApprovalStores(cwd, sessionId, home), + saveGlobalApproval(minted, home), + ]); + + expect(result.purged).toBe(1); + const final = (await readJson(globalStorePath())) as { + approvals: { tool: string; pattern: string }[]; + }; + expect( + final.approvals.some((approval) => approval.tool === "update_plan"), + ).toBe(false); + expect(final.approvals).toContainEqual({ + tool: "run_shell", + pattern: "git *", + }); + expect(final.approvals).toContainEqual(minted); + const backup = (await readJson(backupPath(globalStorePath()))) as { + approvals: { tool: string; pattern: string }[]; + }; + expect(backup.approvals).toContainEqual({ + tool: "update_plan", + pattern: "plan *", + }); + }); + + test("a storm of concurrent grants around the migration loses nothing", async () => { + await mkdir(join(home, ".corbits"), { recursive: true }); + await writeFile( + globalStorePath(), + JSON.stringify({ + approvals: [{ tool: "update_plan", pattern: "plan *" }], + }), + ); + + const minted = Array.from({ length: 10 }, (_, i) => ({ + tool: "run_shell", + pattern: `storm-${i} *`, + })); + await Promise.all([ + migratePersistedApprovalStores(cwd, sessionId, home), + ...minted.map((approval) => saveGlobalApproval(approval, home)), + ]); + + const final = (await readJson(globalStorePath())) as { + approvals: { tool: string; pattern: string }[]; + }; + expect( + final.approvals.some((approval) => approval.tool === "update_plan"), + ).toBe(false); + for (const approval of minted) { + expect(final.approvals).toContainEqual(approval); + } + }); + test("seed loading purges on-disk update_plan keys while the normalizer still drops them in memory", async () => { await writeFile( sessionStorePath(), diff --git a/src/permission/approval-store-migration.ts b/src/permission/approval-store-migration.ts index 7d71cdf39..026c62d0f 100644 --- a/src/permission/approval-store-migration.ts +++ b/src/permission/approval-store-migration.ts @@ -1,12 +1,13 @@ -import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { writeFile } from "node:fs/promises"; import { homedir } from "node:os"; -import { dirname, join } from "node:path"; +import { join } from "node:path"; import { getLogger } from "@intx/log"; import { canonicalGrantTool } from "../agent/canonical-tool-name.js"; import { LOG_NAMESPACE_ROOT, SETTINGS_DIR_NAME } from "../branding.js"; import { sessionDir } from "../session/index.js"; +import { chainObjectWrite } from "./store.js"; const log = getLogger([LOG_NAMESPACE_ROOT, "permission", "approval-migration"]); @@ -64,75 +65,78 @@ function isFileExistsError(err: unknown): boolean { // Purge update_plan keys from one approvals file (session, project, or // global store shape: an `approvals` array plus, for the global file, a // `providerModels` map of arrays). Backup-then-rewrite: the pre-migration -// bytes are saved to `.bak` first (an existing backup is kept, so the +// state is saved to `.bak` first (an existing backup is kept, so the // first backup always holds the true original), and a file with nothing to -// left untouched (no backup, byte-identical) so re-runs are no-ops. Entries -// that are not positive update_plan matches are kept verbatim — pure renames -// are never collapsed here; that stays the load-time normalizer's job. -// Missing, unreadable, or corrupt files are no-ops; write failures propagate. +// purge is left untouched (no backup, byte-identical) so re-runs are no-ops. +// Entries that are not positive update_plan matches are kept verbatim — pure +// renames are never collapsed here; that stays the load-time normalizer's +// job. Missing, unreadable, or corrupt files are no-ops; write failures +// propagate. The rewrite goes through chainObjectWrite, so a concurrent grant +// mint to the same file serializes with the migration instead of losing an +// update, and the tmp+rename lands atomically so a reader never sees a torn +// file. export async function migrateApprovalStoreFile( path: string, ): Promise { - let raw: string; - try { - raw = await readFile(path, "utf-8"); - } catch { - return { path, purged: 0 }; - } - let parsed: unknown; - try { - parsed = JSON.parse(raw) as unknown; - } catch { - return { path, purged: 0 }; - } - if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { - return { path, purged: 0 }; - } - const record = parsed as Record; - const next: Record = { ...record }; - let purged = 0; - const onPurge = (): void => { - purged += 1; - }; - if (Array.isArray(record.approvals)) { - const { kept, changed } = purgeList(record.approvals, onPurge); - if (changed) next.approvals = kept; - } - const providerModels = record.providerModels; - if ( - typeof providerModels === "object" && - providerModels !== null && - !Array.isArray(providerModels) - ) { - const map = providerModels as Record; - const nextMap: Record = {}; - let mapChanged = false; - for (const [key, list] of Object.entries(map)) { - const { kept, changed } = purgeList(list, onPurge); - if (changed) { - nextMap[key] = kept; - mapChanged = true; - } else { - nextMap[key] = list; - } - } - if (mapChanged) next.providerModels = nextMap; - } - if (purged === 0) return { path, purged: 0 }; const backupPath = `${path}.bak`; + let purged = 0; + let rewrote = false; try { - await writeFile(backupPath, raw, { flag: "wx" }); + await chainObjectWrite(path, async (current) => { + const next: Record = { ...current }; + let changed = 0; + const onPurge = (): void => { + changed += 1; + }; + if (Array.isArray(current.approvals)) { + const { kept, changed: listChanged } = purgeList( + current.approvals, + onPurge, + ); + if (listChanged) next.approvals = kept; + } + const providerModels = current.providerModels; + if ( + typeof providerModels === "object" && + providerModels !== null && + !Array.isArray(providerModels) + ) { + const map = providerModels as Record; + const nextMap: Record = {}; + let mapChanged = false; + for (const [key, list] of Object.entries(map)) { + const { kept, changed: listChanged } = purgeList(list, onPurge); + nextMap[key] = listChanged ? kept : list; + mapChanged = mapChanged || listChanged; + } + if (mapChanged) next.providerModels = nextMap; + } + if (changed === 0) return undefined; + try { + await writeFile(backupPath, JSON.stringify(current, null, 2), { + flag: "wx", + }); + } catch (err) { + if (!isFileExistsError(err)) { + log.warn("Skipping approval-store migration for {path}: {error}", { + path, + error: err instanceof Error ? err.message : String(err), + }); + return undefined; + } + } + purged = changed; + rewrote = true; + return next; + }); } catch (err) { - if (!isFileExistsError(err)) { - log.warn("Skipping approval-store migration for {path}: {error}", { - path, - error: err instanceof Error ? err.message : String(err), - }); - return { path, purged: 0 }; - } + log.warn("Skipping approval-store migration for {path}: {error}", { + path, + error: err instanceof Error ? err.message : String(err), + }); + return { path, purged: 0 }; } - await mkdir(dirname(path), { recursive: true }); - await writeFile(path, JSON.stringify(next, null, 2)); + if (!rewrote) return { path, purged: 0 }; return { path, purged, backupPath }; } diff --git a/src/permission/store.ts b/src/permission/store.ts index b6233246f..ee8528da0 100644 --- a/src/permission/store.ts +++ b/src/permission/store.ts @@ -105,14 +105,21 @@ async function readObjectFile(path: string): Promise> { // Serialize the full read-modify-write per path so concurrent grants to the same // file (a global and a provider-model grant resolving together both touch the // global file) never lose an update, and rename atomically so a reader never -// observes a torn file. -function chainObjectWrite( +// observes a torn file. Returning undefined from mutate skips the write, which +// keeps migrations that find nothing to purge byte-identical no-ops. +export function chainObjectWrite( path: string, - mutate: (current: Record) => Record, + mutate: ( + current: Record, + ) => + | Record + | undefined + | Promise | undefined>, ): Promise { const tmp = `${path}.${process.pid}.tmp`; const run = async (): Promise => { - const next = mutate(await readObjectFile(path)); + const next = await mutate(await readObjectFile(path)); + if (next === undefined) return; await mkdir(dirname(path), { recursive: true }); await writeFile(tmp, JSON.stringify(next, null, 2)); await rename(tmp, path); diff --git a/src/session/runtime-assembly-migration.test.ts b/src/session/runtime-assembly-migration.test.ts new file mode 100644 index 000000000..2bdc5331a --- /dev/null +++ b/src/session/runtime-assembly-migration.test.ts @@ -0,0 +1,62 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { withMockedModuleDuring } from "../../tests/helpers/mock-module.js"; +import type * as migrationModule from "../permission/approval-store-migration.js"; +import { generateSessionId, initSessionDir, sessionDir } from "./index.js"; + +describe("loadSeededApprovals migration guard", () => { + let cwd = ""; + let home = ""; + let sessionId = ""; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "migration-guard-")); + home = await mkdtemp(join(tmpdir(), "migration-guard-home-")); + sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + }); + + afterEach(async () => { + if (cwd !== "") await rm(cwd, { recursive: true, force: true }); + if (home !== "") await rm(home, { recursive: true, force: true }); + cwd = ""; + home = ""; + sessionId = ""; + }); + + test("session start proceeds when the migration throws", async () => { + await mkdir(sessionDir(cwd, sessionId, home), { recursive: true }); + await writeFile( + join(sessionDir(cwd, sessionId, home), "permissions.json"), + JSON.stringify({ + approvals: [{ tool: "run_shell", pattern: "session npm *" }], + }), + ); + + let migrationCalls = 0; + const seeded = await withMockedModuleDuring( + import.meta.resolve("../permission/approval-store-migration.js"), + (real: typeof migrationModule) => ({ + ...real, + migratePersistedApprovalStores: (): Promise => { + migrationCalls += 1; + return Promise.reject(new Error("migration boom")); + }, + }), + async () => { + const { loadSeededApprovals } = await import("./runtime-assembly.js"); + return loadSeededApprovals(cwd, sessionId, home); + }, + ); + + expect(migrationCalls).toBe(1); + + expect(seeded).toContainEqual({ + tool: "run_shell", + pattern: "session npm *", + }); + }); +}); diff --git a/src/session/runtime-assembly.ts b/src/session/runtime-assembly.ts index d26635bd5..cde34fb4d 100644 --- a/src/session/runtime-assembly.ts +++ b/src/session/runtime-assembly.ts @@ -141,8 +141,15 @@ export async function loadSeededApprovals( // One-time migration: purge persisted update_plan keys (dropped at load by // normalizeSeededApprovals but never rewritten) so removing the normalizer // later cannot resurrect them. Best-effort and idempotent — a clean tree is - // a no-op, and the normalizer stays as defense-in-depth regardless. - await migratePersistedApprovalStores(cwd, sessionId, home); + // a no-op, and the normalizer stays as defense-in-depth regardless. Guarded + // so a future throw can never break session start. + try { + await migratePersistedApprovalStores(cwd, sessionId, home); + } catch (err) { + persistLogger.warn("Skipping approval-store migration: {error}", { + error: err instanceof Error ? err.message : String(err), + }); + } const sessionApprovals = await loadApprovals(cwd, sessionId, home); const [ projectApprovals, From 7145c25ac73ca56efbf10ee200d84112ab5fd186 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 26 Sep 2026 08:25:12 -0700 Subject: [PATCH 4/4] fix(permissions): write approval backups atomically and exclusively A direct wx write of .bak can crash mid-write and leave a torn rollback copy. The next start treats that EEXIST as success and purges live anyway. Write the full backup to a sibling tmp, then link it onto .bak so the name appears complete or not at all, and a pre-existing original still wins. --- .../approval-store-migration.test.ts | 55 ++++++++++++++++++- src/permission/approval-store-migration.ts | 31 +++++++++-- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/src/permission/approval-store-migration.test.ts b/src/permission/approval-store-migration.test.ts index 79d37f858..582f1319a 100644 --- a/src/permission/approval-store-migration.test.ts +++ b/src/permission/approval-store-migration.test.ts @@ -1,7 +1,14 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { + chmod, + mkdir, + mkdtemp, + readFile, + rm, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { generateSessionId, sessionDir } from "../session/index.js"; import { loadSeededApprovals } from "../session/runtime-assembly.js"; import { normalizeSeededApprovals } from "./authz-grants.js"; @@ -267,4 +274,48 @@ describe("migratePersistedApprovalStores", () => { }); expect(await readJson(projectStorePath())).toEqual({ approvals: [] }); }); + + test("EACCES writing the backup leaves live bytes unchanged", async () => { + const path = sessionStorePath(); + const original = { + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "npm *" }, + ], + }; + await writeFile(path, JSON.stringify(original)); + const liveBytes = await readFile(path, "utf-8"); + const dir = dirname(path); + await chmod(dir, 0o555); + try { + const result = await migratePersistedApprovalStores(cwd, sessionId, home); + expect(result.purged).toBe(0); + expect(result.backups).toEqual([]); + expect(await readFile(path, "utf-8")).toBe(liveBytes); + } finally { + await chmod(dir, 0o755); + } + }); + + test("existing torn .bak plus live update_plan still purges live", async () => { + const path = sessionStorePath(); + const original = { + approvals: [ + { tool: "update_plan", pattern: "plan *" }, + { tool: "run_shell", pattern: "npm *" }, + ], + }; + await writeFile(path, JSON.stringify(original)); + const torn = '{"approvals":[{"tool":"update_plan"'; + await writeFile(backupPath(path), torn); + + const result = await migratePersistedApprovalStores(cwd, sessionId, home); + + expect(result.purged).toBe(1); + expect(result.backups).toEqual([backupPath(path)]); + expect(await readFile(backupPath(path), "utf-8")).toBe(torn); + expect(await readJson(path)).toEqual({ + approvals: [{ tool: "run_shell", pattern: "npm *" }], + }); + }); }); diff --git a/src/permission/approval-store-migration.ts b/src/permission/approval-store-migration.ts index 026c62d0f..1da3f4973 100644 --- a/src/permission/approval-store-migration.ts +++ b/src/permission/approval-store-migration.ts @@ -1,4 +1,4 @@ -import { writeFile } from "node:fs/promises"; +import { link, unlink, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { join } from "node:path"; @@ -62,6 +62,25 @@ function isFileExistsError(err: unknown): boolean { ); } +// Exclusive atomic backup: fully write a sibling tmp, then link it onto +// `.bak` so the rollback copy never appears torn. rename would replace a +// pre-existing bak and lose first-original-wins; a direct wx write of `.bak` +// can crash mid-write and leave a truncated file that a later start treats as +// EEXIST success. link is exclusive (EEXIST if the name is taken) and the +// destination inode is complete at the moment it appears. +async function writeBackupExclusive( + backupPath: string, + body: string, +): Promise { + const tmp = `${backupPath}.${process.pid}.tmp`; + try { + await writeFile(tmp, body); + await link(tmp, backupPath); + } finally { + await unlink(tmp).catch(() => undefined); + } +} + // Purge update_plan keys from one approvals file (session, project, or // global store shape: an `approvals` array plus, for the global file, a // `providerModels` map of arrays). Backup-then-rewrite: the pre-migration @@ -74,7 +93,8 @@ function isFileExistsError(err: unknown): boolean { // propagate. The rewrite goes through chainObjectWrite, so a concurrent grant // mint to the same file serializes with the migration instead of losing an // update, and the tmp+rename lands atomically so a reader never sees a torn -// file. +// file. The backup uses the same tmp-then-place atomicity, exclusive so a +// pre-existing `.bak` wins. export async function migrateApprovalStoreFile( path: string, ): Promise { @@ -113,9 +133,10 @@ export async function migrateApprovalStoreFile( } if (changed === 0) return undefined; try { - await writeFile(backupPath, JSON.stringify(current, null, 2), { - flag: "wx", - }); + await writeBackupExclusive( + backupPath, + JSON.stringify(current, null, 2), + ); } catch (err) { if (!isFileExistsError(err)) { log.warn("Skipping approval-store migration for {path}: {error}", {