From d877057287d34047230bd9b2c2915b3232bac95e Mon Sep 17 00:00:00 2001 From: Dustin Hilgaertner Date: Tue, 16 Jun 2026 16:22:20 -0500 Subject: [PATCH] Fix PR-link reconcile wrong-PR attribution (#520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconcile attached the wrong PR to sessions whose worktree branch carries a repo-name prefix the PR head branch drops (`feature/max-monorepo-maxx-7035-…` worktree vs `feature/maxx-7035-…` PR head), and even attached a PR to sessions whose ticket had none. Three remaining gaps caused this: - Key matching accepted a PR when the ticket key appeared in its *body*, so a PR merely *mentioning* a related ticket (e.g. "MAXX-6854") attached to that unrelated session. Now matches require the key in the PR title or head branch only (GitHubCodeBackend.parseKeyPRMatches). - The ticket key was only derived from a Jira ticket URL. Add Validation.ticketKey(fromBranch:) and use it as a fallback for task-only (Jira/Corveil) sessions so the prefix-drop branch resolves to the right PR. - Picks were chosen per session with no cross-session de-dup. Add dedupeContestedPRs: a PR claimed by sessions with differing work-item identities is dropped from all of them — never guess. Derived links continue to persist via JSONStore. Adds regression coverage for the exact max-monorepo scenario plus branch-key extraction, body-match exclusion, and cross-session de-dup. 🐦‍⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 5A3DD516-CAF3-4FFD-8149-99D4523B7C99 --- CHANGELOG.md | 1 + .../Sources/CrowCore/Validation.swift | 14 +++ .../ValidationTicketKeyTests.swift | 36 ++++++ .../Backends/GitHubCodeBackend.swift | 11 +- .../CrowProviderTests/BackendsTests.swift | 19 +-- Sources/Crow/App/IssueTracker.swift | 52 ++++++++- .../IssueTrackerReconcileTests.swift | 108 ++++++++++++++++++ 7 files changed, 223 insertions(+), 18 deletions(-) create mode 100644 Packages/CrowCore/Tests/CrowCoreTests/ValidationTicketKeyTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cfb541d..4f53c977 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Backfill of merged PRs since the 0.1.0 release, grouped by theme. - #220 — Filtering for the tickets list. - #226 — Per-section select all and icon-only cancel button in selection mode. - #231 — Quick action buttons on the session detail header. +- #520 — PR-link reconcile no longer attaches the wrong PR when the worktree branch carries a repo-name prefix the PR head drops (`feature/max-monorepo-maxx-7035-…` vs `feature/maxx-7035-…`). The ticket key is derived from the worktree branch, key matching ignores body-only mentions, and a PR can attach to at most one session — a session whose ticket has no PR now gets none. ### Terminal Runtime diff --git a/Packages/CrowCore/Sources/CrowCore/Validation.swift b/Packages/CrowCore/Sources/CrowCore/Validation.swift index 908c5d48..ed0478d2 100644 --- a/Packages/CrowCore/Sources/CrowCore/Validation.swift +++ b/Packages/CrowCore/Sources/CrowCore/Validation.swift @@ -78,4 +78,18 @@ public enum Validation { public static func jiraKey(from spec: String) -> String? { parseJiraKey(spec)?.key } + + /// Extract a ticket key (e.g. `MAXX-7035`) embedded in a git branch name such + /// as `feature/max-monorepo-maxx-7035-citations`. Returns the first + /// LETTERS-DIGITS token that validates as a Jira-style key, uppercased. + /// + /// The character class excludes `-`, so on `…-max-monorepo-maxx-7035-…` the + /// first complete LETTERS-DIGITS match is `maxx-7035` → `MAXX-7035`. Numeric + /// GitHub-issue branches like `feature/acme-api-197-fix` yield `api-197`, + /// which fails `parseJiraKey`'s uppercase-project rule → `nil`. + public static func ticketKey(fromBranch branch: String) -> String? { + guard let r = branch.range(of: "[A-Za-z][A-Za-z0-9]+-[0-9]+", options: .regularExpression) + else { return nil } + return parseJiraKey(String(branch[r]).uppercased())?.key + } } diff --git a/Packages/CrowCore/Tests/CrowCoreTests/ValidationTicketKeyTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/ValidationTicketKeyTests.swift new file mode 100644 index 00000000..2f60afd5 --- /dev/null +++ b/Packages/CrowCore/Tests/CrowCoreTests/ValidationTicketKeyTests.swift @@ -0,0 +1,36 @@ +import Foundation +import Testing +@testable import CrowCore + +// MARK: - Validation.ticketKey(fromBranch:) +// +// Regression coverage for #520: worktree branches embed a repo-name prefix +// (`feature/max-monorepo-maxx-7035-…`) that the PR head branch drops. Reconcile +// derives the ticket key from the branch to recover the right PR. + +@Test func ticketKeyExtractsJiraKeyFromPrefixedBranch() { + #expect( + Validation.ticketKey(fromBranch: "feature/max-monorepo-maxx-7035-citations-chain-of-custody") + == "MAXX-7035" + ) +} + +@Test func ticketKeyHandlesBranchWithoutRepoPrefix() { + #expect(Validation.ticketKey(fromBranch: "feature/maxx-7035-citations") == "MAXX-7035") +} + +@Test func ticketKeyExtractsAnyJiraShapedTokenAfterUppercasing() { + // A lowercased branch can't tell a real Jira project ("maxx") from an + // ordinary segment ("api"), so this pure extractor returns the first + // Jira-*shaped* token — `api-197` → `API-197`. Guarding against false keys + // on GitHub/GitLab issue branches is the caller's job (it gates the branch + // fallback to task-only trackers), not this function's. + #expect(Validation.ticketKey(fromBranch: "feature/acme-api-197-fix-tab-url-hash") == "API-197") +} + +@Test func ticketKeyIsNilWhenNoJiraShapedToken() { + // No LETTERS-DIGITS token at all. + #expect(Validation.ticketKey(fromBranch: "feature/cleanup-readme") == nil) + #expect(Validation.ticketKey(fromBranch: "feature/197-only-number") == nil) + #expect(Validation.ticketKey(fromBranch: "") == nil) +} diff --git a/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubCodeBackend.swift b/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubCodeBackend.swift index a52b542b..17a819a5 100644 --- a/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubCodeBackend.swift +++ b/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubCodeBackend.swift @@ -224,9 +224,11 @@ public struct GitHubCodeBackend: CodeBackend { } /// Parse `gh pr list --json …` output into `KeyPRMatch`es. Post-filters to - /// PRs where the key actually appears (case-insensitively) in the title, - /// body, or head branch — `gh`'s search can be fuzzy, and we only want PRs - /// that genuinely reference the ticket. + /// PRs where the key actually appears (case-insensitively) in the **title or + /// head branch** — `gh`'s search can be fuzzy, and a key mentioned only in a + /// PR's *body* (e.g. "related to MAXX-6854") belongs to a different ticket. + /// Matching on body attached phantom PRs to sessions whose ticket had none + /// (#520), so it is deliberately excluded. static func parseKeyPRMatches(_ output: String, candidate: KeyCandidate) -> [KeyPRMatch] { guard let data = output.data(using: .utf8), let arr = try? JSONSerialization.jsonObject(with: data) as? [[String: Any]] else { return [] } @@ -239,9 +241,8 @@ public struct GitHubCodeBackend: CodeBackend { let url = node["url"] as? String, let state = node["state"] as? String else { continue } let title = (node["title"] as? String ?? "").lowercased() - let body = (node["body"] as? String ?? "").lowercased() let head = (node["headRefName"] as? String ?? "").lowercased() - guard title.contains(needle) || body.contains(needle) || head.contains(needle) else { continue } + guard title.contains(needle) || head.contains(needle) else { continue } let updatedAt = (node["updatedAt"] as? String).flatMap { dateFmt.date(from: $0) } matches.append(KeyPRMatch( candidate: candidate, number: number, url: url, state: state, updatedAt: updatedAt diff --git a/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift b/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift index c7d2116a..5dfcaa14 100644 --- a/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift +++ b/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift @@ -442,11 +442,17 @@ final class BackendsTests: XCTestCase { func testGitHubCodeBackendFindPRsMatchingKeysParsesAndFilters() async throws { let fake = FakeShellRunner() - // Two PRs returned by gh search: one references MAXX-6859 in its title, - // one is a fuzzy match that mentions it nowhere — only the first counts. + // Four PRs returned by gh search: + // #52 — key in title AND head → kept + // #53 — key in head only (title/body unrelated) → kept + // #41 — key in body ONLY → rejected (#520: a body mention belongs to a + // different ticket; matching it attached phantom PRs) + // #40 — key nowhere → rejected let json = """ [ {"number":52,"url":"https://github.com/a/b/pull/52","state":"OPEN","updatedAt":"2026-01-02T00:00:00Z","title":"feat: thing. MAXX-6859","headRefName":"feature/maxx-6859-thing","body":"closes MAXX-6859"}, + {"number":53,"url":"https://github.com/a/b/pull/53","state":"OPEN","updatedAt":"2026-01-03T00:00:00Z","title":"feat: unrelated title","headRefName":"feature/maxx-6859-other","body":"no mention"}, + {"number":41,"url":"https://github.com/a/b/pull/41","state":"MERGED","updatedAt":"2026-01-01T00:00:00Z","title":"different ticket","headRefName":"feature/other-work","body":"related to MAXX-6859"}, {"number":40,"url":"https://github.com/a/b/pull/40","state":"MERGED","updatedAt":"2026-01-01T00:00:00Z","title":"unrelated","headRefName":"feature/other","body":"no key here"} ] """ @@ -455,11 +461,10 @@ final class BackendsTests: XCTestCase { let matches = try await backend.findPRsMatchingKeys([ KeyCandidate(repoSlug: "a/b", key: "MAXX-6859") ]) - XCTAssertEqual(matches.count, 1) - XCTAssertEqual(matches[0].number, 52) - XCTAssertEqual(matches[0].state, "OPEN") - XCTAssertEqual(matches[0].candidate.key, "MAXX-6859") - // Command shape: gh pr list --search " in:title,body". + XCTAssertEqual(Set(matches.map(\.number)), [52, 53]) + XCTAssertTrue(matches.allSatisfy { $0.candidate.key == "MAXX-6859" }) + // Command shape: gh pr list --search " in:title,body" (broad recall; + // results are post-filtered to title/head only). let args = fake.calls[0].args XCTAssertEqual(Array(args.prefix(3)), ["gh", "pr", "list"]) XCTAssertTrue(args.contains("--search")) diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index b988d836..59773724 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -1084,6 +1084,26 @@ final class IssueTracker { return picks } + /// Enforce that a single PR attaches to at most one work item. Groups the + /// final per-session picks by PR URL; if a URL is claimed by sessions with + /// more than one distinct work-item identity (ticket key, else branch), the + /// PR can't be attributed to one of them with confidence, so it is dropped + /// from all of them — never guess (#520). Duplicate sessions sharing one + /// identity (same key/branch) keep the link. Pure — no appState, no I/O. + nonisolated static func dedupeContestedPRs( + _ picks: [ReconcileBranchMatch], + identityBySession: [UUID: String] + ) -> [ReconcileBranchMatch] { + let byURL = Dictionary(grouping: picks, by: { $0.url }) + var out: [ReconcileBranchMatch] = [] + for (_, group) in byURL { + let identities = Set(group.compactMap { identityBySession[$0.sessionID] }) + if identities.count > 1 { continue } // contested across tickets → none + out.append(contentsOf: group) + } + return out + } + /// Route a reconcile candidate to a *code* backend. A task-only provider /// (`.jira`/`.corveil`) has no code surface, so a session tracked by one /// resolves PRs through its `codeProvider` — mirroring the @@ -1130,7 +1150,15 @@ final class IssueTracker { // session resolve to a single best pick. matches.append(contentsOf: await fetchPRsByKeyForReconcile(candidates: keyCandidates)) - applyReconciledPRLinks(Self.decideReconcileLinks(matches: matches)) + // Each session's work-item identity (key preferred, else branch) so the + // de-dup pass can tell a legitimate duplicate-session match from one PR + // being claimed by two different tickets. + var identityBySession: [UUID: String] = [:] + for c in candidates { identityBySession[c.sessionID] = c.branch } + for c in keyCandidates { identityBySession[c.sessionID] = c.key } + + let decided = Self.decideReconcileLinks(matches: matches) + applyReconciledPRLinks(Self.dedupeContestedPRs(decided, identityBySession: identityBySession)) } /// Walk appState and build the set of sessions needing a reconcile pass. @@ -1191,14 +1219,26 @@ final class IssueTracker { let links = appState.links(for: session.id) guard !links.contains(where: { $0.linkType == .pr }) else { continue } - // Only task-only trackers whose PR branch won't match the worktree. - guard let ticketURL = session.ticketURL, - Validation.isJiraSpec(ticketURL), - let key = Validation.jiraKey(from: ticketURL) else { continue } - let wts = appState.worktrees(for: session.id) guard let primaryWt = wts.first(where: { $0.isPrimary }) ?? wts.first else { continue } + // Resolve the ticket key: prefer a Jira ticket URL, else derive it + // from the worktree branch (e.g. `feature/max-monorepo-maxx-7035-…` + // → `MAXX-7035`). The branch fallback covers the prefix-drop case + // where the PR head loses the repo prefix the worktree carries (#520). + // + // The branch fallback is gated to task-only trackers (Jira/Corveil): + // a lowercased branch can't distinguish a real Jira project ("maxx") + // from an ordinary word/repo segment ("api"), so a GitHub/GitLab + // issue branch like `feature/acme-api-197-fix` would yield a bogus + // "API-197" key. Those sessions resolve via the branch path instead. + let urlKey = session.ticketURL.flatMap { + Validation.isJiraSpec($0) ? Validation.jiraKey(from: $0) : nil + } + let branchKey = (session.provider?.isTaskOnly == true) + ? Validation.ticketKey(fromBranch: primaryWt.branch) : nil + guard let key = urlKey ?? branchKey else { continue } + let info = resolveRepoInfo(worktree: primaryWt) guard !info.slug.isEmpty else { continue } diff --git a/Tests/CrowTests/IssueTrackerReconcileTests.swift b/Tests/CrowTests/IssueTrackerReconcileTests.swift index 9b82d2e4..a8e92577 100644 --- a/Tests/CrowTests/IssueTrackerReconcileTests.swift +++ b/Tests/CrowTests/IssueTrackerReconcileTests.swift @@ -378,3 +378,111 @@ struct IssueTrackerReconcileKeyFanOutTests { #expect(fanned[0].number == 1) } } + +@Suite("IssueTracker reconcile cross-session de-dup") +struct IssueTrackerReconcileDedupTests { + + private func match(_ sid: UUID, number: Int, url: String, state: String = "OPEN") + -> IssueTracker.ReconcileBranchMatch { + IssueTracker.ReconcileBranchMatch( + sessionID: sid, number: number, url: url, state: state, updatedAt: nil + ) + } + + @Test func dropsPRContestedAcrossDistinctTickets() { + // One PR claimed by two sessions with different ticket keys can't be + // attributed to one of them → attach to neither (#520: MAXX-6854 has no + // PR yet was shown #171). + let s7035 = UUID(); let s6854 = UUID() + let url = "https://github.com/rm/max-monorepo/pull/171" + let out = IssueTracker.dedupeContestedPRs( + [match(s7035, number: 171, url: url), match(s6854, number: 171, url: url)], + identityBySession: [s7035: "MAXX-7035", s6854: "MAXX-6854"] + ) + #expect(out.isEmpty) + } + + @Test func keepsPRSharedByDuplicateSessionsWithSameIdentity() { + // Two sessions on the *same* ticket (a duplicated Jira session) both + // legitimately link the same PR. + let a = UUID(); let b = UUID() + let url = "https://github.com/rm/max-monorepo/pull/52" + let out = IssueTracker.dedupeContestedPRs( + [match(a, number: 52, url: url), match(b, number: 52, url: url)], + identityBySession: [a: "MAXX-6859", b: "MAXX-6859"] + ) + #expect(out.count == 2) + #expect(Set(out.map(\.sessionID)) == [a, b]) + } + + @Test func keepsDistinctPRsForDistinctSessions() { + let s1 = UUID(); let s2 = UUID() + let out = IssueTracker.dedupeContestedPRs( + [ + match(s1, number: 131, url: "https://github.com/rm/max-monorepo/pull/131"), + match(s2, number: 132, url: "https://github.com/rm/max-monorepo/pull/132"), + ], + identityBySession: [s1: "MAXX-6971", s2: "MAXX-6972"] + ) + #expect(out.count == 2) + } +} + +// Regression for the exact #520 max-monorepo scenario, exercised end-to-end at +// the pure-function level: worktree branch `feature/{repo}-{ticket}-{slug}` vs +// PR head `feature/{ticket}-{slug}`. The key (e.g. MAXX-7035) is derived from +// the branch; the backend post-filters matches to head/title (no body), so a +// no-PR session resolves to nothing rather than a phantom PR. +@Suite("IssueTracker reconcile key scenario (#520)") +struct IssueTrackerReconcileKeyScenarioTests { + + private func keyCandidate(_ sid: UUID, _ key: String) -> IssueTracker.ReconcileKeyCandidate { + IssueTracker.ReconcileKeyCandidate( + sessionID: sid, provider: .github, repoSlug: "rm/max-monorepo", key: key, gitlabHost: nil + ) + } + + @Test func branchDerivedKeyResolvesCorrectPRAndNoPRSessionGetsNone() { + let s7035 = UUID() // citations-chain-of-custody → PR #171 + let s7031 = UUID() // documents-observability-ui → PR #168 + let s6854 = UUID() // ut-cascade-dual-write → NO PR exists + + // The branch ticket key MAXX-7035 is what `Validation.ticketKey` pulls + // from `feature/max-monorepo-maxx-7035-citations-chain-of-custody`. + #expect( + Validation.ticketKey(fromBranch: "feature/max-monorepo-maxx-7035-citations-chain-of-custody") + == "MAXX-7035" + ) + + let candidates = [ + keyCandidate(s7035, "MAXX-7035"), + keyCandidate(s7031, "MAXX-7031"), + keyCandidate(s6854, "MAXX-6854"), + ] + + // Backend matches are already post-filtered to head/title. #171's title + // carries MAXX-7035; #168's carries MAXX-7031. MAXX-6854 has no PR — and + // #171 only *mentions* it in its body, which the backend no longer + // matches, so MAXX-6854 produces no match. + let kc7035 = KeyCandidate(repoSlug: "rm/max-monorepo", key: "MAXX-7035") + let kc7031 = KeyCandidate(repoSlug: "rm/max-monorepo", key: "MAXX-7031") + let backendMatches = [ + KeyPRMatch(candidate: kc7035, number: 171, + url: "https://github.com/rm/max-monorepo/pull/171", state: "OPEN", updatedAt: nil), + KeyPRMatch(candidate: kc7031, number: 168, + url: "https://github.com/rm/max-monorepo/pull/168", state: "OPEN", updatedAt: nil), + ] + + let fanned = IssueTracker.fanOutKeyMatches(backendMatches, across: candidates) + let decided = IssueTracker.decideReconcileLinks(matches: fanned) + let identity = [s7035: "MAXX-7035", s7031: "MAXX-7031", s6854: "MAXX-6854"] + let final = IssueTracker.dedupeContestedPRs(decided, identityBySession: identity) + + let bySession = Dictionary(grouping: final, by: { $0.sessionID }) + #expect(bySession[s7035]?.map(\.number) == [171]) + #expect(bySession[s7031]?.map(\.number) == [168]) + #expect(bySession[s6854] == nil) // no PR — never a phantom one + // No PR attaches to two distinct sessions. + #expect(Set(final.map(\.url)).count == final.count) + } +}