feat(wsh): add wsh attach command to connect external terminals to Wave blocks#3255
Open
dfbb wants to merge 18 commits intowavetermdev:mainfrom
Open
feat(wsh): add wsh attach command to connect external terminals to Wave blocks#3255dfbb wants to merge 18 commits intowavetermdev:mainfrom
dfbb wants to merge 18 commits intowavetermdev:mainfrom
Conversation
Spec for a standalone CLI tool that attaches to an existing Wave Terminal block for bidirectional terminal I/O from an external terminal emulator.
- Auth: replace nonexistent authkey file with direct SQLite read of jwtprivatekey from db_mainserver table (Wave uses WAL, allows ro reads) - I/O: WaveFileReadStreamCommand is snapshot-only — switch to combined approach (initial snapshot + WPS "blockfile" event subscription) with ordering rules to avoid missed/duplicated bytes - Term file size corrected to 2MB circular buffer (was 5MB) - Note BlocksListCommand returns only IDs; selector needs separate workspace/tab name lookups - Add WAVETERM_DATA_HOME env var support for data directory discovery
… remove add method
Move waveattach functionality into wsh as `wsh attach`. The standalone cmd/waveattach entrypoint is removed; all logic stays in pkg/waveattach. Localize selector UI strings to English.
|
|
Contributor
WalkthroughA new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/waveattach/attach.go`:
- Around line 47-58: The Ctrl+A handling branch leaves p.gotPrefix true after
writing a literal Ctrl+A, causing the next byte to be misinterpreted as a detach
suffix; modify the case for ctrlA in the switch inside the attach logic so that
after successfully writing the literal ctrlA (the write in the ctrlA branch) you
also set p.gotPrefix = false (just like the default branch does) so the prefix
state is cleared and subsequent bytes are forwarded correctly.
In `@pkg/waveattach/output.go`:
- Around line 94-99: The cutoff timestamp is taken after readSnapshot completes,
which can cause later-written bytes to be buffered with an earlier timestamp and
dropped by buf.flush; move the cutoff capture to before draining the snapshot
stream so the replay cutoff reflects the actual start of draining. Specifically,
capture cutoff := time.Now() immediately before calling readSnapshot (and
likewise in the other occurrence around the second read/call at lines 129-135),
then pass that cutoff into buf.flush (or otherwise ensure buf.flush is called
with the earlier cutoff) so that buf.flush uses the correct replay cutoff for
the data processed by readSnapshot and subsequent io.Copy operations.
In `@pkg/waveattach/selector.go`:
- Around line 148-172: The input loop in the selector (reading from os.Stdin
into buf and switching on n/b) fails when ESC [ A/B arrives across multiple
Reads; add a tiny escape-state buffer/state machine in the same function
surrounding the loop (e.g., escBuf or escState) that accumulates bytes across
reads and matches the sequence ESC (27) then '[' then 'A'/'B' before acting on
Up/Down; when a complete sequence is recognized, update cur like the existing
cases, clear escBuf/state, and on any non-escape bytes fall back to the existing
single-byte handling (q, Ctrl-C, Enter -> entries[cur].BlockId + clear()/error
return). Ensure the state resets on invalid sequences and that normal
single-byte behavior (n==1 checks for 'q', 3, 13) still works when no escape
prefixed bytes are pending.
🪄 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: afe47e64-3898-4c23-9900-f308285c5e13
📒 Files selected for processing (8)
cmd/wsh/cmd/wshcmd-attach.gopkg/waveattach/attach.gopkg/waveattach/attach_test.gopkg/waveattach/auth.gopkg/waveattach/auth_test.gopkg/waveattach/output.gopkg/waveattach/output_test.gopkg/waveattach/selector.go
Comment on lines
+47
to
+58
| switch c { | ||
| case 'd', 'D': | ||
| return true, nil | ||
| case ctrlA: | ||
| if _, err := w.Write([]byte{ctrlA}); err != nil { | ||
| return false, err | ||
| } | ||
| default: | ||
| p.gotPrefix = false | ||
| if _, err := w.Write([]byte{ctrlA, c}); err != nil { | ||
| return false, err | ||
| } |
Contributor
There was a problem hiding this comment.
Reset the prefix state after Ctrl+A Ctrl+A.
The doubled-Ctrl+A branch writes a literal Ctrl+A but leaves gotPrefix set. The next byte is still interpreted as a detach suffix, so sequences like Ctrl+A Ctrl+A d detach instead of forwarding ^Ad, and Ctrl+A Ctrl+A x emits an extra Ctrl+A.
💡 Suggested fix
switch c {
case 'd', 'D':
return true, nil
case ctrlA:
+ p.gotPrefix = false
if _, err := w.Write([]byte{ctrlA}); err != nil {
return false, err
}
default:
p.gotPrefix = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/waveattach/attach.go` around lines 47 - 58, The Ctrl+A handling branch
leaves p.gotPrefix true after writing a literal Ctrl+A, causing the next byte to
be misinterpreted as a detach suffix; modify the case for ctrlA in the switch
inside the attach logic so that after successfully writing the literal ctrlA
(the write in the ctrlA branch) you also set p.gotPrefix = false (just like the
default branch does) so the prefix state is cleared and subsequent bytes are
forwarded correctly.
Comment on lines
+94
to
+99
| if err := readSnapshot(rpcClient, blockId, w); err != nil { | ||
| return fmt.Errorf("reading snapshot: %w", err) | ||
| } | ||
| cutoff := time.Now() | ||
| if err := buf.flush(cutoff, w); err != nil { | ||
| return err |
Contributor
There was a problem hiding this comment.
Capture the replay cutoff before draining the snapshot.
cutoff := time.Now() is taken only after readSnapshot finishes copying the whole stream. Any output appended after the server snapshots term but before io.Copy completes is buffered with an earlier timestamp and then dropped by flush, so attach can lose terminal bytes under load. Cross-file evidence: pkg/wshrpc/wshserver/wshserver.go:1035-1075 reads the file before it starts the async stream writer.
💡 Suggested fix
func StreamOutput(ctx context.Context, rpcClient *wshutil.WshRpc, blockId string, w io.Writer) error {
buf := makeEventBuffer()
blockRef := waveobj.MakeORef(waveobj.OType_Block, blockId).String()
@@
- if err := readSnapshot(rpcClient, blockId, w); err != nil {
+ cutoff, err := readSnapshot(rpcClient, blockId, w)
+ if err != nil {
return fmt.Errorf("reading snapshot: %w", err)
}
- cutoff := time.Now()
if err := buf.flush(cutoff, w); err != nil {
return err
}
@@
-func readSnapshot(rpcClient *wshutil.WshRpc, blockId string, w io.Writer) error {
+func readSnapshot(rpcClient *wshutil.WshRpc, blockId string, w io.Writer) (time.Time, error) {
broker := rpcClient.StreamBroker
if broker == nil {
- return fmt.Errorf("stream broker not available")
+ return time.Time{}, fmt.Errorf("stream broker not available")
}
@@
readerRouteId, err := wshclient.ControlGetRouteIdCommand(rpcClient, &wshrpc.RpcOpts{Route: wshutil.ControlRoute})
if err != nil {
- return fmt.Errorf("getting route id: %w", err)
+ return time.Time{}, fmt.Errorf("getting route id: %w", err)
}
if readerRouteId == "" {
- return fmt.Errorf("no route to receive data")
+ return time.Time{}, fmt.Errorf("no route to receive data")
}
@@
_, err = wshclient.WaveFileReadStreamCommand(rpcClient, data, nil)
if err != nil {
- return fmt.Errorf("starting stream read: %w", err)
+ return time.Time{}, fmt.Errorf("starting stream read: %w", err)
}
+ cutoff := time.Now()
_, err = io.Copy(w, reader)
if err != nil {
- return fmt.Errorf("reading stream: %w", err)
+ return time.Time{}, fmt.Errorf("reading stream: %w", err)
}
- return nil
+ return cutoff, nil
}Also applies to: 129-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/waveattach/output.go` around lines 94 - 99, The cutoff timestamp is taken
after readSnapshot completes, which can cause later-written bytes to be buffered
with an earlier timestamp and dropped by buf.flush; move the cutoff capture to
before draining the snapshot stream so the replay cutoff reflects the actual
start of draining. Specifically, capture cutoff := time.Now() immediately before
calling readSnapshot (and likewise in the other occurrence around the second
read/call at lines 129-135), then pass that cutoff into buf.flush (or otherwise
ensure buf.flush is called with the earlier cutoff) so that buf.flush uses the
correct replay cutoff for the data processed by readSnapshot and subsequent
io.Copy operations.
Comment on lines
+148
to
+172
| buf := make([]byte, 4) | ||
| for { | ||
| n, err := os.Stdin.Read(buf) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| b := buf[:n] | ||
|
|
||
| switch { | ||
| case n == 1 && (b[0] == 'q' || b[0] == 3): | ||
| clear() | ||
| return "", fmt.Errorf("cancelled") | ||
| case n == 1 && b[0] == 13: | ||
| selected := entries[cur].BlockId | ||
| clear() | ||
| return selected, nil | ||
| case n == 3 && b[0] == 27 && b[1] == '[' && b[2] == 'A': | ||
| if cur > 0 { | ||
| cur-- | ||
| } | ||
| case n == 3 && b[0] == 27 && b[1] == '[' && b[2] == 'B': | ||
| if cur < len(entries)-1 { | ||
| cur++ | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle arrow-key escape sequences across reads.
This parser only recognizes Up/Down when the entire ESC [ A/B sequence lands in one Read. In raw mode those bytes can arrive split, so selector navigation becomes unreliable on some terminals. A tiny escape-sequence buffer/state machine here would make the UI deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/waveattach/selector.go` around lines 148 - 172, The input loop in the
selector (reading from os.Stdin into buf and switching on n/b) fails when ESC [
A/B arrives across multiple Reads; add a tiny escape-state buffer/state machine
in the same function surrounding the loop (e.g., escBuf or escState) that
accumulates bytes across reads and matches the sequence ESC (27) then '[' then
'A'/'B' before acting on Up/Down; when a complete sequence is recognized, update
cur like the existing cases, clear escBuf/state, and on any non-escape bytes
fall back to the existing single-byte handling (q, Ctrl-C, Enter ->
entries[cur].BlockId + clear()/error return). Ensure the state resets on invalid
sequences and that normal single-byte behavior (n==1 checks for 'q', 3, 13)
still works when no escape prefixed bytes are pending.
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
2 participants
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
Adds
wsh attach— a new subcommand that lets users attach an external terminal (iTerm, Terminal.app, etc.) to a runningtermblock in Wave Terminal, providing bidirectional I/O.blockfileevents first, then reads the term file snapshot, then flushes buffered events after the snapshot cutoff to avoid missing bytesControllerInputCommand;Ctrl+A Ddetaches without killing the shellSIGWINCHand sends updatedTermSizeto the block; restores the block's original term size on detachtermblocks in the active workspace and auto-selects when only one exists; shows an interactive arrow-key UI when multiple existEvent_ControllerStatusand exits cleanly when the block's shell process finishesKnown Limitations
ControllerInputCommandreturnsno controller found. Only blocks in the currently active workspace accept input.Test Plan
wsh attach --helpprints usagewsh attachauto-attachesstty sizeinside the block reflects the change)Ctrl+A Ddetaches cleanly; the shell inside Wave keeps runningCtrl+Cis forwarded to the block (interrupts a running process)[block closed]and exitswsh attach <blockid>skips the selector