fix(tui): detect missing TTY and show helpful error instead of Bubbletea crash#905
fix(tui): detect missing TTY and show helpful error instead of Bubbletea crash#905decode2 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesTTY Guard for TUI Launch
Upgrade Strategy Method Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/upgrade/executor.go`:
- Around line 521-524: The effectiveMethod function is being called multiple
times during a single tool execution—at line 521 for the spinner message, at
line 576 for result metadata, and again in runStrategy within
internal/update/upgrade/strategy.go at line 72—which causes unnecessary process
overhead and can result in inconsistent routing if one probe fails while another
succeeds. Resolve effectiveMethod once at the start of the tool execution
(before line 521), store the result in a variable, and then pass this resolved
method value through the execution chain instead of calling effectiveMethod
again at line 576. Additionally, update the runStrategy function and any related
execution functions to accept the pre-resolved method as a parameter rather than
computing it internally, eliminating the redundant call at strategy.go line 72.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 044909ec-e20b-449f-9ebd-589a3bb637fc
📒 Files selected for processing (8)
internal/app/app.gointernal/app/app_test.gointernal/app/parity_test.gointernal/cli/run.gointernal/cli/run_component_paths_test.gointernal/update/upgrade/executor.gointernal/update/upgrade/strategy.gointernal/update/upgrade/strategy_test.go
…tea crash When gentle-ai launches the interactive TUI without a terminal attached (scripts, CI/CD, non-interactive SSH), Bubbletea panics with a confusing 'could not open a new TTY' error. Added a TTY guard that checks stdin before launching the TUI. When no TTY is available, returns a clear error listing non-interactive commands (--version, --help, update, install, sync, doctor) instead of crashing. The guard uses the existing isattyFn seam from selfupdate.go for testability. Info commands (version, help) continue to work without a TTY since they are dispatched before the guard. Closes Gentleman-Programming#95
decode2
force-pushed
the
fix/tty-guard-non-interactive
branch
from
9219528 to
1fb584f
Compare
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Scoped, linked to the approved issue, and the guard runs before Bubbletea starts while keeping help/version usable without a TTY. The no-TTY message duplicates a bit of help copy, but that is not blocking for this bugfix. Approving.
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Good direction. The TTY guard is the right behavior, and keeping version/help available without a TTY is exactly the contract we want.
Please tighten the tests before merge. The new no-TTY path stubs stdin, but RunArgs still goes through OS support and system detection before reaching the TUI launch path. That leaves the test dependent on the host runner instead of fully controlling the contract.
Concrete ask: stub the OS/support and system-detection seams in the no-TTY tests, the same way stdin is controlled, so the test only fails when the TTY behavior regresses.
Stub ensureCurrentOSSupported and detectSystem in no-TTY tests to isolate TTY behavior and remove dependency on host runner. Addresses review feedback from Alan-TheGentleman on PR Gentleman-Programming#905
|
@Alan-TheGentleman Stubeen los seams de OS/support y system-detection en los tests no-TTY para aislar el comportamiento TTY y eliminar la dependencia del host runner. Cambios realizados:
Ambos tests ahora controlan completamente el contrato y solo fallarán cuando el comportamiento TTY regrese. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/app_test.go (1)
1202-1214: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert sentinel error identity, not only message text.
At Line 1202 onward, please also validate
errors.Is(err, ErrNoTTYForTUI)before message-content checks. This prevents false positives if another error happens to include similar text.Suggested test tightening
+import "errors" ... err := RunArgs([]string{}, &buf) if err == nil { t.Fatal("RunArgs(empty args) with no TTY: expected error, got nil") } + if !errors.Is(err, ErrNoTTYForTUI) { + t.Fatalf("RunArgs(empty args) with no TTY: error = %v; want ErrNoTTYForTUI", err) + } if !strings.Contains(err.Error(), "no TTY available") { t.Fatalf("RunArgs(empty args) with no TTY: error = %v; want error containing 'no TTY available'", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/app_test.go` around lines 1202 - 1214, The test is only validating error message content using strings.Contains checks, which can produce false positives if another error happens to contain similar text. Add a validation using errors.Is(err, ErrNoTTYForTUI) immediately after the initial nil check on the err returned from RunArgs to confirm the actual error identity before proceeding with the message content checks for "no TTY available", "--version", and "--help". This ensures you are testing for the specific sentinel error, not just relying on string matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/app/app_test.go`:
- Around line 1202-1214: The test is only validating error message content using
strings.Contains checks, which can produce false positives if another error
happens to contain similar text. Add a validation using errors.Is(err,
ErrNoTTYForTUI) immediately after the initial nil check on the err returned from
RunArgs to confirm the actual error identity before proceeding with the message
content checks for "no TTY available", "--version", and "--help". This ensures
you are testing for the specific sentinel error, not just relying on string
matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a53bbcb-415b-46f9-a732-428acaca2325
📒 Files selected for processing (1)
internal/app/app_test.go
…entinel error identity - Resolve effectiveMethod once per tool in the executable loop and thread the pre-resolved method through executeOne and runStrategy, eliminating two redundant calls (and a potential external brew probe on each). - Update runStrategy signature to accept method as a parameter; remove the internal effectiveMethod call from strategy.go. - Update all strategy_test.go call sites to pass the pre-resolved method. - Add errors.Is(err, ErrNoTTYForTUI) assertion before strings.Contains checks in TestRunArgs_NoTTY_ReturnsErrNoTTYForTUI to guard against false positives from coincidental message text matches. - Add errors import to app_test.go. Addresses CodeRabbit review feedback on PR Gentleman-Programming#905.
|
Hi @Alan-TheGentleman, I have addressed your comments:
Ready for re-review! |
2 participants
Summary
Detects missing TTY before launching the interactive TUI and shows a helpful error listing available non-interactive commands, instead of crashing with a confusing Bubbletea panic.
Problem
When
gentle-aiis launched without a terminal attached (scripts, CI/CD, non-interactive SSH), Bubbletea panics with:This gives no guidance on what to do instead.
Fix
Added
stdinIsTerminal()helper that reuses the existingisattyFnseam fromselfupdate.gofor testability.Added
ErrNoTTYForTUIerror with a clear message listing non-interactive commands:gentle-ai --versiongentle-ai --helpgentle-ai updategentle-ai installgentle-ai syncgentle-ai doctorInserted the TTY guard in
RunArgsimmediately before the TUI launch block, after info commands (version, help) are dispatched — so those continue to work without a TTY.Tests
TestRunArgs_NoTTY_ReturnsErrNoTTYForTUI— verifies the error is returned with the expected messageTestRunArgs_NoTTY_StillAllowsVersionAndHelp— verifies info commands work without TTYstdinIsTerminal = func() bool { return true }so the guard doesn't block themCloses #95
Summary by CodeRabbit
Bug Fixes
Tests
--version/--help(including short forms) still succeed and produce output without a terminal.