refactor: move snapshot loop and initial prompt logic into PTYConversation#179
refactor: move snapshot loop and initial prompt logic into PTYConversation#179
Conversation
…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.
|
✅ Preview binaries are ready! To test with modules: |
- 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.
| // 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) | ||
| }, |
There was a problem hiding this comment.
Self-review: Could alternatively extract an Emitter interface and pass this in.
| mu sync.RWMutex | ||
| logger *slog.Logger | ||
| conversation *st.PTYConversation | ||
| conversation st.Conversation |
There was a problem hiding this comment.
self-review: this is the whole point of this PR
There was a problem hiding this comment.
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()intoPTYConversation.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.
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
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Nice work. I think the changes look good in general, although internal state management could use a bit of additional refactoring.
| // to avoid the status flipping "changing" → "stable" → "changing" | ||
| if !c.initialPromptSent { | ||
| // Check if agent is ready for initial prompt and signal if so | ||
| if c.initialPromptReady != nil && len(snapshots) > 0 && c.cfg.ReadyForInitialPrompt != nil && c.cfg.ReadyForInitialPrompt(snapshots[len(snapshots)-1].screen) { |
There was a problem hiding this comment.
This if statement is a bit unwieldy and could use a bit of refactoring.
A few things that caught my eye:
- Use default no-op callbacks/handlers unless overridden, this avoids the != nil checks
- If c.cfg.ReadyForInitialPrompt fn isn't set, we'll never close the initialPromptReady channel, never set
initialPromptSentand keep re-checking this if-statement in perpetuity
| if err := c.sendLocked(c.cfg.InitialPrompt...); err != nil { | ||
| c.cfg.Logger.Error("failed to send initial prompt", "error", err) | ||
| } else { | ||
| c.initialPromptSent = true |
There was a problem hiding this comment.
Why do we manage initial prompt state here instead of sendLocked? IMO this could be simplified to:
case <-initialPromptReady:
if len(c.cfg.InitialPrompt) > 0 {
err := c.send(c.cfg.InitialPrompt...)
// if err
}
initialPromptReady = nil(Or sendLocked with manual mutex handling, but the idea is the same.)
This would keep all the initial prompt state handling contained in one place.
Thinking about this some more, some more refactoring may be necessary as it's weird that sendLocked is called with c.cfg.InitialPrompt without sendLocked knowing that's the initial prompt. We probably shouldn't even allow a send to happen before the initial prompt has been processed.
This could warrant something along the lines of sendInternal(initial bool, ...) and sendInitial(...) and send(...).
|
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. |
would help solve this too: #21 |
3 participants
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.StartSnapshotLoopintoPTYConversation.Start()PTYConversation.Start()🤖 Created using Mux (Opus 4.5).