Skip to content

feat(wsh): add attach command to observe and interact with running terminal blocks#3267

Open
dfbb wants to merge 16 commits into
wavetermdev:mainfrom
dfbb:wsh-attach
Open

feat(wsh): add attach command to observe and interact with running terminal blocks#3267
dfbb wants to merge 16 commits into
wavetermdev:mainfrom
dfbb:wsh-attach

Conversation

@dfbb
Copy link
Copy Markdown

@dfbb dfbb commented May 1, 2026

Overview

wsh attach connects a local terminal to any running Wave Terminal block, streaming its live output and forwarding local keyboard input to the remote session. It is designed for monitoring and interacting with long-running processes, CI jobs, or AI coding agents (e.g. Claude Code) from a separate window or SSH session.

Usage

# Interactive block selector (workspace → tab → block)
wsh attach

# Attach directly by block ID
wsh attach <blockId>

In-session key bindings:

Key Action
Ctrl+Arrow Pan the local viewport in any direction
Ctrl-A j Pan viewport down (alternative for terminals where Ctrl+Down is intercepted)
Ctrl-A k Pan viewport up (alternative for terminals where Ctrl+Up is intercepted)
Ctrl-A d Detach
Ctrl-A r Force full redraw
Ctrl-A s Resync — rebuild emulator state from a fresh snapshot

Design

Viewport model

Wave Terminal blocks have a fixed PTY size stored in Block.RuntimeOpts.TermSize. wsh attach reads this size at startup and creates a moveable viewport window sized to the local terminal. The remote PTY is never resized; only the viewport offset is adjusted with Ctrl+Arrow. This lets a narrow local terminal pan across a wide remote TUI (e.g. 220-column Claude Code) without interrupting it.

Keyboard input typed in the local terminal is forwarded to the remote block via ControllerInputCommand, so the attached session is fully interactive.

