diff --git a/.github/workflows/nix-build.yml b/.github/workflows/nix-build.yml index fc7d24198c..b780c841ee 100644 --- a/.github/workflows/nix-build.yml +++ b/.github/workflows/nix-build.yml @@ -43,6 +43,12 @@ jobs: visual: false result_link: result-cua-driver-integration artifact_name: "" + - name: NixOS set_config persistence test + check_attr: cua-driver-set-config + timeout_minutes: 8 + visual: false + result_link: result-cua-driver-set-config + artifact_name: "" - name: Linux cursor click GIF test check_attr: cua-driver-linux-cursor-click-gif timeout_minutes: 12 diff --git a/flake.nix b/flake.nix index ce510e3ae5..e71408b203 100644 --- a/flake.nix +++ b/flake.nix @@ -51,6 +51,18 @@ }; }; + # set_config persistence test — regression for #1923 (fixed in + # #1928): the {key, value} write shape must persist and read back + # via get_config (it was silently dropped on Linux before). + cua-driver-set-config = import ./nix/cua-driver/tests/set-config.nix { + inherit pkgs; + inherit (pkgs) lib; + cuaDriverModule = { + imports = [ ./nix/cua-driver/module.nix ]; + services.cua-driver.package = cuaDriverPackage; + }; + }; + # Screenshot test — uses cua-driver's own get_window_state tool # to capture a screenshot via MCP, proving the driver can see the display cua-driver-screenshot = import ./nix/cua-driver/tests/screenshot.nix { diff --git a/libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs b/libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs index 7e10040d7f..d30b9d288a 100644 --- a/libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs +++ b/libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs @@ -2855,13 +2855,19 @@ impl Tool for SetConfigTool { SCFG_DEF.get_or_init(|| ToolDef { name: "set_config".into(), description: "Update cua-driver-rs configuration. capture_mode / \ - max_image_dimension take effect immediately. The experimental_pip \ - keys persist to ~/.cua-driver/config.json and apply on next \ + max_image_dimension take effect immediately.\n\n\ + Two input shapes (both accepted, matching Windows/Swift):\n\ + - **{key, value}** (preferred): `{\"key\": \"max_image_dimension\", \"value\": 800}` \ + — single leaf write.\n\ + - **Legacy per-field**: `{\"capture_mode\": \"som\", \"max_image_dimension\": 0}`.\n\n\ + The experimental_pip keys persist to ~/.cua-driver/config.json and apply on next \ daemon restart (the PiP backend is initialised once at startup; \ Linux ships only the trait stub today — see issue #1729).".into(), input_schema: json!({"type":"object","properties":{ - "capture_mode":{"type":"string","enum":["som","vision","ax"],"description":"Default capture mode for get_window_state."}, - "max_image_dimension":{"type":"integer","description":"Max dimension for screenshot resizing (0 = no limit)."}, + "key":{"type":"string","description":"Name of a single config field to write ({key, value} shape). Pair with `value`."}, + "value":{"description":"New value for `key`. JSON type depends on the key."}, + "capture_mode":{"type":"string","enum":["som","vision","ax"],"description":"Legacy per-field shape. Default capture mode for get_window_state."}, + "max_image_dimension":{"type":"integer","description":"Legacy per-field shape. Max dimension for screenshot resizing (0 = no limit)."}, "experimental_pip":{"type":"boolean","description":"Enable the experimental PiP preview window (applies next restart; Linux backend stubbed)."}, "experimental_pip_geometry":{"type":"string","description":"PiP window size + optional position in `WxH` or `WxH+X+Y` form."} },"additionalProperties":false}), @@ -2872,6 +2878,52 @@ impl Tool for SetConfigTool { use cua_driver_core::tool_args::ArgsExt; let mut cfg = self.state.config.write().unwrap(); let mut parts = Vec::new(); + // {key, value} shape (what the Swift/macOS and Windows callers send). + // Linux previously read only the legacy per-field keys below, so a + // `{"key":"max_image_dimension","value":800}` write was silently + // dropped (issue #1923). Dispatch on `key` to the same fields. + 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(); parts.push(format!("capture_mode={s}")); } + 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; parts.push(format!("max_image_dimension={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}")); + } + parts.push(format!("experimental_pip={b} (next restart)")); + } + 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}")); + } + parts.push(format!("experimental_pip_geometry={s} (next restart)")); + } + 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.opt_str("capture_mode") { parts.push(format!("capture_mode={mode}")); cfg.capture_mode = mode; diff --git a/nix/cua-driver/tests/set-config.nix b/nix/cua-driver/tests/set-config.nix new file mode 100644 index 0000000000..541f109c1f --- /dev/null +++ b/nix/cua-driver/tests/set-config.nix @@ -0,0 +1,186 @@ +# CUA Driver set_config Persistence Test +# +# Regression test for #1923 (fixed in PR #1928): on Linux the `set_config` +# tool only read the legacy per-field keys, so a `{"key":..., "value":...}` +# write (the shape the Swift/macOS and Windows callers send) was silently +# dropped. This boots a NixOS VM, drives the driver over MCP stdio, writes +# config via the `{key, value}` shape, then reads it back with `get_config` +# and asserts the new value persisted. +# +# To run: nix build .#checks.x86_64-linux.cua-driver-set-config +# +{ + pkgs, + lib ? pkgs.lib, + cuaDriverModule, + ... +}: + +let + # Python MCP client that drives cua-driver over stdio. + # Writes config via the {key, value} shape, reads it back, asserts. + setConfigTest = pkgs.writeText "set-config-test.py" '' + import subprocess + import json + import sys + import os + import threading + import time + + DRIVER_BIN = os.environ.get("CUA_DRIVER_BIN", "cua-driver") + + def main(): + print("=== CUA Driver set_config Persistence Test ===", flush=True) + + proc = subprocess.Popen( + [DRIVER_BIN, "mcp", "--no-daemon-relaunch"], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env={**os.environ}, + ) + + # Drain stderr in background to prevent blocking + def drain_stderr(): + for line in proc.stderr: + sys.stderr.buffer.write(line) + sys.stderr.buffer.flush() + t = threading.Thread(target=drain_stderr, daemon=True) + t.start() + + def send_request(method, params=None, req_id=None): + msg = {"jsonrpc": "2.0", "method": method} + if params is not None: + msg["params"] = params + if req_id is not None: + msg["id"] = req_id + line = json.dumps(msg) + "\n" + print(f"[send] {line.strip()}", flush=True) + proc.stdin.write(line.encode()) + proc.stdin.flush() + + def read_response(timeout=30): + # Simple blocking read with timeout via thread + result = [None] + def reader(): + result[0] = proc.stdout.readline() + rt = threading.Thread(target=reader) + rt.start() + rt.join(timeout) + if rt.is_alive(): + raise TimeoutError("No response within timeout") + line = result[0].decode().strip() + print(f"[recv] {line}", flush=True) + return json.loads(line) + + def call_tool(name, arguments, req_id): + send_request("tools/call", {"name": name, "arguments": arguments}, req_id=req_id) + resp = read_response() + assert resp.get("id") == req_id, f"Expected id={req_id}, got {resp.get('id')}" + assert "result" in resp, f"Expected result in response: {resp}" + return resp["result"] + + def get_config(req_id): + result = call_tool("get_config", {}, req_id) + sc = result.get("structuredContent", {}) + assert sc, f"get_config returned no structuredContent: {result}" + return sc + + try: + # Initialize + send_request("initialize", { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "nixos-set-config-test", "version": "1.0.0"}, + }, req_id=1) + resp = read_response() + assert resp.get("id") == 1 and "result" in resp, f"initialize failed: {resp}" + send_request("notifications/initialized", {}) + time.sleep(0.5) + + # Baseline: confirm the value we will write differs from the default + # (max_image_dimension default is 1568), so a passing read-back can + # only mean the write took effect — not a coincidental default. + print("\n--- Baseline get_config ---", flush=True) + before = get_config(2) + print(f"before: max_image_dimension={before.get('max_image_dimension')} " + f"capture_mode={before.get('capture_mode')}", flush=True) + assert before.get("max_image_dimension") != 800, \ + f"baseline already 800; cannot prove persistence: {before}" + + # The regression: write via the {key, value} shape (#1923). This is + # the exact path that was silently dropped on Linux before #1928. + print("\n--- set_config {key, value} ---", flush=True) + call_tool("set_config", {"key": "max_image_dimension", "value": 800}, 3) + call_tool("set_config", {"key": "capture_mode", "value": "ax"}, 4) + + # Read back: get_config must reflect the {key, value} writes. + print("\n--- get_config read-back ---", flush=True) + after = get_config(5) + print(f"after: max_image_dimension={after.get('max_image_dimension')} " + f"capture_mode={after.get('capture_mode')}", flush=True) + assert after.get("max_image_dimension") == 800, \ + f"set_config {{key,value}} did not persist max_image_dimension: {after}" + assert after.get("capture_mode") == "ax", \ + f"set_config {{key,value}} did not persist capture_mode: {after}" + + print("\n=== set_config persistence test passed! ===", flush=True) + + finally: + proc.stdin.close() + proc.terminate() + proc.wait(timeout=5) + + if __name__ == "__main__": + main() + ''; + +in + +pkgs.testers.nixosTest { + name = "cua-driver-set-config-test"; + meta = { + maintainers = [ ]; + }; + + nodes.machine = + { + config, + pkgs, + lib, + ... + }: + { + imports = [ cuaDriverModule ]; + virtualisation = { + cores = 2; + memorySize = 2048; + }; + services.cua-driver.enable = true; + environment.systemPackages = with pkgs; [ + xorg.xorgserver # Xvfb for headless X11 + python3 # MCP client test script + ]; + }; + + testScript = '' + machine.start() + machine.wait_for_unit("multi-user.target") + + with subtest("Binary exists and runs"): + machine.succeed("cua-driver --help") + + with subtest("Start Xvfb"): + machine.execute("Xvfb :99 -screen 0 1280x1024x24 >/dev/null 2>&1 &") + machine.wait_until_succeeds("test -e /tmp/.X11-unix/X99", timeout=10) + + with subtest("set_config {key, value} persists and reads back via get_config"): + machine.copy_from_host("${setConfigTest}", "/tmp/set-config-test.py") + result = machine.succeed( + "timeout 60 env DISPLAY=:99 " + "python3 /tmp/set-config-test.py 2>&1" + ) + machine.log(result) + assert "set_config persistence test passed" in result, f"set_config test failed: {result}" + ''; +}