Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ title: Changelog

## [Unreleased](https://github.com/lets-cli/lets/releases/tag/v0.0.X)

* `[Fixed]` `source <(lets completion -s zsh)` no longer hangs: skip terminal background-color detection when lets runs outside the terminal's foreground process group (e.g. inside process substitution), where the query would suspend the process with SIGTTIN/SIGTTOU.
* `[Fixed]` Add spacing between the `EXAMPLES` help title and its examples.
* `[Fixed]` Align styled error output to the left while preserving the padded error badge.
* `[Docs]` Convert design specs into ADRs.
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ require (
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
github.com/lithammer/dedent v1.1.0
github.com/spf13/pflag v1.0.9
golang.org/x/sys v0.42.0 // indirect
golang.org/x/sys v0.42.0
gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b // indirect
gopkg.in/yaml.v3 v3.0.1
)
2 changes: 1 addition & 1 deletion internal/config/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func (c *Config) readMixin(mixin *Mixin) error {
// ensureRemoteConfig's behavior for the top-level remote config.
cachedData, readErr := rm.tryRead()
if readErr != nil {
return fmt.Errorf("download failed (%w); also failed to read cached version: %v", downloadErr, readErr)
return fmt.Errorf("download failed (%w); also failed to read cached version: %w", downloadErr, readErr)
}

if cachedData != nil {
Expand Down
5 changes: 2 additions & 3 deletions internal/logging/formatter.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ import (
"bytes"
"fmt"
"io"
"os"
"strings"
"unicode"

"charm.land/lipgloss/v2"
"github.com/charmbracelet/x/term"
"github.com/fatih/color"
"github.com/lets-cli/fang"
"github.com/lets-cli/lets/internal/theme"
log "github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -44,8 +44,7 @@ func newFormatter(errWriter io.Writer, cs fang.ColorSchemeFunc) *Formatter {
return f
}

isDark := lipgloss.HasDarkBackground(os.Stdin, file)
scheme := cs(lipgloss.LightDark(isDark))
scheme := cs(lipgloss.LightDark(theme.IsDarkBackground(file)))

w, _, err := term.GetSize(file.Fd())
if err != nil || w == 0 {
Expand Down
17 changes: 15 additions & 2 deletions internal/theme/theme.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/charmbracelet/x/exp/charmtone"
"github.com/charmbracelet/x/term"
"github.com/lets-cli/fang"
"github.com/lets-cli/lets/internal/util"
)

// Supported theme names.
Expand Down Expand Up @@ -39,10 +40,22 @@ func ColorSchemeByName(name string) fang.ColorSchemeFunc {
}
}

// IsDarkBackground reports whether the terminal behind out has a dark background.
// Detection queries the terminal (writes OSC 11 to out, reads the reply from stdin),
// which suspends a background process group with SIGTTIN/SIGTTOU — so when the
// process is not in the foreground, skip the query and assume dark (lipgloss's
// own fallback).
func IsDarkBackground(out term.File) bool {
if !util.IsForegroundProcess(out.Fd()) {
return true
}

return lipgloss.HasDarkBackground(os.Stdin, out)
}

// ProgressColorsByName resolves a theme name to fill and empty colors for download progress.
func ProgressColorsByName(name string, out term.File) (color.Color, color.Color) {
isDark := lipgloss.HasDarkBackground(os.Stdin, out)
scheme := ColorSchemeByName(name)(lipgloss.LightDark(isDark))
scheme := ColorSchemeByName(name)(lipgloss.LightDark(IsDarkBackground(out)))

return ProgressColors(scheme)
}
Expand Down
19 changes: 19 additions & 0 deletions internal/util/foreground.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package util

import (
"golang.org/x/sys/unix"
)

// IsForegroundProcess reports whether this process belongs to the foreground
// process group of the terminal referred to by fd. Reading or reconfiguring a
// terminal from a background process group suspends the process with
// SIGTTIN/SIGTTOU — e.g. inside process substitution:
// `source <(lets completion -s zsh)`.
func IsForegroundProcess(fd uintptr) bool {
pgrp, err := unix.IoctlGetInt(int(fd), unix.TIOCGPGRP)
if err != nil {
Comment on lines +12 to +14

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.

issue (bug_risk): Differentiate ioctl failures from true background status to avoid disabling detection on non-tty fds.

Because any unix.IoctlGetInt error is treated as "not foreground", non-tty fds (e.g., when stdout is a file/pipe or EBADF) are incorrectly handled as background, forcing callers to skip detection. Since SIGTTIN/SIGTTOU only apply to ttys, it would be better to treat ENOTTY (and possibly other ioctl failures) as "safe" and either return true or at least distinguish them from a real background process-group indication, so non-tty outputs retain normal detection behavior.

return false
}

return pgrp == unix.Getpgrp()
}
10 changes: 10 additions & 0 deletions tests/completion.bats
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,13 @@ setup() {
assert_line --index 0 "--debug[Run with debug]"
assert_line --index 1 "--env[Set env]"
}

# Regression: querying the terminal background color from a background process
# group suspends the process with SIGTTOU, so `source <(lets completion -s zsh)`
# in an interactive (job-control) shell hung forever. `script` allocates a PTY,
# `set -m` enables job control like an interactive shell.
@test "completion: source <(lets completion -s zsh) must not hang under job control" {
run timeout 10 script -qec "zsh -c 'set -m; source <(lets completion -s zsh); echo SOURCED_OK'" /dev/null
assert_success
assert_output --partial "SOURCED_OK"
Comment on lines +59 to +62

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.

suggestion (testing): Guard the regression test with skips when zsh or script binaries are unavailable

This test will fail on systems without zsh or script (or with a different script implementation), which is common on minimal CI images. Please guard it with availability checks (e.g., command -v zsh >/dev/null || skip "zsh not available" and similarly for script) so the suite stays portable while still exercising the regression where possible.

}
Loading