Repository navigation
Conversation
`cmux omo` (and `cmux`'s session-plugin registration) abort with "Failed to parse <path>. Fix the JSON syntax and retry." whenever the user's ~/.config/opencode/opencode.json contains JSONC syntax — most commonly a leading editor mode line such as `// be in -*- jsonc -*- mode`. opencode itself treats opencode.json as JSONC (it tolerates `//` and `/* */` comments and trailing commas), but cmux decodes it with strict `JSONSerialization`, which rejects comments and fails on the very first byte. The repo already ships `JSONCParser.preprocess` and uses it for other config reads, so opencode.json is the odd one out. This is the first commit of a two-commit red/green pair (per the repo's regression test policy): it only adds the failing test plus a shared `CMUXCLI.parseOpenCodeConfig` seam that both affected call sites (`omoEnsurePlugin` and `updateOpenCodePluginRegistration`) now route through. The helper still performs the old strict parse here, so the new `OmoOpenCodeConfigJSONCParsingTests` fails — proving the test catches the bug. The next commit makes the helper JSONC-aware to turn it green.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesJSONC Config Parsing and Plugin Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (17 passed)
✨ 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 |
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge. The fix is scoped to two config-read paths, the surgical JSONC write preserves user comments, and all three issues from earlier review rounds are resolved in this HEAD. Both affected call sites now parse opencode.json through the shared JSONC-aware helper. The updateOpenCodePluginRegistration path uses a surgical in-place text edit so user comments survive, and falls back to full re-serialisation only when the editor cannot parse the source structure (a narrow edge case that matches the pre-PR behaviour). The order-insensitive no-op guard correctly handles the case where the session plugin already exists in a non-tail position, preventing spurious rewrites. No correctness gaps remain in the changed paths. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["opencode.json bytes (may be JSONC)"] --> B["parseOpenCodeConfig(data:sourcePath:)"]
B --> C["JSONCParser.preprocess — strip // and /* */ comments, trailing commas"]
C --> D["JSONSerialization.jsonObject → [String: Any]"]
D -->|"parse failure"| E["throw CLIError: Failed to parse … Fix the JSON syntax and retry."]
D -->|"success"| F{Caller}
F -->|"omoEnsurePlugin"| G["Mutate config dict (add omo plugin)\nJSONSerialization → strict JSON\nWrite to SHADOW dir (safe)"]
F -->|"updateOpenCodePluginRegistration"| H["openCodePluginListsEqual?\n(order-insensitive canonical comparison)"]
H -->|"equal — no change"| I["return false (no-op, file untouched)"]
H -->|"not equal"| J["JSONCParser.source → sourceText"]
J -->|"sourceText available"| K["openCodePluginListValueJSON → valueJSON\nJSONCObjectEditor.setRootProperty\n(surgical, comment-preserving edit)"]
K -->|"edit succeeds"| L["Write edited JSONC bytes\n(comments preserved)"]
K -->|"edit returns nil"| M["Fallback: JSONSerialization\nWrite strict JSON\n(comments lost — edge case only)"]
J -->|"sourceText nil"| M
L --> N["return true"]
M --> N
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["opencode.json bytes (may be JSONC)"] --> B["parseOpenCodeConfig(data:sourcePath:)"]
B --> C["JSONCParser.preprocess — strip // and /* */ comments, trailing commas"]
C --> D["JSONSerialization.jsonObject → [String: Any]"]
D -->|"parse failure"| E["throw CLIError: Failed to parse … Fix the JSON syntax and retry."]
D -->|"success"| F{Caller}
F -->|"omoEnsurePlugin"| G["Mutate config dict (add omo plugin)\nJSONSerialization → strict JSON\nWrite to SHADOW dir (safe)"]
F -->|"updateOpenCodePluginRegistration"| H["openCodePluginListsEqual?\n(order-insensitive canonical comparison)"]
H -->|"equal — no change"| I["return false (no-op, file untouched)"]
H -->|"not equal"| J["JSONCParser.source → sourceText"]
J -->|"sourceText available"| K["openCodePluginListValueJSON → valueJSON\nJSONCObjectEditor.setRootProperty\n(surgical, comment-preserving edit)"]
K -->|"edit succeeds"| L["Write edited JSONC bytes\n(comments preserved)"]
K -->|"edit returns nil"| M["Fallback: JSONSerialization\nWrite strict JSON\n(comments lost — edge case only)"]
J -->|"sourceText nil"| M
L --> N["return true"]
M --> N
Reviews (9): Last reviewed commit: "Address greptile/coderabbit review on PR..." | Re-trigger Greptile |
@offbyone I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 16 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and changed files
gh pr view 6187 --repo manaflow-ai/cmux --json files,additions,deletions,commits
# Get the diff for the PR
gh pr diff 6187 --repo manaflow-ai/cmuxLength of output: 13387
Implementation ( The helper is correct. One minor note: // Current — swallows the underlying JSONSerialization error
guard let object = try? JSONSerialization.jsonObject(with: sanitized) as? [String: Any] else {
throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry.")
}If you wanted richer diagnostics later, the pattern would be Tests ( The four cases are well-chosen:
The Overall: the fix is correct, minimal, and well-tested. LGTM. [approve] |
Greptile SummaryThis PR fixes
Confidence Score: 3/5The fix is sound for the omoEnsurePlugin (shadow-dir) path, but updateOpenCodePluginRegistration now silently rewrites the user's actual opencode.json as strict JSON whenever the session plugin is installed or removed, discarding all comments and trailing commas the user placed in the file. The parseOpenCodeConfig helper and its use in omoEnsurePlugin are correct: JSONC is accepted, the result is written only to a throwaway shadow directory, and the user's file is never modified. However, updateOpenCodePluginRegistration round-trips the parsed dictionary through JSONSerialization.data(withJSONObject:options: [.prettyPrinted, .sortedKeys]) and writes that back to the user's real config file. For any user whose file contains JSONC syntax, the serialised bytes will never equal the original (comments gone, keys reordered), so existingData == output is always false and the write fires every time, permanently stripping comments without any warning. Before this PR, those same users saw a parse error and their file was left untouched; after this PR the error is gone but their carefully annotated config is silently overwritten. CLI/cmux.swift around updateOpenCodePluginRegistration — specifically the output.write(to: configURL) path that now activates for JSONC files that were previously rejected. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User's opencode.json (JSONC)
participant CMux as cmux
participant Parse as parseOpenCodeConfig
participant JSONC as JSONCParser.preprocess
participant JSON as JSONSerialization
CMux->>Parse: data, sourcePath
Parse->>JSONC: "strip // and /* */ comments, trailing commas"
JSONC-->>Parse: sanitized Data (strict JSON)
Parse->>JSON: jsonObject(with: sanitized)
JSON-->>Parse: [String: Any]
Parse-->>CMux: config dict
alt omoEnsurePlugin path
CMux->>CMux: modify plugin list
CMux->>JSON: data(withJSONObject: config) strict JSON
CMux->>CMux: write to shadowDir/opencode.json (user original untouched)
else updateOpenCodePluginRegistration path
CMux->>CMux: modify plugin list
CMux->>JSON: data(withJSONObject: config) strict JSON (comments stripped)
CMux->>User: write strict JSON back to user opencode.json (JSONC lost)
end
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 27516-27527: The parseOpenCodeConfig function discards underlying
error details when handling parse failures. In the catch block for
JSONCParser.preprocess (around line 27521), append the actual error to the
CLIError message using String(describing: error). For the
JSONSerialization.jsonObject call (around line 27524), change the try? to try
and add a catch block to capture the actual error details, then include them in
the CLIError message to provide users with specific debugging information about
line numbers, character positions, or JSON structure problems that caused the
parse failure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90246607-89ff-49c8-b44b-4da055399940
📒 Files selected for processing (3)
CLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OmoOpenCodeConfigJSONCParsingTests.swift
| static func parseOpenCodeConfig(data: Data, sourcePath: String) throws -> [String: Any] { | ||
| let sanitized: Data | ||
| do { | ||
| sanitized = try JSONCParser.preprocess(data: data) | ||
| } catch { | ||
| throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry.") | ||
| } | ||
| guard let object = try? JSONSerialization.jsonObject(with: sanitized) as? [String: Any] else { | ||
| throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry.") | ||
| } | ||
| return object | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider preserving underlying error context in CLI error messages.
The current implementation catches errors from JSONCParser.preprocess (line 27521) and JSONSerialization (line 27524) but discards the underlying error details. When users encounter parse failures, they won't see which line/character caused the issue or what specific JSON structure problem exists.
Per retrieved learnings, CLI error messages should use String(describing: error) to preserve full structured descriptions for diagnostic purposes. Appending the underlying error would improve debugging while keeping the user-facing message clear.
♻️ Preserve error context
let sanitized: Data
do {
sanitized = try JSONCParser.preprocess(data: data)
} catch {
- throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry.")
+ throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry. Error: \(String(describing: error))")
}
- guard let object = try? JSONSerialization.jsonObject(with: sanitized) as? [String: Any] else {
- throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry.")
+ guard let object = try JSONSerialization.jsonObject(with: sanitized) as? [String: Any] else {
+ throw CLIError(message: "Failed to parse \(sourcePath). Fix the JSON syntax and retry. Top-level JSON must be an object, not an array or primitive.")
}
return objectThe JSONSerialization guard now uses try instead of try? so the error is available in the catch block above (if you add a catch for it), or you can keep the structure and just improve the type-mismatch message as shown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/cmux.swift` around lines 27516 - 27527, The parseOpenCodeConfig function
discards underlying error details when handling parse failures. In the catch
block for JSONCParser.preprocess (around line 27521), append the actual error to
the CLIError message using String(describing: error). For the
JSONSerialization.jsonObject call (around line 27524), change the try? to try
and add a catch block to capture the actual error details, then include them in
the CLIError message to provide users with specific debugging information about
line numbers, character positions, or JSON structure problems that caused the
parse failure.
Source: Learnings
|
@offbyone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
dc910de to
e16964e
Compare
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@offbyone I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 15 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
e16964e to
7cef4db
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func openCodePluginListsEqual(_ lhs: [Any], _ rhs: [Any]) -> Bool { | ||
| guard | ||
| let lhsData = try? JSONSerialization.data(withJSONObject: lhs, options: [.sortedKeys]), | ||
| let rhsData = try? JSONSerialization.data(withJSONObject: rhs, options: [.sortedKeys]) | ||
| else { return false } | ||
| return lhsData == rhsData | ||
| } |
There was a problem hiding this comment.
openCodePluginListsEqual serializes both arrays with JSONSerialization using .sortedKeys, but that option only sorts keys inside JSON objects — it has no effect on JSON array element order. If the user's config already contains ["cmux-session", "other-plugin"], openCodePluginListRemovingSessionPlugin strips the session entry, then it is re-appended at the end, yielding ["other-plugin", "cmux-session"]. The two arrays serialise to different byte sequences, so the no-op guard returns false and the file is rewritten (reordering the plugin) on every install run until the user's original ordering is gone.
| static func openCodePluginListsEqual(_ lhs: [Any], _ rhs: [Any]) -> Bool { | |
| guard | |
| let lhsData = try? JSONSerialization.data(withJSONObject: lhs, options: [.sortedKeys]), | |
| let rhsData = try? JSONSerialization.data(withJSONObject: rhs, options: [.sortedKeys]) | |
| else { return false } | |
| return lhsData == rhsData | |
| } | |
| static func openCodePluginListsEqual(_ lhs: [Any], _ rhs: [Any]) -> Bool { | |
| func canonicalize(_ plugins: [Any]) -> [String] { | |
| plugins.compactMap { element in | |
| guard JSONSerialization.isValidJSONObject([element]), | |
| let data = try? JSONSerialization.data(withJSONObject: [element], options: [.sortedKeys]), | |
| let str = String(data: data, encoding: .utf8) else { return nil } | |
| return str | |
| }.sorted() | |
| } | |
| return canonicalize(lhs) == canonicalize(rhs) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 27518-27531: The variable declared as configParseErorr on line
27518 contains a typo with "Erorr" instead of "Error". This causes a compilation
error because subsequent references on lines 27528 and 27531 correctly use
configParseError. Fix the typo by correcting the variable declaration from
configParseErorr to configParseError to match the references throughout the
code.
In `@cmuxTests/OmoOpenCodeConfigJSONCParsingTests.swift`:
- Around line 113-117: The assertions in this test block use the `contains`
method which is too permissive and allows extra or duplicate plugin entries to
pass without detection. Replace the weak substring checks with stronger
assertions that verify the exact structure of the plugin array and explicitly
check that the model line preserves its trailing comma. This ensures the
surgical insertion path correctly modifies the configuration without allowing
false-positive passes when duplicate entries or structural issues exist.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6aa988a0-1030-45e4-9cc1-59a383c40abc
📒 Files selected for processing (3)
CLI/cmux.swiftSources/JSONCParser.swiftcmuxTests/OmoOpenCodeConfigJSONCParsingTests.swift
Route `CMUXCLI.parseOpenCodeConfig` through the existing `JSONCParser.preprocess` before handing the bytes to `JSONSerialization`, matching how opencode itself reads opencode.json and how cmux reads its other JSONC config files. This fixes `cmux omo` (and the session-plugin registration path) aborting with "Failed to parse <path>. Fix the JSON syntax and retry." when the user's opencode.json contains comments or trailing commas, e.g. a leading `// be in -*- jsonc -*- mode` line. Genuinely malformed configs still surface the same user-facing error, so real syntax mistakes are not masked. Turns the OmoOpenCodeConfigJSONCParsingTests regression test green.
7cef4db to
a95c0fe
Compare
- openCodePluginListsEqual: compare plugin lists order-insensitively. .sortedKeys only sorts object keys, not array element order, so an already-registered config like ["cmux-session", "other"] re-serialized to ["other", "cmux-session"] looked changed and got rewritten, reordering the user's plugins and stripping comments. Canonicalize and sort elements before comparing so a reorder is a true no-op. - Strengthen JSONC insertion test: assert the model line keeps its trailing comma and the resulting plugin array is exactly ["cmux-session"] instead of a permissive contains check. - Extend the no-op equality test with order-insensitive and differing-membership cases.
|
Before we can re-land this on main, please comment |
teamleaderleo
added
area: agents
|
I have read the CLA Document v2.2 and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Before we can re-land this on main, please comment |
teamleaderleo
added
the
ready-to-land
2 participants
Summary
cmuxnow reads the user's~/.config/opencode/opencode.jsonas JSONC instead of strict JSON. Added a single shared helperCMUXCLI.parseOpenCodeConfig(data:sourcePath:)that runs the existingJSONCParser.preprocess(strips//and/* */comments and trailing commas) beforeJSONSerialization, and routed both code paths that readopencode.jsonthrough it:omoEnsurePlugin(thecmux omopath) andupdateOpenCodePluginRegistration(session-plugin registration). The user-facing error message (Failed to parse <path>. Fix the JSON syntax and retry.) is preserved, so genuinely malformed configs still surface it.cmux omoaborted withFailed to parse <path>. Fix the JSON syntax and retry.wheneveropencode.jsoncontained JSONC syntax — most commonly a leading editor mode line such as// be in -*- jsonc -*- mode. opencode itself treatsopencode.jsonas JSONC, and cmux already usesJSONCParser.preprocessfor its other config reads;opencode.jsonwas the one path still doing a strict parse, so it failed on the very first byte. Both affected entrypoints shared the identical bug, so per the repo's shared-behavior policy they were fixed through one common path.Testing
cmuxTests/OmoOpenCodeConfigJSONCParsingTests.swift(Swift Testing) covering the exact repro (leading//mode line), inline/block comments + trailing commas, plain strict JSON, and a malformed-config case that must still throw. Commit 1 adds the test against the still-strict helper (red); commit 2 makes the helper JSONC-aware (green). The test file is wired intocmux.xcodeproj/project.pbxproj(all four sections) and./scripts/lint-pbxproj-test-wiring.shpasses.cmux omo→Error: Failed to parse /Users/.../.config/opencode/opencode.json. Fix the JSON syntax and retry., and bisected it (via an isolatedHOMEcopy) to the leading//comment — every comment form (//,/* */, line 1 or 2) triggered it; the comment-free file parsed.Xcode.app, GhosttyKit unbuilt — soxcodebuild/cmux-unitis unavailable). Instead I compiled the realSources/JSONCParser.swiftwith a standaloneswiftcharness using the exact test inputs and confirmed: the strict parse rejects the leading-comment repro (test is genuinely red without the fix), and the JSONC path parses leading comments, inline/block comments, trailing commas, and plain JSON while still throwing on malformed input. CI'stestsjob is the authoritative compile/run of the test target.Demo Video
Not applicable — this is a CLI startup/config-parsing fix with no UI surface. Behavior change is covered by the automated regression test and the manual repro above.
Review Trigger (Copy/Paste as PR comment)
Checklist
cmux-unittest target was NOT run locally — no Xcode.app on this machine — relying on CI)/releaseflow — flag if you want a CHANGELOG entry now)Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
cmux now parses
~/.config/opencode/opencode.jsonas JSONC and preserves comments/formatting when updating thepluginlist, fixingcmux omoand session plugin registration failures and preventing comment stripping or plugin reordering.CMUXCLI.parseOpenCodeConfig(data:sourcePath:)that runsJSONCParser.preprocessbeforeJSONSerialization.JSONCObjectEditor.setRootProperty, keeping comments, trailing commas, and formatting; falls back to pretty JSON only if needed.CMUXCLI.openCodePluginListsEqual, which now canonicalizes and compares plugin arrays order-insensitively to avoid needless rewrites and reordering.OmoOpenCodeConfigJSONCParsingTestscovering JSONC parsing, comment-preserving plugin updates, order-insensitive no-op detection, plain JSON, and malformed input.Written for commit 3b08caf. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
opencode.jsonhandling with JSONC support (comments and trailing commas) and improved plugin registration updates.Bug Fixes
Tests
Documentation