Skip to content

Commit 664dfbe

Browse files
marcelsafinCopilot
andcommitted
Retire alias command groups on toggle; scope preset cleanup to switched-away agent (#2948)
Fixes three current Copilot review findings on HEAD d0d152e: 1. Command->skills toggle cleanup only matched a stale command's own name against the returned replacement skill name. Aliases (CommandRegistrar tracks and returns primary + alias names flattened into one list) never have their own skill rendered -- only the primary command's skill is rendered -- so an alias's name could never match, leaving its command artifact and tracking behind forever even after the primary's replacement landed. Fixed identically in both presets (register_enabled_presets_for_agent) and extensions (register_enabled_extensions_for_agent): build a primary->alias mapping from the manifest, group stale names by primary, and retire/keep the whole group together based solely on whether the primary's skill replacement actually landed. 2. `integration switch` to a not-yet-installed target unregistered the old agent's extension artifacts but had no preset equivalent, so a preset's command overrides (including custom preset commands) and skill mirrors for the deactivated agent lingered as orphans. Added `PresetManager.unregister_agent_artifacts()`, mirroring `ExtensionManager.unregister_agent_artifacts()`: scoped strictly to the given agent, migrates a legacy flat-list `registered_skills` entry via existing on-disk provenance inference before removing anything (so other agents' real ownership is preserved rather than guessed or dropped), and guards against double-processing an artifact through both the commands and skills paths for native SKILL.md agents. Wired via a new `_unregister_presets_for_agent()` helper into the integration switch command's existing old-agent cleanup phase. Added red-first regression tests: - tests/test_presets.py: alias-group retire/keep/partial-multi-group tests for the command->skills toggle; unregister_agent_artifacts scoping tests for commands and legacy-list skill provenance. - tests/test_extension_skills.py: alias-group retire/keep tests for the extension command->skills toggle. - tests/integrations/test_integration_subcommand.py: end-to-end switch test proving a preset's custom command override is cleaned up when switching to a not-yet-installed integration, with tracking updated correctly and the new agent's registration unaffected. All new tests confirmed red (AttributeError / orphaned file assertions) before the fix and green after. Full suite: 3980 passed, 109 skipped. ruff check clean on all changed files. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent d0d152e commit 664dfbe

7 files changed

Lines changed: 841 additions & 4 deletions

File tree

src/specify_cli/extensions/__init__.py

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2158,10 +2158,46 @@ def register_enabled_extensions_for_agent(self, agent_name: str) -> None:
21582158
# would leave neither artifact (#2948).
21592159
if deferred_stale_commands:
21602160
replaced_skill_names = set(registered_skills or [])
2161+
# Commands may carry aliases (CommandRegistrar.
2162+
# register_commands_for_agent() tracks and
2163+
# returns primary + alias names flattened
2164+
# together into one list), but
2165+
# _register_extension_skills() only ever
2166+
# renders/returns the *primary* command name's
2167+
# skill — running an alias's own name through
2168+
# _skill_name_from_command() never matches
2169+
# anything real, so an alias would stay
2170+
# tracked/on-disk forever even after its
2171+
# primary's skill replacement landed. Map each
2172+
# stale name back to its manifest command's
2173+
# primary so the whole primary+alias group is
2174+
# retired or kept together, based solely on
2175+
# whether the *primary*'s skill replacement
2176+
# actually landed (#2948).
2177+
alias_to_primary: Dict[str, str] = {}
2178+
for cmd_info in manifest.commands:
2179+
primary_name = cmd_info.get("name")
2180+
if not isinstance(primary_name, str):
2181+
continue
2182+
for alias in cmd_info.get("aliases", []) or []:
2183+
if isinstance(alias, str):
2184+
alias_to_primary[alias] = primary_name
2185+
2186+
group_fully_replaced: Dict[str, bool] = {}
2187+
for cmd_name in deferred_stale_commands:
2188+
primary_name = alias_to_primary.get(cmd_name, cmd_name)
2189+
if primary_name in group_fully_replaced:
2190+
continue
2191+
group_fully_replaced[primary_name] = (
2192+
HookExecutor._skill_name_from_command(primary_name)
2193+
in replaced_skill_names
2194+
)
2195+
21612196
fully_replaced = [
21622197
cmd_name for cmd_name in deferred_stale_commands
2163-
if HookExecutor._skill_name_from_command(cmd_name)
2164-
in replaced_skill_names
2198+
if group_fully_replaced.get(
2199+
alias_to_primary.get(cmd_name, cmd_name), False
2200+
)
21652201
]
21662202
if fully_replaced:
21672203
registrar.unregister_commands(

src/specify_cli/integrations/_helpers.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,39 @@ def _register_presets_for_agent(
451451
)
452452

453453

454+
def _unregister_presets_for_agent(
455+
project_root: Path,
456+
agent_key: str,
457+
*,
458+
continuing: str,
459+
) -> None:
460+
"""Best-effort removal of ``agent_key``'s preset command/skill artifacts.
461+
462+
Mirrors ``_unregister_extensions_for_agent``: used by ``switch`` when
463+
uninstalling the previous integration so its preset command overrides
464+
and skill mirrors don't linger as orphans in the old agent's directory
465+
once a different (possibly not-yet-installed) integration becomes
466+
active (#2948).
467+
468+
Best-effort: never aborts the surrounding integration operation.
469+
"""
470+
try:
471+
from ..presets import PresetManager
472+
473+
preset_mgr = PresetManager(project_root)
474+
preset_mgr.unregister_agent_artifacts(agent_key)
475+
except Exception as preset_err:
476+
from .. import _print_cli_warning
477+
478+
_print_cli_warning(
479+
"clean up preset artifacts for",
480+
"integration",
481+
agent_key,
482+
preset_err,
483+
continuing=continuing,
484+
)
485+
486+
454487
# ---------------------------------------------------------------------------
455488
# CLI formatting helpers (re-exported from _commands.py)
456489
# ---------------------------------------------------------------------------

src/specify_cli/integrations/_migrate_commands.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
_set_default_integration,
3737
_set_default_integration_or_exit,
3838
_unregister_extensions_for_agent,
39+
_unregister_presets_for_agent,
3940
_update_init_options_for_integration,
4041
_write_integration_json,
4142
)
@@ -196,6 +197,19 @@ def integration_switch(
196197
continuing="Continuing with integration switch; old extension artifacts may need manual cleanup.",
197198
)
198199

