From 552ebceb0b9ddb894a3e06334a1afa1684ddd689 Mon Sep 17 00:00:00 2001 From: Kieran Klaassen Date: Sat, 14 Feb 2026 20:46:53 -0800 Subject: [PATCH] chore: mark review todos as complete Co-Authored-By: Claude Opus 4.6 --- ...001-complete-p2-extract-gemini-mcp-type.md | 25 +++++++++++++++++++ .../002-complete-p2-test-uniquename-dedup.md | 24 ++++++++++++++++++ .../003-complete-p2-test-toml-triple-quote.md | 24 ++++++++++++++++++ .../004-complete-p2-deep-merge-mcp-servers.md | 25 +++++++++++++++++++ ...-complete-p2-warn-invalid-settings-json.md | 23 +++++++++++++++++ 5 files changed, 121 insertions(+) create mode 100644 todos/001-complete-p2-extract-gemini-mcp-type.md create mode 100644 todos/002-complete-p2-test-uniquename-dedup.md create mode 100644 todos/003-complete-p2-test-toml-triple-quote.md create mode 100644 todos/004-complete-p2-deep-merge-mcp-servers.md create mode 100644 todos/005-complete-p2-warn-invalid-settings-json.md diff --git a/todos/001-complete-p2-extract-gemini-mcp-type.md b/todos/001-complete-p2-extract-gemini-mcp-type.md new file mode 100644 index 0000000..de84fc7 --- /dev/null +++ b/todos/001-complete-p2-extract-gemini-mcp-type.md @@ -0,0 +1,25 @@ +--- +status: pending +priority: p2 +issue_id: "001" +tags: [code-review, typescript, types] +dependencies: [] +--- + +# Extract GeminiMcpServer as named type + +## Problem Statement +The `GeminiBundle` inlines the MCP server type definition, and the converter uses `NonNullable[string]` which is hard to read. Other targets (Cursor) define a named type. + +## Findings +- `src/types/gemini.ts` lines 20-26: inline type in GeminiBundle +- `src/converters/claude-to-gemini.ts` line 117: `NonNullable[string]` + +## Proposed Solution +Extract a named `GeminiMcpServer` type in `src/types/gemini.ts` and use it in both the bundle type and converter. + +## Acceptance Criteria +- [ ] `GeminiMcpServer` type exists in `src/types/gemini.ts` +- [ ] `GeminiBundle.mcpServers` uses `Record` +- [ ] Converter uses `GeminiMcpServer` instead of indexed access type +- [ ] Tests still pass diff --git a/todos/002-complete-p2-test-uniquename-dedup.md b/todos/002-complete-p2-test-uniquename-dedup.md new file mode 100644 index 0000000..9446179 --- /dev/null +++ b/todos/002-complete-p2-test-uniquename-dedup.md @@ -0,0 +1,24 @@ +--- +status: pending +priority: p2 +issue_id: "002" +tags: [code-review, testing] +dependencies: [] +--- + +# Add test for uniqueName dedup when agent collides with skill + +## Problem Statement +The `uniqueName` function handles name collisions by appending `-2`, but there is no test covering the scenario where an agent name collides with a pass-through skill name. + +## Findings +- `src/converters/claude-to-gemini.ts` lines 181-193: uniqueName function +- `tests/gemini-converter.test.ts`: no dedup test + +## Proposed Solution +Add a test where a plugin has both a skill named "security-reviewer" and an agent named "Security Reviewer". The generated skill should get name "security-reviewer-2". + +## Acceptance Criteria +- [ ] Test added for agent/skill name collision +- [ ] Test verifies the deduped name is `security-reviewer-2` +- [ ] All tests pass diff --git a/todos/003-complete-p2-test-toml-triple-quote.md b/todos/003-complete-p2-test-toml-triple-quote.md new file mode 100644 index 0000000..4ebb2f4 --- /dev/null +++ b/todos/003-complete-p2-test-toml-triple-quote.md @@ -0,0 +1,24 @@ +--- +status: pending +priority: p2 +issue_id: "003" +tags: [code-review, testing, security] +dependencies: [] +--- + +# Add test for TOML triple-quote escaping in prompt + +## Problem Statement +The `toToml` function escapes `"""` in prompts, but there is no test verifying this works correctly. This is a potential TOML injection vector. + +## Findings +- `src/converters/claude-to-gemini.ts` line 150: `prompt.replace(/"""/g, '\\"\\"\\"')` +- `tests/gemini-converter.test.ts`: no triple-quote test in `toToml` describe block + +## Proposed Solution +Add a test in the `toToml` describe block that passes a prompt containing `"""` and verifies the output escapes it correctly. + +## Acceptance Criteria +- [ ] Test added for prompt containing `"""` +- [ ] Escaped output does not prematurely close the TOML multi-line string +- [ ] All tests pass diff --git a/todos/004-complete-p2-deep-merge-mcp-servers.md b/todos/004-complete-p2-deep-merge-mcp-servers.md new file mode 100644 index 0000000..4be7602 --- /dev/null +++ b/todos/004-complete-p2-deep-merge-mcp-servers.md @@ -0,0 +1,25 @@ +--- +status: pending +priority: p2 +issue_id: "004" +tags: [code-review, security, data-loss] +dependencies: [] +--- + +# Deep-merge mcpServers in settings.json instead of replacing + +## Problem Statement +The Gemini writer replaces the entire `mcpServers` key in `settings.json`, silently destroying any existing user MCP servers. The test name says "merges" but it actually replaces. + +## Findings +- `src/targets/gemini.ts` line 44: `{ ...existingSettings, mcpServers: bundle.mcpServers }` +- `tests/gemini-writer.test.ts` line 150: test name says "merges" but asserts replacement + +## Proposed Solution +Deep-merge `mcpServers` entries: `{ ...existingMcp, ...bundle.mcpServers }`. Update the test to verify existing servers are preserved alongside new ones. + +## Acceptance Criteria +- [ ] Existing mcpServers entries are preserved when new ones are added +- [ ] New entries with same name override existing (not merged at field level) +- [ ] Test verifies both old and new servers exist after merge +- [ ] All tests pass diff --git a/todos/005-complete-p2-warn-invalid-settings-json.md b/todos/005-complete-p2-warn-invalid-settings-json.md new file mode 100644 index 0000000..2b2d3cd --- /dev/null +++ b/todos/005-complete-p2-warn-invalid-settings-json.md @@ -0,0 +1,23 @@ +--- +status: pending +priority: p2 +issue_id: "005" +tags: [code-review, error-handling] +dependencies: ["004"] +--- + +# Warn when existing settings.json is invalid JSON + +## Problem Statement +When an existing `settings.json` cannot be parsed, the error is silently swallowed and the file is overwritten. Users get no warning that their settings were discarded. + +## Findings +- `src/targets/gemini.ts` lines 37-41: empty catch block + +## Proposed Solution +Add a `console.warn` in the catch block to inform the user that their existing settings.json could not be parsed and will be replaced. + +## Acceptance Criteria +- [ ] `console.warn` emitted when settings.json parse fails +- [ ] File is still replaced (behavior unchanged) +- [ ] All tests pass