diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index ccc8587709..8664c181d3 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -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() == "---" + ), + 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: @@ -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. diff --git a/src/specify_cli/integrations/hermes/__init__.py b/src/specify_cli/integrations/hermes/__init__.py index 63ea5f9986..a82eb6fd4d 100644 --- a/src/specify_cli/integrations/hermes/__init__.py +++ b/src/specify_cli/integrations/hermes/__init__.py @@ -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: @@ -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", "") diff --git a/src/specify_cli/integrations/kimi/__init__.py b/src/specify_cli/integrations/kimi/__init__.py index 3320935a03..2b683b0385 100644 --- a/src/specify_cli/integrations/kimi/__init__.py +++ b/src/specify_cli/integrations/kimi/__init__.py @@ -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 diff --git a/tests/integrations/test_skill_frontmatter_quoting.py b/tests/integrations/test_skill_frontmatter_quoting.py index 739039ea3f..c7e7ebb0e8 100644 --- a/tests/integrations/test_skill_frontmatter_quoting.py +++ b/tests/integrations/test_skill_frontmatter_quoting.py @@ -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") @@ -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" @@ -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