Open a pull request from the app, without a push credential on disk - #179
Merged
juanmaguitar merged 14 commits intoAug 9, 2026
Merged
Conversation
github-prs.js owned the one HTTP call this app makes to GitHub, and it was GET-only, living in a module about reading pull requests. Opening one (#167) needs POST, a request body and an Authorization header, so the primitive moves to github-http.cjs and grows a method — the alternative was a second, subtly different request function in the module that writes. Behaviour is unchanged for the readers: httpGet is a thin wrapper, and the github-prs suite passes untouched. New: postJson/getJson parse defensively (a proxy's HTML error page must reach the failure classification, not throw past it), the Bearer header exists only when a token is passed, and a token-bearing request refuses redirects outright rather than trusting any stack to not forward a credential cross-host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contributor never types a password into the app and the app never writes a push credential anywhere. It shows a short code; the browser takes that code to github.com; the app polls until GitHub says the authorization went through. Every documented poll state is its own outcome — pending, slow_down (adopting GitHub's interval), expired, declined, canceled, offline — because each calls for something different from the UI, and four of the five are indistinguishable at a glance while arriving minutes apart. The client ID ships in the binary (device flow has no client secret) and belongs to the application under the WordPress organisation; an env override exists for testing against another. Scope is public_repo and nothing wider. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No git shells out and no push credential exists: fork the repository, bring the fork's trunk up to date, upload the changed files as blobs, assemble a tree, commit, branch, open the pull request. The Git Data API rather than the contents endpoint, so a five-file change arrives as the one commit a reviewer expects instead of five with intermediate states that never compiled. The two cases that shaped the design are the ones a contributor day will hit: a fork that already exists and is months stale (the commit is based on the local HEAD, which a stale fork does not contain — syncing is load-bearing, and a diverged fork falls back to its own tip with the result saying so), and a same-named repository that is not a fork at all, refused at step one instead of being written into and failing at the end. pr-files.cjs shapes working-tree entries for the tree API: modes (POSIX reads the executable bit, Windows trusts what HEAD recorded), deletions as null-sha entries, text LF-normalised exactly as the patch renders it so a CRLF checkout does not publish a full-file rewrite — and binaries byte-for-byte, the one thing a unified diff cannot carry. Injected platform/stat/git keep both sides of every split testable from one machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The destination panel (#166) named two places a patch can go and deliberately left out the one that gets automated checks and reviewer attention, because there was no path to it the app could walk. This is that path, opt-in and by device code. The card states the whole ask before anything happens — including what the app cannot do for you — and Not now costs nothing. The token lives in one main- process variable for one app run: never on disk, never logged, never sent to the renderer, which is told a login. Cancel reaches an in-flight sign-in at every await, and a canceled sign-in strands no listener on the bridge. The ticket a pull request cites is read from the site's stored metadata, not taken from the caller. Every failure is typed, names how far it got, and lands on the same floor: the patch file, which is always still there. The flow ends by pointing back at the Trac ticket, because a pull request never replaces one. createMinimalPatchForDir's walk is extracted as collectChangedFiles and shared, so the diff and the pull request cannot disagree about what changed. (The patch's known gap — deletions — is #174; the pull request carries them.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found on the first real run: fork, blobs, tree and commit all succeeded, and
the branch failed with an opaque 404. Forking a repository this size is
asynchronous in a way the readiness check did not cover — the repo metadata
answers 200 almost immediately, and forks share their upstream's object store,
so every object write lands while the fork's own ref database is still
initialising. The first call that needs that database is the branch, at the
very end of the sequence.
Ready now means the fork's own trunk ref answers. The wait loop covers both a
fork this call just made and one from an earlier attempt that is still filling
in — which is exactly the state a contributor retrying after a 'failed' first
try arrives in — and its ceiling is GitHub's documented five-minute worst case
for large repositories. The residual race at branch creation names itself
('your fork is still being set up') instead of saying Not Found, and a
mid-wait 401 or rate limit reports as itself rather than spending the loop.
Tests reproduce the bug: metadata ready, refs 404 — the old code declared the
fork usable; the new code waits, gives up namefully, and never mistakes a
revoked token for a slow fork.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Hand-testing found a real one: on the first run against a fresh fork, every step succeeded except the last — Root cause: forking wordpress-develop is asynchronous in a way the readiness check didn't cover. The repository metadata answers 200 almost immediately, and forks share their upstream's object store — so the base-commit read, every blob, the tree and the commit all succeed against a fork whose own ref database is still initialising. The first call that needs that database is the branch, at the very end. Fixed in the last commit, test-first: ready now means the fork's own |
The second finding from the same hand-test: the fork finished initialising and the branch still 404d. Verified directly — the same ref creation on the same fork succeeds with a repo-scoped token — so the call was fine and the app's token was not: a device-flow sign-in against an application the account has authorized before can reuse the old grant with the old scopes, and GitHub's Git object endpoints accept a token the refs endpoint refuses. Blobs, tree and commit all land; the branch is where it breaks. Two defences. The login endpoints now speak their documented format — URL-encoded parameters — instead of relying on them tolerating JSON, so the scope request itself is out of the realm of undocumented behaviour. And fetchViewer reads what GitHub actually granted from X-OAuth-Scopes: a token that cannot push to a public repository fails the sign-in with the remedy spelled out (revoke the app under Settings → Applications, sign in again) instead of succeeding into a dead end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Second finding from the same hand-test, and the real root cause of the branch 404: the fork had finished initialising and the ref creation still failed — while the identical call on the identical fork succeeds with a Two defences pushed: the |
The third finding from the same hand-test, and the one that explains the other two refusing to die: with a fresh grant whose consent screen said 'read and write all public repository data', the branch still 404d. The endpoint itself says why — POST git/refs answers X-Accepted-OAuth-Scopes: repo, the full scope only, while the object endpoints accept any token at all. Verified on the fork and on an ordinary personal repository alike: public_repo uploads the whole change and dies on the branch, exactly as documented not to. So the app asks for repo, which is more than it will ever touch, and says so in the one place a maintainer will look for the reasoning. The sign-in scope check tightens to match: public_repo is now the trap it exists to catch, not a pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Third finding, and the actual root cause behind the branch 404 surviving a fresh grant: The requested scope is now |
A branch-creation failure here has outlived three theories — fork readiness, grant reuse, scope narrowness — each disproved only by reproducing the call outside the app. The error now describes its own response: HTTP status, the scopes GitHub says the presented token had, and GitHub's request id, which is the handle GitHub support can look up. The next unexplained failure arrives with its evidence attached instead of as a bare Not Found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WP_DEV_ENV_HTTP_LOG=1 prints every GitHub request's method, URL and response status (with GitHub's request id) to the terminal the app was started from. Never the token and never the body. Exists because one failure — branch creation 404ing in the app while the identical call succeeds with the same token outside it — has now outlived every reproduction attempt, and the next fact has to come from watching the app's own traffic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wire log caught it: the branch pointed at a commit parented on the local checkout's HEAD, seventeen commits behind trunk, and git/refs answered 404. Reproduced outside the app with a full-scope token: a freshly API-created commit whose parent is anything but the current tip — even a plainly reachable ancestor — cannot be referenced. And the check that was supposed to catch this could not: forks serve GET git/commits from the shared fork network, so 'does the fork contain the local HEAD' answers 200 for commits the fork has never had. So resolveBase now fast-forwards the fork and bases on its trunk tip, unconditionally, with exact meaning 'the local HEAD is that tip'. The sharp edge this creates is guarded: the tree API replaces whole files, so a stale checkout that touched a file upstream also changed would silently revert other people's work — staleTouchedPaths compares each changed file's blob at the two commits (per file, not via compare, whose 300-file cap hides exactly this) and refuses with the filenames and a pointer at the app's own update-to-trunk flow. The drift notice in the card now says what actually happened — based on today's trunk, none of your files were touched upstream — and the signed-in line names where the fork and branch go, which the card had left implied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Fourth finding, caught by the wire log and this one reproduces cleanly outside the app: The branch now bases on the fork's trunk tip unconditionally, after a fast-forward. The sharp edge that creates — whole-file uploads on a newer trunk silently reverting upstream changes to the same files — is guarded per file: if upstream touched a file the contributor also changed since their checkout's HEAD, the flow refuses with the filenames and points at the app's own update-to-trunk flow. Compare-based checking was rejected for its 300-file cap, which hides exactly the clash being looked for on a checkout a few weeks old. Also from testing feedback: the signed-in line now names where the fork and branch go ( |
Real-run testing found four bugs, and every round trip cost a pull request against wordpress-develop that its watchers could see. Both switches are environment variables, absent in production builds: - WP_DEV_ENV_GITHUB_UPSTREAM=owner/repo points the whole sequence — fork, branch, pull request — at a sandbox repository. Same pattern as the client-ID override. The sandbox needs a trunk branch and cannot belong to the signed-in account, since an account cannot fork its own repository. - WP_DEV_ENV_GITHUB_DRY_RUN=1 stops after the branch. Everything up to there writes only to the contributor's own fork, where a spare branch bothers nobody; the pull request is the one step the upstream's watchers hear about. The card says what happened instead of showing 'pull request #null'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Testing on Windows opened a real pull request against wordpress-develop with the dry-run switch believed to be on. The switches are typed into a terminal minutes before the button is pressed, and the app then looks exactly like a shipped build — so a mode nobody can see is a mode nobody can rely on. The card now carries a TEST MODE strip above the button whenever a switch is set, naming which one: a dry run that stops at the branch, or the sandbox repository pull requests are going to instead of wordpress-develop. The primary button reads 'Push branch (dry run)' in that mode, because a button labelled 'Open pull request' that will not open one is the same lie in smaller type. testMode() returns null in every shipped build — pinned at both the module and the IPC layer, since that null is what keeps the strip off a real contributor's screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mode is reported alongside the login but does not belong to it — it comes from an env switch read once per run. Signing in, signing out and losing an authorization each replaced the account wholesale and took the mode with it, so the banner vanished and the button went back to reading "Open pull request" at the one point it could push something. Fixes #197. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 8, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 9, 2026
Three equal cards said the three destinations were three variations on one choice. They are not: two save a file and stop, leaving the contributor to carry it somewhere, and the third signs them in and pushes on their behalf. A row of identical cards hid that split and made the reader rediscover it by reading all three in full. Trac and the mentor handoff now share one card, separated by a hairline that reads as "another way to do the same kind of thing"; the pull request sits in its own. The gap between cards carries meaning the borders used to spend on nothing. Two lines that #179 had left stale are fixed on the way past: the subtitle promised that nothing is uploaded and no account is asked for, and a comment explained why a pull request could not be offered here. Both predate there being a pull request on this screen. Layout only — no behaviour, and the same controls in the same order. First step toward #186. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 9, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
## Why The patch is what a contributor hands over, so a change it does not mention did not happen as far as anyone reviewing it can tell. It was not saying what happened, in three ways: - **A deleted file fell out of the patch entirely.** Remove a file for your ticket and the diff simply never mentions it, with no warning. Its status row survives the scan, but with no working copy to read, the new side was defaulted to the old one — so the file compared equal to itself and was skipped. - **Both sides were always named `a/<path>` and `b/<path>`.** That filename is the only thing this app's own parser reads an add or a delete from. So a deletion came back as a *modification*, and the applier would have written an empty file where the patch said remove. Additions had the mirror problem and were refused with "is not in this checkout". - **Binary files were dropped in silence** — a patch missing a file the contributor changed, and no way for them to know. Two more surfaced while testing, neither reported, both live before this PR: - **Any patch for a file without a trailing newline failed to re-apply**, modifications included. jsdiff keeps reading past the `\ No newline at end of file` marker, so the blank line this generator put between sections became a phantom context line. - **`git apply` rejected every deletion** — see Risks; this one was found by the review pass *after* the first commit here introduced deletions, and it would have been worse than the bug it fixed. ## What changes Root cause in all of it is one loop, `createMinimalPatchForDir` in `src/main.js`: - **The status matrix decides what happened, not the buffers.** `workdir === 0` is the file being gone; `head` is whether a base side exists. This also keeps "deleted" apart from "present but will not open" — emitting a deletion for the second would tell the next checkout to remove a file nobody removed. - **`/dev/null` names whichever side does not exist**, which is what makes an add an add and a delete a delete to every reader, this app's included. - **The section separator is gone.** Each section already ends in a newline. - **Files the patch cannot carry are named in `#` lines above the diff** — binaries, and files whose contents would not read. That is the placement the mentor-handoff header (#166) already established: `git apply` and `patch` skip leading comment lines, and this app's parser starts at the first `---`. The panel's "is there anything to attach" test moves from comparing against the `No changes.` sentinel to asking whether a diff survives under the commentary (`hasDiffLines` in `src/renderer/diff-highlight.cjs`), since the notice now sits above it. **Deliberately not in this PR:** adding or deleting an *empty* file is still dropped — `a === b === ''` skips before the naming, and jsdiff emits no hunks for it, so representing it needs more than a naming change. Filed as #199 rather than bolted on here. ## How to test this **Platforms:** any. Stacked on #185 — that branch is the base, so this diff is only the patch generator. **Starting state:** a site with a linked ticket, freshly on trunk. 1. Delete a file (say `src/wp-login.php`), edit a second, and create a third. Click **Submit patch**. → All three appear. The deletion reads `--- a/src/wp-login.php` / `+++ /dev/null`; the new file reads `--- /dev/null`. 2. Save that patch, and apply it to a **second** site through **Apply a patch or PR**. → The deleted file is really gone there, the edit landed, the new file exists. 3. The step that matters most, and the one the app cannot do for you: run `git apply --check patch-file` against a real `wordpress-develop` checkout. → Exit 0. This is what a core committer does with a Trac attachment, and it is where the deletion bug in the first commit here showed up. 4. Change only an image in the site, nothing else. Click **Submit patch**. → The panel names the file above the diff — *1 file is not in this patch — a text diff cannot carry binary content* — and offers **no** destination, because there is no diff to attach. **What must not have happened:** - **No patch that applies only partway.** `git apply` is all-or-nothing: if the deletion section is malformed, the contributor's unrelated edits are refused with it, which is worse than the deletion being missing. - **Nothing staged.** Generating a patch still leaves `git status` in the site exactly as it was. - **No file silently absent.** Anything changed and not in the diff must be named in the `#` lines. ## Risks and limitations **Review outcome: 3 `[fix here]` · 2 `[follow-up]` — all 3 fixed.** One was serious enough to name here: the first commit's deletions each carried a `\ No newline at end of file` marker jsdiff invents for the empty new side, which asserts something false about the removed file and makes **`git apply` refuse the entire patch**. It would have shipped a fix that broke more than the bug did. Verified against real `git` in both directions — refused with the marker on a file that ended in a newline, clean without it, and still required when the file genuinely lacked one. - **Empty-file adds and deletes are still dropped** — #199. - **A file present but unreadable is named, not carried.** There is no test for it: provoking it needs a permission or lock state that does not reproduce across macOS and Windows, and the repo's standard rules out a platform-skipped test. - **The binary notice puts a non-ASCII em dash in a plain patch** for the first time. Fine for `git apply` and for Trac's UTF-8, worth knowing if anything downstream guesses encodings. ## Related Closes #85, and #174 with it — the deleted-file half was reported twice. Stacked on #185 → #168, all part of #108's line of work. **Since trunk moved:** #179 introduced `collectChangedFiles`, one walk read by both the `.diff` and the pull request, so this branch's changes moved into that shared shape rather than sitting beside it. It converged well — that walk already reports `inHead`/`inWorkdir` per file, with a comment saying the status codes and not the buffers are what decide whether a file is gone. That is exactly the distinction this fix needed, so the deletion test now reads a field the PR path had already established rather than one invented here. --- <details> <summary>Design decisions and alternatives considered</summary> **Why not emit `diff --git` headers and be conventionally git-shaped?** The app's own parser is documented as reading `createTwoFilesPatch` output without them, and `scanSections` in patch-provenance.cjs keys on their absence. Adding them is a bigger change to the reading side than to the writing side, and `/dev/null` alone is what both `git apply` and this app need to tell an add from a delete. **Why a notice rather than dropping binaries silently, or refusing the patch?** Refusing would make one image block a working text patch. Silence is the bug. Naming them costs three lines and tells the contributor exactly what to attach by hand. **Why strip jsdiff's marker instead of hand-writing deletion sections?** The marker is wrong only in one case — a deletion whose old side ended in a newline — and it is load-bearing in the adjacent case. A targeted strip keeps jsdiff as the single producer of hunk text; hand-rolling the section would put this app in the business of emitting unified diffs by hand for one branch of the loop. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **3 [fix here] · 2 [follow-up] — all 3 fixed.** Judgement pass run in a fresh context per the skill, on the first commit here. | # | Dimension | What was wrong | |---|---|---| | 1 🔴 | Cross-platform | Every deletion claimed the removed file had no trailing newline, and `git apply` refuses the whole patch over it. Fixed by stripping the marker only when jsdiff invented it; both directions verified against real `git`. | | 2 🟡 | Architecture | Filenames were chosen from whether a buffer loaded, so an unreadable base blob retyped an edit as an addition that applies nowhere. They now follow the status matrix, and unreadable files are named above the diff. | | 3 🟡 | Tests | The round-trip test only exercised this app's own applier, which tolerates the bad marker — which is why finding 1 survived a green suite. Now asserted on the bytes, with the adjacent "marker is correct here" case pinned too. | `[follow-up]`: empty-file adds/deletes (#199), and the unreadable-file path having no test (both in Risks). Worth flagging rather than burying: **finding 1 was introduced by this PR**, not inherited. Emitting deletions at all is what exposed it, and only a check against real `git` — not the suite, not the app — caught it. </details> <details> <summary>Implementation notes</summary> Verified by running the code rather than reasoning about it: - jsdiff emits the no-newline marker correctly for additions and modifications; **only** the deletion case is wrong, because the empty new side reads as "no trailing newline". - `git apply --check` on a four-section patch (add, modify, delete-with-newline, delete-without) → exit 0, and applying it produces exactly the expected tree. - `statusMatrix` takes a stat shortcut: a same-size rewrite in the same millisecond reads as unchanged. Two tests here depend on differing file sizes and say so — matching sizes would make them green while testing nothing. `src/renderer/index.js` is gitignored and built by `npm run build:once`, so there is no bundle in the diff. </details> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #167.
The destination panel (#166) named two places a patch can go and deliberately left out the one that gets automated checks and the one reviewers actually watch, because there was no path to it from a
.diffthe app could walk. This adds that path — opt-in, and by device code.What this does
.diffcannot: binary files, and deletions (A deleted file never makes it into the patch #174 tracks the patch side). Text uploads LF-normalised exactly as the patch renders it, so a CRLF checkout neither publishes full-file rewrites nor drags in files the contributor never touched.Prerequisite from the issue
Resolved: the OAuth application is owned by the WordPress organisation. Device flow uses no client secret, so the client ID shipping in the binary distributes nothing secret. An env override (
WP_DEV_ENV_GITHUB_CLIENT_ID) allows testing against another application without a rebuild.Self-review (per AGENTS.md)
The review in
.github/instructions/code-review.instructions.mdran against this branch with the judgement pass in a fresh context: 5 [fix here] · 3 [follow-up], security explicitly clean (the token invariant holds at every point it examined).All five were fixed before this PR: CRLF handling on blob uploads (the 🔴), the same-named-repository refusal, dependency-injected tests for the file-mode platform split, a cancel race during the account lookup, and a preload listener leak on cancelled sign-ins. A redirect guard on token-bearing requests was folded in from the review's open question.
The three follow-ups are filed rather than ignored: #176 (change collection memory), #177 (upload timeout), #178 (title validated only at the last step).
Testing
npm test— 479 pass (63 new across five suites: device-flow poll outcomes including slow_down and post-wait expiry, fork/sync/tree/commit/PR sequence against a routed fake API, file modes on both platforms from one machine, deletion and binary tree entries, bridge listener lifecycles, and the IPC wiring including the cancel race).npm run lintclean.By hand on macOS: sign in (code accepted in browser), Not now, cancel mid-wait, and the signed-out → ready → done card states. The paths that need a real second account — a pre-existing stale fork, a diverged fork — are the ones to exercise on the Buildkite artifact.
🤖 Generated with Claude Code