diff --git a/apps/backend/internal/orchestrator/session_launch.go b/apps/backend/internal/orchestrator/session_launch.go index 1815deb9a5..aea429fe75 100644 --- a/apps/backend/internal/orchestrator/session_launch.go +++ b/apps/backend/internal/orchestrator/session_launch.go @@ -27,20 +27,29 @@ const ( // LaunchSessionRequest is the unified request for session.launch. type LaunchSessionRequest struct { - TaskID string `json:"task_id"` - Intent SessionIntent `json:"intent,omitempty"` - SessionID string `json:"session_id,omitempty"` - AgentProfileID string `json:"agent_profile_id,omitempty"` - ExecutorID string `json:"executor_id,omitempty"` - ExecutorProfileID string `json:"executor_profile_id,omitempty"` - Prompt string `json:"prompt,omitempty"` - PlanMode bool `json:"plan_mode,omitempty"` - WorkflowStepID string `json:"workflow_step_id,omitempty"` - Priority string `json:"priority,omitempty"` - LaunchWorkspace bool `json:"launch_workspace,omitempty"` - SkipMessageRecord bool `json:"skip_message_record,omitempty"` - AutoStart bool `json:"auto_start,omitempty"` - Attachments []v1.MessageAttachment `json:"attachments,omitempty"` + TaskID string `json:"task_id"` + Intent SessionIntent `json:"intent,omitempty"` + SessionID string `json:"session_id,omitempty"` + AgentProfileID string `json:"agent_profile_id,omitempty"` + ExecutorID string `json:"executor_id,omitempty"` + ExecutorProfileID string `json:"executor_profile_id,omitempty"` + Prompt string `json:"prompt,omitempty"` + PlanMode bool `json:"plan_mode,omitempty"` + WorkflowStepID string `json:"workflow_step_id,omitempty"` + Priority string `json:"priority,omitempty"` + LaunchWorkspace bool `json:"launch_workspace,omitempty"` + SkipMessageRecord bool `json:"skip_message_record,omitempty"` + AutoStart bool `json:"auto_start,omitempty"` + // DeferredStart marks a prepare whose caller will follow up with an explicit + // IntentStartCreated that carries the prompt (the two-phase create flow: + // cheap sync prepare + async start). It suppresses the passthrough + // launchPrepare→launchStart upgrade so the eager launch doesn't spawn a + // promptless PTY and pre-empt the prompt-bearing start. It is an internal + // server-side coordination flag set by the deferred-start handlers, so it is + // kept off the wire protocol (`json:"-"`) — a client must not be able to + // suppress the upgrade and strand a passthrough session without a PTY. + DeferredStart bool `json:"-"` + Attachments []v1.MessageAttachment `json:"attachments,omitempty"` } // LaunchSessionResponse is the unified response for session.launch. @@ -105,8 +114,14 @@ func (s *Service) LaunchSession(ctx context.Context, req *LaunchSessionRequest) // AutoStart=true means we arrived here from launchStart's blocked-auto-start // downgrade path; skipping the upgrade in that case avoids a launchStart ↔ // launchPrepare bounce. +// +// DeferredStart=true means a prompt-bearing IntentStartCreated will follow this +// prepare (the two-phase create flow); skipping the upgrade there leaves the +// session CREATED so that follow-up start launches the passthrough agent WITH +// the prompt — eagerly launching here would spawn a promptless PTY and the +// later start would be rejected against the now-running session. func (s *Service) launchPrepare(ctx context.Context, req *LaunchSessionRequest) (*LaunchSessionResponse, error) { - if !req.AutoStart && s.isPassthroughProfile(ctx, req.AgentProfileID) { + if s.shouldUpgradePassthroughPrepare(ctx, req) { return s.launchStart(ctx, req) } sessionID, err := s.PrepareTaskSession( @@ -124,6 +139,16 @@ func (s *Service) launchPrepare(ctx context.Context, req *LaunchSessionRequest) }, nil } +// shouldUpgradePassthroughPrepare reports whether a prepare request for a +// passthrough profile should be eagerly upgraded to a full launch so a PTY +// exists for the terminal to attach to. It is the single decision point for the +// upgrade documented on launchPrepare: only genuine prepare-only callers (no +// imminent prompt-bearing start) get the eager launch. See launchPrepare for +// why AutoStart and DeferredStart each suppress it. +func (s *Service) shouldUpgradePassthroughPrepare(ctx context.Context, req *LaunchSessionRequest) bool { + return !req.AutoStart && !req.DeferredStart && s.isPassthroughProfile(ctx, req.AgentProfileID) +} + func (s *Service) isPassthroughProfile(ctx context.Context, profileID string) bool { if profileID == "" || s.agentManager == nil { return false diff --git a/apps/backend/internal/orchestrator/session_launch_test.go b/apps/backend/internal/orchestrator/session_launch_test.go index 91eae9d973..dfa7b2a6da 100644 --- a/apps/backend/internal/orchestrator/session_launch_test.go +++ b/apps/backend/internal/orchestrator/session_launch_test.go @@ -295,6 +295,63 @@ func TestIsPassthroughProfile(t *testing.T) { }) } +// TestShouldUpgradePassthroughPrepare pins the launchPrepare upgrade decision. +// +// Regression guard for the prompt-delivery break introduced by the passthrough +// upgrade (PR #744): the two-phase create flow does a cheap prompt-less prepare +// followed by an async IntentStartCreated that carries the prompt. If a +// passthrough prepare is eagerly upgraded to a full launch there, the PTY spawns +// with an empty prompt and the prompt-bearing start is rejected against the +// now-running session — so the agent sits at an empty prompt. DeferredStart must +// suppress the upgrade so that follow-up start launches the passthrough WITH the +// prompt, exactly as ACP already does. +func TestShouldUpgradePassthroughPrepare(t *testing.T) { + repo := setupTestRepo(t) + passthrough := &mockAgentManager{isPassthrough: true} + regular := &mockAgentManager{isPassthrough: false} + + tests := []struct { + name string + mgr *mockAgentManager + req LaunchSessionRequest + want bool + }{ + { + name: "prepare-only passthrough upgrades (quick-chat terminal needs a PTY)", + mgr: passthrough, + req: LaunchSessionRequest{AgentProfileID: "profile1"}, + want: true, + }, + { + name: "deferred-start passthrough does NOT upgrade (prompt-bearing start follows)", + mgr: passthrough, + req: LaunchSessionRequest{AgentProfileID: "profile1", DeferredStart: true}, + want: false, + }, + { + name: "auto-start passthrough does NOT upgrade (avoids launchStart bounce)", + mgr: passthrough, + req: LaunchSessionRequest{AgentProfileID: "profile1", AutoStart: true}, + want: false, + }, + { + name: "non-passthrough never upgrades", + mgr: regular, + req: LaunchSessionRequest{AgentProfileID: "profile1"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), tt.mgr) + if got := svc.shouldUpgradePassthroughPrepare(context.Background(), &tt.req); got != tt.want { + t.Errorf("shouldUpgradePassthroughPrepare()=%v, want %v", got, tt.want) + } + }) + } +} + // TestLaunchPrepare_PassthroughDoesNotRecurse guards against an infinite // launchStart ↔ launchPrepare bounce when a passthrough profile is combined // with AutoStart=true and a step that blocks auto-start. diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index 89d6beb87b..e4c171399d 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -777,6 +777,10 @@ func (h *TaskHandlers) handlePostCreateTaskSession( return } if body.PrepareSession && !body.StartAgent { + // Prepare-only: no follow-up start is coming, so DeferredStart is + // intentionally omitted — a passthrough profile should be eagerly + // upgraded to a full launch here so the terminal has a PTY to attach to. + // (Contrast startAgentForNewTask below, which sets DeferredStart=true.) resp, err := h.orchestrator.LaunchSession(c.Request.Context(), &orchestrator.LaunchSessionRequest{ TaskID: taskID, Intent: orchestrator.IntentPrepare, @@ -816,6 +820,10 @@ func (h *TaskHandlers) startAgentForNewTask( ExecutorID: body.ExecutorID, ExecutorProfileID: body.ExecutorProfileID, WorkflowStepID: resolvedStepID, + // The async IntentStartCreated below carries the prompt. Mark this as a + // deferred start so a passthrough profile is not eagerly launched here + // with an empty prompt (which would pre-empt that prompt-bearing start). + DeferredStart: true, }) if err != nil { h.logger.Error("failed to prepare session for task", zap.Error(err), zap.String("task_id", taskID)) @@ -1314,6 +1322,12 @@ func (h *TaskHandlers) httpStartConfigChat(c *gin.Context) { Intent: orchestrator.IntentPrepare, AgentProfileID: agentProfileID, ExecutorID: executorID, + // When a prompt is present, launchConfigChatAgent below follows with a + // prompt-bearing IntentStartCreated. Defer the start so a passthrough + // profile isn't eagerly launched here with an empty prompt. With no + // prompt there is no follow-up, so keep the eager upgrade that gives the + // terminal a PTY to attach to. + DeferredStart: body.Prompt != "", }) if err != nil { h.deleteTaskOnError(task.ID, "config chat", err) diff --git a/apps/backend/internal/task/handlers/task_http_handlers_test.go b/apps/backend/internal/task/handlers/task_http_handlers_test.go index 63b293090b..0be3deccf2 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers_test.go +++ b/apps/backend/internal/task/handlers/task_http_handlers_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "testing" @@ -15,10 +16,123 @@ import ( "github.com/stretchr/testify/require" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/orchestrator" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/service" ) +// captureOrchestrator records every LaunchSession request so tests can assert +// on the fields the handler set. prepErr, when non-nil, is returned from +// LaunchSession to short-circuit the two-phase create flow before its async +// start goroutine spawns (keeping assertions race-free). +type captureOrchestrator struct { + mu sync.Mutex + requests []*orchestrator.LaunchSessionRequest + prepErr error +} + +func (m *captureOrchestrator) LaunchSession(_ context.Context, req *orchestrator.LaunchSessionRequest) (*orchestrator.LaunchSessionResponse, error) { + m.mu.Lock() + m.requests = append(m.requests, req) + m.mu.Unlock() + if m.prepErr != nil { + return nil, m.prepErr + } + return &orchestrator.LaunchSessionResponse{SessionID: "sess-1"}, nil +} + +func (m *captureOrchestrator) EnsureSession(_ context.Context, _ string, _ ...orchestrator.EnsureSessionOptions) (*orchestrator.EnsureSessionResponse, error) { + return nil, nil +} + +// TestStartAgentForNewTask_SetsDeferredStart pins the call-site half of the +// passthrough start_agent prompt-delivery fix: the synchronous prepare must +// carry DeferredStart=true so launchPrepare does not eagerly upgrade a +// passthrough profile into a promptless PTY launch and pre-empt the +// prompt-bearing IntentStartCreated that follows. Returning an error from the +// prepare call keeps the async start goroutine from spawning, so the assertion +// reads orch.requests without racing it. +func TestStartAgentForNewTask_SetsDeferredStart(t *testing.T) { + orch := &captureOrchestrator{prepErr: errors.New("prepare failed")} + h := &TaskHandlers{orchestrator: orch, logger: newTestLogger(t)} + + resp := &createTaskResponse{} + body := httpCreateTaskRequest{StartAgent: true, AgentProfileID: "profile-1"} + h.startAgentForNewTask(context.Background(), resp, "task-1", "do the thing", body, "step-1") + + orch.mu.Lock() + defer orch.mu.Unlock() + require.Len(t, orch.requests, 1, "prepare error must short-circuit before the async start goroutine") + prep := orch.requests[0] + assert.Equal(t, orchestrator.IntentPrepare, prep.Intent) + assert.True(t, prep.DeferredStart, + "sync prepare must defer the start so the passthrough PTY is launched with the prompt by the follow-up IntentStartCreated") +} + +// configChatRepo returns a non-nil workspace so resolveConfigChatDefaults does +// not nil-deref; everything else inherits mockRepository's no-op stubs. +type configChatRepo struct { + mockRepository +} + +func (r *configChatRepo) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + return &models.Workspace{ID: id}, nil +} + +// TestHttpStartConfigChat_SetsDeferredStart pins the second call site of the +// passthrough prompt-delivery fix. Unlike start_agent (always deferred), config +// chat defers the start only when a prompt is present — with no prompt there is +// no follow-up start, so the passthrough upgrade must stay on to give the +// terminal a PTY. Both branches are load-bearing, so both are pinned. The prepare +// LaunchSession is invoked synchronously before the async launchConfigChatAgent +// goroutine spawns, so requests[0] is the prepare and is read under the mock's +// mutex — race-free without asserting the (timing-dependent) total count. +func TestHttpStartConfigChat_SetsDeferredStart(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + + cases := []struct { + name string + prompt string + wantDeferredStart bool + }{ + {name: "prompt present defers the start", prompt: "configure my workflow", wantDeferredStart: true}, + {name: "no prompt keeps the eager PTY upgrade", prompt: "", wantDeferredStart: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + repo := &configChatRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + orch := &captureOrchestrator{} + h := &TaskHandlers{service: svc, orchestrator: orch, logger: log} + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + body := `{"agent_profile_id":"cfg-profile","prompt":` + strconv.Quote(tc.prompt) + `}` + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces/ws-1/config-chat", strings.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + h.httpStartConfigChat(c) + + orch.mu.Lock() + defer orch.mu.Unlock() + require.GreaterOrEqual(t, len(orch.requests), 1, "the synchronous prepare must have been issued") + prep := orch.requests[0] + assert.Equal(t, orchestrator.IntentPrepare, prep.Intent, "first call is the synchronous prepare") + assert.Equal(t, tc.wantDeferredStart, prep.DeferredStart, + "config-chat prepare must defer the start iff a prompt will be delivered by the follow-up IntentStartCreated") + }) + } +} + type captureCreateTaskRepo struct { mockRepository captured *models.Task