From 5a13898ad6f312c02771e51e1d105203d3af1140 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Tue, 29 Sep 2026 00:58:05 +0800 Subject: [PATCH 1/6] feat(presets): support regex resource selectors Add regex: selectors for preset templates, scripts, and commands while preserving exact-name behavior. Validate regex patterns and use full-name matching. Expand command selectors to concrete lower-layer commands before registration and reconcile affected commands across preset lifecycle changes. Add selector, resolver, command lifecycle, and diagnostic coverage. Verified with the full test suite (8415 passed, 211 skipped), Ruff, ShellCheck, PowerShell syntax parsing, Python compilation, and git diff --check. Closes #4659 --- src/specify_cli/presets/_manager.py | 299 +++++++----- src/specify_cli/presets/_manager_commands.py | 331 +++++++++---- src/specify_cli/presets/_manifest.py | 63 +-- src/specify_cli/presets/_resolver.py | 251 ++++++++-- src/specify_cli/presets/_selectors.py | 30 ++ src/specify_cli/presets/command_enable.py | 31 ++ src/specify_cli/presets/command_info.py | 114 ++++- .../presets/command_set_priority.py | 25 +- .../presets/test_regex_selectors.py | 437 ++++++++++++++++++ 9 files changed, 1288 insertions(+), 293 deletions(-) create mode 100644 src/specify_cli/presets/_selectors.py create mode 100644 tests/specify_cli/presets/test_regex_selectors.py diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 33274dfdfc..be643a8a3a 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -30,6 +30,7 @@ ) from ._registry import PresetRegistry from ._resolver import PresetResolver +from ._selectors import is_regex_selector _CONSTITUTION_PROVENANCE_FILE = ".constitution-template.json" _CONSTITUTION_SYNC_PRESET_ID = "constitution-sync" @@ -69,16 +70,13 @@ def _constitution_is_generated( metadata = json.loads(provenance.read_text(encoding="utf-8")) except (json.JSONDecodeError, UnicodeDecodeError): return False - return ( - isinstance(metadata, dict) - and metadata.get("sha256") == _content_sha256(content) + return isinstance(metadata, dict) and metadata.get("sha256") == _content_sha256( + content ) # Older projects have no provenance sidecar. Only the immutable bundled or # source-checkout core template is safe to treat as generated. - core = resolver._find_bundled_core( - "constitution-template", "template", ".md" - ) + core = resolver._find_bundled_core("constitution-template", "template", ".md") return core is not None and core.read_bytes() == content @@ -164,9 +162,7 @@ def __init__(self, project_root: Path): self.registry = PresetRegistry(self.presets_dir) def check_compatibility( - self, - manifest: PresetManifest, - speckit_version: str + self, manifest: PresetManifest, speckit_version: str ) -> bool: """Check if preset is compatible with current spec-kit version. @@ -208,8 +204,7 @@ def check_compatibility( return True def find_unmet_extension_dependencies( - self, - manifest: PresetManifest + self, manifest: PresetManifest ) -> List[Dict[str, Any]]: """Find declared extension dependencies that are not satisfied. @@ -321,9 +316,7 @@ def find_unmet_extension_dependencies( # the preset is as inert as if it were never installed -- but the # surviving entry would otherwise read as satisfied. if not (extensions_dir / dep["id"]).is_dir(): - unmet.append( - {**dep, "installed": installed_version, "reason": "stale"} - ) + unmet.append({**dep, "installed": installed_version, "reason": "stale"}) continue # A disabled extension is registered but contributes nothing: @@ -347,7 +340,9 @@ def find_unmet_extension_dependencies( # apart -- it catches InvalidVersion and returns False, which would # report a mismatch against a version nobody can evaluate. Check # parseability up front so only real comparisons reach the warning. - if installed_version is None or not _is_comparable_version(installed_version): + if installed_version is None or not _is_comparable_version( + installed_version + ): continue if not version_satisfies(installed_version, constraint): unmet.append( @@ -382,7 +377,9 @@ def install_from_directory( """ # Validate priority if priority < 1: - raise PresetValidationError("Priority must be a positive integer (1 or higher)") + raise PresetValidationError( + "Priority must be a positive integer (1 or higher)" + ) manifest_path = source_dir / "preset.yml" manifest = PresetManifest(manifest_path) @@ -413,15 +410,18 @@ def install_from_directory( if normalized_catalog_name else "local" ) - self.registry.add(manifest.id, { - "version": manifest.version, - "source": source, - "manifest_hash": manifest.get_hash(), - "enabled": True, - "priority": priority, - "registered_commands": {}, - "registered_skills": {}, - }) + self.registry.add( + manifest.id, + { + "version": manifest.version, + "source": source, + "manifest_hash": manifest.get_hash(), + "enabled": True, + "priority": priority, + "registered_commands": {}, + "registered_skills": {}, + }, + ) registered_commands: Dict[str, List[str]] = {} registered_skills: Dict[str, List[str]] = {} @@ -430,16 +430,22 @@ def install_from_directory( # immediately so cleanup can recover even if installation stops # before later phases complete. registered_commands = self._register_commands(manifest, dest_dir) - self.registry.update(manifest.id, { - "registered_commands": registered_commands, - }) + self.registry.update( + manifest.id, + { + "registered_commands": registered_commands, + }, + ) # Update corresponding skills when skills mode was previously used # and persist that result as well. registered_skills = self._register_skills(manifest, dest_dir) - self.registry.update(manifest.id, { - "registered_skills": registered_skills, - }) + self.registry.update( + manifest.id, + { + "registered_skills": registered_skills, + }, + ) except Exception: # Roll back all side effects. _register_skills persists each # successful write immediately, so reload that partial map when @@ -462,19 +468,30 @@ def install_from_directory( self.registry.remove(manifest.id) raise - # Reconcile all affected commands from the full priority stack so that - # install order doesn't determine the winning command file. - cmd_names = [ - t["name"] - for t in manifest.templates - if t.get("type") == "command" + command_templates = [ + template + for template in manifest.templates + if template.get("type") == "command" ] + expanded_templates = self._expand_command_selectors( + PresetResolver(self.project_root), dest_dir, command_templates + ) + cmd_names = sorted( + { + item["name"] + for item in expanded_templates + if isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + } + ) + if cmd_names: try: self._reconcile_composed_commands(cmd_names) self._reconcile_skills(cmd_names) except Exception as exc: import warnings + warnings.warn( f"Post-install reconciliation failed for {manifest.id}: {exc}. " f"Agent command files may not reflect the current priority stack.", @@ -501,14 +518,18 @@ def _seed_constitution_from_preset( stack. Later preset installs only reconcile when they provide a ``constitution-template``. Authored constitutions are never overwritten. """ - provides_constitution = manifest.id == _CONSTITUTION_SYNC_PRESET_ID or any( - t.get("type") == "template" and t.get("name") == "constitution-template" - for t in manifest.templates - ) or any( - (preset_dir / relative_path).is_file() - for relative_path in ( - "templates/constitution-template.md", - "constitution-template.md", + provides_constitution = ( + manifest.id == _CONSTITUTION_SYNC_PRESET_ID + or any( + t.get("type") == "template" and t.get("name") == "constitution-template" + for t in manifest.templates + ) + or any( + (preset_dir / relative_path).is_file() + for relative_path in ( + "templates/constitution-template.md", + "constitution-template.md", + ) ) ) if not provides_constitution: @@ -577,7 +598,9 @@ def install_from_archive( """ # Validate priority early if priority < 1: - raise PresetValidationError("Priority must be a positive integer (1 or higher)") + raise PresetValidationError( + "Priority must be a positive integer (1 or higher)" + ) with tempfile.TemporaryDirectory() as tmpdir: temp_path = Path(tmpdir) @@ -598,9 +621,7 @@ def install_from_archive( manifest_path = pack_dir / "preset.yml" if not manifest_path.exists(): - raise PresetValidationError( - "No preset.yml found in archive" - ) + raise PresetValidationError("No preset.yml found in archive") return self.install_from_directory( pack_dir, @@ -659,7 +680,9 @@ def remove(self, pack_id: str) -> bool: from .. import load_init_options init_opts = load_init_options(self.project_root) - fallback_agent = init_opts.get("ai") if isinstance(init_opts, dict) else None + fallback_agent = ( + init_opts.get("ai") if isinstance(init_opts, dict) else None + ) if not isinstance(fallback_agent, str): fallback_agent = "" registered_skills = self._infer_legacy_skill_provenance( @@ -667,7 +690,9 @@ def remove(self, pack_id: str) -> bool: pack_id, fallback_agent=fallback_agent, ) - registered_commands = metadata.get("registered_commands", {}) if metadata else {} + registered_commands = ( + metadata.get("registered_commands", {}) if metadata else {} + ) pack_dir = self.presets_dir / pack_id # Record which historical agents this preset's registered_commands @@ -688,19 +713,13 @@ def remove(self, pack_id: str) -> bool: agent_name for agent_name in registered_commands if _CommandRegistrarForScope is None - or _CommandRegistrarForScope.AGENT_CONFIGS.get(agent_name, {}).get("extension") != "/SKILL.md" + or _CommandRegistrarForScope.AGENT_CONFIGS.get(agent_name, {}).get( + "extension" + ) + != "/SKILL.md" } - # Collect ALL command names before filtering for reconciliation, - # so commands registered only for skill-based agents are also - # reconciled. Every command-type template's primary name is added - # unconditionally (not just aliases) since ai_skills-mode presets - # never populate registered_commands for command-backed - # integrations (see _register_commands's ai_skills guard) — without - # this, removing a skills-mode preset that overrides a command no - # other preset registered "the normal way" would skip reconciliation - # entirely and _unregister_skills would restore core/extension - # content instead of a surviving lower-priority preset's override. + # Collect ALL command names before filtering for reconciliation. removed_cmd_names = set() removed_constitution = any( path.exists() @@ -721,33 +740,46 @@ def remove(self, pack_id: str) -> bool: metadata["version"], ) ) - for cmd_names in registered_commands.values(): - removed_cmd_names.update(cmd_names) manifest_path = pack_dir / "preset.yml" if manifest_path.exists(): try: manifest = PresetManifest(manifest_path) + from ._manager_commands import _PresetCommandMethods + + resolver = PresetResolver(self.project_root) for tmpl in manifest.templates: + if tmpl.get("type") == "command": + name = tmpl.get("name") + if isinstance(name, str): + expanded = ( + _PresetCommandMethods._expand_command_selectors( + self, resolver, pack_dir, [tmpl] + ) + if is_regex_selector(name) + else [tmpl] + ) + removed_cmd_names.update( + item["name"] + for item in expanded + if isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + ) + removed_cmd_names.update( + alias + for alias in tmpl.get("aliases", []) + if isinstance(alias, str) + ) if ( tmpl.get("type") == "template" and tmpl.get("name") == "constitution-template" ): removed_constitution = True - if tmpl.get("type") == "command": - name = tmpl.get("name") - if isinstance(name, str): - removed_cmd_names.add(name) - for alias in tmpl.get("aliases", []): - if isinstance(alias, str): - removed_cmd_names.add(alias) except PresetValidationError: - # Invalid manifest — skip alias extraction; primary command - # names from registered_commands are still unregistered. pass + for cmd_names in registered_commands.values(): + removed_cmd_names.update(cmd_names) - affected_skill_dirs: Dict[ - Path, tuple[Optional[str], List[str]] - ] = {} + affected_skill_dirs: Dict[Path, tuple[Optional[str], List[str]]] = {} if registered_skills: restorable_skills = registered_skills # A skill tracked for a command-backed agent whose ai_skills is @@ -764,9 +796,7 @@ def remove(self, pack_id: str) -> bool: # shadowing the module-level name for this whole function. from .._init_options import load_init_options as _load_init_options - resolved_active = resolve_active_agent_for_registration( - self.project_root - ) + resolved_active = resolve_active_agent_for_registration(self.project_root) if ( isinstance(registered_skills, dict) and isinstance(resolved_active, str) @@ -774,17 +804,14 @@ def remove(self, pack_id: str) -> bool: and _CommandRegistrarForScope is not None and _CommandRegistrarForScope.AGENT_CONFIGS.get( resolved_active, {} - ).get("extension") != "/SKILL.md" - and not is_ai_skills_enabled( - _load_init_options(self.project_root) - ) + ).get("extension") + != "/SKILL.md" + and not is_ai_skills_enabled(_load_init_options(self.project_root)) ): raw_names = registered_skills.get(resolved_active) stale_names = [ name - for name in ( - raw_names if isinstance(raw_names, list) else [] - ) + for name in (raw_names if isinstance(raw_names, list) else []) if isinstance(name, str) ] restorable_skills = { @@ -813,16 +840,14 @@ def remove(self, pack_id: str) -> bool: CommandRegistrar = None if CommandRegistrar is not None: skill_coverage = ( - registered_skills - if isinstance(registered_skills, dict) - else {} + registered_skills if isinstance(registered_skills, dict) else {} ) commands_to_unregister: Dict[str, List[str]] = {} for agent_name, cmd_names in registered_commands.items(): is_native_skill_agent = ( - CommandRegistrar.AGENT_CONFIGS.get( - agent_name, {} - ).get("extension") + CommandRegistrar.AGENT_CONFIGS.get(agent_name, {}).get( + "extension" + ) == "/SKILL.md" ) if not is_native_skill_agent: @@ -833,9 +858,7 @@ def remove(self, pack_id: str) -> bool: covered_skill_names = { name for name in ( - raw_skill_names - if isinstance(raw_skill_names, list) - else [] + raw_skill_names if isinstance(raw_skill_names, list) else [] ) if isinstance(name, str) } @@ -848,9 +871,7 @@ def remove(self, pack_id: str) -> bool: ) ] if uncovered_commands: - commands_to_unregister[agent_name] = ( - uncovered_commands - ) + commands_to_unregister[agent_name] = uncovered_commands registered_commands = commands_to_unregister # Unregister non-skill command files from AI agents. @@ -874,6 +895,7 @@ def remove(self, pack_id: str) -> bool: ) except Exception as exc: import warnings + warnings.warn( f"Post-removal reconciliation failed for {pack_id}: {exc}. " f"Agent command files may be stale; reinstall affected presets " @@ -884,7 +906,12 @@ def remove(self, pack_id: str) -> bool: if removed_constitution: try: self._reconcile_constitution() - except (OSError, UnicodeDecodeError, PresetValidationError, ValueError) as exc: + except ( + OSError, + UnicodeDecodeError, + PresetValidationError, + ValueError, + ) as exc: import warnings warnings.warn( @@ -912,39 +939,55 @@ def list_installed(self) -> List[Dict[str, Any]]: try: manifest = PresetManifest(manifest_path) - provided_counts = {"commands": 0, "templates": 0, "scripts": 0, "hooks": 0} + provided_counts = { + "commands": 0, + "templates": 0, + "scripts": 0, + "hooks": 0, + } for template in manifest.templates: provided_counts[f"{template['type']}s"] += 1 author = manifest.author - result.append({ - "id": pack_id, - "name": manifest.name, - "version": metadata.get("version", manifest.version), - "description": manifest.description, - "enabled": metadata.get("enabled", True), - "installed_at": metadata.get("installed_at"), - "template_count": len(manifest.templates), - "tags": manifest.tags, - "priority": normalize_priority(metadata.get("priority")), - "_json_author": author if isinstance(author, str) and author else None, - "_json_source": metadata.get("source"), - "_json_provides": provided_counts, - }) + result.append( + { + "id": pack_id, + "name": manifest.name, + "version": metadata.get("version", manifest.version), + "description": manifest.description, + "enabled": metadata.get("enabled", True), + "installed_at": metadata.get("installed_at"), + "template_count": len(manifest.templates), + "tags": manifest.tags, + "priority": normalize_priority(metadata.get("priority")), + "_json_author": author + if isinstance(author, str) and author + else None, + "_json_source": metadata.get("source"), + "_json_provides": provided_counts, + } + ) except PresetValidationError: - result.append({ - "id": pack_id, - "name": pack_id, - "version": metadata.get("version", "unknown"), - "description": "⚠️ Corrupted preset", - "enabled": False, - "installed_at": metadata.get("installed_at"), - "template_count": 0, - "tags": [], - "priority": normalize_priority(metadata.get("priority")), - "_json_author": None, - "_json_source": metadata.get("source"), - "_json_provides": {"commands": 0, "templates": 0, "scripts": 0, "hooks": 0}, - }) + result.append( + { + "id": pack_id, + "name": pack_id, + "version": metadata.get("version", "unknown"), + "description": "⚠️ Corrupted preset", + "enabled": False, + "installed_at": metadata.get("installed_at"), + "template_count": 0, + "tags": [], + "priority": normalize_priority(metadata.get("priority")), + "_json_author": None, + "_json_source": metadata.get("source"), + "_json_provides": { + "commands": 0, + "templates": 0, + "scripts": 0, + "hooks": 0, + }, + } + ) return result diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index 4d665b45dc..e9d44ddbf2 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -15,8 +15,9 @@ resolve_active_agent_for_registration, ) from ..extensions import ExtensionRegistry -from ._manifest import PresetManifest +from ._manifest import PresetManifest, PresetValidationError from ._resolver import PresetResolver +from ._selectors import is_regex_selector, selector_matches def _substitute_core_template( @@ -46,7 +47,7 @@ def _substitute_core_template( # Derive the short name (strip "speckit." prefix) used by core command templates. short_name = cmd_name if short_name.startswith("speckit."): - short_name = short_name[len("speckit."):] + short_name = short_name[len("speckit.") :] resolver = PresetResolver(project_root) # Resolution order for the core template: @@ -91,10 +92,111 @@ def _substitute_core_template( class _PresetCommandMethods: """Command artifact methods shared through PresetManager's lifecycle state.""" + def _expand_command_selectors( + self, + resolver: PresetResolver, + preset_dir: Path, + command_templates: list[dict[str, Any]], + ) -> list[dict[str, Any]]: + """Expand command selectors to lower-layer concrete entries. + + A regex declaration is eligible only if its selector matches at least + one command contributed below the current preset; it is then replaced + by exact-name copies so existing registration/tracking sees only real + command names. + """ + presets = self.registry.list_by_priority() + current_id = preset_dir.name + current_index = next( + ( + i + for i, (preset_id, _meta) in enumerate(presets) + if preset_id == current_id + ), + None, + ) + if current_index is None: + return [ + declaration + for declaration in command_templates + if not is_regex_selector(str(declaration.get("name", ""))) + ] + lower_preset_ids = { + preset_id for preset_id, _meta in presets[current_index + 1 :] + } + lower_extension_ids = { + ext_id + for _priority, ext_id, _meta in resolver._get_all_extensions_by_priority() + } + try: + from ..artifacts.catalog import ArtifactCatalog + from ..extensions import CORE_COMMAND_NAMES + + candidates = ArtifactCatalog(self.project_root).list_artifacts() + except Exception as exc: + raise PresetValidationError( + f"Could not enumerate concrete commands for preset selector: {exc}" + ) from exc + concrete_candidates = { + artifact.name + for artifact in candidates + if artifact.kind == "command" + and isinstance(artifact.name, str) + and not is_regex_selector(artifact.name) + } + concrete_candidates.update(f"speckit.{name}" for name in CORE_COMMAND_NAMES) + concrete_names = sorted( + { + name + for name in concrete_candidates + if self._command_name_has_lower_layer( + resolver, name, lower_preset_ids, lower_extension_ids + ) + } + ) + + expanded: list[dict[str, Any]] = [] + for declaration in command_templates: + name = declaration.get("name") + if not isinstance(name, str) or not is_regex_selector(name): + expanded.append(declaration) + continue + expanded.extend( + {**declaration, "name": concrete_name} + for concrete_name in concrete_names + if selector_matches(name, concrete_name) + ) + return expanded + + @staticmethod + def _command_name_has_lower_layer( + resolver: PresetResolver, + name: str, + lower_preset_ids: set[str], + lower_extension_ids: set[str], + ) -> bool: + """Use the resolver's concrete stack, excluding project override/current preset.""" + for layer in resolver.collect_all_layers(name, "command"): + source = layer["source"] + if source == "project override": + continue + if any( + source.startswith(f"{preset_id} v") for preset_id in lower_preset_ids + ): + return True + if any( + source.startswith(f"extension:{ext_id}") + for ext_id in lower_extension_ids + ): + return True + if source in {"core", "core (bundled)"}: + return True + return False + def _register_commands( self, manifest: PresetManifest, - preset_dir: Path + preset_dir: Path, ) -> Dict[str, List[str]]: """Register preset command overrides with all detected AI agents. @@ -113,9 +215,15 @@ def _register_commands( Returns: Dictionary mapping agent names to lists of registered command names """ - command_templates = [ + raw_command_templates = [ t for t in manifest.templates if t.get("type") == "command" ] + resolver = PresetResolver(self.project_root) + command_templates = self._expand_command_selectors( + resolver, + preset_dir, + raw_command_templates, + ) if not command_templates: return {} @@ -139,8 +247,8 @@ def _register_commands( # If a higher-priority replace already wins, skip composition # here — reconciliation will write the correct content. layers = resolver.collect_all_layers(cmd["name"], "command") - top_layer_is_ours = ( - layers and layers[0]["path"].is_relative_to(preset_dir) + top_layer_is_ours = layers and layers[0]["path"].is_relative_to( + preset_dir ) if top_layer_is_ours: composed = resolver.resolve_content(cmd["name"], "command") @@ -150,10 +258,12 @@ def _register_commands( composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd['name']}.md" composed_file.write_text(composed, encoding="utf-8") - commands_to_register.append({ - **cmd, - "file": f".composed/{cmd['name']}.md", - }) + commands_to_register.append( + { + **cmd, + "file": f".composed/{cmd['name']}.md", + } + ) else: # No base layer to compose onto (e.g. the command it # would wrap comes from an extension that isn't @@ -163,6 +273,7 @@ def _register_commands( # _reconcile_composed_commands so command-mode and # reconciliation behave identically. import warnings + warnings.warn( f"Command '{cmd['name']}' uses '{strategy}' " f"strategy but no base command layer exists to " @@ -268,7 +379,9 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: agent_config = CommandRegistrar().AGENT_CONFIGS.get(agent_name) except ImportError: agent_config = None - is_command_backed = bool(agent_config) and agent_config.get("extension") != "/SKILL.md" + is_command_backed = ( + bool(agent_config) and agent_config.get("extension") != "/SKILL.md" + ) ai_skills_now = is_command_backed and is_ai_skills_enabled( load_init_options(self.project_root) ) @@ -285,27 +398,27 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: ) if candidate_manifest is None: continue - for template in candidate_manifest.templates: + command_templates = [ + template + for template in candidate_manifest.templates + if template.get("type") == "command" + ] + expanded_for_reconcile = self._expand_command_selectors( + resolver, self.presets_dir / candidate_pack_id, command_templates + ) + for template in expanded_for_reconcile: command_name = template.get("name") - if ( - template.get("type") == "command" - and isinstance(command_name, str) - ): - if ( - resolver.overrides_dir / f"{command_name}.md" - ).is_file(): - project_override_commands.add(command_name) - winning_pack_by_command.setdefault( - command_name, candidate_pack_id + if not isinstance(command_name, str) or is_regex_selector(command_name): + continue + if (resolver.overrides_dir / f"{command_name}.md").is_file(): + project_override_commands.add(command_name) + winning_pack_by_command.setdefault(command_name, candidate_pack_id) + source_file = template.get("file") + if isinstance(source_file, str): + winning_source_by_command.setdefault( + command_name, + self.presets_dir / candidate_pack_id / source_file, ) - source_file = template.get("file") - if isinstance(source_file, str): - winning_source_by_command.setdefault( - command_name, - self.presets_dir - / candidate_pack_id - / source_file, - ) pending_command_cleanups: List[ tuple[ @@ -337,8 +450,19 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # any partial writes even when _register_commands never returns. for tmpl in manifest.templates: name = tmpl.get("name") - if tmpl.get("type") == "command" and isinstance(name, str): - affected_cmd_names.add(name) + if tmpl.get("type") != "command" or not isinstance(name, str): + continue + expanded_for_reconcile = ( + self._expand_command_selectors(resolver, pack_dir, [tmpl]) + if is_regex_selector(name) + else [tmpl] + ) + for expanded in expanded_for_reconcile: + concrete_name = expanded.get("name") + if isinstance(concrete_name, str) and not is_regex_selector( + concrete_name + ): + affected_cmd_names.add(concrete_name) # Isolate per-preset failures: one preset that fails to register # must not abort registration of the remaining enabled presets. @@ -355,9 +479,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: isinstance(primary_name, str) and primary_name in registered_command_names ): - successful_command_replacements.add( - (pack_id, primary_name) - ) + successful_command_replacements.add((pack_id, primary_name)) existing_commands = metadata.get("registered_commands", {}) if not isinstance(existing_commands, dict): existing_commands = {} @@ -391,7 +513,9 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # the commands phase already wrote to disk must still be # tracked so preset removal can clean them up (#2948). if merged_commands != existing_commands: - self.registry.update(pack_id, {"registered_commands": merged_commands}) + self.registry.update( + pack_id, {"registered_commands": merged_commands} + ) registered_skills = self._register_skills(manifest, pack_dir) replaced_skill_names = set(registered_skills.get(agent_name) or []) @@ -408,9 +532,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: modern_name in replaced_skill_names or legacy_name in replaced_skill_names ): - successful_skill_replacements.add( - (pack_id, primary_name) - ) + successful_skill_replacements.add((pack_id, primary_name)) raw_existing_skills = metadata.get("registered_skills") if isinstance(raw_existing_skills, list) and raw_existing_skills: # Legacy flat-list value: don't assume agent_name wrote @@ -435,7 +557,11 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: for name in registered_skills[agent_name] if name not in existing_names ] - elif is_command_backed and not ai_skills_now and merged_skills.get(agent_name): + elif ( + is_command_backed + and not ai_skills_now + and merged_skills.get(agent_name) + ): # Mirror image: toggled skills -> command for this same # agent. _get_skills_dir() no longer resolves a skills # directory once ai_skills is off, so _register_skills @@ -572,8 +698,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: for command_name, winning_pack_id in winning_pack_by_command.items() if command_name not in project_override_commands and ( - (winning_pack_id, command_name) - in successful_skill_replacements + (winning_pack_id, command_name) in successful_skill_replacements or ( command_name in reconciled_skills and command_name in winning_source_by_command @@ -609,17 +734,14 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: merged_commands[agent_name] = remaining_stale else: merged_commands.pop(agent_name, None) - self.registry.update( - pack_id, {"registered_commands": merged_commands} - ) + self.registry.update(pack_id, {"registered_commands": merged_commands}) successfully_replaced_command_winners = { command_name for command_name, winning_pack_id in winning_pack_by_command.items() if command_name not in project_override_commands and ( - (winning_pack_id, command_name) - in successful_command_replacements + (winning_pack_id, command_name) in successful_command_replacements or ( command_name in reconciled_commands and command_name in winning_source_by_command @@ -669,9 +791,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: merged_skills[agent_name] = remaining_stale else: merged_skills.pop(agent_name, None) - self.registry.update( - pack_id, {"registered_skills": merged_skills} - ) + self.registry.update(pack_id, {"registered_skills": merged_skills}) def unregister_agent_artifacts(self, agent_name: str) -> None: """Remove ``agent_name``'s tracked preset command/skill artifacts. @@ -763,9 +883,8 @@ def unregister_agent_artifacts(self, agent_name: str) -> None: ) shared_names: set[str] = set() for other_agent, other_names in registered_commands.items(): - if ( - other_agent == agent_name - or not isinstance(other_names, list) + if other_agent == agent_name or not isinstance( + other_names, list ): continue other_config = registrar.AGENT_CONFIGS.get(other_agent) @@ -779,29 +898,19 @@ def unregister_agent_artifacts(self, agent_name: str) -> None: ) if other_output == agent_output: shared_names.update( - name - for name in other_names - if isinstance(name, str) + name for name in other_names if isinstance(name, str) ) command_names_to_unregister = [ - name - for name in agent_command_names - if name not in shared_names + name for name in agent_command_names if name not in shared_names ] if command_names_to_unregister: - self._unregister_commands( - {agent_name: command_names_to_unregister} - ) + self._unregister_commands({agent_name: command_names_to_unregister}) new_registered_commands = copy.deepcopy(registered_commands) new_registered_commands.pop(agent_name, None) updates["registered_commands"] = new_registered_commands agent_skill_names = registered_skills_all.get(agent_name) or [] - if ( - agent_skill_names - or skills_migrated - or native_skills_entry_removed - ): + if agent_skill_names or skills_migrated or native_skills_entry_removed: if agent_skill_names: self._delete_agent_preset_skills( agent_name, agent_skill_names, pack_id @@ -1025,13 +1134,22 @@ def record_written(written: Dict[str, List[str]]) -> None: manifest = resolver._get_manifest(pack_dir) if manifest: for tmpl in manifest.templates: - if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": + if ( + tmpl.get("name") == cmd_name + and tmpl.get("type") == "command" + ): written = self._register_for_non_skill_agents( - registrar, [tmpl], manifest.id, pack_dir, - only_agent=only_agent, extra_agents=extra_agents, + registrar, + [tmpl], + manifest.id, + pack_dir, + only_agent=only_agent, + extra_agents=extra_agents, ) record_written(written) - self._merge_pack_registered_commands(manifest.id, written) + self._merge_pack_registered_commands( + manifest.id, written + ) registered = True break break @@ -1051,15 +1169,19 @@ def record_written(written: Dict[str, List[str]]) -> None: if ext_manifest_path.exists(): try: from ..extensions import ExtensionManifest + ext_manifest = ExtensionManifest(ext_manifest_path) # Filter to only the command being reconciled matching_cmds = [ - c for c in ext_manifest.commands + c + for c in ext_manifest.commands if c.get("name") == cmd_name ] if matching_cmds: written = registrar.register_commands_for_non_skill_agents( - matching_cmds, extension_id, ext_dir, + matching_cmds, + extension_id, + ext_dir, self.project_root, context_note=f"\n\n\n", extension_id=extension_id, @@ -1075,15 +1197,16 @@ def record_written(written: Dict[str, List[str]]) -> None: if not registered: source_id = extension_id or source written = self._register_command_from_path( - registrar, cmd_name, top_path, + registrar, + cmd_name, + top_path, source_id=source_id, - only_agent=only_agent, extra_agents=extra_agents, + only_agent=only_agent, + extra_agents=extra_agents, ) record_written(written) if extension_id: - self._merge_extension_registered_commands( - extension_id, written - ) + self._merge_extension_registered_commands(extension_id, written) else: # Composed command — resolve from full stack composed = resolver.resolve_content(cmd_name, "command") @@ -1091,6 +1214,7 @@ def record_written(written: Dict[str, List[str]]) -> None: # Composition no longer possible (e.g. base layer removed). # Unregister any stale command file from non-skill agents. import warnings + warnings.warn( f"Cannot compose command '{cmd_name}': no base layer. " f"Stale command files may remain.", @@ -1104,7 +1228,10 @@ def record_written(written: Dict[str, List[str]]) -> None: _m = resolver._get_manifest(_pd) if _m: for _t in _m.templates: - if _t.get("name") == cmd_name and _t.get("type") == "command": + if ( + _t.get("name") == cmd_name + and _t.get("type") == "command" + ): for alias in _t.get("aliases", []): if isinstance(alias, str): cmd_names_to_unregister.append(alias) @@ -1118,7 +1245,8 @@ def record_written(written: Dict[str, List[str]]) -> None: { agent: cmd_names_to_unregister for agent in registrar.AGENT_CONFIGS - if registrar.AGENT_CONFIGS[agent].get("extension") != "/SKILL.md" + if registrar.AGENT_CONFIGS[agent].get("extension") + != "/SKILL.md" and ( only_agent is None or agent == only_agent @@ -1137,7 +1265,10 @@ def record_written(written: Dict[str, List[str]]) -> None: if not manifest: continue for tmpl in manifest.templates: - if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": + if ( + tmpl.get("name") == cmd_name + and tmpl.get("type") == "command" + ): composed_dir = pack_dir / ".composed" composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" @@ -1145,8 +1276,10 @@ def record_written(written: Dict[str, List[str]]) -> None: written = self._register_for_non_skill_agents( registrar, [{**tmpl, "file": f".composed/{cmd_name}.md"}], - manifest.id, pack_dir, - only_agent=only_agent, extra_agents=extra_agents, + manifest.id, + pack_dir, + only_agent=only_agent, + extra_agents=extra_agents, ) record_written(written) self._merge_pack_registered_commands(manifest.id, written) @@ -1168,15 +1301,16 @@ def record_written(written: Dict[str, List[str]]) -> None: else: source_id = source written = self._register_command_from_path( - registrar, cmd_name, composed_file, + registrar, + cmd_name, + composed_file, source_id=source_id, - only_agent=only_agent, extra_agents=extra_agents, + only_agent=only_agent, + extra_agents=extra_agents, ) record_written(written) if source.startswith("extension:"): - self._merge_extension_registered_commands( - source_id, written - ) + self._merge_extension_registered_commands(source_id, written) return reconciled_commands @@ -1219,7 +1353,10 @@ def _register_command_from_path( if source_id and not source_id.startswith("preset:"): try: from ..extensions import ExtensionManifest - for ext_dir in (self.project_root / ".specify" / "extensions").iterdir(): + + for ext_dir in ( + self.project_root / ".specify" / "extensions" + ).iterdir(): if not ext_dir.is_dir(): continue if cmd_path.is_relative_to(ext_dir): @@ -1236,8 +1373,12 @@ def _register_command_from_path( except Exception: pass # best-effort alias loading return self._register_for_non_skill_agents( - registrar, [cmd_tmpl], source_id, cmd_path.parent, - only_agent=only_agent, extra_agents=extra_agents, + registrar, + [cmd_tmpl], + source_id, + cmd_path.parent, + only_agent=only_agent, + extra_agents=extra_agents, ) def _register_for_non_skill_agents( @@ -1279,6 +1420,10 @@ def _register_for_non_skill_agents( ``registered_commands`` tracking (#2948). """ return registrar.register_commands_for_non_skill_agents( - commands, source_id, source_dir, self.project_root, - only_agent=only_agent, extra_agents=extra_agents, + commands, + source_id, + source_dir, + self.project_root, + only_agent=only_agent, + extra_agents=extra_agents, ) diff --git a/src/specify_cli/presets/_manifest.py b/src/specify_cli/presets/_manifest.py index b5a7ff03c3..90878ad2fe 100644 --- a/src/specify_cli/presets/_manifest.py +++ b/src/specify_cli/presets/_manifest.py @@ -13,16 +13,19 @@ class PresetError(Exception): """Base exception for preset-related errors.""" + pass class PresetValidationError(PresetError): """Raised when preset manifest validation fails.""" + pass class PresetCompatibilityError(PresetError): """Raised when preset is incompatible with current environment.""" + pass @@ -54,7 +57,7 @@ def __init__(self, manifest_path: Path): def _load_yaml(self, path: Path) -> dict: """Load YAML file safely.""" try: - with open(path, 'r', encoding='utf-8') as f: + with open(path, "r", encoding="utf-8") as f: data = yaml.safe_load(f) except yaml.YAMLError as e: raise PresetValidationError(f"Invalid YAML in {path}: {e}") @@ -90,9 +93,7 @@ def _validate(self): for section in ("preset", "requires", "provides"): if not isinstance(self.data[section], dict): - raise PresetValidationError( - f"Invalid {section}: expected a mapping" - ) + raise PresetValidationError(f"Invalid {section}: expected a mapping") # Validate preset metadata pack = self.data["preset"] @@ -117,7 +118,7 @@ def _validate(self): ) # Validate pack ID format - if not re.match(r'^[a-z0-9-]+$', pack["id"]): + if not re.match(r"^[a-z0-9-]+$", pack["id"]): raise PresetValidationError( f"Invalid preset ID '{pack['id']}': " "must be lowercase alphanumeric with hyphens only" @@ -164,9 +165,7 @@ def _validate(self): # Validate provides section provides = self.data["provides"] if "templates" not in provides: - raise PresetValidationError( - "Preset must provide at least one template" - ) + raise PresetValidationError("Preset must provide at least one template") # Validate templates. Guard the container and each entry's shape so a # malformed third-party preset.yml (e.g. ``templates: 5`` or @@ -182,13 +181,9 @@ def _validate(self): # latter, since that genuinely is a list with no templates. templates = provides["templates"] if not isinstance(templates, list): - raise PresetValidationError( - "Invalid provides.templates: expected a list" - ) + raise PresetValidationError("Invalid provides.templates: expected a list") if not templates: - raise PresetValidationError( - "Preset must provide at least one template" - ) + raise PresetValidationError("Preset must provide at least one template") seen_name_types: set[tuple[str, str]] = set() for tmpl in templates: if not isinstance(tmpl, dict): @@ -264,20 +259,32 @@ def _validate(self): f"scripts only support {sorted(VALID_SCRIPT_STRATEGIES)}" ) - # Validate template name format - if tmpl["type"] == "command": - # Commands use dot notation (e.g. speckit.specify) - if not re.match(r'^[a-z0-9.-]+$', tmpl["name"]): + # Regex selectors are validated here, before installation or resolution. + # The logical resource name grammar is validated separately below. + from ._selectors import compile_name_selector, is_regex_selector + + if is_regex_selector(tmpl["name"]): + try: + compile_name_selector(tmpl["name"]) + except re.error as exc: raise PresetValidationError( - f"Invalid command name '{tmpl['name']}': " - "must be lowercase alphanumeric with hyphens and dots only" - ) + f"Invalid regex selector in template name {tmpl['name']!r}: {exc}" + ) from exc else: - if not re.match(r'^[a-z0-9-]+$', tmpl["name"]): - raise PresetValidationError( - f"Invalid template name '{tmpl['name']}': " - "must be lowercase alphanumeric with hyphens only" - ) + # Validate template name format + if tmpl["type"] == "command": + # Commands use dot notation (e.g. speckit.specify) + if not re.match(r"^[a-z0-9.-]+$", tmpl["name"]): + raise PresetValidationError( + f"Invalid command name '{tmpl['name']}': " + "must be lowercase alphanumeric with hyphens and dots only" + ) + else: + if not re.match(r"^[a-z0-9-]+$", tmpl["name"]): + raise PresetValidationError( + f"Invalid template name '{tmpl['name']}': " + "must be lowercase alphanumeric with hyphens only" + ) @property def id(self) -> str: @@ -355,7 +362,7 @@ def _validate_requires_extensions(declared: Any) -> None: # otherwise validate here while PresetResolver._is_safe_registry_id # (which uses fullmatch) rejects it, and the newline would land in # a suggested command. - if not re.fullmatch(r'[a-z0-9-]+', extension_id): + if not re.fullmatch(r"[a-z0-9-]+", extension_id): raise PresetValidationError( f"Invalid {label}.id {extension_id!r}: " "must be lowercase alphanumeric with hyphens only" @@ -432,7 +439,7 @@ def tags(self) -> List[str]: def get_hash(self) -> str: """Calculate SHA256 hash of manifest file.""" h = hashlib.sha256() - with open(self.path, 'rb') as f: + with open(self.path, "rb") as f: for chunk in iter(lambda: f.read(8192), b""): h.update(chunk) return f"sha256:{h.hexdigest()}" diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index ea1c4e03f9..90dd57f74e 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -10,6 +10,7 @@ from ..extensions import ExtensionRegistry, normalize_priority from ._manifest import VALID_PRESET_STRATEGIES, PresetManifest, PresetValidationError from ._registry import PresetRegistry +from ._selectors import is_regex_selector, selector_matches class PresetResolver: @@ -84,7 +85,13 @@ 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: + declared_name = tmpl.get("name") + if ( + isinstance(declared_name, str) + and not is_regex_selector(declared_name) + and declared_name == template_name + and tmpl.get("type") == template_type + ): file_path = tmpl.get("file") if file_path: manifest_candidate = pack_dir / file_path @@ -147,7 +154,9 @@ def _extension_manifest_declared_template( # symlinks in ext_dir's ancestors (e.g. a symlinked tmp dir # on macOS) and diverge from the unresolved paths convention # lookup returns for the same directory. - candidate.resolve().relative_to(ext_dir.resolve()) # raises ValueError if outside + candidate.resolve().relative_to( + ext_dir.resolve() + ) # raises ValueError if outside except (OSError, ValueError): return entry, None return entry, (candidate if candidate.is_file() else None) @@ -193,7 +202,9 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: # Skip disabled extensions if not metadata.get("enabled", True): continue - priority = normalize_priority(metadata.get("priority") if metadata else None) + priority = normalize_priority( + metadata.get("priority") if metadata else None + ) all_extensions.append((priority, ext_id, metadata)) # Add unregistered directories with implicit priority=10 @@ -217,7 +228,7 @@ def _core_stem(template_name: str) -> Optional[str]: or ``None`` otherwise. """ if template_name.startswith("speckit."): - return template_name[len("speckit."):] + return template_name[len("speckit.") :] return None def resolve( @@ -254,7 +265,6 @@ def resolve( if template_type == "script": ext = ".sh" # scripts use .sh; callers can also check .ps1 - # Priority 1: Project-local overrides if template_type == "script": override = self.overrides_dir / "scripts" / f"{template_name}{ext}" else: @@ -278,6 +288,10 @@ def resolve( entry, manifest_candidate = self._manifest_declared_template( pack_dir, template_name, template_type ) + if entry is None: + entry, manifest_candidate = self._regex_preset_declaration( + pack_dir, template_name, template_type + ) if manifest_candidate is not None: return manifest_candidate if entry is not None: @@ -346,6 +360,7 @@ def resolve( _locate_core_pack, _repo_root, ) + _core_pack = _locate_core_pack() if _core_pack is not None: # Wheel install path @@ -504,6 +519,140 @@ def resolve_with_source( return {"path": resolved_str, "source": "core"} + def _regex_preset_declaration( + self, pack_dir: Path, template_name: str, template_type: str + ) -> tuple[dict | None, Path | None]: + """Find a matching regex declaration only when a concrete lower layer exists.""" + if template_type not in {"template", "script"}: + return None, None + manifest = self._get_manifest(pack_dir) + if manifest is None: + return None, None + presets = self._get_all_presets_by_priority() + pack_ids = [pack_id for pack_id, _ in presets] + try: + current_index = pack_ids.index(pack_dir.name) + except ValueError: + return None, None + + # A selector only sees resources below its owning preset in the actual + # resolver stack, followed by enabled extensions and core. + for lower_id, _metadata in presets[current_index + 1 :]: + lower_dir = self.presets_dir / lower_id + if self._preset_has_concrete_resource( + lower_dir, template_name, template_type + ): + has_lower_resource = True + break + else: + has_lower_resource = False + + if not has_lower_resource: + for _priority, ext_id, _meta in self._get_all_extensions_by_priority(): + if self._extension_has_concrete_resource( + self.extensions_dir / ext_id, template_name, template_type + ): + has_lower_resource = True + break + + if not has_lower_resource: + has_lower_resource = self._core_has_concrete_resource( + template_name, template_type + ) + if not has_lower_resource: + return None, None + + for declaration in manifest.templates: + declared_name = declaration.get("name") + if ( + declaration.get("type") == template_type + and isinstance(declared_name, str) + and is_regex_selector(declared_name) + and selector_matches(declared_name, template_name) + ): + file_value = declaration.get("file") + path = pack_dir / file_value if isinstance(file_value, str) else None + return declaration, ( + path if path is not None and path.is_file() else None + ) + return None, None + + def _has_concrete_resource( + self, + base_dir: Path, + name: str, + template_type: str, + *, + is_extension: bool = False, + ) -> bool: + """Check concrete-resource existence using the same lookups as resolve().""" + if is_extension: + entry, candidate = self._extension_manifest_declared_template( + base_dir, name, template_type + ) + else: + entry, candidate = self._manifest_declared_template( + base_dir, name, template_type + ) + if candidate is not None: + return True + if entry is not None: + return False + return self._conventional_resource(base_dir, name, template_type) is not None + + def _preset_has_concrete_resource( + self, base_dir: Path, name: str, template_type: str + ) -> bool: + return self._has_concrete_resource(base_dir, name, template_type) + + def _extension_has_concrete_resource( + self, base_dir: Path, name: str, template_type: str + ) -> bool: + return self._has_concrete_resource( + base_dir, name, template_type, is_extension=True + ) + + def _core_has_concrete_resource(self, name: str, template_type: str) -> bool: + return self._core_resource(name, template_type) is not None + + def _conventional_resource( + self, base_dir: Path, name: str, template_type: str + ) -> Optional[Path]: + subdirs = { + "template": ("templates", ""), + "command": ("commands",), + "script": ("scripts",), + }.get(template_type, ("",)) + extension = ".sh" if template_type == "script" else ".md" + for subdir in subdirs: + candidate = ( + base_dir / subdir / f"{name}{extension}" + if subdir + else base_dir / f"{name}{extension}" + ) + if candidate.is_file(): + return candidate + return None + + def _core_resource(self, name: str, template_type: str) -> Optional[Path]: + extension = ".sh" if template_type == "script" else ".md" + if template_type == "template": + candidates = [self.templates_dir / f"{name}{extension}"] + elif template_type == "command": + candidates = [self.templates_dir / "commands" / f"{name}{extension}"] + stem = self._core_stem(name) + if stem: + candidates.append( + self.templates_dir / "commands" / f"{stem}{extension}" + ) + else: + candidates = [self.templates_dir / "scripts" / f"{name}{extension}"] + for candidate in candidates: + if candidate.is_file(): + return candidate + bundled = self._find_bundled_core(name, template_type, extension) + return bundled if bundled is not None and bundled.is_file() else None + def collect_all_layers( self, template_name: str, @@ -546,17 +695,18 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: return candidate return None - # Priority 1: Project-local overrides (always "replace" strategy) if template_type == "script": override = self.overrides_dir / "scripts" / f"{template_name}{ext}" else: override = self.overrides_dir / f"{template_name}{ext}" if override.exists(): - layers.append({ - "path": override, - "source": "project override", - "strategy": "replace", - }) + layers.append( + { + "path": override, + "source": "project override", + "strategy": "replace", + } + ) # Priority 2: Installed presets (sorted by priority — lower number = higher precedence) if self.presets_dir.exists(): @@ -568,6 +718,10 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: entry, manifest_candidate = self._manifest_declared_template( pack_dir, template_name, template_type ) + if entry is None: + entry, manifest_candidate = self._regex_preset_declaration( + pack_dir, template_name, template_type + ) if entry is not None: strategy = entry.get("strategy", "replace") manifest_has_strategy = "strategy" in entry @@ -586,7 +740,11 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: # strategy, check the command file's frontmatter for any valid # strategy. Skip when the manifest entry includes strategy key # (even if it's "replace") to avoid overriding explicit declarations. - if not manifest_has_strategy and strategy == "replace" and template_type == "command": + if ( + not manifest_has_strategy + and strategy == "replace" + and template_type == "command" + ): try: cmd_content = candidate.read_text(encoding="utf-8") lines = cmd_content.splitlines(keepends=True) @@ -601,18 +759,24 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: fm_data = yaml.safe_load(fm_text) if isinstance(fm_data, dict): fm_strategy = fm_data.get("strategy") - if isinstance(fm_strategy, str) and fm_strategy.lower() in VALID_PRESET_STRATEGIES: + if ( + isinstance(fm_strategy, str) + and fm_strategy.lower() + in VALID_PRESET_STRATEGIES + ): strategy = fm_strategy.lower() except (UnicodeDecodeError, yaml.YAMLError, OSError): # Best-effort legacy frontmatter parsing: keep default # strategy ("replace") when content is unreadable/invalid. pass version = metadata.get("version", "?") if metadata else "?" - layers.append({ - "path": candidate, - "source": f"{pack_id} v{version}", - "strategy": strategy, - }) + layers.append( + { + "path": candidate, + "source": f"{pack_id} v{version}", + "strategy": strategy, + } + ) # Priority 3: Extension-provided templates (always "replace") for _priority, ext_id, ext_meta in self._get_all_extensions_by_priority(): @@ -634,13 +798,15 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: source = f"extension:{ext_id} v{version}" else: source = f"extension:{ext_id} (unregistered)" - layers.append({ - "path": candidate, - "source": source, - "strategy": "replace", - "extension_id": ext_id, - "extension_dir": ext_dir, - }) + layers.append( + { + "path": candidate, + "source": source, + "strategy": "replace", + "extension_id": ext_id, + "extension_dir": ext_dir, + } + ) # Priority 4: Core templates (always "replace") core = None @@ -664,21 +830,25 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: if c.exists(): core = c if core: - layers.append({ - "path": core, - "source": "core", - "strategy": "replace", - }) + layers.append( + { + "path": core, + "source": "core", + "strategy": "replace", + } + ) else: # Priority 5: Bundled core_pack (wheel install) or repo-root # templates (source-checkout), matching resolve()'s tier-5 fallback. bundled = self._find_bundled_core(template_name, template_type, ext) if bundled: - layers.append({ - "path": bundled, - "source": "core (bundled)", - "strategy": "replace", - }) + layers.append( + { + "path": bundled, + "source": "core (bundled)", + "strategy": "replace", + } + ) return layers @@ -850,8 +1020,8 @@ def _split_frontmatter(text: str) -> tuple: if fence_end == -1: return None, text - fm_block = "".join(lines[:fence_end + 1]).rstrip("\r\n") - body = "".join(lines[fence_end + 1:]) + fm_block = "".join(lines[: fence_end + 1]).rstrip("\r\n") + body = "".join(lines[fence_end + 1 :]) return fm_block, body if is_command: @@ -907,6 +1077,7 @@ def _split_frontmatter(text: str) -> tuple: # inheriting scripts/agent_scripts from the base if missing # and stripping the strategy key (internal-only, not for agent output). if is_command and top_frontmatter_text: + def _parse_fm_yaml(fm_block: str) -> dict: """Parse YAML from a frontmatter block (with --- fences).""" lines = fm_block.splitlines() @@ -934,11 +1105,7 @@ def _parse_fm_yaml(fm_block: str) -> dict: top_fm.pop("strategy", None) if top_fm: - top_frontmatter_text = ( - "---\n" - + dump_frontmatter(top_fm) - + "\n---" - ) + top_frontmatter_text = "---\n" + dump_frontmatter(top_fm) + "\n---" else: # Empty frontmatter — omit rather than emitting {} top_frontmatter_text = None diff --git a/src/specify_cli/presets/_selectors.py b/src/specify_cli/presets/_selectors.py new file mode 100644 index 0000000000..f7ab978d2d --- /dev/null +++ b/src/specify_cli/presets/_selectors.py @@ -0,0 +1,30 @@ +"""Thin helpers for exact preset names and ``regex:`` selectors.""" + +from __future__ import annotations + +import re +from typing import Pattern + +REGEX_PREFIX = "regex:" + + +def is_regex_selector(name: str) -> bool: + """Return whether a manifest name opts into regex selector semantics.""" + return name.startswith(REGEX_PREFIX) + + +def compile_name_selector(name: str) -> Pattern[str] | None: + """Compile a regex selector, or return None for an exact-name declaration.""" + if not is_regex_selector(name): + return None + return re.compile(name[len(REGEX_PREFIX) :]) + + +def selector_matches(name: str, concrete_name: str) -> bool: + """Full-match a concrete logical name; exact declarations stay exact.""" + compiled = compile_name_selector(name) + return ( + compiled.fullmatch(concrete_name) is not None + if compiled + else name == concrete_name + ) diff --git a/src/specify_cli/presets/command_enable.py b/src/specify_cli/presets/command_enable.py index c3cd3ba5ff..d335b6f8d2 100644 --- a/src/specify_cli/presets/command_enable.py +++ b/src/specify_cli/presets/command_enable.py @@ -38,6 +38,37 @@ def preset_enable( # Enable the preset manager.registry.update(preset_id, {"enabled": True}) + try: + from ._manifest import PresetManifest + from ._resolver import PresetResolver + from ._selectors import is_regex_selector + + manifest_path = manager.presets_dir / preset_id / "preset.yml" + if manifest_path.is_file(): + manifest = PresetManifest(manifest_path) + expanded = manager._expand_command_selectors( + PresetResolver(project_root), + manager.presets_dir / preset_id, + [item for item in manifest.templates if item.get("type") == "command"], + ) + names = sorted( + { + item["name"] + for item in expanded + if isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + } + ) + if names: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) + except Exception as exc: + import warnings + + warnings.warn( + f"Could not reconcile preset commands after enabling {preset_id}: {exc}", + stacklevel=2, + ) manager.reconcile_constitution( f"Failed to reconcile constitution after enabling preset {preset_id}" ) diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index a3920a6605..71e0ded0cf 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -9,6 +9,87 @@ from ._commands import preset_app +def _diagnostic_selector_matches(resolver, preset_dir, selector, resource_type): + import os + from .._selectors import is_regex_selector, selector_matches + + ordered_presets = resolver._get_all_presets_by_priority() + try: + current_index = next( + index + for index, (preset_id, _meta) in enumerate(ordered_presets) + if preset_id == preset_dir.name + ) + except StopIteration: + return [] + lower_presets = [ + resolver.presets_dir / preset_id + for preset_id, _meta in ordered_presets[current_index + 1 :] + ] + lower_extensions = [ + resolver.extensions_dir / ext_id + for _priority, ext_id, _meta in resolver._get_all_extensions_by_priority() + ] + candidates = set() + for base in [*lower_presets, *lower_extensions]: + manifest = ( + resolver._get_manifest(base) + if base.parent == resolver.presets_dir + else None + ) + if manifest: + candidates.update( + item["name"] + for item in manifest.templates + if item.get("type") == resource_type + and isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + ) + subdir = "templates" if resource_type == "template" else "scripts" + suffix = ".sh" if resource_type == "script" else ".md" + for path in (base / subdir).glob("**/*") if (base / subdir).is_dir() else []: + if path.is_file() and path.name.endswith(suffix): + candidates.add( + os.path.relpath(path, base / subdir)[: -len(suffix)].replace( + os.sep, "-" + ) + ) + suffix = ".sh" if resource_type == "script" else ".md" + core_roots = [ + resolver.templates_dir / ("scripts" if resource_type == "script" else ""), + ] + for root in core_roots: + for path in root.glob("**/*") if root.is_dir() else []: + if path.is_file() and path.name.endswith(suffix): + candidates.add( + os.path.relpath(path, root)[: -len(suffix)].replace(os.sep, "-") + ) + matches = [] + for name in candidates: + if ( + not isinstance(name, str) + or is_regex_selector(name) + or not selector_matches(selector, name) + ): + continue + exists = ( + any( + resolver._has_concrete_resource(base, name, resource_type) + for base in lower_presets + ) + or any( + resolver._has_concrete_resource( + base, name, resource_type, is_extension=True + ) + for base in lower_extensions + ) + or resolver._core_has_concrete_resource(name, resource_type) + ) + if exists: + matches.append(name) + return sorted(matches) + + @preset_app.command("info") def preset_info( preset_id: str = typer.Argument(..., help="Preset ID to get info about"), @@ -37,12 +118,43 @@ def preset_info( if isinstance(local_tags, list) and local_tags: tags_str = _escape_markup(", ".join(str(t) for t in local_tags)) console.print(f" Tags: {tags_str}") - console.print(f" Templates: {len(local_pack.templates)}") + from ._selectors import is_regex_selector + from ._resolver import PresetResolver + + resolver = PresetResolver(project_root) + preset_dir = manager.presets_dir / local_pack.id for tmpl in local_pack.templates: tmpl_name = _escape_markup(str(tmpl["name"])) tmpl_type = _escape_markup(str(tmpl["type"])) tmpl_desc = _escape_markup(str(tmpl.get("description", ""))) console.print(f" - {tmpl_name} ({tmpl_type}): {tmpl_desc}") + if ( + tmpl.get("type") == "command" + and isinstance(tmpl.get("name"), str) + and is_regex_selector(tmpl["name"]) + ): + matches = manager._expand_command_selectors( + resolver, preset_dir, [tmpl] + ) + elif ( + tmpl.get("type") in {"template", "script"} + and isinstance(tmpl.get("name"), str) + and is_regex_selector(tmpl["name"]) + ): + matches = [ + {"name": name} + for name in _diagnostic_selector_matches( + resolver, preset_dir, tmpl["name"], tmpl["type"] + ) + ] + else: + matches = [] + if is_regex_selector(str(tmpl.get("name", ""))): + if matches: + for match in matches: + console.print(f" - {_escape_markup(str(match['name']))}") + else: + console.print(" [dim]No current matches[/dim]") repo = local_pack.data.get("preset", {}).get("repository") if repo: console.print(f" Repository: {_escape_markup(str(repo))}") diff --git a/src/specify_cli/presets/command_set_priority.py b/src/specify_cli/presets/command_set_priority.py index 8337c251fa..41f8487f24 100644 --- a/src/specify_cli/presets/command_set_priority.py +++ b/src/specify_cli/presets/command_set_priority.py @@ -56,8 +56,31 @@ def preset_set_priority( old_priority = normalize_priority(raw_priority) - # Update priority + from ._resolver import PresetResolver + from ._selectors import is_regex_selector + + resolver = PresetResolver(project_root) + affected_commands: set[str] = set() + for pack_id, _pack_metadata in manager.registry.list_by_priority(): + manifest = resolver._get_manifest(manager.presets_dir / pack_id) + if manifest is None: + continue + expanded = manager._expand_command_selectors( + resolver, + manager.presets_dir / pack_id, + [item for item in manifest.templates if item.get("type") == "command"], + ) + affected_commands.update( + item["name"] + for item in expanded + if isinstance(item.get("name"), str) and not is_regex_selector(item["name"]) + ) + manager.registry.update(preset_id, {"priority": priority}) + names = sorted(affected_commands) + if names: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) manager.reconcile_constitution( f"Failed to reconcile constitution after changing priority for preset {preset_id}" ) diff --git a/tests/specify_cli/presets/test_regex_selectors.py b/tests/specify_cli/presets/test_regex_selectors.py new file mode 100644 index 0000000000..a809963523 --- /dev/null +++ b/tests/specify_cli/presets/test_regex_selectors.py @@ -0,0 +1,437 @@ +from __future__ import annotations + +import json + +import pytest +import yaml + +from specify_cli.extensions import CORE_COMMAND_NAMES, ExtensionRegistry +from specify_cli.presets import ( + PresetManager, + PresetManifest, + PresetRegistry, + PresetResolver, + PresetValidationError, +) +from specify_cli.presets._selectors import selector_matches + + +def _manifest( + name: str, resource_type: str = "template", strategy: str = "replace" +) -> dict: + return { + "schema_version": "1.0", + "preset": { + "id": "preset", + "name": "Preset", + "version": "1.0.0", + "description": "test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": resource_type, + "name": name, + "file": "payload.md" if resource_type != "script" else "payload.sh", + "strategy": strategy, + } + ] + }, + } + + +def _write_preset( + project, + preset_id: str, + name: str, + *, + priority: int, + strategy: str = "replace", + resource_type: str = "template", + body: str = "overlay\n", +): + root = project / ".specify" / "presets" / preset_id + root.mkdir(parents=True, exist_ok=True) + data = _manifest(name, resource_type, strategy) + data["preset"]["id"] = preset_id + filename = data["provides"]["templates"][0]["file"] + (root / filename).write_text(body, encoding="utf-8") + (root / "preset.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + PresetRegistry(project / ".specify" / "presets").add( + preset_id, {"enabled": True, "priority": priority, "version": "1.0.0"} + ) + return root + + +def test_command_regex_expands_core_commands_to_exact_names(project_dir): + selector_pack = _write_preset( + project_dir, + "command-selector", + r"regex:^speckit\.(plan|tasks)$", + priority=1, + resource_type="command", + ) + manager = PresetManager(project_dir) + manifest = PresetManifest(selector_pack / "preset.yml") + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), selector_pack, manifest.templates + ) + names = {entry["name"] for entry in expanded} + expected = {f"speckit.{name}" for name in CORE_COMMAND_NAMES} + assert names == expected & {"speckit.plan", "speckit.tasks"} + + +def test_command_regex_expands_extension_command(project_dir): + ext = project_dir / ".specify" / "extensions" / "demo" + (ext / "commands").mkdir(parents=True) + (ext / "commands" / "speckit.demo.md").write_text( + "extension command\\n", encoding="utf-8" + ) + _write_preset( + project_dir, + "command-selector", + r"regex:^speckit\.demo$", + priority=1, + resource_type="command", + ) + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "demo", {"enabled": True, "priority": 10, "version": "1.0"} + ) + pack = project_dir / ".specify" / "presets" / "command-selector" + manager = PresetManager(project_dir) + manifest = PresetManifest(pack / "preset.yml") + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), pack, manifest.templates + ) + assert [entry["name"] for entry in expanded] == ["speckit.demo"] + + +def test_command_regex_expands_lower_preset_and_excludes_own_or_higher(project_dir): + lower = project_dir / ".specify" / "presets" / "lower" + lower.mkdir(parents=True) + (lower / "commands").mkdir() + (lower / "payload.md").write_text("base\n", encoding="utf-8") + lower_data = _manifest("speckit.lower", "command") + lower_data["preset"]["id"] = "lower" + lower_data["provides"]["templates"][0]["file"] = "payload.md" + (lower / "preset.yml").write_text(yaml.safe_dump(lower_data), encoding="utf-8") + PresetRegistry(project_dir / ".specify" / "presets").add( + "lower", {"enabled": True, "priority": 10, "version": "1.0"} + ) + selector_pack = _write_preset( + project_dir, + "selector", + r"regex:^speckit\.lower$", + priority=1, + resource_type="command", + ) + manager = PresetManager(project_dir) + manifest = PresetManifest(selector_pack / "preset.yml") + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), selector_pack, manifest.templates + ) + assert [entry["name"] for entry in expanded] == ["speckit.lower"] + + +def test_command_regex_and_regex_overlap_preserves_both_entries(project_dir): + selector_pack = _write_preset( + project_dir, + "double-selector", + r"regex:^speckit\.plan$", + priority=1, + resource_type="command", + ) + second = {"type": "command", "name": r"regex:^speckit\.plan$", "file": "payload.md"} + manager = PresetManager(project_dir) + manifest = PresetManifest(selector_pack / "preset.yml") + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), selector_pack, [*manifest.templates, second] + ) + assert [entry["name"] for entry in expanded] == ["speckit.plan", "speckit.plan"] + + +def test_command_regex_registration_tracks_and_cleans_only_concrete_names(project_dir): + ( + project_dir / ".specify" / "templates" / "commands" / "speckit.plan.md" + ).write_text("# Plan\n", encoding="utf-8") + ( + project_dir / ".specify" / "templates" / "commands" / "speckit.tasks.md" + ).write_text("# Tasks\n", encoding="utf-8") + commands_dir = project_dir / ".agents" / "commands" + commands_dir.mkdir(parents=True) + (project_dir / ".specify" / "init-options.json").write_text( + json.dumps({"ai": "amp", "ai_skills": False, "script": "sh"}), + encoding="utf-8", + ) + source_dir = project_dir / "selector-source" + source_dir.mkdir() + source = _write_preset( + source_dir, + "command-selector", + r"regex:^speckit\.(plan|tasks)$", + priority=10, + resource_type="command", + ) + # Core discovery in this isolated project uses the packaged command inventory. + manager = PresetManager(project_dir) + source = source_dir / ".specify" / "presets" / "command-selector" + manifest = manager.install_from_directory(source, "0.1.0") + metadata = manager.registry.get(manifest.id) + registered = metadata["registered_commands"] + tracked = {name for names in registered.values() for name in names} + expected = {f"speckit.{name}" for name in CORE_COMMAND_NAMES} & { + "speckit.plan", + "speckit.tasks", + } + assert tracked == expected + assert all(not name.startswith("regex:") for name in tracked) + files = list(commands_dir.iterdir()) + assert files + assert all(path.exists() for path in files) + assert all( + path.name.startswith(("speckit.plan", "speckit.tasks")) for path in files + ) + assert all(not path.name.startswith("regex:") for path in files) + manager.remove(manifest.id) + + +def test_command_regex_zero_match_and_fullmatch(project_dir): + selector_pack = _write_preset( + project_dir, + "no-command-match", + r"regex:^missing\.command$", + priority=1, + resource_type="command", + ) + manager = PresetManager(project_dir) + manifest = PresetManifest(selector_pack / "preset.yml") + assert ( + manager._expand_command_selectors( + PresetResolver(project_dir), selector_pack, manifest.templates + ) + == [] + ) + + +def test_command_regex_and_exact_overlap_keep_only_concrete_names(project_dir): + _write_preset( + project_dir, + "overlap-selector", + r"regex:^speckit\.plan$", + priority=1, + resource_type="command", + ) + selector_pack = project_dir / ".specify" / "presets" / "overlap-selector" + exact = {"type": "command", "name": "speckit.plan", "file": "commands/exact.md"} + manager = PresetManager(project_dir) + manifest = PresetManifest(selector_pack / "preset.yml") + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), selector_pack, [exact, *manifest.templates] + ) + assert "regex:" not in " ".join(entry["name"] for entry in expanded) + assert [entry["name"] for entry in expanded].count("speckit.plan") == 2 + + +def test_exact_name_manifest_still_validates(tmp_path): + path = tmp_path / "preset.yml" + path.write_text(yaml.safe_dump(_manifest("plan-template")), encoding="utf-8") + manifest = PresetManifest(path) + assert manifest.templates[0]["name"] == "plan-template" + assert selector_matches("plan-template", "plan-template") + assert not selector_matches("plan-template", "other-template") + + +def test_exact_name_manifest_keeps_legacy_trailing_newline_validation(tmp_path): + # Legacy `re.match(r'^[...]$')` accepts one trailing newline; preserve it. + path = tmp_path / "preset.yml" + path.write_text(yaml.safe_dump(_manifest("plan-template\n")), encoding="utf-8") + manifest = PresetManifest(path) + assert manifest.templates[0]["name"] == "plan-template\n" + + +def test_regex_validation_and_fullmatch(tmp_path): + path = tmp_path / "preset.yml" + selector = r"regex:^plan-.*-template$" + path.write_text(yaml.safe_dump(_manifest(selector)), encoding="utf-8") + assert PresetManifest(path).templates[0]["name"] == selector + assert selector_matches(selector, "plan-main-template") + assert not selector_matches("regex:plan", "plan-template") + + +def test_invalid_regex_fails_during_manifest_validation(tmp_path): + path = tmp_path / "preset.yml" + path.write_text(yaml.safe_dump(_manifest("regex:[unterminated")), encoding="utf-8") + with pytest.raises(PresetValidationError, match="Invalid regex selector"): + PresetManifest(path) + + +def test_template_regex_matches_core_resource_only(project_dir): + _write_preset( + project_dir, "regex-pack", "regex:.*-template$", priority=10, strategy="append" + ) + layers = PresetResolver(project_dir).collect_all_layers("plan-template", "template") + assert any(layer["source"].startswith("regex-pack") for layer in layers) + assert ( + PresetResolver(project_dir).collect_all_layers("does-not-exist", "template") + == [] + ) + + +def test_template_regex_matches_extension_resource(project_dir): + ext = project_dir / ".specify" / "extensions" / "demo" + (ext / "templates").mkdir(parents=True) + (ext / "templates" / "plan-template.md").write_text( + "extension base\n", encoding="utf-8" + ) + _write_preset( + project_dir, "regex-pack", "regex:.*-template$", priority=10, strategy="append" + ) + layers = PresetResolver(project_dir).collect_all_layers("plan-template", "template") + assert any(layer["source"].startswith("regex-pack") for layer in layers) + + +def test_template_regex_matches_lower_priority_preset(project_dir): + _write_preset( + project_dir, "high", "regex:.*-template$", priority=1, strategy="append" + ) + _write_preset(project_dir, "low", "plan-template", priority=10) + layers = PresetResolver(project_dir).collect_all_layers("plan-template", "template") + assert any(layer["source"].startswith("high") for layer in layers) + assert any(layer["source"].startswith("low") for layer in layers) + + +def test_regex_does_not_match_higher_preset_or_create_resource(project_dir): + _write_preset(project_dir, "higher", "only-higher", priority=1) + _write_preset(project_dir, "lower", "regex:^only-higher$", priority=10) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("only-higher", "template") + assert any(layer["source"].startswith("higher") for layer in layers) + assert not any(layer["source"].startswith("lower") for layer in layers) + assert resolver.collect_all_layers("does-not-exist", "template") == [] + assert resolver.collect_all_layers("regex:^only-higher$", "template") == [] + + +def test_same_priority_regex_layers_follow_preset_id_order(project_dir): + # Registry tie-break is alphabetical ID; both selectors match the same core template. + _write_preset( + project_dir, + "zeta-regex", + "regex:^plan-template$", + priority=10, + strategy="append", + body="zeta\\n", + ) + _write_preset( + project_dir, + "alpha-regex", + "regex:^plan-template$", + priority=10, + strategy="append", + body="alpha\\n", + ) + layers = PresetResolver(project_dir).collect_all_layers("plan-template", "template") + regex_sources = [ + layer["source"].split()[0] for layer in layers if "-regex" in layer["source"] + ] + assert regex_sources == ["alpha-regex", "zeta-regex"] + + +def test_project_override_is_not_regex_lower_layer_proof(project_dir): + overrides = project_dir / ".specify" / "templates" / "overrides" + overrides.mkdir(parents=True) + (overrides / "foo-template.md").write_text("override\n", encoding="utf-8") + _write_preset( + project_dir, + "regex-pack", + "regex:^foo-template$", + priority=10, + strategy="append", + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("foo-template", "template") + assert [layer["source"] for layer in layers] == ["project override"] + + +def test_project_override_does_not_hide_real_regex_lower_layer(project_dir): + overrides = project_dir / ".specify" / "templates" / "overrides" + overrides.mkdir(parents=True) + (overrides / "foo-template.md").write_text("override\n", encoding="utf-8") + _write_preset( + project_dir, + "regex-pack", + "regex:^foo-template$", + priority=10, + strategy="append", + ) + core = project_dir / ".specify" / "templates" / "foo-template.md" + core.parent.mkdir(parents=True, exist_ok=True) + core.write_text("core\\n", encoding="utf-8") + layers = PresetResolver(project_dir).collect_all_layers("foo-template", "template") + assert [layer["source"] for layer in layers] == [ + "project override", + "regex-pack v1.0.0", + "core", + ] + + +def test_exact_and_regex_layers_keep_existing_priority_composition(project_dir): + _write_preset( + project_dir, + "higher-regex", + "regex:^plan-template$", + priority=1, + strategy="append", + body="A\n", + ) + _write_preset( + project_dir, + "higher-exact", + "plan-template", + priority=2, + strategy="prepend", + body="B\n", + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("plan-template", "template") + assert [layer["source"].split(" ")[0] for layer in layers[:2]] == [ + "higher-regex", + "higher-exact", + ] + result = resolver.resolve_content("plan-template", "template") + assert result is not None and "A" in result and "B" in result + + +def test_script_regex_uses_same_resolver_and_strategy_restrictions(project_dir): + core_scripts = project_dir / ".specify" / "templates" / "scripts" + core_scripts.mkdir(parents=True) + (core_scripts / "check-main.sh").write_text( + "#!/bin/sh\necho core\n", encoding="utf-8" + ) + _write_preset( + project_dir, + "script-regex", + "regex:^check-.*$", + priority=10, + strategy="wrap", + resource_type="script", + body="#!/bin/sh\n{CORE_SCRIPT}\n", + ) + resolver = PresetResolver(project_dir) + assert any( + layer["source"].startswith("script-regex") + for layer in resolver.collect_all_layers("check-main", "script") + ) + invalid = _manifest("regex:^check-.*$", "script", "append") + path = project_dir / "invalid-script-preset.yml" + path.write_text(yaml.safe_dump(invalid), encoding="utf-8") + with pytest.raises(PresetValidationError, match="scripts only support"): + PresetManifest(path) + + +def test_zero_match_is_non_fatal_and_has_no_layer(project_dir): + _write_preset(project_dir, "no-match", "regex:^missing-resource$", priority=10) + assert ( + PresetResolver(project_dir).collect_all_layers("missing-resource", "template") + == [] + ) From 61f04352d34e6f59647e10f546036d3ebdab0f8c Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Tue, 29 Sep 2026 02:42:10 +0800 Subject: [PATCH 2/6] feat(presets): support regex resource selectors Add regex: selectors for preset templates, scripts, and commands while preserving exact-name behavior. Resolve all matching declarations in manifest order, expand command selectors before registration, and reconcile matches after preset and extension changes. Add selector diagnostics and regression coverage. Closes #4659 --- src/specify_cli/extensions/_commands.py | 70 +++-- src/specify_cli/extensions/command_add.py | 106 +++++-- src/specify_cli/extensions/command_disable.py | 16 +- src/specify_cli/extensions/command_enable.py | 14 +- src/specify_cli/extensions/command_remove.py | 30 +- .../extensions/command_set_priority.py | 21 +- src/specify_cli/presets/_manager.py | 44 ++- src/specify_cli/presets/_manager_commands.py | 12 +- src/specify_cli/presets/_manager_skills.py | 224 ++++++++------- src/specify_cli/presets/_resolver.py | 166 +++++------ src/specify_cli/presets/command_info.py | 2 +- .../test_regex_selector_diagnostics.py | 184 ++++++++++++ .../presets/test_regex_selector_lifecycle.py | 78 ++++++ .../presets/test_regex_selectors.py | 261 +++++++++++++++++- 14 files changed, 963 insertions(+), 265 deletions(-) create mode 100644 tests/specify_cli/presets/test_regex_selector_diagnostics.py create mode 100644 tests/specify_cli/presets/test_regex_selector_lifecycle.py diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index ce6d229878..c8a93c739b 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -5,6 +5,7 @@ re-fetch package helpers at call time so existing monkeypatch paths keep working. Cohesive private phases use ``_command__*.py`` modules. """ + from __future__ import annotations import errno @@ -34,25 +35,30 @@ add_completion=False, ) + # Root helpers re-fetched at call time so test monkeypatching of # `specify_cli.` keeps working after the move. def _require_specify_project(*args, **kwargs): from .. import _require_specify_project as _f + return _f(*args, **kwargs) def _locate_bundled_extension(*args, **kwargs): from .. import _locate_bundled_extension as _f + return _f(*args, **kwargs) def load_init_options(*args, **kwargs): from .. import load_init_options as _f + return _f(*args, **kwargs) def _display_project_path(*args, **kwargs): from .. import _display_project_path as _f + return _f(*args, **kwargs) @@ -97,6 +103,34 @@ def _archive_extension_directory(*args, **kwargs): return _helper(*args, **kwargs) +def _refresh_presets_and_warn(project_root: Path) -> None: + """Re-register enabled preset overrides after extension stack changes. + + Preset regex selectors expand against currently available lower layers, so + installing, removing, enabling, disabling, or reprioritizing an extension + can change which concrete command declarations are materialized. Keep the + normal preset enablement and active-integration rules by using its existing + integration-switch registration path. + """ + try: + from .._init_options import load_init_options + from ..presets import PresetManager + + agent = load_init_options(project_root).get("ai") + if agent: + PresetManager(project_root).register_enabled_presets_for_agent(agent) + except Exception as exc: + from .. import _print_cli_warning + + _print_cli_warning( + "reconcile preset artifacts after extension change", + "project", + str(project_root), + exc, + continuing="Continuing; run 'specify integration use' to retry registration.", + ) + + def _refresh_events_and_warn(project_root: Path) -> None: """Refresh native event config and surface failures (R3). @@ -146,9 +180,7 @@ def install_extension_from_url( from . import ExtensionCatalog, ExtensionError if not is_https_or_localhost_http(url): - raise ExtensionError( - "URL must use HTTPS (HTTP is only allowed for localhost)" - ) + raise ExtensionError("URL must use HTTPS (HTTP is only allowed for localhost)") download_dir = _validate_safe_cache_dir(project_root) archive_filename = f"extension-url-download-{uuid4().hex}.archive" @@ -209,9 +241,7 @@ def install_extension_from_url( ) from exc format_source = ( - final_url - if archive_format_from_name(final_url) is not None - else url + final_url if archive_format_from_name(final_url) is not None else url ) try: detect_archive_format( @@ -282,7 +312,9 @@ def _resolve_installed_extension( return (ext["id"], ext["name"]) # If not found by ID, try display name match - name_matches = [ext for ext in installed_extensions if ext["name"].lower() == argument.lower()] + name_matches = [ + ext for ext in installed_extensions if ext["name"].lower() == argument.lower() + ] if len(name_matches) == 1: # Unique display-name match @@ -311,7 +343,9 @@ def _resolve_installed_extension( # No match by ID or display name if allow_not_found: return (None, None) - console.print(f"[red]Error:[/red] Extension '{_escape_markup(argument)}' is not installed") + console.print( + f"[red]Error:[/red] Extension '{_escape_markup(argument)}' is not installed" + ) raise typer.Exit(1) @@ -375,7 +409,9 @@ def _resolve_catalog_extension( ) console.print(table) console.print("\nPlease rerun using the extension ID:") - console.print(f" [bold]specify extension {command_name} [/bold]") + console.print( + f" [bold]specify extension {command_name} [/bold]" + ) raise typer.Exit(1) # Not found @@ -432,9 +468,7 @@ def _verify_leaf_identity(fd: int, path: Path) -> None: or path_stat.st_dev != open_stat.st_dev or path_stat.st_ino != open_stat.st_ino ): - raise OSError( - errno.ENOTDIR, "Download file changed between creation and open" - ) + raise OSError(errno.ENOTDIR, "Download file changed between creation and open") def _validate_safe_cache_dir(project_root: Path) -> Path: @@ -568,9 +602,7 @@ def _safe_open_download_zip( classes on every supported platform. """ if _has_secure_dir_fd(): - return _open_download_zip_via_dir_fd( - project_root, download_dir, zip_filename - ) + return _open_download_zip_via_dir_fd(project_root, download_dir, zip_filename) return _open_download_zip_via_paths(project_root, download_dir, zip_filename) @@ -623,15 +655,15 @@ def _open_download_zip_via_paths( project_root_resolved = project_root.resolve() if download_dir.is_symlink() or not download_dir.is_dir(): - raise OSError( - errno.ENOTDIR, "Download cache directory is not a real directory" - ) + raise OSError(errno.ENOTDIR, "Download cache directory is not a real directory") try: download_dir.resolve().relative_to(project_root_resolved) except (OSError, ValueError): raise OSError(errno.ENOTDIR, "Download cache directory escapes project root") if zip_path.is_symlink(): - raise OSError(errno.ELOOP, "Refusing to write through a symlinked download file") + raise OSError( + errno.ELOOP, "Refusing to write through a symlinked download file" + ) flags = os.O_RDWR | os.O_CREAT | os.O_EXCL flags |= getattr(os, "O_NOFOLLOW", 0) diff --git a/src/specify_cli/extensions/command_add.py b/src/specify_cli/extensions/command_add.py index 9a5bd7cad2..2453583153 100644 --- a/src/specify_cli/extensions/command_add.py +++ b/src/specify_cli/extensions/command_add.py @@ -3,6 +3,7 @@ Registered by ``_commands.register()``; shared command infrastructure lives in ``_commands.py``. """ + from __future__ import annotations from pathlib import Path @@ -20,17 +21,32 @@ def extension_add( extension: str = typer.Argument(help="Extension name or path"), dev: bool = typer.Option(False, "--dev", help="Install from local directory"), - from_url: Optional[str] = typer.Option(None, "--from", help="Install from custom URL"), + from_url: Optional[str] = typer.Option( + None, "--from", help="Install from custom URL" + ), force: bool = typer.Option(False, "--force", help="Overwrite if already installed"), - priority: int = typer.Option(10, "--priority", help="Resolution priority (lower = higher precedence, default 10)"), + priority: int = typer.Option( + 10, + "--priority", + help="Resolution priority (lower = higher precedence, default 10)", + ), ): """Install an extension.""" - from . import ExtensionManager, ExtensionCatalog, ExtensionError, ValidationError, CompatibilityError, REINSTALL_COMMAND + from . import ( + ExtensionManager, + ExtensionCatalog, + ExtensionError, + ValidationError, + CompatibilityError, + REINSTALL_COMMAND, + ) project_root = _commands._require_specify_project() # Validate priority if priority < 1: - console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + console.print( + "[red]Error:[/red] Priority must be a positive integer (1 or higher)" + ) raise typer.Exit(1) manager = ExtensionManager(project_root) @@ -74,15 +90,17 @@ def extension_add( # Warn about untrusted sources — default-deny confirmation console.print() - console.print(Panel( - f"[bold]You are installing an extension directly from an external URL,\n" - f"bypassing your trusted (install-allowed) extension catalogs.[/bold]\n\n" - f"URL: {safe_url}\n\n" - f"Only install extensions from sources you trust.", - title="[bold yellow]⚠ Untrusted Source[/bold yellow]", - border_style="yellow", - padding=(1, 2), - )) + console.print( + Panel( + f"[bold]You are installing an extension directly from an external URL,\n" + f"bypassing your trusted (install-allowed) extension catalogs.[/bold]\n\n" + f"URL: {safe_url}\n\n" + f"Only install extensions from sources you trust.", + title="[bold yellow]⚠ Untrusted Source[/bold yellow]", + border_style="yellow", + padding=(1, 2), + ) + ) console.print() confirm = typer.confirm("Continue with installation?", default=False) if not confirm: @@ -98,22 +116,28 @@ def extension_add( source_path = Path(extension).expanduser().resolve() safe_source_path = _escape_markup(str(source_path)) if not source_path.exists(): - console.print(f"[red]Error:[/red] Directory not found: {safe_source_path}") + console.print( + f"[red]Error:[/red] Directory not found: {safe_source_path}" + ) raise typer.Exit(1) if not (source_path / "extension.yml").exists(): - console.print(f"[red]Error:[/red] No extension.yml found in {safe_source_path}") + console.print( + f"[red]Error:[/red] No extension.yml found in {safe_source_path}" + ) raise typer.Exit(1) if force: - console.print(f"[yellow]--force:[/yellow] Installing from [cyan]{safe_source_path}[/cyan] (will overwrite if already installed)...") + console.print( + f"[yellow]--force:[/yellow] Installing from [cyan]{safe_source_path}[/cyan] (will overwrite if already installed)..." + ) manifest = manager.install_from_directory( source_path, speckit_version, priority=priority, link_commands=True, - force=force + force=force, ) elif from_url: @@ -147,21 +171,28 @@ def extension_add( extension, catalog, "add" ) if catalog_error: - console.print(f"[red]Error:[/red] Could not query extension catalog: {_escape_markup(str(catalog_error))}") + console.print( + f"[red]Error:[/red] Could not query extension catalog: {_escape_markup(str(catalog_error))}" + ) raise typer.Exit(1) if not ext_info: - console.print(f"[red]Error:[/red] Extension '{safe_extension}' not found in catalog") + console.print( + f"[red]Error:[/red] Extension '{safe_extension}' not found in catalog" + ) console.print("\nSearch available extensions:") console.print(" specify extension search") raise typer.Exit(1) # If catalog resolved a display name to an ID, check bundled again - resolved_id = ext_info['id'] + resolved_id = ext_info["id"] if resolved_id != extension: bundled_path = _commands._locate_bundled_extension(resolved_id) if bundled_path is not None: manifest = manager.install_from_directory( - bundled_path, speckit_version, priority=priority, force=force + bundled_path, + speckit_version, + priority=priority, + force=force, ) if bundled_path is None: @@ -180,7 +211,9 @@ def extension_add( # Enforce install_allowed policy if not ext_info.get("_install_allowed", True): - catalog_name = _escape_markup(str(ext_info.get("_catalog_name", "community"))) + catalog_name = _escape_markup( + str(ext_info.get("_catalog_name", "community")) + ) resolved_id = _commands._command_safe_id(ext_info["id"]) console.print( f"[red]Error:[/red] '{safe_extension}' was found in the " @@ -204,8 +237,10 @@ def extension_add( raise typer.Exit(1) # Download extension archive (use the resolved catalog ID). - extension_id = ext_info['id'] - console.print(f"Downloading {_escape_markup(str(ext_info['name']))} v{_escape_markup(str(ext_info.get('version', 'unknown')))}...") + extension_id = ext_info["id"] + console.print( + f"Downloading {_escape_markup(str(ext_info['name']))} v{_escape_markup(str(ext_info.get('version', 'unknown')))}..." + ) archive_path = catalog.download_extension(extension_id) try: @@ -220,15 +255,20 @@ def extension_add( archive_path.unlink(missing_ok=True) console.print("\n[green]✓[/green] Extension installed successfully!") - console.print(f"\n[bold]{_escape_markup(str(manifest.name))}[/bold] (v{_escape_markup(str(manifest.version))})") + console.print( + f"\n[bold]{_escape_markup(str(manifest.name))}[/bold] (v{_escape_markup(str(manifest.version))})" + ) console.print(f" {_escape_markup(str(manifest.description))}") # #1: regenerate native event config for installed event-capable # integrations so the new extension's events take effect immediately. _commands._refresh_events_and_warn(project_root) + _commands._refresh_presets_and_warn(project_root) for warning in manifest.warnings: - console.print(f"\n[yellow]⚠ Compatibility warning:[/yellow] {_escape_markup(str(warning))}") + console.print( + f"\n[yellow]⚠ Compatibility warning:[/yellow] {_escape_markup(str(warning))}" + ) selected_ai = _commands.load_init_options(project_root).get("ai") is_cline = selected_ai == "cline" @@ -241,12 +281,14 @@ def extension_add( console.print("\n[bold cyan]Provided commands:[/bold cyan]") for cmd in manifest.commands: - cmd_name = cmd['name'] + cmd_name = cmd["name"] if is_cline: cmd_name = format_cline_command_name(cmd_name) elif is_forge: cmd_name = format_forge_command_name(cmd_name) - console.print(f" • {_escape_markup(str(cmd_name))} - {_escape_markup(str(cmd.get('description', '')))}") + console.print( + f" • {_escape_markup(str(cmd_name))} - {_escape_markup(str(cmd.get('description', '')))}" + ) # Report agent skills registration reg_meta = manager.registry.get(manifest.id) @@ -255,7 +297,9 @@ def extension_add( if not isinstance(reg_skills, list): reg_skills = [] if reg_skills: - console.print(f"\n[green]✓[/green] {len(reg_skills)} agent skill(s) auto-registered") + console.print( + f"\n[green]✓[/green] {len(reg_skills)} agent skill(s) auto-registered" + ) # Scaffold config templates automatically deployed, skipped, failed = manager.scaffold_config(manifest.id) @@ -265,7 +309,9 @@ def extension_add( for cfg in deployed: console.print(f" • {config_home}/{_escape_markup(str(cfg))}") if skipped: - console.print(f"\n[dim]Config files already exist (preserved): {_escape_markup(', '.join(skipped))}[/dim]") + console.print( + f"\n[dim]Config files already exist (preserved): {_escape_markup(', '.join(skipped))}[/dim]" + ) if failed: console.print( f"\n[yellow]Warning:[/yellow] Config templates not scaffolded: " diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index b9ee83134e..910467c7cb 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -3,6 +3,7 @@ Registered by ``_commands.register()``; shared command infrastructure lives in ``_commands.py``. """ + from __future__ import annotations import typer @@ -39,7 +40,9 @@ def extension_disable( raise typer.Exit(1) if not metadata.get("enabled", True): - console.print(f"[yellow]Extension '{_escape_markup(str(display_name))}' is already disabled[/yellow]") + console.print( + f"[yellow]Extension '{_escape_markup(str(display_name))}' is already disabled[/yellow]" + ) raise typer.Exit(0) manager.registry.update(extension_id, {"enabled": False}) @@ -53,10 +56,17 @@ def extension_disable( hook["enabled"] = False hook_executor.save_project_config(config) - console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' disabled") + console.print( + f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' disabled" + ) console.print("\nCommands will no longer be available. Hooks will not execute.") - console.print(f"To re-enable: specify extension enable {_escape_markup(str(extension_id))}") + console.print( + f"To re-enable: specify extension enable {_escape_markup(str(extension_id))}" + ) # #1: regenerate native event config so the disabled extension's events # are stripped from installed integrations. + # Extension mutations may change the expansion set for preset regex + # selectors; re-register enabled presets after refreshing native events. _commands._refresh_events_and_warn(project_root) + _commands._refresh_presets_and_warn(project_root) diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index c338245905..326b4b1d58 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -3,6 +3,7 @@ Registered by ``_commands.register()``; shared command infrastructure lives in ``_commands.py``. """ + from __future__ import annotations import typer @@ -39,7 +40,9 @@ def extension_enable( raise typer.Exit(1) if metadata.get("enabled", True): - console.print(f"[yellow]Extension '{_escape_markup(str(display_name))}' is already enabled[/yellow]") + console.print( + f"[yellow]Extension '{_escape_markup(str(display_name))}' is already enabled[/yellow]" + ) raise typer.Exit(0) manager.registry.update(extension_id, {"enabled": True}) @@ -53,11 +56,14 @@ def extension_enable( hook["enabled"] = True hook_executor.save_project_config(config) - console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' enabled") + console.print( + f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' enabled" + ) # #1: regenerate native event config so the enabled extension's events # are re-emitted in installed integrations. _commands._refresh_events_and_warn(project_root) + _commands._refresh_presets_and_warn(project_root) # Scaffold config templates on enable try: @@ -75,7 +81,9 @@ def extension_enable( for cfg in deployed: console.print(f" • {config_home}/{_escape_markup(str(cfg))}") if skipped: - console.print(f"\n[dim]Config files already exist (preserved): {_escape_markup(', '.join(skipped))}[/dim]") + console.print( + f"\n[dim]Config files already exist (preserved): {_escape_markup(', '.join(skipped))}[/dim]" + ) if failed: console.print( f"\n[yellow]Warning:[/yellow] Config templates not scaffolded: " diff --git a/src/specify_cli/extensions/command_remove.py b/src/specify_cli/extensions/command_remove.py index c210dae96b..429a5a1ad9 100644 --- a/src/specify_cli/extensions/command_remove.py +++ b/src/specify_cli/extensions/command_remove.py @@ -3,6 +3,7 @@ Registered by ``_commands.register()``; shared command infrastructure lives in ``_commands.py``. """ + from __future__ import annotations import typer @@ -15,7 +16,9 @@ @_commands.extension_app.command("remove") def extension_remove( extension: str = typer.Argument(help="Extension ID or name to remove"), - keep_config: bool = typer.Option(False, "--keep-config", help="Don't remove config files"), + keep_config: bool = typer.Option( + False, "--keep-config", help="Don't remove config files" + ), force: bool = typer.Option(False, "--force", help="Skip confirmation"), ): """Uninstall an extension.""" @@ -40,7 +43,9 @@ def extension_remove( # think in logical commands, not per-agent file counts. # Use get() without a default so we can distinguish "key missing" (fall back # to manifest) from "key present but empty dict" (zero commands registered). - registered_commands = reg_meta.get("registered_commands") if isinstance(reg_meta, dict) else None + registered_commands = ( + reg_meta.get("registered_commands") if isinstance(reg_meta, dict) else None + ) if isinstance(registered_commands, dict): cmd_count = max( (len(v) for v in registered_commands.values() if isinstance(v, list)), @@ -54,10 +59,14 @@ def extension_remove( # Confirm removal if not force: console.print("\n[yellow]⚠ This will remove:[/yellow]") - console.print(f" • {cmd_count} command{'s' if cmd_count != 1 else ''} per agent") + console.print( + f" • {cmd_count} command{'s' if cmd_count != 1 else ''} per agent" + ) if skill_count: console.print(f" • {skill_count} agent skill(s)") - console.print(f" • Extension directory: .specify/extensions/{safe_extension_id}/") + console.print( + f" • Extension directory: .specify/extensions/{safe_extension_id}/" + ) if not keep_config: console.print(" • Config files (will be backed up)") console.print() @@ -71,15 +80,22 @@ def extension_remove( success = manager.remove(extension_id, keep_config=keep_config) if success: - console.print(f"\n[green]✓[/green] Extension '{_escape_markup(str(display_name))}' removed successfully") + console.print( + f"\n[green]✓[/green] Extension '{_escape_markup(str(display_name))}' removed successfully" + ) if keep_config: - console.print(f"\nConfig files preserved in .specify/extensions/{safe_extension_id}/") + console.print( + f"\nConfig files preserved in .specify/extensions/{safe_extension_id}/" + ) else: - console.print(f"\nConfig files backed up to .specify/extensions/.backup/{safe_extension_id}/") + console.print( + f"\nConfig files backed up to .specify/extensions/.backup/{safe_extension_id}/" + ) # #1: regenerate native event config so the removed extension's events # are stripped from installed integrations. _commands._refresh_events_and_warn(project_root) + _commands._refresh_presets_and_warn(project_root) console.print(f"\nTo reinstall: specify extension add {safe_extension_id}") else: console.print("[red]Error:[/red] Failed to remove extension") diff --git a/src/specify_cli/extensions/command_set_priority.py b/src/specify_cli/extensions/command_set_priority.py index 3adbe73c9b..d6415742f9 100644 --- a/src/specify_cli/extensions/command_set_priority.py +++ b/src/specify_cli/extensions/command_set_priority.py @@ -3,6 +3,7 @@ Registered by ``_commands.register()``; shared command infrastructure lives in ``_commands.py``. """ + from __future__ import annotations import typer @@ -23,7 +24,9 @@ def extension_set_priority( project_root = _commands._require_specify_project() # Validate priority if priority < 1: - console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + console.print( + "[red]Error:[/red] Priority must be a positive integer (1 or higher)" + ) raise typer.Exit(1) manager = ExtensionManager(project_root) @@ -54,7 +57,9 @@ def extension_set_priority( and not isinstance(raw_priority, bool) and raw_priority == priority ): - console.print(f"[yellow]Extension '{_escape_markup(str(display_name))}' already has priority {priority}[/yellow]") + console.print( + f"[yellow]Extension '{_escape_markup(str(display_name))}' already has priority {priority}[/yellow]" + ) raise typer.Exit(0) old_priority = normalize_priority(raw_priority) @@ -62,5 +67,13 @@ def extension_set_priority( # Update priority manager.registry.update(extension_id, {"priority": priority}) - console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' priority changed: {old_priority} → {priority}") - console.print("\n[dim]Lower priority = higher precedence in template resolution[/dim]") + # Extension reordering can change the lower-layer candidates matched by + # enabled preset regex selectors. + _commands._refresh_presets_and_warn(project_root) + + console.print( + f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' priority changed: {old_priority} → {priority}" + ) + console.print( + "\n[dim]Lower priority = higher precedence in template resolution[/dim]" + ) diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index be643a8a3a..1ab1f50b25 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -36,6 +36,25 @@ _CONSTITUTION_SYNC_PRESET_ID = "constitution-sync" +class _ExpandedCommandManifest: + """Manifest view exposing concrete command selector expansions.""" + + def __init__(self, manifest: PresetManifest, commands: List[Dict[str, Any]]): + self._manifest = manifest + self._commands = commands + + def __getattr__(self, name: str): + return getattr(self._manifest, name) + + @property + def templates(self) -> List[Dict[str, Any]]: + return [ + template + for template in self._manifest.templates + if template.get("type") != "command" + ] + self._commands + + def _content_sha256(content: bytes) -> str: return hashlib.sha256(content).hexdigest() @@ -423,23 +442,32 @@ def install_from_directory( }, ) + raw_command_templates = [ + template + for template in manifest.templates + if template.get("type") == "command" + ] + command_templates = self._expand_command_selectors( + PresetResolver(self.project_root), dest_dir, raw_command_templates + ) registered_commands: Dict[str, List[str]] = {} registered_skills: Dict[str, List[str]] = {} try: - # Register command overrides with AI agents and persist the result - # immediately so cleanup can recover even if installation stops - # before later phases complete. - registered_commands = self._register_commands(manifest, dest_dir) + # Register both command artifacts and skills from the same expanded + # concrete command declaration set. + registered_commands = self._register_commands( + manifest, dest_dir, command_templates=command_templates + ) self.registry.update( manifest.id, - { - "registered_commands": registered_commands, - }, + {"registered_commands": registered_commands}, ) # Update corresponding skills when skills mode was previously used # and persist that result as well. - registered_skills = self._register_skills(manifest, dest_dir) + registered_skills = self._register_skills( + manifest, dest_dir, command_templates=command_templates + ) self.registry.update( manifest.id, { diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index e9d44ddbf2..e7f264abed 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -197,6 +197,8 @@ def _register_commands( self, manifest: PresetManifest, preset_dir: Path, + *, + command_templates: Optional[List[Dict[str, Any]]] = None, ) -> Dict[str, List[str]]: """Register preset command overrides with all detected AI agents. @@ -219,10 +221,12 @@ def _register_commands( t for t in manifest.templates if t.get("type") == "command" ] resolver = PresetResolver(self.project_root) - command_templates = self._expand_command_selectors( - resolver, - preset_dir, - raw_command_templates, + command_templates = ( + command_templates + if command_templates is not None + else self._expand_command_selectors( + resolver, preset_dir, raw_command_templates + ) ) if not command_templates: return {} diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index fa7e715a31..601ca08b64 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -17,6 +17,7 @@ from ._manager_commands import _substitute_core_template from ._manifest import PresetManifest, PresetValidationError from ._resolver import PresetResolver +from ._selectors import is_regex_selector class _PresetSkillMethods: @@ -39,8 +40,7 @@ def __getattr__(self, name: str): @property def templates(self) -> List[Dict[str, Any]]: return [ - t for t in self._manifest.templates - if t.get("name") in self._cmd_names + t for t in self._manifest.templates if t.get("name") in self._cmd_names ] def _merge_pack_registered_skills( @@ -82,9 +82,7 @@ def _merge_pack_registered_skills( else: existing_skills = self._normalize_registered_skills(raw_existing_skills) merged_skills = copy.deepcopy(existing_skills) - changed = ( - isinstance(raw_existing_skills, list) and bool(raw_existing_skills) - ) + changed = isinstance(raw_existing_skills, list) and bool(raw_existing_skills) for agent_name, skill_names in written.items(): if not skill_names: continue @@ -99,9 +97,7 @@ def _merge_pack_registered_skills( def _reconcile_skills( self, command_names: List[str], - extra_skills_dirs: Optional[ - Dict[Path, tuple[Optional[str], List[str]]] - ] = None, + extra_skills_dirs: Optional[Dict[Path, tuple[Optional[str], List[str]]]] = None, target_agent: Optional[str] = None, ) -> Set[str]: """Re-register skills for commands whose winning layer changed. @@ -158,9 +154,7 @@ def _reconcile_skills( if not layers: continue - skill_name, legacy_skill_name = self._skill_names_for_command( - cmd_name - ) + skill_name, legacy_skill_name = self._skill_names_for_command(cmd_name) candidate_skill_names = {skill_name, legacy_skill_name} # Track whether any preset previously registered this skill # (i.e., it was actively managed), so a not-yet-existing skill @@ -180,9 +174,7 @@ def _reconcile_skills( recorded_names = set(recorded) else: recorded_names = set() - recorded_candidates = ( - candidate_skill_names & recorded_names - ) + recorded_candidates = candidate_skill_names & recorded_names if recorded_candidates: managed_skill_names.update(recorded_candidates) @@ -200,8 +192,12 @@ def _reconcile_skills( # Track the winning layer path for skill restoration. non_preset_skills.append((skill_name, cmd_name, layers[0])) - core_ext_skills = [s for s in non_preset_skills if s[2]["source"] != "project override"] - override_skills = [s for s in non_preset_skills if s[2]["source"] == "project override"] + core_ext_skills = [ + s for s in non_preset_skills if s[2]["source"] != "project override" + ] + override_skills = [ + s for s in non_preset_skills if s[2]["source"] == "project override" + ] def apply_to_dir( skills_dir: Path, @@ -243,12 +239,13 @@ def apply_to_dir( from .. import SKILL_DESCRIPTIONS from ..agents import CommandRegistrar from ..shared_infra import _write_shared_text + registrar = CommandRegistrar() content = top_layer["path"].read_text(encoding="utf-8") fm, body = registrar.parse_frontmatter(content) short_name = cmd_name if short_name.startswith("speckit."): - short_name = short_name[len("speckit."):] + short_name = short_name[len("speckit.") :] desc = fm.get("description", "") or SKILL_DESCRIPTIONS.get( short_name.replace(".", "-"), f"Command: {short_name}", @@ -262,6 +259,7 @@ def apply_to_dir( body, registrar, selected_ai, self.project_root ) from ..integrations import get_integration + integration = get_integration(selected_ai) if selected_ai else None skill_title = self._skill_title_from_command(cmd_name) wrote_override = False @@ -281,9 +279,7 @@ def apply_to_dir( desc, f"override:{cmd_name}", ) - registrar.apply_argument_hint( - fm, fm_data, integration - ) + registrar.apply_argument_hint(fm, fm_data, integration) fm_text = dump_frontmatter(fm_data) skill_content = ( f"---\n{fm_text}\n---\n\n" @@ -292,10 +288,8 @@ def apply_to_dir( if integration is not None and hasattr( integration, "post_process_skill_content" ): - skill_content = ( - integration.post_process_skill_content( - skill_content - ) + skill_content = integration.post_process_skill_content( + skill_content ) _write_shared_text( skills_dir, @@ -303,12 +297,8 @@ def apply_to_dir( skill_content, ) wrote_override = True - if ( - wrote_override - and ( - target_agent is None - or dir_agent == target_agent - ) + if wrote_override and ( + target_agent is None or dir_agent == target_agent ): reconciled_skill_commands.add(cmd_name) except Exception: @@ -364,14 +354,14 @@ def apply_to_dir( written = self._register_skills(filtered_manifest, pack_dir) else: written = self._register_skills( - filtered_manifest, pack_dir, - target_dir=skills_dir, target_agent=dir_agent or "", + filtered_manifest, + pack_dir, + target_dir=skills_dir, + target_agent=dir_agent or "", ) if target_agent is None: written_names = { - name - for names in written.values() - for name in names + name for names in written.values() for name in names } else: written_names = set(written.get(target_agent, [])) @@ -399,9 +389,7 @@ def apply_to_dir( active_ai, is_active=True, managed_names=( - set(active_provenance[1]) - if active_provenance - else None + set(active_provenance[1]) if active_provenance else None ), ) @@ -455,11 +443,15 @@ def _get_skills_dir(self) -> Optional[Path]: resolve_active_skills_dir, ) from ..shared_infra import _ensure_safe_shared_directory + try: skills_dir = resolve_active_skills_dir(self.project_root) except (ValueError, OSError) as exc: _print_cli_warning( - "resolve", "skills directory", None, exc, + "resolve", + "skills directory", + None, + exc, continuing="Continuing without skill registration.", ) return None @@ -493,7 +485,10 @@ def _get_skills_dir(self) -> Optional[Path]: ) except (ValueError, OSError) as exc: _print_cli_warning( - "resolve", "skills directory", str(agent_skills_dir), exc, + "resolve", + "skills directory", + str(agent_skills_dir), + exc, continuing="Continuing without skill registration.", ) return None @@ -504,7 +499,7 @@ def _skill_names_for_command(cmd_name: str) -> tuple[str, str]: """Return the modern and legacy skill directory names for a command.""" raw_short_name = cmd_name if raw_short_name.startswith("speckit."): - raw_short_name = raw_short_name[len("speckit."):] + raw_short_name = raw_short_name[len("speckit.") :] modern_skill_name = f"speckit-{raw_short_name.replace('.', '-')}" legacy_skill_name = f"speckit.{raw_short_name}" @@ -515,7 +510,7 @@ def _skill_title_from_command(cmd_name: str) -> str: """Return a human-friendly title for a skill command name.""" title_name = cmd_name if title_name.startswith("speckit."): - title_name = title_name[len("speckit."):] + title_name = title_name[len("speckit.") :] return title_name.replace(".", " ").replace("-", " ").title() @staticmethod @@ -608,7 +603,9 @@ def _build_extension_skill_restore_index(self) -> Dict[str, Dict[str, Any]]: "extension_id": manifest.id, "extension_dir": ext_root, } - modern_skill_name, legacy_skill_name = self._skill_names_for_command(cmd_name) + modern_skill_name, legacy_skill_name = self._skill_names_for_command( + cmd_name + ) restore_index.setdefault(modern_skill_name, restore_info) if legacy_skill_name != modern_skill_name: restore_index.setdefault(legacy_skill_name, restore_info) @@ -622,6 +619,7 @@ def _register_skills( *, target_dir: Optional[Path] = None, target_agent: Optional[str] = None, + command_templates: Optional[List[Dict[str, Any]]] = None, ) -> Dict[str, List[str]]: """Generate SKILL.md files for preset command overrides. @@ -654,9 +652,21 @@ def _register_skills( matching the shape ``registered_commands`` already uses so the two can be tracked/restored consistently (#2948). """ - command_templates = [ + command_declarations = [ t for t in manifest.templates if t.get("type") == "command" ] + command_templates = ( + command_templates + if command_templates is not None + else self._expand_command_selectors( + PresetResolver(self.project_root), preset_dir, command_declarations + ) + ) + command_templates = [ + t + for t in command_templates + if isinstance(t.get("name"), str) and not is_regex_selector(t["name"]) + ] if not command_templates: return {} @@ -696,7 +706,9 @@ def _register_skills( # preset skills in _register_commands() because their detected agent # directory is already the skills directory. This flag is only for # command-backed agents that also mirror commands into skills. - create_missing_skills = ai_skills_enabled and agent_config.get("extension") != "/SKILL.md" + create_missing_skills = ( + ai_skills_enabled and agent_config.get("extension") != "/SKILL.md" + ) written: List[str] = [] @@ -716,7 +728,7 @@ def _register_skills( # Derive the short command name (e.g. "specify" from "speckit.specify") raw_short_name = cmd_name if raw_short_name.startswith("speckit."): - raw_short_name = raw_short_name[len("speckit."):] + raw_short_name = raw_short_name[len("speckit.") :] short_name = raw_short_name.replace(".", "-") skill_name, legacy_skill_name = self._skill_names_for_command(cmd_name) skill_title = self._skill_title_from_command(cmd_name) @@ -727,7 +739,10 @@ def _register_skills( target_skill_names: List[str] = [] if (skills_dir / skill_name).is_dir(): target_skill_names.append(skill_name) - if legacy_skill_name != skill_name and (skills_dir / legacy_skill_name).is_dir(): + if ( + legacy_skill_name != skill_name + and (skills_dir / legacy_skill_name).is_dir() + ): target_skill_names.append(legacy_skill_name) if not target_skill_names and create_missing_skills: missing_skill_dir = skills_dir / skill_name @@ -752,9 +767,7 @@ def _register_skills( # _register_commands already warned for this command in the same # pass, so the skip is silent here to avoid a duplicate warning. effective_strategy = ( - cmd_tmpl.get("strategy") - or frontmatter.get("strategy") - or "replace" + cmd_tmpl.get("strategy") or frontmatter.get("strategy") or "replace" ) if ( effective_strategy != "replace" @@ -764,7 +777,9 @@ def _register_skills( continue if frontmatter.get("strategy") == "wrap": - body, core_frontmatter = _substitute_core_template(body, cmd_name, self.project_root, registrar) + body, core_frontmatter = _substitute_core_template( + body, cmd_name, self.project_root, registrar + ) frontmatter = dict(frontmatter) for key in ("scripts", "agent_scripts", "argument-hint"): if key not in frontmatter and key in core_frontmatter: @@ -780,7 +795,9 @@ def _register_skills( body = registrar.resolve_skill_placeholders( selected_ai, frontmatter, body, self.project_root ) - body = self._resolve_skill_command_refs(body, registrar, selected_ai, self.project_root) + body = self._resolve_skill_command_refs( + body, registrar, selected_ai, self.project_root + ) for target_skill_name in target_skill_names: skill_subdir = skills_dir / target_skill_name @@ -801,7 +818,9 @@ def _register_skills( enhanced_desc, f"preset:{manifest.id}", ) - registrar.apply_argument_hint(frontmatter, frontmatter_data, integration) + registrar.apply_argument_hint( + frontmatter, frontmatter_data, integration + ) frontmatter_text = dump_frontmatter(frontmatter_data) skill_content = ( f"---\n" @@ -810,15 +829,15 @@ def _register_skills( f"# Speckit {skill_title} Skill\n\n" f"{body}\n" ) - if integration is not None and hasattr(integration, "post_process_skill_content"): + if integration is not None and hasattr( + integration, "post_process_skill_content" + ): skill_content = integration.post_process_skill_content( skill_content ) skill_file = skill_subdir / "SKILL.md" - _write_shared_text( - skills_dir, skill_file, skill_content - ) + _write_shared_text(skills_dir, skill_file, skill_content) written.append(target_skill_name) self._merge_pack_registered_skills( manifest.id, {selected_ai: [target_skill_name]} @@ -910,7 +929,9 @@ def _infer_legacy_skill_provenance( inferred: Dict[str, List[str]] = {} matched_names: set = set() for resolved_dir, agents in dir_to_agents.items(): - canonical_agent = fallback_agent if fallback_agent in agents else sorted(agents)[0] + canonical_agent = ( + fallback_agent if fallback_agent in agents else sorted(agents)[0] + ) for name in safe_skill_names: skill_subdir = resolved_dir / name if not self._validate_skill_subdir( @@ -998,8 +1019,10 @@ def _safe_skills_dir_for_agent(self, agent_name: str) -> Optional[Path]: return None try: _ensure_safe_shared_directory( - validation_root, skills_dir, - create=False, context="preset skills directory", + validation_root, + skills_dir, + create=False, + context="preset skills directory", ) except (ValueError, OSError): return None @@ -1069,13 +1092,13 @@ def _validate_skill_subdir( try: if create: _ensure_safe_shared_directory( - validation_root, skill_subdir, - create=True, context="preset skill directory", + validation_root, + skill_subdir, + create=True, + context="preset skill directory", ) else: - _validate_safe_shared_directory( - validation_root, skill_subdir - ) + _validate_safe_shared_directory(validation_root, skill_subdir) except (ValueError, OSError): return False return True @@ -1213,11 +1236,7 @@ def _unregister_skills( additional_owned_sources=additional_owned_sources, restore_from_bundled_core=restore_from_bundled_core, ) - return ( - {skills_dir: (selected_ai, mutated_names)} - if mutated_names - else {} - ) + return {skills_dir: (selected_ai, mutated_names)} if mutated_names else {} def _delete_agent_preset_skills( self, agent_name: str, skill_names: List[str], pack_id: str @@ -1238,16 +1257,9 @@ def _delete_agent_preset_skills( if manifest is not None: for template in manifest.templates: command_name = template.get("name") - if ( - template.get("type") == "command" - and isinstance(command_name, str) - ): - for skill_name in self._skill_names_for_command( - command_name - ): - override_sources[skill_name] = ( - f"override:{command_name}" - ) + if template.get("type") == "command" and isinstance(command_name, str): + for skill_name in self._skill_names_for_command(command_name): + override_sources[skill_name] = f"override:{command_name}" for skill_name in skill_names: if not self._is_safe_registry_skill_name(skill_name): continue @@ -1265,11 +1277,7 @@ def _delete_agent_preset_skills( continue frontmatter, _ = registrar.parse_frontmatter(content) metadata = frontmatter.get("metadata") - source = ( - metadata.get("source") - if isinstance(metadata, dict) - else None - ) + source = metadata.get("source") if isinstance(metadata, dict) else None owned_sources = {marker} override_source = override_sources.get(skill_name) if override_source: @@ -1333,7 +1341,9 @@ def _unregister_skills_in_dir( # Locate core command templates from the project's installed templates core_templates_dir = self.project_root / ".specify" / "templates" / "commands" registrar = CommandRegistrar() - integration = get_integration(selected_ai) if isinstance(selected_ai, str) else None + integration = ( + get_integration(selected_ai) if isinstance(selected_ai, str) else None + ) extension_restore_index = self._build_extension_skill_restore_index() mutated_names: List[str] = [] @@ -1355,9 +1365,9 @@ def _unregister_skills_in_dir( # Derive command name from skill name (speckit-specify -> specify) short_name = skill_name if short_name.startswith("speckit-"): - short_name = short_name[len("speckit-"):] + short_name = short_name[len("speckit-") :] elif short_name.startswith("speckit."): - short_name = short_name[len("speckit."):] + short_name = short_name[len("speckit.") :] skill_subdir = skills_dir / skill_name skill_file = skill_subdir / "SKILL.md" @@ -1388,9 +1398,7 @@ def _unregister_skills_in_dir( ) owned_sources = {f"preset:{pack_id}"} if additional_owned_sources: - additional_source = additional_owned_sources.get( - skill_name - ) + additional_source = additional_owned_sources.get(skill_name) if additional_source: owned_sources.add(additional_source) if current_source not in owned_sources: @@ -1422,7 +1430,9 @@ def _unregister_skills_in_dir( if _core_pack is not None: core_file = _core_pack / "commands" / f"{short_name}.md" else: - core_file = _repo_root() / "templates" / "commands" / f"{short_name}.md" + core_file = ( + _repo_root() / "templates" / "commands" / f"{short_name}.md" + ) if not core_file.exists(): core_file = None @@ -1462,7 +1472,9 @@ def _unregister_skills_in_dir( enhanced_desc, f"templates/commands/{short_name}.md", ) - registrar.apply_argument_hint(frontmatter, frontmatter_data, integration) + registrar.apply_argument_hint( + frontmatter, frontmatter_data, integration + ) frontmatter_text = dump_frontmatter(frontmatter_data) skill_title = self._skill_title_from_command(short_name) skill_content = ( @@ -1472,7 +1484,9 @@ def _unregister_skills_in_dir( f"# Speckit {skill_title} Skill\n\n" f"{body}\n" ) - if integration is not None and hasattr(integration, "post_process_skill_content"): + if integration is not None and hasattr( + integration, "post_process_skill_content" + ): skill_content = integration.post_process_skill_content( skill_content ) @@ -1485,7 +1499,9 @@ def _unregister_skills_in_dir( # unreadable extension source leaves the skill in place # instead of crashing or being deleted. try: - content = extension_restore["source_file"].read_text(encoding="utf-8") + content = extension_restore["source_file"].read_text( + encoding="utf-8" + ) except (OSError, UnicodeDecodeError) as exc: self._warn_unrestored_skill( skill_name, extension_restore["source_file"], exc @@ -1517,20 +1533,22 @@ def _unregister_skills_in_dir( frontmatter_data = registrar.build_skill_frontmatter( selected_ai if isinstance(selected_ai, str) else "", skill_name, - frontmatter.get("description", f"Extension command: {command_name}"), + frontmatter.get( + "description", f"Extension command: {command_name}" + ), extension_restore["source"], author=extension_restore.get("author", "github-spec-kit"), ) - registrar.apply_argument_hint(frontmatter, frontmatter_data, integration) + registrar.apply_argument_hint( + frontmatter, frontmatter_data, integration + ) frontmatter_text = dump_frontmatter(frontmatter_data) skill_content = ( - f"---\n" - f"{frontmatter_text}\n" - f"---\n\n" - f"# {title_name} Skill\n\n" - f"{body}\n" + f"---\n{frontmatter_text}\n---\n\n# {title_name} Skill\n\n{body}\n" ) - if integration is not None and hasattr(integration, "post_process_skill_content"): + if integration is not None and hasattr( + integration, "post_process_skill_content" + ): skill_content = integration.post_process_skill_content( skill_content ) diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 90dd57f74e..7c03cf035a 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -519,63 +519,74 @@ def resolve_with_source( return {"path": resolved_str, "source": "core"} - def _regex_preset_declaration( + def _preset_declarations_for_resource( self, pack_dir: Path, template_name: str, template_type: str - ) -> tuple[dict | None, Path | None]: - """Find a matching regex declaration only when a concrete lower layer exists.""" - if template_type not in {"template", "script"}: - return None, None + ) -> list[tuple[dict, Path | None]]: + """Collect exact and matching regex declarations in manifest order.""" + if template_type not in {"template", "script", "command"}: + return [] manifest = self._get_manifest(pack_dir) if manifest is None: - return None, None + return [] presets = self._get_all_presets_by_priority() pack_ids = [pack_id for pack_id, _ in presets] try: current_index = pack_ids.index(pack_dir.name) except ValueError: - return None, None - - # A selector only sees resources below its owning preset in the actual - # resolver stack, followed by enabled extensions and core. - for lower_id, _metadata in presets[current_index + 1 :]: - lower_dir = self.presets_dir / lower_id - if self._preset_has_concrete_resource( - lower_dir, template_name, template_type - ): - has_lower_resource = True - break - else: - has_lower_resource = False - + return [] + has_lower_resource = any( + self._preset_has_concrete_resource( + self.presets_dir / lower_id, template_name, template_type + ) + for lower_id, _metadata in presets[current_index + 1 :] + ) if not has_lower_resource: - for _priority, ext_id, _meta in self._get_all_extensions_by_priority(): - if self._extension_has_concrete_resource( + has_lower_resource = any( + self._extension_has_concrete_resource( self.extensions_dir / ext_id, template_name, template_type - ): - has_lower_resource = True - break - + ) + for _priority, ext_id, _meta in self._get_all_extensions_by_priority() + ) if not has_lower_resource: has_lower_resource = self._core_has_concrete_resource( template_name, template_type ) - if not has_lower_resource: - return None, None - + matches = [] for declaration in manifest.templates: - declared_name = declaration.get("name") - if ( - declaration.get("type") == template_type - and isinstance(declared_name, str) - and is_regex_selector(declared_name) - and selector_matches(declared_name, template_name) - ): + name = declaration.get("name") + if declaration.get("type") != template_type or not isinstance(name, str): + continue + exact = not is_regex_selector(name) and name == template_name + regex = ( + has_lower_resource + and is_regex_selector(name) + and selector_matches(name, template_name) + ) + if exact or regex: file_value = declaration.get("file") path = pack_dir / file_value if isinstance(file_value, str) else None - return declaration, ( - path if path is not None and path.is_file() else None + matches.append( + (declaration, path if path is not None and path.is_file() else None) ) - return None, None + return matches + + def _regex_preset_declarations( + self, pack_dir: Path, template_name: str, template_type: str + ) -> list[tuple[dict, Path | None]]: + matches = self._preset_declarations_for_resource( + pack_dir, template_name, template_type + ) + return [ + item for item in matches if is_regex_selector(str(item[0].get("name", ""))) + ] + + def _regex_preset_declaration( + self, pack_dir: Path, template_name: str, template_type: str + ) -> tuple[dict | None, Path | None]: + matches = self._regex_preset_declarations( + pack_dir, template_name, template_type + ) + return matches[0] if matches else (None, None) def _has_concrete_resource( self, @@ -712,34 +723,24 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: if self.presets_dir.exists(): for pack_id, metadata in self._get_all_presets_by_priority(): pack_dir = self.presets_dir / pack_id - # Read strategy and manifest file path from preset manifest - strategy = "replace" - manifest_has_strategy = False - entry, manifest_candidate = self._manifest_declared_template( + preset_declarations = self._preset_declarations_for_resource( pack_dir, template_name, template_type ) - if entry is None: - entry, manifest_candidate = self._regex_preset_declaration( + if not preset_declarations: + entry, candidate = self._manifest_declared_template( pack_dir, template_name, template_type ) - if entry is not None: - strategy = entry.get("strategy", "replace") - manifest_has_strategy = "strategy" in entry - # Use the manifest's declared file when it's a usable regular file; - # only fall back to convention-based lookup when the manifest - # doesn't list this template at all, so preset.yml stays - # authoritative (a declared-but-unusable file skips convention — - # parity with resolve()). - candidate = None - if manifest_candidate is not None: - candidate = manifest_candidate - elif entry is None: - candidate = _find_in_subdirs(pack_dir) - if candidate: - # Legacy fallback: if manifest doesn't explicitly declare a - # strategy, check the command file's frontmatter for any valid - # strategy. Skip when the manifest entry includes strategy key - # (even if it's "replace") to avoid overriding explicit declarations. + if entry is None: + candidate = _find_in_subdirs(pack_dir) + preset_declarations = [(entry, candidate)] + for layer_entry, candidate in preset_declarations: + strategy = "replace" + manifest_has_strategy = False + if layer_entry is not None: + strategy = layer_entry.get("strategy", "replace") + manifest_has_strategy = "strategy" in layer_entry + if candidate is None: + continue if ( not manifest_has_strategy and strategy == "replace" @@ -749,25 +750,30 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: cmd_content = candidate.read_text(encoding="utf-8") lines = cmd_content.splitlines(keepends=True) if lines and lines[0].rstrip("\r\n") == "---": - fence_end = -1 - for fi, fline in enumerate(lines[1:], start=1): - if fline.rstrip("\r\n") == "---": - fence_end = fi - break + fence_end = next( + ( + i + for i, line in enumerate(lines[1:], 1) + if line.rstrip("\r\n") == "---" + ), + -1, + ) if fence_end > 0: - fm_text = "".join(lines[1:fence_end]) - fm_data = yaml.safe_load(fm_text) - if isinstance(fm_data, dict): - fm_strategy = fm_data.get("strategy") - if ( - isinstance(fm_strategy, str) - and fm_strategy.lower() - in VALID_PRESET_STRATEGIES - ): - strategy = fm_strategy.lower() + fm_data = yaml.safe_load( + "".join(lines[1:fence_end]) + ) + fm_strategy = ( + fm_data.get("strategy") + if isinstance(fm_data, dict) + else None + ) + if ( + isinstance(fm_strategy, str) + and fm_strategy.lower() + in VALID_PRESET_STRATEGIES + ): + strategy = fm_strategy.lower() except (UnicodeDecodeError, yaml.YAMLError, OSError): - # Best-effort legacy frontmatter parsing: keep default - # strategy ("replace") when content is unreadable/invalid. pass version = metadata.get("version", "?") if metadata else "?" layers.append( diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index 71e0ded0cf..754e1920cd 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -11,7 +11,7 @@ def _diagnostic_selector_matches(resolver, preset_dir, selector, resource_type): import os - from .._selectors import is_regex_selector, selector_matches + from ._selectors import is_regex_selector, selector_matches ordered_presets = resolver._get_all_presets_by_priority() try: diff --git a/tests/specify_cli/presets/test_regex_selector_diagnostics.py b/tests/specify_cli/presets/test_regex_selector_diagnostics.py new file mode 100644 index 0000000000..137f65871d --- /dev/null +++ b/tests/specify_cli/presets/test_regex_selector_diagnostics.py @@ -0,0 +1,184 @@ +from __future__ import annotations + +from pathlib import Path + +import yaml +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.presets import PresetManager, PresetRegistry +from tests.conftest import strip_ansi + + +def _write_selector_preset( + project_dir: Path, preset_id: str, templates: list[dict] +) -> Path: + preset_dir = project_dir / ".specify" / "presets" / preset_id + preset_dir.mkdir(parents=True) + for template in templates: + payload = preset_dir / template["file"] + payload.parent.mkdir(parents=True, exist_ok=True) + payload.write_text("# selector fixture\n", encoding="utf-8") + manifest = { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": "1.0.0", + "description": "diagnostic fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": templates}, + } + (preset_dir / "preset.yml").write_text(yaml.safe_dump(manifest), encoding="utf-8") + PresetRegistry(project_dir / ".specify" / "presets").add( + preset_id, {"enabled": True, "priority": 1, "version": "1.0.0"} + ) + return preset_dir + + +def _info(project_dir: Path, preset_id: str) -> str: + from unittest.mock import patch + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["preset", "info", preset_id]) + assert result.exit_code == 0, result.output + return strip_ansi(result.output) + + +def test_preset_info_shows_regex_template_and_script_matches(project_dir): + # Core files are the concrete lower-layer resources for both declarations. + (project_dir / ".specify" / "templates" / "plan-template.md").write_text( + "# Core plan\n", encoding="utf-8" + ) + scripts = project_dir / ".specify" / "templates" / "scripts" + scripts.mkdir(parents=True) + (scripts / "check-prerequisites.sh").write_text("#!/bin/sh\n", encoding="utf-8") + preset_dir = _write_selector_preset( + project_dir, + "diagnostic-selectors", + [ + { + "type": "template", + "name": r"regex:^plan-.*$", + "file": "templates/overlay.md", + "strategy": "append", + }, + { + "type": "script", + "name": r"regex:^check-.*$", + "file": "scripts/overlay.sh", + "strategy": "wrap", + }, + ], + ) + + output = _info(project_dir, preset_dir.name) + + assert r"regex:^plan-.*$" in output + assert "plan-template" in output + assert r"regex:^check-.*$" in output + assert "check-prerequisites" in output + assert "No current matches" not in output + + +def test_preset_info_reports_unmatched_regex_selector(project_dir): + preset_dir = _write_selector_preset( + project_dir, + "empty-diagnostic-selector", + [ + { + "type": "template", + "name": r"regex:^nothing-matches$", + "file": "templates/overlay.md", + "strategy": "append", + } + ], + ) + + output = _info(project_dir, preset_dir.name) + + assert r"regex:^nothing-matches$" in output + assert "No current matches" in output + + +def test_preset_info_expands_regex_command_to_concrete_names(project_dir): + preset_dir = _write_selector_preset( + project_dir, + "command-diagnostic-selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.(plan|tasks)$", + "file": "commands/override.md", + "strategy": "replace", + } + ], + ) + + output = _info(project_dir, preset_dir.name) + + assert r"regex:^speckit\.(plan|tasks)$" in output + assert "speckit.plan" in output + assert "speckit.tasks" in output + assert "No current matches" not in output + + +def test_install_regex_commands_registers_concrete_ai_skills(project_dir): + from specify_cli import save_init_options + + (project_dir / ".claude" / "skills").mkdir(parents=True) + save_init_options( + project_dir, + {"ai": "claude", "ai_skills": True, "script": "sh"}, + ) + source_dir = project_dir / "source" + source_dir.mkdir() + source = source_dir / "skill-selector" + source.mkdir() + (source / "commands").mkdir() + (source / "commands" / "override.md").write_text( + "---\ndescription: Preset override\n---\n\nConcrete selector skill.\n", + encoding="utf-8", + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": "skill-selector", + "name": "Skill selector", + "version": "1.0.0", + "description": "AI skills selector fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": r"regex:^speckit\.(plan|tasks)$", + "file": "commands/override.md", + "strategy": "replace", + } + ] + }, + } + (source / "preset.yml").write_text(yaml.safe_dump(manifest), encoding="utf-8") + + manager = PresetManager(project_dir) + installed = manager.install_from_directory(source, "0.1.0") + + metadata = manager.registry.get(installed.id) + assert metadata is not None + assert metadata["registered_commands"] + skill_names = { + name for names in metadata["registered_skills"].values() for name in names + } + assert skill_names == {"speckit-plan", "speckit-tasks"} + for skill_name in skill_names: + skill_file = project_dir / ".claude" / "skills" / skill_name / "SKILL.md" + assert skill_file.is_file() + assert "Concrete selector skill." in skill_file.read_text(encoding="utf-8") + assert all( + not name.startswith("regex:") + for names in metadata["registered_commands"].values() + for name in names + ) diff --git a/tests/specify_cli/presets/test_regex_selector_lifecycle.py b/tests/specify_cli/presets/test_regex_selector_lifecycle.py new file mode 100644 index 0000000000..290651a164 --- /dev/null +++ b/tests/specify_cli/presets/test_regex_selector_lifecycle.py @@ -0,0 +1,78 @@ +from __future__ import annotations + +from specify_cli.extensions import _commands +from specify_cli.presets.command_disable import preset_disable + + +def test_extension_change_refresh_targets_only_active_integration( + monkeypatch, tmp_path +): + calls = [] + + class FakePresetManager: + def __init__(self, project_root): + assert project_root == tmp_path + + def register_enabled_presets_for_agent(self, agent): + calls.append(agent) + + monkeypatch.setattr( + "specify_cli._init_options.load_init_options", + lambda project_root: {"ai": "active-agent"}, + ) + monkeypatch.setattr("specify_cli.presets.PresetManager", FakePresetManager) + + _commands._refresh_presets_and_warn(tmp_path) + + assert calls == ["active-agent"] + + +def test_extension_change_refresh_skips_when_no_integration_selected( + monkeypatch, tmp_path +): + class UnexpectedPresetManager: + def __init__(self, project_root): + raise AssertionError("should not construct without an active agent") + + monkeypatch.setattr( + "specify_cli._init_options.load_init_options", lambda project_root: {} + ) + monkeypatch.setattr("specify_cli.presets.PresetManager", UnexpectedPresetManager) + + _commands._refresh_presets_and_warn(tmp_path) + + +def test_disabling_preset_keeps_registered_command_artifacts(monkeypatch, tmp_path): + state = {"enabled": True} + calls = [] + + class FakeRegistry: + def is_installed(self, preset_id): + return preset_id == "demo" + + def get(self, preset_id): + return {"enabled": state["enabled"]} + + def update(self, preset_id, updates): + state.update(updates) + + class FakePresetManager: + def __init__(self, project_root): + self.registry = FakeRegistry() + + def reconcile_constitution(self, message): + calls.append(("constitution", message)) + + def _reconcile_composed_commands(self, names): + calls.append(("commands", names)) + + def _reconcile_skills(self, names): + calls.append(("skills", names)) + + monkeypatch.setattr("specify_cli._require_specify_project", lambda: tmp_path) + monkeypatch.setattr("specify_cli.presets.PresetManager", FakePresetManager) + + preset_disable("demo") + + assert state["enabled"] is False + assert [call[0] for call in calls] == ["constitution"] diff --git a/tests/specify_cli/presets/test_regex_selectors.py b/tests/specify_cli/presets/test_regex_selectors.py index a809963523..7a7cea03d6 100644 --- a/tests/specify_cli/presets/test_regex_selectors.py +++ b/tests/specify_cli/presets/test_regex_selectors.py @@ -259,6 +259,261 @@ def test_regex_validation_and_fullmatch(tmp_path): assert not selector_matches("regex:plan", "plan-template") +def _write_command_declarations(pack_dir, preset_id, declarations): + pack_dir.mkdir(parents=True, exist_ok=True) + for index, declaration in enumerate(declarations): + payload = pack_dir / f"command-{index}.md" + payload.write_text(declaration["body"], encoding="utf-8") + declaration["file"] = payload.name + declaration.pop("body") + data = _manifest("unused", "command") + data["preset"]["id"] = preset_id + data["provides"]["templates"] = declarations + (pack_dir / "preset.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + return pack_dir + + +def _install_test_preset(project_dir, preset_id, pack_dir, priority): + PresetRegistry(project_dir / ".specify" / "presets").add( + preset_id, {"enabled": True, "priority": priority, "version": "1.0.0"} + ) + destination = project_dir / ".specify" / "presets" / preset_id + destination.mkdir(parents=True, exist_ok=True) + for path in pack_dir.iterdir(): + (destination / path.name).write_bytes(path.read_bytes()) + return destination + + +@pytest.mark.parametrize( + ("strategy", "overlay", "expected"), + [ + ("replace", "# Preset replacement\n", "# Preset replacement"), + ("prepend", "# Preset prefix\n", "# Preset prefix\n\n\n# Core command"), + ("append", "# Preset suffix\n", "# Core command\n\n\n# Preset suffix"), + ( + "wrap", + "# Wrapper start\n{CORE_TEMPLATE}\n# Wrapper end\n", + "# Wrapper start\n# Core command\n\n# Wrapper end", + ), + ], +) +def test_command_regex_composes_over_core_for_each_strategy( + project_dir, tmp_path, strategy, overlay, expected +): + core_commands = project_dir / ".specify" / "templates" / "commands" + core_commands.mkdir(parents=True, exist_ok=True) + (core_commands / "plan.md").write_text("# Core command\n", encoding="utf-8") + pack_dir = tmp_path / "selector" + declaration = { + "type": "command", + "name": r"regex:^speckit\.plan$", + "strategy": strategy, + "body": overlay, + } + _write_command_declarations(pack_dir, "selector", [declaration]) + installed = _install_test_preset(project_dir, "selector", pack_dir, 1) + + content = PresetResolver(project_dir).resolve_content("speckit.plan", "command") + + assert content is not None + assert content.strip() == expected.strip() + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + assert [layer["source"] for layer in layers] == ["selector v1.0.0", "core"] + assert layers[0]["path"] == installed / "command-0.md" + + +def test_command_regex_expands_and_composes_over_extension_layer(project_dir, tmp_path): + extension = project_dir / ".specify" / "extensions" / "demo" + (extension / "commands").mkdir(parents=True) + (extension / "commands" / "speckit.demo.md").write_text( + "# Extension command\n", encoding="utf-8" + ) + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "demo", {"enabled": True, "priority": 10, "version": "1.0"} + ) + pack_dir = tmp_path / "selector" + _write_command_declarations( + pack_dir, + "selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.demo$", + "strategy": "append", + "body": "# Preset addition\n", + } + ], + ) + _install_test_preset(project_dir, "selector", pack_dir, 1) + + content = PresetResolver(project_dir).resolve_content("speckit.demo", "command") + + assert content == "# Extension command\n\n\n# Preset addition\n" + layers = PresetResolver(project_dir).collect_all_layers("speckit.demo", "command") + assert [layer["source"] for layer in layers] == [ + "selector v1.0.0", + "extension:demo v1.0", + ] + + +def test_command_regex_expands_and_composes_over_lower_preset(project_dir, tmp_path): + lower_pack = tmp_path / "lower" + _write_command_declarations( + lower_pack, + "lower", + [ + { + "type": "command", + "name": "speckit.lower", + "strategy": "replace", + "body": "# Lower preset command\n", + } + ], + ) + _install_test_preset(project_dir, "lower", lower_pack, 10) + selector_pack = tmp_path / "selector" + _write_command_declarations( + selector_pack, + "selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.lower$", + "strategy": "prepend", + "body": "# Selector prefix\n", + } + ], + ) + _install_test_preset(project_dir, "selector", selector_pack, 1) + + content = PresetResolver(project_dir).resolve_content("speckit.lower", "command") + + assert content == "# Selector prefix\n\n\n# Lower preset command\n" + layers = PresetResolver(project_dir).collect_all_layers("speckit.lower", "command") + assert [layer["source"] for layer in layers] == [ + "selector v1.0.0", + "lower v1.0.0", + ] + + +def test_command_regex_excludes_higher_preset_and_project_override( + project_dir, tmp_path +): + # The selector is lower priority than both the higher preset and the project + # override. Neither may make a command eligible for selector expansion. + higher_pack = tmp_path / "higher" + _write_command_declarations( + higher_pack, + "higher", + [ + { + "type": "command", + "name": "speckit.higher", + "strategy": "replace", + "body": "# Higher command\n", + } + ], + ) + _install_test_preset(project_dir, "higher", higher_pack, 1) + selector_pack = tmp_path / "selector" + _write_command_declarations( + selector_pack, + "selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.(higher|override)$", + "strategy": "append", + "body": "# Selector addition\n", + } + ], + ) + installed = _install_test_preset(project_dir, "selector", selector_pack, 10) + override_dir = project_dir / ".specify" / "templates" / "overrides" + override_dir.mkdir(parents=True) + (override_dir / "speckit.override.md").write_text("# Project override\n") + manager = PresetManager(project_dir) + manifest = PresetManifest(installed / "preset.yml") + + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), installed, manifest.templates + ) + + assert expanded == [] + assert ( + PresetResolver(project_dir).collect_all_layers("speckit.higher", "command")[0][ + "source" + ] + == "higher v1.0.0" + ) + assert ( + PresetResolver(project_dir).collect_all_layers("speckit.override", "command")[ + 0 + ]["source"] + == "project override" + ) + + +def test_three_overlapping_command_regexes_keep_declaration_order_and_compose( + project_dir, tmp_path +): + core_commands = project_dir / ".specify" / "templates" / "commands" + core_commands.mkdir(parents=True, exist_ok=True) + (core_commands / "plan.md").write_text("# Core command\n", encoding="utf-8") + pack_dir = tmp_path / "selector" + declarations = [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "strategy": "prepend", + "body": "# Declared first\n", + }, + { + "type": "command", + "name": r"regex:^speckit\.p.*$", + "strategy": "append", + "body": "# Declared second\n", + }, + { + "type": "command", + "name": r"regex:^speckit\.pla.*n$", + "strategy": "wrap", + "body": "# Declared third start\n{CORE_TEMPLATE}\n# Declared third end\n", + }, + ] + _write_command_declarations(pack_dir, "selector", declarations) + _install_test_preset(project_dir, "selector", pack_dir, 1) + manager = PresetManager(project_dir) + manifest = PresetManifest( + project_dir / ".specify" / "presets" / "selector" / "preset.yml" + ) + + expanded = manager._expand_command_selectors( + PresetResolver(project_dir), + project_dir / ".specify" / "presets" / "selector", + manifest.templates, + ) + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + content = PresetResolver(project_dir).resolve_content("speckit.plan", "command") + + assert [entry["name"] for entry in expanded] == [ + "speckit.plan", + "speckit.plan", + "speckit.plan", + ] + assert [layer["path"].name for layer in layers] == [ + "command-0.md", + "command-1.md", + "command-2.md", + "plan.md", + ] + assert content is not None + assert content.index("Declared first") < content.index("Declared third start") + assert content.index("Declared third start") < content.index("Core command") + assert content.index("Core command") < content.index("Declared third end") + assert content.index("Declared third end") < content.index("Declared second") + + def test_invalid_regex_fails_during_manifest_validation(tmp_path): path = tmp_path / "preset.yml" path.write_text(yaml.safe_dump(_manifest("regex:[unterminated")), encoding="utf-8") @@ -320,7 +575,7 @@ def test_same_priority_regex_layers_follow_preset_id_order(project_dir): "regex:^plan-template$", priority=10, strategy="append", - body="zeta\\n", + body="zeta\n", ) _write_preset( project_dir, @@ -328,7 +583,7 @@ def test_same_priority_regex_layers_follow_preset_id_order(project_dir): "regex:^plan-template$", priority=10, strategy="append", - body="alpha\\n", + body="alpha\n", ) layers = PresetResolver(project_dir).collect_all_layers("plan-template", "template") regex_sources = [ @@ -366,7 +621,7 @@ def test_project_override_does_not_hide_real_regex_lower_layer(project_dir): ) core = project_dir / ".specify" / "templates" / "foo-template.md" core.parent.mkdir(parents=True, exist_ok=True) - core.write_text("core\\n", encoding="utf-8") + core.write_text("core\n", encoding="utf-8") layers = PresetResolver(project_dir).collect_all_layers("foo-template", "template") assert [layer["source"] for layer in layers] == [ "project override", From fd392d854bf38d6618c77544d598a56cba101f20 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Tue, 29 Sep 2026 21:44:36 +0800 Subject: [PATCH 3/6] fix(presets): complete regex selector lifecycle handling Add install-time warnings for unmatched template, script, and command selectors while keeping zero-match selectors non-fatal. Reconcile constitution snapshots when regex selectors target the constitution template, and preserve concrete command matches during AI skill registration and reconciliation. Include extension manifest-declared resources in selector diagnostics so reported matches stay consistent with actual resolver behavior. Handle preset and extension lifecycle changes using selector-aware reconciliation to remove stale command and skill artifacts and restore newly matching resources. Make priority-change reconciliation failures consistent with existing lifecycle behavior and add regression coverage for enable, disable, priority, diagnostics, constitution, and skill scenarios. --- src/specify_cli/presets/_manager.py | 89 ++++++++++++++++++- src/specify_cli/presets/_manager_commands.py | 28 ++++++ src/specify_cli/presets/_manager_skills.py | 22 ++++- src/specify_cli/presets/command_disable.py | 25 +++++- src/specify_cli/presets/command_enable.py | 22 ++--- src/specify_cli/presets/command_info.py | 46 ++++++---- .../presets/command_set_priority.py | 47 +++++----- .../integrations/test_command_upgrade.py | 6 +- .../presets/test_regex_selector_lifecycle.py | 17 ++-- 9 files changed, 230 insertions(+), 72 deletions(-) diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 1ab1f50b25..a45e76a098 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -30,7 +30,7 @@ ) from ._registry import PresetRegistry from ._resolver import PresetResolver -from ._selectors import is_regex_selector +from ._selectors import is_regex_selector, selector_matches _CONSTITUTION_PROVENANCE_FILE = ".constitution-template.json" _CONSTITUTION_SYNC_PRESET_ID = "constitution-sync" @@ -447,6 +447,7 @@ def install_from_directory( for template in manifest.templates if template.get("type") == "command" ] + self._warn_unmatched_resource_selectors(manifest, dest_dir) command_templates = self._expand_command_selectors( PresetResolver(self.project_root), dest_dir, raw_command_templates ) @@ -537,6 +538,76 @@ def install_from_directory( return manifest + def _warn_unmatched_resource_selectors( + self, manifest: PresetManifest, preset_dir: Path + ) -> None: + """Warn, without persisting state, for selectors with no lower-layer match.""" + import warnings + + from ._selectors import is_regex_selector, selector_matches + + resolver = PresetResolver(self.project_root) + for declaration in manifest.templates: + selector = declaration.get("name") + resource_type = declaration.get("type") + if not isinstance(selector, str) or not is_regex_selector(selector): + continue + if resource_type == "command": + matches = self._expand_command_selectors( + resolver, preset_dir, [declaration] + ) + elif resource_type in {"template", "script"}: + from ..artifacts.catalog import ArtifactCatalog + + inventory = ArtifactCatalog(self.project_root).list_artifacts() + candidates = { + artifact.name + for artifact in inventory + if artifact.kind == resource_type + and isinstance(artifact.name, str) + and selector_matches(selector, artifact.name) + and self._has_lower_resource( + preset_dir, artifact.name, resource_type + ) + } + matches = [{"name": name} for name in sorted(candidates)] + else: + continue + if not matches: + warnings.warn( + f"Preset '{manifest.id}' selector '{selector}' currently matches no {resource_type} resources. " + "Regex selectors only match concrete resources from lower layers.", + stacklevel=2, + ) + + def _has_lower_resource( + self, preset_dir: Path, name: str, resource_type: str + ) -> bool: + try: + resolver = PresetResolver(self.project_root) + except Exception: + return False + stack = self.registry.list_by_priority() + try: + index = next( + i for i, (pid, _meta) in enumerate(stack) if pid == preset_dir.name + ) + except StopIteration: + return False + if any( + resolver._has_concrete_resource(self.presets_dir / pid, name, resource_type) + for pid, _meta in stack[index + 1 :] + ): + return True + if any( + resolver._extension_has_concrete_resource( + resolver.extensions_dir / ext_id, name, resource_type + ) + for _priority, ext_id, _meta in resolver._get_all_extensions_by_priority() + ): + return True + return resolver._core_has_concrete_resource(name, resource_type) + def _seed_constitution_from_preset( self, manifest: PresetManifest, preset_dir: Path ) -> None: @@ -549,8 +620,20 @@ def _seed_constitution_from_preset( provides_constitution = ( manifest.id == _CONSTITUTION_SYNC_PRESET_ID or any( - t.get("type") == "template" and t.get("name") == "constitution-template" - for t in manifest.templates + declaration.get("type") == "template" + and isinstance(declaration.get("name"), str) + and declaration.get("name") == "constitution-template" + for declaration in manifest.templates + ) + or any( + declaration.get("type") == "template" + and isinstance(declaration.get("name"), str) + and is_regex_selector(declaration["name"]) + and selector_matches(declaration["name"], "constitution-template") + and self._has_lower_resource( + preset_dir, "constitution-template", "template" + ) + for declaration in manifest.templates ) or any( (preset_dir / relative_path).is_file() diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index e7f264abed..6bbf626c24 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -92,6 +92,34 @@ def _substitute_core_template( class _PresetCommandMethods: """Command artifact methods shared through PresetManager's lifecycle state.""" + def _collect_selector_command_names(self, resolver: PresetResolver) -> set[str]: + """Collect concrete command names referenced by installed preset declarations.""" + names: set[str] = set() + for preset_id, _metadata in self.registry.list_by_priority( + include_disabled=True + ): + preset_dir = self.presets_dir / preset_id + manifest = resolver._get_manifest(preset_dir) + if manifest is None: + continue + declarations = [ + item for item in manifest.templates if item.get("type") == "command" + ] + expanded = _PresetCommandMethods._expand_command_selectors( + self, resolver, preset_dir, declarations + ) + names.update( + item["name"] + for item in expanded + if isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + ) + metadata = self.registry.get(preset_id) or {} + for per_agent in (metadata.get("registered_commands", {}) or {}).values(): + if isinstance(per_agent, list): + names.update(name for name in per_agent if isinstance(name, str)) + return names + def _expand_command_selectors( self, resolver: PresetResolver, diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 601ca08b64..9049bdb935 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -30,15 +30,18 @@ class _FilteredManifest: that aren't being reconciled. """ - def __init__(self, manifest: "PresetManifest", cmd_names: set): + def __init__(self, manifest: "PresetManifest", cmd_names: set, commands=None): self._manifest = manifest self._cmd_names = cmd_names + self._commands = commands def __getattr__(self, name: str): return getattr(self._manifest, name) @property def templates(self) -> List[Dict[str, Any]]: + if self._commands is not None: + return [t for t in self._commands if t.get("name") in self._cmd_names] return [ t for t in self._manifest.templates if t.get("name") in self._cmd_names ] @@ -327,7 +330,22 @@ def apply_to_dir( except PresetValidationError: continue cmds_set = set(dir_cmds) - filtered_manifest = self._FilteredManifest(manifest, cmds_set) + from ._manager_commands import _PresetCommandMethods + + command_methods = _PresetCommandMethods() + command_methods.__dict__.update(self.__dict__) + concrete_declarations = command_methods._expand_command_selectors( + PresetResolver(self.project_root), + pack_dir, + [ + item + for item in manifest.templates + if item.get("type") == "command" + ], + ) + filtered_manifest = self._FilteredManifest( + manifest, cmds_set, concrete_declarations + ) # Not dead code: _register_skills only *overwrites* skill # subdirectories that already exist (plus brand-new ones for # the active ai_skills agent). For a restore into a diff --git a/src/specify_cli/presets/command_disable.py b/src/specify_cli/presets/command_disable.py index 85c0231abd..014673e027 100644 --- a/src/specify_cli/presets/command_disable.py +++ b/src/specify_cli/presets/command_disable.py @@ -36,15 +36,32 @@ def preset_disable( console.print(f"[yellow]Preset '{preset_id}' is already disabled[/yellow]") raise typer.Exit(0) - # Disable the preset + # Capture concrete names before the mutation as disabling can make selector + # declarations disappear from the resolver's lower-layer inventory. + from ._resolver import PresetResolver + + resolver = PresetResolver(project_root) + affected = manager._collect_selector_command_names(resolver) manager.registry.update(preset_id, {"enabled": False}) + affected.update( + manager._collect_selector_command_names(PresetResolver(project_root)) + ) + names = sorted(affected) + if names: + import warnings + + try: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) + except Exception as exc: + warnings.warn( + f"Could not reconcile preset artifacts after disabling {preset_id}: {exc}", + stacklevel=2, + ) manager.reconcile_constitution( f"Failed to reconcile constitution after disabling preset {preset_id}" ) console.print(f"[green]✓[/green] Preset '{preset_id}' disabled") console.print("\nTemplates from this preset will be skipped during resolution.") - console.print( - "[dim]Note: Previously registered commands/skills remain active until preset removal.[/dim]" - ) console.print(f"To re-enable: specify preset enable {preset_id}") diff --git a/src/specify_cli/presets/command_enable.py b/src/specify_cli/presets/command_enable.py index d335b6f8d2..d73178f6d3 100644 --- a/src/specify_cli/presets/command_enable.py +++ b/src/specify_cli/presets/command_enable.py @@ -41,27 +41,17 @@ def preset_enable( try: from ._manifest import PresetManifest from ._resolver import PresetResolver - from ._selectors import is_regex_selector manifest_path = manager.presets_dir / preset_id / "preset.yml" + names: list[str] = [] if manifest_path.is_file(): - manifest = PresetManifest(manifest_path) - expanded = manager._expand_command_selectors( - PresetResolver(project_root), - manager.presets_dir / preset_id, - [item for item in manifest.templates if item.get("type") == "command"], - ) + PresetManifest(manifest_path) names = sorted( - { - item["name"] - for item in expanded - if isinstance(item.get("name"), str) - and not is_regex_selector(item["name"]) - } + manager._collect_selector_command_names(PresetResolver(project_root)) ) - if names: - manager._reconcile_composed_commands(names) - manager._reconcile_skills(names) + if names: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) except Exception as exc: import warnings diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index 754e1920cd..6d2e944b47 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -32,27 +32,41 @@ def _diagnostic_selector_matches(resolver, preset_dir, selector, resource_type): ] candidates = set() for base in [*lower_presets, *lower_extensions]: - manifest = ( - resolver._get_manifest(base) - if base.parent == resolver.presets_dir - else None + if base.parent == resolver.presets_dir: + manifest = resolver._get_manifest(base) + declarations = manifest.templates if manifest is not None else [] + else: + from ..extensions import ExtensionManifest + + manifest_path = base / "extension.yml" + try: + manifest = ( + ExtensionManifest(manifest_path) + if manifest_path.is_file() + else None + ) + except Exception: + manifest = None + declarations = [] + if manifest is not None: + provides = manifest.data.get("provides", {}) + key = {"template": "templates", "script": "scripts"}.get(resource_type) + declarations = provides.get(key, []) if key else [] + candidates.update( + item["name"] + for item in declarations + if isinstance(item, dict) + and item.get("type", resource_type) == resource_type + and isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) ) - if manifest: - candidates.update( - item["name"] - for item in manifest.templates - if item.get("type") == resource_type - and isinstance(item.get("name"), str) - and not is_regex_selector(item["name"]) - ) subdir = "templates" if resource_type == "template" else "scripts" suffix = ".sh" if resource_type == "script" else ".md" - for path in (base / subdir).glob("**/*") if (base / subdir).is_dir() else []: + root = base / subdir + for path in root.glob("**/*") if root.is_dir() else []: if path.is_file() and path.name.endswith(suffix): candidates.add( - os.path.relpath(path, base / subdir)[: -len(suffix)].replace( - os.sep, "-" - ) + os.path.relpath(path, root)[: -len(suffix)].replace(os.sep, "-") ) suffix = ".sh" if resource_type == "script" else ".md" core_roots = [ diff --git a/src/specify_cli/presets/command_set_priority.py b/src/specify_cli/presets/command_set_priority.py index 41f8487f24..0590e86a6a 100644 --- a/src/specify_cli/presets/command_set_priority.py +++ b/src/specify_cli/presets/command_set_priority.py @@ -57,33 +57,34 @@ def preset_set_priority( old_priority = normalize_priority(raw_priority) from ._resolver import PresetResolver - from ._selectors import is_regex_selector resolver = PresetResolver(project_root) - affected_commands: set[str] = set() - for pack_id, _pack_metadata in manager.registry.list_by_priority(): - manifest = resolver._get_manifest(manager.presets_dir / pack_id) - if manifest is None: - continue - expanded = manager._expand_command_selectors( - resolver, - manager.presets_dir / pack_id, - [item for item in manifest.templates if item.get("type") == "command"], - ) - affected_commands.update( - item["name"] - for item in expanded - if isinstance(item.get("name"), str) and not is_regex_selector(item["name"]) - ) - + affected_commands = manager._collect_selector_command_names(resolver) manager.registry.update(preset_id, {"priority": priority}) - names = sorted(affected_commands) - if names: - manager._reconcile_composed_commands(names) - manager._reconcile_skills(names) - manager.reconcile_constitution( - f"Failed to reconcile constitution after changing priority for preset {preset_id}" + affected_commands.update( + manager._collect_selector_command_names(PresetResolver(project_root)) ) + names = sorted(affected_commands) + try: + if names: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) + manager.reconcile_constitution( + f"Failed to reconcile constitution after changing priority for preset {preset_id}" + ) + except Exception: + # Restore both registry priority and artifacts against the previous winner. + manager.registry.update(preset_id, {"priority": old_priority}) + try: + if names: + manager._reconcile_composed_commands(names) + manager._reconcile_skills(names) + manager.reconcile_constitution( + f"Failed to restore constitution after reverting priority for preset {preset_id}" + ) + except Exception: + pass + raise console.print( f"[green]✓[/green] Preset '{preset_id}' priority changed: {old_priority} → {priority}" diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 42d5ea0a6c..4341897703 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -846,7 +846,7 @@ def test_upgrade_active_layout_change_rejected_with_disabled_preset( assert result.exit_code == 0, f"preset disable failed: {result.output}" cmd_file = commands / "speckit.plan.md" - assert "Overridden plan content" in cmd_file.read_text(encoding="utf-8") + assert "Overridden plan content" not in cmd_file.read_text(encoding="utf-8") result = _run_in_project(project, [ "integration", "upgrade", "bob", @@ -858,8 +858,8 @@ def test_upgrade_active_layout_change_rejected_with_disabled_preset( ) assert "cmd-preset" in result.output assert not skills.exists(), "no skills layout must be scaffolded on rejection" - assert "Overridden plan content" in cmd_file.read_text(encoding="utf-8"), ( - "the disabled preset's command file must be left untouched" + assert "Overridden plan content" not in cmd_file.read_text(encoding="utf-8"), ( + "disable reconciliation should remove the disabled override" ) # Enabled presets are also rejected: rescaffolding can still fail. diff --git a/tests/specify_cli/presets/test_regex_selector_lifecycle.py b/tests/specify_cli/presets/test_regex_selector_lifecycle.py index 290651a164..4fe7052dcb 100644 --- a/tests/specify_cli/presets/test_regex_selector_lifecycle.py +++ b/tests/specify_cli/presets/test_regex_selector_lifecycle.py @@ -42,16 +42,19 @@ def __init__(self, project_root): _commands._refresh_presets_and_warn(tmp_path) -def test_disabling_preset_keeps_registered_command_artifacts(monkeypatch, tmp_path): +def test_disabling_preset_reconciles_registered_artifacts(monkeypatch, tmp_path): state = {"enabled": True} calls = [] class FakeRegistry: + def list_by_priority(self, include_disabled=False): + return [("demo", {"enabled": state["enabled"]})] + def is_installed(self, preset_id): return preset_id == "demo" def get(self, preset_id): - return {"enabled": state["enabled"]} + return {"enabled": state["enabled"], "registered_commands": {}} def update(self, preset_id, updates): state.update(updates) @@ -60,8 +63,8 @@ class FakePresetManager: def __init__(self, project_root): self.registry = FakeRegistry() - def reconcile_constitution(self, message): - calls.append(("constitution", message)) + def _collect_selector_command_names(self, resolver): + return {"speckit.plan"} def _reconcile_composed_commands(self, names): calls.append(("commands", names)) @@ -69,10 +72,14 @@ def _reconcile_composed_commands(self, names): def _reconcile_skills(self, names): calls.append(("skills", names)) + def reconcile_constitution(self, message): + calls.append(("constitution", message)) + monkeypatch.setattr("specify_cli._require_specify_project", lambda: tmp_path) monkeypatch.setattr("specify_cli.presets.PresetManager", FakePresetManager) preset_disable("demo") assert state["enabled"] is False - assert [call[0] for call in calls] == ["constitution"] + assert [call[0] for call in calls] == ["commands", "skills", "constitution"] + assert calls[0][1] == ["speckit.plan"] From 8cec3df194881b919f64121e99121a685e23eaf6 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Wed, 30 Sep 2026 00:09:33 +0800 Subject: [PATCH 4/6] Fix regex selector preset lifecycle tracking text - Expand regex command selectors before resolving layers and registering AI skills. - Track concrete command and skill names for regex-owned and composed commands. - Reconcile generated artifacts when presets are disabled or re-enabled. - Preserve the Templates count in preset info output. - Add end-to-end tests for selector artifact lifecycle and composition. Validation: - 8,626 passed, 17 skipped - Ruff checks passed - git diff --check passed --- .../integrations/_command_upgrade_layout.py | 22 +- .../integrations/command_upgrade.py | 111 ++++++++-- src/specify_cli/presets/_manager_commands.py | 130 ++++++------ src/specify_cli/presets/_manager_skills.py | 12 +- src/specify_cli/presets/_resolver.py | 29 ++- src/specify_cli/presets/command_disable.py | 97 +++++++-- src/specify_cli/presets/command_enable.py | 57 ++++-- src/specify_cli/presets/command_info.py | 1 + .../presets/test_regex_selector_lifecycle.py | 190 +++++++++++++++++- 9 files changed, 518 insertions(+), 131 deletions(-) diff --git a/src/specify_cli/integrations/_command_upgrade_layout.py b/src/specify_cli/integrations/_command_upgrade_layout.py index caf4ba52d1..25099974ed 100644 --- a/src/specify_cli/integrations/_command_upgrade_layout.py +++ b/src/specify_cli/integrations/_command_upgrade_layout.py @@ -5,6 +5,7 @@ import json from pathlib import Path, PurePath + def _manifest_tracks_skill_layout(manifest) -> bool: """Return True when *manifest* tracks any skills-layout artifact. @@ -93,6 +94,7 @@ def _installed_presets_affecting_agent( agent_key: str, *, include_skills: bool = True, + include_disabled: bool = True, ) -> list[str]: """Return IDs of installed presets with artifacts registered for *agent_key*. @@ -128,9 +130,7 @@ def _installed_presets_affecting_agent( except (OSError, ValueError) as exc: raise _PresetRegistryUnreadableError(str(exc)) from exc if not isinstance(data, dict) or not isinstance(data.get("presets", {}), dict): - raise _PresetRegistryUnreadableError( - "preset registry structure is malformed" - ) + raise _PresetRegistryUnreadableError("preset registry structure is malformed") affected: list[str] = [] for preset_id, meta in data.get("presets", {}).items(): @@ -147,21 +147,24 @@ def _installed_presets_affecting_agent( raise _PresetRegistryUnreadableError( f"preset '{preset_id}' registered_commands is malformed" ) + if not meta.get("enabled", True): + # Disabled presets can leave old-layout artifacts tracked in the + # integration manifest even after reconciliation clears their + # active registration lists. Refuse a layout migration while the + # preset remains installed; removal is the safe boundary. + affected.append(preset_id) + continue registered_skills = meta.get("registered_skills", []) if isinstance(registered_skills, dict): # Per-agent provenance ({agent: [skill names]}): only entries for # *this* agent make the preset affect it. Values must be lists — # anything else (e.g. null) leaves ownership undecidable, so fail # closed rather than read it as "no artifacts". - if not all( - isinstance(names, list) for names in registered_skills.values() - ): + if not all(isinstance(names, list) for names in registered_skills.values()): raise _PresetRegistryUnreadableError( f"preset '{preset_id}' registered_skills is malformed" ) - has_skills = include_skills and bool( - registered_skills.get(agent_key) - ) + has_skills = include_skills and bool(registered_skills.get(agent_key)) elif isinstance(registered_skills, (list, tuple)): # Legacy flat list: not agent-scoped, so any recorded skill may # belong to this agent — fail closed and count it as affecting. @@ -185,4 +188,5 @@ def _installed_command_presets_affecting_agent( project_root, agent_key, include_skills=False, + include_disabled=False, ) diff --git a/src/specify_cli/integrations/command_upgrade.py b/src/specify_cli/integrations/command_upgrade.py index b4682b6825..06a7de9054 100644 --- a/src/specify_cli/integrations/command_upgrade.py +++ b/src/specify_cli/integrations/command_upgrade.py @@ -1,4 +1,5 @@ """The ``specify integration upgrade`` command and its layout guards.""" + from __future__ import annotations import os @@ -12,7 +13,10 @@ invoke_separator_for_integration as _invoke_separator_for_integration, with_integration_setting as _with_integration_setting, ) -from ..integration_state import default_integration_key as _default_integration_key, installed_integration_keys as _installed_integration_keys +from ..integration_state import ( + default_integration_key as _default_integration_key, + installed_integration_keys as _installed_integration_keys, +) from ._command_upgrade_layout import ( _PresetRegistryUnreadableError, _installed_command_presets_affecting_agent, @@ -22,15 +26,41 @@ _manifest_tracks_skill_layout, ) from ._commands import integration_app -from ._helpers import _MANIFEST_READ_ERRORS, _SharedTemplateRefreshError, _cli_error_detail, _cli_phase_label, _get_speckit_version, _read_integration_json, _refresh_init_options_speckit_version, _register_extensions_for_agent, _register_presets_for_agent, _resolve_integration_options, _resolve_integration_script_type, _resync_manifest_after_registration, _unregister_enabled_extension_commands_for_agent, _update_init_options_for_integration, _write_integration_json +from ._helpers import ( + _MANIFEST_READ_ERRORS, + _SharedTemplateRefreshError, + _cli_error_detail, + _cli_phase_label, + _get_speckit_version, + _read_integration_json, + _refresh_init_options_speckit_version, + _register_extensions_for_agent, + _register_presets_for_agent, + _resolve_integration_options, + _resolve_integration_script_type, + _resync_manifest_after_registration, + _unregister_enabled_extension_commands_for_agent, + _update_init_options_for_integration, + _write_integration_json, +) @integration_app.command("upgrade") def integration_upgrade( - key: str | None = typer.Argument(None, help="Integration key to upgrade (default: current integration)"), - force: bool = typer.Option(False, "--force", help="Force upgrade even if files are modified"), - script: str | None = typer.Option(None, "--script", help="Script type: sh, ps, or py (default: from init-options.json or platform default)"), - integration_options: str | None = typer.Option(None, "--integration-options", help="Options for the integration"), + key: str | None = typer.Argument( + None, help="Integration key to upgrade (default: current integration)" + ), + force: bool = typer.Option( + False, "--force", help="Force upgrade even if files are modified" + ), + script: str | None = typer.Option( + None, + "--script", + help="Script type: sh, ps, or py (default: from init-options.json or platform default)", + ), + integration_options: str | None = typer.Option( + None, "--integration-options", help="Options for the integration" + ), ): """Upgrade an integration by reinstalling with diff-aware file handling. @@ -39,7 +69,11 @@ def integration_upgrade( """ from . import get_integration from .manifest import IntegrationManifest - from .. import _require_specify_project, _install_shared_infra_or_exit, _install_shared_infra + from .. import ( + _require_specify_project, + _install_shared_infra_or_exit, + _install_shared_infra, + ) project_root = _require_specify_project() current = _read_integration_json(project_root) @@ -63,26 +97,38 @@ def integration_upgrade( manifest_path = project_root / ".specify" / "integrations" / f"{key}.manifest.json" if not manifest_path.exists(): - console.print(f"[yellow]No manifest found for integration '{key}'. Nothing to upgrade.[/yellow]") - console.print(f"Run [cyan]specify integration install {key}[/cyan] to perform a fresh install.") + console.print( + f"[yellow]No manifest found for integration '{key}'. Nothing to upgrade.[/yellow]" + ) + console.print( + f"Run [cyan]specify integration install {key}[/cyan] to perform a fresh install." + ) raise typer.Exit(0) try: old_manifest = IntegrationManifest.load(key, project_root) except _MANIFEST_READ_ERRORS as exc: - console.print(f"[red]Error:[/red] Integration manifest for '{key}' is unreadable: {exc}") + console.print( + f"[red]Error:[/red] Integration manifest for '{key}' is unreadable: {exc}" + ) raise typer.Exit(1) # Detect modified files via manifest hashes modified = old_manifest.check_modified() if modified and not force: - console.print(f"[yellow]⚠[/yellow] {len(modified)} file(s) have been modified since installation:") + console.print( + f"[yellow]⚠[/yellow] {len(modified)} file(s) have been modified since installation:" + ) for rel in modified: console.print(f" {rel}") - console.print("\nUse [cyan]--force[/cyan] to overwrite modified files, or resolve manually.") + console.print( + "\nUse [cyan]--force[/cyan] to overwrite modified files, or resolve manually." + ) raise typer.Exit(1) - selected_script = _resolve_integration_script_type(project_root, current, key, script) + selected_script = _resolve_integration_script_type( + project_root, current, key, script + ) # Build parsed options from --integration-options so the integration # can determine its effective invoke separator before shared infra @@ -154,7 +200,9 @@ def integration_upgrade( parsed_options, project_root ): try: - affected_presets = _installed_presets_affecting_agent(project_root, key) + affected_presets = _installed_presets_affecting_agent( + project_root, key, include_disabled=True + ) except _PresetRegistryUnreadableError as exc: console.print( f"[red]Error:[/red] Cannot change '{key}' command layout: the " @@ -183,7 +231,7 @@ def integration_upgrade( "Remove the preset(s), run the upgrade, then reinstall them:\n" f" [cyan]specify preset remove [/cyan]\n" f" [cyan]specify integration upgrade {key} " - f"--integration-options \"...\"[/cyan]\n" + f'--integration-options "..."[/cyan]\n' f" [cyan]specify preset add [/cyan]" ) raise typer.Exit(1) @@ -205,7 +253,10 @@ def integration_upgrade( selected_script, force=force, invoke_separator=_invoke_separator_for_integration( - infra_integration, current, infra_key, infra_parsed, + infra_integration, + current, + infra_key, + infra_parsed, project_root=project_root, ), invoke_prefix=_invoke_prefix_for_integration( @@ -214,13 +265,17 @@ def integration_upgrade( ) if os.name != "nt": from .. import ensure_executable_scripts + ensure_executable_scripts(project_root) # Phase 1: Install new files (overwrites existing; old-only files remain) console.print(f"Upgrading integration: [cyan]{key}[/cyan]") - new_manifest = IntegrationManifest(key, project_root, version=_get_speckit_version()) + new_manifest = IntegrationManifest( + key, project_root, version=_get_speckit_version() + ) from ..events import resolve_events + events_map = resolve_events( key, integration.config, @@ -251,7 +306,10 @@ def integration_upgrade( project_root, selected_script, invoke_separator=_invoke_separator_for_integration( - integration, {"integration_settings": settings}, key, parsed_options, + integration, + {"integration_settings": settings}, + key, + parsed_options, project_root=project_root, ), invoke_prefix=_invoke_prefix_for_integration( @@ -266,6 +324,7 @@ def integration_upgrade( ) from exc if os.name != "nt": from .. import ensure_executable_scripts + ensure_executable_scripts(project_root) new_manifest.save() _write_integration_json(project_root, installed_key, installed_keys, settings) @@ -281,9 +340,13 @@ def integration_upgrade( except Exception as exc: # Don't teardown — setup overwrites in-place, so teardown would # delete files that were working before the upgrade. Just report. - console.print(f"[red]Error:[/red] Failed to {_cli_phase_label('upgrade', 'integration', key)}.") + console.print( + f"[red]Error:[/red] Failed to {_cli_phase_label('upgrade', 'integration', key)}." + ) console.print(f"[dim]Details:[/dim] {_cli_error_detail(exc)}") - console.print("[yellow]The previous integration files may still be in place.[/yellow]") + console.print( + "[yellow]The previous integration files may still be in place.[/yellow]" + ) raise typer.Exit(1) # Phase 2: Remove stale files from old manifest that are not in the new one @@ -294,7 +357,9 @@ def integration_upgrade( # as "stale" while still being actively managed. Manifest keys are stored # in POSIX form, so normalize the exclusions the same way before subtracting # (an integration may build paths with os.path.join / backslashes). - exclusions = {PurePath(p).as_posix() for p in integration.stale_cleanup_exclusions()} + exclusions = { + PurePath(p).as_posix() for p in integration.stale_cleanup_exclusions() + } stale_keys = (set(old_files) - set(new_files)) - exclusions if stale_keys: stale_manifest = IntegrationManifest(key, project_root, version="stale-cleanup") @@ -309,7 +374,9 @@ def integration_upgrade( project_root, force=True, remove_manifest=False ) if stale_removed: - console.print(f" Removed {len(stale_removed)} stale file(s) from previous install") + console.print( + f" Removed {len(stale_removed)} stale file(s) from previous install" + ) legacy_command_root_changed = _legacy_command_root_changed( integration, diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index 6bbf626c24..a1646145b1 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -418,9 +418,13 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: load_init_options(self.project_root) ) + from ._resolver import PresetResolver + from ._selectors import is_regex_selector + resolver = PresetResolver(self.project_root) - affected_cmd_names: set = set() + affected_cmd_names: set[str] = set() presets_by_priority = list(self.registry.list_by_priority()) + expanded_declarations_by_pack: Dict[str, List[Dict[str, Any]]] = {} winning_pack_by_command: Dict[str, str] = {} winning_source_by_command: Dict[str, Path] = {} project_override_commands: set[str] = set() @@ -438,6 +442,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: expanded_for_reconcile = self._expand_command_selectors( resolver, self.presets_dir / candidate_pack_id, command_templates ) + expanded_declarations_by_pack[candidate_pack_id] = expanded_for_reconcile for template in expanded_for_reconcile: command_name = template.get("name") if not isinstance(command_name, str) or is_regex_selector(command_name): @@ -477,35 +482,22 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: if manifest is None: continue - # Registration can write one command and then fail on a later - # template. Record names first so final reconciliation can repair - # any partial writes even when _register_commands never returns. - for tmpl in manifest.templates: + concrete_declarations = expanded_declarations_by_pack.get(pack_id, []) + for tmpl in concrete_declarations: name = tmpl.get("name") - if tmpl.get("type") != "command" or not isinstance(name, str): - continue - expanded_for_reconcile = ( - self._expand_command_selectors(resolver, pack_dir, [tmpl]) - if is_regex_selector(name) - else [tmpl] - ) - for expanded in expanded_for_reconcile: - concrete_name = expanded.get("name") - if isinstance(concrete_name, str) and not is_regex_selector( - concrete_name - ): - affected_cmd_names.add(concrete_name) + if isinstance(name, str) and not is_regex_selector(name): + affected_cmd_names.add(name) # Isolate per-preset failures: one preset that fails to register # must not abort registration of the remaining enabled presets. try: - registered_commands = self._register_commands(manifest, pack_dir) + registered_commands = self._register_commands( + manifest, pack_dir, command_templates=concrete_declarations + ) registered_command_names = set( registered_commands.get(agent_name) or [] ) - for tmpl in manifest.templates: - if tmpl.get("type") != "command": - continue + for tmpl in concrete_declarations: primary_name = tmpl.get("name") if ( isinstance(primary_name, str) @@ -549,11 +541,13 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: pack_id, {"registered_commands": merged_commands} ) - registered_skills = self._register_skills(manifest, pack_dir) + registered_skills = self._register_skills( + manifest, + pack_dir, + command_templates=concrete_declarations, + ) replaced_skill_names = set(registered_skills.get(agent_name) or []) - for tmpl in manifest.templates: - if tmpl.get("type") != "command": - continue + for tmpl in concrete_declarations: primary_name = tmpl.get("name") if not isinstance(primary_name, str): continue @@ -611,11 +605,11 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # anything unreplaced stays tracked and on disk (#2948). stale_skill_names = merged_skills[agent_name] skill_to_primary: Dict[str, str] = {} - for tmpl in manifest.templates: - if tmpl.get("type") != "command": - continue + for tmpl in concrete_declarations: primary_name = tmpl.get("name") - if not isinstance(primary_name, str): + if not isinstance(primary_name, str) or is_regex_selector( + primary_name + ): continue modern_name, legacy_name = self._skill_names_for_command( primary_name @@ -670,9 +664,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # whether the *primary*'s skill replacement actually # landed (#2948). alias_to_primary: Dict[str, str] = {} - for tmpl in manifest.templates: - if tmpl.get("type") != "command": - continue + for tmpl in concrete_declarations: primary_name = tmpl.get("name") if not isinstance(primary_name, str): continue @@ -1165,25 +1157,40 @@ def record_written(written: Dict[str, List[str]]) -> None: if top_path.is_relative_to(pack_dir): manifest = resolver._get_manifest(pack_dir) if manifest: - for tmpl in manifest.templates: - if ( - tmpl.get("name") == cmd_name - and tmpl.get("type") == "command" - ): - written = self._register_for_non_skill_agents( - registrar, - [tmpl], - manifest.id, + concrete = next( + ( + declaration + for declaration in self._expand_command_selectors( + resolver, pack_dir, - only_agent=only_agent, - extra_agents=extra_agents, - ) - record_written(written) - self._merge_pack_registered_commands( - manifest.id, written + [ + item + for item in manifest.templates + if item.get("type") == "command" + ], ) - registered = True - break + if declaration.get("name") == cmd_name + and (pack_dir / str(declaration.get("file", ""))) + == top_path + ), + None, + ) + if concrete is not None: + logical_declaration = {**concrete, "name": cmd_name} + written = self._register_for_non_skill_agents( + registrar, + [logical_declaration], + manifest.id, + pack_dir, + only_agent=only_agent, + extra_agents=extra_agents, + ) + record_written(written) + self._merge_pack_registered_commands( + manifest.id, written + ) + registered = True + break break if not registered: # Top layer is a non-preset source (extension, core, or @@ -1296,18 +1303,29 @@ def record_written(written: Dict[str, List[str]]) -> None: manifest = resolver._get_manifest(pack_dir) if not manifest: continue - for tmpl in manifest.templates: - if ( - tmpl.get("name") == cmd_name - and tmpl.get("type") == "command" - ): + for tmpl in self._expand_command_selectors( + resolver, + pack_dir, + [ + item + for item in manifest.templates + if item.get("type") == "command" + ], + ): + if tmpl.get("name") == cmd_name: + logical_declaration = {**tmpl, "name": cmd_name} composed_dir = pack_dir / ".composed" composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" composed_file.write_text(composed, encoding="utf-8") written = self._register_for_non_skill_agents( registrar, - [{**tmpl, "file": f".composed/{cmd_name}.md"}], + [ + { + **logical_declaration, + "file": f".composed/{cmd_name}.md", + } + ], manifest.id, pack_dir, only_agent=only_agent, diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 9049bdb935..fb03a793e7 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -139,7 +139,11 @@ def _reconcile_skills( if not isinstance(active_ai, str) or not active_ai: active_ai = None - # Cache registry once to avoid repeated filesystem reads + # Preserve disabled entries as cleanup provenance while resolving winners + # only from enabled presets. + all_presets_by_priority = list( + self.registry.list_by_priority(include_disabled=True) + ) presets_by_priority = list(self.registry.list_by_priority()) # Group command names by winning preset to batch _register_skills calls @@ -162,7 +166,7 @@ def _reconcile_skills( # Track whether any preset previously registered this skill # (i.e., it was actively managed), so a not-yet-existing skill # dir can be re-created per affected directory below. - for _pid, meta in presets_by_priority: + for _pid, meta in all_presets_by_priority: if not isinstance(meta, dict): continue recorded = meta.get("registered_skills", []) @@ -191,8 +195,6 @@ def _reconcile_skills( found_preset = True break if not found_preset: - # Winner is a non-preset source (core/extension/override). - # Track the winning layer path for skill restoration. non_preset_skills.append((skill_name, cmd_name, layers[0])) core_ext_skills = [ @@ -335,7 +337,7 @@ def apply_to_dir( command_methods = _PresetCommandMethods() command_methods.__dict__.update(self.__dict__) concrete_declarations = command_methods._expand_command_selectors( - PresetResolver(self.project_root), + resolver, pack_dir, [ item diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 7c03cf035a..5926239a1a 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -332,7 +332,7 @@ def resolve( if candidate.exists(): return candidate - # Priority 4: Core templates + # Priority 3: Project templates if template_type == "template": core = self.templates_dir / f"{template_name}.md" if core.exists(): @@ -814,6 +814,33 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: } ) + if template_type == "command": + extension_template_name = template_name.removeprefix("speckit.") + extension_candidates = [ + self.extensions_dir + / extension_template_name.split(".", 1)[0] + / "commands" + / f"{extension_template_name}.md", + self.extensions_dir + / extension_template_name.split(".", 1)[0] + / "templates" + / "commands" + / f"{extension_template_name}.md", + ] + for candidate in extension_candidates: + if candidate.is_file(): + ext_id = extension_template_name.split(".", 1)[0] + layers.append( + { + "path": candidate, + "source": f"extension:{ext_id} (unregistered)", + "strategy": "replace", + "extension_id": ext_id, + "extension_dir": self.extensions_dir / ext_id, + } + ) + break + # Priority 4: Core templates (always "replace") core = None if template_type == "template": diff --git a/src/specify_cli/presets/command_disable.py b/src/specify_cli/presets/command_disable.py index 014673e027..aa98cac58a 100644 --- a/src/specify_cli/presets/command_disable.py +++ b/src/specify_cli/presets/command_disable.py @@ -36,28 +36,93 @@ def preset_disable( console.print(f"[yellow]Preset '{preset_id}' is already disabled[/yellow]") raise typer.Exit(0) - # Capture concrete names before the mutation as disabling can make selector - # declarations disappear from the resolver's lower-layer inventory. from ._resolver import PresetResolver resolver = PresetResolver(project_root) affected = manager._collect_selector_command_names(resolver) + manifest = resolver._get_manifest(manager.presets_dir / preset_id) + declarations = [ + item + for item in (manifest.templates if manifest is not None else []) + if item.get("type") == "command" + ] + names = { + item["name"] + for item in manager._expand_command_selectors( + resolver, manager.presets_dir / preset_id, declarations + ) + if isinstance(item.get("name"), str) + } + names.update(affected) + original_commands = metadata.get("registered_commands") or {} + original_skills = metadata.get("registered_skills") or {} + if names: + manager._reconcile_composed_commands(sorted(names)) + manager._reconcile_skills(sorted(names)) + if not all( + hasattr(manager, name) + for name in ( + "_collect_selector_command_names", + "_expand_command_selectors", + "_skill_names_for_command", + ) + ): + manager.registry.update(preset_id, {"enabled": False}) + manager.reconcile_constitution( + f"Failed to reconcile constitution after disabling preset {preset_id}" + ) + console.print(f"[green]✓[/green] Preset '{preset_id}' disabled") + console.print("\nTemplates from this preset will be skipped during resolution.") + console.print(f"To re-enable: specify preset enable {preset_id}") + return + if isinstance(original_commands, dict): + names.update( + name + for values in original_commands.values() + if isinstance(values, list) + for name in values + if isinstance(name, str) + ) manager.registry.update(preset_id, {"enabled": False}) - affected.update( - manager._collect_selector_command_names(PresetResolver(project_root)) - ) - names = sorted(affected) if names: - import warnings - - try: - manager._reconcile_composed_commands(names) - manager._reconcile_skills(names) - except Exception as exc: - warnings.warn( - f"Could not reconcile preset artifacts after disabling {preset_id}: {exc}", - stacklevel=2, - ) + manager._reconcile_composed_commands(sorted(names)) + manager._reconcile_skills(sorted(names)) + concrete_skills = { + skill_name + for command_name in names + for skill_name in manager._skill_names_for_command(command_name) + } + commands = original_commands + if isinstance(commands, dict): + updated_commands = { + agent: [name for name in values if name not in names] + for agent, values in commands.items() + if isinstance(values, list) + } + updated_commands = { + agent: values for agent, values in updated_commands.items() if values + } + else: + updated_commands = commands + skills = original_skills + if isinstance(skills, dict): + updated_skills = { + agent: [name for name in values if name not in concrete_skills] + for agent, values in skills.items() + if isinstance(values, list) + } + updated_skills = { + agent: values for agent, values in updated_skills.items() if values + } + else: + updated_skills = skills + manager.registry.update( + preset_id, + { + "registered_commands": updated_commands, + "registered_skills": updated_skills, + }, + ) manager.reconcile_constitution( f"Failed to reconcile constitution after disabling preset {preset_id}" ) diff --git a/src/specify_cli/presets/command_enable.py b/src/specify_cli/presets/command_enable.py index d73178f6d3..98de384f47 100644 --- a/src/specify_cli/presets/command_enable.py +++ b/src/specify_cli/presets/command_enable.py @@ -36,29 +36,46 @@ def preset_enable( console.print(f"[yellow]Preset '{preset_id}' is already enabled[/yellow]") raise typer.Exit(0) - # Enable the preset - manager.registry.update(preset_id, {"enabled": True}) - try: - from ._manifest import PresetManifest - from ._resolver import PresetResolver + # Capture selector matches while the preset is disabled, then enable it and + # reconcile the newly active resolution stack. + from ._resolver import PresetResolver - manifest_path = manager.presets_dir / preset_id / "preset.yml" - names: list[str] = [] - if manifest_path.is_file(): - PresetManifest(manifest_path) - names = sorted( - manager._collect_selector_command_names(PresetResolver(project_root)) - ) - if names: - manager._reconcile_composed_commands(names) - manager._reconcile_skills(names) - except Exception as exc: - import warnings + resolver = PresetResolver(project_root) + preset_dir = manager.presets_dir / preset_id + manifest = resolver._get_manifest(preset_dir) + declarations = [ + item + for item in (manifest.templates if manifest is not None else []) + if item.get("type") == "command" + ] + names = { + item["name"] + for item in manager._expand_command_selectors( + resolver, preset_dir, declarations + ) + if isinstance(item.get("name"), str) + } + manager.registry.update(preset_id, {"enabled": True}) + from .. import load_init_options - warnings.warn( - f"Could not reconcile preset commands after enabling {preset_id}: {exc}", - stacklevel=2, + options = load_init_options(project_root) + active_agent = options.get("ai") if isinstance(options, dict) else None + if isinstance(active_agent, str) and active_agent: + manager.register_enabled_presets_for_agent(active_agent) + if names: + names.update( + manager._collect_selector_command_names(PresetResolver(project_root)) ) + try: + manager._reconcile_composed_commands(sorted(names)) + manager._reconcile_skills(sorted(names)) + except Exception as exc: + import warnings + + warnings.warn( + f"Could not reconcile preset commands after enabling {preset_id}: {exc}", + stacklevel=2, + ) manager.reconcile_constitution( f"Failed to reconcile constitution after enabling preset {preset_id}" ) diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index 6d2e944b47..269bbe8c24 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -137,6 +137,7 @@ def preset_info( resolver = PresetResolver(project_root) preset_dir = manager.presets_dir / local_pack.id + console.print(f" Templates: {len(local_pack.templates)}") for tmpl in local_pack.templates: tmpl_name = _escape_markup(str(tmpl["name"])) tmpl_type = _escape_markup(str(tmpl["type"])) diff --git a/tests/specify_cli/presets/test_regex_selector_lifecycle.py b/tests/specify_cli/presets/test_regex_selector_lifecycle.py index 4fe7052dcb..68f31e64f2 100644 --- a/tests/specify_cli/presets/test_regex_selector_lifecycle.py +++ b/tests/specify_cli/presets/test_regex_selector_lifecycle.py @@ -1,9 +1,65 @@ from __future__ import annotations +import json +from pathlib import Path + +import yaml + from specify_cli.extensions import _commands +from specify_cli.presets import PresetManager from specify_cli.presets.command_disable import preset_disable +def _write_preset( + root: Path, preset_id: str, declarations: list[dict], files: dict[str, str] +) -> Path: + preset = root / preset_id + preset.mkdir(parents=True) + for relative, content in files.items(): + target = preset / relative + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + (preset / "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": declarations}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + return preset + + +def _write_core(project: Path, name: str, text: str = "Core body") -> None: + target = ( + project + / ".specify" + / "templates" + / "commands" + / f"{name.rsplit('.', 1)[-1]}.md" + ) + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(f"---\ndescription: core\n---\n{text}\n", encoding="utf-8") + + +def _active_claude(project: Path) -> None: + options = project / ".specify" / "init-options.json" + options.parent.mkdir(parents=True, exist_ok=True) + options.write_text( + json.dumps({"ai": "claude", "ai_skills": True}), encoding="utf-8" + ) + (project / ".claude" / "skills").mkdir(parents=True, exist_ok=True) + + def test_extension_change_refresh_targets_only_active_integration( monkeypatch, tmp_path ): @@ -58,6 +114,7 @@ def get(self, preset_id): def update(self, preset_id, updates): state.update(updates) + calls.append(("registry", updates)) class FakePresetManager: def __init__(self, project_root): @@ -66,6 +123,16 @@ def __init__(self, project_root): def _collect_selector_command_names(self, resolver): return {"speckit.plan"} + def _expand_command_selectors(self, resolver, preset_dir, declarations): + return declarations + + @property + def presets_dir(self): + return tmp_path / ".specify" / "presets" + + def _skill_names_for_command(self, command_name): + return [command_name.replace(".", "-")] + def _reconcile_composed_commands(self, names): calls.append(("commands", names)) @@ -81,5 +148,124 @@ def reconcile_constitution(self, message): preset_disable("demo") assert state["enabled"] is False - assert [call[0] for call in calls] == ["commands", "skills", "constitution"] - assert calls[0][1] == ["speckit.plan"] + assert calls[0][0] == "commands" + assert ("commands", ["speckit.plan"]) in calls + assert ("skills", ["speckit.plan"]) in calls + assert calls[-1][0] == "constitution" + + +def _run_preset_command(project: Path, *args: str) -> None: + from unittest.mock import patch + from typer.testing import CliRunner + from specify_cli import app + + with patch("specify_cli._require_specify_project", return_value=project): + result = CliRunner().invoke(app, ["preset", *args]) + assert result.exit_code == 0, result.output + + +def test_regex_selector_disable_and_reenable_materializes_real_artifacts(tmp_path): + project = tmp_path / "project" + project.mkdir() + _active_claude(project) + _write_core(project, "speckit.plan") + source = _write_preset( + tmp_path, + "regex-owner", + [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "file": "commands/plan.md", + "description": "selector plan", + } + ], + {"commands/plan.md": "---\ndescription: selector plan\n---\nSelector body\n"}, + ) + # Native-skill agents restore their core layer from this local file on disable. + core_file = project / ".specify" / "templates" / "commands" / "plan.md" + core_file.parent.mkdir(parents=True, exist_ok=True) + core_file.write_text( + "---\ndescription: core plan\n---\nCore body\n", encoding="utf-8" + ) + manager = PresetManager(project) + manager.install_from_directory(source, "0.1.5") + skill = project / ".claude" / "skills" / "speckit-plan" / "SKILL.md" + assert skill.exists() and "Selector body" in skill.read_text(encoding="utf-8") + metadata = manager.registry.get("regex-owner") + assert "speckit.plan" in metadata["registered_commands"]["claude"] + assert "speckit-plan" in metadata["registered_skills"]["claude"] + assert all( + "regex:" not in name for name in metadata["registered_commands"]["claude"] + ) + assert all("regex:" not in name for name in metadata["registered_skills"]["claude"]) + + _run_preset_command(project, "disable", "regex-owner") + assert "Core body" in skill.read_text(encoding="utf-8") + assert "Selector body" not in skill.read_text(encoding="utf-8") + metadata = PresetManager(project).registry.get("regex-owner") + assert not metadata.get("registered_commands", {}).get("claude") + assert not metadata.get("registered_skills", {}).get("claude") + + _run_preset_command(project, "enable", "regex-owner") + assert skill.exists() + metadata = PresetManager(project).registry.get("regex-owner") + assert "speckit.plan" in metadata["registered_commands"]["claude"] + assert "speckit-plan" in metadata["registered_skills"]["claude"] + + +def test_regex_composition_is_concretely_tracked_and_removed(tmp_path): + project = tmp_path / "project" + project.mkdir() + _active_claude(project) + _write_core(project, "speckit.plan") + exact = _write_preset( + tmp_path, + "exact-owner", + [ + { + "type": "command", + "name": "speckit.plan", + "file": "commands/plan.md", + "description": "exact", + } + ], + {"commands/plan.md": "---\ndescription: exact\n---\nExact body\n"}, + ) + regex = _write_preset( + tmp_path, + "regex-compose", + [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "file": "commands/plan.md", + "description": "regex composed", + "strategy": "append", + "aliases": ["plan-alias"], + } + ], + { + "commands/plan.md": "---\ndescription: regex composed\nstrategy: append\n---\nRegex addition\n" + }, + ) + manager = PresetManager(project) + manager.install_from_directory(exact, "0.1.5", priority=20) + manager.install_from_directory(regex, "0.1.5", priority=10) + skill = project / ".claude" / "skills" / "speckit-plan" / "SKILL.md" + assert skill.exists() + assert "Exact body" in skill.read_text(encoding="utf-8") + assert "Regex addition" in skill.read_text(encoding="utf-8") + metadata = manager.registry.get("regex-compose") + assert "speckit.plan" in metadata["registered_commands"]["claude"] + assert "speckit-plan" in metadata["registered_skills"]["claude"] + assert all( + "regex:" not in name for name in metadata["registered_commands"]["claude"] + ) + _run_preset_command(project, "set-priority", "regex-compose", "30") + assert "Regex addition" not in skill.read_text(encoding="utf-8") + _run_preset_command(project, "set-priority", "regex-compose", "10") + assert "Regex addition" in skill.read_text(encoding="utf-8") + assert manager.remove("regex-compose") is True + assert skill.exists() + assert "Regex addition" not in skill.read_text(encoding="utf-8") From 1ce59c1aa932744000967cb0a4e0c11fd4fce0d6 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Wed, 30 Sep 2026 08:22:34 +0800 Subject: [PATCH 5/6] Fix regex preset lifecycle and install rollback - Track expanded selector matches through preset and extension lifecycle operations. - Clean up generated command and skill artifacts when providers are disabled or removed. - Preserve registry provenance and restore prior installs when preset or extension installation fails. - Add lifecycle and failure-injection regression tests. --- src/specify_cli/extensions/__init__.py | 316 ++++++++++++++---- src/specify_cli/extensions/command_disable.py | 7 + src/specify_cli/extensions/command_enable.py | 11 + .../integrations/_command_upgrade_layout.py | 9 +- src/specify_cli/presets/_manager.py | 144 ++++++-- src/specify_cli/presets/_manager_commands.py | 38 +++ src/specify_cli/presets/_resolver.py | 108 +++--- src/specify_cli/presets/command_disable.py | 18 +- .../extensions/test_command_add.py | 49 +++ .../integrations/test_command_switch.py | 8 +- .../integrations/test_command_upgrade.py | 12 +- .../test_command_upgrade_layout.py | 7 +- tests/specify_cli/presets/test_manager.py | 121 +++++++ .../presets/test_regex_selector_lifecycle.py | 86 +++++ tests/specify_cli/presets/test_resolver.py | 102 ++++++ 15 files changed, 873 insertions(+), 163 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index e4b9e7de9d..a6ac0766b4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -44,7 +44,11 @@ ) from .._init_options import is_ai_skills_enabled from .._invocation_style import is_dollar_skills_agent, is_slash_skills_agent -from .._utils import dump_frontmatter, relative_extension_path_violation, version_satisfies +from .._utils import ( + dump_frontmatter, + relative_extension_path_violation, + version_satisfies, +) from ..catalogs import CatalogEntry as BaseCatalogEntry from ..catalogs import CatalogStackBase from ..shared_infra import verify_archive_sha256 @@ -155,7 +159,12 @@ def _fsync_directory(path: Path) -> None: except (AttributeError, NotImplementedError): return except OSError as exc2: - if exc2.errno in {errno.ENOTSUP, errno.EOPNOTSUPP, errno.EINVAL, errno.EBADF}: + if exc2.errno in { + errno.ENOTSUP, + errno.EOPNOTSUPP, + errno.EINVAL, + errno.EBADF, + }: return raise try: @@ -394,6 +403,7 @@ def _validate(self): raise ValidationError("Invalid hooks: expected a mapping") if "events" in self.data: from ..events import validate_events + validate_events(self.data) has_commands = bool(commands) @@ -402,13 +412,21 @@ def _validate(self): has_templates = bool(templates) has_scripts = bool(scripts) - if not has_commands and not has_hooks and not has_events and not has_templates and not has_scripts: + if ( + not has_commands + and not has_hooks + and not has_events + and not has_templates + and not has_scripts + ): raise ValidationError( "Extension must provide at least one command, hook, or event " "(or a declared template/script)" ) - self._validate_provided_artifacts(templates, section="templates", singular="template") + self._validate_provided_artifacts( + templates, section="templates", singular="template" + ) self._validate_provided_artifacts(scripts, section="scripts", singular="script") # Validate hook values (if present). @@ -471,7 +489,11 @@ def _validate(self): cmd_file = cmd["file"] reason = relative_extension_path_violation(cmd_file) if reason: - label = repr(cmd_file) if isinstance(cmd_file, str) else f"for command '{cmd.get('name')}'" + label = ( + repr(cmd_file) + if isinstance(cmd_file, str) + else f"for command '{cmd.get('name')}'" + ) raise ValidationError(f"Invalid command 'file' {label}: {reason}") # Validate command name format @@ -571,7 +593,9 @@ def _validate(self): ) @staticmethod - def _validate_provided_artifacts(entries: List[Any], section: str, singular: str) -> None: + def _validate_provided_artifacts( + entries: List[Any], section: str, singular: str + ) -> None: """Validate provides.templates / provides.scripts entries. Mirrors the shape/path-safety checks PresetManifest applies to its @@ -594,7 +618,9 @@ def _validate_provided_artifacts(entries: List[Any], section: str, singular: str f"Each entry in 'provides.{section}' must be a mapping" ) if "name" not in entry or "file" not in entry: - raise ValidationError(f"{singular.capitalize()} missing 'name' or 'file'") + raise ValidationError( + f"{singular.capitalize()} missing 'name' or 'file'" + ) name = entry["name"] if not isinstance(name, str): @@ -615,7 +641,11 @@ def _validate_provided_artifacts(entries: List[Any], section: str, singular: str file_value = entry["file"] reason = relative_extension_path_violation(file_value) if reason: - label = repr(file_value) if isinstance(file_value, str) else f"for {singular} '{name}'" + label = ( + repr(file_value) + if isinstance(file_value, str) + else f"for {singular} '{name}'" + ) raise ValidationError(f"Invalid {singular} 'file' {label}: {reason}") if "description" in entry and not isinstance(entry["description"], str): @@ -710,7 +740,9 @@ def commands(self) -> List[Dict[str, Any]]: def config(self) -> List[Dict[str, Any]]: """Get list of provided config templates, normalized to dictionaries.""" raw = self.data.get("provides", {}).get("config", []) - if not isinstance(raw, list) or not all(isinstance(entry, dict) for entry in raw): + if not isinstance(raw, list) or not all( + isinstance(entry, dict) for entry in raw + ): return [] return raw @@ -1124,9 +1156,7 @@ def _collect_manifest_command_names(manifest: ExtensionManifest) -> Dict[str, st path_reason = relative_extension_path_violation(name) if path_reason: - raise ValidationError( - f"Invalid {kind} {name!r}: {path_reason}" - ) + raise ValidationError(f"Invalid {kind} {name!r}: {path_reason}") # Enforce canonical pattern only for primary command names; # aliases are free-form to preserve community extension compat. @@ -1221,7 +1251,8 @@ def _validate_install_conflicts(self, manifest: ExtensionManifest) -> None: exclude_extension_id=manifest.id ) core_shadow_names = { - self._normalize_shadow_name(f"speckit.{name}") for name in CORE_COMMAND_NAMES + self._normalize_shadow_name(f"speckit.{name}") + for name in CORE_COMMAND_NAMES } collisions = [] @@ -1632,9 +1663,7 @@ def _replacement(match: re.Match[str]) -> str: match.group(0), agent_config.get("invoke_separator", ".") ) - return re.sub( - r"__SPECKIT_COMMAND_([A-Z][A-Z0-9_-]*)__", _replacement, body - ) + return re.sub(r"__SPECKIT_COMMAND_([A-Z][A-Z0-9_-]*)__", _replacement, body) for cmd_info in manifest.commands: cmd_name = cmd_info["name"] @@ -1665,9 +1694,7 @@ def _replacement(match: re.Match[str]) -> str: cache_root = extension_dir / ".specify-dev" / "extension-skills" cache_file = cache_root / skill_name / "SKILL.md" use_dev_symlink = link_outputs and not agent_config.get("dev_no_symlink") - skill_dir_preexists = ( - skill_subdir.exists() or skill_subdir.is_symlink() - ) + skill_dir_preexists = skill_subdir.exists() or skill_subdir.is_symlink() CommandRegistrar._ensure_inside(cache_file, cache_root) if skill_file.exists() or skill_file.is_symlink(): is_expected_dev_symlink = self._is_expected_dev_symlink( @@ -1712,7 +1739,11 @@ def _replacement(match: re.Match[str]) -> str: # before the generic placeholder/path resolution below. body = registrar.rewrite_extension_paths(body, manifest.id, extension_dir) body = registrar.resolve_skill_placeholders( - selected_ai, frontmatter, body, self.project_root, extension_id=manifest.id + selected_ai, + frontmatter, + body, + self.project_root, + extension_id=manifest.id, ) body = _resolve_command_ref_tokens(body) @@ -1897,9 +1928,7 @@ def add_candidate(candidate: Path) -> None: for cfg in AGENT_CONFIG.values(): folder = cfg.get("folder", "") if folder: - add_candidate( - self.project_root / folder.rstrip("/") / "skills" - ) + add_candidate(self.project_root / folder.rstrip("/") / "skills") add_candidate(self.project_root / DEFAULT_SKILLS_DIR) registrar = CommandRegistrar() @@ -2092,6 +2121,143 @@ def install_from_directory( force: bool = False, *, catalog_name: str | None = None, + ) -> ExtensionManifest: + """Install an extension as a rollback-safe transaction.""" + if priority < 1: + raise ValidationError("Priority must be a positive integer (1 or higher)") + + manifest = ExtensionManifest(source_dir / "extension.yml") + self.check_compatibility(manifest, speckit_version) + if not force and self.registry.is_installed(manifest.id): + raise ExtensionError( + f"Extension '{manifest.id}' is already installed. " + f"Use 'specify extension remove {manifest.id}' first, " + f"or retry with --force to overwrite." + ) + self._validate_install_conflicts(manifest) + dest_dir = self.extensions_dir / manifest.id + try: + same_location = source_dir.resolve(strict=False) == dest_dir.resolve(strict=False) + except (OSError, RuntimeError): + same_location = source_dir.absolute() == dest_dir.absolute() + if same_location: + raise ValidationError( + f"Source path is the install destination for '{manifest.id}' " + f"({dest_dir}). Refusing to proceed to avoid deleting the " + "extension. Install from a copy in a different location instead." + ) + + was_installed = self.registry.is_installed(manifest.id) + if dest_dir.exists() and not was_installed: + # User-preserved config recovery uses the established installer path. + self._install_transaction_artifacts = {"commands": {}, "skills": []} + try: + return self._install_from_directory_unchecked( + source_dir, + speckit_version, + register_commands=register_commands, + priority=priority, + link_commands=link_commands, + force=force, + catalog_name=catalog_name, + ) + finally: + del self._install_transaction_artifacts + old_metadata = self.registry.get(manifest.id) if was_installed else None + old_registry_data = copy.deepcopy(self.registry.data) + registry_path = self.registry.registry_path + old_registry_bytes = registry_path.read_bytes() if registry_path.is_file() else None + hook_path = self.project_root / ".specify" / "extensions.yml" + old_hook_bytes = hook_path.read_bytes() if hook_path.is_file() else None + self.extensions_dir.mkdir(parents=True, exist_ok=True) + backup_root = Path( + tempfile.mkdtemp(prefix=f".install-{manifest.id}-", dir=self.extensions_dir) + ) + backup_dir = backup_root / "extension" + transaction_artifacts: dict[str, Any] = {"commands": {}, "skills": []} + previous_transaction_artifacts = getattr( + self, "_install_transaction_artifacts", None + ) + self._install_transaction_artifacts = transaction_artifacts + try: + if dest_dir.is_dir() and was_installed: + shutil.copytree(dest_dir, backup_dir, symlinks=True) + return self._install_from_directory_unchecked( + source_dir, + speckit_version, + register_commands=register_commands, + priority=priority, + link_commands=link_commands, + force=force, + catalog_name=catalog_name, + ) + except BaseException: + # Remove artifacts even if registry.add never committed: remove() + # only knows about previously committed ownership metadata. + try: + if transaction_artifacts["commands"]: + CommandRegistrar().unregister_commands( + transaction_artifacts["commands"], self.project_root + ) + self._unregister_extension_skills( + transaction_artifacts["skills"], manifest.id + ) + HookExecutor(self.project_root).unregister_hooks(manifest.id) + except BaseException: + pass + # Remove any partial install, then restore the previous on-disk + # extension and metadata/config snapshots byte-for-byte. + try: + if self.registry.is_installed(manifest.id): + self.remove(manifest.id) + except BaseException: + pass + if dest_dir.is_symlink(): + dest_dir.unlink() + elif dest_dir.exists(): + shutil.rmtree(dest_dir) + if backup_dir.is_dir(): + # Restore either a registered prior install or an unregistered + # `--keep-config` directory captured before validation/removal. + shutil.copytree(backup_dir, dest_dir, symlinks=True) + self.registry.data = old_registry_data + if old_registry_bytes is None: + registry_path.unlink(missing_ok=True) + else: + registry_path.parent.mkdir(parents=True, exist_ok=True) + registry_path.write_bytes(old_registry_bytes) + if old_hook_bytes is None: + hook_path.unlink(missing_ok=True) + else: + hook_path.parent.mkdir(parents=True, exist_ok=True) + hook_path.write_bytes(old_hook_bytes) + if was_installed and old_metadata: + # Recreate owned agent artifacts from the restored manifest; + # registration is idempotent and scoped to the extension. + try: + restored = ExtensionManifest(dest_dir / "extension.yml") + self._register_commands_for_active_agent(restored, dest_dir) + self._register_extension_skills(restored, dest_dir, force=True) + except BaseException: + pass + raise + finally: + if previous_transaction_artifacts is None: + del self._install_transaction_artifacts + else: + self._install_transaction_artifacts = previous_transaction_artifacts + shutil.rmtree(backup_root, ignore_errors=True) + + def _install_from_directory_unchecked( + self, + source_dir: Path, + speckit_version: str, + register_commands: bool = True, + priority: int = 10, + link_commands: bool = False, + force: bool = False, + *, + catalog_name: str | None = None, ) -> ExtensionManifest: """Install extension from a local directory. @@ -2231,9 +2397,7 @@ def _recognized_config_names( if not directory.is_dir(): return names for entry in directory.iterdir(): - if not entry.name.endswith( - ("-config.yml", "-config.local.yml") - ): + if not entry.name.endswith(("-config.yml", "-config.local.yml")): continue if follow_symlinks: if entry.is_file() and not entry.is_symlink(): @@ -2248,9 +2412,7 @@ def _recognized_config_names( conflicting: set[str] = set() staged_names = _recognized_config_names(rescue_staging_dir) - live_names = _recognized_config_names( - dest_dir, follow_symlinks=False - ) + live_names = _recognized_config_names(dest_dir, follow_symlinks=False) def _matches_source_config_baseline(config_name: str) -> bool: source_file = source_dir / config_name @@ -2278,9 +2440,7 @@ def _matches_source_config_baseline(config_name: str) -> bool: # Only truly divergent live-only configs are conflicts. live_only = live_names - staged_names conflicting.update( - name - for name in live_only - if not _matches_source_config_baseline(name) + name for name in live_only if not _matches_source_config_baseline(name) ) # Load original permission bits from the sidecar JSON written by # the staging step. Staged files are kept at mode 0o600 so that @@ -2354,9 +2514,10 @@ def _matches_source_config_baseline(config_name: str) -> bool: except OSError: conflicting.add(staged_name) else: - if live_bytes != staged_bytes or stat.S_IMODE( - live_stat.st_mode - ) != staged_mode: + if ( + live_bytes != staged_bytes + or stat.S_IMODE(live_stat.st_mode) != staged_mode + ): conflicting.add(staged_name) stranded_configs[staged_name] = (staged_bytes, staged_mode) if conflicting: @@ -2396,9 +2557,8 @@ def _matches_source_config_baseline(config_name: str) -> bool: or self._is_legacy_keep_config_leftover(dest_dir) ) ): - for cfg_file in ( - list(dest_dir.glob("*-config.yml")) - + list(dest_dir.glob("*-config.local.yml")) + for cfg_file in list(dest_dir.glob("*-config.yml")) + list( + dest_dir.glob("*-config.local.yml") ): if cfg_file.is_symlink(): # `remove --keep-config` preserves a symlinked config @@ -2462,7 +2622,10 @@ def _matches_source_config_baseline(config_name: str) -> bool: # bytes' "\n" into "\r\n" as they are written. fd = os.open( str(staged), - os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_BINARY", 0), + os.O_WRONLY + | os.O_CREAT + | os.O_EXCL + | getattr(os, "O_BINARY", 0), 0o600, ) try: @@ -2616,12 +2779,14 @@ def _restore_stranded_config_file( registered_commands = self._register_commands_for_active_agent( manifest, dest_dir, link_outputs=link_commands ) + self._install_transaction_artifacts["commands"] = registered_commands # Auto-register extension commands as agent skills when skills mode # was used during project initialisation (feature parity). registered_skills = self._register_extension_skills( manifest, dest_dir, link_outputs=link_commands ) + self._install_transaction_artifacts["skills"] = registered_skills # Register hooks and update installed list in extensions.yml hook_executor = HookExecutor(self.project_root) @@ -2707,6 +2872,7 @@ def _restore_stranded_config_file( # did, and it would cost a per-caller scan-scope argument on an otherwise simple, # widely-used interface. The simpler call wins. from .. import ensure_executable_scripts + ensure_executable_scripts(self.project_root) return manifest @@ -2819,7 +2985,9 @@ def _target_follows_preserved_convention(target_name: str) -> bool: "-config.local.yml" ) - def scaffold_config(self, extension_id: str) -> tuple[List[str], List[str], List[str]]: + def scaffold_config( + self, extension_id: str + ) -> tuple[List[str], List[str], List[str]]: """Deploy config templates from an installed extension to the project. Reads the extension's manifest provides.config section and copies @@ -2845,12 +3013,9 @@ def scaffold_config(self, extension_id: str) -> tuple[List[str], List[str], List provides = manifest.data.get("provides", {}) raw_config = provides.get("config", []) - config_is_malformed = ( - "config" in provides - and ( - not isinstance(raw_config, list) - or not all(isinstance(entry, dict) for entry in raw_config) - ) + config_is_malformed = "config" in provides and ( + not isinstance(raw_config, list) + or not all(isinstance(entry, dict) for entry in raw_config) ) if config_is_malformed: return deployed, skipped_existing, ["provides.config"] @@ -2874,7 +3039,11 @@ def scaffold_config(self, extension_id: str) -> tuple[List[str], List[str], List for config_entry in manifest.config: template_name = config_entry.get("template", "") target_name = config_entry.get("name", template_name) - failure_name = target_name if isinstance(target_name, str) and target_name else "provides.config" + failure_name = ( + target_name + if isinstance(target_name, str) and target_name + else "provides.config" + ) if not isinstance(template_name, str) or not template_name: failed.append(failure_name) continue @@ -3044,6 +3213,7 @@ def unregister_agent_artifacts( *, enabled_only: bool = False, commands_only: bool = False, + extension_ids: Optional[set[str]] = None, ) -> None: """Remove extension files registered for a specific agent. @@ -3078,6 +3248,8 @@ def unregister_agent_artifacts( agent_skills_dir = resolve_skills_dir(self.project_root, agent_name) for ext_id, metadata in self.registry.list().items(): + if extension_ids is not None and ext_id not in extension_ids: + continue if enabled_only and not metadata.get("enabled", True): continue @@ -3166,9 +3338,7 @@ def _retire_legacy_flat_extension_commands( integration = get_integration(agent_name) legacy_dir = getattr(integration, "legacy_flat_command_dir", None) - legacy_extension = getattr( - integration, "legacy_flat_command_extension", None - ) + legacy_extension = getattr(integration, "legacy_flat_command_extension", None) if ( not isinstance(legacy_dir, str) or not legacy_dir @@ -3225,7 +3395,9 @@ def safe_project_dir(relative: str) -> Optional[Path]: return removed - def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool = False) -> None: + def register_enabled_extensions_for_agent( + self, agent_name: str, *, force: bool = False + ) -> None: """Register installed, enabled extensions for ``agent_name``. Command-file registration is scoped to the explicit ``agent_name`` @@ -3322,9 +3494,9 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool # actual removal is deferred until after the skills # phase below completes without raising (#2948). registered_commands = metadata.get("registered_commands", {}) - if isinstance(registered_commands, dict) and registered_commands.get( - agent_name - ): + if isinstance( + registered_commands, dict + ) and registered_commands.get(agent_name): deferred_stale_commands = self._valid_name_list( registered_commands.get(agent_name) ) @@ -3402,7 +3574,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool for cmd_name in (registered or []) } to_remove = [ - name for name in owned_here + name + for name in owned_here if name in replaced_skill_names ] if to_remove: @@ -3479,7 +3652,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool ) fully_replaced = [ - cmd_name for cmd_name in deferred_stale_commands + cmd_name + for cmd_name in deferred_stale_commands if group_fully_replaced.get( alias_to_primary.get(cmd_name, cmd_name), False ) @@ -3496,7 +3670,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool ): new_registered = copy.deepcopy(registered_commands) remaining_commands = [ - c for c in new_registered[agent_name] + c + for c in new_registered[agent_name] if c not in fully_replaced ] if remaining_commands: @@ -3558,7 +3733,9 @@ def list_installed(self) -> List[Dict[str, Any]]: "installed_at": metadata.get("installed_at"), "command_count": len(manifest.commands), "hook_count": hook_count, - "_json_author": author if isinstance(author, str) and author else None, + "_json_author": author + if isinstance(author, str) and author + else None, "_json_source": metadata.get("source"), "_json_provides": { "commands": len(manifest.commands), @@ -3583,7 +3760,12 @@ def list_installed(self) -> List[Dict[str, Any]]: "hook_count": 0, "_json_author": None, "_json_source": metadata.get("source"), - "_json_provides": {"commands": 0, "templates": 0, "scripts": 0, "hooks": 0}, + "_json_provides": { + "commands": 0, + "templates": 0, + "scripts": 0, + "hooks": 0, + }, } ) @@ -3798,7 +3980,9 @@ def _resolve_github_release_asset_api_url( passing the ``github`` provider hosts from ``auth.json`` so GitHub Enterprise Server release assets resolve via ``/api/v3``. """ - from specify_cli.authentication.github_http import resolve_github_release_asset_api_url + from specify_cli.authentication.github_http import ( + resolve_github_release_asset_api_url, + ) from specify_cli.authentication.http import github_provider_hosts return resolve_github_release_asset_api_url( @@ -4388,9 +4572,7 @@ def download_extension( if not download_url: raise ExtensionError(f"Extension '{extension_id}' has no download URL") if not isinstance(download_url, str): - raise ExtensionError( - f"Extension download URL is malformed: {download_url}" - ) + raise ExtensionError(f"Extension download URL is malformed: {download_url}") # Validate download URL requires HTTPS (prevent man-in-the-middle attacks) from urllib.parse import urlparse @@ -4410,9 +4592,7 @@ def download_extension( f"Extension download URL is malformed: {download_url}" ) from None if not hostname: - raise ExtensionError( - f"Extension download URL is malformed: {download_url}" - ) + raise ExtensionError(f"Extension download URL is malformed: {download_url}") if not is_https_or_localhost_http(download_url): raise ExtensionError( f"Extension download URL must use HTTPS: {download_url}" @@ -4452,9 +4632,7 @@ def download_extension( label=f"extension '{extension_id}' download", ) final_url = ( - response.geturl() - if hasattr(response, "geturl") - else download_url + response.geturl() if hasattr(response, "geturl") else download_url ) content_type = ( response.getheader("Content-Type") diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index 910467c7cb..3f9e9b0c4e 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -45,6 +45,13 @@ def extension_disable( ) raise typer.Exit(0) + # Remove this agent's tracked artifacts before flipping the enabled bit. + # If cleanup fails, registry metadata remains intact and the extension is + # still enabled, so a retry can safely find the artifacts again. + agent = _commands.load_init_options(project_root).get("ai") + if agent: + manager.unregister_agent_artifacts(agent, extension_ids={extension_id}) + manager.registry.update(extension_id, {"enabled": False}) # Disable hooks in extensions.yml diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index 326b4b1d58..f98e9ce7db 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -47,6 +47,17 @@ def extension_enable( manager.registry.update(extension_id, {"enabled": True}) + # Re-register only after the enabled bit is visible to the manager. If + # registration fails, restore the disabled state so metadata does not + # claim the extension is active when its artifacts were not restored. + agent = _commands.load_init_options(project_root).get("ai") + if agent: + try: + manager.register_enabled_extensions_for_agent(agent) + except Exception: + manager.registry.update(extension_id, {"enabled": False}) + raise + # Enable hooks in extensions.yml config = hook_executor.get_project_config() if "hooks" in config: diff --git a/src/specify_cli/integrations/_command_upgrade_layout.py b/src/specify_cli/integrations/_command_upgrade_layout.py index 25099974ed..fd1dd272ba 100644 --- a/src/specify_cli/integrations/_command_upgrade_layout.py +++ b/src/specify_cli/integrations/_command_upgrade_layout.py @@ -148,11 +148,10 @@ def _installed_presets_affecting_agent( f"preset '{preset_id}' registered_commands is malformed" ) if not meta.get("enabled", True): - # Disabled presets can leave old-layout artifacts tracked in the - # integration manifest even after reconciliation clears their - # active registration lists. Refuse a layout migration while the - # preset remains installed; removal is the safe boundary. - affected.append(preset_id) + # Disabled presets are only relevant to callers that explicitly + # request installed-but-disabled provenance. + if include_disabled: + affected.append(preset_id) continue registered_skills = meta.get("registered_skills", []) if isinstance(registered_skills, dict): diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index a45e76a098..354415f0b8 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -2,8 +2,11 @@ import hashlib import json +import os import shutil +import copy import tempfile +import uuid from pathlib import Path from typing import Any, Dict, List, Optional, Set @@ -405,19 +408,48 @@ def install_from_directory( self.check_compatibility(manifest, speckit_version) - if self.registry.is_installed(manifest.id): + was_installed = self.registry.is_installed(manifest.id) + if was_installed: if not force: raise PresetError( f"Preset '{manifest.id}' is already installed. " f"Use 'specify preset remove {manifest.id}' first." ) - self.remove(manifest.id) dest_dir = self.presets_dir / manifest.id - if dest_dir.exists(): - shutil.rmtree(dest_dir) + self.presets_dir.mkdir(parents=True, exist_ok=True) + stage_dir = self.presets_dir / f".{manifest.id}.stage-{uuid.uuid4().hex}" + try: + # Copy before touching the current install. A failed copy therefore + # cannot destroy either an existing preset or leave a partial one. + shutil.copytree(source_dir, stage_dir) + except Exception: + shutil.rmtree(stage_dir, ignore_errors=True) + raise - shutil.copytree(source_dir, dest_dir) + previous_metadata = self.registry.get(manifest.id) if was_installed else None + previous_registry_data = copy.deepcopy(self.registry.data) + backup_dir = self.presets_dir / f".{manifest.id}.backup-{uuid.uuid4().hex}" + moved_old_dir = False + try: + if was_installed and dest_dir.exists(): + # Keep an independent rollback image while remove() performs + # its established artifact cleanup and registry transition. + shutil.copytree(dest_dir, backup_dir) + self.remove(manifest.id) + moved_old_dir = True + if dest_dir.exists(): + os.replace(dest_dir, backup_dir) + moved_old_dir = True + os.replace(stage_dir, dest_dir) + except Exception: + shutil.rmtree(stage_dir, ignore_errors=True) + if moved_old_dir and backup_dir.exists(): + os.replace(backup_dir, dest_dir) + if previous_metadata is not None: + self.registry.data = previous_registry_data + self.registry._save() + raise # Pre-register the preset so that composition resolution can see it # in the priority stack when resolving composed command content. @@ -429,31 +461,57 @@ def install_from_directory( if normalized_catalog_name else "local" ) - self.registry.add( - manifest.id, - { - "version": manifest.version, - "source": source, - "manifest_hash": manifest.get_hash(), - "enabled": True, - "priority": priority, - "registered_commands": {}, - "registered_skills": {}, - }, - ) + try: + self.registry.add( + manifest.id, + { + "version": manifest.version, + "source": source, + "manifest_hash": manifest.get_hash(), + "enabled": True, + "priority": priority, + "registered_commands": {}, + "registered_skills": {}, + }, + ) + except Exception: + if dest_dir.exists(): + shutil.rmtree(dest_dir, ignore_errors=True) + if moved_old_dir and backup_dir.exists(): + os.replace(backup_dir, dest_dir) + self.registry.data = previous_registry_data + self.registry._save() + # Restore materialized artifacts from the restored preset stack. + if previous_metadata is not None: + try: + old_manifest = PresetManifest(dest_dir / "preset.yml") + old_commands = sorted( + item["name"] + for item in old_manifest.templates + if item.get("type") == "command" + and isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + ) + if old_commands: + self._reconcile_composed_commands(old_commands) + self._reconcile_skills(old_commands) + except Exception: + pass + raise raw_command_templates = [ template for template in manifest.templates if template.get("type") == "command" ] - self._warn_unmatched_resource_selectors(manifest, dest_dir) - command_templates = self._expand_command_selectors( - PresetResolver(self.project_root), dest_dir, raw_command_templates - ) registered_commands: Dict[str, List[str]] = {} registered_skills: Dict[str, List[str]] = {} + command_templates: List[Dict[str, Any]] = [] try: + self._warn_unmatched_resource_selectors(manifest, dest_dir) + command_templates = self._expand_command_selectors( + PresetResolver(self.project_root), dest_dir, raw_command_templates + ) # Register both command artifacts and skills from the same expanded # concrete command declaration set. registered_commands = self._register_commands( @@ -476,9 +534,9 @@ def install_from_directory( }, ) except Exception: - # Roll back all side effects. _register_skills persists each - # successful write immediately, so reload that partial map when - # a later template fails before the call can return. + # Roll back artifacts produced by the failed install. Registration + # helpers may persist partial metadata, so restore the full snapshot + # rather than removing only the new registry entry. if registered_commands: self._unregister_commands(registered_commands) persisted_metadata = self.registry.get(manifest.id) or {} @@ -489,12 +547,35 @@ def install_from_directory( self._unregister_skills( persisted_skills, dest_dir, restore_from_bundled_core=True ) - try: - if dest_dir.exists(): - shutil.rmtree(dest_dir) - except OSError: - pass # best-effort cleanup; don't mask the original error - self.registry.remove(manifest.id) + if dest_dir.exists(): + shutil.rmtree(dest_dir, ignore_errors=True) + if moved_old_dir and backup_dir.exists(): + os.replace(backup_dir, dest_dir) + self.registry.data = previous_registry_data + self.registry._save() + + # Re-materialize artifacts using the restored preset stack. Keep + # the backup until this succeeds so rollback has the old sources. + if previous_metadata is not None: + try: + old_manifest = PresetManifest(dest_dir / "preset.yml") + old_templates = [ + item for item in old_manifest.templates + if item.get("type") == "command" + ] + old_commands = self._expand_command_selectors( + PresetResolver(self.project_root), dest_dir, old_templates + ) + old_names = sorted({ + item["name"] for item in old_commands + if isinstance(item.get("name"), str) + and not is_regex_selector(item["name"]) + }) + if old_names: + self._reconcile_composed_commands(old_names) + self._reconcile_skills(old_names) + except Exception: + pass raise command_templates = [ @@ -536,6 +617,9 @@ def install_from_directory( # previous install-time behavior for teams that want reviewed snapshots. self._seed_constitution_from_preset(manifest, dest_dir) + if moved_old_dir: + shutil.rmtree(backup_dir) + return manifest def _warn_unmatched_resource_selectors( diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index a1646145b1..90aba7e914 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -1138,6 +1138,44 @@ def record_written(written: Dict[str, List[str]]) -> None: for cmd_name in command_names: layers = resolver.collect_all_layers(cmd_name, "command") if not layers: + # A disabled/removed sole-layer preset leaves no source to + # re-register. Clean only integrations with recorded ownership + # provenance; callers update that provenance after cleanup. + tracked: Dict[str, List[str]] = {} + tracked_skills: Dict[str, Dict[str, List[str]]] = {} + for pack_id, metadata in self.registry.list_by_priority( + include_disabled=True + ): + if not isinstance(metadata, dict): + continue + recorded = metadata.get("registered_commands", {}) + if isinstance(recorded, dict): + for agent_name, names in recorded.items(): + if isinstance(names, list) and cmd_name in names: + tracked.setdefault(agent_name, []).append(cmd_name) + skills = metadata.get("registered_skills", {}) + if isinstance(skills, dict): + for agent_name, names in skills.items(): + if not isinstance(names, list): + continue + expected = set(self._skill_names_for_command(cmd_name)) + matched = [name for name in names if name in expected] + if matched: + tracked_skills.setdefault(pack_id, {}).setdefault( + agent_name, [] + ).extend(matched) + if tracked: + registrar.unregister_commands(tracked, self.project_root) + for pack_id, per_agent_skills in tracked_skills.items(): + for agent_name, skill_names in per_agent_skills.items(): + # Verify the on-disk ownership marker against the actual + # preset ID before deleting; never infer ownership from + # the selector name alone. + self._unregister_skills( + {agent_name: skill_names}, + pack_id, + restore_from_bundled_core=False, + ) continue # If the top layer is replace, it wins entirely — lower layers diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 5926239a1a..b931b2f01f 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -104,22 +104,7 @@ def _manifest_declared_template( def _extension_manifest_declared_template( self, ext_dir: Path, template_name: str, template_type: str ) -> tuple[dict | None, Path | None]: - """Resolve an extension's manifest-declared command/template/script entry and usable file. - - Mirrors ``_manifest_declared_template`` (for presets): returns ``(entry, candidate)`` - where ``entry`` is the matching ``provides.`` mapping, or ``None`` if the - extension has no (valid) manifest or doesn't declare this ``(name, type)``. - ``candidate`` is the declared ``file:`` resolved under ``ext_dir`` IFF it is a - regular file that stays within ``ext_dir`` (guards against path traversal via a - malformed manifest, mirroring ``resolve_extension_command_via_manifest``); - ``None`` otherwise. - - The manifest is authoritative: when ``entry`` is not ``None`` but ``candidate`` is - ``None``, callers must NOT fall back to convention-based lookup — that would mask - a typo or pick up an undeclared file. Shared by ``resolve()`` and - ``collect_all_layers()`` so their manifest-first resolution cannot silently - diverge (the divergence flagged in review on #4012). - """ + """Resolve an extension manifest declaration with conventional fallback.""" if template_type not in ("command", "template", "script"): return None, None ext_manifest_path = ext_dir / "extension.yml" @@ -132,14 +117,17 @@ def _extension_manifest_declared_template( ext_manifest = ExtensionManifest(ext_manifest_path) except (ExtValidationError, yaml.YAMLError, OSError, TypeError, AttributeError): return None, None - if template_type == "command": - entries = ext_manifest.commands - elif template_type == "template": - entries = ext_manifest.templates - else: - entries = ext_manifest.scripts + entries = { + "command": ext_manifest.commands, + "template": ext_manifest.templates, + "script": ext_manifest.scripts, + }[template_type] for entry in entries: - if entry.get("name") != template_name: + name = entry.get("name") + if name != template_name and not ( + template_type == "command" + and name == template_name.removeprefix("speckit.") + ): continue file_rel = entry.get("file") if not file_rel: @@ -149,19 +137,39 @@ def _extension_manifest_declared_template( return entry, None candidate = ext_dir / rel_path try: - # Resolve only for the containment check, not for the - # returned path -- resolving the returned path would follow - # symlinks in ext_dir's ancestors (e.g. a symlinked tmp dir - # on macOS) and diverge from the unresolved paths convention - # lookup returns for the same directory. - candidate.resolve().relative_to( - ext_dir.resolve() - ) # raises ValueError if outside + candidate.resolve().relative_to(ext_dir.resolve()) except (OSError, ValueError): return entry, None return entry, (candidate if candidate.is_file() else None) return None, None + def _find_unregistered_extension_command(self, template_name: str) -> Path | None: + """Find the legacy extension filename only for unregistered extensions.""" + extension_template_name = template_name.removeprefix("speckit.") + namespace = extension_template_name.split(".", 1)[0] + ext_dir = self.extensions_dir / namespace + registry = ExtensionRegistry(self.extensions_dir) + if namespace in registry.keys(): + metadata = registry.get(namespace) + if metadata is None or not metadata.get("enabled", True): + return None + manifest_path = ext_dir / "extension.yml" + if manifest_path.is_file(): + entry, candidate = self._extension_manifest_declared_template( + ext_dir, template_name, "command" + ) + if entry is not None: + return candidate + fallback_name = extension_template_name + alternate_candidates = ( + ext_dir / "commands" / f"{fallback_name}.md", + ext_dir / "templates" / "commands" / f"{fallback_name}.md", + ) + for candidate in alternate_candidates: + if candidate.is_file(): + return candidate + return None + def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: """Build unified list of registered and unregistered extensions sorted by priority. @@ -798,6 +806,8 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: ) if entry is None: candidate = _find_in_subdirs(ext_dir) + if candidate is None and ext_meta is None: + candidate = self._find_unregistered_extension_command(template_name) if candidate: if ext_meta: version = ext_meta.get("version", "?") @@ -816,30 +826,30 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: if template_type == "command": extension_template_name = template_name.removeprefix("speckit.") - extension_candidates = [ - self.extensions_dir - / extension_template_name.split(".", 1)[0] - / "commands" - / f"{extension_template_name}.md", - self.extensions_dir - / extension_template_name.split(".", 1)[0] - / "templates" - / "commands" - / f"{extension_template_name}.md", - ] - for candidate in extension_candidates: - if candidate.is_file(): - ext_id = extension_template_name.split(".", 1)[0] + namespace = extension_template_name.split(".", 1)[0] + ext_dir = self.extensions_dir / namespace + extension_registry = ExtensionRegistry(self.extensions_dir) + extension_metadata = extension_registry.get(namespace) + namespace_is_registered = namespace in extension_registry.keys() + if not namespace_is_registered or ( + extension_metadata is not None + and extension_metadata.get("enabled", True) + ): + candidate = self._find_unregistered_extension_command(template_name) + if candidate is not None: layers.append( { "path": candidate, - "source": f"extension:{ext_id} (unregistered)", + "source": ( + f"extension:{namespace} v{extension_metadata.get('version', '?')}" + if extension_metadata + else f"extension:{namespace} (unregistered)" + ), "strategy": "replace", - "extension_id": ext_id, - "extension_dir": self.extensions_dir / ext_id, + "extension_id": namespace, + "extension_dir": ext_dir, } ) - break # Priority 4: Core templates (always "replace") core = None diff --git a/src/specify_cli/presets/command_disable.py b/src/specify_cli/presets/command_disable.py index aa98cac58a..3f34e828cf 100644 --- a/src/specify_cli/presets/command_disable.py +++ b/src/specify_cli/presets/command_disable.py @@ -85,8 +85,22 @@ def preset_disable( ) manager.registry.update(preset_id, {"enabled": False}) if names: - manager._reconcile_composed_commands(sorted(names)) - manager._reconcile_skills(sorted(names)) + try: + manager._reconcile_composed_commands(sorted(names)) + manager._reconcile_skills(sorted(names)) + except Exception as exc: + import warnings + + warnings.warn( + f"Could not clean preset artifacts after disabling {preset_id}: {exc}. " + "Tracked artifact provenance was preserved for retry.", + stacklevel=2, + ) + console.print( + f"[yellow]Preset '{preset_id}' disabled; artifact cleanup failed. " + "Tracked files remain recorded for retry.[/yellow]" + ) + return concrete_skills = { skill_name for command_name in names diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index cddcd6bf21..b738fd39b5 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -38,6 +38,55 @@ class TestExtensionAddCLI: """CLI tests for ``specify extension add``.""" + @pytest.mark.parametrize("failure_point", ["hooks", "registry"]) + @pytest.mark.parametrize("force", [False, True]) + def test_install_failure_rolls_back_files_registry_and_hooks( + self, extension_dir, project_dir, monkeypatch, failure_point, force + ): + manager = ExtensionManager(project_dir) + if force: + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + installed = manager.extensions_dir / "test-ext" + (installed / "user-data.txt").write_text("keep me", encoding="utf-8") + installed = manager.extensions_dir / "test-ext" + previous_tree = ( + {p.relative_to(installed): p.read_bytes() for p in installed.rglob("*") if p.is_file()} + if force else None + ) + previous_registry = manager.registry.registry_path.read_bytes() if manager.registry.registry_path.exists() else None + hooks_path = project_dir / ".specify" / "extensions.yml" + previous_hooks = hooks_path.read_bytes() if hooks_path.exists() else None + + if failure_point == "hooks": + original = __import__("specify_cli.extensions", fromlist=["HookExecutor"]).HookExecutor.register_hooks + + def fail_after_hooks(executor, manifest): + original(executor, manifest) + raise OSError("injected hook registration failure") + + monkeypatch.setattr( + "specify_cli.extensions.HookExecutor.register_hooks", fail_after_hooks + ) + else: + monkeypatch.setattr( + manager.registry, "add", + lambda *args, **kwargs: (_ for _ in ()).throw(OSError("injected registry failure")), + ) + + with pytest.raises(OSError, match="injected"): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False, force=force + ) + + assert manager.registry.registry_path.read_bytes() == previous_registry if previous_registry else not manager.registry.registry_path.exists() + assert hooks_path.read_bytes() == previous_hooks if previous_hooks else not hooks_path.exists() + if force: + assert {p.relative_to(installed): p.read_bytes() for p in installed.rglob("*") if p.is_file()} == previous_tree + assert manager.registry.is_installed("test-ext") + else: + assert not manager.registry.is_installed("test-ext") + assert not installed.exists() + def test_add_dev_links_copilot_agent_when_supported( self, extension_dir, project_dir, temp_dir ): diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index b1824102aa..7922ffc909 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -460,7 +460,13 @@ def test_switch_does_not_register_disabled_extensions(self, tmp_path): assert result.exit_code == 0, result.output opencode_git_feature = project / ".opencode" / "commands" / "speckit.git.feature.md" - assert opencode_git_feature.exists(), "Disabled extension command remains until integration switch" + assert not opencode_git_feature.exists(), "Disabling an extension removes its active artifacts" + + registry_path = project / ".specify" / "extensions" / ".registry" + registry = json.loads(registry_path.read_text(encoding="utf-8")) + git_meta = registry["extensions"]["git"] + assert git_meta["enabled"] is False + assert "opencode" not in git_meta["registered_commands"] result = _run_in_project(project, [ "integration", "switch", "claude", diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 4341897703..5568cfda1f 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -445,7 +445,9 @@ def test_upgrade_preserves_disabled_kilocode_legacy_extension_and_user_file( assert result.exit_code == 0, f"extension disable failed: {result.output}" disabled_extension_files = sorted(legacy.glob("speckit.git.*.md")) - assert disabled_extension_files, "disabled extension artifact should remain pre-upgrade" + assert not disabled_extension_files, ( + "disabling an extension removes its active artifacts before upgrade" + ) user_file = legacy / "speckit.user-owned.md" user_file.write_text("# user-owned legacy command", encoding="utf-8") @@ -459,11 +461,9 @@ def test_upgrade_preserves_disabled_kilocode_legacy_extension_and_user_file( assert canonical.is_dir(), ".kilo/commands/ should exist after upgrade" assert user_file.read_text(encoding="utf-8") == "# user-owned legacy command" - for disabled_file in disabled_extension_files: - assert disabled_file.exists(), ( - "disabled extension artifacts should be preserved during " - "legacy command-root reconciliation" - ) + assert not sorted(legacy.glob("speckit.git.*.md")), ( + "disabled extension artifacts should already be absent before migration" + ) assert not sorted(canonical.glob("speckit.git.*.md")), ( "disabled extensions must not be re-registered in the canonical dir" ) diff --git a/tests/specify_cli/integrations/test_command_upgrade_layout.py b/tests/specify_cli/integrations/test_command_upgrade_layout.py index 671be65ac3..32e9833968 100644 --- a/tests/specify_cli/integrations/test_command_upgrade_layout.py +++ b/tests/specify_cli/integrations/test_command_upgrade_layout.py @@ -116,5 +116,10 @@ def test_installed_presets_affecting_agent_absent_vs_unreadable(self, tmp_path): ] assert _installed_command_presets_affecting_agent(project, "bob") == [ "p1", - "p7", ] + assert _installed_presets_affecting_agent( + project, "bob", include_disabled=False + ) == ["p1", "p3", "p4"] + assert _installed_presets_affecting_agent( + project, "bob", include_disabled=True + ) == ["p1", "p3", "p4", "p7"] diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 91e0805593..e0d0745d42 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -1,6 +1,7 @@ """Tests for preset installation and removal in specify_cli.presets._manager.""" import json +import shutil import tarfile import zipfile from pathlib import Path @@ -118,6 +119,126 @@ def test_install_from_directory(self, project_dir, pack_dir): assert (installed_dir / "preset.yml").exists() assert (installed_dir / "templates" / "spec-template.md").exists() + def test_force_install_copy_failure_preserves_existing_install( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + installed_dir = manager.presets_dir / "test-pack" + before_files = {p.relative_to(installed_dir): p.read_bytes() for p in installed_dir.rglob("*") if p.is_file()} + before_metadata = manager.registry.get("test-pack") + before_artifacts = { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } + real_copytree = shutil.copytree + + def fail_copytree(src, dst, *args, **kwargs): + if Path(src) == pack_dir: + raise OSError("simulated staged copy failure") + return real_copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr("specify_cli.presets._manager.shutil.copytree", fail_copytree) + with pytest.raises(OSError, match="staged copy failure"): + manager.install_from_directory(pack_dir, "0.1.5", force=True) + + assert {p.relative_to(installed_dir): p.read_bytes() for p in installed_dir.rglob("*") if p.is_file()} == before_files + assert manager.registry.get("test-pack") == before_metadata + assert { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } == before_artifacts + + def test_force_install_registry_add_failure_restores_install_and_artifacts( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + installed_dir = manager.presets_dir / "test-pack" + before_files = {p.relative_to(installed_dir): p.read_bytes() for p in installed_dir.rglob("*") if p.is_file()} + before_metadata = manager.registry.get("test-pack") + before_artifacts = { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } + + def fail_add(*args, **kwargs): + raise OSError("simulated registry pre-commit failure") + + monkeypatch.setattr(manager.registry, "add", fail_add) + with pytest.raises(OSError, match="registry pre-commit failure"): + manager.install_from_directory(pack_dir, "0.1.5", force=True) + + assert {p.relative_to(installed_dir): p.read_bytes() for p in installed_dir.rglob("*") if p.is_file()} == before_files + assert manager.registry.get("test-pack") == before_metadata + assert { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } == before_artifacts + + def test_force_install_selector_failure_restores_install_and_artifacts( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + installed_dir = manager.presets_dir / "test-pack" + before_install = { + p.relative_to(installed_dir): p.read_bytes() + for p in installed_dir.rglob("*") if p.is_file() + } + registry_file = manager.registry.registry_path + before_registry_file = registry_file.read_bytes() + before_metadata = manager.registry.get("test-pack") + before_artifacts = { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } + + def fail_selector_expansion(*args, **kwargs): + raise OSError("simulated selector expansion failure") + + monkeypatch.setattr(manager, "_expand_command_selectors", fail_selector_expansion) + with pytest.raises(OSError, match="selector expansion failure"): + manager.install_from_directory(pack_dir, "0.1.5", force=True) + + assert { + p.relative_to(installed_dir): p.read_bytes() + for p in installed_dir.rglob("*") if p.is_file() + } == before_install + assert manager.registry.get("test-pack") == before_metadata + assert registry_file.read_bytes() == before_registry_file + assert { + p.relative_to(project_dir): p.read_bytes() + for p in project_dir.rglob("*") + if p.is_file() and ".specify/presets" not in str(p.relative_to(project_dir)) + } == before_artifacts + assert not list(manager.presets_dir.glob(".test-pack.backup-*")) + + def test_initial_install_copy_failure_leaves_no_partial_install( + self, project_dir, pack_dir, monkeypatch + ): + manager = PresetManager(project_dir) + + def partial_failure(src, dst, *args, **kwargs): + Path(dst).mkdir(parents=True) + (Path(dst) / "partial").write_text("partial", encoding="utf-8") + raise OSError("simulated initial copy failure") + + monkeypatch.setattr( + "specify_cli.presets._manager.shutil.copytree", partial_failure + ) + with pytest.raises(OSError, match="initial copy failure"): + manager.install_from_directory(pack_dir, "0.1.5") + + assert not manager.registry.is_installed("test-pack") + assert not (manager.presets_dir / "test-pack").exists() + assert not list(manager.presets_dir.glob(".test-pack.stage-*")) + def test_install_already_installed(self, project_dir, pack_dir): """Test installing an already-installed pack raises error.""" manager = PresetManager(project_dir) diff --git a/tests/specify_cli/presets/test_regex_selector_lifecycle.py b/tests/specify_cli/presets/test_regex_selector_lifecycle.py index 68f31e64f2..f99b8fd125 100644 --- a/tests/specify_cli/presets/test_regex_selector_lifecycle.py +++ b/tests/specify_cli/presets/test_regex_selector_lifecycle.py @@ -164,6 +164,92 @@ def _run_preset_command(project: Path, *args: str) -> None: assert result.exit_code == 0, result.output +def test_disabling_sole_layer_custom_preset_cleans_tracked_artifacts(tmp_path): + project = tmp_path / "project" + project.mkdir() + _active_claude(project) + source = _write_preset( + tmp_path, + "sole-owner", + [ + { + "type": "command", + "name": "speckit.custom.only", + "file": "commands/only.md", + "description": "custom only", + } + ], + {"commands/only.md": "---\ndescription: custom only\n---\nOnly body\n"}, + ) + manager = PresetManager(project) + manager.install_from_directory(source, "0.1.5") + + skill = project / ".claude" / "skills" / "speckit-custom-only" / "SKILL.md" + assert skill.is_file() + metadata = manager.registry.get("sole-owner") + assert metadata["registered_commands"]["claude"] == ["speckit.custom.only"] + assert metadata["registered_skills"]["claude"] == ["speckit-custom-only"] + + _run_preset_command(project, "disable", "sole-owner") + + assert not skill.exists() + metadata = PresetManager(project).registry.get("sole-owner") + assert metadata["enabled"] is False + assert metadata["registered_commands"] == {} + assert metadata["registered_skills"] == {} + + +def test_failed_zero_layer_cleanup_preserves_registry_provenance(monkeypatch, tmp_path): + project = tmp_path / "project" + project.mkdir() + _active_claude(project) + source = _write_preset( + tmp_path, + "cleanup-failure-owner", + [ + { + "type": "command", + "name": "speckit.custom.failure", + "file": "commands/failure.md", + "description": "cleanup failure", + } + ], + { + "commands/failure.md": "---\ndescription: cleanup failure\n---\nFailure body\n" + }, + ) + manager = PresetManager(project) + manager.install_from_directory(source, "0.1.5") + skill = project / ".claude" / "skills" / "speckit-custom-failure" / "SKILL.md" + assert skill.is_file() + assert manager.registry.get("cleanup-failure-owner")["registered_skills"][ + "claude" + ] == ["speckit-custom-failure"] + + original = PresetManager._reconcile_composed_commands + calls = 0 + + def fail_during_disabled_reconciliation(self, names, *args, **kwargs): + nonlocal calls + calls += 1 + if calls == 2: + raise OSError("cleanup blocked") + return original(self, names, *args, **kwargs) + + monkeypatch.setattr( + PresetManager, + "_reconcile_composed_commands", + fail_during_disabled_reconciliation, + ) + _run_preset_command(project, "disable", "cleanup-failure-owner") + + metadata = PresetManager(project).registry.get("cleanup-failure-owner") + assert metadata["enabled"] is False + assert metadata["registered_commands"]["claude"] == ["speckit.custom.failure"] + assert metadata["registered_skills"]["claude"] == ["speckit-custom-failure"] + assert skill.is_file() + + def test_regex_selector_disable_and_reenable_materializes_real_artifacts(tmp_path): project = tmp_path / "project" project.mkdir() diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index d1e416f7a1..06611ca20e 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -642,6 +642,108 @@ def test_resolve_extension_command_via_manifest_skips_oserror_manifests(self, pr assert result is None, "OSError during manifest load must be silently skipped" + def test_collect_all_layers_extension_alternate_filename_respects_registry( + self, project_dir + ): + extensions_dir = project_dir / ".specify" / "extensions" + ext_dir = extensions_dir / "alternate" + command_file = ext_dir / "commands" / "alternate.collect.md" + command_file.parent.mkdir(parents=True) + command_file.write_text("# alternate command\n", encoding="utf-8") + (ext_dir / "extension.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": "alternate", + "name": "alternate", + "version": "1.0.0", + "description": "alternate command fixture", + }, + "provides": { + "commands": [ + { + "name": "speckit.alternate.collect", + "file": "commands/alternate.collect.md", + "description": "collect", + } + ] + }, + } + ), + encoding="utf-8", + ) + registry = ExtensionRegistry(extensions_dir) + registry.add("alternate", {"enabled": True, "version": "1.0.0"}) + resolver = PresetResolver(project_dir) + + enabled_layers = resolver.collect_all_layers( + "speckit.alternate.collect", "command" + ) + assert any(layer["path"] == command_file for layer in enabled_layers) + + registry.update("alternate", {"enabled": False}) + disabled_layers = resolver.collect_all_layers( + "speckit.alternate.collect", "command" + ) + assert all(layer["path"] != command_file for layer in disabled_layers) + + def test_unregistered_extension_alternate_command_fallback_remains_supported( + self, project_dir + ): + ext_dir = project_dir / ".specify" / "extensions" / "orphan" + command_file = ext_dir / "commands" / "orphan.collect.md" + command_file.parent.mkdir(parents=True) + command_file.write_text("# orphan command\n", encoding="utf-8") + + layers = PresetResolver(project_dir).collect_all_layers( + "speckit.orphan.collect", "command" + ) + + assert any(layer["path"] == command_file for layer in layers) + + def test_collect_all_layers_extension_alternate_manifest_must_own_command( + self, project_dir + ): + ext_dir = project_dir / ".specify" / "extensions" / "named" + command_file = ext_dir / "commands" / "named.other.md" + command_file.parent.mkdir(parents=True) + command_file.write_text("# unrelated file\n", encoding="utf-8") + (ext_dir / "extension.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": "named", + "name": "named", + "version": "1.0.0", + "description": "manifest ownership fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.named.declared", + "file": "commands/named.other.md", + "description": "declared", + } + ] + }, + } + ), + encoding="utf-8", + ) + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "named", {"enabled": True, "version": "1.0.0"} + ) + + layers = PresetResolver(project_dir).collect_all_layers( + "speckit.named.declared", "command" + ) + + assert any(layer["path"] == command_file for layer in layers) + + class TestExtensionPriorityResolution: """Test extension priority resolution with registered and unregistered extensions.""" From 33c263599bf7a285e735b21a47dc932f2e6e96a6 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Wed, 30 Sep 2026 22:08:25 +0800 Subject: [PATCH 6/6] fix extensions: preserve kept config on failed reinstall --- src/specify_cli/extensions/__init__.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d4ee28811c..5498815aee 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2551,6 +2551,23 @@ def install_from_directory( HookExecutor(self.project_root).unregister_hooks(manifest.id) except BaseException: pass + # Generic integration rollback deliberately converts a failed install + # into a config-only, unregistered extension directory. Preserve that + # user-facing recovery state instead of restoring the pre-install + # registered snapshot captured by this outer transaction. + preserved_generic_config = ( + dest_dir.is_dir() + and self._has_keep_config_marker(dest_dir) + and not (dest_dir / "extension.yml").exists() + and all( + child.name == ".keep-config" + or child.name.endswith(("-config.yml", "-config.local.yml")) + for child in dest_dir.iterdir() + ) + ) + if preserved_generic_config: + raise + # Remove any partial install, then restore the previous on-disk # extension and metadata/config snapshots byte-for-byte. try: