From 35012bd96ecffeb9399dfab8a11bcd8425f40edc Mon Sep 17 00:00:00 2001 From: VenkataSakethDakuri <126963412+VenkataSakethDakuri@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:02:15 +0530 Subject: [PATCH] feat: enrich PR observation context (#2492) --- backend/internal/lifecycle/manager_test.go | 28 +++++++++++++ backend/internal/lifecycle/reactions.go | 47 ++++++++++++++++++++-- backend/internal/ports/pr_observations.go | 3 ++ 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/backend/internal/lifecycle/manager_test.go b/backend/internal/lifecycle/manager_test.go index aad971a15..8acb688dc 100644 --- a/backend/internal/lifecycle/manager_test.go +++ b/backend/internal/lifecycle/manager_test.go @@ -376,6 +376,34 @@ func TestPRObservation_MergeConflictNudgesAgent(t *testing.T) { } } +func TestPRObservation_NudgeIncludesPRIdentity(t *testing.T) { + m, st, msg := newManager() + st.sessions["mer-1"] = working("mer-1") + o := ports.PRObservation{ + Fetched: true, + URL: "https://github.com/o/r/pull/7", + Number: 7, + Title: "Add auth", + SourceBranch: "feat/x/auth", + TargetBranch: "feat/x", + CI: domain.CIFailing, + Checks: []ports.PRCheckObservation{{Name: "build", CommitHash: "c1", Status: domain.PRCheckFailed, LogTail: "boom"}}, + } + if err := m.ApplyPRObservation(ctx, "mer-1", o); err != nil { + t.Fatal(err) + } + if len(msg.msgs) != 1 { + t.Fatalf("want one CI nudge, got %d: %v", len(msg.msgs), msg.msgs) + } + got := msg.msgs[0] + if !strings.Contains(got, `PR #7 "Add auth" (feat/x/auth → feat/x)`) { + t.Fatalf("nudge missing PR identity: %q", got) + } + if !strings.Contains(got, "PR: https://github.com/o/r/pull/7") { + t.Fatalf("nudge missing PR URL: %q", got) + } +} + func TestPRObservation_MergedTerminatesWithoutNudge(t *testing.T) { m, st, msg := newManager() st.sessions["mer-1"] = working("mer-1") diff --git a/backend/internal/lifecycle/reactions.go b/backend/internal/lifecycle/reactions.go index 361dde1a5..7c506ccb0 100644 --- a/backend/internal/lifecycle/reactions.go +++ b/backend/internal/lifecycle/reactions.go @@ -157,12 +157,22 @@ func (m *Manager) ApplyPRObservation(ctx context.Context, id domain.SessionID, o if rec.IsTerminated || rec.Activity.State == domain.ActivityWaitingInput { return nil } - // Queue every applicable nudge so one PR condition cannot hide another. + // A single PR can trip several actionable conditions at once (failing CI, + // unresolved review comments, a merge conflict). Queue every applicable nudge + // and send them together, so each surfaces independently instead of one + // returning early and hiding the rest — the bug this reducer had, where a CI + // failure suppressed review feedback on the same PR. Each nudge self-dedups + // via sendOnce; a send error short-circuits, and nudges already sent have + // persisted their own dedup signature so the next poll retries only the rest. + ident := prIdentity(o) var nudges []pendingNudge if o.CI == domain.CIFailing { if ch, ok := firstFailedCheck(o.Checks); ok { - msg := "CI is failing on your PR. Review the output below and push a fix." + msg := "CI is failing on " + ident + ". Review the output below and push a fix." + if o.URL != "" { + msg += "\nPR: " + domain.SanitizeControlChars(o.URL) + } if ch.LogTail != "" { // LogTail is raw CI job output; sanitize before it reaches the // agent's live pane so embedded escape sequences can't drive the @@ -174,7 +184,10 @@ func (m *Manager) ApplyPRObservation(ctx context.Context, id domain.SessionID, o } if o.Review == domain.ReviewChangesRequest || hasUnresolvedComments(o.Comments) { comments, sig := reviewContent(o.Comments) - msg := "A reviewer left feedback on your PR. Address it and push." + msg := "A reviewer left feedback on " + ident + ". Address it and push." + if o.URL != "" { + msg += "\nPR: " + domain.SanitizeControlChars(o.URL) + } if comments != "" { msg += "\n\n" + comments } @@ -194,7 +207,11 @@ func (m *Manager) ApplyPRObservation(ctx context.Context, id domain.SessionID, o return err } if !blocked { - nudges = append(nudges, pendingNudge{key: "merge-conflict:" + o.URL, sig: string(o.Mergeability), msg: "Your PR has merge conflicts. Rebase onto the base branch and resolve them.", maxAttempts: 0}) + msg := "There are merge conflicts on " + ident + ". Rebase onto the base branch and resolve them." + if o.URL != "" { + msg += "\nPR: " + domain.SanitizeControlChars(o.URL) + } + nudges = append(nudges, pendingNudge{key: "merge-conflict:" + o.URL, sig: string(o.Mergeability), msg: msg, maxAttempts: 0}) } } @@ -386,6 +403,9 @@ func scmToPRObservation(o ports.SCMObservation) ports.PRObservation { Fetched: o.Fetched, URL: firstSCMNonEmpty(o.PR.URL, o.PR.HTMLURL), Number: o.PR.Number, + Title: o.PR.Title, + SourceBranch: o.PR.SourceBranch, + TargetBranch: o.PR.TargetBranch, Draft: o.PR.Draft, Merged: o.PR.Merged, Closed: o.PR.Closed, @@ -524,6 +544,25 @@ func firstSCMNonEmpty(a, b string) string { return b } +// prIdentity renders a short, sanitized PR identity ("PR #123 \"Title\" +// (feat/x → main)") for nudge messages so an agent in a multi-PR session can +// tell which PR — and where in a stack — a nudge refers to. Title and branch +// names are provider-controlled and reach the agent's live pane, so both are +// control-char sanitized. Falls back to "your PR" when the number is unknown. +func prIdentity(o ports.PRObservation) string { + if o.Number <= 0 { + return "your PR" + } + id := fmt.Sprintf("PR #%d", o.Number) + if o.Title != "" { + id += fmt.Sprintf(" %q", domain.SanitizeControlChars(o.Title)) + } + if o.SourceBranch != "" && o.TargetBranch != "" { + id += fmt.Sprintf(" (%s → %s)", domain.SanitizeControlChars(o.SourceBranch), domain.SanitizeControlChars(o.TargetBranch)) + } + return id +} + // firstFailedCheck returns the first check in a failed state, preserving the // original CI-nudge behavior of surfacing a single failing check. Extracting it // lets the CI branch queue its nudge and fall through instead of returning from diff --git a/backend/internal/ports/pr_observations.go b/backend/internal/ports/pr_observations.go index eb29615f7..0be0e759f 100644 --- a/backend/internal/ports/pr_observations.go +++ b/backend/internal/ports/pr_observations.go @@ -24,6 +24,9 @@ type PRObservation struct { Fetched bool URL string Number int + Title string + SourceBranch string + TargetBranch string Draft bool Merged bool Closed bool