From c1825c201fd7e682ece559d45a58565337787e8f Mon Sep 17 00:00:00 2001 From: Dustin Hilgaertner Date: Mon, 4 May 2026 17:28:42 -0500 Subject: [PATCH] Route stale-PR follow-up by provider so merged GitLab MRs poll correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 60s polling cycle was hardcoding `gh api graphql` for every stale PR URL — including GitLab MRs. One GitLab URL in the batch failed the whole gh call (`Could not resolve to a Repository with the name 'big-bang/product'`), so `fetchStalePRStates` returned nil and no PR (GitHub or GitLab) got its merged state applied. That's why merged PRs only flipped to purple on app launch. Split URLs by provider: GitHub PRs go through the existing batched aliased GraphQL query; GitLab MRs go through one `glab api projects/{slug}/merge_requests/{iid}` per MR with `GITLAB_HOST` set per host. Return a small `StalePRFetchResult` struct so a single-provider failure no longer masks the other provider's PRs — partial-success now flows through to `applyPRStatuses` while `prDataComplete=false` still blocks auto-completion on degraded data. Closes #241 --- Sources/Crow/App/IssueTracker.swift | 211 ++++++++++++++--- .../CrowTests/IssueTrackerStalePRTests.swift | 218 ++++++++++++++++++ 2 files changed, 403 insertions(+), 26 deletions(-) create mode 100644 Tests/CrowTests/IssueTrackerStalePRTests.swift diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 9084b1b..3a2f3eb 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -274,16 +274,17 @@ final class IssueTracker { // for every viewer (which routinely returned 100 PRs / ~86 KB). let openPRURLs = Set(ghResult.viewerPRs.map(\.url)) let staleCandidateURLs = collectStalePRURLs(excluding: openPRURLs) - // nil here means the follow-up fetch errored (rate limit, exit != 0, - // parse failure). We thread that through to auto-complete so - // "PR missing from payload" doesn't get treated as "PR is closed" - // on a degraded response. An empty list with no candidate URLs is - // a real empty success. - let staleFetchResult: [ViewerPR]? = staleCandidateURLs.isEmpty - ? [] + // `complete == false` means at least one provider's follow-up errored + // (rate limit, exit != 0, parse failure). We thread that through to + // auto-complete so "PR missing from payload" doesn't get treated as + // "PR is closed" on a degraded response. Partial-success is allowed: + // PRs from the working provider still flow through so merged badges + // can flip even if the other provider failed. + let staleFetch = staleCandidateURLs.isEmpty + ? StalePRFetchResult(prs: [], complete: true) : await fetchStalePRStates(urls: staleCandidateURLs) - let stalePRs = staleFetchResult ?? [] - let prDataComplete = staleFetchResult != nil + let stalePRs = staleFetch.prs + let prDataComplete = staleFetch.complete let allKnownPRs = Self.dedupedByURL(ghResult.viewerPRs + stalePRs) applyPRStatuses(viewerPRs: allKnownPRs) @@ -665,20 +666,73 @@ final class IssueTracker { return Array(urls) } - /// Fetch state for a small set of PRs in one aliased GraphQL query. - /// Used for PRs that are linked to a session but are no longer in the - /// open viewer set (typically merged or closed). Returns minimal `ViewerPR` - /// records — only `state`, `url`, repo, and branch refs are populated; - /// checks/reviews are left empty since they're moot for closed PRs. - /// Returns `nil` if the shell call or response parse fails, so callers - /// can distinguish a partial fetch from a successful empty result. - private func fetchStalePRStates(urls: [String]) async -> [ViewerPR]? { + /// Result of a stale-PR follow-up: any PRs successfully fetched, plus + /// whether every provider call returned cleanly. `complete == false` + /// signals downstream auto-completion to treat the cycle as degraded. + private struct StalePRFetchResult { + var prs: [ViewerPR] + var complete: Bool + } + + /// Fetch state for a small set of PRs/MRs that are linked to a session + /// but no longer in the open viewer set (typically merged or closed). + /// Splits URLs by provider — GitHub PRs go through one batched aliased + /// `gh api graphql` call, GitLab MRs go through one `glab api` call per + /// MR (with `GITLAB_HOST` set per host). A failure on either side marks + /// the result incomplete but doesn't suppress the other side's PRs. + /// Returns minimal `ViewerPR` records — only `state`, `url`, repo, and + /// branch refs are populated; checks/reviews are left empty since + /// they're moot for closed PRs. + private func fetchStalePRStates(urls: [String]) async -> StalePRFetchResult { // Parse each URL into (owner, repo, number); skip any we can't parse. - var parsed: [(url: String, owner: String, repo: String, number: Int)] = [] + var githubParsed: [(url: String, owner: String, repo: String, number: Int)] = [] + var gitlabParsedByHost: [String: [(url: String, slug: String, number: Int)]] = [:] for url in urls { + if let g = Self.parseGitLabMRURL(url) { + gitlabParsedByHost[g.host, default: []].append((url, g.slug, g.number)) + continue + } guard let p = ProviderManager.parseTicketURLComponents(url) else { continue } - parsed.append((url, p.org, p.repo, p.number)) + // Anything not parsed as GitLab is treated as GitHub. The + // ProviderManager parser covers github.com URLs; self-hosted + // GitLab URLs are caught above by `parseGitLabMRURL`. + if let host = URL(string: url)?.host, host != "github.com" { + // Unrecognized host that didn't match the GitLab MR shape — + // skip rather than blindly route to gh. + continue + } + githubParsed.append((url, p.org, p.repo, p.number)) + } + guard !githubParsed.isEmpty || !gitlabParsedByHost.isEmpty else { + return StalePRFetchResult(prs: [], complete: true) + } + + var prs: [ViewerPR] = [] + var complete = true + + if !githubParsed.isEmpty { + if let ghPRs = await fetchStalePRStatesGitHub(parsed: githubParsed) { + prs.append(contentsOf: ghPRs) + } else { + complete = false + } + } + + for (host, parsed) in gitlabParsedByHost { + let (mrPRs, ok) = await fetchStaleMRStatesGitLab(parsed: parsed, host: host) + prs.append(contentsOf: mrPRs) + if !ok { complete = false } } + + return StalePRFetchResult(prs: prs, complete: complete) + } + + /// GitHub stale-PR fetch: one aliased GraphQL query covering every + /// (owner, repo, number) tuple. Returns `nil` on shell error or rate + /// limit so the caller can mark the cycle incomplete. + private func fetchStalePRStatesGitHub( + parsed: [(url: String, owner: String, repo: String, number: Int)] + ) async -> [ViewerPR]? { guard !parsed.isEmpty else { return [] } // Build aliased query: pr0, pr1, ... each fetching one pullRequest. @@ -719,6 +773,117 @@ final class IssueTracker { return parseStalePRResponse(result.stdout, count: parsed.count) } + /// GitLab stale-MR fetch: one `glab api projects/{slug}/merge_requests/{iid}` + /// per MR for a given host. GitLab's REST API doesn't support batching by + /// IDs the way GitHub's GraphQL does, but the per-cycle stale set is + /// usually tiny (sessions with merged/closed PRs that haven't been + /// auto-completed yet). Returns `(prs, ok)` where `ok` is false if any + /// call for this host failed. + private func fetchStaleMRStatesGitLab( + parsed: [(url: String, slug: String, number: Int)], + host: String + ) async -> ([ViewerPR], Bool) { + var prs: [ViewerPR] = [] + var ok = true + for entry in parsed { + let encodedSlug = entry.slug.addingPercentEncoding( + withAllowedCharacters: .alphanumerics + ) ?? entry.slug + let endpoint = "projects/\(encodedSlug)/merge_requests/\(entry.number)" + + let output: String + do { + output = try await shell(env: ["GITLAB_HOST": host], cwd: NSHomeDirectory(), "glab", "api", endpoint) + } catch { + print("[IssueTracker] Stale-PR follow-up failed for \(entry.slug)!\(entry.number) on \(host): \(error.localizedDescription.prefix(200))") + ok = false + continue + } + if let pr = Self.parseGitLabStaleMRResponse(output, fallbackURL: entry.url, fallbackSlug: entry.slug) { + prs.append(pr) + } else { + ok = false + } + } + return (prs, ok) + } + + /// Parse a GitLab `projects/{slug}/merge_requests/{iid}` REST response + /// into a minimal `ViewerPR`. State is normalized to GitHub's + /// `OPEN|MERGED|CLOSED` so downstream code stays provider-agnostic. + /// Returns nil if the JSON shape doesn't match. + nonisolated static func parseGitLabStaleMRResponse( + _ output: String, + fallbackURL: String, + fallbackSlug: String + ) -> ViewerPR? { + guard let data = output.data(using: .utf8), + let item = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { + return nil + } + guard let number = item["iid"] as? Int else { return nil } + let url = (item["web_url"] as? String) ?? fallbackURL + let rawState = (item["state"] as? String) ?? "" + let state = normalizeGitLabPRState(rawState) + let headRefName = (item["source_branch"] as? String) ?? "" + let baseRefName = (item["target_branch"] as? String) ?? "" + let headRefOid = (item["sha"] as? String) ?? "" + let isDraft = (item["draft"] as? Bool) ?? (item["work_in_progress"] as? Bool) ?? false + + return ViewerPR( + number: number, + url: url, + state: state, + mergeable: "UNKNOWN", + reviewDecision: "", + isDraft: isDraft, + headRefName: headRefName, + headRefOid: headRefOid, + baseRefName: baseRefName, + repoNameWithOwner: fallbackSlug, + linkedIssueReferences: [], + checksState: "", + failedCheckNames: [], + latestReviewStates: [] + ) + } + + /// Normalize GitLab MR state strings to the GitHub `state` vocabulary + /// the rest of the codebase reads (`OPEN`/`MERGED`/`CLOSED`). Falls back + /// to upper-casing the raw value for unrecognized states (matches + /// `fetchGitLabMRsForReconcile`). + nonisolated static func normalizeGitLabPRState(_ raw: String) -> String { + switch raw { + case "opened": return "OPEN" + case "merged": return "MERGED" + case "closed": return "CLOSED" + default: return raw.uppercased() + } + } + + /// Parse a GitLab MR URL into (host, slug, number). Robust to nested + /// groups (slug is everything between the host and `/-/merge_requests/`). + /// Returns nil for non-GitLab-MR URLs. + nonisolated static func parseGitLabMRURL(_ url: String) -> (host: String, slug: String, number: Int)? { + guard let protoRange = url.range(of: "://") else { return nil } + let afterProto = String(url[protoRange.upperBound...]) + guard let mrRange = afterProto.range(of: "/-/merge_requests/") else { return nil } + let leading = String(afterProto[..= 3 else { return nil } + let host = leadParts[0] + let slug = leadParts.dropFirst().joined(separator: "/") + + // `trailing` is everything after `/-/merge_requests/` — usually just + // the MR number, occasionally `/diffs` or similar from a deep + // link. Take the first segment as the number. + let trailParts = trailing.split(separator: "/").map(String.init) + guard let first = trailParts.first, let number = Int(first) else { return nil } + return (host, slug, number) + } + private func parseStalePRResponse(_ output: String, count: Int) -> [ViewerPR]? { guard let data = output.data(using: .utf8), let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any], @@ -1276,13 +1441,7 @@ final class IssueTracker { guard let number = item["iid"] as? Int, let url = item["web_url"] as? String else { continue } let rawState = (item["state"] as? String) ?? "" - let normalized: String - switch rawState { - case "opened": normalized = "OPEN" - case "merged": normalized = "MERGED" - case "closed": normalized = "CLOSED" - default: normalized = rawState.uppercased() - } + let normalized = Self.normalizeGitLabPRState(rawState) let updatedAt = (item["updated_at"] as? String).flatMap { dateFormatter.date(from: $0) } matches.append(ReconcileBranchMatch( sessionID: candidate.sessionID, diff --git a/Tests/CrowTests/IssueTrackerStalePRTests.swift b/Tests/CrowTests/IssueTrackerStalePRTests.swift new file mode 100644 index 0000000..8cff4be --- /dev/null +++ b/Tests/CrowTests/IssueTrackerStalePRTests.swift @@ -0,0 +1,218 @@ +import Foundation +import Testing +@testable import Crow + +@Suite("IssueTracker stale-PR follow-up") +struct IssueTrackerStalePRTests { + + // MARK: - GitLab MR URL parsing + + @Test func parsesTopLevelGitLabMRURL() { + let parsed = IssueTracker.parseGitLabMRURL( + "https://repo1.dso.mil/big-bang/product/-/merge_requests/123" + ) + #expect(parsed?.host == "repo1.dso.mil") + #expect(parsed?.slug == "big-bang/product") + #expect(parsed?.number == 123) + } + + @Test func parsesNestedGroupGitLabMRURL() { + // Regression guard for nested groups. The two-segment heuristic in + // ProviderManager.parseTicketURLComponents would truncate this to + // "big-bang/product" — the dedicated parser must keep the full path. + let parsed = IssueTracker.parseGitLabMRURL( + "https://repo1.dso.mil/big-bang/product/packages/elasticsearch-kibana/-/merge_requests/7" + ) + #expect(parsed?.host == "repo1.dso.mil") + #expect(parsed?.slug == "big-bang/product/packages/elasticsearch-kibana") + #expect(parsed?.number == 7) + } + + @Test func parsesGitLabComMRURL() { + let parsed = IssueTracker.parseGitLabMRURL( + "https://gitlab.com/foo/bar/-/merge_requests/42" + ) + #expect(parsed?.host == "gitlab.com") + #expect(parsed?.slug == "foo/bar") + #expect(parsed?.number == 42) + } + + @Test func rejectsGitHubPRURL() { + // GitHub PR URLs lack `/-/merge_requests/`, so the GitLab parser + // must return nil and let the caller fall through to the GitHub path. + let parsed = IssueTracker.parseGitLabMRURL( + "https://github.com/foo/bar/pull/9" + ) + #expect(parsed == nil) + } + + @Test func rejectsGitLabIssueURL() { + // `/-/issues/` ≠ `/-/merge_requests/`. Issue URLs aren't in scope + // for the stale-PR fetch. + let parsed = IssueTracker.parseGitLabMRURL( + "https://gitlab.com/foo/bar/-/issues/3" + ) + #expect(parsed == nil) + } + + @Test func rejectsMalformedURL() { + #expect(IssueTracker.parseGitLabMRURL("not a url") == nil) + #expect(IssueTracker.parseGitLabMRURL("https://gitlab.com/foo") == nil) + #expect(IssueTracker.parseGitLabMRURL( + "https://gitlab.com/foo/bar/-/merge_requests/notanumber" + ) == nil) + } + + // MARK: - GitLab state normalization + + @Test func normalizesOpenedToOPEN() { + #expect(IssueTracker.normalizeGitLabPRState("opened") == "OPEN") + } + + @Test func normalizesMergedToMERGED() { + #expect(IssueTracker.normalizeGitLabPRState("merged") == "MERGED") + } + + @Test func normalizesClosedToCLOSED() { + #expect(IssueTracker.normalizeGitLabPRState("closed") == "CLOSED") + } + + @Test func upcasesUnknownState() { + #expect(IssueTracker.normalizeGitLabPRState("locked") == "LOCKED") + #expect(IssueTracker.normalizeGitLabPRState("") == "") + } + + // MARK: - GitLab MR JSON parsing + + @Test func parsesMergedMRResponse() { + let json = """ + { + "iid": 7, + "web_url": "https://repo1.dso.mil/big-bang/product/-/merge_requests/7", + "state": "merged", + "source_branch": "feature/x", + "target_branch": "master", + "sha": "deadbeef", + "draft": false + } + """ + let pr = IssueTracker.parseGitLabStaleMRResponse( + json, + fallbackURL: "https://repo1.dso.mil/big-bang/product/-/merge_requests/7", + fallbackSlug: "big-bang/product" + ) + #expect(pr?.number == 7) + #expect(pr?.state == "MERGED") + #expect(pr?.url == "https://repo1.dso.mil/big-bang/product/-/merge_requests/7") + #expect(pr?.headRefName == "feature/x") + #expect(pr?.baseRefName == "master") + #expect(pr?.headRefOid == "deadbeef") + #expect(pr?.repoNameWithOwner == "big-bang/product") + #expect(pr?.isDraft == false) + } + + @Test func parsesOpenedMRResponse() { + let json = """ + {"iid": 12, "web_url": "https://gitlab.com/foo/bar/-/merge_requests/12", "state": "opened"} + """ + let pr = IssueTracker.parseGitLabStaleMRResponse( + json, fallbackURL: "fallback", fallbackSlug: "foo/bar" + ) + #expect(pr?.state == "OPEN") + #expect(pr?.number == 12) + } + + @Test func parsesClosedMRResponse() { + let json = """ + {"iid": 99, "web_url": "https://gitlab.com/foo/bar/-/merge_requests/99", "state": "closed"} + """ + let pr = IssueTracker.parseGitLabStaleMRResponse( + json, fallbackURL: "fallback", fallbackSlug: "foo/bar" + ) + #expect(pr?.state == "CLOSED") + } + + @Test func usesFallbackURLWhenWebUrlMissing() { + // Defensive: if a future GitLab API change drops `web_url`, we still + // produce a usable record using the URL we already had on hand. + let json = """ + {"iid": 5, "state": "merged"} + """ + let pr = IssueTracker.parseGitLabStaleMRResponse( + json, + fallbackURL: "https://gitlab.com/foo/bar/-/merge_requests/5", + fallbackSlug: "foo/bar" + ) + #expect(pr?.url == "https://gitlab.com/foo/bar/-/merge_requests/5") + #expect(pr?.state == "MERGED") + } + + @Test func returnsNilForMalformedJSON() { + #expect(IssueTracker.parseGitLabStaleMRResponse( + "not json", + fallbackURL: "x", + fallbackSlug: "y" + ) == nil) + // Missing the required `iid` field. + #expect(IssueTracker.parseGitLabStaleMRResponse( + #"{"state": "merged"}"#, + fallbackURL: "x", + fallbackSlug: "y" + ) == nil) + } + + @Test func acceptsLegacyWorkInProgressFlag() { + // Older GitLab versions used `work_in_progress` instead of `draft`. + let json = """ + {"iid": 1, "state": "opened", "work_in_progress": true} + """ + let pr = IssueTracker.parseGitLabStaleMRResponse( + json, fallbackURL: "x", fallbackSlug: "y" + ) + #expect(pr?.isDraft == true) + } + + // MARK: - Mixed-provider dedup integration + + @Test func dedupesGitHubAndGitLabPRsByURL() { + // The orchestrator concats viewer (open) PRs with stale PRs from + // both providers and runs the result through `dedupedByURL`. Verify + // that a GitLab MR with the same URL appearing twice (once OPEN + // from a prior reconcile, once MERGED from this cycle's stale fetch) + // collapses with MERGED winning. + let mrURL = "https://repo1.dso.mil/big-bang/product/-/merge_requests/7" + let prURL = "https://github.com/radiusmethod/corveil/pull/201" + let prs = [ + makeViewerPR(url: prURL, state: "OPEN"), + makeViewerPR(url: mrURL, state: "OPEN"), + makeViewerPR(url: prURL, state: "MERGED"), + makeViewerPR(url: mrURL, state: "MERGED"), + ] + let result = IssueTracker.dedupedByURL(prs) + #expect(result.count == 2) + let byURL = Dictionary(uniqueKeysWithValues: result.map { ($0.url, $0) }) + #expect(byURL[prURL]?.state == "MERGED") + #expect(byURL[mrURL]?.state == "MERGED") + } + + // MARK: - helpers + + private func makeViewerPR(url: String, state: String, number: Int = 1) -> IssueTracker.ViewerPR { + IssueTracker.ViewerPR( + number: number, + url: url, + state: state, + mergeable: "UNKNOWN", + reviewDecision: "", + isDraft: false, + headRefName: "", + headRefOid: "", + baseRefName: "", + repoNameWithOwner: "", + linkedIssueReferences: [], + checksState: "", + failedCheckNames: [], + latestReviewStates: [] + ) + } +}