diff --git a/CHANGELOG.md b/CHANGELOG.md index 675e92a..cccf075 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## 0.4.2 + +### Fixed + +- **Merge was offered on pull requests you cannot merge.** The list is every + open pull request you opened, including ones to projects you only contribute + to. When one of those was green, it got a Merge button that GitHub then + refused. It now sits under *In flight* as *no write access*. + ## 0.4.1 ### Changed diff --git a/README.md b/README.md index d7c00c4..b60e694 100644 --- a/README.md +++ b/README.md @@ -31,11 +31,12 @@ production deploy holding short of the runway is the top strip whichever repo it belongs to, because that is the one burning fuel. **No clearance GitHub wouldn't honour.** A pull request is only cleared to land -when its checks are green *and* GitHub reports no review or branch rule in the -way, and the merge is pinned to the commit GitCleared last had on radar — so a -push that arrives between sweeps is refused, not merged unseen. *Start a -workflow* lists only workflows that declare `workflow_dispatch`, with the -inputs each one's file declares. +when its checks are green, GitHub reports no review or branch rule in the way, +and you have write access to the repository. Your pull requests to other +people's projects are listed, but get no Merge button. A merge is pinned to the +commit GitCleared last had on radar, so a push that arrives between sweeps is +refused, not merged unseen. *Start a workflow* lists only workflows that declare +`workflow_dispatch`, with the inputs each one's file declares. ## Why there's a tower diff --git a/Scripts/asc.py b/Scripts/asc.py index b449a12..5b8d370 100755 --- a/Scripts/asc.py +++ b/Scripts/asc.py @@ -499,6 +499,12 @@ def exists(path: str) -> bool: print(f" availability: {len(territories)} territories, not {', '.join(sorted(EXCLUDED_TERRITORIES))}") +# Stands in, in review-notes.md, for the GitHub token App Review signs in with. The real +# token is supplied at push time and is never written into the repository. +TOKEN_PLACEHOLDER = "" +TOKEN_VARIABLE = "GITCLEARED_REVIEW_TOKEN" + + def reviewer_notes() -> str | None: """The reviewer-facing block out of `review-notes.md`. @@ -510,7 +516,11 @@ def reviewer_notes() -> str | None: if not path.exists(): return None parts = path.read_text().split("\n---\n") - return parts[1].strip() if len(parts) > 2 else None + if len(parts) <= 2: + return None + notes = parts[1].strip() + token = os.environ.get(TOKEN_VARIABLE, "").strip() + return notes.replace(TOKEN_PLACEHOLDER, token) if token else notes CONTACT_FILE = STORE / "review-contact.local.json" @@ -546,6 +556,10 @@ def push_review() -> None: notes = reviewer_notes() if notes: contact["notes"] = notes + if TOKEN_PLACEHOLDER in notes: + print(f" note: no review token yet — set {TOKEN_VARIABLE} and run `review` again before submitting") + else: + print(" review token included") for platform, version_id in editable_versions().items(): existing = call( @@ -739,6 +753,17 @@ def submit_for_review() -> None: existing submission for the platform is reused rather than duplicated. """ app = app_id() + + # A reviewer who cannot sign in rejects the build, so refuse to submit while the + # notes App Review will read still carry the placeholder instead of a token. + for platform, version_id in editable_versions().items(): + detail = call("GET", f"/v1/appStoreVersions/{version_id}/appStoreReviewDetail").get("data") + if TOKEN_PLACEHOLDER in ((detail or {}).get("attributes", {}).get("notes") or ""): + sys.exit( + f"{platform}: the review notes still have the token placeholder. " + f"Set {TOKEN_VARIABLE} and run `Scripts/asc.py review` first." + ) + existing = { item["attributes"]["platform"]: item for item in call( diff --git a/Sources/Cleared/GitHub/GitHubAPI.swift b/Sources/Cleared/GitHub/GitHubAPI.swift index 8e4e729..2dd46a7 100644 --- a/Sources/Cleared/GitHub/GitHubAPI.swift +++ b/Sources/Cleared/GitHub/GitHubAPI.swift @@ -144,6 +144,7 @@ extension GitHubClient { let mergeCommitAllowed: Bool let rebaseMergeAllowed: Bool let viewerDefaultMergeMethod: String? + let viewerPermission: String? } struct Commits: Decodable, Sendable { struct CommitNode: Decodable, Sendable { @@ -184,6 +185,7 @@ extension GitHubClient { repository { name owner { login } squashMergeAllowed mergeCommitAllowed rebaseMergeAllowed viewerDefaultMergeMethod + viewerPermission } commits(last: 1) { nodes { commit { statusCheckRollup { state } } } } } @@ -233,11 +235,24 @@ extension GitHubClient { reviewDecision: ReviewDecision(graphQL: node.reviewDecision), isInMergeQueue: node.isInMergeQueue ?? false, allowedMethods: allowed, - repoDefaultMethod: MergeMethod(graphQL: repository.viewerDefaultMergeMethod) + repoDefaultMethod: MergeMethod(graphQL: repository.viewerDefaultMergeMethod), + viewerCanMerge: Self.canMerge(permission: repository.viewerPermission) ) } } + /// Whether a repository permission lets you merge. A pull request you + /// opened against someone else's project carries your name but not your + /// write access, and GitHub refuses the merge. Only an explicit read-level + /// answer withholds the button; a missing one is not treated as a no. + nonisolated static func canMerge(permission: String?) -> Bool? { + switch permission { + case "ADMIN", "MAINTAIN", "WRITE": true + case "READ", "TRIAGE": false + default: nil + } + } + /// Merges a pull request at the commit Cleared last saw. /// /// Sending the head SHA is what makes one click safe: if someone pushed diff --git a/Sources/Cleared/Models/Core.swift b/Sources/Cleared/Models/Core.swift index cbff0bb..e05789e 100644 --- a/Sources/Cleared/Models/Core.swift +++ b/Sources/Cleared/Models/Core.swift @@ -127,6 +127,10 @@ struct PullRequestSummary: Identifiable, Sendable, Hashable { /// Merge methods the repository permits. Empty when unknown. let allowedMethods: [MergeMethod] let repoDefaultMethod: MergeMethod? + /// Whether this account may merge into the repository at all. False for a + /// pull request to someone else's project, where only its maintainers can + /// merge. Nil when GitHub did not say. + let viewerCanMerge: Bool? init( repo: RepoRef, @@ -143,7 +147,8 @@ struct PullRequestSummary: Identifiable, Sendable, Hashable { reviewDecision: ReviewDecision? = nil, isInMergeQueue: Bool = false, allowedMethods: [MergeMethod] = [], - repoDefaultMethod: MergeMethod? = nil + repoDefaultMethod: MergeMethod? = nil, + viewerCanMerge: Bool? = nil ) { self.repo = repo self.number = number @@ -160,6 +165,7 @@ struct PullRequestSummary: Identifiable, Sendable, Hashable { self.isInMergeQueue = isInMergeQueue self.allowedMethods = allowedMethods self.repoDefaultMethod = repoDefaultMethod + self.viewerCanMerge = viewerCanMerge } var id: String { "\(repo.id)#\(number)" } @@ -184,6 +190,11 @@ struct PullRequestSummary: Identifiable, Sendable, Hashable { case .reviewRequired: return "needs review" case .approved, nil: break } + // After checks and reviews, which are still worth knowing about a pull + // request to someone else's project; before GitHub's own merge verdict, + // which it may not have worked out for a repository you cannot push to + // and which would not change the answer. + if viewerCanMerge == false { return "no write access" } switch mergeState { case .behind: return "behind base branch" case .blocked: return "blocked by branch rules" diff --git a/Store/README.md b/Store/README.md index d174925..9bae5eb 100644 --- a/Store/README.md +++ b/Store/README.md @@ -49,16 +49,19 @@ reason. Changing course is cheap: the name comes from `PRODUCT_NAME` in wordmark on the screenshots, which `make screenshots` redraws. **2. The reviewer needs a way in.** GitCleared does nothing until it has a GitHub -token, so App Review has to be given one. `metadata/review-notes.md` has a -placeholder on its TOKEN line and the steps for making a suitable token: a -throwaway account, one repository, a fine-grained token scoped to it. Submitting -with the placeholder still in place gets a rejection for "unable to sign in". +token, so App Review is given one: the developer's own, limited to the demo +repository `hollandcloud/gitcleared-demo`, which holds a deployment waiting for +approval and a pull request ready to merge. `metadata/review-notes.md` has the +token's exact settings and the steps before each submission. The token is filled +in at push time from `GITCLEARED_REVIEW_TOKEN` and never written into this +repository, and `Scripts/asc.py submit` refuses while the placeholder is still in +App Store Connect. ## What still needs you | Thing | Why it is not done | |---|---| -| The two decisions above | They are yours. | +| The review token | It has to be made on GitHub by its owner. See `metadata/review-notes.md`. | | App Privacy answers | The questionnaire is browser-only. `metadata/app-privacy.md` has the answer and the reasoning. | | Price set to Free | The description says free and open source. The pricing page has to agree. | | Submitting for review | Deliberately left to a person. `Scripts/asc.py submit` exists for when you are ready. | diff --git a/Store/metadata/review-notes.md b/Store/metadata/review-notes.md index 4329662..0c3007b 100644 --- a/Store/metadata/review-notes.md +++ b/Store/metadata/review-notes.md @@ -1,10 +1,10 @@ # App Review notes -Paste the block below into **App Review Information → Notes**, or run -`Scripts/asc.py review`, which sends it. The demo account fields stay empty: -there is no username and password. The app signs in with a GitHub token, which -goes in the notes — **replace the placeholder on the TOKEN line before -submitting** (see "What still needs you" in `Store/README.md`). +The block between the `---` fences is what App Review reads. `Scripts/asc.py +review` sends it, with the token filled in from the `GITCLEARED_REVIEW_TOKEN` +environment variable. The token is never written into this file. The demo +account fields stay empty: the app signs in with a GitHub token, not a username +and password. --- @@ -13,30 +13,34 @@ launch, look at the right-hand side of the menu bar for its icon and click it to open the panel. SIGNING IN. The app works against a GitHub account, so it needs a GitHub -personal access token. One for a test account is below. On first launch the -panel shows "Connect with a personal access token": paste the token into the -field and click Connect. +personal access token. The token below belongs to the developer and is limited +to one demo repository, github.com/hollandcloud/gitcleared-demo, which exists +for this review. On first launch the panel shows "Connect with a personal +access token": paste the token into the field and click Connect. TOKEN: -WHAT YOU SHOULD SEE. Once connected, the panel lists what is waiting on that -account, in this order: +WHAT YOU SHOULD SEE. Within a few seconds of connecting: -• Holding — a deployment waiting at an environment gate, with a Clear button. - Clicking Clear approves the deployment on GitHub. -• Ready to merge — an open pull request whose checks have passed, with a Merge - button. -• In flight — open pull requests that cannot be merged yet, each with the - reason. -• Failed — recent failed workflow runs, each with a Re-run button. +• Holding — "gitcleared-demo · Deploy", waiting at the production environment. + Click Clear to approve it; the deployment then runs on GitHub (it only prints + a line, nothing is really deployed) and leaves the list. +• Ready to merge — "Say when the status was last checked", with a Merge button. + Merging it merges that pull request on GitHub. +• In flight — pull requests that cannot be merged yet, each with the reason: + a draft in the demo repository, and possibly a pull request the developer has + open on a public open-source project, shown without a Merge button. -If the test account has nothing waiting, the panel shows "All clear". That is -the normal resting state, not an error. +Click any row to open it on GitHub. Right-click a row for Copy Link, and on a +held deployment, Reject. -OTHER CONTROLS, along the bottom of the panel: refresh, start a workflow (the -play icon — choose a repository, then a workflow), Settings (the gear, or -Command-comma), and Quit (the power icon, or Command-Q). Right-clicking a row -offers Open on GitHub and Copy Link, and on a held deployment, Reject. +START A WORKFLOW. Click the play icon at the bottom of the panel, choose +hollandcloud/gitcleared-demo, then "Deploy". The sheet shows the inputs the +workflow declares. Run starts it on GitHub; it then waits at production and +appears under Holding. + +OTHER CONTROLS, along the bottom of the panel: refresh, Settings (the gear, or +Command-comma), and Quit (the power icon, or Command-Q). NOTIFICATIONS. The app asks for notification permission after connecting. It uses them only to say that a deployment has started holding. Declining changes @@ -54,17 +58,42 @@ analytics and no advertising. --- -## What the reviewer needs that only the owner can provide +## Before submitting + +**1. Make the review token** — on GitHub, as `hollandcloud`: +https://github.com/settings/personal-access-tokens/new + +| Setting | Value | +|---|---| +| Token name | GitCleared App Review | +| Expiration | 90 days — long enough to outlast a review and a resubmission | +| Resource owner | hollandcloud | +| Repository access | Only select repositories → `gitcleared-demo` | +| Actions | Read and write | +| Deployments | Read and write | +| Contents | Read and write | +| Pull requests | Read-only | + +Store it in Infisical. It is the only copy outside GitHub. + +**2. Put a deployment on hold.** Approving the held one uses it up, and GitHub +drops a waiting run after 30 days: + +```sh +gh workflow run deploy.yml -R hollandcloud/gitcleared-demo +``` + +Run it again after any review that cleared the last one, and before a +resubmission. + +**3. Send the notes with the token, then submit:** -A token. GitHub tokens cannot be created through the API, and one for a -personal account should not be handed to App Review. The practical route: +```sh +GITCLEARED_REVIEW_TOKEN= Scripts/asc.py review +Scripts/asc.py submit # refuses while the notes still have the placeholder +``` -1. Create a throwaway GitHub account for review, or use an existing test one. -2. Give it one repository with an environment that requires that account as a - reviewer, a workflow that deploys to it, and an open pull request with a - passing check — so there is something Holding and something Ready to merge. -3. Create a fine-grained token scoped to that repository only, with Actions, - Deployments, Contents (read and write) and Pull requests (read). -4. Put it on the TOKEN line above and run `Scripts/asc.py review`. +If the reviewer merges the ready pull request, open another one in the demo +repository before resubmitting, so there is still something to merge. -Revoke the token once the review is over. +Revoke the token once the app is approved. diff --git a/Tests/ClearedTests/ClearedTests.swift b/Tests/ClearedTests/ClearedTests.swift index 45d17f8..f93a2a0 100644 --- a/Tests/ClearedTests/ClearedTests.swift +++ b/Tests/ClearedTests/ClearedTests.swift @@ -28,7 +28,8 @@ struct MergeReadinessTests { review: ReviewDecision? = nil, queued: Bool = false, allowed: [MergeMethod] = [], - repoDefault: MergeMethod? = nil + repoDefault: MergeMethod? = nil, + canMerge: Bool? = nil ) -> PullRequestSummary { PullRequestSummary( repo: RepoRef(owner: "o", name: "r"), @@ -44,7 +45,8 @@ struct MergeReadinessTests { reviewDecision: review, isInMergeQueue: queued, allowedMethods: allowed, - repoDefaultMethod: repoDefault + repoDefaultMethod: repoDefault, + viewerCanMerge: canMerge ) } @@ -99,6 +101,31 @@ struct MergeReadinessTests { #expect(pr(state: state, review: .approved).isReadyToMerge) } + @Test("a pull request to a project you only contribute to has no merge button") + func noWriteAccess() { + let contribution = pr(canMerge: false) + #expect(!contribution.isReadyToMerge) + #expect(contribution.blockReason == "no write access") + // Failing checks are still the more useful thing to say... + #expect(pr(checks: .failure, canMerge: false).blockReason == "checks failed") + // ...but not "still checking": GitHub's verdict would not change the answer. + #expect(pr(mergeable: "unknown", state: .unknown, canMerge: false).blockReason == "no write access") + } + + @Test("write access, or no word on it, leaves a green pull request ready", arguments: [true, nil] as [Bool?]) + func writeAccessOrUnknown(canMerge: Bool?) { + #expect(pr(canMerge: canMerge).isReadyToMerge) + } + + @Test("GitHub's permission levels map onto merging", arguments: [ + ("ADMIN", true as Bool?), ("MAINTAIN", true), ("WRITE", true), + ("TRIAGE", false), ("READ", false), ("NONE", nil), + ]) + func permissionLevels(level: String, expected: Bool?) { + #expect(GitHubClient.canMerge(permission: level) == expected) + #expect(GitHubClient.canMerge(permission: nil) == nil) + } + @Test("a pull request already in the merge queue has no merge button") func queuedIsNotOffered() { #expect(pr(queued: true).blockReason == "in merge queue") diff --git a/Tests/ClearedTests/GitHubClientTests.swift b/Tests/ClearedTests/GitHubClientTests.swift index 000d8c4..c0f6eb3 100644 --- a/Tests/ClearedTests/GitHubClientTests.swift +++ b/Tests/ClearedTests/GitHubClientTests.swift @@ -305,6 +305,8 @@ struct QueueReadingTests { Fixture.pullRequest(number: 1), Fixture.pullRequest(number: 2, mergeState: "BLOCKED", review: "REVIEW_REQUIRED"), Fixture.pullRequest(number: 3, mergeCommit: false, rebase: false), + Fixture.pullRequest(number: 4, permission: "READ"), + Fixture.pullRequest(number: 5, permission: nil), "{}", "null", ])) @@ -313,12 +315,14 @@ struct QueueReadingTests { let prs = try await client.myOpenPullRequests() - #expect(prs.map(\.number) == [1, 2, 3]) + #expect(prs.map(\.number) == [1, 2, 3, 4, 5]) #expect(prs[0].isReadyToMerge) #expect(prs[0].headSHA == "abc1") #expect(prs[1].blockReason == "needs review") #expect(prs[2].allowedMethods == [.squash]) #expect(prs[2].mergeMethod(preferring: .merge) == .squash) + #expect(prs[3].blockReason == "no write access") + #expect(prs[4].isReadyToMerge) } @Test("only enabled workflows with a manual trigger can be started") diff --git a/Tests/ClearedTests/StubGitHub.swift b/Tests/ClearedTests/StubGitHub.swift index ed93dc9..3076c0c 100644 --- a/Tests/ClearedTests/StubGitHub.swift +++ b/Tests/ClearedTests/StubGitHub.swift @@ -177,7 +177,8 @@ enum Fixture { squash: Bool = true, mergeCommit: Bool = true, rebase: Bool = true, - defaultMethod: String = "SQUASH" + defaultMethod: String = "SQUASH", + permission: String? = "ADMIN" ) -> String { """ {"number": \(number), "title": "feat: thing \(number)", "url": "https://github.com/acme/web/pull/\(number)", @@ -186,7 +187,8 @@ enum Fixture { "reviewDecision": \(review.map { "\"\($0)\"" } ?? "null"), "isInMergeQueue": false, "repository": {"name": "web", "owner": {"login": "acme"}, "squashMergeAllowed": \(squash), "mergeCommitAllowed": \(mergeCommit), "rebaseMergeAllowed": \(rebase), - "viewerDefaultMergeMethod": "\(defaultMethod)"}, + "viewerDefaultMergeMethod": "\(defaultMethod)", + "viewerPermission": \(permission.map { "\"\($0)\"" } ?? "null")}, "commits": {"nodes": [{"commit": {"statusCheckRollup": \(rollup.map { "{\"state\": \"\($0)\"}" } ?? "null")}}]}} """ } diff --git a/project.yml b/project.yml index d7d352b..a646405 100644 --- a/project.yml +++ b/project.yml @@ -25,8 +25,8 @@ settings: base: SWIFT_VERSION: "6.0" SWIFT_STRICT_CONCURRENCY: complete - MARKETING_VERSION: "0.4.1" - CURRENT_PROJECT_VERSION: "4" + MARKETING_VERSION: "0.4.2" + CURRENT_PROJECT_VERSION: "5" # Ad-hoc by default so anyone can clone and build with no Apple account. # Override both in Local.xcconfig (gitignored) for a stable signature that # keeps the Keychain entry valid across rebuilds — see Local.xcconfig.example.