Skip to content

Guard startup terminal probing on Windows when stderr is redirected #37823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jun 8, 2026
Merged

Guard startup terminal probing on Windows when stderr is redirected #37823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
764cde5
Initial plan
Copilot Jun 8, 2026
0f71a7e
Avoid Windows startup terminal probe on redirected stderr
Copilot Jun 8, 2026
fbed8bc
Harden stderr stat guard in style probe init
Copilot Jun 8, 2026
103f7be
Clarify defensive stderr probe handling comments
Copilot Jun 8, 2026
0406514
Tighten Windows stderr mode detection
Copilot Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion pkg/styles/theme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ package styles

import (
"os"
"runtime"

lipgloss "charm.land/lipgloss/v2"
"charm.land/lipgloss/v2/compat"
Expand All @@ -59,8 +60,36 @@ func configureLipglossCompat() {
compat.Profile = colorprofile.Detect(os.Stderr, os.Environ())
}

func shouldConfigureLipglossCompat(goos string, stderrMode os.FileMode, statErr error) bool {
// On Windows, querying terminal capabilities against redirected/pipe handles
// can hang under some wrapper environments. Skip startup probing unless stderr
// is attached to a character device.
if goos == "windows" {
if statErr != nil {
return false
}
return stderrMode&(os.ModeDevice|os.ModeCharDevice) == (os.ModeDevice | os.ModeCharDevice)
}
return true
}

func init() {
configureLipglossCompat()
stderrInfo, statErr := os.Stderr.Stat()
// Defensive fallback: Stat should not normally return (nil, nil). Treat that
// impossible state as an invalid stderr handle so Windows startup probing is
// safely skipped.
if statErr == nil && stderrInfo == nil {
statErr = os.ErrInvalid

@github-actions github-actions Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The nil-nil → os.ErrInvalid conversion here cannot be exercised from the shouldConfigureLipglossCompat test table — the pure function always receives already-converted values. The existing "windows stat error" case uses os.ErrPermission, which is equivalent at runtime, but the traceability from this init() fallback to its test coverage is invisible.

💡 Options to close the gap

Option A — add an os.ErrInvalid case to the test table with a comment linking it to this path:

{
	name:    "windows ErrInvalid (init nil-nil fallback)",
	goos:    "windows",
	statErr: os.ErrInvalid,
	want:    false,
},

Option B — hoist the nil-nil guard into shouldConfigureLipglossCompat by passing stderrInfo directly (or a (mode, modeValid bool) pair), making the entire decision testable through the pure function.

Option A is a lower-effort win; Option B is structurally cleaner.

}
// Zero mode means "unknown/unset"; on Windows this keeps the startup probe
// disabled unless stderr is explicitly confirmed as a character device.
stderrMode := os.FileMode(0)
if stderrInfo != nil {
stderrMode = stderrInfo.Mode()
}
if shouldConfigureLipglossCompat(runtime.GOOS, stderrMode, statErr) {
configureLipglossCompat()
}
}

// Hex color constants for light and dark variants.
Expand Down
57 changes: 57 additions & 0 deletions pkg/styles/theme_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,63 @@ func TestConfigureLipglossCompatUsesStderr(t *testing.T) {
}
}

func TestShouldConfigureLipglossCompat(t *testing.T) {
tests := []struct {
name string
goos string
stderrMode os.FileMode
statErr error
want bool
}{
{
name: "windows character device",
goos: "windows",
stderrMode: os.ModeDevice | os.ModeCharDevice,
want: true,
},
{
name: "windows redirected pipe",
goos: "windows",
stderrMode: os.ModeNamedPipe,
want: false,
},
{
name: "windows zero mode stat success",
goos: "windows",
stderrMode: os.FileMode(0),
want: false,
},
{
name: "windows stat error",
goos: "windows",
statErr: os.ErrPermission,
want: false,
},
{
name: "windows ErrInvalid fallback",
goos: "windows",
statErr: os.ErrInvalid,
want: false,
},
{
name: "non-windows still configures",
goos: "linux",
stderrMode: os.ModeNamedPipe,
statErr: os.ErrPermission,
want: true,
},

@github-actions github-actions Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Consider adding a "windows stat succeeds, zero mode" case: goos: "windows", stderrMode: os.FileMode(0), statErr: nil, want: false. This pins the behaviour when stat returns a FileMode with no device flags set — a plausible handle state (e.g. a virtual file handle that stat-succeeds but reports nothing) that is currently untested.

💡 Suggested test case 
{
	name:       "windows zero mode stat success",
	goos:       "windows",
	stderrMode: os.FileMode(0),
	want:       false,
},

Since init() initialises stderrMode to os.FileMode(0) before reading stderrInfo.Mode(), this also documents the fallback default intentionally.

@github-actions github-actions Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test case for stderrMode=0, statErr=nil on Windows: the init() comment explicitly calls this the "unknown/unset" conservative default, but shouldConfigureLipglossCompat("windows", 0, nil) is never in the table.

💡 Suggested addition

The stderrMode := os.FileMode(0) fallback in init() is justified by: "Zero mode means 'unknown/unset'; on Windows this keeps the startup probe disabled unless stderr is explicitly confirmed as a character device." A test should pin this:

{
    name:       "windows unknown mode (zero) no error",
    goos:       "windows",
    stderrMode: os.FileMode(0),
    statErr:    nil,
    want:       false,
},

Without this, a refactor that treats zero-mode as "assume TTY" would go undetected.

}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := shouldConfigureLipglossCompat(tt.goos, tt.stderrMode, tt.statErr)
if got != tt.want {
t.Fatalf("shouldConfigureLipglossCompat(%q, %v, %v) = %v, want %v", tt.goos, tt.stderrMode, tt.statErr, got, tt.want)
}
})
}
}

// TestBordersExist verifies that all expected border definitions are defined
func TestBordersExist(t *testing.T) {
borders := map[string]lipgloss.Border{
Expand Down