Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Rollback cleanup can delete pre-existing files, follow symlinks outside the project, and leave partially registered outputs behind instead of safely restoring only this attempt’s changes.
3 open issues
What changed in this PR
Extends failed extension-install rollback to native agent outputs, addressing #5379 alongside the existing generic-route cleanup.
Changes:
- Adds native rollback for command and skill outputs, hooks, and registry entries.
- Shares package cleanup between native and generic routes while preserving rescued user configuration.
- Adds three regression tests covering hook failure, registry-write failure, and kept-config preservation during a failed force reinstall.
| File | Description |
|---|---|
tests/specify_cli/extensions/test_command_add.py |
Adds native rollback and configuration-preservation regression tests. |
src/specify_cli/extensions/__init__.py |
Adds native rollback and extracts shared package cleanup. |
Review effort: Balanced (auto)
Comment on lines
+3077
to
+3079
| CommandRegistrar( | ||
| self.project_root, include_generic=False | ||
| ).unregister_commands(safe_commands, self.project_root) |
| self.project_root, include_generic=False | ||
| ).unregister_commands(safe_commands, self.project_root) | ||
| self._unregister_extension_skills(registered_skills, manifest.id) | ||
| rollback_package_dir() |
Comment on lines
+3204
to
+3206
| try: | ||
| with changing_file(dest_dir, removal=True): | ||
| rollback_native_registration() |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5379
What broke: when
extension addfailed after command rendering (hook registration or the final registry write), the native SKILL.md outputs it already wrote were left behind while the extension stayed uninstalled. Only the generic route had rollback.What changed:
install_from_directorynow rolls back the native route too. It unregisters the command and skill outputs the attempt wrote, removes the registry entry and hooks if they got that far, and removes the installed package while preserving any kept user config. The package cleanup is now shared with the generic rollback.How verified: three new regression tests in tests/specify_cli/extensions/test_command_add.py, all fail before the fix and pass after: hook failure, registry-write failure, and a failed --force reinstall preserving kept config. Ran the extension, generic, and Claude integration suites (379 passed) and ruff check clean.