From 2f6aae7921e413275ca9cd9e98f4e684180e7473 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 11:16:34 +0100 Subject: [PATCH 01/12] feat: add preset update command Add catalogue, URL, and development-source preset updates with staged validation, atomic replacement, dry-run previews, bulk operation support, and command, skill, and constitution reconciliation. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- docs/reference/presets.md | 67 ++ src/specify_cli/presets/__init__.py | 742 +++++++++++++++++++++- src/specify_cli/presets/_commands.py | 560 +++++++++++++--- tests/test_presets.py | 914 ++++++++++++++++++++++++++- 4 files changed, 2178 insertions(+), 105 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 1098abfb42..094505ac8b 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -39,6 +39,73 @@ specify preset remove Removes an installed preset and cleans up its registered commands. +## Update a Preset + +```bash +specify preset update [] +``` + +Updates one installed preset, or all installed presets when no ID is given. +Catalogue updates are resolved by preset ID alone. Spec Kit searches the active +catalogues for a matching entry and only uses entries from catalogues that +allow installation. It does not retain or replay the original URL or local +directory used to install a preset. If the installed preset ID cannot be +resolved through an installation-enabled catalogue, provide an explicit +`--dev ` or `--from ` source. + +This means a preset installed from a catalogue can later be updated from the +catalogue entry currently associated with its ID, while a development or +one-off archive installation remains updateable only when an explicit source +is supplied or when that ID is otherwise available from an installation-enabled +catalogue. The incoming manifest must still use the installed preset ID and +pass normal compatibility and file validation. + +| Option | Description | +| ---------------- | ------------------------------------------------ | +| `--from ` | Update from a `.zip`, `.tar.gz`, or `.tgz` URL | +| `--dev ` | Update from a local directory | +| `--priority ` | Set a new resolution priority for a single update | +| `--all` | Update all installed presets | +| `--dry-run` | Show the manifest diff without changing anything | + +Single-preset updates do not prompt for confirmation and may use `--priority` +to reprioritize the preset. Bulk updates ask once, report each preset +independently, preserve priority and enabled state, and skip compatible +presets that are already current. If `--priority` is supplied to a bulk +update, it is ignored and a note is printed after pre-flight checks to prevent +unintended priority collisions. +Updates stage and validate the new preset before atomically replacing the +installed directory. + +### Recovering from an interrupted update + +An interrupted update can leave `..update-*.bak` (the pre-update +directory) or `..update-*.staging` (the validated candidate) under +`.specify/presets/`. These are crash-recovery artefacts, not additional +installed presets. Do not edit the registry to point at either directory. + +Inspect both manifests before taking action: + +```bash +find .specify/presets -maxdepth 1 \ + \( -name '' -o -name '.*.update-*.bak' -o -name '.*.update-*.staging' \) \ + -print +sed -n '1,120p' .specify/presets//preset.yml +sed -n '1,120p' .specify/presets/..update-*.bak/preset.yml +sed -n '1,120p' .specify/presets/..update-*.staging/preset.yml +``` + +If the live `` directory is missing and the backup manifest is the +version recorded in `.specify/presets/.registry`, restore the backup by +renaming it to ``. Leave a staging directory untouched until its +contents have been inspected, then remove it only after confirming that the +live directory and registry agree. If the state is ambiguous, copy the +affected directories aside for inspection and use the supported update flow +with `--from ` or `--dev `, or remove and re-add the preset. There +is deliberately no repair command. Recovery uses the normal update, remove, and +add flows, and catalogue re-resolution is deliberately based on preset ID +rather than stored source provenance. + ## List Installed Presets ```bash diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index abc63299c2..fabdf44d1f 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -45,7 +45,11 @@ ) from .._invocation_style import get_invocation_prefix from ..integrations.base import IntegrationBase -from .._utils import dump_frontmatter, version_satisfies +from .._utils import ( + dump_frontmatter, + relative_extension_path_violation, + version_satisfies, +) from ..shared_infra import ( _ensure_safe_shared_destination, _ensure_safe_shared_directory, @@ -77,6 +81,14 @@ def _is_comparable_version(value: str) -> bool: return True +def _versions_equal(left: str, right: str) -> bool: + """Compare versions using PEP 440 semantics where possible.""" + try: + return pkg_version.Version(left) == pkg_version.Version(right) + except pkg_version.InvalidVersion: + return left == right + + def _constitution_is_generated( project_root: Path, memory_constitution: Path, @@ -156,9 +168,15 @@ def _materialize_constitution_template( content = composed_content.encode("utf-8") result = "composed" + provenance = memory_constitution.parent / _CONSTITUTION_PROVENANCE_FILE + if memory_constitution.exists() and memory_constitution.read_bytes() == content: + # An unchanged constitution must produce zero file changes, so no + # provenance sidecar is written here either. Provenance is recorded + # only when constitution content is actually materialized below. + return "unchanged" + _ensure_safe_shared_directory(project_root, memory_constitution.parent) _write_shared_bytes(project_root, memory_constitution, content) - provenance = memory_constitution.parent / _CONSTITUTION_PROVENANCE_FILE _write_shared_text( project_root, provenance, @@ -454,6 +472,22 @@ def _validate(self): f"Invalid template {field}: expected a string, " f"got {type(tmpl[field]).__name__}" ) + aliases = tmpl.get("aliases", []) + if aliases is None: + aliases = [] + tmpl["aliases"] = aliases + if not isinstance(aliases, list) or not all( + isinstance(alias, str) for alias in aliases + ): + raise PresetValidationError( + "Invalid template aliases: expected a list of strings" + ) + for alias in aliases: + reason = relative_extension_path_violation(alias) + if reason: + raise PresetValidationError( + f"Invalid template alias '{alias}': {reason}" + ) if tmpl["type"] not in VALID_PRESET_TEMPLATE_TYPES: raise PresetValidationError( @@ -467,13 +501,14 @@ def _validate(self): # counted by PresetManifest.templates. Reject at validation time # instead, mirroring the sibling fix for ExtensionManifest's # provides.templates/scripts (#4016). - name_type = (tmpl["name"], tmpl["type"]) - if name_type in seen_name_types: - raise PresetValidationError( - f"Duplicate template name '{tmpl['name']}' of type " - f"'{tmpl['type']}' in 'provides.templates'" - ) - seen_name_types.add(name_type) + for declared_name in (tmpl["name"], *aliases): + name_type = (declared_name, tmpl["type"]) + if name_type in seen_name_types: + raise PresetValidationError( + f"Duplicate template name or alias '{declared_name}' " + f"of type '{tmpl['type']}' in 'provides.templates'" + ) + seen_name_types.add(name_type) # Validate file path safety: must be relative, no parent traversal file_path = tmpl["file"] @@ -680,6 +715,85 @@ def get_hash(self) -> str: return f"sha256:{h.hexdigest()}" +def diff_preset_manifests( + old_manifest: PresetManifest, new_manifest: PresetManifest +) -> Dict[str, List[Dict[str, Any]]]: + """Diff preset templates by their stable ``(name, type)`` identity.""" + old_entries = { + (item["name"], item["type"]): item for item in old_manifest.templates + } + new_entries = { + (item["name"], item["type"]): item for item in new_manifest.templates + } + added = [] + removed = [] + changed = [] + unchanged = [] + for identity in sorted(new_entries.keys() - old_entries.keys()): + added.append({"identity": identity, "new": new_entries[identity]}) + for identity in sorted(old_entries.keys() - new_entries.keys()): + removed.append({"identity": identity, "old": old_entries[identity]}) + for identity in sorted(old_entries.keys() & new_entries.keys()): + old_item = old_entries[identity] + new_item = new_entries[identity] + if old_item == new_item: + unchanged.append( + {"identity": identity, "old": old_item, "new": new_item} + ) + else: + changed.append( + {"identity": identity, "old": old_item, "new": new_item} + ) + return { + "added": added, + "removed": removed, + "changed": changed, + "unchanged": unchanged, + } + + +def _diff_preset_template_files( + diff: Dict[str, List[Dict[str, Any]]], + old_manifest: PresetManifest, + new_manifest: PresetManifest, + old_dir: Path, + new_dir: Path, +) -> Dict[str, List[Dict[str, Any]]]: + """Include referenced-file content changes in the manifest diff.""" + old_by_identity = { + (item["name"], item["type"]): item for item in old_manifest.templates + } + new_by_identity = { + (item["name"], item["type"]): item for item in new_manifest.templates + } + unchanged = [] + changed = list(diff["changed"]) + for entry in diff["unchanged"]: + identity = entry["identity"] + old_path = old_dir / old_by_identity[identity]["file"] + new_path = new_dir / new_by_identity[identity]["file"] + if ( + not old_path.is_file() + or not new_path.is_file() + or old_path.read_bytes() != new_path.read_bytes() + ): + changed.append( + { + "identity": identity, + "old": old_by_identity[identity], + "new": new_by_identity[identity], + } + ) + else: + unchanged.append(entry) + return { + "added": diff["added"], + "removed": diff["removed"], + "changed": changed, + "unchanged": unchanged, + } + + class PresetRegistry: """Manages the registry of installed presets.""" @@ -1123,7 +1237,9 @@ def find_unmet_extension_dependencies( def _register_commands( self, manifest: PresetManifest, - preset_dir: Path + preset_dir: Path, + *, + command_names: Optional[set[str]] = None, ) -> Dict[str, List[str]]: """Register preset command overrides with all detected AI agents. @@ -1143,7 +1259,13 @@ def _register_commands( Dictionary mapping agent names to lists of registered command names """ command_templates = [ - t for t in manifest.templates if t.get("type") == "command" + t for t in manifest.templates + if t.get("type") == "command" + and ( + command_names is None + or t["name"] in command_names + or any(alias in command_names for alias in t.get("aliases", [])) + ) ] if not command_templates: return {} @@ -1932,6 +2054,7 @@ def _reconcile_composed_commands( command_names: List[str], extra_agents: Optional[Set[str]] = None, target_agent: Optional[str] = None, + include_disabled_id: Optional[str] = None, ) -> Set[str]: """Re-resolve and re-register composed commands from the full stack. @@ -1980,7 +2103,9 @@ def _reconcile_composed_commands( except ImportError: return set() - resolver = PresetResolver(self.project_root) + resolver = PresetResolver( + self.project_root, include_disabled_id=include_disabled_id + ) registrar = CommandRegistrar() reconciled_commands: set[str] = set() @@ -2029,7 +2154,13 @@ def record_written(written: Dict[str, List[str]]) -> None: # Cache registry and manifests outside the loop to avoid # repeated filesystem reads for each command name. - presets_by_priority = list(self.registry.list_by_priority()) + presets_by_priority = [ + (pack_id, metadata) + for pack_id, metadata in self.registry.list_by_priority( + include_disabled=include_disabled_id is not None + ) + if metadata.get("enabled", True) or pack_id == include_disabled_id + ] for cmd_name in command_names: layers = resolver.collect_all_layers(cmd_name, "command") @@ -2393,6 +2524,7 @@ def _reconcile_skills( Dict[Path, tuple[Optional[str], List[str]]] ] = None, target_agent: Optional[str] = None, + include_disabled_id: Optional[str] = None, ) -> Set[str]: """Re-register skills for commands whose winning layer changed. @@ -2420,7 +2552,9 @@ def _reconcile_skills( # command renders its skill whether or not a like-named extension is # installed. The per-name loop below skips anything that doesn't # resolve to a managed skill directory. - resolver = PresetResolver(self.project_root) + resolver = PresetResolver( + self.project_root, include_disabled_id=include_disabled_id + ) active_skills_dir = self._get_skills_dir() from .. import load_init_options @@ -2431,7 +2565,13 @@ def _reconcile_skills( active_ai = None # Cache registry once to avoid repeated filesystem reads - presets_by_priority = list(self.registry.list_by_priority()) + presets_by_priority = [ + (pack_id, metadata) + for pack_id, metadata in self.registry.list_by_priority( + include_disabled=include_disabled_id is not None + ) + if metadata.get("enabled", True) or pack_id == include_disabled_id + ] # Group command names by winning preset to batch _register_skills calls # while only registering skills for the specific commands being @@ -2911,6 +3051,7 @@ def _register_skills( *, target_dir: Optional[Path] = None, target_agent: Optional[str] = None, + command_names: Optional[set[str]] = None, ) -> Dict[str, List[str]]: """Generate SKILL.md files for preset command overrides. @@ -2944,7 +3085,13 @@ def _register_skills( two can be tracked/restored consistently (#2948). """ command_templates = [ - t for t in manifest.templates if t.get("type") == "command" + t for t in manifest.templates + if t.get("type") == "command" + and ( + command_names is None + or t["name"] in command_names + or any(alias in command_names for alias in t.get("aliases", [])) + ) ] if not command_templates: return {} @@ -3927,9 +4074,17 @@ def install_from_directory( # Reconcile all affected commands from the full priority stack so that # install order doesn't determine the winning command file. cmd_names = [ - t["name"] + name for t in manifest.templates if t.get("type") == "command" + for name in ( + [t["name"]] + + [ + alias + for alias in t.get("aliases", []) + if isinstance(alias, str) + ] + ) ] if cmd_names: try: @@ -4010,6 +4165,533 @@ def _reconcile_constitution(self, *, create_if_missing: bool = False) -> None: return _materialize_constitution_template(self.project_root, memory_constitution) + def _validate_update_source( + self, + source_dir: Path, + pack_id: str, + speckit_version: str, + ) -> tuple[PresetManifest, PresetManifest, Dict[str, List[Dict[str, Any]]]]: + """Validate an update without touching the installed preset.""" + current_dir = self.presets_dir / pack_id + try: + old_manifest = PresetManifest(current_dir / "preset.yml") + except PresetValidationError as exc: + raise PresetValidationError( + f"Installed preset '{pack_id}' has a corrupt manifest; " + "remove and add the preset again" + ) from exc + + try: + new_manifest = PresetManifest(source_dir / "preset.yml") + except PresetValidationError as exc: + raise PresetValidationError( + f"Incoming preset '{pack_id}' has an unparseable or corrupt " + "manifest; remove and add the preset again" + ) from exc + if new_manifest.id != pack_id: + raise PresetValidationError( + f"Preset ID mismatch: installed '{pack_id}', " + f"incoming '{new_manifest.id}'. Remove and add the preset again." + ) + self.check_compatibility(new_manifest, speckit_version) + for template in new_manifest.templates: + referenced = source_dir / template["file"] + if not referenced.is_file(): + raise PresetValidationError( + f"Preset '{pack_id}' is missing referenced file " + f"'{template['file']}'. Remove and add the preset again." + ) + diff = diff_preset_manifests(old_manifest, new_manifest) + diff = _diff_preset_template_files( + diff, old_manifest, new_manifest, current_dir, source_dir + ) + + def convention_constitution(base_dir: Path) -> Optional[bytes]: + for relative in ( + "templates/constitution-template.md", + "constitution-template.md", + ): + candidate = base_dir / relative + if candidate.is_file(): + return candidate.read_bytes() + return None + + diff["_constitution_changed"] = ( + convention_constitution(current_dir) + != convention_constitution(source_dir) + ) + diff["_constitution_layer"] = ( + new_manifest.id == _CONSTITUTION_SYNC_PRESET_ID + or any( + item.get("type") == "template" + and item.get("name") == "constitution-template" + for item in old_manifest.templates + new_manifest.templates + ) + or convention_constitution(current_dir) is not None + or convention_constitution(source_dir) is not None + ) + return old_manifest, new_manifest, diff + + def update_from_directory( + self, + source_dir: Path, + speckit_version: str, + *, + pack_id: Optional[str] = None, + priority: Optional[int] = None, + dry_run: bool = False, + expected_version: Optional[str] = None, + ) -> tuple[PresetManifest, Dict[str, List[Dict[str, Any]]]]: + """Update an installed preset while serializing its transaction.""" + from ..workflows._commands import _workflow_install_transaction + + if dry_run: + return self._update_from_directory_locked( + source_dir, + speckit_version, + pack_id=pack_id, + priority=priority, + dry_run=True, + expected_version=expected_version, + ) + + with _workflow_install_transaction(self.project_root): + # Refresh after waiting for the lock so concurrent updates cannot + # overwrite newer registry metadata with this manager's snapshot. + self.registry.data = PresetRegistry(self.presets_dir).data + if expected_version is not None: + metadata = self.registry.get(pack_id or "") + installed_version = ( + metadata.get("version") if isinstance(metadata, dict) else None + ) + if isinstance(installed_version, str): + try: + installed = pkg_version.Version(installed_version) + catalogue = pkg_version.Version(expected_version) + except pkg_version.InvalidVersion: + pass + else: + if installed > catalogue and priority is None: + raise PresetCompatibilityError( + f"installed preset version {installed_version} is " + f"newer than catalogue version {expected_version}" + ) + return self._update_from_directory_locked( + source_dir, + speckit_version, + pack_id=pack_id, + priority=priority, + dry_run=dry_run, + expected_version=expected_version, + ) + + def _update_from_directory_locked( + self, + source_dir: Path, + speckit_version: str, + *, + pack_id: Optional[str] = None, + priority: Optional[int] = None, + dry_run: bool = False, + expected_version: Optional[str] = None, + ) -> tuple[PresetManifest, Dict[str, List[Dict[str, Any]]]]: + """Update an installed preset from a validated directory.""" + if priority is not None and priority < 1: + raise PresetValidationError( + "Priority must be a positive integer (1 or higher)" + ) + try: + incoming = PresetManifest(source_dir / "preset.yml") + except PresetValidationError as exc: + raise PresetValidationError( + "Incoming preset has an unparseable or corrupt manifest; " + "remove and add the preset again" + ) from exc + target_id = pack_id or incoming.id + if not self.registry.is_installed(target_id): + raise PresetError(f"Preset '{target_id}' is not installed") + old_manifest, new_manifest, diff = self._validate_update_source( + source_dir, target_id, speckit_version + ) + if expected_version is not None and not _versions_equal( + new_manifest.version, expected_version + ): + raise PresetValidationError( + f"Preset '{target_id}' manifest version {new_manifest.version} " + f"does not match catalogue version {expected_version}" + ) + if dry_run: + return new_manifest, diff + + metadata = self.registry.get(target_id) + if metadata is None: + raise PresetError(f"Preset '{target_id}' has no valid registry entry") + preserved_priority = normalize_priority( + priority if priority is not None else metadata.get("priority", 10) + ) + enabled = metadata.get("enabled", True) + swap_token = tempfile.mkdtemp( + prefix=f".{target_id}.update-", dir=self.presets_dir + ) + os.rmdir(swap_token) + staging_dir = Path(f"{swap_token}.staging") + backup_dir = Path(f"{swap_token}.bak") + def remove_swap_path(path: Path) -> None: + if path.is_symlink(): + path.unlink() + elif path.exists(): + shutil.rmtree(path) + + try: + shutil.copytree(source_dir, staging_dir) + generated_composition = staging_dir / ".composed" + if generated_composition.is_symlink(): + generated_composition.unlink() + elif generated_composition.is_dir(): + shutil.rmtree(generated_composition) + elif generated_composition.exists(): + generated_composition.unlink() + _, staged_manifest, staged_diff = self._validate_update_source( + staging_dir, target_id, speckit_version + ) + if expected_version is not None and not _versions_equal( + staged_manifest.version, expected_version + ): + raise PresetValidationError( + f"Preset '{target_id}' manifest version " + f"{staged_manifest.version} does not match catalogue version " + f"{expected_version}" + ) + # The source directory may be edited while it is being copied + # (notably during --dev updates). Once staged, the copied tree is + # the immutable input for this update, so its validation result + # must drive both the registry metadata and reconciliation. + new_manifest = staged_manifest + diff = staged_diff + staged_manifest_hash = new_manifest.get_hash() + except Exception: + remove_swap_path(staging_dir) + raise + current_dir = self.presets_dir / target_id + registry_before = copy.deepcopy(metadata) + try: + try: + os.replace(current_dir, backup_dir) + except FileNotFoundError as exc: + raise PresetError( + f"Preset '{target_id}' changed on disk during the update " + "(another update may be running); no changes were made" + ) from exc + try: + os.replace(staging_dir, current_dir) + new_manifest.path = current_dir / "preset.yml" + self.registry.update( + target_id, + { + "version": new_manifest.version, + "manifest_hash": staged_manifest_hash, + "priority": preserved_priority, + "enabled": enabled, + }, + ) + except Exception: + remove_swap_path(current_dir) + if backup_dir.exists(): + os.replace(backup_dir, current_dir) + self.registry.restore(target_id, registry_before) + raise + except Exception: + remove_swap_path(staging_dir) + raise + + primary_command_names = { + item["identity"][0] + for item in diff["added"] + diff["removed"] + diff["changed"] + if item["identity"][1] == "command" + } + command_names = set(primary_command_names) + if priority is not None: + original_priority = normalize_priority(metadata.get("priority", 10)) + else: + original_priority = preserved_priority + if preserved_priority != original_priority: + primary_command_names.update( + item["name"] + for item in new_manifest.templates + if item.get("type") == "command" and isinstance(item.get("name"), str) + ) + primary_command_names.update( + item["name"] + for item in old_manifest.templates + if item.get("type") == "command" and isinstance(item.get("name"), str) + ) + for manifest in (old_manifest, new_manifest): + for item in manifest.templates: + if item.get("type") == "command": + command_names.update( + alias + for alias in item.get("aliases", []) + if isinstance(alias, str) + ) + for item in diff["added"] + diff["removed"] + diff["changed"]: + for template in (item.get("old"), item.get("new")): + if template and template.get("type") == "command": + command_names.update( + alias + for alias in template.get("aliases", []) + if isinstance(alias, str) + ) + reconcile_command_names = set(primary_command_names) + reconcile_command_names.update(command_names) + try: + old_command_names = { + item["name"] + for item in old_manifest.templates + if item.get("type") == "command" + } + new_command_names = { + item["name"] + for item in new_manifest.templates + if item.get("type") == "command" + } + old_command_names.update( + alias + for item in old_manifest.templates + if item.get("type") == "command" + for alias in item.get("aliases", []) + if isinstance(alias, str) + ) + new_command_names.update( + alias + for item in new_manifest.templates + if item.get("type") == "command" + for alias in item.get("aliases", []) + if isinstance(alias, str) + ) + removed_command_names = old_command_names - new_command_names + reconcile_command_names.update(removed_command_names) + registered_commands_before = metadata.get("registered_commands", {}) + if removed_command_names and isinstance( + registered_commands_before, dict + ): + stale_commands = { + agent: [ + name + for name in names + if name in removed_command_names + ] + for agent, names in registered_commands_before.items() + if isinstance(names, list) + } + stale_commands = { + agent: names + for agent, names in stale_commands.items() + if names + } + if stale_commands: + self._unregister_commands(stale_commands) + registered_commands = self._register_commands( + new_manifest, current_dir, command_names=primary_command_names + ) + active_agent = resolve_active_agent_for_registration(self.project_root) + merged_commands: Dict[str, List[str]] = {} + for agent, names in registered_commands_before.items(): + if isinstance(names, list): + names_to_remove = removed_command_names | ( + command_names if agent == active_agent else set() + ) + retained = [name for name in names if name not in names_to_remove] + if retained: + merged_commands[agent] = retained + for agent, names in registered_commands.items(): + merged_commands.setdefault(agent, []).extend( + name for name in names if name not in merged_commands.get(agent, []) + ) + registered_commands = merged_commands + self.registry.update( + target_id, {"registered_commands": registered_commands} + ) + affected_skill_names = set() + for name in reconcile_command_names: + modern, legacy = self._skill_names_for_command(name) + affected_skill_names.update((modern, legacy)) + raw_registered_skills_before = metadata.get("registered_skills", {}) + if isinstance(raw_registered_skills_before, list): + registered_skills_before = self._infer_legacy_skill_provenance( + [name for name in raw_registered_skills_before if isinstance(name, str)], + target_id, + ) + elif isinstance(raw_registered_skills_before, dict): + registered_skills_before = copy.deepcopy(raw_registered_skills_before) + else: + registered_skills_before = {} + if isinstance(registered_skills_before, dict): + removed_skill_names = { + skill_name + for command_name in removed_command_names + for skill_name in self._skill_names_for_command(command_name) + } + stale_skills = { + agent: [ + name + for name in names + if name in removed_skill_names + ] + for agent, names in registered_skills_before.items() + if isinstance(names, list) + } + stale_skills = { + agent: names + for agent, names in stale_skills.items() + if names + } + if stale_skills: + self._unregister_skills( + stale_skills, + current_dir, + restore_from_bundled_core=True, + ) + registered_skills = self._register_skills( + new_manifest, current_dir, command_names=reconcile_command_names + ) + if isinstance(registered_skills_before, dict): + merged_skills: Dict[str, List[str]] = {} + for agent, names in registered_skills_before.items(): + if isinstance(names, list): + names_to_remove = removed_skill_names | ( + affected_skill_names if agent == active_agent else set() + ) + retained = [name for name in names if name not in names_to_remove] + if retained: + merged_skills[agent] = retained + for agent, names in registered_skills.items(): + merged_skills.setdefault(agent, []).extend( + name for name in names + if name not in merged_skills.get(agent, []) + ) + registered_skills = merged_skills + self.registry.update(target_id, {"registered_skills": registered_skills}) + if reconcile_command_names: + historical_agents = { + agent + for agent in registered_commands_before + if agent != active_agent + } + # A priority change can promote this preset above a provider + # whose ownership lives only on another preset's registry + # entry, so inactive agents must be gathered stack-wide rather + # than from this preset's own history alone. + for other_id, other_meta in self.registry.list_by_priority( + include_disabled=True + ): + if other_id == target_id or not isinstance(other_meta, dict): + continue + other_commands = other_meta.get("registered_commands", {}) + if not isinstance(other_commands, dict): + continue + for agent, names in other_commands.items(): + if agent == active_agent or not isinstance(names, list): + continue + if reconcile_command_names.intersection(names): + historical_agents.add(agent) + self._reconcile_composed_commands( + sorted(reconcile_command_names), + extra_agents=historical_agents, + include_disabled_id=target_id if not enabled else None, + ) + extra_skill_dirs = {} + if isinstance(registered_skills_before, dict): + for agent, names in registered_skills_before.items(): + if agent == active_agent: + continue + if not isinstance(names, list): + continue + skill_dir = self._resolve_agent_skills_dir(agent) + extra_skill_dirs[skill_dir] = ( + agent, + sorted( + name + for name in names + if isinstance(name, str) + ), + ) + self._reconcile_skills( + sorted(reconcile_command_names), + extra_skills_dirs=extra_skill_dirs or None, + include_disabled_id=target_id if not enabled else None, + ) + def _preset_has_constitution_layer( + manifest: PresetManifest, preset_dir: Path + ) -> bool: + return ( + any( + item.get("type") == "template" + and item.get("name") == "constitution-template" + for item in manifest.templates + ) + or any( + (preset_dir / relative_path).is_file() + for relative_path in ( + "templates/constitution-template.md", + "constitution-template.md", + ) + ) + ) + + has_constitution_layer = _preset_has_constitution_layer( + old_manifest, backup_dir + ) or _preset_has_constitution_layer(new_manifest, current_dir) + if has_constitution_layer: + self.reconcile_constitution( + f"Failed to reconcile constitution after updating {target_id}", + create_if_missing=True, + ) + except Exception as exc: + import warnings + + warnings.warn( + f"Preset '{target_id}' was swapped, but post-update " + f"reconciliation failed: {exc}. Generated command, skill, or " + "constitution files may be stale. Re-run the update with " + f"--from or --dev , or remove and re-add " + f"preset '{target_id}' to refresh them.", + stacklevel=2, + ) + finally: + remove_swap_path(backup_dir) + remove_swap_path(staging_dir) + return new_manifest, diff + + def update_from_archive( + self, + archive_path: Path, + speckit_version: str, + *, + pack_id: str, + priority: Optional[int] = None, + dry_run: bool = False, + expected_version: Optional[str] = None, + ) -> tuple[PresetManifest, Dict[str, List[Dict[str, Any]]]]: + """Update an installed preset from a supported archive.""" + with tempfile.TemporaryDirectory() as tmpdir: + extracted = Path(tmpdir) + safe_extract_archive( + archive_path, extracted, error_type=PresetValidationError + ) + roots = [extracted] + if not (extracted / "preset.yml").exists(): + roots = [item for item in extracted.iterdir() if item.is_dir()] + if len(roots) != 1 or not (roots[0] / "preset.yml").is_file(): + raise PresetValidationError("No preset.yml found in archive") + return self.update_from_directory( + roots[0], + speckit_version, + pack_id=pack_id, + priority=priority, + dry_run=dry_run, + expected_version=expected_version, + ) + def install_from_archive( self, archive_path: Path, @@ -4418,7 +5100,7 @@ class PresetCatalog: COMMUNITY_CATALOG_URL = "https://raw.githubusercontent.com/github/spec-kit/main/presets/catalog.community.json" CACHE_DURATION = 3600 # 1 hour in seconds - def __init__(self, project_root: Path): + def __init__(self, project_root: Path, cache_dir: Optional[Path] = None): """Initialize preset catalog manager. Args: @@ -4426,7 +5108,7 @@ def __init__(self, project_root: Path): """ self.project_root = project_root self.presets_dir = project_root / ".specify" / "presets" - self.cache_dir = self.presets_dir / ".cache" + self.cache_dir = cache_dir or self.presets_dir / ".cache" self.cache_file = self.cache_dir / "catalog.json" self.cache_metadata_file = self.cache_dir / "catalog-metadata.json" @@ -5298,7 +5980,9 @@ class PresetResolver: 4. .specify/templates/ - Core templates (shipped with Spec Kit) """ - def __init__(self, project_root: Path): + def __init__( + self, project_root: Path, include_disabled_id: Optional[str] = None + ): """Initialize preset resolver. Args: @@ -5309,6 +5993,7 @@ def __init__(self, project_root: Path): self.presets_dir = project_root / ".specify" / "presets" self.overrides_dir = self.templates_dir / "overrides" self.extensions_dir = project_root / ".specify" / "extensions" + self.include_disabled_id = include_disabled_id self._manifest_cache: Dict[str, Optional["PresetManifest"]] = {} def _get_manifest(self, pack_dir: Path) -> Optional["PresetManifest"]: @@ -5333,7 +6018,10 @@ def _get_all_presets_by_priority(self) -> List[tuple[str, dict]]: registry = PresetRegistry(self.presets_dir) return [ (pack_id, metadata) - for pack_id, metadata in registry.list_by_priority() + for pack_id, metadata in registry.list_by_priority( + include_disabled=self.include_disabled_id is not None + ) + if metadata.get("enabled", True) or pack_id == self.include_disabled_id if self._is_safe_registry_id(pack_id) ] @@ -5360,7 +6048,17 @@ def _manifest_declared_template( if not manifest: return None, None for tmpl in manifest.templates: - if tmpl.get("name") == template_name and tmpl.get("type") == template_type: + aliases = tmpl.get("aliases", []) + if ( + tmpl.get("type") == template_type + and ( + tmpl.get("name") == template_name + or ( + isinstance(aliases, list) + and template_name in aliases + ) + ) + ): file_path = tmpl.get("file") if file_path: manifest_candidate = pack_dir / file_path diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index ab74a8e029..af57a71dc9 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -10,6 +10,8 @@ import os import re +import shutil +import tempfile from pathlib import Path import typer @@ -18,7 +20,6 @@ from .._console import console from .._download_security import ( - archive_format_from_name, archive_suffix, detect_archive_format, is_https_or_localhost_http, @@ -40,6 +41,110 @@ preset_app.add_typer(preset_catalog_app, name="catalog") +def _fetch_preset_archive_data( + url: str, + error_type: type[Exception], +) -> tuple[bytes, str | None, str]: + """Fetch bounded archive bytes after validating URL and redirects.""" + import urllib.error + from urllib.parse import urlparse + + from specify_cli._github_http import resolve_github_release_asset_api_url + from specify_cli.authentication.http import github_provider_hosts, open_url + + try: + parsed = urlparse(url) + _ = parsed.port + except ValueError as exc: + raise error_type(f"Invalid URL: {url}") from exc + if not is_https_or_localhost_http(url): + raise error_type("URL must use HTTPS (HTTP is only allowed for localhost)") + + def validate_redirect(old_url, new_url): + if not is_safe_download_redirect(old_url, new_url): + raise error_type( + "redirect target must use HTTPS or remain on localhost" + ) + + try: + resolved_url = resolve_github_release_asset_api_url( + url, open_url, github_hosts=github_provider_hosts() + ) + download_url = resolved_url or url + extra_headers = {"Accept": "application/octet-stream"} if resolved_url else None + with open_url( + download_url, + timeout=60, + extra_headers=extra_headers, + redirect_validator=validate_redirect, + ) as response: + final_url = ( + response.geturl() if hasattr(response, "geturl") else download_url + ) + if not is_https_or_localhost_http(final_url): + raise error_type( + "Preset URL redirected to a disallowed URL: " + f"{final_url}. Redirect targets must use HTTPS with a hostname, " + "or HTTP for localhost (127.0.0.1, ::1)." + ) + data = read_response_limited( + response, error_type=error_type, label=f"preset {url}" + ) + content_type = ( + response.getheader("Content-Type") + if hasattr(response, "getheader") + else None + ) + except urllib.error.URLError as exc: + raise error_type(f"Failed to download preset: {exc}") from exc + return data, content_type, final_url + + +def _write_preset_archive( + data: bytes, + source_url: str, + content_type: str | None, + error_type: type[Exception], +) -> Path: + """Write downloaded bytes to a temporary archive with a detected suffix.""" + fd, name = tempfile.mkstemp(prefix="speckit-preset-update-", suffix=".archive") + path = Path(name) + try: + os.close(fd) + path.write_bytes(data) + detected = detect_archive_format( + path, + source_name=source_url, + content_type=content_type, + error_type=error_type, + ) + detected_path = path.with_suffix(archive_suffix(detected)) + os.replace(path, detected_path) + path = detected_path + return path + except Exception: + path.unlink(missing_ok=True) + raise + + +def _download_preset_archive(url: str, error_type: type[Exception]) -> Path: + """Download and classify a preset archive into a temporary file.""" + data, content_type, final_url = _fetch_preset_archive_data(url, error_type) + return _write_preset_archive(data, final_url, content_type, error_type) + + +def _cleanup_archive(path: Path | None) -> None: + if path is None: + return + try: + path.unlink(missing_ok=True) + except OSError as exc: + console.print( + f"[yellow]Warning:[/yellow] Could not remove temporary archive " + f"'{path}': {exc}" + ) + + def _warn_unmet_extension_dependencies(manager, manifest) -> None: """Warn when a preset's declared extension dependencies are unsatisfied. @@ -206,11 +311,11 @@ def preset_add( """Install a preset.""" from .. import _locate_bundled_preset, _require_specify_project, get_speckit_version from . import ( - PresetManager, PresetCatalog, + PresetCompatibilityError, PresetError, + PresetManager, PresetValidationError, - PresetCompatibilityError, ) project_root = _require_specify_project() @@ -239,20 +344,11 @@ def preset_add( try: _parsed = _urlparse(from_url) - _parsed.port + _ = _parsed.port except ValueError: console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") raise typer.Exit(1) - def _validate_download_redirect(old_url, new_url): - if not is_safe_download_redirect(old_url, new_url): - import urllib.error - - raise urllib.error.URLError( - "redirect target must use HTTPS without entering a local " - "target, or stay within loopback over HTTP" - ) - if not is_https_or_localhost_http(from_url): console.print( "[red]Error:[/red] URL must use HTTPS with a hostname and be " @@ -262,78 +358,24 @@ def _validate_download_redirect(old_url, new_url): raise typer.Exit(1) console.print(f"Installing preset from [cyan]{_escape_markup(from_url)}[/cyan]...") - import urllib.error - import tempfile - - with tempfile.TemporaryDirectory() as tmpdir: - archive_path = Path(tmpdir) / "preset.archive" - try: - from specify_cli.authentication.http import open_url as _open_url - from specify_cli.authentication.http import github_provider_hosts - from specify_cli._github_http import resolve_github_release_asset_api_url - - _preset_extra_headers = None - _resolved_from_url = resolve_github_release_asset_api_url( - from_url, _open_url, github_hosts=github_provider_hosts() - ) - if _resolved_from_url: - from_url = _resolved_from_url - _preset_extra_headers = {"Accept": "application/octet-stream"} - - with _open_url( - from_url, - timeout=60, - extra_headers=_preset_extra_headers, - redirect_validator=_validate_download_redirect, - ) as response: - final_url = response.geturl() if hasattr(response, "geturl") else from_url - if not is_https_or_localhost_http(final_url): - console.print( - "[red]Error:[/red] Preset URL redirected to a disallowed URL: " - f"{final_url}. Redirect targets must use HTTPS with a hostname, " - "or HTTP for localhost (127.0.0.1, ::1)." - ) - raise typer.Exit(1) - archive_data = read_response_limited( - response, - error_type=PresetError, - label=f"preset {from_url}", - ) - content_type = ( - response.getheader("Content-Type") - if hasattr(response, "getheader") - else None - ) - archive_path.write_bytes(archive_data) - format_source = ( - final_url - if archive_format_from_name(final_url) is not None - else from_url - ) - archive_format = detect_archive_format( - archive_path, - source_name=format_source, - content_type=content_type, - error_type=PresetError, - ) - detected_path = archive_path.with_suffix( - archive_suffix(archive_format) - ) - os.replace(archive_path, detected_path) - archive_path = detected_path - except (urllib.error.URLError, PresetError) as e: - console.print( - f"[red]Error:[/red] Failed to download: " - f"{_escape_markup(str(e))}" - ) - raise typer.Exit(1) - + archive_path = None + try: + archive_path = _download_preset_archive(from_url, PresetError) + except PresetError as e: + console.print( + f"[red]Error:[/red] Failed to download: " + f"{_escape_markup(str(e))}" + ) + raise typer.Exit(1) + try: manifest = manager.install_from_zip( archive_path, speckit_version, priority, ) - + finally: + if archive_path is not None and archive_path.exists(): + _cleanup_archive(archive_path) console.print(f"[green]✓[/green] Preset '{manifest.name}' v{manifest.version} installed (priority {priority})") elif preset_id: @@ -384,7 +426,7 @@ def _validate_download_redirect(old_url, new_url): console.print(f"[green]✓[/green] Preset '{manifest.name}' v{manifest.version} installed (priority {priority})") finally: if 'archive_path' in locals() and archive_path.exists(): - archive_path.unlink(missing_ok=True) + _cleanup_archive(archive_path) else: console.print("[red]Error:[/red] Specify a preset ID, --from URL, or --dev path") raise typer.Exit(1) @@ -428,6 +470,360 @@ def preset_remove( raise typer.Exit(1) +@preset_app.command("update") +def preset_update( + preset_id: str = typer.Argument(None, help="Preset ID to update (or all)"), + from_url: str = typer.Option(None, "--from", help="Update from an archive URL"), + dev: str = typer.Option(None, "--dev", help="Update from a local directory"), + priority: int = typer.Option(None, "--priority", help="New priority"), + dry_run: bool = typer.Option(False, "--dry-run", help="Show changes without writing"), + all_presets: bool = typer.Option(False, "--all", help="Update all installed presets"), +): + """Update one preset, or all installed presets.""" + from packaging import version as pkg_version + + from .. import ( + _locate_bundled_preset, + _require_specify_project, + get_speckit_version, + ) + from . import ( + PresetCatalog, + PresetCompatibilityError, + PresetError, + PresetManager, + PresetResolver, + PresetValidationError, + _constitution_is_generated, + ) + + project_root = _require_specify_project() + manager = PresetManager(project_root) + if priority is not None and not (all_presets or not preset_id) and priority < 1: + console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + raise typer.Exit(1) + if from_url and dev: + console.print("[red]Error:[/red] Use only one of --from or --dev") + raise typer.Exit(1) + if preset_id and all_presets: + console.print("[red]Error:[/red] Use either a preset ID or --all, not both") + raise typer.Exit(1) + if (from_url or dev) and (not preset_id or all_presets): + console.print("[red]Error:[/red] --from and --dev require a preset ID") + raise typer.Exit(1) + + installed = manager.list_installed() + bulk = all_presets or not preset_id + effective_priority = None if bulk else priority + ids = [preset_id] if not bulk else [item["id"] for item in installed] + if not ids: + console.print("[yellow]No presets installed.[/yellow]") + return + + dry_run_cache = ( + Path(tempfile.mkdtemp(prefix="speckit-preset-dry-run-")) + if dry_run + else None + ) + catalog = PresetCatalog(project_root, cache_dir=dry_run_cache) + speckit_version = get_speckit_version() + outcomes = [] + catalog_candidates = {} + catalog_sources = {} + + if bulk: + actionable_ids = [] + catalog_archives = {} + for item_id in ids: + safe_id = _escape_markup(str(item_id)) + metadata = manager.registry.get(item_id) + archive_path = None + try: + if not manager.registry.is_installed(item_id): + raise PresetError( + f"Preset '{item_id}' is not installed; use 'preset add' instead" + ) + if metadata is None or "version" not in metadata: + raise PresetError("registry entry is missing or corrupt") + installed_version = pkg_version.Version(str(metadata["version"])) + pack_info = catalog.get_pack_info(item_id) + if not pack_info: + raise PresetError( + "source not re-resolvable — supply --from/--dev explicitly" + ) + if not pack_info.get("_install_allowed", True): + raise PresetError( + f"updates are not allowed from " + f"'{pack_info.get('_catalog_name', 'catalog')}'" + ) + catalog_version = pkg_version.Version(str(pack_info["version"])) + if catalog_version <= installed_version and effective_priority is None: + console.print( + f"[dim]• {safe_id}: Up to date, skipped " + f"(v{installed_version})[/dim]" + ) + outcomes.append("skipped") + continue + if pack_info.get("bundled") and not pack_info.get("download_url"): + source_path = _locate_bundled_preset(item_id) + if source_path is None: + raise PresetError( + f"Preset '{item_id}' is bundled with spec-kit but " + "could not be found in the installed package" + ) + manager.update_from_directory( + source_path, + speckit_version, + pack_id=item_id, + priority=effective_priority, + dry_run=True, + expected_version=str(pack_info["version"]), + ) + catalog_sources[item_id] = source_path + else: + archive_path = catalog.download_pack(item_id) + manager.update_from_archive( + archive_path, + speckit_version, + pack_id=item_id, + priority=effective_priority, + dry_run=True, + expected_version=str(pack_info["version"]), + ) + catalog_candidates[item_id] = pack_info + catalog_archives[item_id] = archive_path + actionable_ids.append(item_id) + except PresetCompatibilityError as exc: + detail = _escape_markup(str(exc).replace("\n", " ")) + console.print( + f"[yellow]•[/yellow] {safe_id}: skipped — {detail}" + ) + outcomes.append("skipped") + if archive_path is not None: + _cleanup_archive(archive_path) + except (KeyError, TypeError, ValueError): + detail = _escape_markup( + f"invalid version metadata for preset '{item_id}'" + ) + console.print(f"[yellow]•[/yellow] {safe_id}: failed — {detail}") + outcomes.append("failed") + if archive_path is not None: + _cleanup_archive(archive_path) + except (OSError, PresetValidationError, PresetError) as exc: + detail = _escape_markup(str(exc).replace("\n", " ")) + console.print(f"[yellow]•[/yellow] {safe_id}: failed — {detail}") + outcomes.append("failed") + if archive_path is not None: + _cleanup_archive(archive_path) + + ids = actionable_ids + if not ids: + if any(outcome == "failed" for outcome in outcomes): + if dry_run_cache is not None: + shutil.rmtree(dry_run_cache, ignore_errors=True) + raise typer.Exit(1) + if dry_run_cache is not None: + shutil.rmtree(dry_run_cache, ignore_errors=True) + return + if priority is not None: + console.print( + "[yellow]Note:[/yellow] --priority is ignored for bulk updates; " + "existing preset priorities will be preserved." + ) + if not typer.confirm("Update all installed presets?"): + for archive_path in catalog_archives.values(): + if archive_path is not None: + _cleanup_archive(archive_path) + console.print("Cancelled") + if dry_run_cache is not None: + shutil.rmtree(dry_run_cache, ignore_errors=True) + return + + for item_id in ids: + safe_id = _escape_markup(str(item_id)) + source_kind = "catalog" + archive_path = catalog_archives.get(item_id) if bulk else None + try: + metadata = manager.registry.get(item_id) + if not manager.registry.is_installed(item_id): + raise PresetError( + f"Preset '{item_id}' is not installed; use 'preset add' instead" + ) + if metadata is None or "version" not in metadata: + raise PresetError("registry entry is missing or corrupt") + try: + installed_version = pkg_version.Version(str(metadata["version"])) + except (KeyError, TypeError, ValueError) as exc: + raise PresetError( + f"invalid installed version for preset '{item_id}'" + ) from exc + source_path = None + pack_info = None + if dev: + source_kind = "dev" + source_path = Path(dev).resolve() + if not source_path.is_dir(): + raise PresetError(f"Directory not found: {dev}") + elif from_url: + source_kind = "url" + archive_path = _download_preset_archive(from_url, PresetError) + else: + pack_info = catalog_candidates.get(item_id) or catalog.get_pack_info(item_id) + if not pack_info: + raise PresetError( + "source not re-resolvable — supply --from/--dev explicitly" + ) + if not pack_info.get("_install_allowed", True): + raise PresetError( + f"updates are not allowed from " + f"'{pack_info.get('_catalog_name', 'catalog')}'" + ) + try: + catalog_version = pkg_version.Version(str(pack_info["version"])) + except (KeyError, TypeError, ValueError) as exc: + raise PresetError( + f"catalog entry for preset '{item_id}' has an invalid version" + ) from exc + if catalog_version < installed_version or ( + catalog_version == installed_version and effective_priority is None + ): + console.print( + f"[dim]• {safe_id}: Up to date, skipped " + f"(v{installed_version})[/dim]" + ) + outcomes.append("skipped") + continue + if pack_info.get("bundled") and not pack_info.get("download_url"): + source_path = catalog_sources.get(item_id) or _locate_bundled_preset( + item_id + ) + if source_path is None: + raise PresetError( + f"Preset '{item_id}' is bundled with spec-kit but " + "could not be found in the installed package" + ) + elif archive_path is None: + archive_path = catalog.download_pack(item_id) + + constitution_path = ( + project_root / ".specify" / "memory" / "constitution.md" + ) + constitution_before = ( + constitution_path.read_bytes() if constitution_path.exists() else None + ) + if source_kind == "dev" or source_path is not None: + manifest, diff = manager.update_from_directory( + source_path, + speckit_version, + pack_id=item_id, + priority=effective_priority, + dry_run=dry_run, + expected_version=( + str(pack_info["version"]) if pack_info is not None else None + ), + ) + else: + manifest, diff = manager.update_from_archive( + archive_path, + speckit_version, + pack_id=item_id, + priority=effective_priority, + dry_run=dry_run, + expected_version=( + str(pack_info["version"]) if pack_info is not None else None + ), + ) + action = "would update" if dry_run else "updated" + added_commands = sum( + entry["identity"][1] == "command" for entry in diff["added"] + ) + removed_commands = sum( + entry["identity"][1] == "command" for entry in diff["removed"] + ) + changed_commands = sum( + entry["identity"][1] == "command" for entry in diff["changed"] + ) + constitution_after = ( + constitution_path.read_bytes() if constitution_path.exists() else None + ) + constitution_diff = diff.get("_constitution_changed", False) or any( + entry["identity"] == ("constitution-template", "template") + for category in ("added", "removed", "changed") + for entry in diff[category] + ) + if dry_run: + sync_metadata = manager.registry.get("constitution-sync") + constitution_can_reconcile = ( + bool(diff.get("_constitution_layer")) + and isinstance(sync_metadata, dict) + and sync_metadata.get("enabled", True) + and constitution_path.exists() + and _constitution_is_generated( + project_root, + constitution_path, + PresetResolver(project_root), + ) + ) + constitution_status = ( + "constitution change planned" + if constitution_can_reconcile + and (constitution_diff or priority is not None) + else "constitution unchanged" + ) + else: + constitution_status = ( + "constitution unchanged" + if constitution_before == constitution_after + else "constitution reconciled" + ) + console.print( + f"[green]✓[/green] {safe_id}: {action} to v{manifest.version} " + f"(+{added_commands} commands, -{removed_commands} commands, " + f"~{changed_commands} commands, " + f"{constitution_status}, " + f"priority {'kept at ' + str(metadata.get('priority', 10)) if effective_priority is None else 'set to ' + str(effective_priority)})" + ) + if dry_run: + for category in ("added", "removed", "changed", "unchanged"): + identities = [ + f"{name} ({template_type})" + for (name, template_type) in ( + entry["identity"] for entry in diff[category] + ) + ] + if identities: + console.print( + f" {category}: {', '.join(identities)}" + ) + console.print(" planned actions: stage, validate, atomically swap, reconcile") + else: + _warn_unmet_extension_dependencies(manager, manifest) + outcomes.append("updated") + except OSError as exc: + detail = _escape_markup(str(exc).replace("\n", " ")) + console.print(f"[yellow]•[/yellow] {safe_id}: failed — {detail}") + outcomes.append("failed") + except (PresetCompatibilityError, PresetValidationError, PresetError) as exc: + detail = _escape_markup(str(exc).replace("\n", " ")) + prefix = ( + "skipped" + if bulk and isinstance(exc, PresetCompatibilityError) + else "failed" + ) + console.print(f"[yellow]•[/yellow] {safe_id}: {prefix} — {detail}") + outcomes.append(prefix) + finally: + if archive_path is not None: + _cleanup_archive(archive_path) + + if any(outcome == "failed" for outcome in outcomes): + if dry_run_cache is not None: + shutil.rmtree(dry_run_cache, ignore_errors=True) + raise typer.Exit(1) + if dry_run_cache is not None: + shutil.rmtree(dry_run_cache, ignore_errors=True) + + @preset_app.command("search") def preset_search( query: str = typer.Argument(None, help="Search query"), @@ -577,7 +973,7 @@ def preset_info( """Show detailed information about a preset.""" from .. import _require_specify_project from ..extensions import normalize_priority - from . import PresetCatalog, PresetManager, PresetError + from . import PresetCatalog, PresetError, PresetManager project_root = _require_specify_project() safe_preset_id = _escape_markup(str(preset_id)) diff --git a/tests/test_presets.py b/tests/test_presets.py index 57a70b4192..fb7c8d87d0 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -41,7 +41,11 @@ ) from specify_cli.extensions import ExtensionRegistry from specify_cli._console import console -from specify_cli.presets._commands import _warn_unmet_extension_dependencies +from specify_cli.presets._commands import ( + _download_preset_archive, + _warn_unmet_extension_dependencies, + _write_preset_archive, +) # ===== Fixtures ===== @@ -526,6 +530,30 @@ def test_duplicate_template_name_and_type_raises_validation_error( with pytest.raises(PresetValidationError, match="Duplicate template name"): PresetManifest(manifest_path) + def test_duplicate_template_alias_and_type_raises_validation_error( + self, temp_dir, valid_pack_data + ): + """Aliases must not shadow another template's primary identity.""" + valid_pack_data["provides"]["templates"] = [ + { + "type": "command", + "name": "specify", + "file": "commands/specify.md", + "aliases": ["spec"], + }, + { + "type": "command", + "name": "other", + "file": "commands/other.md", + "aliases": ["spec"], + }, + ] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, "w") as f: + yaml.dump(valid_pack_data, f) + with pytest.raises(PresetValidationError, match="Duplicate template name or alias"): + PresetManifest(manifest_path) + def test_same_name_different_type_templates_allowed( self, temp_dir, valid_pack_data ): @@ -5481,6 +5509,24 @@ def _create_multi_command_preset_with_aliases(self, temp_dir, preset_id, command yaml.dump(manifest_data, f) return preset_dir + def test_resolver_matches_manifest_aliases(self, project_dir, temp_dir): + preset_dir = self._create_multi_command_preset_with_aliases( + temp_dir, "alias-resolver-preset", [("speckit.primary", ["speckit.alias"])] + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.0") + + resolver = PresetResolver(project_dir) + entry, candidate = resolver._manifest_declared_template( + manager.presets_dir / "alias-resolver-preset", + "speckit.alias", + "command", + ) + + assert entry is not None + assert candidate is not None + assert candidate.name == "speckit.primary.md" + def test_skill_overridden_on_preset_install(self, project_dir, temp_dir): """When skills mode was used, a preset command override should update the skill.""" # Simulate skills mode having been used: write init-options + create skill @@ -14651,3 +14697,869 @@ def test_resolved_content_embeds_core_and_sync_pass(self, project_dir): assert "## Constitution Template Sync" in content assert "supersedes the \"Scope Guard\" above" in content assert "plan-template.md" in content + + +class TestPresetUpdate: + def _updated_source(self, pack_dir, version="2.0.0"): + source = pack_dir.parent / "updated-pack" + shutil.copytree(pack_dir, source) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = version + data["provides"]["templates"].append( + { + "type": "command", + "name": "new-command", + "file": "commands/new-command.md", + } + ) + (source / "commands").mkdir() + (source / "commands" / "new-command.md").write_text("# New\n") + manifest_path.write_text(yaml.safe_dump(data)) + return source + + def test_manifest_diff_uses_name_and_type_identity(self, pack_dir): + source = self._updated_source(pack_dir) + old = PresetManifest(pack_dir / "preset.yml") + new = PresetManifest(source / "preset.yml") + from specify_cli.presets import diff_preset_manifests + + diff = diff_preset_manifests(old, new) + assert [item["identity"] for item in diff["added"]] == [ + ("new-command", "command") + ] + assert diff["changed"] == [] + assert diff["unchanged"][0]["identity"] == ("spec-template", "template") + + def test_update_preserves_priority_and_enabled_state( + self, project_dir, pack_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=3) + manager.registry.update("test-pack", {"enabled": False}) + source = self._updated_source(pack_dir) + + manifest, diff = manager.update_from_directory( + source, "0.1.0", pack_id="test-pack" + ) + + assert manifest.version == "2.0.0" + assert diff["added"] + metadata = manager.registry.get("test-pack") + assert metadata["priority"] == 3 + assert metadata["enabled"] is False + assert (project_dir / ".specify/presets/test-pack/preset.yml").exists() + + def test_dry_run_does_not_modify_installation(self, project_dir, pack_dir): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=4) + before = (project_dir / ".specify/presets/test-pack/preset.yml").read_bytes() + source = self._updated_source(pack_dir) + + manifest, diff = manager.update_from_directory( + source, "0.1.0", pack_id="test-pack", dry_run=True + ) + + assert manifest.version == "2.0.0" + assert diff["added"] + assert ( + project_dir / ".specify/presets/test-pack/preset.yml" + ).read_bytes() == before + assert manager.registry.get("test-pack")["version"] == "1.0.0" + + def test_dry_run_validates_catalogue_expected_version( + self, project_dir, pack_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + + with pytest.raises(PresetValidationError, match="does not match catalogue"): + manager.update_from_directory( + source, + "0.1.0", + pack_id="test-pack", + dry_run=True, + expected_version="9.9.9", + ) + + assert manager.registry.get("test-pack")["version"] == "1.0.0" + + def test_disabled_preset_is_included_when_reconciling_its_update( + self, project_dir, pack_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + manager.registry.update("test-pack", {"enabled": False}) + + resolver = PresetResolver(project_dir, include_disabled_id="test-pack") + layers = resolver.collect_all_layers("spec-template", "template") + + assert layers + assert layers[0]["path"] == ( + project_dir / ".specify" / "presets" / "test-pack" / "templates" / "spec-template.md" + ) + + def test_missing_referenced_file_leaves_installation_untouched( + self, project_dir, pack_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + (source / "commands" / "new-command.md").unlink() + installed = ( + project_dir / ".specify/presets/test-pack/preset.yml" + ).read_bytes() + + with pytest.raises(PresetValidationError, match="[Rr]emove and add"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert ( + project_dir / ".specify/presets/test-pack/preset.yml" + ).read_bytes() == installed + + def test_template_file_content_change_is_reconciled( + self, project_dir, pack_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = pack_dir.parent / "content-updated-pack" + shutil.copytree(pack_dir, source) + source_file = source / "templates" / "spec-template.md" + source_file.write_text(source_file.read_text() + "\nUpdated.\n") + + _, _, diff = manager._validate_update_source( + source, "test-pack", "0.1.0" + ) + + assert diff["changed"][0]["identity"] == ("spec-template", "template") + assert diff["unchanged"] == [] + + def test_id_mismatch_is_rejected_before_swap(self, project_dir, pack_dir): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + data = yaml.safe_load((source / "preset.yml").read_text()) + data["preset"]["id"] = "renamed-pack" + (source / "preset.yml").write_text(yaml.safe_dump(data)) + + with pytest.raises(PresetValidationError, match="Remove and add"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert manager.registry.get("test-pack")["version"] == "1.0.0" + + def test_corrupt_manifest_is_rejected_before_swap(self, project_dir, pack_dir): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + (source / "preset.yml").write_text("not: [valid") + + with pytest.raises(PresetValidationError, match="corrupt"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert manager.registry.get("test-pack")["version"] == "1.0.0" + + def test_type_change_remains_reconcilable(self, project_dir, pack_dir): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + data = yaml.safe_load((source / "preset.yml").read_text()) + data["provides"]["templates"][0]["type"] = "script" + data["provides"]["templates"][0]["name"] = "spec-template-script" + (source / "preset.yml").write_text(yaml.safe_dump(data)) + (source / "templates" / "spec-template.md").write_text("# Script\n") + + _, diff = manager.update_from_directory( + source, "0.1.0", pack_id="test-pack" + ) + + assert ("spec-template", "template") in [ + entry["identity"] for entry in diff["removed"] + ] + assert ("spec-template-script", "script") in [ + entry["identity"] for entry in diff["added"] + ] + + def test_swap_failure_restores_directory_and_registry( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + installed_manifest = ( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).read_bytes() + original_update = manager.registry.update + + def fail_update(pack_id, updates): + if updates.get("version") == "2.0.0": + raise OSError("simulated registry failure") + return original_update(pack_id, updates) + + monkeypatch.setattr(manager.registry, "update", fail_update) + with pytest.raises(OSError, match="simulated registry failure"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert ( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).read_bytes() == installed_manifest + assert manager.registry.get("test-pack")["version"] == "1.0.0" + + def test_staged_validation_failure_leaves_live_install_untouched( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + installed_manifest = ( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).read_bytes() + original_validate = manager._validate_update_source + calls = 0 + + def fail_staged(source_dir, pack_id, speckit_version): + nonlocal calls + calls += 1 + if calls == 2: + raise PresetValidationError("staged validation failed") + return original_validate(source_dir, pack_id, speckit_version) + + monkeypatch.setattr(manager, "_validate_update_source", fail_staged) + with pytest.raises(PresetValidationError, match="staged validation"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert ( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).read_bytes() == installed_manifest + assert not list( + (project_dir / ".specify" / "presets").glob(".test-pack.update-*") + ) + + def test_staged_validation_result_is_authoritative( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + original_copytree = shutil.copytree + + def copy_then_mutate(source_dir, destination_dir, *args, **kwargs): + result = original_copytree(source_dir, destination_dir, *args, **kwargs) + if isinstance(source_dir, Path) and source_dir == source: + data = yaml.safe_load((source_dir / "preset.yml").read_text()) + data["preset"]["version"] = "3.0.0" + (source_dir / "preset.yml").write_text(yaml.safe_dump(data)) + return result + + monkeypatch.setattr(shutil, "copytree", copy_then_mutate) + + manifest, _ = manager.update_from_directory( + source, "0.1.0", pack_id="test-pack" + ) + + assert manifest.version == "2.0.0" + assert manager.registry.get("test-pack")["version"] == "2.0.0" + + def test_convention_only_constitution_update_reconciles( + self, project_dir, temp_dir + ): + manager = PresetManager(project_dir) + manager.install_from_directory(CONSTITUTION_SYNC_PRESET_DIR, "0.15.0") + source = _make_convention_constitution_preset(temp_dir) + manager.install_from_directory(source, "0.1.5") + updated = temp_dir / "updated-convention-constitution" + shutil.copytree(source, updated) + data = yaml.safe_load((updated / "preset.yml").read_text()) + data["preset"]["version"] = "2.0.0" + (updated / "preset.yml").write_text(yaml.safe_dump(data)) + (updated / "templates" / "constitution-template.md").write_text( + "# Updated Convention Constitution\n" + ) + + manager.update_from_directory( + updated, "0.1.5", pack_id="convention-constitution" + ) + + memory = project_dir / ".specify" / "memory" / "constitution.md" + assert memory.read_text() == "# Updated Convention Constitution\n" + + def test_archive_write_cleans_temp_file_on_detection_failure( + self, monkeypatch + ): + created = {} + original_mkstemp = tempfile.mkstemp + + def record_mkstemp(*args, **kwargs): + fd, name = original_mkstemp(*args, **kwargs) + created["path"] = Path(name) + return fd, name + + monkeypatch.setattr(tempfile, "mkstemp", record_mkstemp) + monkeypatch.setattr( + "specify_cli.presets._commands.detect_archive_format", + lambda *args, **kwargs: (_ for _ in ()).throw( + PresetValidationError("invalid archive") + ), + ) + + with pytest.raises(PresetValidationError, match="invalid archive"): + _write_preset_archive( + b"not an archive", + "https://example.com/preset", + None, + PresetValidationError, + ) + + assert not created["path"].exists() + + def test_archive_download_uses_final_redirect_url_and_canonical_suffix( + self, monkeypatch + ): + """A .zip request redirected to a tarball must retain the final format.""" + archive = io.BytesIO() + with tarfile.open(fileobj=archive, mode="w:gz") as tar: + info = tarfile.TarInfo("preset.yml") + payload = b"schema_version: '1.0'\n" + info.size = len(payload) + tar.addfile(info, io.BytesIO(payload)) + + monkeypatch.setattr( + "specify_cli.presets._commands._fetch_preset_archive_data", + lambda url, error_type: ( + archive.getvalue(), + "application/gzip", + "https://cdn.example.test/preset.tgz", + ), + ) + + path = _download_preset_archive( + "https://example.test/preset.zip", PresetValidationError + ) + try: + assert path.suffixes[-2:] == [".tar", ".gz"] + finally: + path.unlink(missing_ok=True) + + def test_cli_missing_id_directs_user_to_add(self, project_dir): + from typer.testing import CliRunner + from specify_cli import app + + result = CliRunner().invoke( + app, + ["preset", "update", "missing-pack"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "preset" in result.output and "add" in result.output + + def test_cli_single_dry_run_does_not_prompt( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.1.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "test-pack", + "--dev", + str(source), + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "would update" in result.output + assert "planned actions" in result.output + assert "Update all installed presets?" not in result.output + + def test_cli_dry_run_does_not_plan_constitution_for_command_only_priority_change( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(CONSTITUTION_SYNC_PRESET_DIR, "0.15.0") + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.1.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "test-pack", + "--dev", + str(source), + "--priority", + "5", + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution unchanged" in result.output + assert "constitution reconciliation pending" not in result.output + + def test_dry_run_does_not_create_transaction_lock( + self, project_dir, pack_dir + ): + """Dry-run validation must not leave a project lock behind.""" + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + lock_path = project_dir / ".specify" / ".workflow-install.lock" + assert not lock_path.exists() + + manager.update_from_directory( + source, "0.1.0", pack_id="test-pack", dry_run=True + ) + + assert not lock_path.exists() + + def _second_pack(self, pack_dir, pack_id, version="1.0.0"): + source = pack_dir.parent / pack_id + shutil.copytree(pack_dir, source) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["id"] = pack_id + data["preset"]["name"] = pack_id + data["preset"]["version"] = version + manifest_path.write_text(yaml.safe_dump(data)) + return source + + def _bulk_cli_env(self, project_dir, monkeypatch, pack_infos, sources): + import specify_cli + from specify_cli.presets import PresetCatalog + + monkeypatch.setattr( + "specify_cli._require_specify_project", lambda: project_dir + ) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.1.0") + monkeypatch.setattr( + PresetCatalog, + "get_pack_info", + lambda self, pack_id: pack_infos.get(pack_id), + ) + monkeypatch.setattr( + specify_cli, + "_locate_bundled_preset", + lambda pack_id: sources.get(pack_id), + ) + + def test_cli_bulk_isolates_failures_and_preserves_state( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + second_source = self._second_pack(pack_dir, "second-pack") + manager.install_from_directory(second_source, "0.1.0", priority=5) + manager.registry.update("second-pack", {"enabled": False}) + + broken = pack_dir.parent / "broken-update" + shutil.copytree(pack_dir, broken) + (broken / "preset.yml").write_text("preset: [unbalanced\n") + good = self._second_pack(pack_dir, "second-pack-v2", version="2.0.0") + data = yaml.safe_load((good / "preset.yml").read_text()) + data["preset"]["id"] = "second-pack" + data["preset"]["name"] = "second-pack" + (good / "preset.yml").write_text(yaml.safe_dump(data)) + + self._bulk_cli_env( + project_dir, + monkeypatch, + { + "test-pack": {"version": "2.0.0", "bundled": True}, + "second-pack": {"version": "2.0.0", "bundled": True}, + }, + {"test-pack": broken, "second-pack": good}, + ) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 1, result.output + assert result.output.count("Update all installed presets?") == 1 + assert "test-pack: failed" in result.output + registry = PresetManager(project_dir).registry + second = registry.get("second-pack") + assert second["version"] == "2.0.0" + assert second["enabled"] is False + assert second["priority"] == 5 + assert registry.get("test-pack")["version"] == "1.0.0" + + def test_cli_bulk_skips_already_current_presets( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "1.0.0", "bundled": True}}, + {"test-pack": pack_dir}, + ) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 0, result.output + assert "Up to date, skipped" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "1.0.0" + + def test_cli_bulk_cancellation_leaves_installations_untouched( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "2.0.0", "bundled": True}}, + {"test-pack": source}, + ) + + result = CliRunner().invoke(app, ["preset", "update"], input="n\n") + + assert result.exit_code == 0, result.output + assert "Cancelled" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "1.0.0" + + def _two_actionable_packs(self, project_dir, pack_dir, monkeypatch): + """Install two presets that both clear preflight, so the main update + loop is genuinely exercised rather than short-circuited early.""" + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + manager.install_from_directory( + self._second_pack(pack_dir, "second-pack"), "0.1.0", priority=5 + ) + + def _renamed(tmp_id, target_id): + src = self._second_pack(pack_dir, tmp_id, version="2.0.0") + data = yaml.safe_load((src / "preset.yml").read_text()) + data["preset"]["id"] = target_id + data["preset"]["name"] = target_id + (src / "preset.yml").write_text(yaml.safe_dump(data)) + return src + + self._bulk_cli_env( + project_dir, + monkeypatch, + { + "test-pack": {"version": "2.0.0", "bundled": True}, + "second-pack": {"version": "2.0.0", "bundled": True}, + }, + { + "test-pack": _renamed("first-new", "test-pack"), + "second-pack": _renamed("second-new", "second-pack"), + }, + ) + + def test_cli_bulk_confirms_once_for_multiple_presets( + self, project_dir, pack_dir, monkeypatch + ): + """Two actionable presets must produce exactly one confirmation.""" + from typer.testing import CliRunner + from specify_cli import app + + self._two_actionable_packs(project_dir, pack_dir, monkeypatch) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 0, result.output + assert result.output.count("Update all installed presets?") == 1 + registry = PresetManager(project_dir).registry + assert registry.get("test-pack")["version"] == "2.0.0" + assert registry.get("second-pack")["version"] == "2.0.0" + + def test_cli_all_flag_matches_bare_bulk_invocation( + self, project_dir, pack_dir, monkeypatch + ): + """The explicit --all spelling drives the same bulk path.""" + from typer.testing import CliRunner + from specify_cli import app + + self._two_actionable_packs(project_dir, pack_dir, monkeypatch) + + result = CliRunner().invoke(app, ["preset", "update", "--all"], input="y\n") + + assert result.exit_code == 0, result.output + assert result.output.count("Update all installed presets?") == 1 + registry = PresetManager(project_dir).registry + assert registry.get("test-pack")["version"] == "2.0.0" + assert registry.get("second-pack")["version"] == "2.0.0" + + def test_cli_bulk_execution_failure_does_not_abort_later_presets( + self, project_dir, pack_dir, monkeypatch + ): + """A failure raised inside the main update loop, after preflight and + confirmation both succeeded, must not stop a later preset updating.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.presets import PresetManager as RealManager + + self._two_actionable_packs(project_dir, pack_dir, monkeypatch) + real_update = RealManager.update_from_directory + + def failing_update(self, source, speckit_version, **kwargs): + # Only fail the real update, never the preflight dry run, so the + # failure is guaranteed to occur in the main loop. + if kwargs.get("pack_id") == "test-pack" and not kwargs.get("dry_run"): + raise PresetError("simulated execution failure") + return real_update(self, source, speckit_version, **kwargs) + + monkeypatch.setattr(RealManager, "update_from_directory", failing_update) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 1, result.output + assert "test-pack: failed — simulated execution failure" in result.output + registry = PresetManager(project_dir).registry + assert registry.get("test-pack")["version"] == "1.0.0" + assert registry.get("second-pack")["version"] == "2.0.0" + + def test_cli_bulk_reports_incompatible_as_skipped_not_failed( + self, project_dir, pack_dir, monkeypatch + ): + """An incompatible preset surfacing during the real update is a true + no-op: reported as skipped, and it must not force a nonzero exit.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.presets import PresetCompatibilityError + from specify_cli.presets import PresetManager as RealManager + + self._two_actionable_packs(project_dir, pack_dir, monkeypatch) + real_update = RealManager.update_from_directory + + def incompatible(self, source, speckit_version, **kwargs): + if kwargs.get("pack_id") == "test-pack" and not kwargs.get("dry_run"): + raise PresetCompatibilityError("requires speckit >=9.0.0") + return real_update(self, source, speckit_version, **kwargs) + + monkeypatch.setattr(RealManager, "update_from_directory", incompatible) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 0, result.output + assert "test-pack: skipped" in result.output + assert "failed" not in result.output + registry = PresetManager(project_dir).registry + assert registry.get("test-pack")["version"] == "1.0.0" + assert registry.get("second-pack")["version"] == "2.0.0" + + def test_cli_bulk_rejects_discovery_only_catalogue( + self, project_dir, pack_dir, monkeypatch + ): + """The _install_allowed discovery-only gate applies to updates.""" + from typer.testing import CliRunner + from specify_cli import app + + PresetManager(project_dir).install_from_directory(pack_dir, "0.1.0") + self._bulk_cli_env( + project_dir, + monkeypatch, + { + "test-pack": { + "version": "2.0.0", + "bundled": True, + "_install_allowed": False, + "_catalog_name": "community", + } + }, + {"test-pack": self._updated_source(pack_dir)}, + ) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 1, result.output + assert "not allowed from" in result.output + assert "community" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "1.0.0" + + def test_cli_bulk_skips_when_installed_is_newer_than_catalogue( + self, project_dir, pack_dir, monkeypatch + ): + """A catalogue entry older than the installed version is a skip, never + a silent downgrade.""" + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + manager.registry.update("test-pack", {"version": "5.0.0"}) + + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "2.0.0", "bundled": True}}, + {"test-pack": self._updated_source(pack_dir)}, + ) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 0, result.output + assert "Up to date, skipped" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "5.0.0" + + def test_cli_bulk_priority_is_ignored_for_equal_catalogue_version( + self, project_dir, pack_dir, monkeypatch + ): + """Bulk priority remains ignored when the catalogue is already current.""" + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "1.0.0", "bundled": True}}, + {"test-pack": pack_dir}, + ) + + result = CliRunner().invoke( + app, ["preset", "update", "--all", "--priority", "3"], input="y\n" + ) + + assert result.exit_code == 0, result.output + assert "Up to date, skipped" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["priority"] == 7 + + def test_cli_single_priority_update_runs_at_equal_catalogue_version( + self, project_dir, pack_dir, monkeypatch + ): + """An explicit single-item priority change is actionable at equal version.""" + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "1.0.0", "bundled": True}}, + {"test-pack": pack_dir}, + ) + + result = CliRunner().invoke( + app, ["preset", "update", "test-pack", "--priority", "3"] + ) + + assert result.exit_code == 0, result.output + assert "updated" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["priority"] == 3 + + def test_cli_bulk_unresolvable_source_reports_clearly( + self, project_dir, pack_dir, monkeypatch + ): + """A preset with no catalogue entry cannot be re-resolved by id.""" + from typer.testing import CliRunner + from specify_cli import app + + PresetManager(project_dir).install_from_directory(pack_dir, "0.1.0") + self._bulk_cli_env(project_dir, monkeypatch, {}, {}) + + result = CliRunner().invoke(app, ["preset", "update"], input="y\n") + + assert result.exit_code == 1, result.output + assert "not re-resolvable" in result.output + assert "--from/--dev" in result.output + + +class TestPresetUpdateConcurrency: + def _updated_source(self, pack_dir, version="2.0.0"): + source = pack_dir.parent / "concurrent-updated-pack" + shutil.copytree(pack_dir, source) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = version + manifest_path.write_text(yaml.safe_dump(data)) + return source + + def test_vanished_install_during_swap_reports_clear_error( + self, project_dir, pack_dir, monkeypatch + ): + """A racing update that removes the live directory mid-swap must yield a + readable message rather than a raw errno.""" + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir) + import os + + real_replace = os.replace + current = project_dir / ".specify" / "presets" / "test-pack" + + def racing_replace(src, dst): + if Path(src) == current: + shutil.rmtree(current) + return real_replace(src, dst) + + monkeypatch.setattr( + "specify_cli.presets.os.replace", racing_replace + ) + + with pytest.raises(PresetError, match="changed on disk during the update"): + manager.update_from_directory(source, "0.1.0", pack_id="test-pack") + + assert not list( + (project_dir / ".specify" / "presets").glob(".test-pack.update-*") + ) + + def test_catalogue_version_is_rechecked_after_lock( + self, project_dir, pack_dir, monkeypatch + ): + """A newer concurrent install must prevent a stale catalogue update.""" + from contextlib import contextmanager + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + source = self._updated_source(pack_dir, version="2.0.0") + + @contextmanager + def racing_transaction(_project_root): + concurrent = PresetManager(project_dir) + concurrent.registry.update("test-pack", {"version": "3.0.0"}) + yield + + monkeypatch.setattr( + "specify_cli.workflows._commands._workflow_install_transaction", + racing_transaction, + ) + + with pytest.raises(PresetCompatibilityError, match="newer than catalogue"): + manager.update_from_directory( + source, + "0.1.0", + pack_id="test-pack", + expected_version="2.0.0", + ) + + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "3.0.0" + assert ( + PresetManifest( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).version + == "1.0.0" + ) From a016e472e693c2b1a89199cf7d75a2e335297e8a Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 11:35:04 +0100 Subject: [PATCH 02/12] fix: close preset update reconciliation gaps Prevent priority-bearing stale catalogue downgrades, reconcile affected inactive skill directories across the preset stack, and align constitution dry-run reporting with real update behaviour. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 100 ++++++++- src/specify_cli/presets/_commands.py | 18 +- tests/test_presets.py | 300 +++++++++++++++++++++++++++ 3 files changed, 403 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index fabdf44d1f..e1f3ad2f3c 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4221,8 +4221,7 @@ def convention_constitution(base_dir: Path) -> Optional[bytes]: != convention_constitution(source_dir) ) diff["_constitution_layer"] = ( - new_manifest.id == _CONSTITUTION_SYNC_PRESET_ID - or any( + any( item.get("type") == "template" and item.get("name") == "constitution-template" for item in old_manifest.templates + new_manifest.templates @@ -4271,7 +4270,7 @@ def update_from_directory( except pkg_version.InvalidVersion: pass else: - if installed > catalogue and priority is None: + if installed > catalogue: raise PresetCompatibilityError( f"installed preset version {installed_version} is " f"newer than catalogue version {expected_version}" @@ -4520,6 +4519,7 @@ def remove_swap_path(path: Path) -> None: registered_skills_before = self._infer_legacy_skill_provenance( [name for name in raw_registered_skills_before if isinstance(name, str)], target_id, + active_agent or "", ) elif isinstance(raw_registered_skills_before, dict): registered_skills_before = copy.deepcopy(raw_registered_skills_before) @@ -4599,25 +4599,107 @@ def remove_swap_path(path: Path) -> None: extra_agents=historical_agents, include_disabled_id=target_id if not enabled else None, ) - extra_skill_dirs = {} + extra_skill_dirs: Dict[ + Path, tuple[Optional[str], Set[str]] + ] = {} + + def add_extra_skill_dir( + agent: str, names: Set[str] + ) -> None: + if not names: + return + skill_dir = self._safe_skills_dir_for_agent(agent) + if skill_dir is None: + return + existing_agent, existing_names = extra_skill_dirs.get( + skill_dir, (agent, set()) + ) + extra_skill_dirs[skill_dir] = ( + existing_agent, + existing_names | names, + ) + if isinstance(registered_skills_before, dict): for agent, names in registered_skills_before.items(): if agent == active_agent: continue if not isinstance(names, list): continue - skill_dir = self._resolve_agent_skills_dir(agent) - extra_skill_dirs[skill_dir] = ( + add_extra_skill_dir( agent, - sorted( + { name for name in names + if self._is_safe_registry_skill_name(name) + }, + ) + + for other_id, other_meta in self.registry.list_by_priority( + include_disabled=True + ): + if other_id == target_id or not isinstance(other_meta, dict): + continue + raw_other_skills = other_meta.get("registered_skills", {}) + if isinstance(raw_other_skills, list): + other_skills = self._infer_legacy_skill_provenance( + [ + name + for name in raw_other_skills if isinstance(name, str) - ), + ], + other_id, + active_agent or "", ) + elif isinstance(raw_other_skills, dict): + other_skills = raw_other_skills + else: + continue + for agent, names in other_skills.items(): + if not isinstance(agent, str) or not isinstance(names, list): + continue + add_extra_skill_dir( + agent, + { + name + for name in names + if self._is_safe_registry_skill_name(name) + and name in affected_skill_names + }, + ) + + if extra_skill_dirs: + active_skill_dir = self._get_skills_dir() + if active_skill_dir is not None: + active_tracked_names = { + name + for skill_map in ( + registered_skills_before, + registered_skills, + ) + if isinstance(skill_map, dict) + and isinstance(active_agent, str) + for name in skill_map.get(active_agent, []) + if self._is_safe_registry_skill_name(name) + and name in affected_skill_names + } + active_dir_agent, active_dir_names = ( + extra_skill_dirs.get( + active_skill_dir, + (active_agent, set()), + ) + ) + extra_skill_dirs[active_skill_dir] = ( + active_dir_agent, + active_dir_names | active_tracked_names, + ) + + reconciled_skill_dirs = { + skill_dir: (agent, sorted(names)) + for skill_dir, (agent, names) in extra_skill_dirs.items() + } self._reconcile_skills( sorted(reconcile_command_names), - extra_skills_dirs=extra_skill_dirs or None, + extra_skills_dirs=reconciled_skill_dirs or None, include_disabled_id=target_id if not enabled else None, ) def _preset_has_constitution_layer( diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index af57a71dc9..60d89120e5 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -757,17 +757,23 @@ def preset_update( bool(diff.get("_constitution_layer")) and isinstance(sync_metadata, dict) and sync_metadata.get("enabled", True) - and constitution_path.exists() - and _constitution_is_generated( - project_root, - constitution_path, - PresetResolver(project_root), + and ( + not constitution_path.exists() + or _constitution_is_generated( + project_root, + constitution_path, + PresetResolver(project_root), + ) ) ) constitution_status = ( "constitution change planned" if constitution_can_reconcile - and (constitution_diff or priority is not None) + and ( + not constitution_path.exists() + or constitution_diff + or effective_priority is not None + ) else "constitution unchanged" ) else: diff --git a/tests/test_presets.py b/tests/test_presets.py index fb7c8d87d0..32d4416c7c 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -14718,6 +14718,38 @@ def _updated_source(self, pack_dir, version="2.0.0"): manifest_path.write_text(yaml.safe_dump(data)) return source + def _command_preset(self, temp_dir, preset_id, body): + preset_dir = temp_dir / preset_id + (preset_dir / "commands").mkdir(parents=True) + (preset_dir / "commands" / "speckit.specify.md").write_text( + "---\ndescription: Test command\n---\n\n" + f"{body}\n" + ) + (preset_dir / "preset.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + } + ] + }, + } + ) + ) + return preset_dir + def test_manifest_diff_uses_name_and_type_identity(self, pack_dir): source = self._updated_source(pack_dir) old = PresetManifest(pack_dir / "preset.yml") @@ -15113,6 +15145,201 @@ def test_cli_dry_run_does_not_plan_constitution_for_command_only_priority_change assert "constitution unchanged" in result.output assert "constitution reconciliation pending" not in result.output + def test_cli_dry_run_plans_missing_constitution_without_creating_it( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + source = _make_convention_constitution_preset(temp_dir) + manager.install_from_directory(source, "0.15.0") + manifest_path = source / "preset.yml" + manifest_data = yaml.safe_load(manifest_path.read_text()) + manifest_data["preset"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest_data)) + + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_path.unlink() + registry_path = project_dir / ".specify" / "presets" / ".registry" + registry_before = registry_path.read_bytes() + installed_before = ( + project_dir + / ".specify" + / "presets" + / "convention-constitution" + / "preset.yml" + ).read_bytes() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "convention-constitution", + "--dev", + str(source), + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution change planned" in result.output + assert not constitution_path.exists() + assert registry_path.read_bytes() == registry_before + assert ( + project_dir + / ".specify" + / "presets" + / "convention-constitution" + / "preset.yml" + ).read_bytes() == installed_before + assert not ( + project_dir / ".specify" / ".workflow-install.lock" + ).exists() + + def test_cli_bulk_dry_run_ignores_priority_in_constitution_preview( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + source = _make_convention_constitution_preset(temp_dir) + manager.install_from_directory(source, "0.15.0", priority=7) + manifest_path = source / "preset.yml" + manifest_data = yaml.safe_load(manifest_path.read_text()) + manifest_data["preset"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest_data)) + registry_path = project_dir / ".specify" / "presets" / ".registry" + registry_before = registry_path.read_bytes() + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_before = constitution_path.read_bytes() + self._bulk_cli_env( + project_dir, + monkeypatch, + { + "constitution-sync": { + "version": "1.0.0", + "bundled": True, + }, + "convention-constitution": { + "version": "2.0.0", + "bundled": True, + }, + }, + { + "constitution-sync": CONSTITUTION_SYNC_PRESET_DIR, + "convention-constitution": source, + }, + ) + + result = CliRunner().invoke( + app, + ["preset", "update", "--all", "--priority", "3", "--dry-run"], + input="y\n", + ) + + assert result.exit_code == 0, result.output + assert "constitution unchanged" in result.output + assert registry_path.read_bytes() == registry_before + assert constitution_path.read_bytes() == constitution_before + assert PresetManager(project_dir).registry.get( + "convention-constitution" + )["priority"] == 7 + + @pytest.mark.parametrize( + "legacy_registry_owner", [None, "other", "target"] + ) + def test_priority_update_reconciles_other_presets_inactive_skill_agent( + self, project_dir, temp_dir, legacy_registry_owner + ): + from specify_cli import save_init_options + + save_init_options( + project_dir, + {"ai": "claude", "ai_skills": True, "script": "sh"}, + ) + higher_source = self._command_preset( + temp_dir, "higher-preset", "higher preset body" + ) + target_source = self._command_preset( + temp_dir, "target-preset", "target preset body" + ) + manager = PresetManager(project_dir) + manager.install_from_directory(higher_source, "0.1.0", priority=5) + + save_init_options( + project_dir, + {"ai": "codex", "ai_skills": True, "script": "sh"}, + ) + manager.install_from_directory(target_source, "0.1.0", priority=10) + if legacy_registry_owner == "other": + manager.registry.update( + "higher-preset", + {"registered_skills": ["speckit-specify"]}, + ) + elif legacy_registry_owner == "target": + manager.registry.update( + "target-preset", + {"registered_skills": ["speckit-specify"]}, + ) + + claude_skill = ( + project_dir + / ".claude" + / "skills" + / "speckit-specify" + / "SKILL.md" + ) + codex_skill = ( + project_dir + / ".agents" + / "skills" + / "speckit-specify" + / "SKILL.md" + ) + assert "preset:higher-preset" in claude_skill.read_text() + assert "preset:higher-preset" in codex_skill.read_text() + + manifest_path = target_source / "preset.yml" + manifest_data = yaml.safe_load(manifest_path.read_text()) + manifest_data["preset"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest_data)) + (target_source / "commands" / "speckit.specify.md").write_text( + "---\ndescription: Test command\n---\n\n" + "updated target preset body\n" + ) + + manager.update_from_directory( + target_source, + "0.1.0", + pack_id="target-preset", + priority=1, + ) + + assert "preset:target-preset" in claude_skill.read_text() + assert "updated target preset body" in claude_skill.read_text() + assert "preset:target-preset" in codex_skill.read_text() + assert "updated target preset body" in codex_skill.read_text() + assert not (project_dir / ".github" / "skills").exists() + assert not ( + project_dir / ".agents" / "skills" / "speckit.specify" + ).exists() + target_skills = PresetManager(project_dir).registry.get( + "target-preset" + )["registered_skills"] + assert "speckit-specify" in target_skills["claude"] + assert "speckit-specify" in target_skills["codex"] + def test_dry_run_does_not_create_transaction_lock( self, project_dir, pack_dir ): @@ -15563,3 +15790,76 @@ def racing_transaction(_project_root): ).version == "1.0.0" ) + + def test_priority_update_rechecks_catalogue_version_after_lock( + self, project_dir, pack_dir, monkeypatch + ): + """A priority change must not permit a stale catalogue downgrade.""" + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + source = self._updated_source(pack_dir, version="2.0.0") + + @contextmanager + def racing_transaction(_project_root): + concurrent = PresetManager(project_dir) + concurrent.registry.update( + "test-pack", {"version": "3.0.0", "priority": 4} + ) + yield + + monkeypatch.setattr( + "specify_cli.workflows._commands._workflow_install_transaction", + racing_transaction, + ) + + with pytest.raises(PresetCompatibilityError, match="newer than catalogue"): + manager.update_from_directory( + source, + "0.1.0", + pack_id="test-pack", + priority=5, + expected_version="2.0.0", + ) + + metadata = PresetManager(project_dir).registry.get("test-pack") + assert metadata["version"] == "3.0.0" + assert metadata["priority"] == 4 + assert ( + PresetManifest( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ).version + == "1.0.0" + ) + + def test_equal_catalogue_version_priority_update_still_applies_after_lock( + self, project_dir, pack_dir, monkeypatch + ): + """An equal freshly observed version still permits reprioritisation.""" + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + source = self._updated_source(pack_dir, version="2.0.0") + + @contextmanager + def racing_transaction(_project_root): + concurrent = PresetManager(project_dir) + concurrent.registry.update("test-pack", {"version": "2.0.0"}) + yield + + monkeypatch.setattr( + "specify_cli.workflows._commands._workflow_install_transaction", + racing_transaction, + ) + + manager.update_from_directory( + source, + "0.1.0", + pack_id="test-pack", + priority=5, + expected_version="2.0.0", + ) + + metadata = PresetManager(project_dir).registry.get("test-pack") + assert metadata["version"] == "2.0.0" + assert metadata["priority"] == 5 From 2882fd006f7fad7113c6fc4bb5ec70c722bf432c Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 12:22:54 +0100 Subject: [PATCH 03/12] fix: stabilise preset update reconciliation Prevent disabled preset updates from materialising generated command or skill artefacts while still atomically replacing their source and registry metadata. Make update dry-run constitution reporting compare the prospective resolved bytes against the current generated constitution, matching real materialisation semantics for replace and composed layers. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 140 +++++--- src/specify_cli/presets/_commands.py | 32 +- tests/test_presets.py | 502 ++++++++++++++++++++++++++- 3 files changed, 612 insertions(+), 62 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index e1f3ad2f3c..5b31fb0603 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -153,20 +153,10 @@ def _materialize_constitution_template( via ``resolve_content``; ``None`` when no constitution template resolves. """ resolver = PresetResolver(project_root) - layers = resolver.collect_all_layers("constitution-template", "template") - if not layers: + resolved = _resolve_constitution_template_bytes(resolver) + if resolved is None: return None - - top_layer = layers[0] - if top_layer["strategy"] == "replace": - content = top_layer["path"].read_bytes() - result = "copied" - else: - composed_content = resolver.resolve_content("constitution-template", "template") - if composed_content is None: - return None - content = composed_content.encode("utf-8") - result = "composed" + content, top_layer, result = resolved provenance = memory_constitution.parent / _CONSTITUTION_PROVENANCE_FILE if memory_constitution.exists() and memory_constitution.read_bytes() == content: @@ -192,6 +182,27 @@ def _materialize_constitution_template( return result +def _resolve_constitution_template_bytes( + resolver: "PresetResolver", +) -> tuple[bytes, Dict[str, Any], str] | None: + """Resolve constitution bytes using the same rules as materialization.""" + layers = resolver.collect_all_layers("constitution-template", "template") + if not layers: + return None + + top_layer = layers[0] + if top_layer["strategy"] == "replace": + content = top_layer["path"].read_bytes() + result = "copied" + else: + composed_content = resolver.resolve_content("constitution-template", "template") + if composed_content is None: + return None + content = composed_content.encode("utf-8") + result = "composed" + return content, top_layer, result + + def _substitute_core_template( body: str, cmd_name: str, @@ -2054,7 +2065,6 @@ def _reconcile_composed_commands( command_names: List[str], extra_agents: Optional[Set[str]] = None, target_agent: Optional[str] = None, - include_disabled_id: Optional[str] = None, ) -> Set[str]: """Re-resolve and re-register composed commands from the full stack. @@ -2103,9 +2113,7 @@ def _reconcile_composed_commands( except ImportError: return set() - resolver = PresetResolver( - self.project_root, include_disabled_id=include_disabled_id - ) + resolver = PresetResolver(self.project_root) registrar = CommandRegistrar() reconciled_commands: set[str] = set() @@ -2156,10 +2164,7 @@ def record_written(written: Dict[str, List[str]]) -> None: # repeated filesystem reads for each command name. presets_by_priority = [ (pack_id, metadata) - for pack_id, metadata in self.registry.list_by_priority( - include_disabled=include_disabled_id is not None - ) - if metadata.get("enabled", True) or pack_id == include_disabled_id + for pack_id, metadata in self.registry.list_by_priority() ] for cmd_name in command_names: @@ -2524,7 +2529,6 @@ def _reconcile_skills( Dict[Path, tuple[Optional[str], List[str]]] ] = None, target_agent: Optional[str] = None, - include_disabled_id: Optional[str] = None, ) -> Set[str]: """Re-register skills for commands whose winning layer changed. @@ -2552,9 +2556,7 @@ def _reconcile_skills( # command renders its skill whether or not a like-named extension is # installed. The per-name loop below skips anything that doesn't # resolve to a managed skill directory. - resolver = PresetResolver( - self.project_root, include_disabled_id=include_disabled_id - ) + resolver = PresetResolver(self.project_root) active_skills_dir = self._get_skills_dir() from .. import load_init_options @@ -2567,10 +2569,7 @@ def _reconcile_skills( # Cache registry once to avoid repeated filesystem reads presets_by_priority = [ (pack_id, metadata) - for pack_id, metadata in self.registry.list_by_priority( - include_disabled=include_disabled_id is not None - ) - if metadata.get("enabled", True) or pack_id == include_disabled_id + for pack_id, metadata in self.registry.list_by_priority() ] # Group command names by winning preset to batch _register_skills calls @@ -4319,12 +4318,33 @@ def _update_from_directory_locked( f"Preset '{target_id}' manifest version {new_manifest.version} " f"does not match catalogue version {expected_version}" ) - if dry_run: - return new_manifest, diff - metadata = self.registry.get(target_id) if metadata is None: raise PresetError(f"Preset '{target_id}' has no valid registry entry") + if dry_run: + if diff.get("_constitution_layer"): + prospective_metadata = { + **metadata, + "version": new_manifest.version, + "priority": normalize_priority( + priority + if priority is not None + else metadata.get("priority", 10) + ), + } + prospective_resolver = PresetResolver( + self.project_root, + preset_overrides={ + target_id: (source_dir, prospective_metadata), + }, + ) + resolved_constitution = _resolve_constitution_template_bytes( + prospective_resolver + ) + if resolved_constitution is not None: + diff["_constitution_content_after"] = resolved_constitution[0] + return new_manifest, diff + preserved_priority = normalize_priority( priority if priority is not None else metadata.get("priority", 10) ) @@ -4403,6 +4423,15 @@ def remove_swap_path(path: Path) -> None: remove_swap_path(staging_dir) raise + if not enabled: + # Disabled presets keep their existing generated artifacts and + # provenance until removal. Updating only replaces the source and + # metadata while retaining enough ownership data for a later + # removal to clean up artifacts left on disk. + remove_swap_path(backup_dir) + remove_swap_path(staging_dir) + return new_manifest, diff + primary_command_names = { item["identity"][0] for item in diff["added"] + diff["removed"] + diff["changed"] @@ -4597,7 +4626,6 @@ def remove_swap_path(path: Path) -> None: self._reconcile_composed_commands( sorted(reconcile_command_names), extra_agents=historical_agents, - include_disabled_id=target_id if not enabled else None, ) extra_skill_dirs: Dict[ Path, tuple[Optional[str], Set[str]] @@ -4700,7 +4728,6 @@ def add_extra_skill_dir( self._reconcile_skills( sorted(reconcile_command_names), extra_skills_dirs=reconciled_skill_dirs or None, - include_disabled_id=target_id if not enabled else None, ) def _preset_has_constitution_layer( manifest: PresetManifest, preset_dir: Path @@ -6063,7 +6090,10 @@ class PresetResolver: """ def __init__( - self, project_root: Path, include_disabled_id: Optional[str] = None + self, + project_root: Path, + *, + preset_overrides: Optional[Dict[str, tuple[Path, Dict[str, Any]]]] = None, ): """Initialize preset resolver. @@ -6075,9 +6105,13 @@ def __init__( self.presets_dir = project_root / ".specify" / "presets" self.overrides_dir = self.templates_dir / "overrides" self.extensions_dir = project_root / ".specify" / "extensions" - self.include_disabled_id = include_disabled_id + self.preset_overrides = preset_overrides or {} self._manifest_cache: Dict[str, Optional["PresetManifest"]] = {} + def _preset_dir(self, pack_id: str) -> Path: + override = self.preset_overrides.get(pack_id) + return override[0] if override is not None else self.presets_dir / pack_id + def _get_manifest(self, pack_dir: Path) -> Optional["PresetManifest"]: """Get a cached preset manifest, parsing it on first access.""" key = str(pack_dir) @@ -6098,12 +6132,30 @@ def _is_safe_registry_id(value: object) -> bool: def _get_all_presets_by_priority(self) -> List[tuple[str, dict]]: registry = PresetRegistry(self.presets_dir) + if self.preset_overrides: + presets = { + pack_id: metadata + for pack_id, metadata in registry.list_by_priority( + include_disabled=True + ) + } + for pack_id, (_pack_dir, metadata) in self.preset_overrides.items(): + presets[pack_id] = copy.deepcopy(metadata) + return sorted( + ( + (pack_id, metadata) + for pack_id, metadata in presets.items() + if metadata.get("enabled", True) + and self._is_safe_registry_id(pack_id) + ), + key=lambda item: ( + normalize_priority(item[1].get("priority", 10)), + item[0], + ), + ) return [ (pack_id, metadata) - for pack_id, metadata in registry.list_by_priority( - include_disabled=self.include_disabled_id is not None - ) - if metadata.get("enabled", True) or pack_id == self.include_disabled_id + for pack_id, metadata in registry.list_by_priority() if self._is_safe_registry_id(pack_id) ] @@ -6320,7 +6372,7 @@ def resolve( # Priority 2: Installed presets (sorted by priority — lower number wins) if not skip_presets and self.presets_dir.exists(): for pack_id, _metadata in self._get_all_presets_by_priority(): - pack_dir = self.presets_dir / pack_id + pack_dir = self._preset_dir(pack_id) # The preset manifest is authoritative: if it declares this # template with an explicit ``file:``, resolve to that path — # and do NOT fall back to convention when it's missing, to @@ -6521,9 +6573,9 @@ def resolve_with_source( if str(self.overrides_dir) in resolved_str: return {"path": resolved_str, "source": "project override"} - if str(self.presets_dir) in resolved_str and self.presets_dir.exists(): + if self.presets_dir.exists() or self.preset_overrides: for pack_id, metadata in self._get_all_presets_by_priority(): - pack_dir = self.presets_dir / pack_id + pack_dir = self._preset_dir(pack_id) try: resolved.relative_to(pack_dir) version = metadata.get("version", "?") @@ -6613,7 +6665,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: # Priority 2: Installed presets (sorted by priority — lower number = higher precedence) if self.presets_dir.exists(): for pack_id, metadata in self._get_all_presets_by_priority(): - pack_dir = self.presets_dir / pack_id + pack_dir = self._preset_dir(pack_id) # Read strategy and manifest file path from preset manifest strategy = "replace" manifest_has_strategy = False diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 60d89120e5..35349e327b 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -746,15 +746,20 @@ def preset_update( constitution_after = ( constitution_path.read_bytes() if constitution_path.exists() else None ) - constitution_diff = diff.get("_constitution_changed", False) or any( - entry["identity"] == ("constitution-template", "template") - for category in ("added", "removed", "changed") - for entry in diff[category] - ) if dry_run: sync_metadata = manager.registry.get("constitution-sync") + target_enabled = metadata.get("enabled", True) + prospective_constitution = diff.get( + "_constitution_content_after" + ) + prospective_bytes = ( + prospective_constitution + if isinstance(prospective_constitution, bytes) + else None + ) constitution_can_reconcile = ( bool(diff.get("_constitution_layer")) + and target_enabled and isinstance(sync_metadata, dict) and sync_metadata.get("enabled", True) and ( @@ -769,10 +774,10 @@ def preset_update( constitution_status = ( "constitution change planned" if constitution_can_reconcile + and prospective_bytes is not None and ( not constitution_path.exists() - or constitution_diff - or effective_priority is not None + or constitution_before != prospective_bytes ) else "constitution unchanged" ) @@ -782,12 +787,18 @@ def preset_update( if constitution_before == constitution_after else "constitution reconciled" ) + generated_status = ( + ", generated files unchanged while disabled" + if not metadata.get("enabled", True) + else "" + ) console.print( f"[green]✓[/green] {safe_id}: {action} to v{manifest.version} " f"(+{added_commands} commands, -{removed_commands} commands, " f"~{changed_commands} commands, " f"{constitution_status}, " - f"priority {'kept at ' + str(metadata.get('priority', 10)) if effective_priority is None else 'set to ' + str(effective_priority)})" + f"priority {'kept at ' + str(metadata.get('priority', 10)) if effective_priority is None else 'set to ' + str(effective_priority)}" + f"{generated_status})" ) if dry_run: for category in ("added", "removed", "changed", "unchanged"): @@ -801,7 +812,10 @@ def preset_update( console.print( f" {category}: {', '.join(identities)}" ) - console.print(" planned actions: stage, validate, atomically swap, reconcile") + planned_actions = "stage, validate, atomically swap" + if metadata.get("enabled", True): + planned_actions += ", reconcile" + console.print(f" planned actions: {planned_actions}") else: _warn_unmet_extension_dependencies(manager, manifest) outcomes.append("updated") diff --git a/tests/test_presets.py b/tests/test_presets.py index 32d4416c7c..cc6da14014 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -14750,6 +14750,51 @@ def _command_preset(self, temp_dir, preset_id, body): ) return preset_dir + def _updated_command_source(self, preset_dir, version, body): + source = preset_dir.parent / f"updated-{preset_dir.name}" + shutil.copytree(preset_dir, source) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = version + manifest_path.write_text(yaml.safe_dump(data)) + (source / "commands" / "speckit.specify.md").write_text( + "---\ndescription: Test command\n---\n\n" + f"{body}\n" + ) + return source + + def _constitution_preset( + self, temp_dir, preset_id, body, *, strategy="replace", version="1.0.0" + ): + preset_dir = temp_dir / preset_id + (preset_dir / "templates").mkdir(parents=True) + (preset_dir / "templates" / "constitution-template.md").write_text(body) + (preset_dir / "preset.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": version, + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "template", + "name": "constitution-template", + "file": "templates/constitution-template.md", + "strategy": strategy, + } + ] + }, + } + ) + ) + return preset_dir + def test_manifest_diff_uses_name_and_type_identity(self, pack_dir): source = self._updated_source(pack_dir) old = PresetManifest(pack_dir / "preset.yml") @@ -14817,19 +14862,142 @@ def test_dry_run_validates_catalogue_expected_version( assert manager.registry.get("test-pack")["version"] == "1.0.0" - def test_disabled_preset_is_included_when_reconciling_its_update( - self, project_dir, pack_dir + def test_disabled_preset_update_preserves_existing_artifacts( + self, project_dir, temp_dir + ): + from specify_cli import save_init_options + + save_init_options( + project_dir, {"ai": "qwen", "ai_skills": False, "script": "sh"} + ) + (project_dir / ".qwen" / "commands").mkdir(parents=True) + preset_dir = self._command_preset( + temp_dir, "disabled-update-preset", "Original disabled body" + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.0") + manager.registry.update("disabled-update-preset", {"enabled": False}) + + command_file = project_dir / ".qwen" / "commands" / "speckit.specify.md" + command_before = command_file.read_bytes() + metadata_before = manager.registry.get("disabled-update-preset") + + save_init_options( + project_dir, {"ai": "claude", "ai_skills": True, "script": "sh"} + ) + source = self._updated_command_source( + preset_dir, "2.0.0", "Updated disabled body" + ) + + manager.update_from_directory( + source, "0.1.0", pack_id="disabled-update-preset" + ) + + metadata_after = manager.registry.get("disabled-update-preset") + assert command_file.read_bytes() == command_before + assert not (project_dir / ".claude" / "skills").exists() + assert "Updated disabled body" in ( + project_dir + / ".specify" + / "presets" + / "disabled-update-preset" + / "commands" + / "speckit.specify.md" + ).read_text(encoding="utf-8") + assert metadata_after["version"] == "2.0.0" + assert metadata_after["enabled"] is False + assert ( + metadata_after["registered_commands"] + == metadata_before["registered_commands"] + ) + assert metadata_after["registered_skills"] == metadata_before["registered_skills"] + assert not list( + (project_dir / ".specify" / "presets").glob( + ".disabled-update-preset.update-*" + ) + ) + + def test_disabled_preset_update_does_not_replace_enabled_winner( + self, project_dir, temp_dir ): + from specify_cli import save_init_options + + save_init_options( + project_dir, {"ai": "qwen", "ai_skills": False, "script": "sh"} + ) + (project_dir / ".qwen" / "commands").mkdir(parents=True) + disabled_dir = self._command_preset( + temp_dir, "disabled-winner-preset", "Disabled body v1" + ) + enabled_dir = self._command_preset( + temp_dir, "enabled-winner-preset", "Enabled body" + ) manager = PresetManager(project_dir) - manager.install_from_directory(pack_dir, "0.1.0") - manager.registry.update("test-pack", {"enabled": False}) + manager.install_from_directory(disabled_dir, "0.1.0", priority=1) + manager.registry.update("disabled-winner-preset", {"enabled": False}) + manager.install_from_directory(enabled_dir, "0.1.0", priority=5) + + command_file = project_dir / ".qwen" / "commands" / "speckit.specify.md" + assert "Enabled body" in command_file.read_text(encoding="utf-8") + source = self._updated_command_source( + disabled_dir, "2.0.0", "Disabled body v2" + ) + + manager.update_from_directory( + source, "0.1.0", pack_id="disabled-winner-preset" + ) + + assert "Enabled body" in command_file.read_text(encoding="utf-8") + assert "Disabled body v2" not in command_file.read_text( + encoding="utf-8" + ) + + def test_disabled_preset_update_retains_removed_command_for_cleanup( + self, project_dir, temp_dir + ): + from specify_cli import save_init_options + + save_init_options( + project_dir, {"ai": "qwen", "ai_skills": False, "script": "sh"} + ) + (project_dir / ".qwen" / "commands").mkdir(parents=True) + preset_dir = self._command_preset( + temp_dir, "disabled-removed-command", "Removed command body" + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.0") + manager.registry.update("disabled-removed-command", {"enabled": False}) + command_file = project_dir / ".qwen" / "commands" / "speckit.specify.md" + + source = preset_dir.parent / "updated-disabled-removed-command" + shutil.copytree(preset_dir, source) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = "2.0.0" + data["provides"]["templates"] = [ + { + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + } + ] + manifest_path.write_text(yaml.safe_dump(data)) + (source / "commands" / "speckit.specify.md").unlink() + (source / "templates").mkdir() + (source / "templates" / "spec-template.md").write_text("# Spec\n") + + manager.update_from_directory( + source, "0.1.0", pack_id="disabled-removed-command" + ) + + metadata = manager.registry.get("disabled-removed-command") + assert command_file.exists() + assert metadata["registered_commands"]["qwen"] == ["speckit.specify"] - resolver = PresetResolver(project_dir, include_disabled_id="test-pack") - layers = resolver.collect_all_layers("spec-template", "template") + manager.remove("disabled-removed-command") - assert layers - assert layers[0]["path"] == ( - project_dir / ".specify" / "presets" / "test-pack" / "templates" / "spec-template.md" + assert "Removed command body" not in command_file.read_text( + encoding="utf-8" ) def test_missing_referenced_file_leaves_installation_untouched( @@ -15256,6 +15424,322 @@ def test_cli_bulk_dry_run_ignores_priority_in_constitution_preview( "convention-constitution" )["priority"] == 7 + def test_cli_dry_run_ignores_shadowed_constitution_change( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + higher_source = _make_convention_constitution_preset(temp_dir) + higher_manifest = higher_source / "preset.yml" + higher_data = yaml.safe_load(higher_manifest.read_text()) + higher_data["preset"]["id"] = "higher-constitution" + higher_manifest.write_text(yaml.safe_dump(higher_data)) + (higher_source / "templates" / "constitution-template.md").write_text( + "# Higher Constitution\n" + ) + manager.install_from_directory(higher_source, "0.15.0", priority=1) + + lower_source = temp_dir / "lower-constitution" + shutil.copytree(higher_source, lower_source) + lower_manifest = lower_source / "preset.yml" + lower_data = yaml.safe_load(lower_manifest.read_text()) + lower_data["preset"]["id"] = "lower-constitution" + lower_manifest.write_text(yaml.safe_dump(lower_data)) + (lower_source / "templates" / "constitution-template.md").write_text( + "# Lower Constitution\n" + ) + manager.install_from_directory(lower_source, "0.15.0", priority=10) + + lower_data["preset"]["version"] = "2.0.0" + lower_manifest.write_text(yaml.safe_dump(lower_data)) + (lower_source / "templates" / "constitution-template.md").write_text( + "# Updated Lower Constitution\n" + ) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_before = constitution_path.read_bytes() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "lower-constitution", + "--dev", + str(lower_source), + "--priority", + "8", + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution unchanged" in result.output + assert constitution_path.read_bytes() == constitution_before + + def test_cli_dry_run_plans_new_winning_constitution_layer( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + lower_source = _make_convention_constitution_preset(temp_dir) + lower_manifest = lower_source / "preset.yml" + lower_data = yaml.safe_load(lower_manifest.read_text()) + lower_data["preset"]["id"] = "lower-constitution" + lower_manifest.write_text(yaml.safe_dump(lower_data)) + manager.install_from_directory(lower_source, "0.15.0", priority=5) + + target_source = self._command_preset( + temp_dir, "higher-new-constitution", "Command body" + ) + manager.install_from_directory(target_source, "0.15.0", priority=1) + (target_source / "templates").mkdir() + (target_source / "templates" / "constitution-template.md").write_text( + "# New Winning Constitution\n" + ) + target_manifest = target_source / "preset.yml" + target_data = yaml.safe_load(target_manifest.read_text()) + target_data["preset"]["version"] = "2.0.0" + target_manifest.write_text(yaml.safe_dump(target_data)) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_before = constitution_path.read_bytes() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "higher-new-constitution", + "--dev", + str(target_source), + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution change planned" in result.output + assert constitution_path.read_bytes() == constitution_before + + def test_cli_dry_run_plans_change_to_composed_constitution_layer( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + target_source = self._constitution_preset( + temp_dir, "composed-target", "# Target v1\n" + ) + manager.install_from_directory(target_source, "0.15.0", priority=5) + top_source = self._constitution_preset( + temp_dir, "composed-top", "# Top\n", strategy="prepend" + ) + manager.install_from_directory(top_source, "0.15.0", priority=1) + + target_manifest = target_source / "preset.yml" + target_data = yaml.safe_load(target_manifest.read_text()) + target_data["preset"]["version"] = "2.0.0" + target_manifest.write_text(yaml.safe_dump(target_data)) + (target_source / "templates" / "constitution-template.md").write_text( + "# Target v2\n" + ) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_before = constitution_path.read_bytes() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + runner = CliRunner() + + dry_run = runner.invoke( + app, + [ + "preset", + "update", + "composed-target", + "--dev", + str(target_source), + "--dry-run", + ], + ) + assert dry_run.exit_code == 0, dry_run.output + assert "constitution change planned" in dry_run.output + assert constitution_path.read_bytes() == constitution_before + + update = runner.invoke( + app, + [ + "preset", + "update", + "composed-target", + "--dev", + str(target_source), + ], + ) + assert update.exit_code == 0, update.output + assert "constitution reconciled" in update.output + + def test_cli_dry_run_plans_missing_shadowed_constitution( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + higher_source = self._constitution_preset( + temp_dir, "missing-higher", "# Higher\n" + ) + manager.install_from_directory(higher_source, "0.15.0", priority=1) + target_source = self._constitution_preset( + temp_dir, "missing-target", "# Target v1\n" + ) + manager.install_from_directory(target_source, "0.15.0", priority=5) + + target_manifest = target_source / "preset.yml" + target_data = yaml.safe_load(target_manifest.read_text()) + target_data["preset"]["version"] = "2.0.0" + target_manifest.write_text(yaml.safe_dump(target_data)) + (target_source / "templates" / "constitution-template.md").write_text( + "# Target v2\n" + ) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_path.unlink() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "missing-target", + "--dev", + str(target_source), + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution change planned" in result.output + assert not constitution_path.exists() + + def test_cli_dry_run_ignores_priority_change_with_same_constitution( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + source = self._constitution_preset( + temp_dir, "same-constitution", "# Same Constitution\n" + ) + manager.install_from_directory(source, "0.15.0", priority=10) + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(data)) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + constitution_before = constitution_path.read_bytes() + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + + result = CliRunner().invoke( + app, + [ + "preset", + "update", + "same-constitution", + "--dev", + str(source), + "--priority", + "8", + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + assert "constitution unchanged" in result.output + assert constitution_path.read_bytes() == constitution_before + + def test_cli_dry_run_compares_replace_constitution_as_bytes( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + install_constitution_sync_preset(manager) + source = self._constitution_preset( + temp_dir, "crlf-constitution", "# placeholder\n" + ) + crlf_content = b"# CRLF\r\n\r\nBody line\r\n" + (source / "templates" / "constitution-template.md").write_bytes( + crlf_content + ) + manager.install_from_directory(source, "0.15.0") + manifest_path = source / "preset.yml" + data = yaml.safe_load(manifest_path.read_text()) + data["preset"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(data)) + constitution_path = ( + project_dir / ".specify" / "memory" / "constitution.md" + ) + assert constitution_path.read_bytes() == crlf_content + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.15.0") + runner = CliRunner() + + dry_run = runner.invoke( + app, + [ + "preset", + "update", + "crlf-constitution", + "--dev", + str(source), + "--dry-run", + ], + ) + + assert dry_run.exit_code == 0, dry_run.output + assert "constitution unchanged" in dry_run.output + assert constitution_path.read_bytes() == crlf_content + + update = runner.invoke( + app, + [ + "preset", + "update", + "crlf-constitution", + "--dev", + str(source), + ], + ) + + assert update.exit_code == 0, update.output + assert "constitution unchanged" in update.output + assert constitution_path.read_bytes() == crlf_content + @pytest.mark.parametrize( "legacy_registry_owner", [None, "other", "target"] ) From edd4e3a19fabc201389596f412d80ea667ff0bab Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 12:32:27 +0100 Subject: [PATCH 04/12] fix: close remaining preset update review gaps Avoid recursive staging when a dev preset source contains the project staging directory, and make single catalogue reprioritisation fail clearly instead of silently skipping when the installed preset is newer than the catalogue. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 10 ++++- src/specify_cli/presets/_commands.py | 18 ++++++-- tests/test_presets.py | 66 ++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 5b31fb0603..9a4529d6f5 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4361,8 +4361,16 @@ def remove_swap_path(path: Path) -> None: elif path.exists(): shutil.rmtree(path) + def ignore_swap_paths(directory: str, names: List[str]) -> Set[str]: + directory_path = Path(directory) + return { + name + for name in names + if directory_path / name in (staging_dir, backup_dir) + } + try: - shutil.copytree(source_dir, staging_dir) + shutil.copytree(source_dir, staging_dir, ignore=ignore_swap_paths) generated_composition = staging_dir / ".composed" if generated_composition.is_symlink(): generated_composition.unlink() diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 35349e327b..f5d70b0c37 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -684,9 +684,21 @@ def preset_update( raise PresetError( f"catalog entry for preset '{item_id}' has an invalid version" ) from exc - if catalog_version < installed_version or ( - catalog_version == installed_version and effective_priority is None - ): + if catalog_version < installed_version: + if effective_priority is not None: + raise PresetError( + f"installed preset version {installed_version} is " + f"newer than catalogue version {catalog_version}; " + "use 'specify preset set-priority' to reprioritize " + "without downgrading" + ) + console.print( + f"[dim]• {safe_id}: Up to date, skipped " + f"(v{installed_version})[/dim]" + ) + outcomes.append("skipped") + continue + if catalog_version == installed_version and effective_priority is None: console.print( f"[dim]• {safe_id}: Up to date, skipped " f"(v{installed_version})[/dim]" diff --git a/tests/test_presets.py b/tests/test_presets.py index cc6da14014..f416bb924d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -15000,6 +15000,43 @@ def test_disabled_preset_update_retains_removed_command_for_cleanup( encoding="utf-8" ) + def test_dev_update_from_project_root_does_not_copy_staging_into_itself( + self, project_dir, temp_dir + ): + preset_dir = self._command_preset( + temp_dir, "project-root-update", "Original body" + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.0") + + shutil.copytree(preset_dir / "commands", project_dir / "commands") + (project_dir / "preset.yml").write_text( + (preset_dir / "preset.yml").read_text(encoding="utf-8"), + encoding="utf-8", + ) + data = yaml.safe_load((project_dir / "preset.yml").read_text()) + data["preset"]["version"] = "2.0.0" + (project_dir / "preset.yml").write_text(yaml.safe_dump(data)) + (project_dir / "commands" / "speckit.specify.md").write_text( + "---\ndescription: Test command\n---\n\nUpdated root body\n", + encoding="utf-8", + ) + + manager.update_from_directory( + project_dir, "0.1.0", pack_id="project-root-update" + ) + + installed = ( + project_dir + / ".specify" + / "presets" + / "project-root-update" + ) + assert "Updated root body" in ( + installed / "commands" / "speckit.specify.md" + ).read_text(encoding="utf-8") + assert not list(installed.glob("**/.project-root-update.update-*")) + def test_missing_referenced_file_leaves_installation_untouched( self, project_dir, pack_dir ): @@ -16182,6 +16219,35 @@ def test_cli_single_priority_update_runs_at_equal_catalogue_version( assert "updated" in result.output assert PresetManager(project_dir).registry.get("test-pack")["priority"] == 3 + def test_cli_single_priority_update_rejects_newer_installed_catalogue( + self, project_dir, pack_dir, monkeypatch + ): + """A single catalogue update must not silently ignore requested priority + when applying it would require a downgrade.""" + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=7) + manager.registry.update("test-pack", {"version": "5.0.0"}) + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "2.0.0", "bundled": True}}, + {"test-pack": self._updated_source(pack_dir)}, + ) + + result = CliRunner().invoke( + app, ["preset", "update", "test-pack", "--priority", "3"] + ) + + assert result.exit_code == 1, result.output + assert "newer than catalogue" in result.output + assert "preset set-priority" in result.output + metadata = PresetManager(project_dir).registry.get("test-pack") + assert metadata["version"] == "5.0.0" + assert metadata["priority"] == 7 + def test_cli_bulk_unresolvable_source_reports_clearly( self, project_dir, pack_dir, monkeypatch ): From edabf2efcd42ece029638b156e3bd678d6500033 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 12:40:17 +0100 Subject: [PATCH 05/12] fix: exclude preset state from root dev staging When a development preset source is the project root, avoid copying .specify/presets into the staged preset so update staging does not include registry, cache, or other installed preset state. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 6 +++--- tests/test_presets.py | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 9a4529d6f5..69ac840c00 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4361,16 +4361,16 @@ def remove_swap_path(path: Path) -> None: elif path.exists(): shutil.rmtree(path) - def ignore_swap_paths(directory: str, names: List[str]) -> Set[str]: + def ignore_staging_state(directory: str, names: List[str]) -> Set[str]: directory_path = Path(directory) return { name for name in names - if directory_path / name in (staging_dir, backup_dir) + if directory_path / name in (self.presets_dir, staging_dir, backup_dir) } try: - shutil.copytree(source_dir, staging_dir, ignore=ignore_swap_paths) + shutil.copytree(source_dir, staging_dir, ignore=ignore_staging_state) generated_composition = staging_dir / ".composed" if generated_composition.is_symlink(): generated_composition.unlink() diff --git a/tests/test_presets.py b/tests/test_presets.py index f416bb924d..5000dbc6bc 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -15035,6 +15035,8 @@ def test_dev_update_from_project_root_does_not_copy_staging_into_itself( assert "Updated root body" in ( installed / "commands" / "speckit.specify.md" ).read_text(encoding="utf-8") + assert not (installed / ".specify" / "presets").exists() + assert not (installed / ".registry").exists() assert not list(installed.glob("**/.project-root-update.update-*")) def test_missing_referenced_file_leaves_installation_untouched( From e2c6e81668a185aebd570a021d6793dfa29bf208 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 12:52:55 +0100 Subject: [PATCH 06/12] fix: skip project specify state during root dev staging Exclude the project .specify directory when a development preset update stages from the project root, avoiding recursive preset state and Windows lock-file copy failures. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 8 +++++++- tests/test_presets.py | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 69ac840c00..279dbd49d7 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4366,7 +4366,13 @@ def ignore_staging_state(directory: str, names: List[str]) -> Set[str]: return { name for name in names - if directory_path / name in (self.presets_dir, staging_dir, backup_dir) + if directory_path / name + in ( + self.project_root / ".specify", + self.presets_dir, + staging_dir, + backup_dir, + ) } try: diff --git a/tests/test_presets.py b/tests/test_presets.py index 5000dbc6bc..dc962f40df 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -15035,7 +15035,7 @@ def test_dev_update_from_project_root_does_not_copy_staging_into_itself( assert "Updated root body" in ( installed / "commands" / "speckit.specify.md" ).read_text(encoding="utf-8") - assert not (installed / ".specify" / "presets").exists() + assert not (installed / ".specify").exists() assert not (installed / ".registry").exists() assert not list(installed.glob("**/.project-root-update.update-*")) From c8404a49bea4d2e36a42eb66d210126c4f325b30 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:13:38 +0100 Subject: [PATCH 07/12] fix: refresh disabled preset updates on enable Refresh generated commands and skills when a preset is re-enabled after a disabled update, removing stale artefacts before re-registering the active agent output. Use the locally bundled preset version for bundled updates and fail clearly when the installed spec-kit package lags behind the catalogue. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/_commands.py | 142 ++++++++++++++++-- tests/test_presets.py | 206 +++++++++++++++++++++++++++ 2 files changed, 339 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index f5d70b0c37..d580313762 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -133,6 +133,23 @@ def _download_preset_archive(url: str, error_type: type[Exception]) -> Path: return _write_preset_archive(data, final_url, content_type, error_type) +def _bundled_update_source(preset_id: str): + """Locate a bundled preset and return its parsed local version.""" + from packaging import version as pkg_version + + from .. import _locate_bundled_preset + from . import PresetManifest, PresetValidationError + + bundled_dir = _locate_bundled_preset(preset_id) + if bundled_dir is None: + return None, None + try: + manifest = PresetManifest(bundled_dir / "preset.yml") + return bundled_dir, pkg_version.Version(manifest.version) + except (PresetValidationError, pkg_version.InvalidVersion, OSError): + return None, None + + def _cleanup_archive(path: Path | None) -> None: if path is None: return @@ -483,7 +500,6 @@ def preset_update( from packaging import version as pkg_version from .. import ( - _locate_bundled_preset, _require_specify_project, get_speckit_version, ) @@ -565,12 +581,19 @@ def preset_update( outcomes.append("skipped") continue if pack_info.get("bundled") and not pack_info.get("download_url"): - source_path = _locate_bundled_preset(item_id) - if source_path is None: + source_path, bundled_version = _bundled_update_source(item_id) + if source_path is None or bundled_version < catalog_version: + local_desc = ( + f"only ships v{bundled_version}" + if source_path is not None + else "does not ship a local copy" + ) raise PresetError( - f"Preset '{item_id}' is bundled with spec-kit but " - "could not be found in the installed package" + f"preset v{catalog_version} is available, but this " + f"spec-kit release {local_desc}; upgrade spec-kit, " + "then rerun 'specify preset update'" ) + pack_info = {**pack_info, "version": str(bundled_version)} manager.update_from_directory( source_path, speckit_version, @@ -706,9 +729,23 @@ def preset_update( outcomes.append("skipped") continue if pack_info.get("bundled") and not pack_info.get("download_url"): - source_path = catalog_sources.get(item_id) or _locate_bundled_preset( - item_id - ) + bundled_source = catalog_sources.get(item_id) + if bundled_source is not None: + source_path = bundled_source + else: + source_path, bundled_version = _bundled_update_source(item_id) + if source_path is None or bundled_version < catalog_version: + local_desc = ( + f"only ships v{bundled_version}" + if source_path is not None + else "does not ship a local copy" + ) + raise PresetError( + f"preset v{catalog_version} is available, but this " + f"spec-kit release {local_desc}; upgrade spec-kit, " + "then rerun 'specify preset update'" + ) + pack_info = {**pack_info, "version": str(bundled_version)} if source_path is None: raise PresetError( f"Preset '{item_id}' is bundled with spec-kit but " @@ -1150,7 +1187,8 @@ def preset_enable( ): """Enable a disabled preset.""" from .. import _require_specify_project - from . import PresetManager + from .._init_options import resolve_active_agent_for_registration + from . import PresetManager, PresetManifest project_root = _require_specify_project() manager = PresetManager(project_root) @@ -1172,6 +1210,92 @@ def preset_enable( # Enable the preset manager.registry.update(preset_id, {"enabled": True}) + pack_dir = manager.presets_dir / preset_id + manifest = PresetManifest(pack_dir / "preset.yml") + current_command_names = { + name + for template in manifest.templates + if template.get("type") == "command" + for name in ( + [template.get("name")] + + [ + alias + for alias in template.get("aliases", []) + if isinstance(alias, str) + ] + ) + if isinstance(name, str) + } + stale_commands = {} + registered_commands = metadata.get("registered_commands", {}) + if isinstance(registered_commands, dict): + for agent, names in registered_commands.items(): + if not isinstance(agent, str) or not isinstance(names, list): + continue + stale_names = [ + name + for name in names + if isinstance(name, str) and name not in current_command_names + ] + if stale_names: + stale_commands[agent] = stale_names + if stale_commands: + manager._unregister_commands(stale_commands) + updated_commands = {} + for agent, names in registered_commands.items(): + if not isinstance(agent, str) or not isinstance(names, list): + continue + retained = [ + name + for name in names + if isinstance(name, str) + and name not in stale_commands.get(agent, []) + ] + if retained: + updated_commands[agent] = retained + manager.registry.update(preset_id, {"registered_commands": updated_commands}) + current_skill_names = { + skill_name + for command_name in current_command_names + for skill_name in manager._skill_names_for_command(command_name) + } + raw_registered_skills = metadata.get("registered_skills", {}) + registered_skills = manager._normalize_registered_skills(raw_registered_skills) + stale_skills = {} + for agent, names in registered_skills.items(): + if not isinstance(agent, str) or not isinstance(names, list): + continue + stale_names = [ + name + for name in names + if isinstance(name, str) and name not in current_skill_names + ] + if stale_names: + stale_skills[agent] = stale_names + if stale_skills: + manager._unregister_skills( + stale_skills, + pack_dir, + restore_from_bundled_core=True, + ) + updated_skills = {} + for agent, names in registered_skills.items(): + retained = [ + name for name in names if name not in stale_skills.get(agent, []) + ] + if retained: + updated_skills[agent] = retained + manager.registry.update(preset_id, {"registered_skills": updated_skills}) + + active_agent = resolve_active_agent_for_registration(project_root) + if isinstance(active_agent, str): + manager.register_enabled_presets_for_agent(active_agent) + if stale_commands: + manager._reconcile_composed_commands( + sorted({name for names in stale_commands.values() for name in names}), + extra_agents=set(stale_commands), + ) + manager.reconcile_constitution( f"Failed to reconcile constitution after enabling preset {preset_id}" ) diff --git a/tests/test_presets.py b/tests/test_presets.py index dc962f40df..cd0c075ebc 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -14750,6 +14750,40 @@ def _command_preset(self, temp_dir, preset_id, body): ) return preset_dir + def _multi_command_preset(self, temp_dir, preset_id, commands, version="1.0.0"): + preset_dir = temp_dir / preset_id + (preset_dir / "commands").mkdir(parents=True) + templates = [] + for command_name, body in commands.items(): + file_name = f"{command_name}.md" + (preset_dir / "commands" / file_name).write_text( + "---\ndescription: Test command\n---\n\n" + f"{body}\n" + ) + templates.append( + { + "type": "command", + "name": command_name, + "file": f"commands/{file_name}", + } + ) + (preset_dir / "preset.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": version, + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": templates}, + } + ) + ) + return preset_dir + def _updated_command_source(self, preset_dir, version, body): source = preset_dir.parent / f"updated-{preset_dir.name}" shutil.copytree(preset_dir, source) @@ -15000,6 +15034,134 @@ def test_disabled_preset_update_retains_removed_command_for_cleanup( encoding="utf-8" ) + def test_enable_after_disabled_update_refreshes_generated_commands( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app, save_init_options + + save_init_options( + project_dir, {"ai": "qwen", "ai_skills": False, "script": "sh"} + ) + (project_dir / ".qwen" / "commands").mkdir(parents=True) + source_v1 = self._multi_command_preset( + temp_dir, + "enable-refresh-preset", + { + "speckit.specify": "Specify v1", + "speckit.removed": "Removed v1", + }, + ) + manager = PresetManager(project_dir) + manager.install_from_directory(source_v1, "0.1.0") + manager.registry.update("enable-refresh-preset", {"enabled": False}) + + source_v2 = self._multi_command_preset( + temp_dir, + "enable-refresh-preset-v2", + { + "speckit.specify": "Specify v2", + "speckit.added": "Added v2", + }, + version="2.0.0", + ) + data = yaml.safe_load((source_v2 / "preset.yml").read_text()) + data["preset"]["id"] = "enable-refresh-preset" + (source_v2 / "preset.yml").write_text(yaml.safe_dump(data)) + + manager.update_from_directory( + source_v2, "0.1.0", pack_id="enable-refresh-preset" + ) + + commands_dir = project_dir / ".qwen" / "commands" + assert "Specify v1" in ( + commands_dir / "speckit.specify.md" + ).read_text(encoding="utf-8") + assert (commands_dir / "speckit.removed.md").exists() + assert not (commands_dir / "speckit.added.md").exists() + + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + result = CliRunner().invoke(app, ["preset", "enable", "enable-refresh-preset"]) + + assert result.exit_code == 0, result.output + assert "Specify v2" in ( + commands_dir / "speckit.specify.md" + ).read_text(encoding="utf-8") + assert "Added v2" in ( + commands_dir / "speckit.added.md" + ).read_text(encoding="utf-8") + assert not (commands_dir / "speckit.removed.md").exists() + registered = PresetManager(project_dir).registry.get( + "enable-refresh-preset" + )["registered_commands"]["qwen"] + assert "speckit.added" in registered + assert "speckit.removed" not in registered + + def test_enable_after_disabled_update_refreshes_generated_skills( + self, project_dir, temp_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app, save_init_options + + save_init_options( + project_dir, {"ai": "claude", "ai_skills": True, "script": "sh"} + ) + skills_dir = project_dir / ".claude" / "skills" + skills_dir.mkdir(parents=True) + source_v1 = self._multi_command_preset( + temp_dir, + "enable-skill-refresh-preset", + { + "speckit.specify": "Specify skill v1", + "speckit.removed": "Removed skill v1", + }, + ) + manager = PresetManager(project_dir) + manager.install_from_directory(source_v1, "0.1.0") + manager.registry.update("enable-skill-refresh-preset", {"enabled": False}) + + source_v2 = self._multi_command_preset( + temp_dir, + "enable-skill-refresh-preset-v2", + { + "speckit.specify": "Specify skill v2", + "speckit.added": "Added skill v2", + }, + version="2.0.0", + ) + data = yaml.safe_load((source_v2 / "preset.yml").read_text()) + data["preset"]["id"] = "enable-skill-refresh-preset" + (source_v2 / "preset.yml").write_text(yaml.safe_dump(data)) + + manager.update_from_directory( + source_v2, "0.1.0", pack_id="enable-skill-refresh-preset" + ) + + assert "Specify skill v1" in ( + skills_dir / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + assert (skills_dir / "speckit-removed" / "SKILL.md").exists() + assert not (skills_dir / "speckit-added" / "SKILL.md").exists() + + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + result = CliRunner().invoke( + app, ["preset", "enable", "enable-skill-refresh-preset"] + ) + + assert result.exit_code == 0, result.output + assert "Specify skill v2" in ( + skills_dir / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + assert "Added skill v2" in ( + skills_dir / "speckit-added" / "SKILL.md" + ).read_text(encoding="utf-8") + assert not (skills_dir / "speckit-removed").exists() + registered = PresetManager(project_dir).registry.get( + "enable-skill-refresh-preset" + )["registered_skills"]["claude"] + assert "speckit-added" in registered + assert "speckit-removed" not in registered + def test_dev_update_from_project_root_does_not_copy_staging_into_itself( self, project_dir, temp_dir ): @@ -16250,6 +16412,50 @@ def test_cli_single_priority_update_rejects_newer_installed_catalogue( assert metadata["version"] == "5.0.0" assert metadata["priority"] == 7 + def test_cli_bundled_update_uses_local_bundled_version_when_newer( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + local_source = self._updated_source(pack_dir, version="3.0.0") + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "2.0.0", "bundled": True}}, + {"test-pack": local_source}, + ) + + result = CliRunner().invoke(app, ["preset", "update", "test-pack"]) + + assert result.exit_code == 0, result.output + assert "updated to v3.0.0" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "3.0.0" + + def test_cli_bundled_update_blocks_when_local_bundled_copy_lags( + self, project_dir, pack_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0") + local_source = self._updated_source(pack_dir, version="1.5.0") + self._bulk_cli_env( + project_dir, + monkeypatch, + {"test-pack": {"version": "2.0.0", "bundled": True}}, + {"test-pack": local_source}, + ) + + result = CliRunner().invoke(app, ["preset", "update", "test-pack"]) + + assert result.exit_code == 1, result.output + assert "upgrade spec-kit" in result.output + assert PresetManager(project_dir).registry.get("test-pack")["version"] == "1.0.0" + def test_cli_bulk_unresolvable_source_reports_clearly( self, project_dir, pack_dir, monkeypatch ): From 2797ce7ca8709ba40286a2cd3f22f15d708393c1 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:41:08 +0100 Subject: [PATCH 08/12] fix: validate manifest and legacy state before enabling preset - Validate the installed preset.yml before flipping enabled=True, so a missing/corrupt manifest fails closed with the preset left disabled instead of enabling first and erroring after mutating the registry. - Fix legacy flat-list registered_skills handling: use _infer_legacy_skill_provenance when the raw registry value is a list, so stale-skill cleanup on enable correctly identifies per-agent ownership instead of silently doing nothing. - Fall back to registering commands/skills for every detected agent when resolve_active_agent_for_registration returns MISSING_INIT_OPTIONS_FILE (legacy pre-init-options projects), instead of silently skipping all rescaffolding, matching install-time behaviour elsewhere in the codebase. Adds regression tests for all three scenarios under TestPresetEnableDisable. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/_commands.py | 72 ++++++++- tests/test_presets.py | 220 +++++++++++++++++++++++++++ 2 files changed, 284 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index d580313762..b50410371d 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1186,9 +1186,14 @@ def preset_enable( preset_id: str = typer.Argument(help="Preset ID to enable"), ): """Enable a disabled preset.""" + import copy + from .. import _require_specify_project - from .._init_options import resolve_active_agent_for_registration - from . import PresetManager, PresetManifest + from .._init_options import ( + MISSING_INIT_OPTIONS_FILE, + resolve_active_agent_for_registration, + ) + from . import PresetManager, PresetManifest, PresetValidationError project_root = _require_specify_project() manager = PresetManager(project_root) @@ -1208,10 +1213,23 @@ def preset_enable( console.print(f"[yellow]Preset '{preset_id}' is already enabled[/yellow]") raise typer.Exit(0) + pack_dir = manager.presets_dir / preset_id + # Validate the installed manifest *before* flipping `enabled`: a missing + # or corrupt preset.yml must fail closed with the preset still disabled, + # not raise after the registry has already been mutated. + try: + manifest = PresetManifest(pack_dir / "preset.yml") + except PresetValidationError as e: + console.print( + f"[red]Error:[/red] Cannot enable '{preset_id}': installed " + f"manifest is invalid ({_escape_markup(str(e))})" + ) + raise typer.Exit(1) + # Enable the preset manager.registry.update(preset_id, {"enabled": True}) - pack_dir = manager.presets_dir / preset_id - manifest = PresetManifest(pack_dir / "preset.yml") + resolved_agent = resolve_active_agent_for_registration(project_root) + fallback_agent = resolved_agent if isinstance(resolved_agent, str) else "" current_command_names = { name for template in manifest.templates @@ -1260,7 +1278,19 @@ def preset_enable( for skill_name in manager._skill_names_for_command(command_name) } raw_registered_skills = metadata.get("registered_skills", {}) - registered_skills = manager._normalize_registered_skills(raw_registered_skills) + if isinstance(raw_registered_skills, list): + # Legacy flat-list value: infer real per-agent ownership from + # on-disk provenance rather than dropping it, otherwise a removed + # command's skill can never be identified as stale here. + registered_skills = manager._infer_legacy_skill_provenance( + [name for name in raw_registered_skills if isinstance(name, str)], + preset_id, + fallback_agent, + ) + else: + registered_skills = manager._normalize_registered_skills( + raw_registered_skills, fallback_agent=fallback_agent + ) stale_skills = {} for agent, names in registered_skills.items(): if not isinstance(agent, str) or not isinstance(names, list): @@ -1287,9 +1317,35 @@ def preset_enable( updated_skills[agent] = retained manager.registry.update(preset_id, {"registered_skills": updated_skills}) - active_agent = resolve_active_agent_for_registration(project_root) - if isinstance(active_agent, str): - manager.register_enabled_presets_for_agent(active_agent) + if resolved_agent is MISSING_INIT_OPTIONS_FILE: + # Legacy pre-init-options project: there is no single "active + # agent" to target, so mirror install-time behaviour and + # register/re-register this preset's commands and skills for + # every agent directory actually present on disk, merging the + # fresh result into the stored registry state. + fresh_commands = manager._register_commands(manifest, pack_dir) + if fresh_commands: + current_metadata = manager.registry.get(preset_id) or {} + merged = copy.deepcopy(current_metadata.get("registered_commands") or {}) + for agent, names in fresh_commands.items(): + existing = merged.get(agent, []) + merged[agent] = existing + [ + name for name in names if name not in existing + ] + manager.registry.update(preset_id, {"registered_commands": merged}) + + fresh_skills = manager._register_skills(manifest, pack_dir) + if fresh_skills: + current_metadata = manager.registry.get(preset_id) or {} + merged_skills = copy.deepcopy(current_metadata.get("registered_skills") or {}) + for agent, names in fresh_skills.items(): + existing = merged_skills.get(agent, []) + merged_skills[agent] = existing + [ + name for name in names if name not in existing + ] + manager.registry.update(preset_id, {"registered_skills": merged_skills}) + elif isinstance(resolved_agent, str): + manager.register_enabled_presets_for_agent(resolved_agent) if stale_commands: manager._reconcile_composed_commands( sorted({name for names in stale_commands.values() for name in names}), diff --git a/tests/test_presets.py b/tests/test_presets.py index cd0c075ebc..c7e22ef418 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11329,6 +11329,226 @@ def test_enable_corrupted_registry_entry(self, project_dir, pack_dir): assert result.exit_code == 1 assert "corrupted state" in result.output.lower() + def test_enable_fails_closed_on_corrupt_installed_manifest( + self, project_dir, pack_dir + ): + """Enable must not flip `enabled` when the installed manifest is invalid. + + A missing or corrupt preset.yml must be validated before the + registry is mutated, otherwise the command fails after the preset + has already been marked enabled with unreconciled artifacts (#4441). + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + manager.registry.update("test-pack", {"enabled": False}) + + installed_manifest = ( + project_dir / ".specify" / "presets" / "test-pack" / "preset.yml" + ) + installed_manifest.write_text("not: [valid") + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["preset", "enable", "test-pack"]) + + assert result.exit_code != 0 + assert "invalid" in result.output.lower() + manager2 = PresetManager(project_dir) + assert manager2.registry.get("test-pack")["enabled"] is False + + def test_enable_reconciles_legacy_flat_list_registered_skills( + self, project_dir, temp_dir + ): + """A legacy flat-list `registered_skills` value must still let enable + detect and clean up a skill whose command was removed while disabled. + + `_normalize_registered_skills()` drops a legacy flat list when no + fallback agent is supplied, which would otherwise skip cleanup of + the removed command's stale skill entirely (#4441). + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app, save_init_options + + save_init_options( + project_dir, {"ai": "claude", "ai_skills": True, "script": "sh"} + ) + skills_dir = project_dir / ".claude" / "skills" + skills_dir.mkdir(parents=True) + + preset_dir = temp_dir / "legacy-skill-preset" + (preset_dir / "commands").mkdir(parents=True) + for name, body in ( + ("speckit.keep", "Keep body"), + ("speckit.drop", "Drop body"), + ): + (preset_dir / "commands" / f"{name}.md").write_text( + f"---\ndescription: {name}\n---\n\n{body}\n" + ) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": "legacy-skill-preset", + "name": "legacy-skill-preset", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": "speckit.keep", + "file": "commands/speckit.keep.md", + }, + { + "type": "command", + "name": "speckit.drop", + "file": "commands/speckit.drop.md", + }, + ] + }, + } + (preset_dir / "preset.yml").write_text(yaml.safe_dump(manifest_data)) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.0") + metadata = manager.registry.get("legacy-skill-preset") + assert set(metadata["registered_skills"]["claude"]) == { + "speckit-keep", + "speckit-drop", + } + + # Simulate a legacy pre-#2948 registry: flatten registered_skills to + # a bare list, matching what old registries stored before per-agent + # provenance existed. + manager.registry.update( + "legacy-skill-preset", + {"registered_skills": ["speckit-keep", "speckit-drop"]}, + ) + manager.registry.update("legacy-skill-preset", {"enabled": False}) + + # Simulate a disabled update that dropped "speckit.drop" from the + # installed manifest. + installed_manifest_path = ( + project_dir + / ".specify" + / "presets" + / "legacy-skill-preset" + / "preset.yml" + ) + installed_manifest = yaml.safe_load(installed_manifest_path.read_text()) + installed_manifest["provides"]["templates"] = [ + t + for t in installed_manifest["provides"]["templates"] + if t["name"] != "speckit.drop" + ] + installed_manifest_path.write_text(yaml.safe_dump(installed_manifest)) + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "enable", "legacy-skill-preset"] + ) + + assert result.exit_code == 0, result.output + assert not (skills_dir / "speckit-drop").exists() + assert (skills_dir / "speckit-keep" / "SKILL.md").exists() + registered = PresetManager(project_dir).registry.get( + "legacy-skill-preset" + )["registered_skills"] + assert "speckit-drop" not in registered.get("claude", []) + assert "speckit-keep" in registered.get("claude", []) + + def test_enable_legacy_project_refreshes_all_detected_agents( + self, project_dir, temp_dir + ): + """A legacy pre-init-options project has no single active agent; + enabling a preset updated while disabled must still refresh every + agent directory actually present on disk, not silently do nothing + (#4441). + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + claude_skills = project_dir / ".claude" / "skills" + claude_skills.mkdir(parents=True) + gemini_commands = project_dir / ".gemini" / "commands" + gemini_commands.mkdir(parents=True) + + def build_preset(preset_id, body, version="1.0.0"): + preset_dir = temp_dir / preset_id + (preset_dir / "commands").mkdir(parents=True) + (preset_dir / "commands" / "speckit.specify.md").write_text( + f"---\ndescription: test\n---\n\n{body}\n" + ) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": version, + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + }, + ] + }, + } + (preset_dir / "preset.yml").write_text(yaml.safe_dump(manifest_data)) + return preset_dir + + source_v1 = build_preset("legacy-refresh-preset", "Specify v1") + manager = PresetManager(project_dir) + manager.install_from_directory(source_v1, "0.1.0") + + assert "Specify v1" in ( + gemini_commands / "speckit.specify.toml" + ).read_text(encoding="utf-8") + assert "Specify v1" in ( + claude_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + + manager.registry.update("legacy-refresh-preset", {"enabled": False}) + + source_v2 = build_preset( + "legacy-refresh-preset-v2", "Specify v2", version="2.0.0" + ) + data = yaml.safe_load((source_v2 / "preset.yml").read_text()) + data["preset"]["id"] = "legacy-refresh-preset" + (source_v2 / "preset.yml").write_text(yaml.safe_dump(data)) + manager.update_from_directory( + source_v2, "0.1.0", pack_id="legacy-refresh-preset" + ) + + # Confirm the disabled update deliberately left generated artifacts + # untouched while disabled. + assert "Specify v1" in ( + gemini_commands / "speckit.specify.toml" + ).read_text(encoding="utf-8") + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "enable", "legacy-refresh-preset"] + ) + + assert result.exit_code == 0, result.output + assert "Specify v2" in ( + gemini_commands / "speckit.specify.toml" + ).read_text(encoding="utf-8") + assert "Specify v2" in ( + claude_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + def test_disable_corrupted_registry_entry(self, project_dir, pack_dir): """Test disable fails gracefully for corrupted registry entry.""" from typer.testing import CliRunner From 901deffedb8c7b2a49e16b9dfdf2c64f46ca69fc Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:04:09 +0100 Subject: [PATCH 09/12] fix: rollback and reconcile skills-before-commands on preset enable - Snapshot the pre-enable registry entry and wrap the reconciliation body in try/except: any failure restores the registry to its prior disabled state instead of leaving the preset marked enabled with only partially refreshed artifacts. - Reorder preset_enable to unregister/reconcile stale skills before stale commands, mirroring PresetManager.remove(). Native skill-only agents (claude, codex) track the same physical SKILL.md file under both registered_commands and registered_skills; unregistering commands first (no core-fallback there) could delete that file before the skills pass ever got a chance to restore/reconcile it. - Filter stale commands already covered by the skills pass out of the _unregister_commands call for native skill-only agents, avoiding a double-delete of the same file. - Pass _unregister_skills's returned affected directories through to _reconcile_skills, so a surviving lower-priority preset's override is reconciled back in instead of left showing core/extension content. - Add _command_name_for_skill_name (inverse of _skill_names_for_command) and use it to derive command names for stale skills, so skill-only reconciliation still runs even when no commands went stale. Adds regression tests for registry rollback on reconciliation failure and for surviving lower-priority preset reconciliation across historical and active agents. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 16 ++ src/specify_cli/presets/_commands.py | 323 ++++++++++++++++++--------- tests/test_presets.py | 210 ++++++++++++++++- 3 files changed, 437 insertions(+), 112 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 279dbd49d7..4107d662bf 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2939,6 +2939,22 @@ def _skill_names_for_command(cmd_name: str) -> tuple[str, str]: legacy_skill_name = f"speckit.{raw_short_name}" return modern_skill_name, legacy_skill_name + @staticmethod + def _command_name_for_skill_name(skill_name: str) -> Optional[str]: + """Return the command name a skill directory name derives from. + + Inverse of `_skill_names_for_command`: "speckit-specify" and + "speckit.specify" both map back to "speckit.specify". Returns None + for names that don't follow the skill-naming convention. + """ + if skill_name.startswith("speckit-"): + short_name = skill_name[len("speckit-"):] + elif skill_name.startswith("speckit."): + short_name = skill_name[len("speckit."):] + else: + return None + return f"speckit.{short_name}" + @staticmethod def _skill_title_from_command(cmd_name: str) -> str: """Return a human-friendly title for a skill command name.""" diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index b50410371d..28fafaa527 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1226,135 +1226,236 @@ def preset_enable( ) raise typer.Exit(1) - # Enable the preset + # Enable the preset. Snapshot the pre-enable registry entry first: if + # anything below raises, the registry is restored to this snapshot so a + # failed enable never leaves the preset marked enabled with only + # partially refreshed artifacts (fail-closed, matching the manifest + # validation above). Filesystem writes already performed before the + # failure are not unwound — the registry itself is what CLI/tests treat + # as the source of truth for enabled/disabled state. + registry_before = copy.deepcopy(metadata) manager.registry.update(preset_id, {"enabled": True}) - resolved_agent = resolve_active_agent_for_registration(project_root) - fallback_agent = resolved_agent if isinstance(resolved_agent, str) else "" - current_command_names = { - name - for template in manifest.templates - if template.get("type") == "command" - for name in ( - [template.get("name")] - + [ - alias - for alias in template.get("aliases", []) - if isinstance(alias, str) - ] - ) - if isinstance(name, str) - } - stale_commands = {} - registered_commands = metadata.get("registered_commands", {}) - if isinstance(registered_commands, dict): - for agent, names in registered_commands.items(): + try: + resolved_agent = resolve_active_agent_for_registration(project_root) + fallback_agent = resolved_agent if isinstance(resolved_agent, str) else "" + current_command_names = { + name + for template in manifest.templates + if template.get("type") == "command" + for name in ( + [template.get("name")] + + [ + alias + for alias in template.get("aliases", []) + if isinstance(alias, str) + ] + ) + if isinstance(name, str) + } + current_skill_names = { + skill_name + for command_name in current_command_names + for skill_name in manager._skill_names_for_command(command_name) + } + + # Compute stale skills *before* stale commands (mirrors + # PresetManager.remove()): for a native skill-only agent (extension + # == "/SKILL.md", e.g. claude/codex), `_register_commands` writes + # the exact same SKILL.md file `_register_skills` does, so + # `registered_commands` and `registered_skills` both track that + # agent for the same on-disk file. Restoring/reconciling via + # `_unregister_skills` first, then excluding that coverage from the + # commands pass below, avoids `_unregister_commands` deleting the + # file outright (no core-fallback there) before `_unregister_skills` + # ever gets a chance to restore or reconcile it. + raw_registered_skills = metadata.get("registered_skills", {}) + if isinstance(raw_registered_skills, list): + # Legacy flat-list value: infer real per-agent ownership from + # on-disk provenance rather than dropping it, otherwise a removed + # command's skill can never be identified as stale here. + registered_skills = manager._infer_legacy_skill_provenance( + [name for name in raw_registered_skills if isinstance(name, str)], + preset_id, + fallback_agent, + ) + else: + registered_skills = manager._normalize_registered_skills( + raw_registered_skills, fallback_agent=fallback_agent + ) + stale_skills = {} + for agent, names in registered_skills.items(): if not isinstance(agent, str) or not isinstance(names, list): continue stale_names = [ name for name in names - if isinstance(name, str) and name not in current_command_names + if isinstance(name, str) and name not in current_skill_names ] if stale_names: - stale_commands[agent] = stale_names - if stale_commands: - manager._unregister_commands(stale_commands) - updated_commands = {} + stale_skills[agent] = stale_names + # Populated by _unregister_skills below with the directories it + # restored to extension/core content, so a surviving lower-priority + # preset that should actually win those commands can be reconciled + # back in afterwards instead of being left showing core content. + affected_skill_dirs: Dict[Path, tuple] = {} + if stale_skills: + affected_skill_dirs = manager._unregister_skills( + stale_skills, + pack_dir, + restore_from_bundled_core=True, + ) + updated_skills = {} + for agent, names in registered_skills.items(): + retained = [ + name for name in names if name not in stale_skills.get(agent, []) + ] + if retained: + updated_skills[agent] = retained + manager.registry.update(preset_id, {"registered_skills": updated_skills}) + + stale_commands = {} + registered_commands = metadata.get("registered_commands", {}) + if isinstance(registered_commands, dict): for agent, names in registered_commands.items(): if not isinstance(agent, str) or not isinstance(names, list): continue - retained = [ + stale_names = [ name for name in names - if isinstance(name, str) - and name not in stale_commands.get(agent, []) + if isinstance(name, str) and name not in current_command_names ] - if retained: - updated_commands[agent] = retained - manager.registry.update(preset_id, {"registered_commands": updated_commands}) - current_skill_names = { - skill_name - for command_name in current_command_names - for skill_name in manager._skill_names_for_command(command_name) - } - raw_registered_skills = metadata.get("registered_skills", {}) - if isinstance(raw_registered_skills, list): - # Legacy flat-list value: infer real per-agent ownership from - # on-disk provenance rather than dropping it, otherwise a removed - # command's skill can never be identified as stale here. - registered_skills = manager._infer_legacy_skill_provenance( - [name for name in raw_registered_skills if isinstance(name, str)], - preset_id, - fallback_agent, + if stale_names: + stale_commands[agent] = stale_names + if stale_commands: + # Exclude native skill-only agents (extension == "/SKILL.md") + # whose stale command names are already covered by the + # stale-skills pass above: for those agents the "command" + # and "skill" registrations are the same physical SKILL.md + # file, which _unregister_skills already restored/reconciled + # with core-fallback support. _unregister_commands has no + # such fallback — it always deletes — so re-running it here + # would blow away what was just restored (mirrors + # PresetManager.remove()'s identical filtering). + from ..agents import CommandRegistrar as _CommandRegistrarForFilter + + commands_to_unregister: Dict[str, List[str]] = {} + for agent, names in stale_commands.items(): + is_native_skill_agent = ( + _CommandRegistrarForFilter.AGENT_CONFIGS.get(agent, {}).get( + "extension" + ) + == "/SKILL.md" + ) + if not is_native_skill_agent: + commands_to_unregister[agent] = names + continue + covered_skill_names = { + skill_name + for skill_name in stale_skills.get(agent, []) + } + uncovered = [ + name + for name in names + if covered_skill_names.isdisjoint( + manager._skill_names_for_command(name) + ) + ] + if uncovered: + commands_to_unregister[agent] = uncovered + if commands_to_unregister: + manager._unregister_commands(commands_to_unregister) + updated_commands = {} + for agent, names in registered_commands.items(): + if not isinstance(agent, str) or not isinstance(names, list): + continue + retained = [ + name + for name in names + if isinstance(name, str) + and name not in stale_commands.get(agent, []) + ] + if retained: + updated_commands[agent] = retained + manager.registry.update(preset_id, {"registered_commands": updated_commands}) + + if resolved_agent is MISSING_INIT_OPTIONS_FILE: + # Legacy pre-init-options project: there is no single "active + # agent" to target, so mirror install-time behaviour and + # register/re-register this preset's commands and skills for + # every agent directory actually present on disk, merging the + # fresh result into the stored registry state. + fresh_commands = manager._register_commands(manifest, pack_dir) + if fresh_commands: + current_metadata = manager.registry.get(preset_id) or {} + merged = copy.deepcopy(current_metadata.get("registered_commands") or {}) + for agent, names in fresh_commands.items(): + existing = merged.get(agent, []) + merged[agent] = existing + [ + name for name in names if name not in existing + ] + manager.registry.update(preset_id, {"registered_commands": merged}) + + fresh_skills = manager._register_skills(manifest, pack_dir) + if fresh_skills: + current_metadata = manager.registry.get(preset_id) or {} + merged_skills = copy.deepcopy(current_metadata.get("registered_skills") or {}) + for agent, names in fresh_skills.items(): + existing = merged_skills.get(agent, []) + merged_skills[agent] = existing + [ + name for name in names if name not in existing + ] + manager.registry.update(preset_id, {"registered_skills": merged_skills}) + elif isinstance(resolved_agent, str): + manager.register_enabled_presets_for_agent(resolved_agent) + + reconcile_command_names = sorted( + {name for names in stale_commands.values() for name in names} ) - else: - registered_skills = manager._normalize_registered_skills( - raw_registered_skills, fallback_agent=fallback_agent + if stale_commands: + manager._reconcile_composed_commands( + reconcile_command_names, + extra_agents=set(stale_commands), + ) + # A skill can go stale without ever being registered as a command + # (skills-only agents have no `registered_commands` entry), so + # `reconcile_command_names` above (derived only from + # `stale_commands`) can be empty even though `affected_skill_dirs` + # is not — and `_reconcile_skills` no-ops on an empty command-name + # list. Recover a command name for each stale skill via + # `_command_name_for_skill_name` (the inverse of + # `_skill_names_for_command`), so the surviving-lower-priority-preset + # lookup below still runs for skills-only reconciliation. + stale_skill_command_names = { + command_name + for names in stale_skills.values() + for skill_name in names + for command_name in [manager._command_name_for_skill_name(skill_name)] + if command_name is not None + } + skill_reconcile_command_names = sorted( + set(reconcile_command_names) | stale_skill_command_names ) - stale_skills = {} - for agent, names in registered_skills.items(): - if not isinstance(agent, str) or not isinstance(names, list): - continue - stale_names = [ - name - for name in names - if isinstance(name, str) and name not in current_skill_names - ] - if stale_names: - stale_skills[agent] = stale_names - if stale_skills: - manager._unregister_skills( - stale_skills, - pack_dir, - restore_from_bundled_core=True, + if skill_reconcile_command_names or affected_skill_dirs: + # Mirrors PresetManager.remove(): a stale skill directory + # restored to extension/core content above may actually be + # owned by a surviving lower-priority preset, which must be + # reconciled back in rather than left showing core content. + manager._reconcile_skills( + skill_reconcile_command_names, extra_skills_dirs=affected_skill_dirs + ) + + manager.reconcile_constitution( + f"Failed to reconcile constitution after enabling preset {preset_id}" ) - updated_skills = {} - for agent, names in registered_skills.items(): - retained = [ - name for name in names if name not in stale_skills.get(agent, []) - ] - if retained: - updated_skills[agent] = retained - manager.registry.update(preset_id, {"registered_skills": updated_skills}) - - if resolved_agent is MISSING_INIT_OPTIONS_FILE: - # Legacy pre-init-options project: there is no single "active - # agent" to target, so mirror install-time behaviour and - # register/re-register this preset's commands and skills for - # every agent directory actually present on disk, merging the - # fresh result into the stored registry state. - fresh_commands = manager._register_commands(manifest, pack_dir) - if fresh_commands: - current_metadata = manager.registry.get(preset_id) or {} - merged = copy.deepcopy(current_metadata.get("registered_commands") or {}) - for agent, names in fresh_commands.items(): - existing = merged.get(agent, []) - merged[agent] = existing + [ - name for name in names if name not in existing - ] - manager.registry.update(preset_id, {"registered_commands": merged}) - - fresh_skills = manager._register_skills(manifest, pack_dir) - if fresh_skills: - current_metadata = manager.registry.get(preset_id) or {} - merged_skills = copy.deepcopy(current_metadata.get("registered_skills") or {}) - for agent, names in fresh_skills.items(): - existing = merged_skills.get(agent, []) - merged_skills[agent] = existing + [ - name for name in names if name not in existing - ] - manager.registry.update(preset_id, {"registered_skills": merged_skills}) - elif isinstance(resolved_agent, str): - manager.register_enabled_presets_for_agent(resolved_agent) - if stale_commands: - manager._reconcile_composed_commands( - sorted({name for names in stale_commands.values() for name in names}), - extra_agents=set(stale_commands), + except Exception as e: + manager.registry.restore(preset_id, registry_before) + console.print( + f"[red]Error:[/red] Failed to enable '{preset_id}': " + f"{_escape_markup(str(e))}. The preset has been restored to its " + f"previous disabled state; resolve the underlying issue and " + f"re-run 'specify preset enable {preset_id}'." ) - - manager.reconcile_constitution( - f"Failed to reconcile constitution after enabling preset {preset_id}" - ) + raise typer.Exit(1) from e console.print(f"[green]✓[/green] Preset '{preset_id}' enabled") console.print("\nTemplates from this preset will now be included in resolution.") diff --git a/tests/test_presets.py b/tests/test_presets.py index c7e22ef418..4d3736b188 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11432,7 +11432,13 @@ def test_enable_reconciles_legacy_flat_list_registered_skills( manager.registry.update("legacy-skill-preset", {"enabled": False}) # Simulate a disabled update that dropped "speckit.drop" from the - # installed manifest. + # installed manifest. A real `update_from_directory` replaces the + # installed preset directory wholesale, so the dropped command's + # file is gone from disk too — remove it here as well, or the + # resolver's convention-based fallback (an on-disk file with no + # matching manifest entry still resolves as a layer) would treat + # the leftover file as still "provided" by this preset and + # reconciliation would legitimately recreate it. installed_manifest_path = ( project_dir / ".specify" @@ -11447,6 +11453,14 @@ def test_enable_reconciles_legacy_flat_list_registered_skills( if t["name"] != "speckit.drop" ] installed_manifest_path.write_text(yaml.safe_dump(installed_manifest)) + ( + project_dir + / ".specify" + / "presets" + / "legacy-skill-preset" + / "commands" + / "speckit.drop.md" + ).unlink() with patch.object(Path, "cwd", return_value=project_dir): result = CliRunner().invoke( @@ -11549,6 +11563,200 @@ def build_preset(preset_id, body, version="1.0.0"): claude_skills / "speckit-specify" / "SKILL.md" ).read_text(encoding="utf-8") + def test_enable_restores_disabled_state_on_reconciliation_failure( + self, project_dir, temp_dir + ): + """If reconciliation raises partway through enable, the registry + must be restored to its pre-enable (disabled) snapshot rather than + being left showing ``enabled: True`` with only partially refreshed + artifacts (#4441). + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + # An agent directory must exist for registration to actually + # record any command names — otherwise registered_commands stays + # empty and there's nothing for enable to consider stale. + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + def build_preset(preset_id, command_names, version="1.0.0"): + preset_dir = temp_dir / f"{preset_id}-{version}" + (preset_dir / "commands").mkdir(parents=True) + templates = [] + for name in command_names: + (preset_dir / "commands" / f"{name}.md").write_text( + f"---\ndescription: test\n---\n\n{name} body\n" + ) + templates.append({ + "type": "command", + "name": name, + "file": f"commands/{name}.md", + }) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": version, + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": templates}, + } + (preset_dir / "preset.yml").write_text(yaml.safe_dump(manifest_data)) + return preset_dir + + manager = PresetManager(project_dir) + source_v1 = build_preset( + "rollback-preset", ["speckit.specify", "speckit.plan"] + ) + manager.install_from_directory(source_v1, "0.1.0") + manager.registry.update("rollback-preset", {"enabled": False}) + + # Update while disabled, dropping speckit.plan — this makes it a + # stale command that enable must reconcile (and is exactly the code + # path we're about to make fail). + source_v2 = build_preset( + "rollback-preset", ["speckit.specify"], version="2.0.0" + ) + manager.update_from_directory( + source_v2, "0.1.0", pack_id="rollback-preset" + ) + + registry_before = manager.registry.get("rollback-preset") + assert registry_before["enabled"] is False + + with patch.object( + PresetManager, + "_unregister_commands", + side_effect=RuntimeError("simulated reconciliation failure"), + ), patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "enable", "rollback-preset"] + ) + + assert result.exit_code != 0 + assert "restored" in result.output.lower() or "previous disabled state" in result.output.lower() + + # Reload registry fresh from disk: it must still show disabled, + # not a half-completed "enabled" state. + manager2 = PresetManager(project_dir) + metadata_after = manager2.registry.get("rollback-preset") + assert metadata_after["enabled"] is False + assert metadata_after["version"] == "2.0.0" + + def test_enable_reconciles_surviving_lower_priority_preset_skill_for_historical_agent( + self, project_dir, temp_dir + ): + """A disabled update that drops a command the preset used to + override must, on enable, hand that command's skill back to a + surviving lower-priority preset for *every* agent directory the + preset's ``registered_skills`` actually spans, including a + historical (currently inactive) agent, not just the one active + agent that ``register_enabled_presets_for_agent`` refreshes. + + Mirrors ``test_remove_reconciles_skill_for_every_historical_agent`` + for the enable-after-disabled-update path (#4441): without + capturing ``_unregister_skills``'s return value and handing it to + ``_reconcile_skills``, the historical (claude) directory is left + showing core/extension content instead of the surviving + lo-priority preset's override. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + self_ = TestPresetSkills() + self_._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills = project_dir / ".claude" / "skills" + + # A core template fallback is required so unregistering the stale + # override restores core content rather than deleting the skill + # directory outright, matching the historical-agent removal test. + core_cmds = project_dir / ".specify" / "templates" / "commands" + core_cmds.mkdir(parents=True, exist_ok=True) + (core_cmds / "specify.md").write_text( + "---\ndescription: Core specify command\n---\n\nCore specify body\n", + encoding="utf-8", + ) + + manager = PresetManager(project_dir) + + lo_dir = self_._create_command_preset( + temp_dir, "lo-priority-preset", "speckit.specify", "lo", "Lo content" + ) + manager.install_from_directory(lo_dir, "0.1.5", priority=10) + + hi_dir = self_._create_command_preset( + temp_dir, "hi-priority-preset", "speckit.specify", "hi", "Hi content" + ) + manager.install_from_directory(hi_dir, "0.1.5", priority=1) + assert "Hi content" in ( + claude_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + + # Switch the active integration to codex and rescaffold, mirroring + # `integration use codex` — this records the hi-priority preset's + # registered_skills for codex too, leaving claude as a historical + # (now inactive) agent it still owns a directory in. + self_._write_init_options(project_dir, ai="codex", ai_skills=True) + codex_skills = project_dir / ".agents" / "skills" + manager.register_enabled_presets_for_agent("codex") + + metadata_hi = manager.registry.get("hi-priority-preset") + assert set(metadata_hi.get("registered_skills", {})) == {"claude", "codex"}, ( + "sanity: hi-priority-preset's registered_skills must span " + "both the historical (claude) and active (codex) agents" + ) + + manager.registry.update("hi-priority-preset", {"enabled": False}) + + # Update the disabled hi-priority preset so it no longer provides + # speckit.specify at all — its skill override becomes stale in + # both the historical and active agent directories. + hi_dir_v2 = self_._create_command_preset( + temp_dir, "hi-priority-preset-v2", "speckit.other", "hi2", "Hi other content" + ) + data = yaml.safe_load((hi_dir_v2 / "preset.yml").read_text()) + data["preset"]["id"] = "hi-priority-preset" + data["preset"]["version"] = "2.0.0" + (hi_dir_v2 / "preset.yml").write_text(yaml.safe_dump(data)) + manager.update_from_directory( + hi_dir_v2, "0.1.5", pack_id="hi-priority-preset" + ) + + # Still disabled: the stale override remains untouched on disk in + # both directories. + assert "Hi content" in ( + claude_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + assert "Hi content" in ( + codex_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "enable", "hi-priority-preset"] + ) + + assert result.exit_code == 0, result.output + # The surviving lo-priority preset's override must win back both + # directories — codex (currently active, refreshed by + # register_enabled_presets_for_agent) and claude (historical, + # only fixed by the _reconcile_skills(extra_skills_dirs=...) call). + assert "Lo content" in ( + claude_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8"), ( + "the historical (claude) directory must be reconciled back " + "to the surviving lo-priority preset's override, not left " + "showing core/extension content" + ) + assert "Lo content" in ( + codex_skills / "speckit-specify" / "SKILL.md" + ).read_text(encoding="utf-8") + def test_disable_corrupted_registry_entry(self, project_dir, pack_dir): """Test disable fails gracefully for corrupted registry entry.""" from typer.testing import CliRunner From 790a89e2058add9fc7853d5f5cd0bd80e4e0c7d9 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:17:44 +0100 Subject: [PATCH 10/12] fix: use builtin generics instead of undefined Dict/List annotations _commands.py doesn't import typing.Dict/List; with `from __future__ import annotations` these are lazily-evaluated string annotations so ruff's F821 caught the undefined names rather than a runtime failure. Switch to builtin dict/list generics (PEP 585) instead of adding an unused-elsewhere typing import. Assisted-by: GitHub Copilot (model: Auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/_commands.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 28fafaa527..8594961230 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1297,7 +1297,7 @@ def preset_enable( # restored to extension/core content, so a surviving lower-priority # preset that should actually win those commands can be reconciled # back in afterwards instead of being left showing core content. - affected_skill_dirs: Dict[Path, tuple] = {} + affected_skill_dirs: dict[Path, tuple] = {} if stale_skills: affected_skill_dirs = manager._unregister_skills( stale_skills, @@ -1338,7 +1338,7 @@ def preset_enable( # PresetManager.remove()'s identical filtering). from ..agents import CommandRegistrar as _CommandRegistrarForFilter - commands_to_unregister: Dict[str, List[str]] = {} + commands_to_unregister: dict[str, list[str]] = {} for agent, names in stale_commands.items(): is_native_skill_agent = ( _CommandRegistrarForFilter.AGENT_CONFIGS.get(agent, {}).get( From 8d2210cbaf1b1f8d30a96c8c8e9ddc47ca394e83 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:39:11 +0100 Subject: [PATCH 11/12] fix: use forward-matching to recover stale namespaced-command names in preset enable Copilot's review flagged that recovering a command name from a skill directory name by naive reversal (`_command_name_for_skill_name`) is mathematically lossy for namespaced commands: "speckit.git.feature" and "speckit.git-feature" both encode to "speckit-git-feature", so reversing the encoding can't tell them apart. Replace the reversal with forward-matching: build the set of command names this preset is known to use (current manifest + everything ever recorded in its own registered_commands), and for each stale skill, find which known command name's _skill_names_for_command(...) output actually produces it. This is unambiguous because it goes in the transform's defined (forward) direction, and is scoped to only this preset's own command universe. Removed the now-dead, buggy _command_name_for_skill_name helper. Added a regression test (namespaced speckit.git.feature command, with the affected agent in a historical/inactive-agent scenario) covering this exact case end-to-end. A related reordering change to update_from_directory (unregistering stale skills before stale commands) was evaluated but reverted: traced step-by-step, update_from_directory's own _reconcile_skills call unconditionally rebuilds every affected command name's skill content after both unregister calls run, regardless of their order, so no reachable regression exists there -- reordering would have added complexity without fixing anything. Assisted-by: GitHub Copilot (model: auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/__init__.py | 16 --- src/specify_cli/presets/_commands.py | 35 ++++-- tests/test_presets.py | 181 +++++++++++++++++++++++++++ 3 files changed, 205 insertions(+), 27 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 4107d662bf..279dbd49d7 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2939,22 +2939,6 @@ def _skill_names_for_command(cmd_name: str) -> tuple[str, str]: legacy_skill_name = f"speckit.{raw_short_name}" return modern_skill_name, legacy_skill_name - @staticmethod - def _command_name_for_skill_name(skill_name: str) -> Optional[str]: - """Return the command name a skill directory name derives from. - - Inverse of `_skill_names_for_command`: "speckit-specify" and - "speckit.specify" both map back to "speckit.specify". Returns None - for names that don't follow the skill-naming convention. - """ - if skill_name.startswith("speckit-"): - short_name = skill_name[len("speckit-"):] - elif skill_name.startswith("speckit."): - short_name = skill_name[len("speckit."):] - else: - return None - return f"speckit.{short_name}" - @staticmethod def _skill_title_from_command(cmd_name: str) -> str: """Return a human-friendly title for a skill command name.""" diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 8594961230..c7e2268b0a 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1421,17 +1421,30 @@ def preset_enable( # `reconcile_command_names` above (derived only from # `stale_commands`) can be empty even though `affected_skill_dirs` # is not — and `_reconcile_skills` no-ops on an empty command-name - # list. Recover a command name for each stale skill via - # `_command_name_for_skill_name` (the inverse of - # `_skill_names_for_command`), so the surviving-lower-priority-preset - # lookup below still runs for skills-only reconciliation. - stale_skill_command_names = { - command_name - for names in stale_skills.values() - for skill_name in names - for command_name in [manager._command_name_for_skill_name(skill_name)] - if command_name is not None - } + # list. Recover the command name for each stale skill by matching + # forward via `_skill_names_for_command` against every command name + # this preset is known to have used (its current manifest plus + # everything ever recorded in `registered_commands`), rather than + # reversing the skill-name encoding: that reverse mapping is lossy + # for namespaced commands (e.g. "speckit.git.feature" -> + # "speckit-git-feature" collides with "speckit.git-feature" on the + # way back), so the surviving-lower-priority-preset lookup below + # could target the wrong command entirely. + all_known_command_names = set(current_command_names) + if isinstance(registered_commands, dict): + for names in registered_commands.values(): + if isinstance(names, list): + all_known_command_names.update( + name for name in names if isinstance(name, str) + ) + stale_skill_command_names = set() + for names in stale_skills.values(): + for skill_name in names: + for command_name in all_known_command_names: + modern, legacy = manager._skill_names_for_command(command_name) + if skill_name in (modern, legacy): + stale_skill_command_names.add(command_name) + break skill_reconcile_command_names = sorted( set(reconcile_command_names) | stale_skill_command_names ) diff --git a/tests/test_presets.py b/tests/test_presets.py index 4d3736b188..94aa1d818f 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11757,6 +11757,109 @@ def test_enable_reconciles_surviving_lower_priority_preset_skill_for_historical_ codex_skills / "speckit-specify" / "SKILL.md" ).read_text(encoding="utf-8") + def test_enable_reconciles_stale_skill_for_namespaced_command( + self, project_dir, temp_dir + ): + """A stale skill left over from a disabled update for a namespaced + command (e.g. ``speckit.git.feature``) must be reconciled back to + the surviving lower-priority preset's content on enable (#4441). + + ``speckit.git.feature`` (a 3-segment, extension-style namespaced + command per ``presets/scaffold/preset.yml``) encodes to the skill + directory ``speckit-git-feature``. A review flagged that recovering + the command name from that directory name by naive reversal (every + hyphen after the first segment treated as literal) collides with + ``speckit.git-feature`` and would target the wrong command; the + forward-matching lookup in ``preset_enable`` avoids that ambiguity + by construction rather than relying on an invertible encoding. This + test locks in correct end-to-end behaviour for the namespaced case, + including when the agent whose skill goes stale (copilot, in + skills-opt-in mode) is a historical/inactive one that + ``register_enabled_presets_for_agent`` doesn't otherwise refresh. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + self_ = TestPresetSkills() + self_._write_init_options(project_dir, ai="copilot", ai_skills=True) + copilot_skills = project_dir / ".github" / "skills" + + manager = PresetManager(project_dir) + + lo_dir = self_._create_command_preset( + temp_dir, + "lo-priority-preset", + "speckit.git.feature", + "Lo git feature", + "Lo content", + ) + manager.install_from_directory(lo_dir, "0.1.5", priority=10) + + hi_dir = self_._create_command_preset( + temp_dir, + "hi-priority-preset", + "speckit.git.feature", + "Hi git feature", + "Hi content", + ) + manager.install_from_directory(hi_dir, "0.1.5", priority=1) + assert "Hi content" in ( + copilot_skills / "speckit-git-feature" / "SKILL.md" + ).read_text(encoding="utf-8") + + hi_metadata = manager.registry.get("hi-priority-preset") + assert not hi_metadata.get("registered_commands", {}).get("copilot"), ( + "sanity: copilot in skills-opt-in mode must not record this " + "override in registered_commands, otherwise the lossy-inverse " + "bug this test targets can't be reached" + ) + + # Switch the active agent to claude — copilot becomes historical + # (inactive) and register_enabled_presets_for_agent will no longer + # touch it, isolating its recovery to the stale-skill fallback. + self_._write_init_options(project_dir, ai="claude", ai_skills=True) + manager.register_enabled_presets_for_agent("claude") + + manager.registry.update("hi-priority-preset", {"enabled": False}) + + # Update the disabled hi-priority preset so it no longer provides + # speckit.git.feature at all -- its skill override becomes stale. + hi_dir_v2 = self_._create_command_preset( + temp_dir, + "hi-priority-preset-v2", + "speckit.other", + "Unrelated", + "Hi other content", + ) + data = yaml.safe_load((hi_dir_v2 / "preset.yml").read_text()) + data["preset"]["id"] = "hi-priority-preset" + data["preset"]["version"] = "2.0.0" + (hi_dir_v2 / "preset.yml").write_text(yaml.safe_dump(data)) + manager.update_from_directory( + hi_dir_v2, "0.1.5", pack_id="hi-priority-preset" + ) + + # Still disabled: the stale override remains untouched on disk. + assert "Hi content" in ( + copilot_skills / "speckit-git-feature" / "SKILL.md" + ).read_text(encoding="utf-8") + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "enable", "hi-priority-preset"] + ) + + assert result.exit_code == 0, result.output + assert "Lo content" in ( + copilot_skills / "speckit-git-feature" / "SKILL.md" + ).read_text(encoding="utf-8"), ( + "the namespaced command's stale skill must be reconciled back " + "to the surviving lo-priority preset's override under its " + "correct command name, not left on stale/core content because " + "of a lossy skill-name-to-command-name reversal" + ) + def test_disable_corrupted_registry_entry(self, project_dir, pack_dir): """Test disable fails gracefully for corrupted registry entry.""" from typer.testing import CliRunner @@ -16900,6 +17003,84 @@ def test_cli_bulk_unresolvable_source_reports_clearly( assert "not re-resolvable" in result.output assert "--from/--dev" in result.output + def test_update_removing_command_reconciles_surviving_preset_for_native_skill_agent( + self, project_dir, temp_dir + ): + """A command dropped by an update must reconcile back to a + surviving lower-priority preset's content for native skill-only + agents (e.g. claude), never leaving the SKILL.md directory deleted. + + Guards the exact scenario a review flagged as a possible ordering + bug (stale commands unregistered before stale skills, for agents + where "command" and "skill" track the same physical file). It + already passes on unmodified code because `_reconcile_skills` runs + unconditionally afterwards for every affected command name, + regenerating content regardless of unregister ordering -- this + test locks that guarantee in place. + """ + from specify_cli import save_init_options + + save_init_options( + project_dir, {"ai": "claude", "ai_skills": True, "script": "sh"} + ) + lo_source = self._multi_command_preset( + temp_dir, + "lo-preset", + {"speckit.plan": "lo-preset plan body"}, + version="1.0.0", + ) + hi_source = self._multi_command_preset( + temp_dir, + "hi-preset", + { + "speckit.specify": "hi-preset specify body", + "speckit.plan": "hi-preset plan body", + }, + version="1.0.0", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(lo_source, "0.1.0", priority=20) + manager.install_from_directory(hi_source, "0.1.0", priority=10) + + plan_skill = ( + project_dir / ".claude" / "skills" / "speckit-plan" / "SKILL.md" + ) + specify_skill = ( + project_dir / ".claude" / "skills" / "speckit-specify" / "SKILL.md" + ) + assert "hi-preset plan body" in plan_skill.read_text() + assert "hi-preset specify body" in specify_skill.read_text() + + # Update hi-preset to a version that drops "speckit.plan" entirely. + updated_hi_source = self._multi_command_preset( + temp_dir / "updated", + "hi-preset", + {"speckit.specify": "hi-preset specify body v2"}, + version="2.0.0", + ) + manager.update_from_directory( + updated_hi_source, "0.1.0", pack_id="hi-preset" + ) + + assert specify_skill.is_file() + assert "hi-preset specify body v2" in specify_skill.read_text() + + # The dropped command's skill must still exist and now show the + # surviving lower-priority preset's content, not be deleted. + assert plan_skill.is_file(), ( + "speckit-plan SKILL.md was deleted instead of being reconciled " + "to the surviving lower-priority preset" + ) + assert "lo-preset plan body" in plan_skill.read_text() + + hi_metadata = PresetManager(project_dir).registry.get("hi-preset") + assert "speckit.plan" not in hi_metadata["registered_commands"].get( + "claude", [] + ) + assert "speckit-plan" not in hi_metadata["registered_skills"].get( + "claude", [] + ) + class TestPresetUpdateConcurrency: def _updated_source(self, pack_dir, version="2.0.0"): From 20f6c0360b16f85c01b1d76321d46596ecea30f1 Mon Sep 17 00:00:00 2001 From: "Damien Hardy (digimangos)" <49808393+digimangos@users.noreply.github.com> Date: Sat, 5 Sep 2026 16:09:21 +0100 Subject: [PATCH 12/12] fix: broaden stale-skill command-name recovery across the preset stack For a command-backed integration running in explicit skills-opt-in mode (e.g. Copilot with ai_skills on), _register_commands's guard never populates registered_commands for it, and after a disabled update the stale command is also absent from the preset's own current manifest. The prior forward-match fix (8d2210cb) only searched this preset's own current manifest plus its own registered_commands, so it could not recover the command name in this case: _reconcile_skills received an empty command list and no-op'd, and the skill directory was deleted outright (no core/extension fallback to restore). Broaden the candidate search in preset_enable to also scan every other installed preset's registered_commands and re-parsed preset.yml (including aliases), since the surviving lower-priority preset that should win the skill back always still lists the command in its own current manifest. Added a regression test that discriminates the fix using two command-backed, skills-opt-in agents (one historical, one active) so neither agent's registered_commands can mask the bug via the active-agent full-rescaffold path. Confirmed the test fails (skill file deleted) against the narrower fix and passes with this change. Assisted-by: GitHub Copilot (model: auto, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab5495b7-024f-486a-8abd-7cbfe54056d6 --- src/specify_cli/presets/_commands.py | 61 ++++++++++++++++++++++++---- tests/test_presets.py | 60 ++++++++++++++++++--------- 2 files changed, 94 insertions(+), 27 deletions(-) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index c7e2268b0a..871eaa21eb 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1422,14 +1422,26 @@ def preset_enable( # `stale_commands`) can be empty even though `affected_skill_dirs` # is not — and `_reconcile_skills` no-ops on an empty command-name # list. Recover the command name for each stale skill by matching - # forward via `_skill_names_for_command` against every command name - # this preset is known to have used (its current manifest plus - # everything ever recorded in `registered_commands`), rather than - # reversing the skill-name encoding: that reverse mapping is lossy - # for namespaced commands (e.g. "speckit.git.feature" -> - # "speckit-git-feature" collides with "speckit.git-feature" on the - # way back), so the surviving-lower-priority-preset lookup below - # could target the wrong command entirely. + # forward via `_skill_names_for_command`, rather than reversing the + # skill-name encoding: that reverse mapping is lossy for namespaced + # commands (e.g. "speckit.git.feature" -> "speckit-git-feature" + # collides with "speckit.git-feature" on the way back), so the + # surviving-lower-priority-preset lookup below could target the + # wrong command entirely. + # + # The candidate set can't be limited to *this* preset's own current + # manifest plus its own `registered_commands`: a command-backed + # integration running in explicit skills-opt-in mode (e.g. Copilot + # with `ai_skills` on) never gets an entry in `registered_commands` + # at all (see `_register_commands`'s ai_skills guard), and the + # command itself is, by definition, no longer present in this + # preset's *current* (post-update) manifest — it's the very thing + # that went stale. In that case this preset alone can never supply + # the name. The preset that's actually meant to win the skill back + # (a surviving lower-priority provider) still lists the command in + # its own current manifest, so the candidate set is gathered from + # every installed preset's manifest and registered_commands, not + # just this one's. all_known_command_names = set(current_command_names) if isinstance(registered_commands, dict): for names in registered_commands.values(): @@ -1437,6 +1449,39 @@ def preset_enable( all_known_command_names.update( name for name in names if isinstance(name, str) ) + if stale_skills: + for other_pack_id, other_metadata in manager.registry.list().items(): + if other_pack_id == preset_id or not isinstance(other_metadata, dict): + continue + other_registered_commands = other_metadata.get( + "registered_commands", {} + ) + if isinstance(other_registered_commands, dict): + for names in other_registered_commands.values(): + if isinstance(names, list): + all_known_command_names.update( + name for name in names if isinstance(name, str) + ) + other_pack_dir = manager.presets_dir / other_pack_id + other_manifest_path = other_pack_dir / "preset.yml" + if other_manifest_path.exists(): + try: + other_manifest = PresetManifest(other_manifest_path) + except PresetValidationError: + continue + for template in other_manifest.templates: + if template.get("type") != "command": + continue + for name in ( + [template.get("name")] + + [ + alias + for alias in template.get("aliases", []) + if isinstance(alias, str) + ] + ): + if isinstance(name, str): + all_known_command_names.add(name) stale_skill_command_names = set() for names in stale_skills.values(): for skill_name in names: diff --git a/tests/test_presets.py b/tests/test_presets.py index 94aa1d818f..d8f2243288 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11762,20 +11762,36 @@ def test_enable_reconciles_stale_skill_for_namespaced_command( ): """A stale skill left over from a disabled update for a namespaced command (e.g. ``speckit.git.feature``) must be reconciled back to - the surviving lower-priority preset's content on enable (#4441). + the surviving lower-priority preset's content on enable (#4441), + even when the preset whose command went stale never had a + ``registered_commands`` entry for it at all. ``speckit.git.feature`` (a 3-segment, extension-style namespaced command per ``presets/scaffold/preset.yml``) encodes to the skill - directory ``speckit-git-feature``. A review flagged that recovering - the command name from that directory name by naive reversal (every - hyphen after the first segment treated as literal) collides with - ``speckit.git-feature`` and would target the wrong command; the - forward-matching lookup in ``preset_enable`` avoids that ambiguity - by construction rather than relying on an invertible encoding. This - test locks in correct end-to-end behaviour for the namespaced case, - including when the agent whose skill goes stale (copilot, in - skills-opt-in mode) is a historical/inactive one that - ``register_enabled_presets_for_agent`` doesn't otherwise refresh. + directory ``speckit-git-feature``. Recovering the command name from + that directory name by naive reversal (every hyphen after the first + segment treated as literal) collides with ``speckit.git-feature`` + and would target the wrong command; forward-matching avoids that + ambiguity by construction. + + Copilot starts active (skills-opt-in mode), so + ``_register_commands``'s guard never populates + ``registered_commands`` for it there. The active agent is then + switched to ``amp`` -- another command-backed, skills-opt-in + integration -- making copilot *historical*. This matters: + ``register_enabled_presets_for_agent`` only rescaffolds the + newly-active agent's own directory, so if copilot stayed active it + would be fully re-derived (correctly) by that call regardless of + whether the stale-skill candidate search below works at all, + masking the bug. With copilot historical and amp active instead, + neither agent's ``registered_commands`` is ever populated (amp is + also skills-opt-in), and the disabled update also drops the command + from hi-priority-preset's own current manifest. So neither + hi-priority-preset's manifest nor its own ``registered_commands`` + can supply the name for copilot's stale skill -- the only source is + the surviving lo-priority preset's own current manifest, which the + candidate search must reach across the whole installed preset stack + (not just the preset whose skill went stale) to find. """ from typer.testing import CliRunner from unittest.mock import patch @@ -11811,20 +11827,26 @@ def test_enable_reconciles_stale_skill_for_namespaced_command( hi_metadata = manager.registry.get("hi-priority-preset") assert not hi_metadata.get("registered_commands", {}).get("copilot"), ( "sanity: copilot in skills-opt-in mode must not record this " - "override in registered_commands, otherwise the lossy-inverse " - "bug this test targets can't be reached" + "override in registered_commands, otherwise the candidate " + "search this test targets can't be reached" ) - # Switch the active agent to claude — copilot becomes historical - # (inactive) and register_enabled_presets_for_agent will no longer - # touch it, isolating its recovery to the stale-skill fallback. - self_._write_init_options(project_dir, ai="claude", ai_skills=True) - manager.register_enabled_presets_for_agent("claude") + # Switch the active agent to amp (also command-backed, + # skills-opt-in) so copilot becomes historical -- see the docstring + # for why this is required to actually reach the bug. + self_._write_init_options(project_dir, ai="amp", ai_skills=True) + manager.register_enabled_presets_for_agent("amp") + hi_metadata = manager.registry.get("hi-priority-preset") + assert not hi_metadata.get("registered_commands", {}).get("copilot"), ( + "sanity: switching active agent must not retroactively " + "populate registered_commands for the now-historical copilot" + ) manager.registry.update("hi-priority-preset", {"enabled": False}) # Update the disabled hi-priority preset so it no longer provides - # speckit.git.feature at all -- its skill override becomes stale. + # speckit.git.feature at all -- its skill override becomes stale, + # and the command is now also absent from its own current manifest. hi_dir_v2 = self_._create_command_preset( temp_dir, "hi-priority-preset-v2",