fix(cua-driver): persist set_config to disk on Windows + Linux (config parity) - #2034
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds config JSON helpers, loads ChangesDriver config persistence
Sequence Diagram(s)sequenceDiagram
participant ToolStateNew
participant SetConfigInvoke
participant ReadConfigValue
participant WriteConfigKey
participant ConfigFile
ToolStateNew->>ReadConfigValue: load capture_mode and max_image_dimension
ReadConfigValue->>ConfigFile: read JSON key
ConfigFile-->>ReadConfigValue: stored config data
ReadConfigValue-->>ToolStateNew: DriverConfig overrides
SetConfigInvoke->>WriteConfigKey: persist capture_mode or max_image_dimension
WriteConfigKey->>ConfigFile: write updated key
ConfigFile-->>WriteConfigKey: write result
WriteConfigKey-->>SetConfigInvoke: warn on failure, continue with in-memory cfg
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 2
🧹 Nitpick comments (1)
libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs (1)
29-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
load_driver_config()is duplicated verbatim inplatform-windows.This exact helper now exists in both
platform-linuxandplatform-windowstools/impl_.rs. Since both only depend onpip_preview::read_config_valueandDriverConfig, consider hoisting it (or the read logic) into thepip-previewcrate to keep the two platforms from drifting.🤖 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-linux/src/tools/impl_.rs` around lines 29 - 44, The load_driver_config helper is duplicated across the Linux and Windows platform tool implementations, so the config-loading logic can drift over time. Move the shared read-and-apply behavior behind a single reusable helper in pip_preview, or otherwise centralize it there, and have both platform-linux and platform-windows call that shared entry point while still populating DriverConfig from pip_preview::read_config_value.
🤖 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/pip-preview/src/lib.rs`:
- Around line 23-26: The default_config_path function is treating empty HOME or
USERPROFILE values as valid, which can produce a relative
.cua-driver/config.json path instead of None. Update default_config_path to
check the environment variables for both presence and non-empty content before
building the PathBuf, and return None when either HOME or USERPROFILE is unset
or empty so the function’s contract is preserved.
In `@libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs`:
- Around line 3429-3445: Validate the config inputs in the set_config path
before writing them out: in the branches handling capture_mode and
max_image_dimension, reject unsupported or overflowing values instead of
persisting them unchanged. For capture_mode, only accept the known string values
already recognized by the config loader and return a ToolResult::error for
anything else so unknown modes cannot be saved. For max_image_dimension, check
that the incoming u64 fits in u32 before assigning to cfg.max_image_dimension
and before calling pip_preview::write_config_key, and persist the same u32 value
that is stored in memory.
---
Nitpick comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs`:
- Around line 29-44: The load_driver_config helper is duplicated across the
Linux and Windows platform tool implementations, so the config-loading logic can
drift over time. Move the shared read-and-apply behavior behind a single
reusable helper in pip_preview, or otherwise centralize it there, and have both
platform-linux and platform-windows call that shared entry point while still
populating DriverConfig from pip_preview::read_config_value.
🪄 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: c39ad09e-3435-481c-abed-c1daee571b5d
📒 Files selected for processing (3)
libs/cua-driver/rust/crates/pip-preview/src/lib.rslibs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
Comment on lines
23
to
26
| pub fn default_config_path() -> Option<std::path::PathBuf> { | ||
| std::env::var("HOME") | ||
| .ok() | ||
| std::env::var_os("HOME") | ||
| .or_else(|| std::env::var_os("USERPROFILE")) | ||
| .map(|h| std::path::PathBuf::from(h).join(".cua-driver").join("config.json")) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat empty HOME / USERPROFILE as unset.
Line 24 and Line 25 use var_os directly, so HOME="" or USERPROFILE="" becomes .cua-driver/config.json relative to the current working directory instead of returning None. That breaks the documented contract here and can persist config to the wrong location.
Suggested fix
pub fn default_config_path() -> Option<std::path::PathBuf> {
std::env::var_os("HOME")
.or_else(|| std::env::var_os("USERPROFILE"))
+ .filter(|h| !h.is_empty())
.map(|h| std::path::PathBuf::from(h).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
| pub fn default_config_path() -> Option<std::path::PathBuf> { | |
| std::env::var("HOME") | |
| .ok() | |
| std::env::var_os("HOME") | |
| .or_else(|| std::env::var_os("USERPROFILE")) | |
| .map(|h| std::path::PathBuf::from(h).join(".cua-driver").join("config.json")) | |
| pub fn default_config_path() -> Option<std::path::PathBuf> { | |
| std::env::var_os("HOME") | |
| .or_else(|| std::env::var_os("USERPROFILE")) | |
| .filter(|h| !h.is_empty()) | |
| .map(|h| std::path::PathBuf::from(h).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/pip-preview/src/lib.rs` around lines 23 - 26, The
default_config_path function is treating empty HOME or USERPROFILE values as
valid, which can produce a relative .cua-driver/config.json path instead of
None. Update default_config_path to check the environment variables for both
presence and non-empty content before building the PathBuf, and return None when
either HOME or USERPROFILE is unset or empty so the function’s contract is
preserved.
Comment on lines
+3429
to
+3445
| Some(s) => { | ||
| cfg.capture_mode = s.to_owned(); | ||
| if let Err(e) = pip_preview::write_config_key("capture_mode", Value::String(s.to_owned())) { | ||
| tracing::warn!("set_config: failed to persist capture_mode: {e}"); | ||
| } | ||
| 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}")); } | ||
| Some(n) => { | ||
| cfg.max_image_dimension = n as u32; | ||
| if let Err(e) = pip_preview::write_config_key("max_image_dimension", Value::from(n)) { | ||
| tracing::warn!("set_config: failed to persist max_image_dimension: {e}"); | ||
| } | ||
| parts.push(format!("max_image_dimension={n}")); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# capture_mode consumers / validation in cua-driver
rg -nP --type=rust -C3 'capture_mode' libs/cua-driver/rust/crates | rg -nP -C3 'match|enum|som|vision|ax|parse|validate'Repository: trycua/cua
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs'
# Map the relevant symbols first, then inspect the smallest useful ranges.
ast-grep outline "$FILE" --view expanded | sed -n '1,220p'
printf '\n--- load_driver_config / set_config ranges ---\n'
rg -n -C 4 'fn load_driver_config|capture_mode|max_image_dimension|set_config|DriverConfig' "$FILE"Repository: trycua/cua
Length of output: 21516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs'
# Show the relevant code around config load and set_config.
sed -n '150,220p' "$FILE"
printf '\n---\n'
sed -n '5000,5065p' "$FILE"Repository: trycua/cua
Length of output: 3675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs'
python3 - <<'PY'
from pathlib import Path
file = Path("libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs")
text = file.read_text()
for needle in [
"pub fn load_driver_config()",
"\"capture_mode\" => match val.as_str()",
"\"max_image_dimension\" => match val.as_u64()",
]:
idx = text.find(needle)
print(f"== {needle} == {idx}")
if idx != -1:
start = max(0, text.rfind("\n", 0, idx-400))
end = text.find("\n", idx+900)
print(text[start:end])
print("\n---")
PYRepository: trycua/cua
Length of output: 4447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs'
echo '--- load_driver_config ---'
sed -n '169,190p' "$FILE"
echo
echo '--- set_config capture_mode / max_image_dimension ---'
sed -n '5030,5058p' "$FILE"
echo
echo '--- capture_mode consumers ---'
rg -n -C 2 'do_tree|do_shot|capture_mode !=|capture_mode ==' "$FILE"Repository: trycua/cua
Length of output: 2424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs'
# Narrowly inspect the exact file under review.
rg -n -C 3 'pub fn load_driver_config|capture_mode|max_image_dimension|set_config|do_tree|do_shot' "$FILE"Repository: trycua/cua
Length of output: 10954
Validate config values before persisting.
capture_modeis accepted as any string on the{key, value}path and reloaded unchanged. Unknown values don’t fail startup, but they are persisted and later behave likesombecause only"vision"and"ax"are special-cased.max_image_dimensiontruncatesu64 -> u32in memory, while the fullu64is written to disk. Values aboveu32::MAXcome back as the default on reload; reject overflow or persist the sameu32value.
🤖 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-linux/src/tools/impl_.rs` around lines
3429 - 3445, Validate the config inputs in the set_config path before writing
them out: in the branches handling capture_mode and max_image_dimension, reject
unsupported or overflowing values instead of persisting them unchanged. For
capture_mode, only accept the known string values already recognized by the
config loader and return a ToolResult::error for anything else so unknown modes
cannot be saved. For max_image_dimension, check that the incoming u64 fits in
u32 before assigning to cfg.max_image_dimension and before calling
pip_preview::write_config_key, and persist the same u32 value that is stored in
memory.
`set_config` (e.g. capture_mode) only mutated the in-memory `DriverConfig`
on Windows and Linux, so a value set in one `cua-driver call` process was
lost in the next — each stateless one-shot reverted to the default. macOS
already loads/saves `~/.cua-driver/config.json` at startup; this brings the
other two platforms to parity.
- pip-preview: `default_config_path` now falls back to `%USERPROFILE%` when
`HOME` is unset (Windows), and add `read_config_value(key)` so the
per-platform loaders can rehydrate `DriverConfig` from the shared file.
- platform-windows / platform-linux: add `load_driver_config()` (called from
`ToolState::new`) to load capture_mode + max_image_dimension on startup,
and persist both fields via `pip_preview::write_config_key` in `set_config`
(both the {key,value} and legacy per-field shapes).
Config now survives across separate `cua-driver call` invocations for all
persisted fields, fixing the desktop-scope footgun.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMXCW4M5uK1HRGjjH4wueZ
Co-authored-by: outdog-hwh <168953287+outdog-hwh@users.noreply.github.com>
f-trycua
force-pushed
the
fix/persist-config-win-linux
branch
from
June 26, 2026 03:38
3f0eb2c to
b58e7d4
Compare
Contributor
Linux visual regression artifactsMatrix jobs now run independently. Download visual artifacts from this workflow run.
|
f-trycua
added a commit
that referenced
this pull request
Jun 26, 2026
…t coverage (Phases 3-6) (#2043) * refactor(cua-driver): test-suite taxonomy, protocol split+dedup, transport test (Phases 3-6) Phase 3 — rename tests into a 4-family taxonomy (git mv, blame preserved): protocol_ / transport_ / harness_ / modality_ / guard_. harness_web_windows→harness_web, harness_lo_vcl→harness_libreoffice, harness_bg_modality→modality_background, harness_desktop_scope→modality_desktop_scope, focus_check→modality_focus, e2e_windows_bg_input→modality_input_e2e, ux_guard→guard_ux, element_token→protocol_element_token. Phase 5 — split mcp_protocol_test (3,412 lines / 65 tests) into 5 protocol_*_test files (handshake/tools_call/schema/media/session) on a new testkit RawDriver (raw send/recv, no auto-init). Deduped 24 macOS↔Windows mirror pairs into single cfg!-branching tests; 13 mac-only + 4 win-only kept. All 40 macOS protocol tests pass post-split. Phase 4 — add transport_config_persistence_test: set_config persists to disk across stateless CLI invocations (#2034) vs visible within an MCP session — the one behavior only observable across both transports. Phase 6 — rewrite TEST_SUITE.md / TEST_HARNESS_STRUCTURE.md to the new names + a transport × modality × platform coverage matrix. Deferred (noted): merging modality_input_e2e ↔ modality_background, and extracting the focus-steal sentinel into a shared testkit assert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(cua-driver): fix stale Windows protocol assertions surfaced by running them The old mcp_protocol Windows mirror asserted serverInfo.name="cua-driver-rs" and a roster with type_text_chars+screenshot — both stale (the Windows mirror had never actually run). The live Windows server returns name="cua-driver" and registers neither extra tool. Drop the bogus platform branches; the macOS values are correct for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(cua-driver): fix 3 more stale Windows protocol branches found by running Running the split suite on Windows surfaced three never-run stale assertions: - schema: the Windows server has no type_text_chars tool (hidden on both platforms), so merge the two type_text_chars schema tests into one. - media set_config_screenshot_resize: skip the resize assertion when the screenshot is unavailable (Session 0 / no display) on BOTH platforms, not just macOS. - session multi_cursor: Windows get_agent_cursor_state returns ALL cursors (macOS scopes to one); verify each cursor's state within the full list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
Persist
set_configto disk on Windows and Linux so config survives across statelesscua-driver callinvocations — bringing them to parity with macOS, which already does this. Addresses #2011 (Windows config disagreement) and fixes the footgun where the desktop-scope flow (#2019) couldn't holdcapture_scopeacross CLI calls.The bug
Each
cua-driver callis a fresh process. On macOS,ToolState::default()loads~/.cua-driver/config.jsonat startup andset_configwrites keys back, so config persists across calls. On Windows/Linux,ToolState::new()builtDriverConfig::default()with no disk load, andset_configmutated only the in-memory config forcapture_mode/max_image_dimension(onlyexperimental_pip*ever touched disk). So a separatecua-driver call get_configreverted to defaults (som). This is exactly whyset_config capture_scope=desktopthen a separateclickdidn't take effect.Changes (3 files)
crates/pip-preview/src/lib.rs(the already-shared config helper):default_config_path()now falls back to%USERPROFILE%whenHOMEis unset. Windows runs withHOMEempty, so without this the config file can't even be resolved there — meaning the existing pip persistence was latently broken on Windows too.read_config_value(key)so per-platform loaders can read arbitrary keys.crates/platform-windows/.../tools/impl_.rs&crates/platform-linux/.../tools/impl_.rs:load_driver_config()(mirrors macOS), wired intoToolState::new()to loadcapture_mode+max_image_dimensionfrom disk at startup.set_confignow persists both fields viapip_preview::write_config_key, in both input shapes ({key,value}and legacy per-field). Persist failureswarn!and continue, matching macOS.platform-macosis untouched (already correct).Verified live (two separate
callprocesses)cua-gnome-test): freshget_config→som;set_config capture_mode=visionin one process; separateget_config→vision. On-disk/root/.cua-driver/config.json={"capture_mode":"ax","max_image_dimension":800}.fbonacci-windows-vm, withHOMEempty): same round-trip persists; on-diskC:\Windows\system32\config\systemprofile\.cua-driver\config.jsonconfirms the%USERPROFILE%fallback works.(Tested with
capture_modebecausecapture_scopeonly exists on the #2019 branch — it exercises the identical persistence path.)Relationship to other PRs
cua-driver configdisagrees withconfig get, MCPget_config, and persistedconfig.json#2011 — addresses the Windows config inconsistency for both Windows and Linux.config_file_path()keys only onHOME(errors"$HOME is not set"when unset), so it silently no-ops on stock Windows; its own test forcesHOME=…, masking the gap. This PR adds the%USERPROFILE%fallback and is runtime-verified on Windows withHOMEempty.capture_scopeto each platform'sload_driver_config()+ theset_configpersist calls (~2 lines/platform, same pattern) so desktop-scope survives across processes.Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01KMXCW4M5uK1HRGjjH4wueZ
Summary by CodeRabbit
New Features
Bug Fixes