From 7cb5d8c0b3d2c1bd49c6ef3720d87365031e7e2c Mon Sep 17 00:00:00 2001 From: Henry Date: Sun, 6 Sep 2026 22:10:53 +0800 Subject: [PATCH] fix(backend): break approver-first tie when neither quorum seat sits at the current step (#3442) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../internal/workflow/engine/quorum.go | 45 +++++- .../workflow/engine/quorum_role_test.go | 150 +++++++++++++++++- ...quorum-decision-recording-agent-surface.md | 17 +- .../workflow-quorum-decision-recording.md | 13 ++ 4 files changed, 211 insertions(+), 14 deletions(-) diff --git a/apps/backend/internal/workflow/engine/quorum.go b/apps/backend/internal/workflow/engine/quorum.go index e638e5754f..1b976ba6cc 100644 --- a/apps/backend/internal/workflow/engine/quorum.go +++ b/apps/backend/internal/workflow/engine/quorum.go @@ -904,10 +904,13 @@ func (e *Engine) evaluateGuardStateReadOnly( // participates, per AC-57's "exactly once, in the engine" mandate. // 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. +// decisions against. When neither seat sits at stepID, the role named by +// stepID's own wait_for_quorum guard wins, so a decision is never filed +// under a role no guard at that step is counting. Approver wins as the +// final tiebreak — when the step names both roles, names neither, or the +// step cannot be loaded — matching the precedence in +// office/dashboard/decisions.go's resolveDeciderRole (AC-4). Returns +// ErrParticipantNotFound when the agent occupies neither seat. func (e *Engine) ResolveParticipantRole( ctx context.Context, taskID, stepID, agentProfileID string, ) (role, participantID string, err error) { @@ -945,9 +948,43 @@ func (e *Engine) ResolveParticipantRole( if len(matches) == 0 { return "", "", ErrParticipantNotFound } + if guardRoles := e.guardRolesAtStep(ctx, workflowID, stepID); len(guardRoles) == 1 { + for _, m := range matches { + if guardRoles[m.role] { + return m.role, m.seat.ID, nil + } + } + } return matches[0].role, matches[0].seat.ID, nil } +// guardRolesAtStep returns the set of roles named by wait_for_quorum guards +// on stepID's eligible on_turn_complete transitions, used to break a +// cross-step role resolution tie in favor of the role the step is waiting on. +// Returns an empty set when the step names no such guard or cannot be +// loaded, so callers fall back to approver-wins rather than treating a load +// failure as a role match. +func (e *Engine) guardRolesAtStep(ctx context.Context, workflowID, stepID string) map[string]bool { + if workflowID == "" { + return nil + } + step, err := e.store.LoadStep(ctx, workflowID, stepID) + if err != nil { + return nil + } + roles := make(map[string]bool) + for _, action := range step.Events[TriggerOnTurnComplete] { + if !isTransitionAction(action.Kind) || action.RequiresApproval { + continue + } + if action.Guard == nil || action.Guard.WaitForQuorum == nil || action.Guard.WaitForQuorum.Role == "" { + continue + } + roles[action.Guard.WaitForQuorum.Role] = true + } + return roles +} + // roleSeat pairs a resolved role with the seat matched under it. type roleSeat struct { role string diff --git a/apps/backend/internal/workflow/engine/quorum_role_test.go b/apps/backend/internal/workflow/engine/quorum_role_test.go index dc96eac573..56f32076ba 100644 --- a/apps/backend/internal/workflow/engine/quorum_role_test.go +++ b/apps/backend/internal/workflow/engine/quorum_role_test.go @@ -197,10 +197,12 @@ func TestResolveParticipantRole_StepPreferredTransitionFires(t *testing.T) { } // 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. +// no-guard-at-step fallback: a caller holding both seats, with neither +// seat's StepID matching the step being queried and that step naming no +// wait_for_quorum guard (quorumEngine's step has a nil Guard), still +// resolves under approver-wins — the step-preference early return applies +// when either seat sits at the queried step, and the guard-role tiebreak only +// kicks in when the queried step names exactly one guard role. 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}, @@ -216,3 +218,143 @@ func TestResolveParticipantRole_NeitherSeatAtStep_ApproverWins(t *testing.T) { t.Errorf("role/participantID = %q/%q, want approver/seat-approver", role, participantID) } } + +// TestResolveParticipantRole_BothSeatsAtSameNonCurrentStep_GuardRoleWins is +// the regression test for the thin-workspace Review deadlock: both seats +// were cast at Backlog (before the task ever reached Review), so neither +// sits at the step being evaluated and the old approver-first cross-step +// scan always won. The step actually being evaluated (Review) has a +// reviewer wait_for_quorum guard, so resolution must prefer reviewer, and +// recording the decision under the resolved seat must satisfy that guard +// and fire Review -> Approval instead of parking forever. +func TestResolveParticipantRole_BothSeatsAtSameNonCurrentStep_GuardRoleWins(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: "backlog", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "backlog", 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_IgnoresApprovalRequiredGuard keeps role +// resolution aligned with automatic transition evaluation: an approval-only +// action is not a guard that the current step can satisfy automatically. +func TestResolveParticipantRole_IgnoresApprovalRequiredGuard(t *testing.T) { + store := &stepStoreForQuorum{ + state: MachineState{TaskID: "task-1", SessionID: "sess-1", WorkflowID: "wf", CurrentStepID: "review"}, + step: StepSpec{ + ID: "review", WorkflowID: "wf", Position: 1, + Events: map[Trigger][]Action{ + TriggerOnTurnComplete: { + {Kind: ActionMoveToNext, RequiresApproval: true, Guard: &TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "approver", Threshold: QuorumAllApprove}, + }}, + {Kind: ActionMoveToNext, Guard: &TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "reviewer", Threshold: QuorumAllApprove}, + }}, + }, + }, + }, + next: StepSpec{ID: "approval", Position: 2}, + applied: map[string]bool{}, + } + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "backlog", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "backlog", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + eng := New(store, MapRegistry{}, 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.Errorf("role/participantID = %q/%q, want reviewer/seat-reviewer", role, participantID) + } +} + +// TestResolveParticipantRole_BothSeatsAtDifferentNonCurrentSteps_GuardRoleWins +// documents the AC-4 behavior for seats attached to two different earlier +// steps. The implementation already applies the guard-role tie-break here. +func TestResolveParticipantRole_BothSeatsAtDifferentNonCurrentSteps_GuardRoleWins(t *testing.T) { + store := quorumStore(&TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "reviewer", Threshold: QuorumAllApprove}, + }) + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "backlog", 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{}, 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.Errorf("role/participantID = %q/%q, want reviewer/seat-reviewer", role, participantID) + } +} + +// TestResolveParticipantRole_BothSeatsAtSameNonCurrentStep_StepNamesBothRoles_ApproverWins +// covers AC-4's third fallback branch: the queried step names *two* guard +// roles (reviewer and approver each gated separately), so the single-role +// tiebreak in ResolveParticipantRole cannot apply and resolution must fall +// through to the final approver-wins line — distinct from the sibling test +// above, where the step names no guard role at all. +func TestResolveParticipantRole_BothSeatsAtSameNonCurrentStep_StepNamesBothRoles_ApproverWins(t *testing.T) { + store := &stepStoreForQuorum{ + state: MachineState{TaskID: "task-1", SessionID: "sess-1", WorkflowID: "wf", CurrentStepID: "review"}, + step: StepSpec{ + ID: "review", WorkflowID: "wf", Position: 1, + Events: map[Trigger][]Action{ + TriggerOnTurnComplete: { + {Kind: ActionMoveToNext, Guard: &TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "reviewer", Threshold: QuorumAllApprove}, + }}, + {Kind: ActionMoveToNext, Guard: &TransitionGuard{ + WaitForQuorum: &WaitForQuorumGuard{Role: "approver", Threshold: QuorumAllApprove}, + }}, + }, + }, + }, + next: StepSpec{ID: "approval", Position: 2}, + applied: map[string]bool{}, + } + participants := scopedParticipants{perTask: []ParticipantInfo{ + {ID: "seat-reviewer", TaskID: "task-1", StepID: "backlog", Role: "reviewer", AgentProfileID: "agent-a", DecisionRequired: true}, + {ID: "seat-approver", TaskID: "task-1", StepID: "backlog", Role: "approver", AgentProfileID: "agent-a", DecisionRequired: true}, + }} + eng := New(store, MapRegistry{}, WithParticipantStore(participants)) + + 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) + } +} diff --git a/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md b/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md index 5adefbd037..e4920c7f32 100644 --- a/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md +++ b/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md @@ -49,11 +49,16 @@ An Office participant can submit a validated verdict with a reason, and the resu the call with a permission error and write no row. - **AC-TASKS-QUORUM-RECORDING-001.4:** WHEN the caller holds both `reviewer` and `approver` seats at the task's current `workflow_step_id`, THE SYSTEM SHALL record the decision - under `approver`. WHEN the caller holds those two roles at different steps, - THE SYSTEM SHALL record the decision under the role whose seat sits at the - task's current `workflow_step_id` (which is guaranteed to be one of the two - roles by the precondition of AC-TASKS-QUORUM-RECORDING-001.1), and SHALL NOT - apply approver-wins across steps. + under `approver`. WHEN exactly one of those seats sits at the task's current + `workflow_step_id`, THE SYSTEM SHALL record the decision under that seat's + role and SHALL NOT apply approver-wins across steps. WHEN neither seat sits + at the task's current `workflow_step_id`, whether both seats share one + earlier step or the seats occupy different earlier steps, THE SYSTEM SHALL + record the decision under the role named by the current step's own + eligible automatic `on_turn_complete` `wait_for_quorum` guard, when that + step names exactly one role; only WHEN the current step names both roles, + names neither, or cannot be resolved SHALL approver-wins apply as the final + tiebreak. - **AC-TASKS-QUORUM-RECORDING-001.4a:** THE SYSTEM SHALL apply AC-TASKS-QUORUM-RECORDING-001.4 to the agent decision surface only. The human decision path resolves the role step-blind and retains unconditional approver-wins, so the @@ -110,4 +115,4 @@ An Office participant can submit a validated verdict with a reason, and the resu -Behavior outside this acceptance-criteria group is defined by the core quorum requirement and the other quorum capability requirements. \ No newline at end of file +Behavior outside this acceptance-criteria group is defined by the core quorum requirement and the other quorum capability requirements. diff --git a/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md b/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md index fc45724e90..cb0396f798 100644 --- a/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md +++ b/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md @@ -245,6 +245,19 @@ counted by that guard at all, so a single pair of counts is either ambiguous or actively misleading. `guards` gives the agent the count against each guard that exists, which is the question it was actually asking. +A third case reaches the same cross-step scan without either seat sitting at +the current step at all: both `AddTaskReviewer`/`AddTaskApprover` seats are +cast while the task sits on an earlier step (e.g. Backlog), so by the time +the task reaches Review neither seat's `StepID` matches `review`. The seats +can share that earlier step or come from two different earlier steps. Here +`ResolveParticipantRole` reads the role named by Review's own eligible +`wait_for_quorum` guard and prefers the matching seat — `reviewer`, in the +thin-workspace Review case — over the fixed approver-first order. This +guard-role tiebreak only applies when the current step names exactly one +role; a step naming both roles, naming neither, or that fails to load falls +back to approver-wins unchanged, so this case narrows rather than replaces +the existing precedence. + This precedence applies only to the agent decision surface. The human decision path retains its existing `resolveDeciderRole` behavior and unconditional approver precedence, as required by AC-TASKS-QUORUM-RECORDING-001.4a. The two