Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/specify_cli/integrations/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1627,13 +1627,27 @@ def setup(
command_name = src_file.stem # e.g. "plan"
skill_name = f"speckit-{command_name.replace('.', '-')}"

# Parse frontmatter for description
# Parse frontmatter for description. Locate the closing ``---`` on
# its own line rather than with ``raw.split("---", 2)`` — a bare
# substring split stops at the first ``---`` *anywhere*, including
# one inside a value such as ``description: Separate sections
# with ---``, which truncates the frontmatter and drops later keys.
# The block between the delimiters is parsed unstripped so trailing
# newlines in literal (``|``) block scalars survive.
frontmatter: dict[str, Any] = {}
if raw.startswith("---"):
parts = raw.split("---", 2)
if len(parts) >= 3:
fm_lines = raw.splitlines(keepends=True)
fm_close = next(
(
i
for i in range(1, len(fm_lines))
if fm_lines[i].rstrip() == "---"
Comment on lines +1640 to +1644
),
None,
)
if fm_close is not None:
try:
fm = yaml.safe_load(parts[1])
fm = yaml.safe_load("".join(fm_lines[1:fm_close]))
if isinstance(fm, dict):
frontmatter = fm
except yaml.YAMLError:
Expand All @@ -1648,11 +1662,27 @@ def setup(
# Strip the processed frontmatter — we rebuild it for skills.
# Preserve leading whitespace in the body to match release ZIP
# output byte-for-byte (the template body starts with \n after
# the closing ---).
# the closing ---). Scan for the closing ``---`` on its own line
# rather than ``split("---", 2)`` so a ``---`` embedded in a value
# does not truncate the frontmatter and spill it into the body.
if processed_body.startswith("---"):
parts = processed_body.split("---", 2)
if len(parts) >= 3:
processed_body = parts[2]
body_lines = processed_body.splitlines(keepends=True)
close_idx = next(
(
i
for i in range(1, len(body_lines))
if body_lines[i].rstrip() == "---"
),
None,
)
if close_idx is not None:
# Keep whatever trails the ``---`` marker on the closing
# line (normally just the newline) so the body stays
# byte-for-byte identical to ``split("---", 2)[2]``. The
# line-anchored check guarantees ``---`` sits at index 0.
processed_body = body_lines[close_idx][3:] + "".join(
body_lines[close_idx + 1 :]
)

# Select description — use the original template description
# to stay byte-for-byte identical with release ZIP output.
Expand Down
44 changes: 37 additions & 7 deletions src/specify_cli/integrations/hermes/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,27 @@ def setup(
command_name = src_file.stem # e.g. "plan"
skill_name = f"speckit-{command_name.replace('.', '-')}"

# Parse frontmatter for description
# Parse frontmatter for description. Locate the closing ``---`` on
# its own line rather than with ``raw.split("---", 2)`` — a bare
# substring split stops at the first ``---`` *anywhere*, including
# one inside a value such as ``description: Separate sections
# with ---``, which truncates the frontmatter and drops later keys.
# The block between the delimiters is parsed unstripped so trailing
# newlines in literal (``|``) block scalars survive.
frontmatter: dict[str, Any] = {}
if raw.startswith("---"):
parts = raw.split("---", 2)
if len(parts) >= 3:
fm_lines = raw.splitlines(keepends=True)
fm_close = next(
(
i
for i in range(1, len(fm_lines))
if fm_lines[i].rstrip() == "---"
),
None,
)
if fm_close is not None:
try:
fm = yaml.safe_load(parts[1])
fm = yaml.safe_load("".join(fm_lines[1:fm_close]))
if isinstance(fm, dict):
frontmatter = fm
except yaml.YAMLError:
Expand All @@ -143,10 +157,26 @@ def setup(
project_root=project_root,
)
# Strip the processed frontmatter — we rebuild it for skills.
# Scan for the closing ``---`` on its own line rather than
# ``split("---", 2)`` so a ``---`` embedded in a value does not
# truncate the frontmatter and spill it into the body.
if processed_body.startswith("---"):
parts = processed_body.split("---", 2)
if len(parts) >= 3:
processed_body = parts[2]
body_lines = processed_body.splitlines(keepends=True)
close_idx = next(
(
i
for i in range(1, len(body_lines))
if body_lines[i].rstrip() == "---"
),
None,
)
if close_idx is not None:
# Keep whatever trails the ``---`` marker on the closing
# line so the body stays byte-for-byte identical to
# ``split("---", 2)[2]`` for well-formed templates.
processed_body = body_lines[close_idx][3:] + "".join(
body_lines[close_idx + 1 :]
)

# Select description
description = frontmatter.get("description", "")
Expand Down
16 changes: 13 additions & 3 deletions src/specify_cli/integrations/kimi/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -324,14 +324,24 @@ def _is_speckit_generated_skill(skill_dir: Path) -> bool:
if not content.startswith("---"):
return False

parts = content.split("---", 2)
if len(parts) < 3:
# Locate the closing ``---`` on its own line rather than with
# ``content.split("---", 2)`` — a bare substring split stops at the first
# ``---`` *anywhere*, including one inside a value such as
# ``description: Separate sections with ---``, which truncates the parsed
# frontmatter and can drop the metadata block this check relies on (so a
# Speckit-generated skill would not be recognized on teardown).
lines = content.splitlines(keepends=True)
close_idx = next(
(i for i in range(1, len(lines)) if lines[i].rstrip() == "---"),
None,
)
if close_idx is None:
return False

try:
import yaml

frontmatter = yaml.safe_load(parts[1])
frontmatter = yaml.safe_load("".join(lines[1:close_idx]))
except Exception:
return False

Expand Down
132 changes: 132 additions & 0 deletions tests/integrations/test_skill_frontmatter_quoting.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,19 @@
Body of the command.
"""

# A description whose value contains an embedded ``---``. A substring split
# (``raw.split("---", 2)``) stops at this inner marker, truncating the parsed
# frontmatter — the closing document separator on its own line is the real
# boundary. See TestSkillFrontmatterEmbeddedDashes below.
DASHED_DESCRIPTION = "Separate sections with --- markers"
DASHED_TEMPLATE = """---
description: Separate sections with --- markers
name-marker: sentinel
---

Body of the command.
"""


def _parse_frontmatter(skill_file: Path) -> dict:
content = skill_file.read_text(encoding="utf-8")
Expand Down Expand Up @@ -90,6 +103,62 @@ def test_control_character_description_parses(self, tmp_path, monkeypatch):
assert fm["description"] == CONTROL


def _parse_frontmatter_line_anchored(skill_file: Path) -> dict:
"""Parse SKILL.md frontmatter using the closing ``---`` on its own line.

Unlike ``_parse_frontmatter`` (which uses ``split("---", 2)``), this is
robust to a ``---`` embedded in a value, so it can validate that the
generated frontmatter is itself well formed.
"""
content = skill_file.read_text(encoding="utf-8")
assert content.startswith("---\n")
lines = content.splitlines(keepends=True)
end = next(i for i in range(1, len(lines)) if lines[i].rstrip() == "---")
return yaml.safe_load("".join(lines[1:end]))


class TestSkillFrontmatterEmbeddedDashes:
"""A ``---`` inside a description value must not truncate parsing (#3634).

The skills setup path parsed template frontmatter with
``raw.split("---", 2)``, which stops at the first ``---`` *anywhere* —
including one inside a value such as ``description: ... --- ...``. That
dropped every frontmatter key after the marker (so the description fell
back to the generic default) and spilled the leftover frontmatter into
the skill body. The parser must match the closing ``---`` on its own line.
"""

def _generate(self, tmp_path, monkeypatch, template: str) -> Path:
integration = get_integration("agy")
monkeypatch.setattr(
integration,
"shared_commands_dir",
lambda: _fake_templates(tmp_path, template),
)
manifest = IntegrationManifest("agy", tmp_path)
created = integration.setup(tmp_path, manifest)
skill_files = [f for f in created if f.name == "SKILL.md"]
assert len(skill_files) == 1
return skill_files[0]

def test_dashed_description_is_preserved(self, tmp_path, monkeypatch):
skill_file = self._generate(tmp_path, monkeypatch, DASHED_TEMPLATE)
fm = _parse_frontmatter_line_anchored(skill_file)
# Buggy split("---", 2) truncates the value to "Separate sections with"
# (or drops it entirely, falling back to "Spec Kit: plan workflow").
assert fm["description"] == DASHED_DESCRIPTION

def test_leftover_frontmatter_not_spilled_into_body(self, tmp_path, monkeypatch):
skill_file = self._generate(tmp_path, monkeypatch, DASHED_TEMPLATE)
content = skill_file.read_text(encoding="utf-8")
lines = content.splitlines(keepends=True)
end = next(i for i in range(1, len(lines)) if lines[i].rstrip() == "---")
body = "".join(lines[end + 1 :])
# The template's trailing frontmatter key must not leak into the body.
assert "name-marker: sentinel" not in body
assert "Body of the command." in body


class TestHermesSkillFrontmatterQuoting:
def test_multiline_description_survives(self, tmp_path, monkeypatch):
home = tmp_path / "home"
Expand All @@ -109,3 +178,66 @@ def test_multiline_description_survives(self, tmp_path, monkeypatch):

fm = _parse_frontmatter(skill_files[0])
assert fm["description"] == MULTILINE

def test_dashed_description_is_preserved(self, tmp_path, monkeypatch):
"""Hermes overrides setup(), so it needs the same line-anchored parse."""
home = tmp_path / "home"
home.mkdir(exist_ok=True)
monkeypatch.setattr(Path, "home", lambda: home)

integration = get_integration("hermes")
monkeypatch.setattr(
integration,
"shared_commands_dir",
lambda: _fake_templates(tmp_path, DASHED_TEMPLATE),
)
manifest = IntegrationManifest("hermes", tmp_path)
created = integration.setup(tmp_path, manifest)
skill_files = [f for f in created if f.name == "SKILL.md"]
assert len(skill_files) == 1

fm = _parse_frontmatter_line_anchored(skill_files[0])
assert fm["description"] == DASHED_DESCRIPTION

content = skill_files[0].read_text(encoding="utf-8")
lines = content.splitlines(keepends=True)
end = next(i for i in range(1, len(lines)) if lines[i].rstrip() == "---")
body = "".join(lines[end + 1 :])
assert "name-marker: sentinel" not in body


class TestKimiGeneratedSkillDetection:
"""``_is_speckit_generated_skill`` must survive a ``---`` in a value.

Teardown only removes a legacy skill directory it recognizes as
Speckit-generated via the frontmatter ``metadata`` block. A substring split
truncated the frontmatter before ``metadata`` when a description embedded
``---``, so the directory was left behind on uninstall.
"""

def _write_skill(self, skill_dir: Path, description: str) -> None:
skill_dir.mkdir(parents=True, exist_ok=True)
(skill_dir / "SKILL.md").write_text(
"---\n"
'name: "speckit-plan"\n'
f"description: {description}\n"
"metadata:\n"
' author: "github-spec-kit"\n'
' source: "templates/commands/plan.md"\n'
"---\n\nBody.\n",
encoding="utf-8",
)

def test_detects_skill_with_dashes_in_description(self, tmp_path):
from specify_cli.integrations.kimi import _is_speckit_generated_skill

skill_dir = tmp_path / "speckit-plan"
self._write_skill(skill_dir, "Separate sections with --- markers")
assert _is_speckit_generated_skill(skill_dir) is True

def test_still_detects_plain_description(self, tmp_path):
from specify_cli.integrations.kimi import _is_speckit_generated_skill

skill_dir = tmp_path / "speckit-plan"
self._write_skill(skill_dir, "Plain description")
assert _is_speckit_generated_skill(skill_dir) is True