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 <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -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
|
||||
}
|
||||
|
||||
@@ -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},
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user