From 9c380da4842f27413253a1fb262334fea51f017a Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Tue, 2 Jun 2026 16:26:36 -0400 Subject: [PATCH] Fix duplicate review sessions on in-flight crow:auto kickoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After #405 made `createReviewSession` non-blocking on the main actor, the IssueTracker watcher can tick during the ~10s clone window and enqueue a second kickoff for the same PR — the cross-reference field it gates on (`ReviewRequest.reviewSessionID`) is populated lazily on the next refresh, so it stays `nil` until after the session has already been appended. Stop trusting the lagging cross-reference. Consult the authoritative `appState.sessions` (filtered to active reviews) via a single shared `existingReviewSession(forPRURL:)` helper. Layered checks: - Watcher guard in AppDelegate skips kickoff when a session for the PR URL already exists in `appState`, regardless of the `request` cross-ref. - Universal backstop in `SessionService.createReviewSession` early-returns the existing session ID. Safe because `enqueueReviewKickoff` is already serialized — Task B observes Task A's appended row. - Single + batch Start Review buttons in `ReviewBoardView` use the same helper, so the UI flips to "Go to Session" the instant the row appears instead of lagging by an IssueTracker tick. Also extract the PR-URL parser inlined in `createReviewSession` into `Session.parseReviewPR(url:)` so all sites share one implementation. Closes #406 🐦‍⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8B3BF03E-DEA0-4C05-B65F-D83BC2CF60CB --- .../CrowCore/Sources/CrowCore/AppState.swift | 13 +++ .../Sources/CrowCore/Models/Session.swift | 13 +++ .../AppStateReviewSessionLookupTests.swift | 100 ++++++++++++++++++ .../SessionParseReviewPRTests.swift | 22 ++++ .../Sources/CrowUI/ReviewBoardView.swift | 15 ++- Sources/Crow/App/AppDelegate.swift | 9 +- Sources/Crow/App/SessionService.swift | 21 +++- 7 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 Packages/CrowCore/Tests/CrowCoreTests/AppStateReviewSessionLookupTests.swift create mode 100644 Packages/CrowCore/Tests/CrowCoreTests/SessionParseReviewPRTests.swift diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 006a95c..a3b3cfe 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -497,6 +497,19 @@ public final class AppState { return reviewRequests.first { $0.url == prLink.url } } + /// Authoritative lookup for "is there already an active review session + /// for this PR URL?". Cross-references `reviewSessions` (which already + /// excludes completed/archived) against `links` by `.pr` linkType + + /// exact URL match. Used by the kickoff watcher, the review-board + /// buttons, and `SessionService.createReviewSession` as a single source + /// of truth so they don't rely on the lagging `ReviewRequest.reviewSessionID` + /// cross-reference that IssueTracker populates one tick late (CROW-406). + public func existingReviewSession(forPRURL url: String) -> Session? { + reviewSessions.first { session in + links(for: session.id).contains { $0.linkType == .pr && $0.url == url } + } + } + /// Labels for a session, sourced from its linked AssignedIssue or ReviewRequest. public func labels(forSession session: Session) -> [LabelInfo] { if let issue = assignedIssue(for: session) { diff --git a/Packages/CrowCore/Sources/CrowCore/Models/Session.swift b/Packages/CrowCore/Sources/CrowCore/Models/Session.swift index d91ae0f..9ee6550 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/Session.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/Session.swift @@ -65,6 +65,19 @@ public struct Session: Identifiable, Codable, Sendable { self.autoMergeEnabledAt = autoMergeEnabledAt } + /// Parse a GitHub PR URL (`https://github.com///pull/`) + /// into its components. Returns `nil` if the URL is malformed. Shared by + /// `SessionService.createReviewSession` and the kickoff-dedup helpers on + /// `AppState`; keep here so callers don't reinvent the same parser. + public static func parseReviewPR(url: String) -> (owner: String, repo: String, number: Int)? { + let components = url.split(separator: "/") + guard components.count >= 5, + let number = Int(components.last ?? "") else { return nil } + let owner = String(components[components.count - 4]) + let repo = String(components[components.count - 3]) + return (owner, repo, number) + } + // Backward-compatible decoding: default `kind`, `agentKind`, and // `reviewPromptDispatched` when missing from older persisted data. // `reviewPromptDispatched` defaults to `true` so existing review sessions diff --git a/Packages/CrowCore/Tests/CrowCoreTests/AppStateReviewSessionLookupTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/AppStateReviewSessionLookupTests.swift new file mode 100644 index 0000000..0994f26 --- /dev/null +++ b/Packages/CrowCore/Tests/CrowCoreTests/AppStateReviewSessionLookupTests.swift @@ -0,0 +1,100 @@ +import Foundation +import Testing +@testable import CrowCore + +@MainActor +private func makeReviewSession(name: String = "review-crow-406", status: SessionStatus = .active) -> Session { + Session( + name: name, + status: status, + kind: .review, + agentKind: .claudeCode, + provider: .github + ) +} + +@MainActor +@Test func existingReviewSessionReturnsNilWhenNoSessionMatches() { + let appState = AppState() + let session = makeReviewSession() + appState.sessions = [session] + appState.links[session.id] = [ + SessionLink(sessionID: session.id, label: "PR #1", url: "https://github.com/org/other/pull/1", linkType: .pr) + ] + + #expect(appState.existingReviewSession(forPRURL: "https://github.com/org/repo/pull/406") == nil) +} + +@MainActor +@Test func existingReviewSessionReturnsSessionWithMatchingPRLink() { + let appState = AppState() + let session = makeReviewSession() + let prURL = "https://github.com/radiusmethod/crow/pull/406" + appState.sessions = [session] + appState.links[session.id] = [ + SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr) + ] + + #expect(appState.existingReviewSession(forPRURL: prURL)?.id == session.id) +} + +@MainActor +@Test func existingReviewSessionIgnoresCompletedSessions() { + let appState = AppState() + let session = makeReviewSession(status: .completed) + let prURL = "https://github.com/radiusmethod/crow/pull/406" + appState.sessions = [session] + appState.links[session.id] = [ + SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr) + ] + + #expect(appState.existingReviewSession(forPRURL: prURL) == nil) +} + +@MainActor +@Test func existingReviewSessionIgnoresArchivedSessions() { + let appState = AppState() + let session = makeReviewSession(status: .archived) + let prURL = "https://github.com/radiusmethod/crow/pull/406" + appState.sessions = [session] + appState.links[session.id] = [ + SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr) + ] + + #expect(appState.existingReviewSession(forPRURL: prURL) == nil) +} + +@MainActor +@Test func existingReviewSessionIgnoresNonPRLinkTypes() { + let appState = AppState() + let session = makeReviewSession() + let prURL = "https://github.com/radiusmethod/crow/pull/406" + appState.sessions = [session] + // Same URL string but linked as `.repo`, not `.pr` — should not match. + appState.links[session.id] = [ + SessionLink(sessionID: session.id, label: "Repo", url: prURL, linkType: .repo) + ] + + #expect(appState.existingReviewSession(forPRURL: prURL) == nil) +} + +@MainActor +@Test func existingReviewSessionReturnsFirstMatchWhenDuplicatesExist() { + let appState = AppState() + let first = makeReviewSession(name: "review-crow-406-a") + let second = makeReviewSession(name: "review-crow-406-b") + let prURL = "https://github.com/radiusmethod/crow/pull/406" + appState.sessions = [first, second] + appState.links[first.id] = [ + SessionLink(sessionID: first.id, label: "PR #406", url: prURL, linkType: .pr) + ] + appState.links[second.id] = [ + SessionLink(sessionID: second.id, label: "PR #406", url: prURL, linkType: .pr) + ] + + // We don't care which session wins — only that the helper returns + // exactly one of them so callers get a stable, non-nil answer. + let match = appState.existingReviewSession(forPRURL: prURL) + #expect(match != nil) + #expect(match?.id == first.id || match?.id == second.id) +} diff --git a/Packages/CrowCore/Tests/CrowCoreTests/SessionParseReviewPRTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/SessionParseReviewPRTests.swift new file mode 100644 index 0000000..a8a036f --- /dev/null +++ b/Packages/CrowCore/Tests/CrowCoreTests/SessionParseReviewPRTests.swift @@ -0,0 +1,22 @@ +import Foundation +import Testing +@testable import CrowCore + +@Test func parseReviewPRExtractsOwnerRepoAndNumber() { + let parsed = Session.parseReviewPR(url: "https://github.com/radiusmethod/crow/pull/406") + #expect(parsed?.owner == "radiusmethod") + #expect(parsed?.repo == "crow") + #expect(parsed?.number == 406) +} + +@Test func parseReviewPRReturnsNilForNonIntegerTrailingSegment() { + #expect(Session.parseReviewPR(url: "https://github.com/owner/repo/pull/abc") == nil) +} + +@Test func parseReviewPRReturnsNilForTooFewPathComponents() { + #expect(Session.parseReviewPR(url: "github.com/owner/123") == nil) +} + +@Test func parseReviewPRReturnsNilForEmptyString() { + #expect(Session.parseReviewPR(url: "") == nil) +} diff --git a/Packages/CrowUI/Sources/CrowUI/ReviewBoardView.swift b/Packages/CrowUI/Sources/CrowUI/ReviewBoardView.swift index 45edb25..c48d591 100644 --- a/Packages/CrowUI/Sources/CrowUI/ReviewBoardView.swift +++ b/Packages/CrowUI/Sources/CrowUI/ReviewBoardView.swift @@ -136,9 +136,15 @@ public struct ReviewBoardView: View { .buttonStyle(.plain) Button { + // Pre-filter PRs that already have an active review session + // so a stale multi-select doesn't enqueue duplicate kickoffs + // (CROW-406). The dedup in createReviewSession would catch + // these too, but filtering here keeps the kickoff queue from + // accumulating no-op work. let urls = appState.filteredReviewRequests .filter { selectedRequestIDs.contains($0.id) } .map(\.url) + .filter { appState.existingReviewSession(forPRURL: $0) == nil } appState.onBatchStartReview?(urls) selectedRequestIDs.removeAll() isSelectionMode = false @@ -306,10 +312,13 @@ struct ReviewRow: View { private var reviewAction: some View { if isSelectionMode { EmptyView() - } else if let sessionID = request.reviewSessionID, - appState.sessions.contains(where: { $0.id == sessionID }) { + } else if let existing = appState.existingReviewSession(forPRURL: request.url) { + // Look up the session directly from `appState` instead of the + // lagging `request.reviewSessionID` so the button flips to + // "Go to Session" the instant the session row is appended, + // not on the next IssueTracker refresh (CROW-406). Button { - appState.selectedSessionID = sessionID + appState.selectedSessionID = existing.id } label: { HStack(spacing: 4) { Image(systemName: "arrow.right.circle") diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 9fc2b56..ab9c43a 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -621,7 +621,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate { let shaAdvanced = linkedSession != nil && request.headRefOid != nil && linkedSession?.lastReviewedHeadSha != request.headRefOid - guard request.reviewSessionID == nil || shaAdvanced else { continue } + // Authoritative `appState`-side check: a prior tick during an + // in-flight kickoff may have already created a session even + // though `request.reviewSessionID` hasn't been repopulated by + // the next IssueTracker refresh yet (CROW-406). Without this, + // a watcher tick during the ~10s clone window enqueues a + // duplicate kickoff for the same PR. + let existingByPR = self.appState.existingReviewSession(forPRURL: request.url) + guard (request.reviewSessionID == nil && existingByPR == nil) || shaAdvanced else { continue } // B-fallback: tear down the stale round-1 session so the new // session doesn't double up in `reviewSessions` for the same diff --git a/Sources/Crow/App/SessionService.swift b/Sources/Crow/App/SessionService.swift index 097b427..b26e434 100644 --- a/Sources/Crow/App/SessionService.swift +++ b/Sources/Crow/App/SessionService.swift @@ -1246,15 +1246,26 @@ final class SessionService { /// what produced the SwiftUI reentrant-layout crash in #266. @discardableResult func createReviewSession(prURL: String, selectAfterCreate: Bool = false) async -> UUID? { + // Universal backstop against duplicate sessions for the same PR. The + // serial kickoff queue in AppDelegate guarantees that by the time a + // second `createReviewSession` runs, the first has already appended + // its row to `appState.sessions` — so this check is authoritative, + // not racy. Belt-and-suspenders for the auto-review watcher race + // (CROW-406) and any future caller that re-enters during a kickoff. + if let existing = appState.existingReviewSession(forPRURL: prURL) { + NSLog("[SessionService] Skipping duplicate review session for \(prURL); reusing \(existing.id)") + if selectAfterCreate { appState.selectedSessionID = existing.id } + return existing.id + } + // Parse org/repo and PR number from URL like "https://github.com/org/repo/pull/123" - let components = prURL.split(separator: "/") - guard components.count >= 5, - let prNumber = Int(components.last ?? "") else { + guard let parsed = Session.parseReviewPR(url: prURL) else { NSLog("[SessionService] Could not parse PR URL: \(prURL)") return nil } - let owner = String(components[components.count - 4]) - let repoName = String(components[components.count - 3]) + let owner = parsed.owner + let repoName = parsed.repo + let prNumber = parsed.number let repoSlug = "\(owner)/\(repoName)" // Determine clone path