fix(cua-driver/macos): set_config accepts {key,value} shape (parity with Win/Linux) - #2059
Merged
Merged
Conversation
…ith Win/Linux)
macOS set_config read only the direct `capture_scope`/`capture_mode`/
`max_image_dimension` fields and silently ignored the `{key, value}` shape that
Windows, Linux, and the CLI `config set` all accept — so an MCP client sending
`{"key":"capture_scope","value":"desktop"}` was a no-op on macOS (the agent then
saw capture_scope still "window"). This bit the desktop-scope work.
Make macOS accept BOTH shapes (additive, fully back-compat): a direct field wins
if both are present, otherwise fall back to the `{key, value}` pair — mirroring
the Linux normalization. Adds `key`/`value` to the tool schema (they were
rejected under `additionalProperties:false`). The contract is now identical on
all three platforms; documented in the set_config reference.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
ChangesSet config input shape
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/content/docs/reference/cua-driver/mcp-tools.mdx`:
- Around line 527-528: Update the argument table in the MCP tools docs to list
capture_scope, key, and value so it matches the accepted inputs described in the
surrounding paragraph. Make sure the table reflects both supported shapes for
the config tool contract, and keep the existing direct-field vs {key, value}
behavior aligned with the documentation.
In `@libs/cua-driver/rust/crates/platform-macos/src/tools/set_config.rs`:
- Around line 112-116: Validate capture_mode in set_config’s capture_mode
handling so the {key, value} path enforces the same allowed values as the direct
field. Mirror the existing capture_scope validation near the capture_mode lookup
in the relevant config parsing flow, ensuring both args.opt_str("capture_mode")
and kv_str("capture_mode") reject any value outside ["som", "vision", "ax"]
before persisting it.
- Around line 87-116: The new `{key, value}` fallback in `invoke` only covers
`capture_mode`, `capture_scope`, and `max_image_dimension`, so PiP-related
settings can still silently no-op on macOS. Update `set_config.rs` so
`experimental_pip` and `experimental_pip_geometry` are also read from the same
`kv` fallback path, or explicitly reject `{key, value}` for those options with a
clear error. Keep the handling consistent with the existing `kv_str`/`kv_u64`
helpers and the direct-field precedence used for the other config keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56b86b22-b8b1-484e-8523-c1a01af8322d
📒 Files selected for processing (2)
docs/content/docs/reference/cua-driver/mcp-tools.mdxlibs/cua-driver/rust/crates/platform-macos/src/tools/set_config.rs
Comment on lines
+527
to
+528
| Two equivalent argument shapes are accepted on every platform: a **direct field** (`{"capture_scope": "desktop"}`) or a **`{key, value}` pair** (`{"key": "capture_scope", "value": "desktop"}`, the same shape the CLI `config set` uses). A direct field wins if both are supplied. | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the argument table to include capture_scope, key, and value.
The new paragraph documents the alternate shape, but the reference table below still omits those inputs, so the docs no longer fully match the tool contract.
🤖 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 `@docs/content/docs/reference/cua-driver/mcp-tools.mdx` around lines 527 - 528,
Update the argument table in the MCP tools docs to list capture_scope, key, and
value so it matches the accepted inputs described in the surrounding paragraph.
Make sure the table reflects both supported shapes for the config tool contract,
and keep the existing direct-field vs {key, value} behavior aligned with the
documentation.
Comment on lines
+87
to
+116
| // Accept BOTH shapes, matching Windows/Linux + the CLI `config set`: | ||
| // - direct fields: {"capture_scope":"desktop"} | ||
| // - {key, value}: {"key":"capture_scope","value":"desktop"} | ||
| // A direct field wins if both are somehow present. | ||
| let kv: Option<(String, Value)> = args | ||
| .opt_str("key") | ||
| .and_then(|k| args.get("value").map(|v| (k, v.clone()))); | ||
| let kv_str = |name: &str| -> Option<String> { | ||
| kv.as_ref() | ||
| .filter(|(k, _)| k == name) | ||
| .and_then(|(_, v)| v.as_str().map(str::to_owned)) | ||
| }; | ||
| let kv_u64 = |name: &str| -> Option<u64> { | ||
| kv.as_ref().filter(|(k, _)| k == name).and_then(|(_, v)| v.as_u64()) | ||
| }; | ||
|
|
||
| // Validate max_image_dimension up front so both branches share the | ||
| // u32 check and we never half-apply. | ||
| let max_dim: Option<u32> = match args.opt_u64("max_image_dimension") { | ||
| let max_dim: Option<u32> = match args.opt_u64("max_image_dimension").or_else(|| kv_u64("max_image_dimension")) { | ||
| Some(dim) => match u32::try_from(dim) { | ||
| Ok(d) => Some(d), | ||
| Err(_) => return ToolResult::error(format!("max_image_dimension {dim} exceeds u32::MAX")), | ||
| }, | ||
| None => None, | ||
| }; | ||
| let capture_mode = args.opt_str("capture_mode"); | ||
| let capture_mode = args.opt_str("capture_mode").or_else(|| kv_str("capture_mode")); | ||
|
|
||
| // Validate capture_scope up front so both branches share the check and | ||
| // we never half-apply an invalid value. | ||
| let capture_scope = args.opt_str("capture_scope"); | ||
| let capture_scope = args.opt_str("capture_scope").or_else(|| kv_str("capture_scope")); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle {key, value} for the PiP settings too, or reject them explicitly.
The new fallback only feeds capture_mode, capture_scope, and max_image_dimension. experimental_pip and experimental_pip_geometry are still read from direct fields only later in invoke, so requests like {"key":"experimental_pip","value":true} now validate against the schema/docs but still no-op on macOS. That breaks the “equivalent argument shapes” contract this PR is adding.
🤖 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 `@libs/cua-driver/rust/crates/platform-macos/src/tools/set_config.rs` around
lines 87 - 116, The new `{key, value}` fallback in `invoke` only covers
`capture_mode`, `capture_scope`, and `max_image_dimension`, so PiP-related
settings can still silently no-op on macOS. Update `set_config.rs` so
`experimental_pip` and `experimental_pip_geometry` are also read from the same
`kv` fallback path, or explicitly reject `{key, value}` for those options with a
clear error. Keep the handling consistent with the existing `kv_str`/`kv_u64`
helpers and the direct-field precedence used for the other config keys.
Comment on lines
+112
to
+116
| let capture_mode = args.opt_str("capture_mode").or_else(|| kv_str("capture_mode")); | ||
|
|
||
| // Validate capture_scope up front so both branches share the check and | ||
| // we never half-apply an invalid value. | ||
| let capture_scope = args.opt_str("capture_scope"); | ||
| let capture_scope = args.opt_str("capture_scope").or_else(|| kv_str("capture_scope")); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate capture_mode on the {key, value} path.
kv_str("capture_mode") accepts any string, but the direct-field shape is constrained to ["som", "vision", "ax"]. That means {"key":"capture_mode","value":"anything"} can persist an invalid mode that the direct-field contract would reject. Please mirror the capture_scope check here so both input shapes enforce the same values.
🤖 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 `@libs/cua-driver/rust/crates/platform-macos/src/tools/set_config.rs` around
lines 112 - 116, Validate capture_mode in set_config’s capture_mode handling so
the {key, value} path enforces the same allowed values as the direct field.
Mirror the existing capture_scope validation near the capture_mode lookup in the
relevant config parsing flow, ensuring both args.opt_str("capture_mode") and
kv_str("capture_mode") reject any value outside ["som", "vision", "ax"] before
persisting it.
Contributor
Linux visual regression artifactsMatrix jobs now run independently. Download visual artifacts from this workflow run.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
macOS
set_configread only the direct fields ({"capture_scope":"desktop"}) and silently ignored the{key, value}shape that Windows, Linux, and the CLIconfig setall accept. So an MCP client sending{"key":"capture_scope","value":"desktop"}was a no-op on macOS — the agent then sawcapture_scopestill"window". This is the cross-platform inconsistency flagged during the desktop-scope work (and it bit the macOS desktop-scope test until worked around).Fix (additive, fully back-compat)
macOS now accepts both shapes: a direct field wins if both are present, otherwise it falls back to the
{key, value}pair — mirroring the Linux normalization. Also addskey/valueto the tool schema (they were rejected underadditionalProperties: false).The contract is now identical on all three platforms. Documented in the
set_configreference.Verification
Compiles clean (release). The canonical
{key, value}shape is whattransport_config_persistence_testalready uses; no behavior change for direct-field callers.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
{key, value}pairs.Bug Fixes
{key, value}shape in addition to direct field input.