dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix(backend): break approver-first tie when neither quorum seat sits at the current step (#3442)

Browse Source 
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
2026-09-06 22:10:53 +08:00
committed by GitHub
parent 177b5596b5
commit 7cb5d8c0b3
4 changed files with 211 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/workflow/engine/quorum.go
+41 -4
View File
@@ -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
apps/backend/internal/workflow/engine/quorum_role_test.go
+146 -4
View File
@@ -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)
}
}
docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md
+10 -5
View File
@@ -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
docs/specs/tasks/system-design/workflow-quorum-decision-recording.md
+13
View File
@@ -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