[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config#3449
Merged
mnriem merged 35 commits intoJul 21, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes extension config loss when reinstalling after remove --keep-config.
Changes:
- Rescues and restores preserved extension configuration files.
- Adds regression tests for standard and local configs.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Adds preserved-config rescue and restoration. |
tests/test_extensions.py |
Adds reinstall config-preservation tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous)
Assisted-by: GitHub Copilot (model: GPT-5.6-Sol, autonomous)
…ew feedback - _recognized_config_names() now accepts follow_symlinks=False for live dir so symlinked *-config.yml entries are detected and treated as conflicts rather than being silently deleted by rmtree. - Add explanatory comment to bare 'except OSError: pass' in _restore_stranded_config_file's finally block. - Resolve CodeQL dual-import style: use 'from specify_cli import extensions as _ext_module' instead of 'import specify_cli.extensions as _ext_module'. Assisted-by: GitHub Copilot (model: claude-sonnet-4, autonomous)
Add test_staging_failure_aborts_before_dest_dir_removal covering three failure modes (mkdir, os.open/O_CREAT, fsync with EIO) in the rescue staging block. Each parametrized case verifies: - the install aborts before dest_dir is removed - the preserved config bytes remain authoritative - any partial staging is cleaned up and not left as complete - the extension stays unregistered Addresses review feedback on PRRT_kwDOPiFCnc6R351t. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)
Exercises the retry-from-staging branch (if staging_is_complete at line 1505 of extensions/__init__.py) in a scenario where the live config is absent — simulating a power loss that interrupted the rollback before it could write the config back. When the live copy is gone, the live-dir fallback (elif dest_dir.exists()) finds no stranded configs and the packaged default would be kept. Only the staging-complete branch can restore the original bytes and mode. This proves staging (not the fallback) is used on retry. Addresses review feedback on PRRT_kwDOPiFCnc6SAL3L. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)
… fix live-only conflict message
Thread 64: Remove os.fchmod/chmod from staged files to avoid Windows
read-only attribute that prevents shutil.rmtree from cleaning up.
Original permission bits are now written to a .rescue-modes.json sidecar
in the staging dir and reloaded during retry, with a fall-back to the
staged file's own mode for backwards-compat with pre-sidecar staging dirs.
Thread 65: Split the ValidationError message for staging-vs-live conflicts
into two accurate cases: files that diverged between both locations
("Both copies have been preserved") and live-only files that have no
backup counterpart, which previously incorrectly claimed "Both copies
have been preserved" and offered a restore instruction that was impossible.
Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…partially-failed installs When `remove --keep-config` strands config files, write a `.keep-config` marker into the extension directory. `install_from_directory` now only enters the rescue path when that marker is present, preventing a partially- failed install (which also leaves dest_dir with no registry entry but no marker) from having its packaged default configs treated as user-preserved data on a retry from an updated package. Refs: #3449 (comment) Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)
…ontent choice Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
src/specify_cli/extensions/init.py:1565
- This only detects live-only names. If execution stops after publishing staging but before
rmtree, the original directory and marker remain; a user who then deletes one kept config produces a staged-only name. The retry does not flag it and later restores the deleted file, silently undoing the user's change. Detect staged-only entries when the live keep-config marker shows the destructive phase had not begun, and treat them as a conflict like byte/mode edits.
live_only = live_names - staged_names
conflicting.update(live_only)
src/specify_cli/extensions/init.py:773
- A regular file named
.keep-configdoes not prove it was written byremove --keep-config:copytreealso permits an extension package to ship this exact file. If such an install fails after copying but before registry registration, its retry is misclassified and stale packaged configs are rescued over newer defaults—the case this provenance marker is intended to prevent. Store provenance outside the copied extension tree or reserve/reject this filename in extension sources.
marker = directory / ".keep-config"
return marker.is_file() and not marker.is_symlink()
src/specify_cli/extensions/init.py:1497
- The PR description still says this is an in-memory rescue with two tests and only the extension installer/test files changed, but the current implementation adds durable on-disk staging, provenance markers, crash/conflict semantics, many additional tests, and two unrelated file changes. Update the description, risk analysis, and verification scope so reviewers can assess the behavior actually proposed.
# memory and also write a durable staging copy outside dest_dir so
# that a partial rmtree, failed copytree, or partial restore cannot
# permanently discard the user's original bytes on a retry. The
# staging dir is removed only after every config has been successfully
# restored.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
…s sidecar shape Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous)
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/specify_cli/extensions/init.py:1713
- The marker is not necessarily installation-owned:
copytreealso copies a source package's top-level.keep-config(the ignore loader only reserves.extensionignore). If that install then fails after copying but beforeregistry.add, the retry enters this branch and treats the package's default configs as user-preserved data, defeating the provenance check. Treat.keep-configas a reserved installer file—reject or always exclude it from source copies—and only create it fromremove(..., keep_config=True).
self._has_keep_config_marker(dest_dir)
src/specify_cli/extensions/init.py:794
- A config-only directory does not prove a legacy
--keep-configremoval.shutil.copytreecan leave exactly this layout when a config copy succeeds butextension.ymland other payload copies fail; a later retry from an updated package then rescues the old packaged config and overwrites the new default. Avoid automatically treating arbitrary unmarked config-only directories as legacy leftovers; legacy migration needs stronger provenance or an explicit user recovery path.
if entry.name.endswith(("-config.yml", "-config.local.yml")) and (
entry.is_file() or entry.is_symlink()
):
has_config = True
continue
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Medium
1 task
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.
Bug fix — reinstall-overwrites-kept-config
Proposed fix for issue #3427, applying the remediation from the bug assessment.
Verdict: Valid · Severity: medium
Summary
When
specify extension remove <ext> --keep-configis used, the extension is unregistered but its*-config.ymlfiles survive in the extension directory. A subsequent plainspecify extension add <ext>unconditionally deleted that directory before copying the fresh extension in, silently discarding the preserved config. The fix rescues those stranded config files into memory before thermtreeand writes them back aftercopytree, so user-customized values always win over the packaged defaults.Changes
src/specify_cli/extensions/__init__.pyrmtree(dest_dir), collect any*-config.yml/*-config.local.ymlfiles from an unregistereddest_dirinto memory; restore them aftercopytreetests/test_extensions.pytest_reinstall_after_keep_config_preserves_configandtest_reinstall_after_keep_config_preserves_local_configTests Added or Updated
tests/test_extensions.py::TestInstallFromDirectory::test_reinstall_after_keep_config_preserves_config— pins that a customized*-config.ymlsurvives aremove --keep-config→ plain reinstall cycletests/test_extensions.py::TestInstallFromDirectory::test_reinstall_after_keep_config_preserves_local_config— same for*-config.local.ymloverride filesLocal Verification
--forcereinstall path, applying it to the previously-unhandled "unregistered but config-bearing directory" case.Deviations from Assessment
None. The implementation follows the preferred remediation exactly as described.
Risks & Review Notes
not self.registry.is_installed(manifest.id)guard ensures we only rescue configs when the extension is genuinely unregistered, avoiding picking up stale files from a different install.shutil.copytree, so packaged defaults are always superseded by the user's values — no risk of defaults silently winning.install_from_zip()delegates toinstall_from_directory(), so it is covered without additional changes.install_from_directory().Refs #3427 · cc
@grafvonbremove --keep-config#3427