Repository navigation
Fix omo model override - #3939
austinywang wants to merge 24 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds parsing for ChangesModel Override Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 the
Confidence Score: 5/5Safe to merge — the three previously flagged issues (stale override, non-atomic Go writes, incomplete agent key coverage in tests) are all addressed and verified by new tests. Both the Swift CLI and Go relay now rebuild the shadow oh-my-opencode config from the user source on every invocation. Argument parsing handles all flag forms and the -- terminator consistently in both implementations. Shadow writes use atomic temp-file rename. User config validation fails fast before writing anything. Regression tests cover all 14 agent keys, 8 category keys, the reset path, the -- terminator, and invalid-config rejection including path-leakage checks. No files require special attention. The agent/category key lists are intentionally duplicated across Swift and Go with a keep-in-sync comment; that coupling is a maintenance concern but not a correctness issue in this PR. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["cmux omo [args...]"] --> B["omoRequestedModel(args)"]
B --> C{--model / -m found before --?}
C -- "yes" --> D["requestedModel = value"]
C -- "no" --> E["requestedModel = nil / empty"]
D --> F["omoEnsurePlugin(requestedModel)"]
E --> F
F --> G["Read user opencode.json\nfail-fast on read/parse error"]
G --> H["Write shadow opencode.json atomically"]
H --> I["Symlink oh-my-opencode.jsonc if user has one"]
I --> J["Read user oh-my-opencode.json\nfail-fast on read/parse error"]
J --> K{requestedModel non-empty?}
K -- "yes" --> L["omoApplyModelOverride\nagents x14, categories x8"]
K -- "no" --> M["Use user config as-is"]
L --> N["Write shadow oh-my-opencode.json atomically"]
M --> N
N --> O["setenv OPENCODE_CONFIG_DIR=shadowDir"]
O --> P["exec opencode with original args"]
Reviews (17): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
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 12648-12656: The parser for the "--model"/"-m" branch currently
accepts "--" as a model; update the block in CLI/cmux.swift (the branch handling
arg == "--model" || arg == "-m" that uses commandArgs, index, and
requestedModel) to treat a next token equal to "--" as the terminator: if the
next token is "--" do not assign requestedModel and do not consume it as the
model value (i.e., return or continue respecting the top-level "--" stop
behavior), otherwise trim and assign as before; ensure index advancement only
happens when a real model value was consumed.
In `@daemon/remote/cmd/cmuxd-remote/agent_launch.go`:
- Around line 563-568: The branch handling the "-m=value" argument sets
requestedModel but lacks a trailing continue like the "--model=value" case; add
a continue after assigning requestedModel in the strings.HasPrefix(arg, "-m=")
block so the loop immediately proceeds to the next arg (matching the pattern
used around the earlier "--model=" branch) and avoids falling through to
subsequent argument handling for the same arg.
🪄 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: efe0691a-0690-488a-802e-d4180fbbdcb6
📒 Files selected for processing (3)
CLI/cmux.swiftcmuxTests/OpenCodeHookRegressionTests.swiftdaemon/remote/cmd/cmuxd-remote/agent_launch.go
Dismissed after the two CodeRabbit findings were fixed in bd132ba and the follow-up CodeRabbit review approved the latest head.
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 15100-15104: The current logic treats read/parse failures of the
userOmoConfig file the same as a missing file and silently sets omoConfig = [:];
change this so you first check whether the file exists (e.g.
FileManager.fileExists(atPath: userOmoConfig.path)) and only default to an empty
dictionary when the file does not exist, but if the file exists and
Data(contentsOf:) or JSONSerialization.jsonObject(with:) fails, surface an
explicit CLI error (throw or process exit with a clear message) indicating the
file is unreadable or contains invalid JSON so the user is informed rather than
silently losing their settings; update the block around userOmoConfig/omoConfig
to implement this behavior.
🪄 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: cce8a5e2-c328-4c78-aeea-4a00e9bc5adb
📒 Files selected for processing (2)
CLI/cmux.swiftdocs/cli-contract.md
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="daemon/remote/cmd/cmuxd-remote/main_test.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/main_test.go:126">
P2: The test asserts a shadow OMO config file must exist after invalid user config, which can lock in the wrong behavior instead of verifying that no shadow config is written.</violation>
</file>
<file name="cmuxTests/OpenCodeHookRegressionTests.swift">
<violation number="1" location="cmuxTests/OpenCodeHookRegressionTests.swift:245">
P3: The invalid-config test is too permissive: it allows a newly written shadow config file as long as its contents are "{", so it can miss regressions where shadow config is written despite parse failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
…-3682-omo-change-model
Closes #3682.
Summary
cmux omo --modelregression with a CLI-level test.Verification
HOMEand fakeopencode:/opt/homebrew/bin/cmux omo --model deepinfra/zai-org/GLM-4.7-Flash run helloforwarded the model but produced no oh-my-opencode agent/category model overrides before the fix.Note
Medium Risk
Touches
cmux omolaunch and config-shadow generation in both the local Swift CLI and remote relay; incorrect parsing or config writes could break OpenCode launches or persist unintended config overrides.Overview
Fixes a regression where
cmux omo --modelwas forwarded toopencodebut did not override the model inside the generatedoh-my-opencode.jsonshadow config.Adds
--model/-mparsing (including=...forms and honoring--), applies the override across a fixed set of agent and category keys, and rebuilds the shadow config from the user source on every run so overrides don’t persist. Also tightens config IO: clearer read/parse errors without leaking full paths, only passthrough-symlinksoh-my-opencode.jsonc, and writes remote shadow files atomically.Adds Swift and Go tests covering model override application/reset,
--terminator handling, and invalid user config rejection, and documents thecmux omocontract behavior.Reviewed by Cursor Bugbot for commit 52e9c8d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes a regression where
cmux omo --modelwas forwarded but not applied tooh-my-opencodeconfigs. Overrides now update all agent/category entries and reset each run by rebuilding the shadow config (closes #3682).agentsandcategoriesin the shadow config (CLI andcmuxd-remote), rebuilt from the user source on every run so overrides never persist.--model/-mwith or without=, and honor--as a terminator.oh-my-opencode.jsonandopencode.json; show path-safe read/parse errors; skip writing any shadow config on failure; pass through onlyoh-my-opencode.jsonc; write shadow configs atomically.docs/cli-contract.md.Written for commit 52e9c8d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
--so it won’t be treated as a model argument.Tests