Output pipeline

  1. SnapshotWaveFileReadStreamCommand streams the current block file (a 2 MB circular buffer of raw PTY bytes) into an xterm-go terminal emulator, replaying the full screen state.
  2. Live events — A WPS subscription (Event_BlockFile) delivers new PTY chunks as FileOp_Append events. An eventBuffer queues events while the snapshot is in flight; a post-snapshot cutoff timestamp discards events already captured in the snapshot, preventing double-feed corruption.
  3. Debounced render loop — A 16 ms time.Timer (not time.Ticker, which has a channel-drain race on Reset) coalesces rapid PTY bursts. Full-screen TUI repaints (typically ESC[2J + 3–4 event chunks arriving within ~2 ms) are always fully consumed before the emulator is read for rendering.
  4. Resync (Ctrl-A s) — Resets the xterm-go emulator and replays a fresh snapshot, recovering from any state drift caused by emulator divergence.

Diff renderer (pkg/waveattach/screen.go)

Each Render call compares every cell against the previous frame's renderedCell (character, fg/bg colors, underline style, wide-char width) and emits only changed cells with explicit cursor positioning (ESC[row;colH). This minimises output bandwidth and avoids full-screen flicker on slow connections.

Additional rendering features:

  • SGR sync — full 16/256/RGB foreground, background, and underline color; bold, italic, dim, blink, inverse, strikethrough, overline.
  • Wide-character support — 2-cell wide characters (CJK, emoji) with proper continuation-cell tracking.
  • Alt-screen sync — mirrors ESC[?1049h/l transitions so TUI apps that use the alternate screen buffer display correctly.
  • Cursor style sync — mirrors DECSCUSR cursor shape/blink state from the remote.
  • ESC[2J detection — schedules a full clear+redraw when the remote TUI erases the display, preventing stale cells from showing through.
  • yBase tracking — detects scroll-offset changes in the xterm-go buffer and invalidates the diff cache, preventing incorrect partial updates after the remote terminal scrolls.

Bug fix: EventRecv ordering (pkg/wshutil/wshrpc.go)

The WshRpc message loop previously dispatched every incoming message — including eventrecv — to a new goroutine. When 4 back-to-back PTY events arrive (a single TUI frame split across chunks), 4 goroutines race to acquire the event-buffer mutex and write chunks in non-deterministic order. This produces mixed-frame content in the xterm-go emulator — visible as garbled / overlaid cells on every TUI repaint (e.g. when pressing Ctrl+L in Claude Code).

Fix: eventrecv is now handled synchronously in the dispatch loop, exactly as streamdata and streamdataack are already handled. This guarantees arrival-order processing. Event handlers are expected to be fast (non-blocking channel sends or mutex-protected buffer writes), so processing them on the dispatch goroutine adds negligible latency.

Files Changed

File Description
cmd/wsh/cmd/wshcmd-attach.go wsh attach CLI entrypoint (cobra command, argument parsing)
pkg/waveattach/auth.go Wave data-dir discovery (macOS/Linux/WAVETERM_DATA_HOME), Unix socket connect, JWT auth via AuthenticateCommand
pkg/waveattach/selector.go Interactive block selector: lists workspaces → tabs → blocks with a simple arrow-key TUI
pkg/waveattach/output.go Snapshot streaming, WPS event subscription, eventBuffer ordering, debounced ViewportRenderer
pkg/waveattach/screen.go Viewport (xterm-go wrapper), diff renderer, SGR/cursor/alt-screen/wide-char support
pkg/waveattach/attach.go Main attach loop: raw-mode stdin, keyboard input forwarding, Ctrl+Arrow viewport pan, prefix-key state machine
pkg/wshutil/wshrpc.go Synchronous eventrecv dispatch to fix PTY chunk ordering

Adds `wsh attach` — a command that streams the live output of any Wave
Terminal block to a local terminal window without affecting the remote
session. Useful for monitoring long-running processes, CI jobs, or AI
coding agents from a separate window or SSH session.

Key capabilities:
- Interactive block selector (workspace → tab → block)
- Live PTY streaming via snapshot + WPS event subscription
- Viewport model: server PTY size is fixed; local terminal is a
  moveable window into the remote screen (Ctrl+Arrow to pan)
- Diff-based renderer that emits only changed cells per frame,
  with full SGR, wide-character, alt-screen, and cursor-style sync
- Debounced render loop (16 ms) coalesces rapid PTY bursts so that
  full-screen TUI repaints are always consumed before rendering
- Resync command (Ctrl-A s) rebuilds xterm-go state from a fresh
  snapshot when local state drifts from the remote

Bug fix included: EventRecv messages are now dispatched synchronously
in the WshRpc message loop (same pattern as StreamData/StreamDataAck)
so that back-to-back PTY events are always processed in arrival order.
Without this fix, concurrent goroutines race to write PTY chunks into
the terminal emulator, producing mixed-frame garbling.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 1, 2026

Walkthrough

This pull request adds a full Wave Terminal attach feature and remote-mode support: a new wsh attach CLI, waveattach package (auth/connect, Attach, StreamOutput, Viewport rendering, input prefix handling, selector, and platform stubs), remote-entry reverse proxy and tests, Electron remote-mode wiring (startup, session injection, route ids), frontend WebSocket/header and route-id changes, router and event additions, config/schema updates, and supporting tests and docs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 19.23% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main feature: adding an attach command to observe and interact with running terminal blocks, which aligns with the primary changes in this changeset.
Description check ✅ Passed The description provides detailed context on the attach command feature, design decisions, usage, and implementation, all of which directly relate to the changeset's primary objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 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/wsh/cmd/wshcmd-attach.go`:
- Around line 11-22: The attach command advertises a --list mode but never
registers or handles that flag; add a boolean flag to attachCmd (e.g.,
Flags().Bool("list", false, "...")) and update attachRun to check that flag
early: when true, call the existing pkg/waveattach.ListTermBlocks selector
function to fetch the blocks, print or format the list to stdout, and return nil
(skip normal attach flow). Ensure the flag is declared on the attachCmd variable
(not root) and that attachRun uses cmd.Flags().GetBool("list") to branch before
attempting to parse a block id or perform the attach logic.

In `@pkg/waveattach/attach.go`:
- Around line 267-275: The current read-only attach path forwards local stdin to
the remote via sendInput after pk.feed, which enables remote writes; stop
forwarding by removing the sendInput call in the branch that handles filtered
input (the code invoking pk.feed and sendInput) so that pk.feed returns and any
output is handled locally (via the out buffer) only; also remove or guard the
identical sendInput usage in the similar block around the other occurrence (the
one you noted at 304-310) so ControllerInputCommand is not generated remotely
and input remains local-only unless an explicit opt-in mode is added.

In `@pkg/waveattach/auth_test.go`:
- Around line 24-27: The fallback tests can be escaped if the environment has
XDG_DATA_HOME set; update the fallback tests (e.g.,
TestResolveDataDir_FallbackProd and the other fallback tests in this file) to
explicitly clear XDG_DATA_HOME at their start by calling
t.Setenv("XDG_DATA_HOME", "") so ResolveDataDir() will use the temporary HOME
fallback; apply the same t.Setenv call to the other two fallback test functions
in this file.

In `@pkg/waveattach/auth.go`:
- Around line 64-69: ResolveDataDir currently returns the first existing
candidate dir which may be stale; change it to only return a candidate that
actually hosts a running Wave instance by checking for the required
socket/database (e.g. check filepath.Join(candidate, "wave.sock") exists and is
connectable, or the DB file) and continue iterating on error; alternatively (or
additionally) update Connect to not treat a single failed connect as fatal but
continue through the candidates list until a successful net.Dial/connection to
"wave.sock" is made, referencing ResolveDataDir and Connect to implement the
socket existence/connectivity check and iteration logic.

In `@pkg/waveattach/promptsp_test.go`:
- Around line 35-64: TestPromptSP_RealData reads fixtures from /tmp which makes
the unit test non-hermetic; change it to load fixtures from a repo-controlled
test corpus (e.g., testdata/) or require an explicit integration opt-in.
Specifically, update TestPromptSP_RealData to look for files under "testdata"
(not /tmp) and call t.Skipf if they are absent, or alternatively guard the whole
test with testing.Short() or a custom -integration flag so it only runs when
explicitly enabled; keep the existing logic that writes to newViewport and calls
vp.Write/vp.Render unchanged except for the new file paths/gating. Ensure any
added flag or skip logic is documented in the test comment.

In `@pkg/waveattach/screen.go`:
- Around line 293-301: The current workaround in the render pass unconditionally
replaces any cell where ch == "%" and cell.AttributeData.IsBold()/IsInverse()
with a space, which can erase legitimate content; instead, narrow the rule to
the actual PROMPT_SP artifact by detecting its exact signature before mutating
rendered cells: relocate this logic out of the generic renderer into the
terminal input/event processing where raw sequences are available (the code that
decodes incoming terminal bytes), or at minimum check for a stronger
prompt-specific pattern (e.g., the single-cell bold+inverse '%' that appears at
the prompt input column/rightmost column immediately after a wrap/no-content
cell and not surrounded by other styled cells) before changing ch, and only
mutate the exact matching cell (use the same identifiers ch, cell,
cell.AttributeData.IsBold/IsInverse to implement the more specific match).

In `@pkg/wshutil/wshrpc.go`:
- Around line 432-438: The main dispatch loop (runServer) currently calls
handleEventRecv synchronously for every Command_EventRecv, which blocks
runServer because handleEventRecv -> EventListener.RecvEvent executes listener
callbacks inline; instead, create a dedicated ordered event processor (a
buffered channel + single goroutine) and send EventRecv messages to it from
runServer so only that processor serializes arrival order; inside that processor
call handleEventRecv sequentially for events that require ordering (e.g.,
PTY/ordered stream event types determined from msg.Event.Type), but for other
EventRecv types dispatch to listeners asynchronously (goroutine per listener or
worker pool) to avoid stalling runServer and exitCh; update runServer to push to
the new queue rather than calling handleEventRecv directly and ensure the
controller-status listener remains non-blocking or uses a buffered exitCh.
🪄 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: CHILL

Plan: Pro

Run ID: 62ac5b0a-b69d-4a54-8b3d-0a78d9b7c40f

📥 Commits

Reviewing files that changed from the base of the PR and between efd450f and e64d1fe.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (13)
  • cmd/wsh/cmd/wshcmd-attach.go
  • go.mod
  • pkg/waveattach/attach.go
  • pkg/waveattach/attach_test.go
  • pkg/waveattach/auth.go
  • pkg/waveattach/auth_test.go
  • pkg/waveattach/output.go
  • pkg/waveattach/output_test.go
  • pkg/waveattach/promptsp_test.go
  • pkg/waveattach/screen.go
  • pkg/waveattach/screen_test.go
  • pkg/waveattach/selector.go
  • pkg/wshutil/wshrpc.go

Comment on lines +11 to +22
var attachCmd = &cobra.Command{
Use: "attach [blockid]",
Short: "attach to a Wave Terminal block from an external terminal",
Long: "Attach to a running term block in Wave Terminal. Press Ctrl+A D to detach.",
Args: cobra.MaximumNArgs(1),
RunE: attachRun,
DisableFlagsInUseLine: true,
}

func init() {
rootCmd.AddCommand(attachCmd)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add the advertised --list entrypoint.

This command never registers or handles a --list flag, so wsh attach --list will currently fail as an unknown option even though it is one of the PR’s documented usage modes. Since pkg/waveattach/selector.go already exposes ListTermBlocks, this looks like a missing CLI branch rather than a missing backend capability.

Also applies to: 24-40

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/wsh/cmd/wshcmd-attach.go` around lines 11 - 22, The attach command
advertises a --list mode but never registers or handles that flag; add a boolean
flag to attachCmd (e.g., Flags().Bool("list", false, "...")) and update
attachRun to check that flag early: when true, call the existing
pkg/waveattach.ListTermBlocks selector function to fetch the blocks, print or
format the list to stdout, and return nil (skip normal attach flow). Ensure the
flag is declared on the attachCmd variable (not root) and that attachRun uses
cmd.Flags().GetBool("list") to branch before attempting to parse a block id or
perform the attach logic.

Comment thread pkg/waveattach/attach.go
Comment on lines +267 to +275
if len(filtered) > 0 {
var out bytes.Buffer
action, err := pk.feed(filtered, &out)
if err != nil {
return err
}
if out.Len() > 0 {
sendInput(rpcClient, blockId, out.Bytes())
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Remove remote stdin forwarding from this read-only attach path.

Every non-local keystroke eventually reaches ControllerInputCommand, which means an observer can still type into the live shell. That breaks the PR’s core “read-only / without affecting the remote session” guarantee and turns attach into a write path. This should stay local-only unless you introduce a separate explicit opt-in mode for remote input.

Also applies to: 304-310

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/waveattach/attach.go` around lines 267 - 275, The current read-only
attach path forwards local stdin to the remote via sendInput after pk.feed,
which enables remote writes; stop forwarding by removing the sendInput call in
the branch that handles filtered input (the code invoking pk.feed and sendInput)
so that pk.feed returns and any output is handled locally (via the out buffer)
only; also remove or guard the identical sendInput usage in the similar block
around the other occurrence (the one you noted at 304-310) so
ControllerInputCommand is not generated remotely and input remains local-only
unless an explicit opt-in mode is added.

Comment on lines +24 to +27
func TestResolveDataDir_FallbackProd(t *testing.T) {
home := t.TempDir()
t.Setenv("HOME", home)
t.Setenv("WAVETERM_DATA_HOME", "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unset XDG_DATA_HOME in the fallback tests.

On Linux, ResolveDataDir() checks XDG_DATA_HOME before $HOME/.waveterm*. If the runner already exports it to a path containing a Wave dir, these tests can escape the temp home and become flaky.

Suggested hardening
 func TestResolveDataDir_FallbackProd(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)
+	t.Setenv("XDG_DATA_HOME", "")
 	t.Setenv("WAVETERM_DATA_HOME", "")
 	prod := filepath.Join(home, ".waveterm")
 func TestResolveDataDir_FallbackDev(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)
+	t.Setenv("XDG_DATA_HOME", "")
 	t.Setenv("WAVETERM_DATA_HOME", "")
 	dev := filepath.Join(home, ".waveterm-dev")
 func TestResolveDataDir_NoneFound(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)
+	t.Setenv("XDG_DATA_HOME", "")
 	t.Setenv("WAVETERM_DATA_HOME", "")
 	if _, err := ResolveDataDir(); err == nil {

Also applies to: 41-44, 58-61

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/waveattach/auth_test.go` around lines 24 - 27, The fallback tests can be
escaped if the environment has XDG_DATA_HOME set; update the fallback tests
(e.g., TestResolveDataDir_FallbackProd and the other fallback tests in this
file) to explicitly clear XDG_DATA_HOME at their start by calling
t.Setenv("XDG_DATA_HOME", "") so ResolveDataDir() will use the temporary HOME
fallback; apply the same t.Setenv call to the other two fallback test functions
in this file.

Comment thread pkg/waveattach/auth.go
Comment on lines +64 to +69
for _, candidate := range candidates {
if st, err := os.Stat(candidate); err == nil && st.IsDir() {
return candidate, nil
}
}
return "", fmt.Errorf("Wave data directory not found. Is Wave running? (set $WAVETERM_DATA_HOME to override)")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't stop at the first existing data directory.

ResolveDataDir() returns as soon as a candidate directory exists, but Connect() later requires a live wave.sock there. If both prod and dev data dirs exist and only the later one is running, wsh attach will fail on the stale first match instead of continuing to a usable candidate. Prefer the first directory that has the required socket/database, or keep iterating in Connect() until you find one.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/waveattach/auth.go` around lines 64 - 69, ResolveDataDir currently
returns the first existing candidate dir which may be stale; change it to only
return a candidate that actually hosts a running Wave instance by checking for
the required socket/database (e.g. check filepath.Join(candidate, "wave.sock")
exists and is connectable, or the DB file) and continue iterating on error;
alternatively (or additionally) update Connect to not treat a single failed
connect as fatal but continue through the candidates list until a successful
net.Dial/connection to "wave.sock" is made, referencing ResolveDataDir and
Connect to implement the socket existence/connectivity check and iteration
logic.

Comment thread pkg/waveattach/output.go
Comment on lines +154 to +173
rpcClient.EventListener.On(wps.Event_BlockFile, func(ev *wps.WaveEvent) {
var fed wps.WSFileEventData
if err := utilfn.ReUnmarshal(&fed, ev.Data); err != nil {
return
}
if fed.ZoneId != blockId || fed.FileName != wavebase.BlockFile_Term {
return
}
if fed.FileOp != wps.FileOp_Append {
return
}
data, err := base64.StdEncoding.DecodeString(fed.Data64)
if err != nil {
return
}
if err := buf.write(time.Now(), data, vp); err != nil {
return
}
vr.requestRender()
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Serialize viewport writes through the same synchronization path as resize/move/render.

buf.write(..., vp) mutates the emulator from the event-listener goroutine, while pkg/waveattach/attach.go concurrently calls vp.Resize, vp.Reset, vp.Move*, and vr.Render(). ViewportRenderer.mu only protects Render(), so this can still race and produce torn frames or -race failures under load. Please funnel all Viewport mutations through one goroutine or one shared lock.

Comment on lines +35 to +64
// TestPromptSP_RealData uses captured snapshot+event bytes.
func TestPromptSP_RealData(t *testing.T) {
snapshot, err := os.ReadFile("/tmp/snapshot.bin")
if err != nil {
t.Skipf("no snapshot.bin: %v", err)
}
var events [][]byte
for i := 0; ; i++ {
data, err := os.ReadFile(fmt.Sprintf("/tmp/event_%d.bin", i))
if err != nil {
break
}
events = append(events, data)
}

vp := newViewport(24, 139, 80, 24)
if _, err := vp.Write(snapshot); err != nil {
t.Fatalf("snapshot: %v", err)
}
for i, ev := range events {
if _, err := vp.Write(ev); err != nil {
t.Fatalf("event %d: %v", i, err)
}
var out strings.Builder
vp.Render(&out)
if strings.Contains(out.String(), "%") {
t.Errorf("event %d render has %%", i)
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid /tmp fixtures in a unit test.

This test depends on undeclared files outside the repo, so it can silently exercise arbitrary stale captures when they happen to exist on the runner. Move the corpus into testdata/ or gate this behind an explicit integration-test opt-in so the default unit suite stays hermetic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/waveattach/promptsp_test.go` around lines 35 - 64, TestPromptSP_RealData
reads fixtures from /tmp which makes the unit test non-hermetic; change it to
load fixtures from a repo-controlled test corpus (e.g., testdata/) or require an
explicit integration opt-in. Specifically, update TestPromptSP_RealData to look
for files under "testdata" (not /tmp) and call t.Skipf if they are absent, or
alternatively guard the whole test with testing.Short() or a custom -integration
flag so it only runs when explicitly enabled; keep the existing logic that
writes to newViewport and calls vp.Write/vp.Render unchanged except for the new
file paths/gating. Ensure any added flag or skip logic is documented in the test
comment.

Comment thread pkg/waveattach/screen.go
Comment on lines +293 to +301
// Suppress zsh PROMPT_SP % — bold+inverse % written mid-line because
// Wave shell integration leaves the cursor at the prompt input column,
// not col=0. The self-clearing wrap mechanism never fires, so the %
// persists in the buffer and our diff render would show it indefinitely.
if ch == "%" && cell.AttributeData.IsBold() != 0 && cell.AttributeData.IsInverse() != 0 {
ch = " "
cell.Fg = 0
cell.Bg = 0
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This PROMPT_SP workaround can erase real terminal content.

This rewrites every bold+inverse % cell to a space, regardless of origin. Any app that legitimately renders that combination will display incorrectly in attach, which is supposed to be a faithful observer. Please narrow this to a stronger prompt-specific signature or move the workaround to a place where you can identify the exact bad sequence instead of patching rendered cells globally.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/waveattach/screen.go` around lines 293 - 301, The current workaround in
the render pass unconditionally replaces any cell where ch == "%" and
cell.AttributeData.IsBold()/IsInverse() with a space, which can erase legitimate
content; instead, narrow the rule to the actual PROMPT_SP artifact by detecting
its exact signature before mutating rendered cells: relocate this logic out of
the generic renderer into the terminal input/event processing where raw
sequences are available (the code that decodes incoming terminal bytes), or at
minimum check for a stronger prompt-specific pattern (e.g., the single-cell
bold+inverse '%' that appears at the prompt input column/rightmost column
immediately after a wrap/no-content cell and not surrounded by other styled
cells) before changing ch, and only mutate the exact matching cell (use the same
identifiers ch, cell, cell.AttributeData.IsBold/IsInverse to implement the more
specific match).

Comment thread pkg/wshutil/wshrpc.go
Comment on lines +432 to +438
// EventRecv is handled synchronously to preserve arrival order. Goroutine
// dispatch causes non-deterministic ordering when back-to-back events race
// to acquire the handler mutex, corrupting ordered byte streams (e.g. PTY output).
if msg.Command == wshrpc.Command_EventRecv {
w.handleEventRecv(&msg)
continue
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't run all eventrecv listeners on the main dispatch loop.

handleEventRecv() ends up calling EventListener.RecvEvent(), and that executes listener callbacks synchronously (pkg/wshutil/wshevent.go:62-66). With this change, any slow or blocking listener now stalls runServer() itself. The new controller-status listener in pkg/waveattach/attach.go, for example, sends into exitCh; once that channel fills, unrelated RPC responses and timeouts stop being processed too. Preserve PTY ordering with a dedicated ordered queue or by serializing only the ordered PTY event types instead of every Command_EventRecv.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/wshutil/wshrpc.go` around lines 432 - 438, The main dispatch loop
(runServer) currently calls handleEventRecv synchronously for every
Command_EventRecv, which blocks runServer because handleEventRecv ->
EventListener.RecvEvent executes listener callbacks inline; instead, create a
dedicated ordered event processor (a buffered channel + single goroutine) and
send EventRecv messages to it from runServer so only that processor serializes
arrival order; inside that processor call handleEventRecv sequentially for
events that require ordering (e.g., PTY/ordered stream event types determined
from msg.Event.Type), but for other EventRecv types dispatch to listeners
asynchronously (goroutine per listener or worker pool) to avoid stalling
runServer and exitCh; update runServer to push to the new queue rather than
calling handleEventRecv directly and ensure the controller-status listener
remains non-blocking or uses a buffered exitCh.

@dfbb dfbb changed the title feat(wsh): add attach command for read-only terminal observation feat(wsh): add attach command to observe and interact with running terminal blocks May 1, 2026
dfbb added 15 commits May 17, 2026 12:49
- Switch auth injector to X-Remote-Password in remote mode (emain.ts, emain-tabview.ts)
- Skip dock menu, auto-updater, and global hotkeys in remote mode
- Set window title to "Wave — [remote: host:port]" in remote mode
- Use waveapp-remote-<suffix>.log for log files in remote mode
- Return null from get-data-dir / get-config-dir IPC in remote mode
- Add config key constants for remote:password/listenport/bindaddr
…tion

- attach.go: exclude on Windows (uses syscall.SIGWINCH)
- attach_windows.go: Windows stub returning unsupported error
- auth.go: exclude on linux/mips and linux/mips64 (modernc/libc not supported)
- auth_mips.go: mips stub returning unsupported error
xterm-go MaxBufferSize overflows 32-bit int on linux/mips(64).
sqlite/libc has no support for linux/mips big-endian.
syscall.SIGWINCH is not available on windows.

Replace per-file stubs with a single waveattach_unsupported.go that
provides all public API stubs for windows and linux/mips(64) targets.
All implementation files now share the same build constraint.
… routing

- Bridge tab/window control events via wps.electron:control so remote
  Electron clients sync with native UI changes
- Append per-renderer uuid suffix to tab:/feblock: routes to prevent
  ws stableid collision between local and remote Electron processes
- Add prefix-match fallback in wshrouter.getLinkForRoute for legacy
  callers still addressing bare tab:/feblock: routes
- Self-heal active tab on subscribe to recover from lost startup events
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
emain/emain-log.ts (1)

49-53: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pruning still targets waveapp.*.log, so remote log rotations won’t be cleaned up.

rotateLogIfNeeded now writes ${LogBaseName}.${n}.log, but prune still filters only waveapp.(n).log. In remote mode this can cause unbounded log growth.

Suggested fix
 function pruneOldLogs(logsDir: string): { pruned: string[]; error: any } {
@@
-    for (const file of files) {
-        const match = file.match(/^waveapp\.(\d+)\.log$/);
+    const pattern = new RegExp(`^${LogBaseName}\\.(\\d+)\\.log$`);
+    for (const file of files) {
+        const match = file.match(pattern);
         if (match) {
             logFiles.push({ name: file, num: parseInt(match[1], 10) });
         }
     }
🤖 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 `@emain/emain-log.ts` around lines 49 - 53, Prune logic still only matches
/^waveapp\.(\d+)\.log$/ while rotateLogIfNeeded writes files as
`${LogBaseName}.${n}.log`; update the pruning match to use the actual
LogBaseName rather than hardcoded "waveapp". In the function that builds
logFiles (and any prune-related code), replace the literal regex with a dynamic
one constructed from LogBaseName (escape it if needed) to capture the numeric
suffix (e.g., new RegExp(`^${escapedLogBaseName}\\.(\\d+)\\.log$`)), so rotated
logs created in remote mode are discovered and cleaned up. Ensure references:
rotateLogIfNeeded, LogBaseName, and the prune/logFiles collection code are
updated accordingly.
🧹 Nitpick comments (1)
schema/settings.json (1)

355-360: ⚡ Quick win

Constrain remote config fields in schema to prevent invalid settings.

Add basic validation (minLength for password and port bounds) so invalid values are rejected at config-validation time instead of failing later.

Suggested schema update
         "remote:password": {
-          "type": "string"
+          "type": "string",
+          "minLength": 1
         },
         "remote:listenport": {
-          "type": "integer"
+          "type": "integer",
+          "minimum": 1,
+          "maximum": 65535
         },
🤖 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 `@schema/settings.json` around lines 355 - 360, The schema currently defines
"remote:password" (type string) and "remote:listenport" (type integer) without
validation; update the schema to add basic constraints so invalid values are
rejected: for "remote:password" add a minLength (e.g., 1) and optionally a
maxLength, and for "remote:listenport" add minimum and maximum bounds (e.g.,
minimum 1 and maximum 65535) to enforce valid TCP port ranges; keep the same
property names ("remote:password", "remote:listenport") and types when adding
these validation keys.
🤖 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 `@emain/remotemode.ts`:
- Around line 22-24: The code treats "--remote-host" without a following value
as null; change the parsing so that when a === "--remote-host" you validate
argv[i+1] exists and is not another flag (e.g., doesn't start with '-') and if
it is missing or looks like a flag, throw an error or call process.exit(1) with
a clear message instead of setting value to null; update the matching block(s)
that set value = argv[i + 1] ?? null (including the similar code around lines
31-35) to perform this presence/format check and fail fast when the flag is
provided without a valid host argument.

In `@pkg/waveattach/screen.go`:
- Around line 19-28: renderedCell currently lacks text attribute flags so
attribute-only changes
(bold/italic/dim/blink/inverse/invisible/strikethrough/overline) are ignored and
rendering is skipped; add a bitfield (e.g. attrs uint32 or similar) to the
renderedCell struct, compute the attribute flags in the render loop where cells
are prepared (the place that currently sets
ch/fg/bg/width/underlineStyle/underlineColor), store them into renderedCell, and
update the SGR emission/comparison logic (the block that decides whether to emit
SGR and skips rendering when cells match) to include the new attrs field so
attribute-only changes trigger re-rendering. Ensure the attribute bits cover
bold, italic, dim, blink, inverse, invisible, strikethrough and overline and are
used wherever renderedCell equality is checked.

---

Outside diff comments:
In `@emain/emain-log.ts`:
- Around line 49-53: Prune logic still only matches /^waveapp\.(\d+)\.log$/
while rotateLogIfNeeded writes files as `${LogBaseName}.${n}.log`; update the
pruning match to use the actual LogBaseName rather than hardcoded "waveapp". In
the function that builds logFiles (and any prune-related code), replace the
literal regex with a dynamic one constructed from LogBaseName (escape it if
needed) to capture the numeric suffix (e.g., new
RegExp(`^${escapedLogBaseName}\\.(\\d+)\\.log$`)), so rotated logs created in
remote mode are discovered and cleaned up. Ensure references: rotateLogIfNeeded,
LogBaseName, and the prune/logFiles collection code are updated accordingly.

---

Nitpick comments:
In `@schema/settings.json`:
- Around line 355-360: The schema currently defines "remote:password" (type
string) and "remote:listenport" (type integer) without validation; update the
schema to add basic constraints so invalid values are rejected: for
"remote:password" add a minLength (e.g., 1) and optionally a maxLength, and for
"remote:listenport" add minimum and maximum bounds (e.g., minimum 1 and maximum
65535) to enforce valid TCP port ranges; keep the same property names
("remote:password", "remote:listenport") and types when adding these validation
keys.
🪄 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: CHILL

Plan: Pro

Run ID: 9d3ca5e5-d765-48e8-8c2b-355d24709bb1

📥 Commits

Reviewing files that changed from the base of the PR and between e64d1fe and d36c789.

⛔ Files ignored due to path filters (2)
  • go.sum is excluded by !**/*.sum
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (35)
  • CLAUDE.md
  • cmd/server/main-server.go
  • emain/emain-log.ts
  • emain/emain-platform.ts
  • emain/emain-tabview.ts
  • emain/emain-wavesrv.ts
  • emain/emain-window.ts
  • emain/emain-wsh.ts
  • emain/emain.ts
  • emain/remoteauth.ts
  • emain/remotemode.ts
  • frontend/app/store/ws.ts
  • frontend/app/store/wshrouter.ts
  • frontend/app/store/wshrpcutil-base.ts
  • frontend/types/gotypes.d.ts
  • frontend/types/waveevent.d.ts
  • go.mod
  • pkg/service/workspaceservice/workspaceservice.go
  • pkg/tsgen/tsgenevent.go
  • pkg/waveattach/attach.go
  • pkg/waveattach/auth.go
  • pkg/waveattach/output.go
  • pkg/waveattach/screen.go
  • pkg/waveattach/selector.go
  • pkg/waveattach/waveattach_unsupported.go
  • pkg/wconfig/metaconsts.go
  • pkg/wconfig/settingsconfig.go
  • pkg/wcore/window.go
  • pkg/wcore/workspace.go
  • pkg/web/remoteentry.go
  • pkg/web/remoteentry_test.go
  • pkg/wps/wps.go
  • pkg/wps/wpstypes.go
  • pkg/wshutil/wshrouter.go
  • schema/settings.json
💤 Files with no reviewable changes (1)
  • pkg/wps/wps.go
✅ Files skipped from review due to trivial changes (4)
  • pkg/waveattach/waveattach_unsupported.go
  • frontend/types/gotypes.d.ts
  • CLAUDE.md
  • pkg/wconfig/metaconsts.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • go.mod
  • pkg/waveattach/output.go
  • pkg/waveattach/auth.go
  • pkg/waveattach/selector.go
  • pkg/waveattach/attach.go

Comment thread emain/remotemode.ts
Comment on lines +22 to +24
if (a === "--remote-host") {
value = argv[i + 1] ?? null;
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fail fast when --remote-host is provided without a value.

Right now, --remote-host without a value is treated as “remote mode off”, which hides CLI misconfiguration and starts the wrong mode.

Suggested fix
 function parseRemoteHostArg(argv: string[]): RemoteTarget | null {
     let value: string | null = null;
+    let flagSeen = false;
     for (let i = 0; i < argv.length; i++) {
         const a = argv[i];
         if (a === "--remote-host") {
+            flagSeen = true;
             value = argv[i + 1] ?? null;
             break;
         }
         if (a.startsWith("--remote-host=")) {
+            flagSeen = true;
             value = a.slice("--remote-host=".length);
             break;
         }
     }
-    if (!value) return null;
+    if (!value) {
+        if (flagSeen) {
+            throw new Error("invalid --remote-host value: missing host[:port]");
+        }
+        return null;
+    }

Also applies to: 31-35

🤖 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 `@emain/remotemode.ts` around lines 22 - 24, The code treats "--remote-host"
without a following value as null; change the parsing so that when a ===
"--remote-host" you validate argv[i+1] exists and is not another flag (e.g.,
doesn't start with '-') and if it is missing or looks like a flag, throw an
error or call process.exit(1) with a clear message instead of setting value to
null; update the matching block(s) that set value = argv[i + 1] ?? null
(including the similar code around lines 31-35) to perform this presence/format
check and fail fast when the flag is provided without a valid host argument.

Comment thread pkg/waveattach/screen.go
Comment on lines +19 to +28
// renderedCell tracks what was last drawn at a (row, col) on the local terminal.
// width: 1 normal, 2 wide-start, 0 wide-continuation.
type renderedCell struct {
ch string
fg uint32
bg uint32
width int
underlineStyle xterm.UnderlineStyle
underlineColor uint32
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Text attribute changes (bold/italic/etc.) are not tracked and won't render.

renderedCell only stores ch, fg, bg, width, underlineStyle, underlineColor. It does not track attributes like bold, italic, dim, blink, inverse, invisible, strikethrough, or overline.

When a cell changes from bold to non-bold (or any other attribute-only change) with identical colors, the comparison at line 319 will match and skip rendering, causing visual corruption.

🛠️ Suggested fix: add attribute flags to renderedCell
 type renderedCell struct {
 	ch             string
 	fg             uint32
 	bg             uint32
 	width          int
 	underlineStyle xterm.UnderlineStyle
 	underlineColor uint32
+	attrFlags      uint32 // bold|dim|italic|blink|inverse|invisible|strikethrough|overline bits
 }

Then in the render loop, compute and store the attribute flags:

+			attrFlags := uint32(0)
+			if a.IsBold() != 0 { attrFlags |= 1 << 0 }
+			if a.IsDim() != 0 { attrFlags |= 1 << 1 }
+			if a.IsItalic() != 0 { attrFlags |= 1 << 2 }
+			if a.IsBlink() != 0 { attrFlags |= 1 << 3 }
+			if a.IsInverse() != 0 { attrFlags |= 1 << 4 }
+			if a.IsInvisible() != 0 { attrFlags |= 1 << 5 }
+			if a.IsStrikethrough() != 0 { attrFlags |= 1 << 6 }
+			if a.IsOverline() != 0 { attrFlags |= 1 << 7 }
-			newRC := renderedCell{ch: ch, fg: cell.Fg, bg: cell.Bg, width: cellW, underlineStyle: ulStyle, underlineColor: ulColor}
+			newRC := renderedCell{ch: ch, fg: cell.Fg, bg: cell.Bg, width: cellW, underlineStyle: ulStyle, underlineColor: ulColor, attrFlags: attrFlags}

And update the SGR emission condition accordingly:

-			if cell.Fg != prevFg || cell.Bg != prevBg || ulStyle != prevUlStyle || ulColor != prevUlColor {
+			if cell.Fg != prevFg || cell.Bg != prevBg || ulStyle != prevUlStyle || ulColor != prevUlColor || attrFlags != prevAttrFlags {

Also applies to: 318-327

🤖 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 `@pkg/waveattach/screen.go` around lines 19 - 28, renderedCell currently lacks
text attribute flags so attribute-only changes
(bold/italic/dim/blink/inverse/invisible/strikethrough/overline) are ignored and
rendering is skipped; add a bitfield (e.g. attrs uint32 or similar) to the
renderedCell struct, compute the attribute flags in the render loop where cells
are prepared (the place that currently sets
ch/fg/bg/width/underlineStyle/underlineColor), store them into renderedCell, and
update the SGR emission/comparison logic (the block that decides whether to emit
SGR and skips rendering when cells match) to include the new attrs field so
attribute-only changes trigger re-rendering. Ensure the attribute bits cover
bold, italic, dim, blink, inverse, invisible, strikethrough and overline and are
used wherever renderedCell equality is checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant