From 5a3d1bf63a9bb1431a5c090d1afdfe28cc3f28da Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 1 Sep 2026 15:20:03 +0000 Subject: [PATCH] fix(office): stop review and approval from seating the same agent (#3239) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- .../office/engine_adapters/seat_caster.go | 154 ++++++-- .../engine_adapters/seat_caster_test.go | 337 +++++++++++++++++- .../internal/workflow/engine/adapters.go | 19 +- .../engine/ensure_participant_seat_test.go | 23 +- .../workflow/engine/phase2_callbacks.go | 2 +- .../requirements/review-participant-seats.md | 46 ++- .../review-participant-seats-01.md | 78 ++-- 7 files changed, 543 insertions(+), 116 deletions(-) diff --git a/apps/backend/internal/office/engine_adapters/seat_caster.go b/apps/backend/internal/office/engine_adapters/seat_caster.go index a19667fc8a..81983d70ea 100644 --- a/apps/backend/internal/office/engine_adapters/seat_caster.go +++ b/apps/backend/internal/office/engine_adapters/seat_caster.go @@ -8,30 +8,67 @@ import ( "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" ) // SeatCasterWorkflowRepo captures the workflow-repo subset the seat caster -// needs to resolve the task's runner (REQ-OFFICE-REVIEW-SEATS-002's "the -// task's runner" fallback and self-review comparison). The caller supplies -// the immutable step that the task entered, so this adapter does not re-read +// needs: resolving the task's runner (REQ-OFFICE-REVIEW-SEATS-002's "the +// task's runner" fallback and self-review comparison), and listing every +// participant seat already recorded for the task's current workflow, for +// the cross-step exclusion in castFromCandidates. The caller supplies the +// immutable step that the task entered, so this adapter does not re-read // mutable task state after the transition commits. type SeatCasterWorkflowRepo interface { ResolveCurrentRunner(ctx context.Context, stepID, taskID string) (string, error) + ListParticipantsForTaskWorkflow(ctx context.Context, taskID, workflowID string) ([]*wfmodels.WorkflowStepParticipant, error) +} + +// seatRolePools maps a participant role to the set of agent roles eligible +// for its seat, per AC-OFFICE-REVIEW-SEATS-002.1: approver draws from `ceo`; +// reviewer draws from `ceo` ∪ `specialist` — and only those two. Any other +// participant role (watcher, collaborator, ...) keeps today's ceo-only +// behavior; the AC does not cover them, and the pool must not be widened +// without an amended AC. +var seatRolePools = map[string]map[models.AgentRole]bool{ + string(wfmodels.ParticipantRoleReviewer): { + models.AgentRoleCEO: true, + models.AgentRoleSpecialist: true, + }, + string(wfmodels.ParticipantRoleApprover): { + models.AgentRoleCEO: true, + }, +} + +// eligibleAgentRolesFor returns role's agent-role allowlist. It is always a +// positive match, never a denylist: eligibleCandidates lists agents with no +// Role filter (a union cannot be expressed as a single filter value), so +// this allowlist is what keeps worker/assistant/empty-role agent profiles +// from ever being seated. +func eligibleAgentRolesFor(role string) map[models.AgentRole]bool { + if pool, ok := seatRolePools[role]; ok { + return pool + } + return map[models.AgentRole]bool{models.AgentRoleCEO: true} } // SeatCasterAdapter implements engine.ParticipantSeatCaster by applying the // casting resolution algorithm of REQ-OFFICE-REVIEW-SEATS-002 (system design // "Casting resolution"): // -// 1. List the task's workspace's Office agents whose role is `ceo` and whose -// status is neither `stopped` nor `pending_approval`, ordered by -// `created_at` then agent profile identifier. +// 1. List the task's workspace's Office agents eligible for role's seat +// (eligibleAgentRolesFor) and whose status is neither `stopped` nor +// `pending_approval`, ordered by `created_at` then agent profile +// identifier. // 2. Empty list: seat the task's runner (fallback provenance); no runner // resolves is unfillable. -// 3. Otherwise seat the candidate list's first member unless that member is -// the task's runner and an alternative exists, in which case seat the -// second member instead — this is what keeps an ordinary cast from -// being a self-review when a different reviewer is available. +// 3. Otherwise seat the first candidate that is neither the task's runner +// nor already holding another seat on this task's current workflow +// (best-effort cross-step exclusion, AC-OFFICE-REVIEW-SEATS-002.3) — +// this is what lets a reviewer and an approver land on different agents +// when the workspace shape allows it. If no such candidate exists, fall +// back to the runner-exclusion-only rule (skip the runner if any other +// candidate remains, otherwise seat the runner) so a seat is never left +// empty. // // The status exclusion is applied here, over ListAgentInstancesFiltered's // result, rather than by adding a filter to that shared method or changing @@ -51,7 +88,7 @@ func NewSeatCasterAdapter(office OfficeRepo, workflow SeatCasterWorkflowRepo) *S // CastParticipantSeat satisfies engine.ParticipantSeatCaster. func (a *SeatCasterAdapter) CastParticipantSeat( - ctx context.Context, taskID, stepID, role string, + ctx context.Context, workflowID, taskID, stepID, role string, ) (engine.ParticipantSeatCastResult, error) { if taskID == "" { return engine.ParticipantSeatCastResult{}, fmt.Errorf("task_id is required to cast a participant seat") @@ -72,29 +109,38 @@ func (a *SeatCasterAdapter) CastParticipantSeat( return engine.ParticipantSeatCastResult{}, fmt.Errorf("resolve task runner: %w", err) } - candidates, err := a.eligibleCEOCandidates(ctx, fields.WorkspaceID) + candidates, err := a.eligibleCandidates(ctx, fields.WorkspaceID, role) if err != nil { return engine.ParticipantSeatCastResult{}, fmt.Errorf("list eligible candidates: %w", err) } - return castFromCandidates(candidates, runner, fields.WorkspaceID), nil + return castFromCandidates(candidates, runner, a.alreadySeatedAgents(ctx, taskID, workflowID), fields.WorkspaceID), nil } -// eligibleCEOCandidates returns the workspace's Office agents whose role is -// `ceo` and whose status is neither `stopped` nor `pending_approval`, -// ordered by `created_at` ascending then agent profile identifier ascending -// (AC-OFFICE-REVIEW-SEATS-002.1, -002.2). -func (a *SeatCasterAdapter) eligibleCEOCandidates( - ctx context.Context, workspaceID string, +// eligibleCandidates returns the workspace's Office agents eligible for +// role's seat (eligibleAgentRolesFor) whose status is neither `stopped` nor +// `pending_approval`, ordered by `created_at` ascending then agent profile +// identifier ascending (AC-OFFICE-REVIEW-SEATS-002.1, -002.2). +// +// ListAgentInstancesFiltered's Role filter is a single string and cannot +// express the reviewer pool's ceo ∪ specialist union, so this lists with an +// empty Role filter and applies the allowlist here instead of pushing a +// union down to the shared listing method (AC-OFFICE-REVIEW-SEATS-002.2 — +// a shared listing whose behavior changes for everyone is how an unrelated +// Office surface silently loses agents). +func (a *SeatCasterAdapter) eligibleCandidates( + ctx context.Context, workspaceID, role string, ) ([]*models.AgentInstance, error) { - agents, err := a.Office.ListAgentInstancesFiltered(ctx, workspaceID, sqlite.AgentListFilter{ - Role: string(models.AgentRoleCEO), - }) + agents, err := a.Office.ListAgentInstancesFiltered(ctx, workspaceID, sqlite.AgentListFilter{}) if err != nil { return nil, err } + allowedRoles := eligibleAgentRolesFor(role) eligible := make([]*models.AgentInstance, 0, len(agents)) for _, ag := range agents { + if !allowedRoles[ag.Role] { + continue + } if ag.Status == models.AgentStatusStopped || ag.Status == models.AgentStatusPendingApproval { continue } @@ -110,13 +156,47 @@ func (a *SeatCasterAdapter) eligibleCEOCandidates( return eligible, nil } +// alreadySeatedAgents returns the set of agent profile ids already holding a +// participant seat anywhere on taskID's current workflow (workflowID), for +// castFromCandidates' best-effort cross-step exclusion. Scoping to workflowID +// keeps rows left over from a workflow the task has since switched away from +// (switch_workflow durably keeps them; see ListParticipantsForTaskWorkflow's +// doc comment) from ever counting as already seated. A read failure is +// non-fatal: the exclusion signal is optional and must never block casting a +// seat (AC-OFFICE-REVIEW-SEATS-002.3's best-effort clause), so this returns +// an empty set instead of propagating the error, and the caller casts +// without exclusion. +func (a *SeatCasterAdapter) alreadySeatedAgents(ctx context.Context, taskID, workflowID string) map[string]bool { + seated := map[string]bool{} + participants, err := a.Workflow.ListParticipantsForTaskWorkflow(ctx, taskID, workflowID) + if err != nil { + return seated + } + for _, p := range participants { + if p.AgentProfileID != "" { + seated[p.AgentProfileID] = true + } + } + return seated +} + // castFromCandidates applies steps 2-5 of the casting resolution algorithm -// to an already-ordered, already-filtered candidate list and a resolved -// runner (which may be empty, meaning "does not resolve"). workspaceID is -// stamped onto every result, including an Unfillable one, so the caller can -// emit AC-OFFICE-REVIEW-SEATS-004.1's warning record without a second -// lookup. -func castFromCandidates(candidates []*models.AgentInstance, runner, workspaceID string) engine.ParticipantSeatCastResult { +// to an already-ordered, already-filtered candidate list, a resolved runner +// (which may be empty, meaning "does not resolve"), and the set of agent +// profile ids already seated elsewhere on this task's current workflow. +// workspaceID is stamped onto every result, including an Unfillable one, so +// the caller can emit AC-OFFICE-REVIEW-SEATS-004.1's warning record without +// a second lookup. +// +// The cross-step exclusion is best-effort and never blocking +// (AC-OFFICE-REVIEW-SEATS-002.3): if every candidate is either the runner or +// already seated, the runner-exclusion-only rule decides instead of leaving +// the seat empty. SelfReview is then true for either reason, since both mean +// the chosen agent is already participating in this task's review in some +// capacity. +func castFromCandidates( + candidates []*models.AgentInstance, runner string, alreadySeated map[string]bool, workspaceID string, +) engine.ParticipantSeatCastResult { if len(candidates) == 0 { if runner == "" { return engine.ParticipantSeatCastResult{Unfillable: true, WorkspaceID: workspaceID} @@ -128,16 +208,26 @@ func castFromCandidates(candidates []*models.AgentInstance, runner, workspaceID SelfReview: true, } } - idx := 0 - if candidates[0].ID == runner && len(candidates) > 1 { - idx = 1 + + idx := -1 + for i, c := range candidates { + if c.ID != runner && !alreadySeated[c.ID] { + idx = i + break + } + } + if idx == -1 { + idx = 0 + if candidates[0].ID == runner && len(candidates) > 1 { + idx = 1 + } } chosen := candidates[idx] return engine.ParticipantSeatCastResult{ AgentProfileID: chosen.ID, WorkspaceID: workspaceID, Provenance: engine.SeatProvenanceEligiblePool, - SelfReview: chosen.ID == runner, + SelfReview: chosen.ID == runner || alreadySeated[chosen.ID], } } diff --git a/apps/backend/internal/office/engine_adapters/seat_caster_test.go b/apps/backend/internal/office/engine_adapters/seat_caster_test.go index c58c350af3..fa2e6dfdad 100644 --- a/apps/backend/internal/office/engine_adapters/seat_caster_test.go +++ b/apps/backend/internal/office/engine_adapters/seat_caster_test.go @@ -3,12 +3,14 @@ package engine_adapters import ( "context" "errors" + "fmt" "testing" "time" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" ) // fakeSeatCasterWorkflowRepo is a minimal SeatCasterWorkflowRepo fake for @@ -17,6 +19,11 @@ type fakeSeatCasterWorkflowRepo struct { runner string runnerErr error + seatedAgentIDs []string + participantsErr error + gotParticipantsFor string + gotParticipantsForWF string + gotRunnerStepID string gotRunnerTaskID string } @@ -27,12 +34,27 @@ func (f *fakeSeatCasterWorkflowRepo) ResolveCurrentRunner(_ context.Context, ste return f.runner, f.runnerErr } +func (f *fakeSeatCasterWorkflowRepo) ListParticipantsForTaskWorkflow( + _ context.Context, taskID, workflowID string, +) ([]*wfmodels.WorkflowStepParticipant, error) { + f.gotParticipantsFor = taskID + f.gotParticipantsForWF = workflowID + if f.participantsErr != nil { + return nil, f.participantsErr + } + participants := make([]*wfmodels.WorkflowStepParticipant, 0, len(f.seatedAgentIDs)) + for _, id := range f.seatedAgentIDs { + participants = append(participants, &wfmodels.WorkflowStepParticipant{AgentProfileID: id}) + } + return participants, nil +} + func TestSeatCasterAdapter_EmptyCandidateListFallsBackToRunner(t *testing.T) { office := &fakeOfficeRepo{fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}} wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -58,7 +80,7 @@ func TestSeatCasterAdapter_EmptyCandidateListAndNoRunnerIsUnfillable(t *testing. wf := &fakeSeatCasterWorkflowRepo{runner: ""} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -84,7 +106,7 @@ func TestSeatCasterAdapter_SingleCandidateIsRunnerRecordsSelfReview(t *testing.T wf := &fakeSeatCasterWorkflowRepo{runner: "agent-A"} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -112,7 +134,7 @@ func TestSeatCasterAdapter_FirstCandidateIsRunnerSeatsSecond(t *testing.T) { wf := &fakeSeatCasterWorkflowRepo{runner: "agent-A"} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -140,7 +162,7 @@ func TestSeatCasterAdapter_FirstCandidateNotRunnerSeatsFirstAsEligiblePool(t *te wf := &fakeSeatCasterWorkflowRepo{runner: "someone-else"} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -168,7 +190,7 @@ func TestSeatCasterAdapter_ExcludesStoppedAndPendingApprovalCandidates(t *testin wf := &fakeSeatCasterWorkflowRepo{runner: ""} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -200,7 +222,7 @@ func TestSeatCasterAdapter_OrdersCandidatesByCreatedAtThenIdentifier(t *testing. wf := &fakeSeatCasterWorkflowRepo{runner: ""} a := NewSeatCasterAdapter(office, wf) - got, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer") + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -211,7 +233,7 @@ func TestSeatCasterAdapter_OrdersCandidatesByCreatedAtThenIdentifier(t *testing. func TestSeatCasterAdapter_RejectsEmptyTaskID(t *testing.T) { a := NewSeatCasterAdapter(&fakeOfficeRepo{}, &fakeSeatCasterWorkflowRepo{}) - if _, err := a.CastParticipantSeat(context.Background(), "", "step-1", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "", "step-1", "reviewer"); err == nil { t.Fatal("expected error for empty task id") } } @@ -219,7 +241,7 @@ func TestSeatCasterAdapter_RejectsEmptyTaskID(t *testing.T) { func TestSeatCasterAdapter_PropagatesWorkspaceResolutionError(t *testing.T) { office := &fakeOfficeRepo{fieldsErr: errors.New("boom")} a := NewSeatCasterAdapter(office, &fakeSeatCasterWorkflowRepo{}) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer"); err == nil { t.Fatal("expected error") } } @@ -227,7 +249,7 @@ func TestSeatCasterAdapter_PropagatesWorkspaceResolutionError(t *testing.T) { func TestSeatCasterAdapter_PropagatesTaskWithNoWorkspaceAsError(t *testing.T) { office := &fakeOfficeRepo{fields: &sqlite.TaskExecutionFields{ID: "t-1"}} a := NewSeatCasterAdapter(office, &fakeSeatCasterWorkflowRepo{}) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer"); err == nil { t.Fatal("expected error for a task with no workspace") } } @@ -236,7 +258,7 @@ func TestSeatCasterAdapter_RejectsEmptyStepID(t *testing.T) { office := &fakeOfficeRepo{fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}} wf := &fakeSeatCasterWorkflowRepo{} a := NewSeatCasterAdapter(office, wf) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "", "reviewer"); err == nil { t.Fatal("expected error for empty step id") } } @@ -245,7 +267,7 @@ func TestSeatCasterAdapter_PropagatesRunnerResolutionError(t *testing.T) { office := &fakeOfficeRepo{fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}} wf := &fakeSeatCasterWorkflowRepo{runnerErr: errors.New("boom")} a := NewSeatCasterAdapter(office, wf) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer"); err == nil { t.Fatal("expected error") } } @@ -257,7 +279,7 @@ func TestSeatCasterAdapter_PropagatesCandidateListingError(t *testing.T) { } wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} a := NewSeatCasterAdapter(office, wf) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "step-1", "reviewer"); err == nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", "reviewer"); err == nil { t.Fatal("expected error") } } @@ -267,7 +289,7 @@ func TestSeatCasterAdapter_UsesEnteredStepToResolveRunner(t *testing.T) { wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} a := NewSeatCasterAdapter(office, wf) - if _, err := a.CastParticipantSeat(context.Background(), "t-1", "step-42", "reviewer"); err != nil { + if _, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-42", "reviewer"); err != nil { t.Fatalf("unexpected error: %v", err) } if wf.gotRunnerStepID != "step-42" || wf.gotRunnerTaskID != "t-1" { @@ -276,6 +298,293 @@ func TestSeatCasterAdapter_UsesEnteredStepToResolveRunner(t *testing.T) { } } +// TestSeatCasterAdapter_ScopesExclusionReadToCallersWorkflow covers the P1 +// fix: the cross-step exclusion read must scope to the workflowID the +// caller passes, not read task-wide across every workflow the task has ever +// been on. A workflow the task switched away from durably keeps its +// workflow_step_participants rows (switch_workflow), so an unscoped read +// would count a stale row from that abandoned workflow as "already seated" +// for the current one. +func TestSeatCasterAdapter_ScopesExclusionReadToCallersWorkflow(t *testing.T) { + office := &fakeOfficeRepo{fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}} + wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} + a := NewSeatCasterAdapter(office, wf) + + if _, err := a.CastParticipantSeat(context.Background(), "wf-current", "t-1", "step-1", "reviewer"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if wf.gotParticipantsFor != "t-1" || wf.gotParticipantsForWF != "wf-current" { + t.Errorf("ListParticipantsForTaskWorkflow called with task=%q workflow=%q, want t-1/wf-current", + wf.gotParticipantsFor, wf.gotParticipantsForWF) + } +} + +// TestSeatCasterAdapter_ReviewerPoolIncludesSpecialists is proof shape (a) +// from the task plan: 2 ceos + specialists. Reviewer is cast first with no +// exclusions in play, so it seats the earliest candidate; approver's +// narrower ceo-only pool then excludes that agent, landing on the other +// ceo. Fails with the change reverted, since the old code seats ceo-1 for +// both roles. +func TestSeatCasterAdapter_ReviewerPoolIncludesSpecialists(t *testing.T) { + tCEO1 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + tCEO2 := tCEO1.Add(time.Minute) + tSpec := tCEO1.Add(2 * time.Minute) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "ceo-1", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: tCEO1}, + {ID: "ceo-2", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: tCEO2}, + {ID: "spec-1", Role: models.AgentRoleSpecialist, Status: models.AgentStatusIdle, CreatedAt: tSpec}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: ""} + a := NewSeatCasterAdapter(office, wf) + + reviewer, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-review", "reviewer") + if err != nil { + t.Fatalf("reviewer cast: %v", err) + } + if reviewer.AgentProfileID != "ceo-1" { + t.Fatalf("reviewer = %q, want ceo-1 (earliest created_at)", reviewer.AgentProfileID) + } + // Pins the regression a reverted seat_caster.go:132 (Role: ceo) would + // reintroduce: the pool union can only be expressed by listing with no + // Role filter and applying the allowlist in Go (AC-OFFICE-REVIEW-SEATS-002.2). + if office.gotFilter.Role != "" { + t.Errorf("reviewer cast: filter role = %q, want empty (pool allowlist is applied in Go, not pushed to the shared listing)", office.gotFilter.Role) + } + + wf.seatedAgentIDs = []string{reviewer.AgentProfileID} + approver, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("approver cast: %v", err) + } + if approver.AgentProfileID != "ceo-2" { + t.Fatalf("approver = %q, want ceo-2 (ceo-1 excluded; the approver pool never includes spec-1)", approver.AgentProfileID) + } + if office.gotFilter.Role != "" { + t.Errorf("approver cast: filter role = %q, want empty (pool allowlist is applied in Go, not pushed to the shared listing)", office.gotFilter.Role) + } + if reviewer.AgentProfileID == approver.AgentProfileID { + t.Fatal("expected reviewer and approver to be different agents") + } +} + +// TestSeatCasterAdapter_ReviewerWidensToOlderSpecialist is proof shape (b): +// 1 ceo + 1 specialist, with the specialist older. Reviewer's widened pool +// picks the specialist over the ceo by created_at; approver's ceo-only pool +// never sees the specialist at all. Fails with the change reverted, since +// the old code never considers the specialist for either role. +func TestSeatCasterAdapter_ReviewerWidensToOlderSpecialist(t *testing.T) { + tSpec := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + tCEO := tSpec.Add(time.Minute) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "spec-1", Role: models.AgentRoleSpecialist, Status: models.AgentStatusIdle, CreatedAt: tSpec}, + {ID: "ceo-1", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: tCEO}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: ""} + a := NewSeatCasterAdapter(office, wf) + + reviewer, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-review", "reviewer") + if err != nil { + t.Fatalf("reviewer cast: %v", err) + } + if reviewer.AgentProfileID != "spec-1" { + t.Fatalf("reviewer = %q, want spec-1 (earliest created_at across ceo ∪ specialist)", reviewer.AgentProfileID) + } + // Pins the regression a reverted seat_caster.go:132 (Role: ceo) would + // reintroduce: the pool union can only be expressed by listing with no + // Role filter and applying the allowlist in Go (AC-OFFICE-REVIEW-SEATS-002.2). + if office.gotFilter.Role != "" { + t.Errorf("reviewer cast: filter role = %q, want empty (pool allowlist is applied in Go, not pushed to the shared listing)", office.gotFilter.Role) + } + + wf.seatedAgentIDs = []string{reviewer.AgentProfileID} + approver, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("approver cast: %v", err) + } + if approver.AgentProfileID != "ceo-1" { + t.Fatalf("approver = %q, want ceo-1 (the approver pool is ceo-only)", approver.AgentProfileID) + } + if office.gotFilter.Role != "" { + t.Errorf("approver cast: filter role = %q, want empty (pool allowlist is applied in Go, not pushed to the shared listing)", office.gotFilter.Role) + } + if reviewer.AgentProfileID == approver.AgentProfileID { + t.Fatal("expected reviewer and approver to be different agents") + } +} + +// TestSeatCasterAdapter_ApproverPoolStaysCEOOnly pins the D-B ruling's +// accepted non-divergence: a workspace with exactly one ceo (plus any +// number of specialists) seats the same agent for both reviewer and +// approver, because the approver pool never widens to specialists. This is +// ruled behavior, not a regression — do not "fix" it by widening the +// approver pool. +func TestSeatCasterAdapter_ApproverPoolStaysCEOOnly(t *testing.T) { + t0 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + agents := []*models.AgentInstance{ + {ID: "ceo-1", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: t0}, + } + for i := 0; i < 4; i++ { + agents = append(agents, &models.AgentInstance{ + ID: fmt.Sprintf("spec-%d", i), + Role: models.AgentRoleSpecialist, + Status: models.AgentStatusIdle, + CreatedAt: t0.Add(time.Duration(i+1) * time.Minute), + }) + } + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: agents, + } + wf := &fakeSeatCasterWorkflowRepo{runner: ""} + a := NewSeatCasterAdapter(office, wf) + + reviewer, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-review", "reviewer") + if err != nil { + t.Fatalf("reviewer cast: %v", err) + } + if reviewer.AgentProfileID != "ceo-1" { + t.Fatalf("reviewer = %q, want ceo-1 (earliest created_at)", reviewer.AgentProfileID) + } + + wf.seatedAgentIDs = []string{reviewer.AgentProfileID} + approver, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("approver cast: %v", err) + } + if approver.AgentProfileID != "ceo-1" { + t.Fatalf("approver = %q, want ceo-1 (the sole ceo; must not widen to specialists)", approver.AgentProfileID) + } + if !approver.SelfReview { + t.Error("expected SelfReview when the only approver candidate is already seated as reviewer") + } +} + +// TestSeatCasterAdapter_ApproverPoolExcludesSpecialistsEvenWhenCEOPoolEmpty +// covers AC-OFFICE-REVIEW-SEATS-002.5/002.6 for the narrower approver pool: +// a workspace with specialists but no ceo still falls back to the runner +// for an approver seat, rather than reaching into the specialist pool that +// only reviewer is entitled to. +func TestSeatCasterAdapter_ApproverPoolExcludesSpecialistsEvenWhenCEOPoolEmpty(t *testing.T) { + t0 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "spec-1", Role: models.AgentRoleSpecialist, Status: models.AgentStatusIdle, CreatedAt: t0}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} + a := NewSeatCasterAdapter(office, wf) + + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.AgentProfileID != "runner-agent" { + t.Errorf("agent = %q, want runner-agent (approver pool is ceo-only and has no members here)", got.AgentProfileID) + } + if got.Provenance != engine.SeatProvenanceRunnerFallback { + t.Errorf("provenance = %q, want %q", got.Provenance, engine.SeatProvenanceRunnerFallback) + } +} + +// TestSeatCasterAdapter_ExclusionNeverEmptiesASeat covers +// AC-OFFICE-REVIEW-SEATS-002.3's never-blocking guarantee: the sole +// candidate is already seated at another step on this task, and must be +// seated anyway rather than left unfillable. +func TestSeatCasterAdapter_ExclusionNeverEmptiesASeat(t *testing.T) { + t0 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "ceo-1", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: t0}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: "", seatedAgentIDs: []string{"ceo-1"}} + a := NewSeatCasterAdapter(office, wf) + + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.Unfillable { + t.Fatal("expected the sole already-seated candidate to be seated anyway, not left unfillable") + } + if got.AgentProfileID != "ceo-1" { + t.Errorf("agent = %q, want ceo-1", got.AgentProfileID) + } + if !got.SelfReview { + t.Error("expected SelfReview when the only candidate already holds another seat on this task") + } +} + +// TestSeatCasterAdapter_ExclusionReadFailureIsNonFatal covers +// AC-OFFICE-REVIEW-SEATS-002.3's best-effort clause: a failure reading the +// cross-step exclusion signal must not fail the cast — it degrades to +// casting without exclusion. +func TestSeatCasterAdapter_ExclusionReadFailureIsNonFatal(t *testing.T) { + t1 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + t2 := t1.Add(time.Minute) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "ceo-1", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: t1}, + {ID: "ceo-2", Role: models.AgentRoleCEO, Status: models.AgentStatusIdle, CreatedAt: t2}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: "", participantsErr: errors.New("boom")} + a := NewSeatCasterAdapter(office, wf) + + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-approve", "approver") + if err != nil { + t.Fatalf("expected the seat to still be cast when the exclusion read fails: %v", err) + } + if got.AgentProfileID != "ceo-1" { + t.Errorf("agent = %q, want ceo-1 (cast without exclusion when the read errors)", got.AgentProfileID) + } +} + +// TestSeatCasterAdapter_PoolAllowlistIsPositiveNotADenylist covers the +// guard called out in the task plan: the empty-Role listing this adapter +// now issues also returns worker/assistant/qa/security/devops and +// empty-role agent profiles, and the Go allowlist must exclude them by not +// naming their roles, not by naming roles to reject. +func TestSeatCasterAdapter_PoolAllowlistIsPositiveNotADenylist(t *testing.T) { + t0 := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + office := &fakeOfficeRepo{ + fields: &sqlite.TaskExecutionFields{ID: "t-1", WorkspaceID: "ws-1"}, + agents: []*models.AgentInstance{ + {ID: "worker-1", Role: models.AgentRoleWorker, Status: models.AgentStatusIdle, CreatedAt: t0}, + {ID: "assistant-1", Role: models.AgentRoleAssistant, Status: models.AgentStatusIdle, CreatedAt: t0}, + {ID: "qa-1", Role: models.AgentRoleQA, Status: models.AgentStatusIdle, CreatedAt: t0}, + {ID: "security-1", Role: models.AgentRoleSecurity, Status: models.AgentStatusIdle, CreatedAt: t0}, + {ID: "devops-1", Role: models.AgentRoleDevOps, Status: models.AgentStatusIdle, CreatedAt: t0}, + {ID: "no-role-1", Role: "", Status: models.AgentStatusIdle, CreatedAt: t0}, + }, + } + wf := &fakeSeatCasterWorkflowRepo{runner: "runner-agent"} + a := NewSeatCasterAdapter(office, wf) + + for _, role := range []string{"reviewer", "approver"} { + got, err := a.CastParticipantSeat(context.Background(), "wf-1", "t-1", "step-1", role) + if err != nil { + t.Fatalf("role %q: unexpected error: %v", role, err) + } + if got.AgentProfileID != "runner-agent" { + t.Errorf("role %q: agent = %q, want runner-agent — none of worker/assistant/qa/security/devops/empty-role may be seated", + role, got.AgentProfileID) + } + if got.Provenance != engine.SeatProvenanceRunnerFallback { + t.Errorf("role %q: provenance = %q, want %q", role, got.Provenance, engine.SeatProvenanceRunnerFallback) + } + } +} + func TestSeatCasterAdapter_SatisfiesParticipantSeatCaster(t *testing.T) { var _ engine.ParticipantSeatCaster = (*SeatCasterAdapter)(nil) } diff --git a/apps/backend/internal/workflow/engine/adapters.go b/apps/backend/internal/workflow/engine/adapters.go index a11c889d79..b5de4a286d 100644 --- a/apps/backend/internal/workflow/engine/adapters.go +++ b/apps/backend/internal/workflow/engine/adapters.go @@ -165,6 +165,12 @@ const ( // resolved for the role at all. WorkspaceID is populated on every result, // including Unfillable ones, so the AC-OFFICE-REVIEW-SEATS-004.1 warning // record can identify the workspace without a second lookup. +// +// SelfReview is true both when the chosen agent is the task's runner and +// when it already holds another seat on this task (the office seat +// caster's best-effort cross-step exclusion, AC-OFFICE-REVIEW-SEATS-002.3, +// ran out of alternatives). It is a counter label on RecordSeatProvenance, +// not a persisted or user-visible field. type ParticipantSeatCastResult struct { AgentProfileID string WorkspaceID string @@ -175,13 +181,18 @@ type ParticipantSeatCastResult struct { // ParticipantSeatCaster resolves which agent should fill a role's seat when // none exists yet, per REQ-002's five-step deterministic algorithm. The -// office package implements this against the workspace's CEO-role agent -// roster; the engine treats the result as opaque. +// office package implements this against role-specific workspace agent pools; +// the engine treats the result as opaque. type ParticipantSeatCaster interface { // stepID is the immutable workflow step that the task entered. Callers // must pass this value instead of asking the adapter to re-read mutable - // task state after the transition commits. - CastParticipantSeat(ctx context.Context, taskID, stepID, role string) (ParticipantSeatCastResult, error) + // task state after the transition commits. workflowID scopes any + // cross-step exclusion read the caster performs to the task's current + // workflow, so participant rows left over from a workflow the task has + // since switched away from (switch_workflow durably keeps them; see + // ListParticipantsForTaskWorkflow's doc comment) never count as already + // seated for the new workflow's casting decision. + CastParticipantSeat(ctx context.Context, workflowID, taskID, stepID, role string) (ParticipantSeatCastResult, error) } // AgentProfileResolver answers whether an agent profile id still resolves to diff --git a/apps/backend/internal/workflow/engine/ensure_participant_seat_test.go b/apps/backend/internal/workflow/engine/ensure_participant_seat_test.go index 80cc8f15bb..b6a83515bd 100644 --- a/apps/backend/internal/workflow/engine/ensure_participant_seat_test.go +++ b/apps/backend/internal/workflow/engine/ensure_participant_seat_test.go @@ -49,18 +49,20 @@ func (f *fakeParticipantSeatWriter) EnsureRoleSeat( // fakeParticipantSeatCaster records CastParticipantSeat calls for // EnsureParticipantSeatCallback assertions. type fakeParticipantSeatCaster struct { - result ParticipantSeatCastResult - err error - calls int - lastTaskID string - lastStepID string - lastRole string + result ParticipantSeatCastResult + err error + calls int + lastWorkflowID string + lastTaskID string + lastStepID string + lastRole string } func (f *fakeParticipantSeatCaster) CastParticipantSeat( - _ context.Context, taskID, stepID, role string, + _ context.Context, workflowID, taskID, stepID, role string, ) (ParticipantSeatCastResult, error) { f.calls++ + f.lastWorkflowID = workflowID f.lastTaskID = taskID f.lastStepID = stepID f.lastRole = role @@ -149,9 +151,10 @@ func TestEnsureParticipantSeatCallback_SuccessfulCastWritesSeat(t *testing.T) { if _, err := cb.Execute(context.Background(), newEnsureSeatInput("reviewer")); err != nil { t.Fatalf("unexpected error: %v", err) } - if caster.calls != 1 || caster.lastTaskID != "task-1" || caster.lastStepID != "step-1" || caster.lastRole != "reviewer" { - t.Fatalf("expected caster invoked once for (task-1, step-1, reviewer), got calls=%d taskID=%q stepID=%q role=%q", - caster.calls, caster.lastTaskID, caster.lastStepID, caster.lastRole) + if caster.calls != 1 || caster.lastWorkflowID != "wf-1" || caster.lastTaskID != "task-1" || + caster.lastStepID != "step-1" || caster.lastRole != "reviewer" { + t.Fatalf("expected caster invoked once for (wf-1, task-1, step-1, reviewer), got calls=%d workflowID=%q taskID=%q stepID=%q role=%q", + caster.calls, caster.lastWorkflowID, caster.lastTaskID, caster.lastStepID, caster.lastRole) } if len(writer.ensureCalls) != 1 { t.Fatalf("expected exactly one seat write, got %+v", writer.ensureCalls) diff --git a/apps/backend/internal/workflow/engine/phase2_callbacks.go b/apps/backend/internal/workflow/engine/phase2_callbacks.go index decd16c354..269b4c8d18 100644 --- a/apps/backend/internal/workflow/engine/phase2_callbacks.go +++ b/apps/backend/internal/workflow/engine/phase2_callbacks.go @@ -572,7 +572,7 @@ func (c EnsureParticipantSeatCallback) Execute(ctx context.Context, in ActionInp c.recordSeatEnsureError(taskID, stepID, role, err) return ActionResult{}, err } - cast, err := c.Caster.CastParticipantSeat(ctx, taskID, stepID, role) + cast, err := c.Caster.CastParticipantSeat(ctx, workflowID, taskID, stepID, role) if err != nil { c.recordSeatEnsureError(taskID, stepID, role, err) return ActionResult{}, fmt.Errorf("ensure_participant_seat cast: %w", err) diff --git a/docs/specs/office/requirements/review-participant-seats.md b/docs/specs/office/requirements/review-participant-seats.md index 8a22aa75ab..ad89ea5d36 100644 --- a/docs/specs/office/requirements/review-participant-seats.md +++ b/docs/specs/office/requirements/review-participant-seats.md @@ -21,8 +21,8 @@ This capability makes those seats exist by default. When a task **enters** a step that declares it, the system resolves which Office agent should fill the step's participant role and writes a seat for that task at that step, before the step's own fan-out reads the slate. Casting is **derived**, not configured: -the workspace CEO reviews, and the task's runner is the fallback when there is -no CEO to seat. +the reviewer pool uses `ceo` and `specialist`, while other roles use `ceo`. +The runner is the fallback when the pool is empty. Seats are written for the step being **entered**, never for the step the task stands on when it is created; that distinction is the whole defect behind the @@ -72,8 +72,10 @@ quorum primitives written into here, not the casting decision. row order; AC-OFFICE-REVIEW-SEATS-002.10 states why that distinction is a requirement rather than an implementation detail. - **Eligible agent** - an Office agent in the task's workspace whose Office - agent role is `ceo` and whose status is neither `stopped` nor - `pending_approval`. + agent role is in the participant role's pool (`ceo` and `specialist` for the + reviewer role; `ceo` alone for the approver role and for every other + participant role, per AC-OFFICE-REVIEW-SEATS-002.1) and whose status is + neither `stopped` nor `pending_approval`. - **Warning record** - the structured record accompanying each counter increment in REQ-OFFICE-REVIEW-SEATS-004, carrying the identifiers a counter label may not. @@ -138,7 +140,9 @@ configured. #### Acceptance criteria - **AC-OFFICE-REVIEW-SEATS-002.1:** The system shall resolve the candidate list - for a participant role to the eligible agents of the task's workspace, + for a participant role to the task's workspace agents whose agent role is in + that participant role's pool — `ceo` and `specialist` for the reviewer role; + `ceo` alone for the approver role and for every other participant role — ordered by `created_at` ascending and then by agent profile identifier ascending. - **AC-OFFICE-REVIEW-SEATS-002.2:** The system shall exclude from the candidate @@ -146,8 +150,11 @@ configured. and shall do so without changing the behavior of any other caller of the agent listing it uses. - **AC-OFFICE-REVIEW-SEATS-002.3:** When the candidate list holds more than one - eligible agent and its first member is the task's runner, the system shall - seat the next member instead. + eligible agent, the system shall seat the first member that is neither the + task's runner nor already seated in another participant role on the task. + This exclusion is best-effort and shall not leave the seat empty: when no + such member exists, the system shall fall back to seating the first member, + or the second member when the first is the task's runner. - **AC-OFFICE-REVIEW-SEATS-002.4:** When the candidate list holds exactly one eligible agent and that agent is the task's runner, the system shall seat that agent and shall record the self-review. @@ -163,10 +170,14 @@ configured. - **AC-OFFICE-REVIEW-SEATS-002.8:** When resolution fails with an error rather than an empty result, the system shall write no seat, emit the unfillable signal, and not fail the step entry. -- **AC-OFFICE-REVIEW-SEATS-002.9:** When the candidate list holds more than one - eligible agent and its first member is not the task's runner, the system - shall seat that first member and record the seating as coming from the - eligible pool rather than the fallback. +- **AC-OFFICE-REVIEW-SEATS-002.9:** When the candidate list holds exactly one + eligible agent and that agent is not the task's runner, the system shall + seat that agent, record the seating as coming from the eligible pool rather + than the fallback, and record the self-review when that agent is already + seated in another participant role on the task. This criterion's domain + (exactly one non-runner candidate) is disjoint from + AC-OFFICE-REVIEW-SEATS-002.3's (more than one candidate); the two do not + compete over the same input. - **AC-OFFICE-REVIEW-SEATS-002.10:** Resolution of the task's runner shall produce the same result on every database engine the product supports, and its final precedence tier shall order candidate seats by a persisted column @@ -312,11 +323,14 @@ Each exclusion below is a contract, not an omission. bringing an existing workspace's step *configuration* up to date is required by AC-OFFICE-REVIEW-SEATS-005.6 and is not a sweep, because it touches workflow rows once at startup and touches no task. -- **Widening the eligible pool beyond the CEO role.** Office agent roles and - participant roles are disjoint vocabularies and no Office agent role names a - reviewer. The shipped `security` role's description does mention approving or - blocking at review gates, so this is a live choice rather than a vacuous one; - widening it belongs to the configurable-slate capability. +- **Widening either pool beyond the roles AC-OFFICE-REVIEW-SEATS-002.1 names.** + The reviewer pool is `ceo` and `specialist`; the approver pool, and every + other participant role's pool, stays `ceo` alone — a per-role ruling, not a + general relaxation. Office agent roles and participant roles remain disjoint + vocabularies otherwise: the shipped `security` role's description does + mention approving or blocking at review gates, so admitting it is a live + choice rather than a vacuous one; widening either pool further belongs to + the configurable-slate capability. - **The `backlog` step declaring no events.** A task started there has no turn-complete and no children-completed handler and parks permanently. Real, and not fixed by seats. diff --git a/docs/specs/office/system-design/review-participant-seats-01.md b/docs/specs/office/system-design/review-participant-seats-01.md index 8ede747e00..66e23e0935 100644 --- a/docs/specs/office/system-design/review-participant-seats-01.md +++ b/docs/specs/office/system-design/review-participant-seats-01.md @@ -254,54 +254,54 @@ task rejected at Approval and reworked, same seat, new review round. Given workspace, task, step and role: -1. List the workspace's Office agents whose role is `ceo` and whose status is - neither `stopped` nor `pending_approval`, ordered by `created_at` then - identifier. +1. List the workspace's Office agents whose agent role is in that role's pool + (`ceo` + `specialist` for reviewer, `ceo` alone otherwise, per the D-B + ruling, 2026-09-01) and whose status is neither `stopped` nor + `pending_approval`, ordered by `created_at` then identifier + (`AC-OFFICE-REVIEW-SEATS-002.1`). Both filters apply over the shared + listing's result, not a change to it, so other callers are unaffected + (`AC-OFFICE-REVIEW-SEATS-002.2`). 2. If the list is empty, seat the task's runner, record the fallback provenance, and record self-review if the runner is the seated agent (`AC-OFFICE-REVIEW-SEATS-002.5`). If the runner does not resolve, write no seat and emit the unfillable signal (`AC-OFFICE-REVIEW-SEATS-002.6`). 3. If the list holds one agent and it is the task's runner, seat it and record - self-review (`AC-OFFICE-REVIEW-SEATS-002.4`). -4. If the list holds more than one agent and the first is the task's runner, - seat the second (`AC-OFFICE-REVIEW-SEATS-002.3`). -5. Otherwise seat the first, recording the provenance as *eligible pool* rather - than *fallback* (`AC-OFFICE-REVIEW-SEATS-002.9`). This is the ordinary case, - and naming it is what makes the provenance counter's values exhaustive - rather than exceptional. + self-review (`AC-OFFICE-REVIEW-SEATS-002.4`) — including a one-`ceo` + workspace seating the same agent for both reviewer and approver, accepted + under the D-B ruling. +4. If the list holds more than one agent, seat the first member that is + neither the runner nor already seated in another role on the task, + workflow-wide (`AC-OFFICE-REVIEW-SEATS-002.3`). Best-effort: a failed read + is non-fatal, and if every member is excluded, casting falls back to the + first member (second, if the first is the runner) as before. Self-review + is recorded either way. +5. Otherwise (exactly one candidate, not the runner) seat it, recording the + provenance as *eligible pool* rather than *fallback*, and recording + self-review if it is already seated in another role on the task + (`AC-OFFICE-REVIEW-SEATS-002.9`). Disjoint from step 4's domain. **The runner resolver does not work on Postgres, and is repaired here.** Step 2 -is the only path that seats anyone in a CEO-less workspace, and in this card's -scenario it is the path that runs: the task stands at `review`, its `runner` -seat was written at `work`, and `review` declares no primary agent profile, so -the resolver's first two tiers miss and the third decides. That third tier -orders by `rowid`, which does not exist on Postgres, in a file that branches on -the dialect elsewhere - so the call errors instead of returning an agent, -routing to `AC-OFFICE-REVIEW-SEATS-002.6`: no seat, unfillable signal, task -parked. The defect this capability exists to remove would survive on Postgres -while every SQLite test passed. +is the path that seats the task's runner when the eligible pool is empty. In +this card's no-eligible-candidate scenario it runs: the task stands at +`review`, its `runner` seat was written at `work`, and `review` declares no +primary agent profile, so the resolver's +first two tiers miss and the third decides. That tier orders by `rowid`, +absent on Postgres, so the call errors instead of returning an agent, routing +to `AC-OFFICE-REVIEW-SEATS-002.6`: no seat, unfillable signal, task parked - +the defect this capability removes would survive on Postgres while every +SQLite test passed. -Swapping `rowid` for the table's `id` does not fix it: that column is a text -identifier with no ordering meaning, so ordering by it is a random pick wearing -a determinism costume, which `AC-OFFICE-REVIEW-SEATS-002.10` forbids. The table -carries no timestamp, so **no existing column can answer "most recently -written"**. +Swapping `rowid` for `id` does not fix it: `id` is a text identifier with no +ordering meaning, so ordering by it is a random pick, which +`AC-OFFICE-REVIEW-SEATS-002.10` forbids, and the table carries no timestamp to +order by instead. -**Design position: the repair is in scope, and it is a column.** The migration -*Persistence* already requires also adds a creation timestamp, and the third -tier orders by it descending with `agent_profile_id` ascending as tiebreak - a -named column and a named tiebreak, identical on both engines. Pre-existing rows -backfill to one constant, so they are mutually tied and resolved by the -tiebreak: still deterministic. Scoping the repair out was considered and -rejected; it leaves this capability's only CEO-less path broken on one of two -supported engines. - -**Where the status exclusion is applied.** `AC-OFFICE-REVIEW-SEATS-002.2` -requires excluding `stopped` and `pending_approval` without changing any other -caller of the agent listing. The exclusion is therefore applied in the caster, -over the listing's result, not by adding a filter to the shared listing method -or by changing its default. A shared listing whose behavior changes for -everyone is how an unrelated Office surface silently loses agents. +**Design position: the repair is in scope, and it is a column.** The +migration *Persistence* already requires also adds a creation timestamp; the +third tier orders by it descending, `agent_profile_id` ascending as tiebreak - +identical on both engines. Pre-existing rows backfill to one constant, tied +and resolved by the tiebreak. Scoping the repair out was rejected: it leaves +this capability's only CEO-less path broken on one supported engine. ## Failure and recovery