fix(install): write opencode MCP config in official schema#530
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates OpenCode platform support to use the newer mcp schema and prefer opencode.jsonc/opencode.json, including merge behavior and a warning for legacy .opencode.json files.
Changes:
- Switch OpenCode config target from
.opencode.json+mcpServerstoopencode.jsonc/opencode.json+mcp. - Update OpenCode server entry format to
type: localand a list-basedcommand. - Add a warning when a legacy
.opencode.jsoncontainingcode-review-graphis detected; expand tests/docs accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_skills.py | Updates/extends tests to validate new OpenCode config paths, schema, merge behavior, and legacy warning output. |
| docs/USAGE.md | Documents OpenCode config file locations as opencode.json / opencode.jsonc. |
| code_review_graph/skills.py | Implements OpenCode config path selection, new entry format, legacy warning, and adjusts JSONC comment stripping. |
| | **Zed** | `.zed/settings.json` | | ||
| | **Continue** | `.continue/config.json` | | ||
| | **OpenCode** | `.opencode.json` | | ||
| | **OpenCode** | `opencode.jsonc` (existing) → `opencode.json` (else create `opencode.jsonc`) | |
| # for editors like Zed that allow non-standard JSON). | ||
| stripped = re.sub(r'//.*?$', '', raw, flags=re.MULTILINE) | ||
| stripped = _strip_jsonc_comments(raw) | ||
| stripped = re.sub(r',(\s*[}\]])', r'\1', stripped) |
|
Friendly ping, @tirth8205 👋 Just flagging this is still on the table — issue #388 has 3+ users (PhilYang09, dragonabyss, Dhruv-Garg79) confirming Summary:
Happy to iterate on review feedback or split into smaller PRs if that helps. If you're not actively maintaining right now, no worries — at minimum the diff here gives anyone hitting #388 a known-good reference. Just let me know either way so I can stop pinging 🙂 |
|
Thanks @fdcp — and apologies for the slow response on this and #388. I applied the branch and verified it end to end: the schema target is right (confirmed against opencode.ai/docs/mcp-servers and the published config.json — opencode.json/.jsonc, top-level "mcp", type "local", array command), the legacy-hint-without-delete call is the right one, and _strip_jsonc_comments is a genuine fix over the old regex. Full suite: 1282 passed; mypy clean. Two changes to get this merged:
Agreed the trailing-comma regex is out of scope — please file that follow-up issue. Happy to merge once those land. |
code-review-graph install --platform opencode wrote
{repo_root}/.opencode.json with a Cursor-shaped mcpServers payload
(command + args + type stdio). OpenCode's documented schema is
different: project config lives in opencode.json, MCP servers go
under the mcp key, local servers use type local and a single
array-form command. The previous file was never loaded by OpenCode.
- config_path -> opencode.json, key -> mcp, needs_type -> False
- _build_server_entry emits McpLocalConfig (array command, type local)
for opencode and nothing else
- _warn_legacy_opencode_config prints a one-line hint when an old
.opencode.json with a code-review-graph entry is still present
(no auto-delete, safer than clobbering)
- test_install_opencode_config asserts the new schema; two new tests
cover the legacy-file warning (presence + absence)
- docs/USAGE.md updated to reflect the new path
Closes tirth8205#388.
…nt stripping - _opencode_config_path helper: PLATFORMS["opencode"].config_path and _warn_legacy_opencode_config now share one source of truth, so the warning's "new config is at ..." no longer disagrees with the actual write path (previously the warning said opencode.json while the lambda defaulted to opencode.jsonc). - _strip_jsonc_comments helper: stateful scanner that respects quoted strings. The earlier regex r'(^|\s)//.*$' still ate "//" inside string values when preceded by whitespace (e.g. "text": "\thello // world"), so it is replaced in install_platform_configs and _warn_legacy_opencode_config. - Tests: 3 new in TestJsoncHelpers (string preservation, full-line and inline stripping, _opencode_config_path precedence). 1274 passed.
…ync opencode docs - _strip_jsonc_comments now stops at either "\r" or "\n". The previous check (text[i] != "\n") silently consumed the "\r" in Windows CRLF line endings and would consume the entire file on old-Mac CR-only line endings. Found by Copilot re-review (thread on skills.py L286). - docs/USAGE.md: clarify the opencode config precedence (opencode.jsonc existing -> opencode.json existing -> else create opencode.jsonc) so the table matches _opencode_config_path. - Tests: 1 new in TestJsoncHelpers covering both \r\n and \r-only line endings. 1275 passed.
McpLocalConfig in opencode's schema is additionalProperties: false (only type/command/environment/enabled/timeout). Pass --repo to serve via the command array instead, which feeds _default_repo_root in main.py. Also fix E501 in test_skills.py:913 (103 > 100 chars).
85b26ca to
b7ec0ef
Compare
|
Both changes are pushed (commits rebased onto current
Verification:
Follow-up issue filed per your request: #553 (trailing-comma regex at One thing I can't do from a fork: the |
|
Follow-up: I re-ran the full verification on the rebased head after pushing
So the branch now has:
Still waiting on the fork-PR workflow approval gate in Checks ( |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Small follow-up pushed on top: Updated verification:
So the branch now has the requested install/schema fixes plus a small lint cleanup in the touched test file. |
|
Quick nudge, @tirth8205: the code side is done on head |
code-review-graph install --platform opencodewas writing{repo_root}/.opencode.jsonwith a Cursor-shapedmcpServerspayload (command+args+type: stdio). OpenCode's documented schema is different: project config lives inopencode.json/opencode.jsonc, MCP servers go under themcpkey, local servers usetype: localwith a single array-formcommand. The previous file was never loaded by OpenCode, soinstall --platform opencodewas effectively a no-op. Issue #388 (closed as fixed by PR #198 in 2026-05, but that PR only added a hook plugin — not the MCP config fix) was reopened in practice.Changes:
PLATFORMS["opencode"]:config_pathnow resolvesopencode.jsoncif it already exists, otherwiseopencode.json, otherwise createsopencode.jsonc(matches OpenCode's documented convention);key: mcp;needs_type: false_build_server_entryemits a single dict for OpenCode with array-formcommand([command, *args]) andtype: local; no separateargs/envarray. All other platforms unchanged_warn_legacy_opencode_configprints a one-line hint when an old.opencode.jsonwith acode-review-graphentry is still present (no auto-delete — safer than clobbering). The hint now derives the "new config is at …" path from the same_opencode_config_pathhelper used byconfig_path, so the message stays consistent with the actual write path_strip_jsonc_commentshelper — replaces the previous regex call ininstall_platform_configsand_warn_legacy_opencode_config. The earlierr'(^|\s)//.*$'regex still ate//sequences inside quoted strings when preceded by whitespace (e.g."text": "\thello // world"). The new helper is a small stateful scanner that tracks string boundaries, stops at either\ror\n(so CRLF and old-Mac CR-only files are handled), and only strips//outside quoted regions. Found and fixed while adding.jsoncsupporttest_install_opencode_configrewritten for the new schema; new tests cover the legacy-file warning (presence + absence), merge-into-existing behavior for bothopencode.jsonandopencode.jsonc, JSONC comment stripping (string preservation, full-line and inline stripping, CRLF/CR line endings), and_opencode_config_pathprecedencedocs/USAGE.mdupdated to showopencode.json/opencode.jsoncReview status (Copilot, 3 rounds): 8 of 10 threads resolved. The 2 remaining are intentionally left open:
→arrow that could be misread as a migration; the code only does path selection, not migration) — left as-is to keep the diff minimal; happy to refine if the maintainer prefers.skills.py:416(re.sub(r',(\s*[}\])])', …)) is pre-existing onmainand is string-unaware in the same class as the JSONC-comment bug. Out of scope for this PR; worth a follow-up issue if the maintainer agrees.Fixes #388.