fix: handle SCM observer seam facts

This commit is contained in:
harshitsinghbhandari 2026-05-27 22:19:07 +05:30
parent bbaf6650e3
commit f0766ebd8f
12 changed files with 136 additions and 14 deletions

View File

@ -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

View File

@ -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},

View File

@ -57,6 +57,8 @@ type OpenPRInput struct {
Draft bool
CIFailing bool
ChangesRequested bool
BotComments bool
MergeConflicts bool
Approved bool
Mergeable bool
ReviewPending bool

View File

@ -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"

View File

@ -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

View File

@ -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{

View File

@ -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.

View File

@ -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)

View File

@ -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},
}

View File

@ -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

View File

@ -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))

View File

@ -24,6 +24,7 @@ type SCMFacts struct {
Fetched bool
ObservedAt time.Time
PRState domain.PRState
Draft bool
PRNumber int
PRURL string
CISummary CISummary