fix(opencode): preserve config when updating MCP servers#1623
Conversation
|
@jasperan is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors MCP service to perform an atomic read/parse phase across agents before any writes, accumulates per-agent pending writes, delays error throwing until after all agents are processed, and adds strict JSONC parsing and preserving JSONC-aware read/write behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
![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
🧹 Nitpick comments (1)
src/main/services/mcp/configIO.ts (1)
111-124: Consider adding a brief comment explaining the validation-only call.Line 113 calls
parseJsoncConfigbut discards its return value. While this is correct (it's validating before applying edits to preserve comments), a brief comment would clarify the intent.📝 Suggested clarification
if (isJsoncConfig(meta)) { if (existingRaw && existingRaw.trim()) { + // Validate existing content before modifying; throws if malformed parseJsoncConfig(meta, existingRaw); const edits = jsoncParser.modify(existingRaw, meta.serversPath, servers, {}); const modified = jsoncParser.applyEdits(existingRaw, edits);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/mcp/configIO.ts` around lines 111 - 124, The call to parseJsoncConfig(meta, existingRaw) in the isJsoncConfig branch is used solely to validate the existing JSONC before generating edits (so comments are preserved) but the return value is intentionally discarded; add a short inline comment above that call explaining it performs validation/parse for side-effects only (e.g., "validate/parse JSONC to ensure it's safe to compute edits; return value intentionally ignored") so future readers know why parseJsoncConfig is invoked even though its result isn't used, referencing parseJsoncConfig, jsoncParser.modify, and jsoncParser.applyEdits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/McpService.ts`:
- Around line 130-145: Update the documentation agents/integrations/mcp.md to
state explicitly that the read phase is atomic (any read error aborts the
operation) and the write phase is best-effort (writeServers is called for each
entry in pendingWrites and the operation continues on individual failures). In
code, enhance the post-write error reporting where writeFailures, writeServers,
pendingWrites, readFailures and buildConfigFailureError are used: after the
for-loop collect both failed agentIds (writeFailures) and successful agentIds
(those from pendingWrites not in writeFailures) and include both lists in the
thrown buildConfigFailureError (or in a new more descriptive error payload) so
the error message clearly shows which agents succeeded and which failed.
---
Nitpick comments:
In `@src/main/services/mcp/configIO.ts`:
- Around line 111-124: The call to parseJsoncConfig(meta, existingRaw) in the
isJsoncConfig branch is used solely to validate the existing JSONC before
generating edits (so comments are preserved) but the return value is
intentionally discarded; add a short inline comment above that call explaining
it performs validation/parse for side-effects only (e.g., "validate/parse JSONC
to ensure it's safe to compute edits; return value intentionally ignored") so
future readers know why parseJsoncConfig is invoked even though its result isn't
used, referencing parseJsoncConfig, jsoncParser.modify, and
jsoncParser.applyEdits.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8d15c27-7250-48d7-b4cf-848e7daca84f
📒 Files selected for processing (7)
src/main/services/McpService.tssrc/main/services/mcp/configIO.tssrc/main/services/mcp/configPaths.tssrc/shared/mcp/types.tssrc/test/main/mcp/McpService.test.tssrc/test/main/mcp/configIO.test.tssrc/test/main/mcp/configPaths.test.ts
|
Thank you for taking this on and for fixing this. @jasperan |
2 participants
Summary
opencode.jsonas JSONC-capable even though the filename ends in.jsonFixes #1616
Test Plan
pnpm exec vitest run src/test/main/mcp/configPaths.test.ts src/test/main/mcp/configIO.test.ts src/test/main/mcp/McpService.test.tspnpm run type-checkSummary by CodeRabbit
New Features
Improvements
Tests
Documentation