From 797270f7ea772f1147ea248319f0860d281bf65c Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 2 Sep 2026 16:55:44 +0000 Subject: [PATCH] fix(workflow): recognize reviewer decisions from agents who also hold the approver seat (#3270) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../internal/workflow/engine/quorum.go | 44 +++++++--- .../engine/quorum_agent_resolver_test.go | 57 +++++++++++++ .../workflow/engine/quorum_role_test.go | 83 +++++++++++++++++++ 3 files changed, 173 insertions(+), 11 deletions(-) diff --git a/apps/backend/internal/workflow/engine/quorum.go b/apps/backend/internal/workflow/engine/quorum.go index 5d8a386b77..e638e5754f 100644 --- a/apps/backend/internal/workflow/engine/quorum.go +++ b/apps/backend/internal/workflow/engine/quorum.go @@ -902,8 +902,10 @@ func (e *Engine) evaluateGuardStateReadOnly( // slate construction the quorum evaluator itself uses — so role // resolution and slate membership can never disagree about who // participates, per AC-57's "exactly once, in the engine" mandate. -// Approver is checked before reviewer, matching the existing -// approver-wins precedence in office/dashboard/decisions.go's +// When the caller holds both seats, the seat whose StepID equals stepID +// wins, since that is the seat the current step's guard actually reads +// decisions against. If both seats sit at stepID, or neither does, +// approver wins, matching the precedence in office/dashboard/decisions.go's // resolveDeciderRole (AC-4). Returns ErrParticipantNotFound when the // agent occupies neither seat. func (e *Engine) ResolveParticipantRole( @@ -916,28 +918,48 @@ func (e *Engine) ResolveParticipantRole( if state, stateErr := e.store.LoadState(ctx, taskID, ""); stateErr == nil { workflowID = state.WorkflowID } + + var matches []roleSeat + var firstSeatsErr error for _, r := range []wfmodels.ParticipantRole{ wfmodels.ParticipantRoleApprover, wfmodels.ParticipantRoleReviewer, } { seats, seatsErr := e.requiredSeatsForWorkflow(ctx, stepID, taskID, workflowID, string(r)) if seatsErr != nil { - return "", "", fmt.Errorf("resolve participant role: %w", seatsErr) + if firstSeatsErr == nil { + firstSeatsErr = seatsErr + } + continue } - if id, ok := seatIDFor(seats, agentProfileID); ok { - return string(r), id, nil + if seat, ok := seatFor(seats, agentProfileID); ok { + if seat.StepID == stepID { + return string(r), seat.ID, nil + } + matches = append(matches, roleSeat{role: string(r), seat: seat}) } } - return "", "", ErrParticipantNotFound + if firstSeatsErr != nil { + return "", "", fmt.Errorf("resolve participant role: %w", firstSeatsErr) + } + if len(matches) == 0 { + return "", "", ErrParticipantNotFound + } + return matches[0].role, matches[0].seat.ID, nil } -// seatIDFor scans seats for one occupied by agentProfileID, returning its -// AC-50 canonical seat id. -func seatIDFor(seats []ParticipantInfo, agentProfileID string) (string, bool) { +// roleSeat pairs a resolved role with the seat matched under it. +type roleSeat struct { + role string + seat ParticipantInfo +} + +// seatFor scans seats for one occupied by agentProfileID. +func seatFor(seats []ParticipantInfo, agentProfileID string) (ParticipantInfo, bool) { for _, s := range seats { if s.AgentProfileID == agentProfileID { - return s.ID, true + return s, true } } - return "", false + return ParticipantInfo{}, false } diff --git a/apps/backend/internal/workflow/engine/quorum_agent_resolver_test.go b/apps/backend/internal/workflow/engine/quorum_agent_resolver_test.go index 0b1fd9d036..80e48831f0 100644 --- a/apps/backend/internal/workflow/engine/quorum_agent_resolver_test.go +++ b/apps/backend/internal/workflow/engine/quorum_agent_resolver_test.go @@ -227,6 +227,63 @@ func TestComputeGuardOutcome_ResolverErrorSurfacesAsEvaluationError(t *testing.T // held one resolvable and one unresolved-agent seat still fires once the // resolvable seat approves, instead of waiting forever on a seat that can // never decide. +// perIDErrResolver errors only for the agent profile ids listed in errFor, +// resolving every other id as present. +type perIDErrResolver struct { + errFor map[string]error +} + +func (r *perIDErrResolver) AgentProfileExists(_ context.Context, agentProfileID string) (bool, error) { + if err, ok := r.errFor[agentProfileID]; ok { + return false, err + } + return true, nil +} + +// TestResolveParticipantRole_ApproverAtStepSkipsReviewerResolverError is the +// regression test for FINDING R1-A: when the caller's approver seat already +// sits at stepID, that seat is both matches[0] and the step-preferred +// answer, so the reviewer slate must never be built. A resolver error on the +// reviewer seat's agent profile would otherwise fail the whole resolution. +func TestResolveParticipantRole_ApproverAtStepSkipsReviewerResolverError(t *testing.T) { + boom := errors.New("boom") + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-approver", TaskID: "task-1", StepID: "review", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-reviewer", TaskID: "task-1", StepID: "other-step", Role: "reviewer", AgentProfileID: "agent-b", DecisionRequired: true}, + }} + resolver := &perIDErrResolver{errFor: map[string]error{"agent-b": boom}} + eng := quorumEngineWithResolver(newFakeDecisionStore(), participants, resolver) + + role, participantID, err := eng.ResolveParticipantRole(context.Background(), "task-1", "review", "agent-a") + if err != nil { + t.Fatalf("ResolveParticipantRole: %v", err) + } + if role != "approver" || participantID != "seat-approver" { + t.Errorf("role/participantID = %q/%q, want approver/seat-approver", role, participantID) + } +} + +// TestResolveParticipantRole_ReviewerAtStepDefersApproverResolverError is the +// inverse of the approver-at-step case: an unrelated approver resolver error +// must not hide a valid reviewer seat at the step being evaluated. +func TestResolveParticipantRole_ReviewerAtStepDefersApproverResolverError(t *testing.T) { + boom := errors.New("boom") + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-approver", TaskID: "task-1", StepID: "other-step", Role: "approver", AgentProfileID: "agent-b", DecisionRequired: true}, + {ID: "seat-reviewer", TaskID: "task-1", StepID: "review", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + resolver := &perIDErrResolver{errFor: map[string]error{"agent-b": boom}} + eng := quorumEngineWithResolver(newFakeDecisionStore(), participants, resolver) + + role, participantID, err := eng.ResolveParticipantRole(context.Background(), "task-1", "review", "agent-a") + if err != nil { + t.Fatalf("ResolveParticipantRole: %v", err) + } + if role != "reviewer" || participantID != "seat-reviewer" { + t.Errorf("role/participantID = %q/%q, want reviewer/seat-reviewer", role, participantID) + } +} + func TestComputeGuardOutcome_ContinuesEvaluatingAfterDroppingUnresolvedSeat(t *testing.T) { parts := scopedParticipants{template: []ParticipantInfo{ {ID: "p1", StepID: "review", Role: "reviewer", AgentProfileID: "rev-A", DecisionRequired: true}, diff --git a/apps/backend/internal/workflow/engine/quorum_role_test.go b/apps/backend/internal/workflow/engine/quorum_role_test.go index 0d55da0f40..dc96eac573 100644 --- a/apps/backend/internal/workflow/engine/quorum_role_test.go +++ b/apps/backend/internal/workflow/engine/quorum_role_test.go @@ -133,3 +133,86 @@ func TestResolveParticipantRole_StoreErrorWrapped(t *testing.T) { t.Fatalf("err = %v, want wrapped store error", err) } } + +// TestResolveParticipantRole_StepPreferredOverApproverWins is the +// regression test for the Review re-entry bug: a caller holding both +// seats at different steps must resolve under the role whose seat sits +// at the step actually being evaluated, not under approver-wins. The +// approver seat was cast once the task first reached Approval and +// persists across the Review re-entry that follows an Approval +// rejection, so both rows coexist by the time this resolves at "review". +func TestResolveParticipantRole_StepPreferredOverApproverWins(t *testing.T) { + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "review", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "approval", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + eng := quorumEngine(nil, participants) + + role, participantID, err := eng.ResolveParticipantRole(context.Background(), "task-1", "review", "agent-a") + if err != nil { + t.Fatalf("ResolveParticipantRole: %v", err) + } + if role != "reviewer" || participantID != "seat-reviewer" { + t.Errorf("role/participantID = %q/%q, want reviewer/seat-reviewer", role, participantID) + } +} + +// TestResolveParticipantRole_StepPreferredTransitionFires proves the +// second half of the DoD: resolving under the step-preferred role and +// recording the decision under the resolved seat actually satisfies the +// reviewer quorum guard and fires Review -> Approval, where it used to +// park forever because the decision was misfiled against a seat absent +// from the reviewer slate. +func TestResolveParticipantRole_StepPreferredTransitionFires(t *testing.T) { + store := quorumStore(&TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "reviewer", Threshold: QuorumAllApprove}, + }) + decisions := newFakeDecisionStore() + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "review", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "approval", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + eng := New(store, MapRegistry{}, WithDecisionStore(decisions), WithParticipantStore(participants)) + + role, participantID, err := eng.ResolveParticipantRole(context.Background(), "task-1", "review", "agent-a") + if err != nil { + t.Fatalf("ResolveParticipantRole: %v", err) + } + if role != "reviewer" || participantID != "seat-reviewer" { + t.Fatalf("role/participantID = %q/%q, want reviewer/seat-reviewer", role, participantID) + } + + result, err := eng.RecordParticipantDecision(context.Background(), "sess-1", DecisionInfo{ + TaskID: "task-1", StepID: "review", ParticipantID: participantID, Decision: DecisionApproved, + }) + if err != nil { + t.Fatalf("RecordParticipantDecision: %v", err) + } + if !result.Transitioned { + t.Fatalf("expected the resolved reviewer decision to satisfy quorum and transition: %#v", result) + } + if result.FromStepID != "review" || result.ToStepID != "approval" { + t.Fatalf("unexpected transition endpoints: %#v", result) + } +} + +// TestResolveParticipantRole_NeitherSeatAtStep_ApproverWins is AC-4's +// fallback case: a caller holding both seats, with neither seat's StepID +// matching the step being queried, still resolves under approver-wins — +// the step-preference added for the Review re-entry bug only kicks in +// when the approver seat itself sits at the queried step. +func TestResolveParticipantRole_NeitherSeatAtStep_ApproverWins(t *testing.T) { + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "review-1", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "approval-1", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + eng := quorumEngine(nil, participants) + + role, participantID, err := eng.ResolveParticipantRole(context.Background(), "task-1", "review-2", "agent-a") + if err != nil { + t.Fatalf("ResolveParticipantRole: %v", err) + } + if role != "approver" || participantID != "seat-approver" { + t.Errorf("role/participantID = %q/%q, want approver/seat-approver", role, participantID) + } +}