Skip to content

fix(server): GitHub PR lookups stop probing owner-qualified heads - #13200

Merged
juliusmarminge merged 2 commits into
batch-pr-sync-summariesfrom
github-head-selector-probes
Sep 23, 2026
Merged

juliusmarminge merged 2 commits into
batch-pr-sync-summariesfrom
github-head-selector-probes

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Stacked on #13198.

Problem

For a branch that tracks a fork, or any remote other than origin, GitManager looked up its PR by probing gh pr list --head <owner>:<branch> and --head <remote>:<branch> before the bare branch name. gh pr list does not support that syntax: its help says "<owner>:<branch>" syntax not supported, and the source passes the value straight into GraphQL pullRequests(headRefName:). So both probes always came back empty, and each still cost a GraphQL call.

Tests hid this because the fake gh answered owner:branch keys, which real gh never does.

The open-PR probe on the bare name also used --limit 1. When another fork had a same-named branch (main, fix-typo, …), its PR could take the single slot, matchesBranchHeadContext would reject it, and the real PR went unseen.

Fix

  • On GitHub, findOpenPr and findLatestPrForHeadContext drop selectors containing :. Git branch names cannot contain :, and the bare head branch is always among the selectors, so nothing is lost. matchesBranchHeadContext already checks the owner and repository.
  • Other providers (GitLab, Bitbucket, Azure, Forgejo) parse owner:branch themselves and keep all their selectors.
  • gh pr create --head owner:branch does support that syntax, so preferredHeadSelector is unchanged.
  • GitHub head-branch probes (open and all-state) now read a full page of 100 instead of 1 or 20. That is still one request at the same GraphQL cost, and same-named branches on other forks can no longer crowd out the real match.
  • Test fixtures now answer the bare branch name, as gh does, and the dead owner: decoys are gone.

Estimate

For every status or PR lookup on a fork or non-origin branch:

  • open-PR lookup (stacked action, status refresh): 3 GraphQL calls become 1;
  • all-states lookup (branch PR, status): 3 become 1, or 2 become 1 when the owner and the remote alias are the same.

Branches that track origin in the same repository are unaffected. How much this saves across the fleet depends on how many fork checkouts are open. For PR-review worktrees (t3code/pr-N/... tracking a contributor fork) every refresh cost two or three times what it needed to.

Verification

  • vp test run src/git/GitManager.test.ts: 114 passed.
  • apps/server typecheck is clean.

Made by Claude Opus 5.5 in Claude Code.

🤖 Generated with Claude Code


Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Improved GitHub pull-request lookup when multiple requests share a branch name, helping select the request associated with the correct branch.
    • Avoided treating ambiguous results as matches when identifying details are unavailable.
    • Preserved provider-specific lookup behavior.

`gh pr list --head` filters on the head ref name alone, so `owner:branch`
and `remote:branch` selectors always list nothing while spending a GraphQL
call. Fork lookups on GitHub now query the bare branch and let
matchesBranchHeadContext pick the right fork.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 23, 2026
Comment thread apps/server/src/git/GitManager.ts Outdated
@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 13.5 KiB — 15.1 KiB ✅
Codex Thread snapshot wire — 7.1 KiB — 7.3 KiB ✅
Codex Live turn WebSocket wire — 6.4 KiB — 7.8 KiB ✅
Codex Live turn WebSocket decoded — 56.2 KiB — 66.4 KiB ✅
Codex Live turn messages — 9 — 21 ✅
Claude Total thread wire — 13.5 KiB — 15.1 KiB ✅
Claude Thread snapshot wire — 7.1 KiB — 7.3 KiB ✅
Claude Live turn WebSocket wire — 6.4 KiB — 7.8 KiB ✅
Claude Live turn WebSocket decoded — 57.0 KiB — 66.4 KiB ✅
Claude Live turn messages — 9 — 21 ✅

Baseline: unavailable · PR result: 36211c8 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@macroscopeapp

macroscopeapp Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, well-scoped server-side fix that removes invalid GitHub PR probes, preserves other providers, and expands fork matching coverage with updated tests. A remaining high-severity finding reports that the limit of 10 can still miss a valid PR when many same-named PRs exist.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 6f25f3b2-911d-4997-b5cd-85e9632745a9

📥 Commits

