Skip to content

fix(server): name the asking project for a pull request read through another checkout - #20728

Open
only21mil wants to merge 3 commits into
pingdotgg:mainfrom
only21mil:fix/16455-borrowed-read-target
Open

only21mil wants to merge 3 commits into
pingdotgg:mainfrom
only21mil:fix/16455-borrowed-read-target

Conversation

@only21mil

@only21mil only21mil commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Part of #16455

Problem

A pull request stored under a repository name no checkout matches (for example old-org/app after a move) is read through the first project on that host. detail, summary and preview then name that lending project, so a handoff checks out there instead of in the project the user asked from, and the persisted read is keyed by the lender.

Change

When the asking project is supported on the reference's host, a borrowed route records it as checkout. detail, summary, preview, the canonical reference and the persisted read key take the asking project; provider reads and credentials still use the lender. A stack action refreshes both projects, since each can hold its own cached read. With no supported asking checkout on that host, the lender stays the target, as before. #23368 carries the separate checkout guard. Credit to @Absterrg0, whose closed #16514 first served such a reference from the requesting project.

Scope and approval

Part of #16455. The triage confirms that the borrowed project is presented as the pull request's project and lists options that keep borrowed reads without using that project as the checkout target; the reporter suggested the same. This PR names the asking project; the guard in #23368 checks that it is a checkout of the repository. The triage leaves the fix direction to a maintainer, so it does not approve this choice.

This PR moves the handoff only where the request names the asking project, as the Pull Requests page does when its URL names that project and the stored, old repository name. From the code, the sidebar link and the thread-details pull request row pick the project on the client before this code runs (findProjectOnChangeRequestHost in ThreadDetailsPrRows.tsx), so those routes still target the lending project. When the asking project is unrelated as well, this PR alone still checks the pull request out there; #23368 refuses that case.

This splits the closed #18278 and carries its borrowed-read change with the same code. Its guard half, #20727, was closed in favour of #23368. This PR does not change which checkouts a guard accepts.

Verification

Linux x64, real github.com read with a read-only token. zeit/ms#170 is a real open pull request; that repository is now vercel/ms. Projects: lender (first on the host, a clone of an unrelated repository) and app (a clone of vercel/ms). Revisions: main 32882540fb, this head 4438db4891, and this head merged locally with that main (015be2f96a, not pushed). Each run opens Pull requests at /pull-requests?projectId=<app>&selectedProjectId=<app>&selectedHost=github.com&repository=zeit%2Fms&number=170, then Check out, In a separate worktree.

  • Web app in dev mode, clicks in the Browser panel.
  • Linux desktop app (vp run dev:desktop): the route set and the clicks sent through the debugging port.

I kept separate captures for web and Linux desktop so main, this head and the local merge can be compared.

Web, main 32882540fb: the draft opens in lender.

Web on main: lender draft after Check out

Web, this head 4438db4891: the draft opens in app with #170.

Web on this head: app draft with #170 after Check out

Web, the merge 015be2f96a: the draft opens in app with #170.

Web on the merge: app draft with #170 after Check out

Linux desktop, main 32882540fb: the draft opens in lender.

Desktop on main: lender draft after Check out

Linux desktop, this head 4438db4891: the draft opens in app.

Desktop on this head: app draft after Check out

Linux desktop, the merge 015be2f96a: the draft opens in app.

Desktop on the merge: app draft after Check out

Worktree listings and the unit run

Worktrees after Check out in the desktop runs (clean clones each run; the web runs agree):

$ git -C /home/tester/lender worktree list
/home/tester/lender                                                                      7fd1a60 [master]
/mnt/scratch/t3home-desk-main2/worktrees/lender/t3code-pr-170-feature-o-44b97de70871335a afced27 [t3code/pr-170/feature/optional-rounding]
$ git -C /home/tester/app worktree list
/home/tester/app 4ff48ce [main]
$ git -C /home/tester/lender worktree list
/home/tester/lender 7fd1a60 [master]
$ git -C /home/tester/app worktree list
/home/tester/app                                                                    4ff48ce [main]
/mnt/scratch/t3home-desk-head/worktrees/app/t3code-pr-170-feature-optional-rounding afced27 [t3code/pr-170/feature/optional-rounding]
$ git -C /home/tester/lender worktree list
/home/tester/lender 7fd1a60 [master]
$ git -C /home/tester/app worktree list
/home/tester/app                                                                       4ff48ce [main]
/mnt/scratch/t3home-desk-merged/worktrees/app/t3code-pr-170-feature-o-027cc472cb842ad0 afced27 [t3code/pr-170/feature/optional-rounding]

vp test run src/pullRequest/PullRequestService.test.ts in apps/server. On 015be2f96a with main's PullRequestService.ts (this PR's tests, main's code):

   × names the requesting project, not the one lending its credentials, for a pull request no checkout names 32ms
   × keeps each requesting project's checkout when two projects read one borrowed pull request 33ms
   × refreshes a borrowed pull request after a turn in the project that asked for it 35ms
   × refreshes the asking project's sibling layers after a borrowed stack action fails 27ms
      Tests  4 failed | 118 passed (122)

On 015be2f96a: Tests 122 passed (122). On 4438db4891: Tests 118 passed (118).

Earlier, against a local GitHub Enterprise stand-in at main d756fed7aa and this head: a stack rebase that fails at a layer's push refreshes that pull request's read in both projects. A viewed-files mark made from the asking project can show in the lending project only after the 15 s viewed-files cache lifetime, as two checkouts of one repository already do on main.

Not checked:

  • macOS and Windows desktop.
  • Mobile: from the code, its pull request checkout runs in the project its new-task draft names (threadOutboxDelivery.ts), not the one a detail read returns.
  • A real stack merge failure on github.com (only the stand-in above).
  • Sidebar and thread-details routes, which send the lending project before this code runs (unchanged here).
  • Asking checkouts cloned through ssh.github.com or another SSH host (lender fallback, from the code).

Generated with Claude Opus 5.5 in T3 Code Nightly

…another checkout

When no checkout names a hosted pull request, the read goes through
another project on the host, and detail, summary and preview named that
lending project. A hand-off then checked the pull request out in a
project that only lent its credentials.

They now name the asking project when it is supported on the same host.
The canonical reference and the persisted read key follow it, so two
asking projects keep their own targets and a turn in the asking project
refreshes its read. Recovery after a stack action refreshes the asking
project and the lending project. Reads keep the lending project's
checkout and credentials.

Co-authored-by: Absterrg0 <148704891+Absterrg0@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Oct 17, 2026
@github-actions github-actions Bot added the size:S 10-29 changed lines (additions + deletions). label Oct 17, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 2a55b2a

Macroscope's review found this PR approvable — This is a focused server-side bug fix that separates the asking project’s checkout/cache identity from the lending project’s credentials without changing provider contracts or defaults. Regression tests cover the corrected handoff, cache refresh behavior, and existing fallback cases.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Borrowed pull-request reads use the requesting project’s checkout identity for returned metadata and persisted-read cache keys when a matching checkout exists. Stack actions refresh summaries for both the requesting and credential-lending projects.

Changes: Borrowed Pull-Request Context

Layer / File(s) Summary
Resolve checkout context
apps/server/src/pullRequest/PullRequestService.ts, apps/server/src/pullRequest/PullRequestService.test.ts
Routes record a matching checkout project. Summary, detail, and preview results use its identity when available; detail results also use its title and workspace root. Tests cover borrowed-read identities and cases where no matching checkout exists.
Scope borrowed-read cache
apps/server/src/pullRequest/PullRequestService.ts, apps/server/src/pullRequest/PullRequestService.test.ts
Persisted-read cache keys use the checkout project ID when available. Tests cover cache reuse and refresh after a turn in the requesting project.
Refresh summaries after stack actions
apps/server/src/pullRequest/PullRequestService.ts, apps/server/src/pullRequest/PullRequestService.test.ts
Stack actions refresh summaries for both the credentials project and checkout project when their IDs differ. Tests cover a failed borrowed stack action.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related issues

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 4438d

Borrowed pull-request reads now name the asking project while credentials stay with the lending project. No concrete merge-blocking risk was identified in the supplied change. Viewed-file invalidation and platform-specific behavior were not verified by the author.

Security architecture risk: Low · up to 2a55b

The change directs handoffs to the requesting project instead of the project lending credentials. Provider actions retain their existing permission checks and credential context. Remaining uncertainty concerns cache freshness across project-routing changes, rather than a demonstrated authorization bypass.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The traced effect spans the requesting project's thread and working tree, plus the remote pull request accessed through existing lender credentials. Handoff requires an explicit action; merely reading detail does not invoke checkout. Checkout preparation can run the selected project's setup script, making correct workspace identity security-relevant.

Security Findings and Attack Paths

  • inferred — No introduced provider-authorization bypass was demonstrated in the inspected path. Although returned checkout metadata changes, the web action runner submits the original reference, and the server resolves provider ownership and requests permissions rather than treating cached detail as an authorization grant.

Trust Boundaries and Controls

  • observed — Provider reads and actions continue using the lender's workspace and resolved repository. Existing expected-account verification also uses the lender credential context. Credential borrowing is pre-existing behavior, not authority introduced by the new checkout field.

Resilience and Maintainability Implications

  • inferred — Non-stack mutation invalidation follows the canonical requesting-project reference, so a separately cached lender view can remain stale. This is a cache-consistency limitation, not a demonstrated permission bypass: action permission lookup calls the provider independently. Persistent entries expire after 60 seconds, although held summary behavior means that expiry is not a universal display-freshness guarantee.
Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly describes the borrowed-read project identity change.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the template's main sections (Problem, Change, Scope and approval, Verification) and is detailed. It explains that a pull request stored under an unmatched repository name is r…
Full details: Description check

Explanation

The description follows the template's main sections (Problem, Change, Scope and approval, Verification) and is detailed. It explains that a pull request stored under an unmatched repository name is read through the first project on the host, so checkouts land in the lending project. The Change section states that a borrowed route records the asking project as checkout, which moves detail, summary, preview, canonical reference, and persisted read key to that project while provider reads and credentials stay with the lender. Scope links issue #16455 and its triage comment, and it states that the triage leaves the fix direction to a maintainer, so it does not claim approval. Verification lists the environment (Linux x64, real github.com read), before/after screenshots for web and Linux desktop, worktree listings, and the unit test run with pass/fail counts. It also lists items not checked: macOS, Windows, mobile, sidebar and thread-details routes, and SSH fallback. The template asks for a conventional commit title in the PR title, but the title is not part of the provided description, so this cannot be checked here. The template also asks for the model and harness used; the description ends with 'Generated with Claude Opus 5.5 in T3 Code Nightly', which satisfies that request. Minor gaps: the description does not give an explicit conventional commit title, and the Scope section does not directly link a triaged bug issue approval comment for this specific fix direction, though it explains why approval is not claimed.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 2a55b2a and 4438db4.

Files selected for processing: 2

  • apps/server/src/pullRequest/PullRequestService.test.ts
  • apps/server/src/pullRequest/PullRequestService.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Resolve the conflict in requireProject: keep main's repositoryKeyFromRemote
for the borrowed route's remote and keep the asking project's checkout.
The new tests call fakePullRequestProvider, which replaced the local
fakeProvider helper on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

maria-rcks commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol and claude-opus-5-5 on behalf of Maria

Verified in a real dev environment

verified unchanged head 18edd14c51a1db7bd7300de44db97e4ff3f85734 on linux, against github/main at 15fe283272. ran one background dev server at a time, Playwright from the repo dependencies, and /usr/bin/google-chrome. projects were added through the app from real github clones. hosting reads used real github; provider turns used codex gpt-6.1-sol. no mocks or seeded fixtures.

  • on main, preview/detail/summary requested from the asking fork returned the lender project and workspace. clicking check out → in a separate worktree opened the lender draft. materializing the branch in that unrelated repository failed.
  • on this head, the same explicit-project pull requests URL returned the asking fork in all three responses. checkout created its worktree, opened its draft, and reported success. a real codex turn there ran pwd and git rev-parse HEAD, confirming the asking project worktree and exact head above.
  • two asking projects retained their own identities for the same borrowed pr. after the real turn, the asking project summary observation advanced from 1793476439327 to 1793476465567, within the 60-second cache lifetime.
  • nearby reads passed: an exact repository match retained its checkout; an asking project on git.cafe reading a github reference retained the github lender fallback.
  • blacksmith passed all 96 existing service tests and vp run check:changed d53873b6a4799e87afd63c8d288ef0089f5a3748 (the recorded pr base): format, lint, both knip passes, graphql, affected typechecks, and tests. no source changes or push were needed.

command and server response transcript

before: main opens the lending project for the asking fork

after: the pr opens the asking fork worktree successfully

real codex turn confirms the worktree and exact pr head

checkout interaction in the running app

Not verified

native mobile, electron, macos/windows, remote/relay connections, other hosting providers, real repository rename redirects, separate github accounts per checkout, viewed-file invalidation, and failed stack-action recovery. no merge, rebase, or other remote mutation was exercised.

this verifies the service behavior when the request names the asking project. the existing sidebar/thread-details link routing can select the lender before reaching this service; this pr does not change that routing. resolve/fix controls were not exercised. the checkout recording uses the explicit-project URL documented in the pr, then the actual app controls.

Merged current main (4438db4)

merged github/main (c52df24d28) with a merge commit. main moved the summary builder into a shared toSummary, so the conflict was resolved by keeping main's helper and applying this PR's (project.checkout ?? project.project).id there. action replies now report the same asking project as summary reads. no other changes. blacksmith: PullRequestService.test.ts 118/118 and vp run check:changed (format, lint, knip, graphql, typecheck, tests) passed.

…ad-target

# Conflicts:
#	apps/server/src/pullRequest/PullRequestService.ts
@only21mil

Copy link
Copy Markdown
Contributor Author

Note

🤖 Opus 5.5 replying on behalf of Victor.

@maria-rcks thanks for the report. We ran both items in the web app at 4438db4891, and merged with main d756fed7aa, against a local GitHub Enterprise stand-in whose git remote refuses the push to a stack's top layer: when Rebase stack fails at #8 after #7's push lands, #7 opened from the asking project and from the lending project both show the new push at once, and a retry gets the same push refusal rather than "the stack changed", the same as on main; a control build that refreshes only the lending project left the asking project on the old read. For viewed files, a mark made from the asking project survives a reload there, but the lending project's view of the same pull request can show the old state for up to the 15 s viewed-files cache lifetime, where main updates it at once; that matches two checkouts of one repository on main, and the description now says so.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants