Skip to content

feat(sync): add manual rollback #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ndycode wants to merge 2 commits into
base: git-plan/14-auto-snapshots
Choose a base branch
from
+634 −19
Open

feat(sync): add manual rollback #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
769fe33
feat(sync): add manual rollback
ndycode Mar 12, 2026
5444db0
feat(sync): add manual rollback
ndycode Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
265 changes: 250 additions & 15 deletions lib/codex-cli/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import { existsSync, promises as fs } from "node:fs";
import { createLogger } from "../logger.js";
import { MODEL_FAMILIES, type ModelFamily } from "../prompts/codex.js";
import {
type AccountMetadataV3,
type AccountStorageV3,
getLastAccountsSaveTimestamp,
getStoragePath,
type NamedBackupMetadata,
normalizeAccountStorage,
restoreNamedBackup,
saveAccounts,
snapshotAccountStorage,
} from "../storage.js";
import {
appendSyncHistoryEntry,
cloneSyncHistoryEntry,
readLatestSyncHistorySync,
readSyncHistory,
} from "../sync-history.js";
import {
incrementCodexCliMetric,
Expand Down Expand Up @@ -66,6 +73,37 @@ export interface CodexCliSyncSummary {
selectionChanged: boolean;
}

export type CodexCliSyncTrigger = "manual" | "automatic";

export interface CodexCliSyncRollbackSnapshot {
name: string;
path: string;
}

export interface CodexCliSyncRollbackExecutionResult {
snapshot: CodexCliSyncRollbackSnapshot;
restore: {
imported: number;
total: number;
skipped: number;
};
}

export interface CodexCliSyncRollbackPlan {
status: "ready" | "unavailable";
reason: string;
snapshot: CodexCliSyncRollbackSnapshot | null;
accountCount?: number;
storage?: AccountStorageV3;
}

export interface CodexCliSyncRollbackPlanResult {
status: "restored" | "unavailable" | "error";
reason: string;
snapshot: CodexCliSyncRollbackSnapshot | null;
accountCount?: number;
}

export interface CodexCliSyncBackupContext {
enabled: boolean;
targetPath: string;
Expand All @@ -89,6 +127,8 @@ export interface CodexCliSyncRun {
targetPath: string;
summary: CodexCliSyncSummary;
message?: string;
trigger: CodexCliSyncTrigger;
rollbackSnapshot: CodexCliSyncRollbackSnapshot | null;
}

type UpsertAction = "skipped" | "added" | "updated" | "unchanged";
Expand All @@ -107,6 +147,33 @@ interface ReconcileResult {
let lastCodexCliSyncRun: CodexCliSyncRun | null = null;
let lastHistoryLoadAttempted = false;

function normalizeCodexCliSyncRun(
run: CodexCliSyncRun | null,
): CodexCliSyncRun | null {
if (!run) return null;
return {
...run,
trigger: run.trigger ?? "automatic",
summary: { ...run.summary },
rollbackSnapshot: run.rollbackSnapshot ? { ...run.rollbackSnapshot } : null,
};
}

function cloneCodexCliSyncRun(
run: CodexCliSyncRun | null,
): CodexCliSyncRun | null {
const normalized = normalizeCodexCliSyncRun(run);
return normalized
? {
...normalized,
summary: { ...normalized.summary },
rollbackSnapshot: normalized.rollbackSnapshot
? { ...normalized.rollbackSnapshot }
: null,
}
: null;
}

function createEmptySyncSummary(): CodexCliSyncSummary {
return {
sourceAccountCount: 0,
Expand All @@ -120,8 +187,166 @@ function createEmptySyncSummary(): CodexCliSyncSummary {
};
}

async function captureRollbackSnapshot(): Promise<CodexCliSyncRollbackSnapshot | null> {
const snapshot: NamedBackupMetadata | null = await snapshotAccountStorage({
reason: "codex-cli-sync",
failurePolicy: "warn",
});
if (!snapshot) return null;
return { name: snapshot.name, path: snapshot.path };
}

async function findLatestManualCodexCliSyncRun(): Promise<CodexCliSyncRun | null> {
const history = await readSyncHistory({ kind: "codex-cli-sync" });
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Manual rollback lookup ignores the cached latest run, so rollback can become unavailable after a history-write failure.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-cli/sync.ts, line 200:

<comment>Manual rollback lookup ignores the cached latest run, so rollback can become unavailable after a history-write failure.</comment>

<file context>
@@ -120,8 +187,166 @@ function createEmptySyncSummary(): CodexCliSyncSummary {
+}
+
+async function findLatestManualCodexCliSyncRun(): Promise<CodexCliSyncRun | null> {
+	const history = await readSyncHistory({ kind: "codex-cli-sync" });
+	for (let i = history.length - 1; i >= 0; i -= 1) {
+		const entry = history[i];
</file context>
Suggested change
const history = await readSyncHistory({ kind: "codex-cli-sync" });
const cached = cloneCodexCliSyncRun(lastCodexCliSyncRun);
if (cached?.trigger === "manual") {
return cached;
}
const history = await readSyncHistory({ kind: "codex-cli-sync" });
Fix with Cubic

for (let i = history.length - 1; i >= 0; i -= 1) {
const entry = history[i];
if (!entry || entry.kind !== "codex-cli-sync") continue;
const normalized = normalizeCodexCliSyncRun(entry.run);
if (!normalized) continue;
if (normalized.trigger !== "manual") continue;
return normalized;
}
return null;
}

export async function rollbackLastCodexCliSync(): Promise<CodexCliSyncRollbackExecutionResult> {
const lastRun = await findLatestManualCodexCliSyncRun();
if (!lastRun) {
throw new Error(
"No manual Codex CLI sync run with a rollback snapshot is available to restore.",
);
}
const snapshot = lastRun.rollbackSnapshot;
if (!snapshot) {
throw new Error(
"Latest manual Codex CLI sync run did not record a rollback snapshot.",
);
}
if (!snapshot.name || snapshot.name.trim().length === 0) {
throw new Error(
"Rollback snapshot name is missing from the recorded manual sync run.",
);
}
if (!snapshot.path || snapshot.path.trim().length === 0) {
throw new Error(
"Rollback snapshot path is missing from the recorded manual sync run.",
);
}
if (!existsSync(snapshot.path)) {
throw new Error(`Rollback snapshot not found at ${snapshot.path}`);
}
const restoreResult = await restoreNamedBackup(snapshot.name);
Comment on lines +235 to +238
Copy link

@greptile-apps greptile-apps bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOCTOU race + path mismatch in rollbackLastCodexCliSync

existsSync(snapshot.path) checks one path, but restoreNamedBackup(snapshot.name) re-resolves the location internally via normalizeBackupName + resolveNamedBackupPath. those two paths can diverge, so the guard can pass while the actual restore target is missing.

additionally, on Windows, antivirus or a competing writer can delete/lock the file between the existsSync check and the async restoreNamedBackup call — a classic TOCTOU window. since restoreNamedBackup already throws with a descriptive error when the backup is absent or invalid (via assessNamedBackupRestore), the synchronous pre-check is redundant and introduces the race.

drop the existsSync guard and let restoreNamedBackup handle the missing-file case:

Suggested change
if (!existsSync(snapshot.path)) {
throw new Error(`Rollback snapshot not found at ${snapshot.path}`);
}
const restoreResult = await restoreNamedBackup(snapshot.name);
const restoreResult = await restoreNamedBackup(snapshot.name);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 235-238

Comment:
**TOCTOU race + path mismatch in `rollbackLastCodexCliSync`**

`existsSync(snapshot.path)` checks one path, but `restoreNamedBackup(snapshot.name)` re-resolves the location internally via `normalizeBackupName` + `resolveNamedBackupPath`. those two paths can diverge, so the guard can pass while the actual restore target is missing.

additionally, on Windows, antivirus or a competing writer can delete/lock the file between the `existsSync` check and the async `restoreNamedBackup` call — a classic TOCTOU window. since `restoreNamedBackup` already throws with a descriptive error when the backup is absent or invalid (via `assessNamedBackupRestore`), the synchronous pre-check is redundant and introduces the race.

drop the `existsSync` guard and let `restoreNamedBackup` handle the missing-file case:

```suggestion
	const restoreResult = await restoreNamedBackup(snapshot.name);
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex

Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: rollbackLastCodexCliSync calls merge-based backup restore, so it may not actually roll back to the pre-sync state.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-cli/sync.ts, line 238:

<comment>`rollbackLastCodexCliSync` calls merge-based backup restore, so it may not actually roll back to the pre-sync state.</comment>

<file context>
@@ -120,8 +187,166 @@ function createEmptySyncSummary(): CodexCliSyncSummary {
+	if (!existsSync(snapshot.path)) {
+		throw new Error(`Rollback snapshot not found at ${snapshot.path}`);
+	}
+	const restoreResult = await restoreNamedBackup(snapshot.name);
+	return {
+		snapshot,
</file context>
Suggested change
const restoreResult = await restoreNamedBackup(snapshot.name);
const raw = await fs.readFile(snapshot.path, "utf-8");
const parsed = JSON.parse(raw) as unknown;
const normalized = normalizeAccountStorage(parsed);
if (!normalized) {
throw new Error("Rollback snapshot is invalid or empty.");
}
await saveAccounts(normalized);
const restoreResult = {
imported: normalized.accounts.length,
total: normalized.accounts.length,
skipped: 0,
};
Fix with Cubic

return {
snapshot,
restore: restoreResult,
};
}
Comment on lines +212 to +243
Copy link

@greptile-apps greptile-apps bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two exported rollback functions use divergent restore strategies

rollbackLastCodexCliSync (this function) restores via restoreNamedBackup(snapshot.name) which calls importAccounts — a merge operation that folds the snapshot's accounts into the current live storage.

rollbackLatestCodexCliSync (line 315) restores via saveAccounts(resolvedPlan.storage) — a full overwrite of whatever is on disk.

the UI in settings-hub.ts uses rollbackLatestCodexCliSync (overwrite). tests for the "rolls back even with newer automatic runs" scenario use rollbackLastCodexCliSync (merge). the two strategies are not equivalent:

  • the merge path may retain token fields from current live accounts that weren't in the pre-sync snapshot, meaning stale or revoked tokens can survive a rollback.
  • the overwrite path is deterministic but bypasses the named-backup restore pipeline entirely.

one path should be authoritative. if rollbackLastCodexCliSync is the public API, the settings-hub should use it (with try/catch). if rollbackLatestCodexCliSync is preferred, rollbackLastCodexCliSync should delegate to it rather than calling restoreNamedBackup directly. the divergence is a token-safety risk and makes it impossible to reason about post-rollback state.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 212-243

Comment:
**Two exported rollback functions use divergent restore strategies**

`rollbackLastCodexCliSync` (this function) restores via `restoreNamedBackup(snapshot.name)` which calls `importAccounts` — a **merge** operation that folds the snapshot's accounts into the current live storage.

`rollbackLatestCodexCliSync` (line 315) restores via `saveAccounts(resolvedPlan.storage)` — a **full overwrite** of whatever is on disk.

the UI in `settings-hub.ts` uses `rollbackLatestCodexCliSync` (overwrite). tests for the "rolls back even with newer automatic runs" scenario use `rollbackLastCodexCliSync` (merge). the two strategies are not equivalent:

- the merge path may retain token fields from current live accounts that weren't in the pre-sync snapshot, meaning stale or revoked tokens can survive a rollback.
- the overwrite path is deterministic but bypasses the named-backup restore pipeline entirely.

one path should be authoritative. if `rollbackLastCodexCliSync` is the public API, the settings-hub should use it (with `try/catch`). if `rollbackLatestCodexCliSync` is preferred, `rollbackLastCodexCliSync` should delegate to it rather than calling `restoreNamedBackup` directly. the divergence is a token-safety risk and makes it impossible to reason about post-rollback state.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex


function formatRollbackPlanFromSnapshot(
snapshot: CodexCliSyncRollbackSnapshot | null,
): CodexCliSyncRollbackPlan {
if (!snapshot) {
return {
status: "unavailable",
reason:
"Latest manual Codex CLI sync did not capture a rollback checkpoint to restore.",
snapshot: null,
};
}

return {
status: "ready",
reason: "Rollback checkpoint is ready.",
snapshot,
};
}

async function loadRollbackSnapshot(
snapshot: CodexCliSyncRollbackSnapshot | null,
): Promise<CodexCliSyncRollbackPlan> {
if (!snapshot) return formatRollbackPlanFromSnapshot(snapshot);

try {
const raw = await fs.readFile(snapshot.path, "utf-8");
const parsed = JSON.parse(raw) as unknown;
const normalized = normalizeAccountStorage(parsed);
if (!normalized) {
return {
status: "unavailable",
reason: "Rollback checkpoint is invalid or empty.",
snapshot,
};
}
return {
status: "ready",
reason: `Rollback checkpoint ready (${normalized.accounts.length} account(s)).`,
snapshot,
accountCount: normalized.accounts.length,
storage: normalized,
};
} catch (error) {
const reason =
(error as NodeJS.ErrnoException).code === "ENOENT"
? `Rollback checkpoint is missing at ${snapshot.path}.`
: `Failed to read rollback checkpoint: ${
error instanceof Error ? error.message : String(error)
}`;
return {
status: "unavailable",
reason,
snapshot,
};
}
}

export async function getLatestCodexCliSyncRollbackPlan(): Promise<CodexCliSyncRollbackPlan> {
const lastManualRun = await findLatestManualCodexCliSyncRun();
if (!lastManualRun) {
return {
status: "unavailable",
reason:
"No manual Codex CLI sync with a rollback checkpoint is available.",
snapshot: null,
};
}
return loadRollbackSnapshot(lastManualRun.rollbackSnapshot);
}

export async function rollbackLatestCodexCliSync(
plan?: CodexCliSyncRollbackPlan,
): Promise<CodexCliSyncRollbackPlanResult> {
const resolvedPlan =
plan && plan.status === "ready"
? plan
: await getLatestCodexCliSyncRollbackPlan();
if (resolvedPlan.status !== "ready" || !resolvedPlan.storage) {
return {
status: "unavailable",
reason: resolvedPlan.reason,
snapshot: resolvedPlan.snapshot,
};
}

try {
await saveAccounts(resolvedPlan.storage);
return {
status: "restored",
reason: resolvedPlan.reason,
snapshot: resolvedPlan.snapshot,
accountCount:
resolvedPlan.accountCount ?? resolvedPlan.storage.accounts.length,
};
} catch (error) {
return {
status: "error",
reason: error instanceof Error ? error.message : String(error),
snapshot: resolvedPlan.snapshot,
};
}
}

async function setLastCodexCliSyncRun(run: CodexCliSyncRun): Promise<void> {
lastCodexCliSyncRun = run;
lastCodexCliSyncRun = normalizeCodexCliSyncRun(run);
try {
await appendSyncHistoryEntry({
kind: "codex-cli-sync",
Expand All @@ -136,32 +361,27 @@ async function setLastCodexCliSyncRun(run: CodexCliSyncRun): Promise<void> {
}

export function getLastCodexCliSyncRun(): CodexCliSyncRun | null {
if (lastCodexCliSyncRun) {
return {
...lastCodexCliSyncRun,
summary: { ...lastCodexCliSyncRun.summary },
};
}
const cached = cloneCodexCliSyncRun(lastCodexCliSyncRun);
if (cached) return cached;

if (!lastHistoryLoadAttempted) {
lastHistoryLoadAttempted = true;
const latest = readLatestSyncHistorySync();
const cloned = cloneSyncHistoryEntry(latest);
if (cloned?.kind === "codex-cli-sync") {
lastCodexCliSyncRun = cloned.run;
return {
...cloned.run,
summary: { ...cloned.run.summary },
};
lastCodexCliSyncRun = normalizeCodexCliSyncRun(cloned.run);
return cloneCodexCliSyncRun(lastCodexCliSyncRun);
}
}

return null;
}

export function __resetLastCodexCliSyncRunForTests(): void {
lastCodexCliSyncRun = null;
lastHistoryLoadAttempted = false;
export function __resetLastCodexCliSyncRunForTests(
run: CodexCliSyncRun | null = null,
): void {
lastCodexCliSyncRun = normalizeCodexCliSyncRun(run);
lastHistoryLoadAttempted = run !== null;
}

function buildIndexByAccountId(
Expand Down Expand Up @@ -553,9 +773,12 @@ export async function previewCodexCliSync(
*/
export async function syncAccountStorageFromCodexCli(
current: AccountStorageV3 | null,
options: { trigger?: CodexCliSyncTrigger } = {},
): Promise<{ storage: AccountStorageV3 | null; changed: boolean }> {
incrementCodexCliMetric("reconcileAttempts");
const targetPath = getStoragePath();
const trigger: CodexCliSyncTrigger = options.trigger ?? "automatic";
let rollbackSnapshot: CodexCliSyncRollbackSnapshot | null = null;
try {
const state = await loadCodexCliState();
if (!state) {
Expand All @@ -577,10 +800,16 @@ export async function syncAccountStorageFromCodexCli(
(process.env.CODEX_MULTI_AUTH_SYNC_CODEX_CLI ?? "").trim() === "0"
? "Codex CLI sync disabled by environment override."
: "No Codex CLI sync source was available.",
trigger,
rollbackSnapshot,
});
return { storage: current, changed: false };
}

if (trigger === "manual") {
rollbackSnapshot = await captureRollbackSnapshot();
}
Comment on lines +809 to +811
Copy link

@greptile-apps greptile-apps bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshot captured before reconcile result is known — wasted I/O on no-ops

captureRollbackSnapshot() is called here regardless of whether the upcoming reconcileCodexCliState() will produce any changes. for a manual sync that resolves as a no-op, a named backup file is written to disk and stored as the rollback checkpoint for that run. the user can then "roll back" to the exact state they already have, which is confusing. it also generates unnecessary disk writes and snapshot metadata on Windows, where file I/O under antivirus is slow and risky.

consider moving the snapshot capture to after reconcile, and only snapshotting when reconciled.changed === true:

const reconciled = reconcileCodexCliState(current, state);
if (trigger === "manual" && reconciled.changed) {
  rollbackSnapshot = await captureRollbackSnapshot();
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 809-811

Comment:
**Snapshot captured before reconcile result is known — wasted I/O on no-ops**

`captureRollbackSnapshot()` is called here regardless of whether the upcoming `reconcileCodexCliState()` will produce any changes. for a manual sync that resolves as a no-op, a named backup file is written to disk and stored as the rollback checkpoint for that run. the user can then "roll back" to the exact state they already have, which is confusing. it also generates unnecessary disk writes and snapshot metadata on Windows, where file I/O under antivirus is slow and risky.

consider moving the snapshot capture to after reconcile, and only snapshotting when `reconciled.changed === true`:

```ts
const reconciled = reconcileCodexCliState(current, state);
if (trigger === "manual" && reconciled.changed) {
  rollbackSnapshot = await captureRollbackSnapshot();
}
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex


const reconciled = reconcileCodexCliState(current, state);
const next = reconciled.next;
const changed = reconciled.changed;
Expand All @@ -593,6 +822,8 @@ export async function syncAccountStorageFromCodexCli(
sourcePath: state.path,
targetPath,
summary: reconciled.summary,
trigger,
rollbackSnapshot,
});
log.debug("Codex CLI reconcile completed", {
operation: "reconcile-storage",
Expand All @@ -613,6 +844,8 @@ export async function syncAccountStorageFromCodexCli(
sourcePath: state.path,
targetPath,
summary: reconciled.summary,
trigger,
rollbackSnapshot,
});
log.debug("Codex CLI reconcile completed", {
operation: "reconcile-storage",
Expand Down Expand Up @@ -640,6 +873,8 @@ export async function syncAccountStorageFromCodexCli(
targetAccountCountAfter: current?.accounts.length ?? 0,
},
message: error instanceof Error ? error.message : String(error),
trigger,
rollbackSnapshot,
});
log.warn("Codex CLI reconcile failed", {
operation: "reconcile-storage",
Expand Down
Loading