[#182] Fix YAML quoting gaps in ManifestBuilder export#190
Conversation
bguidolim
force-pushed
the
feature/182-yaml-quoting-gaps
branch
from
ed38378 to
fd8c067
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens ManifestBuilder’s YAML serialization so user-controlled strings can’t accidentally generate invalid techpack.yaml, improving reliability of mcs pack add and related manifest loading/validation.
Changes:
- Extended
yamlQuote()to detect and escape newline/tab characters. - Applied quoting to additional high-risk manifest fields (metadata, templates, prompts, MCP config, file paths, settingsFile).
- Added 4 round-trip tests covering special characters, control characters, URL fragments, and component descriptions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Sources/mcs/Export/ManifestBuilder.swift | Expands YAML quoting/escaping and applies it across more emitted manifest fields. |
| Tests/MCSTests/ManifestBuilderTests.swift | Adds new round-trip tests validating YAML quoting edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -441,7 +441,7 @@ struct ManifestBuilder { | |||
| } | |||
|
|
|||
| yaml.line(" - id: \(comp.id)") | |||
There was a problem hiding this comment.
Component id is emitted without YAML quoting/escaping. For MCP server components the id is built from the discovered server name (mcp-\(server.name)), which can contain user-controlled characters like : , #, or tabs/newlines; those can make the YAML invalid even though description is now quoted. Consider applying yamlQuote to comp.id during rendering (or sanitizing the MCP server name when forming the id) to close this remaining quoting gap.
| yaml.line(" - id: \(comp.id)") | |
| yaml.line(" - id: \(yamlQuote(comp.id))") |
| @@ -332,9 +332,9 @@ struct ManifestBuilder { | |||
| yaml.keyValue("schemaVersion", manifest.schemaVersion) | |||
| yaml.keyValue("identifier", manifest.identifier) | |||
There was a problem hiding this comment.
identifier is user-provided (ExportCommand prompts for it) and is still emitted without YAML quoting/escaping. If the user enters values containing YAML-significant sequences (e.g. : , #, leading &, etc.), the generated techpack.yaml can still become invalid YAML. Consider passing quoted: true here (or running it through yamlQuote) so parsing succeeds and schema validation can fail with a clearer error instead of a YAML decode failure.
| yaml.keyValue("identifier", manifest.identifier) | |
| yaml.keyValue("identifier", manifest.identifier, quoted: true) |
- Add \n and \t detection and escaping to yamlQuote() - Quote metadata description, author, component descriptions, URLs, and hookEvent - Sanitize MCP server names in component IDs via sanitizeID() - Add 4 round-trip tests for special characters, newlines, and URL fragments
bguidolim
force-pushed
the
feature/182-yaml-quoting-gaps
branch
from
fd8c067 to
c907522
Compare
Closes #182
Context
ManifestBuilderemits several string values into generatedtechpack.yamlwithout YAML quoting. User-controlled strings containing#,:,&, newlines, or tabs produce invalid YAML that silently breaks onmcs pack add.Changes
yamlQuote(): detect and escape\nand\tcontrol charactersdescriptionandauthor(viaquoted: true), componentdescriptionand MCPurl(viayamlQuote())sectionIdentifier,contentFile, promptkey, MCPcommand,source/destination, andsettingsFilesanitizeID()to server names at construction time#Test plan
swift test --filter MCSTests.ManifestBuilderTests— 7/7 pass (3 existing + 4 new)swift test— 585/585 pass, zero regressions