fix: address code review findings for gemini target
- Extract named GeminiMcpServer type (eliminates NonNullable indexing) - Deep-merge mcpServers in settings.json (preserves existing entries) - Warn when existing settings.json cannot be parsed - Add test for uniqueName dedup (agent/skill name collision) - Add test for TOML triple-quote escaping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
import { formatFrontmatter } from "../utils/frontmatter"
|
import { formatFrontmatter } from "../utils/frontmatter"
|
||||||
import type { ClaudeAgent, ClaudeCommand, ClaudeMcpServer, ClaudePlugin } from "../types/claude"
|
import type { ClaudeAgent, ClaudeCommand, ClaudeMcpServer, ClaudePlugin } from "../types/claude"
|
||||||
import type { GeminiBundle, GeminiCommand, GeminiSkill } from "../types/gemini"
|
import type { GeminiBundle, GeminiCommand, GeminiMcpServer, GeminiSkill } from "../types/gemini"
|
||||||
import type { ClaudeToOpenCodeOptions } from "./claude-to-opencode"
|
import type { ClaudeToOpenCodeOptions } from "./claude-to-opencode"
|
||||||
|
|
||||||
export type ClaudeToGeminiOptions = ClaudeToOpenCodeOptions
|
export type ClaudeToGeminiOptions = ClaudeToOpenCodeOptions
|
||||||
@@ -109,12 +109,12 @@ export function transformContentForGemini(body: string): string {
|
|||||||
|
|
||||||
function convertMcpServers(
|
function convertMcpServers(
|
||||||
servers?: Record<string, ClaudeMcpServer>,
|
servers?: Record<string, ClaudeMcpServer>,
|
||||||
): GeminiBundle["mcpServers"] | undefined {
|
): Record<string, GeminiMcpServer> | undefined {
|
||||||
if (!servers || Object.keys(servers).length === 0) return undefined
|
if (!servers || Object.keys(servers).length === 0) return undefined
|
||||||
|
|
||||||
const result: NonNullable<GeminiBundle["mcpServers"]> = {}
|
const result: Record<string, GeminiMcpServer> = {}
|
||||||
for (const [name, server] of Object.entries(servers)) {
|
for (const [name, server] of Object.entries(servers)) {
|
||||||
const entry: NonNullable<GeminiBundle["mcpServers"]>[string] = {}
|
const entry: GeminiMcpServer = {}
|
||||||
if (server.command) {
|
if (server.command) {
|
||||||
entry.command = server.command
|
entry.command = server.command
|
||||||
if (server.args && server.args.length > 0) entry.args = server.args
|
if (server.args && server.args.length > 0) entry.args = server.args
|
||||||
|
|||||||
@@ -37,11 +37,14 @@ export async function writeGeminiBundle(outputRoot: string, bundle: GeminiBundle
|
|||||||
try {
|
try {
|
||||||
existingSettings = await readJson<Record<string, unknown>>(settingsPath)
|
existingSettings = await readJson<Record<string, unknown>>(settingsPath)
|
||||||
} catch {
|
} catch {
|
||||||
// If existing file is invalid JSON, start fresh
|
console.warn("Warning: existing settings.json could not be parsed and will be replaced.")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
const merged = { ...existingSettings, mcpServers: bundle.mcpServers }
|
const existingMcp = (existingSettings.mcpServers && typeof existingSettings.mcpServers === "object")
|
||||||
|
? existingSettings.mcpServers as Record<string, unknown>
|
||||||
|
: {}
|
||||||
|
const merged = { ...existingSettings, mcpServers: { ...existingMcp, ...bundle.mcpServers } }
|
||||||
await writeJson(settingsPath, merged)
|
await writeJson(settingsPath, merged)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,15 +13,17 @@ export type GeminiCommand = {
|
|||||||
content: string // Full TOML content
|
content: string // Full TOML content
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export type GeminiMcpServer = {
|
||||||
|
command?: string
|
||||||
|
args?: string[]
|
||||||
|
env?: Record<string, string>
|
||||||
|
url?: string
|
||||||
|
headers?: Record<string, string>
|
||||||
|
}
|
||||||
|
|
||||||
export type GeminiBundle = {
|
export type GeminiBundle = {
|
||||||
generatedSkills: GeminiSkill[] // From agents
|
generatedSkills: GeminiSkill[] // From agents
|
||||||
skillDirs: GeminiSkillDir[] // From skills (pass-through)
|
skillDirs: GeminiSkillDir[] // From skills (pass-through)
|
||||||
commands: GeminiCommand[]
|
commands: GeminiCommand[]
|
||||||
mcpServers?: Record<string, {
|
mcpServers?: Record<string, GeminiMcpServer>
|
||||||
command?: string
|
|
||||||
args?: string[]
|
|
||||||
env?: Record<string, string>
|
|
||||||
url?: string
|
|
||||||
headers?: Record<string, string>
|
|
||||||
}>
|
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -267,6 +267,25 @@ describe("convertClaudeToGemini", () => {
|
|||||||
expect(bundle.commands).toHaveLength(0)
|
expect(bundle.commands).toHaveLength(0)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("agent name colliding with skill name gets deduplicated", () => {
|
||||||
|
const plugin: ClaudePlugin = {
|
||||||
|
...fixturePlugin,
|
||||||
|
skills: [{ name: "security-reviewer", description: "Existing skill", sourceDir: "/tmp/skill", skillPath: "/tmp/skill/SKILL.md" }],
|
||||||
|
agents: [{ name: "Security Reviewer", description: "Agent version", body: "Body.", sourcePath: "/tmp/agents/sr.md" }],
|
||||||
|
commands: [],
|
||||||
|
}
|
||||||
|
|
||||||
|
const bundle = convertClaudeToGemini(plugin, {
|
||||||
|
agentMode: "subagent",
|
||||||
|
inferTemperature: false,
|
||||||
|
permissions: "none",
|
||||||
|
})
|
||||||
|
|
||||||
|
// Agent should be deduplicated since skill already has "security-reviewer"
|
||||||
|
expect(bundle.generatedSkills[0].name).toBe("security-reviewer-2")
|
||||||
|
expect(bundle.skillDirs[0].name).toBe("security-reviewer")
|
||||||
|
})
|
||||||
|
|
||||||
test("hooks present emits console.warn", () => {
|
test("hooks present emits console.warn", () => {
|
||||||
const warnings: string[] = []
|
const warnings: string[] = []
|
||||||
const originalWarn = console.warn
|
const originalWarn = console.warn
|
||||||
@@ -339,4 +358,16 @@ describe("toToml", () => {
|
|||||||
const result = toToml('Say "hello"', "Prompt")
|
const result = toToml('Say "hello"', "Prompt")
|
||||||
expect(result).toContain('description = "Say \\"hello\\""')
|
expect(result).toContain('description = "Say \\"hello\\""')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("escapes triple quotes in prompt", () => {
|
||||||
|
const result = toToml("A command", 'Content with """ inside it')
|
||||||
|
// Should not contain an unescaped """ that would close the TOML multi-line string prematurely
|
||||||
|
// The prompt section should have the escaped version
|
||||||
|
expect(result).toContain('description = "A command"')
|
||||||
|
expect(result).toContain('prompt = """')
|
||||||
|
// The inner """ should be escaped
|
||||||
|
expect(result).not.toMatch(/""".*""".*"""/s) // Should not have 3 separate triple-quote sequences (open, content, close would make 3)
|
||||||
|
// Verify it contains the escaped form
|
||||||
|
expect(result).toContain('\\"\\"\\"')
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -173,7 +173,9 @@ describe("writeGeminiBundle", () => {
|
|||||||
const content = JSON.parse(await fs.readFile(settingsPath, "utf8"))
|
const content = JSON.parse(await fs.readFile(settingsPath, "utf8"))
|
||||||
// Should preserve existing model key
|
// Should preserve existing model key
|
||||||
expect(content.model).toBe("gemini-2.5-pro")
|
expect(content.model).toBe("gemini-2.5-pro")
|
||||||
// mcpServers should be replaced (not merged) with new content
|
// Should preserve existing MCP server
|
||||||
|
expect(content.mcpServers.old.command).toBe("old-cmd")
|
||||||
|
// Should add new MCP server
|
||||||
expect(content.mcpServers.newServer.command).toBe("new-cmd")
|
expect(content.mcpServers.newServer.command).toBe("new-cmd")
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user