fix(all): always report missing gems on stderr; only wait on a dialog without a tty - #1544
Open
simtel12 wants to merge 1 commit into
Open
fix(all): always report missing gems on stderr; only wait on a dialog without a tty#1544simtel12 wants to merge 1 commit into
simtel12 wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… without a tty On Linux, alert_linux showed a GUI dialog whenever zenity/kdialog/xmessage was on PATH and then waited on it for up to ALERT_TIMEOUT_SECONDS with the child's output sent to /dev/null. The message existed only inside that dialog, so a terminal launch where the dialog is missed -- another workspace, behind other windows, or not rendering at all -- looked like a silent two-minute hang ending in exit 1, with no indication of the cause. This lands hardest on the early-exit CLI commands. GemCheck.verify! runs at lich.rbw:39, but the argv dispatcher is not required until lich.rbw:133, so --help, --version, --active-sessions and the rest are all unreachable while the dialog blocks -- including the --help that would have explained the fix. Separate the reporting from the waiting, which were previously the same act: - Always warn on stderr. It costs nothing, it carries the whole message rather than a pointer to it, and when stderr is captured it is the only record that survives. This is what makes the failure legible, and it is unconditional. - Only wait on a dialog when there was no terminal that report could reach. Launcher starts have no other channel, so they keep the dialog and its timeout unchanged; a terminal launch already has the message and no longer blocks for two minutes to deliver it a second time. The trailing no-dialog-tool branch goes away: the unconditional warn is now the fallback it used to provide.
simtel12
force-pushed
the
fix/gemcheck-tty-alert
branch
from
August 23, 2026 23:16
3c2a0dc to
6e4a6d4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1543.
On Linux,
alert_linuxopened a GUI dialog wheneverzenity/kdialog/xmessagewas onPATH, andrun_with_timeoutthen waited on it for up toALERT_TIMEOUT_SECONDS(120) with the child's stdout and stderr sent toFile::NULL. The message existed only inside that dialog. A dialog raised from a terminal launch is easy to miss — another workspace, behind other windows, or not rendering at all — so from the terminal Lich appeared to hang for two minutes and then exit 1 having printed nothing.It lands hardest on the early-exit CLI commands:
GemCheck.verify!runs atlich.rbw:39, butlib/main/argv_options.rbis not required untillich.rbw:133. So--help,--version,--active-sessions,--session-info,--link-to-sgeand the rest are all unreachable while the dialog blocks — including the--helpthat would have told the user to runbundle install.Easy to hit right now: following the 4.0.3 → 4.0.5 bump from #1529 with a fresh
rbenv installleaves you on an empty gem home, which is exactly the triggering condition.Change
The reporting and the waiting used to be the same act. Separating them is the whole fix:
warnon stderr, unconditionally. It costs nothing, it carries the whole message rather than a pointer to it, and when stderr is captured (a launcher's output, a service log) it is the only record that survives. This is the part that makes the failure legible.So nobody loses information relative to today, and the GUI path is untouched for the launches that actually depend on it. Both
$stdoutand$stderrare probed, since a terminal launch may redirect either;$stdout.isattyis already the idiom for this inlib/main/argv_options.rb(--link-to-sge,--unlink-from-sge, …).The trailing no-dialog-tool
else warnbranch is removed — the unconditional warn is now the fallback it used to provide.One deliberate non-change: a terminal launch no longer pops the dialog at all, rather than popping it and not waiting. Firing it off unwaited would leave an orphaned window behind an exited process, which is precisely what
run_with_timeoutwas written to avoid ("the spawned child keeps running as an orphan; spawning it ourselves lets us kill it directly"). If you'd rather terminal launches still get a window, the natural shape is a short bounded wait rather than fire-and-forget — happy to switch if you prefer that trade.Scoped to Linux, matching the issue.
alert_macoshas the same shape and would benefit from the same treatment — happy to extend it here or in a follow-up. I leftalert_windowsalone deliberately: it is bound up with the Ruby4Lich5 self-healing flow and I have no way to test it.Test plan
bundle exec rspec(full suite): 6338 examples, 0 failuresbundle exec rubocopon both touched files: no offensesmainand pass with the fixalert_linuxspecs now stub both streams explicitly rather than inheriting the runner's — rspec's stdout is a tty when run directly in a terminal but a pipe under CI, so they would otherwise pass or fail by accidentcmd_available?always true,run_with_timeoutsleeping the full timeout): under a pty the message prints in 0.000s and no dialog is spawned; with output redirected the full alert lands on stderr before the dialog wait begins, and the process still blocks on the dialog — confirming the launcher path is genuinely unchanged