diff --git a/docs/components/github-pull-requests.md b/docs/components/github-pull-requests.md index 79bcb164..4fbcb3a6 100644 --- a/docs/components/github-pull-requests.md +++ b/docs/components/github-pull-requests.md @@ -17,7 +17,9 @@ itself. If a repository has multiple GitHub remotes, Prowl checks each remote for a PR on the worktree branch. `origin` is preferred, `upstream` comes next, and other named remotes are used alphabetically, so fork-based worktrees can show upstream -PRs without changing `origin` or restarting the app. +PRs without changing `origin` or restarting the app. A returned PR's head +repository must match one of the repository's configured GitHub remotes; PRs +from unrelated forks that happen to use the same branch name are ignored. Prowl also watches the repository's git config while the app is running. When remote URLs are added, removed, or changed, it refreshes the repository's PR diff --git a/supacode/Clients/Github/GithubCLIClient.swift b/supacode/Clients/Github/GithubCLIClient.swift index 25988a1e..94f4dd5d 100644 --- a/supacode/Clients/Github/GithubCLIClient.swift +++ b/supacode/Clients/Github/GithubCLIClient.swift @@ -416,7 +416,12 @@ nonisolated private func sanitizeCrossRepoRequests( continue } sanitized.append( - CrossRepoPullRequestRequest(owner: request.owner, repo: request.repo, branches: branches) + CrossRepoPullRequestRequest( + owner: request.owner, + repo: request.repo, + branches: branches, + allowedHeadRepositories: request.allowedHeadRepositories + ) ) } return sanitized @@ -547,8 +552,7 @@ nonisolated private func fetchCrossRepoChunk( let prs = rankCrossRepoPullRequests( pullRequestsByAlias: payload.pullRequestsByAlias, aliasMap: branchAliasMap, - owner: key.owner, - repo: key.repo + allowedHeadRepositories: plan.allowedHeadRepositoriesByRepoAlias[alias] ?? [key] ) success[key] = prs } @@ -559,6 +563,7 @@ nonisolated private struct CrossRepoBatchQueryPlan: Sendable { let query: String let repoAliases: [String: RepoKey] let branchAliasesByRepo: [String: [String: String]] + let allowedHeadRepositoriesByRepoAlias: [String: Set] } nonisolated private func makeCrossRepoBatchQuery( @@ -566,11 +571,13 @@ nonisolated private func makeCrossRepoBatchQuery( ) -> CrossRepoBatchQueryPlan { var repoAliases: [String: RepoKey] = [:] var branchAliasesByRepo: [String: [String: String]] = [:] + var allowedHeadRepositoriesByRepoAlias: [String: Set] = [:] var repoBlocks: [String] = [] let orderBy = "orderBy: {field: UPDATED_AT, direction: DESC}" for (repoIndex, request) in requests.enumerated() { let repoAlias = "r\(repoIndex)" repoAliases[repoAlias] = RepoKey(owner: request.owner, repo: request.repo) + allowedHeadRepositoriesByRepoAlias[repoAlias] = request.allowedHeadRepositories var branchAliasMap: [String: String] = [:] var selections: [String] = [] for (branchIndex, branch) in request.branches.enumerated() { @@ -653,55 +660,22 @@ nonisolated private func makeCrossRepoBatchQuery( return CrossRepoBatchQueryPlan( query: query, repoAliases: repoAliases, - branchAliasesByRepo: branchAliasesByRepo + branchAliasesByRepo: branchAliasesByRepo, + allowedHeadRepositoriesByRepoAlias: allowedHeadRepositoriesByRepoAlias ) } nonisolated private func rankCrossRepoPullRequests( pullRequestsByAlias: [String: GithubGraphQLPullRequestResponse.PullRequestConnection], aliasMap: [String: String], - owner: String, - repo: String + allowedHeadRepositories: Set ) -> [String: GithubPullRequest] { - let normalizedOwner = owner.lowercased() - let normalizedRepo = repo.lowercased() var results: [String: GithubPullRequest] = [:] for (alias, connection) in pullRequestsByAlias { guard let branch = aliasMap[alias] else { continue } - let upstreamCandidates = connection.nodes.filter { - $0.matches(owner: normalizedOwner, repo: normalizedRepo) - } - let candidates: [GithubGraphQLPullRequestResponse.PullRequestNode] - if !upstreamCandidates.isEmpty { - candidates = upstreamCandidates - } else { - let forkCandidates = connection.nodes.filter { - $0.headRepository != nil && $0.doesNotTargetSameBranch(branch) - } - candidates = - if !forkCandidates.isEmpty { - forkCandidates - } else { - connection.nodes.filter { - $0.headRepository == nil && $0.doesNotTargetSameBranch(branch) - } - } - } - if let node = candidates.max(by: { left, right in - let leftRank = left.stateRank - let rightRank = right.stateRank - if leftRank != rightRank { - return leftRank < rightRank - } - let leftDate = left.updatedAt ?? .distantPast - let rightDate = right.updatedAt ?? .distantPast - if leftDate != rightDate { - return leftDate < rightDate - } - return left.number < right.number - }) { + if let node = connection.bestMatchingPullRequest(allowedHeadRepositories: allowedHeadRepositories) { results[branch] = node.pullRequest } } diff --git a/supacode/Clients/Github/GithubCLIModels.swift b/supacode/Clients/Github/GithubCLIModels.swift index c7b93f55..8284aee2 100644 --- a/supacode/Clients/Github/GithubCLIModels.swift +++ b/supacode/Clients/Github/GithubCLIModels.swift @@ -145,6 +145,19 @@ nonisolated struct CrossRepoPullRequestRequest: Sendable, Hashable { let owner: String let repo: String let branches: [String] + let allowedHeadRepositories: Set + + init( + owner: String, + repo: String, + branches: [String], + allowedHeadRepositories: Set? = nil + ) { + self.owner = owner + self.repo = repo + self.branches = branches + self.allowedHeadRepositories = allowedHeadRepositories ?? [RepoKey(owner: owner, repo: repo)] + } var key: RepoKey { RepoKey(owner: owner, repo: repo) diff --git a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift index e2337a91..4e6ceb0a 100644 --- a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift +++ b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift @@ -8,45 +8,12 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { owner: String, repo: String ) -> [String: GithubPullRequest] { - let normalizedOwner = owner.lowercased() - let normalizedRepo = repo.lowercased() var results: [String: GithubPullRequest] = [:] for (alias, connection) in data.repository.pullRequestsByAlias { guard let branch = aliasMap[alias] else { continue } - let upstreamCandidates = connection.nodes.filter { $0.matches(owner: normalizedOwner, repo: normalizedRepo) } - let candidates: [PullRequestNode] - if !upstreamCandidates.isEmpty { - candidates = upstreamCandidates - } else { - // Without an upstream-repository match, same-name base branches are likely from unrelated - // fork workflows and can shadow the local worktree branch this app is trying to resolve. - let forkCandidates = connection.nodes.filter { - $0.headRepository != nil && $0.doesNotTargetSameBranch(branch) - } - candidates = - if !forkCandidates.isEmpty { - forkCandidates - } else { - connection.nodes.filter { - $0.headRepository == nil && $0.doesNotTargetSameBranch(branch) - } - } - } - if let node = candidates.max(by: { left, right in - let leftRank = left.stateRank - let rightRank = right.stateRank - if leftRank != rightRank { - return leftRank < rightRank - } - let leftDate = left.updatedAt ?? .distantPast - let rightDate = right.updatedAt ?? .distantPast - if leftDate != rightDate { - return leftDate < rightDate - } - return left.number < right.number - }) { + if let node = connection.bestMatchingPullRequest(owner: owner, repo: repo) { results[branch] = node.pullRequest } } @@ -87,6 +54,31 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { nonisolated struct PullRequestConnection: Decodable { let nodes: [PullRequestNode] + + func bestMatchingPullRequest(owner: String, repo: String) -> PullRequestNode? { + bestMatchingPullRequest(allowedHeadRepositories: [RepoKey(owner: owner, repo: repo)]) + } + + func bestMatchingPullRequest(allowedHeadRepositories: Set) -> PullRequestNode? { + // GitHub's `headRefName` filter is repository-agnostic: forks can have the + // same branch name as the local worktree. Only a head repository matching + // one of the local GitHub remotes proves this PR belongs to this checkout. + let allowedKeys = Set(allowedHeadRepositories.map { $0.normalizedHeadRepositoryKey }) + let candidates = nodes.filter { $0.matchesAnyRepository(in: allowedKeys) } + return candidates.max(by: { left, right in + let leftRank = left.stateRank + let rightRank = right.stateRank + if leftRank != rightRank { + return leftRank < rightRank + } + let leftDate = left.updatedAt ?? .distantPast + let rightDate = right.updatedAt ?? .distantPast + if leftDate != rightDate { + return leftDate < rightDate + } + return left.number < right.number + }) + } } nonisolated struct PullRequestNode: Decodable { @@ -146,16 +138,16 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { guard let headRepository else { return false } - return headRepository.owner.login.lowercased() == owner - && headRepository.name.lowercased() == repo + return headRepository.normalizedKey == RepoKey(owner: owner, repo: repo).normalizedHeadRepositoryKey } - func doesNotTargetSameBranch(_ branch: String) -> Bool { - guard let baseRefName else { - return true + func matchesAnyRepository(in normalizedKeys: Set) -> Bool { + guard let headRepository else { + return false } - return baseRefName != branch + return normalizedKeys.contains(headRepository.normalizedKey) } + } nonisolated struct CommitConnection: Decodable { @@ -169,9 +161,19 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { nonisolated struct HeadRepository: Decodable { let name: String let owner: HeadRepositoryOwner + + nonisolated var normalizedKey: String { + RepoKey(owner: owner.login, repo: name).normalizedHeadRepositoryKey + } } nonisolated struct HeadRepositoryOwner: Decodable { let login: String } } + +extension RepoKey { + nonisolated var normalizedHeadRepositoryKey: String { + "\(owner.lowercased())/\(repo.lowercased())" + } +} diff --git a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift index ba7a1ce6..ca570830 100644 --- a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift +++ b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift @@ -232,7 +232,8 @@ final class PullRequestRefreshCoordinator { CrossRepoPullRequestRequest( owner: group.key.owner, repo: group.key.repo, - branches: group.branches + branches: group.branches, + allowedHeadRepositories: group.allowedHeadRepositories ) } do { @@ -398,8 +399,12 @@ final class PullRequestRefreshCoordinator { private func groupBranchesByRepo(_ requests: [Request]) -> [RepoKey: RepoRequestGroup] { var groupsByKey: [RepoKey: RepoRequestGroup] = [:] for request in requests { + let allowedHeadRepositories = Set(request.repositories.map(\.key)) for repository in request.repositories { - groupsByKey[repository.key, default: RepoRequestGroup(key: repository.key)].append(branches: request.branches) + groupsByKey[repository.key, default: RepoRequestGroup(key: repository.key)].append( + branches: request.branches, + allowedHeadRepositories: allowedHeadRepositories + ) } } return groupsByKey @@ -408,16 +413,21 @@ final class PullRequestRefreshCoordinator { private struct RepoRequestGroup: Sendable { let key: RepoKey private(set) var branches: [String] = [] + private(set) var allowedHeadRepositories: Set = [] private var seenBranches: Set = [] init(key: RepoKey) { self.key = key } - mutating func append(branches newBranches: [String]) { + mutating func append( + branches newBranches: [String], + allowedHeadRepositories newAllowedHeadRepositories: Set + ) { for branch in newBranches where seenBranches.insert(branch).inserted { branches.append(branch) } + allowedHeadRepositories.formUnion(newAllowedHeadRepositories) } } diff --git a/supacodeTests/GithubBatchPullRequestsTests.swift b/supacodeTests/GithubBatchPullRequestsTests.swift index 5b450ada..a66ebfec 100644 --- a/supacodeTests/GithubBatchPullRequestsTests.swift +++ b/supacodeTests/GithubBatchPullRequestsTests.swift @@ -66,7 +66,7 @@ struct GithubBatchPullRequestsTests { #expect(prs["feature-b"] == nil) } - @Test func fallsBackToForkOnlyMatches() throws { + @Test func ignoresForkOnlyMatches() throws { let json = """ { "data": { @@ -104,11 +104,10 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 9) - #expect(prs["feature-a"]?.title == "Fork PR") + #expect(prs["feature-a"] == nil) } - @Test func fallsBackToMergedPullRequestWithDeletedFork() throws { + @Test func ignoresPullRequestWithUnknownHeadRepository() throws { let json = """ { "data": { @@ -143,11 +142,10 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 7) - #expect(prs["feature-a"]?.title == "Deleted Fork") + #expect(prs["feature-a"] == nil) } - @Test func forkFallbackIgnoresSameBaseBranchMatches() throws { + @Test func ignoresForkEvenWhenBaseBranchDiffers() throws { let json = """ { "data": { @@ -203,8 +201,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 13) - #expect(prs["feature-a"]?.title == "Fork PR From Feature") + #expect(prs["feature-a"] == nil) } @Test func prefersOpenOverMergedEvenIfOlder() throws { diff --git a/supacodeTests/GithubCLIClientTests.swift b/supacodeTests/GithubCLIClientTests.swift index af4cb7a9..00e3bbe5 100644 --- a/supacodeTests/GithubCLIClientTests.swift +++ b/supacodeTests/GithubCLIClientTests.swift @@ -666,6 +666,67 @@ struct GithubCLIClientTests { #expect(pullRequest.number == 42) } + @Test func batchAcrossRepositoriesIgnoresForkOnlyPullRequestMatches() async throws { + let probe = GithubBatchShellProbe() + let shell = makeBatchAcrossShellMock(probe: probe) { arguments in + let stdout = crossRepoGraphQLResponseWithForkOnlyPR( + for: arguments, + prNumber: 2174 + ) + return ShellOutput(stdout: stdout, stderr: "", exitCode: 0) + } + let client = GithubCLIClient.live(shell: shell) + let requests = [ + CrossRepoPullRequestRequest(owner: "onevcat", repo: "Kingfisher", branches: ["master"]) + ] + + let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) + + let kingfisherPRs = try #require(result.successByRepo[RepoKey(owner: "onevcat", repo: "Kingfisher")]) + #expect(kingfisherPRs["master"] == nil) + } + + @Test func batchAcrossRepositoriesAllowsPullRequestFromConfiguredHeadRemote() async throws { + let probe = GithubBatchShellProbe() + let shell = makeBatchAcrossShellMock(probe: probe) { arguments in + let stdout = crossRepoGraphQLResponseWithHeadRepositoryPR( + for: arguments, + fixture: HeadRepositoryPRFixture( + baseOwner: "supabitapp", + baseRepo: "supacode", + headOwner: "onevcat", + headRepo: "Prowl", + branch: "feature" + ) + ) + return ShellOutput(stdout: stdout, stderr: "", exitCode: 0) + } + let client = GithubCLIClient.live(shell: shell) + let allowedHeadRepositories: Set = [ + RepoKey(owner: "onevcat", repo: "Prowl"), + RepoKey(owner: "supabitapp", repo: "supacode"), + ] + let requests = [ + CrossRepoPullRequestRequest( + owner: "onevcat", + repo: "Prowl", + branches: ["feature"], + allowedHeadRepositories: allowedHeadRepositories + ), + CrossRepoPullRequestRequest( + owner: "supabitapp", + repo: "supacode", + branches: ["feature"], + allowedHeadRepositories: allowedHeadRepositories + ), + ] + + let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) + + let upstreamPRs = try #require(result.successByRepo[RepoKey(owner: "supabitapp", repo: "supacode")]) + #expect(upstreamPRs["feature"]?.number == 42) + } + @Test func executableResolutionIsSingleFlightAndReused() async { let probe = GithubBatchShellProbe() let shell = ShellClient( @@ -872,7 +933,10 @@ nonisolated func crossRepoGraphQLResponseWithSinglePR( "baseRefName": "main", "commits": ["totalCount": 1], "author": ["login": "khoi"], - "headRepository": NSNull(), + "headRepository": [ + "name": entry.repo, + "owner": ["login": entry.owner], + ], "statusCheckRollup": NSNull(), ] aliasPayload[alias] = ["nodes": [node]] @@ -888,6 +952,112 @@ nonisolated func crossRepoGraphQLResponseWithSinglePR( return json } +nonisolated func crossRepoGraphQLResponseWithForkOnlyPR( + for arguments: [String], + prNumber: Int +) -> String { + guard let queryArgument = arguments.first(where: { $0.hasPrefix("query=") }) else { + return #"{"data":{}}"# + } + let structure = parseCrossRepoQuery(String(queryArgument.dropFirst("query=".count))) + var repositoryPayload: [String: Any] = [:] + for entry in structure.repos { + var aliasPayload: [String: Any] = [:] + for alias in entry.branchAliases { + let node: [String: Any] = [ + "number": prNumber, + "title": "Add extension to has image property components", + "state": "CLOSED", + "additions": 254, + "deletions": 70, + "isDraft": false, + "reviewDecision": NSNull(), + "mergeable": "CONFLICTING", + "mergeStateStatus": "DIRTY", + "url": "https://github.com/onevcat/Kingfisher/pull/2174", + "updatedAt": NSNull(), + "headRefName": "master", + "baseRefName": "v8", + "commits": ["totalCount": 11], + "author": ["login": "Mxlris"], + "headRepository": [ + "name": "Kingfisher", + "owner": ["login": "MxIris-Library-Forks"], + ], + "statusCheckRollup": NSNull(), + ] + aliasPayload[alias] = ["nodes": [node]] + } + repositoryPayload[entry.alias] = aliasPayload + } + let body: [String: Any] = ["data": repositoryPayload] + guard let data = try? JSONSerialization.data(withJSONObject: body), + let json = String(bytes: data, encoding: .utf8) + else { + return #"{"data":{}}"# + } + return json +} + +nonisolated struct HeadRepositoryPRFixture { + let baseOwner: String + let baseRepo: String + let headOwner: String + let headRepo: String + let branch: String +} + +nonisolated func crossRepoGraphQLResponseWithHeadRepositoryPR( + for arguments: [String], + fixture: HeadRepositoryPRFixture +) -> String { + guard let queryArgument = arguments.first(where: { $0.hasPrefix("query=") }) else { + return #"{"data":{}}"# + } + let structure = parseCrossRepoQuery(String(queryArgument.dropFirst("query=".count))) + var repositoryPayload: [String: Any] = [:] + for entry in structure.repos { + var aliasPayload: [String: Any] = [:] + for alias in entry.branchAliases { + if entry.owner == fixture.baseOwner, entry.repo == fixture.baseRepo { + let node: [String: Any] = [ + "number": 42, + "title": "Fork workflow PR", + "state": "OPEN", + "additions": 10, + "deletions": 2, + "isDraft": false, + "reviewDecision": NSNull(), + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "url": "https://github.com/\(fixture.baseOwner)/\(fixture.baseRepo)/pull/42", + "updatedAt": NSNull(), + "headRefName": fixture.branch, + "baseRefName": "main", + "commits": ["totalCount": 3], + "author": ["login": fixture.headOwner], + "headRepository": [ + "name": fixture.headRepo, + "owner": ["login": fixture.headOwner], + ], + "statusCheckRollup": NSNull(), + ] + aliasPayload[alias] = ["nodes": [node]] + } else { + aliasPayload[alias] = ["nodes": []] + } + } + repositoryPayload[entry.alias] = aliasPayload + } + let body: [String: Any] = ["data": repositoryPayload] + guard let data = try? JSONSerialization.data(withJSONObject: body), + let json = String(bytes: data, encoding: .utf8) + else { + return #"{"data":{}}"# + } + return json +} + nonisolated func makeBatchAcrossShellMock( probe: GithubBatchShellProbe, responseBuilder: @escaping @Sendable (_ arguments: [String]) async throws -> ShellOutput diff --git a/supacodeTests/PullRequestRefreshCoordinatorTests.swift b/supacodeTests/PullRequestRefreshCoordinatorTests.swift index ca403f2d..fe4e0a9c 100644 --- a/supacodeTests/PullRequestRefreshCoordinatorTests.swift +++ b/supacodeTests/PullRequestRefreshCoordinatorTests.swift @@ -329,6 +329,11 @@ struct PullRequestRefreshCoordinatorTests { let calls = await probe.batchedCalls() #expect(calls.count == 1) #expect(Set(calls.first?.requests.map(\.repo) ?? []) == ["fork", "upstream"]) + let expectedAllowedHeadRepositories: Set = [ + RepoKey(owner: "khoi", repo: "fork"), + RepoKey(owner: "khoi", repo: "upstream"), + ] + #expect(calls.first?.requests.allSatisfy { $0.allowedHeadRepositories == expectedAllowedHeadRepositories } == true) let refreshed = await outcomes.snapshot().compactMap { outcome -> [String: GithubPullRequest]? in if case .refreshed("local", _, _, let prsByBranch) = outcome {