Sharper TUI rendering: sprite glyphs, default theme, terminal-query callbacks - #8
Merged
Conversation
Pango leaves visible seams between adjacent block-character cells and produces gappy borders for box-drawing characters. Add a custom Cairo geometric renderer for U+2500-U+257F (box drawing) and U+2580-U+259F (block elements) — ported from Ghostty's font/sprite/draw/{box,block}.zig. Pango still handles all other glyphs.
Roost previously left libghostty-vt's palette and default fg/bg uncustomised, so cells with palette-indexed backgrounds (e.g. Codex's gray-card chrome resolved to colors that happened to match the surface background and rendered invisibly. Add a Theme value type plus a DefaultTheme mirroring cmux's dark scheme (16 named ANSI entries; 16-255 generated to match libghostty's built-in xterm cube and gray ramp). Install it via a new SetTheme cgo wrapper at NewTerminal time. Wire cursor and selection colors through the same struct so the future config-file loader has every user-visible color covered the day it lands. Also flip antialias off for the block-element sprite path as defensive insurance against AA-drift seams. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> COMMITEOF )
Programs running in the terminal probe its state via OSC and CSI queries (DA1 to detect capabilities, OSC 10/11 for fg/bg color, CSI ?996n for the light/dark scheme). Without WRITE_PTY wired, libghostty had no way to send any response back to the pty, so probing programs got silence and fell back to degraded behaviour. With it wired, a Roost terminal now reports as VT220 with ANSI color (matching xterm-256color), responds to DSR and kitty keyboard queries, and identifies as a dark scheme. Each callback shares libghostty's single userdata slot via a runtime/cgo.Handle wrapping a per-Terminal callbacks struct, freed in Close. A separate callbacks_export.go isolates the //export functions so cgo's auto-generated _cgo_export.h doesnt collide with the static helpers in callbacks.go. OSC 10/11 query responses still dont surface — confirmed via diagnostic that libghostty-vts color-operation handler explicitly drops the .query arm (stream_terminal.zig). That gap is upstream; a Roost-side workaround will follow as a separate change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
libghostty-vts color-operation handler stubs the .query arm (stream_terminal.zig:616-618), so when programs query the terminal for its current foreground/background/cursor colour they get silence. Codex relies on this query-and-respond loop: it asks for the background via OSC 11, and only emits its prompt-row gray-bar bg SGR when it gets an answer. Without one, no gray bar. Extend the existing OSC scanner (already running in the per-session pty pump alongside libghostty for OSC 9/777 notifications) so it also recognises OSC 10/11/12 queries and synthesises the standard rgb:RRRR/GGGG/BBBB response from DefaultThemes fg/bg/cursor. The response is queued through the same Session.QueueWrite that handles keystrokes, so it serialises correctly. Becomes dead code once libghostty-vt fills in its .query arm upstream — at which point both responses race and the program reads the first identical one. Easy to delete then. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Theme model and applies it to rendering and libghostty; implements a Cairo sprite renderer and tests for box/block glyphs; wires libghostty callbacks and cgo-exported hooks; replaces per-call PTY write goroutines with a single writer goroutine; extends OSC scanner to synthesize color-query responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant Terminal
participant GhosttyC as "libghostty C"
participant Writer
Session->>Terminal: SetTheme(fg,bg,cursor,palette)
Terminal->>GhosttyC: set foreground/background/cursor/palette
GhosttyC-->>Terminal: OK
Session->>Terminal: SetDeviceAttributes(attrs)
Terminal->>GhosttyC: register device attributes callback
GhosttyC-->>Terminal: OK
Session->>Terminal: SetPtyWriter(QueueWrite)
Terminal->>GhosttyC: register write callback
GhosttyC-->>Writer: write events -> QueueWrite
Session->>Writer: QueueWrite(bytes)
Writer->>Writer: enqueue and perform ordered pty.Write
sequenceDiagram
participant OSCInput
participant Scanner
participant Handler
participant Session
OSCInput->>Scanner: Feed(OSC 11 "?" BEL)
Scanner->>Handler: QueryColors()
Handler->>Session: obtain theme colors
Session-->>Handler: fg,bg,cursor RGB
Handler-->>Scanner: synthesized "rgb:..." bytes
Scanner->>Handler: OnQueryResponse("rgb:... BEL")
Handler->>Session: QueueWrite(response bytes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ghostty/callbacks_export.go`:
- Around line 21-27: The resolveCallbacks function currently calls
cgo.Handle(uintptr(userdata)).Value() directly and can panic if the handle was
deleted; update resolveCallbacks to guard the Value() call by wrapping it in a
defer/recover so any panic from Value() is caught and the function returns nil
instead of crashing; specifically, around the
cgo.Handle(uintptr(userdata)).Value() invocation in resolveCallbacks, add a
defer that recovers from a panic and sets the returned *terminalCallbacks to
nil, leaving the existing userdata==nil check intact and ensuring callers safely
receive nil for stale/deleted handles.
In `@internal/ghostty/callbacks.go`:
- Around line 97-99: The conditional that clears callbacks calls
C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_WRITE_PTY, nil) and then
returns nil unconditionally, swallowing any failure; change both occurrences
(the blocks that pass nil to ghostty_terminal_set when clearing callbacks) to
check the C function's return value and return a non-nil Go error when it
indicates failure (convert the C return code to an error via fmt.Errorf or a
helper like errorFromGhostty) instead of always returning nil so callers can
observe and handle ghostty_terminal_set failures.
🪄 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: 87d3a817-9c91-456e-9bb3-19e03e9980d9
📒 Files selected for processing (10)
cmd/roost/render.gocmd/roost/session.gocmd/roost/sprite.gocmd/roost/sprite_test.gocmd/roost/theme.gointernal/ghostty/callbacks.gointernal/ghostty/callbacks_export.gointernal/ghostty/terminal.gointernal/osc/scanner.gointernal/osc/scanner_test.go
…ell height
Two related bugs were combining to clip glyph descenders inside multi-row prompt boxes (opencode and codex gray bars) and visibly compress text like ls output. First, measureCells took cell height from layout.PixelSize("M"), which is the M-glyph's tight box — M has no descender, so per-line cell height was undercounted and descender ink on g/p/q/y bled into the row below. Second, the render walk in drawTerminal painted each cell as BG-fill-then-glyph in row-major order, so even when a descender extended into row N+1, that descender ink was painted before row N+1's BG fill and got immediately overwritten by it.
measureCells now uses Pango's metrics.Height() (recommended baseline-to-baseline distance, includes any line-gap the font specifies; falls back to ascent+descent if the font reports Height = 0). This matches what Ghostty derives from Freetype face metrics and what xterm/kitty use.
drawTerminal splits the cell walk into two sequential passes: pass A paints BG fills only, pass B paints glyphs (and captures cursor info). Doing all backgrounds first means a Pass B descender lands on top of any row N+1 BG fill that was already committed in Pass A, instead of being stamped underneath it. This is the canonical CPU-renderer pattern (xterm.js does the same on Canvas); Ghostty avoids it by composing on the GPU instead. Cellcolors lifted out as a small helper since both passes need to resolve a cell's effective fg/bg/inverse identically.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ensureCallbacks was bridging a cgo.Handle into libghostty's void* userdata slot via unsafe.Pointer(uintptr(handle)) — the canonical pattern when the C API takes void*, but go vet's unsafeptr check correctly flags the uintptr->unsafe.Pointer conversion as suspicious because it can't tell that cgo.Handle is GC-pinned. CI lint and Linux/macOS test jobs both fail on it. Add a small static C helper roost_register_userdata that takes a uintptr_t and does the (void*)h cast on the C side. The Go call becomes C.roost_register_userdata(t.c, C.uintptr_t(t.cbsHandle)) — same wire bits, no Go-side unsafe conversion, no vet finding. Drop the now-unused unsafe import. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/roost/session.go`:
- Around line 337-342: QueryColors currently returns hard-coded DefaultTheme
colors; change it to read the live session theme/state so OSC 10/11/12 reflect
current colors. Update the QueryColors closure used in session.go to call
toOSCRGB on the session's current color fields (e.g., s.Theme.Foreground,
s.Theme.Background, s.Theme.Cursor or the equivalent s.State.* fields) instead
of DefaultTheme, so QueryColors returns the session's current colors.
- Around line 192-203: term.SetPtyWriter currently causes each libghostty
callback to spawn a writer goroutine that blocks on writeMu; instead create a
single per-session buffered byte channel (e.g. ptyWriteChan) and a dedicated
per-session writer goroutine that drains that channel and performs the actual
PTY writes (the existing per-tab writer loop used with glib.IdleAdd). Change
term.SetPtyWriter to enqueue raw byte slices into this channel (use a
non-blocking/drop policy or bounded buffer to avoid unbounded growth) and remove
any code paths that spawn new writer goroutines per callback; reference
term.SetPtyWriter, QueueWrite, writeMu and the per-session writer loop when
implementing this switch.
🪄 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: 6c2ba291-ff08-481c-bb90-77da44a41b02
📒 Files selected for processing (2)
cmd/roost/render.gocmd/roost/session.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/ghostty/callbacks.go (1)
102-104:⚠️ Potential issue | 🟠 MajorCheck and propagate clear-path
ghostty_terminal_setfailures.Line 103 and Line 121 ignore libghostty return codes when clearing callbacks. That swallows failures and can leave callback state unexpectedly active.
Patch
func (t *Terminal) SetPtyWriter(fn func([]byte)) error { @@ t.cbs.writePty = fn if fn == nil { - C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_WRITE_PTY, nil) + if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_WRITE_PTY, nil); rc != C.GHOSTTY_SUCCESS { + return fmt.Errorf("clear WRITE_PTY: %d", int(rc)) + } return nil } @@ func (t *Terminal) SetDeviceAttributes(d *DeviceAttrs) error { @@ t.cbs.deviceAttrs = d if d == nil { - C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_DEVICE_ATTRIBUTES, nil) + if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_DEVICE_ATTRIBUTES, nil); rc != C.GHOSTTY_SUCCESS { + return fmt.Errorf("clear DEVICE_ATTRIBUTES: %d", int(rc)) + } return nil }As per coding guidelines, "Return errors instead of logging-and-swallowing; log only at the boundary that handles errors."
Also applies to: 120-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/callbacks.go` around lines 102 - 104, The calls to C.ghostty_terminal_set (e.g., the one using t.c and GHOSTTY_TERMINAL_OPT_WRITE_PTY when fn == nil) ignore and swallow the C return code; change these to capture the integer return, check for non-zero, and return a Go error (e.g., fmt.Errorf("ghostty_terminal_set failed: %d") or wrap with a sentinel) instead of returning nil. Apply the same change to the other occurrence around lines 120-122 so both clear-path branches propagate failures from C.ghostty_terminal_set rather than silently ignoring them.
🧹 Nitpick comments (1)
internal/ghostty/callbacks.go (1)
101-108: Apply Go-side callback state only after C registration succeeds.Each setter currently mutates
t.cbsbefore confirmingghostty_terminal_setsuccess. On failure, the method returns an error but leaves partially-updated Go state.Refactor sketch
func (t *Terminal) SetPtyWriter(fn func([]byte)) error { if err := t.ensureCallbacks(); err != nil { return err } - t.cbs.writePty = fn if fn == nil { - if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_WRITE_PTY, nil); rc != C.GHOSTTY_SUCCESS { + if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_WRITE_PTY, nil); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("clear WRITE_PTY: %d", int(rc)) } + t.cbs.writePty = nil return nil } if rc := C.roost_register_write_pty(t.c); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("set WRITE_PTY: %d", int(rc)) } + t.cbs.writePty = fn return nil }func (t *Terminal) SetDeviceAttributes(d *DeviceAttrs) error { @@ - t.cbs.deviceAttrs = d if d == nil { if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_DEVICE_ATTRIBUTES, nil); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("clear DEVICE_ATTRIBUTES: %d", int(rc)) } + t.cbs.deviceAttrs = nil return nil } if rc := C.roost_register_device_attrs(t.c); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("set DEVICE_ATTRIBUTES: %d", int(rc)) } + t.cbs.deviceAttrs = d return nil }func (t *Terminal) SetColorSchemeDark(dark bool) error { @@ - t.cbs.hasScheme = true + var scheme C.GhosttyColorScheme if dark { - t.cbs.colorScheme = C.GHOSTTY_COLOR_SCHEME_DARK + scheme = C.GHOSTTY_COLOR_SCHEME_DARK } else { - t.cbs.colorScheme = C.GHOSTTY_COLOR_SCHEME_LIGHT + scheme = C.GHOSTTY_COLOR_SCHEME_LIGHT } if rc := C.roost_register_color_scheme(t.c); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("set COLOR_SCHEME: %d", int(rc)) } + t.cbs.colorScheme = scheme + t.cbs.hasScheme = true return nil }Also applies to: 119-126, 137-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/callbacks.go` around lines 101 - 108, The setter currently assigns t.cbs.writePty before calling the C registration, which leaves Go state mutated on C failure; change the logic so the C call (C.ghostty_terminal_set / C.roost_register_write_pty) is performed first and only on success update t.cbs.writePty (for the fn==nil branch: clear the C option first and then set t.cbs.writePty=nil). Apply the same pattern to the other setters referenced (the blocks around 119-126 and 137-145) so that any C registration/unregistration completes successfully before mutating t.cbs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/ghostty/callbacks.go`:
- Around line 102-104: The calls to C.ghostty_terminal_set (e.g., the one using
t.c and GHOSTTY_TERMINAL_OPT_WRITE_PTY when fn == nil) ignore and swallow the C
return code; change these to capture the integer return, check for non-zero, and
return a Go error (e.g., fmt.Errorf("ghostty_terminal_set failed: %d") or wrap
with a sentinel) instead of returning nil. Apply the same change to the other
occurrence around lines 120-122 so both clear-path branches propagate failures
from C.ghostty_terminal_set rather than silently ignoring them.
---
Nitpick comments:
In `@internal/ghostty/callbacks.go`:
- Around line 101-108: The setter currently assigns t.cbs.writePty before
calling the C registration, which leaves Go state mutated on C failure; change
the logic so the C call (C.ghostty_terminal_set / C.roost_register_write_pty) is
performed first and only on success update t.cbs.writePty (for the fn==nil
branch: clear the C option first and then set t.cbs.writePty=nil). Apply the
same pattern to the other setters referenced (the blocks around 119-126 and
137-145) so that any C registration/unregistration completes successfully before
mutating t.cbs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 004eb11f-1f44-4e87-9676-f473030d4afe
📒 Files selected for processing (1)
internal/ghostty/callbacks.go
…lear failures Two CodeRabbit findings against the WRITE_PTY/DEVICE_ATTRIBUTES/COLOR_SCHEME wiring: resolveCallbacks called cgo.Handle.Value with no panic guard. Value panics if the handle was already deleted, which can happen if libghostty fires a callback in the brief window between Terminal.Close calling cbsHandle.Delete and the C-side teardown completing. Add a defer-recover that converts the panic into a nil callbacks return; the call sites already check for nil and skip silently, so this turns a potential crash into a benign drop. SetPtyWriter and SetDeviceAttributes ignored ghostty_terminal_set return codes on the nil-clear path, returning nil unconditionally. If the C call failed, the Go-side state was cleared but libghostty was still pointed at the now-stale callback. Check rc and surface a clear-error so callers can see and react. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ssion CodeRabbits review pointed out that Session.QueueWrite was spawning a goroutine per call, each blocked on writeMu. Under probe-heavy load (libghostty WRITE_PTY responses + keystrokes + mouse events) those goroutines could pile up serialised on the mutex, growing without bound when the PTY stalls. Switch to the canonical Go pattern for serialised I/O: a buffered channel feeding one long-lived writer goroutine. - writeChan (cap 256) carries owned byte slices from senders to the writer. - runWriter drains writeChan in FIFO order and performs short-write looped pty.Write. One goroutine for the lifetime of the Session. - QueueWrite becomes a non-blocking select-send. On stop it drops; on full queue it logs and drops. Blocking would freeze the GTK main thread, which is worse than losing one reply during extreme load. - stopWrite + writerDone chans coordinate teardown: Close signals stopWrite after the pump has exited and waits for writerDone before scheduling libghostty cleanup, so no in-flight pty.Write races with libghostty teardown. - writeMu and the sync import drop out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ge global
CodeRabbit flagged that the QueryColors closure resolved each query against DefaultTheme directly. That works today because nothing mutates the colours after construction, but it bakes a coupling: anyone who ever swaps a sessions theme has to remember to update both libghostty (via SetTheme) and the OSC scanner closure separately. Reading from a single per-session source removes that future foot-gun and keeps the answers internally consistent.
Add a theme field to Session, populate it from DefaultTheme in NewSession, push it into libghostty there, and have the QueryColors closure read s.theme.{Foreground,Background,Cursor}. Reading effective colours from libghostty itself would be cleaner still, but ghostty_terminal_get is main-thread-only and the scanner runs in the pump goroutine, so a session-cached theme is the right plumbing for now.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ghostty/callbacks.go (1)
114-121: Own a copy ofDeviceAttrsinstead of storing caller memory.
roostDeviceAttrsFnreads this later from C callback context, andPrimaryFeaturesis a slice. Keeping the caller’s pointer means any later mutation changes live terminal responses and can race with callback reads. Copy the struct and clonePrimaryFeaturesin the setter so the terminal owns stable data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/callbacks.go` around lines 114 - 121, SetDeviceAttributes currently stores the caller's DeviceAttrs pointer directly (t.cbs.deviceAttrs = d), which can race with C callbacks (roostDeviceAttrsFn) because DeviceAttrs.PrimaryFeatures is a slice; instead allocate a new DeviceAttrs, copy all scalar fields and make a fresh slice copy of PrimaryFeatures (and any other slice or reference fields) and assign that copy to t.cbs.deviceAttrs; handle nil input by clearing t.cbs.deviceAttrs, and keep the function name Terminal.SetDeviceAttributes and the field t.cbs.deviceAttrs as the places to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ghostty/callbacks.go`:
- Around line 101-109: The code currently updates t.cbs fields (e.g.,
t.cbs.writePty) before calling the C registration functions
(ghostty_terminal_set / roost_register_write_pty), which can leave Go state
inconsistent on C failure; change these setters so they do the C call first and
only assign t.cbs.writePty (and other callback fields) after the C call
succeeds, and if the C call fails leave the existing t.cbs value unchanged (or
roll back to the previous value) and return the error; apply the same
commit-after-success/rollback pattern to the other affected setters
(SetDeviceAttributes and SetColorSchemeDark) so a failed C call never leaves
Go’s t.cbs out of sync with libghostty.
---
Nitpick comments:
In `@internal/ghostty/callbacks.go`:
- Around line 114-121: SetDeviceAttributes currently stores the caller's
DeviceAttrs pointer directly (t.cbs.deviceAttrs = d), which can race with C
callbacks (roostDeviceAttrsFn) because DeviceAttrs.PrimaryFeatures is a slice;
instead allocate a new DeviceAttrs, copy all scalar fields and make a fresh
slice copy of PrimaryFeatures (and any other slice or reference fields) and
assign that copy to t.cbs.deviceAttrs; handle nil input by clearing
t.cbs.deviceAttrs, and keep the function name Terminal.SetDeviceAttributes and
the field t.cbs.deviceAttrs as the places to change.
🪄 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: 9060012d-fab5-42c6-b2a5-34055ec1e950
📒 Files selected for processing (4)
cmd/roost/app.gocmd/roost/session.gointernal/ghostty/callbacks.gointernal/ghostty/callbacks_export.go
✅ Files skipped from review due to trivial changes (1)
- cmd/roost/app.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/roost/session.go
CodeRabbit follow-up: Set* methods updated t.cbs before calling into libghostty. If the C call failed, Go-side state was already mutated while libghostty was still pointed at the previous handler. Most surfaceable in the clear path: a failed nil-clear would null t.cbs.writePty before libghostty heard about it, so subsequent callbacks would dispatch to a nil function and silently drop bytes — even though the user got a clear-failed error and might assume the old handler was still running.
Reorder all three setters (SetPtyWriter, SetDeviceAttributes, SetColorSchemeDark) to run the C call first and only commit t.cbs.{writePty, deviceAttrs, hasScheme, colorScheme} after success. SetColorSchemeDark builds the scheme value into a local first so the two-field commit happens together.
Safe by construction: roost_register_* registers the function pointer with libghostty, but libghostty does not invoke any callback until vt_write later processes a query. Since these setters run on the GTK main thread alongside vt_write, no callback can fire in the window between C registration succeeding and the Go-side state commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ghostty/callbacks.go (1)
115-133: CopyDeviceAttrsbefore storing it.
internal/ghostty/callbacks_export.go:54-82dereferencescbs.deviceAttrsandPrimaryFeaturesdirectly. Keeping the caller-owned pointer here makes post-registration mutations observable in the callback path and leaves a race-prone footgun in the API.Patch
func (t *Terminal) SetDeviceAttributes(d *DeviceAttrs) error { if err := t.ensureCallbacks(); err != nil { return err } if d == nil { if rc := C.ghostty_terminal_set(t.c, C.GHOSTTY_TERMINAL_OPT_DEVICE_ATTRIBUTES, nil); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("clear DEVICE_ATTRIBUTES: %d", int(rc)) } t.cbs.deviceAttrs = nil return nil } + attrs := *d + attrs.PrimaryFeatures = append([]uint16(nil), d.PrimaryFeatures...) if rc := C.roost_register_device_attrs(t.c); rc != C.GHOSTTY_SUCCESS { return fmt.Errorf("set DEVICE_ATTRIBUTES: %d", int(rc)) } - t.cbs.deviceAttrs = d + t.cbs.deviceAttrs = &attrs return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/callbacks.go` around lines 115 - 133, SetDeviceAttributes currently stores the caller-owned *DeviceAttrs directly (t.cbs.deviceAttrs = d), which allows post-registration mutation and races; instead make and store an independent copy: allocate a new DeviceAttrs value, deep-copy any reference fields (notably PrimaryFeatures and other slices/ptrs) from the incoming d, call C.roost_register_device_attrs as before, then set t.cbs.deviceAttrs to the newly copied value so the callback path only sees owned, immutable data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/ghostty/callbacks.go`:
- Around line 115-133: SetDeviceAttributes currently stores the caller-owned
*DeviceAttrs directly (t.cbs.deviceAttrs = d), which allows post-registration
mutation and races; instead make and store an independent copy: allocate a new
DeviceAttrs value, deep-copy any reference fields (notably PrimaryFeatures and
other slices/ptrs) from the incoming d, call C.roost_register_device_attrs as
before, then set t.cbs.deviceAttrs to the newly copied value so the callback
path only sees owned, immutable data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df45855f-a13b-4f26-ace2-ba58e76f27e0
📒 Files selected for processing (1)
internal/ghostty/callbacks.go
This was referenced Apr 29, 2026
Merged
This was referenced May 7, 2026
12 tasks
charliek
added a commit
that referenced
this pull request
May 23, 2026
…ail-hard, entitlements cleanup, PtySupervisor double-emit Address four findings from the M6-M9 sub-agent review: **1. Linux reorder events for parity with Mac.** Sub-agent #8 caught that the Mac M9 polish wired `Workspace.tabsReordered` / `projectsReordered` events but Linux's `crates/roost-linux/src/daemon/state.rs::reorder_tabs` / `reorder_projects` mutate positions in-place without emitting. Added two new `WorkspaceEvent` variants (`TabsReordered`, `ProjectsReordered`) with the same payload shape as Mac (the post-reorder display order — supplied prefix + sorted unlisted). Both reorder methods now `events.send(...)` after persist. The GTK app.rs match arm currently drops the events with a comment — the UI's drag-reorder path already updates AdwTabBar inline before firing the RPC, so re-applying the broadcast would be double-work. Cross-client convergence is a follow-up slice; the event is emitted so a future `events.subscribe` consumer (or a sibling GTK process) can react. **2. `bundle.sh` codesign fail-hard.** Sub-agent #5 caught that the previous `codesign … || echo "warn ... (continuing)"` form swallowed signature failures with exit 0. A botched signature silently produces a Gatekeeper-rejected app + a notarization failure at release time. Replaced with a `codesign_or_die` helper that exits 1 on failure unless `ROOST_ALLOW_UNSIGNED=1` is set (for the rare dev case where Xcode CLT codesign is unavailable). **3. `Roost.entitlements` cleanup.** Sub-agent #6 caught that `com.apple.security.cs.disable-library-validation` was included based on a misreading of the docs — library validation governs in-process `dlopen` / framework loads, not `execve` of a separately-signed embedded binary (the `Contents/Resources/bin/ roostctl` exec path is governed by Gatekeeper / quarantine, not library validation). Apple notarization will scrutinize any unnecessary hardened-runtime entitlements, so removed. Plist is now an empty dict with a long comment block listing what we INTENTIONALLY don't need (allow-jit, network.*, sandbox) and the conditions under which we'd add each. **4. PtySupervisor double-`.tabExited` race.** Sub-agent #3.b walked through a scenario where the read source yields `.eof` and the bg-reap teardown yields `.forcedExit` onto the same AsyncStream FIFO; the drain task would emit `.tabExited` twice (once from `reapAndCleanup` on `.eof`, once from `.forcedExit`). Added an `emittedExit` bool to the drain task that suppresses the second emit; `reapAndCleanup` is now `@discardableResult -> Bool` so the drain knows whether it actually emitted (vs. the session-already-gone-from-close-race case where it returns false without emitting and the subsequent `.forcedExit` is the one that should fire). All emit-once paths preserved. `swift test`: 132/132. `cargo test -p roost-linux`: 20/20 state-machine tests + 34 IPC-dispatch + the rest still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 23, 2026
* fix(linux): Linux IPC robustness sweep (closes #80) Address the deferred CR items from the inline-core refactor (PR #78). Reconciled against the merged code: #1 (parse-error reply), #10 (pending HashSet), #13 (smoke-test ordering) were already done; the rest are fixed here. Project is early-stage, so the bar is long-term correctness. PTY supervisor (crates/roost-linux/src/daemon/pty.rs, tab_session.rs): - #11: wait task now removes the session on child exit (sessions behind Arc<Mutex>), so writes to a dead PTY return NotFound instead of silently succeeding. - #12: close() escalates SIGHUP -> SIGKILL after a grace period. The cloned portable-pty ChildKiller only sends SIGHUP, so a shell that traps/ignores it used to outlive close(); a reaped flag + pid watchdog force-kill it, mirroring the Mac teardown. - #8: send_input/send_resize feed one per-tab serial channel drained by a single task, so keystrokes can't reorder (the old per-call tokio::spawn could under the multi-thread runtime). Workspace state (crates/roost-linux/src/daemon/state.rs): - #4: new project/tab position uses max(position)+1, not len()/count(), so a delete-then-create can't collide. - #5: every mutator now publishes its event while still holding `inner`, so broadcast order matches commit order; persist runs after the drop. The events-resync Resync path is preserved. Server framing (crates/roost-ipc/src/server.rs): - #2: on an envelope decode failure, peel `id` from the raw JSON (string- or number-encoded) so the parse-error reply lands at the client's id, not 0. Lifecycle (crates/roost-linux/src/main.rs, ipc.rs, app.rs, messages.rs): - #7: IPC bind is now a synchronous startup requirement; bind failure aborts startup instead of leaving the UI socket-less. - #6: a second launch dials the new `app.activate` op; the handler forwards to the GTK thread which raises the running window. - #9: events.subscribe returns not-implemented (Linux + Mac) instead of a false {} ACK, so clients don't wait forever. Real streaming is deferred to its first consumer (roostctl watch). Tests: write-after-exit -> NotFound, SIGHUP-ignoring child force-killed, send_input ordering, position max+1 after delete, id-preserving parse error, bind-failure surfaced, events.subscribe not-implemented. Rust workspace + Mac build/clippy/tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(mac): IPC handler dispatch harness (#80 follow-up) The Rust handler has tests/ipc_dispatch.rs; the Mac IPCHandlerImpl had no equivalent, leaving its hand-written cross-cutting logic untested — strict unknown-field rejection (decodeParams), ipcDim u16 validation, mapWorkspace/mapPty error-code mapping, the not-implemented/unknown-op paths, and result encoding. The two handlers must stay behaviorally convergent over the shared wire contract, so this guards that. Calls IPCHandlerImpl.handle(op:params:) directly — no socket. Exercises only non-PTY-spawning ops to avoid the forkpty/swift-testing SIGTRAP that disables the PTY paths elsewhere (tab.open stays on the manual pass; the error-mapping ops reach the supervisor only on the lookup-fails path). Seeds 8 tests: events.subscribe→not-implemented (the #9 path), unknown-op, unknown-field strictness, project.rename→not-found, tab.resize out-of-range→invalid-param, tab.resize missing→not-found, identify profile echo, project.create→tab.list round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): address CodeRabbit review on #80 sweep Resolves 4 of CodeRabbit's 6 findings on PR #86 (the persistence-ordering finding is pre-existing/orthogonal and tracked separately). pty.rs: - Major: spawn() acquired the master reader/writer AFTER spawn_command, so a try_clone_reader/take_writer failure returned with a live child and no wait task — an orphaned PTY. Acquire reader+writer BEFORE spawning, making spawn_command the last fallible step (no orphan). - Major: the exit wait task removed the session by tab_id alone, which could evict a newer session that reused the same tab_id (close() frees the slot synchronously; pty_smoke reuses id 42). Gate the remove on Arc::ptr_eq of the per-spawn `reaped` identity so only the owning waiter deletes. (My prior "monotonic ids" comment over-assumed caller discipline; the supervisor can't rely on it.) - Major: input and resize were unified at TabSession but re-split into the supervisor's two channels (select!), so mixed input/resize wasn't FIFO end-to-end. Merge them into one WriterCmd channel drained by a single ordered loop — genuine submission-order delivery. ipc.rs + messages.rs: - Minor: app.activate ACK'd any payload. Add an empty strict AppActivateParams and decode it so the op validates its envelope (rejects unknown fields) like every other op. IPCHandlerTests.swift: - Minor: replace `as? T != nil` (SwiftLint prefer_type_checking) with a meaningful empty-array assertion on a fresh project's tabs. Rust clippy --all-targets clean; roost-ipc + roost-linux tests green (A3 ordering + dup-spawn still pass under the new channel/guard). Mac swift build + 140 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): order state.json persistence by commit seq (CR #3) Resolves the last CodeRabbit finding on PR #86. persist_async did synchronous file I/O on the caller thread after dropping `inner`, so two concurrent mutators could write state.json out of commit order — a slow earlier commit could clobber a newer one, regressing restart durability. Keep persistence synchronous (state_persist.rs and restart-reload depend on it; an async writer thread would break that), but make it ordered: - Inner.persist_seq: a monotonic commit counter bumped under the lock when a snapshot is taken, so the seq reflects commit order. - snapshot_for_persist now returns (SnapshotFile, seq). - persist(seq, snapshot) serializes on a new persist_guard mutex and drops any snapshot whose seq is <= the highest already written, so the newest committed snapshot always wins regardless of which thread races first. Newest-wins verified by persist_drops_stale_out_of_order_writes. Rust clippy --all-targets clean; roost-ipc + roost-linux suites green (incl. state_persist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt ipc.rs import block The AppActivateParams import addition left the use block un-reflowed; CI's rust-lint (cargo fmt --all -- --check) caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
charliek
added a commit
that referenced
this pull request
May 28, 2026
…gray bar (#144) * fix(osc): synthesise OSC 10/11/12 query replies so codex renders its gray bar Codex (and reportedly claude-code) only emits its highlighted prompt-row background SGR sequence *after* the terminal answers an OSC 11 background-color query. libghostty-vt's color-operation handler is a no-op for the .query arm (see stream_terminal.zig upstream), so without us answering the queries the prompt row renders against the canvas bg and looks invisible. The legacy Go port fixed this in PR #8 ("Sharper TUI rendering"); the Rust + Swift ports forwarded the ColorQuery event to the daemon report path but never synthesised a reply. Both UIs already emitted `ColorQuery(n)` events from the OSC scanner; this PR closes the loop: - `crates/roost-osc/src/lib.rs`: new dependency-free `format_color_query_response(n, (r,g,b)) -> Option<Vec<u8>>` that produces the standard XTerm `\x1b]N;rgb:RRRR/GGGG/BBBB\x07` form (16-bit-per-channel, BEL-terminated) — byte-identical to the legacy Go `internal/osc/scanner.go:294-298`. 5 unit tests covering 10/11/12, unknown N, and channel order. - `crates/roost-linux/src/app.rs`: the output-drain task intercepts ColorQuery before forwarding to `report_osc_event` and writes the reply via `session.send_input(...)` — same per-tab serial channel as keystrokes so ordering is preserved relative to typing. - `mac/Sources/Roost/TerminalView.swift`: `appendBytes` synthesises the reply from the current theme and routes it through the existing `onKey` callback (same destination as user keystrokes — bytes into the PTY's stdin via the tab's keystroke continuation). New `formatColorQueryResponse(n:, color:)` static helper mirrors the Rust formatter byte-for-byte. 5 swift-testing cases mirror the Rust suite. Verified visually on Mac: codex's "›" prompt row now renders with the expected gray-card background, matching cmux / legacy Go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: prefer range pattern for OSC color-query number match * docs: clarify OSC reply ordering + document dynamic-color limitation (#145) Address Cursor's review feedback on PR #144: - Update roost-osc module docs to reflect that ColorQuery events now have a real synthesised reply path via format_color_query_response. - Tighten the wiring-site comments on both UIs to make the FIFO ordering guarantee precise (serialised with other PTY-input writes once enqueued, not against PTY output still draining). - Document the static-theme vs dynamic-OSC-set limitation inline so a future reader sees the trade-off + the link to follow-up #145. Codex (the primary use case) only queries, so it's unaffected; vim colorscheme-change plugins would hit it. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Four commits that bring Roost's rendering and terminal-protocol behaviour up to parity with cmux / Ghostty / Cursor for AI-agent TUIs (Codex, Claude Code, OpenCode):
0dad9c8— Custom Cairo geometric renderer for box-drawing (U+2500–U+257F) and block-element (U+2580–U+259F) glyphs, ported from Ghostty'sfont/sprite/draw/{box,block}.zig. Pango still handles all other glyphs. Fixes the OpenCode wordmark seams and Codex/Claude bordered chrome.1a4b7b4— Default dark theme installed on every new terminal via libghostty'sOPT_COLOR_FOREGROUND/BACKGROUND/CURSOR/PALETTE. Mirrors cmux's dark palette for indices 0–15; generates indices 16–255 from libghostty's standard xterm cube + gray ramp algorithm. Also wires cursor + selection ribbon colours through the sameThemevalue type so a futureLoadTheme(path string) (Theme, error)is a drop-in.138233e—WRITE_PTY/DEVICE_ATTRIBUTES/COLOR_SCHEMEcallbacks wired into libghostty-vt. Programs that probe terminal state (DA1, DSR, kitty keyboard, color scheme) now get answers instead of silence. Single sharedcgo.Handleper terminal carries the callback state through libghostty's userdata slot.15c8b60— Synthesise OSC 10/11/12 query responses in the existinginternal/oscscanner. libghostty-vt's color-operation handler explicitly drops the.queryarm (stream_terminal.zig:616-618), so without this Codex's OSC 11 query ("what is your background?") got silence and Codex skipped emitting its prompt-row gray-bar BG SGR. The scanner runs alongside libghostty in the per-session pty pump and writes the response back via the sameSession.QueueWritekeystrokes use. Becomes a no-op once libghostty fills in.queryupstream.Visual before/after
Known remaining issue
Cells with explicit BG fills now reveal a pre-existing cell-height issue:
session.go'smeasureCells()useslayout.PixelSize("M")which is 1–2px shy of the font's full ascent+descent, so capital letters and descenders extend slightly outside the cell box. Visible as "clipping" wherever a cell has an explicit BG (Codex's prompt row, OpenCode's prompt). Fixing this is a focused follow-up — switch to Pango font metrics (ascent + descent + leading) for cell height. Out of scope for this PR.Test plan
go test ./...— all pass (sprite geometry tests, OSC scanner regression + new query tests,internal/ghosttybuild)./build/build.sh build— clean cgo compile, no missing symbolscodexfrom a Roost shell — header box closed, prompt row has gray cardclaude— input divider renders cleanlyopencode— wordmark solid, prompt row has gray bgvim,htop,git log --graph,tree— unchanged; standard ANSI colors render correctly; selection ribbon and cursor unchangedinternal/osc/scanner_test.go)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests