Skip to content

feat(opencode): support opencode.jsonc config files#2522

Open
nb5p wants to merge 2 commits intofarion1231:mainfrom
nb5p:feat/opencode-jsonc-support
Open

feat(opencode): support opencode.jsonc config files#2522
nb5p wants to merge 2 commits intofarion1231:mainfrom
nb5p:feat/opencode-jsonc-support

Conversation

@nb5p
Copy link
Copy Markdown

@nb5p nb5p commented May 2, 2026

Summary / 概述

Add support for opencode.jsonc to get_opencode_config_path(); when both opencode.json and opencode.jsonc exist, prefer opencode.jsonc, otherwise fall back to opencode.json. Restores the JSONC portion from #1279 (sosyz, 6c72a23) that was not merged due to a force push.
在 get_opencode_config_path() 新增 opencode.jsonc 支持,如果 opencode.json 同时存在时,优先选择 opencode.jsonc,否则回退到 opencode.json。恢复了 #1279 (sosyz, 6c72a23) 中,由于被 force push 导致该PR 未合并的 JSONC 部分。

Related Issue / 关联 Issue

Fixed #2521
Ref #1279

Screenshots / 截图

Before / 修改前 After / 修改后

Checklist / 检查清单

  • pnpm typecheck passes / 通过 TypeScript 类型检查
  • pnpm format:check passes / 通过代码格式检查
  • cargo clippy passes (if Rust code changed) / 通过 Clippy 检查(如修改了 Rust 代码)
  • Updated i18n files if user-facing text changed / 如修改了用户可见文本,已更新国际化文件

Make get_opencode_config_path() prefer opencode.jsonc over opencode.json
when both exist, falling back to opencode.json otherwise. Restores the
file-detection half of farion1231#1279 (sosyz, 6c72a23) that
never landed because the PR head branch was force-moved.

Co-Authored-By: sosyz <30596875+sosyz@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@farion1231
Copy link
Copy Markdown
Owner

@codex review
@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Claude finished @farion1231's task in 2m 12s —— View job


Claude PR Review

  • Read changed file src-tauri/src/opencode_config.rs
  • Inspect surrounding code (callers, related modules)
  • Review correctness, edge cases, security, style
  • Post review feedback

The runtime change itself is small, correct, and the precedence (.jsonc over .json) matches what the linked issue asks for. read_opencode_config already used json5::from_str, so JSONC content has been parseable all along — this PR just teaches the path resolver to find the file. Concerns are concentrated in the new test module and one behavioral subtlety on the write path.

🔴 Tests don't follow this repo's test-isolation convention

src-tauri/src/opencode_config.rs:268,286,314,345 set HOME directly, but src-tauri/src/config.rs:13-28 explicitly documents not to do that:

不要直接使用 HOME 环境变量:它可能由 Git/Cygwin/MSYS 等第三方工具注入… 为了让 Windows CI/本地测试能稳定隔离真实用户数据,可通过 CC_SWITCH_TEST_HOME 显式覆盖 home dir。

get_home_dir() only honors CC_SWITCH_TEST_HOME; on Windows it falls through to dirs::home_dir() which uses SHGetKnownFolderPath and ignores HOME entirely. So test_read_opencode_config_with_comments and test_read_opencode_config_trailing_commas will read the developer/CI user's real ~/.config/opencode/ on Windows rather than the temp dir — at best they fail, at worst they trample real config.

Every other config module in this crate already follows the convention:

  • src-tauri/src/app_config.rs:940 (uses CC_SWITCH_TEST_HOME + #[serial])
  • src-tauri/src/hermes_config.rs:1028-1034 (saves/restores CC_SWITCH_TEST_HOME)
  • src-tauri/src/openclaw_config.rs:917+970-1057 (#[serial] on every env-touching test)

Fix this →

🔴 Tests are not #[serial] — they will race

cargo test runs tests within a binary in parallel by default. All four new tests mutate process-global HOME. With multiple in-flight, one test's set_var becomes visible to another's get_opencode_config_path, and the final remove_var in test A leaves test B with no HOME mid-run. Worse, these will also race against the existing serial-annotated tests in app_config.rs / hermes_config.rs / openclaw_config.rs because those guard a different var (CC_SWITCH_TEST_HOME) — fixing the previous point fixes this too, but only if #[serial] is also added.

🟡 Tests unconditionally remove_var at the end — clobbers prior state

Even ignoring parallelism, every test ends with std::env::remove_var("HOME"). If the test binary was launched with HOME set (it always is on Linux/macOS), the first test silently destroys it for everything that runs afterwards in the same process. Compare the save/restore pattern at src-tauri/src/hermes_config.rs:1033-1037 — that's the model to copy.

🟡 Writing back to opencode.jsonc silently strips comments

This isn't introduced by this PR per se, but the PR makes it reachable for the first time and is worth calling out either in code or in the PR description. Flow:

  1. User has ~/.config/opencode/opencode.jsonc with comments + trailing commas.
  2. They use cc-switch to add/switch a provider.
  3. write_opencode_config (opencode_config.rs:80-86) calls write_json_file, which serializes via serde_json::to_string_pretty after sort_json_keys (config.rs:181-193).
  4. The file is rewritten as plain sorted JSON. Comments and trailing commas are gone. The filename still ends in .jsonc.

For users whose only reason to use .jsonc is comments, this is exactly the surprise they wouldn't expect from a feature labelled "support jsonc". Options, ordered by effort:

  • Document the limitation in the PR description / a comment near write_opencode_config.
  • Detect that the source file is .jsonc, log a warning on first write.
  • Best long-term: preserve comments using a JSONC-aware writer (e.g. jsonc-parser with edits) — but that's clearly out of scope for this PR.

I'd accept option 1 for this PR; just make sure the limitation is recorded somewhere a future user can find it.

🟢 Minor

  • The two parsing tests (..._with_comments, ..._trailing_commas) don't actually exercise the PR's change — read_opencode_config was already using json5::from_str, so they would have passed before this PR too if you'd named the file opencode.json. Still useful as regression guards once isolation is fixed; just not the coverage they look like at first glance.
  • let json_path at opencode_config.rs:283 is now unused after the previous test changed (only fs::write(&json_path, …) references it — fine, just flagging in case clippy -D warnings is enforced; it is not flagged here because of the write usage).

Summary

Substantively the runtime patch (get_opencode_config_path precedence) is good. The test module needs to be brought in line with the rest of the crate before merge, and the comment-preservation gap should at least be acknowledged.
• Branch: feat/opencode-jsonc-support

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a60abd7d7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/opencode_config.rs Outdated
fs::write(&jsonc_path, r#"{"provider": {"test-jsonc": {}}}"#).unwrap();

// Set override dir to use temp directory
std::env::set_var("HOME", opencode_dir.parent().unwrap().parent().unwrap());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use test-home override env var in opencode path tests

These tests only set HOME before calling get_opencode_config_path(), but get_home_dir() in this codebase is designed to use CC_SWITCH_TEST_HOME for deterministic test isolation (and explicitly avoids relying on HOME semantics on Windows). As written, the test can resolve to the real user profile path on Windows, so it may not read the temp files created in setup_test_env, causing host-dependent failures.

Useful? React with 👍 / 👎.

Comment thread src-tauri/src/opencode_config.rs Outdated
assert!(path.ends_with("opencode.jsonc"));

// Clean up env var
std::env::remove_var("HOME");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore prior HOME value after test mutations

Each test removes HOME unconditionally during cleanup instead of restoring the prior value, which can leak global process state into later tests in the same binary and make failures order-dependent. This is especially risky because Rust tests run in parallel by default unless explicitly serialized.

Useful? React with 👍 / 👎.

@farion1231
Copy link
Copy Markdown
Owner

@claude review

(Re-running with the updated review prompt to verify it still catches the real blocker — test isolation convention — without piling on P3 noise.)

@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Claude encountered an error —— View job


Re-reviewing PR #2522 with updated review prompt

  • Read changed file src-tauri/src/opencode_config.rs
  • Verify CLAUDE.md test-isolation convention (CC_SWITCH_TEST_HOME)
  • Confirm whether #[serial] is required to avoid races with sibling crates' tests
  • Post final review
    • Branch: feat/opencode-jsonc-support

@farion1231
Copy link
Copy Markdown
Owner

@claude review

(Re-running after dropping the over-aggressive --max-turns 5 cap that aborted the previous attempt.)

@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Claude finished @farion1231's task in 2m 23s —— View job


Claude PR Review

The runtime change in get_opencode_config_path is correct and minimal. Two real issues, both in the new test module, plus one user-visible behavior gap on the write path.


Important — tests use HOME, but get_home_dir() only honors CC_SWITCH_TEST_HOME

src-tauri/src/opencode_config.rs:268,286,314,345 set HOME, but src-tauri/src/config.rs:13-21 documents the convention explicitly:

不要直接使用 HOME 环境变量:它可能由 Git/Cygwin/MSYS 等第三方工具注入… 为了让 Windows CI/本地测试能稳定隔离真实用户数据,可通过 CC_SWITCH_TEST_HOME 显式覆盖 home dir。

get_home_dir() (config.rs:22-34) only reads CC_SWITCH_TEST_HOME, then falls through to dirs::home_dir(). On Windows that's SHGetKnownFolderPath(FOLDERID_Profile), which ignores HOME entirely — so on Windows these tests read the real user's ~/.config/opencode/ rather than the temp dir. Linux CI happens to pass because dirs::home_dir() falls back to HOME on Unix, but the convention is broken either way. Sibling modules already follow it: src-tauri/src/hermes_config.rs:1028-1038 (with save/restore), src-tauri/src/openclaw_config.rs, src-tauri/src/app_config.rs.

Fix this → (confidence: 95)


Important — tests are not #[serial], will race in-binary

cargo test runs tests within a binary in parallel. All four new tests mutate process-global env, so e.g. test A's set_var becomes visible to test B's get_opencode_config_path, and test A's remove_var leaves test B with no override mid-call. Concrete failure: test_read_opencode_config_prefers_jsonc asserts path.ends_with("opencode.jsonc") while test_read_opencode_config_fallback_to_json asserts the opposite — if they interleave, either can fail. Sibling modules guard this with #[serial] (e.g. src-tauri/src/hermes_config.rs:1285,1325,1360,...); switching to CC_SWITCH_TEST_HOME without #[serial] would also race against the sibling tests that already serialize on that var. (confidence: 88)


Important — writing back to opencode.jsonc silently strips comments

Before this PR, .jsonc was never opened, so this scenario was unreachable. After this PR:

  1. User has ~/.config/opencode/opencode.jsonc with comments (the only reason to use .jsonc).
  2. cc-switch performs any provider add/switch.
  3. write_opencode_config (opencode_config.rs:80-86) → write_json_file (config.rs:181-193) → serde_json::to_string_pretty(sort_json_keys(...)).
  4. The file is rewritten as plain sorted JSON. Comments and trailing commas are gone, but the filename is still .jsonc.

This is the exact surprise a user enabling .jsonc support wouldn't expect. Cheapest mitigation: document the limitation in the PR description and/or a comment near write_opencode_config. Long-term fix (a JSONC-aware writer) is out of scope here. (confidence: 85)


Summary

The runtime patch is good. Test module needs to switch to CC_SWITCH_TEST_HOME + #[serial] + save/restore prior value, matching hermes_config.rs. Comment-stripping-on-write is a real user-facing gap that should at least be acknowledged before this lands.
· Branch: feat/opencode-jsonc-support

Address bot review on PR farion1231#2522 (claude-bot, codex-bot):

- Use CC_SWITCH_TEST_HOME instead of HOME so tests are reliable on
  Windows (per config.rs:13-28; HOME is ignored by dirs::home_dir on
  Windows). Matches the convention in hermes_config.rs / openclaw_config.rs.
- RAII TempHome saves the prior value via var_os (preserves non-UTF-8
  paths) and restores on Drop instead of unconditionally remove_var.
- Add #[serial] to every test that mutates env vars.
- Document on write_opencode_config that writing back to .jsonc strips
  comments (serde_json::to_string_pretty round-trip).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mandala5741
Copy link
Copy Markdown

We hope to remove support for Hermes Agent. Hermes Agent is a plagiarizing platform that is not worthy of integration. What was once an open and collaborative tech ecosystem is now being forced into closure due to the unethical practices of Hermes Agent. This is the “gift” that Nous Research has brought to the open-source community — a devastating blow to the trust system of the entire industry. It’s not about how bad the technology is; it’s about how rotten the integrity is. It parades under a grandiose name (“Hermes”), yet acts like a “horse thief.” From code architecture to core concepts, it has plagiarized the work of a small Chinese team in every possible way. After being exposed, instead of reflecting or communicating, it resorted to bullying, demanding that the victim “delete their account.” It even uses AI tools to launder the plagiarized content, trying to exploit loopholes in open-source licenses.

Now, although Nous Research has chosen to remain silent and play dead, the 80,000+ stars on GitHub cannot mask the “stench” wafting from its codebase. As one netizen aptly put it: “OpenClaw may be noisy, but it’s the real deal. This ‘horse’ may look pretty on the outside, but take a whiff — it smells entirely of someone else’s work.”

We don’t need to keep such a “rotten horse” in our stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenCode无法导入JSONC格式的配置

3 participants