200+
# Unregister preset commands/skills for the old agent for the same
201+
# reason: without this, a preset's command overrides (including
202+
# custom preset commands) and skill mirrors rendered for
203+
# installed_key would remain orphaned in its directory once a
204+
# different, possibly not-yet-installed integration becomes active
205+
# (#2948). Scoped strictly to installed_key; other agents' files,
206+
# tracking, and the preset packs themselves are untouched.
207+
_unregister_presets_for_agent(
208+
project_root,
209+
installed_key,
210+
continuing="Continuing with integration switch; old preset artifacts may need manual cleanup.",
211+
)
212+
199213
# Clear metadata so a failed Phase 2 doesn't leave stale references
200214
installed_keys = [installed for installed in installed_keys if installed != installed_key]
201215
_clear_init_options_for_integration(project_root, installed_key)

src/specify_cli/presets/__init__.py

Lines changed: 136 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -902,11 +902,46 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None:
902902
# unreplaced stays tracked and on disk (#2948).
903903
if stale_command_names:
904904
replaced_skill_names = set(registered_skills.get(agent_name) or [])
905+
# Commands may carry aliases (CommandRegistrar.register_
906+
# commands() tracks and returns primary + alias names
907+
# flattened together into one list), but _register_
908+
# skills() only ever renders/returns the *primary*
909+
# command name's skill — running an alias's own name
910+
# through _skill_names_for_command() never matches
911+
# anything real, so an alias would stay tracked/on-disk
912+
# forever even after its primary's skill replacement
913+
# landed. Map each stale name back to its template's
914+
# primary via the manifest so the whole primary+alias
915+
# group is retired or kept together, based solely on
916+
# whether the *primary*'s skill replacement actually
917+
# landed (#2948).
918+
alias_to_primary: Dict[str, str] = {}
919+
for tmpl in manifest.templates:
920+
if tmpl.get("type") != "command":
921+
continue
922+
primary_name = tmpl.get("name")
923+
if not isinstance(primary_name, str):
924+
continue
925+
for alias in tmpl.get("aliases", []):
926+
if isinstance(alias, str):
927+
alias_to_primary[alias] = primary_name
928+
929+
group_fully_replaced: Dict[str, bool] = {}
930+
for cmd_name in stale_command_names:
931+
primary_name = alias_to_primary.get(cmd_name, cmd_name)
932+
if primary_name in group_fully_replaced:
933+
continue
934+
modern_name, legacy_name = self._skill_names_for_command(primary_name)
935+
group_fully_replaced[primary_name] = (
936+
modern_name in replaced_skill_names
937+
or legacy_name in replaced_skill_names
938+
)
939+
905940
fully_replaced = []
906941
remaining_stale = []
907942
for cmd_name in stale_command_names:
908-
modern_name, legacy_name = self._skill_names_for_command(cmd_name)
909-
if modern_name in replaced_skill_names or legacy_name in replaced_skill_names:
943+
primary_name = alias_to_primary.get(cmd_name, cmd_name)
944+
if group_fully_replaced.get(primary_name, False):
910945
fully_replaced.append(cmd_name)
911946
else:
912947
remaining_stale.append(cmd_name)
@@ -952,6 +987,105 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None:
952987
stacklevel=2,
953988
)
954989

