Skip to content

Commit 58e9f67

Browse files
committed
fix: address twenty-first round of Copilot PR review feedback
- Fix _create_pack helper for command type: use commands/ subdir and file path for template_type='command' instead of always templates/ - Cache registry and manifests in reconcile loop: compute list_by_priority() once and reuse resolver._get_manifest() cache to avoid O(commands*presets) YAML loads - Use line-based frontmatter splitter: scan for --- on its own line to avoid false matches on --- inside YAML values, consistent with integrations/base.py approach - Fix PS1 stderr capture: redirect to temp file instead of 2>&1 pipe which could merge stderr into stdout and break stratResult parsing
1 parent 06e153e commit 58e9f67

File tree

3 files changed

+39
-33
lines changed

3 files changed

+39
-33
lines changed

scripts/powershell/common.ps1

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ 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
426+
$pyStderrFile = [System.IO.Path]::GetTempFileName()
427427
$stratResult = & $pyCmd[0] @pyArgs -c @"
428428
import sys
429429
try:
@@ -442,20 +442,17 @@ try:
442442
print('replace\t')
443443
except Exception:
444444
print('replace\t')
445-
"@ $manifest $TemplateName 2>&1 | ForEach-Object {
446-
if ($_ -is [System.Management.Automation.ErrorRecord]) {
447-
$pyStderr = $_.ToString()
448-
} else { $_ }
449-
}
445+
"@ $manifest $TemplateName 2>$pyStderrFile
450446
if ($stratResult) {
451447
$parts = $stratResult.Trim() -split "`t", 2
452448
$strategy = $parts[0].ToLowerInvariant()
453449
if ($parts.Count -gt 1 -and $parts[1]) { $manifestFilePath = $parts[1] }
454450
}
455451
# Warn only when PyYAML is explicitly missing
456-
if ($pyStderr -and $pyStderr -match 'yaml_missing') {
452+
if ((Test-Path $pyStderrFile) -and (Get-Content $pyStderrFile -Raw -ErrorAction SilentlyContinue) -match 'yaml_missing') {
457453
Write-Warning "PyYAML not available; composition strategies in $manifest may be ignored"
458454
}
455+
Remove-Item $pyStderrFile -Force -ErrorAction SilentlyContinue
459456
} catch {
460457
$strategy = 'replace'
461458
}

src/specify_cli/presets.py

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,10 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
638638
resolver = PresetResolver(self.project_root)
639639
registrar = CommandRegistrar()
640640

641+
# Cache registry and manifests outside the loop to avoid
642+
# repeated filesystem reads for each command name.
643+
presets_by_priority = list(PresetRegistry(self.presets_dir).list_by_priority()) if self.presets_dir.exists() else []
644+
641645
for cmd_name in command_names:
642646
layers = resolver.collect_all_layers(cmd_name, "command")
643647
if not layers:
@@ -655,15 +659,11 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
655659
top_path = top_layer["path"]
656660
# Try to find which preset owns this layer
657661
registered = False
658-
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
662+
for pack_id, _meta in presets_by_priority:
659663
pack_dir = self.presets_dir / pack_id
660664
if top_path.is_relative_to(pack_dir):
661-
manifest_path = pack_dir / "preset.yml"
662-
if manifest_path.exists():
663-
try:
664-
manifest = PresetManifest(manifest_path)
665-
except PresetValidationError:
666-
continue
665+
manifest = resolver._get_manifest(pack_dir)
666+
if manifest:
667667
for tmpl in manifest.templates:
668668
if tmpl.get("name") == cmd_name and tmpl.get("type") == "command":
669669
self._register_for_non_skill_agents(
@@ -694,14 +694,10 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
694694

695695
# Write to the highest-priority preset's .composed dir
696696
registered = False
697-
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
697+
for pack_id, _meta in presets_by_priority:
698698
pack_dir = self.presets_dir / pack_id
699-
manifest_path = pack_dir / "preset.yml"
700-
if not manifest_path.exists():
701-
continue
702-
try:
703-
manifest = PresetManifest(manifest_path)
704-
except PresetValidationError:
699+
manifest = resolver._get_manifest(pack_dir)
700+
if not manifest:
705701
continue
706702
for tmpl in manifest.templates:
707703
if tmpl.get("name") == cmd_name and tmpl.get("type") == "command":
@@ -2445,20 +2441,24 @@ def resolve_content(
24452441
def _split_frontmatter(text: str) -> tuple:
24462442
"""Return (frontmatter_block_with_fences, body) or (None, text).
24472443
2448-
Detects frontmatter by the presence of ``---`` fences rather than
2449-
whether the parsed YAML has keys, so empty frontmatter blocks
2450-
(``---\\n---``) are also stripped.
2444+
Uses line-based fence detection (fence must be ``---`` on its
2445+
own line) to avoid false matches on ``---`` inside YAML values.
24512446
"""
2452-
if not text.startswith("---"):
2447+
lines = text.splitlines(keepends=True)
2448+
if not lines or lines[0].rstrip("\r\n") != "---":
24532449
return None, text
2454-
end = text.find("---", 3)
2455-
if end == -1:
2450+
2451+
fence_end = -1
2452+
for i, line in enumerate(lines[1:], start=1):
2453+
if line.rstrip("\r\n") == "---":
2454+
fence_end = i
2455+
break
2456+
2457+
if fence_end == -1:
24562458
return None, text
2457-
fm_block = text[:end + 3]
2458-
body = text[end + 3:]
2459-
# Remove only the single newline after the closing fence
2460-
if body.startswith("\n"):
2461-
body = body[1:]
2459+
2460+
fm_block = "".join(lines[:fence_end + 1]).rstrip("\n")
2461+
body = "".join(lines[fence_end + 1:])
24622462
return fm_block, body
24632463

24642464
if is_command:

tests/test_presets.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3746,8 +3746,13 @@ def _create_pack(temp_dir, valid_pack_data, pack_id, content,
37463746
tmpl_entry = {
37473747
"type": template_type,
37483748
"name": template_name,
3749-
"file": f"templates/{template_name}.md" if template_type != "script" else f"scripts/{template_name}.sh",
37503749
}
3750+
if template_type == "script":
3751+
tmpl_entry["file"] = f"scripts/{template_name}.sh"
3752+
elif template_type == "command":
3753+
tmpl_entry["file"] = f"commands/{template_name}.md"
3754+
else:
3755+
tmpl_entry["file"] = f"templates/{template_name}.md"
37513756
if strategy != "replace":
37523757
tmpl_entry["strategy"] = strategy
37533758
pack_data["provides"] = {"templates": [tmpl_entry]}
@@ -3761,6 +3766,10 @@ def _create_pack(temp_dir, valid_pack_data, pack_id, content,
37613766
subdir = pack_dir / "scripts"
37623767
subdir.mkdir(exist_ok=True)
37633768
(subdir / f"{template_name}.sh").write_text(content)
3769+
elif template_type == "command":
3770+
subdir = pack_dir / "commands"
3771+
subdir.mkdir(exist_ok=True)
3772+
(subdir / f"{template_name}.md").write_text(content)
37643773
else:
37653774
subdir = pack_dir / "templates"
37663775
subdir.mkdir(exist_ok=True)

0 commit comments

Comments
 (0)