From 59aad558e309c7754f6038cee3fce046d6b4d15c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 20:55:10 +0000 Subject: [PATCH 01/16] Initial plan From a7f547d641ddd7f224f03a78cc1ca570f99d78b3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 21:05:55 +0000 Subject: [PATCH 02/16] feat(presets): Add composition strategies (prepend, append, wrap) for templates, commands, and scripts - Add strategy field validation in PresetManifest._validate() - Add VALID_PRESET_STRATEGIES and VALID_SCRIPT_STRATEGIES constants - Add PresetResolver.resolve_content() for composed content resolution - Add PresetResolver._collect_all_layers() for full priority stack - Update _register_commands() to compose before writing - Update CLI preset resolve command to show composition chain - Add resolve_template_content() to bash common.sh - Add Resolve-TemplateContent to PowerShell common.ps1 - Update scaffold preset.yml with strategy documentation - Update presets/README.md and ARCHITECTURE.md - Add 26 unit tests covering validation, composition, and chaining Agent-Logs-Url: https://github.com/github/spec-kit/sessions/c285c51b-f00b-480a-9eb2-ae70e3cbcb72 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> --- presets/ARCHITECTURE.md | 18 ++ presets/README.md | 44 ++- presets/scaffold/preset.yml | 24 ++ scripts/bash/common.sh | 143 +++++++++ scripts/powershell/common.ps1 | 147 +++++++++ src/specify_cli/__init__.py | 11 + src/specify_cli/presets.py | 244 ++++++++++++++- tests/test_presets.py | 551 ++++++++++++++++++++++++++++++++++ 8 files changed, 1170 insertions(+), 12 deletions(-) diff --git a/presets/ARCHITECTURE.md b/presets/ARCHITECTURE.md index d0e6547816..d7a45cb9ff 100644 --- a/presets/ARCHITECTURE.md +++ b/presets/ARCHITECTURE.md @@ -41,6 +41,24 @@ The resolution is implemented three times to ensure consistency: - **Bash**: `resolve_template()` in `scripts/bash/common.sh` - **PowerShell**: `Resolve-Template` in `scripts/powershell/common.ps1` +### Composition Strategies + +Templates, commands, and scripts support a `strategy` field that controls how a preset's content is combined with lower-priority content instead of fully replacing it: + +| Strategy | Description | Templates | Commands | Scripts | +|----------|-------------|-----------|----------|---------| +| `replace` (default) | Fully replaces lower-priority content | ✓ | ✓ | ✓ | +| `prepend` | Places content before lower-priority content (separated by a blank line) | ✓ | ✓ | — | +| `append` | Places content after lower-priority content (separated by a blank line) | ✓ | ✓ | — | +| `wrap` | Content contains `{CORE_TEMPLATE}` (templates/commands) or `$CORE_SCRIPT` (scripts) placeholder replaced with lower-priority content | ✓ | ✓ | ✓ | + +Composition is recursive — multiple composing presets chain. The `PresetResolver.resolve_content()` method walks the full priority stack bottom-up and applies each layer's strategy. + +Content resolution functions for composition: +- **Python**: `PresetResolver.resolve_content()` in `src/specify_cli/presets.py` +- **Bash**: `resolve_template_content()` in `scripts/bash/common.sh` +- **PowerShell**: `Resolve-TemplateContent` in `scripts/powershell/common.ps1` + ## Command Registration When a preset is installed with `type: "command"` entries, the `PresetManager` registers them into all detected agent directories using the shared `CommandRegistrar` from `src/specify_cli/agents.py`. diff --git a/presets/README.md b/presets/README.md index dd3997b239..4072de1d0a 100644 --- a/presets/README.md +++ b/presets/README.md @@ -61,7 +61,37 @@ specify preset add healthcare-compliance --priority 5 # overrides enterprise-sa specify preset add pm-workflow --priority 1 # overrides everything ``` -Presets **override**, they don't merge. If two presets both provide `spec-template`, the one with the lowest priority number wins entirely. +Presets **override by default**, they don't merge. If two presets both provide `spec-template` with the default `replace` strategy, the one with the lowest priority number wins entirely. However, presets can use **composition strategies** to augment rather than replace content. + +### Composition Strategies + +Presets can declare a `strategy` per template to control how content is combined: + +```yaml +provides: + templates: + - type: "template" + name: "spec-template" + file: "templates/spec-addendum.md" + strategy: "append" # adds content after the core template +``` + +| Strategy | Description | +|----------|-------------| +| `replace` (default) | Fully replaces the lower-priority template | +| `prepend` | Places content **before** the resolved lower-priority template, separated by a blank line | +| `append` | Places content **after** the resolved lower-priority template, separated by a blank line | +| `wrap` | Content contains `{CORE_TEMPLATE}` placeholder (or `$CORE_SCRIPT` for scripts) replaced with the lower-priority content | + +**Supported combinations:** + +| Type | `replace` | `prepend` | `append` | `wrap` | +|------|-----------|-----------|----------|--------| +| **template** | ✓ (default) | ✓ | ✓ | ✓ | +| **command** | ✓ (default) | ✓ | ✓ | ✓ | +| **script** | ✓ (default) | — | — | ✓ | + +Multiple composing presets chain recursively. For example, a security preset with `prepend` and a compliance preset with `append` will produce: security header + core content + compliance footer. ## Catalog Management @@ -108,13 +138,5 @@ See [scaffold/](scaffold/) for a scaffold you can copy to create your own preset The following enhancements are under consideration for future releases: -- **Composition strategies** — Allow presets to declare a `strategy` per template instead of the default `replace`: - - | Type | `replace` | `prepend` | `append` | `wrap` | - |------|-----------|-----------|----------|--------| - | **template** | ✓ (default) | ✓ | ✓ | ✓ | - | **command** | ✓ (default) | ✓ | ✓ | ✓ | - | **script** | ✓ (default) | — | — | ✓ | - - For artifacts and commands (which are LLM directives), `wrap` would inject preset content before and after the core template using a `{CORE_TEMPLATE}` placeholder. For scripts, `wrap` would run custom logic before/after the core script via a `$CORE_SCRIPT` variable. -- **Script overrides** — Enable presets to provide alternative versions of core scripts (e.g. `create-new-feature.sh`) for workflow customization. A `strategy: "wrap"` option could allow presets to run custom logic before/after the core script without fully replacing it. +- **Structural merge strategies** — Parsing Markdown sections for per-section granularity (e.g., "replace only ## Security"). +- **Conflict detection** — `specify preset lint` / `specify preset doctor` for detecting composition conflicts. diff --git a/presets/scaffold/preset.yml b/presets/scaffold/preset.yml index 975a92a413..ae7d398fa0 100644 --- a/presets/scaffold/preset.yml +++ b/presets/scaffold/preset.yml @@ -32,6 +32,14 @@ provides: templates: # CUSTOMIZE: Define your template overrides # Templates are document scaffolds (spec-template.md, plan-template.md, etc.) + # + # Strategy options (optional, defaults to "replace"): + # replace - Fully replaces the lower-priority template (default) + # prepend - Places this content BEFORE the lower-priority template + # append - Places this content AFTER the lower-priority template + # wrap - Uses {CORE_TEMPLATE} placeholder, replaced with lower-priority content + # + # Note: Scripts only support "replace" and "wrap" strategies. - type: "template" name: "spec-template" file: "templates/spec-template.md" @@ -45,6 +53,22 @@ provides: # description: "Custom plan template" # replaces: "plan-template" + # COMPOSITION EXAMPLES: + # Append additional sections to an existing template: + # - type: "template" + # name: "spec-template" + # file: "templates/spec-addendum.md" + # description: "Add compliance section to spec template" + # strategy: "append" + # + # Wrap a command with preamble/sign-off: + # - type: "command" + # name: "speckit.specify" + # file: "commands/specify-wrapper.md" + # description: "Wrap specify command with compliance checks" + # strategy: "wrap" + # # In the wrapper file, use {CORE_TEMPLATE} where the original content goes + # OVERRIDE EXTENSION TEMPLATES: # Presets sit above extensions in the resolution stack, so you can # override templates provided by any installed extension. diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index b41d17dec3..58ba7649a2 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -373,3 +373,146 @@ except Exception: return 1 } +# Resolve a template name to composed content using composition strategies. +# Reads strategy metadata from preset manifests and composes content +# from multiple layers using prepend, append, or wrap strategies. +# +# Usage: CONTENT=$(resolve_template_content "template-name" "$REPO_ROOT") +# Returns composed content string on stdout; exit code 1 if not found. +resolve_template_content() { + local template_name="$1" + local repo_root="$2" + local base="$repo_root/.specify/templates" + + # Collect all layers (highest priority first) + local -a layer_paths=() + local -a layer_strategies=() + + # Priority 1: Project overrides (always "replace") + local override="$base/overrides/${template_name}.md" + if [ -f "$override" ]; then + layer_paths+=("$override") + layer_strategies+=("replace") + fi + + # Priority 2: Installed presets (sorted by priority from .registry) + local presets_dir="$repo_root/.specify/presets" + if [ -d "$presets_dir" ]; then + local registry_file="$presets_dir/.registry" + local sorted_presets="" + if [ -f "$registry_file" ] && command -v python3 >/dev/null 2>&1; then + if sorted_presets=$(SPECKIT_REGISTRY="$registry_file" python3 -c " +import json, sys, os +try: + with open(os.environ['SPECKIT_REGISTRY']) as f: + data = json.load(f) + presets = data.get('presets', {}) + for pid, meta in sorted(presets.items(), key=lambda x: x[1].get('priority', 10)): + print(pid) +except Exception: + sys.exit(1) +" 2>/dev/null); then + if [ -n "$sorted_presets" ]; then + while IFS= read -r preset_id; do + local candidate="$presets_dir/$preset_id/templates/${template_name}.md" + if [ -f "$candidate" ]; then + # Read strategy from preset manifest + local strategy="replace" + local manifest="$presets_dir/$preset_id/preset.yml" + if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then + local s + s=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c " +import yaml, sys, os +try: + with open(os.environ['SPECKIT_MANIFEST']) as f: + data = yaml.safe_load(f) + for t in data.get('provides', {}).get('templates', []): + if t.get('name') == os.environ['SPECKIT_TMPL'] and t.get('type', 'template') == 'template': + print(t.get('strategy', 'replace')) + sys.exit(0) + print('replace') +except Exception: + print('replace') +" 2>/dev/null) && strategy="$s" + fi + layer_paths+=("$candidate") + layer_strategies+=("$strategy") + fi + done <<< "$sorted_presets" + fi + fi + fi + fi + + # Priority 3: Extension-provided templates (always "replace") + local ext_dir="$repo_root/.specify/extensions" + if [ -d "$ext_dir" ]; then + for ext in "$ext_dir"/*/; do + [ -d "$ext" ] || continue + case "$(basename "$ext")" in .*) continue;; esac + local candidate="$ext/templates/${template_name}.md" + if [ -f "$candidate" ]; then + layer_paths+=("$candidate") + layer_strategies+=("replace") + fi + done + fi + + # Priority 4: Core templates (always "replace") + local core="$base/${template_name}.md" + if [ -f "$core" ]; then + layer_paths+=("$core") + layer_strategies+=("replace") + fi + + local count=${#layer_paths[@]} + [ "$count" -eq 0 ] && return 1 + + # Check if any layer uses a non-replace strategy + local has_composition=false + for s in "${layer_strategies[@]}"; do + [ "$s" != "replace" ] && has_composition=true && break + done + + if [ "$has_composition" = false ]; then + cat "${layer_paths[0]}" + return 0 + fi + + # Compose bottom-up: start from lowest priority + local content="" + local started=false + local i + for (( i=count-1; i>=0; i-- )); do + local path="${layer_paths[$i]}" + local strat="${layer_strategies[$i]}" + local layer_content + layer_content=$(cat "$path") + + if [ "$started" = false ]; then + if [ "$strat" = "replace" ]; then + content="$layer_content" + fi + # Keep consuming replace layers from the bottom until we hit a non-replace + if [ "$strat" != "replace" ]; then + started=true + case "$strat" in + prepend) content="${layer_content}\n\n${content}" ;; + append) content="${content}\n\n${layer_content}" ;; + wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; + esac + fi + else + case "$strat" in + replace) content="$layer_content" ;; + prepend) content="${layer_content}\n\n${content}" ;; + append) content="${content}\n\n${layer_content}" ;; + wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; + esac + fi + done + + printf '%s' "$content" + return 0 +} + diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index 0d6544aaf4..a499713db0 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -354,3 +354,150 @@ function Resolve-Template { return $null } +# Resolve a template name to composed content using composition strategies. +# Reads strategy metadata from preset manifests and composes content +# from multiple layers using prepend, append, or wrap strategies. +function Resolve-TemplateContent { + param( + [Parameter(Mandatory=$true)][string]$TemplateName, + [Parameter(Mandatory=$true)][string]$RepoRoot + ) + + $base = Join-Path $RepoRoot '.specify/templates' + + # Collect all layers (highest priority first) + $layerPaths = @() + $layerStrategies = @() + + # Priority 1: Project overrides (always "replace") + $override = Join-Path $base "overrides/$TemplateName.md" + if (Test-Path $override) { + $layerPaths += $override + $layerStrategies += 'replace' + } + + # Priority 2: Installed presets (sorted by priority from .registry) + $presetsDir = Join-Path $RepoRoot '.specify/presets' + if (Test-Path $presetsDir) { + $registryFile = Join-Path $presetsDir '.registry' + $sortedPresets = @() + if (Test-Path $registryFile) { + try { + $registryData = Get-Content $registryFile -Raw | ConvertFrom-Json + $presets = $registryData.presets + if ($presets) { + $sortedPresets = $presets.PSObject.Properties | + Sort-Object { if ($null -ne $_.Value.priority) { $_.Value.priority } else { 10 } } | + ForEach-Object { $_.Name } + } + } catch { + $sortedPresets = @() + } + } + + foreach ($presetId in $sortedPresets) { + $candidate = Join-Path $presetsDir "$presetId/templates/$TemplateName.md" + if (Test-Path $candidate) { + # Read strategy from preset manifest + $strategy = 'replace' + $manifest = Join-Path $presetsDir "$presetId/preset.yml" + if (Test-Path $manifest) { + try { + # Use python3 to parse YAML manifest for strategy + $stratResult = & python3 -c @" +import yaml, sys +try: + with open(sys.argv[1]) as f: + data = yaml.safe_load(f) + for t in data.get('provides', {}).get('templates', []): + if t.get('name') == sys.argv[2] and t.get('type', 'template') == 'template': + print(t.get('strategy', 'replace')) + sys.exit(0) + print('replace') +except Exception: + print('replace') +"@ $manifest $TemplateName 2>$null + if ($stratResult) { $strategy = $stratResult.Trim() } + } catch { + $strategy = 'replace' + } + } + $layerPaths += $candidate + $layerStrategies += $strategy + } + } + + if ($sortedPresets.Count -eq 0) { + foreach ($preset in Get-ChildItem -Path $presetsDir -Directory -ErrorAction SilentlyContinue | Where-Object { $_.Name -notlike '.*' }) { + $candidate = Join-Path $preset.FullName "templates/$TemplateName.md" + if (Test-Path $candidate) { + $layerPaths += $candidate + $layerStrategies += 'replace' + } + } + } + } + + # Priority 3: Extension-provided templates (always "replace") + $extDir = Join-Path $RepoRoot '.specify/extensions' + if (Test-Path $extDir) { + foreach ($ext in Get-ChildItem -Path $extDir -Directory -ErrorAction SilentlyContinue | Where-Object { $_.Name -notlike '.*' } | Sort-Object Name) { + $candidate = Join-Path $ext.FullName "templates/$TemplateName.md" + if (Test-Path $candidate) { + $layerPaths += $candidate + $layerStrategies += 'replace' + } + } + } + + # Priority 4: Core templates (always "replace") + $core = Join-Path $base "$TemplateName.md" + if (Test-Path $core) { + $layerPaths += $core + $layerStrategies += 'replace' + } + + if ($layerPaths.Count -eq 0) { return $null } + + # Check if any layer uses a non-replace strategy + $hasComposition = $false + foreach ($s in $layerStrategies) { + if ($s -ne 'replace') { $hasComposition = $true; break } + } + + if (-not $hasComposition) { + return (Get-Content $layerPaths[0] -Raw) + } + + # Compose bottom-up: start from lowest priority + $content = $null + $started = $false + for ($i = $layerPaths.Count - 1; $i -ge 0; $i--) { + $path = $layerPaths[$i] + $strat = $layerStrategies[$i] + $layerContent = Get-Content $path -Raw + + if (-not $started) { + if ($strat -eq 'replace') { + $content = $layerContent + } + if ($strat -ne 'replace') { + $started = $true + switch ($strat) { + 'prepend' { $content = "$layerContent`n`n$content" } + 'append' { $content = "$content`n`n$layerContent" } + 'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content } + } + } + } else { + switch ($strat) { + 'replace' { $content = $layerContent } + 'prepend' { $content = "$layerContent`n`n$content" } + 'append' { $content = "$content`n`n$layerContent" } + 'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content } + } + } + } + + return $content +} \ No newline at end of file diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 8c6fd02b9f..eb20bbc85f 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2601,6 +2601,17 @@ def preset_resolve( if result: console.print(f" [bold]{template_name}[/bold]: {result['path']}") console.print(f" [dim](from: {result['source']})[/dim]") + + # Show composition chain if any layers use non-replace strategies + layers = resolver._collect_all_layers(template_name) + has_composition = any(layer["strategy"] != "replace" for layer in layers) + if has_composition: + console.print("\n [bold]Composition chain:[/bold]") + for i, layer in enumerate(reversed(layers)): + strategy_label = layer["strategy"] + if strategy_label == "replace": + strategy_label = "base" + console.print(f" {i + 1}. [{strategy_label}] {layer['source']} → {layer['path']}") else: console.print(f" [yellow]{template_name}[/yellow]: not found") console.print(" [dim]No template with this name exists in the resolution stack[/dim]") diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index d5513c8323..fb619fbf28 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -53,6 +53,9 @@ class PresetCompatibilityError(PresetError): VALID_PRESET_TEMPLATE_TYPES = {"template", "command", "script"} +VALID_PRESET_STRATEGIES = {"replace", "prepend", "append", "wrap"} +# Scripts only support replace and wrap (prepend/append don't make semantic sense for executable code) +VALID_SCRIPT_STRATEGIES = {"replace", "wrap"} class PresetManifest: @@ -151,6 +154,19 @@ def _validate(self): "must be a relative path within the preset directory" ) + # Validate strategy field (optional, defaults to "replace") + strategy = tmpl.get("strategy", "replace") + if strategy not in VALID_PRESET_STRATEGIES: + raise PresetValidationError( + f"Invalid strategy '{strategy}': " + f"must be one of {sorted(VALID_PRESET_STRATEGIES)}" + ) + if tmpl["type"] == "script" and strategy not in VALID_SCRIPT_STRATEGIES: + raise PresetValidationError( + f"Invalid strategy '{strategy}' for script: " + f"scripts only support {sorted(VALID_SCRIPT_STRATEGIES)}" + ) + # Validate template name format if tmpl["type"] == "command": # Commands use dot notation (e.g. speckit.specify) @@ -502,6 +518,10 @@ def _register_commands( file, and writes it to every detected agent directory using the CommandRegistrar from the agents module. + When a command uses a composition strategy (prepend, append, wrap), + the content is composed with the lower-priority command before + registration. + Args: manifest: Preset manifest preset_dir: Installed preset directory @@ -531,6 +551,31 @@ def _register_commands( if not filtered: return {} + # Handle composition strategies: resolve composed content for non-replace commands + resolver = PresetResolver(self.project_root) + composed_dir = None + commands_to_register = [] + for cmd in filtered: + strategy = cmd.get("strategy", "replace") + if strategy != "replace": + # Resolve composed content using the full priority stack + composed = resolver.resolve_content(cmd["name"], "command") + if composed: + # Write composed content to a temporary subdirectory + if composed_dir is None: + composed_dir = preset_dir / ".composed" + composed_dir.mkdir(parents=True, exist_ok=True) + composed_file = composed_dir / f"{cmd['name']}.md" + composed_file.write_text(composed, encoding="utf-8") + commands_to_register.append({ + **cmd, + "file": f".composed/{cmd['name']}.md", + }) + else: + commands_to_register.append(cmd) + else: + commands_to_register.append(cmd) + try: from .agents import CommandRegistrar except ImportError: @@ -538,7 +583,7 @@ def _register_commands( registrar = CommandRegistrar() return registrar.register_commands_for_all_agents( - filtered, manifest.id, preset_dir, self.project_root + commands_to_register, manifest.id, preset_dir, self.project_root ) def _unregister_commands(self, registered_commands: Dict[str, List[str]]) -> None: @@ -1873,3 +1918,200 @@ def resolve_with_source( continue return {"path": resolved_str, "source": "core"} + + def _collect_all_layers( + self, + template_name: str, + template_type: str = "template", + ) -> List[Dict[str, Any]]: + """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. + + Args: + template_name: Template name (e.g., "spec-template") + template_type: Template type ("template", "command", or "script") + + Returns: + List of layer dicts ordered highest-to-lowest priority. + """ + if template_type == "template": + subdirs = ["templates", ""] + elif template_type == "command": + subdirs = ["commands"] + elif template_type == "script": + subdirs = ["scripts"] + else: + subdirs = [""] + + ext = ".md" + if template_type == "script": + ext = ".sh" + + layers: List[Dict[str, Any]] = [] + + def _find_in_subdirs(base_dir: Path) -> Optional[Path]: + for subdir in subdirs: + if subdir: + candidate = base_dir / subdir / f"{template_name}{ext}" + else: + candidate = base_dir / f"{template_name}{ext}" + if candidate.exists(): + 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", + }) + + # Priority 2: Installed presets (sorted by priority — lower number = higher precedence) + if self.presets_dir.exists(): + registry = PresetRegistry(self.presets_dir) + for pack_id, metadata in registry.list_by_priority(): + pack_dir = self.presets_dir / pack_id + candidate = _find_in_subdirs(pack_dir) + if candidate: + # Read strategy from preset manifest + strategy = "replace" + manifest_path = pack_dir / "preset.yml" + if manifest_path.exists(): + try: + manifest = PresetManifest(manifest_path) + for tmpl in manifest.templates: + if (tmpl.get("name") == template_name + and tmpl.get("type") == template_type): + strategy = tmpl.get("strategy", "replace") + break + except PresetValidationError: + pass + version = metadata.get("version", "?") if metadata else "?" + layers.append({ + "path": candidate, + "source": f"{pack_id} v{version}", + "strategy": strategy, + }) + + # Priority 3: Extension-provided templates (always "replace") + for _priority, ext_id, ext_meta in self._get_all_extensions_by_priority(): + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + 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", + }) + + # Priority 4: Core templates (always "replace") + core = None + if template_type == "template": + c = self.templates_dir / f"{template_name}.md" + if c.exists(): + core = c + elif template_type == "command": + c = self.templates_dir / "commands" / f"{template_name}.md" + if c.exists(): + core = c + elif template_type == "script": + c = self.templates_dir / "scripts" / f"{template_name}{ext}" + if c.exists(): + core = c + if core: + layers.append({ + "path": core, + "source": "core", + "strategy": "replace", + }) + + return layers + + def resolve_content( + self, + template_name: str, + template_type: str = "template", + ) -> Optional[str]: + """Resolve a template name and return composed content. + + Walks the priority stack and composes content using strategies: + - replace (default): highest-priority content wins entirely + - prepend: content is placed before lower-priority content + - append: content is placed after lower-priority content + - wrap: content contains {CORE_TEMPLATE} placeholder replaced + with lower-priority content (or $CORE_SCRIPT for scripts) + + Composition is recursive — multiple composing presets chain. + + Args: + template_name: Template name (e.g., "spec-template") + template_type: Template type ("template", "command", or "script") + + Returns: + Composed content string, or None if not found + """ + layers = self._collect_all_layers(template_name, template_type) + if not layers: + return None + + # Check if any layer uses a non-replace strategy + has_composition = any(layer["strategy"] != "replace" for layer in layers) + + if not has_composition: + # Pure replacement — just read the highest-priority file + return layers[0]["path"].read_text(encoding="utf-8") + + # Composition: build content bottom-up (lowest priority first) + # Start from the lowest-priority "replace" layer as the base, + # then apply composition layers on top. + # + # layers is ordered highest-priority first. We process in reverse. + reversed_layers = list(reversed(layers)) + + # Find the base content: the first "replace" layer from the bottom + content = None + start_idx = 0 + for i, layer in enumerate(reversed_layers): + if layer["strategy"] == "replace": + content = layer["path"].read_text(encoding="utf-8") + start_idx = i + 1 + else: + # Once we hit a non-replace layer, stop looking for base + break + + # If no base content found, there's nothing to compose onto + if content is None: + return None + + # Apply composition layers from bottom to top + for layer in reversed_layers[start_idx:]: + layer_content = layer["path"].read_text(encoding="utf-8") + strategy = layer["strategy"] + + if strategy == "replace": + content = layer_content + elif strategy == "prepend": + content = layer_content + "\n\n" + content + elif strategy == "append": + content = content + "\n\n" + layer_content + elif strategy == "wrap": + if template_type == "script": + content = layer_content.replace("$CORE_SCRIPT", content) + else: + content = layer_content.replace("{CORE_TEMPLATE}", content) + + return content diff --git a/tests/test_presets.py b/tests/test_presets.py index 60322b99a1..a44b417c86 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3041,3 +3041,554 @@ def test_bundled_preset_missing_locally_cli_error(self, project_dir): output = strip_ansi(result.output).lower() assert "bundled" in output, result.output assert "reinstall" in output, result.output +# ===== Composition Strategy Tests ===== + + +class TestCompositionStrategyValidation: + """Test strategy field validation in PresetManifest.""" + + def test_valid_replace_strategy(self, temp_dir, valid_pack_data): + """Test that replace strategy is accepted.""" + valid_pack_data["provides"]["templates"][0]["strategy"] = "replace" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + (temp_dir / "templates").mkdir(exist_ok=True) + (temp_dir / "templates" / "spec-template.md").write_text("test") + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "replace" + + def test_valid_prepend_strategy(self, temp_dir, valid_pack_data): + """Test that prepend strategy is accepted for templates.""" + valid_pack_data["provides"]["templates"][0]["strategy"] = "prepend" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + (temp_dir / "templates").mkdir(exist_ok=True) + (temp_dir / "templates" / "spec-template.md").write_text("test") + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "prepend" + + def test_valid_append_strategy(self, temp_dir, valid_pack_data): + """Test that append strategy is accepted for templates.""" + valid_pack_data["provides"]["templates"][0]["strategy"] = "append" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + (temp_dir / "templates").mkdir(exist_ok=True) + (temp_dir / "templates" / "spec-template.md").write_text("test") + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "append" + + def test_valid_wrap_strategy(self, temp_dir, valid_pack_data): + """Test that wrap strategy is accepted for templates.""" + valid_pack_data["provides"]["templates"][0]["strategy"] = "wrap" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + (temp_dir / "templates").mkdir(exist_ok=True) + (temp_dir / "templates" / "spec-template.md").write_text("test") + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "wrap" + + def test_default_strategy_is_replace(self, pack_dir): + """Test that omitting strategy defaults to replace.""" + manifest = PresetManifest(pack_dir / "preset.yml") + assert manifest.templates[0].get("strategy", "replace") == "replace" + + def test_invalid_strategy_rejected(self, temp_dir, valid_pack_data): + """Test that invalid strategy values are rejected.""" + valid_pack_data["provides"]["templates"][0]["strategy"] = "merge" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + with pytest.raises(PresetValidationError, match="Invalid strategy"): + PresetManifest(manifest_path) + + def test_prepend_rejected_for_scripts(self, temp_dir, valid_pack_data): + """Test that prepend strategy is rejected for scripts.""" + valid_pack_data["provides"]["templates"] = [{ + "type": "script", + "name": "create-new-feature", + "file": "scripts/create-new-feature.sh", + "strategy": "prepend", + }] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + with pytest.raises(PresetValidationError, match="Invalid strategy.*for script"): + PresetManifest(manifest_path) + + def test_append_rejected_for_scripts(self, temp_dir, valid_pack_data): + """Test that append strategy is rejected for scripts.""" + valid_pack_data["provides"]["templates"] = [{ + "type": "script", + "name": "create-new-feature", + "file": "scripts/create-new-feature.sh", + "strategy": "append", + }] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + with pytest.raises(PresetValidationError, match="Invalid strategy.*for script"): + PresetManifest(manifest_path) + + def test_wrap_accepted_for_scripts(self, temp_dir, valid_pack_data): + """Test that wrap strategy is accepted for scripts.""" + valid_pack_data["provides"]["templates"] = [{ + "type": "script", + "name": "create-new-feature", + "file": "scripts/create-new-feature.sh", + "strategy": "wrap", + }] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "wrap" + + def test_replace_accepted_for_scripts(self, temp_dir, valid_pack_data): + """Test that replace strategy is accepted for scripts.""" + valid_pack_data["provides"]["templates"] = [{ + "type": "script", + "name": "create-new-feature", + "file": "scripts/create-new-feature.sh", + "strategy": "replace", + }] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "replace" + + def test_prepend_accepted_for_commands(self, temp_dir, valid_pack_data): + """Test that prepend strategy is accepted for commands.""" + valid_pack_data["provides"]["templates"] = [{ + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + "strategy": "prepend", + }] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + manifest = PresetManifest(manifest_path) + assert manifest.templates[0]["strategy"] == "prepend" + + +class TestResolveContent: + """Test PresetResolver.resolve_content() composition.""" + + def test_resolve_content_core_template(self, project_dir): + """Test resolve_content returns core template when no composition.""" + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Core Spec Template" in content + + def test_resolve_content_nonexistent(self, project_dir): + """Test resolve_content returns None for nonexistent template.""" + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("nonexistent") + assert content is None + + def test_resolve_content_replace_strategy(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with default replace strategy.""" + manager = PresetManager(project_dir) + manager.install_from_directory( + _create_pack(temp_dir, valid_pack_data, "replace-pack", + "# Replaced Content\n"), + "0.1.5" + ) + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Replaced Content" in content + assert "Core Spec Template" not in content + + def test_resolve_content_append_strategy(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with append strategy.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "append-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Appended Section\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Core Spec Template" in content + assert "Appended Section" in content + # Core should come first, appended after + assert content.index("Core Spec Template") < content.index("Appended Section") + + def test_resolve_content_prepend_strategy(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with prepend strategy.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "prepend-pack", "name": "Prepend"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "prepend", + }] + } + pack_dir = temp_dir / "prepend-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Security Header\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Security Header" in content + assert "Core Spec Template" in content + # Prepended content should come first + assert content.index("Security Header") < content.index("Core Spec Template") + + def test_resolve_content_wrap_strategy(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with wrap strategy for templates.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "wrap-pack", "name": "Wrap"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "wrap", + }] + } + pack_dir = temp_dir / "wrap-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text( + "# Wrapper Start\n\n{CORE_TEMPLATE}\n\n# Wrapper End\n" + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Wrapper Start" in content + assert "Core Spec Template" in content + assert "Wrapper End" in content + # Wrapper should surround core + assert content.index("Wrapper Start") < content.index("Core Spec Template") + assert content.index("Core Spec Template") < content.index("Wrapper End") + + def test_resolve_content_wrap_strategy_script(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with wrap strategy for scripts uses $CORE_SCRIPT.""" + # Create core script + scripts_dir = project_dir / ".specify" / "templates" / "scripts" + scripts_dir.mkdir(parents=True, exist_ok=True) + (scripts_dir / "test-script.sh").write_text("echo 'core script'\n") + + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "script-wrap", "name": "Script Wrap"} + pack_data["provides"] = { + "templates": [{ + "type": "script", + "name": "test-script", + "file": "scripts/test-script.sh", + "strategy": "wrap", + }] + } + pack_dir = temp_dir / "script-wrap" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "scripts").mkdir() + (pack_dir / "scripts" / "test-script.sh").write_text( + "#!/bin/bash\necho 'before'\n$CORE_SCRIPT\necho 'after'\n" + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("test-script", "script") + assert content is not None + assert "echo 'before'" in content + assert "echo 'core script'" in content + assert "echo 'after'" in content + + def test_resolve_content_multi_preset_chain(self, project_dir, temp_dir, valid_pack_data): + """Test multi-preset composition chain: prepend + append stacking.""" + # Create preset A (priority 1): prepend security header + pack_a_data = {**valid_pack_data} + pack_a_data["preset"] = {**valid_pack_data["preset"], "id": "preset-a", "name": "A"} + pack_a_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "prepend", + }] + } + pack_a_dir = temp_dir / "preset-a" + pack_a_dir.mkdir() + with open(pack_a_dir / "preset.yml", 'w') as f: + yaml.dump(pack_a_data, f) + (pack_a_dir / "templates").mkdir() + (pack_a_dir / "templates" / "spec-template.md").write_text("## Security Header\n") + + # Create preset B (priority 2): append compliance footer + pack_b_data = {**valid_pack_data} + pack_b_data["preset"] = {**valid_pack_data["preset"], "id": "preset-b", "name": "B"} + pack_b_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_b_dir = temp_dir / "preset-b" + pack_b_dir.mkdir() + with open(pack_b_dir / "preset.yml", 'w') as f: + yaml.dump(pack_b_data, f) + (pack_b_dir / "templates").mkdir() + (pack_b_dir / "templates" / "spec-template.md").write_text("## Compliance Footer\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_a_dir, "0.1.5", priority=1) + manager.install_from_directory(pack_b_dir, "0.1.5", priority=2) + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + # Result: + + + assert "Security Header" in content + assert "Core Spec Template" in content + assert "Compliance Footer" in content + assert content.index("Security Header") < content.index("Core Spec Template") + assert content.index("Core Spec Template") < content.index("Compliance Footer") + + def test_resolve_content_override_trumps_composition(self, project_dir, temp_dir, valid_pack_data): + """Test that project overrides trump composition (replace at top priority).""" + # Install a composing preset + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "append-pack", "name": "Append"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "append-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Appended\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + # Create project override (replaces everything) + overrides_dir = project_dir / ".specify" / "templates" / "overrides" + overrides_dir.mkdir(parents=True) + (overrides_dir / "spec-template.md").write_text("# Override Only\n") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content is not None + assert "Override Only" in content + # Override replaces, so appended content should not be visible + assert "Core Spec Template" not in content + + def test_resolve_content_command_type(self, project_dir, temp_dir, valid_pack_data): + """Test resolve_content with command template type.""" + # Create core command + commands_dir = project_dir / ".specify" / "templates" / "commands" + commands_dir.mkdir(parents=True, exist_ok=True) + (commands_dir / "speckit.plan.md").write_text("# Core Plan Command\n") + + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "cmd-append", "name": "CmdAppend"} + pack_data["provides"] = { + "templates": [{ + "type": "command", + "name": "speckit.plan", + "file": "commands/speckit.plan.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "cmd-append" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "commands").mkdir() + (pack_dir / "commands" / "speckit.plan.md").write_text("## Additional Instructions\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("speckit.plan", "command") + assert content is not None + assert "Core Plan Command" in content + assert "Additional Instructions" in content + + def test_resolve_content_blank_line_separator(self, project_dir, temp_dir, valid_pack_data): + """Test that prepend/append use blank line separator.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "sep-test", "name": "SepTest"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "sep-test" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("appended") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + # Should have blank line separator + assert "\n\n" in content + + +class TestCollectAllLayers: + """Test PresetResolver._collect_all_layers() method.""" + + def test_single_core_layer(self, project_dir): + """Test collecting layers with only core template.""" + resolver = PresetResolver(project_dir) + layers = resolver._collect_all_layers("spec-template") + assert len(layers) == 1 + assert layers[0]["source"] == "core" + assert layers[0]["strategy"] == "replace" + + def test_layers_include_presets(self, project_dir, temp_dir, valid_pack_data): + """Test that layers include installed preset.""" + manager = PresetManager(project_dir) + pack_dir = _create_pack(temp_dir, valid_pack_data, "test-pack", + "# From Pack\n") + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + layers = resolver._collect_all_layers("spec-template") + assert len(layers) == 2 + # Highest priority first + assert "test-pack" in layers[0]["source"] + assert layers[1]["source"] == "core" + + def test_layers_order_matches_priority(self, project_dir, temp_dir, valid_pack_data): + """Test that layers are ordered by priority (highest first).""" + manager = PresetManager(project_dir) + for pid, prio in [("pack-lo", 10), ("pack-hi", 1)]: + d = {**valid_pack_data} + d["preset"] = {**valid_pack_data["preset"], "id": pid, "name": pid} + p = temp_dir / pid + p.mkdir() + with open(p / "preset.yml", 'w') as f: + yaml.dump(d, f) + (p / "templates").mkdir() + (p / "templates" / "spec-template.md").write_text(f"# {pid}\n") + manager.install_from_directory(p, "0.1.5", priority=prio) + + resolver = PresetResolver(project_dir) + layers = resolver._collect_all_layers("spec-template") + assert len(layers) == 3 # pack-hi, pack-lo, core + assert "pack-hi" in layers[0]["source"] + assert "pack-lo" in layers[1]["source"] + assert layers[2]["source"] == "core" + + def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_pack_data): + """Test that layers read strategy from preset manifest.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": "strat-pack", "name": "Strat"} + pack_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "append", + }] + } + pack_dir = temp_dir / "strat-pack" + pack_dir.mkdir() + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + (pack_dir / "templates").mkdir() + (pack_dir / "templates" / "spec-template.md").write_text("## Footer\n") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + layers = resolver._collect_all_layers("spec-template") + # Preset layer should have strategy=append + assert layers[0]["strategy"] == "append" + # Core layer should be replace + assert layers[1]["strategy"] == "replace" + + +def _create_pack(temp_dir, valid_pack_data, pack_id, content, + strategy="replace", template_type="template", + template_name="spec-template"): + """Helper to create a preset pack directory.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": pack_id, "name": pack_id} + + tmpl_entry = { + "type": template_type, + "name": template_name, + "file": f"templates/{template_name}.md" if template_type != "script" else f"scripts/{template_name}.sh", + } + if strategy != "replace": + tmpl_entry["strategy"] = strategy + pack_data["provides"] = {"templates": [tmpl_entry]} + + pack_dir = temp_dir / pack_id + pack_dir.mkdir(exist_ok=True) + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + + if template_type == "script": + subdir = pack_dir / "scripts" + subdir.mkdir(exist_ok=True) + (subdir / f"{template_name}.sh").write_text(content) + else: + subdir = pack_dir / "templates" + subdir.mkdir(exist_ok=True) + (subdir / f"{template_name}.md").write_text(content) + + return pack_dir From 753addc9b719ee969d1efdbaee2e15a212a42c55 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 21:08:51 +0000 Subject: [PATCH 03/16] fix: Address code review feedback - edge case in bash/ps1 composition, improve default strategy test Agent-Logs-Url: https://github.com/github/spec-kit/sessions/c285c51b-f00b-480a-9eb2-ae70e3cbcb72 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> --- scripts/bash/common.sh | 4 ++++ scripts/powershell/common.ps1 | 2 ++ tests/test_presets.py | 5 ++++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index 58ba7649a2..6e25434414 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -481,6 +481,7 @@ except Exception: # Compose bottom-up: start from lowest priority local content="" + local has_base=false local started=false local i for (( i=count-1; i>=0; i-- )); do @@ -492,9 +493,12 @@ except Exception: if [ "$started" = false ]; then if [ "$strat" = "replace" ]; then content="$layer_content" + has_base=true fi # Keep consuming replace layers from the bottom until we hit a non-replace if [ "$strat" != "replace" ]; then + # No base content to compose onto + [ "$has_base" = false ] && return 1 started=true case "$strat" in prepend) content="${layer_content}\n\n${content}" ;; diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index a499713db0..1c75a9a2a1 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -482,6 +482,8 @@ except Exception: $content = $layerContent } if ($strat -ne 'replace') { + # No base content to compose onto + if ($null -eq $content) { return $null } $started = $true switch ($strat) { 'prepend' { $content = "$layerContent`n`n$content" } diff --git a/tests/test_presets.py b/tests/test_presets.py index a44b417c86..e08aa35c3d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3092,8 +3092,11 @@ def test_valid_wrap_strategy(self, temp_dir, valid_pack_data): assert manifest.templates[0]["strategy"] == "wrap" def test_default_strategy_is_replace(self, pack_dir): - """Test that omitting strategy defaults to replace.""" + """Test that omitting strategy defaults to replace (key is absent).""" manifest = PresetManifest(pack_dir / "preset.yml") + # Strategy key should not be present in the manifest data + assert "strategy" not in manifest.templates[0] + # But consumers should treat missing strategy as "replace" assert manifest.templates[0].get("strategy", "replace") == "replace" def test_invalid_strategy_rejected(self, temp_dir, valid_pack_data): From fa0482845ebe1fad40a23abb12deeea24130cca1 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 13:50:39 -0500 Subject: [PATCH 04/16] fix: add explanatory comment to empty except block in layer resolution Address code-quality review: explain why PresetValidationError is intentionally caught and ignored (falls back to default 'replace' strategy so layer resolution continues). --- src/specify_cli/presets.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index fb619fbf28..ef2795732c 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -1992,6 +1992,8 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: strategy = tmpl.get("strategy", "replace") break except PresetValidationError: + # Invalid manifest — fall back to default "replace" + # strategy so layer resolution still works. pass version = metadata.get("version", "?") if metadata else "?" layers.append({ From 18346df58f53ff8e9a250d255272e2e55b8394d6 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 14:20:45 -0500 Subject: [PATCH 05/16] fix: address Copilot PR review feedback on composition strategies - Fix bash newline handling: use printf instead of literal \n\n in prepend/append string concatenation (common.sh) - Fix PowerShell wrap: use .Replace() instead of -replace to avoid $ capture-group interpolation in replacement strings (common.ps1) - Fix layer collection to use manifest file path when available, falling back to convention-based path (presets.py, common.sh, common.ps1) - Fix install ordering: pre-register preset before command/skill registration so resolve_content() sees it in the priority stack - Fix skills divergence: use composed content from .composed/ dir when available instead of original command file for skill generation - Add fallback directory scan in bash resolve_template_content when python3/registry is unavailable, matching resolve_template() behavior - Clarify doc examples: note that file field can differ from convention path (scaffold/preset.yml, presets/README.md) --- presets/README.md | 2 +- presets/scaffold/preset.yml | 4 ++ scripts/bash/common.sh | 67 ++++++++++++++++++++++++--------- scripts/powershell/common.ps1 | 48 ++++++++++++++--------- src/specify_cli/presets.py | 71 ++++++++++++++++++++++++----------- 5 files changed, 135 insertions(+), 57 deletions(-) diff --git a/presets/README.md b/presets/README.md index 4072de1d0a..7d7b9ae8a2 100644 --- a/presets/README.md +++ b/presets/README.md @@ -65,7 +65,7 @@ Presets **override by default**, they don't merge. If two presets both provide ` ### Composition Strategies -Presets can declare a `strategy` per template to control how content is combined: +Presets can declare a `strategy` per template to control how content is combined. The `name` field identifies which template to compose with in the priority stack, while `file` points to the actual content file (which can differ from the convention path `templates/.md`): ```yaml provides: diff --git a/presets/scaffold/preset.yml b/presets/scaffold/preset.yml index ae7d398fa0..0180c3375a 100644 --- a/presets/scaffold/preset.yml +++ b/presets/scaffold/preset.yml @@ -54,6 +54,10 @@ provides: # replaces: "plan-template" # COMPOSITION EXAMPLES: + # The `file` field points to the content file (can differ from the + # convention path `templates/.md`). The `name` field identifies + # which template to compose with in the priority stack. + # # Append additional sections to an existing template: # - type: "template" # name: "spec-template" diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index 6e25434414..ce7d536eb0 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -414,33 +414,66 @@ except Exception: " 2>/dev/null); then if [ -n "$sorted_presets" ]; then while IFS= read -r preset_id; do - local candidate="$presets_dir/$preset_id/templates/${template_name}.md" - if [ -f "$candidate" ]; then - # Read strategy from preset manifest - local strategy="replace" - local manifest="$presets_dir/$preset_id/preset.yml" - if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then - local s - s=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c " + # Read strategy and file path from preset manifest + local strategy="replace" + local manifest_file="" + local manifest="$presets_dir/$preset_id/preset.yml" + if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then + local result + result=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c " import yaml, sys, os try: with open(os.environ['SPECKIT_MANIFEST']) as f: data = yaml.safe_load(f) for t in data.get('provides', {}).get('templates', []): if t.get('name') == os.environ['SPECKIT_TMPL'] and t.get('type', 'template') == 'template': - print(t.get('strategy', 'replace')) + print(t.get('strategy', 'replace') + '\t' + t.get('file', '')) sys.exit(0) - print('replace') + print('replace\t') except Exception: - print('replace') -" 2>/dev/null) && strategy="$s" - fi + print('replace\t') +" 2>/dev/null) && { + strategy="${result%% *}" + manifest_file="${result#* }" + } + fi + # Try manifest file path first, then convention path + local candidate="" + if [ -n "$manifest_file" ]; then + local mf="$presets_dir/$preset_id/$manifest_file" + [ -f "$mf" ] && candidate="$mf" + fi + if [ -z "$candidate" ]; then + local cf="$presets_dir/$preset_id/templates/${template_name}.md" + [ -f "$cf" ] && candidate="$cf" + fi + if [ -n "$candidate" ]; then layer_paths+=("$candidate") layer_strategies+=("$strategy") fi done <<< "$sorted_presets" fi + else + # python3 failed — fall back to unordered directory scan (replace only) + for preset in "$presets_dir"/*/; do + [ -d "$preset" ] || continue + local candidate="$preset/templates/${template_name}.md" + if [ -f "$candidate" ]; then + layer_paths+=("$candidate") + layer_strategies+=("replace") + fi + done fi + else + # No python3 or registry — fall back to unordered directory scan (replace only) + for preset in "$presets_dir"/*/; do + [ -d "$preset" ] || continue + local candidate="$preset/templates/${template_name}.md" + if [ -f "$candidate" ]; then + layer_paths+=("$candidate") + layer_strategies+=("replace") + fi + done fi fi @@ -501,16 +534,16 @@ except Exception: [ "$has_base" = false ] && return 1 started=true case "$strat" in - prepend) content="${layer_content}\n\n${content}" ;; - append) content="${content}\n\n${layer_content}" ;; + prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; + append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; esac fi else case "$strat" in replace) content="$layer_content" ;; - prepend) content="${layer_content}\n\n${content}" ;; - append) content="${content}\n\n${layer_content}" ;; + prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; + append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; esac fi diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index 1c75a9a2a1..8347d6a888 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -396,32 +396,46 @@ function Resolve-TemplateContent { } foreach ($presetId in $sortedPresets) { - $candidate = Join-Path $presetsDir "$presetId/templates/$TemplateName.md" - if (Test-Path $candidate) { - # Read strategy from preset manifest - $strategy = 'replace' - $manifest = Join-Path $presetsDir "$presetId/preset.yml" - if (Test-Path $manifest) { - try { - # Use python3 to parse YAML manifest for strategy - $stratResult = & python3 -c @" + # Read strategy and file path from preset manifest + $strategy = 'replace' + $manifestFilePath = '' + $manifest = Join-Path $presetsDir "$presetId/preset.yml" + if (Test-Path $manifest) { + try { + # Use python3 to parse YAML manifest for strategy and file path + $stratResult = & python3 -c @" import yaml, sys try: with open(sys.argv[1]) as f: data = yaml.safe_load(f) for t in data.get('provides', {}).get('templates', []): if t.get('name') == sys.argv[2] and t.get('type', 'template') == 'template': - print(t.get('strategy', 'replace')) + print(t.get('strategy', 'replace') + '\t' + t.get('file', '')) sys.exit(0) - print('replace') + print('replace\t') except Exception: - print('replace') + print('replace\t') "@ $manifest $TemplateName 2>$null - if ($stratResult) { $strategy = $stratResult.Trim() } - } catch { - $strategy = 'replace' + if ($stratResult) { + $parts = $stratResult.Trim() -split "`t", 2 + $strategy = $parts[0] + if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] } } + } catch { + $strategy = 'replace' } + } + # Try manifest file path first, then convention path + $candidate = $null + if ($manifestFilePath) { + $mf = Join-Path $presetsDir "$presetId/$manifestFilePath" + if (Test-Path $mf) { $candidate = $mf } + } + if (-not $candidate) { + $cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md" + if (Test-Path $cf) { $candidate = $cf } + } + if ($candidate) { $layerPaths += $candidate $layerStrategies += $strategy } @@ -488,7 +502,7 @@ except Exception: switch ($strat) { 'prepend' { $content = "$layerContent`n`n$content" } 'append' { $content = "$content`n`n$layerContent" } - 'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content } + 'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) } } } } else { @@ -496,7 +510,7 @@ except Exception: 'replace' { $content = $layerContent } 'prepend' { $content = "$layerContent`n`n$content" } 'append' { $content = "$content`n`n$layerContent" } - 'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content } + 'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) } } } } diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index ef2795732c..36177ad159 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -779,6 +779,12 @@ def _register_skills( if not source_file.exists(): continue + # Use composed content if available (written by _register_commands + # for commands with non-replace strategies), otherwise the original. + composed_file = preset_dir / ".composed" / f"{cmd_name}.md" + if composed_file.exists(): + source_file = composed_file + # Derive the short command name (e.g. "specify" from "speckit.specify") raw_short_name = cmd_name if raw_short_name.startswith("speckit."): @@ -1013,18 +1019,30 @@ def install_from_directory( shutil.copytree(source_dir, dest_dir) - # Register command overrides with AI agents - registered_commands = self._register_commands(manifest, dest_dir) - - # Update corresponding skills when --ai-skills was previously used - registered_skills = self._register_skills(manifest, dest_dir) - + # Pre-register the preset so that composition resolution can see it + # in the priority stack when resolving composed command content. self.registry.add(manifest.id, { "version": manifest.version, "source": "local", "manifest_hash": manifest.get_hash(), "enabled": True, "priority": priority, + "registered_commands": {}, + "registered_skills": [], + }) + + try: + # Register command overrides with AI agents + registered_commands = self._register_commands(manifest, dest_dir) + + # Update corresponding skills when --ai-skills was previously used + registered_skills = self._register_skills(manifest, dest_dir) + except Exception: + # Roll back registry entry on failure + self.registry.remove(manifest.id) + raise + + self.registry.update(manifest.id, { "registered_commands": registered_commands, "registered_skills": registered_skills, }) @@ -1978,23 +1996,32 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: registry = PresetRegistry(self.presets_dir) for pack_id, metadata in registry.list_by_priority(): pack_dir = self.presets_dir / pack_id - candidate = _find_in_subdirs(pack_dir) + # Read strategy and manifest file path from preset manifest + strategy = "replace" + manifest_file_path = None + manifest_path = pack_dir / "preset.yml" + if manifest_path.exists(): + try: + manifest = PresetManifest(manifest_path) + for tmpl in manifest.templates: + if (tmpl.get("name") == template_name + and tmpl.get("type") == template_type): + strategy = tmpl.get("strategy", "replace") + manifest_file_path = tmpl.get("file") + break + except PresetValidationError: + # Invalid manifest — fall back to default "replace" + # strategy so layer resolution still works. + pass + # Use manifest file path if specified, otherwise convention-based lookup + candidate = None + if manifest_file_path: + manifest_candidate = pack_dir / manifest_file_path + if manifest_candidate.exists(): + candidate = manifest_candidate + if candidate is None: + candidate = _find_in_subdirs(pack_dir) if candidate: - # Read strategy from preset manifest - strategy = "replace" - manifest_path = pack_dir / "preset.yml" - if manifest_path.exists(): - try: - manifest = PresetManifest(manifest_path) - for tmpl in manifest.templates: - if (tmpl.get("name") == template_name - and tmpl.get("type") == template_type): - strategy = tmpl.get("strategy", "replace") - break - except PresetValidationError: - # Invalid manifest — fall back to default "replace" - # strategy so layer resolution still works. - pass version = metadata.get("version", "?") if metadata else "?" layers.append({ "path": candidate, From 3d35a88cc2c64d010d76707958c0c4acef792156 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 14:54:29 -0500 Subject: [PATCH 06/16] fix: address second round of Copilot PR review feedback - Fix 'preset resolve' CLI to derive display from _collect_all_layers() instead of resolve_with_source(), showing top layer path and noting when final output is composed from multiple layers - Add _reconcile_composed_commands() for stack-level command reconciliation: after install or remove, recompute effective command content from the full priority stack so command files always reflect the current state - Call reconciliation after preset remove so the next layer takes effect instead of leaving stale/missing command files - Persist registry incrementally: save registered_commands immediately after command registration, then registered_skills after skill registration, so interrupted installs can be cleaned up properly --- src/specify_cli/__init__.py | 18 ++++-- src/specify_cli/presets.py | 108 ++++++++++++++++++++++++++++++++++-- 2 files changed, 114 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index eb20bbc85f..bd63cc5d59 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2596,22 +2596,28 @@ def preset_resolve( raise typer.Exit(1) resolver = PresetResolver(project_root) + layers = resolver._collect_all_layers(template_name) result = resolver.resolve_with_source(template_name) - if result: - console.print(f" [bold]{template_name}[/bold]: {result['path']}") - console.print(f" [dim](from: {result['source']})[/dim]") + if layers: + # Use the highest-priority layer for display because the final output + # may be composed and may not map to resolve_with_source()'s single path. + display_layer = layers[0] + console.print(f" [bold]{template_name}[/bold]: {display_layer['path']}") + console.print(f" [dim](top layer from: {display_layer['source']})[/dim]") - # Show composition chain if any layers use non-replace strategies - layers = resolver._collect_all_layers(template_name) - has_composition = any(layer["strategy"] != "replace" for layer in layers) + has_composition = len(layers) > 1 or any(layer["strategy"] != "replace" for layer in layers) if has_composition: + console.print(" [dim]Final output is composed from multiple preset layers; the path above is the highest-priority contributing layer.[/dim]") console.print("\n [bold]Composition chain:[/bold]") for i, layer in enumerate(reversed(layers)): strategy_label = layer["strategy"] if strategy_label == "replace": strategy_label = "base" console.print(f" {i + 1}. [{strategy_label}] {layer['source']} → {layer['path']}") + elif result: + console.print(f" [bold]{template_name}[/bold]: {result['path']}") + console.print(f" [dim](from: {result['source']})[/dim]") else: console.print(f" [yellow]{template_name}[/yellow]: not found") console.print(" [dim]No template with this name exists in the resolution stack[/dim]") diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 36177ad159..98b4559847 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -600,6 +600,88 @@ def _unregister_commands(self, registered_commands: Dict[str, List[str]]) -> Non registrar = CommandRegistrar() registrar.unregister_commands(registered_commands, self.project_root) + def _reconcile_composed_commands(self, command_names: List[str]) -> None: + """Re-resolve and re-register composed commands from the full stack. + + After install or remove, recompute the effective content for each + command name that participates in composition, and write the winning + content to the agent directories. This ensures command files always + reflect the current priority stack rather than depending on + install/remove order. + + Args: + command_names: List of command names to reconcile + """ + if not command_names: + return + + try: + from .agents import CommandRegistrar + except ImportError: + return + + resolver = PresetResolver(self.project_root) + registrar = CommandRegistrar() + + for cmd_name in command_names: + layers = resolver._collect_all_layers(cmd_name, "command") + if not layers: + continue + + has_composition = any(l["strategy"] != "replace" for l in layers) + if not has_composition: + # Pure replace — the top layer wins. Find which preset owns it + # and re-register from that preset's file. + top_layer = layers[0] + top_path = top_layer["path"] + # Find the preset that owns this layer + for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): + pack_dir = self.presets_dir / pack_id + if str(top_path).startswith(str(pack_dir)): + manifest_path = pack_dir / "preset.yml" + if manifest_path.exists(): + try: + manifest = PresetManifest(manifest_path) + except PresetValidationError: + continue + for tmpl in manifest.templates: + if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": + registrar.register_commands_for_all_agents( + [tmpl], manifest.id, pack_dir, self.project_root + ) + break + break + else: + # Composed command — resolve from full stack + composed = resolver.resolve_content(cmd_name, "command") + if not composed: + continue + + # Write to the highest-priority preset's .composed dir + for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): + pack_dir = self.presets_dir / pack_id + manifest_path = pack_dir / "preset.yml" + if not manifest_path.exists(): + continue + try: + manifest = PresetManifest(manifest_path) + except PresetValidationError: + continue + for tmpl in manifest.templates: + if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": + composed_dir = pack_dir / ".composed" + composed_dir.mkdir(parents=True, exist_ok=True) + composed_file = composed_dir / f"{cmd_name}.md" + composed_file.write_text(composed, encoding="utf-8") + registrar.register_commands_for_all_agents( + [{**tmpl, "file": f".composed/{cmd_name}.md"}], + manifest.id, pack_dir, self.project_root, + ) + break + else: + continue + break + def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. @@ -1032,21 +1114,25 @@ def install_from_directory( }) try: - # Register command overrides with AI agents + # 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) + self.registry.update(manifest.id, { + "registered_commands": registered_commands, + }) # Update corresponding skills when --ai-skills was previously used + # and persist that result as well. registered_skills = self._register_skills(manifest, dest_dir) + self.registry.update(manifest.id, { + "registered_skills": registered_skills, + }) except Exception: # Roll back registry entry on failure self.registry.remove(manifest.id) raise - self.registry.update(manifest.id, { - "registered_commands": registered_commands, - "registered_skills": registered_skills, - }) - return manifest def install_from_zip( @@ -1136,6 +1222,10 @@ def remove(self, pack_id: str) -> bool: } # Unregister non-skill command files from AI agents. + # Collect all command names for post-removal reconciliation. + removed_cmd_names = set() + for cmd_names in registered_commands.values(): + removed_cmd_names.update(cmd_names) if registered_commands: self._unregister_commands(registered_commands) @@ -1143,6 +1233,12 @@ def remove(self, pack_id: str) -> bool: shutil.rmtree(pack_dir) self.registry.remove(pack_id) + + # Reconcile: if other presets still provide these commands, + # re-resolve from the remaining stack so the next layer takes effect. + if removed_cmd_names: + self._reconcile_composed_commands(list(removed_cmd_names)) + return True def list_installed(self) -> List[Dict[str, Any]]: From 3b273e75de09d76985e98e0682db82202b0a3cdf Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:41:40 -0500 Subject: [PATCH 07/16] fix: address third round of Copilot PR review feedback - Fix empty composed content check: use 'is not None' instead of truthiness so intentionally blank overrides aren't skipped - Handle non-preset sources in _reconcile_composed_commands(): when the winning layer after removal is an extension, core, or project override, re-register from that path instead of silently leaving command absent - Fix has_composition in 'preset resolve' CLI: only show composition chain when non-replace strategies are present, not for simple multi- preset replace stacks - Fix PowerShell fallback scan: use if/else structure matching Resolve-Template so directory scan runs when registry is missing - Add test: removing a high-priority preset preserves lower-priority preset's command in the resolution stack --- scripts/powershell/common.ps1 | 65 +++++++++++++++++---------------- src/specify_cli/__init__.py | 2 +- src/specify_cli/presets.py | 51 +++++++++++++++++++++++--- tests/test_presets.py | 69 +++++++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 38 deletions(-) diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index 8347d6a888..dd36f4c176 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -395,15 +395,16 @@ function Resolve-TemplateContent { } } - foreach ($presetId in $sortedPresets) { - # Read strategy and file path from preset manifest - $strategy = 'replace' - $manifestFilePath = '' - $manifest = Join-Path $presetsDir "$presetId/preset.yml" - if (Test-Path $manifest) { - try { - # Use python3 to parse YAML manifest for strategy and file path - $stratResult = & python3 -c @" + if ($sortedPresets.Count -gt 0) { + foreach ($presetId in $sortedPresets) { + # Read strategy and file path from preset manifest + $strategy = 'replace' + $manifestFilePath = '' + $manifest = Join-Path $presetsDir "$presetId/preset.yml" + if (Test-Path $manifest) { + try { + # Use python3 to parse YAML manifest for strategy and file path + $stratResult = & python3 -c @" import yaml, sys try: with open(sys.argv[1]) as f: @@ -416,32 +417,32 @@ try: except Exception: print('replace\t') "@ $manifest $TemplateName 2>$null - if ($stratResult) { - $parts = $stratResult.Trim() -split "`t", 2 - $strategy = $parts[0] - if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] } + if ($stratResult) { + $parts = $stratResult.Trim() -split "`t", 2 + $strategy = $parts[0] + if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] } + } + } catch { + $strategy = 'replace' } - } catch { - $strategy = 'replace' + } + # Try manifest file path first, then convention path + $candidate = $null + if ($manifestFilePath) { + $mf = Join-Path $presetsDir "$presetId/$manifestFilePath" + if (Test-Path $mf) { $candidate = $mf } + } + if (-not $candidate) { + $cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md" + if (Test-Path $cf) { $candidate = $cf } + } + if ($candidate) { + $layerPaths += $candidate + $layerStrategies += $strategy } } - # Try manifest file path first, then convention path - $candidate = $null - if ($manifestFilePath) { - $mf = Join-Path $presetsDir "$presetId/$manifestFilePath" - if (Test-Path $mf) { $candidate = $mf } - } - if (-not $candidate) { - $cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md" - if (Test-Path $cf) { $candidate = $cf } - } - if ($candidate) { - $layerPaths += $candidate - $layerStrategies += $strategy - } - } - - if ($sortedPresets.Count -eq 0) { + } else { + # Fallback: alphabetical directory order (no registry or parse failure) foreach ($preset in Get-ChildItem -Path $presetsDir -Directory -ErrorAction SilentlyContinue | Where-Object { $_.Name -notlike '.*' }) { $candidate = Join-Path $preset.FullName "templates/$TemplateName.md" if (Test-Path $candidate) { diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index bd63cc5d59..3c0b3cac08 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2606,7 +2606,7 @@ def preset_resolve( console.print(f" [bold]{template_name}[/bold]: {display_layer['path']}") console.print(f" [dim](top layer from: {display_layer['source']})[/dim]") - has_composition = len(layers) > 1 or any(layer["strategy"] != "replace" for layer in layers) + has_composition = any(layer["strategy"] != "replace" for layer in layers) if has_composition: console.print(" [dim]Final output is composed from multiple preset layers; the path above is the highest-priority contributing layer.[/dim]") console.print("\n [bold]Composition chain:[/bold]") diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 98b4559847..9490d9b2fb 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -560,7 +560,7 @@ def _register_commands( if strategy != "replace": # Resolve composed content using the full priority stack composed = resolver.resolve_content(cmd["name"], "command") - if composed: + if composed is not None: # Write composed content to a temporary subdirectory if composed_dir is None: composed_dir = preset_dir / ".composed" @@ -630,11 +630,11 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: has_composition = any(l["strategy"] != "replace" for l in layers) if not has_composition: - # Pure replace — the top layer wins. Find which preset owns it - # and re-register from that preset's file. + # Pure replace — the top layer wins. top_layer = layers[0] top_path = top_layer["path"] - # Find the preset that owns this layer + # Try to find which preset owns this layer + registered = False for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): pack_dir = self.presets_dir / pack_id if str(top_path).startswith(str(pack_dir)): @@ -649,15 +649,23 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: registrar.register_commands_for_all_agents( [tmpl], manifest.id, pack_dir, self.project_root ) + registered = True break break + if not registered: + # Top layer is a non-preset source (extension, core, or + # project override). Register directly from the layer path. + self._register_command_from_path( + registrar, cmd_name, top_path + ) else: # Composed command — resolve from full stack composed = resolver.resolve_content(cmd_name, "command") - if not composed: + if composed is None: continue # Write to the highest-priority preset's .composed dir + registered = False for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): pack_dir = self.presets_dir / pack_id manifest_path = pack_dir / "preset.yml" @@ -677,10 +685,43 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: [{**tmpl, "file": f".composed/{cmd_name}.md"}], manifest.id, pack_dir, self.project_root, ) + registered = True break else: continue break + if not registered: + # No preset owns this composed command — write to a + # shared .composed dir and register from the top layer. + shared_composed = self.presets_dir / ".composed" + shared_composed.mkdir(parents=True, exist_ok=True) + composed_file = shared_composed / f"{cmd_name}.md" + composed_file.write_text(composed, encoding="utf-8") + self._register_command_from_path( + registrar, cmd_name, composed_file + ) + + def _register_command_from_path( + self, + registrar: Any, + cmd_name: str, + cmd_path: Path, + ) -> None: + """Register a single command from a file path (non-preset source). + + Used by reconciliation when the winning layer is an extension, + core template, or project override rather than a preset. + """ + if not cmd_path.exists(): + return + cmd_tmpl = { + "name": cmd_name, + "type": "command", + "file": cmd_path.name, + } + registrar.register_commands_for_all_agents( + [cmd_tmpl], "reconciled", cmd_path.parent, self.project_root + ) def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. diff --git a/tests/test_presets.py b/tests/test_presets.py index e08aa35c3d..265d5af2c8 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3564,6 +3564,75 @@ def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_p assert layers[1]["strategy"] == "replace" +class TestRemoveReconciliation: + """Test that removing a preset re-registers the next layer's command.""" + + def test_remove_restores_lower_priority_command( + self, project_dir, temp_dir, valid_pack_data + ): + """After removing the top-priority preset, the next preset's command + should be re-registered in agent directories.""" + manager = PresetManager(project_dir) + + # Install a low-priority preset with a command + lo_data = {**valid_pack_data} + lo_data["preset"] = { + **valid_pack_data["preset"], + "id": "lo-preset", + "name": "Lo", + } + lo_data["provides"] = { + "templates": [{ + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + }] + } + lo_dir = temp_dir / "lo-preset" + lo_dir.mkdir() + with open(lo_dir / "preset.yml", "w") as f: + yaml.dump(lo_data, f) + (lo_dir / "commands").mkdir() + (lo_dir / "commands" / "speckit.specify.md").write_text( + "---\ndescription: lo\n---\nLo content\n" + ) + manager.install_from_directory(lo_dir, "0.1.5", priority=10) + + # Install a high-priority preset overriding the same command + hi_data = {**valid_pack_data} + hi_data["preset"] = { + **valid_pack_data["preset"], + "id": "hi-preset", + "name": "Hi", + } + hi_data["provides"] = { + "templates": [{ + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + }] + } + hi_dir = temp_dir / "hi-preset" + hi_dir.mkdir() + with open(hi_dir / "preset.yml", "w") as f: + yaml.dump(hi_data, f) + (hi_dir / "commands").mkdir() + (hi_dir / "commands" / "speckit.specify.md").write_text( + "---\ndescription: hi\n---\nHi content\n" + ) + manager.install_from_directory(hi_dir, "0.1.5", priority=1) + + # Remove the high-priority preset + manager.remove("hi-preset") + + # The low-priority preset's command should still be present + # in the resolution stack + resolver = PresetResolver(project_dir) + layers = resolver._collect_all_layers("speckit.specify", "command") + assert len(layers) >= 1 + assert "lo-preset" in layers[0]["source"] + + def _create_pack(temp_dir, valid_pack_data, pack_id, content, strategy="replace", template_type="template", template_name="spec-template"): From 9510e7193ad9a0b71c311e4d29a67942e30b3a3a Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:45:45 -0500 Subject: [PATCH 08/16] fix: rename ambiguous variable 'l' to 'layer' (ruff E741) --- src/specify_cli/presets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 9490d9b2fb..2bb7eb05ff 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -628,7 +628,7 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: if not layers: continue - has_composition = any(l["strategy"] != "replace" for l in layers) + has_composition = any(layer["strategy"] != "replace" for layer in layers) if not has_composition: # Pure replace — the top layer wins. top_layer = layers[0] From 3895d0d43197c521ec6e91584a4c5e8dc77bebcc Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:57:30 -0500 Subject: [PATCH 09/16] fix: address fourth round of Copilot PR review feedback - Use Path.is_relative_to() instead of str.startswith() for proper path containment check in _reconcile_composed_commands() - Fix install rollback to undo all side effects: unregister commands and skills, remove copied preset dir before dropping registry entry - Validate wrap placeholder presence: raise PresetValidationError when wrapper content is missing {CORE_TEMPLATE} or $CORE_SCRIPT - Guard python3 availability in PowerShell Resolve-TemplateContent with Get-Command check before invoking - Handle missing PyYAML gracefully in both bash and PowerShell: check 'import yaml' explicitly and fall back to replace/convention path --- scripts/bash/common.sh | 8 +++++++- scripts/powershell/common.ps1 | 9 +++++++-- src/specify_cli/presets.py | 28 ++++++++++++++++++++++++---- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index ce7d536eb0..18d55cb82c 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -419,9 +419,15 @@ except Exception: local manifest_file="" local manifest="$presets_dir/$preset_id/preset.yml" if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then + # Requires PyYAML; falls back to replace/convention if unavailable local result result=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c " -import yaml, sys, os +import sys, os +try: + import yaml +except ImportError: + print('replace\t') + sys.exit(0) try: with open(os.environ['SPECKIT_MANIFEST']) as f: data = yaml.safe_load(f) diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index dd36f4c176..c37f44dd71 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -401,11 +401,16 @@ function Resolve-TemplateContent { $strategy = 'replace' $manifestFilePath = '' $manifest = Join-Path $presetsDir "$presetId/preset.yml" - if (Test-Path $manifest) { + if ((Test-Path $manifest) -and (Get-Command python3 -ErrorAction SilentlyContinue)) { try { # Use python3 to parse YAML manifest for strategy and file path $stratResult = & python3 -c @" -import yaml, sys +import sys +try: + import yaml +except ImportError: + print('replace\t') + sys.exit(0) try: with open(sys.argv[1]) as f: data = yaml.safe_load(f) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 2bb7eb05ff..996aba2275 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -637,7 +637,7 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: registered = False for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): pack_dir = self.presets_dir / pack_id - if str(top_path).startswith(str(pack_dir)): + if top_path.is_relative_to(pack_dir): manifest_path = pack_dir / "preset.yml" if manifest_path.exists(): try: @@ -1170,7 +1170,19 @@ def install_from_directory( "registered_skills": registered_skills, }) except Exception: - # Roll back registry entry on failure + # Roll back all side effects: unregister any commands/skills that + # were written, remove the copied preset dir, and drop the + # registry entry. + metadata = self.registry.get(manifest.id) + if metadata: + rc = metadata.get("registered_commands", {}) + if rc: + self._unregister_commands(rc) + rs = metadata.get("registered_skills", []) + if rs: + self._unregister_skills(rs, dest_dir) + if dest_dir.exists(): + shutil.rmtree(dest_dir) self.registry.remove(manifest.id) raise @@ -2276,8 +2288,16 @@ def resolve_content( content = content + "\n\n" + layer_content elif strategy == "wrap": if template_type == "script": - content = layer_content.replace("$CORE_SCRIPT", content) + placeholder = "$CORE_SCRIPT" else: - content = layer_content.replace("{CORE_TEMPLATE}", content) + placeholder = "{CORE_TEMPLATE}" + if placeholder not in layer_content: + raise PresetValidationError( + f"Wrap strategy in '{layer['source']}' is missing " + f"the {placeholder} placeholder. The wrapper must " + f"contain {placeholder} to indicate where the " + f"lower-priority content should be inserted." + ) + content = layer_content.replace(placeholder, content) return content From a77f7e622a547045cb46ec1328d746ee934e9fb6 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 16:19:04 -0500 Subject: [PATCH 10/16] fix: address fifth round of Copilot PR review feedback - Reconcile all commands from full priority stack after install so install order doesn't determine the winning command file - Fix bash wrap substitution: use split-and-rejoin (%%/# parameter expansion) instead of //pattern/replacement to avoid & expansion corrupting content containing ampersands - Support python/py/py-3 fallbacks in PowerShell Resolve-TemplateContent via Get-Python3Command helper, matching Windows environments where python3 may not be available - Reconciliation skips skill-based agents to avoid overwriting properly formatted SKILL.md files written by _register_skills() --- scripts/bash/common.sh | 14 +++++++++-- scripts/powershell/common.ps1 | 22 +++++++++++++--- src/specify_cli/presets.py | 47 ++++++++++++++++++++++++++++++----- 3 files changed, 72 insertions(+), 11 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index 18d55cb82c..a37f7f9101 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -542,7 +542,13 @@ except Exception: case "$strat" in prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; - wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; + wrap) + # Split on placeholder and rejoin with content to avoid + # bash & expansion in parameter substitution replacement. + local before="${layer_content%%\{CORE_TEMPLATE\}*}" + local after="${layer_content#*\{CORE_TEMPLATE\}}" + content="${before}${content}${after}" + ;; esac fi else @@ -550,7 +556,11 @@ except Exception: replace) content="$layer_content" ;; prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; - wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;; + wrap) + local before="${layer_content%%\{CORE_TEMPLATE\}*}" + local after="${layer_content#*\{CORE_TEMPLATE\}}" + content="${before}${content}${after}" + ;; esac fi done diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index c37f44dd71..550482031e 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -287,6 +287,21 @@ function Test-DirHasFiles { } } +# Find a usable Python 3 executable (python3, python, or py -3). +# Returns the command/arguments as an array, or $null if none found. +function Get-Python3Command { + if (Get-Command python3 -ErrorAction SilentlyContinue) { return @('python3') } + if (Get-Command python -ErrorAction SilentlyContinue) { + $ver = & python --version 2>&1 + if ($ver -match 'Python 3') { return @('python') } + } + if (Get-Command py -ErrorAction SilentlyContinue) { + $ver = & py -3 --version 2>&1 + if ($ver -match 'Python 3') { return @('py', '-3') } + } + return $null +} + # Resolve a template name to a file path using the priority stack: # 1. .specify/templates/overrides/ # 2. .specify/presets//templates/ (sorted by priority from .registry) @@ -401,10 +416,11 @@ function Resolve-TemplateContent { $strategy = 'replace' $manifestFilePath = '' $manifest = Join-Path $presetsDir "$presetId/preset.yml" - if ((Test-Path $manifest) -and (Get-Command python3 -ErrorAction SilentlyContinue)) { + $pyCmd = Get-Python3Command + if ((Test-Path $manifest) -and $pyCmd) { try { - # Use python3 to parse YAML manifest for strategy and file path - $stratResult = & python3 -c @" + # Use Python to parse YAML manifest for strategy and file path + $stratResult = & $pyCmd[0] @($pyCmd[1..99]) -c @" import sys try: import yaml diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 996aba2275..6c411d33e3 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -646,8 +646,8 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: continue for tmpl in manifest.templates: if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": - registrar.register_commands_for_all_agents( - [tmpl], manifest.id, pack_dir, self.project_root + self._register_for_non_skill_agents( + registrar, [tmpl], manifest.id, pack_dir ) registered = True break @@ -681,9 +681,10 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" composed_file.write_text(composed, encoding="utf-8") - registrar.register_commands_for_all_agents( + self._register_for_non_skill_agents( + registrar, [{**tmpl, "file": f".composed/{cmd_name}.md"}], - manifest.id, pack_dir, self.project_root, + manifest.id, pack_dir, ) registered = True break @@ -719,10 +720,36 @@ def _register_command_from_path( "type": "command", "file": cmd_path.name, } - registrar.register_commands_for_all_agents( - [cmd_tmpl], "reconciled", cmd_path.parent, self.project_root + self._register_for_non_skill_agents( + registrar, [cmd_tmpl], "reconciled", cmd_path.parent ) + def _register_for_non_skill_agents( + self, + registrar: Any, + commands: List[Dict[str, Any]], + source_id: str, + source_dir: Path, + ) -> None: + """Register commands for all non-skill agents. + + Used during reconciliation to avoid overwriting properly formatted + SKILL.md files that were written by _register_skills(). + """ + registrar._ensure_configs() + for agent_name, agent_config in registrar.AGENT_CONFIGS.items(): + if agent_config.get("extension") == "/SKILL.md": + continue + agent_dir = self.project_root / agent_config["dir"] + if agent_dir.exists(): + try: + registrar.register_commands( + agent_name, commands, source_id, + source_dir, self.project_root, + ) + except (ValueError, Exception): + continue + def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. @@ -1186,6 +1213,14 @@ def install_from_directory( self.registry.remove(manifest.id) raise + # Reconcile all affected commands from the full priority stack so that + # install order doesn't determine the winning command file. + cmd_names = [ + t["name"] for t in manifest.templates if t.get("type") == "command" + ] + if cmd_names: + self._reconcile_composed_commands(cmd_names) + return manifest def install_from_zip( From 6dddae8e28b0dc23272f1698152be0783afcb8db Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 16:51:39 -0500 Subject: [PATCH 11/16] fix: address sixth round of Copilot PR review feedback - Validate {CORE_TEMPLATE} placeholder in bash wrap strategy: fail with clear error if placeholder is missing in wrapper content - Validate {CORE_TEMPLATE} placeholder in PowerShell wrap strategy: throw if placeholder is missing, matching Python resolver behavior - Fix PowerShell pyCmd arg slicing: build extra-args array based on actual Count to avoid out-of-range $null arguments - Narrow exception catch in _register_for_non_skill_agents(): catch only ValueError instead of broad Exception - Add _reconcile_skills() post-removal: re-register SKILL.md files from the next winning preset so skills stay in sync with the effective command stack after removing a top-priority preset --- scripts/bash/common.sh | 11 +++++++++- scripts/powershell/common.ps1 | 17 +++++++++++++--- src/specify_cli/presets.py | 38 ++++++++++++++++++++++++++++++++++- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index a37f7f9101..a37093a006 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -543,7 +543,12 @@ except Exception: prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; wrap) - # Split on placeholder and rejoin with content to avoid + # Validate placeholder exists + case "$layer_content" in + *'{CORE_TEMPLATE}'*) ;; + *) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;; + esac + # Split on first placeholder and rejoin with content to avoid # bash & expansion in parameter substitution replacement. local before="${layer_content%%\{CORE_TEMPLATE\}*}" local after="${layer_content#*\{CORE_TEMPLATE\}}" @@ -557,6 +562,10 @@ except Exception: prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;; append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;; wrap) + case "$layer_content" in + *'{CORE_TEMPLATE}'*) ;; + *) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;; + esac local before="${layer_content%%\{CORE_TEMPLATE\}*}" local after="${layer_content#*\{CORE_TEMPLATE\}}" content="${before}${content}${after}" diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index 550482031e..bdeda93426 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -420,7 +420,8 @@ function Resolve-TemplateContent { if ((Test-Path $manifest) -and $pyCmd) { try { # Use Python to parse YAML manifest for strategy and file path - $stratResult = & $pyCmd[0] @($pyCmd[1..99]) -c @" + $pyArgs = if ($pyCmd.Count -gt 1) { $pyCmd[1..($pyCmd.Count-1)] } else { @() } + $stratResult = & $pyCmd[0] @pyArgs -c @" import sys try: import yaml @@ -524,7 +525,12 @@ except Exception: switch ($strat) { 'prepend' { $content = "$layerContent`n`n$content" } 'append' { $content = "$content`n`n$layerContent" } - 'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) } + 'wrap' { + if (-not $layerContent.Contains('{CORE_TEMPLATE}')) { + throw "Wrap strategy missing {CORE_TEMPLATE} placeholder" + } + $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) + } } } } else { @@ -532,7 +538,12 @@ except Exception: 'replace' { $content = $layerContent } 'prepend' { $content = "$layerContent`n`n$content" } 'append' { $content = "$content`n`n$layerContent" } - 'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) } + 'wrap' { + if (-not $layerContent.Contains('{CORE_TEMPLATE}')) { + throw "Wrap strategy missing {CORE_TEMPLATE} placeholder" + } + $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) + } } } } diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 6c411d33e3..b923391942 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -747,9 +747,42 @@ def _register_for_non_skill_agents( agent_name, commands, source_id, source_dir, self.project_root, ) - except (ValueError, Exception): + except ValueError: continue + def _reconcile_skills(self, command_names: List[str]) -> None: + """Re-register skills for commands whose winning layer changed. + + After a preset is removed, finds the next preset in the priority + stack that provides each command and re-runs skill registration + for that preset so SKILL.md files reflect the current winner. + + Args: + command_names: List of command names to reconcile skills for + """ + if not command_names: + return + + resolver = PresetResolver(self.project_root) + for cmd_name in command_names: + layers = resolver._collect_all_layers(cmd_name, "command") + if not layers: + continue + + top_path = layers[0]["path"] + # Find the preset that owns the winning layer + for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority(): + pack_dir = self.presets_dir / pack_id + if top_path.is_relative_to(pack_dir): + manifest_path = pack_dir / "preset.yml" + if manifest_path.exists(): + try: + manifest = PresetManifest(manifest_path) + except PresetValidationError: + break + self._register_skills(manifest, pack_dir) + break + def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. @@ -1326,6 +1359,9 @@ def remove(self, pack_id: str) -> bool: # re-resolve from the remaining stack so the next layer takes effect. if removed_cmd_names: self._reconcile_composed_commands(list(removed_cmd_names)) + # Also reconcile skills so SKILL.md files reflect the new winning + # command layer rather than being left absent or stale. + self._reconcile_skills(list(removed_cmd_names)) return True From 7a23eed86b33e234a4fa6cb76b41ac6df45a8e1a Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:07:34 -0500 Subject: [PATCH 12/16] fix: address seventh round of Copilot PR review feedback - Strip frontmatter from command layers during composition: parse each layer into (frontmatter, body), compose bodies only, then reattach the highest-priority frontmatter to avoid leaking YAML metadata - Filter disabled presets in bash resolve_template and resolve_template_content: skip presets with enabled=false - Filter disabled presets in PowerShell Resolve-Template and Resolve-TemplateContent: add Where-Object enabled filter to match Python resolver behavior --- scripts/bash/common.sh | 6 ++++-- scripts/powershell/common.ps1 | 2 ++ src/specify_cli/presets.py | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index a37093a006..c4fcdc9601 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -321,7 +321,8 @@ try: data = json.load(f) presets = data.get('presets', {}) for pid, meta in sorted(presets.items(), key=lambda x: x[1].get('priority', 10)): - print(pid) + if meta.get('enabled', True) is not False: + print(pid) except Exception: sys.exit(1) " 2>/dev/null); then @@ -408,7 +409,8 @@ try: data = json.load(f) presets = data.get('presets', {}) for pid, meta in sorted(presets.items(), key=lambda x: x[1].get('priority', 10)): - print(pid) + if meta.get('enabled', True) is not False: + print(pid) except Exception: sys.exit(1) " 2>/dev/null); then diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index bdeda93426..fdf46b7ebc 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -330,6 +330,7 @@ function Resolve-Template { $presets = $registryData.presets if ($presets) { $sortedPresets = $presets.PSObject.Properties | + Where-Object { $null -eq $_.Value.enabled -or $_.Value.enabled -ne $false } | Sort-Object { if ($null -ne $_.Value.priority) { $_.Value.priority } else { 10 } } | ForEach-Object { $_.Name } } @@ -402,6 +403,7 @@ function Resolve-TemplateContent { $presets = $registryData.presets if ($presets) { $sortedPresets = $presets.PSObject.Properties | + Where-Object { $null -eq $_.Value.enabled -or $_.Value.enabled -ne $false } | Sort-Object { if ($null -ne $_.Value.priority) { $_.Value.priority } else { 10 } } | ForEach-Object { $_.Name } } diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index b923391942..3d54478e96 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -2346,11 +2346,41 @@ def resolve_content( if content is None: return None + # For command composition, strip frontmatter from each layer to avoid + # leaking YAML metadata into the composed body. The highest-priority + # layer's frontmatter will be reattached at the end. + is_command = template_type == "command" + top_frontmatter_text = None + + def _strip_frontmatter(text: str) -> tuple: + """Return (frontmatter_text_with_fences, body) or (None, text).""" + if not text.startswith("---"): + return None, text + end = text.find("---", 3) + if end == -1: + return None, text + fm_block = text[:end + 3] + body = text[end + 3:].strip() + return fm_block, body + + if is_command: + fm, body = _strip_frontmatter(content) + if fm: + top_frontmatter_text = fm + content = body + # Apply composition layers from bottom to top for layer in reversed_layers[start_idx:]: layer_content = layer["path"].read_text(encoding="utf-8") strategy = layer["strategy"] + if is_command: + fm, layer_body = _strip_frontmatter(layer_content) + layer_content = layer_body + # Track the highest-priority frontmatter seen + if fm: + top_frontmatter_text = fm + if strategy == "replace": content = layer_content elif strategy == "prepend": @@ -2371,4 +2401,8 @@ def resolve_content( ) content = layer_content.replace(placeholder, content) + # Reattach the highest-priority frontmatter for commands + if is_command and top_frontmatter_text: + content = top_frontmatter_text + "\n\n" + content + return content From 3906ea6f9f2346d926e1ba7ec811ebc813a63c34 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:22:36 -0500 Subject: [PATCH 13/16] fix: address eighth round of Copilot PR review feedback - Hoist Get-Python3Command call out of preset loop in PowerShell Resolve-TemplateContent to avoid repeated Get-Command/version checks - Short-circuit composition when top layer is replace: in resolve_content, _reconcile_composed_commands, and bash resolve_template_content, return the top layer directly when its strategy is replace, ignoring irrelevant lower-priority composition layers - Add test: replace-over-wrap verifies that a high-priority replace preset wins without evaluating a lower-priority wrap that lacks a placeholder --- scripts/bash/common.sh | 7 ++++++ scripts/powershell/common.ps1 | 2 +- src/specify_cli/presets.py | 12 ++++++++- tests/test_presets.py | 47 +++++++++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index c4fcdc9601..c9232f29e6 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -515,6 +515,13 @@ except Exception: [ "$s" != "replace" ] && has_composition=true && break done + # If the top (highest-priority) layer is replace, it wins entirely — + # lower layers are irrelevant regardless of their strategies. + if [ "${layer_strategies[0]}" = "replace" ]; then + cat "${layer_paths[0]}" + return 0 + fi + if [ "$has_composition" = false ]; then cat "${layer_paths[0]}" return 0 diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index fdf46b7ebc..de4eb9e927 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -413,12 +413,12 @@ function Resolve-TemplateContent { } if ($sortedPresets.Count -gt 0) { + $pyCmd = Get-Python3Command foreach ($presetId in $sortedPresets) { # Read strategy and file path from preset manifest $strategy = 'replace' $manifestFilePath = '' $manifest = Join-Path $presetsDir "$presetId/preset.yml" - $pyCmd = Get-Python3Command if ((Test-Path $manifest) -and $pyCmd) { try { # Use Python to parse YAML manifest for strategy and file path diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 3d54478e96..446e29d2b0 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -628,7 +628,12 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: if not layers: continue - has_composition = any(layer["strategy"] != "replace" for layer in layers) + # If the top layer is replace, it wins entirely — lower layers + # are irrelevant regardless of their strategies. + top_is_replace = layers[0]["strategy"] == "replace" + has_composition = not top_is_replace and any( + layer["strategy"] != "replace" for layer in layers + ) if not has_composition: # Pure replace — the top layer wins. top_layer = layers[0] @@ -2317,6 +2322,11 @@ def resolve_content( if not layers: return None + # If the top (highest-priority) layer is replace, it wins entirely — + # lower layers are irrelevant regardless of their strategies. + if layers[0]["strategy"] == "replace": + return layers[0]["path"].read_text(encoding="utf-8") + # Check if any layer uses a non-replace strategy has_composition = any(layer["strategy"] != "replace" for layer in layers) diff --git a/tests/test_presets.py b/tests/test_presets.py index 265d5af2c8..60b46ce59a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3487,6 +3487,53 @@ def test_resolve_content_blank_line_separator(self, project_dir, temp_dir, valid # Should have blank line separator assert "\n\n" in content + def test_resolve_content_replace_over_wrap(self, project_dir, temp_dir, valid_pack_data): + """Top-priority replace layer should win even if a lower layer uses wrap.""" + # Install a low-priority wrap preset (with no placeholder — would fail if evaluated) + wrap_data = {**valid_pack_data} + wrap_data["preset"] = {**valid_pack_data["preset"], "id": "wrap-lo", "name": "WrapLo"} + wrap_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + "strategy": "wrap", + }] + } + wrap_dir = temp_dir / "wrap-lo" + wrap_dir.mkdir() + with open(wrap_dir / "preset.yml", "w") as f: + yaml.dump(wrap_data, f) + (wrap_dir / "templates").mkdir() + # Intentionally missing {CORE_TEMPLATE} — would error if composition ran + (wrap_dir / "templates" / "spec-template.md").write_text("wrapper without placeholder") + + manager = PresetManager(project_dir) + manager.install_from_directory(wrap_dir, "0.1.5", priority=10) + + # Install a high-priority replace preset + rep_data = {**valid_pack_data} + rep_data["preset"] = {**valid_pack_data["preset"], "id": "rep-hi", "name": "RepHi"} + rep_data["provides"] = { + "templates": [{ + "type": "template", + "name": "spec-template", + "file": "templates/spec-template.md", + }] + } + rep_dir = temp_dir / "rep-hi" + rep_dir.mkdir() + with open(rep_dir / "preset.yml", "w") as f: + yaml.dump(rep_data, f) + (rep_dir / "templates").mkdir() + (rep_dir / "templates" / "spec-template.md").write_text("# Replaced content\n") + + manager.install_from_directory(rep_dir, "0.1.5", priority=1) + + resolver = PresetResolver(project_dir) + content = resolver.resolve_content("spec-template") + assert content == "# Replaced content\n" + class TestCollectAllLayers: """Test PresetResolver._collect_all_layers() method.""" From 506d9c37bb08b66d91af02df40e0f14ab71c613e Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:40:24 -0500 Subject: [PATCH 14/16] fix: address ninth round of Copilot PR review feedback - Add top-replace short-circuit in PowerShell Resolve-TemplateContent: return top layer directly when its strategy is replace, matching Python and bash resolver behavior - Reconcile skills after install: call _reconcile_skills() for affected command names so SKILL.md files reflect the actual priority winner - Promote _collect_all_layers to public collect_all_layers: the method is used by the CLI (preset resolve command), so it should be a public API rather than a private helper --- scripts/powershell/common.ps1 | 6 ++++++ src/specify_cli/__init__.py | 2 +- src/specify_cli/presets.py | 11 +++++++---- tests/test_presets.py | 10 +++++----- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/scripts/powershell/common.ps1 b/scripts/powershell/common.ps1 index de4eb9e927..bd07c3506d 100644 --- a/scripts/powershell/common.ps1 +++ b/scripts/powershell/common.ps1 @@ -498,6 +498,12 @@ except Exception: if ($layerPaths.Count -eq 0) { return $null } + # If the top (highest-priority) layer is replace, it wins entirely — + # lower layers are irrelevant regardless of their strategies. + if ($layerStrategies[0] -eq 'replace') { + return (Get-Content $layerPaths[0] -Raw) + } + # Check if any layer uses a non-replace strategy $hasComposition = $false foreach ($s in $layerStrategies) { diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 3c0b3cac08..3f92181f44 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2596,7 +2596,7 @@ def preset_resolve( raise typer.Exit(1) resolver = PresetResolver(project_root) - layers = resolver._collect_all_layers(template_name) + layers = resolver.collect_all_layers(template_name) result = resolver.resolve_with_source(template_name) if layers: diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 446e29d2b0..daacd4021d 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -624,7 +624,7 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: registrar = CommandRegistrar() for cmd_name in command_names: - layers = resolver._collect_all_layers(cmd_name, "command") + layers = resolver.collect_all_layers(cmd_name, "command") if not layers: continue @@ -770,7 +770,7 @@ def _reconcile_skills(self, command_names: List[str]) -> None: resolver = PresetResolver(self.project_root) for cmd_name in command_names: - layers = resolver._collect_all_layers(cmd_name, "command") + layers = resolver.collect_all_layers(cmd_name, "command") if not layers: continue @@ -1258,6 +1258,9 @@ def install_from_directory( ] if cmd_names: self._reconcile_composed_commands(cmd_names) + # Also reconcile skills so SKILL.md files reflect the actual + # winning command layer, not just the last-installed preset. + self._reconcile_skills(cmd_names) return manifest @@ -2162,7 +2165,7 @@ def resolve_with_source( return {"path": resolved_str, "source": "core"} - def _collect_all_layers( + def collect_all_layers( self, template_name: str, template_type: str = "template", @@ -2318,7 +2321,7 @@ def resolve_content( Returns: Composed content string, or None if not found """ - layers = self._collect_all_layers(template_name, template_type) + layers = self.collect_all_layers(template_name, template_type) if not layers: return None diff --git a/tests/test_presets.py b/tests/test_presets.py index 60b46ce59a..ca1335626a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3541,7 +3541,7 @@ class TestCollectAllLayers: def test_single_core_layer(self, project_dir): """Test collecting layers with only core template.""" resolver = PresetResolver(project_dir) - layers = resolver._collect_all_layers("spec-template") + layers = resolver.collect_all_layers("spec-template") assert len(layers) == 1 assert layers[0]["source"] == "core" assert layers[0]["strategy"] == "replace" @@ -3554,7 +3554,7 @@ def test_layers_include_presets(self, project_dir, temp_dir, valid_pack_data): manager.install_from_directory(pack_dir, "0.1.5") resolver = PresetResolver(project_dir) - layers = resolver._collect_all_layers("spec-template") + layers = resolver.collect_all_layers("spec-template") assert len(layers) == 2 # Highest priority first assert "test-pack" in layers[0]["source"] @@ -3575,7 +3575,7 @@ def test_layers_order_matches_priority(self, project_dir, temp_dir, valid_pack_d manager.install_from_directory(p, "0.1.5", priority=prio) resolver = PresetResolver(project_dir) - layers = resolver._collect_all_layers("spec-template") + layers = resolver.collect_all_layers("spec-template") assert len(layers) == 3 # pack-hi, pack-lo, core assert "pack-hi" in layers[0]["source"] assert "pack-lo" in layers[1]["source"] @@ -3604,7 +3604,7 @@ def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_p manager.install_from_directory(pack_dir, "0.1.5") resolver = PresetResolver(project_dir) - layers = resolver._collect_all_layers("spec-template") + layers = resolver.collect_all_layers("spec-template") # Preset layer should have strategy=append assert layers[0]["strategy"] == "append" # Core layer should be replace @@ -3675,7 +3675,7 @@ def test_remove_restores_lower_priority_command( # The low-priority preset's command should still be present # in the resolution stack resolver = PresetResolver(project_dir) - layers = resolver._collect_all_layers("speckit.specify", "command") + layers = resolver.collect_all_layers("speckit.specify", "command") assert len(layers) >= 1 assert "lo-preset" in layers[0]["source"] From 0055e7a67bddaaa49d3b879a20bc5230b8f7adf9 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 17:55:24 -0500 Subject: [PATCH 15/16] fix: pass real source attribution in _register_command_from_path Instead of hardcoding source_id='reconciled', pass through the actual layer source (e.g., 'core', 'project override', 'extension:') from collect_all_layers() so rendered command files show accurate provenance. --- src/specify_cli/presets.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index daacd4021d..f014efde3a 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -661,7 +661,8 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: # Top layer is a non-preset source (extension, core, or # project override). Register directly from the layer path. self._register_command_from_path( - registrar, cmd_name, top_path + registrar, cmd_name, top_path, + source_id=layers[0]["source"], ) else: # Composed command — resolve from full stack @@ -704,7 +705,8 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None: composed_file = shared_composed / f"{cmd_name}.md" composed_file.write_text(composed, encoding="utf-8") self._register_command_from_path( - registrar, cmd_name, composed_file + registrar, cmd_name, composed_file, + source_id=layers[0]["source"], ) def _register_command_from_path( @@ -712,11 +714,18 @@ def _register_command_from_path( registrar: Any, cmd_name: str, cmd_path: Path, + source_id: str = "reconciled", ) -> None: """Register a single command from a file path (non-preset source). Used by reconciliation when the winning layer is an extension, core template, or project override rather than a preset. + + Args: + registrar: CommandRegistrar instance + cmd_name: Command name + cmd_path: Path to the command file + source_id: Source attribution for rendered output """ if not cmd_path.exists(): return @@ -726,7 +735,7 @@ def _register_command_from_path( "file": cmd_path.name, } self._register_for_non_skill_agents( - registrar, [cmd_tmpl], "reconciled", cmd_path.parent + registrar, [cmd_tmpl], source_id, cmd_path.parent ) def _register_for_non_skill_agents( From 454e6146f75d11fc8cf6c6e6c7952c4601ea1276 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 20 Apr 2026 18:22:23 -0500 Subject: [PATCH 16/16] fix: address tenth round of Copilot PR review feedback - Fix bash wrap to replace all {CORE_TEMPLATE} occurrences using a while loop, matching Python str.replace and PowerShell .Replace behavior - Fail fast when composition has no base: raise PresetValidationError instead of silently degrading to implicit replace when a non-replace strategy command has no lower-priority base to compose onto - Strengthen remove reconciliation test: create a gemini commands dir and verify on-disk command file content switches from hi to lo after removing the top-priority preset --- scripts/bash/common.sh | 21 +++++++++++++-------- src/specify_cli/presets.py | 8 +++++++- tests/test_presets.py | 17 +++++++++++++++-- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index c9232f29e6..6bc24f0e4b 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -557,11 +557,13 @@ except Exception: *'{CORE_TEMPLATE}'*) ;; *) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;; esac - # Split on first placeholder and rejoin with content to avoid - # bash & expansion in parameter substitution replacement. - local before="${layer_content%%\{CORE_TEMPLATE\}*}" - local after="${layer_content#*\{CORE_TEMPLATE\}}" - content="${before}${content}${after}" + # Replace all occurrences to match Python/PowerShell behavior + while [[ "$layer_content" == *'{CORE_TEMPLATE}'* ]]; do + local before="${layer_content%%\{CORE_TEMPLATE\}*}" + local after="${layer_content#*\{CORE_TEMPLATE\}}" + layer_content="${before}${content}${after}" + done + content="$layer_content" ;; esac fi @@ -575,9 +577,12 @@ except Exception: *'{CORE_TEMPLATE}'*) ;; *) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;; esac - local before="${layer_content%%\{CORE_TEMPLATE\}*}" - local after="${layer_content#*\{CORE_TEMPLATE\}}" - content="${before}${content}${after}" + while [[ "$layer_content" == *'{CORE_TEMPLATE}'* ]]; do + local before="${layer_content%%\{CORE_TEMPLATE\}*}" + local after="${layer_content#*\{CORE_TEMPLATE\}}" + layer_content="${before}${content}${after}" + done + content="$layer_content" ;; esac fi diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index f014efde3a..91d528ce2e 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -572,7 +572,13 @@ def _register_commands( "file": f".composed/{cmd['name']}.md", }) else: - commands_to_register.append(cmd) + raise PresetValidationError( + f"Command '{cmd['name']}' uses '{strategy}' strategy " + f"but no base command layer exists to compose onto. " + f"Ensure a lower-priority preset, extension, or core " + f"command provides this command before using " + f"composition strategies." + ) else: commands_to_register.append(cmd) diff --git a/tests/test_presets.py b/tests/test_presets.py index ca1335626a..5734ee9025 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3621,6 +3621,10 @@ def test_remove_restores_lower_priority_command( should be re-registered in agent directories.""" manager = PresetManager(project_dir) + # Create a gemini commands dir so reconciliation writes there + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + # Install a low-priority preset with a command lo_data = {**valid_pack_data} lo_data["preset"] = { @@ -3669,16 +3673,25 @@ def test_remove_restores_lower_priority_command( ) manager.install_from_directory(hi_dir, "0.1.5", priority=1) + # Verify the hi-preset's content is active in agent dir + cmd_files = list(gemini_dir.glob("*specify*")) + assert cmd_files, "Command file should exist in gemini dir" + assert "Hi content" in cmd_files[0].read_text() + # Remove the high-priority preset manager.remove("hi-preset") - # The low-priority preset's command should still be present - # in the resolution stack + # The low-priority preset's command should now be in the resolution stack resolver = PresetResolver(project_dir) layers = resolver.collect_all_layers("speckit.specify", "command") assert len(layers) >= 1 assert "lo-preset" in layers[0]["source"] + # Verify on-disk agent command file switched to lo-preset content + 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 _create_pack(temp_dir, valid_pack_data, pack_id, content, strategy="replace", template_type="template",