Skip to content

fix(server): project identity follows the gh base remote so fork PRs link - #130

Merged
NoahHendrickson merged 3 commits into
customfrom
fix/server-repository-identity-base-remote
Sep 9, 2026
Merged

NoahHendrickson merged 3 commits into
customfrom
fix/server-repository-identity-base-remote

Conversation

@NoahHendrickson

Copy link
Copy Markdown
Owner

Problem

Thread cards in the fork's own checkout never show a PR link, while the composer chip shows the PR fine (threads 7db829e5, 6319d3f7). The two read different sources.

Since upstream pingdotgg#10101 (absorbed in the 2026-09-08 sync), the sidebar card renders only the PR the server has stored on the thread. The ThreadPullRequestReactor that stores it refuses any PR whose repository differs from the project's repository identity. The identity resolver always prefers a remote named upstream over origin, so this checkout identifies as pingdotgg/t3code while its PRs live on NoahHendrickson/t3code. The reactor finds the PR, sees a foreign repository, and silently drops it. The database confirms it: zero PR link events ever for the t3code project, dozens for every single-remote project.

The composer chip reads live git status, which asks gh directly and has no identity check, which is why it kept working.

Fix

gh repo set-default writes remote.<name>.gh-resolved = base, and gh pr list already answers from that remote. The resolver now reads the same marker: when a checkout has more than one remote and one carries the base marker, that remote is the identity. Single-remote checkouts, no marker, or a marker naming a missing remote keep upstream's upstream-then-origin order, so every other project resolves exactly as before. The extra git config call runs only for multi-remote checkouts and is cached with the identity.

Fenced as server-repository-identity-base-remote with a manifest entry, three fenced resolver test cases, and a fork guard. Composes with #129, which fixes the step before this one (looking up the right branch); the two touch different files.

Verification

  • RepositoryIdentityResolver.test.ts: 13 passed, including the 3 new cases (marker wins over upstream; stale marker falls back to upstream; single remote makes no config call).
  • Fork guard serverRepositoryIdentityBaseRemote.test.ts: 2 passed.
  • Server tsc --noEmit: 0 errors. Fork lint: no blocking warnings. vp fmt clean.

Claude Fable 5.1 in Claude Code.

🤖 Generated with Claude Code

…link

Upstream's repository identity resolver always prefers a remote named
`upstream` over `origin`. In this fork PRs go to origin, and since upstream
pingdotgg#10101 the thread PR reactor refuses any PR whose repository differs from
the project identity, so no thread card in the fork's own checkout ever got
a PR link while the composer chip kept showing it.

Read the `remote.<name>.gh-resolved = base` marker that `gh repo set-default`
writes, and let that remote be the identity when a checkout has more than
one remote. Single-remote checkouts, no marker, or a stale marker keep
upstream's order. Fenced as server-repository-identity-base-remote with a
manifest entry, resolver tests and a fork guard.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Sep 9, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thermo-nuclear review

Approval bar not met. The identity layer is the right home and the file stays at 230 lines — that's not the problem. The problem is wiring a fork-only gh marker into the shared remote picker instead of returning early from the resolve path.

pickPrimaryRemote had one job: prefer upstream, then origin, then sorted first. This PR gives it a second parameter (unfenced), an unused = null default, a fenced if inside the picker, and a let whose only job is to thread a maybe-name through. That is spaghetti growth plus a merge hazard: every upstream edit of this function now conflicts on a signature line the fence does not own.

The judo is an early return in resolveRepositoryIdentityFromCacheKey. If a marked remote exists, build identity and return. Then call pickPrimaryRemote(remotes) with no second argument. That deletes the parameter, the default, the picker branch, and the mutable let. parseBaseRemoteName stays — that parse is real. Do not extract a new policy file; this is not that kind of complexity.

The fork guard's indexOf check cements the worse shape: it requires remotes.get(baseRemoteName) to appear earlier in the file than ["upstream", "origin"], which is only true because the lookup was stuffed into the picker. After the early-return, that assertion goes red even though the marked remote still wins. Assert the outcome, not source order.

Not findings: file size, putting this in the identity resolver vs the PR reactor, the remotes.size > 1 gate, or parseBaseRemoteName itself.

Open in Web View Automation 

Sent by Cursor Automation: Thermo nuke 4.6

