Skip to content

Commit 3b273e7

Browse files
committed
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
1 parent 3d35a88 commit 3b273e7

File tree

4 files changed

+149
-38
lines changed

4 files changed

+149
-38
lines changed

scripts/powershell/common.ps1

Lines changed: 33 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -395,15 +395,16 @@ function Resolve-TemplateContent {
395395
}
396396
}
397397

398-
foreach ($presetId in $sortedPresets) {
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 @"
398+
if ($sortedPresets.Count -gt 0) {
399+
foreach ($presetId in $sortedPresets) {
400+
# Read strategy and file path from preset manifest
401+
$strategy = 'replace'
402+
$manifestFilePath = ''
403+
$manifest = Join-Path $presetsDir "$presetId/preset.yml"
404+
if (Test-Path $manifest) {
405+
try {
406+
# Use python3 to parse YAML manifest for strategy and file path
407+
$stratResult = & python3 -c @"
407408
import yaml, sys
408409
try:
409410
with open(sys.argv[1]) as f:
@@ -416,32 +417,32 @@ try:
416417
except Exception:
417418
print('replace\t')
418419
"@ $manifest $TemplateName 2>$null
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] }
420+
if ($stratResult) {
421+
$parts = $stratResult.Trim() -split "`t", 2
422+
$strategy = $parts[0]
423+
if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] }
424+
}
425+
} catch {
426+
$strategy = 'replace'
423427
}
424-
} catch {
425-
$strategy = 'replace'
428+
}
429+
# Try manifest file path first, then convention path
430+
$candidate = $null
431+
if ($manifestFilePath) {
432+
$mf = Join-Path $presetsDir "$presetId/$manifestFilePath"
433+
if (Test-Path $mf) { $candidate = $mf }
434+
}
435+
if (-not $candidate) {
436+
$cf = Join-Path $presetsDir "$presetId/templates/$TemplateName.md"
437+
if (Test-Path $cf) { $candidate = $cf }
438+
}
439+
if ($candidate) {
440+
$layerPaths += $candidate
441+
$layerStrategies += $strategy
426442
}
427443
}
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) {
439-
$layerPaths += $candidate
440-
$layerStrategies += $strategy
441-
}
442-
}
443-
444-
if ($sortedPresets.Count -eq 0) {
444+
} else {
445+
# Fallback: alphabetical directory order (no registry or parse failure)
445446
foreach ($preset in Get-ChildItem -Path $presetsDir -Directory -ErrorAction SilentlyContinue | Where-Object { $_.Name -notlike '.*' }) {
446447
$candidate = Join-Path $preset.FullName "templates/$TemplateName.md"
447448
if (Test-Path $candidate) {

src/specify_cli/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2606,7 +2606,7 @@ def preset_resolve(
26062606
console.print(f" [bold]{template_name}[/bold]: {display_layer['path']}")
26072607
console.print(f" [dim](top layer from: {display_layer['source']})[/dim]")
26082608

2609-
has_composition = len(layers) > 1 or any(layer["strategy"] != "replace" for layer in layers)
2609+
has_composition = any(layer["strategy"] != "replace" for layer in layers)
26102610
if has_composition:
26112611
console.print(" [dim]Final output is composed from multiple preset layers; the path above is the highest-priority contributing layer.[/dim]")
26122612
console.print("\n [bold]Composition chain:[/bold]")

src/specify_cli/presets.py

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -560,7 +560,7 @@ def _register_commands(
560560
if strategy != "replace":
561561
# Resolve composed content using the full priority stack
562562
composed = resolver.resolve_content(cmd["name"], "command")
563-
if composed:
563+
if composed is not None:
564564
# Write composed content to a temporary subdirectory
565565
if composed_dir is None:
566566
composed_dir = preset_dir / ".composed"
@@ -630,11 +630,11 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
630630

631631
has_composition = any(l["strategy"] != "replace" for l in layers)
632632
if not has_composition:
633-
# Pure replace — the top layer wins. Find which preset owns it
634-
# and re-register from that preset's file.
633+
# Pure replace — the top layer wins.
635634
top_layer = layers[0]
636635
top_path = top_layer["path"]
637-
# Find the preset that owns this layer
636+
# Try to find which preset owns this layer
637+
registered = False
638638
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
639639
pack_dir = self.presets_dir / pack_id
640640
if str(top_path).startswith(str(pack_dir)):
@@ -649,15 +649,23 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
649649
registrar.register_commands_for_all_agents(
650650
[tmpl], manifest.id, pack_dir, self.project_root
651651
)
652+
registered = True
652653
break
653654
break
655+
if not registered:
656+
# Top layer is a non-preset source (extension, core, or
657+
# project override). Register directly from the layer path.
658+
self._register_command_from_path(
659+
registrar, cmd_name, top_path
660+
)
654661
else:
655662
# Composed command — resolve from full stack
656663
composed = resolver.resolve_content(cmd_name, "command")
657-
if not composed:
664+
if composed is None:
658665
continue
659666

660667
# Write to the highest-priority preset's .composed dir
668+
registered = False
661669
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
662670
pack_dir = self.presets_dir / pack_id
663671
manifest_path = pack_dir / "preset.yml"
@@ -677,10 +685,43 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
677685
[{**tmpl, "file": f".composed/{cmd_name}.md"}],
678686
manifest.id, pack_dir, self.project_root,
679687
)
688+
registered = True
680689
break
681690
else:
682691
continue
683692
break
693+
if not registered:
694+
# No preset owns this composed command — write to a
695+
# shared .composed dir and register from the top layer.
696+
shared_composed = self.presets_dir / ".composed"
697+
shared_composed.mkdir(parents=True, exist_ok=True)
698+
composed_file = shared_composed / f"{cmd_name}.md"
699+
composed_file.write_text(composed, encoding="utf-8")
700+
self._register_command_from_path(
701+
registrar, cmd_name, composed_file
702+
)
703+
704+
def _register_command_from_path(
705+
self,
706+
registrar: Any,
707+
cmd_name: str,
708+
cmd_path: Path,
709+
) -> None:
710+
"""Register a single command from a file path (non-preset source).
711+
712+
Used by reconciliation when the winning layer is an extension,
713+
core template, or project override rather than a preset.
714+
"""
715+
if not cmd_path.exists():
716+
return
717+
cmd_tmpl = {
718+
"name": cmd_name,
719+
"type": "command",
720+
"file": cmd_path.name,
721+
}
722+
registrar.register_commands_for_all_agents(
723+
[cmd_tmpl], "reconciled", cmd_path.parent, self.project_root
724+
)
684725

685726
def _get_skills_dir(self) -> Optional[Path]:
686727
"""Return the active skills directory for preset skill overrides.

tests/test_presets.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3564,6 +3564,75 @@ def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_p
35643564
assert layers[1]["strategy"] == "replace"
35653565

35663566

3567+
class TestRemoveReconciliation:
3568+
"""Test that removing a preset re-registers the next layer's command."""
3569+
3570+
def test_remove_restores_lower_priority_command(
3571+
self, project_dir, temp_dir, valid_pack_data
3572+
):
3573+
"""After removing the top-priority preset, the next preset's command
3574+
should be re-registered in agent directories."""
3575+
manager = PresetManager(project_dir)
3576+
3577+
# Install a low-priority preset with a command
3578+
lo_data = {**valid_pack_data}
3579+
lo_data["preset"] = {
3580+
**valid_pack_data["preset"],
3581+
"id": "lo-preset",
3582+
"name": "Lo",
3583+
}
3584+
lo_data["provides"] = {
3585+
"templates": [{
3586+
"type": "command",
3587+
"name": "speckit.specify",
3588+
"file": "commands/speckit.specify.md",
3589+
}]
3590+
}
3591+
lo_dir = temp_dir / "lo-preset"
3592+
lo_dir.mkdir()
3593+
with open(lo_dir / "preset.yml", "w") as f:
3594+
yaml.dump(lo_data, f)
3595+
(lo_dir / "commands").mkdir()
3596+
(lo_dir / "commands" / "speckit.specify.md").write_text(
3597+
"---\ndescription: lo\n---\nLo content\n"
3598+
)
3599+
manager.install_from_directory(lo_dir, "0.1.5", priority=10)
3600+
3601+
# Install a high-priority preset overriding the same command
3602+
hi_data = {**valid_pack_data}
3603+
hi_data["preset"] = {
3604+
**valid_pack_data["preset"],
3605+
"id": "hi-preset",
3606+
"name": "Hi",
3607+
}
3608+
hi_data["provides"] = {
3609+
"templates": [{
3610+
"type": "command",
3611+
"name": "speckit.specify",
3612+
"file": "commands/speckit.specify.md",
3613+
}]
3614+
}
3615+
hi_dir = temp_dir / "hi-preset"
3616+
hi_dir.mkdir()
3617+
with open(hi_dir / "preset.yml", "w") as f:
3618+
yaml.dump(hi_data, f)
3619+
(hi_dir / "commands").mkdir()
3620+
(hi_dir / "commands" / "speckit.specify.md").write_text(
3621+
"---\ndescription: hi\n---\nHi content\n"
3622+
)
3623+
manager.install_from_directory(hi_dir, "0.1.5", priority=1)
3624+
3625+
# Remove the high-priority preset
3626+
manager.remove("hi-preset")
3627+
3628+
# The low-priority preset's command should still be present
3629+
# in the resolution stack
3630+
resolver = PresetResolver(project_dir)
3631+
layers = resolver._collect_all_layers("speckit.specify", "command")
3632+
assert len(layers) >= 1
3633+
assert "lo-preset" in layers[0]["source"]
3634+
3635+
35673636
def _create_pack(temp_dir, valid_pack_data, pack_id, content,
35683637
strategy="replace", template_type="template",
35693638
template_name="spec-template"):

0 commit comments

Comments
 (0)