diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 8ca29547b7..5e5af005b9 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,106 @@ 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. 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 | +| --- | --- | +| 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. 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`, +`\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..82e4d6637b 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, 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. - 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,37 @@ def install_from_directory( return manifest + @staticmethod + def _warn_unmatched_command_selectors( + manifest: PresetManifest, + command_templates: List[Dict[str, Any]], + preset_dir: Path, + ) -> None: + """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 usable): + 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 +729,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 +799,29 @@ 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) + ) + # 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: 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..dfb9fcd399 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,127 @@ 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 + ) + and resolver._has_effective_replace_base( + name, "command", below_pack_id=current_id + ) + } + ) + + 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 +232,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 {} @@ -136,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") @@ -183,6 +305,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 +363,80 @@ 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, [])) + usable = self._usable_command_declarations(pack_id, declarations) + current = { + name + for declaration in usable + 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 usable 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 _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``. @@ -273,6 +478,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 +488,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 +539,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 +558,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 +612,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 +633,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 +670,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 +729,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 +925,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 @@ -993,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") @@ -1016,16 +1249,34 @@ 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 command_declarations( + pack_id, pack_dir, manifest, ) - 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 +1379,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 command_declarations(pack_id, pack_dir, manifest): + 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/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 5a4a4fb739..07fa827e8e 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,18 +711,28 @@ 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. + # 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 @@ -761,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 @@ -832,7 +850,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 +876,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..6faec47517 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 REGEX_COMPILE_ERRORS + + if isinstance(exc.__cause__, REGEX_COMPILE_ERRORS): + 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,332 @@ 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 [] + candidates = [] + for declaration in manifest.templates: + name = declaration.get("name") + if declaration.get("type") != template_type or not isinstance(name, str): + continue + 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 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 + ) + 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: + 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) + ) + 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( + 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 + 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 + + @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 all usable exact replace declarations without evaluating selectors.""" + if template_type == "command": + # 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 self._effective_preset_strategy(declaration, candidate, template_type) == "replace" + ] + + 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 + # 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)) + ): + 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 + # 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: + 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._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 +928,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 +965,90 @@ 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": - 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 - 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() - except (UnicodeDecodeError, yaml.YAMLError, OSError): - # Best-effort legacy frontmatter parsing: keep default - # strategy ("replace") when content is unreadable/invalid. - pass + 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: + if candidate is None: + continue + strategy = self._effective_preset_strategy( + layer_entry, candidate, template_type + ) 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..6dd0b0e6c3 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 = ( @@ -38,9 +39,13 @@ def preset_resolve( project_root = _require_specify_project() resolver = PresetResolver(project_root) + # 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" - 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: @@ -55,6 +60,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 +112,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..d275f0b27d 100644 --- a/tests/specify_cli/presets/test_command_resolve.py +++ b/tests/specify_cli/presets/test_command_resolve.py @@ -2,12 +2,16 @@ from pathlib import Path +import pytest 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 +218,65 @@ 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) + + +@pytest.mark.parametrize("same_name_template", [False, True]) +def test_resolve_reports_selector_attribution_for_dotless_command( + project_dir, temp_dir, same_name_template +): + """Dotless commands expose selectors while same-name templates keep precedence.""" + 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) + if same_name_template: + template = project_dir / ".specify" / "templates" / "new.md" + template.write_text("EXACT TEMPLATE\n") + + 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) + 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.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..63aca2d2fb 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,615 @@ 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("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)], +) +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"] + + +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) + + +@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_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 7ae833bfb9..6a27aa3ae5 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2805,3 +2805,157 @@ 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 + ) + + +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 + 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_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..7fb99c19eb 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -1,6 +1,8 @@ """Tests for preset layer resolution and composition in specify_cli.presets._resolver.""" import json +import re +import warnings from pathlib import Path import pytest @@ -9,13 +11,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 +1807,1052 @@ 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_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 +): + 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") + + +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 + + +@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 + 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"}) + 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 ( + 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] + + 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")) + + +@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", + ] 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