Fix three bugs found in a pre-release audit#534
Merged
Conversation
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 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 { |
Contributor
There was a problem hiding this comment.
&& semantics with an infix || inside the second clause. The logic is correct, but a reader who doesn't notice that the comma binds looser than || could misread it as (scalar.value != 0x0A, scalar.value < 0x20) || scalar.value == 0x7F. Using && with explicit parentheses makes the grouping unambiguous.
Suggested change
| if scalar.value != 0x0A, scalar.value < 0x20 || scalar.value == 0x7F { | |
| if scalar.value != 0x0A && (scalar.value < 0x20 || scalar.value == 0x7F) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+39
to
43
| func test_newlineOnly_isUnsafe() { | ||
| // Newlines are whitespace; a newline-only completion is still nothing worth inserting. | ||
| XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("\n\n")) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
now passes and that a newline-only string still fails, but there is no explicit test showing that (0x0D) is still rejected. is stripped by SuggestionTextNormalizer before it can reach the gate in production, but a gate-level test would make the 0x0A-only exemption self-documenting and guard against future normalizer changes that might pass through.
Suggested change
| func test_newlineOnly_isUnsafe() { | |
| // Newlines are whitespace; a newline-only completion is still nothing worth inserting. | |
| XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("\n\n")) | |
| } | |
| } | |
| func test_newlineOnly_isUnsafe() { | |
| // Newlines are whitespace; a newline-only completion is still nothing worth inserting. | |
| XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("\n\n")) | |
| } | |
| func test_carriageReturn_isUnsafe() { | |
| // Only 0x0A is exempted; \r (0x0D) is still a rejected C0 control character. | |
| XCTAssertFalse(InsertionSafetyGate.isSafeToInsert("a\rb")) | |
| } | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A pre-release bug audit (fanned out across the runtime, coordinator, insertion, support helpers, and settings). Three confirmed bugs fixed; two findings deliberately left alone and explained below. No new features.
Fixed
cotabbyMultiLineEnabledis a user-facing toggle, butInsertionSafetyGaterejected every control char< 0x20, including the line feed the normalizer keeps in multi-line mode. So multi-line completions always normalized to empty and never appeared. Now0x0Apasses; tab/other C0/U+FFFDare still rejected. Tests added.completeActiveSuggestionalways calledschedulePrediction(), but the typed-through-exhaustion caller runs from the synchronous input tap before the host publishes the keystroke. The next suggestion was built against stale text and looked like the typed chars were ignored (the exact Chromium AX-publish race thatschedulePredictionAfterHostPublishDelayexists to fix). That path now awaits host publish; the reconcile-path caller, where AX has settled, still schedules immediately.changeCount(only restore if our completion is still there) and overlapping pastes coalesce onto one saved clipboard with a single rescheduled restore.Deliberately not fixed (flagged for later, with reasons)
parse_special, and tokenize FIM prefix/suffix without BOS).trimKV. Only reachable on the non-default beam path (we ship greedy, width 1) and only when a rare KV removal fails. Low severity, delicate code; left for a focused follow-up.Validation
The coordinator and paste fixes live in CGEvent / NSPasteboard / @mainactor code the repo intentionally does not unit-test; both were verified by tracing the paths. Paste is default-off, so fix 3 affects only opt-in users.
Linked issues
None. Pre-release hardening.
Risk / rollout notes
falsepreserves the reconcile path.Greptile Summary
This PR fixes three pre-release bugs: multi-line completions were silently rejected by
InsertionSafetyGatebecause\nwas caught by the C0 control-character check, the typed-through-exhaustion path scheduled the next prediction against pre-keystroke AX text in Chromium, and the opt-in paste path could clobber or leak the clipboard when pastes overlapped or the user copied during the restore window.InsertionSafetyGate): Exempts0x0Afrom the C0 rejection so\ninside a multi-line completion passes the gate; all other control characters andU+FFFDare still rejected. The normalizer already strips\rbefore reaching the gate, so Windows line endings cannot bypass the fix.SuggestionCoordinator+Acceptance): Routes the typed-match-exhausted caller throughschedulePredictionAfterHostPublishDelay()via a newawaitHostPublishparameter (defaultfalse); all existing callers are unaffected.SuggestionInserter): Coalesces overlapping pastes onto one saved clipboard, guards the async restore with achangeCountequality check, and cancels/reschedules the restore on each subsequent paste.Confidence Score: 4/5
Safe to merge. All three bug fixes are well-reasoned and the changed paths are narrow: the gate fix only affects multi-line mode, the coordinator change is one routing flag with a safe default, and the paste fix is confined to a default-off opt-in path guarded by @mainactor serial execution.
The three fixes are individually correct and touch isolated paths. The only findings are a readability nit on the Swift condition-list form in InsertionSafetyGate and a missing test asserting that
remains rejected — both editorial rather than functional.
InsertionSafetyGate.swift has the minor readability issue on line 31; InsertionSafetyGateTests.swift is missing the
-still-rejected test case.
Important Files Changed
Reviews (1): Last reviewed commit: "Fix three bugs found in a pre-release au..." | Re-trigger Greptile