Fix stray 'C' insertion from Speakly dictation - #2413
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSanitizes externally committed text in GhosttyNSView by stripping leading terminal escape/CSI/control bytes, forwards the sanitized UTF‑8 payload to the terminal surface, and splits control scalars into explicit control key events for in-app input routing. Adds unit and integration tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant IME as "IME / Accessibility"
participant App as "NSApp"
participant View as "GhosttyNSView"
participant Surface as "ghostty_surface"
IME->>View: insertText(commitString)
View->>App: check NSApp.currentEvent
alt currentEvent == nil
View->>View: sanitizeExternalCommittedText(bytes)
alt sanitized non-empty
View->>Surface: ghostty_surface_text(sanitized UTF-8)
Note right of Surface: printable text delivered
else sanitized empty
View-->>IME: drop (no send)
end
else
View->>View: iterate unicodeScalars
loop per scalar
alt control scalar (LF/CR/TAB/ESC)
View->>Surface: ghostty_surface_key(control key event)
else printable
View->>View: buffer printable scalar
end
end
View->>Surface: ghostty_surface_text(flush buffered UTF-8)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a bug where Speakly (and similar accessibility/dictation tools) injected a stray
Key observations:
Confidence Score: 5/5Safe to merge; all identified issues are edge-case or process concerns, not runtime regressions. The fix is correct and targeted: it matches existing paste-path patterns, the sanitizer handles the specific Speakly ESC[C case cleanly, and three behavioral tests are included. The two logic comments (dead 0x9B branch and partial nF consumption) are real observations but neither causes incorrect behavior for any realistic dictation input. The commit-structure note is a process gap, not a code defect. Sources/GhosttyTerminalView.swift — specifically the consumeLeadingEscapeSequence default branch and the 0x9B C1-CSI path. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[insertText called] --> B{keyTextAccumulator != nil?}
B -- Yes --> C[Accumulate into keyDown buffer]
B -- No --> D{NSApp.currentEvent == nil?}
D -- No\nKeyboard event --> E[chars unchanged]
D -- Yes\nExternal / AX / Speakly --> F[sanitizeExternalCommittedText]
F --> G{Leading ESC / ctrl bytes?}
G -- 0x1B ESC --> H[consumeLeadingEscapeSequence\nCSI / SS3 / OSC / single-char]
G -- 0x9B C1 CSI\ndead code for UTF-8 --> I[consumeLeadingCSISequence]
G -- byte < 0x20 or 0x7F --> J[strip bare control char]
G -- printable --> K[break — keep remaining text]
H --> G
I --> G
J --> G
K --> L[sanitizedChars]
E --> L
L --> M{sanitizedChars.isEmpty?}
M -- Yes --> N[return early — nothing to send]
M -- No --> O[sendTextToSurface\nghostty_surface_text — paste path]
Reviews (1): Last reviewed commit: "Fix Speakly escape leak on text insertio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if byte == 0x9B { | ||
| index = consumeLeadingCSISequence(in: bytes, from: index + 1) | ||
| continue |
There was a problem hiding this comment.
Swift's String stores characters in UTF-8. The Unicode character U+009B (C1 CSI) encodes as the two-byte sequence 0xC2 0x9B. When iterating over text.utf8, byte index 0 will be 0xC2, which is ≥ 0x20 and not 0x1B, so the outer loop's break fires immediately before this branch is ever reached.
The 0x9B check would only trigger for ISO-8859-1 / Latin-1 raw bytes — not for valid Swift String input.
This is dead code on the committed-text path (no real-world dictation tool produces a bare 0x9B via the Cocoa text-input pipeline), but it could create a false sense of coverage. A quick comment documenting this limitation would help future readers, or the branch could be removed.
| case 0x50, 0x5D, 0x5E, 0x5F: | ||
| // DCS/OSC/PM/APC: consume until BEL/ST or EOF. | ||
| return consumeLeadingEscapedStringSequence(in: bytes, from: next + 1) | ||
| default: | ||
| // Single-character escape. | ||
| return min(bytes.count, next + 1) | ||
| } |
There was a problem hiding this comment.
The default branch handles every byte after ESC that isn't covered by the explicit case labels, including the 0x20–0x2F range. Those bytes are ECMA-48 "intermediate" bytes for nF sequences that require one or more additional intermediate bytes followed by a final byte — e.g. ESC SP F (3 bytes) for C0 charset designation.
Because default only consumes one byte past the ESC (return min(bytes.count, next + 1)), the remaining intermediate/final bytes of any nF sequence are left in the buffer. The outer sanitizeExternalCommittedText loop then encounters the final byte (e.g. F = 0x46, which is ≥ 0x20), hits the break, and includes that byte in the returned string — producing stray output.
In practice dictation tools do not inject nF sequences, so this shouldn't affect the Speakly fix. Worth a comment or a narrow guard to prevent silent partial strips if the sanitizer is extended later:
case 0x20...0x2F:
// nF sequences: intermediate + final. Consume through the final byte (0x20–0x7E).
var i = next + 1
while i < bytes.count, (0x20...0x2F).contains(bytes[i]) { i += 1 }
return i < bytes.count ? i + 1 : i| final class ExternalCommittedTextSanitizationTests: XCTestCase { | ||
| func testStripsLeadingCSISequenceFromExternalCommittedText() { | ||
| XCTAssertEqual( | ||
| GhosttyNSView.sanitizeExternalCommittedText("\u{1B}[Chello"), | ||
| "hello" | ||
| ) | ||
| } | ||
|
|
||
| func testStripsMultipleLeadingControlAndEscapeSequences() { | ||
| XCTAssertEqual( | ||
| GhosttyNSView.sanitizeExternalCommittedText("\u{1B}[1;5C\u{1B}OChello"), | ||
| "hello" | ||
| ) | ||
| } | ||
|
|
||
| func testLeavesLiteralBracketPrefixedTextUntouched() { | ||
| XCTAssertEqual( | ||
| GhosttyNSView.sanitizeExternalCommittedText("[Code] review"), | ||
| "[Code] review" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Per CLAUDE.md ("Regression test commit policy"), regression tests for a bug fix must be split into two commits so CI can prove the test fails without the fix:
- Commit 1 — failing test only (CI goes red)
- Commit 2 — the fix (CI goes green)
Both the new ExternalCommittedTextSanitizationTests and the sanitizer implementation landed in the same commit (e63d3ebc). The CI evidence that these tests catch the bug without the fix is therefore missing from the PR's commit history.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
10068-10085:⚠️ Potential issue | 🟠 MajorRoute this through
TerminalSurface.sendText(_:)instead of writing raw bytes here.Line 10069 still drops eventless committed text whenever the runtime surface is temporarily nil.
TerminalSurface.sendText(_:)already usesghostty_surface_textand queues bytes until the surface comes back, so reusing it here avoids silent dictation/accessibility text loss during reparent/startup windows.💡 Suggested change
fileprivate func sendTextToSurface(_ chars: String) { - guard let surface = surface else { return } - guard let data = chars.data(using: .utf8), !data.isEmpty else { return } + guard !chars.isEmpty else { return } `#if` DEBUG let typingTimingStart = CmuxTypingTiming.start() `#endif` `#if` DEBUG cmuxWriteChildExitProbe( @@ ], increments: ["probeInsertTextCount": 1] ) `#endif` - data.withUnsafeBytes { rawBuffer in - guard let baseAddress = rawBuffer.baseAddress?.assumingMemoryBound(to: CChar.self) else { return } - ghostty_surface_text(surface, baseAddress, UInt(rawBuffer.count)) - } + terminalSurface?.sendText(chars) `#if` DEBUG CmuxTypingTiming.logDuration( path: "terminal.sendTextToSurface", startedAt: typingTimingStart, extra: "textBytes=\(chars.utf8.count)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 10068 - 10085, The sendTextToSurface(_:) function is writing raw bytes to ghostty_surface_text and thus drops committed text when surface is nil; change it to call TerminalSurface.sendText(_:) on the terminalSurface instance instead (use terminalSurface.sendText(chars)) so the existing queuing and nil-surface handling in TerminalSurface is reused; remove or replace the data.withUnsafeBytes / ghostty_surface_text call and any direct utf8-to-CChar buffer handling, but keep the existing debug probes and early returns for empty data/surface checks, referencing sendTextToSurface(_:), terminalSurface, TerminalSurface.sendText(_:), and ghostty_surface_text in your edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 10111-10113: The check for a single byte 0x9B is ineffective
because UTF-8 encodes U+009B as 0xC2 0x9B; update the sanitization loop in
GhosttyTerminalView.swift (the place iterating over bytes and calling
consumeLeadingCSISequence) to detect the two-byte UTF-8 sequence 0xC2 followed
by 0x9B (or equivalently decode the next scalar and check for U+009B) and treat
it the same as the CSI introducer: advance the index past both bytes and call
consumeLeadingCSISequence(in:bytes, from:); ensure you handle bounds so you
don't read past the end when peeking the next byte.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 10068-10085: The sendTextToSurface(_:) function is writing raw
bytes to ghostty_surface_text and thus drops committed text when surface is nil;
change it to call TerminalSurface.sendText(_:) on the terminalSurface instance
instead (use terminalSurface.sendText(chars)) so the existing queuing and
nil-surface handling in TerminalSurface is reused; remove or replace the
data.withUnsafeBytes / ghostty_surface_text call and any direct utf8-to-CChar
buffer handling, but keep the existing debug probes and early returns for empty
data/surface checks, referencing sendTextToSurface(_:), terminalSurface,
TerminalSurface.sendText(_:), and ghostty_surface_text in your edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9eef4e59-78e2-4b10-ae82-0399e6b2b5c3
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
e63d3eb to
4ac8a2e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
9903-9905:⚠️ Potential issue | 🟠 MajorFix unreachable CSI check for UTF-8
U+009B(Line 9903).
text.utf8never emits standalone0x9Bfor valid Swift strings, so this branch won’t catchU+009B-prefixed CSI input.Proposed fix
- if byte == 0x9B { - index = consumeLeadingCSISequence(in: bytes, from: index + 1) + if byte == 0xC2, index + 1 < bytes.count, bytes[index + 1] == 0x9B { + index = consumeLeadingCSISequence(in: bytes, from: index + 2) continue }#!/bin/bash python - <<'PY' s = "\u009BC" print("UTF-8 bytes for U+009BC:", list(s.encode("utf-8"))) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 9903 - 9905, The branch checking for a standalone byte 0x9B is unreachable because Swift's text.utf8 encodes U+009B as the two-byte sequence 0xC2 0x9B; update the loop that inspects `bytes` to detect the UTF‑8 sequence for U+009B instead of a single 0x9B: where you currently test `if byte == 0x9B { index = consumeLeadingCSISequence(in: bytes, from: index + 1) ... }`, change the logic to check `if byte == 0xC2 && nextByte == 0x9B` (safely checking bounds), and call `consumeLeadingCSISequence(in: bytes, from: index + 2)` so the CSI is consumed correctly; keep the existing `consumeLeadingCSISequence` call semantics and bounds checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9903-9905: The branch checking for a standalone byte 0x9B is
unreachable because Swift's text.utf8 encodes U+009B as the two-byte sequence
0xC2 0x9B; update the loop that inspects `bytes` to detect the UTF‑8 sequence
for U+009B instead of a single 0x9B: where you currently test `if byte == 0x9B {
index = consumeLeadingCSISequence(in: bytes, from: index + 1) ... }`, change the
logic to check `if byte == 0xC2 && nextByte == 0x9B` (safely checking bounds),
and call `consumeLeadingCSISequence(in: bytes, from: index + 2)` so the CSI is
consumed correctly; keep the existing `consumeLeadingCSISequence` call semantics
and bounds checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc13dbf2-adcb-4626-9d1c-5db89d990fad
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/CJKIMEInputTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
9903-9905:⚠️ Potential issue | 🟠 MajorMake the escape sanitizer UTF-8-safe.
Line 9903 can never match valid
String.utf8input because U+009B is encoded as0xC2 0x9B, not a lone0x9Bbyte. Also Lines 9930-9938 consume one arbitrary byte afterESC/ESC O; if a malformed prefix is followed by non-ASCII text, that splits the first scalar and the sanitized result comes back as�.... Match C1 introducers in their UTF-8 form and only consume ASCII finals.🐛 Proposed fix
- if byte == 0x9B { - index = consumeLeadingCSISequence(in: bytes, from: index + 1) + if byte == 0xC2, index + 1 < bytes.count, bytes[index + 1] == 0x9B { + index = consumeLeadingCSISequence(in: bytes, from: index + 2) continue } @@ case 0x4F: // SS3: ESC O final - return min(bytes.count, next + 2) + if next + 1 < bytes.count, bytes[next + 1] < 0x80 { + return next + 2 + } + return min(bytes.count, next + 1) @@ default: // Single-character escape. - return min(bytes.count, next + 1) + return bytes[next] < 0x80 ? min(bytes.count, next + 1) : nextThis read-only script reproduces the current byte walk and should show the raw
0x9Bbranch never fires forU+009B, while malformedESC + é/ESC O + 你inputs leave split UTF-8 suffixes behind:#!/bin/bash set -euo pipefail sed -n '9898,9940p' Sources/GhosttyTerminalView.swift python - <<'PY' def consume_csi(bs, start): i = start while i < len(bs): b = bs[i] if 0x20 <= b <= 0x3F: i += 1 continue if 0x40 <= b <= 0x7E: return i + 1 break return i def consume_escaped_string(bs, start): i = start while i < len(bs): b = bs[i] if b == 0x07: return i + 1 if b == 0x1B: n = i + 1 if n < len(bs) and bs[n] == 0x5C: return n + 1 return i if b < 0x20 or b == 0x7F: return i + 1 i += 1 return len(bs) def consume_escape(bs, start): n = start + 1 if n >= len(bs): return len(bs) b = bs[n] if b == 0x5B: return consume_csi(bs, n + 1) if b == 0x4F: return min(len(bs), n + 2) if b in (0x50, 0x5D, 0x5E, 0x5F): return consume_escaped_string(bs, n + 1) return min(len(bs), n + 1) def sanitize_like_current(text): bs = list(text.encode("utf-8")) idx = 0 while idx < len(bs): b = bs[idx] if b == 0x1B: idx = consume_escape(bs, idx) continue if b == 0x9B: idx = consume_csi(bs, idx + 1) continue break suffix = bytes(bs[idx:]) return bs, idx, list(suffix), suffix.decode("utf-8", errors="replace") for label, text in { "U+009B + C": "\u009BC", "ESC + é": "\x1bé", "ESC O + 你": "\x1bO你", }.items(): bs, idx, suffix, decoded = sanitize_like_current(text) print(f"{label}: bytes={bs} idx={idx} suffix={suffix} decoded={decoded!r}") PYAlso applies to: 9930-9938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 9903 - 9905, The sanitizer currently looks for a lone 0x9B byte and consumes one arbitrary byte after ESC/ESC O, which breaks UTF‑8 (U+009B is 0xC2 0x9B and multi‑byte scalars get split). Update the byte-walk to treat the C1 CSI introducer as the two‑byte UTF‑8 sequence (0xC2, 0x9B) instead of byte == 0x9B (so when you see 0xC2 followed by 0x9B advance the index and call consumeLeadingCSISequence), and tighten the ESC / ESC O handling to only consume ASCII CSI finals (bytes in 0x40..0x7E) rather than an arbitrary next byte; use the same final‑byte checks used by consumeLeadingCSISequence/consumeCSI logic to decide how far to advance. Ensure these changes reference the existing branches checking byte == 0x1B and byte == 0x9B and the function consumeLeadingCSISequence.
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
9860-9878: ReuseTerminalSurface.sendText(_:)here.This now duplicates the UTF-8 →
ghostty_surface_textbridge already centralized inTerminalSurface.sendText(_:)/writeTextData(_:to:), and it bypasses the pending-text queue those helpers already provide. Reusing the wrapper keeps one code path and avoids dropping committed text during brief surface reattachment windows.♻️ Proposed refactor
fileprivate func sendTextToSurface(_ chars: String) { - guard let surface = surface else { return } - guard let data = chars.data(using: .utf8), !data.isEmpty else { return } + guard let terminalSurface, !chars.isEmpty else { return } `#if` DEBUG let typingTimingStart = CmuxTypingTiming.start() `#endif` `#if` DEBUG cmuxWriteChildExitProbe( @@ ], increments: ["probeInsertTextCount": 1] ) `#endif` - data.withUnsafeBytes { rawBuffer in - guard let baseAddress = rawBuffer.baseAddress?.assumingMemoryBound(to: CChar.self) else { return } - ghostty_surface_text(surface, baseAddress, UInt(rawBuffer.count)) - } + terminalSurface.sendText(chars) `#if` DEBUG CmuxTypingTiming.logDuration( path: "terminal.sendTextToSurface", startedAt: typingTimingStart, extra: "textBytes=\(chars.utf8.count)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 9860 - 9878, sendTextToSurface(_:) duplicates low-level UTF-8 → ghostty_surface_text logic and bypasses the pending-text queue; replace its body to call terminalSurface.sendText(chars) (or use TerminalSurface.writeTextData(_:to:) as appropriate) after ensuring terminalSurface is non-nil, so text goes through the centralized wrapper and its queue semantics; keep existing debug probes/metrics but remove the direct data.withUnsafeBytes / ghostty_surface_text call and use terminalSurface/TerminalSurface.sendText(_:)/writeTextData(_:to:) to perform the actual send.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9903-9905: The sanitizer currently looks for a lone 0x9B byte and
consumes one arbitrary byte after ESC/ESC O, which breaks UTF‑8 (U+009B is 0xC2
0x9B and multi‑byte scalars get split). Update the byte-walk to treat the C1 CSI
introducer as the two‑byte UTF‑8 sequence (0xC2, 0x9B) instead of byte == 0x9B
(so when you see 0xC2 followed by 0x9B advance the index and call
consumeLeadingCSISequence), and tighten the ESC / ESC O handling to only consume
ASCII CSI finals (bytes in 0x40..0x7E) rather than an arbitrary next byte; use
the same final‑byte checks used by consumeLeadingCSISequence/consumeCSI logic to
decide how far to advance. Ensure these changes reference the existing branches
checking byte == 0x1B and byte == 0x9B and the function
consumeLeadingCSISequence.
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9860-9878: sendTextToSurface(_:) duplicates low-level UTF-8 →
ghostty_surface_text logic and bypasses the pending-text queue; replace its body
to call terminalSurface.sendText(chars) (or use
TerminalSurface.writeTextData(_:to:) as appropriate) after ensuring
terminalSurface is non-nil, so text goes through the centralized wrapper and its
queue semantics; keep existing debug probes/metrics but remove the direct
data.withUnsafeBytes / ghostty_surface_text call and use
terminalSurface/TerminalSurface.sendText(_:)/writeTextData(_:to:) to perform the
actual send.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f994db17-0738-464d-b9c5-844348e1861c
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/CJKIMEInputTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9888-9995: The sanitizer currently strips ESC sequences but can
slice multi-byte UTF-8 scalars and misses C1 control encodings; update
sanitizeExternalCommittedText and consume helpers so non-ASCII C1 sequences and
full UTF-8 scalars are consumed correctly: in sanitizeExternalCommittedText,
when bytes[index]==0xC2 detect any C2 0x80...0x9F pair and switch on the second
byte to handle U+008F (SS3 -> treat like ESC O and advance via
consumeLeadingEscapeSequence/SS3 logic), U+0090 (DCS), U+009B (CSI), U+009D
(OSC), U+009E (PM), U+009F (APC) (map DCS/OSC/PM/APC to
consumeLeadingEscapedStringSequence and CSI to consumeLeadingCSISequence),
advancing the index past the two-byte C1 UTF-8 prefix before calling the
respective consumer; and change consumeLeadingEscapeSequence's default
(single-character escape) to advance past the entire UTF-8 scalar starting at
the byte after ESC (implement a small helper that walks UTF-8 continuation bytes
so you return the index at the next scalar boundary rather than just next+1) so
non-ASCII chars are not left half-sliced (reference symbols:
sanitizeExternalCommittedText, consumeLeadingEscapeSequence,
consumeLeadingCSISequence, consumeLeadingEscapedStringSequence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45f3a53a-54c9-49d4-8bc6-fa14b3e92db6
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/CJKIMEInputTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:9876">
P3: This adds a second copy of the text/control-key encoding logic instead of reusing the existing `TerminalSurface.sendInput` path. Keeping two copies of this mapping in a typing path is brittle and can cause behavior drift between input entry points.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
9945-10049:⚠️ Potential issue | 🟠 MajorMake the escape stripper UTF-8-safe and cover the remaining C1 introducers.
Line 9957 only recognizes
U+009B.U+008F/U+0090/U+009D–U+009Fstill bypass sanitization, and Line 9995 still advances one raw byte afterESC, which can start the suffix in the middle of a UTF-8 scalar. Line 9974 then decodes that invalid slice with replacement characters instead of preserving or dropping the original text.Suggested fix
if byte == 0xC2 { let next = index + 1 - if next < bytes.count, bytes[next] == 0x9B { - // U+009B (C1 CSI) is encoded as the UTF-8 byte pair C2 9B. - index = consumeLeadingCSISequence(in: bytes, from: next + 1) - continue + if next < bytes.count { + switch bytes[next] { + case 0x8F: + index = min(bytes.count, next + 2) // U+008F SS3 + continue + case 0x90, 0x9D, 0x9E, 0x9F: + index = consumeLeadingEscapedStringSequence(in: bytes, from: next + 1) + continue + case 0x9B: + index = consumeLeadingCSISequence(in: bytes, from: next + 1) // U+009B CSI + continue + default: + break + } } } @@ case 0x50, 0x5D, 0x5E, 0x5F: // DCS/OSC/PM/APC: consume until BEL/ST or EOF. return consumeLeadingEscapedStringSequence(in: bytes, from: next + 1) default: - // Single-character escape. - return min(bytes.count, next + 1) + // Don't split the first UTF-8 scalar after ESC. + return bytes[next] < 0x80 ? min(bytes.count, next + 1) : next } @@ if byte == 0x07 { return index + 1 } + + if byte == 0xC2 { + let next = index + 1 + if next < bytes.count, bytes[next] == 0x9C { + return next + 1 // U+009C ST + } + } @@ - return String(decoding: bytes[index...], as: UTF8.self) + return String(bytes: bytes[index...], encoding: .utf8) ?? ""Run the following to confirm the missing C1 branches and the mid-scalar slice risk:
#!/bin/bash python - <<'PY' print("C1 UTF-8 encodings:") for cp in [0x8F, 0x90, 0x9B, 0x9C, 0x9D, 0x9E, 0x9F]: print(hex(cp), list(chr(cp).encode("utf-8"))) print("ESC + あ bytes:", [0x1B] + list("あ".encode("utf-8"))) PY rg -n "0x8F|0x90|0x9B|0x9C|0x9D|0x9E|0x9F|return min\\(bytes\\.count, next \\+ 1\\)|String\\(decoding: bytes\\[index\\.\\.\\.\\], as: UTF8\\.self\\)" Sources/GhosttyTerminalView.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 9945 - 10049, sanitizeExternalCommittedText currently only recognizes U+009B and may slice in the middle of a multi-byte UTF‑8 scalar (causing replacement chars) and also misses other C1 introducers; update sanitizeExternalCommittedText to treat all C1 bytes (0x8F, 0x90, 0x9B–0x9F) like 0x9B by routing them into consumeLeadingCSISequence or the appropriate handler, and modify consumeLeadingEscapeSequence (the default single-character escape branch) to advance to the next UTF‑8 scalar boundary instead of advancing exactly one raw byte (implement a small helper that skips UTF‑8 continuation bytes 0x80...0xBF); finally ensure the final slice in sanitizeExternalCommittedText is taken from a UTF‑8-valid boundary (e.g., use String(bytes[index...], encoding: .utf8) or fall back to dropping the prefix) so you don’t decode mid‑scalar and produce replacement characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9945-10049: sanitizeExternalCommittedText currently only
recognizes U+009B and may slice in the middle of a multi-byte UTF‑8 scalar
(causing replacement chars) and also misses other C1 introducers; update
sanitizeExternalCommittedText to treat all C1 bytes (0x8F, 0x90, 0x9B–0x9F) like
0x9B by routing them into consumeLeadingCSISequence or the appropriate handler,
and modify consumeLeadingEscapeSequence (the default single-character escape
branch) to advance to the next UTF‑8 scalar boundary instead of advancing
exactly one raw byte (implement a small helper that skips UTF‑8 continuation
bytes 0x80...0xBF); finally ensure the final slice in
sanitizeExternalCommittedText is taken from a UTF‑8-valid boundary (e.g., use
String(bytes[index...], encoding: .utf8) or fall back to dropping the prefix) so
you don’t decode mid‑scalar and produce replacement characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e1c38c1-acc9-4c5a-8a94-91a905673f3f
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEInputTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/CJKIMEInputTests.swift
This branch was successfully deployed
1 participant
Summary
ghostty_surface_textinstead of keyboard-event encoding.Cprefix.Testing
ExternalCommittedTextSanitizationTestsincmuxTests/CJKIMEInputTests.swift.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Closes #2400
Summary by cubic
Fixes the stray “C” from Speakly dictation by sanitizing escape‑prefixed committed text and sending IME/AX text as typed input so shells and editors behave correctly. Programmatic
insertTextkeeps literal ESC bytes for automation.insertText:as external committed text and sanitize: strip leading ESC/CSI/SS3 and OSC/DCS/APC/PM sequences, including C1 CSI (U+009B); preserve newline/tab; drop if empty.insertText.insertTextnewline → Return and ESC preservation, and AX value sanitization.Written for commit b71d6fb. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests