Skip to content

Commit 06e153e

Browse files
committed
fix: address twentieth round of Copilot PR review feedback
- Fix install rollback: capture registered_commands/registered_skills in local vars before try block so rollback uses actual partial progress instead of reading from registry (which may not be updated) - Fix frontmatter body stripping: remove only the single newline after the closing fence instead of strip() which could remove intentional leading/trailing whitespace from command bodies - Fix PyYAML warning precision: capture stderr from Python helper and check for explicit 'yaml_missing' signal instead of grep heuristic that could false-positive on unrelated strategy: fields
1 parent 0495c1e commit 06e153e

File tree

3 files changed

+30
-21
lines changed

3 files changed

+30
-21
lines changed

scripts/bash/common.sh

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -423,12 +423,14 @@ except Exception:
423423
if [ -f "$manifest" ] && command -v python3 >/dev/null 2>&1; then
424424
# Requires PyYAML; falls back to replace/convention if unavailable
425425
local result
426+
local py_stderr
427+
py_stderr=$(mktemp)
426428
result=$(SPECKIT_MANIFEST="$manifest" SPECKIT_TMPL="$template_name" python3 -c "
427429
import sys, os
428430
try:
429431
import yaml
430432
except ImportError:
431-
print('yaml_missing\treplace\t', file=sys.stderr)
433+
print('yaml_missing', file=sys.stderr)
432434
print('replace\t')
433435
sys.exit(0)
434436
try:
@@ -441,16 +443,17 @@ try:
441443
print('replace\t')
442444
except Exception:
443445
print('replace\t')
444-
" 2>/dev/null)
446+
" 2>"$py_stderr")
445447
local parse_status=$?
446448
if [ $parse_status -eq 0 ] && [ -n "$result" ]; then
447449
IFS=$'\t' read -r strategy manifest_file <<< "$result"
448450
strategy=$(printf '%s' "$strategy" | tr '[:upper:]' '[:lower:]')
449451
fi
450-
# Warn if PyYAML is missing and preset uses non-default file/strategy
451-
if [ -f "$manifest" ] && grep -q 'strategy:' "$manifest" 2>/dev/null && [ "$strategy" = "replace" ] && [ -z "$manifest_file" ]; then
452+
# Warn only when PyYAML is explicitly missing
453+
if grep -q 'yaml_missing' "$py_stderr" 2>/dev/null; then
452454
echo "Warning: PyYAML not available; composition strategies in $manifest may be ignored" >&2
453455
fi
456+
rm -f "$py_stderr"
454457
fi
455458
# Try manifest file path first, then convention path
456459
local candidate=""

scripts/powershell/common.ps1

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -423,13 +423,13 @@ function Resolve-TemplateContent {
423423
try {
424424
# Use Python to parse YAML manifest for strategy and file path
425425
$pyArgs = if ($pyCmd.Count -gt 1) { $pyCmd[1..($pyCmd.Count-1)] } else { @() }
426+
$pyStderr = $null
426427
$stratResult = & $pyCmd[0] @pyArgs -c @"
427428
import sys
428429
try:
429430
import yaml
430431
except ImportError:
431-
import sys as _s
432-
print('yaml_missing', file=_s.stderr)
432+
print('yaml_missing', file=sys.stderr)
433433
print('replace\t')
434434
sys.exit(0)
435435
try:
@@ -442,17 +442,22 @@ try:
442442
print('replace\t')
443443
except Exception:
444444
print('replace\t')
445-
"@ $manifest $TemplateName 2>$null
445+
"@ $manifest $TemplateName 2>&1 | ForEach-Object {
446+
if ($_ -is [System.Management.Automation.ErrorRecord]) {
447+
$pyStderr = $_.ToString()
448+
} else { $_ }
449+
}
446450
if ($stratResult) {
447451
$parts = $stratResult.Trim() -split "`t", 2
448452
$strategy = $parts[0].ToLowerInvariant()
449453
if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] }
450454
}
455+
# Warn only when PyYAML is explicitly missing
456+
if ($pyStderr -and $pyStderr -match 'yaml_missing') {
457+
Write-Warning "PyYAML not available; composition strategies in $manifest may be ignored"
458+
}
451459
} catch {
452460
$strategy = 'replace'
453-
if ((Test-Path $manifest) -and (Select-String -Path $manifest -Pattern 'strategy:' -Quiet -ErrorAction SilentlyContinue)) {
454-
Write-Warning "Cannot parse preset manifest $manifest; composition strategies may be ignored"
455-
}
456461
}
457462
}
458463
# Try manifest file path first, then convention path

src/specify_cli/presets.py

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,6 +1247,8 @@ def install_from_directory(
12471247
"registered_skills": [],
12481248
})
12491249

1250+
registered_commands: Dict[str, List[str]] = {}
1251+
registered_skills: List[str] = []
12501252
try:
12511253
# Register command overrides with AI agents and persist the result
12521254
# immediately so cleanup can recover even if installation stops
@@ -1264,16 +1266,12 @@ def install_from_directory(
12641266
})
12651267
except Exception:
12661268
# Roll back all side effects: unregister any commands/skills that
1267-
# were written, remove the copied preset dir, and drop the
1268-
# registry entry.
1269-
metadata = self.registry.get(manifest.id)
1270-
if metadata:
1271-
rc = metadata.get("registered_commands", {})
1272-
if rc:
1273-
self._unregister_commands(rc)
1274-
rs = metadata.get("registered_skills", [])
1275-
if rs:
1276-
self._unregister_skills(rs, dest_dir)
1269+
# were written (using local vars which capture partial progress),
1270+
# remove the copied preset dir, and drop the registry entry.
1271+
if registered_commands:
1272+
self._unregister_commands(registered_commands)
1273+
if registered_skills:
1274+
self._unregister_skills(registered_skills, dest_dir)
12771275
if dest_dir.exists():
12781276
shutil.rmtree(dest_dir)
12791277
self.registry.remove(manifest.id)
@@ -2457,7 +2455,10 @@ def _split_frontmatter(text: str) -> tuple:
24572455
if end == -1:
24582456
return None, text
24592457
fm_block = text[:end + 3]
2460-
body = text[end + 3:].strip()
2458+
body = text[end + 3:]
2459+
# Remove only the single newline after the closing fence
2460+
if body.startswith("\n"):
2461+
body = body[1:]
24612462
return fm_block, body
24622463

24632464
if is_command:

0 commit comments

Comments
 (0)