Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 141 additions & 47 deletions libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,54 @@ impl Default for DriverConfig {
fn default() -> Self { Self { capture_mode: "som".into(), max_image_dimension: 1568 } }
}

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"))
Comment on lines +169 to +172

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ 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.

Suggested change
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.

}

fn load_driver_config() -> DriverConfig {
let mut cfg = DriverConfig::default();
let path = match config_file_path() {
Some(path) => path,
None => return cfg,
};
let text = match std::fs::read_to_string(&path) {
Ok(text) => text,
Err(_) => return cfg,
};
let json: Value = match serde_json::from_str(&text) {
Ok(json) => json,
Err(_) => return cfg,
};
if let Some(mode) = json.get("capture_mode").and_then(|value| value.as_str()) {
cfg.capture_mode = mode.to_owned();
}
if let Some(dim) = json.get("max_image_dimension").and_then(|value| value.as_u64()) {
if let Ok(dim32) = u32::try_from(dim) {
cfg.max_image_dimension = dim32;
}
}
cfg
}

fn write_driver_config_key(key: &str, value: &Value) -> Result<(), String> {
let path = config_file_path().ok_or_else(|| "$HOME is not set".to_string())?;
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();
Comment on lines +202 to +208

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 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:


🏁 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.

if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent).map_err(|err| err.to_string())?;
}
let body = serde_json::to_string_pretty(&json).map_err(|err| err.to_string())?;
std::fs::write(&path, body).map_err(|err| err.to_string())?;
Ok(())
}

pub struct ResizeRegistry {
ratios: std::sync::Mutex<std::collections::HashMap<u32, f64>>,
}
Expand Down Expand Up @@ -219,7 +267,7 @@ impl ToolState {
cursor_registry: Arc::new(CursorRegistry::new()),
resize_registry: Arc::new(ResizeRegistry::new()),
zoom_registry: Arc::new(ZoomRegistry::new()),
config: Arc::new(RwLock::new(DriverConfig::default())),
config: Arc::new(RwLock::new(load_driver_config())),
})
}
}
Expand Down Expand Up @@ -5005,57 +5053,90 @@ impl Tool for SetConfigTool {
})
}
async fn invoke(&self, args: Value) -> ToolResult {
let mut cfg = self.state.config.write().unwrap();
let mut applied = false;
// 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(); applied = true; }
None => return ToolResult::error(format!("`capture_mode` must be a string, got {val}.")),
},
"max_image_dimension" => match val.as_u64() {
Some(n) => { cfg.max_image_dimension = n as u32; applied = true; }
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}"));
let mut persisted_capture_mode: Option<String> = None;
let mut persisted_max_image_dimension: Option<u32> = None;
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)
};
Comment on lines +5059 to +5139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 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.

