feat(qemu): default a VM to a window on a graphical host - #28
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughVM display selection now defaults to QEMU windows on graphical hosts. Explicit VNC preferences and hosts without graphical sessions use VNC. CLI, TUI, tests, comments, and documentation reflect the updated behavior. ChangesVM display behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
DisplayKind's "auto" default no longer singles out the uninstalled disk-mode install console. Every mode and installed state now opens a real qemu window on a graphical host; display="vnc" opts a VM out, and a headless host still vetoes the window unconditionally. DisplayKind drops the now-unused mode and installed parameters. The change touches few call sites (qemu.NeedsWindow, core.DisplayKind, tui/detail.go), so dropping them was cleaner than keeping dead arguments around. Updates the doc comments, TUI hints, and docs/ prose that described "auto" as the installer-console default, and fixes the console-password row in the TUI to name the surface the password is actually reached at instead of hardcoding VNC.
9b66f2b to
cc4cbb4
Compare
![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
🤖 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 `@docs/concepts/access-and-auth.md`:
- Line 133: Update the console-access documentation in
docs/concepts/access-and-auth.md at lines 133-133 to state that VNC is used when
the host is headless or display = "vnc"; apply the same wording and condition to
the cloud-console comment in internal/core/core.go at lines 250-251 so both
copies remain aligned.
In `@docs/reference/cli.md`:
- Line 157: Update the shell example code fences at the referenced documentation
locations to include the sh language identifier on both opening fences,
resolving markdownlint MD040 without changing the example contents.
In `@internal/config/config.go`:
- Around line 48-53: Update the Display field comment to state that "auto" uses
a QEMU window unless there is no usable graphical session or STOAT_GRAPHICAL=0
disables windows, in which cases it falls back to VNC. Keep the existing value
descriptions and legacy empty-value behavior unchanged.
In `@internal/qemu/args.go`:
- Around line 22-26: Update the DisplayKind comment to reflect that “window”, ""
and “auto” request a window only when the host is graphical; document that
DisplayKind returns VNC for non-graphical hosts, including when display is set
to “window”.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 00f25820-3af4-4192-b061-db8840b59eef
📒 Files selected for processing (20)
docs/concepts/access-and-auth.mddocs/getting-started/installation.mddocs/reference/cli.mddocs/troubleshooting.mdinternal/cli/display_test.gointernal/cli/run_vm.gointernal/cli/wire/dto_test.gointernal/cloudinit/cloudinit.gointernal/config/config.gointernal/config/password.gointernal/core/core.gointernal/core/display.gointernal/core/display_test.gointernal/qemu/args.gointernal/qemu/args_test.gointernal/qemu/display_test.gointernal/tui/creds_test.gointernal/tui/detail.gointernal/tui/detail_test.gointernal/tui/form.go
| ``` | ||
| ssh stoat@127.0.0.1:2202 | ||
| console stoat / stoat (qemu window only) | ||
| console stoat / stoat (over the qemu window, or vnc on a headless host) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both VNC routes for console access.
The text limits VNC to the headless fallback, but display = "vnc" also selects VNC on graphical hosts. Keep both copies aligned.
docs/concepts/access-and-auth.md#L133-L133: mention VNC when the host is headless ordisplay = "vnc".internal/core/core.go#L250-L251: apply the same condition to the cloud-console comment.
📍 Affects 2 files
docs/concepts/access-and-auth.md#L133-L133(this comment)internal/core/core.go#L250-L251
🤖 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 `@docs/concepts/access-and-auth.md` at line 133, Update the console-access
documentation in docs/concepts/access-and-auth.md at lines 133-133 to state that
VNC is used when the host is headless or display = "vnc"; apply the same wording
and condition to the cloud-console comment in internal/core/core.go at lines
250-251 so both copies remain aligned.
| Exactly one kind of VM gets a real QEMU window: a **disk-mode VM that is not yet installed**, on a host with a graphical session. Its OS installer draws to VGA and a human has to drive it, so `up` says so: | ||
| A VM gets a real QEMU window by default, on a host with a graphical session. Set `display = "vnc"` in `vm.toml` (or cycle it with the `d` key in the TUI) to keep a VM headless instead. QEMU then starts with `-display none` and a VNC server bound to a unix socket in the VM's directory; `-display none` cannot be undone on a running QEMU, so binding VNC at launch keeps a misbehaving guest recoverable. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the shell examples.
markdownlint reports MD040 at Line 157 and Line 175. Add sh to both opening fences so the documentation lint passes.
Also applies to: 175-175
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/reference/cli.md` at line 157, Update the shell example code fences at
the referenced documentation locations to include the sh language identifier on
both opening fences, resolving markdownlint MD040 without changing the example
contents.
Source: Linters/SAST tools
| // Display is the user's screen preference: "" or "auto" (default), | ||
| // "window", or "vnc". core.validateDisplay is the single place that | ||
| // checks the value; empty means an old vm.toml predates this field, so | ||
| // it must read the same as "auto". qemu.DisplayKind is the rule that | ||
| // turns this into DisplayWindow or DisplayVNC. | ||
| // "window", or "vnc". "auto" opens a real qemu window on a graphical | ||
| // host, and falls back to VNC only when the host has no display server; | ||
| // set "vnc" to opt out of a window on a graphical host. core.validateDisplay | ||
| // is the single place that checks the value; empty means an old vm.toml | ||
| // predates this field, so it must read the same as "auto". |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the host override in the Display contract.
The comment says "auto" falls back only when the host has no display server. STOAT_GRAPHICAL=0 also forces VNC on graphical hosts. Describe both conditions, such as no usable graphical session or an override that disables windows.
🤖 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/config/config.go` around lines 48 - 53, Update the Display field
comment to state that "auto" uses a QEMU window unless there is no usable
graphical session or STOAT_GRAPHICAL=0 disables windows, in which cases it falls
back to VNC. Keep the existing value descriptions and legacy empty-value
behavior unchanged.
| // DisplayKind is the rule, stated over the two facts it actually depends on. | ||
| // | ||
| // pref is config.VM.Display: "window" or "vnc" pins the surface outright, | ||
| // "" or "auto" runs the installer-console default below. Only an uninstalled | ||
| // disk-mode VM wants a window under that default, because its OS installer | ||
| // draws to VGA rather than the serial console and a human has to drive it. | ||
| // live, cloud and installed disk VMs reach ssh with no console interaction, | ||
| // so their screen goes to the VNC socket unless pref overrides it. | ||
| // pref is config.VM.Display: "window" pins a window, "vnc" pins the socket, | ||
| // "" or "auto" defaults to a window. Every mode and installed state defaults | ||
| // the same way; a VM opts into VNC by setting display="vnc", not by its mode. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the headless-host rule in this comment.
Lines 24-25 imply that "window" always selects a window. DisplayKind returns VNC when graphical is false, including for "window".
State that "window", "", and "auto" request a window only on a graphical host.
Proposed fix
-// pref is config.VM.Display: "window" pins a window, "vnc" pins the socket,
-// "" or "auto" defaults to a window. Every mode and installed state defaults
+// pref is config.VM.Display: "vnc" selects the socket. "window", "" and
+// "auto" select a window on a graphical host. Every mode and installed state defaults📝 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.
| // DisplayKind is the rule, stated over the two facts it actually depends on. | |
| // | |
| // pref is config.VM.Display: "window" or "vnc" pins the surface outright, | |
| // "" or "auto" runs the installer-console default below. Only an uninstalled | |
| // disk-mode VM wants a window under that default, because its OS installer | |
| // draws to VGA rather than the serial console and a human has to drive it. | |
| // live, cloud and installed disk VMs reach ssh with no console interaction, | |
| // so their screen goes to the VNC socket unless pref overrides it. | |
| // pref is config.VM.Display: "window" pins a window, "vnc" pins the socket, | |
| // "" or "auto" defaults to a window. Every mode and installed state defaults | |
| // the same way; a VM opts into VNC by setting display="vnc", not by its mode. | |
| // DisplayKind is the rule, stated over the two facts it actually depends on. | |
| // | |
| // pref is config.VM.Display: "vnc" selects the socket. "window", "" and | |
| // "auto" select a window on a graphical host. Every mode and installed state defaults | |
| // the same way; a VM opts into VNC by setting display="vnc", not by its mode. |
🤖 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/qemu/args.go` around lines 22 - 26, Update the DisplayKind comment
to reflect that “window”, "" and “auto” request a window only when the host is
graphical; document that DisplayKind returns VNC for non-graphical hosts,
including when display is set to “window”.
1 participant
Flips the display default.
auto(and an emptydisplay) now resolves to a qemu window whenever the host can draw one, for every mode and installed state. Before, only the uninstalled-disk installer console got a window; live, cloud, and installed VMs went to VNC, so a provisioned desktop VM showed nothing on the host.DisplayKinddrops to two facts:modeandinstalledno longer affect the surface, so they left the signature;NeedsWindow,core.DisplayKind, and the two detail.go call sites were updated. Thegraphicalveto stays unconditional and first, so a window preference still degrades to VNC on a headless host rather than crashing qemu's-display gtk.display = "vnc"opts a single VM back to headless;STOAT_GRAPHICAL=0does it host-wide.Swept the now-false prose: the "cloud VMs get no window" comments (config, password, core, cloudinit), the installer-console-specific
printDisplaywording, the detail console-password row (it reported "over vnc" unconditionally, now reports the real surface), the form hint, and the docs (troubleshooting, installation, cli, access-and-auth).Tests:
DisplayKindtable now expects a window for live/cloud/installed on a graphical host, VNC on a headless one; several TUI/CLI tests pinSTOAT_GRAPHICALexplicitly since the sandbox reads as graphical.just checkandjust testpass.Summary by CodeRabbit
New Features
Documentation