I was reading gemini-cli's extension update path for unrelated reasons and found that its rollback restores nothing. Not "restores the wrong thing" — restores an empty directory, every time, by construction.
gemini-cli is Google's terminal agent, Apache-2.0. The code is in packages/cli/src/config/extensions/update.ts
.
The shape is the one everybody writes:
const tempDir = await ExtensionStorage.createTmpDir();
try {
// load previous config, then install the new version over extension.path
await extensionManager.installOrUpdateExtension(/* ... */);
// dispatch UPDATED or UPDATED_NEEDS_RESTART
} catch (e) {
await copyExtension(tempDir, extension.path);
} finally {
await fs.promises.rm(tempDir, { recursive: true, force: true });
}
(trimmed; the error handling and the state dispatch have more in them than this)
Read the catch
on its own and it's obviously correct: something failed, put the old extension back from the temp dir. Read it against the line above it and the temp dir is a directory that createTmpDir
made and nobody ever wrote to. The update mutates extension.path
in place. Nothing copies the current install into tempDir
first. So the recovery path takes an empty directory and copies it over the half-updated extension, which is worse than the failure it was catching.
There was already a test for this. It passed.
expect(copyExtension).toHaveBeenCalledWith('/tmp/mock-dir', extension.path);
That assertion is true. It was always true. copyExtension
really is called, with the temp dir as source and the extension path as destination, exactly as the rollback intends. The call happened. The test verified the call happened.
What no assertion covered was whether the source had anything in it.
That's a specific and repeatable blind spot, and I don't think it's a mocking mistake so much as a consequence of how you write a test for a recovery path at all. You mock the filesystem because you don't want a real one. Once copyExtension
is a mock, the only observable thing left is the call itself: name, arguments, order. The contents of a directory stop existing as a concept inside the test. So you assert on what's still visible, and it feels like coverage, because a test named "restores on failure" is green and there is a real assertion under it.
The missing assertion is an ordering one. Not "was copyExtension
called with these arguments" but "was something copied into tempDir
before the update touched extension.path
". That one fails on the code as written, and it fails for the actual reason.
Copy the extension into tempDir
before any mutation, and carry a backedUp
flag so the rollback only restores when there is a real backup to restore:
if (backedUp) {
await copyExtension(tempDir, extension.path);
}
The flag isn't decoration. Without it, a backup that itself failed halfway leaves you back in the original bug, restoring a partial directory over an installation that was in better shape than what replaced it. Rollback should decline to run rather than run on nothing, and a boolean is the cheapest way to say that.
The tests change from asserting one call to asserting both operations in sequence, which is what makes the regression catchable later: if someone moves the backup to after the mutation, the argument assertion still passes and the ordering assertion doesn't.
That's PR #29166, open at the time of writing. The before-state is readable on main
.
I went looking through my own code for the same shape afterward. The general form: a mock turns a stateful operation into a call record, and the test then asserts on the call record, which is the part that was never in doubt. Backup and restore is the easiest place to hit it, because backup is the step with no visible output. Nothing downstream reads the temp dir except the failure path, and the failure path is the thing you're mocking.
The question that surfaces it: if I delete the line I believe is doing the work, does any test go red? For this rollback, deleting the backup line changes nothing, because the backup line was never there.