fix(github): keep draft pull requests out of merge-ready state (#1851)

This commit is contained in:
Carlos Florêncio
2026-07-21 23:22:49 +01:00
committed by GitHub
parent c645885cb5
commit 5499eadf19
8 changed files with 190 additions and 35 deletions
@@ -530,23 +530,14 @@ func (s *Service) findTaskPRForStatus(ctx context.Context, taskID string, pr *PR
return nil, nil
}
// SyncTaskPR updates a TaskPR record with the latest PR status. Multi-repo:
// the row is found by (task_id, owner, repo, pr_number) since the same
// task can have several PRs; the legacy GetTaskPR(taskID) "first match"
// would cross repos and silently update the wrong row.
// It only publishes a github.task_pr.updated event when data actually changed,
// preventing feedback loops with frontend sync handlers.
//
//nolint:cyclop // sequential field-by-field "populated/preserve" reconciliation; splitting helpers hides intent
func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error {
if status == nil || status.PR == nil {
return fmt.Errorf("sync task PR: missing PR data for task %s", taskID)
}
tp, err := s.findTaskPRForStatus(ctx, taskID, status.PR)
if err != nil || tp == nil {
return err
}
type taskPRSyncState struct {
checksTotal, checksPassing int
unresolved, reviewCount, pendingReviewCount int
requiredReviews *int
baseBranch, mergeableState string
}
func (s *Service) prepareTaskPRSyncState(ctx context.Context, tp *TaskPR, status *PRStatus) taskPRSyncState {
// Some sync paths (notably the batched GraphQL poller) don't populate
// ChecksTotal / ChecksPassing — they only carry the rollup state. The
// caller sets status.ChecksPopulated=true when it actually counted
@@ -591,6 +582,37 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
} else if fetched := s.fetchRequiredReviews(ctx, tp.Owner, tp.Repo, nextBaseBranch); fetched != nil {
nextRequiredReviews = fetched
}
// GitHub reports draft status separately from mergeStateStatus and can
// return CLEAN for a draft PR. Persist the effective blocker so every
// TaskPR consumer agrees that drafts are not ready to merge.
nextMergeableState := status.MergeableState
if status.PR.Draft {
nextMergeableState = "draft"
}
return taskPRSyncState{
checksTotal: nextChecksTotal, checksPassing: nextChecksPassing,
unresolved: nextUnresolved, reviewCount: nextReviewCount, pendingReviewCount: nextPendingReviewCount,
requiredReviews: nextRequiredReviews, baseBranch: nextBaseBranch, mergeableState: nextMergeableState,
}
}
// SyncTaskPR updates a TaskPR record with the latest PR status. Multi-repo:
// the row is found by (task_id, owner, repo, pr_number) since the same
// task can have several PRs; the legacy GetTaskPR(taskID) "first match"
// would cross repos and silently update the wrong row.
// It only publishes a github.task_pr.updated event when data actually changed,
// preventing feedback loops with frontend sync handlers.
//
//nolint:cyclop // sequential field-by-field reconciliation keeps update intent clear
func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error {
if status == nil || status.PR == nil {
return fmt.Errorf("sync task PR: missing PR data for task %s", taskID)
}
tp, err := s.findTaskPRForStatus(ctx, taskID, status.PR)
if err != nil || tp == nil {
return err
}
next := s.prepareTaskPRSyncState(ctx, tp, status)
changed := tp.State != status.PR.State ||
tp.PRTitle != status.PR.Title ||
@@ -598,14 +620,14 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
tp.Deletions != status.PR.Deletions ||
tp.ReviewState != status.ReviewState ||
tp.ChecksState != status.ChecksState ||
tp.MergeableState != status.MergeableState ||
tp.ReviewCount != nextReviewCount ||
tp.PendingReviewCount != nextPendingReviewCount ||
!intPtrEqual(tp.RequiredReviews, nextRequiredReviews) ||
tp.ChecksTotal != nextChecksTotal ||
tp.ChecksPassing != nextChecksPassing ||
tp.UnresolvedReviewThreads != nextUnresolved ||
tp.BaseBranch != nextBaseBranch ||
tp.MergeableState != next.mergeableState ||
tp.ReviewCount != next.reviewCount ||
tp.PendingReviewCount != next.pendingReviewCount ||
!intPtrEqual(tp.RequiredReviews, next.requiredReviews) ||
tp.ChecksTotal != next.checksTotal ||
tp.ChecksPassing != next.checksPassing ||
tp.UnresolvedReviewThreads != next.unresolved ||
tp.BaseBranch != next.baseBranch ||
!timeEqual(tp.MergedAt, status.PR.MergedAt) ||
!timeEqual(tp.ClosedAt, status.PR.ClosedAt)
@@ -617,14 +639,14 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
tp.ClosedAt = status.PR.ClosedAt
tp.ReviewState = status.ReviewState
tp.ChecksState = status.ChecksState
tp.MergeableState = status.MergeableState
tp.ReviewCount = nextReviewCount
tp.PendingReviewCount = nextPendingReviewCount
tp.RequiredReviews = nextRequiredReviews
tp.ChecksTotal = nextChecksTotal
tp.ChecksPassing = nextChecksPassing
tp.UnresolvedReviewThreads = nextUnresolved
tp.BaseBranch = nextBaseBranch
tp.MergeableState = next.mergeableState
tp.ReviewCount = next.reviewCount
tp.PendingReviewCount = next.pendingReviewCount
tp.RequiredReviews = next.requiredReviews
tp.ChecksTotal = next.checksTotal
tp.ChecksPassing = next.checksPassing
tp.UnresolvedReviewThreads = next.unresolved
tp.BaseBranch = next.baseBranch
// CommentCount is no longer updated from polling -- only refreshed on-demand
now := time.Now().UTC()
tp.LastSyncedAt = &now
@@ -927,6 +927,56 @@ func TestSyncTaskPR_PublishesEventOnMergeableStateChange(t *testing.T) {
}
}
func TestSyncTaskPR_DraftOverridesCleanMergeableState(t *testing.T) {
svc, store, eb := setupSyncTest(t)
ctx := context.Background()
if err := store.CreateTaskPR(ctx, &TaskPR{
TaskID: "t1",
Owner: "owner",
Repo: "repo",
PRNumber: 1,
PRURL: "https://github.com/owner/repo/pull/1",
PRTitle: "Draft change",
HeadBranch: "feat",
BaseBranch: "main",
State: "open",
ChecksState: "success",
MergeableState: "clean",
}); err != nil {
t.Fatalf("create task PR: %v", err)
}
status := &PRStatus{
PR: &PR{
Number: 1,
Title: "Draft change",
State: "open",
Draft: true,
RepoOwner: "owner",
RepoName: "repo",
MergeableState: "clean",
},
ChecksState: "success",
MergeableState: "clean",
}
if err := svc.SyncTaskPR(ctx, "t1", status); err != nil {
t.Fatalf("sync: %v", err)
}
if got := eb.publishedCount(); got != 1 {
t.Fatalf("expected draft normalization to publish an update, got %d events", got)
}
stored, err := store.GetTaskPR(ctx, "t1")
if err != nil {
t.Fatalf("get task PR: %v", err)
}
if stored.MergeableState != "draft" {
t.Fatalf("stored mergeable_state = %q, want draft", stored.MergeableState)
}
}
// TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync verifies the
// "preserve when batch sync didn't populate" path: a PRStatus with
// ChecksPopulated=false must NOT clobber the persisted ChecksTotal /
@@ -286,6 +286,22 @@ describe("PRStatusChip mobile branch", () => {
expect(screen.getByTestId(CHIP_TESTID).getAttribute(ATTR_STATUS)).toBe("failed");
});
it("identifies a draft PR with passing CI as draft", () => {
renderWithStore(
{
taskPRs: {
byTaskId: {
"task-1": [makePR({ mergeable_state: "draft" })],
},
},
},
<PRStatusChip taskId="task-1" />,
);
const chip = screen.getByTestId(CHIP_TESTID);
expect(chip.getAttribute(ATTR_STATUS)).toBe("draft");
expect(chip.getAttribute(ATTR_READY_TO_MERGE)).toBe("false");
});
it("shows automation badges on the mobile chip trigger", () => {
renderWithStore(
{
@@ -31,6 +31,7 @@ import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover";
import {
hasPRChecksInProgressForDisplay,
hasPRChecksPassedForDisplay,
isPRDraft,
isPRAwaitingReview,
isPRReadyToMerge,
isPRWaitingOnBranchProtection,
@@ -52,6 +53,7 @@ type ChipStatus =
| "conflict"
| "blocked"
| "behind"
| "draft"
| "waiting"
| "in_progress"
| "neutral";
@@ -68,6 +70,7 @@ function chipStatus(pr: TaskPR): ChipStatus {
// getPRStatusColor + PRStatusIcon (dirty = red, behind = amber).
if (pr.mergeable_state === "dirty") return "conflict";
if (pr.mergeable_state === "behind") return "behind";
if (isPRDraft(pr)) return "draft";
// Pending checks / pending review must beat checks_state === "success" so a
// PR with all checks green but reviewers still outstanding renders as
// in-progress, not passed. Without this order, the chip flips to green the
@@ -92,6 +95,7 @@ const CHIP_STATUS_RANK: Record<ChipStatus, number> = {
conflict: 5,
blocked: 4,
behind: 3,
draft: 0.5,
in_progress: 2,
waiting: 1.5,
passed: 1,
@@ -555,6 +559,8 @@ function ChipStatusGlyph({ status }: { status: ChipStatus }) {
return <IconAlertTriangleFilled className="h-3.5 w-3.5 text-red-500" aria-hidden="true" />;
case "behind":
return <IconAlertTriangleFilled className="h-3.5 w-3.5 text-yellow-500" aria-hidden="true" />;
case "draft":
return <IconPointFilled className="h-3.5 w-3.5 text-muted-foreground" aria-hidden="true" />;
case "blocked":
return (
<IconShield
@@ -0,0 +1,47 @@
import { describe, expect, it } from "vitest";
import type { TaskPR } from "@/lib/types/github";
import { getPRStatusColor, getPRTooltip } from "./pr-task-icon";
function draftPR(): TaskPR {
return {
id: "id",
task_id: "task",
owner: "o",
repo: "r",
pr_number: 1,
pr_url: "",
pr_title: "Test PR",
head_branch: "feat",
base_branch: "main",
author_login: "alice",
state: "open",
review_state: "approved",
checks_state: "success",
mergeable_state: "draft",
review_count: 1,
pending_review_count: 0,
comment_count: 0,
unresolved_review_threads: 0,
checks_total: 1,
checks_passing: 1,
additions: 0,
deletions: 0,
created_at: "",
merged_at: null,
closed_at: null,
last_synced_at: null,
updated_at: "",
};
}
describe("draft PR task status", () => {
it("stays muted even when checks pass", () => {
expect(getPRStatusColor(draftPR())).toBe("text-muted-foreground");
});
it("identifies the draft without claiming it is ready to merge", () => {
const tooltip = getPRTooltip(draftPR());
expect(tooltip).toContain("Draft");
expect(tooltip).not.toContain("Ready to merge");
});
});
+10 -1
View File
@@ -62,6 +62,10 @@ export function isPRReadyToMerge(pr: TaskPR): boolean {
return pr.review_state === "" && pr.pending_review_count === 0;
}
export function isPRDraft(pr: TaskPR): boolean {
return pr.state === "open" && pr.mergeable_state === "draft";
}
// CI passed but the PR is still waiting on human review (reviewers requested
// or pending review state). Distinct from yellow "CI running". An approved
// PR with extra reviewers still pending also counts — GitHub's
@@ -104,6 +108,9 @@ export function getPRStatusColor(pr: TaskPR): string {
if (pr.review_state === "changes_requested" || pr.checks_state === "failure") {
return "text-red-500";
}
if (isPRDraft(pr)) {
return MUTED_FOREGROUND;
}
const blockerColor = openMergeBlockerColor(pr);
if (blockerColor) return blockerColor;
if (isPRReadyToMerge(pr)) {
@@ -133,7 +140,9 @@ export function getPRTooltip(pr: TaskPR): string {
if (pr.state !== "open") parts.push(`State: ${pr.state}`);
if (pr.review_state) parts.push(`Review: ${pr.review_state}`);
if (pr.checks_state) parts.push(`CI: ${pr.checks_state}`);
if (isPRReadyToMerge(pr)) {
if (isPRDraft(pr)) {
parts.push("Draft");
} else if (isPRReadyToMerge(pr)) {
parts.push("Ready to merge");
} else if (pr.mergeable_state && pr.mergeable_state !== "unknown" && pr.state === "open") {
parts.push(`Mergeable: ${pr.mergeable_state}`);
@@ -29,6 +29,7 @@ import {
getPRStatusColor,
hasPRChecksInProgressForDisplay,
hasPRChecksPassedWithoutReviewWaitForDisplay,
isPRDraft,
isPRAwaitingReview,
isPRReadyToMerge,
isPRWaitingOnBranchProtection,
@@ -70,6 +71,9 @@ function PRStatusIcon({ pr }: { pr: TaskPR }) {
if (pr.checks_state === "failure" || pr.review_state === "changes_requested") {
return <IconX className="h-3 w-3 text-red-500" />;
}
if (isPRDraft(pr)) {
return <IconGitPullRequest className="h-3 w-3 text-muted-foreground" />;
}
const blockerBadge = mergeBlockerBadge(pr);
if (blockerBadge) return blockerBadge;
if (isPRReadyToMerge(pr)) {
+3 -2
View File
@@ -18,7 +18,7 @@ Users can already see pull request CI/review status above the task chat input, b
- The automation section includes an info icon or equivalent help affordance that explains what each control watches, how often Kandev checks watched PRs, how feedback snapshots prevent duplicate prompts, and how auto-merge decides readiness.
- The same controls are available anywhere the task PR CI popover is rendered, including the normal chat input status bar and passthrough toolbar surfaces.
- `Auto-fix CI & address comments` causes Kandev to send or queue an agent prompt when a linked PR gets actionable CI or review feedback.
- `Auto-merge when ready` causes Kandev to merge a linked PR only when the PR is open, checks are passing, review requirements are satisfied, unresolved review threads are cleared, and the PR is cleanly mergeable.
- `Auto-merge when ready` causes Kandev to merge a linked PR only when the PR is open and not a draft, checks are passing, review requirements are satisfied, unresolved review threads are cleared, and the PR is cleanly mergeable.
- The auto-fix prompt is customizable per task from the PR CI popover.
- The per-task prompt editor is opened from an edit button in the automation section.
- The per-task prompt editor links to Settings > Prompts so the user can edit the default `ci-auto-fix` prompt.
@@ -226,8 +226,9 @@ Auto-merge cycle for one task/PR:
- **GIVEN** auto-fix has already used 10 rounds for a PR and the 10th round is still queued, **WHEN** new actionable feedback appears, **THEN** Kandev replaces that pending queued prompt without incrementing the round count.
- **GIVEN** auto-fix sends a prompt for feedback that the backend considered prompt-worthy but the agent determines is already addressed or otherwise non-actionable, **WHEN** the agent reviews that prompt, **THEN** the agent does not modify files, commit, or push and only reports that there is nothing actionable to address.
- **GIVEN** auto-fix is enabled and the task session is running, **WHEN** new actionable PR feedback appears, **THEN** the prompt is queued and delivered after the current turn rather than interrupting the running session, and the chat history shows the `@ci-auto-fix` user message with visible PR snapshot details before the agent output for the queued turn.
- **GIVEN** a linked draft PR has passing checks and GitHub reports clean mergeability, **WHEN** Kandev refreshes its status, **THEN** PR status surfaces identify it as a draft and do not present it as ready to merge.
- **GIVEN** auto-merge is enabled and the PR has passing checks, required reviews, no unresolved threads, and clean mergeability, **WHEN** the PR watch poll observes the ready state, **THEN** Kandev merges the PR with the existing backend merge-method selection.
- **GIVEN** auto-merge is enabled but the PR has requested changes, pending required review, failing checks, unresolved threads, or dirty mergeability, **WHEN** the PR watch poll observes the state, **THEN** Kandev does not merge.
- **GIVEN** auto-merge is enabled but the PR is a draft or has requested changes, pending required review, failing checks, unresolved threads, or dirty mergeability, **WHEN** the PR watch poll observes the state, **THEN** Kandev does not merge.
- **GIVEN** auto-merge attempted a ready-state merge and GitHub rejected it, **WHEN** the same ready state is observed again, **THEN** Kandev does not retry until the readiness signature changes.
- **GIVEN** a task has two open linked PRs, **WHEN** the user enables either automation control, **THEN** both PRs are eligible for automation and each PR records its own last-fix and last-merge state.
- **GIVEN** the user is on mobile, **WHEN** they open the PR CI drawer, **THEN** the automation controls and prompt editor are usable without text overflow or overlapping controls.