fix(config): expose cua idle timeout in dashboard - #8075
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
default.py, all other CUA sandbox defaults are sourced fromCUA_DEFAULT_CONFIG, butcua_idle_timeoutis hardcoded to0; consider wiring this throughCUA_DEFAULT_CONFIGas well for consistency and to avoid divergence from the backend default. - The
hintforprovider_settings.sandbox.cua_idle_timeoutindefault.pyis in Chinese while thedescriptionis English; if this field is surfaced directly to users (rather than only via i18n), consider aligning the base-language text with the surrounding config metadata for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `default.py`, all other CUA sandbox defaults are sourced from `CUA_DEFAULT_CONFIG`, but `cua_idle_timeout` is hardcoded to `0`; consider wiring this through `CUA_DEFAULT_CONFIG` as well for consistency and to avoid divergence from the backend default.
- The `hint` for `provider_settings.sandbox.cua_idle_timeout` in `default.py` is in Chinese while the `description` is English; if this field is surfaced directly to users (rather than only via i18n), consider aligning the base-language text with the surrounding config metadata for consistency.
## Individual Comments
### Comment 1
<location path="astrbot/core/config/default.py" line_range="182" />
<code_context>
"cua_image": CUA_DEFAULT_CONFIG["image"],
"cua_os_type": CUA_DEFAULT_CONFIG["os_type"],
"cua_ttl": CUA_DEFAULT_CONFIG["ttl"],
+ "cua_idle_timeout": 0,
"cua_telemetry_enabled": CUA_DEFAULT_CONFIG["telemetry_enabled"],
"cua_local": CUA_DEFAULT_CONFIG["local"],
</code_context>
<issue_to_address>
**suggestion:** Consider sourcing the default value from `CUA_DEFAULT_CONFIG` instead of hardcoding `0`.
Other CUA defaults here come from `CUA_DEFAULT_CONFIG`, but `cua_idle_timeout` is hardcoded. Referencing a config field instead will keep defaults centralized and reduce the chance of this value drifting if the configuration changes in the future.
Suggested implementation:
```python
"cua_ttl": CUA_DEFAULT_CONFIG["ttl"],
"cua_idle_timeout": CUA_DEFAULT_CONFIG["idle_timeout"],
"cua_telemetry_enabled": CUA_DEFAULT_CONFIG["telemetry_enabled"],
```
1. Ensure that `CUA_DEFAULT_CONFIG` in the same module (or wherever it is defined) includes a key like `"idle_timeout"` (or adjust the key name here to match the actual configuration, e.g. `"cua_idle_timeout"` or `"sandbox_idle_timeout"`).
2. If the configuration key name differs, update the `"cua_idle_timeout": CUA_DEFAULT_CONFIG["idle_timeout"],` line to reference the correct key name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a new cua_idle_timeout configuration setting for the CUA sandbox, including metadata updates and translations for the dashboard. Feedback suggests centralizing the default value using CUA_DEFAULT_CONFIG for consistency and ensuring the backend logic is updated to utilize this new parameter, supported by additional unit tests.
| "cua_image": CUA_DEFAULT_CONFIG["image"], | ||
| "cua_os_type": CUA_DEFAULT_CONFIG["os_type"], | ||
| "cua_ttl": CUA_DEFAULT_CONFIG["ttl"], | ||
| "cua_idle_timeout": 0, |
There was a problem hiding this comment.
For consistency with other CUA configuration fields in this dictionary, it is recommended to use CUA_DEFAULT_CONFIG to provide the default value for cua_idle_timeout. This ensures that default values are centralized in cua_defaults.py and improves maintainability.
Additionally, please verify that the backend logic (e.g., CuaBooter and build_cua_booter_kwargs in astrbot/core/computer/booters/cua.py) is updated to actually consume this new setting, as it currently appears to be ignored in those areas. Furthermore, as this is new functionality, please ensure it is accompanied by corresponding unit tests.
| "cua_idle_timeout": 0, | |
| "cua_idle_timeout": CUA_DEFAULT_CONFIG.get("idle_timeout", 0), |
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
|
@sourcery-ai review |
* fix(config): expose cua idle timeout in dashboard * fix(config): remove exposed cua ttl setting * fix(config): centralize cua idle timeout default
* fix(config): expose cua idle timeout in dashboard * fix(config): remove exposed cua ttl setting * fix(config): centralize cua idle timeout default
Summary
cua_idle_timeoutto sandbox config metadata so the dashboard can render the settingProblem
The CUA idle-timeout follow-up added behavior and configuration defaults, but the dashboard configuration layer was incomplete. The new
provider_settings.sandbox.cua_idle_timeoutsetting was not exposed through config metadata and had no frontend i18n entries, so users could not discover or edit it from the WebUI.Fix
This PR wires the setting through the remaining dashboard-facing layers:
cua_idle_timeoutto the default config mapprovider_settings.sandbox.cua_idle_timeoutmetadata alongside the other CUA sandbox settingszh-CN,en-US, andru-RUValidation
uv run pytest tests/unit/test_cua_computer_use.py tests/unit/test_computer.py -quv run ruff check astrbot/core/config/default.py tests/unit/test_cua_computer_use.pySummary by Sourcery
Expose the CUA sandbox idle-timeout setting through the dashboard configuration metadata and align defaults and tests with the CUA booter configuration.
Bug Fixes:
Enhancements: