fix: add /dev/tty fallback for MinTTY/Git Bash TTY detection#163
fix: add /dev/tty fallback for MinTTY/Git Bash TTY detection#163greynewell merged 3 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds tests validating install/postinstall scripts' content and augments stdin terminal detection with a /dev/tty fallback (and tests) so MinTTY/Git Bash environments on Windows are treated as interactive when possible. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 27 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
17-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImplement the
/dev/ttyfallback here.
openDevTtyis still a stub that always returnsnot implemented, andstdinIsTerminal()never calls it. That means the MinTTY/Git Bash path this PR is meant to fix still cannot work. Please wire the fallback in here and close the file handle after a successful open.🛠️ Suggested shape of the fix
-var openDevTty = func() (*os.File, error) { - return nil, fmt.Errorf("not implemented") -} +var openDevTty = func() (*os.File, error) { + return os.Open("/dev/tty") +} @@ var stdinIsTerminal = func() bool { - return term.IsTerminal(int(syscall.Stdin)) //nolint:unconvert // syscall.Stdin is uintptr on Windows + if term.IsTerminal(int(syscall.Stdin)) { //nolint:unconvert // syscall.Stdin is uintptr on Windows + return true + } + f, err := openDevTty() + if err != nil { + return false + } + _ = f.Close() + return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 17 - 29, openDevTty currently returns a stub error and stdinIsTerminal never uses it, so implement openDevTty to attempt os.Open("/dev/tty") and return the *os.File and error, and modify stdinIsTerminal to fall back to opening /dev/tty when term.IsTerminal(int(syscall.Stdin)) is false: call openDevTty(), if it returns a non-nil *os.File then defer/close the file after checking term.IsTerminal(int(file.Fd())) (or equivalent), and return that check result; ensure any opened file is closed before returning and preserve existing nolint comment on the syscall.Stdin usage; reference symbols: openDevTty and stdinIsTerminal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/install_script_test.go`:
- Around line 16-21: The test "does not run setup wizard at install time" is
asserting the opposite of intended behavior; when an interactive TTY is
available install.sh should invoke the setup subcommand. Update the test in
cmd/install_script_test.go (the t.Run block using variable content and the
$BINARY literal check) to assert that content contains the setup invocation
(e.g., check for `" setup"` or `"supermodel setup"`) and change the error
message to state that install.sh must invoke the setup wizard when a TTY is
present rather than forbidding it.
In `@cmd/npm_package_test.go`:
- Around line 27-34: The test "includes getting-started message" expects
user-facing guidance that the postinstall script currently doesn't emit; update
either the test in cmd/npm_package_test.go or the postinstall script
npm/install.js. Fix option A: modify npm/install.js to append a concise message
after download/print path such as "To get started, run 'supermodel' in your
project directory" so the test's checks for "run 'supermodel'" or "get started"
pass. Fix option B: relax the test in cmd/npm_package_test.go by replacing the
expectation with what npm/install.js actually prints (e.g., look for the install
path or binary download confirmation) so the assertion reflects current
behavior.
In `@cmd/root_test.go`:
- Around line 11-35: The test TestStdinIsTerminalUsesDevTtyFallback is flaky
because it only mocks openDevTty; stub the stdinIsTerminal function too so the
test always exercises the /dev/tty fallback: save the original stdinIsTerminal,
set stdinIsTerminal = func() bool { return false } before calling
stdinIsTerminal(), and restore the original in t.Cleanup (similar to how
openDevTty is saved/restored). This ensures openDevTty is invoked
deterministically and the assertions about called and got remain valid.
---
Outside diff comments:
In `@cmd/root.go`:
- Around line 17-29: openDevTty currently returns a stub error and
stdinIsTerminal never uses it, so implement openDevTty to attempt
os.Open("/dev/tty") and return the *os.File and error, and modify
stdinIsTerminal to fall back to opening /dev/tty when
term.IsTerminal(int(syscall.Stdin)) is false: call openDevTty(), if it returns a
non-nil *os.File then defer/close the file after checking
term.IsTerminal(int(file.Fd())) (or equivalent), and return that check result;
ensure any opened file is closed before returning and preserve existing nolint
comment on the syscall.Stdin usage; reference symbols: openDevTty and
stdinIsTerminal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0f32022-7604-43ad-b825-5792a0bb8852
📒 Files selected for processing (5)
cmd/install_script_test.gocmd/npm_package_test.gocmd/root.gocmd/root_test.gointernal/auth/handler_test.go
| t.Run("does not run setup wizard at install time", func(t *testing.T) { | ||
| // The script uses $BINARY variable, so match the literal subcommand argument | ||
| if strings.Contains(content, `" setup`) || strings.Contains(content, "supermodel setup") { | ||
| t.Error("install.sh must not run the setup subcommand at install time — the wizard now auto-launches from bare 'supermodel' in a project directory (PR #152)") | ||
| } | ||
| }) |
There was a problem hiding this comment.
This assertion is backwards for the interactive install flow.
install.sh is supposed to launch setup when /dev/tty is available, so checking that it “does not run setup wizard at install time” will fail on the intended path. Flip this assertion to match the real behavior, or the test will stay red even when the installer is correct.
Possible fix
- if strings.Contains(content, `" setup`) || strings.Contains(content, "supermodel setup") {
- t.Error("install.sh must not run the setup subcommand at install time")
- }
+ if !strings.Contains(content, `" setup`) && !strings.Contains(content, "supermodel setup") {
+ t.Error("install.sh should launch setup when /dev/tty is available")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("does not run setup wizard at install time", func(t *testing.T) { | |
| // The script uses $BINARY variable, so match the literal subcommand argument | |
| if strings.Contains(content, `" setup`) || strings.Contains(content, "supermodel setup") { | |
| t.Error("install.sh must not run the setup subcommand at install time — the wizard now auto-launches from bare 'supermodel' in a project directory (PR #152)") | |
| } | |
| }) | |
| t.Run("does not run setup wizard at install time", func(t *testing.T) { | |
| // The script uses $BINARY variable, so match the literal subcommand argument | |
| if !strings.Contains(content, `" setup`) && !strings.Contains(content, "supermodel setup") { | |
| t.Error("install.sh should launch setup when /dev/tty is available") | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/install_script_test.go` around lines 16 - 21, The test "does not run
setup wizard at install time" is asserting the opposite of intended behavior;
when an interactive TTY is available install.sh should invoke the setup
subcommand. Update the test in cmd/install_script_test.go (the t.Run block using
variable content and the $BINARY literal check) to assert that content contains
the setup invocation (e.g., check for `" setup"` or `"supermodel setup"`) and
change the error message to state that install.sh must invoke the setup wizard
when a TTY is present rather than forbidding it.
| t.Run("includes getting-started message", func(t *testing.T) { | ||
| lower := strings.ToLower(content) | ||
| hasRunSupermodel := strings.Contains(lower, "run 'supermodel'") || strings.Contains(lower, `run "supermodel"`) | ||
| hasGetStarted := strings.Contains(lower, "get started") | ||
| if !hasRunSupermodel && !hasGetStarted { | ||
| t.Error("npm/install.js must print a getting-started message directing users to run 'supermodel' in their project directory") | ||
| } | ||
| }) |
There was a problem hiding this comment.
This postinstall assertion doesn’t match npm/install.js yet.
The current script only downloads the binary and prints the install path; it doesn’t include run 'supermodel' / run "supermodel" or a “get started” message, so this subtest will fail every time. Either add that user-facing guidance to npm/install.js or loosen the test to what the script actually emits.
Possible fix
- if !hasRunSupermodel && !hasGetStarted {
- t.Error("npm/install.js must print a getting-started message directing users to run 'supermodel' in their project directory")
- }
+ if !hasRunSupermodel && !hasGetStarted {
+ t.Error("npm/install.js must print a getting-started message directing users to run 'supermodel' in their project directory")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/npm_package_test.go` around lines 27 - 34, The test "includes
getting-started message" expects user-facing guidance that the postinstall
script currently doesn't emit; update either the test in cmd/npm_package_test.go
or the postinstall script npm/install.js. Fix option A: modify npm/install.js to
append a concise message after download/print path such as "To get started, run
'supermodel' in your project directory" so the test's checks for "run
'supermodel'" or "get started" pass. Fix option B: relax the test in
cmd/npm_package_test.go by replacing the expectation with what npm/install.js
actually prints (e.g., look for the install path or binary download
confirmation) so the assertion reflects current behavior.
term.IsTerminal(syscall.Stdin) returns false in MinTTY even at an interactive terminal. Fall back to opening /dev/tty (POSIX TTY device) — if it opens, stdin is interactive. Closes #154. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l hook Extract term.IsTerminal into a stubable `termIsTerminal` var so the /dev/tty fallback test is deterministic regardless of whether the test process itself has an interactive stdin. Addresses CodeRabbit review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
greynewell
force-pushed
the
fix/mintty-tty-detection
branch
from
199f07a to
c66477c
Compare
1 participant
Closes #154.
On Windows Git Bash (MinTTY),
term.IsTerminal(syscall.Stdin)returns false even at an interactive terminal. This causes baresupermodelto error instead of launching the setup wizard.Fix: if
term.IsTerminalreturns false, try opening/dev/tty. If that succeeds, treat stdin as interactive. This is the standard POSIX fallback and works in MinTTY.TDD: failing test committed first.
🤖 Generated with Claude Code
Summary by CodeRabbit