Skip to content

Commit 87245bd

Browse files
authored
Remove dead IsStdoutTerminal() and simplify containsAny test helper (#1494)
Two quick-win cleanups from the `golang.org/x/term` module review. ## Changes - **`internal/tty/tty.go`**: Remove `IsStdoutTerminal()` — exported but never called anywhere in the project. `IsStderrTerminal()` (used by the logger) is retained. - **`internal/tty/container_test.go`**: Replace hand-rolled substring search in the `containsAny` helper with `strings.Contains`: ```go // Before: manual nested loop func containsAny(s string, substrings []string) bool { for _, substr := range substrings { if len(s) >= len(substr) { for i := 0; i <= len(s)-len(substr); i++ { if s[i:i+len(substr)] == substr { return true } } } } return false } // After: standard library func containsAny(s string, substrings []string) bool { for _, substr := range substrings { if strings.Contains(s, substr) { return true } } return false } ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build609076577/b298/launcher.test /tmp/go-build609076577/b298/launcher.test -test.testlogfile=/tmp/go-build609076577/b298/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 64/src/internal/fmtsort/sort.go x_amd64/compile user.name 7533934/b010/` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build609076577/b280/config.test /tmp/go-build609076577/b280/config.test -test.testlogfile=/tmp/go-build609076577/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 40tls/fipstls.go ache/go/1.25.7/x64/pkg/tool/linu-o credential.helpe/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build609076577/b298/launcher.test /tmp/go-build609076577/b298/launcher.test -test.testlogfile=/tmp/go-build609076577/b298/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 64/src/internal/fmtsort/sort.go x_amd64/compile user.name 7533934/b010/` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build609076577/b298/launcher.test /tmp/go-build609076577/b298/launcher.test -test.testlogfile=/tmp/go-build609076577/b298/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 64/src/internal/fmtsort/sort.go x_amd64/compile user.name 7533934/b010/` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build609076577/b307/mcp.test /tmp/go-build609076577/b307/mcp.test -test.testlogfile=/tmp/go-build609076577/b307/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true arm.go --local x_amd64/compile user.name /gc` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>[go-fan] Go Module Review: golang.org/x/term</issue_title> <issue_description># 🐹 Go Fan Report: golang.org/x/term ## Module Overview `golang.org/x/term` is the official Go extended library for cross-platform terminal handling. It provides terminal detection, mode switching (raw mode), terminal sizing, password reading, and a full line-editing terminal emulator. It supports Linux, macOS, Windows, and Plan9. ## Current Usage in gh-aw The module is used exclusively through the thin `internal/tty` wrapper package. - **Files**: 1 file imports the module (`internal/tty/tty.go`) - **Import count**: 1 direct import - **Key APIs Used**: - `term.IsTerminal(int(os.Stderr.Fd()))` — checks stderr is a TTY, used at init in `internal/logger/logger.go` to enable ANSI color output - `term.IsTerminal(int(os.Stdout.Fd()))` — defined as `IsStdoutTerminal()` in the tty package but **never called anywhere** in the project ## Research Findings `golang.org/x/term` v0.38.0 is the current version in use. The most recent activity (Feb 11, 2026) was a routine Go directive update to 1.25.0 — no new user-facing APIs were added. The module API has been stable for years. ### Available APIs (not yet used) | API | Description | |-----|-------------| | `GetSize(fd int)` | Returns terminal width and height | | `ReadPassword(fd int)` | Reads a password without echo | | `MakeRaw(fd int)` | Puts terminal in raw mode | | `Restore(fd int, state)` | Restores terminal state | | `NewTerminal(rw, prompt)` | Full line-editing terminal emulator | ### Best Practices - Wrapping `golang.org/x/term` in an internal package (as this project does in `internal/tty`) is the recommended approach — it prevents the dependency from spreading throughout the codebase and makes it easy to mock/test. - The `int(os.Stderr.Fd())` cast pattern is idiomatic and correct. ## Improvement Opportunities ### 🏃 Quick Wins 1. **Remove dead code: `IsStdoutTerminal()`** - `IsStdoutTerminal()` in `internal/tty/tty.go` (line 13) is **never called** anywhere in the project. - Either add a caller or remove this exported function. - Dead exported functions in internal packages create confusion about intent. 2. **Simplify `containsAny` test helper in `container_test.go`** - The `containsAny` helper (lines 313–325) reimplements substring search with a manual nested loop: ```go for i := 0; i <= len(s)-len(substr); i++ { if s[i:i+len(substr)] == substr { return true } } ``` - This should use `strings.Contains` from the standard library, which is simpler, more readable, and better optimized: ```go func containsAny(s string, substrings []string) bool { for _, substr := range substrings { if strings.Contains(s, substr) { return true } } return false } ``` ### ✨ Feature Opportunities 3. **Terminal width-aware log output** - `term.GetSize(int(os.Stderr.Fd()))` could be used in the logger to adapt log line width to the terminal width. - Currently, the logger enables/disables color based on TTY detection but never adapts output width. Long debug lines could be wrapped or truncated at the terminal width for better readability. 4. **Password/secret prompting at startup** - If the gateway ever needs to prompt users interactively for API keys or credentials (e.g., when running without a config file), `term.ReadPassword()` is already available via the existing dependency at zero extra cost. ### 📐 Best Practice Alignment - ✅ Wrapping in `internal/tty` is correct and idiomatic - ✅ `int(os.Fd())` cast pattern is standard - ⚠️ `IsStdoutTerminal()` exported but unused — either use it or remove it - ⚠️ Test helper `containsAny` should use `strings.Contains` ### 🔧 General Improvements The `internal/tty` package currently mixes two concerns: terminal detection (`tty.go`) and container detection (`container.go`). These could eventually be split into separate focused packages (`internal/tty` and `internal/container`), but this is low priority since the package is small. ## Recommendations 1. **[High Priority]** Remove or add a caller for `IsStdoutTerminal()` — dead exported code is confusing 2. **[Medium Priority]** Replace `containsAny` manual loop with `strings.Contains` — simpler and more standard 3. **[Low Priority]** Consider `term.GetSize()` for terminal-width-aware formatting in the logger ## Next Steps - [ ] Decide whether `IsStdoutTerminal()` should be removed or used somewhere (e.g., in a future stdout color detection path) - [ ] Update `containsAny` test helper to use `strings.Contains` --- *Generated by Go Fan 🐹* *Module summary saved to: `/tmp/gh-aw/agent/term-module-summary.md` (workspace write permission not available for specs/mods/)* *Run: [§22476915418](https://github.com/github/gh-aw-mcpg/actions/runs/22476915418)* **Ref... </details> > **Custom agent used: agentic-workflows** > GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #1459 <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security)
2 parents 5932b4f + 8d289f1 commit 87245bd

File tree

2 files changed

+3
-11
lines changed

2 files changed

+3
-11
lines changed

internal/tty/container_test.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package tty
22

33
import (
44
"os"
5+
"strings"
56
"testing"
67

78
"github.com/stretchr/testify/assert"
@@ -312,12 +313,8 @@ func TestIsRunningInContainer_ConcurrentAccess(t *testing.T) {
312313
// Helper function to check if a string contains any of the given substrings
313314
func containsAny(s string, substrings []string) bool {
314315
for _, substr := range substrings {
315-
if len(s) >= len(substr) {
316-
for i := 0; i <= len(s)-len(substr); i++ {
317-
if s[i:i+len(substr)] == substr {
318-
return true
319-
}
320-
}
316+
if strings.Contains(s, substr) {
317+
return true
321318
}
322319
}
323320
return false

internal/tty/tty.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,6 @@ import (
99
"golang.org/x/term"
1010
)
1111

12-
// IsStdoutTerminal returns true if stdout is connected to a terminal.
13-
func IsStdoutTerminal() bool {
14-
return term.IsTerminal(int(os.Stdout.Fd()))
15-
}
16-
1712
// IsStderrTerminal returns true if stderr is connected to a terminal.
1813
func IsStderrTerminal() bool {
1914
return term.IsTerminal(int(os.Stderr.Fd()))

0 commit comments

Comments
 (0)