Reviewing files that changed from the base of the PR and between ec1440f and 1f3c140.

📒 Files selected for processing (2)
  • apps/server/src/git/GitManager.test.ts
  • apps/server/src/git/GitManager.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

GitManager filters colon-containing head selectors for GitHub pull-request lookups. Open pull-request lookup requests up to 10 results per selector and matches the returned pull requests against the existing head context. Tests cover bare-branch lookups and matching among same-named branches.

Changes

GitHub pull-request lookup

Layer / File(s) Summary
Filter selectors and match pull requests
apps/server/src/git/GitManager.ts, apps/server/src/git/GitManager.test.ts
GitManager excludes colon-containing selectors for GitHub and requests up to 10 open pull requests per selector. Tests cover bare-branch lookups, returned head matching, and related fork scenarios.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to 1f3c1

No actionable merge-blocking risk remains after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: GitHub PR lookups no longer probe owner-qualified head selectors.
Description check ✅ Passed The description explains the problem, the fix, and verification results. It covers the template’s What Changed and Why sections, and UI details are not applicable to these server changes. The Checklis…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

A bare branch name also matches same-named branches on other forks, so a
limit of 10 could miss the tracked fork's PR. GitHub probes now ask for 100
(one request, same GraphQL cost); other providers keep their limits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge
juliusmarminge merged commit eafb4a9 into main Sep 23, 2026
23 of 25 checks passed
@juliusmarminge
juliusmarminge deleted the github-head-selector-probes branch September 23, 2026 03:50
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 23, 2026
Merges `pingdotgg/t3code` at `aca3c87cd` into the fork — 62 commits from
base
`5a61f50cc`. Landed 339 files (8865+/6131-) against 335 in the upstream
range;
fork delta unchanged at 786 files. The gap of 4 is the three `docs/fork`
files
below plus `ThreadStatusIndicators.test.tsx`, which auto-merged clean
while
still passing the `variant` prop upstream deleted.

`verify.mjs`: all 10 checks pass, tests included.

## Conflicts

Six, each resolved with the verdict `preflight.mjs` printed.

| Path | Verdict | Resolution |
| --- | --- | --- |
| `DiffPanel.tsx` | converged — diff-panel-gates | Upstream rewrote the
scope dropdown as `DropdownMenuRadioGroup` / `DropdownMenuRadioItem`
with the per-turn list in a submenu. Took it whole; re-applied the two
`FEATURES.turnDiffs` gates at their new anchors. |
| `LegacySidebar.tsx` | converged — mobile-touch-upstream-files |
Upstream deleted the add-project button's icon-color className. Took
upstream's button inside the fork's `FEATURES.projectManagement`
wrapper. |
| `ThreadStatusIndicators.tsx` | converged — thread-status-indicators |
Upstream swapped `variant` for a `render` prop and extracted
`PullRequestBadge`. The fork's several-links popover is now a sibling
`PullRequestLinksBadge`; both render a shared `PullRequestBadgeFace`,
extracted so `duplicate-adds.mjs` does not read the shared icon-and-text
span as a merge artifact. |
| `settings/ProjectActionsList.tsx` | unlisted → decide-then-add-entry |
pingdotgg#13029 restyled the Edit button to `variant="ghost-muted"`. Took
upstream's button, kept the `editable` wrapper, added a
`project-actions-list` inventory entry. |
| `settings/ProjectDefaultsSettings.tsx` | converged —
project-defaults-settings | Took upstream whole, re-applied the five
`workspaceOwnsProjectDefaults` gates. Upstream now renders the model and
workspace rows from both a `category === "project"` and a `category ===
"general"` branch, so each gate exists twice. |
| `settings/ProjectSettingsPanel.tsx` | converged —
project-settings-panel | Upstream's new info Alert is unconditional and
first; its new `<ProjectDefaultsSettings category="project" />` section
is gated on `workspaceSettings` as a sibling rather than a fragment,
because folding it in would have re-indented upstream's JSX. |

`pnpm-lock.yaml` auto-merged and was re-derived with `install.mjs`,
which moved
`type-fest` 5.7.0 → 5.10.0 in two msw snapshot blocks and nothing else.
The
fork's `moatless-api` and `mermaid` edges survive.

