mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-19 13:35:44 +00:00
fix: fast-fail terminal actionability failures and unredact check identifiers
Two root-cause fixes surfaced by a real session trace where clicking a
non-clickable group hung the full 5s auto-wait budget and returned an opaque
TIMEOUT whose failing check was unreadable.
Issue 1 (observability): ActionabilityCheck's check-identifier field was named
'name', colliding with the sensitive 'name' key the trace sanitizer redacts
(element accessible names, incl. Occluder.name). So the bounded-vocabulary
check identifier (visible/supported_action/...) was scrubbed from traces. Root
fix: rename the field to 'check' — a token the sanitizer leaves readable —
disambiguating it from Occluder.name, which stays correctly redacted. The
sanitizer keeps failing closed; the non-sensitive field just no longer collides.
Issue 2 (behavior): check_with_stability collapsed every actionability failure
to ACTION_FAILED, so the poll loop retried structurally-permanent failures for
the whole budget then returned a generic TIMEOUT. Root fix: model the intrinsic
transient-vs-terminal nature of each check. The terminal checks (supported_action,
policy, editable) — which waiting cannot heal — now carry their semantic permanent
code (ACTION_NOT_SUPPORTED / POLICY_DENIED); the report derives the error code
from the failing checks. The auto-wait poll loop's existing is_permanent_error
then fails them fast with a precise code, no poll-loop change. Transient checks
(visible/stable/enabled/receives_events) still surface ACTION_FAILED and retry.
Verified on a real app: click of a non-clickable scrollarea now returns
ACTION_NOT_SUPPORTED in 0.09s (was ~5s TIMEOUT) with the trace showing
{check: supported_action, reason: 'Click is not available'}. Full workspace tests
pass, e2e 72/0.
This commit is contained in:
parent
78b587aaf0
commit
8f434ddf65
9 changed files with 123 additions and 27 deletions
|
|
@ -1,15 +1,29 @@
|
|||
use super::ActionabilityStatus;
|
||||
use crate::error::ErrorCode;
|
||||
use crate::node::Rect;
|
||||
use serde::Serialize;
|
||||
|
||||
/// One actionability gate result. `check` is the gate's stable identifier
|
||||
/// (`visible`, `stable`, `enabled`, `supported_action`, `policy`, `editable`,
|
||||
/// `receives_events`) — a bounded vocabulary token, deliberately NOT keyed
|
||||
/// `name` so `sanitize_trace_value` leaves it readable in traces (unlike
|
||||
/// `Occluder.name`, which is a real element name and must stay redacted).
|
||||
/// `terminal_code`, set only on a failing check whose failure is permanent
|
||||
/// (waiting cannot heal it, e.g. an unsupported action or a policy denial),
|
||||
/// carries the error code the caller should surface; it is not serialized —
|
||||
/// its effect is that the auto-wait poll loop fails fast instead of retrying
|
||||
/// to the deadline. Transient failures (offscreen, unstable, disabled,
|
||||
/// occluded) leave it `None` and remain retryable.
|
||||
#[derive(Debug, Clone, Serialize, PartialEq)]
|
||||
pub struct ActionabilityCheck {
|
||||
pub name: &'static str,
|
||||
pub check: &'static str,
|
||||
pub status: ActionabilityStatus,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub reason: Option<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub occluder: Option<Occluder>,
|
||||
#[serde(skip)]
|
||||
pub terminal_code: Option<ErrorCode>,
|
||||
}
|
||||
|
||||
/// The element a hit test actually landed on when it failed to reach the
|
||||
|
|
|
|||
|
|
@ -106,14 +106,18 @@ fn check_with_stability(
|
|||
if report.actionable {
|
||||
return Ok(report);
|
||||
}
|
||||
let code = report.terminal_code().unwrap_or(ErrorCode::ActionFailed);
|
||||
let suggestion = if code == ErrorCode::ActionFailed {
|
||||
"Wait for the target to become actionable, refresh the snapshot, or use an explicit physical/focus command if intended."
|
||||
} else {
|
||||
"Waiting will not help: this element cannot satisfy the action as targeted. Target an element that advertises the action (check available_actions in a fresh snapshot) or adjust the interaction policy (e.g. pass --headed)."
|
||||
};
|
||||
Err(AdapterError::new(
|
||||
ErrorCode::ActionFailed,
|
||||
code,
|
||||
format!("Target is not actionable: {}", failure_reasons(&report)),
|
||||
)
|
||||
.with_details(json!(report))
|
||||
.with_suggestion(
|
||||
"Wait for the target to become actionable, refresh the snapshot, or use an explicit physical/focus command if intended.",
|
||||
))
|
||||
.with_suggestion(suggestion))
|
||||
}
|
||||
|
||||
fn visibility_check(entry: &RefEntry) -> ActionabilityCheck {
|
||||
|
|
@ -177,18 +181,27 @@ fn action_supported_check(entry: &RefEntry, request: &ActionRequest) -> Actionab
|
|||
);
|
||||
}
|
||||
let expected = capability::for_action(&request.action).join(" or ");
|
||||
fail("supported_action", format!("{expected} is not available"))
|
||||
fail_terminal(
|
||||
"supported_action",
|
||||
format!("{expected} is not available"),
|
||||
ErrorCode::ActionNotSupported,
|
||||
)
|
||||
}
|
||||
|
||||
fn policy_check(request: &ActionRequest) -> ActionabilityCheck {
|
||||
if request.action.requires_cursor_policy() && !request.policy.allow_cursor_move {
|
||||
return fail(
|
||||
return fail_terminal(
|
||||
"policy",
|
||||
"action requires cursor movement but policy denies it",
|
||||
ErrorCode::PolicyDenied,
|
||||
);
|
||||
}
|
||||
if request.action.may_use_focus_fallback() && !request.policy.allow_focus_steal {
|
||||
return fail("policy", "action requires focus but policy denies it");
|
||||
return fail_terminal(
|
||||
"policy",
|
||||
"action requires focus but policy denies it",
|
||||
ErrorCode::PolicyDenied,
|
||||
);
|
||||
}
|
||||
pass("policy")
|
||||
}
|
||||
|
|
@ -206,7 +219,11 @@ fn editable_check(entry: &RefEntry, action: &Action) -> ActionabilityCheck {
|
|||
if capability::contains(&entry.available_actions, capability::SET_VALUE) {
|
||||
return pass("editable");
|
||||
}
|
||||
fail("editable", format!("role {} is not editable", entry.role))
|
||||
fail_terminal(
|
||||
"editable",
|
||||
format!("role {} is not editable", entry.role),
|
||||
ErrorCode::ActionNotSupported,
|
||||
)
|
||||
}
|
||||
|
||||
fn receives_events_check(
|
||||
|
|
@ -274,7 +291,7 @@ fn failure_reasons(report: &ActionabilityReport) -> String {
|
|||
.filter(|check| matches!(check.status, ActionabilityStatus::Fail))
|
||||
.map(|check| {
|
||||
let reason = check.reason.as_deref().unwrap_or("failed");
|
||||
format!("{} ({reason})", check.name)
|
||||
format!("{} ({reason})", check.check)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ")
|
||||
|
|
@ -284,30 +301,50 @@ fn may_use_fallback(action: &Action, request: &ActionRequest) -> bool {
|
|||
action.may_use_focus_fallback() && request.policy.allow_focus_steal
|
||||
}
|
||||
|
||||
fn pass(name: &'static str) -> ActionabilityCheck {
|
||||
fn pass(check: &'static str) -> ActionabilityCheck {
|
||||
ActionabilityCheck {
|
||||
name,
|
||||
check,
|
||||
status: ActionabilityStatus::Pass,
|
||||
reason: None,
|
||||
occluder: None,
|
||||
terminal_code: None,
|
||||
}
|
||||
}
|
||||
|
||||
fn fail(name: &'static str, reason: impl Into<String>) -> ActionabilityCheck {
|
||||
fn fail(check: &'static str, reason: impl Into<String>) -> ActionabilityCheck {
|
||||
ActionabilityCheck {
|
||||
name,
|
||||
check,
|
||||
status: ActionabilityStatus::Fail,
|
||||
reason: Some(reason.into()),
|
||||
occluder: None,
|
||||
terminal_code: None,
|
||||
}
|
||||
}
|
||||
|
||||
fn unknown(name: &'static str, reason: impl Into<String>) -> ActionabilityCheck {
|
||||
/// A failure that waiting cannot heal — the element's role/action set or the
|
||||
/// interaction policy would have to change. Carries the permanent `code` the
|
||||
/// auto-wait poll loop surfaces immediately instead of retrying to the deadline.
|
||||
fn fail_terminal(
|
||||
check: &'static str,
|
||||
reason: impl Into<String>,
|
||||
code: ErrorCode,
|
||||
) -> ActionabilityCheck {
|
||||
ActionabilityCheck {
|
||||
name,
|
||||
check,
|
||||
status: ActionabilityStatus::Fail,
|
||||
reason: Some(reason.into()),
|
||||
occluder: None,
|
||||
terminal_code: Some(code),
|
||||
}
|
||||
}
|
||||
|
||||
fn unknown(check: &'static str, reason: impl Into<String>) -> ActionabilityCheck {
|
||||
ActionabilityCheck {
|
||||
check,
|
||||
status: ActionabilityStatus::Unknown,
|
||||
reason: Some(reason.into()),
|
||||
occluder: None,
|
||||
terminal_code: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -321,10 +358,11 @@ fn occluded(
|
|||
None => "occluded by another element".to_string(),
|
||||
};
|
||||
ActionabilityCheck {
|
||||
name: "receives_events",
|
||||
check: "receives_events",
|
||||
status: ActionabilityStatus::Fail,
|
||||
reason: Some(reason),
|
||||
occluder: Some(Occluder { role, name, bounds }),
|
||||
terminal_code: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
use super::ActionabilityCheck;
|
||||
use super::{ActionabilityCheck, ActionabilityStatus};
|
||||
use crate::error::ErrorCode;
|
||||
use serde::Serialize;
|
||||
|
||||
#[derive(Debug, Clone, Serialize, PartialEq)]
|
||||
|
|
@ -6,3 +7,16 @@ pub struct ActionabilityReport {
|
|||
pub actionable: bool,
|
||||
pub checks: Vec<ActionabilityCheck>,
|
||||
}
|
||||
|
||||
impl ActionabilityReport {
|
||||
/// The error code for a non-actionable report: the first failing check that
|
||||
/// declared its failure terminal (permanent — waiting cannot heal it), or
|
||||
/// `None` when every failure is transient and the action should be retried
|
||||
/// within the auto-wait budget.
|
||||
pub(crate) fn terminal_code(&self) -> Option<ErrorCode> {
|
||||
self.checks
|
||||
.iter()
|
||||
.filter(|check| matches!(check.status, ActionabilityStatus::Fail))
|
||||
.find_map(|check| check.terminal_code.clone())
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -241,7 +241,7 @@ fn live_actionability_fails_when_action_disappears_after_snapshot() {
|
|||
)
|
||||
.unwrap_err();
|
||||
|
||||
assert_eq!(err.code, ErrorCode::ActionFailed);
|
||||
assert_eq!(err.code, ErrorCode::ActionNotSupported);
|
||||
assert!(err.message.contains("supported_action"));
|
||||
}
|
||||
|
||||
|
|
@ -271,7 +271,7 @@ fn live_actionability_allows_identity_resolved_bounds_change() {
|
|||
let stable = report
|
||||
.checks
|
||||
.iter()
|
||||
.find(|check| check.name == "stable")
|
||||
.find(|check| check.check == "stable")
|
||||
.unwrap();
|
||||
assert_eq!(stable.status, ActionabilityStatus::Unknown);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -135,6 +135,7 @@ fn text_input_requires_editable_target() {
|
|||
)
|
||||
.unwrap_err();
|
||||
|
||||
assert_eq!(err.code, ErrorCode::ActionNotSupported);
|
||||
assert!(err.message.contains("editable"));
|
||||
}
|
||||
|
||||
|
|
@ -142,6 +143,7 @@ fn text_input_requires_editable_target() {
|
|||
fn cursor_movement_requires_physical_policy() {
|
||||
let err = check(&entry(), &ActionRequest::headless(Action::Hover)).unwrap_err();
|
||||
|
||||
assert_eq!(err.code, ErrorCode::PolicyDenied);
|
||||
assert!(err.message.contains("policy"));
|
||||
}
|
||||
|
||||
|
|
@ -157,6 +159,7 @@ fn headless_type_text_fails_policy_before_dispatch() {
|
|||
)
|
||||
.unwrap_err();
|
||||
|
||||
assert_eq!(err.code, ErrorCode::PolicyDenied);
|
||||
assert!(err.message.contains("policy"));
|
||||
assert!(err.message.contains("focus"));
|
||||
}
|
||||
|
|
@ -165,7 +168,7 @@ fn headless_type_text_fails_policy_before_dispatch() {
|
|||
fn right_click_requires_right_click_capability_before_dispatch() {
|
||||
let err = check(&entry(), &ActionRequest::headless(Action::RightClick)).unwrap_err();
|
||||
|
||||
assert_eq!(err.code, ErrorCode::ActionFailed);
|
||||
assert_eq!(err.code, ErrorCode::ActionNotSupported);
|
||||
assert!(err.message.contains("supported_action"));
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -211,11 +211,18 @@ fn element_wait_actionable_type_fails_on_uneditable_role() {
|
|||
)
|
||||
.unwrap_err();
|
||||
|
||||
assert_eq!(err.code(), "TIMEOUT");
|
||||
assert_eq!(err.code(), "ACTION_NOT_SUPPORTED");
|
||||
match err {
|
||||
AppError::Adapter(adapter_error) => {
|
||||
let details = adapter_error.details.unwrap();
|
||||
assert_eq!(details["last_observed"]["actionable"], false);
|
||||
assert_eq!(details["actionable"], false);
|
||||
let editable = details["checks"]
|
||||
.as_array()
|
||||
.expect("a terminal actionability failure carries the check report")
|
||||
.iter()
|
||||
.find(|check| check["check"] == "editable")
|
||||
.expect("the editable check is reported");
|
||||
assert_eq!(editable["status"], "fail");
|
||||
}
|
||||
_ => panic!("expected adapter error"),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -90,7 +90,7 @@ fn run_receives_events_check(outcome: Result<HitTestResult, AdapterError>) -> Ac
|
|||
report
|
||||
.checks
|
||||
.into_iter()
|
||||
.find(|check| check.name == "receives_events")
|
||||
.find(|check| check.check == "receives_events")
|
||||
.expect("Click requires a receives_events check")
|
||||
}
|
||||
|
||||
|
|
@ -151,7 +151,7 @@ fn intercepted_by_result_fails_and_carries_redactable_occluder() {
|
|||
.expect("details.checks is an array");
|
||||
let receives_events = checks
|
||||
.iter()
|
||||
.find(|check| check["name"] == "receives_events")
|
||||
.find(|check| check["check"] == "receives_events")
|
||||
.expect("receives_events check must be present when Click requires a hit test");
|
||||
assert_eq!(receives_events["status"], "fail");
|
||||
assert_eq!(receives_events["occluder"]["name"], "Save changes?");
|
||||
|
|
|
|||
|
|
@ -56,3 +56,23 @@ fn trace_redaction_covers_nested_shapes_and_substring_keys() {
|
|||
assert!(value["action"]["password"].is_null());
|
||||
assert_eq!(value["action"]["counter"], 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trace_keeps_actionability_check_identifier_but_redacts_occluder_name() {
|
||||
let value = sanitize_trace_value(json!({
|
||||
"checks": [
|
||||
{ "check": "supported_action", "status": "fail", "reason": "Click is not available" },
|
||||
{
|
||||
"check": "receives_events",
|
||||
"status": "fail",
|
||||
"occluder": { "role": "AXSheet", "name": "Save changes?" }
|
||||
}
|
||||
]
|
||||
}));
|
||||
|
||||
assert_eq!(value["checks"][0]["check"], "supported_action");
|
||||
assert_eq!(value["checks"][0]["reason"], "Click is not available");
|
||||
assert_eq!(value["checks"][1]["check"], "receives_events");
|
||||
assert_eq!(value["checks"][1]["occluder"]["role"], "AXSheet");
|
||||
assert_eq!(value["checks"][1]["occluder"]["name"]["redacted"], true);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -59,11 +59,11 @@ All ref-based interaction commands accept `--snapshot <snapshot_id>`. Omit it fo
|
|||
|
||||
Success responses for ref actions include a `steps` array when the activation chain recorded attempts: each entry is `{ "label": "AXPress", "outcome": "attempted" | "skipped" | "succeeded" }` in execution order, showing which activation path produced the result.
|
||||
|
||||
When the actionability preflight blocks an action, the error envelope carries the full report in `error.details`: `{ "actionable": false, "checks": [ { "name": "...", "status": "...", "reason": "..." } ] }`. Check names are `visible`, `stable`, `enabled`, `supported_action`, `policy`, `editable`, and `receives_events`; statuses are `pass`, `fail`, and `unknown`. The dispatch actions that activate an element (`click`, `double-click`, `right-click`, `triple-click`, `type`, `set-value`, `select`, `toggle`, `check`, `uncheck`, `expand`, `collapse`, `clear`, `focus`, `scroll`, `scroll-to`) run the `visible`/`stable`/`enabled`/`supported_action`/`policy`/`editable` battery; the four hit-test variants (`click`, `double-click`, `right-click`, `triple-click`) additionally run `receives_events`. `hover` and `drag` are different — they resolve a target point and run **only** the `receives_events` occlusion check, so their report's `checks` array holds that single entry (no `visible`/`enabled`/etc.). Use the failing check's `reason` to pick recovery: `wait --element <ref> --predicate actionable`, a fresh snapshot, or `--headed` when a `policy` check failed and a physical gesture is intended.
|
||||
When the actionability preflight blocks an action, the error envelope carries the full report in `error.details`: `{ "actionable": false, "checks": [ { "check": "...", "status": "...", "reason": "..." } ] }`. The `check` identifiers are `visible`, `stable`, `enabled`, `supported_action`, `policy`, `editable`, and `receives_events`; statuses are `pass`, `fail`, and `unknown`. Failures split by whether waiting can help: the **transient** checks (`visible`, `stable`, `enabled`, `receives_events`) can change over time — scroll into view, settle, become enabled, occlusion clears — so they surface as `ACTION_FAILED` and are retried within `--timeout-ms`; the **terminal** checks (`supported_action`, `policy`, `editable`) cannot be healed by waiting (the element's role/action set or the interaction policy would have to change), so they fail fast with a precise code — `ACTION_NOT_SUPPORTED` (`supported_action`/`editable`) or `POLICY_DENIED` (`policy`) — instead of polling to `TIMEOUT`. The dispatch actions that activate an element (`click`, `double-click`, `right-click`, `triple-click`, `type`, `set-value`, `select`, `toggle`, `check`, `uncheck`, `expand`, `collapse`, `clear`, `focus`, `scroll`, `scroll-to`) run the `visible`/`stable`/`enabled`/`supported_action`/`policy`/`editable` battery; the four hit-test variants (`click`, `double-click`, `right-click`, `triple-click`) additionally run `receives_events`. `hover` and `drag` are different — they resolve a target point and run **only** the `receives_events` occlusion check, so their report's `checks` array holds that single entry (no `visible`/`enabled`/etc.). Use the failing check's `reason` to pick recovery: `wait --element <ref> --predicate actionable`, a fresh snapshot, or `--headed` when a `policy` check failed and a physical gesture is intended.
|
||||
|
||||
**`receives_events` failures.** When a hit test at the target's center point lands on a different element, `receives_events` fails with `reason: "occluded by <role>"` and a structured `occluder` object on that check: `{ "role", "name", "bounds" }` (the element that actually received the hit, when it can be identified). The target's own bounds have not changed — something else is now on top of them. Recovery is to bring the target's window or element to the front (or dismiss whatever is covering it), then retry; blind-retrying without changing z-order will fail the same way again.
|
||||
|
||||
Every ref-resolving action accepts `--timeout-ms` (default `5000`), but it budgets different things. For the dispatch actions (`click`, `double-click`, `triple-click`, `right-click`, `clear`, `focus`, `toggle`, `check`, `uncheck`, `expand`, `collapse`, `scroll-to`, `type`, `set-value`, `select`, `scroll`) it is the actionability-wait budget: they poll roughly every 100ms until the target becomes actionable, then fail with `TIMEOUT` once the budget is exhausted. For `hover` and `drag` it budgets the ref-*resolution* retry instead — only `STALE_REF`/`AMBIGUOUS_TARGET`/`TIMEOUT` are retried within the budget; a `receives_events` occlusion failure on hover/drag is returned immediately as `ACTION_FAILED`, not polled to `TIMEOUT`.
|
||||
Every ref-resolving action accepts `--timeout-ms` (default `5000`), but it budgets different things. For the dispatch actions (`click`, `double-click`, `triple-click`, `right-click`, `clear`, `focus`, `toggle`, `check`, `uncheck`, `expand`, `collapse`, `scroll-to`, `type`, `set-value`, `select`, `scroll`) it is the actionability-wait budget: they poll roughly every 100ms until the target becomes actionable, then fail with `TIMEOUT` once the budget is exhausted — unless the block is a terminal check (`supported_action`/`policy`/`editable`), which fails fast on the first attempt with `ACTION_NOT_SUPPORTED`/`POLICY_DENIED` rather than waiting out the budget. For `hover` and `drag` it budgets the ref-*resolution* retry instead — only `STALE_REF`/`AMBIGUOUS_TARGET`/`TIMEOUT` are retried within the budget; a `receives_events` occlusion failure on hover/drag is returned immediately as `ACTION_FAILED`, not polled to `TIMEOUT`.
|
||||
|
||||
**Implicit scroll-into-view.** Before acting, every ref action other than `scroll`, `scroll-to`, `hover`, and `drag` automatically attempts to bring an offscreen or zero-bounds target into view (macOS: `AXScrollToVisible`) before dispatching the action. This is best-effort and silent — it has no separate error code, and a failed attempt does not block the action itself, it just proceeds without having scrolled. Use the standalone `scroll-to` command when you need an explicit, verifiable scroll instead of relying on this implicit step.
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue