From 076c93d9d48e3f268b69dc3e9f2113158ce457a7 Mon Sep 17 00:00:00 2001 From: Jacob Fu <141651335+FuJacob@users.noreply.github.com> Date: Mon, 1 Jun 2026 22:33:48 -0700 Subject: [PATCH] Fix three bugs found in a pre-release audit 1. Multi-line completions were silently suppressed. InsertionSafetyGate rejected any control character below 0x20, including the line feed the normalizer keeps in multi-line mode, so every multi-line completion normalized to empty and was never shown. Exempt 0x0A (newline); other control chars are still rejected. 2. Typing through a suggestion regenerated against pre-keystroke text in Chromium. completeActiveSuggestion always scheduled the next prediction immediately, but the typed-through-exhaustion path runs from the synchronous input tap before the host publishes the keystroke, so the new suggestion was built against stale text (the documented Chromium AX-publish race). It now waits for host publish on that path; the reconcile-path caller, where AX has settled, still schedules at once. 3. The opt-in paste path could clobber or leak the user's clipboard. The restore ran unconditionally after a fixed delay, so a copy during the window was overwritten, and two pastes within the window snapshotted each other's injected text and left a completion stuck on the clipboard. The restore now only runs if our completion is still on the clipboard (change-count guard), and overlapping pastes coalesce onto a single saved clipboard and reschedule the one restore. --- .../SuggestionCoordinator+Acceptance.swift | 13 +++++- .../Suggestion/SuggestionInserter.swift | 42 +++++++++++++++++-- Cotabby/Support/InsertionSafetyGate.swift | 7 ++-- CotabbyTests/InsertionSafetyGateTests.swift | 11 +++++ 4 files changed, 65 insertions(+), 8 deletions(-) diff --git a/Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift b/Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift index 36b459a..7132a27 100644 --- a/Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift +++ b/Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift @@ -319,6 +319,7 @@ extension SuggestionCoordinator { completeActiveSuggestion( reason: "Overlay hidden because the user typed through the rest of the suggestion.", scheduleNextPrediction: true, + awaitHostPublish: true, stage: "typed-match-exhausted", message: "The user typed the remaining suggestion characters exactly.", acceptanceAction: "User typed through the rest of the suggestion." @@ -356,6 +357,7 @@ extension SuggestionCoordinator { func completeActiveSuggestion( reason: String, scheduleNextPrediction: Bool, + awaitHostPublish: Bool = false, stage: String, message: String, acceptanceAction: String @@ -368,7 +370,16 @@ extension SuggestionCoordinator { logStage(stage, workID: currentWorkID, generation: generation, message: message) if scheduleNextPrediction { - schedulePrediction() + // Callers reacting to a *synthetic-tap* keystroke (the user typing through a suggestion) + // must wait for the host to publish the keystroke before regenerating, or the new + // suggestion is built against pre-keystroke text in Chromium editors and looks like the + // typed characters were ignored. Reconcile-path callers, where AX has already settled, + // schedule immediately. + if awaitHostPublish { + schedulePredictionAfterHostPublishDelay() + } else { + schedulePrediction() + } } } diff --git a/Cotabby/Services/Suggestion/SuggestionInserter.swift b/Cotabby/Services/Suggestion/SuggestionInserter.swift index bcafd0c..f6a249b 100644 --- a/Cotabby/Services/Suggestion/SuggestionInserter.swift +++ b/Cotabby/Services/Suggestion/SuggestionInserter.swift @@ -16,6 +16,13 @@ final class SuggestionInserter { private(set) var lastErrorMessage: String? + /// In-flight state for the opt-in paste path. The user's real clipboard is snapshotted once and + /// restored after the paste lands. While a restore is pending, a second paste must NOT re-snapshot + /// (the pasteboard then holds OUR completion, which would leak back to the user), so overlapping + /// pastes coalesce onto this single saved clipboard and reschedule the one pending restore. + private var pendingPasteboardRestore: DispatchWorkItem? + private var savedClipboardForRestore: [[NSPasteboard.PasteboardType: Data]]? + /// Virtual key code for Delete/Backspace. Posting these at the HID level deletes one UTF-16 unit /// of already-typed text per pair, which is how the picker removes the literal `:query` run. private static let backspaceKeyCode: CGKeyCode = 0x33 @@ -152,17 +159,28 @@ final class SuggestionInserter { /// ignores it. private func insertViaPaste(_ text: String) -> Bool { let pasteboard = NSPasteboard.general - let saved = Self.snapshotPasteboard(pasteboard) + // Snapshot the user's clipboard only when no restore is already pending. If one is (an + // overlapping paste), the pasteboard currently holds our previous completion, so re-snapshotting + // would save that and leak it back to the user; reuse the already-saved real clipboard. + if pendingPasteboardRestore == nil { + savedClipboardForRestore = Self.snapshotPasteboard(pasteboard) + } + let saved = savedClipboardForRestore ?? [] pasteboard.clearContents() guard pasteboard.setString(text, forType: .string) else { + pendingPasteboardRestore?.cancel() Self.restorePasteboard(saved, to: pasteboard) + clearPendingPasteboardRestore() return false } + let expectedChangeCount = pasteboard.changeCount guard let keyDown = CGEvent(keyboardEventSource: nil, virtualKey: Self.vKeyCode, keyDown: true), let keyUp = CGEvent(keyboardEventSource: nil, virtualKey: Self.vKeyCode, keyDown: false) else { + pendingPasteboardRestore?.cancel() Self.restorePasteboard(saved, to: pasteboard) + clearPendingPasteboardRestore() return false } keyDown.flags = .maskCommand @@ -173,13 +191,29 @@ final class SuggestionInserter { keyDown.post(tap: .cghidEventTap) keyUp.post(tap: .cghidEventTap) - // Give the host time to service Cmd-V, then hand the clipboard back to the user. - DispatchQueue.main.asyncAfter(deadline: .now() + Self.pasteboardRestoreDelay) { - Self.restorePasteboard(saved, to: NSPasteboard.general) + // Give the host time to service Cmd-V, then hand the clipboard back, but only if our completion + // is still the thing on it. If the user copied something during the window, `changeCount` + // advanced and we leave their new clipboard alone. An overlapping paste cancels this restore + // and reschedules one for the same saved clipboard. + pendingPasteboardRestore?.cancel() + let restore = DispatchWorkItem { [weak self] in + if NSPasteboard.general.changeCount == expectedChangeCount { + Self.restorePasteboard(saved, to: NSPasteboard.general) + } + self?.clearPendingPasteboardRestore() } + pendingPasteboardRestore = restore + DispatchQueue.main.asyncAfter(deadline: .now() + Self.pasteboardRestoreDelay, execute: restore) return true } + /// Clears the in-flight paste-restore bookkeeping once a restore has run (or been abandoned on a + /// failure path), so the next paste snapshots the user's real clipboard afresh. + private func clearPendingPasteboardRestore() { + pendingPasteboardRestore = nil + savedClipboardForRestore = nil + } + /// Captures every representation of every pasteboard item so the user's clipboard can be restored /// exactly, not just its plain-text form. private static func snapshotPasteboard(_ pasteboard: NSPasteboard) -> [[NSPasteboard.PasteboardType: Data]] { diff --git a/Cotabby/Support/InsertionSafetyGate.swift b/Cotabby/Support/InsertionSafetyGate.swift index fd10f3c..7b9d5b5 100644 --- a/Cotabby/Support/InsertionSafetyGate.swift +++ b/Cotabby/Support/InsertionSafetyGate.swift @@ -25,9 +25,10 @@ enum InsertionSafetyGate { if scalar == "\u{FFFD}" { return false } - // C0 control range and DEL. Newlines are already handled upstream; an interior tab or - // other control character is corruption, not content. - if scalar.value < 0x20 || scalar.value == 0x7F { + // C0 control range and DEL. A line feed is legitimate content in a multi-line completion + // (the normalizer keeps newlines when multi-line mode is on), so it must pass; any other + // control character (an interior tab, a stray escape) is corruption, not content. + if scalar.value != 0x0A, scalar.value < 0x20 || scalar.value == 0x7F { return false } if !CharacterSet.whitespacesAndNewlines.contains(scalar) { diff --git a/CotabbyTests/InsertionSafetyGateTests.swift b/CotabbyTests/InsertionSafetyGateTests.swift index 86cccfb..dd3d2a6 100644 --- a/CotabbyTests/InsertionSafetyGateTests.swift +++ b/CotabbyTests/InsertionSafetyGateTests.swift @@ -29,4 +29,15 @@ final class InsertionSafetyGateTests: XCTestCase { func test_interiorControlCharacter_isUnsafe() { XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("a\tb")) } + + func test_multiLineContent_isSafe() { + // A line feed is legitimate content in a multi-line completion, so it must pass (the previous + // behavior rejected it, silently suppressing every multi-line completion). + XCTAssertTrue(InsertionSafetyGate.isSafeToInsert("first line\nsecond line")) + } + + func test_newlineOnly_isUnsafe() { + // Newlines are whitespace; a newline-only completion is still nothing worth inserting. + XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("\n\n")) + } }