From 4b4347a87518ea491602f2ba179e763075110e89 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Wed, 14 Oct 2026 22:52:45 +0800 Subject: [PATCH 1/7] feat(presets): support regex selectors for command declarations A preset may declare `regex:` in a `provides.templates` entry of type `command`. The selector expands, in declaration order, into the concrete lower-layer commands it matches; those concrete names then flow through the existing command resolution, registration, composition and removal paths. Matching is against the full logical command name only, never a path, file name or substring, and a literal selector is never used as an output path, a `.composed` file name or a registry key. A `regex:` name on a non-command declaration is a validation error. Diagnostics: `preset info` reports current matches and `preset resolve ` attributes selector-contributed layers. Review follow-up (this revision): - Removed the ownership subsystem (`ownership.py`) and all related overwrite-policy changes, which had grown into this branch and were not required by the selector feature. - Kept the change to command selector parsing, bounded matching, expansion, resolution, diagnostics, and the reconciliation strictly required by those selectors. - Moved coverage into the existing surface-owned test suites; no new test modules are added. - Added the requested negative coverage: an unmatched selector alongside an exact declaration for the same target name must not warn. AI disclosure: this change was authored with AI assistance (Hermes Agent); behaviour was reviewed and verified against the test suite by a human. --- docs/reference/presets.md | 96 ++- src/specify_cli/presets/_manager.py | 55 +- src/specify_cli/presets/_manager_commands.py | 274 +++++- src/specify_cli/presets/_manager_skills.py | 34 +- src/specify_cli/presets/_manifest.py | 22 +- src/specify_cli/presets/_resolver.py | 402 +++++++-- src/specify_cli/presets/_selectors.py | 364 ++++++++ src/specify_cli/presets/command_info.py | 35 + src/specify_cli/presets/command_resolve.py | 23 +- tests/specify_cli/presets/_helpers.py | 139 ++++ .../specify_cli/presets/test_command_info.py | 75 ++ .../presets/test_command_resolve.py | 24 + tests/specify_cli/presets/test_manager.py | 97 +++ .../presets/test_manager_commands.py | 440 +++++++++- .../presets/test_manager_skills.py | 80 ++ tests/specify_cli/presets/test_manifest.py | 98 +++ tests/specify_cli/presets/test_resolver.py | 781 ++++++++++++++++++ .../presets/test_selector_engine.py | 336 ++++++++ 18 files changed, 3258 insertions(+), 117 deletions(-) create mode 100644 src/specify_cli/presets/_selectors.py create mode 100644 tests/specify_cli/presets/test_selector_engine.py diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 8ca29547b7..23a06fd02a 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -151,7 +151,7 @@ keep their exit code (normally 2), as with `list --json`. specify preset resolve ``` -Shows which file will be used for a given name by tracing the full resolution stack. Useful for debugging when multiple presets provide the same file. +Shows which file will be used for a given name by tracing the full resolution stack. Useful for debugging when multiple presets provide the same file. When a layer was contributed by a `regex:` selector, the trace names the declaration that matched the requested name. ## Enable / Disable a Preset @@ -291,6 +291,100 @@ Templates and scripts are looked up from the stack when Spec Kit needs them. Com By default, files use a **replace** strategy: the first match in the priority stack wins and is used entirely. Templates and commands can also use composition strategies: **prepend** places preset content before lower-priority content, **append** places it after lower-priority content, and **wrap** replaces `{CORE_TEMPLATE}` with lower-priority content. Scripts support **replace** and **wrap**; script wrappers use `$CORE_SCRIPT` as the placeholder. +### Regex Selectors + +A `provides.templates` entry with `type: command` can match several commands with a `regex:` prefix on its `name`: + +```yaml +provides: + templates: + - type: command + name: 'regex:^speckit\.(plan|tasks|implement)$' + file: commands/workflow-guidance.md + strategy: append + +``` + +An entry without the prefix keeps exact-name matching and is unaffected: + +```yaml +provides: + templates: + - type: command + name: speckit.plan + file: commands/plan-guidance.md + strategy: append +``` + +How selectors behave: + +- `regex:` selectors are supported only for `command` declarations. Template and script declarations reject the prefix. +- A `name` beginning with `regex:` uses the bounded regular-expression syntax described below. The pattern is matched with **full-match** semantics against the logical command name (for example `speckit.plan`) — the same name you pass to `specify preset resolve` and see in `specify preset info`. It never matches file paths, directory names, or file extensions. +- A selector is eligible only against concrete resources provided by **lower layers** of the resolution stack: lower-priority presets, installed and enabled extensions, and Spec Kit core (including the bundled core pack in a wheel install). Project-local overrides and other `regex:` declarations are never matched, so selectors cannot chain off one another and cannot be satisfied by an override. +- Expansion preserves manifest declaration order, including overlapping exact and regex entries and declarations sharing the same file. Within each selector, concrete names are sorted. +- Every concrete resource a selector matches behaves exactly as though the preset contained a separate exact-name entry with the same `file` and `strategy`. One declaration therefore expands into zero, one, or several concrete resources. +- Matches are evaluated during resolution and existing registration points, including preset installation and active integration registration. Provider changes do not automatically refresh every previously generated match. Re-register affected presets with `specify integration use ` when their concrete providers change. Re-registration also retires tracked matches whose providers have disappeared. +- For `command` entries the matches are expanded to concrete command names **before** registration. Registration, composition, reconciliation, and cleanup all run against those concrete names; the literal `regex:...` string is never stored as a command name in the preset registry, an agent directory, or the `.composed` cache. +- Composition and priority are unchanged. `replace`, `prepend`, `append`, and `wrap` compose against the normal resolution stack, including when command declarations render as agent skills. + +Zero-match and invalid patterns are handled as follows: + +- A `regex:` selector that currently matches no lower-layer resource contributes nothing. Installing such a preset emits a warning unless an exact command declaration in the same preset satisfies the target name; it does **not** fail the installation. +- `specify preset info` lists each selector's current concrete matches nested beneath the declaration, or `No current matches` when there are none. +- `specify preset resolve ` attributes the layers it traces back to the declaration that matched, for example `sel-pack v1.0.0 → …/guidance.md (via regex:^speckit\.plan$)`, so a selector-driven resolution is visible in the stack trace and not only in `preset info`. + +#### Regex selector safety boundaries + +A `regex:` selector pattern is validated when a preset is installed, and again +whenever a selector is evaluated — including a selector found in a manifest that is +already on disk. Validation enforces these limits, and a pattern past one of them is +rejected with a `PresetValidationError` that names the limit it broke: + +| Limit | Value | +| --- | --- | +| pattern length | 4096 characters after the `regex:` prefix | +| compiled states | 4096 | +| compiled edges | 2048 | +| resource name length | 1024 characters | + +Matching never uses Python's backtracking engine. A selector is compiled to a state +machine whose work is bounded by `(resource name length + 1) × compiled edges`, so no +valid selector can make the CLI hang, however long or unlucky the resource name is. +The state and edge counts are capped separately because a branch reuses one entry +state for all of its alternatives: `(?:a|\B|\B|…){4000}` keeps the state count small +while producing millions of edges. + +A selector found in a manifest that is already on disk — written by an older version, +or edited by hand — does not raise during resolution. It is reported with a warning +and treated as matching nothing, so an unsafe manifest cannot break unrelated +commands. Only the installation path fails hard. + +The supported syntax is a subset of Python's regular expressions: literal characters +and escapes, `.`, character classes (`[a-z]`, `[^x]`), the predefined classes (`\d`, +`\w`, `\s` and their negations), grouping (`(...)`, `(?:...)`), alternation, the +anchors `^ $ \A \Z \b \B`, inline flags (`(?i)`, `(?i:...)`), and the quantifiers +`* + ? {m} {m,} {m,n}` including their lazy forms. The following are **refused**, +because their behaviour cannot be analysed from the parsed pattern alone or because +they are the shapes that make backtracking matchers hang: + +- backreferences (`\1`) and named backreferences (`(?P=name)`); +- lookarounds (`(?=…)`, `(?!…)`, `(?<=…)`, `(?…)`) and possessive quantifiers (`a*+`); +- a repetition nested inside another repetition (`(a+)+`, `(a*)*`); +- a repetition whose body can match the empty string (`(a|)*`, `(?:)*`, `\B*`); +- an unbounded repetition over a zero-width alternation, such as `(a|a)*`, which the + parser factorises into `a` followed by a branch whose alternatives are all empty. + +Two similar-looking cases are deliberately treated differently: + +- `(ab|a)*` and `(ba|b)*c` are **accepted**. The parser factorises the shared prefix + out of the alternation, which leaves a branch with an empty alternative, but the + repeated body still has to consume at least `a`/`b` in every alternative. It cannot match + the empty string. +- `(a|a)*` is **refused**, because there the factorised branch has no non-empty + alternative at all. The selector safety rule rejects that unbounded form. +- An invalid regular expression is rejected during manifest validation, so a preset with a malformed pattern fails to install with a clear validation error rather than at resolution time. + The resolution stack, from highest to lowest precedence: 1. **Project-local overrides** — `.specify/templates/overrides/` diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 963bb5c640..8ef41bd84b 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, selector_matches _CONSTITUTION_PROVENANCE_FILE = ".constitution-template.json" _CONSTITUTION_SYNC_PRESET_ID = "constitution-sync" @@ -435,20 +436,32 @@ def install_from_directory( registered_commands: Dict[str, List[str]] = {} registered_skills: Dict[str, List[str]] = {} try: + command_templates = self._expand_command_selectors( + PresetResolver(self.project_root), dest_dir, + [t for t in manifest.templates if t.get("type") == "command"], + ) + self._warn_unmatched_command_selectors(manifest, command_templates) # 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) + registered_commands = self._register_commands( + manifest, dest_dir, command_templates=command_templates + ) 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) + registered_skills = self._register_skills( + manifest, dest_dir, command_templates=command_templates + ) self.registry.update(manifest.id, { "registered_skills": registered_skills, }) + if any(t.get("_selector") for t in command_templates): + for agent in registered_commands.keys() | registered_skills.keys(): + self._refresh_selector_matches(manifest.id, agent, command_templates) except Exception: # Roll back all side effects. _register_skills persists each # successful write immediately, so reload that partial map when @@ -474,11 +487,7 @@ def install_from_directory( # Reconcile all affected commands from the full priority stack so that # install order doesn't determine the winning command file. - cmd_names = [ - t["name"] - for t in manifest.templates - if t.get("type") == "command" - ] + cmd_names = [t["name"] for t in command_templates] if cmd_names: try: self._reconcile_composed_commands(cmd_names) @@ -502,6 +511,24 @@ def install_from_directory( return manifest + @staticmethod + def _warn_unmatched_command_selectors( + manifest: PresetManifest, command_templates: List[Dict[str, Any]] + ) -> None: + """Warn for valid command selectors with no concrete lower-layer match.""" + import warnings + + for declaration in manifest.templates: + selector = declaration["name"] + if declaration["type"] != "command" or not is_regex_selector(selector): + continue + if not any(selector_matches(selector, t["name"]) for t in command_templates): + warnings.warn( + f"Preset '{manifest.id}' selector '{selector}' currently matches no command resources. " + "Regex selectors only match concrete commands from lower layers.", + stacklevel=2, + ) + def _seed_constitution_from_preset( self, manifest: PresetManifest, preset_dir: Path ) -> None: @@ -689,7 +716,7 @@ def remove(self, pack_id: str) -> bool: # branch of _unregister_skills restores only the currently # active agent's directory, leaving this preset's overrides in # every previously active agent's directory orphaned. Infer - # real per-agent ownership from the on-disk preset marker now, + # real per-agent provenance from the on-disk preset marker now, # while pack_id is still known, and hand the resulting mapping # through the same dict-based cleanup path already used for # non-legacy registries (#2948). @@ -759,12 +786,20 @@ def remove(self, pack_id: str) -> bool: ) ) for cmd_names in registered_commands.values(): - removed_cmd_names.update(cmd_names) + removed_cmd_names.update( + name for name in cmd_names + if isinstance(name, str) and not is_regex_selector(name) + ) manifest_path = pack_dir / "preset.yml" if manifest_path.exists(): try: manifest = PresetManifest(manifest_path) - for tmpl in manifest.templates: + declarations = [t for t in manifest.templates if t.get("type") != "command"] + declarations += self._expand_command_selectors( + PresetResolver(self.project_root), pack_dir, + [t for t in manifest.templates if t.get("type") == "command"], + ) + for tmpl in declarations: if ( tmpl.get("type") == "template" and tmpl.get("name") == "constitution-template" diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index d171eb28dd..1eff380784 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -16,8 +16,9 @@ ) from ..extensions import ExtensionRegistry from ..integrations._file_changes import write_text as _write_text -from ._manifest import PresetManifest -from ._resolver import PresetResolver +from ._manifest import PresetManifest, PresetValidationError +from ._resolver import PresetResolver, _conventional_command_names +from ._selectors import is_regex_selector, selector_matches def _substitute_core_template( @@ -92,10 +93,124 @@ def _substitute_core_template( class _PresetCommandMethods: """Command artifact methods shared through PresetManager's lifecycle state.""" + @staticmethod + def _expand_command_selectors( + 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. + """ + if not any(is_regex_selector(t.get("name", "")) for t in command_templates): + return command_templates + presets = resolver._get_all_presets_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(resolver.project_root).list_artifacts() + # The resolver also serves conventional command files, including the + # namespace fallback ``commands/.md`` and + # ``templates/commands/``, which the artifact inventory reports under + # a different name. A selector that matches such a concrete name must + # still expand; ``_command_name_has_lower_layer`` below keeps only + # names an actual lower layer provides. + conventional: set[str] = set() + for preset_id in lower_preset_ids: + conventional |= _conventional_command_names( + resolver.presets_dir / preset_id + ) + for ext_id in lower_extension_ids: + conventional |= _conventional_command_names( + resolver.extensions_dir / ext_id + ) + 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 |= conventional + concrete_candidates.update(f"speckit.{name}" for name in CORE_COMMAND_NAMES) + concrete_names = sorted( + { + name + for name in concrete_candidates + if _PresetCommandMethods._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, "_selector": 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.""" + return ( + any( + resolver._preset_has_concrete_resource( + resolver.presets_dir / preset_id, name, "command" + ) + for preset_id in lower_preset_ids + ) + or any( + resolver._extension_has_concrete_resource( + resolver.extensions_dir / extension_id, name, "command" + ) + for extension_id in lower_extension_ids + ) + or resolver._core_has_concrete_resource(name, "command") + ) + def _register_commands( self, manifest: PresetManifest, - preset_dir: Path + preset_dir: Path, + *, + command_templates: Optional[List[Dict[str, Any]]] = None, ) -> Dict[str, List[str]]: """Register preset command overrides with all detected AI agents. @@ -114,9 +229,11 @@ def _register_commands( Returns: Dictionary mapping agent names to lists of registered command names """ - command_templates = [ - t for t in manifest.templates if t.get("type") == "command" - ] + if command_templates is None: + command_templates = self._expand_command_selectors( + PresetResolver(self.project_root), preset_dir, + [t for t in manifest.templates if t.get("type") == "command"], + ) if not command_templates: return {} @@ -183,6 +300,15 @@ def _register_commands( else: commands_to_register.append(cmd) + if any(command.get("_selector") for command in command_templates): + # Resolution retains every contributing layer; render each concrete + # command once, using its first usable declaration (or composed file). + winners: Dict[str, Dict[str, Any]] = {} + for command in commands_to_register: + if (preset_dir / command["file"]).is_file(): + winners.setdefault(command["name"], command) + commands_to_register = list(winners.values()) + try: registrar = self._command_registrar() except ImportError: @@ -232,6 +358,60 @@ def _register_commands( only_agent=active_agent, ) + def _refresh_selector_matches( + self, + pack_id: str, + agent_name: str, + declarations: List[Dict[str, Any]], + ) -> set[str]: + """Retire previously expanded names before rendering the new priority stack. + + Skills lose the original command spelling when dots become hyphens, so + retain concrete selector names per agent instead of trying to reconstruct + them from skill filenames. Actual cleanup still uses upstream's tracked + command/skill removal paths. + """ + metadata = self.registry.get(pack_id) or {} + previous_by_agent = metadata.get("selector_commands", {}) + previous = set(previous_by_agent.get(agent_name, [])) + current = { + name + for declaration in declarations + for name in [declaration["name"], *declaration.get("aliases", [])] + } + stale = previous - current + for key in ("registered_commands", "registered_skills"): + registered = metadata.get(key, {}) + if not isinstance(registered, dict): + continue + stale_names = stale if key == "registered_commands" else { + skill for name in stale for skill in self._skill_names_for_command(name) + } + retired = [name for name in registered.get(agent_name, []) if name in stale_names] + if not retired: + continue + if key == "registered_commands": + self._unregister_commands({agent_name: retired}) + else: + self._unregister_skills( + {agent_name: retired}, self.presets_dir / pack_id, + restore_from_bundled_core=True, + ) + remaining = { + **registered, + agent_name: [name for name in registered[agent_name] if name not in retired], + } + self.registry.update(pack_id, {key: remaining}) + matches = sorted({ + name + for declaration in declarations if declaration.get("_selector") + for name in [declaration["name"], *declaration.get("aliases", [])] + }) + self.registry.update(pack_id, { + "selector_commands": {**previous_by_agent, agent_name: matches}, + }) + return previous + def register_enabled_presets_for_agent(self, agent_name: str) -> None: """Re-register enabled presets' command overrides and skills for ``agent_name``. @@ -273,6 +453,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: resolver = PresetResolver(self.project_root) affected_cmd_names: set = 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() @@ -282,7 +463,16 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: ) if candidate_manifest is None: continue - for template in candidate_manifest.templates: + concrete_declarations = self._expand_command_selectors( + resolver, self.presets_dir / candidate_pack_id, + [t for t in candidate_manifest.templates if t.get("type") == "command"], + ) + expanded_declarations_by_pack[candidate_pack_id] = concrete_declarations + if any(is_regex_selector(t.get("name", "")) for t in candidate_manifest.templates): + affected_cmd_names.update(self._refresh_selector_matches( + candidate_pack_id, agent_name, concrete_declarations + )) + for template in concrete_declarations: command_name = template.get("name") if ( template.get("type") == "command" @@ -324,15 +514,18 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: ] = [] successful_command_replacements: set[tuple[str, str]] = set() for pack_id, metadata in reversed(presets_by_priority): + metadata = self.registry.get(pack_id) or metadata pack_dir = self.presets_dir / pack_id manifest = resolver._get_manifest(pack_dir) if manifest is None: continue + concrete_declarations = expanded_declarations_by_pack.get(pack_id, []) + # 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: + for tmpl in concrete_declarations: name = tmpl.get("name") if tmpl.get("type") == "command" and isinstance(name, str): affected_cmd_names.add(name) @@ -340,11 +533,13 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # 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: + for tmpl in concrete_declarations: if tmpl.get("type") != "command": continue primary_name = tmpl.get("name") @@ -392,7 +587,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: registered_skills = self._register_skills(manifest, pack_dir) replaced_skill_names = set(registered_skills.get(agent_name) or []) - for tmpl in manifest.templates: + for tmpl in concrete_declarations: if tmpl.get("type") != "command": continue primary_name = tmpl.get("name") @@ -413,7 +608,7 @@ def register_enabled_presets_for_agent(self, agent_name: str) -> None: # Legacy flat-list value: don't assume agent_name wrote # every name (the first post-upgrade operation may be a # direct switch to a different skill-mode agent) — - # infer real ownership from on-disk provenance instead + # infer real provenance from on-disk provenance instead # (#2948). existing_skills = self._infer_legacy_skill_provenance( [n for n in raw_existing_skills if isinstance(n, str)], @@ -450,7 +645,7 @@ 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: + for tmpl in concrete_declarations: if tmpl.get("type") != "command": continue primary_name = tmpl.get("name") @@ -509,7 +704,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: + for tmpl in concrete_declarations: if tmpl.get("type") != "command": continue primary_name = tmpl.get("name") @@ -705,7 +900,7 @@ def unregister_agent_artifacts(self, agent_name: str) -> None: raw_skills = metadata.get("registered_skills", []) if isinstance(raw_skills, list) and raw_skills: # Legacy flat-list value predating per-agent provenance: - # infer real ownership from on-disk markers before removing + # infer real provenance from on-disk markers before removing # anything, so only agent_name's actual share is unregistered # and the rest migrates to per-agent form instead of either # guessing every name belongs to agent_name or blindly @@ -1016,16 +1211,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, pack_dir, - only_agent=only_agent, extra_agents=extra_agents, + concrete = next( + ( + declaration + for declaration in self._expand_command_selectors( + resolver, + pack_dir, + [ + item + for item in manifest.templates + if item.get("type") == "command" + ], ) - record_written(written) - self._merge_pack_registered_commands(manifest.id, written) - 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 @@ -1128,8 +1347,11 @@ 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, + [t for t in manifest.templates if t.get("type") == "command"], + ): + if tmpl.get("name") == cmd_name: composed_dir = pack_dir / ".composed" composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 5a4a4fb739..77e91c0337 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -15,7 +15,7 @@ from .._utils import dump_frontmatter from ..integrations.base import IntegrationBase from ..integrations._registration import project_registration -from ._manager_commands import _substitute_core_template +from ._manager_commands import _PresetCommandMethods, _substitute_core_template from ._manifest import PresetManifest, PresetValidationError from ._resolver import PresetResolver @@ -30,9 +30,10 @@ class _FilteredManifest: that aren't being reconciled. """ - def __init__(self, manifest: "PresetManifest", cmd_names: set): + def __init__(self, manifest: "PresetManifest", cmd_names: set, commands: List[Dict[str, Any]]): self._manifest = manifest self._cmd_names = cmd_names + self._commands = commands def __getattr__(self, name: str): return getattr(self._manifest, name) @@ -40,7 +41,7 @@ def __getattr__(self, name: str): @property def templates(self) -> List[Dict[str, Any]]: return [ - t for t in self._manifest.templates + t for t in self._commands if t.get("name") in self._cmd_names ] @@ -72,7 +73,7 @@ def _merge_pack_registered_skills( return # pack_id no longer installed (e.g. removed mid-loop) raw_existing_skills = metadata.get("registered_skills") if isinstance(raw_existing_skills, list) and raw_existing_skills: - # Legacy flat-list value: infer real per-agent ownership from + # Legacy flat-list value: infer real per-agent provenance from # on-disk provenance rather than guessing (#2948). fallback_agent = next(iter(written)) if written else None existing_skills = self._infer_legacy_skill_provenance( @@ -338,7 +339,11 @@ def apply_to_dir( except PresetValidationError: continue cmds_set = set(dir_cmds) - filtered_manifest = self._FilteredManifest(manifest, cmds_set) + concrete_declarations = _PresetCommandMethods._expand_command_selectors( + resolver, pack_dir, + [t for t in manifest.templates if t.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 @@ -626,6 +631,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. @@ -658,9 +664,11 @@ def _register_skills( matching the shape ``registered_commands`` already uses so the two can be tracked/restored consistently (#2948). """ - command_templates = [ - t for t in manifest.templates if t.get("type") == "command" - ] + if command_templates is None: + command_templates = _PresetCommandMethods._expand_command_selectors( + PresetResolver(self.project_root), preset_dir, + [t for t in manifest.templates if t.get("type") == "command"], + ) if not command_templates: return {} @@ -703,12 +711,16 @@ def _register_skills( written: List[str] = [] + rendered_commands: Set[str] = set() for cmd_tmpl in command_templates: cmd_name = cmd_tmpl["name"] + if cmd_name in rendered_commands: + continue cmd_file_rel = cmd_tmpl["file"] source_file = preset_dir / cmd_file_rel - if not source_file.exists(): + if not source_file.is_file(): continue + rendered_commands.add(cmd_name) # Use composed content if available (written by _register_commands # for commands with non-replace strategies), otherwise the original. @@ -832,7 +844,7 @@ def _register_skills( def _infer_legacy_skill_provenance( self, skill_names: List[str], pack_id: str, fallback_agent: str ) -> Dict[str, List[str]]: - """Infer per-agent ownership of a legacy flat-list ``registered_skills`` value. + """Infer per-agent provenance of a legacy flat-list ``registered_skills`` value. Pre-#2948 registries recorded ``registered_skills`` as a flat list with no record of which agent directory each name was actually @@ -858,7 +870,7 @@ def _infer_legacy_skill_provenance( ``SKILL.md`` whose frontmatter records this exact preset as the owner (``metadata.source == "preset:"``, the same marker :meth:`_register_skills` writes) — this marker check is what keeps - the broadened probe from falsely attributing ownership to an + the broadened probe from falsely attributing provenance to an agent's directory that never actually held this preset's override (e.g. a command-mode agent that never rendered skills, or an unrelated skill of the same name). A name can legitimately be diff --git a/src/specify_cli/presets/_manifest.py b/src/specify_cli/presets/_manifest.py index b5a7ff03c3..331765ef61 100644 --- a/src/specify_cli/presets/_manifest.py +++ b/src/specify_cli/presets/_manifest.py @@ -264,8 +264,28 @@ def _validate(self): f"scripts only support {sorted(VALID_SCRIPT_STRATEGIES)}" ) + # A regex selector is validated here, before installation or resolution. + from ._selectors import ( + REGEX_COMPILE_ERRORS, + compile_name_selector, + is_regex_selector, + ) + + if is_regex_selector(tmpl["name"]): + if tmpl["type"] != "command": + # Only command declarations support selectors in this change. + raise PresetValidationError( + f"regex: selectors are only supported for command " + f"declarations, not {tmpl['type']!r}" + ) + try: + compile_name_selector(tmpl["name"]) + except REGEX_COMPILE_ERRORS as exc: + raise PresetValidationError( + f"Invalid regex selector in template name {tmpl['name']!r}: {exc}" + ) from exc # Validate template name format - if tmpl["type"] == "command": + elif tmpl["type"] == "command": # Commands use dot notation (e.g. speckit.specify) if not re.match(r'^[a-z0-9.-]+$', tmpl["name"]): raise PresetValidationError( diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index ea1c4e03f9..458f2accad 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -10,6 +10,31 @@ 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 + + +#: Conventional subdirectories per resource type. ``templates/commands`` is a +#: conventional lower-preset command location in its own right (the eligibility check and +#: selector expansion both enumerate it), so resolution and layer collection must search +#: it too -- otherwise a matched resource is eligible yet never offered as a layer, and a +#: selector materializes nothing. +_RESOURCE_SUBDIRS: dict[str, tuple[str, ...]] = { + "template": ("templates", ""), + "command": ("commands", "templates/commands"), + "script": ("scripts",), +} + + +def _conventional_command_names(root: Path) -> set[str]: + """Enumerate logical names from the resolver's conventional command locations.""" + names: set[str] = set() + for subdir in _RESOURCE_SUBDIRS["command"]: + for path in (root / subdir).glob("*.md"): + if path.is_file() and path.stem and not is_regex_selector(path.stem): + names.add(path.stem) + if not path.stem.startswith("speckit."): + names.add(f"speckit.{path.stem}") + return names class PresetResolver: @@ -43,7 +68,16 @@ def _get_manifest(self, pack_dir: Path) -> Optional["PresetManifest"]: if manifest_path.exists(): try: self._manifest_cache[key] = PresetManifest(manifest_path) - except PresetValidationError: + except PresetValidationError as exc: + from ._selectors import SelectorSafetyError + + if isinstance(exc.__cause__, SelectorSafetyError): + import warnings + + warnings.warn( + f"Ignoring unsafe preset manifest {manifest_path}: {exc}", + stacklevel=2, + ) self._manifest_cache[key] = None else: self._manifest_cache[key] = None @@ -84,7 +118,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 @@ -153,6 +193,38 @@ def _extension_manifest_declared_template( return entry, (candidate if candidate.is_file() else None) return None, None + def _find_unregistered_extension_command( + self, template_name: str, ext_dir: Path | None = None + ) -> Path | None: + """Find a legacy command filename within its matching extension only.""" + extension_template_name = template_name.removeprefix("speckit.") + namespace = extension_template_name.split(".", 1)[0] + expected_dir = self.extensions_dir / namespace + if ext_dir is not None and ext_dir.resolve() != expected_dir.resolve(): + return None + ext_dir = expected_dir + 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. @@ -266,6 +338,19 @@ def resolve( if not skip_presets and self.presets_dir.exists(): for pack_id, _metadata in self._get_all_presets_by_priority(): pack_dir = self.presets_dir / pack_id + if template_type == "command": + declarations = self._preset_declarations_for_resource( + pack_dir, template_name, template_type + ) + if declarations: + for _declaration, candidate in declarations: + if candidate is not None: + return candidate + continue + candidate = self._conventional_resource(pack_dir, template_name, template_type) + if candidate is not None: + return candidate + continue # The preset manifest is authoritative: if it declares this # template with an explicit ``file:``, resolve to that path — # and do NOT fall back to convention when it's missing, to @@ -300,6 +385,11 @@ def resolve( ext_dir = self.extensions_dir / ext_id if not ext_dir.is_dir(): continue + if template_type == "command": + candidate = self._extension_resource(ext_dir, template_name, template_type) + if candidate is not None: + return candidate + continue # The extension manifest is authoritative, same as preset manifests # above: check it before convention-based lookup so a declared entry # at a non-conventional path wins over a stale conventional file. @@ -504,6 +594,158 @@ def resolve_with_source( return {"path": resolved_str, "source": "core"} + def _preset_declarations_for_resource( + self, pack_dir: Path, template_name: str, template_type: str + ) -> list[tuple[dict, Path | None]]: + """Collect exact and matching regex declarations in manifest order.""" + if template_type != "command": + return [] + manifest = self._get_manifest(pack_dir) + if manifest is 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 [] + 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: + has_lower_resource = any( + self._extension_has_concrete_resource( + self.extensions_dir / ext_id, template_name, template_type + ) + 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 + ) + matches = [] + for declaration in manifest.templates: + 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 + matches.append( + (declaration, path if path is not None and path.is_file() else None) + ) + return matches + + 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_resource( + self, base_dir: Path, name: str, template_type: str + ) -> Path | None: + """Discover one candidate per enabled provider, honoring declarations. + + Legacy namespace filenames are command-only and scoped to base_dir; + they must never rediscover another provider's file or bypass a missing + manifest declaration. Share this lookup with regex eligibility so every + eligible base can actually be resolved and collected. + """ + entry, candidate = self._extension_manifest_declared_template( + base_dir, name, template_type + ) + if entry is not None: + return candidate + candidate = self._conventional_resource(base_dir, name, template_type) + if candidate is None and template_type == "command": + candidate = self._conventional_resource( + base_dir / "templates", name, template_type + ) + if candidate is None and template_type == "command": + candidate = self._find_unregistered_extension_command(name, base_dir) + return candidate + + def _extension_has_concrete_resource( + self, base_dir: Path, name: str, template_type: str + ) -> bool: + return self._extension_resource(base_dir, name, template_type) is not None + + 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]: + # ``templates/commands`` is a conventional command location in its own + # right: _conventional_command_names and _expand_command_selectors both + # enumerate it, for lower presets as well as for extensions. Searching it + # here — at the single shared lookup — is what keeps the preset-side + # eligibility check that calls this method directly in agreement with + # selector expansion; otherwise a matched resource is never offered as a + # layer and the selector materializes nothing. + subdirs = _RESOURCE_SUBDIRS.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, @@ -512,7 +754,10 @@ def collect_all_layers( """Collect all layers in the priority stack for a template. Returns layers from highest priority (checked first) to lowest priority. - Each layer is a dict with 'path', 'source', and 'strategy' keys. + Each layer is a dict with 'path', 'source', and 'strategy' keys; preset + layers additionally carry 'declaration', the manifest name that matched + the requested resource (a ``regex:`` selector or an exact name), so + diagnostics can attribute a contributing layer to its declaration. Args: template_name: Template name (e.g., "spec-template") @@ -546,101 +791,126 @@ 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(): 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 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 not manifest_has_strategy and strategy == "replace" and template_type == "command": + if not preset_declarations: + entry, candidate = self._manifest_declared_template( + pack_dir, template_name, template_type + ) + 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" + and template_type == "command" + ): try: 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({ - "path": candidate, - "source": f"{pack_id} v{version}", - "strategy": strategy, - }) + layers.append( + { + "path": candidate, + "source": f"{pack_id} v{version}", + "strategy": strategy, + # Attribution for diagnostics: the manifest + # declaration that produced this concrete layer. A + # ``regex:`` value identifies the selector that + # matched the requested name; an exact declaration + # keeps its literal name. Extension, override, and + # core layers have no declaration. + "declaration": ( + layer_entry.get("name") + if isinstance(layer_entry, dict) + else None + ), + } + ) # Priority 3: Extension-provided templates (always "replace") for _priority, ext_id, ext_meta in self._get_all_extensions_by_priority(): ext_dir = self.extensions_dir / ext_id if not ext_dir.is_dir(): continue - # The extension manifest is authoritative, same as preset manifests - # above: check it before convention-based lookup so a declared entry - # at a non-conventional path wins over a stale conventional file, and - # a declared-but-missing file isn't silently masked by convention. - entry, candidate = self._extension_manifest_declared_template( - ext_dir, template_name, template_type - ) - if entry is None: - candidate = _find_in_subdirs(ext_dir) + if template_type == "command": + candidate = self._extension_resource(ext_dir, template_name, template_type) + else: + entry, candidate = self._extension_manifest_declared_template( + ext_dir, template_name, template_type + ) + if entry is None: + candidate = _find_in_subdirs(ext_dir) if candidate: if ext_meta: version = ext_meta.get("version", "?") 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, + } + ) + + # No separate namespace fallback: the scoped extension loop above is + # the only path that can append a legacy alternate command filename. # Priority 4: Core templates (always "replace") core = None diff --git a/src/specify_cli/presets/_selectors.py b/src/specify_cli/presets/_selectors.py new file mode 100644 index 0000000000..f6e3faebcd --- /dev/null +++ b/src/specify_cli/presets/_selectors.py @@ -0,0 +1,364 @@ +"""Exact names and bounded, non-backtracking ``regex:`` selectors. + +Regexes use a Thompson state machine, never Python's backtracking matcher for +compound expressions. Each state is visited at most once per input position, so +the work is bounded by (input length + 1) x (number of compiled edges) and no +pattern can backtrack. Compilation and matching limits are 4,096 pattern +characters, 4,096 states, 2,048 compiled edges and 1,024 input +characters; a pattern past a compilation limit +is rejected with ``SelectorSafetyError`` (a ``re.error`` subclass) that names it, +and exact-name equality is unchanged. + +Groups, alternation, character classes, ordinary/lazy quantifiers, flags and +anchors retain Python full-match semantics. Rejected during validation AND runtime +compilation are backreferences, lookarounds, atomic and possessive constructs, +nested repetitions, a repetition whose body can match empty, and an unbounded +repetition over a zero-width alternation (every alternative of a factorised branch +empty, as in (a|a)*). An alternation the parser factorises while some alternative +still consumes input -- (ab|a)*, (ba|b)*, (a|ab)* -- is ordinary and accepted: +safety comes from the state machine and the graph limits, not from recognising +payload shapes, so the accepted set only narrows where a repetition cannot be +analysed without guessing. + +CPython's regex parser/compiler (Python 3.11-3.14) supply syntax and single-atom +predicates only. No user-controlled compound expression reaches ``re`` execution. +""" + +from __future__ import annotations + +import re +from functools import lru_cache +from re import _compiler, _constants as C, _parser + +REGEX_PREFIX = "regex:" +REGEX_COMPILE_ERRORS = (re.error, OverflowError, RecursionError) +_MAX_PATTERN = 4096 +_MAX_STATES = 4096 +# Concrete resource names are logical names: manifest-declared names and +# filesystem stems, which are capped at 255 bytes by every filesystem we run on. +# 1024 leaves roughly four times the 255-byte +# filesystem limit while keeping a hostile many-state pattern from being run +# over an unbounded pseudo-name. Matching rejects longer input with the same +# ``SelectorSafetyError``; exact-name equality is never length-capped. +_MAX_INPUT = 1024 +# Edges must be capped independently of states: a branch reuses its entry state for +# every alternative and a repetition unrolls those edges, so a branch of N empty +# alternatives unrolled K times keeps the state count small while producing N*K +# edges. With only a state cap, `(?:a|\B|\B|...)` with 1000 alternatives under +# `{4000}` compiles to 4004 states but 4,004,003 edges, i.e. 16 billion edge +# examinations against the largest permitted name -- a hang, not a slow match. +_MAX_EDGES = 2048 +# There is deliberately no fixed transition budget. Work is provably bounded by +# (len(concrete name) + 1) x (number of compiled edges), and both factors are +# capped here, so the bound is computed per match from the compiled graph and the +# actual input instead of being guessed. Every earlier fixed budget (100_000, then +# 2_000_000) was reachable by a pattern that passes validation -- "a*" * 200 + "b" +# on a 100-character name, then "a*" * 800 + "b" on a 1,000-character name -- and +# turned a selector that does match into a non-match. The caps here keep the +# worst permitted match at about (_MAX_INPUT + 1) * _MAX_EDGES, near a second. + + +_ATOM_CACHE_LIMIT = 4096 +_atom_cache: dict[tuple[int, str, int], re.Pattern[str]] = {} + + +def _atom_predicate(op: int, arg: object, flags: int) -> re.Pattern[str]: + """Compile a single-token predicate once per distinct token and flags. + + Unrolling a repetition compiles the same atom once per copy, and compiling a + large case-folded character set is not cheap: ``(?i:[\u0000-\uffff]){4000}`` + asked CPython's compiler for the same predicate 4000 times and spent over + eight seconds at import, before a single character was matched. The token and + its flags determine the predicate completely and compiled patterns are + immutable, so one compile is reused for every copy. The cache is bounded and + cleared wholesale rather than evicted, which keeps it trivial to reason about. + """ + key = (op, repr(arg), flags) + cached = _atom_cache.get(key) + if cached is None: + atom = _parser.SubPattern(_parser.State(), [(op, arg)]) + cached = _compiler.compile(atom, flags) + if len(_atom_cache) >= _ATOM_CACHE_LIMIT: + _atom_cache.clear() + _atom_cache[key] = cached + return cached + + +def _can_match_empty(tokens) -> bool: + """Return whether a token sequence can match the empty string. + + ``LITERAL``/``NOT_LITERAL``/``IN``/``ANY``/``CATEGORY`` each consume exactly + one character, so a sequence holding one cannot match empty. ``AT`` (^ $ \\b + \\B \\A \\Z) is zero-width. A repetition matches empty when it may be taken + zero times *or* when its body can; a branch matches empty when any alternative + does. Anything else is unsupported here and already rejected by construction. + """ + for op, arg in tokens: + if op is C.SUBPATTERN: + if not _can_match_empty(arg[3]): + return False + elif op is C.BRANCH: + if not any(_can_match_empty(body) for body in arg[1]): + return False + elif op in (C.MAX_REPEAT, C.MIN_REPEAT): + minimum, _maximum, body = arg + if minimum > 0 and not _can_match_empty(body): + return False + elif op is C.AT: + continue + else: + return False + return True + + +def _has_zero_width_alternation(tokens) -> bool: + """Return whether a branch in ``tokens`` has only empty alternatives. + + CPython's parser factors a shared alternation prefix out of the branches, so + ``(?:x|x)`` arrives as ``LITERAL x`` followed by ``BRANCH([[], []])``. A branch + all of whose alternatives are empty is itself zero-width -- repeating that + without bound is the genuinely ambiguous shape. A factorised *suffix* such as + ``(?:ab|a)`` leaves at least one alternative that consumes input, so the branch + is ordinary alternation and is not reported here. + """ + for op, arg in tokens: + if op is C.SUBPATTERN: + if _has_zero_width_alternation(arg[3]): + return True + elif op is C.BRANCH: + bodies = arg[1] + if bodies and all(not body for body in bodies): + return True + if any(_has_zero_width_alternation(body) for body in bodies): + return True + elif op in (C.MAX_REPEAT, C.MIN_REPEAT): + if _has_zero_width_alternation(arg[2]): + return True + return False + + +def _validate_repetitions(tokens) -> None: + """Refuse repetitions whose body cannot be reasoned about safely. + + Two shapes, both read from the parsed semantics rather than from whether some + node happens to be an empty list: a repetition whose body can match the empty + string, and an unbounded repetition over a zero-width alternation. Called once + the graph exists, so a pattern that exceeds the state or edge cap is reported + as oversized instead of as ambiguous. + """ + for op, arg in tokens: + if op is C.SUBPATTERN: + _validate_repetitions(arg[3]) + elif op is C.BRANCH: + for body in arg[1]: + _validate_repetitions(body) + elif op in (C.MAX_REPEAT, C.MIN_REPEAT): + _minimum, maximum, body = arg + if _can_match_empty(body): + raise _unsafe("repetition over a body that can match empty") + if maximum == C.MAXREPEAT and _has_zero_width_alternation(body): + raise _unsafe("zero-width alternation under an unbounded repetition") + _validate_repetitions(body) + + +class SelectorSafetyError(re.error): + """A selector exceeded the supported syntax or bounded execution limits.""" + + +def _unsafe(reason: str) -> SelectorSafetyError: + return SelectorSafetyError( + f"Regex selector safety boundary: {reason}. Bounded non-backtracking " + f"selectors allow {_MAX_PATTERN} pattern characters, {_MAX_STATES} " + f"states, {_MAX_EDGES} edges and {_MAX_INPUT} input characters, and reject " + "nested repetitions, " + "backreferences, lookarounds and atomic " + "or possessive constructs. A repetition whose body can match empty, and an " + "unbounded repetition over a zero-width alternation (for example (a|a)*), " + "are refused as well." + ) + + +def is_regex_selector(name: str) -> bool: + """Return whether a manifest name opts into regex selector semantics.""" + return name.startswith(REGEX_PREFIX) + + +class _Selector: + def __init__(self, pattern: str): + parsed = _parser.parse(pattern, 0) + # Edges are (destination, single-atom predicate or None, consumes input). + self.edges: list[list[tuple[int, re.Pattern | None, bool]]] = [] + self.edge_count = 0 + + def state(): + if len(self.edges) >= _MAX_STATES: + raise _unsafe("compiled state limit exceeded") + self.edges.append([]) + return len(self.edges) - 1 + + def edge(start, end, predicate=None, consumes=False): + if self.edge_count >= _MAX_EDGES: + raise _unsafe("compiled edge limit exceeded") + self.edges[start].append((end, predicate, consumes)) + self.edge_count += 1 + + def sequence(tokens, start, end, flags, repeated=False): + for index, (op, arg) in enumerate(tokens): + dest = end if index == len(tokens) - 1 else state() + if op is C.SUBPATTERN: + _, add, remove, body = arg + sequence( + body, + start, + dest, + _compiler._combine_flags(flags, add, remove), + repeated, + ) + elif op is C.BRANCH: + # An empty alternative here is not itself a reason to refuse: + # the parser produces one whenever branches share a prefix, and + # what matters is whether the enclosing repetition can be + # ambiguous. That is decided when the repetition is built, from + # the body's semantics rather than from this node's shape. + for body in arg[1]: + sequence(body, start, dest, flags, repeated) + elif op in (C.MAX_REPEAT, C.MIN_REPEAT): + if repeated: + raise _unsafe("nested repetition") + minimum, maximum, body = arg + if minimum > _MAX_STATES or ( + maximum != C.MAXREPEAT and maximum > _MAX_STATES + ): + raise _unsafe("repetition exceeds compiled state limit") + # The repetition gets its own entry and exit states. Reusing the + # enclosing ``start``/``dest`` makes the loop-back edge point at a + # state shared with the surrounding construct; inside a branch + # alternative that state is the branch entry, so a partially matched + # alternative could leave its own repetition and continue in a + # sibling alternative — ``regex:(a|b)*c|d`` wrongly matched "ad", + # ``regex:a*b|c`` wrongly matched "ac", ``regex:(a*)b|c`` wrongly + # matched "ac". + entry = state() + exit_state = state() + edge(start, entry) + cursor = entry + for _ in range(minimum): + nxt = state() + sequence(body, cursor, nxt, flags, True) + cursor = nxt + if maximum == C.MAXREPEAT: + edge(cursor, exit_state) + tail = state() + sequence(body, cursor, tail, flags, True) + edge(tail, cursor) + else: + for _ in range(maximum - minimum): + edge(cursor, exit_state) + nxt = state() + sequence(body, cursor, nxt, flags, True) + cursor = nxt + edge(cursor, exit_state) + edge(exit_state, dest) + elif op in (C.LITERAL, C.NOT_LITERAL, C.IN, C.ANY, C.CATEGORY, C.AT): + edge(start, dest, _atom_predicate(op, arg, flags), op is not C.AT) + else: + raise _unsafe(f"unsupported construct {op}") + start = dest + if not tokens: + edge(start, end) + + self.start, self.end = state(), state() + sequence(parsed, self.start, self.end, parsed.state.flags) + # Deliberately after the walk: the state and edge caps are what bound the + # work, so an oversized pattern must be reported as oversized. Running the + # semantic repetition rules first would mask those caps for the very + # zero-width branch shapes that can inflate the edge count. + _validate_repetitions(parsed) + + def matches(self, text: str) -> bool: + if len(text) > _MAX_INPUT: + raise _unsafe("input character limit exceeded") + # (positions x edges) is a proven upper bound, not a guess: the epsilon + # closure below visits a state at most once per position and walks each of + # its edges once. The guard is therefore an unreachable net for a traversal + # bug rather than a limit a valid selector can hit. + budget = (len(text) + 1) * self.edge_count + active = {self.start} + work = 0 + for position in range(len(text) + 1): + pending = list(active) + following = set() + while pending: + current = pending.pop() + for dest, predicate, consumes in self.edges[current]: + work += 1 + if work > budget: + raise _unsafe("transition budget exceeded") + if ( + predicate is not None + and predicate.match(text, position) is None + ): + continue + if consumes: + following.add(dest) + elif dest not in active: + active.add(dest) + pending.append(dest) + if position == len(text): + return self.end in active + if not following: + return False + active = following + return False + + +@lru_cache(maxsize=128) +def _compile_pattern(pattern: str) -> _Selector: + return _Selector(pattern) + + +def compile_name_selector(name: str) -> _Selector | None: + """Validate/compile a bounded regex, or return None for an exact name.""" + if not is_regex_selector(name): + return None + if len(name) - len(REGEX_PREFIX) > _MAX_PATTERN: + raise _unsafe("pattern character limit exceeded") + return _compile_pattern(name[len(REGEX_PREFIX) :]) + + +def selector_matches(name: str, concrete_name: str) -> bool: + """Full-match with bounded work, including selectors from historical manifests. + + Every caller uses this as a boolean predicate inside resolution, installation + and diagnostics (``_resolver``, ``_manager``, ``_manager_commands``, + ``command_info``), so neither a rejected construct nor an exceeded execution + limit may escape as an exception here: the selector is reported as a non-match + with a warning instead. The loud, actionable rejection lives where a human can + act on it — ``compile_name_selector`` still raises during manifest validation, + and the resolver warns and ignores an unsafe manifest that is already on disk. + """ + try: + compiled = compile_name_selector(name) + except REGEX_COMPILE_ERRORS as exc: + # Includes SelectorSafetyError plus the parser's own syntax errors + # (unbalanced group, "a{2,1}", a dangling backreference, ...), so a + # malformed selector degrades the same way an unsafe one does. + _warn_ignored_selector(name, concrete_name, exc) + return False + if compiled is None: + return name == concrete_name + try: + return compiled.matches(concrete_name) + except REGEX_COMPILE_ERRORS as exc: + _warn_ignored_selector(name, concrete_name, exc) + return False + + +def _warn_ignored_selector(selector: str, concrete_name: str, exc: Exception) -> None: + """Report a selector that could not be evaluated, without failing the caller.""" + import warnings + + warnings.warn( + f"Ignoring regex selector {selector!r} for {concrete_name!r}: {exc}", + stacklevel=3, + ) diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index b6b7b2848f..bab6a04621 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -12,6 +12,7 @@ from .._installed_list_json import InstalledListJSONCommand, emit_json, emit_json_error from .._project import resolve_specify_project_root from ._commands import preset_app +from ._manifest import PresetValidationError @preset_app.command("info", cls=InstalledListJSONCommand) @@ -65,6 +66,19 @@ def preset_info( # Check if installed locally first manager = PresetManager(project_root) local_pack = manager.get_pack(preset_id) + if local_pack is None and manager.registry.is_installed(preset_id): + # Installed but unreadable — for example a `regex:` selector rejected + # by the selector safety boundary. ``get_pack`` keeps its Optional + # contract so the listing/inventory paths stay usable, so surface the + # precise validation reason here rather than reporting the preset as + # missing. + from ._manifest import PresetManifest + + try: + PresetManifest(manager.presets_dir / preset_id / "preset.yml") + except PresetValidationError as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc if local_pack: console.print( @@ -79,12 +93,33 @@ 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}") + from ._selectors import is_regex_selector + from ._resolver import PresetResolver + + 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"])) 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] + ) + 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_resolve.py b/src/specify_cli/presets/command_resolve.py index 7256307160..43d571fdaf 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -20,6 +20,7 @@ def preset_resolve( """Show which template will be resolved for a given name.""" from .. import _require_specify_project from . import PresetResolver + from ._selectors import is_regex_selector is_command = "." in template_name valid_name = ( @@ -55,6 +56,18 @@ def preset_resolve( f" [dim](top layer from: " f"{_escape_markup(str(display_layer['source']))})[/dim]" ) + # Selector attribution: when the winning layer was produced by a + # ``regex:`` declaration, name the declaration that matched this + # concrete resource. The acceptance criteria require resolution + # diagnostics, not only ``preset info``, to expose selector matches. + display_declaration = display_layer.get("declaration") + if isinstance(display_declaration, str) and is_regex_selector( + display_declaration + ): + console.print( + f" [dim](declaration {_escape_markup(display_declaration)} " + f"matched {safe_template_name})[/dim]" + ) has_composition = layers[0]["strategy"] != "replace" and any( layer["strategy"] != "replace" for layer in layers @@ -95,13 +108,21 @@ def preset_resolve( strategy_label = layer["strategy"] if strategy_label == "replace" and i == 0: strategy_label = "base" + # Attribute a selector-contributed layer to its declaration so a + # stack trace explains *why* the layer is present. + declaration = layer.get("declaration") + attribution = ( + f" (via {_escape_markup(str(declaration))})" + if isinstance(declaration, str) and is_regex_selector(declaration) + else "" + ) # Escape the literal bracket (\[) so Rich renders `[]` # instead of parsing it as a style tag and swallowing the label, # mirroring `workflow info`'s step-graph line. console.print( f" {i + 1}. \\[{_escape_markup(str(strategy_label))}] " f"{_escape_markup(str(layer['source']))} → " - f"{_escape_markup(str(layer['path']))}" + f"{_escape_markup(str(layer['path']))}{attribution}" ) else: # No layers found — fall back to resolve_with_source for non-composition cases diff --git a/tests/specify_cli/presets/_helpers.py b/tests/specify_cli/presets/_helpers.py index 32cb132adb..d4b9789ffe 100644 --- a/tests/specify_cli/presets/_helpers.py +++ b/tests/specify_cli/presets/_helpers.py @@ -14,6 +14,7 @@ PresetCatalogEntry, PresetManager, PresetManifest, + PresetRegistry, ) REPO_ROOT = Path(__file__).parents[3] @@ -252,3 +253,141 @@ def _create_multi_command_preset_with_aliases(self, temp_dir, preset_id, command with open(preset_dir / "preset.yml", "w") as f: yaml.dump(manifest_data, f) return preset_dir + + +def selector_manifest( + name: str, resource_type: str = "command", 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_selector_preset( + project, + preset_id: str, + name: str, + *, + priority: int, + strategy: str = "replace", + resource_type: str = "command", + body: str = "overlay\n", +): + root = project / ".specify" / "presets" / preset_id + root.mkdir(parents=True, exist_ok=True) + data = selector_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 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 = selector_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_selector_fixture(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 + + +def write_selector_contract_preset(project, identifier, declarations, priority=1): + root = project / ".specify" / "presets" / identifier + root.mkdir(parents=True, exist_ok=True) + entries = [] + for index, (name, resource_type, strategy, body) in enumerate(declarations): + filename = f"payload-{index}.{'sh' if resource_type == 'script' else 'md'}" + if body is not None: + (root / filename).write_text(body, encoding="utf-8") + entries.append( + { + "name": name, + "type": resource_type, + "strategy": strategy, + "file": filename, + } + ) + (root / "preset.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "preset": { + "id": identifier, + "name": identifier, + "version": "1.0.0", + "description": "Selector contract regression", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": entries}, + } + ), + encoding="utf-8", + ) + PresetRegistry(project / ".specify" / "presets").add( + identifier, {"enabled": True, "priority": priority, "version": "1.0.0"} + ) + return root + + +def write_diagnostic_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 diff --git a/tests/specify_cli/presets/test_command_info.py b/tests/specify_cli/presets/test_command_info.py index 1379e6e3a1..bc77abf44b 100644 --- a/tests/specify_cli/presets/test_command_info.py +++ b/tests/specify_cli/presets/test_command_info.py @@ -4,7 +4,9 @@ from typing import ClassVar import yaml +from typer.testing import CliRunner +from specify_cli import app from specify_cli.presets import ( PresetCatalog, PresetManager, @@ -13,6 +15,7 @@ from tests.specify_cli.presets._helpers import ( default_catalog_entries, seed_catalog, + write_diagnostic_selector_preset, ) @@ -248,3 +251,75 @@ def test_unbalanced_markup_does_not_crash_list_or_info(self, temp_dir, project_d result = self._invoke(project_dir, args) assert result.exit_code == 0, (args, result.output, result.exception) assert "Broken [/red] tag" in strip_ansi(result.output) + + +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_matches_bundled_core_without_local_commands(project_dir): + preset_dir = write_diagnostic_selector_preset( + project_dir, + "bundled-commands", + [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "file": "commands/overlay.md", + "strategy": "append", + } + ], + ) + import shutil + + shutil.rmtree(project_dir / ".specify" / "templates" / "commands") + output = _info(project_dir, preset_dir.name) + assert "speckit.plan" in output + assert "No current matches" not in output + + +def test_preset_info_reports_unmatched_regex_selector(project_dir): + preset_dir = write_diagnostic_selector_preset( + project_dir, + "empty-diagnostic-selector", + [ + { + "type": "command", + "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_diagnostic_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 diff --git a/tests/specify_cli/presets/test_command_resolve.py b/tests/specify_cli/presets/test_command_resolve.py index 42e55d572a..a7f5ba39d6 100644 --- a/tests/specify_cli/presets/test_command_resolve.py +++ b/tests/specify_cli/presets/test_command_resolve.py @@ -3,11 +3,14 @@ from pathlib import Path import yaml +from typer.testing import CliRunner +from specify_cli import app from specify_cli.presets import ( PresetManager, ) from tests.conftest import strip_ansi +from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset class TestPresetResolve: @@ -214,3 +217,24 @@ def test_resolve_renders_composition_strategy_labels(self, temp_dir, project_dir assert "Composition chain" in output, output assert "[base]" in output, output assert "[append]" in output, output + + +def test_resolve_reports_command_selector_declaration(project_dir): + from unittest.mock import patch + + write_diagnostic_selector_preset( + project_dir, + "attributed-command", + [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "file": "commands/overlay.md", + "strategy": "append", + } + ], + ) + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["preset", "resolve", "speckit.plan"]) + assert result.exit_code == 0, result.output + assert r"regex:^speckit\.plan$" in strip_ansi(result.output) diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 91e0805593..377b525b05 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -1,7 +1,10 @@ """Tests for preset installation and removal in specify_cli.presets._manager.""" import json +import subprocess +import sys import tarfile +import warnings import zipfile from pathlib import Path @@ -21,6 +24,7 @@ PresetArtifactTestHelpers, install_constitution_sync_preset, install_self_test_preset, + write_selector_preset, ) from tests.specify_cli.presets._helpers import ( make_convention_constitution_preset as _make_convention_constitution_preset, @@ -1607,3 +1611,96 @@ def test_remove_restores_lower_priority_command( cmd_files = list(gemini_dir.glob("*specify*")) assert cmd_files, "Command file should still exist after removal" assert "Lo content" in cmd_files[0].read_text() + + +def test_unmatched_selector_with_own_exact_declaration_does_not_warn(project_dir, tmp_path): + """An exact declaration still takes effect without a spurious no-match warning.""" + + from specify_cli import save_init_options + from specify_cli.presets import PresetResolver + from tests.specify_cli.presets._helpers import write_command_declarations as _write_command_declarations + + save_init_options(project_dir, {"ai": "amp", "script": "sh"}) + commands = project_dir / ".agents/commands" + commands.mkdir(parents=True) + source = _write_command_declarations(tmp_path / "source", "same-target", [ + {"type": "command", "name": "regex:^new$", "body": "INELIGIBLE SELECTOR"}, + {"type": "command", "name": "new", "body": "EXACT DECLARATION"}, + ]) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + PresetManager(project_dir).install_from_directory(source, "0.1.0") + assert not any("matches no command resources" in str(w.message) for w in caught) + assert "EXACT DECLARATION" in (commands / "new.md").read_text() + assert "INELIGIBLE SELECTOR" not in (commands / "new.md").read_text() + layers = PresetResolver(project_dir).collect_all_layers("new", "command") + assert len(layers) == 1 + assert layers[0]["path"].name == "command-1.md" + + +@pytest.mark.parametrize("entry", ["install", "info", "resolve"]) +def test_persisted_unsafe_manifest_is_bounded(project_dir, entry): + pack = write_selector_preset( + project_dir, "unsafe", "regex:(a+)+$", priority=1, resource_type="command" + ) + code = f""" +import os +import warnings +from pathlib import Path +from typer.testing import CliRunner +from specify_cli import app +from specify_cli.presets import PresetManager, PresetResolver, PresetValidationError +root = Path({str(project_dir)!r}) +pack = Path({str(pack)!r}) +entry = {entry!r} +if entry == "install": + try: + PresetManager(root).install_from_directory(pack, "0.1.5") + except PresetValidationError as exc: + assert "safety boundary" in str(exc) + else: + raise AssertionError("unsafe selector installed") +elif entry == "info": + os.chdir(root) + result = CliRunner().invoke(app, ["preset", "info", "unsafe"]) + assert result.exit_code != 0 + assert "safety boundary" in result.output + str(result.exception), result.output +else: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + assert PresetResolver(root).resolve_content("a" * 100 + "!", "command") is None + assert any("Ignoring unsafe preset manifest" in str(w.message) and "safety boundary" in str(w.message) for w in caught) +""" + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, timeout=10 + ) + assert result.returncode == 0, result.stderr + + +def test_persisted_unsafe_manifest_keeps_read_only_paths_usable(project_dir): + """A rejected selector must not break consumers that read "None" as unreadable. + + ``PresetManager.get_pack`` is an Optional accessor. The artifact inventory and + the bundle reference checks all treat "manifest failed validation" as ``None``; + only ``preset info`` re-reads the manifest to report the precise reason. A + persisted unsafe selector must therefore not turn every other read-only + command into an unhandled validation error — and the preset must stay + removable, or the project could not recover. + """ + write_selector_preset( + project_dir, "unsafe", "regex:(a+)+$", priority=1, resource_type="command" + ) + manager = PresetManager(project_dir) + assert manager.registry.is_installed("unsafe") + + # Optional contract preserved: no raise (this raised before the fix). + assert manager.get_pack("unsafe") is None + + # The real read-only consumer keeps enumerating the project. + from specify_cli.artifacts.catalog import ArtifactCatalog + + assert isinstance(ArtifactCatalog(project_dir).list_artifacts(), list) + + # Recovery must still be possible. + assert manager.remove("unsafe") is True + assert not (project_dir / ".specify" / "presets" / "unsafe").exists() diff --git a/tests/specify_cli/presets/test_manager_commands.py b/tests/specify_cli/presets/test_manager_commands.py index d83ef12d17..284a9d29fd 100644 --- a/tests/specify_cli/presets/test_manager_commands.py +++ b/tests/specify_cli/presets/test_manager_commands.py @@ -1,14 +1,25 @@ """Tests for preset command artifacts in specify_cli.presets._manager_commands.""" +import json from pathlib import Path import pytest import yaml -from specify_cli.presets import PresetManager +from specify_cli.extensions import CORE_COMMAND_NAMES, ExtensionRegistry +from specify_cli.presets import ( + PresetManager, + PresetManifest, + PresetRegistry, + PresetResolver, +) from tests.specify_cli.presets._helpers import ( PresetArtifactTestHelpers, + install_selector_fixture, install_self_test_preset, + selector_manifest, + write_command_declarations, + write_selector_preset, ) @@ -3158,3 +3169,430 @@ def test_install_composes_extension_command_and_rewrites_subdir_paths_for_non_sk assert ".specify/extensions/fakeext/agents/control/commander.md" in content assert "Read agents/control" not in content assert "## Extra" in content + + +@pytest.mark.parametrize("change", ["disable", "remove"]) +@pytest.mark.parametrize("agent,skills", [("amp", False), ("copilot", True), ("claude", True)]) +def test_selector_refresh_retires_former_extension_match(project_dir, change, agent, skills): + """Refresh must include prior matches after their lower layer disappears.""" + import shutil + + from specify_cli import save_init_options + from specify_cli.extensions import ExtensionRegistry + from tests.specify_cli.presets._helpers import write_selector_preset as _write_preset + + save_init_options(project_dir, {"ai": agent, "ai_skills": skills, "script": "sh"}) + relative = {"amp": ".agents/commands", "copilot": ".github/skills", "claude": ".claude/skills"}[agent] + output_dir = project_dir / relative + output_dir.mkdir(parents=True) + extension = project_dir / ".specify/extensions/demo" + (extension / "commands").mkdir(parents=True) + (extension / "commands/speckit.demo.collect.md").write_text("EXTENSION BASE\n") + registry = ExtensionRegistry(project_dir / ".specify/extensions") + registry.add("demo", {"enabled": True, "priority": 10, "version": "1.0.0"}) + _write_preset(project_dir, "selector", r"regex:^speckit\.demo\.collect$", priority=1, body="SELECTOR BODY\n") + manager = PresetManager(project_dir) + manager.register_enabled_presets_for_agent(agent) + output = output_dir / ("speckit-demo-collect/SKILL.md" if skills else "speckit.demo.collect.md") + assert "SELECTOR BODY" in output.read_text() + + if change == "disable": + registry.update("demo", {"enabled": False}) + else: + registry.remove("demo") + shutil.rmtree(extension) + manager.register_enabled_presets_for_agent(agent) + + assert not output.exists() + metadata = manager.registry.get("selector") + assert "speckit.demo.collect" not in metadata.get("registered_commands", {}).get(agent, []) + assert "speckit-demo-collect" not in metadata.get("registered_skills", {}).get(agent, []) + + +def test_command_regex_expands_core_commands_to_exact_names(project_dir): + selector_pack = write_selector_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_selector_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 = selector_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_selector_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_selector_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_selector_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_selector_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_selector_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_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_selector_fixture(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_selector_fixture(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" + ) + + +@pytest.mark.parametrize( + "agent,skills", + [("amp", False), ("claude", True), ("kimi", False), ("copilot", True)], +) +def test_selector_install_reregister_and_remove_restore_lower_command( + project_dir, tmp_path, agent, skills +): + from specify_cli import save_init_options + + directories = { + "amp": ".agents/commands", + "claude": ".claude/skills", + "kimi": ".kimi-code/skills", + "copilot": ".github/skills", + } + output_root = project_dir / directories[agent] + output_root.mkdir(parents=True, exist_ok=True) + save_init_options(project_dir, {"ai": agent, "ai_skills": skills, "script": "sh"}) + lower = tmp_path / "lower-source" + lower.mkdir() + write_command_declarations( + lower, + "lower-command", + [ + { + "type": "command", + "name": "speckit.plan", + "strategy": "replace", + "body": "LOWER COMMAND", + }, + ], + ) + upper = tmp_path / "upper-source" + upper.mkdir() + write_command_declarations( + upper, + "selector-command", + [ + { + "type": "command", + "name": r"regex:^speckit\.plan$", + "strategy": "replace", + "body": "FIRST SELECTOR", + }, + { + "type": "command", + "name": r"regex:speckit\.(plan|tasks)", + "strategy": "replace", + "body": "SECOND SELECTOR", + }, + ], + ) + manager = PresetManager(project_dir) + manager.install_from_directory(lower, "0.1.0", priority=20) + manager.install_from_directory(upper, "0.1.0", priority=10) + output = output_root / ( + "speckit.plan.md" if agent == "amp" else "speckit-plan/SKILL.md" + ) + assert "FIRST SELECTOR" in output.read_text() + assert "SECOND SELECTOR" not in output.read_text() + manager.register_enabled_presets_for_agent(agent) + assert "FIRST SELECTOR" in output.read_text() + assert "regex:" not in json.dumps( + manager.registry.get("selector-command")["registered_commands"] + ) + manager.remove("selector-command") + assert "LOWER COMMAND" in output.read_text() + + +@pytest.mark.parametrize( + "agent,skills", + [("amp", False), ("claude", True), ("copilot", True), ("copilot", False)], +) +def test_selector_tracks_the_concrete_matches_it_writes(project_dir, tmp_path, agent, skills): + """A selector materializes and tracks its concrete matches, never the raw pattern. + + The overwrite-policy assertions this test used to make were removed together with + the ownership subsystem, which the maintainer asked to drop from this change. + """ + from specify_cli import save_init_options + + directories = { + "amp": ".agents/commands", + "claude": ".claude/skills", + "copilot": ".github/skills" if skills else ".github/agents", + } + root = project_dir / directories[agent] + root.mkdir(parents=True, exist_ok=True) + save_init_options(project_dir, {"ai": agent, "ai_skills": skills, "script": "sh"}) + suffix = ( + "-plan/SKILL.md" + if skills + else (".plan.agent.md" if agent == "copilot" else ".plan.md") + ) + target = root / f"speckit{suffix}" + target.parent.mkdir(parents=True, exist_ok=True) + source = write_command_declarations( + tmp_path / "source", + "guarded-selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.(plan|tasks)$", + "body": "SELECTOR BODY", + "strategy": "replace", + }, + ], + ) + manager = PresetManager(project_dir) + manager.install_from_directory(source, "0.1.0") + manager.register_enabled_presets_for_agent(agent) + metadata = manager.registry.get("guarded-selector") + tracked = [ + name + for key in ("registered_commands", "registered_skills") + for names in metadata[key].values() + for name in names + ] + assert any("plan" in name for name in tracked) + assert any("tasks" in name for name in tracked) + assert "regex:" not in json.dumps(tracked) + assert "SELECTOR BODY" in target.read_text() + manager.remove("guarded-selector") + assert "SELECTOR BODY" not in target.read_text() + + +@pytest.mark.parametrize("agent,skills", [("amp", False), ("copilot", True), ("claude", True)]) +def test_selector_refresh_keeps_available_matches_and_own_exact_command(project_dir, tmp_path, agent, skills): + """Losing one provider must not retire a surviving match or its exact replacement.""" + from specify_cli import save_init_options + + save_init_options(project_dir, {"ai": agent, "ai_skills": skills, "script": "sh"}) + relative = {"amp": ".agents/commands", "copilot": ".github/skills", "claude": ".claude/skills"}[agent] + output_dir = project_dir / relative + output_dir.mkdir(parents=True) + extension = project_dir / ".specify/extensions/demo" + (extension / "commands").mkdir(parents=True) + (extension / "commands/speckit.demo.collect.md").write_text("EXTENSION BASE\n") + registry = ExtensionRegistry(project_dir / ".specify/extensions") + registry.add("demo", {"enabled": True, "priority": 10, "version": "1.0.0"}) + source = write_command_declarations(tmp_path / "source", "selector", [ + {"type": "command", "name": r"regex:^speckit\.(demo\.collect|plan)$", "body": "SELECTOR BODY\n"}, + {"type": "command", "name": "speckit.demo.collect", "body": "EXACT BODY\n"}, + ]) + manager = PresetManager(project_dir) + manager.install_from_directory(source, "0.1.0") + output = output_dir / ("speckit-demo-collect/SKILL.md" if skills else "speckit.demo.collect.md") + surviving = output_dir / ("speckit-plan/SKILL.md" if skills else "speckit.plan.md") + assert "SELECTOR BODY" in output.read_text() + assert "SELECTOR BODY" in surviving.read_text() + + registry.update("demo", {"enabled": False}) + # Reopen the manager to prove that prior matches survive between operations. + manager = PresetManager(project_dir) + manager.register_enabled_presets_for_agent(agent) + + assert "EXACT BODY" in output.read_text() + assert "SELECTOR BODY" not in output.read_text() + assert "SELECTOR BODY" in surviving.read_text() + assert manager.registry.get("selector")["selector_commands"][agent] == ["speckit.plan"] diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 7ae833bfb9..e30630d850 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2805,3 +2805,83 @@ def test_register_skills_preset_scripts_take_precedence_over_core(self, project_ # Preset's scripts must not be overwritten by core assert preset_fm["scripts"] == {"sh": "preset-run.sh"} + + +def test_reconcile_skills_keeps_winning_selector_declaration(project_dir): + """Concrete-name filtering must retain the expanded winning selector.""" + from specify_cli import save_init_options + from tests.specify_cli.presets._helpers import write_selector_preset as _write_preset + + save_init_options(project_dir, {"ai": "copilot", "ai_skills": True, "script": "sh"}) + output = project_dir / ".github/skills/speckit-plan/SKILL.md" + output.parent.mkdir(parents=True) + output.write_text("---\nmetadata:\n source: preset:selector\n---\nOLD BODY\n") + _write_preset(project_dir, "selector", r"regex:^speckit\.plan$", priority=1, body="WINNING SELECTOR\n") + manager = PresetManager(project_dir) + manager.registry.update("selector", {"registered_skills": {"copilot": ["speckit-plan"]}}) + + written = manager._reconcile_skills(["speckit.plan"], target_agent="copilot") + + assert written == {"speckit.plan"} + assert "WINNING SELECTOR" in output.read_text() + assert "OLD BODY" not in output.read_text() + + +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_manifest.py b/tests/specify_cli/presets/test_manifest.py index 27ac723daa..64bdc89bc9 100644 --- a/tests/specify_cli/presets/test_manifest.py +++ b/tests/specify_cli/presets/test_manifest.py @@ -1,13 +1,21 @@ """Tests for preset manifest validation in specify_cli.presets._manifest.""" +import re + import pytest import yaml from specify_cli.presets import ( VALID_PRESET_TEMPLATE_TYPES, + PresetManager, PresetManifest, PresetValidationError, ) +from specify_cli.presets._selectors import selector_matches +from tests.specify_cli.presets._helpers import ( + selector_manifest, + write_selector_contract_preset, +) class TestPresetManifest: @@ -553,3 +561,93 @@ def test_prepend_accepted_for_commands(self, temp_dir, valid_pack_data): yaml.dump(valid_pack_data, f) manifest = PresetManifest(manifest_path) assert manifest.templates[0]["strategy"] == "prepend" + + +INVALID_PATTERNS = [ + pytest.param("[unterminated", re.error, id="grammar"), + pytest.param("a{999999999999999999999}", OverflowError, id="huge-repetition"), + pytest.param("(" * 2000 + "a" + ")" * 2000, RecursionError, id="deep-nesting"), +] + + +def test_exact_name_manifest_still_validates(tmp_path): + path = tmp_path / "preset.yml" + path.write_text(yaml.safe_dump(selector_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(selector_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(selector_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(selector_manifest("regex:[unterminated")), encoding="utf-8") + with pytest.raises(PresetValidationError, match="Invalid regex selector"): + PresetManifest(path) + + +@pytest.mark.parametrize("resource_type", ["template", "script"]) +def test_non_command_regex_is_rejected_before_install(tmp_path, resource_type): + source = tmp_path / "source" + source.mkdir() + (source / "preset.yml").write_text( + yaml.safe_dump(selector_manifest("regex:.*", resource_type)), encoding="utf-8" + ) + with pytest.raises(PresetValidationError, match="only supported for command"): + PresetManager(tmp_path).install_from_directory(source, "0.1.0") + assert not (tmp_path / ".specify" / "presets" / "preset").exists() + + +@pytest.mark.parametrize("resource_type", ["command"]) +@pytest.mark.parametrize( + "pattern", ["(a+)+$", "(a|a)*$", "(a*)*$", r"(a)\1", r"(?=a)a"] +) +def test_every_regex_declaration_validated(tmp_path, resource_type, pattern): + data = selector_manifest("regex:valid.*", resource_type) + data["provides"]["templates"].append( + { + **data["provides"]["templates"][0], + "name": f"regex:{pattern}", + } + ) + path = tmp_path / "preset.yml" + path.write_text(yaml.safe_dump(data)) + with pytest.raises(PresetValidationError, match="safety boundary"): + PresetManifest(path) + + +@pytest.mark.parametrize("pattern,compile_error", INVALID_PATTERNS) +def test_regex_compile_failures_become_manifest_validation_errors( + tmp_path, pattern, compile_error +): + pack = write_selector_contract_preset( + tmp_path, "malformed", [(f"regex:{pattern}", "command", "replace", "bad")] + ) + with pytest.raises(PresetValidationError, match="Invalid regex selector") as caught: + PresetManifest(pack / "preset.yml") + assert isinstance(caught.value.__cause__, compile_error) + + +@pytest.mark.parametrize( + "selector", ["regex:a{1000}", "regex:(?:selector-)?contract", "selector-contract"] +) +def test_valid_regex_and_exact_names_remain_accepted(tmp_path, selector): + pack = write_selector_contract_preset(tmp_path, "valid", [(selector, "command", "replace", "valid")]) + assert PresetManifest(pack / "preset.yml").templates[0]["name"] == selector diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index d1e416f7a1..bbcf6a6f63 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -1,6 +1,7 @@ """Tests for preset layer resolution and composition in specify_cli.presets._resolver.""" import json +import re from pathlib import Path import pytest @@ -9,13 +10,18 @@ from specify_cli.extensions import ExtensionRegistry from specify_cli.presets import ( PresetManager, + PresetManifest, PresetRegistry, PresetResolver, PresetValidationError, ) from tests.specify_cli.presets._helpers import ( CORE_TEMPLATE_NAMES, + install_selector_fixture, install_self_test_preset, + write_command_declarations, + write_selector_contract_preset, + write_selector_preset, ) @@ -1800,3 +1806,778 @@ def _create_pack(temp_dir, valid_pack_data, pack_id, content, (subdir / f"{template_name}.md").write_text(content) return pack_dir + + +def test_command_regex_matches_core_resource_only(project_dir): + write_selector_preset( + project_dir, + "regex-pack", + "regex:^speckit\.plan$", + priority=10, + strategy="append", + ) + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + assert any(layer["source"].startswith("regex-pack") for layer in layers) + assert ( + PresetResolver(project_dir).collect_all_layers("does-not-exist", "command") + == [] + ) + + +def test_command_regex_matches_extension_resource(project_dir): + ext = project_dir / ".specify" / "extensions" / "demo" + (ext / "templates" / "commands").mkdir(parents=True) + (ext / "templates" / "commands" / "speckit.plan.md").write_text( + "extension base\n", encoding="utf-8" + ) + write_selector_preset( + project_dir, + "regex-pack", + "regex:^speckit\.plan$", + priority=10, + strategy="append", + ) + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + assert any(layer["source"].startswith("regex-pack") for layer in layers) + + +def test_command_regex_matches_lower_priority_preset(project_dir): + write_selector_preset( + project_dir, "high", "regex:^speckit\.plan$", priority=1, strategy="append" + ) + write_selector_preset(project_dir, "low", "speckit.plan", priority=10) + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + 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_selector_preset(project_dir, "higher", "only-higher", priority=1) + write_selector_preset(project_dir, "lower", "regex:^only-higher$", priority=10) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("only-higher", "command") + 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", "command") == [] + assert resolver.collect_all_layers("regex:^only-higher$", "command") == [] + + +def test_same_priority_regex_layers_follow_preset_id_order(project_dir): + # Registry tie-break is alphabetical ID; both selectors match the same core command. + write_selector_preset( + project_dir, + "zeta-regex", + "regex:^speckit.plan$", + priority=10, + strategy="append", + body="zeta\n", + ) + write_selector_preset( + project_dir, + "alpha-regex", + "regex:^speckit.plan$", + priority=10, + strategy="append", + body="alpha\n", + ) + layers = PresetResolver(project_dir).collect_all_layers("speckit.plan", "command") + 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 / "speckit.foo.md").write_text("override\n", encoding="utf-8") + write_selector_preset( + project_dir, + "regex-pack", + "regex:^speckit.foo$", + priority=10, + strategy="append", + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("speckit.foo", "command") + 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 / "speckit.foo.md").write_text("override\n", encoding="utf-8") + write_selector_preset( + project_dir, + "regex-pack", + "regex:^speckit.foo$", + priority=10, + strategy="append", + ) + core = project_dir / ".specify" / "templates" / "commands" / "speckit.foo.md" + core.parent.mkdir(parents=True, exist_ok=True) + core.write_text("core\n", encoding="utf-8") + layers = PresetResolver(project_dir).collect_all_layers("speckit.foo", "command") + 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_selector_preset( + project_dir, + "higher-regex", + "regex:^speckit.plan$", + priority=1, + strategy="append", + body="A\n", + ) + write_selector_preset( + project_dir, + "higher-exact", + "speckit.plan", + priority=2, + strategy="prepend", + body="B\n", + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("speckit.plan", "command") + assert [layer["source"].split(" ")[0] for layer in layers[:2]] == [ + "higher-regex", + "higher-exact", + ] + result = resolver.resolve_content("speckit.plan", "command") + assert result is not None and "A" in result and "B" in result + + +def test_zero_match_is_non_fatal_and_has_no_layer(project_dir): + write_selector_preset(project_dir, "no-match", "regex:^missing-resource$", priority=10) + assert ( + PresetResolver(project_dir).collect_all_layers("missing-resource", "command") + == [] + ) + + +def _name(resource_type): + return ( + "speckit.selector-contract" + if resource_type == "command" + else "selector-contract" + ) + + +def _assert_parity( + resolver, name, resource_type, expected_paths, expected_sources, content +): + layers = resolver.collect_all_layers(name, resource_type) + assert [layer["path"] for layer in layers] == expected_paths + assert [layer["source"] for layer in layers] == expected_sources + assert resolver.resolve(name, resource_type) == expected_paths[0] + assert resolver.resolve_with_source(name, resource_type) == { + "path": str(expected_paths[0]), + "source": expected_sources[0], + } + assert resolver.resolve_content(name, resource_type) == content + + +@pytest.mark.parametrize("resource_type", ["command"]) +@pytest.mark.parametrize("order", ["regex-exact", "exact-regex", "regex-regex"]) +def test_overlapping_declarations_share_manifest_order(tmp_path, resource_type, order): + name = _name(resource_type) + regex = f"regex:{re.escape(name)}" + names = { + "regex-exact": [regex, name], + "exact-regex": [name, regex], + "regex-regex": [regex, f"regex:^{re.escape(name)}$"], + }[order] + pack = write_selector_contract_preset( + tmp_path, + "selector", + [ + (entry, resource_type, "replace", f"body-{index}") + for index, entry in enumerate(names) + ], + ) + lower = write_selector_contract_preset( + tmp_path, "lower", [(name, resource_type, "replace", "base")], 10 + ) + suffix = "sh" if resource_type == "script" else "md" + _assert_parity( + PresetResolver(tmp_path), + name, + resource_type, + [ + pack / f"payload-0.{suffix}", + pack / f"payload-1.{suffix}", + lower / f"payload-0.{suffix}", + ], + ["selector v1.0.0", "selector v1.0.0", "lower v1.0.0"], + "body-0", + ) + + +@pytest.mark.parametrize("resource_type", ["command"]) +@pytest.mark.parametrize("regex_first", [True, False]) +def test_exact_and_regex_compose_over_lower_without_deduplication( + tmp_path, resource_type, regex_first +): + name = _name(resource_type) + names = [f"regex:{re.escape(name)}", name] + if not regex_first: + names.reverse() + placeholder = "$CORE_SCRIPT" if resource_type == "script" else "{CORE_TEMPLATE}" + pack = write_selector_contract_preset( + tmp_path, + "selector", + [ + (entry, resource_type, "wrap", f"layer-{index}[{placeholder}]") + for index, entry in enumerate(names) + ], + ) + lower = write_selector_contract_preset( + tmp_path, "lower", [(name, resource_type, "replace", "base")], 10 + ) + suffix = "sh" if resource_type == "script" else "md" + _assert_parity( + PresetResolver(tmp_path), + name, + resource_type, + [ + pack / f"payload-0.{suffix}", + pack / f"payload-1.{suffix}", + lower / f"payload-0.{suffix}", + ], + ["selector v1.0.0", "selector v1.0.0", "lower v1.0.0"], + "layer-0[layer-1[base]]", + ) + + +@pytest.mark.parametrize("overlap", ["regex-exact", "regex-regex"]) +def test_overlaps_using_the_same_file_are_not_deduplicated(tmp_path, overlap): + name = _name("command") + pack = write_selector_contract_preset( + tmp_path, + "selector", + [ + (f"regex:{name}", "command", "wrap", "overlay[{CORE_TEMPLATE}]"), + ( + name if overlap == "regex-exact" else f"regex:^{name}$", + "command", + "wrap", + "unused", + ), + ], + ) + manifest_path = pack / "preset.yml" + data = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + data["provides"]["templates"][1]["file"] = "payload-0.md" + manifest_path.write_text(yaml.safe_dump(data), encoding="utf-8") + lower = write_selector_contract_preset(tmp_path, "lower", [(name, "command", "replace", "base")], 10) + _assert_parity( + PresetResolver(tmp_path), + name, + "command", + [pack / "payload-0.md", pack / "payload-0.md", lower / "payload-0.md"], + ["selector v1.0.0", "selector v1.0.0", "lower v1.0.0"], + "overlay[overlay[base]]", + ) + + +@pytest.mark.parametrize("missing", ["exact", "regex", "both"]) +def test_unusable_declarations_skip_to_the_first_usable_layer(tmp_path, missing): + name = _name("command") + pack = write_selector_contract_preset( + tmp_path, + "selector", + [ + ( + name, + "command", + "replace", + None if missing in {"exact", "both"} else "exact", + ), + ( + f"regex:{name}", + "command", + "replace", + None if missing in {"regex", "both"} else "regex", + ), + ], + ) + # A matching declaration still prevents a stray conventional file winning. + (pack / f"{name}.md").write_text("stray", encoding="utf-8") + lower = write_selector_contract_preset(tmp_path, "lower", [(name, "command", "replace", "base")], 10) + paths, sources = [], [] + if missing not in {"exact", "both"}: + paths.append(pack / "payload-0.md") + sources.append("selector v1.0.0") + if missing not in {"regex", "both"}: + paths.append(pack / "payload-1.md") + sources.append("selector v1.0.0") + paths.append(lower / "payload-0.md") + sources.append("lower v1.0.0") + content = {"exact": "regex", "regex": "exact", "both": "base"}[missing] + _assert_parity(PresetResolver(tmp_path), name, "command", paths, sources, content) + + +def test_own_exact_declaration_does_not_make_regex_eligible(tmp_path): + name = _name("command") + pack = write_selector_contract_preset( + tmp_path, + "selector", + [ + (f"regex:{name}", "command", "replace", "ineligible"), + (name, "command", "replace", "exact"), + ], + ) + _assert_parity( + PresetResolver(tmp_path), + name, + "command", + [pack / "payload-1.md"], + ["selector v1.0.0"], + "exact", + ) + + +INVALID_PATTERNS = [ + pytest.param("[unterminated", re.error, id="grammar"), + pytest.param("a{999999999999999999999}", OverflowError, id="huge-repetition"), + pytest.param("(" * 2000 + "a" + ")" * 2000, RecursionError, id="deep-nesting"), +] + + +@pytest.mark.parametrize("pattern,compile_error", INVALID_PATTERNS) +def test_installed_malformed_selector_is_isolated_from_healthy_presets( + tmp_path, pattern, compile_error +): + name = _name("command") + write_selector_contract_preset( + tmp_path, "malformed", [(f"regex:{pattern}", "command", "replace", "bad")] + ) + healthy = write_selector_contract_preset( + tmp_path, "healthy", [(name, "command", "replace", "healthy")], 10 + ) + installed = { + entry["id"]: entry for entry in PresetManager(tmp_path).list_installed() + } + assert set(installed) == {"malformed", "healthy"} + assert installed["malformed"]["description"] == "⚠️ Corrupted preset" + assert installed["malformed"]["enabled"] is False + assert installed["healthy"]["enabled"] is True + _assert_parity( + PresetResolver(tmp_path), + name, + "command", + [healthy / "payload-0.md"], + ["healthy v1.0.0"], + "healthy", + ) + + +def _extension(project_dir, extension_id, filename, *, declared=False): + root = project_dir / ".specify" / "extensions" / extension_id + candidate = root / filename + candidate.parent.mkdir(parents=True, exist_ok=True) + candidate.write_text(f"# {extension_id} base\n", encoding="utf-8") + if declared: + (root / "extension.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": extension_id, + "name": extension_id, + "version": "1.0.0", + "description": "resolver fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.target.collect", + "file": filename, + "description": "collect", + } + ] + }, + } + ), + encoding="utf-8", + ) + return candidate + + +def _preset(project_dir, preset_id, declarations, *, priority=1): + root = project_dir / ".specify" / "presets" / preset_id + root.mkdir(parents=True, exist_ok=True) + (root / "payload.md").write_text("overlay\n", encoding="utf-8") + (root / "preset.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "preset": { + "id": preset_id, + "name": preset_id, + "version": "1.0.0", + "description": "resolver fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": declarations}, + } + ), + encoding="utf-8", + ) + PresetRegistry(root.parent).add( + preset_id, {"version": "1.0.0", "priority": priority, "enabled": True} + ) + return root / "payload.md" + + +@pytest.mark.parametrize("registered", [False, True]) +@pytest.mark.parametrize( + "filename,declared", + [ + ("commands/target.collect.md", False), + ("templates/commands/target.collect.md", False), + ("commands/actual.md", True), + ("commands/speckit.target.collect.md", False), + ], +) +def test_each_extension_candidate_is_emitted_once_with_own_attribution( + project_dir, registered, filename, declared +): + candidate = _extension(project_dir, "target", filename, declared=declared) + # An earlier unrelated orphan must not discover target's namespace fallback. + unrelated = project_dir / ".specify" / "extensions" / "aaa-orphan" + unrelated.mkdir() + if registered: + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "target", {"enabled": True, "version": "1.0.0", "priority": 20} + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("speckit.target.collect", "command") + assert len(layers) == 1 + assert layers[0]["path"] == candidate + assert layers[0]["extension_id"] == "target" + assert layers[0]["extension_dir"] == ( + project_dir / ".specify" / "extensions" / "target" + ) + assert layers[0]["source"] == ( + "extension:target v1.0.0" if registered else "extension:target (unregistered)" + ) + assert resolver.resolve("speckit.target.collect", "command") == candidate + assert ( + resolver.resolve_content("speckit.target.collect", "command") + == "# target base\n" + ) + + +@pytest.mark.parametrize("declared", [False, True]) +def test_disabled_namespace_is_never_revived_by_unrelated_extension( + project_dir, declared +): + _extension(project_dir, "target", "commands/target.collect.md", declared=declared) + (project_dir / ".specify" / "extensions" / "aaa-orphan").mkdir() + registry = ExtensionRegistry(project_dir / ".specify" / "extensions") + registry.add("target", {"enabled": False, "version": "1.0.0"}) + resolver = PresetResolver(project_dir) + assert resolver.collect_all_layers("speckit.target.collect", "command") == [] + assert resolver.resolve("speckit.target.collect", "command") is None + assert resolver.resolve_content("speckit.target.collect", "command") is None + + +@pytest.mark.parametrize("template_type", ["command", "template", "script"]) +def test_missing_manifest_command_never_falls_back_to_legacy_file( + project_dir, template_type +): + candidate = _extension( + project_dir, "target", "commands/target.collect.md", declared=True + ) + manifest_path = candidate.parents[1] / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"][0]["file"] = "commands/missing.md" + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + resolver = PresetResolver(project_dir) + assert resolver.collect_all_layers("speckit.target.collect", template_type) == [] + assert resolver.resolve("speckit.target.collect", template_type) is None + + +@pytest.mark.parametrize("template_type", ["template", "script"]) +def test_legacy_command_is_not_a_template_or_script_layer(project_dir, template_type): + _extension(project_dir, "target", "commands/target.collect.md") + resolver = PresetResolver(project_dir) + assert resolver.collect_all_layers("speckit.target.collect", template_type) == [] + assert resolver.resolve("speckit.target.collect", template_type) is None + assert resolver.resolve_content("speckit.target.collect", template_type) is None + + +@pytest.mark.parametrize("enabled", [False, True]) +def test_regex_eligibility_uses_enabled_legacy_namespace_base(project_dir, enabled): + candidate = _extension(project_dir, "target", "commands/target.collect.md") + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "target", {"enabled": enabled, "version": "1.0.0"} + ) + overlay = _preset( + project_dir, + "selector", + [ + { + "type": "command", + "name": r"regex:^speckit\.target\.collect$", + "file": "payload.md", + "strategy": "append", + } + ], + ) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("speckit.target.collect", "command") + if enabled: + assert [layer["path"] for layer in layers] == [overlay, candidate] + assert resolver.resolve("speckit.target.collect", "command") == overlay + assert resolver.resolve_content("speckit.target.collect", "command") == ( + "# target base\n\n\noverlay\n" + ) + else: + assert layers == [] + assert resolver.resolve("speckit.target.collect", "command") is None + assert resolver.resolve_content("speckit.target.collect", "command") is None + + +def test_distinct_providers_and_exact_regex_declarations_are_not_deduplicated( + project_dir, +): + base = _extension(project_dir, "target", "commands/actual.md", declared=True) + other = _extension(project_dir, "other", "commands/actual.md", declared=True) + ExtensionRegistry(project_dir / ".specify" / "extensions").add( + "target", {"enabled": True, "priority": 1, "version": "1.0.0"} + ) + declarations = [ + { + "type": "command", + "name": name, + "file": "payload.md", + "strategy": "append", + } + for name in ("speckit.target.collect", r"regex:^speckit\.target\.collect$") + ] + overlay = _preset(project_dir, "selector", declarations) + lower = _preset(project_dir, "lower", [declarations[0]], priority=2) + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("speckit.target.collect", "command") + assert [layer["path"] for layer in layers] == [overlay, overlay, lower, base, other] + assert resolver.resolve_content("speckit.target.collect", "command") == ( + "# target base\n\n\noverlay\n\n\noverlay\n\n\noverlay\n" + ) + + +def test_lower_preset_conventional_templates_command_is_a_candidate(tmp_path): + """A lower preset's templates/commands/.md must be discoverable. + + _conventional_command_names and _expand_command_selectors both enumerate that + location, for lower presets as much as for extensions, so the shared + conventional lookup the eligibility check relies on has to agree with them. + Before the fix the file was enumerated as a candidate name but resolved to no + layer, so a selector that matched it materialized nothing. + """ + from specify_cli.presets._resolver import PresetResolver + + pack = tmp_path / ".specify" / "presets" / "lower" + (pack / "templates" / "commands").mkdir(parents=True) + body = pack / "templates" / "commands" / "speckit.lower.extra.md" + body.write_text("---\ndescription: lower\n---\nLOWER BODY\n", encoding="utf-8") + + resolver = PresetResolver(tmp_path) + + # The shared lookup resolves it, and the eligibility check agrees. + assert ( + resolver._conventional_resource(pack, "speckit.lower.extra", "command") == body + ) + assert resolver._has_concrete_resource(pack, "speckit.lower.extra", "command") + + # The neighbouring conventional location keeps working. + (pack / "commands").mkdir() + flat = pack / "commands" / "speckit.lower.flat.md" + flat.write_text("---\ndescription: flat\n---\nFLAT BODY\n", encoding="utf-8") + assert ( + resolver._conventional_resource(pack, "speckit.lower.flat", "command") == flat + ) + + # A missing resource stays missing. + assert ( + resolver._conventional_resource(pack, "speckit.lower.absent", "command") is None + ) + + +@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_selector_fixture(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_selector_fixture(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_selector_fixture(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_selector_fixture(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_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_selector_fixture(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") diff --git a/tests/specify_cli/presets/test_selector_engine.py b/tests/specify_cli/presets/test_selector_engine.py new file mode 100644 index 0000000000..f7899bd094 --- /dev/null +++ b/tests/specify_cli/presets/test_selector_engine.py @@ -0,0 +1,336 @@ +"""Parsing and bounded matching tests for the command selector engine.""" + +import re +import subprocess +import sys +import time +import warnings + +import pytest + +from specify_cli.presets._selectors import ( + _MAX_INPUT, + SelectorSafetyError, + compile_name_selector, + selector_matches, +) + + +_PATHOLOGICAL_PAYLOAD = "a*" * 400 + "b" + + +def test_complexity_guard_is_what_answers_the_payload_not_a_limit(): + """Inside every limit, the bounded matcher must answer correctly and fast. + + This runs in a killed-and-reaped child process. If the matcher ever regressed to + a backtracking engine, this payload would keep it busy for effectively forever; + the case must then fail on the timeout rather than hang the suite. + """ + code = f""" +import time +from specify_cli.presets._selectors import _MAX_INPUT, compile_name_selector +text = "a" * _MAX_INPUT +compiled = compile_name_selector("regex:" + {_PATHOLOGICAL_PAYLOAD!r}) +assert compiled is not None, "pattern is outside a cap, so nothing is proven" +assert compiled.edge_count < 2048 +assert len(text) <= _MAX_INPUT, "input is outside the limit, so nothing is proven" +started = time.perf_counter() +assert compiled.matches(text) is False +assert compiled.matches("a" * (_MAX_INPUT - 1) + "b") is True +elapsed = time.perf_counter() - started +assert elapsed < 5.0, f"{{elapsed:.3f}}s for a payload a backtracker cannot do" +""" + try: + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, timeout=30 + ) + except subprocess.TimeoutExpired: + pytest.fail( + "the matcher did not finish this payload inside 30s: the complexity " + "protection has regressed" + ) + assert result.returncode == 0, result.stderr + + +def test_the_payload_is_genuinely_pathological_for_a_backtracking_engine(): + """Effectiveness proof: the payload must defeat plain ``re``, or it proves nothing. + + Without this, the case above could pass simply because the pattern is easy. + ``re.fullmatch`` is run in a killed-and-reaped child, so the cost is bounded. + """ + code = ( + "import re, sys\n" + f"pattern = {_PATHOLOGICAL_PAYLOAD!r}\n" + f"text = 'a' * {_MAX_INPUT}\n" + "print(re.fullmatch(pattern, text) is not None)\n" + ) + started = time.perf_counter() + try: + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, timeout=5 + ) + except subprocess.TimeoutExpired: + return # pathological: killed and reaped, exactly as required + elapsed = time.perf_counter() - started + assert result.returncode != 0 or elapsed > 1.0, ( + "the payload is not pathological for a backtracking engine, so the " + f"complexity case above proves nothing (re finished in {elapsed:.3f}s)" + ) + + +@pytest.mark.parametrize("pattern", ["(a+)+$", "(a|a)*$", "(a*)*$"]) +def test_rejected_payloads_are_refused_loudly(pattern): + with pytest.raises(SelectorSafetyError, match="safety boundary"): + compile_name_selector("regex:" + pattern) + + +@pytest.mark.parametrize("pattern", ["(a+)+$", "(a|a)*$", "(a*)*$"]) +def test_rejected_payloads_never_hang_a_worker(pattern): + """The whole pipeline stays alive and fail-closed, with the input inside the limit.""" + code = f""" +import warnings +from specify_cli.presets._selectors import selector_matches +with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + matched = selector_matches("regex:" + {pattern!r}, "a" * 1024 + "!") + assert matched is False, "unsafe selector matched" + assert any("safety boundary" in str(w.message) for w in caught), [ + str(w.message) for w in caught + ] +""" + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, timeout=10 + ) + assert result.returncode == 0, result.stderr + + +def test_supported_semantics_and_limits(): + start = time.monotonic() + cases = [ + ("plan", "plan"), + ("plan", "plan-template"), + ("regex:plan", "plan-template"), + (r"regex:^speckit\.(plan|tasks)$", "speckit.plan"), + (r"regex:(foo|bar)+[a-z]{2,4}\d?", "foobarxy2"), + (r"regex:(ab)+c*", "ababccc"), + (r"regex:a.*z", "a.b.z"), + (r"regex:a.*z", "a\nz"), + (r"regex:(?s)a.*z", "a\nz"), + (r"regex:(?i:a)(?-i:B)", "aB"), + (r"regex:(?i)a(?-i:B)", "Ab"), + (r"regex:\b\w+\b", "éabc"), + (r"regex:a+?b{1,3}?$", "aaaabb"), + (r"regex:^a$", "a\n"), + (r"regex:(ab|c)?", ""), + ] + for selector, name in cases: + expected = ( + re.fullmatch(selector[6:], name) is not None + if selector.startswith("regex:") + else selector == name + ) + assert selector_matches(selector, name) == expected, (selector, name) + assert selector_matches("x" * 100000, "x" * 100000) + assert selector_matches("regex:a+", "a" * 1000) + # An execution limit is a non-match with a warning, never an exception: the + # callers of selector_matches are boolean predicates in resolution paths. + with pytest.warns(UserWarning, match="input character limit"): + assert selector_matches("regex:.*", "a" * 100000) is False + with pytest.raises(re.error, match="pattern character limit"): + compile_name_selector("regex:" + "a" * 4097) + with pytest.raises(re.error, match="state limit"): + compile_name_selector("regex:a{10000}") + assert time.monotonic() - start < 5 + + +@pytest.mark.parametrize( + ("pattern", "name"), + [ + (r"(a|b)*c|d", "ad"), + (r".*-plan|tasks", "speckit.tasks"), + (r"a*b|c", "ac"), + (r"(?:a*b|c)d", "acd"), + (r"[a-z]*x|y", "xy"), + (r"(a*)b|c", "ac"), + ], +) +def test_quantifier_led_alternation_does_not_leak_into_a_sibling(pattern, name): + """A repetition must not fall out of its own alternative into a sibling branch. + + A repetition with a zero minimum owns its entry state; reusing the enclosing + branch entry as that state let the loop-back edge return to the shared branch + entry, so the partially matched alternative could then follow a *different* + alternative's edge. Every case below was a false positive before the fix, which + would have let a preset selector claim lower-layer resources it must not. + """ + assert re.fullmatch(pattern, name) is None + assert selector_matches(f"regex:{pattern}", name) is False + + +@pytest.mark.parametrize( + ("pattern", "name"), + [ + (r"(a|b)*c", "abc"), + (r".*-plan|tasks", "tasks"), + (r"a*b|c", "b"), + (r"a*b|c", "c"), + (r"[a-z]*x|y", "y"), + (r"(a*)b|c", "c"), + (r"(a*)b|c", "ab"), + ], +) +def test_quantifier_led_alternation_still_accepts_its_own_language(pattern, name): + """Guard against over-correcting the leak fix into false negatives.""" + assert re.fullmatch(pattern, name) is not None + assert selector_matches(f"regex:{pattern}", name) is True + + +@pytest.mark.parametrize("repeats", [50, 100, 200]) +def test_transition_budget_never_turns_a_valid_selector_into_a_non_match(repeats): + """A manifest-valid selector must not stop matching because of the work budget. + + ``"a*" * 200 + "b"`` compiles (well under the pattern and state limits) and + ``re.fullmatch`` accepts it, but a budget tight enough to be exhausted by + (len(input) + 1) x (edges) made ``selector_matches`` report a non-match for a + selector that does match — silently dropping the preset's declaration. + """ + pattern = "a*" * repeats + "b" + concrete_name = "a" * 100 + "b" + assert re.fullmatch(pattern, concrete_name) is not None + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + assert selector_matches(f"regex:{pattern}", concrete_name) is True + assert not caught, [str(w.message) for w in caught] + + +@pytest.mark.parametrize("pattern", ["(", "a{2,1}", "[", r"(a)\2", "a**"]) +def test_malformed_selector_degrades_without_raising(pattern): + """A selector the parser cannot compile must warn and not match, not raise. + + ``selector_matches`` is used as a boolean predicate by every resolution, + installation and diagnostics caller, so a malformed selector has to degrade + the same way an unsafe one does rather than propagate ``re.error``. + """ + with pytest.warns(UserWarning, match="Ignoring regex selector"): + assert selector_matches(f"regex:{pattern}", "x") is False + + +@pytest.mark.parametrize( + ("repeats", "name_length"), [(200, 100), (300, 1000), (200, 1023)] +) +def test_no_fixed_transition_budget_can_deny_a_valid_match(repeats, name_length): + """The work bound is derived, so no finite budget can turn a match into a miss. + + A fixed budget is always reachable by some pattern that validates: 100_000 was + exceeded by ``"a*" * 200 + "b"`` on a 100-character name, and 2_000_000 by + ``"a*" * 800 + "b"`` on a 1,000-character name. Both match under + ``re.fullmatch``, so every case that still fits the caps must match here, with + no warning about a budget. + """ + pattern = "a*" * repeats + "b" + concrete_name = "a" * name_length + "b" + assert re.fullmatch(pattern, concrete_name) is not None + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + assert selector_matches(f"regex:{pattern}", concrete_name) is True + assert not caught, [str(w.message) for w in caught] + + +@pytest.mark.parametrize( + "pattern", + [ + r"(?i:[\u0000-\uffff]){1500}", + r"(?i:[\u0000-\U0010ffff]){1500}", + r"(?u:[^\x00]){1500}", + ], +) +def test_unrolled_repetition_does_not_recompile_each_atom(pattern): + """Unrolling a repetition must reuse one compiled predicate per distinct atom. + + Compiling a large case-folded character set costs a couple of milliseconds, so + asking CPython's compiler once per unrolled copy made a manifest-valid selector + take over eight seconds to compile -- a stall on every command that validates + or resolves a manifest. The budget below is deliberately loose; the fix removes + a factor of 4000, so the margin is two orders of magnitude even on a slow runner. + """ + start = time.monotonic() + compile_name_selector(f"regex:{pattern}") + assert time.monotonic() - start < 2.0 + + +def test_oversized_compiled_graph_is_rejected_before_it_can_be_matched(): + """The graph itself must be capped, not only its state count. + + A branch reuses one entry state for all its alternatives, and a repetition + unrolls those edges, so ``(?:a|\B|\B|...)`` stays under the state cap while + producing millions of edges. Only a state cap let 4004 states carry 4,004,003 + edges, i.e. ~16 billion edge examinations against the largest permitted name. + The edge cap rejects that shape up front instead. + """ + # Modest version: this family is now refused by the semantic rule rather than + # by size -- ``\B`` is zero-width, so the repeated body can match empty and is + # ambiguous however small it is. It is kept here because it is the shape that + # produced the oversized graph, and it must still be refused. + modest = "(?:a|" + "|".join([r"\B"] * 20) + "){20}" + with pytest.raises(re.error, match="can match empty"): + compile_name_selector(f"regex:{modest}") + + # Amplified version: same state magnitude, millions of edges. + amplified = "(?:a|" + "|".join([r"\B"] * 1000) + "){4000}" + with pytest.raises(re.error, match="edge limit"): + compile_name_selector(f"regex:{amplified}") + + # Negative control: the cap is not so tight that ordinary selectors suffer. + for pattern in [r"speckit\.provider\..*", r"^speckit\.(plan|tasks)$", r"a{1000}"]: + assert compile_name_selector(f"regex:{pattern}") is not None + + +@pytest.mark.parametrize( + ("pattern", "name"), + [ + (r"(ab|a)*c", "aabc"), + (r"(ab|a)*c", "abc"), + (r"(ba|b)*c", "bac"), + (r"(a|a){1}c", "ac"), + (r"(a|ab)*c", "aabc"), + (r"(?:ab|a)", "a"), + (r"(?:ab|ac)*c", "abc"), + ], +) +def test_factorised_alternation_under_a_repetition_is_accepted(pattern, name): + """A shared alternative prefix must not make a repetition look empty-bodied. + + CPython factors the prefix out, leaving a trailing branch with an empty + alternative, so ``(ab|a)*`` parses as ``a`` then ``(b|)``. That body consumes + ``a`` in every alternative and cannot match empty, yet the old rule refused it + on the strength of the empty alternative alone -- and refused even a bounded + ``{1}``. The repetition is only ambiguous when its *body* can match empty, or + when every alternative of the factorised branch is empty. + """ + assert re.fullmatch(pattern, name) is not None + assert selector_matches(f"regex:{pattern}", name) is True + + +@pytest.mark.parametrize( + "pattern", + [ + r"(a|a)*$", + r"(a+)+$", + r"(a*)*$", + r"(?:x|x)*", + r"(?:)*", + r"()*", + r"(a|)*", + r"(a|b|)*", + ], +) +def test_empty_or_nested_repetition_is_still_rejected(pattern): + """The legal-expression fix must not lift the rejection of ambiguous shapes.""" + with pytest.raises(re.error, match="safety boundary"): + compile_name_selector(f"regex:{pattern}") + + +@pytest.mark.parametrize("pattern", [r"(a|a){1}c", r"(a|a){2}c", r"(ab|a){1}c"]) +def test_bounded_repetition_of_a_factorised_alternation_is_accepted(pattern): + """A bounded repetition cannot repeat a zero-width alternation without bound.""" + assert compile_name_selector(f"regex:{pattern}") is not None From 0a0b024ab2585b35ec4c5a4e5e1bd2f4f2ed767e Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Thu, 15 Oct 2026 01:40:17 +0800 Subject: [PATCH 2/7] fix(presets): address the four findings from the reduced-scope review - Skill rendering no longer prefers a stale `.composed` artifact left behind by a retired selector match: composed content is reused only while the current winning declaration stack still composes and belongs to this preset, so an exact `replace` fallback is rendered instead. - Every selector compilation cause in `REGEX_COMPILE_ERRORS` is reported for persisted manifests, not only `SelectorSafetyError`; unrelated invalid manifests stay silent. - Manifests whose command declarations are all exact no longer trigger the lower-layer extension scan: exact declarations are collected first and eligibility is calculated only when a regex declaration needs it, so a corrupt extension registry cannot affect exact resolution. - Removed the duplicate conventional lookup that admitted the undocumented `templates/templates/commands/` location; exact resolution and selector candidates now share the documented locations. Regression coverage for each finding lives in the existing surface-owned suites (`test_manager_skills.py`, `test_resolver.py`); all four tests fail before the fix and pass after it. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- src/specify_cli/presets/_manager_skills.py | 14 +++- src/specify_cli/presets/_resolver.py | 72 +++++++++--------- .../presets/test_manager_skills.py | 38 ++++++++++ tests/specify_cli/presets/test_resolver.py | 74 +++++++++++++++++++ 4 files changed, 158 insertions(+), 40 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 77e91c0337..07fa827e8e 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -722,11 +722,17 @@ def _register_skills( continue rendered_commands.add(cmd_name) - # Use composed content if available (written by _register_commands - # for commands with non-replace strategies), otherwise the original. + # A previous selector match may have left composed content behind. + # Reuse it only while this preset still owns the top composing layer. composed_file = preset_dir / ".composed" / f"{cmd_name}.md" if composed_file.exists(): - source_file = composed_file + layers = resolver.collect_all_layers(cmd_name, "command") + if ( + layers + and layers[0]["strategy"] != "replace" + and layers[0]["path"].is_relative_to(preset_dir) + ): + source_file = composed_file # Derive the short command name (e.g. "specify" from "speckit.specify") raw_short_name = cmd_name @@ -773,7 +779,7 @@ def _register_skills( ) if ( effective_strategy != "replace" - and not composed_file.exists() + and source_file != composed_file and resolver.resolve_content(cmd_name, "command") is None ): continue diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 458f2accad..31c7f8a515 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -69,9 +69,9 @@ def _get_manifest(self, pack_dir: Path) -> Optional["PresetManifest"]: try: self._manifest_cache[key] = PresetManifest(manifest_path) except PresetValidationError as exc: - from ._selectors import SelectorSafetyError + from ._selectors import REGEX_COMPILE_ERRORS - if isinstance(exc.__cause__, SelectorSafetyError): + if isinstance(exc.__cause__, REGEX_COMPILE_ERRORS): import warnings warnings.warn( @@ -603,41 +603,45 @@ def _preset_declarations_for_resource( manifest = self._get_manifest(pack_dir) if manifest is 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 [] - 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: - has_lower_resource = any( - self._extension_has_concrete_resource( - self.extensions_dir / ext_id, template_name, template_type - ) - 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 - ) - matches = [] + candidates = [] for declaration in manifest.templates: 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) + regex = is_regex_selector(name) + if (not regex and name == template_name) or ( + regex and selector_matches(name, template_name) + ): + candidates.append((declaration, regex)) + + has_lower_resource = False + if any(regex for _declaration, regex in candidates): + 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 [] + 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 exact or regex: + if not has_lower_resource: + has_lower_resource = any( + self._extension_has_concrete_resource( + self.extensions_dir / ext_id, template_name, template_type + ) + 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 + ) + matches = [] + for declaration, regex in candidates: + if not regex or has_lower_resource: file_value = declaration.get("file") path = pack_dir / file_value if isinstance(file_value, str) else None matches.append( @@ -689,10 +693,6 @@ def _extension_resource( if entry is not None: return candidate candidate = self._conventional_resource(base_dir, name, template_type) - if candidate is None and template_type == "command": - candidate = self._conventional_resource( - base_dir / "templates", name, template_type - ) if candidate is None and template_type == "command": candidate = self._find_unregistered_extension_command(name, base_dir) return candidate diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index e30630d850..41d9bea63b 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2885,3 +2885,41 @@ def test_install_regex_commands_registers_concrete_ai_skills(project_dir): for names in metadata["registered_commands"].values() for name in names ) + + +def test_skills_ignore_stale_composition_after_selector_falls_back_to_exact(project_dir): + from specify_cli import save_init_options + from specify_cli.presets import PresetManifest + from tests.specify_cli.presets._helpers import write_selector_contract_preset + + save_init_options(project_dir, {"ai": "claude", "ai_skills": True, "script": "sh"}) + (project_dir / ".claude" / "skills").mkdir(parents=True) + name = "speckit.fallback" + lower = write_selector_contract_preset( + project_dir, "lower", [(name, "command", "replace", "LOWER BODY\n")], 10 + ) + pack = write_selector_contract_preset( + project_dir, + "selector", + [ + (r"regex:^speckit\.fallback$", "command", "prepend", "SELECTOR BODY\n"), + (name, "command", "replace", "EXACT FALLBACK\n"), + ], + ) + manager = PresetManager(project_dir) + manifest = PresetManifest(pack / "preset.yml") + manager._register_commands(manifest, pack) + manager._register_skills(manifest, pack) + output = project_dir / ".claude" / "skills" / "speckit-fallback" / "SKILL.md" + assert "SELECTOR BODY" in output.read_text() + assert "EXACT FALLBACK" in output.read_text() + composed = pack / ".composed" / f"{name}.md" + assert "SELECTOR BODY" in composed.read_text() + + (lower / "payload-0.md").unlink() + manager._register_commands(manifest, pack) + manager._register_skills(manifest, pack) + + assert "EXACT FALLBACK" in output.read_text() + assert "SELECTOR BODY" not in output.read_text() + assert "LOWER BODY" not in output.read_text() diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index bbcf6a6f63..487d395ef1 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -2,6 +2,7 @@ import json import re +import warnings from pathlib import Path import pytest @@ -2149,6 +2150,79 @@ def test_own_exact_declaration_does_not_make_regex_eligible(tmp_path): ] +@pytest.mark.parametrize("pattern,compile_error", INVALID_PATTERNS) +def test_malformed_selector_manifest_warns_once_but_unrelated_invalid_manifest_is_silent( + tmp_path, monkeypatch, pattern, compile_error +): + from specify_cli.presets import _resolver + + malformed = write_selector_contract_preset( + tmp_path, "malformed", [(f"regex:{pattern}", "command", "replace", "bad")] + ) + errors = {} + + def capture_validation_error(path): + try: + return PresetManifest(path) + except PresetValidationError as exc: + errors[path] = exc + raise + + monkeypatch.setattr(_resolver, "PresetManifest", capture_validation_error) + invalid = write_selector_contract_preset(tmp_path, "invalid", []) + (invalid / "preset.yml").write_text("schema_version: invalid\n", encoding="utf-8") + resolver = PresetResolver(tmp_path) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + assert resolver._get_manifest(malformed) is None + assert resolver._get_manifest(malformed) is None + assert resolver._get_manifest(invalid) is None + + assert len(caught) == 1 + error = errors[malformed / "preset.yml"] + assert isinstance(error.__cause__, compile_error) + assert str(malformed / "preset.yml") in str(caught[0].message) + assert str(error) in str(caught[0].message) + + +def test_exact_command_resolves_without_consulting_corrupt_extension_registry(tmp_path, monkeypatch): + name = "speckit.exact" + pack = write_selector_contract_preset( + tmp_path, "exact", [(name, "command", "replace", "EXACT BODY")] + ) + extensions = tmp_path / ".specify" / "extensions" + extensions.mkdir() + (extensions / ".registry").write_text("{broken", encoding="utf-8") + resolver = PresetResolver(tmp_path) + assert resolver.resolve(name, "command") == pack / "payload-0.md" + + def unexpected_scan(): + pytest.fail("Exact declarations must not scan extensions") + + monkeypatch.setattr(resolver, "_get_all_extensions_by_priority", unexpected_scan) + assert resolver.resolve(name, "command") == pack / "payload-0.md" + + +@pytest.mark.parametrize( + "location,eligible", + [("commands", True), ("templates/commands", True), ("templates/templates/commands", False)], +) +def test_extension_conventional_command_locations_agree_with_selector_candidates( + tmp_path, location, eligible +): + from specify_cli.presets._resolver import _conventional_command_names + + name = "speckit.target.collect" + body = _extension(tmp_path, "target", f"{location}/{name}.md") + resolver = PresetResolver(tmp_path) + extension = resolver.extensions_dir / "target" + + assert (name in _conventional_command_names(extension)) is eligible + assert resolver.resolve(name, "command") == (body if eligible else None) + assert resolver._extension_has_concrete_resource(extension, name, "command") is eligible + + @pytest.mark.parametrize("pattern,compile_error", INVALID_PATTERNS) def test_installed_malformed_selector_is_isolated_from_healthy_presets( tmp_path, pattern, compile_error From ce13b9bb0685ff5b796de0aab389c7b68e3d2b5b Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Thu, 15 Oct 2026 02:56:17 +0800 Subject: [PATCH 3/7] fix(presets): address the four previously-missed review findings - An eligible-by-pattern selector that matches no lower-layer resource is no longer collapsed to nothing. The matching declaration is preserved with no candidate, so callers suppress their convention fallback instead of reading "undeclared" and resolving a conventional file the selector was only meant to augment (`resolve()` and `collect_all_layers()` both follow this). - A selector declaration's own conventional payload no longer counts as a concrete lower resource. `_has_concrete_resource()` now treats a manifest that declares the command - via an exact entry or a matching selector - as authoritative before consulting the convention lookup, so regex declarations cannot chain through each other. - `specify preset resolve ` resolves dotless command names as commands. Command manifests accept names such as `new`, so the presence of a dot can no longer decide the resource type: when the name resolves as a command, the selector attribution path is used instead of reporting it missing. - Documented that only the pattern/state/edge limits invalidate a manifest, while an oversized concrete resource name is a warned non-match at match time. Each fix has regression coverage in the existing suites (`test_resolver.py`, `test_command_resolve.py`); all three new tests fail before the fix and pass after it. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- docs/reference/presets.md | 8 ++- src/specify_cli/presets/_resolver.py | 41 ++++++++++++ src/specify_cli/presets/command_resolve.py | 7 ++ .../presets/test_command_resolve.py | 38 +++++++++++ tests/specify_cli/presets/test_resolver.py | 64 +++++++++++++++++++ 5 files changed, 156 insertions(+), 2 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 23a06fd02a..965d4e6cc0 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -337,8 +337,12 @@ Zero-match and invalid patterns are handled as follows: A `regex:` selector pattern is validated when a preset is installed, and again whenever a selector is evaluated — including a selector found in a manifest that is -already on disk. Validation enforces these limits, and a pattern past one of them is -rejected with a `PresetValidationError` that names the limit it broke: +already on disk. The pattern, compiled-state and compiled-edge limits are enforced when the manifest +is validated, and a pattern past one of them is rejected with a +`PresetValidationError` that names the limit it broke. The resource-name limit applies +to the names a selector is matched against, not to the manifest: an oversized concrete +name is a warned non-match (`selector_matches` returns `False`) rather than an invalid +preset: | Limit | Value | | --- | --- | diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 31c7f8a515..d57d73a2e9 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -647,6 +647,13 @@ def _preset_declarations_for_resource( matches.append( (declaration, path if path is not None and path.is_file() else None) ) + else: + # The selector matched this name but no lower-layer resource makes + # it eligible. Keep the declaration with no candidate so callers + # suppress their convention fallback: collapsing it to nothing + # would read as "undeclared" and let a selector resolve (and so + # create) a conventional file it was only meant to augment. + matches.append((declaration, None)) return matches def _has_concrete_resource( @@ -670,8 +677,42 @@ def _has_concrete_resource( return True if entry is not None: return False + if template_type == "command" and self._declares_command_name(base_dir, name): + # The manifest declares this command - possibly through a selector + # that matches it - so the declaration is authoritative even when its + # own payload is unusable. Falling back to the convention lookup here + # would treat a selector's payload as a concrete lower resource and + # let regex declarations chain, which the contract forbids. + return False return self._conventional_resource(base_dir, name, template_type) is not None + def _declares_command_name(self, pack_dir: Path, name: str) -> bool: + """Whether this preset's manifest declares ``name`` as a command. + + True for an exact ``command`` declaration and for a selector declaration + whose pattern matches ``name``; used only to decide whether the + convention lookup may be consulted. + """ + manifest = self._get_manifest(pack_dir) + if manifest is None: + return False + for declaration in manifest.templates: + if declaration.get("type") != "command": + continue + declared_name = declaration.get("name") + if not isinstance(declared_name, str): + continue + if not is_regex_selector(declared_name): + if declared_name == name: + return True + continue + try: + if selector_matches(declared_name, name): + return True + except Exception: # noqa: BLE001 - a malformed selector declares nothing + continue + return False + def _preset_has_concrete_resource( self, base_dir: Path, name: str, template_type: str ) -> bool: diff --git a/src/specify_cli/presets/command_resolve.py b/src/specify_cli/presets/command_resolve.py index 43d571fdaf..994745f241 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -39,7 +39,14 @@ def preset_resolve( project_root = _require_specify_project() resolver = PresetResolver(project_root) + # A dotless name is a valid command name too (command manifests allow names + # such as ``new``), so the presence of a dot cannot by itself decide the + # type. Prefer whichever surface actually resolves the name, so selector + # attribution is reachable for accepted dotless commands. template_type = "command" if is_command else "template" + if not is_command: + if resolver.collect_all_layers(template_name, "command"): + template_type = "command" layers = resolver.collect_all_layers(template_name, template_type) safe_template_name = _escape_markup(str(template_name)) diff --git a/tests/specify_cli/presets/test_command_resolve.py b/tests/specify_cli/presets/test_command_resolve.py index a7f5ba39d6..909995f7b7 100644 --- a/tests/specify_cli/presets/test_command_resolve.py +++ b/tests/specify_cli/presets/test_command_resolve.py @@ -238,3 +238,41 @@ def test_resolve_reports_command_selector_declaration(project_dir): result = CliRunner().invoke(app, ["preset", "resolve", "speckit.plan"]) assert result.exit_code == 0, result.output assert r"regex:^speckit\.plan$" in strip_ansi(result.output) + + +def test_resolve_reports_selector_attribution_for_dotless_command( + project_dir, temp_dir +): + """A dotless command name must resolve as a command, not as a template. + + Command manifests accept names such as ``new``, so the presence of a dot cannot + decide the resource type. Before the fix ``preset resolve new`` classified the + name as a template, leaving the selector-attribution branch unreachable and + reporting the command as missing even though a selector declared it. + """ + from tests.specify_cli.presets._helpers import ( + install_selector_fixture, + write_command_declarations, + ) + + lower = write_command_declarations( + temp_dir / "lower-exact", + "lower-exact", + [{"type": "command", "name": "new", "body": "LOWER NEW\n"}], + ) + install_selector_fixture(project_dir, "lower-exact", lower, 20) + higher = write_command_declarations( + temp_dir / "higher-selector", + "higher-selector", + [{"type": "command", "name": r"regex:^new$", "body": "SELECTOR NEW\n"}], + ) + install_selector_fixture(project_dir, "higher-selector", higher, 10) + + from unittest.mock import patch + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["preset", "resolve", "new"]) + + assert result.exit_code == 0, (result.output, result.exception) + output = strip_ansi(result.output) + assert r"regex:^new$" in output, output diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index 487d395ef1..1c1023cbc7 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -2655,3 +2655,67 @@ def test_three_overlapping_command_regexes_keep_declaration_order_and_compose( 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_ineligible_selector_declaration_does_not_create_its_conventional_payload( + project_dir, +): + """A selector that matches no lower provider must not fabricate the resource. + + Copilot's previously-missed finding: an ineligible matching selector was + collapsed to nothing, which callers read as "undeclared" and then satisfied + through the convention lookup. A declaration whose payload sits at the + conventional location ``commands/foo.md`` therefore resolved that file even + though no lower ``foo`` command exists, so the selector created the very + resource it was only meant to augment. + """ + from specify_cli.presets import PresetResolver + from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset + + write_diagnostic_selector_preset( + project_dir, + "selector-no-lower", + [{"type": "command", "name": r"regex:^foo$", "file": "commands/foo.md"}], + ) + resolver = PresetResolver(project_dir) + pack = project_dir / ".specify" / "presets" / "selector-no-lower" + + declarations = resolver._preset_declarations_for_resource(pack, "foo", "command") + assert declarations, "the matching declaration must be preserved, not dropped" + assert [candidate for _declaration, candidate in declarations] == [None] + assert resolver.resolve("foo", "command") is None + assert resolver.collect_all_layers("foo", "command") == [] + + +def test_unmatched_selector_payload_is_not_evidence_for_a_higher_selector(project_dir): + """Selector declarations must not chain through their own conventional payload. + + A lower preset declaring ``regex:^foo$`` with its payload at ``commands/foo.md`` + was discovered by the convention lookup, so it counted as a concrete lower + resource and made a higher ``regex:^foo$`` eligible - even though the contract + forbids using another selector declaration as evidence of existence. + """ + from specify_cli.presets import PresetRegistry, PresetResolver + from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset + + for preset_id in ("selector-lower", "selector-higher"): + write_diagnostic_selector_preset( + project_dir, + preset_id, + [{"type": "command", "name": r"regex:^foo$", "file": "commands/foo.md"}], + ) + registry = PresetRegistry(project_dir / ".specify" / "presets") + registry.add("selector-lower", {"enabled": True, "priority": 20, "version": "1.0.0"}) + registry.add("selector-higher", {"enabled": True, "priority": 10, "version": "1.0.0"}) + + resolver = PresetResolver(project_dir) + presets_dir = project_dir / ".specify" / "presets" + assert ( + resolver._has_concrete_resource(presets_dir / "selector-lower", "foo", "command") + is False + ) + higher = resolver._preset_declarations_for_resource( + presets_dir / "selector-higher", "foo", "command" + ) + assert [candidate for _declaration, candidate in higher] == [None] + assert resolver.resolve("foo", "command") is None From 52bcc33781d5a6d5cd94403ceba3bb937d7da9e9 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Thu, 15 Oct 2026 03:53:40 +0800 Subject: [PATCH 4/7] fix(presets): address the open finding and the four previously-missed ones - Selector reconciliation filters unusable declarations before computing the current and tracked match sets. A declaration whose payload has disappeared materializes nothing, so treating it as current left a previously materialized selector command (or skill) on disk after the selector lost its provider and its exact fallback pointed at a missing file. - The unmatched-selector warning is suppressed only by a usable exact fallback: a declared-but-missing payload registers nothing, so staying silent about the unmatched selector would hide that nothing was written. - Selector eligibility requires an effective `replace` base below. A lower layer that only composes (append/prepend/wrap) cannot produce the command (`resolve_content` returns None), so it no longer makes an upper `replace` selector eligible and cannot let a selector create a name that never existed below. Undeclared conventional files, legacy extension namespace files and core resources still count as bases, matching the existing resolution behaviour. - Documented that normal resolution warns and ignores an unusable selector while installation and `preset info` surface the hard validation error. Regression coverage lives in the existing surface-owned suites (`test_resolver.py`, `test_manager_commands.py`) and covers both reconciliation paths: the shared filter is asserted for command files and for skills in the same test. All three new tests fail before the change and pass after it. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- docs/reference/presets.md | 4 +- src/specify_cli/presets/_manager.py | 21 ++++- src/specify_cli/presets/_manager_commands.py | 24 ++++- src/specify_cli/presets/_resolver.py | 88 +++++++++++++++++++ .../presets/test_manager_commands.py | 69 +++++++++++++++ tests/specify_cli/presets/test_resolver.py | 40 +++++++++ 6 files changed, 239 insertions(+), 7 deletions(-) diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 965d4e6cc0..5e5af005b9 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -361,7 +361,9 @@ while producing millions of edges. A selector found in a manifest that is already on disk — written by an older version, or edited by hand — does not raise during resolution. It is reported with a warning and treated as matching nothing, so an unsafe manifest cannot break unrelated -commands. Only the installation path fails hard. +commands. Installation fails hard, and `specify preset info` revalidates an installed +manifest and exits with status 1, so both surfaces report the validation error rather +than silently using the preset. The supported syntax is a subset of Python's regular expressions: literal characters and escapes, `.`, character classes (`[a-z]`, `[^x]`), the predefined classes (`\d`, diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 8ef41bd84b..b1341a83db 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -440,7 +440,7 @@ def install_from_directory( PresetResolver(self.project_root), dest_dir, [t for t in manifest.templates if t.get("type") == "command"], ) - self._warn_unmatched_command_selectors(manifest, command_templates) + self._warn_unmatched_command_selectors(manifest, command_templates, dest_dir) # Register command overrides with AI agents and persist the result # immediately so cleanup can recover even if installation stops # before later phases complete. @@ -513,16 +513,29 @@ def install_from_directory( @staticmethod def _warn_unmatched_command_selectors( - manifest: PresetManifest, command_templates: List[Dict[str, Any]] + manifest: PresetManifest, + command_templates: List[Dict[str, Any]], + preset_dir: Path, ) -> None: - """Warn for valid command selectors with no concrete lower-layer match.""" + """Warn for valid command selectors with no concrete lower-layer match. + + An exact declaration only suppresses the warning when its payload is still + usable: a declared-but-missing fallback registers nothing, so staying + silent about the unmatched selector would hide that nothing was written. + """ import warnings + usable = [ + template + for template in command_templates + if isinstance(template.get("file"), str) + and (preset_dir / template["file"]).is_file() + ] for declaration in manifest.templates: selector = declaration["name"] if declaration["type"] != "command" or not is_regex_selector(selector): continue - if not any(selector_matches(selector, t["name"]) for t in command_templates): + if not any(selector_matches(selector, t["name"]) for t in usable): warnings.warn( f"Preset '{manifest.id}' selector '{selector}' currently matches no command resources. " "Regex selectors only match concrete commands from lower layers.", diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index 1eff380784..a202f639ac 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -374,9 +374,10 @@ def _refresh_selector_matches( metadata = self.registry.get(pack_id) or {} previous_by_agent = metadata.get("selector_commands", {}) previous = set(previous_by_agent.get(agent_name, [])) + usable = self._usable_command_declarations(pack_id, declarations) current = { name - for declaration in declarations + for declaration in usable for name in [declaration["name"], *declaration.get("aliases", [])] } stale = previous - current @@ -404,7 +405,7 @@ def _refresh_selector_matches( self.registry.update(pack_id, {key: remaining}) matches = sorted({ name - for declaration in declarations if declaration.get("_selector") + for declaration in usable if declaration.get("_selector") for name in [declaration["name"], *declaration.get("aliases", [])] }) self.registry.update(pack_id, { @@ -412,6 +413,25 @@ def _refresh_selector_matches( }) return previous + def _usable_command_declarations( + self, pack_id: str, declarations: List[Dict[str, Any]] + ) -> List[Dict[str, Any]]: + """Keep only declarations whose payload still exists in the preset. + + A declaration whose ``file`` has since disappeared materializes nothing, so + it must not count as a current match: treating it as current left a + previously materialized selector command (or skill) on disk after the + selector lost its provider and its exact fallback pointed at a missing + file, because nothing looked stale. + """ + pack_dir = self.presets_dir / pack_id + usable = [] + for declaration in declarations: + file_value = declaration.get("file") + if isinstance(file_value, str) and (pack_dir / file_value).is_file(): + usable.append(declaration) + return usable + def register_enabled_presets_for_agent(self, agent_name: str) -> None: """Re-register enabled presets' command overrides and skills for ``agent_name``. diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index d57d73a2e9..c9f41b40dd 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -639,6 +639,14 @@ def _preset_declarations_for_resource( has_lower_resource = self._core_has_concrete_resource( template_name, template_type ) + if has_lower_resource and not self._has_effective_replace_base( + template_name, template_type, below_pack_id=pack_dir.name + ): + # A lower layer that only composes (append/prepend/wrap) cannot + # produce the command on its own: resolve_content() returns None + # for it. Such a layer must not make a selector eligible, or the + # selector would create a name that never existed below. + has_lower_resource = False matches = [] for declaration, regex in candidates: if not regex or has_lower_resource: @@ -713,6 +721,86 @@ def _declares_command_name(self, pack_dir: Path, name: str) -> bool: continue return False + def _declared_replace_resources( + self, base_dir: Path, name: str, template_type: str + ) -> list[dict]: + """Manifest declarations providing a usable ``replace`` payload for ``name``. + + Pure manifest scan (no eligibility recursion): an entry counts only when it + matches the name exactly or through a selector and its ``file`` exists. + """ + manifest = self._get_manifest(base_dir) + if manifest is None: + return [] + found = [] + for declaration in manifest.templates: + if declaration.get("type") != template_type: + continue + declared_name = declaration.get("name") + if not isinstance(declared_name, str): + continue + if is_regex_selector(declared_name): + try: + matched = selector_matches(declared_name, name) + except Exception: # noqa: BLE001 - a malformed selector declares nothing + matched = False + else: + matched = declared_name == name + if not matched or str(declaration.get("strategy", "replace")) != "replace": + continue + file_value = declaration.get("file") + if isinstance(file_value, str) and (base_dir / file_value).is_file(): + found.append(declaration) + return found + + def _has_effective_replace_base( + self, name: str, template_type: str, *, below_pack_id: str | None = None + ) -> bool: + """Whether a layer below can produce ``name`` with an effective replace base.""" + if self._core_has_concrete_resource(name, template_type): + return True + presets = self._get_all_presets_by_priority() + pack_ids = [pack_id for pack_id, _metadata in presets] + start = 0 + if below_pack_id is not None and below_pack_id in pack_ids: + start = pack_ids.index(below_pack_id) + 1 + for pack_id in pack_ids[start:]: + base_dir = self.presets_dir / pack_id + if self._declared_replace_resources(base_dir, name, template_type): + return True + # An undeclared conventional file is that layer's own content and + # defaults to ``replace``, so it is a valid base as well. Only a + # layer that declares the name purely for composition is not. + entry, _candidate = self._manifest_declared_template( + base_dir, name, template_type + ) + if entry is None and self._conventional_resource( + base_dir, name, template_type + ) is not None: + return True + for _priority, ext_id, _metadata in self._get_all_extensions_by_priority(): + base_dir = self.extensions_dir / ext_id + # Extensions carry ``extension.yml``, so their declarations come from + # their own manifest lookup rather than the preset one. + entry, candidate = self._extension_manifest_declared_template( + base_dir, name, template_type + ) + if ( + candidate is not None + and str((entry or {}).get("strategy", "replace")) == "replace" + ): + return True + if entry is None: + if self._conventional_resource(base_dir, name, template_type) is not None: + return True + if ( + template_type == "command" + and self._find_unregistered_extension_command(name, base_dir) + is not None + ): + return True + return False + def _preset_has_concrete_resource( self, base_dir: Path, name: str, template_type: str ) -> bool: diff --git a/tests/specify_cli/presets/test_manager_commands.py b/tests/specify_cli/presets/test_manager_commands.py index 284a9d29fd..96519d6af1 100644 --- a/tests/specify_cli/presets/test_manager_commands.py +++ b/tests/specify_cli/presets/test_manager_commands.py @@ -3596,3 +3596,72 @@ def test_selector_refresh_keeps_available_matches_and_own_exact_command(project_ assert "SELECTOR BODY" not in output.read_text() assert "SELECTOR BODY" in surviving.read_text() assert manager.registry.get("selector")["selector_commands"][agent] == ["speckit.plan"] + + +def test_refresh_retires_names_whose_payload_disappeared(project_dir, temp_dir): + """A declaration with a missing payload is not a current match. + + Copilot's open finding: after a selector lost its provider and a same-name exact + fallback pointed at a missing file, the unusable declaration still counted as + current, so ``stale`` stayed empty and the previously materialized command (or + skill) was left on disk. + """ + from tests.specify_cli.presets._helpers import install_selector_fixture, write_command_declarations + + pack = write_command_declarations( + temp_dir / "refresh-pack", + "refresh-pack", + [{"type": "command", "name": "speckit.foo", "body": "PAYLOAD\n"}], + ) + install_selector_fixture(project_dir, "refresh-pack", pack, 10) + manager = PresetManager(project_dir) + modern_skill, legacy_skill = manager._skill_names_for_command("speckit.foo") + manager.registry.update( + "refresh-pack", + { + "registered_commands": {"copilot": ["speckit.foo"]}, + "registered_skills": {"copilot": [modern_skill, legacy_skill]}, + "selector_commands": {"copilot": ["speckit.foo"]}, + }, + ) + + # The payload is gone, so the declaration is unusable and must be retired. + (manager.presets_dir / "refresh-pack" / "command-0.md").unlink() + stale = manager._refresh_selector_matches( + "refresh-pack", + "copilot", + [{"type": "command", "name": "speckit.foo", "file": "command-0.md"}], + ) + + assert stale == {"speckit.foo"} + metadata = manager.registry.get("refresh-pack") + # Both reconciliation paths share this filter: the retired command and its + # skills must be dropped from tracking, not left because a same-name + # declaration exists without a payload. + assert metadata["registered_commands"].get("copilot", []) == [] + assert metadata["registered_skills"].get("copilot", []) == [] + assert metadata["selector_commands"]["copilot"] == [] + + +def test_unusable_exact_fallback_does_not_suppress_selector_warning(project_dir, tmp_path): + """The unmatched-selector warning survives a declared-but-missing fallback. + + Suppressing it is only honest when the exact fallback actually materializes: + a missing payload registers nothing, so staying silent would hide that. + """ + from specify_cli.presets._manifest import PresetManifest + from tests.specify_cli.presets._helpers import write_command_declarations + + pack = write_command_declarations( + tmp_path / "warn-pack", + "warn-pack", + [ + {"type": "command", "name": r"regex:^new$", "body": "SEL\n"}, + {"type": "command", "name": "new", "body": "EXACT\n"}, + ], + ) + (pack / "command-1.md").unlink() + manifest = PresetManifest(pack / "preset.yml") + templates = [declaration for declaration in manifest.templates] + with pytest.warns(UserWarning, match="currently matches no command resources"): + PresetManager._warn_unmatched_command_selectors(manifest, templates, pack) diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index 1c1023cbc7..a65084d45a 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -2719,3 +2719,43 @@ def test_unmatched_selector_payload_is_not_evidence_for_a_higher_selector(projec ) assert [candidate for _declaration, candidate in higher] == [None] assert resolver.resolve("foo", "command") is None + + +def test_composition_only_lower_declaration_is_not_a_selector_base(project_dir): + """A lower layer that only composes cannot make a selector eligible. + + A lower preset declaring a name with ``append`` and nothing below it to replace + produces no content (``resolve_content`` returns ``None``), so the name does not + exist as a command. Treating it as a concrete base let an upper ``replace`` + selector create a name that never existed below. + """ + from specify_cli.presets import PresetRegistry, PresetResolver + from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset + + write_diagnostic_selector_preset( + project_dir, + "append-only-lower", + [{"type": "command", "name": "foo", "file": "commands/foo.md", "strategy": "append"}], + ) + write_diagnostic_selector_preset( + project_dir, + "upper-selector", + [ + {"type": "command", "name": "foo", "file": "commands/foo.md"}, + {"type": "command", "name": r"regex:^foo$", "file": "commands/foo.md"}, + ], + ) + registry = PresetRegistry(project_dir / ".specify" / "presets") + registry.add("append-only-lower", {"enabled": True, "priority": 20, "version": "1.0.0"}) + registry.add("upper-selector", {"enabled": True, "priority": 10, "version": "1.0.0"}) + + resolver = PresetResolver(project_dir) + assert ( + resolver._has_effective_replace_base("foo", "command", below_pack_id="upper-selector") + is False + ) + declarations = resolver._preset_declarations_for_resource( + project_dir / ".specify" / "presets" / "upper-selector", "foo", "command" + ) + # The exact declaration still resolves; the selector contributes no layer. + assert [candidate is not None for _declaration, candidate in declarations] == [True, False] From b906e1c1ddc01cb870125562346c7e548e9160b9 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Thu, 15 Oct 2026 05:04:35 +0800 Subject: [PATCH 5/7] fix(presets): align selector expansion and replace-base eligibility with resolution - Expansion now applies the same eligibility rule as resolution: a candidate name counts only when the lower stack has an effective `replace` base. A lower command declared only with append/prepend/wrap no longer appears in `preset info`, suppresses the unmatched warning, or is tracked as a selector match. - Replace-base detection requires a *usable* declaration: it goes through the same declaration lookup as eligibility, so an ineligible middle selector is no longer reported as a base for an upper selector, and the conventional fallback is suppressed when the manifest declares the command through a selector. The lookups stay bounded to layers strictly below the one being evaluated, so the recursive check terminates. - Pre-composition and reconciliation only write `.composed` content when the top layer is the declaration's own payload, so an unusable declaration can no longer be selected by first-wins deduplication and leave selector-only alias files behind. - `preset resolve` keeps exact-template precedence for dotless names and only falls back to the command surface when no template layer exists, restoring the previous exact-template diagnostics while keeping selector attribution for command-only names. Regression coverage in the existing surface-owned suites; every new case fails before the change and passes after it. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- src/specify_cli/presets/_manager_commands.py | 14 ++++- src/specify_cli/presets/_resolver.py | 51 +++++++---------- src/specify_cli/presets/command_resolve.py | 13 ++--- .../presets/test_command_resolve.py | 22 ++++--- .../presets/test_manager_commands.py | 57 +++++++++++++++++++ tests/specify_cli/presets/test_resolver.py | 25 +++++++- 6 files changed, 130 insertions(+), 52 deletions(-) diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index a202f639ac..e131ca0316 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -165,6 +165,9 @@ def _expand_command_selectors( if _PresetCommandMethods._command_name_has_lower_layer( resolver, name, lower_preset_ids, lower_extension_ids ) + and resolver._has_effective_replace_base( + name, "command", below_pack_id=current_id + ) } ) @@ -253,12 +256,14 @@ def _register_commands( for cmd in command_templates: strategy = cmd.get("strategy", "replace") if strategy != "replace": - # Only pre-compose if this preset is the top composing layer. + # Only pre-compose this declaration if its payload is the top layer. + # Another declaration in this preset cannot make a missing payload + # usable or confer its aliases on the concrete command. # 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) + layers and layers[0]["path"] == preset_dir / cmd["file"] ) if top_layer_is_ours: composed = resolver.resolve_content(cmd["name"], "command") @@ -1371,7 +1376,10 @@ def record_written(written: Dict[str, List[str]]) -> None: resolver, pack_dir, [t for t in manifest.templates if t.get("type") == "command"], ): - if tmpl.get("name") == cmd_name: + if ( + tmpl.get("name") == cmd_name + and pack_dir / tmpl["file"] == layers[0]["path"] + ): composed_dir = pack_dir / ".composed" composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index c9f41b40dd..9e6edfe810 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -724,34 +724,21 @@ def _declares_command_name(self, pack_dir: Path, name: str) -> bool: def _declared_replace_resources( self, base_dir: Path, name: str, template_type: str ) -> list[dict]: - """Manifest declarations providing a usable ``replace`` payload for ``name``. - - Pure manifest scan (no eligibility recursion): an entry counts only when it - matches the name exactly or through a selector and its ``file`` exists. - """ - manifest = self._get_manifest(base_dir) - if manifest is None: - return [] - found = [] - for declaration in manifest.templates: - if declaration.get("type") != template_type: - continue - declared_name = declaration.get("name") - if not isinstance(declared_name, str): - continue - if is_regex_selector(declared_name): - try: - matched = selector_matches(declared_name, name) - except Exception: # noqa: BLE001 - a malformed selector declares nothing - matched = False - else: - matched = declared_name == name - if not matched or str(declaration.get("strategy", "replace")) != "replace": - continue - file_value = declaration.get("file") - if isinstance(file_value, str) and (base_dir / file_value).is_file(): - found.append(declaration) - return found + """Return usable replace declarations, including eligible selectors only.""" + if template_type == "command": + # Selector eligibility checks strictly below base_dir's position in + # the priority stack, so recursive replace-base checks terminate. + declarations = self._preset_declarations_for_resource( + base_dir, name, template_type + ) + else: + declarations = [self._manifest_declared_template(base_dir, name, template_type)] + return [ + declaration + for declaration, candidate in declarations + if declaration is not None and candidate is not None + and declaration.get("strategy", "replace") == "replace" + ] def _has_effective_replace_base( self, name: str, template_type: str, *, below_pack_id: str | None = None @@ -774,9 +761,11 @@ def _has_effective_replace_base( entry, _candidate = self._manifest_declared_template( base_dir, name, template_type ) - if entry is None and self._conventional_resource( - base_dir, name, template_type - ) is not None: + if ( + entry is None + and not (template_type == "command" and self._declares_command_name(base_dir, name)) + and self._conventional_resource(base_dir, name, template_type) is not None + ): return True for _priority, ext_id, _metadata in self._get_all_extensions_by_priority(): base_dir = self.extensions_dir / ext_id diff --git a/src/specify_cli/presets/command_resolve.py b/src/specify_cli/presets/command_resolve.py index 994745f241..6dd0b0e6c3 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -39,16 +39,13 @@ def preset_resolve( project_root = _require_specify_project() resolver = PresetResolver(project_root) - # A dotless name is a valid command name too (command manifests allow names - # such as ``new``), so the presence of a dot cannot by itself decide the - # type. Prefer whichever surface actually resolves the name, so selector - # attribution is reachable for accepted dotless commands. + # Preserve exact-template precedence for dotless names, falling back to the + # command surface so command-only names still expose selector attribution. template_type = "command" if is_command else "template" - if not is_command: - if resolver.collect_all_layers(template_name, "command"): - template_type = "command" - layers = resolver.collect_all_layers(template_name, template_type) + if not is_command and not layers: + template_type = "command" + layers = resolver.collect_all_layers(template_name, template_type) safe_template_name = _escape_markup(str(template_name)) if layers: diff --git a/tests/specify_cli/presets/test_command_resolve.py b/tests/specify_cli/presets/test_command_resolve.py index 909995f7b7..d275f0b27d 100644 --- a/tests/specify_cli/presets/test_command_resolve.py +++ b/tests/specify_cli/presets/test_command_resolve.py @@ -2,6 +2,7 @@ from pathlib import Path +import pytest import yaml from typer.testing import CliRunner @@ -240,16 +241,11 @@ def test_resolve_reports_command_selector_declaration(project_dir): assert r"regex:^speckit\.plan$" in strip_ansi(result.output) +@pytest.mark.parametrize("same_name_template", [False, True]) def test_resolve_reports_selector_attribution_for_dotless_command( - project_dir, temp_dir + project_dir, temp_dir, same_name_template ): - """A dotless command name must resolve as a command, not as a template. - - Command manifests accept names such as ``new``, so the presence of a dot cannot - decide the resource type. Before the fix ``preset resolve new`` classified the - name as a template, leaving the selector-attribution branch unreachable and - reporting the command as missing even though a selector declared it. - """ + """Dotless commands expose selectors while same-name templates keep precedence.""" from tests.specify_cli.presets._helpers import ( install_selector_fixture, write_command_declarations, @@ -267,6 +263,9 @@ def test_resolve_reports_selector_attribution_for_dotless_command( [{"type": "command", "name": r"regex:^new$", "body": "SELECTOR NEW\n"}], ) install_selector_fixture(project_dir, "higher-selector", higher, 10) + if same_name_template: + template = project_dir / ".specify" / "templates" / "new.md" + template.write_text("EXACT TEMPLATE\n") from unittest.mock import patch @@ -275,4 +274,9 @@ def test_resolve_reports_selector_attribution_for_dotless_command( assert result.exit_code == 0, (result.output, result.exception) output = strip_ansi(result.output) - assert r"regex:^new$" in output, output + if same_name_template: + assert str(template) in output.replace("\n", ""), output + assert "regex:^new$" not in output, output + assert "higher-selector" not in output, output + else: + assert r"regex:^new$" in output, output diff --git a/tests/specify_cli/presets/test_manager_commands.py b/tests/specify_cli/presets/test_manager_commands.py index 96519d6af1..16eeb98aa4 100644 --- a/tests/specify_cli/presets/test_manager_commands.py +++ b/tests/specify_cli/presets/test_manager_commands.py @@ -3665,3 +3665,60 @@ def test_unusable_exact_fallback_does_not_suppress_selector_warning(project_dir, templates = [declaration for declaration in manifest.templates] with pytest.warns(UserWarning, match="currently matches no command resources"): PresetManager._warn_unmatched_command_selectors(manifest, templates, pack) + + +@pytest.mark.parametrize("strategy", ["append", "prepend", "wrap"]) +def test_command_regex_expansion_requires_effective_replace_base(project_dir, strategy): + from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset + + lower = write_diagnostic_selector_preset( + project_dir, "composition-only", + [{"type": "command", "name": "foo", "file": "commands/foo.md", "strategy": strategy}], + ) + pack = write_diagnostic_selector_preset( + project_dir, "upper-selector", + [{"type": "command", "name": r"regex:^foo$", "file": "payload.md"}], + ) + manager = PresetManager(project_dir) + manager.registry.update(lower.name, {"priority": 20}) + manifest = PresetManifest(pack / "preset.yml") + resolver = PresetResolver(project_dir) + assert resolver.resolve_content("foo", "command") is None + expanded = manager._expand_command_selectors(resolver, pack, manifest.templates) + assert expanded == [] + with pytest.warns(UserWarning, match="currently matches no command resources"): + manager._warn_unmatched_command_selectors(manifest, expanded, pack) + manager._refresh_selector_matches(pack.name, "copilot", expanded) + assert manager.registry.get(pack.name)["selector_commands"]["copilot"] == [] + + +@pytest.mark.parametrize("missing_selector", [True, False]) +@pytest.mark.parametrize("usable_strategy", ["replace", "append"]) +def test_overlapping_composition_does_not_register_unusable_aliases( + project_dir, tmp_path, missing_selector, usable_strategy +): + from specify_cli import save_init_options + + output = project_dir / ".github" / "agents" + output.mkdir(parents=True) + save_init_options(project_dir, {"ai": "copilot", "ai_skills": False, "script": "sh"}) + names = [r"regex:^speckit\.plan$", "speckit.plan"] + if not missing_selector: + names.reverse() + source = write_command_declarations(tmp_path / "overlap", "overlap", [ + {"type": "command", "name": names[0], "strategy": "append", + "aliases": ["speckit.unusable"], "body": "MISSING BODY\n"}, + {"type": "command", "name": names[1], "strategy": usable_strategy, + "aliases": ["speckit.usable"], "body": "USABLE BODY\n"}, + ]) + installed = install_selector_fixture(project_dir, "overlap", source, 1) + (installed / "command-0.md").unlink() + manager = PresetManager(project_dir) + manager.register_enabled_presets_for_agent("copilot") + + assert not (output / "speckit.unusable.agent.md").exists() + assert "USABLE BODY" in (output / "speckit.plan.agent.md").read_text() + assert "USABLE BODY" in (output / "speckit.usable.agent.md").read_text() + tracked = manager.registry.get("overlap")["registered_commands"]["copilot"] + assert "speckit.unusable" not in tracked + assert "speckit.usable" in tracked diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index a65084d45a..8c550fb125 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -2721,7 +2721,14 @@ def test_unmatched_selector_payload_is_not_evidence_for_a_higher_selector(projec assert resolver.resolve("foo", "command") is None -def test_composition_only_lower_declaration_is_not_a_selector_base(project_dir): +@pytest.mark.parametrize( + "middle_strategy,middle_file", + [(None, None), ("replace", "payload.md"), ("replace", "commands/foo.md"), + ("append", "commands/foo.md")], +) +def test_composition_only_lower_declaration_is_not_a_selector_base( + project_dir, middle_strategy, middle_file +): """A lower layer that only composes cannot make a selector eligible. A lower preset declaring a name with ``append`` and nothing below it to replace @@ -2748,6 +2755,14 @@ def test_composition_only_lower_declaration_is_not_a_selector_base(project_dir): registry = PresetRegistry(project_dir / ".specify" / "presets") registry.add("append-only-lower", {"enabled": True, "priority": 20, "version": "1.0.0"}) registry.add("upper-selector", {"enabled": True, "priority": 10, "version": "1.0.0"}) + if middle_strategy is not None: + write_diagnostic_selector_preset( + project_dir, + "middle-selector", + [{"type": "command", "name": r"regex:^foo$", + "file": middle_file, "strategy": middle_strategy}], + ) + registry.add("middle-selector", {"enabled": True, "priority": 15, "version": "1.0.0"}) resolver = PresetResolver(project_dir) assert ( @@ -2759,3 +2774,11 @@ def test_composition_only_lower_declaration_is_not_a_selector_base(project_dir): ) # The exact declaration still resolves; the selector contributes no layer. assert [candidate is not None for _declaration, candidate in declarations] == [True, False] + + if middle_strategy is not None: + middle = resolver._preset_declarations_for_resource( + project_dir / ".specify" / "presets" / "middle-selector", "foo", "command" + ) + assert [candidate for _declaration, candidate in middle] == [None] + assert all("middle-selector" not in layer["source"] + for layer in resolver.collect_all_layers("foo", "command")) From 87a5907e01e44d1961d6b56629a359571e9cf907 Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Fri, 16 Oct 2026 01:49:30 +0800 Subject: [PATCH 6/7] fix(presets): make selector base detection linear and frontmatter-aware - The replace-base lookup inspects usable exact declarations directly instead of recursively evaluating selectors, which rescan the same suffixes. Twelve selector presets over one exact base drop from 20,452 declaration evaluations (545 ms) to 157 (6 ms), and every exact declaration is considered, not only the first; a multi-preset complexity regression pins that. - Base strategies are now calculated by one shared, frontmatter-aware helper that layer collection also uses: an explicit manifest strategy still wins, but a payload declaring `strategy: append` (declared or conventional) no longer counts as replace content, so it cannot back an upper selector's eligibility. - Removal unions the concrete selector names persisted in the registry with the current expansion, so a disabled selector-only preset is still reconciled against a surviving lower preset when removed in AI-skills mode. Regression coverage in the existing surface-owned suites; every new case fails before the change and passes after it. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- src/specify_cli/presets/_manager.py | 9 ++ src/specify_cli/presets/_resolver.py | 122 ++++++++++-------- .../presets/test_manager_skills.py | 36 ++++++ tests/specify_cli/presets/test_resolver.py | 74 +++++++++++ 4 files changed, 190 insertions(+), 51 deletions(-) diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index b1341a83db..82e4d6637b 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -803,6 +803,15 @@ def remove(self, pack_id: str) -> bool: name for name in cmd_names if isinstance(name, str) and not is_regex_selector(name) ) + # Disabled selectors no longer expand, and command-backed integrations + # in AI-skills mode have no registered_commands. Retain their recorded + # concrete names so removal still reconciles surviving preset layers. + selector_commands = (metadata or {}).get("selector_commands", {}) + for cmd_names in selector_commands.values(): + removed_cmd_names.update( + name for name in cmd_names + if isinstance(name, str) and not is_regex_selector(name) + ) manifest_path = pack_dir / "preset.yml" if manifest_path.exists(): try: diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 9e6edfe810..6faec47517 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -721,23 +721,76 @@ def _declares_command_name(self, pack_dir: Path, name: str) -> bool: continue return False + @staticmethod + def _effective_preset_strategy( + declaration: dict | None, candidate: Path, template_type: str + ) -> str: + """Honor explicit manifest strategy before legacy command frontmatter.""" + strategy = "replace" + manifest_has_strategy = False + if declaration is not None: + strategy = declaration.get("strategy", "replace") + manifest_has_strategy = "strategy" in declaration + 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) + if lines and lines[0].rstrip("\r\n") == "---": + fence_end = next( + ( + i + for i, line in enumerate(lines[1:], 1) + if line.rstrip("\r\n") == "---" + ), + -1, + ) + if fence_end > 0: + 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): + pass + return strategy + def _declared_replace_resources( self, base_dir: Path, name: str, template_type: str ) -> list[dict]: - """Return usable replace declarations, including eligible selectors only.""" + """Return all usable exact replace declarations without evaluating selectors.""" if template_type == "command": - # Selector eligibility checks strictly below base_dir's position in - # the priority stack, so recursive replace-base checks terminate. - declarations = self._preset_declarations_for_resource( - base_dir, name, template_type - ) + # A selector needs an independent base below it, so traversing + # selectors cannot reveal another base and would rescan suffixes + # exponentially. Keep every exact declaration, not only the first. + manifest = self._get_manifest(base_dir) + declarations = [] + for declaration in manifest.templates if manifest is not None else []: + if declaration.get("type") != template_type or declaration.get("name") != name: + continue + file_value = declaration.get("file") + candidate = base_dir / file_value if isinstance(file_value, str) else None + if candidate is not None and candidate.is_file(): + declarations.append((declaration, candidate)) else: declarations = [self._manifest_declared_template(base_dir, name, template_type)] return [ declaration for declaration, candidate in declarations if declaration is not None and candidate is not None - and declaration.get("strategy", "replace") == "replace" + and self._effective_preset_strategy(declaration, candidate, template_type) == "replace" ] def _has_effective_replace_base( @@ -755,18 +808,21 @@ def _has_effective_replace_base( base_dir = self.presets_dir / pack_id if self._declared_replace_resources(base_dir, name, template_type): return True - # An undeclared conventional file is that layer's own content and - # defaults to ``replace``, so it is a valid base as well. Only a - # layer that declares the name purely for composition is not. + # Undeclared conventional commands may still opt into composition + # through legacy frontmatter, just as they do during layer collection. entry, _candidate = self._manifest_declared_template( base_dir, name, template_type ) if ( entry is None and not (template_type == "command" and self._declares_command_name(base_dir, name)) - and self._conventional_resource(base_dir, name, template_type) is not None ): - return True + candidate = self._conventional_resource(base_dir, name, template_type) + if ( + candidate is not None + and self._effective_preset_strategy(None, candidate, template_type) == "replace" + ): + return True for _priority, ext_id, _metadata in self._get_all_extensions_by_priority(): base_dir = self.extensions_dir / ext_id # Extensions carry ``extension.yml``, so their declarations come from @@ -937,47 +993,11 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: 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" - and template_type == "command" - ): - try: - cmd_content = candidate.read_text(encoding="utf-8") - lines = cmd_content.splitlines(keepends=True) - if lines and lines[0].rstrip("\r\n") == "---": - fence_end = next( - ( - i - for i, line in enumerate(lines[1:], 1) - if line.rstrip("\r\n") == "---" - ), - -1, - ) - if fence_end > 0: - 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): - pass + strategy = self._effective_preset_strategy( + layer_entry, candidate, template_type + ) version = metadata.get("version", "?") if metadata else "?" layers.append( { diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 41d9bea63b..6a27aa3ae5 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2887,6 +2887,42 @@ def test_install_regex_commands_registers_concrete_ai_skills(project_dir): ) +def test_remove_disabled_selector_restores_lower_preset_ai_skill(project_dir, monkeypatch): + from specify_cli import save_init_options + from specify_cli.presets.command_disable import preset_disable + from tests.specify_cli.presets._helpers import write_command_declarations + + save_init_options(project_dir, {"ai": "copilot", "ai_skills": True, "script": "sh"}) + manager = PresetManager(project_dir) + for preset_id, name, priority, body in ( + ("lower", "speckit.plan", 10, "LOWER PRESET"), + ("upper", r"regex:^speckit\.plan$", 1, "UPPER SELECTOR"), + ): + source = write_command_declarations( + project_dir / "source" / preset_id, preset_id, + [{"name": name, "type": "command", "strategy": "replace", "body": body}], + ) + manager.install_from_directory(source, "0.1.5", priority=priority) + + output = project_dir / ".github/skills/speckit-plan/SKILL.md" + assert "UPPER SELECTOR" in output.read_text() + metadata = manager.registry.get("upper") + assert not metadata.get("registered_commands") + assert metadata["selector_commands"] == {"copilot": ["speckit.plan"]} + + monkeypatch.chdir(project_dir) + preset_disable("upper") + manager = PresetManager(project_dir) + assert manager.registry.get("upper")["enabled"] is False + assert "UPPER SELECTOR" in output.read_text() + assert manager.remove("upper") is True + content = output.read_text() + assert "LOWER PRESET" in content + assert "preset:lower" in content + assert "UPPER SELECTOR" not in content + assert not manager.registry.is_installed("upper") + + def test_skills_ignore_stale_composition_after_selector_falls_back_to_exact(project_dir): from specify_cli import save_init_options from specify_cli.presets import PresetManifest diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index 8c550fb125..7fb99c19eb 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -2782,3 +2782,77 @@ def test_composition_only_lower_declaration_is_not_a_selector_base( assert [candidate for _declaration, candidate in middle] == [None] assert all("middle-selector" not in layer["source"] for layer in resolver.collect_all_layers("foo", "command")) + + +@pytest.mark.parametrize("preset_count", [4, 8, 12]) +def test_selector_base_scan_has_bounded_declaration_evaluations(project_dir, monkeypatch, preset_count): + """A grid of append selectors must not recursively rescan every suffix.""" + name = "complexity-command" + write_selector_contract_preset( + project_dir, "base", [(name, "command", "replace", "BASE\n")], 100 + ) + for index in range(preset_count): + write_selector_contract_preset( + project_dir, f"selector-{index}", + [(f"regex:^{name}$", "command", "append", f"APPEND {index}\n"), + ("regex:^complexity-.*$", "command", "append", f"EXTRA {index}\n")], + index + 1, + ) + resolver = PresetResolver(project_dir) + evaluate = resolver._preset_declarations_for_resource + evaluations = 0 + + def counted(*args, **kwargs): + nonlocal evaluations + evaluations += 1 + return evaluate(*args, **kwargs) + + monkeypatch.setattr(resolver, "_preset_declarations_for_resource", counted) + layers = resolver.collect_all_layers(name, "command") + assert len(layers) == 2 * preset_count + 1 + print(f"selector presets={preset_count}: declaration evaluations={evaluations}") + assert evaluations <= preset_count + 1 + content = resolver.resolve_content(name, "command") + assert "BASE" in content + for index in range(preset_count): + assert f"APPEND {index}\n" in content + assert f"EXTRA {index}\n" in content + + +@pytest.mark.parametrize("declared", [True, False], ids=["declared", "conventional"]) +def test_selector_base_honors_command_frontmatter(project_dir, declared): + """Legacy append payloads cannot establish a command for a selector.""" + from tests.specify_cli.presets._helpers import write_diagnostic_selector_preset + + entry = {"name": "foo", "type": "command", "file": "commands/foo.md"} + lower = write_diagnostic_selector_preset(project_dir, "lower", [entry] if declared else []) + payload = lower / "commands/foo.md" + payload.parent.mkdir(exist_ok=True) + payload.write_text("---\nstrategy: append\n---\nLOWER\n", encoding="utf-8") + registry = PresetRegistry(project_dir / ".specify/presets") + registry.update("lower", {"priority": 10}) + assert PresetResolver(project_dir).resolve_content("foo", "command") is None + write_selector_contract_preset( + project_dir, "upper", [("regex:^foo$", "command", "replace", "UPPER\n")] + ) + + resolver = PresetResolver(project_dir) + assert resolver.resolve_content("foo", "command") is None + assert [layer["source"] for layer in resolver.collect_all_layers("foo", "command")] == ["lower v1.0.0"] + + # Explicit manifest strategy wins over legacy frontmatter. An earlier exact + # append or missing payload in the stack must not hide the usable replace. + write_selector_contract_preset( + project_dir, "middle-append", [("foo", "command", "append", "MIDDLE\n")], 5 + ) + write_selector_contract_preset( + project_dir, "middle-missing", [("foo", "command", "replace", None)], 6 + ) + data = yaml.safe_load((lower / "preset.yml").read_text()) + data["provides"]["templates"] = [{**entry, "strategy": "replace"}] + (lower / "preset.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + resolver = PresetResolver(project_dir) + assert resolver.resolve_content("foo", "command") == "UPPER\n" + assert [layer["strategy"] for layer in resolver.collect_all_layers("foo", "command")] == [ + "replace", "append", "replace", + ] From fc1260c01a9cb059406f0b9f4eb24a71300c885d Mon Sep 17 00:00:00 2001 From: lmtyy <3512819960@qq.com> Date: Fri, 16 Oct 2026 08:40:05 +0800 Subject: [PATCH 7/7] perf(presets): expand a preset's selector declarations once per reconciliation `_reconcile_composed_commands()` called `_expand_command_selectors()` inside its per-command loop, rebuilding the full artifact inventory once per matched command. For a selector matching N commands that is N full-project scans (quadratic when the matches dominate the inventory). The expansion is now cached per preset for the duration of one reconciliation operation and shared by both branches. Measured on a selector matching 2 and 4 lower commands (replace, append and mixed strategies): inventory scans per reconciliation drop from 2/4 to 1/1, while the reconciled result and the written content are unchanged. A regression test counts `ArtifactCatalog.list_artifacts` calls and asserts exactly one scan per reconciliation regardless of the number of matches. AI disclosure: authored with AI assistance (Hermes Agent); reviewed and verified by a human against the test suite. --- src/specify_cli/presets/_manager_commands.py | 28 +++++---- .../presets/test_manager_commands.py | 59 +++++++++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/presets/_manager_commands.py b/src/specify_cli/presets/_manager_commands.py index e131ca0316..dfb9fcd399 100644 --- a/src/specify_cli/presets/_manager_commands.py +++ b/src/specify_cli/presets/_manager_commands.py @@ -1213,6 +1213,19 @@ def record_written(written: Dict[str, List[str]]) -> None: # Cache registry and manifests outside the loop to avoid # repeated filesystem reads for each command name. presets_by_priority = list(self.registry.list_by_priority()) + expanded_declarations_by_pack: Dict[str, List[Dict[str, Any]]] = {} + + def command_declarations( + pack_id: str, pack_dir: Path, manifest: PresetManifest + ) -> List[Dict[str, Any]]: + # Share one inventory expansion across replace and composed commands + # for this preset, keeping the cache local to this reconciliation. + if pack_id not in expanded_declarations_by_pack: + expanded_declarations_by_pack[pack_id] = self._expand_command_selectors( + resolver, pack_dir, + [item for item in manifest.templates if item.get("type") == "command"], + ) + return expanded_declarations_by_pack[pack_id] for cmd_name in command_names: layers = resolver.collect_all_layers(cmd_name, "command") @@ -1239,14 +1252,8 @@ def record_written(written: Dict[str, List[str]]) -> None: concrete = next( ( declaration - for declaration in self._expand_command_selectors( - resolver, - pack_dir, - [ - item - for item in manifest.templates - if item.get("type") == "command" - ], + for declaration in command_declarations( + pack_id, pack_dir, manifest, ) if declaration.get("name") == cmd_name and (pack_dir / str(declaration.get("file", ""))) @@ -1372,10 +1379,7 @@ def record_written(written: Dict[str, List[str]]) -> None: manifest = resolver._get_manifest(pack_dir) if not manifest: continue - for tmpl in self._expand_command_selectors( - resolver, pack_dir, - [t for t in manifest.templates if t.get("type") == "command"], - ): + for tmpl in command_declarations(pack_id, pack_dir, manifest): if ( tmpl.get("name") == cmd_name and pack_dir / tmpl["file"] == layers[0]["path"] diff --git a/tests/specify_cli/presets/test_manager_commands.py b/tests/specify_cli/presets/test_manager_commands.py index 16eeb98aa4..63aca2d2fb 100644 --- a/tests/specify_cli/presets/test_manager_commands.py +++ b/tests/specify_cli/presets/test_manager_commands.py @@ -3436,6 +3436,65 @@ def test_command_regex_excludes_higher_preset_and_project_override( ) +@pytest.mark.parametrize("match_count", [2, 4]) +@pytest.mark.parametrize("strategies", [("replace",), ("append",), ("replace", "append")]) +def test_reconcile_caches_selector_inventory_per_preset( + project_dir, tmp_path, monkeypatch, match_count, strategies +): + """Both reconciliation branches share one expansion per preset per operation.""" + from specify_cli import save_init_options + from specify_cli.artifacts.catalog import ArtifactCatalog + + output_root = project_dir / ".agents" / "commands" + output_root.mkdir(parents=True) + save_init_options(project_dir, {"ai": "amp", "ai_skills": False, "script": "sh"}) + names = [f"speckit.cached{index}" for index in range(match_count)] + lower = write_command_declarations( + tmp_path / "lower", + "lower", + [ + {"type": "command", "name": name, "strategy": "replace", "body": f"BASE {name}\n"} + for name in [*names, "speckit.unmatched"] + ], + ) + install_selector_fixture(project_dir, "lower", lower, 20) + upper = write_command_declarations( + tmp_path / "upper", + "upper", + [ + { + "type": "command", + "name": r"regex:^speckit\.cached(" + "|".join( + str(index) for index in range(offset, match_count, len(strategies)) + ) + ")$", + "strategy": strategy, + "body": f"OVERLAY {strategy}\n", + } + for offset, strategy in enumerate(strategies) + ], + ) + install_selector_fixture(project_dir, "upper", upper, 10) + manager = PresetManager(project_dir) + scan_count = 0 + list_artifacts = ArtifactCatalog.list_artifacts + + def counted_inventory(catalog, *args, **kwargs): + nonlocal scan_count + scan_count += 1 + return list_artifacts(catalog, *args, **kwargs) + + monkeypatch.setattr(ArtifactCatalog, "list_artifacts", counted_inventory) + for operation in range(1, 3): + assert manager._reconcile_composed_commands(names) == set(names) + for index, name in enumerate(names): + strategy = strategies[index % len(strategies)] + content = (output_root / f"{name}.md").read_text() + assert f"OVERLAY {strategy}" in content + assert (f"BASE {name}" in content) == (strategy == "append") + assert not (output_root / "speckit.unmatched.md").exists() + assert scan_count == operation + + @pytest.mark.parametrize( "agent,skills", [("amp", False), ("claude", True), ("kimi", False), ("copilot", True)],