fix: preserve .vscode/settings.json and script +x bit on integration upgrade#3020
Conversation
…upgrade During 'specify integration upgrade', Phase 2 stale-cleanup removes files present in the old manifest but absent from the new one. Copilot's setup() merges into an existing .vscode/settings.json and stops tracking it, so the file was being deleted on upgrade (destroying user settings). Add a stale_cleanup_exclusions() hook that integrations use to protect such conditionally-tracked merge targets. Also restore the executable bit on shared .sh scripts after the managed-refresh step on POSIX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes two upgrade regressions in specify integration upgrade: (1) preventing stale-cleanup from deleting conditionally-untracked merge targets like .vscode/settings.json (Copilot), and (2) ensuring shared POSIX .sh scripts retain executable permissions after a managed refresh.
Changes:
- Add
IntegrationBase.stale_cleanup_exclusions()hook (default empty) so integrations can protect specific project-relative paths from Phase 2 stale cleanup. - Implement Copilot’s override to exclude
.vscode/settings.jsonfrom stale deletion. - During
integration upgrade, re-apply executable bits to shared.shscripts on POSIX after the managed-refresh step; add regression tests for both issues.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/integrations/base.py |
Adds the stale_cleanup_exclusions() hook to the integration base class. |
src/specify_cli/integrations/copilot/__init__.py |
Excludes .vscode/settings.json from upgrade stale cleanup for Copilot. |
src/specify_cli/integrations/_migrate_commands.py |
Applies exclusions during stale cleanup and restores +x after managed shared-infra refresh (POSIX). |
tests/integrations/test_integration_subcommand.py |
Adds regression tests for settings preservation and script execute-bit restoration. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
- Normalize stale_cleanup_exclusions() to POSIX before subtracting from manifest keys, so exclusions built with os.path.join / backslashes still match on Windows. - Strengthen test_upgrade_preserves_existing_vscode_settings to add a user-defined key and assert it survives the upgrade (via --force, exercising the merge + stale-cleanup path) instead of the brittle after == before check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review — both points addressed in 255cb8c. POSIX normalization ( Brittle test assertion ( Full Posted on behalf of @BenBtg by GitHub Copilot (model: Claude Opus 4.8), reviewed by @BenBtg. |
|
Thank you! |
The bug
During
specify integration upgrade, Phase 2 stale-cleanup deletes files that were recorded in the prior manifest but are absent from the new one. The Copilot integration'ssetup()merges into.vscode/settings.jsonwhen it already exists and intentionally stops tracking it in the manifest. As a result, on upgrade that file was treated as stale and deleted — destroying the user's VS Code settings.Separately, shared
.shscripts could lose their executable (+x) bit during the managed-refresh step.The fix
stale_cleanup_exclusions()hook onIntegrationBase(returns an empty set by default) so integrations can protect conditionally-tracked merge targets from stale-cleanup..vscode/settings.json.integration upgradestale-cleanup in_migrate_commands.py, and restore the executable bit on shared.shscripts after the managed-refresh step (POSIX only).Regression tests
Two new tests in
tests/integrations/test_integration_subcommand.py:test_upgrade_preserves_existing_vscode_settings— verifies an existing.vscode/settings.jsonsurvives an upgrade.test_upgrade_restores_executable_bit_on_shared_scripts— verifies shared.shscripts keep their+xbit after upgrade.Full
tests/integrationssuite passes (2382 passed, 1 skipped).Notes
Split out of #3001 to keep that PR scoped to the
/speckit.convergecommand.🤖 Authored with GitHub Copilot on behalf of @BenBtg, who reviewed the change.