refactor: move snapshot loop and initial prompt logic into PTYConversation #179

johnstcn · 2026-02-04T12:56:03Z

This PR refactors the snapshot loop and initial prompt handling to be fully encapsulated within PTYConversation, removing direct field manipulation from the HTTP server layer.

Moved StartSnapshotLoop into PTYConversation.Start()
Initial prompt sending is now handled within PTYConversation.Start()
Fix: Status no longer flips from "changing" → "stable" → "changing" when sending the initial prompt - it stays "changing" until the agent has actually finished responding

Also:

Fixes race condition in ./e2e
Improves logging in ./e2e
Removes separate test-only code paths in ./lib/screentracker
Refactors a lot of test code (for the better)

🤖 Created using Mux (Opus 4.5).

…nversation Changes: - Add InitialPrompt []MessagePart and OnSnapshot callback to PTYConversationConfig - Remove initialPrompt string parameter from NewPTY function (now reads from config) - Add initialPromptReady chan struct{} field for signaling readiness - Add sendLocked() helper (same as Send but without lock) - Add messagesLocked() helper that returns a copy of messages - Update statusLocked() to return ConversationStatusChanging while initial prompt is pending, fixing the status flip-flop issue (changing → stable → changing) - Update Start() to use select with: - Ticker for snapshots (calling OnSnapshot callback if set) - initialPromptReady channel to send initial prompt when ready This consolidates initial prompt logic inside PTYConversation.Start() instead of requiring the server to manipulate internal fields directly. The server.go changes to use this new API will be done in a separate commit.

…itialPrompt handling - Update NewServer to format InitialPrompt into []MessagePart via FormatMessage - Pass InitialPrompt and OnSnapshot callback in PTYConversationConfig - OnSnapshot callback emits status/messages/screen changes to EventEmitter - Remove initialPrompt string parameter from NewPTY call (now in config) - Simplify StartSnapshotLoop to just call s.conversation.Start(ctx) - Remove redundant goroutine, ticker, and initial prompt send logic The snapshot loop and initial prompt handling are now internalized in PTYConversation.Start(), which calls the OnSnapshot callback after each snapshot.

- Update all NewPTY calls to use new signature (config only, no initialPrompt param) - For tests needing initial prompt, use InitialPrompt config field with []MessagePart - Update tests to expect status 'changing' until InitialPromptSent is true (new behavior prevents status flip 'changing' -> 'stable' -> 'changing') - Remove direct manipulation of internal fields where possible, use Status() API - Keep minimal internal field access (InitialPromptSent) where needed for testing post-send behavior without running the Start() goroutine

sendLocked() was failing with ErrMessageValidationChanging because statusLocked() returns ConversationStatusChanging when InitialPromptSent is false. This is a chicken-and-egg problem: we need to send the initial prompt before we can set InitialPromptSent=true. Solution: Add skipStatusCheck parameter to sendLocked() to bypass the status check for the initial prompt case. The Start() goroutine passes true to skip the check, while external Send() calls pass false to preserve the existing validation behavior.

Remove the StartSnapshotLoop method which only delegated to s.conversation.Start(ctx), and add a Conversation() accessor method instead. This allows callers to invoke Start() directly on the conversation. Part of refactoring to move snapshot loop logic inside PTYConversation.

…versation Remove exported InitialPromptSent and ReadyForInitialPrompt boolean fields from PTYConversation struct: - InitialPromptSent → initialPromptSent (unexported) - ReadyForInitialPrompt boolean removed entirely The initialPromptReady channel now handles readiness signaling entirely. When the agent is ready (detected via cfg.ReadyForInitialPrompt callback), the channel is closed and set to nil to prevent double-close. This simplifies statusLocked() by removing the intermediate boolean state and using the channel's nil state to track whether readiness was already signaled. Note: Tests will need updates to verify behavior through Status() API rather than setting internal fields directly.

