Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions Sources/mcs/Export/ManifestBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -332,9 +332,9 @@ struct ManifestBuilder {
yaml.keyValue("schemaVersion", manifest.schemaVersion)
yaml.keyValue("identifier", manifest.identifier)
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
yaml.keyValue("identifier", manifest.identifier)
yaml.keyValue("identifier", manifest.identifier, quoted: true)

Copilot uses AI. Check for mistakes.
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 ────────────────────────────────────────────────────────
Expand Down Expand Up @@ -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))")
}
}

Expand All @@ -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))")
Expand Down Expand Up @@ -441,7 +441,7 @@ struct ManifestBuilder {
}

yaml.line(" - id: \(comp.id)")
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
yaml.line(" - id: \(comp.id)")
yaml.line(" - id: \(yamlQuote(comp.id))")

Copilot uses AI. Check for mistakes.
yaml.line(" description: \(comp.description)")
yaml.line(" description: \(yamlQuote(comp.description))")

// isRequired
if comp.isRequired == true {
Expand All @@ -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)
}
Expand All @@ -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:")
Expand Down Expand Up @@ -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:")
Expand Down Expand Up @@ -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
Expand Down
116 changes: 116 additions & 0 deletions Tests/MCSTests/ManifestBuilderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = []
) 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("#"))
}
}