diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index 7a31779938..e1078f9804 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -714,7 +714,8 @@ func (h *Handler) handlePullRequestEvent(ctx context.Context, body []byte) { // Auto-link: scan title/body/branch for issue identifiers, look them // up in this workspace, attach the link rows. Idempotent (ON CONFLICT - // DO NOTHING) so re-firing the webhook doesn't duplicate. + // upserts the close_intent flag — see LinkIssueToPullRequest) so + // re-firing the webhook doesn't duplicate. // // RFC MUL-2414 §4.8: the PR mirror upsert above always runs (so re-enabling // GitHub features restores history without backfill), but the link rows @@ -727,57 +728,70 @@ func (h *Handler) handlePullRequestEvent(ctx context.Context, body []byte) { // closingIdents is the subset of identifiers that this PR explicitly // declared via a closing keyword ("Closes/Fixes/Resolves MUL-X"). // Linking still happens for every mention (idents above), but the - // auto-advance-to-done gate below only fires for keyword-declared - // identifiers — bare title prefixes and branch-name references are - // link-only. + // link row's close_intent column — and therefore whether the + // auto-advance gate eventually fires — is only set for keyword- + // declared identifiers. Bare title prefixes and branch-name + // references are link-only. closingIdents := map[string]struct{}{} for _, c := range extractClosingIdentifiers(p.PullRequest.Title, p.PullRequest.Body) { closingIdents[c] = struct{}{} } prefix := h.getIssuePrefix(ctx, inst.WorkspaceID) + // reevalIssues collects each issue whose link row we just touched so + // we can re-run the auto-advance gate against the persisted aggregate + // after every link upsert in this event. Driving the gate off + // persisted state (instead of "did *this* webhook declare closing + // intent?") is what fixes the multi-PR sibling case: a PR with + // `Closes MUL-1` merges first while a link-only sibling is still + // open, then the sibling closes later — its webhook has no closing + // keyword, but the earlier link row carries close_intent=true, so + // MUL-1 still advances. + reevalIssues := make([]db.Issue, 0, len(idents)) for _, id := range idents { issue, ok := h.lookupIssueByIdentifier(ctx, inst.WorkspaceID, prefix, id) if !ok { continue } + _, declared := closingIdents[id] if err := h.Queries.LinkIssueToPullRequest(ctx, db.LinkIssueToPullRequestParams{ - IssueID: issue.ID, - PullRequestID: pr.ID, - LinkedByType: strToText("system"), - LinkedByID: pgtype.UUID{}, + IssueID: issue.ID, + PullRequestID: pr.ID, + CloseIntent: declared, + LinkedByType: strToText("system"), + LinkedByID: pgtype.UUID{}, }); err != nil { slog.Warn("github: link failed", "err", err) continue } linkedIssueIDs = append(linkedIssueIDs, uuidToString(issue.ID)) + reevalIssues = append(reevalIssues, issue) + } - // A terminal PR event (`merged` or `closed`) may be the moment the - // last in-flight sibling resolves, so we re-evaluate the issue on - // both. We advance the issue to done when: - // 1. this PR explicitly declared closing intent for the issue - // (closing keyword in title/body — see closingIdents above); - // 2. the issue isn't already terminal (`done` / `cancelled`); - // 3. no sibling PR is still `open` / `draft`; - // 4. at least one linked PR (this one or a sibling) is `merged`. - // Rule (1) is what prevents "Follow up in MUL-2" / "Unblocks MUL-3" - // references from being treated the same as "Closes MUL-1". Rule - // (4) prevents an "all closed-without-merge" sequence from - // silently auto-closing the issue — if nothing was ever - // delivered, the user should decide what to do manually. - if _, declared := closingIdents[id]; !declared { - continue - } - if (state == "merged" || state == "closed") && issue.Status != "done" && issue.Status != "cancelled" { - counts, err := h.Queries.GetSiblingPullRequestStateCountsForIssue(ctx, db.GetSiblingPullRequestStateCountsForIssueParams{ - IssueID: issue.ID, - ID: pr.ID, - }) - if err != nil { - slog.Warn("github: count sibling pr states failed", "err", err, "issue_id", uuidToString(issue.ID)) + // A terminal PR event (`merged` or `closed`) may be the moment the + // last in-flight sibling resolves. We re-evaluate every issue we + // just linked once both the PR row and the link row are persisted, + // so the aggregate query sees the freshest state. We advance the + // issue to done when: + // 1. the issue isn't already terminal (`done` / `cancelled`); + // 2. no linked PR is still `open` / `draft`; + // 3. at least one merged linked PR declared close_intent (a + // "Closes/Fixes/Resolves" keyword on its link row). + // Rule (3) is what prevents "Follow up in MUL-2" / "Unblocks MUL-3" + // references from being treated the same as "Closes MUL-1", and + // also prevents an "all closed-without-merge" sequence from + // silently auto-closing the issue — if nothing carrying closing + // intent was ever delivered, the user should decide manually. + if state == "merged" || state == "closed" { + for _, issue := range reevalIssues { + if issue.Status == "done" || issue.Status == "cancelled" { continue } - anyMerged := state == "merged" || counts.MergedCount > 0 - if counts.OpenCount == 0 && anyMerged { + counts, err := h.Queries.GetIssuePullRequestCloseAggregate(ctx, issue.ID) + if err != nil { + slog.Warn("github: count linked pr states failed", "err", err, "issue_id", uuidToString(issue.ID)) + continue + } + if counts.OpenCount == 0 && counts.MergedWithCloseIntentCount > 0 { h.advanceIssueToDone(ctx, issue, workspaceID) } } diff --git a/server/internal/handler/github_test.go b/server/internal/handler/github_test.go index 9213509bb2..1fcbd7b857 100644 --- a/server/internal/handler/github_test.go +++ b/server/internal/handler/github_test.go @@ -996,6 +996,152 @@ func TestWebhook_MergedPR_BranchNameDoesNotClose(t *testing.T) { } } +// firePRWebhook fires a webhook for a single PR with caller-controlled +// title, body, branch, and lifecycle (open / merged / closed without +// merge). Tests below need the open→merged sequence so close_intent on +// one PR has to persist across multiple webhook events for a sibling PR. +func firePRWebhook(t *testing.T, secret string, installationID int64, prNumber int32, title, body, branch, lifecycle string) { + t.Helper() + var action, state string + var merged bool + var mergedAt, closedAt any + switch lifecycle { + case "opened": + action, state, merged = "opened", "open", false + mergedAt, closedAt = nil, nil + case "merged": + action, state, merged = "closed", "closed", true + mergedAt, closedAt = "2026-04-29T00:00:00Z", "2026-04-29T00:00:00Z" + case "closed": + action, state, merged = "closed", "closed", false + mergedAt, closedAt = nil, "2026-04-29T00:00:00Z" + default: + t.Fatalf("firePRWebhook: unknown lifecycle %q", lifecycle) + } + payload := map[string]any{ + "action": action, + "pull_request": map[string]any{ + "number": prNumber, + "html_url": fmt.Sprintf("https://github.com/acme/widget/pull/%d", prNumber), + "title": title, + "body": body, + "state": state, + "draft": false, + "merged": merged, + "merged_at": mergedAt, + "closed_at": closedAt, + "created_at": "2026-04-28T00:00:00Z", + "updated_at": "2026-04-29T00:00:00Z", + "head": map[string]any{"ref": branch}, + "user": map[string]any{"login": "octocat"}, + }, + "repository": map[string]any{"name": "widget", "owner": map[string]any{"login": "acme"}}, + "installation": map[string]any{"id": installationID}, + } + raw, _ := json.Marshal(payload) + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(raw) + sig := "sha256=" + hex.EncodeToString(mac.Sum(nil)) + + rec := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/api/webhooks/github", bytes.NewReader(raw)) + req.Header.Set("X-GitHub-Event", "pull_request") + req.Header.Set("X-Hub-Signature-256", sig) + testHandler.HandleGitHubWebhook(rec, req) + if rec.Code != http.StatusAccepted { + t.Fatalf("webhook pr=%d (%s): expected 202, got %d (%s)", prNumber, lifecycle, rec.Code, rec.Body.String()) + } +} + +// TestWebhook_LinkOnlySiblingMergeAfterCloseKeywordPR is the regression +// guard for the multi-PR sibling case Elon flagged on the first attempt +// of this fix. Scenario: +// +// 1. PR A declares closing intent (`Closes MUL-X`) and is opened. +// 2. PR B references the same issue (link-only — no closing keyword) +// and is opened. +// 3. PR A merges. The issue stays in_progress because PR B is open. +// 4. PR B merges later. PR B's webhook has no closing keyword, so the +// previous implementation skipped re-evaluating the issue and the +// issue stayed stuck in_progress forever. +// +// The persisted close_intent column on issue_pull_request fixes this: +// the aggregate sees PR A's merged+close_intent row regardless of which +// webhook drives the re-evaluation, so PR B's merge advances the issue. +func TestWebhook_LinkOnlySiblingMergeAfterCloseKeywordPR(t *testing.T) { + if testHandler == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + secret := "link-only-sibling-secret" + t.Setenv("GITHUB_WEBHOOK_SECRET", secret) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "needs two prs", + "status": "in_progress", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: %d %s", w.Code, w.Body.String()) + } + var created IssueResponse + json.NewDecoder(w.Body).Decode(&created) + + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM issue_pull_request WHERE issue_id = $1`, created.ID) + testPool.Exec(ctx, `DELETE FROM activity_log WHERE issue_id = $1`, created.ID) + testPool.Exec(ctx, `DELETE FROM github_pull_request WHERE workspace_id = $1`, testWorkspaceID) + testPool.Exec(ctx, `DELETE FROM github_installation WHERE workspace_id = $1`, testWorkspaceID) + testPool.Exec(ctx, `DELETE FROM issue WHERE id = $1`, created.ID) + }) + + const installationID int64 = 30264004 + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "link-only-sibling-acct", + AccountType: "User", + }); err != nil { + t.Fatalf("CreateGitHubInstallation: %v", err) + } + + // 1) PR A opens with closing intent. + firePRWebhook(t, secret, installationID, 1, "Implement primary path", "Closes "+created.Identifier, "feat/primary", "opened") + // 2) PR B opens link-only — title prefix mention, no closing keyword. + firePRWebhook(t, secret, installationID, 2, created.Identifier+": follow-up cleanup", "", "feat/cleanup", "opened") + + // Sanity: issue is still in_progress (both PRs open). + got, err := testHandler.Queries.GetIssue(ctx, parseUUID(created.ID)) + if err != nil { + t.Fatalf("GetIssue after open: %v", err) + } + if got.Status != "in_progress" { + t.Fatalf("after both PRs opened: status = %q, want in_progress", got.Status) + } + + // 3) PR A merges. PR B still open → issue stays in_progress. + firePRWebhook(t, secret, installationID, 1, "Implement primary path", "Closes "+created.Identifier, "feat/primary", "merged") + got, err = testHandler.Queries.GetIssue(ctx, parseUUID(created.ID)) + if err != nil { + t.Fatalf("GetIssue after A merge: %v", err) + } + if got.Status != "in_progress" { + t.Fatalf("after PR A merged with PR B still open: status = %q, want in_progress", got.Status) + } + + // 4) PR B merges (link-only, no closing keyword). The persisted + // close_intent on PR A's link must still carry the advance. + firePRWebhook(t, secret, installationID, 2, created.Identifier+": follow-up cleanup", "", "feat/cleanup", "merged") + got, err = testHandler.Queries.GetIssue(ctx, parseUUID(created.ID)) + if err != nil { + t.Fatalf("GetIssue after B merge: %v", err) + } + if got.Status != "done" { + t.Errorf("after both PRs merged (A with close_intent, B link-only): status = %q, want done", got.Status) + } +} + // ── CI / mergeable_state tests ───────────────────────────────────────────── func TestDerivePRMergeableState(t *testing.T) { diff --git a/server/migrations/109_issue_pull_request_close_intent.down.sql b/server/migrations/109_issue_pull_request_close_intent.down.sql new file mode 100644 index 0000000000..3581184d7c --- /dev/null +++ b/server/migrations/109_issue_pull_request_close_intent.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE issue_pull_request + DROP COLUMN close_intent; diff --git a/server/migrations/109_issue_pull_request_close_intent.up.sql b/server/migrations/109_issue_pull_request_close_intent.up.sql new file mode 100644 index 0000000000..3be7a0f765 --- /dev/null +++ b/server/migrations/109_issue_pull_request_close_intent.up.sql @@ -0,0 +1,7 @@ +-- Persist whether a PR ↔ issue link was created with explicit closing intent +-- (a "Closes/Fixes/Resolves PREFIX-N" keyword in the PR title or body). The +-- webhook auto-advance gate consults this column so a link-only sibling PR +-- closing later still resolves an issue that an earlier closing-keyword PR +-- had blocked from advancing. +ALTER TABLE issue_pull_request + ADD COLUMN close_intent BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/server/pkg/db/generated/github.sql.go b/server/pkg/db/generated/github.sql.go index 0a1d329873..191fec1b2d 100644 --- a/server/pkg/db/generated/github.sql.go +++ b/server/pkg/db/generated/github.sql.go @@ -183,52 +183,50 @@ func (q *Queries) GetGitHubPullRequest(ctx context.Context, arg GetGitHubPullReq return i, err } -const getSiblingPullRequestStateCountsForIssue = `-- name: GetSiblingPullRequestStateCountsForIssue :one +const getIssuePullRequestCloseAggregate = `-- name: GetIssuePullRequestCloseAggregate :one SELECT COALESCE(SUM(CASE WHEN pr.state IN ('open', 'draft') THEN 1 ELSE 0 END), 0)::bigint AS open_count, - COALESCE(SUM(CASE WHEN pr.state = 'merged' THEN 1 ELSE 0 END), 0)::bigint AS merged_count + COALESCE(SUM(CASE WHEN pr.state = 'merged' AND ipr.close_intent THEN 1 ELSE 0 END), 0)::bigint AS merged_with_close_intent_count FROM github_pull_request pr JOIN issue_pull_request ipr ON ipr.pull_request_id = pr.id WHERE ipr.issue_id = $1 - AND pr.id <> $2 ` -type GetSiblingPullRequestStateCountsForIssueParams struct { - IssueID pgtype.UUID `json:"issue_id"` - ID pgtype.UUID `json:"id"` +type GetIssuePullRequestCloseAggregateRow struct { + OpenCount int64 `json:"open_count"` + MergedWithCloseIntentCount int64 `json:"merged_with_close_intent_count"` } -type GetSiblingPullRequestStateCountsForIssueRow struct { - OpenCount int64 `json:"open_count"` - MergedCount int64 `json:"merged_count"` -} - -// Returns, for the PRs linked to an issue excluding one PR by id (the PR -// currently being processed by the webhook handler), how many are still in -// flight (open or draft) and how many have already merged. The webhook -// handler combines these with the current event's state to decide whether -// to auto-advance the issue: the issue moves to done only when there is no -// in-flight sibling AND at least one linked PR (current or sibling) merged. -func (q *Queries) GetSiblingPullRequestStateCountsForIssue(ctx context.Context, arg GetSiblingPullRequestStateCountsForIssueParams) (GetSiblingPullRequestStateCountsForIssueRow, error) { - row := q.db.QueryRow(ctx, getSiblingPullRequestStateCountsForIssue, arg.IssueID, arg.ID) - var i GetSiblingPullRequestStateCountsForIssueRow - err := row.Scan(&i.OpenCount, &i.MergedCount) +// Aggregates the issue's linked PRs into the two counts that gate +// auto-advance: how many are still in flight (`open` or `draft`) and how +// many merged PRs declared explicit closing intent on the link row. The +// webhook auto-advances the issue when open_count = 0 AND +// merged_with_close_intent_count > 0. Both the PR state and the link row +// (with close_intent) are persisted before this query runs, so the result +// is event-agnostic — a link-only sibling closing after a closing-keyword +// PR has already merged still resolves the issue. +func (q *Queries) GetIssuePullRequestCloseAggregate(ctx context.Context, issueID pgtype.UUID) (GetIssuePullRequestCloseAggregateRow, error) { + row := q.db.QueryRow(ctx, getIssuePullRequestCloseAggregate, issueID) + var i GetIssuePullRequestCloseAggregateRow + err := row.Scan(&i.OpenCount, &i.MergedWithCloseIntentCount) return i, err } const linkIssueToPullRequest = `-- name: LinkIssueToPullRequest :exec INSERT INTO issue_pull_request ( - issue_id, pull_request_id, linked_by_type, linked_by_id + issue_id, pull_request_id, linked_by_type, linked_by_id, close_intent ) VALUES ( - $1, $2, $3, $4 + $1, $2, $4, $5, $3 ) -ON CONFLICT (issue_id, pull_request_id) DO NOTHING +ON CONFLICT (issue_id, pull_request_id) DO UPDATE SET + close_intent = issue_pull_request.close_intent OR EXCLUDED.close_intent ` type LinkIssueToPullRequestParams struct { IssueID pgtype.UUID `json:"issue_id"` PullRequestID pgtype.UUID `json:"pull_request_id"` + CloseIntent bool `json:"close_intent"` LinkedByType pgtype.Text `json:"linked_by_type"` LinkedByID pgtype.UUID `json:"linked_by_id"` } @@ -236,10 +234,17 @@ type LinkIssueToPullRequestParams struct { // ===================== // Issue ↔ Pull Request link // ===================== +// close_intent is monotonic: once a PR has been linked with closing intent +// (a "Closes/Fixes/Resolves" keyword in title or body), a later webhook +// re-fire for the same PR without the keyword (e.g. body was edited to +// drop the keyword after merge) must not clear the flag. The OR-merge in +// DO UPDATE keeps the strongest declaration that has ever applied to this +// link. func (q *Queries) LinkIssueToPullRequest(ctx context.Context, arg LinkIssueToPullRequestParams) error { _, err := q.db.Exec(ctx, linkIssueToPullRequest, arg.IssueID, arg.PullRequestID, + arg.CloseIntent, arg.LinkedByType, arg.LinkedByID, ) diff --git a/server/pkg/db/generated/models.go b/server/pkg/db/generated/models.go index 980b9f87b9..cda61871f0 100644 --- a/server/pkg/db/generated/models.go +++ b/server/pkg/db/generated/models.go @@ -380,6 +380,7 @@ type IssuePullRequest struct { LinkedByType pgtype.Text `json:"linked_by_type"` LinkedByID pgtype.UUID `json:"linked_by_id"` LinkedAt pgtype.Timestamptz `json:"linked_at"` + CloseIntent bool `json:"close_intent"` } type IssueReaction struct { diff --git a/server/pkg/db/queries/github.sql b/server/pkg/db/queries/github.sql index e4a1db2d16..a15a7ebd8b 100644 --- a/server/pkg/db/queries/github.sql +++ b/server/pkg/db/queries/github.sql @@ -151,20 +151,21 @@ ORDER BY pr.pr_created_at DESC; SELECT issue_id FROM issue_pull_request WHERE pull_request_id = $1; --- name: GetSiblingPullRequestStateCountsForIssue :one --- Returns, for the PRs linked to an issue excluding one PR by id (the PR --- currently being processed by the webhook handler), how many are still in --- flight (open or draft) and how many have already merged. The webhook --- handler combines these with the current event's state to decide whether --- to auto-advance the issue: the issue moves to done only when there is no --- in-flight sibling AND at least one linked PR (current or sibling) merged. +-- name: GetIssuePullRequestCloseAggregate :one +-- Aggregates the issue's linked PRs into the two counts that gate +-- auto-advance: how many are still in flight (`open` or `draft`) and how +-- many merged PRs declared explicit closing intent on the link row. The +-- webhook auto-advances the issue when open_count = 0 AND +-- merged_with_close_intent_count > 0. Both the PR state and the link row +-- (with close_intent) are persisted before this query runs, so the result +-- is event-agnostic — a link-only sibling closing after a closing-keyword +-- PR has already merged still resolves the issue. SELECT COALESCE(SUM(CASE WHEN pr.state IN ('open', 'draft') THEN 1 ELSE 0 END), 0)::bigint AS open_count, - COALESCE(SUM(CASE WHEN pr.state = 'merged' THEN 1 ELSE 0 END), 0)::bigint AS merged_count + COALESCE(SUM(CASE WHEN pr.state = 'merged' AND ipr.close_intent THEN 1 ELSE 0 END), 0)::bigint AS merged_with_close_intent_count FROM github_pull_request pr JOIN issue_pull_request ipr ON ipr.pull_request_id = pr.id -WHERE ipr.issue_id = $1 - AND pr.id <> $2; +WHERE ipr.issue_id = $1; -- ===================== -- GitHub PR check suite @@ -195,12 +196,19 @@ WHERE EXCLUDED.updated_at >= github_pull_request_check_suite.updated_at; -- ===================== -- name: LinkIssueToPullRequest :exec +-- close_intent is monotonic: once a PR has been linked with closing intent +-- (a "Closes/Fixes/Resolves" keyword in title or body), a later webhook +-- re-fire for the same PR without the keyword (e.g. body was edited to +-- drop the keyword after merge) must not clear the flag. The OR-merge in +-- DO UPDATE keeps the strongest declaration that has ever applied to this +-- link. INSERT INTO issue_pull_request ( - issue_id, pull_request_id, linked_by_type, linked_by_id + issue_id, pull_request_id, linked_by_type, linked_by_id, close_intent ) VALUES ( - $1, $2, sqlc.narg('linked_by_type'), sqlc.narg('linked_by_id') + $1, $2, sqlc.narg('linked_by_type'), sqlc.narg('linked_by_id'), $3 ) -ON CONFLICT (issue_id, pull_request_id) DO NOTHING; +ON CONFLICT (issue_id, pull_request_id) DO UPDATE SET + close_intent = issue_pull_request.close_intent OR EXCLUDED.close_intent; -- name: UnlinkIssueFromPullRequest :exec DELETE FROM issue_pull_request