Skip to content

Commit 18346df

Browse files
mnriemmnriem
committed
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)
1 parent fa04828 commit 18346df

File tree

5 files changed

+135
-57
lines changed

5 files changed

+135
-57
lines changed

‎presets/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ Presets **override by default**, they don't merge. If two presets both provide `
6565

6666
### Composition Strategies
6767

68-
Presets can declare a `strategy` per template to control how content is combined:
68+
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/<name>.md`):
6969

7070
```yaml
7171
provides:

‎presets/scaffold/preset.yml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ provides:
5454
# replaces: "plan-template"
5555

5656
# COMPOSITION EXAMPLES:
57+
# The `file` field points to the content file (can differ from the
58+
# convention path `templates/<name>.md`). The `name` field identifies
59+
# which template to compose with in the priority stack.
60+
#
5761
# Append additional sections to an existing template:
5862
# - type: "template"
5963
# name: "spec-template"

‎scripts/bash/common.sh‎

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -414,33 +414,66 @@ except Exception:
414414
" 2>/dev/null); then
415415
if [ -n "$sorted_presets" ]; then
416416
while IFS= read -r preset_id; do
417-
local candidate="$presets_dir/$preset_id/templates/${template_name}.md"
418-
if [ -f "$candidate" ]; then
419-
# Read strategy from preset manifest
420-
local strategy="replace"
421-
local manifest="$presets_dir/$preset_id/preset.yml"
422-
if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then
423-
local s
424-
s=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c "
417+
# Read strategy and file path from preset manifest
418+
local strategy="replace"
419+
local manifest_file=""
420+
local manifest="$presets_dir/$preset_id/preset.yml"
421+
if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then
422+
local result
423+
result=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c "
425424
import yaml, sys, os
426425
try:
427426
with open(os.environ['SPECKIT_MANIFEST']) as f:
428427
data = yaml.safe_load(f)
429428
for t in data.get('provides', {}).get('templates', []):
430429
if t.get('name') == os.environ['SPECKIT_TMPL'] and t.get('type', 'template') == 'template':
431-
print(t.get('strategy', 'replace'))
430+
print(t.get('strategy', 'replace') + '\t' + t.get('file', ''))
432431
sys.exit(0)
433-
print('replace')
432+
print('replace\t')
434433
except Exception:
435-
print('replace')
436-
" 2>/dev/null) && strategy="$s"
437-
fi
434+
print('replace\t')
435+
" 2>/dev/null) && {
436+
strategy="${result%% *}"
437+
manifest_file="${result#* }"
438+
}
439+
fi
440+
# Try manifest file path first, then convention path
441+
local candidate=""
442+
if [ -n "$manifest_file" ]; then
443+
local mf="$presets_dir/$preset_id/$manifest_file"
444+
[ -f "$mf" ] && candidate="$mf"
445+
fi
446+
if [ -z "$candidate" ]; then
447+
local cf="$presets_dir/$preset_id/templates/${template_name}.md"
448+
[ -f "$cf" ] && candidate="$cf"
449+
fi
450+
if [ -n "$candidate" ]; then
438451
layer_paths+=("$candidate")
439452
layer_strategies+=("$strategy")
440453
fi
441454
done <<< "$sorted_presets"
442455
fi
456+
else
457+
# python3 failed — fall back to unordered directory scan (replace only)
458+
for preset in "$presets_dir"/*/; do
459+
[ -d "$preset" ] || continue
460+
local candidate="$preset/templates/${template_name}.md"
461+
if [ -f "$candidate" ]; then
462+
layer_paths+=("$candidate")
463+
layer_strategies+=("replace")
464+
fi
465+
done
443466
fi
467+
else
468+
# No python3 or registry — fall back to unordered directory scan (replace only)
469+
for preset in "$presets_dir"/*/; do
470+
[ -d "$preset" ] || continue
471+
local candidate="$preset/templates/${template_name}.md"
472+
if [ -f "$candidate" ]; then
473+
layer_paths+=("$candidate")
474+
layer_strategies+=("replace")
475+
fi
476+
done
444477
fi
445478
fi
446479

