test(integrations): guard multiline/control-char SKILL.md frontmatter escaping

[Quratulain-bilal]

Resolved the conflicts by rebasing onto main.

When this PR was opened it added a helper to escape multiline / control characters in generated SKILL.md frontmatter. In the meantime #3399 landed yaml_quote() in integrations/base.py and routes every SKILL.md frontmatter field through it, which already provides that escaping. So the production change here was fully redundant and I dropped it during the rebase (including the now-unused _yaml_string.py).

What remains is the regression coverage, which main did not have: two tests on the shared SkillsIntegrationTests mixin that round-trip a multiline description and a control-character description back through the YAML parser, asserting the rendered SKILL.md frontmatter stays valid and lossless. They run across every skills integration (agy, codex, hermes, kimi, …) and lock in the yaml_quote() behavior so a future hand-rolled quote can't silently regress it.

Net diff is now test-only. Happy to close instead if you'd rather not carry the extra coverage, but it guards a real corruption path that had no direct test before.

[Copilot]

Pull request overview

Fixes YAML frontmatter generation for skills-based integrations by centralizing proper YAML double-quoted scalar escaping, preventing multiline descriptions and control characters from corrupting or breaking generated SKILL.md files.

Changes:

• Added specify_cli/_yaml_string.quote_yaml_double() to escape newlines/CR/tab and other control characters into valid YAML escapes.
• Switched the manual SKILL.md frontmatter renderers in SkillsIntegration and HermesIntegration to use the shared quoting helper.
• Added a regression test to ensure multiline template descriptions round-trip through YAML parsing.

Show a summary per file

File	Description
tests/integrations/test_integration_base_skills.py	Adds regression coverage for multiline description round-trip in generated SKILL.md frontmatter.
src/specify_cli/integrations/hermes/init.py	Uses shared YAML quoting helper for Hermes skill frontmatter rendering.
src/specify_cli/integrations/base.py	Uses shared YAML quoting helper for skills frontmatter rendering in the base skills setup path.
src/specify_cli/_yaml_string.py	Introduces a shared, reusable YAML double-quoted scalar escaping helper.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Low

[mnriem]

Please address Copilot feedback and resolve conflicts

[Quratulain-bilal]

good call. added a second round-trip (fcdbbcd) that feeds a C0 control char (U+0008, via a yaml escape in the source template) through quote_yaml_double and asserts it survives byte for byte. confirmed it fails on the pre-fix naive quote (raw control char makes the SKILL.md unparseable, yaml ReaderError) and passes with the fix.

[Copilot]

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Low

[mnriem]

Please resolve conflicts

[Quratulain-bilal]

Closing this as superseded rather than resolving the conflicts, since the conflict is with the fix that replaced it.

This PR added a _yaml_string.py helper to escape multiline / control characters in SKILL.md frontmatter. That exact need is now covered on main by #3399, which introduced yaml_quote() in integrations/base.py and routes every generated SKILL.md frontmatter field through it (name, description, compatibility, metadata.author, metadata.source). yaml_quote() uses the YAML emitter to produce the escapes, so newlines and control characters are handled — the same guarantee this PR aimed for.

Rebasing would just re-introduce a second, parallel helper for behavior that already ships, so closing is cleaner. Happy to reopen if there's an edge case yaml_quote() misses that this approach caught.

[Copilot]

Review details

Comments suppressed due to low confidence (1)

tests/integrations/test_integration_base_skills.py:190

• This test also runs through TestHermesIntegration, so the unisolated setup() call writes the generated sample skill into the user's actual Hermes home and may destroy an existing skill. Redirect Path.home() to tmp_path before setup to keep the regression test hermetic.

        i = get_integration(self.KEY)
        template = tmp_path / "sample.md"

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[Copilot]

Review details

Comments suppressed due to low confidence (5)

tests/integrations/test_integration_base_skills.py:187

• The implementation retained after the rebase is named yaml_quote; quote_yaml_double does not exist. Update the regression-test explanation to reference the actual helper.

        SKILL.md unparseable / lossy. ``quote_yaml_double`` must escape it so the

tests/integrations/test_integration_base_skills.py:169

• This inherited test also runs for Hermes, whose setup() writes to Path.home()/.hermes/skills; unlike every Hermes-specific setup test, it does not isolate Path.home() first (test_integration_hermes.py:3-10,36-39). Running the suite can therefore overwrite a developer's real speckit-sample/SKILL.md. Redirect Hermes' global skills directory to tmp_path before setup.

        created = i.setup(tmp_path, m)

tests/integrations/test_integration_base_skills.py:203

• This test is inherited by Hermes and invokes its global-home setup without the isolation required by test_integration_hermes.py:3-10. It can modify the user's real ~/.hermes/skills/speckit-sample/SKILL.md; redirect the Hermes destination into tmp_path before calling setup.

        created = i.setup(tmp_path, m)

src/specify_cli/integrations/base.py:1633

• Splitting away line endings and joining only the frontmatter slice removes the terminal newline when a literal block scalar is the final key before ---. That changes description: | from "first\nsecond\n" to "first\nsecond"; the existing hostile-template regression test in test_skill_frontmatter_quoting.py:95-98 catches this, while the new mixin test does not because it places scripts after the description. Preserve the original line endings when parsing.

                fm_lines = raw.split("\n")

src/specify_cli/integrations/base.py:1671

• This drops the newline attached to the closing template delimiter, so generated skills change from ---\n\nBody to ---\nBody, contrary to the byte-parity requirement in the comment above. Keep the closing marker's trailing line ending when stripping frontmatter.

                    processed_body = "\n".join(body_lines[body_close + 1 :])

• Files reviewed: 2/2 changed files
• Comments generated: 5
• Review effort level: Medium

[Copilot]

Review details

Suppressed comments (3)

src/specify_cli/integrations/base.py:39

• The PR is described as test-only, but this production-file diff rolls back unrelated behavior already on main: event APIs/install hooks are removed, native $//skill: invocation prefixes are removed, and the line-ending-preserving frontmatter parser is reverted. This is immediately breaking: agents.py:787 and shared_infra.py:369 still call resolve_command_refs with three arguments, while integrations/copilot/__init__.py:385 still calls emit_events. Please restore base.py to main and keep this PR limited to the intended regression tests.

    "- When constructing slash commands from hook command names, "
    "replace dots (`.`) with hyphens (`-`). "
    "For example, `speckit.git.commit` → `/speckit-git-commit`.\n"

tests/integrations/test_integration_base_skills.py:203

• This second inherited setup call has the same Hermes side effect: it writes the generated skill into the real ~/.hermes/skills directory because Path.home() is not redirected. Keep the regression test hermetic before invoking setup.

        m = IntegrationManifest(self.KEY, tmp_path)
        created = i.setup(tmp_path, m)

tests/integrations/test_integration_base_skills.py:188

• quote_yaml_double does not exist in this codebase; the production helper added by #3399 is yaml_quote. Keeping the stale name makes the regression documentation point maintainers to the wrong implementation.

        SKILL.md unparseable / lossy. ``quote_yaml_double`` must escape it so the
        value survives byte-for-byte."""

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[mnriem]

Thank you!