Repository navigation
fix(board): pin gh calls to origin and surface a card with no PR - #111
Conversation
… call `gh` resolves the base repository itself when no `--repo` is given, and its rule prefers a remote literally named `upstream`. This repository gained that remote during the upstream sync, so every PR lookup since has asked pingdotgg/t3code and truthfully answered "no pull request" — which is how card T3O-46 reached Ready for merge with no PR link and no Merge button. The same unpinned resolution would have pointed `gh pr merge` and `gh pr create` at upstream too. T3 Code now acts on the project's `origin` remote, always — the same default as plain git. The registry already resolves an origin-preferring provider context; the GitHub provider reads its remote URL and names the host-qualified repository on every invocation that takes one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The registry resolves the provider context and binds it into every call, so this is where "a fork asks its own repository, not upstream's" is actually decided. Both new cases fail without the pinning fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The refresh returned void, so a card with no pull request could not tell "we looked and there is none" from "we could not reach the forge" — and the automatic two-minute lookup cache meant asking again inside that window told you nothing new. Give the refresh its own payload with a `force` flag and a result union: linked, none, lookup-failed with the forge's own words, no-branch or unknown-card. Forcing bumps the checkout's PR-lookup epoch, which bypasses the TTL and the per-branch failure backoff together. Automatic callers are unchanged: they omit `force` and ignore the answer. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
A card at Ready for merge with no pull request offered "Move to Done" and nothing else — no link, no Merge button, no word about why. The board face was silent too, so a card could park there indefinitely without anyone noticing. The detail pane now names the branch it could not find a pull request for and offers "Check again", which forces a fresh lookup and reports what came back: a link, still nothing, or the forge's own words when it could not be asked. The card face wears an amber "No PR" chip in the single ranked notice slot, below Conflicts and the auto-merge hold and above the generic attention chip, behind the same settle grace the other amber chips use. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…e upstream debt seams.md gains the T3O-48 marker inventory and the reason behind it, plus a note that upstream's RepositoryIdentityResolver.pickPrimaryRemote has the same origin-vs-upstream bug one level up — which T3O-47 will meet when it adopts apps/server/src/pullRequest/. status-colours.md places No PR in the ranked notice slot. A new user page explains the chip, Check again, and what each of its three answers means. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Move the refresh-outcome helper above the doc block it had wedged itself under, and drop `muted` from the page-side no-pull-request predicate: it is `stage === "done"`, which a merge-role card can never be, so deriving the whole card summary per card per render to read it bought nothing. The card face keeps its own guard, where the summary is already in hand. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
brentkelly
left a comment
brentkelly
left a comment
There was a problem hiding this comment.
High-level summary
This PR fixes the reported bug — a card stuck at "Ready for merge" with no PR link and no Merge button — at its root: every gh invocation now names the project's origin repository via a new fork-owned githubRemote.ts parser and host-qualified --repo/positional args, so gh can no longer resolve the base repo itself and prefer an upstream remote. It also makes board.refreshCardPullRequest return a typed outcome instead of void, adds a forced (cache- and backoff-bypassing) "Check again" control plus a branch-naming empty state in the detail pane, and an amber "No PR" chip on the card face. The fix is well-grounded and well-tested: the origin-over-upstream regression is pinned end-to-end through the real registry→provider→fake-gh path, the force flag's cache-bypass rests on the existing invalidateStatus→bumpPrLookupEpoch mechanism (verified to share the normalized cache key with findBranchPullRequest), the outcome union is exercised across the reactor, RPC and gateway, and the seam markers stay small and greppable. Overall impression is strong. Round 1 found no blocking correctness or security issues; one improvement is worth addressing — a cross-surface inconsistency where the card-face "No PR" chip can appear for a worktree-less merge-stage card while the detail pane, guarded on a worktree, offers no matching explanation or "Check again" affordance — and no nitpicks.
| }): boolean { | ||
| // A Done card needs no guard of its own: it is not at the merge stage, which | ||
| // is what `atMergeStage` already says. | ||
| if (!input.atMergeStage || input.hasPr) return false; |
There was a problem hiding this comment.
Warning
Improvement (1/1): Card-face "No PR" chip fires without the worktree guard the detail pane requires
boardCardNoPullRequest returns true for any merge-stage card with !hasPr (subject to the settle grace), but the detail pane's missing-PR notice in BoardCardDetailView.tsx gates on card.worktree !== null. A merge-stage card with no worktree — a state the reactor itself recognises, since refreshCardPullRequestLink returns { outcome: "no-branch" } for worktree === null, and one reachable by manually parking a never-built card at Ready for merge — therefore wears the amber "No PR" chip whose tooltip reads "open it and check again", yet opening it shows neither the "No pull request found…" line nor the "Check again" button. The affordance the chip advertises does not exist, which is the same "no explanation" dead-end this card set out to remove. (The hasPr-vs-current-round divergence for reopened cards is handled deliberately in the detail pane's comments, so this is specifically the no-worktree case.)
Suggested fix: Add the worktree condition to the predicate so the two surfaces agree, e.g. accept a hasBranch: boolean input and return false when it is false, mirroring card.worktree !== null in the detail pane.
There was a problem hiding this comment.
Fixed in ca63e3e6e — from the pane's side rather than the chip's.
Gating the chip on a branch would need a worktree field on BoardCardShell, which carries none and is held to a per-card byte budget (board.test.ts asserts it). Widening the pane costs nothing and is the better answer anyway: a merge-stage card that never built a branch is the same dead end one step further along, so it deserves an explanation, not silence.
BoardCardDetailView now renders the notice for any un-archived merge-stage card with no current-round PR. With a worktree it reads as before, naming the branch and offering Check again; without one it reads "No pull request, and no branch to look one up on." and offers no button — refreshCardPullRequest answers no-branch for that card, so the button could only repeat the sentence beside it.
Two smaller follow-ons: the chip's tooltip no longer promises a re-check it cannot always offer ("open it to see why"), and docs/user/board-no-pull-request.md covers the branchless shape.
Test: apps/web/src/board/BoardCardDetailView.test.tsx → "says why a merge-stage card with no branch has no pull request". Confirmed failing on a11f35515 (the pane rendered only "Move to Done") and passing on ca63e3e6e. Board web suite: 45 files, 569 tests green; @t3tools/web typecheck clean.
There was a problem hiding this comment.
Note
Adjudication (round 1): fix-upheld
Verified at the line, not from the triage note.
BoardCardDetailView.tsx:1383-1398—missingPullRequestno longer gates oncard.worktree !== null; it computesbranch: card.worktree?.branch ?? nulland withholdsonCheckonly when the worktree is null. The render arm at:1435-1448emits "No pull request, and no branch to look one up on." for the branchless shape, so the chip now opens onto an explanation in both shapes. The finding is answered, even though the fix landed on the pane rather than the predicate.- The stated reason for not taking the suggested
hasBranchgate checks out:BoardCardShell(packages/contracts/src/board.ts:5410) carrieshasPrand no worktree or branch field, under the documented byte budget. So narrowing the chip really would have cost payload. - "the button could only repeat the sentence beside it" also checks out:
refreshCardPullRequestLinkreturns{ outcome: "no-branch" }forworktree === null(apps/server/src/board/supervisorReactor.ts:3791). Withholding the control is correct, not a shortcut.
Proof run rather than taken on trust:
BoardCardDetailView.test.tsx→ "says why a merge-stage card with no branch has no pull request" fails against the pre-fix file (a11f35515) withexpected … to contain 'No pull request, and no branch to look one up on.'and passes atca63e3e6e. It is a real regression test for this finding, not a mirror of the implementation — it asserts the user-visible sentence, the absence ofCheck again, and thatMove to Donestill renders.- Full file: 55 passed.
apps/web/src/board: 45 files, 569 tests passed.
No new issues from the widening: a merge-stage card with a retired-round PR and no worktree reads the same way it already did with a branch, and nothing else consumes missingPullRequest.
The card face's amber `No PR` chip fires on the stage and `hasPr` alone — the shell carries no worktree — while the detail pane's notice and its `Check again` button gated on `card.worktree !== null`. A card parked at Ready for merge that never built a branch therefore wore a chip pointing at a pane that said nothing at all: the same dead end this card set out to remove. The pane now covers that shape too, with the branchless wording and no button, since `refreshCardPullRequest` answers `no-branch` for it and the button could only repeat the sentence beside it. The chip's tooltip stops promising a re-check it cannot always offer. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
brentkelly
left a comment
brentkelly
left a comment
There was a problem hiding this comment.
High-level summary — round 2 (re-review at ca63e3e)
This PR fixes the reported bug — a card at Ready for merge with no PR link and no Merge button — at its root: no gh invocation passed --repo, so gh resolved the base repository itself and its rule prefers a remote named upstream, which in a fork asks the wrong repository and truthfully answers "no pull request". A new fork-owned githubRemote.ts parses the checkout's origin remote (host-qualified, enterprise-aware) and every gh read and write now names that repository, sourced from the registry's already-origin-preferring context; getDefaultBranch is pinned positionally too, which also protects the PR base. On top of that the refresh RPC now returns a typed outcome instead of Schema.Void, a human's "Check again" forces a cache- and backoff-bypassing lookup, the detail pane explains a PR-less merge-stage card (with or without a branch), and the card face wears an amber "No PR" chip in the single ranked notice slot.
On a fresh read the change holds up well. The source-control pinning is mechanical and greppable behind T3o: markers, the origin-over-upstream regression is pinned end-to-end through registry → real provider → fake gh, the outcome union is exercised across every arm, invalidateStatus's PR-cache bypass is covered at the GitManager level, and the reactor forwards force under test. The round-1 finding (chip firing on a card shape the pane could not explain) is resolved and its adjudication upheld. Nothing blocks this round: 0 critical, 0 improvement, 1 nitpick — a narrow test-coverage note on the gateway's force wrapper, non-blocking since both sides of it are already covered.
Status of round-1 findings
| # | Prior finding | Status |
|---|---|---|
| 1 | 🟡 no-pr-chip-worktree-guard — card-face "No PR" chip fired for a worktree-less merge-stage card whose detail pane offered no explanation and no control |
✅ closed — BoardCardDetailView.tsx now renders the missing-PR notice for any un-archived merge-stage card with no current-round PR: branch named + "Check again" when a worktree exists, else "No pull request, and no branch to look one up on." with no button (refreshCardPullRequest answers no-branch there). Chip tooltip softened to "open it to see why"; regression test BoardCardDetailView.test.tsx → "says why a merge-stage card with no branch has no pull request". |
| // epoch, which is part of the cache key — so it bypasses the TTL and | ||
| // the per-key failure backoff together, without either being reachable | ||
| // directly. Already on the service; no new upstream surface. | ||
| (input.force === true ? gitManager.invalidateStatus(input.cwd) : Effect.void) |
There was a problem hiding this comment.
Note
Nitpick (1/1): The gateway's force → invalidateStatus wiring has no direct test
This three-line branch is the mechanism the whole "Check again" feature rests on: force bypasses the two-minute lookup cache and its failure backoff only because it routes through invalidateStatus, which bumps the PR-lookup epoch. The neighbours are covered — cardPullRequest.test.ts proves the reactor forwards force:true to this gateway (it stubs the gateway), and GitManager.test.ts proves invalidateStatus bypasses the PR-lookup cache — but nothing asserts this wrapper actually calls invalidateStatus when force is true. If a refactor dropped that call, "Check again" would silently start returning the cached answer — the exact bug class this card fixes — and every existing test would stay green. Non-blocking, since the gap is narrow and both sides are tested.
Suggested fix: add a focused BoardPullRequestGateway test with a fake GitManager that records calls, asserting invalidateStatus is invoked iff force === true before findBranchPullRequest.
There was a problem hiding this comment.
Fixed in fca5abe9e — taken rather than waved through, since the gap is exactly the kind a refactor closes by accident.
New file apps/server/src/board/BoardPullRequestGateway.test.ts, two cases against a Layer.mock(GitManager) that records call order:
- "invalidates the lookup cache before finding, when forced" — asserts the recorded order is
["invalidateStatus:/repo", "findBranchPullRequest:/repo:board/t3o-48"], so it holds both that the call happens and that it happens first (invalidating after the lookup would still bump the epoch, and still return the stale answer to the user who pressed the button). - "leaves the cache alone on an unforced lookup" — covers
forceomitted andforce: falsetogether, so the bypass cannot quietly become unconditional and turn every automatic card-open refresh into a forge call.
Proof it bites: replacing the ternary at BoardPullRequestGateway.ts:121 with a bare Effect.void fails the first case with the expected diff on calls; restored, both pass. Recording the sequence rather than spying per-method is what buys the ordering assertion, and it matches how GitWorkflowService.test.ts mocks GitManager for the same "was this dependency asked" question.
Checks: apps/server/src/board 70 files / 915 tests passing; t3 typecheck diagnostics byte-identical with and without the new file (the non-zero exit is pre-existing suggestion output across untouched files); vp fmt --check and vp lint clean on it.
"Check again" only reaches the forge because `force` routes through `invalidateStatus`, which bumps the PR-lookup epoch. Both neighbours were covered — the reactor forwards `force`, and `invalidateStatus` bypasses the cache — but nothing held the three lines between them, so dropping that call would quietly restore the cached answer with every test still green. Records the call order against a mocked GitManager: forced lookups invalidate before finding, unforced ones leave the cache alone. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Problem
A card at "Ready for merge" showed no PR link and no Merge button. Every gh PR lookup omitted
--repo, so gh resolved the base repository itself and preferred a remote namedupstream(added during the T3O-46 sync card), truthfully answering "no pull request" against the wrong repo.Fix
sourceControl/githubRemote.tsparses the remote;GitHubSourceControlProviderderives the repository from the registry's origin-preferring context and names it host-qualified--repoon every gh call (reads and writes, includinggetDefaultBranch).board.refreshCardPullRequestgains aforceflag and returns an outcome (linked / none / lookup-failed / no-branch / unknown-card) instead of void.🤖 Generated with Claude Code