diff --git a/apps/backend/internal/orchestrator/agent_error_fire_site_pin_test.go b/apps/backend/internal/orchestrator/agent_error_fire_site_pin_test.go new file mode 100644 index 0000000000..a5f5dcb994 --- /dev/null +++ b/apps/backend/internal/orchestrator/agent_error_fire_site_pin_test.go @@ -0,0 +1,238 @@ +package orchestrator + +// TestOnAgentErrorFireSitesArePinned is the regression guard for the defect +// this card fixes: on_agent_error declared, compiled, and silently never +// dispatched outside Office. It walks the whole backend source tree +// (go/parser, rooted at apps/backend so a third fire site added in ANY +// package — not just internal/, and not just this one — can fail it) and +// asserts the set of functions that actually FIRE engine.TriggerOnAgentError +// is exactly the two registered ones. A bare identifier scan would also match +// the trigger constant declaration, the compileGenericActions trigger-map +// entry, the OnAgentErrorPayload doc comment, and the Office engine +// dispatcher's session-resolution branches — so a fire site is defined +// syntactically: an occurrence in the value of the `Trigger:` key of an +// engine.HandleInput composite literal (the orchestrator shape), or a direct +// call argument to dispatchEngineTrigger (the Office shape, which passes the +// trigger positionally). Mirrors the design of +// task/repository/sqlite/step_transition_writers_pin_test.go. + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// registeredAgentErrorFireSites is the closed set of functions known to fire +// engine.TriggerOnAgentError, keyed "packageRelDir/ReceiverType.FuncName" +// (relative to the apps/backend scan root). A builder who renames either +// function must update this set in the same commit. +var registeredAgentErrorFireSites = []string{ + "internal/office/service/Service.dispatchAgentErrorTrigger", + "internal/orchestrator/Service.dispatchKanbanAgentErrorTrigger", +} + +func TestOnAgentErrorFireSitesArePinned(t *testing.T) { + root, err := findAgentErrorBackendSourceRoot(".") + if err != nil { + t.Fatalf("locate backend source root: %v", err) + } + if filepath.Base(root) != "backend" { + t.Fatalf("scan root %q is not the apps/backend directory", root) + } + if _, statErr := os.Stat(filepath.Join(root, "internal", "office")); statErr != nil { + t.Fatalf("scan root %q does not contain internal/office: %v", root, statErr) + } + + found, err := findAgentErrorFireSites(root) + if err != nil { + t.Fatalf("scan backend source for on_agent_error fire sites: %v", err) + } + + registered := make(map[string]bool, len(registeredAgentErrorFireSites)) + for _, name := range registeredAgentErrorFireSites { + registered[name] = true + } + + var unregistered []string + for name := range found { + if !registered[name] { + unregistered = append(unregistered, name) + } + } + if len(unregistered) > 0 { + sort.Strings(unregistered) + t.Fatalf( + "function(s) %v fire engine.TriggerOnAgentError but are not in "+ + "registeredAgentErrorFireSites. A third fire site is a regression "+ + "of this card's own defect (double dispatch) unless it replaces one "+ + "of the two registered sites — update the registered set only if "+ + "that is genuinely what happened.", + unregistered) + } + + var missing []string + for _, name := range registeredAgentErrorFireSites { + if !found[name] { + missing = append(missing, name) + } + } + if len(missing) > 0 { + sort.Strings(missing) + t.Fatalf( + "registeredAgentErrorFireSites lists %v but no matching fire site was "+ + "found — the function was likely renamed or removed, or on_agent_error "+ + "dispatch was removed from it (the exact defect this card fixes). "+ + "Update registeredAgentErrorFireSites to match reality.", + missing) + } +} + +func findAgentErrorBackendSourceRoot(startDir string) (string, error) { + dir, err := filepath.Abs(startDir) + if err != nil { + return "", err + } + for { + if _, statErr := os.Stat(filepath.Join(dir, "go.mod")); statErr == nil { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + return "", fmt.Errorf("no go.mod found above %s", startDir) + } + dir = parent + } +} + +func findAgentErrorFireSites(root string) (map[string]bool, error) { + var paths []string + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if d.IsDir() { + if d.Name() == "testdata" && path != root { + return filepath.SkipDir + } + return nil + } + name := d.Name() + if strings.HasSuffix(name, "_test.go") { + return nil + } + if strings.HasSuffix(name, ".go") { + paths = append(paths, path) + } + return nil + }) + if err != nil { + return nil, err + } + + found := make(map[string]bool) + fset := token.NewFileSet() + for _, path := range paths { + file, parseErr := parser.ParseFile(fset, path, nil, 0) + if parseErr != nil { + return nil, parseErr + } + relDir, relErr := filepath.Rel(root, filepath.Dir(path)) + if relErr != nil { + return nil, relErr + } + ast.Inspect(file, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Body == nil { + return true + } + if functionFiresTriggerOnAgentError(fn.Body) { + found[agentErrorFuncIdentity(fn, relDir)] = true + } + return true + }) + } + return found, nil +} + +// functionFiresTriggerOnAgentError reports whether body contains an +// occurrence of TriggerOnAgentError in one of the two fire-site shapes. +func functionFiresTriggerOnAgentError(body *ast.BlockStmt) bool { + fires := false + ast.Inspect(body, func(n ast.Node) bool { + switch v := n.(type) { + case *ast.CompositeLit: + sel, ok := v.Type.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "HandleInput" { + return true + } + for _, elt := range v.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + key, ok := kv.Key.(*ast.Ident) + if !ok || key.Name != "Trigger" { + continue + } + if identIsTriggerOnAgentError(kv.Value) { + fires = true + } + } + case *ast.CallExpr: + sel, ok := v.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "dispatchEngineTrigger" { + return true + } + for _, arg := range v.Args { + if identIsTriggerOnAgentError(arg) { + fires = true + } + } + } + return true + }) + return fires +} + +func identIsTriggerOnAgentError(expr ast.Expr) bool { + switch v := expr.(type) { + case *ast.SelectorExpr: + return v.Sel.Name == "TriggerOnAgentError" + case *ast.Ident: + return v.Name == "TriggerOnAgentError" + default: + return false + } +} + +func agentErrorFuncIdentity(fn *ast.FuncDecl, relDir string) string { + name := fn.Name.Name + if recv := agentErrorReceiverTypeName(fn); recv != "" { + name = recv + "." + name + } + if relDir == "" || relDir == "." { + return name + } + return filepath.ToSlash(relDir) + "/" + name +} + +func agentErrorReceiverTypeName(fn *ast.FuncDecl) string { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return "" + } + expr := fn.Recv.List[0].Type + if star, ok := expr.(*ast.StarExpr); ok { + expr = star.X + } + if ident, ok := expr.(*ast.Ident); ok { + return ident.Name + } + return "" +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index 777e72045a..bf97fcaa2c 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -1364,7 +1364,9 @@ func (s *Service) handleAgentCompletedLocked(ctx context.Context, data watcher.A // handleAgentFailed handles agent failure events func (s *Service) handleAgentFailed(ctx context.Context, data watcher.AgentEventData) { if data.SessionID == "" { - s.handleAgentFailedLocked(ctx, data) + if dispatch := s.handleAgentFailedLocked(ctx, data); dispatch != nil { + dispatch() + } return } @@ -1374,21 +1376,26 @@ func (s *Service) handleAgentFailed(ctx context.Context, data watcher.AgentEvent // recovery must not create messages, arm retries, or force-clean the // execution that graceful teardown now owns. lock, release := s.acquireCancelInFlightGuard(data.SessionID) - defer release() lock.Lock() - defer lock.Unlock() if s.isCancelInFlight(data.SessionID) { s.logger.Debug("deferring agent.failed while cancellation is in progress", zap.String("task_id", data.TaskID), zap.String("session_id", data.SessionID), zap.String("agent_execution_id", data.AgentExecutionID)) + lock.Unlock() + release() return } - s.handleAgentFailedLocked(ctx, data) + dispatch := s.handleAgentFailedLocked(ctx, data) + lock.Unlock() + release() + if dispatch != nil { + dispatch() + } } -func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.AgentEventData) { +func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.AgentEventData) func() { data = s.withPromptAttemptEvidence(data) defer s.clearPromptAttemptEvidence(data.SessionID, data.AgentExecutionID, data.PromptGeneration) s.logger.Warn("handling agent failed", @@ -1405,7 +1412,7 @@ func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.Agen s.retireExecutionActivityAndPublish( context.WithoutCancel(ctx), data.TaskID, data.SessionID, data.AgentExecutionID, ) - return + return nil } // Short transient provider errors get a paced, visible retry-with-backoff @@ -1416,10 +1423,10 @@ func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.Agen // handleTransientFailure returns false (falling through) for non-transient // errors, office tasks, or an exhausted budget. if data.SessionID != "" && s.handleTransientFailure(ctx, data) { - return + return nil } if data.SessionID != "" && s.routeDynamicAgentFailure(ctx, data, classifyKanbanFailure(data)) { - return + return nil } // All paths below are terminal for this execution (resume recovery included). @@ -1435,14 +1442,13 @@ func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.Agen // failure path. errMsg := data.ErrorMessage if errMsg == "" { - errMsg = "agent failed" + errMsg = defaultAgentFailedMessage } s.finalizeAutomationRun(ctx, data.TaskID, false, errMsg) // Make all agent CLI failures recoverable — let the user choose to resume or start fresh. if data.SessionID != "" { - s.handleRecoverableFailureLocked(ctx, data) - return + return s.handleRecoverableFailureLockedState(ctx, data) } // No session — fall back to scheduler retry + task to REVIEW unless another @@ -1452,6 +1458,7 @@ func (s *Service) handleAgentFailedLocked(ctx context.Context, data watcher.Agen s.writeTaskReviewState(ctx, data.TaskID, data.SessionID) go s.cleanupAgentExecution(data.AgentExecutionID, data.TaskID, data.SessionID) + return nil } func (s *Service) shouldDropSessionFailure( @@ -1732,20 +1739,22 @@ func (s *Service) clearResumeToken(ctx context.Context, sessionID string) error // resume the agent session or start fresh. func (s *Service) handleRecoverableFailure(ctx context.Context, data watcher.AgentEventData) { if data.SessionID == "" { - s.handleRecoverableFailureLocked(ctx, data) + if dispatch := s.handleRecoverableFailureLockedState(ctx, data); dispatch != nil { + dispatch() + } return } lock, release := s.acquireCancelInFlightGuard(data.SessionID) - defer release() lock.Lock() - defer lock.Unlock() if _, err := s.repo.GetTaskSession(ctx, data.SessionID); err != nil { if errors.Is(err, models.ErrTaskSessionNotFound) { s.logger.Debug("skipping recoverable failure for deleted session", zap.String("task_id", data.TaskID), zap.String("session_id", data.SessionID)) + lock.Unlock() + release() return } s.logger.Warn("failed to reload session before recoverable failure; continuing", @@ -1753,13 +1762,29 @@ func (s *Service) handleRecoverableFailure(ctx context.Context, data watcher.Age zap.String("session_id", data.SessionID), zap.Error(err)) } - s.handleRecoverableFailureLocked(ctx, data) + dispatch := s.handleRecoverableFailureLockedState(ctx, data) + lock.Unlock() + release() + if dispatch != nil { + dispatch() + } } -// handleRecoverableFailureLocked performs recovery side effects while the -// session's cancelInFlight guard is held. Deletion uses the same guard, so an -// active error event cannot publish after the deleted-session inactive event. +// handleRecoverableFailureLocked performs recovery side effects and dispatches +// the workflow trigger synchronously. Production callers that already hold the +// session guard must use handleRecoverableFailureLockedState and invoke the +// returned dispatch after releasing that guard. func (s *Service) handleRecoverableFailureLocked(ctx context.Context, data watcher.AgentEventData) { + if dispatch := s.handleRecoverableFailureLockedState(ctx, data); dispatch != nil { + dispatch() + } +} + +// handleRecoverableFailureLockedState performs recovery side effects while the +// session's cancelInFlight guard is held and returns the workflow dispatch to +// run after the guard is released. Deletion uses the same guard, so an active +// error event cannot publish after the deleted-session inactive event. +func (s *Service) handleRecoverableFailureLockedState(ctx context.Context, data watcher.AgentEventData) func() { s.logger.Warn("handling recoverable agent failure", zap.String("task_id", data.TaskID), zap.String("session_id", data.SessionID), @@ -1821,12 +1846,21 @@ func (s *Service) handleRecoverableFailureLocked(ctx context.Context, data watch // Clean up the agent execution. go s.cleanupAgentExecution(data.AgentExecutionID, data.TaskID, data.SessionID) + + // The caller runs this after releasing cancelInFlight. A direct + // auto_start_agent callback can start the same session and reacquire that + // guard, so dispatching while it is held would deadlock. Panic recovery is + // the recovered wrapper's job, not this one's — routes R2-R5 reach this + // closure with no recover above them on the stack. + return func() { + s.dispatchKanbanAgentErrorTriggerRecovered(context.WithoutCancel(ctx), data) + } } func (s *Service) persistLastAgentError(ctx context.Context, data watcher.AgentEventData) { errMsg := data.ErrorMessage if errMsg == "" { - errMsg = "agent failed" + errMsg = defaultAgentFailedMessage } details := routingerr.Sanitize(data.FailureDetails) // Keep this metadata until the user dismisses the UI notice locally or a @@ -2121,11 +2155,23 @@ func (s *Service) handleAgentStartFailed(ctx context.Context, taskID, sessionID, failureData.FailureCode = string(routingerr.CodeManagedRuntimeNpmResolution) failureData.FailureDetails = classified.RawExcerpt } + var unlockGuard func() + var releaseGuard func() + var dispatch func() + defer func() { + if unlockGuard != nil { + unlockGuard() + releaseGuard() + } + if dispatch != nil { + dispatch() + } + }() if sessionID != "" { lock, release := s.acquireCancelInFlightGuard(sessionID) - defer release() lock.Lock() - defer lock.Unlock() + unlockGuard = lock.Unlock + releaseGuard = release if s.isCancelInFlight(sessionID) { s.logger.Debug("deferring agent start failure while cancellation is in progress", zap.String("task_id", taskID), @@ -2149,7 +2195,7 @@ func (s *Service) handleAgentStartFailed(ctx context.Context, taskID, sessionID, zap.String("task_id", taskID), zap.String("session_id", sessionID), zap.String("agent_execution_id", agentExecutionID)) - s.handleRecoverableFailureLocked(ctx, failureData) + dispatch = s.handleRecoverableFailureLockedState(ctx, failureData) return true } @@ -2166,7 +2212,7 @@ func (s *Service) handleAgentStartFailed(ctx context.Context, taskID, sessionID, s.logger.Info("agent start failure is auth error, treating as recoverable", zap.String("task_id", taskID), zap.String("session_id", sessionID)) - s.handleRecoverableFailureLocked(ctx, failureData) + dispatch = s.handleRecoverableFailureLockedState(ctx, failureData) return true } diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error.go b/apps/backend/internal/orchestrator/event_handlers_agent_error.go new file mode 100644 index 0000000000..91963fdaa2 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error.go @@ -0,0 +1,290 @@ +package orchestrator + +import ( + "context" + "errors" + "fmt" + "runtime/debug" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/orchestrator/watcher" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/workflow/engine" +) + +// Stable, dispatch-owned log messages. Kept distinct from every +// other log line in this package so a test can key on the dispatch's own +// records without also matching a line the surrounding handler or a shared +// helper (e.g. otherWorkingSessionID) writes for itself. +const ( + msgAgentErrorUserInitiated = "on_agent_error dispatch: failure was user-initiated" + msgAgentErrorSessionVanished = "on_agent_error dispatch: session no longer exists" + msgAgentErrorSessionReloadFailed = "on_agent_error dispatch: failed to reload session" + msgAgentErrorTaskLoadFailed = "on_agent_error dispatch: failed to load task" + msgAgentErrorAnotherSessionWorking = "on_agent_error dispatch: another session is working" + msgAgentErrorActionUnregistered = "on_agent_error action has no registered callback" + msgAgentErrorDispatched = "on_agent_error dispatched" + msgAgentErrorNoActions = "on_agent_error dispatch: step declares no actions" + msgAgentErrorDispatchFailed = "on_agent_error dispatch failed" + msgAgentErrorDispatchPanicked = "on_agent_error dispatch panicked" + + // defaultAgentFailedMessage backfills an empty AgentEventData.ErrorMessage + // (e.g. a synthetic start-failure event). Shared with the two other + // terminal-failure call sites in event_handlers_agent.go so the literal + // stays a single source of truth (goconst). + defaultAgentFailedMessage = "agent failed" +) + +// agentErrorDispatchDeps bundles the workflow engine, the callback registry +// the pre-engine walk checks against, and the store both read — the three +// collaborators initWorkflowEngine rebuilds together on every reinit. +// Holding them as one struct, published through Service.agentErrorDeps in a +// single atomic.Pointer write and read exactly once per dispatch, closes the +// race three separate fields would otherwise leave open: a dispatch racing a +// Set*/reinit call could pair a new engine with a stale registry, or walk +// one version of a step while the engine executes another. +type agentErrorDispatchDeps struct { + engine *engine.Engine + registry engine.CallbackRegistry + store *workflowStore +} + +// agentErrorOperationID derives on_agent_error's idempotency key from the +// failure's identity rather than from a run. Both components +// matter: a session can host several executions in sequence (rotation, +// resume, dynamic re-route), each of which can fail, so a session-only key +// would suppress every failure after the first for the life of the process. +func agentErrorOperationID(sessionID, agentExecutionID string) string { + if agentExecutionID == "" { + return fmt.Sprintf("agent_error:session:%s", sessionID) + } + return fmt.Sprintf("agent_error:session:%s:%s", sessionID, agentExecutionID) +} + +// agentErrorFailedAgentID resolves OnAgentErrorPayload.FailedAgentID. +// The synthetic failure events built by handleAgentStartFailed and +// the transient-loop exits carry no AgentProfileID, so the session fallback +// is the normal path for four of the five dispatching routes. +func agentErrorFailedAgentID(data watcher.AgentEventData, session *models.TaskSession) string { + if data.AgentProfileID != "" { + return data.AgentProfileID + } + if session != nil { + return session.AgentProfileID + } + return "" +} + +// agentErrorMessage resolves OnAgentErrorPayload.ErrorMessage. +func agentErrorMessage(data watcher.AgentEventData) string { + if data.ErrorMessage != "" { + return data.ErrorMessage + } + return defaultAgentFailedMessage +} + +// isAgentErrorTransitionActionKind reports whether kind is one of the three +// structural transition kinds, which the engine executes without a +// registered callback regardless of position, guard, or requires_approval. +func isAgentErrorTransitionActionKind(kind engine.ActionKind) bool { + switch kind { + case engine.ActionMoveToNext, engine.ActionMoveToPrevious, engine.ActionMoveToStep: + return true + default: + return false + } +} + +// warnUnregisteredAgentErrorActions is the pre-engine walk: it reads the +// current step's compiled on_agent_error actions through the same store the +// engine will use and warns on any non-transition action with no +// registered callback. A failed read is advisory-only — the walk is +// skipped silently and the dispatch still proceeds to the engine. +func (s *Service) warnUnregisteredAgentErrorActions( + ctx context.Context, deps *agentErrorDispatchDeps, task *models.Task, sessionID string, +) { + step, err := deps.store.LoadStep(ctx, task.WorkflowID, task.WorkflowStepID) + if err != nil { + return + } + for _, action := range step.Events[engine.TriggerOnAgentError] { + if isAgentErrorTransitionActionKind(action.Kind) { + continue + } + if _, ok := deps.registry.Get(action.Kind); ok { + continue + } + s.logger.Warn(msgAgentErrorActionUnregistered, + zap.String("workflow_id", task.WorkflowID), + zap.String("step_id", task.WorkflowStepID), + zap.String("step_name", step.Name), + zap.String("action_type", string(action.Kind)), + zap.String("task_id", task.ID), + zap.String("session_id", sessionID)) + } +} + +// resolveAgentErrorDispatchTarget reloads the session and task for a failure +// and evaluates the ownership/shape guards. ok is +// false when dispatch must stop; every stop path has already logged at the +// correct level (or, for the silent shape guards, deliberately logs +// nothing). +func (s *Service) resolveAgentErrorDispatchTarget( + ctx context.Context, data watcher.AgentEventData, +) (session *models.TaskSession, task *models.Task, ok bool) { + session, err := s.repo.GetTaskSession(ctx, data.SessionID) + if err != nil { + if errors.Is(err, models.ErrTaskSessionNotFound) { + s.logger.Debug(msgAgentErrorSessionVanished, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID)) + return nil, nil, false + } + s.logger.Warn(msgAgentErrorSessionReloadFailed, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.Error(err)) + return nil, nil, false + } + if session == nil { + s.logger.Debug(msgAgentErrorSessionVanished, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID)) + return nil, nil, false + } + + task, err = s.repo.GetTask(ctx, data.TaskID) + if err != nil || task == nil { + s.logger.Warn(msgAgentErrorTaskLoadFailed, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.Error(err)) + return nil, nil, false + } + + if task.IsFromOffice || task.IsEphemeral || task.WorkflowStepID == "" || taskArchived(task) { + return nil, nil, false + } + + blockingSessionID, okOther := s.otherWorkingSessionID(ctx, data.TaskID, data.SessionID) + if !okOther { + // otherWorkingSessionID already logged its own WARNING. + return nil, nil, false + } + if blockingSessionID != "" { + s.logger.Debug(msgAgentErrorAnotherSessionWorking, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("blocking_session_id", blockingSessionID)) + return nil, nil, false + } + return session, task, true +} + +// dispatchKanbanAgentErrorTrigger is the non-Office fire site for +// engine.TriggerOnAgentError — the counterpart to Office's +// dispatchAgentErrorTrigger. It is called as the final action of terminal +// agent-session-failure handling, after that failure's own +// bookkeeping is already committed, so a dispatch failure here must never +// mask it. +// +// Guards run first-match-wins in the order fixed by "Guard evaluation +// order" in docs/specs/workflow-on-agent-error-dispatch/spec.md: the engine +// snapshot, the session id, the user-initiated marker, then a single +// session-then-task reload feeding the ownership and shape guards +// (resolveAgentErrorDispatchTarget). +// +// ctx is stripped of the caller's cancellation (see the call site in +// handleRecoverableFailureLocked): this dispatch is the recovery path for an +// already-failed session, so a canceled request context must not suppress +// the recovery it exists to run. +func (s *Service) dispatchKanbanAgentErrorTrigger(ctx context.Context, data watcher.AgentEventData) { + deps := s.agentErrorDeps.Load() + if deps == nil || deps.engine == nil { + return + } + if data.SessionID == "" { + return + } + if data.UserInitiated { + s.logger.Debug(msgAgentErrorUserInitiated, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID)) + return + } + + session, task, ok := s.resolveAgentErrorDispatchTarget(ctx, data) + if !ok { + return + } + + state := s.buildMachineState(ctx, task, session) + operationID := agentErrorOperationID(data.SessionID, data.AgentExecutionID) + + s.warnUnregisteredAgentErrorActions(ctx, deps, task, data.SessionID) + + result, err := deps.engine.HandleTrigger(ctx, engine.HandleInput{ + TaskID: data.TaskID, + SessionID: data.SessionID, + Trigger: engine.TriggerOnAgentError, + OperationID: operationID, + EvaluateOnly: true, + PreloadedState: &state, + Payload: engine.OnAgentErrorPayload{ + FailedAgentID: agentErrorFailedAgentID(data, session), + FailedSessionID: data.SessionID, + ErrorMessage: agentErrorMessage(data), + }, + }) + if err != nil { + s.logger.Error(msgAgentErrorDispatchFailed, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("operation_id", operationID), + zap.Error(err)) + return + } + if result.Idempotent { + return + } + + if result.Transitioned { + s.applyEngineTransition(ctx, data.TaskID, session, result, engine.TriggerOnAgentError, task.Description, true) + } + + if result.ActionCount > 0 { + s.logger.Info(msgAgentErrorDispatched, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("step_id", task.WorkflowStepID), + zap.String("operation_id", operationID)) + return + } + s.logger.Debug(msgAgentErrorNoActions, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("step_id", task.WorkflowStepID), + zap.String("operation_id", operationID)) +} + +// dispatchKanbanAgentErrorTriggerRecovered wraps dispatchKanbanAgentErrorTrigger +// with panic recovery. The dispatch now runs synchronously after the caller +// has released the failed session's cancelInFlight guard (see +// handleRecoverableFailureLockedState), so — unlike an agent.failed delivery +// that arrives through the event bus's own recover-and-log invokeHandler +// (events/bus/safe_handler.go) — routes R2-R5 reach this call with no +// recover above them on the stack. A panicking callback (e.g. auto_start_agent) +// would otherwise take the whole backend process down. +func (s *Service) dispatchKanbanAgentErrorTriggerRecovered(ctx context.Context, data watcher.AgentEventData) { + defer func() { + if r := recover(); r != nil { + s.logger.Error(msgAgentErrorDispatchPanicked, + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.Any("panic", r), + zap.String("stack", string(debug.Stack()))) + } + }() + s.dispatchKanbanAgentErrorTrigger(ctx, data) +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_coverage_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_coverage_test.go new file mode 100644 index 0000000000..badbe1163d --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_coverage_test.go @@ -0,0 +1,648 @@ +package orchestrator + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/orchestrator/watcher" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + + "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// newAgentErrorTransientTestService is newAgentErrorTestService's sibling for +// the R4/R5 (transient-retry) and AC-E3 (auto-start) routes, which need a +// caller-supplied mockAgentManager (a custom promptErr, isAgentRunning, or +// promptDone channel) rather than the plain one newAgentErrorTestService +// builds internally. +func newAgentErrorTransientTestService( + t *testing.T, repo *sqliterepo.Repository, stepGetter *mockStepGetter, agentMgr *mockAgentManager, + configure func(*Service), +) (*Service, *observer.ObservedLogs) { + t.Helper() + svc := createTestServiceWithScheduler(repo, stepGetter, newMockTaskRepo(), agentMgr) + core, logs := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("build observed logger: %v", err) + } + svc.logger = log + if configure != nil { + configure(svc) + } + svc.initWorkflowEngine() + return svc, logs +} + +// --- AC-A10: a user cancel racing a claimed retry timer must never let the +// cancel's own suppressed delivery mark the marker shared with the timer's +// delivery. Driven sequentially per two independent contexts, not via a real +// goroutine race — the marker's value is what's contractual, not the +// interleaving that produces it. --- + +func TestDispatchKanbanAgentErrorTrigger_ConcurrentCancelDoesNotLeakMarker(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + t.Cleanup(svc.cancelAllTransientRetries) + + // Represents a retry timer that has already been scheduled and is about + // to reach retryTransientPrompt on its own. + svc.transientRetries.Store("s1", &transientRetryEntry{attempt: 1, cancel: func() {}}) + + if !svc.CancelTransientRetry(ctx, "t1", "s1") { + t.Fatal("CancelTransientRetry = false, want true (a loop was active)") + } + if decisions.clearCalls != 0 { + t.Fatalf("cancel's own delivery clearCalls = %d, want 0 (AC-A8 suppression)", decisions.clearCalls) + } + + // The claimed timer still reaches R4 on its own, unaffected context. Its + // event must not carry the cancel's UserInitiated marker. + svc.retryTransientPrompt(ctx, "t1", "s1", "exec-1") + + if decisions.clearCalls != 1 { + t.Fatalf("timer's own delivery clearCalls = %d, want 1 (AC-A8's marker must not leak into R4)", decisions.clearCalls) + } + // AC-A10 requires "at most one", never exactly one unconditionally: the + // cancel outrunning the timer at one of retryTransientPrompt's ctx.Err() + // checks is a legitimate zero-outcome variant of this same race (AC-B5). + // This drive pins the timer arriving, so it also happens to observe 1 + // here, but the assertion must state the actual invariant. + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) > 1 { + t.Fatalf("total dispatch INFO records across both deliveries = %d, want at most 1", len(got)) + } +} + +// --- AC-A4: on_agent_error must evaluate against the step the task landed on +// after handleRecoverableFailureLocked's own on_turn_complete reconciliation +// (a pending step-completion signal moving step1 -> step2), not the stale +// step1 the handler observed on entry. Reads the task fresh, after the +// decision that used the earlier snapshot. --- + +func TestDispatchKanbanAgentErrorTrigger_ReadsPostReconciliationStep(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") // seedSession leaves the session RUNNING. + + 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, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + seedPendingStepCompletionSignal(t, repo, "step1", "all done") + + svc.handleAgentFailed(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "agent crashed", + }) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("get task: %v", err) + } + if task.WorkflowStepID != "step2" { + t.Fatalf("expected on_turn_complete reconciliation to move task to step2, got %q", task.WorkflowStepID) + } + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (on_agent_error must fire against step2's declared action, not step1's)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("dispatched records = %d, want 1", len(got)) + } +} + +// --- AC-C7: a declined transition (target step fails to load) still reports +// a dispatch and, because the engine already marked the operation applied +// when it evaluated the action, is never retried on redelivery even though +// the underlying transition never landed. --- + +func TestDispatchKanbanAgentErrorTrigger_DeclinedTransitionIsNotRetried(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step-missing"}}, + }}, + } + stepGetter.getStepFunc = func(_ context.Context, id string) (*wfmodels.WorkflowStep, error) { + if id == "step-missing" { + return nil, errors.New("step not found") + } + return stepGetter.steps[id], nil + } + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + + data := watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom"} + svc.handleRecoverableFailureLocked(ctx, data) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Fatalf("WorkflowStepID = %q, want step1 (the transition must have been declined)", task.WorkflowStepID) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1 (dispatch still reports success)", len(got)) + } + + logs.TakeAll() + svc.handleRecoverableFailureLocked(ctx, data) + + task, err = repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Fatalf("WorkflowStepID = %q after redelivery, want step1 (still not retried)", task.WorkflowStepID) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("redelivery emitted %d dispatch record(s), want 0 (idempotent)", len(got)) + } +} + +// --- AC-A6/E2: a step declaring no on_agent_error actions at all — the +// overwhelmingly common case — logs at DEBUG only, never INFO. --- + +func TestDispatchKanbanAgentErrorTrigger_NoDeclaredActionsLogsDebugOnly(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + } + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if got := filterLogs(logs, msgAgentErrorNoActions); len(got) != 1 { + t.Fatalf("got %d %q DEBUG records, want 1", len(got), msgAgentErrorNoActions) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch INFO record, want none for a step with no declared actions") + } +} + +// --- AC-E3: taskDescription is pinned to the reloaded task's Description, so +// the prompt an auto_start_agent transition launches carries it. --- + +func TestDispatchKanbanAgentErrorTrigger_TaskDescriptionReachesLaunchedPrompt(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("get task: %v", err) + } + task.Description = "AC-E3 MARKER description" + if err := repo.UpdateTask(ctx, task); err != nil { + t.Fatalf("update task description: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step2"}}, + }}, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ + ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1, + Events: wfmodels.StepEvents{OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterAutoStartAgent}}}, + } + + seedExecutorRunning(t, repo, "s1", "t1", "exec-1") + + agentMgr := &mockAgentManager{isAgentRunning: true, repoForExecutionLookup: repo, promptDone: make(chan struct{})} + svc, _ := newAgentErrorTransientTestService(t, repo, stepGetter, agentMgr, nil) + svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + select { + case <-agentMgr.promptDone: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the auto-start prompt") + } + + agentMgr.mu.Lock() + got := agentMgr.capturedPrompts[0] + agentMgr.mu.Unlock() + if !strings.Contains(got, "AC-E3 MARKER description") { + t.Fatalf("launched prompt = %q, want it to contain the reloaded task's Description", got) + } +} + +// --- AC-E10: a move_to_step transition to a WIP-limited, full destination +// still applies (the task's WorkflowStepID moves) but on_enter dispatch is +// deferred — never fired — while the task sits queued. --- + +func TestDispatchKanbanAgentErrorTrigger_WIPLimitedDestinationDefersOnEnter(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + now := time.Now().UTC() + if err := repo.CreateTask(ctx, &models.Task{ + ID: "occupant", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step2", + Title: "Occupant", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create occupant: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step2"}}, + }}, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ + ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1, WIPLimit: 1, + Events: wfmodels.StepEvents{OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterAutoStartAgent}}}, + } + + svc, _ := newAgentErrorTestService(t, repo, stepGetter, nil) + onEnterCalled := make(chan struct{}, 1) + svc.onProcessOnEnterComplete = func() { + select { + case onEnterCalled <- struct{}{}: + default: + } + } + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step2" { + t.Fatalf("WorkflowStepID = %q, want step2 (the transition itself must still apply)", task.WorkflowStepID) + } + if task.WIPAdmitted { + t.Error("expected WIPAdmitted=false at a full-capacity destination") + } + if task.QueuedForStepID != "step2" { + t.Errorf("QueuedForStepID = %q, want step2", task.QueuedForStepID) + } + select { + case <-onEnterCalled: + t.Error("expected on_enter dispatch to be deferred for a WIP-queued destination") + case <-time.After(250 * time.Millisecond): + } +} + +// --- AC-E9: guard-gated and requires_approval transitions never apply +// automatically through on_agent_error. --- + +func TestDispatchKanbanAgentErrorTrigger_GuardsAndApproval(t *testing.T) { + ctx := context.Background() + + t.Run("unsatisfied wait_for_quorum guard blocks the transition", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{ + "step_id": "step2", + "if": map[string]interface{}{ + "wait_for_quorum": map[string]interface{}{"role": "reviewer", "threshold": "all_approve"}, + }, + }}, + }}, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + // engineParticipants is deliberately left unwired, so the guard + // evaluates unsatisfied rather than the transition applying. + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Fatalf("WorkflowStepID = %q, want step1 (unsatisfied guard must block the transition)", task.WorkflowStepID) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1 (the action is still declared and counted)", len(got)) + } + }) + + t.Run("requires_approval transition never applies automatically", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{ + "step_id": "step2", "requires_approval": true, + }}, + }}, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, nil) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Fatalf("WorkflowStepID = %q, want step1 (requires_approval must never apply automatically)", task.WorkflowStepID) + } + }) +} + +// --- AC-E14: the two remaining branches of the walk-failure matrix. --- + +func TestDispatchKanbanAgentErrorTrigger_WalkFailureAndEngineFailureLogsError(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + stepGetter.getStepFunc = func(context.Context, string) (*wfmodels.WorkflowStep, error) { + return nil, errors.New("step store unavailable") + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0 (the engine never reached the action list)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatchFailed); len(got) != 1 { + t.Fatalf("got %d %q ERROR records, want 1", len(got), msgAgentErrorDispatchFailed) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch INFO record, want none") + } + if got := filterLogs(logs, msgAgentErrorNoActions); len(got) != 0 { + t.Errorf("got %d %q DEBUG record(s), want none (AC-E5's ERROR and AC-E2's DEBUG are disjoint)", len(got), msgAgentErrorNoActions) + } +} + +func TestDispatchKanbanAgentErrorTrigger_RedeliveryIdempotentEngineNeverCallsLoadStep(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + data := watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom"} + svc.handleRecoverableFailureLocked(ctx, data) + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("first delivery: got %d INFO records, want 1", len(got)) + } + + // Force a genuine cache miss on redelivery. Without this, both the walk's + // own LoadStep and (if the engine wrongly called it too) the engine's + // LoadStep would be served from workflowStore's step cache populated by + // the first delivery, and a broken getter below would prove nothing about + // which caller actually reached it. + if err := svc.handleWorkflowStepCacheInvalidation(ctx, stepEvent(events.WorkflowStepUpdated, "step1", "wf1")); err != nil { + t.Fatalf("invalidate step cache: %v", err) + } + stepGetter.getStepFunc = func(context.Context, string) (*wfmodels.WorkflowStep, error) { + return nil, errors.New("step store unavailable") + } + callsBefore := stepGetter.GetStepCalls() + logs.TakeAll() + decisions.clearCalls = 0 + + svc.handleRecoverableFailureLocked(ctx, data) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0 (idempotent redelivery)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch INFO record, want none") + } + if got := filterLogs(logs, msgAgentErrorNoActions); len(got) != 0 { + t.Errorf("got a dispatch DEBUG record, want none (idempotent short-circuit, not an empty-actions dispatch)") + } + if got := filterLogs(logs, msgAgentErrorDispatchFailed); len(got) != 0 { + t.Errorf("got a dispatch-failed ERROR record, want none (the engine must not have re-run LoadStep)") + } + // Exactly one real GetStep call is expected here: the pre-engine walk's + // own LoadStep, which hits the now-broken getter on a genuine cache miss + // and fails silently (AC-E14). A second call would mean the engine's own + // loadExecutionContext/LoadStep ran despite the operation already being + // applied — the isOperationAlreadyApplied short-circuit AC-E14's third + // branch requires must run first. + if got := stepGetter.GetStepCalls() - callsBefore; got != 1 { + t.Errorf("GetStep calls on redelivery = %d, want exactly 1 (the walk's own call only; "+ + "the engine must not call LoadStep on an idempotent short-circuit)", got) + } +} + +// --- AC-F1: an absent or engine-nil snapshot (dispatch called before +// initWorkflowEngine ever populated agentErrorDeps) is a silent skip. --- + +func TestDispatchKanbanAgentErrorTrigger_NilDepsIsANoOp(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, stepGetter, newMockTaskRepo(), agentMgr) + core, logs := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("build observed logger: %v", err) + } + svc.logger = log + // Deliberately never call svc.initWorkflowEngine(): agentErrorDeps stays nil. + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + if len(logs.All()) != 0 { + t.Errorf("expected no log records before initWorkflowEngine, got %+v", logs.All()) + } + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Errorf("WorkflowStepID = %q, want step1 (unchanged)", task.WorkflowStepID) + } +} + +// --- AC-F8: overlapping guard pairs — a silent guard earlier in the fixed +// evaluation order must preempt a later recording guard's own record, even +// when the later guard's own condition (a genuinely blocking sibling +// session) is also true. --- + +func TestDispatchKanbanAgentErrorTrigger_OverlappingGuardPairs(t *testing.T) { + ctx := context.Background() + + baseStep := func() *wfmodels.WorkflowStep { + return &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + } + addBlockingSibling := func(t *testing.T, repo *sqliterepo.Repository, taskID string) { + t.Helper() + now := time.Now().UTC() + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s2", TaskID: taskID, State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create sibling session: %v", err) + } + } + + t.Run("AC-F4 archived preempts AC-F5's sibling-working record", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + addBlockingSibling(t, repo, "t1") + if err := repo.ArchiveTask(ctx, "t1"); err != nil { + t.Fatalf("archive task: %v", err) + } + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 0 { + t.Errorf("got %d %q records, want 0 (archived must preempt the F5 check entirely)", + len(got), msgAgentErrorAnotherSessionWorking) + } + }) + + t.Run("AC-F2 ephemeral preempts AC-F5's sibling-working record", func(t *testing.T) { + repo := setupTestRepo(t) + now := time.Now().UTC() + requireNoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateTask(ctx, &models.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + IsEphemeral: true, Title: "Ephemeral", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + })) + requireNoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s1", TaskID: "t1", State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + })) + addBlockingSibling(t, repo, "t1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 0 { + t.Errorf("got %d %q records, want 0 (ephemeral must preempt the F5 check entirely)", + len(got), msgAgentErrorAnotherSessionWorking) + } + }) + + t.Run("AC-F3 empty workflow step id preempts AC-F5's sibling-working record", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + addBlockingSibling(t, repo, "t1") + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("get task: %v", err) + } + task.WorkflowStepID = "" + if err := repo.UpdateTask(ctx, task); err != nil { + t.Fatalf("update task: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 0 { + t.Errorf("got %d %q records, want 0 (empty step id must preempt the F5 check entirely)", + len(got), msgAgentErrorAnotherSessionWorking) + } + }) +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_named_pairs_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_named_pairs_test.go new file mode 100644 index 0000000000..4add7ca6e3 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_named_pairs_test.go @@ -0,0 +1,126 @@ +package orchestrator + +// AC-F8's three named overlapping-guard-pair scenarios +// (docs/specs/workflow-on-agent-error-dispatch/spec.md:904-916), split into +// their own file because the sibling coverage test file is already near the +// repo's 800-effective-line test-file limit. + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/orchestrator/watcher" + "github.com/kandev/kandev/internal/task/models" + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +func namedPairBaseStep() *wfmodels.WorkflowStep { + return &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } +} + +// AC-A5 (silent) precedes AC-B2 (WARNING): a sessionless event never reads +// the task, so an unloadable task id produces no AC-B2 WARNING. +func TestDispatchKanbanAgentErrorTrigger_SessionlessEventOnUnloadableTaskNeverWarns(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + stepGetter := newMockStepGetter() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "does-not-exist", SessionID: ""}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorTaskLoadFailed); len(got) != 0 { + t.Errorf("got %d %q WARNING(s) for a sessionless event, want 0 (AC-A5 must skip before any task read)", + len(got), msgAgentErrorTaskLoadFailed) + } + if all := logs.All(); len(all) != 0 { + t.Errorf("expected no log records at all, got %+v", all) + } +} + +// AC-A8 (DEBUG) precedes AC-F4 (silent): a user-initiated cancel on an +// archived task still logs its own DEBUG, because the marker check returns +// before the archived-task shape guard ever runs. +func TestDispatchKanbanAgentErrorTrigger_UserInitiatedOnArchivedTaskStillLogsDebug(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + if err := repo.ArchiveTask(ctx, "t1"); err != nil { + t.Fatalf("archive task: %v", err) + } + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = namedPairBaseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", UserInitiated: true, + }) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorUserInitiated); len(got) != 1 { + t.Fatalf("got %d %q DEBUG record(s), want 1 (AC-A8 must fire before AC-F4's archived check)", + len(got), msgAgentErrorUserInitiated) + } +} + +// AC-F6 (WARNING) precedes AC-B2 (WARNING): a session reload failure on a +// task that is also unloadable still produces exactly one WARNING, and it is +// AC-F6's — the task is never read once the session read has already failed, +// pinning the session-before-task order. +func TestDispatchKanbanAgentErrorTrigger_SessionReadFailureOnUnloadableTaskIsOneWarning(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = namedPairBaseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + double := &agentErrorSessionAndTaskReadErrorRepo{ + sessionExecutorStore: svc.repo, + sessionErr: errors.New("session db timeout"), + taskErr: errors.New("task db timeout"), + } + svc.repo = double + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorSessionReloadFailed); len(got) != 1 { + t.Fatalf("got %d %q WARNING(s), want 1 (AC-F6)", len(got), msgAgentErrorSessionReloadFailed) + } + if got := filterLogs(logs, msgAgentErrorTaskLoadFailed); len(got) != 0 { + t.Errorf("got %d %q WARNING(s), want 0 (AC-B2 must not also fire)", len(got), msgAgentErrorTaskLoadFailed) + } + if double.getTaskCalls != 0 { + t.Errorf("GetTask called %d time(s), want 0 (task must never be read once the session read has failed)", + double.getTaskCalls) + } +} + +type agentErrorSessionAndTaskReadErrorRepo struct { + sessionExecutorStore + sessionErr error + taskErr error + getTaskCalls int +} + +func (r *agentErrorSessionAndTaskReadErrorRepo) GetTaskSession(context.Context, string) (*models.TaskSession, error) { + return nil, r.sessionErr +} + +func (r *agentErrorSessionAndTaskReadErrorRepo) GetTask(context.Context, string) (*models.Task, error) { + r.getTaskCalls++ + return nil, r.taskErr +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_panic_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_panic_test.go new file mode 100644 index 0000000000..7602d7672e --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_panic_test.go @@ -0,0 +1,99 @@ +package orchestrator + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/orchestrator/watcher" + "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +// A panicking dispatch does not take the process down. Routes R2-R5 never +// reach the event bus's own recover-and-log wrapper, and this dispatch now +// runs synchronously (after the session guard is released, not on a +// goroutine of its own), so nothing above it on the call stack recovers a +// panicking callback unless dispatchKanbanAgentErrorTriggerRecovered does. +// The panic is injected through a registered callback's Execute, the only +// available seam — not a stub engine. +func TestDispatchKanbanAgentErrorTrigger_RecoversFromPanickingCallback(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + installPanickingAgentErrorCallback(t, svc, engine.ActionClearDecisions) + + // handleAgentFailed is a real guard-holding production entry point, not a + // direct call to the dispatch function, so this exercises the actual + // call chain: lock -> handleRecoverableFailureLockedState -> unlock -> + // dispatchKanbanAgentErrorTriggerRecovered. + handlerReturned := make(chan struct{}) + go func() { + svc.handleAgentFailed(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + close(handlerReturned) + }() + + select { + case <-handlerReturned: + case <-time.After(3 * time.Second): + t.Fatal("handleAgentFailed did not return after a panicking callback: the panic escaped recovery") + } + + errs := filterLogs(logs, msgAgentErrorDispatchPanicked) + if len(errs) != 1 { + t.Fatalf("got %d %q ERROR record(s), want 1 (all: %+v)", len(errs), msgAgentErrorDispatchPanicked, logs.All()) + } + fields := errs[0].ContextMap() + if fields["task_id"] != "t1" || fields["session_id"] != "s1" { + t.Errorf("unexpected identity fields: %+v", fields) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got %d dispatch INFO record(s), want 0 (the panic ERROR is not a dispatch record)", len(got)) + } +} + +// installPanickingAgentErrorCallback swaps svc's agentErrorDeps for a copy +// whose registry resolves kind to a callback that panics on Execute, +// mirroring agentErrorCapturePayload's swap-the-registry approach. Must run +// after svc.initWorkflowEngine() (newAgentErrorTestService already calls it). +func installPanickingAgentErrorCallback(t *testing.T, svc *Service, kind engine.ActionKind) { + t.Helper() + deps := svc.agentErrorDeps.Load() + if deps == nil { + t.Fatal("installPanickingAgentErrorCallback: agentErrorDeps not initialized") + } + registry := &panickingCallbackRegistry{inner: deps.registry, kind: kind} + options := append([]engine.Option{engine.WithLogger(svc.logger)}, svc.engineOptions...) + svc.agentErrorDeps.Store(&agentErrorDispatchDeps{ + engine: engine.New(deps.store, registry, options...), + registry: registry, + store: deps.store, + }) +} + +type panickingCallbackRegistry struct { + inner engine.CallbackRegistry + kind engine.ActionKind +} + +func (r *panickingCallbackRegistry) Get(kind engine.ActionKind) (engine.ActionCallback, bool) { + if kind == r.kind { + return panickingCallback{}, true + } + return r.inner.Get(kind) +} + +type panickingCallback struct{} + +func (panickingCallback) Execute(context.Context, engine.ActionInput) (engine.ActionResult, error) { + panic("installPanickingAgentErrorCallback: injected test panic") +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_payload_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_payload_test.go new file mode 100644 index 0000000000..5ef87a1585 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_payload_test.go @@ -0,0 +1,200 @@ +package orchestrator + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/orchestrator/watcher" + "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +// agentErrorCapturePayload swaps svc's agentErrorDeps for a copy whose +// registry wraps kind's real callback so the test can observe the +// engine.OnAgentErrorPayload the engine actually delivers, rather than the +// value the dispatch computed before calling it (AC-D1/D2/D3). Must run +// after svc.initWorkflowEngine() (or newAgentErrorTestService / +// newAgentErrorTransientTestService, which call it). The wrapped callback +// still forwards to the real one, so its side effects (e.g. clearCalls) +// are unaffected. +func agentErrorCapturePayload(t *testing.T, svc *Service, kind engine.ActionKind) *[]engine.OnAgentErrorPayload { + t.Helper() + deps := svc.agentErrorDeps.Load() + if deps == nil { + t.Fatal("agentErrorCapturePayload: agentErrorDeps not initialized") + } + captured := &[]engine.OnAgentErrorPayload{} + registry := &payloadCapturingRegistry{inner: deps.registry, kind: kind, captured: captured} + options := append([]engine.Option{engine.WithLogger(svc.logger)}, svc.engineOptions...) + svc.agentErrorDeps.Store(&agentErrorDispatchDeps{ + engine: engine.New(deps.store, registry, options...), + registry: registry, + store: deps.store, + }) + return captured +} + +type payloadCapturingRegistry struct { + inner engine.CallbackRegistry + kind engine.ActionKind + captured *[]engine.OnAgentErrorPayload +} + +func (r *payloadCapturingRegistry) Get(kind engine.ActionKind) (engine.ActionCallback, bool) { + cb, ok := r.inner.Get(kind) + if !ok || kind != r.kind { + return cb, ok + } + return payloadCapturingCallback{inner: cb, captured: r.captured}, true +} + +type payloadCapturingCallback struct { + inner engine.ActionCallback + captured *[]engine.OnAgentErrorPayload +} + +func (c payloadCapturingCallback) Execute(ctx context.Context, in engine.ActionInput) (engine.ActionResult, error) { + if p, ok := in.Payload.(engine.OnAgentErrorPayload); ok { + *c.captured = append(*c.captured, p) + } + return c.inner.Execute(ctx, in) +} + +// --- AC-D1/D2/D3: the payload the engine actually receives. No test +// anywhere previously observed OnAgentErrorPayload's fields. --- + +func TestDispatchKanbanAgentErrorTrigger_PayloadFields(t *testing.T) { + ctx := context.Background() + + t.Run("AC-D1/D3: FailedSessionID and an explicit ErrorMessage pass through unchanged", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + got := (*captured)[0] + if got.FailedSessionID != "s1" { + t.Errorf("FailedSessionID = %q, want s1", got.FailedSessionID) + } + if got.ErrorMessage != "boom" { + t.Errorf("ErrorMessage = %q, want boom", got.ErrorMessage) + } + }) + + t.Run("AC-D3: an empty ErrorMessage defaults to the literal agent failed", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", + }) + + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + if got := (*captured)[0].ErrorMessage; got != "agent failed" { + t.Errorf("ErrorMessage = %q, want the default %q", got, "agent failed") + } + }) + + t.Run("AC-D2: an explicit event AgentProfileID is used directly", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", AgentProfileID: "profile-event", + }) + + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + if got := (*captured)[0].FailedAgentID; got != "profile-event" { + t.Errorf("FailedAgentID = %q, want the event's own profile-event", got) + } + }) + + t.Run("AC-D2: an empty event AgentProfileID falls back to the session's own", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("get session: %v", err) + } + session.AgentProfileID = "profile-session" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", + }) + + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + if got := (*captured)[0].FailedAgentID; got != "profile-session" { + t.Errorf("FailedAgentID = %q, want the session's fallback profile-session", got) + } + }) + + t.Run("AC-D2: both empty yields an empty FailedAgentID", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", + }) + + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + if got := (*captured)[0].FailedAgentID; got != "" { + t.Errorf("FailedAgentID = %q, want empty (neither event nor session set one)", got) + } + }) +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_routes_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_routes_test.go new file mode 100644 index 0000000000..e647a90fe5 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_routes_test.go @@ -0,0 +1,214 @@ +package orchestrator + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/kandev/kandev/internal/agent/runtime/routingerr" + "github.com/kandev/kandev/internal/orchestrator/watcher" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +// --- AC-A1 route coverage: all five production fire sites reach the real +// dispatch, with the workflow engine actually wired (agentErrorDeps +// populated). R4/R5 previously only exercised via newTransientTestService, +// which never calls initWorkflowEngine — this closed that gap. Each also +// asserts the payload the engine receives (AC-D1/D2/D3), per this spec's +// Verification text for AC-A1/A2. --- + +func TestDispatchKanbanAgentErrorTrigger_R1BusDrivenFailureDispatches(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.handleAgentFailed(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "agent crashed", + }) + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (R1's bus-driven entry point must reach the real dispatch)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1", len(got)) + } + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + if got := (*captured)[0]; got.FailedSessionID != "s1" || got.ErrorMessage != "agent crashed" { + t.Errorf("payload = %+v, want FailedSessionID=s1 ErrorMessage=%q", got, "agent crashed") + } +} + +func TestDispatchKanbanAgentErrorTrigger_R2ManagedRuntimeNpmFailureDispatches(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + npmErr := fmt.Errorf("failed to initialize ACP: %w", &routingerr.ManagedRuntimeStartupError{ + Code: routingerr.CodeManagedRuntimeNpmResolution, + Details: "npm error code ETARGET", + }) + handled := svc.handleAgentStartFailed(ctx, "t1", "s1", "exec-1", npmErr, false) + + if !handled { + t.Fatal("expected handled=true for a managed npm runtime startup failure") + } + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (R2's npm-resolution branch must reach the real dispatch)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1", len(got)) + } + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + wantMsg := "managed npm runtime failed to prepare" + if got := (*captured)[0]; got.FailedSessionID != "s1" || got.ErrorMessage != wantMsg { + t.Errorf("payload = %+v, want FailedSessionID=s1 ErrorMessage=%q", got, wantMsg) + } +} + +func TestDispatchKanbanAgentErrorTrigger_R3AuthErrorDispatches(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + handled := svc.handleAgentStartFailed( + ctx, "t1", "s1", "exec-1", errors.New("authentication required: please log in"), false, + ) + + if !handled { + t.Fatal("expected handled=true for an auth-error start failure") + } + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (R3's auth-error branch must reach the real dispatch)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1", len(got)) + } + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + wantMsg := "authentication required: please log in" + if got := (*captured)[0]; got.FailedSessionID != "s1" || got.ErrorMessage != wantMsg { + t.Errorf("payload = %+v, want FailedSessionID=s1 ErrorMessage=%q", got, wantMsg) + } +} + +func TestDispatchKanbanAgentErrorTrigger_R4NoCachedPromptDispatches(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTransientTestService(t, repo, stepGetter, agentMgr, func(s *Service) { + s.engineDecisions = decisions + }) + t.Cleanup(svc.cancelAllTransientRetries) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.scheduleTransientRetry("t1", "s1", "", 1, time.Hour) + svc.retryTransientPrompt(ctx, "t1", "s1", "") + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (R4's no-cached-prompt path must reach the real dispatch)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1", len(got)) + } + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + wantMsg := "Automatic provider retry was not possible. Resume or start fresh to continue." + if got := (*captured)[0]; got.FailedSessionID != "s1" || got.ErrorMessage != wantMsg { + t.Errorf("payload = %+v, want FailedSessionID=s1 ErrorMessage=%q", got, wantMsg) + } +} + +func TestDispatchKanbanAgentErrorTrigger_R5SynchronousPromptErrorDispatches(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("get session: %v", err) + } + session.State = models.TaskSessionStateWaitingForInput + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + seedExecutorRunning(t, repo, "s1", "t1", "exec-1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + agentMgr := &mockAgentManager{ + repoForExecutionLookup: repo, + promptErr: errors.New("session rejected prompt synchronously"), + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTransientTestService(t, repo, stepGetter, agentMgr, func(s *Service) { + s.engineDecisions = decisions + }) + t.Cleanup(svc.cancelAllTransientRetries) + captured := agentErrorCapturePayload(t, svc, engine.ActionClearDecisions) + + svc.rememberTurnPrompt("s1", "hello", "", false, nil) + svc.transientRetries.Store("s1", &transientRetryEntry{attempt: 1, cancel: func() {}}) + + svc.retryTransientPrompt(ctx, "t1", "s1", "exec-1") + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (R5's synchronous PromptTask failure must reach the real dispatch)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatch INFO records, want 1", len(got)) + } + if len(*captured) != 1 { + t.Fatalf("got %d payload(s), want 1", len(*captured)) + } + wantMsg := "Automatic provider retry could not be started. Resume or start fresh to continue." + if got := (*captured)[0]; got.FailedSessionID != "s1" || got.ErrorMessage != wantMsg { + t.Errorf("payload = %+v, want FailedSessionID=s1 ErrorMessage=%q", got, wantMsg) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent_error_test.go b/apps/backend/internal/orchestrator/event_handlers_agent_error_test.go new file mode 100644 index 0000000000..6f1c54705e --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_agent_error_test.go @@ -0,0 +1,891 @@ +package orchestrator + +import ( + "context" + "errors" + "testing" + "time" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/orchestrator/watcher" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + + "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// newAgentErrorTestService builds a Service wired against a real engine +// (svc.initWorkflowEngine) with an observed logger so tests can assert on +// this dispatch's own log records. configure runs before initWorkflowEngine +// so callback dependencies (engineDecisions, etc.) are picked up by +// buildWorkflowCallbacks. +func newAgentErrorTestService( + t *testing.T, repo *sqliterepo.Repository, stepGetter *mockStepGetter, configure func(*Service), +) (*Service, *observer.ObservedLogs) { + t.Helper() + // handleRecoverableFailureLocked's last-but-one step (before this card's + // dispatch) fires a background cleanupAgentExecution that dereferences + // svc.executor — createTestServiceWithScheduler is the fixture that wires + // one, unlike the bare createTestService used elsewhere in this package. + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, stepGetter, newMockTaskRepo(), agentMgr) + core, logs := observer.New(zapcore.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("build observed logger: %v", err) + } + svc.logger = log + if configure != nil { + configure(svc) + } + svc.initWorkflowEngine() + return svc, logs +} + +func filterLogs(logs *observer.ObservedLogs, msg string) []observer.LoggedEntry { + var out []observer.LoggedEntry + for _, e := range logs.All() { + if e.Message == msg { + out = append(out, e) + } + } + return out +} + +func TestHandleRecoverableFailureDispatchesAfterSessionGuardRelease(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{ + OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionAutoStartAgent}}, + }, + } + svc, _ := newAgentErrorTestService(t, repo, stepGetter, nil) + + callback := &guardReacquiringAgentErrorCallback{svc: svc, done: make(chan struct{})} + registry := engine.MapRegistry{engine.ActionAutoStartAgent: callback} + workflowEngine := engine.New(svc.workflowStore, registry) + svc.workflowEngine = workflowEngine + svc.agentErrorDeps.Store(&agentErrorDispatchDeps{ + engine: workflowEngine, + registry: registry, + store: svc.workflowStore, + }) + + finished := make(chan struct{}) + go func() { + svc.handleRecoverableFailure(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", + }) + close(finished) + }() + + select { + case <-finished: + case <-time.After(time.Second): + t.Fatal("recoverable failure did not finish while dispatching auto_start_agent") + } + select { + case <-callback.done: + case <-time.After(time.Second): + t.Fatal("deferred on_agent_error callback did not complete") + } +} + +type guardReacquiringAgentErrorCallback struct { + svc *Service + done chan struct{} +} + +func (c *guardReacquiringAgentErrorCallback) Execute( + _ context.Context, in engine.ActionInput, +) (engine.ActionResult, error) { + lock, release := c.svc.acquireCancelInFlightGuard(in.State.SessionID) + defer release() + lock.Lock() + close(c.done) + lock.Unlock() + return engine.ActionResult{}, nil +} + +// --- AC-A1/A6/C1/C3: dispatch fires, records exactly one INFO, and a +// redelivery of the same failure is idempotent (no re-run, no record). --- + +func TestDispatchKanbanAgentErrorTrigger_ClearDecisionsDispatchesAndIsIdempotent(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{ + OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionClearDecisions}, + }, + }, + } + + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { + s.engineDecisions = decisions + }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1", decisions.clearCalls) + } + infos := filterLogs(logs, msgAgentErrorDispatched) + if len(infos) != 1 { + t.Fatalf("got %d %q records, want 1 (all: %+v)", len(infos), msgAgentErrorDispatched, logs.All()) + } + fields := infos[0].ContextMap() + if fields["task_id"] != "t1" || fields["session_id"] != "s1" || fields["step_id"] != "step1" { + t.Errorf("unexpected identity fields: %+v", fields) + } + if fields["operation_id"] != "agent_error:session:s1:exec-1" { + t.Errorf("operation_id = %v, want agent_error:session:s1:exec-1", fields["operation_id"]) + } + if got := filterLogs(logs, msgAgentErrorNoActions); len(got) != 0 { + t.Errorf("first delivery emitted %d %q record(s), want 0 (AC-A6's INFO and AC-E2's DEBUG are disjoint)", len(got), msgAgentErrorNoActions) + } + + // AC-C3: a redelivery of the exact same failure must not re-run the + // action list and must emit no dispatch record of its own. + decisions.clearCalls = 0 + logs.TakeAll() + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + if decisions.clearCalls != 0 { + t.Errorf("redelivery clearCalls = %d, want 0 (idempotent)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("redelivery emitted %d %q record(s), want 0", len(got), msgAgentErrorDispatched) + } + if got := filterLogs(logs, msgAgentErrorNoActions); len(got) != 0 { + t.Errorf("redelivery emitted %d %q record(s), want 0 (idempotent short-circuit, not an empty-actions dispatch)", len(got), msgAgentErrorNoActions) + } +} + +// --- AC-C2/C4/C8: idempotency key shape, distinct executions both fire, +// empty-AgentExecutionID collapses to one key. --- + +func TestDispatchKanbanAgentErrorTrigger_IdempotencyKeyShape(t *testing.T) { + ctx := context.Background() + + t.Run("two distinct executions on one session both dispatch", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-2"}) + + if decisions.clearCalls != 2 { + t.Fatalf("clearCalls = %d, want 2 (distinct executions must not collapse)", decisions.clearCalls) + } + }) + + t.Run("two failures with empty AgentExecutionID collapse to one dispatch", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1"}) + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1"}) + + if decisions.clearCalls != 1 { + t.Fatalf("clearCalls = %d, want 1 (empty execution ids collapse to one key)", decisions.clearCalls) + } + infos := filterLogs(logs, msgAgentErrorDispatched) + if len(infos) != 1 { + t.Fatalf("got %d %q record(s), want 1", len(infos), msgAgentErrorDispatched) + } + if got := infos[0].ContextMap()["operation_id"]; got != "agent_error:session:s1" { + t.Errorf("operation_id = %v, want agent_error:session:s1 (AC-C2's empty-execution-id shape)", got) + } + }) +} + +// --- AC-C6: an action that errors leaves the operation unmarked, so a +// redelivery re-runs the whole list from the start. --- + +func TestDispatchKanbanAgentErrorTrigger_ActionErrorLeavesUnmarkedAndRetries(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &failingThenSucceedingDecisionStore{failCount: 1} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + data := watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"} + svc.handleRecoverableFailureLocked(ctx, data) + if decisions.calls != 1 { + t.Fatalf("first delivery calls = %d, want 1", decisions.calls) + } + if got := filterLogs(logs, msgAgentErrorDispatchFailed); len(got) != 1 { + t.Fatalf("got %d ERROR records for the failed first delivery, want 1", len(got)) + } + + logs.TakeAll() + svc.handleRecoverableFailureLocked(ctx, data) + if decisions.calls != 2 { + t.Fatalf("redelivery calls = %d, want 2 (unmarked operation reruns the list)", decisions.calls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("got %d dispatched INFO records on the successful redelivery, want 1", len(got)) + } +} + +type failingThenSucceedingDecisionStore struct { + spyDecisionStore + calls int + failCount int +} + +func (d *failingThenSucceedingDecisionStore) ClearStepDecisions(ctx context.Context, taskID, stepID string) (int64, error) { + d.calls++ + if d.calls <= d.failCount { + return 0, errors.New("transient decision store failure") + } + return d.spyDecisionStore.ClearStepDecisions(ctx, taskID, stepID) +} + +// --- AC-E3/E8/E10/E11: move_to_step applies the transition through +// applyEngineTransition — on_enter is dispatched (launchProcessOnEnter always +// fires onProcessOnEnterComplete, even with an empty on_enter list, which is +// what this test synchronizes on) and the session_step_history ledger row +// carries the new on_agent_error trigger label rather than auto_complete. --- + +func TestDispatchKanbanAgentErrorTrigger_MoveToStepAppliesTransition(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{ + OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step2"}}, + }, + }, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1} + + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + taskRepo := newMockTaskRepo() + seedMockTaskState(taskRepo, "t1", v1.TaskStateInProgress) + svc := createTestServiceWithScheduler(repo, stepGetter, taskRepo, agentMgr) + recorder := &fakeStepHistoryRecorder{} + svc.stepHistoryRecorder = recorder + onEnterDone := make(chan struct{}, 1) + svc.onProcessOnEnterComplete = func() { + select { + case onEnterDone <- struct{}{}: + default: + } + } + svc.initWorkflowEngine() + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{ + TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1", ErrorMessage: "boom", + }) + + select { + case <-onEnterDone: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for on_enter dispatch after the on_agent_error move") + } + + updated, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if updated.WorkflowStepID != "step2" { + t.Fatalf("task WorkflowStepID = %q, want step2", updated.WorkflowStepID) + } + + calls := recorder.Calls() + if len(calls) != 1 { + t.Fatalf("got %d session_step_history calls, want 1: %+v", len(calls), calls) + } + if calls[0].trigger != wfmodels.StepTransitionTriggerAgentError { + t.Errorf("session_step_history trigger = %q, want %q", calls[0].trigger, wfmodels.StepTransitionTriggerAgentError) + } + if calls[0].fromStepID != "step1" || calls[0].toStepID != "step2" { + t.Errorf("session_step_history from/to = %q/%q, want step1/step2", calls[0].fromStepID, calls[0].toStepID) + } +} + +// --- AC-B1/B2: an Office-owned task never dispatches through this path, +// and a task-load failure produces zero dispatch (fail closed). --- + +func TestDispatchKanbanAgentErrorTrigger_OfficeAndLoadFailureGuards(t *testing.T) { + ctx := context.Background() + + t.Run("office-owned task does not dispatch", func(t *testing.T) { + repo := setupTestRepo(t) + now := time.Now().UTC() + requireNoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateTask(ctx, &models.Task{ + ID: "t-office", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + ProjectID: "proj1", Title: "Office Task", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + })) + requireNoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s-office", TaskID: "t-office", State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + })) + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t-office", SessionID: "s-office", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0 (Office task must not dispatch through this path)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got %d dispatch INFO records for an Office task, want 0", len(got)) + } + }) + + t.Run("task load failure produces zero dispatch and one WARNING", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + svc.repo = &agentErrorTaskLoadErrorRepo{sessionExecutorStore: svc.repo, err: errors.New("db down")} + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorTaskLoadFailed); len(got) != 1 { + t.Fatalf("got %d %q WARNINGs, want 1", len(got), msgAgentErrorTaskLoadFailed) + } + }) +} + +type agentErrorTaskLoadErrorRepo struct { + sessionExecutorStore + err error +} + +func (r *agentErrorTaskLoadErrorRepo) GetTask(_ context.Context, _ string) (*models.Task, error) { + return nil, r.err +} + +// --- AC-A5/A7/A8/F2/F3/F4/F5/F6/B5: the guard sequence. --- + +func TestDispatchKanbanAgentErrorTrigger_Guards(t *testing.T) { + ctx := context.Background() + + baseStep := func() *wfmodels.WorkflowStep { + return &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + } + + t.Run("AC-A5 empty session id never dispatches, no record", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: ""}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if len(logs.All()) != 0 { + t.Errorf("expected no log records at all, got %+v", logs.All()) + } + }) + + t.Run("AC-A8 user-initiated cancel never dispatches, logs DEBUG", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.scheduleTransientRetry("t1", "s1", "exec-1", 1, 5*time.Second) + t.Cleanup(svc.cancelAllTransientRetries) + + if !svc.CancelTransientRetry(ctx, "t1", "s1") { + t.Fatal("CancelTransientRetry = false, want true (a loop was active)") + } + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0 (a user cancel must not dispatch on_agent_error)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorUserInitiated); len(got) != 1 { + t.Fatalf("got %d %q DEBUG records, want 1", len(got), msgAgentErrorUserInitiated) + } + }) + + t.Run("AC-A9 a claimed retry timer that reaches R4/R5 still dispatches", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, _ := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailure(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 1 { + t.Errorf("clearCalls = %d, want 1 (a non-user-initiated recoverable failure must dispatch)", decisions.clearCalls) + } + }) + + t.Run("AC-A7 vanished session is a DEBUG no-op", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "does-not-exist", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorSessionVanished); len(got) != 1 { + t.Fatalf("got %d %q DEBUG records, want 1", len(got), msgAgentErrorSessionVanished) + } + }) + + t.Run("AC-F6 non-not-found session reload error is a WARNING", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + svc.repo = &agentErrorSessionReloadErrorRepo{sessionExecutorStore: svc.repo, err: errors.New("db timeout")} + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorSessionReloadFailed); len(got) != 1 { + t.Fatalf("got %d %q WARNINGs, want 1", len(got), msgAgentErrorSessionReloadFailed) + } + }) + + t.Run("AC-F2/F3/F4 ephemeral, empty step id, and archived tasks skip silently", func(t *testing.T) { + // IsEphemeral is create-only (UpdateTask's column list omits it) and + // archived_at only moves through ArchiveTask, so each case seeds its + // own repo state through the real write path rather than mutating a + // loaded *models.Task and calling UpdateTask, which would silently + // leave both columns unchanged and pass for the wrong reason. + t.Run("ephemeral", func(t *testing.T) { + repo := setupTestRepo(t) + now := time.Now().UTC() + requireNoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now})) + requireNoError(t, repo.CreateTask(ctx, &models.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + IsEphemeral: true, Title: "Ephemeral", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + })) + requireNoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s1", TaskID: "t1", State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + })) + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch record, want none") + } + }) + + t.Run("empty workflow step id", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("get task: %v", err) + } + task.WorkflowStepID = "" + if err := repo.UpdateTask(ctx, task); err != nil { + t.Fatalf("update task: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch record, want none") + } + }) + + t.Run("archived", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + if err := repo.ArchiveTask(ctx, "t1"); err != nil { + t.Fatalf("archive task: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("got a dispatch record, want none") + } + }) + }) + + t.Run("AC-F5 another working session blocks dispatch, logs DEBUG", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + now := time.Now().UTC() + requireNoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s2", TaskID: "t1", State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + })) + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0 (a sibling session is still working)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 1 { + t.Fatalf("got %d %q DEBUG records, want 1", len(got), msgAgentErrorAnotherSessionWorking) + } + }) + + t.Run("AC-F6 otherWorkingSessionID failure adds no record of its own", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = baseStep() + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + svc.repo = &agentErrorListSessionsErrorRepo{sessionExecutorStore: svc.repo, err: errors.New("list failure")} + + svc.dispatchKanbanAgentErrorTrigger(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if decisions.clearCalls != 0 { + t.Errorf("clearCalls = %d, want 0", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 0 { + t.Errorf("dispatch emitted its own record for an otherWorkingSessionID failure, want none: %+v", got) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("expected no dispatch on an otherWorkingSessionID failure") + } + }) +} + +// --- AC-F7: a guard skip must not mark the operation applied, so a later +// delivery of the exact same failure can still dispatch once the guard +// condition that blocked it clears. --- + +func TestDispatchKanbanAgentErrorTrigger_GuardSkipDoesNotSuppressLaterValidDispatch(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + now := time.Now().UTC() + requireNoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "s2", TaskID: "t1", State: models.TaskSessionStateRunning, StartedAt: now, UpdatedAt: now, + })) + + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + data := watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"} + + // s2 is still working, so AC-F5 skips before the engine is ever called — + // the operation must never be marked applied by this skip. + svc.dispatchKanbanAgentErrorTrigger(ctx, data) + if decisions.clearCalls != 0 { + t.Fatalf("first delivery clearCalls = %d, want 0 (blocked by sibling session)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorAnotherSessionWorking); len(got) != 1 { + t.Fatalf("got %d %q DEBUG records, want 1", len(got), msgAgentErrorAnotherSessionWorking) + } + + // s2 settles; the exact same failure (same session, same execution id, + // same operation id) redelivers and must now dispatch. + s2, err := repo.GetTaskSession(ctx, "s2") + if err != nil { + t.Fatalf("get session: %v", err) + } + s2.State = models.TaskSessionStateWaitingForInput + if err := repo.UpdateTaskSession(ctx, s2); err != nil { + t.Fatalf("update session: %v", err) + } + logs.TakeAll() + + svc.dispatchKanbanAgentErrorTrigger(ctx, data) + if decisions.clearCalls != 1 { + t.Errorf("second delivery clearCalls = %d, want 1 (the earlier guard skip must not have marked the operation applied)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Errorf("got %d dispatch INFO records, want 1", len(got)) + } +} + +type agentErrorSessionReloadErrorRepo struct { + sessionExecutorStore + err error +} + +func (r *agentErrorSessionReloadErrorRepo) GetTaskSession(_ context.Context, _ string) (*models.TaskSession, error) { + return nil, r.err +} + +type agentErrorListSessionsErrorRepo struct { + sessionExecutorStore + err error +} + +func (r *agentErrorListSessionsErrorRepo) ListTaskSessions(_ context.Context, _ string) ([]*models.TaskSession, error) { + return nil, r.err +} + +// --- AC-E4/E7/E12/E13/E14: the pre-engine walk. --- + +func TestDispatchKanbanAgentErrorTrigger_ActionVocabularyWarn(t *testing.T) { + ctx := context.Background() + + t.Run("unregistered action warns with all six fields, dispatch still proceeds", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionQueueRun}}}, + } + // No RunQueueAdapter wired, so queue_run has no registered callback. + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + warnings := filterLogs(logs, msgAgentErrorActionUnregistered) + if len(warnings) != 1 { + t.Fatalf("got %d %q WARNINGs, want 1 (all: %+v)", len(warnings), msgAgentErrorActionUnregistered, logs.All()) + } + fields := warnings[0].ContextMap() + wantFields := map[string]interface{}{ + "workflow_id": "wf1", "step_id": "step1", "step_name": "Step 1", + "action_type": "queue_run", "task_id": "t1", "session_id": "s1", + } + for key, want := range wantFields { + if fields[key] != want { + t.Errorf("field %q = %v, want %v", key, fields[key], want) + } + } + // Since queue_run has no adapter, HandleTrigger reports it as an + // action with no effect: ActionCount > 0 but nothing executed, so + // this is the AC-A6 INFO dispatch record, not AC-E2's DEBUG. Both the + // walk WARNING above and this INFO must be present together — the + // pair is what AC-E4/AC-A6's "does not count against at most one" + // carve-out actually requires proving. + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Errorf("got %d %q INFO record(s), want 1 (dispatch must still proceed alongside the walk WARNING)", len(got), msgAgentErrorDispatched) + } + }) + + t.Run("two move_to_step actions produce no warning", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step2"}}, + {Type: wfmodels.GenericActionMoveToStep, Config: map[string]interface{}{"step_id": "step3"}}, + }}, + } + stepGetter.steps["step2"] = &wfmodels.WorkflowStep{ID: "step2", WorkflowID: "wf1", Name: "Step 2", Position: 1} + stepGetter.steps["step3"] = &wfmodels.WorkflowStep{ID: "step3", WorkflowID: "wf1", Name: "Step 3", Position: 2} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if got := filterLogs(logs, msgAgentErrorActionUnregistered); len(got) != 0 { + t.Errorf("got %d WARNING(s) for move_to_step actions, want 0: %+v", len(got), got) + } + updated, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if updated.WorkflowStepID != "step2" { + t.Errorf("WorkflowStepID = %q, want step2 (first eligible transition wins)", updated.WorkflowStepID) + } + }) + + t.Run("idempotent redelivery still warns but emits no dispatch record", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{ + {Type: wfmodels.GenericActionClearDecisions}, + {Type: wfmodels.GenericActionQueueRun}, + }}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + data := watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"} + svc.handleRecoverableFailureLocked(ctx, data) + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Fatalf("first delivery: got %d INFO records, want 1", len(got)) + } + + logs.TakeAll() + svc.handleRecoverableFailureLocked(ctx, data) + if got := filterLogs(logs, msgAgentErrorActionUnregistered); len(got) != 1 { + t.Errorf("redelivery: got %d WARNINGs, want 1 (walk runs before the idempotency short-circuit)", len(got)) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 0 { + t.Errorf("redelivery: got a dispatch record, want none (idempotent)") + } + }) + + t.Run("a Set* call between construction and dispatch is picked up (AC-E12/E13)", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + stepGetter.steps["step1"] = &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, nil) // not wired yet + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + if got := filterLogs(logs, msgAgentErrorActionUnregistered); len(got) != 1 { + t.Fatalf("before wiring: got %d WARNINGs, want 1", len(got)) + } + if decisions.clearCalls != 0 { + t.Fatalf("before wiring: clearCalls = %d, want 0", decisions.clearCalls) + } + + logs.TakeAll() + svc.SetEngineDecisionStore(decisions) // triggers reinitWorkflowEngine + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-2"}) + if got := filterLogs(logs, msgAgentErrorActionUnregistered); len(got) != 0 { + t.Errorf("after wiring: got %d WARNINGs, want 0 (clear_decisions is now registered)", len(got)) + } + if decisions.clearCalls != 1 { + t.Errorf("after wiring: clearCalls = %d, want 1", decisions.clearCalls) + } + }) + + t.Run("AC-E14: a failed walk LoadStep does not block dispatch", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + stepGetter := newMockStepGetter() + realStep := &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Step 1", Position: 0, + Events: wfmodels.StepEvents{OnAgentError: []wfmodels.GenericAction{{Type: wfmodels.GenericActionClearDecisions}}}, + } + stepGetter.steps["step1"] = realStep + calls := 0 + stepGetter.getStepFunc = func(_ context.Context, id string) (*wfmodels.WorkflowStep, error) { + calls++ + if calls == 1 { + return nil, errors.New("transient step store failure") + } + return stepGetter.steps[id], nil + } + decisions := &spyDecisionStore{} + svc, logs := newAgentErrorTestService(t, repo, stepGetter, func(s *Service) { s.engineDecisions = decisions }) + + svc.handleRecoverableFailureLocked(ctx, watcher.AgentEventData{TaskID: "t1", SessionID: "s1", AgentExecutionID: "exec-1"}) + + if got := filterLogs(logs, msgAgentErrorActionUnregistered); len(got) != 0 { + t.Errorf("got a walk WARNING despite the walk's own LoadStep failing, want none") + } + if decisions.clearCalls != 1 { + t.Errorf("clearCalls = %d, want 1 (the engine's own LoadStep retry must still succeed)", decisions.clearCalls) + } + if got := filterLogs(logs, msgAgentErrorDispatched); len(got) != 1 { + t.Errorf("got %d dispatch INFO records, want 1", len(got)) + } + }) +} diff --git a/apps/backend/internal/orchestrator/event_handlers_transient.go b/apps/backend/internal/orchestrator/event_handlers_transient.go index c760287479..0da9873e65 100644 --- a/apps/backend/internal/orchestrator/event_handlers_transient.go +++ b/apps/backend/internal/orchestrator/event_handlers_transient.go @@ -572,6 +572,7 @@ func (s *Service) CancelTransientRetry(ctx context.Context, taskID, sessionID st SessionID: sessionID, AgentExecutionID: execID, ErrorMessage: "Automatic provider retries cancelled. Resume or start fresh to continue.", + UserInitiated: true, }) return true } diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index ddd446611f..50e1ab0b24 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -5471,6 +5471,8 @@ func (s *Service) applyEngineTransitionWithCommitMode( historyTrigger = wfmodels.StepTransitionTriggerTurnStart case engine.TriggerOnChildrenCompleted: historyTrigger = wfmodels.StepTransitionTriggerChildrenCompleted + case engine.TriggerOnAgentError: + historyTrigger = wfmodels.StepTransitionTriggerAgentError } s.recordAutoStepTransition(ctx, session.ID, result.FromStepID, result.ToStepID, consumedSignal, historyTrigger) diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 3a7671c62d..f03d31eee6 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -18,6 +18,7 @@ import ( "io" "strings" "sync" + "sync/atomic" "time" "go.uber.org/zap" @@ -647,6 +648,12 @@ type Service struct { // Workflow engine for typed state-machine evaluation of step transitions workflowEngine *engine.Engine workflowStore *workflowStore + // agentErrorDeps bundles the engine, callback registry and store the + // on_agent_error dispatch reads, published as one atomic value by + // initWorkflowEngine so a dispatch racing a Set* reinit never pairs a new + // engine with a stale registry or store (see agentErrorDispatchDeps' own + // doc comment). + agentErrorDeps atomic.Pointer[agentErrorDispatchDeps] // childCompletionLocks serializes duplicate on_children_completed deliveries. childCompletionLocksMu sync.Mutex childCompletionLocks map[string]*childCompletionOperationLock @@ -1849,6 +1856,11 @@ func (s *Service) initWorkflowEngine() { // dependencies those methods wire in after Service creation. options := append([]engine.Option{engine.WithLogger(s.logger)}, s.engineOptions...) s.workflowEngine = engine.New(store, callbacks, options...) + s.agentErrorDeps.Store(&agentErrorDispatchDeps{ + engine: s.workflowEngine, + registry: callbacks, + store: store, + }) } // SetEngineRunQueue wires the engine's RunQueueAdapter dependency. Used diff --git a/apps/backend/internal/orchestrator/watcher/watcher.go b/apps/backend/internal/orchestrator/watcher/watcher.go index d49265d137..dd439f961c 100644 --- a/apps/backend/internal/orchestrator/watcher/watcher.go +++ b/apps/backend/internal/orchestrator/watcher/watcher.go @@ -51,6 +51,14 @@ type AgentEventData struct { EvidenceKnown bool `json:"evidence_known,omitempty"` OutputObserved bool `json:"output_observed,omitempty"` EffectObserved bool `json:"effect_observed,omitempty"` + // UserInitiated marks a failure event raised by an explicit user action + // (cancelling the transient retry loop) rather than the agent itself + // failing. Set only at CancelTransientRetry's call site; every other + // route defaults to false. The on_agent_error dispatch must not fire for + // a user-initiated failure — a recovery trigger moving the card out from + // under someone who just asked the retries to stop would defeat the + // purpose of the cancel button. + UserInitiated bool `json:"user_initiated,omitempty"` } // ACPSessionEventData contains data from ACP session events diff --git a/apps/backend/internal/workflow/models/models.go b/apps/backend/internal/workflow/models/models.go index ea25159466..648f5da31e 100644 --- a/apps/backend/internal/workflow/models/models.go +++ b/apps/backend/internal/workflow/models/models.go @@ -339,6 +339,10 @@ const ( // StepTransitionTriggerPluginMove identifies a transition caused by a // plugin's MoveTask RPC call. StepTransitionTriggerPluginMove StepTransitionTrigger = "plugin_move" + // StepTransitionTriggerAgentError identifies an automatic transition + // caused by the on_agent_error workflow event, distinguishing a recovery + // move from a routine auto_complete one. + StepTransitionTriggerAgentError StepTransitionTrigger = "on_agent_error" ) // StepTransitionActor identifies the source of a move. Human identity is diff --git a/docs/specs/spec-lint-exceptions.tsv b/docs/specs/spec-lint-exceptions.tsv index 0a71f5b3ce..ce0bd62c85 100644 --- a/docs/specs/spec-lint-exceptions.tsv +++ b/docs/specs/spec-lint-exceptions.tsv @@ -2,4 +2,5 @@ docs/specs/gitlab-issue-milestone-filter/spec.md 80514 docs/specs/office-task-content-editing/spec.md 84103 docs/specs/task-cost-ledger/spec.md 91931 docs/specs/task-delivery-ledger/spec.md 157207 +docs/specs/workflow-on-agent-error-dispatch/spec.md 64431 docs/specs/workflow-on-enter-action-dispatch/spec.md 116827 diff --git a/docs/specs/workflow-on-agent-error-dispatch/spec.md b/docs/specs/workflow-on-agent-error-dispatch/spec.md new file mode 100644 index 0000000000..bb17e7e91d --- /dev/null +++ b/docs/specs/workflow-on-agent-error-dispatch/spec.md @@ -0,0 +1,836 @@ +--- +status: draft +created: 2026-09-02 +updated: 2026-09-03 +owner: Kandev +--- + +# Workflow `on_agent_error` Dispatch for Non-Office Tasks + +Decisions: + +- [ADR-0004 task-model-unification](../../decisions/0004-task-model-unification.md) — establishes the + workflow engine as "the universal coordinator for task-scoped agent runs", declares + `on_agent_error` as one of the seven Phase 2 triggers, and specifies in §7 that it + "fires on the failing task **at its current step**". + +Related spec (same defect class, different trigger): + +- [`docs/specs/workflow-on-enter-action-dispatch/spec.md`](../workflow-on-enter-action-dispatch/spec.md) + — `on_enter` declared, compiled, only partly dispatched. Its AC-A6 (warn rather than silently + discard) is reused here; see [Action vocabulary](#action-vocabulary). + +## Why + +`on_agent_error` is the workflow trigger that recovers from an agent session failure. Every layer +around it works. The dispatch does not. + +Verified 2026-09-02 by `grep -rn TriggerOnAgentError --include=*.go` over `apps/backend`, minus +`_test.go`. The **only** production fire site is `internal/office/service/event_subscribers.go:677` +(`dispatchAgentErrorTrigger`), gated on an Office `*models.Run` resolved by `resolveLifecycleRun` +(`event_subscribers.go:162`), which returns `sql.ErrNoRows` for a task with no claimed Office run. +`internal/orchestrator` and `internal/workflow` contain zero fire sites. The remaining non-test hits +are the constant, the payload type, the trigger-map entry and the Office dispatcher's +session-resolution branches — telling those apart from a fire site is the regression guard's whole +problem; see [Verification](#verification). + +Everything else round-trips: the YAML loader parses it (`config/workflows/loader.go:302`), the engine +compiles it (`engine/types.go:299`), it persists (`workflow/models/models.go:158`), exports through +git-sync (`export.go:275`), and appears over the API (`task/dto/dto.go:1099`). And **no path except +embedded YAML validates the action type at all.** So a kanban step can declare `on_agent_error`, have +it validate, compile, survive an export/import cycle — and silently never run. An inert recovery path +reads as configured, so nobody looks for the missing safety net. + +### Current exposure, measured + +Queried against the local install's SQLite database on 2026-09-02: exactly **one** step row declares +`on_agent_error`, `Office Default / Work` (`queue_run` → `workspace.ceo_agent`). `Fix / Chore` and +`New Feature Dev` are real kanban workflows there and neither declares it; no shipped template +declares it outside `config/workflows/office-default.yml:53`. So this is a **latent trap, not an +active regression**: the first operator to configure `on_agent_error` on a kanban board gets a clean +bill of health for a step that will do nothing. The ACs are therefore written against the mechanism, +not against a board someone has to edit first. + +## Prior art and dispatch decision + +Both live in the companion [`verification.md`](verification.md): the prior-art receipts, and the +reasoning that put the fire site in `handleRecoverableFailureLockedState` rather than any other +candidate home. Provenance, not contract. + +## What + +### Scope + +A **terminal agent-session failure** is a failure that reaches `handleRecoverableFailureLockedState` +with a non-empty session id. Exactly six production routes do so, and the dispatch must be +correct on all six: + +| # | Route | Site | Dispatches? | +|---|---|---|---| +| R1 | bus-driven terminal failure | `handleAgentFailedLocked` (`event_handlers_agent.go:1411`) | yes | +| R2 | managed-npm-runtime start failure | `handleAgentStartFailed` (`:2088`) | yes | +| R3 | auth-error start failure | `handleAgentStartFailed` (`:2105`) | yes | +| R4 | transient retry, no cached prompt | `retryTransientPrompt` (`event_handlers_transient.go:269`) | yes | +| R5 | transient retry, prompt failed synchronously | `retryTransientPrompt` (`:317`) | yes | +| R6 | **user cancelled the retry loop** | `CancelTransientRetry` (`:565`) | **no** — AC-A8 | + +R4 and R5 are the transient loop giving up: the agent genuinely failed and no retry remains, so they +are terminal and fire. R6 is a human pressing Cancel on the recovery banner, and a recovery trigger +must not move a card out from under the person who just asked the retries to stop. R6 reaches the +handler through the same `handleRecoverableFailure` wrapper as R4/R5 and is otherwise +indistinguishable from them — which is why AC-A8 requires an explicit marker, not inference. + +A transient retry still in flight, a dynamic-route fallback, a dropped rotated execution, a dropped +terminal-session event, and a cancellation-in-flight deferral all return before this function and are +**not** terminal failures. + +A **non-Office task** is one for which `models.Task.IsFromOffice` is false — the +`IsFromOfficePredicate` projection (`internal/task/repository/sqlite/task.go:138`), computed at read +time as `project_id != '' OR workflow_id == workspace.office_workflow_id`, the same predicate +`writeTaskReviewState` and `isOfficeTask` use. No second notion of Office ownership is introduced. + +**Session-backed only.** `Engine.handleTrigger` errors unless both `TaskID` and `SessionID` are +non-empty (`engine.go:249`), and `handleRecoverableFailureLockedState` is also reachable with an empty +session id (the wrapper's `data.SessionID == ""` branch). That call cannot dispatch — named +exclusion, see [AC-A5](#a-dispatch). + +### Execution model + +**Position: last.** The dispatch is the final action of `handleRecoverableFailureLockedState` — after the +session-state write, after `writeTaskReviewState`, after the pending step-completion reconciliation, +and **after the `go s.cleanupAgentExecution(...)` launch that currently ends the function**. That +goroutine runs concurrently, touches only agent-execution teardown, and the dispatch must neither +wait on it nor assume it finished. Two reasons, and the second decides it. First, it cannot mask or +race the failure bookkeeping already committed — Office's `dispatchAgentErrorTrigger` states the same +rationale for its own position. Second, **it picks the right step**: a `step_complete_kandev` signal +that landed mid-turn is reconciled by `reconcileStepCompletionSignalLocked` a few lines earlier, and +that reconciliation can transition the task. Firing before consults the step the agent failed on; +firing after consults the step the task now occupies. The latter is correct — if the signal +reconciled, the agent's work at the old step was accepted, and moving the card again from the old +step's recovery config would fight the transition that just committed. ADR-0004 §7's "at its current +step" is read as *current at dispatch time*. + +#### Off the session guard + +**Every dispatching route reaches `handleRecoverableFailureLockedState` with the failed session's +per-session `cancelInFlight` mutex already held** by an outer frame — `handleAgentFailed`, +`handleRecoverableFailure`, `handleAgentStartFailed`. Only the sessionless variants do not, and +[AC-A5](#a-dispatch) already declines to dispatch from those, so "held" is universal, not a case +split. + +**That mutex is not reentrant, and one action in this vocabulary re-acquires it for the same +session.** `autoStartAgentCallback.Execute` (`workflow_callbacks.go:278`) calls +`LaunchSession(IntentWorkflowStep, SessionID: in.State.SessionID)` → `launchWorkflowStep` → +`StartSessionForWorkflowStep` (`task_operations.go:2326`) → `PromptTask` → +`claimSessionRunningForPrompt` (`:4918`), whose first statement locks that same session's guard. +Calling the dispatch while still holding that guard is a **permanent self-deadlock**, not a stall: +the guard is refcounted, never released, so every later Stop / Cancel / Delete / Prompt on the +session blocks behind it. + +**So the dispatch shall not run while holding that guard.** `handleRecoverableFailureLockedState` +performs its recovery bookkeeping under the guard and returns a `func()` instead of dispatching +directly ([AC-A11](#a-dispatch)). Its caller releases the guard (`lock.Unlock()`; `release()`) and +only then invokes the returned closure — synchronously, on the same goroutine. No new goroutine is +spawned, no completion hook is needed for a test to observe it, and no async lifecycle exists to +drain at shutdown: the closure runs and returns like any other call. A callback that reacquires the +guard now finds it free. + +**The closure recovers panics.** An `agent.failed` dispatch delivered through the event bus runs +inside `invokeHandler`'s own recover-and-log wrapper (`events/bus/safe_handler.go:32`); routes +R2-R5 never reach the bus and gain no equivalent protection now that the dispatch is an ordinary +synchronous call rather than a bus-wrapped one. `dispatchKanbanAgentErrorTriggerRecovered` wraps the +call: a panicking callback (`auto_start_agent` is the obvious one, since it is the action that +re-enters the guard) is recovered, logged at ERROR with the task and session ids, and does not take +the process down. That ERROR is not a dispatch record ([AC-A6](#a-dispatch)) and does not count +against its at-most-one. + +**Why the alternative — excluding `auto_start_agent` — was rejected, and what running off the guard +costs, are recorded in the companion +[`verification.md`](verification.md#why-the-dispatch-runs-off-the-session-guard).** The costs +themselves are contract and stay here, in [Concurrency](#concurrency-and-ordering) and +[Failure modes](#failure-modes); none is a data race. + +**How the engine sees that step.** The dispatch follows `processOnTurnCompleteViaEngine` +(`event_handlers_workflow.go:4721-4751`): `EvaluateOnly: true` with an explicit `PreloadedState`. +`PreloadedState` **replaces** `LoadState` — `loadExecutionContext` (`engine.go:333`) uses one or the +other, never both — so AC-A4's freshness comes from this handler, not the engine. State is built as +the dispatch's first act, *after* the reconciliation: reload the session and then the task, in that +order and with the short-circuit [Guard evaluation order](#guard-evaluation-order) fixes, then +`buildMachineState(ctx, task, session)`. `LoadStep` still reads the step through the store, keyed on +the reloaded task's `WorkflowStepID` — `assembleMachineState` copies that field into +`MachineState.CurrentStepID`; `models.Task` has no field of that name. + +**That reload is ONE read of each, reused, and it is keyed on the EVENT.** The task is read by +`data.TaskID` and the session by `data.SessionID` — the same two fields the surrounding handler +already uses for `persistLastAgentError`, `writeTaskReviewState` and the reconciliation. The dispatch +shall **not** re-derive the task id from the loaded session's `TaskID`, and shall not compare the +two. Using exactly one source makes a mismatched pair unrepresentable inside the dispatch, which +matters because the two are consumed differently downstream — callbacks act on `in.State.TaskID` +while transitions use `HandleInput.TaskID`, so a pair drawn from two sources could start work on one +task while moving another. A row whose `task_id` disagrees with the event is a repository-integrity +failure the whole handler already assumes away; see [Out of scope](#out-of-scope). + +The object each read produces is the *same* one the AC-B1 ownership check, the AC-F2 / AC-F3 / AC-F4 +shape guards and `buildMachineState` consult; the dispatch shall not re-read either mid-flight, +because two reads of one row can observe two versions and let the ownership gate and the engine's +state disagree about the same task. +[Guard evaluation order](#guard-evaluation-order) places that read; [AC-A7](#a-dispatch) / +[AC-F6](#f-guards-nil-empty-and-boundaries) give its failure behaviour. + +**Idempotency.** The dispatch passes an `OperationID` to `Engine.HandleTrigger`, which short-circuits +to `HandleResult{Idempotent: true}` when `TransitionStore.IsOperationApplied` reports the key applied +(`engine.go:252`). The key is derived from the failure's identity, not from a run: + +``` +agent_error:session:: +agent_error:session: // when AgentExecutionID is empty +``` + +Both components are needed: a session can host several executions in sequence (rotation, resume, +dynamic re-route) and each can fail, so a session-only key would suppress every failure after the +first for the life of the process. + +**Why the fallback exists** is in the +[companion](verification.md#why-the-empty-execution-id-fallback-exists); cost: [AC-C8](#c-idempotency). + +**When the marker is written, and what that costs.** `handleTrigger` calls `markOperationApplied` +**only after `processActions` returns without error**, and `applyEngineTransition` runs **after** +`HandleTrigger` has returned and marked. So an action list that errors part-way leaves the operation +**unmarked** and a redelivery re-runs it from the start ([AC-C6](#c-idempotency)), while a transition +the engine resolved but `applyEngineTransition` then declines leaves it **marked** and unretried +([AC-C7](#c-idempotency)). + +**The durability limit is real.** `workflowStore.IsOperationApplied` / `MarkOperationApplied` +(`workflow_store.go:886`) are backed by an in-memory `sync.Map` (`appliedOps`), no persistence, no +eviction — *exactly-once within one backend process lifetime*, not across a restart. Acceptable +because `agent.failed` is a live bus event with no replay: a restart loses the event rather than +redelivering it. A builder must not read "idempotent" as "durable". + +**Transitions go through the orchestrator, not the raw engine.** `Engine.applyTransition` delegates +to `TransitionStore.ApplyTransition`, which moves the task but **does not dispatch the destination +step's `on_enter`**: `workflowStore.applyTransition` only builds a `stepentry.PendingAllocation` when +ctx is wrapped with a `stepentry.ResultHolder`, which only `applyEngineTransition` does +(`workflow_store.go:312-325`). A bare `HandleTrigger` would move a card into a step whose +`on_enter: auto_start_agent` never runs — this card's defect, one trigger over. The dispatch calls +`applyEngineTransition(..., triggerOnEnter: true)`, which validates the target step, runs the +credential preflight, and fires `on_enter`. + +### Guard evaluation order + +The guards in group F, plus AC-A5, AC-A8, AC-B1 and AC-B2, are a **first-match-wins sequence**, not +an unordered set. Each guard is either **silent** or **recording**, and that split is what makes the +order enforceable at all: + +- **Silent:** AC-F1, AC-A5, AC-B1, AC-F2, AC-F3, AC-F4. +- **Recording:** AC-A8, AC-A7 and AC-F5 at DEBUG; AC-B2 and AC-F6 at WARNING — AC-F6 except its + `otherWorkingSessionID` sub-case, which records nothing of its own. + +From the event alone, before any repository read: + +1. **AC-F1** engine snapshot absent or its engine nil → skip. +2. **AC-A5** no session id → skip. +3. **AC-A8** user-initiated (route R6) → skip, DEBUG. + +Then the single task + session read described above. It is **session first, then task**, and it +short-circuits: the task is not read when the session step already skipped. That order is +contractual — it is what decides the record when both reads would fail — and it puts the commonest +vanish case (AC-A7) before any task read. **A `nil` row returned with a `nil` error is the +not-found case, not a success**, for both reads: the orchestrator treats `err != nil || row == nil` +as one condition at a dozen sites, and `assembleMachineState` dereferences `task.WorkflowStepID` +while `buildMachineState` dereferences `session.ID`, so admitting a nil row here is a panic in the +terminal-failure handler. The full table, first match wins: + +4. **Session**, read by `data.SessionID`: `errors.Is(err, models.ErrTaskSessionNotFound)`, or a nil + session with a nil error → **AC-A7**, DEBUG no-op. Any other error → **AC-F6**, WARNING. +5. **Task**, read by `data.TaskID`, only if step 4 did not skip: any error, or a nil task with a nil + error → **AC-B2**, WARNING. AC-B2 owns every task-read failure; AC-F6 owns none of them. + +Then, against that one loaded pair: + +6. **AC-B1** Office-owned → skip. It precedes the shape guards deliberately: Office ownership + decides whether this mechanism acts at all. +7. **AC-F2** ephemeral, **AC-F4** archived, **AC-F3** empty `WorkflowStepID` → skip. +8. **AC-F5** another session on the task is working → skip, DEBUG. + +Then dispatch. A failure satisfying two guards at once takes the **earlier** one. + +**What is contractual here, and what deliberately is not.** The order between two co-occurring +guards is contractual only where it is **observable** — where at least one of them records. Where +both are silent the relative order is **explicitly not enforced**: step 7's three guards may be +evaluated in any order or folded into one condition, and the same goes for AC-F1 against AC-A5 and +for AC-B1 against step 7. Nothing distinguishes those outcomes — same skip, no dispatch, no record — +so a criterion demanding one would be untestable. Every *recording* guard's position **is** +contractual, because a wrong order shows up as the wrong record, or as one that should not have been +emitted at all. + +### Action vocabulary + +**The set of actions a kanban step may declare under `on_agent_error` is exactly the set +`engine.compileGenericActions` already compiles.** No new allow-list, no new action kinds, no +kanban-specific filter at the dispatch site. That set is seven kinds (`engine/types.go:437`): + +| Kind | Meaningful on a kanban task? | Executor | +|---|---|---| +| `move_to_next` / `move_to_previous` / `move_to_step` | yes | engine transition (structural — **no callback**) | +| `auto_start_agent` | yes, and it is a restart loop — see [Failure modes](#failure-modes) | `autoStartAgentCallback` | +| `queue_run` | yes for `target: primary`; needs an Office workspace CEO for `workspace.ceo_agent` | `QueueRunCallback` | +| `clear_decisions` | degenerate — a kanban step has no decisions | `ClearDecisionsCallback` | +| `queue_run_for_each_participant` | degenerate — a kanban step has no participants | `QueueRunForEachParticipantCallback` | + +The three `move_to_*` kinds have **no registered callback** (`workflow_callbacks.go:32-60`); they are +handled structurally inside `evaluateActions`. That is load-bearing for AC-E4 and AC-E7, which is why +both carry an explicit transition carve-out. + +Two facts a builder will otherwise assume wrongly. The embedded-YAML allow-list `validGenericAction` +(`config/workflows/loader.go:200`) accepts only the last three, all Office-shaped, so the four +kanban-meaningful kinds are **not** declarable through a shipped template. And that allow-list is the +*only* validation of generic action types anywhere — import, git-sync and step-write validate none — +so all seven are already reachable. This spec does not widen it; it makes the compiled set execute. + +**A recognised-and-skipped no-op is what hid this defect, so it does not stay silent.** +`Engine.executeCallback` returns `nil` for a kind with no registered callback (`engine.go:391-394`), +the exact shape of the failure this card is about. This spec adopts +`workflow-on-enter-action-dispatch`'s AC-A6 answer: an action that reaches no executor **warns**. + +**Where that warning lives is a decision, not a detail.** It is emitted at the orchestrator dispatch +call site, not inside `executeCallback`, which `evaluateActions` calls identically for all eleven +triggers: a warning there would change behaviour for every trigger the moment this ships, and would +collide with the sibling `on_enter` spec, which needs the same warning at the same line for its own +AC-A6. The call site instead walks the compiled `on_agent_error` actions against the callback +registry before invoking the engine. That registry is a local in `Service.initWorkflowEngine` +(`service.go:1797`) that neither `Service` nor `Engine` exposes, so this card retains it — an +additive field, no engine-package change. + +**Retaining it is not snapshotting it.** `buildWorkflowCallbacks(svc)` rebuilds the registry on +**every** `initWorkflowEngine` call and registers `queue_run`, `queue_run_for_each_participant` and +`clear_decisions` only when `svc.engineRunQueue` / `svc.engineParticipants` / `svc.engineDecisions` +are non-nil — and the eleven `reinitWorkflowEngine()` sites re-enter it as the `Set*` methods wire +those in after construction. A value captured at construction reports those three kinds unregistered +**forever**, so AC-E4 would warn on every dispatch of an action the engine executes correctly: a +false alarm shipped by the criterion written to prevent one. Nor are two fields one atomic +replacement. `initWorkflowEngine` rebuilds **three** collaborators together — the store +(`service.go:1798`), the registry, and the engine built from both (`:1805`) — and the dispatch reads +all three: the engine to call, the registry to walk, and the store to read the actions being walked. +Held separately, each reinit is three unsynchronized writes and each dispatch three unsynchronized +reads, so one racing a `Set*` call can pair a new engine with an old registry, or walk one version of +a step while the engine executes another. One value carrying all three, published once and read once, +closes it by construction. See [AC-E12 and AC-E13](#e-action-vocabulary-and-transitions). + +### Acceptance criteria + +#### A. Dispatch + +- **AC-A1** WHEN an agent session on a non-Office task reaches a terminal agent failure, the + system shall dispatch `engine.TriggerOnAgentError` for that task against the session that failed. +- **AC-A2** The dispatch shall occur on every route in the [Scope](#scope) table marked "yes" — R1 + through R5. A dispatch covering only the bus-driven route R1, or only R1–R3, does not satisfy this + criterion. +- **AC-A3** The dispatch shall be the last action of terminal-failure handling: after the session + state write, REVIEW and signal reconcile, and the `go s.cleanupAgentExecution(...)` launch. + Release `cancelInFlight` before dispatch; callbacks can reacquire. Do not wait on cleanup. +- **AC-A4** The step whose `on_agent_error` actions are evaluated shall be the task's current step + **at dispatch time**, read from a task reloaded after the step-completion reconciliation and + supplied to the engine as `PreloadedState`. WHEN a pending step-completion signal reconciled into a + transition earlier in the same handler, the destination step's configuration is consulted, not the + step the agent failed on. +- **AC-A5** IF the failure carries no session id, THEN the system shall not dispatch and shall not + warn — a sessionless failure is an ordinary outcome, not an anomaly. +- **AC-A6** A trigger is **dispatched** when the dispatch called `Engine.HandleTrigger` and the call + returned no error and `HandleResult.Idempotent` was false. A call the engine short-circuited as + idempotent is **not** a dispatch for the purpose of this criterion, whatever + [AC-C3](#c-idempotency)'s prose calls it. WHEN a trigger is dispatched **and** + `HandleResult.ActionCount > 0`, the system shall emit exactly one INFO record naming task id, + session id, step id, and the operation id. `ActionCount` is `len(actions)` from the engine's own + result, so this is decided after the call, not guessed before it — the same field Office's + dispatcher already reads as its "something happened" predicate. **At most one** record is emitted + per invocation that reaches the engine — exactly one in three of the four cases and none in the + fourth — and the four cases are total and disjoint: dispatched with + actions → this INFO; dispatched with none → [AC-E2](#e-action-vocabulary-and-transitions)'s DEBUG; + short-circuited as idempotent → **nothing** ([AC-C3](#c-idempotency), [AC-C8](#c-idempotency)); + engine error → [AC-E5](#e-action-vocabulary-and-transitions)'s ERROR. "At most one" is the + invariant and the enumeration is authoritative: a builder shall **not** read this criterion as + requiring a record on the idempotent path, which AC-C3 and AC-C8 both forbid. + + **"Record" here, and everywhere this spec counts records, means a *dispatch record*: one of the + four outcomes enumerated above, or a recording guard's line; this criterion is where that set is + defined, and [AC-F9](#f-guards-nil-empty-and-boundaries) states the identity requirement it places + on the records it names. + [AC-E4](#e-action-vocabulary-and-transitions)'s walk WARNING is deliberately **not** a dispatch + record and does not count against "at most one".** It reports how the step is *configured*, is + decided by the pre-engine walk rather than by the call's outcome, and is therefore orthogonal to + the four cases: a step declaring an action with no registered callback emits it **in addition to** + whichever case applies — including the idempotent case, where the dispatch-record count is still + zero. Without this carve-out the criterion would be unsatisfiable for such a step, since AC-E4's + WARNING plus this INFO is two, and AC-C3's and AC-C8's "no dispatch record" would be false whenever the + redelivered step happens to declare an unregistered action. This is the same exemption shape + [AC-F8](#f-guards-nil-empty-and-boundaries) already applies to its two non-dispatch log classes. +- **AC-A7** IF the failed session row no longer exists when the dispatch runs — deleted between the + failure and this point — THEN the system shall treat it as a no-op logged at DEBUG, not as an + error. Detection shall use `errors.Is(err, models.ErrTaskSessionNotFound)`, which the repository + wraps with `%w`. This criterion **takes precedence over AC-F6** for the session reload that builds + `PreloadedState`: a reload failing with `ErrTaskSessionNotFound` is this DEBUG no-op; a reload + failing for any other reason is AC-F6's WARNING. A **missing current step** is deliberately not + covered here — it surfaces as an unwrapped error with no sentinel and takes AC-E5's ERROR path. See + [Out of scope](#out-of-scope). +- **AC-A8** IF the failure is user-initiated — route R6, the user cancelling the transient retry + loop — THEN the system shall not dispatch, and shall log at DEBUG. The signal shall be an + explicit field on `watcher.AgentEventData` set only at `CancelTransientRetry`'s call site, + defaulting to false so every other route is unaffected. It shall not be inferred from the error + message, the failure code, or a missing execution id. +- **AC-A9** WHEN the transient retry loop reaches a terminal exit that is not user-initiated — + routes R4 and R5 — the system shall dispatch. Exhausting or failing to start automatic retries is + an agent failure, and suppressing recovery there would defeat the trigger for exactly the failures + it exists to catch. + +- **AC-A10** AC-A8's suppression is scoped to the R6 delivery itself and shall not leak to a + concurrent retry timer's delivery. WHEN a `runTransientRetry` timer has already claimed its retry + entry (`transientRetries.Load` returned the live entry **and** `entry.claim()` succeeded) and that + timer **does** reach R4 or R5, THEN it shall dispatch, with the AC-A8 marker at its default + `false` — the cancel's marker rides on the cancel's own event and never on the timer's. + **Whether the timer reaches R4 or R5 at all is deliberately NOT this criterion's, and is not + guaranteed:** the cancel also cancels the `retryCtx` that timer is running on, and + `retryTransientPrompt` can return at one of its `ctx.Err()` checks first — the mechanism is in + [Concurrency](#concurrency-and-ordering). So the pair yields **one when the timer gets through, + zero when the cancel outruns it**; both are correct and [AC-B5](#b-office-isolation) names the + zero. A test shall pin the non-leak — a timer that reaches R4/R5 dispatches despite a concurrent + cancel — and shall assert **at most one**, never exactly one. **Accepted residual**, and it shall + **not** be closed by detaching the post-claim continuation from `retryCtx`: that re-drives the + prompt after the user pressed Cancel ([Out of scope](#out-of-scope)). + +- **AC-A11** The dispatch shall not run while the failed session's `cancelInFlight` guard is held. + `handleRecoverableFailureLockedState` shall return a `func()` to its guard-holding caller instead + of dispatching directly; the caller shall release the guard and only then invoke the returned + closure, synchronously, on the same goroutine — no new goroutine is spawned. This is what lets an + action list containing a bare `auto_start_agent` complete instead of self-deadlocking; the + reasoning, including why excluding that action was rejected, is in + [Off the session guard](#off-the-session-guard). + + **The closure shall recover panics.** Routes R2-R5 never reach the event bus's own recover-and-log + `invokeHandler` (`events/bus/safe_handler.go:32`), and the dispatch is now an ordinary synchronous + call with nothing above it on the stack to recover a panic, so a panicking callback would otherwise + crash the whole backend process. The closure shall recover, log at ERROR with the task and session + ids, and return. That ERROR is **not** a dispatch record ([AC-A6](#a-dispatch)) and does not count + against its at-most-one. + +#### B. Office isolation + +- **AC-B1** IF the failing session's task is Office-owned by the authoritative projection + (`models.Task.IsFromOffice`), THEN the system shall not dispatch. Office's Path A remains the only + fire site for those tasks. +- **AC-B2** IF the task cannot be loaded — the read errored, **or** returned a nil task with a nil + error — THEN the system shall not dispatch and shall log at WARNING. This is the sole record for + every task-read failure; AC-F6 emits none. The check fails **closed**: an unavailable ownership answer must never be read as "not + Office", because that is the state that double-fires with Path A. This mirrors + `writeTaskReviewState`'s fail-closed task load, and deliberately does not reuse `isOfficeSession`, + which returns `false` on a lookup error (fails open). +- **AC-B3** Office behaviour shall be unchanged. `dispatchAgentErrorTrigger`, its + `agent_error:` key, `HandleRunFailure` and `escalateFailure` keep their current semantics, + and the Office dispatcher's `resolveSession` / `resolveSessionByID` branches for + `TriggerOnAgentError` shall be untouched. +- **AC-B4** For one agent failure, **at most** one `TriggerOnAgentError` shall be dispatched across + both mechanisms. "At most" is exact; the ways the count is legitimately zero are enumerated in + [AC-B5](#b-office-isolation), which is the sole register of them — this criterion deliberately + carries **no count of its own**, because a number here goes stale the moment AC-B5 gains an entry + and has already done so once. The + one case in which two may fire is the ownership-flip race in + [Failure modes](#failure-modes), an accepted residual; a test for this criterion shall assert "not + more than one", never "exactly one" unconditionally. +- **AC-B5** The dispatch count for one failure shall be zero, not one, in each of these cases, none + of them a defect: an Office-owned task whose Office run is not claimed (Path A's + `resolveLifecycleRun` returns `sql.ErrNoRows` and AC-B1 blocks the orchestrator, so neither fires); + any AC-A5, AC-A8 or AC-B2 skip; any AC-F1 through AC-F6 guard skip; and a user cancel that + cancels `retryCtx` inside an already-claimed timer's window, where the timer returns at one of + `retryTransientPrompt`'s `ctx.Err()` checks before reaching R4 or R5 and the cancel's own R6 + delivery is AC-A8-suppressed, so neither delivery dispatches ([AC-A10](#a-dispatch)). + +#### C. Idempotency + +- **AC-C1** The dispatch shall pass `OperationID` = `agent_error:session::`. +- **AC-C2** IF `AgentExecutionID` is empty, THEN the `OperationID` shall be + `agent_error:session:`. +- **AC-C3** WHEN the same failure is handled twice within one backend process lifetime **and the + first handling completed action evaluation without error**, the second delivery shall execute no + action and the engine shall report `HandleResult{Idempotent: true}`. That delivery is not a + "dispatched trigger" under [AC-A6](#a-dispatch) and emits no **dispatch record**. + [AC-E4](#e-action-vocabulary-and-transitions)'s walk WARNING is not a dispatch record + ([AC-A6](#a-dispatch)) and this criterion does not suppress it: the walk runs before the engine + call and cannot know the operation is already applied. +- **AC-C4** WHEN two distinct agent executions on one session fail in sequence, both shall dispatch. + Suppressing the second is a defect, not idempotency. +- **AC-C5** The exactly-once guarantee is scoped to one backend process lifetime, because + `workflowStore.appliedOps` is an in-memory `sync.Map`. This is a stated limit of the contract, not + a gap to be closed by adding durable marker storage in this card. +- **AC-C6** IF an action in the list returns an error, THEN the operation shall be left **unmarked** + (the engine's existing behaviour: `markOperationApplied` runs only after `processActions` succeeds) + and a subsequent delivery of the same failure shall re-run the list from the start, including + actions that already succeeded. Actions under this trigger are not assumed idempotent and this card + does not make them so. +- **AC-C7** IF the engine resolved a transition and marked the operation, but `applyEngineTransition` + then declines it — target step missing, or credential preflight failure — THEN the transition + shall not be retried. It is **not** an AC-E5 case: the engine returned success. The outcome is + already recorded by `applyEngineTransition`'s own logs (WARNING for a missing target step, ERROR + for a failed commit) + and the dispatch shall not add a duplicate record. The marker precedes the transition + commit and this card does not reorder that. + +- **AC-C8** IF two failures on the same session both carry an **empty** `AgentExecutionID`, THEN + they collapse to the one AC-C2 key and only the first shall dispatch; the second is reported + `Idempotent: true` and emits no **dispatch record**, carrying the same AC-E4 carve-out AC-C3 + states. That suppression holds only where the first handling + completed action evaluation without error: IF the first left the operation **unmarked** because an + action errored, THEN the second re-runs the list as AC-C6 requires and this criterion does not + suppress it. AC-C4 is therefore satisfied by *distinguishable* executions — both ids non-empty and + different — since an empty id is not distinguishable from a prior empty one. The cost, a genuinely + second empty-id failure on one session being skipped, is an accepted residual bounded by AC-C5's + process lifetime; a fresh key per delivery would defeat the operation id for every redelivery to + rescue a rarer case. It shall **not** be closed by adding a counter, a timestamp or any other + discriminator to the key. + +#### D. Payload + +- **AC-D1** `OnAgentErrorPayload.FailedSessionID` shall be the id of the session that failed, never + empty on a dispatched trigger (AC-A5 excludes the only case where it could be). +- **AC-D2** `OnAgentErrorPayload.FailedAgentID` shall be the failure event's `AgentProfileID`; IF + that is empty, THEN the failed session's `AgentProfileID`; IF both are empty, THEN the empty + string. It shall not be populated with an Office run id, an agent execution id, or an agent type. + The synthetic events built by `handleAgentStartFailed` (R2, R3) and by the transient-loop exits + (R4, R5) carry no `AgentProfileID`, so the session fallback is the normal path for four of the five + dispatching routes, not an edge case. +- **AC-D3** `OnAgentErrorPayload.ErrorMessage` shall be the failure event's `ErrorMessage`; IF that + is empty, THEN the literal `agent failed`, matching the default `persistLastAgentError` writes for + the same failure. + +#### E. Action vocabulary and transitions + +- **AC-E1** The dispatch shall not filter by action kind. Every action `compileGenericActions` + produces for `on_agent_error` is handed to the engine, and the engine's per-kind semantics apply + unchanged. +- **AC-E2** WHEN the current step declares no `on_agent_error` actions — the trigger was dispatched + and `HandleResult.ActionCount == 0` — the dispatch shall be a successful no-op logged at DEBUG, and + shall mark the operation applied (the engine already does so on its empty-actions path). This is + the overwhelmingly common case: it must not warn, and per AC-A6 it emits this DEBUG **instead of**, + never in addition to, the AC-A6 INFO. +- **AC-E3** WHEN an `on_agent_error` action resolves a transition target, the system shall apply that + transition through `applyEngineTransition` with `triggerOnEnter: true` — the same call an + `on_turn_complete` transition receives, including target-step validation and credential preflight. + The engine's bare `ApplyTransition`, which skips the destination's `on_enter`, does not satisfy + this criterion. The one case where `on_enter` legitimately does not run despite a committed + transition is AC-E10's WIP-queued destination. + + **The `taskDescription` argument shall be the `Description` of the task already reloaded for + `PreloadedState`** ([Execution model](#execution-model)) — not the empty string, and not a third + read of the task. That argument is not inert: `applyEngineTransitionWithCommit` hands it to + `processOnEnter`, which passes it to `buildWorkflowPrompt`, so it becomes the prompt given to an + agent the destination step auto-starts — the path [Failure modes](#failure-modes) already names. + The repository supplies this argument two different ways at its three existing call sites (the + task's description at the `on_turn_complete` and `on_children_completed` sites, the empty string at + the `on_turn_start` site), so it is named here rather than left to precedent. A recovery move + carries the task's description for the same reason `on_turn_complete` does: the relaunched agent + needs to know what the task is. **Substituting** the empty string does not satisfy this criterion; + a task whose own `Description` is empty passes that empty value through unchanged, which is the + field's value and not a substitution. No default, no placeholder, and no second read of the task. +- **AC-E4** IF a declared `on_agent_error` action reaches no executor, THEN the system shall log at + WARNING naming workflow id, step id, step name, action type, task id and session id, and the + remaining actions shall be unaffected. "Reaches no executor" means **no callback is registered for + its kind in the running configuration**. The three `move_to_*` kinds are explicitly **excluded**: + they have no callback by design and execute structurally, so warning on them would be a false + positive — including for the second and subsequent transition actions AC-E7 sends down the callback + path. The warning is emitted at the orchestrator dispatch call site by walking the step's compiled + `on_agent_error` actions against the callback registry, before the engine call; it shall not be + added inside `engine.executeCallback`, which is shared by all eleven triggers. This WARNING is + **not** a dispatch record ([AC-A6](#a-dispatch)): it does not count against AC-A6's or AC-F8's + "at most one", and because the walk precedes the engine call it is emitted even when the engine + then short-circuits the operation as idempotent ([AC-C3](#c-idempotency)). +- **AC-E5** IF the engine returns an error, THEN the system shall log at ERROR and not propagate it. + Failure bookkeeping already committed by the surrounding handler must not be masked by a + recovery-dispatch failure — the contract `dispatchAgentErrorTrigger` states for Office. This is + also the path a missing current step takes (AC-A7). +- **AC-E6** `validGenericAction` in `config/workflows/loader.go` shall be unchanged and no shipped + workflow template shall gain an `on_agent_error` declaration. This card makes the trigger work; it + configures no board. +- **AC-E7** WHEN a step declares the same **non-transition** action type more than once under + `on_agent_error`, each declaration shall execute, in declared order. There is no deduplication and + none shall be added. Transition actions are **excluded**: `evaluateActions` gates + transition handling on `targetStepID == ""`, so only the first eligible transition resolves a + target and any later transition action is not executed as a transition. This is + first-eligible-transition-wins, the engine's existing behaviour for every trigger, unchanged here. +- **AC-E8** IF a transition action resolves to the step the task already occupies, THEN no transition + shall be applied and no `on_enter` dispatched. The trigger is still marked applied under AC-C1. +- **AC-E9** Transition guards and `requires_approval` shall behave exactly as they do under every + other trigger, and the dispatch shall add no special case. A transition action whose guard is not + satisfied is skipped, recorded through the engine's existing `recordGuardNotFired`, and resolves no + target; an action carrying `requires_approval` is not treated as a transition at all + (`evaluateActions` tests `!action.RequiresApproval`) and takes the callback path, where AC-E4's + transition carve-out keeps it silent. Neither case shall produce an AC-E4 warning. +- **AC-E10** IF a transition commits but the destination step is WIP-limited and the task is queued + rather than admitted (`QueuedForStepID == ToStepID && !WIPAdmitted`), THEN `on_enter` is + **deferred to the queue-promotion event**, not dispatched by this handler, and that is not an + AC-E3 failure. `applyEngineTransitionWithCommit` already takes this branch and returns true. An + AC-E3 test shall therefore use a destination step with no WIP limit, or assert the deferral. +- **AC-E11** A transition committed under this trigger shall be recorded in the ADR-0015 audit + ledger — the **`session_step_history`** table `recordAutoStepTransition` writes — under **its own** + trigger label. `applyEngineTransitionWithCommit` + picks that label with a switch casing only `TriggerOnTurnStart` and `TriggerOnChildrenCompleted`, + everything else falling through to `wfmodels.StepTransitionTriggerAutoComplete` — exhaustive for + its three existing callers, and this card is the first to route a fourth trigger through it. The + system shall add `wfmodels.StepTransitionTriggerAgentError = "on_agent_error"` and the matching + `case engine.TriggerOnAgentError`, so a recovery move is distinguishable from a routine move in + `session_step_history.trigger`. Recording a recovery move as `auto_complete` does **not** satisfy + this criterion. The new value needs no migration (that column is `TEXT NOT NULL`, no CHECK) and no + DTO or frontend change; `queue_promotion` and `plugin_move` were added to this enum the same way. + + **The ledger named here is exact, and the other one is deliberately excluded.** There are two + step-transition ledgers with two separate enums, and the switch this criterion changes reaches only + the first: + - `session_step_history.trigger` ← `wfmodels.StepTransitionTrigger`, written by + `recordAutoStepTransition` → `workflow/service.CreateStepTransition`. **This criterion's target.** + - `task_step_transitions.trigger`, and the `telemetry_step_transitions_inserted_total` expvar + counter keyed off it, ← the separate, explicitly **closed** `steptelemetry.Trigger` enum, + written from `steptelemetry.FromContext(ctx).Trigger` and set by `engineTransitionAttribution`, + which hardcodes `TriggerEngineTransition` and, by an outermost-caller-wins rule + (`steptelemetry.HasTrigger`), will not overwrite an attribution a caller already placed on the + context. + + A `wfmodels` addition therefore does **not** reach the task ledger or that counter, and this + criterion does not claim it does: both continue to read `engine_transition` for an + `on_agent_error` move, exactly as they do for every other engine transition. Distinguishing the + recovery move *there* is [out of scope](#out-of-scope). A test for this criterion shall assert the + `session_step_history` row and shall **not** assert against `task_step_transitions.trigger`, whose + value under this trigger is unchanged by this card. + + This criterion constrains the **label only**: `recordAutoStepTransition`'s pre-existing + best-effort semantics (nil recorder, swallowed write error, async enqueue in production) are shared + by every trigger and are not changed here. The pending-signal clear stays scoped to + `TriggerOnTurnComplete`. + +- **AC-E12** The walk of AC-E4 shall read the action list as `StepSpec.Events[TriggerOnAgentError]` + from `TransitionStore.LoadStep(ctx, task.WorkflowID, task.WorkflowStepID)` — the **same store the + engine will use for its own `LoadStep`**. Re-reading the step row and calling `engine.CompileStep` + at the call site does **not** satisfy this: it bypasses the store's `stepCache` and can compile a + newer row than the engine executes, which is precisely the false alarm AC-E4 exists to prevent. + **The guarantee this buys is bounded, and the bound is contractual.** The walk and the engine make + **two** `LoadStep` calls, not one, so they see one compiled version **only absent an interleaved + cache change on that key** — `stepSpecCache` entries carry a TTL and are dropped by + `stepCache.invalidate`, which `handleWorkflowStepCacheInvalidation` fires on any step edit. A + concurrent edit between the two reads may have the walk inspect one version while the engine + executes another. **Accepted residual:** the cost is one spurious or missed AC-E4 WARNING on a step + being edited at the instant it fails, the dispatch is unaffected, and pinning one `StepSpec` across + both would mean handing a pre-compiled step to the engine, which `PreloadedState` does not carry + and no engine API accepts. A test shall assert the shared-store read path, **not** a version + equality that only holds when nothing invalidates. +- **AC-E13** The engine, the registry walked by AC-E4 and the store read by AC-E12 shall be retained + as **one value** — a single struct holding all three — built inside `initWorkflowEngine` where the + store and `callbacks` are already built, published in a **single atomic write** (an + `atomic.Pointer` or equivalent), and read **exactly once** per dispatch, that one read supplying + all three. Separate fields do **not** satisfy this criterion: each of the eleven + `reinitWorkflowEngine()` calls is then three unsynchronized writes, and a dispatch racing one can + pair a new engine with an old registry, or walk one version of a step while the engine executes + another. A value snapshotted at construction does not satisfy it either — it would report + `queue_run`, `queue_run_for_each_participant` and `clear_decisions` as unregistered even after the + `Set*` wiring registers them. Synchronizing `s.workflowEngine`'s and `s.workflowStore`'s own + pre-existing readers is [out of scope](#out-of-scope). +- **AC-E14** IF AC-E12's `LoadStep` fails, THEN the walk shall be skipped, no AC-E4 warning shall be + emitted, **the walk shall contribute no dispatch record of its own, and the dispatch shall still + proceed to the engine**. This is not a guard and it does not fail closed: the walk is an advisory diagnostic, + and a dispatch that cannot be pre-inspected is still a dispatch that must happen. + **The dispatch's record is then whatever the engine's own outcome dictates, and all three of the + following are reachable** — `stepSpecCache.getOrLoadStep` propagates a fetch error to its waiters + and **never caches it**, so the engine's `LoadStep` on the same key is an independent retry, not a + replay of the walk's failure: + - IF the engine's `LoadStep` also fails, THEN the outcome is + [AC-E5](#e-action-vocabulary-and-transitions)'s single ERROR, and it is the only record. + - IF the engine's `LoadStep` **succeeds** — the walk hit a transient error the retry did not — + THEN the dispatch proceeds normally and records exactly what + [AC-A6](#a-dispatch)'s four-case table says for the result it gets: the INFO when actions ran, + [AC-E2](#e-action-vocabulary-and-transitions)'s DEBUG when there were none. This criterion does + **not** suppress that record, and a builder shall not read "no warning" as covering the dispatch. + - IF the operation id was already applied, THEN the engine **never calls `LoadStep` at all** and + neither branch above is taken: `handleTrigger` tests `isOperationAlreadyApplied` **before** + `loadExecutionContext`, so it short-circuits to `HandleResult{Idempotent: true}` above the step + load. The outcome is [AC-C3](#c-idempotency)'s — no action executed, no dispatch record — and this + criterion neither adds one nor overrides it. A builder shall not treat the two `LoadStep` + branches as the complete matrix. + The "no warning" here is scoped to the **walk**, so one failure never becomes two dispatch records + and AC-F8 is not contradicted; the idempotent case contributes zero dispatch records, which "at + most one" admits. + Skipping the dispatch on a failed walk does not satisfy this criterion. +#### F. Guards, nil, empty, and boundaries + +Each is a skip with no dispatch; whether it records is fixed by +[Guard evaluation order](#guard-evaluation-order). They mirror guards the orchestrator's other engine +call sites apply, and are enumerated because omitting one produces a nil-dereference or a nonsensical +dispatch. + +- **AC-F1** IF the retained value ([AC-E13](#e-action-vocabulary-and-transitions)) is absent or its + engine nil, THEN skip. +- **AC-F2** IF the task is ephemeral (`task.IsEphemeral`), THEN skip: it has no workflow. Mirrors + `processOnTurnStartViaEngine`. +- **AC-F3** IF `task.WorkflowStepID` is empty, THEN skip. +- **AC-F4** IF the task is archived, THEN skip. Mirrors `writeTaskReviewState`. +- **AC-F5** IF another session on the same task is in a working state, THEN skip and log at DEBUG + naming the blocking session. A sibling still doing work must not have its card moved out from + under it by a peer's failure. The predicate is `otherWorkingSessionID` + (`event_handlers_streaming.go:2127`), the one `writeTaskReviewState` applies a few lines earlier. + It is **re-evaluated**, not inherited: a second, later read that may legitimately disagree with the + first if a sibling started or settled in between. Accepted. +- **AC-F6** IF a repository read the dispatch needs fails — the **session** reload that builds + `PreloadedState` (see [Execution model](#execution-model)), or `otherWorkingSessionID` returning + its `ok == false` error result — THEN skip. A read "fails" when it returns an error **or** a nil + row with a nil error; the two are one condition. Every read on this path fails closed, matching + AC-B2: acting on a partial view of a task's state is worse than not acting, because the action is + moving the operator's card. **Which record it emits depends on which read failed.** A failed + session reload logs the dispatch's own WARNING, **except** one failing with + `ErrTaskSessionNotFound` or returning a nil session, which is AC-A7's DEBUG no-op. A failed + `otherWorkingSessionID` logs **nothing of the dispatch's own**: that helper already writes its own + WARNING (`"failed to list task sessions before REVIEW state reconcile"`) before returning + `ok == false`, and a second line saying the same is the duplicate AC-F8 forbids. **The task reload + is deliberately NOT this criterion's**: every task-read failure, nil task included, is + [AC-B2](#b-office-isolation)'s WARNING and only AC-B2's, so one skip never produces two WARNINGs. + Each skip stays observable as the absence of a dispatch. +- **AC-F7** These guards shall be evaluated before the `OperationID` is marked applied, so a skip + caused by a transient condition does not suppress a later valid dispatch of the same failure within + the process lifetime. +- **AC-F8** The guards shall be evaluated first-match-wins in the sequence given in + [Guard evaluation order](#guard-evaluation-order), and one skip shall produce **at most one** record + of the dispatch's own: that of the guard that fired when it is a recording guard, and none at all + when it is silent. No later guard's record shall be emitted. **The relative order of two + silent guards is not part of this criterion** — it is unobservable and therefore not contractual; + only a recording guard's position is. Two classes of log line do not count against the "at most + one", neither being this card's: those the surrounding handler writes before the dispatch runs + (`handleRecoverableFailure`'s own session-reload WARNING), and those a shared helper the dispatch + calls writes for itself (`otherWorkingSessionID`'s list-failure WARNING — see AC-F6). + [AC-E4](#e-action-vocabulary-and-transitions)'s walk WARNING **is** this card's and is likewise + not counted, because it is not a dispatch record ([AC-A6](#a-dispatch)); it cannot in any case + co-occur with a guard skip, since the walk runs at the dispatch, after every guard in + [Guard evaluation order](#guard-evaluation-order) has passed. +- **AC-F9** Every record the dispatch emits — the AC-A6 INFO, the AC-E2 DEBUG, and each recording + guard's line — shall carry `task_id`, `session_id` and a message string stable and unique to this + dispatch within the orchestrator package. Without that identity AC-F8's "no later guard's record" + is unassertable: the log stream already carries the two classes AC-F8 exempts, and a test cannot + tell them from the dispatch's own. [AC-E4](#e-action-vocabulary-and-transitions)'s walk WARNING + is **not** a dispatch record ([AC-A6](#a-dispatch) defines that set) and is outside this + criterion's enumeration; it carries the six fields AC-E4 requires, which include `task_id` and + `session_id`, so it stays identifiable in a test without being counted. + +## Concurrency and ordering + +- **Two failures, same session, in sequence.** `cancelInFlight` serializes bookkeeping, then is + released before dispatch so callbacks can reacquire it. Distinct executions produce distinct + operation ids (AC-C1) and both dispatch; a repeat is absorbed by AC-C3. +- **Two failures, two sessions, same task, concurrent.** Not serialized — the guard is per-session. + AC-F5 resolves the common shape: whichever session settles while the other is still working skips. + IF both reach a terminal state such that neither observes the other as working, THEN both may + dispatch, and `ApplyTransition` is unguarded, so **the last transition to commit wins**, with no + ordering required. Accepted; a compare-and-swap here is out of scope. +- **A user cancel racing an already-claimed retry timer.** `runTransientRetry` claims its entry before + calling `retryTransientPrompt`, while `CancelTransientRetry` reads `active` and only then clears + state — but clearing it calls `entry.cancel()` unconditionally, with no `claimed` check, and that + cancels the `retryCtx` the claimed timer is still running on. `retryTransientPrompt` then has four + `ctx.Err()` early returns before its R4 / R5 calls. So a won claim does **not** guarantee arrival: + the timer dispatches if it gets past those checks first and returns silently if the cancel does. + Both deliveries funnel through `handleRecoverableFailure` and serialize on `cancelInFlight`, so + this is an ordering outcome, not a data race. The total is therefore **one or zero, never two** — + [AC-A10](#a-dispatch) fixes which part is guaranteed (the non-leak of AC-A8's marker) and + [AC-B5](#b-office-isolation) names the zero as legitimate. +- **Ordering against the surrounding handler** is fixed by AC-A3 and is total: session state → REVIEW + → step-completion reconciliation → `cleanupAgentExecution` launch → dispatch. +- **Ordering of actions within one dispatch** is the stored order of the step's + `events.on_agent_error` JSON array — `compileGenericActions` preserves it, `evaluateActions` + iterates it — with first-eligible-transition-wins as tiebreak (AC-E7). Unchanged by this card. + +## Failure modes + +- **`auto_start_agent` under `on_agent_error` is an unbounded restart loop, and this card does not + bound it.** `autoStartAgentCallback` calls `LaunchSession` with the failed session's own id, so it + relaunches the agent that just failed; the repeat failure has a new execution id, mints a new + operation id (AC-C1) and must dispatch again (AC-C4) — indefinitely, no backoff, no cap, and it is + the most obvious thing an operator would configure here. **Accepted residual:** the loop is a + property of that configuration rather than of the dispatch, bounding it needs per-session attempt + state that does not exist, and AC-A6's one INFO per dispatch makes it visible from the first + iteration. A `move_to_step` whose destination declares `on_enter: auto_start_agent` produces the + identical loop one step over, via AC-E3. +- **Ownership flips mid-failure.** A task whose `project_id` is cleared, or whose workspace + `office_workflow_id` changes, between Office's `resolveLifecycleRun` and this site's task load while + an Office run is claimed, satisfies both predicates and double-fires — the one case AC-B4 exempts. + Accepted; the outcome is one duplicate CEO run. The same window has a **second symptom**: the + handler gates its step-completion reconciliation on the *early* `isOfficeSession` read while AC-B1 + re-reads ownership *later*, so a flip between them skips the reconciliation and the dispatch + evaluates the **stale failed-at step** rather than AC-A4's destination step. Named so a builder does + not read AC-A4 as unconditional. A future card making Office ownership a stored column closes both. +- **Current step deleted between the failure and the dispatch.** `LoadStep` surfaces it as an unwrapped + `fmt.Errorf("step %s not found")` (or `"load step %s: %w"` wrapping the getter's error) with no + sentinel, so it takes AC-E5's ERROR path — louder than the DEBUG a vanished row ideally gets ([Out of scope](#out-of-scope)). +- **Engine error mid-action-list.** `evaluateActions` aborts on the first action error, so later + actions do not run and the operation is left unmarked (AC-C6); AC-E5 makes it visible at ERROR. + Abort-on-first-failure is the engine's contract for every trigger. +- **`queue_run` with `target: workspace.ceo_agent` on a kanban workspace with no CEO.** Resolution + fails, the engine errors, AC-E5 logs it, nothing else in the list runs. A misconfiguration, loud + after this card and silent before it. +- **A callback panics during dispatch.** [AC-A11](#a-dispatch)'s closure recovers it, logs ERROR with + the task and session ids, and returns — the process keeps running and no other route's handling is + affected. Accepted as a defect in that callback, not in the dispatch; the ERROR is the only signal, + there is no retry. +## Out of scope + +Each entry is a named exclusion and part of the contract. + +- **Office's Path A and Path B.** No change to `dispatchAgentErrorTrigger`, `HandleRunFailure`, + `escalateFailure` or the Office dispatcher's session resolution. AC-B3 freezes them. +- **New action kinds, and compile-time validation generally.** ADR-0004 §7's `pause_agent` and + `create_inbox_item` are not implemented and are not added. `compileGenericActions` drops an + unrecognised type — and a `move_to_step` whose `step_id` is missing or unreadable — *before* the + engine sees an action, so AC-E4's warning cannot reach either: an operator's typo'd `step_id` still + reads as configured and still does nothing. That is this card's defect shape one layer up; the fix + is compile-time validation shared by all seven Phase 2 triggers. Different site, different card. + Validating action types on the import / git-sync / step-write paths — none do today, which is how + an unrecognised type reaches the compiler at all — belongs to that same card. +- **Widening `validGenericAction`.** The embedded-YAML allow-list stays at its three Office-shaped + kinds; the four kanban-meaningful kinds remain declarable only through import, git-sync and the + step API. A template-authoring change, not a dispatch change. +- **Distinguishing an `on_agent_error` move in `task_step_transitions` or its expvar counter.** + [AC-E11](#e-action-vocabulary-and-transitions) labels the `session_step_history` ledger only, and + names why the other one is unreachable from that switch. Retargeting it means adding a value to the + closed repo-wide `steptelemetry.Trigger` enum and either branching the shared + `engineTransitionAttribution` on the engine trigger or wrapping the attribution further out — blast + radius every transition writer, not just this dispatch. An `on_agent_error` move therefore reads as + `engine_transition` there, indistinguishable from a routine engine move; a follow-up wanting them + separable in task-level telemetry must decide the attribution site for **all** engine triggers at + once. +- **A `models.ErrWorkflowStepNotFound` sentinel.** Distinguishing a vanished step from a genuine + engine error means a sentinel threaded through `workflow/repository/sqlite.go`, + `workflow/service` and `orchestrator/workflow_store.go` — shared plumbing every trigger depends on. + AC-A7 is narrowed to the session-vanished case, which already has `models.ErrTaskSessionNotFound`. +- **Making a user cancel and an already-claimed retry timer agree.** AC-A10 accepts that the outcome + of that race is decided by which side reaches `retryTransientPrompt`'s `ctx.Err()` checks first: the + timer dispatches if it gets through, and nothing dispatches if the cancel's `entry.cancel()` lands + first. Making it deterministic in *either* direction needs per-session cancel state outliving the + single event delivery AC-A8's field rides on — its own mechanism, with its own lifetime and + eviction rules. Note the seemingly obvious fix is the wrong one: detaching the post-claim + continuation from `retryCtx` would make the timer always arrive, but at the cost of re-driving the + prompt after the user pressed Cancel. +- **Bounding the `auto_start_agent` restart loop.** An accepted residual in + [Failure modes](#failure-modes); a bound needs per-session or per-step attempt accounting that does + not exist and would change what AC-C4 means. +- **Reconciling an event whose `task_id` disagrees with its session row.** [Execution + model](#execution-model) makes this unrepresentable inside the dispatch by reading only `data.TaskID` + and never `session.TaskID`. Detecting and reporting a genuine disagreement is a repository-integrity + concern shared by every consumer of `watcher.AgentEventData`, not a dispatch fix. +- **The no-session failure branch.** It cannot dispatch: engine state is keyed on + `(taskID, sessionID)` and there is no session to key on. Giving it one is not a dispatch fix. +- **Durable operation markers.** AC-C5 states the process-scoped limit; making `appliedOps` durable + is `workflow-on-enter-action-dispatch`'s step-entry-record territory. +- **Synchronizing `s.workflowEngine`'s existing readers.** AC-E4 makes *this dispatch's* engine and + registry one atomically-published value, read once. The field's other readers stay as they are: they + were already unsynchronized before this card, and fixing them is a separate change. +- **A compare-and-swap on `on_agent_error` transitions.** See + [Concurrency](#concurrency-and-ordering); last-write-wins is accepted. +- **Data-bag semantics.** No kind in this vocabulary produces a `DataPatch` today and + `EvaluateOnly: true` skips `PersistData`. `applyEngineTransition` *does* persist `result.DataPatch` + when a transition commits, so the path is empty only because no compilable kind emits one. IF a + future kind under this trigger produces a `DataPatch`, THEN this spec must be re-opened. +## Verification + +The per-criterion test plan, the fire-site pin test's syntactic definition, and the commands live in +the companion [`verification.md`](verification.md). Every criterion above is observable without +reading logs, except where a log record *is* the contract (AC-A6, AC-A8, AC-B2, AC-E2, AC-E4, AC-E5, +AC-F5, AC-F6, AC-F8, AC-F9). diff --git a/docs/specs/workflow-on-agent-error-dispatch/verification.md b/docs/specs/workflow-on-agent-error-dispatch/verification.md new file mode 100644 index 0000000000..2f2a4d2c60 --- /dev/null +++ b/docs/specs/workflow-on-agent-error-dispatch/verification.md @@ -0,0 +1,203 @@ +# Verification plan and decision record — `on_agent_error` dispatch for non-Office tasks + +## Prior art + +**Leg 1 (wiki) and Leg 2 (`saas-kb`) DID NOT RUN** — tooling-unavailable, not empty results; receipts +are in the task plan under `## Prior art — receipts`. + +**Leg 3 — in-repo prior reasoning (ran, and it outranks fresh design here).** ADR-0004 already +specified this trigger and governs three decisions: + +- §7 "fire on the failing task **at its current step**" — adopted literally; see [AC-A4](spec.md#a-dispatch). +- The engine is "the universal coordinator for task-scoped agent runs. **No discriminator + anywhere.**" An Office-only dispatcher is a discriminator, which is why the answer is a second fire + site rather than a kanban-specific parallel mechanism. +- §7's default action list names `pause_agent`, `queue_run` and `create_inbox_item`. **We depart + here**: the first and third are not `ActionKind` constants and do not exist in + `compileGenericActions`. Adding action kinds is out of scope, so the vocabulary is what the + compiler already accepts. + +**Two departures, named because a builder will otherwise trip on them.** ADR-0004 also says +`on_agent_error` fires "only after the runs queue's retry policy exhausts (4 attempts at +[2m, 10m, 30m, 2h])". The shipped Office path does **not** — `event_subscribers.go:654`, "Office +failure path (v1): every agent error is terminal", backed by `AC-OFFICE-RUNTIME-001.1`. This spec +follows shipped Office behaviour, so both paths have the same firing condition. + +## Dispatch decision + +**Terminal agent-session failure for a non-Office task fires `TriggerOnAgentError` from +`orchestrator.Service.handleRecoverableFailureLockedState`, gated on a fail-closed Office-ownership +check and on the failure not being user-initiated.** + +Why that function and not `handleAgentFailedLocked`, the more obvious reading of "the orchestrator's +agent-failure path": it is **the single terminal funnel**. All six routes reaching a terminal +session-backed kanban failure converge here ([Scope](spec.md#scope)), so a site placed after the +`handleRecoverableFailureLockedState(...)` call inside `handleAgentFailedLocked` would miss five. It +is also where the card said to look — "alongside where it already reconciles task state for runtime +failures": it completes the turn, persists the last agent error, sets session state, writes task +REVIEW, and reconciles a pending step-completion signal. + +**A retry in flight never reaches it, but a retry that has given up does.** The first delivery of a +transient provider error returns at `handleTransientFailure`. The retry loop's own **terminal exits** +reach it through `handleRecoverableFailure`; two of those three are genuine failures and the third is +a user pressing Cancel. See [AC-A8](spec.md#a-dispatch), [AC-A9](spec.md#a-dispatch), [AC-A10](spec.md#a-dispatch). + +Why this cannot become a third path overlapping Office's two. Path A (`dispatchAgentErrorTrigger`) +requires a claimed Office `*models.Run`; Path B (`HandleRunFailure` → `escalateFailure`) queues its +own CEO run and never reaches Path A. Both are Office-run-scoped by construction. But the Office +subscriber and the orchestrator watcher both subscribe to `events.AgentFailed` +(`event_subscribers.go:264`, `watcher/watcher.go:330`), so **both handlers run on every failure** and +without an explicit gate the new site fires alongside Path A on every Office failure. The two +operation-id namespaces are disjoint by construction, so a shared key can never absorb that overlap: +the gate is the only thing preventing it, which is why [AC-B1](spec.md#b-office-isolation) fails closed. + +## Why the empty-execution-id fallback exists + +Not because the synthetic events omit the execution id — all four `watcher.AgentEventData` sites set +it. It exists because the field can still arrive **empty**: `CancelTransientRetry` discards +`GetExecutionIDForSession`'s error, `retryTransientPrompt` guards teardown with `if execID != ""`, +and a start failure can be raised before an execution row exists. Rare but reachable; the collapsed +key's cost is [AC-C8](spec.md#c-idempotency)'s. + +## Why the dispatch runs off the session guard + +Rationale for [`spec.md`](spec.md#off-the-session-guard)'s off-guard rule and [AC-A11](spec.md#a-dispatch). + +**Why the alternative was rejected.** Excluding `auto_start_agent` reproduces this card's own defect +one action over — an action that parses, compiles, appears in the step definition and silently never +runs, for the most obvious thing an operator configures under a recovery trigger. It is also a +per-kind allow-list needing maintenance forever: any future callback reaching a guard-acquiring path +reopens the hole, where running off the guard closes it for every kind at once. The vocabulary row +stays "yes". + +**Why a synchronous release-then-dispatch was chosen over a dispatch goroutine.** Both close the +self-deadlock: the failing property is holding the guard while calling the engine, not the choice of +goroutine boundary. A goroutine adds a completion hook for tests, panic recovery of its own, and an +async lifecycle to reason about at shutdown; releasing the guard before an ordinary synchronous call +needs none of that — the call returns like any other, a test drives it directly, and there is nothing +to drain. The one thing the goroutine design bought for free — a recover above every route — is not +free here, because routes R2-R5 never reach the event bus's own `invokeHandler` wrapper. AC-A11 +recovers this explicitly at the closure itself rather than by picking a mechanism that recovers it +incidentally. + +**What it costs.** This dispatch is no longer serialized with the session's other decisions the way +it was while running under the guard. [Concurrency](spec.md#concurrency-and-ordering) and +[Failure modes](spec.md#failure-modes) enumerate and accept each consequence; none is a data race. + +## Verification + +Every criterion below is observable without reading logs, except where a log record *is* the +contract (AC-A6, AC-A8, AC-B2, AC-E2, AC-E4, AC-E5, AC-F5, AC-F6, AC-F8, AC-F9). + +- **Fire-site invariant.** A test asserting `TriggerOnAgentError`'s production fire sites are exactly + two — the regression guard for the defect itself, failing if a third partial dispatcher is added or + either is removed. Copy the **design** of + `task/repository/sqlite/step_transition_writers_pin_test.go`: a `go/ast` walk keying on a + *statement shape*, comparing enclosing function identities against a registered list. + + **A fire site is defined syntactically**, because a bare identifier scan also matches the constant + declaration, the `compileGenericActions` trigger-map entry, a payload doc comment and four + session-resolution references in `internal/office/engine_dispatcher/dispatcher.go`. A fire site is + an occurrence in exactly one of two positions: (1) the value of the `Trigger:` key in an + `engine.HandleInput` composite literal — the orchestrator shape; or (2) a direct call argument to + `dispatchEngineTrigger` — the Office shape, which passes the trigger positionally. A + `*ast.BinaryExpr` operand, a `case` clause, a `const` declaration, a map-literal key and a comment + are **not** fire sites. That predicate, not a directory allowlist, discriminates, so the **scan root + is the whole backend tree** (`apps/backend`, `_test.go` excluded). Assert the set of enclosing + function identities, package-qualified as the prior art qualifies them, equals exactly + `{office/service/Service.dispatchAgentErrorTrigger, + orchestrator/Service.dispatchKanbanAgentErrorTrigger}`. A builder who names the second identity + differently shall update the registered set in the same commit. +- **AC-A1/A2/A9** — one test per dispatching route R1–R5, each asserting exactly one dispatch with + the expected trigger and payload. Five tests, not three. +- **AC-A8** — the `CancelTransientRetry` route produces zero dispatches, asserted on the explicit + marker, not the cancel message text. **AC-A10** — a claimed retry entry that **does** reach R4/R5 + dispatches while the cancel's own delivery does not, proving AC-A8's marker did not leak to the + timer's event. Assert **at most one**, never exactly one, and do not assert the timer arrives: a + cancel that cancels `retryCtx` first legitimately yields zero (AC-B5), so a test demanding one + would be asserting a race it cannot win. Drive the marker, not the interleaving. +- **AC-A11** — two tests. **(1) The regression guard for the defect that reopened this spec.** A + non-Office task whose current step declares exactly `on_agent_error: [auto_start_agent]`, driven + through a **guard-holding** entry point — `handleAgentFailed` or `handleRecoverableFailure`, never + a direct call to the dispatch function — completes within a bounded deadline. Because a dispatch + that runs while still holding the guard **hangs** rather than returning a wrong value, the test + shall enforce its own deadline and fail at it. **(2) A panicking dispatch does not take the process + down.** Inject the panic through a registered callback's `Execute` — the only available seam, not a + stub engine — drive it through a guard-holding entry point, and assert the entry point still + returns, the ERROR record names the task and session ids, and no dispatch INFO/DEBUG record is + also emitted for that delivery. +- **AC-A4** — a task with a pending step-completion signal that reconciles into a transition, + asserting the trigger is evaluated against the destination step. Because `PreloadedState` bypasses + `LoadState`, this fails if the handler builds state before the reconciliation rather than after. +- **AC-B1/B4/B5** — an Office-owned task's failure produces one dispatch and it is not the + orchestrator's; an Office-owned task with no claimed run produces zero; a task-load error (AC-B2) + produces zero. +- **AC-C1/C2/C3/C4** — key shape, empty-execution fallback, replay suppression, two executions on one + session each firing. **AC-C6** — an action that errors leaves the operation unmarked and a + redelivery re-runs the list. **AC-C7** — a declined transition is not retried. **AC-C8** — two + failures on one session both carrying an empty `AgentExecutionID` produce one dispatch and the + second reports `Idempotent: true`; AC-C4's test must not be written to contradict this. +- **AC-A6/E2** — the four-case record table is total and disjoint: a step with one declared action + emits the INFO and no DEBUG; a step with none emits the DEBUG and no INFO; the idempotent replay of + either emits neither; an engine error emits only AC-E5's ERROR. Asserting one case without + asserting the absence of the other three does not satisfy this. All four are assertions about + **dispatch records**, so run them on a step whose declared actions are all registered and no + AC-E4 WARNING is in play. Then add one mixed case — a step declaring an **unregistered** action — + asserting that the AC-E4 WARNING and the applicable dispatch record are **both** emitted and that + the pair does not violate "at most one", because only the second is counted. A test that counts + every log line the dispatch emits, rather than every dispatch record, fails on that case. +- **AC-E3** — a step declaring `move_to_step` under `on_agent_error` moves the task **and** dispatches + the destination step's `on_enter`, using a destination with no WIP limit; asserting the move alone + passes with the defect this criterion exists to prevent. Assert the `taskDescription` argument + separately and observably: give the task a **non-empty, distinctive** description and a destination + step declaring `on_enter: auto_start_agent`, then assert that description reaches the prompt + `buildWorkflowPrompt` produces. A test that only asserts the move and the `on_enter` fires passes + with the empty string this criterion forbids. **AC-E10** — the WIP-queued destination + defers `on_enter` instead. **AC-E11** — that same move writes a **`session_step_history`** row whose + `trigger` is `on_agent_error`, not `auto_complete`, and the existing labels are unchanged for their + own triggers. Do **not** assert against `task_step_transitions.trigger`: that column is fed by the + separate `steptelemetry.Trigger` enum, still reads `engine_transition` under this trigger, and is + named [out of scope](spec.md#out-of-scope) — a test asserting `on_agent_error` there cannot pass. +- **AC-E4** — a kind with no registered callback produces the WARNING with all six named fields; a + step declaring **two** `move_to_step` actions produces **no** warning, the false positive the + transition carve-out exists to prevent; and an **idempotent redelivery** for a step with an + unregistered action still produces that WARNING while producing no dispatch record, which is the + carve-out AC-A6 states and which fails if the builder gates the walk on the operation id. + **AC-E12/E13** — a `Set*` call between construction and + dispatch does not make `queue_run` warn, which fails if the value was snapshotted at construction; + and the walk reads through the engine's own store. Do not assert walk-vs-engine version equality: + two `LoadStep` calls with an invalidation between them may legitimately differ (AC-E12's accepted + residual). **AC-E14** — **all three** outcomes after a failed walk, since the cache never caches + the walk's error: when the engine's `LoadStep` also fails, exactly one dispatch record and it is + AC-E5's ERROR; when it succeeds, the dispatch still happens and records AC-A6's INFO or AC-E2's DEBUG per + the result; and when the operation id was already applied, the engine short-circuits above the step + load so no `LoadStep` runs and no dispatch record is written (AC-C3). Asserting only the first case leaves + the contradiction this criterion was rewritten to remove untested, asserting only the first two + treats the `LoadStep` branches as the whole matrix, and a failing walk is never a skip. + **AC-E7/E9** — the second + transition does not transition; a guard-unsatisfied transition and a `requires_approval` transition + neither transition nor warn. +- **AC-F1..F7** — one skip case each, including a failing state-reload (AC-F6) and the + `otherWorkingSessionID` failure asserting the dispatch adds **no** record of its own. + **AC-F8** — at least two failures satisfying two guards at once, each pair chosen so it is + **observable** (at least one of the two records) or the test proves nothing. A sessionless event on + an unloadable task — AC-A5 silent, then AC-B2 WARNING — asserts the **absence** of the WARNING. A + cancelled retry on an archived task — AC-A8 DEBUG, then AC-F4 silent — asserts the **presence** of + AC-A8's DEBUG, the discriminator being the earlier guard's record since AC-F4 has none. Do **not** + write an ordering test for a silent/silent pair such as AC-F2 against AC-F3. Add a **session-read + failure on a task that is also unloadable**, asserting exactly one WARNING and that it is AC-F6's, + which pins the session-before-task order and AC-F6-vs-AC-B2 ownership together. **AC-F9** — every + dispatch record carries `task_id`, `session_id` and its stable message, which is what lets these + assertions key on the dispatch's own lines rather than the pre-existing ones sharing the stream. A + per-guard suite that never overlaps two guards cannot detect a wrong order. +- **Office regression** — the existing `internal/office/service` and + `internal/office/engine_dispatcher` `on_agent_error` tests pass with no edits to their assertions. + +Commands: + +``` +cd apps/backend +go test ./internal/orchestrator/... -run 'AgentError|AgentFailed|RecoverableFailure' +go test ./internal/office/... -run 'AgentError' +go test ./internal/workflow/engine/... +```