refactor: move snapshot loop and initial prompt logic into PTYConversation by johnstcn · Pull Request #179 · coder/agentapi

added 10 commits

February 4, 2026 11:29
…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.

@johnstcn

- 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

@johnstcn 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

@johnstcn

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

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 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

@johnstcn

@johnstcn

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

@johnstcn

@johnstcn

@johnstcn

@johnstcn

johnstcn

johnstcn

johnstcn

johnstcn

johnstcn

@johnstcn

mafredri

@johnstcn

@johnstcn

@johnstcn

@johnstcn

Add comments explaining why close(msg.errCh) is called at both sites.

Add sendingMessage flag to prevent statusLocked() from returning
'stable' while the send loop is processing a dequeued message
outside the lock.

@johnstcn