@@ -501,16 +534,16 @@ except Exception:
501534
[ "$has_base" = false ] && return 1
502535
started=true
503536
case "$strat" in
504-
prepend) content="${layer_content}\n\n${content}" ;;
505-
append) content="${content}\n\n${layer_content}" ;;
537+
prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;;
538+
append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;;
506539
wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;;
507540
esac
508541
fi
509542
else
510543
case "$strat" in
511544
replace) content="$layer_content" ;;
512-
prepend) content="${layer_content}\n\n${content}" ;;
513-
append) content="${content}\n\n${layer_content}" ;;
545+
prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;;
546+
append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;;
514547
wrap) content="${layer_content//\{CORE_TEMPLATE\}/$content}" ;;
515548
esac
516549
fi

‎scripts/powershell/common.ps1‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -396,32 +396,46 @@ function Resolve-TemplateContent {
396396
}
397397

398398
foreach ($presetId in $sortedPresets) {
399-
$candidate = Join-Path $presetsDir "$presetId/templates/$TemplateName.md"
400-
if (Test-Path $candidate) {
401-
# Read strategy from preset manifest
402-
$strategy = 'replace'
403-
$manifest = Join-Path $presetsDir "$presetId/preset.yml"
404-
if (Test-Path $manifest) {
405-
try {
406-
# Use python3 to parse YAML manifest for strategy
407-
$stratResult = & python3 -c @"
399+
# Read strategy and file path from preset manifest
400+
$strategy = 'replace'
401+
$manifestFilePath = ''
402+
$manifest = Join-Path $presetsDir "$presetId/preset.yml"
403+
if (Test-Path $manifest) {
404+
try {
405+
# Use python3 to parse YAML manifest for strategy and file path
406+
$stratResult = & python3 -c @"
408407
import yaml, sys
409408
try:
410409
with open(sys.argv[1]) as f:
411410
data = yaml.safe_load(f)
412411
for t in data.get('provides', {}).get('templates', []):
413412
if t.get('name') == sys.argv[2] and t.get('type', 'template') == 'template':
414-
print(t.get('strategy', 'replace'))
413+
print(t.get('strategy', 'replace') + '\t' + t.get('file', ''))
415414
sys.exit(0)
416-
print('replace')
415+
print('replace\t')
417416
except Exception:
418-
print('replace')
417+
print('replace\t')
419418
"@ $manifest $TemplateName 2>$null
420-
if ($stratResult) { $strategy = $stratResult.Trim() }
421-
} catch {
422-
$strategy = 'replace'
419+
if ($stratResult) {
420+
$parts = $stratResult.Trim() -split "`t", 2
421+
$strategy = $parts[0]
422+
if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] }
423423
}
424+
} catch {
425+
$strategy = 'replace'
424426
}
427+
}
428+
# Try manifest file path first, then convention path
429+
$candidate = $null
430+
if ($manifestFilePath) {
431+
$mf = Join-Path $presetsDir "$presetId/$manifestFilePath"
432+
if (Test-Path $mf) { $candidate = $mf }
433+
}
434+
if (-not $candidate) {
435+
$cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md"
436+
if (Test-Path $cf) { $candidate = $cf }
437+
}
438+
if ($candidate) {
425439
$layerPaths += $candidate
426440
$layerStrategies += $strategy
427441
}
@@ -488,15 +502,15 @@ except Exception:
488502
switch ($strat) {
489503
'prepend' { $content = "$layerContent`n`n$content" }
490504
'append' { $content = "$content`n`n$layerContent" }
491-
'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content }
505+
'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) }
492506
}
493507
}
494508
} else {
495509
switch ($strat) {
496510
'replace' { $content = $layerContent }
497511
'prepend' { $content = "$layerContent`n`n$content" }
498512
'append' { $content = "$content`n`n$layerContent" }
499-
'wrap' { $content = $layerContent -replace [regex]::Escape('{CORE_TEMPLATE}'), $content }
513+
'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) }
500514
}
501515
}
502516
}

‎src/specify_cli/presets.py‎

