fix(cua-driver/windows): persist config values for config parity - #2033
fix(cua-driver/windows): persist config values for config parity#2033outdog-hwh wants to merge 2 commits into
Conversation
|
@outdog-hwh is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughWindows startup now reads driver config from ChangesWindows config persistence
Sequence Diagram(s)sequenceDiagram
participant Caller as MCP caller
participant Tool as SetConfigTool::invoke
participant State as ToolState.config
participant Disk as config.json
Caller->>Tool: set_config { key, value }
Tool->>State: apply capture_mode / max_image_dimension
Tool->>Disk: write_driver_config_key(...)
Tool-->>Caller: updated config payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/cua-driver/rust/tests/integration/test_api_parity.py (1)
1175-1237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
capture_modein the Windows persistence regression too.The production change persists both
capture_modeandmax_image_dimension, but this test only exercisesmax_image_dimension; adding acapture_modeseed/set/get assertion would catch half of this PR’s 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 `@libs/cua-driver/rust/tests/integration/test_api_parity.py` around lines 1175 - 1237, The Windows persistence regression test only covers max_image_dimension, so it misses the capture_mode part of the config persistence contract. Extend test_config_cli_and_daemon_share_persisted_max_image_dimension in test_api_parity.py to seed capture_mode in the initial config, verify it is surfaced by config and config get, update it through config set, and assert the change is reflected both in CLI output and the persisted config file.
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs`:
- Around line 5059-5139: The config update path is mutating cfg before all
inputs are validated, and capture_mode is also being accepted without enforcing
the som|vision|ax enum. Refactor this block to parse and validate every
requested value up front (including capture_mode and max_image_dimension, plus
the PiP fields) before acquiring the write lock, then apply all validated
changes to cfg atomically in the same section so a failed request cannot leave
partial in-memory state.
- Around line 169-172: The daemon config path resolution in config_file_path
currently depends only on HOME and returns None when it is missing, which makes
it diverge from the CLI helper. Update config_file_path in impl_ to use the same
fallback behavior as the CLI path helper, or refactor both to share a common
path-resolution function, so daemon config.json loading and persistence always
use the identical path.
- Around line 202-208: The persisted JSON root in set_config is being indexed
directly after parsing, which can panic if config.json contains a valid
non-object root like an array, string, number, or bool. Update the logic around
the json Value initialization and the subsequent json[key] assignment in
impl_.rs to first coerce any non-object Value into an empty object before
inserting the key, using the existing set_config flow and Value handling.
---
Nitpick comments:
In `@libs/cua-driver/rust/tests/integration/test_api_parity.py`:
- Around line 1175-1237: The Windows persistence regression test only covers
max_image_dimension, so it misses the capture_mode part of the config
persistence contract. Extend
test_config_cli_and_daemon_share_persisted_max_image_dimension in
test_api_parity.py to seed capture_mode in the initial config, verify it is
surfaced by config and config get, update it through config set, and assert the
change is reflected both in CLI output and the persisted config file.
🪄 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: 33e75e16-1fb1-4166-a606-82535df23714
📒 Files selected for processing (2)
libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rslibs/cua-driver/rust/tests/integration/test_api_parity.py
| fn config_file_path() -> Option<std::path::PathBuf> { | ||
| std::env::var("HOME") | ||
| .ok() | ||
| .map(|home| std::path::PathBuf::from(home).join(".cua-driver").join("config.json")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the daemon config path identical to the CLI path.
Line 170 returns None when HOME is absent, while the CLI helper falls back to /tmp; in that environment the daemon loads defaults and persistence only warns, so config get / daemon state / config.json can drift again. Mirror the CLI path helper or share it.
Proposed fix
-fn config_file_path() -> Option<std::path::PathBuf> {
- std::env::var("HOME")
- .ok()
- .map(|home| std::path::PathBuf::from(home).join(".cua-driver").join("config.json"))
+fn config_file_path() -> std::path::PathBuf {
+ let home = std::env::var("HOME").unwrap_or_else(|_| "/tmp".into());
+ std::path::PathBuf::from(home).join(".cua-driver").join("config.json")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn config_file_path() -> Option<std::path::PathBuf> { | |
| std::env::var("HOME") | |
| .ok() | |
| .map(|home| std::path::PathBuf::from(home).join(".cua-driver").join("config.json")) | |
| fn config_file_path() -> std::path::PathBuf { | |
| let home = std::env::var("HOME").unwrap_or_else(|_| "/tmp".into()); | |
| std::path::PathBuf::from(home).join(".cua-driver").join("config.json") |
🤖 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-windows/src/tools/impl_.rs` around lines
169 - 172, The daemon config path resolution in config_file_path currently
depends only on HOME and returns None when it is missing, which makes it diverge
from the CLI helper. Update config_file_path in impl_ to use the same fallback
behavior as the CLI path helper, or refactor both to share a common
path-resolution function, so daemon config.json loading and persistence always
use the identical path.
| let mut json: Value = path | ||
| .exists() | ||
| .then(|| std::fs::read_to_string(&path).ok()) | ||
| .flatten() | ||
| .and_then(|text| serde_json::from_str(&text).ok()) | ||
| .unwrap_or_else(|| json!({})); | ||
| json[key] = value.clone(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
serde_json Value IndexMut string assignment behavior when the Value is an array/string/number instead of an object
💡 Result:
In serde_json, the IndexMut implementation for Value (invoked when using the [] operator) is designed to panic if the operation is invalid for the current Value variant [1][2][3]. The behavior depends on the type of the index used: - String Indexing: When using a string index (e.g., value["key"] = ...), the Value must be an Object or Null. If the Value is a String, Array, Number, or Bool, the operation will panic [1][2][3]. If the Value is Null, it is treated as an empty object [1][3]. - Number (usize) Indexing: When using a numeric index (e.g., value[0] = ...), the Value must be an Array. The index must also be within the bounds of that array. If the Value is not an Array, or if it is an Array that is too small for the given index, the operation will panic [1][2][3]. Because IndexMut requires a panic to handle these invalid states (as the trait's signature does not allow for returning an Option or Result), it is generally recommended to use the non-panicking get_mut method instead when you cannot guarantee the structure of the JSON data [1][4][5]. get_mut returns Option<&mut Value>, which evaluates to None if the index is invalid or out of bounds [1][6][5].
Citations:
- 1: https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html
- 2: https://docs.rs/serde_json/latest/src/serde_json/value/index.rs.html
- 3: https://docs.serde.rs/src/serde_json/value/index.rs.html
- 4: https://stackoverflow.com/questions/58066201/how-to-handle-potentially-missing-fields-using-serde-json-and-the-value-enum
- 5: https://github.com/serde-rs/json/blob/dab5ed3ee97cef5e2b796513f8d9e4c7416e44bf/src/value/mod.rs
- 6: https://docs.rs/serde%5Fjson/latest/serde%5Fjson/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines and the surrounding function.
FILE="libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs"
wc -l "$FILE"
sed -n '160,240p' "$FILE"Repository: trycua/cua
Length of output: 3088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact function and nearby config handling to see how malformed JSON is treated.
FILE="libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs"
rg -n "set_config|config\.json|json\[key\]|is_object|serde_json::from_str" "$FILE"Repository: trycua/cua
Length of output: 1061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs"
echo "== file size =="
wc -l "$FILE"
echo "== relevant region =="
sed -n '150,250p' "$FILE"
echo "== symbol search =="
rg -n "set_config|config\.json|json\[key\]|is_object|serde_json::from_str|json!" "$FILE"Repository: trycua/cua
Length of output: 10262
Normalize the persisted JSON root before inserting the key. serde_json::Value string indexing panics for valid non-object roots, so a parseable config.json that is an array/string/number/bool can take down set_config; coerce non-object roots to {} before json[key] = ....
🤖 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-windows/src/tools/impl_.rs` around lines
202 - 208, The persisted JSON root in set_config is being indexed directly after
parsing, which can panic if config.json contains a valid non-object root like an
array, string, number, or bool. Update the logic around the json Value
initialization and the subsequent json[key] assignment in impl_.rs to first
coerce any non-object Value into an empty object before inserting the key, using
the existing set_config flow and Value handling.
| let (capture_mode, max_image_dimension) = { | ||
| let mut cfg = self.state.config.write().unwrap(); | ||
| // Swift-compatible {key, value} shape. | ||
| if let (Some(key), Some(val)) = ( | ||
| args.get("key").and_then(|v| v.as_str()), | ||
| args.get("value"), | ||
| ) { | ||
| match key { | ||
| "capture_mode" => match val.as_str() { | ||
| Some(s) => { | ||
| cfg.capture_mode = s.to_owned(); | ||
| persisted_capture_mode = Some(s.to_owned()); | ||
| applied = true; | ||
| } | ||
| applied = true; | ||
| } | ||
| None => return ToolResult::error(format!("`experimental_pip` must be a boolean, got {val}.")), | ||
| }, | ||
| "experimental_pip_geometry" => match val.as_str() { | ||
| Some(s) => { | ||
| if pip_preview::PipGeometry::parse(s).is_none() { | ||
| return ToolResult::error(format!( | ||
| "experimental_pip_geometry `{s}` is not a valid WxH or WxH+X+Y string" | ||
| )); | ||
| None => return ToolResult::error(format!("`capture_mode` must be a string, got {val}.")), | ||
| }, | ||
| "max_image_dimension" => match val.as_u64() { | ||
| Some(n) => match u32::try_from(n) { | ||
| Ok(dim32) => { | ||
| cfg.max_image_dimension = dim32; | ||
| persisted_max_image_dimension = Some(dim32); | ||
| applied = true; | ||
| } | ||
| Err(_) => { | ||
| return ToolResult::error(format!( | ||
| "`max_image_dimension` must fit in u32, got {n}." | ||
| )); | ||
| } | ||
| }, | ||
| None => return ToolResult::error(format!("`max_image_dimension` must be an integer, got {val}.")), | ||
| }, | ||
| "experimental_pip" => match val.as_bool() { | ||
| Some(b) => { | ||
| if let Err(e) = pip_preview::write_config_key("experimental_pip", Value::Bool(b)) { | ||
| return ToolResult::error(format!("failed to persist experimental_pip: {e}")); | ||
| } | ||
| applied = true; | ||
| } | ||
| if let Err(e) = pip_preview::write_config_key("experimental_pip_geometry", Value::String(s.to_owned())) { | ||
| return ToolResult::error(format!("failed to persist experimental_pip_geometry: {e}")); | ||
| None => return ToolResult::error(format!("`experimental_pip` must be a boolean, got {val}.")), | ||
| }, | ||
| "experimental_pip_geometry" => match val.as_str() { | ||
| Some(s) => { | ||
| if pip_preview::PipGeometry::parse(s).is_none() { | ||
| return ToolResult::error(format!( | ||
| "experimental_pip_geometry `{s}` is not a valid WxH or WxH+X+Y string" | ||
| )); | ||
| } | ||
| if let Err(e) = pip_preview::write_config_key("experimental_pip_geometry", Value::String(s.to_owned())) { | ||
| return ToolResult::error(format!("failed to persist experimental_pip_geometry: {e}")); | ||
| } | ||
| applied = true; | ||
| } | ||
| None => return ToolResult::error(format!("`experimental_pip_geometry` must be a string, got {val}.")), | ||
| }, | ||
| other => return ToolResult::error(format!( | ||
| "Unknown config key `{other}`. Known: capture_mode, max_image_dimension, experimental_pip, experimental_pip_geometry." | ||
| )), | ||
| } | ||
| } | ||
| // Legacy per-field shape. | ||
| if let Some(mode) = args.get("capture_mode").and_then(|v| v.as_str()) { | ||
| cfg.capture_mode = mode.to_owned(); | ||
| persisted_capture_mode = Some(mode.to_owned()); | ||
| applied = true; | ||
| } | ||
| if let Some(dim) = args.get("max_image_dimension").and_then(|v| v.as_u64()) { | ||
| match u32::try_from(dim) { | ||
| Ok(dim32) => { | ||
| cfg.max_image_dimension = dim32; | ||
| persisted_max_image_dimension = Some(dim32); | ||
| applied = true; | ||
| } | ||
| None => return ToolResult::error(format!("`experimental_pip_geometry` must be a string, got {val}.")), | ||
| }, | ||
| other => return ToolResult::error(format!( | ||
| "Unknown config key `{other}`. Known: capture_mode, max_image_dimension, experimental_pip, experimental_pip_geometry." | ||
| )), | ||
| Err(_) => { | ||
| return ToolResult::error(format!( | ||
| "`max_image_dimension` must fit in u32, got {dim}." | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Legacy per-field shape. | ||
| if let Some(mode) = args.get("capture_mode").and_then(|v| v.as_str()) { | ||
| cfg.capture_mode = mode.to_owned(); applied = true; | ||
| } | ||
| if let Some(dim) = args.get("max_image_dimension").and_then(|v| v.as_u64()) { | ||
| cfg.max_image_dimension = dim as u32; applied = true; | ||
| } | ||
| (cfg.capture_mode.clone(), cfg.max_image_dimension) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate all config inputs before mutating cfg.
This block applies capture_mode before later max_image_dimension / PiP validation can fail, so a failed request can still change in-memory config. It also accepts any capture_mode string despite the schema’s som|vision|ax enum. Parse and validate requested updates first, then take the write lock and apply them atomically.
🤖 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-windows/src/tools/impl_.rs` around lines
5059 - 5139, The config update path is mutating cfg before all inputs are
validated, and capture_mode is also being accepted without enforcing the
som|vision|ax enum. Refactor this block to parse and validate every requested
value up front (including capture_mode and max_image_dimension, plus the PiP
fields) before acquiring the write lock, then apply all validated changes to cfg
atomically in the same section so a failed request cannot leave partial
in-memory state.
|
Thank you for this, @outdog-hwh — and for picking up #2011. The config-parity problem you're fixing is real and worth closing, and the integration test is a nice touch. 🙏 One gap worth flagging: We ran into exactly this and confirmed it live on a Windows VM (with Happy to credit your work / add you as co-author on the follow-up, and glad to keep collaborating if you'd like to iterate. Really appreciate the contribution. |
|
Closing in favor of #2034, which builds directly on your work here — it adds the |
|
Thank you for the clear follow-up and for crediting me as a co-author in #2034. The USERPROFILE fallback and the Linux extension make sense, and I appreciate you carrying the fix through with runtime verification. I'm glad the original direction was useful, and I'm happy to keep collaborating and pick up more focused Windows / config-parity / cua-driver issues where I can help. |
2 participants
What changed
Persist
capture_modeandmax_image_dimensionback into~/.cua-driver/config.jsonon Windows, and initialize the Windows driver state from that same file at startup.This keeps the daemon,
cua-driver config,cua-driver config get, and the persisted config file aligned formax_image_dimensioninstead of letting each path drift to a different source of truth.The regression test now uses a real Windows named-pipe path (
\\.\pipe\...) for daemon lifecycle coverage, so the config persistence flow can be validated locally on Windows instead of assuming a Unix-style*.sockpath.Related issue
Fixes #2011
Approach
DriverConfigfrom~/.cua-driver/config.jsonwhen the tool state is createdcapture_modeandmax_image_dimensionduringset_configon Windows, matching the file-backed behavior other surfaces already expectconfig,config get,config set, and the on-diskconfig.jsonagainst the same temp HOME directorycua-driver serveon WindowsTesting
python -m py_compile libs/cua-driver/rust/tests/integration/test_api_parity.pycargo test --offline --manifest-path libs/cua-driver/rust/Cargo.toml -p platform-windows --lib -- --nocapturecargo build --offline --manifest-path libs/cua-driver/rust/Cargo.toml -p cua-driverCUA_DRIVER_BINARY=... python -m unittest test_api_parity.RustParityTests.test_serve_status_stop_lifecycle test_api_parity.RustParityTests.test_config_cli_and_daemon_share_persisted_max_image_dimension -vNotes
cargo testpassed locally on Windows after installing the Rust MSVC toolchain and running under the Visual Studio developer environment.Summary by CodeRabbit
New Features
Bug Fixes
Tests