Skip to content

Commit 6dddae8

Browse files
committed
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
1 parent a77f7e6 commit 6dddae8

3 files changed

Lines changed: 61 additions & 5 deletions

File tree

scripts/bash/common.sh

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,12 @@ except Exception:
543543
prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;;
544544
append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;;
545545
wrap)
546-
# Split on placeholder and rejoin with content to avoid
546+
# Validate placeholder exists
547+
case "$layer_content" in
548+
*'{CORE_TEMPLATE}'*) ;;
549+
*) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;;
550+
esac
551+
# Split on first placeholder and rejoin with content to avoid
547552
# bash & expansion in parameter substitution replacement.
548553
local before="${layer_content%%\{CORE_TEMPLATE\}*}"
549554
local after="${layer_content#*\{CORE_TEMPLATE\}}"
@@ -557,6 +562,10 @@ except Exception:
557562
prepend) content="$(printf '%s\n\n%s' "$layer_content" "$content")" ;;
558563
append) content="$(printf '%s\n\n%s' "$content" "$layer_content")" ;;
559564
wrap)
565+
case "$layer_content" in
566+
*'{CORE_TEMPLATE}'*) ;;
567+
*) echo "Error: wrap strategy missing {CORE_TEMPLATE} placeholder" >&2; return 1 ;;
568+
esac
560569
local before="${layer_content%%\{CORE_TEMPLATE\}*}"
561570
local after="${layer_content#*\{CORE_TEMPLATE\}}"
562571
content="${before}${content}${after}"

scripts/powershell/common.ps1

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,8 @@ function Resolve-TemplateContent {
420420
if ((Test-Path $manifest) -and $pyCmd) {
421421
try {
422422
# Use Python to parse YAML manifest for strategy and file path
423-
$stratResult = & $pyCmd[0] @($pyCmd[1..99]) -c @"
423+
$pyArgs = if ($pyCmd.Count -gt 1) { $pyCmd[1..($pyCmd.Count-1)] } else { @() }
424+
$stratResult = & $pyCmd[0] @pyArgs -c @"
424425
import sys
425426
try:
426427
import yaml
@@ -524,15 +525,25 @@ except Exception:
524525
switch ($strat) {
525526
'prepend' { $content = "$layerContent`n`n$content" }
526527
'append' { $content = "$content`n`n$layerContent" }
527-
'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) }
528+
'wrap' {
529+
if (-not $layerContent.Contains('{CORE_TEMPLATE}')) {
530+
throw "Wrap strategy missing {CORE_TEMPLATE} placeholder"
531+
}
532+
$content = $layerContent.Replace('{CORE_TEMPLATE}', $content)
533+
}
528534
}
529535
}
530536
} else {
531537
switch ($strat) {
532538
'replace' { $content = $layerContent }
533539
'prepend' { $content = "$layerContent`n`n$content" }
534540
'append' { $content = "$content`n`n$layerContent" }
535-
'wrap' { $content = $layerContent.Replace('{CORE_TEMPLATE}', $content) }
541+
'wrap' {
542+
if (-not $layerContent.Contains('{CORE_TEMPLATE}')) {
543+
throw "Wrap strategy missing {CORE_TEMPLATE} placeholder"
544+
}
545+
$content = $layerContent.Replace('{CORE_TEMPLATE}', $content)
546+
}
536547
}
537548
}
538549
}

src/specify_cli/presets.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -747,9 +747,42 @@ def _register_for_non_skill_agents(
747747
agent_name, commands, source_id,
748748
source_dir, self.project_root,
749749
)
750-
except (ValueError, Exception):
750+
except ValueError:
751751
continue
752752

753+
def _reconcile_skills(self, command_names: List[str]) -> None:
754+
"""Re-register skills for commands whose winning layer changed.
755+
756+
After a preset is removed, finds the next preset in the priority
757+
stack that provides each command and re-runs skill registration
758+
for that preset so SKILL.md files reflect the current winner.
759+
760+
Args:
761+
command_names: List of command names to reconcile skills for
762+
"""
763+
if not command_names:
764+
return
765+
766+
resolver = PresetResolver(self.project_root)
767+
for cmd_name in command_names:
768+
layers = resolver._collect_all_layers(cmd_name, "command")
769+
if not layers:
770+
continue
771+
772+
top_path = layers[0]["path"]
773+
# Find the preset that owns the winning layer
774+
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
775+
pack_dir = self.presets_dir / pack_id
776+
if top_path.is_relative_to(pack_dir):
777+
manifest_path = pack_dir / "preset.yml"
778+
if manifest_path.exists():
779+
try:
780+
manifest = PresetManifest(manifest_path)
781+
except PresetValidationError:
782+
break
783+
self._register_skills(manifest, pack_dir)
784+
break
785+
753786
def _get_skills_dir(self) -> Optional[Path]:
754787
"""Return the active skills directory for preset skill overrides.
755788
@@ -1326,6 +1359,9 @@ def remove(self, pack_id: str) -> bool:
13261359
# re-resolve from the remaining stack so the next layer takes effect.
13271360
if removed_cmd_names:
13281361
self._reconcile_composed_commands(list(removed_cmd_names))
1362+
# Also reconcile skills so SKILL.md files reflect the new winning
1363+
# command layer rather than being left absent or stale.
1364+
self._reconcile_skills(list(removed_cmd_names))
13291365

13301366
return True
13311367

0 commit comments

Comments
 (0)