feat: address ai's review

Author: 35C4n0r

cmd/server/signals_windows.go

@@ -7,17 +7,14 @@ import (
 	"log/slog"
 	"os"
 	"os/signal"
-	"syscall"
 
 	"github.com/coder/agentapi/lib/httpapi"
 )
 
 // handleSignals sets up signal handlers for Windows.
-// Only handles SIGTERM and SIGINT (SIGHUP and SIGUSR1 don't exist on Windows).
 func handleSignals(ctx context.Context, cancel context.CancelFunc, logger *slog.Logger, srv *httpapi.Server) {
-	// Handle shutdown signals (SIGTERM, SIGINT only on Windows)
 	shutdownCh := make(chan os.Signal, 1)
-	signal.Notify(shutdownCh, os.Interrupt, syscall.SIGTERM)
+	signal.Notify(shutdownCh, os.Interrupt)
 	go func() {
 		defer signal.Stop(shutdownCh)
 		sig := <-shutdownCh

lib/httpapi/events.go

@@ -6,6 +6,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/coder/quartz"
+
 	mf "github.com/coder/agentapi/lib/msgfmt"
 	st "github.com/coder/agentapi/lib/screentracker"
 	"github.com/coder/agentapi/lib/util"
@@ -54,9 +56,9 @@ type ScreenUpdateBody struct {
 }
 
 type ErrorBody struct {
-	Message string    `json:"message" doc:"Error message"`
-	Level   string    `json:"level" doc:"Error level: 'warning' or 'error'"`
-	Time    time.Time `json:"time" doc:"Timestamp when the error occurred"`
+	Message string        `json:"message" doc:"Error message"`
+	Level   st.ErrorLevel `json:"level" doc:"Error level"`
+	Time    time.Time     `json:"time" doc:"Timestamp when the error occurred"`
 }
 
 type Event struct {
@@ -74,6 +76,7 @@ type EventEmitter struct {
 	subscriptionBufSize uint
 	screen              string
 	errors              []ErrorBody
+	clock               quartz.Clock
 }
 
 func convertStatus(status st.ConversationStatus) AgentStatus {
@@ -109,6 +112,12 @@ func WithAgentType(agentType mf.AgentType) EventEmitterOption {
 	}
 }
 
+func WithClock(clock quartz.Clock) EventEmitterOption {
+	return func(e *EventEmitter) {
+		e.clock = clock
+	}
+}
+
 func NewEventEmitter(opts ...EventEmitterOption) *EventEmitter {
 	e := &EventEmitter{
 		messages:            make([]st.ConversationMessage, 0),
@@ -119,6 +128,9 @@ func NewEventEmitter(opts ...EventEmitterOption) *EventEmitter {
 	for _, opt := range opts {
 		opt(e)
 	}
+	if e.clock == nil {
+		e.clock = quartz.NewReal()
+	}
 	return e
 }
 
@@ -202,14 +214,14 @@ func (e *EventEmitter) EmitScreen(newScreen string) {
 	e.screen = newScreen
 }
 
-func (e *EventEmitter) EmitError(message string, level string) {
+func (e *EventEmitter) EmitError(message string, level st.ErrorLevel) {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
 	errorBody := ErrorBody{
 		Message: message,
 		Level:   level,
-		Time:    time.Now(),
+		Time:    e.clock.Now(),
 	}
 
 	// Store the error so new subscribers can receive all errors

lib/httpapi/events_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	st "github.com/coder/agentapi/lib/screentracker"
+	"github.com/coder/quartz"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -97,4 +98,40 @@ func TestEventEmitter(t *testing.T) {
 			t.Fatalf("read should not block")
 		}
 	})
+
+	t.Run("clock-injection", func(t *testing.T) {
+		mockClock := quartz.NewMock(t)
+		fixedTime := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC)
+		mockClock.Set(fixedTime)
+
+		emitter := NewEventEmitter(WithClock(mockClock), WithSubscriptionBufSize(10))
+		_, ch, stateEvents := emitter.Subscribe()
+
+		// Verify initial state events
+		assert.Len(t, stateEvents, 2)
+
+		// Emit an error and verify it uses the mock clock time
+		emitter.EmitError("test error", st.ErrorLevelError)
+
+		event := <-ch
+		assert.Equal(t, EventTypeError, event.Type)
+		errorBody, ok := event.Payload.(ErrorBody)
+		assert.True(t, ok)
+		assert.Equal(t, "test error", errorBody.Message)
+		assert.Equal(t, st.ErrorLevelError, errorBody.Level)
+		assert.Equal(t, fixedTime, errorBody.Time)
+
+		// Advance the clock and emit another error
+		newTime := fixedTime.Add(1 * time.Hour)
+		mockClock.Set(newTime)
+		emitter.EmitError("another error", st.ErrorLevelWarning)
+
+		event = <-ch
+		assert.Equal(t, EventTypeError, event.Type)
+		errorBody, ok = event.Payload.(ErrorBody)
+		assert.True(t, ok)
+		assert.Equal(t, "another error", errorBody.Message)
+		assert.Equal(t, st.ErrorLevelWarning, errorBody.Level)
+		assert.Equal(t, newTime, errorBody.Time)
+	})
 }

lib/httpapi/server.go

@@ -278,7 +278,7 @@ func NewServer(ctx context.Context, config ServerConfig) (*Server, error) {
 	}
 	logger.Info("Created temporary directory for uploads", "tempDir", tempDir)
 
-	ctx, cancel := context.WithCancel(context.Background())
+	shutdownCtx, shutdownCancel := context.WithCancel(context.Background())
 
 	s := &Server{
 		router:       router,
@@ -292,8 +292,8 @@ func NewServer(ctx context.Context, config ServerConfig) (*Server, error) {
 		chatBasePath: strings.TrimSuffix(config.ChatBasePath, "/"),
 		tempDir:      tempDir,
 		clock:        config.Clock,
-		shutdownCtx:  ctx,
-		shutdown:     cancel,
+		shutdownCtx:  shutdownCtx,
+		shutdown:     shutdownCancel,
 	}
 
 	// Register API routes

lib/screentracker/conversation.go

@@ -34,6 +34,22 @@ var ConversationRoleValues = []ConversationRole{
 	ConversationRoleAgent,
 }
 
+type ErrorLevel string
+
+func (e ErrorLevel) Schema(r huma.Registry) *huma.Schema {
+	return util.OpenAPISchema(r, "ErrorLevel", ErrorLevelValues)
+}
+
+const (
+	ErrorLevelWarning ErrorLevel = "warning"
+	ErrorLevelError   ErrorLevel = "error"
+)
+
+var ErrorLevelValues = []ErrorLevel{
+	ErrorLevelWarning,
+	ErrorLevelError,
+}
+
 var (
 	ErrMessageValidationWhitespace = xerrors.New("message must be trimmed of leading and trailing whitespace")
 	ErrMessageValidationEmpty      = xerrors.New("message must not be empty")
@@ -80,14 +96,14 @@ type Emitter interface {
 	EmitMessages([]ConversationMessage)
 	EmitStatus(ConversationStatus)
 	EmitScreen(string)
-	EmitError(message string, level string)
+	EmitError(message string, level ErrorLevel)
 }
 
 type ConversationMessage struct {
-	Id      int
-	Message string
-	Role    ConversationRole
-	Time    time.Time
+	Id      int              `json:"id"`
+	Message string           `json:"message"`
+	Role    ConversationRole `json:"role"`
+	Time    time.Time        `json:"time"`
 }
 
 type StatePersistenceConfig struct {

lib/screentracker/pty_conversation.go

@@ -152,7 +152,7 @@ type noopEmitter struct{}
 func (noopEmitter) EmitMessages([]ConversationMessage) {}
 func (noopEmitter) EmitStatus(ConversationStatus)      {}
 func (noopEmitter) EmitScreen(string)                  {}
-func (noopEmitter) EmitError(_ string, _ string)       {}
+func (noopEmitter) EmitError(_ string, _ ErrorLevel)   {}
 
 func NewPTY(ctx context.Context, cfg PTYConversationConfig, emitter Emitter) *PTYConversation {
 	if cfg.Clock == nil {
@@ -207,7 +207,7 @@ func (c *PTYConversation) Start(ctx context.Context) {
 		if c.initialPromptReady && c.loadStateStatus == LoadStatePending && c.cfg.StatePersistenceConfig.LoadState {
 			if err := c.loadStateLocked(); err != nil {
 				c.cfg.Logger.Error("Failed to load state", "error", err)
-				c.emitter.EmitError(fmt.Sprintf("Failed to restore previous session: %v", err), "warning")
+				c.emitter.EmitError(fmt.Sprintf("Failed to restore previous session: %v", err), ErrorLevelWarning)
 				c.loadStateStatus = LoadStateFailed
 			} else {
 				c.loadStateStatus = LoadStateSucceeded
@@ -649,6 +649,11 @@ func (c *PTYConversation) loadStateLocked() error {
 		return xerrors.Errorf("failed to unmarshal state (corrupted or invalid JSON): %w", err)
 	}
 
+	// Validate version
+	if agentState.Version != 1 {
+		return xerrors.Errorf("unsupported state file version %d (expected 1)", agentState.Version)
+	}
+
 	// Handle initial prompt restoration:
 	// - If a new initial prompt was provided via flags, check if it differs from the saved one.
 	//   If different, mark as not sent (will be sent). If same, preserve sent status.
@@ -657,7 +662,7 @@ func (c *PTYConversation) loadStateLocked() error {
 	if len(c.cfg.InitialPrompt) > 0 {
 		isDifferent := buildStringFromMessageParts(c.cfg.InitialPrompt) != agentState.InitialPrompt
 		c.initialPromptSent = !isDifferent
-	} else {
+	} else if agentState.InitialPrompt != "" {
 		c.cfg.InitialPrompt = []MessagePart{MessagePartText{
 			Content: agentState.InitialPrompt,
 			Alias:   "",

lib/screentracker/pty_conversation_test.go

@@ -56,7 +56,7 @@ type testEmitter struct{}
 func (testEmitter) EmitMessages([]st.ConversationMessage) {}
 func (testEmitter) EmitStatus(st.ConversationStatus)      {}
 func (testEmitter) EmitScreen(string)                     {}
-func (testEmitter) EmitError(_ string, _ string)          {}
+func (testEmitter) EmitError(_ string, _ st.ErrorLevel)   {}
 
 // advanceFor is a shorthand for advanceUntil with a time-based condition.
 func advanceFor(ctx context.Context, t *testing.T, mClock *quartz.Mock, total time.Duration) {
@@ -798,6 +798,57 @@ func TestStatePersistence(t *testing.T) {
 		messages := c.Messages()
 		assert.Len(t, messages, 1)
 	})
+
+	t.Run("LoadState rejects unsupported version", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
+		t.Cleanup(cancel)
+
+		tmpDir := t.TempDir()
+		stateFile := tmpDir + "/unsupported_version.json"
+
+		// Create state file with unsupported version
+		unsupportedState := map[string]interface{}{
+			"version":             999, // Unsupported version
+			"messages":            []interface{}{},
+			"initial_prompt":      "",
+			"initial_prompt_sent": false,
+		}
+		stateBytes, err := json.Marshal(unsupportedState)
+		require.NoError(t, err)
+		err = os.WriteFile(stateFile, stateBytes, 0o644)
+		require.NoError(t, err)
+
+		mClock := quartz.NewMock(t)
+		agent := &testAgent{screen: "ready"}
+		cfg := st.PTYConversationConfig{
+			Clock:                 mClock,
+			SnapshotInterval:      100 * time.Millisecond,
+			ScreenStabilityLength: 200 * time.Millisecond,
+			AgentIO:               agent,
+			Logger:                slog.New(slog.NewTextHandler(io.Discard, nil)),
+			FormatMessage: func(message string, userInput string) string {
+				return message
+			},
+			ReadyForInitialPrompt: func(message string) bool {
+				return message == "ready"
+			},
+			StatePersistenceConfig: st.StatePersistenceConfig{
+				StateFile: stateFile,
+				LoadState: true,
+				SaveState: false,
+			},
+		}
+
+		// Should not panic - logs error and continues with empty state
+		c := st.NewPTY(ctx, cfg, &testEmitter{})
+		c.Start(ctx)
+
+		advanceFor(ctx, t, mClock, 300*time.Millisecond)
+
+		// Should have default initial message (version error causes fallback to empty state)
+		messages := c.Messages()
+		assert.Len(t, messages, 1)
+	})
 }
 
 func TestInitialPromptReadiness(t *testing.T) {
@@ -1286,4 +1337,63 @@ func TestInitialPromptSent(t *testing.T) {
 		}
 		assert.True(t, found, "saved prompt should be sent when no new prompt provided")
 	})
+
+	t.Run("empty prompt from state is not restored", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
+		t.Cleanup(cancel)
+
+		tmpDir := t.TempDir()
+		stateFile := tmpDir + "/state.json"
+
+		// Create state file with empty prompt
+		emptyPromptState := st.AgentState{
+			Version:           1,
+			Messages:          []st.ConversationMessage{},
+			InitialPrompt:     "", // Empty prompt
+			InitialPromptSent: false,
+		}
+		stateBytes, err := json.Marshal(emptyPromptState)
+		require.NoError(t, err)
+		err = os.WriteFile(stateFile, stateBytes, 0o644)
+		require.NoError(t, err)
+
+		mClock := quartz.NewMock(t)
+		agent := &testAgent{screen: "ready"}
+
+		cfg := st.PTYConversationConfig{
+			Clock:                 mClock,
+			SnapshotInterval:      100 * time.Millisecond,
+			ScreenStabilityLength: 200 * time.Millisecond,
+			AgentIO:               agent,
+			Logger:                discardLogger,
+			FormatMessage: func(message string, userInput string) string {
+				return message
+			},
+			ReadyForInitialPrompt: func(message string) bool {
+				return message == "ready"
+			},
+			StatePersistenceConfig: st.StatePersistenceConfig{
+				StateFile: stateFile,
+				LoadState: true,
+				SaveState: false,
+			},
+		}
+
+		c := st.NewPTY(ctx, cfg, &testEmitter{})
+		c.Start(ctx)
+
+		// Agent becomes ready
+		agent.setScreen("ready")
+
+		// Advance time to ensure any prompt would be sent
+		advanceFor(ctx, t, mClock, 500*time.Millisecond)
+
+		// Verify no prompt was sent (should only have the initial screen message)
+		messages := c.Messages()
+		for _, msg := range messages {
+			if msg.Role == st.ConversationRoleUser {
+				t.Errorf("Unexpected user message sent: %q (empty prompt should not be restored)", msg.Message)
+			}
+		}
+	})
 }

openapi.json

@@ -23,8 +23,8 @@
         "additionalProperties": false,
         "properties": {
           "level": {
-            "description": "Error level: 'warning' or 'error'",
-            "type": "string"
+            "$ref": "#/components/schemas/ErrorLevel",
+            "description": "Error level"
           },
           "message": {
             "description": "Error message",
@@ -60,6 +60,15 @@
         },
         "type": "object"
       },
+      "ErrorLevel": {
+        "enum": [
+          "error",
+          "warning"
+        ],
+        "example": "warning",
+        "title": "ErrorLevel",
+        "type": "string"
+      },
       "ErrorModel": {
         "additionalProperties": false,
         "properties": {