From c9075226353d90460c09b60b5c8636198dac9f58 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Mon, 2 Mar 2026 00:13:30 +0100 Subject: [PATCH] Fix YAML quoting gaps in ManifestBuilder export output (#182) - 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 --- Sources/mcs/Export/ManifestBuilder.swift | 30 +++--- Tests/MCSTests/ManifestBuilderTests.swift | 116 ++++++++++++++++++++++ 2 files changed, 133 insertions(+), 13 deletions(-) diff --git a/Sources/mcs/Export/ManifestBuilder.swift b/Sources/mcs/Export/ManifestBuilder.swift index b0bd458..13b0116 100644 --- a/Sources/mcs/Export/ManifestBuilder.swift +++ b/Sources/mcs/Export/ManifestBuilder.swift @@ -120,7 +120,7 @@ struct ManifestBuilder { // ── MCP Servers ─────────────────────────────────────────────────────── for server in config.mcpServers where options.selectedMCPServers.contains(server.name) { - let id = "mcp-\(server.name)" + let id = "mcp-\(sanitizeID(server.name))" // Detect brew dependency hint via Homebrew symlink resolution if let command = server.command, let formula = Homebrew.detectFormula(for: command) { @@ -332,9 +332,9 @@ struct ManifestBuilder { yaml.keyValue("schemaVersion", manifest.schemaVersion) yaml.keyValue("identifier", manifest.identifier) yaml.keyValue("displayName", manifest.displayName, quoted: true) - yaml.keyValue("description", manifest.description) + yaml.keyValue("description", manifest.description, quoted: true) if let author = manifest.author { - yaml.keyValue("author", author) + yaml.keyValue("author", author, quoted: true) } // ── Components ──────────────────────────────────────────────────────── @@ -370,8 +370,8 @@ struct ManifestBuilder { yaml.sectionDivider("Templates — CLAUDE.local.md sections") yaml.line("templates:") for template in templates { - yaml.line(" - sectionIdentifier: \(template.sectionIdentifier)") - yaml.line(" contentFile: \(template.contentFile)") + yaml.line(" - sectionIdentifier: \(yamlQuote(template.sectionIdentifier))") + yaml.line(" contentFile: \(yamlQuote(template.contentFile))") } } @@ -381,7 +381,7 @@ struct ManifestBuilder { yaml.sectionDivider("Prompts — resolved interactively during `mcs sync`") yaml.line("prompts:") for prompt in prompts { - yaml.line(" - key: \(prompt.key)") + yaml.line(" - key: \(yamlQuote(prompt.key))") yaml.line(" type: \(prompt.type.rawValue)") if let label = prompt.label { yaml.line(" label: \(yamlQuote(label))") @@ -441,7 +441,7 @@ struct ManifestBuilder { } yaml.line(" - id: \(comp.id)") - yaml.line(" description: \(comp.description)") + yaml.line(" description: \(yamlQuote(comp.description))") // isRequired if comp.isRequired == true { @@ -450,7 +450,7 @@ struct ManifestBuilder { // hookEvent if let hookEvent = comp.hookEvent { - yaml.line(" hookEvent: \(hookEvent)") + yaml.line(" hookEvent: \(yamlQuote(hookEvent))") } else if comp.type == .hookFile { yaml.comment(" TODO: Add hookEvent (e.g. SessionStart, PreToolUse, Stop)", indent: 4) } @@ -460,10 +460,10 @@ struct ManifestBuilder { case .mcpServer(let config): yaml.line(" mcp:") if config.transport == .http, let url = config.url { - yaml.line(" url: \(url)") + yaml.line(" url: \(yamlQuote(url))") } else { if let command = config.command { - yaml.line(" command: \(command)") + yaml.line(" command: \(yamlQuote(command))") } if let args = config.args, !args.isEmpty { yaml.line(" args:") @@ -498,11 +498,11 @@ struct ManifestBuilder { preconditionFailure("Export does not produce .generic file components") } yaml.line(" \(key):") - yaml.line(" source: \(config.source)") - yaml.line(" destination: \(config.destination)") + yaml.line(" source: \(yamlQuote(config.source))") + yaml.line(" destination: \(yamlQuote(config.destination))") case .settingsFile(let source): - yaml.line(" settingsFile: \(source)") + yaml.line(" settingsFile: \(yamlQuote(source))") case .gitignoreEntries(let entries): yaml.line(" gitignore:") @@ -568,11 +568,15 @@ private func yamlQuote(_ value: String) -> String { || value.contains("#") || value.contains("\"") || value.contains("'") + || value.contains("\n") + || value.contains("\t") || ["true", "false", "yes", "no", "null", "~"].contains(value.lowercased()) if needsQuoting { let escaped = value.replacingOccurrences(of: "\\", with: "\\\\") .replacingOccurrences(of: "\"", with: "\\\"") + .replacingOccurrences(of: "\n", with: "\\n") + .replacingOccurrences(of: "\t", with: "\\t") return "\"\(escaped)\"" } return value diff --git a/Tests/MCSTests/ManifestBuilderTests.swift b/Tests/MCSTests/ManifestBuilderTests.swift index f652ae8..03a2c2e 100644 --- a/Tests/MCSTests/ManifestBuilderTests.swift +++ b/Tests/MCSTests/ManifestBuilderTests.swift @@ -315,4 +315,120 @@ struct ManifestBuilderTests { #expect(prompts.count == 1) #expect(prompts[0].key == "TOKEN") } + + // MARK: - YAML quoting edge cases + + private func buildAndLoadRoundTrip( + config: ConfigurationDiscovery.DiscoveredConfiguration = .init(), + metadata: ManifestBuilder.Metadata, + selectedMCPServers: Set = [] + ) throws -> (result: ManifestBuilder.BuildResult, loaded: ExternalPackManifest) { + let tmpDir = try makeTmpDir() + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let result = ManifestBuilder().build( + from: config, metadata: metadata, + selectedMCPServers: selectedMCPServers, + selectedHookFiles: [], selectedSkillFiles: [], + selectedCommandFiles: [], selectedPlugins: [], selectedSections: [], + includeUserContent: false, includeGitignore: false, includeSettings: false + ) + + let yamlFile = tmpDir.appendingPathComponent("techpack.yaml") + try result.manifestYAML.write(to: yamlFile, atomically: true, encoding: .utf8) + let loaded = try ExternalPackManifest.load(from: yamlFile) + return (result, loaded) + } + + @Test("Special characters in metadata survive YAML round-trip") + func specialCharactersInMetadataRoundTrip() throws { + let metadata = ManifestBuilder.Metadata( + identifier: "special-chars", + displayName: "Special Pack", + description: "Contains # hash and key: value pair", + author: "&anchor-like author" + ) + + let (result, loaded) = try buildAndLoadRoundTrip(metadata: metadata) + + #expect(result.manifestYAML.contains("description: \"Contains # hash and key: value pair\"")) + #expect(result.manifestYAML.contains("author: \"&anchor-like author\"")) + #expect(loaded.description == "Contains # hash and key: value pair") + #expect(loaded.author == "&anchor-like author") + } + + @Test("Newlines and tabs in description are properly escaped") + func newlinesAndTabsRoundTrip() throws { + let metadata = ManifestBuilder.Metadata( + identifier: "newline-pack", + displayName: "Newline Pack", + description: "Line one\nLine two\tTabbed", + author: nil + ) + + let (result, loaded) = try buildAndLoadRoundTrip(metadata: metadata) + + #expect(result.manifestYAML.contains(#"description: "Line one\nLine two\tTabbed""#)) + #expect(loaded.description == "Line one\nLine two\tTabbed") + } + + @Test("HTTP MCP server URL with fragment survives round-trip") + func httpURLWithFragmentRoundTrip() throws { + var config = ConfigurationDiscovery.DiscoveredConfiguration() + config.mcpServers = [ + ConfigurationDiscovery.DiscoveredMCPServer( + name: "fragmented", command: nil, args: [], + env: [:], + url: "https://example.com/mcp#section", scope: "local" + ), + ] + + let metadata = ManifestBuilder.Metadata( + identifier: "url-test", + displayName: "URL Test", + description: "Tests URL quoting", + author: nil + ) + + let (_, loaded) = try buildAndLoadRoundTrip( + config: config, metadata: metadata, selectedMCPServers: ["fragmented"] + ) + let normalized = try loaded.normalized() + + let mcpComp = try #require(normalized.components?.first { $0.type == .mcpServer }) + guard case .mcpServer(let mcpConfig) = mcpComp.installAction else { + Issue.record("Expected mcpServer install action") + return + } + #expect(mcpConfig.url == "https://example.com/mcp#section") + } + + @Test("Component description with special characters survives round-trip") + func componentDescriptionSpecialCharsRoundTrip() throws { + var config = ConfigurationDiscovery.DiscoveredConfiguration() + config.mcpServers = [ + ConfigurationDiscovery.DiscoveredMCPServer( + name: "server#v2", command: "npx", args: ["test"], + env: [:], + url: nil, scope: "local" + ), + ] + + let metadata = ManifestBuilder.Metadata( + identifier: "desc-test", + displayName: "Desc Test", + description: "Tests component descriptions", + author: nil + ) + + let (_, loaded) = try buildAndLoadRoundTrip( + config: config, metadata: metadata, selectedMCPServers: ["server#v2"] + ) + let normalized = try loaded.normalized() + + let mcpComp = try #require(normalized.components?.first { $0.type == .mcpServer }) + #expect(mcpComp.description.contains("server#v2")) + #expect(mcpComp.id.contains("mcp-serverv2")) + #expect(!mcpComp.id.contains("#")) + } }