990+
def unregister_agent_artifacts(self, agent_name: str) -> None:
991+
"""Remove ``agent_name``'s tracked preset command/skill artifacts.
992+
993+
Mirrors ``ExtensionManager.unregister_agent_artifacts()`` (#2948):
994+
used by ``integration switch`` when deactivating the previous
995+
integration, so a preset's command overrides and skill mirrors
996+
written for that agent don't linger as orphans in its directory
997+
once a different (possibly not-yet-installed) integration becomes
998+
active — including custom preset commands and files the registrar
999+
would otherwise skip as user-modified.
1000+
1001+
Scoped strictly to ``agent_name``: only that agent's own tracked
1002+
artifacts and registry entries are touched. Other agents' files,
1003+
tracking, and preset packs themselves are left untouched, and no
1004+
priority-stack reconciliation runs — this is agent-scoped cleanup
1005+
only, not preset removal.
1006+
"""
1007+
if not agent_name:
1008+
return
1009+
1010+
try:
1011+
from ..agents import CommandRegistrar
1012+
1013+
agent_config = CommandRegistrar().AGENT_CONFIGS.get(agent_name)
1014+
except ImportError:
1015+
agent_config = None
1016+
if agent_config is None:
1017+
return
1018+
1019+
for pack_id, metadata in list(self.registry.list().items()):
1020+
pack_dir = self.presets_dir / pack_id
1021+
updates: Dict[str, Any] = {}
1022+
1023+
raw_skills = metadata.get("registered_skills", [])
1024+
if isinstance(raw_skills, list) and raw_skills:
1025+
# Legacy flat-list value predating per-agent provenance:
1026+
# infer real ownership from on-disk markers before removing
1027+
# anything, so only agent_name's actual share is unregistered
1028+
# and the rest migrates to per-agent form instead of either
1029+
# guessing every name belongs to agent_name or blindly
1030+
# leaving other agents' shares unrecoverable (#2948).
1031+
registered_skills_all = self._infer_legacy_skill_provenance(
1032+
[n for n in raw_skills if isinstance(n, str)],
1033+
pack_id,
1034+
fallback_agent=agent_name,
1035+
)
1036+
skills_migrated = True
1037+
elif isinstance(raw_skills, dict):
1038+
registered_skills_all = copy.deepcopy(raw_skills)
1039+
skills_migrated = False
1040+
else:
1041+
registered_skills_all = {}
1042+
skills_migrated = False
1043+
1044+
registered_commands = metadata.get("registered_commands", {})
1045+
if not isinstance(registered_commands, dict):
1046+
registered_commands = {}
1047+
1048+
agent_command_names = [
1049+
n for n in registered_commands.get(agent_name, []) if isinstance(n, str)
1050+
]
1051+
1052+
# Native SKILL.md agents (claude/codex/agy/…) materialize their
1053+
# preset override in _register_commands(), tracked under
1054+
# registered_commands, not registered_skills — see
1055+
# _register_skills()'s own docstring ("Native skill agents …
1056+
# materialize brand-new preset skills in _register_commands()").
1057+
# A legacy flat-list registered_skills value predating that
1058+
# split can still attribute the very same on-disk file to this
1059+
# agent via provenance inference; unregistering through both
1060+
# paths would double-process the identical directory (delete
1061+
# via the commands path, then no-op "restore" via the skills
1062+
# path since the directory is already gone). Mirror remove()'s
1063+
# own coordination: whenever this agent's artifact is already
1064+
# handled via registered_commands, never additionally treat it
1065+
# as a registered_skills entry for the same agent.
1066+
if agent_command_names and agent_config.get("extension") == "/SKILL.md":
1067+
registered_skills_all.pop(agent_name, None)
1068+
1069+
if agent_command_names:
1070+
self._unregister_commands({agent_name: agent_command_names})
1071+
new_registered_commands = copy.deepcopy(registered_commands)
1072+
new_registered_commands.pop(agent_name, None)
1073+
updates["registered_commands"] = new_registered_commands
1074+
1075+
agent_skill_names = registered_skills_all.get(agent_name) or []
1076+
if agent_skill_names or skills_migrated:
1077+
if agent_skill_names:
1078+
self._unregister_skills({agent_name: agent_skill_names}, pack_dir)
1079+
remaining = {
1080+
other_agent: names
1081+
for other_agent, names in registered_skills_all.items()
1082+
if other_agent != agent_name
1083+
}
1084+
updates["registered_skills"] = remaining
1085+
1086+
if updates:
1087+
self.registry.update(pack_id, updates)
1088+
9551089
def _unregister_commands(self, registered_commands: Dict[str, List[str]]) -> None:
9561090
"""Remove previously registered command files from agent directories.
9571091

tests/integrations/test_integration_subcommand.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2281,6 +2281,83 @@ def test_switch_migrates_copilot_skills_extension_commands(self, tmp_path):
22812281
assert "opencode" in git_meta["registered_commands"]
22822282
assert "copilot" not in git_meta["registered_commands"]
22832283

2284+
def test_switch_to_not_yet_installed_unregisters_old_preset_artifacts(self, tmp_path):
2285+
"""Switching to a not-yet-installed integration must also clean up
2286+
the old agent's preset command overrides, mirroring the existing
2287+
extension cleanup on the same code path (#2948).
2288+
2289+
Without this, a preset's command override -- including a custom
2290+
preset command -- rendered for the previous agent lingers as an
2291+
orphan once a different, not-yet-installed integration becomes the
2292+
new active agent.
2293+
"""
2294+
project = _init_project(tmp_path, "auggie")
2295+
2296+
preset_src = tmp_path / "switch-cleanup-preset"
2297+
(preset_src / "commands").mkdir(parents=True)
2298+
(preset_src / "commands" / "speckit.specify.md").write_text(
2299+
"---\ndescription: Custom preset command\n---\nOverridden content\n",
2300+
encoding="utf-8",
2301+
)
2302+
manifest_data = {
2303+
"schema_version": "1.0",
2304+
"preset": {
2305+
"id": "switch-cleanup-preset",
2306+
"name": "Switch Cleanup Preset",
2307+
"version": "1.0.0",
2308+
"description": "Test preset with a custom command override",
2309+
},
2310+
"requires": {"speckit_version": ">=0.1.0"},
2311+
"provides": {
2312+
"templates": [
2313+
{
2314+
"type": "command",
2315+
"name": "speckit.specify",
2316+
"file": "commands/speckit.specify.md",
2317+
}
2318+
]
2319+
},
2320+
}
2321+
import yaml
2322+
2323+
(preset_src / "preset.yml").write_text(yaml.dump(manifest_data), encoding="utf-8")
2324+
2325+
result = _run_in_project(project, ["preset", "add", "--dev", str(preset_src)])
2326+
assert result.exit_code == 0, f"preset add failed: {result.output}"
2327+
2328+
auggie_cmd = project / ".augment" / "commands" / "speckit.specify.md"
2329+
assert auggie_cmd.exists(), "sanity: preset command registered for auggie"
2330+
2331+
registry_path = project / ".specify" / "presets" / ".registry"
2332+
registered = json.loads(registry_path.read_text(encoding="utf-8"))[
2333+
"presets"
2334+
]["switch-cleanup-preset"]["registered_commands"]
2335+
assert "auggie" in registered, "sanity: auggie tracked before switch"
2336+
2337+
# opencode is not yet installed in this project.
2338+
result = _run_in_project(project, [
2339+
"integration", "switch", "opencode",
2340+
"--script", "sh",
2341+
])
2342+
assert result.exit_code == 0, result.output
2343+
2344+
assert not auggie_cmd.exists(), (
2345+
"old agent's preset command override must be removed on switch "
2346+
"to a not-yet-installed integration, mirroring the existing "
2347+
"extension cleanup on this same code path (#2948)"
2348+
)
2349+
2350+
opencode_cmd = project / ".opencode" / "commands" / "speckit.specify.md"
2351+
assert opencode_cmd.exists(), "preset command should be registered for the new agent"
2352+
2353+
registered = json.loads(registry_path.read_text(encoding="utf-8"))[
2354+
"presets"
2355+
]["switch-cleanup-preset"]["registered_commands"]
2356+
assert "auggie" not in registered, (
2357+
"old agent's tracking must be dropped after switch cleanup"
2358+
)
2359+
assert "opencode" in registered
2360+
22842361
def test_switch_does_not_register_disabled_extensions(self, tmp_path):
22852362
"""Disabled extensions should stay disabled and should not migrate commands."""
22862363
project = _init_project(tmp_path, "opencode")

0 commit comments

Comments
 (0)