Skip to content

Remove dead IsStdoutTerminal() and simplify containsAny test helper#1494

Merged
lpcox merged 2 commits intomainfrom
copilot/review-go-module-golang-org-x-term
Feb 28, 2026
Merged

Remove dead IsStdoutTerminal() and simplify containsAny test helper#1494
lpcox merged 2 commits intomainfrom
copilot/review-go-module-golang-org-x-term

Conversation

Copy link
Contributor

Copilot AI commented Feb 28, 2026

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:

// 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

Firewall rules blocked me from connecting to one or more addresses (expand for details)

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:

Original prompt

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:
      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:
      func containsAny(s string, substrings []string) bool {
          for _, substr := range substrings {
              if strings.Contains(s, substr) {
                  return true
              }
          }
          return false
      }

✨ Feature Opportunities

  1. 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.
  2. 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

**Ref...

Custom agent used: agentic-workflows
GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing


🔒 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.

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Review Go module golang.org/x/term for usage in gh-aw Remove dead IsStdoutTerminal() and simplify containsAny test helper Feb 28, 2026
@lpcox lpcox marked this pull request as ready for review February 28, 2026 05:01
Copilot AI review requested due to automatic review settings February 28, 2026 05:01
@lpcox lpcox merged commit 87245bd into main Feb 28, 2026
8 of 9 checks passed
@lpcox lpcox deleted the copilot/review-go-module-golang-org-x-term branch February 28, 2026 05:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Removes unused terminal-detection API from the internal TTY wrapper and simplifies a test helper by using the standard library, aligning with the recent golang.org/x/term module review findings.

Changes:

  • Removed dead exported function IsStdoutTerminal() from internal/tty.
  • Simplified containsAny test helper to use strings.Contains.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/tty/tty.go Removes unused exported stdout TTY helper, leaving stderr TTY detection intact.
internal/tty/container_test.go Replaces manual substring search logic in containsAny with strings.Contains.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go-fan] Go Module Review: golang.org/x/term

3 participants