if let Some(enabled) = args.get("experimental_pip").and_then(|v| v.as_bool()) {
if let Err(e) = pip_preview::write_config_key("experimental_pip", Value::Bool(enabled)) {
return ToolResult::error(format!("failed to persist experimental_pip: {e}"));
Expand All @@ -5076,6 +5157,19 @@ impl Tool for SetConfigTool {
if !applied {
return ToolResult::error("Missing required string field `key` (or a known legacy per-field).");
}
if let Some(mode) = persisted_capture_mode {
if let Err(err) = write_driver_config_key("capture_mode", &Value::String(mode)) {
tracing::warn!("set_config: failed to persist capture_mode: {err}");
}
}
if let Some(dim32) = persisted_max_image_dimension {
if let Err(err) = write_driver_config_key(
"max_image_dimension",
&Value::Number(u64::from(dim32).into()),
) {
tracing::warn!("set_config: failed to persist max_image_dimension: {err}");
}
}
// Emit the same pretty-JSON payload as `get_config` (matches Swift's
// `set_config` return shape — both tools echo the full config after).
let cursor_enabled = self.state.cursor_registry.all_states()
Expand All @@ -5087,8 +5181,8 @@ impl Tool for SetConfigTool {
"schema_version": 1,
"version": env!("CARGO_PKG_VERSION"),
"platform": "windows",
"capture_mode": cfg.capture_mode,
"max_image_dimension": cfg.max_image_dimension,
"capture_mode": capture_mode,
"max_image_dimension": max_image_dimension,
"agent_cursor": { "enabled": cursor_enabled },
"experimental_pip": pip_enabled,
"experimental_pip_geometry": pip_geometry,
Expand Down
72 changes: 72 additions & 0 deletions libs/cua-driver/rust/tests/integration/test_api_parity.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import tempfile
import time
import unittest
import uuid

_HERE = os.path.dirname(os.path.abspath(__file__))
sys.path.insert(0, _HERE)
Expand All @@ -51,13 +52,15 @@ def _run(
args: list[str],
timeout: int = 20,
stdin: str | None = None,
env: dict[str, str] | None = None,
) -> subprocess.CompletedProcess:
return subprocess.run(
args,
capture_output=True,
text=True,
timeout=timeout,
input=stdin,
env=env,
)


Expand Down Expand Up @@ -611,6 +614,8 @@ def test_update_dry_run_exits_zero(self) -> None:
# ── CLI: serve / stop / status daemon lifecycle ───────────────────────────

def _tmp_socket(self) -> str:
if os.name == "nt":
return rf"\\.\pipe\cua-parity-test-{uuid.uuid4().hex}"
return tempfile.mktemp(suffix=".sock", prefix="cua-parity-test-")

def test_status_exits_1_when_no_daemon(self) -> None:
Expand Down Expand Up @@ -1167,6 +1172,73 @@ def setUpClass(cls) -> None:
class RustParityTests(_ParityMixin, unittest.TestCase):
"""Run the full parity suite against the Rust cua-driver-rs binary."""

@unittest.skipUnless(os.name == "nt", "Windows-only config persistence regression")
def test_config_cli_and_daemon_share_persisted_max_image_dimension(self) -> None:
sock = self._tmp_socket()
with tempfile.TemporaryDirectory() as home:
cfg_dir = os.path.join(home, ".cua-driver")
os.makedirs(cfg_dir, exist_ok=True)
cfg_path = os.path.join(cfg_dir, "config.json")
with open(cfg_path, "w", encoding="utf-8") as fh:
json.dump({"max_image_dimension": 777}, fh, indent=2)

env = os.environ.copy()
env["HOME"] = home

proc = subprocess.Popen(
[self.binary, "serve", "--socket", sock],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
env=env,
)
try:
deadline = time.monotonic() + 5.0
while time.monotonic() < deadline:
r = _run([self.binary, "status", "--socket", sock], env=env)
if r.returncode == 0:
break
time.sleep(0.1)
else:
self.fail("daemon did not become ready in 5 s")

r = _run([self.binary, "config", "--socket", sock], env=env)
self.assertEqual(r.returncode, 0, f"config stderr: {r.stderr}")
shown = json.loads(r.stdout)
self.assertEqual(shown["max_image_dimension"], 777)

r = _run(
[self.binary, "config", "get", "max_image_dimension", "--socket", sock],
env=env,
)
self.assertEqual(r.returncode, 0, f"config get stderr: {r.stderr}")
self.assertEqual(r.stdout.strip(), "777")

r = _run(
[self.binary, "config", "set", "max_image_dimension", "888", "--socket", sock],
env=env,
)
self.assertEqual(r.returncode, 0, f"config set stderr: {r.stderr}")

r = _run([self.binary, "config", "--socket", sock], env=env)
self.assertEqual(r.returncode, 0, f"config stderr: {r.stderr}")
shown = json.loads(r.stdout)
self.assertEqual(shown["max_image_dimension"], 888)

r = _run(
[self.binary, "config", "get", "max_image_dimension", "--socket", sock],
env=env,
)
self.assertEqual(r.returncode, 0, f"config get stderr: {r.stderr}")
self.assertEqual(r.stdout.strip(), "888")

with open(cfg_path, encoding="utf-8") as fh:
persisted = json.load(fh)
self.assertEqual(persisted["max_image_dimension"], 888)
finally:
_run([self.binary, "stop", "--socket", sock], env=env)
proc.wait(timeout=3)

@classmethod
def setUpClass(cls) -> None:
cls.binary = default_binary_path()
Expand Down