Lines changed: 49 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,12 @@ def _register_skills(
779779
if not source_file.exists():
780780
continue
781781

782+
# Use composed content if available (written by _register_commands
783+
# for commands with non-replace strategies), otherwise the original.
784+
composed_file = preset_dir / ".composed" / f"{cmd_name}.md"
785+
if composed_file.exists():
786+
source_file = composed_file
787+
782788
# Derive the short command name (e.g. "specify" from "speckit.specify")
783789
raw_short_name = cmd_name
784790
if raw_short_name.startswith("speckit."):
@@ -1013,18 +1019,30 @@ def install_from_directory(
10131019

10141020
shutil.copytree(source_dir, dest_dir)
10151021

1016-
# Register command overrides with AI agents
1017-
registered_commands = self._register_commands(manifest, dest_dir)
1018-
1019-
# Update corresponding skills when --ai-skills was previously used
1020-
registered_skills = self._register_skills(manifest, dest_dir)
1021-
1022+
# Pre-register the preset so that composition resolution can see it
1023+
# in the priority stack when resolving composed command content.
10221024
self.registry.add(manifest.id, {
10231025
"version": manifest.version,
10241026
"source": "local",
10251027
"manifest_hash": manifest.get_hash(),
10261028
"enabled": True,
10271029
"priority": priority,
1030+
"registered_commands": {},
1031+
"registered_skills": [],
1032+
})
1033+
1034+
try:
1035+
# Register command overrides with AI agents
1036+
registered_commands = self._register_commands(manifest, dest_dir)
1037+
1038+
# Update corresponding skills when --ai-skills was previously used
1039+
registered_skills = self._register_skills(manifest, dest_dir)
1040+
except Exception:
1041+
# Roll back registry entry on failure
1042+
self.registry.remove(manifest.id)
1043+
raise
1044+
1045+
self.registry.update(manifest.id, {
10281046
"registered_commands": registered_commands,
10291047
"registered_skills": registered_skills,
10301048
})
@@ -1978,23 +1996,32 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
19781996
registry = PresetRegistry(self.presets_dir)
19791997
for pack_id, metadata in registry.list_by_priority():
19801998
pack_dir = self.presets_dir / pack_id
1981-
candidate = _find_in_subdirs(pack_dir)
1999+
# Read strategy and manifest file path from preset manifest
2000+
strategy = "replace"
2001+
manifest_file_path = None
2002+
manifest_path = pack_dir / "preset.yml"
2003+
if manifest_path.exists():
2004+
try:
2005+
manifest = PresetManifest(manifest_path)
2006+
for tmpl in manifest.templates:
2007+
if (tmpl.get("name") == template_name
2008+
and tmpl.get("type") == template_type):
2009+
strategy = tmpl.get("strategy", "replace")
2010+
manifest_file_path = tmpl.get("file")
2011+
break
2012+
except PresetValidationError:
2013+
# Invalid manifest — fall back to default "replace"
2014+
# strategy so layer resolution still works.
2015+
pass
2016+
# Use manifest file path if specified, otherwise convention-based lookup
2017+
candidate = None
2018+
if manifest_file_path:
2019+
manifest_candidate = pack_dir / manifest_file_path
2020+
if manifest_candidate.exists():
2021+
candidate = manifest_candidate
2022+
if candidate is None:
2023+
candidate = _find_in_subdirs(pack_dir)
19822024
if candidate:
1983-
# Read strategy from preset manifest
1984-
strategy = "replace"
1985-
manifest_path = pack_dir / "preset.yml"
1986-
if manifest_path.exists():
1987-
try:
1988-
manifest = PresetManifest(manifest_path)
1989-
for tmpl in manifest.templates:
1990-
if (tmpl.get("name") == template_name
1991-
and tmpl.get("type") == template_type):
1992-
strategy = tmpl.get("strategy", "replace")
1993-
break
1994-
except PresetValidationError:
1995-
# Invalid manifest — fall back to default "replace"
1996-
# strategy so layer resolution still works.
1997-
pass
19982025
version = metadata.get("version", "?") if metadata else "?"
19992026
layers.append({
20002027
"path": candidate,

0 commit comments

Comments
 (0)