Skip to content

Commit dfd3caa

Browse files
committed
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
1 parent 3cb783f commit dfd3caa

2 files changed

Lines changed: 13 additions & 15 deletions

File tree

lib/screentracker/pty_conversation.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import (
2020
const (
2121
// writeStabilizeEchoTimeout is the timeout for the echo
2222
// detection WaitFor loop in writeStabilize Phase 1. The
23-
// effective ceiling may be slightly longer because the 1s
23+
// effective ceiling may be slightly longer because the
2424
// stability check inside the condition runs outside
2525
// WaitFor's timeout select. Non-echoing agents (e.g. TUI
2626
// agents using bracketed paste) will hit this timeout,
@@ -438,12 +438,12 @@ func (c *PTYConversation) sendMessage(ctx context.Context, messageParts ...Messa
438438
// Phase 1 (echo detection): writes the message text and waits
439439
// for the screen to change and stabilize. This detects agents
440440
// that echo typed input. If the screen doesn't change within
441-
// writeStabilizeEchoTimeout, this is non-fatal — many TUI
441+
// writeStabilizeEchoTimeout, this is non-fatal. Many TUI
442442
// agents buffer bracketed-paste input without rendering it.
443443
//
444444
// Phase 2 (processing detection): writes a carriage return
445445
// and waits for the screen to change, indicating the agent
446-
// started processing. This phase is fatal on timeout if the
446+
// started processing. This phase is fatal on timeout: if the
447447
// agent doesn't react to Enter, it's unresponsive.
448448
func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...MessagePart) error {
449449
screenBeforeMessage := c.cfg.AgentIO.ReadScreen()
@@ -452,7 +452,8 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me
452452
return xerrors.Errorf("failed to write message part: %w", err)
453453
}
454454
}
455-
// wait for the screen to stabilize after the message is written
455+
// Phase 1: wait for the screen to stabilize after the
456+
// message is written (echo detection).
456457
if err := util.WaitFor(ctx, util.WaitTimeout{
457458
Timeout: writeStabilizeEchoTimeout,
458459
MinInterval: 50 * time.Millisecond,
@@ -488,7 +489,8 @@ func (c *PTYConversation) writeStabilize(ctx context.Context, messageParts ...Me
488489
)
489490
}
490491

491-
// wait for the screen to change after the carriage return is written
492+
// Phase 2: wait for the screen to change after the
493+
// carriage return is written (processing detection).
492494
screenBeforeCarriageReturn := c.cfg.AgentIO.ReadScreen()
493495
lastCarriageReturnTime := time.Time{}
494496
if err := util.WaitFor(ctx, util.WaitTimeout{

lib/screentracker/pty_conversation_test.go

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"io"
88
"log/slog"
99
"os"
10+
"slices"
1011
"sync"
1112
"sync/atomic"
1213
"testing"
@@ -473,21 +474,16 @@ func TestMessages(t *testing.T) {
473474
// Then: Send succeeds and the user message is recorded.
474475
msgs := c.Messages()
475476
require.True(t, len(msgs) >= 2)
476-
var foundUserMsg bool
477-
for _, msg := range msgs {
478-
if msg.Role == st.ConversationRoleUser && msg.Message == "hello" {
479-
foundUserMsg = true
480-
break
481-
}
482-
}
483-
assert.True(t, foundUserMsg, "expected user message 'hello' in conversation")
477+
assert.True(t, slices.ContainsFunc(msgs, func(m st.ConversationMessage) bool {
478+
return m.Role == st.ConversationRoleUser && m.Message == "hello"
479+
}), "expected user message 'hello' in conversation")
484480
})
485481

486482
t.Run("send-message-no-echo-no-react", func(t *testing.T) {
487483
ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
488484
t.Cleanup(cancel)
489485

490-
// Given: an agent that is completely unresponsive — it
486+
// Given: an agent that is completely unresponsive. It
491487
// neither echoes input nor reacts to carriage return.
492488
c, _, mClock := newConversation(ctx, t, func(cfg *st.PTYConversationConfig) {
493489
a := &testAgent{screen: "prompt"}
@@ -516,7 +512,7 @@ func TestMessages(t *testing.T) {
516512
t.Run("send-message-no-echo-context-cancelled", func(t *testing.T) {
517513
// Given: a non-echoing agent and a cancellable context.
518514
// The onWrite signals when writeStabilize starts writing
519-
// message parts — this is used to synchronize the cancel.
515+
// message parts. This is used to synchronize the cancel.
520516
sendCtx, sendCancel := context.WithCancel(context.Background())
521517
t.Cleanup(sendCancel)
522518

0 commit comments

Comments
 (0)