From fcc12e4a5a4e56fa922795389d4f7439dd846e01 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 31 Mar 2026 11:30:49 +0000 Subject: [PATCH 1/6] 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/6] 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/6] 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/6] 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") From 3cb783f43220c42eab2f3dbfaa835233463f151b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 9 Apr 2026 16:08:02 +0100 Subject: [PATCH 5/6] fix(screentracker): prevent Send() from blocking when ReadyForInitialPrompt stays false (#215) --- lib/msgfmt/message_box.go | 21 ++++--- .../claude/ready/msg_dashed_box.txt | 8 +++ .../claude/ready/msg_slim_dashed_box.txt | 8 +++ lib/screentracker/pty_conversation.go | 8 +++ lib/screentracker/pty_conversation_test.go | 61 +++++++++++++++---- 5 files changed, 86 insertions(+), 20 deletions(-) create mode 100644 lib/msgfmt/testdata/initialization/claude/ready/msg_dashed_box.txt create mode 100644 lib/msgfmt/testdata/initialization/claude/ready/msg_slim_dashed_box.txt diff --git a/lib/msgfmt/message_box.go b/lib/msgfmt/message_box.go index 1ac75c9e..ca86dd77 100644 --- a/lib/msgfmt/message_box.go +++ b/lib/msgfmt/message_box.go @@ -4,15 +4,22 @@ import ( "strings" ) +// containsHorizontalBorder reports whether the line contains a +// horizontal border made of box-drawing characters (─ or ╌). +func containsHorizontalBorder(line string) bool { + return strings.Contains(line, "───────────────") || + strings.Contains(line, "╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌") +} + // Usually something like -// ─────────────── +// ─────────────── (or ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌) // > -// ─────────────── +// ─────────────── (or ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌) // Used by Claude Code, Goose, and Aider. func findGreaterThanMessageBox(lines []string) int { for i := len(lines) - 1; i >= max(len(lines)-6, 0); i-- { if strings.Contains(lines[i], ">") { - if i > 0 && strings.Contains(lines[i-1], "───────────────") { + if i > 0 && containsHorizontalBorder(lines[i-1]) { return i - 1 } return i @@ -22,14 +29,14 @@ func findGreaterThanMessageBox(lines []string) int { } // Usually something like -// ─────────────── +// ─────────────── (or ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌) // | -// ─────────────── +// ─────────────── (or ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌) func findGenericSlimMessageBox(lines []string) int { for i := len(lines) - 3; i >= max(len(lines)-9, 0); i-- { - if strings.Contains(lines[i], "───────────────") && + if containsHorizontalBorder(lines[i]) && (strings.Contains(lines[i+1], "|") || strings.Contains(lines[i+1], "│") || strings.Contains(lines[i+1], "❯")) && - strings.Contains(lines[i+2], "───────────────") { + containsHorizontalBorder(lines[i+2]) { return i } } diff --git a/lib/msgfmt/testdata/initialization/claude/ready/msg_dashed_box.txt b/lib/msgfmt/testdata/initialization/claude/ready/msg_dashed_box.txt new file mode 100644 index 00000000..f7ab0382 --- /dev/null +++ b/lib/msgfmt/testdata/initialization/claude/ready/msg_dashed_box.txt @@ -0,0 +1,8 @@ + 1 function greet() { + 2 - console.log("Hello, World!"); + 2 + console.log("Hello, Claude!"); + 3 } + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + > Try "what does this code do?" + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + Syntax theme: Monokai Extended (ctrl+t to disable) diff --git a/lib/msgfmt/testdata/initialization/claude/ready/msg_slim_dashed_box.txt b/lib/msgfmt/testdata/initialization/claude/ready/msg_slim_dashed_box.txt new file mode 100644 index 00000000..87e285f5 --- /dev/null +++ b/lib/msgfmt/testdata/initialization/claude/ready/msg_slim_dashed_box.txt @@ -0,0 +1,8 @@ +╭────────────────────────────────────────────╮ +│ ✻ Welcome to Claude Code! │ +│ │ +│ /help for help │ +╰────────────────────────────────────────────╯ + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + │ Type your message... + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ diff --git a/lib/screentracker/pty_conversation.go b/lib/screentracker/pty_conversation.go index 80c9b942..8e325ea7 100644 --- a/lib/screentracker/pty_conversation.go +++ b/lib/screentracker/pty_conversation.go @@ -571,6 +571,14 @@ func (c *PTYConversation) statusLocked() ConversationStatus { return ConversationStatusChanging } + // The send loop gates stableSignal on initialPromptReady. + // Report "changing" until readiness is detected so that Send() + // rejects with ErrMessageValidationChanging instead of blocking + // indefinitely on a stableSignal that will never fire. + if !c.initialPromptReady { + return ConversationStatusChanging + } + // Handle initial prompt readiness: report "changing" until the queue is drained // to avoid the status flipping "changing" -> "stable" -> "changing" if len(c.outboundQueue) > 0 || c.sendingMessage { diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index 8d65e064..77de7eea 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -1200,7 +1200,7 @@ func TestStatePersistence(t *testing.T) { func TestInitialPromptReadiness(t *testing.T) { discardLogger := slog.New(slog.NewTextHandler(io.Discard, nil)) - t.Run("agent not ready - status is stable until agent becomes ready", func(t *testing.T) { + t.Run("agent not ready - status is changing until agent becomes ready", func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testTimeout) t.Cleanup(cancel) mClock := quartz.NewMock(t) @@ -1223,9 +1223,9 @@ func TestInitialPromptReadiness(t *testing.T) { // Take a snapshot with "loading...". Threshold is 1 (stability 0 / interval 1s = 0 + 1 = 1). advanceFor(ctx, t, mClock, 1*time.Second) - // Screen is stable and agent is not ready, so initial prompt hasn't been enqueued yet. - // Status should be stable. - assert.Equal(t, st.ConversationStatusStable, c.Status()) + // Screen is stable but agent is not ready. Status must be + // "changing" so that Send() rejects instead of blocking. + assert.Equal(t, st.ConversationStatusChanging, c.Status()) }) t.Run("agent becomes ready - prompt enqueued and status changes to changing", func(t *testing.T) { @@ -1248,10 +1248,9 @@ func TestInitialPromptReadiness(t *testing.T) { c := st.NewPTY(ctx, cfg, &testEmitter{}) c.Start(ctx) - // Agent not ready initially, status should be stable + // Agent not ready initially, status should be changing. advanceFor(ctx, t, mClock, 1*time.Second) - assert.Equal(t, st.ConversationStatusStable, c.Status()) - + assert.Equal(t, st.ConversationStatusChanging, c.Status()) // Agent becomes ready, prompt gets enqueued, status becomes "changing" agent.setScreen("ready") advanceFor(ctx, t, mClock, 1*time.Second) @@ -1283,10 +1282,9 @@ func TestInitialPromptReadiness(t *testing.T) { c := st.NewPTY(ctx, cfg, &testEmitter{}) c.Start(ctx) - // Status is "stable" while waiting for readiness (prompt not yet enqueued). + // Status is "changing" while waiting for readiness (prompt not yet enqueued). advanceFor(ctx, t, mClock, 1*time.Second) - assert.Equal(t, st.ConversationStatusStable, c.Status()) - + assert.Equal(t, st.ConversationStatusChanging, c.Status()) // Agent becomes ready. The snapshot loop detects this, enqueues the prompt, // then sees queue + stable + ready and signals the send loop. // writeStabilize runs with onWrite changing the screen, so it completes. @@ -1304,7 +1302,7 @@ func TestInitialPromptReadiness(t *testing.T) { assert.Equal(t, st.ConversationStatusStable, c.Status()) }) - t.Run("no initial prompt - normal status logic applies", func(t *testing.T) { + t.Run("ReadyForInitialPrompt always false - status is changing", func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testTimeout) t.Cleanup(cancel) mClock := quartz.NewMock(t) @@ -1325,8 +1323,10 @@ func TestInitialPromptReadiness(t *testing.T) { advanceFor(ctx, t, mClock, 1*time.Second) - // Status should be stable because no initial prompt to wait for. - assert.Equal(t, st.ConversationStatusStable, c.Status()) + // Even without an initial prompt, stableSignal gates on + // initialPromptReady. Status must reflect that Send() + // would block. + assert.Equal(t, st.ConversationStatusChanging, c.Status()) }) t.Run("no initial prompt configured - normal status logic applies", func(t *testing.T) { @@ -1743,3 +1743,38 @@ func TestInitialPromptSent(t *testing.T) { } }) } + +func TestSendRejectsWhenInitialPromptNotReady(t *testing.T) { + // Regression test for https://github.com/coder/agentapi/issues/209. + // Send() used to block forever when ReadyForInitialPrompt never + // returned true, because statusLocked() reported "stable" while + // stableSignal required initialPromptReady. Now statusLocked() + // returns "changing" and Send() rejects immediately. + ctx, cancel := context.WithTimeout(context.Background(), testTimeout) + t.Cleanup(cancel) + + mClock := quartz.NewMock(t) + agent := &testAgent{screen: "onboarding screen without message box"} + cfg := st.PTYConversationConfig{ + Clock: mClock, + SnapshotInterval: 100 * time.Millisecond, + ScreenStabilityLength: 200 * time.Millisecond, + AgentIO: agent, + ReadyForInitialPrompt: func(message string) bool { + return false // Simulates failed message box detection. + }, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + c := st.NewPTY(ctx, cfg, &testEmitter{}) + c.Start(ctx) + + // Fill snapshot buffer to reach stability. + advanceFor(ctx, t, mClock, 300*time.Millisecond) + + // Status reports "changing" because initialPromptReady is false. + assert.Equal(t, st.ConversationStatusChanging, c.Status()) + + // Send() rejects immediately instead of blocking forever. + err := c.Send(st.MessagePartText{Content: "hello"}) + assert.ErrorIs(t, err, st.ErrMessageValidationChanging) +} From dfd3caa9af26fd9a1a73b2637430941cd3f52a70 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 9 Apr 2026 15:25:54 +0000 Subject: [PATCH 6/6] fix(screentracker): address round 2 review feedback - Drop hardcoded '1s' from writeStabilizeEchoTimeout comment so it won't go stale if the stability check duration changes - Replace em-dashes (U+2014) with periods in four comments - Use Phase 1/Phase 2 vocabulary in inline comments to match the doc comment on writeStabilize - Replace loop-and-flag with slices.ContainsFunc in test --- lib/screentracker/pty_conversation.go | 12 +++++++----- lib/screentracker/pty_conversation_test.go | 16 ++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/screentracker/pty_conversation.go b/lib/screentracker/pty_conversation.go index 8e325ea7..452c6ae9 100644 --- a/lib/screentracker/pty_conversation.go +++ b/lib/screentracker/pty_conversation.go @@ -20,7 +20,7 @@ import ( const ( // writeStabilizeEchoTimeout is the timeout for the echo // detection WaitFor loop in writeStabilize Phase 1. The - // effective ceiling may be slightly longer because the 1s + // effective ceiling may be slightly longer because the // 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, @@ -438,12 +438,12 @@ func (c *PTYConversation) sendMessage(ctx context.Context, messageParts ...Messa // 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 +// 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 +// 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() @@ -452,7 +452,8 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me return xerrors.Errorf("failed to write message part: %w", err) } } - // wait for the screen to stabilize after the message is written + // Phase 1: wait for the screen to stabilize after the + // message is written (echo detection). if err := util.WaitFor(ctx, util.WaitTimeout{ Timeout: writeStabilizeEchoTimeout, MinInterval: 50 * time.Millisecond, @@ -488,7 +489,8 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me ) } - // wait for the screen to change after the carriage return is written + // Phase 2: wait for the screen to change after the + // carriage return is written (processing detection). screenBeforeCarriageReturn := c.cfg.AgentIO.ReadScreen() lastCarriageReturnTime := time.Time{} if err := util.WaitFor(ctx, util.WaitTimeout{ diff --git a/lib/screentracker/pty_conversation_test.go b/lib/screentracker/pty_conversation_test.go index 77de7eea..cad58e83 100644 --- a/lib/screentracker/pty_conversation_test.go +++ b/lib/screentracker/pty_conversation_test.go @@ -7,6 +7,7 @@ import ( "io" "log/slog" "os" + "slices" "sync" "sync/atomic" "testing" @@ -473,21 +474,16 @@ func TestMessages(t *testing.T) { // 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, slices.ContainsFunc(msgs, func(m st.ConversationMessage) bool { + return m.Role == st.ConversationRoleUser && m.Message == "hello" + }), "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 + // 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"} @@ -516,7 +512,7 @@ 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. + // message parts. This is used to synchronize the cancel. sendCtx, sendCancel := context.WithCancel(context.Background()) t.Cleanup(sendCancel)