No Merge button on pull requests you cannot merge; review token at push time

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

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
 

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 = "<replace with a token for the review account before submitting>"
+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(

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

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"

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. |

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: <replace with a token for the review account before submitting>
 
-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=<the 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.

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")

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")