W-557: stop a hung frontmost app from dropping keystrokes (+ deterministic freeze harness)#177
Conversation
AppContextDetector.current() runs inside the CGEventTap callback on the main thread and makes synchronous cross-process IPC to the frontmost app: the cold-cache system-wide AX query, the focused-field secure/editable attribute reads, and the non-Arc browser AX URL walk. With the process-wide 0.5s AX messaging timeout, a hung/beach-balling app can stack those past the ~1s tap timeout, so macOS disables the tap and drops the keystroke — the same dropped-keystroke class as W-547 (Safari lag) and W-555 (Arc), just via the AX path rather than AppleScript. - Pin every tap-path AX query to a tight 0.1s messaging timeout (early-abort the browser walk on the first miss). Real local AX answers in single-digit ms, so this only bites a genuinely unresponsive app — exactly when we must bail rather than block the tap. - Move the Arc URL AppleScript refresh fully off the main thread: an osascript subprocess on a background queue, killed after an 800ms budget. The earlier W-555 design ran NSAppleScript on the main thread, so a *frozen* Arc would wedge Mojito's whole main thread (tap + UI) until the Apple Event timed out (~2 min). Off-thread + bounded, a hung Arc stalls only that worker. Verified by SIGSTOP-freezing the frontmost app (Arc, and TextEdit as a non-browser control) while typing: pre-fix drops keystrokes (tapLost timeout), this build does not. Refs: W-557
Bounding each tap-path AX query (prior commit) keeps any single AppContextDetector.current() under the tap timeout, but on sustained typing into a hung app the per-word calls still stacked up enough cumulative main-thread block to trip it — measured ~1 dropped keystroke per 8-word burst against a frozen Arc. Cache the secure/editable classification in FocusedElementCache, keyed to the focused element and invalidated whenever focus moves (element.didSet). current() now does the AX role/settable/range IPC at most once per focus and reads cached booleans on every subsequent keystroke — zero cross-process IPC on the hot path, so a frozen frontmost app can't stall the tap no matter how much you type. With this, SIGSTOP-freezing Arc (or TextEdit) and typing a 12-word burst drops zero keystrokes; the pre-fix build drops on every burst. Refs: W-557
The old arc-typing-regression.sh typed into fresh Arc tabs and asserted zero tapLost. Validated on a real Arc install, it could not discriminate a fixed build from a buggy one: on fast, idle hardware Arc answers the URL/AX IPC in milliseconds, so nothing drops whether or not the blocking bug is present — a false green, exactly what a regression harness must not produce. Replace it with hung-app-typing-regression.sh, which SIGSTOPs the frontmost app so it cannot answer any IPC, types into it, then SIGCONTs, asserting zero keyMonitor tapLost. A build that does blocking cross-process IPC on the tap thread drops keystrokes every time (RED); a correct build never does (GREEN). Deterministic, no timing luck. Targets Arc (AppleScript URL + AX paths) and TextEdit (a non-browser control isolating the AX path). Keeps the guardrails: warmup+probe (proves keystrokes reach the tap, settles Arc's Automation grant), build-from-worktree, hard-fail on driver errors, and an EXIT trap that always thaws every target. Verified: RED against the pre-W-557 build (Arc and TextEdit both drop), GREEN against the fix. Refs: W-556, W-557
There was a problem hiding this comment.
CI is still in-progress (ssot pipeline) — not blocking this review, diff analysis is independent.
Blocking
1. Multi-line comment blocks + W-ticket references in AppContext.swift (CLAUDE.md violation)
Two new blocks in this file break the rule "Never write multi-line comment blocks — one short line max" and "Don't reference the current task, fix, or callers (W-NN) — that belongs in commit messages / PR descriptions and rots as the codebase evolves."
Block 1 — top of AppContextDetector (lines 25–34): The 10-line /// comment explains the timing constraint. The constraint is non-obvious and worth a comment, but should be one line, no ticket refs:
// Tight bound: a slow/hung app's AX calls block this main-thread tap callback past macOS's ~1s timeout, dropping keystrokes.
static let tapAXTimeout: Float = 0.1Block 2 — inside current(), "Classify the field…" (5 lines): Same issue, trim to:
// Cached per focus to avoid repeated AX IPC on the tap thread; invalidated on any focus change.2. Multi-line comment block + W-ticket reference in FocusedElementCache.swift (CLAUDE.md violation)
The new 8-line /// block on haveFieldInfo (lines 30–37) has the same violations. Suggestion:
// Cached per focus to skip repeated AX IPC on the tap thread; reset whenever element changes.
private(set) var haveFieldInfo = false3. Multi-line comment block in BrowserURL.swift osascriptURL (CLAUDE.md violation)
The new 4-line // block inside osascriptURL ("This DFS walk runs on the tap thread…") should be one line:
// Tight timeout: a hung browser can't stall the tap past its ~1s limit via this AX walk.
AXUIElementSetMessagingTimeout(app, AppContextDetector.tapAXTimeout)Question
drive hotkey command l for TextEdit in run_target — For Arc this correctly opens the URL bar. For TextEdit, Cmd+L isn't a standard text-focus shortcut (there's no URL bar). The text area should already be focused after make new document. Does this hotkey do anything useful for TextEdit, or is it a harmless leftover from the Arc-centric design? If it opens the character panel or triggers some other side effect it could skew the freeze test.
Nits
eval "$activate"inrun_targetworks fine for hard-coded strings, butevalis unnecessarily powerful here — passing the app name and constructing theosascriptcall inside the function would be a safer idiom.- The
--phraseflag (useful for manual debugging) was present inarc-typing-regression.shbut dropped from the new harness; the phrase is now hard-coded. Minor flexibility loss.
Three files had new multi-line `///`/`//` blocks and W-ticket refs in comments — violating the "one short line max, no ticket refs" rule. Trimmed each to the non-obvious constraint it was trying to convey. Refs: W-557
Superseded by fix commit(s) from claude[bot] (pr-fix, trace=223a1d53).
…und the walk, harden the harness Cross-provider (Codex) review of the branch found real defects; all fixed: Fix — security & correctness: - Field classification is no longer computed lazily on the tap thread keyed to a bare bool. It's computed OFF the main thread in the FocusedElementCache seed and on each focus-change notification, published only if focus hasn't moved on (CFEqual guard), and reset on any element change. current() is now a pure cache read. Closes the window where a system-wide-fallback classification could be served for a *different* element than it described — a path that could capture a password field (Codex blocker). - current() FAILS CLOSED: an unclassified field reads as secure (don't capture), not editable — never begin capture on a field we haven't verified isn't a password field (Codex high). Removes the synchronous system-wide fallback entirely, which also stops clobbering the process-wide 0.5s AX timeout that other code (caret locator) depends on (Codex medium). - The non-Arc browser AX URL walk is now actually bounded: every node sets its own messaging timeout (a timeout on the app element does NOT propagate to descendants) plus a total wall-clock deadline (Codex high). - Arc URL resolve distinguishes "resolved(nil)" from "unavailable" (hung/killed): a transient stall keeps the last good URL instead of erasing a cached excluded-site URL and letting a denylisted page through (Codex medium). Output goes to a temp file, not a Pipe, so a long URL can't fill the 64KB buffer and manufacture a timeout (Codex medium). Harness: - Activate via `osascript -e`, not `eval` (which ran `tell` as a shell command, exit 127 — activation silently never happened), and verify the target is frontmost before freezing, so the phrase can't land in another app and pass (Codex high). - Never touch a pre-existing TextEdit: skip the control target if it's already running, and only quit-discard a TextEdit this run launched (Codex high). Tests: resolver returns the outcome enum; suite serialized; fixed sleeps replaced with bounded polling; added a keep-last-value-on-unavailable test (Codex medium). Verified: unit suite green; harness RED on the pre-fix build (Arc + TextEdit), GREEN on this build; discrimination intact. Refs: W-557
There was a problem hiding this comment.
Round 2 follow-up — prior findings partially addressed; key CLAUDE.md violations persist and one new correctness bug introduced.
CI: ssot pipeline still in-progress — not blocking this review; diff analysis is independent.
Blocking
1. AppContext.swift lines 24–38 — multi-line /// block + W-ticket refs (new)
CLAUDE.md: "Never write multi-line comment blocks — one short line max" and "Don't reference the current task, fix, or callers (W-NN)." This 15-line /// block above current() violates both. The commit 71f9ae0 addressed some earlier instances; this block was added in their place.
The constraint is non-obvious and worth a comment — but one line, no ticket refs:
// Tap callback: all reads must be cache-hits — no synchronous cross-process IPC; a hung app stalls past the ~1s timeout and macOS drops the keystroke.
static func current() -> ActiveContext {2. FocusedElementCache.swift lines 30–39 — multi-line /// block + W-ticket ref (repeat from prior review #2)
The prior review flagged this as blocking. 71f9ae0 claims to trim it, but this 10-line block with a W-557 reference is still present. Suggested:
// Cached per focus to skip repeated AX IPC on the tap thread; reset on any element change (see element.didSet).
private(set) var focusedIsSecure = false
private(set) var focusedIsEditable = false
private(set) var haveFieldInfo = false3. BrowserURL.swift lines 33–40 — multi-line // block + W-ticket ref in detect() (new)
8-line // block with a W-557 reference. The "why both bounds are needed" is non-obvious and worth a comment, but one line:
// Per-object timeout + total deadline: either bound alone can be beaten (one slow node vs. many fast ones).4. BrowserURL.swift lines 200–204 — multi-line /// block + W-ticket ref on BrowserURLResolution (new)
6-line /// with W-557 on a new type. One line:
/// `.unavailable` means the resolve timed out — callers must keep the last good value, not overwrite it with nil.
enum BrowserURLResolution: Sendable {5. BrowserURL.swift lines 359–368 — multi-line /// block on osascriptURL (repeat from prior review #3)
The prior review flagged this as blocking. Still an 11-line /// block. One line:
// osascript subprocess so it can be killed; temp file instead of Pipe to avoid the 64 KB buffer deadlock.
private nonisolated static func osascriptURL(bundleID: String) -> BrowserURLResolution {6. BrowserURL.swift lines 372–374 — temp file not cleaned up on FileHandle failure (correctness bug, new)
The defer on line 374 registers after the guard on line 373. If FileHandle(forWritingTo: outURL) fails, execution returns .unavailable before the defer runs, leaving the file created on line 372 in /tmp. Fix: move the defer above the guard:
FileManager.default.createFile(atPath: outURL.path, contents: nil)
defer { try? FileManager.default.removeItem(at: outURL) } // always runs now
guard let outHandle = try? FileHandle(forWritingTo: outURL) else { return .unavailable }Question
scripts/e2e/hung-app-typing-regression.sh line 177 — drive hotkey command l for TextEdit (repeat from prior review, not addressed)
For Arc, Cmd+L focuses the URL bar. For TextEdit there is no URL bar; Cmd+L may open a Go-to-Line dialog or do nothing. Either way the tapLost signal is still valid (the test is about IPC blocking, not about where the text lands), but if Cmd+L opens a dialog that intercepts the typed phrase it could confuse a developer debugging a failure. Intentional ("just focus something")? If so, a comment here would help; if not, a TextEdit-specific harmless action (or nothing at all, since the text area is already focused after make new document) would be cleaner.
Nits
AppContext.swiftlines 52–54 (3-line//insidecurrent()), 68–71 (4-line///forclassify()), 76–78 (3-line///forisSecure()) — all still violate the one-line rule.FocusedElementCache.swiftlines 23–25 (3-line///forelement), 44–46 (3-line///forpublishFieldInfo), 53–55 (3-line///forreclassify) — same.BrowserURL.swiftlines 57–59 (3-line///forwalkAXTimeout/walkBudget), 183–185 (3-line//incopyAttributewith W-557 ref), 246–247 (2-line///forresolveBudget), 210–229 (theBrowserURLCacheclass doc comment — 20 lines with multiple W-ticket refs, modified in this PR) — same.
All three files violated CLAUDE.md's one-line comment rule with multi- line /// / // blocks, several citing W-ticket numbers. Collapsed each to the non-obvious constraint it was protecting. Also fixes a correctness bug in osascriptURL: the defer that removes the temp file was registered after the guard that could return early on FileHandle failure, leaving the file in /tmp on that path. Moved defer above the guard so it always runs. Refs: W-557 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Pushed fixes addressing the blocking findings on PR #177. (trace: 223a1d53) |
Superseded by fix commit(s) from claude[bot] (pr-fix, trace=223a1d53).
… cases Follow-up cross-provider review of the revision (7/9 prior findings closed) flagged new edges; fixed the actionable ones: - Harness now PROVES the target froze: after kill -STOP it checks the process is in state `T` and bails inconclusive otherwise, so a failed STOP can't type into a live app and report a false PASS (the harness's cardinal sin). - Arc URL resolve treats a nonzero osascript exit as `.unavailable` (keep the last cached URL), not `.resolved(nil)` — a transient AppleScript/TCC/"no active tab" error no longer clears a valid excluded-site URL and lets a denylisted page through. - Bound the temp-file cleanup before the FileHandle guard (no leak on open failure). - Non-Arc walk checks the wall-clock deadline before the window read and the final value read, not just inside the recursion. - reclassify bails before its AX round-trips when focus has already moved on again (generation check), so a focus storm can't build a serial backlog. - Deterministic unavailable-keeps-value test (clock-driven throttle, no reschedule race). Documented the one residual the reviewer flagged as blocker/high: for AX-opaque apps whose focus-changed observer never registers, a within-app move to a secure field isn't seen until the next app-activation re-seed. Fully closing it needs synchronous re-verification on the tap thread — the exact stall this change removes — so the app/URL exclusion list stays the backstop for password contexts, as the secure-field detection has always documented. current() still fails closed on every app switch and every observed focus move. Verified: unit suite green; harness RED on pre-fix build, GREEN on this build, freeze-verification active. Refs: W-557
There was a problem hiding this comment.
Round 3 follow-up — previous findings partially addressed; blocking comment-style violations still present (some newly introduced by the aa75418 Codex pass).
CI: ssot pipeline still in-progress — not blocking this review; diff analysis is independent.
Fixed ✓
- R2 #1 (
AppContext.swiftmulti-line abovecurrent()) — trimmed to one line in492772a. ✓ - R2 #6 (temp file leak on
FileHandlefailure inosascriptURL) —deferis now placed before theguard let outHandleso a failed open can't leave a file in/tmp. ✓
Blocking — still unresolved
1. FocusedElementCache.swift lines 39–48 — 10-line /// block with W-557 reference (new in aa75418)
CLAUDE.md: "Never write multi-line comment blocks — one short line max" + "Don't reference the current task, fix, or callers (W-NN)."
The block above focusedIsSecure/focusedIsEditable/haveFieldInfo is 10 lines and contains a W-557 reference. Suggested one-liner:
// Cached per focus to skip AX IPC on the tap thread; reset on element change (see element.didSet), fail-closed until set.
private(set) var focusedIsSecure = false
private(set) var focusedIsEditable = false
private(set) var haveFieldInfo = false2. FocusedElementCache.swift lines 62–74 — 14-line /// block on reclassify (new in aa75418)
Same rules. The "inherent limit" note at lines 66–74 is non-obvious and worth keeping, but as one line:
// Classifies off-thread; publishes iff focus hasn't moved. AX-opaque apps that miss within-app moves rely on the exclusion list as backstop.
private nonisolated func reclassify(_ element: AXUIElement) {3. BrowserURL.swift lines 33–40 — 8-line // block with W-557 in detect() (repeat from R2 #3)
Prior review flagged this as blocking; still present. Suggested:
// Two bounds: per-object timeout (does not propagate to descendants) + total deadline (caps a deep tree of fast-but-numerous nodes).
let deadline = Date().addingTimeInterval(Self.walkBudget)4. BrowserURL.swift lines 204–208 — 5-line /// block with W-557 on BrowserURLResolution (repeat from R2 #4)
Prior review flagged this as blocking; still present. Suggested:
/// `.unavailable` means the resolve timed out — callers must keep the last good value, not overwrite it with nil.
enum BrowserURLResolution: Sendable {5. BrowserURL.swift lines 363–372 — 11-line /// block on osascriptURL (repeat from R1 and R2 #5)
Flagged as blocking in both prior rounds; still 11 lines. Suggested:
// osascript subprocess: runs off main, killable if Arc hangs. Temp file instead of Pipe avoids the 64 KB buffer deadlock.
private nonisolated static func osascriptURL(bundleID: String) -> BrowserURLResolution {Question (repeat from R2, still unanswered)
hung-app-typing-regression.sh run_target — drive hotkey command l for TextEdit.
For Arc, Cmd+L focuses the URL bar. TextEdit has no URL bar; the key may open the Go-to-Line dialog or do nothing. The tapLost signal is still valid regardless (the test is about IPC blocking, not where the text lands), but if Cmd+L opens a modal it could confuse a developer debugging a failure. Intentional as "just invoke a hotkey before freezing"? If so, a comment at the call site would help; if not, skipping it for the TextEdit target (or substituting a harmless key) would be cleaner.
Nits (same class — not re-filed as blocking)
AppContext.swift:classify4-line///(lines ~47–50),isSecure3-line///, 3-line//insidecurrent()— all one-liner violations (carried over from R2 nit list).FocusedElementCache.swift:element3-line///(lines 23–25),fieldGeneration4-line///(lines 33–36),publishFieldInfo3-line///(lines 53–55) — same.BrowserURL.swift:walkAXTimeout/walkBudget3-line///(lines 57–59),copyAttribute3-line//withW-557ref (lines 187–189),resolveBudget2-line///(lines 250–251),scheduleRefresh5-line///(lines 326–330),BrowserURLCacheclass doc 20-line///with W-ticket refs (lines 214–233) — same.
Refs: W-557 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Pushed fixes addressing the blocking findings on PR #177. (trace: 223a1d53) |
Superseded by fix commit(s) from claude[bot] (pr-fix, trace=223a1d53).
Problem
W-555 moved Arc's URL AppleScript off the synchronous CGEventTap path, but the same tap callback (
AppContextDetector.current(), on the main thread) still made other synchronous cross-process IPC to the frontmost app — the cold-cache system-wide AX query, the focused-field secure/editable attribute reads, and the non-Arc browser AX URL walk. When that app is slow or hung, the IPC blocks the tap past the ~1s system timeout and macOS drops the keystroke — the same class as W-547 (Safari lag) and W-555 (Arc), via the AX path.Found while validating the W-556 harness on a real Arc install: freezing the frontmost app (
SIGSTOP) and typing reproduced the drop deterministically — and it reproduced on a non-browser (TextEdit) too, proving it's the AX queries, not anything Arc-specific.Fix
Make the tap callback unable to block on a hung app:
FocusedElementCache, keyed to the focused element, invalidated on any focus change.current()now does the role/settable/range IPC at most once per focus and reads cached booleans on every subsequent keystroke — bounding alone wasn't enough, because per-word calls still stacked cumulative main-thread block on a long typing burst.osascriptsubprocess on a background queue, killed after an 800ms budget. The W-555 design ranNSAppleScripton the main thread, so a frozen Arc would wedge Mojito's whole main thread (tap + UI) until the Apple Event timed out (~2 min); off-thread + bounded, a hung Arc stalls only that worker.Harness (replaces the W-556 one)
The old
arc-typing-regression.shtyped into fresh Arc tabs and asserted zero tapLost — but on fast, idle hardware Arc answers in ms, so it passed whether or not the bug was present (a false green, confirmed on a real Arc install). Replaced withhung-app-typing-regression.sh, whichSIGSTOPs the frontmost app so it cannot answer IPC, types, thenSIGCONTs. A blocking build drops keystrokes every time (RED); a correct build never does (GREEN). Targets Arc (AppleScript + AX) and TextEdit (AX-only control). Keeps the warmup/probe, build-from-worktree, hard-fail-on-driver-error, and always-thaw guardrails.Test plan
BrowserURLCacheTestsfor the now-off-main resolver).Refs: W-557, W-555, W-556, W-547