Skip to content

fix(dialog): propagate hotkey failures in Go-to-Folder navigation#206

Open
SebTardif wants to merge 3 commits into
openclaw:mainfrom
SebTardif:fix/dialog-hotkey-cancellation
Open

fix(dialog): propagate hotkey failures in Go-to-Folder navigation#206
SebTardif wants to merge 3 commits into
openclaw:mainfrom
SebTardif:fix/dialog-hotkey-cancellation

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

navigateViaGoToFolder() in DialogService+FileDialogNavigation.swift uses try? on two critical input-synthesis calls: Cmd+Shift+G (open Go-to-Folder sheet) and Cmd+A (select existing text). If either hotkey fails, the error is silently discarded and the method continues, typing the directory path into whatever field currently has focus and pressing Return.

Problem

When InputDriver.hotkey fails for Cmd+Shift+G (line 212), the Go-to-Folder sheet never opens. The method then:

  1. Re-asserts dialog focus (line 216)
  2. Sends Cmd+A to select text (line 218, also try?)
  3. Types the directory path (line 220)
  4. Presses Return (line 221)

Without the Go-to-Folder sheet, these keystrokes act on the main dialog, potentially navigating to a wrong location or triggering an unintended action. All three callers (lines 145, 175, 183) use try await, expecting errors to propagate.

Fix

Replace try? with try on both hotkey calls. The method already declares async throws, and all three callers already handle thrown errors via try await. Adjacent calls in the same method (typeTextValue at line 220, tapKey at line 221) already use try.

Origin

Introduced in commit abf29e33 (Peter Steinberger, 2026-05-07, "refactor(dialog): split file dialog helpers"). The try? was likely carried over from a different context during the refactoring. Present for 54 days.

Related

Real behavior proof

  • Behavior addressed: try? on Cmd+Shift+G and Cmd+A hotkeys in navigateViaGoToFolder() silently discards input-synthesis failures. If the Go-to-Folder sheet fails to open, the method types the path into the wrong field and submits the dialog. The unfixed pattern executes 4 dangerous steps (wait, select-all, type path, press Return) against the main dialog instead of the Go-to-Folder sheet.

  • Real environment tested: macOS 15.5, Swift 6.3.3 (swiftlang-6.3.3.1.3), built from patched source on fix/dialog-hotkey-cancellation branch.

  • Exact steps or command run after this patch:

    cd /Users/sebastientardif/Peekaboo
    git checkout fix/dialog-hotkey-cancellation
    
    # Step 1. Compile standalone proof script
    swiftc -parse-as-library -o /tmp/prove-dialog \
      scripts/prove-dialog-hotkey-propagation.swift
    
    # Step 2. Run proof
    /tmp/prove-dialog
    
    # Step 3. Verify fix in source
    grep -n 'InputDriver.hotkey' \
      Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/UI/DialogService+FileDialogNavigation.swift
  • Evidence after fix: terminal output from the standalone error-propagation proof:

    $ swiftc -parse-as-library -o /tmp/prove-dialog scripts/prove-dialog-hotkey-propagation.swift && /tmp/prove-dialog
    === Dialog hotkey error propagation proof ===
    Reproduces navigateViaGoToFolder() behavior from
    DialogService+FileDialogNavigation.swift:206-223
    
    Test 1: UNFIXED pattern (try? swallows hotkey failure)
      Result: method COMPLETED (no error thrown to caller)
      Steps executed after failed Cmd+Shift+G:
        - sleep(250ms) -- waited for sheet that never opened
        - sleep(75ms)
        - typeTextValue("/Users/demo/Documents")
        - tapKey(.return)
      ⚠️  Path was typed into the WRONG field (Go-to sheet never opened)
    
    Test 2: FIXED pattern (try propagates hotkey failure)
      Result: method THREW: Hotkey synthesis failed: cmd+shift+g
      Steps executed: 0 (none; error stopped execution)
      ✅ Caller sees the failure; no path typed into wrong field
    
    ✅ PASS: Unfixed pattern silently continued past hotkey failure
             and executed 4 dangerous steps (type + submit in wrong field)
    ✅ PASS: Fixed pattern stopped immediately with a thrown error
             and executed 0 steps after the failed hotkey
    ✅ The try (not try?) propagation prevents typing a directory path
       into an unintended field when the Go-to-Folder sheet fails to open.

    Fix verified in source (both hotkey calls now use try instead of try?):

    $ grep -n 'InputDriver.hotkey' Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/UI/DialogService+FileDialogNavigation.swift
    158:            try? InputDriver.hotkey(keys: ["cmd", "a"], holdDuration: 0.05)
    212:        try InputDriver.hotkey(keys: ["cmd", "shift", "g"], holdDuration: 0.05)
    218:        try InputDriver.hotkey(keys: ["cmd", "a"], holdDuration: 0.05)

    Line 158 is a separate best-effort call in the path-text-field fallback (not part of this fix). Lines 212 and 218 now propagate errors.

  • Observed result after fix: The unfixed pattern completes silently after a hotkey failure, executing 4 steps (wait for phantom sheet, select-all in wrong context, type path, submit) against the main dialog. The fixed pattern throws immediately on hotkey failure, executing 0 subsequent steps. The caller receives the error and can retry or report, instead of silently submitting the path to the wrong UI element.

  • What was not tested: Live macOS Accessibility UI automation with an active file dialog (requires accessibility permissions and a running application with an open NSSavePanel/NSOpenPanel). The standalone proof script reproduces the exact control flow and error propagation semantics of the production method without requiring Accessibility entitlements.

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 3:14 PM ET / 19:14 UTC.

Summary
This PR changes two Go-to-Folder InputDriver.hotkey calls in dialog navigation from try? to try and adds a standalone Swift proof script.

Reproducibility: yes. at source level: current main discards the two throwing hotkey calls and then types and submits the path. I did not establish a live file-dialog reproduction with an induced production InputDriver failure.

Review metrics: 2 noteworthy metrics.

  • Production hotkeys changed: 2 call sites changed. The runtime surface is small, but both edits change dialog navigation failure behavior from swallowed errors to thrown errors.
  • Standalone proof added: 1 script added, 109 lines. The proof is useful context, but it is simulated and not durable regression coverage.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add production-path proof: a live file-dialog run, redacted runtime log, or focused harness that injects hotkey failure through DialogService/InputDriver.
  • Redact private details before posting proof, then update the PR body so ClawSweeper can re-review.
  • Consider replacing or supplementing the standalone proof script with focused regression coverage if the codebase can support an injectable production-path test.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body includes copied terminal output from a standalone simulated proof, but it does not show after-fix behavior through a live file dialog or production DialogService/InputDriver path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A short desktop proof of file-dialog path navigation with an induced hotkey failure would materially improve confidence for this UI automation change. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify a file-dialog Go-to-Folder hotkey failure stops path navigation with an error instead of typing the path into the dialog.

Risk before merge

  • [P1] The PR changes Go-to-Folder navigation from best-effort continuation to fail-fast on synthetic hotkey errors, so workflows that currently get past a transient hotkey failure will now return an error before path typing.
  • [P1] The supplied proof is terminal output from a standalone control-flow simulation, not a live file-dialog run or production DialogService/InputDriver injection.
  • [P1] The added proof script is not wired into regression tests, so maintainer acceptance would rely on source review and temporary proof unless production-path coverage is added.

Maintainer options:

  1. Prove the production failure path before merge (recommended)
    Ask for a live file-dialog run, redacted runtime log, or production-code harness that injects a hotkey failure and shows navigation stops before typing or submitting the path.
  2. Accept source-level proof explicitly
    Maintainers can choose to accept the source inspection and standalone harness for this small change while owning the fail-fast behavior change.
  3. Preserve compatibility if fail-fast is too strict
    If current workflows must remain best-effort by default, pause this PR and request a guarded fallback or explicit strict mode instead.

Next step before merge

  • [P1] The remaining blocker is contributor production-path proof or maintainer acceptance of the fail-fast behavior, not a narrow code repair for automation.

Security
Cleared: The diff changes Swift error propagation and adds a local proof script; it does not touch dependencies, CI, credentials, package resolution, or release paths.

Review details

Best possible solution:

Production dialog navigation should stop before typing or submitting when Go-to-Folder hotkey synthesis fails, backed by production-path proof or focused regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main discards the two throwing hotkey calls and then types and submits the path. I did not establish a live file-dialog reproduction with an induced production InputDriver failure.

Is this the best way to solve the issue?

Yes for the production code: changing the two critical hotkey calls to try is the narrowest fix because the helper already throws and its callers already use try await. The merge path still needs production-path proof or explicit maintainer acceptance of the fail-fast behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against b0184ad3ef88.

Label changes

Label justifications:

  • P2: This is a normal-priority dialog automation correctness fix with limited blast radius.
  • merge-risk: 🚨 compatibility: The PR changes existing file-dialog behavior from silent continuation to an early thrown error when synthetic hotkey delivery fails.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body includes copied terminal output from a standalone simulated proof, but it does not show after-fix behavior through a live file dialog or production DialogService/InputDriver path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Introduced the split file-dialog navigation helper and current blame on the relevant Go-to-Folder lines traces through the release snapshot authored by the same person. (role: feature-history owner; confidence: high; commits: abf29e331a64, 1fa8eead7eea; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/UI/DialogService+FileDialogNavigation.swift)
  • SebTardif: Authored merged current-main fixes for nearby swallowed-error/cancellation patterns in capture and daemon code, though not prior main ownership of this dialog path. (role: adjacent bug-pattern contributor; confidence: medium; commits: db5192bb370b, ca727e7528b3; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/WatchCaptureSession+Loop.swift, Apps/CLI/Sources/PeekabooCLI/Commands/Base/Runtime/DaemonLaunchPolicy.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 1, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

SebTardif added 3 commits July 1, 2026 12:05
Indent main() body to match repository SwiftFormat 4-space
indentation convention (was at column 1).

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif SebTardif force-pushed the fix/dialog-hotkey-cancellation branch from 496ffb1 to e201d16 Compare July 1, 2026 19:05
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant