Skip to content

Commit 0ec81eb

Browse files
committed
fix: address thirtieth round of Copilot PR review feedback
- Guard legacy frontmatter fallback: only check command file frontmatter for strategy when the manifest entry doesn't explicitly include the strategy key, preventing override of manifest-declared strategies - Document rollback limitation: note that mid-registration failures may leave orphaned agent command files since partial progress isn't captured by the local vars
1 parent 0a70488 commit 0ec81eb

1 file changed

Lines changed: 13 additions & 6 deletions

File tree

src/specify_cli/presets.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1437,9 +1437,12 @@ def install_from_directory(
14371437
"registered_skills": registered_skills,
14381438
})
14391439
except Exception:
1440-
# Roll back all side effects: unregister any commands/skills that
1441-
# were written (using local vars which capture partial progress),
1442-
# remove the copied preset dir, and drop the registry entry.
1440+
# Roll back all side effects. Note: if _register_commands or
1441+
# _register_skills raised mid-way (e.g. I/O error after writing
1442+
# some files), registered_commands/registered_skills may be empty
1443+
# and some agent command files could be orphaned. Removing dest_dir
1444+
# (which contains .composed/) and the registry entry ensures the
1445+
# preset system is consistent even if orphaned files remain.
14431446
if registered_commands:
14441447
self._unregister_commands(registered_commands)
14451448
if registered_skills:
@@ -2571,12 +2574,14 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
25712574
# Read strategy and manifest file path from preset manifest
25722575
strategy = "replace"
25732576
manifest_file_path = None
2577+
manifest_has_strategy = False
25742578
manifest = self._get_manifest(pack_dir)
25752579
if manifest:
25762580
for tmpl in manifest.templates:
25772581
if (tmpl.get("name") == template_name
25782582
and tmpl.get("type") == template_type):
25792583
strategy = tmpl.get("strategy", "replace")
2584+
manifest_has_strategy = "strategy" in tmpl
25802585
manifest_file_path = tmpl.get("file")
25812586
break
25822587
# Use manifest file path if specified, otherwise convention-based lookup
@@ -2588,9 +2593,11 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
25882593
if candidate is None:
25892594
candidate = _find_in_subdirs(pack_dir)
25902595
if candidate:
2591-
# Legacy fallback: if manifest doesn't declare a strategy,
2592-
# check the command file's frontmatter for any valid strategy
2593-
if strategy == "replace" and template_type == "command":
2596+
# Legacy fallback: if manifest doesn't explicitly declare a
2597+
# strategy, check the command file's frontmatter for any valid
2598+
# strategy. Skip when the manifest entry includes strategy key
2599+
# (even if it's "replace") to avoid overriding explicit declarations.
2600+
if not manifest_has_strategy and strategy == "replace" and template_type == "command":
25942601
try:
25952602
cmd_content = candidate.read_text(encoding="utf-8")
25962603
lines = cmd_content.splitlines(keepends=True)

0 commit comments

Comments
 (0)