Unify Kimi/Codex skill naming and migrate legacy dotted Kimi dirs#1971
Unify Kimi/Codex skill naming and migrate legacy dotted Kimi dirs#1971mnriem merged 23 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes skill naming to a single hyphenated format (speckit-...) across Kimi and Codex, and updates related plumbing (hook invocation rendering, preset/extension overrides, packaging) to match that canonical scheme while adding a temporary Kimi legacy directory migration.
Changes:
- Normalize generated skill directory names to hyphenated form and update extension/preset override mapping for multi-segment command IDs.
- Update hook messaging/execution metadata to emit agent-specific invocation strings (e.g.,
/skill:speckit-planfor Kimi,$speckit-tasksfor Codex skills mode). - Sync template packaging scripts and add a temporary migration helper for legacy dotted Kimi skill directories.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/__init__.py |
Unifies skill naming, adds Kimi dotted-dir migration helper, updates init output/invocation hints, removes Codex skills dir override. |
src/specify_cli/agents.py |
Makes skills-mode output naming consistently hyphenated (including multi-segment IDs). |
src/specify_cli/presets.py |
Fixes preset skill override mapping for multi-segment command IDs by hyphenating derived skill names. |
src/specify_cli/extensions.py |
Adds init-options-aware hook invocation rendering and returns invocation metadata from hook execution. |
tests/test_ai_skills.py |
Updates expectations to hyphenated Kimi skills and adds tests for legacy Kimi dotted-dir migration. |
tests/test_extensions.py |
Updates Codex skill path/name assertions and adds hook invocation rendering tests. |
tests/test_presets.py |
Adds regression test for hyphenated skill override mapping for speckit.<ext>.<cmd>. |
.github/workflows/scripts/create-release-packages.sh |
Updates Kimi packaging to generate hyphenated skill names. |
.github/workflows/scripts/create-release-packages.ps1 |
Updates Kimi packaging to generate hyphenated skill names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/specify_cli/extensions.py:2024
HookExecutor.execute_hook()now returns aninvocationfield, but the docstring’s “Returns” section doesn’t mention it. Please update the return-value documentation to includeinvocationso callers know it’s available (and what format it uses).
Returns:
Dictionary with execution information:
- command: str - Command to execute
- extension: str - Extension ID
- optional: bool - Whether hook is optional
- description: str - Hook description
"""
return {
"command": hook.get("command"),
"invocation": self._render_hook_invocation(hook.get("command")),
"extension": hook.get("extension"),
"optional": hook.get("optional", True),
"description": hook.get("description", ""),
"prompt": hook.get("prompt", "")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/specify_cli/extensions.py:1676
_render_hook_invocation()is annotated to takecommand: str, but it’s called withhook.get('command')(potentiallyNone) and defensively checks for non-strings. Updating the parameter type (e.g.,str | None) would better reflect actual usage and avoid type-checking warnings.
def _render_hook_invocation(self, command: str) -> str:
"""Render an agent-specific invocation string for a hook command."""
if not isinstance(command, str):
return ""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please fix test & lint errors
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/specify_cli/presets.py:704
- Same robustness issue as
_get_skills_dir():load_init_options()may return a non-dict for malformed-but-parseable JSON, which would makeinit_opts.get('ai')raise here. Please defensively coerce to{}unless it’s a dict before readingaiso preset skill writes don’t crash.
init_opts = load_init_options(self.project_root)
selected_ai = init_opts.get("ai")
if not isinstance(selected_ai, str):
return []
src/specify_cli/presets.py:799
init_opts = load_init_options(...)is assumed to be a dict, butload_init_options()can return other JSON types if the file is corrupted (e.g.,[]). That would makeinit_opts.get('ai')raise and break preset removal. Consider normalizinginit_optsto{}unless it’s a dict before readingai.
init_opts = load_init_options(self.project_root)
selected_ai = init_opts.get("ai")
registrar = CommandRegistrar()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address more Copilot feedback. Hang in there we are close!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
More Copilot feedback. It often pivots to one area and then when that is done goes to the next area. Hence why you see the back and forth. Hopefully this will be very close to the last round. You rock for working through this!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/specify_cli/presets.py:859
- Same readability issue as in preset skill registration: the restored SKILL.md header uses
short_name.title(), which preserves hyphens in multi-segment names. Consider replacing-/.with spaces before title-casing to keep generated headers consistent and human-friendly.
f"---\n\n"
f"# Speckit {short_name.title()} Skill\n\n"
f"{body}\n"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you! |
Summary
This PR now combines two tightly related tracks on the same branch:
What Changed
A) Skill naming/migration alignment (original scope)
speckit-*) for Kimi/Codex paths.EXECUTE_COMMAND_INVOCATION.speckit.xxx) with conservative cleanup behavior.B) Regression fixes discovered in review cycle
C) Issue #1976 hardening (newly included)
agents.py:render_toml_command()so TOML remains valid even when body contains triple-quote delimiters._adjust_script_paths()non-mutating (defensive copy) to reduce side effects.presets.py:_unregister_skills()now reusesCommandRegistrar.parse_frontmatter().{SCRIPT},{AGENT_SCRIPT},{ARGS},__AGENT__) for consistency with other skill-generation paths.Why
Validation
uv run pytest tests/test_extensions.py tests/test_presets.py::TestPresetSkills150 passeduv run pytest tests/test_timestamp_branches.py::TestSequentialBranch::test_sequential_supports_four_digit_prefixes tests/test_timestamp_branches.py::TestSequentialBranch::test_sequential_ignores_timestamp_dirs2 passeduvx ruff check src/specify_cli/agents.py src/specify_cli/presets.py tests/test_extensions.py tests/test_presets.pyAll checks passedRelated