From f0766ebd8f6991c2522fa6050bf2abcd6ceb871a Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Wed, 27 May 2026 22:19:07 +0530 Subject: [PATCH] fix: handle SCM observer seam facts --- backend/internal/domain/decide/decide.go | 4 ++ backend/internal/domain/decide/decide_test.go | 16 ++++++ backend/internal/domain/decide/types.go | 2 + backend/internal/domain/lifecycle.go | 2 + backend/internal/domain/status.go | 2 +- backend/internal/domain/status_test.go | 16 ++++++ backend/internal/lifecycle/decide_bridge.go | 21 ++++++- backend/internal/lifecycle/manager.go | 10 +--- backend/internal/lifecycle/manager_test.go | 5 ++ backend/internal/lifecycle/reactions.go | 14 ++++- backend/internal/lifecycle/reactions_test.go | 57 +++++++++++++++++++ backend/internal/ports/facts.go | 1 + 12 files changed, 136 insertions(+), 14 deletions(-) diff --git a/backend/internal/domain/decide/decide.go b/backend/internal/domain/decide/decide.go index 583b80d04..c46df18d5 100644 --- a/backend/internal/domain/decide/decide.go +++ b/backend/internal/domain/decide/decide.go @@ -127,6 +127,10 @@ func ResolveOpenPRDecision(in OpenPRInput) LifecycleDecision { return base(domain.StatusDraft, "draft", domain.PRReasonInProgress, domain.SessionWorking, domain.ReasonPRCreated) case in.ChangesRequested: return base(domain.StatusChangesRequested, "changes_requested", domain.PRReasonChangesRequested, domain.SessionWorking, domain.ReasonResolvingReviewComments) + case in.BotComments: + return base(domain.StatusChangesRequested, "bot_comments", domain.PRReasonBotComments, domain.SessionWorking, domain.ReasonResolvingReviewComments) + case in.MergeConflicts: + return base(domain.StatusPROpen, "merge_conflicts", domain.PRReasonMergeConflicts, domain.SessionWorking, domain.ReasonPRCreated) case in.Mergeable: // Mergeability is the authoritative merge gate, so it already folds in // "approved if review is required". Checking it before Approved means a diff --git a/backend/internal/domain/decide/decide_test.go b/backend/internal/domain/decide/decide_test.go index 9af6e596c..1a8159592 100644 --- a/backend/internal/domain/decide/decide_test.go +++ b/backend/internal/domain/decide/decide_test.go @@ -192,6 +192,20 @@ func TestResolveOpenPRDecision(t *testing.T) { wantPR: domain.PRReasonChangesRequested, wantState: domain.SessionWorking, }, + { + name: "bot comments get distinct PR reason", + in: OpenPRInput{BotComments: true, Approved: true, Mergeable: true}, + wantStatus: domain.StatusChangesRequested, + wantPR: domain.PRReasonBotComments, + wantState: domain.SessionWorking, + }, + { + name: "merge conflicts get distinct PR reason", + in: OpenPRInput{MergeConflicts: true, Approved: true}, + wantStatus: domain.StatusPROpen, + wantPR: domain.PRReasonMergeConflicts, + wantState: domain.SessionWorking, + }, { name: "approved + mergeable -> mergeable", in: OpenPRInput{Approved: true, Mergeable: true}, @@ -312,6 +326,8 @@ func TestDecidersDeriveConsistently(t *testing.T) { {Draft: true, ChangesRequested: true, Approved: true, Mergeable: true, ReviewPending: true, IdleBeyond: true}, {CIFailing: true}, {ChangesRequested: true}, + {BotComments: true}, + {MergeConflicts: true}, {Approved: true, Mergeable: true}, {Mergeable: true}, {Approved: true}, diff --git a/backend/internal/domain/decide/types.go b/backend/internal/domain/decide/types.go index e4fa92cab..0a10691ff 100644 --- a/backend/internal/domain/decide/types.go +++ b/backend/internal/domain/decide/types.go @@ -57,6 +57,8 @@ type OpenPRInput struct { Draft bool CIFailing bool ChangesRequested bool + BotComments bool + MergeConflicts bool Approved bool Mergeable bool ReviewPending bool diff --git a/backend/internal/domain/lifecycle.go b/backend/internal/domain/lifecycle.go index 2df4bb13a..bdb26f294 100644 --- a/backend/internal/domain/lifecycle.go +++ b/backend/internal/domain/lifecycle.go @@ -104,6 +104,8 @@ const ( PRReasonCIFailing PRReason = "ci_failing" PRReasonReviewPending PRReason = "review_pending" PRReasonChangesRequested PRReason = "changes_requested" + PRReasonBotComments PRReason = "bot_comments" + PRReasonMergeConflicts PRReason = "merge_conflicts" PRReasonApproved PRReason = "approved" PRReasonMergeReady PRReason = "merge_ready" PRReasonMerged PRReason = "merged" diff --git a/backend/internal/domain/status.go b/backend/internal/domain/status.go index aff5ba1f1..1cc4404d3 100644 --- a/backend/internal/domain/status.go +++ b/backend/internal/domain/status.go @@ -99,7 +99,7 @@ func openPRStatus(r PRReason) SessionStatus { switch r { case PRReasonCIFailing: return StatusCIFailed - case PRReasonChangesRequested: + case PRReasonChangesRequested, PRReasonBotComments: return StatusChangesRequested case PRReasonApproved: return StatusApproved diff --git a/backend/internal/domain/status_test.go b/backend/internal/domain/status_test.go index dc2c96e77..098549984 100644 --- a/backend/internal/domain/status_test.go +++ b/backend/internal/domain/status_test.go @@ -65,6 +65,22 @@ func TestDeriveLegacyStatus(t *testing.T) { }, want: StatusDraft, }, + { + name: "open PR bot comments display as changes_requested", + in: CanonicalSessionLifecycle{ + Session: SessionSubstate{State: SessionWorking}, + PR: PRSubstate{State: PROpen, Reason: PRReasonBotComments}, + }, + want: StatusChangesRequested, + }, + { + name: "open PR merge conflicts display as plain open", + in: CanonicalSessionLifecycle{ + Session: SessionSubstate{State: SessionWorking}, + PR: PRSubstate{State: PROpen, Reason: PRReasonMergeConflicts}, + }, + want: StatusPROpen, + }, { name: "open PR approved", in: CanonicalSessionLifecycle{ diff --git a/backend/internal/lifecycle/decide_bridge.go b/backend/internal/lifecycle/decide_bridge.go index 942fdad41..501d12ac7 100644 --- a/backend/internal/lifecycle/decide_bridge.go +++ b/backend/internal/lifecycle/decide_bridge.go @@ -101,9 +101,13 @@ func hasRecentActivity(a domain.ActivitySubstate, now time.Time, window time.Dur // in split A — the idle-duration signal is owned by the escalation engine // (split B); the synchronous LCM has no clock of its own here. func openPRInput(f ports.SCMFacts) decide.OpenPRInput { + hasBotComments, hasHumanComments := classifyPendingComments(f.PendingComments) return decide.OpenPRInput{ + Draft: f.PRState == domain.PRDraft || f.Draft, CIFailing: f.CISummary == ports.CIFailing, - ChangesRequested: f.ReviewDecision == ports.ReviewChangesRequested, + ChangesRequested: f.ReviewDecision == ports.ReviewChangesRequested || hasHumanComments, + BotComments: hasBotComments, + MergeConflicts: hasMergeConflicts(f.Mergeability), Approved: f.ReviewDecision == ports.ReviewApproved, Mergeable: f.Mergeability.Mergeable, ReviewPending: f.ReviewDecision == ports.ReviewPending, @@ -112,6 +116,21 @@ func openPRInput(f ports.SCMFacts) decide.OpenPRInput { } } +func classifyPendingComments(comments []ports.ReviewComment) (hasBot, hasHuman bool) { + for _, c := range comments { + if c.IsBot { + hasBot = true + } else { + hasHuman = true + } + } + return hasBot, hasHuman +} + +func hasMergeConflicts(m ports.Mergeability) bool { + return !m.Mergeable && !m.NoConflicts && (m.CIPassing || m.Approved || len(m.Blockers) > 0) +} + // ---- activity -> session axis mapping (activity owns working/idle/waiting) ---- // activityToSession maps an activity classification onto the session sub-state. diff --git a/backend/internal/lifecycle/manager.go b/backend/internal/lifecycle/manager.go index 86c36b84c..f7b99ea6d 100644 --- a/backend/internal/lifecycle/manager.go +++ b/backend/internal/lifecycle/manager.go @@ -206,15 +206,7 @@ func (m *Manager) ApplySCMObservation(ctx context.Context, id domain.SessionID, } switch f.PRState { - case domain.PRDraft: - in := openPRInput(f) - in.Draft = true - d := decide.ResolveOpenPRDecision(in) - var patch ports.LifecyclePatch - changed := setPRIfChanged(&patch, cur, d, f) - return patch, changed, nil - - case domain.PROpen: + case domain.PRDraft, domain.PROpen: d := decide.ResolveOpenPRDecision(openPRInput(f)) var patch ports.LifecyclePatch changed := setPRIfChanged(&patch, cur, d, f) diff --git a/backend/internal/lifecycle/manager_test.go b/backend/internal/lifecycle/manager_test.go index 471e845ba..a1d5b6354 100644 --- a/backend/internal/lifecycle/manager_test.go +++ b/backend/internal/lifecycle/manager_test.go @@ -271,6 +271,8 @@ func TestApplySCMObservation(t *testing.T) { wantStatus domain.SessionStatus }{ {"draft with failing CI", ports.SCMFacts{Fetched: true, PRState: domain.PRDraft, CISummary: ports.CIFailing}, domain.PRReasonCIFailing, domain.StatusCIFailed}, + {"draft via bool with open state", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, Draft: true}, domain.PRReasonInProgress, domain.StatusDraft}, + {"draft via bool with failing CI", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, Draft: true, CISummary: ports.CIFailing}, domain.PRReasonCIFailing, domain.StatusCIFailed}, {"draft ignores review and merge facts", ports.SCMFacts{Fetched: true, PRState: domain.PRDraft, ReviewDecision: ports.ReviewApproved, Mergeability: ports.Mergeability{Mergeable: true}}, domain.PRReasonInProgress, domain.StatusDraft}, } for _, c := range cases { @@ -321,6 +323,9 @@ func TestApplySCMObservation(t *testing.T) { wantStatus domain.SessionStatus }{ {"changes requested", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, ReviewDecision: ports.ReviewChangesRequested}, domain.PRReasonChangesRequested, domain.StatusChangesRequested}, + {"pending human comments", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, PendingComments: []ports.ReviewComment{{Author: "human", Body: "fix"}}}, domain.PRReasonChangesRequested, domain.StatusChangesRequested}, + {"pending bot comments", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, PendingComments: []ports.ReviewComment{{Author: "bot", Body: "fix", IsBot: true}}}, domain.PRReasonBotComments, domain.StatusChangesRequested}, + {"merge conflicts", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, Mergeability: ports.Mergeability{CIPassing: true, Approved: true, NoConflicts: false, Blockers: []string{"merge conflicts"}}}, domain.PRReasonMergeConflicts, domain.StatusPROpen}, {"approved + mergeable", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, ReviewDecision: ports.ReviewApproved, Mergeability: ports.Mergeability{Mergeable: true}}, domain.PRReasonMergeReady, domain.StatusMergeable}, {"review pending", ports.SCMFacts{Fetched: true, PRState: domain.PROpen, ReviewDecision: ports.ReviewPending}, domain.PRReasonReviewPending, domain.StatusReviewPending}, } diff --git a/backend/internal/lifecycle/reactions.go b/backend/internal/lifecycle/reactions.go index fc0921136..761ac4a4a 100644 --- a/backend/internal/lifecycle/reactions.go +++ b/backend/internal/lifecycle/reactions.go @@ -135,13 +135,21 @@ var defaultReactions = map[reactionKey]reactionConfig{ // current state has no reaction. // // A closed PR derives to the idle display status, so it is detected from the PR -// axis directly before falling through to the status mapping. bugbot-comments -// and merge-conflicts have no producer in the split-A decide core yet, so they -// are dormant: configured but unreachable until DECIDE surfaces them. +// axis directly before falling through to the status mapping. Bot review +// comments and merge conflicts are represented as PR reasons so the ACT layer +// can distinguish them from human-requested changes and plain open PRs. func reactionEventFor(l domain.CanonicalSessionLifecycle) (reactionKey, bool) { if l.PR.State == domain.PRClosed { return reactionPRClosed, true } + if isActivePRState(l.PR.State) { + switch l.PR.Reason { + case domain.PRReasonBotComments: + return reactionBugbotComments, true + case domain.PRReasonMergeConflicts: + return reactionMergeConflicts, true + } + } switch domain.DeriveLegacyStatus(l) { case domain.StatusCIFailed: return reactionCIFailed, true diff --git a/backend/internal/lifecycle/reactions_test.go b/backend/internal/lifecycle/reactions_test.go index d0635d4dc..942bc339b 100644 --- a/backend/internal/lifecycle/reactions_test.go +++ b/backend/internal/lifecycle/reactions_test.go @@ -78,6 +78,63 @@ func TestReaction_CIFailedSendsToAgentWithLogTail(t *testing.T) { } } +func TestReaction_BotAndHumanCommentsRouteSeparately(t *testing.T) { + tests := []struct { + name string + comments []ports.ReviewComment + wantMessage string + }{ + { + name: "bot comments -> bugbot-comments", + comments: []ports.ReviewComment{{Author: "bugbot", Body: "fix", IsBot: true}}, + wantMessage: "automated reviewer", + }, + { + name: "human comments -> changes-requested", + comments: []ports.ReviewComment{{Author: "reviewer", Body: "fix"}}, + wantMessage: "reviewer requested changes", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m, store, _, msgr := newReactive() + store.seed(sid, lcOpenPR(domain.PRReasonReviewPending)) + + if err := m.ApplySCMObservation(ctx(), sid, ports.SCMFacts{ + Fetched: true, PRState: domain.PROpen, PendingComments: tt.comments, PRNumber: 7, + }); err != nil { + t.Fatalf("apply: %v", err) + } + + if len(msgr.sent) != 1 { + t.Fatalf("want one send, got %d", len(msgr.sent)) + } + if !strings.Contains(msgr.sent[0].Message, tt.wantMessage) { + t.Errorf("message %q does not contain %q", msgr.sent[0].Message, tt.wantMessage) + } + }) + } +} + +func TestReaction_MergeConflictsSendsToAgent(t *testing.T) { + m, store, _, msgr := newReactive() + store.seed(sid, lcOpenPR(domain.PRReasonReviewPending)) + + if err := m.ApplySCMObservation(ctx(), sid, ports.SCMFacts{ + Fetched: true, PRState: domain.PROpen, PRNumber: 7, + Mergeability: ports.Mergeability{CIPassing: true, Approved: true, NoConflicts: false, Blockers: []string{"merge conflicts"}}, + }); err != nil { + t.Fatalf("apply: %v", err) + } + + if len(msgr.sent) != 1 { + t.Fatalf("want one send, got %d", len(msgr.sent)) + } + if !strings.Contains(msgr.sent[0].Message, "merge conflicts") { + t.Errorf("message = %q, want merge conflict nudge", msgr.sent[0].Message) + } +} + func TestReaction_ApprovedAndGreenNotifiesNeverAutoMerges(t *testing.T) { m, store, notf, msgr := newReactive() store.seed(sid, lcOpenPR(domain.PRReasonReviewPending)) diff --git a/backend/internal/ports/facts.go b/backend/internal/ports/facts.go index 55f4f6ca6..f1b0c702e 100644 --- a/backend/internal/ports/facts.go +++ b/backend/internal/ports/facts.go @@ -24,6 +24,7 @@ type SCMFacts struct { Fetched bool ObservedAt time.Time PRState domain.PRState + Draft bool PRNumber int PRURL string CISummary CISummary