From 7ad6da88bb1c75ebf195d8203f00174ae0f1aa09 Mon Sep 17 00:00:00 2001 From: hollandcloud Date: Wed, 28 Oct 2026 16:25:07 -0400 Subject: [PATCH 1/2] fix: no Merge button on pull requests you cannot merge The pull request list is every open PR you opened, which includes PRs to projects you only contribute to. When one of those was green and mergeable, GitCleared offered Merge, and GitHub refused it: authoring a pull request does not give you write access to its repository. Read the repository's viewerPermission in the same query. READ or TRIAGE withholds the button with the reason "no write access"; a missing value is treated as unknown and changes nothing. The reason comes after checks and reviews, which are still worth seeing on a contribution, and before GitHub's own merge verdict, which it may not have worked out for a repository you cannot push to. Found while setting up the App Review demo: a token scoped to one demo repository can still read every public repository, so the reviewer would have seen the developer's open PR to a public project, possibly with a Merge button that fails. 0.4.2, build 5. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 +++++++ README.md | 11 ++++---- Sources/Cleared/GitHub/GitHubAPI.swift | 17 +++++++++++- Sources/Cleared/Models/Core.swift | 13 ++++++++- Tests/ClearedTests/ClearedTests.swift | 31 ++++++++++++++++++++-- Tests/ClearedTests/GitHubClientTests.swift | 6 ++++- Tests/ClearedTests/StubGitHub.swift | 6 +++-- project.yml | 4 +-- 8 files changed, 83 insertions(+), 14 deletions(-) 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/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/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. From 24e6e3a14dff76aac6cd34519b94108392e59b13 Mon Sep 17 00:00:00 2001 From: hollandcloud Date: Wed, 28 Oct 2026 16:25:08 -0400 Subject: [PATCH 2/2] feat(asc): fill in the review token at push time; refuse to submit without it The review notes describe the demo repository the reviewer signs in to, hollandcloud/gitcleared-demo, and what they will see there. The token line stays a placeholder in the repository. `asc.py review` substitutes GITCLEARED_REVIEW_TOKEN when it is set, and `asc.py submit` refuses while App Store Connect's copy of the notes still has the placeholder, since a reviewer who cannot sign in rejects the build. The owner's section has the token's exact settings and what to do before each submission: hold a fresh deployment, send the notes with the token, submit. Co-Authored-By: Claude Opus 5.5 --- Scripts/asc.py | 27 +++++++++- Store/README.md | 13 +++-- Store/metadata/review-notes.md | 97 ++++++++++++++++++++++------------ 3 files changed, 97 insertions(+), 40 deletions(-) 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/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.