Skip to content

Commit 3895d0d

Browse files
committed
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
1 parent 9510e71 commit 3895d0d

File tree

3 files changed

+38
-7
lines changed

3 files changed

+38
-7
lines changed

scripts/bash/common.sh

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,9 +419,15 @@ except Exception:
419419
local manifest_file=""
420420
local manifest="$presets_dir/$preset_id/preset.yml"
421421
if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then
422+
# Requires PyYAML; falls back to replace/convention if unavailable
422423
local result
423424
result=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c "
424-
import yaml, sys, os
425+
import sys, os
426+
try:
427+
import yaml
428+
except ImportError:
429+
print('replace\t')
430+
sys.exit(0)
425431
try:
426432
with open(os.environ['SPECKIT_MANIFEST']) as f:
427433
data = yaml.safe_load(f)

scripts/powershell/common.ps1

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,11 +401,16 @@ function Resolve-TemplateContent {
401401
$strategy = 'replace'
402402
$manifestFilePath = ''
403403
$manifest = Join-Path $presetsDir "$presetId/preset.yml"
404-
if (Test-Path $manifest) {
404+
if ((Test-Path $manifest) -and (Get-Command python3 -ErrorAction SilentlyContinue)) {
405405
try {
406406
# Use python3 to parse YAML manifest for strategy and file path
407407
$stratResult = & python3 -c @"
408-
import yaml, sys
408+
import sys
409+
try:
410+
import yaml
411+
except ImportError:
412+
print('replace\t')
413+
sys.exit(0)
409414
try:
410415
with open(sys.argv[1]) as f:
411416
data = yaml.safe_load(f)

src/specify_cli/presets.py

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
637637
registered = False
638638
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
639639
pack_dir = self.presets_dir / pack_id
640-
if str(top_path).startswith(str(pack_dir)):
640+
if top_path.is_relative_to(pack_dir):
641641
manifest_path = pack_dir / "preset.yml"
642642
if manifest_path.exists():
643643
try:
@@ -1170,7 +1170,19 @@ def install_from_directory(
11701170
"registered_skills": registered_skills,
11711171
})
11721172
except Exception:
1173-
# Roll back registry entry on failure
1173+
# Roll back all side effects: unregister any commands/skills that
1174+
# were written, remove the copied preset dir, and drop the
1175+
# registry entry.
1176+
metadata = self.registry.get(manifest.id)
1177+
if metadata:
1178+
rc = metadata.get("registered_commands", {})
1179+
if rc:
1180+
self._unregister_commands(rc)
1181+
rs = metadata.get("registered_skills", [])
1182+
if rs:
1183+
self._unregister_skills(rs, dest_dir)
1184+
if dest_dir.exists():
1185+
shutil.rmtree(dest_dir)
11741186
self.registry.remove(manifest.id)
11751187
raise
11761188

@@ -2276,8 +2288,16 @@ def resolve_content(
22762288
content = content + "\n\n" + layer_content
22772289
elif strategy == "wrap":
22782290
if template_type == "script":
2279-
content = layer_content.replace("$CORE_SCRIPT", content)
2291+
placeholder = "$CORE_SCRIPT"
22802292
else:
2281-
content = layer_content.replace("{CORE_TEMPLATE}", content)
2293+
placeholder = "{CORE_TEMPLATE}"
2294+
if placeholder not in layer_content:
2295+
raise PresetValidationError(
2296+
f"Wrap strategy in '{layer['source']}' is missing "
2297+
f"the {placeholder} placeholder. The wrapper must "
2298+
f"contain {placeholder} to indicate where the "
2299+
f"lower-priority content should be inserted."
2300+
)
2301+
content = layer_content.replace(placeholder, content)
22822302

22832303
return content

0 commit comments

Comments
 (0)