From fcc12e4a5a4e56fa922795389d4f7439dd846e01 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 11:30:49 +0000 Subject: [PATCH 1/4] fix(screentracker): stop writeStabilize from fatally timing out when agents don't echo input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of writeStabilize waited 15s for the screen to change after writing message text (echo detection), returning HTTP 500 if it didn't. Many TUI agents using bracketed paste don't echo input until Enter is pressed, causing every message send to fail. Phase 1 timeout is now non-fatal (2s) — if the screen doesn't change, we log at Info level and proceed to Phase 2 (send carriage return). Phase 2 remains the real indicator of agent responsiveness. Key changes: - Guard non-fatal path with errors.Is(err, util.WaitTimedOut) so context cancellation still propagates as a fatal error - Reduce Phase 1 timeout from 15s to 2s (echo is near-instant) - Extract named constants for both timeouts - Add tests for no-echo-success and no-echo-no-react-failure - Add documentation test for TUI selection prompt ESC limitation Closes coder/agentapi#123 --- lib/screentracker/pty_conversation.go | 50 +++++++- lib/screentracker/pty_conversation_test.go | 133 +++++++++++++++++++++ 2 files changed, 179 insertions(+), 4 deletions(-) diff --git a/lib/screentracker/pty_conversation.go b/lib/screentracker/pty_conversation.go index 0e309fec..98d11d8c 100644 --- a/lib/screentracker/pty_conversation.go +++ b/lib/screentracker/pty_conversation.go @@ -3,6 +3,7 @@ package screentracker import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "os" @@ -16,6 +17,24 @@ import ( "golang.org/x/xerrors" ) +const ( + // writeStabilizeEchoTimeout is the maximum time to wait for + // the screen to change after writing message text to the PTY + // (echo detection). This is intentionally short: terminal + // echo is near-instant when it occurs. Non-echoing agents + // (e.g. TUI agents using bracketed paste) will hit this + // timeout, which is non-fatal — see the Phase 1 error + // handler in writeStabilize. Move to PTYConversationConfig + // if agents need different echo detection windows. + writeStabilizeEchoTimeout = 2 * time.Second + + // writeStabilizeProcessTimeout is the maximum time to wait + // for the screen to change after sending a carriage return. + // This detects whether the agent is actually processing the + // input. + writeStabilizeProcessTimeout = 15 * time.Second +) + // A screenSnapshot represents a snapshot of the PTY at a specific time. type screenSnapshot struct { timestamp time.Time @@ -411,7 +430,19 @@ func (c *PTYConversation) sendMessage(ctx context.Context, messageParts ...Messa return nil } -// writeStabilize writes messageParts to the screen and waits for the screen to stabilize after the message is written. +// writeStabilize writes messageParts to the PTY and waits for +// the agent to process them. It operates in two phases: +// +// Phase 1 (echo detection): writes the message text and waits +// for the screen to change and stabilize. This detects agents +// that echo typed input. If the screen doesn't change within +// writeStabilizeEchoTimeout, this is non-fatal — many TUI +// agents buffer bracketed-paste input without rendering it. +// +// Phase 2 (processing detection): writes a carriage return +// and waits for the screen to change, indicating the agent +// started processing. This phase is fatal on timeout — if the +// agent doesn't react to Enter, it's unresponsive. func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...MessagePart) error { screenBeforeMessage := c.cfg.AgentIO.ReadScreen() for _, part := range messageParts { @@ -421,7 +452,7 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me } // wait for the screen to stabilize after the message is written if err := util.WaitFor(ctx, util.WaitTimeout{ - Timeout: 15 * time.Second, + Timeout: writeStabilizeEchoTimeout, MinInterval: 50 * time.Millisecond, InitialWait: true, Clock: c.cfg.Clock, @@ -438,14 +469,25 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me } return false, nil }); err != nil { - return xerrors.Errorf("failed to wait for screen to stabilize: %w", err) + if !errors.Is(err, util.WaitTimedOut) { + // Context cancellation or condition errors are fatal. + return xerrors.Errorf("failed to wait for screen to stabilize: %w", err) + } + // Phase 1 timeout is non-fatal: the agent may not echo + // input (e.g. TUI agents buffer bracketed-paste content + // internally). Proceed to Phase 2 to send the carriage + // return. + c.cfg.Logger.Info( + "screen did not stabilize after writing message, proceeding to send carriage return", + "timeout", writeStabilizeEchoTimeout, + ) } // wait for the screen to change after the carriage return is written screenBeforeCarriageReturn := c.cfg.AgentIO.ReadScreen() lastCarriageReturnTime := time.Time{} if err := util.WaitFor(ctx, util.WaitTimeout{ - Timeout: 15 * time.Second, + Timeout: writeStabilizeProcessTimeout, MinInterval: 25 * time.Millisecond, Clock: c.cfg.Clock, }, func() (bool, error) { diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index 0e7d9635..f5941f02 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -1,6 +1,7 @@ package screentracker_test import ( + "bytes" "context" "encoding/json" "fmt" @@ -8,6 +9,7 @@ import ( "log/slog" "os" "sync" + "sync/atomic" "testing" "time" @@ -447,6 +449,137 @@ func TestMessages(t *testing.T) { c, _, _ := newConversation(context.Background(), t) assert.ErrorIs(t, c.Send(st.MessagePartText{Content: ""}), st.ErrMessageValidationEmpty) }) + + t.Run("send-no-echo-agent-reacts", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + agent := &testAgent{screen: "prompt"} + // Agent doesn't echo input text, but reacts to carriage return. + agent.onWrite = func(data []byte) { + if string(data) == "\r" { + agent.screen = "processing..." + } + } + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + + // Stabilize: fill snapshot buffer so status becomes stable. + advanceFor(ctx, t, mClock, interval*threshold) + + // Send and advance. Phase 1 times out (2s, non-fatal), + // Phase 2 writes \r → onWrite changes screen → succeeds. + sendAndAdvance(ctx, t, c, mClock, st.MessagePartText{Content: "hello"}) + + // User message was recorded. + msgs := c.Messages() + require.True(t, len(msgs) >= 2) + assert.Equal(t, st.ConversationRoleUser, msgs[len(msgs)-1].Role) + assert.Equal(t, "hello", msgs[len(msgs)-1].Message) + }) + + t.Run("send-no-echo-no-react", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + agent := &testAgent{screen: "prompt"} + // Agent is completely unresponsive: no echo, no reaction. + agent.onWrite = func(data []byte) {} + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + + // Stabilize. + advanceFor(ctx, t, mClock, interval*threshold) + + // Send in a goroutine; can't use sendAndAdvance because it + // calls require.NoError internally. + var sendErr error + var sendDone atomic.Bool + go func() { + sendErr = c.Send(st.MessagePartText{Content: "hello"}) + sendDone.Store(true) + }() + advanceUntil(ctx, t, mClock, func() bool { return sendDone.Load() }) + + require.Error(t, sendErr) + assert.Contains(t, sendErr.Error(), "failed to wait for processing to start") + }) + + t.Run("send-tui-selection-esc-cancels", func(t *testing.T) { + // Documents a known limitation: when a TUI agent shows a + // selection prompt, sending a user message wraps it in + // bracketed paste. The ESC (\x1b) in the paste-start + // sequence cancels the selection widget. The user's intended + // choice never reaches the selection handler. + // + // For selection prompts, callers should use MessageTypeRaw + // to send raw keystrokes directly to the PTY. + // + // See lib/httpapi/claude.go for the full formatClaudeCodeMessage + // format; this test focuses on the ESC invariant only. + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + agent := &testAgent{screen: "selection prompt"} + selectionCancelled := false + agent.onWrite = func(data []byte) { + // Simulate TUI selection widget: ESC cancels the + // selection, changing the screen. + if bytes.Contains(data, []byte("\x1b")) { + selectionCancelled = true + agent.screen = "selection cancelled" + } else if string(data) == "\r" { + agent.screen = "post-cancel" + } + } + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + + // Stabilize. + advanceFor(ctx, t, mClock, interval*threshold) + + // Send using bracketed paste, which contains ESC. The + // test focuses on the ESC invariant — any input wrapped + // in bracketed paste will trigger this. + sendAndAdvance(ctx, t, c, mClock, + st.MessagePartText{Content: "\x1b[200~", Hidden: true}, + st.MessagePartText{Content: "2"}, + st.MessagePartText{Content: "\x1b[201~", Hidden: true}, + ) + + // The send succeeded, but the selection was cancelled by + // ESC — not option "2" being selected. + assert.True(t, selectionCancelled, + "ESC in bracketed paste cancels TUI selection prompts; "+ + "use MessageTypeRaw for selection prompts instead") + }) } func TestStatePersistence(t *testing.T) { From 26d03e957a34a08ef5bd77fe76309ef33fcb12fa Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 11:45:16 +0000 Subject: [PATCH 2/4] style(screentracker): use Given/When/Then comments in writeStabilize tests Restructure test comments to follow Cucumber-style Given/When/Then pattern for clarity. Also fix send-no-echo-agent-reacts assertion to scan for the user message instead of assuming it's the last message in the conversation (the snapshot loop may append an agent message after Send returns). --- lib/screentracker/pty_conversation_test.go | 256 +++++++++++---------- 1 file changed, 129 insertions(+), 127 deletions(-) diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index f5941f02..541299fa 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -450,137 +450,139 @@ func TestMessages(t *testing.T) { assert.ErrorIs(t, c.Send(st.MessagePartText{Content: ""}), st.ErrMessageValidationEmpty) }) - t.Run("send-no-echo-agent-reacts", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) - - agent := &testAgent{screen: "prompt"} - // Agent doesn't echo input text, but reacts to carriage return. - agent.onWrite = func(data []byte) { - if string(data) == "\r" { - agent.screen = "processing..." + t.Run("send-no-echo-agent-reacts", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + // Given: an agent that doesn't echo typed input but + // reacts to carriage return by updating the screen. + agent := &testAgent{screen: "prompt"} + agent.onWrite = func(data []byte) { + if string(data) == "\r" { + agent.screen = "processing..." + } } - } - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) - - // Stabilize: fill snapshot buffer so status becomes stable. - advanceFor(ctx, t, mClock, interval*threshold) - - // Send and advance. Phase 1 times out (2s, non-fatal), - // Phase 2 writes \r → onWrite changes screen → succeeds. - sendAndAdvance(ctx, t, c, mClock, st.MessagePartText{Content: "hello"}) - - // User message was recorded. - msgs := c.Messages() - require.True(t, len(msgs) >= 2) - assert.Equal(t, st.ConversationRoleUser, msgs[len(msgs)-1].Role) - assert.Equal(t, "hello", msgs[len(msgs)-1].Message) - }) - - t.Run("send-no-echo-no-react", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) - - agent := &testAgent{screen: "prompt"} - // Agent is completely unresponsive: no echo, no reaction. - agent.onWrite = func(data []byte) {} - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) - - // Stabilize. - advanceFor(ctx, t, mClock, interval*threshold) - - // Send in a goroutine; can't use sendAndAdvance because it - // calls require.NoError internally. - var sendErr error - var sendDone atomic.Bool - go func() { - sendErr = c.Send(st.MessagePartText{Content: "hello"}) - sendDone.Store(true) - }() - advanceUntil(ctx, t, mClock, func() bool { return sendDone.Load() }) - - require.Error(t, sendErr) - assert.Contains(t, sendErr.Error(), "failed to wait for processing to start") - }) - - t.Run("send-tui-selection-esc-cancels", func(t *testing.T) { - // Documents a known limitation: when a TUI agent shows a - // selection prompt, sending a user message wraps it in - // bracketed paste. The ESC (\x1b) in the paste-start - // sequence cancels the selection widget. The user's intended - // choice never reaches the selection handler. - // - // For selection prompts, callers should use MessageTypeRaw - // to send raw keystrokes directly to the PTY. - // - // See lib/httpapi/claude.go for the full formatClaudeCodeMessage - // format; this test focuses on the ESC invariant only. - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) - - agent := &testAgent{screen: "selection prompt"} - selectionCancelled := false - agent.onWrite = func(data []byte) { - // Simulate TUI selection widget: ESC cancels the - // selection, changing the screen. - if bytes.Contains(data, []byte("\x1b")) { - selectionCancelled = true - agent.screen = "selection cancelled" - } else if string(data) == "\r" { - agent.screen = "post-cancel" + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), } - } - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + advanceFor(ctx, t, mClock, interval*threshold) - // Stabilize. - advanceFor(ctx, t, mClock, interval*threshold) + // When: a message is sent. Phase 1 times out (no echo), + // Phase 2 writes \r and the agent reacts. + sendAndAdvance(ctx, t, c, mClock, st.MessagePartText{Content: "hello"}) - // Send using bracketed paste, which contains ESC. The - // test focuses on the ESC invariant — any input wrapped - // in bracketed paste will trigger this. - sendAndAdvance(ctx, t, c, mClock, - st.MessagePartText{Content: "\x1b[200~", Hidden: true}, - st.MessagePartText{Content: "2"}, - st.MessagePartText{Content: "\x1b[201~", Hidden: true}, - ) - - // The send succeeded, but the selection was cancelled by - // ESC — not option "2" being selected. - assert.True(t, selectionCancelled, - "ESC in bracketed paste cancels TUI selection prompts; "+ - "use MessageTypeRaw for selection prompts instead") - }) -} + // Then: Send succeeds and the user message is recorded. + msgs := c.Messages() + require.True(t, len(msgs) >= 2) + var foundUserMsg bool + for _, msg := range msgs { + if msg.Role == st.ConversationRoleUser && msg.Message == "hello" { + foundUserMsg = true + break + } + } + assert.True(t, foundUserMsg, "expected user message 'hello' in conversation") + }) + t.Run("send-no-echo-no-react", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + // Given: an agent that is completely unresponsive — it + // neither echoes input nor reacts to carriage return. + agent := &testAgent{screen: "prompt"} + agent.onWrite = func(data []byte) {} + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + advanceFor(ctx, t, mClock, interval*threshold) + + // When: a message is sent. Both Phase 1 (echo) and + // Phase 2 (processing) time out. + // Note: can't use sendAndAdvance here because it calls + // require.NoError internally. + var sendErr error + var sendDone atomic.Bool + go func() { + sendErr = c.Send(st.MessagePartText{Content: "hello"}) + sendDone.Store(true) + }() + advanceUntil(ctx, t, mClock, func() bool { return sendDone.Load() }) + + // Then: Send fails with a Phase 2 error (not Phase 1). + require.Error(t, sendErr) + assert.Contains(t, sendErr.Error(), "failed to wait for processing to start") + }) + t.Run("send-tui-selection-esc-cancels", func(t *testing.T) { + // Documents a known limitation: when a TUI agent shows a + // selection prompt, sending a user message wraps it in + // bracketed paste. The ESC (\x1b) in the paste-start + // sequence cancels the selection widget. The user's + // intended choice never reaches the selection handler. + // For selection prompts, callers should use + // MessageTypeRaw to send raw keystrokes directly. + // + // See lib/httpapi/claude.go formatClaudeCodeMessage for + // the full format; this test focuses on the ESC + // invariant only. + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + // Given: a TUI agent showing a selection prompt where + // ESC cancels the selection and changes the screen. + agent := &testAgent{screen: "selection prompt"} + selectionCancelled := false + agent.onWrite = func(data []byte) { + if bytes.Contains(data, []byte("\x1b")) { + selectionCancelled = true + agent.screen = "selection cancelled" + } else if string(data) == "\r" { + agent.screen = "post-cancel" + } + } + mClock := quartz.NewMock(t) + mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) + cfg := st.PTYConversationConfig{ + Clock: mClock, + AgentIO: agent, + SnapshotInterval: interval, + ScreenStabilityLength: 200 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + advanceFor(ctx, t, mClock, interval*threshold) + + // When: a message is sent using bracketed paste, which + // contains ESC in the start sequence (\x1b[200~). + sendAndAdvance(ctx, t, c, mClock, + st.MessagePartText{Content: "\x1b[200~", Hidden: true}, + st.MessagePartText{Content: "2"}, + st.MessagePartText{Content: "\x1b[201~", Hidden: true}, + ) + + // Then: Send succeeds, but the selection was cancelled + // by ESC — option "2" was never delivered to the + // selection handler. + assert.True(t, selectionCancelled, + "ESC in bracketed paste cancels TUI selection prompts; "+ + "use MessageTypeRaw for selection prompts instead") + })} func TestStatePersistence(t *testing.T) { t.Run("SaveState creates file with correct structure", func(t *testing.T) { From a6101487824200446b91f22a271ccf51467e47ad Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 13:59:49 +0000 Subject: [PATCH 3/4] fix(screentracker): address PR review comments from mafredri - Add send-message-no-echo-context-cancelled test: verifies the errors.Is(WaitTimedOut) guard by cancelling context during Phase 1 and asserting context.Canceled propagates (P2) - Fix gofmt: correct indentation, proper brace placement (P2) - Fix constant comment: describe WaitFor timeout semantics accurately, note 1s stability check can extend past timeout, add TODO tag (P3) - Drop send-tui-selection-esc-cancels test from screentracker, add ESC limitation comment to formatPaste in claude.go instead (P3) - Shorten log message to match codebase style (Note) - Rename tests to send-message-* prefix, use newConversation helper with opts callbacks (Note) --- lib/httpapi/claude.go | 5 + lib/screentracker/pty_conversation.go | 20 +- lib/screentracker/pty_conversation_test.go | 216 +++++++++------------ 3 files changed, 107 insertions(+), 134 deletions(-) diff --git a/lib/httpapi/claude.go b/lib/httpapi/claude.go index 641efe47..9ccefdd2 100644 --- a/lib/httpapi/claude.go +++ b/lib/httpapi/claude.go @@ -5,6 +5,11 @@ import ( st "github.com/coder/agentapi/lib/screentracker" ) +// formatPaste wraps message in bracketed paste escape sequences. +// These sequences start with ESC (\x1b), which TUI selection +// widgets (e.g. Claude Code's numbered-choice prompt) interpret +// as "cancel". For selection prompts, callers should use +// MessageTypeRaw to send raw keystrokes directly instead. func formatPaste(message string) []st.MessagePart { return []st.MessagePart{ // Bracketed paste mode start sequence diff --git a/lib/screentracker/pty_conversation.go b/lib/screentracker/pty_conversation.go index 98d11d8c..44e61a21 100644 --- a/lib/screentracker/pty_conversation.go +++ b/lib/screentracker/pty_conversation.go @@ -18,14 +18,16 @@ import ( ) const ( - // writeStabilizeEchoTimeout is the maximum time to wait for - // the screen to change after writing message text to the PTY - // (echo detection). This is intentionally short: terminal - // echo is near-instant when it occurs. Non-echoing agents - // (e.g. TUI agents using bracketed paste) will hit this - // timeout, which is non-fatal — see the Phase 1 error - // handler in writeStabilize. Move to PTYConversationConfig - // if agents need different echo detection windows. + // writeStabilizeEchoTimeout is the timeout for the echo + // detection WaitFor loop in writeStabilize Phase 1. The + // effective ceiling may be slightly longer because the 1s + // stability check inside the condition runs outside + // WaitFor's timeout select. Non-echoing agents (e.g. TUI + // agents using bracketed paste) will hit this timeout, + // which is non-fatal. + // + // TODO: move to PTYConversationConfig if agents need + // different echo detection windows. writeStabilizeEchoTimeout = 2 * time.Second // writeStabilizeProcessTimeout is the maximum time to wait @@ -478,7 +480,7 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me // internally). Proceed to Phase 2 to send the carriage // return. c.cfg.Logger.Info( - "screen did not stabilize after writing message, proceeding to send carriage return", + "echo detection timed out, sending carriage return", "timeout", writeStabilizeEchoTimeout, ) } diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index 541299fa..1db6f06d 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -1,7 +1,6 @@ package screentracker_test import ( - "bytes" "context" "encoding/json" "fmt" @@ -450,139 +449,106 @@ func TestMessages(t *testing.T) { assert.ErrorIs(t, c.Send(st.MessagePartText{Content: ""}), st.ErrMessageValidationEmpty) }) - t.Run("send-no-echo-agent-reacts", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) + t.Run("send-message-no-echo-agent-reacts", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) - // Given: an agent that doesn't echo typed input but - // reacts to carriage return by updating the screen. - agent := &testAgent{screen: "prompt"} - agent.onWrite = func(data []byte) { + // Given: an agent that doesn't echo typed input but + // reacts to carriage return by updating the screen. + c, _, mClock := newConversation(ctx, t, func(cfg *st.PTYConversationConfig) { + a := &testAgent{screen: "prompt"} + a.onWrite = func(data []byte) { if string(data) == "\r" { - agent.screen = "processing..." + a.screen = "processing..." } } - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) - advanceFor(ctx, t, mClock, interval*threshold) + cfg.AgentIO = a + }) + advanceFor(ctx, t, mClock, interval*threshold) - // When: a message is sent. Phase 1 times out (no echo), - // Phase 2 writes \r and the agent reacts. - sendAndAdvance(ctx, t, c, mClock, st.MessagePartText{Content: "hello"}) + // When: a message is sent. Phase 1 times out (no echo), + // Phase 2 writes \r and the agent reacts. + sendAndAdvance(ctx, t, c, mClock, st.MessagePartText{Content: "hello"}) - // Then: Send succeeds and the user message is recorded. - msgs := c.Messages() - require.True(t, len(msgs) >= 2) - var foundUserMsg bool - for _, msg := range msgs { - if msg.Role == st.ConversationRoleUser && msg.Message == "hello" { - foundUserMsg = true - break - } + // Then: Send succeeds and the user message is recorded. + msgs := c.Messages() + require.True(t, len(msgs) >= 2) + var foundUserMsg bool + for _, msg := range msgs { + if msg.Role == st.ConversationRoleUser && msg.Message == "hello" { + foundUserMsg = true + break } - assert.True(t, foundUserMsg, "expected user message 'hello' in conversation") + } + assert.True(t, foundUserMsg, "expected user message 'hello' in conversation") + }) + + t.Run("send-message-no-echo-no-react", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + // Given: an agent that is completely unresponsive — it + // neither echoes input nor reacts to carriage return. + c, _, mClock := newConversation(ctx, t, func(cfg *st.PTYConversationConfig) { + a := &testAgent{screen: "prompt"} + a.onWrite = func(data []byte) {} + cfg.AgentIO = a }) - t.Run("send-no-echo-no-react", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) - - // Given: an agent that is completely unresponsive — it - // neither echoes input nor reacts to carriage return. - agent := &testAgent{screen: "prompt"} - agent.onWrite = func(data []byte) {} - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) - advanceFor(ctx, t, mClock, interval*threshold) - - // When: a message is sent. Both Phase 1 (echo) and - // Phase 2 (processing) time out. - // Note: can't use sendAndAdvance here because it calls - // require.NoError internally. - var sendErr error - var sendDone atomic.Bool - go func() { - sendErr = c.Send(st.MessagePartText{Content: "hello"}) - sendDone.Store(true) - }() - advanceUntil(ctx, t, mClock, func() bool { return sendDone.Load() }) - - // Then: Send fails with a Phase 2 error (not Phase 1). - require.Error(t, sendErr) - assert.Contains(t, sendErr.Error(), "failed to wait for processing to start") + advanceFor(ctx, t, mClock, interval*threshold) + + // When: a message is sent. Both Phase 1 (echo) and + // Phase 2 (processing) time out. + // Note: can't use sendAndAdvance here because it calls + // require.NoError internally. + var sendErr error + var sendDone atomic.Bool + go func() { + sendErr = c.Send(st.MessagePartText{Content: "hello"}) + sendDone.Store(true) + }() + advanceUntil(ctx, t, mClock, func() bool { return sendDone.Load() }) + + // Then: Send fails with a Phase 2 error (not Phase 1). + require.Error(t, sendErr) + assert.Contains(t, sendErr.Error(), "failed to wait for processing to start") + }) + + t.Run("send-message-no-echo-context-cancelled", func(t *testing.T) { + // Given: a non-echoing agent and a cancellable context. + sendCtx, sendCancel := context.WithCancel(context.Background()) + t.Cleanup(sendCancel) + + c, _, mClock := newConversation(sendCtx, t, func(cfg *st.PTYConversationConfig) { + a := &testAgent{screen: "prompt"} + a.onWrite = func(data []byte) {} + cfg.AgentIO = a }) - t.Run("send-tui-selection-esc-cancels", func(t *testing.T) { - // Documents a known limitation: when a TUI agent shows a - // selection prompt, sending a user message wraps it in - // bracketed paste. The ESC (\x1b) in the paste-start - // sequence cancels the selection widget. The user's - // intended choice never reaches the selection handler. - // For selection prompts, callers should use - // MessageTypeRaw to send raw keystrokes directly. - // - // See lib/httpapi/claude.go formatClaudeCodeMessage for - // the full format; this test focuses on the ESC - // invariant only. - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - t.Cleanup(cancel) - - // Given: a TUI agent showing a selection prompt where - // ESC cancels the selection and changes the screen. - agent := &testAgent{screen: "selection prompt"} - selectionCancelled := false - agent.onWrite = func(data []byte) { - if bytes.Contains(data, []byte("\x1b")) { - selectionCancelled = true - agent.screen = "selection cancelled" - } else if string(data) == "\r" { - agent.screen = "post-cancel" - } - } - mClock := quartz.NewMock(t) - mClock.Set(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - cfg := st.PTYConversationConfig{ - Clock: mClock, - AgentIO: agent, - SnapshotInterval: interval, - ScreenStabilityLength: 200 * time.Millisecond, - Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), - } - c := st.NewPTY(ctx, cfg, &testEmitter{}) - c.Start(ctx) - advanceFor(ctx, t, mClock, interval*threshold) - - // When: a message is sent using bracketed paste, which - // contains ESC in the start sequence (\x1b[200~). - sendAndAdvance(ctx, t, c, mClock, - st.MessagePartText{Content: "\x1b[200~", Hidden: true}, - st.MessagePartText{Content: "2"}, - st.MessagePartText{Content: "\x1b[201~", Hidden: true}, - ) - - // Then: Send succeeds, but the selection was cancelled - // by ESC — option "2" was never delivered to the - // selection handler. - assert.True(t, selectionCancelled, - "ESC in bracketed paste cancels TUI selection prompts; "+ - "use MessageTypeRaw for selection prompts instead") - })} + advanceFor(sendCtx, t, mClock, interval*threshold) + + // When: a message is sent and the context is cancelled + // during Phase 1 (before echo detection completes). + var sendErr error + var sendDone atomic.Bool + go func() { + sendErr = c.Send(st.MessagePartText{Content: "hello"}) + sendDone.Store(true) + }() + + // Advance past the snapshot interval so the send loop + // picks up the message, then cancel the context. + advanceFor(sendCtx, t, mClock, interval) + sendCancel() + + // Wait for Send to complete (it should fail quickly + // after cancellation). + require.Eventually(t, sendDone.Load, 5*time.Second, 10*time.Millisecond) + + // Then: the error wraps context.Canceled, not a Phase 2 error. + require.Error(t, sendErr) + assert.ErrorIs(t, sendErr, context.Canceled) + assert.NotContains(t, sendErr.Error(), "failed to wait for processing to start") + }) +} func TestStatePersistence(t *testing.T) { t.Run("SaveState creates file with correct structure", func(t *testing.T) { From f6f14b839b505e7b1edfca815cd8e98bfbef49be Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 14:11:10 +0000 Subject: [PATCH 4/4] fix(screentracker): fix context-cancellation test race with mock clock The test had a race: advanceFor could complete before the Send() goroutine enqueued, so the stableSignal never fired, and sendCancel ran while the message was still queued (never reaching writeStabilize). Fix: use onWrite callback as a synchronization point. advanceUntil waits for writeStabilize to start writing (onWrite fires), then cancels. This guarantees Phase 1 WaitFor is running when ctx is cancelled, and its sleep select sees ctx.Done() immediately. --- lib/screentracker/pty_conversation_test.go | 40 +++++++++++++++++----- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index 1db6f06d..8d65e064 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -515,18 +515,27 @@ func TestMessages(t *testing.T) { t.Run("send-message-no-echo-context-cancelled", func(t *testing.T) { // Given: a non-echoing agent and a cancellable context. + // The onWrite signals when writeStabilize starts writing + // message parts — this is used to synchronize the cancel. sendCtx, sendCancel := context.WithCancel(context.Background()) t.Cleanup(sendCancel) + writeStarted := make(chan struct{}, 1) c, _, mClock := newConversation(sendCtx, t, func(cfg *st.PTYConversationConfig) { a := &testAgent{screen: "prompt"} - a.onWrite = func(data []byte) {} + a.onWrite = func(data []byte) { + select { + case writeStarted <- struct{}{}: + default: + } + } cfg.AgentIO = a }) advanceFor(sendCtx, t, mClock, interval*threshold) // When: a message is sent and the context is cancelled - // during Phase 1 (before echo detection completes). + // during Phase 1 (after the message is written to the + // PTY, before echo detection completes). var sendErr error var sendDone atomic.Bool go func() { @@ -534,16 +543,31 @@ func TestMessages(t *testing.T) { sendDone.Store(true) }() - // Advance past the snapshot interval so the send loop - // picks up the message, then cancel the context. - advanceFor(sendCtx, t, mClock, interval) + // Advance tick-by-tick until writeStabilize starts + // (onWrite fires). This gives the send loop goroutine + // scheduling time between advances. + advanceUntil(sendCtx, t, mClock, func() bool { + select { + case <-writeStarted: + return true + default: + return false + } + }) + + // writeStabilize Phase 1 is now running. Its WaitFor is + // blocked on a mock timer sleep select. Cancel: the + // select sees ctx.Done() immediately. sendCancel() - // Wait for Send to complete (it should fail quickly - // after cancellation). + // WaitFor returns ctx.Err(). The errors.Is guard in + // Phase 1 propagates it as fatal. Use Eventually since + // the goroutine needs scheduling time. require.Eventually(t, sendDone.Load, 5*time.Second, 10*time.Millisecond) - // Then: the error wraps context.Canceled, not a Phase 2 error. + // Then: the error wraps context.Canceled, not a Phase 2 + // timeout. This validates the errors.Is(WaitTimedOut) + // guard. require.Error(t, sendErr) assert.ErrorIs(t, sendErr, context.Canceled) assert.NotContains(t, sendErr.Error(), "failed to wait for processing to start")