From e410516404eeee865c404ca9e0359fa52188140c Mon Sep 17 00:00:00 2001 From: Will Fuqua Date: Mon, 15 Jun 2026 20:52:18 +0700 Subject: [PATCH] If clipboard operations throw an exception, don't crash The exception is on Linux if xsel is not installed. Then, clipboard operations will _never_ work so we should thrown an exception telling the user to install xsel. --- src/PrettyPrompt/Panes/CodePane.cs | 44 +++++----- src/PrettyPrompt/Prompt.cs | 5 +- .../TextSelection/WrappedClipboard.cs | 81 ++++++++++++++++--- tests/PrettyPrompt.Tests/ClipboardTests.cs | 56 +++++++++++-- tests/PrettyPrompt.Tests/ConsoleStub.cs | 32 ++++++++ tests/PrettyPrompt.Tests/HistoryTests.cs | 3 +- tests/PrettyPrompt.Tests/RendererTests.cs | 3 +- tests/PrettyPrompt.Tests/SelectionTests.cs | 40 +++++++++ 8 files changed, 222 insertions(+), 42 deletions(-) diff --git a/src/PrettyPrompt/Panes/CodePane.cs b/src/PrettyPrompt/Panes/CodePane.cs index 0f5ba53..fec99bb 100644 --- a/src/PrettyPrompt/Panes/CodePane.cs +++ b/src/PrettyPrompt/Panes/CodePane.cs @@ -4,18 +4,13 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. #endregion -using System; -using System.Collections.Generic; using System.Diagnostics; -using System.Linq; -using System.Text; using System.Threading; using System.Threading.Tasks; using PrettyPrompt.Consoles; using PrettyPrompt.Documents; using PrettyPrompt.Rendering; using PrettyPrompt.TextSelection; -using TextCopy; using static System.ConsoleKey; using static System.ConsoleModifiers; @@ -26,7 +21,7 @@ internal class CodePane : IKeyPressHandler private readonly IConsole console; private readonly PromptConfiguration configuration; private readonly IPromptCallbacks promptCallbacks; - private readonly IClipboard clipboard; + private readonly WrappedClipboard clipboard; private readonly SelectionKeyPressHandler selectionHandler; private int topCoordinate; private int codeAreaWidth; @@ -115,7 +110,7 @@ public int EmptySpaceAtBottomOfWindowHeight } } - public CodePane(IConsole console, PromptConfiguration configuration, IPromptCallbacks promptCallbacks, IClipboard clipboard) + public CodePane(IConsole console, PromptConfiguration configuration, IPromptCallbacks promptCallbacks, WrappedClipboard clipboard) { this.console = console; this.configuration = configuration; @@ -267,9 +262,11 @@ public async Task OnKeyDown(KeyPress key, CancellationToken cancellationToken) case (Control, X) when selection.TryGet(out var selectionValue): { var cutContent = Document.GetText(selectionValue).ToString(); - Selection = null; - Document.Remove(this, selectionValue); - await clipboard.SetTextAsync(cutContent, cancellationToken).ConfigureAwait(false); + if (await clipboard.TrySetTextAsync(cutContent, cancellationToken).ConfigureAwait(false)) + { + Selection = null; + Document.Remove(this, selectionValue); + } break; } case (Control, X) or (Shift, Delete): @@ -285,10 +282,16 @@ public async Task OnKeyDown(KeyPress key, CancellationToken cancellationToken) if (key.ObjectPattern is (Control, X)) { - await clipboard.SetTextAsync(Document.GetText(span).ToString(), cancellationToken).ConfigureAwait(false); + if (await clipboard.TrySetTextAsync(Document.GetText(span).ToString(), cancellationToken).ConfigureAwait(false)) + { + Document.Remove(this, span); + } + } + else + { + // Shift+Delete just deletes the line; it never touches the clipboard. + Document.Remove(this, span); } - - Document.Remove(this, span); break; } case (Control, K) when selection is null: // Ctrl+K = delete from caret to end of the current line (emacs/readline kill-line); deletes only, does not write the clipboard @@ -308,19 +311,23 @@ public async Task OnKeyDown(KeyPress key, CancellationToken cancellationToken) case (Control, C) when selection.TryGet(out var selectionValue): { var copiedContent = Document.GetText(selectionValue).ToString(); - await clipboard.SetTextAsync(copiedContent, cancellationToken).ConfigureAwait(false); + // copy doesn't mutate the document, so a failed clipboard write is simply a no-op + _ = await clipboard.TrySetTextAsync(copiedContent, cancellationToken).ConfigureAwait(false); break; } case (Control | Shift, C): - await clipboard.SetTextAsync(Document.GetText(), cancellationToken).ConfigureAwait(false); + _ = await clipboard.TrySetTextAsync(Document.GetText(), cancellationToken).ConfigureAwait(false); break; case (Shift, Insert) when key.PastedText is not null: PasteText(key.PastedText); break; case (Control, V) or (Control | Shift, V) or (Shift, Insert): { - var clipboardText = await clipboard.GetTextAsync(cancellationToken).ConfigureAwait(false); - PasteText(clipboardText); + var (success, clipboardText) = await clipboard.TryGetTextAsync(cancellationToken).ConfigureAwait(false); + if (success) + { + PasteText(clipboardText); + } break; } case (Control, Z): @@ -395,8 +402,7 @@ string DedentMultipleLinesAndFilter(string text) else { var leadingIndent = nonEmptyLines - .Select(line => line.TakeWhile(char.IsWhiteSpace).Count()) - .Min(); + .Min(line => line.TakeWhile(char.IsWhiteSpace).Count()); if (leadingIndent == 0) { diff --git a/src/PrettyPrompt/Prompt.cs b/src/PrettyPrompt/Prompt.cs index 3637ce2..cc13119 100644 --- a/src/PrettyPrompt/Prompt.cs +++ b/src/PrettyPrompt/Prompt.cs @@ -16,7 +16,6 @@ using PrettyPrompt.Panes; using PrettyPrompt.Rendering; using PrettyPrompt.TextSelection; -using TextCopy; namespace PrettyPrompt; @@ -27,7 +26,7 @@ public sealed class Prompt : IPrompt, IAsyncDisposable private readonly HistoryLog history; private readonly PromptConfiguration configuration; private readonly CancellationManager cancellationManager; - private readonly IClipboard clipboard; + private readonly WrappedClipboard clipboard; private readonly SyntaxHighlighter highlighter; private readonly IPromptCallbacks promptCallbacks; private Task? savePersistentHistoryTask; @@ -51,7 +50,7 @@ public Prompt( this.configuration = configuration ?? new PromptConfiguration(); this.history = new HistoryLog(persistentHistoryFilepath, this.configuration.KeyBindings); this.cancellationManager = new CancellationManager(this.console); - this.clipboard = (console is IConsoleWithClipboard consoleWithClipboard) ? consoleWithClipboard.Clipboard : new WrappedClipboard(); + this.clipboard = (console is IConsoleWithClipboard consoleWithClipboard) ? new WrappedClipboard(consoleWithClipboard.Clipboard) : new WrappedClipboard(); promptCallbacks = callbacks ?? new PromptCallbacks(); this.highlighter = new SyntaxHighlighter(promptCallbacks, PromptConfiguration.HasUserOptedOutFromColor); diff --git a/src/PrettyPrompt/TextSelection/WrappedClipboard.cs b/src/PrettyPrompt/TextSelection/WrappedClipboard.cs index ceb9746..3397cf4 100644 --- a/src/PrettyPrompt/TextSelection/WrappedClipboard.cs +++ b/src/PrettyPrompt/TextSelection/WrappedClipboard.cs @@ -4,62 +4,117 @@ using TextCopy; namespace PrettyPrompt.TextSelection; -internal class WrappedClipboard : IClipboard + +/// +/// Wraps the clipboard so that transient clipboard failures never crash the prompt. +/// The OS clipboard can be flaky for reasons outside our control - e.g. no X display available, or the +/// underlying helper process timing out (TextCopy shells out with a hard-coded 500ms timeout on Linux). +/// A cut/copy/paste hiccup like that should degrade gracefully rather than abort the whole prompt, so the +/// Try* methods report failure instead of throwing. Callers can then avoid destructive edits - e.g. not +/// removing cut text that never made it to the clipboard. +/// +/// A missing clipboard executable (xsel/clip.exe) is different: it's an actionable setup problem rather +/// than a transient hiccup, so the Try* methods surface it as an exception with a helpful message instead +/// of silently swallowing it. +/// +internal sealed class WrappedClipboard { private const string MissingExecutableError = "Could not execute process"; private const string HelpfulErrorMessage = "Could not access clipboard. Check that xsel (Linux) or clip.exe (WSL) is installed."; - private readonly Clipboard clipboard; + + private readonly IClipboard clipboard; public WrappedClipboard() + : this(new Clipboard()) + { + } + + public WrappedClipboard(IClipboard clipboard) { - this.clipboard = new Clipboard(); + this.clipboard = clipboard; } - public string? GetText() + /// Reads the clipboard. Returns (with null) if the read failed. + public bool TryGetText(out string? text) { try { - return clipboard.GetText(); + text = clipboard.GetText(); + return true; } - catch (Exception ex) when (ex.Message.Contains(MissingExecutableError)) + catch (Exception ex) when (IsMissingExecutable(ex)) { throw new Exception(HelpfulErrorMessage, ex); } + catch + { + text = null; + return false; + } } - public async Task GetTextAsync(CancellationToken cancellation = default) + /// Reads the clipboard. Success is (and Text null) if the read failed. + public async Task<(bool Success, string? Text)> TryGetTextAsync(CancellationToken cancellation = default) { try { - return await clipboard.GetTextAsync(cancellation).ConfigureAwait(false); + return (true, await clipboard.GetTextAsync(cancellation).ConfigureAwait(false)); + } + catch (OperationCanceledException) when (cancellation.IsCancellationRequested) + { + throw; } - catch (Exception ex) when (ex.Message.Contains(MissingExecutableError)) + catch (Exception ex) when (IsMissingExecutable(ex)) { throw new Exception(HelpfulErrorMessage, ex); } + catch + { + return (false, null); + } } - public void SetText(string text) + /// Writes to the clipboard. Returns if the write failed. + public bool TrySetText(string text) { try { clipboard.SetText(text); + return true; } - catch (Exception ex) when (ex.Message.Contains(MissingExecutableError)) + catch (Exception ex) when (IsMissingExecutable(ex)) { throw new Exception(HelpfulErrorMessage, ex); } + catch + { + return false; + } } - public async Task SetTextAsync(string text, CancellationToken cancellation = default) + /// Writes to the clipboard. Returns if the write failed. + public async Task TrySetTextAsync(string text, CancellationToken cancellation = default) { try { await clipboard.SetTextAsync(text, cancellation).ConfigureAwait(false); + return true; } - catch (Exception ex) when (ex.Message.Contains(MissingExecutableError)) + catch (OperationCanceledException) when (cancellation.IsCancellationRequested) + { + throw; + } + catch (Exception ex) when (IsMissingExecutable(ex)) { throw new Exception(HelpfulErrorMessage, ex); } + catch + { + return false; + } } + + // A missing clipboard executable is reported by TextCopy as a "Could not execute process" error; + // unlike a timeout, reinstalling the tool is the fix, so we surface it rather than swallow it. + private static bool IsMissingExecutable(Exception ex) => ex.Message.Contains(MissingExecutableError); } diff --git a/tests/PrettyPrompt.Tests/ClipboardTests.cs b/tests/PrettyPrompt.Tests/ClipboardTests.cs index 902a90a..23dc832 100644 --- a/tests/PrettyPrompt.Tests/ClipboardTests.cs +++ b/tests/PrettyPrompt.Tests/ClipboardTests.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Linq; using System.Text; +using System.Threading; using System.Threading.Tasks; using Xunit; @@ -29,16 +30,61 @@ public async Task Clipboard_WrappedCopyPasting() var console = ConsoleStub.NewConsole(); using (console.ProtectClipboard()) { - clipboard.SetText("hello"); + Assert.True(clipboard.TrySetText("hello")); await Task.Delay(100, TestContext.Current.CancellationToken); - var pasted = clipboard.GetText(); + Assert.True(clipboard.TryGetText(out var pasted)); Assert.Equal("hello", pasted); - await clipboard.SetTextAsync("world", TestContext.Current.CancellationToken); + Assert.True(await clipboard.TrySetTextAsync("world", TestContext.Current.CancellationToken)); await Task.Delay(100, TestContext.Current.CancellationToken); - pasted = await clipboard.GetTextAsync(TestContext.Current.CancellationToken); - Assert.Equal("world", pasted); + var (success, pastedAsync) = await clipboard.TryGetTextAsync(TestContext.Current.CancellationToken); + Assert.True(success); + Assert.Equal("world", pastedAsync); } } + [Fact] + public async Task Clipboard_WhenUnderlyingClipboardThrows_ReportsFailureWithoutThrowing() + { + // The OS clipboard can fail for reasons outside our control (missing xsel/clip.exe, no display, + // the TextCopy helper process timing out - see https://github.com/waf/CSharpRepl/issues/327). + // None of those should propagate out and crash the prompt; the Try* methods report failure instead, + // which lets callers avoid destructive edits (e.g. not removing cut text that was never copied). + var throwingClipboard = new WrappedClipboard(new ThrowingClipboard()); + + Assert.False(throwingClipboard.TryGetText(out var text)); + Assert.Null(text); + + var (success, asyncText) = await throwingClipboard.TryGetTextAsync(TestContext.Current.CancellationToken); + Assert.False(success); + Assert.Null(asyncText); + + Assert.False(throwingClipboard.TrySetText("hello")); + Assert.False(await throwingClipboard.TrySetTextAsync("hello", TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task Clipboard_WhenExecutableMissing_ThrowsHelpfulError() + { + // A missing clipboard tool (xsel/clip.exe) is an actionable setup problem rather than a transient + // hiccup, so we surface it with a helpful message rather than silently swallowing it. + var clipboard = new WrappedClipboard(new ThrowingClipboard("Could not execute process")); + + Assert.Contains("xsel", Assert.Throws(() => clipboard.TryGetText(out _)).Message); + Assert.Contains("xsel", (await Assert.ThrowsAsync(async () => await clipboard.TryGetTextAsync(TestContext.Current.CancellationToken))).Message); + Assert.Contains("xsel", Assert.Throws(() => clipboard.TrySetText("hello")).Message); + Assert.Contains("xsel", (await Assert.ThrowsAsync(() => clipboard.TrySetTextAsync("hello", TestContext.Current.CancellationToken))).Message); + } + + [Fact] + public async Task Clipboard_WhenCancelled_PropagatesCancellation() + { + // Genuine cancellation should still surface - it isn't a clipboard failure to swallow. + var clipboard = new WrappedClipboard(new ThrowingClipboard()); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAnyAsync(async () => await clipboard.TryGetTextAsync(cts.Token)); + await Assert.ThrowsAnyAsync(() => clipboard.TrySetTextAsync("hello", cts.Token)); + } } diff --git a/tests/PrettyPrompt.Tests/ConsoleStub.cs b/tests/PrettyPrompt.Tests/ConsoleStub.cs index 7cf5a84..07405d9 100644 --- a/tests/PrettyPrompt.Tests/ConsoleStub.cs +++ b/tests/PrettyPrompt.Tests/ConsoleStub.cs @@ -367,4 +367,36 @@ public abstract class ConsoleWithClipboard : IConsoleWithClipboard public void WriteError(StringBuilder value, bool hideCursor) => WriteError(value.ToString()); public void WriteErrorLine(StringBuilder value, bool hideCursor) => WriteErrorLine(value.ToString()); public void WriteLine(StringBuilder value, bool hideCursor) => WriteLine(value.ToString()); +} + +/// +/// An that always fails, used to simulate an unavailable or flaky OS clipboard. +/// The default message mimics a transient failure (e.g. the TextCopy helper process timing out); pass a +/// message containing "Could not execute process" to mimic a missing xsel/clip.exe executable. +/// See https://github.com/waf/CSharpRepl/issues/327. +/// +internal sealed class ThrowingClipboard : IClipboard +{ + private readonly string message; + + public ThrowingClipboard(string message = "Process timed out.") + { + this.message = message; + } + + public string? GetText() => throw new Exception(message); + + public Task GetTextAsync(CancellationToken cancellation = default) + { + cancellation.ThrowIfCancellationRequested(); + throw new Exception(message); + } + + public void SetText(string text) => throw new Exception(message); + + public Task SetTextAsync(string text, CancellationToken cancellation = default) + { + cancellation.ThrowIfCancellationRequested(); + throw new Exception(message); + } } \ No newline at end of file diff --git a/tests/PrettyPrompt.Tests/HistoryTests.cs b/tests/PrettyPrompt.Tests/HistoryTests.cs index 6f0d069..e5b3319 100644 --- a/tests/PrettyPrompt.Tests/HistoryTests.cs +++ b/tests/PrettyPrompt.Tests/HistoryTests.cs @@ -8,6 +8,7 @@ using PrettyPrompt.Consoles; using PrettyPrompt.History; using PrettyPrompt.Panes; +using PrettyPrompt.TextSelection; using System; using System.IO; using System.Text; @@ -485,7 +486,7 @@ public async Task History_InvalidBase64Value_DoesNotCrash() Substitute.For(), new PromptConfiguration(), Substitute.For(), - Substitute.For() + new WrappedClipboard(Substitute.For()) ); history.Track(codePane); diff --git a/tests/PrettyPrompt.Tests/RendererTests.cs b/tests/PrettyPrompt.Tests/RendererTests.cs index 13b540b..532c646 100644 --- a/tests/PrettyPrompt.Tests/RendererTests.cs +++ b/tests/PrettyPrompt.Tests/RendererTests.cs @@ -5,6 +5,7 @@ using NSubstitute; using PrettyPrompt.Consoles; using PrettyPrompt.Panes; +using PrettyPrompt.TextSelection; using TextCopy; using Xunit; @@ -98,7 +99,7 @@ public async Task RenderOutput_ConsoleHeightTooSmallAndCursorOnFirstLine_ShowsIn private (CodePane codePane, CompletionPane completionPane, OverloadPane overloadPane) BuildUIPanes(string typedInput) { var callbacks = Substitute.For(); - var codePane = new CodePane(console, configuration, new PromptCallbacks(), Substitute.For()); + var codePane = new CodePane(console, configuration, new PromptCallbacks(), new WrappedClipboard(Substitute.For())); codePane.Document.InsertAtCaret(codePane, typedInput); var overloadPane = new OverloadPane(codePane, callbacks, configuration) { diff --git a/tests/PrettyPrompt.Tests/SelectionTests.cs b/tests/PrettyPrompt.Tests/SelectionTests.cs index 17971d6..e40f9be 100644 --- a/tests/PrettyPrompt.Tests/SelectionTests.cs +++ b/tests/PrettyPrompt.Tests/SelectionTests.cs @@ -8,6 +8,7 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using NSubstitute; using Xunit; using static System.ConsoleKey; using static System.ConsoleModifiers; @@ -162,6 +163,45 @@ public async Task ReadLine_CutText_CanBePasted() } } + [Fact] + public async Task ReadLine_CutOnEmptyPromptWithFailingClipboard_DoesNotCrash() + { + // Exact repro from https://github.com/waf/CSharpRepl/issues/327: pressing Ctrl+X on an (empty) prompt + // shells out to the OS clipboard, which can time out or otherwise fail. That must not crash the prompt. + var console = ConsoleStub.NewConsole(); + console.Clipboard.Returns(new ThrowingClipboard()); + using (console.ProtectClipboard()) + { + console.StubInput($"{Control}{X}{Enter}"); + + var prompt = new Prompt(console: console); + var result = await prompt.ReadLineAsync(); + + Assert.True(result.IsSuccess); + Assert.Equal("", result.Text); + } + } + + [Fact] + public async Task ReadLine_CutSelectionWithFailingClipboard_PreservesText() + { + // If the clipboard write fails, the cut must be a no-op rather than silently destroying the user's + // text - we only remove the selection once it's safely on the clipboard. + // https://github.com/waf/CSharpRepl/issues/327 + var console = ConsoleStub.NewConsole(); + console.Clipboard.Returns(new ThrowingClipboard()); + using (console.ProtectClipboard()) + { + console.StubInput($"hello world", $"{Shift}{Home}{Control}{X}{Enter}"); + + var prompt = new Prompt(console: console); + var result = await prompt.ReadLineAsync(); + + Assert.True(result.IsSuccess); + Assert.Equal("hello world", result.Text); + } + } + [Fact] public async Task ReadLine_Delete_LeftSelection() {