[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config#3449
Open
github-actions[bot] wants to merge 33 commits into
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}
Address three review findings on the reinstall config-rescue path: - A complete .rescue-complete marker proves only that staging finished, not that dest_dir was modified. A crash after staging sync but before the rmtree leaves the live kept config intact; if the user edits it before retrying, preferring the staged bytes silently overwrote the newer config. The two copies are indistinguishable in provenance from disk, so detect divergence between a complete staging copy and the live config and abort (preserving both) instead of unconditionally choosing staging. - The staging directory embedded the full extension ID in one path component. Extension IDs are length-unbounded, so a valid long ID could install at dest_dir yet fail every reinstall-after-keep-config with ENAMETOOLONG. Derive the staging component from a fixed-length hash via a new _rescue_staging_dir() helper. - The stranded-config restore used the full config filename as a NamedTemporaryFile prefix; a name already near the component limit plus the random suffix raised ENAMETOOLONG. Use a short fixed prefix. Updates the retry regression test to the new divergence semantics and adds conflict-abort, long-ID, and fixed-prefix coverage. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
| " then reinstall." | ||
| ) | ||
| raise ValidationError(" ".join(msg_parts)) | ||
| elif dest_dir.exists() and not self.registry.is_installed(manifest.id): |
Contributor
There was a problem hiding this comment.
Fixed in the two commits just pushed (short hashes in the latest push).
remove(..., keep_config=True) now writes a .keep-config provenance marker (empty file, presence-only sentinel) into the extension directory after stranding the config files. install_from_directory guards the rescue elif with a new _has_keep_config_marker() helper that checks is_file() and not is_symlink(), so only directories explicitly left by --keep-config enter that path — a partially-failed install (no marker, no registry entry) no longer has its packaged defaults treated as user-preserved data on a retry.
A regression test (test_failed_install_without_keep_config_does_not_rescue_defaults) pins this: it simulates a post-copytree registration failure, updates the packaged default, retries, and asserts the new default wins rather than the old stale one.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous).
…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)
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