diff --git a/apps/backend/internal/office/testharness/routes.go b/apps/backend/internal/office/testharness/routes.go index 28cf9c8705..bfbde905d2 100644 --- a/apps/backend/internal/office/testharness/routes.go +++ b/apps/backend/internal/office/testharness/routes.go @@ -554,6 +554,34 @@ func ensureSeededTurn(ctx context.Context, repo *sqliterepo.Repository, sessionI return turn.ID, nil } +// createSeededTurn always creates a fresh turn rather than reusing the +// session's active one, so a spec can hold a real open turn on the session +// while seeding a second, independently-timed turn alongside it (see +// seedMessageRequest.NewTurn). startedAt/completedAt default to "now" and +// "open" respectively when nil. +func createSeededTurn(ctx context.Context, repo *sqliterepo.Repository, sessionID, taskID string, startedAt, completedAt *time.Time) (string, error) { + now := time.Now().UTC() + turn := &models.Turn{ + ID: uuid.New().String(), + TaskSessionID: sessionID, + TaskID: taskID, + StartedAt: now, + CreatedAt: now, + UpdatedAt: now, + } + if startedAt != nil { + turn.StartedAt = startedAt.UTC() + } + if completedAt != nil { + ca := completedAt.UTC() + turn.CompletedAt = &ca + } + if err := repo.CreateTurn(ctx, turn); err != nil { + return "", err + } + return turn.ID, nil +} + func publishSessionStateChanged(ctx context.Context, eventBus bus.EventBus, session *models.TaskSession, log *logger.Logger) { if eventBus == nil { return @@ -592,6 +620,18 @@ type seedMessageRequest struct { // message is attached to), so e2e specs can exercise the message metadata // dialog's turn_metadata field end to end. TurnMetadata map[string]interface{} `json:"turn_metadata,omitempty"` + // NewTurn, when true, creates a brand-new turn for this message instead + // of reusing the session's active turn. Lets a spec construct two turns + // on the same session (e.g. a still-open turn plus a separate, + // later-started lifecycle-shaped turn) to exercise current-turn-authority + // ordering (D1) end to end, which ensureSeededTurn's reuse-active-turn + // behavior cannot produce. + NewTurn bool `json:"new_turn,omitempty"` + // TurnStartedAt/TurnCompletedAt set the new turn's timestamps when + // NewTurn is true, so a spec can control D1's ordering directly. Ignored + // when NewTurn is false. + TurnStartedAt *time.Time `json:"turn_started_at,omitempty"` + TurnCompletedAt *time.Time `json:"turn_completed_at,omitempty"` } // seedMessageHandler inserts a synthetic message, ensuring an active turn @@ -621,7 +661,12 @@ func seedMessageHandler(repo *sqliterepo.Repository, eventBus bus.EventBus, log return } - turnID, err := ensureSeededTurn(ctx, repo, session.ID, session.TaskID) + var turnID string + if req.NewTurn { + turnID, err = createSeededTurn(ctx, repo, session.ID, session.TaskID, req.TurnStartedAt, req.TurnCompletedAt) + } else { + turnID, err = ensureSeededTurn(ctx, repo, session.ID, session.TaskID) + } if err != nil { log.Error("test harness: ensure turn failed", zap.Error(err)) c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) diff --git a/apps/backend/internal/orchestrator/clarification_guard_test.go b/apps/backend/internal/orchestrator/clarification_guard_test.go index 876cc125bb..5c803ee52a 100644 --- a/apps/backend/internal/orchestrator/clarification_guard_test.go +++ b/apps/backend/internal/orchestrator/clarification_guard_test.go @@ -5,7 +5,9 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/task/models" + taskservice "github.com/kandev/kandev/internal/task/service" ) func TestSessionHasPendingClarification(t *testing.T) { @@ -104,6 +106,62 @@ func TestSessionHasLiveClarificationIgnoresDetachedBundle(t *testing.T) { } } +// TestSessionHasPendingClarificationSurvivesLifecycleShadowViaRealWritePath +// covers the guard-level consumer named in the spec's Verification Notes as +// "the most severe consumer": sessionHasPendingClarification must still see +// the pending clarification on a real open turn after a synthetic +// lifecycle-only turn is written through the actual task-service write path +// (Service.CreateMessage with CompletedTurn: true), the same call +// Service.createCompletedTurn makes on agent resume. Both turns share this +// service's own repo so the guard reads what task/service actually wrote, +// not a hand-seeded row shaped to match. +func TestSessionHasPendingClarificationSurvivesLifecycleShadowViaRealWritePath(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "task-lifecycle-guard", "session-lifecycle-guard", "step1") + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + + taskSvc := taskservice.NewService(taskservice.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, bus.NewMemoryEventBus(testLogger()), testLogger(), taskservice.RepositoryDiscoveryConfig{}) + + now := time.Now().UTC() + requireNoError(t, repo.CreateTurn(ctx, &models.Turn{ + ID: "turn-open-guard", TaskSessionID: "session-lifecycle-guard", TaskID: "task-lifecycle-guard", + StartedAt: now, CreatedAt: now, + })) + requireNoError(t, repo.CreateMessage(ctx, &models.Message{ + ID: "clarification-guard", TaskSessionID: "session-lifecycle-guard", TaskID: "task-lifecycle-guard", + TurnID: "turn-open-guard", AuthorType: models.MessageAuthorAgent, + Type: models.MessageTypeClarificationRequest, Content: "q", CreatedAt: now, + Metadata: map[string]any{"pending_id": "pending-guard", "status": "pending"}, + })) + + if _, err := taskSvc.CreateMessage(ctx, &taskservice.CreateMessageRequest{ + TaskSessionID: "session-lifecycle-guard", + Content: "resumed", + CompletedTurn: true, + }); err != nil { + t.Fatalf("create lifecycle turn via CreateMessage: %v", err) + } + + if !svc.sessionHasPendingClarification(ctx, "session-lifecycle-guard") { + t.Fatal("guard cleared after a lifecycle-only turn shadowed the open turn's pending clarification") + } +} + func seedPendingClarificationMessage(t *testing.T, repo interface { CreateTurn(ctx context.Context, turn *models.Turn) error CreateMessage(ctx context.Context, message *models.Message) error diff --git a/apps/backend/internal/task/dto/converters_test.go b/apps/backend/internal/task/dto/converters_test.go index 5b5b8fc00d..ca7d155633 100644 --- a/apps/backend/internal/task/dto/converters_test.go +++ b/apps/backend/internal/task/dto/converters_test.go @@ -280,3 +280,41 @@ func TestFromTurnPreservesSubsecondOrderingPrecision(t *testing.T) { t.Fatalf("time.Parse(RFC3339, StartedAt): %v", err) } } + +// TestFromTurnPreservesLifecycleOnlyMetadata is AC-11's DTO half: the +// GET .../turns handler (internal/task/handlers/task_http_handlers.go) does +// nothing but call FromTurn per row and gin.Context.JSON the result, so +// pinning that FromTurn copies metadata verbatim - and that json.Marshal +// keeps the lifecycle_only key - proves the response payload carries it, +// without standing up an HTTP server. turnHistoryPredicate (proven unchanged +// by TestTurnReadsHideEmptyUnpublishedReservationUntilMessageEvidence and +// friends in the sqlite package) is what keeps this turn in the result set +// FromTurn is ever handed. +func TestFromTurnPreservesLifecycleOnlyMetadata(t *testing.T) { + now := time.Date(2026, time.August, 24, 9, 0, 0, 0, time.UTC) + got := FromTurn(&models.Turn{ + ID: "turn-lifecycle", + StartedAt: now, + CreatedAt: now, + UpdatedAt: now, + Metadata: map[string]interface{}{models.TurnMetaKeyLifecycleOnly: true}, + }) + + if lifecycleOnly, ok := got.Metadata[models.TurnMetaKeyLifecycleOnly]; !ok || lifecycleOnly != true { + t.Fatalf("TurnDTO.Metadata[%q] = %v, want true", models.TurnMetaKeyLifecycleOnly, lifecycleOnly) + } + + raw, err := json.Marshal(got) + if err != nil { + t.Fatalf("json.Marshal(TurnDTO): %v", err) + } + var decoded struct { + Metadata map[string]interface{} `json:"metadata"` + } + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("json.Unmarshal(TurnDTO JSON): %v", err) + } + if lifecycleOnly, ok := decoded.Metadata[models.TurnMetaKeyLifecycleOnly]; !ok || lifecycleOnly != true { + t.Fatalf("decoded metadata.lifecycle_only = %v, want true (raw JSON: %s)", lifecycleOnly, raw) + } +} diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 02dadbfb23..d0ce51f411 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -290,6 +290,12 @@ const TurnMetaKeyRuntimeConfigSnapshot = "runtime_config_snapshot" // was in when the turn started. Absent when the task held no step. const TurnMetaKeyWorkflowStepIDAtStart = "workflow_step_id_at_start" +// TurnMetaKeyLifecycleOnly marks a turn created only to parent a lifecycle +// message (for example the agent_boot script_execution message on resume). +// A lifecycle turn never reflects real agent work and must never be current- +// turn authority, so every current-turn resolution site excludes it. +const TurnMetaKeyLifecycleOnly = "lifecycle_only" + // TurnMetaKeyPromptDispatchPending marks a successor created before agentctl // acknowledges its prompt. Empty marked turns are not current-turn authority // unless dispatch ambiguity was recorded; publication clears the marker, while diff --git a/apps/backend/internal/task/repository/sqlite/clarification_bundle_query.go b/apps/backend/internal/task/repository/sqlite/clarification_bundle_query.go index dc61b58f6f..d4aa2e4b21 100644 --- a/apps/backend/internal/task/repository/sqlite/clarification_bundle_query.go +++ b/apps/backend/internal/task/repository/sqlite/clarification_bundle_query.go @@ -116,14 +116,15 @@ func clarificationBundleQuery(drv, whereExtra string) string { ) notParentQuestion := dialect.ExcludeTruthyMetadataPredicate(drv, "m.metadata", "parent_question") nonTerminalSession := nonTerminalSessionPredicate("m") + predicate, orderBy := currentTurnAuthority(drv, "turn_row") currentTurn := fmt.Sprintf(`m.turn_id = ( SELECT turn_row.id FROM task_session_turns turn_row WHERE turn_row.task_session_id = m.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 - )`, turnAuthorityPredicate(drv, "turn_row")) + )`, predicate, orderBy) return fmt.Sprintf(` SELECT b.pending_id, b.session_id, b.task_id, b.created_at FROM ( diff --git a/apps/backend/internal/task/repository/sqlite/clarification_bundle_query_test.go b/apps/backend/internal/task/repository/sqlite/clarification_bundle_query_test.go index c03c679505..3ee53c1481 100644 --- a/apps/backend/internal/task/repository/sqlite/clarification_bundle_query_test.go +++ b/apps/backend/internal/task/repository/sqlite/clarification_bundle_query_test.go @@ -32,7 +32,8 @@ func seedBundleTurn(t *testing.T, repo *Repository, turnID, sessionID, taskID st // seedBundleTurnAt creates a turn with an explicit started_at/created_at, so // a test can control which of two turns on the same session is newest per -// turnAuthorityPredicate's ORDER BY (started_at DESC, created_at DESC, id DESC). +// currentTurnAuthority's ordering: open turns (completed_at IS NULL) before +// completed ones, then started_at DESC, created_at DESC, id DESC. func seedBundleTurnAt(t *testing.T, repo *Repository, turnID, sessionID, taskID string, at time.Time) { t.Helper() _, err := repo.db.Exec(repo.db.Rebind(` diff --git a/apps/backend/internal/task/repository/sqlite/message.go b/apps/backend/internal/task/repository/sqlite/message.go index 767e44a133..c088f50718 100644 --- a/apps/backend/internal/task/repository/sqlite/message.go +++ b/apps/backend/internal/task/repository/sqlite/message.go @@ -582,13 +582,14 @@ func (r *Repository) FindMessagesByPendingID(ctx context.Context, pendingID stri // cleanup; they never become current input authority. func (r *Repository) FindActiveClarificationMessagesBySessionID(ctx context.Context, sessionID string) ([]*models.Message, error) { drv := r.ro.DriverName() + predicate, orderBy := currentTurnAuthority(drv, "turn_row") query := fmt.Sprintf(` WITH current_turn AS ( SELECT turn_row.id FROM task_session_turns turn_row WHERE turn_row.task_session_id = ? AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) SELECT m.id, m.task_session_id, m.task_id, m.turn_id, m.author_type, m.author_id, @@ -599,7 +600,7 @@ func (r *Repository) FindActiveClarificationMessagesBySessionID(ctx context.Cont AND m.type = 'clarification_request' AND COALESCE(%s, '') IN ('', 'pending') ORDER BY m.created_at ASC, m.id ASC - `, turnAuthorityPredicate(drv, "turn_row"), dialect.JSONExtract(drv, "m.metadata", "status")) + `, predicate, orderBy, dialect.JSONExtract(drv, "m.metadata", "status")) // First sessionID selects current-turn authority; second scopes messages so // a malformed cross-session turn reference cannot leak another session's row. rows, err := r.ro.QueryContext(ctx, r.ro.Rebind(query), sessionID, sessionID) @@ -661,6 +662,7 @@ func pendingActionsBySessionQuery(driverName string, placeholders []string) stri // Terminal sessions quarantine pending history even when best-effort expiry // persistence failed. Both message CTEs inherit the requested-session // boundary through their task_session_id and turn_id joins to current_turn. + predicate, orderBy := currentTurnAuthority(driverName, "turn_row") return fmt.Sprintf(` WITH current_turn AS ( SELECT task_session_id, id AS turn_id @@ -669,7 +671,7 @@ func pendingActionsBySessionQuery(driverName string, placeholders []string) stri id, ROW_NUMBER() OVER ( PARTITION BY task_session_id - ORDER BY started_at DESC, created_at DESC, id DESC + ORDER BY %s ) AS rn FROM task_session_turns turn_row WHERE turn_row.task_session_id IN (%s) @@ -706,7 +708,7 @@ func pendingActionsBySessionQuery(driverName string, placeholders []string) stri SELECT task_session_id, 'permission' AS action FROM latest_permissions WHERE rn = 1 AND status IN ('', 'pending') - `, placeholderList, turnAuthorityPredicate(driverName, "turn_row"), + `, orderBy, placeholderList, predicate, nonTerminalSessionPredicate("turn_row"), statusExpr, statusExpr, permissionOrderExpr) } diff --git a/apps/backend/internal/task/repository/sqlite/message_clarification_response.go b/apps/backend/internal/task/repository/sqlite/message_clarification_response.go index 3c5c65a088..a491e184e7 100644 --- a/apps/backend/internal/task/repository/sqlite/message_clarification_response.go +++ b/apps/backend/internal/task/repository/sqlite/message_clarification_response.go @@ -54,6 +54,7 @@ func (r *Repository) DetachActiveClarificationMessagesBySessionID( return nil, err } updatedAt := r.nowUTC() + predicate, orderBy := currentTurnAuthority(drv, "turn_row") query := fmt.Sprintf(` UPDATE task_session_messages SET metadata = %s, updated_at = ? @@ -66,7 +67,7 @@ func (r *Repository) DetachActiveClarificationMessagesBySessionID( FROM task_session_turns turn_row WHERE turn_row.task_session_id = task_session_messages.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) RETURNING id, task_session_id, task_id, turn_id, author_type, author_id, @@ -74,7 +75,7 @@ func (r *Repository) DetachActiveClarificationMessagesBySessionID( `, clarificationDetachedMetadataExpr(drv), dialect.JSONExtract(drv, "task_session_messages.metadata", "status"), clarificationNotDetachedPredicate(drv), - turnAuthorityPredicate(drv, "turn_row")) + predicate, orderBy) rows, err := tx.QueryxContext(ctx, r.db.Rebind(query), updatedAt, sessionID) if err != nil { return nil, fmt.Errorf("detach active clarification messages: %w", err) @@ -111,6 +112,7 @@ func (r *Repository) ExpireActiveClarificationBundle( return nil, err } updatedAt := r.nowUTC() + predicate, orderBy := currentTurnAuthority(drv, "turn_row") query := fmt.Sprintf(` UPDATE task_session_messages SET metadata = %s, updated_at = ? @@ -123,7 +125,7 @@ func (r *Repository) ExpireActiveClarificationBundle( FROM task_session_turns turn_row WHERE turn_row.task_session_id = task_session_messages.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) RETURNING id, task_session_id, task_id, turn_id, author_type, author_id, @@ -131,7 +133,7 @@ func (r *Repository) ExpireActiveClarificationBundle( `, clarificationExpiredMetadataExpr(drv), dialect.JSONExtract(drv, "task_session_messages.metadata", "status"), dialect.JSONExtract(drv, "task_session_messages.metadata", "pending_id"), - turnAuthorityPredicate(drv, "turn_row")) + predicate, orderBy) rows, err := tx.QueryxContext(ctx, r.db.Rebind(query), updatedAt, sessionID, pendingID) if err != nil { return nil, fmt.Errorf("expire active clarification messages: %w", err) @@ -345,6 +347,7 @@ func (r *Repository) loadRestorableClarificationBundle( ) ([]*models.Message, error) { pendingIDExpr := dialect.JSONExtract(drv, "m.metadata", "pending_id") bundlePendingIDExpr := dialect.JSONExtract(drv, "bundle.metadata", "pending_id") + predicate, orderBy := currentTurnAuthority(drv, "turn_row") // A pending ID spanning message types, sessions, or turns is malformed. The // NOT EXISTS guard intentionally makes the whole bundle ineligible to restore. query := fmt.Sprintf(` @@ -360,7 +363,7 @@ func (r *Repository) loadRestorableClarificationBundle( FROM task_session_turns turn_row WHERE turn_row.task_session_id = m.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) AND NOT EXISTS ( @@ -377,7 +380,7 @@ func (r *Repository) loadRestorableClarificationBundle( `, pendingIDExpr, nonTerminalSessionPredicate("m"), clarificationResponseDeliveryPendingPredicate(drv, "m"), - turnAuthorityPredicate(drv, "turn_row"), + predicate, orderBy, bundlePendingIDExpr, ) rows, err := tx.QueryxContext( @@ -406,6 +409,7 @@ func (r *Repository) restoreClarificationMessages( ) error { updatedAt := r.nowUTC() statusExpr := dialect.JSONExtract(drv, "metadata", "status") + predicate, orderBy := currentTurnAuthority(drv, "turn_row") updateQuery := r.db.Rebind(fmt.Sprintf(` UPDATE task_session_messages SET metadata = ?, updated_at = ? @@ -417,14 +421,14 @@ func (r *Repository) restoreClarificationMessages( FROM task_session_turns turn_row WHERE turn_row.task_session_id = task_session_messages.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) `, statusExpr, clarificationResponseDeliveryPendingPredicate(drv, "task_session_messages"), nonTerminalSessionPredicate("task_session_messages"), - turnAuthorityPredicate(drv, "turn_row"), + predicate, orderBy, )) restoredMetadataByMessage := make([]map[string]interface{}, len(messages)) for i, message := range messages { @@ -479,6 +483,7 @@ func (r *Repository) claimActiveClarificationBundle( // it is replaced with the requested terminal status before this transaction // commits or is rolled back with the transaction. claimAt := r.nowUTC() + predicate, orderBy := currentTurnAuthority(drv, "turn_row") claimQuery := fmt.Sprintf(` UPDATE task_session_messages SET metadata = %s, updated_at = ? @@ -491,7 +496,7 @@ func (r *Repository) claimActiveClarificationBundle( FROM task_session_turns turn_row WHERE turn_row.task_session_id = task_session_messages.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) AND NOT EXISTS ( @@ -506,7 +511,7 @@ func (r *Repository) claimActiveClarificationBundle( ) `, dialect.JSONSet(drv, "metadata", "status", clarificationStatusResponding), pendingIDExpr, statusExpr, nonTerminalSessionPredicate("task_session_messages"), - turnAuthorityPredicate(drv, "turn_row"), bundlePendingIDExpr) + predicate, orderBy, bundlePendingIDExpr) // pendingID is bound once for the outer bundle and once for the malformed- // bundle NOT EXISTS guard. result, err := tx.ExecContext(ctx, r.db.Rebind(claimQuery), claimAt, pendingID, pendingID) @@ -526,6 +531,7 @@ func (r *Repository) loadClaimedClarificationBundle( drv, pendingID string, ) ([]*models.Message, error) { claimedStatusExpr := dialect.JSONExtract(drv, "m.metadata", "status") + predicate, orderBy := currentTurnAuthority(drv, "turn_row") rows, err := tx.QueryxContext(ctx, r.db.Rebind(fmt.Sprintf(` SELECT m.id, m.task_session_id, m.task_id, m.turn_id, m.author_type, m.author_id, m.content, m.requests_input, m.type, m.metadata, m.created_at, m.updated_at @@ -536,12 +542,12 @@ func (r *Repository) loadClaimedClarificationBundle( FROM task_session_turns turn_row WHERE turn_row.task_session_id = m.task_session_id AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 ) ORDER BY m.created_at ASC, m.id ASC `, dialect.JSONExtract(drv, "m.metadata", "pending_id"), claimedStatusExpr, - turnAuthorityPredicate(drv, "turn_row"))), pendingID, clarificationStatusResponding) + predicate, orderBy)), pendingID, clarificationStatusResponding) if err != nil { return nil, fmt.Errorf("load claimed clarification bundle: %w", err) } diff --git a/apps/backend/internal/task/repository/sqlite/session.go b/apps/backend/internal/task/repository/sqlite/session.go index 7b2bde45a7..03628747f6 100644 --- a/apps/backend/internal/task/repository/sqlite/session.go +++ b/apps/backend/internal/task/repository/sqlite/session.go @@ -216,17 +216,21 @@ func (r *Repository) GetTurn(ctx context.Context, id string) (*models.Turn, erro return scanTurnRow(row) } -// GetActiveTurnBySessionID gets the currently active (non-completed) turn for a session +// GetActiveTurnBySessionID gets the currently active (non-completed) turn for +// a session. This is not the same query as "current turn": it keeps its own +// completed_at IS NULL filter alongside currentTurnAuthority's predicate and +// ordering (D10) so AbandonOpenTurns' re-bury loop keeps working. func (r *Repository) GetActiveTurnBySessionID(ctx context.Context, sessionID string) (*models.Turn, error) { + predicate, orderBy := currentTurnAuthority(r.ro.DriverName(), "turn_row") query := fmt.Sprintf(` SELECT id, task_session_id, task_id, execution_profile_id, route_generation, started_at, completed_at, metadata, created_at, updated_at FROM task_session_turns turn_row WHERE turn_row.task_session_id = ? AND turn_row.completed_at IS NULL AND %s - ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC + ORDER BY %s LIMIT 1 - `, turnAuthorityPredicate(r.ro.DriverName(), "turn_row")) + `, predicate, orderBy) row := r.ro.QueryRowContext(ctx, r.ro.Rebind(query), sessionID) return scanTurnRow(row) } diff --git a/apps/backend/internal/task/repository/sqlite/testdata/current_turn_resolution.json b/apps/backend/internal/task/repository/sqlite/testdata/current_turn_resolution.json new file mode 100644 index 0000000000..6bb758ae90 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/testdata/current_turn_resolution.json @@ -0,0 +1,156 @@ +{ + "cases": [ + { + "name": "lifecycle_only boolean true excludes the newer turn", + "turns": [ + { "id": "bool-true-real", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "bool-true-lifecycle", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": true } } + ], + "expected_current_turn_id": "bool-true-real" + }, + { + "name": "lifecycle_only number 1 excludes the newer turn", + "turns": [ + { "id": "num-one-real", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "num-one-lifecycle", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": 1 } } + ], + "expected_current_turn_id": "num-one-real" + }, + { + "name": "lifecycle_only string true excludes the newer turn", + "turns": [ + { "id": "str-true-real", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "str-true-lifecycle", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "true" } } + ], + "expected_current_turn_id": "str-true-real" + }, + { + "name": "lifecycle_only string 1 excludes the newer turn", + "turns": [ + { "id": "str-one-real", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "str-one-lifecycle", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "1" } } + ], + "expected_current_turn_id": "str-one-real" + }, + { + "name": "lifecycle_only null keeps the newer turn eligible", + "turns": [ + { "id": "null-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "null-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": null } } + ], + "expected_current_turn_id": "null-newer" + }, + { + "name": "lifecycle_only absent key keeps the newer turn eligible", + "turns": [ + { "id": "absent-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "absent-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": {} } + ], + "expected_current_turn_id": "absent-newer" + }, + { + "name": "lifecycle_only boolean false keeps the newer turn eligible", + "turns": [ + { "id": "bool-false-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "bool-false-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": false } } + ], + "expected_current_turn_id": "bool-false-newer" + }, + { + "name": "lifecycle_only number 0 keeps the newer turn eligible", + "turns": [ + { "id": "num-zero-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "num-zero-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": 0 } } + ], + "expected_current_turn_id": "num-zero-newer" + }, + { + "name": "lifecycle_only empty string keeps the newer turn eligible", + "turns": [ + { "id": "empty-str-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "empty-str-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "" } } + ], + "expected_current_turn_id": "empty-str-newer" + }, + { + "name": "lifecycle_only string false keeps the newer turn eligible", + "turns": [ + { "id": "str-false-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "str-false-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "false" } } + ], + "expected_current_turn_id": "str-false-newer" + }, + { + "name": "lifecycle_only string 0 keeps the newer turn eligible", + "turns": [ + { "id": "str-zero-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "str-zero-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "0" } } + ], + "expected_current_turn_id": "str-zero-newer" + }, + { + "name": "lifecycle_only string yes keeps the newer turn eligible", + "turns": [ + { "id": "yes-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "yes-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": "yes" } } + ], + "expected_current_turn_id": "yes-newer" + }, + { + "name": "lifecycle_only empty object keeps the newer turn eligible", + "turns": [ + { "id": "obj-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "obj-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": {} } } + ], + "expected_current_turn_id": "obj-newer" + }, + { + "name": "lifecycle_only empty array keeps the newer turn eligible", + "turns": [ + { "id": "arr-older", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "arr-newer", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": [] } } + ], + "expected_current_turn_id": "arr-newer" + }, + { + "name": "a session with only lifecycle turns resolves no current turn", + "turns": [ + { "id": "all-lifecycle-a", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": { "lifecycle_only": true } }, + { "id": "all-lifecycle-b", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": "2026-08-14T11:00:00Z", "metadata": { "lifecycle_only": "1" } } + ], + "expected_current_turn_id": null + }, + { + "name": "an open turn beats a later completed turn", + "turns": [ + { "id": "order-open", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": null, "metadata": {} }, + { "id": "order-completed-later", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T11:00:00Z", "completed_at": "2026-08-14T11:05:00Z", "metadata": {} } + ], + "expected_current_turn_id": "order-open" + }, + { + "name": "later started_at wins among completed turns", + "turns": [ + { "id": "order-started-later", "started_at": "2026-08-14T11:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} }, + { "id": "order-started-earlier", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} } + ], + "expected_current_turn_id": "order-started-later" + }, + { + "name": "tie on started_at broken by later created_at", + "turns": [ + { "id": "order-created-later", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:01:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} }, + { "id": "order-created-earlier", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} } + ], + "expected_current_turn_id": "order-created-later" + }, + { + "name": "tie on started_at and created_at broken by higher id", + "turns": [ + { "id": "order-tie-2", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} }, + { "id": "order-tie-1", "started_at": "2026-08-14T10:00:00Z", "created_at": "2026-08-14T10:00:00Z", "completed_at": "2026-08-14T12:00:00Z", "metadata": {} } + ], + "expected_current_turn_id": "order-tie-2" + } + ] +} diff --git a/apps/backend/internal/task/repository/sqlite/turn_authority.go b/apps/backend/internal/task/repository/sqlite/turn_authority.go index 6d4d385547..b33bfaac22 100644 --- a/apps/backend/internal/task/repository/sqlite/turn_authority.go +++ b/apps/backend/internal/task/repository/sqlite/turn_authority.go @@ -59,6 +59,33 @@ func turnDispatchAttemptedPredicate(driverName, turnAlias string) string { ) } +// currentTurnAuthority composes turnAuthorityPredicate with R1's lifecycle +// exclusion into predicate, and D1's total order into orderBy. Every +// current-turn resolution site takes both values from this one call and +// hand-writes neither (AC-3). +// +// predicate is a single parenthesized conjunct for `WHERE ... AND %s`. +// orderBy is a bare, turnAlias-qualified expression list with no ORDER BY +// keyword and no LIMIT, so it drops into both `ORDER BY %s` and +// `ROW_NUMBER() OVER (... ORDER BY %s)`. Its first key ranks an open turn +// (completed_at IS NULL) ahead of a completed one regardless of timestamps; +// the remaining keys break ties by started_at, created_at, then id — all +// descending, with id (the primary key) providing a total order. +func currentTurnAuthority(driverName, turnAlias string) (predicate, orderBy string) { + authority := turnAuthorityPredicate(driverName, turnAlias) + lifecycle := turnLifecycleOnlyPredicate(driverName, turnAlias) + predicate = fmt.Sprintf("(%s AND NOT (%s))", authority, lifecycle) + orderBy = fmt.Sprintf( + "CASE WHEN %s.completed_at IS NULL THEN 0 ELSE 1 END ASC, %s.started_at DESC, %s.created_at DESC, %s.id DESC", + turnAlias, turnAlias, turnAlias, turnAlias, + ) + return predicate, orderBy +} + +func turnLifecycleOnlyPredicate(driverName, turnAlias string) string { + return turnMetadataFlagPredicate(driverName, turnAlias, models.TurnMetaKeyLifecycleOnly) +} + func turnMetadataFlagPredicate(driverName, turnAlias, key string) string { value := dialect.JSONExtract( driverName, diff --git a/apps/backend/internal/task/repository/sqlite/turn_authority_fixture_test.go b/apps/backend/internal/task/repository/sqlite/turn_authority_fixture_test.go new file mode 100644 index 0000000000..2b8e19aad6 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/turn_authority_fixture_test.go @@ -0,0 +1,128 @@ +package sqlite + +import ( + "context" + "database/sql" + "encoding/json" + "errors" + "fmt" + "os" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +// turnResolutionFixture mirrors +// testdata/current_turn_resolution.json (AC-10). It is the shared contract +// between this Go repository test and the TypeScript unit test that loads +// the same file from apps/web/lib/utils/pending-clarification-turn-authority.test.ts +// (or a sibling file it imports) - one artifact both implementations are +// proven to agree with, not two independently-trusted ones. +type turnResolutionFixture struct { + Cases []turnResolutionCase `json:"cases"` +} + +type turnResolutionCase struct { + Name string `json:"name"` + Turns []turnResolutionTurn `json:"turns"` + ExpectedCurrentTurnID *string `json:"expected_current_turn_id"` +} + +type turnResolutionTurn struct { + ID string `json:"id"` + StartedAt time.Time `json:"started_at"` + CreatedAt time.Time `json:"created_at"` + CompletedAt *time.Time `json:"completed_at"` + Metadata map[string]interface{} `json:"metadata"` +} + +// loadTurnResolutionFixture SHALL fail the test (not skip) when the fixture +// is missing, unreadable, or unparseable - a fixture that silently stops +// being read is indistinguishable from two implementations that agree. +func loadTurnResolutionFixture(t *testing.T) turnResolutionFixture { + t.Helper() + raw, err := os.ReadFile("testdata/current_turn_resolution.json") + if err != nil { + t.Fatalf("read current_turn_resolution.json: %v", err) + } + var fixture turnResolutionFixture + if err := json.Unmarshal(raw, &fixture); err != nil { + t.Fatalf("parse current_turn_resolution.json: %v", err) + } + if len(fixture.Cases) == 0 { + t.Fatal("current_turn_resolution.json has no cases") + } + return fixture +} + +// resolveCurrentTurnID runs currentTurnAuthority's predicate/orderBy directly +// against the turns table, the same CTE shape +// FindActiveClarificationMessagesBySessionID uses internally, without its +// join to messages - so it resolves a session's current turn regardless of +// whether any message references it. Returns nil when no turn resolves. +func resolveCurrentTurnID(t *testing.T, repo *Repository, sessionID string) *string { + t.Helper() + predicate, orderBy := currentTurnAuthority(repo.ro.DriverName(), "turn_row") + query := fmt.Sprintf(` + SELECT turn_row.id + FROM task_session_turns turn_row + WHERE turn_row.task_session_id = ? + AND %s + ORDER BY %s + LIMIT 1 + `, predicate, orderBy) + row := repo.ro.QueryRowContext(context.Background(), repo.ro.Rebind(query), sessionID) + var id string + switch err := row.Scan(&id); { + case errors.Is(err, sql.ErrNoRows): + return nil + case err != nil: + t.Fatalf("resolve current turn for session %s: %v", sessionID, err) + return nil + default: + return &id + } +} + +// TestCurrentTurnResolutionFixture is AC-10: every case in the shared +// fixture, run against the real Go currentTurnAuthority resolution. The +// TypeScript counterpart runs the identical cases through +// newestDurableTurnId. Neither test may filter the shared cases; each may +// add its own. +func TestCurrentTurnResolutionFixture(t *testing.T) { + fixture := loadTurnResolutionFixture(t) + for i, tc := range fixture.Cases { + t.Run(tc.Name, func(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + taskID := fmt.Sprintf("task-fixture-%d", i) + sessionID := fmt.Sprintf("session-fixture-%d", i) + seedSessionForTurns(t, repo, taskID, sessionID) + + for _, turn := range tc.Turns { + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: turn.ID, + TaskSessionID: sessionID, + TaskID: taskID, + StartedAt: turn.StartedAt, + CreatedAt: turn.CreatedAt, + CompletedAt: turn.CompletedAt, + Metadata: turn.Metadata, + }); err != nil { + t.Fatalf("seed fixture turn %s: %v", turn.ID, err) + } + } + + got := resolveCurrentTurnID(t, repo, sessionID) + switch { + case tc.ExpectedCurrentTurnID == nil && got != nil: + t.Fatalf("current turn = %s, want none to resolve", *got) + case tc.ExpectedCurrentTurnID != nil && got == nil: + t.Fatalf("current turn = none, want %s", *tc.ExpectedCurrentTurnID) + case tc.ExpectedCurrentTurnID != nil && got != nil && *got != *tc.ExpectedCurrentTurnID: + t.Fatalf("current turn = %s, want %s", *got, *tc.ExpectedCurrentTurnID) + } + }) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/turn_authority_postgres_test.go b/apps/backend/internal/task/repository/sqlite/turn_authority_postgres_test.go new file mode 100644 index 0000000000..889a3893a5 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/turn_authority_postgres_test.go @@ -0,0 +1,57 @@ +package sqlite + +import ( + "context" + "fmt" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/testutil" +) + +// TestCurrentTurnResolutionFixturePostgres is AC-10/D9: the same shared +// fixture TestCurrentTurnResolutionFixture proves against SQLite, run +// against PostgreSQL so D1's key 1 (open beats completed) and the +// lifecycle_only exclusion resolve identically on both dialects. Skips +// unless KANDEV_TEST_POSTGRES_DSN is set. Neither this test nor the SQLite +// one may filter the shared cases; each may add its own. +func TestCurrentTurnResolutionFixturePostgres(t *testing.T) { + fixture := loadTurnResolutionFixture(t) + for i, tc := range fixture.Cases { + t.Run(tc.Name, func(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init postgres schema: %v", err) + } + ctx := context.Background() + taskID := fmt.Sprintf("task-fixture-pg-%d", i) + sessionID := fmt.Sprintf("session-fixture-pg-%d", i) + seedSessionForTurns(t, repo, taskID, sessionID) + + for _, turn := range tc.Turns { + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: turn.ID, + TaskSessionID: sessionID, + TaskID: taskID, + StartedAt: turn.StartedAt, + CreatedAt: turn.CreatedAt, + CompletedAt: turn.CompletedAt, + Metadata: turn.Metadata, + }); err != nil { + t.Fatalf("seed fixture turn %s: %v", turn.ID, err) + } + } + + got := resolveCurrentTurnID(t, repo, sessionID) + switch { + case tc.ExpectedCurrentTurnID == nil && got != nil: + t.Fatalf("current turn = %s, want none to resolve", *got) + case tc.ExpectedCurrentTurnID != nil && got == nil: + t.Fatalf("current turn = none, want %s", *tc.ExpectedCurrentTurnID) + case tc.ExpectedCurrentTurnID != nil && got != nil && *got != *tc.ExpectedCurrentTurnID: + t.Fatalf("current turn = %s, want %s", *got, *tc.ExpectedCurrentTurnID) + } + }) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/turn_authority_test.go b/apps/backend/internal/task/repository/sqlite/turn_authority_test.go index 2311797e52..ff70c4004f 100644 --- a/apps/backend/internal/task/repository/sqlite/turn_authority_test.go +++ b/apps/backend/internal/task/repository/sqlite/turn_authority_test.go @@ -8,6 +8,146 @@ import ( "github.com/kandev/kandev/internal/task/models" ) +// seedAuthorityTurn creates a turn with explicit started_at/created_at and an +// optional completed_at, so a test can control every key of D1's total order +// independently. A nil completedAt leaves the turn open. +func seedAuthorityTurn( + t *testing.T, + repo *Repository, + taskID, sessionID, turnID string, + startedAt, createdAt time.Time, + completedAt *time.Time, + metadata map[string]interface{}, +) { + t.Helper() + if err := repo.CreateTurn(context.Background(), &models.Turn{ + ID: turnID, TaskID: taskID, TaskSessionID: sessionID, + StartedAt: startedAt, CreatedAt: createdAt, CompletedAt: completedAt, + Metadata: metadata, + }); err != nil { + t.Fatalf("seed authority turn %s: %v", turnID, err) + } +} + +// TestCurrentTurnAuthorityOrdering exercises D1's total order end-to-end +// through FindActiveClarificationMessagesBySessionID, which applies +// currentTurnAuthority with no extra completed_at filter (unlike +// GetActiveTurnBySessionID) so both open and completed turns compete. Each +// case seeds a pending clarification on exactly one of two candidate turns +// and asserts whether that message survives, which is only true when its +// turn resolves as current. +func TestCurrentTurnAuthorityOrdering(t *testing.T) { + base := time.Date(2026, time.August, 20, 12, 0, 0, 0, time.UTC) + completed := base.Add(-time.Hour) + for _, tt := range []struct { + name string + winnerID string + winnerStarted time.Time + winnerCreated time.Time + winnerDone *time.Time + loserID string + loserStarted time.Time + loserCreated time.Time + loserDone *time.Time + }{ + { + // AC-5/AC-6: an open turn outranks a completed one regardless of + // which started earlier (R2). The completed turn here carries no + // lifecycle_only marker, matching a pre-existing legacy row. + name: "open turn beats later completed turn", + winnerID: "turn-ord-open", + winnerStarted: base, + winnerCreated: base, + winnerDone: nil, + loserID: "turn-ord-completed-later", + loserStarted: base.Add(time.Hour), + loserCreated: base.Add(time.Hour), + loserDone: &completed, + }, + { + name: "later started_at wins among completed turns", + winnerID: "turn-ord-started-later", + winnerStarted: base.Add(time.Hour), + winnerCreated: base, + winnerDone: &completed, + loserID: "turn-ord-started-earlier", + loserStarted: base, + loserCreated: base, + loserDone: &completed, + }, + { + name: "tie on started_at broken by later created_at", + winnerID: "turn-ord-created-later", + winnerStarted: base, + winnerCreated: base.Add(time.Minute), + winnerDone: &completed, + loserID: "turn-ord-created-earlier", + loserStarted: base, + loserCreated: base, + loserDone: &completed, + }, + { + name: "tie on started_at and created_at broken by higher id", + winnerID: "turn-ord-tie-2", + winnerStarted: base, + winnerCreated: base, + winnerDone: &completed, + loserID: "turn-ord-tie-1", + loserStarted: base, + loserCreated: base, + loserDone: &completed, + }, + } { + t.Run(tt.name, func(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + taskID := "task-" + tt.name + sessionID := "session-" + tt.name + seedSessionForTurns(t, repo, taskID, sessionID) + + seedAuthorityTurn(t, repo, taskID, sessionID, tt.winnerID, tt.winnerStarted, tt.winnerCreated, tt.winnerDone, nil) + seedAuthorityTurn(t, repo, taskID, sessionID, tt.loserID, tt.loserStarted, tt.loserCreated, tt.loserDone, nil) + insertClarificationMessage(t, repo, "msg-"+tt.winnerID, sessionID, taskID, tt.winnerID, "pending-"+tt.winnerID, "q1", "pending", 0, tt.winnerStarted) + insertClarificationMessage(t, repo, "msg-"+tt.loserID, sessionID, taskID, tt.loserID, "pending-"+tt.loserID, "q1", "pending", 0, tt.loserStarted) + + messages, err := repo.FindActiveClarificationMessagesBySessionID(ctx, sessionID) + if err != nil { + t.Fatalf("FindActiveClarificationMessagesBySessionID: %v", err) + } + if len(messages) != 1 || messages[0].TurnID != tt.winnerID { + t.Fatalf("messages = %+v, want exactly the winner turn %s's message", messages, tt.winnerID) + } + }) + } +} + +// TestCurrentTurnAuthorityTwoOpenTurnsResolvesDeterministically covers D2's +// observed half: at most one open turn per session is an invariant the +// repository does not enforce with a constraint, so resolution must still +// behave deterministically (not error) if it is ever violated. With both +// candidates open, D1's first key ties and the remaining keys (started_at, +// created_at, id, all descending) pick the winner exactly as they would for +// two completed turns. +func TestCurrentTurnAuthorityTwoOpenTurnsResolvesDeterministically(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + const taskID = "task-two-open-turns" + const sessionID = "session-two-open-turns" + seedSessionForTurns(t, repo, taskID, sessionID) + base := time.Date(2026, time.August, 21, 9, 0, 0, 0, time.UTC) + + seedAuthorityTurn(t, repo, taskID, sessionID, "turn-open-older", base, base, nil, nil) + seedAuthorityTurn(t, repo, taskID, sessionID, "turn-open-newer", base.Add(time.Minute), base.Add(time.Minute), nil, nil) + + active, err := repo.GetActiveTurnBySessionID(ctx, sessionID) + if err != nil { + t.Fatalf("GetActiveTurnBySessionID: %v", err) + } + if active == nil || active.ID != "turn-open-newer" { + t.Fatalf("active turn = %#v, want turn-open-newer (later started_at wins deterministically)", active) + } +} + func TestTurnAuthorityToleratesStringFlagEncodings(t *testing.T) { for _, tt := range []struct { name string diff --git a/apps/backend/internal/task/repository/sqlite/turn_history_lifecycle_test.go b/apps/backend/internal/task/repository/sqlite/turn_history_lifecycle_test.go new file mode 100644 index 0000000000..f49d46ea70 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/turn_history_lifecycle_test.go @@ -0,0 +1,70 @@ +package sqlite + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +// TestListTurnsBySessionKeepsLifecycleTurnVisible is AC-11's predicate half: +// turnHistoryPredicate SHALL keep including lifecycle turns in visible +// history, so the boot message stays grouped under its own turn in the +// transcript, even though currentTurnAuthority now excludes that same turn +// from current-turn resolution (AC-2/AC-3). GET .../turns +// (internal/task/handlers/task_http_handlers.go) calls ListTurnsBySession +// directly and DTO-converts every row it returns with no further filtering, +// so a lifecycle turn present here is a lifecycle turn present in that +// response. +func TestListTurnsBySessionKeepsLifecycleTurnVisible(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + seedSessionForTurns(t, repo, "task-turn-history-lifecycle", "session-turn-history-lifecycle") + + base := time.Date(2026, 8, 24, 9, 0, 0, 0, time.UTC) + lifecycleTurn := &models.Turn{ + ID: "turn-lifecycle-history", + TaskSessionID: "session-turn-history-lifecycle", + TaskID: "task-turn-history-lifecycle", + StartedAt: base, + CreatedAt: base, + CompletedAt: &base, + Metadata: map[string]interface{}{models.TurnMetaKeyLifecycleOnly: true}, + } + conversationalTurn := &models.Turn{ + ID: "turn-conversational-history", + TaskSessionID: "session-turn-history-lifecycle", + TaskID: "task-turn-history-lifecycle", + StartedAt: base.Add(time.Minute), + CreatedAt: base.Add(time.Minute), + } + for _, turn := range []*models.Turn{lifecycleTurn, conversationalTurn} { + if err := repo.CreateTurn(ctx, turn); err != nil { + t.Fatalf("CreateTurn(%s): %v", turn.ID, err) + } + } + + turns, err := repo.ListTurnsBySession(ctx, "session-turn-history-lifecycle") + if err != nil { + t.Fatalf("ListTurnsBySession: %v", err) + } + + ids := make(map[string]*models.Turn, len(turns)) + for _, turn := range turns { + ids[turn.ID] = turn + } + if len(turns) != 2 { + t.Fatalf("ListTurnsBySession returned %d turns %+v, want both the lifecycle and conversational turn", len(turns), turns) + } + got, ok := ids[lifecycleTurn.ID] + if !ok { + t.Fatalf("ListTurnsBySession dropped the lifecycle turn %q; turnHistoryPredicate must keep it visible", lifecycleTurn.ID) + } + if lifecycleOnly, ok := got.Metadata[models.TurnMetaKeyLifecycleOnly]; !ok || lifecycleOnly != true { + t.Fatalf("returned lifecycle turn metadata[%q] = %v, want true", models.TurnMetaKeyLifecycleOnly, lifecycleOnly) + } + if _, ok := ids[conversationalTurn.ID]; !ok { + t.Fatalf("ListTurnsBySession dropped the conversational turn %q", conversationalTurn.ID) + } +} diff --git a/apps/backend/internal/task/service/service_turns.go b/apps/backend/internal/task/service/service_turns.go index a5fdee100c..825fe0a2ba 100644 --- a/apps/backend/internal/task/service/service_turns.go +++ b/apps/backend/internal/task/service/service_turns.go @@ -276,6 +276,8 @@ func (s *Service) createCompletedTurn(ctx context.Context, session *models.TaskS return nil, errors.New("cannot create completed turn without a session") } now := time.Now().UTC() + metadata := turnStartRuntimeMetadata(session) + metadata[models.TurnMetaKeyLifecycleOnly] = true turn := &models.Turn{ ID: uuid.New().String(), TaskSessionID: session.ID, @@ -284,7 +286,7 @@ func (s *Service) createCompletedTurn(ctx context.Context, session *models.TaskS RouteGeneration: session.RouteGeneration, StartedAt: now, CompletedAt: &now, - Metadata: turnStartRuntimeMetadata(session), + Metadata: metadata, CreatedAt: now, UpdatedAt: now, } diff --git a/apps/backend/internal/task/service/service_turns_lifecycle_shadow_test.go b/apps/backend/internal/task/service/service_turns_lifecycle_shadow_test.go new file mode 100644 index 0000000000..5e59d27c71 --- /dev/null +++ b/apps/backend/internal/task/service/service_turns_lifecycle_shadow_test.go @@ -0,0 +1,312 @@ +package service + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// newLifecycleShadowTestService seeds one workspace/workflow/task/session so +// AC-4's scenario has a real session to hang turns and messages off. +func newLifecycleShadowTestService(t *testing.T) (*Service, *sqliterepo.Repository, string, string) { + t.Helper() + svc, _, repo := createTestService(t) + ctx := context.Background() + const wsID, wfID, taskID, sessionID = "ws-cta", "wf-cta", "task-cta", "sess-cta" + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: wsID, Name: "CTA"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ID: wfID, WorkspaceID: wsID, Name: "flow"}); err != nil { + t.Fatalf("create workflow: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: taskID, WorkspaceID: wsID, WorkflowID: wfID, WorkflowStepID: "step-1", + Title: "CTA", State: v1.TaskStateCreated, Priority: "medium", + }); err != nil { + t.Fatalf("create task: %v", err) + } + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: sessionID, TaskID: taskID, State: models.TaskSessionStateCreated, + }); err != nil { + t.Fatalf("create session: %v", err) + } + return svc, repo, taskID, sessionID +} + +// TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarification is +// AC-4: a near-simultaneous lifecycle turn created through the real +// Service.CreateMessage(CompletedTurn: true) write path must never shadow a +// genuinely open turn holding a pending clarification, across every named +// consumer surface (list_pending_questions_kandev, GetPendingActionsBySessionIDs, +// and internal/clarification's respond path). Verified at the service and +// repository layers per the spec's own scoping, not browser-level: the +// respond surface is exercised through CompleteActiveClarificationBundle, +// the exact repository method internal/clarification's resolver calls. +func TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarification(t *testing.T) { + svc, repo, taskID, sessionID := newLifecycleShadowTestService(t) + ctx := context.Background() + + // The conversational turn is seeded already-completed (its agent turn + // finished; the clarification it raised is still unanswered) and MUST + // start in the past: createCompletedTurn stamps time.Now().UTC(), so an + // open conversational turn would already win D1's open-beats-completed + // tie-break for the wrong reason (R2), without ever exercising R1's + // lifecycle_only exclusion. Only pitting two completed turns against each + // other - where D1's ordering alone would let the later (synthetic) one + // win - proves the exclusion is what saves the message. + past := time.Now().UTC().Add(-time.Hour) + const conversationTurnID = "turn-conversation" + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: conversationTurnID, TaskSessionID: sessionID, TaskID: taskID, + StartedAt: past, CreatedAt: past, CompletedAt: &past, UpdatedAt: past, + }); err != nil { + t.Fatalf("seed conversational turn: %v", err) + } + + const pendingID = "pending-cta" + if err := repo.CreateMessage(ctx, &models.Message{ + ID: "msg-cta-question", TaskSessionID: sessionID, TaskID: taskID, + TurnID: conversationTurnID, AuthorType: models.MessageAuthorAgent, + Type: models.MessageTypeClarificationRequest, + Metadata: map[string]interface{}{ + "pending_id": pendingID, + "question_id": "q1", + "status": "pending", + "question_index": 0, + "question_total": 1, + "question": map[string]interface{}{ + "id": "q1", + "title": "title", + "prompt": "prompt", + "options": []map[string]interface{}{ + {"option_id": "opt1", "label": "Yes"}, + }, + }, + }, + CreatedAt: past, + }); err != nil { + t.Fatalf("seed pending clarification message: %v", err) + } + + // R1: reproduce the defect's trigger through the real write path - an + // agent resume creates a synthetic completed turn, never an inserted row + // imitating one. + if _, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: sessionID, + Content: "resumed", + CompletedTurn: true, + }); err != nil { + t.Fatalf("create lifecycle turn via CreateMessage: %v", err) + } + + t.Run("list_pending_questions_kandev surface", func(t *testing.T) { + page, err := repo.ListUnresolvedClarificationBundles(ctx, models.ListClarificationBundlesOptions{ + Unscoped: true, Limit: 10, + }) + if err != nil { + t.Fatalf("ListUnresolvedClarificationBundles: %v", err) + } + if len(page.Bundles) != 1 || page.Bundles[0].PendingID != pendingID { + t.Fatalf("bundles = %+v, want exactly pending bundle %s", page.Bundles, pendingID) + } + }) + + t.Run("GetPendingActionsBySessionIDs surface", func(t *testing.T) { + actions, err := repo.GetPendingActionsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetPendingActionsBySessionIDs: %v", err) + } + if actions[sessionID] != models.TaskPendingActionClarification { + t.Fatalf("pending action for %s = %v, want %v", sessionID, actions[sessionID], models.TaskPendingActionClarification) + } + }) + + t.Run("internal/clarification respond surface", func(t *testing.T) { + completed, claimed, err := repo.CompleteActiveClarificationBundle(ctx, pendingID, "answered", map[string]interface{}{ + "q1": "opt1", + }) + if err != nil { + t.Fatalf("CompleteActiveClarificationBundle: %v", err) + } + if !claimed || len(completed) != 1 { + t.Fatalf("claimed=%v completed=%+v, want a single successful claim, not a conflict", claimed, completed) + } + }) +} + +// TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarificationOpenTurn +// covers AC-4's literal precondition: the real conversational turn is +// genuinely OPEN (never completed) when the synthetic lifecycle turn is +// created after it. TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarification +// deliberately completes the conversational turn to isolate R1's +// lifecycle_only exclusion from D1's open-beats-completed tie-break; this +// test instead exercises the scenario as it actually happens in production - +// an agent resume racing a still-open, awaiting-input turn - across the same +// three named consumer surfaces. +func TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarificationOpenTurn(t *testing.T) { + svc, repo, taskID, sessionID := newLifecycleShadowTestService(t) + ctx := context.Background() + + past := time.Now().UTC().Add(-time.Hour) + const conversationTurnID = "turn-conversation-open" + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: conversationTurnID, TaskSessionID: sessionID, TaskID: taskID, + StartedAt: past, CreatedAt: past, UpdatedAt: past, + }); err != nil { + t.Fatalf("seed open conversational turn: %v", err) + } + + const pendingID = "pending-cta-open" + if err := repo.CreateMessage(ctx, &models.Message{ + ID: "msg-cta-open-question", TaskSessionID: sessionID, TaskID: taskID, + TurnID: conversationTurnID, AuthorType: models.MessageAuthorAgent, + Type: models.MessageTypeClarificationRequest, + Metadata: map[string]interface{}{ + "pending_id": pendingID, + "question_id": "q1", + "status": "pending", + "question_index": 0, + "question_total": 1, + "question": map[string]interface{}{ + "id": "q1", + "title": "title", + "prompt": "prompt", + "options": []map[string]interface{}{ + {"option_id": "opt1", "label": "Yes"}, + }, + }, + }, + CreatedAt: past, + }); err != nil { + t.Fatalf("seed pending clarification message: %v", err) + } + + // R1/D5: reproduce the defect's trigger through the real write path - an + // agent resume creates a synthetic completed turn after the real turn, + // while the real turn is still open and awaiting the clarification's + // answer. + if _, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: sessionID, + Content: "resumed", + CompletedTurn: true, + }); err != nil { + t.Fatalf("create lifecycle turn via CreateMessage: %v", err) + } + + t.Run("list_pending_questions_kandev surface", func(t *testing.T) { + page, err := repo.ListUnresolvedClarificationBundles(ctx, models.ListClarificationBundlesOptions{ + Unscoped: true, Limit: 10, + }) + if err != nil { + t.Fatalf("ListUnresolvedClarificationBundles: %v", err) + } + if len(page.Bundles) != 1 || page.Bundles[0].PendingID != pendingID { + t.Fatalf("bundles = %+v, want exactly pending bundle %s", page.Bundles, pendingID) + } + }) + + t.Run("GetPendingActionsBySessionIDs surface", func(t *testing.T) { + actions, err := repo.GetPendingActionsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetPendingActionsBySessionIDs: %v", err) + } + if actions[sessionID] != models.TaskPendingActionClarification { + t.Fatalf("pending action for %s = %v, want %v", sessionID, actions[sessionID], models.TaskPendingActionClarification) + } + }) + + t.Run("internal/clarification respond surface", func(t *testing.T) { + completed, claimed, err := repo.CompleteActiveClarificationBundle(ctx, pendingID, "answered", map[string]interface{}{ + "q1": "opt1", + }) + if err != nil { + t.Fatalf("CompleteActiveClarificationBundle: %v", err) + } + if !claimed || len(completed) != 1 { + t.Fatalf("claimed=%v completed=%+v, want a single successful claim, not a conflict", claimed, completed) + } + }) +} + +// TestBootResumeLifecycleTurnNeverClaimsClarificationRequest is AC-7 / D5: the +// boot-on-resume path (Service.CreateMessage with CompletedTurn: true, +// Type: "script_execution", mirroring bootMsgAdapter.CreateMessage in +// internal/backendapp/worktree.go) creates a lifecycle turn, but a +// clarification_request written afterward through the real getOrStartTurn +// path must still land on the open conversational turn, never the lifecycle +// one - regardless of the lifecycle turn's later timestamp. +func TestBootResumeLifecycleTurnNeverClaimsClarificationRequest(t *testing.T) { + svc, repo, taskID, sessionID := newLifecycleShadowTestService(t) + ctx := context.Background() + + past := time.Now().UTC().Add(-time.Hour) + const conversationTurnID = "turn-conversation-ac7" + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: conversationTurnID, TaskSessionID: sessionID, TaskID: taskID, + StartedAt: past, CreatedAt: past, UpdatedAt: past, + }); err != nil { + t.Fatalf("seed open conversational turn: %v", err) + } + + bootMsg, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: sessionID, + Content: "", + AuthorType: "agent", + Type: "script_execution", + CompletedTurn: true, + Metadata: map[string]interface{}{ + "script_type": "agent_boot", + "status": "running", + "is_resuming": true, + }, + }) + if err != nil { + t.Fatalf("create boot message via CompletedTurn path: %v", err) + } + if bootMsg.TurnID == conversationTurnID { + t.Fatalf("boot message landed on the conversational turn %s, want a fresh lifecycle turn", conversationTurnID) + } + lifecycleTurnID := bootMsg.TurnID + + clarification, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: sessionID, + AuthorType: "agent", + Type: string(models.MessageTypeClarificationRequest), + Content: "q", + Metadata: map[string]interface{}{ + "pending_id": "pending-ac7", + "question_id": "q1", + "status": "pending", + }, + }) + if err != nil { + t.Fatalf("create clarification_request after boot resume: %v", err) + } + if clarification.TurnID != conversationTurnID { + t.Fatalf("clarification_request turn_id = %s, want the open conversational turn %s", clarification.TurnID, conversationTurnID) + } + if clarification.TurnID == lifecycleTurnID { + t.Fatalf("clarification_request landed on the lifecycle turn %s, want the open conversational turn", lifecycleTurnID) + } +} + +// TestCreateMessageRequestCompletedTurnNotJSONSettable is AC-7's second half: +// CreateMessageRequest.CompletedTurn is tagged json:"-" so no API client can +// set it. Unmarshalling a body that sets completed_turn SHALL leave the field +// false; a caller later making it settable while writing a +// clarification_request would let any client hide its own question. +func TestCreateMessageRequestCompletedTurnNotJSONSettable(t *testing.T) { + var req CreateMessageRequest + body := []byte(`{"session_id":"sess","content":"hi","completed_turn":true}`) + if err := json.Unmarshal(body, &req); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if req.CompletedTurn { + t.Fatal("CompletedTurn = true after unmarshalling a body setting completed_turn, want false (json:\"-\")") + } +} diff --git a/apps/backend/internal/task/service/service_turns_step_stamp_test.go b/apps/backend/internal/task/service/service_turns_step_stamp_test.go index 9df3402e85..999354c846 100644 --- a/apps/backend/internal/task/service/service_turns_step_stamp_test.go +++ b/apps/backend/internal/task/service/service_turns_step_stamp_test.go @@ -255,3 +255,26 @@ func TestCreateCompletedTurnCarriesSameStamp(t *testing.T) { t.Fatalf("synthetic completed turn stamp = %v, want %q", turn.Metadata[models.TurnMetaKeyWorkflowStepIDAtStart], "step-c") } } + +func TestCreateCompletedTurnMarksLifecycleOnly(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + setupStepStampTask(t, repo, "task-lifecycle-only", "step-d") + session := createStepStampSession(t, repo, "session-lifecycle-only", "task-lifecycle-only") + + turn, err := svc.createCompletedTurn(ctx, session) + if err != nil { + t.Fatalf("createCompletedTurn: %v", err) + } + if turn.Metadata[models.TurnMetaKeyLifecycleOnly] != true { + t.Fatalf("synthetic completed turn lifecycle_only = %v, want true", turn.Metadata[models.TurnMetaKeyLifecycleOnly]) + } + + stored, err := repo.GetTurn(ctx, turn.ID) + if err != nil { + t.Fatalf("GetTurn: %v", err) + } + if stored.Metadata[models.TurnMetaKeyLifecycleOnly] != true { + t.Fatalf("persisted completed turn lifecycle_only = %v, want true", stored.Metadata[models.TurnMetaKeyLifecycleOnly]) + } +} diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index a7117fcff5..6f73449d7d 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -1324,6 +1324,13 @@ export class ApiClient { * metadata dialog's `turn_metadata` field. `authorType` defaults to agent; * "user" seeds a prompt row whose prompt_index is computed server-side. * `createdAt` (RFC3339) pins the row's timestamp for deterministic ordering. + * + * `newTurn` creates a brand-new turn for this message instead of reusing + * the session's active turn, so a spec can hold a real open turn on the + * session while seeding a second, independently-timed turn alongside it + * (e.g. a lifecycle-shaped turn that must not shadow the open one). + * `turnStartedAt`/`turnCompletedAt` (RFC3339) set that new turn's + * timestamps; both are ignored unless `newTurn` is true. */ async seedSessionMessage( sessionId: string, @@ -1334,6 +1341,9 @@ export class ApiClient { turnMetadata?: Record; authorType?: "user" | "agent"; createdAt?: string; + newTurn?: boolean; + turnStartedAt?: string; + turnCompletedAt?: string; }, ): Promise { const body: Record = { session_id: sessionId, type: opts.type }; @@ -1342,6 +1352,9 @@ export class ApiClient { if (opts.turnMetadata !== undefined) body.turn_metadata = opts.turnMetadata; if (opts.authorType !== undefined) body.author_type = opts.authorType; if (opts.createdAt !== undefined) body.created_at = opts.createdAt; + if (opts.newTurn !== undefined) body.new_turn = opts.newTurn; + if (opts.turnStartedAt !== undefined) body.turn_started_at = opts.turnStartedAt; + if (opts.turnCompletedAt !== undefined) body.turn_completed_at = opts.turnCompletedAt; await this.request("POST", "/api/v1/_test/messages", body); } diff --git a/apps/web/e2e/tests/chat/clarification-lifecycle-turn-shadow.spec.ts b/apps/web/e2e/tests/chat/clarification-lifecycle-turn-shadow.spec.ts new file mode 100644 index 0000000000..a5a2a067d8 --- /dev/null +++ b/apps/web/e2e/tests/chat/clarification-lifecycle-turn-shadow.spec.ts @@ -0,0 +1,154 @@ +// Regression guard for docs/specs/current-turn-authority/spec.md: a +// near-simultaneous duplicate task_session_turns row (the synthetic +// "lifecycle" turn Service.createCompletedTurn writes on agent resume) must +// never outrank a session's real open turn when the UI resolves which turn +// is "current" — otherwise a pending clarification on the open turn is +// silently hidden from the chat overlay. Reproduces Scenarios 1 and 3 from +// the spec via the e2e test harness (real turn resolution end to end, not +// just the pure newestDurableTurnId unit test AC-10 already covers). +import { test, expect } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import type { SeedData } from "../../fixtures/test-base"; +import { watchWs, waitForHttp } from "../../helpers/causal-waits"; +import { SessionPage } from "../../pages/session-page"; + +/** Matches GET /api/v1/task-sessions/:id/turns, the REST snapshot that backs + * ensureSessionTurnsLoaded — a separate, fire-and-forget fetch from the WS + * message.list round trip (use-session-messages.ts fires it `void` before + * message.list is even awaited). The clarification overlay's turn-scoping + * depends on this fetch too, so a test must wait for both. */ +const TURNS_FETCH_PATH = /\/task-sessions\/[^/]+\/turns/; + +const QUESTION_PROMPT = "Which environment should I deploy to?"; +const STAGING_LABEL = "Staging"; +const PRODUCTION_LABEL = "Production"; + +/** + * Seeds a session with a real turn carrying a pending clarification, plus a + * second turn started 0.8s later that shadows it in "latest by started_at" + * ordering. `markedLifecycle` selects between AC-9's marked + * `lifecycle_only: true` shape (Scenario 1) and AC-6's unmarked legacy + * zero-duration shape (Scenario 3) — both must resolve to turn A. + * + * AC-9's marked variant also completes turn A (instead of leaving it open), + * forcing a D1 key-1 tie against turn B (also completed): with both turns + * completed, D1's remaining keys (started_at DESC) alone would hand + * authority to the later turn B, exactly as if R1's lifecycle_only exclusion + * did not exist. Only R1 filtering B out of contention lets turn A win, so + * this shape actually requires the exclusion mechanism — mirroring + * TestCurrentTurnAuthoritySurvivesLifecycleShadowOverPendingClarification's + * own precedent. AC-6's unmarked variant leaves turn A open, since that + * scenario proves D1's plain open-beats-completed rule, not R1. + */ +async function seedShadowedClarification( + apiClient: ApiClient, + seedData: SeedData, + title: string, + markedLifecycle: boolean, +): Promise<{ taskId: string; sessionId: string }> { + const task = await apiClient.createTask(seedData.workspaceId, title, { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + }); + const { session_id: sessionId } = await apiClient.seedTaskSession(task.id, { + state: "WAITING_FOR_INPUT", + }); + + const turnAStartedAt = new Date(Date.now() - 120_000).toISOString(); + const turnBStartedAt = new Date(Date.now() - 119_200).toISOString(); + + await apiClient.seedSessionMessage(sessionId, { + type: "clarification_request", + newTurn: true, + turnStartedAt: turnAStartedAt, + ...(markedLifecycle ? { turnCompletedAt: turnAStartedAt } : {}), + metadata: { + pending_id: "pend-shadow-1", + session_id: sessionId, + question_id: "q1", + question_index: 0, + question_total: 1, + status: "pending", + question: { + id: "q1", + title: "Deploy target", + prompt: QUESTION_PROMPT, + options: [ + { option_id: "opt-staging", label: STAGING_LABEL, description: "" }, + { option_id: "opt-prod", label: PRODUCTION_LABEL, description: "" }, + ], + }, + }, + }); + + await apiClient.seedSessionMessage(sessionId, { + type: "script_execution", + content: "Resuming session.", + newTurn: true, + turnStartedAt: turnBStartedAt, + turnCompletedAt: turnBStartedAt, + ...(markedLifecycle ? { turnMetadata: { lifecycle_only: true } } : {}), + }); + + return { taskId: task.id, sessionId }; +} + +test.describe("Duplicate lifecycle turn does not hide a pending clarification", () => { + test("a marked lifecycle turn tied with the clarification turn in D1 ordering does not shadow its clarification (AC-9)", async ({ + testPage, + apiClient, + seedData, + }) => { + const { taskId } = await seedShadowedClarification( + apiClient, + seedData, + "Lifecycle shadow - marked", + true, + ); + + const wsWatcher = watchWs(testPage); + const messagesLoaded = wsWatcher.waitForResponse("message.list"); + const turnsLoaded = waitForHttp(testPage, "GET", TURNS_FETCH_PATH); + await testPage.goto(`/t/${taskId}`); + await Promise.all([messagesLoaded, turnsLoaded]); + const session = new SessionPage(testPage); + await session.waitForLoad(); + + await expect(session.clarificationOverlay()).toBeVisible(); + await expect(session.clarificationOverlay()).toContainText(QUESTION_PROMPT); + + const staging = session.clarificationOption(STAGING_LABEL); + await expect(staging).toBeVisible(); + await staging.click(); + await expect(staging).toHaveAttribute("data-selected", "true"); + }); + + test("an unmarked legacy zero-duration turn over an open turn still resolves to the open turn (AC-6)", async ({ + testPage, + apiClient, + seedData, + }) => { + const { taskId } = await seedShadowedClarification( + apiClient, + seedData, + "Lifecycle shadow - unmarked legacy", + false, + ); + + const wsWatcher = watchWs(testPage); + const messagesLoaded = wsWatcher.waitForResponse("message.list"); + const turnsLoaded = waitForHttp(testPage, "GET", TURNS_FETCH_PATH); + await testPage.goto(`/t/${taskId}`); + await Promise.all([messagesLoaded, turnsLoaded]); + const session = new SessionPage(testPage); + await session.waitForLoad(); + + await expect(session.clarificationOverlay()).toBeVisible(); + await expect(session.clarificationOverlay()).toContainText(QUESTION_PROMPT); + + const production = session.clarificationOption(PRODUCTION_LABEL); + await expect(production).toBeVisible(); + await production.click(); + await expect(production).toHaveAttribute("data-selected", "true"); + }); +}); diff --git a/apps/web/lib/utils/pending-clarification-turn-authority-fixture.test.ts b/apps/web/lib/utils/pending-clarification-turn-authority-fixture.test.ts new file mode 100644 index 0000000000..57a0e36fc3 --- /dev/null +++ b/apps/web/lib/utils/pending-clarification-turn-authority-fixture.test.ts @@ -0,0 +1,83 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; +import { sessionId as toSessionId, taskId as toTaskId, type Turn } from "@/lib/types/http"; +import { newestDurableTurnId } from "./pending-clarification"; + +// AC-10: this file and TestCurrentTurnResolutionFixture +// (apps/backend/internal/task/repository/sqlite/turn_authority_fixture_test.go) +// run the identical cases from one shared JSON fixture through the Go and +// TypeScript current-turn resolutions. Neither test may filter the shared +// cases; each may add its own. The path is resolved from this file via +// import.meta.url, not the process working directory, which under Vitest is +// apps/web rather than this file's directory. Building the path in two steps +// (dirname, then join) rather than `new URL(relative, import.meta.url)` +// avoids Vite's static asset-URL transform, which rewrites a +// literal-relative-plus-import.meta.url expression pointing outside the +// project root into an http://.../@fs/... dev-server URL instead of a real +// file: URL. +const FIXTURE_PATH = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "../../../backend/internal/task/repository/sqlite/testdata/current_turn_resolution.json", +); + +type FixtureTurn = { + id: string; + started_at: string; + created_at: string; + completed_at: string | null; + metadata: Record; +}; + +type FixtureCase = { + name: string; + turns: FixtureTurn[]; + expected_current_turn_id: string | null; +}; + +type Fixture = { + cases: FixtureCase[]; +}; + +// loadFixture SHALL fail the test (not skip) when the fixture is missing or +// unparseable - a fixture that silently stops being read is +// indistinguishable from two implementations that agree. +function loadFixture(): Fixture { + const raw = readFileSync(FIXTURE_PATH, "utf8"); + const parsed = JSON.parse(raw) as Fixture; + if (!parsed.cases?.length) { + throw new Error(`${path.basename(FIXTURE_PATH)} has no cases`); + } + return parsed; +} + +// completed_at: null in the fixture maps to an absent property, matching the +// wire contract (Turn.completed_at?: string); the production payload type is +// not widened to admit null. +function toTurn(fixtureTurn: FixtureTurn): Turn { + const turn: Turn = { + id: fixtureTurn.id, + session_id: toSessionId("fixture-session"), + task_id: toTaskId("fixture-task"), + started_at: fixtureTurn.started_at, + created_at: fixtureTurn.created_at, + updated_at: fixtureTurn.created_at, + metadata: fixtureTurn.metadata, + }; + if (fixtureTurn.completed_at !== null) { + turn.completed_at = fixtureTurn.completed_at; + } + return turn; +} + +const fixture = loadFixture(); + +describe("newestDurableTurnId AC-10: shared fixture agreement with the Go resolution", () => { + for (const testCase of fixture.cases) { + it(testCase.name, () => { + const turns = testCase.turns.map(toTurn); + expect(newestDurableTurnId(turns)).toBe(testCase.expected_current_turn_id); + }); + } +}); diff --git a/apps/web/lib/utils/pending-clarification-turn-authority.test.ts b/apps/web/lib/utils/pending-clarification-turn-authority.test.ts new file mode 100644 index 0000000000..ce4b7924e2 --- /dev/null +++ b/apps/web/lib/utils/pending-clarification-turn-authority.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, it } from "vitest"; +import { sessionId as toSessionId, taskId as toTaskId, type Turn } from "@/lib/types/http"; +import { newestDurableTurnId } from "./pending-clarification"; + +// AC-9: newestDurableTurnId must apply the same two rules the backend's +// currentTurnAuthority applies before its started_at/created_at/id +// comparison - skip lifecycle turns, prefer an open turn over a completed +// one - reading the marker from the turn payload rather than inferring +// anything from timestamps. + +const TIMESTAMP_11 = "2026-08-14T11:00:00Z"; +const TIMESTAMP_13 = "2026-08-14T13:00:00Z"; +const NEWER_TURN_ID = "turn-newer"; + +function turn( + id: string, + startedAt: string, + createdAt = startedAt, + overrides: Partial = {}, +): Turn { + return { + id, + session_id: toSessionId("session-1"), + task_id: toTaskId("task-1"), + started_at: startedAt, + created_at: createdAt, + updated_at: createdAt, + ...overrides, + }; +} + +describe("newestDurableTurnId AC-9: lifecycle exclusion and open-turn precedence", () => { + it("skips a lifecycle-only turn even when it is the newest by timestamp", () => { + expect( + newestDurableTurnId([ + turn("turn-real", "2026-08-14T12:00:00Z"), + turn("turn-lifecycle", TIMESTAMP_13, undefined, { metadata: { lifecycle_only: true } }), + ]), + ).toBe("turn-real"); + }); + + it("resolves to null when every turn is lifecycle-only", () => { + expect( + newestDurableTurnId([ + turn("turn-lifecycle", "2026-08-14T12:00:00Z", undefined, { + metadata: { lifecycle_only: true }, + }), + ]), + ).toBeNull(); + }); + + const lifecycleEncodings = [true, 1, "true", "1"]; + it.each(lifecycleEncodings)("treats metadata.lifecycle_only = %j as lifecycle", (value) => { + expect( + newestDurableTurnId([ + turn("turn-real", "2026-08-14T12:00:00Z"), + turn("turn-lifecycle", TIMESTAMP_13, undefined, { metadata: { lifecycle_only: value } }), + ]), + ).toBe("turn-real"); + }); + + const conversationalEncodings = [null, undefined, false, 0, "", "false", "0", "yes", {}, []]; + it.each(conversationalEncodings)( + "treats metadata.lifecycle_only = %j as conversational, not lifecycle", + (value) => { + expect( + newestDurableTurnId([ + turn("turn-older", TIMESTAMP_11), + turn(NEWER_TURN_ID, TIMESTAMP_13, undefined, { metadata: { lifecycle_only: value } }), + ]), + ).toBe(NEWER_TURN_ID); + }, + ); + + it("treats an absent metadata.lifecycle_only key as conversational", () => { + expect( + newestDurableTurnId([turn("turn-older", TIMESTAMP_11), turn(NEWER_TURN_ID, TIMESTAMP_13)]), + ).toBe(NEWER_TURN_ID); + }); + + it("prefers an open turn over a later completed turn", () => { + expect( + newestDurableTurnId([ + turn("turn-open", TIMESTAMP_11), + turn("turn-completed-later", TIMESTAMP_13, undefined, { + completed_at: "2026-08-14T13:05:00Z", + }), + ]), + ).toBe("turn-open"); + }); + + it("treats an explicit null completed_at the same as an absent key", () => { + expect( + newestDurableTurnId([ + turn("turn-open", TIMESTAMP_11, undefined, { completed_at: null as unknown as string }), + turn("turn-completed-later", TIMESTAMP_13, undefined, { + completed_at: "2026-08-14T13:05:00Z", + }), + ]), + ).toBe("turn-open"); + }); +}); diff --git a/apps/web/lib/utils/pending-clarification.ts b/apps/web/lib/utils/pending-clarification.ts index f6be4cd76c..066992c9c2 100644 --- a/apps/web/lib/utils/pending-clarification.ts +++ b/apps/web/lib/utils/pending-clarification.ts @@ -30,29 +30,58 @@ function durableTurnTimestampKey(value: string): string { return `${match[1]}.${(match[2] ?? "").padEnd(9, "0")}Z`; } +// A turn counts as lifecycle-only for exactly these four encodings, mirroring +// the backend's turnMetadataFlagPredicate (D12): boolean true, number 1, +// string "true", string "1". Everything else - an absent key, null, +// undefined, false, 0, "", "false", "0", "yes", an object, an array - is +// conversational. Deliberately a value-set membership check, not a +// truthiness test (misreads "false"/"0"/"yes"/{}/[] as lifecycle) or a +// String() coercion (misreads ["1"]). +const LIFECYCLE_ONLY_VALUES: ReadonlySet = new Set([true, 1, "true", "1"]); + +function isLifecycleOnlyTurn(turn: Turn): boolean { + return LIFECYCLE_ONLY_VALUES.has(turn.metadata?.lifecycle_only); +} + +// completed_at is optional on the wire, so an open turn arrives as an absent +// key or explicit null; == null treats both alike. +function isOpenTurn(turn: Turn): boolean { + return turn.completed_at == null; +} + +// isNewerTurn mirrors D1's total order: an open turn always outranks a +// completed one regardless of timestamps, then started_at, created_at, id - +// all descending. +function isNewerTurn(candidate: Turn, current: Turn): boolean { + const candidateOpen = isOpenTurn(candidate); + if (candidateOpen !== isOpenTurn(current)) return candidateOpen; + const candidateKey = [ + durableTurnTimestampKey(candidate.started_at), + durableTurnTimestampKey(candidate.created_at), + // Mirrors the backend's final deterministic tie-break. Turn IDs do not + // encode creation time; exact timestamp ties have no finer ordering. + candidate.id, + ]; + const currentKey = [ + durableTurnTimestampKey(current.started_at), + durableTurnTimestampKey(current.created_at), + current.id, + ]; + for (let part = 0; part < candidateKey.length; part++) { + if (candidateKey[part] === currentKey[part]) continue; + return candidateKey[part] > currentKey[part]; + } + return false; +} + export function newestDurableTurnId(turns?: readonly Turn[]): string | null | undefined { if (turns === undefined) return undefined; - if (turns.length === 0) return null; - let newest = turns[0]; - for (let index = 1; index < turns.length; index++) { - const candidate = turns[index]; - const newestKey = [ - durableTurnTimestampKey(newest.started_at), - durableTurnTimestampKey(newest.created_at), - newest.id, - ]; - const candidateKey = [ - durableTurnTimestampKey(candidate.started_at), - durableTurnTimestampKey(candidate.created_at), - // Mirrors the backend's final deterministic tie-break. Turn IDs do not - // encode creation time; exact timestamp ties have no finer ordering. - candidate.id, - ]; - for (let part = 0; part < candidateKey.length; part++) { - if (candidateKey[part] === newestKey[part]) continue; - if (candidateKey[part] > newestKey[part]) newest = candidate; - break; - } + const eligible = turns.filter((candidate) => !isLifecycleOnlyTurn(candidate)); + if (eligible.length === 0) return null; + let newest = eligible[0]; + for (let index = 1; index < eligible.length; index++) { + const candidate = eligible[index]; + if (isNewerTurn(candidate, newest)) newest = candidate; } return newest.id; } diff --git a/docs/decisions/2026-08-14-current-turn-clarification-ownership.md b/docs/decisions/2026-08-14-current-turn-clarification-ownership.md index 26759fa913..e1aac40e56 100644 --- a/docs/decisions/2026-08-14-current-turn-clarification-ownership.md +++ b/docs/decisions/2026-08-14-current-turn-clarification-ownership.md @@ -1,6 +1,6 @@ # ADR-2026-08-14-current-turn-clarification-ownership: Current Turn Owns Active Clarification -**Status:** accepted (amended 2026-08-15) +**Status:** accepted (amended 2026-08-15, 2026-08-24) **Date:** 2026-08-14 **Area:** backend, frontend, protocol, workflow @@ -27,8 +27,17 @@ and hides the question. The session's current turn owns active clarification state. - A pending clarification is operational only when its message belongs to the session's newest durable - `task_session_turns` record. Missing status retains the existing legacy meaning of pending only after - current-turn ownership matches. + **conversational** `task_session_turns` record — a turn whose `metadata.lifecycle_only` flag is not + set. "Set" means exactly the value-set `{true, 1, "true", "1"}`; every other encoding — an absent + key, `null`, `false`, `0`, `""`, `"false"`, `"0"`, any other string, an object, or an array — is + conversational, matching `turnMetadataFlagPredicate`'s `IN ('true','1')` test on both dialects. That + marker identifies a synthetic turn born already-completed by lifecycle bookkeeping (for example the + agent-boot-on-resume record), not a turn representing genuine conversation; a lifecycle turn is + excluded from current-turn resolution entirely, however recent. Among conversational turns, + the newest is the one with no open successor: an open turn (`completed_at IS NULL`) always outranks + a completed one regardless of timestamps, and only when both candidates are open or both are + completed do `started_at`, then `created_at`, then `id` (all descending) break the tie. Missing + status retains the existing legacy meaning of pending only after current-turn ownership matches. - Deleting messages never moves current-turn ownership backward or reactivates a superseded bundle. - Detachment preserves late-answer behavior only while that turn remains current. Once a newer turn is accepted, older pending rows become inert history without requiring a destructive rewrite. @@ -74,8 +83,8 @@ This ownership rule stands independently if ADR 0015 is later rejected, supersed session. - Pending projection performs bounded database reads on pending-sensitive events and normal list/boot hydration. It avoids unbounded transcript delivery or inactive-session subscriptions. -- SQLite and PostgreSQL must select the same newest durable turn; dialect-sensitive repository tests - are required. +- SQLite and PostgreSQL must select the same newest durable conversational turn; dialect-sensitive + repository tests are required. ## Alternatives considered diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index e7683ef093..67f06d04fc 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -177,7 +177,7 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-12-empty-utility-bindings-inherit-default | [Empty Built-in Utility Bindings Inherit the Default](2026-08-12-empty-utility-bindings-inherit-default.md) | accepted | backend, frontend | 2026-08-12 | | 2026-08-12-setup-timeout-owns-launch-budget | [One Setup Timeout Owns Launch Budgets](2026-08-12-setup-timeout-owns-launch-budget.md) | accepted | backend | 2026-08-12 | | 2026-08-10-no-em-dash-public-copy | [Keep Em Dashes Out of Public Copy](2026-08-10-no-em-dash-public-copy.md) | accepted | frontend, infra | 2026-08-10 | -| 2026-08-14-current-turn-clarification-ownership | [Current Turn Owns Active Clarification](2026-08-14-current-turn-clarification-ownership.md) | accepted (amended 2026-08-15) | backend, frontend, protocol, workflow | 2026-08-14 | +| 2026-08-14-current-turn-clarification-ownership | [Current Turn Owns Active Clarification](2026-08-14-current-turn-clarification-ownership.md) | accepted (amended 2026-08-15, 2026-08-24) | backend, frontend, protocol, workflow | 2026-08-14 | | 2026-08-12-task-bound-fork-destinations | [Bind Fork Push Destinations to Tasks](2026-08-12-task-bound-fork-destinations.md) | accepted (amended 2026-08-13) | backend, frontend, workflow, security, GitHub | 2026-08-12 | | 2026-08-13-hard-delete-task-contribution-links | [Hard delete owns task contribution links](2026-08-13-hard-delete-task-contribution-links.md) | accepted | backend | 2026-08-13 | | 2026-08-15-office-mode-follows-active-workspace | [Office Mode Follows the Active Workspace](2026-08-15-office-mode-follows-active-workspace.md) | accepted | frontend, backend | 2026-08-15 | diff --git a/docs/specs/current-turn-authority/spec.md b/docs/specs/current-turn-authority/spec.md new file mode 100644 index 0000000000..3eda36bcd5 --- /dev/null +++ b/docs/specs/current-turn-authority/spec.md @@ -0,0 +1,508 @@ +# Current-turn authority: a lifecycle turn must never shadow the real turn + +**Status:** spec +**Area:** backend (task repository, task service, agent lifecycle), frontend (chat overlay), protocol +**Slug:** `current-turn-authority` +**Amends:** [ADR-2026-08-14 Current Turn Owns Active Clarification](../../decisions/2026-08-14-current-turn-clarification-ownership.md) + +--- + +## Why + +A pending `ask_user_question_kandev` clarification became invisible to every surface that exists to +show it: `list_pending_questions_kandev` omitted it, the task-list badge cleared, the chat overlay +stopped rendering it, and the Stall Session Watchdog could not tell the session from one with no +open turn. Task `4be500f5` sat on an unanswered architectural question for 70+ minutes, found only +because a human read the raw transcript. The cause is not a race and not data corruption, but a +**stated invariant enforced in exactly one of eleven places**. + +`Service.createCompletedTurn` (`apps/backend/internal/task/service/service_turns.go:274`) documents +its own contract: + +> persists a synthetic turn that is **never observable as active**. It is used for lifecycle +> messages that must belong to a turn without making an idle task appear to have an active agent +> turn. + +That turn is created on every agent **resume** launch: `bootMsgAdapter.CreateMessage` +(`apps/backend/internal/backendapp/worktree.go:57`) passes `CompletedTurn: isResuming` for the +`agent_boot` `script_execution` message. It is a real `task_session_turns` row with +`started_at = completed_at = created_at = now`, ordinary metadata, and one message. + +Only `GetActiveTurnBySessionID` honours the invariant, by filtering `completed_at IS NULL`. Every +other consumer resolves "the session's current turn" as + +```sql +ORDER BY turn_row.started_at DESC, turn_row.created_at DESC, turn_row.id DESC LIMIT 1 +``` + +gated by `turnAuthorityPredicate`. The synthetic turn passes that predicate on its **first** +disjunct alone (`NOT (prompt_dispatch_pending)` is true; it never needs the message-EXISTS +fallback), and its `started_at` is later. So it wins, and every clarification owned by the real, +still-open turn is reclassified as superseded history. + +### Evidence + +In session `adddd3fc` the real turn `6d91a053` opened at 01:40:08.937 and the resuming boot script +wrote a synthetic turn 0.8s later, which became "current". The `clarification_request` raised on +`6d91a053` at 02:44 stayed invisible on every surface until 04:08, when that turn completed and the +question became legitimately superseded history. It repeated minutes later: the ordinary resume path, +not a one-off. + +Across 6,133 production turns, 775 have `completed_at = started_at` — but **158 carry more than one +message** (real turns buried by `AbandonTurn`) and 27 hold `clarification_request` rows, which is +why D3 forbids inferring "synthetic" from that shape. Shadowing has occurred 207 times across 116 +sessions, leaving 7 pending clarifications across 5 sessions unreachable today. + +--- + +## Prior art + +### Our own prior reasoning + +**The in-repo prior reasoning already answered the design fork.** +[ADR-2026-08-14](../../decisions/2026-08-14-current-turn-clarification-ownership.md) (accepted, +amended 2026-08-15) decided: + +> A pending clarification is operational only when its message belongs to the session's **newest +> durable `task_session_turns` record**. + +It further requires that **one repository derivation** supply the clarification guard, detach/expiry +fallback, response validation, per-session pending projection and task-summary reconciliation, and +that both dialects select the same turn under dialect-sensitive tests (D9). It explicitly +**rejected** deriving ownership from the latest surviving message, since deleting a newer turn's +last message would roll the boundary backward and reactivate older pending history. This spec does +not reopen that: turn identity stays the boundary. + +**Where this spec departs.** "Newest durable record" assumes every durable turn represents +conversational work. `createCompletedTurn` predates the ADR and creates durable records representing +no work at all, whose own doc comment says they must never be treated as active. The two intents +agree; only the word "durable" fails to separate them. This spec narrows it to "newest durable +**conversational** record" (R1) and adds one ordering rule (R2) — which **requires amending the +ADR** (AC-12), not a silent divergence. + +Its "one repository derivation" requirement is also only **partly** honoured today: the ten call +sites share `turnAuthorityPredicate` but hand-write their own ordering, and the frontend hand-writes +a third copy. AC-3 and R3 close that. + +Also read and honoured, both deferring to the same upstream rule rather than restating it: +`external-question-answering/spec.md` (D4, D4a, L2a) and +`tasks/system-design/clarification-active-lifecycle.md`, both under `docs/specs/`. + +### What other products shipped + +Queried the `saas-kb` corpus for agent-question surfacing and session-resume semantics: every hit +(Warp, OpenHands) was end-user documentation that the feature exists, none documenting turn identity +or resume semantics — the layer this defect lives in. Nothing there makes a *durable, queryable* +"which question is still answerable" the contract, as Kandev already does. + +--- + +## Terminology + +- **Turn record** — a row in `task_session_turns`. +- **Lifecycle turn** — a turn created solely to give a lifecycle message a parent, by + `Service.createCompletedTurn`. It represents no agent work and is born already completed. +- **Conversational turn** — any turn record that is not a lifecycle turn. +- **Current turn** — the single conversational turn owning a session's active state, as resolved by + R1 + R2 below. May be absent. +- **Authority-eligible** — a turn record that passes `turnAuthorityPredicate` (the existing + reservation filter) **and** is conversational. +- **Shadowing** — a turn that is not the intended current turn being resolved as the current turn. + +--- + +## Data model + +**No schema change. No migration. No backfill.** + +One new turn-metadata key, written at insert time: + +| key | value | written by | +| --- | --- | --- | +| `lifecycle_only` | JSON `true` | `Service.createCompletedTurn`, on every turn it creates | + +Declared as `models.TurnMetaKeyLifecycleOnly = "lifecycle_only"` beside the existing `TurnMetaKey*` +constants in `internal/task/models/models.go`, and read with `turnMetadataFlagPredicate` so both +dialects normalise it identically (D12), exactly as `prompt_dispatch_pending` already does. +`models.Turn` already serialises `metadata` and the TypeScript `Turn` type already declares +`metadata?: Record`, so the marker reaches the frontend with no DTO change. + +--- + +## Determinism and boundary rules + +- **D1. The current-turn resolution is a total order over authority-eligible turn records**, keyed + in this exact sequence, all named columns: + 1. `CASE WHEN completed_at IS NULL THEN 0 ELSE 1 END` **ascending** (open before completed) + 2. `started_at` **descending** + 3. `created_at` **descending** + 4. `id` **descending** + + Key 4 is a primary key, so the order is total and no tie is broken by chance. Keys 2–4 are already + in the code; key 1 is the only addition, and it is what stops a zero-duration completed turn + outranking a genuinely open turn. + +- **D2. Key 1 is safe only under the single-open-turn invariant**, namely: a session has at most one + turn with `completed_at IS NULL`, and no open turn survives a session resume. `AbandonOpenTurns` + already enforces the second half on the resume path, and the first half holds across all 6,133 + production turn records (zero sessions with more than one open turn). Key 1 changes + the resolved turn **only** when the newest record is completed while an older record is still + open — which under this invariant can only be the shadowing defect itself. It is neither assumed + silently nor claimed as a guarantee: AC-8 requires the enforced half tested and the observed half + to have a defined, tested resolution when violated. + +- **D3. "Lifecycle" is a durable marker, never an inference from `completed_at = started_at`.** + That equality is produced by two unrelated, both-legitimate paths: `createCompletedTurn` (a + lifecycle turn) and `Repository.AbandonTurn` (a *real* turn buried after a crash, so its dead hours + do not poison analytics). They are indistinguishable by shape: 158 zero-duration turns carry + multi-message real conversations and 27 carry `clarification_request` rows, which any rule keyed on + zero duration would silence permanently. A hard prohibition, not a preference. + +- **D4. An absent `lifecycle_only` key means conversational.** Every turn record written before this + change is therefore conversational, including the 775 existing zero-duration rows. Deliberate; see + *Known residual*. + +- **D5. A lifecycle turn never holds a `clarification_request`.** `createCompletedTurn` is reachable + only from the boot-message path; clarification rows are written through `getOrStartTurn`. AC-7 + asserts this so R1's exclusion can never hide a question. + +- **D6. Resolution is a pure function of committed rows**, taking no new lock and adding no write. + Two concurrent readers of the same committed state resolve the same turn. A reader concurrent with + an insert resolves against whichever snapshot its transaction sees; because lifecycle turns never + compete for the current-turn slot, the resolved turn is the same whether or not a concurrent + `createCompletedTurn` has committed. That independence is the point of the fix. + +- **D7. Marking is idempotent by construction.** `createCompletedTurn` writes the marker in the same + `INSERT` as the row. A retried boot-message creation produces another marked lifecycle turn; N + marked lifecycle turns are as excluded as one, so retry needs no dedupe. + +- **D8. Absent current turn is a legitimate result, not an error.** A session with zero + authority-eligible turns resolves to no current turn. Every consumer already handles a `LIMIT 1` + subquery returning no row (the `turn_id = (…)` join matches nothing), unchanged. A session whose + only records are lifecycle turns now resolves to no current turn where it previously resolved to + the lifecycle turn — correct, since it has had no conversational turn and holds no active state. + +- **D9. SQLite and PostgreSQL must resolve identically**, including the `CASE WHEN completed_at IS + NULL` key and the metadata-flag normalisation. Restated from the ADR because key 1 is new and + untested in both dialects. + +- **D10. "Current turn" and "active turn" are different questions and must not be merged.** + `GetActiveTurnBySessionID` answers *"is a turn running right now"* and filters + `completed_at IS NULL`; the other nine sites answer *"which turn owns this session's state"* and + do not. Adopting AC-3's shared helper SHALL NOT remove that filter, or the function begins + returning completed turns and `AbandonOpenTurns` burns its 16-iteration budget re-burying them. + Because a lifecycle turn is always born completed, R1's exclusion is a **no-op** here, applied for + uniformity, not effect. + +- **D11. Both fragments must compose into the four SQL shapes already in the code**, changing + nothing else at any site: the scalar subquery (`clarificationBundleQuery` plus the six in + `message_clarification_response.go`), the `current_turn` CTE + (`FindActiveClarificationMessagesBySessionID`), the `ROW_NUMBER() OVER (PARTITION BY + task_session_id ORDER BY …)` window (`pendingActionsBySessionQuery`), and the top-level `ORDER BY + … LIMIT 1` (`GetActiveTurnBySessionID`, which keeps its own `completed_at IS NULL` per D10). + `predicate` joins each site's existing `WHERE`; `orderBy` replaces each site's ordering. Hence + `orderBy` is a bare expression list, not a canned subquery, which would fit only the first shape. + The window site resolves many sessions in one statement; a helper forcing it back to N scalar + subqueries is a performance regression on the task list and SHALL NOT be accepted. + Qualifying `orderBy` with `turnAlias` is safe at the window site, whose current text is + unqualified, because the alias is in scope. + +- **D12. A NULL or empty `metadata` column normalises to conversational.** The column is + `TEXT DEFAULT '{}'` and `turnMetadataFlagPredicate` already wraps its extraction in + `COALESCE(…, '')` before the `IN ('true','1')` test, so `NULL`, `''`, `'{}'` and malformed JSON + all yield "not a lifecycle turn" — the same result D4 requires for an absent key, via the same + existing helper. No new normalisation is introduced and none SHALL be added. + +- **D13. Clock skew and future timestamps change nothing.** D1 keys 2 and 3 are compared as stored, + unclamped: a turn whose `started_at` is in the future sorts first within its open/completed class, + exactly as today, and key 4 keeps the order total regardless. This spec deliberately adds no clock + validation — the defect is not a timestamp defect, and `started_at` ordering is already + load-bearing in `turnHistoryPredicate`, the turns endpoint, and the frontend comparison. + +- **D14. Bundle claim, winner/loser, and conflict semantics are unchanged.** This spec changes only + *which turn* the claim predicate in `claimActiveClarificationBundle` compares against. Exactly one + caller still claims a bundle, a loser still receives conflict with no write and no resume, and the + restore-on-failed-delivery path is untouched. Those rules stay owned by + `docs/specs/external-question-answering/spec.md` and + [ADR-2026-08-14](../../decisions/2026-08-14-current-turn-clarification-ownership.md), named here + so their absence below reads as deferral, not silence. + +- **D15. The frontend rule is a deliberate subset, sound only via the history pre-filter.** The + backend resolves `turnAuthorityPredicate AND NOT lifecycle`; `newestDurableTurnId` implements D1's + ordering and D4's marker and **no part of** the authority predicate. That holds because every turn + it sees arrives from the turns endpoint, already gated by `turnHistoryPredicate`. The predicates + differ: authority also admits a dispatch-pending, dispatch-attempted, message-less turn, which + history excludes, so such a turn can be current on the backend and never reach the frontend. That + gap is accepted, not accidental — such a turn has no message and so no clarification to hide — and + it is why AC-10's fixture is confined to authority-eligible turns: a fixture fed straight to both + implementations bypasses the pre-filter the frontend relies on, and would assert an agreement + neither implementation claims. + +--- + +## What + +Each criterion is observable through the database, HTTP API, MCP tool surface, or web UI. + +### R1 — a lifecycle turn is never current-turn authority + +- **AC-1.** When `Service.createCompletedTurn` persists a turn, the system SHALL write + `metadata.lifecycle_only = true` in the same insert as the turn row. + +- **AC-2.** The system SHALL exclude every turn whose `lifecycle_only` flag normalises to true from + current-turn resolution, at **every** site resolving a session's current turn. Those sites are + exactly: + + | file | site | + | --- | --- | + | `task/repository/sqlite/session.go` | `GetActiveTurnBySessionID` | + | `task/repository/sqlite/message.go` | `FindActiveClarificationMessagesBySessionID`; `pendingActionsBySessionQuery` | + | `task/repository/sqlite/clarification_bundle_query.go` | `clarificationBundleQuery` | + | `task/repository/sqlite/message_clarification_response.go` | `DetachActiveClarificationMessagesBySessionID`; `ExpireActiveClarificationBundle`; `loadRestorableClarificationBundle`; `restoreClarificationMessages`; `claimActiveClarificationBundle`; `loadClaimedClarificationBundle` | + + A site added later that resolves a current turn without the shared builder is a defect against + AC-3, not a gap in this list. + +- **AC-3.** The system SHALL express D1's ordering and R1's exclusion in **one** named Go helper in + `internal/task/repository/sqlite/turn_authority.go`: + + ```go + func currentTurnAuthority(driverName, turnAlias string) (predicate, orderBy string) + ``` + + Two fragments: the rule occupies two SQL positions and no one string sits in both. + `predicate` is `turnAuthorityPredicate` AND R1's exclusion (`NOT` the `lifecycle_only` flag, via + `turnMetadataFlagPredicate`) as one parenthesised conjunct, for `WHERE … AND %s`. `orderBy` is + D1's four keys as a bare expression list qualified by `turnAlias`, with no `ORDER BY` keyword and + no `LIMIT`, so it drops unchanged into both `ORDER BY %s` and `ROW_NUMBER() OVER (… ORDER BY %s)`. + Every AC-2 site SHALL take both from this one call and hand-write neither. + `turnAuthorityPredicate` is today called at exactly those ten sites, so subsuming it affects no + other consumer. This is the concrete form of the ADR's "one repository derivation" requirement. + +- **AC-4.** While a session's real turn is open and a lifecycle turn exists with a later + `started_at`, the system SHALL resolve the current turn to the real turn, asserted by **a Go + integration test over the service and repository layers**; all three consumers below are Go- + reachable, so no browser-level test is required. Reproducing the sequence in *Why*: after the + lifecycle turn is written, `list_pending_questions_kandev` SHALL return the bundle (through the + MCP handler in `internal/mcp/server/question_handlers.go`), the session's `pending_action` SHALL + be `clarification` (through `GetPendingActionsBySessionIDs`), and `POST + /api/v1/clarification/:pendingId/respond` SHALL succeed rather than return conflict (through + `internal/clarification`). + + The lifecycle turn SHALL come from the real write path — `Service.CreateMessage` with + `CompletedTurn: true` — never an inserted row imitating it, so the test fails if AC-1's marker is + dropped. Ordering SHALL be made deterministic by seeding the conversational turn with an explicit + **past** `started_at` (the existing `seedBundleTurnAt` idiom), since `createCompletedTurn` + stamps `time.Now().UTC()` and is therefore strictly later on every run. Starting both turns at + "now" and relying on them to differ SHALL NOT be accepted: on a coarse clock they tie on + `started_at` and the assertion turns on `id`, which is unrelated to the behaviour tested. + +- **AC-7.** On the boot-on-resume path, every `clarification_request` written by the open + conversational turn SHALL keep **that** turn's `turn_id`, asserted as a repository-level test over + a fixture exercising the path. The broader claim — that no `clarification_request` anywhere can + carry a lifecycle `turn_id` — holds only because `CreateMessageRequest.CompletedTurn` is tagged + `json:"-"` (no API client can set it) and has one production caller, which writes a + `script_execution` message. Neither property is type-enforced, so AC-7 SHALL also assert the + first: unmarshalling a body that sets `completed_turn` SHALL leave the field false. A caller later + setting it while writing a `clarification_request` voids the claim and is a defect here. + +### R2 — an open turn outranks a completed one + +- **AC-5.** The system SHALL order current-turn candidates by D1's four keys in that order, so a + turn with `completed_at IS NULL` is preferred over any completed turn regardless of `started_at`, + and ties within each class fall through to `started_at`, `created_at`, `id` descending. + +- **AC-6.** If a session has an open conversational turn and an unmarked legacy zero-duration turn + with a later `started_at`, then the system SHALL resolve the current turn to the open turn. This + rescues the pre-existing 775 unmarked rows in the only case where invisibility is actively + harmful; see *Known residual* for the case it does not rescue. + +- **AC-8.** The system SHALL assert the two halves of D2 separately, because only one is a + guarantee. That `AbandonOpenTurns` leaves no turn with `completed_at IS NULL` is enforced and + SHALL be tested directly. "At most one open turn per session" is **observed**, not enforced — no + unique or partial index constrains it — so the system SHALL instead assert the defined behaviour + when it does not hold: given two open turns on one session, resolution SHALL pick the one winning + D1 keys 2 through 4, deterministically and without error. A test asserting the invariant itself + could only check the fixture it just built, and would pass while production drifted. + +### R3 — the frontend consumes the same rule + +- **AC-9.** `newestDurableTurnId` in `apps/web/lib/utils/pending-clarification.ts` SHALL apply the + same two rules — skip lifecycle turns, prefer a turn with no `completed_at` — before its existing + `started_at` / `created_at` / `id` comparison, reading the marker from the turn payload rather + than inferring anything from timestamps. + + It SHALL treat a turn as lifecycle **only** when `metadata.lifecycle_only` is one of exactly four + values: boolean `true`, number `1`, string `"true"`, string `"1"`. Everything else — absent key, + `null`, `undefined`, `false`, `0`, `""`, `"false"`, `"0"`, `"yes"`, any object or array — is + conversational. A bare truthiness test SHALL NOT be used, nor a `String(value)` coercion: + truthiness misreads `"false"`, `"0"`, `"yes"`, `{}` and `[]` as lifecycle, coercion misreads + `["1"]`, and D12's backend predicate (`… IN ('true','1')`) treats none of them as lifecycle. A + divergence hides a clarification the backend is serving — the symptom this spec exists to + remove. AC-10's fixture SHALL pin these values. + + Its open-turn check SHALL be `turn.completed_at == null`, treating `null` and `undefined` alike: + the payload type declares `completed_at?: string`, so an open turn arrives as an absent key while + AC-10's fixture writes `null`. When every turn is a lifecycle turn it SHALL return `null`, not + `undefined` — `null` means history loaded with no durable turn and hides all messages, whereas + `undefined` means history not loaded and falls back to `pendingAction`, reopening the shadowing + this spec removes. + +- **AC-10.** The Go and TypeScript resolutions SHALL be validated against **one shared fixture + file**, consumed by both a Go repository test and a TypeScript unit test. Two implementations of + one rule are tolerable only while a single artifact proves they agree, so it is specified here. + + Path: `apps/backend/internal/task/repository/sqlite/testdata/current_turn_resolution.json`, under + the Go package because `go:embed` cannot reach outside its tree while the TypeScript test can + reach it by relative path. Format: a JSON object with a `cases` array, each case + `{ name, turns, expected_current_turn_id }` and each turn + `{ id, started_at, created_at, completed_at, metadata }`, timestamps RFC 3339 UTC and + `completed_at: null` when open. `expected_current_turn_id: null` means **no** current turn + resolves; the fixture SHALL carry at least one such case (D8, Scenario 5). + + Go SHALL read it with `os.ReadFile("testdata/current_turn_resolution.json")` — under `go test` the + working directory is the package directory — then seed each case and run the real query. + TypeScript SHALL resolve the path from the test file via `import.meta.url`, **not** the process + working directory, which under Vitest is `apps/web`. If the file is missing, unreadable or + unparseable both tests SHALL fail and neither may skip, because a fixture that silently stops + being read is indistinguishable from two implementations that agree. Both SHALL run every + case; either may add its own but SHALL NOT filter the shared ones. The cases SHALL cover every + value in AC-9's list: the four resolving as lifecycle, and ten conversational — AC-9 names eleven, + but an absent key and `undefined` are indistinguishable in JSON and SHALL be written once, as the + absent key. + + The fixture describes turns of **one** session and carries no `task_session_id` or `task_id`; both + are `NOT NULL` under a foreign key, so the Go test SHALL supply its own session and task and stamp + every turn with them. No fixture turn SHALL set a `prompt_dispatch_*` key, so every fixture turn is + authority-eligible by construction and the fixture exercises D1's ordering and D4's marker only — + the scope D15 requires. The TypeScript loader SHALL map `completed_at: null` to an absent property + on each `Turn` and `expected_current_turn_id: null` to `null`; the production payload type is not + widened to admit `null`. + +- **AC-11.** `GET /api/v1/task-sessions/:sessionId/turns` SHALL include `metadata.lifecycle_only` on + a lifecycle turn in its response payload, so AC-9 has the input it needs. `turnHistoryPredicate` + SHALL keep including lifecycle turns in visible history — the boot message stays grouped under its + own turn in the transcript, unchanged. + +### R4 — the decision record + +- **AC-12.** The system SHALL amend + `docs/decisions/2026-08-14-current-turn-clarification-ownership.md` so its stated rule reads + "newest durable **conversational** turn record", names the `lifecycle_only` marker, and records + D1's ordering including the open-turn key. It SHALL also correct that ADR's *Consequences* + section, which restates the rule as "SQLite and PostgreSQL must select the same newest durable + turn". The ADR is the only place the rule is written down as a decision, and leaving it saying + "newest durable record" would reintroduce this defect the next time someone implements against it. + +- **AC-13.** The system SHALL update + `docs/specs/tasks/system-design/clarification-active-lifecycle.md`, whose *Data model* section + states "The newest authoritative durable `task_session_turns` record for the session identifies the + current turn", whose *API surface* section orders visible history "matching the reverse ordering + used to select the current turn", and whose *Persistence guarantees* section calls clarification + state reconstructable from "the newest authoritative durable turn" — all three inaccurate under D1. + +--- + +## Failure modes + +- **A lifecycle turn is written without the marker** (a future second producer): treated as + conversational, can shadow again. AC-1 writes the marker inside `createCompletedTurn` rather than + at its call sites, so every caller inherits it. +- **A new consumer hand-writes its own current-turn subquery**: silent divergence. AC-3's shared + helper is the only thing that makes it visible in review. +- **Two open turns coexist**: D1 keys 2 to 4 choose between them, the same answer today's rule + gives. AC-8 requires that resolution tested, not assumed away. +- **An unmarked legacy lifecycle turn is newest and the real turn is also completed**: it stays + current and the older question stays invisible. Not fixed; see *Known residual*. +- **`metadata.lifecycle_only` present but outside `{true, 1, "true", "1"}`**: normalised to "not a + lifecycle turn" by `turnMetadataFlagPredicate` (D12) and AC-9's enumeration. No new rule. + +--- + +## Known residual + +**The 775 existing turn records carry no `lifecycle_only` marker and this spec adds no backfill.** +D3 explains why no migration can safely identify them: their only shared observable shape, +`completed_at = started_at`, is also the shape of 158 real conversations and 27 turns holding +`clarification_request` rows a backfill would silence permanently — converting a bounded visibility +bug into unbounded data loss. Consequently: + +- A legacy lifecycle turn **can no longer shadow an open turn** — R2/AC-6 covers that, the case + where a live question is hidden from a human who could still answer it. +- A legacy lifecycle turn **still shadows a completed turn**. Concretely, 4 of the 7 currently + unreachable pending clarifications (2026-08-10 to 2026-08-18) keep a legacy lifecycle turn as their + resolved current turn while their own turn is closed. Under + [ADR-2026-08-14](../../decisions/2026-08-14-current-turn-clarification-ownership.md) a question + whose turn closed with no successor is superseded history anyway, so their observable outcome + matches the intended contract even though the path to it is wrong. Named so their persistence is + not mistaken for an incomplete fix. +- The residual may shrink as eligible sessions expire; AC-1 prevents any new unmarked lifecycle turn. + +--- + +## Out of scope + +Named exclusions are contracts. Silence would be a defect. + +- **The Stall Session Watchdog automation** (`065b95a3-f69f-43dc-96e9-6fedbf6df021`). Its STEP 3 + hand-writes `ORDER BY started_at DESC LIMIT 1` with no authority predicate and no tiebreakers, + then tests `completed_at IS NULL`, so a lifecycle turn makes a genuinely stalled session look like + it has no open turn and the watchdog never nudges it. **A real second symptom of the same root + cause**, but the prompt lives in the `automations` table, not version control, so its owner + must update it — to D1's ordering, or at minimum "newest turn that is not `lifecycle_only`". +- **Task `35d108bb`'s turn-completion duplicate race.** That concerns + `acquireTurnCompletionCriticalSection` double-dispatching workflow `on_enter` actions: duplicate + **dispatches**, not duplicate turn rows. The extra row here is created deliberately, by name, on a + documented path. **Unrelated**; `35d108bb` landing would change nothing here. +- **Backfilling or deleting the 775 existing unmarked lifecycle turns.** Prohibited by D3, + quantified in *Known residual*. +- **Changing what `AbandonTurn` writes.** `completed_at = started_at` on a buried orphan is + deliberate analytics behaviour, untouched. +- **Removing the lifecycle turn concept**, e.g. a nullable `turn_id` on the boot message or + attaching it to the real turn. A larger change to the message/turn foreign key and to transcript + grouping, not needed to close this defect. +- **Replacing the frontend's independent resolution with a server-supplied `current_turn_id` field.** + A cleaner end state, but it changes the turns endpoint envelope. AC-9 + AC-10 close the drift risk + at a fraction of the blast radius. A follow-up card, not this one. +- **The `turnHistoryPredicate` visible-history rule.** Unchanged by AC-11. +- **Anything about permissions, authorization, or the visibility predicate** in + `clarificationBundleQuery`. Untouched. + +--- + +## Scenarios + +1. **Resume during an open turn, question asked afterwards.** Real turn opens; a marked lifecycle + turn is written 0.8s later; the agent asks on the real turn. The question stays listed, badged + and answerable throughout. (AC-1, AC-2, AC-4) +2. **Resume between turns.** Previous turn completes, a marked lifecycle turn is written, a new real + turn opens 3s later; current turn is the new real turn, so the ordinary path is unchanged. (AC-5) +3. **Legacy unmarked lifecycle turn over an open turn.** No marker, `completed_at = started_at`, + later than an open conversational turn; current turn is the open turn. (AC-6) +4. **Legacy unmarked lifecycle turn over a completed turn.** Same fixture, real turn closed; the + legacy turn stays current and the older bundle stays superseded, asserting the residual so it + cannot change silently. (*Known residual*) +5. **Session with only lifecycle turns.** No current turn resolves, nothing is listed, no consumer + errors. (D8) +6. **Both dialects.** Scenarios 1-5 give identical resolved turn ids on SQLite and PostgreSQL. (D9) +7. **Frontend agreement.** AC-10's fixture yields the same resolved turn id from the Go helper and + from `newestDurableTurnId`. (AC-9, AC-10) + +--- + +## Verification notes + +- The repository-level tests belong beside `clarification_bundle_query_test.go`, whose header + comment documents the old `(started_at, created_at, id)` ordering and must be updated for key 1. +- AC-4's integration test proves the backend-owned surfaces recover (MCP listing, `pending_action`, + the respond endpoint). The chat overlay is frontend-owned, proven by AC-9 and AC-10, not AC-4. +- `apps/backend/internal/orchestrator/clarification_guard.go` is the most severe consumer: a shadowed + clarification makes `sessionHasPendingClarification` return false, letting a workflow step + auto-advance past an unanswered question. Worth an explicit test. + +### User-visible surfaces touched + +The chat clarification overlay (`pending-clarification.ts`); the task-list pending badge via +`pending_action`; `POST /api/v1/clarification/:pendingId/respond` success vs. conflict; MCP +`list_pending_questions_kandev` and `answer_question_kandev`; workflow step auto-advance via the +clarification guard. No new UI copy, so no i18n catalogue work. diff --git a/docs/specs/tasks/system-design/clarification-active-lifecycle.md b/docs/specs/tasks/system-design/clarification-active-lifecycle.md index 6a2211ac13..f3ff054aba 100644 --- a/docs/specs/tasks/system-design/clarification-active-lifecycle.md +++ b/docs/specs/tasks/system-design/clarification-active-lifecycle.md @@ -34,8 +34,12 @@ No schema change. - Clarification questions remain `task_session_messages` rows with `type = "clarification_request"`. - Rows in one bundle share `metadata.pending_id`; terminal status remains in `metadata.status`. -- `task_session_messages.turn_id` associates a question with its turn. The newest authoritative durable - `task_session_turns` record for the session identifies the current turn; deleting messages does not +- `task_session_messages.turn_id` associates a question with its turn. The newest durable + **conversational** `task_session_turns` record for the session identifies the current turn — a + turn whose `metadata.lifecycle_only` flag is not set; a lifecycle turn (for example the + agent-boot-on-resume record) is never current-turn authority, however recent. Ranking among + conversational turns is: an open turn (`completed_at IS NULL`) before any completed turn regardless + of timestamps, then `started_at`, `created_at`, and `id`, all descending. Deleting messages does not delete that parent turn or move ownership backward. - A pre-acknowledgement successor carries `metadata.prompt_dispatch_pending=true` plus the source clarification turn, pending ID, and exact claimed message IDs. An empty row carrying only the pending @@ -66,11 +70,15 @@ No new route. Task-session responses add `pending_action_revision` beside the ex decimal epoch is a database-backed, monotonically allocated backend generation; sequence orders reads within that generation. Clients compare both fields and reject any older result, including an unseen pre-restart epoch delivered after client state has been rebuilt. -- `GET /api/v1/task-sessions/:sessionId/turns` continues to expose durable turn history; - unpublished reservations stay hidden until publication or durable message evidence, including while - an attempt marker makes them internal current-turn authority. Attempted reservations are preserved - across restart because their dispatch outcome is ambiguous. Visible history is ordered ascending by - `started_at`, `created_at`, then `id`, matching the reverse ordering used to select the current turn. +- `GET /api/v1/task-sessions/:sessionId/turns` continues to expose durable turn history, including + lifecycle turns and their `metadata.lifecycle_only` marker; unpublished reservations stay hidden + until publication or durable message evidence, including while an attempt marker makes them internal + current-turn authority. Attempted reservations are preserved across restart because their dispatch + outcome is ambiguous. Visible history is ordered ascending by `started_at`, `created_at`, then `id` + across every visible turn, lifecycle or conversational. That is the reverse of the tie-break keys + used to select the current turn, but not the full current-turn ordering: current-turn selection also + excludes lifecycle turns entirely and ranks an open turn ahead of any completed one regardless of + timestamp, neither of which visible history does. - Task list, workflow snapshot, and boot payloads continue to expose task-level `pending_action` in the status summary and legacy fallback fields. - `POST /api/v1/clarification/:pendingId/respond` uses one state-based contract: @@ -240,7 +248,7 @@ session they can already access. Session selection does not broaden task visibil - Message history remains durable and is not destructively rewritten merely because a newer turn exists. - Active clarification state is reconstructable after restart from message status plus the newest - authoritative durable turn. + durable conversational turn (a lifecycle turn is never authoritative, however recent). - Current-turn ownership is reconstructable from durable turn rows even when a turn has no remaining messages. - Unpublished detached-answer reservations carry enough recovery identity to restore only their own