Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 40 additions & 1 deletion src/specify_cli/extensions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1397,12 +1397,51 @@ def install_from_directory(
backup_config_dir.unlink()
did_remove = self.remove(manifest.id)

# Rescue any config files left behind by a prior `remove --keep-config`.
# When an extension is removed with --keep-config, it is no longer in
# the registry but its config files remain in dest_dir. A subsequent
# plain (non-force) install would delete that directory unconditionally,
# silently discarding the preserved config. We read those files into
# memory now and write them back after copytree so the user's values
# always win over the packaged defaults.
stranded_configs: dict[str, tuple[bytes, int]] = {}
if dest_dir.exists() and not self.registry.is_installed(manifest.id):
for cfg_file in (
list(dest_dir.glob("*-config.yml"))
+ list(dest_dir.glob("*-config.local.yml"))
):
if cfg_file.is_file() and not cfg_file.is_symlink():
stranded_configs[cfg_file.name] = (
cfg_file.read_bytes(),
cfg_file.stat().st_mode,
)

# Install extension (dest_dir computed above during self-install guard)
if dest_dir.exists():
shutil.rmtree(dest_dir)

ignore_fn = self._load_extensionignore(source_dir)
shutil.copytree(source_dir, dest_dir, ignore=ignore_fn)
try:
shutil.copytree(source_dir, dest_dir, ignore=ignore_fn)
except BaseException:
# copytree failed — dest_dir may be absent or only partially
# created. Write the rescued configs back now so they are not
# permanently lost even though the install did not complete.
if stranded_configs:
dest_dir.mkdir(parents=True, exist_ok=True)
for filename, (content, mode) in stranded_configs.items():
target = dest_dir / filename
target.write_bytes(content)
target.chmod(mode & 0o660)
raise

# Restore stranded configs rescued before the rmtree above.
for filename, (content, mode) in stranded_configs.items():
Comment on lines +1438 to +1439

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in fix: add copytree rollback path and strengthen regression test with packaged default config. The copytree call is now wrapped in a try/except BaseException block. If it raises (disk full, permission error, etc.), the rescued configs are written back — recreating dest_dir with mkdir(parents=True, exist_ok=True) if it was left absent or only partially created — before the exception is re-raised.

Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.6, autonomous).

target = dest_dir / filename
target.write_bytes(content)
# Mask to only user/group read-write bits to avoid restoring
# setuid/setgid or world-writable permissions.
target.chmod(mode & 0o660)

# Register commands with AI agents
registered_commands = {}
Expand Down
63 changes: 63 additions & 0 deletions tests/test_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,69 @@ def test_install_force_config_preserved(self, extension_dir, project_dir):
assert new_config.exists()
assert new_config.read_text() == "test: config"

def test_reinstall_after_keep_config_preserves_config(
self, extension_dir, project_dir
):
"""Reinstalling after `remove --keep-config` must not overwrite preserved config."""
manager = ExtensionManager(project_dir)

# Add a packaged default config so the reinstall has a file to overwrite.
# Without the fix, the packaged default would silently win on reinstall.
packaged_config = extension_dir / "test-ext-config.yml"
packaged_config.write_text("model: default-model\nmax_iterations: 1\n")

# Install once (packaged default is copied into the installed directory)
manager.install_from_directory(
extension_dir, "0.1.0", register_commands=False
)

# Overwrite the installed config with user-customized values
ext_dir = project_dir / ".specify" / "extensions" / "test-ext"
config_file = ext_dir / "test-ext-config.yml"
config_file.write_text("model: custom-model\nmax_iterations: 99\n")

# Remove while preserving config
manager.remove("test-ext", keep_config=True)
assert not manager.registry.is_installed("test-ext")
assert config_file.exists()
assert "custom-model" in config_file.read_text()

# Plain reinstall (no --force) — packaged default is still present in
# extension_dir, so a naive implementation would overwrite the custom values.
manager.install_from_directory(
extension_dir, "0.1.0", register_commands=False
)

# Preserved config must survive the reinstall and beat the packaged default
assert config_file.exists()
assert "custom-model" in config_file.read_text()
assert "99" in config_file.read_text()
assert "default-model" not in config_file.read_text()

def test_reinstall_after_keep_config_preserves_local_config(
self, extension_dir, project_dir
):
"""Local config override files (*-config.local.yml) are also rescued on reinstall."""
manager = ExtensionManager(project_dir)

manager.install_from_directory(
extension_dir, "0.1.0", register_commands=False
)

ext_dir = project_dir / ".specify" / "extensions" / "test-ext"
local_cfg = ext_dir / "test-ext-config.local.yml"
local_cfg.write_text("local_override: true\n")

manager.remove("test-ext", keep_config=True)
assert local_cfg.exists()

manager.install_from_directory(
extension_dir, "0.1.0", register_commands=False
)

assert local_cfg.exists()
assert "local_override: true" in local_cfg.read_text()

def test_install_force_without_existing(self, extension_dir, project_dir):
"""Test force-install when extension is NOT already installed (works normally)."""
manager = ExtensionManager(project_dir)
Expand Down