Comment thread apps/server/src/project/RepositoryIdentityResolver.ts Outdated
Comment thread apps/web/src/__fork_guards__/serverRepositoryIdentityBaseRemote.test.ts Outdated
@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.4 KiB −169 B (−1.2%) 15.1 KiB ✅
Codex Thread snapshot wire 7.0 KiB 7.0 KiB −2 B (−0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.6 KiB 6.4 KiB −167 B (−2.5%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 57.1 KiB 56.2 KiB −910 B (−1.6%) 66.4 KiB ✅
Codex Live turn messages 10 8 −2 (−20.0%) 21 ✅
Claude Total thread wire 13.6 KiB 13.6 KiB +13 B (+0.1%) 15.1 KiB ✅
Claude Thread snapshot wire 7.1 KiB 7.1 KiB −2 B (−0.0%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.5 KiB 6.5 KiB +15 B (+0.2%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.8 KiB 57.8 KiB +44 B (+0.1%) 66.4 KiB ✅
Claude Live turn messages 8 9 +1 (+12.5%) 21 ✅

Baseline: da2026e · PR result: 98b5459 · 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.

@NoahHendrickson NoahHendrickson left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review — fix(server): project identity follows the gh base remote so fork PRs link

Verdict: the diagnosis and the mechanism are right; the wiring puts a fork-only concept into an upstream function signature, and one guard assertion is coupled to source order. All checks green.

What I verified

  • Reading remote.<name>.gh-resolved is the correct signal: it is exactly what gh repo set-default writes and what gh pr list resolves from, so the identity and the PR lookup now name the same repository by construction.
  • git config --get-regexp exits 1 when nothing matches, and the code === 0 check handles that as "no base set" rather than an error. Correct.
  • The remotes.size > 1 gate means single-remote checkouts pay nothing, and the identity cache (DEFAULT_POSITIVE_CACHE_TTL = 1 minute) means a gh repo set-default takes effect within a minute — no restart needed. Good.
  • The stale-marker fallback is genuinely exercised (falls back to upstream when the gh base marker names a missing remote), and parseBaseRemoteName's regex is safe for remote names containing dots because .gh-resolved is anchored at the end.

1. pickPrimaryRemote gains a fork-only parameter on an upstream signature

apps/server/src/project/RepositoryIdentityResolver.ts:68

The new second parameter sits outside the fence, so a future upstream edit to this function conflicts on a line no fence owns — and the = null default is dead, since the one call site always passes it. pickPrimaryRemote's job is still "upstream, then origin, then sorted first"; the gh marker isn't part of that.

Cursor suggested an early return in resolveRepositoryIdentityFromCacheKey, which works but duplicates the buildRepositoryIdentity call. Smaller edit that leaves upstream's picker byte-identical and keeps everything inside one fence:

/* fork:begin server-repository-identity-base-remote */
function pickBaseRemote(
  remotes: ReadonlyMap<string, string>,
  baseRemoteName: string | null,
): { readonly remoteName: string; readonly remoteUrl: string } | null {
  if (baseRemoteName === null) return null;
  const remoteUrl = remotes.get(baseRemoteName);
  return remoteUrl ? { remoteName: baseRemoteName, remoteUrl } : null;
}
/* fork:end */

// at the call site:
const remote = pickBaseRemote(remotes, baseRemoteName) ?? pickPrimaryRemote(remotes);

That deletes the parameter, the unused default, and the branch inside the picker, and the stale-marker fallback becomes "return null" rather than a nested if.

2. The guard's indexOf assertion pins source order, not behaviour

apps/web/src/__fork_guards__/serverRepositoryIdentityBaseRemote.test.ts:41

expect(resolver.indexOf("remotes.get(baseRemoteName)")).toBeLessThan(
  resolver.indexOf('["upstream", "origin"] as const'),
);

This only holds because the lookup was stuffed inside pickPrimaryRemote, above the preference array. Under either restructuring above the marked remote still wins and this assertion goes red — a guard that fails on a refactor that preserves the invariant is a guard measuring the wrong thing. The three resolver tests already cover the behaviour (marker wins, stale marker falls back, single remote skips the lookup); I'd drop the indexOf check and keep the remotes.size > 1 / gh-resolved hunk assertions.

3. The marker's other form is silently unhandled — worth a line in the intent

gh repo set-default only writes the literal base when the chosen default repository is one of the checkout's remotes. When it isn't, gh writes remote.<name>.gh-resolved = <owner>/<repo> instead. parseBaseRemoteName matches \s+base$ only, so those checkouts fall back to upstream-then-origin — the same failure mode this PR exists to fix.

I don't think that's fixable here: the identity is built from a remote URL, and in that case there is no remote naming the base repository, so there's nothing to build a locator from. But the manifest intent currently reads as though the marker is honoured in general. One sentence stating the scope ("only the base form; a default repo that isn't a remote keeps upstream's order") would save the next reader the trip through gh's source.

Not findings

File size, choosing the identity resolver over the PR reactor as the home, the remotes.size > 1 gate, and parseBaseRemoteName itself — all fine.

Comment thread apps/server/src/project/RepositoryIdentityResolver.ts Outdated
Move the marker lookup out of pickPrimaryRemote's signature into a fenced
pickBaseRemote Effect.fn, so upstream's picker is byte-identical again and the
call site is a single fallback expression. The guard asserts that shape and
the size gate instead of source order, and the manifest states that only the
literal `base` marker form is honoured.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@NoahHendrickson

Copy link
Copy Markdown
Owner Author

Addressed in 843e5c5:

  • pickPrimaryRemote is byte-identical to upstream again. The marker lookup (size gate, git config call, parse, remote lookup) is one fenced pickBaseRemote Effect.fn and the call site is a single ?? fallback, so the resolve path carries a two-line fence with no let.
  • Guard drops the indexOf order check; it asserts the size gate, the fallback expression, and that upstream's picker signature is unchanged.
  • Manifest intent states that only the literal base marker is honoured, and why the owner/repo form cannot be (finding 3).

RepositoryIdentityResolver.test.ts: 13 passed. Guard green, server tsc clean, fork lint and drift clean.

# Conflicts:
#	.fork/customizations.yaml
@NoahHendrickson
NoahHendrickson merged commit 6a300ec into custom Sep 9, 2026
19 checks passed
@NoahHendrickson
NoahHendrickson deleted the fix/server-repository-identity-base-remote branch September 9, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 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.

2 participants