From 2d73a3d1dbf321d75b809a06862fcb514edf3f89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 30 Jun 2026 13:10:40 +0100 Subject: [PATCH] fix: keep completed office comments editable (#1548) * fix: keep completed office comments editable Keep the office task detail composer editable after completed sessions load, including projectless office tasks. Update verification tests for SHA-pinned GitHub actions. Extract the dockview main area to satisfy the existing max-lines lint rule. * fix: preserve upload artifact version guard * fix: clarify upload artifact pin check * fix: wake completed office sessions on comments * fix: queue completed office comment runs * fix: preserve dispatcher session lookup errors * fix: resolve office comment wake review issues * fix: stabilize comment wake idempotency * fix: salt custom comment wake keys * fix: address comment wake review cleanup * fix: avoid comment payload allocation overflow pattern * fix: salt comment wake action variants * fix: resolve cross-task primary by target step * fix: resolve cross-task participant roles * fix: surface cross-task comment run status * fix: use target steps for cross-task queue runs * fix: avoid comment run lookup scan regression * fix: cover cross-task queue run edge cases * fix: index comment payload run lookup * fix: support postgres comment run index * fix: tighten queue run target test coverage * fix: route source comment run updates * fix: preserve cross-task comment wakes * fix: tighten comment wake identity * fix: align comment wake routing * fix: remove stale primary resolver * fix: dedupe comment run status * fix: tighten legacy comment wake dedupe * fix: remove unreachable comment wake guard * fix: skip duplicate comment reactivity wakes * fix: route comment wakes synchronously * fix: share comment engine dispatch contracts * fix: preserve comment wake fallbacks * fix: preserve done comment runner * fix: guard empty task runner fallback * fix: handle empty task runner fallback * fix: stabilize completed comment wakes * fix: index comment runner fallback --------- Co-authored-by: root --- .../config/workflows/builtin_engine_test.go | 19 + .../config/workflows/office-default.yml | 7 + apps/backend/internal/backendapp/main.go | 5 +- apps/backend/internal/office/dashboard/dto.go | 8 +- .../internal/office/dashboard/handler_test.go | 119 +++++ .../internal/office/dashboard/run_detail.go | 12 +- .../office/dashboard/run_detail_test.go | 58 ++ .../internal/office/dashboard/service.go | 54 +- .../office/dashboard/service_tasks.go | 58 +- .../dashboard/session_termination_test.go | 4 +- .../office/engine_dispatcher/dispatcher.go | 91 +++- .../engine_dispatcher/dispatcher_test.go | 443 +++++++++++++++- .../internal/office/repository/sqlite/base.go | 25 +- .../repository/sqlite/base_migrations.go | 8 + .../office/repository/sqlite/base_test.go | 11 + .../office/repository/sqlite/runs_test.go | 46 ++ .../office/repository/sqlite/tasks_test.go | 35 ++ .../office/scheduler/dashboard_adapter.go | 9 +- .../scheduler/dashboard_adapter_test.go | 26 + .../internal/office/scheduler/reactivity.go | 5 +- .../office/service/engine_dispatcher.go | 40 +- .../office/service/event_subscribers.go | 10 +- .../service/event_subscribers_engine_test.go | 106 +++- .../office/service/event_subscribers_test.go | 3 +- apps/backend/internal/office/service/run.go | 7 +- .../service/run_lifecycle_events_test.go | 52 ++ .../internal/office/service/scheduler_runs.go | 7 +- .../internal/office/service/service.go | 2 +- .../internal/office/shared/interfaces.go | 20 + .../orchestrator/workflow_callbacks.go | 17 + .../internal/runs/commentkeys/commentkeys.go | 80 +++ .../runs/commentkeys/commentkeys_test.go | 81 +++ .../internal/runs/repository/sqlite/runs.go | 91 +++- apps/backend/internal/runs/service/service.go | 100 ++-- .../internal/runs/service/service_test.go | 307 ++++++++++- .../internal/scheduler/cron/heartbeat.go | 2 +- .../internal/workflow/adapters/adapters.go | 32 +- .../workflow/adapters/adapters_test.go | 42 +- .../internal/workflow/engine/engine.go | 3 +- .../engine/office_default_smoke_test.go | 2 +- .../workflow/engine/phase2_callbacks.go | 196 ++++++- .../workflow/engine/queue_run_test.go | 500 +++++++++++++++++- .../workflow/repository/phase2_sqlite.go | 48 +- .../workflow/repository/phase2_sqlite_test.go | 92 ++++ .../task/dockview-desktop-layout.tsx | 48 +- .../task/simple/OfficeSimplePane.test.tsx | 178 +++++++ .../task/simple/OfficeSimplePane.tsx | 66 ++- .../tests/office/mobile-comment-input.spec.ts | 31 ++ scripts/opencode-code-review.test.sh | 22 +- 49 files changed, 2929 insertions(+), 299 deletions(-) create mode 100644 apps/backend/internal/office/scheduler/dashboard_adapter_test.go create mode 100644 apps/backend/internal/runs/commentkeys/commentkeys.go create mode 100644 apps/backend/internal/runs/commentkeys/commentkeys_test.go create mode 100644 apps/web/components/task/simple/OfficeSimplePane.test.tsx create mode 100644 apps/web/e2e/tests/office/mobile-comment-input.spec.ts diff --git a/apps/backend/config/workflows/builtin_engine_test.go b/apps/backend/config/workflows/builtin_engine_test.go index c6dc01f39e..dc5bda525b 100644 --- a/apps/backend/config/workflows/builtin_engine_test.go +++ b/apps/backend/config/workflows/builtin_engine_test.go @@ -55,6 +55,25 @@ func TestOfficeDefault_TriggersCompileThroughEngine(t *testing.T) { t.Errorf("Work.on_comment[0] target = %q, want primary", commentActions[0].QueueRun.Target) } + done := findStep(tmpl.Steps, "Done") + if done == nil { + t.Fatal("Done step not present in template") + } + doneSpec := engine.CompileStep(stepFromDefinition(*done)) + doneCommentActions := doneSpec.Events[engine.TriggerOnComment] + if len(doneCommentActions) != 1 { + t.Fatalf("Done.on_comment compiled to %d actions, want 1", len(doneCommentActions)) + } + if doneCommentActions[0].Kind != engine.ActionQueueRun { + t.Errorf("Done.on_comment[0].Kind = %q, want queue_run", doneCommentActions[0].Kind) + } + if doneCommentActions[0].QueueRun == nil { + t.Fatal("Done.on_comment[0].QueueRun is nil") + } + if doneCommentActions[0].QueueRun.Target != engine.TargetPrimary { + t.Errorf("Done.on_comment[0] target = %q, want primary", doneCommentActions[0].QueueRun.Target) + } + review := findStep(tmpl.Steps, "Review") if review == nil { t.Fatal("Review step not present in template") diff --git a/apps/backend/config/workflows/office-default.yml b/apps/backend/config/workflows/office-default.yml index 5fa54a7a0c..beb99cf995 100644 --- a/apps/backend/config/workflows/office-default.yml +++ b/apps/backend/config/workflows/office-default.yml @@ -122,3 +122,10 @@ steps: stage_type: custom allow_manual_move: true show_in_command_panel: true + events: + on_comment: + - type: queue_run + config: + target: primary + task_id: this + reason: task_comment diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 04858bd034..24403d1e0a 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -1111,6 +1111,9 @@ func startOfficeSchedulersAndGC( engineDispatcher := wireWorkflowEngineForOffice( orchestratorSvc, services.Office, services.Task, services.Workflow, repos, runsSvc, log, ) + if services.OfficeSvcs != nil { + services.OfficeSvcs.Dashboard.SetWorkflowEngineDispatcher(engineDispatcher) + } // Start the runs scheduler (tick + signal listener). It drives // orchScheduler.Tick on both periodic ticks and event-driven signals. runScheduler := runsscheduler.New( @@ -1149,7 +1152,7 @@ func startOfficeSchedulersAndGC( // - RunQueueAdapter — runs service (Phase 3.1) // - ParticipantStore — workflow_step_participants // - DecisionStore — workflow_step_decisions -// - PrimaryAgentResolver — workflow_steps.agent_profile_id +// - PrimaryAgentResolver — current task runner / workflow_steps.agent_profile_id // - CEOAgentResolver — agent_profiles WHERE role='ceo' AND workspace_id != ” // // The orchestrator's engine is rebuilt with these options applied, then diff --git a/apps/backend/internal/office/dashboard/dto.go b/apps/backend/internal/office/dashboard/dto.go index 18274d6cb5..7c5d46b8ec 100644 --- a/apps/backend/internal/office/dashboard/dto.go +++ b/apps/backend/internal/office/dashboard/dto.go @@ -414,10 +414,10 @@ type CommentDTO struct { Source string `json:"source"` CreatedAt string `json:"createdAt"` // Per-comment run lifecycle. Only populated for user comments - // whose comment_created subscriber queued a task_comment run - // (idempotency_key = "task_comment:"). The frontend - // reads these to render a Queued / Working / Failed badge on the - // triggering comment. + // whose comment_created subscriber queued a task_comment run. Exact + // same-task wakes map by idempotency key; salted fan-out wakes map by + // payload.comment_id. The frontend reads these to render a Queued / + // Working / Failed badge on the triggering comment. RunID string `json:"runId,omitempty"` RunStatus string `json:"runStatus,omitempty"` RunError string `json:"runError,omitempty"` diff --git a/apps/backend/internal/office/dashboard/handler_test.go b/apps/backend/internal/office/dashboard/handler_test.go index 47f4b546c0..5803c93d34 100644 --- a/apps/backend/internal/office/dashboard/handler_test.go +++ b/apps/backend/internal/office/dashboard/handler_test.go @@ -21,6 +21,8 @@ import ( "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/office/shared" + "github.com/kandev/kandev/internal/runs/commentkeys" + "github.com/kandev/kandev/internal/workflow/engine" workflowrepo "github.com/kandev/kandev/internal/workflow/repository" ) @@ -55,6 +57,35 @@ func (s *stubSettingsProvider) UpdateRecoveryLookbackHours(_ string, _ int) erro return nil } +type recordingEngineDispatcher struct { + calls []recordedEngineDispatch + err error + handled bool +} + +type recordedEngineDispatch struct { + taskID string + trigger engine.Trigger + payload any + opID string +} + +func (r *recordingEngineDispatcher) HandleTrigger( + ctx context.Context, taskID string, trigger engine.Trigger, payload any, opID string, +) error { + _, err := r.HandleTriggerHandled(ctx, taskID, trigger, payload, opID) + return err +} + +func (r *recordingEngineDispatcher) HandleTriggerHandled( + _ context.Context, taskID string, trigger engine.Trigger, payload any, opID string, +) (bool, error) { + r.calls = append(r.calls, recordedEngineDispatch{ + taskID: taskID, trigger: trigger, payload: payload, opID: opID, + }) + return r.handled, r.err +} + // testDeps wires a real in-memory dashboard service for testing. type testDeps struct { db *sqlx.DB @@ -290,6 +321,94 @@ func TestCreateComment_PublishesOfficeCommentCreated(t *testing.T) { } } +func TestCreateComment_SuppressesLegacyAssigneeWakeAfterEngineDispatch(t *testing.T) { + deps := newTestDeps(t) + insertTestTask(t, deps.db, "task-comment-run", "ws-1", "Comment Run", "todo", 1) + + rt := &recordingReactivity{result: &dashboard.TaskReactivityResult{}} + disp := &recordingEngineDispatcher{handled: true} + eb := bus.NewMemoryEventBus(logger.Default()) + var eventData map[string]string + if _, err := eb.Subscribe(events.OfficeCommentCreated, func(_ context.Context, event *bus.Event) error { + raw, ok := event.Data.(map[string]string) + if !ok { + t.Fatalf("event data type = %T, want map[string]string", event.Data) + } + eventData = raw + return nil + }); err != nil { + t.Fatalf("subscribe: %v", err) + } + deps.svc.SetReactivityApplier(rt) + deps.svc.SetWorkflowEngineDispatcher(disp) + deps.svc.SetEventBus(eb) + comment := &models.TaskComment{ + ID: "comment-existing", + TaskID: "task-comment-run", + AuthorType: "user", + AuthorID: "user-1", + Body: "Please take a look", + CreatedAt: time.Now().UTC(), + } + + if err := deps.svc.CreateComment(context.Background(), comment); err != nil { + t.Fatalf("create comment: %v", err) + } + if len(disp.calls) != 1 { + t.Fatalf("dispatcher calls = %d, want 1", len(disp.calls)) + } + if disp.calls[0].trigger != engine.TriggerOnComment { + t.Fatalf("trigger = %q, want on_comment", disp.calls[0].trigger) + } + if disp.calls[0].opID != "task_comment:comment-existing" { + t.Fatalf("operation id = %q, want task_comment:comment-existing", disp.calls[0].opID) + } + if eventData["engine_dispatched"] != commentkeys.EngineDispatchedValue { + t.Fatalf("engine_dispatched = %q, want %q", eventData["engine_dispatched"], commentkeys.EngineDispatchedValue) + } + if len(rt.calls) != 1 { + t.Fatalf("reactivity calls = %d, want 1", len(rt.calls)) + } + got := rt.calls[0] + if got.Comment == nil || got.Comment.ID != "comment-existing" { + t.Fatalf("reactivity comment = %+v, want comment-existing", got.Comment) + } + if !got.SkipAssigneeCommentWake { + t.Fatal("SkipAssigneeCommentWake = false, want true after synchronous engine dispatch") + } +} + +func TestCreateComment_KeepsLegacyWakeAfterNoopEngineDispatch(t *testing.T) { + deps := newTestDeps(t) + insertTestTask(t, deps.db, "task-comment-noop", "ws-1", "Comment Noop", "todo", 1) + + rt := &recordingReactivity{result: &dashboard.TaskReactivityResult{}} + disp := &recordingEngineDispatcher{handled: false} + deps.svc.SetReactivityApplier(rt) + deps.svc.SetWorkflowEngineDispatcher(disp) + comment := &models.TaskComment{ + ID: "comment-noop", + TaskID: "task-comment-noop", + AuthorType: "user", + AuthorID: "user-1", + Body: "Please take a look", + CreatedAt: time.Now().UTC(), + } + + if err := deps.svc.CreateComment(context.Background(), comment); err != nil { + t.Fatalf("create comment: %v", err) + } + if len(disp.calls) != 1 { + t.Fatalf("dispatcher calls = %d, want 1", len(disp.calls)) + } + if len(rt.calls) != 1 { + t.Fatalf("reactivity calls = %d, want 1", len(rt.calls)) + } + if rt.calls[0].SkipAssigneeCommentWake { + t.Fatal("SkipAssigneeCommentWake = true, want false after no-op engine dispatch") + } +} + func insertTestChildTask( t *testing.T, db *sqlx.DB, diff --git a/apps/backend/internal/office/dashboard/run_detail.go b/apps/backend/internal/office/dashboard/run_detail.go index 133aa796f1..2a29c913a8 100644 --- a/apps/backend/internal/office/dashboard/run_detail.go +++ b/apps/backend/internal/office/dashboard/run_detail.go @@ -10,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/runs/commentkeys" ) // RunDetailRepo is the subset of repo functions GetRunDetail needs. @@ -92,6 +93,15 @@ func taskIDFromPayload(payload string) string { return p.TaskID } +// runSummaryTaskIDFromPayload returns the task id used by the run list row. +// Cross-task wakes execute on payload.task_id but UI links/status badges belong +// to payload.source_task_id, so source_task_id wins when present. This is +// intentionally the link identity, not a raw mirror of the executing task_id. +func runSummaryTaskIDFromPayload(payload string) string { + taskID, _ := commentkeys.IdentityFromPayload(payload) + return taskID +} + // runLinkIDsFromPayload extracts comment_id and routine_id from the // run payload — both optional and present per wakeup source (comment // payloads carry the comment, routine payloads carry the routine). @@ -137,7 +147,7 @@ func buildRunSummaryDTO(run *models.Run) AgentRunSummaryDTO { Reason: run.Reason, Status: string(run.Status), ErrorMessage: run.ErrorMessage, - TaskID: taskIDFromPayload(run.Payload), + TaskID: runSummaryTaskIDFromPayload(run.Payload), CommentID: commentID, RoutineID: routineID, RequestedAt: run.RequestedAt.UTC().Format(time.RFC3339), diff --git a/apps/backend/internal/office/dashboard/run_detail_test.go b/apps/backend/internal/office/dashboard/run_detail_test.go index 0b2ae353ff..c465017b57 100644 --- a/apps/backend/internal/office/dashboard/run_detail_test.go +++ b/apps/backend/internal/office/dashboard/run_detail_test.go @@ -160,6 +160,64 @@ func TestListAgentRunsPaged_EmptyAgent(t *testing.T) { } } +func TestListAgentRunsPaged_UsesSourceTaskForCommentLinks(t *testing.T) { + deps := newRunDetailDeps(t) + ctx := context.Background() + run := &officemodels.Run{ + AgentProfileID: "agent-1", + Reason: "task_comment", + Payload: `{"task_id":"target-task","source_task_id":"source-task","comment_id":"cm-source"}`, + Status: "finished", + CoalescedCount: 1, + } + if err := deps.repo.CreateRun(ctx, run); err != nil { + t.Fatalf("create run: %v", err) + } + + resp, err := dashboard.ListAgentRunsPaged(ctx, deps.repo, "agent-1", "", "", 25) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(resp.Runs) != 1 { + t.Fatalf("want 1 run, got %d", len(resp.Runs)) + } + if resp.Runs[0].TaskID != "source-task" { + t.Fatalf("summary task_id = %q, want source-task", resp.Runs[0].TaskID) + } + if resp.Runs[0].CommentID != "cm-source" { + t.Fatalf("summary comment_id = %q, want cm-source", resp.Runs[0].CommentID) + } +} + +func TestListAgentRunsPaged_UsesSourceTaskWithoutCommentID(t *testing.T) { + deps := newRunDetailDeps(t) + ctx := context.Background() + run := &officemodels.Run{ + AgentProfileID: "agent-1", + Reason: "approval_resolved", + Payload: `{"task_id":"target-task","source_task_id":"source-task"}`, + Status: "finished", + CoalescedCount: 1, + } + if err := deps.repo.CreateRun(ctx, run); err != nil { + t.Fatalf("create run: %v", err) + } + + resp, err := dashboard.ListAgentRunsPaged(ctx, deps.repo, "agent-1", "", "", 25) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(resp.Runs) != 1 { + t.Fatalf("want 1 run, got %d", len(resp.Runs)) + } + if resp.Runs[0].TaskID != "source-task" { + t.Fatalf("summary task_id = %q, want source-task", resp.Runs[0].TaskID) + } + if resp.Runs[0].CommentID != "" { + t.Fatalf("summary comment_id = %q, want empty", resp.Runs[0].CommentID) + } +} + func TestListAgentRunsPaged_TieBreakOnID(t *testing.T) { deps := newRunDetailDeps(t) // Two runs with identical requested_at — must still be strictly diff --git a/apps/backend/internal/office/dashboard/service.go b/apps/backend/internal/office/dashboard/service.go index fd617121d4..d2f8841b09 100644 --- a/apps/backend/internal/office/dashboard/service.go +++ b/apps/backend/internal/office/dashboard/service.go @@ -209,8 +209,12 @@ type TaskReactivityChange struct { Comment *TaskReactivityComment ReopenIntent bool ResumeIntent bool - ActorID string - ActorType string + // SkipAssigneeCommentWake suppresses only the legacy assignee + // task_comment wake. Mention fan-out still runs so @mentions on + // the same comment remain additive. + SkipAssigneeCommentWake bool + ActorID string + ActorType string } // TaskReactivityComment is the slim view of a comment for the reactivity pipeline. @@ -320,23 +324,24 @@ type DashboardService struct { activity shared.ActivityLogger agents shared.AgentReader costs shared.CostChecker - permissions shared.PermissionLister // optional; nil means no permission items in inbox - settingsProvider SettingsProvider // optional; nil means settings endpoints are unavailable - governanceStore GovernanceSettingsStore // optional; nil means governance settings are unavailable - eb bus.EventBus // optional; nil means no events are published - retryCanceller RetryCanceller // optional; nil means retries are not cancelled on reassign - taskCanceller TaskCanceller // optional; used to hard-cancel sessions on status→cancelled - sessionTerm SessionTerminator // optional; flips office session rows to COMPLETED on participation removal - reactivity ReactivityApplier // optional; runs the office reactivity pipeline on mutations - approvalQueuer ApprovalReactivityQueuer // optional; queues approval-flow runs - skillLister SkillLister // optional; nil means skill_count is always 0 - routineLister RoutineLister // optional; nil means routine_count is always 0 - failureNotifier FailureNotifier // optional; nil means assignee changes don't auto-dismiss inbox entries - failureInbox FailureInboxSource // optional; nil disables the new agent_run_failed / agent_paused_after_failures inbox sources - markFixed MarkFixedHandler // optional; nil disables the dismiss endpoint - decisions DecisionStore // workflow-domain decisions store (ADR 0005 Wave E); nil disables decision endpoints - routingProvider RoutingProvider // optional; nil disables /routing endpoints (503) - attemptLister RouteAttemptLister // optional; nil disables attempt embedding on run-detail responses + permissions shared.PermissionLister // optional; nil means no permission items in inbox + settingsProvider SettingsProvider // optional; nil means settings endpoints are unavailable + governanceStore GovernanceSettingsStore // optional; nil means governance settings are unavailable + eb bus.EventBus // optional; nil means no events are published + retryCanceller RetryCanceller // optional; nil means retries are not cancelled on reassign + taskCanceller TaskCanceller // optional; used to hard-cancel sessions on status→cancelled + sessionTerm SessionTerminator // optional; flips office session rows to COMPLETED on participation removal + reactivity ReactivityApplier // optional; runs the office reactivity pipeline on mutations + engineDispatcher shared.WorkflowEngineDispatcher // optional; synchronously routes comment triggers through the engine + approvalQueuer ApprovalReactivityQueuer // optional; queues approval-flow runs + skillLister SkillLister // optional; nil means skill_count is always 0 + routineLister RoutineLister // optional; nil means routine_count is always 0 + failureNotifier FailureNotifier // optional; nil means assignee changes don't auto-dismiss inbox entries + failureInbox FailureInboxSource // optional; nil disables the new agent_run_failed / agent_paused_after_failures inbox sources + markFixed MarkFixedHandler // optional; nil disables the dismiss endpoint + decisions DecisionStore // workflow-domain decisions store (ADR 0005 Wave E); nil disables decision endpoints + routingProvider RoutingProvider // optional; nil disables /routing endpoints (503) + attemptLister RouteAttemptLister // optional; nil disables attempt embedding on run-detail responses } // SetRoutingProvider wires the provider-routing seam used by the @@ -457,6 +462,12 @@ func (s *DashboardService) SetReactivityApplier(r ReactivityApplier) { s.reactivity = r } +// SetWorkflowEngineDispatcher wires the synchronous workflow-engine +// dispatcher used for dashboard-created comments. +func (s *DashboardService) SetWorkflowEngineDispatcher(d shared.WorkflowEngineDispatcher) { + s.engineDispatcher = d +} + // SetApprovalReactivityQueuer sets the queuer used to fire approval-flow // runs (task_changes_requested, task_ready_to_close). When unset, the // approval-flow service still records decisions but no runs fire. @@ -644,8 +655,9 @@ func (s *DashboardService) CreateComment(ctx context.Context, comment *models.Ta if err := s.repo.CreateTaskComment(ctx, comment); err != nil { return err } - s.publishCommentCreated(ctx, comment) - s.runReactivityForComment(ctx, comment) + engineHandled := s.dispatchCommentEngineTrigger(ctx, comment) + s.publishCommentCreated(ctx, comment, engineHandled) + s.runReactivityForComment(ctx, comment, engineHandled) return nil } diff --git a/apps/backend/internal/office/dashboard/service_tasks.go b/apps/backend/internal/office/dashboard/service_tasks.go index 532bcfcfa8..51b03f6bed 100644 --- a/apps/backend/internal/office/dashboard/service_tasks.go +++ b/apps/backend/internal/office/dashboard/service_tasks.go @@ -3,6 +3,7 @@ package dashboard import ( "context" "encoding/json" + "errors" "fmt" "github.com/kandev/kandev/internal/events" @@ -11,6 +12,8 @@ import ( "github.com/kandev/kandev/internal/office/repository/sqlite" officeruntime "github.com/kandev/kandev/internal/office/runtime" "github.com/kandev/kandev/internal/office/shared" + "github.com/kandev/kandev/internal/runs/commentkeys" + "github.com/kandev/kandev/internal/workflow/engine" "go.uber.org/zap" ) @@ -779,7 +782,7 @@ func (s *DashboardService) publishTaskStatusChanged(ctx context.Context, req Tas // comment is a self-comment or the task is closed; resolves @mentions. // Best-effort. func (s *DashboardService) runReactivityForComment( - ctx context.Context, comment *models.TaskComment, + ctx context.Context, comment *models.TaskComment, engineHandled bool, ) { if s.reactivity == nil || comment == nil || comment.ID == "" { return @@ -793,6 +796,7 @@ func (s *DashboardService) runReactivityForComment( AuthorType: comment.AuthorType, AuthorID: comment.AuthorID, }, + SkipAssigneeCommentWake: engineHandled, } if _, err := s.reactivity.ApplyTaskMutation(ctx, comment.TaskID, "", change); err != nil { s.logger.Warn("reactivity pipeline failed (comment)", @@ -800,7 +804,54 @@ func (s *DashboardService) runReactivityForComment( } } -func (s *DashboardService) publishCommentCreated(ctx context.Context, comment *models.TaskComment) { +func (s *DashboardService) dispatchCommentEngineTrigger(ctx context.Context, comment *models.TaskComment) bool { + if s.engineDispatcher == nil || comment == nil || comment.TaskID == "" || comment.ID == "" { + return false + } + if s.isSelfComment(ctx, comment) { + return false + } + handled, err := s.dispatchCommentEngineTriggerOnce(ctx, comment) + if err == nil { + return handled + } + if errors.Is(err, shared.ErrEngineNoSession) { + return false + } + s.logger.Warn("engine comment trigger failed", + zap.String("task_id", comment.TaskID), + zap.String("comment_id", comment.ID), + zap.Error(err)) + return false +} + +type handledWorkflowEngineDispatcher interface { + HandleTriggerHandled(ctx context.Context, taskID string, trigger engine.Trigger, payload any, operationID string) (bool, error) +} + +func (s *DashboardService) dispatchCommentEngineTriggerOnce(ctx context.Context, comment *models.TaskComment) (bool, error) { + payload := engine.OnCommentPayload{ + CommentID: comment.ID, + AuthorID: comment.AuthorID, + } + opID := commentkeys.TaskComment(comment.ID) + if dispatcher, ok := s.engineDispatcher.(handledWorkflowEngineDispatcher); ok { + return dispatcher.HandleTriggerHandled(ctx, comment.TaskID, engine.TriggerOnComment, payload, opID) + } + err := s.engineDispatcher.HandleTrigger(ctx, comment.TaskID, engine.TriggerOnComment, + payload, opID) + return err == nil, err +} + +func (s *DashboardService) isSelfComment(ctx context.Context, comment *models.TaskComment) bool { + if comment.AuthorType != "agent" || comment.AuthorID == "" { + return false + } + fields, err := s.repo.GetTaskExecutionFields(ctx, comment.TaskID) + return err == nil && fields != nil && fields.AssigneeAgentProfileID == comment.AuthorID +} + +func (s *DashboardService) publishCommentCreated(ctx context.Context, comment *models.TaskComment, engineHandled bool) { if s.eb == nil { return } @@ -810,6 +861,9 @@ func (s *DashboardService) publishCommentCreated(ctx context.Context, comment *m "author_type": comment.AuthorType, "author_id": comment.AuthorID, } + if engineHandled { + data["engine_dispatched"] = commentkeys.EngineDispatchedValue + } event := bus.NewEvent(events.OfficeCommentCreated, "office-dashboard", data) if err := s.eb.Publish(ctx, events.OfficeCommentCreated, event); err != nil { s.logger.Error("publish comment created event failed", diff --git a/apps/backend/internal/office/dashboard/session_termination_test.go b/apps/backend/internal/office/dashboard/session_termination_test.go index c8d9f3261f..0ee901e7f2 100644 --- a/apps/backend/internal/office/dashboard/session_termination_test.go +++ b/apps/backend/internal/office/dashboard/session_termination_test.go @@ -72,9 +72,11 @@ var _ dashboard.SessionTerminator = (*recordingTerminator)(nil) type recordingReactivity struct { result *dashboard.TaskReactivityResult err error + calls []dashboard.TaskReactivityChange } -func (r *recordingReactivity) ApplyTaskMutation(_ context.Context, _ string, _ string, _ dashboard.TaskReactivityChange) (*dashboard.TaskReactivityResult, error) { +func (r *recordingReactivity) ApplyTaskMutation(_ context.Context, _ string, _ string, change dashboard.TaskReactivityChange) (*dashboard.TaskReactivityResult, error) { + r.calls = append(r.calls, change) return r.result, r.err } diff --git a/apps/backend/internal/office/engine_dispatcher/dispatcher.go b/apps/backend/internal/office/engine_dispatcher/dispatcher.go index 0cd5dedee2..6962356366 100644 --- a/apps/backend/internal/office/engine_dispatcher/dispatcher.go +++ b/apps/backend/internal/office/engine_dispatcher/dispatcher.go @@ -1,5 +1,5 @@ // Package engine_dispatcher provides the production implementation of -// office/service.WorkflowEngineDispatcher. It bridges the office service's +// office/shared.WorkflowEngineDispatcher. It bridges the office service's // typed event subscribers to the workflow engine's HandleInput envelope // by resolving the task's active session id and invoking // engine.HandleTrigger. @@ -10,25 +10,27 @@ package engine_dispatcher import ( "context" + "errors" "fmt" "github.com/kandev/kandev/internal/common/logger" - officeservice "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" taskmodels "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/workflow/engine" "go.uber.org/zap" ) -// ErrNoSession is re-exported from office/service so callers can compare +// ErrNoSession is re-exported from office/shared so callers can compare // via errors.Is without importing this package directly. Returned when a // trigger arrives for a task with no active session — engine state is // keyed on (taskID, sessionID), so the dispatcher cannot proceed without // one. -var ErrNoSession = officeservice.ErrEngineNoSession +var ErrNoSession = shared.ErrEngineNoSession -// SessionResolver looks up a task's active session. +// SessionResolver looks up a task's session state for workflow triggers. type SessionResolver interface { GetActiveTaskSessionByTaskID(ctx context.Context, taskID string) (*taskmodels.TaskSession, error) + GetTaskSessionByTaskID(ctx context.Context, taskID string) (*taskmodels.TaskSession, error) } // EngineHandle is the engine surface the dispatcher needs. Defined as a @@ -38,7 +40,7 @@ type EngineHandle interface { } // Dispatcher resolves a task's active session and invokes the workflow -// engine. It implements office/service.WorkflowEngineDispatcher. +// engine. It implements shared.WorkflowEngineDispatcher. type Dispatcher struct { engine EngineHandle sessions SessionResolver @@ -56,11 +58,11 @@ func New(eng EngineHandle, sessions SessionResolver, log *logger.Logger) *Dispat } } -// HandleTrigger satisfies office/service.WorkflowEngineDispatcher. +// HandleTrigger satisfies shared.WorkflowEngineDispatcher. // -// Resolves the task's active session — returning ErrNoSession if none -// exists — then invokes engine.HandleTrigger. Errors from the engine -// (e.g. queue_run resolver failures) bubble up so the office event +// Resolves the task's active session — or, for comment wakes, the latest +// reusable completed/idle session — then invokes engine.HandleTrigger. Errors from the +// engine (e.g. queue_run resolver failures) bubble up so the office event // subscriber can log them. func (d *Dispatcher) HandleTrigger( ctx context.Context, @@ -69,17 +71,32 @@ func (d *Dispatcher) HandleTrigger( payload any, operationID string, ) error { + _, err := d.HandleTriggerHandled(ctx, taskID, trigger, payload, operationID) + return err +} + +// HandleTriggerHandled reports whether the workflow engine found actions for +// the trigger. A no-action step is a successful no-op, but callers such as the +// dashboard still need to keep their legacy fallback wake path. +func (d *Dispatcher) HandleTriggerHandled( + ctx context.Context, + taskID string, + trigger engine.Trigger, + payload any, + operationID string, +) (bool, error) { if taskID == "" { - return fmt.Errorf("task_id is required") + return false, fmt.Errorf("task_id is required") } - session, err := d.sessions.GetActiveTaskSessionByTaskID(ctx, taskID) - if err != nil || session == nil { - // No active session — engine cannot LoadState. Caller falls - // back to the legacy QueueRun path. + session, err := d.resolveSession(ctx, taskID, trigger) + if err != nil { + return false, fmt.Errorf("resolve session: %w", err) + } + if session == nil { d.logger.Debug("engine trigger skipped: no active session", zap.String("task_id", taskID), zap.String("trigger", string(trigger))) - return ErrNoSession + return false, ErrNoSession } in := engine.HandleInput{ TaskID: taskID, @@ -88,8 +105,44 @@ func (d *Dispatcher) HandleTrigger( OperationID: operationID, Payload: payload, } - if _, err := d.engine.HandleTrigger(ctx, in); err != nil { - return fmt.Errorf("engine handle %s: %w", trigger, err) + result, err := d.engine.HandleTrigger(ctx, in) + if err != nil { + return false, fmt.Errorf("engine handle %s: %w", trigger, err) } - return nil + return result.Idempotent || result.ActionCount > 0, nil +} + +func (d *Dispatcher) resolveSession( + ctx context.Context, taskID string, trigger engine.Trigger, +) (*taskmodels.TaskSession, error) { + session, err := d.sessions.GetActiveTaskSessionByTaskID(ctx, taskID) + if err == nil && session != nil { + return session, nil + } + if err != nil && !errors.Is(err, taskmodels.ErrTaskSessionNotFound) { + return nil, fmt.Errorf("active session lookup: %w", err) + } + if trigger != engine.TriggerOnComment { + return nil, nil + } + // Comment wakes are allowed after an office task's agent session has + // completed or returned to reusable IDLE state. The workflow engine state is + // keyed by (taskID, sessionID), so a post-completion comment intentionally + // resumes the latest reusable session's persisted machine state instead of + // starting a fresh state machine here. + session, err = d.sessions.GetTaskSessionByTaskID(ctx, taskID) + if err == nil && session != nil { + if !isReusableCommentSession(session.State) { + return nil, nil + } + return session, nil + } + if err != nil && !errors.Is(err, taskmodels.ErrTaskSessionNotFound) { + return nil, fmt.Errorf("latest session lookup: %w", err) + } + return nil, nil +} + +func isReusableCommentSession(state taskmodels.TaskSessionState) bool { + return state == taskmodels.TaskSessionStateCompleted || state == taskmodels.TaskSessionStateIdle } diff --git a/apps/backend/internal/office/engine_dispatcher/dispatcher_test.go b/apps/backend/internal/office/engine_dispatcher/dispatcher_test.go index 6769a671e9..c0dc47bfcf 100644 --- a/apps/backend/internal/office/engine_dispatcher/dispatcher_test.go +++ b/apps/backend/internal/office/engine_dispatcher/dispatcher_test.go @@ -2,38 +2,212 @@ package engine_dispatcher import ( "context" + "encoding/json" "errors" + "strings" "testing" + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events/bus" + officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" + runssqlite "github.com/kandev/kandev/internal/runs/repository/sqlite" + runsservice "github.com/kandev/kandev/internal/runs/service" taskmodels "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/workflow/engine" ) type fakeSessions struct { - session *taskmodels.TaskSession - err error + activeSession *taskmodels.TaskSession + latestSession *taskmodels.TaskSession + activeErr error + latestErr error } func (f *fakeSessions) GetActiveTaskSessionByTaskID(_ context.Context, _ string) (*taskmodels.TaskSession, error) { - return f.session, f.err + return f.activeSession, f.activeErr +} + +func (f *fakeSessions) GetTaskSessionByTaskID(_ context.Context, _ string) (*taskmodels.TaskSession, error) { + return f.latestSession, f.latestErr } type fakeEngine struct { captured engine.HandleInput called bool err error + result engine.HandleResult } func (f *fakeEngine) HandleTrigger(_ context.Context, in engine.HandleInput) (engine.HandleResult, error) { f.called = true f.captured = in - return engine.HandleResult{}, f.err + return f.result, f.err +} + +type realRunsAdapter struct { + svc *runsservice.Service +} + +func (a realRunsAdapter) QueueRun(ctx context.Context, req engine.QueueRunRequest) error { + return a.svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: req.AgentProfileID, + TaskID: req.TaskID, + WorkflowStepID: req.WorkflowStepID, + Reason: req.Reason, + IdempotencyKey: req.IdempotencyKey, + Payload: req.Payload, + }) +} + +type stubPrimary struct { + id string +} + +func (s stubPrimary) PrimaryAgentProfileID(_ context.Context, _, _ string) (string, error) { + return s.id, nil +} + +type commentWorkflowStore struct{} + +func (commentWorkflowStore) LoadState(_ context.Context, taskID, sessionID string) (engine.MachineState, error) { + return engine.MachineState{ + TaskID: taskID, + SessionID: sessionID, + WorkflowID: "workflow-1", + CurrentStepID: "work", + }, nil +} + +func (commentWorkflowStore) LoadStep(_ context.Context, _, stepID string) (engine.StepSpec, error) { + return engine.StepSpec{ + ID: stepID, + WorkflowID: "workflow-1", + Events: map[engine.Trigger][]engine.Action{ + engine.TriggerOnComment: { + { + Kind: engine.ActionQueueRun, + QueueRun: &engine.QueueRunAction{ + Target: "primary", + TaskID: "this", + Reason: "task_comment", + }, + }, + }, + }, + }, nil +} + +func (commentWorkflowStore) LoadNextStep(context.Context, string, int) (engine.StepSpec, error) { + return engine.StepSpec{}, errors.New("unexpected next-step lookup") +} + +func (commentWorkflowStore) LoadPreviousStep(context.Context, string, int) (engine.StepSpec, error) { + return engine.StepSpec{}, errors.New("unexpected previous-step lookup") +} + +func (commentWorkflowStore) ApplyTransition(context.Context, string, string, string, string, engine.Trigger) error { + return errors.New("unexpected transition") +} + +func (commentWorkflowStore) PersistData(context.Context, string, map[string]any) error { + return nil +} + +func (commentWorkflowStore) IsOperationApplied(context.Context, string) (bool, error) { + return false, nil +} + +func (commentWorkflowStore) MarkOperationApplied(context.Context, string) error { + return nil +} + +type transitionWorkflowStore struct { + appliedTaskID string + appliedSessionID string + appliedFrom string + appliedTo string + appliedTrigger engine.Trigger +} + +func (s *transitionWorkflowStore) LoadState(_ context.Context, taskID, sessionID string) (engine.MachineState, error) { + return engine.MachineState{ + TaskID: taskID, + SessionID: sessionID, + WorkflowID: "workflow-1", + CurrentStepID: "review", + }, nil +} + +func (s *transitionWorkflowStore) LoadStep(_ context.Context, _, stepID string) (engine.StepSpec, error) { + return engine.StepSpec{ + ID: stepID, + WorkflowID: "workflow-1", + Events: map[engine.Trigger][]engine.Action{ + engine.TriggerOnComment: { + { + Kind: engine.ActionMoveToStep, + MoveToStep: &engine.MoveToStepAction{StepID: "follow-up"}, + }, + }, + }, + }, nil +} + +func (s *transitionWorkflowStore) LoadNextStep(context.Context, string, int) (engine.StepSpec, error) { + return engine.StepSpec{}, errors.New("unexpected next-step lookup") +} + +func (s *transitionWorkflowStore) LoadPreviousStep(context.Context, string, int) (engine.StepSpec, error) { + return engine.StepSpec{}, errors.New("unexpected previous-step lookup") +} + +func (s *transitionWorkflowStore) ApplyTransition( + _ context.Context, taskID, sessionID, fromStepID, toStepID string, trigger engine.Trigger, +) error { + s.appliedTaskID = taskID + s.appliedSessionID = sessionID + s.appliedFrom = fromStepID + s.appliedTo = toStepID + s.appliedTrigger = trigger + return nil +} + +func (s *transitionWorkflowStore) PersistData(context.Context, string, map[string]any) error { + return nil +} + +func (s *transitionWorkflowStore) IsOperationApplied(context.Context, string) (bool, error) { + return false, nil +} + +func (s *transitionWorkflowStore) MarkOperationApplied(context.Context, string) error { + return nil +} + +func newDispatcherRunsService(t *testing.T) (*runsservice.Service, *runssqlite.Repository) { + t.Helper() + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + officeRepo, err := officesqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init office repo: %v", err) + } + log := logger.Default() + runsRepo := officeRepo.RunsRepository() + svc := runsservice.New(runsRepo, bus.NewMemoryEventBus(log), log, nil) + return svc, runsRepo } func TestDispatcher_ResolvesSessionAndForwards(t *testing.T) { eng := &fakeEngine{} - sessions := &fakeSessions{session: &taskmodels.TaskSession{ID: "sess-1"}} + sessions := &fakeSessions{activeSession: &taskmodels.TaskSession{ID: "sess-1"}} d := New(eng, sessions, logger.Default()) err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, @@ -55,6 +229,195 @@ func TestDispatcher_ResolvesSessionAndForwards(t *testing.T) { } } +func TestDispatcher_HandleTriggerHandledReportsNoop(t *testing.T) { + eng := &fakeEngine{result: engine.HandleResult{}} + sessions := &fakeSessions{activeSession: &taskmodels.TaskSession{ID: "sess-1"}} + d := New(eng, sessions, logger.Default()) + + handled, err := d.HandleTriggerHandled(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{CommentID: "c-1"}, "task_comment:c-1") + if err != nil { + t.Fatalf("handle: %v", err) + } + if handled { + t.Fatal("handled = true, want false for no-action engine result") + } +} + +func TestDispatcher_UsesLatestSessionForCommentWhenActiveSessionMissing(t *testing.T) { + for _, state := range []taskmodels.TaskSessionState{ + taskmodels.TaskSessionStateCompleted, + taskmodels.TaskSessionStateIdle, + } { + t.Run(string(state), func(t *testing.T) { + eng := &fakeEngine{} + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestSession: &taskmodels.TaskSession{ + ID: "sess-reusable", + State: state, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{CommentID: "c-1"}, "task_comment:c-1") + if err != nil { + t.Fatalf("handle: %v", err) + } + if !eng.called { + t.Fatal("engine not invoked") + } + if eng.captured.SessionID != "sess-reusable" { + t.Errorf("session id = %q, want sess-reusable", eng.captured.SessionID) + } + }) + } +} + +func TestDispatcher_SkipsLatestSessionForCommentWhenNotReusable(t *testing.T) { + for _, state := range []taskmodels.TaskSessionState{ + taskmodels.TaskSessionStateFailed, + taskmodels.TaskSessionStateCancelled, + } { + t.Run(string(state), func(t *testing.T) { + eng := &fakeEngine{} + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestSession: &taskmodels.TaskSession{ + ID: "sess-latest", + State: state, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{CommentID: "c-1"}, "task_comment:c-1") + if !errors.Is(err, ErrNoSession) { + t.Fatalf("err = %v, want ErrNoSession", err) + } + if eng.called { + t.Fatal("engine should not be invoked for non-completed latest session") + } + }) + } +} + +func TestDispatcher_CompletedSessionCommentQueuesRun(t *testing.T) { + runsSvc, runsRepo := newDispatcherRunsService(t) + eng := engine.New(commentWorkflowStore{}, engine.MapRegistry{ + engine.ActionQueueRun: engine.QueueRunCallback{ + Adapter: realRunsAdapter{svc: runsSvc}, + Primary: stubPrimary{id: "agent-primary"}, + }, + }) + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestSession: &taskmodels.TaskSession{ + ID: "sess-completed", + State: taskmodels.TaskSessionStateCompleted, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{CommentID: "c-1", AuthorID: "user-1"}, "task_comment:c-1") + if err != nil { + t.Fatalf("handle: %v", err) + } + + statuses, err := runsRepo.GetRunsByCommentIDs(context.Background(), []string{"c-1"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + status, ok := statuses["c-1"] + if !ok { + t.Fatalf("missing comment run for c-1: %+v", statuses) + } + got, err := runsRepo.GetRunByID(context.Background(), status.RunID) + if err != nil { + t.Fatalf("get run: %v", err) + } + if got.AgentProfileID != "agent-primary" { + t.Errorf("agent_profile_id = %q, want agent-primary", got.AgentProfileID) + } + if got.Reason != "task_comment" { + t.Errorf("reason = %q, want task_comment", got.Reason) + } + if got.IdempotencyKey == nil || + !strings.HasPrefix(*got.IdempotencyKey, "task_comment:c-1:work:task-1:agent-primary:") { + t.Errorf("idempotency_key = %v, want salted task_comment key", got.IdempotencyKey) + } + var payload map[string]any + if err := json.Unmarshal([]byte(got.Payload), &payload); err != nil { + t.Fatalf("decode payload: %v", err) + } + for k, want := range map[string]string{ + "agent_profile_id": "agent-primary", + "task_id": "task-1", + "workflow_step_id": "work", + "comment_id": "c-1", + "author_id": "user-1", + } { + if got, _ := payload[k].(string); got != want { + t.Errorf("payload[%s] = %q, want %q (payload=%v)", k, got, want, payload) + } + } +} + +func TestDispatcher_CompletedSessionCommentCanApplyTransition(t *testing.T) { + store := &transitionWorkflowStore{} + eng := engine.New(store, engine.MapRegistry{}) + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestSession: &taskmodels.TaskSession{ + ID: "sess-completed", + State: taskmodels.TaskSessionStateCompleted, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{CommentID: "c-1"}, "task_comment:c-1") + if err != nil { + t.Fatalf("handle: %v", err) + } + if store.appliedTaskID != "task-1" { + t.Fatalf("transition task_id = %q, want task-1", store.appliedTaskID) + } + if store.appliedSessionID != "sess-completed" { + t.Fatalf("transition session_id = %q, want sess-completed", store.appliedSessionID) + } + if store.appliedFrom != "review" || store.appliedTo != "follow-up" { + t.Fatalf("transition = %q -> %q, want review -> follow-up", + store.appliedFrom, store.appliedTo) + } + if store.appliedTrigger != engine.TriggerOnComment { + t.Fatalf("transition trigger = %q, want on_comment", store.appliedTrigger) + } +} + +func TestDispatcher_DoesNotUseLatestSessionForNonCommentTriggers(t *testing.T) { + eng := &fakeEngine{} + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestSession: &taskmodels.TaskSession{ + ID: "sess-completed", + State: taskmodels.TaskSessionStateCompleted, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnBlockerResolved, + engine.OnBlockerResolvedPayload{ResolvedBlockerIDs: []string{"blocker-1"}}, "blocker:1") + if !errors.Is(err, ErrNoSession) { + t.Fatalf("err = %v, want ErrNoSession", err) + } + if eng.called { + t.Fatal("engine should not be invoked for non-comment triggers without an active session") + } +} + func TestDispatcher_ReturnsErrNoSessionWhenSessionMissing(t *testing.T) { eng := &fakeEngine{} sessions := &fakeSessions{} // session nil @@ -70,21 +433,81 @@ func TestDispatcher_ReturnsErrNoSessionWhenSessionMissing(t *testing.T) { } } -func TestDispatcher_ReturnsErrNoSessionOnSessionLookupError(t *testing.T) { +func TestDispatcher_PropagatesActiveSessionLookupError(t *testing.T) { eng := &fakeEngine{} - sessions := &fakeSessions{err: errors.New("db down")} + dbErr := errors.New("db down") + sessions := &fakeSessions{ + activeErr: dbErr, + latestSession: &taskmodels.TaskSession{ + ID: "sess-completed", + State: taskmodels.TaskSessionStateCompleted, + }, + } d := New(eng, sessions, logger.Default()) err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, engine.OnCommentPayload{}, "op") - if !errors.Is(err, ErrNoSession) { - t.Fatalf("err = %v, want ErrNoSession", err) + if !errors.Is(err, dbErr) { + t.Fatalf("err = %v, want wrapped db error", err) + } + if errors.Is(err, ErrNoSession) { + t.Fatalf("err must not masquerade as ErrNoSession: %v", err) + } + if eng.called { + t.Error("engine should not be called when active session lookup fails") + } +} + +func TestDispatcher_PropagatesActiveSessionLookupErrorForNonCommentTrigger(t *testing.T) { + eng := &fakeEngine{} + dbErr := errors.New("db down") + sessions := &fakeSessions{ + activeErr: dbErr, + latestSession: &taskmodels.TaskSession{ + ID: "sess-completed", + State: taskmodels.TaskSessionStateCompleted, + }, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnBlockerResolved, + engine.OnBlockerResolvedPayload{ResolvedBlockerIDs: []string{"blocker-1"}}, "op") + if !errors.Is(err, dbErr) { + t.Fatalf("err = %v, want wrapped db error", err) + } + if errors.Is(err, ErrNoSession) { + t.Fatalf("err must not masquerade as ErrNoSession: %v", err) + } + if eng.called { + t.Error("engine should not be called when active session lookup fails") + } +} + +func TestDispatcher_PropagatesLatestSessionLookupError(t *testing.T) { + eng := &fakeEngine{} + dbErr := errors.New("db down") + sessions := &fakeSessions{ + activeErr: taskmodels.ErrTaskSessionNotFound, + latestErr: dbErr, + } + d := New(eng, sessions, logger.Default()) + + err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, + engine.OnCommentPayload{}, "op") + if !errors.Is(err, dbErr) { + t.Fatalf("err = %v, want wrapped db error", err) + } + if errors.Is(err, ErrNoSession) { + t.Fatalf("err must not masquerade as ErrNoSession: %v", err) + } + if eng.called { + t.Error("engine should not be called when latest session lookup fails") } } func TestDispatcher_PropagatesEngineError(t *testing.T) { eng := &fakeEngine{err: errors.New("boom")} - sessions := &fakeSessions{session: &taskmodels.TaskSession{ID: "sess-1"}} + sessions := &fakeSessions{activeSession: &taskmodels.TaskSession{ID: "sess-1"}} d := New(eng, sessions, logger.Default()) err := d.HandleTrigger(context.Background(), "task-1", engine.TriggerOnComment, diff --git a/apps/backend/internal/office/repository/sqlite/base.go b/apps/backend/internal/office/repository/sqlite/base.go index a62e79c6be..7a8955047b 100644 --- a/apps/backend/internal/office/repository/sqlite/base.go +++ b/apps/backend/internal/office/repository/sqlite/base.go @@ -11,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/db/dialect" runssqlite "github.com/kandev/kandev/internal/runs/repository/sqlite" ) @@ -22,7 +23,8 @@ func newParticipantUUID() string { return uuid.New().String() } // RunnerProjection returns the correlated subquery that resolves the // effective per-task runner from workflow_step_participants, falling -// back to the step's primary agent_profile_id (ADR 0005 Wave F). +// back to the step's primary agent_profile_id and then the task's most +// recently assigned runner (ADR 0005 Wave F). // Inlined into SELECT clauses where the legacy // tasks.assignee_agent_profile_id column would have been read. // @@ -34,12 +36,16 @@ func RunnerProjection(alias string) string { alias = "tasks" } return `COALESCE( - (SELECT wsp.agent_profile_id FROM workflow_step_participants wsp + NULLIF((SELECT wsp.agent_profile_id FROM workflow_step_participants wsp WHERE wsp.step_id = ` + alias + `.workflow_step_id AND wsp.task_id = ` + alias + `.id AND wsp.role = 'runner' - ORDER BY wsp.position ASC, wsp.id ASC LIMIT 1), - (SELECT ws.agent_profile_id FROM workflow_steps ws WHERE ws.id = ` + alias + `.workflow_step_id), + ORDER BY wsp.position ASC, wsp.id ASC LIMIT 1), ''), + NULLIF((SELECT ws.agent_profile_id FROM workflow_steps ws WHERE ws.id = ` + alias + `.workflow_step_id), ''), + NULLIF((SELECT wsp.agent_profile_id FROM workflow_step_participants wsp + WHERE wsp.task_id = ` + alias + `.id + AND wsp.role = 'runner' + ORDER BY wsp.rowid DESC LIMIT 1), ''), '' )` } @@ -346,9 +352,20 @@ func (r *Repository) createRunTables() error { PRIMARY KEY (run_id, skill_id) ); `) + if err != nil { + return err + } + _, err = r.db.Exec(runPayloadCommentIDIndexSQL(r.db.DriverName())) return err } +func runPayloadCommentIDIndexSQL(driver string) string { + return fmt.Sprintf( + `CREATE INDEX IF NOT EXISTS idx_run_payload_comment_id ON runs((%s))`, + dialect.JSONExtract(driver, "payload", "comment_id"), + ) +} + func (r *Repository) createRoutineTables() error { _, err := r.db.Exec(` CREATE TABLE IF NOT EXISTS office_routines ( diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations.go b/apps/backend/internal/office/repository/sqlite/base_migrations.go index 157f631847..deb407de7a 100644 --- a/apps/backend/internal/office/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/office/repository/sqlite/base_migrations.go @@ -21,6 +21,7 @@ import ( func (r *Repository) runMigrations() { r.migrateSchedulerColumns() r.migrateFailureColumns() + r.migrateRunPayloadIndexes() if err := r.migrateTaskPriorityToText(); err != nil { // Surface to stderr; this stage runs from initSchema which doesn't // have a logger handle. The recreate is wrapped in a transaction @@ -39,6 +40,13 @@ func (r *Repository) runMigrations() { r.migrateProviderRouting() } +func (r *Repository) migrateRunPayloadIndexes() { + r.migrate.Apply( + "runs.idx_run_payload_comment_id", + runPayloadCommentIDIndexSQL(r.db.DriverName()), + ) +} + // migrateProviderRouting creates the office_workspace_routing, // office_run_route_attempts, and office_provider_health tables. The // routing columns on the runs queue (logical_provider_order, diff --git a/apps/backend/internal/office/repository/sqlite/base_test.go b/apps/backend/internal/office/repository/sqlite/base_test.go index 9b4a16f201..a91c2e5b6a 100644 --- a/apps/backend/internal/office/repository/sqlite/base_test.go +++ b/apps/backend/internal/office/repository/sqlite/base_test.go @@ -81,6 +81,17 @@ func TestInitSchema_AllTablesExist(t *testing.T) { t.Errorf("table %s not found (count=%d)", table, count) } } + + var indexCount int + err = db.QueryRow( + `SELECT COUNT(*) FROM sqlite_master WHERE type='index' AND name='idx_run_payload_comment_id'`, + ).Scan(&indexCount) + if err != nil { + t.Fatalf("query idx_run_payload_comment_id: %v", err) + } + if indexCount != 1 { + t.Fatalf("idx_run_payload_comment_id not found (count=%d)", indexCount) + } } func TestInitSchema_Idempotent(t *testing.T) { diff --git a/apps/backend/internal/office/repository/sqlite/runs_test.go b/apps/backend/internal/office/repository/sqlite/runs_test.go index 1328571b82..d7af7cae60 100644 --- a/apps/backend/internal/office/repository/sqlite/runs_test.go +++ b/apps/backend/internal/office/repository/sqlite/runs_test.go @@ -512,6 +512,52 @@ func TestGetRunsByCommentIDs_MapsLatestRunPerComment(t *testing.T) { } } +func TestGetRunsByCommentIDs_MapsSaltedCommentKeysByPayload(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + run := &models.Run{ + AgentProfileID: "a1", + Reason: "task_comment", + Payload: `{"task_id":"target-task","source_task_id":"source-task","comment_id":"cm-X"}`, + Status: "queued", + CoalescedCount: 1, + IdempotencyKey: strPtr("task_comment:cm-X:work:target-task:a1:abcd1234"), + } + if err := repo.CreateRun(ctx, run); err != nil { + t.Fatalf("create run: %v", err) + } + nilKeyRun := &models.Run{ + AgentProfileID: "a1", + Reason: "task_comment", + Payload: `{"task_id":"target-task","comment_id":"cm-nil-key"}`, + Status: "queued", + CoalescedCount: 1, + } + if err := repo.CreateRun(ctx, nilKeyRun); err != nil { + t.Fatalf("create nil-key run: %v", err) + } + + got, err := repo.GetRunsByCommentIDs(ctx, []string{"cm-X", "cm-nil-key"}) + if err != nil { + t.Fatalf("get: %v", err) + } + status, ok := got["cm-X"] + if !ok { + t.Fatalf("missing cm-X status: %+v", got) + } + if status.RunID != run.ID || status.Status != "queued" { + t.Fatalf("cm-X = %+v, want run %s queued", status, run.ID) + } + nilKeyStatus, ok := got["cm-nil-key"] + if !ok { + t.Fatalf("missing cm-nil-key status: %+v", got) + } + if nilKeyStatus.RunID != nilKeyRun.ID || nilKeyStatus.Status != "queued" { + t.Fatalf("cm-nil-key = %+v, want run %s queued", nilKeyStatus, nilKeyRun.ID) + } +} + // TestGetRunsByCommentIDs_EmptyInput_ReturnsEmptyMap pins the // degenerate case so callers can hand the result straight into a // range loop without nil-checking. diff --git a/apps/backend/internal/office/repository/sqlite/tasks_test.go b/apps/backend/internal/office/repository/sqlite/tasks_test.go index d36708e6e6..74979d8193 100644 --- a/apps/backend/internal/office/repository/sqlite/tasks_test.go +++ b/apps/backend/internal/office/repository/sqlite/tasks_test.go @@ -90,6 +90,41 @@ func insertTask(t *testing.T, repo *sqlite.Repository, ctx context.Context, id, } } +func TestGetTaskExecutionFields_FallsBackToLatestTaskRunner(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + if _, err := repo.ExecRaw(ctx, ` + INSERT INTO workflow_steps (id, agent_profile_id) + VALUES ('step-work', ''), ('step-review', ''), ('step-done', '') + `); err != nil { + t.Fatalf("insert steps: %v", err) + } + if _, err := repo.ExecRaw(ctx, ` + INSERT INTO tasks (id, workspace_id, workflow_step_id, state, title, created_at, updated_at) + VALUES ('task-done', 'ws-1', 'step-done', 'DONE', 'Done task', datetime('now'), datetime('now')) + `); err != nil { + t.Fatalf("insert task: %v", err) + } + if _, err := repo.ExecRaw(ctx, ` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position) + VALUES + ('p-work', 'step-work', 'task-done', 'runner', 'runner-on-work', 0, 0), + ('p-review', 'step-review', 'task-done', 'runner', 'runner-on-review', 0, 0) + `); err != nil { + t.Fatalf("insert runners: %v", err) + } + + fields, err := repo.GetTaskExecutionFields(ctx, "task-done") + if err != nil { + t.Fatalf("execution fields: %v", err) + } + if fields.AssigneeAgentProfileID != "runner-on-review" { + t.Fatalf("expected runner-on-review, got %q", fields.AssigneeAgentProfileID) + } +} + func TestSearchTasks_MatchesTitle(t *testing.T) { repo := newSearchTestRepo(t) ctx := context.Background() diff --git a/apps/backend/internal/office/scheduler/dashboard_adapter.go b/apps/backend/internal/office/scheduler/dashboard_adapter.go index 1795fec607..82187d003a 100644 --- a/apps/backend/internal/office/scheduler/dashboard_adapter.go +++ b/apps/backend/internal/office/scheduler/dashboard_adapter.go @@ -84,10 +84,11 @@ func convertChangeToMutation(c dashboard.TaskReactivityChange) TaskMutation { } if c.Comment != nil { out.Comment = &MutationComment{ - ID: c.Comment.ID, - Body: c.Comment.Body, - AuthorType: c.Comment.AuthorType, - AuthorID: c.Comment.AuthorID, + ID: c.Comment.ID, + Body: c.Comment.Body, + AuthorType: c.Comment.AuthorType, + AuthorID: c.Comment.AuthorID, + SkipAssigneeWake: c.SkipAssigneeCommentWake, } } return out diff --git a/apps/backend/internal/office/scheduler/dashboard_adapter_test.go b/apps/backend/internal/office/scheduler/dashboard_adapter_test.go new file mode 100644 index 0000000000..c6ee1f7b0c --- /dev/null +++ b/apps/backend/internal/office/scheduler/dashboard_adapter_test.go @@ -0,0 +1,26 @@ +package scheduler + +import ( + "testing" + + "github.com/kandev/kandev/internal/office/dashboard" +) + +func TestConvertChangeToMutation_PropagatesSkipAssigneeCommentWake(t *testing.T) { + got := convertChangeToMutation(dashboard.TaskReactivityChange{ + Comment: &dashboard.TaskReactivityComment{ + ID: "comment-1", + Body: "@Reviewer please look", + AuthorType: "user", + AuthorID: "user-1", + }, + SkipAssigneeCommentWake: true, + }) + + if got.Comment == nil { + t.Fatal("Comment = nil, want converted comment") + } + if !got.Comment.SkipAssigneeWake { + t.Fatal("SkipAssigneeWake = false, want true") + } +} diff --git a/apps/backend/internal/office/scheduler/reactivity.go b/apps/backend/internal/office/scheduler/reactivity.go index 7f497c2f85..2f760a3231 100644 --- a/apps/backend/internal/office/scheduler/reactivity.go +++ b/apps/backend/internal/office/scheduler/reactivity.go @@ -58,6 +58,9 @@ type MutationComment struct { Body string AuthorType string // "user" | "agent" AuthorID string + // SkipAssigneeWake suppresses only the assignee task_comment wake. + // Mention fan-out still runs. + SkipAssigneeWake bool } // ApplyTaskMutationResult summarises the pipeline's side-effects. @@ -253,7 +256,7 @@ func (ss *SchedulerService) reactToComment( selfComment := comment.AuthorType == "agent" && comment.AuthorID == task.AssigneeAgentProfileID // Assignee wake — skip if self-comment or task is closed. - if !selfComment && !closed { + if !comment.SkipAssigneeWake && !selfComment && !closed { queue(task.AssigneeAgentProfileID, RunContext{ Reason: RunReasonTaskComment, TaskID: task.ID, diff --git a/apps/backend/internal/office/service/engine_dispatcher.go b/apps/backend/internal/office/service/engine_dispatcher.go index cf0570ca3c..45e76f50ea 100644 --- a/apps/backend/internal/office/service/engine_dispatcher.go +++ b/apps/backend/internal/office/service/engine_dispatcher.go @@ -1,45 +1,9 @@ package service import ( - "context" - "errors" - - "github.com/kandev/kandev/internal/workflow/engine" + "github.com/kandev/kandev/internal/office/shared" ) -// ErrEngineNoSession is the sentinel a WorkflowEngineDispatcher returns -// when a task has no active session and the engine cannot evaluate a -// trigger. After Phase 4 there is no legacy fallback — subscribers log -// and drop the trigger when the engine cannot evaluate it. -// -// Implementations (see internal/office/engine_dispatcher) MUST use this -// exact sentinel; subscribers compare via errors.Is. -var ErrEngineNoSession = errors.New("workflow engine: no active session for task") - -// WorkflowEngineDispatcher is the office service's contract with the -// workflow engine. The interface lives here (not in the engine package) -// because office is the only caller that needs to dispatch from typed -// trigger payloads — the engine itself accepts a generic HandleInput. -// -// Implementations are responsible for: -// -// 1. Resolving the task's active session id (if any) — without it the -// engine cannot LoadState. -// 2. Building engine.HandleInput from the typed payload. -// 3. Invoking engine.HandleTrigger and translating its result. -// -// The interface stays narrow to keep the office service test surface -// small — only the four event-subscriber triggers are exercised. -type WorkflowEngineDispatcher interface { - HandleTrigger( - ctx context.Context, - taskID string, - trigger engine.Trigger, - payload any, - operationID string, - ) error -} - // SetWorkflowEngineDispatcher wires a dispatcher onto the office service. // After Phase 4, when a dispatcher is set, the four event subscribers // (comment_created, blockers_resolved, children_completed, @@ -48,6 +12,6 @@ type WorkflowEngineDispatcher interface { // // Calling SetWorkflowEngineDispatcher with nil disables engine routing // (useful in tests that don't need a workflow engine wired). -func (s *Service) SetWorkflowEngineDispatcher(d WorkflowEngineDispatcher) { +func (s *Service) SetWorkflowEngineDispatcher(d shared.WorkflowEngineDispatcher) { s.engineDispatcher = d } diff --git a/apps/backend/internal/office/service/event_subscribers.go b/apps/backend/internal/office/service/event_subscribers.go index 03ee0d8650..903066c6fe 100644 --- a/apps/backend/internal/office/service/event_subscribers.go +++ b/apps/backend/internal/office/service/event_subscribers.go @@ -15,6 +15,8 @@ import ( "github.com/kandev/kandev/internal/office/costs" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/office/shared" + "github.com/kandev/kandev/internal/runs/commentkeys" "github.com/kandev/kandev/internal/workflow/engine" ) @@ -36,7 +38,7 @@ func (s *Service) dispatchEngineTrigger( return nil } if err := s.engineDispatcher.HandleTrigger(ctx, taskID, trigger, payload, opID); err != nil { - if errors.Is(err, ErrEngineNoSession) { + if errors.Is(err, shared.ErrEngineNoSession) { s.logger.Debug("engine trigger skipped: no active session", zap.String("task_id", taskID), zap.String("trigger", string(trigger))) @@ -75,6 +77,7 @@ type CommentPostedData struct { AuthorID string `json:"author_id"` AuthorType string `json:"author_type"` AssigneeAgentProfileID string `json:"assignee_agent_profile_id"` + EngineDispatched string `json:"engine_dispatched"` } // ApprovalResolvedData represents an approval resolved event payload. @@ -860,6 +863,9 @@ func (s *Service) queueCommentRun(ctx context.Context, data CommentPostedData) e if data.TaskID == "" || data.CommentID == "" { return nil } + if data.EngineDispatched == commentkeys.EngineDispatchedValue { + return nil + } // Self-comment short-circuit: if the agent that wrote the comment is // also the task's primary participant we don't want to queue a run. // The engine's queue_run resolver enforces the same rule, but @@ -870,7 +876,7 @@ func (s *Service) queueCommentRun(ctx context.Context, data CommentPostedData) e data.AuthorType == participantTypeAgent && fields.AssigneeAgentProfileID == data.AuthorID { return nil } - key := fmt.Sprintf("task_comment:%s", data.CommentID) + key := commentkeys.TaskComment(data.CommentID) return s.dispatchEngineTrigger(ctx, data.TaskID, engine.TriggerOnComment, engine.OnCommentPayload{ CommentID: data.CommentID, diff --git a/apps/backend/internal/office/service/event_subscribers_engine_test.go b/apps/backend/internal/office/service/event_subscribers_engine_test.go index 1822d579dc..fab622ccd3 100644 --- a/apps/backend/internal/office/service/event_subscribers_engine_test.go +++ b/apps/backend/internal/office/service/event_subscribers_engine_test.go @@ -5,8 +5,11 @@ import ( "sync" "testing" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" - "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" + "github.com/kandev/kandev/internal/runs/commentkeys" "github.com/kandev/kandev/internal/workflow/engine" ) @@ -119,13 +122,112 @@ func TestEngineDispatcher_RoutesToDispatcher(t *testing.T) { } } +func TestEngineDispatcher_SkipsAlreadyDispatchedCommentEvent(t *testing.T) { + svc, eb := newTestServiceWithBus(t) + + disp := &fakeDispatcher{} + svc.SetWorkflowEngineDispatcher(disp) + + ctx := context.Background() + createTestAgent(t, svc, "ws-1", "agent-1") + insertTestTask(t, svc, "task-1", "ws-1") + setTestTaskAssignee(t, svc, "task-1", "agent-1") + + event := bus.NewEvent(events.OfficeCommentCreated, "test", map[string]string{ + "task_id": "task-1", + "comment_id": "comment-1", + "author_type": "user", + "author_id": "user-x", + "engine_dispatched": commentkeys.EngineDispatchedValue, + }) + if err := eb.Publish(ctx, events.OfficeCommentCreated, event); err != nil { + t.Fatalf("publish comment event: %v", err) + } + if calls := disp.Calls(); len(calls) != 0 { + t.Fatalf("dispatcher calls = %d, want 0", len(calls)) + } +} + +func TestEngineDispatcher_SkipsDoneStepSelfComment(t *testing.T) { + svc, _ := newTestServiceWithBus(t) + + disp := &fakeDispatcher{} + svc.SetWorkflowEngineDispatcher(disp) + + ctx := context.Background() + createTestAgent(t, svc, "ws-1", "runner-on-review") + insertTestTask(t, svc, "task-done", "ws-1") + svc.ExecSQL(t, ` + INSERT INTO workflow_steps (id, agent_profile_id) + VALUES ('step-work', ''), ('step-review', ''), ('step-done', '') + `) + svc.ExecSQL(t, `UPDATE tasks SET workflow_step_id = 'step-done' WHERE id = 'task-done'`) + svc.ExecSQL(t, ` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position) + VALUES + ('p-work', 'step-work', 'task-done', 'runner', 'runner-on-work', 0, 0), + ('p-review', 'step-review', 'task-done', 'runner', 'runner-on-review', 0, 0) + `) + + comment := &models.TaskComment{ + TaskID: "task-done", + AuthorType: "agent", + AuthorID: "runner-on-review", + Body: "Done", + } + if err := svc.CreateComment(ctx, comment); err != nil { + t.Fatalf("create comment: %v", err) + } + if calls := disp.Calls(); len(calls) != 0 { + t.Fatalf("dispatcher calls = %d, want 0", len(calls)) + } +} + +func TestEngineDispatcher_DispatchesOlderDoneStepRunnerComment(t *testing.T) { + svc, _ := newTestServiceWithBus(t) + + disp := &fakeDispatcher{} + svc.SetWorkflowEngineDispatcher(disp) + + ctx := context.Background() + createTestAgent(t, svc, "ws-1", "runner-on-work") + createTestAgent(t, svc, "ws-1", "runner-on-review") + insertTestTask(t, svc, "task-done", "ws-1") + svc.ExecSQL(t, ` + INSERT INTO workflow_steps (id, agent_profile_id) + VALUES ('step-work', ''), ('step-review', ''), ('step-done', '') + `) + svc.ExecSQL(t, `UPDATE tasks SET workflow_step_id = 'step-done' WHERE id = 'task-done'`) + svc.ExecSQL(t, ` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position) + VALUES + ('p-work', 'step-work', 'task-done', 'runner', 'runner-on-work', 0, 0), + ('p-review', 'step-review', 'task-done', 'runner', 'runner-on-review', 0, 0) + `) + + comment := &models.TaskComment{ + TaskID: "task-done", + AuthorType: "agent", + AuthorID: "runner-on-work", + Body: "Older runner reply", + } + if err := svc.CreateComment(ctx, comment); err != nil { + t.Fatalf("create comment: %v", err) + } + if calls := disp.Calls(); len(calls) != 1 { + t.Fatalf("dispatcher calls = %d, want 1", len(calls)) + } +} + // TestEngineDispatcher_NoSession_DropsTrigger pins that when the // dispatcher returns ErrEngineNoSession the subscriber drops the // trigger silently — there is no legacy fallback after Phase 4. func TestEngineDispatcher_NoSession_DropsTrigger(t *testing.T) { svc, _ := newTestServiceWithBus(t) - disp := &fakeDispatcher{nextErr: service.ErrEngineNoSession} + disp := &fakeDispatcher{nextErr: shared.ErrEngineNoSession} svc.SetWorkflowEngineDispatcher(disp) ctx := context.Background() diff --git a/apps/backend/internal/office/service/event_subscribers_test.go b/apps/backend/internal/office/service/event_subscribers_test.go index d174a47761..502888542e 100644 --- a/apps/backend/internal/office/service/event_subscribers_test.go +++ b/apps/backend/internal/office/service/event_subscribers_test.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" "github.com/kandev/kandev/internal/workflow/engine" ) @@ -31,7 +32,7 @@ func (d *queueRunDispatcher) HandleTrigger( ) error { assignee, err := d.svc.GetTaskAssigneeForTest(ctx, taskID) if err != nil || assignee == "" { - return service.ErrEngineNoSession + return shared.ErrEngineNoSession } switch trigger { case engine.TriggerOnComment: diff --git a/apps/backend/internal/office/service/run.go b/apps/backend/internal/office/service/run.go index 74ca2f05f0..0580322e95 100644 --- a/apps/backend/internal/office/service/run.go +++ b/apps/backend/internal/office/service/run.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/runs/commentkeys" runsservice "github.com/kandev/kandev/internal/runs/service" ) @@ -148,13 +149,13 @@ func (s *Service) publishRunQueued(ctx context.Context, req *models.Run, idempot if s.eb == nil { return } - parsed := ParseRunPayload(req.Payload) + taskID, commentID := commentkeys.IdentityFromPayload(req.Payload) data := map[string]interface{}{ "run_id": req.ID, "agent_profile_id": req.AgentProfileID, "reason": req.Reason, - "task_id": parsed["task_id"], - "comment_id": parsed["comment_id"], + "task_id": taskID, + "comment_id": commentID, "idempotency_key": idempotencyKey, } event := bus.NewEvent(events.OfficeRunQueued, "office-service", data) diff --git a/apps/backend/internal/office/service/run_lifecycle_events_test.go b/apps/backend/internal/office/service/run_lifecycle_events_test.go index 05ca61213c..46b81a9fae 100644 --- a/apps/backend/internal/office/service/run_lifecycle_events_test.go +++ b/apps/backend/internal/office/service/run_lifecycle_events_test.go @@ -342,6 +342,58 @@ func TestFinishRun_PublishesOfficeRunProcessed(t *testing.T) { } } +func TestFinishRun_PublishesOfficeRunProcessedForSourceCommentTask(t *testing.T) { + svc, eb := newTestServiceWithBus(t) + ctx := context.Background() + + createTestAgent(t, svc, "ws-1", "worker-1") + targetTaskID := createOfficeTask(t, svc, "ws-1", "worker-1") + sourceTaskID := "source-task" + insertTestTask(t, svc, sourceTaskID, "ws-1") + + if err := svc.QueueRun( + ctx, "worker-1", service.RunReasonTaskComment, + `{"task_id":"`+targetTaskID+`","source_task_id":"`+sourceTaskID+`","comment_id":"cm-source"}`, + "task_comment:cm-source:target", + ); err != nil { + t.Fatalf("queue: %v", err) + } + run, err := svc.ClaimNextRun(ctx) + if err != nil || run == nil { + t.Fatalf("claim: %v (run=%v)", err, run) + } + + got := make(chan *bus.Event, 1) + sub, err := eb.Subscribe(events.OfficeRunProcessed, func(_ context.Context, e *bus.Event) error { + got <- e + return nil + }) + if err != nil { + t.Fatalf("subscribe: %v", err) + } + defer func() { _ = sub.Unsubscribe() }() + + if err := svc.FinishRun(ctx, run.ID); err != nil { + t.Fatalf("finish: %v", err) + } + + select { + case e := <-got: + data, ok := e.Data.(map[string]interface{}) + if !ok { + t.Fatalf("event data not a map: %T", e.Data) + } + if data["task_id"] != sourceTaskID { + t.Errorf("task_id = %v, want source task %s", data["task_id"], sourceTaskID) + } + if data["comment_id"] != "cm-source" { + t.Errorf("comment_id = %v, want cm-source", data["comment_id"]) + } + default: + t.Fatalf("expected OfficeRunProcessed event; got none") + } +} + // TestFailRun_PublishesOfficeRunProcessedFailed pins the failed-run // path: FailRun publishes status="failed" so the comment badge can // flip to its error state. diff --git a/apps/backend/internal/office/service/scheduler_runs.go b/apps/backend/internal/office/service/scheduler_runs.go index 03be32371b..3222fe7f45 100644 --- a/apps/backend/internal/office/service/scheduler_runs.go +++ b/apps/backend/internal/office/service/scheduler_runs.go @@ -10,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/runs/commentkeys" ) // ClaimNextRun atomically claims the next eligible run from the queue. @@ -77,11 +78,11 @@ func (s *Service) publishRunProcessed( "status": status, } if run != nil { - parsed := ParseRunPayload(run.Payload) + taskID, commentID := commentkeys.IdentityFromPayload(run.Payload) data["agent_profile_id"] = run.AgentProfileID data["reason"] = run.Reason - data["task_id"] = parsed["task_id"] - data["comment_id"] = parsed["comment_id"] + data["task_id"] = taskID + data["comment_id"] = commentID if run.ErrorMessage != "" { data["error_message"] = run.ErrorMessage } diff --git a/apps/backend/internal/office/service/service.go b/apps/backend/internal/office/service/service.go index dafc559408..a7de9e185e 100644 --- a/apps/backend/internal/office/service/service.go +++ b/apps/backend/internal/office/service/service.go @@ -248,7 +248,7 @@ type Service struct { // engine. There is no legacy fallback path — if the engine cannot // evaluate the trigger (no session yet) the trigger is dropped with // a debug log. - engineDispatcher WorkflowEngineDispatcher + engineDispatcher shared.WorkflowEngineDispatcher // pricingLookup resolves per-model pricing for the cost subscriber's // Layer B fallback (models.dev). Optional — nil means Layer B always diff --git a/apps/backend/internal/office/shared/interfaces.go b/apps/backend/internal/office/shared/interfaces.go index 27878fb52c..6f550b9e61 100644 --- a/apps/backend/internal/office/shared/interfaces.go +++ b/apps/backend/internal/office/shared/interfaces.go @@ -2,11 +2,18 @@ package shared import ( "context" + "errors" "time" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/workflow/engine" ) +// ErrEngineNoSession is returned by WorkflowEngineDispatcher when a task has +// no active or reusable session, so the workflow engine cannot evaluate a +// trigger for it. +var ErrEngineNoSession = errors.New("workflow engine: no active session for task") + // AgentReader provides read access to agent instances. // Implemented by the agents feature (and transitionally by office/service.Service). type AgentReader interface { @@ -36,6 +43,19 @@ type RunQueuer interface { QueueRun(ctx context.Context, agentInstanceID, reason, payload, idempotencyKey string) error } +// WorkflowEngineDispatcher routes typed office task events through the +// workflow engine. Implementations resolve the task's session and translate +// typed trigger payloads into engine.HandleInput. +type WorkflowEngineDispatcher interface { + HandleTrigger( + ctx context.Context, + taskID string, + trigger engine.Trigger, + payload any, + operationID string, + ) error +} + // ActivityLogger logs activity entries across office features. // Implemented by the ActivityLoggerImpl in shared/activity.go. type ActivityLogger interface { diff --git a/apps/backend/internal/orchestrator/workflow_callbacks.go b/apps/backend/internal/orchestrator/workflow_callbacks.go index 32ea7426a1..cfc8780286 100644 --- a/apps/backend/internal/orchestrator/workflow_callbacks.go +++ b/apps/backend/internal/orchestrator/workflow_callbacks.go @@ -32,6 +32,7 @@ func buildWorkflowCallbacks(svc *Service) engine.MapRegistry { Participants: svc.engineParticipants, CEOResolver: svc.engineCEOResolver, Primary: svc.enginePrimary, + TaskSteps: workflowTargetStepResolver(svc.enginePrimary, svc.engineParticipants), } if svc.engineParticipants != nil { r[engine.ActionQueueRunForEachParticipant] = engine.QueueRunForEachParticipantCallback{ @@ -55,6 +56,22 @@ func buildWorkflowCallbacks(svc *Service) engine.MapRegistry { return r } +// workflowTargetStepResolver picks the primary adapter first because it is the +// queue_run primary resolver, then falls back to participants. Nil means +// cross-task queue_run targets will surface ErrActionNotYetWired. +func workflowTargetStepResolver( + primary engine.PrimaryAgentResolver, + participants engine.ParticipantStore, +) engine.TargetTaskStepResolver { + if resolver, ok := primary.(engine.TargetTaskStepResolver); ok { + return resolver + } + if resolver, ok := participants.(engine.TargetTaskStepResolver); ok { + return resolver + } + return nil +} + // switchWorkflowDispatcher returns the closure SwitchWorkflowCallback uses // to fire on_exit / on_enter. It reads svc.workflowEngine lazily — at // registration time the engine may not yet be initialised, but it is diff --git a/apps/backend/internal/runs/commentkeys/commentkeys.go b/apps/backend/internal/runs/commentkeys/commentkeys.go new file mode 100644 index 0000000000..246cada519 --- /dev/null +++ b/apps/backend/internal/runs/commentkeys/commentkeys.go @@ -0,0 +1,80 @@ +// Package commentkeys centralizes the idempotency-key contract that links +// office comments to queued runs. +package commentkeys + +import ( + "encoding/json" + "strings" +) + +const ( + // TaskCommentPrefix prefixes run idempotency keys that originate from an + // office task comment. + TaskCommentPrefix = "task_comment:" + // TaskCommentReason is the run reason used for comment-triggered runs. + TaskCommentReason = "task_comment" + // EngineDispatchedValue marks comment events whose synchronous publisher + // path already routed the trigger through the workflow engine. + EngineDispatchedValue = "true" +) + +// TaskComment builds the canonical same-task comment idempotency key. +func TaskComment(commentID string) string { + return TaskCommentPrefix + commentID +} + +// HasTaskCommentPrefix reports whether key uses the comment idempotency prefix. +func HasTaskCommentPrefix(key string) bool { + return strings.HasPrefix(key, TaskCommentPrefix) +} + +// TrimTaskCommentPrefix removes the comment idempotency prefix when present. +func TrimTaskCommentPrefix(key string) string { + return strings.TrimPrefix(key, TaskCommentPrefix) +} + +// CommentIDFromKey extracts the leading comment id from canonical and salted +// task_comment keys. Salted keys append extra colon-separated fields after the +// comment id. +func CommentIDFromKey(key string) string { + if !HasTaskCommentPrefix(key) { + return "" + } + id := TrimTaskCommentPrefix(key) + if before, _, found := strings.Cut(id, ":"); found { + return before + } + return id +} + +// IsSaltedTaskCommentKey reports whether a task_comment key carries extra +// salt after the leading comment id. +func IsSaltedTaskCommentKey(key string) bool { + if !HasTaskCommentPrefix(key) { + return false + } + return strings.Contains(TrimTaskCommentPrefix(key), ":") +} + +// IdentityFromPayload extracts the task/comment pair used to link a run back +// to a comment. Cross-task wakes execute on task_id but the UI badge/comment +// anchor belongs to source_task_id, so source_task_id wins when present. +func IdentityFromPayload(payloadJSON string) (taskID, commentID string) { + if payloadJSON == "" { + return "", "" + } + var p struct { + TaskID string `json:"task_id"` + SourceTaskID string `json:"source_task_id"` + CommentID string `json:"comment_id"` + } + if err := json.Unmarshal([]byte(payloadJSON), &p); err != nil { + return "", "" + } + if p.SourceTaskID != "" { + taskID = p.SourceTaskID + } else { + taskID = p.TaskID + } + return taskID, p.CommentID +} diff --git a/apps/backend/internal/runs/commentkeys/commentkeys_test.go b/apps/backend/internal/runs/commentkeys/commentkeys_test.go new file mode 100644 index 0000000000..4d40eda0b1 --- /dev/null +++ b/apps/backend/internal/runs/commentkeys/commentkeys_test.go @@ -0,0 +1,81 @@ +package commentkeys + +import "testing" + +func TestIdentityFromPayload(t *testing.T) { + tests := []struct { + name string + payload string + wantTask string + wantComment string + }{ + { + name: "task and comment", + payload: `{"task_id":"target-task","comment_id":"cm-1"}`, + wantTask: "target-task", + wantComment: "cm-1", + }, + { + name: "source task overrides task", + payload: `{"task_id":"target-task","source_task_id":"source-task","comment_id":"cm-1"}`, + wantTask: "source-task", + wantComment: "cm-1", + }, + { + name: "comment id only", + payload: `{"comment_id":"cm-1"}`, + wantComment: "cm-1", + }, + { + name: "source task without comment", + payload: `{"task_id":"target-task","source_task_id":"source-task"}`, + wantTask: "source-task", + }, + { + name: "malformed", + payload: `{`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotTask, gotComment := IdentityFromPayload(tt.payload) + if gotTask != tt.wantTask || gotComment != tt.wantComment { + t.Fatalf("IdentityFromPayload() = (%q, %q), want (%q, %q)", + gotTask, gotComment, tt.wantTask, tt.wantComment) + } + }) + } +} + +func TestCommentIDFromKey(t *testing.T) { + tests := []struct { + key string + want string + }{ + {key: "task_comment:cm-1", want: "cm-1"}, + {key: "task_comment:cm-1:step-1:task-1:agent-1:abcd", want: "cm-1"}, + {key: "other:cm-1", want: ""}, + } + for _, tt := range tests { + if got := CommentIDFromKey(tt.key); got != tt.want { + t.Fatalf("CommentIDFromKey(%q) = %q, want %q", tt.key, got, tt.want) + } + } +} + +func TestIsSaltedTaskCommentKey(t *testing.T) { + tests := []struct { + key string + want bool + }{ + {key: "task_comment:cm-1", want: false}, + {key: "task_comment:cm-1:step-1:task-1:agent-1:abcd", want: true}, + {key: "other:cm-1:step-1", want: false}, + } + for _, tt := range tests { + if got := IsSaltedTaskCommentKey(tt.key); got != tt.want { + t.Fatalf("IsSaltedTaskCommentKey(%q) = %v, want %v", tt.key, got, tt.want) + } + } +} diff --git a/apps/backend/internal/runs/repository/sqlite/runs.go b/apps/backend/internal/runs/repository/sqlite/runs.go index 4e3deb73d4..cf95aecda9 100644 --- a/apps/backend/internal/runs/repository/sqlite/runs.go +++ b/apps/backend/internal/runs/repository/sqlite/runs.go @@ -2,13 +2,17 @@ package sqlite import ( "context" + "database/sql" + "encoding/json" "fmt" "strings" "time" "github.com/google/uuid" + "github.com/kandev/kandev/internal/db/dialect" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/runs/commentkeys" ) // CreateRun creates a new run queue entry. @@ -183,8 +187,10 @@ type CommentRunStatus struct { ErrorMessage string } -// GetRunsByCommentIDs returns the latest run associated with each -// comment id by joining on idempotency_key = "task_comment:". +// GetRunsByCommentIDs returns the latest run associated with each comment id. +// The canonical same-task wake joins on idempotency_key = +// "task_comment:"; salted fan-out keys join through the persisted +// payload.comment_id so cross-task comment wakes still surface status. // Comments without a matching run are simply absent from the map. // When a comment has multiple matching rows (rare — would require the // idempotency window to lapse and the comment to re-trigger) the most @@ -196,20 +202,41 @@ func (r *Repository) GetRunsByCommentIDs( if len(commentIDs) == 0 { return out, nil } - keys := make([]string, len(commentIDs)) - args := make([]interface{}, len(commentIDs)) - placeholders := make([]string, len(commentIDs)) + args := make([]interface{}, 0) + keyPlaceholders := make([]string, len(commentIDs)) + commentPlaceholders := make([]string, len(commentIDs)) + wanted := make(map[string]struct{}, len(commentIDs)) for i, id := range commentIDs { - keys[i] = "task_comment:" + id - args[i] = keys[i] - placeholders[i] = "?" + args = append(args, commentkeys.TaskComment(id)) + keyPlaceholders[i] = "?" + wanted[id] = struct{}{} } + for _, id := range commentIDs { + args = append(args, commentkeys.TaskComment(id)) + } + args = append(args, commentkeys.TaskCommentPrefix+"%", commentkeys.TaskCommentReason) + for i, id := range commentIDs { + args = append(args, id) + commentPlaceholders[i] = "?" + } + commentIDExpr := dialect.JSONExtract(r.ro.DriverName(), "payload", "comment_id") query := fmt.Sprintf(` - SELECT id, idempotency_key, status, error_message, requested_at + SELECT id, idempotency_key, status, error_message, requested_at, payload FROM runs WHERE idempotency_key IN (%s) + UNION ALL + SELECT id, idempotency_key, status, error_message, requested_at, payload + FROM runs + WHERE (idempotency_key IS NULL OR idempotency_key NOT IN (%s)) + AND (idempotency_key LIKE ? OR reason = ?) + AND %s IN (%s) ORDER BY requested_at DESC - `, strings.Join(placeholders, ",")) + `, + strings.Join(keyPlaceholders, ","), + strings.Join(keyPlaceholders, ","), + commentIDExpr, + strings.Join(commentPlaceholders, ","), + ) rows, err := r.ro.QueryxContext(ctx, r.ro.Rebind(query), args...) if err != nil { return nil, err @@ -217,13 +244,17 @@ func (r *Repository) GetRunsByCommentIDs( defer func() { _ = rows.Close() }() for rows.Next() { var ( - id, idemKey, status, errMsg string + id, status, errMsg, payload string + idemKey sql.NullString requestedAt time.Time ) - if err := rows.Scan(&id, &idemKey, &status, &errMsg, &requestedAt); err != nil { + if err := rows.Scan(&id, &idemKey, &status, &errMsg, &requestedAt, &payload); err != nil { return nil, err } - commentID := strings.TrimPrefix(idemKey, "task_comment:") + commentID := commentIDFromRun(idemKey.String, payload, wanted) + if commentID == "" { + continue + } // First write wins — rows are ordered DESC so the first row per // comment id is the most recent. if _, ok := out[commentID]; ok { @@ -238,6 +269,37 @@ func (r *Repository) GetRunsByCommentIDs( return out, rows.Err() } +func commentIDFromRun(idempotencyKey, payload string, wanted map[string]struct{}) string { + payloadID := commentIDFromPayload(payload, wanted) + if commentkeys.IsSaltedTaskCommentKey(idempotencyKey) && payloadID != "" { + return payloadID + } + if id := commentkeys.CommentIDFromKey(idempotencyKey); id != "" { + if _, found := wanted[id]; found { + return id + } + } + return payloadID +} + +func commentIDFromPayload(payload string, wanted map[string]struct{}) string { + if payload == "" { + return "" + } + var raw map[string]any + if err := json.Unmarshal([]byte(payload), &raw); err != nil { + return "" + } + id, ok := raw["comment_id"].(string) + if !ok { + return "" + } + if _, found := wanted[id]; !found { + return "" + } + return id +} + // GetClaimedTasklessRunForAgent returns the most recently claimed // taskless run (payload.task_id is empty / missing) for the given // agent. Used by the AgentCompleted event subscriber to attribute a @@ -305,10 +367,11 @@ func (r *Repository) CoalesceRun( SELECT id FROM runs WHERE agent_profile_id = ? AND reason = ? AND status = 'queued' AND requested_at > ? + AND (idempotency_key IS NULL OR idempotency_key NOT LIKE ?) ORDER BY requested_at DESC LIMIT 1 ) - `), payload, agentInstanceID, reason, cutoff) + `), payload, agentInstanceID, reason, cutoff, commentkeys.TaskCommentPrefix+"%") if err != nil { return false, err } diff --git a/apps/backend/internal/runs/service/service.go b/apps/backend/internal/runs/service/service.go index 2445c95940..72f368d124 100644 --- a/apps/backend/internal/runs/service/service.go +++ b/apps/backend/internal/runs/service/service.go @@ -22,6 +22,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/runs/commentkeys" runssqlite "github.com/kandev/kandev/internal/runs/repository/sqlite" ) @@ -48,12 +49,8 @@ type RunQueueAdapter interface { // - IdempotencyKey: when non-empty, the run is suppressed if a row // with the same key landed in the last 24 hours. // - Payload: structured JSON-encoded payload. Must be a non-nil map -// when set; serialised before insert. -// -// Phase 3 keeps the agent_profile_id resolution shape compatible with -// today's office.QueueRun by carrying agent_profile_id inside Payload -// (key "agent_profile_id"). Once the engine emits queue_run with a -// real AgentProfileID, a separate resolver maps profile→instance. +// when set; serialised before insert. QueueRun adds the resolved +// task / workflow / agent envelope before persisting. type QueueRunRequest struct { AgentProfileID string TaskID string @@ -136,8 +133,8 @@ func New( func (s *Service) SubscribeSignal() <-chan struct{} { return s.signalCh } // QueueRun implements RunQueueAdapter. The flow is: -// 1. Resolve agent_profile_id (today: from payload; future: from agent -// profile + task). +// 1. Resolve agent_profile_id (from the request field, payload fallback, +// or a wired resolver). // 2. Idempotency check (24h) on req.IdempotencyKey if set. // 3. Coalescing (5s window for same agent + reason). // 4. Insert into runs table. @@ -168,23 +165,26 @@ func (s *Service) QueueRun(ctx context.Context, req QueueRunRequest) error { } } - payload, err := encodePayload(req.Payload) + payloadMap := runPayload(req, agentInstanceID) + payload, err := encodePayload(payloadMap) if err != nil { return fmt.Errorf("encode payload: %w", err) } - coalesced, err := s.repo.CoalesceRun(ctx, agentInstanceID, req.Reason, CoalesceWindowSeconds, payload) - if err != nil { - return fmt.Errorf("coalesce check: %w", err) - } - if coalesced { - s.log.Debug("run coalesced", - zap.String("agent", agentInstanceID), - zap.String("reason", req.Reason)) - // Coalesced rows are merged into an existing queued row, so - // no new signal is needed — the scheduler already saw the - // original insert. - return nil + if shouldCoalesceRun(req) { + coalesced, err := s.repo.CoalesceRun(ctx, agentInstanceID, req.Reason, CoalesceWindowSeconds, payload) + if err != nil { + return fmt.Errorf("coalesce check: %w", err) + } + if coalesced { + s.log.Debug("run coalesced", + zap.String("agent", agentInstanceID), + zap.String("reason", req.Reason)) + // Coalesced rows are merged into an existing queued row, so + // no new signal is needed — the scheduler already saw the + // original insert. + return nil + } } row, err := s.insertRun(ctx, agentInstanceID, req, payload) @@ -229,15 +229,16 @@ func (s *Service) insertRun( } // resolveAgentInstance picks the agent_profile_id for a request. -// Phase 3 keeps the legacy "payload carries agent_profile_id" path: -// office wakers stuff it in the payload before calling the runs -// service. The engine's queue_run path (Phase 3.2) will pass an -// agent profile id and rely on the resolver to look up the right -// instance — until then the resolver is optional. +// Engine queue_run sends AgentProfileID as a typed field. Legacy +// office QueueRun callers still carry agent_profile_id inside Payload, +// so the resolver-less path accepts both shapes. func (s *Service) resolveAgentInstance(ctx context.Context, req QueueRunRequest) (string, error) { if s.resolver != nil { return s.resolver.ResolveAgentInstance(ctx, req) } + if req.AgentProfileID != "" { + return req.AgentProfileID, nil + } if req.Payload != nil { if v, ok := req.Payload["agent_profile_id"].(string); ok && v != "" { return v, nil @@ -246,6 +247,31 @@ func (s *Service) resolveAgentInstance(ctx context.Context, req QueueRunRequest) return "", nil } +// runPayload starts from the caller-supplied payload, then overwrites the +// standard envelope keys from typed request fields. That overwrite is +// intentional: engine queue_run and legacy office QueueRun callers converge +// on the same persisted JSON shape even when their input payloads differ. +func runPayload(req QueueRunRequest, agentInstanceID string) map[string]any { + out := make(map[string]any, len(req.Payload)) + for k, v := range req.Payload { + out[k] = v + } + if req.TaskID != "" { + out["task_id"] = req.TaskID + } + if req.WorkflowStepID != "" { + out["workflow_step_id"] = req.WorkflowStepID + } + if agentInstanceID != "" { + out["agent_profile_id"] = agentInstanceID + } + return out +} + +func shouldCoalesceRun(req QueueRunRequest) bool { + return !commentkeys.HasTaskCommentPrefix(req.IdempotencyKey) +} + // publishRunQueued emits the OfficeRunQueued bus event so the WS // gateway can fan it out to subscribed clients. Subject keeps the // "office.run.queued" string for frontend stability — the schema @@ -254,7 +280,7 @@ func (s *Service) publishRunQueued(ctx context.Context, row *models.Run, idempot if s.eb == nil { return } - taskID, commentID := extractIdentityFromPayload(row.Payload) + taskID, commentID := commentkeys.IdentityFromPayload(row.Payload) data := map[string]interface{}{ "run_id": row.ID, "agent_profile_id": row.AgentProfileID, @@ -294,23 +320,3 @@ func encodePayload(p map[string]any) (string, error) { } return string(b), nil } - -// extractIdentityFromPayload pulls task_id and comment_id out of a -// JSON payload string. Used by publishRunQueued so the WS gateway can -// route per-task without re-parsing on the client. -func extractIdentityFromPayload(payload string) (taskID, commentID string) { - if payload == "" { - return "", "" - } - var raw map[string]any - if err := json.Unmarshal([]byte(payload), &raw); err != nil { - return "", "" - } - if v, ok := raw["task_id"].(string); ok { - taskID = v - } - if v, ok := raw["comment_id"].(string); ok { - commentID = v - } - return taskID, commentID -} diff --git a/apps/backend/internal/runs/service/service_test.go b/apps/backend/internal/runs/service/service_test.go index 235b79595c..44633b4bb1 100644 --- a/apps/backend/internal/runs/service/service_test.go +++ b/apps/backend/internal/runs/service/service_test.go @@ -2,6 +2,7 @@ package service_test import ( "context" + "encoding/json" "testing" "time" @@ -11,7 +12,9 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/office/models" officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" + runssqlite "github.com/kandev/kandev/internal/runs/repository/sqlite" runsservice "github.com/kandev/kandev/internal/runs/service" ) @@ -21,6 +24,14 @@ import ( // office repo is created here, not the runs repo directly, so the // schema migrations run. func newTestService(t *testing.T) (*runsservice.Service, bus.EventBus) { + t.Helper() + svc, eb, _ := newTestServiceWithRepo(t) + return svc, eb +} + +func newTestServiceWithRepo(t *testing.T) ( + *runsservice.Service, bus.EventBus, *runssqlite.Repository, +) { t.Helper() db, err := sqlx.Open("sqlite3", ":memory:") if err != nil { @@ -37,7 +48,7 @@ func newTestService(t *testing.T) (*runsservice.Service, bus.EventBus) { eb := bus.NewMemoryEventBus(log) svc := runsservice.New(officeRepo.RunsRepository(), eb, log, nil) - return svc, eb + return svc, eb, officeRepo.RunsRepository() } // agentInPayload is the shape office.QueueRun packs into the runs @@ -62,6 +73,300 @@ func TestQueueRun_InsertsRow(t *testing.T) { } } +func TestQueueRun_UsesRequestAgentProfileIDAndAddsEnvelopePayload(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "task-1", + WorkflowStepID: "work", + Reason: "task_comment", + IdempotencyKey: "task_comment:comment-1", + Payload: map[string]any{ + "comment_id": "comment-1", + "author_id": "user-1", + }, + }); err != nil { + t.Fatalf("queue: %v", err) + } + + statuses, err := repo.GetRunsByCommentIDs(ctx, []string{"comment-1"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + status, ok := statuses["comment-1"] + if !ok { + t.Fatalf("missing run status for comment-1: %+v", statuses) + } + run, err := repo.GetRunByID(ctx, status.RunID) + if err != nil { + t.Fatalf("get run: %v", err) + } + if run.AgentProfileID != "agent-primary" { + t.Fatalf("agent_profile_id = %q, want agent-primary", run.AgentProfileID) + } + if run.IdempotencyKey == nil || *run.IdempotencyKey != "task_comment:comment-1" { + t.Fatalf("idempotency_key = %v, want task_comment:comment-1", run.IdempotencyKey) + } + + var payload map[string]any + if err := json.Unmarshal([]byte(run.Payload), &payload); err != nil { + t.Fatalf("decode payload: %v", err) + } + for k, want := range map[string]string{ + "agent_profile_id": "agent-primary", + "task_id": "task-1", + "workflow_step_id": "work", + "comment_id": "comment-1", + "author_id": "user-1", + } { + if got, _ := payload[k].(string); got != want { + t.Fatalf("payload[%s] = %q, want %q (payload=%v)", k, got, want, payload) + } + } +} + +func TestQueueRun_DoesNotCoalesceDistinctTaskComments(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + for _, commentID := range []string{"comment-1", "comment-2"} { + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "task-1", + WorkflowStepID: "work", + Reason: "task_comment", + IdempotencyKey: "task_comment:" + commentID, + Payload: map[string]any{ + "comment_id": commentID, + }, + }); err != nil { + t.Fatalf("queue %s: %v", commentID, err) + } + } + + statuses, err := repo.GetRunsByCommentIDs(ctx, []string{"comment-1", "comment-2"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + if len(statuses) != 2 { + t.Fatalf("statuses = %+v, want one run per distinct comment", statuses) + } + if statuses["comment-1"].RunID == statuses["comment-2"].RunID { + t.Fatalf("distinct comments must not point at the same coalesced run: %+v", statuses) + } +} + +func TestQueueRun_DoesNotCoalesceTaskCommentPrefixWithCustomReason(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + commentIDs := []string{"comment-1", "comment-2"} + keys := []string{ + "task_comment:comment-1:work:task-1:agent-primary:follow-up", + "task_comment:comment-2:work:task-1:agent-primary:follow-up", + } + for i, key := range keys { + commentID := commentIDs[i] + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "task-1", + WorkflowStepID: "work", + Reason: "follow_up", + IdempotencyKey: key, + Payload: map[string]any{ + "comment_id": commentID, + }, + }); err != nil { + t.Fatalf("queue %s: %v", commentID, err) + } + } + + for _, key := range keys { + exists, err := repo.CheckIdempotencyKey(ctx, key, runsservice.IdempotencyWindowHours) + if err != nil { + t.Fatalf("check idempotency key %s: %v", key, err) + } + if !exists { + t.Fatalf("missing inserted run for idempotency key %s", key) + } + } +} + +func TestQueueRun_LegacyCommentWakeDoesNotOverwriteCanonicalCommentRun(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "target-task", + WorkflowStepID: "work", + Reason: "task_comment", + IdempotencyKey: "task_comment:comment-engine:work:target-task:agent-primary:abcd1234", + Payload: map[string]any{ + "comment_id": "comment-engine", + "source_task_id": "source-task", + }, + }); err != nil { + t.Fatalf("queue engine run: %v", err) + } + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "source-task", + Reason: "task_comment", + Payload: map[string]any{ + "comment_id": "comment-legacy", + }, + }); err != nil { + t.Fatalf("queue legacy run: %v", err) + } + + statuses, err := repo.GetRunsByCommentIDs(ctx, []string{"comment-engine", "comment-legacy"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + if len(statuses) != 2 { + t.Fatalf("statuses = %+v, want both comment runs", statuses) + } + if statuses["comment-engine"].RunID == statuses["comment-legacy"].RunID { + t.Fatalf("legacy run coalesced into canonical comment run: %+v", statuses) + } + engineRun, err := repo.GetRunByID(ctx, statuses["comment-engine"].RunID) + if err != nil { + t.Fatalf("get engine run: %v", err) + } + var payload map[string]any + if err := json.Unmarshal([]byte(engineRun.Payload), &payload); err != nil { + t.Fatalf("decode engine payload: %v", err) + } + if payload["task_id"] != "target-task" || payload["source_task_id"] != "source-task" { + t.Fatalf("engine payload was overwritten by legacy coalesce: %#v", payload) + } + +} + +func TestQueueRun_SaltedKeyUsesPayloadCommentID(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "task-1", + WorkflowStepID: "work", + Reason: "task_comment", + IdempotencyKey: "task_comment:trigger-comment:work:task-1:agent-primary:abcd1234", + Payload: map[string]any{ + "comment_id": "action-comment", + }, + }); err != nil { + t.Fatalf("queue: %v", err) + } + + statuses, err := repo.GetRunsByCommentIDs(ctx, []string{"trigger-comment", "action-comment"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + if _, ok := statuses["trigger-comment"]; ok { + t.Fatalf("salted run attached to trigger comment instead of payload comment: %+v", statuses) + } + if _, ok := statuses["action-comment"]; !ok { + t.Fatalf("missing payload comment status: %+v", statuses) + } +} + +// TestQueueRun_CommentStatusIgnoresMentionRuns pins that +// task_mentioned runs carrying the same payload comment_id do not +// replace the task_comment run shown on the comment. +func TestQueueRun_CommentStatusIgnoresMentionRuns(t *testing.T) { + svc, _, repo := newTestServiceWithRepo(t) + ctx := context.Background() + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "task-1", + Reason: "task_comment", + IdempotencyKey: "task_comment:comment-1:work:task-1:agent-primary:abcd1234", + Payload: map[string]any{ + "comment_id": "comment-1", + }, + }); err != nil { + t.Fatalf("queue task_comment: %v", err) + } + if err := repo.CreateRun(ctx, &models.Run{ + ID: "mention-run", + AgentProfileID: "mentioned-agent", + Reason: "task_mentioned", + Payload: `{"comment_id":"comment-1","task_id":"task-1"}`, + Status: "queued", + CoalescedCount: 1, + }); err != nil { + t.Fatalf("create mention run: %v", err) + } + if err := repo.SetRunRequestedAtForTest(ctx, "mention-run", time.Now().UTC().Add(time.Hour)); err != nil { + t.Fatalf("move mention run later: %v", err) + } + + statuses, err := repo.GetRunsByCommentIDs(ctx, []string{"comment-1"}) + if err != nil { + t.Fatalf("get comment runs: %v", err) + } + status, ok := statuses["comment-1"] + if !ok { + t.Fatalf("missing comment status: %+v", statuses) + } + run, err := repo.GetRunByID(ctx, status.RunID) + if err != nil { + t.Fatalf("get run: %v", err) + } + if run.Reason != "task_comment" { + t.Fatalf("comment status attached to reason %q, want task_comment", run.Reason) + } +} + +func TestQueueRun_PublishesSourceTaskForCrossTaskComment(t *testing.T) { + svc, eb := newTestService(t) + ctx := context.Background() + + eventsCh := make(chan map[string]interface{}, 1) + _, err := eb.Subscribe(events.OfficeRunQueued, func(_ context.Context, e *bus.Event) error { + if data, ok := e.Data.(map[string]interface{}); ok { + eventsCh <- data + } + return nil + }) + if err != nil { + t.Fatalf("subscribe: %v", err) + } + + if err := svc.QueueRun(ctx, runsservice.QueueRunRequest{ + AgentProfileID: "agent-primary", + TaskID: "target-task", + WorkflowStepID: "target-step", + Reason: "task_comment", + IdempotencyKey: "task_comment:comment-1:work:target-task:agent-primary:abcd1234", + Payload: map[string]any{ + "comment_id": "comment-1", + "source_task_id": "source-task", + }, + }); err != nil { + t.Fatalf("queue: %v", err) + } + + select { + case data := <-eventsCh: + if data["task_id"] != "source-task" { + t.Fatalf("event task_id = %v, want source-task", data["task_id"]) + } + if data["comment_id"] != "comment-1" { + t.Fatalf("event comment_id = %v, want comment-1", data["comment_id"]) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("timed out waiting for OfficeRunQueued event") + } +} + // TestQueueRun_RejectsWithoutAgent pins that the service refuses to // insert when no agent can be resolved from the payload (the // resolver-less path is what office.QueueRun uses today). diff --git a/apps/backend/internal/scheduler/cron/heartbeat.go b/apps/backend/internal/scheduler/cron/heartbeat.go index 12b82c1462..049c4fdc6e 100644 --- a/apps/backend/internal/scheduler/cron/heartbeat.go +++ b/apps/backend/internal/scheduler/cron/heartbeat.go @@ -78,7 +78,7 @@ type HeartbeatAgentRuntime interface { // (engine state is keyed on session id) and forward to // engine.HandleTrigger. // -// Mirrors office/service.WorkflowEngineDispatcher to keep wiring +// Mirrors office/shared.WorkflowEngineDispatcher to keep wiring // trivial — the office's existing engine_dispatcher.Dispatcher // satisfies this interface unchanged. type HeartbeatEngineDispatcher interface { diff --git a/apps/backend/internal/workflow/adapters/adapters.go b/apps/backend/internal/workflow/adapters/adapters.go index b8d11d528e..1cbd942391 100644 --- a/apps/backend/internal/workflow/adapters/adapters.go +++ b/apps/backend/internal/workflow/adapters/adapters.go @@ -24,7 +24,8 @@ type WorkflowRepo interface { ListStepDecisions(ctx context.Context, taskID, stepID string) ([]*models.WorkflowStepDecision, error) RecordStepDecision(ctx context.Context, d *models.WorkflowStepDecision) error ClearStepDecisions(ctx context.Context, taskID, stepID string) (int64, error) - GetStep(ctx context.Context, id string) (*models.WorkflowStep, error) + ResolveCurrentRunner(ctx context.Context, stepID, taskID string) (string, error) + GetTaskWorkflowStepID(ctx context.Context, taskID string) (string, error) } // Compile-time check that *repository.Repository satisfies WorkflowRepo. @@ -65,6 +66,11 @@ func (a *ParticipantAdapter) ListStepParticipants( return out, nil } +// WorkflowStepIDForTask satisfies engine.TargetTaskStepResolver. +func (a *ParticipantAdapter) WorkflowStepIDForTask(ctx context.Context, taskID string) (string, error) { + return a.Repo.GetTaskWorkflowStepID(ctx, taskID) +} + // DecisionAdapter implements engine.DecisionStore. type DecisionAdapter struct { Repo WorkflowRepo @@ -120,7 +126,8 @@ func (a *DecisionAdapter) ClearStepDecisions( } // PrimaryAgentAdapter implements engine.PrimaryAgentResolver. The "primary" -// agent for a workflow step is the one stored on workflow_steps.agent_profile_id. +// agent for a task is its current runner participant, falling back to the +// workflow step's agent_profile_id when no task-specific runner exists. type PrimaryAgentAdapter struct { Repo WorkflowRepo } @@ -132,18 +139,25 @@ func NewPrimaryAgentAdapter(repo WorkflowRepo) *PrimaryAgentAdapter { // PrimaryAgentProfileID satisfies engine.PrimaryAgentResolver. func (a *PrimaryAgentAdapter) PrimaryAgentProfileID( - ctx context.Context, stepID string, + ctx context.Context, stepID, taskID string, ) (string, error) { - step, err := a.Repo.GetStep(ctx, stepID) + agentID, err := a.Repo.ResolveCurrentRunner(ctx, stepID, taskID) if err != nil { - return "", fmt.Errorf("get step %s: %w", stepID, err) + return "", fmt.Errorf("resolve current runner for step %s task %s: %w", stepID, taskID, err) } - return step.AgentProfileID, nil + return agentID, nil +} + +// WorkflowStepIDForTask satisfies engine.TargetTaskStepResolver. +func (a *PrimaryAgentAdapter) WorkflowStepIDForTask(ctx context.Context, taskID string) (string, error) { + return a.Repo.GetTaskWorkflowStepID(ctx, taskID) } // Compile-time interface assertions. var ( - _ engine.ParticipantStore = (*ParticipantAdapter)(nil) - _ engine.DecisionStore = (*DecisionAdapter)(nil) - _ engine.PrimaryAgentResolver = (*PrimaryAgentAdapter)(nil) + _ engine.ParticipantStore = (*ParticipantAdapter)(nil) + _ engine.TargetTaskStepResolver = (*ParticipantAdapter)(nil) + _ engine.DecisionStore = (*DecisionAdapter)(nil) + _ engine.PrimaryAgentResolver = (*PrimaryAgentAdapter)(nil) + _ engine.TargetTaskStepResolver = (*PrimaryAgentAdapter)(nil) ) diff --git a/apps/backend/internal/workflow/adapters/adapters_test.go b/apps/backend/internal/workflow/adapters/adapters_test.go index 51b2cfd129..2ae126ca47 100644 --- a/apps/backend/internal/workflow/adapters/adapters_test.go +++ b/apps/backend/internal/workflow/adapters/adapters_test.go @@ -15,15 +15,20 @@ import ( type fakeRepo struct { participants []*models.WorkflowStepParticipant decisions []*models.WorkflowStepDecision - step *models.WorkflowStep + runner string + taskStepID string listErr error recordErr error clearErr error - getErr error + runnerErr error + stepErr error - recorded *models.WorkflowStepDecision - cleared bool + recorded *models.WorkflowStepDecision + cleared bool + resolvedStepID string + resolvedTaskID string + lookedUpTaskID string } func (f *fakeRepo) ListStepParticipantsForTask(_ context.Context, _, _ string) ([]*models.WorkflowStepParticipant, error) { @@ -46,8 +51,14 @@ func (f *fakeRepo) ClearStepDecisions(_ context.Context, _, _ string) (int64, er f.cleared = true return int64(len(f.decisions)), nil } -func (f *fakeRepo) GetStep(_ context.Context, _ string) (*models.WorkflowStep, error) { - return f.step, f.getErr +func (f *fakeRepo) ResolveCurrentRunner(_ context.Context, stepID, taskID string) (string, error) { + f.resolvedStepID = stepID + f.resolvedTaskID = taskID + return f.runner, f.runnerErr +} +func (f *fakeRepo) GetTaskWorkflowStepID(_ context.Context, taskID string) (string, error) { + f.lookedUpTaskID = taskID + return f.taskStepID, f.stepErr } func TestParticipantAdapter_TranslatesModelToEngineInfo(t *testing.T) { @@ -134,22 +145,25 @@ func TestDecisionAdapter_ClearForwards(t *testing.T) { } } -func TestPrimaryAgentAdapter_ReturnsStepAgentProfileID(t *testing.T) { - repo := &fakeRepo{step: &models.WorkflowStep{ID: "s-1", AgentProfileID: "ap-primary"}} +func TestPrimaryAgentAdapter_ReturnsCurrentRunner(t *testing.T) { + repo := &fakeRepo{runner: "ap-runner"} a := NewPrimaryAgentAdapter(repo) - got, err := a.PrimaryAgentProfileID(context.Background(), "s-1") + got, err := a.PrimaryAgentProfileID(context.Background(), "s-1", "task-1") if err != nil { t.Fatalf("primary: %v", err) } - if got != "ap-primary" { - t.Errorf("primary = %q, want ap-primary", got) + if got != "ap-runner" { + t.Errorf("primary = %q, want ap-runner", got) + } + if repo.resolvedStepID != "s-1" || repo.resolvedTaskID != "task-1" { + t.Errorf("ResolveCurrentRunner called with step=%q task=%q", repo.resolvedStepID, repo.resolvedTaskID) } } -func TestPrimaryAgentAdapter_PropagatesGetStepError(t *testing.T) { - repo := &fakeRepo{getErr: errors.New("missing")} +func TestPrimaryAgentAdapter_PropagatesResolveCurrentRunnerError(t *testing.T) { + repo := &fakeRepo{runnerErr: errors.New("missing")} a := NewPrimaryAgentAdapter(repo) - if _, err := a.PrimaryAgentProfileID(context.Background(), "s-x"); err == nil { + if _, err := a.PrimaryAgentProfileID(context.Background(), "s-x", "task-x"); err == nil { t.Fatal("expected error") } } diff --git a/apps/backend/internal/workflow/engine/engine.go b/apps/backend/internal/workflow/engine/engine.go index e9fd211fab..f24492b7ae 100644 --- a/apps/backend/internal/workflow/engine/engine.go +++ b/apps/backend/internal/workflow/engine/engine.go @@ -85,6 +85,7 @@ type HandleResult struct { ToStepID string DataPatch map[string]any Idempotent bool + ActionCount int } // Option configures an Engine at construction time. Use With* helpers below. @@ -213,7 +214,7 @@ func (e *Engine) processActions( } } - result := HandleResult{DataPatch: dataPatch} + result := HandleResult{DataPatch: dataPatch, ActionCount: len(actions)} if targetStepID != "" && targetStepID != state.CurrentStepID { if !in.EvaluateOnly { if err := e.applyTransition(ctx, in, state, targetStepID); err != nil { diff --git a/apps/backend/internal/workflow/engine/office_default_smoke_test.go b/apps/backend/internal/workflow/engine/office_default_smoke_test.go index 502506fce9..ad2844949b 100644 --- a/apps/backend/internal/workflow/engine/office_default_smoke_test.go +++ b/apps/backend/internal/workflow/engine/office_default_smoke_test.go @@ -320,7 +320,7 @@ func (p *smokeParticipants) ListStepParticipants(_ context.Context, stepID, _ st type stubPrimary struct{ id string } -func (s stubPrimary) PrimaryAgentProfileID(_ context.Context, _ string) (string, error) { +func (s stubPrimary) PrimaryAgentProfileID(_ context.Context, _, _ string) (string, error) { return s.id, nil } diff --git a/apps/backend/internal/workflow/engine/phase2_callbacks.go b/apps/backend/internal/workflow/engine/phase2_callbacks.go index f3c965c1d8..ea5cd39ef3 100644 --- a/apps/backend/internal/workflow/engine/phase2_callbacks.go +++ b/apps/backend/internal/workflow/engine/phase2_callbacks.go @@ -2,9 +2,14 @@ package engine import ( "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" "errors" "fmt" "strings" + + "github.com/kandev/kandev/internal/runs/commentkeys" ) // ErrActionNotYetWired is the sentinel returned by Phase 2 callbacks when a @@ -24,12 +29,20 @@ const ( defaultQueueReasonR = "queue_run" ) -// PrimaryAgentResolver resolves the step's "primary" agent profile id. The -// engine asks via this interface when a queue_run target is "primary" — the -// answer is just step.PrimaryAgentProfileID for kanban-style steps. The -// indirection keeps the engine package free of model imports. +// PrimaryAgentResolver resolves the task's "primary" agent profile id. The +// engine asks via this interface when a queue_run target is "primary". The +// answer is task-aware so office steps can prefer the task's current runner +// participant while kanban-style steps still fall back to the step primary. +// The indirection keeps the engine package free of model imports. type PrimaryAgentResolver interface { - PrimaryAgentProfileID(ctx context.Context, stepID string) (string, error) + PrimaryAgentProfileID(ctx context.Context, stepID, taskID string) (string, error) +} + +// TargetTaskStepResolver resolves a task's current workflow step for cross-task +// queue_run actions. The engine only has the triggering task's StepSpec, so +// adapters that read task storage provide this for target task lookups. +type TargetTaskStepResolver interface { + WorkflowStepIDForTask(ctx context.Context, taskID string) (string, error) } // QueueRunCallback executes the queue_run action by resolving Target/TaskID @@ -39,6 +52,7 @@ type QueueRunCallback struct { Participants ParticipantStore CEOResolver CEOAgentResolver Primary PrimaryAgentResolver + TaskSteps TargetTaskStepResolver } // Execute satisfies ActionCallback. @@ -50,7 +64,7 @@ func (c QueueRunCallback) Execute(ctx context.Context, in ActionInput) (ActionRe return ActionResult{}, fmt.Errorf("queue_run action missing QueueRun config") } taskID := resolveTaskID(in.Action.QueueRun.TaskID, in.State.TaskID) - agentIDs, err := c.resolveTarget(ctx, in) + agentIDs, workflowStepID, err := c.resolveTarget(ctx, in, taskID) if err != nil { return ActionResult{}, err } @@ -58,10 +72,10 @@ func (c QueueRunCallback) Execute(ctx context.Context, in ActionInput) (ActionRe req := QueueRunRequest{ AgentProfileID: agentID, TaskID: taskID, - WorkflowStepID: in.Step.ID, + WorkflowStepID: workflowStepID, Reason: queueRunReason(in), IdempotencyKey: idempotencyKey(in, agentID, taskID), - Payload: in.Action.QueueRun.Payload, + Payload: queueRunPayload(in, in.Action.QueueRun.Payload, taskID), } if err := c.Adapter.QueueRun(ctx, req); err != nil { return ActionResult{}, fmt.Errorf("queue_run for agent %s: %w", agentID, err) @@ -70,32 +84,79 @@ func (c QueueRunCallback) Execute(ctx context.Context, in ActionInput) (ActionRe return ActionResult{}, nil } -func (c QueueRunCallback) resolveTarget(ctx context.Context, in ActionInput) ([]string, error) { +func (c QueueRunCallback) resolveTarget( + ctx context.Context, in ActionInput, taskID string, +) ([]string, string, error) { target := strings.TrimSpace(in.Action.QueueRun.Target) switch { case target == "" || target == TargetPrimary: - return c.resolvePrimary(ctx, in.Step.ID) + stepID, err := c.resolveTargetStepID(ctx, in, taskID, pickStepResolver(c.Primary, c.TaskSteps)) + if err != nil { + return nil, "", err + } + agentIDs, err := c.resolvePrimary(ctx, taskID, stepID) + return agentIDs, stepID, err case strings.HasPrefix(target, TargetParticipant): role := strings.TrimPrefix(target, TargetParticipant) - return c.resolveParticipantRole(ctx, in.Step.ID, in.State.TaskID, role) + stepID, err := c.resolveTargetStepID(ctx, in, taskID, pickStepResolver(c.Participants, c.TaskSteps)) + if err != nil { + return nil, "", err + } + agentIDs, err := c.resolveParticipantRole(ctx, stepID, taskID, role) + return agentIDs, stepID, err case strings.HasPrefix(target, TargetAgentProfile): id := strings.TrimPrefix(target, TargetAgentProfile) if id == "" { - return nil, fmt.Errorf("queue_run agent_profile_id target is empty") + return nil, "", fmt.Errorf("queue_run agent_profile_id target is empty") } - return []string{id}, nil + stepID, err := c.resolveTargetStepID(ctx, in, taskID, c.TaskSteps) + if err != nil { + return nil, "", err + } + return []string{id}, stepID, nil case target == TargetWorkspaceCEO: - return c.resolveCEO(ctx, in.State.TaskID) + stepID, err := c.resolveTargetStepID(ctx, in, taskID, c.TaskSteps) + if err != nil { + return nil, "", err + } + agentIDs, err := c.resolveCEO(ctx, taskID) + return agentIDs, stepID, err default: - return nil, fmt.Errorf("queue_run: unsupported target %q", target) + return nil, "", fmt.Errorf("queue_run: unsupported target %q", target) } } -func (c QueueRunCallback) resolvePrimary(ctx context.Context, stepID string) ([]string, error) { +func (c QueueRunCallback) resolveTargetStepID( + ctx context.Context, in ActionInput, taskID string, resolver TargetTaskStepResolver, +) (string, error) { + if taskID == in.State.TaskID { + return in.Step.ID, nil + } + if resolver == nil { + return "", fmt.Errorf("%w: queue_run cross-task target requires TargetTaskStepResolver", ErrActionNotYetWired) + } + stepID, err := resolver.WorkflowStepIDForTask(ctx, taskID) + if err != nil { + return "", fmt.Errorf("queue_run resolve target task step: %w", err) + } + if stepID == "" { + return "", fmt.Errorf("queue_run: task %s has no workflow step", taskID) + } + return stepID, nil +} + +func pickStepResolver(v any, fallback TargetTaskStepResolver) TargetTaskStepResolver { + if resolver, ok := v.(TargetTaskStepResolver); ok { + return resolver + } + return fallback +} + +func (c QueueRunCallback) resolvePrimary(ctx context.Context, taskID, stepID string) ([]string, error) { if c.Primary == nil { return nil, fmt.Errorf("%w: queue_run target=primary requires PrimaryAgentResolver", ErrActionNotYetWired) } - id, err := c.Primary.PrimaryAgentProfileID(ctx, stepID) + id, err := c.Primary.PrimaryAgentProfileID(ctx, stepID, taskID) if err != nil { return nil, fmt.Errorf("queue_run resolve primary: %w", err) } @@ -163,13 +224,94 @@ func queueRunReason(in ActionInput) string { // idempotencyKey synthesises a deterministic key from the engine's // operation id (already idempotent across retries) plus action-specific -// salt. When OperationID is empty, the adapter sees an empty key and is -// expected to dedupe via its own mechanism (or accept the duplicate). +// salt. Comment status lookups map runs back through payload.comment_id, so +// even same-task primary comment wakes keep agent/task/action salt. That lets +// a later wake for the same comment reach a newly resolved runner instead of +// being suppressed by a bare task_comment: key. +// When OperationID is empty, the adapter sees an empty key and is expected to +// dedupe via its own mechanism (or accept the duplicate). func idempotencyKey(in ActionInput, agentID, taskID string) string { if in.OperationID == "" { return "" } - return fmt.Sprintf("%s:%s:%s:%s", in.OperationID, in.Step.ID, taskID, agentID) + return fmt.Sprintf("%s:%s:%s:%s:%s", + in.OperationID, in.Step.ID, taskID, agentID, queueActionDigest(in)) +} + +func queueActionDigest(in ActionInput) string { + key := struct { + Kind ActionKind `json:"kind"` + Target string `json:"target,omitempty"` + TaskID string `json:"task_id,omitempty"` + Role string `json:"role,omitempty"` + Reason string `json:"reason"` + Payload map[string]any `json:"payload,omitempty"` + }{ + Kind: in.Action.Kind, + } + switch in.Action.Kind { + case ActionQueueRun: + if in.Action.QueueRun != nil { + key.Target = strings.TrimSpace(in.Action.QueueRun.Target) + key.TaskID = strings.TrimSpace(in.Action.QueueRun.TaskID) + key.Reason = queueRunReason(in) + key.Payload = in.Action.QueueRun.Payload + } + case ActionQueueRunForEachParticipant: + if in.Action.QueueRunForEachParticipant != nil { + cfg := in.Action.QueueRunForEachParticipant + key.Role = strings.TrimSpace(cfg.Role) + key.Reason = queueRunForEachParticipantReason(in) + key.Payload = cfg.Payload + } + } + b, err := json.Marshal(key) + if err != nil { + b = []byte(string(key.Kind) + "\x00" + key.Target + "\x00" + + key.TaskID + "\x00" + key.Role + "\x00" + key.Reason) + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:8]) +} + +func queueRunPayload(in ActionInput, actionPayload map[string]any, targetTaskID string) map[string]any { + out := make(map[string]any, len(actionPayload)) + comment, ok := commentPayload(in.Payload) + if ok { + if comment.CommentID != "" { + out["comment_id"] = comment.CommentID + } + if comment.AuthorID != "" { + out["author_id"] = comment.AuthorID + } + } + // Workflow-authored payload fields are explicit overrides. The trigger's + // comment_id/author_id only provide defaults for ordinary comment wakes. + for k, v := range actionPayload { + out[k] = v + } + if targetTaskID != "" && + in.State.TaskID != "" && + targetTaskID != in.State.TaskID && + (ok || commentkeys.HasTaskCommentPrefix(in.OperationID)) { + out["source_task_id"] = in.State.TaskID + } + if len(out) == 0 { + return nil + } + return out +} + +func commentPayload(payload any) (OnCommentPayload, bool) { + switch p := payload.(type) { + case OnCommentPayload: + return p, true + case *OnCommentPayload: + if p != nil { + return *p, true + } + } + return OnCommentPayload{}, false } // ClearDecisionsCallback executes the clear_decisions action by deleting all @@ -213,10 +355,7 @@ func (c QueueRunForEachParticipantCallback) Execute(ctx context.Context, in Acti if err != nil { return ActionResult{}, fmt.Errorf("queue_run_for_each_participant list participants: %w", err) } - reason := cfg.Reason - if reason == "" { - reason = string(in.Trigger) - } + reason := queueRunForEachParticipantReason(in) for _, p := range all { if p.Role != cfg.Role { continue @@ -227,7 +366,7 @@ func (c QueueRunForEachParticipantCallback) Execute(ctx context.Context, in Acti WorkflowStepID: in.Step.ID, Reason: reason, IdempotencyKey: idempotencyKey(in, p.AgentProfileID, taskID), - Payload: cfg.Payload, + Payload: queueRunPayload(in, cfg.Payload, taskID), } if err := c.Adapter.QueueRun(ctx, req); err != nil { return ActionResult{}, fmt.Errorf("queue_run for participant %s: %w", p.ID, err) @@ -236,6 +375,13 @@ func (c QueueRunForEachParticipantCallback) Execute(ctx context.Context, in Acti return ActionResult{}, nil } +func queueRunForEachParticipantReason(in ActionInput) string { + if in.Action.QueueRunForEachParticipant != nil && in.Action.QueueRunForEachParticipant.Reason != "" { + return in.Action.QueueRunForEachParticipant.Reason + } + return string(in.Trigger) +} + // PlaceholderQueueRunCallback is preserved as a typed alias for backward // compatibility with the Phase 2 prep slice. New code should construct // QueueRunCallback directly. diff --git a/apps/backend/internal/workflow/engine/queue_run_test.go b/apps/backend/internal/workflow/engine/queue_run_test.go index 1588c7a425..dedd0d0c5e 100644 --- a/apps/backend/internal/workflow/engine/queue_run_test.go +++ b/apps/backend/internal/workflow/engine/queue_run_test.go @@ -3,6 +3,7 @@ package engine import ( "context" "errors" + "strings" "testing" ) @@ -19,24 +20,76 @@ func (f *fakeRunQueue) QueueRun(_ context.Context, req QueueRunRequest) error { // fakePrimary returns a fixed agent profile id for any step. type fakePrimary struct { - id string - err error + id string + err error + taskStepID string + stepID *string + taskID *string + resolveTaskID *string } -func (f fakePrimary) PrimaryAgentProfileID(_ context.Context, _ string) (string, error) { +func (f fakePrimary) PrimaryAgentProfileID(_ context.Context, stepID, taskID string) (string, error) { + if f.stepID != nil { + *f.stepID = stepID + } + if f.taskID != nil { + *f.taskID = taskID + } return f.id, f.err } +func (f fakePrimary) WorkflowStepIDForTask(_ context.Context, taskID string) (string, error) { + if f.resolveTaskID != nil { + *f.resolveTaskID = taskID + } + return f.taskStepID, f.err +} + +type sequencePrimary struct { + ids []string + i int +} + +func (s *sequencePrimary) PrimaryAgentProfileID(_ context.Context, _, _ string) (string, error) { + if s.i >= len(s.ids) { + return "", nil + } + id := s.ids[s.i] + s.i++ + return id, nil +} + +func (s *sequencePrimary) WorkflowStepIDForTask(_ context.Context, _ string) (string, error) { + return "step-1", nil +} + // fakeParticipants returns a static slice for any step. type fakeParticipants struct { - list []ParticipantInfo - err error + list []ParticipantInfo + err error + taskStepID string + stepID *string + taskID *string + resolveTaskID *string } -func (f fakeParticipants) ListStepParticipants(_ context.Context, _, _ string) ([]ParticipantInfo, error) { +func (f fakeParticipants) ListStepParticipants(_ context.Context, stepID, taskID string) ([]ParticipantInfo, error) { + if f.stepID != nil { + *f.stepID = stepID + } + if f.taskID != nil { + *f.taskID = taskID + } return f.list, f.err } +func (f fakeParticipants) WorkflowStepIDForTask(_ context.Context, taskID string) (string, error) { + if f.resolveTaskID != nil { + *f.resolveTaskID = taskID + } + return f.taskStepID, f.err +} + // fakeCEO returns a fixed agent profile id (or empty / err). type fakeCEO struct { id string @@ -47,6 +100,19 @@ func (f fakeCEO) ResolveCEOAgentProfileID(_ context.Context, _ string) (string, return f.id, f.err } +type fakeTaskSteps struct { + id string + err error + taskID *string +} + +func (f fakeTaskSteps) WorkflowStepIDForTask(_ context.Context, taskID string) (string, error) { + if f.taskID != nil { + *f.taskID = taskID + } + return f.id, f.err +} + func newQueueRunInput(target, taskID string) ActionInput { return ActionInput{ Trigger: TriggerOnComment, @@ -95,6 +161,207 @@ func TestQueueRunCallback_TargetPrimary(t *testing.T) { } } +func TestQueueRunCallback_OnCommentUsesCommentPayloadAndStableKey(t *testing.T) { + q := &fakeRunQueue{} + cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "agent-primary"}} + in := newQueueRunInput("primary", "this") + in.OperationID = "task_comment:c-1" + in.Payload = OnCommentPayload{ + CommentID: "c-1", + AuthorID: "user-1", + } + in.Action.QueueRun.Payload = nil + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 1 { + t.Fatalf("expected 1 queue run call, got %d", len(q.calls)) + } + got := q.calls[0] + if got.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("idempotency_key dropped agent/action salt") + } + if !strings.HasPrefix(got.IdempotencyKey, "task_comment:c-1:step-1:task-1:agent-primary:") { + t.Fatalf("idempotency_key = %q, want salted task_comment key", got.IdempotencyKey) + } + if got.Payload["comment_id"] != "c-1" { + t.Fatalf("payload comment_id = %v, want c-1 (payload=%#v)", got.Payload["comment_id"], got.Payload) + } + if got.Payload["author_id"] != "user-1" { + t.Fatalf("payload author_id = %v, want user-1 (payload=%#v)", got.Payload["author_id"], got.Payload) + } +} + +func TestQueueRunCallback_OnCommentPrimaryKeysIncludeResolvedAgent(t *testing.T) { + q := &fakeRunQueue{} + primary := &sequencePrimary{ids: []string{"agent-a", "agent-b"}} + cb := QueueRunCallback{Adapter: q, Primary: primary} + in := newQueueRunInput("primary", "this") + in.OperationID = "task_comment:c-1" + in.Payload = OnCommentPayload{CommentID: "c-1"} + in.Action.QueueRun.Payload = nil + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("first queue_run: %v", err) + } + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("second queue_run: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 queue run calls, got %d", len(q.calls)) + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("resolved agent must salt comment idempotency keys: %#v", q.calls) + } + if !strings.Contains(q.calls[0].IdempotencyKey, ":agent-a:") || + !strings.Contains(q.calls[1].IdempotencyKey, ":agent-b:") { + t.Fatalf("idempotency keys missing resolved agents: %#v", q.calls) + } +} + +func TestQueueRunCallback_ActionPayloadOverridesTriggerCommentIdentity(t *testing.T) { + q := &fakeRunQueue{} + cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "agent-primary"}} + in := newQueueRunInput("primary", "this") + in.OperationID = "task_comment:c-trigger" + in.Payload = OnCommentPayload{ + CommentID: "c-trigger", + AuthorID: "user-trigger", + } + in.Action.QueueRun.Payload = map[string]any{ + "comment_id": "c-action", + "author_id": "user-action", + } + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 1 { + t.Fatalf("expected 1 queue run call, got %d", len(q.calls)) + } + got := q.calls[0].Payload + if got["comment_id"] != "c-action" || got["author_id"] != "user-action" { + t.Fatalf("action payload should override trigger comment identity: %#v", got) + } +} + +func TestQueueRunCallback_OnCommentCustomReasonsKeepActionSalt(t *testing.T) { + q := &fakeRunQueue{} + cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "agent-primary"}} + + first := newQueueRunInput("primary", "this") + first.OperationID = "task_comment:c-1" + first.Action.QueueRun.Payload = nil + first.Action.QueueRun.Reason = "follow_up" + second := newQueueRunInput("primary", "this") + second.OperationID = "task_comment:c-1" + second.Action.QueueRun.Payload = nil + second.Action.QueueRun.Reason = "notify_observer" + + if _, err := cb.Execute(context.Background(), first); err != nil { + t.Fatalf("first queue_run: %v", err) + } + if _, err := cb.Execute(context.Background(), second); err != nil { + t.Fatalf("second queue_run: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 queue run calls, got %d", len(q.calls)) + } + for i, call := range q.calls { + if call.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("call %d idempotency_key dropped action salt", i) + } + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("custom reason actions must not share idempotency keys: %#v", q.calls) + } +} + +func TestQueueRunCallback_OnCommentCustomPayloadsKeepActionSalt(t *testing.T) { + q := &fakeRunQueue{} + cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "agent-primary"}} + + first := newQueueRunInput("primary", "this") + first.OperationID = "task_comment:c-1" + first.Action.QueueRun.Payload = map[string]any{"source": "first"} + second := newQueueRunInput("primary", "this") + second.OperationID = "task_comment:c-1" + second.Action.QueueRun.Payload = map[string]any{"source": "second"} + + if _, err := cb.Execute(context.Background(), first); err != nil { + t.Fatalf("first queue_run: %v", err) + } + if _, err := cb.Execute(context.Background(), second); err != nil { + t.Fatalf("second queue_run: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 queue run calls, got %d", len(q.calls)) + } + for i, call := range q.calls { + if call.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("call %d idempotency_key dropped action payload salt", i) + } + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("custom payload actions must not share idempotency keys: %#v", q.calls) + } +} + +func TestQueueRunCallback_PrimaryUsesResolvedTargetTask(t *testing.T) { + q := &fakeRunQueue{} + var resolvedStepID string + var lookupTaskID string + var stepResolverTaskID string + cb := QueueRunCallback{ + Adapter: q, + Primary: fakePrimary{ + id: "agent-for-target-task", + taskStepID: "target-step", + stepID: &resolvedStepID, + taskID: &lookupTaskID, + resolveTaskID: &stepResolverTaskID, + }, + } + in := newQueueRunInput("primary", "target-task") + in.OperationID = "task_comment:c-1" + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 1 { + t.Fatalf("expected 1 queue run call, got %d", len(q.calls)) + } + if stepResolverTaskID != "target-task" { + t.Fatalf("target step resolved against task %q, want target-task", stepResolverTaskID) + } + if lookupTaskID != "target-task" { + t.Fatalf("primary resolved against task %q, want target-task", lookupTaskID) + } + if resolvedStepID != "target-step" { + t.Fatalf("primary resolved against step %q, want target-step", resolvedStepID) + } + got := q.calls[0] + if got.TaskID != "target-task" { + t.Fatalf("queued task_id = %q, want target-task", got.TaskID) + } + if got.WorkflowStepID != "target-step" { + t.Fatalf("workflow_step_id = %q, want target-step", got.WorkflowStepID) + } + if got.AgentProfileID != "agent-for-target-task" { + t.Fatalf("agent_profile_id = %q, want agent-for-target-task", got.AgentProfileID) + } + if got.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("literal-task primary comment wake must keep action/task/agent salt") + } + if got.Payload["comment_id"] != "c-1" { + t.Fatalf("payload comment_id = %v, want c-1", got.Payload["comment_id"]) + } + if got.Payload["source_task_id"] != "task-1" { + t.Fatalf("payload source_task_id = %v, want task-1", got.Payload["source_task_id"]) + } +} + func TestQueueRunCallback_TargetParticipantRole(t *testing.T) { q := &fakeRunQueue{} parts := fakeParticipants{list: []ParticipantInfo{ @@ -119,9 +386,85 @@ func TestQueueRunCallback_TargetParticipantRole(t *testing.T) { } } +func TestQueueRunCallback_ParticipantRoleUsesResolvedTargetStep(t *testing.T) { + q := &fakeRunQueue{} + var listedStepID string + var listedTaskID string + var stepResolverTaskID string + parts := fakeParticipants{ + list: []ParticipantInfo{ + {ID: "p-target", Role: "reviewer", AgentProfileID: "rev-target"}, + }, + taskStepID: "target-step", + stepID: &listedStepID, + taskID: &listedTaskID, + resolveTaskID: &stepResolverTaskID, + } + cb := QueueRunCallback{Adapter: q, Participants: parts} + in := newQueueRunInput("participant_role:reviewer", "target-task") + in.OperationID = "task_comment:c-1" + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 1 { + t.Fatalf("expected 1 queue run call, got %d", len(q.calls)) + } + if stepResolverTaskID != "target-task" { + t.Fatalf("target step resolved against task %q, want target-task", stepResolverTaskID) + } + if listedStepID != "target-step" { + t.Fatalf("participants listed for step %q, want target-step", listedStepID) + } + if listedTaskID != "target-task" { + t.Fatalf("participants listed for task %q, want target-task", listedTaskID) + } + got := q.calls[0] + if got.WorkflowStepID != "target-step" { + t.Fatalf("workflow_step_id = %q, want target-step", got.WorkflowStepID) + } + if got.TaskID != "target-task" { + t.Fatalf("task_id = %q, want target-task", got.TaskID) + } + if got.AgentProfileID != "rev-target" { + t.Fatalf("agent_profile_id = %q, want rev-target", got.AgentProfileID) + } +} + +func TestQueueRunCallback_OnCommentParticipantRoleKeepsPerAgentKeys(t *testing.T) { + q := &fakeRunQueue{} + parts := fakeParticipants{list: []ParticipantInfo{ + {ID: "p1", Role: "reviewer", AgentProfileID: "rev-A"}, + {ID: "p2", Role: "reviewer", AgentProfileID: "rev-B"}, + }} + cb := QueueRunCallback{Adapter: q, Participants: parts} + in := newQueueRunInput("participant_role:reviewer", "") + in.OperationID = "task_comment:c-1" + in.Payload = OnCommentPayload{CommentID: "c-1", AuthorID: "user-1"} + in.Action.QueueRun.Payload = nil + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 queue run calls, got %d", len(q.calls)) + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("fan-out idempotency keys must be per-agent: %#v", q.calls) + } + for i, call := range q.calls { + if call.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("call %d idempotency_key dropped per-agent salt", i) + } + if call.Payload["comment_id"] != "c-1" || call.Payload["author_id"] != "user-1" { + t.Fatalf("call %d payload missing comment identity: %#v", i, call.Payload) + } + } +} + func TestQueueRunCallback_TargetSpecificAgent(t *testing.T) { q := &fakeRunQueue{} - cb := QueueRunCallback{Adapter: q} + cb := QueueRunCallback{Adapter: q, TaskSteps: fakeTaskSteps{id: "target-step"}} if _, err := cb.Execute(context.Background(), newQueueRunInput("agent_profile_id:some-agent", "task-2")); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -134,6 +477,17 @@ func TestQueueRunCallback_TargetSpecificAgent(t *testing.T) { if q.calls[0].TaskID != "task-2" { t.Fatalf("task_id = %q, want task-2 (literal)", q.calls[0].TaskID) } + if q.calls[0].WorkflowStepID != "target-step" { + t.Fatalf("workflow_step_id = %q, want target-step", q.calls[0].WorkflowStepID) + } +} + +func TestQueueRunCallback_TargetSpecificAgentCrossTaskNoStepResolverErrors(t *testing.T) { + cb := QueueRunCallback{Adapter: &fakeRunQueue{}} + _, err := cb.Execute(context.Background(), newQueueRunInput("agent_profile_id:some-agent", "task-2")) + if err == nil || !errors.Is(err, ErrActionNotYetWired) { + t.Fatalf("expected ErrActionNotYetWired, got %v", err) + } } func TestQueueRunCallback_TargetWorkspaceCEO(t *testing.T) { @@ -150,6 +504,45 @@ func TestQueueRunCallback_TargetWorkspaceCEO(t *testing.T) { } } +func TestQueueRunCallback_TargetWorkspaceCEOCrossTaskNoStepResolverErrors(t *testing.T) { + cb := QueueRunCallback{Adapter: &fakeRunQueue{}, CEOResolver: fakeCEO{id: "ceo-agent"}} + _, err := cb.Execute(context.Background(), newQueueRunInput("workspace.ceo_agent", "target-task")) + if err == nil || !errors.Is(err, ErrActionNotYetWired) { + t.Fatalf("expected ErrActionNotYetWired, got %v", err) + } +} + +func TestQueueRunCallback_TargetWorkspaceCEOUsesResolvedTargetStep(t *testing.T) { + q := &fakeRunQueue{} + var resolvedTaskID string + cb := QueueRunCallback{ + Adapter: q, + CEOResolver: fakeCEO{id: "ceo-agent"}, + TaskSteps: fakeTaskSteps{id: "target-step", taskID: &resolvedTaskID}, + } + in := newQueueRunInput("workspace.ceo_agent", "target-task") + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 1 { + t.Fatalf("expected 1 call, got %d", len(q.calls)) + } + if resolvedTaskID != "target-task" { + t.Fatalf("target step resolved for task %q, want target-task", resolvedTaskID) + } + got := q.calls[0] + if got.TaskID != "target-task" { + t.Fatalf("task_id = %q, want target-task", got.TaskID) + } + if got.WorkflowStepID != "target-step" { + t.Fatalf("workflow_step_id = %q, want target-step", got.WorkflowStepID) + } + if got.AgentProfileID != "ceo-agent" { + t.Fatalf("agent_profile_id = %q, want ceo-agent", got.AgentProfileID) + } +} + func TestQueueRunCallback_MissingAdapter_Errors(t *testing.T) { cb := QueueRunCallback{} // no Adapter _, err := cb.Execute(context.Background(), newQueueRunInput("primary", "this")) @@ -202,7 +595,7 @@ func TestQueueRunCallback_TaskIDResolution(t *testing.T) { for _, tc := range cases { t.Run(tc.input, func(t *testing.T) { q := &fakeRunQueue{} - cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "p"}} + cb := QueueRunCallback{Adapter: q, Primary: fakePrimary{id: "p", taskStepID: "target-step"}} if _, err := cb.Execute(context.Background(), newQueueRunInput("primary", tc.input)); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -250,6 +643,97 @@ func TestQueueRunForEachParticipantCallback_FansOut(t *testing.T) { } } +func TestQueueRunForEachParticipantCallback_OnCommentKeepsPerAgentKeys(t *testing.T) { + q := &fakeRunQueue{} + parts := fakeParticipants{list: []ParticipantInfo{ + {ID: "p1", Role: "reviewer", AgentProfileID: "rev-A"}, + {ID: "p2", Role: "reviewer", AgentProfileID: "rev-B"}, + }} + cb := QueueRunForEachParticipantCallback{Adapter: q, Participants: parts} + in := ActionInput{ + Trigger: TriggerOnComment, + State: MachineState{TaskID: "task-1"}, + Step: StepSpec{ID: "step-1"}, + OperationID: "task_comment:c-1", + Payload: OnCommentPayload{CommentID: "c-1", AuthorID: "user-1"}, + Action: Action{ + Kind: ActionQueueRunForEachParticipant, + QueueRunForEachParticipant: &QueueRunForEachParticipantAction{ + Role: "reviewer", + Reason: "task_comment", + }, + }, + } + + if _, err := cb.Execute(context.Background(), in); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 fan-out calls, got %d", len(q.calls)) + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("fan-out idempotency keys must be per-agent: %#v", q.calls) + } + for i, call := range q.calls { + if call.IdempotencyKey == "task_comment:c-1" { + t.Fatalf("call %d idempotency_key dropped per-agent salt", i) + } + if call.Payload["comment_id"] != "c-1" || call.Payload["author_id"] != "user-1" { + t.Fatalf("call %d payload missing comment identity: %#v", i, call.Payload) + } + } +} + +func TestQueueRunForEachParticipantCallback_DifferentActionsKeepActionSalt(t *testing.T) { + q := &fakeRunQueue{} + parts := fakeParticipants{list: []ParticipantInfo{ + {ID: "p1", Role: "reviewer", AgentProfileID: "same-agent"}, + {ID: "p2", Role: "observer", AgentProfileID: "same-agent"}, + }} + cb := QueueRunForEachParticipantCallback{Adapter: q, Participants: parts} + base := ActionInput{ + Trigger: TriggerOnComment, + State: MachineState{TaskID: "task-1"}, + Step: StepSpec{ID: "step-1"}, + OperationID: "task_comment:c-1", + Payload: OnCommentPayload{CommentID: "c-1", AuthorID: "user-1"}, + } + first := base + first.Action = Action{ + Kind: ActionQueueRunForEachParticipant, + QueueRunForEachParticipant: &QueueRunForEachParticipantAction{ + Role: "reviewer", + Reason: "follow_up", + Payload: map[string]any{"source": "reviewer"}, + }, + } + second := base + second.Action = Action{ + Kind: ActionQueueRunForEachParticipant, + QueueRunForEachParticipant: &QueueRunForEachParticipantAction{ + Role: "observer", + Reason: "follow_up", + Payload: map[string]any{"source": "observer"}, + }, + } + + if _, err := cb.Execute(context.Background(), first); err != nil { + t.Fatalf("first queue_run_for_each_participant: %v", err) + } + if _, err := cb.Execute(context.Background(), second); err != nil { + t.Fatalf("second queue_run_for_each_participant: %v", err) + } + if len(q.calls) != 2 { + t.Fatalf("expected 2 fan-out calls, got %d", len(q.calls)) + } + if q.calls[0].AgentProfileID != "same-agent" || q.calls[1].AgentProfileID != "same-agent" { + t.Fatalf("test setup expected both actions to target same agent: %#v", q.calls) + } + if q.calls[0].IdempotencyKey == q.calls[1].IdempotencyKey { + t.Fatalf("different for-each action configs must not share idempotency keys: %#v", q.calls) + } +} + func TestQueueRunForEachParticipantCallback_NoMatchingRole_NoCalls(t *testing.T) { q := &fakeRunQueue{} parts := fakeParticipants{list: []ParticipantInfo{ diff --git a/apps/backend/internal/workflow/repository/phase2_sqlite.go b/apps/backend/internal/workflow/repository/phase2_sqlite.go index 1be3e67606..cb7271fb82 100644 --- a/apps/backend/internal/workflow/repository/phase2_sqlite.go +++ b/apps/backend/internal/workflow/repository/phase2_sqlite.go @@ -39,6 +39,7 @@ func (r *Repository) initPhase2Schema() error { CREATE INDEX IF NOT EXISTS idx_workflow_step_participants_step ON workflow_step_participants(step_id); CREATE INDEX IF NOT EXISTS idx_workflow_step_participants_role ON workflow_step_participants(step_id, role); CREATE INDEX IF NOT EXISTS idx_workflow_step_participants_task ON workflow_step_participants(task_id) WHERE task_id != ''; + CREATE INDEX IF NOT EXISTS idx_workflow_step_participants_task_role ON workflow_step_participants(task_id, role) WHERE task_id != ''; ` if _, err := r.db.Exec(participantsSchema); err != nil { return fmt.Errorf("failed to create workflow_step_participants table: %w", err) @@ -333,6 +334,10 @@ func (r *Repository) GetStepParticipant(ctx context.Context, id string) (*models // wins; multiple rows fall back to the lowest-position id-tiebroken // pick (ambiguity, but deterministic). // 2. The step's `agent_profile_id` (the workflow's primary). +// 3. The most recently assigned runner participant for the task. This +// preserves the task's effective runner after terminal transitions +// to steps that do not carry their own participant rows, such as the +// default Done step. // // Returns "" without error when neither exists. Empty step_id or // task_id is an error — callers must supply both. @@ -371,9 +376,48 @@ func (r *Repository) ResolveCurrentRunner( return "", fmt.Errorf("lookup step primary: %w", err) } if primary.Valid { - return primary.String, nil + if primary.String != "" { + return primary.String, nil + } } - return "", nil + err = r.ro.QueryRowContext(ctx, r.ro.Rebind(` + SELECT agent_profile_id FROM workflow_step_participants + WHERE task_id = ? AND role = 'runner' + ORDER BY rowid DESC + LIMIT 1 + `), taskID).Scan(&agentID) + if err == nil { + if agentID != "" { + return agentID, nil + } + return "", nil + } + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", fmt.Errorf("lookup task runner participant: %w", err) +} + +// GetTaskWorkflowStepID returns the task's current workflow step. Cross-task +// queue_run primary resolution uses this before resolving the target task's +// current runner. +func (r *Repository) GetTaskWorkflowStepID(ctx context.Context, taskID string) (string, error) { + if taskID == "" { + return "", errors.New("task_id is required") + } + var stepID string + err := r.ro.QueryRowContext(ctx, r.ro.Rebind(` + SELECT COALESCE(workflow_step_id, '') + FROM tasks + WHERE id = ? + `), taskID).Scan(&stepID) + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("lookup task workflow step: %w", err) + } + return stepID, nil } // SetTaskRunner writes (or replaces) the runner participant for diff --git a/apps/backend/internal/workflow/repository/phase2_sqlite_test.go b/apps/backend/internal/workflow/repository/phase2_sqlite_test.go index acf86a68c4..f510ed67f8 100644 --- a/apps/backend/internal/workflow/repository/phase2_sqlite_test.go +++ b/apps/backend/internal/workflow/repository/phase2_sqlite_test.go @@ -450,6 +450,98 @@ func TestResolveCurrentRunner_PrefersRunnerParticipant(t *testing.T) { } } +func TestResolveCurrentRunner_FallsBackToTaskRunnerOnOtherStep(t *testing.T) { + repo := setupTestRepo(t) + ctx := context.Background() + work := newPhase2TestStep(t, repo, "Work") + done := newPhase2TestStep(t, repo, "Done") + + if err := repo.SetTaskRunner(ctx, work.ID, "task-done", "runner-agent"); err != nil { + t.Fatalf("set runner: %v", err) + } + got, err := repo.ResolveCurrentRunner(ctx, done.ID, "task-done") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got != "runner-agent" { + t.Fatalf("expected runner-agent, got %q", got) + } +} + +func TestResolveCurrentRunner_FallsBackToLatestTaskRunner(t *testing.T) { + repo := setupTestRepo(t) + ctx := context.Background() + work := newPhase2TestStep(t, repo, "Work") + review := newPhase2TestStep(t, repo, "Review") + done := newPhase2TestStep(t, repo, "Done") + + if err := repo.SetTaskRunner(ctx, work.ID, "task-done", "runner-on-work"); err != nil { + t.Fatalf("set work runner: %v", err) + } + if err := repo.SetTaskRunner(ctx, review.ID, "task-done", "runner-on-review"); err != nil { + t.Fatalf("set review runner: %v", err) + } + got, err := repo.ResolveCurrentRunner(ctx, done.ID, "task-done") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got != "runner-on-review" { + t.Fatalf("expected runner-on-review, got %q", got) + } +} + +func TestResolveCurrentRunner_TreatsEmptyTaskRunnerAsMissing(t *testing.T) { + repo := setupTestRepo(t) + ctx := context.Background() + work := newPhase2TestStep(t, repo, "Work") + done := newPhase2TestStep(t, repo, "Done") + + _, err := repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position) + VALUES (?, ?, ?, 'runner', '', 0, 0) + `), "empty-runner", work.ID, "task-done") + if err != nil { + t.Fatalf("insert empty runner: %v", err) + } + got, err := repo.ResolveCurrentRunner(ctx, done.ID, "task-done") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got != "" { + t.Fatalf("expected no runner, got %q", got) + } +} + +func TestResolveCurrentRunner_PrefersStepPrimaryOverOtherStepRunner(t *testing.T) { + repo := setupTestRepo(t) + ctx := context.Background() + work := &models.WorkflowStep{ + WorkflowID: "wf-test", Name: "Work", Position: 0, + AgentProfileID: "primary-on-work", + } + done := &models.WorkflowStep{ + WorkflowID: "wf-test", Name: "Done", Position: 1, + AgentProfileID: "primary-on-done", + } + if err := repo.CreateStep(ctx, work); err != nil { + t.Fatalf("create work step: %v", err) + } + if err := repo.CreateStep(ctx, done); err != nil { + t.Fatalf("create done step: %v", err) + } + if err := repo.SetTaskRunner(ctx, work.ID, "task-done", "runner-on-work"); err != nil { + t.Fatalf("set runner: %v", err) + } + got, err := repo.ResolveCurrentRunner(ctx, done.ID, "task-done") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got != "primary-on-done" { + t.Fatalf("expected primary-on-done, got %q", got) + } +} + // TestSetTaskRunner_Idempotent verifies SetTaskRunner replaces an // existing runner participant rather than creating a second row. func TestSetTaskRunner_Idempotent(t *testing.T) { diff --git a/apps/web/components/task/dockview-desktop-layout.tsx b/apps/web/components/task/dockview-desktop-layout.tsx index c929e323ae..d3d97caeaf 100644 --- a/apps/web/components/task/dockview-desktop-layout.tsx +++ b/apps/web/components/task/dockview-desktop-layout.tsx @@ -302,6 +302,32 @@ function setupReadyDockview({ api, appStore, layout, refs }: ReadyDockviewSetup) refs.readyDisposersRef.current.push(setupSashDragCapToggle(api)); } +type DockviewMainAreaProps = { + effectiveSessionId: string | null; + hasDevScript: boolean; + onReady: (event: DockviewReadyEvent) => void; +}; + +function DockviewMainArea({ effectiveSessionId, hasDevScript, onReady }: DockviewMainAreaProps) { + return ( +
+ + +
+ ); +} + export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ sessionId, repository, @@ -354,11 +380,9 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ // panel could leak into the old session's persisted layout. useEnvSwitchCleanup(effectiveSessionId, effectiveEnvId, activeTaskId); - // Auto-create a session tab when a session becomes active useAutoSessionTab(effectiveSessionId); useChangesPanelAutoFocus(changesFocusKey); - // Auto-show PR detail panel when the task has an associated PR useAutoPRPanel(); useDockviewUnmountCleanup(saveTimerRef, readyDisposersRef); @@ -377,21 +401,11 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ transition: isRestoringLayout ? "none" : "opacity 60ms ease-out", }} > -
- - -
+ diff --git a/apps/web/components/task/simple/OfficeSimplePane.test.tsx b/apps/web/components/task/simple/OfficeSimplePane.test.tsx new file mode 100644 index 0000000000..60862c453d --- /dev/null +++ b/apps/web/components/task/simple/OfficeSimplePane.test.tsx @@ -0,0 +1,178 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { StateProvider } from "@/components/state-provider"; +import type { Task, TaskSession } from "@/app/office/tasks/[id]/types"; + +const { CHAT_EDITABLE, CHAT_READONLY, CHAT_READONLY_TEST_ID } = vi.hoisted(() => ({ + CHAT_EDITABLE: "editable", + CHAT_READONLY: "readonly", + CHAT_READONLY_TEST_ID: "chat-readonly", +})); + +vi.mock("@/components/routing/app-link", () => ({ + default: ({ href, children, ...props }: { href: string; children: ReactNode }) => ( + + {children} + + ), +})); + +vi.mock("@/app/office/components/office-topbar-portal", () => ({ + OfficeTopbarPortal: ({ children }: { children: ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, +})); + +vi.mock("@/app/office/components/execution-indicator", () => ({ + ExecutionIndicator: () =>
, +})); + +vi.mock("./components/topbar-working-indicator", () => ({ + TopbarWorkingIndicator: () =>
, +})); + +vi.mock("./task-properties", () => ({ + TaskProperties: () =>