fix: pause on clarification timeout (#1580)
This commit is contained in:
@@ -1079,6 +1079,7 @@ func registerMCPAndDebugRoutes(
|
||||
)
|
||||
// Wire config-mode dependencies for agent-native configuration
|
||||
mcpHandlers.SetConfigDeps(p.services.Workflow, p.agentSettingsController, p.mcpConfigSvc)
|
||||
mcpHandlers.SetClarificationInputPauser(p.orchestratorSvc)
|
||||
|
||||
// Enrich list_tasks responses with associated GitHub PRs (link, title,
|
||||
// number, state) when the github service is available.
|
||||
|
||||
@@ -42,6 +42,14 @@ func NewStore(timeout time.Duration) *Store {
|
||||
}
|
||||
}
|
||||
|
||||
// SetOnWaitEntered installs a test hook invoked when WaitForResponse starts
|
||||
// waiting on a pending clarification.
|
||||
func (s *Store) SetOnWaitEntered(fn func(pendingID string)) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
s.onWaitEntered = fn
|
||||
}
|
||||
|
||||
// CreateRequest creates a new clarification request and returns its pending ID
|
||||
// plus a boolean indicating whether a new entry was created (true) or an
|
||||
// existing one was reused (false). If a pending entry for the same session
|
||||
|
||||
@@ -0,0 +1,144 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/clarification"
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
"github.com/kandev/kandev/internal/task/service"
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
type recordingClarificationInputPauser struct {
|
||||
sessions []string
|
||||
count int
|
||||
err error
|
||||
}
|
||||
|
||||
func (p *recordingClarificationInputPauser) PauseForClarificationInput(_ context.Context, sessionID string) (int, error) {
|
||||
p.sessions = append(p.sessions, sessionID)
|
||||
return p.count, p.err
|
||||
}
|
||||
|
||||
type recordingSessionCanceller struct {
|
||||
sessions []string
|
||||
count int
|
||||
}
|
||||
|
||||
func (c *recordingSessionCanceller) DetachSessionAndNotify(_ context.Context, sessionID string) int {
|
||||
c.sessions = append(c.sessions, sessionID)
|
||||
return c.count
|
||||
}
|
||||
|
||||
func TestHandleClarificationTimeout_UsesHardPauser(t *testing.T) {
|
||||
pauser := &recordingClarificationInputPauser{count: 2}
|
||||
h := &Handlers{logger: testLogger(t).WithFields()}
|
||||
h.SetClarificationInputPauser(pauser)
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPClarificationTimeout, map[string]interface{}{"session_id": "s1"})
|
||||
resp, err := h.handleClarificationTimeout(context.Background(), msg)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, ws.MessageTypeResponse, resp.Type)
|
||||
require.Equal(t, []string{"s1"}, pauser.sessions)
|
||||
var payload map[string]interface{}
|
||||
require.NoError(t, json.Unmarshal(resp.Payload, &payload))
|
||||
require.Equal(t, true, payload["ok"])
|
||||
require.Equal(t, true, payload["paused"])
|
||||
require.Equal(t, float64(2), payload["cancelled"])
|
||||
}
|
||||
|
||||
func TestHandleClarificationTimeout_FallsBackWhenHardPauseFails(t *testing.T) {
|
||||
pauser := &recordingClarificationInputPauser{err: errors.New("db unavailable")}
|
||||
canceller := &recordingSessionCanceller{count: 3}
|
||||
h := &Handlers{logger: testLogger(t).WithFields(), sessionCanceller: canceller}
|
||||
h.SetClarificationInputPauser(pauser)
|
||||
|
||||
msg := makeWSMessage(t, ws.ActionMCPClarificationTimeout, map[string]interface{}{"session_id": "s1"})
|
||||
resp, err := h.handleClarificationTimeout(context.Background(), msg)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, ws.MessageTypeResponse, resp.Type)
|
||||
require.Equal(t, []string{"s1"}, pauser.sessions)
|
||||
require.Equal(t, []string{"s1"}, canceller.sessions)
|
||||
|
||||
var payload map[string]interface{}
|
||||
require.NoError(t, json.Unmarshal(resp.Payload, &payload))
|
||||
require.Equal(t, true, payload["ok"])
|
||||
require.Equal(t, false, payload["paused"])
|
||||
require.Equal(t, float64(3), payload["cancelled"])
|
||||
require.NotContains(t, string(resp.Payload), "db unavailable")
|
||||
}
|
||||
|
||||
func TestHandleAskUserQuestion_NoAnswerPausesSession(t *testing.T) {
|
||||
svc, repo := newTestTaskService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Test"}))
|
||||
require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "Board"}))
|
||||
task, err := svc.CreateTask(ctx, &service.CreateTaskRequest{
|
||||
WorkspaceID: "ws-1",
|
||||
WorkflowID: "wf-1",
|
||||
Title: "Task",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
sess := &models.TaskSession{
|
||||
ID: "sess-no-answer",
|
||||
TaskID: task.ID,
|
||||
IsPrimary: true,
|
||||
State: models.TaskSessionStateRunning,
|
||||
}
|
||||
require.NoError(t, repo.CreateTaskSession(ctx, sess))
|
||||
|
||||
store := clarification.NewStore(time.Minute)
|
||||
waitEntered := make(chan struct{}, 1)
|
||||
store.SetOnWaitEntered(func(_ string) {
|
||||
select {
|
||||
case waitEntered <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
})
|
||||
pauser := &recordingClarificationInputPauser{}
|
||||
h := NewHandlers(svc, nil, store, nil, nil, repo, repo, nil, nil, nil, nil, testLogger(t))
|
||||
h.SetClarificationInputPauser(pauser)
|
||||
|
||||
payload := map[string]interface{}{
|
||||
"session_id": sess.ID,
|
||||
"task_id": task.ID,
|
||||
"questions": []map[string]interface{}{
|
||||
{"prompt": "What colour?", "options": []map[string]interface{}{
|
||||
{"label": "Red", "description": "R"},
|
||||
{"label": "Blue", "description": "B"},
|
||||
}},
|
||||
},
|
||||
}
|
||||
waitCtx, cancel := context.WithCancel(ctx)
|
||||
defer cancel()
|
||||
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
defer close(done)
|
||||
msg := makeWSMessage(t, ws.ActionMCPAskUserQuestion, payload)
|
||||
if _, err := h.handleAskUserQuestion(waitCtx, msg); err != nil {
|
||||
t.Errorf("handleAskUserQuestion returned unexpected error: %v", err)
|
||||
}
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-waitEntered:
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("timed out waiting for clarification request to register")
|
||||
}
|
||||
cancel()
|
||||
|
||||
select {
|
||||
case <-done:
|
||||
case <-time.After(time.Second):
|
||||
t.Fatal("timed out waiting for ask_user_question handler")
|
||||
}
|
||||
require.Equal(t, []string{sess.ID}, pauser.sessions)
|
||||
}
|
||||
@@ -46,6 +46,13 @@ type SessionCanceller interface {
|
||||
DetachSessionAndNotify(ctx context.Context, sessionID string) int
|
||||
}
|
||||
|
||||
// ClarificationInputPauser performs the orchestrator-owned hard pause for
|
||||
// ask_user_question calls that end without an answer. The returned int is the
|
||||
// number of clarification bundles detached while pausing.
|
||||
type ClarificationInputPauser interface {
|
||||
PauseForClarificationInput(ctx context.Context, sessionID string) (int, error)
|
||||
}
|
||||
|
||||
// MessageCreator creates messages for clarification requests.
|
||||
type MessageCreator interface {
|
||||
CreateClarificationRequestMessages(ctx context.Context, taskID, sessionID, pendingID string, questions []clarification.Question, clarificationContext string) ([]string, error)
|
||||
@@ -98,6 +105,7 @@ type Handlers struct {
|
||||
workflowCtrl *workflowctrl.Controller
|
||||
clarificationSvc ClarificationService
|
||||
sessionCanceller SessionCanceller
|
||||
inputPauser ClarificationInputPauser
|
||||
messageCreator MessageCreator
|
||||
sessionRepo SessionRepository
|
||||
taskRepo TaskRepository
|
||||
@@ -153,6 +161,12 @@ func NewHandlers(
|
||||
}
|
||||
}
|
||||
|
||||
// SetClarificationInputPauser wires the orchestrator-owned hard pause used when
|
||||
// a clarification tool call ends without delivering an answer to the agent.
|
||||
func (h *Handlers) SetClarificationInputPauser(pauser ClarificationInputPauser) {
|
||||
h.inputPauser = pauser
|
||||
}
|
||||
|
||||
// SetConfigDeps sets the config-mode dependencies for agent-native configuration handlers.
|
||||
// These are optional and only needed when config-mode MCP sessions are used.
|
||||
func (h *Handlers) SetConfigDeps(
|
||||
@@ -2275,6 +2289,14 @@ func (h *Handlers) handleAskUserQuestion(ctx context.Context, msg *ws.Message) (
|
||||
// fallback in the orchestrator handles resuming with a new turn.
|
||||
resp, err := h.clarificationSvc.WaitForResponse(ctx, pendingID)
|
||||
if err != nil {
|
||||
if h.inputPauser != nil {
|
||||
if _, pauseErr := h.inputPauser.PauseForClarificationInput(context.WithoutCancel(ctx), req.SessionID); pauseErr != nil {
|
||||
h.logger.Warn("failed to pause session after clarification ended without answer",
|
||||
zap.String("pending_id", pendingID),
|
||||
zap.String("session_id", req.SessionID),
|
||||
zap.Error(pauseErr))
|
||||
}
|
||||
}
|
||||
h.logger.Warn("clarification wait ended without response",
|
||||
zap.String("pending_id", pendingID),
|
||||
zap.String("session_id", req.SessionID),
|
||||
@@ -2518,6 +2540,28 @@ func (h *Handlers) handleClarificationTimeout(ctx context.Context, msg *ws.Messa
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "session_id is required", nil)
|
||||
}
|
||||
|
||||
if h.inputPauser != nil {
|
||||
cancelled, err := h.inputPauser.PauseForClarificationInput(context.WithoutCancel(ctx), req.SessionID)
|
||||
if err != nil {
|
||||
h.logger.Warn("failed to pause session after clarification timeout",
|
||||
zap.String("session_id", req.SessionID),
|
||||
zap.Error(err))
|
||||
if h.sessionCanceller == nil {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError,
|
||||
"failed to pause session for clarification input", nil)
|
||||
}
|
||||
cancelled = h.sessionCanceller.DetachSessionAndNotify(context.WithoutCancel(ctx), req.SessionID)
|
||||
h.logger.Info("detached clarification waiters after pause failure",
|
||||
zap.String("session_id", req.SessionID),
|
||||
zap.Int("count", cancelled))
|
||||
return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"ok": true, "cancelled": cancelled, "paused": false})
|
||||
}
|
||||
h.logger.Info("paused session after agent MCP clarification timeout",
|
||||
zap.String("session_id", req.SessionID),
|
||||
zap.Int("count", cancelled))
|
||||
return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"ok": true, "cancelled": cancelled, "paused": true})
|
||||
}
|
||||
|
||||
if h.sessionCanceller == nil {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "sessionCanceller is required", nil)
|
||||
}
|
||||
|
||||
@@ -4,6 +4,8 @@ import (
|
||||
"context"
|
||||
|
||||
"go.uber.org/zap"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
// sessionHasPendingClarification reports whether the session still has durable
|
||||
@@ -22,3 +24,23 @@ func (s *Service) sessionHasPendingClarification(ctx context.Context, sessionID
|
||||
}
|
||||
return len(msgs) > 0
|
||||
}
|
||||
|
||||
// turnCompleteBlockedByUserInput reports and applies the workflow barrier for
|
||||
// durable user-input waits. A pending clarification is a platform pause, not a
|
||||
// step-completion signal, so all turn-complete transition entrypoints fail
|
||||
// closed while it exists.
|
||||
func (s *Service) turnCompleteBlockedByUserInput(ctx context.Context, taskID, sessionID string, session *models.TaskSession) bool {
|
||||
if !s.sessionHasPendingClarification(ctx, sessionID) {
|
||||
return false
|
||||
}
|
||||
s.logger.Info("deferring on_turn_complete while clarification is pending",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID))
|
||||
if session != nil {
|
||||
if _, has := models.LoadPendingStepSignal(session.Metadata); has {
|
||||
s.clearPendingStepSignal(ctx, session)
|
||||
}
|
||||
}
|
||||
s.setSessionWaitingForInput(ctx, taskID, sessionID, session)
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -39,3 +39,33 @@ func TestSessionHasPendingClarification(t *testing.T) {
|
||||
t.Fatal("expected pending clarification")
|
||||
}
|
||||
}
|
||||
|
||||
func seedPendingClarificationMessage(t *testing.T, repo interface {
|
||||
CreateTurn(ctx context.Context, turn *models.Turn) error
|
||||
CreateMessage(ctx context.Context, message *models.Message) error
|
||||
}, taskID, sessionID string) {
|
||||
t.Helper()
|
||||
now := time.Now().UTC()
|
||||
turnID := "turn-clarification-" + sessionID
|
||||
requireNoError(t, repo.CreateTurn(context.Background(), &models.Turn{
|
||||
ID: turnID,
|
||||
TaskSessionID: sessionID,
|
||||
TaskID: taskID,
|
||||
StartedAt: now,
|
||||
}))
|
||||
requireNoError(t, repo.CreateMessage(context.Background(), &models.Message{
|
||||
ID: "clarification-" + sessionID,
|
||||
TaskSessionID: sessionID,
|
||||
TaskID: taskID,
|
||||
TurnID: turnID,
|
||||
AuthorType: models.MessageAuthorAgent,
|
||||
Type: "clarification_request",
|
||||
Content: "Which approach?",
|
||||
CreatedAt: now,
|
||||
Metadata: map[string]interface{}{
|
||||
"pending_id": "pending-" + sessionID,
|
||||
"question_id": "q1",
|
||||
"status": "pending",
|
||||
},
|
||||
}))
|
||||
}
|
||||
|
||||
@@ -16,6 +16,8 @@ import (
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
const clarificationInputPauseTimeout = 30 * time.Second
|
||||
|
||||
// subscribeClarificationEvents subscribes to clarification-related events.
|
||||
func (s *Service) subscribeClarificationEvents() {
|
||||
if s.eventBus == nil {
|
||||
@@ -283,6 +285,50 @@ func (s *Service) retryClarificationAfterCancel(ctx context.Context, data clarif
|
||||
return true
|
||||
}
|
||||
|
||||
// PauseForClarificationInput converts a no-answer ask_user_question outcome
|
||||
// into a platform pause. It detaches the pending clarification so a late user
|
||||
// answer resumes through the event fallback path, then silently cancels the
|
||||
// active agent turn without evaluating workflow turn-complete actions. It
|
||||
// returns the number of clarification bundles detached.
|
||||
func (s *Service) PauseForClarificationInput(ctx context.Context, sessionID string) (int, error) {
|
||||
if sessionID == "" {
|
||||
return 0, fmt.Errorf("session_id is required")
|
||||
}
|
||||
writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), clarificationInputPauseTimeout)
|
||||
defer cancel()
|
||||
session, err := s.repo.GetTaskSession(writeCtx, sessionID)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("load session for clarification pause: %w", err)
|
||||
}
|
||||
if session == nil {
|
||||
return 0, nil
|
||||
}
|
||||
|
||||
hasPendingClarification := s.sessionHasPendingClarification(writeCtx, sessionID)
|
||||
detached := 0
|
||||
if s.clarificationCanceller != nil {
|
||||
detached = s.clarificationCanceller.DetachSessionAndNotify(writeCtx, sessionID)
|
||||
}
|
||||
if isTerminalSessionState(session.State) {
|
||||
return detached, nil
|
||||
}
|
||||
if !hasPendingClarification && detached == 0 {
|
||||
return detached, nil
|
||||
}
|
||||
if _, has := models.LoadPendingStepSignal(session.Metadata); has {
|
||||
s.clearPendingStepSignal(writeCtx, session)
|
||||
}
|
||||
|
||||
// The backend wait path and agentctl timeout notification can race for the
|
||||
// same ask_user_question call. A duplicate cancel is safe: lifecycle returns
|
||||
// ErrCancelEscalated/ErrNoExecutionForSession once the first pause wins, and
|
||||
// completeTurnForSession is idempotent when there is no active turn left.
|
||||
if err := s.cancelAgentSilent(writeCtx, session.TaskID, sessionID); err != nil {
|
||||
return detached, err
|
||||
}
|
||||
return detached, nil
|
||||
}
|
||||
|
||||
// cancelAgentSilent cancels the agent turn without creating a visible message
|
||||
// in the chat. Used by clarification recovery so the cancel-and-retry is seamless.
|
||||
//
|
||||
@@ -290,24 +336,38 @@ func (s *Service) retryClarificationAfterCancel(ctx context.Context, data clarif
|
||||
// (agent crashed mid-turn). In that case, skip the cancel signal but still reconcile the
|
||||
// session's state so clarification recovery can proceed with a fresh prompt.
|
||||
func (s *Service) cancelAgentSilent(ctx context.Context, taskID, sessionID string) error {
|
||||
if err := s.agentManager.CancelAgent(ctx, sessionID); err != nil {
|
||||
if !errors.Is(err, lifecycle.ErrNoExecutionForSession) {
|
||||
if s.agentManager == nil {
|
||||
s.logger.Debug("skipping silent clarification cancel because agent manager is not configured",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID))
|
||||
} else if err := s.agentManager.CancelAgent(ctx, sessionID); err != nil {
|
||||
if !errors.Is(err, lifecycle.ErrNoExecutionForSession) && !errors.Is(err, lifecycle.ErrCancelEscalated) {
|
||||
return fmt.Errorf("cancel agent: %w", err)
|
||||
}
|
||||
// Agent crashed or exited mid-turn — clarification recovery cannot signal a cancel,
|
||||
// but we still reconcile state below so a fresh prompt can run. Error level so this
|
||||
// surfaces for root-cause investigation of the crash.
|
||||
s.logger.Error("agent process appears to have crashed during clarification recovery",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID),
|
||||
zap.Error(err))
|
||||
s.logSilentCancelReconciled(taskID, sessionID, err)
|
||||
}
|
||||
|
||||
s.updateTaskSessionState(ctx, taskID, sessionID, models.TaskSessionStateWaitingForInput, "", true, nil)
|
||||
s.completeTurnForSession(ctx, sessionID)
|
||||
return nil
|
||||
}
|
||||
|
||||
func (s *Service) logSilentCancelReconciled(taskID, sessionID string, err error) {
|
||||
if errors.Is(err, lifecycle.ErrCancelEscalated) {
|
||||
s.logger.Warn("agent did not acknowledge silent clarification cancel; reconciling session state",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID),
|
||||
zap.Error(err))
|
||||
return
|
||||
}
|
||||
// Agent crashed or exited mid-turn — clarification recovery cannot signal a cancel,
|
||||
// but we still reconcile state below so a fresh prompt can run. Error level so this
|
||||
// surfaces for root-cause investigation of the crash.
|
||||
s.logger.Error("agent process appears to have crashed during clarification recovery",
|
||||
zap.String("task_id", taskID),
|
||||
zap.String("session_id", sessionID),
|
||||
zap.Error(err))
|
||||
}
|
||||
|
||||
func (s *Service) cancelClarificationWatchdogsForSession(sessionID, reason string) {
|
||||
if sessionID == "" {
|
||||
return
|
||||
|
||||
@@ -2,6 +2,8 @@ package orchestrator
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"errors"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -13,6 +15,15 @@ import (
|
||||
v1 "github.com/kandev/kandev/pkg/api/v1"
|
||||
)
|
||||
|
||||
type zeroClarificationCanceller struct {
|
||||
sessions []string
|
||||
}
|
||||
|
||||
func (c *zeroClarificationCanceller) DetachSessionAndNotify(_ context.Context, sessionID string) int {
|
||||
c.sessions = append(c.sessions, sessionID)
|
||||
return 0
|
||||
}
|
||||
|
||||
func TestHandleClarificationAnswered(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
@@ -272,6 +283,192 @@ func TestHandleClarificationStaleDismissed(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestPauseForClarificationInput_SilentlyCancelsTurnWithoutWorkflowTransition(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedExecutorRunning(t, repo, "s1", "t1", "exec-1")
|
||||
seedPendingClarificationMessage(t, repo, "t1", "s1")
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step1"] = &wfmodels.WorkflowStep{
|
||||
ID: "step1", WorkflowID: "wf1", Name: "Plan", Position: 0,
|
||||
Events: wfmodels.StepEvents{
|
||||
OnTurnComplete: []wfmodels.OnTurnCompleteAction{
|
||||
{Type: wfmodels.OnTurnCompleteMoveToNext},
|
||||
},
|
||||
},
|
||||
}
|
||||
stepGetter.steps["step2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step2", WorkflowID: "wf1", Name: "Implement", Position: 1,
|
||||
}
|
||||
|
||||
agentMgr := &mockAgentManager{repoForExecutionLookup: repo}
|
||||
canceller := &recordingClarificationCanceller{}
|
||||
svc := createEngineService(t, repo, stepGetter, agentMgr)
|
||||
svc.SetClarificationCanceller(canceller)
|
||||
svc.turnService = &repoBackedTurnService{repo: repo}
|
||||
|
||||
detached, err := svc.PauseForClarificationInput(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("pause clarification input: %v", err)
|
||||
}
|
||||
if detached != 1 {
|
||||
t.Fatalf("expected one detached clarification bundle, got %d", detached)
|
||||
}
|
||||
|
||||
if got := agentMgr.cancelAgentCalls.Load(); got != 1 {
|
||||
t.Fatalf("expected silent cancel call, got %d", got)
|
||||
}
|
||||
if len(canceller.sessions) == 0 || canceller.sessions[0] != "s1" {
|
||||
t.Fatalf("expected clarification detach for s1, got %#v", canceller.sessions)
|
||||
}
|
||||
task, err := repo.GetTask(ctx, "t1")
|
||||
if err != nil {
|
||||
t.Fatalf("get task: %v", err)
|
||||
}
|
||||
if task.WorkflowStepID != "step1" {
|
||||
t.Fatalf("timeout pause must not run on_turn_complete; got step %q", task.WorkflowStepID)
|
||||
}
|
||||
session, err := repo.GetTaskSession(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("get session: %v", err)
|
||||
}
|
||||
if session.State != models.TaskSessionStateWaitingForInput {
|
||||
t.Fatalf("expected session waiting for input, got %q", session.State)
|
||||
}
|
||||
if turn, err := repo.GetActiveTurnBySessionID(ctx, "s1"); err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
t.Fatalf("get active turn: %v", err)
|
||||
} else if turn != nil {
|
||||
t.Fatalf("expected active turn to be completed, got %#v", turn)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPauseForClarificationInput_CancelsWhileSessionAlreadyWaiting(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedExecutorRunning(t, repo, "s1", "t1", "exec-1")
|
||||
seedPendingClarificationMessage(t, repo, "t1", "s1")
|
||||
if err := repo.UpdateTaskSessionState(ctx, "s1", models.TaskSessionStateWaitingForInput, ""); err != nil {
|
||||
t.Fatalf("set waiting state: %v", err)
|
||||
}
|
||||
|
||||
agentMgr := &mockAgentManager{repoForExecutionLookup: repo}
|
||||
canceller := &zeroClarificationCanceller{}
|
||||
svc := createEngineService(t, repo, newMockStepGetter(), agentMgr)
|
||||
svc.SetClarificationCanceller(canceller)
|
||||
svc.turnService = &repoBackedTurnService{repo: repo}
|
||||
|
||||
detached, err := svc.PauseForClarificationInput(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("pause clarification input: %v", err)
|
||||
}
|
||||
if detached != 0 {
|
||||
t.Fatalf("expected zero detached clarification bundles from zero canceller, got %d", detached)
|
||||
}
|
||||
if len(canceller.sessions) != 1 || canceller.sessions[0] != "s1" {
|
||||
t.Fatalf("expected clarification detach for s1, got %#v", canceller.sessions)
|
||||
}
|
||||
if got := agentMgr.cancelAgentCalls.Load(); got != 1 {
|
||||
t.Fatalf("waiting ask session must still cancel active agent, got %d calls", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPauseForClarificationInput_IgnoresStaleTimeoutWithoutPendingClarification(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedExecutorRunning(t, repo, "s1", "t1", "exec-1")
|
||||
if err := repo.SetSessionMetadataKey(ctx, "s1", models.SessionMetaKeyPendingStepCompletion, models.PendingStepCompletionSignal{
|
||||
StepID: "step1",
|
||||
Source: "agent",
|
||||
Summary: "ready",
|
||||
SignaledAt: time.Now().UTC(),
|
||||
}); err != nil {
|
||||
t.Fatalf("seed pending step signal: %v", err)
|
||||
}
|
||||
|
||||
agentMgr := &mockAgentManager{repoForExecutionLookup: repo}
|
||||
canceller := &zeroClarificationCanceller{}
|
||||
svc := createEngineService(t, repo, newMockStepGetter(), agentMgr)
|
||||
svc.SetClarificationCanceller(canceller)
|
||||
svc.turnService = &repoBackedTurnService{repo: repo}
|
||||
|
||||
detached, err := svc.PauseForClarificationInput(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("pause stale clarification input: %v", err)
|
||||
}
|
||||
if detached != 0 {
|
||||
t.Fatalf("expected no detached clarifications, got %d", detached)
|
||||
}
|
||||
if len(canceller.sessions) != 1 || canceller.sessions[0] != "s1" {
|
||||
t.Fatalf("expected stale timeout to probe clarification detach for s1, got %#v", canceller.sessions)
|
||||
}
|
||||
if got := agentMgr.cancelAgentCalls.Load(); got != 0 {
|
||||
t.Fatalf("stale timeout must not cancel a later turn, got %d calls", got)
|
||||
}
|
||||
session, err := repo.GetTaskSession(ctx, "s1")
|
||||
if err != nil {
|
||||
t.Fatalf("get session: %v", err)
|
||||
}
|
||||
if _, has := models.LoadPendingStepSignal(session.Metadata); !has {
|
||||
t.Fatal("stale timeout must not clear pending step signal from later turn")
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandleClarificationAnswered_SkipsOnTurnStart(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedExecutorRunning(t, repo, "s1", "t1", "exec-1")
|
||||
if err := repo.UpdateTaskSessionState(ctx, "s1", models.TaskSessionStateWaitingForInput, ""); err != nil {
|
||||
t.Fatalf("set session waiting: %v", err)
|
||||
}
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step1"] = &wfmodels.WorkflowStep{
|
||||
ID: "step1", WorkflowID: "wf1", Name: "Plan", Position: 0,
|
||||
Events: wfmodels.StepEvents{
|
||||
OnTurnStart: []wfmodels.OnTurnStartAction{
|
||||
{Type: wfmodels.OnTurnStartMoveToNext},
|
||||
},
|
||||
},
|
||||
}
|
||||
stepGetter.steps["step2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step2", WorkflowID: "wf1", Name: "Implement", Position: 1,
|
||||
}
|
||||
|
||||
agentMgr := &mockAgentManager{isAgentRunning: true, repoForExecutionLookup: repo}
|
||||
svc := createEngineService(t, repo, stepGetter, agentMgr)
|
||||
event := bus.NewEvent("clarification.answered", "test", map[string]any{
|
||||
"session_id": "s1",
|
||||
"task_id": "t1",
|
||||
"pending_id": "pending-1",
|
||||
"question": "Which database?",
|
||||
"answer_text": "User selected: PostgreSQL",
|
||||
"rejected": false,
|
||||
})
|
||||
|
||||
if err := svc.handleClarificationAnswered(ctx, event); err != nil {
|
||||
t.Fatalf("handle clarification answered: %v", err)
|
||||
}
|
||||
|
||||
task, err := repo.GetTask(ctx, "t1")
|
||||
if err != nil {
|
||||
t.Fatalf("get task: %v", err)
|
||||
}
|
||||
if task.WorkflowStepID != "step1" {
|
||||
t.Fatalf("clarification continuation must not run on_turn_start; got step %q", task.WorkflowStepID)
|
||||
}
|
||||
if len(agentMgr.capturedPrompts) != 1 {
|
||||
t.Fatalf("expected one clarification answer prompt, got %d", len(agentMgr.capturedPrompts))
|
||||
}
|
||||
if !strings.Contains(agentMgr.capturedPrompts[0], "User selected: PostgreSQL") {
|
||||
t.Fatalf("clarification answer prompt missing answer: %q", agentMgr.capturedPrompts[0])
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildClarificationPrompt(t *testing.T) {
|
||||
t.Run("builds accepted prompt with question and answer", func(t *testing.T) {
|
||||
data := clarificationAnsweredData{
|
||||
|
||||
@@ -106,6 +106,80 @@ func TestProcessOnTurnComplete_ExplicitSignalGating(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestProcessOnTurnComplete_BlocksWhileClarificationPending(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedPendingClarificationMessage(t, repo, "t1", "s1")
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step1"] = &wfmodels.WorkflowStep{
|
||||
ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0,
|
||||
Events: wfmodels.StepEvents{
|
||||
OnTurnComplete: []wfmodels.OnTurnCompleteAction{
|
||||
{Type: wfmodels.OnTurnCompleteMoveToNext},
|
||||
},
|
||||
},
|
||||
}
|
||||
stepGetter.steps["step2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1,
|
||||
}
|
||||
svc := createTestService(repo, stepGetter, newMockTaskRepo())
|
||||
|
||||
task, _ := repo.GetTask(ctx, "t1")
|
||||
session, _ := repo.GetTaskSession(ctx, "s1")
|
||||
if got := svc.processOnTurnComplete(ctx, task, session); got {
|
||||
t.Fatal("pending clarification must block legacy on_turn_complete transition")
|
||||
}
|
||||
updated, _ := repo.GetTask(ctx, "t1")
|
||||
if updated.WorkflowStepID != "step1" {
|
||||
t.Fatalf("expected workflow step to remain step1, got %q", updated.WorkflowStepID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestProcessOnTurnCompleteViaEngine_BlocksWhileClarificationPendingEvenWithSignal(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedPendingClarificationMessage(t, repo, "t1", "s1")
|
||||
if err := repo.SetSessionMetadataKey(ctx, "s1", models.SessionMetaKeyPendingStepCompletion, models.PendingStepCompletionSignal{
|
||||
StepID: "step1",
|
||||
Source: models.StepCompletionSourceAgent,
|
||||
Summary: "done without answer",
|
||||
SignaledAt: time.Now().UTC(),
|
||||
}); err != nil {
|
||||
t.Fatalf("seed signal: %v", err)
|
||||
}
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step1"] = &wfmodels.WorkflowStep{
|
||||
ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0,
|
||||
AutoAdvanceRequiresSignal: true,
|
||||
Events: wfmodels.StepEvents{
|
||||
OnTurnComplete: []wfmodels.OnTurnCompleteAction{
|
||||
{Type: wfmodels.OnTurnCompleteMoveToNext},
|
||||
},
|
||||
},
|
||||
}
|
||||
stepGetter.steps["step2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1,
|
||||
}
|
||||
svc := createEngineService(t, repo, stepGetter, &mockAgentManager{})
|
||||
|
||||
session, _ := repo.GetTaskSession(ctx, "s1")
|
||||
if got := svc.processOnTurnCompleteViaEngine(ctx, "t1", session); got {
|
||||
t.Fatal("pending clarification must block engine on_turn_complete transition even with completion signal")
|
||||
}
|
||||
session, _ = repo.GetTaskSession(ctx, "s1")
|
||||
if _, has := models.LoadPendingStepSignal(session.Metadata); has {
|
||||
t.Fatal("pending clarification must clear stale completion signal")
|
||||
}
|
||||
updated, _ := repo.GetTask(ctx, "t1")
|
||||
if updated.WorkflowStepID != "step1" {
|
||||
t.Fatalf("expected workflow step to remain step1, got %q", updated.WorkflowStepID)
|
||||
}
|
||||
}
|
||||
|
||||
// TestLoadPendingStepSignal_RoundTrip verifies the bag survives JSON
|
||||
// rehydration — important for the backend-restart path where the bag is
|
||||
// read from the DB as map[string]interface{} rather than the typed struct.
|
||||
@@ -246,6 +320,46 @@ func TestOnStepCompletionSignaled(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("WAITING + pending clarification → no transition", func(t *testing.T) {
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step1")
|
||||
seedPendingClarificationMessage(t, repo, "t1", "s1")
|
||||
if err := repo.UpdateTaskSessionState(ctx, "s1", models.TaskSessionStateWaitingForInput, ""); err != nil {
|
||||
t.Fatalf("flip session waiting: %v", err)
|
||||
}
|
||||
signal := models.PendingStepCompletionSignal{
|
||||
StepID: "step1",
|
||||
Source: models.StepCompletionSourceAgent,
|
||||
Summary: "ok",
|
||||
SignaledAt: time.Now().UTC(),
|
||||
}
|
||||
if err := repo.SetSessionMetadataKey(ctx, "s1", models.SessionMetaKeyPendingStepCompletion, signal); err != nil {
|
||||
t.Fatalf("seed bag: %v", err)
|
||||
}
|
||||
|
||||
stepGetter := newMockStepGetter()
|
||||
stepGetter.steps["step1"] = &wfmodels.WorkflowStep{
|
||||
ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0,
|
||||
AutoAdvanceRequiresSignal: true,
|
||||
Events: wfmodels.StepEvents{
|
||||
OnTurnComplete: []wfmodels.OnTurnCompleteAction{
|
||||
{Type: wfmodels.OnTurnCompleteMoveToNext},
|
||||
},
|
||||
},
|
||||
}
|
||||
stepGetter.steps["step2"] = &wfmodels.WorkflowStep{
|
||||
ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1,
|
||||
}
|
||||
svc := createTestService(repo, stepGetter, newMockTaskRepo())
|
||||
|
||||
svc.onStepCompletionSignaled(ctx, buildEvent("t1", "s1", "step1"))
|
||||
|
||||
updated, _ := repo.GetTask(ctx, "t1")
|
||||
if updated.WorkflowStepID != "step1" {
|
||||
t.Errorf("expected pending clarification to keep task on step1, got %q", updated.WorkflowStepID)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("stale step → bag cleared, no transition", func(t *testing.T) {
|
||||
repo := setupTestRepo(t)
|
||||
seedSession(t, repo, "t1", "s1", "step_current")
|
||||
|
||||
@@ -56,6 +56,10 @@ func (s *Service) processOnTurnComplete(ctx context.Context, task *models.Task,
|
||||
return false
|
||||
}
|
||||
|
||||
if s.turnCompleteBlockedByUserInput(ctx, taskID, sessionID, session) {
|
||||
return false
|
||||
}
|
||||
|
||||
// ADR 0015 — explicit completion signal gating (legacy path mirror of
|
||||
// processOnTurnCompleteViaEngine). Steps marked
|
||||
// `auto_advance_requires_signal=true` wait for a step_complete_kandev
|
||||
@@ -2015,6 +2019,9 @@ func (s *Service) processOnTurnCompleteViaEngine(ctx context.Context, taskID str
|
||||
s.setSessionWaitingForInput(ctx, taskID, session.ID, session)
|
||||
return false
|
||||
}
|
||||
if s.turnCompleteBlockedByUserInput(ctx, taskID, session.ID, session) {
|
||||
return false
|
||||
}
|
||||
if currentStep.AutoAdvanceRequiresSignal {
|
||||
signal, has := models.LoadPendingStepSignal(session.Metadata)
|
||||
if !has || signal.StepID != task.WorkflowStepID {
|
||||
|
||||
@@ -99,19 +99,55 @@ test.describe("Clarification flow", () => {
|
||||
apiClient,
|
||||
seedData,
|
||||
}) => {
|
||||
const session = await seedClarificationTask(
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
"Clarification Timeout",
|
||||
"clarification-timeout",
|
||||
const workflow = await apiClient.createWorkflow(
|
||||
seedData.workspaceId,
|
||||
"Clarification Timeout Workflow",
|
||||
);
|
||||
const timeoutStep = await apiClient.createWorkflowStep(workflow.id, "Timeout Step", 0);
|
||||
await apiClient.createWorkflowStep(workflow.id, "Should Not Advance", 1);
|
||||
|
||||
await apiClient.updateWorkflowStep(timeoutStep.id, {
|
||||
events: {
|
||||
on_turn_complete: [{ type: "move_to_next" }],
|
||||
},
|
||||
});
|
||||
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Clarification Timeout",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:clarification-timeout",
|
||||
workflow_id: workflow.id,
|
||||
workflow_step_id: timeoutStep.id,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id");
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
|
||||
await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 });
|
||||
await expect(session.chat).toContainText("Question timed out", { timeout: 30_000 });
|
||||
await expect(session.clarificationDeferredNotice()).toBeVisible({ timeout: 10_000 });
|
||||
await expect(session.clarificationExpiredNotice()).not.toBeVisible();
|
||||
|
||||
await expect
|
||||
.poll(async () => (await apiClient.getTask(task.id)).workflow_step_id, {
|
||||
timeout: 10_000,
|
||||
message: "clarification timeout must not run on_turn_complete auto-advance",
|
||||
})
|
||||
.toBe(timeoutStep.id);
|
||||
|
||||
const sessionsAfterTimeout = await apiClient.listTaskSessions(task.id);
|
||||
const primarySession = sessionsAfterTimeout.sessions.find(
|
||||
(candidate) => candidate.id === task.session_id,
|
||||
);
|
||||
expect(primarySession?.state).toBe("WAITING_FOR_INPUT");
|
||||
|
||||
// Agent moved on; a late answer goes through the event fallback as a new prompt.
|
||||
await session.clarificationOption("PostgreSQL").click();
|
||||
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
@@ -11,6 +11,7 @@ Today, a workflow step's `on_turn_complete` actions (most commonly `move_to_next
|
||||
- **Halts are overloaded.** An agent stops emitting for many reasons: it actually finished the requested work; it called `ask_user_question_kandev` (or any clarification mechanism) and is waiting on the user; it hit a provider rate limit or transient error and bailed; it terminated mid-thought due to a context budget; it shelled out and exited; a future agent CLI added a new halt class we don't know about yet. From the orchestrator's vantage point these are indistinguishable — all of them look like "turn complete."
|
||||
- **The reasons evolve faster than our interpreter.** Each new agent type and each upstream CLI revision can introduce new termination modes. Treating every halt as completion guarantees false positives now and on each upgrade.
|
||||
- **Auto-advance changes user-observed behavior.** Users expect the agent to behave the same whether auto-advance is on or off: ask questions and wait indefinitely, hold extended back-and-forth conversations, defer until the user replies. With auto-advance on, the very act of asking a question (which halts the turn) trips a step transition, dropping the conversation onto the next step before the human has answered.
|
||||
- **Clarification waits are durable user-input barriers.** If `ask_user_question_kandev` times out, disconnects, or otherwise ends before the user answers, the workflow must stay on the same step. Running `on_turn_complete` or start-of-next-turn triggers in that state treats "awaiting user input" as "step finished" and can advance the task with missing requirements.
|
||||
- **Working around it via "auto-advance off + side effect" is worse.** Disabling the checkbox and re-implementing advance as a hidden behavior reintroduces the same ambiguity at a different layer and loses the explicit UI affordance.
|
||||
- **Tasks driving themselves through `update_task_kandev` / `move_task_kandev` is an alternative we rejected.** Letting an agent mutate the very task it's running on creates ordering and state-machine hazards (lifecycle races against the in-flight turn, ambiguous "which step's events fire," reconciliation against `task_repositories` and session state). Even if we hardened it, it would not address the underlying interpretation problem — we'd still be guessing what a halt meant.
|
||||
|
||||
@@ -67,9 +68,13 @@ Today, a workflow step's `on_turn_complete` actions (most commonly `move_to_next
|
||||
- **Step requires explicit signal** (auto-advance enabled and `auto_advance_requires_signal == true`): on bare turn-end, read `Metadata["pending_step_completion_signal"]`. If absent (or stale for a prior step), do **not** evaluate transition actions; call `setSessionWaitingForInput` and surface a "Completion pending" UI hint. If present and matches the current step, run the existing `processTurnCompleteActions` / `resolveTransitionTargetStep` / `executeStepTransition` pipeline and clear the bag entry. The same path is triggered out-of-band by a new bus subscriber `onStepCompletionSignaled` for the case where the signal lands *after* the turn already ended (signal-after-halt is the common shape).
|
||||
- **Step does not require explicit signal** (legacy behavior): unchanged.
|
||||
|
||||
5. **Halt-without-signal fallback.** When the agent halts without having called `step_complete_kandev`, the UI shows an inline "Mark complete & advance" action (rendered in the chat composer area, akin to the existing clarification overlay). One click writes the bag entry with `source: "manual_fallback"` and emits the same `workflow.step_completion_signaled` event. This is the safety net: false negatives (work done, agent forgot to call the tool) cost one user click; false positives (agent calls the tool prematurely) are caught because user can keep messaging and the next turn re-opens the step. The fallback is **disabled** during an active ask-clarification waiting window so the button doesn't compete with a pending user question.
|
||||
5. **Clarification hard-pause barrier.** A pending durable clarification request blocks every `on_turn_complete` transition path, including the legacy path, the workflow-engine path, and the `workflow.step_completion_signaled` subscriber. This guard runs before explicit completion-signal evaluation, so even a previously written `step_complete_kandev` signal cannot advance while a user question is pending.
|
||||
|
||||
6. **Re-open semantics.** If the user sends a new message *after* the signal has been written *but before* the transition has executed, the orchestrator clears the bag entry, cancels the pending transition, and treats the message as continued conversation. If the user messages *after* the transition has already moved to the next step, the message lands on the new step as normal — the prior step is not re-opened.
|
||||
When `ask_user_question_kandev` ends without an answer because the MCP wait timed out, the transport disconnected, or the handler context was cancelled, the MCP handler calls the orchestrator's clarification pause path. That path detaches the pending clarification for later answer handling, silently cancels the active agent turn when one exists, marks the session `WAITING_FOR_INPUT`, completes the active turn bookkeeping, and deliberately skips workflow stop triggers. When the user later answers the pending clarification, the answer resumes the current session as a continuation prompt and deliberately skips `on_turn_start` triggers; otherwise answering a question could immediately fire start-of-turn transitions for the same step.
|
||||
|
||||
6. **Halt-without-signal fallback.** When the agent halts without having called `step_complete_kandev`, the UI shows an inline "Mark complete & advance" action (rendered in the chat composer area, akin to the existing clarification overlay). One click writes the bag entry with `source: "manual_fallback"` and emits the same `workflow.step_completion_signaled` event. This is the safety net: false negatives (work done, agent forgot to call the tool) cost one user click; false positives (agent calls the tool prematurely) are caught because user can keep messaging and the next turn re-opens the step. The fallback is **disabled** during an active ask-clarification waiting window so the button doesn't compete with a pending user question.
|
||||
|
||||
7. **Re-open semantics.** If the user sends a new message *after* the signal has been written *but before* the transition has executed, the orchestrator clears the bag entry, cancels the pending transition, and treats the message as continued conversation. If the user messages *after* the transition has already moved to the next step, the message lands on the new step as normal — the prior step is not re-opened.
|
||||
|
||||
### Configuration surface
|
||||
|
||||
@@ -82,6 +87,7 @@ Today, a workflow step's `on_turn_complete` actions (most commonly `move_to_next
|
||||
- **Compliance burden on the agent.** Whether the agent obeys the injected instruction is a function of (a) the model's instruction-following, and (b) the agent CLI's handling of system-prompt prepends. Both vary across Claude, Codex, Cursor, OpenCode. The fallback button is what makes this acceptable; without it, the design would brick non-Claude agents.
|
||||
- **Adds a new MCP tool.** Increases the kandev MCP surface area; tool listing payload grows by one entry. The tool itself is small, but new agents and new CLIs need to be validated against it the same way ADR 0014's strategies were.
|
||||
- **`step_complete_kandev` is fire-and-forget once it returns.** The MCP call only returns after the bag write + bus publish complete, but the actual transition runs asynchronously in the bus subscriber. An agent that calls the tool then immediately continues working may have trailing tokens race with the transition. Mitigation today is behavioural: the tool description in `kandev-context.md` instructs the agent to "call this as the LAST action of the step." The manual fallback button and re-open semantics are the safety nets if the agent gets it wrong.
|
||||
- **Clarification timeout becomes a hard pause, not a completed turn.** A no-answer `ask_user_question_kandev` result cancels the current agent turn without running `on_turn_complete`. The user-visible state is a waiting session with the clarification still answerable; the next user answer resumes the same step instead of triggering turn-start automation.
|
||||
- **Idempotency.** Multiple calls within the same step are deduped at the bag-write site (first call wins, subsequent calls return `{accepted: false, reason: "already_signaled"}` without error). Cross-step re-entry is allowed — a fresh step's `currentStepID` no longer matches the stale bag entry, so the next call writes through.
|
||||
- **No new schema for the signal itself.** Storage piggybacks on the existing `task_sessions.metadata` JSON column. Audit of *consumed* signals piggybacks on the existing `session_step_history.metadata` JSON column. Cancelled signals (overwritten by a user reply) are not retained — chat already surfaces what the user needs.
|
||||
- **Telemetry (deferred).** The plan is two `expvar` counters under `workflow_*` — `step_completion_signal_received_total` and `step_completion_signal_fallback_used_total` — whose ratio (fallback-used / received ≤ 10% per agent type) becomes the headline "is the explicit-signal flow working" metric. Neither counter exists yet; ship in a follow-up alongside the manual fallback button so both halves of the ratio can be measured at once.
|
||||
@@ -91,6 +97,7 @@ Today, a workflow step's `on_turn_complete` actions (most commonly `move_to_next
|
||||
|
||||
- **Naming: `task_complete_kandev`.** Rejected: signal is per-workflow-step, not per-task. A task may pass through `triage → in-progress → review → done`, with `step_complete_kandev` called at the end of each. `task_*` would either misname the per-step signal or imply a separate "the whole task is done" semantic we don't have.
|
||||
- **Heuristic: classify turn-end reasons.** Pattern-match on the ACP stop reason, presence of recent `ask_user_question_kandev` calls, etc. Rejected: re-implements the interpretation problem we're trying to escape, and breaks on every new agent CLI revision.
|
||||
- **Cancel immediately when `ask_user_question_kandev` is called.** Rejected as the default because a healthy agent connection can block inside the tool call until the user answers, preserving the normal conversational flow. We only hard-pause when the wait ends without an answer, or when the agentctl-side timeout event reports that the MCP wait failed. That gives the good path no extra turn churn while still failing closed on timeout/disconnect.
|
||||
- **Dedicated `session_step_completion_signals` table.** Considered (typed columns, partial UNIQUE index for idempotency, `source` column for SQL-aggregated telemetry, full cancelled-signal audit trail). Rejected as overkill: at most one pending signal per session at a time, lifetime measured in seconds, single reader. The `TaskSession.Metadata` bag covers idempotency (existence check), audit (lifted onto the existing `SessionStepHistory.Metadata` on transition), telemetry (in-process `expvar`), and chat visibility (system message) with zero new schema.
|
||||
- **Two-signal: require both halt and explicit tool call.** Effectively what we do, except we let the user provide the second signal via the fallback button when the agent doesn't. Pure "agent must do both" guarantees the stuck-task failure mode without the escape hatch.
|
||||
- **Agent self-mutating via `update_task_kandev` / `move_task_kandev`.** Lets the agent move its own task between steps. Rejected up front: state-machine races (orchestrator step-transition pipeline vs in-flight turn), ambiguous event semantics (does `on_exit` for the old step fire mid-turn?), and conflicts with the existing `workflowStepGetter`-driven transition path. Even if hardened, it doesn't fix the halt-interpretation problem; the agent still has to *decide* when to call it, which is the same decision as calling `task_complete_kandev` — but on a tool that has many other side effects.
|
||||
|
||||
Reference in New Issue
Block a user