fix: accept mcpServers as alias for mcp in config#621
fix: accept mcpServers as alias for mcp in config#621VJ-yadav wants to merge 5 commits intoAltimateAI:mainfrom
Conversation
When AI tools (Claude Code, Copilot) generate altimate-code config, they write `mcpServers` instead of `mcp`. This causes a startup error: "Unrecognized key: mcpServers". Add normalization during config loading that renames `mcpServers` to `mcp` before schema validation, matching the existing pattern for legacy key migration (theme, keybinds, tui). Fixes AltimateAI#619 Co-Authored-By: Vijay Yadav <vijay@studentsucceed.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughConfig normalization now supports Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
cc @VJ-yadav |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/config/config.ts`:
- Around line 1436-1441: The duplicate top-level aliasing block that maps
copy.mcpServers → copy.mcp (checking "mcpServers" in copy && !("mcp" in copy")
and logging) should be removed so all MCP normalization is performed by
normalizeMcpConfig(); delete that if-block (the code referencing
copy.mcpServers, copy.mcp and the log.info call) and rely on
normalizeMcpConfig() to handle both top-level aliasing and per-server
normalization to avoid dead code and duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c77c8e1-75d0-4a66-8ecb-c1475cd1272b
📒 Files selected for processing (1)
packages/opencode/src/config/config.ts
| // Alias mcpServers → mcp (Claude Code / Copilot format compatibility) | ||
| if ("mcpServers" in copy && !("mcp" in copy)) { | ||
| copy.mcp = copy.mcpServers | ||
| delete copy.mcpServers | ||
| log.info("aliased mcpServers to mcp in config", { path: source }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate normalization logic.
This code duplicates the mcpServers → mcp aliasing already handled by normalizeMcpConfig() (lines 1375-1383), which is called immediately after at line 1450. Since this code executes first, the normalization logic inside normalizeMcpConfig becomes unreachable for the top-level key, creating dead code and maintenance confusion.
The normalizeMcpConfig function is more comprehensive—it handles both the top-level key aliasing and individual server entry normalization. Consolidating all MCP normalization in one place improves maintainability and follows the DRY principle.
♻️ Proposed fix: Remove duplicate normalization
const normalized = (() => {
if (!data || typeof data !== "object" || Array.isArray(data)) return data
const copy = { ...(data as Record<string, unknown>) }
- // Alias mcpServers → mcp (Claude Code / Copilot format compatibility)
- if ("mcpServers" in copy && !("mcp" in copy)) {
- copy.mcp = copy.mcpServers
- delete copy.mcpServers
- log.info("aliased mcpServers to mcp in config", { path: source })
- }
const hadLegacy = "theme" in copy || "keybinds" in copy || "tui" in copy
if (hadLegacy) {
delete copy.theme
delete copy.keybinds
delete copy.tui
log.warn("tui keys in opencode config are deprecated; move them to tui.json", { path: source })
}
// altimate_change start — normalize mcpServers to mcp (common key used by other AI tools)
return normalizeMcpConfig(copy, source)
// altimate_change end
})()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/config/config.ts` around lines 1436 - 1441, The
duplicate top-level aliasing block that maps copy.mcpServers → copy.mcp
(checking "mcpServers" in copy && !("mcp" in copy") and logging) should be
removed so all MCP normalization is performed by normalizeMcpConfig(); delete
that if-block (the code referencing copy.mcpServers, copy.mcp and the log.info
call) and rely on normalizeMcpConfig() to handle both top-level aliasing and
per-server normalization to avoid dead code and duplication.
Multi-Model Code Review — Unanimous: CLOSE AS SUPERSEDEDRan a 3-reviewer consensus pass (Claude + GPT 5.4 + Gemini 3.1 Pro). All three independently reached the same verdict: this PR should be closed without merging. Root finding: dead code, superseded by #639The 6 lines added here duplicate logic that already lives in Verified locally: reverted the PR's 6 lines and ran the full config test suite → 82/82 pass. The tests in this PR ( Additional issues (if this were the only fix)
Positive observations
RecommendationClosing as superseded. Thanks @VJ-yadav for surfacing #619 — the compatibility gap you identified was legitimate and got fully fixed by #639. The comprehensive test suite you wrote here would be a valuable contribution if added to `test/config/config.test.ts` — want to open a separate PR with just the test cases not already covered by #639's tests? Reviewed by 3 participants via consensus code review. Convergence: 0 rounds (unanimous). |
|
Closing as superseded by #639. See review comment above for details. |
Thanks @anandgupta42 for the detailed review and the multi-model consensus pass, really appreciate the thoroughness. Glad the compatibility gap I reported in #619 got a proper fix in #639, the shape translation and conflict handling there are much more robust than my simple alias approach. To clarify, my PR only had the config alias in config.ts, no test file changes. The comprehensive test suite at config.test.ts:346-490 came with #639. So I don't have additional test cases from my end to port over, but I can review #639's coverage and open a test PR if I spot any gaps worth adding. |
3 participants
What does this PR do?
Fixes #619 — When AI tools (Claude Code, Copilot) auto-generate
altimate-code.json, they writemcpServersinstead ofmcp, causing a startup error:Unrecognized key: "mcpServers".Added normalization in config loading that renames
mcpServers→mcpbefore schema validation. Follows the existing pattern for legacy key migration (theme,keybinds,tui→ deleted with warning). Only applies whenmcpis not already set, so explicitmcpconfigs are not overwritten.One file changed, 6 lines added.
Type of change
Issue for this PR
Closes #619
How did you verify your code works?
bun test test/config/config.test.ts— 66 passconfig.tsmcpkey if both presentChecklist
config.tsmcpkey if both presentSummary by CodeRabbit
mcpServersas an alias for themcpkey. WhenmcpServersis present withoutmcp, the system automatically migrates the configuration and logs the change for transparency.