feat: detect non-interactive terminals and fail gracefully#13
Conversation
Add early TTY detection via isatty() and post-initialization DSR verification to NetConsoleDriver.Start(). When stdin/stdout is piped or the terminal doesn't respond to escape sequences, roll back terminal state and throw TerminalNotSupportedException with diagnostic details. - Create TerminalNotSupportedException (inherits InvalidOperationException) - Add isatty() P/Invoke and CheckTtyStatus() to TerminalRawMode - Add VerifyTerminalResponds() to TerminalCapabilities (reuses ReadDSRColumn) - Add TerminalVerificationTimeoutMs constant (500ms per-byte) - Make Stop() robust to partial Start() initialization - Add RestoreUnixSignal helper; restore signals when Start() throws - SIGQUIT (Ctrl+\) intentionally never suppressed for emergency exit Fixes #12
There was a problem hiding this comment.
Pull request overview
Adds startup safeguards to prevent SharpConsoleUI from hanging or leaving the user’s terminal in a broken state when run in non-interactive or non-ANSI-capable environments (Fixes #12).
Changes:
- Add Unix TTY detection (
isatty) and throwTerminalNotSupportedExceptionbefore entering raw/alt-screen when stdin/stdout are redirected. - Add post-initialization DSR verification to confirm the terminal actually responds to ANSI escape sequences.
- Make shutdown more tolerant of partial initialization by guarding some post-restore
Console.*calls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| SharpConsoleUI/Helpers/TerminalCapabilities.cs | Adds DSR-based “terminal responds” verification helper. |
| SharpConsoleUI/Drivers/TerminalNotSupportedException.cs | Introduces a dedicated exception type for unsupported terminal environments. |
| SharpConsoleUI/Drivers/NetConsoleDriver.cs | Implements fail-fast TTY check, DSR verification, and partial-cleanup hardening; adds signal restore helper. |
| SharpConsoleUI/Drivers/Input/TerminalRawMode.cs | Adds isatty() P/Invoke and a helper to detect stdin/stdout TTY status. |
| SharpConsoleUI/Configuration/SystemDefaults.cs | Adds a default timeout constant for terminal verification. |
Comments suppressed due to low confidence (1)
SharpConsoleUI/Drivers/NetConsoleDriver.cs:639
Stop()now guards someConsole.*calls, but it still callsSetCursorVisible(true)/ResetCursorShape()unguarded afterCleanup(). In partial-initialization scenarios where Console APIs are unavailable (orConsole.Outwas never restored), these can still throw and defeat the goal of makingStop()safe after a failed/partialStart(). Consider wrapping these calls in the same try/catch or makingSetCursorVisibleresilient to Console failures.
Cleanup();
try { Console.Clear(); } catch { }
// Restore cursor visibility on shutdown
SetCursorVisible(true);
ResetCursorShape();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// Per-byte timeout in milliseconds for DSR terminal verification during startup. | ||
| /// ReadDSRColumn calls readByte up to ~8 times, so effective worst-case timeout | ||
| /// is ~8x this value (~4s at 500ms). Longer than the 150ms VS16 probe timeout | ||
| /// because this is a one-time startup check and some embedded terminals may | ||
| /// have higher latency. | ||
| /// </summary> | ||
| public const int TerminalVerificationTimeoutMs = 500; |
There was a problem hiding this comment.
The comment describing the effective worst-case timeout assumes ReadDSRColumn calls readByte “~8 times”, but the number of reads depends on how many digits are in row/col and can be higher; for the “no response at all” case it’s typically just 1 call (the ESC read timing out). Consider rewording this to avoid an inaccurate bound, or describe the timeout semantics more precisely (per-byte timeout, total varies with response length).
| // Unix always uses raw mode to bypass ConsolePal | ||
| bool isUnix = !RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| _useDirectAnsi = isUnix; | ||
|
|
||
| // Fail fast if stdin or stdout is not a TTY (piped/redirected) | ||
| if (isUnix) | ||
| { | ||
| var (stdinIsTty, stdoutIsTty) = TerminalRawMode.CheckTtyStatus(); | ||
|
|
||
| if (!stdinIsTty || !stdoutIsTty) | ||
| { | ||
| // Restore signals suppressed in constructor so caller's terminal works | ||
| RestoreUnixSignal(SigInt); | ||
| RestoreUnixSignal(SigTstp); | ||
| if (_processExitHandler != null) | ||
| { | ||
| AppDomain.CurrentDomain.ProcessExit -= _processExitHandler; | ||
| _processExitHandler = null; | ||
| } | ||
|
|
||
| var term = Environment.GetEnvironmentVariable("TERM") ?? "(unset)"; | ||
| var details = new StringBuilder(); | ||
| details.AppendLine("SharpConsoleUI cannot start: interactive terminal required."); | ||
| details.AppendLine(); | ||
| details.AppendLine("Current environment:"); | ||
| details.AppendLine($" TERM = {term}"); | ||
| details.AppendLine($" stdin is TTY: {stdinIsTty}"); | ||
| details.AppendLine($" stdout is TTY: {stdoutIsTty}"); | ||
| details.AppendLine(); | ||
| if (!stdinIsTty) | ||
| details.AppendLine("Problem: stdin is not connected to a terminal (it appears to be piped or redirected)."); | ||
| if (!stdoutIsTty) | ||
| details.AppendLine("Problem: stdout is not connected to a terminal (it appears to be piped or redirected)."); | ||
| details.AppendLine(); | ||
| details.AppendLine("How to fix:"); | ||
| details.AppendLine(" - Run the application directly in a terminal emulator (e.g., Terminal, iTerm2, Alacritty, Windows Terminal)"); | ||
| details.AppendLine(" - Do not pipe input/output (avoid: echo | app, app | cat, app > file)"); | ||
| details.AppendLine(" - If using an IDE, run in the integrated terminal, not as a background process"); | ||
| throw new TerminalNotSupportedException(details.ToString()); | ||
| } |
There was a problem hiding this comment.
Start() sets _useDirectAnsi = isUnix before the TTY fail-fast check. If stdin/stdout are not TTY and you throw, the driver instance remains in “direct ANSI” mode even though raw mode was never entered, which can cause later calls (e.g., Stop() in a finally) to run the Unix raw cleanup path unnecessarily. Consider moving _useDirectAnsi assignment until after the TTY check passes, or explicitly resetting _useDirectAnsi = false just before throwing.
| // Restore signals suppressed in constructor so caller's terminal works | ||
| RestoreUnixSignal(SigInt); | ||
| RestoreUnixSignal(SigTstp); | ||
| if (_processExitHandler != null) | ||
| { | ||
| AppDomain.CurrentDomain.ProcessExit -= _processExitHandler; | ||
| _processExitHandler = null; | ||
| } |
There was a problem hiding this comment.
The PR description mentions restoring suppressed signals when Start() throws, but the only restoration currently happens in the non-TTY early-return path. Any other exception during Start() (including failures after raw mode/alt-screen activation) will still leave SIGINT/SIGTSTP suppressed and the ProcessExit handler registered. Consider wrapping the body of Start() in try/catch/finally to roll back terminal state and restore signals / unregister handlers on any failure.
| "System"); | ||
|
|
||
| // Roll back: Stop() handles partial initialization (Task 5 guards) | ||
| Stop(); |
There was a problem hiding this comment.
On DSR verification failure, Start() calls Stop() and then throws. Stop() does not restore SIGINT/SIGTSTP suppression, so after this exception Ctrl+C/Ctrl+Z may remain disabled for the rest of the process. If the intent is “restore signals when Start throws”, make sure this path also restores them (either in Stop() for Unix, or in a Start() exception handler).
| Stop(); | |
| try | |
| { | |
| Stop(); | |
| } | |
| finally | |
| { | |
| // Ensure Ctrl+C/Ctrl+Z behavior is restored even when Start() throws | |
| Console.TreatControlCAsInput = false; | |
| } |
| private static void RestoreUnixSignal(int signum) | ||
| { | ||
| try { _unixSignal(signum, IntPtr.Zero); } catch { } // SIG_DFL = 0 | ||
| } |
There was a problem hiding this comment.
RestoreUnixSignal resets the signal handler to SIG_DFL, which may not match the caller’s previous handler (and signal() returns the previous handler specifically so it can be restored). If the library is going to restore signals, consider capturing the prior handler when suppressing and restoring that exact value instead of always forcing SIG_DFL.
| /// Unlike <see cref="Probe"/> which swallows failures, this returns false | ||
| /// on timeout — indicating the terminal is not processing escape sequences. | ||
| /// </summary> | ||
| /// <param name="write">Action to write escape sequences to the terminal.</param> | ||
| /// <param name="readByte">Function to read a single byte with timeout. Returns -1 on timeout.</param> | ||
| /// <returns>True if the terminal responded, false if it timed out.</returns> |
There was a problem hiding this comment.
The XML docs for VerifyTerminalResponds say it “returns false on timeout” and contrasts with Probe “which swallows failures”, but the implementation catches all exceptions and returns false for parse errors or other failures too. Please align the documentation with behavior (e.g., “timeout or invalid response / error”) or narrow the catch to only handle timeouts.
| /// Unlike <see cref="Probe"/> which swallows failures, this returns false | |
| /// on timeout — indicating the terminal is not processing escape sequences. | |
| /// </summary> | |
| /// <param name="write">Action to write escape sequences to the terminal.</param> | |
| /// <param name="readByte">Function to read a single byte with timeout. Returns -1 on timeout.</param> | |
| /// <returns>True if the terminal responded, false if it timed out.</returns> | |
| /// Unlike <see cref="Probe"/> which swallows failures via cached defaults, | |
| /// this surfaces problems by returning false on timeout or invalid/error responses. | |
| /// </summary> | |
| /// <param name="write">Action to write escape sequences to the terminal.</param> | |
| /// <param name="readByte"> | |
| /// Function to read a single byte with timeout. Returns -1 on timeout; may throw on error. | |
| /// </param> | |
| /// <returns> | |
| /// True if a valid DSR response was received and parsed; false if the read timed out, | |
| /// the response was invalid, or an error occurred while reading/parsing. | |
| /// </returns> |
Summary
isatty()P/Invoke — throwsTerminalNotSupportedExceptionbefore entering raw mode if stdin/stdout is piped or redirectedStop()robust to partialStart()initialization (try/catch around Console calls that may fail)Start()throws, so the caller's terminal works normallyCtrl+\) is intentionally never suppressed, serving as emergency exitFixes #12
Test plan
dotnet build SharpConsoleUI/SharpConsoleUI.csproj— 0 warnings, 0 errorsdotnet test SharpConsoleUI.Tests/SharpConsoleUI.Tests.csproj— 2541/2541 passingdotnet run --project Example | cat) — throws TerminalNotSupportedException immediatelyecho "" | dotnet run --project Example) — throws TerminalNotSupportedException immediately