…napshotLoop The StartSnapshotLoop method was removed from Server in favor of exposing a Conversation() accessor that returns the PTYConversation, which has its own Start(ctx) method.

The InitialPromptSent field was unexported as initialPromptSent. Rework the test to verify the same behavior (normal status logic applies after initial prompt handling) by configuring no InitialPrompt instead of manually setting the field. When no InitialPrompt is configured, initialPromptSent defaults to true, which achieves the same testing outcome through the public API.

…rsation() accessor - Move the s.conversation.Start(ctx) call into NewServer(), just before the return statement, so the conversation starts immediately when the server is created. - Add nil check for config.Process to handle test scenarios where no process is configured. - Remove the Conversation() accessor method from Server since it is no longer needed externally. - Remove the external srv.Conversation().Start(ctx) call from cmd/server/server.go.

Remove the skipStatusCheck parameter from sendLocked and move the status check into Send() where it belongs. This simplifies the code since: - Start() always skipped the check (for initial prompt) - Send() always respected cfg.SkipSendMessageStatusCheck Now the check happens in Send() before calling sendLocked, and the initial prompt in Start() naturally bypasses it by calling sendLocked directly.

github-actions · 2026-02-04T12:58:12Z

✅ Preview binaries are ready!

To test with modules: agentapi_version = "agentapi_179" or download from: https://github.com/coder/agentapi/releases/tag/agentapi_179

- Expand comment for process nil check to explain: - Process is nil only for --print-openapi mode - Process is already running (termexec.StartProcess blocks) - Agent readiness is handled asynchronously via ReadyForInitialPrompt - Add comment for OnSnapshot callback explaining: - Callback pattern keeps screentracker decoupled from httpapi - Preserves clean package boundaries and avoids import cycles

Change Server.conversation from *st.PTYConversation to st.Conversation to program against the interface abstraction rather than the concrete type. This ensures the Conversation interface is a complete abstraction.

Use config.AgentType directly in the OnSnapshot closure instead of creating a redundant local variable.

Remove unnecessary channel creation for nil initialPromptReady. In Go's select statement, nil channel cases are simply skipped (never selected), so we don't need to create a new channel that blocks forever - the nil channel already has the desired behavior. Addresses PR review feedback.

johnstcn · 2026-02-04T15:47:04Z

lib/httpapi/server.go

+		// OnSnapshot uses a callback rather than passing the emitter directly
+		// to keep the screentracker package decoupled from httpapi concerns.
+		// This preserves clean package boundaries and avoids import cycles.
+		OnSnapshot: func(status st.ConversationStatus, messages []st.ConversationMessage, screen string) {
+			emitter.UpdateStatusAndEmitChanges(status, agentType)
+			emitter.UpdateMessagesAndEmitChanges(messages)
+			emitter.UpdateScreenAndEmitChanges(screen)
+		},


Self-review: Could alternatively extract an Emitter interface and pass this in.

johnstcn · 2026-02-04T15:49:12Z

lib/httpapi/server.go

 	mu           sync.RWMutex
 	logger       *slog.Logger
-	conversation *st.PTYConversation
+	conversation st.Conversation


self-review: this is the whole point of this PR

Copilot

Pull request overview

This PR refactors the PTYConversation implementation to fully encapsulate snapshot polling and initial prompt handling, removing direct field manipulation from the HTTP server layer and fixing a status transition bug.

Changes:

Moved snapshot loop from Server.StartSnapshotLoop() into PTYConversation.Start()
Initial prompt configuration and sending logic now managed entirely within PTYConversation using a channel-based signaling mechanism
Status logic updated to stay "changing" until initial prompt is sent, preventing the "changing" → "stable" → "changing" flip

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
lib/screentracker/pty_conversation.go	Added OnSnapshot callback, InitialPrompt config field, channel-based initial prompt signaling in Start(), and updated status logic to prevent premature "stable" transitions
lib/screentracker/pty_conversation_test.go	Updated tests to reflect new API (InitialPrompt moved to config) and new behavior (status stays "changing" until initial prompt sent)
lib/httpapi/server.go	Removed StartSnapshotLoop, integrated snapshot loop via conversation.Start(), added OnSnapshot callback to update emitter, changed conversation field from concrete type to interface
cmd/server/server.go	Removed StartSnapshotLoop call (now handled in NewServer)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/screentracker/pty_conversation.go

