Repository navigation
Conversation
…restores it updateExtension created a temp dir and, on failure, copied that temp dir back over the extension. Nothing ever copied the extension into the temp dir, so the temp dir was always empty and the rollback restored nothing, leaving the half-updated installation in place. Copy the current installation into the temp dir before the update runs, and only restore from it when that backup actually succeeded, so a failed backup cannot overwrite an intact extension with a partial copy. The existing rollback test asserted the restore call was made with the expected arguments, which held equally true when the temp dir was empty, so it passed against the bug. Assert the backup and restore calls in order instead, since a backup taken after the mutation is as empty as no backup at all. Both new assertions fail against the unfixed code. Fixes google-gemini#29033
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue in the extension update logic where rollbacks were ineffective because no backup was created prior to the update. By capturing the current state before any mutations occur and implementing a verification flag, the system now ensures that rollbacks only proceed when a valid backup is available, preventing the corruption of existing installations during failed updates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/S
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a backup mechanism before updating an extension, ensuring that a rollback is only attempted if the backup was successfully created. It also updates the test suite to verify the ordering of operations and adds a test case for failed backups. The reviewer suggested a critical improvement to completely remove the corrupted extension directory before restoring the backup to prevent a dirty state, and to wrap the rollback in a try-catch block to avoid shadowing the original error.
| if (backedUp) { | ||
| await copyExtension(tempDir, extension.path); | ||
| } |
There was a problem hiding this comment.
When rolling back a failed update, copying the backup directory (tempDir) directly over the existing extension.path using copyExtension (which uses fs.promises.cp) will not delete any new files that were successfully written by the failed update before it crashed. This can leave the extension directory in a corrupted, 'dirty' state containing a mix of old and new files.
To ensure a clean and reliable rollback, the corrupted extension.path directory should be completely removed before restoring the backup. Additionally, wrapping the rollback in a try-catch block ensures that if the rollback itself fails, the original installation error e is still thrown and not shadowed by the rollback error.
if (backedUp) {
try {
await fs.promises.rm(extension.path, { recursive: true, force: true });
await copyExtension(tempDir, extension.path);
} catch (rollbackError) {
debugLogger.error(
'Failed to rollback extension update: ' + getErrorMessage(rollbackError)
);
}
}
gemini-cli
Bot
added
the
area/extensions
|
The CLA has been signed. Recheck, please. |
|
@googlebot I signed it! |
gemini-cli
Bot
added
priority/p2
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
1 participant
Summary
updateExtensionnever backed up the extension before updating it. It created a temp dir, and on failure copied that temp dir back overextension.path. Nothing ever copied the extension into the temp dir, so the temp dir was always empty and the rollback restored nothing. A failed update left the half-updated installation in place.Details
packages/cli/src/config/extensions/update.ts. Two changes.The test change isn't garnish. There was already a rollback test at
update.test.ts:278and it passed against this bug, so the fix can regress silently without it. Its whole rollback assertion was:copyExtensionis mocked as a barevi.fn()andcreateTmpDiris stubbed to the string'/tmp/mock-dir'. So it asserted that the restore call was made with the expected arguments. That is equally true when the temp dir is empty. It checked the call, not the effect.The asymmetry was the tell: there was an assertion for the restore direction and none for the backup direction, and the missing call was exactly the unasserted one. This PR asserts both in order. Order matters, because a backup taken after the install has already mutated the directory is as empty as no backup at all, and an unordered pair wouldn't catch that. A second test covers the new guard.
Related Issues
Fixes #29033
How to Validate
15 passed here. For the negative control, revert
update.tsalone and re-run: both new tests fail, the other 13 still pass.Pre-Merge Checklist
Validated on Windows via
npm runonly. I couldn't run the fullnpm run preflight:npm cifails on this machine building the nativetree-sitter-bashdependency, so I installed with--ignore-scriptsand ran the single test file plusprettier --checkon both changed files. Worth a CI run on the other platforms.