diff --git a/apps/backend/internal/backendapp/message_queue_settings_test.go b/apps/backend/internal/backendapp/message_queue_settings_test.go index 433a3a85fd..04088adf6c 100644 --- a/apps/backend/internal/backendapp/message_queue_settings_test.go +++ b/apps/backend/internal/backendapp/message_queue_settings_test.go @@ -32,7 +32,9 @@ func TestResolveQueueMaxPerSessionPrecedence(t *testing.T) { t.Fatalf("new system settings store: %v", err) } store := queuesettings.NewStore(raw) - if err := store.Save(context.Background(), queuesettings.Settings{MaxPerSession: tc.persisted}); err != nil { + if err := store.Save(context.Background(), queuesettings.Settings{ + MaxPerSession: tc.persisted, MergeEnabled: true, AutoMergeEnabled: true, + }); err != nil { t.Fatalf("save setting: %v", err) } t.Setenv(queuesettings.EnvironmentVariable, tc.env) @@ -64,7 +66,8 @@ func TestResolveQueueMergeEnabledPrecedence(t *testing.T) { } store := queuesettings.NewStore(raw) if err := store.Save(context.Background(), queuesettings.Settings{ - MaxPerSession: queuesettings.DefaultMaxPerSession, MergeEnabled: *tc.persisted, + MaxPerSession: queuesettings.DefaultMaxPerSession, + MergeEnabled: *tc.persisted, AutoMergeEnabled: true, }); err != nil { t.Fatalf("save setting: %v", err) } @@ -86,6 +89,47 @@ func TestResolveQueueMergeEnabledWithoutPool(t *testing.T) { } } +func TestResolveQueueAutoMergeEnabledPrecedence(t *testing.T) { + tests := []struct { + name string + persisted *bool + env string + want bool + }{ + {name: "no persisted setting defaults to enabled", want: true}, + {name: "persisted disabled", persisted: new(false), want: false}, + {name: "persisted enabled", persisted: new(true), want: true}, + {name: "capacity environment does not override", persisted: new(false), env: "20", want: false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + pool := newMessageQueueSettingsTestPool(t) + if test.persisted != nil { + raw, err := systemsettings.NewStore(pool) + if err != nil { + t.Fatalf("new system settings store: %v", err) + } + if err := queuesettings.NewStore(raw).Save(context.Background(), queuesettings.Settings{ + MaxPerSession: queuesettings.DefaultMaxPerSession, + MergeEnabled: true, AutoMergeEnabled: *test.persisted, + }); err != nil { + t.Fatalf("save setting: %v", err) + } + } + t.Setenv(queuesettings.EnvironmentVariable, test.env) + if got := resolveQueueAutoMergeEnabled(pool, testLogger(t)); got != test.want { + t.Fatalf("automatic merge enabled = %v, want %v", got, test.want) + } + }) + } +} + +func TestResolveQueueAutoMergeEnabledWithoutPool(t *testing.T) { + if got := resolveQueueAutoMergeEnabled(nil, testLogger(t)); !got { + t.Fatalf("automatic merge enabled = %v, want true", got) + } +} + func newMessageQueueSettingsTestPool(t *testing.T) *db.Pool { t.Helper() connection, err := sqlx.Open("sqlite3", ":memory:") diff --git a/apps/backend/internal/backendapp/orchestrator.go b/apps/backend/internal/backendapp/orchestrator.go index 3444966bd9..e2483b6831 100644 --- a/apps/backend/internal/backendapp/orchestrator.go +++ b/apps/backend/internal/backendapp/orchestrator.go @@ -97,13 +97,17 @@ func provideOrchestrator( if err != nil { return nil, nil, fmt.Errorf("init message queue repo: %w", err) } - maxPerSession := resolveQueueMaxPerSession(pool, log) - mergeEnabled := resolveQueueMergeEnabled(pool, log) + queueSettings := resolveQueueSettings(pool, log).Effective + maxPerSession := queueSettings.MaxPerSession + mergeEnabled := queueSettings.MergeEnabled + autoMergeEnabled := queueSettings.AutoMergeEnabled msgQueue := messagequeue.NewService(queueRepo, maxPerSession, log) msgQueue.SetMergeEnabled(mergeEnabled) + msgQueue.SetAutoMergeEnabled(autoMergeEnabled) log.Info("Message queue initialized", zap.Int("max_per_session", maxPerSession), - zap.Bool("merge_enabled", mergeEnabled)) + zap.Bool("merge_enabled", mergeEnabled), + zap.Bool("auto_merge_enabled", autoMergeEnabled)) if taskSvc.AttachmentService() == nil && taskSvc.AttachmentRepository() != nil { attachmentSvc, attachmentErr := taskservice.NewAttachmentService( taskSvc.AttachmentRepository(), cfg.ResolvedHomeDir(), taskSvc.AuthorizeWorkspaceAccess, log, @@ -276,6 +280,12 @@ func resolveQueueMergeEnabled(pool *db.Pool, log *logger.Logger) bool { return resolveQueueSettings(pool, log).Effective.MergeEnabled } +// resolveQueueAutoMergeEnabled honors the persisted automatic merge setting, +// defaulting to enabled when unset or invalid. +func resolveQueueAutoMergeEnabled(pool *db.Pool, log *logger.Logger) bool { + return resolveQueueSettings(pool, log).Effective.AutoMergeEnabled +} + // resolveQueueSettings loads the persisted message queue settings — falling // back to defaults when unset, invalid, or the store is unavailable — and // resolves them against the KANDEV_QUEUE_MAX_PER_SESSION environment @@ -300,8 +310,9 @@ func resolveQueueSettings(pool *db.Pool, log *logger.Logger) queuesettings.Resol return queuesettings.Resolution{Response: queuesettings.Response{ Settings: queuesettings.DefaultSettings(), Effective: queuesettings.Effective{ - MaxPerSession: messagequeue.DefaultMaxPerSession, - MergeEnabled: true, + MaxPerSession: messagequeue.DefaultMaxPerSession, + MergeEnabled: true, + AutoMergeEnabled: true, }, }} } diff --git a/apps/backend/internal/messageconstraints/attachments.go b/apps/backend/internal/messageconstraints/attachments.go index e4a70aa2ac..e43b085152 100644 --- a/apps/backend/internal/messageconstraints/attachments.go +++ b/apps/backend/internal/messageconstraints/attachments.go @@ -10,4 +10,7 @@ const ( // MaxAttachmentCount is the maximum number of attachments accepted for one // message. MaxAttachmentCount = 10 + // MaxContextFilesPerMessage is the maximum number of context-file + // descriptors retained when compatible queued messages are folded. + MaxContextFilesPerMessage = 200 ) diff --git a/apps/backend/internal/orchestrator/handlers/queue_handlers.go b/apps/backend/internal/orchestrator/handlers/queue_handlers.go index 88a9307b54..1213b42d8a 100644 --- a/apps/backend/internal/orchestrator/handlers/queue_handlers.go +++ b/apps/backend/internal/orchestrator/handlers/queue_handlers.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "errors" + "fmt" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/entityrefs" @@ -54,6 +55,7 @@ const ( // in messagequeue.Service. type QueueService interface { QueueMessageWithMetadata(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []messagequeue.MessageAttachment, metadata map[string]interface{}) (*messagequeue.QueuedMessage, error) + QueueMessageWithMetadataAfterInsert(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []messagequeue.MessageAttachment, metadata map[string]interface{}, afterInsert func(context.Context, *messagequeue.QueuedMessage) error) (*messagequeue.QueuedMessage, error) AppendContent(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []messagequeue.MessageAttachment) (*messagequeue.QueuedMessage, bool, error) GetEntry(ctx context.Context, sessionID, entryID string) (*messagequeue.QueuedMessage, error) UpdateMessageWithMetadata(ctx context.Context, sessionID, entryID, content string, attachments []messagequeue.MessageAttachment, metadataUpdates map[string]interface{}, queuedBy string) error @@ -225,7 +227,7 @@ func (h *QueueHandlers) wsQueueMessage(ctx context.Context, msg *ws.Message) (*w WithContextFiles(req.ContextFiles). WithEntityReferences(req.EntityReferences). ToMap() - queued, err := h.queueService.QueueMessageWithMetadata(ctx, req.SessionID, req.TaskID, req.Content, req.Model, queuedBy, req.PlanMode, req.Attachments, metadata) + queued, err := h.admitQueuedMessage(ctx, &req, queuedBy, metadata) if err != nil { if errors.Is(err, messagequeue.ErrQueueFull) { status := h.queueService.GetStatus(ctx, req.SessionID) @@ -235,22 +237,49 @@ func (h *QueueHandlers) wsQueueMessage(ctx context.Context, msg *ws.Message) (*w fieldMax: status.Max, }) } - h.logger.Error("failed to queue message", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to queue message", nil) - } - if h.attachmentClaimer != nil && len(req.Attachments) > 0 { - if err := h.attachmentClaimer.ClaimMessageAttachments(ctx, req.TaskID, req.SessionID, queueAttachmentsToV1(req.Attachments)); err != nil { - if rollbackErr := h.rollbackQueuedAttachmentClaim(ctx, req.SessionID, queued.ID); rollbackErr != nil { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to roll back queued attachment", nil) - } + if errors.Is(err, errQueuedAttachmentRollback) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to roll back queued attachment", nil) + } + if errors.Is(err, errQueuedAttachmentUnavailable) { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "Attachment is no longer available", nil) } + h.logger.Error("failed to queue message", zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to queue message", nil) } h.publishStatus(ctx, req.SessionID) return ws.NewResponse(msg.ID, msg.Action, queued) } +var ( + errQueuedAttachmentUnavailable = errors.New("queued attachment unavailable") + errQueuedAttachmentRollback = errors.New("queued attachment rollback failed") +) + +func (h *QueueHandlers) admitQueuedMessage(ctx context.Context, req *wsQueueMessageRequest, queuedBy string, metadata map[string]interface{}) (*messagequeue.QueuedMessage, error) { + if h.attachmentClaimer == nil || len(req.Attachments) == 0 { + return h.queueService.QueueMessageWithMetadata( + ctx, req.SessionID, req.TaskID, req.Content, req.Model, queuedBy, req.PlanMode, req.Attachments, metadata, + ) + } + return h.queueService.QueueMessageWithMetadataAfterInsert( + ctx, req.SessionID, req.TaskID, req.Content, req.Model, queuedBy, req.PlanMode, req.Attachments, metadata, + func(admittedCtx context.Context, source *messagequeue.QueuedMessage) error { + claimErr := h.attachmentClaimer.ClaimMessageAttachments( + admittedCtx, req.TaskID, req.SessionID, queueAttachmentsToV1(req.Attachments), + ) + if claimErr == nil { + return nil + } + if rollbackErr := h.rollbackQueuedAttachmentClaim(admittedCtx, req.SessionID, source.ID); rollbackErr != nil { + h.logger.Error("failed to roll back queued attachment", zap.Error(rollbackErr)) + return fmt.Errorf("%w: %v", errQueuedAttachmentRollback, rollbackErr) + } + return fmt.Errorf("%w: %v", errQueuedAttachmentUnavailable, claimErr) + }, + ) +} + type wsCancelAllRequest struct { SessionID string `json:"session_id"` } diff --git a/apps/backend/internal/orchestrator/handlers/queue_handlers_auto_merge_test.go b/apps/backend/internal/orchestrator/handlers/queue_handlers_auto_merge_test.go new file mode 100644 index 0000000000..251405dabb --- /dev/null +++ b/apps/backend/internal/orchestrator/handlers/queue_handlers_auto_merge_test.go @@ -0,0 +1,115 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/orchestrator/messagequeue" + v1 "github.com/kandev/kandev/pkg/api/v1" + ws "github.com/kandev/kandev/pkg/websocket" + "github.com/stretchr/testify/require" +) + +func TestWsQueueMessage_AutoMergeClaimsAttachmentBeforeFold(t *testing.T) { + handlers, service := setupQueueHandlers(t) + service.SetAutoMergeEnabled(true) + claimer := &recordingQueueAttachmentClaimer{} + handlers.SetAttachmentClaimer(claimer) + target, err := service.QueueMessage(context.Background(), "session", "task", "first", "", messagequeue.QueuedByUser, false, nil) + require.NoError(t, err) + + response, err := handlers.wsQueueMessage(context.Background(), autoMergeQueueRequest(t, "second", "source-file")) + require.NoError(t, err) + require.Equal(t, ws.MessageTypeResponse, response.Type) + var queued messagequeue.QueuedMessage + require.NoError(t, json.Unmarshal(response.Payload, &queued)) + require.Equal(t, target.ID, queued.ID) + require.Equal(t, "first\n\nsecond", queued.Content) + require.Equal(t, []string{"source-file"}, claimer.claims) + status := service.GetStatus(context.Background(), "session") + require.Equal(t, 1, status.Count) + require.Equal(t, "source-file", status.Entries[0].Attachments[0].AttachmentID) +} + +func TestWsQueueMessage_AutoMergeClaimFailureRollsBackOnlySource(t *testing.T) { + handlers, service := setupQueueHandlers(t) + service.SetAutoMergeEnabled(true) + claimer := &recordingQueueAttachmentClaimer{claimErr: errors.New("staged file expired")} + handlers.SetAttachmentClaimer(claimer) + target, err := service.QueueMessage(context.Background(), "session", "task", "first", "", messagequeue.QueuedByUser, false, nil) + require.NoError(t, err) + before, err := json.Marshal(target) + require.NoError(t, err) + + response, err := handlers.wsQueueMessage(context.Background(), autoMergeQueueRequest(t, "second", "expired-file")) + require.NoError(t, err) + require.Equal(t, ws.MessageTypeError, response.Type) + require.Equal(t, ws.ErrorCodeValidation, parseError(t, response).Code) + status := service.GetStatus(context.Background(), "session") + require.Equal(t, 1, status.Count) + after, err := json.Marshal(status.Entries[0]) + require.NoError(t, err) + require.JSONEq(t, string(before), string(after)) +} + +func TestWsQueueMessage_AutoMergeBlocksLaterAdmissionUntilClaimFinalizes(t *testing.T) { + handlers, service := setupQueueHandlers(t) + service.SetAutoMergeEnabled(true) + claimer := &blockingQueueAttachmentClaimer{started: make(chan struct{}), release: make(chan struct{})} + handlers.SetAttachmentClaimer(claimer) + _, err := service.QueueMessage(context.Background(), "session", "task", "first", "", messagequeue.QueuedByUser, false, nil) + require.NoError(t, err) + + handlerDone := make(chan error, 1) + go func() { + response, callErr := handlers.wsQueueMessage(context.Background(), autoMergeQueueRequest(t, "second", "source-file")) + if callErr == nil && response.Type != ws.MessageTypeResponse { + callErr = errors.New("queue handler returned an error response") + } + handlerDone <- callErr + }() + <-claimer.started + + laterDone := make(chan error, 1) + go func() { + _, queueErr := service.QueueMessage(context.Background(), "session", "task", "third", "", messagequeue.QueuedByUser, false, nil) + laterDone <- queueErr + }() + select { + case err := <-laterDone: + t.Fatalf("later admission bypassed attachment claim: %v", err) + case <-time.After(50 * time.Millisecond): + } + close(claimer.release) + require.NoError(t, <-handlerDone) + require.NoError(t, <-laterDone) + status := service.GetStatus(context.Background(), "session") + require.Equal(t, 1, status.Count) + require.Equal(t, "first\n\nsecond\n\nthird", status.Entries[0].Content) +} + +type blockingQueueAttachmentClaimer struct { + started chan struct{} + release chan struct{} +} + +func (c *blockingQueueAttachmentClaimer) ClaimMessageAttachments(context.Context, string, string, []v1.MessageAttachment) error { + close(c.started) + <-c.release + return nil +} + +func autoMergeQueueRequest(t *testing.T, content, attachmentID string) *ws.Message { + t.Helper() + return createTestMessage(t, ws.ActionMessageQueueAdd, map[string]interface{}{ + "session_id": "session", + "task_id": "task", + "content": content, + "attachments": []messagequeue.MessageAttachment{{ + Type: "resource", AttachmentID: attachmentID, Name: attachmentID + ".txt", MimeType: "text/plain", SizeBytes: 1, + }}, + }) +} diff --git a/apps/backend/internal/orchestrator/handlers/queue_handlers_test.go b/apps/backend/internal/orchestrator/handlers/queue_handlers_test.go index 115e0f7e78..c237f8298d 100644 --- a/apps/backend/internal/orchestrator/handlers/queue_handlers_test.go +++ b/apps/backend/internal/orchestrator/handlers/queue_handlers_test.go @@ -134,6 +134,7 @@ func setupQueueHandlersWithDrainer(t *testing.T, drainer QueueDrainer) (*QueueHa }) require.NoError(t, err) svc := messagequeue.NewServiceMemory(log) + svc.SetAutoMergeEnabled(false) return NewQueueHandlers(svc, &mockEventBus{}, log, drainer, allowQueueAccess{}, nil), svc } @@ -146,6 +147,7 @@ func setupQueueHandlersWithValidator(t *testing.T, validator entityrefs.Submissi }) require.NoError(t, err) svc := messagequeue.NewServiceMemory(log) + svc.SetAutoMergeEnabled(false) return NewQueueHandlers(svc, &mockEventBus{}, log, nil, allowQueueAccess{}, nil, validator), svc } @@ -200,6 +202,7 @@ func TestQueueHandlersDenyUnauthorizedSessionActions(t *testing.T) { log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) require.NoError(t, err) svc := messagequeue.NewServiceMemory(log) + svc.SetAutoMergeEnabled(false) _, err = svc.QueueMessage(context.Background(), "s", "t", "first", "", messagequeue.QueuedByUser, false, nil) require.NoError(t, err) second, err := svc.QueueMessage(context.Background(), "s", "t", "second", "", messagequeue.QueuedByUser, false, nil) diff --git a/apps/backend/internal/orchestrator/messagequeue/auto_merge.go b/apps/backend/internal/orchestrator/messagequeue/auto_merge.go new file mode 100644 index 0000000000..068f531551 --- /dev/null +++ b/apps/backend/internal/orchestrator/messagequeue/auto_merge.go @@ -0,0 +1,194 @@ +package messagequeue + +import ( + "bytes" + "encoding/json" + + "github.com/kandev/kandev/internal/entityrefs" + "github.com/kandev/kandev/internal/messageconstraints" + apiv1 "github.com/kandev/kandev/pkg/api/v1" +) + +type autoMergedValues struct { + content string + attachments []MessageAttachment + metadata map[string]interface{} +} + +func buildAutoMergedEntry(target, source *QueuedMessage) (*autoMergedValues, bool) { + if !autoMergeAllowed(target, source) || !autoMetadataEquivalent(target.Metadata, source.Metadata) { + return nil, false + } + attachments, ok := combineAutoMergeAttachments(target.Attachments, source.Attachments) + if !ok { + return nil, false + } + metadata, ok := mergeAutoMetadata(target.Metadata, source.Metadata) + if !ok { + return nil, false + } + return &autoMergedValues{ + content: joinAutoMergeContent(target.Content, source.Content), + attachments: attachments, + metadata: metadata, + }, true +} + +func autoMergeAllowed(target, source *QueuedMessage) bool { + if target == nil || source == nil || target.IsDurableLifecycle() || source.IsDurableLifecycle() { + return false + } + if target.TaskID != source.TaskID || target.Model != source.Model || target.PlanMode != source.PlanMode { + return false + } + if source.QueuedBy == QueuedByAgent { + sourceSenderTaskID := metadataString(source.Metadata, MetadataSenderTaskID) + return target.QueuedBy == QueuedByAgent && + sourceSenderTaskID != "" && + sourceSenderTaskID == metadataString(target.Metadata, MetadataSenderTaskID) + } + return source.QueuedBy != "" && source.QueuedBy == target.QueuedBy && !IsReservedQueuedBy(source.QueuedBy) +} + +func joinAutoMergeContent(target, source string) string { + if target == "" { + return source + } + if source == "" { + return target + } + return target + "\n\n" + source +} + +func autoMetadataEquivalent(target, source map[string]interface{}) bool { + targetJSON, targetOK := comparableAutoMetadata(target) + sourceJSON, sourceOK := comparableAutoMetadata(source) + return targetOK && sourceOK && bytes.Equal(targetJSON, sourceJSON) +} + +func comparableAutoMetadata(metadata map[string]interface{}) ([]byte, bool) { + comparable := make(map[string]interface{}, len(metadata)) + for key, value := range metadata { + if key == MetadataEntityReferences || key == MetadataContextFiles { + continue + } + comparable[key] = value + } + encoded, err := json.Marshal(comparable) + return encoded, err == nil +} + +func combineAutoMergeAttachments(target, source []MessageAttachment) ([]MessageAttachment, bool) { + if len(target)+len(source) > messageconstraints.MaxAttachmentCount { + return nil, false + } + combined := append(append([]MessageAttachment(nil), target...), source...) + var total int64 + for _, attachment := range combined { + payloadBytes, err := attachmentPayloadBytes(attachment) + if err != nil || payloadBytes > messageconstraints.MaxAttachmentBytes-total { + return nil, false + } + total += payloadBytes + } + return combined, true +} + +func mergeAutoMetadata(target, source map[string]interface{}) (map[string]interface{}, bool) { + merged := copyMessageMetadata(target, 0) + references, ok := unionAutoEntityReferences(target, source) + if !ok { + return nil, false + } + contexts, ok := unionAutoContextFiles(target, source) + if !ok { + return nil, false + } + merged = setAutoMetadataList(merged, MetadataEntityReferences, references) + merged = setAutoMetadataList(merged, MetadataContextFiles, contexts) + return merged, true +} + +func setAutoMetadataList(metadata map[string]interface{}, key string, value interface{}) map[string]interface{} { + encoded, _ := json.Marshal(value) + if string(encoded) == "null" || string(encoded) == "[]" { + delete(metadata, key) + } else { + if metadata == nil { + metadata = make(map[string]interface{}, 1) + } + metadata[key] = value + } + return metadata +} + +func unionAutoEntityReferences(target, source map[string]interface{}) ([]apiv1.EntityReference, bool) { + var union []apiv1.EntityReference + seen := make(map[string]struct{}) + for _, metadata := range []map[string]interface{}{target, source} { + references, ok := normalizeAutoEntityReferences(metadata[MetadataEntityReferences]) + if !ok { + return nil, false + } + for _, reference := range references { + if _, duplicate := seen[reference.Ref]; duplicate { + continue + } + seen[reference.Ref] = struct{}{} + union = append(union, reference) + } + } + return union, len(union) <= entityrefs.MaxReferencesPerMessage +} + +func normalizeAutoEntityReferences(raw interface{}) ([]apiv1.EntityReference, bool) { + if raw == nil { + return nil, true + } + encoded, err := json.Marshal(raw) + if err != nil { + return nil, false + } + var references []apiv1.EntityReference + if err := json.Unmarshal(encoded, &references); err != nil { + return nil, false + } + normalized, err := entityrefs.NormalizeForSubmission(references) + return normalized, err == nil +} + +func unionAutoContextFiles(target, source map[string]interface{}) ([]apiv1.ContextFileMeta, bool) { + var union []apiv1.ContextFileMeta + seen := make(map[string]struct{}) + for _, metadata := range []map[string]interface{}{target, source} { + contexts, ok := normalizeAutoContextFiles(metadata[MetadataContextFiles]) + if !ok { + return nil, false + } + for _, contextFile := range contexts { + encoded, _ := json.Marshal(contextFile) + key := string(encoded) + if _, duplicate := seen[key]; duplicate { + continue + } + seen[key] = struct{}{} + union = append(union, contextFile) + } + } + return union, len(union) <= messageconstraints.MaxContextFilesPerMessage +} + +func normalizeAutoContextFiles(raw interface{}) ([]apiv1.ContextFileMeta, bool) { + if raw == nil { + return nil, true + } + encoded, err := json.Marshal(raw) + if err != nil { + return nil, false + } + var contexts []apiv1.ContextFileMeta + if err := json.Unmarshal(encoded, &contexts); err != nil { + return nil, false + } + return contexts, true +} diff --git a/apps/backend/internal/orchestrator/messagequeue/repository.go b/apps/backend/internal/orchestrator/messagequeue/repository.go index de618a8e3d..ada0d2745e 100644 --- a/apps/backend/internal/orchestrator/messagequeue/repository.go +++ b/apps/backend/internal/orchestrator/messagequeue/repository.go @@ -116,6 +116,12 @@ type Repository interface { // missing source returns ErrEntryNotFound. MergeIntoAbove(ctx context.Context, sessionID, sourceID, queuedBy string) (*QueuedMessage, error) + // AutoMergeIntoAbove attempts to fold the exact source entry into its + // immediate predecessor. A compatible merge returns the surviving target + // and true. A missing source, missing predecessor, or incompatible pair is a + // successful skip and returns false without mutating either row. + AutoMergeIntoAbove(ctx context.Context, sessionID, sourceID string) (*QueuedMessage, bool, error) + // ReorderEntries atomically rewrites the FIFO positions of the session's // pending entries to match orderedIDs. The submitted list must be an exact // permutation of the currently visible pending (not reserved-in-flight) diff --git a/apps/backend/internal/orchestrator/messagequeue/repository_auto_merge_test.go b/apps/backend/internal/orchestrator/messagequeue/repository_auto_merge_test.go new file mode 100644 index 0000000000..4361563f38 --- /dev/null +++ b/apps/backend/internal/orchestrator/messagequeue/repository_auto_merge_test.go @@ -0,0 +1,451 @@ +package messagequeue + +import ( + "context" + "encoding/json" + "fmt" + "reflect" + "testing" + "time" + + "github.com/kandev/kandev/internal/entityrefs" + "github.com/kandev/kandev/internal/messageconstraints" + apiv1 "github.com/kandev/kandev/pkg/api/v1" +) + +type automaticMergeRepository interface { + AutoMergeIntoAbove(context.Context, string, string) (*QueuedMessage, bool, error) +} + +type autoMergeRepositoryFactory struct { + name string + new func(*testing.T) Repository +} + +func autoMergeRepositoryFactories() []autoMergeRepositoryFactory { + return []autoMergeRepositoryFactory{ + {name: "memory", new: func(*testing.T) Repository { return NewMemoryRepository() }}, + {name: "sqlite", new: newTestSQLiteRepo}, + {name: "postgres", new: newTestPostgresRepo}, + } +} + +func TestRepository_AutoMergeIntoAbove_UserHappyPath(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + autoRepo, ok := repo.(automaticMergeRepository) + if !ok { + t.Fatalf("repository %T does not implement automatic tail merge", repo) + } + + target := insertTestEntry(t, repo, "session", "task", "first", QueuedByUser, nil, nil) + source := insertTestEntry(t, repo, "session", "task", "second", QueuedByUser, nil, nil) + + merged, didMerge, err := autoRepo.AutoMergeIntoAbove(context.Background(), "session", source.ID) + if err != nil { + t.Fatalf("auto merge: %v", err) + } + if !didMerge { + t.Fatal("auto merge skipped compatible entries") + } + if merged == nil || merged.ID != target.ID { + t.Fatalf("survivor = %+v, want target %s", merged, target.ID) + } + if merged.Content != "first\n\nsecond" { + t.Errorf("content = %q, want %q", merged.Content, "first\n\nsecond") + } + + entries, err := repo.ListBySession(context.Background(), "session") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(entries) != 1 || entries[0].ID != target.ID { + t.Fatalf("entries after merge = %+v, want surviving target", entries) + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_PreservesIdentityAndCombinesAdditiveData(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + autoRepo := repo.(automaticMergeRepository) + isDirectory := true + target := insertAutoMergeEntry(t, repo, QueuedMessage{ + SessionID: "session", TaskID: "task", Content: "first", Model: "model", PlanMode: true, QueuedBy: "alice", + Attachments: []MessageAttachment{{AttachmentID: "target-file", Name: "target.txt", SizeBytes: 6}}, + Metadata: map[string]interface{}{ + "route": map[string]interface{}{"attempt": 1, "tags": []string{"one"}}, + MetadataEntityReferences: entityRefs("1"), + MetadataContextFiles: []apiv1.ContextFileMeta{{Path: "a.go", Name: "a.go"}}, + }, + }) + source := insertAutoMergeEntry(t, repo, QueuedMessage{ + SessionID: "session", TaskID: "task", Content: "second", Model: "model", PlanMode: true, QueuedBy: "alice", + Attachments: []MessageAttachment{{Data: "c291cmNl", Name: "source.txt"}}, + Metadata: map[string]interface{}{ + "route": map[string]interface{}{"attempt": float64(1), "tags": []interface{}{"one"}}, + MetadataEntityReferences: entityRefs("1", "2"), + MetadataContextFiles: []interface{}{ + map[string]interface{}{"path": "a.go", "name": "a.go"}, + map[string]interface{}{"path": "dir", "name": "dir", "is_directory": isDirectory}, + }, + }, + }) + targetIdentity := autoMergeIdentity(*target) + + merged, didMerge, err := autoRepo.AutoMergeIntoAbove(context.Background(), "session", source.ID) + if err != nil || !didMerge { + t.Fatalf("auto merge: merged=%v err=%v", didMerge, err) + } + if got := autoMergeIdentity(*merged); !reflect.DeepEqual(got, targetIdentity) { + t.Errorf("survivor identity changed: got %+v want %+v", got, targetIdentity) + } + if merged.Content != "first\n\nsecond" { + t.Errorf("content = %q", merged.Content) + } + if got := attachmentNames(merged.Attachments); !reflect.DeepEqual(got, []string{"target.txt", "source.txt"}) { + t.Errorf("attachment order = %v", got) + } + if got := referenceIDs(merged.Metadata); !reflect.DeepEqual(got, []string{"1", "2"}) { + t.Errorf("reference order = %v", got) + } + contexts, ok := normalizeAutoContextFiles(merged.Metadata[MetadataContextFiles]) + if !ok || len(contexts) != 2 || contexts[0].Path != "a.go" || contexts[1].Path != "dir" { + t.Errorf("context union = %+v, valid=%v", contexts, ok) + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_AgentSameSender(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + targetMessage := defaultAutoMergeEntry("agent", "first") + targetMessage.QueuedBy = QueuedByAgent + targetMessage.Metadata = senderMetadata("sender-task") + target := insertAutoMergeEntry(t, repo, targetMessage) + sourceMessage := defaultAutoMergeEntry("agent", "second") + sourceMessage.QueuedBy = QueuedByAgent + sourceMessage.Metadata = senderMetadata("sender-task") + source := insertAutoMergeEntry(t, repo, sourceMessage) + + merged, didMerge, err := repo.(automaticMergeRepository).AutoMergeIntoAbove(context.Background(), "agent", source.ID) + if err != nil || !didMerge || merged == nil { + t.Fatalf("agent auto merge: result=%+v merged=%v err=%v", merged, didMerge, err) + } + if merged.ID != target.ID || metadataString(merged.Metadata, MetadataSenderTaskID) != "sender-task" { + t.Fatalf("agent survivor = %+v, want target identity and sender", merged) + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_IncompatibilityLeavesRowsUntouched(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + for _, test := range autoMergeSkipCases() { + t.Run(test.name, func(t *testing.T) { + runAutoMergeSkipCase(t, repo, tt.name+"-"+test.name, test.mutate) + }) + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_UsesOnlyImmediatePredecessor(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + autoRepo := repo.(automaticMergeRepository) + insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("immediate", "compatible-before")) + blocker := defaultAutoMergeEntry("immediate", "blocker") + blocker.QueuedBy = "bob" + insertAutoMergeEntry(t, repo, blocker) + source := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("immediate", "source")) + before := autoQueueSnapshot(t, repo, "immediate") + + result, merged, err := autoRepo.AutoMergeIntoAbove(context.Background(), "immediate", source.ID) + if err != nil || merged { + t.Fatalf("immediate-tail skip: merged=%v err=%v", merged, err) + } + if result == nil || result.ID != source.ID { + t.Fatalf("skip result = %+v, want source", result) + } + if after := autoQueueSnapshot(t, repo, "immediate"); after != before { + t.Fatalf("queue changed on skip\nbefore=%s\nafter=%s", before, after) + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_MissingAndHeadAreSafeSkips(t *testing.T) { + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + autoRepo := repo.(automaticMergeRepository) + head := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("missing", "head")) + result, merged, err := autoRepo.AutoMergeIntoAbove(context.Background(), "missing", head.ID) + if err != nil || merged || result == nil || result.ID != head.ID { + t.Fatalf("head skip: result=%+v merged=%v err=%v", result, merged, err) + } + source := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("missing", "source")) + if _, err := repo.TakeByID(context.Background(), "missing", source.ID); err != nil { + t.Fatalf("drain source: %v", err) + } + before := autoQueueSnapshot(t, repo, "missing") + result, merged, err = autoRepo.AutoMergeIntoAbove(context.Background(), "missing", source.ID) + if err != nil || merged || result != nil { + t.Fatalf("missing skip: result=%+v merged=%v err=%v", result, merged, err) + } + if after := autoQueueSnapshot(t, repo, "missing"); after != before { + t.Fatalf("queue changed after missing-source skip") + } + }) + } +} + +func TestRepository_AutoMergeIntoAbove_EmptyContentHasNoExtraSeparator(t *testing.T) { + tests := []struct { + name, target, source, want string + }{ + {name: "empty target", source: "source", want: "source"}, + {name: "empty source", target: "target", want: "target"}, + {name: "both empty", want: ""}, + } + for _, tt := range autoMergeRepositoryFactories() { + t.Run(tt.name, func(t *testing.T) { + repo := tt.new(t) + autoRepo := repo.(automaticMergeRepository) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + session := "empty-" + test.name + insertAutoMergeEntry(t, repo, defaultAutoMergeEntry(session, test.target)) + source := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry(session, test.source)) + merged, didMerge, err := autoRepo.AutoMergeIntoAbove(context.Background(), session, source.ID) + if err != nil || !didMerge || merged.Content != test.want { + t.Fatalf("result=%+v merged=%v err=%v, want content %q", merged, didMerge, err, test.want) + } + }) + } + }) + } +} + +func TestSQLiteRepository_AutoMergeIntoAbove_WriteFailureRollsBack(t *testing.T) { + repo := newTestSQLiteRepo(t) + testAutoMergeWriteRollback(t, repo, ` + CREATE TRIGGER fail_auto_merge_delete + BEFORE DELETE ON queued_messages + BEGIN + SELECT RAISE(ABORT, 'forced automatic merge failure'); + END; + `) +} + +func TestPostgresRepository_AutoMergeIntoAbove_WriteFailureRollsBack(t *testing.T) { + repo := newTestPostgresRepo(t) + db := repo.(*sqliteRepository).db + if _, err := db.Exec(` + CREATE FUNCTION fail_auto_merge_delete() RETURNS trigger AS $$ + BEGIN + RAISE EXCEPTION 'forced automatic merge failure'; + END; + $$ LANGUAGE plpgsql + `); err != nil { + t.Fatalf("create failure function: %v", err) + } + testAutoMergeWriteRollback(t, repo, ` + CREATE TRIGGER fail_auto_merge_delete + BEFORE DELETE ON queued_messages + FOR EACH ROW EXECUTE FUNCTION fail_auto_merge_delete() + `) +} + +type autoMergeIdentityFields struct { + ID string + SessionID string + TaskID string + Position int64 + Model string + PlanMode bool + QueuedBy string + QueuedAt time.Time +} + +func autoMergeIdentity(message QueuedMessage) autoMergeIdentityFields { + return autoMergeIdentityFields{ + ID: message.ID, SessionID: message.SessionID, TaskID: message.TaskID, + Position: message.Position, Model: message.Model, PlanMode: message.PlanMode, + QueuedBy: message.QueuedBy, QueuedAt: message.QueuedAt.UTC().Truncate(time.Microsecond), + } +} + +func insertAutoMergeEntry(t *testing.T, repo Repository, message QueuedMessage) *QueuedMessage { + t.Helper() + if err := repo.Insert(context.Background(), &message, 0); err != nil { + t.Fatalf("insert auto-merge entry: %v", err) + } + return &message +} + +func defaultAutoMergeEntry(session, content string) QueuedMessage { + return QueuedMessage{ + SessionID: session, TaskID: "task", Content: content, Model: "model", QueuedBy: QueuedByUser, + } +} + +func attachmentNames(attachments []MessageAttachment) []string { + names := make([]string, len(attachments)) + for index := range attachments { + names[index] = attachments[index].Name + } + return names +} + +func referenceIDs(metadata map[string]interface{}) []string { + references := entityrefs.NormalizePersisted(metadata[MetadataEntityReferences]) + ids := make([]string, len(references)) + for index := range references { + ids[index] = references[index].ID + } + return ids +} + +func autoQueueSnapshot(t *testing.T, repo Repository, session string) string { + t.Helper() + entries, err := repo.ListBySession(context.Background(), session) + if err != nil { + t.Fatalf("list queue: %v", err) + } + encoded, err := json.Marshal(entries) + if err != nil { + t.Fatalf("marshal queue snapshot: %v", err) + } + return string(encoded) +} + +type autoMergeSkipCase struct { + name string + mutate func(*QueuedMessage, *QueuedMessage) +} + +func autoMergeSkipCases() []autoMergeSkipCase { + return []autoMergeSkipCase{ + {name: "task mismatch", mutate: func(_, source *QueuedMessage) { source.TaskID = "other-task" }}, + {name: "model mismatch", mutate: func(_, source *QueuedMessage) { source.Model = "other-model" }}, + {name: "plan mode mismatch", mutate: func(_, source *QueuedMessage) { source.PlanMode = true }}, + {name: "user identity mismatch", mutate: func(_, source *QueuedMessage) { source.QueuedBy = "bob" }}, + {name: "user agent mismatch", mutate: func(_, source *QueuedMessage) { + source.QueuedBy = QueuedByAgent + source.Metadata = senderMetadata("sender") + }}, + {name: "agent clarification missing sender", mutate: func(target, source *QueuedMessage) { target.QueuedBy = QueuedByAgent; source.QueuedBy = QueuedByAgent }}, + {name: "agent sender mismatch", mutate: func(target, source *QueuedMessage) { + target.QueuedBy, source.QueuedBy = QueuedByAgent, QueuedByAgent + target.Metadata, source.Metadata = senderMetadata("one"), senderMetadata("two") + }}, + {name: "workflow source", mutate: func(target, source *QueuedMessage) { + target.QueuedBy, source.QueuedBy = QueuedByWorkflow, QueuedByWorkflow + }}, + {name: "server source", mutate: func(target, source *QueuedMessage) { target.QueuedBy, source.QueuedBy = QueuedByServer, QueuedByServer }}, + {name: "durable target", mutate: func(target, source *QueuedMessage) { + target.Metadata, source.Metadata = durableMetadata(), durableMetadata() + }}, + {name: "durable source", mutate: func(_, source *QueuedMessage) { source.Metadata = durableMetadata() }}, + {name: "metadata mismatch", mutate: func(target, source *QueuedMessage) { + target.Metadata = map[string]interface{}{"plugin_source": "one"} + source.Metadata = map[string]interface{}{"plugin_source": "two"} + }}, + {name: "attachment count overflow", mutate: func(target, source *QueuedMessage) { + target.Attachments = autoFileAttachments("target", messageconstraints.MaxAttachmentCount/2+1, 1) + source.Attachments = autoFileAttachments("source", messageconstraints.MaxAttachmentCount/2, 1) + }}, + {name: "attachment bytes overflow", mutate: func(target, source *QueuedMessage) { + target.Attachments = autoFileAttachments("target", 1, messageconstraints.MaxAttachmentBytes/2+1) + source.Attachments = autoFileAttachments("source", 1, messageconstraints.MaxAttachmentBytes/2) + }}, + {name: "entity reference overflow", mutate: func(target, source *QueuedMessage) { + target.Metadata = map[string]interface{}{MetadataEntityReferences: manyEntityRefs(entityrefs.MaxReferencesPerMessage)} + source.Metadata = map[string]interface{}{MetadataEntityReferences: entityRefs("overflow")} + }}, + {name: "context file overflow", mutate: func(target, source *QueuedMessage) { + target.Metadata = map[string]interface{}{MetadataContextFiles: autoContextFiles("target", messageconstraints.MaxContextFilesPerMessage/2+1)} + source.Metadata = map[string]interface{}{MetadataContextFiles: autoContextFiles("source", messageconstraints.MaxContextFilesPerMessage/2)} + }}, + {name: "malformed context files", mutate: func(target, source *QueuedMessage) { + target.Metadata = map[string]interface{}{MetadataContextFiles: []interface{}{map[string]interface{}{"path": "valid", "name": "valid"}}} + source.Metadata = map[string]interface{}{MetadataContextFiles: "invalid"} + }}, + } +} + +func runAutoMergeSkipCase(t *testing.T, repo Repository, session string, mutate func(*QueuedMessage, *QueuedMessage)) { + t.Helper() + target := defaultAutoMergeEntry(session, "target") + source := defaultAutoMergeEntry(session, "source") + mutate(&target, &source) + insertAutoMergeEntry(t, repo, target) + insertedSource := insertAutoMergeEntry(t, repo, source) + before := autoQueueSnapshot(t, repo, session) + result, merged, err := repo.(automaticMergeRepository).AutoMergeIntoAbove(context.Background(), session, insertedSource.ID) + if err != nil || merged { + t.Fatalf("skip result: merged=%v err=%v", merged, err) + } + if result == nil || result.ID != insertedSource.ID { + t.Fatalf("skip returned %+v, want source %s", result, insertedSource.ID) + } + if after := autoQueueSnapshot(t, repo, session); after != before { + t.Fatalf("queue mutated on skip\nbefore=%s\nafter=%s", before, after) + } +} + +func senderMetadata(sender string) map[string]interface{} { + return map[string]interface{}{MetadataSenderTaskID: sender} +} + +func durableMetadata() map[string]interface{} { + return map[string]interface{}{MetadataLifecycleDurable: true} +} + +func autoFileAttachments(prefix string, count int, size int64) []MessageAttachment { + attachments := make([]MessageAttachment, count) + for index := range attachments { + attachments[index] = MessageAttachment{AttachmentID: prefix + string(rune('a'+index)), SizeBytes: size} + } + return attachments +} + +func autoContextFiles(prefix string, count int) []apiv1.ContextFileMeta { + files := make([]apiv1.ContextFileMeta, count) + for index := range files { + files[index] = apiv1.ContextFileMeta{Path: prefix + "/file-" + fmt.Sprint(index), Name: "file"} + } + return files +} + +func testAutoMergeWriteRollback(t *testing.T, repo Repository, createTrigger string) { + t.Helper() + db := repo.(*sqliteRepository).db + target := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("rollback", "target")) + source := insertAutoMergeEntry(t, repo, defaultAutoMergeEntry("rollback", "source")) + before := autoQueueSnapshot(t, repo, "rollback") + if _, err := db.Exec(createTrigger); err != nil { + t.Fatalf("create failure trigger: %v", err) + } + result, merged, err := repo.(automaticMergeRepository).AutoMergeIntoAbove(context.Background(), "rollback", source.ID) + if err == nil || merged || result != nil { + t.Fatalf("forced write failure: result=%+v merged=%v err=%v", result, merged, err) + } + if after := autoQueueSnapshot(t, repo, "rollback"); after != before { + t.Fatalf("write failure was not atomic\nbefore=%s\nafter=%s", before, after) + } + entries, _ := repo.ListBySession(context.Background(), "rollback") + if len(entries) != 2 || entries[0].ID != target.ID || entries[1].ID != source.ID { + t.Fatalf("rollback entries = %+v", entries) + } +} diff --git a/apps/backend/internal/orchestrator/messagequeue/repository_memory.go b/apps/backend/internal/orchestrator/messagequeue/repository_memory.go index e0efc92afa..cf92162af0 100644 --- a/apps/backend/internal/orchestrator/messagequeue/repository_memory.go +++ b/apps/backend/internal/orchestrator/messagequeue/repository_memory.go @@ -660,6 +660,44 @@ func (r *memoryRepository) MergeIntoAbove(_ context.Context, sessionID, sourceID return &merged, nil } +// AutoMergeIntoAbove folds one exact source into its immediate compatible +// predecessor. Incompatibility and missing candidates are successful skips. +func (r *memoryRepository) AutoMergeIntoAbove(_ context.Context, sessionID, sourceID string) (*QueuedMessage, bool, error) { + r.mu.Lock() + defer r.mu.Unlock() + + list := r.entries[sessionID] + sourceIndex := -1 + for index, message := range list { + if message.ID == sourceID { + sourceIndex = index + break + } + } + if sourceIndex < 0 { + return nil, false, nil + } + source := list[sourceIndex] + var target *QueuedMessage + for _, message := range list { + if message.Position < source.Position && (target == nil || message.Position > target.Position) { + target = message + } + } + if target == nil { + return cloneQueuedMessage(source), false, nil + } + values, compatible := buildAutoMergedEntry(target, source) + if !compatible { + return cloneQueuedMessage(source), false, nil + } + target.Content = values.content + target.Attachments = values.attachments + target.Metadata = values.metadata + r.entries[sessionID] = append(list[:sourceIndex], list[sourceIndex+1:]...) + return cloneQueuedMessage(target), true, nil +} + // ReorderEntries rewrites the session's visible pending order to match // orderedIDs, mirroring the sqlite repository's semantics: reserved in-flight // rows keep their place in the sequence, visible rows appear in the submitted diff --git a/apps/backend/internal/orchestrator/messagequeue/repository_sqlite.go b/apps/backend/internal/orchestrator/messagequeue/repository_sqlite.go index e9878c089e..8e9c863903 100644 --- a/apps/backend/internal/orchestrator/messagequeue/repository_sqlite.go +++ b/apps/backend/internal/orchestrator/messagequeue/repository_sqlite.go @@ -1319,6 +1319,52 @@ func (r *sqliteRepository) MergeIntoAbove(ctx context.Context, sessionID, source return &merged, nil } +// AutoMergeIntoAbove folds one exact source into its immediate compatible +// predecessor in one transaction. Missing or incompatible candidates are +// successful skips and leave storage unchanged. +func (r *sqliteRepository) AutoMergeIntoAbove(ctx context.Context, sessionID, sourceID string) (*QueuedMessage, bool, error) { + unlock := r.withSessionLock(sessionID) + defer unlock() + + tx, err := r.db.BeginTxx(ctx, nil) + if err != nil { + return nil, false, fmt.Errorf("begin automatic merge tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + source, err := readMergeSource(ctx, r, tx, sessionID, sourceID) + if errors.Is(err, ErrEntryNotFound) { + return nil, false, nil + } + if err != nil { + return nil, false, err + } + target, err := readMergeTarget(ctx, r, tx, sessionID, source.Position) + if errors.Is(err, ErrNoMergeTarget) { + return source, false, nil + } + if err != nil { + return nil, false, err + } + values, compatible := buildAutoMergedEntry(target, source) + if !compatible { + return source, false, nil + } + if err := applyMergeWrites(ctx, r, tx, target, source, values.content, values.attachments, values.metadata, sessionID); err != nil { + if errors.Is(err, ErrEntryNotFound) { + return nil, false, nil + } + return nil, false, err + } + if err := tx.Commit(); err != nil { + return nil, false, err + } + target.Content = values.content + target.Attachments = values.attachments + target.Metadata = values.metadata + return target, true, nil +} + // ReorderEntries atomically rewrites the FIFO positions of the session's // visible pending entries to match orderedIDs. Reserved in-flight lifecycle // rows keep their place in the sequence; visible rows are interleaved in the diff --git a/apps/backend/internal/orchestrator/messagequeue/service.go b/apps/backend/internal/orchestrator/messagequeue/service.go index b4f7533d86..a5908bda6e 100644 --- a/apps/backend/internal/orchestrator/messagequeue/service.go +++ b/apps/backend/internal/orchestrator/messagequeue/service.go @@ -22,12 +22,13 @@ import ( // Service manages queued messages for sessions, backed by Repository. type Service struct { - repo Repository - maxPerSession atomic.Int64 - mergeEnabled atomic.Bool - logger *logger.Logger - admissionMu sync.Mutex - admissions map[string]*sessionAdmission + repo Repository + maxPerSession atomic.Int64 + mergeEnabled atomic.Bool + autoMergeEnabled atomic.Bool + logger *logger.Logger + admissionMu sync.Mutex + admissions map[string]*sessionAdmission } type sessionAdmission struct { @@ -53,6 +54,7 @@ func NewService(repo Repository, maxPerSession int, log *logger.Logger) *Service } service.SetMaxPerSession(maxPerSession) service.mergeEnabled.Store(true) + service.autoMergeEnabled.Store(true) return service } @@ -91,9 +93,20 @@ func (s *Service) SetMergeEnabled(enabled bool) { s.mergeEnabled.Store(enabled) } +// AutoMergeEnabled reports whether ordinary newly admitted messages may be +// folded into a compatible queue tail. It is independent from manual merge. +func (s *Service) AutoMergeEnabled() bool { return s.autoMergeEnabled.Load() } + +// SetAutoMergeEnabled toggles admission-time automatic merging. Existing rows +// are never compacted retroactively. +func (s *Service) SetAutoMergeEnabled(enabled bool) { + s.autoMergeEnabled.Store(enabled) +} + // WithSessionAdmission runs fn under the per-session queue admission lock. -// All queue insertion paths use the same lock. The callback must complete -// synchronously; queue methods called with its context reuse the held lock. +// All queue insertion and mutation paths use the same lock. The callback must +// complete synchronously; queue methods called with its context reuse the held +// lock. func (s *Service) WithSessionAdmission(ctx context.Context, sessionID string, fn func(context.Context) error) error { if fn == nil { return errors.New("session admission callback is nil") @@ -140,14 +153,48 @@ func (s *Service) QueueMessage(ctx context.Context, sessionID, taskID, content, // is propagated to the resulting Message row when the queued message is // drained (e.g. sender_task_id for messages sent via message_task_kandev). func (s *Service) QueueMessageWithMetadata(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}) (*QueuedMessage, error) { - return s.queueMessageWithMetadata( - ctx, sessionID, taskID, content, model, userID, planMode, attachments, metadata, - s.MaxPerSession(), + return s.queueMessageWithMetadataAdmission( + ctx, sessionID, taskID, content, model, userID, planMode, attachments, metadata, nil, ) } -// queueMessageWithMetadata is the admission-checked core of QueueMessageWithMetadata. -func (s *Service) queueMessageWithMetadata(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}, maxPerSession int) (*QueuedMessage, error) { +// QueueMessageWithMetadataAfterInsert admits an exact source row, runs +// afterInsert while the per-session admission lock remains held, then attempts +// the snapshotted automatic fold. Callers use the hook to claim staged +// attachments before folding; if it returns an error, no fold is attempted. +func (s *Service) QueueMessageWithMetadataAfterInsert(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}, afterInsert func(context.Context, *QueuedMessage) error) (*QueuedMessage, error) { + return s.queueMessageWithMetadataAdmission( + ctx, sessionID, taskID, content, model, userID, planMode, attachments, metadata, afterInsert, + ) +} + +// queueMessageWithMetadataAdmission snapshots policy and completes admission +// under one per-session lock. +func (s *Service) queueMessageWithMetadataAdmission(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}, afterInsert func(context.Context, *QueuedMessage) error) (*QueuedMessage, error) { + var queued *QueuedMessage + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + maxPerSession := s.MaxPerSession() + autoMergeEnabled := s.AutoMergeEnabled() + source, err := s.insertQueueMessageWithMetadata( + admittedCtx, sessionID, taskID, content, model, userID, planMode, attachments, metadata, maxPerSession, + ) + if err != nil { + return err + } + if afterInsert != nil { + if err := afterInsert(admittedCtx, source); err != nil { + return err + } + } + queued = s.finalizeAutoMerge(admittedCtx, source, autoMergeEnabled) + return nil + }) + return queued, err +} + +// queueMessageWithMetadataSeparate is used by retry paths whose existing +// contract must not gain admission-time automatic behavior. +func (s *Service) queueMessageWithMetadataSeparate(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}, maxPerSession int) (*QueuedMessage, error) { var queued *QueuedMessage err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { var err error @@ -159,6 +206,28 @@ func (s *Service) queueMessageWithMetadata(ctx context.Context, sessionID, taskI return queued, err } +func (s *Service) finalizeAutoMerge(ctx context.Context, source *QueuedMessage, enabled bool) *QueuedMessage { + if !enabled { + return source + } + merged, didMerge, err := s.repo.AutoMergeIntoAbove(ctx, source.SessionID, source.ID) + if err != nil { + s.logger.Error("automatic queue merge failed; preserving separate admission", + zap.String("session_id", source.SessionID), + zap.String("source_entry_id", source.ID), + zap.Error(err)) + return source + } + if !didMerge || merged == nil { + return source + } + s.logger.Info("automatically merged queued entry into tail", + zap.String("session_id", source.SessionID), + zap.String("source_entry_id", source.ID), + zap.String("surviving_entry_id", merged.ID)) + return merged +} + // insertQueueMessageWithMetadata inserts a message with metadata under the per-session admission lock. func (s *Service) insertQueueMessageWithMetadata(ctx context.Context, sessionID, taskID, content, model, userID string, planMode bool, attachments []MessageAttachment, metadata map[string]interface{}, maxPerSession int) (*QueuedMessage, error) { metadataCopy := copyMessageMetadata(metadata, 0) @@ -293,7 +362,7 @@ func (s *Service) RequeueMessage(ctx context.Context, msg *QueuedMessage, queued msg.Attachments, msg.Metadata, coalesceKey, true, 0, ) } - queued, err := s.queueMessageWithMetadata( + queued, err := s.queueMessageWithMetadataSeparate( ctx, msg.SessionID, msg.TaskID, msg.Content, msg.Model, queuedBy, msg.PlanMode, msg.Attachments, msg.Metadata, 0, ) @@ -389,9 +458,16 @@ func lifecycleGenerationFromMetadata(metadata map[string]interface{}) (int64, bo } // ReserveQueued atomically takes an ordinary head entry or reserves a durable -// lifecycle head entry. A reserved lifecycle row survives until acknowledged. +// lifecycle head entry. The admission lock keeps drains from observing an +// insert before its automatic-merge finalization completes. A reserved +// lifecycle row survives until acknowledged. func (s *Service) ReserveQueued(ctx context.Context, sessionID string) (*QueuedMessage, bool) { - msg, err := s.repo.ReserveHead(ctx, sessionID) + var msg *QueuedMessage + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + msg, err = s.repo.ReserveHead(admittedCtx, sessionID) + return err + }) if err != nil { s.logger.Error("reserve head failed", zap.String("session_id", sessionID), @@ -403,7 +479,9 @@ func (s *Service) ReserveQueued(ctx context.Context, sessionID string) (*QueuedM // AcknowledgeQueued removes a server-reserved entry after prompt acceptance. func (s *Service) AcknowledgeQueued(ctx context.Context, sessionID, entryID string) error { - err := s.repo.AcknowledgeByID(ctx, sessionID, entryID) + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + return s.repo.AcknowledgeByID(admittedCtx, sessionID, entryID) + }) if errors.Is(err, ErrEntryNotFound) { return nil } @@ -475,7 +553,12 @@ func (s *Service) AppendContent(ctx context.Context, sessionID, taskID, content, // TakeQueued atomically removes and returns the head entry. Returns nil, false // when the queue is empty. func (s *Service) TakeQueued(ctx context.Context, sessionID string) (*QueuedMessage, bool) { - msg, err := s.repo.TakeHead(ctx, sessionID) + var msg *QueuedMessage + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + msg, err = s.repo.TakeHead(admittedCtx, sessionID) + return err + }) if err != nil { s.logger.Error("take head failed", zap.String("session_id", sessionID), @@ -504,7 +587,12 @@ func (s *Service) TakeQueued(ctx context.Context, sessionID string) (*QueuedMess // InterruptForPeerMessage to dispatch the specific message that triggered // the interrupt instead of whatever happens to be at the FIFO head. func (s *Service) TakeQueuedEntry(ctx context.Context, sessionID, entryID string) (*QueuedMessage, bool, error) { - msg, err := s.repo.TakeByID(ctx, sessionID, entryID) + var msg *QueuedMessage + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + msg, err = s.repo.TakeByID(admittedCtx, sessionID, entryID) + return err + }) if err != nil { s.logger.Error("take by id failed", zap.String("session_id", sessionID), @@ -553,7 +641,12 @@ func (s *Service) GetEntry(ctx context.Context, sessionID, entryID string) (*Que // mutating any row, so aggregate validation failures or click-time edits leave // the queue untouched. func (s *Service) ClaimSendNow(ctx context.Context, sessionID string, expected []QueuedMessage) (*SendNowClaim, error) { - claim, err := s.repo.ClaimSendNow(ctx, sessionID, expected) + var claim *SendNowClaim + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + claim, err = s.repo.ClaimSendNow(admittedCtx, sessionID, expected) + return err + }) if err != nil { return nil, err } @@ -596,7 +689,9 @@ func (s *Service) AcknowledgeSendNowClaim(ctx context.Context, claim *SendNowCla // UpdateMessageWithMetadata atomically edits queue content and applies // metadata replacements while retaining unrelated metadata keys. func (s *Service) UpdateMessageWithMetadata(ctx context.Context, sessionID, entryID, content string, attachments []MessageAttachment, metadataUpdates map[string]interface{}, queuedBy string) error { - if err := s.repo.UpdateContentAndMetadata(ctx, sessionID, entryID, content, attachments, metadataUpdates, queuedBy); err != nil { + if err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + return s.repo.UpdateContentAndMetadata(admittedCtx, sessionID, entryID, content, attachments, metadataUpdates, queuedBy) + }); err != nil { return err } s.logger.Info("queued entry updated", @@ -609,7 +704,9 @@ func (s *Service) UpdateMessageWithMetadata(ctx context.Context, sessionID, entr // the rationale on the Repository.DeleteByID contract for why. Returns // ErrEntryNotFound when no entry matches. func (s *Service) RemoveEntry(ctx context.Context, sessionID, entryID string) error { - if err := s.repo.DeleteByID(ctx, sessionID, entryID); err != nil { + if err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + return s.repo.DeleteByID(admittedCtx, sessionID, entryID) + }); err != nil { return err } s.logger.Info("queued entry removed", @@ -625,7 +722,12 @@ func (s *Service) MergeIntoAbove(ctx context.Context, sessionID, entryID, queued if !s.MergeEnabled() { return nil, ErrMergeDisabled } - merged, err := s.repo.MergeIntoAbove(ctx, sessionID, entryID, queuedBy) + var merged *QueuedMessage + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + merged, err = s.repo.MergeIntoAbove(admittedCtx, sessionID, entryID, queuedBy) + return err + }) if err != nil { return nil, err } @@ -644,7 +746,9 @@ func (s *Service) ReorderEntries(ctx context.Context, sessionID string, orderedI if err := validateReorderInput(orderedIDs); err != nil { return err } - if err := s.repo.ReorderEntries(ctx, sessionID, orderedIDs); err != nil { + if err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + return s.repo.ReorderEntries(admittedCtx, sessionID, orderedIDs) + }); err != nil { return err } s.logger.Info("queued entries reordered", @@ -656,7 +760,12 @@ func (s *Service) ReorderEntries(ctx context.Context, sessionID string, orderedI // CancelAll clears every queued entry for a session. Returns the number of // rows removed. func (s *Service) CancelAll(ctx context.Context, sessionID string) (int, error) { - n, err := s.repo.DeleteAllBySession(ctx, sessionID) + var n int + err := s.WithSessionAdmission(ctx, sessionID, func(admittedCtx context.Context) error { + var err error + n, err = s.repo.DeleteAllBySession(admittedCtx, sessionID) + return err + }) if err != nil { return 0, err } diff --git a/apps/backend/internal/orchestrator/messagequeue/service_auto_merge_test.go b/apps/backend/internal/orchestrator/messagequeue/service_auto_merge_test.go new file mode 100644 index 0000000000..17dd7f99c5 --- /dev/null +++ b/apps/backend/internal/orchestrator/messagequeue/service_auto_merge_test.go @@ -0,0 +1,331 @@ +package messagequeue + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" +) + +type automaticMergeSettings interface { + AutoMergeEnabled() bool + SetAutoMergeEnabled(bool) +} + +func TestService_AutoMergeCanBeDisabledWithoutChangingManualMerge(t *testing.T) { + for _, test := range []struct { + name, auto, manual string + wantCount int + }{ + {name: "both on", auto: "on", manual: "on", wantCount: 1}, + {name: "auto on manual off", auto: "on", manual: "off", wantCount: 1}, + {name: "auto off manual on", auto: "off", manual: "on", wantCount: 2}, + {name: "both off", auto: "off", manual: "off", wantCount: 2}, + } { + t.Run(test.name, func(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + svc.SetAutoMergeEnabled(test.auto == "on") + svc.SetMergeEnabled(test.manual == "on") + first, err := svc.QueueMessage(context.Background(), "session", "task", "first", "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue first: %v", err) + } + second, err := svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue second: %v", err) + } + if got := svc.GetStatus(context.Background(), "session").Count; got != test.wantCount { + t.Fatalf("queue count = %d, want %d", got, test.wantCount) + } + if test.auto == "on" && second.ID != first.ID { + t.Errorf("automatic survivor = %s, want %s", second.ID, first.ID) + } + if svc.MergeEnabled() != (test.manual == "on") { + t.Errorf("manual merge setting changed") + } + }) + } +} + +func TestService_AutoMergeChainsThreeAdmissions(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + var survivor string + for _, content := range []string{"first", "second", "third"} { + queued, err := svc.QueueMessage(context.Background(), "session", "task", content, "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue %q: %v", content, err) + } + if survivor == "" { + survivor = queued.ID + } else if queued.ID != survivor { + t.Fatalf("survivor changed to %s, want %s", queued.ID, survivor) + } + } + status := svc.GetStatus(context.Background(), "session") + if status.Count != 1 || status.Entries[0].Content != "first\n\nsecond\n\nthird" { + t.Fatalf("chained status = %+v", status) + } +} + +func TestService_AutoMergeDoesNotBypassCapacity(t *testing.T) { + svc := newAutoMergeTestService(t, 1) + first, err := svc.QueueMessage(context.Background(), "session", "task", "first", "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue first: %v", err) + } + second, err := svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + if !errors.Is(err, ErrQueueFull) || second != nil { + t.Fatalf("second admission = %+v, err=%v, want queue full", second, err) + } + status := svc.GetStatus(context.Background(), "session") + if status.Count != 1 || status.Entries[0].ID != first.ID || status.Entries[0].Content != "first" { + t.Fatalf("queue changed after full admission: %+v", status) + } +} + +func TestService_AutoMergeStorageErrorDegradesToSeparateAdmission(t *testing.T) { + wantErr := errors.New("automatic merge unavailable") + repo := &autoMergeErrorRepository{Repository: NewMemoryRepository(), err: wantErr} + svc := newAutoMergeTestServiceWithRepository(t, repo, 10) + first, err := svc.QueueMessage(context.Background(), "session", "task", "first", "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue first: %v", err) + } + second, err := svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("post-insert merge error escaped admission: %v", err) + } + if second.ID == first.ID { + t.Fatal("failed fold should keep distinct source identity") + } + if status := svc.GetStatus(context.Background(), "session"); status.Count != 2 { + t.Fatalf("degraded queue count = %d, want 2", status.Count) + } +} + +func TestService_AutoMergeSerializesDeferredFinalizationAndLaterAdmission(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + inserted := make(chan struct{}) + release := make(chan struct{}) + firstDone := make(chan error, 1) + go func() { + _, err := svc.QueueMessageWithMetadataAfterInsert( + context.Background(), "session", "task", "first", "", QueuedByUser, false, nil, nil, + func(context.Context, *QueuedMessage) error { + close(inserted) + <-release + return nil + }, + ) + firstDone <- err + }() + <-inserted + + secondStarted := make(chan struct{}) + secondDone := make(chan error, 1) + go func() { + close(secondStarted) + _, err := svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + secondDone <- err + }() + <-secondStarted + select { + case err := <-secondDone: + t.Fatalf("later admission bypassed provisional source lock: %v", err) + case <-time.After(50 * time.Millisecond): + } + close(release) + if err := <-firstDone; err != nil { + t.Fatalf("first admission: %v", err) + } + if err := <-secondDone; err != nil { + t.Fatalf("second admission: %v", err) + } + status := svc.GetStatus(context.Background(), "session") + if status.Count != 1 || status.Entries[0].Content != "first\n\nsecond" { + t.Fatalf("serialized queue = %+v", status) + } +} + +func TestService_AutoMergeSerializesFinalizationWithDrainAndRemoval(t *testing.T) { + for _, test := range []struct { + name string + run func(*testing.T, *Service, *QueuedMessage, chan struct{}) + }{ + { + name: "drain", + run: func(t *testing.T, svc *Service, source *QueuedMessage, release chan struct{}) { + started := make(chan struct{}) + result := make(chan struct { + msg *QueuedMessage + ok bool + }, 1) + go func() { + close(started) + msg, ok := svc.ReserveQueued(context.Background(), source.SessionID) + result <- struct { + msg *QueuedMessage + ok bool + }{msg: msg, ok: ok} + }() + <-started + select { + case got := <-result: + t.Fatalf("drain bypassed admission finalization: %+v", got) + case <-time.After(50 * time.Millisecond): + } + close(release) + got := <-result + if !got.ok || got.msg == nil || got.msg.ID != source.ID { + t.Fatalf("drain result = %+v, want source %s", got, source.ID) + } + }, + }, + { + name: "remove", + run: func(t *testing.T, svc *Service, source *QueuedMessage, release chan struct{}) { + started := make(chan struct{}) + result := make(chan error, 1) + go func() { + close(started) + result <- svc.RemoveEntry(context.Background(), source.SessionID, source.ID) + }() + <-started + select { + case err := <-result: + t.Fatalf("removal bypassed admission finalization: %v", err) + case <-time.After(50 * time.Millisecond): + } + close(release) + if err := <-result; err != nil { + t.Fatalf("remove after finalization: %v", err) + } + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + release := make(chan struct{}) + inserted := make(chan *QueuedMessage, 1) + admissionDone := make(chan error, 1) + go func() { + _, err := svc.QueueMessageWithMetadataAfterInsert( + context.Background(), "session", "task", "first", "", QueuedByUser, false, nil, nil, + func(_ context.Context, source *QueuedMessage) error { + inserted <- source + <-release + return nil + }, + ) + admissionDone <- err + }() + source := <-inserted + test.run(t, svc, source, release) + if err := <-admissionDone; err != nil { + t.Fatalf("admission: %v", err) + } + }) + } +} + +func TestService_AutoMergeSkipsExplicitMutationContracts(t *testing.T) { + t.Run("coalesce insert", func(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + for _, key := range []string{"one", "two"} { + if _, _, err := svc.QueueMessageWithCoalesceKey( + context.Background(), "session", "task", key, "", QueuedByUser, false, nil, nil, key, true, + ); err != nil { + t.Fatalf("queue coalesced %s: %v", key, err) + } + } + if got := svc.GetStatus(context.Background(), "session").Count; got != 2 { + t.Fatalf("coalesce entries auto-merged: count=%d", got) + } + }) + + t.Run("restore", func(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + svc.SetAutoMergeEnabled(false) + first, _ := svc.QueueMessage(context.Background(), "session", "task", "first", "", QueuedByUser, false, nil) + _, _ = svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + _, _, _ = svc.TakeQueuedEntry(context.Background(), "session", first.ID) + svc.SetAutoMergeEnabled(true) + if _, err := svc.RestoreMessage(context.Background(), first); err != nil { + t.Fatalf("restore: %v", err) + } + if got := svc.GetStatus(context.Background(), "session").Count; got != 2 { + t.Fatalf("restore auto-merged: count=%d", got) + } + }) + + t.Run("retry", func(t *testing.T) { + svc := newAutoMergeTestService(t, 10) + svc.SetAutoMergeEnabled(false) + first, _ := svc.QueueMessage(context.Background(), "session", "task", "first", "", QueuedByUser, false, nil) + _, _ = svc.QueueMessage(context.Background(), "session", "task", "second", "", QueuedByUser, false, nil) + _, _, _ = svc.TakeQueuedEntry(context.Background(), "session", first.ID) + svc.SetAutoMergeEnabled(true) + if _, _, err := svc.RequeueMessage(context.Background(), first, QueuedByUser, ""); err != nil { + t.Fatalf("requeue: %v", err) + } + if got := svc.GetStatus(context.Background(), "session").Count; got != 2 { + t.Fatalf("retry auto-merged: count=%d", got) + } + }) +} + +type autoMergeErrorRepository struct { + Repository + err error +} + +func (r *autoMergeErrorRepository) AutoMergeIntoAbove(context.Context, string, string) (*QueuedMessage, bool, error) { + return nil, false, r.err +} + +func newAutoMergeTestService(t *testing.T, max int) *Service { + t.Helper() + return newAutoMergeTestServiceWithRepository(t, NewMemoryRepository(), max) +} + +func newAutoMergeTestServiceWithRepository(t *testing.T, repo Repository, max int) *Service { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) + if err != nil { + t.Fatalf("create logger: %v", err) + } + return NewService(repo, max, log) +} + +func TestService_AutoMergeDefaultsOnAndFoldsCompatibleAdmissions(t *testing.T) { + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) + if err != nil { + t.Fatalf("create logger: %v", err) + } + svc := NewServiceMemory(log) + settings, ok := interface{}(svc).(automaticMergeSettings) + if !ok { + t.Fatalf("service %T does not expose an independent automatic-merge setting", svc) + } + if !settings.AutoMergeEnabled() { + t.Fatal("automatic merge should default on") + } + + first, err := svc.QueueMessage(context.Background(), "session", "task", "first", "model", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue first: %v", err) + } + second, err := svc.QueueMessage(context.Background(), "session", "task", "second", "model", QueuedByUser, false, nil) + if err != nil { + t.Fatalf("queue second: %v", err) + } + if second.ID != first.ID || second.Content != "first\n\nsecond" { + t.Fatalf("second admission = %+v, want surviving first entry", second) + } + status := svc.GetStatus(context.Background(), "session") + if status.Count != 1 || status.Entries[0].ID != first.ID { + t.Fatalf("status = %+v, want one merged entry", status) + } +} diff --git a/apps/backend/internal/orchestrator/messagequeue/service_test.go b/apps/backend/internal/orchestrator/messagequeue/service_test.go index b0f6267fb7..d7a7c0b078 100644 --- a/apps/backend/internal/orchestrator/messagequeue/service_test.go +++ b/apps/backend/internal/orchestrator/messagequeue/service_test.go @@ -24,7 +24,11 @@ func setupService(t *testing.T) *Service { OutputPath: "stderr", }) require.NoError(t, err) - return NewServiceMemory(log) + service := NewServiceMemory(log) + // Most legacy service tests exercise explicit FIFO mutation contracts and + // intentionally require separate compatible rows. + service.SetAutoMergeEnabled(false) + return service } func TestQueueMessage(t *testing.T) { @@ -87,6 +91,7 @@ func TestLoweredQueueCapacityBlocksAdmissionsWithoutPruning(t *testing.T) { log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) require.NoError(t, err) svc := NewService(NewMemoryRepository(), 2, log) + svc.SetAutoMergeEnabled(false) ctx := context.Background() for _, content := range []string{"first", "second"} { @@ -104,6 +109,7 @@ func TestRestoreMessageBypassesLoweredCapacity(t *testing.T) { log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) require.NoError(t, err) svc := NewService(NewMemoryRepository(), 2, log) + svc.SetAutoMergeEnabled(false) ctx := context.Background() _, err = svc.QueueMessage(ctx, "s", "t", "first", "", QueuedByUser, false, nil) @@ -134,6 +140,7 @@ func TestRequeueMessageBypassesLoweredCapacity(t *testing.T) { log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console", OutputPath: "stderr"}) require.NoError(t, err) svc := NewService(NewMemoryRepository(), 2, log) + svc.SetAutoMergeEnabled(false) ctx := context.Background() var first *QueuedMessage diff --git a/apps/backend/internal/orchestrator/queue_send_now_test.go b/apps/backend/internal/orchestrator/queue_send_now_test.go index 46d6855995..e6038ef423 100644 --- a/apps/backend/internal/orchestrator/queue_send_now_test.go +++ b/apps/backend/internal/orchestrator/queue_send_now_test.go @@ -277,6 +277,7 @@ func TestSendQueuedNowConflictsAfterFIFOHandoffAccepted(t *testing.T) { }, } svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + svc.messageQueue.SetAutoMergeEnabled(false) svc.executor = executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) svc.messageCreator = &mockMessageCreator{} diff --git a/apps/backend/internal/orchestrator/steer_test.go b/apps/backend/internal/orchestrator/steer_test.go index 8743747ee2..789391a973 100644 --- a/apps/backend/internal/orchestrator/steer_test.go +++ b/apps/backend/internal/orchestrator/steer_test.go @@ -192,6 +192,7 @@ func TestSteerTask_DoesNotOvertakeQueueWriter(t *testing.T) { releaseInsert: make(chan struct{}), } svc.messageQueue = messagequeue.NewService(queueRepo, 10, testLogger()) + svc.messageQueue.SetAutoMergeEnabled(false) ordinaryDone := make(chan error, 1) go func() { diff --git a/apps/backend/internal/orchestrator/task_operations_auto_merge_test.go b/apps/backend/internal/orchestrator/task_operations_auto_merge_test.go new file mode 100644 index 0000000000..53a2593066 --- /dev/null +++ b/apps/backend/internal/orchestrator/task_operations_auto_merge_test.go @@ -0,0 +1,57 @@ +package orchestrator + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/orchestrator/messagequeue" + "github.com/kandev/kandev/internal/task/models" +) + +func TestQueueAndInterruptForPeerMessage_DispatchesSurvivingAutoMergedID(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + agentManager := &mockAgentManager{ + isAgentRunning: true, + repoForExecutionLookup: repo, + promptDone: make(chan struct{}), + } + service := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentManager) + service.executor = executor.NewExecutor(agentManager, repo, testLogger(), executor.ExecutorConfig{}) + service.messageCreator = &mockMessageCreator{} + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateRunning) + seedExecutorRunning(t, repo, "session1", "task1", "exec-1") + metadata := map[string]interface{}{messagequeue.MetadataSenderTaskID: "parent-task"} + target, err := service.messageQueue.QueueMessageWithMetadata( + ctx, "session1", "task1", "first parent message", "", messagequeue.QueuedByAgent, false, nil, metadata, + ) + if err != nil { + t.Fatalf("queue target: %v", err) + } + + queued, dispatched, err := service.QueueAndInterruptForPeerMessage( + ctx, "task1", "session1", "second parent message", metadata, + ) + if err != nil || !dispatched { + t.Fatalf("queue and interrupt: dispatched=%v err=%v", dispatched, err) + } + if queued == nil || queued.ID != target.ID { + t.Fatalf("returned entry = %+v, want surviving target %s", queued, target.ID) + } + select { + case <-agentManager.promptDone: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for merged prompt dispatch") + } + agentManager.mu.Lock() + prompts := append([]string(nil), agentManager.capturedPrompts...) + agentManager.mu.Unlock() + if len(prompts) != 1 || prompts[0] != "first parent message\n\nsecond parent message" { + t.Fatalf("dispatched prompts = %v", prompts) + } + if status := service.messageQueue.GetStatus(ctx, "session1"); status.Count != 0 { + t.Fatalf("merged survivor was not drained: %+v", status.Entries) + } +} diff --git a/apps/backend/internal/system/queuesettings/queuesettings_test.go b/apps/backend/internal/system/queuesettings/queuesettings_test.go index fe8fd5b1ce..05755f6bfa 100644 --- a/apps/backend/internal/system/queuesettings/queuesettings_test.go +++ b/apps/backend/internal/system/queuesettings/queuesettings_test.go @@ -25,15 +25,16 @@ func TestResolvePrecedenceAndNormalization(t *testing.T) { invalidEnv bool }{ {name: "default", want: responseFor(10, 10, SourceDefault, false, true)}, - {name: "setting", configured: &Settings{MaxPerSession: 6, MergeEnabled: true}, want: responseFor(6, 6, SourceSetting, false, true)}, - {name: "environment", configured: &Settings{MaxPerSession: 6, MergeEnabled: true}, environment: Environment{Value: "20", Present: true}, want: responseFor(6, 20, SourceEnvironment, true, true)}, - {name: "zero environment is unlimited", configured: &Settings{MaxPerSession: 6, MergeEnabled: true}, environment: Environment{Value: "0", Present: true}, want: responseFor(6, 0, SourceEnvironment, true, true)}, - {name: "negative environment is unlimited", configured: &Settings{MaxPerSession: 6, MergeEnabled: true}, environment: Environment{Value: "-3", Present: true}, want: responseFor(6, 0, SourceEnvironment, true, true)}, - {name: "invalid environment is ignored", configured: &Settings{MaxPerSession: 6, MergeEnabled: true}, environment: Environment{Value: "many", Present: true}, want: responseFor(6, 6, SourceSetting, false, true), invalidEnv: true}, - {name: "merge disabled setting", configured: &Settings{MaxPerSession: 6, MergeEnabled: false}, want: responseFor(6, 6, SourceSetting, false, false)}, + {name: "setting", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}, want: responseFor(6, 6, SourceSetting, false, true)}, + {name: "environment", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}, environment: Environment{Value: "20", Present: true}, want: responseFor(6, 20, SourceEnvironment, true, true)}, + {name: "zero environment is unlimited", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}, environment: Environment{Value: "0", Present: true}, want: responseFor(6, 0, SourceEnvironment, true, true)}, + {name: "negative environment is unlimited", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}, environment: Environment{Value: "-3", Present: true}, want: responseFor(6, 0, SourceEnvironment, true, true)}, + {name: "invalid environment is ignored", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}, environment: Environment{Value: "many", Present: true}, want: responseFor(6, 6, SourceSetting, false, true), invalidEnv: true}, + {name: "merge disabled setting", configured: &Settings{MaxPerSession: 6, MergeEnabled: false, AutoMergeEnabled: true}, want: responseFor(6, 6, SourceSetting, false, false)}, + {name: "automatic merge disabled setting", configured: &Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: false}, want: responseForAutoMerge(6, 6, SourceSetting, false, true, false)}, { name: "merge disabled setting survives an environment override of max_per_session", - configured: &Settings{MaxPerSession: 6, MergeEnabled: false}, + configured: &Settings{MaxPerSession: 6, MergeEnabled: false, AutoMergeEnabled: true}, environment: Environment{Value: "20", Present: true}, want: responseFor(6, 20, SourceEnvironment, true, false), }, @@ -65,11 +66,18 @@ func TestStoreRoundTripAndValidation(t *testing.T) { if err != nil || loaded != nil { t.Fatalf("missing load = %+v, %v", loaded, err) } - if err := store.Save(context.Background(), Settings{MaxPerSession: 8, MergeEnabled: true}); err != nil { + if err := store.Save(context.Background(), Settings{MaxPerSession: 8, MergeEnabled: true, AutoMergeEnabled: true}); err != nil { t.Fatalf("save: %v", err) } + var saved map[string]interface{} + if err := json.Unmarshal(raw.raw, &saved); err != nil { + t.Fatalf("decode saved JSON: %v", err) + } + if saved["auto_merge_enabled"] != true || len(saved) != 3 { + t.Fatalf("saved settings are not normalized: %s", raw.raw) + } loaded, err = store.Load(context.Background()) - if err != nil || loaded == nil || loaded.MaxPerSession != 8 || !loaded.MergeEnabled { + if err != nil || loaded == nil || loaded.MaxPerSession != 8 || !loaded.MergeEnabled || !loaded.AutoMergeEnabled { t.Fatalf("round trip = %+v, %v", loaded, err) } if err := store.Save(context.Background(), Settings{MaxPerSession: -1}); !errors.Is(err, ErrValidation) { @@ -92,8 +100,8 @@ func TestStoreLoadDefaultsMergeEnabledForPreExistingRecords(t *testing.T) { if err != nil { t.Fatalf("load pre-existing record: %v", err) } - if loaded == nil || loaded.MaxPerSession != 6 || !loaded.MergeEnabled { - t.Fatalf("loaded = %+v, want {MaxPerSession:6 MergeEnabled:true}", loaded) + if loaded == nil || loaded.MaxPerSession != 6 || !loaded.MergeEnabled || !loaded.AutoMergeEnabled { + t.Fatalf("loaded = %+v, want both merge settings enabled", loaded) } } @@ -109,8 +117,21 @@ func TestStoreLoadPreservesExplicitMergeDisabled(t *testing.T) { if err != nil { t.Fatalf("load explicit-false record: %v", err) } - if loaded == nil || loaded.MergeEnabled { - t.Fatalf("loaded = %+v, want MergeEnabled:false preserved", loaded) + if loaded == nil || loaded.MergeEnabled || !loaded.AutoMergeEnabled { + t.Fatalf("loaded = %+v, want manual false and legacy automatic true", loaded) + } +} + +func TestStoreLoadPreservesExplicitAutomaticMergeDisabled(t *testing.T) { + raw := &fakeRawStore{ + raw: []byte(`{"max_per_session":6,"merge_enabled":true,"auto_merge_enabled":false}`), found: true, + } + loaded, err := NewStore(raw).Load(context.Background()) + if err != nil { + t.Fatalf("load explicit automatic false: %v", err) + } + if loaded == nil || loaded.AutoMergeEnabled || !loaded.MergeEnabled { + t.Fatalf("loaded = %+v, want automatic false and manual true", loaded) } } @@ -135,6 +156,12 @@ func TestServiceUpdatePersistsBeforeLiveApply(t *testing.T) { if target.max != 4 { t.Fatalf("target applied before failed save: %d", target.max) } + if _, err := service.Update(context.Background(), SettingsPatch{AutoMergeEnabled: new(false)}); err == nil { + t.Fatal("expected automatic merge persistence error") + } + if !target.AutoMergeEnabled() { + t.Fatal("automatic merge applied before failed save") + } } // TestServiceUpdatePatchPreservesUnspecifiedFields guards against a patch @@ -157,6 +184,9 @@ func TestServiceUpdatePatchPreservesUnspecifiedFields(t *testing.T) { if !response.Settings.MergeEnabled || !target.MergeEnabled() { t.Fatalf("max-only patch reset merge_enabled: response=%+v target=%v", response, target.MergeEnabled()) } + if !response.Settings.AutoMergeEnabled || !target.AutoMergeEnabled() { + t.Fatalf("max-only patch reset auto_merge_enabled: response=%+v target=%v", response, target.AutoMergeEnabled()) + } response, err = service.Update(context.Background(), SettingsPatch{MergeEnabled: new(false)}) if err != nil { @@ -168,6 +198,20 @@ func TestServiceUpdatePatchPreservesUnspecifiedFields(t *testing.T) { if response.Settings.MergeEnabled || target.MergeEnabled() { t.Fatalf("merge-only patch did not apply: response=%+v target=%v", response, target.MergeEnabled()) } + if !response.Settings.AutoMergeEnabled || !target.AutoMergeEnabled() { + t.Fatalf("manual merge patch reset auto merge: response=%+v target=%v", response, target.AutoMergeEnabled()) + } + + response, err = service.Update(context.Background(), SettingsPatch{AutoMergeEnabled: new(false)}) + if err != nil { + t.Fatalf("auto-only update: %v", err) + } + if response.Settings.MaxPerSession != 4 || response.Settings.MergeEnabled || response.Settings.AutoMergeEnabled { + t.Fatalf("auto-only update clobbered other fields: %+v", response) + } + if target.MaxPerSession() != 4 || target.MergeEnabled() || target.AutoMergeEnabled() { + t.Fatalf("auto-only live apply = max:%d manual:%v auto:%v", target.MaxPerSession(), target.MergeEnabled(), target.AutoMergeEnabled()) + } } func TestServiceRecoversFromInvalidPersistedSettingAndAllowsReplacement(t *testing.T) { @@ -284,6 +328,9 @@ func TestServiceEnvironmentLockAllowsMergeOnlyUpdate(t *testing.T) { if response.Settings.MergeEnabled || target.MergeEnabled() { t.Fatalf("merge-only update did not apply: response=%+v target=%v", response, target.MergeEnabled()) } + if target.MaxPerSession() != 20 || response.Effective.MaxPerSession != 20 { + t.Fatalf("merge-only update lost environment capacity: response=%+v target=%d", response, target.MaxPerSession()) + } } func TestHandlerReturnsConflictForEnvironmentLock(t *testing.T) { @@ -310,7 +357,7 @@ func TestHandlerGetReturnsConfiguredAndEffectiveValues(t *testing.T) { gin.SetMode(gin.TestMode) raw := &fakeRawStore{} store := NewStore(raw) - if err := store.Save(context.Background(), Settings{MaxPerSession: 6, MergeEnabled: true}); err != nil { + if err := store.Save(context.Background(), Settings{MaxPerSession: 6, MergeEnabled: true, AutoMergeEnabled: true}); err != nil { t.Fatalf("save baseline: %v", err) } service := NewService(store, &fakeTarget{max: 20}, func() Environment { @@ -337,6 +384,64 @@ func TestHandlerGetReturnsConfiguredAndEffectiveValues(t *testing.T) { } } +func TestHandlerGetDefaultsAutomaticMergeEnabled(t *testing.T) { + gin.SetMode(gin.TestMode) + service := NewService( + NewStore(&fakeRawStore{}), &fakeTarget{max: 10}, + func() Environment { return Environment{} }, testLogger(t), + ) + router := gin.New() + group := router.Group("/api/v1/system") + RegisterRoutes(group, group, service) + + response := httptest.NewRecorder() + router.ServeHTTP(response, httptest.NewRequest( + http.MethodGet, "/api/v1/system/message-queue/settings", nil, + )) + if response.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", response.Code, response.Body.String()) + } + var payload map[string]map[string]interface{} + if err := json.Unmarshal(response.Body.Bytes(), &payload); err != nil { + t.Fatalf("decode response: %v", err) + } + if payload["settings"]["auto_merge_enabled"] != true || payload["effective"]["auto_merge_enabled"] != true { + t.Fatalf("automatic merge defaults missing from response: %s", response.Body.String()) + } +} + +func TestHandlerAutoOnlyPatchPreservesEnvironmentCapacity(t *testing.T) { + gin.SetMode(gin.TestMode) + target := &fakeTarget{max: 20, mergeEnabled: true} + service := NewService( + NewStore(&fakeRawStore{}), target, + func() Environment { return Environment{Value: "20", Present: true} }, testLogger(t), + ) + router := gin.New() + group := router.Group("/api/v1/system") + RegisterRoutes(group, group, service) + request := httptest.NewRequest( + http.MethodPatch, "/api/v1/system/message-queue/settings", + bytes.NewBufferString(`{"auto_merge_enabled":false}`), + ) + request.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + if response.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", response.Code, response.Body.String()) + } + if target.MaxPerSession() != 20 { + t.Fatalf("auto-only PATCH changed live environment capacity to %d", target.MaxPerSession()) + } + var payload map[string]map[string]interface{} + if err := json.Unmarshal(response.Body.Bytes(), &payload); err != nil { + t.Fatalf("decode response: %v", err) + } + if payload["settings"]["auto_merge_enabled"] != false || payload["effective"]["max_per_session"] != float64(20) { + t.Fatalf("auto-only PATCH response = %s", response.Body.String()) + } +} + func TestHandlerRejectsNegativeCapacity(t *testing.T) { gin.SetMode(gin.TestMode) router := gin.New() @@ -394,9 +499,18 @@ func TestHandlerUpdateOmittingMergeEnabledPreservesIt(t *testing.T) { } func responseFor(configured, effective int, source Source, locked bool, mergeEnabled bool) Response { + return responseForAutoMerge(configured, effective, source, locked, mergeEnabled, true) +} + +func responseForAutoMerge(configured, effective int, source Source, locked, mergeEnabled, autoMergeEnabled bool) Response { return Response{ - Settings: Settings{MaxPerSession: configured, MergeEnabled: mergeEnabled}, - Effective: Effective{MaxPerSession: effective, Source: source, Locked: locked, MergeEnabled: mergeEnabled}, + Settings: Settings{ + MaxPerSession: configured, MergeEnabled: mergeEnabled, AutoMergeEnabled: autoMergeEnabled, + }, + Effective: Effective{ + MaxPerSession: effective, Source: source, Locked: locked, + MergeEnabled: mergeEnabled, AutoMergeEnabled: autoMergeEnabled, + }, } } @@ -422,14 +536,17 @@ func (f *fakeRawStore) Save(_ context.Context, _ string, value []byte) error { } type fakeTarget struct { - max int - mergeEnabled bool + max int + mergeEnabled bool + autoMergeEnabled bool } -func (f *fakeTarget) MaxPerSession() int { return f.max } -func (f *fakeTarget) SetMaxPerSession(n int) { f.max = n } -func (f *fakeTarget) MergeEnabled() bool { return f.mergeEnabled } -func (f *fakeTarget) SetMergeEnabled(v bool) { f.mergeEnabled = v } +func (f *fakeTarget) MaxPerSession() int { return f.max } +func (f *fakeTarget) SetMaxPerSession(n int) { f.max = n } +func (f *fakeTarget) MergeEnabled() bool { return f.mergeEnabled } +func (f *fakeTarget) SetMergeEnabled(v bool) { f.mergeEnabled = v } +func (f *fakeTarget) AutoMergeEnabled() bool { return f.autoMergeEnabled } +func (f *fakeTarget) SetAutoMergeEnabled(v bool) { f.autoMergeEnabled = v } type blockingRawStore struct { mu sync.Mutex @@ -480,14 +597,17 @@ func (s *blockingRawStore) Save(_ context.Context, _ string, value []byte) error } type atomicTarget struct { - max atomic.Int64 - mergeEnabled atomic.Bool + max atomic.Int64 + mergeEnabled atomic.Bool + autoMergeEnabled atomic.Bool } -func (t *atomicTarget) MaxPerSession() int { return int(t.max.Load()) } -func (t *atomicTarget) SetMaxPerSession(n int) { t.max.Store(int64(n)) } -func (t *atomicTarget) MergeEnabled() bool { return t.mergeEnabled.Load() } -func (t *atomicTarget) SetMergeEnabled(v bool) { t.mergeEnabled.Store(v) } +func (t *atomicTarget) MaxPerSession() int { return int(t.max.Load()) } +func (t *atomicTarget) SetMaxPerSession(n int) { t.max.Store(int64(n)) } +func (t *atomicTarget) MergeEnabled() bool { return t.mergeEnabled.Load() } +func (t *atomicTarget) SetMergeEnabled(v bool) { t.mergeEnabled.Store(v) } +func (t *atomicTarget) AutoMergeEnabled() bool { return t.autoMergeEnabled.Load() } +func (t *atomicTarget) SetAutoMergeEnabled(v bool) { t.autoMergeEnabled.Store(v) } func testLogger(t *testing.T) *logger.Logger { t.Helper() diff --git a/apps/backend/internal/system/queuesettings/resolver.go b/apps/backend/internal/system/queuesettings/resolver.go index 2fc3a47864..f2794ecfc9 100644 --- a/apps/backend/internal/system/queuesettings/resolver.go +++ b/apps/backend/internal/system/queuesettings/resolver.go @@ -21,9 +21,10 @@ func Resolve(configured *Settings, environment Environment) (Resolution, error) resolution := Resolution{Response: Response{ Settings: settings, Effective: Effective{ - MaxPerSession: settings.MaxPerSession, - Source: source, - MergeEnabled: settings.MergeEnabled, + MaxPerSession: settings.MaxPerSession, + Source: source, + MergeEnabled: settings.MergeEnabled, + AutoMergeEnabled: settings.AutoMergeEnabled, }, }} raw := strings.TrimSpace(environment.Value) @@ -39,10 +40,11 @@ func Resolve(configured *Settings, environment Environment) (Resolution, error) value = 0 } resolution.Effective = Effective{ - MaxPerSession: value, - Source: SourceEnvironment, - Locked: true, - MergeEnabled: settings.MergeEnabled, + MaxPerSession: value, + Source: SourceEnvironment, + Locked: true, + MergeEnabled: settings.MergeEnabled, + AutoMergeEnabled: settings.AutoMergeEnabled, } return resolution, nil } diff --git a/apps/backend/internal/system/queuesettings/service.go b/apps/backend/internal/system/queuesettings/service.go index 81fa456046..2ba61f2837 100644 --- a/apps/backend/internal/system/queuesettings/service.go +++ b/apps/backend/internal/system/queuesettings/service.go @@ -15,6 +15,8 @@ type Target interface { SetMaxPerSession(int) MergeEnabled() bool SetMergeEnabled(bool) + AutoMergeEnabled() bool + SetAutoMergeEnabled(bool) } type EnvironmentReader func() Environment @@ -59,7 +61,7 @@ func (s *Service) Get(ctx context.Context) (Response, error) { // omits keep their current effective value rather than resetting to zero, so // a client that edits only one field can never silently clobber another (see // SettingsPatch). The environment lock only blocks a patch that actually -// attempts to change max_per_session — it does not control merge_enabled. +// attempts to change max_per_session. It controls neither merge setting. func (s *Service) Update(ctx context.Context, patch SettingsPatch) (Response, error) { s.mu.Lock() defer s.mu.Unlock() @@ -87,12 +89,13 @@ func (s *Service) Update(ctx context.Context, patch SettingsPatch) (Response, er if err := s.store.Save(ctx, settings); err != nil { return Response{}, err } - s.target.SetMaxPerSession(settings.MaxPerSession) - s.target.SetMergeEnabled(settings.MergeEnabled) updated, err := Resolve(&settings, environment) if err != nil { return Response{}, err } + s.target.SetMaxPerSession(updated.Effective.MaxPerSession) + s.target.SetMergeEnabled(updated.Effective.MergeEnabled) + s.target.SetAutoMergeEnabled(updated.Effective.AutoMergeEnabled) return updated.Response, nil } diff --git a/apps/backend/internal/system/queuesettings/store.go b/apps/backend/internal/system/queuesettings/store.go index 943d24aed1..f2cbee7548 100644 --- a/apps/backend/internal/system/queuesettings/store.go +++ b/apps/backend/internal/system/queuesettings/store.go @@ -19,25 +19,28 @@ func NewStore(raw RawStore) *Store { return &Store{raw: raw} } -// storedSettings mirrors Settings for JSON decoding but keeps MergeEnabled as -// a pointer so a missing key ("unset" — a record saved before this field -// existed) can be told apart from an explicit `false`. Without this, an -// installation that persisted max_per_session before merge_enabled shipped -// would decode its stale record as merge_enabled=false and silently disable -// merging on upgrade instead of leaving it enabled by default. +// storedSettings mirrors Settings for JSON decoding but keeps both merge +// settings as pointers. Missing keys in records written by older versions +// therefore default on, while explicit false values remain false. type storedSettings struct { - MaxPerSession int `json:"max_per_session"` - MergeEnabled *bool `json:"merge_enabled"` + MaxPerSession int `json:"max_per_session"` + MergeEnabled *bool `json:"merge_enabled"` + AutoMergeEnabled *bool `json:"auto_merge_enabled"` } -// toSettings normalizes a decoded record into Settings, defaulting -// MergeEnabled to true when the persisted JSON has no merge_enabled key. +// toSettings normalizes a decoded record, defaulting omitted merge settings on. func (s storedSettings) toSettings() Settings { mergeEnabled := true if s.MergeEnabled != nil { mergeEnabled = *s.MergeEnabled } - return Settings{MaxPerSession: s.MaxPerSession, MergeEnabled: mergeEnabled} + autoMergeEnabled := true + if s.AutoMergeEnabled != nil { + autoMergeEnabled = *s.AutoMergeEnabled + } + return Settings{ + MaxPerSession: s.MaxPerSession, MergeEnabled: mergeEnabled, AutoMergeEnabled: autoMergeEnabled, + } } func (s *Store) Load(ctx context.Context) (*Settings, error) { diff --git a/apps/backend/internal/system/queuesettings/types.go b/apps/backend/internal/system/queuesettings/types.go index b31d97ee7d..2764c93786 100644 --- a/apps/backend/internal/system/queuesettings/types.go +++ b/apps/backend/internal/system/queuesettings/types.go @@ -29,17 +29,19 @@ var ( ) type Settings struct { - MaxPerSession int `json:"max_per_session"` - MergeEnabled bool `json:"merge_enabled"` + MaxPerSession int `json:"max_per_session"` + MergeEnabled bool `json:"merge_enabled"` + AutoMergeEnabled bool `json:"auto_merge_enabled"` } // SettingsPatch is a partial update to Settings: a nil field means "leave // unchanged" rather than "reset to zero value". Without this distinction a -// client that PATCHes only max_per_session would silently reset -// merge_enabled to false, since Settings has no way to represent omission. +// client that PATCHes one field would otherwise silently reset either merge +// setting to false, since Settings has no way to represent omission. type SettingsPatch struct { - MaxPerSession *int `json:"max_per_session"` - MergeEnabled *bool `json:"merge_enabled"` + MaxPerSession *int `json:"max_per_session"` + MergeEnabled *bool `json:"merge_enabled"` + AutoMergeEnabled *bool `json:"auto_merge_enabled"` } // Apply returns base with every non-nil patch field overlaid. @@ -50,6 +52,9 @@ func (p SettingsPatch) Apply(base Settings) Settings { if p.MergeEnabled != nil { base.MergeEnabled = *p.MergeEnabled } + if p.AutoMergeEnabled != nil { + base.AutoMergeEnabled = *p.AutoMergeEnabled + } return base } @@ -66,6 +71,9 @@ type Effective struct { // it has no environment override, so there is no separate source/lock to // track. MergeEnabled bool `json:"merge_enabled"` + // AutoMergeEnabled mirrors Settings.AutoMergeEnabled and has no + // environment override. + AutoMergeEnabled bool `json:"auto_merge_enabled"` } type Environment struct { @@ -78,10 +86,9 @@ type Resolution struct { InvalidEnvironment bool } -// DefaultSettings returns the shipped defaults: no session-count cap and -// merging enabled. +// DefaultSettings returns the shipped queue cap with both merge modes enabled. func DefaultSettings() Settings { - return Settings{MaxPerSession: DefaultMaxPerSession, MergeEnabled: true} + return Settings{MaxPerSession: DefaultMaxPerSession, MergeEnabled: true, AutoMergeEnabled: true} } func Validate(settings Settings) error { diff --git a/apps/backend/internal/system/system_routes_test.go b/apps/backend/internal/system/system_routes_test.go index 2653e2b00e..da294e9fbf 100644 --- a/apps/backend/internal/system/system_routes_test.go +++ b/apps/backend/internal/system/system_routes_test.go @@ -158,14 +158,17 @@ func (s *queueSettingsRawStore) Save(_ context.Context, _ string, value []byte) } type queueSettingsTarget struct { - max int - mergeEnabled bool + max int + mergeEnabled bool + autoMergeEnabled bool } -func (t *queueSettingsTarget) MaxPerSession() int { return t.max } -func (t *queueSettingsTarget) SetMaxPerSession(n int) { t.max = n } -func (t *queueSettingsTarget) MergeEnabled() bool { return t.mergeEnabled } -func (t *queueSettingsTarget) SetMergeEnabled(v bool) { t.mergeEnabled = v } +func (t *queueSettingsTarget) MaxPerSession() int { return t.max } +func (t *queueSettingsTarget) SetMaxPerSession(n int) { t.max = n } +func (t *queueSettingsTarget) MergeEnabled() bool { return t.mergeEnabled } +func (t *queueSettingsTarget) SetMergeEnabled(v bool) { t.mergeEnabled = v } +func (t *queueSettingsTarget) AutoMergeEnabled() bool { return t.autoMergeEnabled } +func (t *queueSettingsTarget) SetAutoMergeEnabled(v bool) { t.autoMergeEnabled = v } type systemSleepRawStore struct { raw []byte diff --git a/apps/web/components/settings/system/message-queue-settings.test.tsx b/apps/web/components/settings/system/message-queue-settings.test.tsx index c79c6da53a..8f9d34f0dd 100644 --- a/apps/web/components/settings/system/message-queue-settings.test.tsx +++ b/apps/web/components/settings/system/message-queue-settings.test.tsx @@ -9,7 +9,10 @@ let saveContributor: SettingsSaveContributor | null = null; let currentRole: "admin" | "member" | undefined; const MAXIMUM_LABEL = "Maximum messages per session"; const MERGE_TOGGLE_LABEL = "Enable queued message merging"; +const AUTO_MERGE_TOGGLE_LABEL = "Automatically merge consecutive messages"; const EFFECTIVE_VALUE_ID = "message-queue-effective-value"; +const ARIA_PRESSED = "aria-pressed"; +const SAVE_FAILED = "save failed"; vi.mock("@/lib/api/domains/settings-api", () => ({ fetchMessageQueueSettings: (...args: unknown[]) => fetchSettingsMock(...args), @@ -51,16 +54,35 @@ vi.mock("@kandev/ui/switch", () => ({ import { MessageQueueSettings } from "./message-queue-settings"; -function response( - configured = 10, - effective = configured, - source: MessageQueueSettingsResponse["effective"]["source"] = "default", - locked = false, - mergeEnabled = true, -): MessageQueueSettingsResponse { +type ResponseOverrides = { + configured?: number; + effective?: number; + source?: MessageQueueSettingsResponse["effective"]["source"]; + locked?: boolean; + mergeEnabled?: boolean; + autoMergeEnabled?: boolean; +}; + +function response(overrides: ResponseOverrides = {}): MessageQueueSettingsResponse { + const configured = overrides.configured ?? 10; + const effective = overrides.effective ?? configured; + const source = overrides.source ?? "default"; + const locked = overrides.locked ?? false; + const mergeEnabled = overrides.mergeEnabled ?? true; + const autoMergeEnabled = overrides.autoMergeEnabled ?? true; return { - settings: { max_per_session: configured, merge_enabled: mergeEnabled }, - effective: { max_per_session: effective, source, locked, merge_enabled: mergeEnabled }, + settings: { + max_per_session: configured, + merge_enabled: mergeEnabled, + auto_merge_enabled: autoMergeEnabled, + }, + effective: { + max_per_session: effective, + source, + locked, + merge_enabled: mergeEnabled, + auto_merge_enabled: autoMergeEnabled, + }, }; } @@ -76,6 +98,10 @@ function mergeToggle(): HTMLElement { return screen.getByRole("button", { name: MERGE_TOGGLE_LABEL }); } +function autoMergeToggle(): HTMLElement { + return screen.getByRole("button", { name: AUTO_MERGE_TOGGLE_LABEL }); +} + beforeEach(() => { fetchSettingsMock.mockReset(); updateSettingsMock.mockReset(); @@ -110,7 +136,9 @@ describe("MessageQueueSettings — per-session limit", () => { }); it("explains unlimited mode and keeps a mobile-safe single-column surface", async () => { - fetchSettingsMock.mockResolvedValueOnce(response(0, 0, "setting")); + fetchSettingsMock.mockResolvedValueOnce( + response({ configured: 0, effective: 0, source: "setting" }), + ); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); @@ -123,7 +151,9 @@ describe("MessageQueueSettings — per-session limit", () => { }); it("stages an admin edit until the shared contributor saves it", async () => { - updateSettingsMock.mockResolvedValueOnce(response(25, 25, "setting")); + updateSettingsMock.mockResolvedValueOnce( + response({ configured: 25, effective: 25, source: "setting" }), + ); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); @@ -154,7 +184,9 @@ describe("MessageQueueSettings — per-session limit", () => { }); it("shows the effective environment value and locks editing", async () => { - fetchSettingsMock.mockResolvedValueOnce(response(25, 50, "environment", true)); + fetchSettingsMock.mockResolvedValueOnce( + response({ configured: 25, effective: 50, source: "environment", locked: true }), + ); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); @@ -164,7 +196,9 @@ describe("MessageQueueSettings — per-session limit", () => { expect(screen.getByTestId("message-queue-source").textContent).toBe("Environment"); expect(screen.getByText(/KANDEV_QUEUE_MAX_PER_SESSION/)).toBeTruthy(); }); +}); +describe("MessageQueueSettings — permissions and recovery", () => { it("keeps members read-only while showing the setting", async () => { currentRole = "member"; render(); @@ -186,14 +220,14 @@ describe("MessageQueueSettings — per-session limit", () => { }); it("reports save failure and keeps the draft dirty", async () => { - updateSettingsMock.mockRejectedValueOnce(new Error("save failed")); + updateSettingsMock.mockRejectedValueOnce(new Error(SAVE_FAILED)); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); fireEvent.change(input, { target: { value: "20" } }); const contributor = requireContributor(); await act(async () => { - await expect(contributor.save(contributor.revision)).rejects.toThrow("save failed"); + await expect(contributor.save(contributor.revision)).rejects.toThrow(SAVE_FAILED); }); expect(screen.getByText("Failed to save message queue settings.")).toBeTruthy(); @@ -201,7 +235,9 @@ describe("MessageQueueSettings — per-session limit", () => { }); it("discards a draft back to the authoritative configured value", async () => { - fetchSettingsMock.mockResolvedValueOnce(response(12, 12, "setting")); + fetchSettingsMock.mockResolvedValueOnce( + response({ configured: 12, effective: 12, source: "setting" }), + ); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); fireEvent.change(input, { target: { value: "30" } }); @@ -230,7 +266,7 @@ describe("MessageQueueSettings — merge toggle", () => { render(); await screen.findByLabelText(MAXIMUM_LABEL); - expect(mergeToggle().getAttribute("aria-pressed")).toBe("true"); + expect(mergeToggle().getAttribute(ARIA_PRESSED)).toBe("true"); expect( screen.getByText(/Lets you fold a queued message into the message directly above it/), ).toBeTruthy(); @@ -243,7 +279,7 @@ describe("MessageQueueSettings — merge toggle", () => { }); it("stages a toggle change and PATCHes merge_enabled without touching max_per_session", async () => { - updateSettingsMock.mockResolvedValueOnce(response(10, 10, "default", false, false)); + updateSettingsMock.mockResolvedValueOnce(response({ mergeEnabled: false })); render(); await screen.findByLabelText(MAXIMUM_LABEL); @@ -257,11 +293,13 @@ describe("MessageQueueSettings — merge toggle", () => { expect(updateSettingsMock).toHaveBeenCalledWith({ merge_enabled: false }); await waitFor(() => expect(saveContributor?.isDirty).toBe(false)); - expect(mergeToggle().getAttribute("aria-pressed")).toBe("false"); + expect(mergeToggle().getAttribute(ARIA_PRESSED)).toBe("false"); }); it("sends both fields in one PATCH when max_per_session and merge_enabled are both dirty", async () => { - updateSettingsMock.mockResolvedValueOnce(response(25, 25, "setting", false, false)); + updateSettingsMock.mockResolvedValueOnce( + response({ configured: 25, effective: 25, source: "setting", mergeEnabled: false }), + ); render(); const input = await screen.findByLabelText(MAXIMUM_LABEL); @@ -278,7 +316,9 @@ describe("MessageQueueSettings — merge toggle", () => { }); it("stays editable for admins even when max_per_session is environment-locked", async () => { - fetchSettingsMock.mockResolvedValueOnce(response(25, 50, "environment", true, true)); + fetchSettingsMock.mockResolvedValueOnce( + response({ configured: 25, effective: 50, source: "environment", locked: true }), + ); render(); await screen.findByLabelText(MAXIMUM_LABEL); @@ -294,7 +334,7 @@ describe("MessageQueueSettings — merge toggle", () => { }); it("discards a toggle change back to the authoritative value", async () => { - fetchSettingsMock.mockResolvedValueOnce(response(10, 10, "default", false, true)); + fetchSettingsMock.mockResolvedValueOnce(response()); render(); await screen.findByLabelText(MAXIMUM_LABEL); fireEvent.click(mergeToggle()); @@ -302,7 +342,128 @@ describe("MessageQueueSettings — merge toggle", () => { act(() => contributor.discard()); - expect(mergeToggle().getAttribute("aria-pressed")).toBe("true"); + expect(mergeToggle().getAttribute(ARIA_PRESSED)).toBe("true"); expect(saveContributor?.isDirty).toBe(false); }); }); + +describe("MessageQueueSettings — automatic merge toggle", () => { + it("is on by default and explains separate-message fallback", async () => { + render(); + + const toggle = await screen.findByRole("button", { name: AUTO_MERGE_TOGGLE_LABEL }); + expect(toggle.getAttribute(ARIA_PRESSED)).toBe("true"); + expect(screen.getByText(/stays as a separate queued message/)).toBeTruthy(); + const touchTarget = screen.getByTestId("message-queue-auto-merge-touch-target"); + expect(touchTarget.className).toContain("min-h-11"); + expect(touchTarget.className).toContain("min-w-11"); + }); + + it("saves only auto_merge_enabled when it is the only changed draft", async () => { + updateSettingsMock.mockResolvedValueOnce(response({ autoMergeEnabled: false })); + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + fireEvent.click(autoMergeToggle()); + const contributor = requireContributor(); + + await act(async () => contributor.save(contributor.revision)); + + expect(updateSettingsMock).toHaveBeenCalledWith({ auto_merge_enabled: false }); + await waitFor(() => expect(saveContributor?.isDirty).toBe(false)); + }); + + it("saves capacity, manual merge, and automatic merge in one PATCH", async () => { + updateSettingsMock.mockResolvedValueOnce( + response({ + configured: 25, + effective: 25, + source: "setting", + mergeEnabled: false, + autoMergeEnabled: false, + }), + ); + render(); + const input = await screen.findByLabelText(MAXIMUM_LABEL); + fireEvent.change(input, { target: { value: "25" } }); + fireEvent.click(mergeToggle()); + fireEvent.click(autoMergeToggle()); + const contributor = requireContributor(); + + await act(async () => contributor.save(contributor.revision)); + + expect(updateSettingsMock).toHaveBeenCalledWith({ + max_per_session: 25, + merge_enabled: false, + auto_merge_enabled: false, + }); + }); + + it("stays editable for admins under a capacity environment lock", async () => { + fetchSettingsMock.mockResolvedValueOnce( + response({ configured: 25, effective: 50, source: "environment", locked: true }), + ); + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + + expect(autoMergeToggle()).toHaveProperty("disabled", false); + }); + + it("is read-only for members", async () => { + currentRole = "member"; + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + + expect(autoMergeToggle()).toHaveProperty("disabled", true); + }); + + it("discards back to the authoritative automatic-merge value", async () => { + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + fireEvent.click(autoMergeToggle()); + const contributor = requireContributor(); + + act(() => contributor.discard()); + + expect(autoMergeToggle().getAttribute(ARIA_PRESSED)).toBe("true"); + expect(saveContributor?.isDirty).toBe(false); + }); + + it("does not overwrite a newer local toggle change when a save resolves", async () => { + let resolveSave: (value: MessageQueueSettingsResponse) => void = () => {}; + updateSettingsMock.mockReturnValueOnce( + new Promise((resolve) => { + resolveSave = resolve; + }), + ); + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + fireEvent.click(autoMergeToggle()); + const submittedContributor = requireContributor(); + let savePromise: Promise = Promise.resolve(); + + act(() => { + savePromise = Promise.resolve(submittedContributor.save(submittedContributor.revision)); + }); + fireEvent.click(autoMergeToggle()); + resolveSave(response({ source: "setting", autoMergeEnabled: false })); + await act(async () => savePromise); + + expect(autoMergeToggle().getAttribute(ARIA_PRESSED)).toBe("true"); + expect(saveContributor?.isDirty).toBe(true); + }); + + it("keeps a failed automatic-merge draft dirty and reports the error", async () => { + updateSettingsMock.mockRejectedValueOnce(new Error(SAVE_FAILED)); + render(); + await screen.findByLabelText(MAXIMUM_LABEL); + fireEvent.click(autoMergeToggle()); + const contributor = requireContributor(); + + await act(async () => { + await expect(contributor.save(contributor.revision)).rejects.toThrow(SAVE_FAILED); + }); + + expect(screen.getByText("Failed to save message queue settings.")).toBeTruthy(); + expect(saveContributor?.isDirty).toBe(true); + }); +}); diff --git a/apps/web/components/settings/system/message-queue-settings.tsx b/apps/web/components/settings/system/message-queue-settings.tsx index 39aba7439a..d42c5e9d17 100644 --- a/apps/web/components/settings/system/message-queue-settings.tsx +++ b/apps/web/components/settings/system/message-queue-settings.tsx @@ -43,14 +43,15 @@ function sourceLabelKey(source: MessageQueueSettingsSource): string { } } -/** Loads the persisted snapshot and owns the two editable drafts (the - * max-per-session text field and the merge-enabled toggle). Split out of +/** Loads the persisted snapshot and owns the editable queue-setting drafts. + * Split out of * `useMessageQueueSettingsDraft` so neither function needs to grow past the * project's per-function line limit as the page gains fields. */ function useMessageQueueSettingsLoad() { const [snapshot, setSnapshot] = useState(null); const [draft, setDraft] = useState(""); const [mergeDraft, setMergeDraft] = useState(true); + const [autoMergeDraft, setAutoMergeDraft] = useState(true); const [loading, setLoading] = useState(true); const [loadFailed, setLoadFailed] = useState(false); const loadVersion = useRef(0); @@ -65,6 +66,7 @@ function useMessageQueueSettingsLoad() { setSnapshot(response); setDraft(String(response.settings.max_per_session)); setMergeDraft(response.settings.merge_enabled); + setAutoMergeDraft(response.settings.auto_merge_enabled); } catch { if (version === loadVersion.current) setLoadFailed(true); } finally { @@ -86,58 +88,71 @@ function useMessageQueueSettingsLoad() { setDraft, mergeDraft, setMergeDraft, + autoMergeDraft, + setAutoMergeDraft, loading, loadFailed, reload, }; } -/** Derives dirty/validity/permission state from the loaded snapshot and the - * two drafts, and registers the combined save/discard contributor that the - * page's floating save bar drives. */ -function useMessageQueueSettingsDraft() { - const { t } = useTranslation(); - const role = useAppStore((state) => state.auth.user?.role); - const [saveFailed, setSaveFailed] = useState(false); - const load = useMessageQueueSettingsLoad(); - const { snapshot, setSnapshot, draft, setDraft, mergeDraft, setMergeDraft } = load; +type QueueSettingsContributorOptions = { + load: ReturnType; + parsed: number | null; + isMaxDirty: boolean; + isMergeDirty: boolean; + isAutoMergeDirty: boolean; + isAdmin: boolean; + isLocked: boolean; + invalidReason: string | undefined; + onSaveFailed: (failed: boolean) => void; +}; - const parsed = parseMaximum(draft); +/** Registers all queue drafts as one atomic settings-page contributor. */ +function useQueueSettingsContributor({ + load, + parsed, + isMaxDirty, + isMergeDirty, + isAutoMergeDirty, + isAdmin, + isLocked, + invalidReason, + onSaveFailed, +}: QueueSettingsContributorOptions) { + const { + snapshot, + setSnapshot, + draft, + setDraft, + mergeDraft, + setMergeDraft, + autoMergeDraft, + setAutoMergeDraft, + } = load; const baseline = snapshot?.settings.max_per_session; const mergeBaseline = snapshot?.settings.merge_enabled; - const isMaxDirty = baseline !== undefined && draft !== String(baseline); - const isMergeDirty = mergeBaseline !== undefined && mergeDraft !== mergeBaseline; - const isDirty = isMaxDirty || isMergeDirty; - const isAdmin = role === undefined || role === "admin"; - const isLocked = snapshot?.effective.locked === true; - // The environment lock only governs max_per_session; merging has no - // environment override, so an admin may always toggle it. - const canEdit = isAdmin && !isLocked; - const canEditMerge = isAdmin; - let invalidReason: string | undefined; - if (parsed === null) invalidReason = t("system:messageQueueValidation"); - else if (!isAdmin) invalidReason = t("system:messageQueueAdminOnly"); - else if (isLocked && isMaxDirty) { - invalidReason = t("system:messageQueueEnvironmentLocked", { variable: ENVIRONMENT_VARIABLE }); - } - + const autoMergeBaseline = snapshot?.settings.auto_merge_enabled; + const isDirty = isMaxDirty || isMergeDirty || isAutoMergeDirty; useSettingsSaveContributor({ id: "system-message-queue", - revision: `${draft}:${mergeDraft}`, + revision: `${draft}:${mergeDraft}:${autoMergeDraft}`, isDirty, - canSave: parsed !== null && (isMaxDirty ? canEdit : canEditMerge), + canSave: parsed !== null && isAdmin && (!isMaxDirty || !isLocked), invalidReason, save: async () => { - if (parsed === null || (isMaxDirty && !canEdit) || (isMergeDirty && !canEditMerge)) { + if (parsed === null || !isAdmin || (isMaxDirty && isLocked)) { throw new Error(invalidReason); } const submitted = draft; const submittedMerge = mergeDraft; - setSaveFailed(false); + const submittedAutoMerge = autoMergeDraft; + onSaveFailed(false); try { const response = await updateMessageQueueSettings({ ...(isMaxDirty ? { max_per_session: parsed } : {}), ...(isMergeDirty ? { merge_enabled: mergeDraft } : {}), + ...(isAutoMergeDirty ? { auto_merge_enabled: autoMergeDraft } : {}), }); setSnapshot(response); setDraft((current) => @@ -146,28 +161,70 @@ function useMessageQueueSettingsDraft() { setMergeDraft((current) => current === submittedMerge ? response.settings.merge_enabled : current, ); + setAutoMergeDraft((current) => + current === submittedAutoMerge ? response.settings.auto_merge_enabled : current, + ); } catch (error) { - setSaveFailed(true); + onSaveFailed(true); throw error; } }, discard: () => { if (baseline !== undefined) setDraft(String(baseline)); if (mergeBaseline !== undefined) setMergeDraft(mergeBaseline); - setSaveFailed(false); + if (autoMergeBaseline !== undefined) setAutoMergeDraft(autoMergeBaseline); + onSaveFailed(false); }, }); +} + +/** Derives dirty, validation, and permission state for all queue drafts. */ +function useMessageQueueSettingsDraft() { + const { t } = useTranslation(); + const role = useAppStore((state) => state.auth.user?.role); + const [saveFailed, setSaveFailed] = useState(false); + const load = useMessageQueueSettingsLoad(); + const { snapshot, draft, mergeDraft, autoMergeDraft } = load; + const parsed = parseMaximum(draft); + const baseline = snapshot?.settings.max_per_session; + const mergeBaseline = snapshot?.settings.merge_enabled; + const autoMergeBaseline = snapshot?.settings.auto_merge_enabled; + const isMaxDirty = baseline !== undefined && draft !== String(baseline); + const isMergeDirty = mergeBaseline !== undefined && mergeDraft !== mergeBaseline; + const isAutoMergeDirty = autoMergeBaseline !== undefined && autoMergeDraft !== autoMergeBaseline; + const isAdmin = role === undefined || role === "admin"; + const isLocked = snapshot?.effective.locked === true; + let invalidReason: string | undefined; + if (parsed === null) invalidReason = t("system:messageQueueValidation"); + else if (!isAdmin) invalidReason = t("system:messageQueueAdminOnly"); + else if (isLocked && isMaxDirty) { + invalidReason = t("system:messageQueueEnvironmentLocked", { variable: ENVIRONMENT_VARIABLE }); + } + + useQueueSettingsContributor({ + load, + parsed, + isMaxDirty, + isMergeDirty, + isAutoMergeDirty, + isAdmin, + isLocked, + invalidReason, + onSaveFailed: setSaveFailed, + }); return { snapshot, draft, - setDraft, + setDraft: load.setDraft, mergeDraft, - setMergeDraft, + setMergeDraft: load.setMergeDraft, + autoMergeDraft, + setAutoMergeDraft: load.setAutoMergeDraft, loading: load.loading, loadFailed: load.loadFailed, saveFailed, - isDirty, + isDirty: isMaxDirty || isMergeDirty || isAutoMergeDirty, isAdmin, isLocked, reload: load.reload, @@ -272,8 +329,48 @@ function MergeToggleFields({ enabled, onChange, disabled }: MergeToggleFieldsPro ); } -/** Settings → General → Message Queue page: the per-session queue limit and - * the queued-message merge toggle, sharing one save contributor. */ +/** Renders the automatic same-source admission merge setting. */ +function AutoMergeToggleFields({ enabled, onChange, disabled }: MergeToggleFieldsProps) { + const { t } = useTranslation(); + const label = t("system:messageQueueAutoMergeToggleLabel"); + return ( +
+
+
+ +

+ {t("system:messageQueueAutoMergeDescription")} +

+
+
+ +
+
+
+ +

{t("system:messageQueueAutoMergeNotice")}

+
+
+ ); +} + +/** Settings → Task Behavior → Message Queue controls, sharing one save contributor. */ export function MessageQueueSettings() { const { t } = useTranslation(); const state = useMessageQueueSettingsDraft(); @@ -344,6 +441,11 @@ export function MessageQueueSettings() { onChange={state.setMergeDraft} disabled={!state.isAdmin} /> + ); diff --git a/apps/web/e2e/helpers/message-queue-settings.ts b/apps/web/e2e/helpers/message-queue-settings.ts new file mode 100644 index 0000000000..3e8ebe754b --- /dev/null +++ b/apps/web/e2e/helpers/message-queue-settings.ts @@ -0,0 +1,59 @@ +import type { ApiClient } from "./api-client"; +import type { + MessageQueueSettingsPatch, + MessageQueueSettingsResponse, + MessageQueueSettingsValue, +} from "../../lib/types/system"; + +type KandevTest = typeof import("../fixtures/test-base").test; + +export const MESSAGE_QUEUE_SETTINGS_PATH = "/api/v1/system/message-queue/settings"; + +export async function requestMessageQueueSettings( + apiClient: ApiClient, + method: "GET" | "PATCH", + patch?: MessageQueueSettingsPatch, +): Promise { + const response = await apiClient.rawRequest(method, MESSAGE_QUEUE_SETTINGS_PATH, patch); + if (!response.ok) { + throw new Error( + `${method} ${MESSAGE_QUEUE_SETTINGS_PATH} failed (${response.status}): ${await response.text()}`, + ); + } + return response.json() as Promise; +} + +export async function restoreMessageQueueSettings( + apiClient: ApiClient, + baseline: MessageQueueSettingsValue, +): Promise { + const current = await requestMessageQueueSettings(apiClient, "GET"); + const patch: MessageQueueSettingsPatch = { + merge_enabled: baseline.merge_enabled, + auto_merge_enabled: baseline.auto_merge_enabled, + }; + if (!current.effective.locked) { + patch.max_per_session = baseline.max_per_session; + } + await requestMessageQueueSettings(apiClient, "PATCH", patch); +} + +/** + * Register per-test isolation for specs whose assertions require separate + * compatible queue rows. Captures and restores every install-wide queue field + * so the helper cannot leak a concurrent capacity or manual-merge baseline. + */ +export function registerSeparateQueueRows(test: KandevTest): void { + let baseline: MessageQueueSettingsValue | undefined; + + test.beforeEach(async ({ apiClient }) => { + baseline = (await requestMessageQueueSettings(apiClient, "GET")).settings; + await requestMessageQueueSettings(apiClient, "PATCH", { auto_merge_enabled: false }); + }); + + test.afterEach(async ({ apiClient }) => { + if (!baseline) return; + await restoreMessageQueueSettings(apiClient, baseline); + baseline = undefined; + }); +} diff --git a/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts b/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts index 25ba8ecf5d..217b952462 100644 --- a/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts +++ b/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts @@ -4,6 +4,9 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { waitForSessionState } from "../../helpers/session"; import { SessionPage } from "../../pages/session-page"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); /** * E2E coverage for the cross-task message attribution feature. diff --git a/apps/web/e2e/tests/chat/message-queue-reorder.spec.ts b/apps/web/e2e/tests/chat/message-queue-reorder.spec.ts index 3f4832fde0..527388a3ba 100644 --- a/apps/web/e2e/tests/chat/message-queue-reorder.spec.ts +++ b/apps/web/e2e/tests/chat/message-queue-reorder.spec.ts @@ -4,6 +4,9 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { typeWhileBusy } from "../../helpers/type-while-busy"; import { SessionPage } from "../../pages/session-page"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); /** Create a task whose agent finishes its intro turn and becomes idle. */ async function seedIdleTask( diff --git a/apps/web/e2e/tests/chat/message-queue.spec.ts b/apps/web/e2e/tests/chat/message-queue.spec.ts index e7fd14ce3a..dd66f39fab 100644 --- a/apps/web/e2e/tests/chat/message-queue.spec.ts +++ b/apps/web/e2e/tests/chat/message-queue.spec.ts @@ -5,6 +5,12 @@ import type { ApiClient } from "../../helpers/api-client"; import { typeWhileBusy } from "../../helpers/type-while-busy"; import { SessionPage } from "../../pages/session-page"; import { expectFullQueueScrolls, seedFullQueueTask } from "./message-queue-scroll-helpers"; +import { + registerSeparateQueueRows, + requestMessageQueueSettings, +} from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); // --------------------------------------------------------------------------- // Quick Chat queue tests @@ -229,6 +235,42 @@ test.describe("Task session queue", () => { await expectFullQueueScrolls(session); }); + test("automatic merge compacts compatible user rows and disabled mode keeps them separate", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTask(seedData.workspaceId, "Automatic queue merge", { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }); + const { session_id: sessionId } = await apiClient.seedTaskSession(task.id, { + state: "RUNNING", + agentProfileId: seedData.agentProfileId, + }); + await requestMessageQueueSettings(apiClient, "PATCH", { auto_merge_enabled: true }); + + await apiClient.queueMessage(task.id, sessionId, "automatic first"); + await apiClient.queueMessage(task.id, sessionId, "automatic second"); + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await openQueuePanel(session.activeChat()); + const panel = session.activeChat().getByTestId("queued-ghost-list"); + const entries = panel.getByTestId("queue-entry-text"); + await expect(entries).toHaveCount(1); + await expect(entries.first()).toContainText("automatic first"); + await expect(entries.first()).toContainText("automatic second"); + + await requestMessageQueueSettings(apiClient, "PATCH", { auto_merge_enabled: false }); + await apiClient.queueMessage(task.id, sessionId, "separate third"); + await apiClient.queueMessage(task.id, sessionId, "separate fourth"); + await expect(entries).toHaveCount(3, { timeout: 10_000 }); + await expect(entries.nth(1)).toHaveText("separate third"); + await expect(entries.nth(2)).toHaveText("separate fourth"); + }); + test("queue message via submit button on task session page", async ({ testPage, apiClient, @@ -287,10 +329,12 @@ test.describe("Task session queue", () => { ); await session.sendMessage("/slow 30s"); await expect(session.agentStatus()).toBeVisible({ timeout: 15_000 }); + await expect(session.chat.getByText("Running slow response (30s total)...")).toBeVisible({ + timeout: 15_000, + }); const taskID = new URL(testPage.url()).pathname.split("/").pop(); if (!taskID) throw new Error("task URL did not contain a task ID"); const workflowStepBefore = (await apiClient.getTask(taskID)).workflow_step_id; - await testPage.waitForTimeout(500); const editor = testPage.locator(".tiptap.ProseMirror").first(); await queueMessagesWhileBusy(testPage, editor, [ diff --git a/apps/web/e2e/tests/chat/mid-turn-steering.spec.ts b/apps/web/e2e/tests/chat/mid-turn-steering.spec.ts index 74b1f797e7..177e3a0231 100644 --- a/apps/web/e2e/tests/chat/mid-turn-steering.spec.ts +++ b/apps/web/e2e/tests/chat/mid-turn-steering.spec.ts @@ -5,6 +5,9 @@ import type { ApiClient } from "../../helpers/api-client"; import { waitForActiveSessionForegroundActivity } from "../../helpers/session-store"; import { typeWhileBusy } from "../../helpers/type-while-busy"; import { SessionPage } from "../../pages/session-page"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); // End-to-end coverage for the mid-turn steering composer contract // (docs/specs/platform/mid-turn-steering.md). CONTRIBUTING.md requires Playwright diff --git a/apps/web/e2e/tests/chat/mobile-message-queue-management.spec.ts b/apps/web/e2e/tests/chat/mobile-message-queue-management.spec.ts index 4918b0ce5e..dff89e5f74 100644 --- a/apps/web/e2e/tests/chat/mobile-message-queue-management.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-message-queue-management.spec.ts @@ -5,6 +5,9 @@ import type { ApiClient } from "../../helpers/api-client"; import { typeWhileBusy } from "../../helpers/type-while-busy"; import { SessionPage } from "../../pages/session-page"; import { expectFullQueueScrolls, seedFullQueueTask } from "./message-queue-scroll-helpers"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); async function expectTouchTarget(locator: Locator): Promise { await expect(locator).toBeVisible(); diff --git a/apps/web/e2e/tests/chat/mobile-message-queue-reorder.spec.ts b/apps/web/e2e/tests/chat/mobile-message-queue-reorder.spec.ts index 8ae65f9633..d8eb2b184a 100644 --- a/apps/web/e2e/tests/chat/mobile-message-queue-reorder.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-message-queue-reorder.spec.ts @@ -4,6 +4,9 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { typeWhileBusy } from "../../helpers/type-while-busy"; import { SessionPage } from "../../pages/session-page"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); async function expectTouchTarget(locator: Locator): Promise { await expect(locator).toBeVisible(); diff --git a/apps/web/e2e/tests/system/message-queue-settings.spec.ts b/apps/web/e2e/tests/system/message-queue-settings.spec.ts index d5416ebd72..4822aba4d3 100644 --- a/apps/web/e2e/tests/system/message-queue-settings.spec.ts +++ b/apps/web/e2e/tests/system/message-queue-settings.spec.ts @@ -1,50 +1,32 @@ import { test, expect } from "../../fixtures/test-base"; -import type { ApiClient } from "../../helpers/api-client"; import type { - MessageQueueSettingsPatch, MessageQueueSettingsResponse, + MessageQueueSettingsValue, } from "../../../lib/types/system"; +import { + MESSAGE_QUEUE_SETTINGS_PATH, + requestMessageQueueSettings, + restoreMessageQueueSettings, +} from "../../helpers/message-queue-settings"; -const SETTINGS_PATH = "/api/v1/system/message-queue/settings"; - -async function requestSettings( - apiClient: ApiClient, - method: "GET" | "PATCH", - patch?: MessageQueueSettingsPatch, -): Promise { - const response = await apiClient.rawRequest(method, SETTINGS_PATH, patch); - if (!response.ok) { - throw new Error( - `${method} ${SETTINGS_PATH} failed (${response.status}): ${await response.text()}`, - ); - } - return response.json() as Promise; -} - -test.describe.serial("Message Queue general settings", () => { - let baseline: number | undefined; - let mergeBaseline: boolean | undefined; +test.describe.serial("Message Queue task behavior settings", () => { + let baseline: MessageQueueSettingsValue | undefined; test.beforeEach(async ({ apiClient }) => { - const current = (await requestSettings(apiClient, "GET")).settings; - baseline = current.max_per_session; - mergeBaseline = current.merge_enabled; + baseline = (await requestMessageQueueSettings(apiClient, "GET")).settings; }); test.afterEach(async ({ apiClient }) => { - if (baseline === undefined || mergeBaseline === undefined) return; - await requestSettings(apiClient, "PATCH", { - max_per_session: baseline, - merge_enabled: mergeBaseline, - }); + if (!baseline) return; + await restoreMessageQueueSettings(apiClient, baseline); baseline = undefined; - mergeBaseline = undefined; }); test("admin navigates to Task behavior, saves, and reloads the live setting", async ({ testPage, }) => { - const updated = baseline === 17 ? 18 : 17; + if (!baseline) throw new Error("message queue settings baseline was not captured"); + const updated = baseline.max_per_session === 17 ? 18 : 17; await testPage.goto("/settings/preferences/appearance"); const settingsNav = testPage.getByTestId("app-sidebar-settings-mode"); await settingsNav.getByRole("link", { name: /^Task Behavior/ }).click(); @@ -68,14 +50,14 @@ test.describe.serial("Message Queue general settings", () => { ); const input = testPage.getByTestId("message-queue-max-per-session"); - await expect(input).toHaveValue(String(baseline)); + await expect(input).toHaveValue(String(baseline.max_per_session)); await input.fill(String(updated)); await expect(testPage.getByTestId("settings-floating-save")).toBeVisible(); const saveResponse = testPage.waitForResponse( (response) => response.request().method() === "PATCH" && - new URL(response.url()).pathname === SETTINGS_PATH, + new URL(response.url()).pathname === MESSAGE_QUEUE_SETTINGS_PATH, ); await testPage.getByRole("button", { name: "Save changes" }).click(); expect((await saveResponse).status()).toBe(200); @@ -90,10 +72,11 @@ test.describe.serial("Message Queue general settings", () => { test("admin toggles queued message merging without touching the session limit", async ({ testPage, }) => { + if (!baseline) throw new Error("message queue settings baseline was not captured"); await testPage.goto("/settings/general/message-queue"); const toggle = testPage.getByTestId("message-queue-merge-enabled"); await expect(toggle).toBeVisible(); - const wasEnabled = mergeBaseline === true; + const wasEnabled = baseline.merge_enabled; await expect(toggle).toHaveAttribute("aria-checked", String(wasEnabled)); await expect( testPage.getByText(/Only adjacent messages from the same sender can be merged/), @@ -105,7 +88,7 @@ test.describe.serial("Message Queue general settings", () => { const saveResponse = testPage.waitForResponse( (response) => response.request().method() === "PATCH" && - new URL(response.url()).pathname === SETTINGS_PATH, + new URL(response.url()).pathname === MESSAGE_QUEUE_SETTINGS_PATH, ); await toggle.click(); await expect(testPage.getByTestId("settings-floating-save")).toBeVisible(); @@ -122,18 +105,57 @@ test.describe.serial("Message Queue general settings", () => { ); }); + test("automatic merge defaults on and saves off then on without changing other fields", async ({ + testPage, + }) => { + if (!baseline) throw new Error("message queue settings baseline was not captured"); + expect(baseline.auto_merge_enabled).toBe(true); + await testPage.goto("/settings/preferences/task-behavior"); + const toggle = testPage.getByTestId("message-queue-auto-merge-enabled"); + await expect(toggle).toHaveAttribute("aria-checked", "true"); + + for (const expected of [false, true]) { + const saveResponse = testPage.waitForResponse( + (response) => + response.request().method() === "PATCH" && + new URL(response.url()).pathname === MESSAGE_QUEUE_SETTINGS_PATH, + ); + await toggle.click(); + await testPage.getByRole("button", { name: "Save changes" }).click(); + const response = await saveResponse; + expect(response.status()).toBe(200); + expect(response.request().postDataJSON()).toEqual({ auto_merge_enabled: expected }); + const saved = (await response.json()) as MessageQueueSettingsResponse; + expect(saved.settings).toMatchObject({ + max_per_session: baseline.max_per_session, + merge_enabled: baseline.merge_enabled, + auto_merge_enabled: expected, + }); + await testPage.reload(); + await expect(testPage.getByTestId("message-queue-auto-merge-enabled")).toHaveAttribute( + "aria-checked", + String(expected), + ); + } + }); + test("environment override is visible and read-only", async ({ testPage }) => { - await testPage.route(`**${SETTINGS_PATH}`, async (route) => { + await testPage.route(`**${MESSAGE_QUEUE_SETTINGS_PATH}`, async (route) => { await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify({ - settings: { max_per_session: baseline, merge_enabled: true }, + settings: { + max_per_session: baseline?.max_per_session ?? 10, + merge_enabled: true, + auto_merge_enabled: true, + }, effective: { max_per_session: 41, source: "environment", locked: true, merge_enabled: true, + auto_merge_enabled: true, }, } satisfies MessageQueueSettingsResponse), }); @@ -145,8 +167,9 @@ test.describe.serial("Message Queue general settings", () => { await expect(testPage.getByTestId("message-queue-source")).toHaveText("Environment"); await expect(testPage.getByText(/KANDEV_QUEUE_MAX_PER_SESSION/).first()).toBeVisible(); await expect(testPage.getByTestId("settings-floating-save")).toHaveCount(0); - // The environment lock only governs max_per_session; merging stays editable. + // The environment lock only governs max_per_session; both merge settings stay editable. await expect(testPage.getByTestId("message-queue-merge-enabled")).toBeEnabled(); + await expect(testPage.getByTestId("message-queue-auto-merge-enabled")).toBeEnabled(); }); test("member can navigate to the setting but cannot edit it", async ({ testPage }) => { @@ -173,6 +196,7 @@ test.describe.serial("Message Queue general settings", () => { await expect(testPage.getByTestId("message-queue-max-per-session")).toBeDisabled(); await expect(testPage.getByTestId("message-queue-merge-enabled")).toBeDisabled(); + await expect(testPage.getByTestId("message-queue-auto-merge-enabled")).toBeDisabled(); await expect(testPage.getByText("Only administrators can change this setting.")).toBeVisible(); await expect(testPage.getByTestId("settings-floating-save")).toHaveCount(0); }); diff --git a/apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts b/apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts index f43bb9440e..f1adecfc7b 100644 --- a/apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts +++ b/apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts @@ -1,6 +1,24 @@ import { test, expect } from "../../fixtures/test-base"; import { expectElementsNotToIntersect } from "../../helpers/layout-assertions"; import { MobileKanbanPage } from "../../pages/mobile-kanban-page"; +import type { MessageQueueSettingsValue } from "../../../lib/types/system"; +import { + MESSAGE_QUEUE_SETTINGS_PATH, + requestMessageQueueSettings, + restoreMessageQueueSettings, +} from "../../helpers/message-queue-settings"; + +let baseline: MessageQueueSettingsValue | undefined; + +test.beforeEach(async ({ apiClient }) => { + baseline = (await requestMessageQueueSettings(apiClient, "GET")).settings; +}); + +test.afterEach(async ({ apiClient }) => { + if (!baseline) return; + await restoreMessageQueueSettings(apiClient, baseline); + baseline = undefined; +}); test("mobile navigation reaches the Message Queue section with touch-safe shared settings layout", async ({ testPage, @@ -38,9 +56,35 @@ test("mobile navigation reaches the Message Queue section with touch-safe shared ); expect(nestedScrollOwners).toBe(0); - const current = await input.inputValue(); - await input.fill(current === "23" ? "24" : "23"); + if (!baseline) throw new Error("message queue settings baseline was not captured"); + const toggle = testPage.getByTestId("message-queue-auto-merge-enabled"); + const touchTarget = testPage.getByTestId("message-queue-auto-merge-touch-target"); + await expect(toggle).toHaveAttribute("aria-checked", String(baseline.auto_merge_enabled)); + const touchBox = await touchTarget.boundingBox(); + expect(touchBox).not.toBeNull(); + expect(touchBox!.width).toBeGreaterThanOrEqual(44); + expect(touchBox!.height).toBeGreaterThanOrEqual(44); + + await toggle.tap(); const saveBar = testPage.getByTestId("settings-floating-save"); await expect(saveBar).toBeVisible(); - await expectElementsNotToIntersect(input, saveBar); + await expectElementsNotToIntersect(touchTarget, saveBar); + await saveBar.getByRole("button", { name: "Reset" }).tap(); + await expect(toggle).toHaveAttribute("aria-checked", String(baseline.auto_merge_enabled)); + await expect(saveBar).not.toBeVisible(); + + const expected = !baseline.auto_merge_enabled; + const saveResponse = testPage.waitForResponse( + (response) => + response.request().method() === "PATCH" && + new URL(response.url()).pathname === MESSAGE_QUEUE_SETTINGS_PATH, + ); + await toggle.tap(); + await saveBar.getByRole("button", { name: "Save changes" }).tap(); + expect((await saveResponse).request().postDataJSON()).toEqual({ auto_merge_enabled: expected }); + await testPage.reload(); + await expect(testPage.getByTestId("message-queue-auto-merge-enabled")).toHaveAttribute( + "aria-checked", + String(expected), + ); }); diff --git a/apps/web/e2e/tests/task/sidebar-queued-count.spec.ts b/apps/web/e2e/tests/task/sidebar-queued-count.spec.ts index 637387532e..ebe39b9e47 100644 --- a/apps/web/e2e/tests/task/sidebar-queued-count.spec.ts +++ b/apps/web/e2e/tests/task/sidebar-queued-count.spec.ts @@ -10,6 +10,9 @@ */ import { test, expect } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; +import { registerSeparateQueueRows } from "../../helpers/message-queue-settings"; + +registerSeparateQueueRows(test); const stepOpts = (seedData: { workflowId: string; startStepId: string }) => ({ workflow_id: seedData.workflowId, diff --git a/apps/web/lib/api/domains/settings-api.test.ts b/apps/web/lib/api/domains/settings-api.test.ts index 643a704b53..4d00a1bf6e 100644 --- a/apps/web/lib/api/domains/settings-api.test.ts +++ b/apps/web/lib/api/domains/settings-api.test.ts @@ -42,8 +42,14 @@ describe("message queue settings api", () => { it("fetches the current setting without cache", async () => { fetchSpy.mockResolvedValueOnce( jsonResponse({ - settings: { max_per_session: 10 }, - effective: { max_per_session: 10, source: "default", locked: false }, + settings: { max_per_session: 10, merge_enabled: true, auto_merge_enabled: true }, + effective: { + max_per_session: 10, + source: "default", + locked: false, + merge_enabled: true, + auto_merge_enabled: true, + }, }), ); @@ -58,8 +64,14 @@ describe("message queue settings api", () => { it("PATCHes the requested per-session maximum", async () => { fetchSpy.mockResolvedValueOnce( jsonResponse({ - settings: { max_per_session: 25 }, - effective: { max_per_session: 25, source: "setting", locked: false }, + settings: { max_per_session: 25, merge_enabled: true, auto_merge_enabled: true }, + effective: { + max_per_session: 25, + source: "setting", + locked: false, + merge_enabled: true, + auto_merge_enabled: true, + }, }), ); @@ -72,6 +84,25 @@ describe("message queue settings api", () => { expect(lastCall().init?.method).toBe("PATCH"); expect(lastCall().init?.body).toBe(JSON.stringify({ max_per_session: 25 })); }); + + it("PATCHes automatic merge without changing other queue settings", async () => { + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + settings: { max_per_session: 10, merge_enabled: true, auto_merge_enabled: false }, + effective: { + max_per_session: 10, + source: "default", + locked: false, + merge_enabled: true, + auto_merge_enabled: false, + }, + }), + ); + + await updateMessageQueueSettings({ auto_merge_enabled: false }); + + expect(lastCall().init?.body).toBe(JSON.stringify({ auto_merge_enabled: false })); + }); }); describe("sleep inhibition settings api", () => { diff --git a/apps/web/lib/types/system.ts b/apps/web/lib/types/system.ts index f554e0f134..3a96cb82b1 100644 --- a/apps/web/lib/types/system.ts +++ b/apps/web/lib/types/system.ts @@ -189,6 +189,7 @@ export interface SystemMetricsSettingsResponse { export interface MessageQueueSettingsValue { max_per_session: number; merge_enabled: boolean; + auto_merge_enabled: boolean; } /** Partial PATCH payload: omitted fields are left unchanged server-side. */ diff --git a/apps/web/src/locales/en/system.json b/apps/web/src/locales/en/system.json index d5621c8007..562021ba5d 100644 --- a/apps/web/src/locales/en/system.json +++ b/apps/web/src/locales/en/system.json @@ -164,6 +164,9 @@ "licensesTitle": "Third-party licenses", "licensesToggleLabel": "Show license text for {{name}}", "messageQueueAdminOnly": "Only administrators can change this setting.", + "messageQueueAutoMergeDescription": "Combines consecutive compatible queued messages from the same source into one turn before delivery.", + "messageQueueAutoMergeNotice": "If a message has a different source or incompatible task, model, mode, metadata, or shared limits, it stays as a separate queued message. This applies only to new admissions and does not change the manual merge control.", + "messageQueueAutoMergeToggleLabel": "Automatically merge consecutive messages", "messageQueueConfigured": "Configured:", "messageQueueDescription": "Control how many pending messages each session can hold before new work is rejected.", "messageQueueEffective": "Effective:", diff --git a/apps/web/src/locales/pseudo/system.json b/apps/web/src/locales/pseudo/system.json index 2b99ff36b0..a37d4659bd 100644 --- a/apps/web/src/locales/pseudo/system.json +++ b/apps/web/src/locales/pseudo/system.json @@ -164,6 +164,9 @@ "licensesTitle": "Ţĥĩŕď-ƥàŕţŷ ĺĩćēńśēś", "licensesToggleLabel": "Śĥōŵ ĺĩćēńśē ţēxţ ƒōŕ {{name}}", "messageQueueAdminOnly": "Ōńĺŷ àďḿĩńĩśţŕàţōŕś ćàń ćĥàńĝē ţĥĩś śēţţĩńĝ.", + "messageQueueAutoMergeDescription": "Ćōḿƀĩńēś ćōńśēćũţĩvē ćōḿƥàţĩƀĺē qũēũēď ḿēśśàĝēś ƒŕōḿ ţĥē śàḿē śōũŕćē ĩńţō ōńē ţũŕń ƀēƒōŕē ďēĺĩvēŕŷ.", + "messageQueueAutoMergeNotice": "Ĩƒ à ḿēśśàĝē ĥàś à ďĩƒƒēŕēńţ śōũŕćē ōŕ ĩńćōḿƥàţĩƀĺē ţàśķ, ḿōďēĺ, ḿōďē, ḿēţàďàţà, ōŕ śĥàŕēď ĺĩḿĩţś, ĩţ śţàŷś àś à śēƥàŕàţē qũēũēď ḿēśśàĝē. Ţĥĩś àƥƥĺĩēś ōńĺŷ ţō ńēŵ àďḿĩśśĩōńś àńď ďōēś ńōţ ćĥàńĝē ţĥē ḿàńũàĺ ḿēŕĝē ćōńţŕōĺ.", + "messageQueueAutoMergeToggleLabel": "Àũţōḿàţĩćàĺĺŷ ḿēŕĝē ćōńśēćũţĩvē ḿēśśàĝēś", "messageQueueConfigured": "Ćōńƒĩĝũŕēď:", "messageQueueDescription": "Ćōńţŕōĺ ĥōŵ ḿàńŷ ƥēńďĩńĝ ḿēśśàĝēś ēàćĥ śēśśĩōń ćàń ĥōĺď ƀēƒōŕē ńēŵ ŵōŕķ ĩś ŕēĵēćţēď.", "messageQueueEffective": "Ēƒƒēćţĩvē:", diff --git a/docs/plans/message-queue-auto-merge/plan.md b/docs/plans/message-queue-auto-merge/plan.md new file mode 100644 index 0000000000..3f4edf3992 --- /dev/null +++ b/docs/plans/message-queue-auto-merge/plan.md @@ -0,0 +1,264 @@ +--- +spec: docs/specs/ui/message-queue-auto-merge.md +created: 2026-08-12 +status: completed +--- + +# Implementation Plan: Automatically Merge Consecutive Queued Messages + +## Overview + +Add a default-on, install-wide automatic-merge setting without changing the +manual merge switch. Build the strict tail-compatibility mutation first, route +ordinary admission and staged attachment admission through it safely, then add +settings persistence and UI. Finish with desktop/mobile E2E coverage and public +documentation. + +--- + +## Backend + +### Automatic tail-merge contract + +- Add a repository operation in + `apps/backend/internal/orchestrator/messagequeue/repository.go` that targets + one newly inserted source ID and its immediate predecessor. It returns the + surviving target plus whether a fold occurred; incompatibility is a normal + `false` result, not `ErrNoMergeTarget`. +- Implement it under the existing per-session lock in + `repository_memory.go` and as one transaction in `repository_sqlite.go` + (the database-backed implementation also serves the env-gated Postgres + suite). Select by source ID and session, select only the greatest lower + position, compute the complete candidate, update the target, and delete the + exact source. A missing/drained source never causes a search for another + source. +- Keep manual `MergeIntoAbove` and `mergeAllowed` unchanged. Add automatic-only + helpers in `merge.go`: + - source compatibility: same task/model/plan mode; user identities equal and + non-reserved, or agent identities equal with one identical non-empty + `sender_task_id`; lifecycle/workflow/server/clarification rows excluded; + - non-additive metadata comparison after normalizing JSON-round-tripped map + values; + - stable entity-reference and context-file unions; + - combined attachment count/byte validation using the same limits and byte + accounting as send-now; + - content joining that adds `\n\n` only when both bodies are non-empty. +- Preserve the target's immutable fields and target-then-source order. Any + mismatch or limit overflow leaves both rows unchanged and returns a skipped + result. + +### Admission integration + +- Extend `messagequeue.Service` in `service.go` with a separate atomic + `autoMergeEnabled` value, defaulting to true, plus getters/setters. Do not + reuse `mergeEnabled`; test all four switch combinations. +- Within `QueueMessageWithMetadata`, snapshot capacity and automatic-merge + state under `WithSessionAdmission`, perform the existing capped insert, then + attempt the repository fold. Capacity remains an admission prerequisite. + Return the surviving target on success and the source on skip. +- Treat a post-insert automatic-fold storage error as degraded success: the + transaction leaves the accepted source separate, the service logs the + failure, and callers do not receive an error that encourages a duplicate + retry. +- Add a narrow deferred-finalization path for user WebSocket admissions that + contain staged file-backed attachments. Hold `WithSessionAdmission` across + insertion, claim, fold, and rollback so no later admission can merge into the + provisional source. `queue_handlers.go` must insert a distinct source first, + claim its attachments, and only then finalize the snapshotted + automatic-merge decision. On claim failure, the existing rollback removes + only that source ID. Inline attachments can use ordinary admission. +- Preserve exact-ID consumers. `QueueAndInterruptForPeerMessage` and + `message_task_kandev` use the returned surviving ID, so an immediate parent + interrupt takes the complete merged entry. Workflow/coalesce, restoration, + explicit append, clarification, and reserved lifecycle paths remain excluded + by their APIs or compatibility checks. +- Publish `message.queue.status_changed` only after the final merge-or-separate + state is known. Do not add `auto_merge_enabled` to `QueueStatus`; the queue + panel has no setting control. + +### Persistent settings and startup wiring + +- Extend `apps/backend/internal/system/queuesettings/types.go` with + `AutoMergeEnabled` on `Settings`, `SettingsPatch`, and `Effective`. Apply and + validate it like the independent manual merge boolean. +- Extend `store.go`'s pointer-backed legacy decoder so an omitted + `auto_merge_enabled` becomes true while an explicit false survives. Saving + writes the normalized three-field object. +- Extend `resolver.go` and `service.go` so GET/PATCH round-trip the field, + partial updates preserve both other values, max-capacity environment locks do + not block an auto-only patch, persistence precedes live apply, and the queue + target receives all three effective values. In particular, an auto-only or + manual-only PATCH under an environment capacity override must reapply the + resolved environment capacity, not the lower persisted capacity. +- In `apps/backend/internal/backendapp/orchestrator.go`, resolve and apply the + saved automatic setting before the orchestrator accepts work. Update + `message_queue_settings_test.go`, `system_routes_test.go`, and + `internal/system/queuesettings/queuesettings_test.go` for default, upgrade, + explicit false, partial PATCH, environment lock, restart, and live-apply + cases. + +--- + +## Frontend + +### Settings API and types + +- Add `auto_merge_enabled` to `MessageQueueSettingsValue` in + `apps/web/lib/types/system.ts`. The existing partial patch type then accepts + an auto-only update without changing the HTTP client contract. +- Update `apps/web/lib/api/domains/settings-api.test.ts` fixtures and assertions + to prove the field is decoded and sent alone. + +### Message Queue settings card + +- Extend `useMessageQueueSettingsLoad` and + `useMessageQueueSettingsDraft` in + `apps/web/components/settings/system/message-queue-settings.tsx` with a third + draft/baseline. Include it in contributor revision, dirty/valid/save/discard + logic, and concurrency-safe response reconciliation. +- Add a separate `AutoMergeToggleFields` presentation block below the existing + manual merge block. Use the label **Automatically merge consecutive + messages**, explanatory same-source/fallback copy, a stable test ID, and the + existing admin/member and shared floating-save behavior. +- Keep the current Task Behavior page, card width, single settings scroll owner, + and one-column responsive layout. The switch and surrounding row must remain + touch-safe and clear the mobile save bar. No queue-panel component changes. +- Add all user-facing copy to `apps/web/src/locales/en/system.json`, regenerate + the pseudo catalog, and run i18n checks and the changed-line ratchet. + +--- + +## Tests + +- **What:** strict same-source compatibility, identity preservation, ordered + content/context/attachment merge, metadata mismatch fallback, combined-limit + fallback, and no half-write. + **File:** new + `apps/backend/internal/orchestrator/messagequeue/repository_auto_merge_test.go` + plus env-gated Postgres cases. + **How:** table-drive the same contract over memory, SQLite, and Postgres; + compare complete pre/post snapshots on skipped and failing paths. +- **What:** default-on service admission, disabled behavior, three-message + chaining, cap-before-merge, concurrent admissions/drain, degraded repository + error, and independence from manual merge. + **File:** new `service_auto_merge_test.go` and focused updates to existing + queue tests that intentionally need multiple compatible rows. + **How:** RED service tests first, then race tests using the existing admission + lock and repository hooks. +- **What:** file-backed attachment claim succeeds before fold and claim failure + removes only the new source. + **File:** `queue_handlers_test.go` or a new + `queue_handlers_auto_merge_test.go`. + **How:** handler-to-real-memory-service tests with blocking/failing attachment + claim fakes and byte-for-byte target assertions. +- **What:** returned surviving IDs remain valid for normal peer queueing and + parent interrupt dispatch, while different senders, clarification, workflow, + lifecycle, plugin-source mismatch, append, coalesce, and restore do not + conflate. + **File:** focused cases in `internal/orchestrator`, `internal/mcp/handlers`, + and `internal/backendapp` tests. + **How:** integration tests through existing real service fixtures. +- **What:** settings default/upgrade/persistence/partial PATCH/live apply and + environment-lock independence, including retention of the environment's live + capacity after an auto-only PATCH. + **File:** `internal/system/queuesettings/queuesettings_test.go`, + `internal/backendapp/message_queue_settings_test.go`, and + `internal/system/system_routes_test.go`. + **How:** store, service, HTTP, and startup wiring tests. +- **What:** third draft loads, saves alone, discards, survives a concurrent + response, remains editable under only a max environment lock, and is disabled + for members. + **File:** `message-queue-settings.test.tsx` and `settings-api.test.ts`. + **How:** component tests with the existing save-provider harness and API + mocks. + +--- + +## E2E Tests + +- **Scenario:** default-on setting persists and can be switched off/on without + changing capacity or manual merge. + **File:** `apps/web/e2e/tests/system/message-queue-settings.spec.ts`. + **What to verify:** GET default, auto-only PATCH body, shared save bar, + reload, environment-cap lock independence, member read-only state, and full + baseline restoration in `afterEach`. +- **Scenario:** two compatible user messages become one when enabled and remain + two when disabled. + **File:** `apps/web/e2e/tests/chat/message-queue.spec.ts`. + **What to verify:** busy agent, final queue count/content/order, no transient + second row after the response, and setting restoration even on failure. +- **Scenario:** mobile exposes the same touch-safe switch without overflow, + nested scroll ownership, or save-bar overlap. + **File:** + `apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts`. + **What to verify:** visible switch, minimum touch size, save/discard, and + persisted value. +- Existing management, manual merge, send-now, and reorder E2E scenarios that + require multiple compatible rows explicitly disable auto merge for their + setup and restore the prior install value during teardown. Update desktop and + `mobile-*` specs together. + +--- + +## Verification Results + +- Backend repository, admission, handler, MCP, settings, startup, and race suites + passed, including memory, SQLite, and environment-gated Postgres contracts. +- Frontend passed 34 focused component/API tests, typecheck, focused ESLint, + pseudo-locale generation, `i18n:check`, and `i18n:ratchet`. +- Desktop Chromium E2E passed 20 tests; mobile Chromium passed 4 tests; the + focused attribution, steering, and sidebar-count audit passed 3 tests. +- Public documentation tests passed 60 tests, the validator accepted 41 + published pages, and `git diff --check` passed. +- No blockers remain. + +Review remediation verification also passed the focused automatic-merge +repository/service tests and the message-queue race suite after adding the +context-file cap and shared admission serialization for queue mutations. + +--- + +## Implementation Waves And Parallel Candidates + +```text +Wave 1: +- [x] [task-01-auto-merge-repository](task-01-auto-merge-repository.md) + +Wave 2: +- [x] [task-02-auto-merge-admission](task-02-auto-merge-admission.md) + +Wave 3: +- [x] [task-03-auto-merge-settings](task-03-auto-merge-settings.md) + +Wave 4: +- [x] [task-04-auto-merge-settings-ui](task-04-auto-merge-settings-ui.md) + +Wave 5: +- [x] [task-05-auto-merge-e2e](task-05-auto-merge-e2e.md) + +Wave 6: +- [x] [task-06-auto-merge-docs](task-06-auto-merge-docs.md) +``` + +The dependency chain is sequential in the primary conversation. No wave +authorizes subagents; only a later explicit user request can do that. + +## Environment prerequisite + +If `apps/node_modules` is absent, run `pnpm install --frozen-lockfile` from +`apps/` before frontend or E2E checks. Do not change the lockfile. + +## Risks and boundaries + +- Automatic admission must not call the manual setting gate or loosen manual + merge provenance rules. +- File attachment claim rollback is destructive to the just-inserted source ID; + automatic fold must never run before that claim succeeds, and a later + admission must never use the provisional source as its target. +- A post-insert fold error cannot be returned as an admission failure because a + caller retry would duplicate accepted content. +- Existing tests frequently queue multiple compatible user rows. They must opt + out explicitly when row count is part of the scenario, while product defaults + stay on. +- No schema migration, feature flag, environment variable, retroactive sweep, + queue-panel toggle, or capacity bypass is planned. diff --git a/docs/plans/message-queue-auto-merge/task-01-auto-merge-repository.md b/docs/plans/message-queue-auto-merge/task-01-auto-merge-repository.md new file mode 100644 index 0000000000..91b64bc251 --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-01-auto-merge-repository.md @@ -0,0 +1,88 @@ +--- +id: "01-auto-merge-repository" +title: "Build automatic tail merge" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 01: Build automatic tail merge + +## Acceptance + +1. The repository contract and memory/database implementations atomically fold + an exact source ID into only its immediate predecessor, return the surviving + target and a merged/skipped result, and never mutate either row on skip or + error. +2. Automatic compatibility enforces source, task, model, plan mode, metadata, + lifecycle, attachment, reference, and context rules from the spec while + preserving target identity and target-then-source order. Manual merge + behavior remains unchanged. +3. Table-driven memory, SQLite, and env-gated Postgres tests cover happy paths, + every mismatch/limit fallback, missing/drained candidates, and atomic + rollback. + +## Verification + +```bash +cd apps/backend && go test -count=1 ./internal/orchestrator/messagequeue +cd apps/backend && go test -race -count=1 -run 'Test.*AutoMerge' ./internal/orchestrator/messagequeue +``` + +## Files likely touched + +- `apps/backend/internal/orchestrator/messagequeue/repository.go` +- `apps/backend/internal/orchestrator/messagequeue/merge.go` +- `apps/backend/internal/orchestrator/messagequeue/repository_memory.go` +- `apps/backend/internal/orchestrator/messagequeue/repository_sqlite.go` +- `apps/backend/internal/orchestrator/messagequeue/send_now.go` +- `apps/backend/internal/orchestrator/messagequeue/repository_auto_merge_test.go` +- `apps/backend/internal/orchestrator/messagequeue/repository_postgres_auto_merge_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `What`, `Data model`, `Admission lifecycle and concurrency`, `Failure + modes`, and compatibility/limit scenarios. +- Plan: `Backend > Automatic tail-merge contract`. +- Existing patterns: `MergeIntoAbove` transaction and memory lock, + `buildSendNowEnvelope` attachment/reference validation, persisted metadata + normalization, and env-gated Postgres merge tests. + +## Risks + +- JSON-round-tripped metadata types must compare semantically, not by Go slice + representation. +- Incompatibility is a successful skip. Do not reuse manual merge errors for it. +- Keep new Go functions within repository lint limits by extracting pure + compatibility and union helpers. + +## Output contract + +Report summary, files changed, exact commands/outcomes, blockers, risks, and +update this task plus `plan.md` status in the same conversation. + +## Results + +- Added `AutoMergeIntoAbove` to the repository contract and implemented it for + memory plus database-backed repositories without changing manual merge. +- Added automatic-only compatibility, semantic metadata comparison, stable + entity/context unions, attachment limit enforcement, and empty-body joining. +- Added shared memory/SQLite/env-gated Postgres contract coverage, including + mismatch and limit skips, immediate-predecessor selection, missing/drained + sources, identity preservation, and transaction rollback. +- Review remediation caps merged context-file metadata at 200 descriptors and + makes the identity assertion tolerant of Postgres timestamp precision. +- Verification passed: + - `cd apps/backend && go test -count=1 ./internal/orchestrator/messagequeue` + - `cd apps/backend && go test -race -count=1 -run 'Test.*AutoMerge' ./internal/orchestrator/messagequeue` +- Blockers: none. diff --git a/docs/plans/message-queue-auto-merge/task-02-auto-merge-admission.md b/docs/plans/message-queue-auto-merge/task-02-auto-merge-admission.md new file mode 100644 index 0000000000..8effdca7d4 --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-02-auto-merge-admission.md @@ -0,0 +1,99 @@ +--- +id: "02-auto-merge-admission" +title: "Integrate automatic merge admission" +status: completed +wave: 2 +depends_on: ["01-auto-merge-repository"] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 02: Integrate automatic merge admission + +## Acceptance + +1. `messagequeue.Service` has a default-on automatic setting independent from + manual merge; normal capped admissions return the surviving target when + folded, preserve a separate source on skip/error, and do not auto-process + append, coalesce, restore, retry, or reserved lifecycle APIs. +2. File-backed WebSocket admissions hold per-session admission serialization, + insert a distinct rollback-safe source, claim attachments, then finalize the + snapshotted fold. No later admission can target the provisional source; + claim failure leaves the prior target byte-for-byte unchanged, and status + publishes only the final state. +3. Integration tests prove concurrent ordering/drain behavior, cap-before-fold, + all four manual/automatic switch combinations, and exact-ID peer/parent + interrupt delivery with excluded workflow, clarification, plugin-source, + append, coalesce, and restore paths. + +## Verification + +```bash +cd apps/backend && go test -count=1 ./internal/orchestrator/messagequeue ./internal/orchestrator/handlers ./internal/orchestrator ./internal/mcp/handlers ./internal/backendapp +cd apps/backend && go test -race -count=1 ./internal/orchestrator/messagequeue ./internal/orchestrator/handlers +``` + +## Files likely touched + +- `apps/backend/internal/orchestrator/messagequeue/service.go` +- `apps/backend/internal/orchestrator/messagequeue/types.go` +- `apps/backend/internal/orchestrator/messagequeue/service_auto_merge_test.go` +- `apps/backend/internal/orchestrator/handlers/queue_handlers.go` +- `apps/backend/internal/orchestrator/handlers/queue_handlers_auto_merge_test.go` +- `apps/backend/internal/orchestrator/task_operations.go` +- `apps/backend/internal/orchestrator/task_operations_test.go` +- `apps/backend/internal/orchestrator/event_handlers_clarification_test.go` +- `apps/backend/internal/mcp/handlers/message_task_test.go` +- `apps/backend/internal/backendapp/adapters_plugin_messenger_test.go` +- Existing queue/service/handler tests whose setup intentionally needs separate + compatible rows. + +## Dependencies + +Task 01. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `What`, `API surface`, `Admission lifecycle and concurrency`, `Failure + modes`, and all admission scenarios. +- Plan: `Backend > Admission integration` and corresponding test rows. +- Existing patterns: `WithSessionAdmission`, queue attachment claim rollback, + `QueueAndInterruptForPeerMessage`, `QueueMessageWithCoalesceKey`, + `RestoreMessage`, and status publishers. + +## Risks + +- Never report post-insert fold failure as failed admission. +- The deferred attachment API must carry the admission-time setting snapshot + and retain the admission lock through claim/finalize; a settings change or + later admission cannot retroactively alter the provisional source. +- A returned merged ID may identify an older target. Every exact-ID caller must + use that returned value intentionally. + +## Output contract + +Report summary, files changed, exact commands/outcomes, blockers, risks, and +update this task plus `plan.md` status in the same conversation. + +## Results + +- Added an independent default-on live automatic-merge setting and admission + folding that snapshots capacity/policy under the per-session lock. +- Added deferred after-insert finalization so staged attachments are claimed + before folding; rollback and later admissions remain serialized by source ID. +- Kept capacity as a precondition, degraded post-insert fold errors to separate + success, and preserved explicit append/coalesce/restore/retry contracts. +- Added service, handler, concurrency, storage-error, four-switch-combination, + and exact surviving-ID parent-interrupt coverage. Legacy tests that require + multiple compatible rows now explicitly disable automatic merging. +- Review remediation serializes queue drains, targeted takes, removals, edits, + reorders, manual merges, cancel-all, and send-now claims with admission + finalization; drain/removal race coverage now guards the provisional source. +- Verification passed: + - `cd apps/backend && go test -count=1 ./internal/orchestrator/messagequeue ./internal/orchestrator/handlers ./internal/orchestrator ./internal/mcp/handlers ./internal/backendapp` + - `cd apps/backend && go test -race -count=1 ./internal/orchestrator/messagequeue ./internal/orchestrator/handlers` +- Blockers: none. diff --git a/docs/plans/message-queue-auto-merge/task-03-auto-merge-settings.md b/docs/plans/message-queue-auto-merge/task-03-auto-merge-settings.md new file mode 100644 index 0000000000..7edce99d54 --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-03-auto-merge-settings.md @@ -0,0 +1,88 @@ +--- +id: "03-auto-merge-settings" +title: "Persist automatic merge setting" +status: completed +wave: 3 +depends_on: ["02-auto-merge-admission"] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 03: Persist automatic merge setting + +## Acceptance + +1. Message Queue settings GET/PATCH include `auto_merge_enabled` in configured + and effective values; default and legacy-missing values are true, explicit + false persists, and partial patches preserve capacity and manual merge. +2. Persistence completes before live apply, an environment capacity lock does + not block an auto-only update or lose its effective live capacity, and + backend startup applies the resolved values before new admissions. +3. Store, resolver, service, HTTP route, and backend wiring tests cover fresh, + legacy, restart, invalid-record fallback, partial update, explicit false, + live apply, and environment-lock cases. + +## Verification + +```bash +cd apps/backend && go test -count=1 ./internal/system/queuesettings ./internal/system ./internal/backendapp +cd apps/backend && go test -race -count=1 ./internal/system/queuesettings ./internal/orchestrator/messagequeue +``` + +## Files likely touched + +- `apps/backend/internal/system/queuesettings/types.go` +- `apps/backend/internal/system/queuesettings/store.go` +- `apps/backend/internal/system/queuesettings/resolver.go` +- `apps/backend/internal/system/queuesettings/service.go` +- `apps/backend/internal/system/queuesettings/queuesettings_test.go` +- `apps/backend/internal/backendapp/orchestrator.go` +- `apps/backend/internal/backendapp/message_queue_settings_test.go` +- `apps/backend/internal/system/system_routes_test.go` + +## Dependencies + +Task 02. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `Data model`, `API surface`, `Permissions`, `Persistence guarantees`, + and settings scenarios. +- Plan: `Backend > Persistent settings and startup wiring`. +- Existing patterns: pointer-backed `merge_enabled` legacy decoding, + `SettingsPatch.Apply`, persisted-before-live-apply service flow, and + `resolveQueueMergeEnabled` startup wiring. + +## Risks + +- Plain bool decoding would silently turn legacy installs off. The stored shape + needs a pointer for the new field. +- Keep `merge_enabled` and `auto_merge_enabled` names and live setters distinct. +- The capacity environment source/lock metadata remains about capacity only. +- Live apply must use resolved effective capacity after any merge-only patch; + copying the persisted capacity would override the active environment value. + +## Output contract + +Report summary, files changed, exact commands/outcomes, blockers, risks, and +update this task plus `plan.md` status in the same conversation. + +## Results + +- Added `auto_merge_enabled` to configured/effective responses and partial + patches, with pointer-backed legacy decoding that defaults omitted values on. +- Persisted normalized three-field JSON and applied all live values only after + a successful save. +- Fixed merge-only live apply under a capacity environment override to reapply + the resolved environment capacity instead of the persisted value. +- Wired startup to resolve once and apply capacity, manual merge, and automatic + merge before queue admissions; added fresh/legacy/false/restart resolver, + HTTP, persistence, and live-target tests. +- Verification passed: + - `cd apps/backend && go test -count=1 ./internal/system/queuesettings ./internal/system ./internal/backendapp` + - `cd apps/backend && go test -race -count=1 ./internal/system/queuesettings ./internal/orchestrator/messagequeue` +- Blockers: none. diff --git a/docs/plans/message-queue-auto-merge/task-04-auto-merge-settings-ui.md b/docs/plans/message-queue-auto-merge/task-04-auto-merge-settings-ui.md new file mode 100644 index 0000000000..fa35b7cb8f --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-04-auto-merge-settings-ui.md @@ -0,0 +1,82 @@ +--- +id: "04-auto-merge-settings-ui" +title: "Add automatic merge switch" +status: completed +wave: 4 +depends_on: ["03-auto-merge-settings"] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 04: Add automatic merge switch + +## Acceptance + +1. Frontend settings types and the Message Queue card load a default-on + `auto_merge_enabled` draft and save/discard it independently through the + shared settings contributor without resetting capacity or manual merge. +2. A localized **Automatically merge consecutive messages** switch explains + same-source fallback behavior, is admin-editable even under only a capacity + environment lock, and is read-only for members. No queue-panel control is + added. +3. Component/API tests cover load, auto-only PATCH, dirty/discard/save, + concurrent response reconciliation, failure, environment lock, member + permission, and mobile-safe rendering; typecheck, focused lint, pseudo + generation, and i18n gates pass. + +## Verification + +```bash +cd apps && pnpm install --frozen-lockfile && pnpm --filter @kandev/web test -- components/settings/system/message-queue-settings.test.tsx lib/api/domains/settings-api.test.ts && cd web && pnpm run typecheck && pnpm exec eslint --max-warnings 0 components/settings/system/message-queue-settings.tsx components/settings/system/message-queue-settings.test.tsx lib/api/domains/settings-api.ts lib/api/domains/settings-api.test.ts lib/types/system.ts && pnpm run i18n:pseudo && pnpm run i18n:check && pnpm run i18n:ratchet +``` + +## Files likely touched + +- `apps/web/lib/types/system.ts` +- `apps/web/lib/api/domains/settings-api.test.ts` +- `apps/web/components/settings/system/message-queue-settings.tsx` +- `apps/web/components/settings/system/message-queue-settings.test.tsx` +- `apps/web/src/locales/en/system.json` +- `apps/web/src/locales/pseudo/system.json` + +## Dependencies + +Task 03. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `What`, `API surface`, `Permissions`, `Responsive and mobile behavior`, + and settings/mobile scenarios. +- Plan: `Frontend` and frontend test rows. +- Mobile contract: existing Task Behavior entry point; same Message Queue card; + one-column presentation; `settings-scroll-container` is the only scroll + owner; touch-safe switch; shared floating save bar; desktop/mobile capability + parity. +- Existing pattern: `MergeToggleFields`, the two-draft save coordinator, and + `message-queue-settings.test.tsx` save-provider harness. + +## Risks + +- The current component is near file/function lint limits. Extract the new + presentation block and keep draft derivation readable. +- New copy must use `t()` at render time and pass both i18n checks. +- An async save response must not overwrite a newer local toggle change. + +## Output contract + +Report summary, files changed, exact commands/outcomes, blockers, risks, and +update this task plus `plan.md` status in the same conversation. + +## Results + +- Added the third `auto_merge_enabled` draft, independent partial PATCH, + discard, and stale-response reconciliation to the shared Message Queue save + contributor. +- Added the localized default-on switch and same-source fallback explanation + with a 44px mobile touch target and no new scroll owner. +- Verified 34 focused component/API tests, web typecheck, zero-warning focused + ESLint, pseudo-locale generation, `i18n:check`, and `i18n:ratchet`. diff --git a/docs/plans/message-queue-auto-merge/task-05-auto-merge-e2e.md b/docs/plans/message-queue-auto-merge/task-05-auto-merge-e2e.md new file mode 100644 index 0000000000..df4cb4f65d --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-05-auto-merge-e2e.md @@ -0,0 +1,101 @@ +--- +id: "05-auto-merge-e2e" +title: "Prove automatic merge flows" +status: completed +wave: 5 +depends_on: ["04-auto-merge-settings-ui"] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 05: Prove automatic merge flows + +## Acceptance + +1. Desktop E2E proves the fresh/default value, auto-only save/reload, enabled + same-user compaction, and disabled separate-row behavior against the real + backend. +2. Mobile E2E proves the switch is reachable, touch-safe, persistence-equivalent, + free of horizontal overflow/nested scrolling, and clear of the save bar. +3. Every affected queue E2E scenario that requires multiple compatible rows + explicitly disables automatic merge, captures all three install-wide queue + settings, and restores its baseline in teardown even after failure. + +## Verification + +```bash +cd apps && pnpm install --frozen-lockfile +cd apps/web && pnpm e2e:run --host tests/system/message-queue-settings.spec.ts tests/chat/message-queue.spec.ts tests/chat/message-queue-reorder.spec.ts +cd apps/web && pnpm e2e:run --host --project mobile-chrome tests/system/mobile-message-queue-settings.spec.ts tests/chat/mobile-message-queue-management.spec.ts tests/chat/mobile-message-queue-reorder.spec.ts +``` + +## Files likely touched + +- `apps/web/e2e/tests/system/message-queue-settings.spec.ts` +- `apps/web/e2e/tests/system/mobile-message-queue-settings.spec.ts` +- `apps/web/e2e/tests/chat/message-queue.spec.ts` +- `apps/web/e2e/tests/chat/message-queue-reorder.spec.ts` +- `apps/web/e2e/tests/chat/mobile-message-queue-management.spec.ts` +- `apps/web/e2e/tests/chat/mobile-message-queue-reorder.spec.ts` +- A focused E2E settings helper under `apps/web/e2e/helpers/` if reuse avoids + duplicated baseline/restore logic. + +## Dependencies + +Task 04 and all backend tasks. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: default, enabled/disabled, cap, independence, persistence, and mobile + scenarios. +- Plan: `E2E Tests`. +- Existing patterns: API baseline capture in + `message-queue-settings.spec.ts`, busy-agent queue setup in + `message-queue.spec.ts`, and mobile layout assertions in + `mobile-message-queue-settings.spec.ts`. +- E2E fixture contract: install-wide mutable state must be captured and restored + by the owning spec; mobile changes accompany desktop changes. + +## Risks + +- Default-on behavior changes setup assumptions in manual merge, send-now, and + reorder tests. Audit all multi-row queue construction, not only new tests. +- Do not rely on test ordering or the fixture reset to restore install-wide + settings. +- Assert final authoritative state after the queue response/status event, not a + transient render. + +## Output contract + +Report summary, files changed, exact commands/outcomes, baseline restoration and +cleanup evidence, blockers, risks, and update this task plus `plan.md` status in +the same conversation. + +## Results + +- Added shared E2E setup that captures all three install-wide queue settings, + disables automatic merging for separate-row scenarios, and restores the full + baseline after every test. +- Added real-backend desktop coverage for default-on persistence, auto-only + updates, enabled compaction, disabled fallback, capacity-lock independence, + and member permissions. +- Added mobile coverage for touch geometry, save/discard persistence, overflow, + scroll ownership, and save-bar clearance. Audited attribution, steering, + sidebar-count, reorder, and queue-management scenarios that require separate + rows. +- Stabilized the existing row Send Now scenario by waiting for observable mock + turn progress before requesting cancellation. +- Verification passed: + - desktop Chromium: 20 tests; + - mobile Chromium: 4 tests; + - focused attribution, steering, and sidebar-count audit: 3 tests; + - focused E2E ESLint: zero warnings. +- Blockers: none. + +Review remediation makes shared teardown read the effective environment lock +before restoring and omit `max_per_session` when capacity is environment- +controlled. diff --git a/docs/plans/message-queue-auto-merge/task-06-auto-merge-docs.md b/docs/plans/message-queue-auto-merge/task-06-auto-merge-docs.md new file mode 100644 index 0000000000..ffbbd764d8 --- /dev/null +++ b/docs/plans/message-queue-auto-merge/task-06-auto-merge-docs.md @@ -0,0 +1,90 @@ +--- +id: "06-auto-merge-docs" +title: "Document automatic queue merging" +status: completed +wave: 6 +depends_on: ["05-auto-merge-e2e"] +plan: "plan.md" +spec: "../../specs/ui/message-queue-auto-merge.md" +--- + +# Task 06: Document automatic queue merging + +## Acceptance + +1. Public coordination, session, operations, configuration, and WebSocket docs + explain the default-on switch, strict same-source fallback, capacity-first + behavior, returned surviving ID, independent manual switch, and current Task + Behavior settings location. +2. Related queue management/manual merge specs link to this feature and show the + normalized three-field setting without changing manual merge behavior. +3. After all implementation evidence passes, the new spec/index and design + package statuses are synchronized, links resolve, public-doc validators pass, + and `git diff --check` is clean. + +## Verification + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +## Files likely touched + +- `docs/public/coordination.md` +- `docs/public/sessions-and-review.md` +- `docs/public/operations.md` +- `docs/public/configuration.md` +- `docs/public/websocket-api.md` +- `docs/specs/ui/message-queue-management.md` +- `docs/specs/ui/message-queue-merge.md` +- `docs/specs/ui/message-queue-auto-merge.md` +- `docs/specs/INDEX.md` +- `docs/plans/message-queue-auto-merge/plan.md` +- `docs/plans/message-queue-auto-merge/task-*.md` + +## Dependencies + +Task 05. + +## Parallelism + +Sequential. + +## Inputs + +- Full spec and completed task results. +- Plan: `Risks and boundaries` plus public-documentation impact. +- Existing public queue sections found by searching `Message Queue`, + `message.queue.add`, and `KANDEV_QUEUE_MAX_PER_SESSION`. +- Docs-maintainer rule: document observable behavior and configuration, keep + public terminology current, and run both validator commands. + +## Risks + +- Do not imply that automatic merging bypasses a full queue or retroactively + compacts pending rows. +- Distinguish message-queue auto merge from PR/MR auto-merge terminology. +- Mark the feature shipped only after implementation and E2E evidence exist. + +## Output contract + +Report summary, files changed, exact commands/outcomes, generated/cleanup +evidence, blockers, risks, and update this task plus `plan.md` status in the same +conversation. + +## Results + +- Updated public coordination, sessions, operations, configuration, and + WebSocket documentation with the current settings path and observable + automatic-merge contract. +- Linked the management and manual-merge specs, normalized the documented + three-field settings shape, and marked the implemented feature spec shipped. +- Verification passed: + - `node --test scripts/validate-public-docs.test.mjs` (60 tests); + - `node scripts/validate-public-docs.mjs` (41 published pages); + - `git diff --check`. +- Pseudo-locale regeneration completed, build output remained ignored, and the + lockfile stayed unchanged. +- Blockers: none. diff --git a/docs/public/configuration.md b/docs/public/configuration.md index 293c7315a9..8444a0b84e 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -358,7 +358,7 @@ These variables are supported by specific runtime components but are not YAML fi | `KANDEV_MCP_LOG_FILE` | unset | File path for per-agentctl MCP debug logs. Logs tool names, arguments, session IDs, results for tool errors, and timings; invalid paths warn and disable this sink. | | `OTEL_EXPORTER_OTLP_ENDPOINT` | unset | Enables OTLP/HTTP tracing for backend and agentctl spans; unset uses a no-op tracer. See the transport warning below. | -Changing concurrency values trades process pressure against throughput and requires a restart. The queue environment value is also read at startup. Without that environment override, an admin can use **Settings > General > Message Queue** to save an install-wide limit that applies live. `0` means unlimited. Lowering the live limit does not prune existing rows; new admissions remain blocked until the pending count drops below the limit, while retries of already accepted work remain eligible. +Changing concurrency values trades process pressure against throughput and requires a restart. The queue environment value is also read at startup. Under **Settings > Task Behavior > Message Queue**, an admin can save an install-wide capacity and independently control manual and automatic merging. All three values apply live and persist across restarts. `KANDEV_QUEUE_MAX_PER_SESSION` overrides and locks only capacity; neither merge switch has an environment override. `0` means unlimited. Lowering the live limit does not prune existing rows; new admissions remain blocked until the pending count drops below the limit, while retries of already accepted work remain eligible. The default-on automatic switch affects only later admissions and never bypasses capacity or sweeps existing rows. The current OTLP exporter strips an `http://` or `https://` prefix from the configured endpoint and always uses `WithInsecure()`. Treat this as implementation-bound cleartext transport: send it only to a trusted private collector over a protected network, not directly across an untrusted network. The service name is `kandev-agentctl`, and spans can include task/session/execution IDs plus raw agent-event JSON truncated to 8192 characters. That payload can contain prompts, files, and tool data. Use collector-side access controls and retention accordingly. diff --git a/docs/public/coordination.md b/docs/public/coordination.md index e4665fdf36..6df9998059 100644 --- a/docs/public/coordination.md +++ b/docs/public/coordination.md @@ -126,7 +126,9 @@ For predictable top-level creation, pass `repository_url`, `repository_id`, or ` | Created but not started | Starts the session with the message. | | Failed or cancelled | Returns an error. | -The default delivery mode is queued. Each session accepts 10 queued messages by default. An admin can change the install-wide limit under **Settings > General > Message Queue**; `0` means unlimited. The saved value applies immediately to new admissions without removing messages already waiting. `KANDEV_QUEUE_MAX_PER_SESSION` has higher precedence, locks the UI field, and requires a restart when changed; zero or a negative value means unlimited. Only one queued message drains per agent turn. When the cap is reached, the sender receives a structured `queue_full` error and should retry after space becomes available. +The default delivery mode is queued. Each session accepts 10 queued messages by default. An admin can change the install-wide limit under **Settings > Task Behavior > Message Queue**; `0` means unlimited. The saved value applies immediately to new admissions without removing messages already waiting. `KANDEV_QUEUE_MAX_PER_SESSION` has higher precedence, locks only the capacity field, and requires a restart when changed; zero or a negative value means unlimited. Only one queued message drains per agent turn. When the cap is reached, the sender receives a structured `queue_full` error and should retry after space becomes available. + +**Automatically merge consecutive messages** is enabled by default on the same settings card. After capacity admission succeeds, a new message may fold into the immediately preceding pending entry when both have the same strict source and compatible task, model, mode, metadata, attachments, and references. Otherwise it remains a separate FIFO entry. The earlier entry survives, so a successful admission may return an older queue-entry ID. The switch affects only later admissions and never sweeps rows already waiting. It is independent from **Enable queued message merging**, which controls the manual **Merge with above** action. In the task workbench, expand the queue chip to manage pending messages. Every visible pending row has **Remove**, whether it came from a user, another agent, workflow automation, or a server action. **Clear all** removes all visible pending rows in that session and releases their capacity. After removal, merge, or drain, displayed positions compact to `#1` through `#N` while FIFO order stays unchanged. Provenance still matters for editing and merging: only user-origin content can be edited. A row already reserved for delivery is hidden from the panel and cannot be cancelled there. diff --git a/docs/public/operations.md b/docs/public/operations.md index f803562259..78505ea5ab 100644 --- a/docs/public/operations.md +++ b/docs/public/operations.md @@ -89,11 +89,13 @@ kandev service logs -f Add `--system` to both commands for a system service. -## Message queue capacity +## Message queue settings -Open **Settings > General > Message Queue** to set the install-wide number of pending messages allowed in each session. The default is `10`; `0` means unlimited. Admin saves apply immediately to later admissions. Lowering the limit does not prune rows already waiting, so a queue at or above the new limit rejects new work until messages run or are removed. Delivery retries for work accepted before the change are not discarded by the lower cap. +Open **Settings > Task Behavior > Message Queue** to manage install-wide queue behavior. The default capacity is `10`; `0` means unlimited. Admin saves apply immediately to later admissions. Lowering the limit does not prune rows already waiting, so a queue at or above the new limit rejects new work until messages run or are removed. Delivery retries for work accepted before the change are not discarded by the lower cap. -`KANDEV_QUEUE_MAX_PER_SESSION` has higher precedence than the saved setting. A valid environment value makes the UI field read-only; zero or a negative value means unlimited. Invalid text is logged and ignored in favor of the saved setting or default. Environment changes require a backend restart, while UI changes do not. +`KANDEV_QUEUE_MAX_PER_SESSION` has higher precedence than the saved capacity. A valid environment value makes only that field read-only; zero or a negative value means unlimited. Invalid text is logged and ignored in favor of the saved setting or default. Environment changes require a backend restart, while UI changes do not. + +**Automatically merge consecutive messages** is on by default. Capacity is checked before any fold, so a full queue still rejects a compatible message. After admission, a new row folds only into its immediate pending predecessor when both rows have the same strict source and compatible task, model, mode, metadata, attachments, and references. Any mismatch or combined limit leaves the new row separate. The earlier row survives and its ID is returned. Turning the switch on does not compact existing rows. This setting is independent from **Enable queued message merging**, which controls the manual queue action. To recover capacity in one session, expand its queue chip in the task workbench. **Remove** deletes one visible pending row and **Clear all** deletes all visible pending rows, including user-, agent-, workflow-, and server-origin work. After removal, merge, or drain, displayed positions immediately compact to `#1` through `#N`; durable FIFO ordering is unchanged. A row already reserved for delivery is hidden and is not cancelled by either action. diff --git a/docs/public/sessions-and-review.md b/docs/public/sessions-and-review.md index 9a8ab1cb50..cb75228bb5 100644 --- a/docs/public/sessions-and-review.md +++ b/docs/public/sessions-and-review.md @@ -87,7 +87,9 @@ Delivery follows the target state: - a created session starts with the message as its first prompt; - a failed or cancelled session rejects the message. -The default pending-message limit is 10 per session. An admin can change it live under **Settings > General > Message Queue**; `0` removes the cap. A valid `KANDEV_QUEUE_MAX_PER_SESSION` value takes precedence and makes the UI field read-only; changing the environment still requires a restart. Malformed environment values are logged and ignored, so the saved setting or default applies instead. Lowering the saved limit does not delete entries already waiting. Interrupt delivery is restricted to a direct parent task messaging its child. Other senders must queue. +The default pending-message limit is 10 per session. An admin can change it live under **Settings > Task Behavior > Message Queue**; `0` removes the cap. A valid `KANDEV_QUEUE_MAX_PER_SESSION` value takes precedence and makes only the capacity field read-only; changing the environment still requires a restart. Malformed environment values are logged and ignored, so the saved setting or default applies instead. Lowering the saved limit does not delete entries already waiting. + +The same card enables **Automatically merge consecutive messages** by default. Once capacity admission succeeds, compatible consecutive messages from the same strict source fold into the earlier pending entry; incompatible messages remain separate. The earlier entry's ID survives, and only admissions after the setting is read are eligible. This behavior is independent from the manual **Enable queued message merging** switch. Interrupt delivery is restricted to a direct parent task messaging its child. Other senders must queue. For urgent replacement work, the parent should use `message_task_kandev` with `delivery_mode: "interrupt"`; this cancels the current approach and immediately tries to dispatch the new prompt, with a safe queued fallback. Use `stop_task_kandev` only for halt-only intent. A successful stop marks every accepted live child session `CANCELLED` and schedules graceful teardown asynchronously. Kandev then attempts to move an eligible unarchived, non-Office task from `IN_PROGRESS` or `SCHEDULING` to `REVIEW`; other task states remain unchanged. A child with no live execution returns idempotent `not_running`, and its worktrees, environment, commits, task record, descendants, and queued messages are preserved. See [Coordination](coordination.md) for the complete authority and lifecycle contract. @@ -296,7 +298,7 @@ Before moving a task to done: - **New Agent has no profiles:** create a profile compatible with the task executor. A profile for another executor is intentionally hidden. - **Summary or generated text fails:** configure the corresponding utility agent with an enabled ACP profile in **Settings > Utility Agents**. Repair any stale or disabled profile binding before retrying. - **Resume fails:** start fresh when the executor no longer has resumable session state, then supply a summary or copy the relevant context. -- **A peer message never arrives:** check the target session state and ID. Running sessions queue messages; failed or cancelled sessions reject them. For a full queue, expand its chip and run, remove, or clear pending work before retrying; an admin can also review the install-wide limit under **Settings > General > Message Queue**. +- **A peer message never arrives:** check the target session state and ID. Running sessions queue messages; failed or cancelled sessions reject them. For a full queue, expand its chip and run, remove, or clear pending work before retrying; an admin can also review the install-wide limit under **Settings > Task Behavior > Message Queue**. - **Changes is empty:** select the correct repository and comparison, then confirm the agent wrote inside the materialized task path. - **Review marks became stale:** the underlying diff changed. Re-review the new hash before marking the file complete. - **Walkthrough does not appear:** confirm an active task-MCP session exists and that the saved `changes-walkthrough` prompt was not removed or made invalid. diff --git a/docs/public/websocket-api.md b/docs/public/websocket-api.md index 63ffef4285..c0462dc216 100644 --- a/docs/public/websocket-api.md +++ b/docs/public/websocket-api.md @@ -253,6 +253,8 @@ A successful response returns the normalized query and an ordered `groups` array `message.queue.add` accepts the same optional array. `message.queue.update` also accepts it and treats the array as a replacement: send `[]` after removing every generated reference link so stale metadata is cleared. Queue status and message responses return validated entries under `metadata.entity_references`. +Ordinary `message.queue.add` admissions use the default-on automatic-merge setting. Capacity is enforced first. The newly admitted row folds only into its immediate pending predecessor when both rows have the same strict source and compatible task, model, plan mode, metadata, attachments, context files, and entity references; the merged context-file union is capped at 200 descriptors. Otherwise it remains separate. A successful fold returns the surviving earlier row's `entry_id`, while a fallback returns the new row's ID. The final state is published through `message.queue.status_changed`. Automatic merging applies only to later ordinary admissions, not existing rows, explicit append/coalesce/restore/retry/send-now operations, or the independent manual merge action. + `message.queue.merge` folds a queued entry into the entry directly above it. The payload requires `session_id` and `entry_id` (the source entry being merged away); `user_id` is optional and defaults to `user`. On success the response carries the surviving merged entry's `entry_id` (the target's id) and the server broadcasts an updated `message.queue.status_changed`. The request is rejected with `entry_not_found` when the entry was already drained or is not owned by the caller, with a validation error when no mergeable entry exists above it, and with `merge_reference_overflow` when the combined entity-reference lists would exceed the per-message cap (the merge is rejected atomically; neither row changes). If the agent is busy, use the `message.queue.*` operations rather than retrying `message.add`. Permission prompts are represented in persisted/session message data; answer one with `permission.respond`. Its payload requires `session_id` and `pending_id`, plus `option_id` unless `cancelled:true`; optional `rejected:true` distinguishes an explicit denial from dismissing the prompt. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index afc4ba56e0..425c22d313 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -186,10 +186,11 @@ Subscription quota tracking and per-agent cheap-model profile routing. | [entity-reference-composer](ui/entity-reference-composer.md) | draft | | [agent-launch-prompt-composer](ui/agent-launch-prompt-composer.md) | shipped | | [mermaid-rendering](ui/mermaid-rendering.md) | shipped | -[message-queue-management](ui/message-queue-management.md) | shipped | -[message-queue-merge](ui/message-queue-merge.md) | shipped | -[message-queue-reorder](ui/message-queue-reorder.md) | building | -[message-queue-send-now](ui/message-queue-send-now.md) | shipped | +| [message-queue-auto-merge](ui/message-queue-auto-merge.md) | shipped | +| [message-queue-management](ui/message-queue-management.md) | shipped | +| [message-queue-merge](ui/message-queue-merge.md) | shipped | +| [message-queue-reorder](ui/message-queue-reorder.md) | building | +| [message-queue-send-now](ui/message-queue-send-now.md) | shipped | | [settings-manual-save](ui/settings-manual-save.md) | shipped | | [settings-discovery](ui/settings-discovery.md) | shipped | | [executor-settings-card-spacing](ui/executor-settings-card-spacing.md) | shipped | diff --git a/docs/specs/ui/message-queue-auto-merge.md b/docs/specs/ui/message-queue-auto-merge.md new file mode 100644 index 0000000000..49777995f6 --- /dev/null +++ b/docs/specs/ui/message-queue-auto-merge.md @@ -0,0 +1,262 @@ +--- +status: shipped +created: 2026-08-12 +owner: kandev +--- + +# Automatically Merge Consecutive Queued Messages + +## Why + +Users and coordinating agents often send several short follow-ups while a +session is busy. Delivering every follow-up as a separate turn adds latency and +queue noise even when the messages came from the same source and can safely be +handled together. + +## What + +- **Settings > Task Behavior > Message Queue** exposes an + **Automatically merge consecutive messages** switch. The install-wide switch + is on by default. +- The switch applies immediately to messages admitted after the setting is + read. It does not compact entries that were already pending. +- When enabled, a newly admitted message is automatically folded into the + pending tail entry only when the two entries are consecutive and compatible. +- The earlier tail entry survives. It keeps its ID, FIFO position, queued time, + task, model, plan-mode value, and sender identity. Content and additive + context from the new entry follow the earlier entry in submission order. +- User entries have the same source only when both have the same non-reserved + `queued_by` identity. Agent entries have the same source only when both are + agent-owned and carry the same non-empty `sender_task_id`. +- Workflow, server, clarification, durable lifecycle, and reserved in-flight + entries are never automatic merge sources or targets. An agent entry without + `sender_task_id` is also excluded. +- Entries must have the same `task_id`, `model`, and `plan_mode`. Metadata other + than `entity_references` and `context_files` must be equivalent. This keeps + plugin source, sender-session provenance, parent-question state, workflow + state, and future metadata from being silently discarded. +- Merged content uses one blank line between two non-empty bodies. When either + body is empty, the other body is retained without an added leading or trailing + separator. +- Attachments retain target-then-source order. Automatic merging occurs only + when the combined attachment count and payload size remain within the shared + per-message limits. +- Entity references form a stable target-then-source union, deduplicated by + canonical reference, and must remain within the per-message reference limit. + Context files form a stable target-then-source union with exact duplicates + removed and remain capped at 200 descriptors. Exceeding that cap keeps the + newly admitted entry separate. +- If any compatibility, provenance, metadata, attachment, or reference check + fails, the newly admitted message remains a separate FIFO entry. Automatic + merge incompatibility never rejects an otherwise valid message. +- The existing manual **Merge with above** setting and action remain separate. + `merge_enabled` controls only the manual action; `auto_merge_enabled` controls + only admission-time automatic merging. Either switch may be enabled without + the other. +- No automatic-merge control is added to the queue panel. The queue panel + reflects the resulting authoritative queue after admission. + +## Data model + +The existing install-wide `settings` row with key `message_queue` gains one +boolean. Its normalized JSON value is: + +```json +{ + "max_per_session": 10, + "merge_enabled": true, + "auto_merge_enabled": true +} +``` + +- `auto_merge_enabled` defaults to `true` for a fresh installation. +- A persisted record that predates this field and omits it also resolves to + `true`. +- An explicit `false` remains false across reloads and backend restarts. +- The setting has no environment-variable override. The existing environment + lock for `max_per_session` does not lock either merge switch. + +No queued-message schema changes. A successful automatic merge updates the +surviving target's content, attachments, and additive metadata, then removes +the new source entry. A fallback leaves both rows intact. + +## API surface + +The existing HTTP settings routes retain their authorization and partial-PATCH +contract: + +```text +GET /api/v1/system/message-queue/settings +PATCH /api/v1/system/message-queue/settings +``` + +GET and PATCH responses include the new field in both views: + +```json +{ + "settings": { + "max_per_session": 10, + "merge_enabled": true, + "auto_merge_enabled": true + }, + "effective": { + "max_per_session": 10, + "merge_enabled": true, + "auto_merge_enabled": true, + "source": "default", + "locked": false + } +} +``` + +PATCH may name only the new field: + +```json +{ "auto_merge_enabled": false } +``` + +Omitted fields remain unchanged. A capacity environment lock blocks only a +PATCH that names `max_per_session`; an auto-only PATCH leaves the environment's +effective live capacity unchanged. + +No new WebSocket action is added. Existing admissions, including +`message.queue.add` and inter-task queued delivery, return the surviving target +entry when an automatic merge succeeds and return the new source entry when it +stays separate. `message.queue.status_changed` publishes the final queue state. +The explicit `message.queue.append`, coalescing, restoration, retry, edit, +reorder, and manual merge contracts do not gain automatic behavior. + +## Admission lifecycle and concurrency + +1. An admission snapshots `auto_merge_enabled` and the current queue capacity. +2. Normal validation and capacity admission run first. A queue that is already + full still returns `queue_full`; automatic merge does not bypass the cap. +3. The new entry is durably inserted. File-backed attachments are claimed using + the existing task/session ownership checks before any fold is attempted. +4. When the snapshotted setting is on, the backend atomically checks the new + entry and its immediate predecessor and either folds the new entry into that + predecessor or leaves both entries unchanged. +5. The final authoritative queue is published. + +Concurrent admissions for one session preserve their admission order. The +per-session admission serialization spans insertion, any staged attachment +claim, and the final fold or rollback. A later admission therefore cannot merge +into a provisional source whose attachment claim may still fail. The tail +check, target rewrite, and source removal are one repository mutation, so a +drain, remove, reorder, or manual merge can win before or after it but cannot +observe a half-merged row. If a concurrent action removes either candidate +before the automatic fold, no unrelated row is selected as a replacement +target. + +## Permissions + +- Signed-in install members may read the configured and effective values under + the existing system-settings rules. +- Only install admins may change `auto_merge_enabled`. +- Automatic merging grants no new queue mutation permission. It runs only as + part of an already-authorized admission and uses persisted sender provenance; + callers cannot name a target or claim a reserved sender identity. + +## Failure modes + +- **Incompatible tail:** the source remains a separate FIFO entry. No error is + shown solely because automatic merge was skipped. +- **Combined limit exceeded:** both messages remain separate; no attachment, + context file, or entity reference is truncated. +- **Attachment claim fails:** the newly inserted source entry is rolled back by + its own ID before automatic merge runs. An older target is never removed or + rewritten by this rollback. +- **Automatic fold storage failure after admission:** the transaction leaves + both rows separate, the failure is logged, and the already accepted message + is not reported as failed in a way that invites a duplicate retry. +- **Concurrent drain or mutation wins:** the fold is skipped or loses with no + partial write; the final queue status reconciles clients. +- **Invalid legacy settings JSON:** the existing invalid-setting fallback + applies. When the record is valid but omits `auto_merge_enabled`, the value is + `true`. + +## Persistence guarantees + +The saved switch survives backend restarts in the existing settings store. A +restart applies it before new queue admissions. Already merged rows remain +merged; disabling the switch does not reconstruct source rows. Already separate +rows remain separate; enabling the switch does not sweep them. + +## Responsive and mobile behavior + +- Desktop and mobile use the same Message Queue settings card, values, save + contributor, permissions, and validation. +- The new switch is a one-column settings row with a touch-safe control and + localized label and description. +- The existing `settings-scroll-container` remains the only page scroll owner. + The switch stays clear of the shared floating save bar and safe-area inset. +- The mobile queue panel does not gain a new control or scroll region. + +## Scenarios + +- **GIVEN** a fresh install, **WHEN** an admin opens Message Queue settings, + **THEN** automatic merge is shown as enabled. +- **GIVEN** a legacy `message_queue` setting without `auto_merge_enabled`, + **WHEN** the backend starts, **THEN** automatic merge is enabled and the next + save writes the normalized field. +- **GIVEN** automatic merge is enabled and two consecutive compatible user + messages are admitted, **WHEN** the second admission completes, **THEN** one + queued entry remains with both contents in order and the first entry's + identity. +- **GIVEN** automatic merge is disabled, **WHEN** the same two user messages are + admitted, **THEN** two FIFO entries remain. +- **GIVEN** two consecutive agent messages with the same non-empty + `sender_task_id`, **WHEN** the second is admitted, **THEN** they form one + agent-owned entry and the surviving ID can be targeted by immediate parent + interrupt delivery. +- **GIVEN** two agent messages from different sender tasks or sender sessions, + **WHEN** the later message is admitted, **THEN** it remains separate. +- **GIVEN** a user message follows an agent message, **WHEN** it is admitted, + **THEN** the two remain separate. +- **GIVEN** a workflow, server, clarification, or reserved lifecycle entry is a + source or tail candidate, **WHEN** another message is admitted, **THEN** no + automatic fold uses that entry. +- **GIVEN** two same-source entries differ in task, model, plan mode, plugin + source, parent-question metadata, or other non-additive metadata, **WHEN** the + later entry is admitted, **THEN** both remain separate with their metadata + unchanged. +- **GIVEN** compatible entries carry attachments, context files, and entity + references within all limits, **WHEN** they merge, **THEN** every value is + present in target-then-source order with only exact/canonical duplicates + removed. +- **GIVEN** a combined attachment, context-file, or entity-reference limit would + be exceeded, **WHEN** the later message is admitted, **THEN** both entries + remain separate and the admission succeeds if queue capacity permits. +- **GIVEN** a compatible file-backed message whose attachment claim fails, + **WHEN** admission rolls back, **THEN** the prior tail is byte-for-byte + unchanged. +- **GIVEN** the queue is already at its capacity, **WHEN** a compatible message + is submitted, **THEN** the existing `queue_full` response is returned and no + row changes. +- **GIVEN** automatic merge is on and manual merge is off, **WHEN** compatible + messages are admitted, **THEN** they merge automatically while the manual + action remains unavailable; the inverse switch combination leaves admissions + separate while preserving the manual action. +- **GIVEN** an environment value controls queue capacity, **WHEN** an admin + saves only `auto_merge_enabled`, **THEN** the automatic setting changes and + the environment-controlled effective and live capacity remain unchanged. +- **GIVEN** pending rows predate an off-to-on settings change, **WHEN** the + setting is saved, **THEN** those rows remain unchanged and only later + admissions are eligible. +- **GIVEN** a phone viewport, **WHEN** an admin changes the switch and saves, + **THEN** the value persists without horizontal overflow, nested scrolling, or + overlap with the shared save bar. + +## Out of scope + +- Retroactively compacting an existing queue. +- Merging non-consecutive entries or searching past an incompatible tail. +- A per-workspace, per-task, per-session, or queue-panel override. +- Changing the manual **Merge with above** compatibility or error behavior. +- Changing `message.queue.append`, workflow coalescing, delivery restoration, + retry, send-now, edit, or reorder semantics. +- Bypassing queue capacity because a message might merge. + +## Implementation plan + +[Implementation plan](../../plans/message-queue-auto-merge/plan.md) diff --git a/docs/specs/ui/message-queue-management.md b/docs/specs/ui/message-queue-management.md index 1f0a83ae9b..6378fd89e7 100644 --- a/docs/specs/ui/message-queue-management.md +++ b/docs/specs/ui/message-queue-management.md @@ -26,6 +26,9 @@ remains full. Those same rows have no individual remove action. - Removal does not change provenance rules for other operations. Only user-owned messages may be edited. Merge behavior remains defined by [Merge Enqueued Messages Individually](message-queue-merge.md), and + admission-time compaction is defined by + [Automatically Merge Consecutive Queued Messages](message-queue-auto-merge.md). + The two merge settings are independent. The interrupt-and-dispatch behavior is defined by [Send Queued Messages Now](message-queue-send-now.md). - Successful removal publishes the existing @@ -41,8 +44,9 @@ remains full. Those same rows have no individual remove action. ### Capacity setting -- **Settings > General > Message Queue** exposes the maximum number of persisted - messages allowed per session. +- **Settings > Task Behavior > Message Queue** exposes the maximum number of + persisted messages allowed per session alongside independent manual and + automatic merge switches. - The default is `10`. A positive integer sets a cap; `0` means unlimited. - A saved setting applies immediately to later admissions. Existing entries are never trimmed. If a queue already exceeds a newly lowered limit, new @@ -90,9 +94,15 @@ GET response and PATCH response: ```json { - "settings": { "max_per_session": 10 }, + "settings": { + "max_per_session": 10, + "merge_enabled": true, + "auto_merge_enabled": true + }, "effective": { "max_per_session": 10, + "merge_enabled": true, + "auto_merge_enabled": true, "source": "default", "locked": false } @@ -106,9 +116,11 @@ persisted value, or the default when no override exists. PATCH accepts: { "max_per_session": 25 } ``` -PATCH rejects negative values with `400`, rejects a valid environment lock -with `409`, and requires the admin role. GET requires an authenticated install -user under the existing system-route middleware. +PATCH is partial: omitted capacity, manual-merge, and automatic-merge fields +remain unchanged. It rejects negative capacity with `400`, rejects a valid +environment capacity lock with `409` only when the patch names +`max_per_session`, and requires the admin role. GET requires an authenticated +install user under the existing system-route middleware. ## Data Model @@ -116,7 +128,11 @@ No queue-table migration is required. Capacity uses the existing install-wide `settings` table under key `message_queue` with JSON value: ```json -{ "max_per_session": 25 } +{ + "max_per_session": 25, + "merge_enabled": true, + "auto_merge_enabled": true +} ``` `queued_by` stays unchanged as immutable provenance. The existing @@ -210,8 +226,9 @@ starts accepting work. **THEN** no capacity error is produced solely by queue length and the page explains that the queue is unlimited. - **GIVEN** a phone viewport, **WHEN** the user opens the queue panel and the - General settings menu, **THEN** remove/clear are touch-sized and Message Queue - is reachable without horizontal overflow or a second page scroll owner. + Task Behavior settings menu, **THEN** remove/clear are touch-sized and Message + Queue is reachable without horizontal overflow or a second page scroll + owner. ## Out of Scope diff --git a/docs/specs/ui/message-queue-merge.md b/docs/specs/ui/message-queue-merge.md index 670d93f035..d1cebc4aa2 100644 --- a/docs/specs/ui/message-queue-merge.md +++ b/docs/specs/ui/message-queue-merge.md @@ -7,8 +7,12 @@ owner: kandev # Merge Enqueued Messages Individually Queue-wide removal and capacity behavior is defined separately by -[Manage Pending Message Queues](message-queue-management.md). This spec governs -only the merge operation. +[Manage Pending Message Queues](message-queue-management.md), and admission-time +compaction is defined by +[Automatically Merge Consecutive Queued Messages](message-queue-auto-merge.md). +This spec governs only the manual merge operation. Its `merge_enabled` setting +does not control automatic merging, and `auto_merge_enabled` does not change +the behavior or compatibility rules below. ## Why