The public InitialPrompt string field is no longer used after refactoring. The initial prompt is now stored in cfg.InitialPrompt (as []MessagePart) and managed internally. Removing this field avoids confusion and maintains clean encapsulation. Addresses PR review feedback.

mafredri

Nice work. I think the changes look good in general, although internal state management could use a bit of additional refactoring.

lib/screentracker/pty_conversation.go

mafredri · 2026-02-05T10:02:14Z

Thinking about the current implementation some more, I wonder why we even need initial prompt handling? Wouldn't it be nicer to just have a queue of messages where the initial prompt goes in first? I'd assume this would simplify the logic as you just wait until stable to send the message?

johnstcn · 2026-02-05T10:05:17Z

Thinking about the current implementation some more, I wonder why we even need initial prompt handling? Wouldn't it be nicer to just have a queue of messages where the initial prompt goes in first? I'd assume this would simplify the logic as you just wait until stable to send the message?

Yeah, that's a nice idea actually.

35C4n0r · 2026-02-05T10:37:33Z

Thinking about the current implementation some more, I wonder why we even need initial prompt handling? Wouldn't it be nicer to just have a queue of messages where the initial prompt goes in first? I'd assume this would simplify the logic as you just wait until stable to send the message?

would help solve this too: #21

Replace initialPromptSent bool and initialPromptReady chan with: - outboundQueue chan []MessagePart (buffered, size 1) - agentReady chan struct{} (nil if no initial prompt) The initial prompt is now enqueued in NewPTY() and sent via the queue in Start(). This makes the code more extensible for future queued message handling. Start() now uses a two-phase loop: - Phase 1: Wait for agentReady while still processing ticker snapshots - Phase 2: Normal loop with ticker + outboundQueue select cases No external API behavior changes.

Add no-op default functions for OnSnapshot and ReadyForInitialPrompt in NewPTY() constructor instead of checking for nil throughout the code. This removes: - Two nil checks for OnSnapshot in Start() phase 1 and phase 2 loops - One nil check for ReadyForInitialPrompt in statusLocked()

Replace atomic.Bool with a channel that gets closed for one-time signaling, which is more idiomatic Go. Remove sync/atomic import since it's no longer needed.

johnstcn · 2026-02-05T20:13:40Z

e2e/echo_test.go

-		_, err = os.Stat(binaryPath)
-		if err != nil {


review: rebuilding binary here on every run as a stale binary caused me to miss a potential issue

johnstcn · 2026-02-05T20:25:16Z

lib/screentracker/pty_conversation.go

 }

 func (c *PTYConversation) Start(ctx context.Context) {
+	// Initial prompt readiness loop - polls ReadyForInitialPrompt until it returns true,


Start now runs three goroutines:

Wait for initial readiness

Poll for status and signal readiness to send messages

Pull from internal queue and send messages

Wait goroutine blocks poll until initial ready, and then exits.
Poll blocks pull until agent is stable and there is a message to send.
Pull pulls from outbound queue and sends.

- Add outboundMessage type with parts and errCh for async response - Update outboundQueue to use outboundMessage type - Rewrite Send() to validate, then enqueue with error channel - Simplify send loop to read from queue and return errors via errCh - Remove duplicate validation from sendLocked() (now done in Send()) - Add started flag to track if Start() was called (for test compat)

lib/screentracker/pty_conversation.go

35C4n0r · 2026-02-10T10:40:12Z

Something's wrong with the PR preview build workflow, fixing it in a separate PR.

- Remove 'started' field and synchronous bypass in Send() - Remove SkipWritingMessage config field - Remove public Snapshot() test-only method - Convert readiness/snapshot loops from NewTicker to TickerFunc - Pass Clock to WaitFor calls in writeStabilize

- Rename sendLocked to sendMessage; release lock during writeStabilize to avoid deadlocking with the snapshot TickerFunc, then re-acquire and re-apply the pre-send agent message to correct for intermediate snapshot loop updates. - Fix advancePast: retry briefly when no events are pending instead of calling Advance(remaining) which races with goroutine timer creation. - Fix sendWithClockDrive: use AdvanceNext instead of Advance(d) to avoid race between Peek and Advance. - Fix initial prompt lifecycle test: advance 2 ticks to account for snapshot timer alignment.

- Pass context.Context into newConversation, return 3 values - Simplify advancePast: remove retry loop and time.Sleep - Simplify sendWithClockDrive: remove Peek polling and time.Sleep

…e tests

- Replace time.Now() with fixed time.Date(2025, 1, 1, ...) - Delete msgNoTime type and stripTimes function - Rewrite assertMessages to compare full ConversationMessage fields with flexible time checking (zero = assert non-zero) - Update all call sites to use []st.ConversationMessage with named fields

Extract a shared driveClockUntil helper that advances the mock clock one event at a time until a condition is met. Use it in both sendWithClockDrive and the initial prompt lifecycle test, replacing the ad-hoc 500-iteration loop. Also change sendWithClockDrive to return nothing (instead of error) since all call sites were wrapping it with require.NoError. The error check now happens inside via require.NoError.

johnstcn · 2026-02-10T18:21:29Z

e2e/echo.go

-			// Clear spinner on cancellation
-			fmt.Print("\r" + strings.Repeat(" ", 20) + "\r")
-			return
+func runSpinner(ctx context.Context) <-chan struct{} {


review: turns out there was a race condition in our e2e echo agent 😂

johnstcn · 2026-02-10T18:21:51Z

e2e/echo_test.go

+				if err := json.NewEncoder(&sb).Encode(evt); err != nil {
+					t.Logf("Failed to encode event: %v", err)
+				}
+				t.Logf("Got event: %s", sb.String())


review: improved logging

johnstcn · 2026-02-10T18:23:20Z

lib/screentracker/pty_conversation.go

+	// Re-apply the pre-send agent message from the screen captured before
+	// the write. While the lock was released during writeStabilize, the
+	// snapshot loop continued taking snapshots and calling
+	// updateLastAgentMessageLocked with whatever was on screen at each
+	// tick (typically echoed user input or intermediate terminal state).
+	// Those updates corrupt the agent message for this turn. Restoring it
+	// here ensures the conversation history is correct. The next line sets
+	// screenBeforeLastUserMessage so the *next* agent message will be
+	// diffed relative to the pre-send screen.
+	c.updateLastAgentMessageLocked(screenBeforeMessage, now)


review: I'm not really a fan of this. I'd much prefer to have the snapshot loop be paused while we're writing but this seems to cause deadlocks in tests.

johnstcn · 2026-02-10T18:23:46Z

lib/screentracker/pty_conversation.go

+	// Handle initial prompt readiness: report "changing" until the queue is drained
+	// to avoid the status flipping "changing" -> "stable" -> "changing"
+	if len(c.outboundQueue) > 0 {


review: this is a behavioural change

johnstcn · 2026-02-10T18:24:53Z

lib/screentracker/pty_conversation_test.go

+func assertMessages(t *testing.T, c *st.PTYConversation, expected []st.ConversationMessage) {
+	t.Helper()
+	actual := c.Messages()
+	for i := range actual {
+		require.False(t, actual[i].Time.IsZero(), "message %d Time should be non-zero", i)
+		actual[i].Time = time.Time{}
+	}
+	require.Equal(t, expected, actual)
+}


review: this is so we don't need to care about the times, just that they are non-zero.

johnstcn added 10 commits February 4, 2026 11:29

refactor: use srv.Conversation().Start(ctx) instead of removed StartS…

d16c552

…napshotLoop The StartSnapshotLoop method was removed from Server in favor of exposing a Conversation() accessor that returns the PTYConversation, which has its own Start(ctx) method.

johnstcn self-assigned this Feb 4, 2026

johnstcn added 3 commits February 4, 2026 15:48

refactor: use Conversation interface in Server struct

f5bd08d

Change Server.conversation from *st.PTYConversation to st.Conversation to program against the interface abstraction rather than the concrete type. This ensures the Conversation interface is a complete abstraction.

refactor: remove unnecessary agentType local variable

86c9d91

Use config.AgentType directly in the OnSnapshot closure instead of creating a redundant local variable.

johnstcn commented Feb 4, 2026

View reviewed changes

johnstcn requested review from 35C4n0r and mafredri February 4, 2026 16:14

johnstcn marked this pull request as ready for review February 4, 2026 16:14

Copilot AI review requested due to automatic review settings February 4, 2026 16:14

Copilot started reviewing on behalf of johnstcn February 4, 2026 16:14 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

lib/screentracker/pty_conversation.go Outdated Show resolved Hide resolved

mafredri reviewed Feb 5, 2026

View reviewed changes

lib/screentracker/pty_conversation.go Outdated Show resolved Hide resolved

lib/screentracker/pty_conversation.go Outdated Show resolved Hide resolved

johnstcn added 2 commits February 5, 2026 11:40

refactor: use channel for initialPromptReady one-time signaling

0b9e687

Replace atomic.Bool with a channel that gets closed for one-time signaling, which is more idiomatic Go. Remove sync/atomic import since it's no longer needed.

johnstcn commented Feb 5, 2026

View reviewed changes

johnstcn added 2 commits February 9, 2026 13:17

fix: handle context cancellation in inner select

18e2773

35C4n0r approved these changes Feb 10, 2026

View reviewed changes

johnstcn commented Feb 10, 2026

View reviewed changes

lib/screentracker/pty_conversation.go Outdated Show resolved Hide resolved

johnstcn added 15 commits February 10, 2026 11:05

Remove SkipSendMessageStatusCheck from PTYConversationConfig

c17e6d7

Reorder readiness check for short-circuit and expand re-apply comment

c073f04

refactor: simplify test helpers in pty_conversation_test.go

429f35d

- Pass context.Context into newConversation, return 3 values - Simplify advancePast: remove retry loop and time.Sleep - Simplify sendWithClockDrive: remove Peek polling and time.Sleep

Replace wall-clock deadline with context timeout in sendWithClockDriv…

85d3965

…e tests

Simplify assertMessages to use require.Equal instead of per-field loop

38f119d

screentracker test: add ctx.Done select and onWrite comment

9a1b238

fix: rewrite util.WaitFor to use a single timer

a134116

e2e: improve debug output

131e4a5

refactor: inline fillToStable

8debc4e

refactor: simplify advance test helpers

87d36ab

chore: fix e2e race condition

0e37003

johnstcn commented Feb 10, 2026

View reviewed changes

35C4n0r self-requested a review February 10, 2026 18:52

Merge branch 'main' into conversation-ew5m

5df1ec3

refactor: move snapshot loop and initial prompt logic into PTYConversation #179

Are you sure you want to change the base?

refactor: move snapshot loop and initial prompt logic into PTYConversation #179

Conversation

johnstcn commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mafredri commented Feb 5, 2026

Uh oh!

johnstcn commented Feb 5, 2026

Uh oh!

35C4n0r commented Feb 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

35C4n0r commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnstcn commented Feb 4, 2026 •

edited

Loading