feat: optional (best-effort) clash setup steps — click-if-present, else skip

[guohai]

What

Adds an optional: true flag to agent-profile setup steps (SetupStep). When set, if the step's selector never appears within its timeout, the step is skipped (no-op) instead of throwing and failing the match.

Why

Debugging live clash matches surfaced this: the Agora ConvoAI demo shows a one-time "Personal Data Protection Guide" → Agree modal that only appears for accounts that haven't consented yet.

• A hard click "Agree" step fails (timeout) once the account is consented and the modal stops rendering.
• Omitting it fails for a fresh account, where the modal blocks "Call AI Engine".

An optional step handles both states: click when present, no-op when absent. Generalizes to any conditional UI.

How

In the step loop, if step.optional && step.selector, probe with waitForSelector({ state: "visible" }); on timeout, log and continue instead of running the action.

Usage

{ "action": "click", "selector": "button:text-is('Agree')", "optional": true, "timeout": 8000 }

Notes

• Non-optional steps are unchanged (still fail-fast).
• Typecheck clean; 64 clash unit tests pass.
• Runner-side — image rebuilds on merge, then vox-upgrade.sh.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• vox_clash_runner/browser-agent.ts:120 catches every error from waitForSelector and treats it as “not present.” That can silently skip real failures for optional steps, such as an invalid selector, page/frame closure, browser crash, or Playwright protocol error. This makes misconfigured setup profiles look successful and can leave the match in the wrong state. Consider only swallowing Playwright timeout errors and rethrowing everything else.

Open Questions

• Was the intent specifically “selector timed out,” or should optional also suppress malformed selectors/runtime page errors? The PR description says timeout/no-op, so I’m assuming only timeout should be skipped.

No security or performance issues found beyond that error-swallowing edge case.

[github-actions]

🤖 Claude Code Review

The change is small, well-scoped, and correct. Optional steps that reference a selector wait up to timeout for visibility and cleanly skip on absence; the happy path falls through to the existing switch unchanged. No security concerns — selectors/values come from the same config+secrets source as before, so the trust model is unchanged.

Two minor observations, neither blocking:

• optional is silently ignored when a step has no selector. The guard is step.optional && step.selector, so an optional press/wait-by-timeout step still runs unconditionally. That matches the documented use case (consent modals are selector-based), but if someone marks a selector-less step optional they'll get non-obvious behavior. A one-line comment or a warn log would make the limitation explicit.

• Redundant double waitForSelector on the present path (browser-agent.ts:120 then :133/:139/:151). When an optional selector is present, it's waited for twice. The second call returns immediately since the element is already visible, so this is a negligible cost, not a bug — noting only for completeness.

Nothing else stands out. The per-absent-step timeout cost (default 10s each) is inherent to best-effort probing and is an acceptable trade-off for this feature. Looks good to merge.

[github-actions]

🤖 Codex Code Review

No findings.

• The optional-step behavior is scoped to selector-based steps and only swallows Playwright timeout failures, so invalid selectors/page crashes still fail fast.
• I don’t see security, logic, or performance regressions in vox_clash_runner/browser-agent.ts.
• I did not run tests because the environment is read-only; review was based on the PR diff and current file contents.

[github-actions]

🤖 Claude Code Review

I reviewed the diff (the optional best-effort setup step feature in vox_clash_runner/browser-agent.ts).

This is a clean, well-scoped change. The core design decision — only swallowing TimeoutError and rethrowing everything else (invalid selector, closed page, crash) — is exactly right and prevents the "silently swallow real failures" trap that best-effort logic usually falls into. The comments explain the why well.

No security concerns: selectors/values are the same config-sourced data as before (no new injection surface), and the log line prints step.selector but not step.value, so resolved secrets don't leak into logs.

A few minor notes, none blocking:

• Redundant waitForSelector for present optional steps (lines 121 vs 137/143/155). When an optional element is present, it's waited-for twice: once in the pre-check, then again inside the click/fill/wait case. Harmless (second wait resolves immediately since it's already visible) and no double-timeout on the skip path, but it's dead motion. Not worth restructuring for.

• Cost of absent optional steps. Each optional step that isn't present costs the full timeout (default 10s) on every run, since it must wait out the timeout to conclude "not present." Fine for a one-time consent modal, but if several optional steps are configured and rarely appear, per-run latency adds up. Worth keeping optional-step timeouts short in the YAML config. Consider documenting that.

• optional only gates when selector is set (line 119). For selector-less actions (press, or wait with only timeout), optional is silently ignored and the step always runs. That's the sensible interpretation ("nothing to detect as absent"), but the field's JSDoc doesn't say so — a config author could reasonably expect optional to also soften a bare wait/press. A one-line note in the doc comment would close that gap.

Overall: correct, defensive, and the error-handling refinement in commit 18d1c5c is the right call. Good to merge.