Sweep: one hit — `apps/server/src/provider/ProviderAuthFlow.ts` and its
test,
new from pingdotgg#12983 on the keyword `auth`. A false positive for the
auth-session
concern (a provider CLI's own sign-in, not the user session), taken
as-is, and
recorded under _Provider setup_ in the gaps because it adds
`provider.auth.respond`. Tripwires steady; no new upstream workflows; no
stale
inventory entries.

Unsupported methods: ADD and DROP both empty, KEEP unchanged at two. No
`rpc.ts` union edits — `provider.auth.respond` took
`ProviderSetupRpcError`,
which already carries `UnsupportedMethodError`.

## Feature classification

### Usable as-is

- **The web component-library pass** — roughly 24 commits
(pingdotgg#12984–pingdotgg#13043) over
`apps/web/src/components/ui/*`: button variants and sizes, dropdown
radio
groups and submenus, popover and tooltip `render` props. Nothing here
touches
the wire; it is the source of four of the six conflicts and all of them
were
  restyles.
- **Settings scope sentence and scope pickers in breadcrumbs** —
`242816af8`
  and `db9a0671b`, including the new `SettingsScopeSentence.tsx`. Reads
  settings the fork already serves.
- **Small web fixes** — `219c1d265`, `6975efd3d`, `68607c5a9`,
`b954af60c`,
  `7c2702d68` + `da6a85b13`, `aff9318bf`, `438bf466f`.
- **Every mobile-only commit**, and the non-targets: `e4422eec7`
(desktop),
  `d7819c188` (device-hub bump, moot while `FEATURES.deviceHub` is off),
  `ca864a25b` / `f25a8e4b7` / `17e34773b` (model manifest).

### Unsupported in Moatless / needs implementation

- **`7e65b226e` feat(auth): share provider sign-in flows and credential
  bindings (pingdotgg#12983).** Adds `provider.auth.respond`
(`WS_METHODS.providerAuthRespond`) and `ProviderAuthRespondInput`,
reshapes
  `provider.auth.start`'s payload from `ProviderSetupInput` to
  `ProviderAuthStartInput`, and adds `methods`, `interaction` and
`credentialOwner` to `ProviderAuthState`. Server-only implementation in
  `apps/server/src/provider/ProviderAuthFlow.ts` and
  `ProviderCredentialStore.ts`.

It makes a provider sign-in interactive — the server asks, the client
answers
through the new method — which is the same shape as the nine
`provider.auth.*`
/ `provider.install.*` methods the backend already refuses. It falls
under the
existing _Provider setup_ gap, now listing ten methods; it needed no
union
edit because it took the same error type as its siblings. Closes if
Moatless
  ever manages provider credentials on the client's behalf.

### Backend behavior to consider reproducing in Moatless

All six are now bullets under _Runtime fixes upstream made to its own
server_
in `docs/fork/gaps.md`, with the closing condition on each. Four are
GitHub
quota work.

- **`96c4bfa0a` provider compatibility advisory** (pingdotgg#13130,
`apps/server/src/provider/providerCompatibility.ts`) — a per-driver
policy on
the model manifest yields a `supported` / `unsupported` / `broken`
advisory on
the published `ServerProvider`, rendered above the composer. Moatless
installs
  the provider CLIs, so it is the side that knows the version.
- **`f193a6863` bypass owned caches on an explicit provider refresh**
(pingdotgg#13109) —
splits `server.refreshProviders` by its existing `refreshModels` flag: a
user
refresh force-refreshes the model manifest and version cache, background
polls
  keep their timers. The flag is already on the contract.
- **`eafb4a934` GitHub PR lookups stop probing owner-qualified heads**
(pingdotgg#13200)
— `gh pr list --head` answers an `owner:branch` selector with nothing
while
still spending a GraphQL call; upstream drops those selectors and widens
the
  remaining probe to 100, which GitHub prices like `first:1`.
- **`18de6bb32` batched PR summary reads** (pingdotgg#13198) — summaries batch
behind a
10ms request window and a per-batch GraphQL query, with the sync
reactor's
  concurrency raised 8 → 25 so a sweep's reads land in the same window.
- **`5975ec78b` `wouldSettle` before an uncached PR re-query** (pingdotgg#13189)
— a
settlement sweep pays for the reused-branch-race lookup only when some
thread
  in the group would actually settle.
- **`f22331240` three-dot PR diffs** (pingdotgg#13170) — `readRangeContext` moved
its
diff stat and patch to `base...HEAD` while leaving the commit log
two-dot, so
a PR description written after the base advanced describes the branch's
own
  changes. One character per command.

## Docs

- `docs/fork/inventory.json` — new `project-actions-list` entry;
  `provider-settings-gates` gained `ProviderModelsSection.tsx` and
  `ProviderSettingsPanel.environment.test.tsx`.
- `docs/fork/gaps.md` — _Provider setup_ names `provider.auth.respond`
as its
tenth method; six new bullets under _Runtime fixes upstream made to its
own
  server_.
- `docs/fork/upstream-merge-log.md` — dated entry.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/5258e642-4bee-4f38-878b-747496b2d2ea
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 23, 2026
## What's Changed
* chore(mobile): drop dead nitro-markdown tgz override and @expo/metro-runtime by @juliusmarminge in pingdotgg/t3code#13148
* feat(web): show settings scope as a sentence at the top of the page by @juliusmarminge in pingdotgg/t3code#13139
* refactor(web): move settings scope pickers into breadcrumbs by @Yash-Singh1 in pingdotgg/t3code#13165
* feat(auth): share provider sign-in flows and credential bindings by @juliusmarminge in pingdotgg/t3code#12983
* refactor(mobile): git sheets use uniwind platform variants instead of className ternaries by @juliusmarminge in pingdotgg/t3code#13161
* chore(mobile): name the two project favicon caches by their job by @juliusmarminge in pingdotgg/t3code#13160
* revert(mobile): git sheets back to Platform.OS ternaries (un-guarded uniwind variants broke both platforms) by @juliusmarminge in pingdotgg/t3code#13169
* docs(mobile): document the two mobile routes that intentionally skip deep links by @juliusmarminge in pingdotgg/t3code#13164
* refactor(mobile): break module cycles with focused extractions by @juliusmarminge in pingdotgg/t3code#13151
* fix(server): generate PR diffs from branch changes by @Yash-Singh1 in pingdotgg/t3code#13170
* fix(web): preserve nested scroll behavior in chat timeline by @Yash-Singh1 in pingdotgg/t3code#13167
* test(web): cover usage model ordering without static markup by @flamboh in pingdotgg/t3code#13104
* fix(desktop): find linuxbrew node for the WSL backend by @CodyRay in pingdotgg/t3code#7827
* chore(models): use GPT-6 Luna for text generation by @extoci in pingdotgg/t3code#13115
* fix(mobile): keep ordinary offline outbox failures out of console.warn by @juliusmarminge in pingdotgg/t3code#13144
* feat(providers): check remote compatibility ranges by @juliusmarminge in pingdotgg/t3code#13130
* chore(lint): keep mobile theme escape-hatch allowlist honest by @juliusmarminge in pingdotgg/t3code#13146
* fix(web): the pull request badge reads at the meta size again by @juliusmarminge in pingdotgg/t3code#13175
* fix(mobile): uniwind platform variants stay guarded on both platforms by @juliusmarminge in pingdotgg/t3code#13172
* refactor(mobile): git sheets use uniwind platform variants instead of className ternaries by @juliusmarminge in pingdotgg/t3code#13185
* refactor(mobile): remaining className platform ternaries become class variants by @juliusmarminge in pingdotgg/t3code#13188
* fix(web): align provider emails without clipping by @Derpedyea in pingdotgg/t3code#13174
* perf(mobile): recycle the default v2 home list and scope the snooze minute tick by @juliusmarminge in pingdotgg/t3code#13149
* refactor(mobile): retire the legacy grouped thread list by @juliusmarminge in pingdotgg/t3code#13183
* fix(server): background PR checks spend less GitHub quota by @juliusmarminge in pingdotgg/t3code#13189
* fix(server): background PR sync reads summaries in batches by @juliusmarminge in pingdotgg/t3code#13198
* fix(server): GitHub PR lookups stop probing owner-qualified heads by @juliusmarminge in pingdotgg/t3code#13200
* chore(mobile): clear the legacy-list deletion fallout by @juliusmarminge in pingdotgg/t3code#13203

## New Contributors
* @CodyRay made their first contribution in pingdotgg/t3code#7827

**Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260922.2123...v0.0.43-nightly.20260923.2135

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260923.2135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant