From cbc76e6803a8faade027b087a8160d40937f225f Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 28 Jul 2026 20:34:35 +0500 Subject: [PATCH] fix(skills): apply the line-anchored delimiter scan to hermes and kimi Hermes overrides SkillsIntegration.setup() with its own copy of the frontmatter parse and body strip, and Kimi's _is_speckit_generated_skill() parses frontmatter independently, so all three carried the same split("---", 2) bug the base class just fixed. A description such as "Separate sections with --- markers" truncates the parsed frontmatter at the embedded marker, dropping later keys and spilling the remainder into the body; for Kimi that means a Speckit-generated skill is no longer recognized on teardown and gets left behind. Scan for a closing "---" on its own line instead. The body slice keeps whatever trails the marker so output stays byte-for-byte identical for well-formed templates. --- .../integrations/hermes/__init__.py | 44 ++++++++++--- src/specify_cli/integrations/kimi/__init__.py | 16 ++++- .../test_skill_frontmatter_quoting.py | 63 +++++++++++++++++++ 3 files changed, 113 insertions(+), 10 deletions(-) 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 4517fac037..3a289d60ed 100644 --- a/src/specify_cli/integrations/kimi/__init__.py +++ b/src/specify_cli/integrations/kimi/__init__.py @@ -323,14 +323,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 b42ad88459..c7e7ebb0e8 100644 --- a/tests/integrations/test_skill_frontmatter_quoting.py +++ b/tests/integrations/test_skill_frontmatter_quoting.py @@ -178,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