From a30b8cf85f130c2038945fff7a7fca9185a62151 Mon Sep 17 00:00:00 2001 From: Ben Buttigieg <70525+BenBtg@users.noreply.github.com> Date: Wed, 17 Jun 2026 15:40:25 +0100 Subject: [PATCH 1/2] fix: preserve .vscode/settings.json and script +x bit on integration 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> --- .../integrations/_migrate_commands.py | 8 +++- src/specify_cli/integrations/base.py | 12 +++++ .../integrations/copilot/__init__.py | 11 +++++ .../test_integration_subcommand.py | 45 +++++++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/integrations/_migrate_commands.py b/src/specify_cli/integrations/_migrate_commands.py index 01cb51d687..368e1d91cf 100644 --- a/src/specify_cli/integrations/_migrate_commands.py +++ b/src/specify_cli/integrations/_migrate_commands.py @@ -461,6 +461,9 @@ def integration_upgrade( raise _SharedTemplateRefreshError( f"Failed to refresh shared infrastructure for '{key}': {exc}" ) from exc + if os.name != "nt": + from .. import ensure_executable_scripts + ensure_executable_scripts(project_root) new_manifest.save() _write_integration_json(project_root, installed_key, installed_keys, settings) if installed_key == key: @@ -478,7 +481,10 @@ def integration_upgrade( # Phase 2: Remove stale files from old manifest that are not in the new one old_files = old_manifest.files new_files = new_manifest.files - stale_keys = set(old_files) - set(new_files) + # Exclude integration-declared paths that use conditional manifest tracking + # (e.g. merge targets like .vscode/settings.json) so they are never deleted + # as "stale" while still being actively managed. + stale_keys = (set(old_files) - set(new_files)) - integration.stale_cleanup_exclusions() if stale_keys: stale_manifest = IntegrationManifest(key, project_root, version="stale-cleanup") stale_manifest._files = {k: old_files[k] for k in stale_keys} diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index def5ad20ba..e236d30ba2 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -393,6 +393,18 @@ def command_filename(self, template_name: str) -> str: """ return f"speckit.{template_name}.md" + def stale_cleanup_exclusions(self) -> set[str]: + """Return project-relative paths that upgrade must never stale-delete. + + During ``integration upgrade``, files recorded in a previous manifest + but absent from the freshly written one are treated as stale and + removed. Conditionally-tracked files (e.g. a settings file that the + integration merges into when it already exists, and therefore stops + tracking) would otherwise be deleted even though they are still + managed. Subclasses list such paths here to protect them. + """ + return set() + def commands_dest(self, project_root: Path) -> Path: """Return the absolute path to the commands output directory. diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index 6e3daeefeb..71b6d6919d 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -282,6 +282,17 @@ def command_filename(self, template_name: str) -> str: """Copilot commands use ``.agent.md`` extension.""" return f"speckit.{template_name}.agent.md" + def stale_cleanup_exclusions(self) -> set[str]: + """Protect ``.vscode/settings.json`` from upgrade stale-deletion. + + ``setup()`` records this file in the manifest only when it creates it; + when it already exists the file is merged and intentionally left + untracked. On upgrade the untracked-but-existing file would otherwise + be flagged stale and deleted, destroying user settings (and the file + the integration still manages). + """ + return {".vscode/settings.json"} + def post_process_skill_content(self, content: str) -> str: """Inject shared hook guidance into Copilot skill content. diff --git a/tests/integrations/test_integration_subcommand.py b/tests/integrations/test_integration_subcommand.py index b62f06e821..2f951b2228 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -2272,6 +2272,51 @@ def test_upgrade_migrates_opencode_legacy_dir(self, tmp_path): f"found: {[f.name for f in core_remaining]}" ) + def test_upgrade_preserves_existing_vscode_settings(self, tmp_path): + """Regression: copilot upgrade must not stale-delete .vscode/settings.json. + + On init the file is created and recorded in the manifest. On upgrade, + setup() merges into the now-existing file and intentionally stops + tracking it, so without ``stale_cleanup_exclusions()`` the Phase 2 + stale cleanup would delete it (destroying the user's settings). + """ + project = _init_project(tmp_path, "copilot") + settings = project / ".vscode" / "settings.json" + assert settings.is_file(), "init should create .vscode/settings.json" + before = json.loads(settings.read_text(encoding="utf-8")) + assert before, "settings.json should contain managed defaults" + + result = _run_in_project(project, [ + "integration", "upgrade", "copilot", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + assert settings.is_file(), ".vscode/settings.json must survive upgrade" + after = json.loads(settings.read_text(encoding="utf-8")) + assert after == before, "managed settings must be intact after upgrade" + + def test_upgrade_restores_executable_bit_on_shared_scripts(self, tmp_path): + """Regression: scripts refreshed by the managed-refresh step stay +x.""" + if os.name == "nt": + pytest.skip("POSIX execute bits are not meaningful on Windows") + project = _init_project(tmp_path, "copilot") + script = project / ".specify" / "scripts" / "bash" / "check-prerequisites.sh" + assert script.is_file() + # Simulate a perms-losing install (e.g. wheel extraction dropping +x). + script.chmod(0o644) + assert not (script.stat().st_mode & 0o111) + + result = _run_in_project(project, [ + "integration", "upgrade", "copilot", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + assert script.stat().st_mode & 0o111, ( + "shared .sh scripts must be executable after upgrade" + ) + # ── Full lifecycle ─────────────────────────────────────────────────── From 255cb8c68d1d9983c4c8c07da623f3011b6078a4 Mon Sep 17 00:00:00 2001 From: Ben Buttigieg <70525+BenBtg@users.noreply.github.com> Date: Wed, 17 Jun 2026 19:51:00 +0100 Subject: [PATCH 2/2] fix: address review on stale-cleanup fix - 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> --- src/specify_cli/integrations/_migrate_commands.py | 8 ++++++-- tests/integrations/test_integration_subcommand.py | 11 +++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/integrations/_migrate_commands.py b/src/specify_cli/integrations/_migrate_commands.py index 368e1d91cf..06c51bd1ec 100644 --- a/src/specify_cli/integrations/_migrate_commands.py +++ b/src/specify_cli/integrations/_migrate_commands.py @@ -2,6 +2,7 @@ from __future__ import annotations import os +from pathlib import PurePath import typer @@ -483,8 +484,11 @@ def integration_upgrade( new_files = new_manifest.files # Exclude integration-declared paths that use conditional manifest tracking # (e.g. merge targets like .vscode/settings.json) so they are never deleted - # as "stale" while still being actively managed. - stale_keys = (set(old_files) - set(new_files)) - integration.stale_cleanup_exclusions() + # as "stale" while still being actively managed. Manifest keys are stored + # in POSIX form, so normalize the exclusions the same way before subtracting + # (an integration may build paths with os.path.join / backslashes). + exclusions = {PurePath(p).as_posix() for p in integration.stale_cleanup_exclusions()} + stale_keys = (set(old_files) - set(new_files)) - exclusions if stale_keys: stale_manifest = IntegrationManifest(key, project_root, version="stale-cleanup") stale_manifest._files = {k: old_files[k] for k in stale_keys} diff --git a/tests/integrations/test_integration_subcommand.py b/tests/integrations/test_integration_subcommand.py index 2f951b2228..985d8bce36 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -2286,15 +2286,22 @@ def test_upgrade_preserves_existing_vscode_settings(self, tmp_path): before = json.loads(settings.read_text(encoding="utf-8")) assert before, "settings.json should contain managed defaults" + # Simulate a user editing their settings: add a custom key that the + # integration does not manage. It must survive the upgrade. + before["editor.fontSize"] = 17 + settings.write_text(json.dumps(before), encoding="utf-8") + result = _run_in_project(project, [ "integration", "upgrade", "copilot", - "--script", "sh", + "--script", "sh", "--force", ]) assert result.exit_code == 0, result.output assert settings.is_file(), ".vscode/settings.json must survive upgrade" after = json.loads(settings.read_text(encoding="utf-8")) - assert after == before, "managed settings must be intact after upgrade" + assert after.get("editor.fontSize") == 17, ( + "user-defined settings must be preserved after upgrade" + ) def test_upgrade_restores_executable_bit_on_shared_scripts(self, tmp_path): """Regression: scripts refreshed by the managed-refresh step stay +x."""